Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains
@ 2019-08-23  9:04 Neil Armstrong
  2019-08-23  9:04 ` [PATCH v2 1/5] dt-bindings: power: add Amlogic Everything-Else power domains bindings Neil Armstrong
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Neil Armstrong @ 2019-08-23  9:04 UTC (permalink / raw)
  To: khilman, ulf.hansson
  Cc: Neil Armstrong, linux-pm, linux-amlogic, linux-arm-kernel, linux-kernel

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.

Changes since v1 at [1]:
- removed open-coded reset & clock get, enable/assert, disable/deassert
- moved to clk_bulk and reset_array with count check with a warning
- removed remaining sm1_pwrc in probe
- reordered arguments for VPU_PD and TOP_PD
- added get_power for TOP_PD aswell
- ported special VPU handling from gx-vpu-pwrc
- added shutdown driver call to avoid errors on reboot
- fixed patch 4 commit log
- collected rob's review tag on patch 1

[1] https://patchwork.kernel.org/cover/11106393/

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           | 492 ++++++++++++++++++
 include/dt-bindings/power/meson-g12a-power.h  |  13 +
 include/dt-bindings/power/meson-sm1-power.h   |  18 +
 11 files changed, 733 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/5] dt-bindings: power: add Amlogic Everything-Else power domains bindings
  2019-08-23  9:04 [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains Neil Armstrong
@ 2019-08-23  9:04 ` Neil Armstrong
  2019-08-23  9:04 ` [PATCH v2 2/5] soc: amlogic: Add support for Everything-Else power domains controller Neil Armstrong
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Neil Armstrong @ 2019-08-23  9:04 UTC (permalink / raw)
  To: khilman, ulf.hansson, devicetree
  Cc: Neil Armstrong, linux-pm, linux-amlogic, linux-arm-kernel,
	linux-kernel, Rob Herring

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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 2/5] soc: amlogic: Add support for Everything-Else power domains controller
  2019-08-23  9:04 [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains Neil Armstrong
  2019-08-23  9:04 ` [PATCH v2 1/5] dt-bindings: power: add Amlogic Everything-Else power domains bindings Neil Armstrong
