* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description [not found] ` <20260319-yearly-wrongful-883f7fd86a69@spud> @ 2026-03-23 13:52 ` Vyacheslav Yurkov 2026-03-23 20:14 ` Conor Dooley 0 siblings, 1 reply; 14+ messages in thread From: Vyacheslav Yurkov @ 2026-03-23 13:52 UTC (permalink / raw) To: Conor Dooley Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-03-23 13:52 ` [PATCH 2/2] dt-bindings: Add clock guard DT description Vyacheslav Yurkov @ 2026-03-23 20:14 ` Conor Dooley 2026-03-26 9:54 ` Vyacheslav Yurkov 0 siblings, 1 reply; 14+ messages in thread From: Conor Dooley @ 2026-03-23 20:14 UTC (permalink / raw) To: Vyacheslav Yurkov Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-03-23 20:14 ` Conor Dooley @ 2026-03-26 9:54 ` Vyacheslav Yurkov 2026-03-26 10:08 ` Krzysztof Kozlowski 2026-03-26 10:44 ` Conor Dooley 0 siblings, 2 replies; 14+ messages in thread From: Vyacheslav Yurkov @ 2026-03-26 9:54 UTC (permalink / raw) To: Conor Dooley Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-03-26 9:54 ` Vyacheslav Yurkov @ 2026-03-26 10:08 ` Krzysztof Kozlowski 2026-03-26 13:39 ` Vyacheslav Yurkov 2026-03-26 10:44 ` Conor Dooley 1 sibling, 1 reply; 14+ messages in thread From: Krzysztof Kozlowski @ 2026-03-26 10:08 UTC (permalink / raw) To: Vyacheslav Yurkov, Conor Dooley Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-03-26 10:08 ` Krzysztof Kozlowski @ 2026-03-26 13:39 ` Vyacheslav Yurkov 2026-03-26 13:49 ` Krzysztof Kozlowski 2026-03-26 18:32 ` Conor Dooley 0 siblings, 2 replies; 14+ messages in thread From: Vyacheslav Yurkov @ 2026-03-26 13:39 UTC (permalink / raw) To: Krzysztof Kozlowski, Conor Dooley Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-03-26 13:39 ` Vyacheslav Yurkov @ 2026-03-26 13:49 ` Krzysztof Kozlowski 2026-03-26 18:32 ` Conor Dooley 1 sibling, 0 replies; 14+ messages in thread From: Krzysztof Kozlowski @ 2026-03-26 13:49 UTC (permalink / raw) To: Vyacheslav Yurkov, Conor Dooley Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-03-26 13:39 ` Vyacheslav Yurkov 2026-03-26 13:49 ` Krzysztof Kozlowski @ 2026-03-26 18:32 ` Conor Dooley 2026-03-28 2:58 ` Vyacheslav Yurkov 1 sibling, 1 reply; 14+ messages in thread From: Conor Dooley @ 2026-03-26 18:32 UTC (permalink / raw) To: Vyacheslav Yurkov Cc: Krzysztof Kozlowski, Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-03-26 18:32 ` Conor Dooley @ 2026-03-28 2:58 ` Vyacheslav Yurkov 2026-04-07 16:17 ` Conor Dooley 0 siblings, 1 reply; 14+ messages in thread From: Vyacheslav Yurkov @ 2026-03-28 2:58 UTC (permalink / raw) To: Conor Dooley Cc: Krzysztof Kozlowski, Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-03-28 2:58 ` Vyacheslav Yurkov @ 2026-04-07 16:17 ` Conor Dooley 0 siblings, 0 replies; 14+ messages in thread From: Conor Dooley @ 2026-04-07 16:17 UTC (permalink / raw) To: Vyacheslav Yurkov Cc: Krzysztof Kozlowski, Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-03-26 9:54 ` Vyacheslav Yurkov 2026-03-26 10:08 ` Krzysztof Kozlowski @ 2026-03-26 10:44 ` Conor Dooley 2026-04-20 17:56 ` Vyacheslav Yurkov 1 sibling, 1 reply; 14+ messages in thread From: Conor Dooley @ 2026-03-26 10:44 UTC (permalink / raw) To: Vyacheslav Yurkov Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-03-26 10:44 ` Conor Dooley @ 2026-04-20 17:56 ` Vyacheslav Yurkov 2026-04-21 17:28 ` Conor Dooley 0 siblings, 1 reply; 14+ messages in thread From: Vyacheslav Yurkov @ 2026-04-20 17:56 UTC (permalink / raw) To: Conor Dooley Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-04-20 17:56 ` Vyacheslav Yurkov @ 2026-04-21 17:28 ` Conor Dooley 2026-04-28 10:13 ` Vyacheslav Yurkov 0 siblings, 1 reply; 14+ messages in thread From: Conor Dooley @ 2026-04-21 17:28 UTC (permalink / raw) To: Vyacheslav Yurkov Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-04-21 17:28 ` Conor Dooley @ 2026-04-28 10:13 ` Vyacheslav Yurkov 2026-05-09 18:22 ` Conor Dooley 0 siblings, 1 reply; 14+ messages in thread From: Vyacheslav Yurkov @ 2026-04-28 10:13 UTC (permalink / raw) To: Conor Dooley Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] dt-bindings: Add clock guard DT description 2026-04-28 10:13 ` Vyacheslav Yurkov @ 2026-05-09 18:22 ` Conor Dooley 0 siblings, 0 replies; 14+ messages in thread From: Conor Dooley @ 2026-05-09 18:22 UTC (permalink / raw) To: Vyacheslav Yurkov Cc: Rob Herring, Vyacheslav Yurkov, Michael Turquette, Stephen Boyd, Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk, devicetree [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-05-09 18:22 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260318-feature-clock-guard-v1-0-6137cb4084b7@bruker.com>
[not found] ` <20260318-feature-clock-guard-v1-2-6137cb4084b7@bruker.com>
[not found] ` <20260318225510.GA639444-robh@kernel.org>
[not found] ` <7c7034a7-686a-42c2-bdba-6f31b5179f7c@gmail.com>
[not found] ` <20260319-yearly-wrongful-883f7fd86a69@spud>
2026-03-23 13:52 ` [PATCH 2/2] dt-bindings: Add clock guard DT description Vyacheslav Yurkov
2026-03-23 20:14 ` Conor Dooley
2026-03-26 9:54 ` Vyacheslav Yurkov
2026-03-26 10:08 ` Krzysztof Kozlowski
2026-03-26 13:39 ` Vyacheslav Yurkov
2026-03-26 13:49 ` Krzysztof Kozlowski
2026-03-26 18:32 ` Conor Dooley
2026-03-28 2:58 ` Vyacheslav Yurkov
2026-04-07 16:17 ` Conor Dooley
2026-03-26 10:44 ` Conor Dooley
2026-04-20 17:56 ` Vyacheslav Yurkov
2026-04-21 17:28 ` Conor Dooley
2026-04-28 10:13 ` Vyacheslav Yurkov
2026-05-09 18:22 ` Conor Dooley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox