| Message ID | 20230619085344.2885311-1-msp@baylibre.com |
|---|---|
| Headers | show |
| Series | soc: mediatek: MT8365 power support | expand |
Hi Angelo, On Mon, Jun 19, 2023 at 11:32:18AM +0200, AngeloGioacchino Del Regno wrote: > Il 19/06/23 10:53, Markus Schneider-Pargmann ha scritto: > > To simplify the macros, use a flags field for simple bools. This is in > > preparation for more flags. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > --- > > drivers/soc/mediatek/mtk-pm-domains.c | 6 +++--- > > drivers/soc/mediatek/mtk-pm-domains.h | 19 +++++++++++-------- > > 2 files changed, 14 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c > > index 354249cc1b12..aa9ab413479e 100644 > > --- a/drivers/soc/mediatek/mtk-pm-domains.c > > +++ b/drivers/soc/mediatek/mtk-pm-domains.c > > @@ -128,7 +128,7 @@ static int _scpsys_bus_protect_enable(const struct scpsys_bus_prot_data *bpd, st > > if (!mask) > > break; > > - if (bpd[i].bus_prot_reg_update) > > + if (bpd[i].flags & BUS_PROT_REG_UPDATE) > > regmap_set_bits(regmap, bpd[i].bus_prot_set, mask); > > else > > regmap_write(regmap, bpd[i].bus_prot_set, mask); > > @@ -165,12 +165,12 @@ static int _scpsys_bus_protect_disable(const struct scpsys_bus_prot_data *bpd, > > if (!mask) > > continue; > > - if (bpd[i].bus_prot_reg_update) > > + if (bpd[i].flags & BUS_PROT_REG_UPDATE) > > regmap_clear_bits(regmap, bpd[i].bus_prot_clr, mask); > > else > > regmap_write(regmap, bpd[i].bus_prot_clr, mask); > > - if (bpd[i].ignore_clr_ack) > > + if (bpd[i].flags & BUS_PROT_IGNORE_CLR_ACK) > > continue; > > ret = regmap_read_poll_timeout(regmap, bpd[i].bus_prot_sta, > > diff --git a/drivers/soc/mediatek/mtk-pm-domains.h b/drivers/soc/mediatek/mtk-pm-domains.h > > index 5ec53ee073c4..e26c8c317a6b 100644 > > --- a/drivers/soc/mediatek/mtk-pm-domains.h > > +++ b/drivers/soc/mediatek/mtk-pm-domains.h > > @@ -42,23 +42,27 @@ > > #define SPM_MAX_BUS_PROT_DATA 6 > > -#define _BUS_PROT(_mask, _set, _clr, _sta, _update, _ignore) { \ > > +enum scpsys_bus_prot_flags { > > + BUS_PROT_REG_UPDATE = BIT(1), > > + BUS_PROT_IGNORE_CLR_ACK = BIT(2), > > +}; > > + > > +#define _BUS_PROT(_mask, _set, _clr, _sta, _flags) { \ > > .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, \ > > + .flags = _flags \ > > } > > #define BUS_PROT_WR(_mask, _set, _clr, _sta) \ > > - _BUS_PROT(_mask, _set, _clr, _sta, false, false) > > + _BUS_PROT(_mask, _set, _clr, _sta, 0) > > #define BUS_PROT_WR_IGN(_mask, _set, _clr, _sta) \ > > - _BUS_PROT(_mask, _set, _clr, _sta, false, true) > > + _BUS_PROT(_mask, _set, _clr, _sta, BUS_PROT_IGNORE_CLR_ACK) > > #define BUS_PROT_UPDATE(_mask, _set, _clr, _sta) \ > > - _BUS_PROT(_mask, _set, _clr, _sta, true, false) > > + _BUS_PROT(_mask, _set, _clr, _sta, BUS_PROT_REG_UPDATE) > > #define BUS_PROT_UPDATE_TOPAXI(_mask) \ > > BUS_PROT_UPDATE(_mask, \ > > @@ -71,8 +75,7 @@ struct scpsys_bus_prot_data { > > u32 bus_prot_set; > > u32 bus_prot_clr; > > u32 bus_prot_sta; > > - bool bus_prot_reg_update; > > - bool ignore_clr_ack; > > + u32 flags; > > As far as I understand, we don't expect more than six bits to be populated as bus > protection flags, so we can save some memory by changing that to u8... Thank you. Yes, also we can change it later if we need more flags at some point. I will change it. But I guess it won't save any memory as the compiler probably aligns the struct. Best, Markus > > ...after which: > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > Regards, > Angelo
On Mon, Jun 19, 2023 at 11:29:18AM +0200, AngeloGioacchino Del Regno wrote: > Il 19/06/23 10:53, Markus Schneider-Pargmann ha scritto: > > From: Alexandre Bailon <abailon@baylibre.com> > > > > This updates the power domain to support WAY_EN operations. WAY_EN > > operations on mt8365 are using a different component to check for the > > acknowledgment, namely the infracfg-nao component. Also to enable a way > > it the bit needs to be cleared while disabling a way needs a bit to be > > set. To support these two operations two flags are added, > > BUS_PROT_INVERTED and BUS_PROT_STA_COMPONENT_INFRA_NAO. Additionally > > another regmap is created if the INFRA_NAO capability is set. > > > > This operation is required by the mt8365 for the MM power domain. > > > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > > Signed-off-by: Fabien Parent <fparent@baylibre.com> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > --- > > drivers/soc/mediatek/mtk-pm-domains.c | 39 +++++++++++++++++++++++---- > > drivers/soc/mediatek/mtk-pm-domains.h | 7 +++-- > > 2 files changed, 39 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c > > index 3cdf62c0b6bd..4659f0a0aa08 100644 > > --- a/drivers/soc/mediatek/mtk-pm-domains.c > > +++ b/drivers/soc/mediatek/mtk-pm-domains.c > > @@ -44,6 +44,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; > > @@ -127,13 +128,26 @@ static struct regmap *scpsys_bus_protect_get_regmap(struct scpsys_domain *pd, > > return pd->infracfg; > > } > > +static struct regmap *scpsys_bus_protect_get_sta_regmap(struct scpsys_domain *pd, > > + const struct scpsys_bus_prot_data *bpd) > > +{ > > + if (bpd->flags & BUS_PROT_STA_COMPONENT_INFRA_NAO) > > + return pd->infracfg_nao; > > + else > > + return scpsys_bus_protect_get_regmap(pd, bpd); > > +} > > + > > static int scpsys_bus_protect_clear(struct scpsys_domain *pd, > > const struct scpsys_bus_prot_data *bpd) > > { > > + struct regmap *sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd); > > struct regmap *regmap = scpsys_bus_protect_get_regmap(pd, bpd); > > + u32 expected_ack; > > u32 val; > > u32 sta_mask = bpd->bus_prot_sta_mask; > > + expected_ack = (bpd->flags & BUS_PROT_STA_COMPONENT_INFRA_NAO ? sta_mask : 0); > > + > > if (bpd->flags & BUS_PROT_REG_UPDATE) > > regmap_clear_bits(regmap, bpd->bus_prot_clr, bpd->bus_prot_set_clr_mask); > > else > > @@ -142,14 +156,15 @@ static int scpsys_bus_protect_clear(struct scpsys_domain *pd, > > if (bpd->flags & BUS_PROT_IGNORE_CLR_ACK) > > return 0; > > - return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, > > - val, !(val & sta_mask), > > + return regmap_read_poll_timeout(sta_regmap, bpd->bus_prot_sta, > > + val, (val & sta_mask) == expected_ack, > > MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT); > > } > > static int scpsys_bus_protect_set(struct scpsys_domain *pd, > > const struct scpsys_bus_prot_data *bpd) > > { > > + struct regmap *sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd); > > struct regmap *regmap = scpsys_bus_protect_get_regmap(pd, bpd); > > u32 val; > > u32 sta_mask = bpd->bus_prot_sta_mask; > > @@ -159,7 +174,7 @@ static int scpsys_bus_protect_set(struct scpsys_domain *pd, > > else > > regmap_write(regmap, bpd->bus_prot_set, bpd->bus_prot_set_clr_mask); > > - return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, > > + return regmap_read_poll_timeout(sta_regmap, bpd->bus_prot_sta, > > val, (val & sta_mask) == sta_mask, > > MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT); > > } > > @@ -173,7 +188,10 @@ static int scpsys_bus_protect_enable(struct scpsys_domain *pd) > > if (!bpd->bus_prot_set_clr_mask) > > break; > > - ret = scpsys_bus_protect_set(pd, bpd); > > + if (bpd->flags & BUS_PROT_INVERTED) > > + ret = scpsys_bus_protect_clear(pd, bpd); > > + else > > + ret = scpsys_bus_protect_set(pd, bpd); > > if (ret) > > return ret; > > } > > @@ -190,7 +208,10 @@ static int scpsys_bus_protect_disable(struct scpsys_domain *pd) > > if (!bpd->bus_prot_set_clr_mask) > > continue; > > - ret = scpsys_bus_protect_clear(pd, bpd); > > + if (bpd->flags & BUS_PROT_INVERTED) > > + ret = scpsys_bus_protect_set(pd, bpd); > > + else > > + ret = scpsys_bus_protect_clear(pd, bpd); > > if (ret) > > return ret; > > } > > @@ -377,6 +398,14 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no > > return ERR_CAST(pd->smi); > > } > > + pd->infracfg_nao = syscon_regmap_lookup_by_phandle(node, "mediatek,infracfg-nao"); > > If we don't expect infracfg-nao to be present, what's the point about trying to > get a regmap handle and then failing only if we do expect it to be there? > > At this point you can just do... > > if (MTK_SCPD_CAPS(pd, MTK_SCPD_HAS_INFRA_NAO)) { > pd->infracfg_nao = syscon_regmap_lookup_by_phandle(...); > if (IS_ERR(....)) > return .... > } Yes! My code looks stupid. Thanks! > > > + if (IS_ERR(pd->infracfg_nao)) { > > + if (MTK_SCPD_CAPS(pd, MTK_SCPD_HAS_INFRA_NAO)) > > + 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 356788263db2..562d4e92ce16 100644 > > --- a/drivers/soc/mediatek/mtk-pm-domains.h > > +++ b/drivers/soc/mediatek/mtk-pm-domains.h > > @@ -11,6 +11,7 @@ > > /* can't set MTK_SCPD_KEEP_DEFAULT_OFF at the same time */ > > #define MTK_SCPD_ALWAYS_ON BIT(5) > > #define MTK_SCPD_EXT_BUCK_ISO BIT(6) > > +#define MTK_SCPD_HAS_INFRA_NAO BIT(7) > > #define MTK_SCPD_CAPS(_scpd, _x) ((_scpd)->data->caps & (_x)) > > #define SPM_VDE_PWR_CON 0x0210 > > @@ -45,8 +46,10 @@ > > enum scpsys_bus_prot_flags { > > BUS_PROT_REG_UPDATE = BIT(1), > > BUS_PROT_IGNORE_CLR_ACK = BIT(2), > > - BUS_PROT_COMPONENT_INFRA = BIT(3), > > - BUS_PROT_COMPONENT_SMI = BIT(4), > > + BUS_PROT_INVERTED = BIT(3), > > I get the reason why you're setting inverted as bit 3, but at that point you can > just set BUS_PROT_COMPONENT_INFRA to bit 4 from the very beginning, instead of > using bit 3 for that and then changing them all in a subsequent commit (this one). Yes, I was torn between making the commits independent and avoiding this move later on. I decided for the first. If you prefer I can set it to the correct bits right from the beginning. Best, Markus
Il 22/06/23 10:39, Markus Schneider-Pargmann ha scritto: > On Mon, Jun 19, 2023 at 11:29:18AM +0200, AngeloGioacchino Del Regno wrote: >> Il 19/06/23 10:53, Markus Schneider-Pargmann ha scritto: >>> From: Alexandre Bailon <abailon@baylibre.com> >>> >>> This updates the power domain to support WAY_EN operations. WAY_EN >>> operations on mt8365 are using a different component to check for the >>> acknowledgment, namely the infracfg-nao component. Also to enable a way >>> it the bit needs to be cleared while disabling a way needs a bit to be >>> set. To support these two operations two flags are added, >>> BUS_PROT_INVERTED and BUS_PROT_STA_COMPONENT_INFRA_NAO. Additionally >>> another regmap is created if the INFRA_NAO capability is set. >>> >>> This operation is required by the mt8365 for the MM power domain. >>> >>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >>> Signed-off-by: Fabien Parent <fparent@baylibre.com> >>> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> >>> --- >>> drivers/soc/mediatek/mtk-pm-domains.c | 39 +++++++++++++++++++++++---- >>> drivers/soc/mediatek/mtk-pm-domains.h | 7 +++-- >>> 2 files changed, 39 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c >>> index 3cdf62c0b6bd..4659f0a0aa08 100644 >>> --- a/drivers/soc/mediatek/mtk-pm-domains.c >>> +++ b/drivers/soc/mediatek/mtk-pm-domains.c >>> @@ -44,6 +44,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; >>> @@ -127,13 +128,26 @@ static struct regmap *scpsys_bus_protect_get_regmap(struct scpsys_domain *pd, >>> return pd->infracfg; >>> } >>> +static struct regmap *scpsys_bus_protect_get_sta_regmap(struct scpsys_domain *pd, >>> + const struct scpsys_bus_prot_data *bpd) >>> +{ >>> + if (bpd->flags & BUS_PROT_STA_COMPONENT_INFRA_NAO) >>> + return pd->infracfg_nao; >>> + else >>> + return scpsys_bus_protect_get_regmap(pd, bpd); >>> +} >>> + >>> static int scpsys_bus_protect_clear(struct scpsys_domain *pd, >>> const struct scpsys_bus_prot_data *bpd) >>> { >>> + struct regmap *sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd); >>> struct regmap *regmap = scpsys_bus_protect_get_regmap(pd, bpd); >>> + u32 expected_ack; >>> u32 val; >>> u32 sta_mask = bpd->bus_prot_sta_mask; >>> + expected_ack = (bpd->flags & BUS_PROT_STA_COMPONENT_INFRA_NAO ? sta_mask : 0); >>> + >>> if (bpd->flags & BUS_PROT_REG_UPDATE) >>> regmap_clear_bits(regmap, bpd->bus_prot_clr, bpd->bus_prot_set_clr_mask); >>> else >>> @@ -142,14 +156,15 @@ static int scpsys_bus_protect_clear(struct scpsys_domain *pd, >>> if (bpd->flags & BUS_PROT_IGNORE_CLR_ACK) >>> return 0; >>> - return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, >>> - val, !(val & sta_mask), >>> + return regmap_read_poll_timeout(sta_regmap, bpd->bus_prot_sta, >>> + val, (val & sta_mask) == expected_ack, >>> MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT); >>> } >>> static int scpsys_bus_protect_set(struct scpsys_domain *pd, >>> const struct scpsys_bus_prot_data *bpd) >>> { >>> + struct regmap *sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd); >>> struct regmap *regmap = scpsys_bus_protect_get_regmap(pd, bpd); >>> u32 val; >>> u32 sta_mask = bpd->bus_prot_sta_mask; >>> @@ -159,7 +174,7 @@ static int scpsys_bus_protect_set(struct scpsys_domain *pd, >>> else >>> regmap_write(regmap, bpd->bus_prot_set, bpd->bus_prot_set_clr_mask); >>> - return regmap_read_poll_timeout(regmap, bpd->bus_prot_sta, >>> + return regmap_read_poll_timeout(sta_regmap, bpd->bus_prot_sta, >>> val, (val & sta_mask) == sta_mask, >>> MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT); >>> } >>> @@ -173,7 +188,10 @@ static int scpsys_bus_protect_enable(struct scpsys_domain *pd) >>> if (!bpd->bus_prot_set_clr_mask) >>> break; >>> - ret = scpsys_bus_protect_set(pd, bpd); >>> + if (bpd->flags & BUS_PROT_INVERTED) >>> + ret = scpsys_bus_protect_clear(pd, bpd); >>> + else >>> + ret = scpsys_bus_protect_set(pd, bpd); >>> if (ret) >>> return ret; >>> } >>> @@ -190,7 +208,10 @@ static int scpsys_bus_protect_disable(struct scpsys_domain *pd) >>> if (!bpd->bus_prot_set_clr_mask) >>> continue; >>> - ret = scpsys_bus_protect_clear(pd, bpd); >>> + if (bpd->flags & BUS_PROT_INVERTED) >>> + ret = scpsys_bus_protect_set(pd, bpd); >>> + else >>> + ret = scpsys_bus_protect_clear(pd, bpd); >>> if (ret) >>> return ret; >>> } >>> @@ -377,6 +398,14 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no >>> return ERR_CAST(pd->smi); >>> } >>> + pd->infracfg_nao = syscon_regmap_lookup_by_phandle(node, "mediatek,infracfg-nao"); >> >> If we don't expect infracfg-nao to be present, what's the point about trying to >> get a regmap handle and then failing only if we do expect it to be there? >> >> At this point you can just do... >> >> if (MTK_SCPD_CAPS(pd, MTK_SCPD_HAS_INFRA_NAO)) { >> pd->infracfg_nao = syscon_regmap_lookup_by_phandle(...); >> if (IS_ERR(....)) >> return .... >> } > > Yes! My code looks stupid. Thanks! > Hahaha, no worries! >> >>> + if (IS_ERR(pd->infracfg_nao)) { >>> + if (MTK_SCPD_CAPS(pd, MTK_SCPD_HAS_INFRA_NAO)) >>> + 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 356788263db2..562d4e92ce16 100644 >>> --- a/drivers/soc/mediatek/mtk-pm-domains.h >>> +++ b/drivers/soc/mediatek/mtk-pm-domains.h >>> @@ -11,6 +11,7 @@ >>> /* can't set MTK_SCPD_KEEP_DEFAULT_OFF at the same time */ >>> #define MTK_SCPD_ALWAYS_ON BIT(5) >>> #define MTK_SCPD_EXT_BUCK_ISO BIT(6) >>> +#define MTK_SCPD_HAS_INFRA_NAO BIT(7) >>> #define MTK_SCPD_CAPS(_scpd, _x) ((_scpd)->data->caps & (_x)) >>> #define SPM_VDE_PWR_CON 0x0210 >>> @@ -45,8 +46,10 @@ >>> enum scpsys_bus_prot_flags { >>> BUS_PROT_REG_UPDATE = BIT(1), >>> BUS_PROT_IGNORE_CLR_ACK = BIT(2), >>> - BUS_PROT_COMPONENT_INFRA = BIT(3), >>> - BUS_PROT_COMPONENT_SMI = BIT(4), >>> + BUS_PROT_INVERTED = BIT(3), >> >> I get the reason why you're setting inverted as bit 3, but at that point you can >> just set BUS_PROT_COMPONENT_INFRA to bit 4 from the very beginning, instead of >> using bit 3 for that and then changing them all in a subsequent commit (this one). > > Yes, I was torn between making the commits independent and avoiding this > move later on. I decided for the first. If you prefer I can set it to > the correct bits right from the beginning. > I don't see how setting BUS_PROT_COMPONENT_INFRA to bit(4) from the beginning would add dependencies between commits. The first commit alone will still work, with the added benefit of less noise in this commit. You should, at that point, mention in the commit message that you're setting INFRA to BIT(4) because BIT(3) "is going to be populated in a later commit". That, unless anyone else has strong opinions against. Cheers, Angelo