[PATCH v2 0/4] pmdomains: Fixes and add support for HFRP Direct

[PATCH v2 0/4] pmdomains: Fixes and add support for HFRP Direct

[AngeloGioacchino Del Regno]

Changes in v2:
 - Fixed return value checking in power off path

This series adds support for the DirectCTL HFRPSYS power domains found
on the MT8196 SoC (the ones without HW Voter support) and also adds a
fix to respect the power domain relationships during error cleanup,
which avoids HW lockups in case probe deferrals in the specific case
of "almost fully probed" power domains (where most of them probed and
got set up but a probe deferral happened almost at the end), behavior
seen on the MT8189 SoC during bringup (but honestly I have no idea how
are the current ones working fine without this fix...!).

This was tested on MT8173, MT8186, MT8188, MT8189, MT8192, MT8195 and
also on MT8196, over months of development, both manually and over CI,
with no regressions detected.

AngeloGioacchino Del Regno (4):
  dt-bindings: power: mediatek: Add support for MT8196 direct HFRP
  pmdomain: mediatek: Respect PD relationships during error cleanup
  pmdomain: mediatek: Add support for Direct CTL simple power sequence
  pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

 .../power/mediatek,power-controller.yaml      |   1 +
 drivers/pmdomain/mediatek/mt8196-pm-domains.h |  27 ++++
 drivers/pmdomain/mediatek/mtk-pm-domains.c    | 140 ++++++++++++++----
 drivers/pmdomain/mediatek/mtk-pm-domains.h    |   1 +
 .../dt-bindings/power/mediatek,mt8196-power.h |   4 +
 5 files changed, 147 insertions(+), 26 deletions(-)

-- 
2.54.0

[PATCH v2 1/4] dt-bindings: power: mediatek: Add support for MT8196 direct HFRP

[AngeloGioacchino Del Regno]

Add support for the HFRPSYS direct control power domains in the
MT8196 SoC, controlling power for the DisplayPort and for the
Embedded DisplayPort Transmitter IPs.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../devicetree/bindings/power/mediatek,power-controller.yaml  | 1 +
 include/dt-bindings/power/mediatek,mt8196-power.h             | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
index 07f046277f8a..070c6e5666dc 100644
--- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
+++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
@@ -34,6 +34,7 @@ properties:
       - mediatek,mt8189-power-controller
       - mediatek,mt8192-power-controller
       - mediatek,mt8195-power-controller
+      - mediatek,mt8196-hfrp-power-controller
       - mediatek,mt8196-hwv-hfrp-power-controller
       - mediatek,mt8196-hwv-scp-power-controller
       - mediatek,mt8196-power-controller
diff --git a/include/dt-bindings/power/mediatek,mt8196-power.h b/include/dt-bindings/power/mediatek,mt8196-power.h
index 0f622a93c807..085790bf8124 100644
--- a/include/dt-bindings/power/mediatek,mt8196-power.h
+++ b/include/dt-bindings/power/mediatek,mt8196-power.h
@@ -30,6 +30,10 @@
 #define MT8196_POWER_DOMAIN_MM_PROC_DORMANT		0
 #define MT8196_POWER_DOMAIN_SSR				1
 
+/* HFRPSYS Multimedia Power Control (MMPC) - Direct Control */
+#define MT8196_POWER_DOMAIN_EDPTX			0
+#define MT8196_POWER_DOMAIN_DPTX			1
+
 /* HFRPSYS MultiMedia Power Control (MMPC) - HW Voter */
 #define MT8196_POWER_DOMAIN_VDE0			0
 #define MT8196_POWER_DOMAIN_VDE1			1
-- 
2.54.0

[PATCH v2 2/4] pmdomain: mediatek: Respect PD relationships during error cleanup

[AngeloGioacchino Del Regno]

In case any probe error occurs (usually, a probe deferral) the
power domains shall be cleaned up while respecting their child
to parent relationship, or the system may freeze.

In order to do that without any memory footprint impacts after
the fact, allocate a temporary array in the probe function and
use it to store the indices of the added power domains in the
correct order.

This will be used in the error cleanup path and will be freed
at the end regardless of the probe status as, when the probing
succeeds, the genpd API takes care of unregistering all PDs in
the correct order anyway.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pmdomain/mediatek/mtk-pm-domains.c | 43 +++++++++++++++++-----
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index e1cfd4223473..db543d4b1813 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -738,7 +738,8 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
 }
 
 static struct
-generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node)
+generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node,
+					 u8 *domains_idx, u8 *num_domains)
 {
 	const struct scpsys_domain_data *domain_data;
 	const struct scpsys_hwv_domain_data *hwv_domain_data;
@@ -906,6 +907,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 	else
 		pm_genpd_init(&pd->genpd, NULL, false);
 
+	domains_idx[(*num_domains)++] = (u8) id;
 	scpsys->domains[id] = &pd->genpd;
 
 	return scpsys->pd_data.domains[id];
@@ -917,7 +919,8 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 	return ERR_PTR(ret);
 }
 
