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@xxxxxxxxxx>
>> Signed-off-by: Prathamesh Shete <pshete@xxxxxxxxxx>
>> ---
>> 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
>>
