| Message ID | CAGOxZ53ndi_hYDGBGhY-7q8aOrFid1=D=_5KEbUvzTsnybJQ3Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Alim Sorry for late reply. On 2015/2/16 07:28, Alim Akhtar wrote: > Hi Addy, > > On Sat, Feb 14, 2015 at 11:47 AM, Addy Ke <addy.ke@rock-chips.com> wrote: >> As show in mmc_power_up(), in MMC_POWER_UP state, the voltage isn't >> stable and we may get 'data busy' which can't be cleaned by resetting >> all blocks. So we should not send command to update clock in this state. >> >> Signed-off-by: Addy Ke <addy.ke@rock-chips.com> >> --- >> drivers/mmc/host/dw_mmc.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 4d2e3c2..3472f9b 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -1102,7 +1102,8 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> drv_data->set_ios(slot->host, ios); >> >> /* Slot specific timing and width adjustment */ >> - dw_mci_setup_bus(slot, false); >> + if (ios->power_mode != MMC_POWER_UP) >> + dw_mci_setup_bus(slot, false); >> > This looks a HACK to me. > If stabilizing host voltage regulator is the problem, can you try out > below patch, and see if this resolve your issue? I have test by: cd /sys/bus/platform/drivers/dwmmc_rockchip for i in $(seq 1 10000); do echo "========================" $i echo ff0c0000.dwmmc > unbind sleep .5 echo ff0c0000.dwmmc > bind sleep 2 done There is no error. I think this patch can resolve my issue, thank you. Do you send this patch upstream, or can I put it in my patch list? > > =========== > [PATCH] mmc: dw_mmc: Wait for host voltage regulator to be stable > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> > --- > drivers/mmc/host/dw_mmc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 4d2e3c2..dc10fbb 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -1202,6 +1202,9 @@ static int dw_mci_switch_voltage(struct mmc_host > *mmc, struct mmc_ios *ios) > } > mci_writel(host, UHS_REG, uhs); > > + /* wait for 5ms so that host voltage regulator is stable */ > + usleep_range(5000, 5500); > + > return 0; > } > > =============== > >> if (slot->host->state == STATE_WAITING_CMD11_DONE && ios->clock != 0) >> slot->host->state = STATE_IDLE; >> -- >> 1.8.3.2 >> >> > > >
Alim and Addy, On Sun, Feb 15, 2015 at 3:28 PM, Alim Akhtar <alim.akhtar@gmail.com> wrote: > Hi Addy, > > On Sat, Feb 14, 2015 at 11:47 AM, Addy Ke <addy.ke@rock-chips.com> wrote: >> As show in mmc_power_up(), in MMC_POWER_UP state, the voltage isn't >> stable and we may get 'data busy' which can't be cleaned by resetting >> all blocks. So we should not send command to update clock in this state. >> >> Signed-off-by: Addy Ke <addy.ke@rock-chips.com> >> --- >> drivers/mmc/host/dw_mmc.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 4d2e3c2..3472f9b 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -1102,7 +1102,8 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> drv_data->set_ios(slot->host, ios); >> >> /* Slot specific timing and width adjustment */ >> - dw_mci_setup_bus(slot, false); >> + if (ios->power_mode != MMC_POWER_UP) >> + dw_mci_setup_bus(slot, false); >> > This looks a HACK to me. > If stabilizing host voltage regulator is the problem, can you try out > below patch, and see if this resolve your issue? Actually, IMHO Alim's patch is more of a hack than Addy's. There's already a 10ms delay between "power up" and "power on" in the MMC core in mmc_power_up() state. That delay is commented as: /* * This delay should be sufficient to allow the power supply * to reach the minimum voltage. */ mmc_delay(10); That means that assuming that the voltage is stable in MMC_POWER_UP is not valid anyway. Addy's patch certainly needs more comments. In another context Olof suggested: /* * Skip bus setup while voltage is still stabilizing. Instead, * bus setup will be done at MMC_POWER_ON. */ ...thinking about this more, though: really the voltage should be stabilized when the regulator call returns (see my comments below). In actuality the "right" fix might be to just rearrange this function a little not to turn the clock on _before_ we even try to turn the voltage on. I've got that coded up but I'm still testing it... If you want to try it too, you can find it at <https://chromium-review.googlesource.com/251341>. Note that without my patch I find that I _really_ need Addy's patch to make sure that the card isn't busy in setup_bus. With my patch Addy's code catches the card busy less often. I'm still trying to see if there's a way to totally remove the need for his setup_bus and still trying to grok all the patches flying around... > =========== > [PATCH] mmc: dw_mmc: Wait for host voltage regulator to be stable > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> > --- > drivers/mmc/host/dw_mmc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 4d2e3c2..dc10fbb 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -1202,6 +1202,9 @@ static int dw_mci_switch_voltage(struct mmc_host > *mmc, struct mmc_ios *ios) > } > mci_writel(host, UHS_REG, uhs); > > + /* wait for 5ms so that host voltage regulator is stable */ > + usleep_range(5000, 5500); > + Alim: if you have some other instance where you actually need VQMMC to stabilize it should probably be done in a different way. If I understand correctly it is the regulator core's job to make sure that voltage is stable before returning. If you have a gpio-regulator you may be able to use "startup-delay-us" to specify how long the regulator takes to come up. You could also look at 'regulator-enable-ramp-delay' -Doug
On Thu, Feb 19, 2015 at 03:49:46PM -0800, Doug Anderson wrote: > Alim and Addy, > > On Sun, Feb 15, 2015 at 3:28 PM, Alim Akhtar <alim.akhtar@gmail.com> wrote: > > Hi Addy, > > > > On Sat, Feb 14, 2015 at 11:47 AM, Addy Ke <addy.ke@rock-chips.com> wrote: > >> As show in mmc_power_up(), in MMC_POWER_UP state, the voltage isn't > >> stable and we may get 'data busy' which can't be cleaned by resetting > >> all blocks. So we should not send command to update clock in this state. > >> > >> Signed-off-by: Addy Ke <addy.ke@rock-chips.com> > >> --- > >> drivers/mmc/host/dw_mmc.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > >> index 4d2e3c2..3472f9b 100644 > >> --- a/drivers/mmc/host/dw_mmc.c > >> +++ b/drivers/mmc/host/dw_mmc.c > >> @@ -1102,7 +1102,8 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > >> drv_data->set_ios(slot->host, ios); > >> > >> /* Slot specific timing and width adjustment */ > >> - dw_mci_setup_bus(slot, false); > >> + if (ios->power_mode != MMC_POWER_UP) > >> + dw_mci_setup_bus(slot, false); > >> > > This looks a HACK to me. > > If stabilizing host voltage regulator is the problem, can you try out > > below patch, and see if this resolve your issue? > > Actually, IMHO Alim's patch is more of a hack than Addy's. There's > already a 10ms delay between "power up" and "power on" in the MMC core > in mmc_power_up() state. That delay is commented as: > > /* > * This delay should be sufficient to allow the power supply > * to reach the minimum voltage. > */ > mmc_delay(10); > > That means that assuming that the voltage is stable in MMC_POWER_UP is > not valid anyway. > > > Addy's patch certainly needs more comments. In another context Olof suggested: > > /* > * Skip bus setup while voltage is still stabilizing. Instead, > * bus setup will be done at MMC_POWER_ON. > */ > > > ...thinking about this more, though: really the voltage should be > stabilized when the regulator call returns (see my comments below). > In actuality the "right" fix might be to just rearrange this function > a little not to turn the clock on _before_ we even try to turn the > voltage on. This is exactly why I've been saying that we need to get away from the POWER_UP vs POWER_ON stuff. We instead need a _single_ call into drivers to tell them to apply power and bring the card to a state where it can be talked to. That includes waiting for the power to stabilise, and sending the initial clocks to allow the card to initialise. In the case of extra GPIOs, host drivers will need to call back into the MMC core at the point where they have stabilised the power. The current situation where we split the power-up/power-on sequence between the host drivers and the core is really a hinderance to getting a working implementation - it's additional complexity where none is needed.
Hi, On Thu, Feb 19, 2015 at 3:49 PM, Doug Anderson <dianders@chromium.org> wrote: > I've got that coded up but I'm still testing it... If you want to try > it too, you can find it at > <https://chromium-review.googlesource.com/251341>. > > Note that without my patch I find that I _really_ need Addy's patch to > make sure that the card isn't busy in setup_bus. With my patch Addy's > code catches the card busy less often. I'm still trying to see if > there's a way to totally remove the need for his setup_bus and still > trying to grok all the patches flying around... Ah, this might be the magic needed: https://chromium-review.googlesource.com/251344 I think that together with the previous patch things are happy for me without any of Addy's patches, though it's the end of my work day and I haven't given this nearly as much testing as I'd like. I'll continue testing tomorrow and then post both patches together upstream. -Doug
Hi, On Thu, Feb 19, 2015 at 5:04 PM, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Thu, Feb 19, 2015 at 3:49 PM, Doug Anderson <dianders@chromium.org> wrote: >> I've got that coded up but I'm still testing it... If you want to try >> it too, you can find it at >> <https://chromium-review.googlesource.com/251341>. >> >> Note that without my patch I find that I _really_ need Addy's patch to >> make sure that the card isn't busy in setup_bus. With my patch Addy's >> code catches the card busy less often. I'm still trying to see if >> there's a way to totally remove the need for his setup_bus and still >> trying to grok all the patches flying around... > > Ah, this might be the magic needed: > > https://chromium-review.googlesource.com/251344 > > > I think that together with the previous patch things are happy for me > without any of Addy's patches, though it's the end of my work day and > I haven't given this nearly as much testing as I'd like. > > I'll continue testing tomorrow and then post both patches together upstream. OK, posted three patches upstream... A little hard to follow all the patches flying around (so I'll probably reply a few different places with the same info), but I believe that all of Addy's patches (with the exception of the one intended to fix mmc_test which needs to be spun by him for a bugfix) can be replaced with: * mmc: dw_mmc: Don't start commands while busy https://patchwork.kernel.org/patch/5858221/ * mmc: dw_mmc: Make sure we only adjust the clock when power is on https://patchwork.kernel.org/patch/5858261/ * mmc: dw_mmc: Give a good reset after we give power https://patchwork.kernel.org/patch/5858281/ In order to avoid further spreading info out among several patches, I'd request that you don't respond here but instead respond to my posted patches. Thanks! -Doug
Hi Doug, On Fri, Feb 20, 2015 at 5:19 AM, Doug Anderson <dianders@chromium.org> wrote: > Alim and Addy, > > On Sun, Feb 15, 2015 at 3:28 PM, Alim Akhtar <alim.akhtar@gmail.com> wrote: >> Hi Addy, >> >> On Sat, Feb 14, 2015 at 11:47 AM, Addy Ke <addy.ke@rock-chips.com> wrote: >>> As show in mmc_power_up(), in MMC_POWER_UP state, the voltage isn't >>> stable and we may get 'data busy' which can't be cleaned by resetting >>> all blocks. So we should not send command to update clock in this state. >>> >>> Signed-off-by: Addy Ke <addy.ke@rock-chips.com> >>> --- >>> drivers/mmc/host/dw_mmc.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>> index 4d2e3c2..3472f9b 100644 >>> --- a/drivers/mmc/host/dw_mmc.c >>> +++ b/drivers/mmc/host/dw_mmc.c >>> @@ -1102,7 +1102,8 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>> drv_data->set_ios(slot->host, ios); >>> >>> /* Slot specific timing and width adjustment */ >>> - dw_mci_setup_bus(slot, false); >>> + if (ios->power_mode != MMC_POWER_UP) >>> + dw_mci_setup_bus(slot, false); >>> >> This looks a HACK to me. >> If stabilizing host voltage regulator is the problem, can you try out >> below patch, and see if this resolve your issue? > > Actually, IMHO Alim's patch is more of a hack than Addy's. There's > already a 10ms delay between "power up" and "power on" in the MMC core > in mmc_power_up() state. That delay is commented as: > Well, my suggestion (adding 5ms in switch_volatge) was based on DW_MMC databook (V2.41a) section "7.4.1.2 Voltage switch Normal Scenario" step #7 which says:" After the 5ms timer expires, the host voltage regulator is stable". PS: I have limited to no access of my mails and workstation until March 9th, so replies will be slow. > /* > * This delay should be sufficient to allow the power supply > * to reach the minimum voltage. > */ > mmc_delay(10); > > That means that assuming that the voltage is stable in MMC_POWER_UP is > not valid anyway. > > > Addy's patch certainly needs more comments. In another context Olof suggested: > > /* > * Skip bus setup while voltage is still stabilizing. Instead, > * bus setup will be done at MMC_POWER_ON. > */ > > > ...thinking about this more, though: really the voltage should be > stabilized when the regulator call returns (see my comments below). > In actuality the "right" fix might be to just rearrange this function > a little not to turn the clock on _before_ we even try to turn the > voltage on. > > I've got that coded up but I'm still testing it... If you want to try > it too, you can find it at > <https://chromium-review.googlesource.com/251341>. > > Note that without my patch I find that I _really_ need Addy's patch to > make sure that the card isn't busy in setup_bus. With my patch Addy's > code catches the card busy less often. I'm still trying to see if > there's a way to totally remove the need for his setup_bus and still > trying to grok all the patches flying around... > > >> =========== >> [PATCH] mmc: dw_mmc: Wait for host voltage regulator to be stable >> >> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> >> --- >> drivers/mmc/host/dw_mmc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 4d2e3c2..dc10fbb 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -1202,6 +1202,9 @@ static int dw_mci_switch_voltage(struct mmc_host >> *mmc, struct mmc_ios *ios) >> } >> mci_writel(host, UHS_REG, uhs); >> >> + /* wait for 5ms so that host voltage regulator is stable */ >> + usleep_range(5000, 5500); >> + > > Alim: if you have some other instance where you actually need VQMMC to > stabilize it should probably be done in a different way. If I > understand correctly it is the regulator core's job to make sure that > voltage is stable before returning. If you have a gpio-regulator you > may be able to use "startup-delay-us" to specify how long the > regulator takes to come up. You could also look at > 'regulator-enable-ramp-delay' > > -Doug
Hi, On 02/25/2015 04:52 PM, Alim Akhtar wrote: > Hi Doug, > > > On Fri, Feb 20, 2015 at 5:19 AM, Doug Anderson <dianders@chromium.org> wrote: >> Alim and Addy, >> >> On Sun, Feb 15, 2015 at 3:28 PM, Alim Akhtar <alim.akhtar@gmail.com> wrote: >>> Hi Addy, >>> >>> On Sat, Feb 14, 2015 at 11:47 AM, Addy Ke <addy.ke@rock-chips.com> wrote: >>>> As show in mmc_power_up(), in MMC_POWER_UP state, the voltage isn't >>>> stable and we may get 'data busy' which can't be cleaned by resetting >>>> all blocks. So we should not send command to update clock in this state. >>>> >>>> Signed-off-by: Addy Ke <addy.ke@rock-chips.com> >>>> --- >>>> drivers/mmc/host/dw_mmc.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>>> index 4d2e3c2..3472f9b 100644 >>>> --- a/drivers/mmc/host/dw_mmc.c >>>> +++ b/drivers/mmc/host/dw_mmc.c >>>> @@ -1102,7 +1102,8 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>>> drv_data->set_ios(slot->host, ios); >>>> >>>> /* Slot specific timing and width adjustment */ >>>> - dw_mci_setup_bus(slot, false); >>>> + if (ios->power_mode != MMC_POWER_UP) >>>> + dw_mci_setup_bus(slot, false); >>>> >>> This looks a HACK to me. >>> If stabilizing host voltage regulator is the problem, can you try out >>> below patch, and see if this resolve your issue? >> >> Actually, IMHO Alim's patch is more of a hack than Addy's. There's >> already a 10ms delay between "power up" and "power on" in the MMC core >> in mmc_power_up() state. That delay is commented as: >> > Well, my suggestion (adding 5ms in switch_volatge) was based on DW_MMC > databook (V2.41a) section "7.4.1.2 Voltage switch Normal Scenario" > step #7 which says:" After the 5ms timer expires, the host voltage > regulator is stable". if you want to stable power, How about using SDMMC_CMD_INIT flag? It waits for 80-clock before sending command.(To stable power) - You can refer to CMD register description. Best Regards, Jaehoon Chung > > PS: I have limited to no access of my mails and workstation until > March 9th, so replies will be slow. > >> /* >> * This delay should be sufficient to allow the power supply >> * to reach the minimum voltage. >> */ >> mmc_delay(10); >> >> That means that assuming that the voltage is stable in MMC_POWER_UP is >> not valid anyway. >> >> >> Addy's patch certainly needs more comments. In another context Olof suggested: >> >> /* >> * Skip bus setup while voltage is still stabilizing. Instead, >> * bus setup will be done at MMC_POWER_ON. >> */ >> >> >> ...thinking about this more, though: really the voltage should be >> stabilized when the regulator call returns (see my comments below). >> In actuality the "right" fix might be to just rearrange this function >> a little not to turn the clock on _before_ we even try to turn the >> voltage on. >> >> I've got that coded up but I'm still testing it... If you want to try >> it too, you can find it at >> <https://chromium-review.googlesource.com/251341>. >> >> Note that without my patch I find that I _really_ need Addy's patch to >> make sure that the card isn't busy in setup_bus. With my patch Addy's >> code catches the card busy less often. I'm still trying to see if >> there's a way to totally remove the need for his setup_bus and still >> trying to grok all the patches flying around... >> >> >>> =========== >>> [PATCH] mmc: dw_mmc: Wait for host voltage regulator to be stable >>> >>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> >>> --- >>> drivers/mmc/host/dw_mmc.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>> index 4d2e3c2..dc10fbb 100644 >>> --- a/drivers/mmc/host/dw_mmc.c >>> +++ b/drivers/mmc/host/dw_mmc.c >>> @@ -1202,6 +1202,9 @@ static int dw_mci_switch_voltage(struct mmc_host >>> *mmc, struct mmc_ios *ios) >>> } >>> mci_writel(host, UHS_REG, uhs); >>> >>> + /* wait for 5ms so that host voltage regulator is stable */ >>> + usleep_range(5000, 5500); >>> + >> >> Alim: if you have some other instance where you actually need VQMMC to >> stabilize it should probably be done in a different way. If I >> understand correctly it is the regulator core's job to make sure that >> voltage is stable before returning. If you have a gpio-regulator you >> may be able to use "startup-delay-us" to specify how long the >> regulator takes to come up. You could also look at >> 'regulator-enable-ramp-delay' >> >> -Doug > > >
Alim, On Tue, Feb 24, 2015 at 11:52 PM, Alim Akhtar <alim.akhtar@gmail.com> wrote: >>> This looks a HACK to me. >>> If stabilizing host voltage regulator is the problem, can you try out >>> below patch, and see if this resolve your issue? >> >> Actually, IMHO Alim's patch is more of a hack than Addy's. There's >> already a 10ms delay between "power up" and "power on" in the MMC core >> in mmc_power_up() state. That delay is commented as: >> > Well, my suggestion (adding 5ms in switch_volatge) was based on DW_MMC > databook (V2.41a) section "7.4.1.2 Voltage switch Normal Scenario" > step #7 which says:" After the 5ms timer expires, the host voltage > regulator is stable". So all of that should be handled by the core. Just reading the DW_MMC databook can be confusing because they don't differentiate between what's in the SDMMC spec and what's DW_MMC specific. Check out mmc_set_signal_voltage(), specifically: * During a signal voltage level switch, the clock must be gated * for 5 ms according to the SD spec -Doug
=========== [PATCH] mmc: dw_mmc: Wait for host voltage regulator to be stable Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> --- drivers/mmc/host/dw_mmc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 4d2e3c2..dc10fbb 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -1202,6 +1202,9 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) } mci_writel(host, UHS_REG, uhs); + /* wait for 5ms so that host voltage regulator is stable */ + usleep_range(5000, 5500); + return 0; }