Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Vyacheslav Yurkov]

On 19.03.2026 17:50, Conor Dooley wrote:

>> I described a use case in my cover letter (PATCH 0). Perhaps our approach to
>> tackle the issue is not correct in the first place. The term "virtual clock
>> controller guard" is something we named it, but it's literally just a clock
>> provider which combines several other clocks and input GPIO signals in order
>> for the consumers to check whether they are allowed to probe already or have
>> to wait until the input clocks are enabled.
> 
> Can you explain how this is different to gpio-gate-clock? AFAICT, you're
> trying to support clocks that are enabled by a gpio, and that's what it
> is for.
> 
It partially covers the similar use case, but differs in the sense that 
gpio-gate-clock controls the clock via GPIO (enable/disable), the 
clock-controller-guard gets the GPIO status signals whether the clock 
_was_ enabled externally because a CPU has no direct access to the 
clock. So perhaps the terminology I came up with is not so 
self-explanatory, that's why I posted it for review and other opinions.

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Conor Dooley]

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

On Mon, Mar 23, 2026 at 02:52:21PM +0100, Vyacheslav Yurkov wrote:
> On 19.03.2026 17:50, Conor Dooley wrote:
> 
> > > I described a use case in my cover letter (PATCH 0). Perhaps our approach to
> > > tackle the issue is not correct in the first place. The term "virtual clock
> > > controller guard" is something we named it, but it's literally just a clock
> > > provider which combines several other clocks and input GPIO signals in order
> > > for the consumers to check whether they are allowed to probe already or have
> > > to wait until the input clocks are enabled.
> > 
> > Can you explain how this is different to gpio-gate-clock? AFAICT, you're
> > trying to support clocks that are enabled by a gpio, and that's what it
> > is for.
> > 
> It partially covers the similar use case, but differs in the sense that
> gpio-gate-clock controls the clock via GPIO (enable/disable), the
> clock-controller-guard gets the GPIO status signals whether the clock _was_
> enabled externally because a CPU has no direct access to the clock. So
> perhaps the terminology I came up with is not so self-explanatory, that's
> why I posted it for review and other opinions.

The binding you've got says "GPIOs used to control or guard the clocks",
which is not what you're saying that is going on in this mail. A more
suitable description would be "GPIOs used to check the status of the
clocks".

I want to see an example dts user for this please.

TBH, I don't understand your driver implementation either and why it has

+static const struct clk_ops clkctrl_guard_ops = {

+	.enable =	clkctrl_guard_enable,
+	.disable =	clkctrl_guard_disable,
+	.prepare =	clkctrl_guard_prepare,
+	.unprepare =	clkctrl_guard_unprepare,
+	.is_prepared =	clkctrl_guard_is_prepared,

any of these 4 implemented when you have no control over the clock.
I didn't think it was required to call your parent clocks enables in
your own enable either, thought that was handled by the core recursively
calling clk_enable() on clk->parent. The one thing I would expect you to
have implemented ops wise is is_enabled, which you don't have.
Also no sign of any rate acquisition functions, which I thought were
mandatory.

+	.get_parent =	clkctrl_guard_get_parent,
+};

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

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Vyacheslav Yurkov]

On 23.03.2026 21:14, Conor Dooley wrote:

> 
> The binding you've got says "GPIOs used to control or guard the clocks",
> which is not what you're saying that is going on in this mail. A more
> suitable description would be "GPIOs used to check the status of the
> clocks".

Agree, the description I provided is not very accurate.

> I want to see an example dts user for this please.

DTS example:
   clock_guard: clock_controller_guard {
     compatible = "clock-controller-guard";
     #clock-cells = <1>;
     clocks = <&h2f_clk 0>, <&clk_fgpa_rx 0>, <clk_fpga_tx 0>;
     clock-names = "h2f_clk0", "clk_fpga_rx", "clk_fpga_tx";
     gpios = <&fpga_ip 0 GPIO_ACTIVE_HIGH>, <&fpga_ip 1 GPIO_ACTIVE_HIGH>;
     gpio-names = "gpio-input0", "gpio-input1";
     clock-output-names = "clkctrl-guard";
   };

   custom_device {
     compatible = "...";
     ...
     #clock-cells = <1>;
     clocks = <&clock_guard 0>;
     clock-names = "clock-guard";
   };

The driver usage exaple:

clk = devm_clk_get(dev, "clock-guard");
if (IS_ERR(clk))
	return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");

ret = clk_prepare_enable(clk);
if (ret) {
	dev_warn(dev, "Clock is not ready, %d\n", ret);
	return -EPROBE_DEFER;
}


