Re: [PATCH] mmc: renesas_sdhi: Fix change point of data handling

[claudiu beznea]

Hi, Geert,

On 12.01.2024 14:29, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, Jan 12, 2024 at 12:42 PM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>
>> 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 <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> 
> 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
>