Re: [PATCH v5 06/11] soc: mediatek: add new flow for mtcmos power.
From: Matthias Brugger
Date: Wed Jul 18 2018 - 10:50:58 EST
On 17/07/18 10:52, Mars Cheng wrote:
> From: Owen Chen <owen.chen@xxxxxxxxxxxx>
>
> MT6765 need multiple register and actions to setup bus
> protect.
> 1. turn on subsys CG before release bus protect to receive
> ack.
> 2. turn off subsys CG after set bus protect and receive
> ack.
> 3. bus protect need not only infracfg but other domain
> register to setup. Therefore we add a set/clr APIs
> with more customize arguments.
>
> Signed-off-by: Owen Chen <owen.chen@xxxxxxxxxxxx>
> Signed-off-by: Mars Cheng <mars.cheng@xxxxxxxxxxxx>
> ---
> drivers/soc/mediatek/Makefile | 2 +-
> drivers/soc/mediatek/mtk-infracfg.c | 178 +++++++++++---
> drivers/soc/mediatek/mtk-scpsys-ext.c | 405 +++++++++++++++++++++++++++++++
> drivers/soc/mediatek/mtk-scpsys.c | 147 +++++++++--
> include/linux/soc/mediatek/infracfg.h | 9 +-
> include/linux/soc/mediatek/scpsys-ext.h | 66 +++++
> 6 files changed, 745 insertions(+), 62 deletions(-)
> create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> create mode 100644 include/linux/soc/mediatek/scpsys-ext.h
>
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index 12998b0..9dc6670 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1,3 +1,3 @@
> -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c
> index 958861c..11eadf8 100644
> --- a/drivers/soc/mediatek/mtk-infracfg.c
> +++ b/drivers/soc/mediatek/mtk-infracfg.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
> /*
> * Copyright (c) 2015 Pengutronix, Sascha Hauer <kernel@xxxxxxxxxxxxxx>
> *
> @@ -15,6 +16,7 @@
> #include <linux/jiffies.h>
> #include <linux/regmap.h>
> #include <linux/soc/mediatek/infracfg.h>
> +#include <linux/soc/mediatek/scpsys-ext.h>
> #include <asm/processor.h>
>
> #define MTK_POLL_DELAY_US 10
> @@ -26,62 +28,176 @@
> #define INFRA_TOPAXI_PROTECTEN_CLR 0x0264
>
> /**
> - * mtk_infracfg_set_bus_protection - enable bus protection
> - * @regmap: The infracfg regmap
> - * @mask: The mask containing the protection bits to be enabled.
> - * @reg_update: The boolean flag determines to set the protection bits
> - * by regmap_update_bits with enable register(PROTECTEN) or
> - * by regmap_write with set register(PROTECTEN_SET).
> + * mtk_generic_set_cmd - enable bus protection with set register
> + * @regmap: The bus protect regmap
> + * @set_ofs: The set register offset to set corresponding bit to 1.
> + * @sta_ofs: The status register offset to show bus protect enable/disable.
> + * @mask: The mask containing the protection bits to be disabled.
> *
> * This function enables the bus protection bits for disabled power
> * domains so that the system does not hang when some unit accesses the
> * bus while in power down.
> */
> -int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask,
> - bool reg_update)
> +int mtk_generic_set_cmd(struct regmap *regmap, u32 set_ofs,
> + u32 sta_ofs, u32 mask)
> {
> u32 val;
> int ret;
>
> - if (reg_update)
> - regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask,
> - mask);
> - else
> - regmap_write(infracfg, INFRA_TOPAXI_PROTECTEN_SET, mask);
> + regmap_write(regmap, set_ofs, mask);
>
> - ret = regmap_read_poll_timeout(infracfg, INFRA_TOPAXI_PROTECTSTA1,
> - val, (val & mask) == mask,
> - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> + ret = regmap_read_poll_timeout(regmap, sta_ofs, val,
> + (val & mask) == mask,
> + MTK_POLL_DELAY_US,
> + MTK_POLL_TIMEOUT);
>
> return ret;
> }
>
> /**
> - * mtk_infracfg_clear_bus_protection - disable bus protection
> - * @regmap: The infracfg regmap
> + * mtk_generic_clr_cmd - disable bus protection with clr register
> + * @regmap: The bus protect regmap
> + * @clr_ofs: The clr register offset to clear corresponding bit to 0.
> + * @sta_ofs: The status register offset to show bus protect enable/disable.
> * @mask: The mask containing the protection bits to be disabled.
> - * @reg_update: The boolean flag determines to clear the protection bits
> - * by regmap_update_bits with enable register(PROTECTEN) or
> - * by regmap_write with clear register(PROTECTEN_CLR).
> *
> * This function disables the bus protection bits previously enabled with
> - * mtk_infracfg_set_bus_protection.
> + * mtk_set_bus_protection.
> */
>
> -int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
> - bool reg_update)
> +int mtk_generic_clr_cmd(struct regmap *regmap, u32 clr_ofs,
> + u32 sta_ofs, u32 mask)
> {
> int ret;
> u32 val;
>
> - if (reg_update)
> - regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, 0);
> - else
> - regmap_write(infracfg, INFRA_TOPAXI_PROTECTEN_CLR, mask);
> + regmap_write(regmap, clr_ofs, mask);
>
> - ret = regmap_read_poll_timeout(infracfg, INFRA_TOPAXI_PROTECTSTA1,
> - val, !(val & mask),
> - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> + ret = regmap_read_poll_timeout(regmap, sta_ofs, val,
> + !(val & mask),
> + MTK_POLL_DELAY_US,
> + MTK_POLL_TIMEOUT);
> + return ret;
> +}
> +
> +/**
> + * mtk_generic_enable_cmd - enable bus protection with upd register
> + * @regmap: The bus protect regmap
> + * @upd_ofs: The update register offset to directly rewrite value to
> + * corresponding bit.
> + * @sta_ofs: The status register offset to show bus protect enable/disable.
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function enables the bus protection bits for disabled power
> + * domains so that the system does not hang when some unit accesses the
> + * bus while in power down.
> + */
> +int mtk_generic_enable_cmd(struct regmap *regmap, u32 upd_ofs,
> + u32 sta_ofs, u32 mask)
> +{
> + u32 val;
> + int ret;
> +
> + regmap_update_bits(regmap, upd_ofs, mask, mask);
>
> + ret = regmap_read_poll_timeout(regmap, sta_ofs, val,
> + (val & mask) == mask,
> + MTK_POLL_DELAY_US,
> + MTK_POLL_TIMEOUT);
> return ret;
> }
> +
> +/**
> + * mtk_generic_disable_cmd - disable bus protection with updd register
> + * @regmap: The bus protect regmap
> + * @upd_ofs: The update register offset to directly rewrite value to
> + * corresponding bit.
> + * @sta_ofs: The status register offset to show bus protect enable/disable.
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function disables the bus protection bits previously enabled with
> + * mtk_set_bus_protection.
> + */
> +
> +int mtk_generic_disable_cmd(struct regmap *regmap, u32 upd_ofs,
> + u32 sta_ofs, u32 mask)
> +{
> + int ret;
> + u32 val;
> +
> + regmap_update_bits(regmap, upd_ofs, mask, 0);
> +
> + ret = regmap_read_poll_timeout(regmap, sta_ofs,
> + val, !(val & mask),
> + MTK_POLL_DELAY_US,
> + MTK_POLL_TIMEOUT);
> + return ret;
> +}
> +
> +/**
> + * mtk_set_bus_protection - enable bus protection
> + * @infracfg: The bus protect regmap, default use infracfg
> + * @mask: The mask containing the protection bits to be enabled.
> + *
> + * This function enables the bus protection bits for disabled power
> + * domains so that the system does not hang when some unit accesses the
> + * bus while in power down.
> + */
> +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> + return mtk_generic_set_cmd(infracfg,
> + INFRA_TOPAXI_PROTECTEN_SET,
> + INFRA_TOPAXI_PROTECTSTA1,
> + mask);
> +}
> +
> +/**
> + * mtk_clear_bus_protection - disable bus protection
> + * @infracfg: The bus protect regmap, default use infracfg
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function disables the bus protection bits previously enabled with
> + * mtk_set_bus_protection.
> + */
> +
> +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> + return mtk_generic_clr_cmd(infracfg,
> + INFRA_TOPAXI_PROTECTEN_CLR,
> + INFRA_TOPAXI_PROTECTSTA1,
> + mask);
> +}
> +
> +/**
> + * mtk_infracfg_enable_bus_protection - enable bus protection
> + * @infracfg: The bus protect regmap, default use infracfg
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function enables the bus protection bits for disabled power
> + * domains so that the system does not hang when some unit accesses the
> + * bus while in power down.
> + */
> +int mtk_infracfg_enable_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> + return mtk_generic_enable_cmd(infracfg,
> + INFRA_TOPAXI_PROTECTEN,
> + INFRA_TOPAXI_PROTECTSTA1,
> + mask);
> +}
> +
> +/**
> + * mtk_infracfg_disable_bus_protection - disable bus protection
> + * @infracfg: The bus protect regmap, default use infracfg
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function disables the bus protection bits previously enabled with
> + * mtk_infracfg_set_bus_protection.
> + */
> +
> +int mtk_infracfg_disable_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> + return mtk_generic_disable_cmd(infracfg,
> + INFRA_TOPAXI_PROTECTEN,
> + INFRA_TOPAXI_PROTECTSTA1,
> + mask);
> +}
> diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> new file mode 100644
> index 0000000..965e64d
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
> @@ -0,0 +1,405 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Owen Chen <owen.chen@xxxxxxxxxxxx>
> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/soc/mediatek/infracfg.h>
> +#include <linux/soc/mediatek/scpsys-ext.h>
> +
> +
> +#define MAX_CLKS 10
> +#define INFRA "infracfg"
> +#define SMIC "smi_comm"
Don't use defines for this. While at it I suppose it should be "smi_common"
> +
> +static LIST_HEAD(ext_clk_map_list);
> +static LIST_HEAD(ext_attr_map_list);
> +
> +static struct regmap *infracfg;
> +static struct regmap *smi_comm;
> +
> +enum regmap_type {
> + IFR_TYPE,
> + SMI_TYPE,
> + MAX_REGMAP_TYPE,
> +};
> +
> +/**
> + * struct ext_reg_ctrl - set multiple register for bus protect
> + * @regmap: The bus protect regmap, 1: infracfg, 2: other master regmap
> + * such as SMI.
> + * @set_ofs: The set register offset to set corresponding bit to 1.
> + * @clr_ofs: The clr register offset to clear corresponding bit to 0.
> + * @sta_ofs: The status register offset to show bus protect enable/disable.
> + */
> +struct ext_reg_ctrl {
> + enum regmap_type type;
> + u32 set_ofs;
> + u32 clr_ofs;
> + u32 sta_ofs;
> +};
> +
> +/**
> + * struct ext_clk_ctrl - enable multiple clks for bus protect
> + * @clk: The clk need to enable before pwr on/bus protect.
> + * @scpd_n: The name present the scpsys domain where the clks belongs to.
> + * @clk_list: The list node linked to ext_clk_map_list.
> + */
> +struct ext_clk_ctrl {
> + struct clk *clk;
> + const char *scpd_n;
> + struct list_head clk_list;
> +};
> +
> +struct bus_mask_ops {
> + int (*set)(struct regmap *regmap, u32 set_ofs,
> + u32 sta_ofs, u32 mask);
> + int (*release)(struct regmap *regmap, u32 clr_ofs,
> + u32 sta_ofs, u32 mask);
> +};
> +
> +static struct scpsys_ext_attr *__get_attr_parent(const char *parent_n)
> +{
> + struct scpsys_ext_attr *attr;
> +
> + if (!parent_n)
> + return ERR_PTR(-EINVAL);
> +
> + list_for_each_entry(attr, &ext_attr_map_list, attr_list) {
> + if (attr->scpd_n && !strcmp(parent_n, attr->scpd_n))
> + return attr;
> + }
> +
> + return ERR_PTR(-EINVAL);
> +}
> +
> +int bus_ctrl_set_release(struct scpsys_ext_attr *attr, bool set)
> +{
> + int i;
> + int ret = 0;
> +
> + for (i = 0; i < MAX_STEP_NUM && attr->mask[i].mask; i++) {
> + struct ext_reg_ctrl *rc = attr->mask[i].regs;
> + struct regmap *regmap;
> +
> + if (rc->type == IFR_TYPE)
> + regmap = infracfg;
> + else if (rc->type == SMI_TYPE)
> + regmap = smi_comm;
> + else
> + return -EINVAL;
> +
> + if (set)
> + ret = attr->mask[i].ops->set(regmap,
> + rc->set_ofs,
> + rc->sta_ofs,
> + attr->mask[i].mask);
> + else
> + ret = attr->mask[i].ops->release(regmap,
> + rc->clr_ofs,
> + rc->sta_ofs,
> + attr->mask[i].mask);
> + }
> +
> + return ret;
> +}
> +
> +int bus_ctrl_set(struct scpsys_ext_attr *attr)
> +{
> + return bus_ctrl_set_release(attr, CMD_ENABLE);
> +}
> +
> +int bus_ctrl_release(struct scpsys_ext_attr *attr)
> +{
> + return bus_ctrl_set_release(attr, CMD_DISABLE);
> +}
> +
> +int bus_clk_enable_disable(struct scpsys_ext_attr *attr, bool enable)
> +{
> + int i = 0;
> + int ret = 0;
> + struct ext_clk_ctrl *cc;
> + struct clk *clk[MAX_CLKS];
> +
> + list_for_each_entry(cc, &ext_clk_map_list, clk_list) {
Why can't we handle this in the same way as we do in mtk-scpsys.c ?
> + if (!strcmp(cc->scpd_n, attr->scpd_n)) {
> + if (enable)
> + ret = clk_prepare_enable(cc->clk);
> + else
> + clk_disable_unprepare(cc->clk);
> +
> + if (ret) {
> + pr_err("Failed to %s %s\n",
> + enable ? "enable" : "disable",
> + __clk_get_name(cc->clk));
> + goto err;
> + } else {
> + clk[i] = cc->clk;
> + i++;
> + }
> + }
> + }
> +
> + return ret;
> +
> +err:
> + for (--i; i >= 0; i--)
> + if (enable)
> + clk_disable_unprepare(clk[i]);
> + else
> + clk_prepare_enable(clk[i]);
> + return ret;
> +}
> +
> +int bus_clk_enable(struct scpsys_ext_attr *attr)
> +{
> + struct scpsys_ext_attr *attr_p;
> + int ret = 0;
> +
> + attr_p = __get_attr_parent(attr->parent_n);
Why can't we implement this using the pg_genpd_add_subdomain approach?
> + if (!IS_ERR(attr_p)) {
> + ret = bus_clk_enable_disable(attr_p, CMD_ENABLE);
> + if (ret)
> + return ret;
> + }
> +
> + return bus_clk_enable_disable(attr, CMD_ENABLE);
> +}
> +
> +int bus_clk_disable(struct scpsys_ext_attr *attr)
> +{
> + struct scpsys_ext_attr *attr_p;
> + int ret = 0;
> +
> + ret = bus_clk_enable_disable(attr, CMD_DISABLE);
> + if (ret)
> + return ret;
> +
> + attr_p = __get_attr_parent(attr->parent_n);
> + if (!IS_ERR(attr_p))
> + ret = bus_clk_enable_disable(attr_p, CMD_DISABLE);
Same here.
> +
> + return ret;
> +}
> +
> +const struct bus_mask_ops bus_mask_set_clr_ctrl = {
> + .set = &mtk_generic_set_cmd,
> + .release = &mtk_generic_clr_cmd,
> +};
> +
> +const struct bus_ext_ops ext_bus_ctrl = {
> + .enable = &bus_ctrl_set,
> + .disable = &bus_ctrl_release,
> +};
> +
> +const struct bus_ext_ops ext_cg_ctrl = {
> + .enable = &bus_clk_enable,
> + .disable = &bus_clk_disable,
> +};
> +
> +/*
> + * scpsys bus driver init
> + */
> +struct regmap *syscon_regmap_lookup_by_phandle_idx(struct device_node *np,
> + const char *property,
> + int index)
> +{
> + struct device_node *syscon_np;
> + struct regmap *regmap;
> +
> + if (property)
> + syscon_np = of_parse_phandle(np, property, index);
> + else
> + syscon_np = np;
> +
> + if (!syscon_np)
> + return ERR_PTR(-ENODEV);
> +
> + regmap = syscon_node_to_regmap(syscon_np);
> + of_node_put(syscon_np);
> +
> + return regmap;
> +}
Why do we need this? It is never called...
> +
> +int scpsys_ext_regmap_init(struct platform_device *pdev)
> +{
> + infracfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> + INFRA);
> + if (IS_ERR(infracfg)) {
> + dev_err(&pdev->dev,
> + "Cannot find bus infracfg controller: %ld\n",
> + PTR_ERR(infracfg));
> + return PTR_ERR(infracfg);
> + }
> +
> + smi_comm = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> + SMIC);
> + if (IS_ERR(smi_comm)) {
> + dev_err(&pdev->dev,
> + "Cannot find bus smi_comm controller: %ld\n",
> + PTR_ERR(smi_comm));
> + return PTR_ERR(smi_comm);
> + }
> +
> + return 0;
> +}
> +
> +static int add_clk_to_list(struct platform_device *pdev,
> + const char *name,
> + const char *scpd_n)
> +{
> + struct clk *clk;
> + struct ext_clk_ctrl *cc;
> +
> + clk = devm_clk_get(&pdev->dev, name);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "Failed add clk %ld\n", PTR_ERR(clk));
> + return PTR_ERR(clk);
> + }
> +
> + cc = kzalloc(sizeof(*cc), GFP_KERNEL);
> + cc->clk = clk;
> + cc->scpd_n = kstrdup(scpd_n, GFP_KERNEL);
> +
> + list_add(&cc->clk_list, &ext_clk_map_list);
> +
> + return 0;
> +}
> +
> +static int add_cg_to_list(struct platform_device *pdev)
> +{
> + int i = 0;
> +
> + struct device_node *node = pdev->dev.of_node;
> +
> + if (!node) {
> + dev_err(&pdev->dev, "Cannot find topcksys node: %ld\n",
Why topcksys? Shouldn't that be the node of scpsys?
> + PTR_ERR(node));
> + return PTR_ERR(node);
> + }
> +
> + do {
> + const char *ck_name;
> + char *temp_str;
> + char *tok[2] = {NULL};
> + int cg_idx = 0;
> + int idx = 0;
> + int ret = 0;
> +
> + ret = of_property_read_string_index(node, "clock-names", i,
> + &ck_name);
> + if (ret < 0)
> + break;
> +
> + temp_str = kmalloc_array(strlen(ck_name), sizeof(char),
> + GFP_KERNEL | __GFP_ZERO);
> + memcpy(temp_str, ck_name, strlen(ck_name));
> + temp_str[strlen(ck_name)] = '\0';
why don't you use kstrdup or similar?
> + do {
> + tok[idx] = strsep(&temp_str, "-");
> + idx++;
> + } while (temp_str);
You want to split the clock name like "mm-2" in
char **tok = {"mm", "2"};
correct? That can be done easier AFAIK:
tok[0] = strsep(&temp_str, "-");
tok[1] = &temp_str;
> +
> + if (idx == 2) {
You don't add clocks like "mfg" and "mm". Why?
> + if (kstrtouint(tok[1], 10, &cg_idx))
And then? You don't do anything with cg_idx...
> + return -EINVAL;
> +
> + if (add_clk_to_list(pdev, ck_name, tok[0]))
add_clk_to_list third parameter is the name of the scp domain, but you pass the
clock name here. I'm puzzled.
> + return -EINVAL;
> + }
> + kfree(temp_str);
> + i++;
> + } while (1);
> +
> + return 0;
> +}
> +
> +int scpsys_ext_clk_init(struct platform_device *pdev)
> +{
> + int ret = 0;
> +
> + ret = add_cg_to_list(pdev);
> + if (ret)
> + goto err;
> +
> +err:
> + return ret;
> +}
Why do we need add_cg_to_list, it can be implemented directly here. Why is here
a goto to a return statement that will be executed anyway? Please go through the
code and check that it is clean before submitting.
> +
> +int scpsys_ext_attr_init(const struct scpsys_ext_data *data)
> +{
> + int i, count = 0;
> +
> + for (i = 0; i < data->num_attr; i++) {
> + struct scpsys_ext_attr *node = data->attr + i;
> +
> + if (!node)
> + continue;
> +
> + list_add(&node->attr_list, &ext_attr_map_list);
> + count++;
> + }
> +
> + if (!count)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id of_scpsys_ext_match_tbl[] = {
> + {
> + /* sentinel */
> + }
> +};
> +
> +struct scpsys_ext_data *scpsys_ext_init(struct platform_device *pdev)
> +{
> + const struct of_device_id *match;
> + struct scpsys_ext_data *data;
> + int ret;
> +
> + match = of_match_device(of_scpsys_ext_match_tbl, &pdev->dev);
> +
> + if (!match) {
> + dev_err(&pdev->dev, "no match\n");
> + return ERR_CAST(match);
> + }
> +
> + data = (struct scpsys_ext_data *)match->data;
> + if (IS_ERR(data)) {
> + dev_err(&pdev->dev, "no match scpext data\n");
> + return ERR_CAST(data);
> + }
> +
> + ret = scpsys_ext_attr_init(data);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Failed to init bus attr: %d\n",
> + ret);
> + return ERR_PTR(ret);
> + }
> +
> + ret = scpsys_ext_regmap_init(pdev);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Failed to init bus register: %d\n",
> + ret);
> + return ERR_PTR(ret);
> + }
> +
> + ret = scpsys_ext_clk_init(pdev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to init bus clks: %d\n",
> + ret);
> + return ERR_PTR(ret);
> + }
> +
> + return data;
> +}
> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> index 4bb6c7a..03df2d6 100644
> --- a/drivers/soc/mediatek/mtk-scpsys.c
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
> /*
> * Copyright (c) 2015 Pengutronix, Sascha Hauer <kernel@xxxxxxxxxxxxxx>
> *
> @@ -20,6 +21,7 @@
> #include <linux/pm_domain.h>
> #include <linux/regulator/consumer.h>
> #include <linux/soc/mediatek/infracfg.h>
> +#include <linux/soc/mediatek/scpsys-ext.h>
>
> #include <dt-bindings/power/mt2701-power.h>
> #include <dt-bindings/power/mt2712-power.h>
> @@ -117,6 +119,15 @@ enum clk_id {
>
> #define MAX_CLKS 3
>
> +/**
> + * struct scp_domain_data - scp domain data for power on/off flow
> + * @name: The domain name.
> + * @sta_mask: The mask for power on/off status bit.
> + * @ctl_offs: The offset for main power control register.
> + * @sram_pdn_bits: The mask for sram power control bits.
> + * @sram_pdn_ack_bits The mask for sram power control acked bits.
> + * @caps: The flag for active wake-up action.
> + */
> struct scp_domain_data {
> const char *name;
> u32 sta_mask;
> @@ -150,7 +161,7 @@ struct scp {
> void __iomem *base;
> struct regmap *infracfg;
> struct scp_ctrl_reg ctrl_reg;
> - bool bus_prot_reg_update;
> + struct scpsys_ext_data *ext_data;
> };
>
> struct scp_subdomain {
> @@ -164,7 +175,6 @@ struct scp_soc_data {
> const struct scp_subdomain *subdomains;
> int num_subdomains;
> const struct scp_ctrl_reg regs;
> - bool bus_prot_reg_update;
> };
>
> static int scpsys_domain_is_on(struct scp_domain *scpd)
> @@ -236,6 +246,31 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> val |= PWR_RST_B_BIT;
> writel(val, ctl_addr);
>
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->cg_ops) {
> + ret = attr->cg_ops->enable(attr);
> + if (ret)
> + goto err_ext_clk;
> + }
> + }
> +
> + val &= ~scpd->data->sram_pdn_bits;
> + writel(val, ctl_addr);
> +
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->cg_ops) {
> + ret = attr->cg_ops->enable(attr);
> + if (ret)
> + goto err_ext_clk;
> + }
> + }
> +
> val &= ~scpd->data->sram_pdn_bits;
> writel(val, ctl_addr);
>
> @@ -247,25 +282,65 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> * applied here.
> */
> usleep_range(12000, 12100);
> -
> } else {
> ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
> MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> if (ret < 0)
> - goto err_pwr_ack;
> + goto err_sram;
> }
>
> if (scpd->data->bus_prot_mask) {
> ret = mtk_infracfg_clear_bus_protection(scp->infracfg,
> - scpd->data->bus_prot_mask,
> - scp->bus_prot_reg_update);
> + scpd->data->bus_prot_mask);
> if (ret)
> - goto err_pwr_ack;
> + goto err_sram;
> + }
> +
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->bus_ops) {
> + ret = attr->bus_ops->disable(attr);
> + if (ret)
> + goto err_sram;
> + }
> + }
> +
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->cg_ops) {
> + ret = attr->cg_ops->disable(attr);
> + if (ret)
> + goto err_sram;
> + }
> }
>
> return 0;
>
> +err_sram:
> + val = readl(ctl_addr);
> + val |= scpd->data->sram_pdn_bits;
> + writel(val, ctl_addr);
> +err_ext_clk:
> + val = readl(ctl_addr);
> + val |= PWR_ISO_BIT;
> + writel(val, ctl_addr);
> +
> + val &= ~PWR_RST_B_BIT;
> + writel(val, ctl_addr);
> +
> + val |= PWR_CLK_DIS_BIT;
> + writel(val, ctl_addr);
> err_pwr_ack:
> + val &= ~PWR_ON_BIT;
> + writel(val, ctl_addr);
> +
> + val &= ~PWR_ON_2ND_BIT;
> + writel(val, ctl_addr);
> +
> for (i = MAX_CLKS - 1; i >= 0; i--) {
> if (scpd->clk[i])
> clk_disable_unprepare(scpd->clk[i]);
> @@ -274,8 +349,6 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> if (scpd->supply)
> regulator_disable(scpd->supply);
>
> - dev_err(scp->dev, "Failed to power on domain %s\n", genpd->name);
> -
> return ret;
> }
>
> @@ -289,14 +362,35 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> int ret, tmp;
> int i;
>
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->cg_ops) {
> + ret = attr->cg_ops->enable(attr);
> + if (ret)
> + goto out;
> + }
> + }
> +
> if (scpd->data->bus_prot_mask) {
> ret = mtk_infracfg_set_bus_protection(scp->infracfg,
> - scpd->data->bus_prot_mask,
> - scp->bus_prot_reg_update);
> + scpd->data->bus_prot_mask);
> if (ret)
> goto out;
> }
>
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->bus_ops) {
> + ret = attr->bus_ops->enable(attr);
> + if (ret)
> + goto out;
> + }
> + }
> +
> val = readl(ctl_addr);
> val |= scpd->data->sram_pdn_bits;
> writel(val, ctl_addr);
> @@ -307,6 +401,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> if (ret < 0)
> goto out;
>
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->cg_ops) {
> + ret = attr->cg_ops->disable(attr);
> + if (ret)
> + goto out;
> + }
> + }
> +
> val |= PWR_ISO_BIT;
> writel(val, ctl_addr);
>
> @@ -337,8 +442,6 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> return 0;
>
> out:
> - dev_err(scp->dev, "Failed to power off domain %s\n", genpd->name);
> -
> return ret;
> }
>
> @@ -352,8 +455,7 @@ static void init_clks(struct platform_device *pdev, struct clk **clk)
>
> static struct scp *init_scp(struct platform_device *pdev,
> const struct scp_domain_data *scp_domain_data, int num,
> - const struct scp_ctrl_reg *scp_ctrl_reg,
> - bool bus_prot_reg_update)
> + const struct scp_ctrl_reg *scp_ctrl_reg)
> {
> struct genpd_onecell_data *pd_data;
> struct resource *res;
> @@ -367,11 +469,10 @@ static struct scp *init_scp(struct platform_device *pdev,
>
> scp->ctrl_reg.pwr_sta_offs = scp_ctrl_reg->pwr_sta_offs;
> scp->ctrl_reg.pwr_sta2nd_offs = scp_ctrl_reg->pwr_sta2nd_offs;
> -
> - scp->bus_prot_reg_update = bus_prot_reg_update;
> -
> scp->dev = &pdev->dev;
>
> + scp->ext_data = scpsys_ext_init(pdev);
> +
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> scp->base = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(scp->base))
> @@ -1021,7 +1122,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> },
> - .bus_prot_reg_update = true,
> };
>
> static const struct scp_soc_data mt2712_data = {
> @@ -1033,7 +1133,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> },
> - .bus_prot_reg_update = false,
> };
>
> static const struct scp_soc_data mt6765_data = {
> @@ -1056,7 +1155,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS_MT6797,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND_MT6797
> },
> - .bus_prot_reg_update = true,
I don't understand why you can delete this flag if you don't change anything
else in the data structure. For me this looks like you will break other chips.
Please explain.
I have the gut feeling that this can be implemented in the existing mtk-scpsys
driver. Can you please explain what are the points that this is not possible.
I want to understand the design decisions you made here, because they seem
really odd to me.
Best regards,
Matthias
> };
>
> static const struct scp_soc_data mt7622_data = {
> @@ -1066,7 +1164,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> },
> - .bus_prot_reg_update = true,
> };
>
> static const struct scp_soc_data mt7623a_data = {
> @@ -1076,7 +1173,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> },
> - .bus_prot_reg_update = true,
> };
>
> static const struct scp_soc_data mt8173_data = {
> @@ -1088,7 +1184,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> },
> - .bus_prot_reg_update = true,
> };
>
> /*
> @@ -1132,8 +1227,8 @@ static int scpsys_probe(struct platform_device *pdev)
>
> soc = of_device_get_match_data(&pdev->dev);
>
> - scp = init_scp(pdev, soc->domains, soc->num_domains, &soc->regs,
> - soc->bus_prot_reg_update);
> + scp = init_scp(pdev, soc->domains, soc->num_domains,
> + &soc->regs);
> if (IS_ERR(scp))
> return PTR_ERR(scp);
>
> diff --git a/include/linux/soc/mediatek/infracfg.h b/include/linux/soc/mediatek/infracfg.h
> index fd25f01..bfad082 100644
> --- a/include/linux/soc/mediatek/infracfg.h
> +++ b/include/linux/soc/mediatek/infracfg.h
> @@ -32,8 +32,9 @@
> #define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
> BIT(7) | BIT(8))
>
> -int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask,
> - bool reg_update);
> -int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
> - bool reg_update);
> +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask);
> +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask);
> +int mtk_infracfg_enable_bus_protection(struct regmap *infracfg, u32 mask);
> +int mtk_infracfg_disable_bus_protection(struct regmap *infracfg, u32 mask);
> +
> #endif /* __SOC_MEDIATEK_INFRACFG_H */
> diff --git a/include/linux/soc/mediatek/scpsys-ext.h b/include/linux/soc/mediatek/scpsys-ext.h
> new file mode 100644
> index 0000000..99b5ff1
> --- /dev/null
> +++ b/include/linux/soc/mediatek/scpsys-ext.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SOC_MEDIATEK_SCPSYS_EXT_H
> +#define __SOC_MEDIATEK_SCPSYS_EXT_H
> +
> +#include <linux/platform_device.h>
> +
> +#define CMD_ENABLE 1
> +#define CMD_DISABLE 0
> +
> +#define MAX_STEP_NUM 4
> +
> +/**
> + * struct bus_mask - set mask and corresponding operation for bus protect
> + * @regs: The register set of bus register control, including set/clr/sta.
> + * @mask: The mask set for bus protect.
> + * @flag: The flag to idetify which operation we take for bus protect.
> + */
> +struct bus_mask {
> + struct ext_reg_ctrl *regs;
> + u32 mask;
> + const struct bus_mask_ops *ops;
> +};
> +
> +/**
> + * struct scpsys_ext_attr - extended attribute for bus protect and further
> + * operand.
> + *
> + * @scpd_n: The name present the scpsys domain where the clks belongs to.
> + * @mask: The mask set for bus protect.
> + * @bus_ops: The operation we take for bus protect.
> + * @cg_ops: The operation we take for cg on/off.
> + * @attr_list: The list node linked to ext_attr_map_list.
> + */
> +struct scpsys_ext_attr {
> + const char *scpd_n;
> + struct bus_mask mask[MAX_STEP_NUM];
> + const char *parent_n;
> + const struct bus_ext_ops *bus_ops;
> + const struct bus_ext_ops *cg_ops;
> +
> + struct list_head attr_list;
> +};
> +
> +struct scpsys_ext_data {
> + struct scpsys_ext_attr *attr;
> + u8 num_attr;
> + struct scpsys_ext_attr * (*get_attr)(const char *scpd_n);
> +};
> +
> +struct bus_ext_ops {
> + int (*enable)(struct scpsys_ext_attr *attr);
> + int (*disable)(struct scpsys_ext_attr *attr);
> +};
> +
> +int mtk_generic_set_cmd(struct regmap *regmap, u32 set_ofs,
> + u32 sta_ofs, u32 mask);
> +int mtk_generic_clr_cmd(struct regmap *regmap, u32 clr_ofs,
> + u32 sta_ofs, u32 mask);
> +int mtk_generic_enable_cmd(struct regmap *regmap, u32 upd_ofs,
> + u32 sta_ofs, u32 mask);
> +int mtk_generic_disable_cmd(struct regmap *regmap, u32 upd_ofs,
> + u32 sta_ofs, u32 mask);
> +
> +struct scpsys_ext_data *scpsys_ext_init(struct platform_device *pdev);
> +
> +#endif /* __SOC_MEDIATEK_SCPSYS_EXT_H */
>