diff mbox series

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

Message ID 1584279836-29825-4-git-send-email-aisheng.dong@nxp.com (mailing list archive)
State New, archived
Headers show
Series clk: imx8: add new clock binding for better pm support | expand

Commit Message

Aisheng Dong March 15, 2020, 1:43 p.m. UTC 
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.

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>
---
ChangeLog:
v4->v5:
 * remove global clock_cells
 * put node after using of_find_compatible_node()
 * using ERR_PTR(ret) instead of ERR_PTR(-ENOMEM)
v4: no changes
v3: new patch
---
 drivers/clk/imx/clk-imx8qxp.c | 129 ++++++++++++++++---------------
 drivers/clk/imx/clk-scu.c     | 140 +++++++++++++++++++++++++++++++++-
 drivers/clk/imx/clk-scu.h     |  25 ++++--
 3 files changed, 227 insertions(+), 67 deletions(-)

Comments

Stephen Boyd May 5, 2020, 5:08 a.m. UTC | #1 
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?

> +               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?

> +                       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? 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. 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.
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?
Aisheng Dong May 5, 2020, 1:47 p.m. UTC | #2 
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
Aisheng Dong June 19, 2020, 2:50 p.m. UTC | #3 
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Friday, June 19, 2020 12:05 AM
> 
> Quoting Aisheng Dong (2020-06-01 07:28:57)
> > > From: Stephen Boyd <sboyd@kernel.org>
> > > Sent: Wednesday, May 27, 2020 4:34 PM
> > >
> > > Quoting Aisheng Dong (2020-05-05 06:47:02)
> > > > 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
> > > >
> > > > >
> > > > > > +               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.
> > >
> > > Does EPROBE_DEFER not kick in automatically in that case?
> > >
> >
> > No, there're no power domains under scu clock node in dts.
> > So can't use PROBE_DEFER automatically.
> 
> Why isn't the scu clock node using power domains?
> 

We have tried this in several ways before but finally didn't go that way
due to various reasons.

e.g.
Method 1:
conn-scu-clock-controller {
        compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
        #address-cells = <1>;
        #size-cells = <0>;

        uart0_clk: clock-scu@57 {
        reg = <57>; 
                #clock-cells = <1>;
                clock-indices = <IMX_SC_PM_CLK_PER>;
                clock-output-names = "uart0_clk";
                power-domains = <&pd IMX_SC_R_UART_0>;
        };
        ...
}

Method 2:
#define IMX_SCU_CLK_ID(rsrc, type)      (type << 16 | rsrc)
scu_clk: scu-clock-controller {
        compatible = "fsl,imx8qxp-scu-clk", "fsl,scu-clk";
        #clock-cells = <1>;
        clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_BYPASS)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_MISC0)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>,
                        ...

        clock-output-names = "enet0_clk",
                             "enet0_bypass_clk",
                             "enet0_rgmii_clk",
                             "uart0_clk",
                             ...

        power-domains = <&pd IMX_SC_R_ENET_0>,
                        <&pd IMX_SC_R_ENET_0>,
                        <&pd IMX_SC_R_ENET_0>,
                        <&pd IMX_SC_R_UART_0>,
                        ...
};

See details here:
https://spinics.net/lists/devicetree/msg298642.html

> >
> > > > > > +
> > > > > > +       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
> > >
> > > Typically I see HW designers make one hardware block for a certain
> > > group of clks, like general purpose IO hardware, multimedia hardware, DSP
> hardware, etc.
> > > That isn't always true though. Sometimes I see the hardware
> > > designers decide to stamp down the same hard macro for a clk in
> > > every IP block they develop in the SoC. It gets interesting when
> > > they have to integrate with a third party core like dwc3. Usually
> > > they look to see if they can add some vendor specific register to the IP block
> or make a wrapper around the IP block with the SoC glue bits.
> > >
> > > This usually happens when the clk hard macro library is used by all
> > > the hardware designers. Instead of consolidating that library into a
> > > hardware IP block that gets delivered for the particular SoC they
> > > just spread the hardware block around the system and hope that software
> will figure it out.
> > >
> > > I suspect hardware blocks are given certain addresses on the MMIO
> > > bus because ARM's AMBA/AHB spec mandated that a "device" starts at a 1K
> aligned address.
> > > This probably forced SoC hardware engineers to split their hardware
> > > blocks up into different pieces that they could then plug into the
> > > bus and easily route CPU addresses to different slaves on the bus.
> > >
> > > What's the situation here? I thought that this was a firmware
> > > interface that logically combines all the hardware block clk
> > > controllers. The firmware interface doesn't do everything though
> > > because it seems we still have to power on power domains to use various
> clks?
> >
> > Yes, it is.
> > For MX8QM/QXP, the whole SoC is comprised of a few HW Subsystems.
> > Those HW Subsystems are independent with each other as they have
> > separate power/clock Controllers.
> > Within each subsystem, there's a DSC (distributed slave controller)
> > acting as a standard interface to System Controller (SCU) to provide
> power/clock/reset and some other misc control functions.
> 
> If it's a few then there are maybe 5 SCUs in the SoC?
> 

