JH7110 PMU Support

Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.

Signed-off-by Walker Chen <[email protected]>

---
 .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
 .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml

 create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h

Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml new file +8 -0

new file mode 100644
index 000000000000..f308ae136a57
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/starfive,jh71xx-pmu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH71xx Power Management Unit
+

Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml new file +7 -0

+          - starfive,jh7110-pmu
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1

Quoted 8 lines · Walker Chen

Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.

Signed-off-by Walker Chen <[email protected]>

---
 .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
 .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml

Filename matching compatible, so:
starfive,jh7110-pmu.yaml

Quoted 18 lines

 create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h

Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml new file +8 -0

new file mode 100644
index 000000000000..f308ae136a57
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/starfive,jh71xx-pmu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH71xx Power Management Unit
+

Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml line 18

Quoted 10 lines

Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml new file +10 -0

+maintainers:
+  - Walker Chen <[email protected]>
+
+description: |
+  StarFive JH71xx SoCs include support for multiple power domains which can be
+  powered on/off by software based on different application scenes to save power.
+
+properties:
+  compatible:
+      - enum:

Wrong indentation.

Does not look like you tested the bindings. Please run make dt_binding_check (see Documentation/devicetree/bindings/writing-schema.rst for instructions).

Quoted 8 lines

Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml new file +7 -0

+          - starfive,jh7110-pmu
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1

Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml line 35

Quoted 10 lines

Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml new file +10 -0

+
+  "#power-domain-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#power-domain-cells"
+

Best regards,
Krzysztof

Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.

Signed-off-by Walker Chen <[email protected]>

Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml new file +1 -0

+    };

include/dt-bindings/power/starfive,jh7110-pmu.h new file +0 -0

new file mode 100644
index 000000000000..73c6a79a2181

include/dt-bindings/power/starfive,jh7110-pmu.h new file +6 -0

+/*
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ * Author: Walker Chen <[email protected]>
+ */
+#ifndef __DT_BINDINGS_POWER_JH7110_POWER_H__
+#define __DT_BINDINGS_POWER_JH7110_POWER_H__

Quoted 3 lines · Walker Chen

Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.

Signed-off-by Walker Chen <[email protected]>

include/dt-bindings/power/starfive,jh7110-pmu.h line 1

Quoted 8 lines

Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml new file +1 -0

+    };

include/dt-bindings/power/starfive,jh7110-pmu.h new file +1 -0

new file mode 100644
index 000000000000..73c6a79a2181
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */

Why different license than the bindings? MIT is pretty compatible, but if it does not matter for you, keep same licenses as bindings.

Quoted 7 lines

include/dt-bindings/power/starfive,jh7110-pmu.h new file +6 -0

+/*
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ * Author: Walker Chen <[email protected]>
+ */
+#ifndef __DT_BINDINGS_POWER_JH7110_POWER_H__
+#define __DT_BINDINGS_POWER_JH7110_POWER_H__

include/dt-bindings/power/starfive,jh7110-pmu.h line 17

Quoted 10 lines

include/dt-bindings/power/starfive,jh7110-pmu.h new file +10 -0

+
+#define JH7110_PD_SYSTOP	0
+#define JH7110_PD_CPU		1
+#define JH7110_PD_GPUA		2
+#define JH7110_PD_VDEC		3
+#define JH7110_PD_VOUT		4
+#define JH7110_PD_ISP		5
+#define JH7110_PD_VENC		6
+
+#endif

Best regards,
Krzysztof

Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.

Signed-off-by Walker Chen <[email protected]>

---
 .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
 .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
 create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h

Quoted 10 lines · Walker Chen

Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.

Signed-off-by Walker Chen <[email protected]>

---
 .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
 .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
 create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h

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/starfive,jh71xx-pmu.yaml:18:7: [warning] wrong indentation: expected 4 but found 6 (indentation)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml: properties:compatible: [{'enum': ['starfive,jh7110-pmu']}] is not of type 'object', 'boolean'
from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml: ignoring, error in schema: properties: compatible Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.example.dtb:0:0: /example-0/power-controller@17030000: failed to match any schema with compatible: ['starfive,jh7110-pmu']

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.

Add pmu driver for the StarFive JH71XX SoC.

As the power domains provider, the Power Management Unit (PMU) is designed for including multiple PM domains that can be used for power gating of selected IP blocks for power saving by reduced leakage current. It accepts software encourage command to switch the power mode of SoC.

Signed-off-by Walker Chen <[email protected]>

---
 MAINTAINERS                       |  14 ++
 drivers/soc/Kconfig               |   1 +
 drivers/soc/Makefile              |   1 +
 drivers/soc/starfive/Kconfig      |  11 +
 drivers/soc/starfive/Makefile     |   3 +
 drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
 6 files changed, 426 insertions(+)
 create mode 100644 drivers/soc/starfive/Kconfig
 create mode 100644 drivers/soc/starfive/Makefile
 create mode 100644 drivers/soc/starfive/jh71xx_pmu.c

drivers/soc/starfive/Kconfig new file +6 -0

+	default SOC_STARFIVE
+	select PM_GENERIC_DOMAINS
+	help
+	  Say 'y' here to enable support power domain support.
+	  In order to meet low power requirements, a Power Management Unit (PMU)
+	  is designed for controlling power resources in StarFive JH71XX SoCs.

drivers/soc/starfive/Makefile new file +3 -0

new file mode 100644
index 000000000000..13b589d6b5f3
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_JH71XX_PMU)	+= jh71xx_pmu.o

drivers/soc/starfive/jh71xx_pmu.c new file +50 -0

