[PATCH 0/4] Fix Google Tensor GS101 CPU hotplug support

[PATCH 0/4] Fix Google Tensor GS101 CPU hotplug support

[Peter Griffin]

Hi folks,

As part of an effort to make suspend to RAM functional upstream on
Pixel 6 I noticed that CPU hotplug leads to a system hang.

After debugging and comparing with downstream drivers it became clear
that some extra register writes are required to make CPU hotplug
functional on these older devices which use the el3mon firmware.

This series adds support for programming the CPU_INFORM register hint
required by the firmware and also adds support for the pmu-intr-gen
register region. This is achieved using cpuhp_setup_state() to setup
a cpu hotplug state. This is similar to soc/xilinx/xlnx_event_manager.c
and soc/fsl/qbman/bman_portal.c drivers.

With these changes CPU hotplug is now functional :)

It can be tested with commands such as

echo 0 > /sys/devices/system/cpu/cpu6/online
echo 1 > /sys/devices/system/cpu/cpu6/online
[   15.880597][    T0] Detected PIPT I-cache on CPU6
[   15.880638][    T0] GICv3: CPU6: found redistributor 600 region 0:0x0000000010500000
[   15.880685][    T0] CPU6: Booted secondary processor 0x0000000600 [0x411fd440]

This would (prior to this series) hang the system.

Note 1: It is highly likely that similar changes are required for other
Exynos based SoCs using el3mon. For anyone following along who is
accustomed to looking at downstream Exynos based drivers this replaces
register writes defined in 

drivers/soc/<google|samsung>/cal-if/<socname>/flexpmu_cal_cpu_<socname>.h

Which are used by files in the cal-if folder and exynos-cpupm.c driver.

For the moment I've used the GS101 CPU inform register offsets directly
but these can be moved to driver data once we've established other SoCs
benefit from this.

Note 2: To ensure older DTs which don't define pmu-intr-gen register
region still work. The driver only issues a warning if the registers
can't be mapped, and the behaviour remains the same as today (system
boots, but CPU hotplug will not be functional).

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
Peter Griffin (4):
      dt-bindings: soc: samsung: exynos-pmu: gs101: add pmu-intr-gen reg region
      dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu
      arm64: dts: exynos: gs101: add pmu-intr-gen regs to the PMU node
      soc: samsung: exynos-pmu: enable CPU hotplug support for gs101

 .../devicetree/bindings/mfd/syscon-common.yaml     | 10 +++
 .../bindings/soc/samsung/exynos-pmu.yaml           | 29 ++++++++-
 arch/arm64/boot/dts/exynos/google/gs101.dtsi       |  4 +-
 drivers/soc/samsung/exynos-pmu.c                   | 73 +++++++++++++++++++++-
 drivers/soc/samsung/exynos-pmu.h                   |  1 +
 include/linux/soc/samsung/exynos-regs-pmu.h        | 11 ++++
 6 files changed, 125 insertions(+), 3 deletions(-)
---
base-commit: ed9a4ad6e5bd3a443e81446476718abebee47e82
change-id: 20241213-contrib-pg-cpu-hotplug-suspend2ram-fixes-v1-1f7ad4c45901

Best regards,
-- 
Peter Griffin <peter.griffin@linaro.org>

[PATCH 1/4] dt-bindings: soc: samsung: exynos-pmu: gs101: add pmu-intr-gen reg region

[Peter Griffin]

gs101 also requires access to the pmu interrupt generation register region.
Update the exynos-pmu bindings documentation to reflect this.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 .../bindings/soc/samsung/exynos-pmu.yaml           | 29 +++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-pmu.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-pmu.yaml
index 6cdfe7e059a3..5ac4864e4cde 100644
--- a/Documentation/devicetree/bindings/soc/samsung/exynos-pmu.yaml
+++ b/Documentation/devicetree/bindings/soc/samsung/exynos-pmu.yaml
@@ -73,7 +73,11 @@ properties:
           - const: syscon
 
   reg:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
+
+  reg-names:
+    maxItems: 2
 
   '#clock-cells':
     const: 1
