Re: [PATCH 15/28] dt-bindings: dpll: Add device tree bindings for DPLL device and pin
From: Ivan Vecera
Date: Wed Apr 09 2025 - 03:09:51 EST
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@xxxxxxxxxx>
Did this really happen?
Signed-off-by: Ivan Vecera <ivecera@xxxxxxxxxx>
---
.../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@xxxxxxxxxx>
+
+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@xxxxxxxxxx>
+
+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