Subject: [PATCH] mmc: rpmb: do not force a retune before RPMB switch

Requesting a retune before switching to the RPMB partition has been
observed to cause CRC errors on the RPMB reads (-EILSEQ).

Since RPMB reads can not be retried, the clients would be directly
affected by the errors.

This commit disables the request prior to RPMB switching while allowing
the pause interface to still request a retune before the pause for other
use cases.

This was verified with the sdhci-of-arasan driver (ZynqMP) configured
for HS200 using two separate eMMC cards (DG4064 and 064GB2). In both
cases, the error was easy to reproduce triggering every few tenths of
reads.

Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
drivers/mmc/core/block.c | 2 +-
drivers/mmc/core/host.c | 7 ++++---
drivers/mmc/core/host.h | 2 +-
3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index f9a5cffa64b1..1d69078ad9b2 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -859,7 +859,7 @@ static int mmc_blk_part_switch_pre(struct mmc_card *card,
if (ret)
return ret;
}
- mmc_retune_pause(card->host);
+ mmc_retune_pause(card->host, false);
}

return ret;
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 096093f7be00..a9b95aaa2235 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -119,13 +119,14 @@ void mmc_retune_enable(struct mmc_host *host)

/*
* Pause re-tuning for a small set of operations. The pause begins after the
- * next command and after first doing re-tuning.
+ * next command and, if retune is set, after first doing re-tuning.
*/
-void mmc_retune_pause(struct mmc_host *host)
+void mmc_retune_pause(struct mmc_host *host, bool retune)
{
if (!host->retune_paused) {
host->retune_paused = 1;
- mmc_retune_needed(host);
+ if (retune)
+ mmc_retune_needed(host);
mmc_retune_hold(host);
}
}
diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
index 48c4952512a5..321776b52270 100644
--- a/drivers/mmc/core/host.h
+++ b/drivers/mmc/core/host.h
@@ -18,7 +18,7 @@ void mmc_retune_disable(struct mmc_host *host);
void mmc_retune_hold(struct mmc_host *host);
void mmc_retune_release(struct mmc_host *host);
int mmc_retune(struct mmc_host *host);
-void mmc_retune_pause(struct mmc_host *host);
+void mmc_retune_pause(struct mmc_host *host, bool retune);
void mmc_retune_unpause(struct mmc_host *host);

static inline void mmc_retune_clear(struct mmc_host *host)
--
2.34.1


2023-12-04 16:22:23

by Avri Altman

[permalink] [raw]
Subject: RE: [PATCH] mmc: rpmb: do not force a retune before RPMB switch


> Requesting a retune before switching to the RPMB partition has been
> observed to cause CRC errors on the RPMB reads (-EILSEQ).
>
> Since RPMB reads can not be retried, the clients would be directly affected by
> the errors.
>
> This commit disables the request prior to RPMB switching while allowing the
> pause interface to still request a retune before the pause for other use cases.
>
> This was verified with the sdhci-of-arasan driver (ZynqMP) configured for
> HS200 using two separate eMMC cards (DG4064 and 064GB2). In both cases,
> the error was easy to reproduce triggering every few tenths of reads.
>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
Acked-by: Avri Altman <[email protected]>

> ---
> drivers/mmc/core/block.c | 2 +-
> drivers/mmc/core/host.c | 7 ++++---
> drivers/mmc/core/host.h | 2 +-
> 3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
> f9a5cffa64b1..1d69078ad9b2 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -859,7 +859,7 @@ static int mmc_blk_part_switch_pre(struct mmc_card
> *card,
> if (ret)
> return ret;
> }
> - mmc_retune_pause(card->host);
> + mmc_retune_pause(card->host, false);
> }
>
> return ret;
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index
> 096093f7be00..a9b95aaa2235 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -119,13 +119,14 @@ void mmc_retune_enable(struct mmc_host *host)
>
> /*
> * Pause re-tuning for a small set of operations. The pause begins after the
> - * next command and after first doing re-tuning.
> + * next command and, if retune is set, after first doing re-tuning.
> */
> -void mmc_retune_pause(struct mmc_host *host)
> +void mmc_retune_pause(struct mmc_host *host, bool retune)
> {
> if (!host->retune_paused) {
> host->retune_paused = 1;
> - mmc_retune_needed(host);
> + if (retune)
> + mmc_retune_needed(host);
> mmc_retune_hold(host);
> }
> }
> diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h index
> 48c4952512a5..321776b52270 100644
> --- a/drivers/mmc/core/host.h
> +++ b/drivers/mmc/core/host.h
> @@ -18,7 +18,7 @@ void mmc_retune_disable(struct mmc_host *host);
> void mmc_retune_hold(struct mmc_host *host); void
> mmc_retune_release(struct mmc_host *host); int mmc_retune(struct
> mmc_host *host); -void mmc_retune_pause(struct mmc_host *host);
> +void mmc_retune_pause(struct mmc_host *host, bool retune);
> void mmc_retune_unpause(struct mmc_host *host);
>
> static inline void mmc_retune_clear(struct mmc_host *host)
> --
> 2.34.1