> TBH, I don't understand your driver implementation either and why it has
> 
> +static const struct clk_ops clkctrl_guard_ops = {
> 
> +	.enable =	clkctrl_guard_enable,
> +	.disable =	clkctrl_guard_disable,
> +	.prepare =	clkctrl_guard_prepare,
> +	.unprepare =	clkctrl_guard_unprepare,
> +	.is_prepared =	clkctrl_guard_is_prepared,
> 
> any of these 4 implemented when you have no control over the clock.
> I didn't think it was required to call your parent clocks enables in
> your own enable either, thought that was handled by the core recursively
> calling clk_enable() on clk->parent. The one thing I would expect you to
> have implemented ops wise is is_enabled, which you don't have.
> Also no sign of any rate acquisition functions, which I thought were
> mandatory.
> 
> +	.get_parent =	clkctrl_guard_get_parent,
> +};

Good point on .is_enabled, I indeed missed that. As for the rate 
acquisition functions I referred to this table 
https://docs.kernel.org/driver-api/clk.html#id4 , and it see that 
.set_rate is actually optional.

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Krzysztof Kozlowski]

On 26/03/2026 10:54, Vyacheslav Yurkov wrote:
> On 23.03.2026 21:14, Conor Dooley wrote:
> 
>>
>> The binding you've got says "GPIOs used to control or guard the clocks",
>> which is not what you're saying that is going on in this mail. A more
>> suitable description would be "GPIOs used to check the status of the
>> clocks".
> 
> Agree, the description I provided is not very accurate.
> 
>> I want to see an example dts user for this please.
> 
> DTS example:
>    clock_guard: clock_controller_guard {
>      compatible = "clock-controller-guard";
>      #clock-cells = <1>;
>      clocks = <&h2f_clk 0>, <&clk_fgpa_rx 0>, <clk_fpga_tx 0>;
>      clock-names = "h2f_clk0", "clk_fpga_rx", "clk_fpga_tx";
>      gpios = <&fpga_ip 0 GPIO_ACTIVE_HIGH>, <&fpga_ip 1 GPIO_ACTIVE_HIGH>;
>      gpio-names = "gpio-input0", "gpio-input1";
>      clock-output-names = "clkctrl-guard";
>    };
> 
>    custom_device {
>      compatible = "...";
>      ...
>      #clock-cells = <1>;
>      clocks = <&clock_guard 0>;
>      clock-names = "clock-guard";
>    };

So a pure SW construct? Device has specific clock inputs but you do not
model them and instead replace with one fake-guard-input.

I don't see how this represents the hardware at all.

Maybe some diagrams would help, assuming we still talk about hardware.

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Vyacheslav Yurkov]

On 26.03.2026 11:08, Krzysztof Kozlowski wrote:

>>
>> DTS example:
>>     clock_guard: clock_controller_guard {
>>       compatible = "clock-controller-guard";
>>       #clock-cells = <1>;
>>       clocks = <&h2f_clk 0>, <&clk_fgpa_rx 0>, <clk_fpga_tx 0>;
>>       clock-names = "h2f_clk0", "clk_fpga_rx", "clk_fpga_tx";
>>       gpios = <&fpga_ip 0 GPIO_ACTIVE_HIGH>, <&fpga_ip 1 GPIO_ACTIVE_HIGH>;
>>       gpio-names = "gpio-input0", "gpio-input1";
>>       clock-output-names = "clkctrl-guard";
>>     };
>>
>>     custom_device {
>>       compatible = "...";
>>       ...
>>       #clock-cells = <1>;
>>       clocks = <&clock_guard 0>;
>>       clock-names = "clock-guard";
>>     };
> 
> So a pure SW construct? Device has specific clock inputs but you do not
> model them and instead replace with one fake-guard-input.
> 
> I don't see how this represents the hardware at all.
> 
> Maybe some diagrams would help, assuming we still talk about hardware.
> 
> Best regards,
> Krzysztof

Techincally that's correct, it's a software construct. If this is not a 
right place to submit such a helper driver, I'd appreciate a hint what 
subsystem is the right one.

I was not sure how to provide a diagram in the mailing list, so I posted 
in on Github https://github.com/OSS-Keepers/clock-controller-guard/issues/1

It is a driver which models dependencies for other drivers. These are 
soft or "indirect" dependencies, because we cannot access the FPGA 
unless the FPGA_PLL_locked, and GPIO is telling us we are good to go.

Conor, I think this should answer your question as well.

Thanks,
Slava

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Krzysztof Kozlowski]

