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

[Conor Dooley <conor@kernel.org>]

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