| Message ID | 20250407173149.1010216-6-ivecera@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add Microchip ZL3073x support | expand |
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 > 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. > > 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. > > 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. > > 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. > + 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? > + > + num-dplls: > + description: Number of DPLL channels in this device. Why this is not deducible from compatible? > + $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? > + > + 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. > + > + properties: > + "#address-cells": > + const: 1 Why? > + "#size-cells": > + const: 0 Why? I don't see these being used. > + > + 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. > + > + type: > + description: Type of the pin > + $ref: /schemas/types.yaml#/definitions/string > + enum: [ext, gnss, int, mux, synce] > + > + Just one blank line 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
On 07/04/2025 20:01, Krzysztof Kozlowski wrote: >> + >> + 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. > >> + >> + properties: >> + "#address-cells": >> + const: 1 > > Why? Ah, I see being used. > >> + "#size-cells": >> + const: 0 > > Why? I don't see these being used. And this as well. > >> + >> + patternProperties: >> + "^pin@[0-9]+$": >> + $ref: /schemas/dpll/dpll-pin.yaml >> + unevaluatedProperties: false >> + Best regards, Krzysztof
On Mon, Apr 7, 2025 at 8:02 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 07/04/2025 19:31, Ivan Vecera wrote: > > This adds DT bindings schema for DPLL (device phase-locked loop) > > Reviewed-by: Michal Schmidt <mschmidt@redhat.com> > > Did this really happen? We have been through several iterations of the patchset internally. I pointed out numerous bugs in the driver code and suggested cleanups. However, I know very little about DT, so in v2 we should omit my Reviewed-by on the DT patches. Michal
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 >
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: | + 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 + + num-dplls: + description: Number of DPLL channels in this device. + $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] + + input-pins: + type: object + description: DPLL input 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" + + 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 + + type: + description: Type of the pin + $ref: /schemas/types.yaml#/definitions/string + enum: [ext, gnss, int, mux, synce] + + +required: + - reg + +additionalProperties: false diff --git a/MAINTAINERS b/MAINTAINERS index 3d542440d0b2b..eaf2576a9b746 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7194,6 +7194,8 @@ M: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> M: Jiri Pirko <jiri@resnulli.us> L: netdev@vger.kernel.org S: Supported +F: Documentation/devicetree/bindings/dpll/dpll-device.yaml +F: Documentation/devicetree/bindings/dpll/dpll-pin.yaml F: Documentation/driver-api/dpll.rst F: drivers/dpll/* F: include/linux/dpll.h
This adds DT bindings schema for DPLL (device phase-locked loop) device and associated pin. The schema follows existing DPLL core API 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. 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. 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 Reviewed-by: Michal Schmidt <mschmidt@redhat.com> 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