* [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch @ 2022-11-17 9:48 Sarthak Garg 2022-11-17 11:07 ` Ulf Hansson 0 siblings, 1 reply; 5+ messages in thread From: Sarthak Garg @ 2022-11-17 9:48 UTC (permalink / raw) To: adrian.hunter, ulf.hansson Cc: linux-mmc, linux-kernel, linux-arm-msm, quic_rampraka, quic_pragalla, quic_sayalil, Sarthak Garg, Wolfram Sang, Darrick J. Wong, Jason A. Donenfeld, Greg Kroah-Hartman, Shaik Sajida Bhanu 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(+) 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch 2022-11-17 9:48 [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch Sarthak Garg @ 2022-11-17 11:07 ` Ulf Hansson 2022-11-17 15:23 ` Adrian Hunter 2022-11-18 5:18 ` Sarthak Garg (QUIC) 0 siblings, 2 replies; 5+ messages in thread From: Ulf Hansson @ 2022-11-17 11:07 UTC (permalink / raw) To: Sarthak Garg Cc: adrian.hunter, linux-mmc, linux-kernel, linux-arm-msm, quic_rampraka, quic_pragalla, quic_sayalil, Wolfram Sang, Darrick J. Wong, Jason A. Donenfeld, Greg Kroah-Hartman, Shaik Sajida Bhanu 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch 2022-11-17 11:07 ` Ulf Hansson @ 2022-11-17 15:23 ` Adrian Hunter 2022-11-24 17:11 ` Adrian Hunter 2022-11-18 5:18 ` Sarthak Garg (QUIC) 1 sibling, 1 reply; 5+ messages in thread From: Adrian Hunter @ 2022-11-17 15:23 UTC (permalink / raw) To: Ulf Hansson, Sarthak Garg Cc: linux-mmc, linux-kernel, linux-arm-msm, quic_rampraka, quic_pragalla, quic_sayalil, Wolfram Sang, Darrick J. Wong, Jason A. Donenfeld, Greg Kroah-Hartman, Shaik Sajida Bhanu 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 >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch 2022-11-17 15:23 ` Adrian Hunter @ 2022-11-24 17:11 ` Adrian Hunter 0 siblings, 0 replies; 5+ messages in thread From: Adrian Hunter @ 2022-11-24 17:11 UTC (permalink / raw) To: Ulf Hansson, Sarthak Garg Cc: linux-mmc, linux-kernel, linux-arm-msm, quic_rampraka, quic_pragalla, quic_sayalil, Wolfram Sang, Darrick J. Wong, Jason A. Donenfeld, Greg Kroah-Hartman, Shaik Sajida Bhanu 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 >>> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch 2022-11-17 11:07 ` Ulf Hansson 2022-11-17 15:23 ` Adrian Hunter @ 2022-11-18 5:18 ` Sarthak Garg (QUIC) 1 sibling, 0 replies; 5+ messages in thread From: Sarthak Garg (QUIC) @ 2022-11-18 5:18 UTC (permalink / raw) To: Ulf Hansson Cc: adrian.hunter, linux-mmc, linux-kernel, linux-arm-msm, Ram Prakash Gupta (QUIC), Pradeep Pragallapati (QUIC), Sayali Lokhande (QUIC), Wolfram Sang, Darrick J. Wong, Jason A. Donenfeld, Greg Kroah-Hartman, Shaik Sajida Bhanu > -----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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-24 17:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-17 9:48 [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch Sarthak Garg 2022-11-17 11:07 ` Ulf Hansson 2022-11-17 15:23 ` Adrian Hunter 2022-11-24 17:11 ` Adrian Hunter 2022-11-18 5:18 ` Sarthak Garg (QUIC)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®