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

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

Il 12/03/26 15:42, Ulf Hansson ha scritto:
> On Thu, 12 Mar 2026 at 13:46, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> 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.
> 
> Thanks for clarifying!
> 

Oh you're welcome, of course.

> Although, having both seems problematic for the reasons I pointed out
> above. At least, it looks like some more re-work will be needed to
> support that case.

Yeah, that's right.
It's still a lot of work, but hopefully I made it "less than if this wasn't there".

Not sure if I did, but there's that, anyway...

> 
>>
>> 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;
> 
> Agreed.
> 
>>
>> (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)
> 
> Agreed, a print would be superfluous.
> 

Eh I forgot to add a "return -EINVAL" in the mix, which was the whole point of the
"parenthesis"... ugh. :-)

In any case I can understand that we're on the same page about superfluous stuff,
which is enough for the context of that.

>>
>> ...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.
> 
> Okay, I'm queuing the $subject patch as a fix and by adding a stable/fixes-tag.
> 
>>
>> I can eventually just send a patch that clarifies the `num_domains` if you prefer.
> 
> Well, actually, I would suggest splitting the driver into two drivers
> (each handling its own type of match data) and adding some shared
> helper functions, that both drivers can use.
> 
> It's more work, but we should end up with more maintainable code, I think.
> 
> What do you think?
> 

I had the same idea initially, but with how fundamentally broken is the "Hardware
Voter" mechanism in the current MediaTek SoCs (Dimensity 9400 MT6991, 9500 MT6993
and derivatives like Kompanio Ultra MT8196 and Genio Pro) I didn't want to really
do that.

I suspect that the HWV will change in the future, but I don't really know how, nor
I have any knowledge that would make me able to safely predict.

So, well, while in theory having two drivers, with some common code in between,
could make this more maintainable, I'm afraid that if we do this *right now* in
the current state of things, we will end up with three drivers instead...

Small clarification about the brokenness of the HWV: all of the HWV Domains and
clocks aren't really linked together internally... as much as HWV Domains and
some external regulators, so, basically, as of now, the mechanism is practically
useless if not for MCU-vs-AP *only partial* voting of resources (as dependencies
are not handled, and some of them are not even HWV dependencies.... eh.. anyway,
a big thread happened when initially upstreaming this entire thing, where a bit
more is explained...)

Ewww... I'm losing myself in blurb again..!

In any case - my plan of action is to keep things as they are (structurally, but
of course with the necessary quality/readability enhancements) until something
new, with a proper resource voting system implementation in hardware, comes out.

If everything goes according to the plan (again... if), once a SoC with a proper
implementation (HW ... but really it's firmware that can't be changed) we'll be
sure that any decision that we'll take on this will most probably result in a
flexible and maintainable implementation.

And that's why I'm basically saying "I would agree, but not now" :-)

Cheers,
Angelo

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