[RESEND PATCH v2 0/3] JH7110 PMU Support

[RESEND PATCH v2 0/3] JH7110 PMU Support

[Walker Chen]

Hello,

This patchset adds PMU (Power Management Unit) controller driver for the
StarFive JH7110 SoC. In order to meet low power requirements, PMU is
designed for including multiple PM domains that can be used for power
gating of selected IP blocks for power saving by reduced leakage
current. The first patch adds device tree binding for PM domain provider
and consumer. The second patch adds pmu driver and support JH7110 SoC.
The last patch adds device node about pmu to JH7110 dts. 

The series has been tested on the VisionFive 2 boards which equip with
JH7110 SoC and works normally.

The last patch should be applied after the following patchset:
https://lore.kernel.org/all/20221118011714.70877-1-hal.feng@starfivetech.com/

Changes in v2:
- Squashed 1st patch (dt-bindings header) into 2nd which is related to
  dt-bindings stuff.
- Renamed the dt-bindings header 'jh7110-power.h' to
  'starfive,jh7110-pmu.h' and used dual license for it.
- Renamed the dt-bindings 'starfive,jh71xx-power.yaml' to
  'starfive,jh71xx-pmu.yaml', dropped items from properties.
- Change of MAINTAINERS: added the entry of 'starfive soc drivers';
  changed status to 'Supported' for the entry of
  'STARFIVE JH71XX PMU CONTROLLER DRIVER' and sorted the lines alphabetically.
- Dropped the header file 'include/soc/starfive/pm_domains.h'.
- Dropped starfive_pmu_hw_event_turn_on() and starfive_pmu_hw_event_turn_off().
- Added 'default SOC_STARFIVE' and expanded help text in the Kconfig.
- Added a JH71XX_PMU_ prefix to those macro definitions in driver.
- Replaced the data type 'uint8_t / uint32_t' with 'u8 / u32'.
- Fixed some complains by using checkpatch.pl
- Added spinlock to jh71xx_pmu_int_enable().
- Dropped spinlock from jh71xx_pmu_interrupt().
- Used jh71xx_pmu_ as prefix to all functions.
- Replaced io accessors '__raw_readl / __raw_writel' with 'readl / writel'.
- Added jh71xx_pmu_get_state() to the beginning of jh71xx_pmu_set_state().
- Added more detailed comment about controlling power domain.
- Simplified the usage of loop when performing pm_genpd_init() to register
  power domain.
- Added more detailed description about the features of power domain
  hardware to commit message in 2nd patch.
- Replaced dev_info() with dev_dbg() in jh71xx_pmu_set_state().
- Decreased the timeout numbers of polling power status when switching
  power mode.

Previous versions:
v1 - https://lore.kernel.org/all/20221118133216.17037-1-walker.chen@starfivetech.com/

Best regards,
Walker

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

 .../bindings/power/starfive,jh71xx-pmu.yaml   |  45 ++
 MAINTAINERS                                   |  14 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |   7 +
 drivers/soc/Kconfig                           |   1 +
 drivers/soc/Makefile                          |   1 +
 drivers/soc/starfive/Kconfig                  |  11 +
 drivers/soc/starfive/Makefile                 |   3 +
 drivers/soc/starfive/jh71xx_pmu.c             | 396 ++++++++++++++++++
 .../dt-bindings/power/starfive,jh7110-pmu.h   |  17 +
 9 files changed, 495 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.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/starfive,jh7110-pmu.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

[RESEND PATCH v2 0/3] JH7110 PMU Support

[Walker Chen]

Hello,

This patchset adds PMU (Power Management Unit) controller driver for the
StarFive JH7110 SoC. In order to meet low power requirements, PMU is
designed for including multiple PM domains that can be used for power
gating of selected IP blocks for power saving by reduced leakage
current. The first patch adds device tree binding for PM domain provider
and consumer. The second patch adds pmu driver and support JH7110 SoC.
The last patch adds device node about pmu to JH7110 dts. 

The series has been tested on the VisionFive 2 boards which equip with
JH7110 SoC and works normally.

The last patch should be applied after the following patchset:
https://lore.kernel.org/all/20221118011714.70877-1-hal.feng@starfivetech.com/

Changes in v2:
- Squashed 1st patch (dt-bindings header) into 2nd which is related to
  dt-bindings stuff.
- Renamed the dt-bindings header 'jh7110-power.h' to
  'starfive,jh7110-pmu.h' and used dual license for it.
- Renamed the dt-bindings 'starfive,jh71xx-power.yaml' to
  'starfive,jh71xx-pmu.yaml', dropped items from properties.
- Change of MAINTAINERS: added the entry of 'starfive soc drivers';
  changed status to 'Supported' for the entry of
  'STARFIVE JH71XX PMU CONTROLLER DRIVER' and sorted the lines alphabetically.
- Dropped the header file 'include/soc/starfive/pm_domains.h'.
- Dropped starfive_pmu_hw_event_turn_on() and starfive_pmu_hw_event_turn_off().
- Added 'default SOC_STARFIVE' and expanded help text in the Kconfig.
- Added a JH71XX_PMU_ prefix to those macro definitions in driver.
- Replaced the data type 'uint8_t / uint32_t' with 'u8 / u32'.
- Fixed some complains by using checkpatch.pl
- Added spinlock to jh71xx_pmu_int_enable().
- Dropped spinlock from jh71xx_pmu_interrupt().
- Used jh71xx_pmu_ as prefix to all functions.
- Replaced io accessors '__raw_readl / __raw_writel' with 'readl / writel'.
- Added jh71xx_pmu_get_state() to the beginning of jh71xx_pmu_set_state().
- Added more detailed comment about controlling power domain.
- Simplified the usage of loop when performing pm_genpd_init() to register
  power domain.
- Added more detailed description about the features of power domain
  hardware to commit message in 2nd patch.
- Replaced dev_info() with dev_dbg() in jh71xx_pmu_set_state().
- Decreased the timeout numbers of polling power status when switching
  power mode.

Previous versions:
v1 - https://lore.kernel.org/all/20221118133216.17037-1-walker.chen@starfivetech.com/

Best regards,
Walker

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

 .../bindings/power/starfive,jh71xx-pmu.yaml   |  45 ++
 MAINTAINERS                                   |  14 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |   7 +
 drivers/soc/Kconfig                           |   1 +
 drivers/soc/Makefile                          |   1 +
 drivers/soc/starfive/Kconfig                  |  11 +
 drivers/soc/starfive/Makefile                 |   3 +
 drivers/soc/starfive/jh71xx_pmu.c             | 396 ++++++++++++++++++
 .../dt-bindings/power/starfive,jh7110-pmu.h   |  17 +
 9 files changed, 495 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.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/starfive,jh7110-pmu.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

[RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Walker Chen]

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

Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
---
 .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
 .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
 create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h

diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
new file mode 100644
index 000000000000..f308ae136a57
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/starfive,jh71xx-pmu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH71xx Power Management Unit
+
+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:
+      - 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>;
+    };
diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
new file mode 100644
index 000000000000..73c6a79a2181
--- /dev/null
+++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * 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
+
+#endif
-- 
2.17.1

[RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Walker Chen]

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

Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
---
 .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
 .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
 create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h

diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
new file mode 100644
index 000000000000..f308ae136a57
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/starfive,jh71xx-pmu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH71xx Power Management Unit
+
+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:
+      - 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>;
+    };
diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
new file mode 100644
index 000000000000..73c6a79a2181
--- /dev/null
+++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * 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
+
+#endif
-- 
2.17.1


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

[RESEND PATCH v2 2/3] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

Add pmu driver for the StarFive JH71XX SoC.

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

Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
---
 MAINTAINERS                       |  14 ++
 drivers/soc/Kconfig               |   1 +
 drivers/soc/Makefile              |   1 +
 drivers/soc/starfive/Kconfig      |  11 +
 drivers/soc/starfive/Makefile     |   3 +
 drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
 6 files changed, 426 insertions(+)
 create mode 100644 drivers/soc/starfive/Kconfig
 create mode 100644 drivers/soc/starfive/Makefile
 create mode 100644 drivers/soc/starfive/jh71xx_pmu.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a70c1d0f303e..dbbc25a9f25b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19623,6 +19623,20 @@ F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
 F:	drivers/reset/starfive/
 F:	include/dt-bindings/reset/starfive*
 