@@ -186,6 +190,29 @@ allOf:
       properties:
         dp-phy: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - google,gs101-pmu
+    then:
+      properties:
+        reg:
+          items:
+            - description: PMU register region
+            - description: PMU Interrupt Generation register region
+        reg-names:
+          items:
+            - const: pmu
+            - const: pmu-intr-gen
+    else:
+      properties:
+        reg:
+          maxItems: 1
+        reg-name:
+          maxItems: 1
+
 examples:
   - |
     #include <dt-bindings/clock/exynos5250.h>

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH 2/4] dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu

[Peter Griffin]

To avoid dtschema warnings allow google,gs101-pmu to have
two reg regions.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
I don't really like this patch, but also didn't want to submit the series
with a dtschema warning ;-)

Possibly a better solution is when Robs patch
`mfd: syscon: Allow syscon nodes without a "syscon" compatible` [1]

gets updated with a v2, we could remove syscon compatible from
gs101.dtsi (an ABI issue). If I understood his patch correctly,
it would mean this yaml update would then no longer be required.

Let me know your thoughts

[1] https://lore.kernel.org/lkml/20241211-syscon-fixes-v1-0-b5ac8c219e96@kernel.org/T/#m5ad1ed5c69f693d2a5cc54342a87fbdf3df756d2
---
 Documentation/devicetree/bindings/mfd/syscon-common.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon-common.yaml b/Documentation/devicetree/bindings/mfd/syscon-common.yaml
index 451cbad467a3..9cd9739d5e97 100644
--- a/Documentation/devicetree/bindings/mfd/syscon-common.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon-common.yaml
@@ -59,6 +59,16 @@ allOf:
         compatible:
           minItems: 3
           maxItems: 5
+  - if:
+      properties:
+        compatible:
+          contains:
+            const:
+              - google,gs101-pmu
+  then:
+    properties:
+      reg:
+        maxItems: 2
 
 additionalProperties: true
 

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH 3/4] arm64: dts: exynos: gs101: add pmu-intr-gen regs to the PMU node

[Peter Griffin]

