From: Alex Elder <elder@kernel.org>
Allow a clock to specify a "prerequisite" clock, identified by its
name. The prerequisite clock must be prepared and enabled before a
clock that depends on it is used. In order to simplify locking, we
require a clock and its prerequisite to be associated with the same
CCU. (We'll just trust--but not verify--that nobody defines a cycle
of prerequisite clocks.)
Rework the KONA_CLK() macro, and define a new KONA_CLK_PREREQ()
variant that allows a prerequisite clock to be specified.
Signed-off-by: Alex Elder <elder@linaro.org>
--- Artur: rebase on v6.13, move prereq prepare/unprepare to main
prepare/unprepare functions, use locking versions of clk_prepare
and clk_enable since the non-locking versions are no longer
public ---
Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
drivers/clk/bcm/clk-kona.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/clk/bcm/clk-kona.h | 20 ++++++++++++---
2 files changed, 79 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index e92d57f3bbb147e72221802175a80502897d7504..21f925683d0da05ebc97f92236dfb207b1f9c741 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -9,6 +9,7 @@
#include <linux/delay.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/clk.h>
#include <linux/clk-provider.h>
/*
@@ -961,6 +962,63 @@ static int selector_write(struct ccu_data *ccu, struct bcm_clk_gate *gate,
return ret;
}
+/*
+ * Common clock prepare/unprepare functions. These implement a "prerequisite"
+ * mechanism; the prerequisite clock is prepared and enabled before the main
+ * clock is prepared.
+ */
+
+static int kona_clk_prepare(struct clk_hw *hw)
+{
+ struct kona_clk *bcm_clk = to_kona_clk(hw);
+ const char *clk_name = bcm_clk->init_data.name;
+ const char *prereq_name = bcm_clk->prereq.name;
+ struct clk *prereq_clk = bcm_clk->prereq.clk;
+ int ret;
+
+ /* If there's no prerequisite clock, there's nothing to do */
+ if (!prereq_name)
+ return 0;
+
+ /* Look up the prerequisite clock if we haven't already */
+ if (!prereq_clk) {
+ prereq_clk = __clk_lookup(prereq_name);
+ if (WARN_ON_ONCE(!prereq_clk))
+ return -ENOENT;
+ bcm_clk->prereq.clk = prereq_clk;
+ }
+
+ ret = clk_prepare(prereq_clk);
+ if (ret) {
+ pr_err("%s: unable to prepare prereq clock %s for %s\n",
+ __func__, prereq_name, clk_name);
+ return ret;
+ }
+
+ ret = clk_enable(prereq_clk);
+ if (ret) {
+ clk_unprepare(prereq_clk);
+ pr_err("%s: unable to enable prereq clock %s for %s\n",
+ __func__, prereq_name, clk_name);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void kona_clk_unprepare(struct clk_hw *hw)
+{
+ struct kona_clk *bcm_clk = to_kona_clk(hw);
+ struct clk *prereq_clk = bcm_clk->prereq.clk;
+
+ /* If there's no prerequisite clock, there's nothing to do */
+ if (!bcm_clk->prereq.name)
+ return;
+
+ clk_disable(prereq_clk);
+ clk_unprepare(prereq_clk);
+}
+
/* Peripheral clock operations */
static int kona_peri_clk_enable(struct clk_hw *hw)
@@ -1172,6 +1230,8 @@ static int kona_peri_clk_set_rate(struct clk_hw *hw, unsigned long rate,
}
struct clk_ops kona_peri_clk_ops = {
+ .prepare = kona_clk_prepare,
+ .unprepare = kona_clk_unprepare,
.enable = kona_peri_clk_enable,
.disable = kona_peri_clk_disable,
.is_enabled = kona_peri_clk_is_enabled,
@@ -1260,6 +1320,8 @@ static int kona_bus_clk_is_enabled(struct clk_hw *hw)
}
struct clk_ops kona_bus_clk_ops = {
+ .prepare = kona_clk_prepare,
+ .unprepare = kona_clk_unprepare,
.enable = kona_bus_clk_enable,
.disable = kona_bus_clk_disable,
.is_enabled = kona_bus_clk_is_enabled,
diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
index a5b3d8bdb54eaee9fad80c28796170207b817dfd..c32c621282ec6dd40fff3f7598ee8aa007fed524 100644
--- a/drivers/clk/bcm/clk-kona.h
+++ b/drivers/clk/bcm/clk-kona.h
@@ -406,6 +406,10 @@ struct kona_clk {
struct clk_init_data init_data; /* includes name of this clock */
struct ccu_data *ccu; /* ccu this clock is associated with */
enum bcm_clk_type type;
+ struct {
+ const char *name;
+ struct clk *clk;
+ } prereq;
union {
void *data;
struct peri_clk_data *peri;
@@ -416,16 +420,26 @@ struct kona_clk {
container_of(_hw, struct kona_clk, hw)
/* Initialization macro for an entry in a CCU's kona_clks[] array. */
-#define KONA_CLK(_ccu_name, _clk_name, _type) \
- { \
+#define __KONA_CLK_COMMON(_ccu_name, _clk_name, _type) \
.init_data = { \
.name = #_clk_name, \
.ops = &kona_ ## _type ## _clk_ops, \
}, \
.ccu = &_ccu_name ## _ccu_data, \
.type = bcm_clk_ ## _type, \
- .u.data = &_clk_name ## _data, \
+ .u.data = &_clk_name ## _data
+
+#define KONA_CLK(_ccu_name, _clk_name, _type) \
+ { \
+ __KONA_CLK_COMMON(_ccu_name, _clk_name, _type), \
}
+
+#define KONA_CLK_PREREQ(_ccu_name, _clk_name, _type, _prereq) \
+ { \
+ .prereq.name = #_prereq, \
+ __KONA_CLK_COMMON(_ccu_name, _clk_name, _type), \
+ }
+
#define LAST_KONA_CLK { .type = bcm_clk_none }
/*
--
2.48.1On 2/16/25 10:12 AM, Artur Weber wrote:
> From: Alex Elder <elder@kernel.org>
>
> Allow a clock to specify a "prerequisite" clock, identified by its
> name. The prerequisite clock must be prepared and enabled before a
> clock that depends on it is used. In order to simplify locking, we
> require a clock and its prerequisite to be associated with the same
> CCU. (We'll just trust--but not verify--that nobody defines a cycle
> of prerequisite clocks.)
>
> Rework the KONA_CLK() macro, and define a new KONA_CLK_PREREQ()
> variant that allows a prerequisite clock to be specified.
>
> Signed-off-by: Alex Elder <elder@linaro.org>
> --- Artur: rebase on v6.13, move prereq prepare/unprepare to main
> prepare/unprepare functions, use locking versions of clk_prepare
> and clk_enable since the non-locking versions are no longer
> public ---
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
I'm surprised there is no prepare function for the peripheral
clocks.
The prequisite clock should separate the prepare and the
enable functionality. Right now you have kona_clk_prepare()
doing both. Instead, a clock's prepare function should
prepare its prerequisite (if any). Then its enable function
should enable its parent.
Should all the users of peripheral clocks just also be required
to specify the bus clocks as well? I suppose that doesn't
encode the prerequisite property (bus comes before peripheral);
is that truly a requirement?
-Alex
> ---
> drivers/clk/bcm/clk-kona.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++
> drivers/clk/bcm/clk-kona.h | 20 ++++++++++++---
> 2 files changed, 79 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> index e92d57f3bbb147e72221802175a80502897d7504..21f925683d0da05ebc97f92236dfb207b1f9c741 100644
> --- a/drivers/clk/bcm/clk-kona.c
> +++ b/drivers/clk/bcm/clk-kona.c
> @@ -9,6 +9,7 @@
> #include <linux/delay.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> +#include <linux/clk.h>
> #include <linux/clk-provider.h>
>
> /*
> @@ -961,6 +962,63 @@ static int selector_write(struct ccu_data *ccu, struct bcm_clk_gate *gate,
> return ret;
> }
>
> +/*
> + * Common clock prepare/unprepare functions. These implement a "prerequisite"
> + * mechanism; the prerequisite clock is prepared and enabled before the main
> + * clock is prepared.
> + */
> +
> +static int kona_clk_prepare(struct clk_hw *hw)
> +{
> + struct kona_clk *bcm_clk = to_kona_clk(hw);
> + const char *clk_name = bcm_clk->init_data.name;
> + const char *prereq_name = bcm_clk->prereq.name;
> + struct clk *prereq_clk = bcm_clk->prereq.clk;
> + int ret;
> +
> + /* If there's no prerequisite clock, there's nothing to do */
> + if (!prereq_name)
> + return 0;
> +
> + /* Look up the prerequisite clock if we haven't already */
> + if (!prereq_clk) {
> + prereq_clk = __clk_lookup(prereq_name);
> + if (WARN_ON_ONCE(!prereq_clk))
> + return -ENOENT;
> + bcm_clk->prereq.clk = prereq_clk;
> + }
> +
> + ret = clk_prepare(prereq_clk);
> + if (ret) {
> + pr_err("%s: unable to prepare prereq clock %s for %s\n",
> + __func__, prereq_name, clk_name);
> + return ret;
> + }
> +
> + ret = clk_enable(prereq_clk);
> + if (ret) {
> + clk_unprepare(prereq_clk);
> + pr_err("%s: unable to enable prereq clock %s for %s\n",
> + __func__, prereq_name, clk_name);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void kona_clk_unprepare(struct clk_hw *hw)
> +{
> + struct kona_clk *bcm_clk = to_kona_clk(hw);
> + struct clk *prereq_clk = bcm_clk->prereq.clk;
> +
> + /* If there's no prerequisite clock, there's nothing to do */
> + if (!bcm_clk->prereq.name)
> + return;
> +
> + clk_disable(prereq_clk);
> + clk_unprepare(prereq_clk);
> +}
> +
> /* Peripheral clock operations */
>
> static int kona_peri_clk_enable(struct clk_hw *hw)
> @@ -1172,6 +1230,8 @@ static int kona_peri_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> }
>
> struct clk_ops kona_peri_clk_ops = {
> + .prepare = kona_clk_prepare,
> + .unprepare = kona_clk_unprepare,
> .enable = kona_peri_clk_enable,
> .disable = kona_peri_clk_disable,
> .is_enabled = kona_peri_clk_is_enabled,
> @@ -1260,6 +1320,8 @@ static int kona_bus_clk_is_enabled(struct clk_hw *hw)
> }
>
> struct clk_ops kona_bus_clk_ops = {
> + .prepare = kona_clk_prepare,
> + .unprepare = kona_clk_unprepare,
> .enable = kona_bus_clk_enable,
> .disable = kona_bus_clk_disable,
> .is_enabled = kona_bus_clk_is_enabled,
> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
> index a5b3d8bdb54eaee9fad80c28796170207b817dfd..c32c621282ec6dd40fff3f7598ee8aa007fed524 100644
> --- a/drivers/clk/bcm/clk-kona.h
> +++ b/drivers/clk/bcm/clk-kona.h
> @@ -406,6 +406,10 @@ struct kona_clk {
> struct clk_init_data init_data; /* includes name of this clock */
> struct ccu_data *ccu; /* ccu this clock is associated with */
> enum bcm_clk_type type;
> + struct {
> + const char *name;
> + struct clk *clk;
> + } prereq;
> union {
> void *data;
> struct peri_clk_data *peri;
> @@ -416,16 +420,26 @@ struct kona_clk {
> container_of(_hw, struct kona_clk, hw)
>
> /* Initialization macro for an entry in a CCU's kona_clks[] array. */
> -#define KONA_CLK(_ccu_name, _clk_name, _type) \
> - { \
> +#define __KONA_CLK_COMMON(_ccu_name, _clk_name, _type) \
> .init_data = { \
> .name = #_clk_name, \
> .ops = &kona_ ## _type ## _clk_ops, \
> }, \
> .ccu = &_ccu_name ## _ccu_data, \
> .type = bcm_clk_ ## _type, \
> - .u.data = &_clk_name ## _data, \
> + .u.data = &_clk_name ## _data
> +
> +#define KONA_CLK(_ccu_name, _clk_name, _type) \
> + { \
> + __KONA_CLK_COMMON(_ccu_name, _clk_name, _type), \
> }
> +
> +#define KONA_CLK_PREREQ(_ccu_name, _clk_name, _type, _prereq) \
> + { \
> + .prereq.name = #_prereq, \
> + __KONA_CLK_COMMON(_ccu_name, _clk_name, _type), \
> + }
> +
> #define LAST_KONA_CLK { .type = bcm_clk_none }
>
> /*
>
On 24.02.2025 17:20, Alex Elder wrote: > On 2/16/25 10:12 AM, Artur Weber wrote: >> From: Alex Elder <elder@kernel.org> >> >> Allow a clock to specify a "prerequisite" clock, identified by its >> name. The prerequisite clock must be prepared and enabled before a >> clock that depends on it is used. In order to simplify locking, we >> require a clock and its prerequisite to be associated with the same >> CCU. (We'll just trust--but not verify--that nobody defines a cycle >> of prerequisite clocks.) >> >> Rework the KONA_CLK() macro, and define a new KONA_CLK_PREREQ() >> variant that allows a prerequisite clock to be specified. >> >> Signed-off-by: Alex Elder <elder@linaro.org> >> --- Artur: rebase on v6.13, move prereq prepare/unprepare to main >> prepare/unprepare functions, use locking versions of clk_prepare >> and clk_enable since the non-locking versions are no longer >> public --- >> Signed-off-by: Artur Weber <aweber.kernel@gmail.com> > > I'm surprised there is no prepare function for the peripheral > clocks. Not sure I follow - there is a prepare function for peri clocks, the same one as for all Kona clocks - kona_clk_prepare. Though it only enables the prereq clock... I assume you mean "prepare function specific to peripheral clocks", in which case - what should a prepare function specifically for peripheral clocks do? As I mentioned in the cover letter, clock initialization has to be done explicitly at clock init time, since it's required for later operations like setting the clock rate/parent that are done *before* the first call to the prepare function happens; otherwise we get errors like this: [ 4.740000] [ T39] kona_peri_clk_set_parent: trigger failed for sdio1 [ 4.760000] [ T39] kona_peri_clk_set_rate: gating failure for sdio1 [ 5.650000] [ T36] kona_peri_clk_set_parent: trigger failed for sdio3 [ 5.670000] [ T36] kona_peri_clk_set_rate: gating failure for sdio3 (I did consider moving the relevant clock initialization to a "clk_ops.init" function, but left it out of this patchset for brevity. Might consider actually doing that...) > The prequisite clock should separate the prepare and the > enable functionality. Right now you have kona_clk_prepare() > doing both. Instead, a clock's prepare function should > prepare its prerequisite (if any). Then its enable function > should enable its parent. I copied this behavior from the original patch; there the prereq_prepare function both prepared and enabled the relevant prerequisite clock. Indeed doing each step (prepare/enable/disable/ unprepare) in the relevant functions would probably make more sense. > Should all the users of peripheral clocks just also be required > to specify the bus clocks as well? I suppose that doesn't > encode the prerequisite property (bus comes before peripheral); > is that truly a requirement? I see a few problems with that: - Most drivers only take one clock - that clock is pretty much always the peripheral clock (with the only exception being usb_otg_ahb which is used for the USB OTG controller). - Even if they supported both clocks, they would likely just switch them on/off at the same time as the equivalent peri clock. I mostly figured a clock dependency mechanism would work here since it's what the vendor kernel does - it allows for specifying a dependent clock and enables/disables it whenever the clock with the dependency is enabled/disabled. An alternative option would be to handle the dependency in the device tree, using "simple-pm-bus" nodes, either: - Wrapping around each node for a subdevice that uses a peripheral clock. This is rather unwieldy since it means a lot of subnodes for the most basic of peripherals (sdio1-4, uartb(2,3), etc.). (Also, managing the "ranges" properties for all of these sub-busses would get annoying unless we specify empty ranges, which according to the bindings "is only appropriate if the child node is another 'simple- bus' or similar."[1].) - Using the simple-bus nodes that the BCM2166x DTSI already uses, and connecting the relevant bus clock for all the components to them, and switching the compatible to "simple-pm-bus". It's less granular, but is probably the most sensible option if we go the DT route. As for whether enabling a bus clock before a peripheral clock is required... I went ahead and tested it, and the results seem to hint that it's not. - I seem to remember that not having the bus clock initialized caused some failures on peri clocks, but I can't reproduce this anymore. Most likely I'm mistaking it for policy bit setup, which is required for gating to work (I have another commit ready to add policy bits to all BCM21664 clocks, which I'm planning to send after this patchset). And besides, just initializing bus clocks after peri clocks seems to work given that's how it's set up right now. - Also, it seems that all clocks defined with HW_SW_GATE are gated on by default, and this state is never cleared afterwards, so all the bus clocks I added in the BCM21664 bus clock commit are already enabled at startup... after the peri clocks. And given that everything works fine, it's probably OK. Now that I think about it - maybe it would be easier to just keep things the way they are; drop the prerequisite clock mechanism, let the driver enable bus clocks by default, then if we wanted to explicitly control the bus clocks, use the second DT approach I mentioned. If we ever want to squeeze every last bit of power savings or for any other potential benefit of keeping bus clocks off, we can revisit this mechanism later. Best regards Artur [1] https://github.com/devicetree-org/dt-schema/blob/ed9190d20f146d13e262cc9138506326f7d4da91/dtschema/schemas/simple-bus.yaml#L60-L69
On 2/25/25 12:48 PM, Artur Weber wrote: > On 24.02.2025 17:20, Alex Elder wrote: >> On 2/16/25 10:12 AM, Artur Weber wrote: >>> From: Alex Elder <elder@kernel.org> >>> >>> Allow a clock to specify a "prerequisite" clock, identified by its >>> name. The prerequisite clock must be prepared and enabled before a >>> clock that depends on it is used. In order to simplify locking, we >>> require a clock and its prerequisite to be associated with the same >>> CCU. (We'll just trust--but not verify--that nobody defines a cycle >>> of prerequisite clocks.) >>> >>> Rework the KONA_CLK() macro, and define a new KONA_CLK_PREREQ() >>> variant that allows a prerequisite clock to be specified. >>> >>> Signed-off-by: Alex Elder <elder@linaro.org> >>> --- Artur: rebase on v6.13, move prereq prepare/unprepare to main >>> prepare/unprepare functions, use locking versions of clk_prepare >>> and clk_enable since the non-locking versions are no longer >>> public --- >>> Signed-off-by: Artur Weber <aweber.kernel@gmail.com> >> >> I'm surprised there is no prepare function for the peripheral >> clocks. > > Not sure I follow - there is a prepare function for peri clocks, the I meant *before* your patch, there is no prepare function. > same one as for all Kona clocks - kona_clk_prepare. Though it only > enables the prereq clock... I assume you mean "prepare function specific > to peripheral clocks", in which case - what should a prepare function > specifically for peripheral clocks do? It's just that the prepare function can block, but the enable function cannot. I see __ccu_wait_bit() just loops until the register indicates an update completed, so I guess there's no blocking. > As I mentioned in the cover letter, clock initialization has to be done > explicitly at clock init time, since it's required for later operations > like setting the clock rate/parent that are done *before* the first call > to the prepare function happens; otherwise we get errors like this: > > [ 4.740000] [ T39] kona_peri_clk_set_parent: trigger failed for sdio1 > [ 4.760000] [ T39] kona_peri_clk_set_rate: gating failure for sdio1 > [ 5.650000] [ T36] kona_peri_clk_set_parent: trigger failed for sdio3 > [ 5.670000] [ T36] kona_peri_clk_set_rate: gating failure for sdio3 > > (I did consider moving the relevant clock initialization to a > "clk_ops.init" function, but left it out of this patchset for brevity. > Might consider actually doing that...) > >> The prequisite clock should separate the prepare and the >> enable functionality. Right now you have kona_clk_prepare() >> doing both. Instead, a clock's prepare function should >> prepare its prerequisite (if any). Then its enable function >> should enable its parent. > > I copied this behavior from the original patch; there the > prereq_prepare function both prepared and enabled the relevant > prerequisite clock. Indeed doing each step (prepare/enable/disable/ > unprepare) in the relevant functions would probably make more sense. I hadn't worked on a clock driver before then, and I think I got pulled off that project before this series could be accepted (or even reviewed?). Maybe I just proposed what the vendor kernel did. In any case, *today* I'd say they should be separated. >> Should all the users of peripheral clocks just also be required >> to specify the bus clocks as well? I suppose that doesn't >> encode the prerequisite property (bus comes before peripheral); >> is that truly a requirement? > > I see a few problems with that: > > - Most drivers only take one clock - that clock is pretty much always > the peripheral clock (with the only exception being usb_otg_ahb which > is used for the USB OTG controller). > - Even if they supported both clocks, they would likely just switch them > on/off at the same time as the equivalent peri clock. > > I mostly figured a clock dependency mechanism would work here since it's > what the vendor kernel does - it allows for specifying a dependent clock > and enables/disables it whenever the clock with the dependency is > enabled/disabled. I don't really expect that the bus clock *must* be enabled before the peripheral clock. The requirement is probably just that both must be (prepared and) enabled before using the peripheral, and order doesn't matter. And in that case there's no need to enforce the ordering. > An alternative option would be to handle the dependency in the device > tree, using "simple-pm-bus" nodes, either: > > - Wrapping around each node for a subdevice that uses a peripheral > clock. This is rather unwieldy since it means a lot of subnodes for > the most basic of peripherals (sdio1-4, uartb(2,3), etc.). (Also, > managing the "ranges" properties for all of these sub-busses would > get annoying unless we specify empty ranges, which according to the > bindings "is only appropriate if the child node is another 'simple- > bus' or similar."[1].) The above sounds wrong to me. > - Using the simple-bus nodes that the BCM2166x DTSI already uses, and > connecting the relevant bus clock for all the components to them, > and switching the compatible to "simple-pm-bus". It's less granular, > but is probably the most sensible option if we go the DT route. > > As for whether enabling a bus clock before a peripheral clock is > required... I went ahead and tested it, and the results seem to hint > that it's not. > > - I seem to remember that not having the bus clock initialized caused > some failures on peri clocks, but I can't reproduce this anymore. > Most likely I'm mistaking it for policy bit setup, which is required > for gating to work (I have another commit ready to add policy bits > to all BCM21664 clocks, which I'm planning to send after this > patchset). And besides, just initializing bus clocks after peri > clocks seems to work given that's how it's set up right now. > > - Also, it seems that all clocks defined with HW_SW_GATE are gated > on by default, and this state is never cleared afterwards, so all the > bus clocks I added in the BCM21664 bus clock commit are already > enabled at startup... after the peri clocks. And given that everything > works fine, it's probably OK. > > Now that I think about it - maybe it would be easier to just keep things > the way they are; drop the prerequisite clock mechanism, let the > driver enable bus clocks by default, then if we wanted to explicitly > control the bus clocks, use the second DT approach I mentioned. If we > ever want to squeeze every last bit of power savings or for any other > potential benefit of keeping bus clocks off, we can revisit this > mechanism later. I'm far out of the loop on the Broadcom drivers. If the hardware requires no actual hierarchy/ordering of these clocks, then none should be implied (by DT, or by the code). And if that's the case, having the peripherals depend on both the bus and peripheral clocks probably makes sense. -Alex > > Best regards > Artur > > [1] https://github.com/devicetree-org/dt-schema/blob/ > ed9190d20f146d13e262cc9138506326f7d4da91/dtschema/schemas/simple- > bus.yaml#L60-L69
© 2016 - 2025 Red Hat, Inc.