[PATCH v2 0/4] pmdomain: mediatek: Enable the GPU timestamp counter on MT8188

[PATCH v2 0/4] pmdomain: mediatek: Enable the GPU timestamp counter on MT8188

[Michał Kopeć]

On MT8188 the GPU timestamp counter reads zero until a register in the
MFGCFG block feeds it the SoC system timer, and MFGCFG loses that
setting whenever the MFG1 power domain is turned off. As a result, the
SYSTEM_TIMESTAMP query and the timestamps written by GPU jobs are always
zero, which breaks timestamp queries in userspace drivers.

v1 restored the register from panfrost through a new mediatek,mfgcfg
phandle on the GPU node. As Krzysztof pointed out, the register is lost
by the power domain, so v2 restores it there instead: the power
controller lists MFGCFG in access-controllers and writes the register
every time MFG1 has been powered on.

Patch 4 converts mt8188.dtsi from the deprecated per-domain
mediatek,infracfg phandles to access-controllers. A devicetree with it
needs the driver changes of patches 2 and 3, because older kernels
expect exactly one access controller on MT8188 and fail to probe the
power controller otherwise. Older devicetrees keep working with the new
driver, without the timestamp.

MT8370 uses the MT8188 power controller and mt8188.dtsi, so it is
covered as well.

Tested on the Lenovo Chromebook Duet (MT8188): the timestamp counts at
13 MHz, also after suspend and resume.

Changes in v2:
- Restore the register from the MFG1 power domain instead of panfrost,
  and drop the GPU phandle (Krzysztof).
- Drop the panfrost patch, which makes the review comments about error
  propagation and MT8370 moot.

v1: https://lore.kernel.org/all/20261003200132.866240-1-michal@nozomi.space/

Michał Kopeć (4):
  dt-bindings: power: mediatek: Add the MT8188 access controllers
  pmdomain: mediatek: Restore a register after powering a domain on
  pmdomain: mediatek: mt8188: Enable the GPU timestamp counter
  arm64: dts: mediatek: mt8188: Use access-controllers in the power
    controller

 .../power/mediatek,power-controller.yaml      | 16 +++++++++++++-
 arch/arm64/boot/dts/mediatek/mt8188.dtsi      | 22 +------------------
 drivers/pmdomain/mediatek/mt8188-pm-domains.h | 16 +++++++++++++-
 drivers/pmdomain/mediatek/mtk-pm-domains.c    | 19 ++++++++++++++++
 drivers/pmdomain/mediatek/mtk-pm-domains.h    | 15 +++++++++++++
 5 files changed, 65 insertions(+), 23 deletions(-)

-- 
2.56.0

[PATCH v2 1/4] dt-bindings: power: mediatek: Add the MT8188 access controllers

[Michał Kopeć]

On MT8188 the GPU timestamp counter only counts once a register in the
MFGCFG block feeds it the SoC system timer. MFGCFG loses that setting
whenever the MFG1 power domain is turned off, so the power controller
has to restore it after powering the domain on, and needs access to
MFGCFG for that.

Describe the blocks that the MT8188 power controller accesses,
INFRACFG_AO for bus protection and MFGCFG, and extend the description
of access-controllers to cover blocks with settings to restore.

Signed-off-by: Michał Kopeć <michal@nozomi.space>
---
 .../power/mediatek,power-controller.yaml         | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
index d03e4a9251635..8d96cde7c1a51 100644
--- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
+++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
@@ -54,7 +54,8 @@ properties:
     description:
       A number of phandles to external blocks to set and clear the required
       bits to enable or disable bus protection, necessary to avoid any bus
-      faults while enabling or disabling a power domain.
+      faults while enabling or disabling a power domain, or to restore
+      settings that a block loses when its power domain is turned off.
       For example, this may hold phandles to INFRACFG and SMI.
     minItems: 1
     maxItems: 6
@@ -188,6 +189,19 @@ allOf:
           minItems: 2
           maxItems: 2
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt8188-power-controller
+    then:
+      properties:
+        access-controllers:
+          items:
+            - description: handle to INFRACFG_AO register block
+            - description: handle to MFGCFG register block
+
   - if:
       properties:
         compatible:
-- 
2.56.0

[PATCH v2 2/4] pmdomain: mediatek: Restore a register after powering a domain on

[Michał Kopeć]

Some blocks lose a setting when their power domain is turned off, and it
has to be restored every time the domain is powered on again, before
the devices in the domain use it.

