Re: [PATCH 08/12] soc: mediatek: pm-domains: Add subsystem clocks
From: Weiyi Lu
Date: Fri Sep 25 2020 - 06:56:04 EST
On Thu, 2020-09-10 at 19:28 +0200, Enric Balletbo i Serra wrote:
> From: Matthias Brugger <mbrugger@xxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> [Adapted the patch to the mtk-pm-domains driver]
> Signed-off-by: Matthias Brugger <mbrugger@xxxxxxxx>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> ---
>
> 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)