[PATCH 0/5] arm64: meson: add support for SM1 Power Domains

[PATCH 0/5] arm64: meson: add support for SM1 Power Domains

[Neil Armstrong]

This patchset introduces a new "Everything-Else Power Domain Controller"
designed to handle all the different non-Always On peripherals like :
- VPU
- Ethernet Memories
- USB, PCIe, Audio, NNA on SM1

The current "gx-vpu-pwrc" process has been integrated to support the VPU
and the other power domains in a single driver.

Support for SoC domains has been made generic and easily extendable.

In order to restart from clean architecture :
- the PWRC node has been moved into the HHI simple-mfd, this suits much
  better than beeing in the AO RTI simple-mfd
- a brand new yaml bindings schemas has been written
- reset-names has been added to clarify which resets are needed, so we can
  dispatch them to domains

For G12A, the PWRC now offers support for the ethmac memory power domain.

For SM1, it also offers support for PCIe, USB, NNA, ethmac and Audio power
domains.

The DOS domains has been excluded for now, but can be added very easily.

GX hasn't been integrated for now, but it would follow the same scheme
as G12A support.

Neil Armstrong (5):
  dt-bindings: power: add Amlogic Everything-Else power domains bindings
  soc: amlogic: Add support for Everything-Else power domains controller
  arm64: meson-g12: add Everything-Else power domain controller
  arm64: dts: meson-sm1-sei610: add HDMI display support
  arm64: dts: meson-sm1-sei610: add USB support

 .../bindings/power/amlogic,meson-ee-pwrc.yaml |  93 +++
 .../boot/dts/amlogic/meson-g12-common.dtsi    |  92 +--
 arch/arm64/boot/dts/amlogic/meson-g12a.dtsi   |   9 +
 arch/arm64/boot/dts/amlogic/meson-g12b.dtsi   |   9 +
 .../boot/dts/amlogic/meson-sm1-sei610.dts     |  28 +
 arch/arm64/boot/dts/amlogic/meson-sm1.dtsi    |  15 +-
 drivers/soc/amlogic/Kconfig                   |  11 +
 drivers/soc/amlogic/Makefile                  |   1 +
 drivers/soc/amlogic/meson-ee-pwrc.c           | 560 ++++++++++++++++++
 include/dt-bindings/power/meson-g12a-power.h  |  13 +
 include/dt-bindings/power/meson-sm1-power.h   |  18 +
 11 files changed, 801 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml
 create mode 100644 drivers/soc/amlogic/meson-ee-pwrc.c
 create mode 100644 include/dt-bindings/power/meson-g12a-power.h
 create mode 100644 include/dt-bindings/power/meson-sm1-power.h

-- 
2.22.0

[PATCH 1/5] dt-bindings: power: add Amlogic Everything-Else power domains bindings

[Neil Armstrong]

Add the bindings for the Amlogic Everything-Else power domains,
controlling the Everything-Else peripherals power domains.

The bindings targets the Amlogic G12A and SM1 compatible SoCs,
support for earlier SoCs will be added later.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 .../bindings/power/amlogic,meson-ee-pwrc.yaml | 93 +++++++++++++++++++
 include/dt-bindings/power/meson-g12a-power.h  | 13 +++
 include/dt-bindings/power/meson-sm1-power.h   | 18 ++++
 3 files changed, 124 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml
 create mode 100644 include/dt-bindings/power/meson-g12a-power.h
 create mode 100644 include/dt-bindings/power/meson-sm1-power.h

