[PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[Sarath Babu Naidu Gaddam]

There is currently no standard property to pass PTP device index
information to ethernet driver when they are independent.

ptp-hardware-clock property will contain phandle to PTP clock node.

Freescale driver currently has this implementation but it will be
good to agree on a generic (optional) property name to link to PTP
phandle to Ethernet node. In future or any current ethernet driver
wants to use this method of reading the PHC index,they can simply use
this generic name and point their own PTP clock node, instead of
creating separate property names in each ethernet driver DT node.

axiethernet driver uses this method when PTP support is integrated.

Example:
	fman0: fman@1a00000 {
		ptp-hardware-clock = <&ptp_timer0>;
	}

	ptp_timer0: ptp-timer@1afe000 {
		compatible = "fsl,fman-ptp-timer";
		reg = <0x0 0x1afe000 0x0 0x1000>;
	}

Signed-off-by: Sarath Babu Naidu Gaddam <sarath.babu.naidu.gaddam@amd.com>
---
We want binding to be reviewed/accepted and then make changes in freescale
binding documentation to use this generic binding.

DT information:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
tree/arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi#n23

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
tree/Documentation/devicetree/bindings/net/fsl-fman.txt#n320

Freescale driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
tree/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c#n467

Changes in V2:
1) Changed the ptimer-handle to ptp-hardware-clock based on
   Richard Cochran's comment.
2) Updated commit description.
---
 .../devicetree/bindings/net/ethernet-controller.yaml         | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 3aef506fa158..d2863c1dd585 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -161,6 +161,11 @@ properties:
       - auto
       - in-band-status
 
+  ptp-hardware-clock:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Specifies a reference to a node representing a IEEE1588 timer.
+
   fixed-link:
     oneOf:
       - $ref: /schemas/types.yaml#/definitions/uint32-array
-- 
2.25.1

Re: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[Richard Cochran]

On Thu, Oct 20, 2022 at 11:41:10PM -0600, Sarath Babu Naidu Gaddam wrote:
> There is currently no standard property to pass PTP device index
> information to ethernet driver when they are independent.
> 
> ptp-hardware-clock property will contain phandle to PTP clock node.
> 
> Freescale driver currently has this implementation but it will be
> good to agree on a generic (optional) property name to link to PTP
> phandle to Ethernet node. In future or any current ethernet driver
> wants to use this method of reading the PHC index,they can simply use
> this generic name and point their own PTP clock node, instead of
> creating separate property names in each ethernet driver DT node.
> 
> axiethernet driver uses this method when PTP support is integrated.
> 
> Example:
> 	fman0: fman@1a00000 {
> 		ptp-hardware-clock = <&ptp_timer0>;
> 	}
> 
> 	ptp_timer0: ptp-timer@1afe000 {
> 		compatible = "fsl,fman-ptp-timer";
> 		reg = <0x0 0x1afe000 0x0 0x1000>;
> 	}
> 
> Signed-off-by: Sarath Babu Naidu Gaddam <sarath.babu.naidu.gaddam@amd.com>

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[Krzysztof Kozlowski]

On 21/10/2022 01:41, Sarath Babu Naidu Gaddam wrote:
> There is currently no standard property to pass PTP device index
> information to ethernet driver when they are independent.
> 
> ptp-hardware-clock property will contain phandle to PTP clock node.
> 
> Freescale driver currently has this implementation but it will be
> good to agree on a generic (optional) property name to link to PTP
> phandle to Ethernet node. In future or any current ethernet driver
> wants to use this method of reading the PHC index,they can simply use
> this generic name and point their own PTP clock node, instead of
> creating separate property names in each ethernet driver DT node.
> 
> axiethernet driver uses this method when PTP support is integrated.
> 
> Example:
> 	fman0: fman@1a00000 {
> 		ptp-hardware-clock = <&ptp_timer0>;
> 	}
> 
> 	ptp_timer0: ptp-timer@1afe000 {
> 		compatible = "fsl,fman-ptp-timer";
> 		reg = <0x0 0x1afe000 0x0 0x1000>;
> 	}
> 
> Signed-off-by: Sarath Babu Naidu Gaddam <sarath.babu.naidu.gaddam@amd.com>
> ---
> We want binding to be reviewed/accepted and then make changes in freescale
> binding documentation to use this generic binding.

No, send entire set. We need to see the users of it.

> 
> DT information:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> tree/arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi#n23

Don't wrap links. It's not possible to click them...

> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> tree/Documentation/devicetree/bindings/net/fsl-fman.txt#n320
> 
> Freescale driver:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> tree/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c#n467
> 
> Changes in V2:
> 1) Changed the ptimer-handle to ptp-hardware-clock based on
>    Richard Cochran's comment.
> 2) Updated commit description.
> ---
>  .../devicetree/bindings/net/ethernet-controller.yaml         | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 3aef506fa158..d2863c1dd585 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -161,6 +161,11 @@ properties:
>        - auto
>        - in-band-status
>  
> +  ptp-hardware-clock:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Specifies a reference to a node representing a IEEE1588 timer.

Drop "Specifies a reference to". It's obvious from the schema.

Aren't you expecting here some specific Devicetree node of IEEE1588
timer? IOW, you expect to point to timer, but what this timer must
provide? How is this generic?

In your commit msg you use multiple times "driver", so are you adding it
only to satisfy Linux driver requirements? What about other drivers,
e.g. on BSD or U-Boot?

Best regards,
Krzysztof

Re: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[Rob Herring]

On Thu, Oct 20, 2022 at 11:41:10PM -0600, Sarath Babu Naidu Gaddam wrote:
> There is currently no standard property to pass PTP device index
> information to ethernet driver when they are independent.
> 
> ptp-hardware-clock property will contain phandle to PTP clock node.
> 
> Freescale driver currently has this implementation but it will be
> good to agree on a generic (optional) property name to link to PTP
> phandle to Ethernet node. In future or any current ethernet driver
> wants to use this method of reading the PHC index,they can simply use
> this generic name and point their own PTP clock node, instead of
> creating separate property names in each ethernet driver DT node.

Seems like this does the same thing as 
Documentation/devicetree/bindings/ptp/timestamper.txt.

Or perhaps what we have in bindings/timestamp/ which unfortunately does 
about the same thing.

The latter one is more flexible and follows standard provider/consumer 
patterns. So timestamper.txt should probably be deprecated.

> 
> axiethernet driver uses this method when PTP support is integrated.
> 
> Example:
> 	fman0: fman@1a00000 {
> 		ptp-hardware-clock = <&ptp_timer0>;
> 	}
> 
> 	ptp_timer0: ptp-timer@1afe000 {
> 		compatible = "fsl,fman-ptp-timer";
> 		reg = <0x0 0x1afe000 0x0 0x1000>;
> 	}
> 
> Signed-off-by: Sarath Babu Naidu Gaddam <sarath.babu.naidu.gaddam@amd.com>
> ---
> We want binding to be reviewed/accepted and then make changes in freescale
> binding documentation to use this generic binding.

If you want a common binding, I want to see multiple users and 
preferrably ones that have some differing requirements. It can be 
something existing with a 'this is what it would look like if we had 
used this new common binding'.

Rob

Re: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[Richard Cochran]

On Mon, Oct 24, 2022 at 11:57:23AM -0500, Rob Herring wrote:
> On Thu, Oct 20, 2022 at 11:41:10PM -0600, Sarath Babu Naidu Gaddam wrote:
> > There is currently no standard property to pass PTP device index
> > information to ethernet driver when they are independent.
> > 
> > ptp-hardware-clock property will contain phandle to PTP clock node.
> > 
> > Freescale driver currently has this implementation but it will be
> > good to agree on a generic (optional) property name to link to PTP
> > phandle to Ethernet node. In future or any current ethernet driver
> > wants to use this method of reading the PHC index,they can simply use
> > this generic name and point their own PTP clock node, instead of
> > creating separate property names in each ethernet driver DT node.
> 
> Seems like this does the same thing as 
> Documentation/devicetree/bindings/ptp/timestamper.txt.

That is different. It goes from:

   MAC -> time stamp generator

The proposed binding goes from:

  MAC (with built in time stamp generator) -> PTP Hardware Clock (with get/settime etc)


Thanks,
Richard

Re: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[Richard Cochran]

On Tue, Oct 25, 2022 at 02:46:08PM -0700, Richard Cochran wrote:
> On Mon, Oct 24, 2022 at 11:57:23AM -0500, Rob Herring wrote:
> > On Thu, Oct 20, 2022 at 11:41:10PM -0600, Sarath Babu Naidu Gaddam wrote:
> > > There is currently no standard property to pass PTP device index
> > > information to ethernet driver when they are independent.
> > > 
> > > ptp-hardware-clock property will contain phandle to PTP clock node.
> > > 
> > > Freescale driver currently has this implementation but it will be
> > > good to agree on a generic (optional) property name to link to PTP
> > > phandle to Ethernet node. In future or any current ethernet driver
> > > wants to use this method of reading the PHC index,they can simply use
> > > this generic name and point their own PTP clock node, instead of
> > > creating separate property names in each ethernet driver DT node.
> > 
> > Seems like this does the same thing as 
> > Documentation/devicetree/bindings/ptp/timestamper.txt.
> 
> That is different. It goes from:
> 
>    MAC -> time stamp generator