On 26/03/2026 14:39, Vyacheslav Yurkov wrote:
> On 26.03.2026 11:08, Krzysztof Kozlowski wrote:
> 
>>>
>>> DTS example:
>>>     clock_guard: clock_controller_guard {
>>>       compatible = "clock-controller-guard";
>>>       #clock-cells = <1>;
>>>       clocks = <&h2f_clk 0>, <&clk_fgpa_rx 0>, <clk_fpga_tx 0>;
>>>       clock-names = "h2f_clk0", "clk_fpga_rx", "clk_fpga_tx";
>>>       gpios = <&fpga_ip 0 GPIO_ACTIVE_HIGH>, <&fpga_ip 1 GPIO_ACTIVE_HIGH>;
>>>       gpio-names = "gpio-input0", "gpio-input1";
>>>       clock-output-names = "clkctrl-guard";
>>>     };
>>>
>>>     custom_device {
>>>       compatible = "...";
>>>       ...
>>>       #clock-cells = <1>;
>>>       clocks = <&clock_guard 0>;
>>>       clock-names = "clock-guard";
>>>     };
>>
>> So a pure SW construct? Device has specific clock inputs but you do not
>> model them and instead replace with one fake-guard-input.
>>
>> I don't see how this represents the hardware at all.
>>
>> Maybe some diagrams would help, assuming we still talk about hardware.
>>
>> Best regards,
>> Krzysztof
> 
> Techincally that's correct, it's a software construct. If this is not a 
> right place to submit such a helper driver, I'd appreciate a hint what 
> subsystem is the right one.
> 
> I was not sure how to provide a diagram in the mailing list, so I posted 
> in on Github https://github.com/OSS-Keepers/clock-controller-guard/issues/1

Diagram of hardware. We talk here about hardware.

> 
> It is a driver which models dependencies for other drivers. These are 
> soft or "indirect" dependencies, because we cannot access the FPGA 
> unless the FPGA_PLL_locked, and GPIO is telling us we are good to go.

DT is not for drivers. It's not about subsystem, but concept. DT
describes hardware.

Certain generic bindings which solve real and common hardware problems
are accepted, but so far I miss what hardware problem are you solving here.

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Conor Dooley]

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

On Thu, Mar 26, 2026 at 02:39:22PM +0100, Vyacheslav Yurkov wrote:
> On 26.03.2026 11:08, Krzysztof Kozlowski wrote:
> 
> > > 
> > > DTS example:
> > >     clock_guard: clock_controller_guard {
> > >       compatible = "clock-controller-guard";
> > >       #clock-cells = <1>;
> > >       clocks = <&h2f_clk 0>, <&clk_fgpa_rx 0>, <clk_fpga_tx 0>;
> > >       clock-names = "h2f_clk0", "clk_fpga_rx", "clk_fpga_tx";
> > >       gpios = <&fpga_ip 0 GPIO_ACTIVE_HIGH>, <&fpga_ip 1 GPIO_ACTIVE_HIGH>;
> > >       gpio-names = "gpio-input0", "gpio-input1";
> > >       clock-output-names = "clkctrl-guard";
> > >     };
> > > 
> > >     custom_device {
> > >       compatible = "...";
> > >       ...
> > >       #clock-cells = <1>;
> > >       clocks = <&clock_guard 0>;
> > >       clock-names = "clock-guard";
> > >     };
> > 
> > So a pure SW construct? Device has specific clock inputs but you do not
> > model them and instead replace with one fake-guard-input.
> > 
> > I don't see how this represents the hardware at all.
> > 
> > Maybe some diagrams would help, assuming we still talk about hardware.
> > 
> > Best regards,
> > Krzysztof
> 
> Techincally that's correct, it's a software construct. If this is not a

Is it a software construct?
I assume that the PLL status is going to be some lock bit in a register,
and you're got some "hardware" in your FPGA fabric that reads that bit
and sets a GPIO when it gets locked? Or maybe it's even simpler, and
your GPIO just gets set once your custom HDL comes out of reset, which
happens when the PLL locks?
If that's an approximation of what you have, that's not a software
construct.

> right place to submit such a helper driver, I'd appreciate a hint what
> subsystem is the right one.
> 
> I was not sure how to provide a diagram in the mailing list, so I posted in
> on Github https://github.com/OSS-Keepers/clock-controller-guard/issues/1
> 
> It is a driver which models dependencies for other drivers. These are soft
> or "indirect" dependencies, because we cannot access the FPGA unless the
> FPGA_PLL_locked, and GPIO is telling us we are good to go.
> 
> Conor, I think this should answer your question as well.

Not really, but it gets part of the way there. I want to know what this
provider actually is. I now know it is a PLL, not an off-chip
oscillator, but I know nothing about the interface that you have to it
(or if you have one at all). What compatible string/kernel driver does
it use?

