[PATCH v1 0/4] JH7110 Power Domain Support

[PATCH v1 0/4] JH7110 Power Domain Support

[Walker Chen]

This patchset adds power domain controller driver for the StarFive JH7110 SoC.
The series has been tested on the VisionFive 2 board.

This patchset should be applied after the patchset [1], [2], [3]:
[1] https://lore.kernel.org/all/20221118010627.70576-1-hal.feng@starfivetech.com/
[2] https://lore.kernel.org/all/20221118011108.70715-1-hal.feng@starfivetech.com/
[3] https://lore.kernel.org/all/20221118011714.70877-1-hal.feng@starfivetech.com/

Walker Chen (4):
  dt-bindings: power: Add StarFive JH7110 power domain definitions
  dt-bindings: power: Add starfive,jh71xx-power bindings
  soc: starfive: Add StarFive JH71XX pmu driver
  riscv: dts: starfive: add power controller node

 .../bindings/power/starfive,jh71xx-power.yaml |  46 +++
 MAINTAINERS                                   |   8 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |   7 +
 drivers/soc/Kconfig                           |   1 +
 drivers/soc/Makefile                          |   1 +
 drivers/soc/starfive/Kconfig                  |   9 +
 drivers/soc/starfive/Makefile                 |   3 +
 drivers/soc/starfive/jh71xx_pmu.c             | 380 ++++++++++++++++++
 include/dt-bindings/power/jh7110-power.h      |  18 +
 include/soc/starfive/pm_domains.h             |  15 +
 10 files changed, 488 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
 create mode 100644 drivers/soc/starfive/Kconfig
 create mode 100644 drivers/soc/starfive/Makefile
 create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
 create mode 100644 include/dt-bindings/power/jh7110-power.h
 create mode 100644 include/soc/starfive/pm_domains.h


base-commit: 094226ad94f471a9f19e8f8e7140a09c2625abaa
prerequisite-patch-id: 8ebfffa09b478904bf7c516f76e2d824ddb60140
prerequisite-patch-id: e8dd8258a4c4062eee2cf07c4607d52baea71f3a
prerequisite-patch-id: d050d884d7b091ff30508a70f5ce5164bb3b72e5
prerequisite-patch-id: 0e41f8cfd4861fcbf6f2e6a2559ce28f0450299e
prerequisite-patch-id: 6e1652501859b85f101ff3b15ced585d43c71c1b
prerequisite-patch-id: 587628a67adad5c655e5f998bf6c4a368ec07d3c
prerequisite-patch-id: 596490c0e397df6c0249c1306fbb1d5bf00b5b83
prerequisite-patch-id: dc873317826b50364344b25ac5cd74e811403f3d
prerequisite-patch-id: a50150f41d8e874553023187e22eb24dffae8d16
prerequisite-patch-id: 735e62255c75801bdc4c0b4107850bce821ff7f5
prerequisite-patch-id: 9d2e83a2dd43e193f534283fab73e90b4f435043
prerequisite-patch-id: 7a43e0849a9afa3c6f83547fd16d9271b07619e5
prerequisite-patch-id: e7aa6fb05314bad6d94c465f3f59969871bf3d2e
prerequisite-patch-id: 6276b2a23818c65ff2ad3d65b562615690cffee9
prerequisite-patch-id: d834ece14ffb525b8c3e661e78736692f33fca9b
prerequisite-patch-id: 4c17a3ce4dae9b788795d915bf775630f5c43c53
prerequisite-patch-id: dabb913fd478e97593e45c23fee4be9fd807f851
prerequisite-patch-id: ba61df106fbe2ada21e8f22c3d2cfaf7809c84b6
prerequisite-patch-id: 287572fb64f83f5d931034f7c75674907584a087
prerequisite-patch-id: 536114f0732646095ef5302a165672b3290d4c75
prerequisite-patch-id: 258ea5f9b8bf41b6981345dcc81795f25865d38f
prerequisite-patch-id: 8b6f2c9660c0ac0ee4e73e4c21aca8e6b75e81b9
prerequisite-patch-id: e09e995700a814a763aa304ad3881a7222acf556
prerequisite-patch-id: 841cd71b556b480d6a5a5e332eeca70d6a76ec3f
prerequisite-patch-id: d074c7ffa2917a9f754d5801e3f67bc980f9de4c
prerequisite-patch-id: 5f59bc7cbbf1230e5ff4761fa7c1116d4e6e5d71
prerequisite-patch-id: d5da3475c6a3588e11a1678feb565bdd459b548e
-- 
2.17.1

[PATCH v1 1/4] dt-bindings: power: Add StarFive JH7110 power domain definitions

[Walker Chen]

Add power domain definitions for the StarFive JH7110 SoC.

Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
---
 include/dt-bindings/power/jh7110-power.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 include/dt-bindings/power/jh7110-power.h

diff --git a/include/dt-bindings/power/jh7110-power.h b/include/dt-bindings/power/jh7110-power.h
new file mode 100644
index 000000000000..24160c46fbaf
--- /dev/null
+++ b/include/dt-bindings/power/jh7110-power.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: (GPL-2.0) */
+/*
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ * Author: Walker Chen <walker.chen@starfivetech.com>
+ */
+#ifndef __DT_BINDINGS_POWER_JH7110_POWER_H__
+#define __DT_BINDINGS_POWER_JH7110_POWER_H__
+
+#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
+#define JH7110_PD_GPUB		7
+
+#endif
-- 
2.17.1

[PATCH v1 2/4] dt-bindings: power: Add starfive,jh71xx-power bindings

[Walker Chen]

Add bindings for the power domain controller on the StarFive JH71XX SoC.

Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
---
 .../bindings/power/starfive,jh71xx-power.yaml | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml

diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
new file mode 100644
index 000000000000..2537303b4829
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/starfive,jh71xx-power.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH71xx Power Domains Controller
+
+maintainers:
+  - Walker Chen <walker.chen@starfivetech.com>
+
+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:
+    items:
+      - enum:
+          - starfive,jh7110-pmu
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#power-domain-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#power-domain-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    pwrc: power-controller@17030000 {
+        compatible = "starfive,jh7110-pmu";
+        reg = <0x17030000 0x10000>;
+        interrupts = <111>;
+        #power-domain-cells = <1>;
+    };
-- 
2.17.1

[PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

Add generic power domain driver for the StarFive JH71XX SoC.

Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
---
 MAINTAINERS                       |   8 +
 drivers/soc/Kconfig               |   1 +
 drivers/soc/Makefile              |   1 +
 drivers/soc/starfive/Kconfig      |   9 +
 drivers/soc/starfive/Makefile     |   3 +
 drivers/soc/starfive/jh71xx_pmu.c | 380 ++++++++++++++++++++++++++++++
 include/soc/starfive/pm_domains.h |  15 ++
 7 files changed, 417 insertions(+)
 create mode 100644 drivers/soc/starfive/Kconfig
 create mode 100644 drivers/soc/starfive/Makefile
 create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
 create mode 100644 include/soc/starfive/pm_domains.h

diff --git a/MAINTAINERS b/MAINTAINERS
index a70c1d0f303e..112f1e698723 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19623,6 +19623,14 @@ F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
 F:	drivers/reset/starfive/
 F:	include/dt-bindings/reset/starfive*
 
+STARFIVE JH71XX PMU CONTROLLER DRIVER
+M:	Walker Chen <walker.chen@starfivetech.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/power/starfive*
+F:	drivers/soc/starfive/jh71xx_pmu.c
+F:	include/soc/starfive/pm_domains.h
+F:	include/dt-bindings/power/jh7110-power.h
+
 STATIC BRANCH/CALL
 M:	Peter Zijlstra <peterz@infradead.org>
 M:	Josh Poimboeuf <jpoimboe@kernel.org>
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index e461c071189b..628fda4d5ed9 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -21,6 +21,7 @@ source "drivers/soc/renesas/Kconfig"
 source "drivers/soc/rockchip/Kconfig"
 source "drivers/soc/samsung/Kconfig"
 source "drivers/soc/sifive/Kconfig"
+source "drivers/soc/starfive/Kconfig"
 source "drivers/soc/sunxi/Kconfig"
 source "drivers/soc/tegra/Kconfig"
 source "drivers/soc/ti/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 534669840858..cbe076f42068 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -27,6 +27,7 @@ obj-y				+= renesas/
 obj-y				+= rockchip/
 obj-$(CONFIG_SOC_SAMSUNG)	+= samsung/
 obj-y				+= sifive/
+obj-y				+= starfive/
 obj-y				+= sunxi/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
 obj-y				+= ti/
diff --git a/drivers/soc/starfive/Kconfig b/drivers/soc/starfive/Kconfig
new file mode 100644
index 000000000000..2bbcc1397b15
--- /dev/null
+++ b/drivers/soc/starfive/Kconfig
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+
+config JH71XX_PMU
+	bool "Support PMU for StarFive JH71XX Soc"
+	depends on PM && (SOC_STARFIVE || COMPILE_TEST)
+	select PM_GENERIC_DOMAINS
+	help
+	  Say y here to enable power domain support.
+
diff --git a/drivers/soc/starfive/Makefile b/drivers/soc/starfive/Makefile
new file mode 100644
index 000000000000..13b589d6b5f3
--- /dev/null
+++ b/drivers/soc/starfive/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_JH71XX_PMU)	+= jh71xx_pmu.o
diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
new file mode 100644
index 000000000000..e6c0083d166e
--- /dev/null
+++ b/drivers/soc/starfive/jh71xx_pmu.c
@@ -0,0 +1,380 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * StarFive JH71XX Power Domain 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/jh7110-power.h>
+
+/* register offset */
+#define HW_EVENT_TURN_ON_MASK		0x04
+#define HW_EVENT_TURN_OFF_MASK		0x08
+#define SW_TURN_ON_POWER_MODE		0x0C
+#define SW_TURN_OFF_POWER_MODE		0x10
+#define SW_ENCOURAGE			0x44
+#define PMU_INT_MASK			0x48
+#define PCH_BYPASS			0x4C
+#define PCH_PSTATE			0x50
+#define PCH_TIMEOUT			0x54
+#define LP_TIMEOUT			0x58
+#define HW_TURN_ON_MODE			0x5C
+#define CURR_POWER_MODE			0x80
+#define PMU_EVENT_STATUS		0x88
+#define PMU_INT_STATUS			0x8C
+
+/* sw encourage cfg */
+#define SW_MODE_ENCOURAGE_EN_LO		0x05
+#define SW_MODE_ENCOURAGE_EN_HI		0x50
+#define SW_MODE_ENCOURAGE_DIS_LO	0x0A
+#define SW_MODE_ENCOURAGE_DIS_HI	0xA0
+#define SW_MODE_ENCOURAGE_ON		0xFF
+
+/* pmu int status */
+#define PMU_INT_SEQ_DONE		BIT(0)
+#define PMU_INT_HW_REQ			BIT(1)
+#define PMU_INT_SW_FAIL			GENMASK(3, 2)
+#define PMU_INT_HW_FAIL			GENMASK(5, 4)
+#define PMU_INT_PCH_FAIL		GENMASK(8, 6)
+#define PMU_INT_FAIL_MASK		(PMU_INT_SW_FAIL | \
+					PMU_INT_HW_FAIL | \
+					PMU_INT_PCH_FAIL)
+#define PMU_INT_ALL_MASK		(PMU_INT_SEQ_DONE | \
+					PMU_INT_HW_REQ | \
+					PMU_INT_FAIL_MASK)
+
+#define DELAY_US			10
+#define TIMEOUT_US			100000
+
+struct starfive_power_dev {
+	struct generic_pm_domain genpd;
+	struct starfive_pmu *power;
+	uint32_t mask;
+};
+
+struct starfive_pmu {
+	void __iomem *base;
+	spinlock_t lock;
+	int irq;
+	struct device *pdev;
+	struct starfive_power_dev *dev;
+	struct genpd_onecell_data genpd_data;
+	struct generic_pm_domain **genpd;
+};
+
+struct starfive_pmu_data {
+	const char * const name;
+	uint8_t bit;
+	unsigned int flags;
+};
+
+static void __iomem *pmu_base;
+
+static inline void pmu_writel(u32 val, u32 offset)
+{
+	writel(val, pmu_base + offset);
+}
+
+void starfive_pmu_hw_event_turn_on(u32 mask)
+{
+	pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
+}
+EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
+
+void starfive_pmu_hw_event_turn_off(u32 mask)
+{
+	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
+}
+EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
+
+static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
+{
+	struct starfive_pmu *pmu = pmd->power;
+
+	if (!pmd->mask) {
+		*is_on = false;
+		return -EINVAL;
+	}
+
+	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
+
+	return 0;
+}
+
+static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
+{
+	struct starfive_pmu *pmu = pmd->power;
+	unsigned long flags;
+	uint32_t val;
+	uint32_t mode;
+	uint32_t encourage_lo;
+	uint32_t encourage_hi;
+	bool is_on;
+	int ret;
+
+	if (!pmd->mask)
+		return -EINVAL;
+
+	if (is_on == on) {
+		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
+				pmd->genpd.name, on ? "en" : "dis");
+		return 0;
+	}
+
+	spin_lock_irqsave(&pmu->lock, flags);
+
+	if (on) {
+		mode = SW_TURN_ON_POWER_MODE;
+		encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
+		encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
+	} else {
+		mode = SW_TURN_OFF_POWER_MODE;
+		encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
+		encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
+	}
+
+	__raw_writel(pmd->mask, pmu->base + mode);
+
+	/* write SW_ENCOURAGE to make the configuration take effect */
+	__raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
+	__raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
+	__raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	if (on) {
+		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
+						val & pmd->mask, DELAY_US,
+						TIMEOUT_US);
+		if (ret) {
+			dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
+			return -ETIMEDOUT;
+		}
+	} else {
+		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
+						!(val & pmd->mask), DELAY_US,
+						TIMEOUT_US);
+		if (ret) {
+			dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
+			return -ETIMEDOUT;
+		}
+	}
+
+	return 0;
+}
+
+static int starfive_pmu_on(struct generic_pm_domain *genpd)
+{
+	struct starfive_power_dev *pmd = container_of(genpd,
+		struct starfive_power_dev, genpd);
+
+	return starfive_pmu_set_state(pmd, true);
+}
+
+static int starfive_pmu_off(struct generic_pm_domain *genpd)
+{
+	struct starfive_power_dev *pmd = container_of(genpd,
+		struct starfive_power_dev, genpd);
+
+	return starfive_pmu_set_state(pmd, false);
+}
+
+static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
+{
+	u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
+
+	if (enable)
+		val &= ~mask;
+	else
+		val |= mask;
+
+	__raw_writel(val, pmu->base + PMU_INT_MASK);
+}
+
+static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
+{
+	struct starfive_pmu *pmu = data;
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&pmu->lock, flags);
+	val = __raw_readl(pmu->base + PMU_INT_STATUS);
+
+	if (val & PMU_INT_SEQ_DONE)
+		dev_dbg(pmu->pdev, "sequence done.\n");
+	if (val & PMU_INT_HW_REQ)
+		dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
+	if (val & PMU_INT_SW_FAIL)
+		dev_err(pmu->pdev, "software encourage fail.\n");
+	if (val & PMU_INT_HW_FAIL)
+		dev_err(pmu->pdev, "hardware encourage fail.\n");
+	if (val & PMU_INT_PCH_FAIL)
+		dev_err(pmu->pdev, "p-channel fail event.\n");
+
+	/* clear interrupts */
+	__raw_writel(val, pmu->base + PMU_INT_STATUS);
+	__raw_writel(val, pmu->base + PMU_EVENT_STATUS);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
+static int starfive_pmu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	const struct starfive_pmu_data *entry, *table;
+	struct starfive_pmu *pmu;
+	unsigned int i;
+	uint8_t max_bit = 0;
+	int ret;
+
+	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
+	if (!pmu)
+		return -ENOMEM;
+
+	pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pmu->base))
+		return PTR_ERR(pmu->base);
+
+	/* initialize pmu interrupt  */
+	pmu->irq = platform_get_irq(pdev, 0);
+	if (pmu->irq < 0)
+		return pmu->irq;
+
+	ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
+			       0, pdev->name, pmu);
+	if (ret)
+		dev_err(dev, "request irq failed.\n");
+
+	table = of_device_get_match_data(dev);
+	if (!table)
+		return -EINVAL;
+
+	pmu->pdev = dev;
+	pmu->genpd_data.num_domains = 0;
+	i = 0;
+	for (entry = table; entry->name; entry++) {
+		max_bit = max(max_bit, entry->bit);
+		i++;
+	}
+
+	if (!i)
+		return -ENODEV;
+
+	pmu->genpd_data.num_domains = max_bit + 1;
+
+	pmu->dev = devm_kcalloc(dev, pmu->genpd_data.num_domains,
+				  sizeof(struct starfive_power_dev),
+				  GFP_KERNEL);
+	if (!pmu->dev)
+		return -ENOMEM;
+
+	pmu->genpd = devm_kcalloc(dev, pmu->genpd_data.num_domains,
+				    sizeof(struct generic_pm_domain *),
+				    GFP_KERNEL);
+	if (!pmu->genpd)
+		return -ENOMEM;
+
+	pmu->genpd_data.domains = pmu->genpd;
+
+	i = 0;
+	for (entry = table; entry->name; entry++) {
+		struct starfive_power_dev *pmd = &pmu->dev[i];
+		bool is_on;
+
+		pmd->power = pmu;
+		pmd->mask = BIT(entry->bit);
+		pmd->genpd.name = entry->name;
+		pmd->genpd.flags = entry->flags;
+
+		ret = starfive_pmu_get_state(pmd, &is_on);
+		if (ret)
+			dev_warn(dev, "unable to get current state for %s\n",
+				 pmd->genpd.name);
+
+		pmd->genpd.power_on = starfive_pmu_on;
+		pmd->genpd.power_off = starfive_pmu_off;
+
+		pm_genpd_init(&pmd->genpd, NULL, !is_on);
+		pmu->genpd[entry->bit] = &pmd->genpd;
+
+		i++;
+	}
+
+	spin_lock_init(&pmu->lock);
+	starfive_pmu_int_enable(pmu, PMU_INT_ALL_MASK & ~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 starfive_pmu_data jh7110_power_domains[] = {
+	{
+		.name = "SYSTOP",
+		.bit = JH7110_PD_SYSTOP,
+		.flags = GENPD_FLAG_ALWAYS_ON,
+	}, {
+		.name = "CPU",
+		.bit = JH7110_PD_CPU,
+		.flags = GENPD_FLAG_ALWAYS_ON,
+	}, {
+		.name = "GPUA",
+		.bit = JH7110_PD_GPUA,
+	}, {
+		.name = "VDEC",
+		.bit = JH7110_PD_VDEC,
+	}, {
+		.name = "VOUT",
+		.bit = JH7110_PD_VOUT,
+	}, {
+		.name = "ISP",
+		.bit = JH7110_PD_ISP,
+	}, {
+		.name = "VENC",
+		.bit = JH7110_PD_VENC,
+	}, {
+		.name = "GPUB",
+		.bit = JH7110_PD_GPUB,
+	}, {
+		/* sentinel */
+	},
+};
+
+static const struct of_device_id starfive_pmu_of_match[] = {
+	{
+		.compatible = "starfive,jh7110-pmu",
+		.data = &jh7110_power_domains,
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct platform_driver jh71xx_pmu_driver = {
+	.driver = {
+		.name = "jh71xx-pmu",
+		.of_match_table = starfive_pmu_of_match,
+	},
+	.probe  = starfive_pmu_probe,
+};
+builtin_platform_driver(jh71xx_pmu_driver);
+
+MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>");
+MODULE_DESCRIPTION("StarFive JH71XX Power Domain Driver");
+MODULE_LICENSE("GPL");
diff --git a/include/soc/starfive/pm_domains.h b/include/soc/starfive/pm_domains.h
new file mode 100644
index 000000000000..a20e28e9baf3
--- /dev/null
+++ b/include/soc/starfive/pm_domains.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ * Author: Walker Chen <walker.chen@starfivetech.com>
+ */
+
+#ifndef __SOC_STARFIVE_PMDOMAINS_H__
+#define __SOC_STARFIVE_PMDOMAINS_H__
+
+#include <linux/types.h>
+
+void starfive_pmu_hw_event_turn_on(u32 mask);
+void starfive_pmu_hw_event_turn_off(u32 mask);
+
+#endif /* __SOC_STARFIVE_PMDOMAINS_H__ */
-- 
2.17.1

[PATCH v1 4/4] riscv: dts: starfive: add power controller node

[Walker Chen]

This adds the power controller node for the Starfive JH7110 SoC.
The pmu needs to be used by other modules such as ISP, VPU, etc.

Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
---
 arch/riscv/boot/dts/starfive/jh7110.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index c22e8f1d2640..fa7b60b82d71 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -356,6 +356,13 @@
 			#gpio-cells = <2>;
 		};
 
+		pwrc: power-controller@17030000 {
+			compatible = "starfive,jh7110-pmu";
+			reg = <0x0 0x17030000 0x0 0x10000>;
+			interrupts = <111>;
+			#power-domain-cells = <1>;
+		};
+
 		uart0: serial@10000000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x0 0x10000000 0x0 0x10000>;
-- 
2.17.1

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Emil Renner Berthing]

On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote:
>
> Add generic power domain driver for the StarFive JH71XX SoC.

SoCs

> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  MAINTAINERS                       |   8 +
>  drivers/soc/Kconfig               |   1 +
>  drivers/soc/Makefile              |   1 +
>  drivers/soc/starfive/Kconfig      |   9 +
>  drivers/soc/starfive/Makefile     |   3 +
>  drivers/soc/starfive/jh71xx_pmu.c | 380 ++++++++++++++++++++++++++++++
>  include/soc/starfive/pm_domains.h |  15 ++
>  7 files changed, 417 insertions(+)
>  create mode 100644 drivers/soc/starfive/Kconfig
>  create mode 100644 drivers/soc/starfive/Makefile
>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
>  create mode 100644 include/soc/starfive/pm_domains.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a70c1d0f303e..112f1e698723 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19623,6 +19623,14 @@ F:     Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>  F:     drivers/reset/starfive/
>  F:     include/dt-bindings/reset/starfive*

checkpatch.pl complains about not sorting these lines alphabetically.

> +STARFIVE JH71XX PMU CONTROLLER DRIVER
> +M:     Walker Chen <walker.chen@starfivetech.com>
> +S:     Maintained
> +F:     Documentation/devicetree/bindings/power/starfive*
> +F:     drivers/soc/starfive/jh71xx_pmu.c
> +F:     include/soc/starfive/pm_domains.h
> +F:     include/dt-bindings/power/jh7110-power.h
> +
>  STATIC BRANCH/CALL
>  M:     Peter Zijlstra <peterz@infradead.org>
>  M:     Josh Poimboeuf <jpoimboe@kernel.org>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index e461c071189b..628fda4d5ed9 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -21,6 +21,7 @@ source "drivers/soc/renesas/Kconfig"
>  source "drivers/soc/rockchip/Kconfig"
>  source "drivers/soc/samsung/Kconfig"
>  source "drivers/soc/sifive/Kconfig"
> +source "drivers/soc/starfive/Kconfig"
>  source "drivers/soc/sunxi/Kconfig"
>  source "drivers/soc/tegra/Kconfig"
>  source "drivers/soc/ti/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 534669840858..cbe076f42068 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -27,6 +27,7 @@ obj-y                         += renesas/
>  obj-y                          += rockchip/
>  obj-$(CONFIG_SOC_SAMSUNG)      += samsung/
>  obj-y                          += sifive/
> +obj-y                          += starfive/
>  obj-y                          += sunxi/
>  obj-$(CONFIG_ARCH_TEGRA)       += tegra/
>  obj-y                          += ti/
> diff --git a/drivers/soc/starfive/Kconfig b/drivers/soc/starfive/Kconfig
> new file mode 100644
> index 000000000000..2bbcc1397b15
> --- /dev/null
> +++ b/drivers/soc/starfive/Kconfig
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config JH71XX_PMU
> +       bool "Support PMU for StarFive JH71XX Soc"
> +       depends on PM && (SOC_STARFIVE || COMPILE_TEST)
> +       select PM_GENERIC_DOMAINS
> +       help
> +         Say y here to enable power domain support.

Please expand this help text. Even checkpatch.pl complains about it.

> +
> diff --git a/drivers/soc/starfive/Makefile b/drivers/soc/starfive/Makefile
> new file mode 100644
> index 000000000000..13b589d6b5f3
> --- /dev/null
> +++ b/drivers/soc/starfive/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_JH71XX_PMU)       += jh71xx_pmu.o
> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
> new file mode 100644
> index 000000000000..e6c0083d166e
> --- /dev/null
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * StarFive JH71XX Power Domain 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/jh7110-power.h>
> +
> +/* register offset */
> +#define HW_EVENT_TURN_ON_MASK          0x04
> +#define HW_EVENT_TURN_OFF_MASK         0x08
> +#define SW_TURN_ON_POWER_MODE          0x0C
> +#define SW_TURN_OFF_POWER_MODE         0x10
> +#define SW_ENCOURAGE                   0x44
> +#define PMU_INT_MASK                   0x48
> +#define PCH_BYPASS                     0x4C
> +#define PCH_PSTATE                     0x50
> +#define PCH_TIMEOUT                    0x54
> +#define LP_TIMEOUT                     0x58
> +#define HW_TURN_ON_MODE                        0x5C
> +#define CURR_POWER_MODE                        0x80
> +#define PMU_EVENT_STATUS               0x88
> +#define PMU_INT_STATUS                 0x8C
> +
> +/* sw encourage cfg */
> +#define SW_MODE_ENCOURAGE_EN_LO                0x05
> +#define SW_MODE_ENCOURAGE_EN_HI                0x50
> +#define SW_MODE_ENCOURAGE_DIS_LO       0x0A
> +#define SW_MODE_ENCOURAGE_DIS_HI       0xA0
> +#define SW_MODE_ENCOURAGE_ON           0xFF
> +
> +/* pmu int status */
> +#define PMU_INT_SEQ_DONE               BIT(0)
> +#define PMU_INT_HW_REQ                 BIT(1)
> +#define PMU_INT_SW_FAIL                        GENMASK(3, 2)
> +#define PMU_INT_HW_FAIL                        GENMASK(5, 4)
> +#define PMU_INT_PCH_FAIL               GENMASK(8, 6)
> +#define PMU_INT_FAIL_MASK              (PMU_INT_SW_FAIL | \
> +                                       PMU_INT_HW_FAIL | \
> +                                       PMU_INT_PCH_FAIL)
> +#define PMU_INT_ALL_MASK               (PMU_INT_SEQ_DONE | \
> +                                       PMU_INT_HW_REQ | \
> +                                       PMU_INT_FAIL_MASK)
> +
> +#define DELAY_US                       10
> +#define TIMEOUT_US                     100000