new file mode 100644
index 000000000000..7a0145779e07
@@ -0,0 +1,396 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * StarFive JH71XX PMU (Power Management Unit) Controller Driver
+ *
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <dt-bindings/power/starfive,jh7110-pmu.h>
+
+/* register offset */
+#define JH71XX_PMU_HW_EVENT_ON		0x04
+#define JH71XX_PMU_HW_EVENT_OFF		0x08
+#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
+#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
+#define JH71XX_PMU_SW_ENCOURAGE		0x44
+#define JH71XX_PMU_INT_MASK		0x48
+#define JH71XX_PMU_PCH_BYPASS		0x4C
+#define JH71XX_PMU_PCH_PSTATE		0x50
+#define JH71XX_PMU_PCH_TIMEOUT		0x54
+#define JH71XX_PMU_LP_TIMEOUT		0x58
+#define JH71XX_PMU_HW_TURN_ON		0x5C
+#define JH71XX_PMU_CURR_POWER_MODE	0x80
+#define JH71XX_PMU_EVENT_STATUS		0x88
+#define JH71XX_PMU_INT_STATUS		0x8C
+
+/* sw encourage cfg */
+#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
+#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
+#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF
+
+/* pmu int status */
+#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
+#define JH71XX_PMU_INT_HW_REQ		BIT(1)
+#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
+#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
+#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
+#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
+					JH71XX_PMU_INT_HW_FAIL | \
+					JH71XX_PMU_INT_PCH_FAIL)
+#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \

drivers/soc/starfive/jh71xx_pmu.c new file +6 -0

+};
+
+struct jh71xx_pmu {
+	struct device *dev;
+	const struct jh71xx_pmu_match_data *match_data;
+	void __iomem *base;

drivers/soc/starfive/jh71xx_pmu.c new file +37 -0

+};
+
+static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+
+	if (!mask) {
+		*is_on = false;
+		return -EINVAL;
+	}
+
+	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
+
+	return 0;
+}
+
+static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+	unsigned long flags;
+	u32 val;
+	u32 mode;
+	u32 encourage_lo;
+	u32 encourage_hi;
+	bool is_on;
+	int ret;
+
+	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
+	if (ret) {
+		dev_dbg(pmu->dev, "unable to get current state for %s\n",
+			pmd->genpd.name);
+		return ret;
+	}
+
+	if (is_on == on) {
+		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
+			pmd->genpd.name, on ? "en" : "dis");

drivers/soc/starfive/jh71xx_pmu.c new file +9 -0

