Message-ID: <30200583.43St1lv6Oq@diego>
Date: Thu, 18 Jul 2024 13:30:57 +0200
From: Heiko Stübner <heiko@...ech.de>
To: Dragan Simic <dsimic@...jaro.org>
Cc: Conor Dooley <conor@...nel.org>, mturquette@...libre.com,
 sboyd@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 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 v2 1/3] dt-bindings: clocks: add binding for
 voltage-controlled-oscillators

Hi Dragan,

Am Donnerstag, 18. Juli 2024, 12:53:07 CEST schrieb Dragan Simic:
> On 2024-07-18 11:25, Heiko Stübner wrote:
> > Am Dienstag, 16. Juli 2024, 18:15:08 CEST schrieb Conor Dooley:
> >> On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:
> >> > In contrast to fixed clocks that are described as ungateable, boards
> >> > sometimes use additional oscillators 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 oscillators that are not configurable
> >> > themself, but need to handle supplies for them to work.
> >> >
> >> > In schematics they often can be seen as
> >> >
> >> >          ----------------
> >> > Enable - | 100MHz,3.3V, | - VDD
> >> >          |    3225      |
> >> >    GND - |              | - OUT
> >> >          ----------------
> >> >
> >> > or similar. The enable pin might be separate but can also just be tied
> >> > to the vdd supply, hence it is optional in the binding.
> >> >
> >> > Signed-off-by: Heiko Stuebner <heiko@...ech.de>
> >> > ---
> >> >  .../bindings/clock/voltage-oscillator.yaml    | 49 +++++++++++++++++++
> >> >  1 file changed, 49 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> > new file mode 100644
> >> > index 0000000000000..8bff6b0fd582e
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/clock/voltage-oscillator.yaml
> >> > @@ -0,0 +1,49 @@
> >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> > +%YAML 1.2
> >> > +---
> >> > +$id: http://devicetree.org/schemas/clock/voltage-oscillator.yaml#
> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> > +
> >> > +title: Voltage controlled oscillator
> >> 
> >> Voltage controlled oscillator? Really? That sounds far too similar to 
> >> a
> >> VCO to me, and the input voltage here (according to the description at
> >> least) does not affect the frequency of oscillation.
> > 
> > That naming was suggested by Stephen in v1 [0] .
> > 
> > Of course the schematics for the board I have only describe it as
> > "100MHz,3.3V,3225" , thumbing through some mouser parts matching that
> > only mentions "supply voltage" in their datasheets but not a dependency
> > between rate and voltage.
> > 
> > [0] 
> > https://lore.kernel.org/linux-arm-kernel/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/
> > 
> >> Why the dedicated binding, rather than adding a supply and enable-gpio
> >> to the existing "fixed-clock" binding? I suspect that a large portion 
> >> of
> >> "fixed-clock"s actually require a supply that is (effectively)
> >> always-on.
> > 
> > I guess there are three aspects:
> > - I do remember discussions in the past about not extending generic
> >   bindings with device-specific stuff. I think generic power-sequences
> >   were the topic back then, though that might have changed over time?
> > - There are places that describe "fixed-clock" as
> >   "basic fixed-rate clock that cannot gate" [1]
> > - Stephen also suggested a separate binding [2]
> > 
> > With the fixed-clock being sort of the root for everything else on most
> > systems, I opted to leave it alone. I guess if the consenus really is 
> > that
> > this should go there, I can move it, but discussion in v1
> > 
> > Interestingly the fixed clock had a gpios property 10 years ago [3] :-) 
> > .
> > 
> > [1] 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-fixed-rate.c#n18
> > [2] 
> > https://lore.kernel.org/linux-arm-kernel/68f6dc44a8202fd83792e58aea137632.sboyd@kernel.org/
> > [3] 
> > https://lore.kernel.org/linux-kernel//20140515064420.9521.47383@quantum/T/#t
> 
> After finally going through the v1 discussion [4] in detail,
> here are my further thoughts:
> 
> - I agree with dropping the Diodes stuff, [5] because I see
>    no need for that at this point;  though, am I missing
>    something, where are they actually used?

