Re: [PATCH net-next v8 3/3] arm64: dts: renesas: r8a779g0: Add gPTP node

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new `gptp: ptp@e6449000` node in
  arch/arm64/boot/dts/renesas/r8a779g0.dtsi uses `reg = <0 0xe6449000 0…

--

arm64: dts: renesas: r8a779g0: Add gPTP node

This adds a gptp: ptp@e6449000 node to the R-Car V4H SoC dtsi. The commit
message says the gPTP module is shared between the RAVB and RTSN Ethernet
devices.

> diff --git a/arch/arm64/boot/dts/renesas/r8a779g0.dtsi b/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> index 5284923b6cef7..f2d6696680db8 100644
> --- a/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> @@ -606,6 +606,15 @@ mfis: system-controller@e6260000 {
>  			status = "disabled";
>  		};
>  
> +		gptp: ptp@e6449000 {
> +			compatible = "renesas,r8a779g0-gptp", "renesas,rcar-gen4-gptp";
> +			reg = <0 0xe6449000 0 0x500>;
> +			clocks = <&cpg CPG_MOD 2723>;
> +			power-domains = <&sysc R8A779G0_PD_ALWAYS_ON>;
> +			resets = <&cpg 2723>;
> +			status = "disabled";
> +		};
> +
>  		tsn0: ethernet@e6460000 {
>  			compatible = "renesas,r8a779g0-ethertsn", "renesas,rcar-gen4-ethertsn";
>  			reg = <0 0xe6460000 0 0x7000>,

[Severity: Medium]
Can tsn0 and the new gptp node both be enabled on the same board?

The existing tsn0 node, which this patch does not change, already claims
the same window as its second reg entry:

	tsn0: ethernet@e6460000 {
		reg = <0 0xe6460000 0 0x7000>,
		      <0 0xe6449000 0 0x500>;
		reg-names = "tsnes", "gptp";
		...
		clocks = <&cpg CPG_MOD 2723>;
		resets = <&cpg 2723>;

rtsn_probe() in drivers/net/ethernet/renesas/rtsn.c maps that region
exclusively and runs the timer itself:

	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gptp");
	...
	ptpaddr = devm_ioremap_resource(&pdev->dev, res);
	if (IS_ERR(ptpaddr)) {
		ret = PTR_ERR(ptpaddr);
		goto error_free;
	}

	priv->ptp_priv = rcar_gen4_ptp_alloc(pdev, ptpaddr);

ptp_rcar_gen4_probe() in drivers/ptp/ptp_rcar_gen4.c claims the same
range:

	priv->base = devm_platform_ioremap_resource(pdev, 0);
	if (IS_ERR(priv->base))
		return PTR_ERR(priv->base);

If a board sets both &tsn0 and &gptp to okay, the device that probes
second should get -EBUSY from its request on 0xe6449000-0xe64494ff. When
rtsn is the one that fails, the whole TSN Ethernet interface is lost,
because a failed gptp mapping takes the error_free path in rtsn_probe().

The commit message says:

    The gPTP module is shared between the RAVB and RTSN Ethernet devices on
    the SoC.

The new renesas,rcar-gen4-gptp.yaml binding also says "On R-Car V4H it is
shared between RTSN and RAVB". However, renesas,ethertsn.yaml still
requires the gptp reg entry on the TSN node. rtsn also has no ptp-timer
phandle path. No DTS under arch/arm64/boot/dts/renesas/ references &gptp
yet, so RAVB is not connected to this node either. RAVB uses the timer
only through of_parse_phandle(..., "ptp-timer", 0).

Would it make sense to either move RTSN over to the shared node or
document that the two nodes are mutually exclusive? Moving RTSN would mean
dropping the gptp reg from tsn0 and from the ethertsn binding, and
switching rtsn to a ptp-timer phandle.

A shared-node conversion would also need to handle reset 2723. rtsn takes
it with the exclusive devm_reset_control_get() and pulses it in
rtsn_reset(). That looks like it would wipe the timer setup, which
ptp_rcar_gen4_probe() writes only once.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110080727.2377040-1-niklas.soderlund%2Brenesas%40ragnatech.se