+	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
+	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
 
 
+	 */
+	if (on) {
+		mode = JH71XX_PMU_SW_TURN_ON_POWER;
+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
+	} else {
+		mode = JH71XX_PMU_SW_TURN_OFF_POWER;

drivers/soc/starfive/jh71xx_pmu.c new file +38 -0

+	 *
+	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
+	 * the purpose is to reset the state machine which is going to parse instruction
+	 *  sequence. It has to be written every time.
+	 */
+	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	/* Wait for the power domain bit to be enabled / disabled */
+	if (on) {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, val & mask,
+						1, JH71XX_PMU_TIMEOUT_US);
+	} else {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, !(val & mask),
+						1, JH71XX_PMU_TIMEOUT_US);
+	}
+
+	if (ret) {
+		dev_err(pmu->dev, "%s: failed to power %s\n",
+			pmd->genpd.name, on ? "on" : "off");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
 
+static int jh71xx_pmu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	const struct jh71xx_pmu_match_data *match_data;
+	struct jh71xx_pmu *pmu;
+	unsigned int i;
+	int ret;

drivers/soc/starfive/jh71xx_pmu.c new file +97 -0

+
+	match_data = of_device_get_match_data(dev);
+	if (!match_data)
+		return -EINVAL;
+
+	pmu->genpd = devm_kcalloc(dev, match_data->num_domains,
+				  sizeof(struct generic_pm_domain *),
+				  GFP_KERNEL);
+	if (!pmu->genpd)
+		return -ENOMEM;
+
+	pmu->dev = dev;
+	pmu->match_data = match_data;
+	pmu->genpd_data.domains = pmu->genpd;
+	pmu->genpd_data.num_domains = match_data->num_domains;
+
+	for (i = 0; i < match_data->num_domains; i++) {
+		ret = jh71xx_pmu_init_domain(pmu, i);
+		if (ret) {
+			dev_err(dev, "failed to initialize power domain\n");
+			return ret;
+		}
+	}
+
+	spin_lock_init(&pmu->lock);
+	jh71xx_pmu_int_enable(pmu, JH71XX_PMU_INT_ALL_MASK & ~JH71XX_PMU_INT_PCH_FAIL, true);
+
+	ret = of_genpd_add_provider_onecell(np, &pmu->genpd_data);
+	if (ret) {
+		dev_err(dev, "failed to register genpd driver: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(dev, "registered %u power domains\n", i);
+
+	return 0;
+}
+
+static const struct jh71xx_domain_info jh7110_power_domains[] = {
+	[JH7110_PD_SYSTOP] = {
+		.name = "SYSTOP",
+		.bit = 0,
+		.flags = GENPD_FLAG_ALWAYS_ON,
+	},
+	[JH7110_PD_CPU] = {
+		.name = "CPU",
+		.bit = 1,
+		.flags = GENPD_FLAG_ALWAYS_ON,
+	},
+	[JH7110_PD_GPUA] = {
+		.name = "GPUA",
+		.bit = 2,
+	},
+	[JH7110_PD_VDEC] = {
+		.name = "VDEC",
+		.bit = 3,
+	},
+	[JH7110_PD_VOUT] = {
+		.name = "VOUT",
+		.bit = 4,
+	},
+	[JH7110_PD_ISP] = {
+		.name = "ISP",
+		.bit = 5,
+	},
+	[JH7110_PD_VENC] = {
+		.name = "VENC",
+		.bit = 6,
+	},
+};
+
+static const struct jh71xx_pmu_match_data jh7110_pmu = {
+	.num_domains = ARRAY_SIZE(jh7110_power_domains),
+	.domain_info = jh7110_power_domains,
+};
+
+static const struct of_device_id jh71xx_pmu_of_match[] = {
+	{
+		.compatible = "starfive,jh7110-pmu",
+		.data = (void *)&jh7110_pmu,
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct platform_driver jh71xx_pmu_driver = {
+	.driver = {
+		.name = "jh71xx-pmu",
+		.of_match_table = jh71xx_pmu_of_match,
+	},
+	.probe  = jh71xx_pmu_probe,
+};
+builtin_platform_driver(jh71xx_pmu_driver);
+
+MODULE_AUTHOR("Walker Chen <[email protected]>");
+MODULE_DESCRIPTION("StarFive JH71XX PMU Driver");
+MODULE_LICENSE("GPL");

Signature · 1 line

2.17.1

Hey Walker,

Hopefully just some minor bits here. Hopefully either Emil who has a board, or someone that knows power management stuff better can give this a proper review.

Quoted 21 lines · Walker Chen

Add pmu driver for the StarFive JH71XX SoC.

As the power domains provider, the Power Management Unit (PMU) is designed for including multiple PM domains that can be used for power gating of selected IP blocks for power saving by reduced leakage current. It accepts software encourage command to switch the power mode of SoC.

Signed-off-by Walker Chen <[email protected]>

---
 MAINTAINERS                       |  14 ++
 drivers/soc/Kconfig               |   1 +
 drivers/soc/Makefile              |   1 +
 drivers/soc/starfive/Kconfig      |  11 +
 drivers/soc/starfive/Makefile     |   3 +
 drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
 6 files changed, 426 insertions(+)
 create mode 100644 drivers/soc/starfive/Kconfig
 create mode 100644 drivers/soc/starfive/Makefile
 create mode 100644 drivers/soc/starfive/jh71xx_pmu.c

drivers/soc/starfive/Kconfig line 5

Quoted 3 lines

drivers/soc/starfive/Kconfig new file +3 -0

+config JH71XX_PMU
+	bool "Support PMU for StarFive JH71XX Soc"
+	depends on PM && (SOC_STARFIVE || COMPILE_TEST)

Why not just do:

	depends on PM
	depends on SOC_STARFIVE || COMPILE_TEST

I think that way reads a little better.

Quoted 72 lines

drivers/soc/starfive/Kconfig new file +6 -0

+	default SOC_STARFIVE
+	select PM_GENERIC_DOMAINS
+	help
+	  Say 'y' here to enable support power domain support.
+	  In order to meet low power requirements, a Power Management Unit (PMU)
+	  is designed for controlling power resources in StarFive JH71XX SoCs.

drivers/soc/starfive/Makefile new file +3 -0

new file mode 100644
index 000000000000..13b589d6b5f3
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_JH71XX_PMU)	+= jh71xx_pmu.o

drivers/soc/starfive/jh71xx_pmu.c new file +50 -0

new file mode 100644
index 000000000000..7a0145779e07
@@ -0,0 +1,396 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * StarFive JH71XX PMU (Power Management Unit) Controller Driver
+ *
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <dt-bindings/power/starfive,jh7110-pmu.h>
+
+/* register offset */
+#define JH71XX_PMU_HW_EVENT_ON		0x04
+#define JH71XX_PMU_HW_EVENT_OFF		0x08
+#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
+#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
+#define JH71XX_PMU_SW_ENCOURAGE		0x44
+#define JH71XX_PMU_INT_MASK		0x48
+#define JH71XX_PMU_PCH_BYPASS		0x4C
+#define JH71XX_PMU_PCH_PSTATE		0x50
+#define JH71XX_PMU_PCH_TIMEOUT		0x54
+#define JH71XX_PMU_LP_TIMEOUT		0x58
+#define JH71XX_PMU_HW_TURN_ON		0x5C
+#define JH71XX_PMU_CURR_POWER_MODE	0x80
+#define JH71XX_PMU_EVENT_STATUS		0x88
+#define JH71XX_PMU_INT_STATUS		0x8C
+
+/* sw encourage cfg */
+#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
+#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
+#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF
+
+/* pmu int status */
+#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
+#define JH71XX_PMU_INT_HW_REQ		BIT(1)
+#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
+#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
+#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
+#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
+					JH71XX_PMU_INT_HW_FAIL | \
+					JH71XX_PMU_INT_PCH_FAIL)
+#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \

drivers/soc/starfive/jh71xx_pmu.c line 60

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+					JH71XX_PMU_INT_HW_REQ | \
+					JH71XX_PMU_INT_FAIL_MASK)
+
+/*
+ * The time required for switching power status is based on the time
+ * to turn on the largest domain's power, which is at microsecond level
+ */
+#define JH71XX_PMU_TIMEOUT_US		100
+
+struct jh71xx_domain_info {

	const char * const name;
	unsigned int flags;
	u8 bit;

drivers/soc/starfive/jh71xx_pmu.c line 66

Quoted 3 lines

drivers/soc/starfive/jh71xx_pmu.c new file +3 -0

+};
+
+struct jh71xx_pmu_match_data {

	const struct jh71xx_domain_info *domain_info;
	int num_domains;

Can you switch these two around like so?

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+};
+
+struct jh71xx_pmu {
+	struct device *dev;
+	const struct jh71xx_pmu_match_data *match_data;
+	void __iomem *base;
+	spinlock_t lock;	/* protects pmu reg */
+	int irq;
+	struct genpd_onecell_data genpd_data;
+	struct generic_pm_domain **genpd;

drivers/soc/starfive/jh71xx_pmu.c line 84

Quoted 6 lines

drivers/soc/starfive/jh71xx_pmu.c new file +6 -0

+};
+
+struct jh71xx_pmu_dev {
+	struct generic_pm_domain genpd;
+	const struct jh71xx_domain_info *domain_info;
+	struct jh71xx_pmu *pmu;

And these two too please in the same way.

Quoted 38 lines

drivers/soc/starfive/jh71xx_pmu.c new file +37 -0

+};
+
+static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+
+	if (!mask) {
+		*is_on = false;
+		return -EINVAL;
+	}
+
+	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
+
+	return 0;
+}
+
+static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+	unsigned long flags;
+	u32 val;
+	u32 mode;
+	u32 encourage_lo;
+	u32 encourage_hi;
+	bool is_on;
+	int ret;
+
+	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
+	if (ret) {
+		dev_dbg(pmu->dev, "unable to get current state for %s\n",
+			pmd->genpd.name);
+		return ret;
+	}
+
+	if (is_on == on) {
+		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
+			pmd->genpd.name, on ? "en" : "dis");