2023-12-04 17:59:17

by Avri Altman

[permalink] [raw]
Subject: RE: [PATCH] mmc: rpmb: do not force a retune before RPMB switch

>
> Requesting a retune before switching to the RPMB partition has been
> observed to cause CRC errors on the RPMB reads (-EILSEQ).
>
> Since RPMB reads can not be retried, the clients would be directly affected by
> the errors.
>
> This commit disables the request prior to RPMB switching while allowing the
> pause interface to still request a retune before the pause for other use cases.
>
> This was verified with the sdhci-of-arasan driver (ZynqMP) configured for
> HS200 using two separate eMMC cards (DG4064 and 064GB2). In both cases,
> the error was easy to reproduce triggering every few tenths of reads.
>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> drivers/mmc/core/block.c | 2 +-
> drivers/mmc/core/host.c | 7 ++++---
> drivers/mmc/core/host.h | 2 +-
> 3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
> f9a5cffa64b1..1d69078ad9b2 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -859,7 +859,7 @@ static int mmc_blk_part_switch_pre(struct mmc_card
> *card,
> if (ret)
> return ret;
> }
> - mmc_retune_pause(card->host);
> + mmc_retune_pause(card->host, false);
> }
>
> return ret;
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index
> 096093f7be00..a9b95aaa2235 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -119,13 +119,14 @@ void mmc_retune_enable(struct mmc_host *host)
>
> /*
> * Pause re-tuning for a small set of operations. The pause begins after the
> - * next command and after first doing re-tuning.
> + * next command and, if retune is set, after first doing re-tuning.
> */
> -void mmc_retune_pause(struct mmc_host *host)
> +void mmc_retune_pause(struct mmc_host *host, bool retune)
> {
Since mmc_blk_part_switch_pre is the only caller of mmc_retune_pause,
How about just move those lines into it?

Thanks,
Avri

> if (!host->retune_paused) {
> host->retune_paused = 1;
> - mmc_retune_needed(host);
> + if (retune)
> + mmc_retune_needed(host);
> mmc_retune_hold(host);
> }
> }
> diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h index
> 48c4952512a5..321776b52270 100644
> --- a/drivers/mmc/core/host.h
> +++ b/drivers/mmc/core/host.h
> @@ -18,7 +18,7 @@ void mmc_retune_disable(struct mmc_host *host);
> void mmc_retune_hold(struct mmc_host *host); void
> mmc_retune_release(struct mmc_host *host); int mmc_retune(struct
> mmc_host *host); -void mmc_retune_pause(struct mmc_host *host);
> +void mmc_retune_pause(struct mmc_host *host, bool retune);
> void mmc_retune_unpause(struct mmc_host *host);
>
> static inline void mmc_retune_clear(struct mmc_host *host)
> --
> 2.34.1

Subject: Re: [PATCH] mmc: rpmb: do not force a retune before RPMB switch

