RE: [PATCH v23 05/14] mmc: renesas_sdhi: Make clock divider mask configurable

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 07 August 2026 14:16
> Subject: Re: [PATCH v23 05/14] mmc: renesas_sdhi: Make clock divider mask configurable
> 
> Hi Biju,
> 
> On Thu, 30 Jul 2026 at 13:32, Biju <biju.das.au@xxxxxxxxx> wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > Add a clk_div_mask field to renesas_sdhi_of_data and tmio_mmc_data so
> > each SoC can specify its own divider mask instead of the hardcoded
> > CLK_CTL_DIV_MASK used in renesas_sdhi_set_clock().
> >
> > Default to GENMASK(7, 0) at probe time when unset, and populate all
> > existing internal/sys DMAC of_data tables with this value to keep
> > current SoCs unaffected. This lets RZ/G3L, which has a wider divider
> > field, reuse the same clock-setting code.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> >  * Assigned of_data->clk_div_mask in renesas_sdhi_probe().
> > v21->v22:
> >  * Updated commit description.
> >  * Added clk_div_mask to of_default_cfg and of_rcar_gen2_compatible.
> 
> Thanks for the update!
> 
> > --- a/drivers/mmc/host/renesas_sdhi.h
> > +++ b/drivers/mmc/host/renesas_sdhi.h
> > @@ -41,6 +41,7 @@ struct renesas_sdhi_of_data {
> >         unsigned long sdhi_flags;
> >         u64 clk_mask;
> >         int max_divider;
> > +       u16 clk_div_mask;
> >  };
> >
> >  #define SDHI_CALIB_TABLE_MAX 32
> > diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> > b/drivers/mmc/host/renesas_sdhi_core.c
> > index d893a263e770..5dcbb431e1e1 100644
> > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > @@ -224,7 +224,7 @@ static void renesas_sdhi_set_clock(struct tmio_mmc_host *host,
> >                         clk &= ~0xff;
> >         }
> >
> > -       clock = clk & CLK_CTL_DIV_MASK;
> > +       clock = clk & host->pdata->clk_div_mask;
> >         if (clock != CLK_CTL_DIV_MASK)
> 
> This strikes me as a bit odd; should CLK_CTL_DIV_MASK be changed to
> host->pdata->clk_div_mask in the check, too?

No, this is the value for 1:1 option.

    [    2.648050] ###########clk/clock 4000ff/ff

if we do, (clock != host->pdata->clk_div_mask) then
the check will always true for RZ/G3L(GENMASK(9,0)) and
it never set 1:1 divider value. 

The 1:1 divider values:

Non-RZ/G3L, it is 1111 1111b
For RZ/G3L, it is 00 1111 1111b

> 
> >                 host->mmc->actual_clock /= (1 << (ffs(clock) + 1));
> >
> > @@ -1138,6 +1138,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
> >                 mmc_data->max_segs = of_data->max_segs;
> >                 mmc_data->clk_mask = of_data->clk_mask;
> >                 mmc_data->max_divider = of_data->max_divider;
> > +               mmc_data->clk_div_mask = of_data->clk_div_mask;
> >                 dma_priv->dma_buswidth = of_data->dma_buswidth;
> >                 host->bus_shift = of_data->bus_shift;
> >                 /* Fallback for old DTs */ @@ -1186,6 +1187,9 @@ int
> > renesas_sdhi_probe(struct platform_device *pdev,
> >         if (!mmc_data->max_divider)
> >                 mmc_data->max_divider = SDHI_MAX_DIVIDER_DEFAULT;
> >
> > +       if (!mmc_data->clk_div_mask)
> > +               mmc_data->clk_div_mask = GENMASK(7, 0);
> 
> CLK_CTL_DIV_MASK, perhaps?

Agreed.

Cheers,
Biju