Re: [PATCH net-next v4 09/10] dt-bindings: net: renesas,etheravb: Add optional gPTP phandle for Gen4

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dt-bindings: net: renesas,etheravb: Add optional gPTP phandle for Gen4

This adds an optional ptp-timer phandle property to the EtherAVB binding
and restricts it to the renesas,etheravb-rcar-gen4 compatible, so that a
Gen4 RAVB node can reference the external system-wide gPTP clock shared
with RTSN on V4H and RSWITCH on S4.

> diff --git a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
> index 1e00ef5b3acda..fad002492421a 100644
> --- a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
> +++ b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
> @@ -97,6 +97,8 @@ properties:
>  
>    phy-handle: true
>  
> +  ptp-timer: true
> +

[Severity: High]
Is there an in-tree node that this phandle can point at on Gen4?

The property is re-enabled bare here, so it inherits the contract from
Documentation/devicetree/bindings/net/ethernet-controller.yaml:

  ptp-timer:
    $ref: /schemas/types.yaml#/definitions/phandle
    description:
      Specifies a reference to a node representing an IEEE 1588 PTP device.

On Gen4 there is no standalone PTP device node. On R-Car V4H the gPTP
block only exists as a register window of the RTSN controller node in
arch/arm64/boot/dts/renesas/r8a779g0.dtsi:

  tsn0: ethernet@e6460000 {
      ...
      reg-names = "tsnes", "gptp";

and on S4 it lives inside the RSWITCH node.

The consumer added by the closing patch of this series ("net: ethernet:
ravb: Add gPTP support for Gen4") resolves the phandle in
ravb_gen4_ptp_clock_index() with ptp_clock_index_by_of_node(), which
matches through ptp_clock_of_node_match() in drivers/ptp/ptp_clock.c:

  return (dev->parent && dev_of_node(dev->parent) == parent_np);

But the only Gen4 gPTP providers, rtsn.c and rswitch_main.c, both go
through rcar_gen4_ptp_register() in
drivers/net/ethernet/renesas/rcar_gen4_ptp.c:

  ptp_priv->clock = ptp_clock_register(&ptp_priv->info, NULL);

With a NULL parent, ptp->dev.parent is NULL and the of_node match cannot
succeed, so ravb_gen4_ptp_clock_index() returns -1.

Does that mean a Gen4 DTS writing ptp-timer = <&tsn0> validates and is
parsed but still ends up without PTP support? If so, would it make sense
to also pass the RTSN/RSWITCH device as the ptp_clock_register() parent,
and to add a local description plus an example here so DTS authors and
reviewers have a criterion for which node is a valid target?

>    '#address-cells':
>      description: Number of address cells for the MDIO bus.
>      const: 1

[ ... ]

> @@ -305,6 +307,15 @@ allOf:
>              items:
>                - const: fck
>                - const: refclk
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              const: renesas,etheravb-rcar-gen4
> +    then:
> +      properties:
> +        ptp-timer: false

[Severity: Low]
This isn't a bug, but one note on the ordering within the series, for the
record: at this patch alone the gated ptp-timer property has no reader,
and ravb_gen4_hw_info in drivers/net/ethernet/renesas/ravb_main.c still
sets

  .ptp = &ravb_gen3_ptp_info,

which describes the internal CCC_GAC gPTP with GTI derived from fck, i.e.
the opposite hardware model from the external gPTP this binding
describes.

The following patch, "net: ethernet: ravb: Add gPTP support for Gen4",
resolves that by adding ravb_gen4_ptp_probe() with

  priv->of_gptp = of_parse_phandle(priv->pdev->dev.of_node, "ptp-timer", 0);

and switching ravb_gen4_hw_info to

  .ptp = &ravb_gen4_ptp_info,

so the binding change here is inert documentation on its own and nothing
needs changing for this reason alone.