Message-ID: <1903511.1xdlsreqCQ@diego>
Date: Wed, 10 Jul 2024 10:02:57 +0200
From: Heiko Stübner <heiko@...ech.de>
To: mturquette@...libre.com, Stephen Boyd <sboyd@...nel.org>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 quentin.schulz@...rry.de, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Subject:
 Re: [PATCH 1/6] dt-bindings: clocks: add binding for generic clock-generators

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@...ech.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@...ech.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".

Powered by blists - more mailing lists