Because SoC-FPGAs can route GPIOs from the SoC part to the FPGA fabric
and use them as if interacting with something off-chip, I'm not sure if
we are dealing with an separate FPGA or a SoC-FPGA. Which is it?
Effectively I want to understand why you cannot just read the lock bit
from the PLL directly. In my experience with *SoC*-FPGAs, things like
PLLs that must lock for the fabric to be usable have a register
interface from which the lock bit can be read, that is of course not
clocked by the PLL output clock and therefore accessible before the
PLL has locked.

I think more info is needed here to guide you on where such a "helper
driver" should be located and what the dt represetation should be.


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

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Vyacheslav Yurkov]

On 26.03.2026 19:32, Conor Dooley wrote:

>> I was not sure how to provide a diagram in the mailing list, so I posted in
>> on Github https://github.com/OSS-Keepers/clock-controller-guard/issues/1
>>
>> It is a driver which models dependencies for other drivers. These are soft
>> or "indirect" dependencies, because we cannot access the FPGA unless the
>> FPGA_PLL_locked, and GPIO is telling us we are good to go.
>>
>> Conor, I think this should answer your question as well.
> 
> Not really, but it gets part of the way there. I want to know what this
> provider actually is. I now know it is a PLL, not an off-chip
> oscillator, but I know nothing about the interface that you have to it
> (or if you have one at all). What compatible string/kernel driver does
> it use?
> 
> Because SoC-FPGAs can route GPIOs from the SoC part to the FPGA fabric
> and use them as if interacting with something off-chip, I'm not sure if
> we are dealing with an separate FPGA or a SoC-FPGA. Which is it?
> Effectively I want to understand why you cannot just read the lock bit
> from the PLL directly. In my experience with *SoC*-FPGAs, things like
> PLLs that must lock for the fabric to be usable have a register
> interface from which the lock bit can be read, that is of course not
> clocked by the PLL output clock and therefore accessible before the
> PLL has locked.
> 
> I think more info is needed here to guide you on where such a "helper
> driver" should be located and what the dt represetation should be.

I really appreciate your feedback on this. Here's an attempt to provide 
a better exlanation.

We have various use cases. Most of the time it's a PLL in the FPGA but 
it can also be some signal from a custom FPGA IP used to indicate if 
some preconditions are met and the IP is ready to be used (some kind of 
inverted reset but exposed by the IP). For a PLL we typically get the 
signal connected either to a GPIO IP block (altr,pio-1.0) OR to a bit in 
a custom IP register.
In addition, some of the IPs in our design do not have a proper split 
between registers and IP core, which means that if an external clock 
and/or PLL lock is missing and we access the registers we won’t ever get 
an answer and thus stall the CPU.

We are using a SoC-FPGA and use some GPIO IP within the FPGA 
(altr,pio-1.0 for example).

The PLL itself doesn't have any registers but the signal indicating that 
it is locked is available and routed to such a GPIO.

The point is that we will have several IPs/drivers that will depend on 
the same preconditions (clk, gpios being high or low) and we want to use 
this clk_guard driver as an aggregator for those pre-conditions. Define 
once, reuse a lot.

Slava

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Conor Dooley]

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

On Sat, Mar 28, 2026 at 03:58:47AM +0100, Vyacheslav Yurkov wrote:
> On 26.03.2026 19:32, Conor Dooley wrote:
> 
> > > I was not sure how to provide a diagram in the mailing list, so I posted in
> > > on Github https://github.com/OSS-Keepers/clock-controller-guard/issues/1
> > > 
> > > It is a driver which models dependencies for other drivers. These are soft
> > > or "indirect" dependencies, because we cannot access the FPGA unless the
> > > FPGA_PLL_locked, and GPIO is telling us we are good to go.
> > > 
> > > Conor, I think this should answer your question as well.
> > 
> > Not really, but it gets part of the way there. I want to know what this
> > provider actually is. I now know it is a PLL, not an off-chip
> > oscillator, but I know nothing about the interface that you have to it
> > (or if you have one at all). What compatible string/kernel driver does
> > it use?
> > 
> > Because SoC-FPGAs can route GPIOs from the SoC part to the FPGA fabric
> > and use them as if interacting with something off-chip, I'm not sure if
> > we are dealing with an separate FPGA or a SoC-FPGA. Which is it?
> > Effectively I want to understand why you cannot just read the lock bit
> > from the PLL directly. In my experience with *SoC*-FPGAs, things like
> > PLLs that must lock for the fabric to be usable have a register
> > interface from which the lock bit can be read, that is of course not
> > clocked by the PLL output clock and therefore accessible before the
> > PLL has locked.
> > 
> > I think more info is needed here to guide you on where such a "helper
> > driver" should be located and what the dt represetation should be.
> 
> I really appreciate your feedback on this. Here's an attempt to provide a
> better exlanation.
> 
> We have various use cases. Most of the time it's a PLL in the FPGA but it
> can also be some signal from a custom FPGA IP used to indicate if some
> preconditions are met and the IP is ready to be used (some kind of inverted
> reset but exposed by the IP). For a PLL we typically get the signal
> connected either to a GPIO IP block (altr,pio-1.0) OR to a bit in a custom
> IP register.
> In addition, some of the IPs in our design do not have a proper split
> between registers and IP core, which means that if an external clock and/or
> PLL lock is missing and we access the registers we won’t ever get an answer
> and thus stall the CPU.
> 
> We are using a SoC-FPGA and use some GPIO IP within the FPGA (altr,pio-1.0
> for example).
> 
> The PLL itself doesn't have any registers but the signal indicating that it
> is locked is available and routed to such a GPIO.
> 
> The point is that we will have several IPs/drivers that will depend on the
> same preconditions (clk, gpios being high or low) and we want to use this
> clk_guard driver as an aggregator for those pre-conditions. Define once,
> reuse a lot.