diff --git a/Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml b/Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml
new file mode 100644
index 000000000000..aab70e8b681e
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019 BayLibre, SAS
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/power/amlogic,meson-ee-pwrc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Amlogic Meson Everything-Else Power Domains
+
+maintainers:
+  - Neil Armstrong <narmstrong@baylibre.com>
+
+description: |+
+  The Everything-Else Power Domains node should be the child of a syscon
+  node with the required property:
+
+  - compatible: Should be the following:
+                "amlogic,meson-gx-hhi-sysctrl", "simple-mfd", "syscon"
+
+  Refer to the the bindings described in
+  Documentation/devicetree/bindings/mfd/syscon.txt
+
+properties:
+  compatible:
+    enum:
+      - amlogic,meson-g12a-pwrc
+      - amlogic,meson-sm1-pwrc
+
+  clocks:
+    minItems: 2
+
+  clock-names:
+    items:
+      - const: vpu
+      - const: vapb
+
+  resets:
+    minItems: 11
+
+  reset-names:
+    items:
+      - const: viu
+      - const: venc
+      - const: vcbus
+      - const: bt656
+      - const: rdma
+      - const: venci
+      - const: vencp
+      - const: vdac
+      - const: vdi6
+      - const: vencl
+      - const: vid_lock
+
+  "#power-domain-cells":
+    const: 1
+
+  amlogic,ao-sysctrl:
+    description: phandle to the AO sysctrl node
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/phandle
+
+required:
+  - compatible
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - "#power-domain-cells"
+  - amlogic,ao-sysctrl
+
+examples:
+  - |
+    pwrc: power-controller {
+          compatible = "amlogic,meson-sm1-pwrc";
+          #power-domain-cells = <1>;
+          amlogic,ao-sysctrl = <&rti>;
+          resets = <&reset_viu>,
+                   <&reset_venc>,
+                   <&reset_vcbus>,
+                   <&reset_bt656>,
+                   <&reset_rdma>,
+                   <&reset_venci>,
+                   <&reset_vencp>,
+                   <&reset_vdac>,
+                   <&reset_vdi6>,
+                   <&reset_vencl>,
+                   <&reset_vid_lock>;
+          reset-names = "viu", "venc", "vcbus", "bt656",
+                        "rdma", "venci", "vencp", "vdac",
+                        "vdi6", "vencl", "vid_lock";
+          clocks = <&clk_vpu>, <&clk_vapb>;
+          clock-names = "vpu", "vapb";
+    };
diff --git a/include/dt-bindings/power/meson-g12a-power.h b/include/dt-bindings/power/meson-g12a-power.h
new file mode 100644
index 000000000000..bb5e67a842de
--- /dev/null
+++ b/include/dt-bindings/power/meson-g12a-power.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
+/*
+ * Copyright (c) 2019 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#ifndef _DT_BINDINGS_MESON_G12A_POWER_H
+#define _DT_BINDINGS_MESON_G12A_POWER_H
+
+#define PWRC_G12A_VPU_ID		0
+#define PWRC_G12A_ETH_ID		1
+
+#endif
diff --git a/include/dt-bindings/power/meson-sm1-power.h b/include/dt-bindings/power/meson-sm1-power.h
new file mode 100644
index 000000000000..a020ab00c134
--- /dev/null
+++ b/include/dt-bindings/power/meson-sm1-power.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
+/*
+ * Copyright (c) 2019 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#ifndef _DT_BINDINGS_MESON_SM1_POWER_H
+#define _DT_BINDINGS_MESON_SM1_POWER_H
+
+#define PWRC_SM1_VPU_ID		0
+#define PWRC_SM1_NNA_ID		1
+#define PWRC_SM1_USB_ID		2
+#define PWRC_SM1_PCIE_ID	3
+#define PWRC_SM1_GE2D_ID	4
+#define PWRC_SM1_AUDIO_ID	5
+#define PWRC_SM1_ETH_ID		6
+
+#endif
-- 
2.22.0

[PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller

[Neil Armstrong]

Add support for the General Purpose Amlogic Everything-Else Power controller,
with the first support for G12A and SM1 SoCs dedicated to the VPU, PCIe,
USB, NNA, GE2D and Ethernet Power Domains.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/soc/amlogic/Kconfig         |  11 +
 drivers/soc/amlogic/Makefile        |   1 +
 drivers/soc/amlogic/meson-ee-pwrc.c | 560 ++++++++++++++++++++++++++++
 3 files changed, 572 insertions(+)
 create mode 100644 drivers/soc/amlogic/meson-ee-pwrc.c

diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
index 23bfb8ef4fdb..bc2c912949bd 100644
--- a/drivers/soc/amlogic/Kconfig
+++ b/drivers/soc/amlogic/Kconfig
@@ -37,6 +37,17 @@ config MESON_GX_PM_DOMAINS
 	  Say yes to expose Amlogic Meson GX Power Domains as
 	  Generic Power Domains.
 
+config MESON_EE_PM_DOMAINS
+	bool "Amlogic Meson Everything-Else Power Domains driver"
+	depends on ARCH_MESON || COMPILE_TEST
+	depends on PM && OF
+	default ARCH_MESON
+	select PM_GENERIC_DOMAINS
+	select PM_GENERIC_DOMAINS_OF
+	help
+	  Say yes to expose Amlogic Meson Everything-Else Power Domains as
+	  Generic Power Domains.
+
 config MESON_MX_SOCINFO
 	bool "Amlogic Meson MX SoC Information driver"
 	depends on ARCH_MESON || COMPILE_TEST
diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
index f2e4ed171297..de79d044b545 100644
--- a/drivers/soc/amlogic/Makefile
+++ b/drivers/soc/amlogic/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_MESON_CLK_MEASURE) += meson-clk-measure.o
 obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
 obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
 obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
+obj-$(CONFIG_MESON_EE_PM_DOMAINS) += meson-ee-pwrc.o
diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c
new file mode 100644
index 000000000000..7159888c850b
--- /dev/null
+++ b/drivers/soc/amlogic/meson-ee-pwrc.c
@@ -0,0 +1,560 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2019 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/bitfield.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
+#include <linux/reset-controller.h>
+#include <linux/reset.h>
+#include <linux/clk.h>
+#include <dt-bindings/power/meson-g12a-power.h>
+#include <dt-bindings/power/meson-sm1-power.h>
+
+/* AO Offsets */
+
+#define AO_RTI_GEN_PWR_SLEEP0		(0x3a << 2)
+#define AO_RTI_GEN_PWR_ISO0		(0x3b << 2)
+
+/* HHI Offsets */
+
+#define HHI_MEM_PD_REG0			(0x40 << 2)
+#define HHI_VPU_MEM_PD_REG0		(0x41 << 2)
+#define HHI_VPU_MEM_PD_REG1		(0x42 << 2)
+#define HHI_VPU_MEM_PD_REG3		(0x43 << 2)
+#define HHI_VPU_MEM_PD_REG4		(0x44 << 2)
+#define HHI_AUDIO_MEM_PD_REG0		(0x45 << 2)
+#define HHI_NANOQ_MEM_PD_REG0		(0x46 << 2)
+#define HHI_NANOQ_MEM_PD_REG1		(0x47 << 2)
+#define HHI_VPU_MEM_PD_REG2		(0x4d << 2)
+
+struct meson_ee_pwrc;
+struct meson_ee_pwrc_domain;
+
+struct meson_ee_pwrc_mem_domain {
+	unsigned int reg;
+	unsigned int mask;
+};
+
+struct meson_ee_pwrc_top_domain {
+	unsigned int sleep_reg;
+	unsigned int sleep_mask;
+	unsigned int iso_reg;
+	unsigned int iso_mask;
+};
+
+struct meson_ee_pwrc_domain_desc {
+	char *name;
+	char **reset_names;
+	unsigned int reset_names_count;
+	char **clk_names;
+	unsigned int clk_names_count;
+	struct meson_ee_pwrc_top_domain *top_pd;
+	unsigned int mem_pd_count;
+	struct meson_ee_pwrc_mem_domain *mem_pd;
+	bool (*get_power)(struct meson_ee_pwrc_domain *pwrc_domain);
+};
+
+struct meson_ee_pwrc_domain_data {
+	unsigned int count;
+	struct meson_ee_pwrc_domain_desc *domains;
+};
+
+/* Clock and Resets lists */
+
+static char *g12a_pwrc_vpu_resets[] = {
+	"viu", "venc", "vcbus", "bt656",
+	"rdma", "venci", "vencp", "vdac",
+	"vdi6", "vencl", "vid_lock",
+};
+
+static char *g12a_pwrc_vpu_clks[] = {
+	"vpu", "vapb",
+};
+
+/* TOP Power Domains */
+
+static struct meson_ee_pwrc_top_domain g12a_pwrc_vpu = {
+	.sleep_reg = AO_RTI_GEN_PWR_SLEEP0,
+	.sleep_mask = BIT(8),
+	.iso_reg = AO_RTI_GEN_PWR_SLEEP0,
+	.iso_mask = BIT(9),
+};
+
+#define SM1_EE_PD(__bit)					\
+	{							\
+		.sleep_reg = AO_RTI_GEN_PWR_SLEEP0, 		\
+		.sleep_mask = BIT(__bit), 			\
+		.iso_reg = AO_RTI_GEN_PWR_ISO0, 		\
+		.iso_mask = BIT(__bit), 			\
+	}
+
+static struct meson_ee_pwrc_top_domain sm1_pwrc_vpu = SM1_EE_PD(8);
+static struct meson_ee_pwrc_top_domain sm1_pwrc_nna = SM1_EE_PD(16);
+static struct meson_ee_pwrc_top_domain sm1_pwrc_usb = SM1_EE_PD(17);
+static struct meson_ee_pwrc_top_domain sm1_pwrc_pci = SM1_EE_PD(18);
+static struct meson_ee_pwrc_top_domain sm1_pwrc_ge2d = SM1_EE_PD(19);
+
+/* Memory PD Domains */
+
+#define VPU_MEMPD(__reg)					\
+	{ __reg, GENMASK(1, 0) },				\
+	{ __reg, GENMASK(3, 2) },				\
+	{ __reg, GENMASK(5, 4) },				\
+	{ __reg, GENMASK(7, 6) },				\
+	{ __reg, GENMASK(9, 8) },				\
+	{ __reg, GENMASK(11, 10) },				\
+	{ __reg, GENMASK(13, 12) },				\
+	{ __reg, GENMASK(15, 14) },				\
+	{ __reg, GENMASK(17, 16) },				\
+	{ __reg, GENMASK(19, 18) },				\
+	{ __reg, GENMASK(21, 20) },				\
+	{ __reg, GENMASK(23, 22) },				\
+	{ __reg, GENMASK(25, 24) },				\
+	{ __reg, GENMASK(27, 26) },				\
+	{ __reg, GENMASK(29, 28) },				\
+	{ __reg, GENMASK(31, 30) }
+
+#define VPU_HHI_MEMPD(__reg)					\
+	{ __reg, BIT(8) },					\
+	{ __reg, BIT(9) },					\
+	{ __reg, BIT(10) },					\
+	{ __reg, BIT(11) },					\
+	{ __reg, BIT(12) },					\
+	{ __reg, BIT(13) },					\
+	{ __reg, BIT(14) },					\
+	{ __reg, BIT(15) }
+
+static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_vpu[] = {
+	VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
+	VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
+	VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
+	VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
+};
+
+static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_eth[] = {
+	{ HHI_MEM_PD_REG0, GENMASK(3, 2) },
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_vpu[] = {
+	VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
+	VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
+	VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
+	VPU_MEMPD(HHI_VPU_MEM_PD_REG3),
+	{ HHI_VPU_MEM_PD_REG4, GENMASK(1, 0) },
+	{ HHI_VPU_MEM_PD_REG4, GENMASK(3, 2) },
+	{ HHI_VPU_MEM_PD_REG4, GENMASK(5, 4) },
+	{ HHI_VPU_MEM_PD_REG4, GENMASK(7, 6) },
+	VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_nna[] = {
+	{ HHI_NANOQ_MEM_PD_REG0, 0xff },
+	{ HHI_NANOQ_MEM_PD_REG1, 0xff },
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_usb[] = {
+	{ HHI_MEM_PD_REG0, GENMASK(31, 30) },
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_pcie[] = {
+	{ HHI_MEM_PD_REG0, GENMASK(29, 26) },
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_ge2d[] = {
+	{ HHI_MEM_PD_REG0, GENMASK(25, 18) },
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_audio[] = {
+	{ HHI_MEM_PD_REG0, GENMASK(5, 4) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(1, 0) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(3, 2) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(5, 4) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(7, 6) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(13, 12) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(15, 14) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(17, 16) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(19, 18) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(21, 20) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(23, 22) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(25, 24) },
+	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(27, 26) },
+};
+
+#define VPU_PD(__name, __resets, __clks, __top_pd, __mem, __get_power)	\
+	{								\
+		.name = __name,						\
+		.reset_names_count = ARRAY_SIZE(__resets),		\
+		.reset_names = __resets,				\
+		.clk_names_count = ARRAY_SIZE(__clks),			\
+		.clk_names = __clks,					\
+		.top_pd = __top_pd,					\
+		.mem_pd_count = ARRAY_SIZE(__mem),			\
+		.mem_pd = __mem,					\
+		.get_power = __get_power,				\
+	}
+
+#define TOP_PD(__name, __top_pd, __mem)					\
+	{								\
+		.name = __name,						\
+		.top_pd = __top_pd,					\
+		.mem_pd_count = ARRAY_SIZE(__mem),			\
+		.mem_pd = __mem,					\
+	}
+
+#define MEM_PD(__name, __mem)						\
+	TOP_PD(__name, NULL, __mem)
+
+static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain);
+
+static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
+	[PWRC_G12A_VPU_ID]  = VPU_PD("VPU", g12a_pwrc_vpu_resets,
+				     g12a_pwrc_vpu_clks, &g12a_pwrc_vpu,
+				     g12a_pwrc_mem_vpu,
+				     pwrc_vpu_get_power),
+	[PWRC_G12A_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
+};
+
+static struct meson_ee_pwrc_domain_desc sm1_pwrc_domains[] = {
+	[PWRC_SM1_VPU_ID]  = VPU_PD("VPU", g12a_pwrc_vpu_resets,
+				    g12a_pwrc_vpu_clks, &sm1_pwrc_vpu,
+				    sm1_pwrc_mem_vpu,
+				    pwrc_vpu_get_power),
+	[PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna),
+	[PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb),
+	[PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie),
+	[PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d),
+	[PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
+	[PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
+};
+
+struct meson_ee_pwrc_domain {
+	struct generic_pm_domain base;
+	bool enabled;
+	struct meson_ee_pwrc *pwrc;
+	struct meson_ee_pwrc_domain_desc desc;
+	struct clk **clks;
+	int num_clks;
+	struct reset_control **rstc;
+	int num_rstc;
+};
+
+struct meson_ee_pwrc {
+	struct regmap *regmap_ao;
+	struct regmap *regmap_hhi;
+	struct meson_ee_pwrc_domain *domains;
+	struct genpd_onecell_data xlate;
+};
+
+static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain)
+{
+	u32 reg;
+
+	regmap_read(pwrc_domain->pwrc->regmap_ao,
+		    pwrc_domain->desc.top_pd->sleep_reg, &reg);
+
+	return (reg & pwrc_domain->desc.top_pd->sleep_mask);
+}
+
+static int meson_ee_reset_assert(struct meson_ee_pwrc_domain *pwrc_domain)
+{
+	int i, ret;
+
+	for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
+		ret = reset_control_assert(pwrc_domain->rstc[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int meson_ee_reset_deassert(struct meson_ee_pwrc_domain *pwrc_domain)
+{
+	int i, ret;
+
+	for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
+		ret = reset_control_deassert(pwrc_domain->rstc[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int meson_ee_clk_disable(struct meson_ee_pwrc_domain *pwrc_domain)
+{
+	int i;
+
+	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
+		clk_disable(pwrc_domain->clks[i]);
+
+	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
+		clk_unprepare(pwrc_domain->clks[i]);
+
+	return 0;
+}
+
+static int meson_ee_clk_enable(struct meson_ee_pwrc_domain *pwrc_domain)
+{
+	int i, ret;
+
+	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
+		ret = clk_prepare(pwrc_domain->clks[i]);
+		if (ret)
+			goto fail_prepare;
+	}
+
+	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
+		ret = clk_enable(pwrc_domain->clks[i]);
+		if (ret)
+			goto fail_enable;
+	}
+
+	return 0;
+
+fail_enable:
+	while (--i)
+		clk_disable(pwrc_domain->clks[i]);
+
+	/* Unprepare all clocks */
+	i = pwrc_domain->num_clks;
+
+fail_prepare:
+	while (--i)
+		clk_unprepare(pwrc_domain->clks[i]);
+
+	return ret;
+}
+
+static int meson_ee_pwrc_off(struct generic_pm_domain *domain)
+{
+	struct meson_ee_pwrc_domain *pwrc_domain =
+		container_of(domain, struct meson_ee_pwrc_domain, base);
+	int i;
+
+	if (pwrc_domain->desc.top_pd)
+		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
+				   pwrc_domain->desc.top_pd->sleep_reg,
+				   pwrc_domain->desc.top_pd->sleep_mask,
+				   pwrc_domain->desc.top_pd->sleep_mask);
+	udelay(20);
+
+	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
+		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
+				   pwrc_domain->desc.mem_pd[i].reg,
+				   pwrc_domain->desc.mem_pd[i].mask,
+				   pwrc_domain->desc.mem_pd[i].mask);
+
+	udelay(20);
+
+	if (pwrc_domain->desc.top_pd)
+		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
+				   pwrc_domain->desc.top_pd->iso_reg,
+				   pwrc_domain->desc.top_pd->iso_mask,
+				   pwrc_domain->desc.top_pd->iso_mask);
+
+	if (pwrc_domain->num_clks) {
+		msleep(20);
+		meson_ee_clk_disable(pwrc_domain);
+	}
+
+	return 0;
+}
+
+static int meson_ee_pwrc_on(struct generic_pm_domain *domain)
+{
+	struct meson_ee_pwrc_domain *pwrc_domain =
+		container_of(domain, struct meson_ee_pwrc_domain, base);
+	int i, ret;
+
+	if (pwrc_domain->desc.top_pd)
+		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
+				   pwrc_domain->desc.top_pd->sleep_reg,
+				   pwrc_domain->desc.top_pd->sleep_mask, 0);
+	udelay(20);
+
+	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
+		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
+				   pwrc_domain->desc.mem_pd[i].reg,
+				   pwrc_domain->desc.mem_pd[i].mask, 0);
+
+	udelay(20);
+
+	ret = meson_ee_reset_assert(pwrc_domain);
+	if (ret)
+		return ret;
+
+	if (pwrc_domain->desc.top_pd)
+		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
+				   pwrc_domain->desc.top_pd->iso_reg,
+				   pwrc_domain->desc.top_pd->iso_mask, 0);
+
+	ret = meson_ee_reset_deassert(pwrc_domain);
+	if (ret)
+		return ret;
+
+	return meson_ee_clk_enable(pwrc_domain);
+}
+
+static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
+				     struct meson_ee_pwrc *sm1_pwrc,
+				     struct meson_ee_pwrc_domain *dom)
+{
+	dom->pwrc = sm1_pwrc;
+	dom->num_rstc = dom->desc.reset_names_count;
+	dom->num_clks = dom->desc.clk_names_count;
+
+	if (dom->num_rstc) {
+		int rst;
+
+		dom->rstc = devm_kcalloc(&pdev->dev, dom->num_rstc,
+				sizeof(struct reset_control *),	GFP_KERNEL);
+		if (!dom->rstc)
+			return -ENOMEM;
+
+		for (rst = 0 ; rst < dom->num_rstc ; ++rst) {
+			dom->rstc[rst] = devm_reset_control_get_exclusive(
+					&pdev->dev,
+					dom->desc.reset_names[rst]);
+			if (IS_ERR(dom->rstc[rst]))
+				return PTR_ERR(dom->rstc[rst]);
+		}
+	}
+
+	if (dom->num_clks) {
+		int clk;
+
+		dom->clks = devm_kcalloc(&pdev->dev, dom->num_clks,
+				sizeof(struct clk *), GFP_KERNEL);
+		if (!dom->clks)
+			return -ENOMEM;
+
+		for (clk = 0 ; clk < dom->num_clks ; ++clk) {
+			dom->clks[clk] = devm_clk_get(&pdev->dev,
+					dom->desc.clk_names[clk]);
+			if (IS_ERR(dom->clks[clk]))
+				return PTR_ERR(dom->clks[clk]);
+		}
+	}
+
+	dom->base.name = dom->desc.name;
+	dom->base.power_on = meson_ee_pwrc_on;
+	dom->base.power_off = meson_ee_pwrc_off;
+
+	if (dom->desc.get_power) {
+		bool powered_off = dom->desc.get_power(dom);
+		pm_genpd_init(&dom->base, &pm_domain_always_on_gov,
+			      powered_off);
+	} else
+		pm_genpd_init(&dom->base, NULL, true);
+
+	return 0;
+}
+
+static int meson_ee_pwrc_probe(struct platform_device *pdev)
+{
+	const struct meson_ee_pwrc_domain_data *match;
+	struct regmap *regmap_ao, *regmap_hhi;
+	struct meson_ee_pwrc *sm1_pwrc;
+	int i, ret;
+
+	match = of_device_get_match_data(&pdev->dev);
+	if (!match) {
+		dev_err(&pdev->dev, "failed to get match data\n");
+		return -ENODEV;
+	}
+
+	sm1_pwrc = devm_kzalloc(&pdev->dev, sizeof(*sm1_pwrc), GFP_KERNEL);
+	if (!sm1_pwrc)
+		return -ENOMEM;
+
+	sm1_pwrc->xlate.domains =
+		devm_kcalloc(&pdev->dev,
+			     match->count,
+			     sizeof(*sm1_pwrc->xlate.domains),
+			     GFP_KERNEL);
+	if (!sm1_pwrc->xlate.domains)
+		return -ENOMEM;
+
+	sm1_pwrc->domains =
+		devm_kcalloc(&pdev->dev,
+			     match->count,
+			     sizeof(*sm1_pwrc->domains),
+			     GFP_KERNEL);
+	if (!sm1_pwrc->domains)
+		return -ENOMEM;
+
+	sm1_pwrc->xlate.num_domains = match->count;
+
+	regmap_hhi = syscon_node_to_regmap(of_get_parent(pdev->dev.of_node));
+	if (IS_ERR(regmap_hhi)) {
+		dev_err(&pdev->dev, "failed to get HHI regmap\n");
+		return PTR_ERR(regmap_hhi);
+	}
+
+	regmap_ao = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+						    "amlogic,ao-sysctrl");
+	if (IS_ERR(regmap_ao)) {
+		dev_err(&pdev->dev, "failed to get AO regmap\n");
+		return PTR_ERR(regmap_ao);
+	}
+
+	sm1_pwrc->regmap_ao = regmap_ao;
+	sm1_pwrc->regmap_hhi = regmap_hhi;
+
+	platform_set_drvdata(pdev, sm1_pwrc);
+
+	for (i = 0 ; i < match->count ; ++i) {
+		struct meson_ee_pwrc_domain *dom = &sm1_pwrc->domains[i];
+
+		memcpy(&dom->desc, &match->domains[i], sizeof(dom->desc));
+
+		ret = meson_ee_pwrc_init_domain(pdev, sm1_pwrc, dom);
+		if (ret)
+			return ret;
+
+		sm1_pwrc->xlate.domains[i] = &dom->base;
+	}
+
+	of_genpd_add_provider_onecell(pdev->dev.of_node, &sm1_pwrc->xlate);
+
+	return 0;
+}
+
+static struct meson_ee_pwrc_domain_data meson_ee_g12a_pwrc_data = {
+	.count = ARRAY_SIZE(g12a_pwrc_domains),
+	.domains = g12a_pwrc_domains,
+};
+
+static struct meson_ee_pwrc_domain_data meson_ee_sm1_pwrc_data = {
+	.count = ARRAY_SIZE(sm1_pwrc_domains),
+	.domains = sm1_pwrc_domains,
+};
+
+static const struct of_device_id meson_ee_pwrc_match_table[] = {
+	{
+		.compatible = "amlogic,meson-g12a-pwrc",
+		.data = &meson_ee_g12a_pwrc_data,
+	},
+	{
+		.compatible = "amlogic,meson-sm1-pwrc",
+		.data = &meson_ee_sm1_pwrc_data,
+	},
+	{ /* sentinel */ }
+};
+
+static struct platform_driver meson_ee_pwrc_driver = {
+	.probe	= meson_ee_pwrc_probe,
+	.driver = {
+		.name		= "meson_ee_pwrc",
+		.of_match_table	= meson_ee_pwrc_match_table,
+	},
+};
+builtin_platform_driver(meson_ee_pwrc_driver);
-- 
2.22.0

[PATCH 3/5] arm64: meson-g12: add Everything-Else power domain controller

[Neil Armstrong]

Replace the VPU-centric power domain controller by the generic system-wide
Everything-Else power domain controller and setup the right power-domains
properties on the VPU, Ethernet & USB nodes.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 .../boot/dts/amlogic/meson-g12-common.dtsi    | 92 ++++++++++---------
 arch/arm64/boot/dts/amlogic/meson-g12a.dtsi   |  9 ++
 arch/arm64/boot/dts/amlogic/meson-g12b.dtsi   |  9 ++
 arch/arm64/boot/dts/amlogic/meson-sm1.dtsi    | 15 ++-
 4 files changed, 77 insertions(+), 48 deletions(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
index a921d6334e5b..8baa6318f180 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -1426,6 +1426,53 @@
 						clocks = <&xtal>;
 						clock-names = "xtal";
 					};
+
+					pwrc: power-controller {
+						compatible = "amlogic,meson-g12a-pwrc";
+						#power-domain-cells = <1>;
+						amlogic,ao-sysctrl = <&rti>;
+						resets = <&reset RESET_VIU>,
+							 <&reset RESET_VENC>,
+							 <&reset RESET_VCBUS>,
+							 <&reset RESET_BT656>,
+							 <&reset RESET_RDMA>,
+							 <&reset RESET_VENCI>,
+							 <&reset RESET_VENCP>,
+							 <&reset RESET_VDAC>,
+							 <&reset RESET_VDI6>,
+							 <&reset RESET_VENCL>,
+							 <&reset RESET_VID_LOCK>;
+						reset-names = "viu", "venc", "vcbus", "bt656",
+							      "rdma", "venci", "vencp", "vdac",
+							      "vdi6", "vencl", "vid_lock";
+						clocks = <&clkc CLKID_VPU>,
+							 <&clkc CLKID_VAPB>;
+						clock-names = "vpu", "vapb";
+						/*
+						 * VPU clocking is provided by two identical clock paths
+						 * VPU_0 and VPU_1 muxed to a single clock by a glitch
+						 * free mux to safely change frequency while running.
+						 * Same for VAPB but with a final gate after the glitch free mux.
+						 */
+						assigned-clocks = <&clkc CLKID_VPU_0_SEL>,
+								  <&clkc CLKID_VPU_0>,
+								  <&clkc CLKID_VPU>, /* Glitch free mux */
+								  <&clkc CLKID_VAPB_0_SEL>,
+								  <&clkc CLKID_VAPB_0>,
+								  <&clkc CLKID_VAPB_SEL>; /* Glitch free mux */
+						assigned-clock-parents = <&clkc CLKID_FCLK_DIV3>,
+									 <0>, /* Do Nothing */
+									 <&clkc CLKID_VPU_0>,
+									 <&clkc CLKID_FCLK_DIV4>,
+									 <0>, /* Do Nothing */
+									 <&clkc CLKID_VAPB_0>;
+						assigned-clock-rates = <0>, /* Do Nothing */
+								       <666666666>,
+								       <0>, /* Do Nothing */
+								       <0>, /* Do Nothing */
+								       <250000000>,
+								       <0>; /* Do Nothing */
+					};
 				};
 			};
 
@@ -1773,50 +1820,6 @@
 					clock-names = "xtal", "mpeg-clk";
 				};
 
