[PATCH 0/4] clk: debugfs: add some simple debug functionality

[PATCH 0/4] clk: debugfs: add some simple debug functionality

[Tero Kristo]

Hi,

I have been using a variation of these patches myself for several years
for debugging / testing different clock issues. Basically what I do here
is extend the functionality of debugfs to allow write access to certain
properties, like rate, enable / prepare counts, mux parents.

This allows simple testing of new features or debugging directly from
userspace. The functionality is hidden behind a Kconfig option because
it can be rather dangerous to allow access to these unconditionally if
the user does not know what they are doing.

Any thoughts?

-Tero


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 1/4] clk: debug: add support for setting clk_rate from debugfs

[Tero Kristo]

Debugfs entries for clock drivers don't allow writing to the nodes by
default. Add support for writing to clk_rate nodes via debugfs, this
basically adds a nice debugging capability for testing clk_set_rate
functionality directly from userspace. As this can be considered
dangerous, add a separate Kconfig entry for enabling this feature, and
make it default as not enabled.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/Kconfig |  9 +++++++++
 drivers/clk/clk.c   | 27 +++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 801fa1cd0321..4815ed5248c5 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -23,6 +23,15 @@ config COMMON_CLK
 menu "Common Clock Framework"
 	depends on COMMON_CLK
 
+config COMMON_CLK_DEBUGFS_WRITE_ACCESS
+	bool "Clock debugfs write access enable"
+	depends on DEBUG_FS
+	default n
+	---help---
+	  Enables write access to debugfs entries. This is very useful
+	  for debugging purposes but can be dangerous, thus the default
+	  setting is n.
+
 config COMMON_CLK_WM831X
 	tristate "Clock driver for WM831x/2x PMICs"
 	depends on MFD_WM831X
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ca99e9db6575..b0e82193a63d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3093,6 +3093,28 @@ static int clk_duty_cycle_show(struct seq_file *s, void *data)
 }
 DEFINE_SHOW_ATTRIBUTE(clk_duty_cycle);
 
+#ifdef CONFIG_COMMON_CLK_DEBUGFS_WRITE_ACCESS
+static int clk_dbg_rate_get(void *data, u64 *val)
+{
+	struct clk_core *core = data;
+
+	*val = core->rate;
+
+	return 0;
+}
+
+static int clk_dbg_rate_set(void *data, u64 val)
+{
+	struct clk_core *core = data;
+
+	clk_core_set_rate_nolock(core, val);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(clk_dbg_option_rate, clk_dbg_rate_get, clk_dbg_rate_set, "%llu\n");
+#endif
+
 static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
 {
 	struct dentry *root;
@@ -3103,7 +3125,12 @@ static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
 	root = debugfs_create_dir(core->name, pdentry);
 	core->dentry = root;
 
+#ifdef CONFIG_COMMON_CLK_DEBUGFS_WRITE_ACCESS
+	debugfs_create_file("clk_rate", 0644, root, core,
+			    &clk_dbg_option_rate);
+#else
 	debugfs_create_ulong("clk_rate", 0444, root, &core->rate);
+#endif
 	debugfs_create_ulong("clk_accuracy", 0444, root, &core->accuracy);
 	debugfs_create_u32("clk_phase", 0444, root, &core->phase);
 	debugfs_create_file("clk_flags", 0444, root, core, &clk_flags_fops);
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/4] clk: debug: add support for enable/disable/prep/un-prep from debugfs

[Tero Kristo]

The enable/prepare count variables can now be used to enable/disable/
prepare and un-prepare specific clocks. This is very useful for
debugging purposes, but can be considered dangerous. Thus, it is
protected by the same Kconfig option as the clk_rate modification
option.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b0e82193a63d..e0ceecf727c5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3113,6 +3113,58 @@ static int clk_dbg_rate_set(void *data, u64 val)
 }
 
 DEFINE_SIMPLE_ATTRIBUTE(clk_dbg_option_rate, clk_dbg_rate_get, clk_dbg_rate_set, "%llu\n");
+
+static int clk_dbg_prepare_get(void *data, u64 *val)
+{
+	struct clk_core *core = data;
+
+	*val = core->prepare_count;
+
+	return 0;
+}
+
+static int clk_dbg_prepare_set(void *data, u64 val)
+{
+	struct clk_core *core = data;
+
+	if (val == 1)
+		return clk_core_prepare(core);
+
+	if (val == -1) {
+		clk_core_unprepare(core);
+		return 0;
+	}
+
+	pr_err("1: prepare, -1: unprepare\n");
+	return -EINVAL;
+}
+DEFINE_SIMPLE_ATTRIBUTE(clk_dbg_option_prepare, clk_dbg_prepare_get, clk_dbg_prepare_set, "%llu\n");
+
+static int clk_dbg_enable_get(void *data, u64 *val)
+{
+	struct clk_core *core = data;
+
+	*val = core->enable_count;
+
+	return 0;
+}
+
+static int clk_dbg_enable_set(void *data, u64 val)
+{
+	struct clk_core *core = data;
+
+	if (val == 1)
+		return clk_core_enable(core);
+
+	if (val == -1) {
+		clk_core_disable(core);
+		return 0;
+	}
+
+	pr_err("1: enable, -1: disable\n");
+	return -EINVAL;
+}
+DEFINE_SIMPLE_ATTRIBUTE(clk_dbg_option_enable, clk_dbg_enable_get, clk_dbg_enable_set, "%llu\n");
 #endif
 
 static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
@@ -3134,8 +3186,15 @@ static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
 	debugfs_create_ulong("clk_accuracy", 0444, root, &core->accuracy);
 	debugfs_create_u32("clk_phase", 0444, root, &core->phase);
 	debugfs_create_file("clk_flags", 0444, root, core, &clk_flags_fops);
+#ifdef CONFIG_COMMON_CLK_DEBUGFS_WRITE_ACCESS
+	debugfs_create_file("clk_prepare_count", 0644,
+			    root, core, &clk_dbg_option_prepare);
+	debugfs_create_file("clk_enable_count", 0644,
+			    core->dentry, core, &clk_dbg_option_enable);
+#else
 	debugfs_create_u32("clk_prepare_count", 0444, root, &core->prepare_count);
 	debugfs_create_u32("clk_enable_count", 0444, root, &core->enable_count);
+#endif
 	debugfs_create_u32("clk_protect_count", 0444, root, &core->protect_count);
 	debugfs_create_u32("clk_notifier_count", 0444, root, &core->notifier_count);
 	debugfs_create_file("clk_duty_cycle", 0444, root, core,
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 3/4] clk: ti: mux: add debugfs support for read/write of parent ID

[Tero Kristo]

Add parent_id node under debugfs for mux clocks, that allow both
read/write operations. This can be used to read the current
parent ID, or force a change of current parent of a mux clock.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/ti/mux.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/clk/ti/mux.c b/drivers/clk/ti/mux.c
index 0069e7cf3ebc..f036ecc78034 100644
--- a/drivers/clk/ti/mux.c
+++ b/drivers/clk/ti/mux.c
@@ -21,6 +21,8 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/clk/ti.h>
+#include <linux/debugfs.h>
+
 #include "clock.h"
 
 #undef pr_fmt
@@ -118,12 +120,48 @@ static void clk_mux_restore_context(struct clk_hw *hw)
 	ti_clk_mux_set_parent(hw, mux->saved_parent);
 }
 
