Re: [PATCH v2 1/3] dt-bindings: clocks: add binding for voltage-controlled-oscillators

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4951 bytes --]

On Thu, Jul 18, 2024 at 11:25:28AM +0200, Heiko Stübner wrote:
> Hi Conor,
> 
> 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@sntech.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] .

I think "voltage-oscillator" is a confusing name, and having "voltage
controlled oscillator" in the title doubly so as this isn't a binding
for a VCO.
A VCO is a more general case of the sort of device that you're talking
about here, so a part of me can see it - but I think specific
compatibles would be required for actual VCOs, since the "transfer
function" would vary per device.

> 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.

FWIW, I wouldn't classify this as device-specific. "enable-gpios" and
"vdd-supply" are pretty generic and I think the latter is missing from
the vast majority of real* "fixed-clocks". I would expect that devices
where the datasheet would call

* Real because there's plenty of "fixed-clocks" (both in and out of tree)
that are used to work around the lack of a clock-controller driver for an
SoC.

> 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]

I think that that is something that could be changed, it's "just" a
comment in some code! Sounds like Stephen disagrees though :)

> - Stephen also suggested a separate binding [2]

I liked your "gated-oscillator" suggestion in another reply, but
"gated-fixed-clock" might be a better "thematic" fit since this is a
special case of fixed-clocks?

Cheers,
Conor.

> 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] :-) .

heh!

> 
> [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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]