+STARFIVE SOC DRIVER
+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/
+
+STARFIVE JH71XX PMU CONTROLLER DRIVER
+M:	Walker Chen <walker.chen@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/power/starfive*
+F:	drivers/soc/starfive/jh71xx_pmu.c
+F:	include/dt-bindings/power/starfive,jh7110-pmu.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..29d92df97421
--- /dev/null
+++ b/drivers/soc/starfive/Kconfig
@@ -0,0 +1,11 @@
+# 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
+	select PM_GENERIC_DOMAINS
+	help
+	  Say 'y' here to enable support power domain support.
+	  In order to meet low power requirements, a Power Management Unit (PMU)
+	  is designed for controlling power resources in StarFive JH71XX SoCs.
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..7a0145779e07
--- /dev/null
+++ b/drivers/soc/starfive/jh71xx_pmu.c
@@ -0,0 +1,396 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * StarFive JH71XX PMU (Power Management Unit) Controller Driver
+ *
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <dt-bindings/power/starfive,jh7110-pmu.h>
+
+/* register offset */
+#define JH71XX_PMU_HW_EVENT_ON		0x04
+#define JH71XX_PMU_HW_EVENT_OFF		0x08
+#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
+#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
+#define JH71XX_PMU_SW_ENCOURAGE		0x44
+#define JH71XX_PMU_INT_MASK		0x48
+#define JH71XX_PMU_PCH_BYPASS		0x4C
+#define JH71XX_PMU_PCH_PSTATE		0x50
+#define JH71XX_PMU_PCH_TIMEOUT		0x54
+#define JH71XX_PMU_LP_TIMEOUT		0x58
+#define JH71XX_PMU_HW_TURN_ON		0x5C
+#define JH71XX_PMU_CURR_POWER_MODE	0x80
+#define JH71XX_PMU_EVENT_STATUS		0x88
+#define JH71XX_PMU_INT_STATUS		0x8C
+
+/* sw encourage cfg */
+#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
+#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
+#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF
+
+/* pmu int status */
+#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
+#define JH71XX_PMU_INT_HW_REQ		BIT(1)
+#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
+#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
+#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
+#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
+					JH71XX_PMU_INT_HW_FAIL | \
+					JH71XX_PMU_INT_PCH_FAIL)
+#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \
+					JH71XX_PMU_INT_HW_REQ | \
+					JH71XX_PMU_INT_FAIL_MASK)
+
+/*
+ * The time required for switching power status is based on the time
+ * to turn on the largest domain's power, which is at microsecond level
+ */
+#define JH71XX_PMU_TIMEOUT_US		100
+
+struct jh71xx_domain_info {
+	const char * const name;
+	u8 bit;
+	unsigned int flags;
+};
+
+struct jh71xx_pmu_match_data {
+	int num_domains;
+	const struct jh71xx_domain_info *domain_info;
+};
+
+struct jh71xx_pmu {
+	struct device *dev;
+	const struct jh71xx_pmu_match_data *match_data;
+	void __iomem *base;
+	spinlock_t lock;	/* protects pmu reg */
+	int irq;
+	struct genpd_onecell_data genpd_data;
+	struct generic_pm_domain **genpd;
+};
+
+struct jh71xx_pmu_dev {
+	struct generic_pm_domain genpd;
+	const struct jh71xx_domain_info *domain_info;
+	struct jh71xx_pmu *pmu;
+};
+
+static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+
+	if (!mask) {
+		*is_on = false;
+		return -EINVAL;
+	}
+
+	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
+
+	return 0;
+}
+
+static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+	unsigned long flags;
+	u32 val;
+	u32 mode;
+	u32 encourage_lo;
+	u32 encourage_hi;
+	bool is_on;
+	int ret;
+
+	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
+	if (ret) {
+		dev_dbg(pmu->dev, "unable to get current state for %s\n",
+			pmd->genpd.name);
+		return ret;
+	}
+
+	if (is_on == on) {
+		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
+			pmd->genpd.name, on ? "en" : "dis");
+		return 0;
+	}
+
+	spin_lock_irqsave(&pmu->lock, flags);
+
+	/*
+	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
+	 *
+	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
+	 *    the bit which power domain will be turn-on, write 0 to the others.
+	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
+	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
+	 */
+	if (on) {
+		mode = JH71XX_PMU_SW_TURN_ON_POWER;
+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
+	} else {
+		mode = JH71XX_PMU_SW_TURN_OFF_POWER;
+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
+	}
+
+	writel(mask, pmu->base + mode);
+
+	/*
+	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
+	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
+	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0
+	 *
+	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
+	 * the purpose is to reset the state machine which is going to parse instruction
+	 *  sequence. It has to be written every time.
+	 */
+	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	/* Wait for the power domain bit to be enabled / disabled */
+	if (on) {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, val & mask,
+						1, JH71XX_PMU_TIMEOUT_US);
+	} else {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, !(val & mask),
+						1, JH71XX_PMU_TIMEOUT_US);
+	}
+
+	if (ret) {
+		dev_err(pmu->dev, "%s: failed to power %s\n",
+			pmd->genpd.name, on ? "on" : "off");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int jh71xx_pmu_on(struct generic_pm_domain *genpd)
+{
+	struct jh71xx_pmu_dev *pmd = container_of(genpd,
+						  struct jh71xx_pmu_dev, genpd);
+	u32 pwr_mask = BIT(pmd->domain_info->bit);
+
+	return jh71xx_pmu_set_state(pmd, pwr_mask, true);
+}
+
+static int jh71xx_pmu_off(struct generic_pm_domain *genpd)
+{
+	struct jh71xx_pmu_dev *pmd = container_of(genpd,
+						  struct jh71xx_pmu_dev, genpd);
+	u32 pwr_mask = BIT(pmd->domain_info->bit);
+
+	return jh71xx_pmu_set_state(pmd, pwr_mask, false);
+}
+
+static void jh71xx_pmu_int_enable(struct jh71xx_pmu *pmu, u32 mask, bool enable)
+{
+	u32 val;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pmu->lock, flags);
+	val = readl(pmu->base + JH71XX_PMU_INT_MASK);
+
+	if (enable)
+		val &= ~mask;
+	else
+		val |= mask;
+
+	writel(val, pmu->base + JH71XX_PMU_INT_MASK);
+	spin_unlock_irqrestore(&pmu->lock, flags);
+}
+
+static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data)
+{
+	struct jh71xx_pmu *pmu = data;
+	u32 val;
+
+	val = readl(pmu->base + JH71XX_PMU_INT_STATUS);
+
+	if (val & JH71XX_PMU_INT_SEQ_DONE)
+		dev_dbg(pmu->dev, "sequence done.\n");
+	if (val & JH71XX_PMU_INT_HW_REQ)
+		dev_dbg(pmu->dev, "hardware encourage requestion.\n");
+	if (val & JH71XX_PMU_INT_SW_FAIL)
+		dev_err(pmu->dev, "software encourage fail.\n");
+	if (val & JH71XX_PMU_INT_HW_FAIL)
+		dev_err(pmu->dev, "hardware encourage fail.\n");
+	if (val & JH71XX_PMU_INT_PCH_FAIL)
+		dev_err(pmu->dev, "p-channel fail event.\n");
+
+	/* clear interrupts */
+	writel(val, pmu->base + JH71XX_PMU_INT_STATUS);
+	writel(val, pmu->base + JH71XX_PMU_EVENT_STATUS);
+
+	return IRQ_HANDLED;
+}
+
+static int jh71xx_pmu_init_domain(struct jh71xx_pmu *pmu, int index)
+{
+	struct jh71xx_pmu_dev *pmd;
+	bool is_on;
+	u32 pwr_mask;
+	int ret;
+
+	pmd = devm_kzalloc(pmu->dev, sizeof(*pmd), GFP_KERNEL);
+	if (!pmd)
+		return -ENOMEM;
+
+	pmd->domain_info = &pmu->match_data->domain_info[index];
+	pmd->pmu = pmu;
+	pwr_mask = BIT(pmd->domain_info->bit);
+
+	pmd->genpd.name = pmd->domain_info->name;
+	pmd->genpd.flags = pmd->domain_info->flags;
+
+	ret = jh71xx_pmu_get_state(pmd, pwr_mask, &is_on);
+	if (ret)
+		dev_warn(pmu->dev, "unable to get current state for %s\n",
+			 pmd->genpd.name);
+
+	pmd->genpd.power_on = jh71xx_pmu_on;
+	pmd->genpd.power_off = jh71xx_pmu_off;
+	pm_genpd_init(&pmd->genpd, NULL, !is_on);
+
+	pmu->genpd_data.domains[index] = &pmd->genpd;
+
+	return 0;
+}
+
+static int jh71xx_pmu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	const struct jh71xx_pmu_match_data *match_data;
+	struct jh71xx_pmu *pmu;
+	unsigned int i;
+	int ret;
+
+	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
+	if (!pmu)
+		return -ENOMEM;
+
+	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, jh71xx_pmu_interrupt,
+			       0, pdev->name, pmu);
+	if (ret)
+		dev_err(dev, "request irq failed.\n");
+
+	match_data = of_device_get_match_data(dev);
+	if (!match_data)
+		return -EINVAL;
+
+	pmu->genpd = devm_kcalloc(dev, match_data->num_domains,
+				  sizeof(struct generic_pm_domain *),
+				  GFP_KERNEL);
+	if (!pmu->genpd)
+		return -ENOMEM;
+
+	pmu->dev = dev;
+	pmu->match_data = match_data;
+	pmu->genpd_data.domains = pmu->genpd;
+	pmu->genpd_data.num_domains = match_data->num_domains;
+
+	for (i = 0; i < match_data->num_domains; i++) {
+		ret = jh71xx_pmu_init_domain(pmu, i);
+		if (ret) {
+			dev_err(dev, "failed to initialize power domain\n");
+			return ret;
+		}
+	}
+
+	spin_lock_init(&pmu->lock);
+	jh71xx_pmu_int_enable(pmu, JH71XX_PMU_INT_ALL_MASK & ~JH71XX_PMU_INT_PCH_FAIL, true);
+
+	ret = of_genpd_add_provider_onecell(np, &pmu->genpd_data);
+	if (ret) {
+		dev_err(dev, "failed to register genpd driver: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(dev, "registered %u power domains\n", i);
+
+	return 0;
+}
+
+static const struct jh71xx_domain_info jh7110_power_domains[] = {
+	[JH7110_PD_SYSTOP] = {
+		.name = "SYSTOP",
+		.bit = 0,
+		.flags = GENPD_FLAG_ALWAYS_ON,
+	},
+	[JH7110_PD_CPU] = {
+		.name = "CPU",
+		.bit = 1,
+		.flags = GENPD_FLAG_ALWAYS_ON,
+	},
+	[JH7110_PD_GPUA] = {
+		.name = "GPUA",
+		.bit = 2,
+	},
+	[JH7110_PD_VDEC] = {
+		.name = "VDEC",
+		.bit = 3,
+	},
+	[JH7110_PD_VOUT] = {
+		.name = "VOUT",
+		.bit = 4,
+	},
+	[JH7110_PD_ISP] = {
+		.name = "ISP",
+		.bit = 5,
+	},
+	[JH7110_PD_VENC] = {
+		.name = "VENC",
+		.bit = 6,
+	},
+};
+
+static const struct jh71xx_pmu_match_data jh7110_pmu = {
+	.num_domains = ARRAY_SIZE(jh7110_power_domains),
+	.domain_info = jh7110_power_domains,
+};
+
+static const struct of_device_id jh71xx_pmu_of_match[] = {
+	{
+		.compatible = "starfive,jh7110-pmu",
+		.data = (void *)&jh7110_pmu,
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct platform_driver jh71xx_pmu_driver = {
+	.driver = {
+		.name = "jh71xx-pmu",
+		.of_match_table = jh71xx_pmu_of_match,
+	},
+	.probe  = jh71xx_pmu_probe,
+};
+builtin_platform_driver(jh71xx_pmu_driver);
+
+MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>");
+MODULE_DESCRIPTION("StarFive JH71XX PMU Driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

[RESEND PATCH v2 2/3] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

Add pmu driver for the StarFive JH71XX SoC.

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

Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
---
 MAINTAINERS                       |  14 ++
 drivers/soc/Kconfig               |   1 +
 drivers/soc/Makefile              |   1 +
 drivers/soc/starfive/Kconfig      |  11 +
 drivers/soc/starfive/Makefile     |   3 +
 drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
 6 files changed, 426 insertions(+)
 create mode 100644 drivers/soc/starfive/Kconfig
 create mode 100644 drivers/soc/starfive/Makefile
 create mode 100644 drivers/soc/starfive/jh71xx_pmu.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a70c1d0f303e..dbbc25a9f25b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19623,6 +19623,20 @@ F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
 F:	drivers/reset/starfive/
 F:	include/dt-bindings/reset/starfive*
 
+STARFIVE SOC DRIVER
+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/
+
+STARFIVE JH71XX PMU CONTROLLER DRIVER
+M:	Walker Chen <walker.chen@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/power/starfive*
+F:	drivers/soc/starfive/jh71xx_pmu.c
+F:	include/dt-bindings/power/starfive,jh7110-pmu.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..29d92df97421
--- /dev/null
+++ b/drivers/soc/starfive/Kconfig
@@ -0,0 +1,11 @@
+# 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
+	select PM_GENERIC_DOMAINS
+	help
+	  Say 'y' here to enable support power domain support.
+	  In order to meet low power requirements, a Power Management Unit (PMU)
+	  is designed for controlling power resources in StarFive JH71XX SoCs.
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..7a0145779e07
--- /dev/null
+++ b/drivers/soc/starfive/jh71xx_pmu.c
@@ -0,0 +1,396 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * StarFive JH71XX PMU (Power Management Unit) Controller Driver
+ *
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <dt-bindings/power/starfive,jh7110-pmu.h>
+
+/* register offset */
+#define JH71XX_PMU_HW_EVENT_ON		0x04
+#define JH71XX_PMU_HW_EVENT_OFF		0x08
+#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
+#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
+#define JH71XX_PMU_SW_ENCOURAGE		0x44
+#define JH71XX_PMU_INT_MASK		0x48
+#define JH71XX_PMU_PCH_BYPASS		0x4C
+#define JH71XX_PMU_PCH_PSTATE		0x50
+#define JH71XX_PMU_PCH_TIMEOUT		0x54
+#define JH71XX_PMU_LP_TIMEOUT		0x58
+#define JH71XX_PMU_HW_TURN_ON		0x5C
+#define JH71XX_PMU_CURR_POWER_MODE	0x80
+#define JH71XX_PMU_EVENT_STATUS		0x88
+#define JH71XX_PMU_INT_STATUS		0x8C
+
+/* sw encourage cfg */
+#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
+#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
+#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
+#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF
+
+/* pmu int status */
+#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
+#define JH71XX_PMU_INT_HW_REQ		BIT(1)
+#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
+#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
+#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
+#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
+					JH71XX_PMU_INT_HW_FAIL | \
+					JH71XX_PMU_INT_PCH_FAIL)
+#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \
+					JH71XX_PMU_INT_HW_REQ | \
+					JH71XX_PMU_INT_FAIL_MASK)
+
+/*
+ * The time required for switching power status is based on the time
+ * to turn on the largest domain's power, which is at microsecond level
+ */
+#define JH71XX_PMU_TIMEOUT_US		100
+
+struct jh71xx_domain_info {
+	const char * const name;
+	u8 bit;
+	unsigned int flags;
+};
+
+struct jh71xx_pmu_match_data {
+	int num_domains;
+	const struct jh71xx_domain_info *domain_info;
+};
+
+struct jh71xx_pmu {
+	struct device *dev;
+	const struct jh71xx_pmu_match_data *match_data;
+	void __iomem *base;
+	spinlock_t lock;	/* protects pmu reg */
+	int irq;
+	struct genpd_onecell_data genpd_data;
+	struct generic_pm_domain **genpd;
+};
+
+struct jh71xx_pmu_dev {
+	struct generic_pm_domain genpd;
+	const struct jh71xx_domain_info *domain_info;
+	struct jh71xx_pmu *pmu;
+};
+
+static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+
+	if (!mask) {
+		*is_on = false;
+		return -EINVAL;
+	}
+
+	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
+
+	return 0;
+}
+
+static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+	unsigned long flags;
+	u32 val;
+	u32 mode;
+	u32 encourage_lo;
+	u32 encourage_hi;
+	bool is_on;
+	int ret;
+
+	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
+	if (ret) {
+		dev_dbg(pmu->dev, "unable to get current state for %s\n",
+			pmd->genpd.name);
+		return ret;
+	}
+
+	if (is_on == on) {
+		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
+			pmd->genpd.name, on ? "en" : "dis");
+		return 0;
+	}
+
+	spin_lock_irqsave(&pmu->lock, flags);
+
+	/*
+	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
+	 *
+	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
+	 *    the bit which power domain will be turn-on, write 0 to the others.
+	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
+	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
+	 */
+	if (on) {
+		mode = JH71XX_PMU_SW_TURN_ON_POWER;
+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
+	} else {
+		mode = JH71XX_PMU_SW_TURN_OFF_POWER;
+		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
+		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
+	}
+
+	writel(mask, pmu->base + mode);
+
+	/*
+	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
+	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
+	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0
+	 *
+	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
+	 * the purpose is to reset the state machine which is going to parse instruction
+	 *  sequence. It has to be written every time.
+	 */
+	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	/* Wait for the power domain bit to be enabled / disabled */
+	if (on) {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, val & mask,
+						1, JH71XX_PMU_TIMEOUT_US);
+	} else {
+		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
+						val, !(val & mask),
+						1, JH71XX_PMU_TIMEOUT_US);
+	}
+
+	if (ret) {
+		dev_err(pmu->dev, "%s: failed to power %s\n",
+			pmd->genpd.name, on ? "on" : "off");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int jh71xx_pmu_on(struct generic_pm_domain *genpd)
+{
+	struct jh71xx_pmu_dev *pmd = container_of(genpd,
+						  struct jh71xx_pmu_dev, genpd);
+	u32 pwr_mask = BIT(pmd->domain_info->bit);
+
+	return jh71xx_pmu_set_state(pmd, pwr_mask, true);
+}
+
+static int jh71xx_pmu_off(struct generic_pm_domain *genpd)
+{
+	struct jh71xx_pmu_dev *pmd = container_of(genpd,
+						  struct jh71xx_pmu_dev, genpd);
+	u32 pwr_mask = BIT(pmd->domain_info->bit);
+
+	return jh71xx_pmu_set_state(pmd, pwr_mask, false);
+}
+
+static void jh71xx_pmu_int_enable(struct jh71xx_pmu *pmu, u32 mask, bool enable)
+{
+	u32 val;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pmu->lock, flags);
+	val = readl(pmu->base + JH71XX_PMU_INT_MASK);
+
+	if (enable)
+		val &= ~mask;
+	else
+		val |= mask;
+
+	writel(val, pmu->base + JH71XX_PMU_INT_MASK);
+	spin_unlock_irqrestore(&pmu->lock, flags);
+}
+
+static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data)
+{
+	struct jh71xx_pmu *pmu = data;
+	u32 val;
+
+	val = readl(pmu->base + JH71XX_PMU_INT_STATUS);
+
+	if (val & JH71XX_PMU_INT_SEQ_DONE)
+		dev_dbg(pmu->dev, "sequence done.\n");
+	if (val & JH71XX_PMU_INT_HW_REQ)
+		dev_dbg(pmu->dev, "hardware encourage requestion.\n");
+	if (val & JH71XX_PMU_INT_SW_FAIL)
+		dev_err(pmu->dev, "software encourage fail.\n");
+	if (val & JH71XX_PMU_INT_HW_FAIL)
+		dev_err(pmu->dev, "hardware encourage fail.\n");
+	if (val & JH71XX_PMU_INT_PCH_FAIL)
+		dev_err(pmu->dev, "p-channel fail event.\n");
+
+	/* clear interrupts */
+	writel(val, pmu->base + JH71XX_PMU_INT_STATUS);
+	writel(val, pmu->base + JH71XX_PMU_EVENT_STATUS);
+
+	return IRQ_HANDLED;
+}
+
+static int jh71xx_pmu_init_domain(struct jh71xx_pmu *pmu, int index)
+{
+	struct jh71xx_pmu_dev *pmd;
+	bool is_on;
+	u32 pwr_mask;
+	int ret;
+
+	pmd = devm_kzalloc(pmu->dev, sizeof(*pmd), GFP_KERNEL);
+	if (!pmd)
+		return -ENOMEM;
+
+	pmd->domain_info = &pmu->match_data->domain_info[index];
+	pmd->pmu = pmu;
+	pwr_mask = BIT(pmd->domain_info->bit);
+
+	pmd->genpd.name = pmd->domain_info->name;
+	pmd->genpd.flags = pmd->domain_info->flags;
+
+	ret = jh71xx_pmu_get_state(pmd, pwr_mask, &is_on);
+	if (ret)
+		dev_warn(pmu->dev, "unable to get current state for %s\n",
+			 pmd->genpd.name);
+
+	pmd->genpd.power_on = jh71xx_pmu_on;
+	pmd->genpd.power_off = jh71xx_pmu_off;
+	pm_genpd_init(&pmd->genpd, NULL, !is_on);
+
+	pmu->genpd_data.domains[index] = &pmd->genpd;
+
+	return 0;
+}
+
+static int jh71xx_pmu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	const struct jh71xx_pmu_match_data *match_data;
+	struct jh71xx_pmu *pmu;
+	unsigned int i;
+	int ret;
+
+	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
+	if (!pmu)
+		return -ENOMEM;
+
+	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, jh71xx_pmu_interrupt,
+			       0, pdev->name, pmu);
+	if (ret)
+		dev_err(dev, "request irq failed.\n");
+
+	match_data = of_device_get_match_data(dev);
+	if (!match_data)
+		return -EINVAL;
+
+	pmu->genpd = devm_kcalloc(dev, match_data->num_domains,
+				  sizeof(struct generic_pm_domain *),
+				  GFP_KERNEL);
+	if (!pmu->genpd)
+		return -ENOMEM;
+
+	pmu->dev = dev;
+	pmu->match_data = match_data;
+	pmu->genpd_data.domains = pmu->genpd;
+	pmu->genpd_data.num_domains = match_data->num_domains;
+
+	for (i = 0; i < match_data->num_domains; i++) {
+		ret = jh71xx_pmu_init_domain(pmu, i);
+		if (ret) {
+			dev_err(dev, "failed to initialize power domain\n");
+			return ret;
+		}
+	}
+
+	spin_lock_init(&pmu->lock);
+	jh71xx_pmu_int_enable(pmu, JH71XX_PMU_INT_ALL_MASK & ~JH71XX_PMU_INT_PCH_FAIL, true);
+
+	ret = of_genpd_add_provider_onecell(np, &pmu->genpd_data);
+	if (ret) {
+		dev_err(dev, "failed to register genpd driver: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(dev, "registered %u power domains\n", i);
+
+	return 0;
+}
+
+static const struct jh71xx_domain_info jh7110_power_domains[] = {
+	[JH7110_PD_SYSTOP] = {
+		.name = "SYSTOP",
+		.bit = 0,
+		.flags = GENPD_FLAG_ALWAYS_ON,
+	},
+	[JH7110_PD_CPU] = {
+		.name = "CPU",
+		.bit = 1,
+		.flags = GENPD_FLAG_ALWAYS_ON,
+	},
+	[JH7110_PD_GPUA] = {
+		.name = "GPUA",
+		.bit = 2,
+	},
+	[JH7110_PD_VDEC] = {
+		.name = "VDEC",
+		.bit = 3,
+	},
+	[JH7110_PD_VOUT] = {
+		.name = "VOUT",
+		.bit = 4,
+	},
+	[JH7110_PD_ISP] = {
+		.name = "ISP",
+		.bit = 5,
+	},
+	[JH7110_PD_VENC] = {
+		.name = "VENC",
+		.bit = 6,
+	},
+};
+
+static const struct jh71xx_pmu_match_data jh7110_pmu = {
+	.num_domains = ARRAY_SIZE(jh7110_power_domains),
+	.domain_info = jh7110_power_domains,
+};
+
+static const struct of_device_id jh71xx_pmu_of_match[] = {
+	{
+		.compatible = "starfive,jh7110-pmu",
+		.data = (void *)&jh7110_pmu,
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct platform_driver jh71xx_pmu_driver = {
+	.driver = {
+		.name = "jh71xx-pmu",
+		.of_match_table = jh71xx_pmu_of_match,
+	},
+	.probe  = jh71xx_pmu_probe,
+};
+builtin_platform_driver(jh71xx_pmu_driver);
+
+MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>");
+MODULE_DESCRIPTION("StarFive JH71XX PMU Driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1


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

[RESEND PATCH v2 3/3] riscv: dts: starfive: add pmu controller node

[Walker Chen]

This adds the pmu 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

[RESEND PATCH v2 3/3] riscv: dts: starfive: add pmu controller node

[Walker Chen]

This adds the pmu 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


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

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Krzysztof Kozlowski]

On 08/12/2022 09:45, Walker Chen wrote:
> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
>  .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml

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


>  create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h
> 
> diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
> new file mode 100644
> index 000000000000..f308ae136a57
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/starfive,jh71xx-pmu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH71xx Power Management Unit
> +
> +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:
> +      - enum:

Wrong indentation.

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

> +          - starfive,jh7110-pmu
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  "#power-domain-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - "#power-domain-cells"
> +

Best regards,
Krzysztof

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Krzysztof Kozlowski]

On 08/12/2022 09:45, Walker Chen wrote:
> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
>  .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml

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


>  create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h
> 
> diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
> new file mode 100644
> index 000000000000..f308ae136a57
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/starfive,jh71xx-pmu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH71xx Power Management Unit
> +
> +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:
> +      - enum:

Wrong indentation.

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

> +          - starfive,jh7110-pmu
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  "#power-domain-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - "#power-domain-cells"
> +

Best regards,
Krzysztof


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

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Krzysztof Kozlowski]

On 08/12/2022 09:45, Walker Chen wrote:
> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>



> +    };
> diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
> new file mode 100644
> index 000000000000..73c6a79a2181
> --- /dev/null
> +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */

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

