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:

quoted

On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:

quoted

Hi Jerome,

On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet [off-list ref]
wrote:
[snip]

quoted

quoted

quoted

quoted

+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-

quoted

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.

quoted

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().

quoted

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 at 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

quoted

 
* 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.

quoted

* 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. 

quoted

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.

quoted

quoted

Regards
Martin


[0] https://patchwork.kernel.org/patch/10607157/#22238243

.