drivers/soc/starfive/jh71xx_pmu.c line 131

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+		return 0;
+	}
+
+	spin_lock_irqsave(&pmu->lock, flags);
+
+	/*
+	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
+	 *
+	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
+	 *    the bit which power domain will be turn-on, write 0 to the others.

Some grammatical nit picking..

"Configure the register blah (offset 0x0c) by writing 1 to the bit corresponding to the power domain that will be turned on and writing zero to the others."

Is that a correct correct summation of the operation?

Quoted 12 lines

drivers/soc/starfive/jh71xx_pmu.c new file +9 -0

+	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
+	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
 
 
+	 */
+	if (on) {
+		mode = JH71XX_PMU_SW_TURN_ON_POWER;
+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
+	} else {
+		mode = JH71XX_PMU_SW_TURN_OFF_POWER;

drivers/soc/starfive/jh71xx_pmu.c line 150

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
+	}
+
+	writel(mask, pmu->base + mode);
+
+	/*
+	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
+	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
+	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0

I think you could replace the hard "coded" numbers here with a better description idk without looking at the #defines above what these correspond to. AFAICT, it'd be something like:
First write the ...ENCOURAGE_ON to reset the state machine which parses the command sequence. It must be written every time.
Then write the lower bits of the command sequence, followed by the upper bits. The sequence differs between powering on & off a domain.

Quoted 43 lines

drivers/soc/starfive/jh71xx_pmu.c new file +42 -0

+	 *
+	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
+	 * the purpose is to reset the state machine which is going to parse instruction
+	 *  sequence. It has to be written every time.
+	 */
+	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	/* Wait for the power domain bit to be enabled / disabled */
+	if (on) {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, val & mask,
+						1, JH71XX_PMU_TIMEOUT_US);
+	} else {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, !(val & mask),
+						1, JH71XX_PMU_TIMEOUT_US);
+	}
+
+	if (ret) {
+		dev_err(pmu->dev, "%s: failed to power %s\n",
+			pmd->genpd.name, on ? "on" : "off");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
 
+static int jh71xx_pmu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	const struct jh71xx_pmu_match_data *match_data;
+	struct jh71xx_pmu *pmu;
+	unsigned int i;
+	int ret;
+
+	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
+	if (!pmu)
+		return -ENOMEM;

drivers/soc/starfive/jh71xx_pmu.c line 291

Quoted 6 lines

drivers/soc/starfive/jh71xx_pmu.c new file +6 -0

+
+	pmu->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pmu->base))
+		return PTR_ERR(pmu->base);
+
+	/* initialize pmu interrupt  */

nit: this comment is ~pointless.

drivers/soc/starfive/jh71xx_pmu.c line 299

Quoted 9 lines

drivers/soc/starfive/jh71xx_pmu.c new file +8 -0

+	pmu->irq = platform_get_irq(pdev, 0);
+	if (pmu->irq < 0)
+		return pmu->irq;
+
+	ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
+			       0, pdev->name, pmu);
+	if (ret)
+		dev_err(dev, "request irq failed.\n");

nit: s/request/requesting

Unfortunately I cannot really review the rest of this, but hopefully I'll get a board soon and can try it out - or else send me a link to your TRM or w/e.

Quoted 29 lines · Conor Dooley

Hey Walker,

Hopefully just some minor bits here. Hopefully either Emil who has a board, or someone that knows power management stuff better can give this a proper review.

Quoted 21 lines · Walker Chen

Add pmu driver for the StarFive JH71XX SoC.

As the power domains provider, the Power Management Unit (PMU) is designed for including multiple PM domains that can be used for power gating of selected IP blocks for power saving by reduced leakage current. It accepts software encourage command to switch the power mode of SoC.

Signed-off-by Walker Chen <[email protected]>

---
 MAINTAINERS                       |  14 ++
 drivers/soc/Kconfig               |   1 +
 drivers/soc/Makefile              |   1 +
 drivers/soc/starfive/Kconfig      |  11 +
 drivers/soc/starfive/Makefile     |   3 +
 drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
 6 files changed, 426 insertions(+)
 create mode 100644 drivers/soc/starfive/Kconfig
 create mode 100644 drivers/soc/starfive/Makefile
 create mode 100644 drivers/soc/starfive/jh71xx_pmu.c

Quoted 8 lines

drivers/soc/starfive/Kconfig line 5

Quoted 3 lines

drivers/soc/starfive/Kconfig new file +3 -0

+config JH71XX_PMU
+	bool "Support PMU for StarFive JH71XX Soc"
+	depends on PM && (SOC_STARFIVE || COMPILE_TEST)