These registers are required for cpu hotplug.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 arch/arm64/boot/dts/exynos/google/gs101.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
index 302c5beb224a..93db14e16246 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
+++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
@@ -1393,7 +1393,9 @@ sysreg_apm: syscon@174204e0 {
 
 		pmu_system_controller: system-controller@17460000 {
 			compatible = "google,gs101-pmu", "syscon";
-			reg = <0x17460000 0x10000>;
+			reg = <0x17460000 0x10000>,
+			      <0x17470000 0x10000>;
+			reg-names = "pmu", "pmu-intr-gen";
 
 			poweroff: syscon-poweroff {
 				compatible = "syscon-poweroff";

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH 4/4] soc: samsung: exynos-pmu: enable CPU hotplug support for gs101

[Peter Griffin]

Some additional register writes are required when hotplugging CPUs
on gs101, without these the system hangs when hotplugging.

Specifically a CPU_INFORM register needs to be programmed with
a hint value which is used by the EL3 firmware (el3mon) and the
pmu-intr-gen registers need to be programmed.

With this patch applied, and corresponding DT update CPU hotplug
now works as expected. e.g.

echo 0 > /sys/devices/system/cpu/cpu6/online
echo 1 > /sys/devices/system/cpu/cpu6/online

Note: to maintain compatibility with older DTs that didn't specify
pmu-intr-gen register region only a warning is issued if the
registers can't be mapped, and the old behaviour is maintained.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/soc/samsung/exynos-pmu.c            | 73 ++++++++++++++++++++++++++++-
 drivers/soc/samsung/exynos-pmu.h            |  1 +
 include/linux/soc/samsung/exynos-regs-pmu.h | 11 +++++
 3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index d8c53cec7f37..68eb4eb3813b 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -6,6 +6,7 @@
 // Exynos - CPU PMU(Power Management Unit) support
 
 #include <linux/arm-smccc.h>
+#include <linux/cpuhotplug.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/mfd/core.h>
@@ -32,6 +33,7 @@ struct exynos_pmu_context {
 	struct device *dev;
 	const struct exynos_pmu_data *pmu_data;
 	struct regmap *pmureg;
+	void __iomem *pmuintrgen_base;
 };
 
 void __iomem *pmu_base_addr;
@@ -221,7 +223,8 @@ static const struct regmap_config regmap_smccfg = {
 };
 
 static const struct exynos_pmu_data gs101_pmu_data = {
-	.pmu_secure = true
+	.pmu_secure = true,
+	.pmu_cpuhp = true,
 };
 
 /*
@@ -325,6 +328,52 @@ struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
 
+/*
+ * CPU_INFORM register hint values which are used by
+ * EL3 firmware (el3mon).
+ */
+#define CPU_INFORM_CLEAR	0
+#define CPU_INFORM_C2		1
+
+static int cpuhp_pmu_online(unsigned int cpu)
+{
+	void __iomem *base = pmu_context->pmuintrgen_base;
+	u32 reg;
+	u32 mask;
+
+	/* clear cpu inform hint */
+	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu),
+		     CPU_INFORM_CLEAR);
+
+	mask = (1 << cpu);
+
+	writel(((0 << cpu) & mask), base + GS101_GRP2_INTR_BID_ENABLE);
+
+	reg = readl(base + GS101_GRP2_INTR_BID_UPEND) & mask;
+	writel(reg & mask, base + GS101_GRP2_INTR_BID_CLEAR);
+
+	return 0;
+}
+
+static int cpuhp_pmu_offline(unsigned int cpu)
+{
+	void __iomem *base = pmu_context->pmuintrgen_base;
+	u32 reg, mask;
+
+	/* set cpu inform hint */
+	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu),
+		     CPU_INFORM_C2);
+
+	writel((1 << cpu), base + GS101_GRP2_INTR_BID_ENABLE);
+
+	mask = ((1 << cpu) | (1 << (cpu+8)));
+
+	reg = readl(base + GS101_GRP1_INTR_BID_UPEND) & mask;
+	writel(reg & mask, base + GS101_GRP1_INTR_BID_CLEAR);
+
+	return 0;
+}
+
 static int exynos_pmu_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -377,6 +426,28 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 	pmu_context->pmureg = regmap;
 	pmu_context->dev = dev;
 
+	if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_cpuhp) {
+
+		pmu_context->pmuintrgen_base =
+			devm_platform_ioremap_resource_byname(pdev, "pmu-intr-gen");
+		/*
+		 * To maintain support for older DTs that didn't specify pmu-intr-gen
+		 * register region, just issue a warning rather than fail to probe.
+		 */
+		if (IS_ERR(pmu_context->pmuintrgen_base)) {
+			dev_warn(&pdev->dev,
+				 "failed to map pmu-intr-gen registers\n");
+		} else {
+			cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,
+					"soc/exynos-pmu:prepare",
+					cpuhp_pmu_online, NULL);
+
+			cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+					"soc/exynos-pmu:online",
+					NULL, cpuhp_pmu_offline);
+		}
+	}
+
 	if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_init)
 		pmu_context->pmu_data->pmu_init();
 
