On Thu, Jun 23, 2022 at 06:05:48PM -0700, Stephen Boyd wrote:
> Quoting <Vishal Badole> (2022-06-22 10:02:20)
> >
> > From f2e9d78bd0f135206fbfbf2e0178a5782b972939 Mon Sep 17 00:00:00 2001
> > From: Vishal Badole <[email protected]>
> > Date: Tue, 21 Jun 2022 09:55:51 +0530
> > Subject: [PATCH] Common clock: To list active consumers of clocks
>
> That patch is still malformed. Please try again. Also, stop sending it
> as a reply-to the previous patch. Thanks!
>
We have applied and checked the patch on top of the mainline and not
able to see that it is malformed. We will share revised patch using git
send mail.
> >
> > This feature lists the name of clocks and their consumer devices.
> > Using this feature user can easily check which device is using a
> > perticular clock. Consumers without dev_id are listed as no_dev_id.
> >
> > Co-developed-by: Chinmoy Ghosh <[email protected]>
> > Signed-off-by: Chinmoy Ghosh <[email protected]>
> > Co-developed-by: Mintu Patel <[email protected]>
> > Signed-off-by: Mintu Patel <[email protected]>
> > Acked-by: Vimal Kumar <[email protected]>
>
> The acked-by tag is largely for maintainer use. Please remove it. See
> Documentation/process/5.Posting.rst for more info.
>
Agreed, We will update this in the revised patch.
> > Signed-off-by: Vishal Badole <[email protected]>
> > ---
> > drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 59 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index ed11918..b191009 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3018,6 +3018,63 @@ static int clk_summary_show(struct seq_file *s, void *data)
> > }
> > DEFINE_SHOW_ATTRIBUTE(clk_summary);
> >
> > +static void clk_consumer_show_one(struct seq_file *s, struct clk_core *core, int level)
> > +{
> > + struct clk *clk_user;
> > + const char *consumer;
> > +
> > + hlist_for_each_entry(clk_user, &core->clks, clks_node) {
> > + if (!clk_user->dev_id)
> > + consumer = "no_dev_id";
> > + else
> > + consumer = clk_user->dev_id;
>
> We can just pass NULL if there isn't a dev_id and get a nice "(NULL)"
> print instead of "no_dev_id".
>
Agreed, we will replace "no_dev_id" with "deviceless" in the revised
patch.
> > +
> > + seq_printf(s, "%*s%-*s %30s %5d %7d ",
> > + level * 3 + 1, "",
> > + 30 - level * 3, clk_user->core->name, consumer,
> > + clk_user->core->enable_count, clk_user->core->prepare_count);
>
> It would be great to not print the core enable count here and instead
> have two levels of enable accounting so we can print the per-user count.
> Basically, one in the 'struct clk_core' and one in the 'struct clk'. If
> that isn't done then this print is going to duplicate the count for
> every 'struct clk' and be meaningless.
>
We will add enable count member to struct clock to represent per user
count and will print that one along with clock and consumer name
> > +
> > + if (clk_user->core->ops->is_enabled)
> > + seq_printf(s, " %8c\n", clk_core_is_enabled(clk_user->core) ? 'Y' : 'N');
> > + else if (!clk_user->core->ops->enable)
> > + seq_printf(s, " %8c\n", 'Y');
> > + else
> > + seq_printf(s, " %8c\n", '?');
>
> I don't think we need any of these prints. They're already covered in
> the summary. And the summary should be able to print the users. See
> regulator_summary_show_subtree() for inspiration. It looks like they
> print "deviceless" for the "no_dev_id" case so maybe just use that
> instead of NULL print.
>
We will remove above prints in the revised patch. We are facing indentation issue whle printing consumer in summary
as given below
enable prepare protect duty hardware per-user
clock count count count rateccuracy phase cycle enable consumer count
clk_mcasp0_fixed 0 0 0 24576000 0 50000 Y
deviceless 0
In this case it will be better to have a separate debugfs entry as
clK_consumer to print clock, consumer and per-user count.
> > + }
> > +}
> > +
> > +static void clk_consumer_show_subtree(struct seq_file *s, struct clk_core *c, int level)
> > +{
> > + struct clk_core *child;
> > +
> > + clk_consumer_show_one(s, c, level);
> > +
> > + hlist_for_each_entry(child, &c->children, child_node)
> > + clk_consumer_show_subtree(s, child, level + 1);
> > +}
> > +
> > +static int clk_consumer_show(struct seq_file *s, void *data)
> > +{
> > + struct clk_core *c;
> > + struct hlist_head **lists = (struct hlist_head **)s->private;
> > +
> > + seq_puts(s, " enable prepare hardware\n");
> > + seq_puts(s, " clock consumer count count enable\n");
> > + seq_puts(s, "-----------------------------------------------------------------------------------------\n");
> > + clk_prepare_lock();
> > +
> > + /*Traversing Linked List to print clock consumer*/
>
> Please run scripts/checkpatch.pl, as this comment needs space after /*
> and before */
>
We will update this in revised patch.
> > +
> > + for (; *lists; lists++)
> > + hlist_for_each_entry(c, *lists, child_node)
> > + clk_consumer_show_subtree(s, c, 0);
> > +
> > + clk_prepare_unlock();
> > +
> > + return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(clk_consumer);
> > +
> > static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
> > {
> > int phase;
> -----Original Message-----
> From: <Vishal Badole> <[email protected]>
> Sent: Sunday, June 26, 2022 1:25 PM
> To: Stephen Boyd <[email protected]>; [email protected]; inux-
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH] Common clock: To list active consumers of clocks
>
...
> We will remove above prints in the revised patch. We are facing
> indentation issue whle printing consumer in summary
> as given below
> enable prepare protect duty hardware per-user
> clock count count count rateccuracy phase cycle enable consumer count
> clk_mcasp0_fixed 0 0 0 24576000 0 50000 Y
> deviceless 0
Consider making the kernel output simple, greppable, and parseable (e.g.,
comma-separated fields, one entry per line, no multi-line column headers)
and let a userspace tool do the fancy formatting.
On Tue, Aug 02, 2022 at 10:49:17PM +0000, Elliott, Robert (Servers) wrote:
>
>
> > -----Original Message-----
> > From: <Vishal Badole> <[email protected]>
> > Sent: Sunday, June 26, 2022 1:25 PM
> > To: Stephen Boyd <[email protected]>; [email protected]; inux-
> > [email protected]; [email protected]
> > Cc: [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH] Common clock: To list active consumers of clocks
> >
> ...
> > We will remove above prints in the revised patch. We are facing
> > indentation issue whle printing consumer in summary
> > as given below
> > enable prepare protect duty hardware per-user
> > clock count count count rateccuracy phase cycle enable consumer count
> > clk_mcasp0_fixed 0 0 0 24576000 0 50000 Y
> > deviceless 0
>
>
> Consider making the kernel output simple, greppable, and parseable (e.g.,
> comma-separated fields, one entry per line, no multi-line column headers)
> and let a userspace tool do the fancy formatting.
>
>
>
>
Hi Robert,
We have raised another patch for the same. Please find the below link
for reference:
https://www.spinics.net/lists/kernel/msg4459705.html
Regards,
Vishal
> -----Original Message-----
> From: <Vishal Badole> <[email protected]>
> Sent: Monday, August 8, 2022 12:00 PM
> To: Elliott, Robert (Servers) <[email protected]>
> Cc: Stephen Boyd <[email protected]>; [email protected]; inux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH] Common clock: To list active consumers of clocks
>
> On Tue, Aug 02, 2022 at 10:49:17PM +0000, Elliott, Robert (Servers) wrote:
> >
> >
> > > -----Original Message-----
> > > From: <Vishal Badole> <[email protected]>
> > > Sent: Sunday, June 26, 2022 1:25 PM
> > > To: Stephen Boyd <[email protected]>; [email protected]; inux-
> > > [email protected]; [email protected]
> > > Cc: [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH] Common clock: To list active consumers of clocks
> > >
> > ...
> > > We will remove above prints in the revised patch. We are facing
> > > indentation issue whle printing consumer in summary
> > > as given below
> > > enable prepare protect
> duty hardware per-user
> > > clock count count count
> rateccuracy phase cycle enable consumer count
> > > clk_mcasp0_fixed 0 0 0
> 24576000 0 50000 Y
> > > deviceless 0
> >
> > Consider making the kernel output simple, greppable, and parseable (e.g.,
> > comma-separated fields, one entry per line, no multi-line column headers)
> > and let a userspace tool do the fancy formatting.
> >
> Hi Robert,
> We have raised another patch for the same. Please find the below link
> for reference:
>
> https://www.spinics.net/lists/kernel/msg4459705.html
That output is still not parsable.
I suggest making the kernel output more like:
clock,enable count,prepare count,protect count,rate,accuracy,phase,duty cycle,hardware enable,consumer,per-user count
clk_mcasp0_fixed,0,0,0,24576000,0,0,50000,Y,deviceless,0
clk_mcasp0,0,0,0,24576000,0,0,50000,N,simple-audio-card;cpu,0
and make a userspace program like lsmod, lscpu, lsblk, lspci,
or lsusb to print the data with fancy columns or apply
other filters.
That allows adding or removing column headers, assuming the
userspace program doesn't hardcode assumptions about them.
On Sun, Aug 21, 2022 at 05:07:00AM +0000, Elliott, Robert (Servers) wrote:
>
>
> > -----Original Message-----
> > From: <Vishal Badole> <[email protected]>
> > Sent: Monday, August 8, 2022 12:00 PM
> > To: Elliott, Robert (Servers) <[email protected]>
> > Cc: Stephen Boyd <[email protected]>; [email protected]; inux-
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [PATCH] Common clock: To list active consumers of clocks
> >
> > On Tue, Aug 02, 2022 at 10:49:17PM +0000, Elliott, Robert (Servers) wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: <Vishal Badole> <[email protected]>
> > > > Sent: Sunday, June 26, 2022 1:25 PM
> > > > To: Stephen Boyd <[email protected]>; [email protected]; inux-
> > > > [email protected]; [email protected]
> > > > Cc: [email protected]; [email protected];
> > > > [email protected]
> > > > Subject: Re: [PATCH] Common clock: To list active consumers of clocks
> > > >
> > > ...
> > > > We will remove above prints in the revised patch. We are facing
> > > > indentation issue whle printing consumer in summary
> > > > as given below
> > > > enable prepare protect
> > duty hardware per-user
> > > > clock count count count
> > rateccuracy phase cycle enable consumer count
> > > > clk_mcasp0_fixed 0 0 0
> > 24576000 0 50000 Y
> > > > deviceless 0
> > >
> > > Consider making the kernel output simple, greppable, and parseable (e.g.,
> > > comma-separated fields, one entry per line, no multi-line column headers)
> > > and let a userspace tool do the fancy formatting.
> > >
> > Hi Robert,
> > We have raised another patch for the same. Please find the below link
> > for reference:
> >
> > https://www.spinics.net/lists/kernel/msg4459705.html
>
> That output is still not parsable.
>
> I suggest making the kernel output more like:
> clock,enable count,prepare count,protect count,rate,accuracy,phase,duty cycle,hardware enable,consumer,per-user count
> clk_mcasp0_fixed,0,0,0,24576000,0,0,50000,Y,deviceless,0
> clk_mcasp0,0,0,0,24576000,0,0,50000,N,simple-audio-card;cpu,0
>
> and make a userspace program like lsmod, lscpu, lsblk, lspci,
> or lsusb to print the data with fancy columns or apply
> other filters.
>
> That allows adding or removing column headers, assuming the
> userspace program doesn't hardcode assumptions about them.
>
Hi Robert,
As per the review given by stephen Boyd, who is one of maintainer of
clk.c, suggested to add consumer's name and per user count in clock
summary only. We are also getting proper formatted and parsable output
on our target board console but when we copy and paste the same in
commit message its format is getting changed. Please apply this patch
and check on your target.
Regards,
Vishal