Please add a JH71XX_PMU_ prefix to these definitions to avoid
accidentally clashing with a definition added to a generic header.

> +
> +struct starfive_power_dev {
> +       struct generic_pm_domain genpd;
> +       struct starfive_pmu *power;
> +       uint32_t mask;

u32

> +};
> +
> +struct starfive_pmu {
> +       void __iomem *base;
> +       spinlock_t lock;
> +       int irq;
> +       struct device *pdev;
> +       struct starfive_power_dev *dev;
> +       struct genpd_onecell_data genpd_data;
> +       struct generic_pm_domain **genpd;
> +};
> +
> +struct starfive_pmu_data {
> +       const char * const name;
> +       uint8_t bit;

u8

> +       unsigned int flags;
> +};
> +
> +static void __iomem *pmu_base;
> +
> +static inline void pmu_writel(u32 val, u32 offset)
> +{
> +       writel(val, pmu_base + offset);
> +}
> +
> +void starfive_pmu_hw_event_turn_on(u32 mask)
> +{
> +       pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
> +
> +void starfive_pmu_hw_event_turn_off(u32 mask)
> +{
> +       pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
> +
> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
> +{
> +       struct starfive_pmu *pmu = pmd->power;
> +
> +       if (!pmd->mask) {
> +               *is_on = false;
> +               return -EINVAL;
> +       }
> +
> +       *is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
> +
> +       return 0;
> +}

Maybe just return an int where 0 = off, 1 = on and negative is an error.

> +
> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
> +{
> +       struct starfive_pmu *pmu = pmd->power;
> +       unsigned long flags;
> +       uint32_t val;
> +       uint32_t mode;
> +       uint32_t encourage_lo;
> +       uint32_t encourage_hi;

u32

> +       bool is_on;
> +       int ret;
> +
> +       if (!pmd->mask)
> +               return -EINVAL;
> +
> +       if (is_on == on) {
> +               dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
> +                               pmd->genpd.name, on ? "en" : "dis");
> +               return 0;
> +       }
> +
> +       spin_lock_irqsave(&pmu->lock, flags);
> +
> +       if (on) {
> +               mode = SW_TURN_ON_POWER_MODE;
> +               encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
> +               encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
> +       } else {
> +               mode = SW_TURN_OFF_POWER_MODE;
> +               encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
> +               encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
> +       }
> +
> +       __raw_writel(pmd->mask, pmu->base + mode);
> +
> +       /* write SW_ENCOURAGE to make the configuration take effect */
> +       __raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
> +       __raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
> +       __raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
> +
> +       spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +       if (on) {
> +               ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +                                               val & pmd->mask, DELAY_US,
> +                                               TIMEOUT_US);
> +               if (ret) {
> +                       dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
> +                       return -ETIMEDOUT;
> +               }
> +       } else {
> +               ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +                                               !(val & pmd->mask), DELAY_US,
> +                                               TIMEOUT_US);
> +               if (ret) {
> +                       dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
> +                       return -ETIMEDOUT;
> +               }
> +       }

You can move the dev_err out. Eg. dev_err(pmu->pdev, "%s: failed to
power %s\n", pmd->genpd.name, on ? "on" : "off")

> +       return 0;
> +}
> +
> +static int starfive_pmu_on(struct generic_pm_domain *genpd)
> +{
> +       struct starfive_power_dev *pmd = container_of(genpd,
> +               struct starfive_power_dev, genpd);

checkpatch.pl --strict complains that arguments doesn't match the open
parenthesis

> +
> +       return starfive_pmu_set_state(pmd, true);
> +}
> +
> +static int starfive_pmu_off(struct generic_pm_domain *genpd)
> +{
> +       struct starfive_power_dev *pmd = container_of(genpd,
> +               struct starfive_power_dev, genpd);
> +
> +       return starfive_pmu_set_state(pmd, false);
> +}
> +
> +static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
> +{
> +       u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
> +
> +       if (enable)
> +               val &= ~mask;
> +       else
> +               val |= mask;
> +
> +       __raw_writel(val, pmu->base + PMU_INT_MASK);
> +}

Consider adding comments to functions that need a lock to be held to
be used safely.

> +
> +static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
> +{
> +       struct starfive_pmu *pmu = data;
> +       unsigned long flags;
> +       u32 val;
> +
> +       spin_lock_irqsave(&pmu->lock, flags);
> +       val = __raw_readl(pmu->base + PMU_INT_STATUS);
> +
> +       if (val & PMU_INT_SEQ_DONE)
> +               dev_dbg(pmu->pdev, "sequence done.\n");
> +       if (val & PMU_INT_HW_REQ)
> +               dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
> +       if (val & PMU_INT_SW_FAIL)
> +               dev_err(pmu->pdev, "software encourage fail.\n");
> +       if (val & PMU_INT_HW_FAIL)
> +               dev_err(pmu->pdev, "hardware encourage fail.\n");
> +       if (val & PMU_INT_PCH_FAIL)
> +               dev_err(pmu->pdev, "p-channel fail event.\n");
> +
> +       /* clear interrupts */
> +       __raw_writel(val, pmu->base + PMU_INT_STATUS);
> +       __raw_writel(val, pmu->base + PMU_EVENT_STATUS);
> +
> +       spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int starfive_pmu_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       const struct starfive_pmu_data *entry, *table;
> +       struct starfive_pmu *pmu;
> +       unsigned int i;
> +       uint8_t max_bit = 0;
> +       int ret;
> +
> +       pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> +       if (!pmu)
> +               return -ENOMEM;
> +
> +       pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(pmu->base))
> +               return PTR_ERR(pmu->base);
> +
> +       /* initialize pmu interrupt  */
> +       pmu->irq = platform_get_irq(pdev, 0);
> +       if (pmu->irq < 0)
> +               return pmu->irq;
> +
> +       ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
> +                              0, pdev->name, pmu);
> +       if (ret)
> +               dev_err(dev, "request irq failed.\n");
> +
> +       table = of_device_get_match_data(dev);
> +       if (!table)
> +               return -EINVAL;
> +
> +       pmu->pdev = dev;
> +       pmu->genpd_data.num_domains = 0;
> +       i = 0;
> +       for (entry = table; entry->name; entry++) {
> +               max_bit = max(max_bit, entry->bit);
> +               i++;
> +       }
> +
> +       if (!i)
> +               return -ENODEV;
> +
> +       pmu->genpd_data.num_domains = max_bit + 1;
> +
> +       pmu->dev = devm_kcalloc(dev, pmu->genpd_data.num_domains,
> +                                 sizeof(struct starfive_power_dev),
> +                                 GFP_KERNEL);
> +       if (!pmu->dev)
> +               return -ENOMEM;
> +
> +       pmu->genpd = devm_kcalloc(dev, pmu->genpd_data.num_domains,
> +                                   sizeof(struct generic_pm_domain *),
> +                                   GFP_KERNEL);
> +       if (!pmu->genpd)
> +               return -ENOMEM;
> +
> +       pmu->genpd_data.domains = pmu->genpd;
> +
> +       i = 0;
> +       for (entry = table; entry->name; entry++) {
> +               struct starfive_power_dev *pmd = &pmu->dev[i];
> +               bool is_on;
> +
> +               pmd->power = pmu;
> +               pmd->mask = BIT(entry->bit);
> +               pmd->genpd.name = entry->name;
> +               pmd->genpd.flags = entry->flags;
> +
> +               ret = starfive_pmu_get_state(pmd, &is_on);
> +               if (ret)
> +                       dev_warn(dev, "unable to get current state for %s\n",
> +                                pmd->genpd.name);
> +
> +               pmd->genpd.power_on = starfive_pmu_on;
> +               pmd->genpd.power_off = starfive_pmu_off;
> +
> +               pm_genpd_init(&pmd->genpd, NULL, !is_on);
> +               pmu->genpd[entry->bit] = &pmd->genpd;
> +
> +               i++;
> +       }
> +
> +       spin_lock_init(&pmu->lock);
> +       starfive_pmu_int_enable(pmu, PMU_INT_ALL_MASK & ~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 starfive_pmu_data jh7110_power_domains[] = {
> +       {
> +               .name = "SYSTOP",
> +               .bit = JH7110_PD_SYSTOP,
> +               .flags = GENPD_FLAG_ALWAYS_ON,
> +       }, {
> +               .name = "CPU",
> +               .bit = JH7110_PD_CPU,
> +               .flags = GENPD_FLAG_ALWAYS_ON,
> +       }, {
> +               .name = "GPUA",
> +               .bit = JH7110_PD_GPUA,
> +       }, {
> +               .name = "VDEC",
> +               .bit = JH7110_PD_VDEC,
> +       }, {
> +               .name = "VOUT",
> +               .bit = JH7110_PD_VOUT,
> +       }, {
> +               .name = "ISP",
> +               .bit = JH7110_PD_ISP,
> +       }, {
> +               .name = "VENC",
> +               .bit = JH7110_PD_VENC,
> +       }, {
> +               .name = "GPUB",
> +               .bit = JH7110_PD_GPUB,
> +       }, {
> +               /* sentinel */
> +       },
> +};
> +
> +static const struct of_device_id starfive_pmu_of_match[] = {
> +       {
> +               .compatible = "starfive,jh7110-pmu",
> +               .data = &jh7110_power_domains,
> +       }, {
> +               /* sentinel */
> +       }
> +};
> +
> +static struct platform_driver jh71xx_pmu_driver = {
> +       .driver = {
> +               .name = "jh71xx-pmu",
> +               .of_match_table = starfive_pmu_of_match,
> +       },
> +       .probe  = starfive_pmu_probe,
> +};
> +builtin_platform_driver(jh71xx_pmu_driver);

You're using a mix of starfive_, starfive_pmu_ and jh71xx_pmu_
prefixes. Please choose one, preferably jh71xx_pmu_ since that's what
you call the driver.

> +MODULE_LICENSE("GPL");
> diff --git a/include/soc/starfive/pm_domains.h b/include/soc/starfive/pm_domains.h
> new file mode 100644
> index 000000000000..a20e28e9baf3
> --- /dev/null
> +++ b/include/soc/starfive/pm_domains.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + * Author: Walker Chen <walker.chen@starfivetech.com>
> + */
> +
> +#ifndef __SOC_STARFIVE_PMDOMAINS_H__
> +#define __SOC_STARFIVE_PMDOMAINS_H__
> +
> +#include <linux/types.h>
> +
> +void starfive_pmu_hw_event_turn_on(u32 mask);
> +void starfive_pmu_hw_event_turn_off(u32 mask);
> +
> +#endif /* __SOC_STARFIVE_PMDOMAINS_H__ */

The header and functions within are named very generic, but
implemented by the jh71xx-specific driver.

Also who should use this header? These functions are never called by
anything in this series.

> --
> 2.17.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 4/4] riscv: dts: starfive: add power controller node

[Emil Renner Berthing]

On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote:
>
> This adds the power controller node for the Starfive JH7110 SoC.
> The pmu needs to be used by other modules such as ISP, VPU, etc.
>
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>

Hi Walker,

You called the driver jh71xx which suggests it also applies to the
jh7100. Are you missing a node in the jh7100 device tree?

> ---
>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index c22e8f1d2640..fa7b60b82d71 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -356,6 +356,13 @@
>                         #gpio-cells = <2>;
>                 };
>
> +               pwrc: power-controller@17030000 {
> +                       compatible = "starfive,jh7110-pmu";
> +                       reg = <0x0 0x17030000 0x0 0x10000>;
> +                       interrupts = <111>;
> +                       #power-domain-cells = <1>;
> +               };
> +
>                 uart0: serial@10000000 {
>                         compatible = "snps,dw-apb-uart";
>                         reg = <0x0 0x10000000 0x0 0x10000>;
> --
> 2.17.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/4] JH7110 Power Domain Support

[Emil Renner Berthing]

On Fri, 18 Nov 2022 at 14:34, Walker Chen <walker.chen@starfivetech.com> wrote:
>
> This patchset adds power domain controller driver for the StarFive JH7110 SoC.
> The series has been tested on the VisionFive 2 board.

Hi Walker,

Thanks for upstreaming this! I've left some comments on the individual patches.

/Emil