On 04/12/23 17:58:45, Avri Altman wrote:
> >
> > Requesting a retune before switching to the RPMB partition has been
> > observed to cause CRC errors on the RPMB reads (-EILSEQ).
> >
> > Since RPMB reads can not be retried, the clients would be directly affected by
> > the errors.
> >
> > This commit disables the request prior to RPMB switching while allowing the
> > pause interface to still request a retune before the pause for other use cases.
> >
> > This was verified with the sdhci-of-arasan driver (ZynqMP) configured for
> > HS200 using two separate eMMC cards (DG4064 and 064GB2). In both cases,
> > the error was easy to reproduce triggering every few tenths of reads.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> > ---
> > drivers/mmc/core/block.c | 2 +-
> > drivers/mmc/core/host.c | 7 ++++---
> > drivers/mmc/core/host.h | 2 +-
> > 3 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
> > f9a5cffa64b1..1d69078ad9b2 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -859,7 +859,7 @@ static int mmc_blk_part_switch_pre(struct mmc_card
> > *card,
> > if (ret)
> > return ret;
> > }
> > - mmc_retune_pause(card->host);
> > + mmc_retune_pause(card->host, false);
> > }
> >
> > return ret;
> > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index
> > 096093f7be00..a9b95aaa2235 100644
> > --- a/drivers/mmc/core/host.c
> > +++ b/drivers/mmc/core/host.c
> > @@ -119,13 +119,14 @@ void mmc_retune_enable(struct mmc_host *host)
> >
> > /*
> > * Pause re-tuning for a small set of operations. The pause begins after the
> > - * next command and after first doing re-tuning.
> > + * next command and, if retune is set, after first doing re-tuning.
> > */
> > -void mmc_retune_pause(struct mmc_host *host)
> > +void mmc_retune_pause(struct mmc_host *host, bool retune)
> > {
> Since mmc_blk_part_switch_pre is the only caller of mmc_retune_pause,
> How about just move those lines into it?

+1 from me

I wasnt sure if there was some other plan for mmc_retune_pause in the
future hence why I didnt remove it and extended it instead.

Should I wait for Adrian's confirmation or just go ahead?.

Subject: Re: [PATCH] mmc: rpmb: do not force a retune before RPMB switch

On 04/12/23 19:14:01, Jorge Ramirez-Ortiz, Foundries wrote:
> On 04/12/23 17:58:45, Avri Altman wrote:
> > >
> > > Requesting a retune before switching to the RPMB partition has been
> > > observed to cause CRC errors on the RPMB reads (-EILSEQ).
> > >
> > > Since RPMB reads can not be retried, the clients would be directly affected by
> > > the errors.
> > >
> > > This commit disables the request prior to RPMB switching while allowing the
> > > pause interface to still request a retune before the pause for other use cases.
> > >
> > > This was verified with the sdhci-of-arasan driver (ZynqMP) configured for
> > > HS200 using two separate eMMC cards (DG4064 and 064GB2). In both cases,
> > > the error was easy to reproduce triggering every few tenths of reads.
> > >
> > > Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> > > ---
> > > drivers/mmc/core/block.c | 2 +-
> > > drivers/mmc/core/host.c | 7 ++++---
> > > drivers/mmc/core/host.h | 2 +-
> > > 3 files changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
> > > f9a5cffa64b1..1d69078ad9b2 100644
> > > --- a/drivers/mmc/core/block.c
> > > +++ b/drivers/mmc/core/block.c
> > > @@ -859,7 +859,7 @@ static int mmc_blk_part_switch_pre(struct mmc_card
> > > *card,
> > > if (ret)
> > > return ret;
> > > }
> > > - mmc_retune_pause(card->host);
> > > + mmc_retune_pause(card->host, false);
> > > }
> > >
> > > return ret;
> > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index
> > > 096093f7be00..a9b95aaa2235 100644
> > > --- a/drivers/mmc/core/host.c
> > > +++ b/drivers/mmc/core/host.c
> > > @@ -119,13 +119,14 @@ void mmc_retune_enable(struct mmc_host *host)
> > >
> > > /*
> > > * Pause re-tuning for a small set of operations. The pause begins after the
> > > - * next command and after first doing re-tuning.
> > > + * next command and, if retune is set, after first doing re-tuning.
> > > */
> > > -void mmc_retune_pause(struct mmc_host *host)
> > > +void mmc_retune_pause(struct mmc_host *host, bool retune)
> > > {
> > Since mmc_blk_part_switch_pre is the only caller of mmc_retune_pause,
> > How about just move those lines into it?
>
> +1 from me
>
> I wasnt sure if there was some other plan for mmc_retune_pause in the
> future hence why I didnt remove it and extended it instead.
>
> Should I wait for Adrian's confirmation or just go ahead?.

just making sure this is not falling through the cracks.

2023-12-05 20:12:42

by Adrian Hunter

[permalink] [raw]
Subject: Re: [PATCH] mmc: rpmb: do not force a retune before RPMB switch

On 4/12/23 17:01, Jorge Ramirez-Ortiz wrote:
> Requesting a retune before switching to the RPMB partition has been
> observed to cause CRC errors on the RPMB reads (-EILSEQ).

There are still 2 concerns:
1) We don't really know the root cause. Have you determined
if here are CRC errors in the main partition also?
2) Forcing this on everyone