> +/*
> + * 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
> +
> +#endif

Best regards,
Krzysztof

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Krzysztof Kozlowski]

On 08/12/2022 09:45, Walker Chen wrote:
> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>



> +    };
> diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
> new file mode 100644
> index 000000000000..73c6a79a2181
> --- /dev/null
> +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */

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

> +/*
> + * 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
> +
> +#endif

Best regards,
Krzysztof


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

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Rob Herring]

On Thu, 08 Dec 2022 16:45:21 +0800, Walker Chen wrote:
> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
>  .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
>  create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml:18:7: [warning] wrong indentation: expected 4 but found 6 (indentation)

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

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221208084523.9733-2-walker.chen@starfivetech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Rob Herring]

On Thu, 08 Dec 2022 16:45:21 +0800, Walker Chen wrote:
> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
>  .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
>  create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml:18:7: [warning] wrong indentation: expected 4 but found 6 (indentation)

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

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221208084523.9733-2-walker.chen@starfivetech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


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

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Walker Chen]

On 2022/12/8 16:59, Krzysztof Kozlowski wrote:
> On 08/12/2022 09:45, Walker Chen wrote:
>> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
>>  .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
>>  2 files changed, 62 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
> 
> Filename matching compatible, so:
> starfive,jh7110-pmu.yaml

OK, will be fixed.

> 
> 
>>  create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h
>> 
>> diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
>> new file mode 100644
>> index 000000000000..f308ae136a57
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
>> @@ -0,0 +1,45 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/starfive,jh71xx-pmu.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive JH71xx Power Management Unit
>> +
>> +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:
>> +      - enum:
> 
> Wrong indentation.
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).

This is my mistake, I forgot to make dt_binding_check for .yaml file.

> 
>> +          - starfive,jh7110-pmu
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  "#power-domain-cells":
>> +    const: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - "#power-domain-cells"
>> +
> 
> Best regards,
> Krzysztof
> 

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Walker Chen]

On 2022/12/8 16:59, Krzysztof Kozlowski wrote:
> On 08/12/2022 09:45, Walker Chen wrote:
>> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
>>  .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
>>  2 files changed, 62 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
> 
> Filename matching compatible, so:
> starfive,jh7110-pmu.yaml

OK, will be fixed.

> 
> 
>>  create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h
>> 
>> diff --git a/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
>> new file mode 100644
>> index 000000000000..f308ae136a57
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
>> @@ -0,0 +1,45 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/starfive,jh71xx-pmu.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive JH71xx Power Management Unit
>> +
>> +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:
>> +      - enum:
> 
> Wrong indentation.
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).

This is my mistake, I forgot to make dt_binding_check for .yaml file.

> 
>> +          - starfive,jh7110-pmu
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  "#power-domain-cells":
>> +    const: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - "#power-domain-cells"
>> +
> 
> Best regards,
> Krzysztof
> 


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

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Walker Chen]

On 2022/12/8 17:03, Krzysztof Kozlowski wrote:
> On 08/12/2022 09:45, Walker Chen wrote:
>> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> 
> 
> 
>> +    };
>> diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
>> new file mode 100644
>> index 000000000000..73c6a79a2181
>> --- /dev/null
>> +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> 
> Why different license than the bindings? MIT is pretty compatible, but
> if it does not matter for you, keep same licenses as bindings.

MIT should be the least restrictive license. But it seems that BSD is more used and more safe.
So both of them will be used same licenses in next version. Thanks.

> 
>> +/*
>> + * 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
>> +
>> +#endif
> 
> Best regards,
> Krzysztof
> 

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Walker Chen]

On 2022/12/8 17:03, Krzysztof Kozlowski wrote:
> On 08/12/2022 09:45, Walker Chen wrote:
>> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> 
> 
> 
>> +    };
>> diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
>> new file mode 100644
>> index 000000000000..73c6a79a2181
>> --- /dev/null
>> +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> 
> Why different license than the bindings? MIT is pretty compatible, but
> if it does not matter for you, keep same licenses as bindings.

MIT should be the least restrictive license. But it seems that BSD is more used and more safe.
So both of them will be used same licenses in next version. Thanks.

> 
>> +/*
>> + * 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
>> +
>> +#endif
> 
> Best regards,
> Krzysztof
> 


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

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Walker Chen]

On 2022/12/8 21:31, Rob Herring wrote:
> 
> On Thu, 08 Dec 2022 16:45:21 +0800, Walker Chen wrote:
>> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
>>  .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
>>  2 files changed, 62 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
>>  create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h
>> 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml:18:7: [warning] wrong indentation: expected 4 but found 6 (indentation)
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml: properties:compatible: [{'enum': ['starfive,jh7110-pmu']}] is not of type 'object', 'boolean'
> 	from schema $id: http://json-schema.org/draft-07/schema#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml: ignoring, error in schema: properties: compatible
> Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.example.dtb:0:0: /example-0/power-controller@17030000: failed to match any schema with compatible: ['starfive,jh7110-pmu']
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221208084523.9733-2-walker.chen@starfivetech.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 

Please forgive my mistake, I forgot to make dt_binding_check for .yaml file. 
Now there is no complain after syntax and indentation are fixed.
Thank you for your comment.

Best regards,
Walker Chen

Re: [RESEND PATCH v2 1/3] dt-bindings: power: Add starfive,jh71xx-pmu

[Walker Chen]

On 2022/12/8 21:31, Rob Herring wrote:
> 
> On Thu, 08 Dec 2022 16:45:21 +0800, Walker Chen wrote:
>> Add bindings for Power Management Unit (PMU) on the StarFive JH71XX SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  .../bindings/power/starfive,jh71xx-pmu.yaml   | 45 +++++++++++++++++++
>>  .../dt-bindings/power/starfive,jh7110-pmu.h   | 17 +++++++
>>  2 files changed, 62 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml
>>  create mode 100644 include/dt-bindings/power/starfive,jh7110-pmu.h
>> 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml:18:7: [warning] wrong indentation: expected 4 but found 6 (indentation)
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml: properties:compatible: [{'enum': ['starfive,jh7110-pmu']}] is not of type 'object', 'boolean'
> 	from schema $id: http://json-schema.org/draft-07/schema#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.yaml: ignoring, error in schema: properties: compatible
> Documentation/devicetree/bindings/power/starfive,jh71xx-pmu.example.dtb:0:0: /example-0/power-controller@17030000: failed to match any schema with compatible: ['starfive,jh7110-pmu']
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221208084523.9733-2-walker.chen@starfivetech.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 

Please forgive my mistake, I forgot to make dt_binding_check for .yaml file. 
Now there is no complain after syntax and indentation are fixed.
Thank you for your comment.

Best regards,
Walker Chen


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

Re: [RESEND PATCH v2 2/3] soc: starfive: Add StarFive JH71XX pmu driver

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 12426 bytes --]

Hey Walker,

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

On Thu, Dec 08, 2022 at 04:45:22PM +0800, Walker Chen wrote:
> Add pmu driver for the StarFive JH71XX SoC.
> 
> As the power domains provider, the Power Management Unit (PMU) is
> designed for including multiple PM domains that can be used for power
> gating of selected IP blocks for power saving by reduced leakage
> current. It accepts software encourage command to switch the power mode
> of SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  MAINTAINERS                       |  14 ++
>  drivers/soc/Kconfig               |   1 +
>  drivers/soc/Makefile              |   1 +
>  drivers/soc/starfive/Kconfig      |  11 +
>  drivers/soc/starfive/Makefile     |   3 +
>  drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
>  6 files changed, 426 insertions(+)
>  create mode 100644 drivers/soc/starfive/Kconfig
>  create mode 100644 drivers/soc/starfive/Makefile
>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
> 

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

Why not just do:
	depends on PM
	depends on SOC_STARFIVE || COMPILE_TEST
I think that way reads a little better.

> +	default SOC_STARFIVE
> +	select PM_GENERIC_DOMAINS
> +	help
> +	  Say 'y' here to enable support power domain support.
> +	  In order to meet low power requirements, a Power Management Unit (PMU)
> +	  is designed for controlling power resources in StarFive JH71XX SoCs.
> 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..7a0145779e07
> --- /dev/null
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -0,0 +1,396 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * StarFive JH71XX PMU (Power Management Unit) Controller Driver
> + *
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <dt-bindings/power/starfive,jh7110-pmu.h>
> +
> +/* register offset */
> +#define JH71XX_PMU_HW_EVENT_ON		0x04
> +#define JH71XX_PMU_HW_EVENT_OFF		0x08
> +#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
> +#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
> +#define JH71XX_PMU_SW_ENCOURAGE		0x44
> +#define JH71XX_PMU_INT_MASK		0x48
> +#define JH71XX_PMU_PCH_BYPASS		0x4C
> +#define JH71XX_PMU_PCH_PSTATE		0x50
> +#define JH71XX_PMU_PCH_TIMEOUT		0x54
> +#define JH71XX_PMU_LP_TIMEOUT		0x58
> +#define JH71XX_PMU_HW_TURN_ON		0x5C
> +#define JH71XX_PMU_CURR_POWER_MODE	0x80
> +#define JH71XX_PMU_EVENT_STATUS		0x88
> +#define JH71XX_PMU_INT_STATUS		0x8C
> +
> +/* sw encourage cfg */
> +#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
> +#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
> +#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF
> +
> +/* pmu int status */
> +#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
> +#define JH71XX_PMU_INT_HW_REQ		BIT(1)
> +#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
> +#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
> +#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
> +#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
> +					JH71XX_PMU_INT_HW_FAIL | \
> +					JH71XX_PMU_INT_PCH_FAIL)
> +#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \
> +					JH71XX_PMU_INT_HW_REQ | \
> +					JH71XX_PMU_INT_FAIL_MASK)
> +
> +/*
> + * The time required for switching power status is based on the time
> + * to turn on the largest domain's power, which is at microsecond level
> + */
> +#define JH71XX_PMU_TIMEOUT_US		100
> +
> +struct jh71xx_domain_info {

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

> +};
> +
> +struct jh71xx_pmu_match_data {

	const struct jh71xx_domain_info *domain_info;
	int num_domains;

Can you switch these two around like so?
> +};
> +
> +struct jh71xx_pmu {
> +	struct device *dev;
> +	const struct jh71xx_pmu_match_data *match_data;
> +	void __iomem *base;
> +	spinlock_t lock;	/* protects pmu reg */
> +	int irq;
> +	struct genpd_onecell_data genpd_data;
> +	struct generic_pm_domain **genpd;
> +};
> +
> +struct jh71xx_pmu_dev {
> +	struct generic_pm_domain genpd;
> +	const struct jh71xx_domain_info *domain_info;
> +	struct jh71xx_pmu *pmu;

And these two too please in the same way.

> +};
> +
> +static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
> +{
> +	struct jh71xx_pmu *pmu = pmd->pmu;
> +
> +	if (!mask) {
> +		*is_on = false;
> +		return -EINVAL;
> +	}
> +
> +	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
> +
> +	return 0;
> +}
> +
> +static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
> +{
> +	struct jh71xx_pmu *pmu = pmd->pmu;
> +	unsigned long flags;
> +	u32 val;
> +	u32 mode;
> +	u32 encourage_lo;
> +	u32 encourage_hi;
> +	bool is_on;
> +	int ret;
> +
> +	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
> +	if (ret) {
> +		dev_dbg(pmu->dev, "unable to get current state for %s\n",
> +			pmd->genpd.name);
> +		return ret;
> +	}
> +
> +	if (is_on == on) {
> +		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
> +			pmd->genpd.name, on ? "en" : "dis");
> +		return 0;
> +	}
> +
> +	spin_lock_irqsave(&pmu->lock, flags);
> +
> +	/*
> +	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
> +	 *
> +	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
> +	 *    the bit which power domain will be turn-on, write 0 to the others.

Some grammatical nit picking..

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

Is that a correct correct summation of the operation?

> +	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
> +	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.


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

I think you could replace the hard "coded" numbers here with a better
description idk without looking at the #defines above what these
correspond to. AFAICT, it'd be something like:
First write the ...ENCOURAGE_ON to reset the state machine which parses
the command sequence. It must be written every time.
Then write the lower bits of the command sequence, followed by the upper
bits. The sequence differs between powering on & off a domain.
> +	 *
> +	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
> +	 * the purpose is to reset the state machine which is going to parse instruction
> +	 *  sequence. It has to be written every time.
> +	 */
> +	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> +	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> +	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	/* Wait for the power domain bit to be enabled / disabled */
> +	if (on) {
> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
> +						val, val & mask,
> +						1, JH71XX_PMU_TIMEOUT_US);
> +	} else {
> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
> +						val, !(val & mask),
> +						1, JH71XX_PMU_TIMEOUT_US);
> +	}
> +
> +	if (ret) {
> +		dev_err(pmu->dev, "%s: failed to power %s\n",
> +			pmd->genpd.name, on ? "on" : "off");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}

