| Message ID | 20240112114147.1977955-1-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: renesas_sdhi: Fix change point of data handling | expand |
Hi Claudiu, On Fri, Jan 12, 2024 at 12:42 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > 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@bp.renesas.com> 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
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@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> 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@bp.renesas.com> > > 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 >
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)