Re: [PATCH net-next v8 1/3] dt-bindings: ptp: renesas,rcar-gen4-gptp: Add R-Car Gen4

[netdev-bot+sashiko@kernel.org]

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

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

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 renesas,rcar-gen4-gptp binding describes the gPTP
  timer as a standalone device.
- [Low] The binding sets additionalProperties: false and has no
  'interrupts' property, but the Gen4 gPTP IP appears to have status…
- [Low] The commit message and the binding description (renesas,rcar-
  gen4-gptp.yaml line 19) say that on R-Car S4 the timer is 'shared…
- [Low] Not confirmed (baseline decision: false).
- [Low] Not confirmed (baseline decision: false).

--

dt-bindings: ptp: renesas,rcar-gen4-gptp: Add R-Car Gen4

This adds a DT binding for the R-Car Gen4 gPTP timer on S4, V4H and V4M.
The timer is described as a standalone node with reg, clocks,
power-domains and resets. The patch also adds a MAINTAINERS entry for the
binding.

> diff --git a/Documentation/devicetree/bindings/ptp/renesas,rcar-gen4-gptp.yaml b/Documentation/devicetree/bindings/ptp/renesas,rcar-gen4-gptp.yaml
> new file mode 100644
> index 0000000000000..3edd64d400387
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ptp/renesas,rcar-gen4-gptp.yaml
> @@ -0,0 +1,64 @@

[ ... ]

> +description:
> +  The R-Car Gen4 gPTP timer enables accurate synchronization of the clock in
> +  the control system. The timer is system-wide and used by different Ethernet
> +  devices on each Gen4 platform.
> +
> +    - On R-Car S4 it is shared between RSWITCH and RAVB.

[Severity: Low]
Is the S4 part of this accurate? The tree doesn't seem to have an EtherAVB
on S4:

- renesas,etheravb.yaml lists only r8a779a0, r8a779g0 and r8a779h0 under
  renesas,etheravb-rcar-gen4. There is no renesas,etheravb-r8a779f0.
- ravb_main.c has no S4 match.
- r8a779f0.dtsi only has the renesas,r8a779f0-ether-switch node and no
  etheravb node.

Also, "ptp: Add driver for R-Car Gen4" later in the series only mentions
RSWITCH for S4.

The commit message makes the same claim:

  - On R-Car S4 it is shared between RSWITCH and RAVB.

> +    - On R-Car V4H it is shared between RTSN and RAVB.
> +    - On R-Car V4M it is only used by RAVB.

[ ... ]

> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - power-domains
> +  - resets

[Severity: Low]
This is a minor point. On V4H, the reset (and clock) here is the RTSN
module reset 2723, which tsn0 also uses. On S4 it would be the RSWITCH
module.

In the final series state this does not cause a conflict:

- ptp_rcar_gen4.c never calls any reset_control API.
- rtsn and ptp_rcar_gen4 cannot be bound at the same time because of the
  exclusive MMIO claim described below.

Once that register overlap is resolved and both drivers can bind,
rtsn_reset() would pulse the shared line:

drivers/net/ethernet/renesas/rtsn.c:rtsn_reset() {
    ...
	reset_control_reset(priv->reset);
    ...
}

Would that reset the timer that RAVB relies on?

"ptp: Add driver for R-Car Gen4" says the gPTP shares its clock and reset
with RSWITCH or RTSN. Could the binding description say so too?

> +
> +additionalProperties: false

[Severity: Low]
The binding has additionalProperties: false and no interrupts property.
How would the timer's interrupt outputs be described on this node?

On S4, r8a779f0.dtsi routes dedicated SPIs named "gptp0_status" and
"gptp1_status" in the rswitch node. They might come from the gPTP block,
though they could also be RSWITCH status aggregation. The tree alone
doesn't make that clear.