Apologies for the delay responding, been sick the last week.
I think what you have here is not unreasonable, but may never have users
other than yourself! It's effectively gated-fixed-clock but the gpio
direction is inverted. I'm not keen on the "guarded" wording, but I
think I would want you to become a fixed-clock variant w/ a prefix that
indicates what's different. Then you get locked-gpios instead of
enable-gpios that gated-fixed-clock has.

Or maybe you're a gpio-gate-clock variant instead, and represent the PLL
as a fixed-clock (but I think being a fixed-clock variant makes more
sense).

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

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Conor Dooley]

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

On Thu, Mar 26, 2026 at 10:54:52AM +0100, Vyacheslav Yurkov wrote:
> On 23.03.2026 21:14, Conor Dooley wrote:
> 
> > 
> > The binding you've got says "GPIOs used to control or guard the clocks",
> > which is not what you're saying that is going on in this mail. A more
> > suitable description would be "GPIOs used to check the status of the
> > clocks".
> 
> Agree, the description I provided is not very accurate.
> 
> > I want to see an example dts user for this please.
> 
> DTS example:
>   clock_guard: clock_controller_guard {
>     compatible = "clock-controller-guard";
>     #clock-cells = <1>;
>     clocks = <&h2f_clk 0>, <&clk_fgpa_rx 0>, <clk_fpga_tx 0>;

Unfortunately, this doesn't contain the part that I wanted to see - who
the providers of these clocks here actually are.

To be frank, I am not sure how this block would know that these clocks
are enabled but their providers do not. I can think of a few ideas for
how this block would know, but I don't understand why the providers
themselves don't, and therefore why you need this gpio to tell you.

>     clock-names = "h2f_clk0", "clk_fpga_rx", "clk_fpga_tx";
>     gpios = <&fpga_ip 0 GPIO_ACTIVE_HIGH>, <&fpga_ip 1 GPIO_ACTIVE_HIGH>;
>     gpio-names = "gpio-input0", "gpio-input1";
>     clock-output-names = "clkctrl-guard";
>   };
> 
>   custom_device {
>     compatible = "...";
>     ...
>     #clock-cells = <1>;
>     clocks = <&clock_guard 0>;
>     clock-names = "clock-guard";
>   };
> 
> The driver usage exaple:
> 
> clk = devm_clk_get(dev, "clock-guard");
> if (IS_ERR(clk))
> 	return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> 
> ret = clk_prepare_enable(clk);
> if (ret) {
> 	dev_warn(dev, "Clock is not ready, %d\n", ret);
> 	return -EPROBE_DEFER;
> }
> 
> 
> > TBH, I don't understand your driver implementation either and why it has
> > 
> > +static const struct clk_ops clkctrl_guard_ops = {
> > 
> > +	.enable =	clkctrl_guard_enable,
> > +	.disable =	clkctrl_guard_disable,
> > +	.prepare =	clkctrl_guard_prepare,
> > +	.unprepare =	clkctrl_guard_unprepare,
> > +	.is_prepared =	clkctrl_guard_is_prepared,
> > 
> > any of these 4 implemented when you have no control over the clock.
> > I didn't think it was required to call your parent clocks enables in
> > your own enable either, thought that was handled by the core recursively
> > calling clk_enable() on clk->parent. The one thing I would expect you to
> > have implemented ops wise is is_enabled, which you don't have.
> > Also no sign of any rate acquisition functions, which I thought were
> > mandatory.
> > 
> > +	.get_parent =	clkctrl_guard_get_parent,
> > +};
> 
> Good point on .is_enabled, I indeed missed that. As for the rate acquisition
> functions I referred to this table
> https://docs.kernel.org/driver-api/clk.html#id4 , and it see that .set_rate
> is actually optional.

