| Message ID | 20200910172826.3074357-9-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: mediatek: pm-domains: Add new driver for SCPSYS power domains controller | expand |
On Thu, 2020-09-10 at 19:28 +0200, Enric Balletbo i Serra wrote: > From: Matthias Brugger <mbrugger@suse.com> > > For the bus protection operations, some subsystem clocks need to be enabled > before releasing the protection. This patch identifies the subsystem clocks > by it's name. > > Suggested-by: Weiyi Lu <weiyi.lu@mediatek.com> > [Adapted the patch to the mtk-pm-domains driver] > Signed-off-by: Matthias Brugger <mbrugger@suse.com> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > > drivers/soc/mediatek/mtk-pm-domains.c | 82 +++++++++++++++++++++++---- > 1 file changed, 70 insertions(+), 12 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c > index 0802eccc3a0b..52a93a87e313 100644 > --- a/drivers/soc/mediatek/mtk-pm-domains.c > +++ b/drivers/soc/mediatek/mtk-pm-domains.c > @@ -3,6 +3,7 @@ > * Copyright (c) 2020 Collabora Ltd. > */ > #include <linux/clk.h> > +#include <linux/clk-provider.h> > #include <linux/init.h> > #include <linux/io.h> > #include <linux/iopoll.h> > @@ -81,6 +82,8 @@ struct scpsys_bus_prot_data { > bool bus_prot_reg_update; > }; > > +#define MAX_SUBSYS_CLKS 10 > + > /** > * struct scpsys_domain_data - scp domain data for power on/off flow > * @sta_mask: The mask for power on/off status bit. > @@ -107,6 +110,8 @@ struct scpsys_domain { > struct scpsys *scpsys; > int num_clks; > struct clk_bulk_data *clks; > + int num_subsys_clks; > + struct clk_bulk_data *subsys_clks; > struct regmap *infracfg; > struct regmap *smi; > }; > @@ -309,16 +314,22 @@ static int scpsys_power_on(struct generic_pm_domain *genpd) > val |= PWR_RST_B_BIT; > writel(val, ctl_addr); > > + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks); > + if (ret) > + goto err_pwr_ack; > + > ret = scpsys_sram_enable(pd, ctl_addr); > if (ret < 0) > - goto err_pwr_ack; > + goto err_sram; > > ret = scpsys_bus_protect_disable(pd); > if (ret < 0) > - goto err_pwr_ack; > + goto err_sram; > > return 0; > > +err_sram: > + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks); > err_pwr_ack: > clk_bulk_disable(pd->num_clks, pd->clks); > dev_err(scpsys->dev, "Failed to power on domain %s\n", genpd->name); > @@ -342,6 +353,8 @@ static int scpsys_power_off(struct generic_pm_domain *genpd) > if (ret < 0) > return ret; > > + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks); > + > /* subsys power off */ > val = readl(ctl_addr); > val |= PWR_ISO_BIT; > @@ -374,8 +387,11 @@ static int scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node > { > const struct scpsys_domain_data *domain_data; > struct scpsys_domain *pd; > - int i, ret; > + int i, ret, num_clks; > u32 id; > + int clk_ind = 0; > + struct property *prop; > + const char *clk_name; > > ret = of_property_read_u32(node, "reg", &id); > if (ret) { > @@ -410,28 +426,60 @@ static int scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node > if (IS_ERR(pd->smi)) > pd->smi = NULL; > > - pd->num_clks = of_clk_get_parent_count(node); > - if (pd->num_clks > 0) { > + num_clks = of_clk_get_parent_count(node); > + if (num_clks > 0) { > + /* Calculate number of subsys_clks */ > + of_property_for_each_string(node, "clock-names", prop, clk_name) { > + char *subsys; > + > + subsys = strchr(clk_name, '-'); > + if (subsys) > + pd->num_subsys_clks++; > + else > + pd->num_clks++; > + } > + In fact, I don't like the clock naming rules, as Matthias mentioned before. So in my work v17[1] I put subsystem clocks under each power domain sub-node without giving the clock name but put the basic clocks under the power controller node. [1] https://patchwork.kernel.org/patch/11703191/ > pd->clks = devm_kcalloc(scpsys->dev, pd->num_clks, sizeof(*pd->clks), GFP_KERNEL); > if (!pd->clks) > return -ENOMEM; > - } else { > - pd->num_clks = 0; > + > + pd->subsys_clks = devm_kcalloc(scpsys->dev, pd->num_subsys_clks, > + sizeof(*pd->subsys_clks), GFP_KERNEL); > + if (!pd->subsys_clks) > + return -ENOMEM; > } > > for (i = 0; i < pd->num_clks; i++) { > - pd->clks[i].clk = of_clk_get(node, i); > - if (IS_ERR(pd->clks[i].clk)) { > - ret = PTR_ERR(pd->clks[i].clk); > + struct clk *clk = of_clk_get(node, i); > + if (IS_ERR(clk)) { > + ret = PTR_ERR(clk); > dev_err(scpsys->dev, "%pOFn: failed to get clk at index %d: %d\n", node, i, > ret); > - return ret; > + goto err_put_clocks; > + } > + > + pd->clks[clk_ind++].clk = clk; > + } > + > + for (i = 0; i < pd->num_subsys_clks; i++) { > + struct clk *clk = of_clk_get(node, i + clk_ind); > + if (IS_ERR(clk)) { > + ret = PTR_ERR(clk); > + dev_err(scpsys->dev, "%pOFn: failed to get clk at index %d: %d\n", node, > + i + clk_ind, ret); > + goto err_put_subsys_clocks; > } > + > + pd->subsys_clks[i].clk = clk; > } > > + ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks); > + if (ret) > + goto err_put_subsys_clocks; > + > ret = clk_bulk_prepare(pd->num_clks, pd->clks); > if (ret) > - goto err_put_clocks; > + goto err_unprepare_subsys_clocks; > > /* > * Initially turn on all domains to make the domains usable > @@ -456,6 +504,12 @@ static int scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node > > err_unprepare_clocks: > clk_bulk_unprepare(pd->num_clks, pd->clks); > +err_unprepare_subsys_clocks: > + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks); > +err_put_subsys_clocks: > + clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks); > + devm_kfree(scpsys->dev, pd->subsys_clks); > + pd->num_subsys_clks = 0; > err_put_clocks: > clk_bulk_put(pd->num_clks, pd->clks); > devm_kfree(scpsys->dev, pd->clks); > @@ -537,6 +591,10 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd) > clk_bulk_unprepare(pd->num_clks, pd->clks); > clk_bulk_put(pd->num_clks, pd->clks); > pd->num_clks = 0; > + > + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks); > + clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks); > + pd->num_subsys_clks = 0; > } > > static void scpsys_domain_cleanup(struct scpsys *scpsys)
On 25/09/2020 12:55, Weiyi Lu wrote: > On Thu, 2020-09-10 at 19:28 +0200, Enric Balletbo i Serra wrote: >> From: Matthias Brugger <mbrugger@suse.com> >> >> For the bus protection operations, some subsystem clocks need to be enabled >> before releasing the protection. This patch identifies the subsystem clocks >> by it's name. >> >> Suggested-by: Weiyi Lu <weiyi.lu@mediatek.com> >> [Adapted the patch to the mtk-pm-domains driver] >> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >> --- >> >> drivers/soc/mediatek/mtk-pm-domains.c | 82 +++++++++++++++++++++++---- >> 1 file changed, 70 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c >> index 0802eccc3a0b..52a93a87e313 100644 >> --- a/drivers/soc/mediatek/mtk-pm-domains.c >> +++ b/drivers/soc/mediatek/mtk-pm-domains.c [...] >> >> - pd->num_clks = of_clk_get_parent_count(node); >> - if (pd->num_clks > 0) { >> + num_clks = of_clk_get_parent_count(node); >> + if (num_clks > 0) { >> + /* Calculate number of subsys_clks */ >> + of_property_for_each_string(node, "clock-names", prop, clk_name) { >> + char *subsys; >> + >> + subsys = strchr(clk_name, '-'); >> + if (subsys) >> + pd->num_subsys_clks++; >> + else >> + pd->num_clks++; >> + } >> + > > In fact, I don't like the clock naming rules, as Matthias mentioned > before. So in my work v17[1] > I put subsystem clocks under each power domain sub-node without giving > the clock name but put the basic clocks under the power controller node. > > [1] https://patchwork.kernel.org/patch/11703191/ The quick answer, there is no perfect solution to our problem. If we put all basic clocks under the parent node, then we will have to have a list of basic clocks in the driver data for each SoC. Apart from that the DTS would not reflect the exact hardware, as the basic clocks needed for every power domain would be hidden in the driver data. Given this, I think the best way is to distinguish the clocks by it's name, as done by you in earlier series of the scpsys. It will give us a cleaner device tree and we won't need to add long clock lists in the driver data. If I remember correctly Rob acknowledged your binding change for that: https://patchwork.kernel.org/patch/11562521/ Regards, Matthias
diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c index 0802eccc3a0b..52a93a87e313 100644 --- a/drivers/soc/mediatek/mtk-pm-domains.c +++ b/drivers/soc/mediatek/mtk-pm-domains.c @@ -3,6 +3,7 @@ * Copyright (c) 2020 Collabora Ltd. */ #include <linux/clk.h> +#include <linux/clk-provider.h> #include <linux/init.h> #include <linux/io.h> #include <linux/iopoll.h> @@ -81,6 +82,8 @@ struct scpsys_bus_prot_data { bool bus_prot_reg_update; }; +#define MAX_SUBSYS_CLKS 10 + /** * struct scpsys_domain_data - scp domain data for power on/off flow * @sta_mask: The mask for power on/off status bit. @@ -107,6 +110,8 @@ struct scpsys_domain { struct scpsys *scpsys; int num_clks; struct clk_bulk_data *clks; + int num_subsys_clks; + struct clk_bulk_data *subsys_clks; struct regmap *infracfg; struct regmap *smi; }; @@ -309,16 +314,22 @@ static int scpsys_power_on(struct generic_pm_domain *genpd) val |= PWR_RST_B_BIT; writel(val, ctl_addr); + ret = clk_bulk_enable(pd->num_subsys_clks, pd->subsys_clks); + if (ret) + goto err_pwr_ack; + ret = scpsys_sram_enable(pd, ctl_addr); if (ret < 0) - goto err_pwr_ack; + goto err_sram; ret = scpsys_bus_protect_disable(pd); if (ret < 0) - goto err_pwr_ack; + goto err_sram; return 0; +err_sram: + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks); err_pwr_ack: clk_bulk_disable(pd->num_clks, pd->clks); dev_err(scpsys->dev, "Failed to power on domain %s\n", genpd->name); @@ -342,6 +353,8 @@ static int scpsys_power_off(struct generic_pm_domain *genpd) if (ret < 0) return ret; + clk_bulk_disable(pd->num_subsys_clks, pd->subsys_clks); + /* subsys power off */ val = readl(ctl_addr); val |= PWR_ISO_BIT; @@ -374,8 +387,11 @@ static int scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node { const struct scpsys_domain_data *domain_data; struct scpsys_domain *pd; - int i, ret; + int i, ret, num_clks; u32 id; + int clk_ind = 0; + struct property *prop; + const char *clk_name; ret = of_property_read_u32(node, "reg", &id); if (ret) { @@ -410,28 +426,60 @@ static int scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node if (IS_ERR(pd->smi)) pd->smi = NULL; - pd->num_clks = of_clk_get_parent_count(node); - if (pd->num_clks > 0) { + num_clks = of_clk_get_parent_count(node); + if (num_clks > 0) { + /* Calculate number of subsys_clks */ + of_property_for_each_string(node, "clock-names", prop, clk_name) { + char *subsys; + + subsys = strchr(clk_name, '-'); + if (subsys) + pd->num_subsys_clks++; + else + pd->num_clks++; + } + pd->clks = devm_kcalloc(scpsys->dev, pd->num_clks, sizeof(*pd->clks), GFP_KERNEL); if (!pd->clks) return -ENOMEM; - } else { - pd->num_clks = 0; + + pd->subsys_clks = devm_kcalloc(scpsys->dev, pd->num_subsys_clks, + sizeof(*pd->subsys_clks), GFP_KERNEL); + if (!pd->subsys_clks) + return -ENOMEM; } for (i = 0; i < pd->num_clks; i++) { - pd->clks[i].clk = of_clk_get(node, i); - if (IS_ERR(pd->clks[i].clk)) { - ret = PTR_ERR(pd->clks[i].clk); + struct clk *clk = of_clk_get(node, i); + if (IS_ERR(clk)) { + ret = PTR_ERR(clk); dev_err(scpsys->dev, "%pOFn: failed to get clk at index %d: %d\n", node, i, ret); - return ret; + goto err_put_clocks; + } + + pd->clks[clk_ind++].clk = clk; + } + + for (i = 0; i < pd->num_subsys_clks; i++) { + struct clk *clk = of_clk_get(node, i + clk_ind); + if (IS_ERR(clk)) { + ret = PTR_ERR(clk); + dev_err(scpsys->dev, "%pOFn: failed to get clk at index %d: %d\n", node, + i + clk_ind, ret); + goto err_put_subsys_clocks; } + + pd->subsys_clks[i].clk = clk; } + ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks); + if (ret) + goto err_put_subsys_clocks; + ret = clk_bulk_prepare(pd->num_clks, pd->clks); if (ret) - goto err_put_clocks; + goto err_unprepare_subsys_clocks; /* * Initially turn on all domains to make the domains usable @@ -456,6 +504,12 @@ static int scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node err_unprepare_clocks: clk_bulk_unprepare(pd->num_clks, pd->clks); +err_unprepare_subsys_clocks: + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks); +err_put_subsys_clocks: + clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks); + devm_kfree(scpsys->dev, pd->subsys_clks); + pd->num_subsys_clks = 0; err_put_clocks: clk_bulk_put(pd->num_clks, pd->clks); devm_kfree(scpsys->dev, pd->clks); @@ -537,6 +591,10 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd) clk_bulk_unprepare(pd->num_clks, pd->clks); clk_bulk_put(pd->num_clks, pd->clks); pd->num_clks = 0; + + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks); + clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks); + pd->num_subsys_clks = 0; } static void scpsys_domain_cleanup(struct scpsys *scpsys)