> +static int jh71xx_pmu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	const struct jh71xx_pmu_match_data *match_data;
> +	struct jh71xx_pmu *pmu;
> +	unsigned int i;
> +	int ret;
> +
> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> +	if (!pmu)
> +		return -ENOMEM;
> +
> +	pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pmu->base))
> +		return PTR_ERR(pmu->base);
> +
> +	/* initialize pmu interrupt  */

nit: this comment is ~pointless.

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

nit: s/request/requesting

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

Thanks,
Conor.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND PATCH v2 2/3] soc: starfive: Add StarFive JH71XX pmu driver

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 12426 bytes --]

Hey Walker,

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

On Thu, Dec 08, 2022 at 04:45:22PM +0800, Walker Chen wrote:
> Add pmu driver for the StarFive JH71XX SoC.
> 
> As the power domains provider, the Power Management Unit (PMU) is
> designed for including multiple PM domains that can be used for power
> gating of selected IP blocks for power saving by reduced leakage
> current. It accepts software encourage command to switch the power mode
> of SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  MAINTAINERS                       |  14 ++
>  drivers/soc/Kconfig               |   1 +
>  drivers/soc/Makefile              |   1 +
>  drivers/soc/starfive/Kconfig      |  11 +
>  drivers/soc/starfive/Makefile     |   3 +
>  drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
>  6 files changed, 426 insertions(+)
>  create mode 100644 drivers/soc/starfive/Kconfig
>  create mode 100644 drivers/soc/starfive/Makefile
>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
> 

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

Why not just do:
	depends on PM
	depends on SOC_STARFIVE || COMPILE_TEST
I think that way reads a little better.

> +	default SOC_STARFIVE
> +	select PM_GENERIC_DOMAINS
> +	help
> +	  Say 'y' here to enable support power domain support.
> +	  In order to meet low power requirements, a Power Management Unit (PMU)
> +	  is designed for controlling power resources in StarFive JH71XX SoCs.
> 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..7a0145779e07
> --- /dev/null
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -0,0 +1,396 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * StarFive JH71XX PMU (Power Management Unit) Controller Driver
> + *
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <dt-bindings/power/starfive,jh7110-pmu.h>
> +
> +/* register offset */
> +#define JH71XX_PMU_HW_EVENT_ON		0x04
> +#define JH71XX_PMU_HW_EVENT_OFF		0x08
> +#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
> +#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
> +#define JH71XX_PMU_SW_ENCOURAGE		0x44
> +#define JH71XX_PMU_INT_MASK		0x48
> +#define JH71XX_PMU_PCH_BYPASS		0x4C
> +#define JH71XX_PMU_PCH_PSTATE		0x50
> +#define JH71XX_PMU_PCH_TIMEOUT		0x54
> +#define JH71XX_PMU_LP_TIMEOUT		0x58
> +#define JH71XX_PMU_HW_TURN_ON		0x5C
> +#define JH71XX_PMU_CURR_POWER_MODE	0x80
> +#define JH71XX_PMU_EVENT_STATUS		0x88
> +#define JH71XX_PMU_INT_STATUS		0x8C
> +
> +/* sw encourage cfg */
> +#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
> +#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
> +#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF
> +
> +/* pmu int status */
> +#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
> +#define JH71XX_PMU_INT_HW_REQ		BIT(1)
> +#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
> +#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
> +#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
> +#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
> +					JH71XX_PMU_INT_HW_FAIL | \
> +					JH71XX_PMU_INT_PCH_FAIL)
> +#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \
> +					JH71XX_PMU_INT_HW_REQ | \
> +					JH71XX_PMU_INT_FAIL_MASK)
> +
> +/*
> + * The time required for switching power status is based on the time
> + * to turn on the largest domain's power, which is at microsecond level
> + */
> +#define JH71XX_PMU_TIMEOUT_US		100
> +
> +struct jh71xx_domain_info {

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

> +};
> +
> +struct jh71xx_pmu_match_data {

	const struct jh71xx_domain_info *domain_info;
	int num_domains;

Can you switch these two around like so?
> +};
> +
> +struct jh71xx_pmu {
> +	struct device *dev;
> +	const struct jh71xx_pmu_match_data *match_data;
> +	void __iomem *base;
> +	spinlock_t lock;	/* protects pmu reg */
> +	int irq;
> +	struct genpd_onecell_data genpd_data;
> +	struct generic_pm_domain **genpd;
> +};
> +
> +struct jh71xx_pmu_dev {
> +	struct generic_pm_domain genpd;
> +	const struct jh71xx_domain_info *domain_info;
> +	struct jh71xx_pmu *pmu;

And these two too please in the same way.

> +};
> +
> +static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
> +{
> +	struct jh71xx_pmu *pmu = pmd->pmu;
> +
> +	if (!mask) {
> +		*is_on = false;
> +		return -EINVAL;
> +	}
> +
> +	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
> +
> +	return 0;
> +}
> +
> +static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
> +{
> +	struct jh71xx_pmu *pmu = pmd->pmu;
> +	unsigned long flags;
> +	u32 val;
> +	u32 mode;
> +	u32 encourage_lo;
> +	u32 encourage_hi;
> +	bool is_on;
> +	int ret;
> +
> +	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
> +	if (ret) {
> +		dev_dbg(pmu->dev, "unable to get current state for %s\n",
> +			pmd->genpd.name);
> +		return ret;
> +	}
> +
> +	if (is_on == on) {
> +		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
> +			pmd->genpd.name, on ? "en" : "dis");
> +		return 0;
> +	}
> +
> +	spin_lock_irqsave(&pmu->lock, flags);
> +
> +	/*
> +	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
> +	 *
> +	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
> +	 *    the bit which power domain will be turn-on, write 0 to the others.

Some grammatical nit picking..

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

Is that a correct correct summation of the operation?

> +	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
> +	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.


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

I think you could replace the hard "coded" numbers here with a better
description idk without looking at the #defines above what these
correspond to. AFAICT, it'd be something like:
First write the ...ENCOURAGE_ON to reset the state machine which parses
the command sequence. It must be written every time.
Then write the lower bits of the command sequence, followed by the upper
bits. The sequence differs between powering on & off a domain.
> +	 *
> +	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
> +	 * the purpose is to reset the state machine which is going to parse instruction
> +	 *  sequence. It has to be written every time.
> +	 */
> +	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> +	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> +	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	/* Wait for the power domain bit to be enabled / disabled */
> +	if (on) {
> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
> +						val, val & mask,
> +						1, JH71XX_PMU_TIMEOUT_US);
> +	} else {
> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
> +						val, !(val & mask),
> +						1, JH71XX_PMU_TIMEOUT_US);
> +	}
> +
> +	if (ret) {
> +		dev_err(pmu->dev, "%s: failed to power %s\n",
> +			pmd->genpd.name, on ? "on" : "off");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}

> +static int jh71xx_pmu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	const struct jh71xx_pmu_match_data *match_data;
> +	struct jh71xx_pmu *pmu;
> +	unsigned int i;
> +	int ret;
> +
> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> +	if (!pmu)
> +		return -ENOMEM;
> +
> +	pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pmu->base))
> +		return PTR_ERR(pmu->base);
> +
> +	/* initialize pmu interrupt  */

nit: this comment is ~pointless.

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

nit: s/request/requesting

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

Thanks,
Conor.

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

Re: [RESEND PATCH v2 2/3] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/12/20 4:40, Conor Dooley wrote:
> Hey Walker,
> 
> Hopefully just some minor bits here. Hopefully either Emil who has a
> board, or someone that knows power management stuff better can give this
> a proper review.
> 
> On Thu, Dec 08, 2022 at 04:45:22PM +0800, Walker Chen wrote:
>> Add pmu driver for the StarFive JH71XX SoC.
>> 
>> As the power domains provider, the Power Management Unit (PMU) is
>> designed for including multiple PM domains that can be used for power
>> gating of selected IP blocks for power saving by reduced leakage
>> current. It accepts software encourage command to switch the power mode
>> of SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  MAINTAINERS                       |  14 ++
>>  drivers/soc/Kconfig               |   1 +
>>  drivers/soc/Makefile              |   1 +
>>  drivers/soc/starfive/Kconfig      |  11 +
>>  drivers/soc/starfive/Makefile     |   3 +
>>  drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
>>  6 files changed, 426 insertions(+)
>>  create mode 100644 drivers/soc/starfive/Kconfig
>>  create mode 100644 drivers/soc/starfive/Makefile
>>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
>> 
> 
>> +config JH71XX_PMU
>> +	bool "Support PMU for StarFive JH71XX Soc"
>> +	depends on PM && (SOC_STARFIVE || COMPILE_TEST)
> 
> Why not just do:
> 	depends on PM
> 	depends on SOC_STARFIVE || COMPILE_TEST
> I think that way reads a little better.

No problem, will be changed like this way.

> 
>> +	default SOC_STARFIVE
>> +	select PM_GENERIC_DOMAINS
>> +	help
>> +	  Say 'y' here to enable support power domain support.
>> +	  In order to meet low power requirements, a Power Management Unit (PMU)
>> +	  is designed for controlling power resources in StarFive JH71XX SoCs.
>> 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..7a0145779e07
>> --- /dev/null
>> +++ b/drivers/soc/starfive/jh71xx_pmu.c
>> @@ -0,0 +1,396 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * StarFive JH71XX PMU (Power Management Unit) Controller Driver
>> + *
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_domain.h>
>> +#include <dt-bindings/power/starfive,jh7110-pmu.h>
>> +
>> +/* register offset */
>> +#define JH71XX_PMU_HW_EVENT_ON		0x04
>> +#define JH71XX_PMU_HW_EVENT_OFF		0x08
>> +#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
>> +#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
>> +#define JH71XX_PMU_SW_ENCOURAGE		0x44
>> +#define JH71XX_PMU_INT_MASK		0x48
>> +#define JH71XX_PMU_PCH_BYPASS		0x4C
>> +#define JH71XX_PMU_PCH_PSTATE		0x50
>> +#define JH71XX_PMU_PCH_TIMEOUT		0x54
>> +#define JH71XX_PMU_LP_TIMEOUT		0x58
>> +#define JH71XX_PMU_HW_TURN_ON		0x5C
>> +#define JH71XX_PMU_CURR_POWER_MODE	0x80
>> +#define JH71XX_PMU_EVENT_STATUS		0x88
>> +#define JH71XX_PMU_INT_STATUS		0x8C
>> +
>> +/* sw encourage cfg */
>> +#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
>> +#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
>> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
>> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
>> +#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF
>> +
>> +/* pmu int status */
>> +#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
>> +#define JH71XX_PMU_INT_HW_REQ		BIT(1)
>> +#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
>> +#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
>> +#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
>> +#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
>> +					JH71XX_PMU_INT_HW_FAIL | \
>> +					JH71XX_PMU_INT_PCH_FAIL)
>> +#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \
>> +					JH71XX_PMU_INT_HW_REQ | \
>> +					JH71XX_PMU_INT_FAIL_MASK)
>> +
>> +/*
>> + * The time required for switching power status is based on the time
>> + * to turn on the largest domain's power, which is at microsecond level
>> + */
>> +#define JH71XX_PMU_TIMEOUT_US		100
>> +
>> +struct jh71xx_domain_info {
> 
> 	const char * const name;
> 	unsigned int flags;
> 	u8 bit;
> 
>> +};
>> +
>> +struct jh71xx_pmu_match_data {
> 
> 	const struct jh71xx_domain_info *domain_info;
> 	int num_domains;
> 
> Can you switch these two around like so?

Should be no problem.

>> +};
>> +
>> +struct jh71xx_pmu {
>> +	struct device *dev;
>> +	const struct jh71xx_pmu_match_data *match_data;
>> +	void __iomem *base;
>> +	spinlock_t lock;	/* protects pmu reg */
>> +	int irq;
>> +	struct genpd_onecell_data genpd_data;
>> +	struct generic_pm_domain **genpd;
>> +};
>> +
>> +struct jh71xx_pmu_dev {
>> +	struct generic_pm_domain genpd;
>> +	const struct jh71xx_domain_info *domain_info;
>> +	struct jh71xx_pmu *pmu;
> 
> And these two too please in the same way.

Nice :)

