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

[Sumit Garg]

Gentle ping..

On Fri, 15 Mar 2024 at 16:11, Sumit Garg <sumit.garg at linaro.org> 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.
>
> 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.

How would you like me to proceed here?

-Sumit