actually:
     PHY -> time stamp generator

> The proposed binding goes from:
> 
>   MAC (with built in time stamp generator) -> PTP Hardware Clock (with get/settime etc)
> 
> 
> Thanks,
> Richard

Re: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[Richard Cochran]

On Mon, Oct 24, 2022 at 11:57:23AM -0500, Rob Herring wrote:
> On Thu, Oct 20, 2022 at 11:41:10PM -0600, Sarath Babu Naidu Gaddam wrote:
> > There is currently no standard property to pass PTP device index
> > information to ethernet driver when they are independent.
> > 
> > ptp-hardware-clock property will contain phandle to PTP clock node.
> > 
> > Freescale driver currently has this implementation but it will be
> > good to agree on a generic (optional) property name to link to PTP
> > phandle to Ethernet node. In future or any current ethernet driver
> > wants to use this method of reading the PHC index,they can simply use
> > this generic name and point their own PTP clock node, instead of
> > creating separate property names in each ethernet driver DT node.
> 
> Seems like this does the same thing as 
> Documentation/devicetree/bindings/ptp/timestamper.txt.
> 
> Or perhaps what we have in bindings/timestamp/ which unfortunately does 
> about the same thing.
> 
> The latter one is more flexible and follows standard provider/consumer 
> patterns. So timestamper.txt should probably be deprecated.

I don't see how you can do that.  The provider/consumer semantics are
completely opposite.

The three (including present patch) bindings specify three different
relationships.

Thanks,
Richard

RE: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[Gaddam, Sarath Babu Naidu]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Sunday, October 23, 2022 9:12 PM
> To: Gaddam, Sarath Babu Naidu <sarath.babu.naidu.gaddam@amd.com>;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh+dt@kernel.org; richardcochran@gmail.com
> Cc: krzysztof.kozlowski+dt@linaro.org; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> yangbo.lu@nxp.com; Pandey, Radhey Shyam
> <radhey.shyam.pandey@amd.com>; Sarangi, Anirudha
> <anirudha.sarangi@amd.com>; Katakam, Harini
> <harini.katakam@amd.com>; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add
> ptp-hardware-clock
> 
> On 21/10/2022 01:41, Sarath Babu Naidu Gaddam wrote:
> > There is currently no standard property to pass PTP device index
> > information to ethernet driver when they are independent.
> >
> > ptp-hardware-clock property will contain phandle to PTP clock node.
> >
> > Freescale driver currently has this implementation but it will be good
> > to agree on a generic (optional) property name to link to PTP phandle
> > to Ethernet node. In future or any current ethernet driver wants to
> > use this method of reading the PHC index,they can simply use this
> > generic name and point their own PTP clock node, instead of creating
> > separate property names in each ethernet driver DT node.
> >
> > axiethernet driver uses this method when PTP support is integrated.
> >
> > Example:
> > 	fman0: fman@1a00000 {
> > 		ptp-hardware-clock = <&ptp_timer0>;
> > 	}
> >
> > 	ptp_timer0: ptp-timer@1afe000 {
> > 		compatible = "fsl,fman-ptp-timer";
> > 		reg = <0x0 0x1afe000 0x0 0x1000>;
> > 	}
> >
> > Signed-off-by: Sarath Babu Naidu Gaddam
> > <sarath.babu.naidu.gaddam@amd.com>
> > ---
> > We want binding to be reviewed/accepted and then make changes in
> > freescale binding documentation to use this generic binding.
> 
> No, send entire set. We need to see the users of it.
> 
> >
> > DT information:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> > tree/arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi#n23
> 
> Don't wrap links. It's not possible to click them...
> 
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> > tree/Documentation/devicetree/bindings/net/fsl-fman.txt#n320
> >
> > Freescale driver:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> > tree/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c#n467
> >
> > Changes in V2:
> > 1) Changed the ptimer-handle to ptp-hardware-clock based on
> >    Richard Cochran's comment.
> > 2) Updated commit description.
> > ---
> >  .../devicetree/bindings/net/ethernet-controller.yaml         | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > index 3aef506fa158..d2863c1dd585 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > @@ -161,6 +161,11 @@ properties:
> >        - auto
> >        - in-band-status
> >
> > +  ptp-hardware-clock:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Specifies a reference to a node representing a IEEE1588 timer.
> 
> Drop "Specifies a reference to". It's obvious from the schema.
> 
> Aren't you expecting here some specific Devicetree node of IEEE1588 timer?
> IOW, you expect to point to timer, but what this timer must provide? How is
> this generic?