There's only one SCU in the SoC running on a dedicated M4 core.
SCU communicated with each Subsystem via standard DSC interface
to provide centralized clock/power/reset/pinctrl/resource management/misc services.

> >
> > >
> > > > 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.
> > >
> > > What software entity is providing the power domains? I wonder if
> > > things are going about all wrong and the power domains are provided
> > > by the same piece of code that's managing the clks?
> >
> > There's a separate power domain driver based on SCU protocol for only power
> control.
> > This power control is a lit complicated as it supports multi low power states.
> > When power is completely gated, the HW state will be lost and need restore
> by SW.
> > Some of those features are still not upstreamed.
> >
> > > If so, why can't we control the power domains directly from the clk
> > > code without having to go through runtime PM layer or genpd?
> >
> > It's probably hard to do that because:
> > 1. power domain supports multiple low power states for different purpose.
> > Now genpd seems a better place to handle it 2. Not all power domains
> > associated with a clock.
> >
> > > Or to flip it the other way, why do we need clk control for
> > > enable/disable in that case?
> > > Can we just expose rate control for the clks and then let genpd
> > > handle turning clks on an off if they're associated with that power
> > > domain?
> >
> > I wonder this way might also lose the granularity control capability.
> > For a simple device with only one clock, we might be able to do it at most
> times.
> > But for some complicated devices with multiple clocks and which clocks
> > to use depends on the real user cases, simply enable all clocks when PD is UP
> seems not very suitable.
> > For MX8, it's a complicated systems and some device resources may have
> > multi clocks, especially for Audio, Video IO, Display related cases.
> 
> Do they have multiple clks and multiple power domains for one device, like
> Audio or Video? Is it 1:1 between some number of clks and some power
> domain?
> 

For simple devices with only one clock, it might be 1:1.
But for complicated devices with multiple clocks, it's usually not 1:1.
e.g.
AUDIO_PLL_0     PLL     User programmable PLL
AUDIO_PLL_0     MISC0   Audio PLL div 0
AUDIO_PLL_0     MISC1   Audio rec 0

DC_0    MISC0   Display 0
DC_0    MISC1   Display 1

MIPI_0  BYPASS  Bypass
MIPI_0  SLV_BUS DSI rx escape
MIPI_0  MST_BUS DSI tx escape
MIPI_0  PHY     DPHY PLL ref
MIPI_0  PER     DPI - pixel

