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 <[email protected]>
---
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 Thu, 17 Nov 2022 at 10:49, Sarthak Garg <[email protected]> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
> Sent: Thursday, November 17, 2022 4:37 PM
> To: Sarthak Garg (QUIC) <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Ram Prakash Gupta
> (QUIC) <[email protected]>; Pradeep Pragallapati (QUIC)
> <[email protected]>; Sayali Lokhande (QUIC)
> <[email protected]>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; Darrick J. Wong <[email protected]>; Jason A. Donenfeld
> <[email protected]>; Greg Kroah-Hartman <[email protected]>;
> Shaik Sajida Bhanu <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
>>
>> 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/[email protected]/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
>>>
>