+#ifdef CONFIG_COMMON_CLK_DEBUGFS_WRITE_ACCESS
+static int dbg_pid_get(void *data, u64 *val)
+{
+	struct clk_hw *hw = data;
+
+	*val = ti_clk_mux_get_parent(hw);
+
+	return 0;
+}
+
+static int dbg_pid_set(void *data, u64 val)
+{
+	struct clk_hw *hw = data;
+	struct clk_hw *parent = clk_hw_get_parent_by_index(hw, val);
+
+	if (!parent)
+		return -EINVAL;
+
+	clk_hw_reparent(hw, parent);
+
+	return ti_clk_mux_set_parent(hw, val);
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(mux_parent_id_fops, dbg_pid_get, dbg_pid_set, "%llu\n");
+
+static void clk_mux_debug_init(struct clk_hw *hw, struct dentry *dentry)
+{
+	debugfs_create_file("parent_id", 0644, dentry, hw, &mux_parent_id_fops);
+}
+#else
+static void clk_mux_debug_init(struct clk_hw *hw, struct dentry *dentry)
+{
+}
+#endif
+
 const struct clk_ops ti_clk_mux_ops = {
 	.get_parent = ti_clk_mux_get_parent,
 	.set_parent = ti_clk_mux_set_parent,
 	.determine_rate = __clk_mux_determine_rate,
 	.save_context = clk_mux_save_context,
 	.restore_context = clk_mux_restore_context,
+	.debug_init = clk_mux_debug_init,
 };
 
 static struct clk *_register_mux(struct device *dev, const char *name,
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 4/4] clk: keystone: sci-clk: add debugfs support for read/write of parent ID

[Tero Kristo]

Add parent_id node under debugfs for TI SCI clocks, that allow both
read/write operations. This can be used to read the current
parent ID, or force a change of current parent of a multi-parent clock.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/keystone/sci-clk.c | 45 ++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index 7edf8c8432b6..39255b13bdc1 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -24,6 +24,7 @@
 #include <linux/soc/ti/ti_sci_protocol.h>
 #include <linux/bsearch.h>
 #include <linux/list_sort.h>
+#include <linux/debugfs.h>
 
 #define SCI_CLK_SSC_ENABLE		BIT(0)
 #define SCI_CLK_ALLOW_FREQ_CHANGE	BIT(1)
@@ -254,6 +255,41 @@ static int sci_clk_set_parent(struct clk_hw *hw, u8 index)
 					      index + 1 + clk->clk_id);
 }
 
+#ifdef CONFIG_COMMON_CLK_DEBUGFS_WRITE_ACCESS
+static int dbg_pid_get(void *data, u64 *val)
+{
+	struct clk_hw *hw = data;
+
+	*val = sci_clk_get_parent(hw);
+
+	return 0;
+}
+
+static int dbg_pid_set(void *data, u64 val)
+{
+	struct clk_hw *hw = data;
+	struct clk_hw *parent = clk_hw_get_parent_by_index(hw, val);
+
+	if (!parent)
+		return -EINVAL;
+
+	clk_hw_reparent(hw, parent);
+
+	return sci_clk_set_parent(hw, val);
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(sci_parent_id_fops, dbg_pid_get, dbg_pid_set, "%llu\n");
+
+static void sci_clk_debug_init(struct clk_hw *hw, struct dentry *dentry)
+{
+	debugfs_create_file("parent_id", 0644, dentry, hw, &sci_parent_id_fops);
+}
+#else
+static void sci_clk_debug_init(struct clk_hw *hw, struct dentry *dentry)
+{
+}
+#endif
+
 static const struct clk_ops sci_clk_ops = {
 	.prepare = sci_clk_prepare,
 	.unprepare = sci_clk_unprepare,
@@ -263,6 +307,7 @@ static const struct clk_ops sci_clk_ops = {
 	.set_rate = sci_clk_set_rate,
 	.get_parent = sci_clk_get_parent,
 	.set_parent = sci_clk_set_parent,
+	.debug_init = sci_clk_debug_init,
 };
 
 /**
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 0/4] clk: debugfs: add some simple debug functionality

[Tomi Valkeinen]

On 01/10/2019 12:01, Tero Kristo wrote:
> Hi,
> 
> I have been using a variation of these patches myself for several years
> for debugging / testing different clock issues. Basically what I do here
> is extend the functionality of debugfs to allow write access to certain
> properties, like rate, enable / prepare counts, mux parents.
> 
> This allows simple testing of new features or debugging directly from
> userspace. The functionality is hidden behind a Kconfig option because
> it can be rather dangerous to allow access to these unconditionally if
> the user does not know what they are doing.
> 
> Any thoughts?

I haven't reviewed the patches, but I've been using (earlier versions 
of) these, and at least for me they were of great value finding clock 
related issues.

So what it's worth, +1 from me.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 0/4] clk: debugfs: add some simple debug functionality

[Stephen Boyd]

Quoting Tero Kristo (2019-10-01 02:01:58)
> Hi,
> 
> I have been using a variation of these patches myself for several years
> for debugging / testing different clock issues. Basically what I do here
> is extend the functionality of debugfs to allow write access to certain
> properties, like rate, enable / prepare counts, mux parents.
> 
> This allows simple testing of new features or debugging directly from
> userspace. The functionality is hidden behind a Kconfig option because
> it can be rather dangerous to allow access to these unconditionally if
> the user does not know what they are doing.
> 
> Any thoughts?

I just applied Geerts approach to this. Let's follow that and not
provide any Kconfig things.