Re: [PATCH v5 3/3] clk: meson: add sub MMC clock controller driver

[jbrunet@baylibre.com]

On Sun, 2018-11-04 at 02:01 +0800, Jianxin Pan wrote:
> Hi Jerome,
> 
> Thanks for the review, we really appreciate your time.
> 
> I'm very sorry maybe I don't catch all your meaning very well. 
> 
> Please see my comments below.
> 
> On 2018/10/29 3:16, Jerome Brunet wrote:
> > On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
> > > Hi Jerome,
> > > 
> > > On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <jbrunet@baylibre.com>
> > > wrote:
> > > [snip]
> > > > > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > > > > +{
> > > > > > > + struct clk_regmap *clk = to_clk_regmap(hw);
> > > > > > > + struct clk_regmap_div_data *div =
> > > > > > > clk_get_regmap_div_data(clk);
> > > > > > > + unsigned int val;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + ret = regmap_read(clk->map, div->offset, &val);
> > > > > > > + if (ret)
> > > > > > > +         return;
> > > > > > > 
> > > > > > > + val &= (clk_div_mask(div->width) << div->shift);
> > > > > > > + if (!val)
> > > > > > > +         regmap_update_bits(clk->map, div->offset,
> > > > > > > +                            clk_div_mask(div->width) << div-
> > > > > > > >shift,
> > > > > > > +                            clk_div_mask(div->width));
> > > > > > 
> > > > > > This is wrong for several reasons:
> > > > > > * You should hard code the initial value in the driver.
> > > > > > * If shift is not 0, I doubt this will give the expected result.
> > > > > 
> > > > > The value 0x00 of divider means nand clock off then read/write nand
> > > > > register is forbidden.
> > > > 
> > > > That is not entirely true, you can access the clock register or you'd
> > > > be in a
> > > > chicken and egg situation.
> > > > 
> > > > > Should we set the initial value in nand driver, or in sub emmc clk
> > > > > driver?
> > > > 
> > > > In the nand driver, which is the consumer of the clock. see my
> > > > previous comments
> > > > about it.
> > > 
> > > an old version of this series had the code still in the NAND driver
> > > (by writing to the registers directly instead of using the clk API).
> > > this looks pretty much like a "sclk-div" to me (as I commented in v3
> > > of this series: [0]):
> > > - value 0 means disabled
> > > - positive divider values
> > > - (probably no duty control, but that's optional as far as I
> > > understand sclk-div)
> > > - uses max divider value when enabling the clock
> > > 
> > > if switching to sclk-div works then we can get rid of some duplicate
> > > code
> > 
> > It is possible:
> > There is a couple of things to note though:
> > 
> > * sclk does not 'uses max divider value when enabling the clock': Since
> > this
> > divider can gate, it needs to save the divider value when disabling, since
> > the
> > divider value is no longer stored in the register,
> > On init, this cached value is  saved as it is. If the divider is initially
> > disabled, we have to set the cached value to something that makes sense,
> > in case
> > the clock is enabled without a prior call to clk_set_rate().
> > > So in sclk, the clock setting is not changed nor hard coded in init, and
> > > this is
> > a very important difference.
> > 
> I think It's ok for the latest sub mmc clock and nand driver now:
> 1. in mmc_clkc_register_clk_with_parent("div", ...) from mmc_clkc_probe():
>    cached_div is set to div_max durning clk register，but is not set to div
> hw register.
> 
> 2. In meson nand driver v6: 	
> https://lore.kernel.org/lkml/1541090542-19618-3-git-send-email-jianxin.pan@amlogic.com
> 1) In meson_nfc_clk_init() from probe:   get clock handle, then
> prepare_enable and set default rate.
>    nfc->device_clk = devm_clk_get(nfc->dev, "device");
>    ret = clk_prepare_enable(nfc->device_clk);			//Here div hw
> register changed from 0 -> cached_div.
>    default_clk_rate = clk_round_rate(nfc->device_clk, 24000000);
>    ret = clk_set_rate(nfc->device_clk, default_clk_rate);	//Then
> register and cached_div are both updated to the default 24M.
> 2) In meson_nfc_select_chip(), set the actual frequency
>   ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate);	//Here
> register and cached_div are changed again.
> 3) if clk_disable() is called, set div hw register to zero, and
> cached_div  keep unchagned.
>    if clk_disable() is called again,  cached_div is restored to div hw
> register.

You don't need to do all this in your NAND driver: enable - round - set_rate -
disable is a waste of time. 

Directly calling set_rate(24000000), with the clock still off, will have the
same result. Then if your HW needs this clock to be ON to access registers
(like you told us) you should probably turn it on.

> 
> When enabling the clock, divider register does not need to be div_max.  
> Any value is OK except ZERO, the cached_div from init or set_rate is ok
> >  
> > * Even if sclk zero value means gated, it is still a zero based divider,
> > while
> > eMMC/Nand divider is one based. It this controller was to sclk, then
> > something
> > needs to be done for this.
> Could I add another patch to this patchset for sclk to support
> CLK_DIVIDER_ONE_BASED ?

Yes, you should otherwise the calculation are just wrong for your clock.

> > * Since sclk caches a value in its data, and there can multiple instance
> > of eMMC
> > /NAND clock controller, some care must be taken when registering the data.
> OK, I will fix it and alloc mmc_clkc_div_data danymicly durning probe.
> Thank you. 
> > Both the generic divider and sclk could work here ... it's up to you
> > Jianxin.
> > 
> == Why use meson_sclk_div_ops instead of clk_regmap_divider_ops?
> The default divider hw register vaule is 0 when system power on.
> Then there is a WARNING in divider_recalc_rate() durning clk_hw_register():
> [    0.918238] ffe05000.clock-controller#div: Zero divisor and
> CLK_DIVIDER_ALLOW_ZERO not set
> [    0.925581] WARNING: CPU: 3 PID: 1 at drivers/clk/clk-divider.c:127
> divider_recalc_rate+0x88/0x90
> Then I still need to hard code the initual value to nand driver without
> CLK_DIVIDER_ALLOW_ZERO flags.
> > > 
> > > Regards
> > > Martin
> > > 
> > > 
> > > [0] https://patchwork.kernel.org/patch/10607157/#22238243
> > 
> > .
> > 
> 
>