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

[Andrew Jeffery]

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 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?

> +		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