@ 2019-08-23  9:04 ` Neil Armstrong
  2019-08-25 21:10   ` Martin Blumenstingl
  2019-08-23  9:04 ` [PATCH v2 3/5] arm64: meson-g12: add Everything-Else power domain controller Neil Armstrong
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Neil Armstrong @ 2019-08-23  9:04 UTC (permalink / raw)
  To: khilman, ulf.hansson
  Cc: Neil Armstrong, linux-pm, linux-amlogic, linux-arm-kernel, linux-kernel

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 | 492 ++++++++++++++++++++++++++++
 3 files changed, 504 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..5823f5b67d16
--- /dev/null
+++ b/drivers/soc/amlogic/meson-ee-pwrc.c
@@ -0,0 +1,492 @@
+// 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;
+	unsigned int reset_names_count;
+	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;
+};
+
+/* 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, __top_pd, __mem, __get_power, __resets, __clks)	\
+	{								\
+		.name = __name,						\
+		.reset_names_count = __resets,				\
+		.clk_names_count = __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, __get_power)			\
+	{								\
+		.name = __name,						\
+		.top_pd = __top_pd,					\
+		.mem_pd_count = ARRAY_SIZE(__mem),			\
+		.mem_pd = __mem,					\
+		.get_power = __get_power,				\
+	}
+
+#define MEM_PD(__name, __mem)						\
+	TOP_PD(__name, NULL, __mem, NULL)
+
+static bool pwrc_ee_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, g12a_pwrc_mem_vpu,
+				     pwrc_ee_get_power, 11, 2),
+	[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", &sm1_pwrc_vpu, sm1_pwrc_mem_vpu,
+				    pwrc_ee_get_power, 11, 2),
+	[PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna,
+				    pwrc_ee_get_power),
+	[PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb,
+				    pwrc_ee_get_power),
+	[PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie,
+				    pwrc_ee_get_power),
+	[PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d,
+				    pwrc_ee_get_power),
+	[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_bulk_data *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_ee_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_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);
+		clk_bulk_disable_unprepare(pwrc_domain->num_clks,
+					   pwrc_domain->clks);
+	}
+
+	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 = reset_control_assert(pwrc_domain->rstc);
+	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 = reset_control_deassert(pwrc_domain->rstc);
+	if (ret)
+		return ret;
+
+	return clk_bulk_prepare_enable(pwrc_domain->num_clks,
+				       pwrc_domain->clks);
+}
+
+static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
+				     struct meson_ee_pwrc *pwrc,
+				     struct meson_ee_pwrc_domain *dom)
+{
+	dom->pwrc = pwrc;
+	dom->num_rstc = dom->desc.reset_names_count;
+	dom->num_clks = dom->desc.clk_names_count;
+
+	if (dom->num_rstc) {
+		int count = reset_control_get_count(&pdev->dev);
+
+		if (count != dom->num_rstc)
+			dev_warn(&pdev->dev, "Invalid resets count %d for domain %s\n",
+				 count, dom->desc.name);
+
+		dom->rstc = devm_reset_control_array_get(&pdev->dev, false,
+							 false);
+		if (IS_ERR(dom->rstc))
+			return PTR_ERR(dom->rstc);
+	}
+
+	if (dom->num_clks) {
+		int ret = devm_clk_bulk_get_all(&pdev->dev, &dom->clks);
+		if (ret < 0)
+			return ret;
+
+		if (dom->num_clks != ret) {
+			dev_warn(&pdev->dev, "Invalid clocks count %d for domain %s\n",
+				 ret, dom->desc.name);
+			dom->num_clks = ret;
+		}
+	}
+
+	dom->base.name = dom->desc.name;
+	dom->base.power_on = meson_ee_pwrc_on;
+	dom->base.power_off = meson_ee_pwrc_off;
+
+	/*
+         * TOFIX: This is a special case for the VPU power domain, which can
+	 * be enabled previously by the bootloader. In this case the VPU
+         * pipeline may be functional but no driver maybe never attach
+         * to this power domain, and if the domain is disabled it could
+         * cause system errors. This is why the pm_domain_always_on_gov
+         * is used here.
+         * For the same reason, the clocks should be enabled in case
+         * we need to power the domain off, otherwise the internal clocks
+         * prepare/enable counters won't be in sync.
+         */
+	if (dom->num_clks && dom->desc.get_power && !dom->desc.get_power(dom)) {
+		int ret = clk_bulk_prepare_enable(dom->num_clks, dom->clks);
+		if (ret)
+			return ret;
+
+		pm_genpd_init(&dom->base, &pm_domain_always_on_gov, false);
+	} else
+		pm_genpd_init(&dom->base, NULL,
+			      (dom->desc.get_power ?
+			       dom->desc.get_power(dom) : 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 *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;
+	}
+
+	pwrc = devm_kzalloc(&pdev->dev, sizeof(*pwrc), GFP_KERNEL);
+	if (!pwrc)
+		return -ENOMEM;
+
+	pwrc->xlate.domains = devm_kcalloc(&pdev->dev, match->count,
+					   sizeof(*pwrc->xlate.domains),
+					   GFP_KERNEL);
+	if (!pwrc->xlate.domains)
+		return -ENOMEM;
+
+	pwrc->domains = devm_kcalloc(&pdev->dev, match->count,
+				     sizeof(*pwrc->domains), GFP_KERNEL);
+	if (!pwrc->domains)
+		return -ENOMEM;
+
+	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);
+	}
+
+	pwrc->regmap_ao = regmap_ao;
+	pwrc->regmap_hhi = regmap_hhi;
+
+	platform_set_drvdata(pdev, pwrc);
+
+	for (i = 0 ; i < match->count ; ++i) {
+		struct meson_ee_pwrc_domain *dom = &pwrc->domains[i];
+
+		memcpy(&dom->desc, &match->domains[i], sizeof(dom->desc));
+
+		ret = meson_ee_pwrc_init_domain(pdev, pwrc, dom);
+		if (ret)
+			return ret;
+
+		pwrc->xlate.domains[i] = &dom->base;
+	}
+
+	of_genpd_add_provider_onecell(pdev->dev.of_node, &pwrc->xlate);
+
+	return 0;
+}
+
+static void meson_ee_pwrc_shutdown(struct platform_device *pdev)
+{
+	struct meson_ee_pwrc *pwrc = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0 ; i < pwrc->xlate.num_domains ; ++i) {
+		struct meson_ee_pwrc_domain *dom = &pwrc->domains[i];
+
+		if (dom->desc.get_power && !dom->desc.get_power(dom))
+			meson_ee_pwrc_off(&dom->base);
+	}
+}
+
+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,
+	.shutdown = meson_ee_pwrc_shutdown,
+	.driver = {
+		.name		= "meson_ee_pwrc",
+		.of_match_table	= meson_ee_pwrc_match_table,
+	},
+};
+builtin_platform_driver(meson_ee_pwrc_driver);
-- 
2.22.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 3/5] arm64: meson-g12: add Everything-Else power domain controller
  2019-08-23  9:04 [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains Neil Armstrong
  2019-08-23  9:04 ` [PATCH v2 1/5] dt-bindings: power: add Amlogic Everything-Else power domains bindings Neil Armstrong
  2019-08-23  9:04 ` [PATCH v2 2/5] soc: amlogic: Add support for Everything-Else power domains controller Neil Armstrong
@ 2019-08-23  9:04 ` Neil Armstrong
  2019-08-23  9:04 ` [PATCH v2 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support Neil Armstrong
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Neil Armstrong @ 2019-08-23  9:04 UTC (permalink / raw)
  To: khilman, ulf.hansson
  Cc: Neil Armstrong, linux-pm, linux-amlogic, linux-arm-kernel, linux-kernel

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support
  2019-08-23  9:04 [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains Neil Armstrong
                   ` (2 preceding siblings ...)
  2019-08-23  9:04 ` [PATCH v2 3/5] arm64: meson-g12: add Everything-Else power domain controller Neil Armstrong
@ 2019-08-23  9:04 ` Neil Armstrong
  2019-08-25 20:00   ` Martin Blumenstingl
  2019-08-23  9:04 ` [PATCH v2 5/5] arm64: dts: meson-sm1-sei610: add USB support Neil Armstrong
  2019-08-27 22:21 ` [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains Kevin Hilman
  5 siblings, 1 reply; 14+ messages in thread
From: Neil Armstrong @ 2019-08-23  9:04 UTC (permalink / raw)
  To: khilman, ulf.hansson
  Cc: Neil Armstrong, linux-pm, linux-amlogic, linux-arm-kernel, linux-kernel

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 5/5] arm64: dts: meson-sm1-sei610: add USB support
  2019-08-23  9:04 [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains Neil Armstrong
                   ` (3 preceding siblings ...)
  2019-08-23  9:04 ` [PATCH v2 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support Neil Armstrong
@ 2019-08-23  9:04 ` Neil Armstrong
  2019-08-25 19:59   ` Martin Blumenstingl
  2019-08-27 22:21 ` [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains Kevin Hilman
  5 siblings, 1 reply; 14+ messages in thread
From: Neil Armstrong @ 2019-08-23  9:04 UTC (permalink / raw)
  To: khilman, ulf.hansson
  Cc: Neil Armstrong, linux-pm, linux-amlogic, linux-arm-kernel, linux-kernel

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 5/5] arm64: dts: meson-sm1-sei610: add USB support
  2019-08-23  9:04 ` [PATCH v2 5/5] arm64: dts: meson-sm1-sei610: add USB support Neil Armstrong
@ 2019-08-25 19:59   ` Martin Blumenstingl
  0 siblings, 0 replies; 14+ messages in thread
From: Martin Blumenstingl @ 2019-08-25 19:59 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: khilman, ulf.hansson, linux-amlogic, linux-pm, linux-kernel,
	linux-arm-kernel