diff --git a/drivers/soc/samsung/exynos-pmu.h b/drivers/soc/samsung/exynos-pmu.h
index 0a49a2c9a08e..0938bb4fe15f 100644
--- a/drivers/soc/samsung/exynos-pmu.h
+++ b/drivers/soc/samsung/exynos-pmu.h
@@ -22,6 +22,7 @@ struct exynos_pmu_data {
 	const struct exynos_pmu_conf *pmu_config;
 	const struct exynos_pmu_conf *pmu_config_extra;
 	bool pmu_secure;
+	bool pmu_cpuhp;
 
 	void (*pmu_init)(void);
 	void (*powerdown_conf)(enum sys_powerdown);
diff --git a/include/linux/soc/samsung/exynos-regs-pmu.h b/include/linux/soc/samsung/exynos-regs-pmu.h
index ce1a3790d6fb..0d5a17ea8fb8 100644
--- a/include/linux/soc/samsung/exynos-regs-pmu.h
+++ b/include/linux/soc/samsung/exynos-regs-pmu.h
@@ -658,9 +658,20 @@
 #define EXYNOS5433_PAD_RETENTION_FSYSGENIO_OPTION		(0x32A8)
 
 /* For Tensor GS101 */
+/* PMU ALIVE */
 #define GS101_SYSIP_DAT0					(0x810)
+#define GS101_CPU0_INFORM					(0x860)
+#define GS101_CPU_INFORM(cpu)	\
+			(GS101_CPU0_INFORM + (cpu*4))
 #define GS101_SYSTEM_CONFIGURATION				(0x3A00)
 #define GS101_PHY_CTRL_USB20					(0x3EB0)
 #define GS101_PHY_CTRL_USBDP					(0x3EB4)
 
+/* PMU INTR GEN */
+#define GS101_GRP1_INTR_BID_UPEND				(0x0108)
+#define GS101_GRP1_INTR_BID_CLEAR				(0x010c)
+#define GS101_GRP2_INTR_BID_ENABLE				(0x0200)
+#define GS101_GRP2_INTR_BID_UPEND				(0x0208)
+#define GS101_GRP2_INTR_BID_CLEAR				(0x020c)
+
 #endif /* __LINUX_SOC_EXYNOS_REGS_PMU_H */

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH 2/4] dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu

[Rob Herring (Arm)]

On Fri, 13 Dec 2024 16:44:39 +0000, Peter Griffin wrote:
> To avoid dtschema warnings allow google,gs101-pmu to have
> two reg regions.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> I don't really like this patch, but also didn't want to submit the series
> with a dtschema warning ;-)
> 
> Possibly a better solution is when Robs patch
> `mfd: syscon: Allow syscon nodes without a "syscon" compatible` [1]
> 
> gets updated with a v2, we could remove syscon compatible from
> gs101.dtsi (an ABI issue). If I understood his patch correctly,
> it would mean this yaml update would then no longer be required.
> 
> Let me know your thoughts
> 
> [1] https://lore.kernel.org/lkml/20241211-syscon-fixes-v1-0-b5ac8c219e96@kernel.org/T/#m5ad1ed5c69f693d2a5cc54342a87fbdf3df756d2
> ---
>  Documentation/devicetree/bindings/mfd/syscon-common.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/mfd/syscon-common.yaml:68:3: [error] syntax error: expected <block end>, but found '?' (syntax)
./Documentation/devicetree/bindings/mfd/syscon-common.yaml:69:5: [warning] wrong indentation: expected 6 but found 4 (indentation)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/syscon-common.yaml: ignoring, error parsing file
make[2]: *** Deleting file 'Documentation/devicetree/bindings/mfd/syscon-common.example.dts'
Documentation/devicetree/bindings/mfd/syscon-common.yaml:68:3: expected <block end>, but found '?'
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/mfd/syscon-common.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/mfd/syscon-common.yaml:68:3: expected <block end>, but found '?'
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241213-contrib-pg-cpu-hotplug-suspend2ram-fixes-v1-v1-2-c72978f63713@linaro.org

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.

Re: [PATCH 2/4] dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu

[Rob Herring]

On Fri, Dec 13, 2024 at 04:44:39PM +0000, Peter Griffin wrote:
> To avoid dtschema warnings allow google,gs101-pmu to have
> two reg regions.

It's not a "simple" syscon if you have 2 regions, so put it in its own 
schema doc.

> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> I don't really like this patch, but also didn't want to submit the series
> with a dtschema warning ;-)
> 
> Possibly a better solution is when Robs patch
> `mfd: syscon: Allow syscon nodes without a "syscon" compatible` [1]
> 
> gets updated with a v2, we could remove syscon compatible from
> gs101.dtsi (an ABI issue). If I understood his patch correctly,
> it would mean this yaml update would then no longer be required.

Whether you can tolerate an ABI issue is up to you.