> This patchset should be applied after the patchset [1], [2], [3]:
> [1] https://lore.kernel.org/all/20221118010627.70576-1-hal.feng@starfivetech.com/
> [2] https://lore.kernel.org/all/20221118011108.70715-1-hal.feng@starfivetech.com/
> [3] https://lore.kernel.org/all/20221118011714.70877-1-hal.feng@starfivetech.com/
>
> Walker Chen (4):
>   dt-bindings: power: Add StarFive JH7110 power domain definitions
>   dt-bindings: power: Add starfive,jh71xx-power bindings
>   soc: starfive: Add StarFive JH71XX pmu driver
>   riscv: dts: starfive: add power controller node
>
>  .../bindings/power/starfive,jh71xx-power.yaml |  46 +++
>  MAINTAINERS                                   |   8 +
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      |   7 +
>  drivers/soc/Kconfig                           |   1 +
>  drivers/soc/Makefile                          |   1 +
>  drivers/soc/starfive/Kconfig                  |   9 +
>  drivers/soc/starfive/Makefile                 |   3 +
>  drivers/soc/starfive/jh71xx_pmu.c             | 380 ++++++++++++++++++
>  include/dt-bindings/power/jh7110-power.h      |  18 +
>  include/soc/starfive/pm_domains.h             |  15 +
>  10 files changed, 488 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
>  create mode 100644 drivers/soc/starfive/Kconfig
>  create mode 100644 drivers/soc/starfive/Makefile
>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
>  create mode 100644 include/dt-bindings/power/jh7110-power.h
>  create mode 100644 include/soc/starfive/pm_domains.h
>
>
> base-commit: 094226ad94f471a9f19e8f8e7140a09c2625abaa
> prerequisite-patch-id: 8ebfffa09b478904bf7c516f76e2d824ddb60140
> prerequisite-patch-id: e8dd8258a4c4062eee2cf07c4607d52baea71f3a
> prerequisite-patch-id: d050d884d7b091ff30508a70f5ce5164bb3b72e5
> prerequisite-patch-id: 0e41f8cfd4861fcbf6f2e6a2559ce28f0450299e
> prerequisite-patch-id: 6e1652501859b85f101ff3b15ced585d43c71c1b
> prerequisite-patch-id: 587628a67adad5c655e5f998bf6c4a368ec07d3c
> prerequisite-patch-id: 596490c0e397df6c0249c1306fbb1d5bf00b5b83
> prerequisite-patch-id: dc873317826b50364344b25ac5cd74e811403f3d
> prerequisite-patch-id: a50150f41d8e874553023187e22eb24dffae8d16
> prerequisite-patch-id: 735e62255c75801bdc4c0b4107850bce821ff7f5
> prerequisite-patch-id: 9d2e83a2dd43e193f534283fab73e90b4f435043
> prerequisite-patch-id: 7a43e0849a9afa3c6f83547fd16d9271b07619e5
> prerequisite-patch-id: e7aa6fb05314bad6d94c465f3f59969871bf3d2e
> prerequisite-patch-id: 6276b2a23818c65ff2ad3d65b562615690cffee9
> prerequisite-patch-id: d834ece14ffb525b8c3e661e78736692f33fca9b
> prerequisite-patch-id: 4c17a3ce4dae9b788795d915bf775630f5c43c53
> prerequisite-patch-id: dabb913fd478e97593e45c23fee4be9fd807f851
> prerequisite-patch-id: ba61df106fbe2ada21e8f22c3d2cfaf7809c84b6
> prerequisite-patch-id: 287572fb64f83f5d931034f7c75674907584a087
> prerequisite-patch-id: 536114f0732646095ef5302a165672b3290d4c75
> prerequisite-patch-id: 258ea5f9b8bf41b6981345dcc81795f25865d38f
> prerequisite-patch-id: 8b6f2c9660c0ac0ee4e73e4c21aca8e6b75e81b9
> prerequisite-patch-id: e09e995700a814a763aa304ad3881a7222acf556
> prerequisite-patch-id: 841cd71b556b480d6a5a5e332eeca70d6a76ec3f
> prerequisite-patch-id: d074c7ffa2917a9f754d5801e3f67bc980f9de4c
> prerequisite-patch-id: 5f59bc7cbbf1230e5ff4761fa7c1116d4e6e5d71
> prerequisite-patch-id: d5da3475c6a3588e11a1678feb565bdd459b548e
> --
> 2.17.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Conor Dooley]

Hey Walker,

On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:
> Add generic power domain driver for the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  MAINTAINERS                       |   8 +

> diff --git a/MAINTAINERS b/MAINTAINERS
> index a70c1d0f303e..112f1e698723 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19623,6 +19623,14 @@ F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>  F:	drivers/reset/starfive/
>  F:	include/dt-bindings/reset/starfive*
>  
> +STARFIVE JH71XX PMU CONTROLLER DRIVER
> +M:	Walker Chen <walker.chen@starfivetech.com>
> +S:	Maintained

Should this be supported? (ditto the other series that you guys have
sent out in the last few days)

> +F:	Documentation/devicetree/bindings/power/starfive*
> +F:	drivers/soc/starfive/jh71xx_pmu.c
> +F:	include/soc/starfive/pm_domains.h
> +F:	include/dt-bindings/power/jh7110-power.h

I noticed that you have not CCed Arnd on these patches, which makes me
wonder how do you intend getting these patches applied?
Until now, Palmer has mostly merged RISC-V drivers/soc patches, but in
the last few days I've taken over for drivers/soc/{sifive,microchip}.

Are you going to send PRs for this stuff to Arnd, or would you like me
to apply patches for drivers/soc/startech? I happy to do that for you if
you like.

If you're going to send pull requests, I am not sure if Arnd requires
GPG signed tags for them. Arnd?

Otherwise, if you want me to take them, please add something like the
following, in addition to the entry your series already adds for the
specific driver:
STARFIVE SOC DRIVERS
M:	Conor Dooley <conor@kernel.org>
S:	Maintained
T:	git https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/
F:	drivers/soc/starfive/
F:	include/soc/starfive/

Thanks,
Conor.

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Conor Dooley]

Hey Walker,

On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:
> Add generic power domain driver for the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
> diff --git a/drivers/soc/starfive/Kconfig b/drivers/soc/starfive/Kconfig
> new file mode 100644
> index 000000000000..2bbcc1397b15
> --- /dev/null
> +++ b/drivers/soc/starfive/Kconfig
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config JH71XX_PMU
> +	bool "Support PMU for StarFive JH71XX Soc"
> +	depends on PM && (SOC_STARFIVE || COMPILE_TEST)

`default SOC_STARFIVE` perhaps?

> +	select PM_GENERIC_DOMAINS
> +	help
> +	  Say y here to enable power domain support.
> +
> diff --git a/drivers/soc/starfive/Makefile b/drivers/soc/starfive/Makefile
> new file mode 100644
> index 000000000000..13b589d6b5f3
> --- /dev/null
> +++ b/drivers/soc/starfive/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_JH71XX_PMU)	+= jh71xx_pmu.o
> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
> new file mode 100644
> index 000000000000..e6c0083d166e
> --- /dev/null
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * StarFive JH71XX Power Domain 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/jh7110-power.h>
>
> +/* register offset */
> +#define HW_EVENT_TURN_ON_MASK		0x04
> +#define HW_EVENT_TURN_OFF_MASK		0x08
> +#define SW_TURN_ON_POWER_MODE		0x0C
> +#define SW_TURN_OFF_POWER_MODE		0x10
> +#define SW_ENCOURAGE			0x44
> +#define PMU_INT_MASK			0x48
> +#define PCH_BYPASS			0x4C
> +#define PCH_PSTATE			0x50
> +#define PCH_TIMEOUT			0x54
> +#define LP_TIMEOUT			0x58
> +#define HW_TURN_ON_MODE			0x5C
> +#define CURR_POWER_MODE			0x80
> +#define PMU_EVENT_STATUS		0x88
> +#define PMU_INT_STATUS			0x8C
> +
> +/* sw encourage cfg */
> +#define SW_MODE_ENCOURAGE_EN_LO		0x05
> +#define SW_MODE_ENCOURAGE_EN_HI		0x50
> +#define SW_MODE_ENCOURAGE_DIS_LO	0x0A
> +#define SW_MODE_ENCOURAGE_DIS_HI	0xA0
> +#define SW_MODE_ENCOURAGE_ON		0xFF
> +
> +/* pmu int status */
> +#define PMU_INT_SEQ_DONE		BIT(0)
> +#define PMU_INT_HW_REQ			BIT(1)
> +#define PMU_INT_SW_FAIL			GENMASK(3, 2)
> +#define PMU_INT_HW_FAIL			GENMASK(5, 4)
> +#define PMU_INT_PCH_FAIL		GENMASK(8, 6)
> +#define PMU_INT_FAIL_MASK		(PMU_INT_SW_FAIL | \
> +					PMU_INT_HW_FAIL | \
> +					PMU_INT_PCH_FAIL)
> +#define PMU_INT_ALL_MASK		(PMU_INT_SEQ_DONE | \
> +					PMU_INT_HW_REQ | \
> +					PMU_INT_FAIL_MASK)
> +
> +#define DELAY_US			10
> +#define TIMEOUT_US			100000

Some JH71XX_PMU_ prefixes for the above, as I think Emil pointed out,
would be great.

> +struct starfive_power_dev {
> +	struct generic_pm_domain genpd;
> +	struct starfive_pmu *power;
> +	uint32_t mask;
> +};
> +
> +struct starfive_pmu {
> +	void __iomem *base;
> +	spinlock_t lock;
> +	int irq;
> +	struct device *pdev;
> +	struct starfive_power_dev *dev;
> +	struct genpd_onecell_data genpd_data;
> +	struct generic_pm_domain **genpd;
> +};
> +
> +struct starfive_pmu_data {
> +	const char * const name;
> +	uint8_t bit;
> +	unsigned int flags;

Generally, ordering pointers first in these structs would be nice.

> +};
> +
> +static void __iomem *pmu_base;
> +
> +static inline void pmu_writel(u32 val, u32 offset)
> +{
> +	writel(val, pmu_base + offset);
> +}
> +
> +void starfive_pmu_hw_event_turn_on(u32 mask)
> +{
> +	pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
> +
> +void starfive_pmu_hw_event_turn_off(u32 mask)
> +{
> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);

Where are the users for these exports? Also, should they be exported as
GPL?

Either way, what is the point of the extra layer of abstraction here
around the writel()?

> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
> +{
> +	struct starfive_pmu *pmu = pmd->power;
> +
> +	if (!pmd->mask) {
> +		*is_on = false;
> +		return -EINVAL;
> +	}
> +
> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;

Is there a specific reason that you are using the __raw variants here
(and elsewhere) in the driver?

> +
> +	return 0;
> +}
> +
> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
> +{
> +	struct starfive_pmu *pmu = pmd->power;
> +	unsigned long flags;
> +	uint32_t val;
> +	uint32_t mode;
> +	uint32_t encourage_lo;
> +	uint32_t encourage_hi;
> +	bool is_on;
> +	int ret;
> +
> +	if (!pmd->mask)
> +		return -EINVAL;
> +
> +	if (is_on == on) {
> +		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
> +				pmd->genpd.name, on ? "en" : "dis");

Am I missing something here: you've just declared is_on, so it must be
false & therefore this check is all a little pointless? The check just
becomes if (false == on) which I don't think is what you're going for
here? Or did I miss something obvious?

> +		return 0;
> +	}
> +
> +	spin_lock_irqsave(&pmu->lock, flags);
> +
> +	if (on) {
> +		mode = SW_TURN_ON_POWER_MODE;
> +		encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
> +		encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
> +	} else {
> +		mode = SW_TURN_OFF_POWER_MODE;
> +		encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
> +		encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
> +	}
> +
> +	__raw_writel(pmd->mask, pmu->base + mode);
> +
> +	/* write SW_ENCOURAGE to make the configuration take effect */
> +	__raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);

Is register "sticky"? IOW, could you set it in probe and leave this mode
always on? Or does it need to be set every time you want to use this
feature?

> +	__raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
> +	__raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	if (on) {
> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +						val & pmd->mask, DELAY_US,
> +						TIMEOUT_US);
> +		if (ret) {
> +			dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
> +			return -ETIMEDOUT;
> +		}
> +	} else {
> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +						!(val & pmd->mask), DELAY_US,
> +						TIMEOUT_US);

Could you not just decide the 3rd arg outside of the readl_poll..() and
save on duplicating the wait logic here?

> +		if (ret) {
> +			dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int starfive_pmu_on(struct generic_pm_domain *genpd)
> +{
> +	struct starfive_power_dev *pmd = container_of(genpd,
> +		struct starfive_power_dev, genpd);
> +
> +	return starfive_pmu_set_state(pmd, true);
> +}
> +
> +static int starfive_pmu_off(struct generic_pm_domain *genpd)
> +{
> +	struct starfive_power_dev *pmd = container_of(genpd,
> +		struct starfive_power_dev, genpd);
> +
> +	return starfive_pmu_set_state(pmd, false);
> +}
> +
> +static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
> +{
> +	u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
> +
> +	if (enable)
> +		val &= ~mask;
> +	else
> +		val |= mask;
> +
> +	__raw_writel(val, pmu->base + PMU_INT_MASK);
> +}
> +
> +static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
> +{
> +	struct starfive_pmu *pmu = data;
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&pmu->lock, flags);
> +	val = __raw_readl(pmu->base + PMU_INT_STATUS);
> +
> +	if (val & PMU_INT_SEQ_DONE)
> +		dev_dbg(pmu->pdev, "sequence done.\n");
> +	if (val & PMU_INT_HW_REQ)
> +		dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
> +	if (val & PMU_INT_SW_FAIL)
> +		dev_err(pmu->pdev, "software encourage fail.\n");
> +	if (val & PMU_INT_HW_FAIL)
> +		dev_err(pmu->pdev, "hardware encourage fail.\n");
> +	if (val & PMU_INT_PCH_FAIL)
> +		dev_err(pmu->pdev, "p-channel fail event.\n");
> +
> +	/* clear interrupts */
> +	__raw_writel(val, pmu->base + PMU_INT_STATUS);
> +	__raw_writel(val, pmu->base + PMU_EVENT_STATUS);
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int starfive_pmu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	const struct starfive_pmu_data *entry, *table;
> +	struct starfive_pmu *pmu;
> +	unsigned int i;
> +	uint8_t max_bit = 0;
> +	int ret;
> +
> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> +	if (!pmu)
> +		return -ENOMEM;
> +
> +	pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pmu->base))
> +		return PTR_ERR(pmu->base);
> +
> +	/* initialize pmu interrupt  */
> +	pmu->irq = platform_get_irq(pdev, 0);
> +	if (pmu->irq < 0)
> +		return pmu->irq;
> +
> +	ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
> +			       0, pdev->name, pmu);
> +	if (ret)
> +		dev_err(dev, "request irq failed.\n");
> +
> +	table = of_device_get_match_data(dev);
> +	if (!table)
> +		return -EINVAL;
> +
> +	pmu->pdev = dev;
> +	pmu->genpd_data.num_domains = 0;
> +	i = 0;
> +	for (entry = table; entry->name; entry++) {
> +		max_bit = max(max_bit, entry->bit);
> +		i++;
> +	}

This looks like something that could be replaced by the functions in
linux/list.h, no? Same below.

> +
> +	if (!i)
> +		return -ENODEV;
> +
> +	pmu->genpd_data.num_domains = max_bit + 1;
> +
> +	pmu->dev = devm_kcalloc(dev, pmu->genpd_data.num_domains,
> +				  sizeof(struct starfive_power_dev),
> +				  GFP_KERNEL);
> +	if (!pmu->dev)
> +		return -ENOMEM;
> +
> +	pmu->genpd = devm_kcalloc(dev, pmu->genpd_data.num_domains,
> +				    sizeof(struct generic_pm_domain *),
> +				    GFP_KERNEL);

This two allocations both look like checkpatch would whinge about their
alignment.