On Fri, Aug 23, 2019 at 11:06 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> 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>
(based on the patch description as I don't have the schematics for this board)
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support
  2019-08-23  9:04 ` [PATCH v2 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support Neil Armstrong
@ 2019-08-25 20:00   ` Martin Blumenstingl
  0 siblings, 0 replies; 14+ messages in thread
From: Martin Blumenstingl @ 2019-08-25 20:00 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: khilman, ulf.hansson, linux-amlogic, linux-pm, linux-kernel,
	linux-arm-kernel

On Fri, Aug 23, 2019 at 11:06 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Add the HDMI support nodes for the Amlogic SM1 Based SEI610 Board.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
this looks sane so feel free to add my:
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/5] soc: amlogic: Add support for Everything-Else power domains controller
  2019-08-23  9:04 ` [PATCH v2 2/5] soc: amlogic: Add support for Everything-Else power domains controller Neil Armstrong
@ 2019-08-25 21:10   ` Martin Blumenstingl
  2019-08-26  8:10     ` Neil Armstrong
  0 siblings, 1 reply; 14+ messages in thread
From: Martin Blumenstingl @ 2019-08-25 21:10 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: khilman, ulf.hansson, linux-amlogic, linux-pm, linux-kernel,
	linux-arm-kernel

Hi Neil,

thank you for this update
I haven't tried this on the 32-bit SoCs yet, but I am confident that I
can make it work by "just" adding the SoC specific bits!

On Fri, Aug 23, 2019 at 11:06 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
[...]
> +/* 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)
should we switch to the actual register offsets like we did in the
clock drivers?

[...]
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_vpu = SM1_EE_PD(8);
nit-pick: maybe name it sm1_pwrc_vpu_hdmi as the datasheet states that
it's for "VPU/HDMI"

[...]
> +#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) }
the Amlogic implementation from buildroot-openlinux-A113-201901 (the
latest one I have)
kernel/aml-4.9/drivers/amlogic/media/vout/hdmitx/hdmi_tx_20/hw/hdmi_tx_hw.c
uses:
hd_set_reg_bits(P_HHI_MEM_PD_REG0, 0, 8, 8)
that basically translates to: GENMASK(15, 8) (which means we could
drop this macro)

the datasheet also states: 15~8 [...] HDMI memory PD (as a single
8-bit wide register)

[...]
> +static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
> +       [PWRC_G12A_VPU_ID]  = VPU_PD("VPU", &g12a_pwrc_vpu, g12a_pwrc_mem_vpu,
> +                                    pwrc_ee_get_power, 11, 2),
> +       [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", &sm1_pwrc_vpu, sm1_pwrc_mem_vpu,
> +                                   pwrc_ee_get_power, 11, 2),
> +       [PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna,
> +                                   pwrc_ee_get_power),
> +       [PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb,
> +                                   pwrc_ee_get_power),
> +       [PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie,
> +                                   pwrc_ee_get_power),
> +       [PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d,
> +                                   pwrc_ee_get_power),
> +       [PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
> +       [PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
> +};
my impression: I find this hard to read as it merges the TOP and
Memory PD domains from above, adding some seemingly random "11, 2" for
the VPU PD as well as pwrc_ee_get_power for some of the power domains
personally I like the way we describe clk_regmap because it's easy to
read (even though it adds a bit of boilerplate). I'm not sure if we
can make it work here, but this (not compile tested) is what I have in
mind (I chose two random power domains):
  [PWRC_SM1_VPU_ID]  = {
    .name = "VPU",
    .top_pd = SM1_EE_PD(8),
    .mem_pds = {
        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) },
        { HHI_MEM_PD_REG0, GENMASK(15, 8) },
    },
    .num_mem_pds = 9,
    .reset_names_count = 11,
    .clk_names_count = 2,
  },
  [PWRC_SM1_ETH_ID] = {
    .name = "ETH",
    .mem_pds = { HHI_MEM_PD_REG0, GENMASK(3, 2) },
    .num_mem_pds = 1,
  },
...

I'd like to get Kevin's feedback on this
what you have right now is probably good enough for the initial
version of this driver. I'm bringing this discussion up because we
will add support for more SoCs to this driver (we migrate GX over to
it and I want to add 32-bit SoC support, which probably means at least
Meson8 - assuming they kept the power domains identical between
Meson8/8b/8m2).

[...]
> +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_bulk_data *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;
> +};
(my impressions on this: I was surprised to find more structs down
here, I expected them to be together with the other structs further
up)

> +static bool pwrc_ee_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);
should this also check for top_pd->iso_* as well as mem_pd->*?
if the top_pd part was optional we could even use the get_power
callback for *all* power domains in this driver (right now audio and
Ethernet don't have any get_power callback)

> +}
> +
> +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);
all four udelay(20) occurrences should probably be usleep_range(20,
100); (or some other max value), see [0]

[...]
> +       /*
> +         * TOFIX: This is a special case for the VPU power domain, which can
> +        * be enabled previously by the bootloader. In this case the VPU
nit-pick: the indentation seems to be off here

[...]
> +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 *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;
> +       }
> +
> +       pwrc = devm_kzalloc(&pdev->dev, sizeof(*pwrc), GFP_KERNEL);
> +       if (!pwrc)
> +               return -ENOMEM;
> +
> +       pwrc->xlate.domains = devm_kcalloc(&pdev->dev, match->count,
> +                                          sizeof(*pwrc->xlate.domains),
> +                                          GFP_KERNEL);
> +       if (!pwrc->xlate.domains)
> +               return -ENOMEM;
> +
> +       pwrc->domains = devm_kcalloc(&pdev->dev, match->count,
> +                                    sizeof(*pwrc->domains), GFP_KERNEL);
> +       if (!pwrc->domains)
> +               return -ENOMEM;
> +
> +       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);
> +       }
> +
> +       pwrc->regmap_ao = regmap_ao;
> +       pwrc->regmap_hhi = regmap_hhi;
> +
> +       platform_set_drvdata(pdev, pwrc);
> +
> +       for (i = 0 ; i < match->count ; ++i) {
> +               struct meson_ee_pwrc_domain *dom = &pwrc->domains[i];
> +
> +               memcpy(&dom->desc, &match->domains[i], sizeof(dom->desc));
> +
> +               ret = meson_ee_pwrc_init_domain(pdev, pwrc, dom);
> +               if (ret)
> +                       return ret;
> +
> +               pwrc->xlate.domains[i] = &dom->base;
> +       }
> +
> +       of_genpd_add_provider_onecell(pdev->dev.of_node, &pwrc->xlate);
return of_genpd_add_provider_onecell(...) to propagate errors (if any)

bonus question: what about the video decoder power domains?
here is an example from vdec_1_start
(drivers/staging/media/meson/vdec/vdec_1.c):
  /* Enable power for VDEC_1 */
  regmap_update_bits(core->regmap_ao, AO_RTI_GEN_PWR_SLEEP0,
                                   GEN_PWR_VDEC_1, 0);
  usleep_range(10, 20);
  [...]
  /* enable VDEC Memories */
  amvdec_write_dos(core, DOS_MEM_PD_VDEC, 0);
  /* Remove VDEC1 Isolation */
  regmap_write(core->regmap_ao, AO_RTI_GEN_PWR_ISO0, 0);

(my point here is that it mixes video decoder "DOS" registers with
AO_RTI_GEN_PWR registers)
do we also want to add support for these "DOS" power domains to the
meson-ee-pwrc driver?
what about the AO_RTI_GEN_PWR part then - should we keep management
for the video decoder power domain bits in AO_RTI_GEN_PWR as part of
the video decoder driver?


Martin


[0] https://www.kernel.org/doc/Documentation/timers/timers-howto.rst

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/5] soc: amlogic: Add support for Everything-Else power domains controller
  2019-08-25 21:10   ` Martin Blumenstingl
