Re: [PATCH 1/5] pmdomain: mediatek: Fix power domain count

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 11/03/26 18:29, Ulf Hansson ha scritto:
> On Fri, 6 Mar 2026 at 10:50, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 12/02/26 12:34, Ulf Hansson ha scritto:
>>> On Tue, 10 Feb 2026 at 06:40, Adam Ford <aford173@gmail.com> wrote:
>>>>
>>>> The wrong value of the number of domains is wrong which leads to
>>>> failures when trying to enumerate nested power domains.
>>>>
>>>>    PM: genpd_xlate_onecell: invalid domain index 0
>>>>    PM: genpd_xlate_onecell: invalid domain index 1
>>>>    PM: genpd_xlate_onecell: invalid domain index 3
>>>>    PM: genpd_xlate_onecell: invalid domain index 4
>>>>    PM: genpd_xlate_onecell: invalid domain index 5
>>>>    PM: genpd_xlate_onecell: invalid domain index 13
>>>>    PM: genpd_xlate_onecell: invalid domain index 14
>>>>
>>>> Attempts to use these power domains fail, so fix this by
>>>> using the correct value of calculated power domains.
>>>>
>>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>>
>>> We should have a fixes tag for this too I think:
>>>
>>> Fixes: 88914db077b6 ("pmdomain: mediatek: Add support for Hardware
>>> Voter power domains")
>>>
>>>
>>>> ---
>>>>    drivers/pmdomain/mediatek/mtk-pm-domains.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>> index 58648f4f689b..d2b8d0332951 100644
>>>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>> @@ -1228,7 +1228,7 @@ static int scpsys_probe(struct platform_device *pdev)
>>>>           scpsys->soc_data = soc;
>>>>
>>>>           scpsys->pd_data.domains = scpsys->domains;
>>>> -       scpsys->pd_data.num_domains = soc->num_domains;
>>>> +       scpsys->pd_data.num_domains = num_domains;
>>>
>>> Not sure this is the complete fix, as scpsys_add_one_domain() seems to
>>> be using the wrong value of "num_domains" too, no?
>>>
>>
>> No, scpsys_add_one_domain() uses num_domains from the soc_data for DIRECT_CTL,
>> which is expected. Maybe that can be improved, but it's how it is supposed to work.
>>
>> So yeah, this patch is resolving an issue, and it is a complete fix.
> 
> I had a closer look and unfortunately, it still looks weird to me.
> 
> More precisely, we are allocating and registering a number of genpds,
> that corresponds to the sum of "soc->num_domains +
> soc->num_hwv_domains". The index each genpd gets in the array (struct
> genpd_onecell_data) must correspond to the index specified in DT, for
> both a consumer-node and a child-domain-node, right?
> 
> It seems like adding them dynamically changes the index. In other
> words, the index specified using the "reg" property in a
> child-domain-node, may not match what a consumer-node should specify
> in its "power-domains" property. Isn't that a problem? Perhaps not,
> because we never have both "scpsys_domain_data" and
> "scpsys_hwv_domain_data", but then why are we adding them in ->probe()
> with "soc->num_domains + soc->num_hwv_domains"?
> 

As you understood, having both is not really supported right now, and the
intention there was to have the flexibility to do so without restructuring
the entire thing in the future.

Though, from what I understand, the confusion comes from a single line (and
reading it again, I do agree, because I got confused myself for a moment as
well, and I'm the one who wrote this, so that's bad bad bad):

	num_domains = soc->num_domains + soc->num_hwv_domains;

where, to avoid confusion, this should've been:

	num_domains = soc->num_domains ?
		      soc->num_domains : soc->num_hwv_domains;

(perhaps with a check `if (num_domains && num_hwv_domains) dev_err(only one
  supported)` but being this taken from hardcoded data, I'm not really for it)

...and still, like the proposed fix does:

        scpsys->pd_data.num_domains = num_domains;


While I agree, again, in that the code could be improved (and it shall be),
how the logic behaves, right now, with the proposed fix, still looks good to me.

I can eventually just send a patch that clarifies the `num_domains` if you prefer.

Cheers,
Angelo

> [...]
> 
> Kind regards
> Uffe