[PATCH v3 1/2] clk: Add clk_min/max_rate entries in debugfs

[PATCH v3 1/2] clk: Add clk_min/max_rate entries in debugfs

[Leonard Crestez]

Add two files to expose min/max clk rates as determined by
clk_core_get_boundaries, taking all consumer requests into account.

This information does not appear to be otherwise exposed to userspace.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---
 drivers/clk/clk.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Changes since v2:
* Shrink clk_prepare_lock
* Split lock assertion to separate patch
Link to v2: https://patchwork.kernel.org/patch/11021609/

Changes since v1:
* Call clk_prepare_lock/clk_prepare_unlock (Geert)
* Also include in clk_dump, but only with non-default values
Link to v1: https://patchwork.kernel.org/patch/11019873/

Don't add to clk_summary because min/max rates are rarely used and
clk_summary already has too many columns.

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c0990703ce54..e4e224982ae3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2894,19 +2894,26 @@ static int clk_summary_show(struct seq_file *s, void *data)
 }
 DEFINE_SHOW_ATTRIBUTE(clk_summary);
 
 static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
 {
+	unsigned long min_rate, max_rate;
+
 	if (!c)
 		return;
 
 	/* This should be JSON format, i.e. elements separated with a comma */
 	seq_printf(s, "\"%s\": { ", c->name);
 	seq_printf(s, "\"enable_count\": %d,", c->enable_count);
 	seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
 	seq_printf(s, "\"protect_count\": %d,", c->protect_count);
 	seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
+	clk_core_get_boundaries(c, &min_rate, &max_rate);
+	if (min_rate != 0)
+		seq_printf(s, "\"min_rate\": %lu,", min_rate);
+	if (max_rate != ULONG_MAX)
+		seq_printf(s, "\"max_rate\": %lu,", max_rate);
 	seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
 	seq_printf(s, "\"phase\": %d,", clk_core_get_phase(c));
 	seq_printf(s, "\"duty_cycle\": %u",
 		   clk_core_get_scaled_duty_cycle(c, 100000));
 }
@@ -3062,10 +3069,38 @@ static int clk_duty_cycle_show(struct seq_file *s, void *data)
 
 	return 0;
 }
 DEFINE_SHOW_ATTRIBUTE(clk_duty_cycle);
 
+static int clk_min_rate_show(struct seq_file *s, void *data)
+{
+	struct clk_core *core = s->private;
+	unsigned long min_rate, max_rate;
+
+	clk_prepare_lock();
+	clk_core_get_boundaries(core, &min_rate, &max_rate);
+	clk_prepare_unlock();
+	seq_printf(s, "%lu\n", min_rate);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(clk_min_rate);
+
+static int clk_max_rate_show(struct seq_file *s, void *data)
+{
+	struct clk_core *core = s->private;
+	unsigned long min_rate, max_rate;
+
+	clk_prepare_lock();
+	clk_core_get_boundaries(core, &min_rate, &max_rate);
+	clk_prepare_unlock();
+	seq_printf(s, "%lu\n", max_rate);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(clk_max_rate);
+
 static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
 {
 	struct dentry *root;
 
 	if (!core || !pdentry)
@@ -3073,10 +3108,12 @@ static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
 
 	root = debugfs_create_dir(core->name, pdentry);
 	core->dentry = root;
 
 	debugfs_create_ulong("clk_rate", 0444, root, &core->rate);
+	debugfs_create_file("clk_min_rate", 0444, root, core, &clk_min_rate_fops);
+	debugfs_create_file("clk_max_rate", 0444, root, core, &clk_max_rate_fops);
 	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);
 	debugfs_create_u32("clk_prepare_count", 0444, root, &core->prepare_count);
 	debugfs_create_u32("clk_enable_count", 0444, root, &core->enable_count);
-- 
2.17.1

[PATCH v3 2/2] clk: Assert prepare_lock in clk_core_get_boundaries

[Leonard Crestez]

This function iterates the clk consumer list on clk_core so it must be
called under prepare_lock. This is already done by all callers but add a
lockdep assert to check anyway.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---
 drivers/clk/clk.c | 2 ++
 1 file changed, 2 insertions(+)

I wouldn't mind if this is dropped as unnecessary.

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e4e224982ae3..b1c79a58d734 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -591,10 +591,12 @@ static void clk_core_get_boundaries(struct clk_core *core,
 				    unsigned long *min_rate,
 				    unsigned long *max_rate)
 {
 	struct clk *clk_user;
 
+	lockdep_assert_held(&prepare_lock);
+
 	*min_rate = core->min_rate;
 	*max_rate = core->max_rate;
 
 	hlist_for_each_entry(clk_user, &core->clks, clks_node)
 		*min_rate = max(*min_rate, clk_user->min_rate);
-- 
2.17.1

Re: [PATCH v3 1/2] clk: Add clk_min/max_rate entries in debugfs

[Stephen Boyd]

Quoting Leonard Crestez (2019-07-02 06:27:09)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c0990703ce54..e4e224982ae3 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2894,19 +2894,26 @@ static int clk_summary_show(struct seq_file *s, void *data)
>  }
>  DEFINE_SHOW_ATTRIBUTE(clk_summary);
>  
>  static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
>  {
> +       unsigned long min_rate, max_rate;
> +
>         if (!c)
>                 return;
>  
>         /* This should be JSON format, i.e. elements separated with a comma */
>         seq_printf(s, "\"%s\": { ", c->name);
>         seq_printf(s, "\"enable_count\": %d,", c->enable_count);
>         seq_printf(s, "\"prepare_count\": %d,", c->prepare_count);
>         seq_printf(s, "\"protect_count\": %d,", c->protect_count);
>         seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
> +       clk_core_get_boundaries(c, &min_rate, &max_rate);
> +       if (min_rate != 0)
> +               seq_printf(s, "\"min_rate\": %lu,", min_rate);
> +       if (max_rate != ULONG_MAX)
> +               seq_printf(s, "\"max_rate\": %lu,", max_rate);

What are the if conditions about? We always output the values in the
individual files, but for some reason we don't want to do that in the
json output?

>         seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
>         seq_printf(s, "\"phase\": %d,", clk_core_get_phase(c));
>         seq_printf(s, "\"duty_cycle\": %u",
>                    clk_core_get_scaled_duty_cycle(c, 100000));
>  }

Everything else looks fine, so maybe I'll just remove the if statements
if you don't mind.

Re: [PATCH v3 2/2] clk: Assert prepare_lock in clk_core_get_boundaries

[Stephen Boyd]

Quoting Leonard Crestez (2019-07-02 06:27:10)
> This function iterates the clk consumer list on clk_core so it must be
> called under prepare_lock. This is already done by all callers but add a
> lockdep assert to check anyway.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> ---

Applied to clk-next

Re: [PATCH v3 1/2] clk: Add clk_min/max_rate entries in debugfs

[Leonard Crestez]

On 8/8/2019 6:00 PM, Stephen Boyd wrote:
> Quoting Leonard Crestez (2019-07-02 06:27:09)
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c

>>   static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
>>   {
>> +       clk_core_get_boundaries(c, &min_rate, &max_rate);
>> +       if (min_rate != 0)
>> +               seq_printf(s, "\"min_rate\": %lu,", min_rate);
>> +       if (max_rate != ULONG_MAX)
>> +               seq_printf(s, "\"max_rate\": %lu,", max_rate);
> 
> What are the if conditions about? We always output the values in the
> individual files, but for some reason we don't want to do that in the
> json output?

These if conditions are an easy way to avoid spamming "min_rate": 0, 
"max_rate": 18446744073709551615 in json. If you object to the 
inconsistency a nice solution would to be show "null" in both debugfs 
and json.

Outright hiding min/max files from debugfs is impractical, it would 
require filesystem calls from clk_set_min_rate

--
Regards,
Leonard

Re: [PATCH v3 1/2] clk: Add clk_min/max_rate entries in debugfs

[Stephen Boyd]

Quoting Leonard Crestez (2019-08-08 09:46:48)
> On 8/8/2019 6:00 PM, Stephen Boyd wrote:
> > Quoting Leonard Crestez (2019-07-02 06:27:09)
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> 
> >>   static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
> >>   {
> >> +       clk_core_get_boundaries(c, &min_rate, &max_rate);
> >> +       if (min_rate != 0)
> >> +               seq_printf(s, "\"min_rate\": %lu,", min_rate);
> >> +       if (max_rate != ULONG_MAX)
> >> +               seq_printf(s, "\"max_rate\": %lu,", max_rate);
> > 
> > What are the if conditions about? We always output the values in the
> > individual files, but for some reason we don't want to do that in the
> > json output?
> 
> These if conditions are an easy way to avoid spamming "min_rate": 0, 
> "max_rate": 18446744073709551615 in json. If you object to the 
> inconsistency a nice solution would to be show "null" in both debugfs 
> and json.

Aren't those the min and max values though? I don't see it as spam, it's
just more data that is the "default". Given that json is for machine
parsing maybe the parser of this can ignore them if it wants to when the
values match 0 and ULONG_MAX?

Re: [PATCH v3 1/2] clk: Add clk_min/max_rate entries in debugfs

[Stephen Boyd]

Quoting Leonard Crestez (2019-07-16 03:32:44)
> Gentle ping.
> 
> This patch shows up on patchwork but not in my inbox so maybe it was 
> lost in spam filters somehow?
> 
> Geert offered a reviewed-by in v2 but I forgot to add it.
> 

I see v3 on the list and in my inbox.

Re: [PATCH v3 1/2] clk: Add clk_min/max_rate entries in debugfs

[Leonard Crestez]

Gentle ping.

This patch shows up on patchwork but not in my inbox so maybe it was 
lost in spam filters somehow?

Geert offered a reviewed-by in v2 but I forgot to add it.

--
Regards,
Leonard