Hi Dragan,

Am Donnerstag, 18. Juli 2024, 15:00:57 CEST schrieb Dragan Simic:

On 2024-07-18 13:30, Heiko Stübner wrote:

quoted

Am Donnerstag, 18. Juli 2024, 12:53:07 CEST schrieb Dragan Simic:

quoted

On 2024-07-18 11:25, Heiko Stübner wrote:

quoted

Am Dienstag, 16. Juli 2024, 18:15:08 CEST schrieb Conor Dooley:

quoted

On Mon, Jul 15, 2024 at 01:02:49PM +0200, Heiko Stuebner wrote:

quoted

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

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/ (local)

quoted

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/ (local)
[3]
https://lore.kernel.org/linux-kernel//20140515064420.9521.47383@quantum/T/#t (local)

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.

quoted

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.

quoted

quoted

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

quoted

quoted

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

quoted

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

quoted

quoted

- 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 (local)
[5]
https://lore.kernel.org/linux-rockchip/2e5852b9e94b9a8d0261ce7ad79f4329.sboyd@kernel.org/ (local)
[6]
https://lore.kernel.org/linux-rockchip/ec84dc37e2c421ee6d31294e08392d57@manjaro.org/ (local)
[7]
https://lore.kernel.org/linux-rockchip/b3c450a94bcb4ad0bc5b3c7ee8712cb8.sboyd@kernel.org/ (local)

[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

quoted

quoted

quoted

quoted

+
+maintainers:
+  - Heiko Stuebner [off-list ref]
+
+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>;
+    };
+...