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

[Biju Das]

Hi All,

> -----Original Message-----
> From: Biju Das
> Sent: 07 September 2026 13:53

> Subject: RE: [PATCH v24 11/15] mmc: renesas_sdhi: Add RZ/G3L SDHI support
> 
> 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.
> 

Gentle ping.

Cheers,
Biju