LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] clk: add sys node to disable unused clk
@ 2021-08-13  9:11 Zhipeng Wang
  2021-08-17 12:08 ` kernel test robot
  2021-08-29  5:20 ` Stephen Boyd
  0 siblings, 2 replies; 4+ messages in thread
From: Zhipeng Wang @ 2021-08-13  9:11 UTC (permalink / raw)
  To: mturquette; +Cc: sboyd, linux-clk, linux-kernel

The normal sequence is that the clock provider registers clk to
the clock framework, and the clock framework manages the clock resources
of the system. Clk consumers obtain clk through the clock framework API,
enable and disable clk.

Not all clk registered through the clock provider will be used
by the clock consumer, so the clock framework has a function
late_initcall_sync(clk_disable_unused); disables the unused clk.

Now we modularize the clock provider and some consumers, which will
cause late_initcall_sync(clk_disable_unused); cannot work properly, so
increase the sys node.

Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
---
 drivers/clk/clk.c | 74 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 70 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 65508eb89ec9..2bd496c87f80 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -32,6 +32,7 @@ static struct task_struct *enable_owner;
 
 static int prepare_refcnt;
 static int enable_refcnt;
+static bool enable_clk_disable_unused;
 
 static HLIST_HEAD(clk_root_list);
 static HLIST_HEAD(clk_orphan_list);
@@ -1206,7 +1207,7 @@ static void clk_core_disable_unprepare(struct clk_core *core)
 	clk_core_unprepare_lock(core);
 }
 
-static void __init clk_unprepare_unused_subtree(struct clk_core *core)
+static void clk_unprepare_unused_subtree(struct clk_core *core)
 {
 	struct clk_core *child;
 
@@ -1236,7 +1237,7 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
 	clk_pm_runtime_put(core);
 }
 
-static void __init clk_disable_unused_subtree(struct clk_core *core)
+static void clk_disable_unused_subtree(struct clk_core *core)
 {
 	struct clk_core *child;
 	unsigned long flags;
@@ -1282,7 +1283,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
 		clk_core_disable_unprepare(core->parent);
 }
 
-static bool clk_ignore_unused __initdata;
+static bool clk_ignore_unused;
 static int __init clk_ignore_unused_setup(char *__unused)
 {
 	clk_ignore_unused = true;
@@ -1290,7 +1291,7 @@ static int __init clk_ignore_unused_setup(char *__unused)
 }
 __setup("clk_ignore_unused", clk_ignore_unused_setup);
 
-static int __init clk_disable_unused(void)
+static int clk_disable_unused(void)
 {
 	struct clk_core *core;
 
@@ -1319,6 +1320,71 @@ static int __init clk_disable_unused(void)
 }
 late_initcall_sync(clk_disable_unused);
 
+static void clk_disable_unused_enable(bool enable)
+{
+	if (enable) {
+		clk_disable_unused();
+		enable_clk_disable_unused = true;
+		pr_info("clk_disable_unused: enabled\n");
+	} else {
+		enable_clk_disable_unused = false;
+		pr_info("clk_disable_unused: disabled\n");
+	}
+}
+
+static bool clk_disable_unused_status(void)
+{
+	return enable_clk_disable_unused;
+}
+
+static ssize_t clk_disable_unused_show(struct kobject *kobj, struct kobj_attribute *attr,
+					char *buf)
+{
+	return sprintf(buf, "%u\n", clk_disable_unused_status());
+}
+
+static ssize_t clk_disable_unused_store(struct kobject *kobj, struct kobj_attribute *attr,
+					const char *buf, size_t n)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+	if (val > 1)
+		return -EINVAL;
+
+	clk_disable_unused_enable(val);
+
+	return n;
+}
+
+static struct kobj_attribute  clk_ctrl_attr =
+	__ATTR(enable_clk_disable_unused, 0644, clk_disable_unused_show, clk_disable_unused_store);
+
+static struct attribute *clk_ctrl_attrbute[] = {
+	&clk_ctrl_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group clk_ctrl_attr_group = {
+	.attrs = clk_ctrl_attrbute,
+};
+
+static const struct attribute_group *clk_ctrl_attr_groups[] = {
+	&clk_ctrl_attr_group,
+	NULL,
+};
+
+static int __init creat_sys_clk_unused(void)
+{
+	struct kobject *clk_ctrl_kobj = kobject_create_and_add("clk_ctrl", NULL);
+
+	sysfs_create_groups(clk_ctrl_kobj, clk_ctrl_attr_groups);
+
+	return 0;
+}
+late_initcall_sync(creat_sys_clk_unused);
+
 static int clk_core_determine_round_nolock(struct clk_core *core,
 					   struct clk_rate_request *req)
 {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] clk: add sys node to disable unused clk
  2021-08-13  9:11 [PATCH] clk: add sys node to disable unused clk Zhipeng Wang
@ 2021-08-17 12:08 ` kernel test robot
  2021-08-29  5:20 ` Stephen Boyd
  1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-08-17 12:08 UTC (permalink / raw)
  To: Zhipeng Wang, mturquette; +Cc: kbuild-all, sboyd, linux-clk, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3093 bytes --]

