Re: [linux dev-6.6 v1 1/3] arm64: dts: nuvoton: npcm8xx: add reference 25m clock property

[Andrew Jeffery <andrew@codeconstruct.com.au>]

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