Rob

Re: [PATCH 2/4] dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu

[Rob Herring]

On Mon, Dec 16, 2024 at 8:19 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Dec 13, 2024 at 04:44:39PM +0000, Peter Griffin wrote:
> > To avoid dtschema warnings allow google,gs101-pmu to have
> > two reg regions.
>
> It's not a "simple" syscon if you have 2 regions, so put it in its own
> schema doc.

NM, I see it already does. If you keep 'syscon', then 'maxItems: 1'
will probably need to move to syscon.yaml.

Rob

Re: [PATCH 2/4] dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu

[Krzysztof Kozlowski]

On 13/12/2024 17:44, Peter Griffin wrote:
> To avoid dtschema warnings allow google,gs101-pmu to have
> two reg regions.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> I don't really like this patch, but also didn't want to submit the series
> with a dtschema warning ;-)
> 
> Possibly a better solution is when Robs patch
> `mfd: syscon: Allow syscon nodes without a "syscon" compatible` [1]

PMU which spans over two blocks is not a simple syscon. These would be
two syscon devices.

If you request regmap from such syscon, which regmap you get?

I am not sure whether the PMU is really split here. Usually the main PMU
was only one and additional blocks called PMU were somehow specialized
per each IP block.

Maybe you have here two devices, maybe only one. If it is only one, then
it is not a syscon anymore, IMO.



Best regards,
Krzysztof

Re: [PATCH 4/4] soc: samsung: exynos-pmu: enable CPU hotplug support for gs101

[Krzysztof Kozlowski]

On 13/12/2024 17:44, Peter Griffin wrote:
>  /*
> @@ -325,6 +328,52 @@ struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
>  }
>  EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
>  
> +/*
> + * CPU_INFORM register hint values which are used by
> + * EL3 firmware (el3mon).
> + */
> +#define CPU_INFORM_CLEAR	0
> +#define CPU_INFORM_C2		1
> +
> +static int cpuhp_pmu_online(unsigned int cpu)

exynos_cpuhp_pmu_online
or gs101_cpuhp_pmu_online
same for offline

> +{
> +	void __iomem *base = pmu_context->pmuintrgen_base;
> +	u32 reg;
> +	u32 mask;
> +
> +	/* clear cpu inform hint */
> +	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu),
> +		     CPU_INFORM_CLEAR);
> +
> +	mask = (1 << cpu);

BIT(cpu)

> +
> +	writel(((0 << cpu) & mask), base + GS101_GRP2_INTR_BID_ENABLE);

I am not sure if I follow. You want to zero-out all other bits or enable
all other bits?

> +
> +	reg = readl(base + GS101_GRP2_INTR_BID_UPEND) & mask;
> +	writel(reg & mask, base + GS101_GRP2_INTR_BID_CLEAR);

reg is &mask twice.

I don't follow this either, are these auto-cleared? It feels like you
wanted to update some bits, but you are updating entire registers in
both cases.


> +
> +	return 0;
> +}
> +
> +static int cpuhp_pmu_offline(unsigned int cpu)
> +{
> +	void __iomem *base = pmu_context->pmuintrgen_base;
> +	u32 reg, mask;
> +
> +	/* set cpu inform hint */
> +	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu),
> +		     CPU_INFORM_C2);
> +
> +	writel((1 << cpu), base + GS101_GRP2_INTR_BID_ENABLE);
> +
> +	mask = ((1 << cpu) | (1 << (cpu+8)));

What does 8 stands for?

