* [PATCH v4] clk: Introduce 'always-on-clocks' property
@ 2022-09-24 17:45 Marek Vasut
2022-10-04 18:26 ` Stephen Boyd
0 siblings, 1 reply; 3+ messages in thread
From: Marek Vasut @ 2022-09-24 17:45 UTC (permalink / raw
To: linux-clk
Cc: Marek Vasut, Andrew Morton, Michael Turquette, Rob Herring,
Stephen Boyd, devicetree
Some platforms require select clock to be always running, e.g. because
those clock supply vital devices which are not otherwise attached to
the system and thus do not have a matching DT node and clock consumer.
An example is a system where the SoC serves as a crystal oscillator
replacement for a programmable logic device. The "always-on-clocks"
property of a clock controller allows listing clock which must never
be turned off.
Clock listed in the "always-on-clocks" property may have other consumers
in DT, listing the clock in "always-on-clocks" only assures those clock
are never turned off, and none of these optional additional consumers
can turn the clock off either. This is achieved by adding CLK_IS_CRITICAL
flag to these critical clock.
This flag has thus far been added to select clock by hard-coding it in
various clock drivers, this patch provides generic DT interface to add
the flag to arbitrary clock that may be critical.
The implementation is modeled after "protected-clocks", except the protected
clock property is currently driver specific. This patch attempts to provide
a generic implementation of "always-on-clocks" instead.
Unlike "assigned-clocks", the "always-on-clocks" must be parsed much earlier
in __clk_register() to assign CLK_IS_CRITICAL flag to clk_init_data .flags
field.
The new match_clkspec() callback is used to determine whether struct clk_hw
that is currently being registered matches the clock specifier in the DT
"always-on-clocks" property, and if so, then the CLK_IS_CRITICAL is added to
these newly registered clock. This callback can only be driver specific.
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: devicetree@vger.kernel.org
To: linux-clk@vger.kernel.org
---
V2: - Warn in case critical-clock field cannot be parsed and skip those clock
- Use match_clkspec() only for non-zero clock-cells controllers
- Pull the critical-clock code into __clk_register_critical_clock()
- Update commit message
V3: - Pick np from clk_core->of_node
V4: - Rename DT property critical-clocks to always-on-clocks
---
drivers/clk/clk.c | 44 ++++++++++++++++++++++++++++++++++++
include/linux/clk-provider.h | 3 +++
2 files changed, 47 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b70769d0db99f..6b07f1a086277 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3900,6 +3900,48 @@ static void clk_core_free_parent_map(struct clk_core *core)
kfree(core->parents);
}
+static void
+__clk_register_critical_clock(struct clk_core *core, struct clk_hw *hw)
+{
+ struct device_node *np = core->of_node;
+ struct of_phandle_args clkspec;
+ u32 clksize, clktotal;
+ int ret, i, index;
+
+ if (!np)
+ return;
+
+ if (of_property_read_u32(np, "#clock-cells", &clksize))
+ return;
+
+ /* Clock node with #clock-cells = <0> uses always-on-clocks; */
+ if (clksize == 0) {
+ if (of_property_read_bool(np, "always-on-clocks"))
+ core->flags |= CLK_IS_CRITICAL;
+ return;
+ }
+
+ if (!core->ops->match_clkspec)
+ return;
+
+ clkspec.np = np;
+ clktotal = of_property_count_u32_elems(np, "always-on-clocks");
+ clktotal /= clksize;
+ for (index = 0; index < clktotal; index++) {
+ for (i = 0; i < clksize; i++) {
+ ret = of_property_read_u32_index(np, "always-on-clocks",
+ (index * clksize) + i,
+ &(clkspec.args[i]));
+ if (ret) {
+ pr_warn("Skipping always-on-clocks index %d (ret=%d)\n",
+ i, ret);
+ }
+ }
+ if (!core->ops->match_clkspec(hw, &clkspec))
+ core->flags |= CLK_IS_CRITICAL;
+ }
+}
+
static struct clk *
__clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
{
@@ -3944,6 +3986,8 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
core->min_rate = 0;
core->max_rate = ULONG_MAX;
+ __clk_register_critical_clock(core, hw);
+
ret = clk_core_populate_parent_map(core, init);
if (ret)
goto fail_parents;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index dec1bcae43790..e02a66dc482be 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -205,6 +205,8 @@ struct clk_duty {
* directory is provided as an argument. Called with
* prepare_lock held. Returns 0 on success, -EERROR otherwise.
*
+ * @match_clkspec: Check whether clk_hw matches DT clock specifier.
+ * Returns 0 on success, -EERROR otherwise.
*
* The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
* implementations to split any work between atomic (enable) and sleepable
@@ -252,6 +254,7 @@ struct clk_ops {
int (*init)(struct clk_hw *hw);
void (*terminate)(struct clk_hw *hw);
void (*debug_init)(struct clk_hw *hw, struct dentry *dentry);
+ int (*match_clkspec)(struct clk_hw *hw, struct of_phandle_args *clkspec);
};
/**
--
2.35.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4] clk: Introduce 'always-on-clocks' property
2022-09-24 17:45 [PATCH v4] clk: Introduce 'always-on-clocks' property Marek Vasut
@ 2022-10-04 18:26 ` Stephen Boyd
2022-10-06 12:39 ` Marek Vasut
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Boyd @ 2022-10-04 18:26 UTC (permalink / raw
To: Marek Vasut, linux-clk
Cc: Marek Vasut, Andrew Morton, Michael Turquette, Rob Herring,
devicetree
Quoting Marek Vasut (2022-09-24 10:45:17)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b70769d0db99f..6b07f1a086277 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3900,6 +3900,48 @@ static void clk_core_free_parent_map(struct clk_core *core)
> kfree(core->parents);
> }
>
> +static void
> +__clk_register_critical_clock(struct clk_core *core, struct clk_hw *hw)
> +{
> + struct device_node *np = core->of_node;
> + struct of_phandle_args clkspec;
> + u32 clksize, clktotal;
> + int ret, i, index;
> +
> + if (!np)
> + return;
> +
> + if (of_property_read_u32(np, "#clock-cells", &clksize))
> + return;
> +
> + /* Clock node with #clock-cells = <0> uses always-on-clocks; */
> + if (clksize == 0) {
> + if (of_property_read_bool(np, "always-on-clocks"))
> + core->flags |= CLK_IS_CRITICAL;
Why must we set the CLK_IS_CRITICAL flag like this? Instead, when the
clk provider is registered, parse the node of the provider and get the
clks to call clk_prepare_enable() on. We can set the critical flag or
make a new flag that causes clk_disable_unprepare() to not actually turn
the clk off, if we have some sort of underflow issue with other
consumers. Does that fail somehow?
> + return;
> + }
> +
> + if (!core->ops->match_clkspec)
> + return;
> +
> + clkspec.np = np;
> + clktotal = of_property_count_u32_elems(np, "always-on-clocks");
> + clktotal /= clksize;
> + for (index = 0; index < clktotal; index++) {
> + for (i = 0; i < clksize; i++) {
I'm mainly thinking that we're going to spin on this loop constantly for
any clk providers that have many clks to register, but only a few to
keep always on. It would be best to avoid that and only run through the
DT property once.
> + ret = of_property_read_u32_index(np, "always-on-clocks",
> + (index * clksize) + i,
> + &(clkspec.args[i]));
> + if (ret) {
> + pr_warn("Skipping always-on-clocks index %d (ret=%d)\n",
> + i, ret);
> + }
> + }
> + if (!core->ops->match_clkspec(hw, &clkspec))
This callback is provider specific, and not necessarily clk_hw specific.
For example, the clk_ops could be for a generic gate bit, but the
provider wants to keep that gate always on. To implement such a clk we
would have to copy the generic gate clk_ops and set this match_clkspec
op. I'd like to avoid that if possible. If the clk_op must exist, then
perhaps it should be in clk_init_data, which is sort of the place where
we put provider specific information for a clk, i.e. clk_parent_data.
> + core->flags |= CLK_IS_CRITICAL;
> + }
> +}
> +
> static struct clk *
> __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
> {
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4] clk: Introduce 'always-on-clocks' property
2022-10-04 18:26 ` Stephen Boyd
@ 2022-10-06 12:39 ` Marek Vasut
0 siblings, 0 replies; 3+ messages in thread
From: Marek Vasut @ 2022-10-06 12:39 UTC (permalink / raw
To: Stephen Boyd, linux-clk
Cc: Andrew Morton, Michael Turquette, Rob Herring, devicetree
On 10/4/22 20:26, Stephen Boyd wrote:
> Quoting Marek Vasut (2022-09-24 10:45:17)
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index b70769d0db99f..6b07f1a086277 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -3900,6 +3900,48 @@ static void clk_core_free_parent_map(struct clk_core *core)
>> kfree(core->parents);
>> }
>>
>> +static void
>> +__clk_register_critical_clock(struct clk_core *core, struct clk_hw *hw)
>> +{
>> + struct device_node *np = core->of_node;
>> + struct of_phandle_args clkspec;
>> + u32 clksize, clktotal;
>> + int ret, i, index;
>> +
>> + if (!np)
>> + return;
>> +
>> + if (of_property_read_u32(np, "#clock-cells", &clksize))
>> + return;
>> +
>> + /* Clock node with #clock-cells = <0> uses always-on-clocks; */
>> + if (clksize == 0) {
>> + if (of_property_read_bool(np, "always-on-clocks"))
>> + core->flags |= CLK_IS_CRITICAL;
>
> Why must we set the CLK_IS_CRITICAL flag like this?
I don't quite understand the question here. Clock which shouldn't be
turned off should have CLK_IS_CRITICAL flag set.
> Instead, when the
> clk provider is registered, parse the node of the provider and get the
> clks to call clk_prepare_enable() on. We can set the critical flag or
> make a new flag that causes clk_disable_unprepare() to not actually turn
> the clk off, if we have some sort of underflow issue with other
> consumers. Does that fail somehow?
Would your proposal be something that would have to be implemented in
every single clock driver separately ?
>> + return;
>> + }
>> +
>> + if (!core->ops->match_clkspec)
>> + return;
>> +
>> + clkspec.np = np;
>> + clktotal = of_property_count_u32_elems(np, "always-on-clocks");
>> + clktotal /= clksize;
>> + for (index = 0; index < clktotal; index++) {
>> + for (i = 0; i < clksize; i++) {
>
> I'm mainly thinking that we're going to spin on this loop constantly for
> any clk providers that have many clks to register, but only a few to
> keep always on. It would be best to avoid that and only run through the
> DT property once.
Where could this be implemented in the clock core ?
>> + ret = of_property_read_u32_index(np, "always-on-clocks",
>> + (index * clksize) + i,
>> + &(clkspec.args[i]));
>> + if (ret) {
>> + pr_warn("Skipping always-on-clocks index %d (ret=%d)\n",
>> + i, ret);
>> + }
>> + }
>> + if (!core->ops->match_clkspec(hw, &clkspec))
>
> This callback is provider specific, and not necessarily clk_hw specific.
> For example, the clk_ops could be for a generic gate bit, but the
> provider wants to keep that gate always on. To implement such a clk we
> would have to copy the generic gate clk_ops and set this match_clkspec
> op. I'd like to avoid that if possible. If the clk_op must exist, then
> perhaps it should be in clk_init_data, which is sort of the place where
> we put provider specific information for a clk, i.e. clk_parent_data.
The core->ops is copied from struct clk_init_data .ops in
__clk_register() , just before __clk_register_critical_clock() is
called, so that op is already in clk_init_data.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-06 12:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-24 17:45 [PATCH v4] clk: Introduce 'always-on-clocks' property Marek Vasut
2022-10-04 18:26 ` Stephen Boyd
2022-10-06 12:39 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.