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

[netdev-bot+sashiko]

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

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

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).

- [High] dts: shared reset line between gptp and tsn0 causes probe
  failure and state corruption
- [Medium] The new `gptp: ptp@e6449000` node in r8a779g0.dtsi claims `reg
  = <0 0xe6449000 0 0x500>`, plus CPG clock 2723 and CPG reset 2723.

Pre-existing issues:
- [Low] Documentation/devicetree/bindings/ptp/renesas,rcar-gen4-gptp.yaml
  lists `resets` as required, and the new r8a779g0 node supplies `resets…

--

arm64: dts: renesas: r8a779g0: Add gPTP node

This adds a disabled gPTP node at 0xe6449000 to the R-Car V4H (r8a779g0)
SoC dtsi. The node uses CPG module clock and reset 2723 and the always-on
power domain. 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 8a291447b9045..7430b663cf92e 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>;

[Severity: Medium]

Does this window overlap with the existing tsn0 node? The tsn0 node just
below is not changed by this patch. It still claims the same range as its
second reg entry and uses the same clock and reset:

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

Both drivers request this region exclusively. rtsn_probe() in
drivers/net/ethernet/renesas/rtsn.c does:

	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gptp");
	...
	ptpaddr = devm_ioremap_resource(&pdev->dev, res);

It then registers its own PHC through rcar_gen4_ptp_register().

ptp_rcar_gen4_probe() in drivers/ptp/ptp_rcar_gen4.c does:

	priv->base = devm_platform_ioremap_resource(pdev, 0);

Both paths go through devm_request_mem_region(), so whichever driver
probes second fails with -EBUSY.

white-hawk-single.dtsi already enables tsn0. Suppose a V4H board like that
also enables &gptp so RAVB can reference it through ptp-timer in
ravb_gen4_ptp_probe(). Would one of the two devices then fail to probe?
The gptp node comes first in DT order, so RTSN would usually be the one
that fails, and the TSN Ethernet port would be lost.

The node is disabled and no board enables it yet, so nothing breaks at
this commit.

The commit message says:

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

RTSN does not seem to be converted to use the shared node, though. It
still maps the window privately. Should tsn0 and rtsn.c be updated to
reference this node? If not, should the commit message say that enabling
&gptp together with &tsn0 is not supported?

> +			clocks = <&cpg CPG_MOD 2723>;
> +			power-domains = <&sysc R8A779G0_PD_ALWAYS_ON>;
> +			resets = <&cpg 2723>;

[Severity: High]
The existing tsn0 node also uses reset 2723, and rtsn.c treats
that reset as its own. rtsn_probe() takes it with
devm_reset_control_get(&pdev->dev, NULL), which is an exclusive get.
rtsn_reset() then pulses it with reset_control_reset() every time
rtsn_hw_init() runs, which happens whenever the TSN port is brought
up.

This node makes the gPTP block a resource that RAVB can reference
through ptp-timer. Suppose RTSN brings its port up after
ptp_rcar_gen4 has programmed the timer. The reset pulse clears the
gPTP registers (PTPTMEC_REG and the rest) under ptp_rcar_gen4 and
under every RAVB user of that clock. Nothing reprograms them
afterwards, so PTP synchronisation is lost for the whole system.

This also blocks handing the reset to the gPTP driver later.
ptp_rcar_gen4_probe() does not request a reset today, but the
binding requires the property. If the driver ever requests it, the
second exclusive get of the same line fails with -EBUSY, and one of
the two devices will not probe.

The node is disabled, so nothing breaks at this commit. But this
description is what lets both consumers be enabled together. How is
reset 2723 meant to be owned once gptp and tsn0 are both enabled?
Should rtsn.c stop pulsing the shared block when an external gPTP
node is in use, for example by switching to a shared reset that is
only deasserted? Or should tsn0 drop its reset and its gptp reg
window and reference this node instead?

[Severity: Low]

This isn't a bug introduced by this patch, but the
renesas,rcar-gen4-gptp.yaml binding lists resets as required, while
ptp_rcar_gen4_probe() never gets or deasserts a reset control:

	priv->base = devm_platform_ioremap_resource(pdev, 0);
	...
	priv->clk = devm_clk_get(dev, NULL);
	...
	pm_runtime_enable(dev);
	pm_runtime_resume_and_get(dev);

Does the driver depend on the gPTP module already being out of reset when
it probes? Reset 2723 given here is also used by tsn0.

The binding and the driver both come from earlier patches in this series
("dt-bindings: ptp: renesas,rcar-gen4-gptp: Add R-Car Gen4" and "ptp: Add
driver for R-Car Gen4"). Listing the reset in the binding is correct for
describing the hardware.

> +			status = "disabled";
> +		};
> +

[ ... ]

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