.set_rate is not rate acquisition. .round_rate and .determine_rate are.
I thought they were mandatory, but for a gate clock I guess they are not
and the parent rate gets used automatically.

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

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Vyacheslav Yurkov]

On 26.03.2026 11:44, Conor Dooley wrote:
> On Thu, Mar 26, 2026 at 10:54:52AM +0100, Vyacheslav Yurkov wrote:
>> On 23.03.2026 21:14, Conor Dooley wrote:
>>
>>>
>>> The binding you've got says "GPIOs used to control or guard the clocks",
>>> which is not what you're saying that is going on in this mail. A more
>>> suitable description would be "GPIOs used to check the status of the
>>> clocks".
>>
>> Agree, the description I provided is not very accurate.
>>
>>> I want to see an example dts user for this please.
>>
>> DTS example:
>>    clock_guard: clock_controller_guard {
>>      compatible = "clock-controller-guard";
>>      #clock-cells = <1>;
>>      clocks = <&h2f_clk 0>, <&clk_fgpa_rx 0>, <clk_fpga_tx 0>;
> 
> Unfortunately, this doesn't contain the part that I wanted to see - who
> the providers of these clocks here actually are.
> 
> To be frank, I am not sure how this block would know that these clocks
> are enabled but their providers do not. I can think of a few ideas for
> how this block would know, but I don't understand why the providers
> themselves don't, and therefore why you need this gpio to tell you.
> 
>>      clock-names = "h2f_clk0", "clk_fpga_rx", "clk_fpga_tx";
>>      gpios = <&fpga_ip 0 GPIO_ACTIVE_HIGH>, <&fpga_ip 1 GPIO_ACTIVE_HIGH>;
>>      gpio-names = "gpio-input0", "gpio-input1";
>>      clock-output-names = "clkctrl-guard";
>>    };
>>
>>    custom_device {
>>      compatible = "...";
>>      ...
>>      #clock-cells = <1>;
>>      clocks = <&clock_guard 0>;
>>      clock-names = "clock-guard";
>>    };
>>
>> The driver usage exaple:
>>
>> clk = devm_clk_get(dev, "clock-guard");
>> if (IS_ERR(clk))
>> 	return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
>>
>> ret = clk_prepare_enable(clk);
>> if (ret) {
>> 	dev_warn(dev, "Clock is not ready, %d\n", ret);
>> 	return -EPROBE_DEFER;
>> }
>>
>>
>>> TBH, I don't understand your driver implementation either and why it has
>>>
>>> +static const struct clk_ops clkctrl_guard_ops = {
>>>
>>> +	.enable =	clkctrl_guard_enable,
>>> +	.disable =	clkctrl_guard_disable,
>>> +	.prepare =	clkctrl_guard_prepare,
>>> +	.unprepare =	clkctrl_guard_unprepare,
>>> +	.is_prepared =	clkctrl_guard_is_prepared,
>>>
>>> any of these 4 implemented when you have no control over the clock.
>>> I didn't think it was required to call your parent clocks enables in
>>> your own enable either, thought that was handled by the core recursively
>>> calling clk_enable() on clk->parent. The one thing I would expect you to
>>> have implemented ops wise is is_enabled, which you don't have.
>>> Also no sign of any rate acquisition functions, which I thought were
>>> mandatory.
>>>
>>> +	.get_parent =	clkctrl_guard_get_parent,
>>> +};
>>
>> Good point on .is_enabled, I indeed missed that. As for the rate acquisition
>> functions I referred to this table
>> https://docs.kernel.org/driver-api/clk.html#id4 , and it see that .set_rate
>> is actually optional.
> 
> .set_rate is not rate acquisition. .round_rate and .determine_rate are.
> I thought they were mandatory, but for a gate clock I guess they are not
> and the parent rate gets used automatically.

Before I send a v2 I'd like to clarify a few more things:
- I provided a schematics by means of the URL. I believe there's no 
unified way to provide something like that in the documentation, is 
there? So the only way to describe it properly would be to summarize the 
description from the mailing list, right?
- I'm going over the Common Clk Framework again, and perhaps I 
understood it wrong. You mentioned that I have to implement is_enabled, 
but I implemented is_prepared. It seems that I just have to move my 
is_prepared implementation to is_enabled. Does that sound correct?
- In my particular use case I don't need enable/disable ops, but to keep 
the driver generic, I'd probably want to have the bulk_enable 
implementation inside, because I don't know which clocks are assigned in 
a device tree. The clk_core_enable function only enables 1 parent clock, 
not the the list of parent clocks. Or I'm missing something here?