> 
>> +};
>> +
>> +static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
>> +{
>> +	struct jh71xx_pmu *pmu = pmd->pmu;
>> +
>> +	if (!mask) {
>> +		*is_on = false;
>> +		return -EINVAL;
>> +	}
>> +
>> +	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
>> +
>> +	return 0;
>> +}
>> +
>> +static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
>> +{
>> +	struct jh71xx_pmu *pmu = pmd->pmu;
>> +	unsigned long flags;
>> +	u32 val;
>> +	u32 mode;
>> +	u32 encourage_lo;
>> +	u32 encourage_hi;
>> +	bool is_on;
>> +	int ret;
>> +
>> +	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
>> +	if (ret) {
>> +		dev_dbg(pmu->dev, "unable to get current state for %s\n",
>> +			pmd->genpd.name);
>> +		return ret;
>> +	}
>> +
>> +	if (is_on == on) {
>> +		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
>> +			pmd->genpd.name, on ? "en" : "dis");
>> +		return 0;
>> +	}
>> +
>> +	spin_lock_irqsave(&pmu->lock, flags);
>> +
>> +	/*
>> +	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
>> +	 *
>> +	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
>> +	 *    the bit which power domain will be turn-on, write 0 to the others.
> 
> Some grammatical nit picking..
> 
> "Configure the register blah (offset 0x0c) by writing 1 to the bit
> corresponding to the power domain that will be turned on and writing
> zero to the others."
> 
> Is that a correct correct summation of the operation?

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

> 
>> +	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
>> +	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
> 
> 
>> +	 */
>> +	if (on) {
>> +		mode = JH71XX_PMU_SW_TURN_ON_POWER;
>> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
>> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
>> +	} else {
>> +		mode = JH71XX_PMU_SW_TURN_OFF_POWER;
>> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
>> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
>> +	}
>> +
>> +	writel(mask, pmu->base + mode);
>> +
>> +	/*
>> +	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
>> +	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
>> +	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0
> 
> I think you could replace the hard "coded" numbers here with a better
> description idk without looking at the #defines above what these
> correspond to. AFAICT, it'd be something like:
> First write the ...ENCOURAGE_ON to reset the state machine which parses
> the command sequence. It must be written every time.
> Then write the lower bits of the command sequence, followed by the upper
> bits. The sequence differs between powering on & off a domain.

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

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

Will be dropped.

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

Will be fixed.

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

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


Best regards,
Walker

Re: [RESEND PATCH v2 2/3] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/12/20 4:40, Conor Dooley wrote:
> Hey Walker,
> 
> Hopefully just some minor bits here. Hopefully either Emil who has a
> board, or someone that knows power management stuff better can give this
> a proper review.
> 
> On Thu, Dec 08, 2022 at 04:45:22PM +0800, Walker Chen wrote:
>> Add pmu driver for the StarFive JH71XX SoC.
>> 
>> As the power domains provider, the Power Management Unit (PMU) is
>> designed for including multiple PM domains that can be used for power
>> gating of selected IP blocks for power saving by reduced leakage
>> current. It accepts software encourage command to switch the power mode
>> of SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> ---
>>  MAINTAINERS                       |  14 ++
>>  drivers/soc/Kconfig               |   1 +
>>  drivers/soc/Makefile              |   1 +
>>  drivers/soc/starfive/Kconfig      |  11 +
>>  drivers/soc/starfive/Makefile     |   3 +
>>  drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
>>  6 files changed, 426 insertions(+)
>>  create mode 100644 drivers/soc/starfive/Kconfig
>>  create mode 100644 drivers/soc/starfive/Makefile
>>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
>> 
> 
>> +config JH71XX_PMU
>> +	bool "Support PMU for StarFive JH71XX Soc"
>> +	depends on PM && (SOC_STARFIVE || COMPILE_TEST)
> 
> Why not just do:
> 	depends on PM
> 	depends on SOC_STARFIVE || COMPILE_TEST
> I think that way reads a little better.

No problem, will be changed like this way.

> 
>> +	default SOC_STARFIVE
>> +	select PM_GENERIC_DOMAINS
>> +	help
>> +	  Say 'y' here to enable support power domain support.
>> +	  In order to meet low power requirements, a Power Management Unit (PMU)
>> +	  is designed for controlling power resources in StarFive JH71XX SoCs.
>> 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..7a0145779e07
>> --- /dev/null
>> +++ b/drivers/soc/starfive/jh71xx_pmu.c
>> @@ -0,0 +1,396 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * StarFive JH71XX PMU (Power Management Unit) Controller Driver
>> + *
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_domain.h>
>> +#include <dt-bindings/power/starfive,jh7110-pmu.h>
>> +
>> +/* register offset */
>> +#define JH71XX_PMU_HW_EVENT_ON		0x04
>> +#define JH71XX_PMU_HW_EVENT_OFF		0x08
>> +#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
>> +#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
>> +#define JH71XX_PMU_SW_ENCOURAGE		0x44
>> +#define JH71XX_PMU_INT_MASK		0x48
>> +#define JH71XX_PMU_PCH_BYPASS		0x4C
>> +#define JH71XX_PMU_PCH_PSTATE		0x50
>> +#define JH71XX_PMU_PCH_TIMEOUT		0x54
>> +#define JH71XX_PMU_LP_TIMEOUT		0x58
>> +#define JH71XX_PMU_HW_TURN_ON		0x5C
>> +#define JH71XX_PMU_CURR_POWER_MODE	0x80
>> +#define JH71XX_PMU_EVENT_STATUS		0x88
>> +#define JH71XX_PMU_INT_STATUS		0x8C
>> +
>> +/* sw encourage cfg */
>> +#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
>> +#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
>> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
>> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
>> +#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF
>> +
>> +/* pmu int status */
>> +#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
>> +#define JH71XX_PMU_INT_HW_REQ		BIT(1)
>> +#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
>> +#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
>> +#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
>> +#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
>> +					JH71XX_PMU_INT_HW_FAIL | \
>> +					JH71XX_PMU_INT_PCH_FAIL)
>> +#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \
>> +					JH71XX_PMU_INT_HW_REQ | \
>> +					JH71XX_PMU_INT_FAIL_MASK)
>> +
>> +/*
>> + * The time required for switching power status is based on the time
>> + * to turn on the largest domain's power, which is at microsecond level
>> + */
>> +#define JH71XX_PMU_TIMEOUT_US		100
>> +
>> +struct jh71xx_domain_info {
> 
> 	const char * const name;
> 	unsigned int flags;
> 	u8 bit;
> 
>> +};
>> +
>> +struct jh71xx_pmu_match_data {
> 
> 	const struct jh71xx_domain_info *domain_info;
> 	int num_domains;
> 
> Can you switch these two around like so?

Should be no problem.

>> +};
>> +
>> +struct jh71xx_pmu {
>> +	struct device *dev;
>> +	const struct jh71xx_pmu_match_data *match_data;
>> +	void __iomem *base;
>> +	spinlock_t lock;	/* protects pmu reg */
>> +	int irq;
>> +	struct genpd_onecell_data genpd_data;
>> +	struct generic_pm_domain **genpd;
>> +};
>> +
>> +struct jh71xx_pmu_dev {
>> +	struct generic_pm_domain genpd;
>> +	const struct jh71xx_domain_info *domain_info;
>> +	struct jh71xx_pmu *pmu;
> 
> And these two too please in the same way.

Nice :)

> 
>> +};
>> +
>> +static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
>> +{
>> +	struct jh71xx_pmu *pmu = pmd->pmu;
>> +
>> +	if (!mask) {
>> +		*is_on = false;
>> +		return -EINVAL;
>> +	}
>> +
>> +	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
>> +
>> +	return 0;
>> +}
>> +
>> +static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
>> +{
>> +	struct jh71xx_pmu *pmu = pmd->pmu;
>> +	unsigned long flags;
>> +	u32 val;
>> +	u32 mode;
>> +	u32 encourage_lo;
>> +	u32 encourage_hi;
>> +	bool is_on;
>> +	int ret;
>> +
>> +	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
>> +	if (ret) {
>> +		dev_dbg(pmu->dev, "unable to get current state for %s\n",
>> +			pmd->genpd.name);
>> +		return ret;
>> +	}
>> +
>> +	if (is_on == on) {
>> +		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
>> +			pmd->genpd.name, on ? "en" : "dis");
>> +		return 0;
>> +	}
>> +
>> +	spin_lock_irqsave(&pmu->lock, flags);
>> +
>> +	/*
>> +	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
>> +	 *
>> +	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
>> +	 *    the bit which power domain will be turn-on, write 0 to the others.
> 
> Some grammatical nit picking..
> 
> "Configure the register blah (offset 0x0c) by writing 1 to the bit
> corresponding to the power domain that will be turned on and writing
> zero to the others."
> 
> Is that a correct correct summation of the operation?

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

> 
>> +	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
>> +	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
> 
> 
>> +	 */
>> +	if (on) {
>> +		mode = JH71XX_PMU_SW_TURN_ON_POWER;
>> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
>> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
>> +	} else {
>> +		mode = JH71XX_PMU_SW_TURN_OFF_POWER;
>> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
>> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
>> +	}
>> +
>> +	writel(mask, pmu->base + mode);
>> +
>> +	/*
>> +	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
>> +	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
>> +	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0
> 
> I think you could replace the hard "coded" numbers here with a better
> description idk without looking at the #defines above what these
> correspond to. AFAICT, it'd be something like:
> First write the ...ENCOURAGE_ON to reset the state machine which parses
> the command sequence. It must be written every time.
> Then write the lower bits of the command sequence, followed by the upper
> bits. The sequence differs between powering on & off a domain.

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

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

Will be dropped.

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

Will be fixed.

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

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


Best regards,
Walker


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

Re: [RESEND PATCH v2 2/3] soc: starfive: Add StarFive JH71XX pmu driver

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 13349 bytes --]

Hey Walker,
Took another bit of a look.

On Wed, Dec 28, 2022 at 10:08:55AM +0800, Walker Chen wrote:
> On 2022/12/20 4:40, Conor Dooley wrote:
> > Hey Walker,
> > 
> > Hopefully just some minor bits here. Hopefully either Emil who has a
> > board, or someone that knows power management stuff better can give this
> > a proper review.
> > 
> > On Thu, Dec 08, 2022 at 04:45:22PM +0800, Walker Chen wrote:
> >> Add pmu driver for the StarFive JH71XX SoC.
> >> 
> >> As the power domains provider, the Power Management Unit (PMU) is
> >> designed for including multiple PM domains that can be used for power
> >> gating of selected IP blocks for power saving by reduced leakage
> >> current. It accepts software encourage command to switch the power mode
> >> of SoC.
> >> 
> >> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> >> ---
> >>  MAINTAINERS                       |  14 ++
> >>  drivers/soc/Kconfig               |   1 +
> >>  drivers/soc/Makefile              |   1 +
> >>  drivers/soc/starfive/Kconfig      |  11 +
> >>  drivers/soc/starfive/Makefile     |   3 +
> >>  drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
> >>  6 files changed, 426 insertions(+)
> >>  create mode 100644 drivers/soc/starfive/Kconfig
> >>  create mode 100644 drivers/soc/starfive/Makefile
> >>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
> >> 
> > 
> >> +config JH71XX_PMU
> >> +	bool "Support PMU for StarFive JH71XX Soc"
> >> +	depends on PM && (SOC_STARFIVE || COMPILE_TEST)
> > 
> > Why not just do:
> > 	depends on PM
> > 	depends on SOC_STARFIVE || COMPILE_TEST
> > I think that way reads a little better.
> 
> No problem, will be changed like this way.
> 
> > 
> >> +	default SOC_STARFIVE
> >> +	select PM_GENERIC_DOMAINS
> >> +	help
> >> +	  Say 'y' here to enable support power domain support.
> >> +	  In order to meet low power requirements, a Power Management Unit (PMU)
> >> +	  is designed for controlling power resources in StarFive JH71XX SoCs.
> >> 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..7a0145779e07
> >> --- /dev/null
> >> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> >> @@ -0,0 +1,396 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * StarFive JH71XX PMU (Power Management Unit) Controller Driver
> >> + *
> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> >> + */
> >> +
> >> +#include <linux/interrupt.h>
> >> +#include <linux/io.h>
> >> +#include <linux/iopoll.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_domain.h>
> >> +#include <dt-bindings/power/starfive,jh7110-pmu.h>
> >> +
> >> +/* register offset */
> >> +#define JH71XX_PMU_HW_EVENT_ON		0x04
> >> +#define JH71XX_PMU_HW_EVENT_OFF		0x08

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

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

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

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

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

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

This all seems to correspond w/ docs...

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

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

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

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

> >> + */
> >> +#define JH71XX_PMU_TIMEOUT_US		100

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

