diff mbox series

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

Message ID 20220914095628.26093-3-pshete@nvidia.com
State Superseded
Headers show
Series [v2,1/4] mmc: sdhci-tegra: Separate T19x and T23x SoC data | expand

Commit Message

Prathamesh Shete Sept. 14, 2022, 9:56 a.m. UTC 
In case of error condition to avoid system crash
Tegra SDMMC controller requires CMD and DAT resets
issued 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       | 11 ++++++++---
 drivers/mmc/host/sdhci.h       |  2 ++
 3 files changed, 12 insertions(+), 4 deletions(-)

Comments

Adrian Hunter Sept. 14, 2022, 6:21 p.m. UTC | #1 
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
>>
Thierry Reding Sept. 15, 2022, 10:56 a.m. UTC | #2 
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
diff mbox series

Patch

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.
 		 */
-		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 */