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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2983 bytes --]

On Wed, Sep 14, 2022 at 09:21:07PM +0300, Adrian Hunter wrote:
> On 14/09/22 15:20, Thierry Reding wrote:
> > On Wed, Sep 14, 2022 at 03:26:27PM +0530, Prathamesh Shete wrote:
> >> In case of error condition to avoid system crash
> >> Tegra SDMMC controller requires CMD and DAT resets
> >> issued together.
> > 
> > It might be worth specifying exactly what "system crash" means. Does
> > this always happen (i.e. do we have a problem right now?) or are there
> > specific circumstances that cause the crash.
> > 
> >> 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       | 11 ++++++++---
> >>  drivers/mmc/host/sdhci.h       |  2 ++
> >>  3 files changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> >> index b66b0cc51497..7d16dc41fe91 100644
> >> --- a/drivers/mmc/host/sdhci-tegra.c
> >> +++ b/drivers/mmc/host/sdhci-tegra.c
> >> @@ -1530,7 +1530,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 7689ffec5ad1..289fa8ae4866 100644
> >> --- a/drivers/mmc/host/sdhci.c
> >> +++ b/drivers/mmc/host/sdhci.c
> >> @@ -3063,9 +3063,14 @@ static bool sdhci_request_done(struct sdhci_host *host)
> >>  		 * Spec says we should do both at the same time, but Ricoh
> >>  		 * controllers do not like that.
> >>  		 */
> > 
> > The comment above seems to indicate that the current behavior (i.e.
> > splitting the CMD and DATA resets) is actually the quirk, so I wonder if
> > this perhaps should be reversed? I suppose it could be difficult to
> > track down the exact controllers that need the separate resets, but this
> > might be worth doing. It's possible that other controllers might run
> > into the same issue that we are if they work strictly to the spec.
> > 
> > Adrian, any ideas on how much of this is just cargo-culted? Do we play
> > it safe and do the "double workaround" or do we want to attempt to
> > rectify this by adding a Ricoh-specific quirk?
> 
> It is a good question, but it has been that way for a very long time,
> and the spec tends to document them separately anyway, so it doesn't
> seem there is much reason to change.

Fair enough. Prathamesh, perhaps revise the comment above as part of
this patch because with the change below it now sounds a bit confusing.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]