> +
> +	reg = readl(base + GS101_GRP1_INTR_BID_UPEND) & mask;
> +	writel(reg & mask, base + GS101_GRP1_INTR_BID_CLEAR);
> +
> +	return 0;
> +}
> +
>  static int exynos_pmu_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -377,6 +426,28 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>  	pmu_context->pmureg = regmap;
>  	pmu_context->dev = dev;
>  
> +	if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_cpuhp) {
> +

Drop blank line

> +		pmu_context->pmuintrgen_base =
> +			devm_platform_ioremap_resource_byname(pdev, "pmu-intr-gen");
> +		/*
> +		 * To maintain support for older DTs that didn't specify pmu-intr-gen
> +		 * register region, just issue a warning rather than fail to probe.
> +		 */
> +		if (IS_ERR(pmu_context->pmuintrgen_base)) {
> +			dev_warn(&pdev->dev,
> +				 "failed to map pmu-intr-gen registers\n");

Test old DTS, I think you do write() to the ERR_PTR when
offlining/onlining...

> +		} else {
> +			cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,
> +					"soc/exynos-pmu:prepare",
> +					cpuhp_pmu_online, NULL);
> +
> +			cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +					"soc/exynos-pmu:online",
> +					NULL, cpuhp_pmu_offline);
> +		}
> +	}
> +
>  	if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_init)
>  		pmu_context->pmu_data->pmu_init();
>  



Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu

[Peter Griffin]

Hi Krzysztof,

Thanks for your review feedback, it is much appreciated!

On Sun, 22 Dec 2024 at 14:24, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 13/12/2024 17:44, Peter Griffin wrote:
> > To avoid dtschema warnings allow google,gs101-pmu to have
> > two reg regions.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> > I don't really like this patch, but also didn't want to submit the series
> > with a dtschema warning ;-)
> >
> > Possibly a better solution is when Robs patch
> > `mfd: syscon: Allow syscon nodes without a "syscon" compatible` [1]
>
> PMU which spans over two blocks is not a simple syscon. These would be
> two syscon devices.
>
> If you request regmap from such syscon, which regmap you get?

That is a good point, if other drivers in the future need access to
the pmu-intr-gen registers then it would be good if this was modelled
as its own syscon device.

Another point to note is that only the PMU registers need the custom
regmap registered in exynos-pmu, the PMU_INTR_GEN register region
works with normal syscon / mmio accesses, so it would be a different
regmap.

>
> I am not sure whether the PMU is really split here. Usually the main PMU
> was only one and additional blocks called PMU were somehow specialized
> per each IP block.

PMU_INTR_GEN has its own entry in the memory map, so in that respect
it's a "device" (it has its own 65k SFR region).

PMU: Base Address 0x1746_0000
PMU_INTR_GEN: Base Address 0x1747_0000

The documentation isn't particularly detailed on PMU_INTR_GEN. In one
place it says "One PMU interrupt generator for handshaking between PMU
through interrupts". In another, "PMU and PMU_INTR_GEN are for Power
management." and then we have the register names where the description
it really an expanded version of the register name

e.g.
Register: GRP#_INTR_BID_ENABLE
Description: Interrupt Bid Enable
Reset Value: 0x0

Things might be a bit clearer if I had access to the firmware code on
the other side of this PMU handshaking which I believe is the APM, but
sadly I don't.

>
> Maybe you have here two devices, maybe only one. If it is only one, then
> it is not a syscon anymore, IMO.

I was going to suggest modelling PMU_INTR_GEN as its own sycon node,
and then either: -

1) Updating exynos-pmu driver to additionally take a phandle to
pmu-intr-gen syscon, and register the hotplug callbacks.

or

2) Create a new driver named something like exynos-pm or exynos-cpupm
which obtains the PMU regmap and also a phandle to PMU_INTR_GEN
syscon, and register the call backs.

Is there any preference from your side over approach 1 or 2, or maybe
something else entirely?

Thanks,

Peter

Re: [PATCH 2/4] dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu

[Krzysztof Kozlowski]

On 30/12/2024 10:10, Peter Griffin wrote:
> 
>>
>> Maybe you have here two devices, maybe only one. If it is only one, then
>> it is not a syscon anymore, IMO.
> 
> I was going to suggest modelling PMU_INTR_GEN as its own sycon node,
> and then either: -
> 
> 1) Updating exynos-pmu driver to additionally take a phandle to
> pmu-intr-gen syscon, and register the hotplug callbacks.
> 
> or
> 
> 2) Create a new driver named something like exynos-pm or exynos-cpupm
> which obtains the PMU regmap and also a phandle to PMU_INTR_GEN
> syscon, and register the call backs.
> 
> Is there any preference from your side over approach 1 or 2, or maybe
> something else entirely?