-				pwrc_vpu: power-controller-vpu {
-					compatible = "amlogic,meson-g12a-pwrc-vpu";
-					#power-domain-cells = <0>;
-					amlogic,hhi-sysctrl = <&hhi>;
-					resets = <&reset RESET_VIU>,
-						 <&reset RESET_VENC>,
-						 <&reset RESET_VCBUS>,
-						 <&reset RESET_BT656>,
-						 <&reset RESET_RDMA>,
-						 <&reset RESET_VENCI>,
-						 <&reset RESET_VENCP>,
-						 <&reset RESET_VDAC>,
-						 <&reset RESET_VDI6>,
-						 <&reset RESET_VENCL>,
-						 <&reset RESET_VID_LOCK>;
-					clocks = <&clkc CLKID_VPU>,
-						 <&clkc CLKID_VAPB>;
-					clock-names = "vpu", "vapb";
-					/*
-					 * VPU clocking is provided by two identical clock paths
-					 * VPU_0 and VPU_1 muxed to a single clock by a glitch
-					 * free mux to safely change frequency while running.
-					 * Same for VAPB but with a final gate after the glitch free mux.
-					 */
-					assigned-clocks = <&clkc CLKID_VPU_0_SEL>,
-							  <&clkc CLKID_VPU_0>,
-							  <&clkc CLKID_VPU>, /* Glitch free mux */
-							  <&clkc CLKID_VAPB_0_SEL>,
-							  <&clkc CLKID_VAPB_0>,
-							  <&clkc CLKID_VAPB_SEL>; /* Glitch free mux */
-					assigned-clock-parents = <&clkc CLKID_FCLK_DIV3>,
-								 <0>, /* Do Nothing */
-								 <&clkc CLKID_VPU_0>,
-								 <&clkc CLKID_FCLK_DIV4>,
-								 <0>, /* Do Nothing */
-								 <&clkc CLKID_VAPB_0>;
-					assigned-clock-rates = <0>, /* Do Nothing */
-							       <666666666>,
-							       <0>, /* Do Nothing */
-							       <0>, /* Do Nothing */
-							       <250000000>,
-							       <0>; /* Do Nothing */
-				};
-
 				ao_pinctrl: pinctrl@14 {
 					compatible = "amlogic,meson-g12a-aobus-pinctrl";
 					#address-cells = <2>;
@@ -2169,7 +2172,6 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			amlogic,canvas = <&canvas>;
-			power-domains = <&pwrc_vpu>;
 
 			/* CVBS VDAC output port */
 			cvbs_vdac_port: port@0 {
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
index 733a9d46fc4b..eb5d177d7a99 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
@@ -4,6 +4,7 @@
  */
 
 #include "meson-g12-common.dtsi"
+#include <dt-bindings/power/meson-g12a-power.h>
 
 / {
 	compatible = "amlogic,g12a";
@@ -110,6 +111,14 @@
 	};
 };
 
+&ethmac {
+	power-domains = <&pwrc PWRC_G12A_ETH_ID>;
+};
+
+&vpu {
+	power-domains = <&pwrc PWRC_G12A_VPU_ID>;
+};
+
 &sd_emmc_a {
 	amlogic,dram-access-quirk;
 };
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12b.dtsi
index d5edbc1a1991..5628ccd54531 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12b.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12b.dtsi
@@ -5,6 +5,7 @@
  */
 
 #include "meson-g12-common.dtsi"
+#include <dt-bindings/power/meson-g12a-power.h>
 
 / {
 	compatible = "amlogic,g12b";
@@ -101,6 +102,14 @@
 	compatible = "amlogic,g12b-clkc";
 };
 
+&ethmac {
+	power-domains = <&pwrc PWRC_G12A_ETH_ID>;
+};
+
+&vpu {
+	power-domains = <&pwrc PWRC_G12A_VPU_ID>;
+};
+
 &sd_emmc_a {
 	amlogic,dram-access-quirk;
 };
diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi
index e902d4f9165f..37064d7f66c1 100644
--- a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi
@@ -5,6 +5,7 @@
  */
 
 #include "meson-g12-common.dtsi"
+#include <dt-bindings/power/meson-sm1-power.h>
 
 / {
 	compatible = "amlogic,sm1";
@@ -59,10 +60,18 @@
 	compatible = "amlogic,meson-sm1-clk-measure";
 };
 
-&pwrc_vpu {
-	status = "disabled";
+&ethmac {
+	power-domains = <&pwrc PWRC_SM1_ETH_ID>;
+};
+
+&pwrc {
+	compatible = "amlogic,meson-sm1-pwrc";
 };
 
 &vpu {
-	status = "disabled";
+	power-domains = <&pwrc PWRC_SM1_VPU_ID>;
+};
+
+&usb {
+	power-domains = <&pwrc PWRC_SM1_USB_ID>;
 };
-- 
2.22.0

[PATCH 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support

[Neil Armstrong]

Update compatible of the pwc-vpu node and add the HDMI support nodes
for the Amlogic SM1 Based SEI610 Board.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 .../boot/dts/amlogic/meson-sm1-sei610.dts     | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
index 12dab0ba2f26..66bd3bfbaf91 100644
--- a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
@@ -51,6 +51,17 @@
 		};
 	};
 
+	hdmi-connector {
+		compatible = "hdmi-connector";
+		type = "a";
+
+		port {
+			hdmi_connector_in: endpoint {
+				remote-endpoint = <&hdmi_tx_tmds_out>;
+			};
+		};
+	};
+
 	leds {
 		compatible = "gpio-leds";
 
@@ -177,6 +188,18 @@
 	phy-mode = "rmii";
 };
 
+&hdmi_tx {
+	status = "okay";
+	pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
+	pinctrl-names = "default";
+};
+
+&hdmi_tx_tmds_port {
+	hdmi_tx_tmds_out: endpoint {
+		remote-endpoint = <&hdmi_connector_in>;
+	};
+};
+
 &i2c3 {
 	status = "okay";
 	pinctrl-0 = <&i2c3_sda_a_pins>, <&i2c3_sck_a_pins>;
-- 
2.22.0

[PATCH 5/5] arm64: dts: meson-sm1-sei610: add USB support

[Neil Armstrong]

Add the USB properties for the Amlogic SM1 Based SEI610 Board in order to
support the USB DRD Type-C port and the USB3 Type A port.

The USB DRD Type-C controller uses the ID signal to toggle the USB role
between the DWC3 Host controller and the DWC2 Device controller.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
index 66bd3bfbaf91..36ac2e4b970d 100644
--- a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
@@ -321,3 +321,8 @@
 	pinctrl-0 = <&uart_ao_a_pins>;
 	pinctrl-names = "default";
 };
+
+&usb {
+	status = "okay";
+	dr_mode = "otg";
+};
-- 
2.22.0

Re: [PATCH 1/5] dt-bindings: power: add Amlogic Everything-Else power domains bindings

[Rob Herring]

On Wed, Aug 21, 2019 at 6:41 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Add the bindings for the Amlogic Everything-Else power domains,
> controlling the Everything-Else peripherals power domains.
>
> The bindings targets the Amlogic G12A and SM1 compatible SoCs,
> support for earlier SoCs will be added later.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  .../bindings/power/amlogic,meson-ee-pwrc.yaml | 93 +++++++++++++++++++
>  include/dt-bindings/power/meson-g12a-power.h  | 13 +++
>  include/dt-bindings/power/meson-sm1-power.h   | 18 ++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml
>  create mode 100644 include/dt-bindings/power/meson-g12a-power.h
>  create mode 100644 include/dt-bindings/power/meson-sm1-power.h

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller

[Kevin Hilman]

Neil Armstrong <narmstrong@baylibre.com> writes:

> Add support for the General Purpose Amlogic Everything-Else Power controller,
> with the first support for G12A and SM1 SoCs dedicated to the VPU, PCIe,
> USB, NNA, GE2D and Ethernet Power Domains.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

Nice!  Thanks for generalizing this.

A few comments/concerns below, but this is mostly ready.

> ---
>  drivers/soc/amlogic/Kconfig         |  11 +
>  drivers/soc/amlogic/Makefile        |   1 +
>  drivers/soc/amlogic/meson-ee-pwrc.c | 560 ++++++++++++++++++++++++++++
>  3 files changed, 572 insertions(+)
>  create mode 100644 drivers/soc/amlogic/meson-ee-pwrc.c
>
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index 23bfb8ef4fdb..bc2c912949bd 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -37,6 +37,17 @@ config MESON_GX_PM_DOMAINS
>  	  Say yes to expose Amlogic Meson GX Power Domains as
>  	  Generic Power Domains.
>  
> +config MESON_EE_PM_DOMAINS
> +	bool "Amlogic Meson Everything-Else Power Domains driver"
> +	depends on ARCH_MESON || COMPILE_TEST
> +	depends on PM && OF
> +	default ARCH_MESON
> +	select PM_GENERIC_DOMAINS
> +	select PM_GENERIC_DOMAINS_OF
> +	help
> +	  Say yes to expose Amlogic Meson Everything-Else Power Domains as
> +	  Generic Power Domains.
> +
>  config MESON_MX_SOCINFO
>  	bool "Amlogic Meson MX SoC Information driver"
>  	depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index f2e4ed171297..de79d044b545 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_MESON_CLK_MEASURE) += meson-clk-measure.o
>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> +obj-$(CONFIG_MESON_EE_PM_DOMAINS) += meson-ee-pwrc.o
> diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c
> new file mode 100644
> index 000000000000..7159888c850b
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-ee-pwrc.c
> @@ -0,0 +1,560 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2019 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + */
> +
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/bitfield.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/reset.h>
> +#include <linux/clk.h>
> +#include <dt-bindings/power/meson-g12a-power.h>
> +#include <dt-bindings/power/meson-sm1-power.h>
> +
> +/* AO Offsets */
> +
> +#define AO_RTI_GEN_PWR_SLEEP0		(0x3a << 2)
> +#define AO_RTI_GEN_PWR_ISO0		(0x3b << 2)
> +
> +/* HHI Offsets */
> +
> +#define HHI_MEM_PD_REG0			(0x40 << 2)
> +#define HHI_VPU_MEM_PD_REG0		(0x41 << 2)
> +#define HHI_VPU_MEM_PD_REG1		(0x42 << 2)
> +#define HHI_VPU_MEM_PD_REG3		(0x43 << 2)
> +#define HHI_VPU_MEM_PD_REG4		(0x44 << 2)
> +#define HHI_AUDIO_MEM_PD_REG0		(0x45 << 2)
> +#define HHI_NANOQ_MEM_PD_REG0		(0x46 << 2)
> +#define HHI_NANOQ_MEM_PD_REG1		(0x47 << 2)
> +#define HHI_VPU_MEM_PD_REG2		(0x4d << 2)
> +
> +struct meson_ee_pwrc;
> +struct meson_ee_pwrc_domain;
> +
> +struct meson_ee_pwrc_mem_domain {
> +	unsigned int reg;
> +	unsigned int mask;
> +};
> +
> +struct meson_ee_pwrc_top_domain {
> +	unsigned int sleep_reg;
> +	unsigned int sleep_mask;
> +	unsigned int iso_reg;
> +	unsigned int iso_mask;
> +};
> +
> +struct meson_ee_pwrc_domain_desc {
> +	char *name;
> +	char **reset_names;
> +	unsigned int reset_names_count;
> +	char **clk_names;
> +	unsigned int clk_names_count;
> +	struct meson_ee_pwrc_top_domain *top_pd;
> +	unsigned int mem_pd_count;
> +	struct meson_ee_pwrc_mem_domain *mem_pd;
> +	bool (*get_power)(struct meson_ee_pwrc_domain *pwrc_domain);
> +};
> +
> +struct meson_ee_pwrc_domain_data {
> +	unsigned int count;
> +	struct meson_ee_pwrc_domain_desc *domains;
> +};
> +
> +/* Clock and Resets lists */
> +
> +static char *g12a_pwrc_vpu_resets[] = {
> +	"viu", "venc", "vcbus", "bt656",
> +	"rdma", "venci", "vencp", "vdac",
> +	"vdi6", "vencl", "vid_lock",
> +};
> +
> +static char *g12a_pwrc_vpu_clks[] = {
> +	"vpu", "vapb",
> +};
> +
> +/* TOP Power Domains */
> +
> +static struct meson_ee_pwrc_top_domain g12a_pwrc_vpu = {
> +	.sleep_reg = AO_RTI_GEN_PWR_SLEEP0,
> +	.sleep_mask = BIT(8),
> +	.iso_reg = AO_RTI_GEN_PWR_SLEEP0,
> +	.iso_mask = BIT(9),
> +};
> +
> +#define SM1_EE_PD(__bit)					\
> +	{							\
> +		.sleep_reg = AO_RTI_GEN_PWR_SLEEP0, 		\
> +		.sleep_mask = BIT(__bit), 			\
> +		.iso_reg = AO_RTI_GEN_PWR_ISO0, 		\
> +		.iso_mask = BIT(__bit), 			\
> +	}
> +
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_vpu = SM1_EE_PD(8);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_nna = SM1_EE_PD(16);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_usb = SM1_EE_PD(17);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_pci = SM1_EE_PD(18);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_ge2d = SM1_EE_PD(19);
> +
> +/* Memory PD Domains */
> +
> +#define VPU_MEMPD(__reg)					\
> +	{ __reg, GENMASK(1, 0) },				\
> +	{ __reg, GENMASK(3, 2) },				\
> +	{ __reg, GENMASK(5, 4) },				\
> +	{ __reg, GENMASK(7, 6) },				\
> +	{ __reg, GENMASK(9, 8) },				\
> +	{ __reg, GENMASK(11, 10) },				\
> +	{ __reg, GENMASK(13, 12) },				\
> +	{ __reg, GENMASK(15, 14) },				\
> +	{ __reg, GENMASK(17, 16) },				\
> +	{ __reg, GENMASK(19, 18) },				\
> +	{ __reg, GENMASK(21, 20) },				\
> +	{ __reg, GENMASK(23, 22) },				\
> +	{ __reg, GENMASK(25, 24) },				\
> +	{ __reg, GENMASK(27, 26) },				\
> +	{ __reg, GENMASK(29, 28) },				\
> +	{ __reg, GENMASK(31, 30) }
> +
> +#define VPU_HHI_MEMPD(__reg)					\
> +	{ __reg, BIT(8) },					\
> +	{ __reg, BIT(9) },					\
> +	{ __reg, BIT(10) },					\
> +	{ __reg, BIT(11) },					\
> +	{ __reg, BIT(12) },					\
> +	{ __reg, BIT(13) },					\
> +	{ __reg, BIT(14) },					\
> +	{ __reg, BIT(15) }
> +
> +static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_vpu[] = {
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
> +	VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
> +};
> +
> +static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_eth[] = {
> +	{ HHI_MEM_PD_REG0, GENMASK(3, 2) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_vpu[] = {
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG3),
> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(1, 0) },
> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(3, 2) },
> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(5, 4) },
> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(7, 6) },
> +	VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_nna[] = {
> +	{ HHI_NANOQ_MEM_PD_REG0, 0xff },
> +	{ HHI_NANOQ_MEM_PD_REG1, 0xff },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_usb[] = {
> +	{ HHI_MEM_PD_REG0, GENMASK(31, 30) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_pcie[] = {
> +	{ HHI_MEM_PD_REG0, GENMASK(29, 26) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_ge2d[] = {
> +	{ HHI_MEM_PD_REG0, GENMASK(25, 18) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_audio[] = {
> +	{ HHI_MEM_PD_REG0, GENMASK(5, 4) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(1, 0) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(3, 2) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(5, 4) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(7, 6) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(13, 12) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(15, 14) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(17, 16) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(19, 18) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(21, 20) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(23, 22) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(25, 24) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(27, 26) },
> +};
> +
> +#define VPU_PD(__name, __resets, __clks, __top_pd, __mem, __get_power)	\
> +	{								\
> +		.name = __name,						\
> +		.reset_names_count = ARRAY_SIZE(__resets),		\
> +		.reset_names = __resets,				\
> +		.clk_names_count = ARRAY_SIZE(__clks),			\
> +		.clk_names = __clks,					\
> +		.top_pd = __top_pd,					\
> +		.mem_pd_count = ARRAY_SIZE(__mem),			\
> +		.mem_pd = __mem,					\
> +		.get_power = __get_power,				\
> +	}
> +
> +#define TOP_PD(__name, __top_pd, __mem)					\
> +	{								\
> +		.name = __name,						\
> +		.top_pd = __top_pd,					\
> +		.mem_pd_count = ARRAY_SIZE(__mem),			\
> +		.mem_pd = __mem,					\
> +	}

Why can't the TOP_PD domains also have a __get_power().  Shouldn't we
just be able to check the sleep_reg/sleep_mask like in the VPU case?

Also, for readability, I think the arguments to VPU_PD and TOP_PD to
have the same order, at least for the common ones.  IOW, __name,
__top_pd, __mem should be first.

> +#define MEM_PD(__name, __mem)						\
> +	TOP_PD(__name, NULL, __mem)
> +
> +static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain);
> +
> +static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
> +	[PWRC_G12A_VPU_ID]  = VPU_PD("VPU", g12a_pwrc_vpu_resets,
> +				     g12a_pwrc_vpu_clks, &g12a_pwrc_vpu,
> +				     g12a_pwrc_mem_vpu,
> +				     pwrc_vpu_get_power),
> +	[PWRC_G12A_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
> +};
> +
> +static struct meson_ee_pwrc_domain_desc sm1_pwrc_domains[] = {
> +	[PWRC_SM1_VPU_ID]  = VPU_PD("VPU", g12a_pwrc_vpu_resets,
> +				    g12a_pwrc_vpu_clks, &sm1_pwrc_vpu,
> +				    sm1_pwrc_mem_vpu,
> +				    pwrc_vpu_get_power),
> +	[PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna),
> +	[PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb),
> +	[PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie),
> +	[PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d),
> +	[PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
> +	[PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
> +};
> +
> +struct meson_ee_pwrc_domain {
> +	struct generic_pm_domain base;
> +	bool enabled;
> +	struct meson_ee_pwrc *pwrc;
> +	struct meson_ee_pwrc_domain_desc desc;
> +	struct clk **clks;
> +	int num_clks;
> +	struct reset_control **rstc;
> +	int num_rstc;
> +};
> +
> +struct meson_ee_pwrc {
> +	struct regmap *regmap_ao;
> +	struct regmap *regmap_hhi;
> +	struct meson_ee_pwrc_domain *domains;
> +	struct genpd_onecell_data xlate;
> +};
> +
> +static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> +	u32 reg;
> +
> +	regmap_read(pwrc_domain->pwrc->regmap_ao,
> +		    pwrc_domain->desc.top_pd->sleep_reg, &reg);
> +
> +	return (reg & pwrc_domain->desc.top_pd->sleep_mask);
> +}
> +
> +static int meson_ee_reset_assert(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> +	int i, ret;
> +
> +	for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
> +		ret = reset_control_assert(pwrc_domain->rstc[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int meson_ee_reset_deassert(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> +	int i, ret;
> +
> +	for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
> +		ret = reset_control_deassert(pwrc_domain->rstc[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

You should use the reset_array functions, then you don't need these helpers.

> +static int meson_ee_clk_disable(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> +	int i;
> +
> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
> +		clk_disable(pwrc_domain->clks[i]);
> +
> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
> +		clk_unprepare(pwrc_domain->clks[i]);
> +
> +	return 0;
> +}
> +
> +static int meson_ee_clk_enable(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> +	int i, ret;
> +
> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
> +		ret = clk_prepare(pwrc_domain->clks[i]);
> +		if (ret)
> +			goto fail_prepare;
> +	}
> +
> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
> +		ret = clk_enable(pwrc_domain->clks[i]);
> +		if (ret)
> +			goto fail_enable;
> +	}
> +
> +	return 0;
> +fail_enable:
> +	while (--i)
> +		clk_disable(pwrc_domain->clks[i]);
> +
> +	/* Unprepare all clocks */
> +	i = pwrc_domain->num_clks;
> +
> +fail_prepare:
> +	while (--i)
> +		clk_unprepare(pwrc_domain->clks[i]);
> +
> +	return ret;
> +}

Both the clk enable and disable functions above are just open-coding of
the clk_bulk equivalents.  Please use clk_bulk_*, then you don't need
these helpers.  (c.f. the RFT patch I did to convert the old driver to
clk_bulk[1])

> +static int meson_ee_pwrc_off(struct generic_pm_domain *domain)
> +{
> +	struct meson_ee_pwrc_domain *pwrc_domain =
> +		container_of(domain, struct meson_ee_pwrc_domain, base);
> +	int i;
> +
> +	if (pwrc_domain->desc.top_pd)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> +				   pwrc_domain->desc.top_pd->sleep_reg,
> +				   pwrc_domain->desc.top_pd->sleep_mask,
> +				   pwrc_domain->desc.top_pd->sleep_mask);
> +	udelay(20);
> +
> +	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
> +				   pwrc_domain->desc.mem_pd[i].reg,
> +				   pwrc_domain->desc.mem_pd[i].mask,
> +				   pwrc_domain->desc.mem_pd[i].mask);
> +
> +	udelay(20);
> +
> +	if (pwrc_domain->desc.top_pd)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> +				   pwrc_domain->desc.top_pd->iso_reg,
> +				   pwrc_domain->desc.top_pd->iso_mask,
> +				   pwrc_domain->desc.top_pd->iso_mask);
> +
> +	if (pwrc_domain->num_clks) {
> +		msleep(20);
> +		meson_ee_clk_disable(pwrc_domain);
> +	}
> +
> +	return 0;
> +}
> +
> +static int meson_ee_pwrc_on(struct generic_pm_domain *domain)
> +{
> +	struct meson_ee_pwrc_domain *pwrc_domain =
> +		container_of(domain, struct meson_ee_pwrc_domain, base);
> +	int i, ret;
> +
> +	if (pwrc_domain->desc.top_pd)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> +				   pwrc_domain->desc.top_pd->sleep_reg,
> +				   pwrc_domain->desc.top_pd->sleep_mask, 0);
> +	udelay(20);
> +
> +	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
> +				   pwrc_domain->desc.mem_pd[i].reg,
> +				   pwrc_domain->desc.mem_pd[i].mask, 0);
> +
> +	udelay(20);
> +
> +	ret = meson_ee_reset_assert(pwrc_domain);
> +	if (ret)
> +		return ret;
> +
> +	if (pwrc_domain->desc.top_pd)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> +				   pwrc_domain->desc.top_pd->iso_reg,
> +				   pwrc_domain->desc.top_pd->iso_mask, 0);
> +
> +	ret = meson_ee_reset_deassert(pwrc_domain);
> +	if (ret)
> +		return ret;
> +
> +	return meson_ee_clk_enable(pwrc_domain);
> +}
> +
> +static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
> +				     struct meson_ee_pwrc *sm1_pwrc,
> +				     struct meson_ee_pwrc_domain *dom)
> +{
> +	dom->pwrc = sm1_pwrc;
> +	dom->num_rstc = dom->desc.reset_names_count;
> +	dom->num_clks = dom->desc.clk_names_count;
> +
> +	if (dom->num_rstc) {
> +		int rst;
> +
> +		dom->rstc = devm_kcalloc(&pdev->dev, dom->num_rstc,
> +				sizeof(struct reset_control *),	GFP_KERNEL);
> +		if (!dom->rstc)
> +			return -ENOMEM;
> +
> +		for (rst = 0 ; rst < dom->num_rstc ; ++rst) {
> +			dom->rstc[rst] = devm_reset_control_get_exclusive(
> +					&pdev->dev,
> +					dom->desc.reset_names[rst]);
> +			if (IS_ERR(dom->rstc[rst]))
> +				return PTR_ERR(dom->rstc[rst]);
> +		}

Why not simplify and use the helpers that get multiple reset lines (like
devm_reset_control_array_get() used in meson-gx-pwrc-vpu.c)?

You could also use reset_control_get_count() and compare to the expected
number (dom->num_rstc).

> +	}
> +
> +	if (dom->num_clks) {
> +		int clk;
> +
> +		dom->clks = devm_kcalloc(&pdev->dev, dom->num_clks,
> +				sizeof(struct clk *), GFP_KERNEL);
> +		if (!dom->clks)
> +			return -ENOMEM;
> +
> +		for (clk = 0 ; clk < dom->num_clks ; ++clk) {
> +			dom->clks[clk] = devm_clk_get(&pdev->dev,
> +					dom->desc.clk_names[clk]);
> +			if (IS_ERR(dom->clks[clk]))
> +				return PTR_ERR(dom->clks[clk]);
> +		}
> +	}

Please use clk_bulk API, and then just double-check that the number of
clocks found matches the expected number.

> +	dom->base.name = dom->desc.name;
> +	dom->base.power_on = meson_ee_pwrc_on;
> +	dom->base.power_off = meson_ee_pwrc_off;
> +
> +	if (dom->desc.get_power) {
> +		bool powered_off = dom->desc.get_power(dom);

nit: insert blank line here

More importantly, we defintely will have problem here in the
!powered_off case.  TL;DR; the driver's state does not match the actual
hardware state.

When powered_off = false, you're telling the genpd core that this domain
is already turned on.  However, you haven't called _pwrc_on() yet for
the domain, which means internal state of the driver for this domain
(e.g. clock enables, resets, etc.) is not in sync with the HW.  On
SEI610 this case is happending for the VPU, which seems to be enabled by
u-boot, so this driver detects it as already on, which is fine.  But...

Remember that the ->power_off() function will be called during suspend,
and will lead to the clk unprepare/disable calls.  However, for domains
that are detected as on (!powered_off), clk prepare/enable will never
have been called, so that when suspend happens, you'll get "already
unprepared" errors from the clock core

IOW, I think you need something like this here:

		if (!powered_off)
			meson_ee_pwrc_on(&dom->base);

so that the internal state of clock fwk etc. matches the detected state
of the HW.  The problem with that simple fix, at least for the VPU is
that it might cause us to lose any existing display or framebuffer
console that's on-going.  Probably needs some testing.

Anyways, to see what I mean, try suspend/resume (you can test this
series on my integ branch with "rtcwake -d rtc0 -m mem -s4") and you'll
see error splats from the clock core during suspend.



> +		pm_genpd_init(&dom->base, &pm_domain_always_on_gov,
> +			      powered_off);

> +	} else
> +		pm_genpd_init(&dom->base, NULL, true);

nit: the else clause should also have {} to match the if
(c.f. CodingStyle)

Why do you force the always-on governor in the case where ->get_power()
exists, but not the other?

If you force that, then for any devices connected to these domains that
use runtime PM, they will never turn off the domain when it's idle.
IOW, these domains will only ever be turned off on system-wide
suspend/resume.

IMO, unless there's a good reason not to, you should pass NULL for the
governor.

> +	return 0;
> +}
> +
> +static int meson_ee_pwrc_probe(struct platform_device *pdev)
> +{
> +	const struct meson_ee_pwrc_domain_data *match;
> +	struct regmap *regmap_ao, *regmap_hhi;
> +	struct meson_ee_pwrc *sm1_pwrc;

Why the sm1_ prefix throughout this code?  This is now generalized for
more SoCs, so shouldn't be sm1.  I'm assuming this is just left-over
from the original version.  IMO, just called it pwrc.

> +	int i, ret;
> +
> +	match = of_device_get_match_data(&pdev->dev);
> +	if (!match) {
> +		dev_err(&pdev->dev, "failed to get match data\n");
> +		return -ENODEV;
> +	}
> +
> +	sm1_pwrc = devm_kzalloc(&pdev->dev, sizeof(*sm1_pwrc), GFP_KERNEL);
> +	if (!sm1_pwrc)
> +		return -ENOMEM;
> +
> +	sm1_pwrc->xlate.domains =
> +		devm_kcalloc(&pdev->dev,
> +			     match->count,
> +			     sizeof(*sm1_pwrc->xlate.domains),
> +			     GFP_KERNEL);
> +	if (!sm1_pwrc->xlate.domains)
> +		return -ENOMEM;
> +
> +	sm1_pwrc->domains =
> +		devm_kcalloc(&pdev->dev,

nit: this line probably doesn't need to be wrapped.

> +			     match->count,
> +			     sizeof(*sm1_pwrc->domains),
> +			     GFP_KERNEL);
> +	if (!sm1_pwrc->domains)
> +		return -ENOMEM;
> +
> +	sm1_pwrc->xlate.num_domains = match->count;
> +
> +	regmap_hhi = syscon_node_to_regmap(of_get_parent(pdev->dev.of_node));
> +	if (IS_ERR(regmap_hhi)) {
> +		dev_err(&pdev->dev, "failed to get HHI regmap\n");
> +		return PTR_ERR(regmap_hhi);
> +	}
> +
> +	regmap_ao = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +						    "amlogic,ao-sysctrl");
> +	if (IS_ERR(regmap_ao)) {
> +		dev_err(&pdev->dev, "failed to get AO regmap\n");
> +		return PTR_ERR(regmap_ao);
> +	}
> +
> +	sm1_pwrc->regmap_ao = regmap_ao;
> +	sm1_pwrc->regmap_hhi = regmap_hhi;
> +
> +	platform_set_drvdata(pdev, sm1_pwrc);
> +
> +	for (i = 0 ; i < match->count ; ++i) {
> +		struct meson_ee_pwrc_domain *dom = &sm1_pwrc->domains[i];
> +
> +		memcpy(&dom->desc, &match->domains[i], sizeof(dom->desc));
> +
> +		ret = meson_ee_pwrc_init_domain(pdev, sm1_pwrc, dom);
> +		if (ret)
> +			return ret;
> +
> +		sm1_pwrc->xlate.domains[i] = &dom->base;
> +	}
> +
> +	of_genpd_add_provider_onecell(pdev->dev.of_node, &sm1_pwrc->xlate);
> +
> +	return 0;
> +}
> +
> +static struct meson_ee_pwrc_domain_data meson_ee_g12a_pwrc_data = {
> +	.count = ARRAY_SIZE(g12a_pwrc_domains),
> +	.domains = g12a_pwrc_domains,
> +};
> +
> +static struct meson_ee_pwrc_domain_data meson_ee_sm1_pwrc_data = {
> +	.count = ARRAY_SIZE(sm1_pwrc_domains),
> +	.domains = sm1_pwrc_domains,
> +};
> +
> +static const struct of_device_id meson_ee_pwrc_match_table[] = {
> +	{
> +		.compatible = "amlogic,meson-g12a-pwrc",
> +		.data = &meson_ee_g12a_pwrc_data,
> +	},
> +	{
> +		.compatible = "amlogic,meson-sm1-pwrc",
> +		.data = &meson_ee_sm1_pwrc_data,
> +	},
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver meson_ee_pwrc_driver = {
> +	.probe	= meson_ee_pwrc_probe,
> +	.driver = {
> +		.name		= "meson_ee_pwrc",
> +		.of_match_table	= meson_ee_pwrc_match_table,
> +	},
> +};
> +builtin_platform_driver(meson_ee_pwrc_driver);
> -- 
> 2.22.0

Kevin

[1] https://lore.kernel.org/linux-amlogic/20190809230904.28747-1-khilman@baylibre.com/

Re: [PATCH 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support

[Kevin Hilman]

Neil Armstrong <narmstrong@baylibre.com> writes:

> Update compatible of the pwc-vpu node and add the HDMI support nodes
> for the Amlogic SM1 Based SEI610 Board.

I think this changelog is out of date.  It's not doing anything with the
VPU pwrc node.

Kevin

> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  .../boot/dts/amlogic/meson-sm1-sei610.dts     | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
> index 12dab0ba2f26..66bd3bfbaf91 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
> @@ -51,6 +51,17 @@
>  		};
>  	};
>  
> +	hdmi-connector {
> +		compatible = "hdmi-connector";
> +		type = "a";
> +
> +		port {
> +			hdmi_connector_in: endpoint {
> +				remote-endpoint = <&hdmi_tx_tmds_out>;
> +			};
> +		};
> +	};
> +
>  	leds {
>  		compatible = "gpio-leds";
>  
> @@ -177,6 +188,18 @@
>  	phy-mode = "rmii";
>  };
>  
> +&hdmi_tx {
> +	status = "okay";
> +	pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
> +	pinctrl-names = "default";
> +};
> +
> +&hdmi_tx_tmds_port {
> +	hdmi_tx_tmds_out: endpoint {
> +		remote-endpoint = <&hdmi_connector_in>;
> +	};
> +};
> +
>  &i2c3 {
>  	status = "okay";
>  	pinctrl-0 = <&i2c3_sda_a_pins>, <&i2c3_sck_a_pins>;
> -- 
> 2.22.0

Re: [PATCH 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support

[Neil Armstrong]

On 22/08/2019 01:31, Kevin Hilman wrote:
> Neil Armstrong <narmstrong@baylibre.com> writes:
> 
>> Update compatible of the pwc-vpu node and add the HDMI support nodes
>> for the Amlogic SM1 Based SEI610 Board.
> 
> I think this changelog is out of date.  It's not doing anything with the
> VPU pwrc node.

Exact, thanks for pointing it

> 
> Kevin
> 
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  .../boot/dts/amlogic/meson-sm1-sei610.dts     | 23 +++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
>> index 12dab0ba2f26..66bd3bfbaf91 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
>> +++ b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
>> @@ -51,6 +51,17 @@
>>  		};
>>  	};
>>  
>> +	hdmi-connector {
>> +		compatible = "hdmi-connector";
>> +		type = "a";
>> +
>> +		port {
>> +			hdmi_connector_in: endpoint {
>> +				remote-endpoint = <&hdmi_tx_tmds_out>;
>> +			};
>> +		};
>> +	};
>> +
>>  	leds {
>>  		compatible = "gpio-leds";
>>  
>> @@ -177,6 +188,18 @@
>>  	phy-mode = "rmii";
>>  };
>>  
>> +&hdmi_tx {
>> +	status = "okay";
>> +	pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
>> +	pinctrl-names = "default";
>> +};
>> +
>> +&hdmi_tx_tmds_port {
>> +	hdmi_tx_tmds_out: endpoint {
>> +		remote-endpoint = <&hdmi_connector_in>;
>> +	};
>> +};
>> +
>>  &i2c3 {
>>  	status = "okay";
>>  	pinctrl-0 = <&i2c3_sda_a_pins>, <&i2c3_sck_a_pins>;
>> -- 
>> 2.22.0

Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller

[Neil Armstrong]

On 22/08/2019 01:16, Kevin Hilman wrote:
> Neil Armstrong <narmstrong@baylibre.com> writes:
> 
>> Add support for the General Purpose Amlogic Everything-Else Power controller,
>> with the first support for G12A and SM1 SoCs dedicated to the VPU, PCIe,
>> USB, NNA, GE2D and Ethernet Power Domains.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> Nice!  Thanks for generalizing this.
> 
> A few comments/concerns below, but this is mostly ready.
> 
>> ---
>>  drivers/soc/amlogic/Kconfig         |  11 +
>>  drivers/soc/amlogic/Makefile        |   1 +
>>  drivers/soc/amlogic/meson-ee-pwrc.c | 560 ++++++++++++++++++++++++++++
>>  3 files changed, 572 insertions(+)
>>  create mode 100644 drivers/soc/amlogic/meson-ee-pwrc.c
>>
>> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
>> index 23bfb8ef4fdb..bc2c912949bd 100644
>> --- a/drivers/soc/amlogic/Kconfig
>> +++ b/drivers/soc/amlogic/Kconfig
>> @@ -37,6 +37,17 @@ config MESON_GX_PM_DOMAINS
>>  	  Say yes to expose Amlogic Meson GX Power Domains as
>>  	  Generic Power Domains.
>>  
>> +config MESON_EE_PM_DOMAINS
>> +	bool "Amlogic Meson Everything-Else Power Domains driver"
>> +	depends on ARCH_MESON || COMPILE_TEST
>> +	depends on PM && OF
>> +	default ARCH_MESON
>> +	select PM_GENERIC_DOMAINS
>> +	select PM_GENERIC_DOMAINS_OF
>> +	help
>> +	  Say yes to expose Amlogic Meson Everything-Else Power Domains as
>> +	  Generic Power Domains.
>> +
>>  config MESON_MX_SOCINFO
>>  	bool "Amlogic Meson MX SoC Information driver"
>>  	depends on ARCH_MESON || COMPILE_TEST
>> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
>> index f2e4ed171297..de79d044b545 100644
>> --- a/drivers/soc/amlogic/Makefile
>> +++ b/drivers/soc/amlogic/Makefile
>> @@ -4,3 +4,4 @@ obj-$(CONFIG_MESON_CLK_MEASURE) += meson-clk-measure.o
>>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
>> +obj-$(CONFIG_MESON_EE_PM_DOMAINS) += meson-ee-pwrc.o
>> diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c
>> new file mode 100644
>> index 000000000000..7159888c850b
>> --- /dev/null
>> +++ b/drivers/soc/amlogic/meson-ee-pwrc.c
>> @@ -0,0 +1,560 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2019 BayLibre, SAS
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + */
>> +
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/reset.h>
>> +#include <linux/clk.h>
>> +#include <dt-bindings/power/meson-g12a-power.h>
>> +#include <dt-bindings/power/meson-sm1-power.h>
>> +
>> +/* AO Offsets */
>> +
>> +#define AO_RTI_GEN_PWR_SLEEP0		(0x3a << 2)
>> +#define AO_RTI_GEN_PWR_ISO0		(0x3b << 2)
>> +
>> +/* HHI Offsets */
>> +
>> +#define HHI_MEM_PD_REG0			(0x40 << 2)
>> +#define HHI_VPU_MEM_PD_REG0		(0x41 << 2)
>> +#define HHI_VPU_MEM_PD_REG1		(0x42 << 2)
>> +#define HHI_VPU_MEM_PD_REG3		(0x43 << 2)
>> +#define HHI_VPU_MEM_PD_REG4		(0x44 << 2)
>> +#define HHI_AUDIO_MEM_PD_REG0		(0x45 << 2)
>> +#define HHI_NANOQ_MEM_PD_REG0		(0x46 << 2)
>> +#define HHI_NANOQ_MEM_PD_REG1		(0x47 << 2)
>> +#define HHI_VPU_MEM_PD_REG2		(0x4d << 2)
>> +
>> +struct meson_ee_pwrc;
>> +struct meson_ee_pwrc_domain;
>> +
>> +struct meson_ee_pwrc_mem_domain {
>> +	unsigned int reg;
>> +	unsigned int mask;
>> +};
>> +
>> +struct meson_ee_pwrc_top_domain {
>> +	unsigned int sleep_reg;
>> +	unsigned int sleep_mask;
>> +	unsigned int iso_reg;
>> +	unsigned int iso_mask;
>> +};
>> +
>> +struct meson_ee_pwrc_domain_desc {
>> +	char *name;
>> +	char **reset_names;
>> +	unsigned int reset_names_count;
>> +	char **clk_names;
>> +	unsigned int clk_names_count;
>> +	struct meson_ee_pwrc_top_domain *top_pd;
>> +	unsigned int mem_pd_count;
>> +	struct meson_ee_pwrc_mem_domain *mem_pd;
>> +	bool (*get_power)(struct meson_ee_pwrc_domain *pwrc_domain);
>> +};
>> +
>> +struct meson_ee_pwrc_domain_data {
>> +	unsigned int count;
>> +	struct meson_ee_pwrc_domain_desc *domains;
>> +};
>> +
>> +/* Clock and Resets lists */
>> +
>> +static char *g12a_pwrc_vpu_resets[] = {
>> +	"viu", "venc", "vcbus", "bt656",
>> +	"rdma", "venci", "vencp", "vdac",
>> +	"vdi6", "vencl", "vid_lock",
>> +};
>> +
>> +static char *g12a_pwrc_vpu_clks[] = {
>> +	"vpu", "vapb",
>> +};
>> +
>> +/* TOP Power Domains */
>> +
>> +static struct meson_ee_pwrc_top_domain g12a_pwrc_vpu = {
>> +	.sleep_reg = AO_RTI_GEN_PWR_SLEEP0,
>> +	.sleep_mask = BIT(8),
>> +	.iso_reg = AO_RTI_GEN_PWR_SLEEP0,
>> +	.iso_mask = BIT(9),
>> +};
>> +
>> +#define SM1_EE_PD(__bit)					\
>> +	{							\
>> +		.sleep_reg = AO_RTI_GEN_PWR_SLEEP0, 		\
>> +		.sleep_mask = BIT(__bit), 			\
>> +		.iso_reg = AO_RTI_GEN_PWR_ISO0, 		\
>> +		.iso_mask = BIT(__bit), 			\
>> +	}
>> +
>> +static struct meson_ee_pwrc_top_domain sm1_pwrc_vpu = SM1_EE_PD(8);
>> +static struct meson_ee_pwrc_top_domain sm1_pwrc_nna = SM1_EE_PD(16);
>> +static struct meson_ee_pwrc_top_domain sm1_pwrc_usb = SM1_EE_PD(17);
>> +static struct meson_ee_pwrc_top_domain sm1_pwrc_pci = SM1_EE_PD(18);
>> +static struct meson_ee_pwrc_top_domain sm1_pwrc_ge2d = SM1_EE_PD(19);
>> +
>> +/* Memory PD Domains */
>> +
>> +#define VPU_MEMPD(__reg)					\
>> +	{ __reg, GENMASK(1, 0) },				\
>> +	{ __reg, GENMASK(3, 2) },				\
>> +	{ __reg, GENMASK(5, 4) },				\
>> +	{ __reg, GENMASK(7, 6) },				\
>> +	{ __reg, GENMASK(9, 8) },				\
>> +	{ __reg, GENMASK(11, 10) },				\
>> +	{ __reg, GENMASK(13, 12) },				\
>> +	{ __reg, GENMASK(15, 14) },				\
>> +	{ __reg, GENMASK(17, 16) },				\
>> +	{ __reg, GENMASK(19, 18) },				\
>> +	{ __reg, GENMASK(21, 20) },				\
>> +	{ __reg, GENMASK(23, 22) },				\
>> +	{ __reg, GENMASK(25, 24) },				\
>> +	{ __reg, GENMASK(27, 26) },				\
>> +	{ __reg, GENMASK(29, 28) },				\
>> +	{ __reg, GENMASK(31, 30) }
>> +
>> +#define VPU_HHI_MEMPD(__reg)					\
>> +	{ __reg, BIT(8) },					\
>> +	{ __reg, BIT(9) },					\
>> +	{ __reg, BIT(10) },					\
>> +	{ __reg, BIT(11) },					\
>> +	{ __reg, BIT(12) },					\
>> +	{ __reg, BIT(13) },					\
>> +	{ __reg, BIT(14) },					\
>> +	{ __reg, BIT(15) }
>> +
>> +static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_vpu[] = {
>> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
>> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
>> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
>> +	VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
>> +};
>> +
>> +static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_eth[] = {
>> +	{ HHI_MEM_PD_REG0, GENMASK(3, 2) },
>> +};
>> +
>> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_vpu[] = {
>> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
>> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
>> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
>> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG3),
>> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(1, 0) },
>> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(3, 2) },
>> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(5, 4) },
>> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(7, 6) },
>> +	VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
>> +};
>> +
>> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_nna[] = {
>> +	{ HHI_NANOQ_MEM_PD_REG0, 0xff },
>> +	{ HHI_NANOQ_MEM_PD_REG1, 0xff },
>> +};
>> +
>> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_usb[] = {
>> +	{ HHI_MEM_PD_REG0, GENMASK(31, 30) },
>> +};
>> +
>> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_pcie[] = {
>> +	{ HHI_MEM_PD_REG0, GENMASK(29, 26) },
>> +};
>> +
>> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_ge2d[] = {
>> +	{ HHI_MEM_PD_REG0, GENMASK(25, 18) },
>> +};
>> +
>> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_audio[] = {
>> +	{ HHI_MEM_PD_REG0, GENMASK(5, 4) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(1, 0) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(3, 2) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(5, 4) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(7, 6) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(13, 12) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(15, 14) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(17, 16) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(19, 18) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(21, 20) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(23, 22) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(25, 24) },
>> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(27, 26) },
>> +};
>> +
>> +#define VPU_PD(__name, __resets, __clks, __top_pd, __mem, __get_power)	\
>> +	{								\
>> +		.name = __name,						\
>> +		.reset_names_count = ARRAY_SIZE(__resets),		\
>> +		.reset_names = __resets,				\
>> +		.clk_names_count = ARRAY_SIZE(__clks),			\
>> +		.clk_names = __clks,					\
>> +		.top_pd = __top_pd,					\
>> +		.mem_pd_count = ARRAY_SIZE(__mem),			\
>> +		.mem_pd = __mem,					\
>> +		.get_power = __get_power,				\
>> +	}
>> +
>> +#define TOP_PD(__name, __top_pd, __mem)					\
>> +	{								\
>> +		.name = __name,						\
>> +		.top_pd = __top_pd,					\
>> +		.mem_pd_count = ARRAY_SIZE(__mem),			\
>> +		.mem_pd = __mem,					\
>> +	}
> 
> Why can't the TOP_PD domains also have a __get_power().  Shouldn't we
> just be able to check the sleep_reg/sleep_mask like in the VPU case?