Let the domain data name such a register in one of the blocks listed in
access-controllers, and write it once the bus protection of the domain
has been released. Devicetrees that use the older per-domain phandles,
or don't list the block, keep working without the setting.

Signed-off-by: Michał Kopeć <michal@nozomi.space>
---
 drivers/pmdomain/mediatek/mtk-pm-domains.c | 19 +++++++++++++++++++
 drivers/pmdomain/mediatek/mtk-pm-domains.h | 14 ++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index f0fa79e01f9c3..0585a663f48a6 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -72,6 +72,7 @@ struct scpsys_domain {
 	int num_subsys_clks;
 	struct clk_bulk_data *subsys_clks;
 	struct regulator *supply;
+	struct regmap *init_regmap;
 };
 
 struct scpsys {
@@ -79,6 +80,7 @@ struct scpsys {
 	struct regmap *base;
 	const struct scpsys_soc_data *soc_data;
 	u8 bus_prot_index[BUS_PROT_BLOCK_COUNT];
+	unsigned long bus_prot_avail;
 	struct regmap **bus_prot;
 	struct genpd_onecell_data pd_data;
 	struct generic_pm_domain *domains[];
@@ -755,6 +757,13 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 	if (ret < 0)
 		goto err_disable_sram;
 
+	if (pd->init_regmap) {
+		ret = regmap_write(pd->init_regmap, pd->data->reg_init->ofs,
+				   pd->data->reg_init->val);
+		if (ret)
+			goto err_enable_bus_protect;
+	}
+
 	if (MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION)) {
 		ret = clk_bulk_prepare_enable(pd->num_subsys_clks,
 					      pd->subsys_clks);
@@ -900,6 +909,14 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 	pd->hwv_data = hwv_domain_data;
 	pd->scpsys = scpsys;
 
+	if (domain_data && domain_data->reg_init) {
+		enum scpsys_bus_prot_block block = domain_data->reg_init->block;
+
+		/* Older devicetrees don't describe every block it needs */
+		if (scpsys->bus_prot_avail & BIT(block))
+			pd->init_regmap = scpsys->bus_prot[scpsys->bus_prot_index[block]];
+	}
+
 	if (MTK_SCPD_CAPS(pd, MTK_SCPD_DOMAIN_SUPPLY)) {
 		pd->supply = devm_of_regulator_get_optional(scpsys->dev, node, "domain");
 		if (IS_ERR(pd->supply))
@@ -1228,6 +1245,7 @@ static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
 
 		bp_type = bp_blocks[i];
 		scpsys->bus_prot_index[bp_type] = j;
+		scpsys->bus_prot_avail |= BIT(bp_type);
 		scpsys->bus_prot[j] = regmap[i];
 
 		j++;
@@ -1268,6 +1286,7 @@ static int scpsys_get_bus_protection(struct device *dev, struct scpsys *scpsys)
 		 */
 		bp_type = soc->bus_prot_blocks[i];
 		scpsys->bus_prot_index[bp_type] = i;
+		scpsys->bus_prot_avail |= BIT(bp_type);
 
 		scpsys->bus_prot[i] = device_node_to_regmap(node);
 		of_node_put(node);
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.h b/drivers/pmdomain/mediatek/mtk-pm-domains.h
index 45da83e9d649a..4c2c56addb3cc 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.h
@@ -152,6 +152,18 @@ enum scpsys_mtcmos_type {
 	SCPSYS_MTCMOS_TYPE_MAX
 };
 
+/**
+ * struct scpsys_reg_init - register set up after a domain is powered on
+ * @block: The access controller block holding the register.
+ * @ofs: The offset of the register.
+ * @val: The value to write.
+ */
+struct scpsys_reg_init {
+	enum scpsys_bus_prot_block block;
+	u32 ofs;
+	u32 val;
+};
+
 /**
  * struct scpsys_domain_data - scp domain data for power on/off flow
  * @name: The name of the power domain.
@@ -165,6 +177,7 @@ enum scpsys_mtcmos_type {
  * @caps: The flag for active wake-up action.
  * @rtff_type: The power domain RTFF HW type
  * @bp_cfg: bus protection configuration for any subsystem
+ * @reg_init: register that loses its setting when the domain is powered off
  */
 struct scpsys_domain_data {
 	const char *name;
@@ -180,6 +193,7 @@ struct scpsys_domain_data {
 	const struct scpsys_bus_prot_data bp_cfg[SPM_MAX_BUS_PROT_DATA];
 	int pwr_sta_offs;
 	int pwr_sta2nd_offs;
+	const struct scpsys_reg_init *reg_init;
 };
 
 /**
-- 
2.56.0

[PATCH v2 3/4] pmdomain: mediatek: mt8188: Enable the GPU timestamp counter

[Michał Kopeć]

The GPU timestamp counter only counts once the TOP_TSVALUEB enable bits
in MFGCFG feed it the SoC system timer, and MFGCFG loses them whenever
the MFG1 power domain is turned off. Without them GPU_TIMESTAMP reads
zero, so timestamp queries in userspace drivers and the timestamps
written by GPU jobs are always zero.

Add MFGCFG as the second MT8188 access controller and set the bits every
time MFG1 has been powered on.

Signed-off-by: Michał Kopeć <michal@nozomi.space>
---
 drivers/pmdomain/mediatek/mt8188-pm-domains.h | 16 +++++++++++++++-
 drivers/pmdomain/mediatek/mtk-pm-domains.h    |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/pmdomain/mediatek/mt8188-pm-domains.h b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
index 3a989e83e9b79..88035cc6a808b 100644
--- a/drivers/pmdomain/mediatek/mt8188-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
@@ -14,8 +14,21 @@
  * MT8188 power domain support
  */
 
+#define MT8188_MFG_TIMESTAMP			0x130
+#define MT8188_MFG_TIMESTAMP_EN			GENMASK(1, 0)
+
 static enum scpsys_bus_prot_block scpsys_bus_prot_blocks_mt8188[] = {
-	BUS_PROT_BLOCK_INFRA
+	BUS_PROT_BLOCK_INFRA, BUS_PROT_BLOCK_MFGCFG
+};
+
+/*
+ * The GPU timestamp counter only counts once MFGCFG feeds it the SoC system
+ * timer, which MFGCFG forgets whenever MFG1 is powered off.
+ */
+static const struct scpsys_reg_init mt8188_mfg_timestamp = {
+	.block = BUS_PROT_BLOCK_MFGCFG,
+	.ofs = MT8188_MFG_TIMESTAMP,
+	.val = MT8188_MFG_TIMESTAMP_EN,
 };
 
 static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
@@ -70,6 +83,7 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
 				    MT8188_TOP_AXI_PROT_EN_SUB_INFRA_VDNR_STA),
 		},
 		.caps = MTK_SCPD_KEEP_DEFAULT_OFF | MTK_SCPD_DOMAIN_SUPPLY,
+		.reg_init = &mt8188_mfg_timestamp,
 	},
 	[MT8188_POWER_DOMAIN_MFG2] = {
 		.name = "mfg2",
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.h b/drivers/pmdomain/mediatek/mtk-pm-domains.h
index 4c2c56addb3cc..191d5cfb60fe7 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.h
@@ -71,6 +71,7 @@ enum scpsys_bus_prot_block {
 	BUS_PROT_BLOCK_CAM_SUB0,
 	BUS_PROT_BLOCK_IPE_SUB0,
 	BUS_PROT_BLOCK_VLP,
+	BUS_PROT_BLOCK_MFGCFG,
 	BUS_PROT_BLOCK_COUNT,
 };
 
-- 
2.56.0

[PATCH v2 4/4] arm64: dts: mediatek: mt8188: Use access-controllers in the power controller

[Michał Kopeć]

List the blocks that the power controller accesses in access-controllers
instead of the deprecated per-domain mediatek,infracfg phandles, and add
MFGCFG, which the power controller needs to enable the GPU timestamp
counter whenever it powers MFG1 on.

Signed-off-by: Michał Kopeć <michal@nozomi.space>
---
 arch/arm64/boot/dts/mediatek/mt8188.dtsi | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8188.dtsi b/arch/arm64/boot/dts/mediatek/mt8188.dtsi
index 14a320f0c70cb..cf0da77c5fd29 100644
--- a/arch/arm64/boot/dts/mediatek/mt8188.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8188.dtsi
@@ -1000,6 +1000,7 @@ scpsys: syscon@10006000 {
 			/* System Power Manager */
 			spm: power-controller {
 				compatible = "mediatek,mt8188-power-controller";
+				access-controllers = <&infracfg_ao>, <&mfgcfg>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				#power-domain-cells = <1>;
@@ -1016,7 +1017,6 @@ mfg1: power-domain@MT8188_POWER_DOMAIN_MFG1 {
 						clocks = <&apmixedsys CLK_APMIXED_MFGPLL>,
 							 <&topckgen CLK_TOP_MFG_CORE_TMP>;
 						clock-names = "mfg", "alt";
-						mediatek,infracfg = <&infracfg_ao>;
 						#address-cells = <1>;
 						#size-cells = <0>;
 						#power-domain-cells = <1>;
@@ -1077,7 +1077,6 @@ power-domain@MT8188_POWER_DOMAIN_VPPSYS0 {
 						      "ss-emi", "ss-subcmn-rdr", "ss-rsi",
 						      "ss-cmn-l4", "ss-vdec1", "ss-wpe",
 						      "ss-cvdo-ve1";
-					mediatek,infracfg = <&infracfg_ao>;
 					#address-cells = <1>;
 					#size-cells = <0>;
 					#power-domain-cells = <1>;
@@ -1096,7 +1095,6 @@ power-domain@MT8188_POWER_DOMAIN_VDOSYS0 {
 						clock-names = "cfgck", "cfgxo", "ss-gals",
 							      "ss-cmn", "ss-emi", "ss-iommu",
 							      "ss-larb", "ss-rsi", "ss-bus";
-						mediatek,infracfg = <&infracfg_ao>;
 						#address-cells = <1>;
 						#size-cells = <0>;
 						#power-domain-cells = <1>;
@@ -1112,7 +1110,6 @@ power-domain@MT8188_POWER_DOMAIN_VPPSYS1 {
 							clock-names = "cfgck", "cfgxo",
 								      "ss-vpp1-g5", "ss-vpp1-g6",
 								      "ss-vpp1-l5", "ss-vpp1-l6";
-							mediatek,infracfg = <&infracfg_ao>;
 							#power-domain-cells = <0>;
 						};
 
@@ -1120,7 +1117,6 @@ power-domain@MT8188_POWER_DOMAIN_VDEC0 {
 							reg = <MT8188_POWER_DOMAIN_VDEC0>;
 							clocks = <&vdecsys_soc CLK_VDEC1_SOC_LARB1>;
 							clock-names = "ss-vdec1-soc-l1";
-							mediatek,infracfg = <&infracfg_ao>;
 							#address-cells = <1>;
 							#size-cells = <0>;
 							#power-domain-cells = <1>;
@@ -1129,7 +1125,6 @@ power-domain@MT8188_POWER_DOMAIN_VDEC1 {
 								reg = <MT8188_POWER_DOMAIN_VDEC1>;
 								clocks = <&vdecsys CLK_VDEC2_LARB1>;
 								clock-names = "ss-vdec2-l1";
-								mediatek,infracfg = <&infracfg_ao>;
 								#power-domain-cells = <0>;
 							};
 						};
@@ -1141,7 +1136,6 @@ cam_vcore: power-domain@MT8188_POWER_DOMAIN_CAM_VCORE {
 								 <&topckgen CLK_TOP_CCU_AHB>,
 								 <&topckgen CLK_TOP_CFGREG_CLOCK_ISP_AXI_GALS>;
 							clock-names = "cam", "ccu", "bus", "cfgck";
-							mediatek,infracfg = <&infracfg_ao>;
 							#address-cells = <1>;
 							#size-cells = <0>;
 							#power-domain-cells = <1>;
@@ -1156,7 +1150,6 @@ power-domain@MT8188_POWER_DOMAIN_CAM_MAIN {
 								clock-names= "ss-cam-l13", "ss-cam-l14",
 									     "ss-cam-mm0", "ss-cam-mm1",
 									     "ss-camsys";
-								mediatek,infracfg = <&infracfg_ao>;
 								#address-cells = <1>;
 								#size-cells = <0>;
 								#power-domain-cells = <1>;
@@ -1194,7 +1187,6 @@ power-domain@MT8188_POWER_DOMAIN_VDOSYS1 {
 								 <&vdosys1 CLK_VDO1_GALS>;
 							clock-names = "cfgck", "cfgxo", "ss-larb2",
 								      "ss-larb3", "ss-gals";
-							mediatek,infracfg = <&infracfg_ao>;
 							#address-cells = <1>;
 							#size-cells = <0>;
 							#power-domain-cells = <1>;
@@ -1204,19 +1196,16 @@ power-domain@MT8188_POWER_DOMAIN_HDMI_TX {
 								clocks = <&topckgen CLK_TOP_HDMI_APB>,
 									 <&topckgen CLK_TOP_HDCP_24M>;
 								clock-names = "bus", "hdcp";
-								mediatek,infracfg = <&infracfg_ao>;
 								#power-domain-cells = <0>;
 							};
 
 							power-domain@MT8188_POWER_DOMAIN_DP_TX {
 								reg = <MT8188_POWER_DOMAIN_DP_TX>;
-								mediatek,infracfg = <&infracfg_ao>;
 								#power-domain-cells = <0>;
 							};
 
 							power-domain@MT8188_POWER_DOMAIN_EDP_TX {
 								reg = <MT8188_POWER_DOMAIN_EDP_TX>;
-								mediatek,infracfg = <&infracfg_ao>;
 								#power-domain-cells = <0>;
 							};
 						};
@@ -1229,7 +1218,6 @@ power-domain@MT8188_POWER_DOMAIN_VENC {
 								 <&vencsys CLK_VENC1_GALS_SRAM>;
 							clock-names = "ss-ve1-larb", "ss-ve1-core",
 								      "ss-ve1-gals", "ss-ve1-sram";
-							mediatek,infracfg = <&infracfg_ao>;
 							#power-domain-cells = <0>;
 						};
 
@@ -1238,7 +1226,6 @@ power-domain@MT8188_POWER_DOMAIN_WPE {
 							clocks = <&wpesys CLK_WPE_TOP_SMI_LARB7>,
 								 <&wpesys CLK_WPE_TOP_SMI_LARB7_PCLK_EN>;
 							clock-names = "ss-wpe-l7", "ss-wpe-l7pce";
-							mediatek,infracfg = <&infracfg_ao>;
 							#power-domain-cells = <0>;
 						};
 					};
@@ -1246,7 +1233,6 @@ power-domain@MT8188_POWER_DOMAIN_WPE {
 
 				power-domain@MT8188_POWER_DOMAIN_PEXTP_MAC_P0 {
 					reg = <MT8188_POWER_DOMAIN_PEXTP_MAC_P0>;
-					mediatek,infracfg = <&infracfg_ao>;
 					clocks = <&pericfg_ao CLK_PERI_AO_PCIE_P0_FMEM>;
 					clock-names = "ss-pextp-fmem";
 					#power-domain-cells = <0>;
@@ -1270,14 +1256,12 @@ power-domain@MT8188_POWER_DOMAIN_ADSP_AO {
 					clocks = <&topckgen CLK_TOP_AUDIO_LOCAL_BUS>,
 						 <&topckgen CLK_TOP_ADSP>;
 					clock-names = "bus", "main";
-					mediatek,infracfg = <&infracfg_ao>;
 					#address-cells = <1>;
 					#size-cells = <0>;
 					#power-domain-cells = <1>;
 
 					power-domain@MT8188_POWER_DOMAIN_ADSP_INFRA {
 						reg = <MT8188_POWER_DOMAIN_ADSP_INFRA>;
-						mediatek,infracfg = <&infracfg_ao>;
 						#address-cells = <1>;
 						#size-cells = <0>;
 						#power-domain-cells = <1>;
@@ -1286,7 +1270,6 @@ power-domain@MT8188_POWER_DOMAIN_AUDIO_ASRC {
 							reg = <MT8188_POWER_DOMAIN_AUDIO_ASRC>;
 							clocks = <&topckgen CLK_TOP_ASM_H>;
 							clock-names = "asm";
-							mediatek,infracfg = <&infracfg_ao>;
 							#power-domain-cells = <0>;
 						};
 
@@ -1296,13 +1279,11 @@ power-domain@MT8188_POWER_DOMAIN_AUDIO {
 								 <&topckgen CLK_TOP_AUD_INTBUS>,
 								 <&adsp_audio26m CLK_AUDIODSP_AUDIO26M>;
 							clock-names = "a1sys", "intbus", "adspck";
-							mediatek,infracfg = <&infracfg_ao>;
 							#power-domain-cells = <0>;
 						};
 
 						power-domain@MT8188_POWER_DOMAIN_ADSP {
 							reg = <MT8188_POWER_DOMAIN_ADSP>;
-							mediatek,infracfg = <&infracfg_ao>;
 							#power-domain-cells = <0>;
 						};
 					};
@@ -1312,7 +1293,6 @@ power-domain@MT8188_POWER_DOMAIN_ETHER {
 					reg = <MT8188_POWER_DOMAIN_ETHER>;
 					clocks = <&pericfg_ao CLK_PERI_AO_ETHERNET_MAC>;
 					clock-names = "ethermac";
-					mediatek,infracfg = <&infracfg_ao>;
 					#power-domain-cells = <0>;
 				};
 			};
-- 
2.56.0

Re: [PATCH v2 2/4] pmdomain: mediatek: Restore a register after powering a domain on

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pmdomain: mediatek: Incomplete error path in scpsys_power_on() fails to unwind hardware state
--

commit f298ac6e69f935973c035a0a1e72ad788976e0f8
Author: Michał Kopeć <michal@nozomi.space>

pmdomain: mediatek: Restore a register after powering a domain on

Some blocks lose a setting when their power domain is turned off, and it
has to be restored every time the domain is powered on again, before
the devices in the domain use it.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index f0fa79e01f9c3..0585a663f48a6 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> @@ -755,6 +757,13 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>  	if (ret < 0)
>  		goto err_disable_sram;
>  
> +	if (pd->init_regmap) {
> +		ret = regmap_write(pd->init_regmap, pd->data->reg_init->ofs,
> +				   pd->data->reg_init->val);
> +		if (ret)
> +			goto err_enable_bus_protect;

[Severity: Medium]
If regmap_write fails here, the error path jumps to err_enable_bus_protect,
which cascades down to err_pwr_ack:

err_enable_bus_protect:
	scpsys_bus_protect_enable(pd, 0);
err_disable_sram:
...
err_pwr_ack:
	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
err_reg:
	scpsys_regulator_disable(pd->supply);
	return ret;

However, err_pwr_ack does not reverse several earlier steps from
scpsys_power_on(). It fails to call scpsys_bus_protect_enable(pd,
BUS_PROT_IGNORE_SUBCLK), does not re-enable ext_buck_iso, and does not invoke
scpsys_ctl_pwrseq_off() (or the other pwrseq variants).

Does this leave the power domain logically powered off in genpd, but physically
powered on and partially unprotected, risking system instability on subsequent
power cycles?

> +	}
> +
>  	if (MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION)) {
>  		ret = clk_bulk_prepare_enable(pd->num_subsys_clks,
>  					      pd->subsys_clks);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011184755.2058017-1-michal@nozomi.space?part=2

Re: [PATCH v2 3/4] pmdomain: mediatek: mt8188: Enable the GPU timestamp counter

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dt-bindings: ABI breakage for MT8188 older kernels via access-controllers
--

commit 67ac5bbd1956b4f1ebf5fa4f2f96f5fd7b231689
Author: Michał Kopeć <michal@nozomi.space>

pmdomain: mediatek: mt8188: Enable the GPU timestamp counter

This commit adds MFGCFG as a second MT8188 access controller to maintain the
GPU timestamp counter. The counter loses its SoC system timer configuration
when the MFG1 power domain is turned off. Setting these bits when MFG1 is
powered on fixes always-zero timestamps for GPU jobs.

> diff --git a/drivers/pmdomain/mediatek/mt8188-pm-domains.h b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
> index 3a989e83e9b79..88035cc6a808b 100644
> --- a/drivers/pmdomain/mediatek/mt8188-pm-domains.h
> +++ b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
> @@ -14,8 +14,21 @@
>   * MT8188 power domain support
>   */
>  
> +#define MT8188_MFG_TIMESTAMP			0x130
> +#define MT8188_MFG_TIMESTAMP_EN			GENMASK(1, 0)
> +
>  static enum scpsys_bus_prot_block scpsys_bus_prot_blocks_mt8188[] = {
> -	BUS_PROT_BLOCK_INFRA
> +	BUS_PROT_BLOCK_INFRA, BUS_PROT_BLOCK_MFGCFG
>  };

[Severity: High]
Can adding a second bus protection block cause a regression on older kernels?

Older kernels hardcode soc->num_bus_prot_blocks to 1 for MT8188. If they boot
with an updated device tree containing two access-controllers handles,
scpsys_get_bus_protection() will fail because of its strict equality check:

drivers/pmdomain/mediatek/mtk-pm-domains.c:scpsys_get_bus_protection() {
    ...
    num_handles = of_count_phandle_with_args(np, "access-controllers", NULL);
    if (num_handles < 0 || num_handles != soc->num_bus_prot_blocks)
        return dev_err_probe(dev, -EINVAL,
                             "Cannot get access controllers: expected %u, got %d\n",
                             soc->num_bus_prot_blocks, num_handles);
    ...
}

Will this strict check return -EINVAL and prevent power domain initialization
entirely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011184755.2058017-1-michal@nozomi.space?part=3