Why not just do:

	depends on PM
	depends on SOC_STARFIVE || COMPILE_TEST

I think that way reads a little better.

No problem, will be changed like this way.

Quoted 34 lines

Quoted 34 lines

drivers/soc/starfive/Kconfig new file +6 -0

+	default SOC_STARFIVE
+	select PM_GENERIC_DOMAINS
+	help
+	  Say 'y' here to enable support power domain support.
+	  In order to meet low power requirements, a Power Management Unit (PMU)
+	  is designed for controlling power resources in StarFive JH71XX SoCs.

drivers/soc/starfive/Makefile new file +3 -0

new file mode 100644
index 000000000000..13b589d6b5f3
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_JH71XX_PMU)	+= jh71xx_pmu.o

drivers/soc/starfive/jh71xx_pmu.c new file +12 -0

new file mode 100644
index 000000000000..7a0145779e07
@@ -0,0 +1,396 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * StarFive JH71XX PMU (Power Management Unit) Controller Driver
+ *
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>

Quoted 8 lines

drivers/soc/starfive/jh71xx_pmu.c line 20

Quoted 8 lines

drivers/soc/starfive/jh71xx_pmu.c new file +8 -0

+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <dt-bindings/power/starfive,jh7110-pmu.h>
+
+/* register offset */
+#define JH71XX_PMU_HW_EVENT_ON		0x04
+#define JH71XX_PMU_HW_EVENT_OFF		0x08

Quoted 4 lines

drivers/soc/starfive/jh71xx_pmu.c line 24

Quoted 4 lines

drivers/soc/starfive/jh71xx_pmu.c new file +4 -0

+#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
+#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
+#define JH71XX_PMU_SW_ENCOURAGE		0x44
+#define JH71XX_PMU_INT_MASK		0x48

Quoted 5 lines

drivers/soc/starfive/jh71xx_pmu.c line 29

Quoted 5 lines

drivers/soc/starfive/jh71xx_pmu.c new file +5 -0

+#define JH71XX_PMU_PCH_BYPASS		0x4C
+#define JH71XX_PMU_PCH_PSTATE		0x50
+#define JH71XX_PMU_PCH_TIMEOUT		0x54
+#define JH71XX_PMU_LP_TIMEOUT		0x58
+#define JH71XX_PMU_HW_TURN_ON		0x5C

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c line 39

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+#define JH71XX_PMU_CURR_POWER_MODE	0x80
+#define JH71XX_PMU_EVENT_STATUS		0x88
+#define JH71XX_PMU_INT_STATUS		0x8C
+
+/* sw encourage cfg */
+#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
+#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
+#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF

Quoted 5 lines

Quoted 5 lines

drivers/soc/starfive/jh71xx_pmu.c new file +5 -0

+
+/* pmu int status */
+#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
+#define JH71XX_PMU_INT_HW_REQ		BIT(1)
+#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)

Quoted 8 lines

drivers/soc/starfive/jh71xx_pmu.c line 52

Quoted 8 lines

drivers/soc/starfive/jh71xx_pmu.c new file +8 -0

+#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
+#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
+#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
+					JH71XX_PMU_INT_HW_FAIL | \
+					JH71XX_PMU_INT_PCH_FAIL)
+#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \
+					JH71XX_PMU_INT_HW_REQ | \
+					JH71XX_PMU_INT_FAIL_MASK)

Quoted 4 lines

drivers/soc/starfive/jh71xx_pmu.c line 56

Quoted 4 lines

drivers/soc/starfive/jh71xx_pmu.c new file +4 -0

+
+/*
+ * The time required for switching power status is based on the time
+ * to turn on the largest domain's power, which is at microsecond level

Quoted 2 lines

drivers/soc/starfive/jh71xx_pmu.c line 58

Quoted 2 lines

drivers/soc/starfive/jh71xx_pmu.c new file +2 -0

+ */
+#define JH71XX_PMU_TIMEOUT_US		100

Quoted 6 lines

drivers/soc/starfive/jh71xx_pmu.c line 60

Quoted 1 line

drivers/soc/starfive/jh71xx_pmu.c new file +1 -0

+struct jh71xx_domain_info {

	const char * const name;
	unsigned int flags;
	u8 bit;

Quoted 8 lines

drivers/soc/starfive/jh71xx_pmu.c line 66

Quoted 3 lines

drivers/soc/starfive/jh71xx_pmu.c new file +3 -0

+};
+
+struct jh71xx_pmu_match_data {

	const struct jh71xx_domain_info *domain_info;
	int num_domains;

Can you switch these two around like so?

Should be no problem.

Quoted 10 lines

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+};
+
+struct jh71xx_pmu {
+	struct device *dev;
+	const struct jh71xx_pmu_match_data *match_data;
+	void __iomem *base;
+	spinlock_t lock;	/* protects pmu reg */
+	int irq;
+	struct genpd_onecell_data genpd_data;
+	struct generic_pm_domain **genpd;

Quoted 8 lines

drivers/soc/starfive/jh71xx_pmu.c line 84

Quoted 6 lines

drivers/soc/starfive/jh71xx_pmu.c new file +6 -0

+};
+
+struct jh71xx_pmu_dev {
+	struct generic_pm_domain genpd;
+	const struct jh71xx_domain_info *domain_info;
+	struct jh71xx_pmu *pmu;

And these two too please in the same way.

Nice :)

Quoted 51 lines

Quoted 38 lines

drivers/soc/starfive/jh71xx_pmu.c new file +37 -0

