* [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property
@ 2021-11-08 22:42 Marek Vasut
2021-11-10 7:18 ` kernel test robot
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Marek Vasut @ 2021-11-08 22:42 UTC (permalink / raw)
To: linux-clk
Cc: Marek Vasut, Matti Vaittinen, Michael Turquette, Rob Herring,
Stephen Boyd, devicetree, linux-power
NOTE: This is an RFC patch showing how this mechanism might be workable.
Some platforms require clock to be always running, e.g. because those clock
supply devices which are not otherwise attached to the system. One example
is a system where the SoC serves as a crystal oscillator replacement for a
programmable logic device. The critical-clock property of a clock controller
allows listing clock which must never be turned off.
The implementation here is similar to "protected-clock", except protected
clock property is currently driver specific. This patch attempts to make
a generic implementation of "critical-clock" instead.
Unlike "assigned-clocks", the "critical-clock" must be parsed much earlier
in __clk_register() to assign CLK_IS_CRITICAL flag to clk_init_data .flags
field. The parsing code obviously need to be cleaned up and factor out into
separate function.
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
"critical-clock" property, and if so, then the CLK_IS_CRITICAL is added to
these newly registered clock. This callback is currently driver specific,
although I suspect a common and/or generic version of the callback could
be added. Also, this new callback could possibly be used to replace (*get)
argument of of_clk_add_hw_provider() later on too.
Thoughts (on the overall design, not code quality or patch splitting) ?
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-power@fi.rohmeurope.com
To: linux-clk@vger.kernel.org
---
.../bindings/clock/clock-bindings.txt | 16 ++++++++++++
drivers/clk/clk-bd718x7.c | 15 +++++++++++
drivers/clk/clk.c | 25 +++++++++++++++++++
include/linux/clk-provider.h | 2 ++
4 files changed, 58 insertions(+)
diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index f2ea53832ac63..d9a783c35c5a1 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -169,6 +169,22 @@ a shared clock is forbidden.
Configuration of common clocks, which affect multiple consumer devices can
be similarly specified in the clock provider node.
+==Critical clocks==
+
+Some platforms require clock to be always running, e.g. because those clock
+supply devices which are not otherwise attached to the system. One example
+is a system where the SoC serves as a crystal oscillator replacement for a
+programmable logic device. The critical-clock property of a clock controller
+allows listing clock which must never be turned off.
+
+ clock-controller@a000f000 {
+ compatible = "vendor,clk95;
+ reg = <0xa000f000 0x1000>
+ #clocks-cells = <1>;
+ ...
+ critical-clocks = <UART3_CLK>, <SPI5_CLK>;
+ };
+
==Protected clocks==
Some platforms or firmwares may not fully expose all the clocks to the OS, such
diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
index a59bc57f13bc4..f40765e2860e4 100644
--- a/drivers/clk/clk-bd718x7.c
+++ b/drivers/clk/clk-bd718x7.c
@@ -70,10 +70,25 @@ static int bd71837_clk_is_enabled(struct clk_hw *hw)
return enabled & c->mask;
}
+static int bd71837_match_clkspec(struct clk_hw *hw, struct of_phandle_args *clkspec)
+{
+ struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
+
+ /*
+ * if (clk_hw == clkspec)
+ * return 0;
+ * else
+ * return 1;
+ */
+
+ return 0;
+}
+
static const struct clk_ops bd71837_clk_ops = {
.prepare = &bd71837_clk_enable,
.unprepare = &bd71837_clk_disable,
.is_prepared = &bd71837_clk_is_enabled,
+ .match_clkspec = &bd71837_match_clkspec,
};
static int bd71837_clk_probe(struct platform_device *pdev)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f467d63bbf1ee..fa8e9ea446158 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3849,6 +3849,31 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
core->max_rate = ULONG_MAX;
hw->core = core;
+ struct of_phandle_args clkspec;
+ u32 clksize, clktotal;
+ int i, index;
+
+ if (np && core->ops->match_clkspec && !of_property_read_u32(np, "#clock-cells", &clksize)) {
+ if (clksize == 0) {
+ if (of_property_read_bool(np, "critical-clocks"))
+ core->flags |= CLK_IS_CRITICAL;
+ clktotal = 0;
+ } else {
+ clkspec.np = np;
+ clktotal = of_property_count_u32_elems(np, "critical-clocks");
+ clktotal /= clksize;
+ for (index = 0; index < clktotal; index++) {
+ for (i = 0; i < clksize; i++) {
+ ret = of_property_read_u32_index(np, "critical-clocks",
+ (index * clksize) + i,
+ &(clkspec.args[i]));
+ }
+ if (!core->ops->match_clkspec(hw, &clkspec))
+ core->flags |= CLK_IS_CRITICAL;
+ }
+ }
+ }
+
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 f59c875271a0e..766e93efb23c5 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -205,6 +205,7 @@ 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
*
* The clk_enable/clk_disable and clk_prepare/clk_unprepare pairs allow
* implementations to split any work between atomic (enable) and sleepable
@@ -252,6 +253,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.33.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property
2021-11-08 22:42 [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property Marek Vasut
@ 2021-11-10 7:18 ` kernel test robot
2021-11-10 10:36 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2021-11-10 7:18 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5279 bytes --]
Hi Marek,
[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on clk/clk-next]
[also build test WARNING on v5.15 next-20211110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Marek-Vasut/dt-bindings-clk-Introduce-critical-clocks-property/20211109-064424
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: nios2-defconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6f73b7f399440eee282d746b1907e28f45b81a14
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Marek-Vasut/dt-bindings-clk-Introduce-critical-clocks-property/20211109-064424
git checkout 6f73b7f399440eee282d746b1907e28f45b81a14
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/clk/clk.c: In function '__clk_register':
>> drivers/clk/clk.c:3852:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
3852 | struct of_phandle_args clkspec;
| ^~~~~~
vim +3852 drivers/clk/clk.c
3806
3807 static struct clk *
3808 __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
3809 {
3810 int ret;
3811 struct clk_core *core;
3812 const struct clk_init_data *init = hw->init;
3813
3814 /*
3815 * The init data is not supposed to be used outside of registration path.
3816 * Set it to NULL so that provider drivers can't use it either and so that
3817 * we catch use of hw->init early on in the core.
3818 */
3819 hw->init = NULL;
3820
3821 core = kzalloc(sizeof(*core), GFP_KERNEL);
3822 if (!core) {
3823 ret = -ENOMEM;
3824 goto fail_out;
3825 }
3826
3827 core->name = kstrdup_const(init->name, GFP_KERNEL);
3828 if (!core->name) {
3829 ret = -ENOMEM;
3830 goto fail_name;
3831 }
3832
3833 if (WARN_ON(!init->ops)) {
3834 ret = -EINVAL;
3835 goto fail_ops;
3836 }
3837 core->ops = init->ops;
3838
3839 if (dev && pm_runtime_enabled(dev))
3840 core->rpm_enabled = true;
3841 core->dev = dev;
3842 core->of_node = np;
3843 if (dev && dev->driver)
3844 core->owner = dev->driver->owner;
3845 core->hw = hw;
3846 core->flags = init->flags;
3847 core->num_parents = init->num_parents;
3848 core->min_rate = 0;
3849 core->max_rate = ULONG_MAX;
3850 hw->core = core;
3851
> 3852 struct of_phandle_args clkspec;
3853 u32 clksize, clktotal;
3854 int i, index;
3855
3856 if (np && core->ops->match_clkspec && !of_property_read_u32(np, "#clock-cells", &clksize)) {
3857 if (clksize == 0) {
3858 if (of_property_read_bool(np, "critical-clocks"))
3859 core->flags |= CLK_IS_CRITICAL;
3860 clktotal = 0;
3861 } else {
3862 clkspec.np = np;
3863 clktotal = of_property_count_u32_elems(np, "critical-clocks");
3864 clktotal /= clksize;
3865 for (index = 0; index < clktotal; index++) {
3866 for (i = 0; i < clksize; i++) {
3867 ret = of_property_read_u32_index(np, "critical-clocks",
3868 (index * clksize) + i,
3869 &(clkspec.args[i]));
3870 }
3871 if (!core->ops->match_clkspec(hw, &clkspec))
3872 core->flags |= CLK_IS_CRITICAL;
3873 }
3874 }
3875 }
3876
3877 ret = clk_core_populate_parent_map(core, init);
3878 if (ret)
3879 goto fail_parents;
3880
3881 INIT_HLIST_HEAD(&core->clks);
3882
3883 /*
3884 * Don't call clk_hw_create_clk() here because that would pin the
3885 * provider module to itself and prevent it from ever being removed.
3886 */
3887 hw->clk = alloc_clk(core, NULL, NULL);
3888 if (IS_ERR(hw->clk)) {
3889 ret = PTR_ERR(hw->clk);
3890 goto fail_create_clk;
3891 }
3892
3893 clk_core_link_consumer(hw->core, hw->clk);
3894
3895 ret = __clk_core_init(core);
3896 if (!ret)
3897 return hw->clk;
3898
3899 clk_prepare_lock();
3900 clk_core_unlink_consumer(hw->clk);
3901 clk_prepare_unlock();
3902
3903 free_clk(hw->clk);
3904 hw->clk = NULL;
3905
3906 fail_create_clk:
3907 clk_core_free_parent_map(core);
3908 fail_parents:
3909 fail_ops:
3910 kfree_const(core->name);
3911 fail_name:
3912 kfree(core);
3913 fail_out:
3914 return ERR_PTR(ret);
3915 }
3916
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 10198 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property
2021-11-08 22:42 [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property Marek Vasut
2021-11-10 7:18 ` kernel test robot
@ 2021-11-10 10:36 ` kernel test robot
2021-11-12 6:56 ` Vaittinen, Matti
2021-11-29 20:18 ` Rob Herring
3 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2021-11-10 10:36 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 6536 bytes --]
Hi Marek,
[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on clk/clk-next]
[also build test ERROR on v5.15 next-20211110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Marek-Vasut/dt-bindings-clk-Introduce-critical-clocks-property/20211109-064424
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6f73b7f399440eee282d746b1907e28f45b81a14
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Marek-Vasut/dt-bindings-clk-Introduce-critical-clocks-property/20211109-064424
git checkout 6f73b7f399440eee282d746b1907e28f45b81a14
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/clk/clk.c: In function '__clk_register':
>> drivers/clk/clk.c:3852:9: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
3852 | struct of_phandle_args clkspec;
| ^~~~~~
In file included from include/linux/perf_event.h:25,
from include/linux/trace_events.h:10,
from include/trace/trace_events.h:21,
from include/trace/define_trace.h:102,
from include/trace/events/clk.h:270,
from drivers/clk/clk.c:95:
At top level:
arch/arc/include/asm/perf_event.h:126:27: error: 'arc_pmu_cache_map' defined but not used [-Werror=unused-const-variable=]
126 | static const unsigned int arc_pmu_cache_map[C(MAX)][C(OP_MAX)][C(RESULT_MAX)] = {
| ^~~~~~~~~~~~~~~~~
arch/arc/include/asm/perf_event.h:91:27: error: 'arc_pmu_ev_hw_map' defined but not used [-Werror=unused-const-variable=]
91 | static const char * const arc_pmu_ev_hw_map[] = {
| ^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
--
drivers/clk/clk-bd718x7.c: In function 'bd71837_match_clkspec':
>> drivers/clk/clk-bd718x7.c:75:29: error: unused variable 'c' [-Werror=unused-variable]
75 | struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
| ^
cc1: all warnings being treated as errors
vim +3852 drivers/clk/clk.c
3806
3807 static struct clk *
3808 __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
3809 {
3810 int ret;
3811 struct clk_core *core;
3812 const struct clk_init_data *init = hw->init;
3813
3814 /*
3815 * The init data is not supposed to be used outside of registration path.
3816 * Set it to NULL so that provider drivers can't use it either and so that
3817 * we catch use of hw->init early on in the core.
3818 */
3819 hw->init = NULL;
3820
3821 core = kzalloc(sizeof(*core), GFP_KERNEL);
3822 if (!core) {
3823 ret = -ENOMEM;
3824 goto fail_out;
3825 }
3826
3827 core->name = kstrdup_const(init->name, GFP_KERNEL);
3828 if (!core->name) {
3829 ret = -ENOMEM;
3830 goto fail_name;
3831 }
3832
3833 if (WARN_ON(!init->ops)) {
3834 ret = -EINVAL;
3835 goto fail_ops;
3836 }
3837 core->ops = init->ops;
3838
3839 if (dev && pm_runtime_enabled(dev))
3840 core->rpm_enabled = true;
3841 core->dev = dev;
3842 core->of_node = np;
3843 if (dev && dev->driver)
3844 core->owner = dev->driver->owner;
3845 core->hw = hw;
3846 core->flags = init->flags;
3847 core->num_parents = init->num_parents;
3848 core->min_rate = 0;
3849 core->max_rate = ULONG_MAX;
3850 hw->core = core;
3851
> 3852 struct of_phandle_args clkspec;
3853 u32 clksize, clktotal;
3854 int i, index;
3855
3856 if (np && core->ops->match_clkspec && !of_property_read_u32(np, "#clock-cells", &clksize)) {
3857 if (clksize == 0) {
3858 if (of_property_read_bool(np, "critical-clocks"))
3859 core->flags |= CLK_IS_CRITICAL;
3860 clktotal = 0;
3861 } else {
3862 clkspec.np = np;
3863 clktotal = of_property_count_u32_elems(np, "critical-clocks");
3864 clktotal /= clksize;
3865 for (index = 0; index < clktotal; index++) {
3866 for (i = 0; i < clksize; i++) {
3867 ret = of_property_read_u32_index(np, "critical-clocks",
3868 (index * clksize) + i,
3869 &(clkspec.args[i]));
3870 }
3871 if (!core->ops->match_clkspec(hw, &clkspec))
3872 core->flags |= CLK_IS_CRITICAL;
3873 }
3874 }
3875 }
3876
3877 ret = clk_core_populate_parent_map(core, init);
3878 if (ret)
3879 goto fail_parents;
3880
3881 INIT_HLIST_HEAD(&core->clks);
3882
3883 /*
3884 * Don't call clk_hw_create_clk() here because that would pin the
3885 * provider module to itself and prevent it from ever being removed.
3886 */
3887 hw->clk = alloc_clk(core, NULL, NULL);
3888 if (IS_ERR(hw->clk)) {
3889 ret = PTR_ERR(hw->clk);
3890 goto fail_create_clk;
3891 }
3892
3893 clk_core_link_consumer(hw->core, hw->clk);
3894
3895 ret = __clk_core_init(core);
3896 if (!ret)
3897 return hw->clk;
3898
3899 clk_prepare_lock();
3900 clk_core_unlink_consumer(hw->clk);
3901 clk_prepare_unlock();
3902
3903 free_clk(hw->clk);
3904 hw->clk = NULL;
3905
3906 fail_create_clk:
3907 clk_core_free_parent_map(core);
3908 fail_parents:
3909 fail_ops:
3910 kfree_const(core->name);
3911 fail_name:
3912 kfree(core);
3913 fail_out:
3914 return ERR_PTR(ret);
3915 }
3916
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 69185 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property
2021-11-08 22:42 [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property Marek Vasut
2021-11-10 7:18 ` kernel test robot
2021-11-10 10:36 ` kernel test robot
@ 2021-11-12 6:56 ` Vaittinen, Matti
2021-11-29 20:18 ` Rob Herring
3 siblings, 0 replies; 6+ messages in thread
From: Vaittinen, Matti @ 2021-11-12 6:56 UTC (permalink / raw)
To: Marek Vasut, linux-clk
Cc: Michael Turquette, Rob Herring, Stephen Boyd, devicetree, linux-power
On 11/9/21 00:42, Marek Vasut wrote:
> NOTE: This is an RFC patch showing how this mechanism might be workable.
>
> Some platforms require clock to be always running, e.g. because those clock
> supply devices which are not otherwise attached to the system. One example
> is a system where the SoC serves as a crystal oscillator replacement for a
> programmable logic device. The critical-clock property of a clock controller
> allows listing clock which must never be turned off.
Hm. It slightly bugs me to parse the clock properties form DT in many
places. I was thinking that perhaps we could parse all the clk
properties at __clk_register() and store them to be later used when
of_clk_provider is added. After having a chat with Marek I was kindly
explained the DT node might not always be present at __clk_register() -
phase. I don't easily see a better way so I'd better to just learn to
live with the bugging feeling :)
>
> The implementation here is similar to "protected-clock", except protected
> clock property is currently driver specific. This patch attempts to make
> a generic implementation of "critical-clock" instead.
>
> Unlike "assigned-clocks", the "critical-clock" must be parsed much earlier
> in __clk_register() to assign CLK_IS_CRITICAL flag to clk_init_data .flags
> field. The parsing code obviously need to be cleaned up and factor out into
> separate function.
>
> 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
> "critical-clock" property, and if so, then the CLK_IS_CRITICAL is added to
> these newly registered clock. This callback is currently driver specific,
> although I suspect a common and/or generic version of the callback could
> be added. Also, this new callback could possibly be used to replace (*get)
> argument of of_clk_add_hw_provider() later on too.
>
I do also like the idea of being able to replace the get-callback - and
a driver specific callback with a generic op. Can the clock-indices be
somehow used for generic one? Perhaps allowing also adding a driver
specific callback for cases where generic one can't do the job? (I don't
have any concrete idea/code how to do that right now though). But what
ever it is worth, I do like the overall idea. It sounds right to me.
> Thoughts (on the overall design, not code quality or patch splitting) ?
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-power@fi.rohmeurope.com
> To: linux-clk@vger.kernel.org
> ---
> .../bindings/clock/clock-bindings.txt | 16 ++++++++++++
> drivers/clk/clk-bd718x7.c | 15 +++++++++++
> drivers/clk/clk.c | 25 +++++++++++++++++++
> include/linux/clk-provider.h | 2 ++
> 4 files changed, 58 insertions(+)
>
Best Regards
--Matti Vaittinen
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property
2021-11-08 22:42 [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property Marek Vasut
` (2 preceding siblings ...)
2021-11-12 6:56 ` Vaittinen, Matti
@ 2021-11-29 20:18 ` Rob Herring
2022-02-15 10:31 ` Marek Vasut
3 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2021-11-29 20:18 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-clk, Matti Vaittinen, Michael Turquette, Stephen Boyd,
devicetree, linux-power
On Mon, Nov 08, 2021 at 11:42:42PM +0100, Marek Vasut wrote:
> NOTE: This is an RFC patch showing how this mechanism might be workable.
>
> Some platforms require clock to be always running, e.g. because those clock
> supply devices which are not otherwise attached to the system. One example
> is a system where the SoC serves as a crystal oscillator replacement for a
> programmable logic device. The critical-clock property of a clock controller
> allows listing clock which must never be turned off.
>
> The implementation here is similar to "protected-clock", except protected
> clock property is currently driver specific. This patch attempts to make
> a generic implementation of "critical-clock" instead.
>
> Unlike "assigned-clocks", the "critical-clock" must be parsed much earlier
> in __clk_register() to assign CLK_IS_CRITICAL flag to clk_init_data .flags
> field. The parsing code obviously need to be cleaned up and factor out into
> separate function.
>
> 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
> "critical-clock" property, and if so, then the CLK_IS_CRITICAL is added to
> these newly registered clock. This callback is currently driver specific,
> although I suspect a common and/or generic version of the callback could
> be added. Also, this new callback could possibly be used to replace (*get)
> argument of of_clk_add_hw_provider() later on too.
>
> Thoughts (on the overall design, not code quality or patch splitting) ?
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-power@fi.rohmeurope.com
> To: linux-clk@vger.kernel.org
> ---
> .../bindings/clock/clock-bindings.txt | 16 ++++++++++++
> drivers/clk/clk-bd718x7.c | 15 +++++++++++
> drivers/clk/clk.c | 25 +++++++++++++++++++
> include/linux/clk-provider.h | 2 ++
> 4 files changed, 58 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index f2ea53832ac63..d9a783c35c5a1 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -169,6 +169,22 @@ a shared clock is forbidden.
> Configuration of common clocks, which affect multiple consumer devices can
> be similarly specified in the clock provider node.
>
> +==Critical clocks==
> +
> +Some platforms require clock to be always running, e.g. because those clock
> +supply devices which are not otherwise attached to the system. One example
> +is a system where the SoC serves as a crystal oscillator replacement for a
> +programmable logic device. The critical-clock property of a clock controller
> +allows listing clock which must never be turned off.
> +
> + clock-controller@a000f000 {
> + compatible = "vendor,clk95;
> + reg = <0xa000f000 0x1000>
> + #clocks-cells = <1>;
> + ...
> + critical-clocks = <UART3_CLK>, <SPI5_CLK>;
This will need a schema definition in dtschema.
Otherwise, the concept is fine for me.
Rob
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property
2021-11-29 20:18 ` Rob Herring
@ 2022-02-15 10:31 ` Marek Vasut
0 siblings, 0 replies; 6+ messages in thread
From: Marek Vasut @ 2022-02-15 10:31 UTC (permalink / raw)
To: Rob Herring
Cc: linux-clk, Matti Vaittinen, Michael Turquette, Stephen Boyd,
devicetree, linux-power
On 11/29/21 21:18, Rob Herring wrote:
> On Mon, Nov 08, 2021 at 11:42:42PM +0100, Marek Vasut wrote:
>> NOTE: This is an RFC patch showing how this mechanism might be workable.
>>
>> Some platforms require clock to be always running, e.g. because those clock
>> supply devices which are not otherwise attached to the system. One example
>> is a system where the SoC serves as a crystal oscillator replacement for a
>> programmable logic device. The critical-clock property of a clock controller
>> allows listing clock which must never be turned off.
>>
>> The implementation here is similar to "protected-clock", except protected
>> clock property is currently driver specific. This patch attempts to make
>> a generic implementation of "critical-clock" instead.
>>
>> Unlike "assigned-clocks", the "critical-clock" must be parsed much earlier
>> in __clk_register() to assign CLK_IS_CRITICAL flag to clk_init_data .flags
>> field. The parsing code obviously need to be cleaned up and factor out into
>> separate function.
>>
>> 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
>> "critical-clock" property, and if so, then the CLK_IS_CRITICAL is added to
>> these newly registered clock. This callback is currently driver specific,
>> although I suspect a common and/or generic version of the callback could
>> be added. Also, this new callback could possibly be used to replace (*get)
>> argument of of_clk_add_hw_provider() later on too.
>>
>> Thoughts (on the overall design, not code quality or patch splitting) ?
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
>> Cc: Michael Turquette <mturquette@baylibre.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Stephen Boyd <sboyd@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-power@fi.rohmeurope.com
>> To: linux-clk@vger.kernel.org
>> ---
>> .../bindings/clock/clock-bindings.txt | 16 ++++++++++++
>> drivers/clk/clk-bd718x7.c | 15 +++++++++++
>> drivers/clk/clk.c | 25 +++++++++++++++++++
>> include/linux/clk-provider.h | 2 ++
>> 4 files changed, 58 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> index f2ea53832ac63..d9a783c35c5a1 100644
>> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> @@ -169,6 +169,22 @@ a shared clock is forbidden.
>> Configuration of common clocks, which affect multiple consumer devices can
>> be similarly specified in the clock provider node.
>>
>> +==Critical clocks==
>> +
>> +Some platforms require clock to be always running, e.g. because those clock
>> +supply devices which are not otherwise attached to the system. One example
>> +is a system where the SoC serves as a crystal oscillator replacement for a
>> +programmable logic device. The critical-clock property of a clock controller
>> +allows listing clock which must never be turned off.
>> +
>> + clock-controller@a000f000 {
>> + compatible = "vendor,clk95;
>> + reg = <0xa000f000 0x1000>
>> + #clocks-cells = <1>;
>> + ...
>> + critical-clocks = <UART3_CLK>, <SPI5_CLK>;
>
> This will need a schema definition in dtschema.
>
> Otherwise, the concept is fine for me.
I sent out proper patches, also for the schemas.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-15 10:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-08 22:42 [PATCH] [RFC] dt-bindings: clk: Introduce 'critical-clocks' property Marek Vasut
2021-11-10 7:18 ` kernel test robot
2021-11-10 10:36 ` kernel test robot
2021-11-12 6:56 ` Vaittinen, Matti
2021-11-29 20:18 ` Rob Herring
2022-02-15 10:31 ` 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.