-static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *parent)
+static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *parent,
+				u8 *domains_idx, u8 *num_domains)
 {
 	struct generic_pm_domain *child_pd, *parent_pd;
 	struct device_node *child;
@@ -940,7 +943,7 @@ static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *paren
 
 		parent_pd = scpsys->pd_data.domains[id];
 
-		child_pd = scpsys_add_one_domain(scpsys, child);
+		child_pd = scpsys_add_one_domain(scpsys, child, domains_idx, num_domains);
 		if (IS_ERR(child_pd)) {
 			ret = PTR_ERR(child_pd);
 			dev_err_probe(scpsys->dev, ret, "%pOF: failed to get child domain id\n",
@@ -949,7 +952,7 @@ static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *paren
 		}
 
 		/* recursive call to add all subdomains */
-		ret = scpsys_add_subdomain(scpsys, child);
+		ret = scpsys_add_subdomain(scpsys, child, domains_idx, num_domains);
 		if (ret)
 			goto err_put_node;
 
@@ -991,14 +994,16 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
 	clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
 }
 
-static void scpsys_domain_cleanup(struct scpsys *scpsys)
+static void scpsys_domain_cleanup(struct scpsys *scpsys, u8 *domains_idx, u8 num_probed)
 {
 	struct generic_pm_domain *genpd;
 	struct scpsys_domain *pd;
 	int i;
 
-	for (i = scpsys->pd_data.num_domains - 1; i >= 0; i--) {
-		genpd = scpsys->pd_data.domains[i];
+	for (i = num_probed - 1; i >= 0; i--) {
+		u8 pd_idx = domains_idx[i];
+
+		genpd = scpsys->pd_data.domains[pd_idx];
 		if (genpd) {
 			pd = to_scpsys_domain(genpd);
 			scpsys_remove_one_domain(pd);
@@ -1215,6 +1220,8 @@ static int scpsys_probe(struct platform_device *pdev)
 	struct device *parent;
 	struct scpsys *scpsys;
 	int num_domains, ret;
+	u8 num_added_pds = 0;
+	u8 *added_pds_idx;
 
 	soc = of_device_get_match_data(&pdev->dev);
 	if (!soc) {
@@ -1228,6 +1235,19 @@ static int scpsys_probe(struct platform_device *pdev)
 	if (!scpsys)
 		return -ENOMEM;
 
+	/*
+	 * Temporarily store the IDs of the power domains that are added as in
+	 * case of a probe deferral this can be used to correctly cleanup all
+	 * of what was added before.
+	 *
+	 * Note that this array is used only in the probe function and must be
+	 * freed at the end, regardless of whether all of the power domains were
+	 * probed successfully or any failure happened.
+	 */
+	added_pds_idx = devm_kmalloc_array(dev, num_domains, sizeof(*added_pds_idx), GFP_KERNEL);
+	if (!added_pds_idx)
+		return -ENOMEM;
+
 	scpsys->dev = dev;
 	scpsys->soc_data = soc;
 
@@ -1258,13 +1278,15 @@ static int scpsys_probe(struct platform_device *pdev)
 	for_each_available_child_of_node_scoped(np, node) {
 		struct generic_pm_domain *domain;
 
-		domain = scpsys_add_one_domain(scpsys, node);
+		domain = scpsys_add_one_domain(scpsys, node,
+					       added_pds_idx, &num_added_pds);
 		if (IS_ERR(domain)) {
 			ret = PTR_ERR(domain);
 			goto err_cleanup_domains;
 		}
 
-		ret = scpsys_add_subdomain(scpsys, node);
+		ret = scpsys_add_subdomain(scpsys, node,
+					   added_pds_idx, &num_added_pds);
 		if (ret)
 			goto err_cleanup_domains;
 	}
@@ -1280,10 +1302,11 @@ static int scpsys_probe(struct platform_device *pdev)
 		goto err_cleanup_domains;
 	}
 
+	devm_kfree(dev, added_pds_idx);
 	return 0;
 
 err_cleanup_domains:
-	scpsys_domain_cleanup(scpsys);
+	scpsys_domain_cleanup(scpsys, added_pds_idx, num_added_pds);
 	return ret;
 }
 
-- 
2.54.0

[PATCH v2 3/4] pmdomain: mediatek: Add support for Direct CTL simple power sequence

[AngeloGioacchino Del Regno]

Some new SoCs like MT8196, MT6991, and others, have got one
additional power controller (usually in the HFRP Multimedia
block) which needs a simplified power on/off sequence while
using Direct Control strategy.

Domains using the "simple power sequence" are not backed by
the RTFF hardware, have no Bus Protection mechanism, lacks
the ISO, PWR_ON, PWR_ON_2ND bits, and therefore get enabled
automatically after getting out of reset.

This simple power sequence is then a subset of the full one
as only needs the enablement of the specific power domain's
clock input and reset (where, again, after getting out of
reset, the ISO and PWR_ON bits are automatically internally
getting flipped) to enable or disable (power on or off).

Moreover, the simple power sequence power domains guarantee
that they always get enabled/disabled after executing the
relevant power sequence (on/off) so, differently from the
others, there is also no need to poll for a PWR_ACK.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pmdomain/mediatek/mtk-pm-domains.c | 93 ++++++++++++++++++----
 drivers/pmdomain/mediatek/mtk-pm-domains.h |  1 +
 2 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index db543d4b1813..feba436279c5 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -549,9 +549,11 @@ static int scpsys_ctl_pwrseq_on(struct scpsys_domain *pd)
 	return 0;
 }
 
-static void scpsys_ctl_pwrseq_off(struct scpsys_domain *pd)
+static int scpsys_ctl_pwrseq_off(struct scpsys_domain *pd)
 {
 	struct scpsys *scpsys = pd->scpsys;
+	bool tmp;
+	int ret;
 
 	switch (pd->data->rtff_type) {
 	case SCPSYS_RTFF_TYPE_GENERIC:
@@ -583,6 +585,41 @@ static void scpsys_ctl_pwrseq_off(struct scpsys_domain *pd)
 	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
 	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_ON_2ND_BIT);
 	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_ON_BIT);
+
+	/* wait until PWR_ACK = 0 */
+	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
+				 MTK_POLL_TIMEOUT);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int scpsys_simple_pwrseq_on(struct scpsys_domain *pd)
+{
+	struct scpsys *scpsys = pd->scpsys;
+
+	/* Enable subsys clock input and trigger power domain reset state */
+	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_CLK_DIS_BIT);
+	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
+
+	/* Wait for the hardware to stabilize */
+	udelay(1);
+
+	/* Get out of reset: set power on */
+	regmap_set_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
+
+	return 0;
+}
+
+static int scpsys_simple_pwrseq_off(struct scpsys_domain *pd)
+{
+	struct scpsys *scpsys = pd->scpsys;
+
+	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
+	regmap_set_bits(scpsys->base, pd->data->ctl_offs, PWR_CLK_DIS_BIT);
+
+	return 0;
 }
 
 static int scpsys_modem_pwrseq_on(struct scpsys_domain *pd)
@@ -605,14 +642,24 @@ static int scpsys_modem_pwrseq_on(struct scpsys_domain *pd)
 	return 0;
 }
 
