Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221117094859.20582-1-quic_sartgarg@quicinc.com>
In-Reply-To: <20221117094859.20582-1-quic_sartgarg@quicinc.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 17 Nov 2022 12:07:01 +0100
Message-ID: <CAPDyKFqLjKOUwbAY5KXWK7g2xcWBQLW09nYoeVCxarfJPGNrTg@mail.gmail.com>
Subject: Re: [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post
 voltage switch
To:     Sarthak Garg <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,
        quic_rampraka@quicinc.com, quic_pragalla@quicinc.com,
        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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>