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

[Conor Dooley <conor@kernel.org>]

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