Re: [PATCH v4 1/2] dt-bindings: Add GPIO-locked fixed clock

[Conor Dooley <conor@kernel.org>]

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

On Fri, Aug 28, 2026 at 07:31:10AM +0200, Vyacheslav Yurkov wrote:
> On 27.08.2026 18:55, Conor Dooley wrote:
> 
> > > It is n input clocks and 1 output clock. It is kind of a mux, but the CPU
> > > doesn't control the clocks or GPIO signals. The whole idea is that
> > > peripherals check the output clock, when it's locked that means _all_ the
> > > clocks are locked and GPIOs are in expected state. That's why the selection
> > > operation is not really implemented.
> > > 
> > > Actually the number of input clocks don't have to correspond to the number
> > > of the GPIOs, because the GPIO signals indicate the locked state of the
> > > clocks that are not accessible to the CPU.
> > 
> > I'm not entirely sure what you mean by this, but it is starting to sound
> > like you're only having one output because that's the minimum you need to do
> > to ensure that this driver has probed before the peripheral(s) using the
> > N input clocks.
> 
> That's exactly the idea. How else I would ensure in peripheral's probe
> driver that clocks are locked?

Every peripheral should already contain a reference to all of the clocks
that are connected to it, and the peripheral drivers should request
those clocks. If you do that for all clocks a peripheral uses, you don't
need to have an artificial check for the gpios representing clocks
b,c,d,e,f,g... in the enable function for clock a, because their own
{devm,}clk_get_enabled() will interrogate the relevant bit.

At present, I'm expecting that you have a dt like

clk_gpio_locked: gpio-locked-fixed-clock {
    compatible = "gpio-locked-fixed-clock";
    #clock-cells = <0>;

    clocks = <&clk0 0>, <&pll 0>;

    locked-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>,
            <&gpio0 5 GPIO_ACTIVE_HIGH>,
            <&gpio1 2 GPIO_ACTIVE_LOW>;

    clock-output-names = "clkout0";
};

peripheral {
	clocks = <&clk_gpio_locked>, <&clk0 0>, <&pll 0>;
	//or this, depending on whether clkout0 actually produces a
	//rate, which I guess it does given it's a passthrough for the
	//first clocks entry?
	clocks = <&clk_gpio_locked>, <&pll 0>;
};

when really you should have something like

clk_gpio_locked: gpio-locked-fixed-clock {
    compatible = "gpio-locked-fixed-clock";
    #clock-cells = <1>;

    clocks = <&clk0 0>, <&pll 0>, <more clocks here>;

    locked-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>,
            <&gpio0 5 GPIO_ACTIVE_HIGH>,
            <&gpio1 2 GPIO_ACTIVE_LOW>;

    clock-output-names = "clkout0", "clkout1", "more clocks here";
};

peripheral {
	clocks = <&clk_gpio_locked 0>, <&clk_gpio_locked 1>;
};

> > Requiring other input clocks to be stable before
> > declaring the input that's actually connected to the output stable
> > appears to be a shortcut/hack rather than an accurate description of the
> > hardware. If that's the case, I'd be much happier with this if this was
> > implemented as either a) N inputs with N gpios and N outputs, or b) 1 input,
> > M gpios (if multiple represent the stability of that input) and 1 output,
> > with N instances, one for each clock.
> 
> How having N outputs would help? It looks like it would just move the job I
> do here into each peripheral driver instead.

Two reasons. Firstly because what you've got here suits you just fine,
but since you're adding it to a generic binding it should be written in
the most generic manner possible. Some other similar use case might not
want to wait for clocks used by other peripherals to lock before
allowing the driver for what they do care about to probe.
Secondly because it's a bit of a hack currently, you've got one clock
representing the status for a bunch of other clocks. Each clock should
represent it's own status. If this was done in the hardware it'd be one
thing, but you've got a pure software construct blocking one clock from
being locked because other clocks that may be unrelated to it entirely
are not yet locked.
The argument for including this depends on it not being a pure software
construct, and having what appears to be a hack so that you can rely on
the status of one clock rather than correctly determining the locked
state of individual clocks undermines your own argument for inclusion.

Cheers,
Conor.

> > On the other hand, if this is genuinely a mux, then the binding should
> > reflect that, rather than only describe a subset of what you can do and
> > the driver should only check the actual parent out the output, rather
> > than the N-1 other inputs.


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