Hi Dragan, Am Donnerstag, 18. Juli 2024, 15:00:57 CEST schrieb Dragan Simic: > On 2024-07-18 13:30, Heiko Stübner wrote: > > 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@xxxxxxxxx> > >> >> > --- > >> >> > .../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@xxxxxxxxxx/ > >> > > >> >> 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@xxxxxxxxxx/ > >> > [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] > > Ah, I see now, thanks for the clarification. The way Diodes PI6C557 > is used on the ROCK 3B, together with its 25 MHz "passive" crystal, > is pretty much the same as the way Aurasemi AU5426 is used on the > ROCK 5 ITX, together with its 100 MHz "active" clock generator. All > that from the software standpoint, of course. > > To explain it further, the PI6C577 and the AU5426 are the components > that actually generate the clocks for the PCIe interfaces. Thus, > technically we should describe two components per board in their DTs: > > - ROCK 5 ITX: > - 100 MHz clock generator (which goes to the AU5426), > i.e. the "100MHz,3.3V,3225" > - clock buffer that fans the 100 MHz clock out to the > PCIe interfaces, i.e. the Aurasemi AU5426 > > - ROCK 3B: > - 25 MHz "passive" crystal (which goes to the PI6C557) > - clock generator that uses the 25 MHz crystal to generate > 100 MHz and fan it out to the PCIe interfaces, i.e. the > Diodes PI6C557 > > (The "passive" 25 MHz crystal is very similar to the main 24 MHz > crystal used by the SoC, also known as xin24m.) > > However, simplifying and abstracting the things out a bit should > be fine, to end up with the following: > > - ROCK 5 ITX: > - "black box" that generates fixed 100 MHz clocks for the > PCIe interfaces, which can be gated > > - ROCK 3B: > - "black box" that generates fixed 100 MHz clocks for the > PCIe interfaces, which can be gated > > ... and that's our, hopefully, gated-clock. :) I don't think we want too many black-boxes. Devicetree's purpose is to describe the hardware as best as possible, the OS can make of it what it wants afterwards ;-) . That was also Stephen's point in v1 when he wanted to distinguish between the input-less oscillator and the Diodes things that needs an input clock. > > 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) > > The Aurasemi AU5426 also has a few tricks up its sleeve. :) For > example, it can also use a "passive" crystal instead of using an > external clock generator, i.e. it can be configured to work in > the same "big picture" layout as the PI6C577. > > >> - 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. > > Isn't that voltage rail (VCC3V3_PI6C_05, provided by the U90099 > regulator, > which is an RT9193) actually enabled by GPIO1_A4_d (see pages 14 and 35 > in the ROCK 5 ITX schematic and follow PCIE30x_PWREN_H)? > > Unless I'm missing something, that's the source of all troubles, because > we basically also need to enable PCIE30x_PWREN_H when only the other M.2 > slot is in use, or when it's enumerated first. Thus, we need > enable-gpios > to be able to enable the VCC3V3_PI6C_05 independently. And you correctly described the problem I faced and that got me on that journey (sata-pcie-controller probing before M.2 attached controller). PCIE30x4_PWREN_H actually drives two separate regulators though, U90098 and U90099 . U90098 creates VCC3V3_MKEY which is the supply for the main M.2 slot U90099 creates VCC3V3_PI6C_05 which is the oscillator and Au5426 supply PCIE30x4_PWREN_H definitly belongs to a regulator node in the devicetree that then either the M.2 slot or the oscillator can enable first. > >> - 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? > > But what's the output of an oscillator, which we actually care about? > It's nothing more but a clock. :) > > > 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. > > I can't escape wondering why doesn't "fixed-clock", which is also > a very generic name, invite abuse? I guess it does - see the misuse in the rk3588 tiger and jaguar boards. But it is sort of grandfathered I guess. It is still from a time more than 12 years ago, when architectures (like sunxi) tried to model their clock- controllers via individual interconnected dt-nodes. > Speaking about the root of clock signals, the above-mentioned xin24m, > which is represented in DTs as a fixed-clock, is the root of pretty > much everything. Also, it technically isn't a clock, it's a crystal. > That's another example of the above-mentioned abstraction, in which > we care about the way sotware sees it, which is a clock. At this point I think we should dt-people weigh in more on which direction to take ;-) . Because as a lot of dt-binding review in the past mentioned, the "software-perspective" doesn't matter for the binding. Heiko > >> - 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@xxxxxxxxx/T/#u > >> [5] > >> https://lore.kernel.org/linux-rockchip/2e5852b9e94b9a8d0261ce7ad79f4329.sboyd@xxxxxxxxxx/ > >> [6] > >> https://lore.kernel.org/linux-rockchip/ec84dc37e2c421ee6d31294e08392d57@xxxxxxxxxxx/ > >> [7] > >> https://lore.kernel.org/linux-rockchip/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@xxxxxxxxxx/ > > > > [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@xxxxxxxxx> > >> >> > + > >> >> > +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 = <®_vdd>; > >> >> > + }; > >> >> > +... >