On the Rock5 ITX that 100MHz clock comes the one single oscillator thing.

The Diodes parts are not root sources for their clocks but instead sort
PLLs or something, though their manual describes them as
"clock generator supporting PCI Express and Ethernet requirements" [8]

So they generate the 100MHz (frequency actually is
selected by input pins of the chip) from a separate 25MHz source clock.

One example are the Theobroma/Cherry embedded boards changed in v1 where
they currently are described via existing generic things (no schematics).

Another user is the rk3568-rock3b for example, where the diodes part
is enabled by the same rail as the port itself, so in contrast to the
Rock 5 ITX, it works "by accident" on the rock 3b [9]


The interesting part of the Diodes ICs is that they actually allow
configuration of the generated frequency via their S0 + S1 pins,
though they might be statically set in the board layout without
being user configurable. (Rock3b does it this way for example)


> - I agree that "enable-gpios" and "vdd-supply" should be
>    required in the binding, [5] because that's the basis for
>    something to be actually represented this way

I would only require the vdd supply though.

On the Rock 5 ITX, the chip does have an enable-gpio input, but that is
tied directly to the voltage rail and is not user controllable.


> - I agree that it should be better not to touch fixed-clock
>    at this point, simply because it's used in very many places,
>    and because in this case we need more than it requires (see
>    the bullet point above)
> 
> - As I wrote already, [6] I highly disagree with this being
>    called voltage-controlled oscillator (VCO), [7] simply
>    because it isn't a VCO, but a clock that can be gated;
>    though, looking forward to what the last bullet point
>    asks to be clarified further
> 
> - I still think that gated-clock is the best choice for the
>    name, because it uses "clock" that's used throughout the
>    entire codebase, and uses "gated" to reflect the nature
>    of the clock generator

"gated-oscillator" perhaps? 

This would make it more explicit that we're talking about a root
for clock signals. "gated-clock" can be anything, in the middle
of a clock tree. Having a very generic name, also invites misuse.


> - Maybe we could actually use fixed-gated-clock as the name,
>    which would make more sense from the stanpoint of possibly
>    merging it into fixed-clock at some point, but I'd like
>    to hear first what's actually going on with the Diodes
>    stuff that was deleted in v2, which I already asked about
>    in the first bullet point
> 
> [4] 
> https://lore.kernel.org/linux-rockchip/20240709123121.1452394-1-heiko@sntech.de/T/#u
> [5] 
> https://lore.kernel.org/linux-rockchip/2e5852b9e94b9a8d0261ce7ad79f4329.sboyd@kernel.org/
> [6] 
> https://lore.kernel.org/linux-rockchip/ec84dc37e2c421ee6d31294e08392d57@manjaro.org/
> [7] 
> https://lore.kernel.org/linux-rockchip/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/

[8] https://www.diodes.com/assets/Datasheets/PI6C557-03.pdf
[9] https://dl.radxa.com/rock3/docs/hw/3b/Radxa_ROCK_3B_V1.51_SCH.pdf
    page 31, bottom left of the page


> >> > +
> >> > +maintainers:
> >> > +  - Heiko Stuebner <heiko@...ech.de>
> >> > +
> >> > +properties:
> >> > +  compatible:
> >> > +    const: voltage-oscillator
> >> > +
> >> > +  "#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 oscillator.
> >> > +    maxItems: 1
> >> > +
> >> > +  vdd-supply:
> >> > +    description: handle of the regulator that provides the supply voltage
> >> > +
> >> > +required:
> >> > +  - compatible
> >> > +  - "#clock-cells"
> >> > +  - clock-frequency
> >> > +  - vdd-supply
> >> > +
> >> > +additionalProperties: false
> >> > +
> >> > +examples:
> >> > +  - |
> >> > +    voltage-oscillator {
> >> > +      compatible = "voltage-oscillator";
> >> > +      #clock-cells = <0>;
> >> > +      clock-frequency = <1000000000>;
> >> > +      vdd-supply = <&reg_vdd>;
> >> > +    };
> >> > +...
> 

Powered by blists - more mailing lists