Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 16 Jun 2022 00:53:27 +0530
From:   "<Vishal Badole>" <badolevishal1116@gmail.com>
To:     Stephen Boyd <sboyd@kernel.org>
Cc:     linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
        mturquette@baylibre.com, mintu patel <mintupatel89@gmail.com>,
        chinmoy ghosh <chinmoyghosh2001@gmail.com>
Subject: Re: [PATCH 5.4] Common =?utf-8?Q?clock=3A_?=
 =?utf-8?B?4oCL4oCLVG8=?= list active consumers of clocks
Message-ID: <20220615192325.GA21290@Mahakal>
References: <CAEXpiVQihEadxsNodarz2-wxSAipfpzEaA8zKpnozszC+weYTQ@mail.gmail.com>
 <20220610194013.DD39DC34114@smtp.kernel.org>
 <CAOUcgRfB-r3aERYeLumExgpTVzsDsBuyOWT+nCJ_OfOv1g0L9g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAOUcgRfB-r3aERYeLumExgpTVzsDsBuyOWT+nCJ_OfOv1g0L9g@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

Hi Stephen,
Thanks for reviewing and giving your valueable review points for this
patch. Shortly I will attach a revised patch based on your review
points.

Thanks,
Vishal
On Tue, Jun 14, 2022 at 11:25:01PM +0530, chinmoy ghosh wrote:
> Agreed here .
> I feel , if  dev_id <https://elixir.bootlin.com/linux/latest/C/ident/dev_id>
> is not from dts property then dont think we can easily find out consumers
> from clk_hw_create_clk.Depends on how the legacy driver maintained the BSP
> code.
> Check the next .
> 
> Thanks
> Chinmoy
> 
> On Sat, Jun 11, 2022 at 1:10 AM Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Why "5.4" in the subject?
> >
> > Quoting Vishal Badole (2022-05-31 11:28:35)
> > > This feature lists the name of clocks and their consumer devices. This
> > > debug feature can be used to check the active clocks and the devices who
> > > have enabled them.
> > >
> > > for example:
> > > debian@beaglebone:~$ cat /sys/kernel/debug/clk/clk_devices_name
> > >             clock_name                                  devices_name
> > >       ----------------
> > > -------------------------------------------------------------------------
> > >              l4-wkup-clkctrl:0008:0              44e07000.target-module
> > >                     l4ls-clkctrl:0074:0
> >  4804c000.target-module
> > >                     l4ls-clkctrl:0058:0
> >  48311fe0.target-module
> > >                  l4-rtc-clkctrl:0000:0
> >  44e3e074.target-module
> > >                           clk_32768_ck
> > > 44e3e000.rtc
> > >                    l4ls-clkctrl:00d8:0
> >  480c8000.target-module
> > >    cpsw-125mhz-clkctrl:0014:0              4a101200.target-module
> > >
> > > Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> > > Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> > > Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> > > Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
> >
> > Where are the Co-developed-by tags? Also, your SoB should be last.
> >
> > > ---
> > >  drivers/clk/Kconfig |   8 ++++
> > >  drivers/clk/clk.c   | 120
> > +++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 127 insertions(+), 1 deletion(-)
> >
> > The patch is malformed. Not sure what's happening with your MUA.
> >
> > >
> > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > > index c44247d..549cdda 100644
> > > --- a/drivers/clk/Kconfig
> > > +++ b/drivers/clk/Kconfig
> > > @@ -23,6 +23,14 @@ config COMMON_CLK
> > >  menu "Common Clock Framework"
> > >   depends on COMMON_CLK
> > >
> > > +config DEBUG_CLK_CONSUMER
> > > + bool "Debug feature to list clocks and their active consumers"
> >
> > Don't think we need a new config for this. Just add it to the existing
> > CONFIG_DEBUGFS area.
> >
> > > + depends on DEBUG_FS && COMMON_CLK
> > > + help
> > > +  Clock consumer debug feature supports for clock debugging. Chose y
> > > +  to get debug entry in file system to list clocks and their active
> > > +  consumer devices.
> > > +
> > >  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 13332f8..dccbd35 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -105,6 +105,84 @@ struct clk {
> > >   struct hlist_node clks_node;
> > >  };
> > >
> > > +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> > > +/*Linked List Node*/
> > > +struct clk_dev_list {
> > > + struct list_head list;
> > > + const char *clk_name;
> > > + const char *dev_name;
> > > +};
> > > +
> > > +/*Declare and init the head node of the linked list*/
> > > +LIST_HEAD(head_node);
> > > +
> > > +static void clk_dev_entry(struct clk *clk_ptr)
> > > +{
> > > + struct clk_dev_list *new_node_ptr = NULL;
> > > + struct clk_dev_list *temp_node_ptr = NULL;
> > > + int repeat_count = 0;
> > > + static bool is_first_node;
> > > + const char *clk_name_ptr = NULL;
> > > + const char *dev_name_ptr = NULL;
> > > +
> > > + if (clk_ptr->dev) {
> > > + dev_name_ptr = dev_name(clk_ptr->dev);
> > > +
> > > + clk_name_ptr = clk_ptr->core->name;
> > > +
> > > + if (is_first_node) {
> > > + /* Iterate the list to check duplicate entry */
> > > + list_for_each_entry(temp_node_ptr, &head_node, list) {
> > > + if (temp_node_ptr->clk_name == clk_name_ptr &&
> > > +    temp_node_ptr->dev_name == dev_name_ptr) {
> > > + repeat_count++;
> > > + break;
> > > + }
> > > + }
> > > + }
> > > +
> > > + is_first_node = 1;
> > > +
> > > + if (!repeat_count && clk_ptr->core->enable_count) {
> > > + /*Creating Node*/
> > > + new_node_ptr = kmalloc(sizeof(*new_node_ptr),
> > > +       GFP_KERNEL);
> > > + if (!new_node_ptr)
> > > + return;
> > > +
> > > + /*Assgin the data that is received*/
> >
> > Typo in Assign.
> >
> > > + new_node_ptr->clk_name = clk_name_ptr;
> > > + new_node_ptr->dev_name = dev_name_ptr;
> > > +
> > > + /*Init the list within the struct*/
> > > + INIT_LIST_HEAD(&new_node_ptr->list);
> > > +
> > > + /*Add Node to Linked List*/
> > > + list_add_tail(&new_node_ptr->list, &head_node);
> > > + }
> > > + }
> > > +}
> > > +
> > > +/* Function to remove the clk and device entry */
> > > +static void clk_dev_dentry(struct clk *clk)
> > > +{
> > > + struct clk_dev_list *temp_node_ptr = NULL;
> > > + struct clk_dev_list *cur_node_ptr = NULL;
> > > +
> > > + if (clk->dev) {
> > > + /* Go through the list and free the memory */
> > > + list_for_each_entry_safe(cur_node_ptr, temp_node_ptr,
> > > + &head_node, list) {
> > > + if (cur_node_ptr->clk_name == clk->core->name &&
> > > +    cur_node_ptr->dev_name == dev_name(clk->dev)) {
> > > + list_del(&cur_node_ptr->list);
> > > + kfree(cur_node_ptr);
> > > + }
> > > + }
> > > + }
> > > +}
> > > +#endif
> > > +
> > >  /***           runtime pm          ***/
> > >  static int clk_pm_runtime_get(struct clk_core *core)
> > >  {
> > > @@ -1020,6 +1098,9 @@ void clk_disable(struct clk *clk)
> > >   return;
> > >
> > >   clk_core_disable_lock(clk->core);
> > > +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> > > + clk_dev_dentry(clk);
> > > +#endif
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_disable);
> > >
> > > @@ -1181,10 +1262,21 @@ EXPORT_SYMBOL_GPL(clk_restore_context);
> > >   */
> > >  int clk_enable(struct clk *clk)
> > >  {
> > > +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> > > + int ret = 0;
> > > +#endif
> > >   if (!clk)
> > >   return 0;
> > >
> > > +#ifndef CONFIG_DEBUG_CLK_CONSUMER
> > >   return clk_core_enable_lock(clk->core);
> > > +#else
> > > + ret = clk_core_enable_lock(clk->core);
> > > + if (!ret)
> > > + clk_dev_entry(clk);
> > > +
> > > + return ret;
> > > +#endif
> >
> > Not sure what this is doing.
> >
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_enable);
> > >
> > > @@ -2986,6 +3078,29 @@ static void clk_dump_one(struct seq_file *s,
> > struct
> > > clk_core *c, int level)
> > >     clk_core_get_scaled_duty_cycle(c, 100000));
> > >  }
> > >
> > > +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> > > +static int clk_devices_show(struct seq_file *s, void *data)
> > > +{
> > > + struct clk_dev_list *clk_dev_node;
> > > +
> > > + seq_puts(s, "            clock_name
> > >  devices_name\n");
> > > + seq_puts(s,
> > >
> > "-------------------------------------------------------------------------\n");
> > > +
> > > + clk_prepare_lock();
> > > +
> > > + /*Traversing Linked List and Print its Members*/
> > > + list_for_each_entry(clk_dev_node, &head_node, list) {
> >
> > It's hard to read but we already have a list of all clk consumers for a
> > clk_hw pointer, see clk_core_link_consumer(). And we stash the device
> > consuming it with clk_hw_create_clk(). That should be sufficient.
> >
> >
> > > + seq_printf(s, "%35s %35s\n", clk_dev_node->clk_name,
> > > +   clk_dev_node->dev_name);
> > > + }
> > > +
> > > + clk_prepare_unlock();
> > > +
> > > + return 0;
> > > +}
> > > +DEFINE_SHOW_ATTRIBUTE(clk_devices);
> > > +#endif
> > > +
> > >  static void clk_dump_subtree(struct seq_file *s, struct clk_core *c, int
> > > level)
> > >  {
> > >   struct clk_core *child;
> > > @@ -3256,7 +3371,10 @@ static int __init clk_debug_init(void)
> > >      &clk_summary_fops);
> > >   debugfs_create_file("clk_orphan_dump", 0444, rootdir, &orphan_list,
> > >      &clk_dump_fops);
> > > -
> > > +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> > > + debugfs_create_file("clk_devices_name", 0444, rootdir, NULL,
> >
> > Call it 'clk_consumers' please.
> >
> > > +    &clk_devices_fops);
> >