It can, I can add it later, do we need it for this version ?

> 
> Also, for readability, I think the arguments to VPU_PD and TOP_PD to
> have the same order, at least for the common ones.  IOW, __name,
> __top_pd, __mem should be first.

Sure, will fix

> 
>> +#define MEM_PD(__name, __mem)						\
>> +	TOP_PD(__name, NULL, __mem)
>> +
>> +static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain);
>> +
>> +static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
>> +	[PWRC_G12A_VPU_ID]  = VPU_PD("VPU", g12a_pwrc_vpu_resets,
>> +				     g12a_pwrc_vpu_clks, &g12a_pwrc_vpu,
>> +				     g12a_pwrc_mem_vpu,
>> +				     pwrc_vpu_get_power),
>> +	[PWRC_G12A_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
>> +};
>> +
>> +static struct meson_ee_pwrc_domain_desc sm1_pwrc_domains[] = {
>> +	[PWRC_SM1_VPU_ID]  = VPU_PD("VPU", g12a_pwrc_vpu_resets,
>> +				    g12a_pwrc_vpu_clks, &sm1_pwrc_vpu,
>> +				    sm1_pwrc_mem_vpu,
>> +				    pwrc_vpu_get_power),
>> +	[PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna),
>> +	[PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb),
>> +	[PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie),
>> +	[PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d),
>> +	[PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
>> +	[PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
>> +};
>> +
>> +struct meson_ee_pwrc_domain {
>> +	struct generic_pm_domain base;
>> +	bool enabled;
>> +	struct meson_ee_pwrc *pwrc;
>> +	struct meson_ee_pwrc_domain_desc desc;
>> +	struct clk **clks;
>> +	int num_clks;
>> +	struct reset_control **rstc;
>> +	int num_rstc;
>> +};
>> +
>> +struct meson_ee_pwrc {
>> +	struct regmap *regmap_ao;
>> +	struct regmap *regmap_hhi;
>> +	struct meson_ee_pwrc_domain *domains;
>> +	struct genpd_onecell_data xlate;
>> +};
>> +
>> +static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain)
>> +{
>> +	u32 reg;
>> +
>> +	regmap_read(pwrc_domain->pwrc->regmap_ao,
>> +		    pwrc_domain->desc.top_pd->sleep_reg, &reg);
>> +
>> +	return (reg & pwrc_domain->desc.top_pd->sleep_mask);
>> +}
>> +
>> +static int meson_ee_reset_assert(struct meson_ee_pwrc_domain *pwrc_domain)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
>> +		ret = reset_control_assert(pwrc_domain->rstc[i]);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int meson_ee_reset_deassert(struct meson_ee_pwrc_domain *pwrc_domain)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
>> +		ret = reset_control_deassert(pwrc_domain->rstc[i]);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> You should use the reset_array functions, then you don't need these helpers.
> 
>> +static int meson_ee_clk_disable(struct meson_ee_pwrc_domain *pwrc_domain)
>> +{
>> +	int i;
>> +
>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
>> +		clk_disable(pwrc_domain->clks[i]);
>> +
>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
>> +		clk_unprepare(pwrc_domain->clks[i]);
>> +
>> +	return 0;
>> +}
>> +
>> +static int meson_ee_clk_enable(struct meson_ee_pwrc_domain *pwrc_domain)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
>> +		ret = clk_prepare(pwrc_domain->clks[i]);
>> +		if (ret)
>> +			goto fail_prepare;
>> +	}
>> +
>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
>> +		ret = clk_enable(pwrc_domain->clks[i]);
>> +		if (ret)
>> +			goto fail_enable;
>> +	}
>> +
>> +	return 0;
>> +fail_enable:
>> +	while (--i)
>> +		clk_disable(pwrc_domain->clks[i]);
>> +
>> +	/* Unprepare all clocks */
>> +	i = pwrc_domain->num_clks;
>> +
>> +fail_prepare:
>> +	while (--i)
>> +		clk_unprepare(pwrc_domain->clks[i]);
>> +
>> +	return ret;
>> +}
> 
> Both the clk enable and disable functions above are just open-coding of
> the clk_bulk equivalents.  Please use clk_bulk_*, then you don't need
> these helpers.  (c.f. the RFT patch I did to convert the old driver to
> clk_bulk[1])

Yes, but clk_bulk takes _all_ the clocks from the node, you canot specify
a list of names, maybe it's overengineered but I wanted to specify the
exact resets and clocks for each power domain, clk_bulk doesn't provide this.

> 
>> +static int meson_ee_pwrc_off(struct generic_pm_domain *domain)
>> +{
>> +	struct meson_ee_pwrc_domain *pwrc_domain =
>> +		container_of(domain, struct meson_ee_pwrc_domain, base);
>> +	int i;
>> +
>> +	if (pwrc_domain->desc.top_pd)
>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>> +				   pwrc_domain->desc.top_pd->sleep_reg,
>> +				   pwrc_domain->desc.top_pd->sleep_mask,
>> +				   pwrc_domain->desc.top_pd->sleep_mask);
>> +	udelay(20);
>> +
>> +	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
>> +				   pwrc_domain->desc.mem_pd[i].reg,
>> +				   pwrc_domain->desc.mem_pd[i].mask,
>> +				   pwrc_domain->desc.mem_pd[i].mask);
>> +
>> +	udelay(20);
>> +
>> +	if (pwrc_domain->desc.top_pd)
>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>> +				   pwrc_domain->desc.top_pd->iso_reg,
>> +				   pwrc_domain->desc.top_pd->iso_mask,
>> +				   pwrc_domain->desc.top_pd->iso_mask);
>> +
>> +	if (pwrc_domain->num_clks) {
>> +		msleep(20);
>> +		meson_ee_clk_disable(pwrc_domain);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int meson_ee_pwrc_on(struct generic_pm_domain *domain)
>> +{
>> +	struct meson_ee_pwrc_domain *pwrc_domain =
>> +		container_of(domain, struct meson_ee_pwrc_domain, base);
>> +	int i, ret;
>> +
>> +	if (pwrc_domain->desc.top_pd)
>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>> +				   pwrc_domain->desc.top_pd->sleep_reg,
>> +				   pwrc_domain->desc.top_pd->sleep_mask, 0);
>> +	udelay(20);
>> +
>> +	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
>> +				   pwrc_domain->desc.mem_pd[i].reg,
>> +				   pwrc_domain->desc.mem_pd[i].mask, 0);
>> +
>> +	udelay(20);
>> +
>> +	ret = meson_ee_reset_assert(pwrc_domain);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (pwrc_domain->desc.top_pd)
>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>> +				   pwrc_domain->desc.top_pd->iso_reg,
>> +				   pwrc_domain->desc.top_pd->iso_mask, 0);
>> +
>> +	ret = meson_ee_reset_deassert(pwrc_domain);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return meson_ee_clk_enable(pwrc_domain);
>> +}
>> +
>> +static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
>> +				     struct meson_ee_pwrc *sm1_pwrc,
>> +				     struct meson_ee_pwrc_domain *dom)
>> +{
>> +	dom->pwrc = sm1_pwrc;
>> +	dom->num_rstc = dom->desc.reset_names_count;
>> +	dom->num_clks = dom->desc.clk_names_count;
>> +
>> +	if (dom->num_rstc) {
>> +		int rst;
>> +
>> +		dom->rstc = devm_kcalloc(&pdev->dev, dom->num_rstc,
>> +				sizeof(struct reset_control *),	GFP_KERNEL);
>> +		if (!dom->rstc)
>> +			return -ENOMEM;
>> +
>> +		for (rst = 0 ; rst < dom->num_rstc ; ++rst) {
>> +			dom->rstc[rst] = devm_reset_control_get_exclusive(
>> +					&pdev->dev,
>> +					dom->desc.reset_names[rst]);
>> +			if (IS_ERR(dom->rstc[rst]))
>> +				return PTR_ERR(dom->rstc[rst]);
>> +		}
> 
> Why not simplify and use the helpers that get multiple reset lines (like
> devm_reset_control_array_get() used in meson-gx-pwrc-vpu.c)?

Same comment as clk_bulk, we cannot be sure we select the right reset lines.

> 
> You could also use reset_control_get_count() and compare to the expected
> number (dom->num_rstc).

This seems oversimplified

> 
>> +	}
>> +
>> +	if (dom->num_clks) {
>> +		int clk;
>> +
>> +		dom->clks = devm_kcalloc(&pdev->dev, dom->num_clks,
>> +				sizeof(struct clk *), GFP_KERNEL);
>> +		if (!dom->clks)
>> +			return -ENOMEM;
>> +
>> +		for (clk = 0 ; clk < dom->num_clks ; ++clk) {
>> +			dom->clks[clk] = devm_clk_get(&pdev->dev,
>> +					dom->desc.clk_names[clk]);
>> +			if (IS_ERR(dom->clks[clk]))
>> +				return PTR_ERR(dom->clks[clk]);
>> +		}
>> +	}
> 
> Please use clk_bulk API, and then just double-check that the number of
> clocks found matches the expected number.

Same, I'll either take all the clks and resets for the vpu power domain,
or keep this code to make sure we get the right clocks and resets.

> 
>> +	dom->base.name = dom->desc.name;
>> +	dom->base.power_on = meson_ee_pwrc_on;
>> +	dom->base.power_off = meson_ee_pwrc_off;
>> +
>> +	if (dom->desc.get_power) {
>> +		bool powered_off = dom->desc.get_power(dom);
> 
> nit: insert blank line here
> 
> More importantly, we defintely will have problem here in the
> !powered_off case.  TL;DR; the driver's state does not match the actual
> hardware state.
> 
> When powered_off = false, you're telling the genpd core that this domain
> is already turned on.  However, you haven't called _pwrc_on() yet for
> the domain, which means internal state of the driver for this domain
> (e.g. clock enables, resets, etc.) is not in sync with the HW.  On
> SEI610 this case is happending for the VPU, which seems to be enabled by
> u-boot, so this driver detects it as already on, which is fine.  But...
> 
> Remember that the ->power_off() function will be called during suspend,
> and will lead to the clk unprepare/disable calls.  However, for domains
> that are detected as on (!powered_off), clk prepare/enable will never
> have been called, so that when suspend happens, you'll get "already
> unprepared" errors from the clock core
> 
> IOW, I think you need something like this here:
> 
> 		if (!powered_off)
> 			meson_ee_pwrc_on(&dom->base);
> 
> so that the internal state of clock fwk etc. matches the detected state
> of the HW.  The problem with that simple fix, at least for the VPU is
> that it might cause us to lose any existing display or framebuffer
> console that's on-going.  Probably needs some testing.

Yes, I forgot to take the _shutdown() function from gx_pwrc_vpu driver :

349 static void meson_gx_pwrc_vpu_shutdown(struct platform_device *pdev)
350 {
351         struct meson_gx_pwrc_vpu *vpu_pd = platform_get_drvdata(pdev);
352         bool powered_off;
353
354         powered_off = meson_gx_pwrc_vpu_get_power(vpu_pd);
355         if (!powered_off)
356                 vpu_pd->genpd.power_off(&vpu_pd->genpd);
357 }

> 
> Anyways, to see what I mean, try suspend/resume (you can test this
> series on my integ branch with "rtcwake -d rtc0 -m mem -s4") and you'll
> see error splats from the clock core during suspend.
> 
> 
> 
>> +		pm_genpd_init(&dom->base, &pm_domain_always_on_gov,
>> +			      powered_off);
> 
>> +	} else
>> +		pm_genpd_init(&dom->base, NULL, true);
> 
> nit: the else clause should also have {} to match the if
> (c.f. CodingStyle)

OK

> 
> Why do you force the always-on governor in the case where ->get_power()
> exists, but not the other?
> 
> If you force that, then for any devices connected to these domains that
> use runtime PM, they will never turn off the domain when it's idle.
> IOW, these domains will only ever be turned off on system-wide
> suspend/resume.
> 
> IMO, unless there's a good reason not to, you should pass NULL for the
> governor.

It's for legacy when VPU is initialized from vendor U-Boot, look at commit :
339cd0ea082287ea8e2b7e7159a5a33665a2cbe3 "soc: amlogic: meson-gx-pwrc-vpu: fix power-off when powered by bootloader"

    In the case the VPU power domain has been powered on by the bootloader
    and no driver are attached to this power domain, the genpd will power it
    off after a certain amount of time, but the clocks hasn't been enabled
    by the kernel itself and the power-off will trigger some faults.
    This patch enable the clocks to have a coherent state for an eventual
    poweroff and switches to the pm_domain_always_on_gov governor.

I could set always-on governor only if the domain was already enabled,
what do you think ?

And seems I'm also missing the "This patch enable the clocks".

> 
>> +	return 0;
>> +}
>> +
>> +static int meson_ee_pwrc_probe(struct platform_device *pdev)
>> +{
>> +	const struct meson_ee_pwrc_domain_data *match;
>> +	struct regmap *regmap_ao, *regmap_hhi;
>> +	struct meson_ee_pwrc *sm1_pwrc;
> 
> Why the sm1_ prefix throughout this code?  This is now generalized for
> more SoCs, so shouldn't be sm1.  I'm assuming this is just left-over
> from the original version.  IMO, just called it pwrc.

Indeed, it's a leftover.

> 
>> +	int i, ret;
>> +
>> +	match = of_device_get_match_data(&pdev->dev);
>> +	if (!match) {
>> +		dev_err(&pdev->dev, "failed to get match data\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	sm1_pwrc = devm_kzalloc(&pdev->dev, sizeof(*sm1_pwrc), GFP_KERNEL);
>> +	if (!sm1_pwrc)
>> +		return -ENOMEM;
>> +
>> +	sm1_pwrc->xlate.domains =
>> +		devm_kcalloc(&pdev->dev,
>> +			     match->count,
>> +			     sizeof(*sm1_pwrc->xlate.domains),
>> +			     GFP_KERNEL);
>> +	if (!sm1_pwrc->xlate.domains)
>> +		return -ENOMEM;
>> +
>> +	sm1_pwrc->domains =
>> +		devm_kcalloc(&pdev->dev,
> 
> nit: this line probably doesn't need to be wrapped.

Ok

> 
>> +			     match->count,
>> +			     sizeof(*sm1_pwrc->domains),
>> +			     GFP_KERNEL);
>> +	if (!sm1_pwrc->domains)
>> +		return -ENOMEM;
>> +
>> +	sm1_pwrc->xlate.num_domains = match->count;
>> +
>> +	regmap_hhi = syscon_node_to_regmap(of_get_parent(pdev->dev.of_node));
>> +	if (IS_ERR(regmap_hhi)) {
>> +		dev_err(&pdev->dev, "failed to get HHI regmap\n");
>> +		return PTR_ERR(regmap_hhi);
>> +	}
>> +
>> +	regmap_ao = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
>> +						    "amlogic,ao-sysctrl");
>> +	if (IS_ERR(regmap_ao)) {
>> +		dev_err(&pdev->dev, "failed to get AO regmap\n");
>> +		return PTR_ERR(regmap_ao);
>> +	}
>> +
>> +	sm1_pwrc->regmap_ao = regmap_ao;
>> +	sm1_pwrc->regmap_hhi = regmap_hhi;
>> +
>> +	platform_set_drvdata(pdev, sm1_pwrc);
>> +
>> +	for (i = 0 ; i < match->count ; ++i) {
>> +		struct meson_ee_pwrc_domain *dom = &sm1_pwrc->domains[i];
>> +
>> +		memcpy(&dom->desc, &match->domains[i], sizeof(dom->desc));
>> +
>> +		ret = meson_ee_pwrc_init_domain(pdev, sm1_pwrc, dom);
>> +		if (ret)
>> +			return ret;
>> +
>> +		sm1_pwrc->xlate.domains[i] = &dom->base;
>> +	}
>> +
>> +	of_genpd_add_provider_onecell(pdev->dev.of_node, &sm1_pwrc->xlate);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct meson_ee_pwrc_domain_data meson_ee_g12a_pwrc_data = {
>> +	.count = ARRAY_SIZE(g12a_pwrc_domains),
>> +	.domains = g12a_pwrc_domains,
>> +};
>> +
>> +static struct meson_ee_pwrc_domain_data meson_ee_sm1_pwrc_data = {
>> +	.count = ARRAY_SIZE(sm1_pwrc_domains),
>> +	.domains = sm1_pwrc_domains,
>> +};
>> +
>> +static const struct of_device_id meson_ee_pwrc_match_table[] = {
>> +	{
>> +		.compatible = "amlogic,meson-g12a-pwrc",
>> +		.data = &meson_ee_g12a_pwrc_data,
>> +	},
>> +	{
>> +		.compatible = "amlogic,meson-sm1-pwrc",
>> +		.data = &meson_ee_sm1_pwrc_data,
>> +	},
>> +	{ /* sentinel */ }
>> +};
>> +
>> +static struct platform_driver meson_ee_pwrc_driver = {
>> +	.probe	= meson_ee_pwrc_probe,
>> +	.driver = {
>> +		.name		= "meson_ee_pwrc",
>> +		.of_match_table	= meson_ee_pwrc_match_table,
>> +	},
>> +};
>> +builtin_platform_driver(meson_ee_pwrc_driver);
>> -- 
>> 2.22.0

Thanks,
Neil

> 
> Kevin
> 
> [1] https://lore.kernel.org/linux-amlogic/20190809230904.28747-1-khilman@baylibre.com/
> 

Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller

[Kevin Hilman]

Neil Armstrong <narmstrong@baylibre.com> writes:

> On 22/08/2019 01:16, Kevin Hilman wrote:
>> Neil Armstrong <narmstrong@baylibre.com> writes:
>> 
>>> Add support for the General Purpose Amlogic Everything-Else Power controller,
>>> with the first support for G12A and SM1 SoCs dedicated to the VPU, PCIe,
>>> USB, NNA, GE2D and Ethernet Power Domains.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> 
>> Nice!  Thanks for generalizing this.
>> 
>> A few comments/concerns below, but this is mostly ready.

[...]

>>> +#define VPU_PD(__name, __resets, __clks, __top_pd, __mem, __get_power)	\
>>> +	{								\
>>> +		.name = __name,						\
>>> +		.reset_names_count = ARRAY_SIZE(__resets),		\
>>> +		.reset_names = __resets,				\
>>> +		.clk_names_count = ARRAY_SIZE(__clks),			\
>>> +		.clk_names = __clks,					\
>>> +		.top_pd = __top_pd,					\
>>> +		.mem_pd_count = ARRAY_SIZE(__mem),			\
>>> +		.mem_pd = __mem,					\
>>> +		.get_power = __get_power,				\
>>> +	}
>>> +
>>> +#define TOP_PD(__name, __top_pd, __mem)					\
>>> +	{								\
>>> +		.name = __name,						\
>>> +		.top_pd = __top_pd,					\
>>> +		.mem_pd_count = ARRAY_SIZE(__mem),			\
>>> +		.mem_pd = __mem,					\
>>> +	}
>> 
>> Why can't the TOP_PD domains also have a __get_power().  Shouldn't we
>> just be able to check the sleep_reg/sleep_mask like in the VPU case?
>
> It can, I can add it later, do we need it for this version ?

Yes please.  Seems a pretty simple addition, let's have it from the
beginning.

>> Also, for readability, I think the arguments to VPU_PD and TOP_PD to
>> have the same order, at least for the common ones.  IOW, __name,
>> __top_pd, __mem should be first.
>
> Sure, will fix

and you can add __get_power to the common list too.

[...]

>>> +static int meson_ee_clk_disable(struct meson_ee_pwrc_domain *pwrc_domain)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
>>> +		clk_disable(pwrc_domain->clks[i]);
>>> +
>>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
>>> +		clk_unprepare(pwrc_domain->clks[i]);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int meson_ee_clk_enable(struct meson_ee_pwrc_domain *pwrc_domain)
>>> +{
>>> +	int i, ret;
>>> +
>>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
>>> +		ret = clk_prepare(pwrc_domain->clks[i]);
>>> +		if (ret)
>>> +			goto fail_prepare;
>>> +	}
>>> +
>>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
>>> +		ret = clk_enable(pwrc_domain->clks[i]);
>>> +		if (ret)
>>> +			goto fail_enable;
>>> +	}
>>> +
>>> +	return 0;
>>> +fail_enable:
>>> +	while (--i)
>>> +		clk_disable(pwrc_domain->clks[i]);
>>> +
>>> +	/* Unprepare all clocks */
>>> +	i = pwrc_domain->num_clks;
>>> +
>>> +fail_prepare:
>>> +	while (--i)
>>> +		clk_unprepare(pwrc_domain->clks[i]);
>>> +
>>> +	return ret;
>>> +}
>> 
>> Both the clk enable and disable functions above are just open-coding of
>> the clk_bulk equivalents.  Please use clk_bulk_*, then you don't need
>> these helpers.  (c.f. the RFT patch I did to convert the old driver to
>> clk_bulk[1])
>
> Yes, but clk_bulk takes _all_ the clocks from the node, you canot specify
> a list of names, maybe it's overengineered but I wanted to specify the
> exact resets and clocks for each power domain, clk_bulk doesn't provide this.