@ 2019-08-26  8:10     ` Neil Armstrong
  2019-08-26 22:40       ` Martin Blumenstingl
  0 siblings, 1 reply; 14+ messages in thread
From: Neil Armstrong @ 2019-08-26  8:10 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: khilman, ulf.hansson, linux-amlogic, linux-pm, linux-kernel,
	linux-arm-kernel

On 25/08/2019 23:10, Martin Blumenstingl wrote:
> Hi Neil,
> 
> thank you for this update
> I haven't tried this on the 32-bit SoCs yet, but I am confident that I
> can make it work by "just" adding the SoC specific bits!
> 
> On Fri, Aug 23, 2019 at 11:06 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> [...]
>> +/* 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)
> should we switch to the actual register offsets like we did in the
> clock drivers?

I find it simpler to refer to the numbers in the documentation...

> 
> [...]
>> +static struct meson_ee_pwrc_top_domain sm1_pwrc_vpu = SM1_EE_PD(8);
> nit-pick: maybe name it sm1_pwrc_vpu_hdmi as the datasheet states that
> it's for "VPU/HDMI"

Maybe

> 
> [...]
>> +#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) }
> the Amlogic implementation from buildroot-openlinux-A113-201901 (the
> latest one I have)
> kernel/aml-4.9/drivers/amlogic/media/vout/hdmitx/hdmi_tx_20/hw/hdmi_tx_hw.c
> uses:
> hd_set_reg_bits(P_HHI_MEM_PD_REG0, 0, 8, 8)
> that basically translates to: GENMASK(15, 8) (which means we could
> drop this macro)
> 
> the datasheet also states: 15~8 [...] HDMI memory PD (as a single
> 8-bit wide register)

Yep, but the actual code setting the VPU power domain is in u-boot :

drivers/vpu/aml_vpu_power_init.c:
108         for (i = 8; i < 16; i++) {
109                 vpu_hiu_setb(HHI_MEM_PD_REG0, 0, i, 1);
110                 udelay(5);
111         }

the linux code is like never used here, my preference goes to the u-boot code
implementation.

> 
> [...]
>> +static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
>> +       [PWRC_G12A_VPU_ID]  = VPU_PD("VPU", &g12a_pwrc_vpu, g12a_pwrc_mem_vpu,
>> +                                    pwrc_ee_get_power, 11, 2),
>> +       [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", &sm1_pwrc_vpu, sm1_pwrc_mem_vpu,
>> +                                   pwrc_ee_get_power, 11, 2),
>> +       [PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna,
>> +                                   pwrc_ee_get_power),
>> +       [PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb,
>> +                                   pwrc_ee_get_power),
>> +       [PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie,
>> +                                   pwrc_ee_get_power),
>> +       [PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d,
>> +                                   pwrc_ee_get_power),
>> +       [PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
>> +       [PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
>> +};
> my impression: I find this hard to read as it merges the TOP and
> Memory PD domains from above, adding some seemingly random "11, 2" for
> the VPU PD as well as pwrc_ee_get_power for some of the power domains
> personally I like the way we describe clk_regmap because it's easy to
> read (even though it adds a bit of boilerplate). I'm not sure if we
> can make it work here, but this (not compile tested) is what I have in
> mind (I chose two random power domains):
>   [PWRC_SM1_VPU_ID]  = {
>     .name = "VPU",
>     .top_pd = SM1_EE_PD(8),
>     .mem_pds = {
>         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) },
>         { HHI_MEM_PD_REG0, GENMASK(15, 8) },
>     },
>     .num_mem_pds = 9,
>     .reset_names_count = 11,
>     .clk_names_count = 2,
>   },
>   [PWRC_SM1_ETH_ID] = {
>     .name = "ETH",
>     .mem_pds = { HHI_MEM_PD_REG0, GENMASK(3, 2) },
>     .num_mem_pds = 1,
>   },
> ...
> 
> I'd like to get Kevin's feedback on this
> what you have right now is probably good enough for the initial
> version of this driver. I'm bringing this discussion up because we
> will add support for more SoCs to this driver (we migrate GX over to
> it and I want to add 32-bit SoC support, which probably means at least
> Meson8 - assuming they kept the power domains identical between
> Meson8/8b/8m2).

I find it more compact, but nothing is set in stone, you can refactor this as
will when adding meson8 support, no problems here.

> 
> [...]
>> +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_bulk_data *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;
>> +};
> (my impressions on this: I was surprised to find more structs down
> here, I expected them to be together with the other structs further
> up)

These are the "live" structures, opposed to the static structures defining the
data and these are allocated and filled a probe time.

I dislike changing static global data at runtime, this is why I clearly separated both.

> 
>> +static bool pwrc_ee_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);
> should this also check for top_pd->iso_* as well as mem_pd->*?
> if the top_pd part was optional we could even use the get_power
> callback for *all* power domains in this driver (right now audio and
> Ethernet don't have any get_power callback)

We could, but how should we handle if one unexpected bit is set ? No idea...

> 
>> +}
>> +
>> +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);
> all four udelay(20) occurrences should probably be usleep_range(20,
> 100); (or some other max value), see [0]

Ok

> 
> [...]
>> +       /*
>> +         * TOFIX: This is a special case for the VPU power domain, which can
>> +        * be enabled previously by the bootloader. In this case the VPU
> nit-pick: the indentation seems to be off here

Exact

> 
> [...]
>> +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 *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;
>> +       }
>> +
>> +       pwrc = devm_kzalloc(&pdev->dev, sizeof(*pwrc), GFP_KERNEL);
>> +       if (!pwrc)
>> +               return -ENOMEM;
>> +
>> +       pwrc->xlate.domains = devm_kcalloc(&pdev->dev, match->count,
>> +                                          sizeof(*pwrc->xlate.domains),
>> +                                          GFP_KERNEL);
>> +       if (!pwrc->xlate.domains)
>> +               return -ENOMEM;
>> +
>> +       pwrc->domains = devm_kcalloc(&pdev->dev, match->count,
>> +                                    sizeof(*pwrc->domains), GFP_KERNEL);
>> +       if (!pwrc->domains)
>> +               return -ENOMEM;
>> +
>> +       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);
>> +       }
>> +
>> +       pwrc->regmap_ao = regmap_ao;
>> +       pwrc->regmap_hhi = regmap_hhi;
>> +
>> +       platform_set_drvdata(pdev, pwrc);
>> +
>> +       for (i = 0 ; i < match->count ; ++i) {
>> +               struct meson_ee_pwrc_domain *dom = &pwrc->domains[i];
>> +
>> +               memcpy(&dom->desc, &match->domains[i], sizeof(dom->desc));
>> +
>> +               ret = meson_ee_pwrc_init_domain(pdev, pwrc, dom);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               pwrc->xlate.domains[i] = &dom->base;
>> +       }
>> +
>> +       of_genpd_add_provider_onecell(pdev->dev.of_node, &pwrc->xlate);
> return of_genpd_add_provider_onecell(...) to propagate errors (if any)

Indeed, thanks.

> 
> bonus question: what about the video decoder power domains?
> here is an example from vdec_1_start
> (drivers/staging/media/meson/vdec/vdec_1.c):
>   /* Enable power for VDEC_1 */
>   regmap_update_bits(core->regmap_ao, AO_RTI_GEN_PWR_SLEEP0,
>                                    GEN_PWR_VDEC_1, 0);
>   usleep_range(10, 20);
>   [...]
>   /* enable VDEC Memories */
>   amvdec_write_dos(core, DOS_MEM_PD_VDEC, 0);
>   /* Remove VDEC1 Isolation */
>   regmap_write(core->regmap_ao, AO_RTI_GEN_PWR_ISO0, 0);
> 
> (my point here is that it mixes video decoder "DOS" registers with
> AO_RTI_GEN_PWR registers)
> do we also want to add support for these "DOS" power domains to the
> meson-ee-pwrc driver?
> what about the AO_RTI_GEN_PWR part then - should we keep management
> for the video decoder power domain bits in AO_RTI_GEN_PWR as part of
> the video decoder driver?

