RE: [PATCH V6 03/12] clk: imx: scu: add two cells binding support

[Aisheng Dong <aisheng.dong@nxp.com>]

Hi Stephen,

Thanks for the review.

> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Tuesday, May 5, 2020 1:08 PM
> 
> Quoting Dong Aisheng (2020-03-15 06:43:47)
> > This patch implements the new two cells binding for SCU clocks.
> > The usage is as follows:
> > clocks = <&uart0_clk IMX_SC_R_UART_0 IMX_SC_PM_CLK_PER>
> >
> > Due to each SCU clock is associated with a power domain, without power
> > on the domain, the SCU clock can't work. So we create platform devices
> > for each domain clock respectively and manually attach the required
> > domain before register the clock devices, then we can register clocks
> > in the clock platform driver accordingly.
> 
> That's odd. See below.
> 
> >
> > Note because we do not have power domain info in device tree and the
> > SCU resource ID is the same for power domain and clock, so we use
> > resource ID to find power domains.
> >
> > Later, we will also use this clock platform driver to support
> > suspend/resume and runtime pm.
> >
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> [...]
> > diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> > index b8b2072742a5..4fadff14d8b2 100644
> > --- a/drivers/clk/imx/clk-scu.c
> > +++ b/drivers/clk/imx/clk-scu.c
> > @@ -8,6 +8,9 @@
> >  #include <linux/arm-smccc.h>
> >  #include <linux/clk-provider.h>
> >  #include <linux/err.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> >  #include <linux/slab.h>
> >
> >  #include "clk-scu.h"
> > @@ -16,6 +19,20 @@
> >  #define IMX_SIP_SET_CPUFREQ            0x00
> >
> >  static struct imx_sc_ipc *ccm_ipc_handle;
> > +struct device_node *pd_np;
> > +
> > +struct imx_scu_clk_node {
> > +       const char *name;
> > +       u32 rsrc;
> > +       u8 clk_type;
> > +       const char * const *parents;
> > +       int num_parents;
> > +
> > +       struct clk_hw *hw;
> > +       struct list_head node;
> > +};
> > +
> > +struct list_head imx_scu_clks[IMX_SC_R_LAST];
> >
> >  /*
> >   * struct clk_scu - Description of one SCU clock @@ -128,9 +145,32 @@
> > static inline struct clk_scu *to_clk_scu(struct clk_hw *hw)
> >         return container_of(hw, struct clk_scu, hw);  }
> >
> > -int imx_clk_scu_init(void)
> > +int imx_clk_scu_init(struct device_node *np)
> >  {
> > -       return imx_scu_get_handle(&ccm_ipc_handle);
> > +       struct platform_device *pd_dev;
> > +       u32 clk_cells;
> > +       int ret, i;
> > +
> > +       ret = imx_scu_get_handle(&ccm_ipc_handle);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (of_property_read_u32(np, "#clock-cells", &clk_cells))
> 
> Why wouldn't there be #clock-cells in the node?

Okay, will remove the check.

> 
> > +               return -EINVAL;
> > +
> > +       if (clk_cells == 2) {
> > +               for (i = 0; i < IMX_SC_R_LAST; i++)
> > +                       INIT_LIST_HEAD(&imx_scu_clks[i]);
> > +
> > +               pd_np = of_find_compatible_node(NULL, NULL,
> "fsl,scu-pd");
> > +               pd_dev = of_find_device_by_node(pd_np);
> > +               if (!pd_dev || !device_is_bound(&pd_dev->dev)) {
> 
> What is device_is_bound() check for? Add a comment?

Yes, I can add a comment in the code.
It is because SCU clock driver depends on SCU power domain to be ready first.

> 
> > +                       of_node_put(pd_np);
> > +                       return -EPROBE_DEFER;
> > +               }
> > +       }
> > +
> > +       return 0;
> >  }
> >
> >  /*
> > @@ -387,3 +427,99 @@ struct clk_hw *__imx_clk_scu(const char *name,
> > const char * const *parents,
> >
> >         return hw;
> >  }
> > +
> > +struct clk_hw *imx_scu_of_clk_src_get(struct of_phandle_args *clkspec,
> > +                                     void *data) {
> > +       unsigned int rsrc = clkspec->args[0];
> > +       unsigned int idx = clkspec->args[1];
> > +       struct list_head *scu_clks = data;
> > +       struct imx_scu_clk_node *clk;
> > +
> > +       list_for_each_entry(clk, &scu_clks[rsrc], node) {
> > +               if (clk->clk_type == idx)
> > +                       return clk->hw;
> > +       }
> > +
> > +       return ERR_PTR(-ENODEV);
> > +}
> > +
> > +static int imx_clk_scu_probe(struct platform_device *pdev) {
> > +       struct device *dev = &pdev->dev;
> > +       struct imx_scu_clk_node *clk = dev_get_platdata(dev);
> > +       struct clk_hw *hw;
> > +
> > +       hw = __imx_clk_scu(clk->name, clk->parents, clk->num_parents,
> > +                          clk->rsrc, clk->clk_type);
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       clk->hw = hw;
> > +       list_add_tail(&clk->node, &imx_scu_clks[clk->rsrc]);
> > +
> > +       dev_dbg(dev, "register SCU clock rsrc:%d type:%d\n", clk->rsrc,
> > +               clk->clk_type);
> > +
> > +       return 0;
> > +}
> > +
> > +static struct platform_driver imx_clk_scu_driver = {
> > +       .driver = {
> > +               .name = "imx-scu-clk",
> > +               .suppress_bind_attrs = true,
> > +       },
> > +       .probe = imx_clk_scu_probe,
> > +};
> > +builtin_platform_driver(imx_clk_scu_driver);
> > +
> > +static int imx_clk_scu_attach_pd(struct device *dev, u32 rsrc_id) {
> > +       struct of_phandle_args genpdspec = {
> > +               .np = pd_np,
> > +               .args_count = 1,
> > +               .args[0] = rsrc_id,
> > +       };
> > +
> > +       return of_genpd_add_device(&genpdspec, dev); }
> > +
> > +struct clk_hw *imx_clk_scu_alloc_dev(const char *name,
> > +                                    const char * const *parents,
> > +                                    int num_parents, u32 rsrc_id, u8
> > +clk_type) {
> > +       struct imx_scu_clk_node clk = {
> > +               .name = name,
> > +               .rsrc = rsrc_id,
> > +               .clk_type = clk_type,
> > +               .parents = parents,
> > +               .num_parents = num_parents,
> > +       };
> > +       struct platform_device *pdev;
> > +       int ret;
> > +
> > +       pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE);
> > +       if (!pdev) {
> > +               pr_err("%s: failed to allocate scu clk dev rsrc %d
> type %d\n",
> > +                      name, rsrc_id, clk_type);
> > +               return ERR_PTR(-ENOMEM);
> > +       }
> > +
> > +       ret = platform_device_add_data(pdev, &clk, sizeof(clk));
> > +       if (ret) {
> > +               platform_device_put(pdev);
> > +               return ERR_PTR(ret);
> > +       }
> > +
> > +       pdev->driver_override = "imx-scu-clk";
> > +
> > +       ret = imx_clk_scu_attach_pd(&pdev->dev, rsrc_id);
> 
> Why do we have to allocate a device for each power domain? 

This is mainly for each clock runtime pm and suspend/resume support as they're
independent with each other.

> Is this because we
> don't have support for one device being in multiple power domains? That is
> supported now as far as I recall, by basically making dummy platform devices
> like this. 

I know kernel supports multi power domains, but I didn't realize it could be used
for our case.

> So maybe this code isn't necessary and we can have one platform
> device for the clk controller and then have it control certain power domains
> manually from runtime PM callbacks? It's possible the runtime PM callbacks are
> too simple for this case and we need to tell clk providers what clk is having
> runtime PM enabled for it.

To make sure I understand correctly, do you mean we use only one general clk controller
Runtime pm callback to handle all clocks runtime pm status manually?
If doing that, how do we handle different clocks pm requirements with only one device runtime
pm status (clock controller)?
e.g.
One Clock Provider
Consumer A -> Clock A -> Clock Provider resumed -> Clock A resumed
Consumer B -> Clock B (Since Clock Provided is already resumed, no chance to run callback to resume Clock B now).
(Note: assume all clocks need runtime pm enabled for i.MX case)

Or you mean we simply resume all clocks? but that seems lose the granularity
and possibly have no chance to enter runtime suspend anymore once there was one clock in use.

Not sure if I missed something. Please help clarify a bit more.

Right now, I'm a bit afraid this may make things a bit complicated as we have ~150 clocks
and ~150 power domains. Putting them all under one clock controller node in DT may scare people.
And even we did not create platform devices for each clock in the clock driver, using multi-pd
will still result in creating dummy platform devices for each clock automatically by power domain
framework. That means we didn't save any platform devices.

> Maybe we can adjust the core clk framework to introduce a callback for the clk
> that is runtime PM enabling and put the logic there about what to do?

That may help. Since we still only have one device for runtime pm state management,
Still not understand how to do it as it may mix the usage with the runtime pm framework.

Regards
Aisheng
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel