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

[Adrian Hunter <adrian.hunter@intel.com>]

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.

> 
> Thierry
> 
>> -		sdhci_do_reset(host, SDHCI_RESET_CMD);
>> -		sdhci_do_reset(host, SDHCI_RESET_DATA);
>> -
>> +		if (host->quirks2 &
>> +			SDHCI_QUIRK2_ISSUE_CMD_DAT_RESET_TOGETHER) {
>> +			sdhci_do_reset(host, SDHCI_RESET_CMD |
>> +					SDHCI_RESET_DATA);
>> +		} else {
>> +			sdhci_do_reset(host, SDHCI_RESET_CMD);
>> +			sdhci_do_reset(host, SDHCI_RESET_DATA);
>> +		}
>>  		host->pending_reset = false;
>>  	}
>>  
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 95a08f09df30..8045308f7859 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -480,6 +480,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 */
>> -- 
>> 2.17.1
>>