The original idea was that because re-tuning cannot be
done in RPMB, the need to re-rune in RPMB could be avoided
by always re-tuning before switching to RPMB and then switching
straight back. IIRC re-tuning should guarantee at least 4MB
more I/O without issue.

The alternative to dropping re-tuning in this case could
be to add a retry loop for MMC_DRV_OP_IOCTL_RPMB if the error
is -EILSEQ


>
> Since RPMB reads can not be retried, the clients would be directly
> affected by the errors.
>
> This commit disables the request prior to RPMB switching while allowing
> the pause interface to still request a retune before the pause for other
> use cases.
>
> This was verified with the sdhci-of-arasan driver (ZynqMP) configured
> for HS200 using two separate eMMC cards (DG4064 and 064GB2). In both
> cases, the error was easy to reproduce triggering every few tenths of
> reads.
>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> drivers/mmc/core/block.c | 2 +-
> drivers/mmc/core/host.c | 7 ++++---
> drivers/mmc/core/host.h | 2 +-
> 3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index f9a5cffa64b1..1d69078ad9b2 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -859,7 +859,7 @@ static int mmc_blk_part_switch_pre(struct mmc_card *card,
> if (ret)
> return ret;
> }
> - mmc_retune_pause(card->host);
> + mmc_retune_pause(card->host, false);
> }
>
> return ret;
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 096093f7be00..a9b95aaa2235 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -119,13 +119,14 @@ void mmc_retune_enable(struct mmc_host *host)
>
> /*
> * Pause re-tuning for a small set of operations. The pause begins after the
> - * next command and after first doing re-tuning.
> + * next command and, if retune is set, after first doing re-tuning.
> */
> -void mmc_retune_pause(struct mmc_host *host)
> +void mmc_retune_pause(struct mmc_host *host, bool retune)
> {
> if (!host->retune_paused) {
> host->retune_paused = 1;
> - mmc_retune_needed(host);
> + if (retune)
> + mmc_retune_needed(host);

Better to just drop mmc_retune_needed(host);

> mmc_retune_hold(host);

There is still a small chance that re-tuning
is needed anyway in which case it will still be done.

> }
> }
> diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
> index 48c4952512a5..321776b52270 100644
> --- a/drivers/mmc/core/host.h
> +++ b/drivers/mmc/core/host.h
> @@ -18,7 +18,7 @@ void mmc_retune_disable(struct mmc_host *host);
> void mmc_retune_hold(struct mmc_host *host);
> void mmc_retune_release(struct mmc_host *host);
> int mmc_retune(struct mmc_host *host);
> -void mmc_retune_pause(struct mmc_host *host);
> +void mmc_retune_pause(struct mmc_host *host, bool retune);
> void mmc_retune_unpause(struct mmc_host *host);
>
> static inline void mmc_retune_clear(struct mmc_host *host)

2023-12-05 20:14:56

by Adrian Hunter

[permalink] [raw]
Subject: Re: [PATCH] mmc: rpmb: do not force a retune before RPMB switch

On 5/12/23 22:12, Adrian Hunter wrote:
> On 4/12/23 17:01, Jorge Ramirez-Ortiz wrote:
>> Requesting a retune before switching to the RPMB partition has been
>> observed to cause CRC errors on the RPMB reads (-EILSEQ).
>
> There are still 2 concerns:
> 1) We don't really know the root cause. Have you determined
> if here are CRC errors in the main partition also?
> 2) Forcing this on everyone
>
> The original idea was that because re-tuning cannot be
> done in RPMB, the need to re-rune in RPMB could be avoided
> by always re-tuning before switching to RPMB and then switching
> straight back. IIRC re-tuning should guarantee at least 4MB
> more I/O without issue.
>
> The alternative to dropping re-tuning in this case could
> be to add a retry loop for MMC_DRV_OP_IOCTL_RPMB if the error
> is -EILSEQ

Actually, you could still do this patch as well because
a CRC error would cause re-tuning before the next retry.

