On Fri, Oct 2, 2020 at 12:02 AM Enric Balletbo i Serra
[off-list ref] wrote:

Why do you care? It seems like all you ever do with this is check if
the domain is powered on, so it seems like you could just
return (status && status2); ?

+}
+
+static int scpsys_sram_enable(struct scpsys_domain *pd)
+{
+       u32 pdn_ack = pd->data->sram_pdn_ack_bits;
+       struct scpsys *scpsys = pd->scpsys;
+       u32 val;
+       int tmp;

regmap_read_poll_timeout => regmap_read technically takes a `unsigned
int *` parameter, so this should probably be unsigned too (or... just
reuse val?)

+
+       regmap_read(scpsys->base, pd->data->ctl_offs, &val);
+       val &= ~pd->data->sram_pdn_bits;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);

Replace with regmap_update_bits?

+
+       /* Either wait until SRAM_PDN_ACK all 1 or 0 */
+       return regmap_read_poll_timeout(scpsys->base, pd->data->ctl_offs, tmp,
+                                       (tmp & pdn_ack) == 0, MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
+}
+
+static int scpsys_sram_disable(struct scpsys_domain *pd)
+{
+       u32 pdn_ack = pd->data->sram_pdn_ack_bits;
+       struct scpsys *scpsys = pd->scpsys;
+       u32 val;
+       int tmp;
+
+       regmap_read(scpsys->base, pd->data->ctl_offs, &val);
+       val |= pd->data->sram_pdn_bits;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+
+       /* Either wait until SRAM_PDN_ACK all 1 or 0 */
+       return regmap_read_poll_timeout(scpsys->base, pd->data->ctl_offs, tmp,
+                                       (tmp & pdn_ack) == pdn_ack, MTK_POLL_DELAY_US,
+                                       MTK_POLL_TIMEOUT);
+}
+
+static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
+{
+       const struct scpsys_bus_prot_data *bp_data = &pd->data->bp_infracfg;
+
+       if (!bp_data->bus_prot_mask)
+               return 0;
+
+       return mtk_infracfg_set_bus_protection(pd->infracfg, bp_data->bus_prot_mask,
+                                              bp_data->bus_prot_reg_update);
+}
+
+static int scpsys_bus_protect_disable(struct scpsys_domain *pd)
+{
+       const struct scpsys_bus_prot_data *bp_data = &pd->data->bp_infracfg;
+
+       if (!bp_data->bus_prot_mask)
+               return 0;
+
+       return mtk_infracfg_clear_bus_protection(pd->infracfg, bp_data->bus_prot_mask,
+                                                bp_data->bus_prot_reg_update);
+}
+
+static int scpsys_power_on(struct generic_pm_domain *genpd)
+{
+       struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
+       struct scpsys *scpsys = pd->scpsys;
+       int ret, tmp;
+       u32 val;
+
+       ret = clk_bulk_enable(pd->num_clks, pd->clks);
+       if (ret)
+               return ret;
+
+       /* subsys power on */
+       regmap_read(scpsys->base, pd->data->ctl_offs, &val);
+       val |= PWR_ON_BIT;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+       val |= PWR_ON_2ND_BIT;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+
+       /* wait until PWR_ACK = 1 */
+       ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, tmp > 0, MTK_POLL_DELAY_US,
+                                MTK_POLL_TIMEOUT);
+       if (ret < 0)
+               goto err_pwr_ack;
+
+       val &= ~PWR_CLK_DIS_BIT;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+
+       val &= ~PWR_ISO_BIT;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+
+       val |= PWR_RST_B_BIT;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+
+       ret = scpsys_sram_enable(pd);
+       if (ret < 0)
+               goto err_pwr_ack;
+
+       ret = scpsys_bus_protect_disable(pd);
+       if (ret < 0)
+               goto err_pwr_ack;
+
+       return 0;
+
+err_pwr_ack:
+       clk_bulk_disable(pd->num_clks, pd->clks);
+       return ret;
+}
+
+static int scpsys_power_off(struct generic_pm_domain *genpd)
+{
+       struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
+       struct scpsys *scpsys = pd->scpsys;
+       int ret, tmp;
+       u32 val;
+
+       ret = scpsys_bus_protect_enable(pd);
+       if (ret < 0)
+               return ret;
+
+       ret = scpsys_sram_disable(pd);
+       if (ret < 0)
+               return ret;
+
+       /* subsys power off */
+       regmap_read(scpsys->base, pd->data->ctl_offs, &val);
+       val |= PWR_ISO_BIT;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+
+       val &= ~PWR_RST_B_BIT;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+
+       val |= PWR_CLK_DIS_BIT;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+
+       val &= ~PWR_ON_BIT;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+
+       val &= ~PWR_ON_2ND_BIT;
+       regmap_write(scpsys->base, pd->data->ctl_offs, val);
+
+       /* wait until PWR_ACK = 0 */
+       ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, tmp == 0, MTK_POLL_DELAY_US,
+                                MTK_POLL_TIMEOUT);
+       if (ret < 0)
+               return ret;
+
+       clk_bulk_disable(pd->num_clks, pd->clks);
+
+       return 0;
+}
+
+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;
+       u32 id;
+
+       ret = of_property_read_u32(node, "reg", &id);
+       if (ret) {
+               dev_err_probe(scpsys->dev, ret,
+                             "%pOFn: failed to retrieve domain id from reg\n", node);
+               return -EINVAL;
+       }
+
+       if (id >= scpsys->soc_data->num_domains) {
+               dev_err_probe(scpsys->dev, -EINVAL, "%pOFn: invalid domain id %d\n", node, id);
+               return -EINVAL;
+       }
+
+       domain_data = &scpsys->soc_data->domains[id];
+       if (!domain_data) {

Is that even possible at all? I mean, even if
scpsys->soc_data->domains is NULL, as long as id != 0, this will no
happen.

+               dev_err_probe(scpsys->dev, -EINVAL, "%pOFn: undefined domain id %d\n", node, id);
+               return -EINVAL;
+       }
+
+       pd = devm_kzalloc(scpsys->dev, sizeof(*pd), GFP_KERNEL);
+       if (!pd)
+               return -ENOMEM;
+
+       pd->data = domain_data;
+       pd->scpsys = scpsys;
+
+       pd->infracfg = syscon_regmap_lookup_by_phandle(node, "mediatek,infracfg");
+       if (IS_ERR(pd->infracfg))
+               pd->infracfg = NULL;
+
+       pd->num_clks = of_clk_get_parent_count(node);
+       if (pd->num_clks > 0) {
+               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->num_clks < 0 can't happen so you can drop this.

+
+       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);
+                       dev_err_probe(scpsys->dev, ret, "%pOFn: failed to get clk at index %d\n",
+                                     node, i);
+                       return ret;
+               }
+       }
+
+       ret = clk_bulk_prepare(pd->num_clks, pd->clks);
+       if (ret)
+               goto err_put_clocks;
+
+       /*
+        * Initially turn on all domains to make the domains usable
+        * with !CONFIG_PM and to get the hardware in sync with the
+        * software.  The unused domains will be switched off during
+        * late_init time.
+        */
+       ret = scpsys_power_on(&pd->genpd);
+       if (ret < 0) {
+               dev_err_probe(scpsys->dev, ret, "failed to power on domain %pOFN\n", node);
+               goto err_unprepare_clocks;
+       }
+
+       pd->genpd.name = node->name;
+       pd->genpd.power_off = scpsys_power_off;
+       pd->genpd.power_on = scpsys_power_on;
+
+       pm_genpd_init(&pd->genpd, NULL, false);
+
+       scpsys->domains[id] = &pd->genpd;

