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

[Tomer Maimon]

Hi Andrew,

Thanks for your comments.

On Mon, 15 Jul 2024 at 09:05, Andrew Jeffery
<andrew at 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 at 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 at f0801000 {
> >                       compatible = "nuvoton,npcm845-clk";
> >                       #clock-cells = <1>;
> >                       reg = <0x0 0xf0801000 0x0 0x1000>;
> > +                     clocks = <&refclk>;
> >               };
> >
> >               apb {
> > @@ -81,7 +82,7 @@ timer0: timer at 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 at 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 at 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 at 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