Hi Zhipeng,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on v5.14-rc6 next-20210816]
[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/Zhipeng-Wang/clk-add-sys-node-to-disable-unused-clk/20210813-094034
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: arc-randconfig-r011-20210812 (attached as .config)
compiler: arc-elf-gcc (GCC) 10.3.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/166d814a2157788c7f7c7224b08ccdca8aaed7a0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Zhipeng-Wang/clk-add-sys-node-to-disable-unused-clk/20210813-094034
        git checkout 166d814a2157788c7f7c7224b08ccdca8aaed7a0
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.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 warnings (new ones prefixed by >>):

   drivers/clk/clk.c: In function 'creat_sys_clk_unused':
>> drivers/clk/clk.c:1382:2: warning: ignoring return value of 'sysfs_create_groups' declared with attribute 'warn_unused_result' [-Wunused-result]
    1382 |  sysfs_create_groups(clk_ctrl_kobj, clk_ctrl_attr_groups);
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   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:96:
   At top level:
   arch/arc/include/asm/perf_event.h:126:23: warning: 'arc_pmu_cache_map' defined but not used [-Wunused-const-variable=]
     126 | static const unsigned arc_pmu_cache_map[C(MAX)][C(OP_MAX)][C(RESULT_MAX)] = {
         |                       ^~~~~~~~~~~~~~~~~
   arch/arc/include/asm/perf_event.h:91:27: warning: 'arc_pmu_ev_hw_map' defined but not used [-Wunused-const-variable=]
      91 | static const char * const arc_pmu_ev_hw_map[] = {
         |                           ^~~~~~~~~~~~~~~~~


vim +1382 drivers/clk/clk.c

  1377	
  1378	static int __init creat_sys_clk_unused(void)
  1379	{
  1380		struct kobject *clk_ctrl_kobj = kobject_create_and_add("clk_ctrl", NULL);
  1381	
> 1382		sysfs_create_groups(clk_ctrl_kobj, clk_ctrl_attr_groups);
  1383	
  1384		return 0;
  1385	}
  1386	late_initcall_sync(creat_sys_clk_unused);
  1387	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27298 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] clk: add sys node to disable unused clk
  2021-08-13  9:11 [PATCH] clk: add sys node to disable unused clk Zhipeng Wang
  2021-08-17 12:08 ` kernel test robot
@ 2021-08-29  5:20 ` Stephen Boyd
  2021-09-01 12:12   ` [EXT] " Zhipeng Wang
  1 sibling, 1 reply; 4+ messages in thread
From: Stephen Boyd @ 2021-08-29  5:20 UTC (permalink / raw)
  To: Zhipeng Wang, mturquette; +Cc: linux-clk, linux-kernel

Quoting Zhipeng Wang (2021-08-13 02:11:18)
> The normal sequence is that the clock provider registers clk to
> the clock framework, and the clock framework manages the clock resources
> of the system. Clk consumers obtain clk through the clock framework API,
> enable and disable clk.
> 
> Not all clk registered through the clock provider will be used
> by the clock consumer, so the clock framework has a function
> late_initcall_sync(clk_disable_unused); disables the unused clk.
> 
> Now we modularize the clock provider and some consumers, which will
> cause late_initcall_sync(clk_disable_unused); cannot work properly, so
> increase the sys node.

Sorry, I honestly don't know what's going on here. The commit text is
describing the clk disable unused logic, and then we're adding a sysfs
attribute without documenting it in Documentation/ABI? And there's one
sentence about modular clks not working properly, but I don't know what
is wrong about it besides it is "wrong".

I've been considering ripping out the late initcall to disable unused
clks. It pretty much only leads to problems. If we want to save power,
then maybe we should have clk providers tell us what clks aren't ever
going to be used unless they're referenced in DT or via some clkdev
lookup and only turn the ones off that are in that list if they're on.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [EXT] Re: [PATCH] clk: add sys node to disable unused clk
  2021-08-29  5:20 ` Stephen Boyd
@ 2021-09-01 12:12   ` Zhipeng Wang
  0 siblings, 0 replies; 4+ messages in thread
From: Zhipeng Wang @ 2021-09-01 12:12 UTC (permalink / raw)
  To: Stephen Boyd, mturquette; +Cc: linux-clk, linux-kernel

Hi Stephen,

First of all thank you very much for your reply. 

Our clk providers do not just open the clk used as you said, but rely on late_initcall_sync(clk_disable_unused); close the clks that are not used. But in order to support Android gki, I modularized clk providers and some clk consumers. This change makes late_initcall_sync(clk_disable_unused); unable to achieve our expected function. I am sorry for what may not be expressed in the commit.

I also hope you can rip out the late initcall to disable unused clks, because this can promote clk providers to be more reasonable. My main job is Android, so I need to make suggestions to our linux BSP team to improve clk providers. Because of late_initcall_sync(clk_disable_unused);, my suggestion seems to be customized.

Because I call clk_disable_unused in the user space and meet my expectations , it close the clks that are not used. That's why I uploaded this patch to you.

I hope you can make a decision whether to rip out the late initcall to disable unused clks after careful consideration. Thank you very much. 


BRs
Zhipeng
-----Original Message-----
From: Stephen Boyd <sboyd@kernel.org> 
Sent: 2021年8月29日 13:21
To: Zhipeng Wang <zhipeng.wang_1@nxp.com>; mturquette@baylibre.com
Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: [EXT] Re: [PATCH] clk: add sys node to disable unused clk

Caution: EXT Email

Quoting Zhipeng Wang (2021-08-13 02:11:18)
> The normal sequence is that the clock provider registers clk to the 
> clock framework, and the clock framework manages the clock resources 
> of the system. Clk consumers obtain clk through the clock framework 
> API, enable and disable clk.
>
> Not all clk registered through the clock provider will be used by the 
> clock consumer, so the clock framework has a function 
> late_initcall_sync(clk_disable_unused); disables the unused clk.
>
> Now we modularize the clock provider and some consumers, which will 
> cause late_initcall_sync(clk_disable_unused); cannot work properly, so 
> increase the sys node.

Sorry, I honestly don't know what's going on here. The commit text is describing the clk disable unused logic, and then we're adding a sysfs attribute without documenting it in Documentation/ABI? And there's one sentence about modular clks not working properly, but I don't know what is wrong about it besides it is "wrong".

I've been considering ripping out the late initcall to disable unused clks. It pretty much only leads to problems. If we want to save power, then maybe we should have clk providers tell us what clks aren't ever going to be used unless they're referenced in DT or via some clkdev lookup and only turn the ones off that are in that list if they're on.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-09-01 12:12 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13  9:11 [PATCH] clk: add sys node to disable unused clk Zhipeng Wang
2021-08-17 12:08 ` kernel test robot
2021-08-29  5:20 ` Stephen Boyd
2021-09-01 12:12   ` [EXT] " Zhipeng Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).