>
>
>>
>> Since RPMB reads can not be retried, the clients would be directly
>> affected by the errors.
>>
>> This commit disables the request prior to RPMB switching while allowing
>> the pause interface to still request a retune before the pause for other
>> use cases.
>>
>> This was verified with the sdhci-of-arasan driver (ZynqMP) configured
>> for HS200 using two separate eMMC cards (DG4064 and 064GB2). In both
>> cases, the error was easy to reproduce triggering every few tenths of
>> reads.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
>> ---
>> drivers/mmc/core/block.c | 2 +-
>> drivers/mmc/core/host.c | 7 ++++---
>> drivers/mmc/core/host.h | 2 +-
>> 3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>> index f9a5cffa64b1..1d69078ad9b2 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -859,7 +859,7 @@ static int mmc_blk_part_switch_pre(struct mmc_card *card,
>> if (ret)
>> return ret;
>> }
>> - mmc_retune_pause(card->host);
>> + mmc_retune_pause(card->host, false);
>> }
>>
>> return ret;
>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> index 096093f7be00..a9b95aaa2235 100644
>> --- a/drivers/mmc/core/host.c
>> +++ b/drivers/mmc/core/host.c
>> @@ -119,13 +119,14 @@ void mmc_retune_enable(struct mmc_host *host)
>>
>> /*
>> * Pause re-tuning for a small set of operations. The pause begins after the
>> - * next command and after first doing re-tuning.
>> + * next command and, if retune is set, after first doing re-tuning.
>> */
>> -void mmc_retune_pause(struct mmc_host *host)
>> +void mmc_retune_pause(struct mmc_host *host, bool retune)
>> {
>> if (!host->retune_paused) {
>> host->retune_paused = 1;
>> - mmc_retune_needed(host);
>> + if (retune)
>> + mmc_retune_needed(host);
>
> Better to just drop mmc_retune_needed(host);
>
>> mmc_retune_hold(host);
>
> There is still a small chance that re-tuning
> is needed anyway in which case it will still be done.
>
>> }
>> }
>> diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
>> index 48c4952512a5..321776b52270 100644
>> --- a/drivers/mmc/core/host.h
>> +++ b/drivers/mmc/core/host.h
>> @@ -18,7 +18,7 @@ void mmc_retune_disable(struct mmc_host *host);
>> void mmc_retune_hold(struct mmc_host *host);
>> void mmc_retune_release(struct mmc_host *host);
>> int mmc_retune(struct mmc_host *host);
>> -void mmc_retune_pause(struct mmc_host *host);
>> +void mmc_retune_pause(struct mmc_host *host, bool retune);
>> void mmc_retune_unpause(struct mmc_host *host);
>>
>> static inline void mmc_retune_clear(struct mmc_host *host)
>

2024-01-02 19:03:13

by Adrian Hunter

[permalink] [raw]
Subject: Re: [PATCH] mmc: rpmb: do not force a retune before RPMB switch

On 4/12/23 17:01, Jorge Ramirez-Ortiz wrote:
> Requesting a retune before switching to the RPMB partition has been
> observed to cause CRC errors on the RPMB reads (-EILSEQ).
>
> Since RPMB reads can not be retried, the clients would be directly
> affected by the errors.
>
> This commit disables the request prior to RPMB switching while allowing
> the pause interface to still request a retune before the pause for other
> use cases.
>
> This was verified with the sdhci-of-arasan driver (ZynqMP) configured
> for HS200 using two separate eMMC cards (DG4064 and 064GB2). In both
> cases, the error was easy to reproduce triggering every few tenths of
> reads.
>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>

Acked-by: Adrian Hunter <[email protected]>

