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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

> 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@bp.renesas.com>
> 
> 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?
> 
> If the device tree omits the max-frequency property, f_max defaults to
> the raw parent clock rate:
> 
> drivers/mmc/host/renesas_sdhi_core.c:renesas_sdhi_clk_enable() {
>     ...
> 	if (!mmc->f_max)
> 		mmc->f_max = clk_get_rate(priv->clk);
> 
> 	/*
> 	 * 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.
> 
> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]