Thanks,
Slava

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Conor Dooley]

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

On Mon, Apr 20, 2026 at 07:56:31PM +0200, Vyacheslav Yurkov wrote:
> On 26.03.2026 11:44, Conor Dooley wrote:
> > On Thu, Mar 26, 2026 at 10:54:52AM +0100, Vyacheslav Yurkov wrote:
> > > On 23.03.2026 21:14, Conor Dooley wrote:
> > > 
> > > > 
> > > > The binding you've got says "GPIOs used to control or guard the clocks",
> > > > which is not what you're saying that is going on in this mail. A more
> > > > suitable description would be "GPIOs used to check the status of the
> > > > clocks".
> > > 
> > > Agree, the description I provided is not very accurate.
> > > 
> > > > I want to see an example dts user for this please.
> > > 
> > > DTS example:
> > >    clock_guard: clock_controller_guard {
> > >      compatible = "clock-controller-guard";
> > >      #clock-cells = <1>;
> > >      clocks = <&h2f_clk 0>, <&clk_fgpa_rx 0>, <clk_fpga_tx 0>;
> > 
> > Unfortunately, this doesn't contain the part that I wanted to see - who
> > the providers of these clocks here actually are.
> > 
> > To be frank, I am not sure how this block would know that these clocks
> > are enabled but their providers do not. I can think of a few ideas for
> > how this block would know, but I don't understand why the providers
> > themselves don't, and therefore why you need this gpio to tell you.
> > 
> > >      clock-names = "h2f_clk0", "clk_fpga_rx", "clk_fpga_tx";
> > >      gpios = <&fpga_ip 0 GPIO_ACTIVE_HIGH>, <&fpga_ip 1 GPIO_ACTIVE_HIGH>;
> > >      gpio-names = "gpio-input0", "gpio-input1";
> > >      clock-output-names = "clkctrl-guard";
> > >    };
> > > 
> > >    custom_device {
> > >      compatible = "...";
> > >      ...
> > >      #clock-cells = <1>;
> > >      clocks = <&clock_guard 0>;
> > >      clock-names = "clock-guard";
> > >    };
> > > 
> > > The driver usage exaple:
> > > 
> > > clk = devm_clk_get(dev, "clock-guard");
> > > if (IS_ERR(clk))
> > > 	return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> > > 
> > > ret = clk_prepare_enable(clk);
> > > if (ret) {
> > > 	dev_warn(dev, "Clock is not ready, %d\n", ret);
> > > 	return -EPROBE_DEFER;
> > > }
> > > 
> > > 
> > > > TBH, I don't understand your driver implementation either and why it has
> > > > 
> > > > +static const struct clk_ops clkctrl_guard_ops = {
> > > > 
> > > > +	.enable =	clkctrl_guard_enable,
> > > > +	.disable =	clkctrl_guard_disable,
> > > > +	.prepare =	clkctrl_guard_prepare,
> > > > +	.unprepare =	clkctrl_guard_unprepare,
> > > > +	.is_prepared =	clkctrl_guard_is_prepared,
> > > > 
> > > > any of these 4 implemented when you have no control over the clock.
> > > > I didn't think it was required to call your parent clocks enables in
> > > > your own enable either, thought that was handled by the core recursively
> > > > calling clk_enable() on clk->parent. The one thing I would expect you to
> > > > have implemented ops wise is is_enabled, which you don't have.
> > > > Also no sign of any rate acquisition functions, which I thought were
> > > > mandatory.
> > > > 
> > > > +	.get_parent =	clkctrl_guard_get_parent,
> > > > +};
> > > 
> > > Good point on .is_enabled, I indeed missed that. As for the rate acquisition
> > > functions I referred to this table
> > > https://docs.kernel.org/driver-api/clk.html#id4 , and it see that .set_rate
> > > is actually optional.
> > 
> > .set_rate is not rate acquisition. .round_rate and .determine_rate are.
> > I thought they were mandatory, but for a gate clock I guess they are not
> > and the parent rate gets used automatically.
> 
> Before I send a v2 I'd like to clarify a few more things:
> - I provided a schematics by means of the URL. I believe there's no unified
> way to provide something like that in the documentation, is there? So the
> only way to describe it properly would be to summarize the description from
> the mailing list, right?

I don't believe anything we have at the moment is what you're looking
for.

> - I'm going over the Common Clk Framework again, and perhaps I understood it
> wrong. You mentioned that I have to implement is_enabled, but I implemented
> is_prepared. It seems that I just have to move my is_prepared implementation
> to is_enabled. Does that sound correct?

Effectively yes, I think.