+};
+
+static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+
+	if (!mask) {
+		*is_on = false;
+		return -EINVAL;
+	}
+
+	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
+
+	return 0;
+}
+
+static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+	unsigned long flags;
+	u32 val;
+	u32 mode;
+	u32 encourage_lo;
+	u32 encourage_hi;
+	bool is_on;
+	int ret;
+
+	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
+	if (ret) {
+		dev_dbg(pmu->dev, "unable to get current state for %s\n",
+			pmd->genpd.name);
+		return ret;
+	}
+
+	if (is_on == on) {
+		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
+			pmd->genpd.name, on ? "en" : "dis");

drivers/soc/starfive/jh71xx_pmu.c line 131

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+		return 0;
+	}
+
+	spin_lock_irqsave(&pmu->lock, flags);
+
+	/*
+	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
+	 *
+	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
+	 *    the bit which power domain will be turn-on, write 0 to the others.

Some grammatical nit picking..

Quoted 5 lines

"Configure the register blah (offset 0x0c) by writing 1 to the bit corresponding to the power domain that will be turned on and writing zero to the others."

Is that a correct correct summation of the operation?

Yes, maybe your description is better easy-to-understand.

Quoted 23 lines

Quoted 12 lines

drivers/soc/starfive/jh71xx_pmu.c new file +9 -0

+	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
+	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
 
 
+	 */
+	if (on) {
+		mode = JH71XX_PMU_SW_TURN_ON_POWER;
+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
+	} else {
+		mode = JH71XX_PMU_SW_TURN_OFF_POWER;

drivers/soc/starfive/jh71xx_pmu.c line 150

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
+	}
+
+	writel(mask, pmu->base + mode);
+
+	/*
+	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
+	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
+	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0

Quoted 7 lines

I think you could replace the hard "coded" numbers here with a better description idk without looking at the #defines above what these correspond to. AFAICT, it'd be something like:
First write the ...ENCOURAGE_ON to reset the state machine which parses the command sequence. It must be written every time.
Then write the lower bits of the command sequence, followed by the upper bits. The sequence differs between powering on & off a domain.

Thank you for correction for these description. Because English is not my native language, so not very good in some sentences. I'll take your advice.

Quoted 42 lines

Quoted 42 lines

drivers/soc/starfive/jh71xx_pmu.c new file +41 -0

+	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
+	 * the purpose is to reset the state machine which is going to parse instruction
+	 *  sequence. It has to be written every time.
+	 */
+	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	/* Wait for the power domain bit to be enabled / disabled */
+	if (on) {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, val & mask,
+						1, JH71XX_PMU_TIMEOUT_US);
+	} else {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, !(val & mask),
+						1, JH71XX_PMU_TIMEOUT_US);
+	}
+
+	if (ret) {
+		dev_err(pmu->dev, "%s: failed to power %s\n",
+			pmd->genpd.name, on ? "on" : "off");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
 
+static int jh71xx_pmu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	const struct jh71xx_pmu_match_data *match_data;
+	struct jh71xx_pmu *pmu;
+	unsigned int i;
+	int ret;
+
+	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
+	if (!pmu)
+		return -ENOMEM;

Quoted 8 lines

drivers/soc/starfive/jh71xx_pmu.c line 291

Quoted 6 lines

drivers/soc/starfive/jh71xx_pmu.c new file +6 -0

+
+	pmu->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pmu->base))
+		return PTR_ERR(pmu->base);
+
+	/* initialize pmu interrupt  */

nit: this comment is ~pointless.

Will be dropped.

Quoted 11 lines

drivers/soc/starfive/jh71xx_pmu.c line 299

Quoted 9 lines

drivers/soc/starfive/jh71xx_pmu.c new file +8 -0

+	pmu->irq = platform_get_irq(pdev, 0);
+	if (pmu->irq < 0)
+		return pmu->irq;
+
+	ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
+			       0, pdev->name, pmu);
+	if (ret)
+		dev_err(dev, "request irq failed.\n");

nit: s/request/requesting

Will be fixed.

Quoted 4 lines

Unfortunately I cannot really review the rest of this, but hopefully I'll get a board soon and can try it out - or else send me a link to your TRM or w/e.

Anyway, I would like to thank you very much for your time.

Hey Walker,
Took another bit of a look.

Quoted 79 lines · Conor Dooley

Quoted 29 lines

Hey Walker,

Hopefully just some minor bits here. Hopefully either Emil who has a board, or someone that knows power management stuff better can give this a proper review.

Quoted 21 lines · Walker Chen

Add pmu driver for the StarFive JH71XX SoC.

As the power domains provider, the Power Management Unit (PMU) is designed for including multiple PM domains that can be used for power gating of selected IP blocks for power saving by reduced leakage current. It accepts software encourage command to switch the power mode of SoC.

Signed-off-by Walker Chen <[email protected]>

---
 MAINTAINERS                       |  14 ++
 drivers/soc/Kconfig               |   1 +
 drivers/soc/Makefile              |   1 +
 drivers/soc/starfive/Kconfig      |  11 +
 drivers/soc/starfive/Makefile     |   3 +
 drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
 6 files changed, 426 insertions(+)
 create mode 100644 drivers/soc/starfive/Kconfig
 create mode 100644 drivers/soc/starfive/Makefile
 create mode 100644 drivers/soc/starfive/jh71xx_pmu.c

Quoted 8 lines

drivers/soc/starfive/Kconfig line 5

Quoted 3 lines

drivers/soc/starfive/Kconfig new file +3 -0

+config JH71XX_PMU
+	bool "Support PMU for StarFive JH71XX Soc"
+	depends on PM && (SOC_STARFIVE || COMPILE_TEST)

Why not just do:

	depends on PM
	depends on SOC_STARFIVE || COMPILE_TEST

I think that way reads a little better.

No problem, will be changed like this way.

Quoted 38 lines

Quoted 38 lines

drivers/soc/starfive/Kconfig new file +6 -0

+	default SOC_STARFIVE
+	select PM_GENERIC_DOMAINS
+	help
+	  Say 'y' here to enable support power domain support.
+	  In order to meet low power requirements, a Power Management Unit (PMU)
+	  is designed for controlling power resources in StarFive JH71XX SoCs.

drivers/soc/starfive/Makefile new file +3 -0

new file mode 100644
index 000000000000..13b589d6b5f3
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_JH71XX_PMU)	+= jh71xx_pmu.o

drivers/soc/starfive/jh71xx_pmu.c new file +16 -0

new file mode 100644
index 000000000000..7a0145779e07
@@ -0,0 +1,396 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * StarFive JH71XX PMU (Power Management Unit) Controller Driver
+ *
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <dt-bindings/power/starfive,jh7110-pmu.h>

Quoted 4 lines

Quoted 4 lines

drivers/soc/starfive/jh71xx_pmu.c line 20

Quoted 4 lines

drivers/soc/starfive/jh71xx_pmu.c new file +4 -0

+
+/* register offset */
+#define JH71XX_PMU_HW_EVENT_ON		0x04
+#define JH71XX_PMU_HW_EVENT_OFF		0x08

Neither of these are used here at the moment - would they be used in the future? Also the docs fail to describe this PMU_HW_EVENT_OFF register, although the HW_ON, and SW_FOO ones are described.

Quoted 5 lines

Quoted 5 lines

drivers/soc/starfive/jh71xx_pmu.c line 24

Quoted 5 lines

drivers/soc/starfive/jh71xx_pmu.c new file +4 -0

+#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
+#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
+#define JH71XX_PMU_SW_ENCOURAGE		0x44
+#define JH71XX_PMU_INT_MASK		0x48

This one is called the "Timer Interrupt Mask", so we may as well match that naming here, no?

Quoted 6 lines

Quoted 6 lines

drivers/soc/starfive/jh71xx_pmu.c line 29

Quoted 6 lines

drivers/soc/starfive/jh71xx_pmu.c new file +5 -0

+#define JH71XX_PMU_PCH_BYPASS		0x4C
+#define JH71XX_PMU_PCH_PSTATE		0x50
+#define JH71XX_PMU_PCH_TIMEOUT		0x54
+#define JH71XX_PMU_LP_TIMEOUT		0x58
+#define JH71XX_PMU_HW_TURN_ON		0x5C

Same here, this HW related bit is also not used AFAICT. Do you intend adding a user of the HW encourage at some point in the future?

Quoted 11 lines

Quoted 11 lines

drivers/soc/starfive/jh71xx_pmu.c line 39

Quoted 11 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+#define JH71XX_PMU_CURR_POWER_MODE	0x80
+#define JH71XX_PMU_EVENT_STATUS		0x88
+#define JH71XX_PMU_INT_STATUS		0x8C
+
+/* sw encourage cfg */
+#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
+#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
+#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF

This all seems to correspond w/ docs...

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/soc/starfive/jh71xx_pmu.c new file +11 -0

+
+/* pmu int status */
+#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
+#define JH71XX_PMU_INT_HW_REQ		BIT(1)
+#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
+#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
+#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
+#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
+					JH71XX_PMU_INT_HW_FAIL | \
+					JH71XX_PMU_INT_PCH_FAIL)
+#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \

Quoted 2 lines

Quoted 2 lines

drivers/soc/starfive/jh71xx_pmu.c line 52

Quoted 2 lines

drivers/soc/starfive/jh71xx_pmu.c new file +2 -0

+					JH71XX_PMU_INT_HW_REQ | \
+					JH71XX_PMU_INT_FAIL_MASK)

...as does all of this - although, could the FAIL_MASK be dropped as it appears to only be used here & the ALL_MASK definition be replaced with GENMASK(8, 0)?
I don't really mind what you do here, think it just may be slightly easier to read, so if you disagree leave it as is.

Quoted 5 lines

Quoted 5 lines

drivers/soc/starfive/jh71xx_pmu.c line 56

Quoted 5 lines

drivers/soc/starfive/jh71xx_pmu.c new file +4 -0

+
+/*
+ * The time required for switching power status is based on the time
+ * to turn on the largest domain's power, which is at microsecond level

Would you mind mentioning which domain that is?
USB seems to be listed in Table 3-9 as 200 us.

Quoted 3 lines

Quoted 3 lines

drivers/soc/starfive/jh71xx_pmu.c line 58

Quoted 3 lines

drivers/soc/starfive/jh71xx_pmu.c new file +2 -0

+ */
+#define JH71XX_PMU_TIMEOUT_US		100

I'm happy enough with things, apart from my lack of familiarity with the power management APIs. Perhaps when you send the next version, someone else can comment there.

Quoted 129 lines

Quoted 7 lines

drivers/soc/starfive/jh71xx_pmu.c line 60

Quoted 2 lines

drivers/soc/starfive/jh71xx_pmu.c new file +1 -0

+struct jh71xx_domain_info {

	const char * const name;
	unsigned int flags;
	u8 bit;

Quoted 8 lines

drivers/soc/starfive/jh71xx_pmu.c line 66

Quoted 3 lines

drivers/soc/starfive/jh71xx_pmu.c new file +3 -0

+};
+
+struct jh71xx_pmu_match_data {

	const struct jh71xx_domain_info *domain_info;
	int num_domains;

Can you switch these two around like so?

Should be no problem.

Quoted 10 lines

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+};
+
+struct jh71xx_pmu {
+	struct device *dev;
+	const struct jh71xx_pmu_match_data *match_data;
+	void __iomem *base;
+	spinlock_t lock;	/* protects pmu reg */
+	int irq;
+	struct genpd_onecell_data genpd_data;
+	struct generic_pm_domain **genpd;

Quoted 8 lines

drivers/soc/starfive/jh71xx_pmu.c line 84

Quoted 6 lines

drivers/soc/starfive/jh71xx_pmu.c new file +6 -0

+};
+
+struct jh71xx_pmu_dev {
+	struct generic_pm_domain genpd;
+	const struct jh71xx_domain_info *domain_info;
+	struct jh71xx_pmu *pmu;

And these two too please in the same way.

Nice :)

Quoted 51 lines

Quoted 38 lines

drivers/soc/starfive/jh71xx_pmu.c new file +37 -0

+};
+
+static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+
+	if (!mask) {
+		*is_on = false;
+		return -EINVAL;
+	}
+
+	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
+
+	return 0;
+}
+
+static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+	unsigned long flags;
+	u32 val;
+	u32 mode;
+	u32 encourage_lo;
+	u32 encourage_hi;
+	bool is_on;
+	int ret;
+
+	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
+	if (ret) {
+		dev_dbg(pmu->dev, "unable to get current state for %s\n",
+			pmd->genpd.name);
+		return ret;
+	}
+
+	if (is_on == on) {
+		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
+			pmd->genpd.name, on ? "en" : "dis");

drivers/soc/starfive/jh71xx_pmu.c line 131

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+		return 0;
+	}
+
+	spin_lock_irqsave(&pmu->lock, flags);
+
+	/*
+	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
+	 *
+	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
+	 *    the bit which power domain will be turn-on, write 0 to the others.

Some grammatical nit picking..

Quoted 5 lines

"Configure the register blah (offset 0x0c) by writing 1 to the bit corresponding to the power domain that will be turned on and writing zero to the others."

Is that a correct correct summation of the operation?

Yes, maybe your description is better easy-to-understand.

Quoted 23 lines

Quoted 12 lines

drivers/soc/starfive/jh71xx_pmu.c new file +9 -0

+	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
+	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
 
 
+	 */
+	if (on) {
+		mode = JH71XX_PMU_SW_TURN_ON_POWER;
+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
+	} else {
+		mode = JH71XX_PMU_SW_TURN_OFF_POWER;

drivers/soc/starfive/jh71xx_pmu.c line 150

Quoted 10 lines

drivers/soc/starfive/jh71xx_pmu.c new file +10 -0

+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
+	}
+
+	writel(mask, pmu->base + mode);
+
+	/*
+	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
+	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
+	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0

Quoted 7 lines

I think you could replace the hard "coded" numbers here with a better description idk without looking at the #defines above what these correspond to. AFAICT, it'd be something like:
First write the ...ENCOURAGE_ON to reset the state machine which parses the command sequence. It must be written every time.
Then write the lower bits of the command sequence, followed by the upper bits. The sequence differs between powering on & off a domain.

Quoted 2 lines

Thank you for correction for these description. Because English is not my native language, so not very good in some sentences. I'll take your advice.

It may be mine but that does not mean I don't make mistakes either :) In this case, I'd like to re-word my suggestion. How about:
First write ...ENCOURAGE_ON to ...SW_ENCOURAGE. This will reset the state machine which parses the command sequence. This register must be written every time software wants to power on/off a domain.
Then write the lower bits of the command sequence, followed by the upper bits. The sequence differs between powering on & off a domain.

When I read my previous suggestion back, I had to read it more than once to get what I had meant...

Quoted 73 lines

Quoted 43 lines

Quoted 43 lines

drivers/soc/starfive/jh71xx_pmu.c new file +41 -0

+	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
+	 * the purpose is to reset the state machine which is going to parse instruction
+	 *  sequence. It has to be written every time.
+	 */
+	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	/* Wait for the power domain bit to be enabled / disabled */
+	if (on) {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, val & mask,
+						1, JH71XX_PMU_TIMEOUT_US);
+	} else {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, !(val & mask),
+						1, JH71XX_PMU_TIMEOUT_US);
+	}
+
+	if (ret) {
+		dev_err(pmu->dev, "%s: failed to power %s\n",
+			pmd->genpd.name, on ? "on" : "off");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
 
+static int jh71xx_pmu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	const struct jh71xx_pmu_match_data *match_data;
+	struct jh71xx_pmu *pmu;
+	unsigned int i;
+	int ret;
+
+	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
+	if (!pmu)
+		return -ENOMEM;

Quoted 8 lines

drivers/soc/starfive/jh71xx_pmu.c line 291

Quoted 6 lines

drivers/soc/starfive/jh71xx_pmu.c new file +6 -0

+
+	pmu->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pmu->base))
+		return PTR_ERR(pmu->base);
+
+	/* initialize pmu interrupt  */

nit: this comment is ~pointless.

Will be dropped.

Quoted 11 lines

drivers/soc/starfive/jh71xx_pmu.c line 299

Quoted 9 lines

drivers/soc/starfive/jh71xx_pmu.c new file +8 -0

+	pmu->irq = platform_get_irq(pdev, 0);
+	if (pmu->irq < 0)
+		return pmu->irq;
+
+	ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
+			       0, pdev->name, pmu);
+	if (ret)
+		dev_err(dev, "request irq failed.\n");

nit: s/request/requesting

Will be fixed.

Quoted 4 lines

Unfortunately I cannot really review the rest of this, but hopefully I'll get a board soon and can try it out - or else send me a link to your TRM or w/e.

Quoted 1 line

Anyway, I would like to thank you very much for your time.

Oh, while I think of it - I was talking to some people from Imagination at the RISC-V Summit who said they're working on open sourcing their GPU drivers, so hopefully we do end up with an open source driver for the one on JH7110 soon :)