Re: [PATCH net-next 01/12] dt-bindings: ptp: add bindings for NETC Timer

[Krzysztof Kozlowski <krzk@kernel.org>]

On 14/07/2025 09:32, Wei Fang wrote:
>> On 11/07/2025 08:57, Wei Fang wrote:
>>> Add device tree binding doc for the PTP clock based on NETC Timer.
>>
>>
>> A nit, subject: drop second/last, redundant "bindings for". The "dt-bindings"
>> prefix is already stating that these are bindings.
> 
> Okay, I will fix it, thanks
> 
>>> +
>>> +title: NXP NETC Timer PTP clock
>>
>> What is NETC?
>>
> 
> NETC means Ethernet Controller, it is a multi-function PCIe Root Complex
> Integrated Endpoint (RCiEP), Timer is one of its PCIe functions.

This must be explained in description, not here.

> 
>>> +
>>> +description:
>>> +  NETC 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@nxp.com>
>>> +  - Clark Wang <xiaoning.wang@nxp.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - pci1131,ee02
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  clock-names:
>>> +    oneOf:
>>
>> Why oneOf? Drop
>>
>>> +      - enum:
>>> +          - system
>>> +          - ccm_timer
>>> +          - ext_1588
>>
>> Why is this flexible?
>>
> 
> The NETC Timer has 3 reference clock sources, we need to select one
> of them as the reference clock. Set TMR_CTRL[CK_SEL] by parsing the
> clock name to tell the hardware which clock is currently being used.
> Otherwise, we need to add another property to select the clock source.
> 
>>> +
>>> +  nxp,pps-channel:
>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>> +    default: 0
>>> +    description:
>>> +      Specifies to which fixed interval period pulse generator is
>>> +      used to generate PPS signal.
>>> +    enum: [0, 1, 2]
>>
>> Cell phandle tells that. Drop property.
> 
> Sorry, I do not understand what you mean, could you explain it in more
> detail?

Use phandle cells for that - look at other PTP bindings.

> 
> This property is similar to the "fsl,ptp-channel" which has been added
> to fsl,fec.yaml
> https://elixir.bootlin.com/linux/v6.16-rc5/source/Documentation/devicetree/bindings/net/fsl,fec.yaml#L186

This is not FEC. And if you have existing property, you don't add
entirely different one!

> 
> The NETC Timer has three 3 PPS generators, and each corresponding
> to an output pin, but these pins are multiplexed with other devices,
> so the pin of the PPS output depends on the design of the board. This
> property is used to tell the driver which PPS generator/pin the board
> uses.


Again, phandle cells tells you that. And driver does not use it in the
first place.


Best regards,
Krzysztof