> >> +struct jh71xx_domain_info {
> > 
> > 	const char * const name;
> > 	unsigned int flags;
> > 	u8 bit;
> > 
> >> +};
> >> +
> >> +struct jh71xx_pmu_match_data {
> > 
> > 	const struct jh71xx_domain_info *domain_info;
> > 	int num_domains;
> > 
> > Can you switch these two around like so?
> 
> Should be no problem.
> 
> >> +};
> >> +
> >> +struct jh71xx_pmu {
> >> +	struct device *dev;
> >> +	const struct jh71xx_pmu_match_data *match_data;
> >> +	void __iomem *base;
> >> +	spinlock_t lock;	/* protects pmu reg */
> >> +	int irq;
> >> +	struct genpd_onecell_data genpd_data;
> >> +	struct generic_pm_domain **genpd;
> >> +};
> >> +
> >> +struct jh71xx_pmu_dev {
> >> +	struct generic_pm_domain genpd;
> >> +	const struct jh71xx_domain_info *domain_info;
> >> +	struct jh71xx_pmu *pmu;
> > 
> > And these two too please in the same way.
> 
> Nice :)
> 
> > 
> >> +};
> >> +
> >> +static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
> >> +{
> >> +	struct jh71xx_pmu *pmu = pmd->pmu;
> >> +
> >> +	if (!mask) {
> >> +		*is_on = false;
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
> >> +{
> >> +	struct jh71xx_pmu *pmu = pmd->pmu;
> >> +	unsigned long flags;
> >> +	u32 val;
> >> +	u32 mode;
> >> +	u32 encourage_lo;
> >> +	u32 encourage_hi;
> >> +	bool is_on;
> >> +	int ret;
> >> +
> >> +	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
> >> +	if (ret) {
> >> +		dev_dbg(pmu->dev, "unable to get current state for %s\n",
> >> +			pmd->genpd.name);
> >> +		return ret;
> >> +	}
> >> +
> >> +	if (is_on == on) {
> >> +		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
> >> +			pmd->genpd.name, on ? "en" : "dis");
> >> +		return 0;
> >> +	}
> >> +
> >> +	spin_lock_irqsave(&pmu->lock, flags);
> >> +
> >> +	/*
> >> +	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
> >> +	 *
> >> +	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
> >> +	 *    the bit which power domain will be turn-on, write 0 to the others.
> > 
> > Some grammatical nit picking..
> > 
> > "Configure the register blah (offset 0x0c) by writing 1 to the bit
> > corresponding to the power domain that will be turned on and writing
> > zero to the others."
> > 
> > Is that a correct correct summation of the operation?
> 
> Yes, maybe your description is better easy-to-understand.
> 
> > 
> >> +	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
> >> +	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
> > 
> > 
> >> +	 */
> >> +	if (on) {
> >> +		mode = JH71XX_PMU_SW_TURN_ON_POWER;
> >> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
> >> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
> >> +	} else {
> >> +		mode = JH71XX_PMU_SW_TURN_OFF_POWER;
> >> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
> >> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
> >> +	}
> >> +
> >> +	writel(mask, pmu->base + mode);
> >> +
> >> +	/*
> >> +	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
> >> +	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
> >> +	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0
> > 
> > I think you could replace the hard "coded" numbers here with a better
> > description idk without looking at the #defines above what these
> > correspond to. AFAICT, it'd be something like:
> > First write the ...ENCOURAGE_ON to reset the state machine which parses
> > the command sequence. It must be written every time.
> > Then write the lower bits of the command sequence, followed by the upper
> > bits. The sequence differs between powering on & off a domain.
> 
> Thank you for correction for these description. Because English is not my native language,
> so not very good in some sentences. I'll take your advice.

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

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

> >> +	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
> >> +	 * the purpose is to reset the state machine which is going to parse instruction
> >> +	 *  sequence. It has to be written every time.
> >> +	 */
> >> +	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> >> +	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> >> +	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> >> +
> >> +	spin_unlock_irqrestore(&pmu->lock, flags);
> >> +
> >> +	/* Wait for the power domain bit to be enabled / disabled */
> >> +	if (on) {
> >> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
> >> +						val, val & mask,
> >> +						1, JH71XX_PMU_TIMEOUT_US);
> >> +	} else {
> >> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
> >> +						val, !(val & mask),
> >> +						1, JH71XX_PMU_TIMEOUT_US);
> >> +	}
> >> +
> >> +	if (ret) {
> >> +		dev_err(pmu->dev, "%s: failed to power %s\n",
> >> +			pmd->genpd.name, on ? "on" : "off");
> >> +		return -ETIMEDOUT;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> > 
> >> +static int jh71xx_pmu_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +	struct device_node *np = dev->of_node;
> >> +	const struct jh71xx_pmu_match_data *match_data;
> >> +	struct jh71xx_pmu *pmu;
> >> +	unsigned int i;
> >> +	int ret;
> >> +
> >> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> >> +	if (!pmu)
> >> +		return -ENOMEM;
> >> +
> >> +	pmu->base = devm_platform_ioremap_resource(pdev, 0);
> >> +	if (IS_ERR(pmu->base))
> >> +		return PTR_ERR(pmu->base);
> >> +
> >> +	/* initialize pmu interrupt  */
> > 
> > nit: this comment is ~pointless.
> 
> Will be dropped.
> 
> > 
> >> +	pmu->irq = platform_get_irq(pdev, 0);
> >> +	if (pmu->irq < 0)
> >> +		return pmu->irq;
> >> +
> >> +	ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
> >> +			       0, pdev->name, pmu);
> >> +	if (ret)
> >> +		dev_err(dev, "request irq failed.\n");
> > 
> > nit: s/request/requesting
> 
> Will be fixed.
> 
> > 
> > Unfortunately I cannot really review the rest of this, but hopefully
> > I'll get a board soon and can try it out - or else send me a link to
> > your TRM or w/e.
> 
> Anyway, I would like to thank you very much for your time.

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

Thanks,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND PATCH v2 2/3] soc: starfive: Add StarFive JH71XX pmu driver

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 13349 bytes --]

Hey Walker,
Took another bit of a look.

On Wed, Dec 28, 2022 at 10:08:55AM +0800, Walker Chen wrote:
> On 2022/12/20 4:40, Conor Dooley wrote:
> > Hey Walker,
> > 
> > Hopefully just some minor bits here. Hopefully either Emil who has a
> > board, or someone that knows power management stuff better can give this
> > a proper review.
> > 
> > On Thu, Dec 08, 2022 at 04:45:22PM +0800, Walker Chen wrote:
> >> Add pmu driver for the StarFive JH71XX SoC.
> >> 
> >> As the power domains provider, the Power Management Unit (PMU) is
> >> designed for including multiple PM domains that can be used for power
> >> gating of selected IP blocks for power saving by reduced leakage
> >> current. It accepts software encourage command to switch the power mode
> >> of SoC.
> >> 
> >> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> >> ---
> >>  MAINTAINERS                       |  14 ++
> >>  drivers/soc/Kconfig               |   1 +
> >>  drivers/soc/Makefile              |   1 +
> >>  drivers/soc/starfive/Kconfig      |  11 +
> >>  drivers/soc/starfive/Makefile     |   3 +
> >>  drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
> >>  6 files changed, 426 insertions(+)
> >>  create mode 100644 drivers/soc/starfive/Kconfig
> >>  create mode 100644 drivers/soc/starfive/Makefile
> >>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
> >> 
> > 
> >> +config JH71XX_PMU
> >> +	bool "Support PMU for StarFive JH71XX Soc"
> >> +	depends on PM && (SOC_STARFIVE || COMPILE_TEST)
> > 
> > Why not just do:
> > 	depends on PM
> > 	depends on SOC_STARFIVE || COMPILE_TEST
> > I think that way reads a little better.
> 
> No problem, will be changed like this way.
> 
> > 
> >> +	default SOC_STARFIVE
> >> +	select PM_GENERIC_DOMAINS
> >> +	help
> >> +	  Say 'y' here to enable support power domain support.
> >> +	  In order to meet low power requirements, a Power Management Unit (PMU)
> >> +	  is designed for controlling power resources in StarFive JH71XX SoCs.
> >> 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..7a0145779e07
> >> --- /dev/null
> >> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> >> @@ -0,0 +1,396 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * StarFive JH71XX PMU (Power Management Unit) Controller Driver
> >> + *
> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> >> + */
> >> +
> >> +#include <linux/interrupt.h>
> >> +#include <linux/io.h>
> >> +#include <linux/iopoll.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_domain.h>
> >> +#include <dt-bindings/power/starfive,jh7110-pmu.h>
> >> +
> >> +/* register offset */
> >> +#define JH71XX_PMU_HW_EVENT_ON		0x04
> >> +#define JH71XX_PMU_HW_EVENT_OFF		0x08

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

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

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

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

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

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

This all seems to correspond w/ docs...

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

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

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

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

> >> + */
> >> +#define JH71XX_PMU_TIMEOUT_US		100

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

> >> +struct jh71xx_domain_info {
> > 
> > 	const char * const name;
> > 	unsigned int flags;
> > 	u8 bit;
> > 
> >> +};
> >> +
> >> +struct jh71xx_pmu_match_data {
> > 
> > 	const struct jh71xx_domain_info *domain_info;
> > 	int num_domains;
> > 
> > Can you switch these two around like so?
> 
> Should be no problem.
> 
> >> +};
> >> +
> >> +struct jh71xx_pmu {
> >> +	struct device *dev;
> >> +	const struct jh71xx_pmu_match_data *match_data;
> >> +	void __iomem *base;
> >> +	spinlock_t lock;	/* protects pmu reg */
> >> +	int irq;
> >> +	struct genpd_onecell_data genpd_data;
> >> +	struct generic_pm_domain **genpd;
> >> +};
> >> +
> >> +struct jh71xx_pmu_dev {
> >> +	struct generic_pm_domain genpd;
> >> +	const struct jh71xx_domain_info *domain_info;
> >> +	struct jh71xx_pmu *pmu;
> > 
> > And these two too please in the same way.
> 
> Nice :)
> 
> > 
> >> +};
> >> +
> >> +static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
> >> +{
> >> +	struct jh71xx_pmu *pmu = pmd->pmu;
> >> +
> >> +	if (!mask) {
> >> +		*is_on = false;
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
> >> +{
> >> +	struct jh71xx_pmu *pmu = pmd->pmu;
> >> +	unsigned long flags;
> >> +	u32 val;
> >> +	u32 mode;
> >> +	u32 encourage_lo;
> >> +	u32 encourage_hi;
> >> +	bool is_on;
> >> +	int ret;
> >> +
> >> +	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
> >> +	if (ret) {
> >> +		dev_dbg(pmu->dev, "unable to get current state for %s\n",
> >> +			pmd->genpd.name);
> >> +		return ret;
> >> +	}
> >> +
> >> +	if (is_on == on) {
> >> +		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
> >> +			pmd->genpd.name, on ? "en" : "dis");
> >> +		return 0;
> >> +	}
> >> +
> >> +	spin_lock_irqsave(&pmu->lock, flags);
> >> +
> >> +	/*
> >> +	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
> >> +	 *
> >> +	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
> >> +	 *    the bit which power domain will be turn-on, write 0 to the others.
> > 
> > Some grammatical nit picking..
> > 
> > "Configure the register blah (offset 0x0c) by writing 1 to the bit
> > corresponding to the power domain that will be turned on and writing
> > zero to the others."
> > 
> > Is that a correct correct summation of the operation?
> 
> Yes, maybe your description is better easy-to-understand.
> 
> > 
> >> +	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
> >> +	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
> > 
> > 
> >> +	 */
> >> +	if (on) {
> >> +		mode = JH71XX_PMU_SW_TURN_ON_POWER;
> >> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
> >> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
> >> +	} else {
> >> +		mode = JH71XX_PMU_SW_TURN_OFF_POWER;
> >> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
> >> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
> >> +	}
> >> +
> >> +	writel(mask, pmu->base + mode);
> >> +
> >> +	/*
> >> +	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
> >> +	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
> >> +	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0
> > 
> > I think you could replace the hard "coded" numbers here with a better
> > description idk without looking at the #defines above what these
> > correspond to. AFAICT, it'd be something like:
> > First write the ...ENCOURAGE_ON to reset the state machine which parses
> > the command sequence. It must be written every time.
> > Then write the lower bits of the command sequence, followed by the upper
> > bits. The sequence differs between powering on & off a domain.
> 
> Thank you for correction for these description. Because English is not my native language,
> so not very good in some sentences. I'll take your advice.

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

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

> >> +	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
> >> +	 * the purpose is to reset the state machine which is going to parse instruction
> >> +	 *  sequence. It has to be written every time.
> >> +	 */
> >> +	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> >> +	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> >> +	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> >> +
> >> +	spin_unlock_irqrestore(&pmu->lock, flags);
> >> +
> >> +	/* Wait for the power domain bit to be enabled / disabled */
> >> +	if (on) {
> >> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
> >> +						val, val & mask,
> >> +						1, JH71XX_PMU_TIMEOUT_US);
> >> +	} else {
> >> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
> >> +						val, !(val & mask),
> >> +						1, JH71XX_PMU_TIMEOUT_US);
> >> +	}
> >> +
> >> +	if (ret) {
> >> +		dev_err(pmu->dev, "%s: failed to power %s\n",
> >> +			pmd->genpd.name, on ? "on" : "off");
> >> +		return -ETIMEDOUT;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> > 
> >> +static int jh71xx_pmu_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +	struct device_node *np = dev->of_node;
> >> +	const struct jh71xx_pmu_match_data *match_data;
> >> +	struct jh71xx_pmu *pmu;
> >> +	unsigned int i;
> >> +	int ret;
> >> +
> >> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> >> +	if (!pmu)
> >> +		return -ENOMEM;
> >> +
> >> +	pmu->base = devm_platform_ioremap_resource(pdev, 0);
> >> +	if (IS_ERR(pmu->base))
> >> +		return PTR_ERR(pmu->base);
> >> +
> >> +	/* initialize pmu interrupt  */
> > 
> > nit: this comment is ~pointless.
> 
> Will be dropped.
> 
> > 
> >> +	pmu->irq = platform_get_irq(pdev, 0);
> >> +	if (pmu->irq < 0)
> >> +		return pmu->irq;
> >> +
> >> +	ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
> >> +			       0, pdev->name, pmu);
> >> +	if (ret)
> >> +		dev_err(dev, "request irq failed.\n");
> > 
> > nit: s/request/requesting
> 
> Will be fixed.
> 
> > 
> > Unfortunately I cannot really review the rest of this, but hopefully
> > I'll get a board soon and can try it out - or else send me a link to
> > your TRM or w/e.
> 
> Anyway, I would like to thank you very much for your time.

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

Thanks,
Conor.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

Re: [RESEND PATCH v2 2/3] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/12/29 3:50, Conor Dooley wrote:
> Hey Walker,
> Took another bit of a look.

Hey Conor, sorry for delay respond as I was busy with other things.

