| Message ID | 20221117094859.20582-1-quic_sartgarg@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch | expand |
On Thu, 17 Nov 2022 at 10:49, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: > > As per spec we should wait for 1ms after providing the SD clock to the > card again as part of voltage switch sequence but there seems to be a > violation here. Clocks are getting gated before 1ms as part of > sdhci_set_ios function where we try to reset SD Clock Enable by > resetting SDHCI_CLOCK_CARD_EN bit in SDHCI_CLOCK_CONTROL register > leading to voltage switch failures for specific SD cards. > Below ftraces also confirms the above understanding : > > 9.511367: mmc_host_set_uhs_voltage: mmc1 called > 9.511369: mmc_set_ios: mmc1: clock 0Hz busmode 2 powermode 2 cs 0 > Vdd 18 width 1 timing 0 > 9.511370: sdhci_set_ios: mmc1 called > 9.511370: sdhci_set_ios: mmc1 setting clock ios->clock: 0 host->clock: > 400000 > 9.511372: sdhci_msm_set_clock: mmc1 clock: 0 > 9.511394: sdhci_set_ios: mmc1 gating clocks by writing > ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > 9.511413: sdhci_msm_set_clock: mmc1 clock: 0 > 9.511423: mmc_set_signal_voltage: mmc1 called > 9.533816: mmc_set_ios: mmc1: clock 400000Hz busmode 2 powermode 2 cs 0 > Vdd 18 width 1 timing 0 > 9.533820: sdhci_set_ios: mmc1 called > 9.533822: sdhci_set_ios: mmc1 setting clock ios->clock: 400000 > host->clock: 0 > 9.533826: sdhci_msm_set_clock: mmc1 clock: 400000 > 9.533925: sdhci_enable_clk: mmc1 Enabling clocks by writing > SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > 9.533950: sdhci_set_ios: mmc1 gating clocks by writing > ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > 9.533973: sdhci_msm_set_clock: mmc1 clock: 400000 > 9.534043: sdhci_enable_clk: mmc1 Enabling clocks by writing > SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > 9.534045: mmc_host_set_uhs_voltage: mmc1 Done > > Wait for 1ms after enabling the clock post voltage switch to make sure > clock is kept alive for alteast 1ms as per spec. > > Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> I don't know the exact behaviour of sdhci around this, so I will defer to Adrian's input for that. However, let me point out that in mmc_set_uhs_voltage() we are trying to take the 1ms into account. More precisely, after mmc_host_set_uhs_voltage() has been called to switch the voltage to 1.8V and to re-enable the clock, mmc_set_uhs_voltage() does a "mmc_delay(1)" and then it calls the ->card_busy() ops to check that card doesn't signal busy by holding DAT0 low. I understand that the code in mmc_set_uhs_voltage(), expects the host to be rather dumb from the HW perspective, which may not always be the case. Although, I would rather avoid introducing new host flags, along what you propose in the $subject patch. If this can't be managed in sdhci, without some new help from the mmc core, I would rather suggest that we introduce a new host callback that can be used to replace the entire part in mmc_host_set_uhs_voltage() (or something along those lines). Kind regards Uffe > --- > drivers/mmc/core/core.c | 4 ++++ > drivers/mmc/host/sdhci.c | 3 +++ > include/linux/mmc/host.h | 1 + > 3 files changed, 8 insertions(+) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index a1efda85c6f2..d63e00aab6cb 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1181,6 +1181,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) > host->ios.clock = 0; > mmc_set_ios(host); > > + host->doing_signal_voltage_switch = true; > + > if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) > return -EAGAIN; > > @@ -1189,6 +1191,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) > host->ios.clock = clock; > mmc_set_ios(host); > > + host->doing_signal_voltage_switch = false; > + > return 0; > } > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index fb6e9a81f198..ac7c254eef4b 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2312,6 +2312,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > host->ops->set_clock(host, ios->clock); > host->clock = ios->clock; > > + if (mmc->doing_signal_voltage_switch) > + usleep_range(1000, 1250); > + > if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && > host->clock) { > host->timeout_clk = mmc->actual_clock ? > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 8fdd3cf971a3..06c88cd7a8bf 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -521,6 +521,7 @@ struct mmc_host { > bool hsq_enabled; > > u32 err_stats[MMC_ERR_MAX]; > + bool doing_signal_voltage_switch; > unsigned long private[] ____cacheline_aligned; > }; > > -- > 2.17.1 >
On 17/11/22 13:07, Ulf Hansson wrote: > On Thu, 17 Nov 2022 at 10:49, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: >> >> As per spec we should wait for 1ms after providing the SD clock to the >> card again as part of voltage switch sequence but there seems to be a >> violation here. Clocks are getting gated before 1ms as part of >> sdhci_set_ios function where we try to reset SD Clock Enable by >> resetting SDHCI_CLOCK_CARD_EN bit in SDHCI_CLOCK_CONTROL register >> leading to voltage switch failures for specific SD cards. >> Below ftraces also confirms the above understanding : >> >> 9.511367: mmc_host_set_uhs_voltage: mmc1 called >> 9.511369: mmc_set_ios: mmc1: clock 0Hz busmode 2 powermode 2 cs 0 >> Vdd 18 width 1 timing 0 >> 9.511370: sdhci_set_ios: mmc1 called >> 9.511370: sdhci_set_ios: mmc1 setting clock ios->clock: 0 host->clock: >> 400000 >> 9.511372: sdhci_msm_set_clock: mmc1 clock: 0 >> 9.511394: sdhci_set_ios: mmc1 gating clocks by writing >> ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >> 9.511413: sdhci_msm_set_clock: mmc1 clock: 0 >> 9.511423: mmc_set_signal_voltage: mmc1 called >> 9.533816: mmc_set_ios: mmc1: clock 400000Hz busmode 2 powermode 2 cs 0 >> Vdd 18 width 1 timing 0 >> 9.533820: sdhci_set_ios: mmc1 called >> 9.533822: sdhci_set_ios: mmc1 setting clock ios->clock: 400000 >> host->clock: 0 >> 9.533826: sdhci_msm_set_clock: mmc1 clock: 400000 >> 9.533925: sdhci_enable_clk: mmc1 Enabling clocks by writing >> SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >> 9.533950: sdhci_set_ios: mmc1 gating clocks by writing >> ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >> 9.533973: sdhci_msm_set_clock: mmc1 clock: 400000 >> 9.534043: sdhci_enable_clk: mmc1 Enabling clocks by writing >> SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >> 9.534045: mmc_host_set_uhs_voltage: mmc1 Done >> >> Wait for 1ms after enabling the clock post voltage switch to make sure >> clock is kept alive for alteast 1ms as per spec. >> >> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> > > I don't know the exact behaviour of sdhci around this, so I will defer > to Adrian's input for that. sdhci_set_ios() seems to be mucking around with the clock way more than necessary. I'll go over it and see what can be done. > > However, let me point out that in mmc_set_uhs_voltage() we are trying > to take the 1ms into account. More precisely, after > mmc_host_set_uhs_voltage() has been called to switch the voltage to > 1.8V and to re-enable the clock, mmc_set_uhs_voltage() does a > "mmc_delay(1)" and then it calls the ->card_busy() ops to check that > card doesn't signal busy by holding DAT0 low. > > I understand that the code in mmc_set_uhs_voltage(), expects the host > to be rather dumb from the HW perspective, which may not always be the > case. Although, I would rather avoid introducing new host flags, along > what you propose in the $subject patch. If this can't be managed in > sdhci, without some new help from the mmc core, I would rather suggest > that we introduce a new host callback that can be used to replace the > entire part in mmc_host_set_uhs_voltage() (or something along those > lines). > > Kind regards > Uffe > >> --- >> drivers/mmc/core/core.c | 4 ++++ >> drivers/mmc/host/sdhci.c | 3 +++ >> include/linux/mmc/host.h | 1 + >> 3 files changed, 8 insertions(+) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index a1efda85c6f2..d63e00aab6cb 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -1181,6 +1181,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) >> host->ios.clock = 0; >> mmc_set_ios(host); >> >> + host->doing_signal_voltage_switch = true; >> + >> if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) >> return -EAGAIN; >> >> @@ -1189,6 +1191,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) >> host->ios.clock = clock; >> mmc_set_ios(host); >> >> + host->doing_signal_voltage_switch = false; >> + >> return 0; >> } >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index fb6e9a81f198..ac7c254eef4b 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -2312,6 +2312,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> host->ops->set_clock(host, ios->clock); >> host->clock = ios->clock; >> >> + if (mmc->doing_signal_voltage_switch) >> + usleep_range(1000, 1250); >> + >> if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && >> host->clock) { >> host->timeout_clk = mmc->actual_clock ? >> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >> index 8fdd3cf971a3..06c88cd7a8bf 100644 >> --- a/include/linux/mmc/host.h >> +++ b/include/linux/mmc/host.h >> @@ -521,6 +521,7 @@ struct mmc_host { >> bool hsq_enabled; >> >> u32 err_stats[MMC_ERR_MAX]; >> + bool doing_signal_voltage_switch; >> unsigned long private[] ____cacheline_aligned; >> }; >> >> -- >> 2.17.1 >>
> -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Thursday, November 17, 2022 4:37 PM > To: Sarthak Garg (QUIC) <quic_sartgarg@quicinc.com> > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-arm-msm@vger.kernel.org; Ram Prakash Gupta > (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) > <quic_pragalla@quicinc.com>; Sayali Lokhande (QUIC) > <quic_sayalil@quicinc.com>; Wolfram Sang <wsa+renesas@sang- > engineering.com>; Darrick J. Wong <djwong@kernel.org>; Jason A. Donenfeld > <Jason@zx2c4.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; > Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com> > Subject: Re: [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post > voltage switch > > On Thu, 17 Nov 2022 at 10:49, Sarthak Garg <quic_sartgarg@quicinc.com> > wrote: > > > > As per spec we should wait for 1ms after providing the SD clock to the > > card again as part of voltage switch sequence but there seems to be a > > violation here. Clocks are getting gated before 1ms as part of > > sdhci_set_ios function where we try to reset SD Clock Enable by > > resetting SDHCI_CLOCK_CARD_EN bit in SDHCI_CLOCK_CONTROL register > > leading to voltage switch failures for specific SD cards. > > Below ftraces also confirms the above understanding : > > > > 9.511367: mmc_host_set_uhs_voltage: mmc1 called > > 9.511369: mmc_set_ios: mmc1: clock 0Hz busmode 2 powermode 2 cs 0 Vdd > > 18 width 1 timing 0 > > 9.511370: sdhci_set_ios: mmc1 called > > 9.511370: sdhci_set_ios: mmc1 setting clock ios->clock: 0 host->clock: > > 400000 > > 9.511372: sdhci_msm_set_clock: mmc1 clock: 0 > > 9.511394: sdhci_set_ios: mmc1 gating clocks by writing > > ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > > 9.511413: sdhci_msm_set_clock: mmc1 clock: 0 > > 9.511423: mmc_set_signal_voltage: mmc1 called > > 9.533816: mmc_set_ios: mmc1: clock 400000Hz busmode 2 powermode 2 cs 0 > > Vdd 18 width 1 timing 0 > > 9.533820: sdhci_set_ios: mmc1 called > > 9.533822: sdhci_set_ios: mmc1 setting clock ios->clock: 400000 > > host->clock: 0 > > 9.533826: sdhci_msm_set_clock: mmc1 clock: 400000 > > 9.533925: sdhci_enable_clk: mmc1 Enabling clocks by writing > > SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > > 9.533950: sdhci_set_ios: mmc1 gating clocks by writing > > ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > > 9.533973: sdhci_msm_set_clock: mmc1 clock: 400000 > > 9.534043: sdhci_enable_clk: mmc1 Enabling clocks by writing > > SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > > 9.534045: mmc_host_set_uhs_voltage: mmc1 Done > > > > Wait for 1ms after enabling the clock post voltage switch to make sure > > clock is kept alive for alteast 1ms as per spec. > > > > Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> > > I don't know the exact behaviour of sdhci around this, so I will defer to Adrian's > input for that. > > However, let me point out that in mmc_set_uhs_voltage() we are trying to take > the 1ms into account. More precisely, after > mmc_host_set_uhs_voltage() has been called to switch the voltage to 1.8V and > to re-enable the clock, mmc_set_uhs_voltage() does a "mmc_delay(1)" and then > it calls the ->card_busy() ops to check that card doesn't signal busy by holding > DAT0 low. I agree ulf that we are adding a delay of 1ms in mmc_set_uhs_voltage after renabling the clocks but before that delay comes into picture clock's are getting gated as part of sdhci_set_ios function as part of below code. 2384 /* Reset SD Clock Enable */ 2385 clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); 2386 clk &= ~SDHCI_CLOCK_CARD_EN; 2387 sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > > I understand that the code in mmc_set_uhs_voltage(), expects the host to be > rather dumb from the HW perspective, which may not always be the case. > Although, I would rather avoid introducing new host flags, along what you > propose in the $subject patch. If this can't be managed in sdhci, without some > new help from the mmc core, I would rather suggest that we introduce a new > host callback that can be used to replace the entire part in > mmc_host_set_uhs_voltage() (or something along those lines). > > Kind regards > Uffe > > > --- > > drivers/mmc/core/core.c | 4 ++++ > > drivers/mmc/host/sdhci.c | 3 +++ > > include/linux/mmc/host.h | 1 + > > 3 files changed, 8 insertions(+) > > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index > > a1efda85c6f2..d63e00aab6cb 100644 > > --- a/drivers/mmc/core/core.c > > +++ b/drivers/mmc/core/core.c > > @@ -1181,6 +1181,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host > *host) > > host->ios.clock = 0; > > mmc_set_ios(host); > > > > + host->doing_signal_voltage_switch = true; > > + > > if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) > > return -EAGAIN; > > > > @@ -1189,6 +1191,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host > *host) > > host->ios.clock = clock; > > mmc_set_ios(host); > > > > + host->doing_signal_voltage_switch = false; > > + > > return 0; > > } > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index > > fb6e9a81f198..ac7c254eef4b 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -2312,6 +2312,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > mmc_ios *ios) > > host->ops->set_clock(host, ios->clock); > > host->clock = ios->clock; > > > > + if (mmc->doing_signal_voltage_switch) > > + usleep_range(1000, 1250); > > + > > if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && > > host->clock) { > > host->timeout_clk = mmc->actual_clock ? > > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index > > 8fdd3cf971a3..06c88cd7a8bf 100644 > > --- a/include/linux/mmc/host.h > > +++ b/include/linux/mmc/host.h > > @@ -521,6 +521,7 @@ struct mmc_host { > > bool hsq_enabled; > > > > u32 err_stats[MMC_ERR_MAX]; > > + bool doing_signal_voltage_switch; > > unsigned long private[] ____cacheline_aligned; > > }; > > > > -- > > 2.17.1 > >
On 17/11/22 17:23, Adrian Hunter wrote: > On 17/11/22 13:07, Ulf Hansson wrote: >> On Thu, 17 Nov 2022 at 10:49, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: >>> >>> As per spec we should wait for 1ms after providing the SD clock to the >>> card again as part of voltage switch sequence but there seems to be a >>> violation here. Clocks are getting gated before 1ms as part of >>> sdhci_set_ios function where we try to reset SD Clock Enable by >>> resetting SDHCI_CLOCK_CARD_EN bit in SDHCI_CLOCK_CONTROL register >>> leading to voltage switch failures for specific SD cards. >>> Below ftraces also confirms the above understanding : >>> >>> 9.511367: mmc_host_set_uhs_voltage: mmc1 called >>> 9.511369: mmc_set_ios: mmc1: clock 0Hz busmode 2 powermode 2 cs 0 >>> Vdd 18 width 1 timing 0 >>> 9.511370: sdhci_set_ios: mmc1 called >>> 9.511370: sdhci_set_ios: mmc1 setting clock ios->clock: 0 host->clock: >>> 400000 >>> 9.511372: sdhci_msm_set_clock: mmc1 clock: 0 >>> 9.511394: sdhci_set_ios: mmc1 gating clocks by writing >>> ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >>> 9.511413: sdhci_msm_set_clock: mmc1 clock: 0 >>> 9.511423: mmc_set_signal_voltage: mmc1 called >>> 9.533816: mmc_set_ios: mmc1: clock 400000Hz busmode 2 powermode 2 cs 0 >>> Vdd 18 width 1 timing 0 >>> 9.533820: sdhci_set_ios: mmc1 called >>> 9.533822: sdhci_set_ios: mmc1 setting clock ios->clock: 400000 >>> host->clock: 0 >>> 9.533826: sdhci_msm_set_clock: mmc1 clock: 400000 >>> 9.533925: sdhci_enable_clk: mmc1 Enabling clocks by writing >>> SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >>> 9.533950: sdhci_set_ios: mmc1 gating clocks by writing >>> ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >>> 9.533973: sdhci_msm_set_clock: mmc1 clock: 400000 >>> 9.534043: sdhci_enable_clk: mmc1 Enabling clocks by writing >>> SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >>> 9.534045: mmc_host_set_uhs_voltage: mmc1 Done >>> >>> Wait for 1ms after enabling the clock post voltage switch to make sure >>> clock is kept alive for alteast 1ms as per spec. >>> >>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> >> >> I don't know the exact behaviour of sdhci around this, so I will defer >> to Adrian's input for that. > > sdhci_set_ios() seems to be mucking around with the clock way > more than necessary. I'll go over it and see what can be done. I have now sent some patches: https://lore.kernel.org/linux-mmc/20221124170649.63851-1-adrian.hunter@intel.com/T/#t > >> >> However, let me point out that in mmc_set_uhs_voltage() we are trying >> to take the 1ms into account. More precisely, after >> mmc_host_set_uhs_voltage() has been called to switch the voltage to >> 1.8V and to re-enable the clock, mmc_set_uhs_voltage() does a >> "mmc_delay(1)" and then it calls the ->card_busy() ops to check that >> card doesn't signal busy by holding DAT0 low. >> >> I understand that the code in mmc_set_uhs_voltage(), expects the host >> to be rather dumb from the HW perspective, which may not always be the >> case. Although, I would rather avoid introducing new host flags, along >> what you propose in the $subject patch. If this can't be managed in >> sdhci, without some new help from the mmc core, I would rather suggest >> that we introduce a new host callback that can be used to replace the >> entire part in mmc_host_set_uhs_voltage() (or something along those >> lines). >> >> Kind regards >> Uffe >> >>> --- >>> drivers/mmc/core/core.c | 4 ++++ >>> drivers/mmc/host/sdhci.c | 3 +++ >>> include/linux/mmc/host.h | 1 + >>> 3 files changed, 8 insertions(+) >>> >>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >>> index a1efda85c6f2..d63e00aab6cb 100644 >>> --- a/drivers/mmc/core/core.c >>> +++ b/drivers/mmc/core/core.c >>> @@ -1181,6 +1181,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) >>> host->ios.clock = 0; >>> mmc_set_ios(host); >>> >>> + host->doing_signal_voltage_switch = true; >>> + >>> if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) >>> return -EAGAIN; >>> >>> @@ -1189,6 +1191,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) >>> host->ios.clock = clock; >>> mmc_set_ios(host); >>> >>> + host->doing_signal_voltage_switch = false; >>> + >>> return 0; >>> } >>> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>> index fb6e9a81f198..ac7c254eef4b 100644 >>> --- a/drivers/mmc/host/sdhci.c >>> +++ b/drivers/mmc/host/sdhci.c >>> @@ -2312,6 +2312,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>> host->ops->set_clock(host, ios->clock); >>> host->clock = ios->clock; >>> >>> + if (mmc->doing_signal_voltage_switch) >>> + usleep_range(1000, 1250); >>> + >>> if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && >>> host->clock) { >>> host->timeout_clk = mmc->actual_clock ? >>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >>> index 8fdd3cf971a3..06c88cd7a8bf 100644 >>> --- a/include/linux/mmc/host.h >>> +++ b/include/linux/mmc/host.h >>> @@ -521,6 +521,7 @@ struct mmc_host { >>> bool hsq_enabled; >>> >>> u32 err_stats[MMC_ERR_MAX]; >>> + bool doing_signal_voltage_switch; >>> unsigned long private[] ____cacheline_aligned; >>> }; >>> >>> -- >>> 2.17.1 >>> >
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index a1efda85c6f2..d63e00aab6cb 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1181,6 +1181,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) host->ios.clock = 0; mmc_set_ios(host); + host->doing_signal_voltage_switch = true; + if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) return -EAGAIN; @@ -1189,6 +1191,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) host->ios.clock = clock; mmc_set_ios(host); + host->doing_signal_voltage_switch = false; + return 0; } diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index fb6e9a81f198..ac7c254eef4b 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2312,6 +2312,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) host->ops->set_clock(host, ios->clock); host->clock = ios->clock; + if (mmc->doing_signal_voltage_switch) + usleep_range(1000, 1250); + if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && host->clock) { host->timeout_clk = mmc->actual_clock ? diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 8fdd3cf971a3..06c88cd7a8bf 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -521,6 +521,7 @@ struct mmc_host { bool hsq_enabled; u32 err_stats[MMC_ERR_MAX]; + bool doing_signal_voltage_switch; unsigned long private[] ____cacheline_aligned; };
As per spec we should wait for 1ms after providing the SD clock to the card again as part of voltage switch sequence but there seems to be a violation here. Clocks are getting gated before 1ms as part of sdhci_set_ios function where we try to reset SD Clock Enable by resetting SDHCI_CLOCK_CARD_EN bit in SDHCI_CLOCK_CONTROL register leading to voltage switch failures for specific SD cards. Below ftraces also confirms the above understanding : 9.511367: mmc_host_set_uhs_voltage: mmc1 called 9.511369: mmc_set_ios: mmc1: clock 0Hz busmode 2 powermode 2 cs 0 Vdd 18 width 1 timing 0 9.511370: sdhci_set_ios: mmc1 called 9.511370: sdhci_set_ios: mmc1 setting clock ios->clock: 0 host->clock: 400000 9.511372: sdhci_msm_set_clock: mmc1 clock: 0 9.511394: sdhci_set_ios: mmc1 gating clocks by writing ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register 9.511413: sdhci_msm_set_clock: mmc1 clock: 0 9.511423: mmc_set_signal_voltage: mmc1 called 9.533816: mmc_set_ios: mmc1: clock 400000Hz busmode 2 powermode 2 cs 0 Vdd 18 width 1 timing 0 9.533820: sdhci_set_ios: mmc1 called 9.533822: sdhci_set_ios: mmc1 setting clock ios->clock: 400000 host->clock: 0 9.533826: sdhci_msm_set_clock: mmc1 clock: 400000 9.533925: sdhci_enable_clk: mmc1 Enabling clocks by writing SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register 9.533950: sdhci_set_ios: mmc1 gating clocks by writing ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register 9.533973: sdhci_msm_set_clock: mmc1 clock: 400000 9.534043: sdhci_enable_clk: mmc1 Enabling clocks by writing SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register 9.534045: mmc_host_set_uhs_voltage: mmc1 Done Wait for 1ms after enabling the clock post voltage switch to make sure clock is kept alive for alteast 1ms as per spec. Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> --- drivers/mmc/core/core.c | 4 ++++ drivers/mmc/host/sdhci.c | 3 +++ include/linux/mmc/host.h | 1 + 3 files changed, 8 insertions(+)