I left the decoders power domains aside so we can discuss it later on,
we should expose multiple power domains, but the driver would need to
be changed to support multiple power domains. But will loose the ability
to enable/disable each domain at will unless it created a sub-device for
each decoder and attaches the domain to to each device and use runtime pm.

It's simpler to discuss it later on !

Thanks for the review,
Neil

> 
> 
> Martin
> 
> 
> [0] https://www.kernel.org/doc/Documentation/timers/timers-howto.rst
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/5] soc: amlogic: Add support for Everything-Else power domains controller
  2019-08-26  8:10     ` Neil Armstrong
@ 2019-08-26 22:40       ` Martin Blumenstingl
  2019-08-27 10:11         ` Neil Armstrong
  0 siblings, 1 reply; 14+ messages in thread
From: Martin Blumenstingl @ 2019-08-26 22:40 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: khilman, ulf.hansson, linux-amlogic, linux-pm, linux-kernel,
	linux-arm-kernel

Hi Neil,

On Mon, Aug 26, 2019 at 10:10 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> On 25/08/2019 23:10, Martin Blumenstingl wrote:
> > Hi Neil,
> >
> > thank you for this update
> > I haven't tried this on the 32-bit SoCs yet, but I am confident that I
> > can make it work by "just" adding the SoC specific bits!
> >
> > On Fri, Aug 23, 2019 at 11:06 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> > [...]
> >> +/* 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)
> > should we switch to the actual register offsets like we did in the
> > clock drivers?
>
> I find it simpler to refer to the numbers in the documentation...
OK, I have no strong preference here
for the 32-bit SoCs I will need to use the offsets based on the
"amlogic,meson8b-pmu", "syscon" [0], so these will be magic anyways

[...]
> >> +#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) }
> > the Amlogic implementation from buildroot-openlinux-A113-201901 (the
> > latest one I have)
> > kernel/aml-4.9/drivers/amlogic/media/vout/hdmitx/hdmi_tx_20/hw/hdmi_tx_hw.c
> > uses:
> > hd_set_reg_bits(P_HHI_MEM_PD_REG0, 0, 8, 8)
> > that basically translates to: GENMASK(15, 8) (which means we could
> > drop this macro)
> >
> > the datasheet also states: 15~8 [...] HDMI memory PD (as a single
> > 8-bit wide register)
>
> Yep, but the actual code setting the VPU power domain is in u-boot :
>
> drivers/vpu/aml_vpu_power_init.c:
> 108         for (i = 8; i < 16; i++) {
> 109                 vpu_hiu_setb(HHI_MEM_PD_REG0, 0, i, 1);
> 110                 udelay(5);
> 111         }
>
> the linux code is like never used here, my preference goes to the u-boot code
> implementation.
I see, let's keep your implementation then

> >
> > [...]
> >> +static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
> >> +       [PWRC_G12A_VPU_ID]  = VPU_PD("VPU", &g12a_pwrc_vpu, g12a_pwrc_mem_vpu,
> >> +                                    pwrc_ee_get_power, 11, 2),
> >> +       [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", &sm1_pwrc_vpu, sm1_pwrc_mem_vpu,
> >> +                                   pwrc_ee_get_power, 11, 2),
> >> +       [PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna,
> >> +                                   pwrc_ee_get_power),
> >> +       [PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb,
> >> +                                   pwrc_ee_get_power),
> >> +       [PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie,
> >> +                                   pwrc_ee_get_power),
> >> +       [PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d,
> >> +                                   pwrc_ee_get_power),
> >> +       [PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
> >> +       [PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
> >> +};
> > my impression: I find this hard to read as it merges the TOP and
> > Memory PD domains from above, adding some seemingly random "11, 2" for
> > the VPU PD as well as pwrc_ee_get_power for some of the power domains
> > personally I like the way we describe clk_regmap because it's easy to
> > read (even though it adds a bit of boilerplate). I'm not sure if we
> > can make it work here, but this (not compile tested) is what I have in
> > mind (I chose two random power domains):
> >   [PWRC_SM1_VPU_ID]  = {
> >     .name = "VPU",
> >     .top_pd = SM1_EE_PD(8),
> >     .mem_pds = {
> >         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) },
> >         { HHI_MEM_PD_REG0, GENMASK(15, 8) },
> >     },
> >     .num_mem_pds = 9,
> >     .reset_names_count = 11,
> >     .clk_names_count = 2,
> >   },
> >   [PWRC_SM1_ETH_ID] = {
> >     .name = "ETH",
> >     .mem_pds = { HHI_MEM_PD_REG0, GENMASK(3, 2) },
> >     .num_mem_pds = 1,
> >   },
> > ...
> >
> > I'd like to get Kevin's feedback on this
> > what you have right now is probably good enough for the initial
> > version of this driver. I'm bringing this discussion up because we
> > will add support for more SoCs to this driver (we migrate GX over to
> > it and I want to add 32-bit SoC support, which probably means at least
> > Meson8 - assuming they kept the power domains identical between
> > Meson8/8b/8m2).
>
> I find it more compact, but nothing is set in stone, you can refactor this as
> will when adding meson8 support, no problems here.
OK. if Kevin (or someone else) has feedback on this then I don't have
to waste time if it turns out that it's not a great idea ;)

> >
> > [...]
> >> +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_bulk_data *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;
> >> +};
> > (my impressions on this: I was surprised to find more structs down
> > here, I expected them to be together with the other structs further
> > up)
>
> These are the "live" structures, opposed to the static structures defining the
> data and these are allocated and filled a probe time.
I see, thanks for the explanation

> I dislike changing static global data at runtime, this is why I clearly separated both.
I didn't mean to make them static - the thing that caught my eye was
that some of the structs are defined at the top of the driver while
these two are define much further down
I am used to having all struct definitions in one place

> >
> >> +static bool pwrc_ee_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);
> > should this also check for top_pd->iso_* as well as mem_pd->*?
> > if the top_pd part was optional we could even use the get_power
> > callback for *all* power domains in this driver (right now audio and
> > Ethernet don't have any get_power callback)
>
> We could, but how should we handle if one unexpected bit is set ? No idea...
hmm, I see
if we need it for other power domains then we can still implement it,
so it's good for now

[...]
> > bonus question: what about the video decoder power domains?
> > here is an example from vdec_1_start
> > (drivers/staging/media/meson/vdec/vdec_1.c):
> >   /* Enable power for VDEC_1 */
> >   regmap_update_bits(core->regmap_ao, AO_RTI_GEN_PWR_SLEEP0,
> >                                    GEN_PWR_VDEC_1, 0);
> >   usleep_range(10, 20);
> >   [...]
> >   /* enable VDEC Memories */
> >   amvdec_write_dos(core, DOS_MEM_PD_VDEC, 0);
> >   /* Remove VDEC1 Isolation */
> >   regmap_write(core->regmap_ao, AO_RTI_GEN_PWR_ISO0, 0);
> >
> > (my point here is that it mixes video decoder "DOS" registers with
> > AO_RTI_GEN_PWR registers)
> > do we also want to add support for these "DOS" power domains to the
> > meson-ee-pwrc driver?
> > what about the AO_RTI_GEN_PWR part then - should we keep management
> > for the video decoder power domain bits in AO_RTI_GEN_PWR as part of
> > the video decoder driver?
>
> I left the decoders power domains aside so we can discuss it later on,
> we should expose multiple power domains, but the driver would need to
> be changed to support multiple power domains. But will loose the ability
> to enable/disable each domain at will unless it created a sub-device for
> each decoder and attaches the domain to to each device and use runtime pm.
>
> It's simpler to discuss it later on !
OK - does this mean you and/or Maxime have "discuss decoder power
domains" on your (long) TODO-list or do you want me to open this
discussion after this driver is merged?


Martin


[0] https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/amlogic/pmu.txt

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/5] soc: amlogic: Add support for Everything-Else power domains controller
  2019-08-26 22:40       ` Martin Blumenstingl
