| Message ID | 20260511074953.3777767-1-ye.li@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jaehoon Chung |
| Headers | show |
| Series | power: regulator: Fix power on/off delay issue | expand |
Hi, On 5/11/2026 9:49 AM, Ye Li wrote: > SD initialization failure happens with some UHS-I SD cards on > iMX8MM/iMX93/iMX91 EVK after > commit 4fcba5d556b4 ("regulator: implement basic reference counter"). > When sending operation condition to SD card, the OCR does not return > correct status. The root cause is regulator on/off delay is missed > in MMC power cycle with above commit, so SD card is not completely > power off. > > When SD startup, the sequence of MMC power cycle is: > mmc_power_init(get vmmc_supply dev) -> mmc_power_off -> udelay(2000) > -> mmc_power_on > > Before above commit, as a fixed regulator, the GPIO is set as: > GPIO inactive (in mmc_power_init) -> > GPIO inactive and delay off-on-delay-us (in mmc_power_off) -> > udelay(2000) -> > GPIO active (in mmc_power_on) > > After the commit: > GPIO inactive (in mmc_power_init) -> > enable_count is 0, regulator_set_enable returns -EALREADY immediately, > so GPIO is inactive but No off-on-delay-us (in mmc_power_off) -> > udelay(2000) -> > GPIO active (in mmc_power_on) > > Add the on/off delay and startup delay after GPIO request in > regulator_common_of_to_plat which is called in regulator device probing. > So in mmc_power_init, after GPIO is set to default inactive, the > off-on-delay-us is applied. > > Signed-off-by: Ye Li <ye.li@nxp.com> > --- > drivers/power/regulator/regulator_common.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c > index 85af8d599ad..158284a3309 100644 > --- a/drivers/power/regulator/regulator_common.c > +++ b/drivers/power/regulator/regulator_common.c > @@ -46,6 +46,12 @@ int regulator_common_of_to_plat(struct udevice *dev, > dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0); > } > > + if ((flags & GPIOD_IS_OUT_ACTIVE) && plat->startup_delay_us) > + udelay(plat->startup_delay_us); > + > + if (!(flags & GPIOD_IS_OUT_ACTIVE) && plat->off_on_delay_us) > + udelay(plat->off_on_delay_us); regulator_common_of_to_plat() is for parsing the values, no udelay() should be applied here. regulator_common_set_enable() applies udelay() when the regulator is enabled. Does your regulator .get_enable() ops not being called or does it not implement its own udelay() based on parsed values? Regards, Jonas > + > return 0; > } >
Hi again, On 5/11/2026 10:30 AM, Jonas Karlman wrote: > Hi, > > On 5/11/2026 9:49 AM, Ye Li wrote: >> SD initialization failure happens with some UHS-I SD cards on >> iMX8MM/iMX93/iMX91 EVK after >> commit 4fcba5d556b4 ("regulator: implement basic reference counter"). >> When sending operation condition to SD card, the OCR does not return >> correct status. The root cause is regulator on/off delay is missed >> in MMC power cycle with above commit, so SD card is not completely >> power off. >> >> When SD startup, the sequence of MMC power cycle is: >> mmc_power_init(get vmmc_supply dev) -> mmc_power_off -> udelay(2000) >> -> mmc_power_on >> >> Before above commit, as a fixed regulator, the GPIO is set as: >> GPIO inactive (in mmc_power_init) -> >> GPIO inactive and delay off-on-delay-us (in mmc_power_off) -> >> udelay(2000) -> >> GPIO active (in mmc_power_on) >> >> After the commit: >> GPIO inactive (in mmc_power_init) -> >> enable_count is 0, regulator_set_enable returns -EALREADY immediately, >> so GPIO is inactive but No off-on-delay-us (in mmc_power_off) -> If the regulator is off at boot-on then this is working as intended. However, if probing the regulator turns it off and that requires a off-on-delay-us delay, maybe you are missing a regulator-boot-on property to indicate that the regulator is on at boot/reset. That should also align the enable count with boot-on state of the regulator. Looking at the kernel the off-on-delay-us is applied before a regulator is enabled, not after it is disabled like in U-Boot. Maybe U-Boot should do something similar, in simplest form always udelay off-on-delay-us before the regulator is enabled. >> udelay(2000) -> >> GPIO active (in mmc_power_on) >> >> Add the on/off delay and startup delay after GPIO request in >> regulator_common_of_to_plat which is called in regulator device probing. >> So in mmc_power_init, after GPIO is set to default inactive, the >> off-on-delay-us is applied. >> >> Signed-off-by: Ye Li <ye.li@nxp.com> >> --- >> drivers/power/regulator/regulator_common.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c >> index 85af8d599ad..158284a3309 100644 >> --- a/drivers/power/regulator/regulator_common.c >> +++ b/drivers/power/regulator/regulator_common.c >> @@ -46,6 +46,12 @@ int regulator_common_of_to_plat(struct udevice *dev, >> dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0); >> } >> >> + if ((flags & GPIOD_IS_OUT_ACTIVE) && plat->startup_delay_us) >> + udelay(plat->startup_delay_us); >> + >> + if (!(flags & GPIOD_IS_OUT_ACTIVE) && plat->off_on_delay_us) >> + udelay(plat->off_on_delay_us); > > regulator_common_of_to_plat() is for parsing the values, no udelay() > should be applied here. > > regulator_common_set_enable() applies udelay() when the regulator is > enabled. Or disabled. > > Does your regulator .get_enable() ops not being called or does it not > implement its own udelay() based on parsed values? I meant set_enable() here :-) Regards, Jonas > > Regards, > Jonas > >> + >> return 0; >> } >> >
On Mon, May 11, 2026 at 03:10:16PM +0200, Jonas Karlman wrote: >Hi again, > >On 5/11/2026 10:30 AM, Jonas Karlman wrote: >> Hi, >> >> On 5/11/2026 9:49 AM, Ye Li wrote: >>> SD initialization failure happens with some UHS-I SD cards on >>> iMX8MM/iMX93/iMX91 EVK after >>> commit 4fcba5d556b4 ("regulator: implement basic reference counter"). >>> When sending operation condition to SD card, the OCR does not return >>> correct status. The root cause is regulator on/off delay is missed >>> in MMC power cycle with above commit, so SD card is not completely >>> power off. >>> >>> When SD startup, the sequence of MMC power cycle is: >>> mmc_power_init(get vmmc_supply dev) -> mmc_power_off -> udelay(2000) >>> -> mmc_power_on >>> >>> Before above commit, as a fixed regulator, the GPIO is set as: >>> GPIO inactive (in mmc_power_init) -> >>> GPIO inactive and delay off-on-delay-us (in mmc_power_off) -> >>> udelay(2000) -> >>> GPIO active (in mmc_power_on) >>> >>> After the commit: >>> GPIO inactive (in mmc_power_init) -> >>> enable_count is 0, regulator_set_enable returns -EALREADY immediately, >>> so GPIO is inactive but No off-on-delay-us (in mmc_power_off) -> > >If the regulator is off at boot-on then this is working as intended. > >However, if probing the regulator turns it off and that requires a >off-on-delay-us delay, maybe you are missing a regulator-boot-on >property to indicate that the regulator is on at boot/reset. That should >also align the enable count with boot-on state of the regulator. > >Looking at the kernel the off-on-delay-us is applied before a regulator >is enabled, not after it is disabled like in U-Boot. Maybe U-Boot should >do something similar, in simplest form always udelay off-on-delay-us >before the regulator is enabled. Yeah. Something below should work(Not tested). diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 85af8d599ad..7bf25f6a176 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -68,6 +68,9 @@ int regulator_common_set_enable(const struct udevice *dev, dev->name, enable, plat->startup_delay_us, dm_gpio_is_valid(&plat->gpio)); + if (enable && plat->off_on_delay_us) + udelay(plat->off_on_delay_us); + /* Enable GPIO is optional */ if (CONFIG_IS_ENABLED(DM_GPIO) && dm_gpio_is_valid(&plat->gpio)) { /* If previously enabled, increase count */ @@ -97,9 +100,6 @@ int regulator_common_set_enable(const struct udevice *dev, if (enable && plat->startup_delay_us) udelay(plat->startup_delay_us); - if (!enable && plat->off_on_delay_us) - udelay(plat->off_on_delay_us); - if (enable) plat->enable_count++; else Regards Peng. > >>> udelay(2000) -> >>> GPIO active (in mmc_power_on) >>> >>> Add the on/off delay and startup delay after GPIO request in >>> regulator_common_of_to_plat which is called in regulator device probing. >>> So in mmc_power_init, after GPIO is set to default inactive, the >>> off-on-delay-us is applied. >>> >>> Signed-off-by: Ye Li <ye.li@nxp.com> >>> --- >>> drivers/power/regulator/regulator_common.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c >>> index 85af8d599ad..158284a3309 100644 >>> --- a/drivers/power/regulator/regulator_common.c >>> +++ b/drivers/power/regulator/regulator_common.c >>> @@ -46,6 +46,12 @@ int regulator_common_of_to_plat(struct udevice *dev, >>> dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0); >>> } >>> >>> + if ((flags & GPIOD_IS_OUT_ACTIVE) && plat->startup_delay_us) >>> + udelay(plat->startup_delay_us); >>> + >>> + if (!(flags & GPIOD_IS_OUT_ACTIVE) && plat->off_on_delay_us) >>> + udelay(plat->off_on_delay_us); >> >> regulator_common_of_to_plat() is for parsing the values, no udelay() >> should be applied here. >> >> regulator_common_set_enable() applies udelay() when the regulator is >> enabled. > >Or disabled. > >> >> Does your regulator .get_enable() ops not being called or does it not >> implement its own udelay() based on parsed values? > >I meant set_enable() here :-) > >Regards, >Jonas > >> >> Regards, >> Jonas >> >>> + >>> return 0; >>> } >>> >> > >
Hi Jonas, Peng, On 5/15/2026 10:07 PM, Peng Fan wrote: > On Mon, May 11, 2026 at 03:10:16PM +0200, Jonas Karlman wrote: >> Hi again, >> >> On 5/11/2026 10:30 AM, Jonas Karlman wrote: >>> Hi, >>> >>> On 5/11/2026 9:49 AM, Ye Li wrote: >>>> SD initialization failure happens with some UHS-I SD cards on >>>> iMX8MM/iMX93/iMX91 EVK after >>>> commit 4fcba5d556b4 ("regulator: implement basic reference counter"). >>>> When sending operation condition to SD card, the OCR does not return >>>> correct status. The root cause is regulator on/off delay is missed >>>> in MMC power cycle with above commit, so SD card is not completely >>>> power off. >>>> >>>> When SD startup, the sequence of MMC power cycle is: >>>> mmc_power_init(get vmmc_supply dev) -> mmc_power_off -> udelay(2000) >>>> -> mmc_power_on >>>> >>>> Before above commit, as a fixed regulator, the GPIO is set as: >>>> GPIO inactive (in mmc_power_init) -> >>>> GPIO inactive and delay off-on-delay-us (in mmc_power_off) -> >>>> udelay(2000) -> >>>> GPIO active (in mmc_power_on) >>>> >>>> After the commit: >>>> GPIO inactive (in mmc_power_init) -> >>>> enable_count is 0, regulator_set_enable returns -EALREADY immediately, >>>> so GPIO is inactive but No off-on-delay-us (in mmc_power_off) -> >> >> If the regulator is off at boot-on then this is working as intended. >> >> However, if probing the regulator turns it off and that requires a >> off-on-delay-us delay, maybe you are missing a regulator-boot-on >> property to indicate that the regulator is on at boot/reset. That should >> also align the enable count with boot-on state of the regulator. This regulator is on at boot-on. But there is no regulator-boot-on used in kernel upstream dts. This will require to add it specifically for u-boot. >> >> Looking at the kernel the off-on-delay-us is applied before a regulator >> is enabled, not after it is disabled like in U-Boot. Maybe U-Boot should >> do something similar, in simplest form always udelay off-on-delay-us >> before the regulator is enabled. Agree, moving off-on-delay-us before regulator is enabled should work. I will use it in v2. > > Yeah. Something below should work(Not tested). > > diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c > index 85af8d599ad..7bf25f6a176 100644 > --- a/drivers/power/regulator/regulator_common.c > +++ b/drivers/power/regulator/regulator_common.c > @@ -68,6 +68,9 @@ int regulator_common_set_enable(const struct udevice *dev, > dev->name, enable, plat->startup_delay_us, > dm_gpio_is_valid(&plat->gpio)); > > + if (enable && plat->off_on_delay_us) > + udelay(plat->off_on_delay_us); > + Thanks for the patch. But I think off_on_delay_us should locate just before calling dm_gpio_set_value. Will send it in v2. Best regards, Ye Li > /* Enable GPIO is optional */ > if (CONFIG_IS_ENABLED(DM_GPIO) && dm_gpio_is_valid(&plat->gpio)) { > /* If previously enabled, increase count */ > @@ -97,9 +100,6 @@ int regulator_common_set_enable(const struct udevice *dev, > if (enable && plat->startup_delay_us) > udelay(plat->startup_delay_us); > > - if (!enable && plat->off_on_delay_us) > - udelay(plat->off_on_delay_us); > - > if (enable) > plat->enable_count++; > else > > Regards > Peng. > >> >>>> udelay(2000) -> >>>> GPIO active (in mmc_power_on) >>>> >>>> Add the on/off delay and startup delay after GPIO request in >>>> regulator_common_of_to_plat which is called in regulator device probing. >>>> So in mmc_power_init, after GPIO is set to default inactive, the >>>> off-on-delay-us is applied. >>>> >>>> Signed-off-by: Ye Li <ye.li@nxp.com> >>>> --- >>>> drivers/power/regulator/regulator_common.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c >>>> index 85af8d599ad..158284a3309 100644 >>>> --- a/drivers/power/regulator/regulator_common.c >>>> +++ b/drivers/power/regulator/regulator_common.c >>>> @@ -46,6 +46,12 @@ int regulator_common_of_to_plat(struct udevice *dev, >>>> dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0); >>>> } >>>> >>>> + if ((flags & GPIOD_IS_OUT_ACTIVE) && plat->startup_delay_us) >>>> + udelay(plat->startup_delay_us); >>>> + >>>> + if (!(flags & GPIOD_IS_OUT_ACTIVE) && plat->off_on_delay_us) >>>> + udelay(plat->off_on_delay_us); >>> >>> regulator_common_of_to_plat() is for parsing the values, no udelay() >>> should be applied here. >>> >>> regulator_common_set_enable() applies udelay() when the regulator is >>> enabled. >> >> Or disabled. >> >>> >>> Does your regulator .get_enable() ops not being called or does it not >>> implement its own udelay() based on parsed values? >> >> I meant set_enable() here :-) >> >> Regards, >> Jonas >> >>> >>> Regards, >>> Jonas >>> >>>> + >>>> return 0; >>>> } >>>> >>> >> >>
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 85af8d599ad..158284a3309 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -46,6 +46,12 @@ int regulator_common_of_to_plat(struct udevice *dev, dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0); } + if ((flags & GPIOD_IS_OUT_ACTIVE) && plat->startup_delay_us) + udelay(plat->startup_delay_us); + + if (!(flags & GPIOD_IS_OUT_ACTIVE) && plat->off_on_delay_us) + udelay(plat->off_on_delay_us); + return 0; }
SD initialization failure happens with some UHS-I SD cards on iMX8MM/iMX93/iMX91 EVK after commit 4fcba5d556b4 ("regulator: implement basic reference counter"). When sending operation condition to SD card, the OCR does not return correct status. The root cause is regulator on/off delay is missed in MMC power cycle with above commit, so SD card is not completely power off. When SD startup, the sequence of MMC power cycle is: mmc_power_init(get vmmc_supply dev) -> mmc_power_off -> udelay(2000) -> mmc_power_on Before above commit, as a fixed regulator, the GPIO is set as: GPIO inactive (in mmc_power_init) -> GPIO inactive and delay off-on-delay-us (in mmc_power_off) -> udelay(2000) -> GPIO active (in mmc_power_on) After the commit: GPIO inactive (in mmc_power_init) -> enable_count is 0, regulator_set_enable returns -EALREADY immediately, so GPIO is inactive but No off-on-delay-us (in mmc_power_off) -> udelay(2000) -> GPIO active (in mmc_power_on) Add the on/off delay and startup delay after GPIO request in regulator_common_of_to_plat which is called in regulator device probing. So in mmc_power_init, after GPIO is set to default inactive, the off-on-delay-us is applied. Signed-off-by: Ye Li <ye.li@nxp.com> --- drivers/power/regulator/regulator_common.c | 6 ++++++ 1 file changed, 6 insertions(+)