Re: [PATCH v4 3/4] mmc: sdhci-tegra: Issue CMD and DAT resets together

Adrian Hunter
2022-09-27T10:23:56Z

On 27/09/22 13:09, Prathamesh Shete wrote:
> In case of error condition to avoid system crash
> Tegra SDMMC controller requires CMD and DAT resets
> issued together. SDHCI controller FSM goes into
> bad state due to rapid SD card hot-plug event.
> Issuing reset on the CMD FSM before DATA FSM results
> in kernel panic, hence add support to issue CMD and
> DAT resets together.
> This is applicable to Tegra186 and later chips.
>
> Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
> drivers/mmc/host/sdhci-tegra.c | 3 ++-
> drivers/mmc/host/sdhci.c | 8 +++++++-
> drivers/mmc/host/sdhci.h | 2 ++
> 3 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 46f37cc26dbb..61dc5ee0726d 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -1536,7 +1536,8 @@ static const struct sdhci_pltfm_data sdhci_tegra186_pdata = {
> SDHCI_QUIRK_NO_HISPD_BIT |
> SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC |
> SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> + SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER,
> .ops = &tegra186_sdhci_ops,
> };
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 2b5dda521b0e..5123ec3fc74a 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -266,12 +266,14 @@ enum sdhci_reset_reason {
> SDHCI_RESET_FOR_TUNING_ABORT,
> SDHCI_RESET_FOR_CARD_REMOVED,
> SDHCI_RESET_FOR_CQE_RECOVERY,
> + SDHCI_RESET_FOR_CMD_DAT_TOGETHER,
> };
>
> static void sdhci_reset_for_reason(struct sdhci_host *host, enum sdhci_reset_reason reason)
> {
> switch (reason) {
> case SDHCI_RESET_FOR_INIT:
> + case SDHCI_RESET_FOR_CMD_DAT_TOGETHER:
> sdhci_do_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
> break;
> case SDHCI_RESET_FOR_REQUEST_ERROR:
> @@ -3084,7 +3086,11 @@ static bool sdhci_request_done(struct sdhci_host *host)
> /* This is to force an update */
> host->ops->set_clock(host, host->clock);
>
> - sdhci_reset_for(host, REQUEST_ERROR);
> + if (host->quirks2 &
> + SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER)
> + sdhci_reset_for(host, CMD_DAT_TOGETHER);
> + else
> + sdhci_reset_for(host, REQUEST_ERROR);

This is what I am trying to avoid because it is inconsistent.
Consider:
- why is the quirk needed for some REQUEST_ERRORs but not others?
- or why is the quirk not needed for all CMD / DAT resets?

So, I was expecting the quirk to be applied in sdhci_reset_for_reason()
not here.

>
> host->pending_reset = false;
> }
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d750c464bd1e..6a5766774b05 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -478,6 +478,8 @@ struct sdhci_host {
> * block count.
> */
> #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT (1<<18)
> +/* Issue CMD and DATA reset together */
> +#define SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER (1<<19)
>
> int irq; /* Device IRQ */
> void __iomem *ioaddr; /* Mapped address */