Re: [PATCH v10 09/23] dt-binding: clock: Document canaan,k210-clk bindings

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

On 2020/12/17 19:17, Stephen Boyd wrote:
> Quoting Damien Le Moal (2020-12-17 00:13:57)
>> On 2020/12/17 17:10, Stephen Boyd wrote:
>>> Quoting Damien Le Moal (2020-12-13 05:50:42)
>>>> diff --git a/include/dt-bindings/clock/k210-clk.h b/include/dt-bindings/clock/k210-clk.h
>>>> index 5a2fd64d1a49..b2de702cbf75 100644
>>>> --- a/include/dt-bindings/clock/k210-clk.h
>>>> +++ b/include/dt-bindings/clock/k210-clk.h
>>>> @@ -3,18 +3,51 @@
>>>>   * Copyright (C) 2019-20 Sean Anderson <seanga2@xxxxxxxxx>
>>>>   * Copyright (c) 2020 Western Digital Corporation or its affiliates.
>>>>   */
>>>> -#ifndef K210_CLK_H
>>>> -#define K210_CLK_H
>>>> +#ifndef CLOCK_K210_CLK_H
>>>> +#define CLOCK_K210_CLK_H
>>>>  
>>>>  /*
>>>> - * Arbitrary identifiers for clocks.
>>>> - * The structure is: in0 -> pll0 -> aclk -> cpu
>>>> - *
>>>> - * Since we use the hardware defaults for now, set all these to the same clock.
>>>> + * Kendryte K210 SoC clock identifiers (arbitrary values).
>>>>   */
>>>> -#define K210_CLK_PLL0   0
>>>> -#define K210_CLK_PLL1   0
>>>> -#define K210_CLK_ACLK   0
>>>> -#define K210_CLK_CPU    0
>>>
>>> This seems to open a bisection hole. I see that ACLK is used in the
>>> existing dtsi file, and that is the same as CLK_CPU, but after this
>>> patch it will change to not exist anymore. Can we leave ACLK around
>>> defined to be 0? I imagine it won't be used in the future so we can
>>> remove it later. I can then apply this for v5.11-rc1 and then merge the
>>> clk driver patch in clk tree.
>>>
>>>> +#define K210_CLK_CPU   0
>>>> +#define K210_CLK_SRAM0 1
>>>> +#define K210_CLK_SRAM1 2
>>>
>>
>> Patch 6 of the series removes the use of K210_CLK_CPU and K210_CLK_ACLK from the
>> device trees. I added that patch as the DT modification proper comes only at
>> patch 16. Maybe I should squash patch 6 into this one ?
>>
> 
> Preferably the defines are just left alone forever and then forgotten.
> The dt-bindings directory is almost ABI and so changing numbers or
> removing defines is hard to do. Usually patches in this directory are an
> additive thing.

I just did that. It works.

Ideally, patches 7, 8 and 9 need to go in together with the clk driver patch.
Since the builtin DTB patch precedes the clock driver patch that touches the
sysctl driver, I need to rework it a little, keeping the
SOC_DECLARE_BUILTIN_DTB() for now. And finally, a small DTS update patch needs
to be added too for the sysctl & sysclk nodes update. That would make it a 5
patch series for the clock driver addition. Would this work ?

Or, you just take patch 9 (clk doc) and patch 13 (clk driver), slightly modified
to move the sysctl register definitions into a common header (currently part of
patch 7). 2 patches only, without any other change, resulting in the clock
driver not being used until the rest of the series goes into 5.12. Do you prefer
that solution ?


-- 
Damien Le Moal
Western Digital Research