[PATCH v3 04/11] imx8mp: power-domain: Don't power off pd_bus

[Marek Vasut]

On 3/21/24 6:46 AM, Sumit Garg wrote:
> On Thu, 21 Mar 2024 at 11:06, Marek Vasut <marex at denx.de> wrote:
>>
>> On 3/15/24 11:41 AM, Sumit Garg wrote:
>>> On Fri, 15 Mar 2024 at 14:53, Marek Vasut <marex at denx.de> wrote:
>>>>
>>>> On 3/15/24 6:31 AM, Sumit Garg wrote:
>>>>> On Thu, 14 Mar 2024 at 09:45, Marek Vasut <marex at denx.de> wrote:
>>>>>>
>>>>>> On 3/12/24 8:03 AM, Sumit Garg wrote:
>>>>>>> power_domain_on/off() isn't refcounted and power domain bus shouldn't be
>>>>>>> turned off for a single peripheral domain as it would negatively affect
>>>>>>> other peripheral domains. So lets just skip turning off bus power
>>>>>>> domain.
>>>>>>
>>>>>> What exactly is the issue and how did you trigger it ?
>>>>>>
>>>>>> Details please.
>>>>>
>>>>> I suppose the issue can be triggered via the "=> usb start => usb
>>>>> stop" sequence where one of the USB controllers is configured in
>>>>> peripheral mode.
>>>>
>>>> 'usb start ; usb stop' causes no problems on MX8MP , maybe the test case
>>>> is more extensive ?
>>>>
>>>> Please, write down the necessary steps to reproduce this problem, and
>>>> what happens when that problem occurs.
>>>
>>> After digging in more, it looks like dev_power_domain_off() is never
>>> (U-Boot life-cycle) invoked for USB controller devices derived from
>>> DT. So this USB power domain sequence is never reachable.
>>
>> The imx8mp_hsiomix_off() is never called on 'usb stop' command ?
>>
> 
> Yeah, that's the case.
> 
>> But then why would the 'usb start ; usb stop' test break power domain
>> state here ?
> 
> It won't break with current implementation, earlier I made this
> assumption that 'usb stop' turns down the power domain.

So, maybe I am a little confused, what is this patch solving then ?

>>> BTW, dev_power_domain_on() is invoked when USB controller devices are
>>> added based on DT.
>>
>> I would expect imx8mp_hsiomix_off() to be called either on 'usb stop' or
>> just before Linux boots .
>>
>> [...]
>>
>>>>>> Why not add counter into imx8mp_hsiomix_priv structure in this driver ?
>>>>>
>>>>> Sure I can do that but do you think the current approach can have any
>>>>> side effects?
>>>>
>>>> Bus domain not getting cycled (which can leave it in some odd state),
>>>> and increased power consumption if the next stage doesn't turn the
>>>> domain off.
>>>
>>> Given above, would you like me to drop power domain off path entirely
>>> here?
>>
>> Can the series go in without this patch ?
> 
> Okay let me drop this patch.

We can fix whatever it is that needs to be fixed in a smaller follow up 
series.

>>> I think if people are concerned about power consumption then it
>>> should be implemented properly in U-Boot to remove all the DT based
>>> devices before passing on control to the next stage.
>>
>> I would expect imx8mp_hsiomix_off() to be called either on 'usb stop' or
>> just before Linux boots (esp. at that point), so if you do not power off
>> the bus domain before booting Linux, you may hand over a device which
>> was not fully power cycled.
> 
> Unfortunately that's the current situation I see. IMO, the better
> solution would be to just remove all the DT devices before passing on
> control to Linux. That should automatically power off devices.

Doesn't CONFIG_DM_DEVICE_REMOVE=y do something like that already ?