Thanks for review comments.
 Format can be as documented by users Documentation/devicetree/bindings/ptp/ members. The node should be accessible to derive the index but the format of the PTP clock node is upto the vendor.


> 
> In your commit msg you use multiple times "driver", so are you adding it only
> to satisfy Linux driver requirements? What about other drivers, e.g. on BSD
> or U-Boot?

AFAIK this is for Linux. It is not relevant to uboot as there's no PTP support there.

Thanks,
Sarath

Re: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[Krzysztof Kozlowski]

On 10/11/2022 10:57, Gaddam, Sarath Babu Naidu wrote:
>>>
>>> +  ptp-hardware-clock:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      Specifies a reference to a node representing a IEEE1588 timer.
>>
>> Drop "Specifies a reference to". It's obvious from the schema.
>>
>> Aren't you expecting here some specific Devicetree node of IEEE1588 timer?
>> IOW, you expect to point to timer, but what this timer must provide? How is
>> this generic?
> 
> Thanks for review comments.
>  Format can be as documented by users Documentation/devicetree/bindings/ptp/ members. The node should be accessible to derive the index but the format of the PTP clock node is upto the vendor.

I am not sure what do you mean here. Anyway description might need
something more specific.

> 
> 
>>
>> In your commit msg you use multiple times "driver", so are you adding it only
>> to satisfy Linux driver requirements? What about other drivers, e.g. on BSD
>> or U-Boot?
> 
> AFAIK this is for Linux. It is not relevant to uboot as there's no PTP support there.

And BSD? Bindings are not for Linux only. Please abstract from any OS
specifics.

Also your messages needs wrapping. Use mailing list reply style.

Best regards,
Krzysztof

RE: [PATCH net-next V2] dt-bindings: net: ethernet-controller: Add ptp-hardware-clock

[Gaddam, Sarath Babu Naidu]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, November 10, 2022 7:36 PM
> To: Gaddam, Sarath Babu Naidu
> <sarath.babu.naidu.gaddam@amd.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; richardcochran@gmail.com
> Cc: krzysztof.kozlowski+dt@linaro.org; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> yangbo.lu@nxp.com; Pandey, Radhey Shyam
> <radhey.shyam.pandey@amd.com>; Sarangi, Anirudha
> <anirudha.sarangi@amd.com>; Katakam, Harini
> <harini.katakam@amd.com>; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH net-next V2] dt-bindings: net: ethernet-controller:
> Add ptp-hardware-clock
> 
> On 10/11/2022 10:57, Gaddam, Sarath Babu Naidu wrote:
> >>>
> >>> +  ptp-hardware-clock:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description:
> >>> +      Specifies a reference to a node representing a IEEE1588 timer.
> >>
> >> Drop "Specifies a reference to". It's obvious from the schema.
> >>
> >> Aren't you expecting here some specific Devicetree node of IEEE1588
> timer?
> >> IOW, you expect to point to timer, but what this timer must provide?
> >> How is this generic?
> >
> > Thanks for review comments.
> >  Format can be as documented by users
> Documentation/devicetree/bindings/ptp/ members. The node should be
> accessible to derive the index but the format of the PTP clock node is
> upto the vendor.
> 
> I am not sure what do you mean here. Anyway description might need
> something more specific.

Apologies for picking up on this thread after a long time.
PTP clock node(timer) is upto the vendor. Driver which needs a reference
to this node can be accessed by this new property. I will update the
description.

> >
> >
> >>
> >> In your commit msg you use multiple times "driver", so are you
> adding
> >> it only to satisfy Linux driver requirements? What about other
> >> drivers, e.g. on BSD or U-Boot?
> >
> > AFAIK this is for Linux. It is not relevant to uboot as there's no PTP
> support there.
> 
> And BSD? Bindings are not for Linux only. Please abstract from any OS
> specifics.

This new binding is a generic property. It can be used in other 
drivers also if they want to access the PTP device node in the 
current driver and It's an optional property.
 
I will change the commit description as below. If this is fine, I 
will send another version with updated commit description.
 
"There is currently no standard property to pass PTP device index  
information to ethernet driver when they are independent.
 
ptp-hardware-clock property will contain phandle to PTP clock node.

Its a generic (optional) property name to link to PTP phandle to  
Ethernet node. Any future or current ethernet drivers that need
a reference to the PHC used on their system can simply use this 
generic property name instead of using custom property 
implementation in their device tree nodes."

Thanks,
Sarath

> Also your messages needs wrapping. Use mailing list reply style.
> 
> Best regards,
> Krzysztof