RE: [PATCH v17 06/17] mmc: renesas_sdhi: Introduce renesas_sdhi_hw_info to abstract clock mask

[Biju Das]

Hi Wolfram,

Thanks for the feedback.

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Sent: 17 June 2026 13:36
> Subject: Re: [PATCH v17 06/17] mmc: renesas_sdhi: Introduce renesas_sdhi_hw_info to abstract clock mask
> 
> On Wed, Jun 03, 2026 at 07:57:06AM +0100, Biju wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > The RZ/G3L SoC has 11 divider bits and requires a different clock mask
> > in renesas_sdhi_set_clock().
> >
> > Add a new renesas_sdhi_hw_info struct to hold hardware-specific
> > parameters, starting with clk_mask. This replaces the hardcoded
> > constant in renesas_sdhi_set_clock() with a value sourced from the
> > per-device hw_info, and widens the clk variable from u32 to u64
> > accordingly, as clk_mask for RZ/G3L exceeds 32 bits.
> >
> > Wire hw_info through renesas_sdhi_of_data_with_quirks (internalDMAC
> > path) and a new renesas_sdhi_of_data_with_info wrapper (sysDMAC path),
> > and plumb it into renesas_sdhi_probe() so it is stored in the
> > per-instance renesas_sdhi struct.
> >
> > All existing users are assigned sdhi_hw_info_generic, preserving
> > current behaviour. No functional change.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v1->v2:
> >  * No change.
> > ---
> >  drivers/mmc/host/renesas_sdhi.h               | 12 ++++
> >  drivers/mmc/host/renesas_sdhi_core.c          |  7 +-
> >  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 16 ++++-
> >  drivers/mmc/host/renesas_sdhi_sys_dmac.c      | 66 ++++++++++++++-----
> >  4 files changed, 81 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/mmc/host/renesas_sdhi.h
> > b/drivers/mmc/host/renesas_sdhi.h index 09bf9b24a8c3..a7fc525b7218
> > 100644
> > --- a/drivers/mmc/host/renesas_sdhi.h
> > +++ b/drivers/mmc/host/renesas_sdhi.h
> > @@ -41,6 +41,15 @@ struct renesas_sdhi_of_data {
> >
> >  #define SDHI_CALIB_TABLE_MAX 32
> >
> > +struct renesas_sdhi_hw_info {
> > +	u64 clk_mask;
> > +};
> > +
> > +struct renesas_sdhi_of_data_with_info {
> > +	const struct renesas_sdhi_of_data *of_data;
> > +	const struct renesas_sdhi_hw_info *info; };
> 
> ? Why don't you put it in renesas_sdhi_of_data and tmio_mmc_data instead?

Ok, I have prototyped with renesas_sdhi_of_data and tmio_mmc_data as per your 
suggestion

The below variables[1] added to struct tmio_mmc_data and
the feature flags[2]. Please let me know is it ok or not?

[1]
	u64				clk_mask;
	unsigned int 		max_divider;
	u32				osel_tmpout;

[2]
 /* Some controllers have tuning delay */
#define TMIO_MMC_TUNING_DELAY		BIT(13)

/* Some controllers have internal divider */
#define TMIO_MMC_INTERNAL_DIVIDER	BIT(14)

/* Some controllers have hw adjustment delay */
#define TMIO_MMC_HWADJ2			BIT(15)

/* Some controllers have HS400mode2  */
#define TMIO_MMC_HS400MODE2		BIT(16)

/* Some controllers have HS400ES  */
#define TMIO_MMC_HS400ES		BIT(17)

> 
> You wouldn't even need to put the default value in all other of_data by
> using:
> 
> In the 'if (of_data)' block of probe():
> 
> 	mmc_data->clk_mask = of_data->clk_mask
> 
> and outside of this block:
> 
> 	if (!mmc_data->clk_mask) mmc_data->clk_mask = <default>;
> 
> or something similar.

OK.

> 
> The main thing is that we don't need a hw_info struct IMO. It should be already all there...
> 
> Same for everything which gets added later to hw_info.

Agreed, it now works without hw_info struct.

Cheers,
Biju