@ 2019-08-27 10:11         ` Neil Armstrong
  2019-08-27 22:20           ` Kevin Hilman
  0 siblings, 1 reply; 14+ messages in thread
From: Neil Armstrong @ 2019-08-27 10:11 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: khilman, ulf.hansson, linux-amlogic, linux-pm, linux-kernel,
	linux-arm-kernel

On 27/08/2019 00:40, Martin Blumenstingl wrote:
> Hi Neil,
> 
> On Mon, Aug 26, 2019 at 10:10 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> On 25/08/2019 23:10, Martin Blumenstingl wrote:
>>> Hi Neil,
>>>
>>> thank you for this update
>>> I haven't tried this on the 32-bit SoCs yet, but I am confident that I
>>> can make it work by "just" adding the SoC specific bits!
>>>
>>> On Fri, Aug 23, 2019 at 11:06 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>> [...]
>>>> +/* 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)
>>> should we switch to the actual register offsets like we did in the
>>> clock drivers?
>>
>> I find it simpler to refer to the numbers in the documentation...
> OK, I have no strong preference here
> for the 32-bit SoCs I will need to use the offsets based on the
> "amlogic,meson8b-pmu", "syscon" [0], so these will be magic anyways
> 
> [...]
>>>> +#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) }
>>> the Amlogic implementation from buildroot-openlinux-A113-201901 (the
>>> latest one I have)
>>> kernel/aml-4.9/drivers/amlogic/media/vout/hdmitx/hdmi_tx_20/hw/hdmi_tx_hw.c
>>> uses:
>>> hd_set_reg_bits(P_HHI_MEM_PD_REG0, 0, 8, 8)
>>> that basically translates to: GENMASK(15, 8) (which means we could
>>> drop this macro)
>>>
>>> the datasheet also states: 15~8 [...] HDMI memory PD (as a single
>>> 8-bit wide register)
>>
>> Yep, but the actual code setting the VPU power domain is in u-boot :
>>
>> drivers/vpu/aml_vpu_power_init.c:
>> 108         for (i = 8; i < 16; i++) {
>> 109                 vpu_hiu_setb(HHI_MEM_PD_REG0, 0, i, 1);
>> 110                 udelay(5);
>> 111         }
>>
>> the linux code is like never used here, my preference goes to the u-boot code
>> implementation.
> I see, let's keep your implementation then
> 
>>>
>>> [...]
>>>> +static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
>>>> +       [PWRC_G12A_VPU_ID]  = VPU_PD("VPU", &g12a_pwrc_vpu, g12a_pwrc_mem_vpu,
>>>> +                                    pwrc_ee_get_power, 11, 2),
>>>> +       [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", &sm1_pwrc_vpu, sm1_pwrc_mem_vpu,
>>>> +                                   pwrc_ee_get_power, 11, 2),
>>>> +       [PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna,
>>>> +                                   pwrc_ee_get_power),
>>>> +       [PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb,
>>>> +                                   pwrc_ee_get_power),
>>>> +       [PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie,
>>>> +                                   pwrc_ee_get_power),
>>>> +       [PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d,
>>>> +                                   pwrc_ee_get_power),
>>>> +       [PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
>>>> +       [PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
>>>> +};
>>> my impression: I find this hard to read as it merges the TOP and
>>> Memory PD domains from above, adding some seemingly random "11, 2" for
>>> the VPU PD as well as pwrc_ee_get_power for some of the power domains
>>> personally I like the way we describe clk_regmap because it's easy to
>>> read (even though it adds a bit of boilerplate). I'm not sure if we
>>> can make it work here, but this (not compile tested) is what I have in
>>> mind (I chose two random power domains):
>>>   [PWRC_SM1_VPU_ID]  = {
>>>     .name = "VPU",
>>>     .top_pd = SM1_EE_PD(8),
>>>     .mem_pds = {
>>>         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) },
>>>         { HHI_MEM_PD_REG0, GENMASK(15, 8) },
>>>     },
>>>     .num_mem_pds = 9,
>>>     .reset_names_count = 11,
>>>     .clk_names_count = 2,
>>>   },
>>>   [PWRC_SM1_ETH_ID] = {
>>>     .name = "ETH",
>>>     .mem_pds = { HHI_MEM_PD_REG0, GENMASK(3, 2) },
>>>     .num_mem_pds = 1,
>>>   },
>>> ...
>>>
>>> I'd like to get Kevin's feedback on this
>>> what you have right now is probably good enough for the initial
>>> version of this driver. I'm bringing this discussion up because we
>>> will add support for more SoCs to this driver (we migrate GX over to
>>> it and I want to add 32-bit SoC support, which probably means at least
>>> Meson8 - assuming they kept the power domains identical between
>>> Meson8/8b/8m2).
>>
>> I find it more compact, but nothing is set in stone, you can refactor this as
>> will when adding meson8 support, no problems here.
> OK. if Kevin (or someone else) has feedback on this then I don't have
> to waste time if it turns out that it's not a great idea ;)
> 
>>>
>>> [...]
>>>> +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_bulk_data *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;
>>>> +};
>>> (my impressions on this: I was surprised to find more structs down
>>> here, I expected them to be together with the other structs further
>>> up)
>>
>> These are the "live" structures, opposed to the static structures defining the
>> data and these are allocated and filled a probe time.
> I see, thanks for the explanation
> 
>> I dislike changing static global data at runtime, this is why I clearly separated both.
> I didn't mean to make them static - the thing that caught my eye was
> that some of the structs are defined at the top of the driver while
> these two are define much further down
> I am used to having all struct definitions in one place

I'll let Kevin leave his feedback on this aswell.

> 
>>>
>>>> +static bool pwrc_ee_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);
>>> should this also check for top_pd->iso_* as well as mem_pd->*?
>>> if the top_pd part was optional we could even use the get_power
>>> callback for *all* power domains in this driver (right now audio and
>>> Ethernet don't have any get_power callback)
>>
>> We could, but how should we handle if one unexpected bit is set ? No idea...
> hmm, I see
> if we need it for other power domains then we can still implement it,
> so it's good for now
> 
> [...]
>>> bonus question: what about the video decoder power domains?
>>> here is an example from vdec_1_start
>>> (drivers/staging/media/meson/vdec/vdec_1.c):
>>>   /* Enable power for VDEC_1 */
>>>   regmap_update_bits(core->regmap_ao, AO_RTI_GEN_PWR_SLEEP0,
>>>                                    GEN_PWR_VDEC_1, 0);
>>>   usleep_range(10, 20);
>>>   [...]
>>>   /* enable VDEC Memories */
>>>   amvdec_write_dos(core, DOS_MEM_PD_VDEC, 0);
>>>   /* Remove VDEC1 Isolation */
>>>   regmap_write(core->regmap_ao, AO_RTI_GEN_PWR_ISO0, 0);
>>>
>>> (my point here is that it mixes video decoder "DOS" registers with
>>> AO_RTI_GEN_PWR registers)
>>> do we also want to add support for these "DOS" power domains to the
>>> meson-ee-pwrc driver?
>>> what about the AO_RTI_GEN_PWR part then - should we keep management
>>> for the video decoder power domain bits in AO_RTI_GEN_PWR as part of
>>> the video decoder driver?
>>
>> I left the decoders power domains aside so we can discuss it later on,
>> we should expose multiple power domains, but the driver would need to
>> be changed to support multiple power domains. But will loose the ability
>> to enable/disable each domain at will unless it created a sub-device for
>> each decoder and attaches the domain to to each device and use runtime pm.
>>
>> It's simpler to discuss it later on !
> OK - does this mean you and/or Maxime have "discuss decoder power
> domains" on your (long) TODO-list or do you want me to open this
> discussion after this driver is merged?