> >
> > >
> > > >
> > > > > 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.
> > > >
> > >
> > > Right. I'm thinking that we may need a clk_op that is called when a
> > > clk is runtime PM enabled so the device driver that provides that
> > > clk can decide what genpds to manually enable. This would only be
> > > used in the case where the clks are provided by a clk controller
> > > that exists in multiple power domains. I'm not super clear on
> > > multi-pd and how it interacts with runtime PM but I assume that it
> > > doesn't actually turn on any genpd when the consumer device is runtime
> PM active. Instead the consumer driver has to figure out what to power on by
> itself.
> > > The callback would do this similarly from the CCF so that the manual
> > > steps could be done again.
> >
> > After more thinking, I guess we probably don't need a clk_op if we
> > register the correct struct device (abstract clk provider) to clk register API. E.g.
> clk_hw_register(dev, hw).
> > That abstract struct dev could be the virtual power domain device
> > created and returned by PD framework when calling multi PD APIs. This
> > can make us be able to fully reuse all the current rumtime PM support
> > in CCF. Nothing else special need to care. (synchronization work with
> > rpm/genpd framework)
> 
> Whatever 'dev' is used to register the clk is presumed to be the device that is the
> clk provider. If 'dev' isn't the one that's matched to the device node in DT the
> CCF will become confused about what parent clks are supposed to be. That's
> one problem but it may not be a problem here because this is just one big DT
> node?
> 
> >
> > See: multi pd API will create a virtual PD device to be associated with that PD
> for later RPM usages.
> > struct device *dev_pm_domain_attach_by_name(struct device *dev,
> >                                             const char *name) {
> >         if (dev->pm_domain)
> >                 return ERR_PTR(-EEXIST);
> >
> >         return genpd_dev_pm_attach_by_name(dev, name); }
> > EXPORT_SYMBOL_GPL(dev_pm_domain_attach_by_name);
> >
> > However, I'm still a bit feeling this probably could work for a clock controller
> simply with <10 PDs.
> > But not very suitable for MX8QM/QXP as it has 150+ PDs. Defining them
> > all will make DT looks terrible and drivers slightly a bit complicated
> > to fetch those PDs efficiently from DT for each clock, also make the future
> maintain work a bit harder when adding new clocks.
> > Not sure whether it's worth to do that as compared with current
> > approach, it really does not Improve too much as multi pd also need create
> virtual devices if the virtual devices are the only concern.
> > In contrast, it might increase, probably unnecessary complexities.
> 
> Either way there are going to be 150+ devices for the 150+ power domains in
> the system.  Why are there so many power domains? That seems like the
> problem. I doubt there are 150 devices in the SoC, or even 75 of them.
> 

It's abstract power domain concept introduced by SCU firmware that each device
Is associated with a power domain.
In real HW, many devices within the same Subsystem might share one of the same HW
power domain. But SCU firmware hides those internal details completely to users.
So users are unaware of it.

Additionally, power domain are also associated with device resource management by
xRDC (Resource Domain Controller) component for security reasons.

Without enable the resource power by SCU firmware, one SW execution environment 
(e.g. OS/ATF/OPTEE/VM) can't access those resource like registers, clocks and etc.

> >
> > How would you suggest?
> > Do you think if we should continue to investigate this Or if we can
> > make the function work first as this improvement work does not affect users?
> > Currently as this patch is a very fundamental change which blocks a lot other
> drivers upstream work.
> >
> 
> If you want to go ahead with this patch I guess I will stop caring.

Thanks a lot

> Just be aware
> that needing to make virtual devices to fit this into the kernel looks wrong. I'd
> like to see one device node for the firmware interface of clks, and one struct
> device for said firmware interface that attaches to one struct driver for that
> firmware interface.
> 