-static void scpsys_modem_pwrseq_off(struct scpsys_domain *pd)
+static int scpsys_modem_pwrseq_off(struct scpsys_domain *pd)
 {
 	struct scpsys *scpsys = pd->scpsys;
+	bool tmp;
+	int ret;
 
 	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_ON_BIT);
 
 	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SKIP_RESET_B))
 		regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
+
+	/* wait until PWR_ACK = 0 */
+	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
+				 MTK_POLL_TIMEOUT);
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
 static int scpsys_power_on(struct generic_pm_domain *genpd)
@@ -635,6 +682,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 
 	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
 		ret = scpsys_modem_pwrseq_on(pd);
+	else if (MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
+		ret = scpsys_simple_pwrseq_on(pd);
 	else
 		ret = scpsys_ctl_pwrseq_on(pd);
 
@@ -662,9 +711,11 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 			goto err_pwr_ack;
 	}
 
-	ret = scpsys_sram_enable(pd);
-	if (ret < 0)
-		goto err_disable_subsys_clks;
+	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
+		ret = scpsys_sram_enable(pd);
+		if (ret < 0)
+			goto err_disable_subsys_clks;
+	}
 
 	ret = scpsys_bus_protect_disable(pd, 0);
 	if (ret < 0)
@@ -682,7 +733,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 err_enable_bus_protect:
 	scpsys_bus_protect_enable(pd, 0);
 err_disable_sram:
-	scpsys_sram_disable(pd);
+	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
+		scpsys_sram_disable(pd);
 err_disable_subsys_clks:
 	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
 		clk_bulk_disable_unprepare(pd->num_subsys_clks,
@@ -698,16 +750,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
 {
 	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
 	struct scpsys *scpsys = pd->scpsys;
-	bool tmp;
 	int ret;
 
 	ret = scpsys_bus_protect_enable(pd, 0);
 	if (ret < 0)
 		return ret;
 
-	ret = scpsys_sram_disable(pd);
-	if (ret < 0)
-		return ret;
+	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
+		ret = scpsys_sram_disable(pd);
+		if (ret < 0)
+			return ret;
+	}
 
 	if (pd->data->ext_buck_iso_offs && MTK_SCPD_CAPS(pd, MTK_SCPD_EXT_BUCK_ISO))
 		regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
@@ -720,15 +773,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
 		return ret;
 
 	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
-		scpsys_modem_pwrseq_off(pd);
+		ret = scpsys_modem_pwrseq_off(pd);
+	else if (MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
+		ret = scpsys_simple_pwrseq_off(pd);
 	else
-		scpsys_ctl_pwrseq_off(pd);
+		ret = scpsys_ctl_pwrseq_off(pd);
 
-	/* wait until PWR_ACK = 0 */
-	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
-				 MTK_POLL_TIMEOUT);
-	if (ret < 0)
+	if (ret < 0) {
+		/* Re-enable clocks so that next power off doesn't break the refcount */
+		clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
 		return ret;
+	}
 
 	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
 
@@ -1083,6 +1138,12 @@ static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
 		regmap[2] = NULL;
 	}
 
+	/* If no access controllers are needed, don't allocate and don't fail */
+	if (num_regmaps == 0) {
+		scpsys->bus_prot = NULL;
+		return 0;
+	}
+
 	scpsys->bus_prot = devm_kmalloc_array(dev, num_regmaps,
 					      sizeof(*scpsys->bus_prot), GFP_KERNEL);
 	if (!scpsys->bus_prot)
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.h b/drivers/pmdomain/mediatek/mtk-pm-domains.h
index a5dca24cbc2f..092403de66fa 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.h
@@ -17,6 +17,7 @@
 #define MTK_SCPD_MODEM_PWRSEQ		BIT(10)
 #define MTK_SCPD_SKIP_RESET_B		BIT(11)
 #define MTK_SCPD_INFRA_PWR_CTL		BIT(12)
+#define MTK_SCPD_SIMPLE_PWRSEQ		BIT(13)
 #define MTK_SCPD_CAPS(_scpd, _x)	((_scpd)->data ?		\
 					 (_scpd)->data->caps & (_x) :	\
 					 (_scpd)->hwv_data->caps & (_x))
-- 
2.54.0

[PATCH v2 4/4] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

[AngeloGioacchino Del Regno]

Add support for the power domains provided by the HFRPSYS Power
Controller of the MT8196 SoC.
Those control power to the eDP and DP Transmitter IPs.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pmdomain/mediatek/mt8196-pm-domains.h | 27 +++++++++++++++++++
 drivers/pmdomain/mediatek/mtk-pm-domains.c    |  4 +++
 2 files changed, 31 insertions(+)

diff --git a/drivers/pmdomain/mediatek/mt8196-pm-domains.h b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
index 2e4b28720659..d704c9fa9337 100644
--- a/drivers/pmdomain/mediatek/mt8196-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
@@ -602,6 +602,27 @@ static const struct scpsys_hwv_domain_data hfrpsys_hwv_domain_data_mt8196[] = {
 	},
 };
 
