Message-ID: <aJ94NL34kpdMKaH6@lizhi-Precision-Tower-5810>
Date: Fri, 15 Aug 2025 14:11:00 -0400
From: Frank Li <Frank.li@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Wei Fang <wei.fang@....com>, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, richardcochran@...il.com,
	claudiu.manoil@....com, vladimir.oltean@....com,
	xiaoning.wang@....com, andrew+netdev@...n.ch, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	vadim.fedorenko@...ux.dev, shawnguo@...nel.org,
	s.hauer@...gutronix.de, festevam@...il.com, fushi.peng@....com,
	devicetree@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, imx@...ts.linux.dev,
	kernel@...gutronix.de
Subject: Re: [PATCH v3 net-next 01/15] dt-bindings: ptp: add NETC Timer PTP
 clock

On Fri, Aug 15, 2025 at 08:05:12AM +0200, Krzysztof Kozlowski wrote:
> On 14/08/2025 20:50, Frank Li wrote:
> > On Thu, Aug 14, 2025 at 10:25:14AM +0200, Krzysztof Kozlowski wrote:
> >> On Tue, Aug 12, 2025 at 05:46:20PM +0800, Wei Fang wrote:
> >>> NXP NETC (Ethernet Controller) is a multi-function PCIe Root Complex
> >>> Integrated Endpoint (RCiEP), the Timer is one of its functions which
> >>> provides current time with nanosecond resolution, precise periodic
> >>> pulse, pulse on timeout (alarm), and time capture on external pulse
> >>> support. And also supports time synchronization as required for IEEE
> >>> 1588 and IEEE 802.1AS-2020. So add device tree binding doc for the
> >>> PTP clock based on NETC Timer.
> >>>
> >>> Signed-off-by: Wei Fang <wei.fang@....com>
> >>>
> >>> ---
> >>> v2 changes:
> >>> 1. Refine the subject and the commit message
> >>> 2. Remove "nxp,pps-channel"
> >>> 3. Add description to "clocks" and "clock-names"
> >>> v3 changes:
> >>> 1. Remove the "system" clock from clock-names
> >>> ---
> >>>  .../devicetree/bindings/ptp/nxp,ptp-netc.yaml | 63 +++++++++++++++++++
> >>>  1 file changed, 63 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/ptp/nxp,ptp-netc.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/ptp/nxp,ptp-netc.yaml b/Documentation/devicetree/bindings/ptp/nxp,ptp-netc.yaml
> >>> new file mode 100644
> >>> index 000000000000..60fb2513fd76
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/ptp/nxp,ptp-netc.yaml
> >>> @@ -0,0 +1,63 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/ptp/nxp,ptp-netc.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: NXP NETC V4 Timer PTP clock
> >>> +
> >>> +description:
> >>> +  NETC V4 Timer provides current time with nanosecond resolution, precise
> >>> +  periodic pulse, pulse on timeout (alarm), and time capture on external
> >>> +  pulse support. And it supports time synchronization as required for
> >>> +  IEEE 1588 and IEEE 802.1AS-2020.
> >>> +
> >>> +maintainers:
> >>> +  - Wei Fang <wei.fang@....com>
> >>> +  - Clark Wang <xiaoning.wang@....com>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - pci1131,ee02
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  clocks:
> >>> +    maxItems: 1
> >>> +    description:
> >>> +      The reference clock of NETC Timer, if not present, indicates that
> >>> +      the system clock of NETC IP is selected as the reference clock.
> >>> +
> >>> +  clock-names:
> >>> +    description:
> >>> +      The "ccm_timer" means the reference clock comes from CCM of SoC.
> >>> +      The "ext_1588" means the reference clock comes from external IO
> >>> +      pins.
> >>> +    enum:
> >>> +      - ccm_timer
> >>
> >> You should name here how the input pin is called, not the source. Pin is
> >> "ref"?
> >>
> >>> +      - ext_1588
> >>
> >> This should be just "ext"? We probably talked about this, but this feels
> >> like you describe one input in different ways.
> >>
> >> You will get the same questions in the future, if commit msg does not
> >> reflect previous talks.
> >>
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +
> >>> +allOf:
> >>> +  - $ref: /schemas/pci/pci-device.yaml
> >>> +
> >>> +unevaluatedProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    pcie {
> >>> +        #address-cells = <3>;
> >>> +        #size-cells = <2>;
> >>> +
> >>> +        ethernet@18,0 {
> >>
> >> That's rather timer or ptp-timer or your binding is incorrect. Please
> >> describe COMPLETE device in your binding.
> >
> > Krzysztof:
> >
> > 	I have question about "COMPLETE" here. For some MFD/syscon, I know
> > need descript all children nodes to make MFD/syscon complete.
> >
> > 	But here it is PCIe device.
> >
> > pcie_4ca00000: pcie@...00000 {
> > 	compatible = "pci-host-ecam-generic";
> > 	...
> >
> > 	enetc_port0: ethernet@0,0 {
> >         	compatible = "fsl,imx95-enetc", "...";
> > 		...
> >
> > 	ptp-timer@18,0 {
> > 		compatible = "pci1131,ee02";
> > 	}
> > };
> >
> > 	parent "pci-host-ecam-generic" is common pci binding, each children
> > is indepentant part.
> >
> > 	I am not sure how to decript COMPLETE device for PCI devices.
>
> I don't know what is missing here, but naming it ethernet suggested
> there are other functions not being described in the binding.

name 'ethernet@18,0' is wrong, which should be fixed as 'ptp-timer@18,0'.

which is independence device. This file descript its's bindings.

Other embedded pcie devices located in ENETC submodule is descript by other
binding files.

Such as MIDO: Documentation/devicetree/bindings/net/fsl,enetc-mdio.yaml
NIC: Documentation/devicetree/bindings/net/fsl,enetc.yaml

For ptp-timer, I think it should be complete enough. Do we need descirpt
its's consumer? An example

https://lore.kernel.org/imx/20250812094634.489901-4-wei.fang@nxp.com/

+    pcie {
+      #address-cells = <3>;
+      #size-cells = <2>;
+
+      ethernet@0,0 {
+          compatible = "pci1131,e101";
+          reg = <0x000000 0 0 0 0>;
+          clocks = <&scmi_clk 102>;
+          clock-names = "ref";
+          phy-handle = <&ethphy0>;
+          phy-mode = "rgmii-id";
+          ptp-timer = <&netc_timer>;

This phandle point to ptp-timer@18,0

+      };
+    };

Frank
>
>
>
> Best regards,
> Krzysztof

Powered by blists - more mailing lists