| Message ID | 20240714152617.3055768-1-tmaimon77@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [linux,dev-6.6,v1,1/3] arm64: dts: nuvoton: npcm8xx: add reference 25m clock property | expand |
Hi Tomer, In the future, can you please send your series with a cover letter with the patches threaded under it? If you're not already using it, b4 is a helpful tool for sending patches: https://b4.docs.kernel.org/en/latest/ I ask because it's not clear to me what the relationship of this series is with respect to what's going on upstream. A cover letter is a great place to explain whether the patches are: 1. A backport of those under review upstream 2. A backport of patches already merged upstream 3. Specific to the openbmc/linux tree and have no upstream equivalent In the case of 1 and 2 (which are the ideal cases), I really prefer you include a link to the upstream equivalents. The link makes it easier for me to gauge how mature the patches are. Regarding the patch content (rather than process), while the patches all touch the NPCM8XX devicetree, they don't seem to have a coherent feel otherwise :( On Sun, 2024-07-14 at 18:26 +0300, Tomer Maimon wrote: > The NPCM8XX clock driver uses a 25Mhz external clock, therefore adding > clock property. > > The new required clock property does not break the NPCM8XX clock ABI > since the NPCM8XX clock driver hasn't merged yet to the Linux vanilla. This is a statement with respect to upstream, but it seems we've already applied some of the patches here, and so there's possibly a concern? > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 9 +++++---- > arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts | 7 +++++++ > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > index 91c1b5c4d635..9bd22f7d43f4 100644 > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > @@ -58,6 +58,7 @@ clk: clock-controller@f0801000 { > compatible = "nuvoton,npcm845-clk"; > #clock-cells = <1>; > reg = <0x0 0xf0801000 0x0 0x1000>; > + clocks = <&refclk>; > }; > > apb { > @@ -81,7 +82,7 @@ timer0: timer@8000 { > compatible = "nuvoton,npcm845-timer"; > interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; > reg = <0x8000 0x1C>; > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > + clocks = <&refclk>; > clock-names = "refclk"; > }; > > @@ -153,7 +154,7 @@ watchdog0: watchdog@801c { > interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; > reg = <0x801c 0x4>; > status = "disabled"; > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > + clocks = <&refclk>; > syscon = <&gcr>; > }; > > @@ -162,7 +163,7 @@ watchdog1: watchdog@901c { > interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>; > reg = <0x901c 0x4>; > status = "disabled"; > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > + clocks = <&refclk>; > syscon = <&gcr>; > }; > > @@ -171,7 +172,7 @@ watchdog2: watchdog@a01c { > interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>; > reg = <0xa01c 0x4>; > status = "disabled"; > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > + clocks = <&refclk>; > syscon = <&gcr>; > }; > }; > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > index a5ab2bc0f835..83c2f4e138e5 100644 > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > @@ -19,6 +19,13 @@ chosen { > memory { > reg = <0x0 0x0 0x0 0x40000000>; > }; > + > + refclk: refclk-25mhz { The node name should probably just be 'clock' according to the generic node names recommendation? > + compatible = "fixed-clock"; > + clock-output-names = "ref"; > + clock-frequency = <25000000>; > + #clock-cells = <0>; > + }; Defining this in the .dts but referencing the label inside the .dtsi feels a bit off to me (as the .dtsi is no-longer self-contained). How about we define the node in the .dtsi but override it in the .dts? Andrew
Hi Andrew, Thanks for your comments. On Mon, 15 Jul 2024 at 09:05, Andrew Jeffery <andrew@codeconstruct.com.au> wrote: > > Hi Tomer, > > In the future, can you please send your series with a cover letter with > the patches threaded under it? Sure! > > If you're not already using it, b4 is a helpful tool for sending > patches: I wasn't aware to B$, I will try it, thanks :-) > > https://b4.docs.kernel.org/en/latest/ > > I ask because it's not clear to me what the relationship of this series > is with respect to what's going on upstream. A cover letter is a great > place to explain whether the patches are: > > 1. A backport of those under review upstream > 2. A backport of patches already merged upstream > 3. Specific to the openbmc/linux tree and have no upstream equivalent > > In the case of 1 and 2 (which are the ideal cases), I really prefer you > include a link to the upstream equivalents. The link makes it easier > for me to gauge how mature the patches are. If I am sending one patch only do you like me to add under --- in the patch explanation as well? > > Regarding the patch content (rather than process), while the patches > all touch the NPCM8XX devicetree, they don't seem to have a coherent > feel otherwise :( > > On Sun, 2024-07-14 at 18:26 +0300, Tomer Maimon wrote: > > The NPCM8XX clock driver uses a 25Mhz external clock, therefore adding > > clock property. > > > > The new required clock property does not break the NPCM8XX clock ABI > > since the NPCM8XX clock driver hasn't merged yet to the Linux vanilla. > > This is a statement with respect to upstream, but it seems we've > already applied some of the patches here, and so there's possibly a > concern? Unfortunately, the NPCM8XX clock driver has been removed in dev-6.6, so the OpenBMC Linux kernel is dev-6.6 is in the same state as the Linux kernel vanilla. BTW, I don't see any concern with the reference clock patch, but the DT maintainers asked me to mention it not cause any ABI issue. > > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 9 +++++---- > > arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts | 7 +++++++ > > 2 files changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > index 91c1b5c4d635..9bd22f7d43f4 100644 > > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > @@ -58,6 +58,7 @@ clk: clock-controller@f0801000 { > > compatible = "nuvoton,npcm845-clk"; > > #clock-cells = <1>; > > reg = <0x0 0xf0801000 0x0 0x1000>; > > + clocks = <&refclk>; > > }; > > > > apb { > > @@ -81,7 +82,7 @@ timer0: timer@8000 { > > compatible = "nuvoton,npcm845-timer"; > > interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; > > reg = <0x8000 0x1C>; > > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > > + clocks = <&refclk>; > > clock-names = "refclk"; > > }; > > > > @@ -153,7 +154,7 @@ watchdog0: watchdog@801c { > > interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; > > reg = <0x801c 0x4>; > > status = "disabled"; > > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > > + clocks = <&refclk>; > > syscon = <&gcr>; > > }; > > > > @@ -162,7 +163,7 @@ watchdog1: watchdog@901c { > > interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>; > > reg = <0x901c 0x4>; > > status = "disabled"; > > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > > + clocks = <&refclk>; > > syscon = <&gcr>; > > }; > > > > @@ -171,7 +172,7 @@ watchdog2: watchdog@a01c { > > interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>; > > reg = <0xa01c 0x4>; > > status = "disabled"; > > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > > + clocks = <&refclk>; > > syscon = <&gcr>; > > }; > > }; > > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > index a5ab2bc0f835..83c2f4e138e5 100644 > > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > @@ -19,6 +19,13 @@ chosen { > > memory { > > reg = <0x0 0x0 0x0 0x40000000>; > > }; > > + > > + refclk: refclk-25mhz { > > The node name should probably just be 'clock' according to the generic > node names recommendation? What do you mean? refclock? I am not sure, for example: https://elixir.bootlin.com/linux/v6.10-rc7/source/arch/arm64/boot/dts/freescale/imx8mq-evk.dts#L24 > > > + compatible = "fixed-clock"; > > + clock-output-names = "ref"; > > + clock-frequency = <25000000>; > > + #clock-cells = <0>; > > + }; > > Defining this in the .dts but referencing the label inside the .dtsi > feels a bit off to me (as the .dtsi is no-longer self-contained). How > about we define the node in the .dtsi but override it in the .dts? I had a dissection about it with Krzysztof :-) I was told that since it is a reference clock on the board and not inside the SoC it should be defined in the DTS. > > Andrew Best regards, Tomer
On Mon, 2024-07-15 at 12:16 +0300, Tomer Maimon wrote: > Hi Andrew, > > Thanks for your comments. > > On Mon, 15 Jul 2024 at 09:05, Andrew Jeffery > <andrew@codeconstruct.com.au> wrote: > > > > Hi Tomer, > > > > In the future, can you please send your series with a cover letter with > > the patches threaded under it? > Sure! > > > > If you're not already using it, b4 is a helpful tool for sending > > patches: > I wasn't aware to B$, I will try it, thanks :-) > > > > https://b4.docs.kernel.org/en/latest/ > > > > I ask because it's not clear to me what the relationship of this series > > is with respect to what's going on upstream. A cover letter is a great > > place to explain whether the patches are: > > > > 1. A backport of those under review upstream > > 2. A backport of patches already merged upstream > > 3. Specific to the openbmc/linux tree and have no upstream equivalent > > > > In the case of 1 and 2 (which are the ideal cases), I really prefer you > > include a link to the upstream equivalents. The link makes it easier > > for me to gauge how mature the patches are. > If I am sending one patch only do you like me to add under --- in the > patch explanation as well? Yeah, that would be great, if you're just sending the one patch rather than a series. Thanks. > > > > Regarding the patch content (rather than process), while the patches > > all touch the NPCM8XX devicetree, they don't seem to have a coherent > > feel otherwise :( > > > > On Sun, 2024-07-14 at 18:26 +0300, Tomer Maimon wrote: > > > The NPCM8XX clock driver uses a 25Mhz external clock, therefore adding > > > clock property. > > > > > > The new required clock property does not break the NPCM8XX clock ABI > > > since the NPCM8XX clock driver hasn't merged yet to the Linux vanilla. > > > > This is a statement with respect to upstream, but it seems we've > > already applied some of the patches here, and so there's possibly a > > concern? > Unfortunately, the NPCM8XX clock driver has been removed in dev-6.6, > so the OpenBMC Linux kernel is dev-6.6 is in the same state as the > Linux kernel vanilla. > BTW, I don't see any concern with the reference clock patch, but the > DT maintainers asked me to mention it not cause any ABI issue. Okay. I guess I should have poked at the (absence) of the driver. > > > > > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > > --- > > > arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 9 +++++---- > > > arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts | 7 +++++++ > > > 2 files changed, 12 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > > index 91c1b5c4d635..9bd22f7d43f4 100644 > > > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > > @@ -58,6 +58,7 @@ clk: clock-controller@f0801000 { > > > compatible = "nuvoton,npcm845-clk"; > > > #clock-cells = <1>; > > > reg = <0x0 0xf0801000 0x0 0x1000>; > > > + clocks = <&refclk>; > > > }; > > > > > > apb { > > > @@ -81,7 +82,7 @@ timer0: timer@8000 { > > > compatible = "nuvoton,npcm845-timer"; > > > interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; > > > reg = <0x8000 0x1C>; > > > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > > > + clocks = <&refclk>; > > > clock-names = "refclk"; > > > }; > > > > > > @@ -153,7 +154,7 @@ watchdog0: watchdog@801c { > > > interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; > > > reg = <0x801c 0x4>; > > > status = "disabled"; > > > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > > > + clocks = <&refclk>; > > > syscon = <&gcr>; > > > }; > > > > > > @@ -162,7 +163,7 @@ watchdog1: watchdog@901c { > > > interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>; > > > reg = <0x901c 0x4>; > > > status = "disabled"; > > > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > > > + clocks = <&refclk>; > > > syscon = <&gcr>; > > > }; > > > > > > @@ -171,7 +172,7 @@ watchdog2: watchdog@a01c { > > > interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>; > > > reg = <0xa01c 0x4>; > > > status = "disabled"; > > > - clocks = <&clk NPCM8XX_CLK_REFCLK>; > > > + clocks = <&refclk>; > > > syscon = <&gcr>; > > > }; > > > }; > > > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > > index a5ab2bc0f835..83c2f4e138e5 100644 > > > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > > @@ -19,6 +19,13 @@ chosen { > > > memory { > > > reg = <0x0 0x0 0x0 0x40000000>; > > > }; > > > + > > > + refclk: refclk-25mhz { > > > > The node name should probably just be 'clock' according to the generic > > node names recommendation? > What do you mean? refclock? I am not sure, for example: > https://elixir.bootlin.com/linux/v6.10-rc7/source/arch/arm64/boot/dts/freescale/imx8mq-evk.dts#L24 I meant the node name (refclk-25mhz), not the label (refclk), but plenty of other devicetrees call it random things, so don't worry about it. > > > > > + compatible = "fixed-clock"; > > > + clock-output-names = "ref"; > > > + clock-frequency = <25000000>; > > > + #clock-cells = <0>; > > > + }; > > > > Defining this in the .dts but referencing the label inside the .dtsi > > feels a bit off to me (as the .dtsi is no-longer self-contained). How > > about we define the node in the .dtsi but override it in the .dts? > I had a dissection about it with Krzysztof :-) I was told that since > it is a reference clock on the board and not inside the SoC it should > be defined in the DTS. Hah, okay, I guess do whatever Krysztof recommends. If that's what you've got, then it is what it is. Andrew
diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi index 91c1b5c4d635..9bd22f7d43f4 100644 --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi @@ -58,6 +58,7 @@ clk: clock-controller@f0801000 { compatible = "nuvoton,npcm845-clk"; #clock-cells = <1>; reg = <0x0 0xf0801000 0x0 0x1000>; + clocks = <&refclk>; }; apb { @@ -81,7 +82,7 @@ timer0: timer@8000 { compatible = "nuvoton,npcm845-timer"; interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; reg = <0x8000 0x1C>; - clocks = <&clk NPCM8XX_CLK_REFCLK>; + clocks = <&refclk>; clock-names = "refclk"; }; @@ -153,7 +154,7 @@ watchdog0: watchdog@801c { interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; reg = <0x801c 0x4>; status = "disabled"; - clocks = <&clk NPCM8XX_CLK_REFCLK>; + clocks = <&refclk>; syscon = <&gcr>; }; @@ -162,7 +163,7 @@ watchdog1: watchdog@901c { interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>; reg = <0x901c 0x4>; status = "disabled"; - clocks = <&clk NPCM8XX_CLK_REFCLK>; + clocks = <&refclk>; syscon = <&gcr>; }; @@ -171,7 +172,7 @@ watchdog2: watchdog@a01c { interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>; reg = <0xa01c 0x4>; status = "disabled"; - clocks = <&clk NPCM8XX_CLK_REFCLK>; + clocks = <&refclk>; syscon = <&gcr>; }; }; diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts index a5ab2bc0f835..83c2f4e138e5 100644 --- a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts @@ -19,6 +19,13 @@ chosen { memory { reg = <0x0 0x0 0x0 0x40000000>; }; + + refclk: refclk-25mhz { + compatible = "fixed-clock"; + clock-output-names = "ref"; + clock-frequency = <25000000>; + #clock-cells = <0>; + }; }; &serial0 {
The NPCM8XX clock driver uses a 25Mhz external clock, therefore adding clock property. The new required clock property does not break the NPCM8XX clock ABI since the NPCM8XX clock driver hasn't merged yet to the Linux vanilla. Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> --- arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 9 +++++---- arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts | 7 +++++++ 2 files changed, 12 insertions(+), 4 deletions(-)