So far, I'm still no sure if we can finally archive that due to complexities of
various requirements in MX8. Personally i feel a bit risky and trying it definitely need
lot more time to ensure it can meet all of our feature requirements on MX8.
But I did understand your concern. Anyway, I will continue to investigate.
However, if you do not mind too much, I'd treat it as a continued improvement work
in order to not block the whole MX8 upstream work too long.
(dt binding part of users won't change)

The current approach has been verified working well in NXP official released 5.4 kernel and
can meet various product requirements. Probably could be a good start point for the reset 
drivers upstreaming work first.

> Connecting that to genpd for proper power domain management is another
> issue. If the core clk framework needs to be multi-device power domain aware
> so that various clks can turn on along with the power domain that's needed to
> operate them then we'll need to add that in the core framework.
> 

Yes. I guess the problem for MX8 is how to co-work with the exist power domain driver.
For MX8, there're also more issues because MX8 power domain supports multiple lower power
States. And we need a genpd governer to help choose a state. That's the future work we may do
In the next step.
If we power up clock power in CCF, we need think how to do It well by co-exist with genpd framework.

> I'm amazed that the firmware interface doesn't handle the requirement that
> genpds need to be powered on to control the clks. I'd think the firmware would
> be happy to turn on power domains so that it can go poke clk registers.

I guess one reason may be SCU support multiple low power states.
It wants user to choose which one to enter.
/*
 * Defines for SC PM Power Mode
 */
#define IMX_SC_PM_PW_MODE_OFF   0       /* Power off */
#define IMX_SC_PM_PW_MODE_STBY  1       /* Power in standby */
#define IMX_SC_PM_PW_MODE_LP    2       /* Power in low-power */
#define IMX_SC_PM_PW_MODE_ON    3       /* Power on */

Regards
Aisheng
diff mbox series

Patch

diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c
index 5e2903efc488..2ec3e0c4749d 100644
--- a/drivers/clk/imx/clk-imx8qxp.c
+++ b/drivers/clk/imx/clk-imx8qxp.c
@@ -22,9 +22,10 @@  static int imx8qxp_clk_probe(struct platform_device *pdev)
 	struct device_node *ccm_node = pdev->dev.of_node;
 	struct clk_hw_onecell_data *clk_data;
 	struct clk_hw **clks;
+	u32 clk_cells;
 	int ret, i;
 
-	ret = imx_clk_scu_init();
+	ret = imx_clk_scu_init(ccm_node);
 	if (ret)
 		return ret;
 
@@ -33,6 +34,9 @@  static int imx8qxp_clk_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
+	if (of_property_read_u32(ccm_node, "#clock-cells", &clk_cells))
+		return -EINVAL;
+
 	clk_data->num = IMX_SCU_CLK_END;
 	clks = clk_data->hws;
 
@@ -55,78 +59,78 @@  static int imx8qxp_clk_probe(struct platform_device *pdev)
 	clks[IMX_LSIO_BUS_CLK]		= clk_hw_register_fixed_rate(NULL, "lsio_bus_clk_root", NULL, 0, 100000000);
 
 	/* ARM core */
-	clks[IMX_A35_CLK]		= imx_clk_scu("a35_clk", IMX_SC_R_A35, IMX_SC_PM_CLK_CPU);
+	clks[IMX_A35_CLK]		= imx_clk_scu("a35_clk", IMX_SC_R_A35, IMX_SC_PM_CLK_CPU, clk_cells);
 
 	/* LSIO SS */
-	clks[IMX_LSIO_PWM0_CLK]		= imx_clk_scu("pwm0_clk", IMX_SC_R_PWM_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_PWM1_CLK]		= imx_clk_scu("pwm1_clk", IMX_SC_R_PWM_1, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_PWM2_CLK]		= imx_clk_scu("pwm2_clk", IMX_SC_R_PWM_2, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_PWM3_CLK]		= imx_clk_scu("pwm3_clk", IMX_SC_R_PWM_3, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_PWM4_CLK]		= imx_clk_scu("pwm4_clk", IMX_SC_R_PWM_4, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_PWM5_CLK]		= imx_clk_scu("pwm5_clk", IMX_SC_R_PWM_5, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_PWM6_CLK]		= imx_clk_scu("pwm6_clk", IMX_SC_R_PWM_6, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_PWM7_CLK]		= imx_clk_scu("pwm7_clk", IMX_SC_R_PWM_7, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_GPT0_CLK]		= imx_clk_scu("gpt0_clk", IMX_SC_R_GPT_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_GPT1_CLK]		= imx_clk_scu("gpt1_clk", IMX_SC_R_GPT_1, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_GPT2_CLK]		= imx_clk_scu("gpt2_clk", IMX_SC_R_GPT_2, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_GPT3_CLK]		= imx_clk_scu("gpt3_clk", IMX_SC_R_GPT_3, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_GPT4_CLK]		= imx_clk_scu("gpt4_clk", IMX_SC_R_GPT_4, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_FSPI0_CLK]	= imx_clk_scu("fspi0_clk", IMX_SC_R_FSPI_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_LSIO_FSPI1_CLK]	= imx_clk_scu("fspi1_clk", IMX_SC_R_FSPI_1, IMX_SC_PM_CLK_PER);
+	clks[IMX_LSIO_PWM0_CLK]		= imx_clk_scu("pwm0_clk", IMX_SC_R_PWM_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_PWM1_CLK]		= imx_clk_scu("pwm1_clk", IMX_SC_R_PWM_1, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_PWM2_CLK]		= imx_clk_scu("pwm2_clk", IMX_SC_R_PWM_2, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_PWM3_CLK]		= imx_clk_scu("pwm3_clk", IMX_SC_R_PWM_3, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_PWM4_CLK]		= imx_clk_scu("pwm4_clk", IMX_SC_R_PWM_4, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_PWM5_CLK]		= imx_clk_scu("pwm5_clk", IMX_SC_R_PWM_5, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_PWM6_CLK]		= imx_clk_scu("pwm6_clk", IMX_SC_R_PWM_6, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_PWM7_CLK]		= imx_clk_scu("pwm7_clk", IMX_SC_R_PWM_7, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_GPT0_CLK]		= imx_clk_scu("gpt0_clk", IMX_SC_R_GPT_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_GPT1_CLK]		= imx_clk_scu("gpt1_clk", IMX_SC_R_GPT_1, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_GPT2_CLK]		= imx_clk_scu("gpt2_clk", IMX_SC_R_GPT_2, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_GPT3_CLK]		= imx_clk_scu("gpt3_clk", IMX_SC_R_GPT_3, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_GPT4_CLK]		= imx_clk_scu("gpt4_clk", IMX_SC_R_GPT_4, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_FSPI0_CLK]	= imx_clk_scu("fspi0_clk", IMX_SC_R_FSPI_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_LSIO_FSPI1_CLK]	= imx_clk_scu("fspi1_clk", IMX_SC_R_FSPI_1, IMX_SC_PM_CLK_PER, clk_cells);
 
 	/* ADMA SS */
-	clks[IMX_ADMA_UART0_CLK]	= imx_clk_scu("uart0_clk", IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_UART1_CLK]	= imx_clk_scu("uart1_clk", IMX_SC_R_UART_1, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_UART2_CLK]	= imx_clk_scu("uart2_clk", IMX_SC_R_UART_2, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_UART3_CLK]	= imx_clk_scu("uart3_clk", IMX_SC_R_UART_3, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_SPI0_CLK]		= imx_clk_scu("spi0_clk",  IMX_SC_R_SPI_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_SPI1_CLK]		= imx_clk_scu("spi1_clk",  IMX_SC_R_SPI_1, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_SPI2_CLK]		= imx_clk_scu("spi2_clk",  IMX_SC_R_SPI_2, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_SPI3_CLK]		= imx_clk_scu("spi3_clk",  IMX_SC_R_SPI_3, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_CAN0_CLK]		= imx_clk_scu("can0_clk",  IMX_SC_R_CAN_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_I2C0_CLK]		= imx_clk_scu("i2c0_clk",  IMX_SC_R_I2C_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_I2C1_CLK]		= imx_clk_scu("i2c1_clk",  IMX_SC_R_I2C_1, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_I2C2_CLK]		= imx_clk_scu("i2c2_clk",  IMX_SC_R_I2C_2, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_I2C3_CLK]		= imx_clk_scu("i2c3_clk",  IMX_SC_R_I2C_3, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_FTM0_CLK]		= imx_clk_scu("ftm0_clk",  IMX_SC_R_FTM_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_FTM1_CLK]		= imx_clk_scu("ftm1_clk",  IMX_SC_R_FTM_1, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_ADC0_CLK]		= imx_clk_scu("adc0_clk",  IMX_SC_R_ADC_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_PWM_CLK]		= imx_clk_scu("pwm_clk",   IMX_SC_R_LCD_0_PWM_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_ADMA_LCD_CLK]		= imx_clk_scu("lcd_clk",   IMX_SC_R_LCD_0, IMX_SC_PM_CLK_PER);
+	clks[IMX_ADMA_UART0_CLK]	= imx_clk_scu("uart0_clk", IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_UART1_CLK]	= imx_clk_scu("uart1_clk", IMX_SC_R_UART_1, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_UART2_CLK]	= imx_clk_scu("uart2_clk", IMX_SC_R_UART_2, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_UART3_CLK]	= imx_clk_scu("uart3_clk", IMX_SC_R_UART_3, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_SPI0_CLK]		= imx_clk_scu("spi0_clk",  IMX_SC_R_SPI_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_SPI1_CLK]		= imx_clk_scu("spi1_clk",  IMX_SC_R_SPI_1, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_SPI2_CLK]		= imx_clk_scu("spi2_clk",  IMX_SC_R_SPI_2, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_SPI3_CLK]		= imx_clk_scu("spi3_clk",  IMX_SC_R_SPI_3, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_CAN0_CLK]		= imx_clk_scu("can0_clk",  IMX_SC_R_CAN_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_I2C0_CLK]		= imx_clk_scu("i2c0_clk",  IMX_SC_R_I2C_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_I2C1_CLK]		= imx_clk_scu("i2c1_clk",  IMX_SC_R_I2C_1, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_I2C2_CLK]		= imx_clk_scu("i2c2_clk",  IMX_SC_R_I2C_2, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_I2C3_CLK]		= imx_clk_scu("i2c3_clk",  IMX_SC_R_I2C_3, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_FTM0_CLK]		= imx_clk_scu("ftm0_clk",  IMX_SC_R_FTM_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_FTM1_CLK]		= imx_clk_scu("ftm1_clk",  IMX_SC_R_FTM_1, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_ADC0_CLK]		= imx_clk_scu("adc0_clk",  IMX_SC_R_ADC_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_PWM_CLK]		= imx_clk_scu("pwm_clk",   IMX_SC_R_LCD_0_PWM_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_ADMA_LCD_CLK]		= imx_clk_scu("lcd_clk",   IMX_SC_R_LCD_0, IMX_SC_PM_CLK_PER, clk_cells);
 
 	/* Connectivity */
-	clks[IMX_CONN_SDHC0_CLK]	= imx_clk_scu("sdhc0_clk", IMX_SC_R_SDHC_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_CONN_SDHC1_CLK]	= imx_clk_scu("sdhc1_clk", IMX_SC_R_SDHC_1, IMX_SC_PM_CLK_PER);
-	clks[IMX_CONN_SDHC2_CLK]	= imx_clk_scu("sdhc2_clk", IMX_SC_R_SDHC_2, IMX_SC_PM_CLK_PER);
-	clks[IMX_CONN_ENET0_ROOT_CLK]	= imx_clk_scu("enet0_clk", IMX_SC_R_ENET_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_CONN_ENET0_BYPASS_CLK]	= imx_clk_scu("enet0_bypass_clk", IMX_SC_R_ENET_0, IMX_SC_PM_CLK_BYPASS);
-	clks[IMX_CONN_ENET0_RGMII_CLK]	= imx_clk_scu("enet0_rgmii_clk", IMX_SC_R_ENET_0, IMX_SC_PM_CLK_MISC0);
-	clks[IMX_CONN_ENET1_ROOT_CLK]	= imx_clk_scu("enet1_clk", IMX_SC_R_ENET_1, IMX_SC_PM_CLK_PER);
-	clks[IMX_CONN_ENET1_BYPASS_CLK]	= imx_clk_scu("enet1_bypass_clk", IMX_SC_R_ENET_1, IMX_SC_PM_CLK_BYPASS);
-	clks[IMX_CONN_ENET1_RGMII_CLK]	= imx_clk_scu("enet1_rgmii_clk", IMX_SC_R_ENET_1, IMX_SC_PM_CLK_MISC0);
-	clks[IMX_CONN_GPMI_BCH_IO_CLK]	= imx_clk_scu("gpmi_io_clk", IMX_SC_R_NAND, IMX_SC_PM_CLK_MST_BUS);
-	clks[IMX_CONN_GPMI_BCH_CLK]	= imx_clk_scu("gpmi_bch_clk", IMX_SC_R_NAND, IMX_SC_PM_CLK_PER);
-	clks[IMX_CONN_USB2_ACLK]	= imx_clk_scu("usb3_aclk_div", IMX_SC_R_USB_2, IMX_SC_PM_CLK_PER);
-	clks[IMX_CONN_USB2_BUS_CLK]	= imx_clk_scu("usb3_bus_div", IMX_SC_R_USB_2, IMX_SC_PM_CLK_MST_BUS);
-	clks[IMX_CONN_USB2_LPM_CLK]	= imx_clk_scu("usb3_lpm_div", IMX_SC_R_USB_2, IMX_SC_PM_CLK_MISC);
+	clks[IMX_CONN_SDHC0_CLK]	= imx_clk_scu("sdhc0_clk", IMX_SC_R_SDHC_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_CONN_SDHC1_CLK]	= imx_clk_scu("sdhc1_clk", IMX_SC_R_SDHC_1, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_CONN_SDHC2_CLK]	= imx_clk_scu("sdhc2_clk", IMX_SC_R_SDHC_2, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_CONN_ENET0_ROOT_CLK]	= imx_clk_scu("enet0_clk", IMX_SC_R_ENET_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_CONN_ENET0_BYPASS_CLK]	= imx_clk_scu("enet0_bypass_clk", IMX_SC_R_ENET_0, IMX_SC_PM_CLK_BYPASS, clk_cells);
+	clks[IMX_CONN_ENET0_RGMII_CLK]	= imx_clk_scu("enet0_rgmii_clk", IMX_SC_R_ENET_0, IMX_SC_PM_CLK_MISC0, clk_cells);
+	clks[IMX_CONN_ENET1_ROOT_CLK]	= imx_clk_scu("enet1_clk", IMX_SC_R_ENET_1, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_CONN_ENET1_BYPASS_CLK]	= imx_clk_scu("enet1_bypass_clk", IMX_SC_R_ENET_1, IMX_SC_PM_CLK_BYPASS, clk_cells);
+	clks[IMX_CONN_ENET1_RGMII_CLK]	= imx_clk_scu("enet1_rgmii_clk", IMX_SC_R_ENET_1, IMX_SC_PM_CLK_MISC0, clk_cells);
+	clks[IMX_CONN_GPMI_BCH_IO_CLK]	= imx_clk_scu("gpmi_io_clk", IMX_SC_R_NAND, IMX_SC_PM_CLK_MST_BUS, clk_cells);
+	clks[IMX_CONN_GPMI_BCH_CLK]	= imx_clk_scu("gpmi_bch_clk", IMX_SC_R_NAND, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_CONN_USB2_ACLK]	= imx_clk_scu("usb3_aclk_div", IMX_SC_R_USB_2, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_CONN_USB2_BUS_CLK]	= imx_clk_scu("usb3_bus_div", IMX_SC_R_USB_2, IMX_SC_PM_CLK_MST_BUS, clk_cells);
+	clks[IMX_CONN_USB2_LPM_CLK]	= imx_clk_scu("usb3_lpm_div", IMX_SC_R_USB_2, IMX_SC_PM_CLK_MISC, clk_cells);
 
 	/* Display controller SS */
-	clks[IMX_DC0_DISP0_CLK]		= imx_clk_scu("dc0_disp0_clk", IMX_SC_R_DC_0, IMX_SC_PM_CLK_MISC0);
-	clks[IMX_DC0_DISP1_CLK]		= imx_clk_scu("dc0_disp1_clk", IMX_SC_R_DC_0, IMX_SC_PM_CLK_MISC1);
+	clks[IMX_DC0_DISP0_CLK]		= imx_clk_scu("dc0_disp0_clk", IMX_SC_R_DC_0, IMX_SC_PM_CLK_MISC0, clk_cells);
+	clks[IMX_DC0_DISP1_CLK]		= imx_clk_scu("dc0_disp1_clk", IMX_SC_R_DC_0, IMX_SC_PM_CLK_MISC1, clk_cells);
 
 	/* MIPI-LVDS SS */
-	clks[IMX_MIPI0_I2C0_CLK]	= imx_clk_scu("mipi0_i2c0_clk", IMX_SC_R_MIPI_0_I2C_0, IMX_SC_PM_CLK_MISC2);
-	clks[IMX_MIPI0_I2C1_CLK]	= imx_clk_scu("mipi0_i2c1_clk", IMX_SC_R_MIPI_0_I2C_1, IMX_SC_PM_CLK_MISC2);
+	clks[IMX_MIPI0_I2C0_CLK]	= imx_clk_scu("mipi0_i2c0_clk", IMX_SC_R_MIPI_0_I2C_0, IMX_SC_PM_CLK_MISC2, clk_cells);
+	clks[IMX_MIPI0_I2C1_CLK]	= imx_clk_scu("mipi0_i2c1_clk", IMX_SC_R_MIPI_0_I2C_1, IMX_SC_PM_CLK_MISC2, clk_cells);
 
 	/* MIPI CSI SS */
-	clks[IMX_CSI0_CORE_CLK]		= imx_clk_scu("mipi_csi0_core_clk", IMX_SC_R_CSI_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_CSI0_ESC_CLK]		= imx_clk_scu("mipi_csi0_esc_clk",  IMX_SC_R_CSI_0, IMX_SC_PM_CLK_MISC);
-	clks[IMX_CSI0_I2C0_CLK]		= imx_clk_scu("mipi_csi0_i2c0_clk", IMX_SC_R_CSI_0_I2C_0, IMX_SC_PM_CLK_PER);
-	clks[IMX_CSI0_PWM0_CLK]		= imx_clk_scu("mipi_csi0_pwm0_clk", IMX_SC_R_CSI_0_PWM_0, IMX_SC_PM_CLK_PER);
+	clks[IMX_CSI0_CORE_CLK]		= imx_clk_scu("mipi_csi0_core_clk", IMX_SC_R_CSI_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_CSI0_ESC_CLK]		= imx_clk_scu("mipi_csi0_esc_clk",  IMX_SC_R_CSI_0, IMX_SC_PM_CLK_MISC, clk_cells);
+	clks[IMX_CSI0_I2C0_CLK]		= imx_clk_scu("mipi_csi0_i2c0_clk", IMX_SC_R_CSI_0_I2C_0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_CSI0_PWM0_CLK]		= imx_clk_scu("mipi_csi0_pwm0_clk", IMX_SC_R_CSI_0_PWM_0, IMX_SC_PM_CLK_PER, clk_cells);
 
 	/* GPU SS */
-	clks[IMX_GPU0_CORE_CLK]		= imx_clk_scu("gpu_core0_clk",	 IMX_SC_R_GPU_0_PID0, IMX_SC_PM_CLK_PER);
-	clks[IMX_GPU0_SHADER_CLK]	= imx_clk_scu("gpu_shader0_clk", IMX_SC_R_GPU_0_PID0, IMX_SC_PM_CLK_MISC);
+	clks[IMX_GPU0_CORE_CLK]		= imx_clk_scu("gpu_core0_clk",	 IMX_SC_R_GPU_0_PID0, IMX_SC_PM_CLK_PER, clk_cells);
+	clks[IMX_GPU0_SHADER_CLK]	= imx_clk_scu("gpu_shader0_clk", IMX_SC_R_GPU_0_PID0, IMX_SC_PM_CLK_MISC, clk_cells);
 
 	for (i = 0; i < clk_data->num; i++) {
 		if (IS_ERR(clks[i]))
@@ -134,7 +138,12 @@  static int imx8qxp_clk_probe(struct platform_device *pdev)
 				i, PTR_ERR(clks[i]));
 	}
 