I've been going on the assumption that there's no reason to list clocks
in the pwrc DT node that you don't want managed by the driver.  This
also seems to match the exisiting driver, and this new one.

What is the case where you would list clocks in the DT node for the
power-domain, but not want to manage them in the driver?

If there's no good reason to do that, then clk_bulk greatly simplifies
this code.

>>> +static int meson_ee_pwrc_off(struct generic_pm_domain *domain)
>>> +{
>>> +	struct meson_ee_pwrc_domain *pwrc_domain =
>>> +		container_of(domain, struct meson_ee_pwrc_domain, base);
>>> +	int i;
>>> +
>>> +	if (pwrc_domain->desc.top_pd)
>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>>> +				   pwrc_domain->desc.top_pd->sleep_reg,
>>> +				   pwrc_domain->desc.top_pd->sleep_mask,
>>> +				   pwrc_domain->desc.top_pd->sleep_mask);
>>> +	udelay(20);
>>> +
>>> +	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
>>> +				   pwrc_domain->desc.mem_pd[i].reg,
>>> +				   pwrc_domain->desc.mem_pd[i].mask,
>>> +				   pwrc_domain->desc.mem_pd[i].mask);
>>> +
>>> +	udelay(20);
>>> +
>>> +	if (pwrc_domain->desc.top_pd)
>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>>> +				   pwrc_domain->desc.top_pd->iso_reg,
>>> +				   pwrc_domain->desc.top_pd->iso_mask,
>>> +				   pwrc_domain->desc.top_pd->iso_mask);
>>> +
>>> +	if (pwrc_domain->num_clks) {
>>> +		msleep(20);
>>> +		meson_ee_clk_disable(pwrc_domain);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int meson_ee_pwrc_on(struct generic_pm_domain *domain)
>>> +{
>>> +	struct meson_ee_pwrc_domain *pwrc_domain =
>>> +		container_of(domain, struct meson_ee_pwrc_domain, base);
>>> +	int i, ret;
>>> +
>>> +	if (pwrc_domain->desc.top_pd)
>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>>> +				   pwrc_domain->desc.top_pd->sleep_reg,
>>> +				   pwrc_domain->desc.top_pd->sleep_mask, 0);
>>> +	udelay(20);
>>> +
>>> +	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
>>> +				   pwrc_domain->desc.mem_pd[i].reg,
>>> +				   pwrc_domain->desc.mem_pd[i].mask, 0);
>>> +
>>> +	udelay(20);
>>> +
>>> +	ret = meson_ee_reset_assert(pwrc_domain);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (pwrc_domain->desc.top_pd)
>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>>> +				   pwrc_domain->desc.top_pd->iso_reg,
>>> +				   pwrc_domain->desc.top_pd->iso_mask, 0);
>>> +
>>> +	ret = meson_ee_reset_deassert(pwrc_domain);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return meson_ee_clk_enable(pwrc_domain);
>>> +}
>>> +
>>> +static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
>>> +				     struct meson_ee_pwrc *sm1_pwrc,
>>> +				     struct meson_ee_pwrc_domain *dom)
>>> +{
>>> +	dom->pwrc = sm1_pwrc;
>>> +	dom->num_rstc = dom->desc.reset_names_count;
>>> +	dom->num_clks = dom->desc.clk_names_count;
>>> +
>>> +	if (dom->num_rstc) {
>>> +		int rst;
>>> +
>>> +		dom->rstc = devm_kcalloc(&pdev->dev, dom->num_rstc,
>>> +				sizeof(struct reset_control *),	GFP_KERNEL);
>>> +		if (!dom->rstc)
>>> +			return -ENOMEM;
>>> +
>>> +		for (rst = 0 ; rst < dom->num_rstc ; ++rst) {
>>> +			dom->rstc[rst] = devm_reset_control_get_exclusive(
>>> +					&pdev->dev,
>>> +					dom->desc.reset_names[rst]);
>>> +			if (IS_ERR(dom->rstc[rst]))
>>> +				return PTR_ERR(dom->rstc[rst]);
>>> +		}
>> 
>> Why not simplify and use the helpers that get multiple reset lines (like
>> devm_reset_control_array_get() used in meson-gx-pwrc-vpu.c)?
>
> Same comment as clk_bulk, we cannot be sure we select the right reset lines.

Again, what is the case for listing resets in the power-domain node that
you don't want to be managed by the driver?

>> You could also use reset_control_get_count() and compare to the expected
>> number (dom->num_rstc).
>
> This seems oversimplified

Similar to above, if you're always going to manage all the reset lines
in the DT node, then simple is good.

If there are reasons to list reset lines in the DT node that will not be
managed by the driver, I'm defintiely curious why.

If not, using the reset API helpers is much more readable and
maintainble IMO.

>> 
>>> +	}
>>> +
>>> +	if (dom->num_clks) {
>>> +		int clk;
>>> +
>>> +		dom->clks = devm_kcalloc(&pdev->dev, dom->num_clks,
>>> +				sizeof(struct clk *), GFP_KERNEL);
>>> +		if (!dom->clks)
>>> +			return -ENOMEM;
>>> +
>>> +		for (clk = 0 ; clk < dom->num_clks ; ++clk) {
>>> +			dom->clks[clk] = devm_clk_get(&pdev->dev,
>>> +					dom->desc.clk_names[clk]);
>>> +			if (IS_ERR(dom->clks[clk]))
>>> +				return PTR_ERR(dom->clks[clk]);
>>> +		}
>>> +	}
>> 
>> Please use clk_bulk API, and then just double-check that the number of
>> clocks found matches the expected number.
>
> Same, I'll either take all the clks and resets for the vpu power domain,
> or keep this code to make sure we get the right clocks and resets.

Similar to above.  IMO, we should be sure to put the "right clocks and
resets" into the DT, and then simplify the code.

>> 
>>> +	dom->base.name = dom->desc.name;
>>> +	dom->base.power_on = meson_ee_pwrc_on;
>>> +	dom->base.power_off = meson_ee_pwrc_off;
>>> +
>>> +	if (dom->desc.get_power) {
>>> +		bool powered_off = dom->desc.get_power(dom);
>> 
>> nit: insert blank line here
>> 
>> More importantly, we defintely will have problem here in the
>> !powered_off case.  TL;DR; the driver's state does not match the actual
>> hardware state.
>> 
>> When powered_off = false, you're telling the genpd core that this domain
>> is already turned on.  However, you haven't called _pwrc_on() yet for
>> the domain, which means internal state of the driver for this domain
>> (e.g. clock enables, resets, etc.) is not in sync with the HW.  On
>> SEI610 this case is happending for the VPU, which seems to be enabled by
>> u-boot, so this driver detects it as already on, which is fine.  But...
>> 
>> Remember that the ->power_off() function will be called during suspend,
>> and will lead to the clk unprepare/disable calls.  However, for domains
>> that are detected as on (!powered_off), clk prepare/enable will never
>> have been called, so that when suspend happens, you'll get "already
>> unprepared" errors from the clock core
>> 
>> IOW, I think you need something like this here:
>> 
>> 		if (!powered_off)
>> 			meson_ee_pwrc_on(&dom->base);
>> 
>> so that the internal state of clock fwk etc. matches the detected state
>> of the HW.  The problem with that simple fix, at least for the VPU is
>> that it might cause us to lose any existing display or framebuffer
>> console that's on-going.  Probably needs some testing.
>
> Yes, I forgot to take the _shutdown() function from gx_pwrc_vpu driver :
>
> 349 static void meson_gx_pwrc_vpu_shutdown(struct platform_device *pdev)
> 350 {
> 351         struct meson_gx_pwrc_vpu *vpu_pd = platform_get_drvdata(pdev);
> 352         bool powered_off;
> 353
> 354         powered_off = meson_gx_pwrc_vpu_get_power(vpu_pd);
> 355         if (!powered_off)
> 356                 vpu_pd->genpd.power_off(&vpu_pd->genpd);
> 357 }

OK, yeah, I hadn't noticed that in the original driver.  I tested
something simliar with suspend/resume on SEI610 and it gets rid of the
"already unprepared" splats from the clock core.

>> 
>> Anyways, to see what I mean, try suspend/resume (you can test this
>> series on my integ branch with "rtcwake -d rtc0 -m mem -s4") and you'll
>> see error splats from the clock core during suspend.
>> 
>> 
>> 
>>> +		pm_genpd_init(&dom->base, &pm_domain_always_on_gov,
>>> +			      powered_off);
>> 
>>> +	} else
>>> +		pm_genpd_init(&dom->base, NULL, true);
>> 
>> nit: the else clause should also have {} to match the if
>> (c.f. CodingStyle)
>
> OK
>
>> 
>> Why do you force the always-on governor in the case where ->get_power()
>> exists, but not the other?
>> 
>> If you force that, then for any devices connected to these domains that
>> use runtime PM, they will never turn off the domain when it's idle.
>> IOW, these domains will only ever be turned off on system-wide
>> suspend/resume.
>> 
>> IMO, unless there's a good reason not to, you should pass NULL for the
>> governor.
>
> It's for legacy when VPU is initialized from vendor U-Boot, look at commit :
> 339cd0ea082287ea8e2b7e7159a5a33665a2cbe3 "soc: amlogic: meson-gx-pwrc-vpu: fix power-off when powered by bootloader"
>
>     In the case the VPU power domain has been powered on by the bootloader
>     and no driver are attached to this power domain, the genpd will power it
>     off after a certain amount of time, but the clocks hasn't been enabled
>     by the kernel itself and the power-off will trigger some faults.
>     This patch enable the clocks to have a coherent state for an eventual
>     poweroff and switches to the pm_domain_always_on_gov governor.

The key phrase there being "and no driver is attached".  Now that we
have a driver, it claims this domain so I don't think it will be
powered off:

# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary 
domain                          status          slaves
    /device                                             runtime status
----------------------------------------------------------------------
ETH                             on              
    /devices/platform/soc/ff3f0000.ethernet             unsupported
AUDIO                           off-0           
GE2D                            off-0           
PCI                             off-0           
USB                             on              
    /devices/platform/soc/ffe09000.usb                  active
NNA                             off-0           
VPU                             on              
    /devices/platform/soc/ff900000.vpu                  unsupported

In my tests with a framebuffer console (over HDMI), I don't see the
display being powered off.

> I could set always-on governor only if the domain was already enabled,
> what do you think ?

I don't think that's necessary now that we have a driver.  We really
want to be able to power-down this domain when the display is not in
use, and if you use always_on, that will never happen.

> And seems I'm also missing the "This patch enable the clocks".

I'm not sure what patch you're referring to.

Kevin

Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller

[Neil Armstrong]

On 22/08/2019 22:32, Kevin Hilman wrote:
> Neil Armstrong <narmstrong@baylibre.com> writes:
> 
>> On 22/08/2019 01:16, Kevin Hilman wrote:
>>> Neil Armstrong <narmstrong@baylibre.com> writes:
>>>
>>>> Add support for the General Purpose Amlogic Everything-Else Power controller,
>>>> with the first support for G12A and SM1 SoCs dedicated to the VPU, PCIe,
>>>> USB, NNA, GE2D and Ethernet Power Domains.
>>>>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>
>>> Nice!  Thanks for generalizing this.
>>>
>>> A few comments/concerns below, but this is mostly ready.
> 
> [...]
> 
>>>> +#define VPU_PD(__name, __resets, __clks, __top_pd, __mem, __get_power)	\
>>>> +	{								\
>>>> +		.name = __name,						\
>>>> +		.reset_names_count = ARRAY_SIZE(__resets),		\
>>>> +		.reset_names = __resets,				\
>>>> +		.clk_names_count = ARRAY_SIZE(__clks),			\
>>>> +		.clk_names = __clks,					\
>>>> +		.top_pd = __top_pd,					\
>>>> +		.mem_pd_count = ARRAY_SIZE(__mem),			\
>>>> +		.mem_pd = __mem,					\
>>>> +		.get_power = __get_power,				\
>>>> +	}
>>>> +
>>>> +#define TOP_PD(__name, __top_pd, __mem)					\
>>>> +	{								\
>>>> +		.name = __name,						\
>>>> +		.top_pd = __top_pd,					\
>>>> +		.mem_pd_count = ARRAY_SIZE(__mem),			\
>>>> +		.mem_pd = __mem,					\
>>>> +	}
>>>
>>> Why can't the TOP_PD domains also have a __get_power().  Shouldn't we
>>> just be able to check the sleep_reg/sleep_mask like in the VPU case?
>>
>> It can, I can add it later, do we need it for this version ?
> 
> Yes please.  Seems a pretty simple addition, let's have it from the
> beginning.
> 
>>> Also, for readability, I think the arguments to VPU_PD and TOP_PD to
>>> have the same order, at least for the common ones.  IOW, __name,
>>> __top_pd, __mem should be first.
>>
>> Sure, will fix
> 
> and you can add __get_power to the common list too.
> 
> [...]
> 
>>>> +static int meson_ee_clk_disable(struct meson_ee_pwrc_domain *pwrc_domain)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
>>>> +		clk_disable(pwrc_domain->clks[i]);
>>>> +
>>>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
>>>> +		clk_unprepare(pwrc_domain->clks[i]);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int meson_ee_clk_enable(struct meson_ee_pwrc_domain *pwrc_domain)
>>>> +{
>>>> +	int i, ret;
>>>> +
>>>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
>>>> +		ret = clk_prepare(pwrc_domain->clks[i]);
>>>> +		if (ret)
>>>> +			goto fail_prepare;
>>>> +	}
>>>> +
>>>> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
>>>> +		ret = clk_enable(pwrc_domain->clks[i]);
>>>> +		if (ret)
>>>> +			goto fail_enable;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +fail_enable:
>>>> +	while (--i)
>>>> +		clk_disable(pwrc_domain->clks[i]);
>>>> +
>>>> +	/* Unprepare all clocks */
>>>> +	i = pwrc_domain->num_clks;
>>>> +
>>>> +fail_prepare:
>>>> +	while (--i)
>>>> +		clk_unprepare(pwrc_domain->clks[i]);
>>>> +
>>>> +	return ret;
>>>> +}
>>>
>>> Both the clk enable and disable functions above are just open-coding of
>>> the clk_bulk equivalents.  Please use clk_bulk_*, then you don't need
>>> these helpers.  (c.f. the RFT patch I did to convert the old driver to
>>> clk_bulk[1])
>>
>> Yes, but clk_bulk takes _all_ the clocks from the node, you canot specify
>> a list of names, maybe it's overengineered but I wanted to specify the
>> exact resets and clocks for each power domain, clk_bulk doesn't provide this.
> 
> I've been going on the assumption that there's no reason to list clocks
> in the pwrc DT node that you don't want managed by the driver.  This
> also seems to match the exisiting driver, and this new one.
> 
> What is the case where you would list clocks in the DT node for the
> power-domain, but not want to manage them in the driver?
> 
> If there's no good reason to do that, then clk_bulk greatly simplifies
> this code.