> +	if (!pmu->genpd)
> +		return -ENOMEM;
> +
> +	pmu->genpd_data.domains = pmu->genpd;
> +
> +	i = 0;
> +	for (entry = table; entry->name; entry++) {
> +		struct starfive_power_dev *pmd = &pmu->dev[i];
> +		bool is_on;
> +
> +		pmd->power = pmu;
> +		pmd->mask = BIT(entry->bit);
> +		pmd->genpd.name = entry->name;
> +		pmd->genpd.flags = entry->flags;
> +
> +		ret = starfive_pmu_get_state(pmd, &is_on);
> +		if (ret)
> +			dev_warn(dev, "unable to get current state for %s\n",
> +				 pmd->genpd.name);

In what scenario would it not be possible to get the state? Again, I
hope I'm not missing something obvious. From what I can see, this is the
only caller of starfive_pmu_get_state(), which looks like:
> > +	struct starfive_pmu *pmu = pmd->power;
> > +
> > +	if (!pmd->mask) {

How can !pmd->mask evaluate to true, given it's just been set to BIT(n)?

> > +		*is_on = false;
> > +		return -EINVAL;
> > +	}
> > +
> > +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
> > +
> > +	return 0;

If I've not missed something, looks like you could likely remove the
function and just do the readl() here?

> +
> +		pmd->genpd.power_on = starfive_pmu_on;
> +		pmd->genpd.power_off = starfive_pmu_off;
> +
> +		pm_genpd_init(&pmd->genpd, NULL, !is_on);
> +		pmu->genpd[entry->bit] = &pmd->genpd;
> +
> +		i++;
> +	}
> +
> +	spin_lock_init(&pmu->lock);
> +	starfive_pmu_int_enable(pmu, PMU_INT_ALL_MASK & ~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 starfive_pmu_data jh7110_power_domains[] = {

Is this driver jh7110 only or actually jh71XX? Have you just started out
by implementing one SoC both intend to support both?

> +	{
> +		.name = "SYSTOP",
> +		.bit = JH7110_PD_SYSTOP,
> +		.flags = GENPD_FLAG_ALWAYS_ON,
> +	}, {
> +		.name = "CPU",
> +		.bit = JH7110_PD_CPU,
> +		.flags = GENPD_FLAG_ALWAYS_ON,
> +	}, {
> +		.name = "GPUA",
> +		.bit = JH7110_PD_GPUA,
> +	}, {
> +		.name = "VDEC",
> +		.bit = JH7110_PD_VDEC,
> +	}, {
> +		.name = "VOUT",
> +		.bit = JH7110_PD_VOUT,
> +	}, {
> +		.name = "ISP",
> +		.bit = JH7110_PD_ISP,
> +	}, {
> +		.name = "VENC",
> +		.bit = JH7110_PD_VENC,
> +	}, {
> +		.name = "GPUB",
> +		.bit = JH7110_PD_GPUB,
> +	}, {
> +		/* sentinel */
> +	},

Can drop this , after the sentinel ;)

I don't know /jack/ about power domain stuff, so I can barely review
this at all sadly.
Thanks,
Conor.

> +};
> +
> +static const struct of_device_id starfive_pmu_of_match[] = {
> +	{
> +		.compatible = "starfive,jh7110-pmu",
> +		.data = &jh7110_power_domains,
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +
> +static struct platform_driver jh71xx_pmu_driver = {
> +	.driver = {
> +		.name = "jh71xx-pmu",
> +		.of_match_table = starfive_pmu_of_match,
> +	},
> +	.probe  = starfive_pmu_probe,
> +};
> +builtin_platform_driver(jh71xx_pmu_driver);

Re: [PATCH v1 1/4] dt-bindings: power: Add StarFive JH7110 power domain definitions

[Krzysztof Kozlowski]

On 18/11/2022 14:32, Walker Chen wrote:
> Add power domain definitions for the StarFive JH7110 SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  include/dt-bindings/power/jh7110-power.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 include/dt-bindings/power/jh7110-power.h
> 
> diff --git a/include/dt-bindings/power/jh7110-power.h b/include/dt-bindings/power/jh7110-power.h
> new file mode 100644
> index 000000000000..24160c46fbaf
> --- /dev/null
> +++ b/include/dt-bindings/power/jh7110-power.h

Filename matching compatible or bindings file.

> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: (GPL-2.0) */

Dual license for bindings.

> +/*
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + * Author: Walker Chen <walker.chen@starfivetech.com>
> + */
> +#ifndef __DT_BINDINGS_POWER_JH7110_POWER_H__
> +#define __DT_BINDINGS_POWER_JH7110_POWER_H__

Best regards,
Krzysztof

Re: [PATCH v1 2/4] dt-bindings: power: Add starfive,jh71xx-power bindings

[Krzysztof Kozlowski]

On 18/11/2022 14:32, Walker Chen wrote:
> Add bindings for the power domain controller on the StarFive JH71XX SoC.
> 

Subject: drop second, redundant "bindings".

> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  .../bindings/power/starfive,jh71xx-power.yaml | 46 +++++++++++++++++++

1st patch should be squashed here. Headers are part of bindings file.

>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
> new file mode 100644
> index 000000000000..2537303b4829
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml

Filename like compatible.

> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/starfive,jh71xx-power.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH71xx Power Domains Controller
> +
> +maintainers:
> +  - Walker Chen <walker.chen@starfivetech.com>
> +
> +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:
> +    items:

No items. You have just one item,

> +      - enum:
> +          - starfive,jh7110-pmu
> +

Best regards,
Krzysztof

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Krzysztof Kozlowski]

On 18/11/2022 14:32, Walker Chen wrote:
> Add generic power domain driver for the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>

Thank you for your patch. There is something to discuss/improve.

> +
> +MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>");
> +MODULE_DESCRIPTION("StarFive JH71XX Power Domain Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/soc/starfive/pm_domains.h b/include/soc/starfive/pm_domains.h
> new file mode 100644
> index 000000000000..a20e28e9baf3
> --- /dev/null
> +++ b/include/soc/starfive/pm_domains.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + * Author: Walker Chen <walker.chen@starfivetech.com>
> + */
> +
> +#ifndef __SOC_STARFIVE_PMDOMAINS_H__
> +#define __SOC_STARFIVE_PMDOMAINS_H__
> +
> +#include <linux/types.h>
> +
> +void starfive_pmu_hw_event_turn_on(u32 mask);
> +void starfive_pmu_hw_event_turn_off(u32 mask);

These are not used outside of driver. Drop entire header file.


Best regards,
Krzysztof

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Kevin Hilman]

Hi Walker,

Walker Chen <walker.chen@starfivetech.com> writes:

> Add generic power domain driver for the StarFive JH71XX SoC.
>
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>

A bit more detail about the features power domain hardware would be
helpful for reviewers.  I'm assuming there's no public documenation for
this, but if there is, a link to that would be great also.

[...]

> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
> new file mode 100644
> index 000000000000..e6c0083d166e
> --- /dev/null
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * StarFive JH71XX Power Domain 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/jh7110-power.h>
> +
> +/* register offset */
> +#define HW_EVENT_TURN_ON_MASK		0x04
> +#define HW_EVENT_TURN_OFF_MASK		0x08
> +#define SW_TURN_ON_POWER_MODE		0x0C
> +#define SW_TURN_OFF_POWER_MODE		0x10
> +#define SW_ENCOURAGE			0x44
> +#define PMU_INT_MASK			0x48
> +#define PCH_BYPASS			0x4C
> +#define PCH_PSTATE			0x50
> +#define PCH_TIMEOUT			0x54
> +#define LP_TIMEOUT			0x58
> +#define HW_TURN_ON_MODE			0x5C
> +#define CURR_POWER_MODE			0x80
> +#define PMU_EVENT_STATUS		0x88
> +#define PMU_INT_STATUS			0x8C
> +
> +/* sw encourage cfg */
> +#define SW_MODE_ENCOURAGE_EN_LO		0x05
> +#define SW_MODE_ENCOURAGE_EN_HI		0x50
> +#define SW_MODE_ENCOURAGE_DIS_LO	0x0A
> +#define SW_MODE_ENCOURAGE_DIS_HI	0xA0
> +#define SW_MODE_ENCOURAGE_ON		0xFF
> +
> +/* pmu int status */
> +#define PMU_INT_SEQ_DONE		BIT(0)
> +#define PMU_INT_HW_REQ			BIT(1)
> +#define PMU_INT_SW_FAIL			GENMASK(3, 2)
> +#define PMU_INT_HW_FAIL			GENMASK(5, 4)
> +#define PMU_INT_PCH_FAIL		GENMASK(8, 6)
> +#define PMU_INT_FAIL_MASK		(PMU_INT_SW_FAIL | \
> +					PMU_INT_HW_FAIL | \
> +					PMU_INT_PCH_FAIL)
> +#define PMU_INT_ALL_MASK		(PMU_INT_SEQ_DONE | \
> +					PMU_INT_HW_REQ | \
> +					PMU_INT_FAIL_MASK)
> +
> +#define DELAY_US			10
> +#define TIMEOUT_US			100000

Where do these delay/timeout numbers come from?  Is this just based one
experimentation, or is this something described by the HW docs.  Please
add some comments.

> +
> +struct starfive_power_dev {
> +	struct generic_pm_domain genpd;
> +	struct starfive_pmu *power;
> +	uint32_t mask;
> +};
> +
> +struct starfive_pmu {
> +	void __iomem *base;
> +	spinlock_t lock;
> +	int irq;
> +	struct device *pdev;
> +	struct starfive_power_dev *dev;
> +	struct genpd_onecell_data genpd_data;
> +	struct generic_pm_domain **genpd;
> +};
> +
> +struct starfive_pmu_data {
> +	const char * const name;
> +	uint8_t bit;
> +	unsigned int flags;
> +};
> +
> +static void __iomem *pmu_base;
> +
> +static inline void pmu_writel(u32 val, u32 offset)
> +{
> +	writel(val, pmu_base + offset);
> +}

You use writel() in this helper, but __raw_writel() throughout the rest
of the driver.  If that's intentional, you should explain a bit more
about why.  If not, please make it consistent.

If you're going to use a wrapper/helper, you should use it throughout.
But in this case, I'm not sure it's adding anything to readability.  In
fact, it's only adding confusion since you don't use it for most of the
register accesses.

> +void starfive_pmu_hw_event_turn_on(u32 mask)
> +{
> +	pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
> +
> +void starfive_pmu_hw_event_turn_off(u32 mask)
> +{
> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);

This looks wrong.  Why are you allowing external users to power on/off the
domains?  The sate of the domain should be managed by the PM core (and
runtime PM use-counts etc.) allowing external users to change the domain
state is going to lead to the PM core being confused about the state of
the domain.

> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
> +{
> +	struct starfive_pmu *pmu = pmd->power;
> +
> +	if (!pmd->mask) {
> +		*is_on = false;
> +		return -EINVAL;
> +	}
> +
> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
> +
> +	return 0;
> +}
> +
> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
> +{
> +	struct starfive_pmu *pmu = pmd->power;
> +	unsigned long flags;
> +	uint32_t val;
> +	uint32_t mode;
> +	uint32_t encourage_lo;
> +	uint32_t encourage_hi;
> +	bool is_on;
> +	int ret;
> +
> +	if (!pmd->mask)
> +		return -EINVAL;
> +
> +	if (is_on == on) {

Comparing an uninitialzed stack variable to 'on'?

> +		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
> +				pmd->genpd.name, on ? "en" : "dis");

Using dev_info() will probably make this a bit verbose.  I'd suggest
dev_dbg() for this kind of message.

> +		return 0;
> +	}
> +
> +	spin_lock_irqsave(&pmu->lock, flags);
> +
> +	if (on) {
> +		mode = SW_TURN_ON_POWER_MODE;
> +		encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
> +		encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
> +	} else {
> +		mode = SW_TURN_OFF_POWER_MODE;
> +		encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
> +		encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
> +	}
> +
> +	__raw_writel(pmd->mask, pmu->base + mode);
> +
> +	/* write SW_ENCOURAGE to make the configuration take effect */
> +	__raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
> +	__raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
> +	__raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);

This "encourage" feature is peculiar.  What happens if you do not do
this?  Does it take a lot longer for the domain to change state? or does
it not change at all?  In the absence of HW specs, a bit of description
here would be helpful.

> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	if (on) {
> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +						val & pmd->mask, DELAY_US,
> +						TIMEOUT_US);
> +		if (ret) {
> +			dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
> +			return -ETIMEDOUT;
> +		}
> +	} else {
> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +						!(val & pmd->mask), DELAY_US,
> +						TIMEOUT_US);
> +		if (ret) {
> +			dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int starfive_pmu_on(struct generic_pm_domain *genpd)
> +{
> +	struct starfive_power_dev *pmd = container_of(genpd,
> +		struct starfive_power_dev, genpd);
> +
> +	return starfive_pmu_set_state(pmd, true);
> +}
> +
> +static int starfive_pmu_off(struct generic_pm_domain *genpd)
> +{
> +	struct starfive_power_dev *pmd = container_of(genpd,
> +		struct starfive_power_dev, genpd);
> +
> +	return starfive_pmu_set_state(pmd, false);
> +}
> +
> +static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
> +{
> +	u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
> +
> +	if (enable)
> +		val &= ~mask;
> +	else
> +		val |= mask;
> +
> +	__raw_writel(val, pmu->base + PMU_INT_MASK);
> +}
> +
> +static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
> +{
> +	struct starfive_pmu *pmu = data;
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&pmu->lock, flags);

I don't think you need to spinlock in the interrupt itself, as
interrupts will already be disabled, and this handler is not
IRQF_SHARED.

> +	val = __raw_readl(pmu->base + PMU_INT_STATUS);
> +
> +	if (val & PMU_INT_SEQ_DONE)
> +		dev_dbg(pmu->pdev, "sequence done.\n");
> +	if (val & PMU_INT_HW_REQ)
> +		dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
> +	if (val & PMU_INT_SW_FAIL)
> +		dev_err(pmu->pdev, "software encourage fail.\n");
> +	if (val & PMU_INT_HW_FAIL)
> +		dev_err(pmu->pdev, "hardware encourage fail.\n");
> +	if (val & PMU_INT_PCH_FAIL)
> +		dev_err(pmu->pdev, "p-channel fail event.\n");
> +
> +	/* clear interrupts */
> +	__raw_writel(val, pmu->base + PMU_INT_STATUS);
> +	__raw_writel(val, pmu->base + PMU_EVENT_STATUS);
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	return IRQ_HANDLED;
> +}
> +

[...]

Kevin

Re: [PATCH v1 1/4] dt-bindings: power: Add StarFive JH7110 power domain definitions

[Walker Chen]

On 2022/11/21 18:12, Krzysztof Kozlowski wrote:
> On 18/11/2022 14:32, Walker Chen wrote:
>> Add power domain definitions for the StarFive JH7110 SoC.
>>
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  include/dt-bindings/power/jh7110-power.h | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>  create mode 100644 include/dt-bindings/power/jh7110-power.h
>>
>> diff --git a/include/dt-bindings/power/jh7110-power.h b/include/dt-bindings/power/jh7110-power.h
>> new file mode 100644
>> index 000000000000..24160c46fbaf
>> --- /dev/null
>> +++ b/include/dt-bindings/power/jh7110-power.h
> 
> Filename matching compatible or bindings file.

So the file name should be changed to "starfive,jh7110-power.h" and the compatible in the driver 
should also be changed to "starfive,jh7110-power". Is it right ?

> 
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: (GPL-2.0) */
> 
> Dual license for bindings.

Ok, the license will be changed to GPL-2.0 or MIT in the patch v2. 

> 
>> +/*
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + * Author: Walker Chen <walker.chen@starfivetech.com>
>> + */
>> +#ifndef __DT_BINDINGS_POWER_JH7110_POWER_H__
>> +#define __DT_BINDINGS_POWER_JH7110_POWER_H__
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 1/4] dt-bindings: power: Add StarFive JH7110 power domain definitions

[Krzysztof Kozlowski]

On 22/11/2022 08:46, Walker Chen wrote:
> On 2022/11/21 18:12, Krzysztof Kozlowski wrote:
>> On 18/11/2022 14:32, Walker Chen wrote:
>>> Add power domain definitions for the StarFive JH7110 SoC.
>>>
>>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>>> ---
>>>  include/dt-bindings/power/jh7110-power.h | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>  create mode 100644 include/dt-bindings/power/jh7110-power.h
>>>
>>> diff --git a/include/dt-bindings/power/jh7110-power.h b/include/dt-bindings/power/jh7110-power.h
>>> new file mode 100644
>>> index 000000000000..24160c46fbaf
>>> --- /dev/null
>>> +++ b/include/dt-bindings/power/jh7110-power.h
>>
>> Filename matching compatible or bindings file.
> 
> So the file name should be changed to "starfive,jh7110-power.h" and the compatible in the driver 
> should also be changed to "starfive,jh7110-power". Is it right ?

I said filename should be changed. I don't remember what was your
compatible, but if I did not comment there, in means it looked fine.

> 
>>
>>> @@ -0,0 +1,18 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0) */
>>
>> Dual license for bindings.
> 
> Ok, the license will be changed to GPL-2.0 or MIT in the patch v2. 

Any reasons why not using the licenses recommended by checkpatch?


Best regards,
Krzysztof

Re: [PATCH v1 1/4] dt-bindings: power: Add StarFive JH7110 power domain definitions

[Walker Chen]

On 2022/11/22 16:03, Krzysztof Kozlowski wrote:
> On 22/11/2022 08:46, Walker Chen wrote:
>> On 2022/11/21 18:12, Krzysztof Kozlowski wrote:
>>> On 18/11/2022 14:32, Walker Chen wrote:
>>>> Add power domain definitions for the StarFive JH7110 SoC.
>>>>
>>>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>>>> ---
>>>>  include/dt-bindings/power/jh7110-power.h | 18 ++++++++++++++++++
>>>>  1 file changed, 18 insertions(+)
>>>>  create mode 100644 include/dt-bindings/power/jh7110-power.h
>>>>
>>>> diff --git a/include/dt-bindings/power/jh7110-power.h b/include/dt-bindings/power/jh7110-power.h
>>>> new file mode 100644
>>>> index 000000000000..24160c46fbaf
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/power/jh7110-power.h
>>>
>>> Filename matching compatible or bindings file.
>> 
>> So the file name should be changed to "starfive,jh7110-power.h" and the compatible in the driver 
>> should also be changed to "starfive,jh7110-power". Is it right ?
> 
> I said filename should be changed. I don't remember what was your
> compatible, but if I did not comment there, in means it looked fine.
> 
>> 
>>>
>>>> @@ -0,0 +1,18 @@
>>>> +/* SPDX-License-Identifier: (GPL-2.0) */
>>>
>>> Dual license for bindings.
>> 
>> Ok, the license will be changed to GPL-2.0 or MIT in the patch v2. 
> 
> Any reasons why not using the licenses recommended by checkpatch?
> 
Well, it sounds like a better way to decide which license to use by checkpatch.
Thanks for your tip.

Best Regards,
Walker Chen

Re: [PATCH v1 2/4] dt-bindings: power: Add starfive,jh71xx-power bindings

[Walker Chen]

On 2022/11/21 18:13, Krzysztof Kozlowski wrote:
> On 18/11/2022 14:32, Walker Chen wrote:
>> Add bindings for the power domain controller on the StarFive JH71XX SoC.
>> 
> 
> Subject: drop second, redundant "bindings".

Will fix.

> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  .../bindings/power/starfive,jh71xx-power.yaml | 46 +++++++++++++++++++
> 
> 1st patch should be squashed here. Headers are part of bindings file.

Will be done in the next version of patch.

> 
>>  1 file changed, 46 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
>> new file mode 100644
>> index 000000000000..2537303b4829
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
> 
> Filename like compatible.

As mentioned in the previous email, the compatible in the driver should be changed to "starfive,jh7110-power".

> 
>> @@ -0,0 +1,46 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/starfive,jh71xx-power.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive JH71xx Power Domains Controller
>> +
>> +maintainers:
>> +  - Walker Chen <walker.chen@starfivetech.com>
>> +
>> +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:
>> +    items:
> 
> No items. You have just one item,

Yes, this issue will be fixed. Thank you for your advice.

Best Regards,
Walker Chen

Re: [PATCH v1 4/4] riscv: dts: starfive: add power controller node

[Walker Chen]

On 2022/11/19 2:36, Emil Renner Berthing wrote:
> On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote:
>>
>> This adds the power controller node for the Starfive JH7110 SoC.
>> The pmu needs to be used by other modules such as ISP, VPU, etc.
>>
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> 
> Hi Walker,
> 
> You called the driver jh71xx which suggests it also applies to the
> jh7100. Are you missing a node in the jh7100 device tree?

No, there is no power domain controller on the jh7100. Our next generation of chips jh7120 will 
still use this power management unit, so here this driver name is called jh71xx_pmu.c or changed
to jh71xx_power.c , do you think such a name is appropriate ?  
Your reply will be highly appreciated!

