Message-ID: <5bd6c5d4-4ca1-4e53-b073-717a8e8295c6@redhat.com>
Date: Wed, 9 Apr 2025 09:09:15 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, netdev@...r.kernel.org
Cc: Michal Schmidt <mschmidt@...hat.com>,
 Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
 Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
 Jiri Pirko <jiri@...nulli.us>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Prathosh Satish <Prathosh.Satish@...rochip.com>,
 Lee Jones <lee@...nel.org>, Kees Cook <kees@...nel.org>,
 Andy Shevchenko <andy@...nel.org>, Andrew Morton
 <akpm@...ux-foundation.org>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH 15/28] dt-bindings: dpll: Add device tree bindings for
 DPLL device and pin

On 07. 04. 25 8:01 odp., Krzysztof Kozlowski wrote:
> On 07/04/2025 19:31, Ivan Vecera wrote:
>> This adds DT bindings schema for DPLL (device phase-locked loop)
> 
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> 
> A nit, subject: drop second/last, redundant "device tree bindings for".
> The "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Will fix this in v2.

>> device and associated pin. The schema follows existing DPLL core API
> 
> What is core API in terms of Devicetree?
> 
>> and should be used to expose information that should be provided
>> by platform firmware.
>>
>> The schema for DPLL device describe a DPLL chip that can contain
>> one or more DPLLs (channels) and platform can specify their types.
>> For now 'pps' and 'eec' types supported and these values are mapped
>> to DPLL core's enums.
> 
> Describe entire hardware, not what is supported.

Ack

>>
>> The DPLL device can have optionally 'input-pins' and 'output-pins'
>> sub-nodes that contain pin sub-nodes.
>>
>> These pin sub-nodes follows schema for dpll-pin and can contain
>> information about the particular pin.
> 
> Describe the hardware, not the schema. We can read the contents of
> patch. What we cannot read is the hardware and why you are making all
> these choices.

OK

>>
>> The pin contains the following properties:
>> * reg - pin HW index (physical pin number of given type)
>> * label - string that is used as board label by DPLL core
>> * type - string that indicates pin type (mapped to DPLL core pin type)
>> * esync-control - boolean that indicates whether embeddded sync control
>>                    is allowed for this pin
>> * supported-frequencies - list of 64bit values that represents frequencies
>>                            that are allowed to be configured for the pin
> 
> Drop. Describe the hardware.
> 
> 
>>
>> Reviewed-by: Michal Schmidt <mschmidt@...hat.com>
> 
> Did this really happen?
> 
>> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
>> ---
>>   .../devicetree/bindings/dpll/dpll-device.yaml | 84 +++++++++++++++++++
>>   .../devicetree/bindings/dpll/dpll-pin.yaml    | 43 ++++++++++
>>   MAINTAINERS                                   |  2 +
>>   3 files changed, 129 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/dpll/dpll-device.yaml
>>   create mode 100644 Documentation/devicetree/bindings/dpll/dpll-pin.yaml
> 
> Filenames matching compatibles... unless this is common schema, but
> commit description did not mention it.

Yes, this is common schema to describe a common properties of DPLL 
device that is inherited by a concrete HW implementation (next patch).

>>
>> diff --git a/Documentation/devicetree/bindings/dpll/dpll-device.yaml b/Documentation/devicetree/bindings/dpll/dpll-device.yaml
>> new file mode 100644
>> index 0000000000000..e6c309abb857f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dpll/dpll-device.yaml
>> @@ -0,0 +1,84 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/dpll/dpll-device.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Digital Phase-Locked Loop (DPLL) Device
>> +
>> +maintainers:
>> +  - Ivan Vecera <ivecera@...hat.com>
>> +
>> +description: |
> 
> Do not need '|' unless you need to preserve formatting.

OK

>> +  Digital Phase-Locked Loop (DPLL) device are used for precise clock
>> +  synchronization in networking and telecom hardware. The device can
>> +  have one or more channels (DPLLs) and one or more input and output
>> +  pins. Each DPLL channel can either produce pulse-per-clock signal
>> +  or drive ethernet equipment clock. The type of each channel is
>> +  indicated by dpll-types property.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^dpll(@.*)?$"
>> +
>> +  "#address-cells":
>> +    const: 0
>> +
>> +  "#size-cells":
>> +    const: 0
> 
> Why do you need these cells?

