Re: [PATCH v2 3/3] arm64: zynqmp: Switch Versal NET to firmware clock interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect clock identifier assignments for GEM (Ethernet) controllers due to using legacy Versal IDs instead of the new Versal NET macros.
- [High] Untested power-management node introduces a fatal hardware interrupt collision.
- [Medium] Missing `resets` properties for standard peripherals despite adding full reset controller bindings.
--

commit 0cc079ca7fe5612fdd8dc6e417a47204046c354d
Author: Michal Simek <michal.simek@amd.com>

arm64: zynqmp: Switch Versal NET to firmware clock interface

This commit switches Versal NET from using fixed clocks to the firmware-based
CCF clock interface. It also adds device tree macro headers for Versal NET
and base Versal clocks, power domains, and resets that are required by the
CCF clock dtsi.

> diff --git a/arch/arm64/boot/dts/xilinx/versal-net-clk.dtsi b/arch/arm64/boot/dts/xilinx/versal-net-clk.dtsi
> index b7a8a1a512cbd..dead487539cde 100644
> --- a/arch/arm64/boot/dts/xilinx/versal-net-clk.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/versal-net-clk.dtsi

[ ... ]

> +	firmware {
> +		versal_net_firmware: versal-net-firmware {
> +			compatible = "xlnx,versal-net-firmware", "xlnx,versal-firmware";
> +			bootph-all;
> +			method = "smc";
> +			#power-domain-cells = <1>;
> +
> +			versal_net_reset: reset-controller {
> +				compatible = "xlnx,versal-net-reset";
> +				#reset-cells = <1>;
> +			};
> +
> +			versal_net_clk: clock-controller {
> +				bootph-all;
> +				#clock-cells = <1>;
> +				compatible = "xlnx,versal-net-clk", "xlnx,versal-clk";
> +				clocks = <&ref_clk>, <&ref_clk>, <&ref_clk>;
> +				clock-names = "ref", "pl_alt_ref", "alt_ref";
> +			};
> +
> +			versal_net_power: power-management { /* untested */
> +				compatible = "xlnx,zynqmp-power";
> +				interrupt-parent = <&gic>;
> +				interrupts = <0 57 4>;

[Severity: High]
Will this interrupt assignment cause a probe failure?

The interrupts property here is identically claimed by the zynqmp-ipi node
below.

Looking at the power driver (drivers/soc/xilinx/zynqmp_power.c), it
requests this interrupt via devm_request_threaded_irq() without the
IRQF_SHARED flag, while the IPI mailbox driver uses IRQF_SHARED.

Because the flags mismatch and IRQF_SHARED is missing from the power driver,
the second driver to initialize will fail, which could break either system
power management or critical IPI communication.

> +				mboxes = <&ipi_mailbox_pmu1 0>,
> +					 <&ipi_mailbox_pmu1 1>;
> +				mbox-names = "tx", "rx";
> +			};
> +		};
>  	};
>  
> -	clk200: clk200 {

[ ... ]

> +	zynqmp-ipi {
> +		compatible = "xlnx,zynqmp-ipi-mailbox";
> +		interrupt-parent = <&gic>;
> +		interrupts = <0 57 4>;

[ ... ]

>  &can0 {
> -	clocks = <&clk160>, <&clk160>;
> +	clocks = <&can0_clk>, <&versal_net_clk LPD_LSBUS>;
> +	power-domains = <&versal_net_firmware PM_DEV_CAN_FD_0>;
>  };
>  
>  &can1 {
> -	clocks = <&clk160>, <&clk160>;
> +	clocks = <&can1_clk>, <&versal_net_clk LPD_LSBUS>;
> +	power-domains = <&versal_net_firmware PM_DEV_CAN_FD_1>;
>  };

[Severity: Medium]
Should these peripheral nodes include their corresponding reset properties?

The commit introduces comprehensive reset IDs (e.g., VERSAL_RST_GEM_0,
VERSAL_RST_CAN_FD_0) and instantiates the versal_net_reset controller, but
the resets property is only added to the usb0 and usb1 nodes.

Standard peripheral drivers like the GEM ethernet driver (macb) actively query
and trigger optional device resets via reset_control_get_optional() during
probe or error recovery. Omitting these reset links might prevent the drivers
from fully resetting the hardware, leading to reliability issues across warm
reboots.

>  
>  &gem0 {
> -	clocks = <&clk125>, <&clk125>, <&clk125>, <&clk125>, <&clk250>;
> +	clocks = <&versal_net_clk LPD_LSBUS>,
> +		 <&versal_net_clk GEM0_REF>, <&versal_net_clk GEM0_TX>,
> +		 <&versal_net_clk GEM0_RX>, <&versal_net_clk GEM_TSU>;

[Severity: High]
Are these the correct clock identifiers for the GEM ethernet controllers?

The xlnx-versal-net-clk.h file introduces new clock IDs for Versal NET, such
as GEM0_REF_TX (0xA8) and GEM0_REF_RX (0xA9). However, it appears the legacy
Versal clock macros GEM0_TX (49) and GEM0_RX (48) are not undefined, so this
node compiles using the legacy IDs. 

When the GEM Ethernet driver attempts to enable its clocks, the dynamically
queried clock driver will request ID 49 instead of 0xA8 from the firmware.
Will this fail to enable the correct clocks and break network connectivity?

> +	power-domains = <&versal_net_firmware PM_DEV_GEM_0>;
>  };
>  
>  &gem1 {
> -	clocks = <&clk125>, <&clk125>, <&clk125>, <&clk125>, <&clk250>;
> +	clocks = <&versal_net_clk LPD_LSBUS>,
> +		 <&versal_net_clk GEM1_REF>, <&versal_net_clk GEM1_TX>,
> +		 <&versal_net_clk GEM1_RX>, <&versal_net_clk GEM_TSU>;
> +	power-domains = <&versal_net_firmware PM_DEV_GEM_1>;
>  };

[ ... ]

> diff --git a/arch/arm64/boot/dts/xilinx/xlnx-versal-net-clk.h b/arch/arm64/boot/dts/xilinx/xlnx-versal-net-clk.h
> new file mode 100644
> index 0000000000000..f894d044c6f94
> --- /dev/null
> +++ b/arch/arm64/boot/dts/xilinx/xlnx-versal-net-clk.h
> @@ -0,0 +1,78 @@

[ ... ]

> +#define GEM0_REF_RX	0xA9
> +#define GEM0_REF_TX	0xA8
> +#define GEM1_REF_RX	0xA2
> +#define GEM1_REF_TX	0xA1

[ ... ]

> +/* Remove Versal specific node IDs */
> +#undef APU_PLL
> +#undef RPU_PLL

[ ... ]

> +#undef MIO_50_OR_51
> +#undef MIO_24_OR_25
> +
> +#endif

[Severity: High]
Should the legacy GEM0_TX, GEM0_RX, GEM1_TX, and GEM1_RX macros be undefined
here?

Since they are not removed along with the other Versal specific node IDs, the
dtsi file silently falls back to the old identifiers instead of the new
GEM0_REF_TX style IDs, leading to the incorrect clock request discussed above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780063921.git.michal.simek@amd.com?part=3