> 
>> ---
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> index c22e8f1d2640..fa7b60b82d71 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -356,6 +356,13 @@
>>                         #gpio-cells = <2>;
>>                 };
>>
>> +               pwrc: power-controller@17030000 {
>> +                       compatible = "starfive,jh7110-pmu";
>> +                       reg = <0x0 0x17030000 0x0 0x10000>;
>> +                       interrupts = <111>;
>> +                       #power-domain-cells = <1>;
>> +               };
>> +
>>                 uart0: serial@10000000 {
>>                         compatible = "snps,dw-apb-uart";
>>                         reg = <0x0 0x10000000 0x0 0x10000>;
>> --
>> 2.17.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/11/19 2:31, Emil Renner Berthing wrote:
> On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote:
>>
>> Add generic power domain driver for the StarFive JH71XX SoC.
> 
> SoCs
> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  MAINTAINERS                       |   8 +
>>  drivers/soc/Kconfig               |   1 +
>>  drivers/soc/Makefile              |   1 +
>>  drivers/soc/starfive/Kconfig      |   9 +
>>  drivers/soc/starfive/Makefile     |   3 +
>>  drivers/soc/starfive/jh71xx_pmu.c | 380 ++++++++++++++++++++++++++++++
>>  include/soc/starfive/pm_domains.h |  15 ++
>>  7 files changed, 417 insertions(+)
>>  create mode 100644 drivers/soc/starfive/Kconfig
>>  create mode 100644 drivers/soc/starfive/Makefile
>>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
>>  create mode 100644 include/soc/starfive/pm_domains.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a70c1d0f303e..112f1e698723 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19623,6 +19623,14 @@ F:     Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>>  F:     drivers/reset/starfive/
>>  F:     include/dt-bindings/reset/starfive*
> 
> checkpatch.pl complains about not sorting these lines alphabetically.

This warning will be fixed in the next versioin of patch. 

> 
>> +STARFIVE JH71XX PMU CONTROLLER DRIVER
>> +M:     Walker Chen <walker.chen@starfivetech.com>
>> +S:     Maintained
>> +F:     Documentation/devicetree/bindings/power/starfive*
>> +F:     drivers/soc/starfive/jh71xx_pmu.c
>> +F:     include/soc/starfive/pm_domains.h
>> +F:     include/dt-bindings/power/jh7110-power.h
>> +
>>  STATIC BRANCH/CALL
>>  M:     Peter Zijlstra <peterz@infradead.org>
>>  M:     Josh Poimboeuf <jpoimboe@kernel.org>
>> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
>> index e461c071189b..628fda4d5ed9 100644
>> --- a/drivers/soc/Kconfig
>> +++ b/drivers/soc/Kconfig
>> @@ -21,6 +21,7 @@ source "drivers/soc/renesas/Kconfig"
>>  source "drivers/soc/rockchip/Kconfig"
>>  source "drivers/soc/samsung/Kconfig"
>>  source "drivers/soc/sifive/Kconfig"
>> +source "drivers/soc/starfive/Kconfig"
>>  source "drivers/soc/sunxi/Kconfig"
>>  source "drivers/soc/tegra/Kconfig"
>>  source "drivers/soc/ti/Kconfig"
>> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
>> index 534669840858..cbe076f42068 100644
>> --- a/drivers/soc/Makefile
>> +++ b/drivers/soc/Makefile
>> @@ -27,6 +27,7 @@ obj-y                         += renesas/
>>  obj-y                          += rockchip/
>>  obj-$(CONFIG_SOC_SAMSUNG)      += samsung/
>>  obj-y                          += sifive/
>> +obj-y                          += starfive/
>>  obj-y                          += sunxi/
>>  obj-$(CONFIG_ARCH_TEGRA)       += tegra/
>>  obj-y                          += ti/
>> diff --git a/drivers/soc/starfive/Kconfig b/drivers/soc/starfive/Kconfig
>> new file mode 100644
>> index 000000000000..2bbcc1397b15
>> --- /dev/null
>> +++ b/drivers/soc/starfive/Kconfig
>> @@ -0,0 +1,9 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +config JH71XX_PMU
>> +       bool "Support PMU for StarFive JH71XX Soc"
>> +       depends on PM && (SOC_STARFIVE || COMPILE_TEST)
>> +       select PM_GENERIC_DOMAINS
>> +       help
>> +         Say y here to enable power domain support.
> 
> Please expand this help text. Even checkpatch.pl complains about it.

I think it should be described in this detail:
Say 'y' here to enables support for Power Management Unit that 
is used for enabling and disabling SoC devices.

> 
>> +
>> diff --git a/drivers/soc/starfive/Makefile b/drivers/soc/starfive/Makefile
>> new file mode 100644
>> index 000000000000..13b589d6b5f3
>> --- /dev/null
>> +++ b/drivers/soc/starfive/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +obj-$(CONFIG_JH71XX_PMU)       += jh71xx_pmu.o
>> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
>> new file mode 100644
>> index 000000000000..e6c0083d166e
>> --- /dev/null
>> +++ b/drivers/soc/starfive/jh71xx_pmu.c
>> @@ -0,0 +1,380 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * StarFive JH71XX Power Domain 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/jh7110-power.h>
>> +
>> +/* register offset */
>> +#define HW_EVENT_TURN_ON_MASK          0x04
>> +#define HW_EVENT_TURN_OFF_MASK         0x08
>> +#define SW_TURN_ON_POWER_MODE          0x0C
>> +#define SW_TURN_OFF_POWER_MODE         0x10
>> +#define SW_ENCOURAGE                   0x44
>> +#define PMU_INT_MASK                   0x48
>> +#define PCH_BYPASS                     0x4C
>> +#define PCH_PSTATE                     0x50
>> +#define PCH_TIMEOUT                    0x54
>> +#define LP_TIMEOUT                     0x58
>> +#define HW_TURN_ON_MODE                        0x5C
>> +#define CURR_POWER_MODE                        0x80
>> +#define PMU_EVENT_STATUS               0x88
>> +#define PMU_INT_STATUS                 0x8C
>> +
>> +/* sw encourage cfg */
>> +#define SW_MODE_ENCOURAGE_EN_LO                0x05
>> +#define SW_MODE_ENCOURAGE_EN_HI                0x50
>> +#define SW_MODE_ENCOURAGE_DIS_LO       0x0A
>> +#define SW_MODE_ENCOURAGE_DIS_HI       0xA0
>> +#define SW_MODE_ENCOURAGE_ON           0xFF
>> +
>> +/* pmu int status */
>> +#define PMU_INT_SEQ_DONE               BIT(0)
>> +#define PMU_INT_HW_REQ                 BIT(1)
>> +#define PMU_INT_SW_FAIL                        GENMASK(3, 2)
>> +#define PMU_INT_HW_FAIL                        GENMASK(5, 4)
>> +#define PMU_INT_PCH_FAIL               GENMASK(8, 6)
>> +#define PMU_INT_FAIL_MASK              (PMU_INT_SW_FAIL | \
>> +                                       PMU_INT_HW_FAIL | \
>> +                                       PMU_INT_PCH_FAIL)
>> +#define PMU_INT_ALL_MASK               (PMU_INT_SEQ_DONE | \
>> +                                       PMU_INT_HW_REQ | \
>> +                                       PMU_INT_FAIL_MASK)
>> +
>> +#define DELAY_US                       10
>> +#define TIMEOUT_US                     100000
> 
> Please add a JH71XX_PMU_ prefix to these definitions to avoid
> accidentally clashing with a definition added to a generic header.

Good advice! Will be changed in the next version of patch.

> 
>> +
>> +struct starfive_power_dev {
>> +       struct generic_pm_domain genpd;
>> +       struct starfive_pmu *power;
>> +       uint32_t mask;
> 
> u32

Yes, checkpatch.pl complains about this. Will be fixed.

> 
>> +};
>> +
>> +struct starfive_pmu {
>> +       void __iomem *base;
>> +       spinlock_t lock;
>> +       int irq;
>> +       struct device *pdev;
>> +       struct starfive_power_dev *dev;
>> +       struct genpd_onecell_data genpd_data;
>> +       struct generic_pm_domain **genpd;
>> +};
>> +
>> +struct starfive_pmu_data {
>> +       const char * const name;
>> +       uint8_t bit;
> 
> u8

Yes, checkpatch.pl complains about this. Will be fixed.

> 
>> +       unsigned int flags;
>> +};
>> +
>> +static void __iomem *pmu_base;
>> +
>> +static inline void pmu_writel(u32 val, u32 offset)
>> +{
>> +       writel(val, pmu_base + offset);
>> +}
>> +
>> +void starfive_pmu_hw_event_turn_on(u32 mask)
>> +{
>> +       pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
>> +}
>> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
>> +
>> +void starfive_pmu_hw_event_turn_off(u32 mask)
>> +{
>> +       pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
>> +}
>> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
>> +
>> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
>> +{
>> +       struct starfive_pmu *pmu = pmd->power;
>> +
>> +       if (!pmd->mask) {
>> +               *is_on = false;
>> +               return -EINVAL;
>> +       }
>> +
>> +       *is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
>> +
>> +       return 0;
>> +}
> 
> Maybe just return an int where 0 = off, 1 = on and negative is an error.

No obvious problems, this usage is referenced from /drivers/soc/bcm/bcm63xx/bcm63xx-power.c
Maybe drop this function, read the status of power domain to is_on directly before calling pm_genpd_init().

> 
>> +
>> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
>> +{
>> +       struct starfive_pmu *pmu = pmd->power;
>> +       unsigned long flags;
>> +       uint32_t val;
>> +       uint32_t mode;
>> +       uint32_t encourage_lo;
>> +       uint32_t encourage_hi;
> 
> u32

Yes, checkpatch.pl complains about this. Will be fixed.

> 
>> +       bool is_on;
>> +       int ret;
>> +
>> +       if (!pmd->mask)
>> +               return -EINVAL;
>> +
>> +       if (is_on == on) {
>> +               dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
>> +                               pmd->genpd.name, on ? "en" : "dis");
>> +               return 0;
>> +       }
>> +
>> +       spin_lock_irqsave(&pmu->lock, flags);
>> +
>> +       if (on) {
>> +               mode = SW_TURN_ON_POWER_MODE;
>> +               encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
>> +               encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
>> +       } else {
>> +               mode = SW_TURN_OFF_POWER_MODE;
>> +               encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
>> +               encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
>> +       }
>> +
>> +       __raw_writel(pmd->mask, pmu->base + mode);
>> +
>> +       /* write SW_ENCOURAGE to make the configuration take effect */
>> +       __raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
>> +       __raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
>> +       __raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
>> +
>> +       spin_unlock_irqrestore(&pmu->lock, flags);
>> +
>> +       if (on) {
>> +               ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
>> +                                               val & pmd->mask, DELAY_US,
>> +                                               TIMEOUT_US);
>> +               if (ret) {
>> +                       dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
>> +                       return -ETIMEDOUT;
>> +               }
>> +       } else {
>> +               ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
>> +                                               !(val & pmd->mask), DELAY_US,
>> +                                               TIMEOUT_US);
>> +               if (ret) {
>> +                       dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
>> +                       return -ETIMEDOUT;
>> +               }
>> +       }
> 
> You can move the dev_err out. Eg. dev_err(pmu->pdev, "%s: failed to
> power %s\n", pmd->genpd.name, on ? "on" : "off")

I think use only one readl_poll_timeout_atomic() to distinguish between different situation 
according to the 3rd argument outside of the readl_poll..().

> 
>> +       return 0;
>> +}
>> +
>> +static int starfive_pmu_on(struct generic_pm_domain *genpd)
>> +{
>> +       struct starfive_power_dev *pmd = container_of(genpd,
>> +               struct starfive_power_dev, genpd);
> 
> checkpatch.pl --strict complains that arguments doesn't match the open
> parenthesis

Yes, checkpatch.pl complains about this. Will be fixed.

> 
>> +
>> +       return starfive_pmu_set_state(pmd, true);
>> +}
>> +
>> +static int starfive_pmu_off(struct generic_pm_domain *genpd)
>> +{
>> +       struct starfive_power_dev *pmd = container_of(genpd,
>> +               struct starfive_power_dev, genpd);
>> +
>> +       return starfive_pmu_set_state(pmd, false);
>> +}
>> +
>> +static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
>> +{
>> +       u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
>> +
>> +       if (enable)
>> +               val &= ~mask;
>> +       else
>> +               val |= mask;
>> +
>> +       __raw_writel(val, pmu->base + PMU_INT_MASK);
>> +}
> 
> Consider adding comments to functions that need a lock to be held to
> be used safely.

Well, your advice is very helpful.

> 
>> +
>> +static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
>> +{
>> +       struct starfive_pmu *pmu = data;
>> +       unsigned long flags;
>> +       u32 val;
>> +
>> +       spin_lock_irqsave(&pmu->lock, flags);
>> +       val = __raw_readl(pmu->base + PMU_INT_STATUS);
>> +
>> +       if (val & PMU_INT_SEQ_DONE)
>> +               dev_dbg(pmu->pdev, "sequence done.\n");
>> +       if (val & PMU_INT_HW_REQ)
>> +               dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
>> +       if (val & PMU_INT_SW_FAIL)
>> +               dev_err(pmu->pdev, "software encourage fail.\n");
>> +       if (val & PMU_INT_HW_FAIL)
>> +               dev_err(pmu->pdev, "hardware encourage fail.\n");
>> +       if (val & PMU_INT_PCH_FAIL)
>> +               dev_err(pmu->pdev, "p-channel fail event.\n");
>> +
>> +       /* clear interrupts */
>> +       __raw_writel(val, pmu->base + PMU_INT_STATUS);
>> +       __raw_writel(val, pmu->base + PMU_EVENT_STATUS);
>> +
>> +       spin_unlock_irqrestore(&pmu->lock, flags);
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static int starfive_pmu_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +       const struct starfive_pmu_data *entry, *table;
>> +       struct starfive_pmu *pmu;
>> +       unsigned int i;
>> +       uint8_t max_bit = 0;
>> +       int ret;
>> +
>> +       pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
>> +       if (!pmu)
>> +               return -ENOMEM;
>> +
>> +       pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
>> +       if (IS_ERR(pmu->base))
>> +               return PTR_ERR(pmu->base);
>> +
>> +       /* initialize pmu interrupt  */
>> +       pmu->irq = platform_get_irq(pdev, 0);
>> +       if (pmu->irq < 0)
>> +               return pmu->irq;
>> +
>> +       ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
>> +                              0, pdev->name, pmu);
>> +       if (ret)
>> +               dev_err(dev, "request irq failed.\n");
>> +
>> +       table = of_device_get_match_data(dev);
>> +       if (!table)
>> +               return -EINVAL;
>> +
>> +       pmu->pdev = dev;
>> +       pmu->genpd_data.num_domains = 0;
>> +       i = 0;
>> +       for (entry = table; entry->name; entry++) {
>> +               max_bit = max(max_bit, entry->bit);
>> +               i++;
>> +       }
>> +
>> +       if (!i)
>> +               return -ENODEV;
>> +
>> +       pmu->genpd_data.num_domains = max_bit + 1;
>> +
>> +       pmu->dev = devm_kcalloc(dev, pmu->genpd_data.num_domains,
>> +                                 sizeof(struct starfive_power_dev),
>> +                                 GFP_KERNEL);
>> +       if (!pmu->dev)
>> +               return -ENOMEM;
>> +
>> +       pmu->genpd = devm_kcalloc(dev, pmu->genpd_data.num_domains,
>> +                                   sizeof(struct generic_pm_domain *),
>> +                                   GFP_KERNEL);
>> +       if (!pmu->genpd)
>> +               return -ENOMEM;
>> +
>> +       pmu->genpd_data.domains = pmu->genpd;
>> +
>> +       i = 0;
>> +       for (entry = table; entry->name; entry++) {
>> +               struct starfive_power_dev *pmd = &pmu->dev[i];
>> +               bool is_on;
>> +
>> +               pmd->power = pmu;
>> +               pmd->mask = BIT(entry->bit);
>> +               pmd->genpd.name = entry->name;
>> +               pmd->genpd.flags = entry->flags;
>> +
>> +               ret = starfive_pmu_get_state(pmd, &is_on);
>> +               if (ret)
>> +                       dev_warn(dev, "unable to get current state for %s\n",
>> +                                pmd->genpd.name);
>> +
>> +               pmd->genpd.power_on = starfive_pmu_on;
>> +               pmd->genpd.power_off = starfive_pmu_off;
>> +
>> +               pm_genpd_init(&pmd->genpd, NULL, !is_on);
>> +               pmu->genpd[entry->bit] = &pmd->genpd;
>> +
>> +               i++;
>> +       }
>> +
>> +       spin_lock_init(&pmu->lock);
>> +       starfive_pmu_int_enable(pmu, PMU_INT_ALL_MASK & ~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 starfive_pmu_data jh7110_power_domains[] = {
>> +       {
>> +               .name = "SYSTOP",
>> +               .bit = JH7110_PD_SYSTOP,
>> +               .flags = GENPD_FLAG_ALWAYS_ON,
>> +       }, {
>> +               .name = "CPU",
>> +               .bit = JH7110_PD_CPU,
>> +               .flags = GENPD_FLAG_ALWAYS_ON,
>> +       }, {
>> +               .name = "GPUA",
>> +               .bit = JH7110_PD_GPUA,
>> +       }, {
>> +               .name = "VDEC",
>> +               .bit = JH7110_PD_VDEC,
>> +       }, {
>> +               .name = "VOUT",
>> +               .bit = JH7110_PD_VOUT,
>> +       }, {
>> +               .name = "ISP",
>> +               .bit = JH7110_PD_ISP,
>> +       }, {
>> +               .name = "VENC",
>> +               .bit = JH7110_PD_VENC,
>> +       }, {
>> +               .name = "GPUB",
>> +               .bit = JH7110_PD_GPUB,
>> +       }, {
>> +               /* sentinel */
>> +       },
>> +};
>> +
>> +static const struct of_device_id starfive_pmu_of_match[] = {
>> +       {
>> +               .compatible = "starfive,jh7110-pmu",
>> +               .data = &jh7110_power_domains,
>> +       }, {
>> +               /* sentinel */
>> +       }
>> +};
>> +
>> +static struct platform_driver jh71xx_pmu_driver = {
>> +       .driver = {
>> +               .name = "jh71xx-pmu",
>> +               .of_match_table = starfive_pmu_of_match,
>> +       },
>> +       .probe  = starfive_pmu_probe,
>> +};
>> +builtin_platform_driver(jh71xx_pmu_driver);
> 
> You're using a mix of starfive_, starfive_pmu_ and jh71xx_pmu_
> prefixes. Please choose one, preferably jh71xx_pmu_ since that's what
> you call the driver.

Yes, jh71xx_pmu_ is preferable, will use the unified name in the next version.

> 
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/soc/starfive/pm_domains.h b/include/soc/starfive/pm_domains.h
>> new file mode 100644
>> index 000000000000..a20e28e9baf3
>> --- /dev/null
>> +++ b/include/soc/starfive/pm_domains.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + * Author: Walker Chen <walker.chen@starfivetech.com>
>> + */
>> +
>> +#ifndef __SOC_STARFIVE_PMDOMAINS_H__
>> +#define __SOC_STARFIVE_PMDOMAINS_H__
>> +
>> +#include <linux/types.h>
>> +
>> +void starfive_pmu_hw_event_turn_on(u32 mask);
>> +void starfive_pmu_hw_event_turn_off(u32 mask);
>> +
>> +#endif /* __SOC_STARFIVE_PMDOMAINS_H__ */
> 
> The header and functions within are named very generic, but
> implemented by the jh71xx-specific driver.
> 
> Also who should use this header? These functions are never called by
> anything in this series.

These two functions will be used by the GPU module. Only the power-off of the GPU is not controlled 
by the software through PMU module. So here the functions are needed to export.

Thank you for taking the time to review this driver!

Best Regards,
Walker Chen

Re: [PATCH v1 0/4] JH7110 Power Domain Support

[Walker Chen]

On 2022/11/19 2:38, Emil Renner Berthing wrote:
> On Fri, 18 Nov 2022 at 14:34, Walker Chen <walker.chen@starfivetech.com> wrote:
>>
>> This patchset adds power domain controller driver for the StarFive JH7110 SoC.
>> The series has been tested on the VisionFive 2 board.
> 
> Hi Walker,
> 
> Thanks for upstreaming this! I've left some comments on the individual patches.

Sorry for late reply. Thank you for taking the time to review this driver!
Your advice is very helpful to me.

Best Regards,
Walker Chen

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Conor Dooley]

Hey Walker,
Just jumping in here...

On Thu, Nov 24, 2022 at 05:08:57PM +0800, Walker Chen wrote:
> On 2022/11/19 2:31, Emil Renner Berthing wrote:
> > On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote:

> >> diff --git a/include/soc/starfive/pm_domains.h b/include/soc/starfive/pm_domains.h
> >> new file mode 100644
> >> index 000000000000..a20e28e9baf3
> >> --- /dev/null
> >> +++ b/include/soc/starfive/pm_domains.h
> >> @@ -0,0 +1,15 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> >> + * Author: Walker Chen <walker.chen@starfivetech.com>
> >> + */
> >> +
> >> +#ifndef __SOC_STARFIVE_PMDOMAINS_H__
> >> +#define __SOC_STARFIVE_PMDOMAINS_H__
> >> +
> >> +#include <linux/types.h>
> >> +
> >> +void starfive_pmu_hw_event_turn_on(u32 mask);
> >> +void starfive_pmu_hw_event_turn_off(u32 mask);
> >> +
> >> +#endif /* __SOC_STARFIVE_PMDOMAINS_H__ */
> > 
> > The header and functions within are named very generic, but
> > implemented by the jh71xx-specific driver.
> > 
> > Also who should use this header? These functions are never called by
> > anything in this series.
> 
> These two functions will be used by the GPU module. Only the power-off
> of the GPU is not controlled by the software through PMU module. So
> here the functions are needed to export.