Both I think, let this be an open discussion !

Neil

> 
> 
> Martin
> 
> 
> [0] https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/amlogic/pmu.txt
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/5] soc: amlogic: Add support for Everything-Else power domains controller
  2019-08-27 10:11         ` Neil Armstrong
@ 2019-08-27 22:20           ` Kevin Hilman
  0 siblings, 0 replies; 14+ messages in thread
From: Kevin Hilman @ 2019-08-27 22:20 UTC (permalink / raw)
  To: Neil Armstrong, Martin Blumenstingl
  Cc: ulf.hansson, linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel

Neil Armstrong <narmstrong@baylibre.com> writes:

> On 27/08/2019 00:40, Martin Blumenstingl wrote:
>> Hi Neil,
>> 
>> On Mon, Aug 26, 2019 at 10:10 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>>
>>> On 25/08/2019 23:10, Martin Blumenstingl wrote:
>>>> Hi Neil,
>>>>
>>>> thank you for this update
>>>> I haven't tried this on the 32-bit SoCs yet, but I am confident that I
>>>> can make it work by "just" adding the SoC specific bits!
>>>>
>>>> On Fri, Aug 23, 2019 at 11:06 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>>> [...]
>>>>> +/* 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)
>>>> should we switch to the actual register offsets like we did in the
>>>> clock drivers?
>>>
>>> I find it simpler to refer to the numbers in the documentation...
>> OK, I have no strong preference here
>> for the 32-bit SoCs I will need to use the offsets based on the
>> "amlogic,meson8b-pmu", "syscon" [0], so these will be magic anyways
>> 
>> [...]
>>>>> +#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) }
>>>> the Amlogic implementation from buildroot-openlinux-A113-201901 (the
>>>> latest one I have)
>>>> kernel/aml-4.9/drivers/amlogic/media/vout/hdmitx/hdmi_tx_20/hw/hdmi_tx_hw.c
>>>> uses:
>>>> hd_set_reg_bits(P_HHI_MEM_PD_REG0, 0, 8, 8)
>>>> that basically translates to: GENMASK(15, 8) (which means we could
>>>> drop this macro)
>>>>
>>>> the datasheet also states: 15~8 [...] HDMI memory PD (as a single
>>>> 8-bit wide register)
>>>
>>> Yep, but the actual code setting the VPU power domain is in u-boot :
>>>
>>> drivers/vpu/aml_vpu_power_init.c:
>>> 108         for (i = 8; i < 16; i++) {
>>> 109                 vpu_hiu_setb(HHI_MEM_PD_REG0, 0, i, 1);
>>> 110                 udelay(5);
>>> 111         }
>>>
>>> the linux code is like never used here, my preference goes to the u-boot code
>>> implementation.
>> I see, let's keep your implementation then
>> 
>>>>
>>>> [...]
>>>>> +static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
>>>>> +       [PWRC_G12A_VPU_ID]  = VPU_PD("VPU", &g12a_pwrc_vpu, g12a_pwrc_mem_vpu,
>>>>> +                                    pwrc_ee_get_power, 11, 2),
>>>>> +       [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", &sm1_pwrc_vpu, sm1_pwrc_mem_vpu,
>>>>> +                                   pwrc_ee_get_power, 11, 2),
>>>>> +       [PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna,
>>>>> +                                   pwrc_ee_get_power),
>>>>> +       [PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb,
>>>>> +                                   pwrc_ee_get_power),
>>>>> +       [PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie,
>>>>> +                                   pwrc_ee_get_power),
>>>>> +       [PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d,
>>>>> +                                   pwrc_ee_get_power),
>>>>> +       [PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
>>>>> +       [PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
>>>>> +};
>>>> my impression: I find this hard to read as it merges the TOP and
>>>> Memory PD domains from above, adding some seemingly random "11, 2" for
>>>> the VPU PD as well as pwrc_ee_get_power for some of the power domains
>>>> personally I like the way we describe clk_regmap because it's easy to
>>>> read (even though it adds a bit of boilerplate). I'm not sure if we
>>>> can make it work here, but this (not compile tested) is what I have in
>>>> mind (I chose two random power domains):
>>>>   [PWRC_SM1_VPU_ID]  = {
>>>>     .name = "VPU",
>>>>     .top_pd = SM1_EE_PD(8),
>>>>     .mem_pds = {
>>>>         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) },
>>>>         { HHI_MEM_PD_REG0, GENMASK(15, 8) },
>>>>     },
>>>>     .num_mem_pds = 9,
>>>>     .reset_names_count = 11,
>>>>     .clk_names_count = 2,
>>>>   },
>>>>   [PWRC_SM1_ETH_ID] = {
>>>>     .name = "ETH",
>>>>     .mem_pds = { HHI_MEM_PD_REG0, GENMASK(3, 2) },
>>>>     .num_mem_pds = 1,
>>>>   },
>>>> ...
>>>>
>>>> I'd like to get Kevin's feedback on this
>>>> what you have right now is probably good enough for the initial
>>>> version of this driver. I'm bringing this discussion up because we
>>>> will add support for more SoCs to this driver (we migrate GX over to
>>>> it and I want to add 32-bit SoC support, which probably means at least
>>>> Meson8 - assuming they kept the power domains identical between
>>>> Meson8/8b/8m2).
>>>
>>> I find it more compact, but nothing is set in stone, you can refactor this as
>>> will when adding meson8 support, no problems here.
>> OK. if Kevin (or someone else) has feedback on this then I don't have
>> to waste time if it turns out that it's not a great idea ;)
>> 
>>>>
>>>> [...]
>>>>> +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_bulk_data *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;
>>>>> +};
>>>> (my impressions on this: I was surprised to find more structs down
>>>> here, I expected them to be together with the other structs further
>>>> up)
>>>
>>> These are the "live" structures, opposed to the static structures defining the
>>> data and these are allocated and filled a probe time.
>> I see, thanks for the explanation
>> 
>>> I dislike changing static global data at runtime, this is why I clearly separated both.
>> I didn't mean to make them static - the thing that caught my eye was
>> that some of the structs are defined at the top of the driver while
>> these two are define much further down
>> I am used to having all struct definitions in one place
>
> I'll let Kevin leave his feedback on this aswell.

I don't find the current approach super easy to read either, but OTOH, I
don't have any (good) ideas for doing it better, so I'm fine with the
current approach.

My primay wish is to have a single domain controller for all the EE
domains, which this series provides well.  We can iterate on cleanups as
we expand to other SoCs.

Unless there are major objections, I plan to queue this for v5.4.

Thanks,

Kevin

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains
  2019-08-23  9:04 [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains Neil Armstrong
                   ` (4 preceding siblings ...)
  2019-08-23  9:04 ` [PATCH v2 5/5] arm64: dts: meson-sm1-sei610: add USB support Neil Armstrong
@ 2019-08-27 22:21 ` Kevin Hilman
  5 siblings, 0 replies; 14+ messages in thread
From: Kevin Hilman @ 2019-08-27 22:21 UTC (permalink / raw)
  To: Neil Armstrong, ulf.hansson
  Cc: Neil Armstrong, linux-pm, linux-amlogic, linux-arm-kernel, linux-kernel

Neil Armstrong <narmstrong@baylibre.com> writes:

> 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.
>
> Changes since v1 at [1]:
> - removed open-coded reset & clock get, enable/assert, disable/deassert
> - moved to clk_bulk and reset_array with count check with a warning
> - removed remaining sm1_pwrc in probe
> - reordered arguments for VPU_PD and TOP_PD
> - added get_power for TOP_PD aswell
> - ported special VPU handling from gx-vpu-pwrc
> - added shutdown driver call to avoid errors on reboot
> - fixed patch 4 commit log
> - collected rob's review tag on patch 1
>
> [1] https://patchwork.kernel.org/cover/11106393/

Series queued for v5.4...

> Neil Armstrong (5):
>   dt-bindings: power: add Amlogic Everything-Else power domains bindings
>   soc: amlogic: Add support for Everything-Else power domains controller

These two in v5.4/drivers,

>   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

and these 3 in v5.4/dt64,

Thanks,

Kevin

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23  9:04 [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains Neil Armstrong
2019-08-23  9:04 ` [PATCH v2 1/5] dt-bindings: power: add Amlogic Everything-Else power domains bindings Neil Armstrong
2019-08-23  9:04 ` [PATCH v2 2/5] soc: amlogic: Add support for Everything-Else power domains controller Neil Armstrong
2019-08-25 21:10   ` Martin Blumenstingl
2019-08-26  8:10     ` Neil Armstrong
2019-08-26 22:40       ` Martin Blumenstingl
2019-08-27 10:11         ` Neil Armstrong
2019-08-27 22:20           ` Kevin Hilman
2019-08-23  9:04 ` [PATCH v2 3/5] arm64: meson-g12: add Everything-Else power domain controller Neil Armstrong
2019-08-23  9:04 ` [PATCH v2 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support Neil Armstrong
2019-08-25 20:00   ` Martin Blumenstingl
2019-08-23  9:04 ` [PATCH v2 5/5] arm64: dts: meson-sm1-sei610: add USB support Neil Armstrong
2019-08-25 19:59   ` Martin Blumenstingl
2019-08-27 22:21 ` [PATCH v2 0/5] arm64: meson: add support for SM1 Power Domains Kevin Hilman

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org linux-pm@archiver.kernel.org
	public-inbox-index linux-pm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox