RE: [PATCH v24 11/15] mmc: renesas_sdhi: Add RZ/G3L SDHI support

[Biju Das]

Hi Wolfram,

Thanks for the feedback.

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Sent: 03 September 2026 13:02
> Subject: Re: [PATCH v24 11/15] mmc: renesas_sdhi: Add RZ/G3L SDHI support
> 
> 
> Okay, let's start this one high level...
> 
> On Thu, Aug 20, 2026 at 06:16:41PM +0100, Biju wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > RZ/G3L (r9a08g046) has an SDHI controller that differs from existing
> > platforms in clock divider width, a second HW adjustment register, and
> > an extra tuning register.
> >
> > Add TMIO_MMC_HWADJ to mark controllers with a second HWADJ2 register
> > (offset 0x010), programmed by renesas_sdhi_set_hw_adjustment_delay()
> > alongside TMPPORT2 (0x3FFF at 3.3V, 0xFF at 1.8V). Clear the new
> > HWADJ4 register (0x022) at tuning start when TMIO_MMC_INTERNAL_DIVIDER
> > is set.
> >
> > Update renesas_sdhi_set_clock() to write the clock control register
> > with a 32-bit access when max_divider isn't the default, moving bits
> > [9:8] of the clock value into bits [17:16] of the register — needed to
> > drive RZ/G3L's wider 11-bit divider field via the max_divider/
> > clk_div_mask support added earlier in this series.
> >
> > Add SDHI_VER_RZ_G3L_SDMMC (0xce10) and handle it in
> > renesas_sdhi_sdbuf_width(). Add of_data_rzg3l with RZ/G3L's wider
> > clk_mask, an 11-bit max_divider of 2048, a dedicated tap table, and
> > the relevant flags including TMIO_MMC_INTERNAL_DIVIDER and
> > TMIO_MMC_HWADJ. Wire it up via of_rzg3l_compatible (fixed_addr_mode
> > quirk) and register the "renesas,sdhi-r9a08g046" compatible string.
> 
> After arriving at patch 11, I feel a significant loss in motivation to read this patch description. It is
> very wordy, includes a lot of the WHAT, hiding the WHY. Maybe you can adjust the prompt, but I would
> suggest to write it yourself.

OK, will fix commit message in next version.

> 
> > -	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clock);
> > +	if (host->pdata->max_divider != SDHI_MAX_DIVIDER_DEFAULT) {
> > +		u64 tmp;
> > +
> > +		tmp = FIELD_GET(RZG3L_CLK_CTL_DIV9_DIV8_SRC, clk);
> > +		clock |= FIELD_PREP(RZG3L_CLK_CTL_DIV9_DIV8, tmp);
> > +		clock &= ~RZG3L_CLK_CTL_DIV9_DIV8_SRC;
> 
> I am confused. Bit 8+9 are always zero?

No.

for (clk = host->pdata->clk_mask; new_clock + clk_margin >= (clock << 1); clk >>= 1)
		clock <<= 1;

clk will have a valid value.

clock = clk & host->pdata->clk_div_mask;

So initially bit 8+9 will have a value. This will be moved to bit 16 + 17 first
Then we clear the bits 8 + 9. Bit8(SCLKEN) and Bit9(SDCLKOFFEN)

> 
> > -#define SH_MOBILE_SDHI_SCC_TMPPORT2	0x00E
> > +#define SH_MOBILE_SDHI_SCC_TMPPORT2	0x00E /* G3L: SDm_SCC_HS400MODE1 */
> > +#define RZG3L_SDHI_SCC_HWADJ2		0x010
> 
> Does RZG3L have a dedicated naming? I'd guess *_HWADJ1. I wonder if it makes sense to have a dedicated
> define for it. I tend to think so.

There is no *_HWADJ1 mentioned in HW manual.
Hardware manual mention this register as "SDm_SCC_HWADJ2"

> 
> > +		if (hwadj2)
> > +			sd_scc_write32(host, priv, RZG3L_SDHI_SCC_HWADJ2, 0x3FFF);
> > +	} else {
> >  		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TMPPORT2, 0x1);
> > +		if (hwadj2)
> > +			sd_scc_write32(host, priv, RZG3L_SDHI_SCC_HWADJ2, 0xFF);
> 
> Magic values?

HW manual mentions the following,

HWADJ2[15:0] 1FFh Hardware Adjustment.
For 1.8V operation, specify 00FFh.
For 3.3V operation, specify 3FFFh.

Is the below macros ok for representing magic values??

SD_SCC_HWADJ2_1_8V 0xFF
SD_SCC_HWADJ2_3_3V 0x3FFF



> 
> >  	if (preserve) {
> >  		card_opt = sd_ctrl_read16(host, CTL_SD_MEM_CARD_OPT);
> > -		clk_ctrl = sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL);
> > +		if (host->pdata->flags & TMIO_MMC_INTERNAL_DIVIDER)
> > +			clk_ctrl_32 = sd_ctrl_read32(host, CTL_SD_CARD_CLK_CTL);
> > +		else
> > +			clk_ctrl = sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL);
> > +
> >  		if (host->pdata->flags & TMIO_MMC_MIN_RCAR2)
> >  			sdif_mode = sd_ctrl_read16(host, CTL_SDIF_MODE);
> >  	}
> > @@ -217,7 +222,11 @@ static void tmio_mmc_reset(struct tmio_mmc_host
> > *host, bool preserve)
> >
> >  	if (preserve) {
> >  		sd_ctrl_write16(host, CTL_SD_MEM_CARD_OPT, card_opt);
> > -		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clk_ctrl);
> > +		if (host->pdata->flags & TMIO_MMC_INTERNAL_DIVIDER)
> > +			sd_ctrl_write32(host, CTL_SD_CARD_CLK_CTL, clk_ctrl_32);
> > +		else
> > +			sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, clk_ctrl);
> > +
> 
> We should hide this complexity in a function, I'd say. To make sure future updates modifying
> CTL_SD_CARD_CLK_CTL will have it easy.

OK.

> 
> > +/* Some controllers have hw adjustment delay */
> > +#define TMIO_MMC_HWADJ			BIT(15)
> 
> I wonder how likely it is that a SDHI variant has either only this or only TUNING_DELAY. Do you know of
> one? What would you think about merging the two? You know the RZ SDHI much better...

HWADJ2 present only on RZ/G3L,
whereas TUNING_DELAY present on both RZ/G2L and RZ/G3L.

> 
> I will stop reviewing at this patch. There are enough issues to tackle, I'd think. Once we got this
> right, we can improve HS400 on top of it.
> 
> Makes sense to you?

Yes. I have responded to all your comments. Please provide suggestions.

Looking forward to hearing from you.

Cheers,
Biju