From: Alexandre Bailon <[email protected]>
This updates the power domain to support WAY_EN operations. These
operations enable a path between different units of the chip and are
labeled as 'way_en' in the register descriptions.
This operation is required by the mt8365 for the MM power domain.
Signed-off-by: Alexandre Bailon <[email protected]>
Signed-off-by: Fabien Parent <[email protected]>
Signed-off-by: Markus Schneider-Pargmann <[email protected]>
---
Notes:
Changes in v3:
- Separated the way_en functions for clarity
- Added some checks for infracfg_nao
Changes in v2:
- some minor style fixes.
- Renamed 'wayen' to 'way_en' to clarify the meaning
- Updated commit message
drivers/soc/mediatek/mtk-pm-domains.c | 162 +++++++++++++++++++++-----
drivers/soc/mediatek/mtk-pm-domains.h | 28 +++--
2 files changed, 149 insertions(+), 41 deletions(-)
diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
index 9734f1091c69..c2cbe0de6aa1 100644
--- a/drivers/soc/mediatek/mtk-pm-domains.c
+++ b/drivers/soc/mediatek/mtk-pm-domains.c
@@ -43,6 +43,7 @@ struct scpsys_domain {
struct clk_bulk_data *clks;
int num_subsys_clks;
struct clk_bulk_data *subsys_clks;
+ struct regmap *infracfg_nao;
struct regmap *infracfg;
struct regmap *smi;
struct regulator *supply;
@@ -117,26 +118,61 @@ static int scpsys_sram_disable(struct scpsys_domain *pd)
MTK_POLL_TIMEOUT);
}
-static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd, struct regmap *regmap)
+static int __scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd,
+ struct regmap *regmap)
+{
+ u32 val;
+ u32 mask = bpd->bus_prot_mask;
+ u32 sta_mask = bpd->bus_prot_sta_mask;
+
+ if (bpd->bus_prot_reg_update)
+ regmap_set_bits(regmap, bpd->bus_prot_set, mask);
+ else
+ regmap_write(regmap, bpd->bus_prot_set, mask);
+
+ return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, val,
+ (val & sta_mask) == sta_mask,
+ MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
+}
+
+static int scpsys_bus_way_disable(const struct scpsys_bus_prot_data *bpd,
+ struct regmap *regmap,
+ struct regmap *ack_regmap)
+{
+ u32 val;
+ u32 mask = bpd->bus_prot_mask;
+ u32 sta_mask = bpd->bus_prot_sta_mask;
+
+ if (bpd->bus_prot_reg_update)
+ regmap_clear_bits(regmap, bpd->bus_prot_set, mask);
+ else
+ regmap_write(regmap, bpd->bus_prot_set, mask);
+
+ if (bpd->ignore_clr_ack)
+ return 0;
+
+ return regmap_read_poll_timeout(ack_regmap, bpd->bus_prot_sta, val,
+ (val & sta_mask) == sta_mask,
+ MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
+}
+
+static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd,
+ struct regmap *regmap, struct regmap *infracfg_nao)
{
int i, ret;
for (i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
- u32 val, mask = bpd[i].bus_prot_mask;
-
- if (!mask)
+ if (!bpd[i].bus_prot_mask)
break;
- if (bpd[i].bus_prot_reg_update)
- regmap_set_bits(regmap, bpd[i].bus_prot_set, mask);
+ if (bpd[i].way_en)
+ ret = scpsys_bus_way_disable(&bpd[i], regmap, infracfg_nao);
else
- regmap_write(regmap, bpd[i].bus_prot_set, mask);
-
- ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta,
- val, (val & mask) == mask,
- MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
- if (ret)
+ ret = __scpsys_bus_protect_enable(&bpd[i], regmap);
+ if (ret) {
+ pr_err("%s %d %d\n", __PRETTY_FUNCTION__, __LINE__, ret);
return ret;
+ }
}
return 0;
@@ -146,37 +182,71 @@ static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
{
int ret;
- ret = _scpsys_bus_protect_enable(pd->data->bp_infracfg, pd->infracfg);
+ ret = _scpsys_bus_protect_enable(pd->data->bp_infracfg,
+ pd->infracfg, pd->infracfg_nao);
if (ret)
return ret;
- return _scpsys_bus_protect_enable(pd->data->bp_smi, pd->smi);
+ return _scpsys_bus_protect_enable(pd->data->bp_smi, pd->smi, NULL);
+}
+
+static int __scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd,
+ struct regmap *regmap)
+{
+ u32 val;
+ u32 mask = bpd->bus_prot_mask;
+ u32 sta_mask = bpd->bus_prot_sta_mask;
+
+ if (bpd->bus_prot_reg_update)
+ regmap_clear_bits(regmap, bpd->bus_prot_clr, mask);
+ else
+ regmap_write(regmap, bpd->bus_prot_clr, mask);
+
+ if (bpd->ignore_clr_ack)
+ return 0;
+
+ return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, val,
+ !(val & sta_mask), MTK_POLL_DELAY_US,
+ MTK_POLL_TIMEOUT);
+}
+
+static int scpsys_bus_way_enable(const struct scpsys_bus_prot_data *bpd,
+ struct regmap *regmap,
+ struct regmap *ack_regmap)
+{
+ u32 val;
+ u32 mask = bpd->bus_prot_mask;
+ u32 sta_mask = bpd->bus_prot_sta_mask;
+
+ if (bpd->bus_prot_reg_update)
+ regmap_set_bits(regmap, bpd->bus_prot_clr, mask);
+ else
+ regmap_write(regmap, bpd->bus_prot_clr, mask);
+
+ return regmap_read_poll_timeout(ack_regmap, bpd->bus_prot_sta, val,
+ (val & sta_mask) == sta_mask,
+ MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
}
static int _scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd,
- struct regmap *regmap)
+ struct regmap *regmap,
+ struct regmap *infracfg_nao)
{
int i, ret;
for (i = SPM_MAX_BUS_PROT_DATA - 1; i >= 0; i--) {
- u32 val, mask = bpd[i].bus_prot_mask;
-
- if (!mask)
+ if (!bpd[i].bus_prot_mask)
continue;
- if (bpd[i].bus_prot_reg_update)
- regmap_clear_bits(regmap, bpd[i].bus_prot_clr, mask);
+ if (bpd[i].way_en)
+ ret = scpsys_bus_way_enable(&bpd[i], regmap,
+ infracfg_nao);
else
- regmap_write(regmap, bpd[i].bus_prot_clr, mask);
-
- if (bpd[i].ignore_clr_ack)
- continue;
-
- ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta,
- val, !(val & mask),
- MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
- if (ret)
+ ret = __scpsys_bus_protect_disable(&bpd[i], regmap);
+ if (ret) {
+ pr_err("%s %d %d\n", __PRETTY_FUNCTION__, __LINE__, ret);
return ret;
+ }
}
return 0;
@@ -186,11 +256,12 @@ static int scpsys_bus_protect_disable(struct scpsys_domain *pd)
{
int ret;
- ret = _scpsys_bus_protect_disable(pd->data->bp_smi, pd->smi);
+ ret = _scpsys_bus_protect_disable(pd->data->bp_smi, pd->smi, NULL);
if (ret)
return ret;
- return _scpsys_bus_protect_disable(pd->data->bp_infracfg, pd->infracfg);
+ return _scpsys_bus_protect_disable(pd->data->bp_infracfg,
+ pd->infracfg, pd->infracfg_nao);
}
static int scpsys_regulator_enable(struct regulator *supply)
@@ -294,6 +365,21 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
return 0;
}
+static bool scpsys_bp_infracfg_has_way_en(const struct scpsys_bus_prot_data *bpd)
+{
+ int i;
+
+ for (i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
+ if (!bpd[i].bus_prot_mask)
+ break;
+
+ if (bpd[i].way_en)
+ return true;
+ }
+
+ return false;
+}
+
static struct
generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node)
{
@@ -364,6 +450,20 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
return ERR_CAST(pd->smi);
}
+ if (scpsys_bp_infracfg_has_way_en(pd->data->bp_smi)) {
+ dev_err(scpsys->dev, "bp_smi does not support WAY_EN\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ pd->infracfg_nao = syscon_regmap_lookup_by_phandle_optional(
+ node, "mediatek,infracfg_nao");
+ if (IS_ERR(pd->infracfg_nao)) {
+ if (scpsys_bp_infracfg_has_way_en(pd->data->bp_infracfg))
+ return ERR_CAST(pd->infracfg_nao);
+
+ pd->infracfg_nao = NULL;
+ }
+
num_clks = of_clk_get_parent_count(node);
if (num_clks > 0) {
/* Calculate number of subsys_clks */
diff --git a/drivers/soc/mediatek/mtk-pm-domains.h b/drivers/soc/mediatek/mtk-pm-domains.h
index 7d3c0c36316c..974c68a1d89c 100644
--- a/drivers/soc/mediatek/mtk-pm-domains.h
+++ b/drivers/soc/mediatek/mtk-pm-domains.h
@@ -41,23 +41,29 @@
#define SPM_MAX_BUS_PROT_DATA 6
-#define _BUS_PROT(_mask, _set, _clr, _sta, _update, _ignore) { \
- .bus_prot_mask = (_mask), \
- .bus_prot_set = _set, \
- .bus_prot_clr = _clr, \
- .bus_prot_sta = _sta, \
- .bus_prot_reg_update = _update, \
- .ignore_clr_ack = _ignore, \
+#define _BUS_PROT(_mask, _sta_mask, _set, _clr, _sta, _update, _ignore, _way_en) { \
+ .bus_prot_mask = (_mask), \
+ .bus_prot_set = _set, \
+ .bus_prot_clr = _clr, \
+ .bus_prot_sta = _sta, \
+ .bus_prot_sta_mask = _sta_mask, \
+ .bus_prot_reg_update = _update, \
+ .ignore_clr_ack = _ignore, \
+ .way_en = _way_en, \
}
#define BUS_PROT_WR(_mask, _set, _clr, _sta) \
- _BUS_PROT(_mask, _set, _clr, _sta, false, false)
+ _BUS_PROT(_mask, _mask, _set, _clr, _sta, false, false, false)
#define BUS_PROT_WR_IGN(_mask, _set, _clr, _sta) \
- _BUS_PROT(_mask, _set, _clr, _sta, false, true)
+ _BUS_PROT(_mask, _mask, _set, _clr, _sta, false, true, false)
#define BUS_PROT_UPDATE(_mask, _set, _clr, _sta) \
- _BUS_PROT(_mask, _set, _clr, _sta, true, false)
+ _BUS_PROT(_mask, _mask, _set, _clr, _sta, true, false, false)
+
+#define BUS_PROT_WAY_EN(_en_mask, _sta_mask, _set, _sta) \
+ _BUS_PROT(_en_mask, _sta_mask, _set, _set, _sta, true, false, \
+ true)
#define BUS_PROT_UPDATE_TOPAXI(_mask) \
BUS_PROT_UPDATE(_mask, \
@@ -70,8 +76,10 @@ struct scpsys_bus_prot_data {
u32 bus_prot_set;
u32 bus_prot_clr;
u32 bus_prot_sta;
+ u32 bus_prot_sta_mask;
bool bus_prot_reg_update;
bool ignore_clr_ack;
+ bool way_en;
};
/**
--
2.37.2
On 22/08/2022 16:43, Markus Schneider-Pargmann wrote:
> From: Alexandre Bailon <[email protected]>
>
> This updates the power domain to support WAY_EN operations. These
> operations enable a path between different units of the chip and are
> labeled as 'way_en' in the register descriptions.
>
> This operation is required by the mt8365 for the MM power domain.
>
> Signed-off-by: Alexandre Bailon <[email protected]>
> Signed-off-by: Fabien Parent <[email protected]>
> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> ---
>
> Notes:
> Changes in v3:
> - Separated the way_en functions for clarity
> - Added some checks for infracfg_nao
>
> Changes in v2:
> - some minor style fixes.
> - Renamed 'wayen' to 'way_en' to clarify the meaning
> - Updated commit message
>
> drivers/soc/mediatek/mtk-pm-domains.c | 162 +++++++++++++++++++++-----
> drivers/soc/mediatek/mtk-pm-domains.h | 28 +++--
> 2 files changed, 149 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
> index 9734f1091c69..c2cbe0de6aa1 100644
> --- a/drivers/soc/mediatek/mtk-pm-domains.c
> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> @@ -43,6 +43,7 @@ struct scpsys_domain {
> struct clk_bulk_data *clks;
> int num_subsys_clks;
> struct clk_bulk_data *subsys_clks;
> + struct regmap *infracfg_nao;
> struct regmap *infracfg;
> struct regmap *smi;
> struct regulator *supply;
> @@ -117,26 +118,61 @@ static int scpsys_sram_disable(struct scpsys_domain *pd)
> MTK_POLL_TIMEOUT);
> }
>
> -static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd, struct regmap *regmap)
> +static int __scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd,
> + struct regmap *regmap)
> +{
> + u32 val;
> + u32 mask = bpd->bus_prot_mask;
> + u32 sta_mask = bpd->bus_prot_sta_mask;
> +
> + if (bpd->bus_prot_reg_update)
> + regmap_set_bits(regmap, bpd->bus_prot_set, mask);
> + else
> + regmap_write(regmap, bpd->bus_prot_set, mask);
> +
> + return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, val,
> + (val & sta_mask) == sta_mask,
> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> +}
> +
> +static int scpsys_bus_way_disable(const struct scpsys_bus_prot_data *bpd,
> + struct regmap *regmap,
> + struct regmap *ack_regmap)
> +{
> + u32 val;
> + u32 mask = bpd->bus_prot_mask;
> + u32 sta_mask = bpd->bus_prot_sta_mask;
> +
> + if (bpd->bus_prot_reg_update)
> + regmap_clear_bits(regmap, bpd->bus_prot_set, mask);
> + else
> + regmap_write(regmap, bpd->bus_prot_set, mask);
> +
> + if (bpd->ignore_clr_ack)
> + return 0;
> +
> + return regmap_read_poll_timeout(ack_regmap, bpd->bus_prot_sta, val,
> + (val & sta_mask) == sta_mask,
> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> +}
> +
> +static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd,
> + struct regmap *regmap, struct regmap *infracfg_nao)
> {
> int i, ret;
>
> for (i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
> - u32 val, mask = bpd[i].bus_prot_mask;
> -
> - if (!mask)
> + if (!bpd[i].bus_prot_mask)
> break;
>
> - if (bpd[i].bus_prot_reg_update)
> - regmap_set_bits(regmap, bpd[i].bus_prot_set, mask);
> + if (bpd[i].way_en)
> + ret = scpsys_bus_way_disable(&bpd[i], regmap, infracfg_nao);
> else
> - regmap_write(regmap, bpd[i].bus_prot_set, mask);
> -
> - ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta,
> - val, (val & mask) == mask,
> - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> - if (ret)
> + ret = __scpsys_bus_protect_enable(&bpd[i], regmap);
> + if (ret) {
> + pr_err("%s %d %d\n", __PRETTY_FUNCTION__, __LINE__, ret);
> return ret;
> + }
> }
>
> return 0;
> @@ -146,37 +182,71 @@ static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
> {
> int ret;
>
> - ret = _scpsys_bus_protect_enable(pd->data->bp_infracfg, pd->infracfg);
> + ret = _scpsys_bus_protect_enable(pd->data->bp_infracfg,
> + pd->infracfg, pd->infracfg_nao);
> if (ret)
> return ret;
>
> - return _scpsys_bus_protect_enable(pd->data->bp_smi, pd->smi);
> + return _scpsys_bus_protect_enable(pd->data->bp_smi, pd->smi, NULL);
> +}
> +
> +static int __scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd,
> + struct regmap *regmap)
> +{
> + u32 val;
> + u32 mask = bpd->bus_prot_mask;
> + u32 sta_mask = bpd->bus_prot_sta_mask;
> +
> + if (bpd->bus_prot_reg_update)
> + regmap_clear_bits(regmap, bpd->bus_prot_clr, mask);
> + else
> + regmap_write(regmap, bpd->bus_prot_clr, mask);
> +
> + if (bpd->ignore_clr_ack)
> + return 0;
> +
> + return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, val,
> + !(val & sta_mask), MTK_POLL_DELAY_US,
> + MTK_POLL_TIMEOUT);
> +}
> +
> +static int scpsys_bus_way_enable(const struct scpsys_bus_prot_data *bpd,
> + struct regmap *regmap,
> + struct regmap *ack_regmap)
> +{
> + u32 val;
> + u32 mask = bpd->bus_prot_mask;
> + u32 sta_mask = bpd->bus_prot_sta_mask;
> +
> + if (bpd->bus_prot_reg_update)
> + regmap_set_bits(regmap, bpd->bus_prot_clr, mask);
> + else
> + regmap_write(regmap, bpd->bus_prot_clr, mask);
> +
> + return regmap_read_poll_timeout(ack_regmap, bpd->bus_prot_sta, val,
> + (val & sta_mask) == sta_mask,
> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> }
>
> static int _scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd,
> - struct regmap *regmap)
> + struct regmap *regmap,
> + struct regmap *infracfg_nao)
> {
> int i, ret;
>
> for (i = SPM_MAX_BUS_PROT_DATA - 1; i >= 0; i--) {
> - u32 val, mask = bpd[i].bus_prot_mask;
> -
> - if (!mask)
> + if (!bpd[i].bus_prot_mask)
> continue;
>
> - if (bpd[i].bus_prot_reg_update)
> - regmap_clear_bits(regmap, bpd[i].bus_prot_clr, mask);
> + if (bpd[i].way_en)
> + ret = scpsys_bus_way_enable(&bpd[i], regmap,
> + infracfg_nao);
> else
> - regmap_write(regmap, bpd[i].bus_prot_clr, mask);
> -
> - if (bpd[i].ignore_clr_ack)
> - continue;
> -
> - ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta,
> - val, !(val & mask),
> - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> - if (ret)
> + ret = __scpsys_bus_protect_disable(&bpd[i], regmap);
> + if (ret) {
> + pr_err("%s %d %d\n", __PRETTY_FUNCTION__, __LINE__, ret);
> return ret;
> + }
> }
>
> return 0;
> @@ -186,11 +256,12 @@ static int scpsys_bus_protect_disable(struct scpsys_domain *pd)
> {
> int ret;
>
> - ret = _scpsys_bus_protect_disable(pd->data->bp_smi, pd->smi);
> + ret = _scpsys_bus_protect_disable(pd->data->bp_smi, pd->smi, NULL);
> if (ret)
> return ret;
>
> - return _scpsys_bus_protect_disable(pd->data->bp_infracfg, pd->infracfg);
> + return _scpsys_bus_protect_disable(pd->data->bp_infracfg,
> + pd->infracfg, pd->infracfg_nao);
> }
>
> static int scpsys_regulator_enable(struct regulator *supply)
> @@ -294,6 +365,21 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> return 0;
> }
>
> +static bool scpsys_bp_infracfg_has_way_en(const struct scpsys_bus_prot_data *bpd)
> +{
> + int i;
> +
> + for (i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
> + if (!bpd[i].bus_prot_mask)
> + break;
So MT8365_POWER_DOMAIN_MM will return false as the first member of bp_infracfg
is BUS_PROT_WR(...)
Apart from that, why don't you use a CAPS to acheive the same?
> +
> + if (bpd[i].way_en)
> + return true;
> + }
> +
> + return false;
> +}
> +
> static struct
> generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node)
> {
> @@ -364,6 +450,20 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
> return ERR_CAST(pd->smi);
> }
>
> + if (scpsys_bp_infracfg_has_way_en(pd->data->bp_smi)) {
> + dev_err(scpsys->dev, "bp_smi does not support WAY_EN\n");
Do we really need to check the correctness of the driver data at runtime?
> + return ERR_PTR(-EINVAL);
> + }
> +
> + pd->infracfg_nao = syscon_regmap_lookup_by_phandle_optional(
> + node, "mediatek,infracfg_nao");
Not in the binding description.
> + if (IS_ERR(pd->infracfg_nao)) {
> + if (scpsys_bp_infracfg_has_way_en(pd->data->bp_infracfg))
> + return ERR_CAST(pd->infracfg_nao);
> +
> + pd->infracfg_nao = NULL;
> + }
> +
> num_clks = of_clk_get_parent_count(node);
> if (num_clks > 0) {
> /* Calculate number of subsys_clks */
> diff --git a/drivers/soc/mediatek/mtk-pm-domains.h b/drivers/soc/mediatek/mtk-pm-domains.h
> index 7d3c0c36316c..974c68a1d89c 100644
> --- a/drivers/soc/mediatek/mtk-pm-domains.h
> +++ b/drivers/soc/mediatek/mtk-pm-domains.h
> @@ -41,23 +41,29 @@
>
> #define SPM_MAX_BUS_PROT_DATA 6
>
> -#define _BUS_PROT(_mask, _set, _clr, _sta, _update, _ignore) { \
> - .bus_prot_mask = (_mask), \
> - .bus_prot_set = _set, \
> - .bus_prot_clr = _clr, \
> - .bus_prot_sta = _sta, \
> - .bus_prot_reg_update = _update, \
> - .ignore_clr_ack = _ignore, \
> +#define _BUS_PROT(_mask, _sta_mask, _set, _clr, _sta, _update, _ignore, _way_en) { \
> + .bus_prot_mask = (_mask), \
> + .bus_prot_set = _set, \
> + .bus_prot_clr = _clr, \
> + .bus_prot_sta = _sta, \
> + .bus_prot_sta_mask = _sta_mask, \
> + .bus_prot_reg_update = _update, \
> + .ignore_clr_ack = _ignore, \
> + .way_en = _way_en, \
> }
>
> #define BUS_PROT_WR(_mask, _set, _clr, _sta) \
> - _BUS_PROT(_mask, _set, _clr, _sta, false, false)
> + _BUS_PROT(_mask, _mask, _set, _clr, _sta, false, false, false)
>
> #define BUS_PROT_WR_IGN(_mask, _set, _clr, _sta) \
> - _BUS_PROT(_mask, _set, _clr, _sta, false, true)
> + _BUS_PROT(_mask, _mask, _set, _clr, _sta, false, true, false)
>
> #define BUS_PROT_UPDATE(_mask, _set, _clr, _sta) \
> - _BUS_PROT(_mask, _set, _clr, _sta, true, false)
> + _BUS_PROT(_mask, _mask, _set, _clr, _sta, true, false, false)
> +
> +#define BUS_PROT_WAY_EN(_en_mask, _sta_mask, _set, _sta) \
> + _BUS_PROT(_en_mask, _sta_mask, _set, _set, _sta, true, false, \
> + true)
>
> #define BUS_PROT_UPDATE_TOPAXI(_mask) \
> BUS_PROT_UPDATE(_mask, \
> @@ -70,8 +76,10 @@ struct scpsys_bus_prot_data {
> u32 bus_prot_set;
> u32 bus_prot_clr;
> u32 bus_prot_sta;
> + u32 bus_prot_sta_mask;
I'm not very happy with the naming. In the end we need an extra mask for bus
protection using WAY_EN. But right now I can't come up with a good name.
Regards,
Matthias
> bool bus_prot_reg_update;
> bool ignore_clr_ack;
> + bool way_en;
> };
>
> /**
Hi Matthias,
On Mon, Aug 22, 2022 at 06:17:58PM +0200, Matthias Brugger wrote:
>
>
> On 22/08/2022 16:43, Markus Schneider-Pargmann wrote:
> > From: Alexandre Bailon <[email protected]>
> >
> > This updates the power domain to support WAY_EN operations. These
> > operations enable a path between different units of the chip and are
> > labeled as 'way_en' in the register descriptions.
> >
> > This operation is required by the mt8365 for the MM power domain.
> >
> > Signed-off-by: Alexandre Bailon <[email protected]>
> > Signed-off-by: Fabien Parent <[email protected]>
> > Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> > ---
> >
> > Notes:
> > Changes in v3:
> > - Separated the way_en functions for clarity
> > - Added some checks for infracfg_nao
> > Changes in v2:
> > - some minor style fixes.
> > - Renamed 'wayen' to 'way_en' to clarify the meaning
> > - Updated commit message
> >
> > drivers/soc/mediatek/mtk-pm-domains.c | 162 +++++++++++++++++++++-----
> > drivers/soc/mediatek/mtk-pm-domains.h | 28 +++--
> > 2 files changed, 149 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
> > index 9734f1091c69..c2cbe0de6aa1 100644
> > --- a/drivers/soc/mediatek/mtk-pm-domains.c
> > +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> > @@ -43,6 +43,7 @@ struct scpsys_domain {
> > struct clk_bulk_data *clks;
> > int num_subsys_clks;
> > struct clk_bulk_data *subsys_clks;
> > + struct regmap *infracfg_nao;
> > struct regmap *infracfg;
> > struct regmap *smi;
> > struct regulator *supply;
> > @@ -117,26 +118,61 @@ static int scpsys_sram_disable(struct scpsys_domain *pd)
> > MTK_POLL_TIMEOUT);
> > }
> > -static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd, struct regmap *regmap)
> > +static int __scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd,
> > + struct regmap *regmap)
> > +{
> > + u32 val;
> > + u32 mask = bpd->bus_prot_mask;
> > + u32 sta_mask = bpd->bus_prot_sta_mask;
> > +
> > + if (bpd->bus_prot_reg_update)
> > + regmap_set_bits(regmap, bpd->bus_prot_set, mask);
> > + else
> > + regmap_write(regmap, bpd->bus_prot_set, mask);
> > +
> > + return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, val,
> > + (val & sta_mask) == sta_mask,
> > + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > +}
> > +
> > +static int scpsys_bus_way_disable(const struct scpsys_bus_prot_data *bpd,
> > + struct regmap *regmap,
> > + struct regmap *ack_regmap)
> > +{
> > + u32 val;
> > + u32 mask = bpd->bus_prot_mask;
> > + u32 sta_mask = bpd->bus_prot_sta_mask;
> > +
> > + if (bpd->bus_prot_reg_update)
> > + regmap_clear_bits(regmap, bpd->bus_prot_set, mask);
> > + else
> > + regmap_write(regmap, bpd->bus_prot_set, mask);
> > +
> > + if (bpd->ignore_clr_ack)
> > + return 0;
> > +
> > + return regmap_read_poll_timeout(ack_regmap, bpd->bus_prot_sta, val,
> > + (val & sta_mask) == sta_mask,
> > + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > +}
> > +
> > +static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd,
> > + struct regmap *regmap, struct regmap *infracfg_nao)
> > {
> > int i, ret;
> > for (i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
> > - u32 val, mask = bpd[i].bus_prot_mask;
> > -
> > - if (!mask)
> > + if (!bpd[i].bus_prot_mask)
> > break;
> > - if (bpd[i].bus_prot_reg_update)
> > - regmap_set_bits(regmap, bpd[i].bus_prot_set, mask);
> > + if (bpd[i].way_en)
> > + ret = scpsys_bus_way_disable(&bpd[i], regmap, infracfg_nao);
> > else
> > - regmap_write(regmap, bpd[i].bus_prot_set, mask);
> > -
> > - ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta,
> > - val, (val & mask) == mask,
> > - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > - if (ret)
> > + ret = __scpsys_bus_protect_enable(&bpd[i], regmap);
> > + if (ret) {
> > + pr_err("%s %d %d\n", __PRETTY_FUNCTION__, __LINE__, ret);
> > return ret;
> > + }
> > }
> > return 0;
> > @@ -146,37 +182,71 @@ static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
> > {
> > int ret;
> > - ret = _scpsys_bus_protect_enable(pd->data->bp_infracfg, pd->infracfg);
> > + ret = _scpsys_bus_protect_enable(pd->data->bp_infracfg,
> > + pd->infracfg, pd->infracfg_nao);
> > if (ret)
> > return ret;
> > - return _scpsys_bus_protect_enable(pd->data->bp_smi, pd->smi);
> > + return _scpsys_bus_protect_enable(pd->data->bp_smi, pd->smi, NULL);
> > +}
> > +
> > +static int __scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd,
> > + struct regmap *regmap)
> > +{
> > + u32 val;
> > + u32 mask = bpd->bus_prot_mask;
> > + u32 sta_mask = bpd->bus_prot_sta_mask;
> > +
> > + if (bpd->bus_prot_reg_update)
> > + regmap_clear_bits(regmap, bpd->bus_prot_clr, mask);
> > + else
> > + regmap_write(regmap, bpd->bus_prot_clr, mask);
> > +
> > + if (bpd->ignore_clr_ack)
> > + return 0;
> > +
> > + return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, val,
> > + !(val & sta_mask), MTK_POLL_DELAY_US,
> > + MTK_POLL_TIMEOUT);
> > +}
> > +
> > +static int scpsys_bus_way_enable(const struct scpsys_bus_prot_data *bpd,
> > + struct regmap *regmap,
> > + struct regmap *ack_regmap)
> > +{
> > + u32 val;
> > + u32 mask = bpd->bus_prot_mask;
> > + u32 sta_mask = bpd->bus_prot_sta_mask;
> > +
> > + if (bpd->bus_prot_reg_update)
> > + regmap_set_bits(regmap, bpd->bus_prot_clr, mask);
> > + else
> > + regmap_write(regmap, bpd->bus_prot_clr, mask);
> > +
> > + return regmap_read_poll_timeout(ack_regmap, bpd->bus_prot_sta, val,
> > + (val & sta_mask) == sta_mask,
> > + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > }
> > static int _scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd,
> > - struct regmap *regmap)
> > + struct regmap *regmap,
> > + struct regmap *infracfg_nao)
> > {
> > int i, ret;
> > for (i = SPM_MAX_BUS_PROT_DATA - 1; i >= 0; i--) {
> > - u32 val, mask = bpd[i].bus_prot_mask;
> > -
> > - if (!mask)
> > + if (!bpd[i].bus_prot_mask)
> > continue;
> > - if (bpd[i].bus_prot_reg_update)
> > - regmap_clear_bits(regmap, bpd[i].bus_prot_clr, mask);
> > + if (bpd[i].way_en)
> > + ret = scpsys_bus_way_enable(&bpd[i], regmap,
> > + infracfg_nao);
> > else
> > - regmap_write(regmap, bpd[i].bus_prot_clr, mask);
> > -
> > - if (bpd[i].ignore_clr_ack)
> > - continue;
> > -
> > - ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta,
> > - val, !(val & mask),
> > - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > - if (ret)
> > + ret = __scpsys_bus_protect_disable(&bpd[i], regmap);
> > + if (ret) {
> > + pr_err("%s %d %d\n", __PRETTY_FUNCTION__, __LINE__, ret);
> > return ret;
> > + }
> > }
> > return 0;
> > @@ -186,11 +256,12 @@ static int scpsys_bus_protect_disable(struct scpsys_domain *pd)
> > {
> > int ret;
> > - ret = _scpsys_bus_protect_disable(pd->data->bp_smi, pd->smi);
> > + ret = _scpsys_bus_protect_disable(pd->data->bp_smi, pd->smi, NULL);
> > if (ret)
> > return ret;
> > - return _scpsys_bus_protect_disable(pd->data->bp_infracfg, pd->infracfg);
> > + return _scpsys_bus_protect_disable(pd->data->bp_infracfg,
> > + pd->infracfg, pd->infracfg_nao);
> > }
> > static int scpsys_regulator_enable(struct regulator *supply)
> > @@ -294,6 +365,21 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> > return 0;
> > }
> > +static bool scpsys_bp_infracfg_has_way_en(const struct scpsys_bus_prot_data *bpd)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
> > + if (!bpd[i].bus_prot_mask)
> > + break;
>
> So MT8365_POWER_DOMAIN_MM will return false as the first member of
> bp_infracfg is BUS_PROT_WR(...)
I am not sure I understand what you mean. Why should it break out of the
loop if the first member is a BUS_PROT_WR()? BUS_PROT_WR() sets a mask
as well which is checked here exactly the same way as is done in
_scpsys_bus_protect_enable() even before this patch.
This is only a loop condition. Actually I can move it into the loop
header as well. Either you define SPM_MAX_BUS_PROT_DATA fields or you
have to exit if you find a field that is empty, basically the mask not
being set.
>
> Apart from that, why don't you use a CAPS to acheive the same?
>
> > +
> > + if (bpd[i].way_en)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > static struct
> > generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node)
> > {
> > @@ -364,6 +450,20 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
> > return ERR_CAST(pd->smi);
> > }
> > + if (scpsys_bp_infracfg_has_way_en(pd->data->bp_smi)) {
> > + dev_err(scpsys->dev, "bp_smi does not support WAY_EN\n");
>
> Do we really need to check the correctness of the driver data at runtime?
bp_smi is called without a infracfg_nao regmap. If we don't check it
here, we need to make a check during bus protection operations. Last
time I got a review to not do it during in the bus protection path.
>
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + pd->infracfg_nao = syscon_regmap_lookup_by_phandle_optional(
> > + node, "mediatek,infracfg_nao");
>
> Not in the binding description.
Thanks, I will fix that for the next version.
>
> > + if (IS_ERR(pd->infracfg_nao)) {
> > + if (scpsys_bp_infracfg_has_way_en(pd->data->bp_infracfg))
> > + return ERR_CAST(pd->infracfg_nao);
> > +
> > + pd->infracfg_nao = NULL;
> > + }
> > +
> > num_clks = of_clk_get_parent_count(node);
> > if (num_clks > 0) {
> > /* Calculate number of subsys_clks */
> > diff --git a/drivers/soc/mediatek/mtk-pm-domains.h b/drivers/soc/mediatek/mtk-pm-domains.h
> > index 7d3c0c36316c..974c68a1d89c 100644
> > --- a/drivers/soc/mediatek/mtk-pm-domains.h
> > +++ b/drivers/soc/mediatek/mtk-pm-domains.h
> > @@ -41,23 +41,29 @@
> > #define SPM_MAX_BUS_PROT_DATA 6
> > -#define _BUS_PROT(_mask, _set, _clr, _sta, _update, _ignore) { \
> > - .bus_prot_mask = (_mask), \
> > - .bus_prot_set = _set, \
> > - .bus_prot_clr = _clr, \
> > - .bus_prot_sta = _sta, \
> > - .bus_prot_reg_update = _update, \
> > - .ignore_clr_ack = _ignore, \
> > +#define _BUS_PROT(_mask, _sta_mask, _set, _clr, _sta, _update, _ignore, _way_en) { \
> > + .bus_prot_mask = (_mask), \
> > + .bus_prot_set = _set, \
> > + .bus_prot_clr = _clr, \
> > + .bus_prot_sta = _sta, \
> > + .bus_prot_sta_mask = _sta_mask, \
> > + .bus_prot_reg_update = _update, \
> > + .ignore_clr_ack = _ignore, \
> > + .way_en = _way_en, \
> > }
> > #define BUS_PROT_WR(_mask, _set, _clr, _sta) \
> > - _BUS_PROT(_mask, _set, _clr, _sta, false, false)
> > + _BUS_PROT(_mask, _mask, _set, _clr, _sta, false, false, false)
> > #define BUS_PROT_WR_IGN(_mask, _set, _clr, _sta) \
> > - _BUS_PROT(_mask, _set, _clr, _sta, false, true)
> > + _BUS_PROT(_mask, _mask, _set, _clr, _sta, false, true, false)
> > #define BUS_PROT_UPDATE(_mask, _set, _clr, _sta) \
> > - _BUS_PROT(_mask, _set, _clr, _sta, true, false)
> > + _BUS_PROT(_mask, _mask, _set, _clr, _sta, true, false, false)
> > +
> > +#define BUS_PROT_WAY_EN(_en_mask, _sta_mask, _set, _sta) \
> > + _BUS_PROT(_en_mask, _sta_mask, _set, _set, _sta, true, false, \
> > + true)
> > #define BUS_PROT_UPDATE_TOPAXI(_mask) \
> > BUS_PROT_UPDATE(_mask, \
> > @@ -70,8 +76,10 @@ struct scpsys_bus_prot_data {
> > u32 bus_prot_set;
> > u32 bus_prot_clr;
> > u32 bus_prot_sta;
> > + u32 bus_prot_sta_mask;
>
> I'm not very happy with the naming. In the end we need an extra mask for bus
> protection using WAY_EN. But right now I can't come up with a good name.
I think the naming is good as it is a specific mask for the status
register. bus_prot_mask is now basically only responsible for set and
clr. Maybe renaming bus_prot_mask to bus_prot_set_clr_mask is better?
Thanks,
Markus
>
> Regards,
> Matthias
>
> > bool bus_prot_reg_update;
> > bool ignore_clr_ack;
> > + bool way_en;
> > };
> > /**
On 06/09/2022 11:49, Markus Schneider-Pargmann wrote:
> Hi Matthias,
>
> On Mon, Aug 22, 2022 at 06:17:58PM +0200, Matthias Brugger wrote:
>>
>>
>> On 22/08/2022 16:43, Markus Schneider-Pargmann wrote:
>>> From: Alexandre Bailon <[email protected]>
>>>
>>> This updates the power domain to support WAY_EN operations. These
>>> operations enable a path between different units of the chip and are
>>> labeled as 'way_en' in the register descriptions.
If you can come up with a more verbose description of WAY_EN functionality it
would be appreciated, although I know it's not always easy to get that
information. 10 years in it will help to understand better what this is about.
>>>
>>> This operation is required by the mt8365 for the MM power domain.
>>>
>>> Signed-off-by: Alexandre Bailon <[email protected]>
>>> Signed-off-by: Fabien Parent <[email protected]>
>>> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
>>> ---
>>>
>>> Notes:
>>> Changes in v3:
>>> - Separated the way_en functions for clarity
>>> - Added some checks for infracfg_nao
>>> Changes in v2:
>>> - some minor style fixes.
>>> - Renamed 'wayen' to 'way_en' to clarify the meaning
>>> - Updated commit message
>>>
>>> drivers/soc/mediatek/mtk-pm-domains.c | 162 +++++++++++++++++++++-----
>>> drivers/soc/mediatek/mtk-pm-domains.h | 28 +++--
>>> 2 files changed, 149 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
>>> index 9734f1091c69..c2cbe0de6aa1 100644
>>> --- a/drivers/soc/mediatek/mtk-pm-domains.c
>>> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
>>> @@ -43,6 +43,7 @@ struct scpsys_domain {
>>> struct clk_bulk_data *clks;
>>> int num_subsys_clks;
>>> struct clk_bulk_data *subsys_clks;
>>> + struct regmap *infracfg_nao;
>>> struct regmap *infracfg;
>>> struct regmap *smi;
>>> struct regulator *supply;
>>> @@ -117,26 +118,61 @@ static int scpsys_sram_disable(struct scpsys_domain *pd)
>>> MTK_POLL_TIMEOUT);
>>> }
>>> -static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd, struct regmap *regmap)
>>> +static int __scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd,
>>> + struct regmap *regmap)
>>> +{
>>> + u32 val;
>>> + u32 mask = bpd->bus_prot_mask;
>>> + u32 sta_mask = bpd->bus_prot_sta_mask;
>>> +
>>> + if (bpd->bus_prot_reg_update)
>>> + regmap_set_bits(regmap, bpd->bus_prot_set, mask);
>>> + else
>>> + regmap_write(regmap, bpd->bus_prot_set, mask);
>>> +
>>> + return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, val,
>>> + (val & sta_mask) == sta_mask,
>>> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> +}
>>> +
>>> +static int scpsys_bus_way_disable(const struct scpsys_bus_prot_data *bpd,
>>> + struct regmap *regmap,
>>> + struct regmap *ack_regmap)
>>> +{
>>> + u32 val;
>>> + u32 mask = bpd->bus_prot_mask;
>>> + u32 sta_mask = bpd->bus_prot_sta_mask;
>>> +
>>> + if (bpd->bus_prot_reg_update)
>>> + regmap_clear_bits(regmap, bpd->bus_prot_set, mask);
>>> + else
>>> + regmap_write(regmap, bpd->bus_prot_set, mask);
BUS_PROT_WAY_EN sets bus_prot_reg_update to true, what do we need the else
branch for?
>>> +
>>> + if (bpd->ignore_clr_ack)
>>> + return 0;
Same here, ignore_clr_ack is always false.
Actually it seems to me that __scpsys_bus_protect_enable and
scpsys_bus_way_disable is nearly the same (other regmap, regmap_clear_bits
instead of set_bits). Can't we put the check of way_en in e.g. the old
_scpsys_bus_protect_enable to get rid of all the code duplication?
>>> +
>>> + return regmap_read_poll_timeout(ack_regmap, bpd->bus_prot_sta, val,
>>> + (val & sta_mask) == sta_mask,
>>> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> +}
>>> +
>>> +static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd,
>>> + struct regmap *regmap, struct regmap *infracfg_nao)
>>> {
>>> int i, ret;
>>> for (i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
>>> - u32 val, mask = bpd[i].bus_prot_mask;
>>> -
>>> - if (!mask)
>>> + if (!bpd[i].bus_prot_mask)
>>> break;
>>> - if (bpd[i].bus_prot_reg_update)
>>> - regmap_set_bits(regmap, bpd[i].bus_prot_set, mask);
>>> + if (bpd[i].way_en)
>>> + ret = scpsys_bus_way_disable(&bpd[i], regmap, infracfg_nao);
>>> else
>>> - regmap_write(regmap, bpd[i].bus_prot_set, mask);
>>> -
>>> - ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta,
>>> - val, (val & mask) == mask,
>>> - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> - if (ret)
>>> + ret = __scpsys_bus_protect_enable(&bpd[i], regmap);
>>> + if (ret) {
>>> + pr_err("%s %d %d\n", __PRETTY_FUNCTION__, __LINE__, ret);
>>> return ret;
>>> + }
>>> }
>>> return 0;
>>> @@ -146,37 +182,71 @@ static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
>>> {
>>> int ret;
>>> - ret = _scpsys_bus_protect_enable(pd->data->bp_infracfg, pd->infracfg);
>>> + ret = _scpsys_bus_protect_enable(pd->data->bp_infracfg,
>>> + pd->infracfg, pd->infracfg_nao);
>>> if (ret)
>>> return ret;
>>> - return _scpsys_bus_protect_enable(pd->data->bp_smi, pd->smi);
>>> + return _scpsys_bus_protect_enable(pd->data->bp_smi, pd->smi, NULL);
>>> +}
>>> +
>>> +static int __scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd,
>>> + struct regmap *regmap)
>>> +{
>>> + u32 val;
>>> + u32 mask = bpd->bus_prot_mask;
>>> + u32 sta_mask = bpd->bus_prot_sta_mask;
>>> +
>>> + if (bpd->bus_prot_reg_update)
>>> + regmap_clear_bits(regmap, bpd->bus_prot_clr, mask);
>>> + else
>>> + regmap_write(regmap, bpd->bus_prot_clr, mask);
>>> +
>>> + if (bpd->ignore_clr_ack)
>>> + return 0;
>>> +
>>> + return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, val,
>>> + !(val & sta_mask), MTK_POLL_DELAY_US,
>>> + MTK_POLL_TIMEOUT);
>>> +}
>>> +
>>> +static int scpsys_bus_way_enable(const struct scpsys_bus_prot_data *bpd,
>>> + struct regmap *regmap,
>>> + struct regmap *ack_regmap)
>>> +{
>>> + u32 val;
>>> + u32 mask = bpd->bus_prot_mask;
>>> + u32 sta_mask = bpd->bus_prot_sta_mask;
>>> +
>>> + if (bpd->bus_prot_reg_update)
>>> + regmap_set_bits(regmap, bpd->bus_prot_clr, mask);
>>> + else
>>> + regmap_write(regmap, bpd->bus_prot_clr, mask);
same as disable case.
>>> +
>>> + return regmap_read_poll_timeout(ack_regmap, bpd->bus_prot_sta, val,
>>> + (val & sta_mask) == sta_mask,
>>> + MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> }
>>> static int _scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd,
>>> - struct regmap *regmap)
>>> + struct regmap *regmap,
>>> + struct regmap *infracfg_nao)
>>> {
>>> int i, ret;
>>> for (i = SPM_MAX_BUS_PROT_DATA - 1; i >= 0; i--) {
>>> - u32 val, mask = bpd[i].bus_prot_mask;
>>> -
>>> - if (!mask)
>>> + if (!bpd[i].bus_prot_mask)
>>> continue;
>>> - if (bpd[i].bus_prot_reg_update)
>>> - regmap_clear_bits(regmap, bpd[i].bus_prot_clr, mask);
>>> + if (bpd[i].way_en)
>>> + ret = scpsys_bus_way_enable(&bpd[i], regmap,
>>> + infracfg_nao);
>>> else
>>> - regmap_write(regmap, bpd[i].bus_prot_clr, mask);
>>> -
>>> - if (bpd[i].ignore_clr_ack)
>>> - continue;
>>> -
>>> - ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta,
>>> - val, !(val & mask),
>>> - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
>>> - if (ret)
>>> + ret = __scpsys_bus_protect_disable(&bpd[i], regmap);
>>> + if (ret) {
>>> + pr_err("%s %d %d\n", __PRETTY_FUNCTION__, __LINE__, ret);
>>> return ret;
>>> + }
>>> }
>>> return 0;
>>> @@ -186,11 +256,12 @@ static int scpsys_bus_protect_disable(struct scpsys_domain *pd)
>>> {
>>> int ret;
>>> - ret = _scpsys_bus_protect_disable(pd->data->bp_smi, pd->smi);
>>> + ret = _scpsys_bus_protect_disable(pd->data->bp_smi, pd->smi, NULL);
>>> if (ret)
>>> return ret;
>>> - return _scpsys_bus_protect_disable(pd->data->bp_infracfg, pd->infracfg);
>>> + return _scpsys_bus_protect_disable(pd->data->bp_infracfg,
>>> + pd->infracfg, pd->infracfg_nao);
>>> }
>>> static int scpsys_regulator_enable(struct regulator *supply)
>>> @@ -294,6 +365,21 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>>> return 0;
>>> }
>>> +static bool scpsys_bp_infracfg_has_way_en(const struct scpsys_bus_prot_data *bpd)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
>>> + if (!bpd[i].bus_prot_mask)
>>> + break;
>>
>> So MT8365_POWER_DOMAIN_MM will return false as the first member of
>> bp_infracfg is BUS_PROT_WR(...)
>
> I am not sure I understand what you mean. Why should it break out of the
> loop if the first member is a BUS_PROT_WR()? BUS_PROT_WR() sets a mask
> as well which is checked here exactly the same way as is done in
> _scpsys_bus_protect_enable() even before this patch.
Right, my error, please see comment further down about the new macros.
>
> This is only a loop condition. Actually I can move it into the loop
> header as well. Either you define SPM_MAX_BUS_PROT_DATA fields or you
> have to exit if you find a field that is empty, basically the mask not
> being set.
>
>>
>> Apart from that, why don't you use a CAPS to acheive the same?
>>
Just in case you missed that. I'd pretty much prefer to add a caps to
scpsys_domain_data signaling that the power domain uses WAY_EN.
>>> +
>>> + if (bpd[i].way_en)
>>> + return true;
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> static struct
>>> generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node)
>>> {
>>> @@ -364,6 +450,20 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
>>> return ERR_CAST(pd->smi);
>>> }
>>> + if (scpsys_bp_infracfg_has_way_en(pd->data->bp_smi)) {
>>> + dev_err(scpsys->dev, "bp_smi does not support WAY_EN\n");
>>
>> Do we really need to check the correctness of the driver data at runtime?
>
> bp_smi is called without a infracfg_nao regmap. If we don't check it
> here, we need to make a check during bus protection operations. Last
> time I got a review to not do it during in the bus protection path.
>
Maybe I'm missing something here but bpi_smi is defined in the
scpsys_domain_data. Why do we need to check at runtime if the SoC specific data
hardcoded in the dirver is correct? What am I missing here?
Apart do I understand correctly that the second call to
_scpsys_bus_protect_enable() in scpsys_bus_protect_enable() will never use
way_en? My understanding that this also holds for the disable path. Can't we
remodel the code that in this case we don't loop over scpsys_bus_prot_data and
check for way_en?
>>
>>> + return ERR_PTR(-EINVAL);
>>> + }
>>> +
>>> + pd->infracfg_nao = syscon_regmap_lookup_by_phandle_optional(
>>> + node, "mediatek,infracfg_nao");
>>
>> Not in the binding description.
>
> Thanks, I will fix that for the next version.
>
>>
>>> + if (IS_ERR(pd->infracfg_nao)) {
>>> + if (scpsys_bp_infracfg_has_way_en(pd->data->bp_infracfg))
>>> + return ERR_CAST(pd->infracfg_nao);
>>> +
>>> + pd->infracfg_nao = NULL;
>>> + }
>>> +
>>> num_clks = of_clk_get_parent_count(node);
>>> if (num_clks > 0) {
>>> /* Calculate number of subsys_clks */
>>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.h b/drivers/soc/mediatek/mtk-pm-domains.h
>>> index 7d3c0c36316c..974c68a1d89c 100644
>>> --- a/drivers/soc/mediatek/mtk-pm-domains.h
>>> +++ b/drivers/soc/mediatek/mtk-pm-domains.h
>>> @@ -41,23 +41,29 @@
>>> #define SPM_MAX_BUS_PROT_DATA 6
>>> -#define _BUS_PROT(_mask, _set, _clr, _sta, _update, _ignore) { \
>>> - .bus_prot_mask = (_mask), \
>>> - .bus_prot_set = _set, \
>>> - .bus_prot_clr = _clr, \
>>> - .bus_prot_sta = _sta, \
>>> - .bus_prot_reg_update = _update, \
>>> - .ignore_clr_ack = _ignore, \
>>> +#define _BUS_PROT(_mask, _sta_mask, _set, _clr, _sta, _update, _ignore, _way_en) { \
_sta_mask is second parameter but not second member of the struct. That makes
reading the driver unnecessary complicated.
>>> + .bus_prot_mask = (_mask), \
>>> + .bus_prot_set = _set, \
>>> + .bus_prot_clr = _clr, \
>>> + .bus_prot_sta = _sta, \
>>> + .bus_prot_sta_mask = _sta_mask, \
>>> + .bus_prot_reg_update = _update, \
>>> + .ignore_clr_ack = _ignore, \
>>> + .way_en = _way_en, \
>>> }
>>> #define BUS_PROT_WR(_mask, _set, _clr, _sta) \
>>> - _BUS_PROT(_mask, _set, _clr, _sta, false, false)
>>> + _BUS_PROT(_mask, _mask, _set, _clr, _sta, false, false, false)
>>> #define BUS_PROT_WR_IGN(_mask, _set, _clr, _sta) \
>>> - _BUS_PROT(_mask, _set, _clr, _sta, false, true)
>>> + _BUS_PROT(_mask, _mask, _set, _clr, _sta, false, true, false)
>>> #define BUS_PROT_UPDATE(_mask, _set, _clr, _sta) \
>>> - _BUS_PROT(_mask, _set, _clr, _sta, true, false)
>>> + _BUS_PROT(_mask, _mask, _set, _clr, _sta, true, false, false)
>>> +
>>> +#define BUS_PROT_WAY_EN(_en_mask, _sta_mask, _set, _sta) \
>>> + _BUS_PROT(_en_mask, _sta_mask, _set, _set, _sta, true, false, \
>>> + true)
>>> #define BUS_PROT_UPDATE_TOPAXI(_mask) \
>>> BUS_PROT_UPDATE(_mask, \
>>> @@ -70,8 +76,10 @@ struct scpsys_bus_prot_data {
>>> u32 bus_prot_set;
>>> u32 bus_prot_clr;
>>> u32 bus_prot_sta;
>>> + u32 bus_prot_sta_mask;
>>
>> I'm not very happy with the naming. In the end we need an extra mask for bus
>> protection using WAY_EN. But right now I can't come up with a good name.
>
> I think the naming is good as it is a specific mask for the status
> register. bus_prot_mask is now basically only responsible for set and
> clr. Maybe renaming bus_prot_mask to bus_prot_set_clr_mask is better?
Yes makes sense. Please add this as an extra patch for easier review.
Regards,
Matthias
>
> Thanks,
> Markus
>
>>
>> Regards,
>> Matthias
>>
>>> bool bus_prot_reg_update;
>>> bool ignore_clr_ack;
>>> + bool way_en;
>>> };
>>> /**