Do we want to add some logic to make sure scpsys->domains[id] == NULL
before this? (to make sure we don't accidentally probe 2 domains with
the same id, which I guess could happen with some incorrect
devicetree)

+       return 0;
+
+err_unprepare_clocks:
+       clk_bulk_unprepare(pd->num_clks, pd->clks);
+err_put_clocks:
+       clk_bulk_put(pd->num_clks, pd->clks);

+       devm_kfree(scpsys->dev, pd->clks);
+       pd->num_clks = 0;

Why do you need these 2 lines? Can't you just let the core free the
devm_ allocated data?

+       return ret;
+}
+
+static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *parent)
+{
+       struct device_node *child;
+       struct generic_pm_domain *child_pd, *parent_pd;
+       int ret;
+
+       for_each_child_of_node(parent, child) {
+               u32 id;
+
+               ret = of_property_read_u32(parent, "reg", &id);
+               if (ret) {
+                       dev_err_probe(scpsys->dev, ret,
+                                     "%pOFn: failed to get parent domain id\n", child);
+                       goto err_put_node;
+               }
+               parent_pd = scpsys->pd_data.domains[id];

No bounds check on id here? Also, wondering if you want to move the
logic, including bounds check, to a small function (since it's
repeated 3 times now, but I think that can be reduced to 2).

+
+               ret = scpsys_add_one_domain(scpsys, child);
+               if (ret)
+                       goto err_put_node;
+
+               ret = of_property_read_u32(child, "reg", &id);
+               if (ret) {
+                       dev_err_probe(scpsys->dev, ret,
+                                     "%pOFn: failed to get child domain id\n", child);
+                       goto err_put_node;
+               }
+               child_pd = scpsys->pd_data.domains[id];

You already obtained the child_pd in scpsys_add_one_domain... Can you
have scpsys_add_one_domain return `child_pd` as a pointer (or a
PTR_ERR)?

+
+               ret = pm_genpd_add_subdomain(parent_pd, child_pd);
+               if (ret) {
+                       dev_err_probe(scpsys->dev, ret,
+                                     "failed to add %s subdomain to parent %s\n", child_pd->name,
+                                     parent_pd->name);
+                       goto err_put_node;
+               } else {
+                       dev_dbg(scpsys->dev, "%s add subdomain: %s\n", parent_pd->name,
+                               child_pd->name);
+               }
+
+               /* recursive call to add all subdomains */
+               ret = scpsys_add_subdomain(scpsys, child);
+               if (ret)
+                       goto err_put_node;
+       }
+
+       return 0;
+
+err_put_node:
+       of_node_put(child);
+       return ret;
+}
+
+static void scpsys_remove_one_domain(struct scpsys_domain *pd)
+{
+       int ret;
+
+       /*
+        * We're in the error cleanup already, so we only complain,
+        * but won't emit another error on top of the original one.
+        */
+       ret = pm_genpd_remove(&pd->genpd);
+       if (ret < 0)
+               dev_err(pd->scpsys->dev,
+                       "failed to remove domain '%s' : %d - state may be inconsistent\n",
+                       pd->genpd.name, ret);
+
+       scpsys_power_off(&pd->genpd);
+
+       clk_bulk_unprepare(pd->num_clks, pd->clks);
+       clk_bulk_put(pd->num_clks, pd->clks);

+       pd->num_clks = 0;

What's the point of setting num_clks to 0?

+}
+
+static void scpsys_domain_cleanup(struct scpsys *scpsys)
+{
+       struct generic_pm_domain *genpd;
+       struct scpsys_domain *pd;
+       int i;
+
+       for (i = scpsys->pd_data.num_domains - 1; i >= 0; i--) {
+               genpd = scpsys->pd_data.domains[i];
+               if (genpd) {
+                       pd = to_scpsys_domain(genpd);
+                       scpsys_remove_one_domain(pd);
+               }
+       }
+}
[snip]

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