> 
> On Wed, Dec 28, 2022 at 10:08:55AM +0800, Walker Chen wrote:
>> On 2022/12/20 4:40, Conor Dooley wrote:
>> > Hey Walker,
>> > 
>> > Hopefully just some minor bits here. Hopefully either Emil who has a
>> > board, or someone that knows power management stuff better can give this
>> > a proper review.
>> > 
>> > On Thu, Dec 08, 2022 at 04:45:22PM +0800, Walker Chen wrote:
>> >> Add pmu driver for the StarFive JH71XX SoC.
>> >> 
>> >> As the power domains provider, the Power Management Unit (PMU) is
>> >> designed for including multiple PM domains that can be used for power
>> >> gating of selected IP blocks for power saving by reduced leakage
>> >> current. It accepts software encourage command to switch the power mode
>> >> of SoC.
>> >> 
>> >> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> >> ---
>> >>  MAINTAINERS                       |  14 ++
>> >>  drivers/soc/Kconfig               |   1 +
>> >>  drivers/soc/Makefile              |   1 +
>> >>  drivers/soc/starfive/Kconfig      |  11 +
>> >>  drivers/soc/starfive/Makefile     |   3 +
>> >>  drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
>> >>  6 files changed, 426 insertions(+)
>> >>  create mode 100644 drivers/soc/starfive/Kconfig
>> >>  create mode 100644 drivers/soc/starfive/Makefile
>> >>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
>> >> 
>> > 
>> >> +config JH71XX_PMU
>> >> +	bool "Support PMU for StarFive JH71XX Soc"
>> >> +	depends on PM && (SOC_STARFIVE || COMPILE_TEST)
>> > 
>> > Why not just do:
>> > 	depends on PM
>> > 	depends on SOC_STARFIVE || COMPILE_TEST
>> > I think that way reads a little better.
>> 
>> No problem, will be changed like this way.
>> 
>> > 
>> >> +	default SOC_STARFIVE
>> >> +	select PM_GENERIC_DOMAINS
>> >> +	help
>> >> +	  Say 'y' here to enable support power domain support.
>> >> +	  In order to meet low power requirements, a Power Management Unit (PMU)
>> >> +	  is designed for controlling power resources in StarFive JH71XX SoCs.
>> >> 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..7a0145779e07
>> >> --- /dev/null
>> >> +++ b/drivers/soc/starfive/jh71xx_pmu.c
>> >> @@ -0,0 +1,396 @@
>> >> +// SPDX-License-Identifier: GPL-2.0-or-later
>> >> +/*
>> >> + * StarFive JH71XX PMU (Power Management Unit) Controller Driver
>> >> + *
>> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> >> + */
>> >> +
>> >> +#include <linux/interrupt.h>
>> >> +#include <linux/io.h>
>> >> +#include <linux/iopoll.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/of.h>
>> >> +#include <linux/of_device.h>
>> >> +#include <linux/platform_device.h>
>> >> +#include <linux/pm_domain.h>
>> >> +#include <dt-bindings/power/starfive,jh7110-pmu.h>
>> >> +
>> >> +/* register offset */
>> >> +#define JH71XX_PMU_HW_EVENT_ON		0x04
>> >> +#define JH71XX_PMU_HW_EVENT_OFF		0x08
> 
> Neither of these are used here at the moment - would they be used in the
> future? Also the docs fail to describe this PMU_HW_EVENT_OFF register,
> although the HW_ON, and SW_FOO ones are described.

They are not used in the current version, may be used in the future with the improvement of driver.
If so, how about your suggestion ?  drop them ?

> 
>> >> +#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
>> >> +#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
>> >> +#define JH71XX_PMU_SW_ENCOURAGE		0x44
>> >> +#define JH71XX_PMU_INT_MASK		0x48
> 
> This one is called the "Timer Interrupt Mask", so we may as well match
> that naming here, no?

OK, will be changed.

> 
>> >> +#define JH71XX_PMU_PCH_BYPASS		0x4C
>> >> +#define JH71XX_PMU_PCH_PSTATE		0x50
>> >> +#define JH71XX_PMU_PCH_TIMEOUT		0x54
>> >> +#define JH71XX_PMU_LP_TIMEOUT		0x58
>> >> +#define JH71XX_PMU_HW_TURN_ON		0x5C
> 
> Same here, this HW related bit is also not used AFAICT.
> Do you intend adding a user of the HW encourage at some point in the
> future?

I haven't thought about this yet. At least SW encourage can work now.

> 
>> >> +#define JH71XX_PMU_CURR_POWER_MODE	0x80
>> >> +#define JH71XX_PMU_EVENT_STATUS		0x88
>> >> +#define JH71XX_PMU_INT_STATUS		0x8C
>> >> +
>> >> +/* sw encourage cfg */
>> >> +#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
>> >> +#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
>> >> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
>> >> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
>> >> +#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF
> 
> This all seems to correspond w/ docs...
> 
>> >> +
>> >> +/* pmu int status */
>> >> +#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
>> >> +#define JH71XX_PMU_INT_HW_REQ		BIT(1)
>> >> +#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
>> >> +#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
>> >> +#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
>> >> +#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
>> >> +					JH71XX_PMU_INT_HW_FAIL | \
>> >> +					JH71XX_PMU_INT_PCH_FAIL)
>> >> +#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \
>> >> +					JH71XX_PMU_INT_HW_REQ | \
>> >> +					JH71XX_PMU_INT_FAIL_MASK)
> 
> ...as does all of this - although, could the FAIL_MASK be dropped as it
> appears to only be used here & the ALL_MASK definition be replaced with
> GENMASK(8, 0)?
> I don't really mind what you do here, think it just may be slightly
> easier to read, so if you disagree leave it as is.

Maybe your way is more readable.

> 
>> >> +
>> >> +/*
>> >> + * The time required for switching power status is based on the time
>> >> + * to turn on the largest domain's power, which is at microsecond level
> 
> Would you mind mentioning which domain that is?
> USB seems to be listed in Table 3-9 as 200 us.

Hardware colleague told me this time is based on the time to turn on the largest 
domain's power, but he didn't tell me which one it was. This time refers to the
 time required for switching the status of power domains, is not the time of power-up for analog PHY.
USB is not one of power domains in JH7110 SoC.

> 
>> >> + */
>> >> +#define JH71XX_PMU_TIMEOUT_US		100
> 
> I'm happy enough with things, apart from my lack of familiarity with the
> power management APIs. Perhaps when you send the next version, someone
> else can comment there.

Anyway, I appreciate your detailed reading and comments.

> 
>> >> +struct jh71xx_domain_info {
>> > 
>> > 	const char * const name;
>> > 	unsigned int flags;
>> > 	u8 bit;
>> > 
>> >> +};
>> >> +
>> >> +struct jh71xx_pmu_match_data {
>> > 
>> > 	const struct jh71xx_domain_info *domain_info;
>> > 	int num_domains;
>> > 
>> > Can you switch these two around like so?
>> 
>> Should be no problem.
>> 
>> >> +};
>> >> +
>> >> +struct jh71xx_pmu {
>> >> +	struct device *dev;
>> >> +	const struct jh71xx_pmu_match_data *match_data;
>> >> +	void __iomem *base;
>> >> +	spinlock_t lock;	/* protects pmu reg */
>> >> +	int irq;
>> >> +	struct genpd_onecell_data genpd_data;
>> >> +	struct generic_pm_domain **genpd;
>> >> +};
>> >> +
>> >> +struct jh71xx_pmu_dev {
>> >> +	struct generic_pm_domain genpd;
>> >> +	const struct jh71xx_domain_info *domain_info;
>> >> +	struct jh71xx_pmu *pmu;
>> > 
>> > And these two too please in the same way.
>> 
>> Nice :)
>> 
>> > 
>> >> +};
>> >> +
>> >> +static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
>> >> +{
>> >> +	struct jh71xx_pmu *pmu = pmd->pmu;
>> >> +
>> >> +	if (!mask) {
>> >> +		*is_on = false;
>> >> +		return -EINVAL;
>> >> +	}
>> >> +
>> >> +	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
>> >> +{
>> >> +	struct jh71xx_pmu *pmu = pmd->pmu;
>> >> +	unsigned long flags;
>> >> +	u32 val;
>> >> +	u32 mode;
>> >> +	u32 encourage_lo;
>> >> +	u32 encourage_hi;
>> >> +	bool is_on;
>> >> +	int ret;
>> >> +
>> >> +	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
>> >> +	if (ret) {
>> >> +		dev_dbg(pmu->dev, "unable to get current state for %s\n",
>> >> +			pmd->genpd.name);
>> >> +		return ret;
>> >> +	}
>> >> +
>> >> +	if (is_on == on) {
>> >> +		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
>> >> +			pmd->genpd.name, on ? "en" : "dis");
>> >> +		return 0;
>> >> +	}
>> >> +
>> >> +	spin_lock_irqsave(&pmu->lock, flags);
>> >> +
>> >> +	/*
>> >> +	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
>> >> +	 *
>> >> +	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
>> >> +	 *    the bit which power domain will be turn-on, write 0 to the others.
>> > 
>> > Some grammatical nit picking..
>> > 
>> > "Configure the register blah (offset 0x0c) by writing 1 to the bit
>> > corresponding to the power domain that will be turned on and writing
>> > zero to the others."
>> > 
>> > Is that a correct correct summation of the operation?
>> 
>> Yes, maybe your description is better easy-to-understand.
>> 
>> > 
>> >> +	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
>> >> +	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
>> > 
>> > 
>> >> +	 */
>> >> +	if (on) {
>> >> +		mode = JH71XX_PMU_SW_TURN_ON_POWER;
>> >> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
>> >> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
>> >> +	} else {
>> >> +		mode = JH71XX_PMU_SW_TURN_OFF_POWER;
>> >> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
>> >> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
>> >> +	}
>> >> +
>> >> +	writel(mask, pmu->base + mode);
>> >> +
>> >> +	/*
>> >> +	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
>> >> +	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
>> >> +	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0
>> > 
>> > I think you could replace the hard "coded" numbers here with a better
>> > description idk without looking at the #defines above what these
>> > correspond to. AFAICT, it'd be something like:
>> > First write the ...ENCOURAGE_ON to reset the state machine which parses
>> > the command sequence. It must be written every time.
>> > Then write the lower bits of the command sequence, followed by the upper
>> > bits. The sequence differs between powering on & off a domain.
>> 
>> Thank you for correction for these description. Because English is not my native language,
>> so not very good in some sentences. I'll take your advice.
> 
> It may be mine but that does not mean I don't make mistakes either :)
> In this case, I'd like to re-word my suggestion. How about:
> First write ...ENCOURAGE_ON to ...SW_ENCOURAGE. This will reset the state
> machine which parses the command sequence. This register must be written
> every time software wants to power on/off a domain.
> Then write the lower bits of the command sequence, followed by the upper
> bits. The sequence differs between powering on & off a domain.
> 
> When I read my previous suggestion back, I had to read it more than once
> to get what I had meant...

In brief, the key is to clearly describe the software steps and it's easy to understand.

> 
>> >> +	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
>> >> +	 * the purpose is to reset the state machine which is going to parse instruction
>> >> +	 *  sequence. It has to be written every time.
>> >> +	 */
>> >> +	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
>> >> +	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
>> >> +	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
>> >> +
>> >> +	spin_unlock_irqrestore(&pmu->lock, flags);
>> >> +
>> >> +	/* Wait for the power domain bit to be enabled / disabled */
>> >> +	if (on) {
>> >> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
>> >> +						val, val & mask,
>> >> +						1, JH71XX_PMU_TIMEOUT_US);
>> >> +	} else {
>> >> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
>> >> +						val, !(val & mask),
>> >> +						1, JH71XX_PMU_TIMEOUT_US);
>> >> +	}
>> >> +
>> >> +	if (ret) {
>> >> +		dev_err(pmu->dev, "%s: failed to power %s\n",
>> >> +			pmd->genpd.name, on ? "on" : "off");
>> >> +		return -ETIMEDOUT;
>> >> +	}
>> >> +
>> >> +	return 0;
>> >> +}
>> > 
>> >> +static int jh71xx_pmu_probe(struct platform_device *pdev)
>> >> +{
>> >> +	struct device *dev = &pdev->dev;
>> >> +	struct device_node *np = dev->of_node;
>> >> +	const struct jh71xx_pmu_match_data *match_data;
>> >> +	struct jh71xx_pmu *pmu;
>> >> +	unsigned int i;
>> >> +	int ret;
>> >> +
>> >> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
>> >> +	if (!pmu)
>> >> +		return -ENOMEM;
>> >> +
>> >> +	pmu->base = devm_platform_ioremap_resource(pdev, 0);
>> >> +	if (IS_ERR(pmu->base))
>> >> +		return PTR_ERR(pmu->base);
>> >> +
>> >> +	/* initialize pmu interrupt  */
>> > 
>> > nit: this comment is ~pointless.
>> 
>> Will be dropped.
>> 
>> > 
>> >> +	pmu->irq = platform_get_irq(pdev, 0);
>> >> +	if (pmu->irq < 0)
>> >> +		return pmu->irq;
>> >> +
>> >> +	ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
>> >> +			       0, pdev->name, pmu);
>> >> +	if (ret)
>> >> +		dev_err(dev, "request irq failed.\n");
>> > 
>> > nit: s/request/requesting
>> 
>> Will be fixed.
>> 
>> > 
>> > Unfortunately I cannot really review the rest of this, but hopefully
>> > I'll get a board soon and can try it out - or else send me a link to
>> > your TRM or w/e.
>> 
>> Anyway, I would like to thank you very much for your time.
> 
> Oh, while I think of it - I was talking to some people from Imagination
> at the RISC-V Summit who said they're working on open sourcing their
> GPU drivers, so hopefully we do end up with an open source driver for
> the one on JH7110 soon :)

Of course, hopefully the GPU can also be open source eventually.

Best regards,
Walker Chen

Re: [RESEND PATCH v2 2/3] soc: starfive: Add StarFive JH71XX pmu driver

[Walker Chen]

On 2022/12/29 3:50, Conor Dooley wrote:
> Hey Walker,
> Took another bit of a look.

Hey Conor, sorry for delay respond as I was busy with other things.

