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

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

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

Hi Biju,

> > static unsigned int renesas_sdhi_clk_get_rate(struct tmio_mmc_host *host,
> > 					      struct clk *clk)
> > {
> > 	unsigned int freq = clk_get_rate(clk);
> > 
> > 	return host->pdata->flags & TMIO_MMC_INTERNAL_DIVIDER ? freq / 2 : freq; }
> > 
> > Was this AI generated?
> 
> I kept 2 function as renesas_sdhi_is_internal_divider_enabled() is reused in
> renesas_sdhi_set_clock()while adding support for HS400 mode.
> 
> Currently, we decided to focus only for basic RZ/G3L support. So I can merge it into
> Single function as you suggested.

Please do. And if the flag is checked later again, I think we can just
check the flag. We don't need a helper for checking each flag, or?

> 
> > 
> > > @@ -184,10 +205,14 @@ static unsigned int
> > > renesas_sdhi_clk_update(struct tmio_mmc_host *host,
> > >
> > >  	clk_set_rate(ref_clk, best_freq);
> > >
> > > -	if (priv->clkh)
> > > +	if (priv->clkh) {
> > > +		if (host->pdata->flags & TMIO_MMC_INTERNAL_DIVIDER)
> > > +			clkh_shift = 1;
> > 
> > No, this needs to be handled a bit above where clkh_shift is initially set up, so we have it all in one
> > block.
> 
> There is a difference here.
> 
> 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
> 
> Currently framework request a clk to set, we set HS clk first, followed by
> SD_CLK.
> 
> On RZ/G3L, SD_CLK value is based on internal divider.
> 
> For HS400 mode, SD_CLK = SDx_Clock(HS clk)/2
> 
> For Non-HS400 mode, SD_CLK = SDx_Clock(HS clk)/4
> 
> I don't find an easy way to move this logic to the above.

I get an idea what is the problem here, but I am still confused about
one thing. I'll summarize what I understood:

* as of this patch, HS400 is not supported on G3L
* so, in renesas_sdhi_clk_update(), 'clk_shift' value is 2 before clock
  calculation
* as HS400 is not supported yet, the internal divider is always active
* so, you need to decrease the 'clk_shift' value from 2 to 1 after clock
  calculation

So far, so good. My expectation is now that when you add HS400 mode, you
would need some extra code keeping 'clk_shift' at 2 because the internal
divider is disabled then. But I don't find such code in later patches.

What am I missing?

Thanks and happy hacking,

   Wolfram


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