No preference, choose whatever results in simpler or more readable code.

Option 1 assumes that exynos-pmu on GS101 will drop the "syscon"
compatible, although it still might expose syscon through drivers. Just
the standard binding syscon does not feel fit here.

I don't have the hardware/user manual, so I don't know what PMU_INTR_GEN
really is. GS downstream code has something like PMUCAL, which looks
like separate device.

Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu

[Peter Griffin]

Hi Krzysztof,

Thanks for your feedback!

On Fri, 3 Jan 2025 at 17:14, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 30/12/2024 10:10, Peter Griffin wrote:
> >
> >>
> >> Maybe you have here two devices, maybe only one. If it is only one, then
> >> it is not a syscon anymore, IMO.
> >
> > I was going to suggest modelling PMU_INTR_GEN as its own sycon node,
> > and then either: -
> >
> > 1) Updating exynos-pmu driver to additionally take a phandle to
> > pmu-intr-gen syscon, and register the hotplug callbacks.
> >
> > or
> >
> > 2) Create a new driver named something like exynos-pm or exynos-cpupm
> > which obtains the PMU regmap and also a phandle to PMU_INTR_GEN
> > syscon, and register the call backs.
> >
> > Is there any preference from your side over approach 1 or 2, or maybe
> > something else entirely?
>
> No preference, choose whatever results in simpler or more readable code.
>
> Option 1 assumes that exynos-pmu on GS101 will drop the "syscon"
> compatible, although it still might expose syscon through drivers. Just
> the standard binding syscon does not feel fit here.

I agree we should drop syscon compatible for gs101 as it requires a
"special" regmap. However other Exynos based SoCs will likely want to
re-use this pmu_intr_gen CPU pm code and they will likely have syscon
compatible in their exynos-pmu node (as protecting PMU registers from
Linux AFAIK was a Google hardening measure).

So just to clarify, dropping syscon compatible on option 1 is because
it's gs101 "special" regmap, or because exynos-pmu node now references
additional pmu_intr_gen syscon?

>
> I don't have the hardware/user manual, so I don't know what PMU_INTR_GEN
> really is.

There isn't much description in the manual, but AIUI pmu_intr_gen is
just a way for the OS to trigger an interrupt so that the APM programs
the PMU registers instead of the OS programming PMU registers
directly. It looks like the system could also be configured to not use
APM (it would need different firmware), in which case the OS would
just program PMU registers directly.

I only see these GRP(x)_INTR_BID_ENABLE / GRP(x)_INTR_BID_CLEAR
registers mentioned in downstream code in the context of
flexpmu_cal_system_gs101.h (which is basically lists of registers to
program for different power/sleep modes - which looks like what
exynos-pmu is currently doing for older chipsets) and
flexpmu_cal_cpu_gs101.h (which is used for cpu on/off) related things.

So even if I split the CPU pm parts into a separate driver, it looks
like programming pmu-intr-gen regs would still be required to
enter/exit sleep modes.

With that in mind I think it seems more natural to grow the exynos-pmu
node & driver.

> GS downstream code has something like PMUCAL, which looks
> like separate device.

PMUCAL is just the PMU registers with a whole bunch of layering. I
believe CAL just stands for Cpu Abstraction Layer and seems to be used
in downstream Samsung driver code when they have a bunch of "generic"
driver code and then what appears to be a lot of automatically
generated header files for a particular SoC for reading/writing all
the SFRs.

The CAL suffix is used for PMU and also for clocks (CMU). Most of the
PMUCAL code is just accessing PMU and pmu-intr-gen registers (if APM
is configured). There are some places like flexpmu_cal_system_gs101.h
where it seems to be used as a convenient place to read/write
registers all over the SoC memory map (CMU, SYSREG* etc). So unpicking
that into all the various subsystems will be interesting.

Thanks,

Peter