From: Claudiu Beznea <[email protected]>
On latest kernel revisions it has been noticed (on a RZ/G3S system) that
when booting Linux and root file system is on eMMC, at some point in
the booting process, when the systemd applications are started, the
"mmc0: tuning execution failed: -5" message is displayed on console.
On kernel v6.7-rc5 this is reproducible in 90% of the boots. This was
missing on the same system with kernel v6.5.0-rc1. It was also noticed on
kernel revisions v6.6-rcX on a RZ/G2UL based system but not on the kernel
this fix is based on (v6.7-rc5).
Investigating it on RZ/G3S lead to the conclusion that every time the issue
is reproduced all the probed TAPs are OK. According to datasheet, when this
happens the change point of data need to be considered for tuning.
Previous code considered the change point of data happens when the content
of the SMPCMP register is zero. According to RZ/V2M hardware manual,
chapter "Change Point of the Input Data" (as this is the most clear
description that I've found about change point of the input data and all
RZ hardware manual are similar on this chapter), at the time of tuning,
data is captured by the previous and next TAPs and the result is stored in
the SMPCMP register (previous TAP in bits 22..16, next TAP in bits 7..0).
If there is a mismatch b/w the previous and the next TAPs, it indicates
that there is a change point of the input data.
To comply with this, the code checks if this mismatch is present and
updates the priv->smpcmp mask.
This change has been checked on the devices with the following DTSes by
doing 50 consecutive reboots and checking for the tuning failure message:
- r9a08g045s33-smarc.dts
- r8a7742-iwg21d-q7.dts
- r8a7743-iwg20d-q7.dts
- r8a7744-iwg20d-q7.dts
- r8a7745-iwg22d-sodimm.dts
- r8a77470-iwg23s-sbc.dts
- r8a774a1-hihope-rzg2m-ex.dts
- r8a774b1-hihope-rzg2n-ex.dts
- r8a774c0-ek874.dts
- r8a774e1-hihope-rzg2h-ex.dts
- r9a07g043u11-smarc-rzg2ul.dts
- r9a07g044c2-smarc-rzg2lc.dts
- r9a07g044l2-smarc-rzg2l.dts
- r9a07g054l2-smarc-rzv2l.dts
On r8a774a1-hihope-rzg2m-ex, even though the hardware manual doesn't say
anything special about it in the "Change Point of the Input Data" chapter
or SMPCMP register description, it has been noticed that although all TAPs
probed in the tuning process are OK the SMPCMP is zero. For this updated
the renesas_sdhi_select_tuning() function to use priv->taps in case all
TAPs are OK.
Fixes: 5fb6bf51f6d1 ("mmc: renesas_sdhi: improve TAP selection if all TAPs are good")
Signed-off-by: Claudiu Beznea <[email protected]>
---
drivers/mmc/host/renesas_sdhi_core.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index c675dec587ef..f86260800076 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -18,6 +18,7 @@
*
*/
+#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/iopoll.h>
@@ -312,6 +313,9 @@ static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
#define SH_MOBILE_SDHI_SCC_SMPCMP_CMD_REQDOWN BIT(8)
#define SH_MOBILE_SDHI_SCC_SMPCMP_CMD_REQUP BIT(24)
#define SH_MOBILE_SDHI_SCC_SMPCMP_CMD_ERR (BIT(8) | BIT(24))
+#define SH_MOBILE_SDHI_SCC_SMPCMP_CMPNGU_DATA GENMASK(23, 16)
+#define SH_MOBILE_SDHI_SCC_SMPCMP_CMPNGD_DATA GENMASK(7, 0)
+
#define SH_MOBILE_SDHI_SCC_TMPPORT2_HS400OSEL BIT(4)
#define SH_MOBILE_SDHI_SCC_TMPPORT2_HS400EN BIT(31)
@@ -641,7 +645,14 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
* identifying the change point of data.
*/
if (bitmap_full(priv->taps, taps_size)) {
- bitmap = priv->smpcmp;
+ /*
+ * On some setups it happens that all TAPS are OK but
+ * no change point of data. Any tap should be OK for this.
+ */
+ if (bitmap_empty(priv->smpcmp, taps_size))
+ bitmap = priv->taps;
+ else
+ bitmap = priv->smpcmp;
min_tap_row = 1;
} else {
bitmap = priv->taps;
@@ -698,6 +709,7 @@ static int renesas_sdhi_execute_tuning(struct mmc_host *mmc, u32 opcode)
/* Issue CMD19 twice for each tap */
for (i = 0; i < 2 * priv->tap_num; i++) {
+ u32 val, cmpngu_data, cmpngd_data;
int cmd_error = 0;
/* Set sampling clock position */
@@ -706,7 +718,10 @@ static int renesas_sdhi_execute_tuning(struct mmc_host *mmc, u32 opcode)
if (mmc_send_tuning(mmc, opcode, &cmd_error) == 0)
set_bit(i, priv->taps);
- if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP) == 0)
+ val = sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP);
+ cmpngu_data = FIELD_GET(SH_MOBILE_SDHI_SCC_SMPCMP_CMPNGU_DATA, val);
+ cmpngd_data = FIELD_GET(SH_MOBILE_SDHI_SCC_SMPCMP_CMPNGD_DATA, val);
+ if (!cmd_error && cmpngu_data != cmpngd_data)
set_bit(i, priv->smpcmp);
if (cmd_error)
--
2.39.2
Hi Claudiu,
On Fri, Jan 12, 2024 at 12:42 PM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> On latest kernel revisions it has been noticed (on a RZ/G3S system) that
> when booting Linux and root file system is on eMMC, at some point in
> the booting process, when the systemd applications are started, the
> "mmc0: tuning execution failed: -5" message is displayed on console.
> On kernel v6.7-rc5 this is reproducible in 90% of the boots. This was
> missing on the same system with kernel v6.5.0-rc1. It was also noticed on
> kernel revisions v6.6-rcX on a RZ/G2UL based system but not on the kernel
> this fix is based on (v6.7-rc5).
>
> Investigating it on RZ/G3S lead to the conclusion that every time the issue
> is reproduced all the probed TAPs are OK. According to datasheet, when this
> happens the change point of data need to be considered for tuning.
>
> Previous code considered the change point of data happens when the content
> of the SMPCMP register is zero. According to RZ/V2M hardware manual,
> chapter "Change Point of the Input Data" (as this is the most clear
> description that I've found about change point of the input data and all
> RZ hardware manual are similar on this chapter), at the time of tuning,
> data is captured by the previous and next TAPs and the result is stored in
> the SMPCMP register (previous TAP in bits 22..16, next TAP in bits 7..0).
> If there is a mismatch b/w the previous and the next TAPs, it indicates
> that there is a change point of the input data.
>
> To comply with this, the code checks if this mismatch is present and
> updates the priv->smpcmp mask.
>
> This change has been checked on the devices with the following DTSes by
> doing 50 consecutive reboots and checking for the tuning failure message:
> - r9a08g045s33-smarc.dts
> - r8a7742-iwg21d-q7.dts
> - r8a7743-iwg20d-q7.dts
> - r8a7744-iwg20d-q7.dts
> - r8a7745-iwg22d-sodimm.dts
> - r8a77470-iwg23s-sbc.dts
> - r8a774a1-hihope-rzg2m-ex.dts
> - r8a774b1-hihope-rzg2n-ex.dts
> - r8a774c0-ek874.dts
> - r8a774e1-hihope-rzg2h-ex.dts
> - r9a07g043u11-smarc-rzg2ul.dts
> - r9a07g044c2-smarc-rzg2lc.dts
> - r9a07g044l2-smarc-rzg2l.dts
> - r9a07g054l2-smarc-rzv2l.dts
>
> On r8a774a1-hihope-rzg2m-ex, even though the hardware manual doesn't say
> anything special about it in the "Change Point of the Input Data" chapter
> or SMPCMP register description, it has been noticed that although all TAPs
> probed in the tuning process are OK the SMPCMP is zero. For this updated
> the renesas_sdhi_select_tuning() function to use priv->taps in case all
> TAPs are OK.
>
> Fixes: 5fb6bf51f6d1 ("mmc: renesas_sdhi: improve TAP selection if all TAPs are good")
> Signed-off-by: Claudiu Beznea <[email protected]>
Thanks for your patch!
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -641,7 +645,14 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
> * identifying the change point of data.
> */
> if (bitmap_full(priv->taps, taps_size)) {
> - bitmap = priv->smpcmp;
> + /*
> + * On some setups it happens that all TAPS are OK but
> + * no change point of data. Any tap should be OK for this.
> + */
> + if (bitmap_empty(priv->smpcmp, taps_size))
> + bitmap = priv->taps;
> + else
> + bitmap = priv->smpcmp;
> min_tap_row = 1;
I know nothing about tuning, but should min_tap_row still be 1?
Or can you fall back to the else case if priv->smpcmp is empty?
I.e. can this be simplified to:
if (!bitmap_empty(priv->smpcmp, taps_size) &&
bitmap_full(priv->taps, taps_size)) {
...
} else {
...
}
> } else {
> bitmap = priv->taps;
> @@ -706,7 +718,10 @@ static int renesas_sdhi_execute_tuning(struct mmc_host *mmc, u32 opcode)
> if (mmc_send_tuning(mmc, opcode, &cmd_error) == 0)
> set_bit(i, priv->taps);
>
> - if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP) == 0)
> + val = sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP);
The SH_MOBILE_SDHI_SCC_SMPCMP register is read even if its value is
not used below.
> + cmpngu_data = FIELD_GET(SH_MOBILE_SDHI_SCC_SMPCMP_CMPNGU_DATA, val);
> + cmpngd_data = FIELD_GET(SH_MOBILE_SDHI_SCC_SMPCMP_CMPNGD_DATA, val);
> + if (!cmd_error && cmpngu_data != cmpngd_data)
> set_bit(i, priv->smpcmp);
So better move the SH_MOBILE_SDHI_SCC_SMPCMP register access
inside the if (), and change the below to else.
>
> if (cmd_error)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi, Geert,
On 12.01.2024 14:29, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Fri, Jan 12, 2024 at 12:42 PM Claudiu <[email protected]> wrote:
>> From: Claudiu Beznea <[email protected]>
>>
>> On latest kernel revisions it has been noticed (on a RZ/G3S system) that
>> when booting Linux and root file system is on eMMC, at some point in
>> the booting process, when the systemd applications are started, the
>> "mmc0: tuning execution failed: -5" message is displayed on console.
>> On kernel v6.7-rc5 this is reproducible in 90% of the boots. This was
>> missing on the same system with kernel v6.5.0-rc1. It was also noticed on
>> kernel revisions v6.6-rcX on a RZ/G2UL based system but not on the kernel
>> this fix is based on (v6.7-rc5).
>>
>> Investigating it on RZ/G3S lead to the conclusion that every time the issue
>> is reproduced all the probed TAPs are OK. According to datasheet, when this
>> happens the change point of data need to be considered for tuning.
>>
>> Previous code considered the change point of data happens when the content
>> of the SMPCMP register is zero. According to RZ/V2M hardware manual,
>> chapter "Change Point of the Input Data" (as this is the most clear
>> description that I've found about change point of the input data and all
>> RZ hardware manual are similar on this chapter), at the time of tuning,
>> data is captured by the previous and next TAPs and the result is stored in
>> the SMPCMP register (previous TAP in bits 22..16, next TAP in bits 7..0).
>> If there is a mismatch b/w the previous and the next TAPs, it indicates
>> that there is a change point of the input data.
>>
>> To comply with this, the code checks if this mismatch is present and
>> updates the priv->smpcmp mask.
>>
>> This change has been checked on the devices with the following DTSes by
>> doing 50 consecutive reboots and checking for the tuning failure message:
>> - r9a08g045s33-smarc.dts
>> - r8a7742-iwg21d-q7.dts
>> - r8a7743-iwg20d-q7.dts
>> - r8a7744-iwg20d-q7.dts
>> - r8a7745-iwg22d-sodimm.dts
>> - r8a77470-iwg23s-sbc.dts
>> - r8a774a1-hihope-rzg2m-ex.dts
>> - r8a774b1-hihope-rzg2n-ex.dts
>> - r8a774c0-ek874.dts
>> - r8a774e1-hihope-rzg2h-ex.dts
>> - r9a07g043u11-smarc-rzg2ul.dts
>> - r9a07g044c2-smarc-rzg2lc.dts
>> - r9a07g044l2-smarc-rzg2l.dts
>> - r9a07g054l2-smarc-rzv2l.dts
>>
>> On r8a774a1-hihope-rzg2m-ex, even though the hardware manual doesn't say
>> anything special about it in the "Change Point of the Input Data" chapter
>> or SMPCMP register description, it has been noticed that although all TAPs
>> probed in the tuning process are OK the SMPCMP is zero. For this updated
>> the renesas_sdhi_select_tuning() function to use priv->taps in case all
>> TAPs are OK.
>>
>> Fixes: 5fb6bf51f6d1 ("mmc: renesas_sdhi: improve TAP selection if all TAPs are good")
>> Signed-off-by: Claudiu Beznea <[email protected]>
>
> Thanks for your patch!
>
>> --- a/drivers/mmc/host/renesas_sdhi_core.c
>> +++ b/drivers/mmc/host/renesas_sdhi_core.c
>> @@ -641,7 +645,14 @@ static int renesas_sdhi_select_tuning(struct tmio_mmc_host *host)
>> * identifying the change point of data.
>> */
>> if (bitmap_full(priv->taps, taps_size)) {
>> - bitmap = priv->smpcmp;
>> + /*
>> + * On some setups it happens that all TAPS are OK but
>> + * no change point of data. Any tap should be OK for this.
>> + */
>> + if (bitmap_empty(priv->smpcmp, taps_size))
>> + bitmap = priv->taps;
>> + else
>> + bitmap = priv->smpcmp;
>> min_tap_row = 1;
>
> I know nothing about tuning, but should min_tap_row still be 1?
As of my understanding of this code, yes, it should be harmless in keeping
it 1 as the above:
if (tap_cnt >= min_tap_row)
will be true due to the fact that priv->taps is full.
> Or can you fall back to the else case if priv->smpcmp is empty?
> I.e. can this be simplified to:
>
> if (!bitmap_empty(priv->smpcmp, taps_size) &&
> bitmap_full(priv->taps, taps_size)) {
This will not cover all the cases, if I understand your request.
The idea was to keep the code as it previously was and, as I mentioned in
the comment, it happens that priv->taps to be full but smpcmp to be empty
(and code tries to address this scenario, too).
As of my understanding of the tuning, if all the taps are OK ( ==
priv->taps is full) then a change point of the input data should be
reported though priv->smpcmp but that doesn't happens on
r8a774a1-hihope-rzg2m-ex as of my experiments, thus I tried to address this
case, too.
> ...
> } else {
> ...
> }
>
>> } else {
>> bitmap = priv->taps;
>> @@ -706,7 +718,10 @@ static int renesas_sdhi_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> if (mmc_send_tuning(mmc, opcode, &cmd_error) == 0)
>> set_bit(i, priv->taps);
>>
>> - if (sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP) == 0)
>> + val = sd_scc_read32(host, priv, SH_MOBILE_SDHI_SCC_SMPCMP);
>
> The SH_MOBILE_SDHI_SCC_SMPCMP register is read even if its value is
> not used below.
>
>> + cmpngu_data = FIELD_GET(SH_MOBILE_SDHI_SCC_SMPCMP_CMPNGU_DATA, val);
>> + cmpngd_data = FIELD_GET(SH_MOBILE_SDHI_SCC_SMPCMP_CMPNGD_DATA, val);
>> + if (!cmd_error && cmpngu_data != cmpngd_data)
>> set_bit(i, priv->smpcmp);
>
> So better move the SH_MOBILE_SDHI_SCC_SMPCMP register access
> inside the if (), and change the below to else.
Ok, agree.
>
>>
>> if (cmd_error)
>
> Gr{oetje,eeting}s,
>
> Geert
>