| Message ID | 20201213135056.24446-10-damien.lemoal@wdc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISC-V Kendryte K210 support improvements | expand |
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
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 ?
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.
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 ?
Quoting Damien Le Moal (2020-12-17 02:43:50) > > 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 ? > I was thinking of just applying this DT binding patch now so it gets merged into the next -rc1. Then anyone can use the defines because they're in Linus' tree and wherever the dts file ends up can just base on -rc1. I probably won't merge the clk driver until v5.12 given that the merge window is open.
On 2020/12/17 19:49, Stephen Boyd wrote: > Quoting Damien Le Moal (2020-12-17 02:43:50) >> >> 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 ? >> > > I was thinking of just applying this DT binding patch now so it gets > merged into the next -rc1. Then anyone can use the defines because > they're in Linus' tree and wherever the dts file ends up can just base > on -rc1. I probably won't merge the clk driver until v5.12 given that > the merge window is open. OK. Makes sense. Sending just the clk binding patch then, with the K210_CLK_ACLK clock definition left in. Thanks ! >
On 2020/12/17 19:49, Stephen Boyd wrote: > Quoting Damien Le Moal (2020-12-17 02:43:50) >> >> 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 ? >> > > I was thinking of just applying this DT binding patch now so it gets > merged into the next -rc1. Then anyone can use the defines because > they're in Linus' tree and wherever the dts file ends up can just base > on -rc1. I probably won't merge the clk driver until v5.12 given that > the merge window is open. > One more thing: to avoid dt-binding checks error for this patch, can you also take patch 8 that adds "canaan" as a vendor name ?
Quoting Damien Le Moal (2020-12-17 02:54:14) > > One more thing: to avoid dt-binding checks error for this patch, can you also > take patch 8 that adds "canaan" as a vendor name ? > Sure.
Quoting Damien Le Moal (2020-12-17 02:51:20) > On 2020/12/17 19:49, Stephen Boyd wrote: > > I was thinking of just applying this DT binding patch now so it gets > > merged into the next -rc1. Then anyone can use the defines because > > they're in Linus' tree and wherever the dts file ends up can just base > > on -rc1. I probably won't merge the clk driver until v5.12 given that > > the merge window is open. > > OK. Makes sense. Sending just the clk binding patch then, with the K210_CLK_ACLK > clock definition left in. > Did you send it?
On Sat, 2020-12-19 at 21:35 -0800, Stephen Boyd wrote: > Quoting Damien Le Moal (2020-12-17 02:51:20) > > On 2020/12/17 19:49, Stephen Boyd wrote: > > > I was thinking of just applying this DT binding patch now so it gets > > > merged into the next -rc1. Then anyone can use the defines because > > > they're in Linus' tree and wherever the dts file ends up can just base > > > on -rc1. I probably won't merge the clk driver until v5.12 given that > > > the merge window is open. > > > > OK. Makes sense. Sending just the clk binding patch then, with the K210_CLK_ACLK > > clock definition left in. > > > > Did you send it? Just did. My apologies for the delay. Already in vacation mode :) Happy holidays ! -- Damien Le Moal Western Digital
diff --git a/Documentation/devicetree/bindings/clock/canaan,k210-clk.yaml b/Documentation/devicetree/bindings/clock/canaan,k210-clk.yaml new file mode 100644 index 000000000000..565ca468cb44 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/canaan,k210-clk.yaml @@ -0,0 +1,54 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/canaan,k210-clk.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Canaan Kendryte K210 Clock Device Tree Bindings + +maintainers: + - Damien Le Moal <damien.lemoal@wdc.com> + +description: | + Canaan Kendryte K210 SoC clocks driver bindings. The clock + controller node must be defined as a child node of the K210 + system controller node. + + See also: + - dt-bindings/clock/k210-clk.h + +properties: + compatible: + const: canaan,k210-clk + + clocks: + description: + Phandle of the SoC 26MHz fixed-rate oscillator clock. + + '#clock-cells': + const: 1 + +required: + - compatible + - '#clock-cells' + - clocks + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/k210-clk.h> + clocks { + in0: oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <26000000>; + }; + }; + + /* ... */ + sysclk: clock-controller { + #clock-cells = <1>; + compatible = "canaan,k210-clk"; + clocks = <&in0>; + }; 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 +#define K210_CLK_CPU 0 +#define K210_CLK_SRAM0 1 +#define K210_CLK_SRAM1 2 +#define K210_CLK_AI 3 +#define K210_CLK_DMA 4 +#define K210_CLK_FFT 5 +#define K210_CLK_ROM 6 +#define K210_CLK_DVP 7 +#define K210_CLK_APB0 8 +#define K210_CLK_APB1 9 +#define K210_CLK_APB2 10 +#define K210_CLK_I2S0 11 +#define K210_CLK_I2S1 12 +#define K210_CLK_I2S2 13 +#define K210_CLK_I2S0_M 14 +#define K210_CLK_I2S1_M 15 +#define K210_CLK_I2S2_M 16 +#define K210_CLK_WDT0 17 +#define K210_CLK_WDT1 18 +#define K210_CLK_SPI0 19 +#define K210_CLK_SPI1 20 +#define K210_CLK_SPI2 21 +#define K210_CLK_I2C0 22 +#define K210_CLK_I2C1 23 +#define K210_CLK_I2C2 24 +#define K210_CLK_SPI3 25 +#define K210_CLK_TIMER0 26 +#define K210_CLK_TIMER1 27 +#define K210_CLK_TIMER2 28 +#define K210_CLK_GPIO 29 +#define K210_CLK_UART1 30 +#define K210_CLK_UART2 31 +#define K210_CLK_UART3 32 +#define K210_CLK_FPIOA 33 +#define K210_CLK_SHA 34 +#define K210_CLK_AES 35 +#define K210_CLK_OTP 36 +#define K210_CLK_RTC 37 -#endif /* K210_CLK_H */ +#define K210_NUM_CLKS 38 + +#endif /* CLOCK_K210_CLK_H */