...the general policy is to avoid adding things without users. I think
they should be kept as static functions for now & when your GPU driver
is being upstreamed you can expose these functions. That way your usage
of them can be reviewed with the appropriate context.

Thanks,
Conor.

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/11/24 17:28, Conor Dooley wrote:
> Hey Walker,
> Just jumping in here...
> 
> On Thu, Nov 24, 2022 at 05:08:57PM +0800, Walker Chen wrote:
>> On 2022/11/19 2:31, Emil Renner Berthing wrote:
>> > On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote:
> 
>> >> diff --git a/include/soc/starfive/pm_domains.h b/include/soc/starfive/pm_domains.h
>> >> new file mode 100644
>> >> index 000000000000..a20e28e9baf3
>> >> --- /dev/null
>> >> +++ b/include/soc/starfive/pm_domains.h
>> >> @@ -0,0 +1,15 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0 */
>> >> +/*
>> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> >> + * Author: Walker Chen <walker.chen@starfivetech.com>
>> >> + */
>> >> +
>> >> +#ifndef __SOC_STARFIVE_PMDOMAINS_H__
>> >> +#define __SOC_STARFIVE_PMDOMAINS_H__
>> >> +
>> >> +#include <linux/types.h>
>> >> +
>> >> +void starfive_pmu_hw_event_turn_on(u32 mask);
>> >> +void starfive_pmu_hw_event_turn_off(u32 mask);
>> >> +
>> >> +#endif /* __SOC_STARFIVE_PMDOMAINS_H__ */
>> > 
>> > The header and functions within are named very generic, but
>> > implemented by the jh71xx-specific driver.
>> > 
>> > Also who should use this header? These functions are never called by
>> > anything in this series.
>> 
>> These two functions will be used by the GPU module. Only the power-off
>> of the GPU is not controlled by the software through PMU module. So
>> here the functions are needed to export.
> 
> ...the general policy is to avoid adding things without users. I think
> they should be kept as static functions for now & when your GPU driver
> is being upstreamed you can expose these functions. That way your usage
> of them can be reviewed with the appropriate context.
> 

OK, thank you for your advice. I will modify the code according to your suggestion
 in the next version of patch.

Best Regards,
Walker Chen

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/11/19 3:36, Conor Dooley wrote:
> Hey Walker,
> 
> On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:
>> Add generic power domain driver for the StarFive JH71XX SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  MAINTAINERS                       |   8 +
> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a70c1d0f303e..112f1e698723 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19623,6 +19623,14 @@ F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>>  F:	drivers/reset/starfive/
>>  F:	include/dt-bindings/reset/starfive*
>>  
>> +STARFIVE JH71XX PMU CONTROLLER DRIVER
>> +M:	Walker Chen <walker.chen@starfivetech.com>
>> +S:	Maintained
> 
> Should this be supported? (ditto the other series that you guys have
> sent out in the last few days)

Ok, will be changed to Supported.

> 
>> +F:	Documentation/devicetree/bindings/power/starfive*
>> +F:	drivers/soc/starfive/jh71xx_pmu.c
>> +F:	include/soc/starfive/pm_domains.h
>> +F:	include/dt-bindings/power/jh7110-power.h
> 
> I noticed that you have not CCed Arnd on these patches, which makes me
> wonder how do you intend getting these patches applied?
> Until now, Palmer has mostly merged RISC-V drivers/soc patches, but in
> the last few days I've taken over for drivers/soc/{sifive,microchip}.
> 
> Are you going to send PRs for this stuff to Arnd, or would you like me
> to apply patches for drivers/soc/startech? I happy to do that for you if
> you like.
> 
> If you're going to send pull requests, I am not sure if Arnd requires
> GPG signed tags for them. Arnd?
> 
> Otherwise, if you want me to take them, please add something like the
> following, in addition to the entry your series already adds for the
> specific driver:
> STARFIVE SOC DRIVERS
> M:	Conor Dooley <conor@kernel.org>
> S:	Maintained
> T:	git https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/
> F:	drivers/soc/starfive/
> F:	include/soc/starfive/

Thank you for taking the time to review and comment. I hope you apply these patches 
for drivers/soc/starfive. If you do that, I am very grateful to you!
The above description will be added to MAINTAINERS in the next version of patch.

Best Regards,
Walker Chen

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/11/19 8:24, Conor Dooley wrote:
> Hey Walker,
> 
> On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:
>> Add generic power domain driver for the StarFive JH71XX SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>> diff --git a/drivers/soc/starfive/Kconfig b/drivers/soc/starfive/Kconfig
>> new file mode 100644
>> index 000000000000..2bbcc1397b15
>> --- /dev/null
>> +++ b/drivers/soc/starfive/Kconfig
>> @@ -0,0 +1,9 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +config JH71XX_PMU
>> +	bool "Support PMU for StarFive JH71XX Soc"
>> +	depends on PM && (SOC_STARFIVE || COMPILE_TEST)
> 
> `default SOC_STARFIVE` perhaps?

OK, 'default SOC_STARFIVE' will be added to the next line of 'depends on PM && (SOC_STARFIVE || COMPILE_TEST)'

> 
>> +	select PM_GENERIC_DOMAINS
>> +	help
>> +	  Say y here to enable power domain support.
>> +
>> diff --git a/drivers/soc/starfive/Makefile b/drivers/soc/starfive/Makefile
>> new file mode 100644
>> index 000000000000..13b589d6b5f3
>> --- /dev/null
>> +++ b/drivers/soc/starfive/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +obj-$(CONFIG_JH71XX_PMU)	+= jh71xx_pmu.o
>> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
>> new file mode 100644
>> index 000000000000..e6c0083d166e
>> --- /dev/null
>> +++ b/drivers/soc/starfive/jh71xx_pmu.c
>> @@ -0,0 +1,380 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * StarFive JH71XX Power Domain 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/jh7110-power.h>
>>
>> +/* register offset */
>> +#define HW_EVENT_TURN_ON_MASK		0x04
>> +#define HW_EVENT_TURN_OFF_MASK		0x08
>> +#define SW_TURN_ON_POWER_MODE		0x0C
>> +#define SW_TURN_OFF_POWER_MODE		0x10
>> +#define SW_ENCOURAGE			0x44
>> +#define PMU_INT_MASK			0x48
>> +#define PCH_BYPASS			0x4C
>> +#define PCH_PSTATE			0x50
>> +#define PCH_TIMEOUT			0x54
>> +#define LP_TIMEOUT			0x58
>> +#define HW_TURN_ON_MODE			0x5C
>> +#define CURR_POWER_MODE			0x80
>> +#define PMU_EVENT_STATUS		0x88
>> +#define PMU_INT_STATUS			0x8C
>> +
>> +/* sw encourage cfg */
>> +#define SW_MODE_ENCOURAGE_EN_LO		0x05
>> +#define SW_MODE_ENCOURAGE_EN_HI		0x50
>> +#define SW_MODE_ENCOURAGE_DIS_LO	0x0A
>> +#define SW_MODE_ENCOURAGE_DIS_HI	0xA0
>> +#define SW_MODE_ENCOURAGE_ON		0xFF
>> +
>> +/* pmu int status */
>> +#define PMU_INT_SEQ_DONE		BIT(0)
>> +#define PMU_INT_HW_REQ			BIT(1)
>> +#define PMU_INT_SW_FAIL			GENMASK(3, 2)
>> +#define PMU_INT_HW_FAIL			GENMASK(5, 4)
>> +#define PMU_INT_PCH_FAIL		GENMASK(8, 6)
>> +#define PMU_INT_FAIL_MASK		(PMU_INT_SW_FAIL | \
>> +					PMU_INT_HW_FAIL | \
>> +					PMU_INT_PCH_FAIL)
>> +#define PMU_INT_ALL_MASK		(PMU_INT_SEQ_DONE | \
>> +					PMU_INT_HW_REQ | \
>> +					PMU_INT_FAIL_MASK)
>> +
>> +#define DELAY_US			10
>> +#define TIMEOUT_US			100000
> 
> Some JH71XX_PMU_ prefixes for the above, as I think Emil pointed out,
> would be great.

I will modify the name of macro according to your suggestions.

> 
>> +struct starfive_power_dev {
>> +	struct generic_pm_domain genpd;
>> +	struct starfive_pmu *power;
>> +	uint32_t mask;
>> +};
>> +
>> +struct starfive_pmu {
>> +	void __iomem *base;
>> +	spinlock_t lock;
>> +	int irq;
>> +	struct device *pdev;
>> +	struct starfive_power_dev *dev;
>> +	struct genpd_onecell_data genpd_data;
>> +	struct generic_pm_domain **genpd;
>> +};
>> +
>> +struct starfive_pmu_data {
>> +	const char * const name;
>> +	uint8_t bit;
>> +	unsigned int flags;
> 
> Generally, ordering pointers first in these structs would be nice.

These structs are referenced from drivers/soc/bcm/bcm63xx/bcm63xx-power.c

> 
>> +};
>> +
>> +static void __iomem *pmu_base;
>> +
>> +static inline void pmu_writel(u32 val, u32 offset)
>> +{
>> +	writel(val, pmu_base + offset);
>> +}
>> +
>> +void starfive_pmu_hw_event_turn_on(u32 mask)
>> +{
>> +	pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
>> +}
>> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
>> +
>> +void starfive_pmu_hw_event_turn_off(u32 mask)
>> +{
>> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
>> +}
>> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
> 
> Where are the users for these exports? Also, should they be exported as
> GPL?
> 
> Either way, what is the point of the extra layer of abstraction here
> around the writel()?

The two export functions are only prepared for GPU module. But accordint to
 the latest information, it seems that there is no open source plan for GPU. 
So the two functions will be drop in next version of patch.

> 
>> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
>> +{
>> +	struct starfive_pmu *pmu = pmd->power;
>> +
>> +	if (!pmd->mask) {
>> +		*is_on = false;
>> +		return -EINVAL;
>> +	}
>> +
>> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
> 
> Is there a specific reason that you are using the __raw variants here
> (and elsewhere) in the driver?

Will use unified function '__raw_readl' and '__raw_writel'

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
>> +{
>> +	struct starfive_pmu *pmu = pmd->power;
>> +	unsigned long flags;
>> +	uint32_t val;
>> +	uint32_t mode;
>> +	uint32_t encourage_lo;
>> +	uint32_t encourage_hi;
>> +	bool is_on;
>> +	int ret;
>> +
>> +	if (!pmd->mask)
>> +		return -EINVAL;
>> +
>> +	if (is_on == on) {
>> +		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
>> +				pmd->genpd.name, on ? "en" : "dis");
> 
> Am I missing something here: you've just declared is_on, so it must be
> false & therefore this check is all a little pointless? The check just
> becomes if (false == on) which I don't think is what you're going for
> here? Or did I miss something obvious?

Sorry, indeed I missed several lines of code. It should be witten like this:
	ret = jh71xx_pmu_get_state(pmd, &is_on);
        if (ret) {
                dev_dbg(pmu->pdev, "unable to get current state for %s\n",
                        pmd->genpd.name);
                return ret;
        }

        if (is_on == on) {
                dev_dbg(pmu->pdev, "pm domain [%s] is already %sable status.\n",
                        pmd->genpd.name, on ? "en" : "dis");
                return 0;
        }

> 
>> +		return 0;
>> +	}
>> +
>> +	spin_lock_irqsave(&pmu->lock, flags);
>> +
>> +	if (on) {
>> +		mode = SW_TURN_ON_POWER_MODE;
>> +		encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
>> +		encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
>> +	} else {
>> +		mode = SW_TURN_OFF_POWER_MODE;
>> +		encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
>> +		encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
>> +	}
>> +
>> +	__raw_writel(pmd->mask, pmu->base + mode);
>> +
>> +	/* write SW_ENCOURAGE to make the configuration take effect */
>> +	__raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
> 
> Is register "sticky"? IOW, could you set it in probe and leave this mode
> always on? Or does it need to be set every time you want to use this
> feature?

These power domain registers need to be set by other module according to the specific situation. 
For example some power domains should be turned off via System PM mechanism when system goes to sleep, 
and then they are turned on when system resume.

> 
>> +	__raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
>> +	__raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
>> +
>> +	spin_unlock_irqrestore(&pmu->lock, flags);
>> +
>> +	if (on) {
>> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
>> +						val & pmd->mask, DELAY_US,
>> +						TIMEOUT_US);
>> +		if (ret) {
>> +			dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
>> +			return -ETIMEDOUT;
>> +		}
>> +	} else {
>> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
>> +						!(val & pmd->mask), DELAY_US,
>> +						TIMEOUT_US);
> 
> Could you not just decide the 3rd arg outside of the readl_poll..() and
> save on duplicating the wait logic here?

Seems that the readl_poll..() can only be called in two cases 
because the CURR_POWER_MODE register is read to val and then operation with mask every time.

> 
>> +		if (ret) {
>> +			dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
>> +			return -ETIMEDOUT;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int starfive_pmu_on(struct generic_pm_domain *genpd)
>> +{
>> +	struct starfive_power_dev *pmd = container_of(genpd,
>> +		struct starfive_power_dev, genpd);
>> +
>> +	return starfive_pmu_set_state(pmd, true);
>> +}
>> +
>> +static int starfive_pmu_off(struct generic_pm_domain *genpd)
>> +{
>> +	struct starfive_power_dev *pmd = container_of(genpd,
>> +		struct starfive_power_dev, genpd);
>> +
>> +	return starfive_pmu_set_state(pmd, false);
>> +}
>> +
>> +static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
>> +{
>> +	u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
>> +
>> +	if (enable)
>> +		val &= ~mask;
>> +	else
>> +		val |= mask;
>> +
>> +	__raw_writel(val, pmu->base + PMU_INT_MASK);
>> +}
>> +
>> +static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
>> +{
>> +	struct starfive_pmu *pmu = data;
>> +	unsigned long flags;
>> +	u32 val;
>> +
>> +	spin_lock_irqsave(&pmu->lock, flags);
>> +	val = __raw_readl(pmu->base + PMU_INT_STATUS);
>> +
>> +	if (val & PMU_INT_SEQ_DONE)
>> +		dev_dbg(pmu->pdev, "sequence done.\n");
>> +	if (val & PMU_INT_HW_REQ)
>> +		dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
>> +	if (val & PMU_INT_SW_FAIL)
>> +		dev_err(pmu->pdev, "software encourage fail.\n");
>> +	if (val & PMU_INT_HW_FAIL)
>> +		dev_err(pmu->pdev, "hardware encourage fail.\n");
>> +	if (val & PMU_INT_PCH_FAIL)
>> +		dev_err(pmu->pdev, "p-channel fail event.\n");
>> +
>> +	/* clear interrupts */
>> +	__raw_writel(val, pmu->base + PMU_INT_STATUS);
>> +	__raw_writel(val, pmu->base + PMU_EVENT_STATUS);
>> +
>> +	spin_unlock_irqrestore(&pmu->lock, flags);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int starfive_pmu_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	const struct starfive_pmu_data *entry, *table;
>> +	struct starfive_pmu *pmu;
>> +	unsigned int i;
>> +	uint8_t max_bit = 0;
>> +	int ret;
>> +
>> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
>> +	if (!pmu)
>> +		return -ENOMEM;
>> +
>> +	pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(pmu->base))
>> +		return PTR_ERR(pmu->base);
>> +
>> +	/* initialize pmu interrupt  */
>> +	pmu->irq = platform_get_irq(pdev, 0);
>> +	if (pmu->irq < 0)
>> +		return pmu->irq;
>> +
>> +	ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
>> +			       0, pdev->name, pmu);
>> +	if (ret)
>> +		dev_err(dev, "request irq failed.\n");
>> +
>> +	table = of_device_get_match_data(dev);
>> +	if (!table)
>> +		return -EINVAL;
>> +
>> +	pmu->pdev = dev;
>> +	pmu->genpd_data.num_domains = 0;
>> +	i = 0;
>> +	for (entry = table; entry->name; entry++) {
>> +		max_bit = max(max_bit, entry->bit);
>> +		i++;
>> +	}
> 
> This looks like something that could be replaced by the functions in
> linux/list.h, no? Same below.

Nowadays other platforms on linux mainline mostly write in this way or write like this: 
	for (i = 0; i < num; i++) {
		...
		pm_genpd_init(&pmd[i]->genpd, NULL, !is_on);
	}

> 
>> +
>> +	if (!i)
>> +		return -ENODEV;
>> +
>> +	pmu->genpd_data.num_domains = max_bit + 1;
>> +
>> +	pmu->dev = devm_kcalloc(dev, pmu->genpd_data.num_domains,
>> +				  sizeof(struct starfive_power_dev),
>> +				  GFP_KERNEL);
>> +	if (!pmu->dev)
>> +		return -ENOMEM;
>> +
>> +	pmu->genpd = devm_kcalloc(dev, pmu->genpd_data.num_domains,
>> +				    sizeof(struct generic_pm_domain *),
>> +				    GFP_KERNEL);
> 
> This two allocations both look like checkpatch would whinge about their
> alignment.

Yes, I will adjust these alignment. Thank you for your remind.

> 
>> +	if (!pmu->genpd)
>> +		return -ENOMEM;
>> +
>> +	pmu->genpd_data.domains = pmu->genpd;
>> +
>> +	i = 0;
>> +	for (entry = table; entry->name; entry++) {
>> +		struct starfive_power_dev *pmd = &pmu->dev[i];
>> +		bool is_on;
>> +
>> +		pmd->power = pmu;
>> +		pmd->mask = BIT(entry->bit);
>> +		pmd->genpd.name = entry->name;
>> +		pmd->genpd.flags = entry->flags;
>> +
>> +		ret = starfive_pmu_get_state(pmd, &is_on);
>> +		if (ret)
>> +			dev_warn(dev, "unable to get current state for %s\n",
>> +				 pmd->genpd.name);
> 
> In what scenario would it not be possible to get the state? Again, I
> hope I'm not missing something obvious. From what I can see, this is the
> only caller of starfive_pmu_get_state(), which looks like:
>> > +	struct starfive_pmu *pmu = pmd->power;
>> > +
>> > +	if (!pmd->mask) {

> How can !pmd->mask evaluate to true, given it's just been set to BIT(n)?
> 
>> > +		*is_on = false;
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
>> > +
>> > +	return 0;
> 
> If I've not missed something, looks like you could likely remove the
> function and just do the readl() here?

I will modify the code like this:
static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, bool on)
{
	int ret;
	...
	
	ret = jh71xx_pmu_get_state(pmd, &is_on);
	if (ret) {
		dev_dbg(pmu->pdev, "unable to get current state for %s\n",
			pmd->genpd.name);
		return ret;
	}

	if (is_on == on) {
		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
			 pmd->genpd.name, on ? "en" : "dis");
		return 0;
	}
	...
}

This place where jh71xx_pmu_get_state() is called will return and then can not set the state 
for power domain if pmd->mask is missed to assign a value.

