RE: [PATCH v24 03/15] mmc: renesas_sdhi: Add clk_mask field to support flexible clock divider widths

[Biju Das]

Hi Wolfram,


> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Sent: 02 September 2026 11:54
> Subject: Re: [PATCH v24 03/15] mmc: renesas_sdhi: Add clk_mask field to support flexible clock divider
> widths
> 
> Hi,
> 
> Sashiko found issues with your patch. I curated the list and left only those which I see as valid. Please
> have a look at these issues.

We need to discuss potential solution for this issue.

> 
> Thank you and happy hacking,
> 
>    Wolfram
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [High] A very low or zero clock rate from the clock provider causes an infinite loop in
> `renesas_sdhi_set_clock`.
> > --
> >
> > --- Patch [3]: [PATCH v24 03/15] mmc: renesas_sdhi: Add clk_mask field
> > to support flexible clock divider widths --- commit
> > 1c7318b14c028686fc8ca703eb8ac2be269937b6
> > Author: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > mmc: renesas_sdhi: Add clk_mask field to support flexible clock
> > divider widths
> >
> > This patch introduces a clk_mask field in platform data to replace a
> > hardcoded 32-bit clock mask, supporting 11-bit clock dividers on newer
> > SoCs like the RZ/G3L. It updates renesas_sdhi_set_clock() to use the
> > dynamic mask and sets up backward-compatible default mask values.
> >
> > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> > > b/drivers/mmc/host/renesas_sdhi_core.c
> > > index f9ec78d699f45..604d886e483c7 100644
> > > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > [ ... ]
> > > @@ -213,7 +213,7 @@ static void renesas_sdhi_set_clock(struct tmio_mmc_host *host,
> > >  	 * provided for actual_clock in renesas_sdhi_clk_update().
> > >  	 */
> > >  	clk_margin = new_clock >> 10;
> > > -	for (clk = 0x80000080; new_clock + clk_margin >= (clock << 1); clk >>= 1)
> > > +	for (clk = host->pdata->clk_mask; new_clock + clk_margin >= (clock
> > > +<< 1); clk >>= 1)
> >
> > [Severity: High]
> > This isn't a bug introduced by this patch, but does this loop risk an
> > infinite hang if the clock provider returns a very low rate?
> >
> > In renesas_sdhi_set_clock(), if the clock provider supplies a rate
> > smaller than the maximum divider, the division to calculate the
> > initial clock value can evaluate to 0. Since 0 << 1 is still 0, the
> > loop condition new_clock + clk_margin >= (clock << 1) will remain
> > permanently true for any strictly positive new_clock value.


If (host->mmc->actual_clock < 512), we get clock=0.


In this case, shall we set divider to maximum and skip the loop?
So that clk runs with maximum allowable divider value.

Please share your thoughts on this.

Or

Do you have different solution, please let me know?

Cheers,
Biju


> >
> > Could this prevent the loop from ever terminating, causing an
> > unrecoverable CPU hang? Should there be an additional safety check
> > (such as clk != 0 or clock > 0) added to the loop condition to
> > gracefully handle misbehaving clock providers?
> >
> > >  		clock <<= 1;
> > >
> > >  	/* 1/1 clock is option */
> >
> > --
> > Sashiko AI review ·
> > https://sashiko.dev/#/patchset/20260820171657.62815-1-biju.das.jz@bp.r
> > enesas.com?part=3