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

[Sumit Garg]

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.

BTW, dev_power_domain_on() is invoked when USB controller devices are
added based on DT.

>
> >>> Fixes: 898e7610c62a ("imx: power-domain: Add i.MX8MP HSIOMIX driver")
> >>> Signed-off-by: Sumit Garg <sumit.garg at linaro.org>
> >>> ---
> >>>    drivers/power/domain/imx8mp-hsiomix.c | 6 +-----
> >>>    1 file changed, 1 insertion(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
> >>> index e2d772c5ec7..448746432a2 100644
> >>> --- a/drivers/power/domain/imx8mp-hsiomix.c
> >>> +++ b/drivers/power/domain/imx8mp-hsiomix.c
> >>> @@ -50,7 +50,7 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
> >>>
> >>>        ret = power_domain_on(domain);
> >>>        if (ret)
> >>> -             goto err_pd;
> >>> +             return ret;
> >>>
> >>>        ret = clk_enable(&priv->clk_usb);
> >>>        if (ret)
> >>> @@ -63,8 +63,6 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
> >>>
> >>>    err_clk:
> >>>        power_domain_off(domain);
> >>> -err_pd:
> >>> -     power_domain_off(&priv->pd_bus);
> >>>        return ret;
> >>
> >> 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? 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.

-Sumit