Re: [PATCH v4 11/21] riscv: Add Canaan Kendryte K210 clock driver

[Damien Le Moal <Damien.LeMoal@xxxxxxx>]

On 2020/12/07 17:44, Geert Uytterhoeven wrote:
> Hi Damien,
> 
> On Mon, Dec 7, 2020 at 4:52 AM Damien Le Moal <Damien.LeMoal@xxxxxxx> wrote:
>> I prepared a v5 series addressing your comments (and other comments).
>> I will post that later today after some more tests.
> 
> Thanks, already looking at k210-sysctl-v18...
> 
>> On Fri, 2020-12-04 at 22:19 -0800, Stephen Boyd wrote:
>>>> --- /dev/null
>>>> +++ b/drivers/clk/clk-k210.c
> 
>>>> +       in0_clk = of_clk_get(np, 0);
>>>> +       if (IS_ERR(in0_clk)) {
>>>> +               pr_warn("%pOFP: in0 oscillator not found\n", np);
>>>> +               hws[K210_CLK_IN0] =
>>>> +                       clk_hw_register_fixed_rate(NULL, "in0", NULL,
>>>> +                                                  0, K210_IN0_RATE);
>>>> +       } else {
>>>> +               hws[K210_CLK_IN0] = __clk_get_hw(in0_clk);
>>>> +       }
>>>> +       if (IS_ERR(hws[K210_CLK_IN0])) {
>>>> +               pr_err("%pOFP: failed to get base oscillator\n", np);
>>>> +               goto err;
>>>> +       }
>>>> +
>>>> +       in0 = clk_hw_get_name(hws[K210_CLK_IN0]);
>>>> +       aclk_parents[0] = in0;
>>>> +       pll_parents[0] = in0;
>>>> +       mux_parents[0] = in0;
>>>
>>> Can we use the new way of specifying clk parents so that we don't have
>>> to use __clk_get_hw(), of_clk_get(), and clk_hw_get_name()? Hopefully
>>> the core can handl that all instead of this driver.
>>
>> I removed all this by adding:
>>
>> clock-output-names = "in0";
>>
>> to the DT fixed-rate oscillator clock node (and documented that too). Doing so,
>> clk_hw_get_name(), __clk_get_hw() and of_clk_get() are not needed anymore and
>> the parents clock names arrays do not need run-time update.
> 
> "clock-output-names" is deprecated for clocks with a single output:
> the clock name will be taken from the node name.

Arg. I missed that.

> However, relying on a clock name like this is fragile.
> Instead, your driver should use the phandle from the clocks property,
> using of_clk_get_by_name() or of_clk_get().

That is what all versions before V5 used. But Stephen mentioned that the driver
should not, if possible, use of_clk_get()/__clk_get_name(). Hence the change.
Easy to revert back.

> 
> Stephen: I'm a bit puzzled, as you suggest _not_ using of_clk_get()?

Another solution to this would be to simply remove the fixed-rate clock node
from the DT and have the k210 clock driver unconditionally create that clock
(that is one line !). That actually may be even more simple than the previous
version, albeit at the cost of having the DT not being a perfect description of
the hardware. I am fine with that though.

Thoughts ?

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 


-- 
Damien Le Moal
Western Digital Research