> ---
> drivers/mmc/core/block.c | 2 +-
> drivers/mmc/core/host.c | 7 ++++---
> drivers/mmc/core/host.h | 2 +-
> 3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index f9a5cffa64b1..1d69078ad9b2 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -859,7 +859,7 @@ static int mmc_blk_part_switch_pre(struct mmc_card *card,
> if (ret)
> return ret;
> }
> - mmc_retune_pause(card->host);
> + mmc_retune_pause(card->host, false);
> }
>
> return ret;
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 096093f7be00..a9b95aaa2235 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -119,13 +119,14 @@ void mmc_retune_enable(struct mmc_host *host)
>
> /*
> * Pause re-tuning for a small set of operations. The pause begins after the
> - * next command and after first doing re-tuning.
> + * next command and, if retune is set, after first doing re-tuning.
> */
> -void mmc_retune_pause(struct mmc_host *host)
> +void mmc_retune_pause(struct mmc_host *host, bool retune)
> {
> if (!host->retune_paused) {
> host->retune_paused = 1;
> - mmc_retune_needed(host);
> + if (retune)
> + mmc_retune_needed(host);
> mmc_retune_hold(host);
> }
> }
> diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
> index 48c4952512a5..321776b52270 100644
> --- a/drivers/mmc/core/host.h
> +++ b/drivers/mmc/core/host.h
> @@ -18,7 +18,7 @@ void mmc_retune_disable(struct mmc_host *host);
> void mmc_retune_hold(struct mmc_host *host);
> void mmc_retune_release(struct mmc_host *host);
> int mmc_retune(struct mmc_host *host);
> -void mmc_retune_pause(struct mmc_host *host);
> +void mmc_retune_pause(struct mmc_host *host, bool retune);
> void mmc_retune_unpause(struct mmc_host *host);
>
> static inline void mmc_retune_clear(struct mmc_host *host)


2024-01-03 08:08:44

by Adrian Hunter

[permalink] [raw]
Subject: Re: [PATCH] mmc: rpmb: do not force a retune before RPMB switch

On 2/01/24 21:02, Adrian Hunter wrote:
> On 4/12/23 17:01, Jorge Ramirez-Ortiz wrote:
>> Requesting a retune before switching to the RPMB partition has been
>> observed to cause CRC errors on the RPMB reads (-EILSEQ).
>>
>> Since RPMB reads can not be retried, the clients would be directly
>> affected by the errors.
>>
>> This commit disables the request prior to RPMB switching while allowing
>> the pause interface to still request a retune before the pause for other
>> use cases.
>>
>> This was verified with the sdhci-of-arasan driver (ZynqMP) configured
>> for HS200 using two separate eMMC cards (DG4064 and 064GB2). In both
>> cases, the error was easy to reproduce triggering every few tenths of
>> reads.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
>
> Acked-by: Adrian Hunter <[email protected]>

Oops, this is the wrong patch. I meant to ack the later one,
which I have now.

>
>> ---
>> drivers/mmc/core/block.c | 2 +-
>> drivers/mmc/core/host.c | 7 ++++---
>> drivers/mmc/core/host.h | 2 +-
>> 3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>> index f9a5cffa64b1..1d69078ad9b2 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -859,7 +859,7 @@ static int mmc_blk_part_switch_pre(struct mmc_card *card,
>> if (ret)
>> return ret;
>> }
>> - mmc_retune_pause(card->host);
>> + mmc_retune_pause(card->host, false);
>> }
>>
>> return ret;
>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> index 096093f7be00..a9b95aaa2235 100644
>> --- a/drivers/mmc/core/host.c
>> +++ b/drivers/mmc/core/host.c
>> @@ -119,13 +119,14 @@ void mmc_retune_enable(struct mmc_host *host)
>>
>> /*
>> * Pause re-tuning for a small set of operations. The pause begins after the
>> - * next command and after first doing re-tuning.
>> + * next command and, if retune is set, after first doing re-tuning.
>> */
>> -void mmc_retune_pause(struct mmc_host *host)
>> +void mmc_retune_pause(struct mmc_host *host, bool retune)
>> {
>> if (!host->retune_paused) {
>> host->retune_paused = 1;
>> - mmc_retune_needed(host);
>> + if (retune)
>> + mmc_retune_needed(host);
>> mmc_retune_hold(host);
>> }
>> }
>> diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
>> index 48c4952512a5..321776b52270 100644
>> --- a/drivers/mmc/core/host.h
>> +++ b/drivers/mmc/core/host.h
>> @@ -18,7 +18,7 @@ void mmc_retune_disable(struct mmc_host *host);
>> void mmc_retune_hold(struct mmc_host *host);
>> void mmc_retune_release(struct mmc_host *host);
>> int mmc_retune(struct mmc_host *host);
>> -void mmc_retune_pause(struct mmc_host *host);
>> +void mmc_retune_pause(struct mmc_host *host, bool retune);
>> void mmc_retune_unpause(struct mmc_host *host);
>>
>> static inline void mmc_retune_clear(struct mmc_host *host)
>