> - In my particular use case I don't need enable/disable ops, but to keep the
> driver generic, I'd probably want to have the bulk_enable implementation
> inside, because I don't know which clocks are assigned in a device tree. The

Why don't you know this? I'd expect there to be 1:1 mapping of gpios to
clocks, with an equal number of input and output clocks, since all
you're doing is detecting if the clocks are ready to go?

> clk_core_enable function only enables 1 parent clock, not the the list of
> parent clocks. Or I'm missing something here?


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

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Vyacheslav Yurkov]

On 21.04.2026 19:28, Conor Dooley wrote:

>> Before I send a v2 I'd like to clarify a few more things:
>> - I provided a schematics by means of the URL. I believe there's no unified
>> way to provide something like that in the documentation, is there? So the
>> only way to describe it properly would be to summarize the description from
>> the mailing list, right?
> 
> I don't believe anything we have at the moment is what you're looking
> for.
> 
>> - I'm going over the Common Clk Framework again, and perhaps I understood it
>> wrong. You mentioned that I have to implement is_enabled, but I implemented
>> is_prepared. It seems that I just have to move my is_prepared implementation
>> to is_enabled. Does that sound correct?
> 
> Effectively yes, I think.
> 
>> - In my particular use case I don't need enable/disable ops, but to keep the
>> driver generic, I'd probably want to have the bulk_enable implementation
>> inside, because I don't know which clocks are assigned in a device tree. The
> 
> Why don't you know this? I'd expect there to be 1:1 mapping of gpios to
> clocks, with an equal number of input and output clocks, since all
> you're doing is detecting if the clocks are ready to go?
> 
>> clk_core_enable function only enables 1 parent clock, not the the list of
>> parent clocks. Or I'm missing something here?
> 

Thanks for your support. Yes, I talked to the HW team and I have this 
information.

One last important bit, which I'm trying to figure out, is how to notify 
the users of the driver about the state change. I understand that Common 
Clock Framework doesn't support clocks drifting to unlocked state, and 
I'm OK with this limitation. Right now what happens on the clock 
consumer side is that it gets -EPROBE_DEFER when any providers are not 
there or not initialized. But if the state of GPIO is not the expected 
one, then -EBUSY is propagated to the probe of the dependent driver. I 
can also change EBUSY to EPROBE_DEFER, but how to trigger the deferred 
probe again is something I don't know. The only alternative I can think 
of is a call to rmmod / insmod from the userspace.

Is there any other way to achieve this?

Slava

Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Conor Dooley]

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

On Tue, Apr 28, 2026 at 12:13:41PM +0200, Vyacheslav Yurkov wrote:
> On 21.04.2026 19:28, Conor Dooley wrote:
> 
> > > Before I send a v2 I'd like to clarify a few more things:
> > > - I provided a schematics by means of the URL. I believe there's no unified
> > > way to provide something like that in the documentation, is there? So the
> > > only way to describe it properly would be to summarize the description from
> > > the mailing list, right?
> > 
> > I don't believe anything we have at the moment is what you're looking
> > for.
> > 
> > > - I'm going over the Common Clk Framework again, and perhaps I understood it
> > > wrong. You mentioned that I have to implement is_enabled, but I implemented
> > > is_prepared. It seems that I just have to move my is_prepared implementation
> > > to is_enabled. Does that sound correct?
> > 
> > Effectively yes, I think.
> > 
> > > - In my particular use case I don't need enable/disable ops, but to keep the
> > > driver generic, I'd probably want to have the bulk_enable implementation
> > > inside, because I don't know which clocks are assigned in a device tree. The
> > 
> > Why don't you know this? I'd expect there to be 1:1 mapping of gpios to
> > clocks, with an equal number of input and output clocks, since all
> > you're doing is detecting if the clocks are ready to go?
> > 
> > > clk_core_enable function only enables 1 parent clock, not the the list of
> > > parent clocks. Or I'm missing something here?
> > 
> 
> Thanks for your support. Yes, I talked to the HW team and I have this
> information.
> 
> One last important bit, which I'm trying to figure out, is how to notify the
> users of the driver about the state change. I understand that Common Clock
> Framework doesn't support clocks drifting to unlocked state, and I'm OK with
> this limitation. Right now what happens on the clock consumer side is that
> it gets -EPROBE_DEFER when any providers are not there or not initialized.
> But if the state of GPIO is not the expected one, then -EBUSY is propagated
> to the probe of the dependent driver. I can also change EBUSY to
> EPROBE_DEFER, but how to trigger the deferred probe again is something I
> don't know. The only alternative I can think of is a call to rmmod / insmod
> from the userspace.
> 
> Is there any other way to achieve this?

I don't know, that's a question for the clock subsystem folks.

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