| Message ID | 20240312070338.86127-5-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | imx8mp: Enable PCIe/NVMe support | expand |
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. > Fixes: 898e7610c62a ("imx: power-domain: Add i.MX8MP HSIOMIX driver") > Signed-off-by: Sumit Garg <sumit.garg@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 ?
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. > > > Fixes: 898e7610c62a ("imx: power-domain: Add i.MX8MP HSIOMIX driver") > > Signed-off-by: Sumit Garg <sumit.garg@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? -Sumit
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. >>> Fixes: 898e7610c62a ("imx: power-domain: Add i.MX8MP HSIOMIX driver") >>> Signed-off-by: Sumit Garg <sumit.garg@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.
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. 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@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
Gentle ping.. On Fri, 15 Mar 2024 at 16:11, Sumit Garg <sumit.garg@linaro.org> 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. > > 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@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
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 ? But then why would the 'usb start ; usb stop' test break power domain state here ? > 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 ? > 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. And sorry for the delay, I was a bit busy.
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. > > > 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. > > > 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. > > And sorry for the delay, I was a bit busy. No worries. -Sumit
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 ? >>> 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 ?
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
On 3/22/24 5:55 AM, Sumit Garg wrote: Hi, >>>>> 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. That's odd. Please try and edit drivers/core/device-remove.c , look up device_remove() at the end , and see if the remove is being called on the hsiomix device . You might need to add some sort of printf() there, but that's where the removal happens. The removal before boot is I think called from drivers/core/root.c dm_remove_devices_flags(), which itself is called from arch/arm/lib/bootm.c announce_and_cleanup() .
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; } @@ -85,8 +83,6 @@ static int imx8mp_hsiomix_off(struct power_domain *power_domain) else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) power_domain_off(&priv->pd_usb_phy2); - power_domain_off(&priv->pd_bus); - return 0; }
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. Fixes: 898e7610c62a ("imx: power-domain: Add i.MX8MP HSIOMIX driver") Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- drivers/power/domain/imx8mp-hsiomix.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)