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

From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Stephen Boyd <sboyd@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Rob Herring <robh+dt@kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>
Cc: Sean Anderson <seanga2@gmail.com>
Subject: Re: [PATCH v10 09/23] dt-binding: clock: Document canaan,k210-clk bindings
Date: Thu, 17 Dec 2020 10:43:50 +0000	[thread overview]
Message-ID: <CH2PR04MB6522E9EC44FF8FFA5981107CE7C40@CH2PR04MB6522.namprd04.prod.outlook.com> (raw)
In-Reply-To: 160820021596.1580929.1686334340234415270@swboyd.mtv.corp.google.com

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@gmail.com>
>>>>   * 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