[PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

[Michal Wilczynski]

On 6/4/25 14:07, Krzysztof Kozlowski wrote:
> On 04/06/2025 13:53, Michal Wilczynski wrote:
>>>>
>>>> The GPU node will depend on the AON node, which will be the sole
>>>> provider for the 'gpu-power' sequencer (based on the discussion in patch
>>>> 1).
>>>>
>>>> Therefore, if the AON/pwrseq driver has already completed its probe, and
>>>> devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
>>>> (because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
>>>> interpretation is that the AON driver did not register this optional
>>>> sequencer. Since AON is the only anticipated source, it implies the
>>>> sequencer won't become available later from its designated provider.
>>>
>>> I don't understand why you made this assumption. AON could be a module
>>> and this driver built-in. AON will likely probe later.
>>
>> You're absolutely right that AON could be a module and would generally
>> probe later in that scenario. However, the GPU device also has a
>> 'power-domains = <&aon TH1520_GPU_PD>' dependency. If the AON driver (as
>> the PM domain provider) were a late probing module, the GPU driver's
>> probe would hit -EPROBE_DEFER when its power domain is requested
>> which happens before attempting to get other resources like a power
>> sequencer.
> 
> Huh, so basically you imply certain hardware design and certain DTS
> description in your driver code. Well, that's clearly fragile design to
> me, because you should not rely how hardware properties are presented in
> DTS. Will work here on th1520 with this DTS, won't work with something else.
> 
> Especially that this looks like generic Imagination GPU code, common to
> multiple devices, not TH1520 only specific.
> 
>>
>> So, if the GPU driver's code does reach the devm_pwrseq_get(dev,
>> "gpu-power") call, it strongly implies the AON driver has already
>> successfully probed.
>>
>> This leads to the core challenge with the optional 'gpu-power'
>> sequencer: Even if the AON driver has already probed, if it then chooses
>> not to register the "gpu-power" sequence (because it's an optional
>> feature), pwrseq_get() will still find "no device matched" on the
>> pwrseq_bus and return EPROBE_DEFER.
>>
>> If the GPU driver defers here, as it normally should for -EPROBE_DEFER,
>> it could wait indefinitely for an optional sequence that its
>> already probed AON provider will not supply.
>>
>> Anyway I think you're right, that this is probably confusing and we
>> shouldn't rely on this behavior.
>>
>> To solve this, and to allow the GPU driver to correctly handle
>> -EPROBE_DEFER when a sequencer is genuinely expected, I propose using a
>> boolean property on the GPU's DT node, e.g.
>> img,gpu-expects-power-sequencer. If the GPU node provides this property
>> it means the pwrseq 'gpu-power' is required.
> 
> No, that would be driver design in DTS.
> 
> I think the main problem is the pwrseq API: you should get via phandle,
> not name of the pwrseq controller. That's how all producer-consumer
> relationships are done in OF platforms.

Bart,
Given Krzysztof's valid concerns about the current name based
lookup in pwrseq_get() and the benefits of phandle based resource
linking in OF platforms: Would you be open to a proposal for extending
the pwrseq API to allow consumers to obtain a sequencer (or a specific
target sequence) via a phandle defined in their Device Tree node? For
instance, a consumer device could specify power-sequencer =
<&aon> and a new API variant could resolve this.

> 
> It's also fragile to rely on names in case of systems with multiple
> similar devices. This does not affect your platform and this hardware in
> general, but shows issues with interface: imagine multiple gpus and
> multiple pwr sequence devices. Which one should be obtained?
> gpu-power-1? But if GPUs are the same class of devices (e.g. 2x TG1520
> GPU) this is just imprecise.
> 
> 
> Best regards,
> Krzysztof
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski at samsung.com>