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

[Wolfram Sang]

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.

> -	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?

> -#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.

> +		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?

>  	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.

> +/* 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...

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?

Attachment: signature.asc
Description: PGP signature