| Message ID | 1524211433-48972-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20 April 2018 at 10:03, Shawn Lin <shawn.lin@rock-chips.com> wrote: > This reverts commit 79bccc5aefb4e64e651abe04f78c3e6bf8acd6f0. > > The intention for this revert is that the it's too engineering > solution for a special board but force all platforms to wait > for that long time, especially painful for mmc_power_up for eMMC > when booting. I fully agree with this reasoning. > > In practise, the modern hardware should have a stable power supply > with 1ms after setting it for no matter PMIC or discrete power. But > more importnatly, most regulators implement the callback of > ->set_voltage_time_sel() for regulator core to wait for specific > period of time for the power supply to be stable, which means once > regulator_set_voltage_* return, the power should reach the the minimum > voltage that works for initialization. > > It has been long time proved 2ms is enough when testing variety of > boards. If it finally turned out some boards need more than 2ms here, > the best we should do is to ask folks to get that period of time from > firmware node, for instance DT, and implement ->set_voltage_time_sel() > for the regulaor used, but not bother mmc core again. This is true if there is an external regulator, but sometimes we use host specific ways to power the card. > > Just revert it now and see if it could smokes out something interesting. And what to when/if there are errors reported? Revert the revert? :-) The 2ms was there in 2009, so it's not like it has recently changed to 10 ms. I am not really comfortable to change it like this. An option is to use an opt-out method, perhaps via a new host cap? Kind regards Uffe > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > Not sure if Toshiba Tecra M5 is still supported for upstream kernel > or if there are real users now. > > drivers/mmc/core/core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 121ce50..b154518 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1658,7 +1658,7 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) > * This delay should be sufficient to allow the power supply > * to reach the minimum voltage. > */ > - mmc_delay(10); > + mmc_delay(2); > > mmc_pwrseq_post_power_on(host); > > @@ -1671,7 +1671,7 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) > * This delay must be at least 74 clock sizes, or 1 ms, or the > * time required to reach a stable voltage. > */ > - mmc_delay(10); > + mmc_delay(2); > } > > void mmc_power_off(struct mmc_host *host) > -- > 1.9.1 > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > > The intention for this revert is that the it's too engineering > > solution for a special board but force all platforms to wait > > for that long time, especially painful for mmc_power_up for eMMC > > when booting. Careful! It was *introduced* for one specific board. But we can't tell if other platforms benefit from this since 2009(!). > > In practise, the modern hardware should have a stable power supply While I agree, sill notice the "should". Reality is often enough different. > > It has been long time proved 2ms is enough when testing variety of Where is that proof? If we have such a proof, things would be easy, but I really have strong doubts. We have indications, at best, I'd say. > This is true if there is an external regulator, but sometimes we use > host specific ways to power the card. This is also true and to be considered. > > > > Just revert it now and see if it could smokes out something interesting. Hell, no! "Let's change it to my needs and if it breaks for other people, they can complain"? That's not how Linux development works, we should try as hard as possible to not introduce regressions. > An option is to use an opt-out method, perhaps via a new host cap? Opt-in for the shortened delay, I'd suggest. Platforms which know they need less delay can set it. White listing. NAK for the original patch. Regards, Wolfram
On 2018/4/20 17:07, Ulf Hansson wrote: > On 20 April 2018 at 10:03, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> This reverts commit 79bccc5aefb4e64e651abe04f78c3e6bf8acd6f0. >> >> The intention for this revert is that the it's too engineering >> solution for a special board but force all platforms to wait >> for that long time, especially painful for mmc_power_up for eMMC >> when booting. > > I fully agree with this reasoning. > >> >> In practise, the modern hardware should have a stable power supply >> with 1ms after setting it for no matter PMIC or discrete power. But >> more importnatly, most regulators implement the callback of >> ->set_voltage_time_sel() for regulator core to wait for specific >> period of time for the power supply to be stable, which means once >> regulator_set_voltage_* return, the power should reach the the minimum >> voltage that works for initialization. >> >> It has been long time proved 2ms is enough when testing variety of >> boards. If it finally turned out some boards need more than 2ms here, >> the best we should do is to ask folks to get that period of time from >> firmware node, for instance DT, and implement ->set_voltage_time_sel() >> for the regulaor used, but not bother mmc core again. > > This is true if there is an external regulator, but sometimes we use > host specific ways to power the card. > >> >> Just revert it now and see if it could smokes out something interesting. > > And what to when/if there are errors reported? Revert the revert? :-) > > The 2ms was there in 2009, so it's not like it has recently changed to > 10 ms. I am not really comfortable to change it like this. > yes, that's why it's just a RFC that we could have disscussions based on code. :) > An option is to use an opt-out method, perhaps via a new host cap? Maybe we should allow a more scaleable method, for instance, ask host drivers to reported a desired delay if wanted, but fall back to use 10ms by default if not? > > Kind regards > Uffe > >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> Not sure if Toshiba Tecra M5 is still supported for upstream kernel >> or if there are real users now. >> >> drivers/mmc/core/core.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 121ce50..b154518 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -1658,7 +1658,7 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) >> * This delay should be sufficient to allow the power supply >> * to reach the minimum voltage. >> */ >> - mmc_delay(10); >> + mmc_delay(2); >> >> mmc_pwrseq_post_power_on(host); >> >> @@ -1671,7 +1671,7 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) >> * This delay must be at least 74 clock sizes, or 1 ms, or the >> * time required to reach a stable voltage. >> */ >> - mmc_delay(10); >> + mmc_delay(2); >> } >> >> void mmc_power_off(struct mmc_host *host) >> -- >> 1.9.1 >> >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018/4/20 17:27, Wolfram Sang wrote: > Hi, > >>> The intention for this revert is that the it's too engineering >>> solution for a special board but force all platforms to wait >>> for that long time, especially painful for mmc_power_up for eMMC >>> when booting. > > Careful! It was *introduced* for one specific board. But we can't tell > if other platforms benefit from this since 2009(!). > I agree. >>> In practise, the modern hardware should have a stable power supply > > While I agree, sill notice the "should". Reality is often enough > different. > right, I think "most likely" should be appropriate. >>> It has been long time proved 2ms is enough when testing variety of > > Where is that proof? If we have such a proof, things would be easy, but > I really have strong doubts. We have indications, at best, I'd say. > I just checked the instances(boards) as much as I could got from different vendors, but I didn't say it stands for every one. :) >> This is true if there is an external regulator, but sometimes we use >> host specific ways to power the card. > > This is also true and to be considered. > >>> >>> Just revert it now and see if it could smokes out something interesting. > > Hell, no! "Let's change it to my needs and if it breaks for other > people, they can complain"? That's not how Linux development works, we > should try as hard as possible to not introduce regressions. > Thanks for reminding :) >> An option is to use an opt-out method, perhaps via a new host cap? > > Opt-in for the shortened delay, I'd suggest. Platforms which know they > need less delay can set it. White listing. > > NAK for the original patch. Actually I never expect to apply a RFC, but always post them for disscussion for how we could to improve it :). Opt-in for the shoteded delay looks fine to me. If we are both agree with that, then I could post a regular patch for it. > > Regards, > > Wolfram > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] > >> An option is to use an opt-out method, perhaps via a new host cap? > > Opt-in for the shortened delay, I'd suggest. Platforms which know they > need less delay can set it. White listing. Exactly! That's what I meant - thanks for making it clear. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 121ce50..b154518 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1658,7 +1658,7 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) * This delay should be sufficient to allow the power supply * to reach the minimum voltage. */ - mmc_delay(10); + mmc_delay(2); mmc_pwrseq_post_power_on(host); @@ -1671,7 +1671,7 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) * This delay must be at least 74 clock sizes, or 1 ms, or the * time required to reach a stable voltage. */ - mmc_delay(10); + mmc_delay(2); } void mmc_power_off(struct mmc_host *host)
This reverts commit 79bccc5aefb4e64e651abe04f78c3e6bf8acd6f0. The intention for this revert is that the it's too engineering solution for a special board but force all platforms to wait for that long time, especially painful for mmc_power_up for eMMC when booting. In practise, the modern hardware should have a stable power supply with 1ms after setting it for no matter PMIC or discrete power. But more importnatly, most regulators implement the callback of ->set_voltage_time_sel() for regulator core to wait for specific period of time for the power supply to be stable, which means once regulator_set_voltage_* return, the power should reach the the minimum voltage that works for initialization. It has been long time proved 2ms is enough when testing variety of boards. If it finally turned out some boards need more than 2ms here, the best we should do is to ask folks to get that period of time from firmware node, for instance DT, and implement ->set_voltage_time_sel() for the regulaor used, but not bother mmc core again. Just revert it now and see if it could smokes out something interesting. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- Not sure if Toshiba Tecra M5 is still supported for upstream kernel or if there are real users now. drivers/mmc/core/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)