Hi,
This series aims to solve power-off failures and occasional SMI hang issues that
occur during camera stress tests. The issue arises because, when MTCMOS powers on
or off, signal glitches are sometimes produced. This is fairly normal, but the
software must address it to avoid mistaking the glitch for a transaction signal.
The solutions in these patches can be summarized as follows:
1. Disable the sub-common port after turning off the Larb CG and before turning
off the Larb MTCMOS.
2. Use CLAMP to disable/enable the SMI common port.
3. Implement an AXI reset.
For previous discussion on the direction of the code modifications, please refer
to: https://lore.kernel.org/linux-arm-kernel/[email protected]/
Change in v2
- fix commit title to "pmdomain: mediatek:"
- add dt-binding definition
- remove unused function
yu-chang.lee (3):
pmdomain: mediatek: add smi_larb_reset function when power on
dt-bindings: power: Add mediatek larb definition
pmdomain: mediatek: support smi clamp protection
.../power/mediatek,power-controller.yaml | 4 +
drivers/pmdomain/mediatek/mt8188-pm-domains.h | 69 ++++++-
drivers/pmdomain/mediatek/mtk-pm-domains.c | 168 ++++++++++++++----
drivers/pmdomain/mediatek/mtk-pm-domains.h | 13 ++
4 files changed, 218 insertions(+), 36 deletions(-)
--
2.18.0
Add Smart Multimedia Interface Local Arbiter to mediatek
power domain.
Signed-off-by: yu-chang.lee <[email protected]>
---
.../devicetree/bindings/power/mediatek,power-controller.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
index 8985e2df8a56..228c0dec5253 100644
--- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
+++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
@@ -125,6 +125,10 @@ $defs:
$ref: /schemas/types.yaml#/definitions/phandle
description: phandle to the device containing the SMI register range.
+ mediatek,larb:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: phandle to the device containing the LARB register range.
+
required:
- reg
--
2.18.0
This patch avoid mtcmos power glitch from happening by set and clear
smi larb reset.
Signed-off-by: yu-chang.lee <[email protected]>
---
drivers/pmdomain/mediatek/mt8188-pm-domains.h | 28 +++++++++
drivers/pmdomain/mediatek/mtk-pm-domains.c | 59 +++++++++++++++++++
drivers/pmdomain/mediatek/mtk-pm-domains.h | 12 ++++
3 files changed, 99 insertions(+)
diff --git a/drivers/pmdomain/mediatek/mt8188-pm-domains.h b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
index 06834ab6597c..7bbba4d56a77 100644
--- a/drivers/pmdomain/mediatek/mt8188-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
@@ -573,6 +573,18 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
.pwr_sta2nd_offs = 0x170,
.sram_pdn_bits = BIT(8),
.sram_pdn_ack_bits = BIT(12),
+ .reset_smi = {
+ SMI_RESET_WR(MT8188_SMI_LARB10_RESET,
+ MT8188_SMI_LARB10_RESET_ADDR),
+ SMI_RESET_WR(MT8188_SMI_LARB11A_RESET,
+ MT8188_SMI_LARB11A_RESET_ADDR),
+ SMI_RESET_WR(MT8188_SMI_LARB11C_RESET,
+ MT8188_SMI_LARB11C_RESET_ADDR),
+ SMI_RESET_WR(MT8188_SMI_LARB11B_RESET,
+ MT8188_SMI_LARB11B_RESET_ADDR),
+ SMI_RESET_WR(MT8188_SMI_LARB15_RESET,
+ MT8188_SMI_LARB15_RESET_ADDR),
+ },
.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
},
[MT8188_POWER_DOMAIN_IPE] = {
@@ -583,6 +595,10 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
.pwr_sta2nd_offs = 0x170,
.sram_pdn_bits = BIT(8),
.sram_pdn_ack_bits = BIT(12),
+ .reset_smi = {
+ SMI_RESET_WR(MT8188_SMI_LARB12_RESET,
+ MT8188_SMI_LARB12_RESET_ADDR),
+ },
.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
},
[MT8188_POWER_DOMAIN_CAM_VCORE] = {
@@ -660,6 +676,12 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
.pwr_sta2nd_offs = 0x170,
.sram_pdn_bits = BIT(8),
.sram_pdn_ack_bits = BIT(12),
+ .reset_smi = {
+ SMI_RESET_WR(MT8188_SMI_LARB16A_RESET,
+ MT8188_SMI_LARB16A_RESET_ADDR),
+ SMI_RESET_WR(MT8188_SMI_LARB17A_RESET,
+ MT8188_SMI_LARB17A_RESET_ADDR),
+ },
.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
},
[MT8188_POWER_DOMAIN_CAM_SUBB] = {
@@ -670,6 +692,12 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
.pwr_sta2nd_offs = 0x170,
.sram_pdn_bits = BIT(8),
.sram_pdn_ack_bits = BIT(12),
+ .reset_smi = {
+ SMI_RESET_WR(MT8188_SMI_LARB16B_RESET,
+ MT8188_SMI_LARB16B_RESET_ADDR),
+ SMI_RESET_WR(MT8188_SMI_LARB17B_RESET,
+ MT8188_SMI_LARB17B_RESET_ADDR),
+ },
.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
},
};
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index e274e3315fe7..9ab6fa105c8c 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -48,6 +48,8 @@ struct scpsys_domain {
struct regmap *infracfg_nao;
struct regmap *infracfg;
struct regmap *smi;
+ struct regmap **larb;
+ int num_larb;
struct regulator *supply;
};
@@ -230,6 +232,39 @@ static int scpsys_regulator_disable(struct regulator *supply)
return supply ? regulator_disable(supply) : 0;
}
+static int _scpsys_smi_larb_reset(const struct smi_reset_data bpd,
+ struct regmap *regmap)
+{
+ int ret;
+ u32 mask = bpd.smi_reset_mask;
+
+ if (!mask)
+ return 0;
+
+ ret = regmap_set_bits(regmap, bpd.smi_reset_addr, mask);
+ if (ret)
+ return ret;
+
+ ret = regmap_clear_bits(regmap, bpd.smi_reset_addr, mask);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int scpsys_smi_larb_reset(struct scpsys_domain *pd)
+{
+ int ret, i;
+
+ for (i = 0; i < pd->num_larb; i++) {
+ ret = _scpsys_smi_larb_reset(pd->data->reset_smi[i], pd->larb[i]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
static int scpsys_power_on(struct generic_pm_domain *genpd)
{
struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
@@ -279,6 +314,10 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
if (ret < 0)
goto err_disable_subsys_clks;
+ ret = scpsys_smi_larb_reset(pd);
+ if (ret < 0)
+ goto err_disable_subsys_clks;
+
ret = scpsys_bus_protect_disable(pd);
if (ret < 0)
goto err_disable_sram;
@@ -355,6 +394,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
struct scpsys_domain *pd;
struct device_node *root_node = scpsys->dev->of_node;
struct device_node *smi_node;
+ struct device_node *larb_node;
struct property *prop;
const char *clk_name;
int i, ret, num_clks;
@@ -418,6 +458,25 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
return ERR_CAST(pd->smi);
}
+ pd->num_larb = of_count_phandle_with_args(node, "mediatek,larb", NULL);
+ if (pd->num_larb > 0) {
+ pd->larb = devm_kcalloc(scpsys->dev, pd->num_larb, sizeof(*pd->larb), GFP_KERNEL);
+ if (!pd->larb)
+ return ERR_PTR(-ENOMEM);
+
+ for (i = 0; i < pd->num_larb; i++) {
+ larb_node = of_parse_phandle(node, "mediatek,larb", i);
+ if (!larb_node)
+ return ERR_PTR(-EINVAL);
+
+ pd->larb[i] = device_node_to_regmap(larb_node);
+ if (IS_ERR(pd->larb[i]))
+ return ERR_CAST(pd->larb[i]);
+ }
+ } else {
+ pd->num_larb = 0;
+ }
+
if (MTK_SCPD_CAPS(pd, MTK_SCPD_HAS_INFRA_NAO)) {
pd->infracfg_nao = syscon_regmap_lookup_by_phandle(node, "mediatek,infracfg-nao");
if (IS_ERR(pd->infracfg_nao))
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.h b/drivers/pmdomain/mediatek/mtk-pm-domains.h
index aaba5e6b0536..31c2a1bb500f 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.h
@@ -43,6 +43,7 @@
#define PWR_STATUS_USB BIT(25)
#define SPM_MAX_BUS_PROT_DATA 6
+#define SPM_MAX_SMI_RESET_DATA 6
enum scpsys_bus_prot_flags {
BUS_PROT_REG_UPDATE = BIT(1),
@@ -79,6 +80,16 @@ enum scpsys_bus_prot_flags {
INFRA_TOPAXI_PROTECTEN, \
INFRA_TOPAXI_PROTECTSTA1)
+#define SMI_RESET_WR(_mask, _addr) { \
+ .smi_reset_mask = (_mask), \
+ .smi_reset_addr = _addr, \
+ }
+
+struct smi_reset_data {
+ u32 smi_reset_mask;
+ u32 smi_reset_addr;
+};
+
struct scpsys_bus_prot_data {
u32 bus_prot_set_clr_mask;
u32 bus_prot_set;
@@ -110,6 +121,7 @@ struct scpsys_domain_data {
u32 ext_buck_iso_mask;
u16 caps;
const struct scpsys_bus_prot_data bp_cfg[SPM_MAX_BUS_PROT_DATA];
+ const struct smi_reset_data reset_smi[SPM_MAX_SMI_RESET_DATA];
int pwr_sta_offs;
int pwr_sta2nd_offs;
};
--
2.18.0
In order to avoid power glitch, this patch use smi clamp
to disable/enable smi common port.
Signed-off-by: yu-chang.lee <[email protected]>
---
drivers/pmdomain/mediatek/mt8188-pm-domains.h | 41 ++++++-
drivers/pmdomain/mediatek/mtk-pm-domains.c | 109 +++++++++++++-----
drivers/pmdomain/mediatek/mtk-pm-domains.h | 1 +
3 files changed, 115 insertions(+), 36 deletions(-)
diff --git a/drivers/pmdomain/mediatek/mt8188-pm-domains.h b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
index 7bbba4d56a77..39f057dca92c 100644
--- a/drivers/pmdomain/mediatek/mt8188-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
@@ -573,6 +573,18 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
.pwr_sta2nd_offs = 0x170,
.sram_pdn_bits = BIT(8),
.sram_pdn_ack_bits = BIT(12),
+ .bp_cfg = {
+ BUS_PROT_WR(SMI,
+ MT8188_SMI_COMMON_SMI_CLAMP_DIP_TO_VDO0,
+ MT8188_SMI_COMMON_CLAMP_EN_SET,
+ MT8188_SMI_COMMON_CLAMP_EN_CLR,
+ MT8188_SMI_COMMON_CLAMP_EN_STA),
+ BUS_PROT_WR(SMI,
+ MT8188_SMI_COMMON_SMI_CLAMP_DIP_TO_VPP1,
+ MT8188_SMI_COMMON_CLAMP_EN_SET,
+ MT8188_SMI_COMMON_CLAMP_EN_CLR,
+ MT8188_SMI_COMMON_CLAMP_EN_STA),
+ },
.reset_smi = {
SMI_RESET_WR(MT8188_SMI_LARB10_RESET,
MT8188_SMI_LARB10_RESET_ADDR),
@@ -585,7 +597,7 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
SMI_RESET_WR(MT8188_SMI_LARB15_RESET,
MT8188_SMI_LARB15_RESET_ADDR),
},
- .caps = MTK_SCPD_KEEP_DEFAULT_OFF,
+ .caps = MTK_SCPD_KEEP_DEFAULT_OFF | MTK_SCPD_CLAMP_PROTECTION,
},
[MT8188_POWER_DOMAIN_IPE] = {
.name = "ipe",
@@ -595,11 +607,18 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
.pwr_sta2nd_offs = 0x170,
.sram_pdn_bits = BIT(8),
.sram_pdn_ack_bits = BIT(12),
+ .bp_cfg = {
+ BUS_PROT_WR(SMI,
+ MT8188_SMI_COMMON_SMI_CLAMP_IPE_TO_VPP1,
+ MT8188_SMI_COMMON_CLAMP_EN_SET,
+ MT8188_SMI_COMMON_CLAMP_EN_CLR,
+ MT8188_SMI_COMMON_CLAMP_EN_STA),
+ },
.reset_smi = {
SMI_RESET_WR(MT8188_SMI_LARB12_RESET,
MT8188_SMI_LARB12_RESET_ADDR),
},
- .caps = MTK_SCPD_KEEP_DEFAULT_OFF,
+ .caps = MTK_SCPD_KEEP_DEFAULT_OFF | MTK_SCPD_CLAMP_PROTECTION,
},
[MT8188_POWER_DOMAIN_CAM_VCORE] = {
.name = "cam_vcore",
@@ -676,13 +695,20 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
.pwr_sta2nd_offs = 0x170,
.sram_pdn_bits = BIT(8),
.sram_pdn_ack_bits = BIT(12),
+ .bp_cfg = {
+ BUS_PROT_WR(SMI,
+ MT8188_SMI_COMMON_SMI_CLAMP_IPE_TO_VPP1,
+ MT8188_SMI_COMMON_CLAMP_EN_SET,
+ MT8188_SMI_COMMON_CLAMP_EN_CLR,
+ MT8188_SMI_COMMON_CLAMP_EN_STA),
+ },
.reset_smi = {
SMI_RESET_WR(MT8188_SMI_LARB16A_RESET,
MT8188_SMI_LARB16A_RESET_ADDR),
SMI_RESET_WR(MT8188_SMI_LARB17A_RESET,
MT8188_SMI_LARB17A_RESET_ADDR),
},
- .caps = MTK_SCPD_KEEP_DEFAULT_OFF,
+ .caps = MTK_SCPD_KEEP_DEFAULT_OFF | MTK_SCPD_CLAMP_PROTECTION,
},
[MT8188_POWER_DOMAIN_CAM_SUBB] = {
.name = "cam_subb",
@@ -692,13 +718,20 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
.pwr_sta2nd_offs = 0x170,
.sram_pdn_bits = BIT(8),
.sram_pdn_ack_bits = BIT(12),
+ .bp_cfg = {
+ BUS_PROT_WR(SMI,
+ MT8188_SMI_COMMON_SMI_CLAMP_CAM_SUBB_TO_VDO0,
+ MT8188_SMI_COMMON_CLAMP_EN_SET,
+ MT8188_SMI_COMMON_CLAMP_EN_CLR,
+ MT8188_SMI_COMMON_CLAMP_EN_STA),
+ },
.reset_smi = {
SMI_RESET_WR(MT8188_SMI_LARB16B_RESET,
MT8188_SMI_LARB16B_RESET_ADDR),
SMI_RESET_WR(MT8188_SMI_LARB17B_RESET,
MT8188_SMI_LARB17B_RESET_ADDR),
},
- .caps = MTK_SCPD_KEEP_DEFAULT_OFF,
+ .caps = MTK_SCPD_KEEP_DEFAULT_OFF | MTK_SCPD_CLAMP_PROTECTION,
},
};
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index 9ab6fa105c8c..2a86ff4bf23e 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -47,9 +47,10 @@ struct scpsys_domain {
struct clk_bulk_data *subsys_clks;
struct regmap *infracfg_nao;
struct regmap *infracfg;
- struct regmap *smi;
+ struct regmap **smi;
struct regmap **larb;
int num_larb;
+ int num_smi;
struct regulator *supply;
};
@@ -122,29 +123,19 @@ static int scpsys_sram_disable(struct scpsys_domain *pd)
MTK_POLL_TIMEOUT);
}
-static struct regmap *scpsys_bus_protect_get_regmap(struct scpsys_domain *pd,
- const struct scpsys_bus_prot_data *bpd)
-{
- if (bpd->flags & BUS_PROT_COMPONENT_SMI)
- return pd->smi;
- else
- 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);
+ return pd->infracfg;
}
static int scpsys_bus_protect_clear(struct scpsys_domain *pd,
- const struct scpsys_bus_prot_data *bpd)
+ const struct scpsys_bus_prot_data *bpd,
+ struct regmap *sta_regmap, struct regmap *regmap)
{
- struct regmap *sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd);
- struct regmap *regmap = scpsys_bus_protect_get_regmap(pd, bpd);
u32 sta_mask = bpd->bus_prot_sta_mask;
u32 expected_ack;
u32 val;
@@ -165,10 +156,9 @@ static int scpsys_bus_protect_clear(struct scpsys_domain *pd,
}
static int scpsys_bus_protect_set(struct scpsys_domain *pd,
- const struct scpsys_bus_prot_data *bpd)
+ const struct scpsys_bus_prot_data *bpd,
+ struct regmap *sta_regmap, struct regmap *regmap)
{
- struct regmap *sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd);
- struct regmap *regmap = scpsys_bus_protect_get_regmap(pd, bpd);
u32 sta_mask = bpd->bus_prot_sta_mask;
u32 val;
@@ -182,19 +172,32 @@ static int scpsys_bus_protect_set(struct scpsys_domain *pd,
MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
}
-static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
+static int scpsys_clamp_bus_protection_enable(struct scpsys_domain *pd, bool is_smi)
{
+ int smi_count = 0;
+
for (int i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
const struct scpsys_bus_prot_data *bpd = &pd->data->bp_cfg[i];
+ struct regmap *sta_regmap, *regmap;
+ bool is_smi = bpd->flags & BUS_PROT_COMPONENT_SMI;
int ret;
if (!bpd->bus_prot_set_clr_mask)
break;
+ if (is_smi) {
+ sta_regmap = pd->smi[smi_count];
+ regmap = pd->smi[smi_count];
+ smi_count++;
+ } else {
+ sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd);
+ regmap = pd->infracfg;
+ }
+
if (bpd->flags & BUS_PROT_INVERTED)
- ret = scpsys_bus_protect_clear(pd, bpd);
+ ret = scpsys_bus_protect_clear(pd, bpd, sta_regmap, regmap);
else
- ret = scpsys_bus_protect_set(pd, bpd);
+ ret = scpsys_bus_protect_set(pd, bpd, sta_regmap, regmap);
if (ret)
return ret;
}
@@ -202,19 +205,32 @@ static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
return 0;
}
-static int scpsys_bus_protect_disable(struct scpsys_domain *pd)
+static int scpsys_clamp_bus_protection_disable(struct scpsys_domain *pd, bool is_smi)
{
+ int smi_count = pd->num_smi - 1;
+
for (int i = SPM_MAX_BUS_PROT_DATA - 1; i >= 0; i--) {
const struct scpsys_bus_prot_data *bpd = &pd->data->bp_cfg[i];
+ struct regmap *sta_regmap, *regmap;
+ bool is_smi = bpd->flags & BUS_PROT_COMPONENT_SMI;
int ret;
if (!bpd->bus_prot_set_clr_mask)
continue;
+ if (is_smi) {
+ sta_regmap = pd->smi[smi_count];
+ regmap = pd->smi[smi_count];
+ smi_count--;
+ } else {
+ sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd);
+ regmap = pd->infracfg;
+ }
+
if (bpd->flags & BUS_PROT_INVERTED)
- ret = scpsys_bus_protect_set(pd, bpd);
+ ret = scpsys_bus_protect_set(pd, bpd, sta_regmap, regmap);
else
- ret = scpsys_bus_protect_clear(pd, bpd);
+ ret = scpsys_bus_protect_clear(pd, bpd, sta_regmap, regmap);
if (ret)
return ret;
}
@@ -272,6 +288,12 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
bool tmp;
int ret;
+ if (MTK_SCPD_CAPS(pd, MTK_SCPD_CLAMP_PROTECTION)) {
+ ret = scpsys_clamp_bus_protection_enable(pd, true);
+ if (ret)
+ return ret;
+ }
+
ret = scpsys_regulator_enable(pd->supply);
if (ret)
return ret;
@@ -318,7 +340,13 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
if (ret < 0)
goto err_disable_subsys_clks;
- ret = scpsys_bus_protect_disable(pd);
+ if (MTK_SCPD_CAPS(pd, MTK_SCPD_CLAMP_PROTECTION)) {
+ ret = scpsys_clamp_bus_protection_disable(pd, true);
+ if (ret)
+ return ret;
+ }
+
+ ret = scpsys_clamp_bus_protection_disable(pd, false);
if (ret < 0)
goto err_disable_sram;
@@ -332,7 +360,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
return 0;
err_enable_bus_protect:
- scpsys_bus_protect_enable(pd);
+ scpsys_clamp_bus_protection_enable(pd, false);
err_disable_sram:
scpsys_sram_disable(pd);
err_disable_subsys_clks:
@@ -353,7 +381,13 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
bool tmp;
int ret;
- ret = scpsys_bus_protect_enable(pd);
+ if (MTK_SCPD_CAPS(pd, MTK_SCPD_CLAMP_PROTECTION)) {
+ ret = scpsys_clamp_bus_protection_enable(pd, true);
+ if (ret)
+ return ret;
+ }
+
+ ret = scpsys_clamp_bus_protection_enable(pd, false);
if (ret < 0)
return ret;
@@ -450,12 +484,23 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
if (IS_ERR(pd->infracfg))
return ERR_CAST(pd->infracfg);
- smi_node = of_parse_phandle(node, "mediatek,smi", 0);
- if (smi_node) {
- pd->smi = device_node_to_regmap(smi_node);
- of_node_put(smi_node);
- if (IS_ERR(pd->smi))
- return ERR_CAST(pd->smi);
+ pd->num_smi = of_count_phandle_with_args(node, "mediatek,smi", NULL);
+ if (pd->num_smi > 0) {
+ pd->smi = devm_kcalloc(scpsys->dev, pd->num_smi, sizeof(*pd->smi), GFP_KERNEL);
+ if (!pd->smi)
+ return ERR_PTR(-ENOMEM);
+
+ for (i = 0; i < pd->num_smi; i++) {
+ smi_node = of_parse_phandle(node, "mediatek,smi", i);
+ if (!smi_node)
+ return ERR_PTR(-EINVAL);
+
+ pd->smi[i] = device_node_to_regmap(smi_node);
+ if (IS_ERR(pd->smi[i]))
+ return ERR_CAST(pd->smi[i]);
+ }
+ } else {
+ pd->num_smi = 0;
}
pd->num_larb = of_count_phandle_with_args(node, "mediatek,larb", NULL);
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.h b/drivers/pmdomain/mediatek/mtk-pm-domains.h
index 31c2a1bb500f..e0eb7214719e 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.h
@@ -13,6 +13,7 @@
#define MTK_SCPD_EXT_BUCK_ISO BIT(6)
#define MTK_SCPD_HAS_INFRA_NAO BIT(7)
#define MTK_SCPD_STRICT_BUS_PROTECTION BIT(8)
+#define MTK_SCPD_CLAMP_PROTECTION BIT(9)
#define MTK_SCPD_CAPS(_scpd, _x) ((_scpd)->data->caps & (_x))
#define SPM_VDE_PWR_CON 0x0210
--
2.18.0
On 27/03/2024 06:57, yu-chang.lee wrote:
> Add Smart Multimedia Interface Local Arbiter to mediatek
> power domain.
>
> Signed-off-by: yu-chang.lee <[email protected]>
> ---
> .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> index 8985e2df8a56..228c0dec5253 100644
> --- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> @@ -125,6 +125,10 @@ $defs:
> $ref: /schemas/types.yaml#/definitions/phandle
> description: phandle to the device containing the SMI register range.
>
> + mediatek,larb:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: phandle to the device containing the LARB register range.
Why do you need it?
Plus I also see mediatek,larbs and mediatek,larb-id... so now we have
third one similar.
Best regards,
Krzysztof
On Wed, 27 Mar 2024 13:57:31 +0800, yu-chang.lee wrote:
> Add Smart Multimedia Interface Local Arbiter to mediatek
> power domain.
>
> Signed-off-by: yu-chang.lee <[email protected]>
> ---
> .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: [error] syntax error: expected <block end>, but found '<block mapping start>' (syntax)
/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:129:9: [warning] wrong indentation: expected 7 but found 8 (indentation)
dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/power/mediatek,power-controller.example.dts'
Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: did not find expected key
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/power/mediatek,power-controller.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml:
while parsing a block mapping
in "<unicode string>", line 64, column 5
did not find expected key
in "<unicode string>", line 128, column 6
/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: did not find expected key
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On 27/03/2024 09:39, Krzysztof Kozlowski wrote:
> On 27/03/2024 06:57, yu-chang.lee wrote:
>> Add Smart Multimedia Interface Local Arbiter to mediatek
>> power domain.
>>
>> Signed-off-by: yu-chang.lee <[email protected]>
>> ---
>> .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>> index 8985e2df8a56..228c0dec5253 100644
>> --- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>> @@ -125,6 +125,10 @@ $defs:
>> $ref: /schemas/types.yaml#/definitions/phandle
>> description: phandle to the device containing the SMI register range.
>>
>> + mediatek,larb:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description: phandle to the device containing the LARB register range.
>
> Why do you need it?
>
> Plus I also see mediatek,larbs and mediatek,larb-id... so now we have
> third one similar.
.. and not even tested!
Best regards,
Krzysztof
On Wed, 2024-03-27 at 10:23 +0100, Krzysztof Kozlowski wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 27/03/2024 09:39, Krzysztof Kozlowski wrote:
> > On 27/03/2024 06:57, yu-chang.lee wrote:
> >> Add Smart Multimedia Interface Local Arbiter to mediatek
> >> power domain.
> >>
> >> Signed-off-by: yu-chang.lee <[email protected]>
> >> ---
> >> .../devicetree/bindings/power/mediatek,power-controller.yaml | 4
> ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git
> a/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> b/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> >> index 8985e2df8a56..228c0dec5253 100644
> >> --- a/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> >> +++ b/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> >> @@ -125,6 +125,10 @@ $defs:
> >> $ref: /schemas/types.yaml#/definitions/phandle
> >> description: phandle to the device containing the SMI
> register range.
> >>
> >> + mediatek,larb:
> >> + $ref: /schemas/types.yaml#/definitions/phandle
> >> + description: phandle to the device containing the LARB
> register range.
> >
> > Why do you need it?
> >
> > Plus I also see mediatek,larbs and mediatek,larb-id... so now we
> have
> > third one similar.
>
> ... and not even tested!
>
> Best regards,
> Krzysztof
>
Hi,
I will double check the format of yaml for the next version, sorry for
inconvenience. But I did test it on mt8188 chromebook, the reason why
power domain need larb node is that when mtcmos power on, signal glitch
may produce. Power domain driver must reset larb when this happen to
prevent dummy transaction on bus. That why I need larb node in dts.
Best Regards,
Yu-chang
On 27/03/2024 10:34, Yu-chang Lee (李禹璋) wrote:
> On Wed, 2024-03-27 at 10:23 +0100, Krzysztof Kozlowski wrote:
>>
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>> On 27/03/2024 09:39, Krzysztof Kozlowski wrote:
>>> On 27/03/2024 06:57, yu-chang.lee wrote:
>>>> Add Smart Multimedia Interface Local Arbiter to mediatek
>>>> power domain.
>>>>
>>>> Signed-off-by: yu-chang.lee <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/power/mediatek,power-controller.yaml | 4
>> ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git
>> a/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>> b/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>>> index 8985e2df8a56..228c0dec5253 100644
>>>> --- a/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>>> @@ -125,6 +125,10 @@ $defs:
>>>> $ref: /schemas/types.yaml#/definitions/phandle
>>>> description: phandle to the device containing the SMI
>> register range.
>>>>
>>>> + mediatek,larb:
>>>> + $ref: /schemas/types.yaml#/definitions/phandle
>>>> + description: phandle to the device containing the LARB
>> register range.
>>>
>>> Why do you need it?
>>>
>>> Plus I also see mediatek,larbs and mediatek,larb-id... so now we
>> have
>>> third one similar.
>>
>> ... and not even tested!
>>
>> Best regards,
>> Krzysztof
>>
> Hi,
>
> I will double check the format of yaml for the next version, sorry for
> inconvenience. But I did test it on mt8188 chromebook, the reason why
How do you test a binding on chromebook?
> power domain need larb node is that when mtcmos power on, signal glitch
> may produce. Power domain driver must reset larb when this happen to
> prevent dummy transaction on bus. That why I need larb node in dts.
No one talks here about larb node...
Best regards,
Krzysztof
On Wed, 2024-03-27 at 09:39 +0100, Krzysztof Kozlowski wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 27/03/2024 06:57, yu-chang.lee wrote:
> > Add Smart Multimedia Interface Local Arbiter to mediatek
> > power domain.
> >
> > Signed-off-by: yu-chang.lee <[email protected]>
> > ---
> > .../devicetree/bindings/power/mediatek,power-controller.yaml | 4
> ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git
> a/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> b/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> > index 8985e2df8a56..228c0dec5253 100644
> > --- a/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> > +++ b/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> > @@ -125,6 +125,10 @@ $defs:
> > $ref: /schemas/types.yaml#/definitions/phandle
> > description: phandle to the device containing the SMI
> register range.
> >
> > + mediatek,larb:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: phandle to the device containing the LARB
> register range.
>
> Why do you need it?
>
> Plus I also see mediatek,larbs and mediatek,larb-id... so now we have
> third one similar.
>
MM driver used "mediatek,larbs" for it larb node.
Power domain driver used "mediatek,larb".
"mediatek,larb-id" is for larb in dts.
The naming is no related to each other.
Best Regards,
Yu-chang.
> Best regards,
> Krzysztof
>
On 27/03/2024 11:01, Yu-chang Lee (李禹璋) wrote:
> On Wed, 2024-03-27 at 09:39 +0100, Krzysztof Kozlowski wrote:
>>
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>> On 27/03/2024 06:57, yu-chang.lee wrote:
>>> Add Smart Multimedia Interface Local Arbiter to mediatek
>>> power domain.
>>>
>>> Signed-off-by: yu-chang.lee <[email protected]>
>>> ---
>>> .../devicetree/bindings/power/mediatek,power-controller.yaml | 4
>> ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git
>> a/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>> b/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>> index 8985e2df8a56..228c0dec5253 100644
>>> --- a/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>> @@ -125,6 +125,10 @@ $defs:
>>> $ref: /schemas/types.yaml#/definitions/phandle
>>> description: phandle to the device containing the SMI
>> register range.
>>>
>>> + mediatek,larb:
>>> + $ref: /schemas/types.yaml#/definitions/phandle
>>> + description: phandle to the device containing the LARB
>> register range.
>>
>> Why do you need it?
>>
>> Plus I also see mediatek,larbs and mediatek,larb-id... so now we have
>> third one similar.
>>
> MM driver used "mediatek,larbs" for it larb node.
> Power domain driver used "mediatek,larb".
> "mediatek,larb-id" is for larb in dts.
>
> The naming is no related to each other.
Then it is just confusing.
Best regards,
Krzysztof
On Wed, 2024-03-27 at 10:59 +0100, Krzysztof Kozlowski wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 27/03/2024 10:34, Yu-chang Lee (李禹璋) wrote:
> > On Wed, 2024-03-27 at 10:23 +0100, Krzysztof Kozlowski wrote:
> >>
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >> On 27/03/2024 09:39, Krzysztof Kozlowski wrote:
> >>> On 27/03/2024 06:57, yu-chang.lee wrote:
> >>>> Add Smart Multimedia Interface Local Arbiter to mediatek
> >>>> power domain.
> >>>>
> >>>> Signed-off-by: yu-chang.lee <[email protected]>
> >>>> ---
> >>>> .../devicetree/bindings/power/mediatek,power-controller.yaml |
> 4
> >> ++++
> >>>> 1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git
> >> a/Documentation/devicetree/bindings/power/mediatek,power-
> >> controller.yaml
> >> b/Documentation/devicetree/bindings/power/mediatek,power-
> >> controller.yaml
> >>>> index 8985e2df8a56..228c0dec5253 100644
> >>>> --- a/Documentation/devicetree/bindings/power/mediatek,power-
> >> controller.yaml
> >>>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-
> >> controller.yaml
> >>>> @@ -125,6 +125,10 @@ $defs:
> >>>> $ref: /schemas/types.yaml#/definitions/phandle
> >>>> description: phandle to the device containing the SMI
> >> register range.
> >>>>
> >>>> + mediatek,larb:
> >>>> + $ref: /schemas/types.yaml#/definitions/phandle
> >>>> + description: phandle to the device containing the LARB
> >> register range.
> >>>
> >>> Why do you need it?
> >>>
> >>> Plus I also see mediatek,larbs and mediatek,larb-id... so now we
> >> have
> >>> third one similar.
> >>
> >> ... and not even tested!
> >>
> >> Best regards,
> >> Krzysztof
> >>
> > Hi,
> >
> > I will double check the format of yaml for the next version, sorry
> for
> > inconvenience. But I did test it on mt8188 chromebook, the reason
> why
>
> How do you test a binding on chromebook?
>
> > power domain need larb node is that when mtcmos power on, signal
> glitch
> > may produce. Power domain driver must reset larb when this happen
> to
> > prevent dummy transaction on bus. That why I need larb node in dts.
>
> No one talks here about larb node...
Sorry, May you elaborate on what information I need to provide to you
or it is just a syntax problem I need to fix?
Thanks
Best Regards,
Yu-chang
>
> Best regards,
> Krzysztof
>
On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
>>>>
>>> Hi,
>>>
>>> I will double check the format of yaml for the next version, sorry
>> for
>>> inconvenience. But I did test it on mt8188 chromebook, the reason
>> why
>>
>> How do you test a binding on chromebook?
>>
>>> power domain need larb node is that when mtcmos power on, signal
>> glitch
>>> may produce. Power domain driver must reset larb when this happen
>> to
>>> prevent dummy transaction on bus. That why I need larb node in dts.
>>
>> No one talks here about larb node...
>
> Sorry, May you elaborate on what information I need to provide to you
> or it is just a syntax problem I need to fix?
Please explain the purpose of this property (how is it going to be used
by drivers) and what does it represent.
Best regards,
Krzysztof
On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
> >>>>
> >>> Hi,
> >>>
> >>> I will double check the format of yaml for the next version,
> sorry
> >> for
> >>> inconvenience. But I did test it on mt8188 chromebook, the reason
> >> why
> >>
> >> How do you test a binding on chromebook?
> >>
> >>> power domain need larb node is that when mtcmos power on, signal
> >> glitch
> >>> may produce. Power domain driver must reset larb when this happen
> >> to
> >>> prevent dummy transaction on bus. That why I need larb node in
> dts.
> >>
> >> No one talks here about larb node...
> >
> > Sorry, May you elaborate on what information I need to provide to
> you
> > or it is just a syntax problem I need to fix?
>
> Please explain the purpose of this property (how is it going to be
> used by drivers)and what does it represent.
>
It represent SMI LARB(Local ARBitration). In power domain driver when
power on power domain, It need to reset LARB to prevent potential power
glitch which may cause dummy transaction on bus. Without taking care of
this issue it often leads to camera hang in stress test.
Best Regards,
Yu-chang
On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote:
> On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote:
>>
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>> On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
>>>>>>
>>>>> Hi,
>>>>>
>>>>> I will double check the format of yaml for the next version,
>> sorry
>>>> for
>>>>> inconvenience. But I did test it on mt8188 chromebook, the reason
>>>> why
>>>>
>>>> How do you test a binding on chromebook?
>>>>
>>>>> power domain need larb node is that when mtcmos power on, signal
>>>> glitch
>>>>> may produce. Power domain driver must reset larb when this happen
>>>> to
>>>>> prevent dummy transaction on bus. That why I need larb node in
>> dts.
>>>>
>>>> No one talks here about larb node...
>>>
>>> Sorry, May you elaborate on what information I need to provide to
>> you
>>> or it is just a syntax problem I need to fix?
>>
>> Please explain the purpose of this property (how is it going to be
>> used by drivers)and what does it represent.
>>
>
> It represent SMI LARB(Local ARBitration). In power domain driver when
> power on power domain, It need to reset LARB to prevent potential power
> glitch which may cause dummy transaction on bus. Without taking care of
> this issue it often leads to camera hang in stress test.
That sounds rather like missing resets... or something else connecting
these devices. Maybe the explanation is just imprecise...
Best regards,
Krzysztof
Hello Yu-chang Lee,
SMI LARB must have a power domain, according to "mediatek,smi-larb.yaml"
Now you try to create a link from power domain to larb.
Here is my understanding: when you enable/disable power domain, the
larb linked to this power domain may have an issue. Then you want to
retrieve de LARB linked to the power domain though the dts to manage
the LARB. IMHO, using the dts to have this information into the power
driver isn't necessary and may introduce some bugs if the LARB node
and power node in the DTS aren't aligned.
It seems not implemented today but during the LARB probe, it should
"subscribe" to the linked power domain. Then, when the power domain
status is changing, it is able to "notify" (callback) the LARB, then
implement the good stuff to handle this power domain status change
into LARB driver.
Regards,
Alexandre
On Wed, Mar 27, 2024 at 12:04 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote:
> > On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote:
> >>
> >> External email : Please do not click links or open attachments until
> >> you have verified the sender or the content.
> >> On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
> >>>>>>
> >>>>> Hi,
> >>>>>
> >>>>> I will double check the format of yaml for the next version,
> >> sorry
> >>>> for
> >>>>> inconvenience. But I did test it on mt8188 chromebook, the reason
> >>>> why
> >>>>
> >>>> How do you test a binding on chromebook?
> >>>>
> >>>>> power domain need larb node is that when mtcmos power on, signal
> >>>> glitch
> >>>>> may produce. Power domain driver must reset larb when this happen
> >>>> to
> >>>>> prevent dummy transaction on bus. That why I need larb node in
> >> dts.
> >>>>
> >>>> No one talks here about larb node...
> >>>
> >>> Sorry, May you elaborate on what information I need to provide to
> >> you
> >>> or it is just a syntax problem I need to fix?
> >>
> >> Please explain the purpose of this property (how is it going to be
> >> used by drivers)and what does it represent.
> >>
> >
> > It represent SMI LARB(Local ARBitration). In power domain driver when
> > power on power domain, It need to reset LARB to prevent potential power
> > glitch which may cause dummy transaction on bus. Without taking care of
> > this issue it often leads to camera hang in stress test.
>
> That sounds rather like missing resets... or something else connecting
> these devices. Maybe the explanation is just imprecise...
>
> Best regards,
> Krzysztof
>
>
On Wed, 2024-03-27 at 12:55 +0100, Alexandre Mergnat wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Hello Yu-chang Lee,
>
> SMI LARB must have a power domain, according to "mediatek,smi-
> larb.yaml"
> Now you try to create a link from power domain to larb.
>
> Here is my understanding: when you enable/disable power domain, the
> larb linked to this power domain may have an issue. Then you want to
> retrieve de LARB linked to the power domain though the dts to manage
> the LARB.
Yes, this is what I am trying to do.
> IMHO, using the dts to have this information into the power
> driver isn't necessary and may introduce some bugs if the LARB node
> and power node in the DTS aren't aligned.
>
> It seems not implemented today but during the LARB probe, it should
> "subscribe" to the linked power domain. Then, when the power domain
> status is changing, it is able to "notify" (callback) the LARB, then
> implement the good stuff to handle this power domain status change
> into LARB driver.
>
The problem with this method and why "smi clamp" is in power domain
driver is that our HW designer gave us a programming guide strictly
states the sequence of what we need to do to power on/off power domain.
Using callback, this sequence is no longer guaranteed and the side
effect is unknown...
So I would like to stick with adding larbs just like smi into power
domain node.
> Regards,
> Alexandre
Best Regards,
Yu-chang
>
> On Wed, Mar 27, 2024 at 12:04 PM Krzysztof Kozlowski
> <[email protected]> wrote:
> >
> > On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote:
> > > On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote:
> > >>
> > >> External email : Please do not click links or open attachments
> until
> > >> you have verified the sender or the content.
> > >> On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
> > >>>>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> I will double check the format of yaml for the next version,
> > >> sorry
> > >>>> for
> > >>>>> inconvenience. But I did test it on mt8188 chromebook, the
> reason
> > >>>> why
> > >>>>
> > >>>> How do you test a binding on chromebook?
> > >>>>
> > >>>>> power domain need larb node is that when mtcmos power on,
> signal
> > >>>> glitch
> > >>>>> may produce. Power domain driver must reset larb when this
> happen
> > >>>> to
> > >>>>> prevent dummy transaction on bus. That why I need larb node
> in
> > >> dts.
> > >>>>
> > >>>> No one talks here about larb node...
> > >>>
> > >>> Sorry, May you elaborate on what information I need to provide
> to
> > >> you
> > >>> or it is just a syntax problem I need to fix?
> > >>
> > >> Please explain the purpose of this property (how is it going to
> be
> > >> used by drivers)and what does it represent.
> > >>
> > >
> > > It represent SMI LARB(Local ARBitration). In power domain driver
> when
> > > power on power domain, It need to reset LARB to prevent potential
> power
> > > glitch which may cause dummy transaction on bus. Without taking
> care of
> > > this issue it often leads to camera hang in stress test.
> >
> > That sounds rather like missing resets... or something else
> connecting
> > these devices. Maybe the explanation is just imprecise...
> >
> > Best regards,
> > Krzysztof
> >
> >
On Wed, 2024-03-27 at 12:04 +0100, Krzysztof Kozlowski wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote:
> > On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote:
> >>
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >> On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
> >>>>>>
> >>>>> Hi,
> >>>>>
> >>>>> I will double check the format of yaml for the next version,
> >> sorry
> >>>> for
> >>>>> inconvenience. But I did test it on mt8188 chromebook, the
> reason
> >>>> why
> >>>>
> >>>> How do you test a binding on chromebook?
> >>>>
> >>>>> power domain need larb node is that when mtcmos power on,
> signal
> >>>> glitch
> >>>>> may produce. Power domain driver must reset larb when this
> happen
> >>>> to
> >>>>> prevent dummy transaction on bus. That why I need larb node in
> >> dts.
> >>>>
> >>>> No one talks here about larb node...
> >>>
> >>> Sorry, May you elaborate on what information I need to provide to
> >> you
> >>> or it is just a syntax problem I need to fix?
> >>
> >> Please explain the purpose of this property (how is it going to be
> >> used by drivers)and what does it represent.
> >>
> >
> > It represent SMI LARB(Local ARBitration). In power domain driver
> when
> > power on power domain, It need to reset LARB to prevent potential
> power
> > glitch which may cause dummy transaction on bus. Without taking
> care of
> > this issue it often leads to camera hang in stress test.
>
> That sounds rather like missing resets... or something else
> connecting
> these devices. Maybe the explanation is just imprecise...
>
Maybe the term "reset" is misleading here. What power domain driver
trying to do is "set and then clr" the smi larb to clear potential
glitch signal that is already in there. And this step is strongly
related to power domain onf that why I want to add it in to power
domain node without depending larb driver to do the work to prevent
this setting missing.
> Best regards,
> Krzysztof
Best Regards,
yu-chang
>
On 28/03/2024 07:06, Yu-chang Lee (李禹璋) wrote:
> On Wed, 2024-03-27 at 12:55 +0100, Alexandre Mergnat wrote:
>>
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>> Hello Yu-chang Lee,
>>
>> SMI LARB must have a power domain, according to "mediatek,smi-
>> larb.yaml"
>> Now you try to create a link from power domain to larb.
>>
>> Here is my understanding: when you enable/disable power domain, the
>> larb linked to this power domain may have an issue. Then you want to
>> retrieve de LARB linked to the power domain though the dts to manage
>> the LARB.
>
> Yes, this is what I am trying to do.
>
>> IMHO, using the dts to have this information into the power
>> driver isn't necessary and may introduce some bugs if the LARB node
>> and power node in the DTS aren't aligned.
>>
>> It seems not implemented today but during the LARB probe, it should
>> "subscribe" to the linked power domain. Then, when the power domain
>> status is changing, it is able to "notify" (callback) the LARB, then
>> implement the good stuff to handle this power domain status change
>> into LARB driver.
>>
>
> The problem with this method and why "smi clamp" is in power domain
> driver is that our HW designer gave us a programming guide strictly
> states the sequence of what we need to do to power on/off power domain.
> Using callback, this sequence is no longer guaranteed and the side
> effect is unknown...
>
> So I would like to stick with adding larbs just like smi into powe
You want your power domain driver to poke, asynchronously, without
locking into registers of another device. And how does this not cause
issues?
No, work with your hardware engineers or Linux engineers on sane behavior.
Best regards,
Krzysztof
Hi yu-chang.lee,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on krzk-dt/for-next linus/master v6.9-rc1 next-20240328]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/yu-chang-lee/pmdomain-mediatek-add-smi_larb_reset-function-when-power-on/20240327-140007
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240327055732.28198-3-yu-chang.lee%40mediatek.com
patch subject: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240331/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: [error] syntax error: expected <block end>, but found '<block mapping start>' (syntax)
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:129:9: [warning] wrong indentation: expected 7 but found 8 (indentation)
--
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: did not find expected key
>> Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml:
while parsing a block mapping
in "<unicode string>", line 64, column 5
did not find expected key
in "<unicode string>", line 128, column 6
--
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml: ignoring, error parsing file
vim +128 Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
8
9 maintainers:
10 - MandyJH Liu <[email protected]>
11 - Matthias Brugger <[email protected]>
12
13 description: |
14 Mediatek processors include support for multiple power domains which can be
15 powered up/down by software based on different application scenes to save power.
16
17 IP cores belonging to a power domain should contain a 'power-domains'
18 property that is a phandle for SCPSYS node representing the domain.
19
20 properties:
21 $nodename:
22 pattern: '^power-controller(@[0-9a-f]+)?$'
23
24 compatible:
25 enum:
26 - mediatek,mt6795-power-controller
27 - mediatek,mt8167-power-controller
28 - mediatek,mt8173-power-controller
29 - mediatek,mt8183-power-controller
30 - mediatek,mt8186-power-controller
31 - mediatek,mt8188-power-controller
32 - mediatek,mt8192-power-controller
33 - mediatek,mt8195-power-controller
34 - mediatek,mt8365-power-controller
35
36 '#power-domain-cells':
37 const: 1
38
39 '#address-cells':
40 const: 1
41
42 '#size-cells':
43 const: 0
44
45 patternProperties:
46 "^power-domain@[0-9a-f]+$":
47 $ref: "#/$defs/power-domain-node"
48 patternProperties:
49 "^power-domain@[0-9a-f]+$":
50 $ref: "#/$defs/power-domain-node"
51 patternProperties:
52 "^power-domain@[0-9a-f]+$":
53 $ref: "#/$defs/power-domain-node"
54 patternProperties:
55 "^power-domain@[0-9a-f]+$":
56 $ref: "#/$defs/power-domain-node"
57 unevaluatedProperties: false
58 unevaluatedProperties: false
59 unevaluatedProperties: false
60 unevaluatedProperties: false
61
62 $defs:
63 power-domain-node:
64 type: object
65 description: |
66 Represents the power domains within the power controller node as documented
67 in Documentation/devicetree/bindings/power/power-domain.yaml.
68
69 properties:
70
71 '#power-domain-cells':
72 description:
73 Must be 0 for nodes representing a single PM domain and 1 for nodes
74 providing multiple PM domains.
75
76 '#address-cells':
77 const: 1
78
79 '#size-cells':
80 const: 0
81
82 reg:
83 description: |
84 Power domain index. Valid values are defined in:
85 "include/dt-bindings/power/mt6795-power.h" - for MT8167 type power domain.
86 "include/dt-bindings/power/mt8167-power.h" - for MT8167 type power domain.
87 "include/dt-bindings/power/mt8173-power.h" - for MT8173 type power domain.
88 "include/dt-bindings/power/mt8183-power.h" - for MT8183 type power domain.
89 "include/dt-bindings/power/mediatek,mt8188-power.h" - for MT8188 type power domain.
90 "include/dt-bindings/power/mt8192-power.h" - for MT8192 type power domain.
91 "include/dt-bindings/power/mt8195-power.h" - for MT8195 type power domain.
92 "include/dt-bindings/power/mediatek,mt8365-power.h" - for MT8365 type power domain.
93 maxItems: 1
94
95 clocks:
96 description: |
97 A number of phandles to clocks that need to be enabled during domain
98 power-up sequencing.
99
100 clock-names:
101 description: |
102 List of names of clocks, in order to match the power-up sequencing
103 for each power domain we need to group the clocks by name. BASIC
104 clocks need to be enabled before enabling the corresponding power
105 domain, and should not have a '-' in their name (i.e mm, mfg, venc).
106 SUSBYS clocks need to be enabled before releasing the bus protection,
107 and should contain a '-' in their name (i.e mm-0, isp-0, cam-0).
108
109 In order to follow properly the power-up sequencing, the clocks must
110 be specified by order, adding first the BASIC clocks followed by the
111 SUSBSYS clocks.
112
113 domain-supply:
114 description: domain regulator supply.
115
116 mediatek,infracfg:
117 $ref: /schemas/types.yaml#/definitions/phandle
118 description: phandle to the device containing the INFRACFG register range.
119
120 mediatek,infracfg-nao:
121 $ref: /schemas/types.yaml#/definitions/phandle
122 description: phandle to the device containing the INFRACFG-NAO register range.
123
124 mediatek,smi:
125 $ref: /schemas/types.yaml#/definitions/phandle
126 description: phandle to the device containing the SMI register range.
127
> 128 mediatek,larb:
> 129 $ref: /schemas/types.yaml#/definitions/phandle
130 description: phandle to the device containing the LARB register range.
131
132 required:
133 - reg
134
135 required:
136 - compatible
137
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Thu, 28 Mar 2024 at 07:06, Yu-chang Lee (李禹璋)
<[email protected]> wrote:
>
> On Wed, 2024-03-27 at 12:55 +0100, Alexandre Mergnat wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > Hello Yu-chang Lee,
> >
> > SMI LARB must have a power domain, according to "mediatek,smi-
> > larb.yaml"
> > Now you try to create a link from power domain to larb.
> >
> > Here is my understanding: when you enable/disable power domain, the
> > larb linked to this power domain may have an issue. Then you want to
> > retrieve de LARB linked to the power domain though the dts to manage
> > the LARB.
>
> Yes, this is what I am trying to do.
>
> > IMHO, using the dts to have this information into the power
> > driver isn't necessary and may introduce some bugs if the LARB node
> > and power node in the DTS aren't aligned.
> >
> > It seems not implemented today but during the LARB probe, it should
> > "subscribe" to the linked power domain. Then, when the power domain
> > status is changing, it is able to "notify" (callback) the LARB, then
> > implement the good stuff to handle this power domain status change
> > into LARB driver.
> >
>
> The problem with this method and why "smi clamp" is in power domain
> driver is that our HW designer gave us a programming guide strictly
> states the sequence of what we need to do to power on/off power domain.
> Using callback, this sequence is no longer guaranteed and the side
> effect is unknown...
In most cases, using the runtime PM callbacks in the consumer driver
(LARB driver) is sufficient to deal with resets. For some special
cases drivers are making use of the genpd on/off notifiers
(GENPD_NOTIFY_*), as they really need to know when their devices have
been power collapsed. Have you tried both these options?
[...]
Kind regards
Uffe