I guess I could put back the code if we need to split clocks and resets across
power domains in the future.

> 
>>>> +static int meson_ee_pwrc_off(struct generic_pm_domain *domain)
>>>> +{
>>>> +	struct meson_ee_pwrc_domain *pwrc_domain =
>>>> +		container_of(domain, struct meson_ee_pwrc_domain, base);
>>>> +	int i;
>>>> +
>>>> +	if (pwrc_domain->desc.top_pd)
>>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>>>> +				   pwrc_domain->desc.top_pd->sleep_reg,
>>>> +				   pwrc_domain->desc.top_pd->sleep_mask,
>>>> +				   pwrc_domain->desc.top_pd->sleep_mask);
>>>> +	udelay(20);
>>>> +
>>>> +	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
>>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
>>>> +				   pwrc_domain->desc.mem_pd[i].reg,
>>>> +				   pwrc_domain->desc.mem_pd[i].mask,
>>>> +				   pwrc_domain->desc.mem_pd[i].mask);
>>>> +
>>>> +	udelay(20);
>>>> +
>>>> +	if (pwrc_domain->desc.top_pd)
>>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>>>> +				   pwrc_domain->desc.top_pd->iso_reg,
>>>> +				   pwrc_domain->desc.top_pd->iso_mask,
>>>> +				   pwrc_domain->desc.top_pd->iso_mask);
>>>> +
>>>> +	if (pwrc_domain->num_clks) {
>>>> +		msleep(20);
>>>> +		meson_ee_clk_disable(pwrc_domain);
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int meson_ee_pwrc_on(struct generic_pm_domain *domain)
>>>> +{
>>>> +	struct meson_ee_pwrc_domain *pwrc_domain =
>>>> +		container_of(domain, struct meson_ee_pwrc_domain, base);
>>>> +	int i, ret;
>>>> +
>>>> +	if (pwrc_domain->desc.top_pd)
>>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>>>> +				   pwrc_domain->desc.top_pd->sleep_reg,
>>>> +				   pwrc_domain->desc.top_pd->sleep_mask, 0);
>>>> +	udelay(20);
>>>> +
>>>> +	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
>>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
>>>> +				   pwrc_domain->desc.mem_pd[i].reg,
>>>> +				   pwrc_domain->desc.mem_pd[i].mask, 0);
>>>> +
>>>> +	udelay(20);
>>>> +
>>>> +	ret = meson_ee_reset_assert(pwrc_domain);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	if (pwrc_domain->desc.top_pd)
>>>> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
>>>> +				   pwrc_domain->desc.top_pd->iso_reg,
>>>> +				   pwrc_domain->desc.top_pd->iso_mask, 0);
>>>> +
>>>> +	ret = meson_ee_reset_deassert(pwrc_domain);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	return meson_ee_clk_enable(pwrc_domain);
>>>> +}
>>>> +
>>>> +static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
>>>> +				     struct meson_ee_pwrc *sm1_pwrc,
>>>> +				     struct meson_ee_pwrc_domain *dom)
>>>> +{
>>>> +	dom->pwrc = sm1_pwrc;
>>>> +	dom->num_rstc = dom->desc.reset_names_count;
>>>> +	dom->num_clks = dom->desc.clk_names_count;
>>>> +
>>>> +	if (dom->num_rstc) {
>>>> +		int rst;
>>>> +
>>>> +		dom->rstc = devm_kcalloc(&pdev->dev, dom->num_rstc,
>>>> +				sizeof(struct reset_control *),	GFP_KERNEL);
>>>> +		if (!dom->rstc)
>>>> +			return -ENOMEM;
>>>> +
>>>> +		for (rst = 0 ; rst < dom->num_rstc ; ++rst) {
>>>> +			dom->rstc[rst] = devm_reset_control_get_exclusive(
>>>> +					&pdev->dev,
>>>> +					dom->desc.reset_names[rst]);
>>>> +			if (IS_ERR(dom->rstc[rst]))
>>>> +				return PTR_ERR(dom->rstc[rst]);
>>>> +		}
>>>
>>> Why not simplify and use the helpers that get multiple reset lines (like
>>> devm_reset_control_array_get() used in meson-gx-pwrc-vpu.c)?
>>
>> Same comment as clk_bulk, we cannot be sure we select the right reset lines.
> 
> Again, what is the case for listing resets in the power-domain node that
> you don't want to be managed by the driver?
> 
>>> You could also use reset_control_get_count() and compare to the expected
>>> number (dom->num_rstc).
>>
>> This seems oversimplified
> 
> Similar to above, if you're always going to manage all the reset lines
> in the DT node, then simple is good.
> 
> If there are reasons to list reset lines in the DT node that will not be
> managed by the driver, I'm defintiely curious why.
> 
> If not, using the reset API helpers is much more readable and
> maintainble IMO.

Will simply add the resets and clocks numbers and check the number at probe.

> 
>>>
>>>> +	}
>>>> +
>>>> +	if (dom->num_clks) {
>>>> +		int clk;
>>>> +
>>>> +		dom->clks = devm_kcalloc(&pdev->dev, dom->num_clks,
>>>> +				sizeof(struct clk *), GFP_KERNEL);
>>>> +		if (!dom->clks)
>>>> +			return -ENOMEM;
>>>> +
>>>> +		for (clk = 0 ; clk < dom->num_clks ; ++clk) {
>>>> +			dom->clks[clk] = devm_clk_get(&pdev->dev,
>>>> +					dom->desc.clk_names[clk]);
>>>> +			if (IS_ERR(dom->clks[clk]))
>>>> +				return PTR_ERR(dom->clks[clk]);
>>>> +		}
>>>> +	}
>>>
>>> Please use clk_bulk API, and then just double-check that the number of
>>> clocks found matches the expected number.
>>
>> Same, I'll either take all the clks and resets for the vpu power domain,
>> or keep this code to make sure we get the right clocks and resets.
> 
> Similar to above.  IMO, we should be sure to put the "right clocks and
> resets" into the DT, and then simplify the code.
> 
>>>
>>>> +	dom->base.name = dom->desc.name;
>>>> +	dom->base.power_on = meson_ee_pwrc_on;
>>>> +	dom->base.power_off = meson_ee_pwrc_off;
>>>> +
>>>> +	if (dom->desc.get_power) {
>>>> +		bool powered_off = dom->desc.get_power(dom);
>>>
>>> nit: insert blank line here
>>>
>>> More importantly, we defintely will have problem here in the
>>> !powered_off case.  TL;DR; the driver's state does not match the actual
>>> hardware state.
>>>
>>> When powered_off = false, you're telling the genpd core that this domain
>>> is already turned on.  However, you haven't called _pwrc_on() yet for
>>> the domain, which means internal state of the driver for this domain
>>> (e.g. clock enables, resets, etc.) is not in sync with the HW.  On
>>> SEI610 this case is happending for the VPU, which seems to be enabled by
>>> u-boot, so this driver detects it as already on, which is fine.  But...
>>>
>>> Remember that the ->power_off() function will be called during suspend,
>>> and will lead to the clk unprepare/disable calls.  However, for domains
>>> that are detected as on (!powered_off), clk prepare/enable will never
>>> have been called, so that when suspend happens, you'll get "already
>>> unprepared" errors from the clock core
>>>
>>> IOW, I think you need something like this here:
>>>
>>> 		if (!powered_off)
>>> 			meson_ee_pwrc_on(&dom->base);
>>>
>>> so that the internal state of clock fwk etc. matches the detected state
>>> of the HW.  The problem with that simple fix, at least for the VPU is
>>> that it might cause us to lose any existing display or framebuffer
>>> console that's on-going.  Probably needs some testing.
>>
>> Yes, I forgot to take the _shutdown() function from gx_pwrc_vpu driver :
>>
>> 349 static void meson_gx_pwrc_vpu_shutdown(struct platform_device *pdev)
>> 350 {
>> 351         struct meson_gx_pwrc_vpu *vpu_pd = platform_get_drvdata(pdev);
>> 352         bool powered_off;
>> 353
>> 354         powered_off = meson_gx_pwrc_vpu_get_power(vpu_pd);
>> 355         if (!powered_off)
>> 356                 vpu_pd->genpd.power_off(&vpu_pd->genpd);
>> 357 }
> 
> OK, yeah, I hadn't noticed that in the original driver.  I tested
> something simliar with suspend/resume on SEI610 and it gets rid of the
> "already unprepared" splats from the clock core.
> 
>>>
>>> Anyways, to see what I mean, try suspend/resume (you can test this
>>> series on my integ branch with "rtcwake -d rtc0 -m mem -s4") and you'll
>>> see error splats from the clock core during suspend.
>>>
>>>
>>>
>>>> +		pm_genpd_init(&dom->base, &pm_domain_always_on_gov,
>>>> +			      powered_off);
>>>
>>>> +	} else
>>>> +		pm_genpd_init(&dom->base, NULL, true);
>>>
>>> nit: the else clause should also have {} to match the if
>>> (c.f. CodingStyle)
>>
>> OK
>>
>>>
>>> Why do you force the always-on governor in the case where ->get_power()
>>> exists, but not the other?
>>>
>>> If you force that, then for any devices connected to these domains that
>>> use runtime PM, they will never turn off the domain when it's idle.
>>> IOW, these domains will only ever be turned off on system-wide
>>> suspend/resume.
>>>
>>> IMO, unless there's a good reason not to, you should pass NULL for the
>>> governor.
>>
>> It's for legacy when VPU is initialized from vendor U-Boot, look at commit :
>> 339cd0ea082287ea8e2b7e7159a5a33665a2cbe3 "soc: amlogic: meson-gx-pwrc-vpu: fix power-off when powered by bootloader"
>>
>>     In the case the VPU power domain has been powered on by the bootloader
>>     and no driver are attached to this power domain, the genpd will power it
>>     off after a certain amount of time, but the clocks hasn't been enabled
>>     by the kernel itself and the power-off will trigger some faults.
>>     This patch enable the clocks to have a coherent state for an eventual
>>     poweroff and switches to the pm_domain_always_on_gov governor.
> 
> The key phrase there being "and no driver is attached".  Now that we
> have a driver, it claims this domain so I don't think it will be
> powered off:
> 
> # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary 
> domain                          status          slaves
>     /device                                             runtime status
> ----------------------------------------------------------------------
> ETH                             on              
>     /devices/platform/soc/ff3f0000.ethernet             unsupported
> AUDIO                           off-0           
> GE2D                            off-0           
> PCI                             off-0           
> USB                             on              
>     /devices/platform/soc/ffe09000.usb                  active
> NNA                             off-0           
> VPU                             on              
>     /devices/platform/soc/ff900000.vpu                  unsupported
> 
> In my tests with a framebuffer console (over HDMI), I don't see the
> display being powered off.

It's in the case where the driver is a module loaded by the post-initramfs
system after the genpd timeout, or if the display driver is disabled.

In the later I had some system failures when vendor u-boot enabled the
display and genpd disabled the power domain later on.

> 
>> I could set always-on governor only if the domain was already enabled,
>> what do you think ?
> 
> I don't think that's necessary now that we have a driver.  We really
> want to be able to power-down this domain when the display is not in
> use, and if you use always_on, that will never happen.
> 
>> And seems I'm also missing the "This patch enable the clocks".
> 
> I'm not sure what patch you're referring to.

It's also added in 339cd0ea082287ea8e2b7e7159a5a33665a2cbe3 "soc: amlogic: meson-gx-pwrc-vpu: fix power-off when powered by bootloader"

I would like to keep the same behavior as meson-gx-pwrc-vpu, since it works fine
and we debugged all the issues we got.

Neil


> 
> Kevin
> 

Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller

[Kevin Hilman]

Neil Armstrong <narmstrong@baylibre.com> writes:

[...]

>>> It's for legacy when VPU is initialized from vendor U-Boot, look at commit :
>>> 339cd0ea082287ea8e2b7e7159a5a33665a2cbe3 "soc: amlogic: meson-gx-pwrc-vpu: fix power-off when powered by bootloader"
>>>
>>>     In the case the VPU power domain has been powered on by the bootloader
>>>     and no driver are attached to this power domain, the genpd will power it
>>>     off after a certain amount of time, but the clocks hasn't been enabled
>>>     by the kernel itself and the power-off will trigger some faults.
>>>     This patch enable the clocks to have a coherent state for an eventual
>>>     poweroff and switches to the pm_domain_always_on_gov governor.
>> 
>> The key phrase there being "and no driver is attached".  Now that we
>> have a driver, it claims this domain so I don't think it will be
>> powered off:
>> 
>> # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary 
>> domain                          status          slaves
>>     /device                                             runtime status
>> ----------------------------------------------------------------------
>> ETH                             on              
>>     /devices/platform/soc/ff3f0000.ethernet             unsupported
>> AUDIO                           off-0           
>> GE2D                            off-0           
>> PCI                             off-0           
>> USB                             on              
>>     /devices/platform/soc/ffe09000.usb                  active
>> NNA                             off-0           
>> VPU                             on              
>>     /devices/platform/soc/ff900000.vpu                  unsupported
>> 
>> In my tests with a framebuffer console (over HDMI), I don't see the
>> display being powered off.
>
> It's in the case where the driver is a module loaded by the post-initramfs
> system after the genpd timeout, or if the display driver is disabled.
>
> In the later I had some system failures when vendor u-boot enabled the
> display and genpd disabled the power domain later on.

OK, thanks for the explanation.  I get it now.

>> 
>>> I could set always-on governor only if the domain was already enabled,
>>> what do you think ?
>> 
>> I don't think that's necessary now that we have a driver.  We really
>> want to be able to power-down this domain when the display is not in
>> use, and if you use always_on, that will never happen.
>> 
>>> And seems I'm also missing the "This patch enable the clocks".
>> 
>> I'm not sure what patch you're referring to.
>
> It's also added in 339cd0ea082287ea8e2b7e7159a5a33665a2cbe3 "soc: amlogic: meson-gx-pwrc-vpu: fix power-off when powered by bootloader"
>
> I would like to keep the same behavior as meson-gx-pwrc-vpu, since it works fine
> and we debugged all the issues we got.

OK, that's fine with me.

We'll have to revist when we start using runtime PM enabled drviers and
want to power down the display IPs on idle, but that's fine to do later.

Thanks,

Kevin