There are 'input-pins' and 'output-pins' sub-nodes that do not use '@' 
suffix and 'reg' property. They can be specified only once so address 
nor size do not make sense.

>> +
>> +  num-dplls:
>> +    description: Number of DPLL channels in this device.
> 
> Why this is not deducible from compatible?

Yes, it is. Concrete HW implementation should know this.
Will drop.

>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +
>> +  dpll-types:
>> +    description: List of DPLL types, one per DPLL instance.
>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>> +    items:
>> +      enum: [pps, eec]
> 
> Why this is not deducible from compatible?

The compatible does not specify how the DPLL channels are used. 
Particular hardware schematics/wiring specify the type of the channel usage.

>> +
>> +  input-pins:
>> +    type: object
>> +    description: DPLL input pins
>> +    unevaluatedProperties: false
> 
> So this is all for pinctrl? Or something else? Could not figure out from
> commit msg. This does not help me either.

No, these pins have nothing to do with pinctrl.
Will describe in next version.

>> +
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
> 
> Why?
> 
>> +      "#size-cells":
>> +        const: 0
> 
> Why? I don't see these being used.

The pin has '@' suffix and 'reg' property that specifies the HW index of 
the pin. (e.g pin@3 under output-pins is the 3rd physical output pin).

>> +
>> +    patternProperties:
>> +      "^pin@[0-9]+$":
>> +        $ref: /schemas/dpll/dpll-pin.yaml
>> +        unevaluatedProperties: false
>> +
>> +    required:
>> +      - "#address-cells"
>> +      - "#size-cells"
>> +
>> +  output-pins:
>> +    type: object
>> +    description: DPLL output pins
>> +    unevaluatedProperties: false
>> +
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +      "#size-cells":
>> +        const: 0
>> +
>> +    patternProperties:
>> +      "^pin@[0-9]+$":
>> +        $ref: /schemas/dpll/dpll-pin.yaml
>> +        unevaluatedProperties: false
>> +
>> +    required:
>> +      - "#address-cells"
>> +      - "#size-cells"
>> +
>> +dependentRequired:
>> +  dpll-types: [ num-dplls ]
>> +
>> +additionalProperties: true
>> diff --git a/Documentation/devicetree/bindings/dpll/dpll-pin.yaml b/Documentation/devicetree/bindings/dpll/dpll-pin.yaml
>> new file mode 100644
>> index 0000000000000..9aea8ceabb5af
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dpll/dpll-pin.yaml
>> @@ -0,0 +1,43 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/dpll/dpll-pin.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: DPLL Pin
>> +
>> +maintainers:
>> +  - Ivan Vecera <ivecera@...hat.com>
>> +
>> +description: |
>> +  Schema for defining input and output pins of a Digital Phase-Locked Loop (DPLL).
>> +  Each pin can have a set of supported frequencies, label, type and may support
>> +  embedded sync.
>> +
>> +properties:
>> +  reg:
>> +    description: Hardware index of the pin.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  esync-control:
>> +    description: Indicates whether the pin supports embedded sync functionality.
>> +    type: boolean
>> +
>> +  label:
>> +    description: String exposed as the pin board label
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +
>> +  supported-frequencies:
>> +    description: List of supported frequencies for this pin, expressed in Hz.
>> +    $ref: /schemas/types.yaml#/definitions/uint64-array
> 
> Use common property suffixes and drop ref.

Should I use 'supported-frequencies-hz'? If so... This property unit 
type is specified [1] as uint32-matrix, can I use this for list of 
uint64 numbers?

[1] 
https://github.com/devicetree-org/dt-schema/blob/dd3e3dce83607661f2831a8fac9112fae5ebe6cd/dtschema/schemas/property-units.yaml#L56

>> +
>> +  type:
>> +    description: Type of the pin
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    enum: [ext, gnss, int, mux, synce]
>> +
>> +
> 
> Just one blank line

Ack

> I bet that half of my questions could be answered with proper hardware
> description which is missing in commit msg and binding description.
> Instead your commit msg explains schema which makes no sense - I
> mentioned, we can read the schema.
>> +required:
>> +  - reg
> Best regards,
> Krzysztof
> 

Powered by blists - more mailing lists