> 
>> +
>> +		pmd->genpd.power_on = starfive_pmu_on;
>> +		pmd->genpd.power_off = starfive_pmu_off;
>> +
>> +		pm_genpd_init(&pmd->genpd, NULL, !is_on);
>> +		pmu->genpd[entry->bit] = &pmd->genpd;
>> +
>> +		i++;
>> +	}
>> +
>> +	spin_lock_init(&pmu->lock);
>> +	starfive_pmu_int_enable(pmu, PMU_INT_ALL_MASK & ~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 starfive_pmu_data jh7110_power_domains[] = {
> 
> Is this driver jh7110 only or actually jh71XX? Have you just started out
> by implementing one SoC both intend to support both?

JH7110 is our first SoC, probably the next generation of SoC jh7120 still use this controller driver.
Maybe now the naming for JH71XX is not very suitable.

> 
>> +	{
>> +		.name = "SYSTOP",
>> +		.bit = JH7110_PD_SYSTOP,
>> +		.flags = GENPD_FLAG_ALWAYS_ON,
>> +	}, {
>> +		.name = "CPU",
>> +		.bit = JH7110_PD_CPU,
>> +		.flags = GENPD_FLAG_ALWAYS_ON,
>> +	}, {
>> +		.name = "GPUA",
>> +		.bit = JH7110_PD_GPUA,
>> +	}, {
>> +		.name = "VDEC",
>> +		.bit = JH7110_PD_VDEC,
>> +	}, {
>> +		.name = "VOUT",
>> +		.bit = JH7110_PD_VOUT,
>> +	}, {
>> +		.name = "ISP",
>> +		.bit = JH7110_PD_ISP,
>> +	}, {
>> +		.name = "VENC",
>> +		.bit = JH7110_PD_VENC,
>> +	}, {
>> +		.name = "GPUB",
>> +		.bit = JH7110_PD_GPUB,
>> +	}, {
>> +		/* sentinel */
>> +	},
> 
> Can drop this , after the sentinel ;)
> 
> I don't know /jack/ about power domain stuff, so I can barely review
> this at all sadly.
> Thanks,
> Conor.

Thank you for taking the time to review the code, you helped me a lot. Thank you so much.

Best Regards,
Walker Chen

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Conor Dooley]

On Fri, Nov 25, 2022 at 06:04:59PM +0800, Walker Chen wrote:
> On 2022/11/19 8:24, Conor Dooley wrote:
> > On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:

> >> +void starfive_pmu_hw_event_turn_off(u32 mask)
> >> +{
> >> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
> >> +}
> >> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
> > 
> > Where are the users for these exports? Also, should they be exported as
> > GPL?
> > 
> > Either way, what is the point of the extra layer of abstraction here
> > around the writel()?
> 
> The two export functions are only prepared for GPU module. But accordint to
>  the latest information, it seems that there is no open source plan for GPU. 
> So the two functions will be drop in next version of patch.

That's a shame!

> >> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
> >> +{
> >> +	struct starfive_pmu *pmu = pmd->power;
> >> +
> >> +	if (!pmd->mask) {
> >> +		*is_on = false;
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
> > 
> > Is there a specific reason that you are using the __raw variants here
> > (and elsewhere) in the driver?
> 
> Will use unified function '__raw_readl' and '__raw_writel'

No no, I want to know *why* you are using the __raw accessors here. My
understanding was that __raw variants are unbarriered & unordered with
respect to other io accesses.

I do notice that the bcm driver you mentioned uses the __raw variants,
but only __raw variants - whereas you use readl() which is ordered and
barriered & __raw_readl().

Is there a reason why you would not use readl() or readl_relaxed()?

> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
> >> +{
> >> +	struct starfive_pmu *pmu = pmd->power;
> >> +	unsigned long flags;
> >> +	uint32_t val;
> >> +	uint32_t mode;
> >> +	uint32_t encourage_lo;
> >> +	uint32_t encourage_hi;
> >> +	bool is_on;
> >> +	int ret;
> >> +
> >> +	if (!pmd->mask)
> >> +		return -EINVAL;
> >> +
> >> +	if (is_on == on) {
> >> +		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
> >> +				pmd->genpd.name, on ? "en" : "dis");
> > 
> > Am I missing something here: you've just declared is_on, so it must be
> > false & therefore this check is all a little pointless? The check just
> > becomes if (false == on) which I don't think is what you're going for
> > here? Or did I miss something obvious?
> 
> Sorry, indeed I missed several lines of code. It should be witten like this:
> 	ret = jh71xx_pmu_get_state(pmd, &is_on);
>         if (ret) {
>                 dev_dbg(pmu->pdev, "unable to get current state for %s\n",
>                         pmd->genpd.name);
>                 return ret;
>         }

Heh, this looks more sane :)

> 
>         if (is_on == on) {
>                 dev_dbg(pmu->pdev, "pm domain [%s] is already %sable status.\n",
>                         pmd->genpd.name, on ? "en" : "dis");
>                 return 0;
>         }
> 
> > 
> >> +		return 0;
> >> +	}
> >> +
> >> +	spin_lock_irqsave(&pmu->lock, flags);
> >> +
> >> +	if (on) {
> >> +		mode = SW_TURN_ON_POWER_MODE;
> >> +		encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
> >> +		encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
> >> +	} else {
> >> +		mode = SW_TURN_OFF_POWER_MODE;
> >> +		encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
> >> +		encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
> >> +	}
> >> +
> >> +	__raw_writel(pmd->mask, pmu->base + mode);
> >> +
> >> +	/* write SW_ENCOURAGE to make the configuration take effect */
> >> +	__raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
> > 
> > Is register "sticky"? IOW, could you set it in probe and leave this mode
> > always on? Or does it need to be set every time you want to use this
> > feature?
> 
> These power domain registers need to be set by other module according to the
> specific situation. 
> For example some power domains should be turned off via System PM mechanism
> when system goes to sleep, 
> and then they are turned on when system resume.

I was just wondering if SW_MODE_ENCOURAGE_ON would retain it's value
during operation or if it had to be written every time. It's fine if
that's not the case.

> >> +	__raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
> >> +	__raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
> >> +
> >> +	spin_unlock_irqrestore(&pmu->lock, flags);
> >> +
> >> +	if (on) {
> >> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> >> +						val & pmd->mask, DELAY_US,
> >> +						TIMEOUT_US);
> >> +		if (ret) {
> >> +			dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
> >> +			return -ETIMEDOUT;
> >> +		}
> >> +	} else {
> >> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> >> +						!(val & pmd->mask), DELAY_US,
> >> +						TIMEOUT_US);
> > 
> > Could you not just decide the 3rd arg outside of the readl_poll..() and
> > save on duplicating the wait logic here?
> 
> Seems that the readl_poll..() can only be called in two cases 
> because the CURR_POWER_MODE register is read to val and then operation with mask every time.
> 

I'm sorry, I completely forgot that read*_poll..() actually not actually
a function. Please ignore this comment!

> >> +static int starfive_pmu_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +	struct device_node *np = dev->of_node;
> >> +	const struct starfive_pmu_data *entry, *table;
> >> +	struct starfive_pmu *pmu;
> >> +	unsigned int i;
> >> +	uint8_t max_bit = 0;
> >> +	int ret;
> >> +
> >> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> >> +	if (!pmu)
> >> +		return -ENOMEM;
> >> +
> >> +	pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
> >> +	if (IS_ERR(pmu->base))
> >> +		return PTR_ERR(pmu->base);
> >> +
> >> +	/* initialize pmu interrupt  */
> >> +	pmu->irq = platform_get_irq(pdev, 0);
> >> +	if (pmu->irq < 0)
> >> +		return pmu->irq;
> >> +
> >> +	ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
> >> +			       0, pdev->name, pmu);
> >> +	if (ret)
> >> +		dev_err(dev, "request irq failed.\n");
> >> +
> >> +	table = of_device_get_match_data(dev);
> >> +	if (!table)
> >> +		return -EINVAL;
> >> +
> >> +	pmu->pdev = dev;
> >> +	pmu->genpd_data.num_domains = 0;
> >> +	i = 0;
> >> +	for (entry = table; entry->name; entry++) {
> >> +		max_bit = max(max_bit, entry->bit);
> >> +		i++;
> >> +	}
> > 
> > This looks like something that could be replaced by the functions in
> > linux/list.h, no? Same below.
> 
> Nowadays other platforms on linux mainline mostly write in this way or write like this: 
> 	for (i = 0; i < num; i++) {
> 		...
> 		pm_genpd_init(&pmd[i]->genpd, NULL, !is_on);
> 	}

That's not what this specific bit of code is doing though, right? You're
walking jh7110_power_domains to find the highest bit. I was looking at what
some other drivers do, and took a look at drivers/soc/qcom/rpmhpd.c
where they know the size of this struct at compile time & so can do
store the number of power domains in the match data. If you did that,
you could use a loop like the one other platforms use.

> >> +	if (!pmu->genpd)
> >> +		return -ENOMEM;
> >> +
> >> +	pmu->genpd_data.domains = pmu->genpd;
> >> +
> >> +	i = 0;
> >> +	for (entry = table; entry->name; entry++) {

And it's the same here. By now, you should know how many power domains
you have, no?

Anyways, as I said before I don't know much about this power domain
stuff, it's just that these two loops seem odd.

> >> +		struct starfive_power_dev *pmd = &pmu->dev[i];
> >> +		bool is_on;
> >> +
> >> +		pmd->power = pmu;
> >> +		pmd->mask = BIT(entry->bit);
> >> +		pmd->genpd.name = entry->name;
> >> +		pmd->genpd.flags = entry->flags;
> >> +
> >> +		ret = starfive_pmu_get_state(pmd, &is_on);
> >> +		if (ret)
> >> +			dev_warn(dev, "unable to get current state for %s\n",
> >> +				 pmd->genpd.name);

> >> +static const struct starfive_pmu_data jh7110_power_domains[] = {
> > 
> > Is this driver jh7110 only or actually jh71XX? Have you just started out
> > by implementing one SoC both intend to support both?
> 
> JH7110 is our first SoC, probably the next generation of SoC jh7120 still
> use this controller driver.
> Maybe now the naming for JH71XX is not very suitable.

Right. My question was more about if this supported the JH7100 too, but
I saw from your answer to Emil that it won't. I don't have a preference
as to whether you call it jh71xx or jh7110, I'll leave that up to
yourselves and Emil. This particular struct should still be called
`jh7110_power_domains` though since it is particular to this SoC, no
matter what you end up calling the file etc.

> > I don't know /jack/ about power domain stuff, so I can barely review
> > this at all sadly.

> Thank you for taking the time to review the code, you helped me a lot.
> Thank you so much.

No worries, looking forward to getting my board :)

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/11/21 18:14, Krzysztof Kozlowski wrote:
> On 18/11/2022 14:32, Walker Chen wrote:
>> Add generic power domain driver for the StarFive JH71XX SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +
>> +MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>");
>> +MODULE_DESCRIPTION("StarFive JH71XX Power Domain Driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/soc/starfive/pm_domains.h b/include/soc/starfive/pm_domains.h
>> new file mode 100644
>> index 000000000000..a20e28e9baf3
>> --- /dev/null
>> +++ b/include/soc/starfive/pm_domains.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + * Author: Walker Chen <walker.chen@starfivetech.com>
>> + */
>> +
>> +#ifndef __SOC_STARFIVE_PMDOMAINS_H__
>> +#define __SOC_STARFIVE_PMDOMAINS_H__
>> +
>> +#include <linux/types.h>
>> +
>> +void starfive_pmu_hw_event_turn_on(u32 mask);
>> +void starfive_pmu_hw_event_turn_off(u32 mask);
> 
> These are not used outside of driver. Drop entire header file.
> 

Yes, these two exported functions will be drop in the next version of patches.

Best Regards,
Walker Chen

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/11/22 8:09, Kevin Hilman wrote:
> Hi Walker,
> 
> Walker Chen <walker.chen@starfivetech.com> writes:
> 
>> Add generic power domain driver for the StarFive JH71XX SoC.
>>
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> 
> A bit more detail about the features power domain hardware would be
> helpful for reviewers.  I'm assuming there's no public documenation for
> this, but if there is, a link to that would be great also.
> 
> [...]
> 

OK, more detailed description will be added to commit message in the next version of patch.

>> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
>> new file mode 100644
>> index 000000000000..e6c0083d166e
>> --- /dev/null
>> +++ b/drivers/soc/starfive/jh71xx_pmu.c
>> @@ -0,0 +1,380 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * StarFive JH71XX Power Domain 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/jh7110-power.h>
>> +
>> +/* register offset */
>> +#define HW_EVENT_TURN_ON_MASK		0x04
>> +#define HW_EVENT_TURN_OFF_MASK		0x08
>> +#define SW_TURN_ON_POWER_MODE		0x0C
>> +#define SW_TURN_OFF_POWER_MODE		0x10
>> +#define SW_ENCOURAGE			0x44
>> +#define PMU_INT_MASK			0x48
>> +#define PCH_BYPASS			0x4C
>> +#define PCH_PSTATE			0x50
>> +#define PCH_TIMEOUT			0x54
>> +#define LP_TIMEOUT			0x58
>> +#define HW_TURN_ON_MODE			0x5C
>> +#define CURR_POWER_MODE			0x80
>> +#define PMU_EVENT_STATUS		0x88
>> +#define PMU_INT_STATUS			0x8C
>> +
>> +/* sw encourage cfg */
>> +#define SW_MODE_ENCOURAGE_EN_LO		0x05
>> +#define SW_MODE_ENCOURAGE_EN_HI		0x50
>> +#define SW_MODE_ENCOURAGE_DIS_LO	0x0A
>> +#define SW_MODE_ENCOURAGE_DIS_HI	0xA0
>> +#define SW_MODE_ENCOURAGE_ON		0xFF
>> +
>> +/* pmu int status */
>> +#define PMU_INT_SEQ_DONE		BIT(0)
>> +#define PMU_INT_HW_REQ			BIT(1)
>> +#define PMU_INT_SW_FAIL			GENMASK(3, 2)
>> +#define PMU_INT_HW_FAIL			GENMASK(5, 4)
>> +#define PMU_INT_PCH_FAIL		GENMASK(8, 6)
>> +#define PMU_INT_FAIL_MASK		(PMU_INT_SW_FAIL | \
>> +					PMU_INT_HW_FAIL | \
>> +					PMU_INT_PCH_FAIL)
>> +#define PMU_INT_ALL_MASK		(PMU_INT_SEQ_DONE | \
>> +					PMU_INT_HW_REQ | \
>> +					PMU_INT_FAIL_MASK)
>> +
>> +#define DELAY_US			10
>> +#define TIMEOUT_US			100000
> 
> Where do these delay/timeout numbers come from?  Is this just based one
> experimentation, or is this something described by the HW docs.  Please
> add some comments.
>

Now the delay time is not accurate enough though the things can work normally.
It will be changed to more accurate value according to hardware feature.
 
>> +
>> +struct starfive_power_dev {
>> +	struct generic_pm_domain genpd;
>> +	struct starfive_pmu *power;
>> +	uint32_t mask;
>> +};
>> +
>> +struct starfive_pmu {
>> +	void __iomem *base;
>> +	spinlock_t lock;
>> +	int irq;
>> +	struct device *pdev;
>> +	struct starfive_power_dev *dev;
>> +	struct genpd_onecell_data genpd_data;
>> +	struct generic_pm_domain **genpd;
>> +};
>> +
>> +struct starfive_pmu_data {
>> +	const char * const name;
>> +	uint8_t bit;
>> +	unsigned int flags;
>> +};
>> +
>> +static void __iomem *pmu_base;
>> +
>> +static inline void pmu_writel(u32 val, u32 offset)
>> +{
>> +	writel(val, pmu_base + offset);
>> +}
> 
> You use writel() in this helper, but __raw_writel() throughout the rest
> of the driver.  If that's intentional, you should explain a bit more
> about why.  If not, please make it consistent.
> 
> If you're going to use a wrapper/helper, you should use it throughout.
> But in this case, I'm not sure it's adding anything to readability.  In
> fact, it's only adding confusion since you don't use it for most of the
> register accesses.

Your suggestion will be taken seriously consider. Thanks.

> 
>> +void starfive_pmu_hw_event_turn_on(u32 mask)
>> +{
>> +	pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
>> +}
>> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
>> +
>> +void starfive_pmu_hw_event_turn_off(u32 mask)
>> +{
>> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
>> +}
>> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
> 
> This looks wrong.  Why are you allowing external users to power on/off the
> domains?  The sate of the domain should be managed by the PM core (and
> runtime PM use-counts etc.) allowing external users to change the domain
> state is going to lead to the PM core being confused about the state of
> the domain.
> 

These two functions are used by the GPU module. Only the power-off of the GPU is not controlled 
by the software through PMU module. So here the functions are needed to export. But according to
 the latest information, it seems that there is no open source plan for GPU. 
So the two functions will be drop in next version of patch.

>> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
>> +{
>> +	struct starfive_pmu *pmu = pmd->power;
>> +
>> +	if (!pmd->mask) {
>> +		*is_on = false;
>> +		return -EINVAL;
>> +	}
>> +
>> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
>> +
>> +	return 0;
>> +}
>> +
>> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
>> +{
>> +	struct starfive_pmu *pmu = pmd->power;
>> +	unsigned long flags;
>> +	uint32_t val;
>> +	uint32_t mode;
>> +	uint32_t encourage_lo;
>> +	uint32_t encourage_hi;
>> +	bool is_on;
>> +	int ret;
>> +
>> +	if (!pmd->mask)
>> +		return -EINVAL;
>> +
>> +	if (is_on == on) {
> 
> Comparing an uninitialzed stack variable to 'on'?
> 

Sorry, indeed I missed several lines of code. It should be written like this:
	ret = jh71xx_pmu_get_state(pmd, &is_on);
        if (ret) {
                dev_dbg(pmu->pdev, "unable to get current state for %s\n",
                        pmd->genpd.name);
                return ret;
        }

        if (is_on == on) {
                dev_dbg(pmu->pdev, "pm domain [%s] is already %sable status.\n",
                        pmd->genpd.name, on ? "en" : "dis");
                return 0;
        }

>> +		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
>> +				pmd->genpd.name, on ? "en" : "dis");
> 
> Using dev_info() will probably make this a bit verbose.  I'd suggest
> dev_dbg() for this kind of message.
> 

Will fix.

>> +		return 0;
>> +	}
>> +
>> +	spin_lock_irqsave(&pmu->lock, flags);
>> +
>> +	if (on) {
>> +		mode = SW_TURN_ON_POWER_MODE;
>> +		encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
>> +		encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
>> +	} else {
>> +		mode = SW_TURN_OFF_POWER_MODE;
>> +		encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
>> +		encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
>> +	}
>> +
>> +	__raw_writel(pmd->mask, pmu->base + mode);
>> +
>> +	/* write SW_ENCOURAGE to make the configuration take effect */
>> +	__raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
>> +	__raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
>> +	__raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
> 
> This "encourage" feature is peculiar.  What happens if you do not do
> this?  Does it take a lot longer for the domain to change state? or does
> it not change at all?  In the absence of HW specs, a bit of description
> here would be helpful.

There is need specific command sequence for controlling one power domain on JH7110 SoC. 
SW turn-on command sequence: Write the register Software encourage (offset 0x44) 0xff –> 0x05 –> 0x50
SW turn-off command sequence: Write the register Software encourage (offset 0x44) 0xff –> 0x0a –> 0xa0
More detailed comment will be added in next version.

> 
>> +	spin_unlock_irqrestore(&pmu->lock, flags);
>> +
>> +	if (on) {
>> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
>> +						val & pmd->mask, DELAY_US,
>> +						TIMEOUT_US);
>> +		if (ret) {
>> +			dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
>> +			return -ETIMEDOUT;
>> +		}
>> +	} else {
>> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
>> +						!(val & pmd->mask), DELAY_US,
>> +						TIMEOUT_US);
>> +		if (ret) {
>> +			dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
>> +			return -ETIMEDOUT;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int starfive_pmu_on(struct generic_pm_domain *genpd)
>> +{
>> +	struct starfive_power_dev *pmd = container_of(genpd,
>> +		struct starfive_power_dev, genpd);
>> +
>> +	return starfive_pmu_set_state(pmd, true);
>> +}
>> +
>> +static int starfive_pmu_off(struct generic_pm_domain *genpd)
>> +{
>> +	struct starfive_power_dev *pmd = container_of(genpd,
>> +		struct starfive_power_dev, genpd);
>> +
>> +	return starfive_pmu_set_state(pmd, false);
>> +}
>> +
>> +static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
>> +{
>> +	u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
>> +
>> +	if (enable)
>> +		val &= ~mask;
>> +	else
>> +		val |= mask;
>> +
>> +	__raw_writel(val, pmu->base + PMU_INT_MASK);
>> +}
>> +
>> +static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
>> +{
>> +	struct starfive_pmu *pmu = data;
>> +	unsigned long flags;
>> +	u32 val;
>> +
>> +	spin_lock_irqsave(&pmu->lock, flags);
> 
> I don't think you need to spinlock in the interrupt itself, as
> interrupts will already be disabled, and this handler is not
> IRQF_SHARED.

Helpful suggestion, it will be fixed. thanks.

> 
>> +	val = __raw_readl(pmu->base + PMU_INT_STATUS);
>> +
>> +	if (val & PMU_INT_SEQ_DONE)
>> +		dev_dbg(pmu->pdev, "sequence done.\n");
>> +	if (val & PMU_INT_HW_REQ)
>> +		dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
>> +	if (val & PMU_INT_SW_FAIL)
>> +		dev_err(pmu->pdev, "software encourage fail.\n");
>> +	if (val & PMU_INT_HW_FAIL)
>> +		dev_err(pmu->pdev, "hardware encourage fail.\n");
>> +	if (val & PMU_INT_PCH_FAIL)
>> +		dev_err(pmu->pdev, "p-channel fail event.\n");
>> +
>> +	/* clear interrupts */
>> +	__raw_writel(val, pmu->base + PMU_INT_STATUS);
>> +	__raw_writel(val, pmu->base + PMU_EVENT_STATUS);
>> +
>> +	spin_unlock_irqrestore(&pmu->lock, flags);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
> 
> [...]
> 
> Kevin

Re: [PATCH v1 4/4] riscv: dts: starfive: add power controller node

[Emil Renner Berthing]

On Wed, 23 Nov 2022 at 03:12, Walker Chen <walker.chen@starfivetech.com> wrote:
>
> On 2022/11/19 2:36, Emil Renner Berthing wrote:
> > On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote:
> >>
> >> This adds the power controller node for the Starfive JH7110 SoC.
> >> The pmu needs to be used by other modules such as ISP, VPU, etc.
> >>
> >> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> >
> > Hi Walker,
> >
> > You called the driver jh71xx which suggests it also applies to the
> > jh7100. Are you missing a node in the jh7100 device tree?
>
> No, there is no power domain controller on the jh7100. Our next generation of chips jh7120 will
> still use this power management unit, so here this driver name is called jh71xx_pmu.c or changed
> to jh71xx_power.c , do you think such a name is appropriate ?
> Your reply will be highly appreciated!

I see. In that case jh71xx seems appropriate, thanks.

> >
> >> ---
> >>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> >> index c22e8f1d2640..fa7b60b82d71 100644
> >> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> >> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> >> @@ -356,6 +356,13 @@
> >>                         #gpio-cells = <2>;
> >>                 };
> >>
> >> +               pwrc: power-controller@17030000 {
> >> +                       compatible = "starfive,jh7110-pmu";
> >> +                       reg = <0x0 0x17030000 0x0 0x10000>;
> >> +                       interrupts = <111>;
> >> +                       #power-domain-cells = <1>;
> >> +               };
> >> +
> >>                 uart0: serial@10000000 {
> >>                         compatible = "snps,dw-apb-uart";
> >>                         reg = <0x0 0x10000000 0x0 0x10000>;
> >> --
> >> 2.17.1
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
>

Re: [PATCH v1 2/4] dt-bindings: power: Add starfive,jh71xx-power bindings

[Rob Herring]

On Tue, Nov 22, 2022 at 09:22:03PM +0800, Walker Chen wrote:
> On 2022/11/21 18:13, Krzysztof Kozlowski wrote:
> > On 18/11/2022 14:32, Walker Chen wrote:
> >> Add bindings for the power domain controller on the StarFive JH71XX SoC.
> >> 
> > 
> > Subject: drop second, redundant "bindings".
> 
> Will fix.
> 
> > 
> >> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> >> ---
> >>  .../bindings/power/starfive,jh71xx-power.yaml | 46 +++++++++++++++++++
> > 
> > 1st patch should be squashed here. Headers are part of bindings file.
> 
> Will be done in the next version of patch.
> 
> > 
> >>  1 file changed, 46 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
> >> 
> >> diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
> >> new file mode 100644
> >> index 000000000000..2537303b4829
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
> > 
> > Filename like compatible.
> 
> As mentioned in the previous email, the compatible in the driver should be changed to "starfive,jh7110-power".

Is the h/w block called 'power' or 'pmu'? Call it what the h/w is 
called.

Rob

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/11/25 19:17, Conor Dooley wrote:
> On Fri, Nov 25, 2022 at 06:04:59PM +0800, Walker Chen wrote:
>> On 2022/11/19 8:24, Conor Dooley wrote:
>> > On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:
> 
>> >> +void starfive_pmu_hw_event_turn_off(u32 mask)
>> >> +{
>> >> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
>> >> +}
>> >> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
>> > 
>> > Where are the users for these exports? Also, should they be exported as
>> > GPL?
>> > 
>> > Either way, what is the point of the extra layer of abstraction here
>> > around the writel()?
>> 
>> The two export functions are only prepared for GPU module. But accordint to
>>  the latest information, it seems that there is no open source plan for GPU. 
>> So the two functions will be drop in next version of patch.
> 
> That's a shame!

Need to comply with certain commercial terms.

> 
>> >> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
>> >> +{
>> >> +	struct starfive_pmu *pmu = pmd->power;
>> >> +
>> >> +	if (!pmd->mask) {
>> >> +		*is_on = false;
>> >> +		return -EINVAL;
>> >> +	}
>> >> +
>> >> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
>> > 
>> > Is there a specific reason that you are using the __raw variants here
>> > (and elsewhere) in the driver?
>> 
>> Will use unified function '__raw_readl' and '__raw_writel'
> 
> No no, I want to know *why* you are using the __raw accessors here. My
> understanding was that __raw variants are unbarriered & unordered with
> respect to other io accesses.
> 
> I do notice that the bcm driver you mentioned uses the __raw variants,
> but only __raw variants - whereas you use readl() which is ordered and
> barriered & __raw_readl().
> 
> Is there a reason why you would not use readl() or readl_relaxed()?

Your question led me to deeply understand the usage of these io accessors.
__raw_readl / __raw_writel denotes native byte order, no memory barrier.
readl / writel do guarantee the byte order with barrier, ensure that previous writes are done.
Seem that non-raw accessors are more safe.

> 
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
>> >> +{
>> >> +	struct starfive_pmu *pmu = pmd->power;
>> >> +	unsigned long flags;
>> >> +	uint32_t val;
>> >> +	uint32_t mode;
>> >> +	uint32_t encourage_lo;
>> >> +	uint32_t encourage_hi;
>> >> +	bool is_on;
>> >> +	int ret;
>> >> +
>> >> +	if (!pmd->mask)
>> >> +		return -EINVAL;
>> >> +
>> >> +	if (is_on == on) {
>> >> +		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
>> >> +				pmd->genpd.name, on ? "en" : "dis");
>> > 
>> > Am I missing something here: you've just declared is_on, so it must be
>> > false & therefore this check is all a little pointless? The check just
>> > becomes if (false == on) which I don't think is what you're going for
>> > here? Or did I miss something obvious?
>> 
>> Sorry, indeed I missed several lines of code. It should be witten like this:
>> 	ret = jh71xx_pmu_get_state(pmd, &is_on);
>>         if (ret) {
>>                 dev_dbg(pmu->pdev, "unable to get current state for %s\n",
>>                         pmd->genpd.name);
>>                 return ret;
>>         }
> 
> Heh, this looks more sane :)
> 
>> 
>>         if (is_on == on) {
>>                 dev_dbg(pmu->pdev, "pm domain [%s] is already %sable status.\n",
>>                         pmd->genpd.name, on ? "en" : "dis");
>>                 return 0;
>>         }
>> 
>> > 
>> >> +		return 0;
>> >> +	}
>> >> +
>> >> +	spin_lock_irqsave(&pmu->lock, flags);
>> >> +
>> >> +	if (on) {
>> >> +		mode = SW_TURN_ON_POWER_MODE;
>> >> +		encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
>> >> +		encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
>> >> +	} else {
>> >> +		mode = SW_TURN_OFF_POWER_MODE;
>> >> +		encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
>> >> +		encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
>> >> +	}
>> >> +
>> >> +	__raw_writel(pmd->mask, pmu->base + mode);
>> >> +
>> >> +	/* write SW_ENCOURAGE to make the configuration take effect */
>> >> +	__raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
>> > 
>> > Is register "sticky"? IOW, could you set it in probe and leave this mode
>> > always on? Or does it need to be set every time you want to use this
>> > feature?
>> 
>> These power domain registers need to be set by other module according to the
>> specific situation. 
>> For example some power domains should be turned off via System PM mechanism
>> when system goes to sleep, 
>> and then they are turned on when system resume.
> 
> I was just wondering if SW_MODE_ENCOURAGE_ON would retain it's value
> during operation or if it had to be written every time. It's fine if
> that's not the case.

Writing SW_MODE_ENCOURAGE_ON (0xFF) to 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. 

> 
>> >> +	__raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
>> >> +	__raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
>> >> +
>> >> +	spin_unlock_irqrestore(&pmu->lock, flags);
>> >> +
>> >> +	if (on) {
>> >> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
>> >> +						val & pmd->mask, DELAY_US,
>> >> +						TIMEOUT_US);
>> >> +		if (ret) {
>> >> +			dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
>> >> +			return -ETIMEDOUT;
>> >> +		}
>> >> +	} else {
>> >> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
>> >> +						!(val & pmd->mask), DELAY_US,
>> >> +						TIMEOUT_US);
>> > 
>> > Could you not just decide the 3rd arg outside of the readl_poll..() and
>> > save on duplicating the wait logic here?
>> 
>> Seems that the readl_poll..() can only be called in two cases 
>> because the CURR_POWER_MODE register is read to val and then operation with mask every time.
>> 
> 
> I'm sorry, I completely forgot that read*_poll..() actually not actually
> a function. Please ignore this comment!
> 
>> >> +static int starfive_pmu_probe(struct platform_device *pdev)
>> >> +{
>> >> +	struct device *dev = &pdev->dev;
>> >> +	struct device_node *np = dev->of_node;
>> >> +	const struct starfive_pmu_data *entry, *table;
>> >> +	struct starfive_pmu *pmu;
>> >> +	unsigned int i;
>> >> +	uint8_t max_bit = 0;
>> >> +	int ret;
>> >> +
>> >> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
>> >> +	if (!pmu)
>> >> +		return -ENOMEM;
>> >> +
>> >> +	pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
>> >> +	if (IS_ERR(pmu->base))
>> >> +		return PTR_ERR(pmu->base);
>> >> +
>> >> +	/* initialize pmu interrupt  */
>> >> +	pmu->irq = platform_get_irq(pdev, 0);
>> >> +	if (pmu->irq < 0)
>> >> +		return pmu->irq;
>> >> +
>> >> +	ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
>> >> +			       0, pdev->name, pmu);
>> >> +	if (ret)
>> >> +		dev_err(dev, "request irq failed.\n");
>> >> +
>> >> +	table = of_device_get_match_data(dev);
>> >> +	if (!table)
>> >> +		return -EINVAL;
>> >> +
>> >> +	pmu->pdev = dev;
>> >> +	pmu->genpd_data.num_domains = 0;
>> >> +	i = 0;
>> >> +	for (entry = table; entry->name; entry++) {
>> >> +		max_bit = max(max_bit, entry->bit);
>> >> +		i++;
>> >> +	}
>> > 
>> > This looks like something that could be replaced by the functions in
>> > linux/list.h, no? Same below.
>> 
>> Nowadays other platforms on linux mainline mostly write in this way or write like this: 
>> 	for (i = 0; i < num; i++) {
>> 		...
>> 		pm_genpd_init(&pmd[i]->genpd, NULL, !is_on);
>> 	}
> 
> That's not what this specific bit of code is doing though, right? You're
> walking jh7110_power_domains to find the highest bit. I was looking at what
> some other drivers do, and took a look at drivers/soc/qcom/rpmhpd.c
> where they know the size of this struct at compile time & so can do
> store the number of power domains in the match data. If you did that,
> you could use a loop like the one other platforms use.
> 
>> >> +	if (!pmu->genpd)
>> >> +		return -ENOMEM;
>> >> +
>> >> +	pmu->genpd_data.domains = pmu->genpd;
>> >> +
>> >> +	i = 0;
>> >> +	for (entry = table; entry->name; entry++) {
> 
> And it's the same here. By now, you should know how many power domains
> you have, no?
> 
> Anyways, as I said before I don't know much about this power domain
> stuff, it's just that these two loops seem odd.

About the usage of loop, I took a look at other platforms like drivers/soc/qcom/rpmhpd.c,
maybe that way is simpler and easier to understand. I will try to change to that looping in next version.

> 
>> >> +		struct starfive_power_dev *pmd = &pmu->dev[i];
>> >> +		bool is_on;
>> >> +
>> >> +		pmd->power = pmu;
>> >> +		pmd->mask = BIT(entry->bit);
>> >> +		pmd->genpd.name = entry->name;
>> >> +		pmd->genpd.flags = entry->flags;
>> >> +
>> >> +		ret = starfive_pmu_get_state(pmd, &is_on);
>> >> +		if (ret)
>> >> +			dev_warn(dev, "unable to get current state for %s\n",
>> >> +				 pmd->genpd.name);
> 
>> >> +static const struct starfive_pmu_data jh7110_power_domains[] = {
>> > 
>> > Is this driver jh7110 only or actually jh71XX? Have you just started out
>> > by implementing one SoC both intend to support both?
>> 
>> JH7110 is our first SoC, probably the next generation of SoC jh7120 still
>> use this controller driver.
>> Maybe now the naming for JH71XX is not very suitable.
> 
> Right. My question was more about if this supported the JH7100 too, but
> I saw from your answer to Emil that it won't. I don't have a preference
> as to whether you call it jh71xx or jh7110, I'll leave that up to
> yourselves and Emil. This particular struct should still be called
> `jh7110_power_domains` though since it is particular to this SoC, no
> matter what you end up calling the file etc.

Emil has gave me a good suggestion.

> 
>> > I don't know /jack/ about power domain stuff, so I can barely review
>> > this at all sadly.
> 
>> Thank you for taking the time to review the code, you helped me a lot.
>> Thank you so much.
> 
> No worries, looking forward to getting my board :)
> 
Have you purchased a VisionFive 2 board online?  Do you need the shopping link ? 
https://forum.rvspace.org/t/how-to-purchase-visionfive-2/665

Best Regards,
Walker Chen

Re: [PATCH v1 2/4] dt-bindings: power: Add starfive,jh71xx-power bindings

[Walker Chen]

On 2022/11/30 23:24, Rob Herring wrote:
> On Tue, Nov 22, 2022 at 09:22:03PM +0800, Walker Chen wrote:
>> On 2022/11/21 18:13, Krzysztof Kozlowski wrote:
>> > On 18/11/2022 14:32, Walker Chen wrote:
>> >> Add bindings for the power domain controller on the StarFive JH71XX SoC.
>> >> 
>> > 
>> > Subject: drop second, redundant "bindings".
>> 
>> Will fix.
>> 
>> > 
>> >> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> >> ---
>> >>  .../bindings/power/starfive,jh71xx-power.yaml | 46 +++++++++++++++++++
>> > 
>> > 1st patch should be squashed here. Headers are part of bindings file.
>> 
>> Will be done in the next version of patch.
>> 
>> > 
>> >>  1 file changed, 46 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
>> >> 
>> >> diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
>> >> new file mode 100644
>> >> index 000000000000..2537303b4829
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-power.yaml
>> > 
>> > Filename like compatible.
>> 
>> As mentioned in the previous email, the compatible in the driver should be changed to "starfive,jh7110-power".
> 
> Is the h/w block called 'power' or 'pmu'? Call it what the h/w is 
> called.

h/w block is called PMU, is the abbreviation of Power Management Unit.
It is more appropriate to change the compatile to 'starfive,jh7110-pmu'.

Best Regards,
Walker Chen

Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Conor Dooley]

Hey Walker,

Apologies for my formatting here. 

On 1 December 2022 03:56:27 GMT, Walker Chen <walker.chen@starfivetech.com> wrote:
>On 2022/11/25 19:17, Conor Dooley wrote:
>> On Fri, Nov 25, 2022 at 06:04:59PM +0800, Walker Chen wrote:
>>> On 2022/11/19 8:24, Conor Dooley wrote:
>>> > On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:
>> 
>>> >> +void starfive_pmu_hw_event_turn_off(u32 mask)
>>> >> +{
>>> >> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
>>> >> +}
>>> >> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
>>> > 
>>> > Where are the users for these exports? Also, should they be exported as
>>> > GPL?
>>> > 
>>> > Either way, what is the point of the extra layer of abstraction here
>>> > around the writel()?
>>> 
>>> The two export functions are only prepared for GPU module. But accordint to
>>>  the latest information, it seems that there is no open source plan for GPU. 
>>> So the two functions will be drop in next version of patch.
>> 
>> That's a shame!
>
>Need to comply with certain commercial terms.
>
>> 
>>> >> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
>>> >> +{
>>> >> +	struct starfive_pmu *pmu = pmd->power;
>>> >> +
>>> >> +	if (!pmd->mask) {
>>> >> +		*is_on = false;
>>> >> +		return -EINVAL;
>>> >> +	}
>>> >> +
>>> >> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
>>> > 
>>> > Is there a specific reason that you are using the __raw variants here
>>> > (and elsewhere) in the driver?
>>> 
>>> Will use unified function '__raw_readl' and '__raw_writel'
>> 
>> No no, I want to know *why* you are using the __raw accessors here. My
>> understanding was that __raw variants are unbarriered & unordered with
>> respect to other io accesses.
>> 
>> I do notice that the bcm driver you mentioned uses the __raw variants,
>> but only __raw variants - whereas you use readl() which is ordered and
>> barriered & __raw_readl().
>> 
>> Is there a reason why you would not use readl() or readl_relaxed()?
>
>Your question led me to deeply understand the usage of these io accessors.
>__raw_readl / __raw_writel denotes native byte order, no memory barrier.
>readl / writel do guarantee the byte order with barrier, ensure that previous writes are done.
>Seem that non-raw accessors are more safe.

Yeah, if there's no good reason to use these "raw"  versions then please use readl/readl_relaxed.


>> No worries, looking forward to getting my board :)
>> 
>Have you purchased a VisionFive 2 board online?  

I have :)