Re: [PATCH 15/28] dt-bindings: dpll: Add device tree bindings for DPLL device and pin

Ivan Vecera
2025-04-09T07:09:28Z

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@redhat.com>
>
> Did this really happen?
>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.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@redhat.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@redhat.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
>