diff mbox series

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

Message ID 20220914095628.26093-3-pshete@nvidia.com (mailing list archive)
State New, archived
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

Thierry Reding Sept. 14, 2022, 12:20 p.m. UTC | #1 
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?

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
>
Adrian Hunter Sept. 14, 2022, 6:21 p.m. UTC | #2 
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 | #3 
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 */