A binding should describe the hardware fully, even where the driver doesn't
use a resource. Should an optional interrupts property be added here?

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/r8a779g0-cpg-mssr.h>
> +    #include <dt-bindings/power/r8a779g0-sysc.h>
> +
> +    ptp@e6449000 {
> +            compatible = "renesas,r8a779g0-gptp", "renesas,rcar-gen4-gptp";
> +            reg = <0xe6449000 0x500>;

[Severity: Medium]
Isn't this the same register window that renesas,ethertsn.yaml already
requires as the second reg entry of the RTSN node?

Documentation/devicetree/bindings/net/renesas,ethertsn.yaml:
  reg-names:
    items:
      - const: tsnes
      - const: gptp

r8a779g0.dtsi uses this window in tsn0:

		tsn0: ethernet@e6460000 {
			compatible = "renesas,r8a779g0-ethertsn", "renesas,rcar-gen4-ethertsn";
			reg = <0 0xe6460000 0 0x7000>,
			      <0 0xe6449000 0 0x500>;
			reg-names = "tsnes", "gptp";

rtsn_probe() maps that region exclusively and registers its own PHC on it:

drivers/net/ethernet/renesas/rtsn.c:rtsn_probe() {
    ...
	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gptp");
    ...
	ptpaddr = devm_ioremap_resource(&pdev->dev, res);
    ...
	priv->ptp_priv = rcar_gen4_ptp_alloc(pdev, ptpaddr);
    ...
}

Later in the series, "arm64: dts: renesas: r8a779g0: Add gPTP node" adds a
ptp@e6449000 node with the same reg, clock and reset. In "ptp: Add driver
for R-Car Gen4", ptp_rcar_gen4_probe() maps it with
devm_platform_ioremap_resource(), which is also an exclusive request.

Take a V4H board with &tsn0 enabled, such as White Hawk Single. If it also
enables &gptp so that RAVB can use ptp-timer = <&gptp>, won't whichever of
rtsn and ptp_rcar_gen4 probes second fail with -EBUSY? That would lose
either the TSN interface or the PTP clock that RAVB needs.

S4 has a similar overlap. The gPTP block sits at RSWITCH base +
RSWITCH_GPTP_OFFSET_S4, inside the base region that renesas_eth_sw_probe()
maps exclusively. A renesas,r8a779f0-gptp node written to this binding
would overlap that region.

dtc doesn't check for reg overlap between non-sibling nodes, so this would
only show up at runtime.

Is there a transition plan for the gptp reg entry in renesas,ethertsn.yaml
and for the rswitch mapping, such as deprecating the ethertsn gptp reg?
The commit message doesn't mention the overlap or the DT ABI impact.

> +            clocks = <&cpg CPG_MOD 2723>;
> +            power-domains = <&sysc R8A779G0_PD_ALWAYS_ON>;
> +            resets = <&cpg 2723>;
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe23913b1e432..085fedac38ef5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23822,6 +23822,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/mtd/renesas-nandc.yaml
>  F:	drivers/mtd/nand/raw/renesas-nand-controller.c
>  
> +RENESAS R-CAR GEN4 GPTP DRIVER
> +M:	Niklas Söderlund <niklas.soderlund@ragnatech.se>
> +L:	linux-renesas-soc@vger.kernel.org
> +S:	Supported
> +F:	Documentation/devicetree/bindings/ptp/renesas,rcar-gen4-gptp.yaml

[Severity: Low]
This is a minor note. At this point the "GPTP DRIVER" section lists only
the binding, and nothing binds renesas,rcar-gen4-gptp yet.

Later in the series this is resolved. "ptp: Add driver for R-Car Gen4"
adds F: drivers/ptp/ptp_rcar_gen4.c to this section, along with a platform
driver that matches renesas,rcar-gen4-gptp. Putting the binding before the
driver looks fine.

> +
>  RENESAS R-CAR GYROADC DRIVER
>  M:	Marek Vasut <marek.vasut+renesas@mailbox.org>
>  L:	linux-iio@vger.kernel.org

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