RE: [PATCH v24 08/15] mmc: renesas_sdhi: Add TMIO_MMC_INTERNAL_DIVIDER flag

[Biju Das]

Hi Wolfram,

Thanks for the feedback.

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Sent: 03 September 2026 10:38
> Subject: Re: [PATCH v24 08/15] mmc: renesas_sdhi: Add TMIO_MMC_INTERNAL_DIVIDER flag
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Medium] Fallback `f_max` and `f_min` initialization fails to account for the internal clock divider.
> > --
> >
> > --- Patch [8]: [PATCH v24 08/15] mmc: renesas_sdhi: Add
> > TMIO_MMC_INTERNAL_DIVIDER flag --- commit
> > c4a63c19303286f5b383bdabaa2a3f3827a8c5c5
> > Author: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > mmc: renesas_sdhi: Add TMIO_MMC_INTERNAL_DIVIDER flag
> >
> > This commit introduces the TMIO_MMC_INTERNAL_DIVIDER flag to support
> > SoCs like RZ/G3L that have a fixed internal /2 clock divider. It adds
> > a helper function to fold this division into the reported clock rate
> > and uses it in the clock update path.
> >
> > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> > > b/drivers/mmc/host/renesas_sdhi_core.c
> > > index ff428b6187bbd..b5f2ff68db758 100644
> > > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > > @@ -125,6 +125,27 @@ static int renesas_sdhi_clk_enable(struct tmio_mmc_host *host)
> > >  	return 0;
> > >  }
> > >
> >
> > [Severity: Medium]
> > Since this patch introduces an internal /2 clock divider, should
> > renesas_sdhi_clk_enable() (which is just above this hunk) also be
> > updated to account for it?

For this patch, we don't have a SoC with TMIO_MMC_INTERNAL_DIVIDER flag set.
So, it retains previous functionality. Please comment if you think otherwise.

> >
> > If the device tree omits the max-frequency property, f_max defaults to
> > the raw parent clock rate:
> >

This behaviour is same as there is no SoC has set with TMIO_MMC_INTERNAL_DIVIDER flag.

> > drivers/mmc/host/renesas_sdhi_core.c:renesas_sdhi_clk_enable() {
> >     ...
> > 	if (!mmc->f_max)
> > 		mmc->f_max = clk_get_rate(priv->clk);

The SoC with TMIO_MMC_INTERNAL_DIVIDER has always frequency defined in SoC dtsi
and this condition is skipped. Please let me know should I add a comment here??

> >
> > 	/*
> > 	 * Minimum frequency is the minimum input clock frequency
> > 	 * divided by our maximum divider.
> > 	 */
> > 	mmc->f_min = max(clk_round_rate(priv->clk, 1) / host->pdata->max_divider, 1L);
> >     ...
> > }
> >
> > For hardware with the internal divider, this advertises double the
> > actual achievable bus frequency. If the clock provider cannot scale up
> > to 4x the requested frequency, the MMC core might negotiate a
> > high-speed mode that the hardware cannot satisfy, resulting in a mode
> > mismatch where the bus silently runs at half the expected speed.

SDx_Clock (HS clk) Figure 4.4-8 Block Diagram of the Deformed Clock System (7)
IMCLK = SDx_Clock/2

SD_Clock  Figure 6.2-2 Functional Diagram of Clock Divider for SD_CLK

SD_Clock is dynamic setting based on the speed mode.

For HS400 mode, SD_CLK = IMCLK = SDx_Clock/2 and SD_CLK_CTRL = 0

For Non-HS400 mode, SD_CLK = IMCLK /2 = SDx_Clock/4 and SD_CLK_CTRL = 0xff for 1/1 option and
other values for non 1/1 option.

We know the maximum frequency supported by the SoC and we must define this in SoC dtsi.

I believe we should support the below check only for non-DT platforms as we don't know
the SoC max sd clock capability.

	if (!mmc->f_max)
		mmc->f_max = clk_get_rate(priv->clk);


> >
> > Could mmc->f_max and mmc->f_min calculation be adjusted when the
> > TMIO_MMC_INTERNAL_DIVIDER flag is set?
> 
> Looks valid from a glimpse. Please comment if you think this report makes sense or not.

Please comment on my response.

Cheers,
Biju