On Wed, 2018-10-17 at 07:15 -0700, Stephen Boyd wrote:

Quoting Weiyi Lu (2018-10-17 06:53:12)

quoted

On Fri, 2018-10-12 at 10:53 -0700, Stephen Boyd wrote:

quoted

Quoting Weiyi Lu (2018-09-20 02:57:27)

quoted

+
+       rc = of_parse_phandle_with_args(node, "mediatek,refclk",
+                       "#clock-cells", 0, &refclk);
+       if (!rc) {
+               of_property_read_string(refclk.np, "clock-output-names",
+                               &refclk_name);
+               for (i = 0; i < num_plls; i++)
+                       plls[i].parent_name = refclk_name;

Use of_clk_parent_fill()?

Thanks for the hint, i might use of_clk_get_parent_name() instead like
below to get each parent clock name.
refclk_name = of_clk_get_parent_name(node, 0);
refclk_aud_name = of_clk_get_parent_name(node, 1);

Alright.

quoted

quoted

quoted

+       }
+
+       rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
+                       "#clock-cells", 0, &refclk_aud);

This is odd. Is this a custom 'clocks' property? What's going on here?
Why can't we use assigned clock parents for this?

Yes. both the reference clock of all PLL and the reference clock of
audio PLL could be customized.These two reference clocks shall be
provided by some component like crystal oscillators on the board. So we
might unable to switch the PLL reference source at runtime, in other
words, it should be set statically. That's why we didn't provide the
set_parent ops for pll clock type. It's also the main reason we choose
not to use assigned-clock-parents for this requirement.

Ok. Do you need some new software clk flag that indicates we should
"lock" the parent or rate configured at boot time? I would like to see
this driver implement the clk_ops for the hardware and have the
framework be the one that configures the tree, instead of doing it some
custom way for this single driver.

Hi Stephen, do you mean we might be able to add a flag just like
CLK_IS_CRITICAL and how it works on clk_enable & clk_disable ops?
And should we make this new flag to allow to set parent before clock is
registered and prohibit from changing the rate and parent after clock
registration. If my understanding is correct, I'll give a try.
BTW, how about the two patch for the ECO change before this patch. Is
there any problem on those two patches?

quoted

quoted

quoted

+       if (!rc) {
+               of_property_read_string(refclk_aud.np, "clock-output-names",
+                               &refclk_aud_name);
+               if (strcmp(refclk_name, refclk_aud_name)) {
+                       plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;