On Fri, 22 Mar 2024 at 00:47, Marek Vasut marex@denx.de wrote:
On 3/21/24 6:46 AM, Sumit Garg wrote:
On Thu, 21 Mar 2024 at 11:06, Marek Vasut marex@denx.de wrote:
On 3/15/24 11:41 AM, Sumit Garg wrote:
On Fri, 15 Mar 2024 at 14:53, Marek Vasut marex@denx.de wrote:
On 3/15/24 6:31 AM, Sumit Garg wrote:
On Thu, 14 Mar 2024 at 09:45, Marek Vasut marex@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 ?
It isn't actually solving anything since there isn't a need for refcount for PD bus since power domain off isn't exercised during the lifecycle of U-Boot. Hence, I dropped it.
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.
Sure, see below.
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 ?
I just did simple experiment via following diff:
diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c index 6188a04c45e..0ddcd39923a 100644 --- a/drivers/power/domain/imx8mp-hsiomix.c +++ b/drivers/power/domain/imx8mp-hsiomix.c @@ -101,6 +101,7 @@ static int imx8mp_hsiomix_set(struct power_domain *power_domain, bool power_on) if (gpr_reg0_bits) setbits_le32(priv->base + GPR_REG0, gpr_reg0_bits); } else { + while(1); if (gpr_reg0_bits) clrbits_le32(priv->base + GPR_REG0, gpr_reg0_bits);
The boot doesn't hang suggesting that CONFIG_DM_DEVICE_REMOVE=y isn't effective to remove any DT devices. It can for sure be another followup series to make it effective.
-Sumit