RE: [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch

["Sarthak Garg (QUIC)" <quic_sartgarg@quicinc.com>]

> -----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
> >