* [PATCH] clk: Add clk_set_parent debugfs node @ 2021-09-14 14:19 Sam Protsenko 2021-10-05 10:11 ` Sam Protsenko 0 siblings, 1 reply; 4+ messages in thread From: Sam Protsenko @ 2021-09-14 14:19 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd; +Cc: linux-clk, linux-kernel Useful for testing mux clocks. One can write the index of the parent to set into clk_set_parent node, starting from 0. Example # cat clk_possible_parrents dout_shared0_div4 dout_shared1_div4 # cat clk_parent dout_shared0_div4 # echo 1 > clk_set_parent # cat clk_parent dout_shared1_div4 Define CLOCK_ALLOW_WRITE_DEBUGFS in drivers/clk/clk.c in order to use this feature. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- drivers/clk/clk.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 65508eb89ec9..3e5456580db9 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3214,6 +3214,30 @@ static int current_parent_show(struct seq_file *s, void *data) } DEFINE_SHOW_ATTRIBUTE(current_parent); +#ifdef CLOCK_ALLOW_WRITE_DEBUGFS +static int clk_set_parent_set(void *data, u64 val) +{ + struct clk_core *core = data, *parent; + int ret; + + if (val >= core->num_parents) + return -EINVAL; + + parent = clk_core_get_parent_by_index(core, val); + if (IS_ERR_OR_NULL(parent)) + return PTR_ERR(parent); + + clk_prepare_lock(); + ret = clk_core_set_parent_nolock(core, parent); + clk_prepare_unlock(); + + return ret; +} + +DEFINE_DEBUGFS_ATTRIBUTE(clk_set_parent_fops, NULL, clk_set_parent_set, + "%llu\n"); +#endif + static int clk_duty_cycle_show(struct seq_file *s, void *data) { struct clk_core *core = s->private; @@ -3285,9 +3309,14 @@ static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry) debugfs_create_file("clk_parent", 0444, root, core, ¤t_parent_fops); - if (core->num_parents > 1) + if (core->num_parents > 1) { debugfs_create_file("clk_possible_parents", 0444, root, core, &possible_parents_fops); +#ifdef CLOCK_ALLOW_WRITE_DEBUGFS + debugfs_create_file("clk_set_parent", 0200, root, core, + &clk_set_parent_fops); +#endif + } if (core->ops->debug_init) core->ops->debug_init(core->hw, core->dentry); -- 2.30.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] clk: Add clk_set_parent debugfs node 2021-09-14 14:19 [PATCH] clk: Add clk_set_parent debugfs node Sam Protsenko @ 2021-10-05 10:11 ` Sam Protsenko 2021-10-05 10:43 ` Geert Uytterhoeven 0 siblings, 1 reply; 4+ messages in thread From: Sam Protsenko @ 2021-10-05 10:11 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd, Chanwoo Choi, Sylwester Nawrocki, Krzysztof Kozlowski, Mike Tipton, Geert Uytterhoeven Cc: linux-clk, Linux Kernel Mailing List On Tue, 14 Sept 2021 at 17:19, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > Useful for testing mux clocks. One can write the index of the parent to > set into clk_set_parent node, starting from 0. Example > > # cat clk_possible_parrents > dout_shared0_div4 dout_shared1_div4 > # cat clk_parent > dout_shared0_div4 > # echo 1 > clk_set_parent > # cat clk_parent > dout_shared1_div4 > > Define CLOCK_ALLOW_WRITE_DEBUGFS in drivers/clk/clk.c in order to use > this feature. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- + Adding more folks for review Guys, can you please review this one? > drivers/clk/clk.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 65508eb89ec9..3e5456580db9 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -3214,6 +3214,30 @@ static int current_parent_show(struct seq_file *s, void *data) > } > DEFINE_SHOW_ATTRIBUTE(current_parent); > > +#ifdef CLOCK_ALLOW_WRITE_DEBUGFS > +static int clk_set_parent_set(void *data, u64 val) > +{ > + struct clk_core *core = data, *parent; > + int ret; > + > + if (val >= core->num_parents) > + return -EINVAL; > + > + parent = clk_core_get_parent_by_index(core, val); > + if (IS_ERR_OR_NULL(parent)) > + return PTR_ERR(parent); > + > + clk_prepare_lock(); > + ret = clk_core_set_parent_nolock(core, parent); > + clk_prepare_unlock(); > + > + return ret; > +} > + > +DEFINE_DEBUGFS_ATTRIBUTE(clk_set_parent_fops, NULL, clk_set_parent_set, > + "%llu\n"); > +#endif > + > static int clk_duty_cycle_show(struct seq_file *s, void *data) > { > struct clk_core *core = s->private; > @@ -3285,9 +3309,14 @@ static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry) > debugfs_create_file("clk_parent", 0444, root, core, > ¤t_parent_fops); > > - if (core->num_parents > 1) > + if (core->num_parents > 1) { > debugfs_create_file("clk_possible_parents", 0444, root, core, > &possible_parents_fops); > +#ifdef CLOCK_ALLOW_WRITE_DEBUGFS > + debugfs_create_file("clk_set_parent", 0200, root, core, > + &clk_set_parent_fops); > +#endif > + } > > if (core->ops->debug_init) > core->ops->debug_init(core->hw, core->dentry); > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] clk: Add clk_set_parent debugfs node 2021-10-05 10:11 ` Sam Protsenko @ 2021-10-05 10:43 ` Geert Uytterhoeven 2021-10-07 13:39 ` Sam Protsenko 0 siblings, 1 reply; 4+ messages in thread From: Geert Uytterhoeven @ 2021-10-05 10:43 UTC (permalink / raw) To: Sam Protsenko Cc: Michael Turquette, Stephen Boyd, Chanwoo Choi, Sylwester Nawrocki, Krzysztof Kozlowski, Mike Tipton, linux-clk, Linux Kernel Mailing List Hi Sam, On Tue, Oct 5, 2021 at 12:11 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > On Tue, 14 Sept 2021 at 17:19, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > Useful for testing mux clocks. One can write the index of the parent to > > set into clk_set_parent node, starting from 0. Example > > > > # cat clk_possible_parrents > > dout_shared0_div4 dout_shared1_div4 > > # cat clk_parent > > dout_shared0_div4 > > # echo 1 > clk_set_parent > > # cat clk_parent > > dout_shared1_div4 > > > > Define CLOCK_ALLOW_WRITE_DEBUGFS in drivers/clk/clk.c in order to use > > this feature. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > + Adding more folks for review > > Guys, can you please review this one? Thanks for your patch! > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -3214,6 +3214,30 @@ static int current_parent_show(struct seq_file *s, void *data) > > } > > DEFINE_SHOW_ATTRIBUTE(current_parent); > > > > +#ifdef CLOCK_ALLOW_WRITE_DEBUGFS > > +static int clk_set_parent_set(void *data, u64 val) u64 is overkill, num_parents is u8. > > +{ > > + struct clk_core *core = data, *parent; > > + int ret; > > + > > + if (val >= core->num_parents) > > + return -EINVAL; clk_core_get_parent_by_index() called below already checks this. > > + > > + parent = clk_core_get_parent_by_index(core, val); > > + if (IS_ERR_OR_NULL(parent)) > > + return PTR_ERR(parent); > > + > > + clk_prepare_lock(); > > + ret = clk_core_set_parent_nolock(core, parent); > > + clk_prepare_unlock(); > > + > > + return ret; > > +} > > + > > +DEFINE_DEBUGFS_ATTRIBUTE(clk_set_parent_fops, NULL, clk_set_parent_set, > > + "%llu\n"); > > +#endif > > + > > static int clk_duty_cycle_show(struct seq_file *s, void *data) > > { > > struct clk_core *core = s->private; > > @@ -3285,9 +3309,14 @@ static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry) > > debugfs_create_file("clk_parent", 0444, root, core, > > ¤t_parent_fops); > > > > - if (core->num_parents > 1) > > + if (core->num_parents > 1) { > > debugfs_create_file("clk_possible_parents", 0444, root, core, > > &possible_parents_fops); > > +#ifdef CLOCK_ALLOW_WRITE_DEBUGFS > > + debugfs_create_file("clk_set_parent", 0200, root, core, > > + &clk_set_parent_fops); > > +#endif Why add a new file, instead of making the existing "clk_parent" file writable, like is done for "clk_rate"? Yes, "clk_parent" prints a name, while you use a parent number, but I guess that can be fixed? Or even both can be accepted? > > + } > > > > if (core->ops->debug_init) > > core->ops->debug_init(core->hw, core->dentry); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] clk: Add clk_set_parent debugfs node 2021-10-05 10:43 ` Geert Uytterhoeven @ 2021-10-07 13:39 ` Sam Protsenko 0 siblings, 0 replies; 4+ messages in thread From: Sam Protsenko @ 2021-10-07 13:39 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Michael Turquette, Stephen Boyd, Chanwoo Choi, Sylwester Nawrocki, Krzysztof Kozlowski, Mike Tipton, linux-clk, Linux Kernel Mailing List Hi Geert, On Tue, 5 Oct 2021 at 13:43, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Sam, > > On Tue, Oct 5, 2021 at 12:11 PM Sam Protsenko > <semen.protsenko@linaro.org> wrote: > > On Tue, 14 Sept 2021 at 17:19, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > Useful for testing mux clocks. One can write the index of the parent to > > > set into clk_set_parent node, starting from 0. Example > > > > > > # cat clk_possible_parrents > > > dout_shared0_div4 dout_shared1_div4 > > > # cat clk_parent > > > dout_shared0_div4 > > > # echo 1 > clk_set_parent > > > # cat clk_parent > > > dout_shared1_div4 > > > > > > Define CLOCK_ALLOW_WRITE_DEBUGFS in drivers/clk/clk.c in order to use > > > this feature. > > > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > --- > > > > + Adding more folks for review > > > > Guys, can you please review this one? > > Thanks for your patch! > Thanks for review! :) > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c > > > @@ -3214,6 +3214,30 @@ static int current_parent_show(struct seq_file *s, void *data) > > > } > > > DEFINE_SHOW_ATTRIBUTE(current_parent); > > > > > > +#ifdef CLOCK_ALLOW_WRITE_DEBUGFS > > > +static int clk_set_parent_set(void *data, u64 val) > > u64 is overkill, num_parents is u8. > u64 is required by simple_attr_open() signature, because clk_set_parent_set() is being passed there as a parameter eventually (via DEFINE_DEBUGFS_ATTRIBUTE()). But yeah, I'll use u8 when reworking the code for using with existing 'clk_parent' file. > > > +{ > > > + struct clk_core *core = data, *parent; > > > + int ret; > > > + > > > + if (val >= core->num_parents) > > > + return -EINVAL; > > clk_core_get_parent_by_index() called below already checks this. > Thanks, will remove this. > > > + > > > + parent = clk_core_get_parent_by_index(core, val); > > > + if (IS_ERR_OR_NULL(parent)) > > > + return PTR_ERR(parent); Also just noticed that this block is incorrect. I should've used just 'if (!parent)' here instead. I remember Russel King was raising the question about removing that API for good, as too many people tend to use that incorrectly, and now I can see why. > > > + > > > + clk_prepare_lock(); > > > + ret = clk_core_set_parent_nolock(core, parent); > > > + clk_prepare_unlock(); > > > + > > > + return ret; > > > +} > > > + > > > +DEFINE_DEBUGFS_ATTRIBUTE(clk_set_parent_fops, NULL, clk_set_parent_set, > > > + "%llu\n"); > > > +#endif > > > + > > > static int clk_duty_cycle_show(struct seq_file *s, void *data) > > > { > > > struct clk_core *core = s->private; > > > @@ -3285,9 +3309,14 @@ static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry) > > > debugfs_create_file("clk_parent", 0444, root, core, > > > ¤t_parent_fops); > > > > > > - if (core->num_parents > 1) > > > + if (core->num_parents > 1) { > > > debugfs_create_file("clk_possible_parents", 0444, root, core, > > > &possible_parents_fops); > > > +#ifdef CLOCK_ALLOW_WRITE_DEBUGFS > > > + debugfs_create_file("clk_set_parent", 0200, root, core, > > > + &clk_set_parent_fops); > > > +#endif > > Why add a new file, instead of making the existing "clk_parent" file > writable, like is done for "clk_rate"? > Yes, "clk_parent" prints a name, while you use a parent number, but > I guess that can be fixed? Or even both can be accepted? > Ok, I'll merge that feature into existing 'clk_parent' file. At the time I implemented this I was busy with something else, and use existing code around as an example. But it's not too hard to do this properly, by defining 'struct file_operations' manually, like it's done for example in dwc3_lsp_write(). Will send v2 with fixes shortly. > > > + } > > > > > > if (core->ops->debug_init) > > > core->ops->debug_init(core->hw, core->dentry); > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-07 13:40 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-14 14:19 [PATCH] clk: Add clk_set_parent debugfs node Sam Protsenko 2021-10-05 10:11 ` Sam Protsenko 2021-10-05 10:43 ` Geert Uytterhoeven 2021-10-07 13:39 ` Sam Protsenko
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).