Both MT8183 & MT6765 have more control steps of bus protection
than previous project. And there add more bus protection registers
reside at infracfg & smi-common. Also add new APIs for multiple
step bus protection control with more customized arguments.
And then use bp_table for bus protection of all compatibles,
instead of mixing bus_prot_mask and bus_prot_reg_update.
Signed-off-by: Weiyi Lu <[email protected]>
---
drivers/soc/mediatek/Makefile | 2 +-
drivers/soc/mediatek/mtk-scpsys-ext.c | 101 +++++++++++++++++++++++++++++
drivers/soc/mediatek/mtk-scpsys.c | 117 +++++++++++++++++++++-------------
drivers/soc/mediatek/scpsys-ext.h | 67 +++++++++++++++++++
4 files changed, 240 insertions(+), 47 deletions(-)
create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
create mode 100644 drivers/soc/mediatek/scpsys-ext.h
diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index b017330..b442be9 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
-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-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
new file mode 100644
index 0000000..df402ac
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ * Author: Owen Chen <[email protected]>
+ */
+#include <linux/ktime.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include "scpsys-ext.h"
+
+#define MTK_POLL_DELAY_US 10
+#define MTK_POLL_TIMEOUT USEC_PER_SEC
+
+static int set_bus_protection(struct regmap *map, u32 mask,
+ u32 reg_set, u32 reg_sta, u32 reg_en)
+{
+ u32 val;
+
+ if (reg_set)
+ regmap_write(map, reg_set, mask);
+ else
+ regmap_update_bits(map, reg_en, mask, mask);
+
+ return regmap_read_poll_timeout(map, reg_sta,
+ val, (val & mask) == mask,
+ MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
+}
+
+static int clear_bus_protection(struct regmap *map, u32 mask,
+ u32 reg_clr, u32 reg_sta, u32 reg_en, bool ignore_ack)
+{
+ u32 val;
+
+ if (reg_clr)
+ regmap_write(map, reg_clr, mask);
+ else
+ regmap_update_bits(map, reg_en, mask, 0);
+
+ if (ignore_ack)
+ return 0;
+
+ return regmap_read_poll_timeout(map, reg_sta,
+ val, !(val & mask),
+ MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
+}
+
+int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
+ struct regmap *infracfg, struct regmap *smi_common)
+{
+ int i;
+
+ for (i = 0; i < MAX_STEPS; i++) {
+ struct regmap *map = NULL;
+ int ret;
+
+ if (bp_table[i].type == INVALID_TYPE)
+ break;
+ else if (bp_table[i].type == IFR_TYPE)
+ map = infracfg;
+ else if (bp_table[i].type == SMI_TYPE)
+ map = smi_common;
+
+ ret = set_bus_protection(map,
+ bp_table[i].mask, bp_table[i].set_ofs,
+ bp_table[i].sta_ofs, bp_table[i].en_ofs);
+
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
+ struct regmap *infracfg, struct regmap *smi_common)
+{
+ int i;
+
+ for (i = MAX_STEPS - 1; i >= 0; i--) {
+ struct regmap *map = NULL;
+ int ret;
+
+ if (bp_table[i].type == INVALID_TYPE)
+ continue;
+ else if (bp_table[i].type == IFR_TYPE)
+ map = infracfg;
+ else if (bp_table[i].type == SMI_TYPE)
+ map = smi_common;
+
+ ret = clear_bus_protection(map,
+ bp_table[i].mask, bp_table[i].clr_ofs,
+ bp_table[i].sta_ofs, bp_table[i].en_ofs,
+ bp_table[i].ignore_clr_ack);
+
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
index db35a28..763ca58 100644
--- a/drivers/soc/mediatek/mtk-scpsys.c
+++ b/drivers/soc/mediatek/mtk-scpsys.c
@@ -11,7 +11,7 @@
#include <linux/platform_device.h>
#include <linux/pm_domain.h>
#include <linux/regulator/consumer.h>
-#include <linux/soc/mediatek/infracfg.h>
+#include "scpsys-ext.h"
#include <dt-bindings/power/mt2701-power.h>
#include <dt-bindings/power/mt2712-power.h>
@@ -87,9 +87,9 @@
* @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.
- * @bus_prot_mask: The mask for single step bus protection.
* @basic_clk_name: The basic clocks required by this power domain.
* @caps: The flag for active wake-up action.
+ * @bp_table: The mask table for multiple step bus protection.
*/
struct scp_domain_data {
const char *name;
@@ -97,9 +97,9 @@ struct scp_domain_data {
int ctl_offs;
u32 sram_pdn_bits;
u32 sram_pdn_ack_bits;
- u32 bus_prot_mask;
const char *basic_clk_name[MAX_CLKS];
u8 caps;
+ struct bus_prot bp_table[MAX_STEPS];
};
struct scp;
@@ -123,8 +123,8 @@ struct scp {
struct device *dev;
void __iomem *base;
struct regmap *infracfg;
+ struct regmap *smi_common;
struct scp_ctrl_reg ctrl_reg;
- bool bus_prot_reg_update;
};
struct scp_subdomain {
@@ -138,7 +138,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)
@@ -252,24 +251,16 @@ static int scpsys_bus_protect_enable(struct scp_domain *scpd)
{
struct scp *scp = scpd->scp;
- if (!scpd->data->bus_prot_mask)
- return 0;
-
- return mtk_infracfg_set_bus_protection(scp->infracfg,
- scpd->data->bus_prot_mask,
- scp->bus_prot_reg_update);
+ return mtk_scpsys_ext_set_bus_protection(scpd->data->bp_table,
+ scp->infracfg, scp->smi_common);
}
static int scpsys_bus_protect_disable(struct scp_domain *scpd)
{
struct scp *scp = scpd->scp;
- if (!scpd->data->bus_prot_mask)
- return 0;
-
- return mtk_infracfg_clear_bus_protection(scp->infracfg,
- scpd->data->bus_prot_mask,
- scp->bus_prot_reg_update);
+ return mtk_scpsys_ext_clear_bus_protection(scpd->data->bp_table,
+ scp->infracfg, scp->smi_common);
}
static int scpsys_power_on(struct generic_pm_domain *genpd)
@@ -404,8 +395,7 @@ static int init_basic_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;
@@ -419,8 +409,6 @@ 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;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -448,6 +436,17 @@ static struct scp *init_scp(struct platform_device *pdev,
return ERR_CAST(scp->infracfg);
}
+ scp->smi_common = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+ "smi_comm");
+
+ if (scp->smi_common == ERR_PTR(-ENODEV)) {
+ scp->smi_common = NULL;
+ } else if (IS_ERR(scp->smi_common)) {
+ dev_err(&pdev->dev, "Cannot find smi_common controller: %ld\n",
+ PTR_ERR(scp->smi_common));
+ return ERR_CAST(scp->smi_common);
+ }
+
for (i = 0; i < num; i++) {
struct scp_domain *scpd = &scp->domains[i];
const struct scp_domain_data *data = &scp_domain_data[i];
@@ -530,8 +529,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.name = "conn",
.sta_mask = PWR_STATUS_CONN,
.ctl_offs = SPM_CONN_PWR_CON,
- .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
- MT2701_TOP_AXI_PROT_EN_CONN_S,
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
+ MT2701_TOP_AXI_PROT_EN_CONN_M |
+ MT2701_TOP_AXI_PROT_EN_CONN_S),
+ },
.caps = MTK_SCPD_ACTIVE_WAKEUP,
},
[MT2701_POWER_DOMAIN_DISP] = {
@@ -540,7 +542,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.ctl_offs = SPM_DIS_PWR_CON,
.sram_pdn_bits = GENMASK(11, 8),
.basic_clk_name = {"mm"},
- .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_MM_M0,
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
+ MT2701_TOP_AXI_PROT_EN_MM_M0),
+ },
.caps = MTK_SCPD_ACTIVE_WAKEUP,
},
[MT2701_POWER_DOMAIN_MFG] = {
@@ -675,7 +680,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.sram_pdn_bits = GENMASK(8, 8),
.sram_pdn_ack_bits = GENMASK(16, 16),
.basic_clk_name = {"mfg"},
- .bus_prot_mask = BIT(14) | BIT(21) | BIT(23),
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0x260, 0x264, 0x220, 0x228,
+ BIT(14) | BIT(21) | BIT(23)),
+ },
.caps = MTK_SCPD_ACTIVE_WAKEUP,
},
[MT2712_POWER_DOMAIN_MFG_SC1] = {
@@ -747,7 +755,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.sram_pdn_bits = GENMASK(8, 8),
.sram_pdn_ack_bits = GENMASK(12, 12),
.basic_clk_name = {"mm"},
- .bus_prot_mask = (BIT(1) | BIT(2)),
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
+ BIT(1) | BIT(2)),
+ },
},
[MT6797_POWER_DOMAIN_AUDIO] = {
.name = "audio",
@@ -794,7 +805,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.ctl_offs = SPM_ETHSYS_PWR_CON,
.sram_pdn_bits = GENMASK(11, 8),
.sram_pdn_ack_bits = GENMASK(15, 12),
- .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_ETHSYS,
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
+ MT7622_TOP_AXI_PROT_EN_ETHSYS),
+ },
.caps = MTK_SCPD_ACTIVE_WAKEUP,
},
[MT7622_POWER_DOMAIN_HIF0] = {
@@ -804,7 +818,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.sram_pdn_bits = GENMASK(11, 8),
.sram_pdn_ack_bits = GENMASK(15, 12),
.basic_clk_name = {"hif_sel"},
- .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_HIF0,
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
+ MT7622_TOP_AXI_PROT_EN_HIF0),
+ },
.caps = MTK_SCPD_ACTIVE_WAKEUP,
},
[MT7622_POWER_DOMAIN_HIF1] = {
@@ -814,7 +831,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.sram_pdn_bits = GENMASK(11, 8),
.sram_pdn_ack_bits = GENMASK(15, 12),
.basic_clk_name = {"hif_sel"},
- .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_HIF1,
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
+ MT7622_TOP_AXI_PROT_EN_HIF1),
+ },
.caps = MTK_SCPD_ACTIVE_WAKEUP,
},
[MT7622_POWER_DOMAIN_WB] = {
@@ -823,7 +843,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.ctl_offs = SPM_WB_PWR_CON,
.sram_pdn_bits = 0,
.sram_pdn_ack_bits = 0,
- .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_WB,
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
+ MT7622_TOP_AXI_PROT_EN_WB),
+ },
.caps = MTK_SCPD_ACTIVE_WAKEUP | MTK_SCPD_FWAIT_SRAM,
},
};
@@ -837,8 +860,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.name = "conn",
.sta_mask = PWR_STATUS_CONN,
.ctl_offs = SPM_CONN_PWR_CON,
- .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
- MT2701_TOP_AXI_PROT_EN_CONN_S,
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
+ MT2701_TOP_AXI_PROT_EN_CONN_M |
+ MT2701_TOP_AXI_PROT_EN_CONN_S),
+ },
.caps = MTK_SCPD_ACTIVE_WAKEUP,
},
[MT7623A_POWER_DOMAIN_ETH] = {
@@ -903,8 +929,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.sram_pdn_bits = GENMASK(11, 8),
.sram_pdn_ack_bits = GENMASK(12, 12),
.basic_clk_name = {"mm"},
- .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
- MT8173_TOP_AXI_PROT_EN_MM_M1,
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
+ MT8173_TOP_AXI_PROT_EN_MM_M0 |
+ MT8173_TOP_AXI_PROT_EN_MM_M1),
+ },
},
[MT8173_POWER_DOMAIN_VENC_LT] = {
.name = "venc_lt",
@@ -950,10 +979,13 @@ static void mtk_register_power_domains(struct platform_device *pdev,
.ctl_offs = SPM_MFG_PWR_CON,
.sram_pdn_bits = GENMASK(13, 8),
.sram_pdn_ack_bits = GENMASK(21, 16),
- .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
- MT8173_TOP_AXI_PROT_EN_MFG_M0 |
- MT8173_TOP_AXI_PROT_EN_MFG_M1 |
- MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
+ .bp_table = {
+ BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
+ MT8173_TOP_AXI_PROT_EN_MFG_S |
+ MT8173_TOP_AXI_PROT_EN_MFG_M0 |
+ MT8173_TOP_AXI_PROT_EN_MFG_M1 |
+ MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT),
+ },
},
};
@@ -969,7 +1001,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 = {
@@ -981,7 +1012,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 mt6797_data = {
@@ -993,7 +1023,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,
};
static const struct scp_soc_data mt7622_data = {
@@ -1003,7 +1032,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 = {
@@ -1013,7 +1041,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 = {
@@ -1025,7 +1052,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,
};
/*
@@ -1066,8 +1092,7 @@ 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/drivers/soc/mediatek/scpsys-ext.h b/drivers/soc/mediatek/scpsys-ext.h
new file mode 100644
index 0000000..458b2c5
--- /dev/null
+++ b/drivers/soc/mediatek/scpsys-ext.h
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SOC_MEDIATEK_SCPSYS_EXT_H
+#define __SOC_MEDIATEK_SCPSYS_EXT_H
+
+#define MAX_STEPS 4
+
+#define _BUS_PROT(_type, _set_ofs, _clr_ofs, \
+ _en_ofs, _sta_ofs, _mask, _ignore_clr_ack) { \
+ .type = _type, \
+ .set_ofs = _set_ofs, \
+ .clr_ofs = _clr_ofs, \
+ .en_ofs = _en_ofs, \
+ .sta_ofs = _sta_ofs, \
+ .mask = _mask, \
+ .ignore_clr_ack = _ignore_clr_ack, \
+ }
+
+#define BUS_PROT(_type, _set_ofs, _clr_ofs, \
+ _en_ofs, _sta_ofs, _mask) \
+ _BUS_PROT(_type, _set_ofs, _clr_ofs, \
+ _en_ofs, _sta_ofs, _mask, false)
+
+#define BUS_PROT_IGN(_type, _set_ofs, _clr_ofs, \
+ _en_ofs, _sta_ofs, _mask) \
+ _BUS_PROT(_type, _set_ofs, _clr_ofs, \
+ _en_ofs, _sta_ofs, _mask, true)
+
+#define MT2701_TOP_AXI_PROT_EN_MM_M0 BIT(1)
+#define MT2701_TOP_AXI_PROT_EN_CONN_M BIT(2)
+#define MT2701_TOP_AXI_PROT_EN_CONN_S BIT(8)
+
+#define MT7622_TOP_AXI_PROT_EN_ETHSYS (BIT(3) | BIT(17))
+#define MT7622_TOP_AXI_PROT_EN_HIF0 (BIT(24) | BIT(25))
+#define MT7622_TOP_AXI_PROT_EN_HIF1 (BIT(26) | BIT(27) | \
+ BIT(28))
+#define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
+ BIT(7) | BIT(8))
+
+#define MT8173_TOP_AXI_PROT_EN_MM_M0 BIT(1)
+#define MT8173_TOP_AXI_PROT_EN_MM_M1 BIT(2)
+#define MT8173_TOP_AXI_PROT_EN_MFG_S BIT(14)
+#define MT8173_TOP_AXI_PROT_EN_MFG_M0 BIT(21)
+#define MT8173_TOP_AXI_PROT_EN_MFG_M1 BIT(22)
+#define MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT BIT(23)
+
+enum regmap_type {
+ INVALID_TYPE = 0,
+ IFR_TYPE,
+ SMI_TYPE,
+};
+
+struct bus_prot {
+ enum regmap_type type;
+ u32 set_ofs;
+ u32 clr_ofs;
+ u32 en_ofs;
+ u32 sta_ofs;
+ u32 mask;
+ bool ignore_clr_ack;
+};
+
+int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
+ struct regmap *infracfg, struct regmap *smi_common);
+int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
+ struct regmap *infracfg, struct regmap *smi_common);
+
+#endif /* __SOC_MEDIATEK_SCPSYS_EXT_H */
--
1.8.1.1.dirty
On Fri, Dec 20, 2019 at 11:46 AM Weiyi Lu <[email protected]> wrote:
>
> Both MT8183 & MT6765 have more control steps of bus protection
> than previous project. And there add more bus protection registers
> reside at infracfg & smi-common. Also add new APIs for multiple
> step bus protection control with more customized arguments.
> And then use bp_table for bus protection of all compatibles,
> instead of mixing bus_prot_mask and bus_prot_reg_update.
>
> Signed-off-by: Weiyi Lu <[email protected]>
I think this looks much better, thanks!
Reviewed-by: Nicolas Boichat <[email protected]>
> ---
> drivers/soc/mediatek/Makefile | 2 +-
> drivers/soc/mediatek/mtk-scpsys-ext.c | 101 +++++++++++++++++++++++++++++
> drivers/soc/mediatek/mtk-scpsys.c | 117 +++++++++++++++++++++-------------
> drivers/soc/mediatek/scpsys-ext.h | 67 +++++++++++++++++++
> 4 files changed, 240 insertions(+), 47 deletions(-)
> create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> create mode 100644 drivers/soc/mediatek/scpsys-ext.h
On 20/12/2019 04:45, Weiyi Lu wrote:
> Both MT8183 & MT6765 have more control steps of bus protection
> than previous project. And there add more bus protection registers
> reside at infracfg & smi-common. Also add new APIs for multiple
> step bus protection control with more customized arguments.
> And then use bp_table for bus protection of all compatibles,
> instead of mixing bus_prot_mask and bus_prot_reg_update.
>
> Signed-off-by: Weiyi Lu <[email protected]>
> ---
> drivers/soc/mediatek/Makefile | 2 +-
> drivers/soc/mediatek/mtk-scpsys-ext.c | 101 +++++++++++++++++++++++++++++
> drivers/soc/mediatek/mtk-scpsys.c | 117 +++++++++++++++++++++-------------
> drivers/soc/mediatek/scpsys-ext.h | 67 +++++++++++++++++++
> 4 files changed, 240 insertions(+), 47 deletions(-)
> create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> create mode 100644 drivers/soc/mediatek/scpsys-ext.h
>
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index b017330..b442be9 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1,5 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0-only
> obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
It seems that we would need another patch which get's rid of the mtk-infracfg
first and then add stuff like the possibility to have different steps.
> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> new file mode 100644
> index 0000000..df402ac
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
I'm not quite sure why we should put this into a new file. I suppose the
rational behind it is the fact that we access other blocks through regmap.
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Owen Chen <[email protected]>
> + */
> +#include <linux/ktime.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include "scpsys-ext.h"
> +
> +#define MTK_POLL_DELAY_US 10
> +#define MTK_POLL_TIMEOUT USEC_PER_SEC
> +
> +static int set_bus_protection(struct regmap *map, u32 mask,
> + u32 reg_set, u32 reg_sta, u32 reg_en)
> +{
> + u32 val;
> +
> + if (reg_set)
> + regmap_write(map, reg_set, mask);
> + else
> + regmap_update_bits(map, reg_en, mask, mask);
> +
> + return regmap_read_poll_timeout(map, reg_sta,
> + val, (val & mask) == mask,
> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> +}
> +
> +static int clear_bus_protection(struct regmap *map, u32 mask,
> + u32 reg_clr, u32 reg_sta, u32 reg_en, bool ignore_ack)
> +{
> + u32 val;
> +
> + if (reg_clr)
> + regmap_write(map, reg_clr, mask);
> + else
> + regmap_update_bits(map, reg_en, mask, 0);
> +
> + if (ignore_ack)
> + return 0;
> +
> + return regmap_read_poll_timeout(map, reg_sta,
> + val, !(val & mask),
> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> +}
> +
> +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
> + struct regmap *infracfg, struct regmap *smi_common)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_STEPS; i++) {
struct bus_prot bp_table = bp_table[i];
> + struct regmap *map = NULL;
> + int ret;
> +
> + if (bp_table[i].type == INVALID_TYPE)
> + break;
> + else if (bp_table[i].type == IFR_TYPE)
> + map = infracfg;
> + else if (bp_table[i].type == SMI_TYPE)
> + map = smi_common;
if (bp_table.type == IFR_TYPE)
map = infracfg;
else if (bp_table.type == SMI_TYPE)
map = smi_common;
else
break;
> +
> + ret = set_bus_protection(map,
> + bp_table[i].mask, bp_table[i].set_ofs,
> + bp_table[i].sta_ofs, bp_table[i].en_ofs);
passing map and bp_table here makes code much more readable. I always end-up
counting the variables when the names in the callee changes.
> +
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
> + struct regmap *infracfg, struct regmap *smi_common)
> +{
> + int i;
> +
> + for (i = MAX_STEPS - 1; i >= 0; i--) {
> + struct regmap *map = NULL;
> + int ret;
> +
> + if (bp_table[i].type == INVALID_TYPE)
> + continue;
> + else if (bp_table[i].type == IFR_TYPE)
> + map = infracfg;
> + else if (bp_table[i].type == SMI_TYPE)
> + map = smi_common;
> +
> + ret = clear_bus_protection(map,
> + bp_table[i].mask, bp_table[i].clr_ofs,
> + bp_table[i].sta_ofs, bp_table[i].en_ofs,
> + bp_table[i].ignore_clr_ack);
> +
same here.
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> index db35a28..763ca58 100644
> --- a/drivers/soc/mediatek/mtk-scpsys.c
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -11,7 +11,7 @@
> #include <linux/platform_device.h>
> #include <linux/pm_domain.h>
> #include <linux/regulator/consumer.h>
> -#include <linux/soc/mediatek/infracfg.h>
> +#include "scpsys-ext.h"
>
> #include <dt-bindings/power/mt2701-power.h>
> #include <dt-bindings/power/mt2712-power.h>
> @@ -87,9 +87,9 @@
> * @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.
> - * @bus_prot_mask: The mask for single step bus protection.
> * @basic_clk_name: The basic clocks required by this power domain.
> * @caps: The flag for active wake-up action.
> + * @bp_table: The mask table for multiple step bus protection.
> */
> struct scp_domain_data {
> const char *name;
> @@ -97,9 +97,9 @@ struct scp_domain_data {
> int ctl_offs;
> u32 sram_pdn_bits;
> u32 sram_pdn_ack_bits;
> - u32 bus_prot_mask;
> const char *basic_clk_name[MAX_CLKS];
> u8 caps;
> + struct bus_prot bp_table[MAX_STEPS];
> };
>
> struct scp;
> @@ -123,8 +123,8 @@ struct scp {
> struct device *dev;
> void __iomem *base;
> struct regmap *infracfg;
> + struct regmap *smi_common;
> struct scp_ctrl_reg ctrl_reg;
> - bool bus_prot_reg_update;
> };
>
> struct scp_subdomain {
> @@ -138,7 +138,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)
> @@ -252,24 +251,16 @@ static int scpsys_bus_protect_enable(struct scp_domain *scpd)
> {
> struct scp *scp = scpd->scp;
>
> - if (!scpd->data->bus_prot_mask)
> - return 0;
> -
> - return mtk_infracfg_set_bus_protection(scp->infracfg,
> - scpd->data->bus_prot_mask,
> - scp->bus_prot_reg_update);
> + return mtk_scpsys_ext_set_bus_protection(scpd->data->bp_table,
> + scp->infracfg, scp->smi_common);
Now that's just a wrapper which does add nothing but another indirection. We can
call mtk_scpsys_ext_set_bus_protection() directly (also I don't like the name,
actually why don't mtk_scpsys_set_bus_protection()?)
> }
>
> static int scpsys_bus_protect_disable(struct scp_domain *scpd)
> {
> struct scp *scp = scpd->scp;
>
> - if (!scpd->data->bus_prot_mask)
> - return 0;
> -
> - return mtk_infracfg_clear_bus_protection(scp->infracfg,
> - scpd->data->bus_prot_mask,
> - scp->bus_prot_reg_update);
> + return mtk_scpsys_ext_clear_bus_protection(scpd->data->bp_table,
> + scp->infracfg, scp->smi_common);
> }
>
> static int scpsys_power_on(struct generic_pm_domain *genpd)
> @@ -404,8 +395,7 @@ static int init_basic_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;
> @@ -419,8 +409,6 @@ 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;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -448,6 +436,17 @@ static struct scp *init_scp(struct platform_device *pdev,
> return ERR_CAST(scp->infracfg);
> }
>
> + scp->smi_common = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> + "smi_comm");
> +
> + if (scp->smi_common == ERR_PTR(-ENODEV)) {
> + scp->smi_common = NULL;
> + } else if (IS_ERR(scp->smi_common)) {
> + dev_err(&pdev->dev, "Cannot find smi_common controller: %ld\n",
> + PTR_ERR(scp->smi_common));
> + return ERR_CAST(scp->smi_common);
> + }
> +
> for (i = 0; i < num; i++) {
> struct scp_domain *scpd = &scp->domains[i];
> const struct scp_domain_data *data = &scp_domain_data[i];
> @@ -530,8 +529,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .name = "conn",
> .sta_mask = PWR_STATUS_CONN,
> .ctl_offs = SPM_CONN_PWR_CON,
> - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
> - MT2701_TOP_AXI_PROT_EN_CONN_S,
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> + MT2701_TOP_AXI_PROT_EN_CONN_M |
> + MT2701_TOP_AXI_PROT_EN_CONN_S),
> + },
> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> },
> [MT2701_POWER_DOMAIN_DISP] = {
> @@ -540,7 +542,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .ctl_offs = SPM_DIS_PWR_CON,
> .sram_pdn_bits = GENMASK(11, 8),
> .basic_clk_name = {"mm"},
> - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_MM_M0,
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> + MT2701_TOP_AXI_PROT_EN_MM_M0),
> + },
> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> },
> [MT2701_POWER_DOMAIN_MFG] = {
> @@ -675,7 +680,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .sram_pdn_bits = GENMASK(8, 8),
> .sram_pdn_ack_bits = GENMASK(16, 16),
> .basic_clk_name = {"mfg"},
> - .bus_prot_mask = BIT(14) | BIT(21) | BIT(23),
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0x260, 0x264, 0x220, 0x228,
> + BIT(14) | BIT(21) | BIT(23)),
> + },
> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> },
> [MT2712_POWER_DOMAIN_MFG_SC1] = {
> @@ -747,7 +755,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .sram_pdn_bits = GENMASK(8, 8),
> .sram_pdn_ack_bits = GENMASK(12, 12),
> .basic_clk_name = {"mm"},
> - .bus_prot_mask = (BIT(1) | BIT(2)),
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> + BIT(1) | BIT(2)),
> + },
> },
> [MT6797_POWER_DOMAIN_AUDIO] = {
> .name = "audio",
> @@ -794,7 +805,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .ctl_offs = SPM_ETHSYS_PWR_CON,
> .sram_pdn_bits = GENMASK(11, 8),
> .sram_pdn_ack_bits = GENMASK(15, 12),
> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_ETHSYS,
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> + MT7622_TOP_AXI_PROT_EN_ETHSYS),
> + },
> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> },
> [MT7622_POWER_DOMAIN_HIF0] = {
> @@ -804,7 +818,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .sram_pdn_bits = GENMASK(11, 8),
> .sram_pdn_ack_bits = GENMASK(15, 12),
> .basic_clk_name = {"hif_sel"},
> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_HIF0,
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> + MT7622_TOP_AXI_PROT_EN_HIF0),
> + },
> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> },
> [MT7622_POWER_DOMAIN_HIF1] = {
> @@ -814,7 +831,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .sram_pdn_bits = GENMASK(11, 8),
> .sram_pdn_ack_bits = GENMASK(15, 12),
> .basic_clk_name = {"hif_sel"},
> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_HIF1,
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> + MT7622_TOP_AXI_PROT_EN_HIF1),
> + },
> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> },
> [MT7622_POWER_DOMAIN_WB] = {
> @@ -823,7 +843,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .ctl_offs = SPM_WB_PWR_CON,
> .sram_pdn_bits = 0,
> .sram_pdn_ack_bits = 0,
> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_WB,
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> + MT7622_TOP_AXI_PROT_EN_WB),
> + },
> .caps = MTK_SCPD_ACTIVE_WAKEUP | MTK_SCPD_FWAIT_SRAM,
> },
> };
> @@ -837,8 +860,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .name = "conn",
> .sta_mask = PWR_STATUS_CONN,
> .ctl_offs = SPM_CONN_PWR_CON,
> - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
> - MT2701_TOP_AXI_PROT_EN_CONN_S,
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> + MT2701_TOP_AXI_PROT_EN_CONN_M |
> + MT2701_TOP_AXI_PROT_EN_CONN_S),
> + },
> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> },
> [MT7623A_POWER_DOMAIN_ETH] = {
> @@ -903,8 +929,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .sram_pdn_bits = GENMASK(11, 8),
> .sram_pdn_ack_bits = GENMASK(12, 12),
> .basic_clk_name = {"mm"},
> - .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
> - MT8173_TOP_AXI_PROT_EN_MM_M1,
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> + MT8173_TOP_AXI_PROT_EN_MM_M0 |
> + MT8173_TOP_AXI_PROT_EN_MM_M1),
> + },
> },
> [MT8173_POWER_DOMAIN_VENC_LT] = {
> .name = "venc_lt",
> @@ -950,10 +979,13 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .ctl_offs = SPM_MFG_PWR_CON,
> .sram_pdn_bits = GENMASK(13, 8),
> .sram_pdn_ack_bits = GENMASK(21, 16),
> - .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
> - MT8173_TOP_AXI_PROT_EN_MFG_M0 |
> - MT8173_TOP_AXI_PROT_EN_MFG_M1 |
> - MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
> + .bp_table = {
> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> + MT8173_TOP_AXI_PROT_EN_MFG_S |
> + MT8173_TOP_AXI_PROT_EN_MFG_M0 |
> + MT8173_TOP_AXI_PROT_EN_MFG_M1 |
> + MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT),
> + },
> },
> };
>
> @@ -969,7 +1001,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 = {
> @@ -981,7 +1012,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 mt6797_data = {
> @@ -993,7 +1023,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,
> };
>
> static const struct scp_soc_data mt7622_data = {
> @@ -1003,7 +1032,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 = {
> @@ -1013,7 +1041,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 = {
> @@ -1025,7 +1052,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,
> };
>
> /*
> @@ -1066,8 +1092,7 @@ 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/drivers/soc/mediatek/scpsys-ext.h b/drivers/soc/mediatek/scpsys-ext.h
> new file mode 100644
> index 0000000..458b2c5
> --- /dev/null
> +++ b/drivers/soc/mediatek/scpsys-ext.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SOC_MEDIATEK_SCPSYS_EXT_H
> +#define __SOC_MEDIATEK_SCPSYS_EXT_H
> +
> +#define MAX_STEPS 4
> +
> +#define _BUS_PROT(_type, _set_ofs, _clr_ofs, \
> + _en_ofs, _sta_ofs, _mask, _ignore_clr_ack) { \
> + .type = _type, \
> + .set_ofs = _set_ofs, \
> + .clr_ofs = _clr_ofs, \
> + .en_ofs = _en_ofs, \
> + .sta_ofs = _sta_ofs, \
> + .mask = _mask, \
> + .ignore_clr_ack = _ignore_clr_ack, \
> + }
> +
> +#define BUS_PROT(_type, _set_ofs, _clr_ofs, \
> + _en_ofs, _sta_ofs, _mask) \
> + _BUS_PROT(_type, _set_ofs, _clr_ofs, \
> + _en_ofs, _sta_ofs, _mask, false)
> +
> +#define BUS_PROT_IGN(_type, _set_ofs, _clr_ofs, \
> + _en_ofs, _sta_ofs, _mask) \
> + _BUS_PROT(_type, _set_ofs, _clr_ofs, \
> + _en_ofs, _sta_ofs, _mask, true)
> +
> +#define MT2701_TOP_AXI_PROT_EN_MM_M0 BIT(1)
> +#define MT2701_TOP_AXI_PROT_EN_CONN_M BIT(2)
> +#define MT2701_TOP_AXI_PROT_EN_CONN_S BIT(8)
> +
> +#define MT7622_TOP_AXI_PROT_EN_ETHSYS (BIT(3) | BIT(17))
> +#define MT7622_TOP_AXI_PROT_EN_HIF0 (BIT(24) | BIT(25))
> +#define MT7622_TOP_AXI_PROT_EN_HIF1 (BIT(26) | BIT(27) | \
> + BIT(28))
> +#define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
> + BIT(7) | BIT(8))
> +
> +#define MT8173_TOP_AXI_PROT_EN_MM_M0 BIT(1)
> +#define MT8173_TOP_AXI_PROT_EN_MM_M1 BIT(2)
> +#define MT8173_TOP_AXI_PROT_EN_MFG_S BIT(14)
> +#define MT8173_TOP_AXI_PROT_EN_MFG_M0 BIT(21)
> +#define MT8173_TOP_AXI_PROT_EN_MFG_M1 BIT(22)
> +#define MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT BIT(23)
> +
> +enum regmap_type {
> + INVALID_TYPE = 0,
> + IFR_TYPE,
> + SMI_TYPE,
> +};
> +
> +struct bus_prot {
> + enum regmap_type type;
> + u32 set_ofs;
> + u32 clr_ofs;
Please try to keep variable names consistent. You change for example set_ofs to
reg_set. That makes the code more difficult to read and understand.
> + u32 en_ofs;
> + u32 sta_ofs;
> + u32 mask;
> + bool ignore_clr_ack;
> +};
> +
> +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
> + struct regmap *infracfg, struct regmap *smi_common);
> +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
> + struct regmap *infracfg, struct regmap *smi_common);
> +
> +#endif /* __SOC_MEDIATEK_SCPSYS_EXT_H */
>
On Tue, 2020-02-11 at 18:49 +0100, Matthias Brugger wrote:
>
> On 20/12/2019 04:45, Weiyi Lu wrote:
> > Both MT8183 & MT6765 have more control steps of bus protection
> > than previous project. And there add more bus protection registers
> > reside at infracfg & smi-common. Also add new APIs for multiple
> > step bus protection control with more customized arguments.
> > And then use bp_table for bus protection of all compatibles,
> > instead of mixing bus_prot_mask and bus_prot_reg_update.
> >
> > Signed-off-by: Weiyi Lu <[email protected]>
> > ---
> > drivers/soc/mediatek/Makefile | 2 +-
> > drivers/soc/mediatek/mtk-scpsys-ext.c | 101 +++++++++++++++++++++++++++++
> > drivers/soc/mediatek/mtk-scpsys.c | 117 +++++++++++++++++++++-------------
> > drivers/soc/mediatek/scpsys-ext.h | 67 +++++++++++++++++++
> > 4 files changed, 240 insertions(+), 47 deletions(-)
> > create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> > create mode 100644 drivers/soc/mediatek/scpsys-ext.h
> >
> > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> > index b017330..b442be9 100644
> > --- a/drivers/soc/mediatek/Makefile
> > +++ b/drivers/soc/mediatek/Makefile
> > @@ -1,5 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> > -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
>
> It seems that we would need another patch which get's rid of the mtk-infracfg
> first and then add stuff like the possibility to have different steps.
>
Actually I have a PATCH 05/11 to remove the mtk-infracfg.
In this patch, I have some changes, like calling
mtk_scpsys_ext_set_bus_protection(...) instead of
mtk_infracfg_set_bus_protection(...) in scpsys_bus_protect_enable(...)
and replacing bus_prot_mask by bp_table.
I thought I should introduce the new method first and remove useless one
later. What do you think?
> > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> > diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > new file mode 100644
> > index 0000000..df402ac
> > --- /dev/null
> > +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
>
> I'm not quite sure why we should put this into a new file. I suppose the
> rational behind it is the fact that we access other blocks through regmap.
>
Yes, those operation are accross infracfg and smi-common so we put these
into new files.
> > @@ -0,0 +1,101 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <[email protected]>
> > + */
> > +#include <linux/ktime.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_device.h>
> > +#include <linux/regmap.h>
> > +#include "scpsys-ext.h"
> > +
> > +#define MTK_POLL_DELAY_US 10
> > +#define MTK_POLL_TIMEOUT USEC_PER_SEC
> > +
> > +static int set_bus_protection(struct regmap *map, u32 mask,
> > + u32 reg_set, u32 reg_sta, u32 reg_en)
> > +{
> > + u32 val;
> > +
> > + if (reg_set)
> > + regmap_write(map, reg_set, mask);
> > + else
> > + regmap_update_bits(map, reg_en, mask, mask);
> > +
> > + return regmap_read_poll_timeout(map, reg_sta,
> > + val, (val & mask) == mask,
> > + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > +}
> > +
> > +static int clear_bus_protection(struct regmap *map, u32 mask,
> > + u32 reg_clr, u32 reg_sta, u32 reg_en, bool ignore_ack)
> > +{
> > + u32 val;
> > +
> > + if (reg_clr)
> > + regmap_write(map, reg_clr, mask);
> > + else
> > + regmap_update_bits(map, reg_en, mask, 0);
> > +
> > + if (ignore_ack)
> > + return 0;
> > +
> > + return regmap_read_poll_timeout(map, reg_sta,
> > + val, !(val & mask),
> > + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > +}
> > +
> > +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
> > + struct regmap *infracfg, struct regmap *smi_common)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < MAX_STEPS; i++) {
>
> struct bus_prot bp_table = bp_table[i];
>
OK, got it.
> > + struct regmap *map = NULL;
> > + int ret;
> > +
> > + if (bp_table[i].type == INVALID_TYPE)
> > + break;
> > + else if (bp_table[i].type == IFR_TYPE)
> > + map = infracfg;
> > + else if (bp_table[i].type == SMI_TYPE)
> > + map = smi_common;
>
> if (bp_table.type == IFR_TYPE)
> map = infracfg;
> else if (bp_table.type == SMI_TYPE)
> map = smi_common;
> else
> break;
>
OK, got it.
> > +
> > + ret = set_bus_protection(map,
> > + bp_table[i].mask, bp_table[i].set_ofs,
> > + bp_table[i].sta_ofs, bp_table[i].en_ofs);
>
> passing map and bp_table here makes code much more readable. I always end-up
> counting the variables when the names in the callee changes.
>
OK, I'll fix it.
> > +
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
> > + struct regmap *infracfg, struct regmap *smi_common)
> > +{
> > + int i;
> > +
> > + for (i = MAX_STEPS - 1; i >= 0; i--) {
> > + struct regmap *map = NULL;
> > + int ret;
> > +
> > + if (bp_table[i].type == INVALID_TYPE)
> > + continue;
> > + else if (bp_table[i].type == IFR_TYPE)
> > + map = infracfg;
> > + else if (bp_table[i].type == SMI_TYPE)
> > + map = smi_common;
> > +
> > + ret = clear_bus_protection(map,
> > + bp_table[i].mask, bp_table[i].clr_ofs,
> > + bp_table[i].sta_ofs, bp_table[i].en_ofs,
> > + bp_table[i].ignore_clr_ack);
> > +
>
> same here.
>
Got it.
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> > index db35a28..763ca58 100644
> > --- a/drivers/soc/mediatek/mtk-scpsys.c
> > +++ b/drivers/soc/mediatek/mtk-scpsys.c
> > @@ -11,7 +11,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/pm_domain.h>
> > #include <linux/regulator/consumer.h>
> > -#include <linux/soc/mediatek/infracfg.h>
> > +#include "scpsys-ext.h"
> >
> > #include <dt-bindings/power/mt2701-power.h>
> > #include <dt-bindings/power/mt2712-power.h>
> > @@ -87,9 +87,9 @@
> > * @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.
> > - * @bus_prot_mask: The mask for single step bus protection.
> > * @basic_clk_name: The basic clocks required by this power domain.
> > * @caps: The flag for active wake-up action.
> > + * @bp_table: The mask table for multiple step bus protection.
> > */
> > struct scp_domain_data {
> > const char *name;
> > @@ -97,9 +97,9 @@ struct scp_domain_data {
> > int ctl_offs;
> > u32 sram_pdn_bits;
> > u32 sram_pdn_ack_bits;
> > - u32 bus_prot_mask;
> > const char *basic_clk_name[MAX_CLKS];
> > u8 caps;
> > + struct bus_prot bp_table[MAX_STEPS];
> > };
> >
> > struct scp;
> > @@ -123,8 +123,8 @@ struct scp {
> > struct device *dev;
> > void __iomem *base;
> > struct regmap *infracfg;
> > + struct regmap *smi_common;
> > struct scp_ctrl_reg ctrl_reg;
> > - bool bus_prot_reg_update;
> > };
> >
> > struct scp_subdomain {
> > @@ -138,7 +138,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)
> > @@ -252,24 +251,16 @@ static int scpsys_bus_protect_enable(struct scp_domain *scpd)
> > {
> > struct scp *scp = scpd->scp;
> >
> > - if (!scpd->data->bus_prot_mask)
> > - return 0;
> > -
> > - return mtk_infracfg_set_bus_protection(scp->infracfg,
> > - scpd->data->bus_prot_mask,
> > - scp->bus_prot_reg_update);
> > + return mtk_scpsys_ext_set_bus_protection(scpd->data->bp_table,
> > + scp->infracfg, scp->smi_common);
>
> Now that's just a wrapper which does add nothing but another indirection. We can
> call mtk_scpsys_ext_set_bus_protection() directly (also I don't like the name,
> actually why don't mtk_scpsys_set_bus_protection()?)
>
You're right. I'll called mtk_scpsys_ext_set_bus_protection() directly.
> > }
> >
> > static int scpsys_bus_protect_disable(struct scp_domain *scpd)
> > {
> > struct scp *scp = scpd->scp;
> >
> > - if (!scpd->data->bus_prot_mask)
> > - return 0;
> > -
> > - return mtk_infracfg_clear_bus_protection(scp->infracfg,
> > - scpd->data->bus_prot_mask,
> > - scp->bus_prot_reg_update);
> > + return mtk_scpsys_ext_clear_bus_protection(scpd->data->bp_table,
> > + scp->infracfg, scp->smi_common);
> > }
> >
> > static int scpsys_power_on(struct generic_pm_domain *genpd)
> > @@ -404,8 +395,7 @@ static int init_basic_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;
> > @@ -419,8 +409,6 @@ 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;
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > @@ -448,6 +436,17 @@ static struct scp *init_scp(struct platform_device *pdev,
> > return ERR_CAST(scp->infracfg);
> > }
> >
> > + scp->smi_common = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> > + "smi_comm");
> > +
> > + if (scp->smi_common == ERR_PTR(-ENODEV)) {
> > + scp->smi_common = NULL;
> > + } else if (IS_ERR(scp->smi_common)) {
> > + dev_err(&pdev->dev, "Cannot find smi_common controller: %ld\n",
> > + PTR_ERR(scp->smi_common));
> > + return ERR_CAST(scp->smi_common);
> > + }
> > +
> > for (i = 0; i < num; i++) {
> > struct scp_domain *scpd = &scp->domains[i];
> > const struct scp_domain_data *data = &scp_domain_data[i];
> > @@ -530,8 +529,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .name = "conn",
> > .sta_mask = PWR_STATUS_CONN,
> > .ctl_offs = SPM_CONN_PWR_CON,
> > - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
> > - MT2701_TOP_AXI_PROT_EN_CONN_S,
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> > + MT2701_TOP_AXI_PROT_EN_CONN_M |
> > + MT2701_TOP_AXI_PROT_EN_CONN_S),
> > + },
> > .caps = MTK_SCPD_ACTIVE_WAKEUP,
> > },
> > [MT2701_POWER_DOMAIN_DISP] = {
> > @@ -540,7 +542,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .ctl_offs = SPM_DIS_PWR_CON,
> > .sram_pdn_bits = GENMASK(11, 8),
> > .basic_clk_name = {"mm"},
> > - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_MM_M0,
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> > + MT2701_TOP_AXI_PROT_EN_MM_M0),
> > + },
> > .caps = MTK_SCPD_ACTIVE_WAKEUP,
> > },
> > [MT2701_POWER_DOMAIN_MFG] = {
> > @@ -675,7 +680,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .sram_pdn_bits = GENMASK(8, 8),
> > .sram_pdn_ack_bits = GENMASK(16, 16),
> > .basic_clk_name = {"mfg"},
> > - .bus_prot_mask = BIT(14) | BIT(21) | BIT(23),
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0x260, 0x264, 0x220, 0x228,
> > + BIT(14) | BIT(21) | BIT(23)),
> > + },
> > .caps = MTK_SCPD_ACTIVE_WAKEUP,
> > },
> > [MT2712_POWER_DOMAIN_MFG_SC1] = {
> > @@ -747,7 +755,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .sram_pdn_bits = GENMASK(8, 8),
> > .sram_pdn_ack_bits = GENMASK(12, 12),
> > .basic_clk_name = {"mm"},
> > - .bus_prot_mask = (BIT(1) | BIT(2)),
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> > + BIT(1) | BIT(2)),
> > + },
> > },
> > [MT6797_POWER_DOMAIN_AUDIO] = {
> > .name = "audio",
> > @@ -794,7 +805,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .ctl_offs = SPM_ETHSYS_PWR_CON,
> > .sram_pdn_bits = GENMASK(11, 8),
> > .sram_pdn_ack_bits = GENMASK(15, 12),
> > - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_ETHSYS,
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> > + MT7622_TOP_AXI_PROT_EN_ETHSYS),
> > + },
> > .caps = MTK_SCPD_ACTIVE_WAKEUP,
> > },
> > [MT7622_POWER_DOMAIN_HIF0] = {
> > @@ -804,7 +818,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .sram_pdn_bits = GENMASK(11, 8),
> > .sram_pdn_ack_bits = GENMASK(15, 12),
> > .basic_clk_name = {"hif_sel"},
> > - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_HIF0,
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> > + MT7622_TOP_AXI_PROT_EN_HIF0),
> > + },
> > .caps = MTK_SCPD_ACTIVE_WAKEUP,
> > },
> > [MT7622_POWER_DOMAIN_HIF1] = {
> > @@ -814,7 +831,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .sram_pdn_bits = GENMASK(11, 8),
> > .sram_pdn_ack_bits = GENMASK(15, 12),
> > .basic_clk_name = {"hif_sel"},
> > - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_HIF1,
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> > + MT7622_TOP_AXI_PROT_EN_HIF1),
> > + },
> > .caps = MTK_SCPD_ACTIVE_WAKEUP,
> > },
> > [MT7622_POWER_DOMAIN_WB] = {
> > @@ -823,7 +843,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .ctl_offs = SPM_WB_PWR_CON,
> > .sram_pdn_bits = 0,
> > .sram_pdn_ack_bits = 0,
> > - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_WB,
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> > + MT7622_TOP_AXI_PROT_EN_WB),
> > + },
> > .caps = MTK_SCPD_ACTIVE_WAKEUP | MTK_SCPD_FWAIT_SRAM,
> > },
> > };
> > @@ -837,8 +860,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .name = "conn",
> > .sta_mask = PWR_STATUS_CONN,
> > .ctl_offs = SPM_CONN_PWR_CON,
> > - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
> > - MT2701_TOP_AXI_PROT_EN_CONN_S,
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> > + MT2701_TOP_AXI_PROT_EN_CONN_M |
> > + MT2701_TOP_AXI_PROT_EN_CONN_S),
> > + },
> > .caps = MTK_SCPD_ACTIVE_WAKEUP,
> > },
> > [MT7623A_POWER_DOMAIN_ETH] = {
> > @@ -903,8 +929,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .sram_pdn_bits = GENMASK(11, 8),
> > .sram_pdn_ack_bits = GENMASK(12, 12),
> > .basic_clk_name = {"mm"},
> > - .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
> > - MT8173_TOP_AXI_PROT_EN_MM_M1,
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> > + MT8173_TOP_AXI_PROT_EN_MM_M0 |
> > + MT8173_TOP_AXI_PROT_EN_MM_M1),
> > + },
> > },
> > [MT8173_POWER_DOMAIN_VENC_LT] = {
> > .name = "venc_lt",
> > @@ -950,10 +979,13 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .ctl_offs = SPM_MFG_PWR_CON,
> > .sram_pdn_bits = GENMASK(13, 8),
> > .sram_pdn_ack_bits = GENMASK(21, 16),
> > - .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
> > - MT8173_TOP_AXI_PROT_EN_MFG_M0 |
> > - MT8173_TOP_AXI_PROT_EN_MFG_M1 |
> > - MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
> > + .bp_table = {
> > + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> > + MT8173_TOP_AXI_PROT_EN_MFG_S |
> > + MT8173_TOP_AXI_PROT_EN_MFG_M0 |
> > + MT8173_TOP_AXI_PROT_EN_MFG_M1 |
> > + MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT),
> > + },
> > },
> > };
> >
> > @@ -969,7 +1001,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 = {
> > @@ -981,7 +1012,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 mt6797_data = {
> > @@ -993,7 +1023,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,
> > };
> >
> > static const struct scp_soc_data mt7622_data = {
> > @@ -1003,7 +1032,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 = {
> > @@ -1013,7 +1041,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 = {
> > @@ -1025,7 +1052,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,
> > };
> >
> > /*
> > @@ -1066,8 +1092,7 @@ 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/drivers/soc/mediatek/scpsys-ext.h b/drivers/soc/mediatek/scpsys-ext.h
> > new file mode 100644
> > index 0000000..458b2c5
> > --- /dev/null
> > +++ b/drivers/soc/mediatek/scpsys-ext.h
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __SOC_MEDIATEK_SCPSYS_EXT_H
> > +#define __SOC_MEDIATEK_SCPSYS_EXT_H
> > +
> > +#define MAX_STEPS 4
> > +
> > +#define _BUS_PROT(_type, _set_ofs, _clr_ofs, \
> > + _en_ofs, _sta_ofs, _mask, _ignore_clr_ack) { \
> > + .type = _type, \
> > + .set_ofs = _set_ofs, \
> > + .clr_ofs = _clr_ofs, \
> > + .en_ofs = _en_ofs, \
> > + .sta_ofs = _sta_ofs, \
> > + .mask = _mask, \
> > + .ignore_clr_ack = _ignore_clr_ack, \
> > + }
> > +
> > +#define BUS_PROT(_type, _set_ofs, _clr_ofs, \
> > + _en_ofs, _sta_ofs, _mask) \
> > + _BUS_PROT(_type, _set_ofs, _clr_ofs, \
> > + _en_ofs, _sta_ofs, _mask, false)
> > +
> > +#define BUS_PROT_IGN(_type, _set_ofs, _clr_ofs, \
> > + _en_ofs, _sta_ofs, _mask) \
> > + _BUS_PROT(_type, _set_ofs, _clr_ofs, \
> > + _en_ofs, _sta_ofs, _mask, true)
> > +
> > +#define MT2701_TOP_AXI_PROT_EN_MM_M0 BIT(1)
> > +#define MT2701_TOP_AXI_PROT_EN_CONN_M BIT(2)
> > +#define MT2701_TOP_AXI_PROT_EN_CONN_S BIT(8)
> > +
> > +#define MT7622_TOP_AXI_PROT_EN_ETHSYS (BIT(3) | BIT(17))
> > +#define MT7622_TOP_AXI_PROT_EN_HIF0 (BIT(24) | BIT(25))
> > +#define MT7622_TOP_AXI_PROT_EN_HIF1 (BIT(26) | BIT(27) | \
> > + BIT(28))
> > +#define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
> > + BIT(7) | BIT(8))
> > +
> > +#define MT8173_TOP_AXI_PROT_EN_MM_M0 BIT(1)
> > +#define MT8173_TOP_AXI_PROT_EN_MM_M1 BIT(2)
> > +#define MT8173_TOP_AXI_PROT_EN_MFG_S BIT(14)
> > +#define MT8173_TOP_AXI_PROT_EN_MFG_M0 BIT(21)
> > +#define MT8173_TOP_AXI_PROT_EN_MFG_M1 BIT(22)
> > +#define MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT BIT(23)
> > +
> > +enum regmap_type {
> > + INVALID_TYPE = 0,
> > + IFR_TYPE,
> > + SMI_TYPE,
> > +};
> > +
> > +struct bus_prot {
> > + enum regmap_type type;
> > + u32 set_ofs;
> > + u32 clr_ofs;
>
> Please try to keep variable names consistent. You change for example set_ofs to
> reg_set. That makes the code more difficult to read and understand.
>
OK, I'll keep those names consistent.
> > + u32 en_ofs;
> > + u32 sta_ofs;
> > + u32 mask;
> > + bool ignore_clr_ack;
> > +};
> > +
> > +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
> > + struct regmap *infracfg, struct regmap *smi_common);
> > +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
> > + struct regmap *infracfg, struct regmap *smi_common);
> > +
> > +#endif /* __SOC_MEDIATEK_SCPSYS_EXT_H */
> >
On 12/02/2020 03:55, Weiyi Lu wrote:
> On Tue, 2020-02-11 at 18:49 +0100, Matthias Brugger wrote:
>>
>> On 20/12/2019 04:45, Weiyi Lu wrote:
>>> Both MT8183 & MT6765 have more control steps of bus protection
>>> than previous project. And there add more bus protection registers
>>> reside at infracfg & smi-common. Also add new APIs for multiple
>>> step bus protection control with more customized arguments.
>>> And then use bp_table for bus protection of all compatibles,
>>> instead of mixing bus_prot_mask and bus_prot_reg_update.
>>>
>>> Signed-off-by: Weiyi Lu <[email protected]>
>>> ---
>>> drivers/soc/mediatek/Makefile | 2 +-
>>> drivers/soc/mediatek/mtk-scpsys-ext.c | 101 +++++++++++++++++++++++++++++
>>> drivers/soc/mediatek/mtk-scpsys.c | 117 +++++++++++++++++++++-------------
>>> drivers/soc/mediatek/scpsys-ext.h | 67 +++++++++++++++++++
>>> 4 files changed, 240 insertions(+), 47 deletions(-)
>>> create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
>>> create mode 100644 drivers/soc/mediatek/scpsys-ext.h
>>>
>>> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
>>> index b017330..b442be9 100644
>>> --- a/drivers/soc/mediatek/Makefile
>>> +++ b/drivers/soc/mediatek/Makefile
>>> @@ -1,5 +1,5 @@
>>> # SPDX-License-Identifier: GPL-2.0-only
>>> obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
>>> -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
>>> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
>>
>> It seems that we would need another patch which get's rid of the mtk-infracfg
>> first and then add stuff like the possibility to have different steps.
>>
>
> Actually I have a PATCH 05/11 to remove the mtk-infracfg.
> In this patch, I have some changes, like calling
> mtk_scpsys_ext_set_bus_protection(...) instead of
> mtk_infracfg_set_bus_protection(...) in scpsys_bus_protect_enable(...)
> and replacing bus_prot_mask by bp_table.
> I thought I should introduce the new method first and remove useless one
> later. What do you think?
Yes, but first patch would be a step to get rid of mtk-infracfg and a second
patch add bp_table and the like.
>
>>> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
>>> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
>>> diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
>>> new file mode 100644
>>> index 0000000..df402ac
>>> --- /dev/null
>>> +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
>>
>> I'm not quite sure why we should put this into a new file. I suppose the
>> rational behind it is the fact that we access other blocks through regmap.
>>
>
> Yes, those operation are accross infracfg and smi-common so we put these
> into new files.
Are you exepct other drivers to use this functions? If not I will have to think
again, but I don't see any reason to put it into a new file.
>
>>> @@ -0,0 +1,101 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2018 MediaTek Inc.
>>> + * Author: Owen Chen <[email protected]>
>>> + */
>>> +#include <linux/ktime.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/regmap.h>
>>> +#include "scpsys-ext.h"
>>> +
>>> +#define MTK_POLL_DELAY_US 10
>>> +#define MTK_POLL_TIMEOUT USEC_PER_SEC
>>> +
>>> +static int set_bus_protection(struct regmap *map, u32 mask,
>>> + u32 reg_set, u32 reg_sta, u32 reg_en)
>>> +{
>>> + u32 val;
>>> +
>>> + if (reg_set)
>>> + regmap_write(map, reg_set, mask);
>>> + else
>>> + regmap_update_bits(map, reg_en, mask, mask);
>>> +
>>> + return regmap_read_poll_timeout(map, reg_sta,
>>> + val, (val & mask) == mask,
>>> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> +}
>>> +
>>> +static int clear_bus_protection(struct regmap *map, u32 mask,
>>> + u32 reg_clr, u32 reg_sta, u32 reg_en, bool ignore_ack)
>>> +{
>>> + u32 val;
>>> +
>>> + if (reg_clr)
>>> + regmap_write(map, reg_clr, mask);
>>> + else
>>> + regmap_update_bits(map, reg_en, mask, 0);
>>> +
>>> + if (ignore_ack)
>>> + return 0;
>>> +
>>> + return regmap_read_poll_timeout(map, reg_sta,
>>> + val, !(val & mask),
>>> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> +}
>>> +
>>> +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
>>> + struct regmap *infracfg, struct regmap *smi_common)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < MAX_STEPS; i++) {
>>
>> struct bus_prot bp_table = bp_table[i];
>>
>
> OK, got it.
>
>>> + struct regmap *map = NULL;
>>> + int ret;
>>> +
>>> + if (bp_table[i].type == INVALID_TYPE)
>>> + break;
>>> + else if (bp_table[i].type == IFR_TYPE)
>>> + map = infracfg;
>>> + else if (bp_table[i].type == SMI_TYPE)
>>> + map = smi_common;
>>
>> if (bp_table.type == IFR_TYPE)
>> map = infracfg;
>> else if (bp_table.type == SMI_TYPE)
>> map = smi_common;
>> else
>> break;
>>
>
> OK, got it.
>
>>> +
>>> + ret = set_bus_protection(map,
>>> + bp_table[i].mask, bp_table[i].set_ofs,
>>> + bp_table[i].sta_ofs, bp_table[i].en_ofs);
>>
>> passing map and bp_table here makes code much more readable. I always end-up
>> counting the variables when the names in the callee changes.
>>
>
> OK, I'll fix it.
>
>>> +
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
>>> + struct regmap *infracfg, struct regmap *smi_common)
>>> +{
>>> + int i;
>>> +
>>> + for (i = MAX_STEPS - 1; i >= 0; i--) {
>>> + struct regmap *map = NULL;
>>> + int ret;
>>> +
>>> + if (bp_table[i].type == INVALID_TYPE)
>>> + continue;
>>> + else if (bp_table[i].type == IFR_TYPE)
>>> + map = infracfg;
>>> + else if (bp_table[i].type == SMI_TYPE)
>>> + map = smi_common;
>>> +
>>> + ret = clear_bus_protection(map,
>>> + bp_table[i].mask, bp_table[i].clr_ofs,
>>> + bp_table[i].sta_ofs, bp_table[i].en_ofs,
>>> + bp_table[i].ignore_clr_ack);
>>> +
>>
>> same here.
>>
>
> Got it.
>
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
>>> index db35a28..763ca58 100644
>>> --- a/drivers/soc/mediatek/mtk-scpsys.c
>>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
>>> @@ -11,7 +11,7 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/pm_domain.h>
>>> #include <linux/regulator/consumer.h>
>>> -#include <linux/soc/mediatek/infracfg.h>
>>> +#include "scpsys-ext.h"
>>>
>>> #include <dt-bindings/power/mt2701-power.h>
>>> #include <dt-bindings/power/mt2712-power.h>
>>> @@ -87,9 +87,9 @@
>>> * @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.
>>> - * @bus_prot_mask: The mask for single step bus protection.
>>> * @basic_clk_name: The basic clocks required by this power domain.
>>> * @caps: The flag for active wake-up action.
>>> + * @bp_table: The mask table for multiple step bus protection.
>>> */
>>> struct scp_domain_data {
>>> const char *name;
>>> @@ -97,9 +97,9 @@ struct scp_domain_data {
>>> int ctl_offs;
>>> u32 sram_pdn_bits;
>>> u32 sram_pdn_ack_bits;
>>> - u32 bus_prot_mask;
>>> const char *basic_clk_name[MAX_CLKS];
>>> u8 caps;
>>> + struct bus_prot bp_table[MAX_STEPS];
>>> };
>>>
>>> struct scp;
>>> @@ -123,8 +123,8 @@ struct scp {
>>> struct device *dev;
>>> void __iomem *base;
>>> struct regmap *infracfg;
>>> + struct regmap *smi_common;
>>> struct scp_ctrl_reg ctrl_reg;
>>> - bool bus_prot_reg_update;
>>> };
>>>
>>> struct scp_subdomain {
>>> @@ -138,7 +138,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)
>>> @@ -252,24 +251,16 @@ static int scpsys_bus_protect_enable(struct scp_domain *scpd)
>>> {
>>> struct scp *scp = scpd->scp;
>>>
>>> - if (!scpd->data->bus_prot_mask)
>>> - return 0;
>>> -
>>> - return mtk_infracfg_set_bus_protection(scp->infracfg,
>>> - scpd->data->bus_prot_mask,
>>> - scp->bus_prot_reg_update);
>>> + return mtk_scpsys_ext_set_bus_protection(scpd->data->bp_table,
>>> + scp->infracfg, scp->smi_common);
>>
>> Now that's just a wrapper which does add nothing but another indirection. We can
>> call mtk_scpsys_ext_set_bus_protection() directly (also I don't like the name,
>> actually why don't mtk_scpsys_set_bus_protection()?)
>>
>
> You're right. I'll called mtk_scpsys_ext_set_bus_protection() directly.
>
>>> }
>>>
>>> static int scpsys_bus_protect_disable(struct scp_domain *scpd)
>>> {
>>> struct scp *scp = scpd->scp;
>>>
>>> - if (!scpd->data->bus_prot_mask)
>>> - return 0;
>>> -
>>> - return mtk_infracfg_clear_bus_protection(scp->infracfg,
>>> - scpd->data->bus_prot_mask,
>>> - scp->bus_prot_reg_update);
>>> + return mtk_scpsys_ext_clear_bus_protection(scpd->data->bp_table,
>>> + scp->infracfg, scp->smi_common);
>>> }
>>>
>>> static int scpsys_power_on(struct generic_pm_domain *genpd)
>>> @@ -404,8 +395,7 @@ static int init_basic_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;
>>> @@ -419,8 +409,6 @@ 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;
>>>
>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> @@ -448,6 +436,17 @@ static struct scp *init_scp(struct platform_device *pdev,
>>> return ERR_CAST(scp->infracfg);
>>> }
>>>
>>> + scp->smi_common = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
>>> + "smi_comm");
>>> +
>>> + if (scp->smi_common == ERR_PTR(-ENODEV)) {
>>> + scp->smi_common = NULL;
>>> + } else if (IS_ERR(scp->smi_common)) {
>>> + dev_err(&pdev->dev, "Cannot find smi_common controller: %ld\n",
>>> + PTR_ERR(scp->smi_common));
>>> + return ERR_CAST(scp->smi_common);
>>> + }
>>> +
>>> for (i = 0; i < num; i++) {
>>> struct scp_domain *scpd = &scp->domains[i];
>>> const struct scp_domain_data *data = &scp_domain_data[i];
>>> @@ -530,8 +529,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .name = "conn",
>>> .sta_mask = PWR_STATUS_CONN,
>>> .ctl_offs = SPM_CONN_PWR_CON,
>>> - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
>>> - MT2701_TOP_AXI_PROT_EN_CONN_S,
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
>>> + MT2701_TOP_AXI_PROT_EN_CONN_M |
>>> + MT2701_TOP_AXI_PROT_EN_CONN_S),
>>> + },
>>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
>>> },
>>> [MT2701_POWER_DOMAIN_DISP] = {
>>> @@ -540,7 +542,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .ctl_offs = SPM_DIS_PWR_CON,
>>> .sram_pdn_bits = GENMASK(11, 8),
>>> .basic_clk_name = {"mm"},
>>> - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_MM_M0,
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
>>> + MT2701_TOP_AXI_PROT_EN_MM_M0),
>>> + },
>>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
>>> },
>>> [MT2701_POWER_DOMAIN_MFG] = {
>>> @@ -675,7 +680,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .sram_pdn_bits = GENMASK(8, 8),
>>> .sram_pdn_ack_bits = GENMASK(16, 16),
>>> .basic_clk_name = {"mfg"},
>>> - .bus_prot_mask = BIT(14) | BIT(21) | BIT(23),
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0x260, 0x264, 0x220, 0x228,
>>> + BIT(14) | BIT(21) | BIT(23)),
>>> + },
>>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
>>> },
>>> [MT2712_POWER_DOMAIN_MFG_SC1] = {
>>> @@ -747,7 +755,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .sram_pdn_bits = GENMASK(8, 8),
>>> .sram_pdn_ack_bits = GENMASK(12, 12),
>>> .basic_clk_name = {"mm"},
>>> - .bus_prot_mask = (BIT(1) | BIT(2)),
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
>>> + BIT(1) | BIT(2)),
>>> + },
>>> },
>>> [MT6797_POWER_DOMAIN_AUDIO] = {
>>> .name = "audio",
>>> @@ -794,7 +805,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .ctl_offs = SPM_ETHSYS_PWR_CON,
>>> .sram_pdn_bits = GENMASK(11, 8),
>>> .sram_pdn_ack_bits = GENMASK(15, 12),
>>> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_ETHSYS,
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
>>> + MT7622_TOP_AXI_PROT_EN_ETHSYS),
>>> + },
>>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
>>> },
>>> [MT7622_POWER_DOMAIN_HIF0] = {
>>> @@ -804,7 +818,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .sram_pdn_bits = GENMASK(11, 8),
>>> .sram_pdn_ack_bits = GENMASK(15, 12),
>>> .basic_clk_name = {"hif_sel"},
>>> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_HIF0,
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
>>> + MT7622_TOP_AXI_PROT_EN_HIF0),
>>> + },
>>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
>>> },
>>> [MT7622_POWER_DOMAIN_HIF1] = {
>>> @@ -814,7 +831,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .sram_pdn_bits = GENMASK(11, 8),
>>> .sram_pdn_ack_bits = GENMASK(15, 12),
>>> .basic_clk_name = {"hif_sel"},
>>> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_HIF1,
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
>>> + MT7622_TOP_AXI_PROT_EN_HIF1),
>>> + },
>>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
>>> },
>>> [MT7622_POWER_DOMAIN_WB] = {
>>> @@ -823,7 +843,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .ctl_offs = SPM_WB_PWR_CON,
>>> .sram_pdn_bits = 0,
>>> .sram_pdn_ack_bits = 0,
>>> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_WB,
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
>>> + MT7622_TOP_AXI_PROT_EN_WB),
>>> + },
>>> .caps = MTK_SCPD_ACTIVE_WAKEUP | MTK_SCPD_FWAIT_SRAM,
>>> },
>>> };
>>> @@ -837,8 +860,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .name = "conn",
>>> .sta_mask = PWR_STATUS_CONN,
>>> .ctl_offs = SPM_CONN_PWR_CON,
>>> - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
>>> - MT2701_TOP_AXI_PROT_EN_CONN_S,
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
>>> + MT2701_TOP_AXI_PROT_EN_CONN_M |
>>> + MT2701_TOP_AXI_PROT_EN_CONN_S),
>>> + },
>>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
>>> },
>>> [MT7623A_POWER_DOMAIN_ETH] = {
>>> @@ -903,8 +929,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .sram_pdn_bits = GENMASK(11, 8),
>>> .sram_pdn_ack_bits = GENMASK(12, 12),
>>> .basic_clk_name = {"mm"},
>>> - .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
>>> - MT8173_TOP_AXI_PROT_EN_MM_M1,
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
>>> + MT8173_TOP_AXI_PROT_EN_MM_M0 |
>>> + MT8173_TOP_AXI_PROT_EN_MM_M1),
>>> + },
>>> },
>>> [MT8173_POWER_DOMAIN_VENC_LT] = {
>>> .name = "venc_lt",
>>> @@ -950,10 +979,13 @@ static void mtk_register_power_domains(struct platform_device *pdev,
>>> .ctl_offs = SPM_MFG_PWR_CON,
>>> .sram_pdn_bits = GENMASK(13, 8),
>>> .sram_pdn_ack_bits = GENMASK(21, 16),
>>> - .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
>>> - MT8173_TOP_AXI_PROT_EN_MFG_M0 |
>>> - MT8173_TOP_AXI_PROT_EN_MFG_M1 |
>>> - MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
>>> + .bp_table = {
>>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
>>> + MT8173_TOP_AXI_PROT_EN_MFG_S |
>>> + MT8173_TOP_AXI_PROT_EN_MFG_M0 |
>>> + MT8173_TOP_AXI_PROT_EN_MFG_M1 |
>>> + MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT),
>>> + },
>>> },
>>> };
>>>
>>> @@ -969,7 +1001,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 = {
>>> @@ -981,7 +1012,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 mt6797_data = {
>>> @@ -993,7 +1023,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,
>>> };
>>>
>>> static const struct scp_soc_data mt7622_data = {
>>> @@ -1003,7 +1032,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 = {
>>> @@ -1013,7 +1041,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 = {
>>> @@ -1025,7 +1052,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,
>>> };
>>>
>>> /*
>>> @@ -1066,8 +1092,7 @@ 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/drivers/soc/mediatek/scpsys-ext.h b/drivers/soc/mediatek/scpsys-ext.h
>>> new file mode 100644
>>> index 0000000..458b2c5
>>> --- /dev/null
>>> +++ b/drivers/soc/mediatek/scpsys-ext.h
>>> @@ -0,0 +1,67 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef __SOC_MEDIATEK_SCPSYS_EXT_H
>>> +#define __SOC_MEDIATEK_SCPSYS_EXT_H
>>> +
>>> +#define MAX_STEPS 4
>>> +
>>> +#define _BUS_PROT(_type, _set_ofs, _clr_ofs, \
>>> + _en_ofs, _sta_ofs, _mask, _ignore_clr_ack) { \
>>> + .type = _type, \
>>> + .set_ofs = _set_ofs, \
>>> + .clr_ofs = _clr_ofs, \
>>> + .en_ofs = _en_ofs, \
>>> + .sta_ofs = _sta_ofs, \
>>> + .mask = _mask, \
>>> + .ignore_clr_ack = _ignore_clr_ack, \
>>> + }
>>> +
>>> +#define BUS_PROT(_type, _set_ofs, _clr_ofs, \
>>> + _en_ofs, _sta_ofs, _mask) \
>>> + _BUS_PROT(_type, _set_ofs, _clr_ofs, \
>>> + _en_ofs, _sta_ofs, _mask, false)
>>> +
>>> +#define BUS_PROT_IGN(_type, _set_ofs, _clr_ofs, \
>>> + _en_ofs, _sta_ofs, _mask) \
>>> + _BUS_PROT(_type, _set_ofs, _clr_ofs, \
>>> + _en_ofs, _sta_ofs, _mask, true)
>>> +
>>> +#define MT2701_TOP_AXI_PROT_EN_MM_M0 BIT(1)
>>> +#define MT2701_TOP_AXI_PROT_EN_CONN_M BIT(2)
>>> +#define MT2701_TOP_AXI_PROT_EN_CONN_S BIT(8)
>>> +
>>> +#define MT7622_TOP_AXI_PROT_EN_ETHSYS (BIT(3) | BIT(17))
>>> +#define MT7622_TOP_AXI_PROT_EN_HIF0 (BIT(24) | BIT(25))
>>> +#define MT7622_TOP_AXI_PROT_EN_HIF1 (BIT(26) | BIT(27) | \
>>> + BIT(28))
>>> +#define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
>>> + BIT(7) | BIT(8))
>>> +
>>> +#define MT8173_TOP_AXI_PROT_EN_MM_M0 BIT(1)
>>> +#define MT8173_TOP_AXI_PROT_EN_MM_M1 BIT(2)
>>> +#define MT8173_TOP_AXI_PROT_EN_MFG_S BIT(14)
>>> +#define MT8173_TOP_AXI_PROT_EN_MFG_M0 BIT(21)
>>> +#define MT8173_TOP_AXI_PROT_EN_MFG_M1 BIT(22)
>>> +#define MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT BIT(23)
>>> +
>>> +enum regmap_type {
>>> + INVALID_TYPE = 0,
>>> + IFR_TYPE,
>>> + SMI_TYPE,
>>> +};
>>> +
>>> +struct bus_prot {
>>> + enum regmap_type type;
>>> + u32 set_ofs;
>>> + u32 clr_ofs;
>>
>> Please try to keep variable names consistent. You change for example set_ofs to
>> reg_set. That makes the code more difficult to read and understand.
>>
>
> OK, I'll keep those names consistent.
>
>>> + u32 en_ofs;
>>> + u32 sta_ofs;
>>> + u32 mask;
>>> + bool ignore_clr_ack;
>>> +};
>>> +
>>> +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
>>> + struct regmap *infracfg, struct regmap *smi_common);
>>> +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
>>> + struct regmap *infracfg, struct regmap *smi_common);
>>> +
>>> +#endif /* __SOC_MEDIATEK_SCPSYS_EXT_H */
>>>
>
On Wed, 2020-02-12 at 10:23 +0100, Matthias Brugger wrote:
>
> On 12/02/2020 03:55, Weiyi Lu wrote:
> > On Tue, 2020-02-11 at 18:49 +0100, Matthias Brugger wrote:
> >>
> >> On 20/12/2019 04:45, Weiyi Lu wrote:
> >>> Both MT8183 & MT6765 have more control steps of bus protection
> >>> than previous project. And there add more bus protection registers
> >>> reside at infracfg & smi-common. Also add new APIs for multiple
> >>> step bus protection control with more customized arguments.
> >>> And then use bp_table for bus protection of all compatibles,
> >>> instead of mixing bus_prot_mask and bus_prot_reg_update.
> >>>
> >>> Signed-off-by: Weiyi Lu <[email protected]>
> >>> ---
> >>> drivers/soc/mediatek/Makefile | 2 +-
> >>> drivers/soc/mediatek/mtk-scpsys-ext.c | 101 +++++++++++++++++++++++++++++
> >>> drivers/soc/mediatek/mtk-scpsys.c | 117 +++++++++++++++++++++-------------
> >>> drivers/soc/mediatek/scpsys-ext.h | 67 +++++++++++++++++++
> >>> 4 files changed, 240 insertions(+), 47 deletions(-)
> >>> create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> >>> create mode 100644 drivers/soc/mediatek/scpsys-ext.h
> >>>
> >>> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> >>> index b017330..b442be9 100644
> >>> --- a/drivers/soc/mediatek/Makefile
> >>> +++ b/drivers/soc/mediatek/Makefile
> >>> @@ -1,5 +1,5 @@
> >>> # SPDX-License-Identifier: GPL-2.0-only
> >>> obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> >>> -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> >>> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
> >>
> >> It seems that we would need another patch which get's rid of the mtk-infracfg
> >> first and then add stuff like the possibility to have different steps.
> >>
> >
> > Actually I have a PATCH 05/11 to remove the mtk-infracfg.
> > In this patch, I have some changes, like calling
> > mtk_scpsys_ext_set_bus_protection(...) instead of
> > mtk_infracfg_set_bus_protection(...) in scpsys_bus_protect_enable(...)
> > and replacing bus_prot_mask by bp_table.
> > I thought I should introduce the new method first and remove useless one
> > later. What do you think?
>
> Yes, but first patch would be a step to get rid of mtk-infracfg and a second
> patch add bp_table and the like.
>
OK, I'll split into such sequence.
> >
> >>> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> >>> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> >>> diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> >>> new file mode 100644
> >>> index 0000000..df402ac
> >>> --- /dev/null
> >>> +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
> >>
> >> I'm not quite sure why we should put this into a new file. I suppose the
> >> rational behind it is the fact that we access other blocks through regmap.
> >>
> >
> > Yes, those operation are accross infracfg and smi-common so we put these
> > into new files.
>
> Are you exepct other drivers to use this functions? If not I will have to think
> again, but I don't see any reason to put it into a new file.
>
I thought no other driver would use these functions unless there will
have drivers want to take over the bus protection process by themselves.
Do you prefer just putting these functions into mtk-scpsys.c?
> >
> >>> @@ -0,0 +1,101 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Copyright (c) 2018 MediaTek Inc.
> >>> + * Author: Owen Chen <[email protected]>
> >>> + */
> >>> +#include <linux/ktime.h>
> >>> +#include <linux/mfd/syscon.h>
> >>> +#include <linux/of_device.h>
> >>> +#include <linux/regmap.h>
> >>> +#include "scpsys-ext.h"
> >>> +
> >>> +#define MTK_POLL_DELAY_US 10
> >>> +#define MTK_POLL_TIMEOUT USEC_PER_SEC
> >>> +
> >>> +static int set_bus_protection(struct regmap *map, u32 mask,
> >>> + u32 reg_set, u32 reg_sta, u32 reg_en)
> >>> +{
> >>> + u32 val;
> >>> +
> >>> + if (reg_set)
> >>> + regmap_write(map, reg_set, mask);
> >>> + else
> >>> + regmap_update_bits(map, reg_en, mask, mask);
> >>> +
> >>> + return regmap_read_poll_timeout(map, reg_sta,
> >>> + val, (val & mask) == mask,
> >>> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> >>> +}
> >>> +
> >>> +static int clear_bus_protection(struct regmap *map, u32 mask,
> >>> + u32 reg_clr, u32 reg_sta, u32 reg_en, bool ignore_ack)
> >>> +{
> >>> + u32 val;
> >>> +
> >>> + if (reg_clr)
> >>> + regmap_write(map, reg_clr, mask);
> >>> + else
> >>> + regmap_update_bits(map, reg_en, mask, 0);
> >>> +
> >>> + if (ignore_ack)
> >>> + return 0;
> >>> +
> >>> + return regmap_read_poll_timeout(map, reg_sta,
> >>> + val, !(val & mask),
> >>> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> >>> +}
> >>> +
> >>> +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
> >>> + struct regmap *infracfg, struct regmap *smi_common)
> >>> +{
> >>> + int i;
> >>> +
> >>> + for (i = 0; i < MAX_STEPS; i++) {
> >>
> >> struct bus_prot bp_table = bp_table[i];
> >>
> >
> > OK, got it.
> >
> >>> + struct regmap *map = NULL;
> >>> + int ret;
> >>> +
> >>> + if (bp_table[i].type == INVALID_TYPE)
> >>> + break;
> >>> + else if (bp_table[i].type == IFR_TYPE)
> >>> + map = infracfg;
> >>> + else if (bp_table[i].type == SMI_TYPE)
> >>> + map = smi_common;
> >>
> >> if (bp_table.type == IFR_TYPE)
> >> map = infracfg;
> >> else if (bp_table.type == SMI_TYPE)
> >> map = smi_common;
> >> else
> >> break;
> >>
> >
> > OK, got it.
> >
> >>> +
> >>> + ret = set_bus_protection(map,
> >>> + bp_table[i].mask, bp_table[i].set_ofs,
> >>> + bp_table[i].sta_ofs, bp_table[i].en_ofs);
> >>
> >> passing map and bp_table here makes code much more readable. I always end-up
> >> counting the variables when the names in the callee changes.
> >>
> >
> > OK, I'll fix it.
> >
> >>> +
> >>> + if (ret)
> >>> + return ret;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
> >>> + struct regmap *infracfg, struct regmap *smi_common)
> >>> +{
> >>> + int i;
> >>> +
> >>> + for (i = MAX_STEPS - 1; i >= 0; i--) {
> >>> + struct regmap *map = NULL;
> >>> + int ret;
> >>> +
> >>> + if (bp_table[i].type == INVALID_TYPE)
> >>> + continue;
> >>> + else if (bp_table[i].type == IFR_TYPE)
> >>> + map = infracfg;
> >>> + else if (bp_table[i].type == SMI_TYPE)
> >>> + map = smi_common;
> >>> +
> >>> + ret = clear_bus_protection(map,
> >>> + bp_table[i].mask, bp_table[i].clr_ofs,
> >>> + bp_table[i].sta_ofs, bp_table[i].en_ofs,
> >>> + bp_table[i].ignore_clr_ack);
> >>> +
> >>
> >> same here.
> >>
> >
> > Got it.
> >
> >>> + if (ret)
> >>> + return ret;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> >>> index db35a28..763ca58 100644
> >>> --- a/drivers/soc/mediatek/mtk-scpsys.c
> >>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> >>> @@ -11,7 +11,7 @@
> >>> #include <linux/platform_device.h>
> >>> #include <linux/pm_domain.h>
> >>> #include <linux/regulator/consumer.h>
> >>> -#include <linux/soc/mediatek/infracfg.h>
> >>> +#include "scpsys-ext.h"
> >>>
> >>> #include <dt-bindings/power/mt2701-power.h>
> >>> #include <dt-bindings/power/mt2712-power.h>
> >>> @@ -87,9 +87,9 @@
> >>> * @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.
> >>> - * @bus_prot_mask: The mask for single step bus protection.
> >>> * @basic_clk_name: The basic clocks required by this power domain.
> >>> * @caps: The flag for active wake-up action.
> >>> + * @bp_table: The mask table for multiple step bus protection.
> >>> */
> >>> struct scp_domain_data {
> >>> const char *name;
> >>> @@ -97,9 +97,9 @@ struct scp_domain_data {
> >>> int ctl_offs;
> >>> u32 sram_pdn_bits;
> >>> u32 sram_pdn_ack_bits;
> >>> - u32 bus_prot_mask;
> >>> const char *basic_clk_name[MAX_CLKS];
> >>> u8 caps;
> >>> + struct bus_prot bp_table[MAX_STEPS];
> >>> };
> >>>
> >>> struct scp;
> >>> @@ -123,8 +123,8 @@ struct scp {
> >>> struct device *dev;
> >>> void __iomem *base;
> >>> struct regmap *infracfg;
> >>> + struct regmap *smi_common;
> >>> struct scp_ctrl_reg ctrl_reg;
> >>> - bool bus_prot_reg_update;
> >>> };
> >>>
> >>> struct scp_subdomain {
> >>> @@ -138,7 +138,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)
> >>> @@ -252,24 +251,16 @@ static int scpsys_bus_protect_enable(struct scp_domain *scpd)
> >>> {
> >>> struct scp *scp = scpd->scp;
> >>>
> >>> - if (!scpd->data->bus_prot_mask)
> >>> - return 0;
> >>> -
> >>> - return mtk_infracfg_set_bus_protection(scp->infracfg,
> >>> - scpd->data->bus_prot_mask,
> >>> - scp->bus_prot_reg_update);
> >>> + return mtk_scpsys_ext_set_bus_protection(scpd->data->bp_table,
> >>> + scp->infracfg, scp->smi_common);
> >>
> >> Now that's just a wrapper which does add nothing but another indirection. We can
> >> call mtk_scpsys_ext_set_bus_protection() directly (also I don't like the name,
> >> actually why don't mtk_scpsys_set_bus_protection()?)
> >>
> >
> > You're right. I'll called mtk_scpsys_ext_set_bus_protection() directly.
> >
> >>> }
> >>>
> >>> static int scpsys_bus_protect_disable(struct scp_domain *scpd)
> >>> {
> >>> struct scp *scp = scpd->scp;
> >>>
> >>> - if (!scpd->data->bus_prot_mask)
> >>> - return 0;
> >>> -
> >>> - return mtk_infracfg_clear_bus_protection(scp->infracfg,
> >>> - scpd->data->bus_prot_mask,
> >>> - scp->bus_prot_reg_update);
> >>> + return mtk_scpsys_ext_clear_bus_protection(scpd->data->bp_table,
> >>> + scp->infracfg, scp->smi_common);
> >>> }
> >>>
> >>> static int scpsys_power_on(struct generic_pm_domain *genpd)
> >>> @@ -404,8 +395,7 @@ static int init_basic_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;
> >>> @@ -419,8 +409,6 @@ 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;
> >>>
> >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>> @@ -448,6 +436,17 @@ static struct scp *init_scp(struct platform_device *pdev,
> >>> return ERR_CAST(scp->infracfg);
> >>> }
> >>>
> >>> + scp->smi_common = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> >>> + "smi_comm");
> >>> +
> >>> + if (scp->smi_common == ERR_PTR(-ENODEV)) {
> >>> + scp->smi_common = NULL;
> >>> + } else if (IS_ERR(scp->smi_common)) {
> >>> + dev_err(&pdev->dev, "Cannot find smi_common controller: %ld\n",
> >>> + PTR_ERR(scp->smi_common));
> >>> + return ERR_CAST(scp->smi_common);
> >>> + }
> >>> +
> >>> for (i = 0; i < num; i++) {
> >>> struct scp_domain *scpd = &scp->domains[i];
> >>> const struct scp_domain_data *data = &scp_domain_data[i];
> >>> @@ -530,8 +529,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .name = "conn",
> >>> .sta_mask = PWR_STATUS_CONN,
> >>> .ctl_offs = SPM_CONN_PWR_CON,
> >>> - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
> >>> - MT2701_TOP_AXI_PROT_EN_CONN_S,
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> >>> + MT2701_TOP_AXI_PROT_EN_CONN_M |
> >>> + MT2701_TOP_AXI_PROT_EN_CONN_S),
> >>> + },
> >>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> >>> },
> >>> [MT2701_POWER_DOMAIN_DISP] = {
> >>> @@ -540,7 +542,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .ctl_offs = SPM_DIS_PWR_CON,
> >>> .sram_pdn_bits = GENMASK(11, 8),
> >>> .basic_clk_name = {"mm"},
> >>> - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_MM_M0,
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> >>> + MT2701_TOP_AXI_PROT_EN_MM_M0),
> >>> + },
> >>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> >>> },
> >>> [MT2701_POWER_DOMAIN_MFG] = {
> >>> @@ -675,7 +680,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .sram_pdn_bits = GENMASK(8, 8),
> >>> .sram_pdn_ack_bits = GENMASK(16, 16),
> >>> .basic_clk_name = {"mfg"},
> >>> - .bus_prot_mask = BIT(14) | BIT(21) | BIT(23),
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0x260, 0x264, 0x220, 0x228,
> >>> + BIT(14) | BIT(21) | BIT(23)),
> >>> + },
> >>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> >>> },
> >>> [MT2712_POWER_DOMAIN_MFG_SC1] = {
> >>> @@ -747,7 +755,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .sram_pdn_bits = GENMASK(8, 8),
> >>> .sram_pdn_ack_bits = GENMASK(12, 12),
> >>> .basic_clk_name = {"mm"},
> >>> - .bus_prot_mask = (BIT(1) | BIT(2)),
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> >>> + BIT(1) | BIT(2)),
> >>> + },
> >>> },
> >>> [MT6797_POWER_DOMAIN_AUDIO] = {
> >>> .name = "audio",
> >>> @@ -794,7 +805,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .ctl_offs = SPM_ETHSYS_PWR_CON,
> >>> .sram_pdn_bits = GENMASK(11, 8),
> >>> .sram_pdn_ack_bits = GENMASK(15, 12),
> >>> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_ETHSYS,
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> >>> + MT7622_TOP_AXI_PROT_EN_ETHSYS),
> >>> + },
> >>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> >>> },
> >>> [MT7622_POWER_DOMAIN_HIF0] = {
> >>> @@ -804,7 +818,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .sram_pdn_bits = GENMASK(11, 8),
> >>> .sram_pdn_ack_bits = GENMASK(15, 12),
> >>> .basic_clk_name = {"hif_sel"},
> >>> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_HIF0,
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> >>> + MT7622_TOP_AXI_PROT_EN_HIF0),
> >>> + },
> >>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> >>> },
> >>> [MT7622_POWER_DOMAIN_HIF1] = {
> >>> @@ -814,7 +831,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .sram_pdn_bits = GENMASK(11, 8),
> >>> .sram_pdn_ack_bits = GENMASK(15, 12),
> >>> .basic_clk_name = {"hif_sel"},
> >>> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_HIF1,
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> >>> + MT7622_TOP_AXI_PROT_EN_HIF1),
> >>> + },
> >>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> >>> },
> >>> [MT7622_POWER_DOMAIN_WB] = {
> >>> @@ -823,7 +843,10 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .ctl_offs = SPM_WB_PWR_CON,
> >>> .sram_pdn_bits = 0,
> >>> .sram_pdn_ack_bits = 0,
> >>> - .bus_prot_mask = MT7622_TOP_AXI_PROT_EN_WB,
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> >>> + MT7622_TOP_AXI_PROT_EN_WB),
> >>> + },
> >>> .caps = MTK_SCPD_ACTIVE_WAKEUP | MTK_SCPD_FWAIT_SRAM,
> >>> },
> >>> };
> >>> @@ -837,8 +860,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .name = "conn",
> >>> .sta_mask = PWR_STATUS_CONN,
> >>> .ctl_offs = SPM_CONN_PWR_CON,
> >>> - .bus_prot_mask = MT2701_TOP_AXI_PROT_EN_CONN_M |
> >>> - MT2701_TOP_AXI_PROT_EN_CONN_S,
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> >>> + MT2701_TOP_AXI_PROT_EN_CONN_M |
> >>> + MT2701_TOP_AXI_PROT_EN_CONN_S),
> >>> + },
> >>> .caps = MTK_SCPD_ACTIVE_WAKEUP,
> >>> },
> >>> [MT7623A_POWER_DOMAIN_ETH] = {
> >>> @@ -903,8 +929,11 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .sram_pdn_bits = GENMASK(11, 8),
> >>> .sram_pdn_ack_bits = GENMASK(12, 12),
> >>> .basic_clk_name = {"mm"},
> >>> - .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MM_M0 |
> >>> - MT8173_TOP_AXI_PROT_EN_MM_M1,
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> >>> + MT8173_TOP_AXI_PROT_EN_MM_M0 |
> >>> + MT8173_TOP_AXI_PROT_EN_MM_M1),
> >>> + },
> >>> },
> >>> [MT8173_POWER_DOMAIN_VENC_LT] = {
> >>> .name = "venc_lt",
> >>> @@ -950,10 +979,13 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> >>> .ctl_offs = SPM_MFG_PWR_CON,
> >>> .sram_pdn_bits = GENMASK(13, 8),
> >>> .sram_pdn_ack_bits = GENMASK(21, 16),
> >>> - .bus_prot_mask = MT8173_TOP_AXI_PROT_EN_MFG_S |
> >>> - MT8173_TOP_AXI_PROT_EN_MFG_M0 |
> >>> - MT8173_TOP_AXI_PROT_EN_MFG_M1 |
> >>> - MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT,
> >>> + .bp_table = {
> >>> + BUS_PROT(IFR_TYPE, 0, 0, 0x220, 0x228,
> >>> + MT8173_TOP_AXI_PROT_EN_MFG_S |
> >>> + MT8173_TOP_AXI_PROT_EN_MFG_M0 |
> >>> + MT8173_TOP_AXI_PROT_EN_MFG_M1 |
> >>> + MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT),
> >>> + },
> >>> },
> >>> };
> >>>
> >>> @@ -969,7 +1001,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 = {
> >>> @@ -981,7 +1012,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 mt6797_data = {
> >>> @@ -993,7 +1023,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,
> >>> };
> >>>
> >>> static const struct scp_soc_data mt7622_data = {
> >>> @@ -1003,7 +1032,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 = {
> >>> @@ -1013,7 +1041,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 = {
> >>> @@ -1025,7 +1052,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,
> >>> };
> >>>
> >>> /*
> >>> @@ -1066,8 +1092,7 @@ 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/drivers/soc/mediatek/scpsys-ext.h b/drivers/soc/mediatek/scpsys-ext.h
> >>> new file mode 100644
> >>> index 0000000..458b2c5
> >>> --- /dev/null
> >>> +++ b/drivers/soc/mediatek/scpsys-ext.h
> >>> @@ -0,0 +1,67 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>> +#ifndef __SOC_MEDIATEK_SCPSYS_EXT_H
> >>> +#define __SOC_MEDIATEK_SCPSYS_EXT_H
> >>> +
> >>> +#define MAX_STEPS 4
> >>> +
> >>> +#define _BUS_PROT(_type, _set_ofs, _clr_ofs, \
> >>> + _en_ofs, _sta_ofs, _mask, _ignore_clr_ack) { \
> >>> + .type = _type, \
> >>> + .set_ofs = _set_ofs, \
> >>> + .clr_ofs = _clr_ofs, \
> >>> + .en_ofs = _en_ofs, \
> >>> + .sta_ofs = _sta_ofs, \
> >>> + .mask = _mask, \
> >>> + .ignore_clr_ack = _ignore_clr_ack, \
> >>> + }
> >>> +
> >>> +#define BUS_PROT(_type, _set_ofs, _clr_ofs, \
> >>> + _en_ofs, _sta_ofs, _mask) \
> >>> + _BUS_PROT(_type, _set_ofs, _clr_ofs, \
> >>> + _en_ofs, _sta_ofs, _mask, false)
> >>> +
> >>> +#define BUS_PROT_IGN(_type, _set_ofs, _clr_ofs, \
> >>> + _en_ofs, _sta_ofs, _mask) \
> >>> + _BUS_PROT(_type, _set_ofs, _clr_ofs, \
> >>> + _en_ofs, _sta_ofs, _mask, true)
> >>> +
> >>> +#define MT2701_TOP_AXI_PROT_EN_MM_M0 BIT(1)
> >>> +#define MT2701_TOP_AXI_PROT_EN_CONN_M BIT(2)
> >>> +#define MT2701_TOP_AXI_PROT_EN_CONN_S BIT(8)
> >>> +
> >>> +#define MT7622_TOP_AXI_PROT_EN_ETHSYS (BIT(3) | BIT(17))
> >>> +#define MT7622_TOP_AXI_PROT_EN_HIF0 (BIT(24) | BIT(25))
> >>> +#define MT7622_TOP_AXI_PROT_EN_HIF1 (BIT(26) | BIT(27) | \
> >>> + BIT(28))
> >>> +#define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
> >>> + BIT(7) | BIT(8))
> >>> +
> >>> +#define MT8173_TOP_AXI_PROT_EN_MM_M0 BIT(1)
> >>> +#define MT8173_TOP_AXI_PROT_EN_MM_M1 BIT(2)
> >>> +#define MT8173_TOP_AXI_PROT_EN_MFG_S BIT(14)
> >>> +#define MT8173_TOP_AXI_PROT_EN_MFG_M0 BIT(21)
> >>> +#define MT8173_TOP_AXI_PROT_EN_MFG_M1 BIT(22)
> >>> +#define MT8173_TOP_AXI_PROT_EN_MFG_SNOOP_OUT BIT(23)
> >>> +
> >>> +enum regmap_type {
> >>> + INVALID_TYPE = 0,
> >>> + IFR_TYPE,
> >>> + SMI_TYPE,
> >>> +};
> >>> +
> >>> +struct bus_prot {
> >>> + enum regmap_type type;
> >>> + u32 set_ofs;
> >>> + u32 clr_ofs;
> >>
> >> Please try to keep variable names consistent. You change for example set_ofs to
> >> reg_set. That makes the code more difficult to read and understand.
> >>
> >
> > OK, I'll keep those names consistent.
> >
> >>> + u32 en_ofs;
> >>> + u32 sta_ofs;
> >>> + u32 mask;
> >>> + bool ignore_clr_ack;
> >>> +};
> >>> +
> >>> +int mtk_scpsys_ext_set_bus_protection(const struct bus_prot *bp_table,
> >>> + struct regmap *infracfg, struct regmap *smi_common);
> >>> +int mtk_scpsys_ext_clear_bus_protection(const struct bus_prot *bp_table,
> >>> + struct regmap *infracfg, struct regmap *smi_common);
> >>> +
> >>> +#endif /* __SOC_MEDIATEK_SCPSYS_EXT_H */
> >>>
> >
On 13/02/2020 03:15, Weiyi Lu wrote:
> On Wed, 2020-02-12 at 10:23 +0100, Matthias Brugger wrote:
>>
>> On 12/02/2020 03:55, Weiyi Lu wrote:
>>> On Tue, 2020-02-11 at 18:49 +0100, Matthias Brugger wrote:
>>>>
>>>> On 20/12/2019 04:45, Weiyi Lu wrote:
>>>>> Both MT8183 & MT6765 have more control steps of bus protection
>>>>> than previous project. And there add more bus protection registers
>>>>> reside at infracfg & smi-common. Also add new APIs for multiple
>>>>> step bus protection control with more customized arguments.
>>>>> And then use bp_table for bus protection of all compatibles,
>>>>> instead of mixing bus_prot_mask and bus_prot_reg_update.
>>>>>
>>>>> Signed-off-by: Weiyi Lu <[email protected]>
>>>>> ---
>>>>> drivers/soc/mediatek/Makefile | 2 +-
>>>>> drivers/soc/mediatek/mtk-scpsys-ext.c | 101 +++++++++++++++++++++++++++++
>>>>> drivers/soc/mediatek/mtk-scpsys.c | 117 +++++++++++++++++++++-------------
>>>>> drivers/soc/mediatek/scpsys-ext.h | 67 +++++++++++++++++++
>>>>> 4 files changed, 240 insertions(+), 47 deletions(-)
>>>>> create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
>>>>> create mode 100644 drivers/soc/mediatek/scpsys-ext.h
>>>>>
>>>>> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
>>>>> index b017330..b442be9 100644
>>>>> --- a/drivers/soc/mediatek/Makefile
>>>>> +++ b/drivers/soc/mediatek/Makefile
>>>>> @@ -1,5 +1,5 @@
>>>>> # SPDX-License-Identifier: GPL-2.0-only
>>>>> obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
>>>>> -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
>>>>> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
>>>>
>>>> It seems that we would need another patch which get's rid of the mtk-infracfg
>>>> first and then add stuff like the possibility to have different steps.
>>>>
>>>
>>> Actually I have a PATCH 05/11 to remove the mtk-infracfg.
>>> In this patch, I have some changes, like calling
>>> mtk_scpsys_ext_set_bus_protection(...) instead of
>>> mtk_infracfg_set_bus_protection(...) in scpsys_bus_protect_enable(...)
>>> and replacing bus_prot_mask by bp_table.
>>> I thought I should introduce the new method first and remove useless one
>>> later. What do you think?
>>
>> Yes, but first patch would be a step to get rid of mtk-infracfg and a second
>> patch add bp_table and the like.
>>
>
> OK, I'll split into such sequence.
>
>>>
>>>>> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
>>>>> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
>>>>> diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
>>>>> new file mode 100644
>>>>> index 0000000..df402ac
>>>>> --- /dev/null
>>>>> +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
>>>>
>>>> I'm not quite sure why we should put this into a new file. I suppose the
>>>> rational behind it is the fact that we access other blocks through regmap.
>>>>
>>>
>>> Yes, those operation are accross infracfg and smi-common so we put these
>>> into new files.
>>
>> Are you exepct other drivers to use this functions? If not I will have to think
>> again, but I don't see any reason to put it into a new file.
>>
>
> I thought no other driver would use these functions unless there will
> have drivers want to take over the bus protection process by themselves.
>
> Do you prefer just putting these functions into mtk-scpsys.c?
>
Yes I'd prefer that.
Regards,
Matthias