| Message ID | 20240709123121.1452394-2-heiko@sntech.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Binding and driver for "dumb" clock generators | expand |
Quoting Heiko Stuebner (2024-07-09 05:31:16) > In contrast to fixed clocks that are described as ungateable, boards > sometimes use additional clock generators for things like PCIe reference > clocks, that need actual supplies to get enabled and enable-gpios to be > toggled for them to work. > > This adds a binding for such clock generators that are not configurable > themself, but need to handle supplies for them to work. Sounds like vdd-supply is required? > > While in a lot of cases the type of the IC used is described in board > schematics, in some cases just a generic type description like > "100MHz, 3.3V" might also be used. The binding therefore allows both > cases. Specifying the type is of course preferred. > > The clock-frequency is set in devicetree, because while some clock > generators have pins to decide between multipls output rates, those > are generally set statically on the board-layout-level. > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > .../bindings/clock/clock-generator.yaml | 62 +++++++++++++++++++ > 1 file changed, 62 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/clock-generator.yaml > > diff --git a/Documentation/devicetree/bindings/clock/clock-generator.yaml b/Documentation/devicetree/bindings/clock/clock-generator.yaml > new file mode 100644 > index 0000000000000..f44e61e414e89 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/clock-generator.yaml > @@ -0,0 +1,62 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/clock-generator.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Simple clock generators > + > +maintainers: > + - Heiko Stuebner <heiko@sntech.de> > + > +properties: > + $nodename: > + anyOf: > + - description: > + Preferred name is 'clock-<freq>' with <freq> being the output > + frequency as defined in the 'clock-frequency' property. > + pattern: "^clock-([0-9]+|[a-z0-9-]+)$" > + - description: Any name allowed > + deprecated: true Drop the deprecated stuff from the fixed-clock binding? > + > + compatible: > + oneOf: > + - const: clock-generator > + - items: > + - enum: > + - diodes,pi6c557-03b I see this datasheet[1]. Can that link be included somewhere? That shows there's a clock input pin, which means I expect a 'clocks' property. Maybe instead of creating a generic binding just make a binding for these diodes parts? It certainly looks like a generic binding could come later when another vendor supports the same binding. > + - diodes,pi6c557-05b > + - const: clock-generator > + > + "#clock-cells": > + const: 0 > + > + clock-frequency: true > + > + clock-output-names: > + maxItems: 1 > + > + enable-gpios: > + description: > + Contains a single GPIO specifier for the GPIO that enables and disables > + the clock generator. > + maxItems: 1 > + > + vdd-supply: > + description: handle of the regulator that provides the supply voltage > + > +required: > + - compatible > + - "#clock-cells" > + - clock-frequency This is the same required properties as fixed rate clocks. I'd guess that at least 'enable-gpios' or 'vdd-supply' should also be required, or the node would simply use fixed-clock compatible. > + > +additionalProperties: false > + > +examples: > + - | > + clock { > + compatible = "clock-generator"; > + #clock-cells = <0>; > + clock-frequency = <1000000000>; > + }; [1] https://diodes.com/assets/Datasheets/PI6C557-03B.pdf
Hello Heiko, Am Dienstag, 9. Juli 2024, 14:31:16 CEST schrieb Heiko Stuebner: > In contrast to fixed clocks that are described as ungateable, boards > sometimes use additional clock generators for things like PCIe reference > clocks, that need actual supplies to get enabled and enable-gpios to be > toggled for them to work. Fixed clocks are intended to be ungateable? Where does this come from? > This adds a binding for such clock generators that are not configurable > themself, but need to handle supplies for them to work. > > While in a lot of cases the type of the IC used is described in board > schematics, in some cases just a generic type description like > "100MHz, 3.3V" might also be used. The binding therefore allows both > cases. Specifying the type is of course preferred. > > The clock-frequency is set in devicetree, because while some clock > generators have pins to decide between multipls output rates, those > are generally set statically on the board-layout-level. > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > .../bindings/clock/clock-generator.yaml | 62 +++++++++++++++++++ > 1 file changed, 62 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/clock-generator.yaml > > diff --git a/Documentation/devicetree/bindings/clock/clock-generator.yaml b/Documentation/devicetree/bindings/clock/clock-generator.yaml > new file mode 100644 > index 0000000000000..f44e61e414e89 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/clock-generator.yaml > @@ -0,0 +1,62 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/clock-generator.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Simple clock generators > + > +maintainers: > + - Heiko Stuebner <heiko@sntech.de> > + > +properties: > + $nodename: > + anyOf: > + - description: > + Preferred name is 'clock-<freq>' with <freq> being the output > + frequency as defined in the 'clock-frequency' property. > + pattern: "^clock-([0-9]+|[a-z0-9-]+)$" > + - description: Any name allowed > + deprecated: true > + > + compatible: > + oneOf: > + - const: clock-generator > + - items: > + - enum: > + - diodes,pi6c557-03b > + - diodes,pi6c557-05b > + - const: clock-generator > + > + "#clock-cells": > + const: 0 > + > + clock-frequency: true > + > + clock-output-names: > + maxItems: 1 > + > + enable-gpios: > + description: > + Contains a single GPIO specifier for the GPIO that enables and disables > + the clock generator. > + maxItems: 1 > + > + vdd-supply: > + description: handle of the regulator that provides the supply voltage So essentially only enable-gpios and vdd-supply is added in comparison to fixed-clock. Does it make sense to add that to the fixed-clocks instead? Similar to fixed-regulator. > + > +required: > + - compatible > + - "#clock-cells" > + - clock-frequency With this list it's essentially the same as fixed-clock. Best regards, Alexander > + > +additionalProperties: false > + > +examples: > + - | > + clock { > + compatible = "clock-generator"; > + #clock-cells = <0>; > + clock-frequency = <1000000000>; > + }; > +... >
Hi, Am Mittwoch, 10. Juli 2024, 09:02:34 CEST schrieb Alexander Stein: > Am Dienstag, 9. Juli 2024, 14:31:16 CEST schrieb Heiko Stuebner: > > In contrast to fixed clocks that are described as ungateable, boards > > sometimes use additional clock generators for things like PCIe reference > > clocks, that need actual supplies to get enabled and enable-gpios to be > > toggled for them to work. > > Fixed clocks are intended to be ungateable? Where does this come from? "DOC: basic fixed-rate clock that cannot gate" https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-fixed-rate.c#n18 > > This adds a binding for such clock generators that are not configurable > > themself, but need to handle supplies for them to work. > > > > While in a lot of cases the type of the IC used is described in board > > schematics, in some cases just a generic type description like > > "100MHz, 3.3V" might also be used. The binding therefore allows both > > cases. Specifying the type is of course preferred. > > > > The clock-frequency is set in devicetree, because while some clock > > generators have pins to decide between multipls output rates, those > > are generally set statically on the board-layout-level. > > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > --- > > .../bindings/clock/clock-generator.yaml | 62 +++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/clock/clock-generator.yaml > > > > diff --git a/Documentation/devicetree/bindings/clock/clock-generator.yaml b/Documentation/devicetree/bindings/clock/clock-generator.yaml > > new file mode 100644 > > index 0000000000000..f44e61e414e89 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/clock/clock-generator.yaml > > @@ -0,0 +1,62 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/clock/clock-generator.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Simple clock generators > > + > > +maintainers: > > + - Heiko Stuebner <heiko@sntech.de> > > + > > +properties: > > + $nodename: > > + anyOf: > > + - description: > > + Preferred name is 'clock-<freq>' with <freq> being the output > > + frequency as defined in the 'clock-frequency' property. > > + pattern: "^clock-([0-9]+|[a-z0-9-]+)$" > > + - description: Any name allowed > > + deprecated: true > > + > > + compatible: > > + oneOf: > > + - const: clock-generator > > + - items: > > + - enum: > > + - diodes,pi6c557-03b > > + - diodes,pi6c557-05b > > + - const: clock-generator > > + > > + "#clock-cells": > > + const: 0 > > + > > + clock-frequency: true > > + > > + clock-output-names: > > + maxItems: 1 > > + > > + enable-gpios: > > + description: > > + Contains a single GPIO specifier for the GPIO that enables and disables > > + the clock generator. > > + maxItems: 1 > > + > > + vdd-supply: > > + description: handle of the regulator that provides the supply voltage > > So essentially only enable-gpios and vdd-supply is added in comparison to > fixed-clock. Does it make sense to add that to the fixed-clocks instead? > Similar to fixed-regulator. I wasn't that sure which way to go in the first place. The deciding point was reading that line about the fixed clock not being gateable, so I opted to not touch the fixed-clock. But you're definitly right, this _could_ live inside the fixed-clock as well, if we decide to get rid of the not-gateable thing above. Heiko
Hi Stephen, Am Dienstag, 9. Juli 2024, 23:45:20 CEST schrieb Stephen Boyd: > Quoting Heiko Stuebner (2024-07-09 05:31:16) > > In contrast to fixed clocks that are described as ungateable, boards > > sometimes use additional clock generators for things like PCIe reference > > clocks, that need actual supplies to get enabled and enable-gpios to be > > toggled for them to work. > > > > This adds a binding for such clock generators that are not configurable > > themself, but need to handle supplies for them to work. > > Sounds like vdd-supply is required? yeah, I guess this could move to required > > > > While in a lot of cases the type of the IC used is described in board > > schematics, in some cases just a generic type description like > > "100MHz, 3.3V" might also be used. The binding therefore allows both > > cases. Specifying the type is of course preferred. > > > > The clock-frequency is set in devicetree, because while some clock > > generators have pins to decide between multipls output rates, those > > are generally set statically on the board-layout-level. > > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > --- > > .../bindings/clock/clock-generator.yaml | 62 +++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/clock/clock-generator.yaml > > > > diff --git a/Documentation/devicetree/bindings/clock/clock-generator.yaml b/Documentation/devicetree/bindings/clock/clock-generator.yaml > > new file mode 100644 > > index 0000000000000..f44e61e414e89 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/clock/clock-generator.yaml > > @@ -0,0 +1,62 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/clock/clock-generator.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Simple clock generators > > + > > +maintainers: > > + - Heiko Stuebner <heiko@sntech.de> > > + > > +properties: > > + $nodename: > > + anyOf: > > + - description: > > + Preferred name is 'clock-<freq>' with <freq> being the output > > + frequency as defined in the 'clock-frequency' property. > > + pattern: "^clock-([0-9]+|[a-z0-9-]+)$" > > + - description: Any name allowed > > + deprecated: true > > Drop the deprecated stuff from the fixed-clock binding? ok > > > + > > + compatible: > > + oneOf: > > + - const: clock-generator > > + - items: > > + - enum: > > + - diodes,pi6c557-03b > > I see this datasheet[1]. Can that link be included somewhere? That shows > there's a clock input pin, which means I expect a 'clocks' property. ok, I guess we could add a clock property and make it mandatory for the Diodes compatibles. > Maybe instead of creating a generic binding just make a binding for > these diodes parts? It certainly looks like a generic binding could come > later when another vendor supports the same binding. I was actually primarily aiming at solving the Rock 5 ITX clock generator issue described in patch 5, where the 100 MHz clock generator is just described as "100MHz,3.3V,3225" in the schematics, but definitly needs the supply regulator to be enabled [1]. The Theobroma boards were addons because I felt they could also describe the hardware better instead of lumping together 2 existing bindings. > > + - diodes,pi6c557-05b > > + - const: clock-generator > > + > > + "#clock-cells": > > + const: 0 > > + > > + clock-frequency: true > > + > > + clock-output-names: > > + maxItems: 1 > > + > > + enable-gpios: > > + description: > > + Contains a single GPIO specifier for the GPIO that enables and disables > > + the clock generator. > > + maxItems: 1 > > + > > + vdd-supply: > > + description: handle of the regulator that provides the supply voltage > > + > > +required: > > + - compatible > > + - "#clock-cells" > > + - clock-frequency > > This is the same required properties as fixed rate clocks. I'd guess > that at least 'enable-gpios' or 'vdd-supply' should also be required, or > the node would simply use fixed-clock compatible. ok, as said above, having the vdd-supply as requirement does make sense. Heiko [1] https://dl.radxa.com/rock5/5itx/radxa_rock_5_itx_X1100_schematic.pdf page 35, bottom left, to the left of the big Au5426 "clock buffer".
Quoting Heiko Stübner (2024-07-10 00:45:17) > Am Mittwoch, 10. Juli 2024, 09:02:34 CEST schrieb Alexander Stein: > > > > So essentially only enable-gpios and vdd-supply is added in comparison to > > fixed-clock. Does it make sense to add that to the fixed-clocks instead? > > Similar to fixed-regulator. > > I wasn't that sure which way to go in the first place. > The deciding point was reading that line about the fixed clock not > being gateable, so I opted to not touch the fixed-clock. > > But you're definitly right, this _could_ live inside the fixed-clock > as well, if we decide to get rid of the not-gateable thing above. It's probably more complicated to combine it with the fixed-clock binding after making properties required like vdd-supply. I'd just make a new binding and look at combining later.
Quoting Heiko Stübner (2024-07-10 01:02:57) > Am Dienstag, 9. Juli 2024, 23:45:20 CEST schrieb Stephen Boyd: > > Quoting Heiko Stuebner (2024-07-09 05:31:16) > > > diff --git a/Documentation/devicetree/bindings/clock/clock-generator.yaml b/Documentation/devicetree/bindings/clock/clock-generator.yaml > > > new file mode 100644 > > > index 0000000000000..f44e61e414e89 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/clock/clock-generator.yaml > > > @@ -0,0 +1,62 @@ [...] > > > > Maybe instead of creating a generic binding just make a binding for > > these diodes parts? It certainly looks like a generic binding could come > > later when another vendor supports the same binding. > > I was actually primarily aiming at solving the Rock 5 ITX clock generator > issue described in patch 5, where the 100 MHz clock generator is just > described as "100MHz,3.3V,3225" in the schematics, but definitly needs > the supply regulator to be enabled [1]. That looks like a VCO (voltage controlled oscillator). Maybe the compatible string can be "voltage-oscillator" or "clock-vco" and it can require the vdd-supply. Those diodes parts look different. They look like PLLs that have a reference clock, hence the 'clocks' property I was expecting to see. That would use the VCO you have to make the PCIE reference clk frequencies. A generic compatible for those diodes parts is likely "phase-locked-loop", or "clock-pll", but I'd avoid that given that PLLs are almost always complicated and can have multiple output frequencies if they have post and pre-dividers, etc. It's easier to be specific here and make a binding for the part you have.
Am Donnerstag, 11. Juli 2024, 01:21:15 CEST schrieb Stephen Boyd: > Quoting Heiko Stübner (2024-07-10 00:45:17) > > Am Mittwoch, 10. Juli 2024, 09:02:34 CEST schrieb Alexander Stein: > > > > > > So essentially only enable-gpios and vdd-supply is added in comparison to > > > fixed-clock. Does it make sense to add that to the fixed-clocks instead? > > > Similar to fixed-regulator. > > > > I wasn't that sure which way to go in the first place. > > The deciding point was reading that line about the fixed clock not > > being gateable, so I opted to not touch the fixed-clock. > > > > But you're definitly right, this _could_ live inside the fixed-clock > > as well, if we decide to get rid of the not-gateable thing above. > > It's probably more complicated to combine it with the fixed-clock > binding after making properties required like vdd-supply. I'd just make > a new binding and look at combining later. Maybe I am missing something IMHO adding optional vdd-supply and enable-gpios doesn't seem a big deal. Anyway I don't have a hard opinion here. To me fixed-clocks still seems very appropriate for having a controlling GPIO and power supply. I just would get rid of the (comment only) hint they are ungatable. Best regards, Alexander
Am Donnerstag, 11. Juli 2024, 07:27:40 CEST schrieb Alexander Stein: > Am Donnerstag, 11. Juli 2024, 01:21:15 CEST schrieb Stephen Boyd: > > Quoting Heiko Stübner (2024-07-10 00:45:17) > > > Am Mittwoch, 10. Juli 2024, 09:02:34 CEST schrieb Alexander Stein: > > > > > > > > So essentially only enable-gpios and vdd-supply is added in comparison to > > > > fixed-clock. Does it make sense to add that to the fixed-clocks instead? > > > > Similar to fixed-regulator. > > > > > > I wasn't that sure which way to go in the first place. > > > The deciding point was reading that line about the fixed clock not > > > being gateable, so I opted to not touch the fixed-clock. > > > > > > But you're definitly right, this _could_ live inside the fixed-clock > > > as well, if we decide to get rid of the not-gateable thing above. > > > > It's probably more complicated to combine it with the fixed-clock > > binding after making properties required like vdd-supply. I'd just make > > a new binding and look at combining later. > > Maybe I am missing something IMHO adding optional vdd-supply and > enable-gpios doesn't seem a big deal. > Anyway I don't have a hard opinion here. To me fixed-clocks still > seems very appropriate for having a controlling GPIO and power supply. > I just would get rid of the (comment only) hint they are ungatable. I think the main issue is that the fixed-rate code is not limited to the actual fixed-rate clock. The clk_fixed_rate_ops is exported and used itself in a number of completely different clock drivers. The same is true for the clk_register_fixed_rate function, also exported and used in even more places throughout the kernel while implicitly using clk_fixed_rate_ops. For just being a simple always-on fixed rate clock, the clk-fixed-rate.c is already pretty complex and adding supply handling will entail modifying the shared clk-ops, or defining a separate clk-ops and clk-register function, which is what we're doing here already. Heiko
diff --git a/Documentation/devicetree/bindings/clock/clock-generator.yaml b/Documentation/devicetree/bindings/clock/clock-generator.yaml new file mode 100644 index 0000000000000..f44e61e414e89 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/clock-generator.yaml @@ -0,0 +1,62 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/clock-generator.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Simple clock generators + +maintainers: + - Heiko Stuebner <heiko@sntech.de> + +properties: + $nodename: + anyOf: + - description: + Preferred name is 'clock-<freq>' with <freq> being the output + frequency as defined in the 'clock-frequency' property. + pattern: "^clock-([0-9]+|[a-z0-9-]+)$" + - description: Any name allowed + deprecated: true + + compatible: + oneOf: + - const: clock-generator + - items: + - enum: + - diodes,pi6c557-03b + - diodes,pi6c557-05b + - const: clock-generator + + "#clock-cells": + const: 0 + + clock-frequency: true + + clock-output-names: + maxItems: 1 + + enable-gpios: + description: + Contains a single GPIO specifier for the GPIO that enables and disables + the clock generator. + maxItems: 1 + + vdd-supply: + description: handle of the regulator that provides the supply voltage + +required: + - compatible + - "#clock-cells" + - clock-frequency + +additionalProperties: false + +examples: + - | + clock { + compatible = "clock-generator"; + #clock-cells = <0>; + clock-frequency = <1000000000>; + }; +...
In contrast to fixed clocks that are described as ungateable, boards sometimes use additional clock generators for things like PCIe reference clocks, that need actual supplies to get enabled and enable-gpios to be toggled for them to work. This adds a binding for such clock generators that are not configurable themself, but need to handle supplies for them to work. While in a lot of cases the type of the IC used is described in board schematics, in some cases just a generic type description like "100MHz, 3.3V" might also be used. The binding therefore allows both cases. Specifying the type is of course preferred. The clock-frequency is set in devicetree, because while some clock generators have pins to decide between multipls output rates, those are generally set statically on the board-layout-level. Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- .../bindings/clock/clock-generator.yaml | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/clock-generator.yaml