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

[Sumit Garg]

On Fri, 22 Mar 2024 at 00:47, Marek Vasut <marex at denx.de> wrote:
>
> 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 ?
>

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