+static const struct scpsys_domain_data hfrpsys_domain_data_mt8196[] = {
+	[MT8196_POWER_DOMAIN_EDPTX] = {
+		.name = "edp-tx",
+		.sta_mask = MT8196_PWR_ACK,
+		.sta2nd_mask = MT8196_PWR_ACK_2ND,
+		.ctl_offs = 0x74,
+		.pwr_sta_offs = 0x74,
+		.pwr_sta2nd_offs = 0x74,
+		.caps = MTK_SCPD_SIMPLE_PWRSEQ,
+	},
+	[MT8196_POWER_DOMAIN_DPTX] = {
+		.name = "dp-tx",
+		.sta_mask = MT8196_PWR_ACK,
+		.sta2nd_mask = MT8196_PWR_ACK_2ND,
+		.ctl_offs = 0x78,
+		.pwr_sta_offs = 0x78,
+		.pwr_sta2nd_offs = 0x78,
+		.caps = MTK_SCPD_SIMPLE_PWRSEQ,
+	},
+};
+
 static const struct scpsys_soc_data mt8196_scpsys_data = {
 	.domains_data = scpsys_domain_data_mt8196,
 	.num_domains = ARRAY_SIZE(scpsys_domain_data_mt8196),
@@ -616,6 +637,12 @@ static const struct scpsys_soc_data mt8196_scpsys_hwv_data = {
 	.type = SCPSYS_MTCMOS_TYPE_HW_VOTER,
 };
 
+static const struct scpsys_soc_data mt8196_hfrpsys_data = {
+	.domains_data = hfrpsys_domain_data_mt8196,
+	.num_domains = ARRAY_SIZE(hfrpsys_domain_data_mt8196),
+	.type = SCPSYS_MTCMOS_TYPE_DIRECT_CTL,
+};
+
 static const struct scpsys_soc_data mt8196_hfrpsys_hwv_data = {
 	.hwv_domains_data = hfrpsys_hwv_domain_data_mt8196,
 	.num_hwv_domains = ARRAY_SIZE(hfrpsys_hwv_domain_data_mt8196),
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index feba436279c5..55e747880423 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -1258,6 +1258,10 @@ static const struct of_device_id scpsys_of_match[] = {
 		.compatible = "mediatek,mt8196-power-controller",
 		.data = &mt8196_scpsys_data,
 	},
+	{
+		.compatible = "mediatek,mt8196-hfrp-power-controller",
+		.data = &mt8196_hfrpsys_data,
+	},
 	{
 		.compatible = "mediatek,mt8196-hwv-hfrp-power-controller",
 		.data = &mt8196_hfrpsys_hwv_data,
-- 
2.54.0

Re: [PATCH v2 3/4] pmdomain: mediatek: Add support for Direct CTL simple power sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Low] Ignoring the return value of `clk_bulk_prepare_enable` in the error recovery path of `scpsys_power_off` violates its `__must_check` attribute.

Pre-existing issues:
- [High] If `scpsys_bus_protect_enable()` fails in `scpsys_power_off()`, it returns early without re-enabling `subsys_clks`, bypassing the newly added clock recovery block and breaking the clock refcount.
- [High] Error paths in `scpsys_power_off()` leave the hardware in a corrupted state (SRAM disabled, bus protection active, and buck isolated) because they fail to revert these states before returning an error.
- [High] Error paths in `scpsys_power_on()` disable clocks and regulators without reverting the power-on hardware sequence, leaving the domain hardware partially ON while genpd considers it OFF.
--

--- Patch [3]: [PATCH v2 3/4] pmdomain: mediatek: Add support for Direct CTL simple power sequence ---
commit a25fe6d3db6b42f564ecc5977f0a00e29a0cfd96
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

pmdomain: mediatek: Add support for Direct CTL simple power sequence

This commit adds a simplified power sequence for certain new MediaTek SoCs 
(like MT8196 and MT6991). The new sequence avoids using RTFF hardware and 
certain control bits, handling power domain enablement automatically after 
exiting reset.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index db543d4b1813c..feba436279c54 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
[ ... ]
> @@ -682,7 +733,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>  err_enable_bus_protect:
>  	scpsys_bus_protect_enable(pd, 0);
>  err_disable_sram:
> -	scpsys_sram_disable(pd);
> +	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
> +		scpsys_sram_disable(pd);
>  err_disable_subsys_clks:
>  	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
>  		clk_bulk_disable_unprepare(pd->num_subsys_clks,

[Severity: High]
This is a pre-existing issue, but if a step fails during scpsys_power_on()
after the initial MTCMOS power-on sequence, the function jumps to these 
error labels. It disables the clocks and regulators, but does this leave 
the domain's MTCMOS physically ON since it doesn't call a corresponding 
power-off sequence to revert it? Could this hardware inconsistency cause 
issues while the framework considers the domain OFF?

> @@ -698,16 +750,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>  {
>  	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
>  	struct scpsys *scpsys = pd->scpsys;
> -	bool tmp;
>  	int ret;
>  
>  	ret = scpsys_bus_protect_enable(pd, 0);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = scpsys_sram_disable(pd);
> -	if (ret < 0)
> -		return ret;
> +	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
> +		ret = scpsys_sram_disable(pd);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: High]
This is a pre-existing issue, but if scpsys_sram_disable() or 
scpsys_bus_protect_enable() fail here and return early, the framework will 
still treat the domain as ON. Does this early return leave the hardware 
physically isolated, potentially causing active consumer devices to hit an 
active bus protector or unpowered SRAM?

[ ... ]
> @@ -720,15 +773,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>  		return ret;
>  
>  	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
> -		scpsys_modem_pwrseq_off(pd);
> +		ret = scpsys_modem_pwrseq_off(pd);
> +	else if (MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
> +		ret = scpsys_simple_pwrseq_off(pd);
>  	else
> -		scpsys_ctl_pwrseq_off(pd);
> +		ret = scpsys_ctl_pwrseq_off(pd);
>  
> -	/* wait until PWR_ACK = 0 */
> -	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
> -				 MTK_POLL_TIMEOUT);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		/* Re-enable clocks so that next power off doesn't break the refcount */
> +		clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);

[Severity: High]
This is a pre-existing issue, but if scpsys_bus_protect_enable() fails 
earlier in scpsys_power_off() when called with BUS_PROT_IGNORE_SUBCLK, it 
returns early. Does that early return completely bypass this newly added 
clock recovery block, leaving the subsystem clocks disabled while the 
domain is logically considered ON?

[Severity: Low]
Does this error recovery block ignore the return value of 
clk_bulk_prepare_enable()? Since the function is declared with 
__must_check in the clk API, could this cause a -Wunused-result compiler 
warning and break builds using -Werror?

>  		return ret;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707155535.199383-1-angelogioacchino.delregno@collabora.com?part=3

Re: [PATCH v2 4/4] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Resource leak on error cleanup due to dummy `sta_mask` in simple power sequence domains causing `scpsys_domain_is_on()` to incorrectly return false.

Pre-existing issues:
- [High] Kernel panic during probe error cleanup due to unconditional dereference of `pd->data` for `HW_VOTER` domains in `scpsys_domain_is_on()`.
--

--- Patch [4]: [PATCH v2 4/4] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains ---
commit 6f2aa6326ce2b3105bc55383da9b0e0d8b25e2d6
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

Add support for the power domains provided by the HFRPSYS Power Controller of
the MT8196 SoC. Those control power to the eDP and DP Transmitter IPs.

> diff --git a/drivers/pmdomain/mediatek/mt8196-pm-domains.h b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
> index 2e4b287206596..d704c9fa93374 100644
> --- a/drivers/pmdomain/mediatek/mt8196-pm-domains.h
> +++ b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
> @@ -602,6 +602,27 @@ static const struct scpsys_hwv_domain_data hfrpsys_hwv_domain_data_mt8196[] = {
>  	},
>  };
>  
> +static const struct scpsys_domain_data hfrpsys_domain_data_mt8196[] = {
> +	[MT8196_POWER_DOMAIN_EDPTX] = {
> +		.name = "edp-tx",
> +		.sta_mask = MT8196_PWR_ACK,

[Severity: Medium]
Can this lead to a resource leak during error cleanup?

The MT8196_POWER_DOMAIN_EDPTX and DPTX domains use simple power sequence,
bypassing standard PWR_ACK polling. To pass validation during initialization,
they are assigned MT8196_PWR_ACK as a sta_mask.

If this bit is a dummy and not automatically driven high by hardware for these
specific domains (since it is never polled during power_on),
scpsys_domain_is_on() will perpetually read 0 and return false.

If a failure triggers probe error cleanup, scpsys_remove_one_domain() would
skip scpsys_power_off() for these domains because it incorrectly believes they
are already off. Does this leave the domain permanently powered on in a
corrupted state and leak prepared clocks and regulators?

> +		.sta2nd_mask = MT8196_PWR_ACK_2ND,
> +		.ctl_offs = 0x74,
> +		.pwr_sta_offs = 0x74,
> +		.pwr_sta2nd_offs = 0x74,
> +		.caps = MTK_SCPD_SIMPLE_PWRSEQ,
> +	},

[ ... ]

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index feba436279c54..55e7478804236 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> @@ -1258,6 +1258,10 @@ static const struct of_device_id scpsys_of_match[] = {
>  		.compatible = "mediatek,mt8196-power-controller",
>  		.data = &mt8196_scpsys_data,
>  	},
> +	{
> +		.compatible = "mediatek,mt8196-hfrp-power-controller",
> +		.data = &mt8196_hfrpsys_data,
> +	},

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a kernel
panic during probe error cleanup?

If a hardware voter domain is probed successfully, it is added to the probed
list. If a subsequently probed domain fails, scpsys_domain_cleanup()
initiates rollback, calling scpsys_remove_one_domain() on the hardware
voter domain.

scpsys_remove_one_domain() unconditionally calls scpsys_domain_is_on(), which
attempts to read pd->data->sta_mask:

scpsys_remove_one_domain()
    scpsys_domain_is_on()
        u32 mask = pd->data->sta_mask;

For hardware voter domains, pd->data is explicitly set to NULL, which appears
to result in a guaranteed NULL pointer dereference. Could this bring the
system down during boot on unsupported or failing hardware configurations?

>  	{
>  		.compatible = "mediatek,mt8196-hwv-hfrp-power-controller",
>  		.data = &mt8196_hfrpsys_hwv_data,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707155535.199383-1-angelogioacchino.delregno@collabora.com?part=4

Re: [PATCH v2 3/4] pmdomain: mediatek: Add support for Direct CTL simple power sequence

[AngeloGioacchino Del Regno]

On 7/7/26 18:08, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [Low] Ignoring the return value of `clk_bulk_prepare_enable` in the error recovery path of `scpsys_power_off` violates its `__must_check` attribute.
> 
> Pre-existing issues:
> - [High] If `scpsys_bus_protect_enable()` fails in `scpsys_power_off()`, it returns early without re-enabling `subsys_clks`, bypassing the newly added clock recovery block and breaking the clock refcount.
> - [High] Error paths in `scpsys_power_off()` leave the hardware in a corrupted state (SRAM disabled, bus protection active, and buck isolated) because they fail to revert these states before returning an error.
> - [High] Error paths in `scpsys_power_on()` disable clocks and regulators without reverting the power-on hardware sequence, leaving the domain hardware partially ON while genpd considers it OFF.
> --
> 
> --- Patch [3]: [PATCH v2 3/4] pmdomain: mediatek: Add support for Direct CTL simple power sequence ---
> commit a25fe6d3db6b42f564ecc5977f0a00e29a0cfd96
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> pmdomain: mediatek: Add support for Direct CTL simple power sequence
> 
> This commit adds a simplified power sequence for certain new MediaTek SoCs
> (like MT8196 and MT6991). The new sequence avoids using RTFF hardware and
> certain control bits, handling power domain enablement automatically after
> exiting reset.
> 
>> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> index db543d4b1813c..feba436279c54 100644
>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> [ ... ]
>> @@ -682,7 +733,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>   err_enable_bus_protect:
>>   	scpsys_bus_protect_enable(pd, 0);
>>   err_disable_sram:
>> -	scpsys_sram_disable(pd);
>> +	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
>> +		scpsys_sram_disable(pd);
>>   err_disable_subsys_clks:
>>   	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
>>   		clk_bulk_disable_unprepare(pd->num_subsys_clks,
> 
> [Severity: High]
> This is a pre-existing issue, but if a step fails during scpsys_power_on()
> after the initial MTCMOS power-on sequence, the function jumps to these
> error labels. It disables the clocks and regulators, but does this leave
> the domain's MTCMOS physically ON since it doesn't call a corresponding
> power-off sequence to revert it? Could this hardware inconsistency cause
> issues while the framework considers the domain OFF?
> 
>> @@ -698,16 +750,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>>   {
>>   	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
>>   	struct scpsys *scpsys = pd->scpsys;
>> -	bool tmp;
>>   	int ret;
>>   
>>   	ret = scpsys_bus_protect_enable(pd, 0);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	ret = scpsys_sram_disable(pd);
>> -	if (ret < 0)
>> -		return ret;
>> +	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
>> +		ret = scpsys_sram_disable(pd);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
> 
> [Severity: High]
> This is a pre-existing issue, but if scpsys_sram_disable() or
> scpsys_bus_protect_enable() fail here and return early, the framework will
> still treat the domain as ON. Does this early return leave the hardware
> physically isolated, potentially causing active consumer devices to hit an
> active bus protector or unpowered SRAM?
> 

The problem here is that if sram_disable() fails, sram_enable() is guaranteed to
also fail, there's nothing that can really be done to fix the disablement failure
if any happens.

I have never encountered a single failure here during disablement, but should it
happen, the only way to go here is to pray and hope that the next power_on() will
fix things up.

Can't do anything about it.

> [ ... ]
>> @@ -720,15 +773,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>>   		return ret;
>>   
>>   	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
>> -		scpsys_modem_pwrseq_off(pd);
>> +		ret = scpsys_modem_pwrseq_off(pd);
>> +	else if (MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
>> +		ret = scpsys_simple_pwrseq_off(pd);
>>   	else
>> -		scpsys_ctl_pwrseq_off(pd);
>> +		ret = scpsys_ctl_pwrseq_off(pd);
>>   
>> -	/* wait until PWR_ACK = 0 */
>> -	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
>> -				 MTK_POLL_TIMEOUT);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>> +		/* Re-enable clocks so that next power off doesn't break the refcount */
>> +		clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
> 
> [Severity: High]
> This is a pre-existing issue, but if scpsys_bus_protect_enable() fails
> earlier in scpsys_power_off() when called with BUS_PROT_IGNORE_SUBCLK, it
> returns early. Does that early return completely bypass this newly added
> clock recovery block, leaving the subsystem clocks disabled while the
> domain is logically considered ON?
> 

No that shouldn't happen, but to really resolve that we'd need a dual-stage probe
and/or enablement mechanism, like regulators. Can't do anything about that now,
but a talk for plumbers may be coming........

> [Severity: Low]
> Does this error recovery block ignore the return value of
> clk_bulk_prepare_enable()? Since the function is declared with
> __must_check in the clk API, could this cause a -Wunused-result compiler
> warning and break builds using -Werror?
> 

Nah. It's fine.

>>   		return ret;
>> +	}
> 

Re: [PATCH v2 4/4] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

[AngeloGioacchino Del Regno]

On 7/7/26 18:20, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Resource leak on error cleanup due to dummy `sta_mask` in simple power sequence domains causing `scpsys_domain_is_on()` to incorrectly return false.
> 
> Pre-existing issues:
> - [High] Kernel panic during probe error cleanup due to unconditional dereference of `pd->data` for `HW_VOTER` domains in `scpsys_domain_is_on()`.
> --
> 
> --- Patch [4]: [PATCH v2 4/4] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains ---
> commit 6f2aa6326ce2b3105bc55383da9b0e0d8b25e2d6
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains
> 
> Add support for the power domains provided by the HFRPSYS Power Controller of
> the MT8196 SoC. Those control power to the eDP and DP Transmitter IPs.
> 
>> diff --git a/drivers/pmdomain/mediatek/mt8196-pm-domains.h b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
>> index 2e4b287206596..d704c9fa93374 100644
>> --- a/drivers/pmdomain/mediatek/mt8196-pm-domains.h
>> +++ b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
>> @@ -602,6 +602,27 @@ static const struct scpsys_hwv_domain_data hfrpsys_hwv_domain_data_mt8196[] = {
>>   	},
>>   };
>>   
>> +static const struct scpsys_domain_data hfrpsys_domain_data_mt8196[] = {
>> +	[MT8196_POWER_DOMAIN_EDPTX] = {
>> +		.name = "edp-tx",
>> +		.sta_mask = MT8196_PWR_ACK,
> 
> [Severity: Medium]
> Can this lead to a resource leak during error cleanup?
> 
> The MT8196_POWER_DOMAIN_EDPTX and DPTX domains use simple power sequence,
> bypassing standard PWR_ACK polling. To pass validation during initialization,
> they are assigned MT8196_PWR_ACK as a sta_mask.
> 
> If this bit is a dummy and not automatically driven high by hardware for these
> specific domains (since it is never polled during power_on),
> scpsys_domain_is_on() will perpetually read 0 and return false.

Why Sashiko assumes that this is a dummy is bogus. Why should it ever be a dummy
and why would I add it if it was useless?!

> 
> If a failure triggers probe error cleanup, scpsys_remove_one_domain() would
> skip scpsys_power_off() for these domains because it incorrectly believes they
> are already off. Does this leave the domain permanently powered on in a
> corrupted state and leak prepared clocks and regulators?
> 
>> +		.sta2nd_mask = MT8196_PWR_ACK_2ND,
>> +		.ctl_offs = 0x74,
>> +		.pwr_sta_offs = 0x74,
>> +		.pwr_sta2nd_offs = 0x74,
>> +		.caps = MTK_SCPD_SIMPLE_PWRSEQ,
>> +	},
> 
> [ ... ]
> 
>> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> index feba436279c54..55e7478804236 100644
>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> @@ -1258,6 +1258,10 @@ static const struct of_device_id scpsys_of_match[] = {
>>   		.compatible = "mediatek,mt8196-power-controller",
>>   		.data = &mt8196_scpsys_data,
>>   	},
>> +	{
>> +		.compatible = "mediatek,mt8196-hfrp-power-controller",
>> +		.data = &mt8196_hfrpsys_data,
>> +	},
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but could this lead to a kernel
> panic during probe error cleanup?
> 
> If a hardware voter domain is probed successfully, it is added to the probed
> list. If a subsequently probed domain fails, scpsys_domain_cleanup()
> initiates rollback, calling scpsys_remove_one_domain() on the hardware
> voter domain.
> 
> scpsys_remove_one_domain() unconditionally calls scpsys_domain_is_on(), which
> attempts to read pd->data->sta_mask:
> 
> scpsys_remove_one_domain()
>      scpsys_domain_is_on()
>          u32 mask = pd->data->sta_mask;
> 
> For hardware voter domains, pd->data is explicitly set to NULL, which appears
> to result in a guaranteed NULL pointer dereference. Could this bring the
> system down during boot on unsupported or failing hardware configurations?
> 

I'll send a Fixes patch for that.

>>   	{
>>   		.compatible = "mediatek,mt8196-hwv-hfrp-power-controller",
>>   		.data = &mt8196_hfrpsys_hwv_data,
> 

Re: [PATCH v2 1/4] dt-bindings: power: mediatek: Add support for MT8196 direct HFRP

[Krzysztof Kozlowski]

On Tue, Jul 07, 2026 at 05:55:32PM +0200, AngeloGioacchino Del Regno wrote:
> Add support for the HFRPSYS direct control power domains in the
> MT8196 SoC, controlling power for the DisplayPort and for the
> Embedded DisplayPort Transmitter IPs.
> 
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../devicetree/bindings/power/mediatek,power-controller.yaml  | 1 +
>  include/dt-bindings/power/mediatek,mt8196-power.h             | 4 ++++
>  2 files changed, 5 insertions(+)

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof