| Message ID | 68e96af2df96512300604d797ade2088d7e6e496.1562073871.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v3,1/2] clk: Add clk_min/max_rate entries in debugfs | expand |
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.
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
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?
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);
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.