-	return of_clk_add_hw_provider(ccm_node, of_clk_hw_onecell_get, clk_data);
+	if (clk_cells == 2)
+		ret = of_clk_add_hw_provider(ccm_node, imx_scu_of_clk_src_get, imx_scu_clks);
+	else
+		ret = of_clk_add_hw_provider(ccm_node, of_clk_hw_onecell_get, clk_data);
+
+	return ret;
 }
 
 static const struct of_device_id imx8qxp_match[] = {
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))
+		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)) {
+			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);
+	if (ret)
+		pr_warn("%s: failed to attached the power domain %d\n",
+			name, ret);
+
+	platform_device_add(pdev);
+
+	/* For API backwards compatiblilty, simply return NULL for success */
+	return NULL;
+}
diff --git a/drivers/clk/imx/clk-scu.h b/drivers/clk/imx/clk-scu.h
index 2bcfaf06a458..a512f81ed801 100644
--- a/drivers/clk/imx/clk-scu.h
+++ b/drivers/clk/imx/clk-scu.h
@@ -8,22 +8,37 @@ 
 #define __IMX_CLK_SCU_H
 
 #include <linux/firmware/imx/sci.h>
+#include <linux/of.h>
 
-int imx_clk_scu_init(void);
+extern struct list_head imx_scu_clks[];
+
+int imx_clk_scu_init(struct device_node *np);
+struct clk_hw *imx_scu_of_clk_src_get(struct of_phandle_args *clkspec,
+				      void *data);
+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 clk_hw *__imx_clk_scu(const char *name, const char * const *parents,
 			     int num_parents, u32 rsrc_id, u8 clk_type);
 
 static inline struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id,
-					 u8 clk_type)
+					 u8 clk_type, u8 clk_cells)
 {
-	return __imx_clk_scu(name, NULL, 0, rsrc_id, clk_type);
+	if (clk_cells == 2)
+		return imx_clk_scu_alloc_dev(name, NULL, 0, rsrc_id, clk_type);
+	else
+		return __imx_clk_scu(name, NULL, 0, rsrc_id, clk_type);
 }
 
 static inline struct clk_hw *imx_clk_scu2(const char *name, const char * const *parents,
-					  int num_parents, u32 rsrc_id, u8 clk_type)
+					  int num_parents, u32 rsrc_id, u8 clk_type,
+					  u8 clk_cells)
 {
-	return __imx_clk_scu(name, parents, num_parents, rsrc_id, clk_type);
+	if (clk_cells == 2)
+		return imx_clk_scu_alloc_dev(name, parents, num_parents, rsrc_id, clk_type);
+	else
+		return __imx_clk_scu(name, parents, num_parents, rsrc_id, clk_type);
 }
 
 struct clk_hw *imx_clk_lpcg_scu(const char *name, const char *parent_name,