| Message ID | 1566299605-15641-4-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: imx8: add new clock binding for better pm support | expand |
Quoting Dong Aisheng (2019-08-20 04:13:17) > diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c > index 5e2903e..1ad3f2a 100644 > --- a/drivers/clk/imx/clk-imx8qxp.c > +++ b/drivers/clk/imx/clk-imx8qxp.c > @@ -134,7 +134,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 (clock_cells == 2) Can you just read this from the DT node again instead of having a global variable called "clock_cells" for this? > + 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 fbef740..48bfb08 100644 > --- a/drivers/clk/imx/clk-scu.c > +++ b/drivers/clk/imx/clk-scu.c > @@ -16,6 +19,21 @@ > #define IMX_SIP_SET_CPUFREQ 0x00 > > static struct imx_sc_ipc *ccm_ipc_handle; > +struct device_node *pd_np; > +u32 clock_cells; > + > +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 +146,29 @@ 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; > + int ret, i; > + > + ret = imx_scu_get_handle(&ccm_ipc_handle); > + if (ret) > + return ret; > + > + if (of_property_read_u32(np, "#clock-cells", &clock_cells)) > + return -EINVAL; > + > + if (clock_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)) > + return -EPROBE_DEFER; Do you need to put some nodes here with of_node_put() one failure or when they're done being used? > + } > + > + return 0; > } > > /* > @@ -387,3 +425,99 @@ struct clk_hw *__imx_clk_scu(const char *name, const char * const *parents, [...] > + > +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(-ENOMEM); Why not 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; > +}
Hi Stephen, Thanks for the review. On Sat, Sep 7, 2019 at 5:29 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Dong Aisheng (2019-08-20 04:13:17) > > diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c > > index 5e2903e..1ad3f2a 100644 > > --- a/drivers/clk/imx/clk-imx8qxp.c > > +++ b/drivers/clk/imx/clk-imx8qxp.c > > @@ -134,7 +134,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 (clock_cells == 2) > > Can you just read this from the DT node again instead of having a global > variable called "clock_cells" for this? > I tried thinking about it. One problem is that we also need this information in the exist clk registration API to keep the backwards compatibility: e.g. static inline struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id, u8 clk_type) { - return __imx_clk_scu(name, NULL, 0, rsrc_id, clk_type); + if (clock_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); } Parsing it for all clocks seems not good. In the future, i planned to totally remove the legacy binding support which is a premature one and missing continued support. Then we will also remove this unneeded clock_cells. > > + 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 fbef740..48bfb08 100644 > > --- a/drivers/clk/imx/clk-scu.c > > +++ b/drivers/clk/imx/clk-scu.c > > @@ -16,6 +19,21 @@ > > #define IMX_SIP_SET_CPUFREQ 0x00 > > > > static struct imx_sc_ipc *ccm_ipc_handle; > > +struct device_node *pd_np; > > +u32 clock_cells; > > + > > +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 +146,29 @@ 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; > > + int ret, i; > > + > > + ret = imx_scu_get_handle(&ccm_ipc_handle); > > + if (ret) > > + return ret; > > + > > + if (of_property_read_u32(np, "#clock-cells", &clock_cells)) > > + return -EINVAL; > > + > > + if (clock_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)) > > + return -EPROBE_DEFER; > > Do you need to put some nodes here with of_node_put() one failure or > when they're done being used? > Good catch. We should put the node for of_find_compatible_node(). > > + } > > + > > + return 0; > > } > > > > /* > > @@ -387,3 +425,99 @@ struct clk_hw *__imx_clk_scu(const char *name, const char * const *parents, > [...] > > + > > +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(-ENOMEM); > > Why not ERR_PTR(ret)? > Good catch. Will fix. Regards Aisheng > > + } > > + > > + 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; > > +}
Quoting Dong Aisheng (2019-09-09 03:23:25) > Hi Stephen, > > Thanks for the review. > > On Sat, Sep 7, 2019 at 5:29 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Dong Aisheng (2019-08-20 04:13:17) > > > diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c > > > index 5e2903e..1ad3f2a 100644 > > > --- a/drivers/clk/imx/clk-imx8qxp.c > > > +++ b/drivers/clk/imx/clk-imx8qxp.c > > > @@ -134,7 +134,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 (clock_cells == 2) > > > > Can you just read this from the DT node again instead of having a global > > variable called "clock_cells" for this? > > > > I tried thinking about it. > One problem is that we also need this information in the exist clk > registration API to > keep the backwards compatibility: > e.g. > static inline struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id, > u8 clk_type) > { > - return __imx_clk_scu(name, NULL, 0, rsrc_id, clk_type); > + if (clock_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); > } > > Parsing it for all clocks seems not good. Can you parse it once for the clock controller and then pass it to the registration function as the number of cells? I dislike the global and the name of the global. > > In the future, i planned to totally remove the legacy binding support which > is a premature one and missing continued support. > Then we will also remove this unneeded clock_cells. Ok sure.
Hi Stephen, Sorry for the delay due to a horrible busy months. Just a bit relax now. On Tue, Sep 17, 2019 at 2:44 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Dong Aisheng (2019-09-09 03:23:25) > > Hi Stephen, > > > > Thanks for the review. > > > > On Sat, Sep 7, 2019 at 5:29 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > Quoting Dong Aisheng (2019-08-20 04:13:17) > > > > diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c > > > > index 5e2903e..1ad3f2a 100644 > > > > --- a/drivers/clk/imx/clk-imx8qxp.c > > > > +++ b/drivers/clk/imx/clk-imx8qxp.c > > > > @@ -134,7 +134,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 (clock_cells == 2) > > > > > > Can you just read this from the DT node again instead of having a global > > > variable called "clock_cells" for this? > > > > > > > I tried thinking about it. > > One problem is that we also need this information in the exist clk > > registration API to > > keep the backwards compatibility: > > e.g. > > static inline struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id, > > u8 clk_type) > > { > > - return __imx_clk_scu(name, NULL, 0, rsrc_id, clk_type); > > + if (clock_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); > > } > > > > Parsing it for all clocks seems not good. > > Can you parse it once for the clock controller and then pass it to the > registration function as the number of cells? I dislike the global and > the name of the global. > Yes, i can do it. Why i didn't do it before is because there're tens of APIs callers already and finally we will back to the original API again after removing the legacy users. So i used a global variable as a temporarily workaround during transition phase. But i do agree that make the code look ugly. Regards Aisheng > > > > In the future, i planned to totally remove the legacy binding support which > > is a premature one and missing continued support. > > Then we will also remove this unneeded clock_cells. > > Ok sure. >
diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c index 5e2903e..1ad3f2a 100644 --- a/drivers/clk/imx/clk-imx8qxp.c +++ b/drivers/clk/imx/clk-imx8qxp.c @@ -24,7 +24,7 @@ static int imx8qxp_clk_probe(struct platform_device *pdev) struct clk_hw **clks; int ret, i; - ret = imx_clk_scu_init(); + ret = imx_clk_scu_init(ccm_node); if (ret) return ret; @@ -134,7 +134,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 (clock_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 fbef740..48bfb08 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,21 @@ #define IMX_SIP_SET_CPUFREQ 0x00 static struct imx_sc_ipc *ccm_ipc_handle; +struct device_node *pd_np; +u32 clock_cells; + +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 +146,29 @@ 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; + int ret, i; + + ret = imx_scu_get_handle(&ccm_ipc_handle); + if (ret) + return ret; + + if (of_property_read_u32(np, "#clock-cells", &clock_cells)) + return -EINVAL; + + if (clock_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)) + return -EPROBE_DEFER; + } + + return 0; } /* @@ -387,3 +425,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(-ENOMEM); + } + + 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 2bcfaf0..819dc32 100644 --- a/drivers/clk/imx/clk-scu.h +++ b/drivers/clk/imx/clk-scu.h @@ -8,8 +8,17 @@ #define __IMX_CLK_SCU_H #include <linux/firmware/imx/sci.h> +#include <linux/of.h> -int imx_clk_scu_init(void); +extern u32 clock_cells; +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); @@ -17,13 +26,19 @@ struct clk_hw *__imx_clk_scu(const char *name, const char * const *parents, static inline struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id, u8 clk_type) { - return __imx_clk_scu(name, NULL, 0, rsrc_id, clk_type); + if (clock_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) { - return __imx_clk_scu(name, parents, num_parents, rsrc_id, clk_type); + if (clock_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,
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: no changes v3: new patch --- drivers/clk/imx/clk-imx8qxp.c | 9 ++- drivers/clk/imx/clk-scu.c | 138 +++++++++++++++++++++++++++++++++++++++++- drivers/clk/imx/clk-scu.h | 21 ++++++- 3 files changed, 161 insertions(+), 7 deletions(-)