> 
> On Wed, Dec 28, 2022 at 10:08:55AM +0800, Walker Chen wrote:
>> On 2022/12/20 4:40, Conor Dooley wrote:
>> > Hey Walker,
>> > 
>> > Hopefully just some minor bits here. Hopefully either Emil who has a
>> > board, or someone that knows power management stuff better can give this
>> > a proper review.
>> > 
>> > On Thu, Dec 08, 2022 at 04:45:22PM +0800, Walker Chen wrote:
>> >> Add pmu driver for the StarFive JH71XX SoC.
>> >> 
>> >> As the power domains provider, the Power Management Unit (PMU) is
>> >> designed for including multiple PM domains that can be used for power
>> >> gating of selected IP blocks for power saving by reduced leakage
>> >> current. It accepts software encourage command to switch the power mode
>> >> of SoC.
>> >> 
>> >> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
>> >> ---
>> >>  MAINTAINERS                       |  14 ++
>> >>  drivers/soc/Kconfig               |   1 +
>> >>  drivers/soc/Makefile              |   1 +
>> >>  drivers/soc/starfive/Kconfig      |  11 +
>> >>  drivers/soc/starfive/Makefile     |   3 +
>> >>  drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
>> >>  6 files changed, 426 insertions(+)
>> >>  create mode 100644 drivers/soc/starfive/Kconfig
>> >>  create mode 100644 drivers/soc/starfive/Makefile
>> >>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
>> >> 
>> > 
>> >> +config JH71XX_PMU
>> >> +	bool "Support PMU for StarFive JH71XX Soc"
>> >> +	depends on PM && (SOC_STARFIVE || COMPILE_TEST)
>> > 
>> > Why not just do:
>> > 	depends on PM
>> > 	depends on SOC_STARFIVE || COMPILE_TEST
>> > I think that way reads a little better.
>> 
>> No problem, will be changed like this way.
>> 
>> > 
>> >> +	default SOC_STARFIVE
>> >> +	select PM_GENERIC_DOMAINS
>> >> +	help
>> >> +	  Say 'y' here to enable support power domain support.
>> >> +	  In order to meet low power requirements, a Power Management Unit (PMU)
>> >> +	  is designed for controlling power resources in StarFive JH71XX SoCs.
>> >> 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..7a0145779e07
>> >> --- /dev/null
>> >> +++ b/drivers/soc/starfive/jh71xx_pmu.c
>> >> @@ -0,0 +1,396 @@
>> >> +// SPDX-License-Identifier: GPL-2.0-or-later
>> >> +/*
>> >> + * StarFive JH71XX PMU (Power Management Unit) Controller Driver
>> >> + *
>> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> >> + */
>> >> +
>> >> +#include <linux/interrupt.h>
>> >> +#include <linux/io.h>
>> >> +#include <linux/iopoll.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/of.h>
>> >> +#include <linux/of_device.h>
>> >> +#include <linux/platform_device.h>
>> >> +#include <linux/pm_domain.h>
>> >> +#include <dt-bindings/power/starfive,jh7110-pmu.h>
>> >> +
>> >> +/* register offset */
>> >> +#define JH71XX_PMU_HW_EVENT_ON		0x04
>> >> +#define JH71XX_PMU_HW_EVENT_OFF		0x08
> 
> Neither of these are used here at the moment - would they be used in the
> future? Also the docs fail to describe this PMU_HW_EVENT_OFF register,
> although the HW_ON, and SW_FOO ones are described.

They are not used in the current version, may be used in the future with the improvement of driver.
If so, how about your suggestion ?  drop them ?

> 
>> >> +#define JH71XX_PMU_SW_TURN_ON_POWER	0x0C
>> >> +#define JH71XX_PMU_SW_TURN_OFF_POWER	0x10
>> >> +#define JH71XX_PMU_SW_ENCOURAGE		0x44
>> >> +#define JH71XX_PMU_INT_MASK		0x48
> 
> This one is called the "Timer Interrupt Mask", so we may as well match
> that naming here, no?

OK, will be changed.

> 
>> >> +#define JH71XX_PMU_PCH_BYPASS		0x4C
>> >> +#define JH71XX_PMU_PCH_PSTATE		0x50
>> >> +#define JH71XX_PMU_PCH_TIMEOUT		0x54
>> >> +#define JH71XX_PMU_LP_TIMEOUT		0x58
>> >> +#define JH71XX_PMU_HW_TURN_ON		0x5C
> 
> Same here, this HW related bit is also not used AFAICT.
> Do you intend adding a user of the HW encourage at some point in the
> future?

I haven't thought about this yet. At least SW encourage can work now.

> 
>> >> +#define JH71XX_PMU_CURR_POWER_MODE	0x80
>> >> +#define JH71XX_PMU_EVENT_STATUS		0x88
>> >> +#define JH71XX_PMU_INT_STATUS		0x8C
>> >> +
>> >> +/* sw encourage cfg */
>> >> +#define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
>> >> +#define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
>> >> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO	0x0A
>> >> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI	0xA0
>> >> +#define JH71XX_PMU_SW_ENCOURAGE_ON	0xFF
> 
> This all seems to correspond w/ docs...
> 
>> >> +
>> >> +/* pmu int status */
>> >> +#define JH71XX_PMU_INT_SEQ_DONE		BIT(0)
>> >> +#define JH71XX_PMU_INT_HW_REQ		BIT(1)
>> >> +#define JH71XX_PMU_INT_SW_FAIL		GENMASK(3, 2)
>> >> +#define JH71XX_PMU_INT_HW_FAIL		GENMASK(5, 4)
>> >> +#define JH71XX_PMU_INT_PCH_FAIL		GENMASK(8, 6)
>> >> +#define JH71XX_PMU_INT_FAIL_MASK	(JH71XX_PMU_INT_SW_FAIL | \
>> >> +					JH71XX_PMU_INT_HW_FAIL | \
>> >> +					JH71XX_PMU_INT_PCH_FAIL)
>> >> +#define JH71XX_PMU_INT_ALL_MASK		(JH71XX_PMU_INT_SEQ_DONE | \
>> >> +					JH71XX_PMU_INT_HW_REQ | \
>> >> +					JH71XX_PMU_INT_FAIL_MASK)
> 
> ...as does all of this - although, could the FAIL_MASK be dropped as it
> appears to only be used here & the ALL_MASK definition be replaced with
> GENMASK(8, 0)?
> I don't really mind what you do here, think it just may be slightly
> easier to read, so if you disagree leave it as is.

Maybe your way is more readable.

> 
>> >> +
>> >> +/*
>> >> + * The time required for switching power status is based on the time
>> >> + * to turn on the largest domain's power, which is at microsecond level
> 
> Would you mind mentioning which domain that is?
> USB seems to be listed in Table 3-9 as 200 us.

Hardware colleague told me this time is based on the time to turn on the largest 
domain's power, but he didn't tell me which one it was. This time refers to the
 time required for switching the status of power domains, is not the time of power-up for analog PHY.
USB is not one of power domains in JH7110 SoC.

> 
>> >> + */
>> >> +#define JH71XX_PMU_TIMEOUT_US		100
> 
> I'm happy enough with things, apart from my lack of familiarity with the
> power management APIs. Perhaps when you send the next version, someone
> else can comment there.

Anyway, I appreciate your detailed reading and comments.

> 
>> >> +struct jh71xx_domain_info {
>> > 
>> > 	const char * const name;
>> > 	unsigned int flags;
>> > 	u8 bit;
>> > 
>> >> +};
>> >> +
>> >> +struct jh71xx_pmu_match_data {
>> > 
>> > 	const struct jh71xx_domain_info *domain_info;
>> > 	int num_domains;
>> > 
>> > Can you switch these two around like so?
>> 
>> Should be no problem.
>> 
>> >> +};
>> >> +
>> >> +struct jh71xx_pmu {
>> >> +	struct device *dev;
>> >> +	const struct jh71xx_pmu_match_data *match_data;
>> >> +	void __iomem *base;
>> >> +	spinlock_t lock;	/* protects pmu reg */
>> >> +	int irq;
>> >> +	struct genpd_onecell_data genpd_data;
>> >> +	struct generic_pm_domain **genpd;
>> >> +};
>> >> +
>> >> +struct jh71xx_pmu_dev {
>> >> +	struct generic_pm_domain genpd;
>> >> +	const struct jh71xx_domain_info *domain_info;
>> >> +	struct jh71xx_pmu *pmu;
>> > 
>> > And these two too please in the same way.
>> 
>> Nice :)
>> 
>> > 
>> >> +};
>> >> +
>> >> +static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
>> >> +{
>> >> +	struct jh71xx_pmu *pmu = pmd->pmu;
>> >> +
>> >> +	if (!mask) {
>> >> +		*is_on = false;
>> >> +		return -EINVAL;
>> >> +	}
>> >> +
>> >> +	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
>> >> +{
>> >> +	struct jh71xx_pmu *pmu = pmd->pmu;
>> >> +	unsigned long flags;
>> >> +	u32 val;
>> >> +	u32 mode;
>> >> +	u32 encourage_lo;
>> >> +	u32 encourage_hi;
>> >> +	bool is_on;
>> >> +	int ret;
>> >> +
>> >> +	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
>> >> +	if (ret) {
>> >> +		dev_dbg(pmu->dev, "unable to get current state for %s\n",
>> >> +			pmd->genpd.name);
>> >> +		return ret;
>> >> +	}
>> >> +
>> >> +	if (is_on == on) {
>> >> +		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
>> >> +			pmd->genpd.name, on ? "en" : "dis");
>> >> +		return 0;
>> >> +	}
>> >> +
>> >> +	spin_lock_irqsave(&pmu->lock, flags);
>> >> +
>> >> +	/*
>> >> +	 * The PMU accepts software encourage to switch power mode in the following 2 steps:
>> >> +	 *
>> >> +	 * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
>> >> +	 *    the bit which power domain will be turn-on, write 0 to the others.
>> > 
>> > Some grammatical nit picking..
>> > 
>> > "Configure the register blah (offset 0x0c) by writing 1 to the bit
>> > corresponding to the power domain that will be turned on and writing
>> > zero to the others."
>> > 
>> > Is that a correct correct summation of the operation?
>> 
>> Yes, maybe your description is better easy-to-understand.
>> 
>> > 
>> >> +	 *    Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
>> >> +	 *    write 1 to the bit which power domain will be turn-off, write 0 to the others.
>> > 
>> > 
>> >> +	 */
>> >> +	if (on) {
>> >> +		mode = JH71XX_PMU_SW_TURN_ON_POWER;
>> >> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
>> >> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
>> >> +	} else {
>> >> +		mode = JH71XX_PMU_SW_TURN_OFF_POWER;
>> >> +		encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
>> >> +		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
>> >> +	}
>> >> +
>> >> +	writel(mask, pmu->base + mode);
>> >> +
>> >> +	/*
>> >> +	 * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
>> >> +	 * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
>> >> +	 * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0
>> > 
>> > I think you could replace the hard "coded" numbers here with a better
>> > description idk without looking at the #defines above what these
>> > correspond to. AFAICT, it'd be something like:
>> > First write the ...ENCOURAGE_ON to reset the state machine which parses
>> > the command sequence. It must be written every time.
>> > Then write the lower bits of the command sequence, followed by the upper
>> > bits. The sequence differs between powering on & off a domain.
>> 
>> Thank you for correction for these description. Because English is not my native language,
>> so not very good in some sentences. I'll take your advice.
> 
> It may be mine but that does not mean I don't make mistakes either :)
> In this case, I'd like to re-word my suggestion. How about:
> First write ...ENCOURAGE_ON to ...SW_ENCOURAGE. This will reset the state
> machine which parses the command sequence. This register must be written
> every time software wants to power on/off a domain.
> Then write the lower bits of the command sequence, followed by the upper
> bits. The sequence differs between powering on & off a domain.
> 
> When I read my previous suggestion back, I had to read it more than once
> to get what I had meant...

In brief, the key is to clearly describe the software steps and it's easy to understand.

> 
>> >> +	 * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the SW_ENCOURAGE register,
>> >> +	 * the purpose is to reset the state machine which is going to parse instruction
>> >> +	 *  sequence. It has to be written every time.
>> >> +	 */
>> >> +	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
>> >> +	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
>> >> +	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
>> >> +
>> >> +	spin_unlock_irqrestore(&pmu->lock, flags);
>> >> +
>> >> +	/* Wait for the power domain bit to be enabled / disabled */
>> >> +	if (on) {
>> >> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
>> >> +						val, val & mask,
>> >> +						1, JH71XX_PMU_TIMEOUT_US);
>> >> +	} else {
>> >> +		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
>> >> +						val, !(val & mask),
>> >> +						1, JH71XX_PMU_TIMEOUT_US);
>> >> +	}
>> >> +
>> >> +	if (ret) {
>> >> +		dev_err(pmu->dev, "%s: failed to power %s\n",
>> >> +			pmd->genpd.name, on ? "on" : "off");
>> >> +		return -ETIMEDOUT;
>> >> +	}
>> >> +
>> >> +	return 0;
>> >> +}
>> > 
>> >> +static int jh71xx_pmu_probe(struct platform_device *pdev)
>> >> +{
>> >> +	struct device *dev = &pdev->dev;
>> >> +	struct device_node *np = dev->of_node;
>> >> +	const struct jh71xx_pmu_match_data *match_data;
>> >> +	struct jh71xx_pmu *pmu;
>> >> +	unsigned int i;
>> >> +	int ret;
>> >> +
>> >> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
>> >> +	if (!pmu)
>> >> +		return -ENOMEM;
>> >> +
>> >> +	pmu->base = devm_platform_ioremap_resource(pdev, 0);
>> >> +	if (IS_ERR(pmu->base))
>> >> +		return PTR_ERR(pmu->base);
>> >> +
>> >> +	/* initialize pmu interrupt  */
>> > 
>> > nit: this comment is ~pointless.
>> 
>> Will be dropped.
>> 
>> > 
>> >> +	pmu->irq = platform_get_irq(pdev, 0);
>> >> +	if (pmu->irq < 0)
>> >> +		return pmu->irq;
>> >> +
>> >> +	ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
>> >> +			       0, pdev->name, pmu);
>> >> +	if (ret)
>> >> +		dev_err(dev, "request irq failed.\n");
>> > 
>> > nit: s/request/requesting
>> 
>> Will be fixed.
>> 
>> > 
>> > Unfortunately I cannot really review the rest of this, but hopefully
>> > I'll get a board soon and can try it out - or else send me a link to
>> > your TRM or w/e.
>> 
>> Anyway, I would like to thank you very much for your time.
> 
> Oh, while I think of it - I was talking to some people from Imagination
> at the RISC-V Summit who said they're working on open sourcing their
> GPU drivers, so hopefully we do end up with an open source driver for
> the one on JH7110 soon :)

Of course, hopefully the GPU can also be open source eventually.

Best regards,
Walker Chen


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