[PATCH 0/7] Apple SoC PMGR device power states driver

[PATCH 0/7] Apple SoC PMGR device power states driver

[Hector Martin]

This series adds the driver for the Apple PMGR device power state
registers. These registers can clockgate and (in some cases) powergate
specific SoC blocks. They also control the reset line, and can have
additional features such as automatic power management.

The current driver supports only the lowest/highest power states,
provided via the genpd framework, plus reset support provided via
the reset subsystem.

Apple's PMGRs (there are two in the T8103) have a uniform register
bit layout (sometimes with varying features). To be able to support
multiple SoC generations as well as express pd relationships
dynamically, this binding describes each PMGR power state control
as a single devicetree node. Future SoC generations are expected to
retain backwards compatibility, allowing this driver to work on them
with only DT changes.

#1-#2: Adds the required device tree bindings
#3: The driver itself.
#4: Somewhat unrelated DT change, but I wanted to get it out of the way
    for #7
#5: Instantiates the driver in t8103.dtsi.
#6: Adds runtime-pm support to the Samsung UART driver, as a first
    consumer.
#7: Instantiates a second UART, to more easily test this.

There are currently no consumers for the reset functionality, so
it is untested, but we will be testing it soon with the NVMe driver
(as it is required to allow driver re-binding to work properly).

Hector Martin (7):
  dt-bindings: arm: apple: Add apple,pmgr binding
  dt-bindings: power: Add apple,pmgr-pwrstate binding
  soc: apple: Add driver for Apple PMGR power state controls
  arm64: dts: apple: t8103: Rename clk24 to clkref
  arm64: dts: apple: t8103: Add the UART PMGR tree
  tty: serial: samsung_tty: Support runtime PM
  arm64: dts: apple: t8103: Add UART2

 .../bindings/arm/apple/apple,pmgr.yaml        |  74 +++++
 .../bindings/power/apple,pmgr-pwrstate.yaml   | 117 ++++++++
 MAINTAINERS                                   |   3 +
 arch/arm64/boot/dts/apple/t8103-j274.dts      |   5 +
 arch/arm64/boot/dts/apple/t8103.dtsi          | 134 ++++++++-
 drivers/soc/Kconfig                           |   1 +
 drivers/soc/Makefile                          |   1 +
 drivers/soc/apple/Kconfig                     |  21 ++
 drivers/soc/apple/Makefile                    |   2 +
 drivers/soc/apple/apple-pmgr-pwrstate.c       | 281 ++++++++++++++++++
 drivers/tty/serial/samsung_tty.c              |  88 +++---
 11 files changed, 690 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
 create mode 100644 Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
 create mode 100644 drivers/soc/apple/Kconfig
 create mode 100644 drivers/soc/apple/Makefile
 create mode 100644 drivers/soc/apple/apple-pmgr-pwrstate.c

-- 
2.33.0

[PATCH 1/7] dt-bindings: arm: apple: Add apple,pmgr binding

[Hector Martin]

The PMGR block in Apple Silicon SoCs is responsible for SoC power
management. There are two PMGRs in T8103, with different register
layouts but compatible registers. In order to support this as well
as future SoC generations with backwards-compatible registers, we
declare these blocks as syscons and bind to individual registers
in child nodes. Each register controls one SoC device.

The respective apple compatibles are defined in case device-specific
quirks are necessary in the future, but currently these nodes are
expected to be bound by the generic syscon driver.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 .../bindings/arm/apple/apple,pmgr.yaml        | 74 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml

diff --git a/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
new file mode 100644
index 000000000000..0304164e4140
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/apple/apple,pmgr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple SoC Power Manager (PMGR)
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+description: |
+  Apple SoCs include a PMGR block responsible for power management,
+  which can control various clocks, resets, power states, and
+  performance features. This node represents the PMGR as a syscon,
+  with sub-nodes representing individual features.
+
+  Apple SoCs may have a secondary "mini-PMGR"; it is represented
+  separately in the device tree, but works the same way.
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - apple,t8103-pmgr
+          - apple,t8103-minipmgr
+          - apple,pmgr
+
+  required:
+    - compatible
+
+properties:
+  $nodename:
+    pattern: "^power-management@[0-9a-f]+$"
+
+  compatible:
+    items:
+      - enum:
+          - apple,t8103-pmgr
+          - apple,t8103-minipmgr
+      - const: apple,pmgr
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: true
+
+examples:
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        power-management@23b700000 {
+            compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <0x2 0x3b700000 0x0 0x14000>;
+        };
+
+        power-management@23b700000 {
+            compatible = "apple,t8103-minipmgr", "apple,pmgr", "syscon", "simple-mfd";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <0x2 0x3d280000 0x0 0xc000>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index abdcbcfef73d..d25598842d15 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1719,6 +1719,7 @@ B:	https://github.com/AsahiLinux/linux/issues
 C:	irc://irc.oftc.net/asahi-dev
 T:	git https://github.com/AsahiLinux/linux.git
 F:	Documentation/devicetree/bindings/arm/apple.yaml
+F:	Documentation/devicetree/bindings/arm/apple/*
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
 F:	arch/arm64/boot/dts/apple/
-- 
2.33.0

[PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Hector Martin]

This syscon child node represents a single SoC device controlled by the
PMGR block. This layout allows us to declare all device power state
controls (power/clock gating and reset) in the device tree, including
dependencies, instead of hardcoding it into the driver. The register
layout is uniform.

Each pmgr-pwrstate node provides genpd and reset features, to be
consumed by downstream device nodes.

Future SoCs are expected to use backwards compatible registers, and the
"apple,pmgr-pwrstate" represents any such interfaces (possibly with
additional features gated by the more specific compatible), allowing
them to be bound without driver updates. If a backwards incompatible
change is introduced in future SoCs, it will require a new compatible,
such as "apple,pmgr-pwrstate-v2".

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 .../bindings/power/apple,pmgr-pwrstate.yaml   | 117 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml

diff --git a/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
new file mode 100644
index 000000000000..a14bf5f30ff0
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
@@ -0,0 +1,117 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/apple,pmgr-pwrstate.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple SoC PMGR Power States
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+allOf:
+  - $ref: "power-domain.yaml#"
+
+description: |
+  Apple SoCs include a PMGR block responsible for power management,
+  which can control various clocks, resets, power states, and
+  performance features. This binding describes the device power
+  state registers, which control power states and resets.
+
+  Each instance of a power controller within the PMGR syscon node
+  represents a generic power domain provider, as documented in
+  Documentation/devicetree/bindings/power/power-domain.yaml.
+  The provider controls a single SoC block. The power hierarchy is
+  represented via power-domains relationships between these nodes.
+
+  See Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
+  for the top-level PMGR node documentation.
+
+  IP cores belonging to a power domain should contain a
+  "power-domains" property that is a phandle for the
+  power domain node representing the domain.
+
+properties:
+  $nodename:
+    pattern: "^power-controller@[0-9a-f]+$"
+
+  compatible:
+    items:
+      - enum:
+          - apple,t8103-pmgr-pwrstate
+      - const: apple,pmgr-pwrstate
+
+  reg:
+    maxItems: 1
+
+  "#power-domain-cells":
+    const: 0
+
+  "#reset-cells":
+    const: 0
+
+  power-domains:
+    description:
+      Reference to parent power domains. A domain may have multiple parents,
+      and all will be powered up when it is powered.
+
+  apple,domain-name:
+    description: |
+      Specifies the name of the SoC device being controlled. This is used to
+      name the power/reset domains.
+    $ref: /schemas/types.yaml#/definitions/string
+
+  apple,always-on:
+    description: |
+      Forces this power domain to always be powered up.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - "#power-domain-cells"
+  - "#reset-cells"
+  - "apple,domain-name"
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        power-management@23b700000 {
+            compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <0x2 0x3b700000 0x0 0x14000>;
+
+            ps_sio: power-controller@1c0 {
+                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+                reg = <0x1c0>;
+                #power-domain-cells = <0>;
+                #reset-cells = <0>;
+                apple,domain-name = "sio";
+                apple,always-on;
+            };
+
+            ps_uart_p: power-controller@220 {
+                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+                reg = <0x220>;
+                #power-domain-cells = <0>;
+                #reset-cells = <0>;
+                apple,domain-name = "uart_p";
+                power-domains = <&ps_sio>;
+            };
+
+            ps_uart0: power-controller@270 {
+                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+                reg = <0x270>;
+                #power-domain-cells = <0>;
+                #reset-cells = <0>;
+                apple,domain-name = "uart0";
+                power-domains = <&ps_uart_p>;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d25598842d15..5fe53d9a2956 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1722,6 +1722,7 @@ F:	Documentation/devicetree/bindings/arm/apple.yaml
 F:	Documentation/devicetree/bindings/arm/apple/*
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
+F:	Documentation/devicetree/bindings/power/apple*
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/irqchip/irq-apple-aic.c
 F:	include/dt-bindings/interrupt-controller/apple-aic.h
-- 
2.33.0

[PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Hector Martin]

Implements genpd and reset providers for downstream devices. Each
instance of the driver binds to a single register and represents a
single SoC power domain.

The driver does not currently implement all features (auto-pm,
clockgate-only state), but we declare the respective registers for
documentation purposes. These features will be added as they become
useful for downstream devices.

This also creates the apple/soc tree and Kconfig submenu.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 MAINTAINERS                             |   1 +
 drivers/soc/Kconfig                     |   1 +
 drivers/soc/Makefile                    |   1 +
 drivers/soc/apple/Kconfig               |  21 ++
 drivers/soc/apple/Makefile              |   2 +
 drivers/soc/apple/apple-pmgr-pwrstate.c | 281 ++++++++++++++++++++++++
 6 files changed, 307 insertions(+)
 create mode 100644 drivers/soc/apple/Kconfig
 create mode 100644 drivers/soc/apple/Makefile
 create mode 100644 drivers/soc/apple/apple-pmgr-pwrstate.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5fe53d9a2956..def5e05da2bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1725,6 +1725,7 @@ F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
 F:	Documentation/devicetree/bindings/power/apple*
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/irqchip/irq-apple-aic.c
+F:	drivers/soc/apple/*
 F:	include/dt-bindings/interrupt-controller/apple-aic.h
 F:	include/dt-bindings/pinctrl/apple.h
 
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index e8a30c4c5aec..a8562678c437 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -3,6 +3,7 @@ menu "SOC (System On Chip) specific Drivers"
 
 source "drivers/soc/actions/Kconfig"
 source "drivers/soc/amlogic/Kconfig"
+source "drivers/soc/apple/Kconfig"
 source "drivers/soc/aspeed/Kconfig"
 source "drivers/soc/atmel/Kconfig"
 source "drivers/soc/bcm/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index a05e9fbcd3e0..adb30c2d4fea 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -4,6 +4,7 @@
 #
 
 obj-$(CONFIG_ARCH_ACTIONS)	+= actions/
+obj-$(CONFIG_ARCH_APPLE)	+= apple/
 obj-y				+= aspeed/
 obj-$(CONFIG_ARCH_AT91)		+= atmel/
 obj-y				+= bcm/
diff --git a/drivers/soc/apple/Kconfig b/drivers/soc/apple/Kconfig
new file mode 100644
index 000000000000..271092b6aee7
--- /dev/null
+++ b/drivers/soc/apple/Kconfig
@@ -0,0 +1,21 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+if ARCH_APPLE || COMPILE_TEST
+
+menu "Apple SoC drivers"
+
+config APPLE_PMGR_PWRSTATE
+	tristate "Apple SoC PMGR power state control"
+	select REGMAP
+	select MFD_SYSCON
+	select PM_GENERIC_DOMAINS
+	select RESET_CONTROLLER
+	default ARCH_APPLE
+	help
+	  The PMGR block in Apple SoCs provides high-level power state
+	  controls for SoC devices. This driver manages them through the
+	  generic power domain framework, and also provides reset support.
+
+endmenu
+
+endif
diff --git a/drivers/soc/apple/Makefile b/drivers/soc/apple/Makefile
new file mode 100644
index 000000000000..c114e84667e4
--- /dev/null
+++ b/drivers/soc/apple/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_APPLE_PMGR_PWRSTATE)	+= apple-pmgr-pwrstate.o
diff --git a/drivers/soc/apple/apple-pmgr-pwrstate.c b/drivers/soc/apple/apple-pmgr-pwrstate.c
new file mode 100644
index 000000000000..a0338dbb29b8
--- /dev/null
+++ b/drivers/soc/apple/apple-pmgr-pwrstate.c
@@ -0,0 +1,281 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple SoC PMGR device power state driver
+ *
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/reset-controller.h>
+#include <linux/module.h>
+
+#define APPLE_PMGR_RESET        BIT(31)
+#define APPLE_PMGR_AUTO_ENABLE  BIT(28)
+#define APPLE_PMGR_PS_AUTO      GENMASK(27, 24)
+#define APPLE_PMGR_PARENT_OFF   BIT(11)
+#define APPLE_PMGR_DEV_DISABLE  BIT(10)
+#define APPLE_PMGR_WAS_CLKGATED BIT(9)
+#define APPLE_PMGR_WAS_PWRGATED BIT(8)
+#define APPLE_PMGR_PS_ACTUAL    GENMASK(7, 4)
+#define APPLE_PMGR_PS_TARGET    GENMASK(3, 0)
+
+#define APPLE_PMGR_PS_ACTIVE    0xf
+#define APPLE_PMGR_PS_CLKGATE   0x4
+#define APPLE_PMGR_PS_PWRGATE   0x0
+
+#define APPLE_PMGR_PS_SET_TIMEOUT 100
+#define APPLE_PMGR_RESET_TIME 1
+
+struct apple_pmgr_ps {
+	struct device *dev;
+	struct generic_pm_domain genpd;
+	struct reset_controller_dev rcdev;
+	struct regmap *regmap;
+	u32 offset;
+};
+
+#define genpd_to_apple_pmgr_ps(_genpd) container_of(_genpd, struct apple_pmgr_ps, genpd)
+#define rcdev_to_apple_pmgr_ps(_rcdev) container_of(_rcdev, struct apple_pmgr_ps, rcdev)
+
+static int apple_pmgr_ps_set(struct generic_pm_domain *genpd, u32 pstate)
+{
+	int ret;
+	struct apple_pmgr_ps *ps = genpd_to_apple_pmgr_ps(genpd);
+	u32 reg;
+
+	regmap_read(ps->regmap, ps->offset, &reg);
+
+	/* Resets are synchronous, and only work if the device is powered and clocked. */
+	if (reg & APPLE_PMGR_RESET && pstate != APPLE_PMGR_PS_ACTIVE)
+		dev_err(ps->dev, "PS 0x%x: powering off with RESET active\n", ps->offset);
+
+	reg &= ~(APPLE_PMGR_AUTO_ENABLE | APPLE_PMGR_WAS_CLKGATED | APPLE_PMGR_WAS_PWRGATED |
+		 APPLE_PMGR_PS_TARGET);
+	reg |= FIELD_PREP(APPLE_PMGR_PS_TARGET, pstate);
+
+	dev_dbg(ps->dev, "PS 0x%x: pwrstate = 0x%x: 0x%x\n", ps->offset, pstate, reg);
+
+	regmap_write(ps->regmap, ps->offset, reg);
+
+	ret = regmap_read_poll_timeout_atomic(
+		ps->regmap, ps->offset, reg,
+		(FIELD_GET(APPLE_PMGR_PS_ACTUAL, reg) == pstate), 1,
+		APPLE_PMGR_PS_SET_TIMEOUT);
+	if (ret < 0)
+		dev_err(ps->dev, "PS 0x%x: Failed to reach power state 0x%x (now: 0x%x)\n",
+			ps->offset, pstate, reg);
+	return ret;
+}
+
+static bool apple_pmgr_ps_is_active(struct apple_pmgr_ps *ps)
+{
+	u32 reg;
+
+	regmap_read(ps->regmap, ps->offset, &reg);
+	return FIELD_GET(APPLE_PMGR_PS_ACTUAL, reg) == APPLE_PMGR_PS_ACTIVE;
+}
+
+static int apple_pmgr_ps_power_on(struct generic_pm_domain *genpd)
+{
+	return apple_pmgr_ps_set(genpd, APPLE_PMGR_PS_ACTIVE);
+}
+
+static int apple_pmgr_ps_power_off(struct generic_pm_domain *genpd)
+{
+	return apple_pmgr_ps_set(genpd, APPLE_PMGR_PS_PWRGATE);
+}
+
+static int apple_pmgr_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
+
+	mutex_lock(&ps->genpd.mlock);
+
+	if (ps->genpd.status == GENPD_STATE_OFF)
+		dev_err(ps->dev, "PS 0x%x: asserting RESET while powered down\n", ps->offset);
+
+	dev_dbg(ps->dev, "PS 0x%x: assert reset\n", ps->offset);
+	/* Quiesce device before asserting reset */
+	regmap_set_bits(ps->regmap, ps->offset, APPLE_PMGR_DEV_DISABLE);
+	regmap_set_bits(ps->regmap, ps->offset, APPLE_PMGR_RESET);
+
+	mutex_unlock(&ps->genpd.mlock);
+
+	return 0;
+}
+
+static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
+
+	mutex_lock(&ps->genpd.mlock);
+
+	dev_dbg(ps->dev, "PS 0x%x: deassert reset\n", ps->offset);
+	regmap_clear_bits(ps->regmap, ps->offset, APPLE_PMGR_RESET);
+	regmap_clear_bits(ps->regmap, ps->offset, APPLE_PMGR_DEV_DISABLE);
+
+	if (ps->genpd.status == GENPD_STATE_OFF)
+		dev_err(ps->dev, "PS 0x%x: RESET was deasserted while powered down\n", ps->offset);
+
+	mutex_unlock(&ps->genpd.mlock);
+
+	return 0;
+}
+
+static int apple_pmgr_reset_reset(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	int ret;
+
+	ret = apple_pmgr_reset_assert(rcdev, id);
+	if (ret)
+		return ret;
+
+	usleep_range(APPLE_PMGR_RESET_TIME, 2 * APPLE_PMGR_RESET_TIME);
+
+	return apple_pmgr_reset_deassert(rcdev, id);
+}
+
+static int apple_pmgr_reset_status(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
+	u32 reg;
+
+	regmap_read(ps->regmap, ps->offset, &reg);
+
+	return !!(reg & APPLE_PMGR_RESET);
+}
+
+const struct reset_control_ops apple_pmgr_reset_ops = {
+	.assert		= apple_pmgr_reset_assert,
+	.deassert	= apple_pmgr_reset_deassert,
+	.reset		= apple_pmgr_reset_reset,
+	.status		= apple_pmgr_reset_status,
+};
+
+static int apple_pmgr_reset_xlate(struct reset_controller_dev *rcdev,
+				  const struct of_phandle_args *reset_spec)
+{
+	return 0;
+}
+
+static int apple_pmgr_ps_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct apple_pmgr_ps *ps;
+	struct regmap *regmap;
+	struct of_phandle_iterator it;
+	int ret;
+	const char *name;
+
+	regmap = syscon_node_to_regmap(node->parent);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	ps = devm_kzalloc(dev, sizeof(*ps), GFP_KERNEL);
+	if (!ps)
+		return -ENOMEM;
+
+	ps->dev = dev;
+	ps->regmap = regmap;
+
+	ret = of_property_read_string(node, "apple,domain-name", &name);
+	if (ret < 0) {
+		dev_err(dev, "missing apple,domain-name property\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(node, "reg", &ps->offset);
+	if (ret < 0) {
+		dev_err(dev, "missing reg property\n");
+		return ret;
+	}
+
+	if (of_property_read_bool(node, "apple,always-on"))
+		ps->genpd.flags |= GENPD_FLAG_ALWAYS_ON;
+
+	ps->genpd.name = name;
+	ps->genpd.power_on = apple_pmgr_ps_power_on;
+	ps->genpd.power_off = apple_pmgr_ps_power_off;
+
+	ret = pm_genpd_init(&ps->genpd, NULL, !apple_pmgr_ps_is_active(ps));
+	if (ret < 0) {
+		dev_err(dev, "pm_genpd_init failed\n");
+		return ret;
+	}
+
+	ret = of_genpd_add_provider_simple(node, &ps->genpd);
+	if (ret < 0) {
+		dev_err(dev, "of_genpd_add_provider_simple failed\n");
+		return ret;
+	}
+
+	of_for_each_phandle(&it, ret, node, "power-domains", "#power-domain-cells", -1) {
+		struct of_phandle_args parent, child;
+
+		parent.np = it.node;
+		parent.args_count = of_phandle_iterator_args(&it, parent.args, MAX_PHANDLE_ARGS);
+		child.np = node;
+		child.args_count = 0;
+		ret = of_genpd_add_subdomain(&parent, &child);
+
+		if (ret == -EPROBE_DEFER) {
+			of_node_put(parent.np);
+			goto err_remove;
+		} else if (ret < 0) {
+			dev_err(dev, "failed to add to parent domain: %d (%s -> %s)\n",
+				ret, it.node->name, node->name);
+			of_node_put(parent.np);
+			goto err_remove;
+		}
+	}
+
+	pm_genpd_remove_device(dev);
+
+	ps->rcdev.owner = THIS_MODULE;
+	ps->rcdev.nr_resets = 1;
+	ps->rcdev.ops = &apple_pmgr_reset_ops;
+	ps->rcdev.of_node = dev->of_node;
+	ps->rcdev.of_reset_n_cells = 0;
+	ps->rcdev.of_xlate = apple_pmgr_reset_xlate;
+
+	ret = devm_reset_controller_register(dev, &ps->rcdev);
+	if (ret < 0)
+		goto err_remove;
+
+	return 0;
+err_remove:
+	of_genpd_del_provider(node);
+	pm_genpd_remove(&ps->genpd);
+	return ret;
+}
+
+static const struct of_device_id apple_pmgr_ps_of_match[] = {
+	{ .compatible = "apple,t8103-pmgr-pwrstate" },
+	{ .compatible = "apple,pmgr-pwrstate" },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, apple_pmgr_ps_of_match);
+
+static struct platform_driver apple_pmgr_ps_driver = {
+	.probe = apple_pmgr_ps_probe,
+	.driver = {
+		.name = "apple-pmgr-pwrstate",
+		.of_match_table = apple_pmgr_ps_of_match,
+	},
+};
+
+MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
+MODULE_DESCRIPTION("PMGR power state driver for Apple SoCs");
+MODULE_LICENSE("GPL v2");
+
+module_platform_driver(apple_pmgr_ps_driver);
-- 
2.33.0

[PATCH 4/7] arm64: dts: apple: t8103: Rename clk24 to clkref

[Hector Martin]

We now know that this frequency comes from the external reference
oscillator and is used for various SoC blocks, and isn't just a random
24MHz clock, so let's call it something more appropriate.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 arch/arm64/boot/dts/apple/t8103.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index a1e22a2ea2e5..9f60f9e48ea0 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -95,11 +95,11 @@ timer {
 			     <AIC_FIQ AIC_TMR_HV_VIRT IRQ_TYPE_LEVEL_HIGH>;
 	};
 
-	clk24: clock-24m {
+	clkref: clock-ref {
 		compatible = "fixed-clock";
 		#clock-cells = <0>;
 		clock-frequency = <24000000>;
-		clock-output-names = "clk24";
+		clock-output-names = "clkref";
 	};
 
 	soc {
@@ -120,7 +120,7 @@ serial0: serial@235200000 {
 			 * TODO: figure out the clocking properly, there may
 			 * be a third selectable clock.
 			 */
-			clocks = <&clk24>, <&clk24>;
+			clocks = <&clkref>, <&clkref>;
 			clock-names = "uart", "clk_uart_baud0";
 			status = "disabled";
 		};
-- 
2.33.0

[PATCH 5/7] arm64: dts: apple: t8103: Add the UART PMGR tree

[Hector Martin]

Note that the UART driver does not currently support runtime-pm, so this
effectively always keeps the UART0 device on. However, this does clockgate
all the other UARTs, as those are not currently instantiated.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 arch/arm64/boot/dts/apple/t8103.dtsi | 116 +++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index 9f60f9e48ea0..63056ddc7ef7 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -122,6 +122,7 @@ serial0: serial@235200000 {
 			 */
 			clocks = <&clkref>, <&clkref>;
 			clock-names = "uart", "clk_uart_baud0";
+			power-domains = <&ps_uart0>;
 			status = "disabled";
 		};
 
@@ -131,5 +132,120 @@ aic: interrupt-controller@23b100000 {
 			interrupt-controller;
 			reg = <0x2 0x3b100000 0x0 0x8000>;
 		};
+
+		pmgr: power-management@23b700000 {
+			compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			reg = <0x2 0x3b700000 0x0 0x14000>;
+
+			ps_sio_busif: power-controller@1c0 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x1c0>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "sio_busif";
+			};
+
+			ps_sio: power-controller@1c8 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x1c8>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "sio";
+				power-domains = <&ps_sio_busif>;
+			};
+
+			ps_uart_p: power-controller@220 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x220>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "uart_p";
+				power-domains = <&ps_sio>;
+			};
+
+			ps_uart0: power-controller@270 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x270>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "uart0";
+				power-domains = <&ps_uart_p>;
+			};
+
+			ps_uart1: power-controller@278 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x278>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "uart1";
+				power-domains = <&ps_uart_p>;
+			};
+
+			ps_uart2: power-controller@280 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x280>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "uart2";
+				power-domains = <&ps_uart_p>;
+			};
+
+			ps_uart3: power-controller@288 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x288>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "uart3";
+				power-domains = <&ps_uart_p>;
+			};
+
+			ps_uart4: power-controller@290 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x290>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "uart4";
+				power-domains = <&ps_uart_p>;
+			};
+
+			ps_uart5: power-controller@298 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x298>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "uart5";
+				power-domains = <&ps_uart_p>;
+			};
+
+			ps_uart6: power-controller@2a0 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x2a0>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "uart6";
+				power-domains = <&ps_uart_p>;
+			};
+
+			ps_uart7: power-controller@2a8 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x2a8>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "uart7";
+				power-domains = <&ps_uart_p>;
+			};
+
+			ps_uart8: power-controller@2b0 {
+				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+				reg = <0x2b0>;
+				#power-domain-cells = <0>;
+				#reset-cells = <0>;
+				apple,domain-name = "uart8";
+				power-domains = <&ps_uart_p>;
+			};
+		};
 	};
 };
-- 
2.33.0

[PATCH 6/7] tty: serial: samsung_tty: Support runtime PM

[Hector Martin]

This allows idle UART devices to be suspended using the standard
runtime-PM framework. The logic is modeled after stm32-usart.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/tty/serial/samsung_tty.c | 88 ++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 34 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index e2f49863e9c2..d68e3341adc6 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -40,6 +40,7 @@
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 #include <asm/irq.h>
 
 /* UART name and device definitions */
@@ -1381,31 +1382,49 @@ static void exynos_usi_init(struct uart_port *port)
 
 /* power power management control */
 
-static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
-			      unsigned int old)
+static int __maybe_unused s3c24xx_serial_runtime_suspend(struct device *dev)
 {
+	struct uart_port *port = dev_get_drvdata(dev);
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
 	int timeout = 10000;
 
-	ourport->pm_level = level;
+	while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
+		udelay(100);
 
-	switch (level) {
-	case 3:
-		while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
-			udelay(100);
+	if (!IS_ERR(ourport->baudclk))
+		clk_disable_unprepare(ourport->baudclk);
 
-		if (!IS_ERR(ourport->baudclk))
-			clk_disable_unprepare(ourport->baudclk);
+	clk_disable_unprepare(ourport->clk);
+	return 0;
+};
 
-		clk_disable_unprepare(ourport->clk);
-		break;
+static int __maybe_unused s3c24xx_serial_runtime_resume(struct device *dev)
+{
+	struct uart_port *port = dev_get_drvdata(dev);
+	struct s3c24xx_uart_port *ourport = to_ourport(port);
 
-	case 0:
-		clk_prepare_enable(ourport->clk);
+	clk_prepare_enable(ourport->clk);
 
-		if (!IS_ERR(ourport->baudclk))
-			clk_prepare_enable(ourport->baudclk);
+	if (!IS_ERR(ourport->baudclk))
+		clk_prepare_enable(ourport->baudclk);
+	return 0;
+};
+
+static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
+			      unsigned int old)
+{
+	struct s3c24xx_uart_port *ourport = to_ourport(port);
+
+	ourport->pm_level = level;
 
+	switch (level) {
+	case UART_PM_STATE_OFF:
+		pm_runtime_mark_last_busy(port->dev);
+		pm_runtime_put_sync(port->dev);
+		break;
+
+	case UART_PM_STATE_ON:
+		pm_runtime_get_sync(port->dev);
 		exynos_usi_init(port);
 		break;
 	default:
@@ -2282,18 +2301,15 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 		}
 	}
 
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	dev_dbg(&pdev->dev, "%s: adding port\n", __func__);
 	uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
 	platform_set_drvdata(pdev, &ourport->port);
 
-	/*
-	 * Deactivate the clock enabled in s3c24xx_serial_init_port here,
-	 * so that a potential re-enablement through the pm-callback overlaps
-	 * and keeps the clock enabled in this case.
-	 */
-	clk_disable_unprepare(ourport->clk);
-	if (!IS_ERR(ourport->baudclk))
-		clk_disable_unprepare(ourport->baudclk);
+	pm_runtime_put_sync(&pdev->dev);
 
 	ret = s3c24xx_serial_cpufreq_register(ourport);
 	if (ret < 0)
@@ -2309,8 +2325,14 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
 	struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);
 
 	if (port) {
+		pm_runtime_get_sync(&dev->dev);
+
 		s3c24xx_serial_cpufreq_deregister(to_ourport(port));
 		uart_remove_one_port(&s3c24xx_uart_drv, port);
+
+		pm_runtime_disable(&dev->dev);
+		pm_runtime_set_suspended(&dev->dev);
+		pm_runtime_put_noidle(&dev->dev);
 	}
 
 	uart_unregister_driver(&s3c24xx_uart_drv);
@@ -2319,8 +2341,8 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
 }
 
 /* UART power management code */
-#ifdef CONFIG_PM_SLEEP
-static int s3c24xx_serial_suspend(struct device *dev)
+
+static int __maybe_unused s3c24xx_serial_suspend(struct device *dev)
 {
 	struct uart_port *port = s3c24xx_dev_to_port(dev);
 
@@ -2330,7 +2352,7 @@ static int s3c24xx_serial_suspend(struct device *dev)
 	return 0;
 }
 
-static int s3c24xx_serial_resume(struct device *dev)
+static int __maybe_unused s3c24xx_serial_resume(struct device *dev)
 {
 	struct uart_port *port = s3c24xx_dev_to_port(dev);
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
@@ -2350,7 +2372,7 @@ static int s3c24xx_serial_resume(struct device *dev)
 	return 0;
 }
 
-static int s3c24xx_serial_resume_noirq(struct device *dev)
+static int __maybe_unused s3c24xx_serial_resume_noirq(struct device *dev)
 {
 	struct uart_port *port = s3c24xx_dev_to_port(dev);
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
@@ -2420,16 +2442,14 @@ static int s3c24xx_serial_resume_noirq(struct device *dev)
 }
 
 static const struct dev_pm_ops s3c24xx_serial_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
 	.suspend = s3c24xx_serial_suspend,
 	.resume = s3c24xx_serial_resume,
 	.resume_noirq = s3c24xx_serial_resume_noirq,
+#endif
+	SET_RUNTIME_PM_OPS(s3c24xx_serial_runtime_suspend,
+			   s3c24xx_serial_runtime_resume, NULL)
 };
-#define SERIAL_SAMSUNG_PM_OPS	(&s3c24xx_serial_pm_ops)
-
-#else /* !CONFIG_PM_SLEEP */
-
-#define SERIAL_SAMSUNG_PM_OPS	NULL
-#endif /* CONFIG_PM_SLEEP */
 
 /* Console code */
 
@@ -2924,7 +2944,7 @@ static struct platform_driver samsung_serial_driver = {
 	.id_table	= s3c24xx_serial_driver_ids,
 	.driver		= {
 		.name	= "samsung-uart",
-		.pm	= SERIAL_SAMSUNG_PM_OPS,
+		.pm	= &s3c24xx_serial_pm_ops,
 		.of_match_table	= of_match_ptr(s3c24xx_uart_dt_match),
 	},
 };
-- 
2.33.0

[PATCH 7/7] arm64: dts: apple: t8103: Add UART2

[Hector Martin]

This UART is connected to the debug port of the WLAN module. It is
mostly useless, but makes for a good test case for runtime-pm without
having to unbind the console from the main system UART.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 arch/arm64/boot/dts/apple/t8103-j274.dts |  5 +++++
 arch/arm64/boot/dts/apple/t8103.dtsi     | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/apple/t8103-j274.dts b/arch/arm64/boot/dts/apple/t8103-j274.dts
index e0f6775b9878..16c5eb7f53b1 100644
--- a/arch/arm64/boot/dts/apple/t8103-j274.dts
+++ b/arch/arm64/boot/dts/apple/t8103-j274.dts
@@ -17,6 +17,7 @@ / {
 
 	aliases {
 		serial0 = &serial0;
+		serial2 = &serial2;
 	};
 
 	chosen {
@@ -43,3 +44,7 @@ memory@800000000 {
 &serial0 {
 	status = "okay";
 };
+
+&serial2 {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index 63056ddc7ef7..844ed7bd0451 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -126,6 +126,18 @@ serial0: serial@235200000 {
 			status = "disabled";
 		};
 
+		serial2: serial@235208000 {
+			compatible = "apple,s5l-uart";
+			reg = <0x2 0x35208000 0x0 0x1000>;
+			reg-io-width = <4>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 607 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clkref>, <&clkref>;
+			clock-names = "uart", "clk_uart_baud0";
+			power-domains = <&ps_uart2>;
+			status = "disabled";
+		};
+
 		aic: interrupt-controller@23b100000 {
 			compatible = "apple,t8103-aic", "apple,aic";
 			#interrupt-cells = <3>;
-- 
2.33.0

Re: [PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Linus Walleij]

Hi Hector,

On Tue, Oct 5, 2021 at 6:00 PM Hector Martin <marcan@marcan.st> wrote:

>  drivers/soc/apple/Kconfig               |  21 ++
>  drivers/soc/apple/Makefile              |   2 +
>  drivers/soc/apple/apple-pmgr-pwrstate.c | 281 ++++++++++++++++++++++++

This is traditionally where we put the ARM SoC drivers, but
Mac has traditionally used drivers/macintosh for their custom
board etc stuff. Or is that just for any off-chip stuff?

I suppose it doesn't matter much (unless there is code under
drivers/macintosh we want to reuse for M1), but it could be a bit
confusing?

Yours,
Linus Walleij

Re: [PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Hector Martin]

Hi Linus,

On 06/10/2021 01.08, Linus Walleij wrote:
> Hi Hector,
> 
> On Tue, Oct 5, 2021 at 6:00 PM Hector Martin <marcan@marcan.st> wrote:
> 
>>   drivers/soc/apple/Kconfig               |  21 ++
>>   drivers/soc/apple/Makefile              |   2 +
>>   drivers/soc/apple/apple-pmgr-pwrstate.c | 281 ++++++++++++++++++++++++
> 
> This is traditionally where we put the ARM SoC drivers, but
> Mac has traditionally used drivers/macintosh for their custom
> board etc stuff. Or is that just for any off-chip stuff?
> 
> I suppose it doesn't matter much (unless there is code under
> drivers/macintosh we want to reuse for M1), but it could be a bit
> confusing?

Hmm, it seems that tree is mostly about the PowerPC era Macs; the only 
thing enabled for x86 there is MAC_EMUMOUSEBTN. There is also 
platform/x86/apple-gmux.c for an x86 Mac specific thing...

We already broke tradition with the "apple," DT compatible prefix (used 
to be AAPL for the PowerPC Macs), and these chips aren't even just used 
in Macs (e.g. the iPad, which in theory people would be able to run 
Linux on if someone figures out a jailbreak), so perhaps it's time for 
another break here?

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Linus Walleij]

On Tue, Oct 5, 2021 at 6:15 PM Hector Martin <marcan@marcan.st> wrote:

> We already broke tradition with the "apple," DT compatible prefix (used
> to be AAPL for the PowerPC Macs), and these chips aren't even just used
> in Macs (e.g. the iPad, which in theory people would be able to run
> Linux on if someone figures out a jailbreak), so perhaps it's time for
> another break here?

Yeah fair enough. It's probably more intuitive under drivers/soc anyway.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 1/7] dt-bindings: arm: apple: Add apple,pmgr binding

[Mark Kettenis]

> From: Hector Martin <marcan@marcan.st>
> Date: Wed,  6 Oct 2021 00:59:17 +0900
> 
> The PMGR block in Apple Silicon SoCs is responsible for SoC power
> management. There are two PMGRs in T8103, with different register
> layouts but compatible registers. In order to support this as well
> as future SoC generations with backwards-compatible registers, we
> declare these blocks as syscons and bind to individual registers
> in child nodes. Each register controls one SoC device.
> 
> The respective apple compatibles are defined in case device-specific
> quirks are necessary in the future, but currently these nodes are
> expected to be bound by the generic syscon driver.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  .../bindings/arm/apple/apple,pmgr.yaml        | 74 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml

This works for U-Boot and OpenBSD.

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>.

> diff --git a/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> new file mode 100644
> index 000000000000..0304164e4140
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/apple/apple,pmgr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SoC Power Manager (PMGR)
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +description: |
> +  Apple SoCs include a PMGR block responsible for power management,
> +  which can control various clocks, resets, power states, and
> +  performance features. This node represents the PMGR as a syscon,
> +  with sub-nodes representing individual features.
> +
> +  Apple SoCs may have a secondary "mini-PMGR"; it is represented
> +  separately in the device tree, but works the same way.
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - apple,t8103-pmgr
> +          - apple,t8103-minipmgr
> +          - apple,pmgr
> +
> +  required:
> +    - compatible
> +
> +properties:
> +  $nodename:
> +    pattern: "^power-management@[0-9a-f]+$"
> +
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t8103-pmgr
> +          - apple,t8103-minipmgr
> +      - const: apple,pmgr
> +      - const: syscon
> +      - const: simple-mfd
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        power-management@23b700000 {
> +            compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0x2 0x3b700000 0x0 0x14000>;
> +        };
> +
> +        power-management@23b700000 {
> +            compatible = "apple,t8103-minipmgr", "apple,pmgr", "syscon", "simple-mfd";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0x2 0x3d280000 0x0 0xc000>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index abdcbcfef73d..d25598842d15 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1719,6 +1719,7 @@ B:	https://github.com/AsahiLinux/linux/issues
>  C:	irc://irc.oftc.net/asahi-dev
>  T:	git https://github.com/AsahiLinux/linux.git
>  F:	Documentation/devicetree/bindings/arm/apple.yaml
> +F:	Documentation/devicetree/bindings/arm/apple/*
>  F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
>  F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
>  F:	arch/arm64/boot/dts/apple/
> -- 
> 2.33.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Mark Kettenis]

> From: Hector Martin <marcan@marcan.st>
> Date: Wed,  6 Oct 2021 00:59:18 +0900
> 
> This syscon child node represents a single SoC device controlled by the
> PMGR block. This layout allows us to declare all device power state
> controls (power/clock gating and reset) in the device tree, including
> dependencies, instead of hardcoding it into the driver. The register
> layout is uniform.
> 
> Each pmgr-pwrstate node provides genpd and reset features, to be
> consumed by downstream device nodes.
> 
> Future SoCs are expected to use backwards compatible registers, and the
> "apple,pmgr-pwrstate" represents any such interfaces (possibly with
> additional features gated by the more specific compatible), allowing
> them to be bound without driver updates. If a backwards incompatible
> change is introduced in future SoCs, it will require a new compatible,
> such as "apple,pmgr-pwrstate-v2".

Or we drop the apple,mpgr-pwrstate and go with only SoC-specific
compatibles from that point onwards.

> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  .../bindings/power/apple,pmgr-pwrstate.yaml   | 117 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml

This works for U-Boot.  Didn't write an OpenBSD driver yet but it
should work there as well.

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>


> diff --git a/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> new file mode 100644
> index 000000000000..a14bf5f30ff0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/apple,pmgr-pwrstate.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SoC PMGR Power States
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +allOf:
> +  - $ref: "power-domain.yaml#"
> +
> +description: |
> +  Apple SoCs include a PMGR block responsible for power management,
> +  which can control various clocks, resets, power states, and
> +  performance features. This binding describes the device power
> +  state registers, which control power states and resets.
> +
> +  Each instance of a power controller within the PMGR syscon node
> +  represents a generic power domain provider, as documented in
> +  Documentation/devicetree/bindings/power/power-domain.yaml.
> +  The provider controls a single SoC block. The power hierarchy is
> +  represented via power-domains relationships between these nodes.
> +
> +  See Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> +  for the top-level PMGR node documentation.
> +
> +  IP cores belonging to a power domain should contain a
> +  "power-domains" property that is a phandle for the
> +  power domain node representing the domain.
> +
> +properties:
> +  $nodename:
> +    pattern: "^power-controller@[0-9a-f]+$"
> +
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t8103-pmgr-pwrstate
> +      - const: apple,pmgr-pwrstate
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#power-domain-cells":
> +    const: 0
> +
> +  "#reset-cells":
> +    const: 0
> +
> +  power-domains:
> +    description:
> +      Reference to parent power domains. A domain may have multiple parents,
> +      and all will be powered up when it is powered.
> +
> +  apple,domain-name:
> +    description: |
> +      Specifies the name of the SoC device being controlled. This is used to
> +      name the power/reset domains.
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  apple,always-on:
> +    description: |
> +      Forces this power domain to always be powered up.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#power-domain-cells"
> +  - "#reset-cells"
> +  - "apple,domain-name"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        power-management@23b700000 {
> +            compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0x2 0x3b700000 0x0 0x14000>;
> +
> +            ps_sio: power-controller@1c0 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x1c0>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "sio";
> +                apple,always-on;
> +            };
> +
> +            ps_uart_p: power-controller@220 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x220>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "uart_p";
> +                power-domains = <&ps_sio>;
> +            };
> +
> +            ps_uart0: power-controller@270 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x270>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "uart0";
> +                power-domains = <&ps_uart_p>;
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d25598842d15..5fe53d9a2956 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1722,6 +1722,7 @@ F:	Documentation/devicetree/bindings/arm/apple.yaml
>  F:	Documentation/devicetree/bindings/arm/apple/*
>  F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
>  F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
> +F:	Documentation/devicetree/bindings/power/apple*
>  F:	arch/arm64/boot/dts/apple/
>  F:	drivers/irqchip/irq-apple-aic.c
>  F:	include/dt-bindings/interrupt-controller/apple-aic.h
> -- 
> 2.33.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Mark Kettenis]

> From: Hector Martin <marcan@marcan.st>
> Date: Wed,  6 Oct 2021 00:59:19 +0900
> 
> Implements genpd and reset providers for downstream devices. Each
> instance of the driver binds to a single register and represents a
> single SoC power domain.
> 
> The driver does not currently implement all features (auto-pm,
> clockgate-only state), but we declare the respective registers for
> documentation purposes. These features will be added as they become
> useful for downstream devices.
> 
> This also creates the apple/soc tree and Kconfig submenu.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  MAINTAINERS                             |   1 +
>  drivers/soc/Kconfig                     |   1 +
>  drivers/soc/Makefile                    |   1 +
>  drivers/soc/apple/Kconfig               |  21 ++
>  drivers/soc/apple/Makefile              |   2 +
>  drivers/soc/apple/apple-pmgr-pwrstate.c | 281 ++++++++++++++++++++++++
>  6 files changed, 307 insertions(+)
>  create mode 100644 drivers/soc/apple/Kconfig
>  create mode 100644 drivers/soc/apple/Makefile
>  create mode 100644 drivers/soc/apple/apple-pmgr-pwrstate.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5fe53d9a2956..def5e05da2bc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1725,6 +1725,7 @@ F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
>  F:	Documentation/devicetree/bindings/power/apple*
>  F:	arch/arm64/boot/dts/apple/
>  F:	drivers/irqchip/irq-apple-aic.c
> +F:	drivers/soc/apple/*
>  F:	include/dt-bindings/interrupt-controller/apple-aic.h
>  F:	include/dt-bindings/pinctrl/apple.h
>  
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index e8a30c4c5aec..a8562678c437 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -3,6 +3,7 @@ menu "SOC (System On Chip) specific Drivers"
>  
>  source "drivers/soc/actions/Kconfig"
>  source "drivers/soc/amlogic/Kconfig"
> +source "drivers/soc/apple/Kconfig"
>  source "drivers/soc/aspeed/Kconfig"
>  source "drivers/soc/atmel/Kconfig"
>  source "drivers/soc/bcm/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index a05e9fbcd3e0..adb30c2d4fea 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -4,6 +4,7 @@
>  #
>  
>  obj-$(CONFIG_ARCH_ACTIONS)	+= actions/
> +obj-$(CONFIG_ARCH_APPLE)	+= apple/
>  obj-y				+= aspeed/
>  obj-$(CONFIG_ARCH_AT91)		+= atmel/
>  obj-y				+= bcm/
> diff --git a/drivers/soc/apple/Kconfig b/drivers/soc/apple/Kconfig
> new file mode 100644
> index 000000000000..271092b6aee7
> --- /dev/null
> +++ b/drivers/soc/apple/Kconfig
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +if ARCH_APPLE || COMPILE_TEST
> +
> +menu "Apple SoC drivers"
> +
> +config APPLE_PMGR_PWRSTATE
> +	tristate "Apple SoC PMGR power state control"
> +	select REGMAP
> +	select MFD_SYSCON
> +	select PM_GENERIC_DOMAINS
> +	select RESET_CONTROLLER
> +	default ARCH_APPLE
> +	help
> +	  The PMGR block in Apple SoCs provides high-level power state
> +	  controls for SoC devices. This driver manages them through the
> +	  generic power domain framework, and also provides reset support.
> +
> +endmenu
> +
> +endif
> diff --git a/drivers/soc/apple/Makefile b/drivers/soc/apple/Makefile
> new file mode 100644
> index 000000000000..c114e84667e4
> --- /dev/null
> +++ b/drivers/soc/apple/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_APPLE_PMGR_PWRSTATE)	+= apple-pmgr-pwrstate.o
> diff --git a/drivers/soc/apple/apple-pmgr-pwrstate.c b/drivers/soc/apple/apple-pmgr-pwrstate.c
> new file mode 100644
> index 000000000000..a0338dbb29b8
> --- /dev/null
> +++ b/drivers/soc/apple/apple-pmgr-pwrstate.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SoC PMGR device power state driver
> + *
> + * Copyright The Asahi Linux Contributors
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/bitfield.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/reset-controller.h>
> +#include <linux/module.h>
> +
> +#define APPLE_PMGR_RESET        BIT(31)
> +#define APPLE_PMGR_AUTO_ENABLE  BIT(28)
> +#define APPLE_PMGR_PS_AUTO      GENMASK(27, 24)
> +#define APPLE_PMGR_PARENT_OFF   BIT(11)
> +#define APPLE_PMGR_DEV_DISABLE  BIT(10)
> +#define APPLE_PMGR_WAS_CLKGATED BIT(9)
> +#define APPLE_PMGR_WAS_PWRGATED BIT(8)
> +#define APPLE_PMGR_PS_ACTUAL    GENMASK(7, 4)
> +#define APPLE_PMGR_PS_TARGET    GENMASK(3, 0)
> +
> +#define APPLE_PMGR_PS_ACTIVE    0xf
> +#define APPLE_PMGR_PS_CLKGATE   0x4
> +#define APPLE_PMGR_PS_PWRGATE   0x0
> +
> +#define APPLE_PMGR_PS_SET_TIMEOUT 100
> +#define APPLE_PMGR_RESET_TIME 1
> +
> +struct apple_pmgr_ps {
> +	struct device *dev;
> +	struct generic_pm_domain genpd;
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +	u32 offset;
> +};
> +
> +#define genpd_to_apple_pmgr_ps(_genpd) container_of(_genpd, struct apple_pmgr_ps, genpd)
> +#define rcdev_to_apple_pmgr_ps(_rcdev) container_of(_rcdev, struct apple_pmgr_ps, rcdev)
> +
> +static int apple_pmgr_ps_set(struct generic_pm_domain *genpd, u32 pstate)
> +{
> +	int ret;
> +	struct apple_pmgr_ps *ps = genpd_to_apple_pmgr_ps(genpd);
> +	u32 reg;
> +
> +	regmap_read(ps->regmap, ps->offset, &reg);
> +
> +	/* Resets are synchronous, and only work if the device is powered and clocked. */
> +	if (reg & APPLE_PMGR_RESET && pstate != APPLE_PMGR_PS_ACTIVE)
> +		dev_err(ps->dev, "PS 0x%x: powering off with RESET active\n", ps->offset);
> +
> +	reg &= ~(APPLE_PMGR_AUTO_ENABLE | APPLE_PMGR_WAS_CLKGATED | APPLE_PMGR_WAS_PWRGATED |
> +		 APPLE_PMGR_PS_TARGET);
> +	reg |= FIELD_PREP(APPLE_PMGR_PS_TARGET, pstate);
> +
> +	dev_dbg(ps->dev, "PS 0x%x: pwrstate = 0x%x: 0x%x\n", ps->offset, pstate, reg);
> +
> +	regmap_write(ps->regmap, ps->offset, reg);
> +
> +	ret = regmap_read_poll_timeout_atomic(
> +		ps->regmap, ps->offset, reg,
> +		(FIELD_GET(APPLE_PMGR_PS_ACTUAL, reg) == pstate), 1,
> +		APPLE_PMGR_PS_SET_TIMEOUT);
> +	if (ret < 0)
> +		dev_err(ps->dev, "PS 0x%x: Failed to reach power state 0x%x (now: 0x%x)\n",
> +			ps->offset, pstate, reg);
> +	return ret;
> +}
> +
> +static bool apple_pmgr_ps_is_active(struct apple_pmgr_ps *ps)
> +{
> +	u32 reg;
> +
> +	regmap_read(ps->regmap, ps->offset, &reg);
> +	return FIELD_GET(APPLE_PMGR_PS_ACTUAL, reg) == APPLE_PMGR_PS_ACTIVE;
> +}
> +
> +static int apple_pmgr_ps_power_on(struct generic_pm_domain *genpd)
> +{
> +	return apple_pmgr_ps_set(genpd, APPLE_PMGR_PS_ACTIVE);
> +}
> +
> +static int apple_pmgr_ps_power_off(struct generic_pm_domain *genpd)
> +{
> +	return apple_pmgr_ps_set(genpd, APPLE_PMGR_PS_PWRGATE);
> +}
> +
> +static int apple_pmgr_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
> +
> +	mutex_lock(&ps->genpd.mlock);
> +
> +	if (ps->genpd.status == GENPD_STATE_OFF)
> +		dev_err(ps->dev, "PS 0x%x: asserting RESET while powered down\n", ps->offset);
> +
> +	dev_dbg(ps->dev, "PS 0x%x: assert reset\n", ps->offset);
> +	/* Quiesce device before asserting reset */
> +	regmap_set_bits(ps->regmap, ps->offset, APPLE_PMGR_DEV_DISABLE);
> +	regmap_set_bits(ps->regmap, ps->offset, APPLE_PMGR_RESET);
> +
> +	mutex_unlock(&ps->genpd.mlock);
> +
> +	return 0;
> +}
> +
> +static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
> +
> +	mutex_lock(&ps->genpd.mlock);
> +
> +	dev_dbg(ps->dev, "PS 0x%x: deassert reset\n", ps->offset);
> +	regmap_clear_bits(ps->regmap, ps->offset, APPLE_PMGR_RESET);
> +	regmap_clear_bits(ps->regmap, ps->offset, APPLE_PMGR_DEV_DISABLE);
> +
> +	if (ps->genpd.status == GENPD_STATE_OFF)
> +		dev_err(ps->dev, "PS 0x%x: RESET was deasserted while powered down\n", ps->offset);
> +
> +	mutex_unlock(&ps->genpd.mlock);
> +
> +	return 0;
> +}
> +
> +static int apple_pmgr_reset_reset(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	int ret;
> +
> +	ret = apple_pmgr_reset_assert(rcdev, id);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(APPLE_PMGR_RESET_TIME, 2 * APPLE_PMGR_RESET_TIME);
> +
> +	return apple_pmgr_reset_deassert(rcdev, id);
> +}
> +
> +static int apple_pmgr_reset_status(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
> +	u32 reg;
> +
> +	regmap_read(ps->regmap, ps->offset, &reg);
> +
> +	return !!(reg & APPLE_PMGR_RESET);
> +}
> +
> +const struct reset_control_ops apple_pmgr_reset_ops = {
> +	.assert		= apple_pmgr_reset_assert,
> +	.deassert	= apple_pmgr_reset_deassert,
> +	.reset		= apple_pmgr_reset_reset,
> +	.status		= apple_pmgr_reset_status,
> +};
> +
> +static int apple_pmgr_reset_xlate(struct reset_controller_dev *rcdev,
> +				  const struct of_phandle_args *reset_spec)
> +{
> +	return 0;
> +}
> +
> +static int apple_pmgr_ps_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct apple_pmgr_ps *ps;
> +	struct regmap *regmap;
> +	struct of_phandle_iterator it;
> +	int ret;
> +	const char *name;
> +
> +	regmap = syscon_node_to_regmap(node->parent);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	ps = devm_kzalloc(dev, sizeof(*ps), GFP_KERNEL);
> +	if (!ps)
> +		return -ENOMEM;
> +
> +	ps->dev = dev;
> +	ps->regmap = regmap;
> +
> +	ret = of_property_read_string(node, "apple,domain-name", &name);
> +	if (ret < 0) {
> +		dev_err(dev, "missing apple,domain-name property\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "reg", &ps->offset);
> +	if (ret < 0) {
> +		dev_err(dev, "missing reg property\n");
> +		return ret;
> +	}
> +
> +	if (of_property_read_bool(node, "apple,always-on"))
> +		ps->genpd.flags |= GENPD_FLAG_ALWAYS_ON;
> +
> +	ps->genpd.name = name;
> +	ps->genpd.power_on = apple_pmgr_ps_power_on;
> +	ps->genpd.power_off = apple_pmgr_ps_power_off;
> +
> +	ret = pm_genpd_init(&ps->genpd, NULL, !apple_pmgr_ps_is_active(ps));
> +	if (ret < 0) {
> +		dev_err(dev, "pm_genpd_init failed\n");
> +		return ret;
> +	}
> +
> +	ret = of_genpd_add_provider_simple(node, &ps->genpd);
> +	if (ret < 0) {
> +		dev_err(dev, "of_genpd_add_provider_simple failed\n");
> +		return ret;
> +	}
> +
> +	of_for_each_phandle(&it, ret, node, "power-domains", "#power-domain-cells", -1) {
> +		struct of_phandle_args parent, child;
> +
> +		parent.np = it.node;
> +		parent.args_count = of_phandle_iterator_args(&it, parent.args, MAX_PHANDLE_ARGS);
> +		child.np = node;
> +		child.args_count = 0;
> +		ret = of_genpd_add_subdomain(&parent, &child);
> +
> +		if (ret == -EPROBE_DEFER) {
> +			of_node_put(parent.np);
> +			goto err_remove;
> +		} else if (ret < 0) {
> +			dev_err(dev, "failed to add to parent domain: %d (%s -> %s)\n",
> +				ret, it.node->name, node->name);
> +			of_node_put(parent.np);
> +			goto err_remove;
> +		}
> +	}
> +
> +	pm_genpd_remove_device(dev);
> +
> +	ps->rcdev.owner = THIS_MODULE;
> +	ps->rcdev.nr_resets = 1;
> +	ps->rcdev.ops = &apple_pmgr_reset_ops;
> +	ps->rcdev.of_node = dev->of_node;
> +	ps->rcdev.of_reset_n_cells = 0;
> +	ps->rcdev.of_xlate = apple_pmgr_reset_xlate;
> +
> +	ret = devm_reset_controller_register(dev, &ps->rcdev);
> +	if (ret < 0)
> +		goto err_remove;
> +
> +	return 0;
> +err_remove:
> +	of_genpd_del_provider(node);
> +	pm_genpd_remove(&ps->genpd);
> +	return ret;
> +}
> +
> +static const struct of_device_id apple_pmgr_ps_of_match[] = {
> +	{ .compatible = "apple,t8103-pmgr-pwrstate" },
> +	{ .compatible = "apple,pmgr-pwrstate" },
> +	{}
> +};

I think this only needs to list "apple,pmgr-pwrstate" as that is the
one that will be present on all SoCs that is backward compatible with
the t8103 (M1) SoC.

> +
> +MODULE_DEVICE_TABLE(of, apple_pmgr_ps_of_match);
> +
> +static struct platform_driver apple_pmgr_ps_driver = {
> +	.probe = apple_pmgr_ps_probe,
> +	.driver = {
> +		.name = "apple-pmgr-pwrstate",
> +		.of_match_table = apple_pmgr_ps_of_match,
> +	},
> +};
> +
> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
> +MODULE_DESCRIPTION("PMGR power state driver for Apple SoCs");
> +MODULE_LICENSE("GPL v2");
> +
> +module_platform_driver(apple_pmgr_ps_driver);
> -- 
> 2.33.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 4/7] arm64: dts: apple: t8103: Rename clk24 to clkref

[Mark Kettenis]

> From: Hector Martin <marcan@marcan.st>
> Date: Wed,  6 Oct 2021 00:59:20 +0900
> Content-Type: text/plain; charset="us-ascii"
> 
> We now know that this frequency comes from the external reference
> oscillator and is used for various SoC blocks, and isn't just a random
> 24MHz clock, so let's call it something more appropriate.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  arch/arm64/boot/dts/apple/t8103.dtsi | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>

> diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
> index a1e22a2ea2e5..9f60f9e48ea0 100644
> --- a/arch/arm64/boot/dts/apple/t8103.dtsi
> +++ b/arch/arm64/boot/dts/apple/t8103.dtsi
> @@ -95,11 +95,11 @@ timer {
>  			     <AIC_FIQ AIC_TMR_HV_VIRT IRQ_TYPE_LEVEL_HIGH>;
>  	};
>  
> -	clk24: clock-24m {
> +	clkref: clock-ref {
>  		compatible = "fixed-clock";
>  		#clock-cells = <0>;
>  		clock-frequency = <24000000>;
> -		clock-output-names = "clk24";
> +		clock-output-names = "clkref";
>  	};
>  
>  	soc {
> @@ -120,7 +120,7 @@ serial0: serial@235200000 {
>  			 * TODO: figure out the clocking properly, there may
>  			 * be a third selectable clock.
>  			 */
> -			clocks = <&clk24>, <&clk24>;
> +			clocks = <&clkref>, <&clkref>;
>  			clock-names = "uart", "clk_uart_baud0";
>  			status = "disabled";
>  		};
> -- 
> 2.33.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 5/7] arm64: dts: apple: t8103: Add the UART PMGR tree

[Mark Kettenis]

> From: Hector Martin <marcan@marcan.st>
> Date: Wed,  6 Oct 2021 00:59:21 +0900
> Content-Type: text/plain; charset="us-ascii"
> 
> Note that the UART driver does not currently support runtime-pm, so this
> effectively always keeps the UART0 device on. However, this does clockgate
> all the other UARTs, as those are not currently instantiated.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  arch/arm64/boot/dts/apple/t8103.dtsi | 116 +++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>

> diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
> index 9f60f9e48ea0..63056ddc7ef7 100644
> --- a/arch/arm64/boot/dts/apple/t8103.dtsi
> +++ b/arch/arm64/boot/dts/apple/t8103.dtsi
> @@ -122,6 +122,7 @@ serial0: serial@235200000 {
>  			 */
>  			clocks = <&clkref>, <&clkref>;
>  			clock-names = "uart", "clk_uart_baud0";
> +			power-domains = <&ps_uart0>;
>  			status = "disabled";
>  		};
>  
> @@ -131,5 +132,120 @@ aic: interrupt-controller@23b100000 {
>  			interrupt-controller;
>  			reg = <0x2 0x3b100000 0x0 0x8000>;
>  		};
> +
> +		pmgr: power-management@23b700000 {
> +			compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			reg = <0x2 0x3b700000 0x0 0x14000>;
> +
> +			ps_sio_busif: power-controller@1c0 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x1c0>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "sio_busif";
> +			};
> +
> +			ps_sio: power-controller@1c8 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x1c8>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "sio";
> +				power-domains = <&ps_sio_busif>;
> +			};
> +
> +			ps_uart_p: power-controller@220 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x220>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "uart_p";
> +				power-domains = <&ps_sio>;
> +			};
> +
> +			ps_uart0: power-controller@270 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x270>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "uart0";
> +				power-domains = <&ps_uart_p>;
> +			};
> +
> +			ps_uart1: power-controller@278 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x278>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "uart1";
> +				power-domains = <&ps_uart_p>;
> +			};
> +
> +			ps_uart2: power-controller@280 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x280>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "uart2";
> +				power-domains = <&ps_uart_p>;
> +			};
> +
> +			ps_uart3: power-controller@288 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x288>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "uart3";
> +				power-domains = <&ps_uart_p>;
> +			};
> +
> +			ps_uart4: power-controller@290 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x290>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "uart4";
> +				power-domains = <&ps_uart_p>;
> +			};
> +
> +			ps_uart5: power-controller@298 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x298>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "uart5";
> +				power-domains = <&ps_uart_p>;
> +			};
> +
> +			ps_uart6: power-controller@2a0 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x2a0>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "uart6";
> +				power-domains = <&ps_uart_p>;
> +			};
> +
> +			ps_uart7: power-controller@2a8 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x2a8>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "uart7";
> +				power-domains = <&ps_uart_p>;
> +			};
> +
> +			ps_uart8: power-controller@2b0 {
> +				compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +				reg = <0x2b0>;
> +				#power-domain-cells = <0>;
> +				#reset-cells = <0>;
> +				apple,domain-name = "uart8";
> +				power-domains = <&ps_uart_p>;
> +			};
> +		};
>  	};
>  };
> -- 
> 2.33.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 7/7] arm64: dts: apple: t8103: Add UART2

[Mark Kettenis]

> From: Hector Martin <marcan@marcan.st>
> Date: Wed,  6 Oct 2021 00:59:23 +0900
> 
> This UART is connected to the debug port of the WLAN module. It is
> mostly useless, but makes for a good test case for runtime-pm without
> having to unbind the console from the main system UART.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  arch/arm64/boot/dts/apple/t8103-j274.dts |  5 +++++
>  arch/arm64/boot/dts/apple/t8103.dtsi     | 12 ++++++++++++
>  2 files changed, 17 insertions(+)

Doesn't break U-Boot or OpenBSD.

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>


> diff --git a/arch/arm64/boot/dts/apple/t8103-j274.dts b/arch/arm64/boot/dts/apple/t8103-j274.dts
> index e0f6775b9878..16c5eb7f53b1 100644
> --- a/arch/arm64/boot/dts/apple/t8103-j274.dts
> +++ b/arch/arm64/boot/dts/apple/t8103-j274.dts
> @@ -17,6 +17,7 @@ / {
>  
>  	aliases {
>  		serial0 = &serial0;
> +		serial2 = &serial2;
>  	};
>  
>  	chosen {
> @@ -43,3 +44,7 @@ memory@800000000 {
>  &serial0 {
>  	status = "okay";
>  };
> +
> +&serial2 {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
> index 63056ddc7ef7..844ed7bd0451 100644
> --- a/arch/arm64/boot/dts/apple/t8103.dtsi
> +++ b/arch/arm64/boot/dts/apple/t8103.dtsi
> @@ -126,6 +126,18 @@ serial0: serial@235200000 {
>  			status = "disabled";
>  		};
>  
> +		serial2: serial@235208000 {
> +			compatible = "apple,s5l-uart";
> +			reg = <0x2 0x35208000 0x0 0x1000>;
> +			reg-io-width = <4>;
> +			interrupt-parent = <&aic>;
> +			interrupts = <AIC_IRQ 607 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&clkref>, <&clkref>;
> +			clock-names = "uart", "clk_uart_baud0";
> +			power-domains = <&ps_uart2>;
> +			status = "disabled";
> +		};
> +
>  		aic: interrupt-controller@23b100000 {
>  			compatible = "apple,t8103-aic", "apple,aic";
>  			#interrupt-cells = <3>;
> -- 
> 2.33.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 1/7] dt-bindings: arm: apple: Add apple,pmgr binding

[Rob Herring]

On Wed, 06 Oct 2021 00:59:17 +0900, Hector Martin wrote:
> The PMGR block in Apple Silicon SoCs is responsible for SoC power
> management. There are two PMGRs in T8103, with different register
> layouts but compatible registers. In order to support this as well
> as future SoC generations with backwards-compatible registers, we
> declare these blocks as syscons and bind to individual registers
> in child nodes. Each register controls one SoC device.
> 
> The respective apple compatibles are defined in case device-specific
> quirks are necessary in the future, but currently these nodes are
> expected to be bound by the generic syscon driver.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  .../bindings/arm/apple/apple,pmgr.yaml        | 74 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/arm/apple/apple,pmgr.example.dts:30.40-35.15: ERROR (duplicate_node_names): /example-0/soc/power-management@23b700000: Duplicate node name
ERROR: Input tree has errors, aborting (use -f to force output)
make[1]: *** [scripts/Makefile.lib:385: Documentation/devicetree/bindings/arm/apple/apple,pmgr.example.dt.yaml] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1441: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1536742

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Rob Herring]

On Tue, Oct 5, 2021 at 10:59 AM Hector Martin <marcan@marcan.st> wrote:
>
> This syscon child node represents a single SoC device controlled by the
> PMGR block. This layout allows us to declare all device power state
> controls (power/clock gating and reset) in the device tree, including
> dependencies, instead of hardcoding it into the driver. The register
> layout is uniform.
>
> Each pmgr-pwrstate node provides genpd and reset features, to be
> consumed by downstream device nodes.
>
> Future SoCs are expected to use backwards compatible registers, and the
> "apple,pmgr-pwrstate" represents any such interfaces (possibly with
> additional features gated by the more specific compatible), allowing
> them to be bound without driver updates. If a backwards incompatible
> change is introduced in future SoCs, it will require a new compatible,
> such as "apple,pmgr-pwrstate-v2".

Is that because past SoCs used the same registers? I don't see how
else you have any insight to what future SoCs will do.

Normally we don't do 1 node per register type bindings, so I'm a bit
leery about doing 1 node per domain.

>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  .../bindings/power/apple,pmgr-pwrstate.yaml   | 117 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
>
> diff --git a/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> new file mode 100644
> index 000000000000..a14bf5f30ff0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/apple,pmgr-pwrstate.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SoC PMGR Power States
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +allOf:
> +  - $ref: "power-domain.yaml#"
> +
> +description: |
> +  Apple SoCs include a PMGR block responsible for power management,
> +  which can control various clocks, resets, power states, and
> +  performance features. This binding describes the device power
> +  state registers, which control power states and resets.
> +
> +  Each instance of a power controller within the PMGR syscon node
> +  represents a generic power domain provider, as documented in
> +  Documentation/devicetree/bindings/power/power-domain.yaml.
> +  The provider controls a single SoC block. The power hierarchy is
> +  represented via power-domains relationships between these nodes.
> +
> +  See Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> +  for the top-level PMGR node documentation.
> +
> +  IP cores belonging to a power domain should contain a
> +  "power-domains" property that is a phandle for the
> +  power domain node representing the domain.
> +
> +properties:
> +  $nodename:
> +    pattern: "^power-controller@[0-9a-f]+$"

Drop this and define this node in the syscon schema with a $ref to this schema.

> +
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t8103-pmgr-pwrstate
> +      - const: apple,pmgr-pwrstate
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#power-domain-cells":
> +    const: 0
> +
> +  "#reset-cells":
> +    const: 0
> +
> +  power-domains:
> +    description:
> +      Reference to parent power domains. A domain may have multiple parents,
> +      and all will be powered up when it is powered.
> +
> +  apple,domain-name:
> +    description: |
> +      Specifies the name of the SoC device being controlled. This is used to
> +      name the power/reset domains.
> +    $ref: /schemas/types.yaml#/definitions/string

No other power domain binding needs this, why do you?

> +
> +  apple,always-on:
> +    description: |
> +      Forces this power domain to always be powered up.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#power-domain-cells"
> +  - "#reset-cells"
> +  - "apple,domain-name"
> +
> +additionalProperties: false
> +
> +examples:

I prefer 1 complete example in the MFD schema rather than piecemeal examples.

> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        power-management@23b700000 {
> +            compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
> +            #address-cells = <1>;
> +            #size-cells = <0>;

As the child nodes are memory mapped devices, size should be 1. Then
address translation works (though Linux doesn't care (currently)).

> +            reg = <0x2 0x3b700000 0x0 0x14000>;
> +
> +            ps_sio: power-controller@1c0 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x1c0>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "sio";
> +                apple,always-on;
> +            };
> +
> +            ps_uart_p: power-controller@220 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x220>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "uart_p";
> +                power-domains = <&ps_sio>;
> +            };
> +
> +            ps_uart0: power-controller@270 {
> +                compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
> +                reg = <0x270>;
> +                #power-domain-cells = <0>;
> +                #reset-cells = <0>;
> +                apple,domain-name = "uart0";
> +                power-domains = <&ps_uart_p>;
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d25598842d15..5fe53d9a2956 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1722,6 +1722,7 @@ F:        Documentation/devicetree/bindings/arm/apple.yaml
>  F:     Documentation/devicetree/bindings/arm/apple/*
>  F:     Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
>  F:     Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
> +F:     Documentation/devicetree/bindings/power/apple*
>  F:     arch/arm64/boot/dts/apple/
>  F:     drivers/irqchip/irq-apple-aic.c
>  F:     include/dt-bindings/interrupt-controller/apple-aic.h
> --
> 2.33.0
>

Re: [PATCH 1/7] dt-bindings: arm: apple: Add apple,pmgr binding

[Krzysztof Kozlowski]

On 05/10/2021 17:59, Hector Martin wrote:
> The PMGR block in Apple Silicon SoCs is responsible for SoC power
> management. There are two PMGRs in T8103, with different register
> layouts but compatible registers. In order to support this as well
> as future SoC generations with backwards-compatible registers, we
> declare these blocks as syscons and bind to individual registers
> in child nodes. Each register controls one SoC device.
> 
> The respective apple compatibles are defined in case device-specific
> quirks are necessary in the future, but currently these nodes are
> expected to be bound by the generic syscon driver.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  .../bindings/arm/apple/apple,pmgr.yaml        | 74 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> new file mode 100644
> index 000000000000..0304164e4140
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/apple/apple,pmgr.yaml#

Please don't store all Apple-related bindings in bindings/arm/apple, but
instead group per device type like in most of other bindings. In this
case - this looks like something close to power domain controller, so it
should be in bindings/power/

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SoC Power Manager (PMGR)
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +description: |
> +  Apple SoCs include a PMGR block responsible for power management,
> +  which can control various clocks, resets, power states, and
> +  performance features. This node represents the PMGR as a syscon,
> +  with sub-nodes representing individual features.
> +
> +  Apple SoCs may have a secondary "mini-PMGR"; it is represented
> +  separately in the device tree, but works the same way.
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - apple,t8103-pmgr
> +          - apple,t8103-minipmgr
> +          - apple,pmgr
> +
> +  required:
> +    - compatible
> +
> +properties:
> +  $nodename:
> +    pattern: "^power-management@[0-9a-f]+$"
> +
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t8103-pmgr
> +          - apple,t8103-minipmgr
> +      - const: apple,pmgr
> +      - const: syscon
> +      - const: simple-mfd

No power-domain-cells? Why? What exactly this device is going to do?
Maybe I'll check the driver first.... :)

> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: true

additionalProperties: false

> +
> +examples:
> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        power-management@23b700000 {
> +            compatible = "apple,t8103-pmgr", "apple,pmgr", "syscon", "simple-mfd";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0x2 0x3b700000 0x0 0x14000>;
> +        };
> +
> +        power-management@23b700000 {
> +            compatible = "apple,t8103-minipmgr", "apple,pmgr", "syscon", "simple-mfd";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0x2 0x3d280000 0x0 0xc000>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index abdcbcfef73d..d25598842d15 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1719,6 +1719,7 @@ B:	https://github.com/AsahiLinux/linux/issues
>  C:	irc://irc.oftc.net/asahi-dev
>  T:	git https://github.com/AsahiLinux/linux.git
>  F:	Documentation/devicetree/bindings/arm/apple.yaml
> +F:	Documentation/devicetree/bindings/arm/apple/*
>  F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
>  F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
>  F:	arch/arm64/boot/dts/apple/
> 


Best regards,
Krzysztof

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Krzysztof Kozlowski]

On 05/10/2021 17:59, Hector Martin wrote:
> This syscon child node represents a single SoC device controlled by the
> PMGR block. This layout allows us to declare all device power state
> controls (power/clock gating and reset) in the device tree, including
> dependencies, instead of hardcoding it into the driver. The register
> layout is uniform.
> 
> Each pmgr-pwrstate node provides genpd and reset features, to be
> consumed by downstream device nodes.
> 
> Future SoCs are expected to use backwards compatible registers, and the
> "apple,pmgr-pwrstate" represents any such interfaces (possibly with
> additional features gated by the more specific compatible), allowing
> them to be bound without driver updates. If a backwards incompatible
> change is introduced in future SoCs, it will require a new compatible,
> such as "apple,pmgr-pwrstate-v2".
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  .../bindings/power/apple,pmgr-pwrstate.yaml   | 117 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> new file mode 100644
> index 000000000000..a14bf5f30ff0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/apple,pmgr-pwrstate.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SoC PMGR Power States
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +allOf:
> +  - $ref: "power-domain.yaml#"
> +
> +description: |
> +  Apple SoCs include a PMGR block responsible for power management,
> +  which can control various clocks, resets, power states, and
> +  performance features. This binding describes the device power
> +  state registers, which control power states and resets.
> +
> +  Each instance of a power controller within the PMGR syscon node
> +  represents a generic power domain provider, as documented in
> +  Documentation/devicetree/bindings/power/power-domain.yaml.
> +  The provider controls a single SoC block. The power hierarchy is
> +  represented via power-domains relationships between these nodes.
> +
> +  See Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
> +  for the top-level PMGR node documentation.
> +
> +  IP cores belonging to a power domain should contain a
> +  "power-domains" property that is a phandle for the
> +  power domain node representing the domain.

Skip this last paragraph - it is obvious in usage of power domains.
Specific bindings should not duplicate generic knowledge.

> +
> +properties:
> +  $nodename:
> +    pattern: "^power-controller@[0-9a-f]+$"

Usually we call nodes as power-domain.

> +
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t8103-pmgr-pwrstate
> +      - const: apple,pmgr-pwrstate
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#power-domain-cells":
> +    const: 0
> +
> +  "#reset-cells":
> +    const: 0
> +
> +  power-domains:
> +    description:
> +      Reference to parent power domains. A domain may have multiple parents,
> +      and all will be powered up when it is powered.

How many items?

> +
> +  apple,domain-name:

Use existing binding "label".

> +    description: |
> +      Specifies the name of the SoC device being controlled. This is used to
> +      name the power/reset domains.
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  apple,always-on:
> +    description: |
> +      Forces this power domain to always be powered up.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#power-domain-cells"
> +  - "#reset-cells"
> +  - "apple,domain-name"
> +
> +additionalProperties: false

Your parent schema should include this one for evaluating children.



Best regards,
Krzysztof

Re: [PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Krzysztof Kozlowski]

On 05/10/2021 17:59, Hector Martin wrote:
> Implements genpd and reset providers for downstream devices. Each
> instance of the driver binds to a single register and represents a
> single SoC power domain.
> 
> The driver does not currently implement all features (auto-pm,
> clockgate-only state), but we declare the respective registers for
> documentation purposes. These features will be added as they become
> useful for downstream devices.
> 
> This also creates the apple/soc tree and Kconfig submenu.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  MAINTAINERS                             |   1 +
>  drivers/soc/Kconfig                     |   1 +
>  drivers/soc/Makefile                    |   1 +
>  drivers/soc/apple/Kconfig               |  21 ++
>  drivers/soc/apple/Makefile              |   2 +
>  drivers/soc/apple/apple-pmgr-pwrstate.c | 281 ++++++++++++++++++++++++
>  6 files changed, 307 insertions(+)
>  create mode 100644 drivers/soc/apple/Kconfig
>  create mode 100644 drivers/soc/apple/Makefile
>  create mode 100644 drivers/soc/apple/apple-pmgr-pwrstate.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5fe53d9a2956..def5e05da2bc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1725,6 +1725,7 @@ F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
>  F:	Documentation/devicetree/bindings/power/apple*
>  F:	arch/arm64/boot/dts/apple/
>  F:	drivers/irqchip/irq-apple-aic.c
> +F:	drivers/soc/apple/*
>  F:	include/dt-bindings/interrupt-controller/apple-aic.h
>  F:	include/dt-bindings/pinctrl/apple.h
>  
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index e8a30c4c5aec..a8562678c437 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -3,6 +3,7 @@ menu "SOC (System On Chip) specific Drivers"
>  
>  source "drivers/soc/actions/Kconfig"
>  source "drivers/soc/amlogic/Kconfig"
> +source "drivers/soc/apple/Kconfig"
>  source "drivers/soc/aspeed/Kconfig"
>  source "drivers/soc/atmel/Kconfig"
>  source "drivers/soc/bcm/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index a05e9fbcd3e0..adb30c2d4fea 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -4,6 +4,7 @@
>  #
>  
>  obj-$(CONFIG_ARCH_ACTIONS)	+= actions/
> +obj-$(CONFIG_ARCH_APPLE)	+= apple/
>  obj-y				+= aspeed/
>  obj-$(CONFIG_ARCH_AT91)		+= atmel/
>  obj-y				+= bcm/
> diff --git a/drivers/soc/apple/Kconfig b/drivers/soc/apple/Kconfig
> new file mode 100644
> index 000000000000..271092b6aee7
> --- /dev/null
> +++ b/drivers/soc/apple/Kconfig
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +if ARCH_APPLE || COMPILE_TEST
> +
> +menu "Apple SoC drivers"
> +
> +config APPLE_PMGR_PWRSTATE
> +	tristate "Apple SoC PMGR power state control"
> +	select REGMAP
> +	select MFD_SYSCON
> +	select PM_GENERIC_DOMAINS
> +	select RESET_CONTROLLER
> +	default ARCH_APPLE
> +	help
> +	  The PMGR block in Apple SoCs provides high-level power state
> +	  controls for SoC devices. This driver manages them through the
> +	  generic power domain framework, and also provides reset support.
> +
> +endmenu
> +
> +endif
> diff --git a/drivers/soc/apple/Makefile b/drivers/soc/apple/Makefile
> new file mode 100644
> index 000000000000..c114e84667e4
> --- /dev/null
> +++ b/drivers/soc/apple/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_APPLE_PMGR_PWRSTATE)	+= apple-pmgr-pwrstate.o
> diff --git a/drivers/soc/apple/apple-pmgr-pwrstate.c b/drivers/soc/apple/apple-pmgr-pwrstate.c
> new file mode 100644
> index 000000000000..a0338dbb29b8
> --- /dev/null
> +++ b/drivers/soc/apple/apple-pmgr-pwrstate.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SoC PMGR device power state driver
> + *
> + * Copyright The Asahi Linux Contributors
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/bitfield.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/reset-controller.h>
> +#include <linux/module.h>
> +
> +#define APPLE_PMGR_RESET        BIT(31)
> +#define APPLE_PMGR_AUTO_ENABLE  BIT(28)
> +#define APPLE_PMGR_PS_AUTO      GENMASK(27, 24)
> +#define APPLE_PMGR_PARENT_OFF   BIT(11)
> +#define APPLE_PMGR_DEV_DISABLE  BIT(10)
> +#define APPLE_PMGR_WAS_CLKGATED BIT(9)
> +#define APPLE_PMGR_WAS_PWRGATED BIT(8)
> +#define APPLE_PMGR_PS_ACTUAL    GENMASK(7, 4)
> +#define APPLE_PMGR_PS_TARGET    GENMASK(3, 0)
> +
> +#define APPLE_PMGR_PS_ACTIVE    0xf
> +#define APPLE_PMGR_PS_CLKGATE   0x4
> +#define APPLE_PMGR_PS_PWRGATE   0x0
> +
> +#define APPLE_PMGR_PS_SET_TIMEOUT 100
> +#define APPLE_PMGR_RESET_TIME 1
> +
> +struct apple_pmgr_ps {
> +	struct device *dev;
> +	struct generic_pm_domain genpd;
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +	u32 offset;
> +};
> +
> +#define genpd_to_apple_pmgr_ps(_genpd) container_of(_genpd, struct apple_pmgr_ps, genpd)
> +#define rcdev_to_apple_pmgr_ps(_rcdev) container_of(_rcdev, struct apple_pmgr_ps, rcdev)
> +
> +static int apple_pmgr_ps_set(struct generic_pm_domain *genpd, u32 pstate)
> +{
> +	int ret;
> +	struct apple_pmgr_ps *ps = genpd_to_apple_pmgr_ps(genpd);
> +	u32 reg;
> +
> +	regmap_read(ps->regmap, ps->offset, &reg);

MMIO accesses should not fail, but regmap API could fail if for example
clk_enable() fails. In such case you will write below value based on
random stack init. Please check the return value here.

> +
> +	/* Resets are synchronous, and only work if the device is powered and clocked. */
> +	if (reg & APPLE_PMGR_RESET && pstate != APPLE_PMGR_PS_ACTIVE)
> +		dev_err(ps->dev, "PS 0x%x: powering off with RESET active\n", ps->offset);
> +
> +	reg &= ~(APPLE_PMGR_AUTO_ENABLE | APPLE_PMGR_WAS_CLKGATED | APPLE_PMGR_WAS_PWRGATED |
> +		 APPLE_PMGR_PS_TARGET);
> +	reg |= FIELD_PREP(APPLE_PMGR_PS_TARGET, pstate);
> +
> +	dev_dbg(ps->dev, "PS 0x%x: pwrstate = 0x%x: 0x%x\n", ps->offset, pstate, reg);
> +
> +	regmap_write(ps->regmap, ps->offset, reg);
> +
> +	ret = regmap_read_poll_timeout_atomic(
> +		ps->regmap, ps->offset, reg,
> +		(FIELD_GET(APPLE_PMGR_PS_ACTUAL, reg) == pstate), 1,
> +		APPLE_PMGR_PS_SET_TIMEOUT);
> +	if (ret < 0)
> +		dev_err(ps->dev, "PS 0x%x: Failed to reach power state 0x%x (now: 0x%x)\n",
> +			ps->offset, pstate, reg);
> +	return ret;
> +}
> +
> +static bool apple_pmgr_ps_is_active(struct apple_pmgr_ps *ps)
> +{
> +	u32 reg;
> +
> +	regmap_read(ps->regmap, ps->offset, &reg);

Check the return value or initialize reg to 0.

> +	return FIELD_GET(APPLE_PMGR_PS_ACTUAL, reg) == APPLE_PMGR_PS_ACTIVE;
> +}
> +
> +static int apple_pmgr_ps_power_on(struct generic_pm_domain *genpd)
> +{
> +	return apple_pmgr_ps_set(genpd, APPLE_PMGR_PS_ACTIVE);
> +}
> +
> +static int apple_pmgr_ps_power_off(struct generic_pm_domain *genpd)
> +{
> +	return apple_pmgr_ps_set(genpd, APPLE_PMGR_PS_PWRGATE);
> +}
> +
> +static int apple_pmgr_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
> +
> +	mutex_lock(&ps->genpd.mlock);
> +
> +	if (ps->genpd.status == GENPD_STATE_OFF)
> +		dev_err(ps->dev, "PS 0x%x: asserting RESET while powered down\n", ps->offset);
> +
> +	dev_dbg(ps->dev, "PS 0x%x: assert reset\n", ps->offset);
> +	/* Quiesce device before asserting reset */
> +	regmap_set_bits(ps->regmap, ps->offset, APPLE_PMGR_DEV_DISABLE);
> +	regmap_set_bits(ps->regmap, ps->offset, APPLE_PMGR_RESET);
> +
> +	mutex_unlock(&ps->genpd.mlock);
> +
> +	return 0;
> +}
> +
> +static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
> +
> +	mutex_lock(&ps->genpd.mlock);

This looks wrong: it can be a spin-lock, not mutex, so you should use
genpd_lock.

However now I wonder if there could be a case when a reset-controller
consumer calls it from it's GENPD_NOTIFY_ON notifier? In such case you
would have this lock taken.

> +
> +	dev_dbg(ps->dev, "PS 0x%x: deassert reset\n", ps->offset);
> +	regmap_clear_bits(ps->regmap, ps->offset, APPLE_PMGR_RESET);
> +	regmap_clear_bits(ps->regmap, ps->offset, APPLE_PMGR_DEV_DISABLE);
> +
> +	if (ps->genpd.status == GENPD_STATE_OFF)
> +		dev_err(ps->dev, "PS 0x%x: RESET was deasserted while powered down\n", ps->offset);
> +
> +	mutex_unlock(&ps->genpd.mlock);
> +
> +	return 0;
> +}
> +
> +static int apple_pmgr_reset_reset(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	int ret;
> +
> +	ret = apple_pmgr_reset_assert(rcdev, id);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(APPLE_PMGR_RESET_TIME, 2 * APPLE_PMGR_RESET_TIME);
> +
> +	return apple_pmgr_reset_deassert(rcdev, id);
> +}
> +
> +static int apple_pmgr_reset_status(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
> +	u32 reg;
> +
> +	regmap_read(ps->regmap, ps->offset, &reg);
> +
> +	return !!(reg & APPLE_PMGR_RESET);
> +}
> +
> +const struct reset_control_ops apple_pmgr_reset_ops = {
> +	.assert		= apple_pmgr_reset_assert,
> +	.deassert	= apple_pmgr_reset_deassert,
> +	.reset		= apple_pmgr_reset_reset,
> +	.status		= apple_pmgr_reset_status,
> +};
> +
> +static int apple_pmgr_reset_xlate(struct reset_controller_dev *rcdev,
> +				  const struct of_phandle_args *reset_spec)
> +{
> +	return 0;
> +}
> +
> +static int apple_pmgr_ps_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct apple_pmgr_ps *ps;
> +	struct regmap *regmap;
> +	struct of_phandle_iterator it;
> +	int ret;
> +	const char *name;
> +
> +	regmap = syscon_node_to_regmap(node->parent);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	ps = devm_kzalloc(dev, sizeof(*ps), GFP_KERNEL);
> +	if (!ps)
> +		return -ENOMEM;
> +
> +	ps->dev = dev;
> +	ps->regmap = regmap;
> +
> +	ret = of_property_read_string(node, "apple,domain-name", &name);
> +	if (ret < 0) {
> +		dev_err(dev, "missing apple,domain-name property\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "reg", &ps->offset);
> +	if (ret < 0) {
> +		dev_err(dev, "missing reg property\n");
> +		return ret;
> +	}
> +
> +	if (of_property_read_bool(node, "apple,always-on"))
> +		ps->genpd.flags |= GENPD_FLAG_ALWAYS_ON;
> +
> +	ps->genpd.name = name;
> +	ps->genpd.power_on = apple_pmgr_ps_power_on;
> +	ps->genpd.power_off = apple_pmgr_ps_power_off;
> +
> +	ret = pm_genpd_init(&ps->genpd, NULL, !apple_pmgr_ps_is_active(ps));
> +	if (ret < 0) {
> +		dev_err(dev, "pm_genpd_init failed\n");
> +		return ret;
> +	}
> +
> +	ret = of_genpd_add_provider_simple(node, &ps->genpd);
> +	if (ret < 0) {
> +		dev_err(dev, "of_genpd_add_provider_simple failed\n");
> +		return ret;
> +	}
> +
> +	of_for_each_phandle(&it, ret, node, "power-domains", "#power-domain-cells", -1) {
> +		struct of_phandle_args parent, child;
> +
> +		parent.np = it.node;
> +		parent.args_count = of_phandle_iterator_args(&it, parent.args, MAX_PHANDLE_ARGS);
> +		child.np = node;
> +		child.args_count = 0;
> +		ret = of_genpd_add_subdomain(&parent, &child);
> +
> +		if (ret == -EPROBE_DEFER) {
> +			of_node_put(parent.np);
> +			goto err_remove;
> +		} else if (ret < 0) {
> +			dev_err(dev, "failed to add to parent domain: %d (%s -> %s)\n",
> +				ret, it.node->name, node->name);
> +			of_node_put(parent.np);
> +			goto err_remove;
> +		}
> +	}
> +
> +	pm_genpd_remove_device(dev);
> +
> +	ps->rcdev.owner = THIS_MODULE;
> +	ps->rcdev.nr_resets = 1;
> +	ps->rcdev.ops = &apple_pmgr_reset_ops;
> +	ps->rcdev.of_node = dev->of_node;
> +	ps->rcdev.of_reset_n_cells = 0;
> +	ps->rcdev.of_xlate = apple_pmgr_reset_xlate;
> +
> +	ret = devm_reset_controller_register(dev, &ps->rcdev);
> +	if (ret < 0)
> +		goto err_remove;
> +
> +	return 0;
> +err_remove:
> +	of_genpd_del_provider(node);
> +	pm_genpd_remove(&ps->genpd);
> +	return ret;
> +}
> +
> +static const struct of_device_id apple_pmgr_ps_of_match[] = {
> +	{ .compatible = "apple,t8103-pmgr-pwrstate" },
> +	{ .compatible = "apple,pmgr-pwrstate" },

You call the device/driver "pwrstate", which it seems is "power state".
These are not power states. These are power controllers or power
domains. Power state is rather a state of power domain - e.g. on or
gated. How about renaming it to power domain or pd?

> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, apple_pmgr_ps_of_match);
> +
> +static struct platform_driver apple_pmgr_ps_driver = {
> +	.probe = apple_pmgr_ps_probe,
> +	.driver = {
> +		.name = "apple-pmgr-pwrstate",
> +		.of_match_table = apple_pmgr_ps_of_match,
> +	},
> +};
> +
> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
> +MODULE_DESCRIPTION("PMGR power state driver for Apple SoCs");
> +MODULE_LICENSE("GPL v2");
> +
> +module_platform_driver(apple_pmgr_ps_driver);
> 


Best regards,
Krzysztof

Re: [PATCH 1/7] dt-bindings: arm: apple: Add apple,pmgr binding

[Krzysztof Kozlowski]

On 06/10/2021 08:56, Krzysztof Kozlowski wrote:
> On 05/10/2021 17:59, Hector Martin wrote:
>> The PMGR block in Apple Silicon SoCs is responsible for SoC power
>> management. There are two PMGRs in T8103, with different register
>> layouts but compatible registers. In order to support this as well
>> as future SoC generations with backwards-compatible registers, we
>> declare these blocks as syscons and bind to individual registers
>> in child nodes. Each register controls one SoC device.
>>
>> The respective apple compatibles are defined in case device-specific
>> quirks are necessary in the future, but currently these nodes are
>> expected to be bound by the generic syscon driver.
>>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> ---
>>  .../bindings/arm/apple/apple,pmgr.yaml        | 74 +++++++++++++++++++
>>  MAINTAINERS                                   |  1 +
>>  2 files changed, 75 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
>> new file mode 100644
>> index 000000000000..0304164e4140
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
>> @@ -0,0 +1,74 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/arm/apple/apple,pmgr.yaml#
> 
> Please don't store all Apple-related bindings in bindings/arm/apple, but
> instead group per device type like in most of other bindings. In this
> case - this looks like something close to power domain controller, so it
> should be in bindings/power/
> 
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Apple SoC Power Manager (PMGR)
>> +
>> +maintainers:
>> +  - Hector Martin <marcan@marcan.st>
>> +
>> +description: |
>> +  Apple SoCs include a PMGR block responsible for power management,
>> +  which can control various clocks, resets, power states, and
>> +  performance features. This node represents the PMGR as a syscon,
>> +  with sub-nodes representing individual features.
>> +
>> +  Apple SoCs may have a secondary "mini-PMGR"; it is represented
>> +  separately in the device tree, but works the same way.
>> +
>> +select:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        enum:
>> +          - apple,t8103-pmgr
>> +          - apple,t8103-minipmgr
>> +          - apple,pmgr
>> +
>> +  required:
>> +    - compatible
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^power-management@[0-9a-f]+$"
>> +
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - apple,t8103-pmgr
>> +          - apple,t8103-minipmgr
>> +      - const: apple,pmgr
>> +      - const: syscon
>> +      - const: simple-mfd
> 
> No power-domain-cells? Why? What exactly this device is going to do?
> Maybe I'll check the driver first.... :)
> 

After looking at the code, there is no device for
apple,t8103-pmgr/apple,pmgr. What is this binding about? Is there really
a central (central as in "one device for SoC") block managing power
which you want to model here?


Best regards,
Krzysztof

Re: [PATCH 6/7] tty: serial: samsung_tty: Support runtime PM

[Krzysztof Kozlowski]

On 05/10/2021 17:59, Hector Martin wrote:
> This allows idle UART devices to be suspended using the standard
> runtime-PM framework. The logic is modeled after stm32-usart.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/tty/serial/samsung_tty.c | 88 ++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index e2f49863e9c2..d68e3341adc6 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -40,6 +40,7 @@
>  #include <linux/clk.h>
>  #include <linux/cpufreq.h>
>  #include <linux/of.h>
> +#include <linux/pm_runtime.h>
>  #include <asm/irq.h>
>  
>  /* UART name and device definitions */
> @@ -1381,31 +1382,49 @@ static void exynos_usi_init(struct uart_port *port)
>  
>  /* power power management control */
>  
> -static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
> -			      unsigned int old)
> +static int __maybe_unused s3c24xx_serial_runtime_suspend(struct device *dev)
>  {
> +	struct uart_port *port = dev_get_drvdata(dev);
>  	struct s3c24xx_uart_port *ourport = to_ourport(port);
>  	int timeout = 10000;
>  
> -	ourport->pm_level = level;
> +	while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
> +		udelay(100);
>  
> -	switch (level) {
> -	case 3:
> -		while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
> -			udelay(100);
> +	if (!IS_ERR(ourport->baudclk))
> +		clk_disable_unprepare(ourport->baudclk);
>  
> -		if (!IS_ERR(ourport->baudclk))
> -			clk_disable_unprepare(ourport->baudclk);
> +	clk_disable_unprepare(ourport->clk);
> +	return 0;
> +};
>  
> -		clk_disable_unprepare(ourport->clk);
> -		break;
> +static int __maybe_unused s3c24xx_serial_runtime_resume(struct device *dev)
> +{
> +	struct uart_port *port = dev_get_drvdata(dev);
> +	struct s3c24xx_uart_port *ourport = to_ourport(port);
>  
> -	case 0:
> -		clk_prepare_enable(ourport->clk);
> +	clk_prepare_enable(ourport->clk);
>  
> -		if (!IS_ERR(ourport->baudclk))
> -			clk_prepare_enable(ourport->baudclk);
> +	if (!IS_ERR(ourport->baudclk))
> +		clk_prepare_enable(ourport->baudclk);
> +	return 0;
> +};
> +
> +static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
> +			      unsigned int old)
> +{
> +	struct s3c24xx_uart_port *ourport = to_ourport(port);
> +
> +	ourport->pm_level = level;
>  
> +	switch (level) {
> +	case UART_PM_STATE_OFF:
> +		pm_runtime_mark_last_busy(port->dev);
> +		pm_runtime_put_sync(port->dev);
> +		break;
> +
> +	case UART_PM_STATE_ON:
> +		pm_runtime_get_sync(port->dev);
>  		exynos_usi_init(port);
>  		break;
>  	default:
> @@ -2282,18 +2301,15 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +

You need to cleanup in error paths (put/disable).

>  	dev_dbg(&pdev->dev, "%s: adding port\n", __func__);
>  	uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
>  	platform_set_drvdata(pdev, &ourport->port);
>  
> -	/*
> -	 * Deactivate the clock enabled in s3c24xx_serial_init_port here,
> -	 * so that a potential re-enablement through the pm-callback overlaps
> -	 * and keeps the clock enabled in this case.
> -	 */
> -	clk_disable_unprepare(ourport->clk);
> -	if (!IS_ERR(ourport->baudclk))
> -		clk_disable_unprepare(ourport->baudclk);
> +	pm_runtime_put_sync(&pdev->dev);
>  
>  	ret = s3c24xx_serial_cpufreq_register(ourport);
>  	if (ret < 0)
> @@ -2309,8 +2325,14 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
>  	struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);
>  
>  	if (port) {
> +		pm_runtime_get_sync(&dev->dev);

1. You need to check return status.
2. Why do you need to resume the device here?

> +
>  		s3c24xx_serial_cpufreq_deregister(to_ourport(port));
>  		uart_remove_one_port(&s3c24xx_uart_drv, port);
> +
> +		pm_runtime_disable(&dev->dev);

Why disabling it only if port!=NULL? Can remove() be called if
platform_set_drvdata() was not?

> +		pm_runtime_set_suspended(&dev->dev);
> +		pm_runtime_put_noidle(&dev->dev);
>  	}
>  
>  	uart_unregister_driver(&s3c24xx_uart_drv);
> @@ -2319,8 +2341,8 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
>  }
>  

Best regards,
Krzysztof

Re: [PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Philipp Zabel]

Hi Hector,

On Wed, 2021-10-06 at 00:59 +0900, Hector Martin wrote:
> Implements genpd and reset providers for downstream devices. Each
> instance of the driver binds to a single register and represents a
> single SoC power domain.
> 
> The driver does not currently implement all features (auto-pm,
> clockgate-only state), but we declare the respective registers for
> documentation purposes. These features will be added as they become
> useful for downstream devices.
> 
> This also creates the apple/soc tree and Kconfig submenu.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  MAINTAINERS                             |   1 +
>  drivers/soc/Kconfig                     |   1 +
>  drivers/soc/Makefile                    |   1 +
>  drivers/soc/apple/Kconfig               |  21 ++
>  drivers/soc/apple/Makefile              |   2 +
>  drivers/soc/apple/apple-pmgr-pwrstate.c | 281 ++++++++++++++++++++++++
>  6 files changed, 307 insertions(+)
>  create mode 100644 drivers/soc/apple/Kconfig
>  create mode 100644 drivers/soc/apple/Makefile
>  create mode 100644 drivers/soc/apple/apple-pmgr-pwrstate.c
> 
[...]
> diff --git a/drivers/soc/apple/apple-pmgr-pwrstate.c b/drivers/soc/apple/apple-pmgr-pwrstate.c
> new file mode 100644
> index 000000000000..a0338dbb29b8
> --- /dev/null
> +++ b/drivers/soc/apple/apple-pmgr-pwrstate.c
> @@ -0,0 +1,281 @@
[...]
> +static int apple_pmgr_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
> +
> +	mutex_lock(&ps->genpd.mlock);
> +
> +	if (ps->genpd.status == GENPD_STATE_OFF)
> +		dev_err(ps->dev, "PS 0x%x: asserting RESET while powered down\n", ps->offset);
> +
> +	dev_dbg(ps->dev, "PS 0x%x: assert reset\n", ps->offset);
> +	/* Quiesce device before asserting reset */
> +	regmap_set_bits(ps->regmap, ps->offset, APPLE_PMGR_DEV_DISABLE);
> +	regmap_set_bits(ps->regmap, ps->offset, APPLE_PMGR_RESET);
> +
> +	mutex_unlock(&ps->genpd.mlock);
> +
> +	return 0;
> +}
> +
> +static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
> +
> +	mutex_lock(&ps->genpd.mlock);
> +
> +	dev_dbg(ps->dev, "PS 0x%x: deassert reset\n", ps->offset);
> +	regmap_clear_bits(ps->regmap, ps->offset, APPLE_PMGR_RESET);
> +	regmap_clear_bits(ps->regmap, ps->offset, APPLE_PMGR_DEV_DISABLE);
> +
> +	if (ps->genpd.status == GENPD_STATE_OFF)
> +		dev_err(ps->dev, "PS 0x%x: RESET was deasserted while powered down\n", ps->offset);
> +
> +	mutex_unlock(&ps->genpd.mlock);
> +
> +	return 0;
> +}
> +
> +static int apple_pmgr_reset_reset(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	int ret;
> +
> +	ret = apple_pmgr_reset_assert(rcdev, id);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(APPLE_PMGR_RESET_TIME, 2 * APPLE_PMGR_RESET_TIME);

Is this delay known to be long enough for all consumers using the
reset_control_reset() functionality? Are there any users at all?

Is it ok for a genpd transition to happen during this sleep?

> +	return apple_pmgr_reset_deassert(rcdev, id);
> +}

regards
Philipp

Re: [PATCH 6/7] tty: serial: samsung_tty: Support runtime PM

[Rafael J. Wysocki]

On Wed, Oct 6, 2021 at 9:44 AM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 05/10/2021 17:59, Hector Martin wrote:
> > This allows idle UART devices to be suspended using the standard
> > runtime-PM framework. The logic is modeled after stm32-usart.
> >
> > Signed-off-by: Hector Martin <marcan@marcan.st>
> > ---
> >  drivers/tty/serial/samsung_tty.c | 88 ++++++++++++++++++++------------
> >  1 file changed, 54 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > index e2f49863e9c2..d68e3341adc6 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -40,6 +40,7 @@
> >  #include <linux/clk.h>
> >  #include <linux/cpufreq.h>
> >  #include <linux/of.h>
> > +#include <linux/pm_runtime.h>
> >  #include <asm/irq.h>
> >
> >  /* UART name and device definitions */
> > @@ -1381,31 +1382,49 @@ static void exynos_usi_init(struct uart_port *port)
> >
> >  /* power power management control */
> >
> > -static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
> > -                           unsigned int old)
> > +static int __maybe_unused s3c24xx_serial_runtime_suspend(struct device *dev)
> >  {
> > +     struct uart_port *port = dev_get_drvdata(dev);
> >       struct s3c24xx_uart_port *ourport = to_ourport(port);
> >       int timeout = 10000;
> >
> > -     ourport->pm_level = level;
> > +     while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
> > +             udelay(100);
> >
> > -     switch (level) {
> > -     case 3:
> > -             while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
> > -                     udelay(100);
> > +     if (!IS_ERR(ourport->baudclk))
> > +             clk_disable_unprepare(ourport->baudclk);
> >
> > -             if (!IS_ERR(ourport->baudclk))
> > -                     clk_disable_unprepare(ourport->baudclk);
> > +     clk_disable_unprepare(ourport->clk);
> > +     return 0;
> > +};
> >
> > -             clk_disable_unprepare(ourport->clk);
> > -             break;
> > +static int __maybe_unused s3c24xx_serial_runtime_resume(struct device *dev)
> > +{
> > +     struct uart_port *port = dev_get_drvdata(dev);
> > +     struct s3c24xx_uart_port *ourport = to_ourport(port);
> >
> > -     case 0:
> > -             clk_prepare_enable(ourport->clk);
> > +     clk_prepare_enable(ourport->clk);
> >
> > -             if (!IS_ERR(ourport->baudclk))
> > -                     clk_prepare_enable(ourport->baudclk);
> > +     if (!IS_ERR(ourport->baudclk))
> > +             clk_prepare_enable(ourport->baudclk);
> > +     return 0;
> > +};
> > +
> > +static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
> > +                           unsigned int old)
> > +{
> > +     struct s3c24xx_uart_port *ourport = to_ourport(port);
> > +
> > +     ourport->pm_level = level;
> >
> > +     switch (level) {
> > +     case UART_PM_STATE_OFF:
> > +             pm_runtime_mark_last_busy(port->dev);
> > +             pm_runtime_put_sync(port->dev);
> > +             break;
> > +
> > +     case UART_PM_STATE_ON:
> > +             pm_runtime_get_sync(port->dev);
> >               exynos_usi_init(port);
> >               break;
> >       default:
> > @@ -2282,18 +2301,15 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
> >               }
> >       }
> >
> > +     pm_runtime_get_noresume(&pdev->dev);
> > +     pm_runtime_set_active(&pdev->dev);
> > +     pm_runtime_enable(&pdev->dev);
> > +
>
> You need to cleanup in error paths (put/disable).
>
> >       dev_dbg(&pdev->dev, "%s: adding port\n", __func__);
> >       uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
> >       platform_set_drvdata(pdev, &ourport->port);
> >
> > -     /*
> > -      * Deactivate the clock enabled in s3c24xx_serial_init_port here,
> > -      * so that a potential re-enablement through the pm-callback overlaps
> > -      * and keeps the clock enabled in this case.
> > -      */
> > -     clk_disable_unprepare(ourport->clk);
> > -     if (!IS_ERR(ourport->baudclk))
> > -             clk_disable_unprepare(ourport->baudclk);
> > +     pm_runtime_put_sync(&pdev->dev);
> >
> >       ret = s3c24xx_serial_cpufreq_register(ourport);
> >       if (ret < 0)
> > @@ -2309,8 +2325,14 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
> >       struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);
> >
> >       if (port) {
> > +             pm_runtime_get_sync(&dev->dev);
>
> 1. You need to check return status.

Why?  What can be done differently if the resume fails?

> 2. Why do you need to resume the device here?

This appears to be to prevent the device from suspending after the given point.

> > +
> >               s3c24xx_serial_cpufreq_deregister(to_ourport(port));
> >               uart_remove_one_port(&s3c24xx_uart_drv, port);
> > +
> > +             pm_runtime_disable(&dev->dev);
>
> Why disabling it only if port!=NULL? Can remove() be called if
> platform_set_drvdata() was not?
>
> > +             pm_runtime_set_suspended(&dev->dev);
> > +             pm_runtime_put_noidle(&dev->dev);
> >       }
> >
> >       uart_unregister_driver(&s3c24xx_uart_drv);
> > @@ -2319,8 +2341,8 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
> >  }
> >
>
> Best regards,
> Krzysztof

Re: [PATCH 6/7] tty: serial: samsung_tty: Support runtime PM

[Krzysztof Kozlowski]

On 06/10/2021 15:25, Rafael J. Wysocki wrote:
> On Wed, Oct 6, 2021 at 9:44 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 05/10/2021 17:59, Hector Martin wrote:
>>> This allows idle UART devices to be suspended using the standard
>>> runtime-PM framework. The logic is modeled after stm32-usart.
>>>
>>> Signed-off-by: Hector Martin <marcan@marcan.st>
>>> ---
>>>  drivers/tty/serial/samsung_tty.c | 88 ++++++++++++++++++++------------
>>>  1 file changed, 54 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>>> index e2f49863e9c2..d68e3341adc6 100644
>>> --- a/drivers/tty/serial/samsung_tty.c
>>> +++ b/drivers/tty/serial/samsung_tty.c
>>> @@ -40,6 +40,7 @@
>>>  #include <linux/clk.h>
>>>  #include <linux/cpufreq.h>
>>>  #include <linux/of.h>
>>> +#include <linux/pm_runtime.h>
>>>  #include <asm/irq.h>
>>>
>>>  /* UART name and device definitions */
>>> @@ -1381,31 +1382,49 @@ static void exynos_usi_init(struct uart_port *port)
>>>
>>>  /* power power management control */
>>>
>>> -static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
>>> -                           unsigned int old)
>>> +static int __maybe_unused s3c24xx_serial_runtime_suspend(struct device *dev)
>>>  {
>>> +     struct uart_port *port = dev_get_drvdata(dev);
>>>       struct s3c24xx_uart_port *ourport = to_ourport(port);
>>>       int timeout = 10000;
>>>
>>> -     ourport->pm_level = level;
>>> +     while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
>>> +             udelay(100);
>>>
>>> -     switch (level) {
>>> -     case 3:
>>> -             while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
>>> -                     udelay(100);
>>> +     if (!IS_ERR(ourport->baudclk))
>>> +             clk_disable_unprepare(ourport->baudclk);
>>>
>>> -             if (!IS_ERR(ourport->baudclk))
>>> -                     clk_disable_unprepare(ourport->baudclk);
>>> +     clk_disable_unprepare(ourport->clk);
>>> +     return 0;
>>> +};
>>>
>>> -             clk_disable_unprepare(ourport->clk);
>>> -             break;
>>> +static int __maybe_unused s3c24xx_serial_runtime_resume(struct device *dev)
>>> +{
>>> +     struct uart_port *port = dev_get_drvdata(dev);
>>> +     struct s3c24xx_uart_port *ourport = to_ourport(port);
>>>
>>> -     case 0:
>>> -             clk_prepare_enable(ourport->clk);
>>> +     clk_prepare_enable(ourport->clk);
>>>
>>> -             if (!IS_ERR(ourport->baudclk))
>>> -                     clk_prepare_enable(ourport->baudclk);
>>> +     if (!IS_ERR(ourport->baudclk))
>>> +             clk_prepare_enable(ourport->baudclk);
>>> +     return 0;
>>> +};
>>> +
>>> +static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
>>> +                           unsigned int old)
>>> +{
>>> +     struct s3c24xx_uart_port *ourport = to_ourport(port);
>>> +
>>> +     ourport->pm_level = level;
>>>
>>> +     switch (level) {
>>> +     case UART_PM_STATE_OFF:
>>> +             pm_runtime_mark_last_busy(port->dev);
>>> +             pm_runtime_put_sync(port->dev);
>>> +             break;
>>> +
>>> +     case UART_PM_STATE_ON:
>>> +             pm_runtime_get_sync(port->dev);
>>>               exynos_usi_init(port);
>>>               break;
>>>       default:
>>> @@ -2282,18 +2301,15 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>>>               }
>>>       }
>>>
>>> +     pm_runtime_get_noresume(&pdev->dev);
>>> +     pm_runtime_set_active(&pdev->dev);
>>> +     pm_runtime_enable(&pdev->dev);
>>> +
>>
>> You need to cleanup in error paths (put/disable).
>>
>>>       dev_dbg(&pdev->dev, "%s: adding port\n", __func__);
>>>       uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
>>>       platform_set_drvdata(pdev, &ourport->port);
>>>
>>> -     /*
>>> -      * Deactivate the clock enabled in s3c24xx_serial_init_port here,
>>> -      * so that a potential re-enablement through the pm-callback overlaps
>>> -      * and keeps the clock enabled in this case.
>>> -      */
>>> -     clk_disable_unprepare(ourport->clk);
>>> -     if (!IS_ERR(ourport->baudclk))
>>> -             clk_disable_unprepare(ourport->baudclk);
>>> +     pm_runtime_put_sync(&pdev->dev);
>>>
>>>       ret = s3c24xx_serial_cpufreq_register(ourport);
>>>       if (ret < 0)
>>> @@ -2309,8 +2325,14 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
>>>       struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);
>>>
>>>       if (port) {
>>> +             pm_runtime_get_sync(&dev->dev);
>>
>> 1. You need to check return status.
> 
> Why?  What can be done differently if the resume fails?

The answer is connected with the point (2) below. If there were for
example register access here, maybe it should be simply skipped to avoid
imprecise abort...

> 
>> 2. Why do you need to resume the device here?
> 
> This appears to be to prevent the device from suspending after the given point.

Makes sense.

> 
>>> +
>>>               s3c24xx_serial_cpufreq_deregister(to_ourport(port));
>>>               uart_remove_one_port(&s3c24xx_uart_drv, port);
>>> +
>>> +             pm_runtime_disable(&dev->dev);
>>
>> Why disabling it only if port!=NULL? Can remove() be called if
>> platform_set_drvdata() was not?
>>
>>> +             pm_runtime_set_suspended(&dev->dev);
>>> +             pm_runtime_put_noidle(&dev->dev);
>>>       }
>>>
>>>       uart_unregister_driver(&s3c24xx_uart_drv);
>>> @@ -2319,8 +2341,8 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
>>>  }
>>>
>>
>> Best regards,
>> Krzysztof


Best regards,
Krzysztof

Re: [PATCH 1/7] dt-bindings: arm: apple: Add apple,pmgr binding

[Hector Martin]

On 06/10/2021 07.45, Rob Herring wrote:
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/arm/apple/apple,pmgr.example.dts:30.40-35.15: ERROR (duplicate_node_names): /example-0/soc/power-management@23b700000: Duplicate node name
> ERROR: Input tree has errors, aborting (use -f to force output)
> make[1]: *** [scripts/Makefile.lib:385: Documentation/devicetree/bindings/arm/apple/apple,pmgr.example.dt.yaml] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1441: dt_binding_check] Error 2

Argh, sorry about that. I ran the check before adding the mini-pmgr node 
to the example right before sending out the series, and of course I 
screwed it up. It'll be fixed and double checked for v2.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 1/7] dt-bindings: arm: apple: Add apple,pmgr binding

[Hector Martin]

On 06/10/2021 16.30, Krzysztof Kozlowski wrote:
> After looking at the code, there is no device for
> apple,t8103-pmgr/apple,pmgr. What is this binding about? Is there really
> a central (central as in "one device for SoC") block managing power
> which you want to model here?

The pwrstate driver binds to individual power control registers within 
the syscon node. The parent node is bound by the generic syscon driver, 
so there is no specific SoC driver for it, but I still want to include 
SoC-specific compatibles so we can have something to use for quirks if 
we run into trouble in the future.

There are two PMGRs in the Apple M1, and thus there would be two syscon 
nodes, each containing one subnode per PM domain. The devicetree in this 
series currently only instantiates one of those, though.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 1/7] dt-bindings: arm: apple: Add apple,pmgr binding

[Hector Martin]

On 06/10/2021 15.56, Krzysztof Kozlowski wrote:
>> diff --git a/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
>> new file mode 100644
>> index 000000000000..0304164e4140
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
>> @@ -0,0 +1,74 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/arm/apple/apple,pmgr.yaml#
> 
> Please don't store all Apple-related bindings in bindings/arm/apple, but
> instead group per device type like in most of other bindings. In this
> case - this looks like something close to power domain controller, so it
> should be in bindings/power/

This is a controller that, right now, is only used to instantiate device 
power management controls, but the controller itself is just a generic 
syscon device. Depending on the register range, it could conceivably 
encompass other register types (e.g. clock selects) within it, though 
I'm not sure I want to do that right now. Apple calls several of these 
different register sets as a whole a "PMGR". So I'm not sure if it 
really qualifies as "just" a power domain controller. If we want to 
restrict this to the power state portion of PMGR, then it might make 
sense to call it something more specific...

See arm/rockchip/pmu.yaml for the setup this is modeled after.

> No power-domain-cells? Why? What exactly this device is going to do?
> Maybe I'll check the driver first.... :)

It's a syscon, it does nothing on its own. All the work is done by the 
child nodes and the driver that binds to those.

>> +additionalProperties: true
> 
> additionalProperties: false

Fixed for v2.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Hector Martin]

On 06/10/2021 05.16, Mark Kettenis wrote:
> Or we drop the apple,mpgr-pwrstate and go with only SoC-specific
> compatibles from that point onwards.

I think if Apple has discrete compat breaks and several SoCs still share 
compatibility, it'd make sense to encode those as pmgr-pwrstate-v2, v3, 
etc. That way compatible SoCs can continue to benefit from 
forwards-compatible kernels, and we only end up with a hard dep on a new 
kernel for incompatible SoCs.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Hector Martin]

On 06/10/2021 09.58, Rob Herring wrote:
> On Tue, Oct 5, 2021 at 10:59 AM Hector Martin <marcan@marcan.st> wrote:
>> Future SoCs are expected to use backwards compatible registers, and the
>> "apple,pmgr-pwrstate" represents any such interfaces (possibly with
>> additional features gated by the more specific compatible), allowing
>> them to be bound without driver updates. If a backwards incompatible
>> change is introduced in future SoCs, it will require a new compatible,
>> such as "apple,pmgr-pwrstate-v2".
> 
> Is that because past SoCs used the same registers? I don't see how
> else you have any insight to what future SoCs will do.
> 
> Normally we don't do 1 node per register type bindings, so I'm a bit
> leery about doing 1 node per domain.

Yes, we can trace a pretty clear lineage from past SoCs. Plus Apple 
folks themselves have confirmed that this is an explicit policy:

https://twitter.com/stuntpants/status/1442276493669724160

Already within this SoC we have two PMGR instances with different 
register sets (one covers all devices that stay on during system sleep), 
although I am only instantiating one in our devicetree at the moment. 
And of course there is the A14, which is the same generation as the M1 
and has exactly the same register format, but a different set of domains.

Having sub-nodes describing individual power domains has some prior art 
(e.g. power/rockchip,power-controller.yaml). In that case the nodes are 
all managed by the parent node, and use the hierarchical format, but the 
hierarchical format cannot handle multi-parent nodes (which we do have, 
or at least Apple describes them that way). Since we really have no 
"top-level" implementation specifics to worry about, I think it makes 
sense to just bind to single nodes at this point, which makes the driver 
very simple since it doesn't have to perform any bookkeeping for 
multiple domains.

I realize this is all kind of "not the way things are usually done", but 
I don't want to pass up on the opportunity to have one driver last us 
multiple SoCs if we have the chance, and it's looking like it should :-)

Note that as new features are implemented (e.g. auto-PM, which I will 
add to this driver later), that also naturally lends itself to 
forwards-compat, as SoCs without those features at all simply wouldn't 
request them in the DT. In this case an "apple,auto-pm" flag would 
enable that for domains where we want it, and those that don't support 
it (or a hypothetical past SoC without the feature at all) would simply 
not use it.

>> +properties:
>> +  $nodename:
>> +    pattern: "^power-controller@[0-9a-f]+$"
> 
> Drop this and define this node in the syscon schema with a $ref to this schema.

Ack, makes sense.

>> +  apple,domain-name:
>> +    description: |
>> +      Specifies the name of the SoC device being controlled. This is used to
>> +      name the power/reset domains.
>> +    $ref: /schemas/types.yaml#/definitions/string
> 
> No other power domain binding needs this, why do you?

Because they all hardcode the domain names in the drivers for every SoC :-)

Without a name of some sort in the devicetree, all our genpds would have 
to use numeric register offsets or the like, which seems quite ugly.

> I prefer 1 complete example in the MFD schema rather than piecemeal examples.

Sure. Would we leave this schema without examples then?

> As the child nodes are memory mapped devices, size should be 1. Then
> address translation works (though Linux doesn't care (currently)).

This requires all the reg properties to also declare the reg size, right?

One thing I wonder is whether it would make sense to allow 
#power-domain-cells = <1> and then be able to declare consecutive ranges 
of related power registers in one node. This would be useful for e.g. 
the 9 UARTs, the 4 SPI controllers, the 6 MCAs, the 5 I2C controllers, 
the 5 PWM controllers, etc (which all have uniform parents and features 
and are consecutive, so could be described together). I'm not sure if 
it's worth it, thoughts?

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Hector Martin]

On 07/10/2021 00.52, Hector Martin wrote:
> I realize this is all kind of "not the way things are usually done", but
> I don't want to pass up on the opportunity to have one driver last us
> multiple SoCs if we have the chance, and it's looking like it should :-)

Addendum: just found some prior art for this. See power/pd-samsung.yaml, 
which is another single-PD binding (though in that case they put them in 
the SoC node directly, not under a syscon).

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Hector Martin]

On 06/10/2021 16.05, Krzysztof Kozlowski wrote:
>> +  IP cores belonging to a power domain should contain a
>> +  "power-domains" property that is a phandle for the
>> +  power domain node representing the domain.
> 
> Skip this last paragraph - it is obvious in usage of power domains.
> Specific bindings should not duplicate generic knowledge.

Ack, I'll drop it.

>> +properties:
>> +  $nodename:
>> +    pattern: "^power-controller@[0-9a-f]+$"
> 
> Usually we call nodes as power-domain.

I had it as that originally, but these aren't power domains. These are 
power management domains (they can clock *and* power gate separately, 
where supported) plus also do reset management. So I wasn't sure if it 
was really fair calling them "power-domain" at that point.

>> +  power-domains:
>> +    description:
>> +      Reference to parent power domains. A domain may have multiple parents,
>> +      and all will be powered up when it is powered.
> 
> How many items?

One or more (if there are none the property should not exist). I guess 
that should be encoded.

>> +
>> +  apple,domain-name:
> 
> Use existing binding "label".

Ah, I'd missed that one! I'm glad there's an existing binding for this 
already.

> Your parent schema should include this one for evaluating children.

Yup, I'll do it like that for v2.

Thanks!


-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Hector Martin]

On 06/10/2021 05.21, Mark Kettenis wrote:
>> +static const struct of_device_id apple_pmgr_ps_of_match[] = {
>> +	{ .compatible = "apple,t8103-pmgr-pwrstate" },
>> +	{ .compatible = "apple,pmgr-pwrstate" },
>> +	{}
>> +};
> 
> I think this only needs to list "apple,pmgr-pwrstate" as that is the
> one that will be present on all SoCs that is backward compatible with
> the t8103 (M1) SoC.

Ah, yeah, we don't need to bind to t8103 unless we run into a quirk 
problem. I'll remove it. Thanks!


-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Hector Martin]

On 06/10/2021 16.28, Krzysztof Kozlowski wrote:
>> +static int apple_pmgr_ps_set(struct generic_pm_domain *genpd, u32 pstate)
>> +{
>> +	int ret;
>> +	struct apple_pmgr_ps *ps = genpd_to_apple_pmgr_ps(genpd);
>> +	u32 reg;
>> +
>> +	regmap_read(ps->regmap, ps->offset, &reg);
> 
> MMIO accesses should not fail, but regmap API could fail if for example
> clk_enable() fails. In such case you will write below value based on
> random stack init. Please check the return value here.

Ack, will fix for v2 (as well as the related ones below).

>> +static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
>> +
>> +	mutex_lock(&ps->genpd.mlock);
> 
> This looks wrong: it can be a spin-lock, not mutex, so you should use
> genpd_lock.

genpd_lock() is not part of the public API, which is why I did it like 
this. This gets decided by whether the GENPD_FLAG_IRQ_SAFE flag is set, 
so it should be a mutex in this case, as that is not set.

> However now I wonder if there could be a case when a reset-controller
> consumer calls it from it's GENPD_NOTIFY_ON notifier? In such case you
> would have this lock taken.

Hm, yeah, I wonder if we'll hit that use case. Probably not, though. I 
mostly expect our drivers to only reset devices on initial probe or in 
some kind of panic recovery scenario, not while doing PM stuff.

>> +static const struct of_device_id apple_pmgr_ps_of_match[] = {
>> +	{ .compatible = "apple,t8103-pmgr-pwrstate" },
>> +	{ .compatible = "apple,pmgr-pwrstate" },
> 
> You call the device/driver "pwrstate", which it seems is "power state".
> These are not power states. These are power controllers or power
> domains. Power state is rather a state of power domain - e.g. on or
> gated. How about renaming it to power domain or pd?

It's a bit confusing. Apple calls these registers "ps" registers, which 
presumably stands for "power state". They can both clockgate and 
powergate devices (where supported), as well as enable auto-PM and also 
handle reset. So they're a bit more complex and higher level than a 
simple power domain, which is why I called the driver "pwrstate", since 
it controls the power state of a specific SoC domain or block. In fact, 
the device PM is controlled via a 4-bit power state index, though right 
now only 0, 4, 15 are used (power gated, clock gated, active). Many 
devices will not support individual power gating and would just 
clockgate at 0, and right now the driver never uses 4, but might in the 
future. If that needs to be exposed to consumers, then it'd have to be 
via genpd idle states.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Hector Martin]

On 06/10/2021 18.24, Philipp Zabel wrote:
>> +static int apple_pmgr_reset_reset(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +	int ret;
>> +
>> +	ret = apple_pmgr_reset_assert(rcdev, id);
>> +	if (ret)
>> +		return ret;
>> +
>> +	usleep_range(APPLE_PMGR_RESET_TIME, 2 * APPLE_PMGR_RESET_TIME);
> 
> Is this delay known to be long enough for all consumers using the
> reset_control_reset() functionality? Are there any users at all?

I tested this for UART and ANS outside of Linux, and found that even 
back to back register writes worked, so the 1us thing is already 
conservative. I have no idea if we'll run into some weirdo block that 
needs more time, though. If we do then this will have to be bumped or 
turned into a property.

> Is it ok for a genpd transition to happen during this sleep?

I expect consumers to call reset while the device is active; it won't 
even work without that, as the reset is synchronous and just doesn't 
take effect while clock gated (at least for UART). See the dev_err()s 
that fire when that happens.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 1/7] dt-bindings: arm: apple: Add apple,pmgr binding

[Krzysztof Kozlowski]

On 06/10/2021 17:26, Hector Martin wrote:
> On 06/10/2021 15.56, Krzysztof Kozlowski wrote:
>>> diff --git a/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
>>> new file mode 100644
>>> index 000000000000..0304164e4140
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/apple/apple,pmgr.yaml
>>> @@ -0,0 +1,74 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/arm/apple/apple,pmgr.yaml#
>>
>> Please don't store all Apple-related bindings in bindings/arm/apple, but
>> instead group per device type like in most of other bindings. In this
>> case - this looks like something close to power domain controller, so it
>> should be in bindings/power/
> 
> This is a controller that, right now, is only used to instantiate device 
> power management controls, but the controller itself is just a generic 
> syscon device. Depending on the register range, it could conceivably 
> encompass other register types (e.g. clock selects) within it, though 
> I'm not sure I want to do that right now. Apple calls several of these 
> different register sets as a whole a "PMGR". So I'm not sure if it 
> really qualifies as "just" a power domain controller. If we want to 
> restrict this to the power state portion of PMGR, then it might make 
> sense to call it something more specific...
> 
> See arm/rockchip/pmu.yaml for the setup this is modeled after.
> 

Makes sense now and actually few other designs including Samsung Exynos
have it as well.


Best regards,
Krzysztof

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Krzysztof Kozlowski]

On 06/10/2021 17:59, Hector Martin wrote:
> On 06/10/2021 16.05, Krzysztof Kozlowski wrote:
>>> +  IP cores belonging to a power domain should contain a
>>> +  "power-domains" property that is a phandle for the
>>> +  power domain node representing the domain.
>>
>> Skip this last paragraph - it is obvious in usage of power domains.
>> Specific bindings should not duplicate generic knowledge.
> 
> Ack, I'll drop it.
> 
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^power-controller@[0-9a-f]+$"
>>
>> Usually we call nodes as power-domain.
> 
> I had it as that originally, but these aren't power domains. These are 
> power management domains (they can clock *and* power gate separately, 
> where supported) plus also do reset management. So I wasn't sure if it 
> was really fair calling them "power-domain" at that point.

OK, thanks for explanation.

> 
>>> +  power-domains:
>>> +    description:
>>> +      Reference to parent power domains. A domain may have multiple parents,
>>> +      and all will be powered up when it is powered.
>>
>> How many items?
> 
> One or more (if there are none the property should not exist). I guess 
> that should be encoded.

Probably this should not go without any constraints. Are you sure it
could have more than one? It would mean more than one parent.



Best regards,
Krzysztof

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Krzysztof Kozlowski]

On Wed, 6 Oct 2021 at 17:56, Hector Martin <marcan@marcan.st> wrote:
>
> On 07/10/2021 00.52, Hector Martin wrote:
> > I realize this is all kind of "not the way things are usually done", but
> > I don't want to pass up on the opportunity to have one driver last us
> > multiple SoCs if we have the chance, and it's looking like it should :-)
>
> Addendum: just found some prior art for this. See power/pd-samsung.yaml,
> which is another single-PD binding (though in that case they put them in
> the SoC node directly, not under a syscon).

Maybe the design is actually similar. In the Exynos there is a entire
subblock managing power - called Power Management Unit (PMU). It
controls most of power-related parts, except clock gating. For example
it covers registers related to entering deep-sleep modes or power
domains. However we split this into two:
1. Actual PMU driver which controls system-level power (and provides
syscon for other drivers needing to poke its registers... eh, life).
2. Power domain driver which binds multiple devices to a small address
spaces (three registers) inside PMU address space.

The address spaces above overlap, so the (1) PMU driver takes for
example 1004_0000 - 1004_5000 and power domain devices bind to e.g.
1004_4000, 1004_4020, 1004_4040.

Best regards,
Krzysztof

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Hector Martin]

On 07/10/2021 22.12, Krzysztof Kozlowski wrote:
>>>> +  power-domains:
>>>> +    description:
>>>> +      Reference to parent power domains. A domain may have multiple parents,
>>>> +      and all will be powered up when it is powered.
>>>
>>> How many items?
>>
>> One or more (if there are none the property should not exist). I guess
>> that should be encoded.
> 
> Probably this should not go without any constraints. Are you sure it
> could have more than one? It would mean more than one parent.

Yes, at least that is the way Apple describes it in their device tree. 
As I understand it, this is basically a dependency tree of SoC blocks 
that need to be powered up/clocked for a downstream device to work. In 
other words, it's not just a pure clock/power tree, but also represents 
blocks of logic that are shared between devices. So, for example, the 
ADT has relationships like these:

SIO_BUSIF parents: (none)
SIO       parents: SIO_BUSIF
PMS_BUSIF parents: (none)
PMS       parents: (none)
AUDIO_P   parents: SIO
SIO_ADMA  parents: SIO, PMS
MCA0      parents: AUDIO_P, SIO_ADMA

That said, we know some of the data from the ADT is dodgy and doesn't 
match the true hardware (see also the dependency from SIO to SIO_BUSIF 
there, but not from PMS to PMS_BUSIF, which feels wrong), so as we learn 
more about the real relationships between these domains we'll adjust the 
devicetree to better reflect the hardware layout.

There is also the case that even if technically a downstream device 
depends on two domains (directly), the existing genpd infrastructure 
doesn't handle that automatically like it does the single domain case, 
so it ends up making sense to just have some extra domain-domain 
dependencies to keep a bunch of boilerplate manual genpd handling code 
out of device drivers.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 2/7] dt-bindings: power: Add apple,pmgr-pwrstate binding

[Hector Martin]

On 08/10/2021 16.50, Krzysztof Kozlowski wrote:
> On Wed, 6 Oct 2021 at 17:56, Hector Martin <marcan@marcan.st> wrote:
>> Addendum: just found some prior art for this. See power/pd-samsung.yaml,
>> which is another single-PD binding (though in that case they put them in
>> the SoC node directly, not under a syscon).
> 
> Maybe the design is actually similar. In the Exynos there is a entire
> subblock managing power - called Power Management Unit (PMU). It
> controls most of power-related parts, except clock gating. For example
> it covers registers related to entering deep-sleep modes or power
> domains. However we split this into two:
> 1. Actual PMU driver which controls system-level power (and provides
> syscon for other drivers needing to poke its registers... eh, life).
> 2. Power domain driver which binds multiple devices to a small address
> spaces (three registers) inside PMU address space.
> 
> The address spaces above overlap, so the (1) PMU driver takes for
> example 1004_0000 - 1004_5000 and power domain devices bind to e.g.
> 1004_4000, 1004_4020, 1004_4040.

It's similar, except Apple tends to group registers into uniform arrays, 
sometimes with gaps. They definitely do some ugly overlap stuff in their 
devicetree/OS which we'll try to avoid (e.g. the audio driver directly 
poking clock select registers...).

Here's an incomplete memory map of the PMGR-related stuff in this SoC:

2_3b00_0000:  Clocking
      0_0000:   PLLs
      4_0000:   Clock selects / dividers
         000:    86 selects x 32bit (device clocks)
         200:    8 selects x 32bit (aux clocks)
         280:    2 selects x 32bit
      4_4000:   NCOs (used for audio) (5 x one 16KiB page each)
2_3b70_0000:  PMGR
      0_0000:   Pwr-state registers
        0000:    10 controls x 64bit  (CPUs)
        0100:    143 controls x 64bit (general devices)
        0c00:    2 controls x 64bit   (SEP)
        4000:    13 controls x 64bit  (ISP)
        8000:    5 controls x 64bit   (VENC)
        c000:    7 controls x 64bit   (ANE)
      1_0000:    10 controls x 64bit  (DISP0)
      1_c000:   Power gates
          10:    74 power gates (24 bytes each?)
      3_4100:   Performance counters? (16 bytes each, big array)
      5_4000:   Secondary CPU spin-up controls

I believe the weird groupings into page-sized areas have to do with 
security, so they can map only those ranges to certain coprocessors and 
the like via the IOMMUs.

There is also a MiniPMGR that uses the same register formats, but 
different counts/offsets, at 2_3d28_0000 (this one stays up in sleep 
mode, AIUI)

So I think we won't need any overlaps, since e.g. the whole 00000-14000 
subrange is all power state controls, so a driver doing system-level 
stuff wouldn't have to care about those. Those would just be bound by 
the syscon in this patchset. I chose to use a syscon to avoid having raw 
ioremaps for each domain instance, since each one of those eats up a 
whole page of mapping AIUI (and shows up in /proc/iomem individually).

One question is whether if we need to poke at power gates directly 
(which isn't clear right now), we should have a separate top-level 
pmgr-pwrgate syscon as a parent, or just instantiate power gate subnodes 
under the same pmgr syscon at 1c000.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 6/7] tty: serial: samsung_tty: Support runtime PM

[Hector Martin]

On 06/10/2021 16.43, Krzysztof Kozlowski wrote:
> On 05/10/2021 17:59, Hector Martin wrote:
>> +	pm_runtime_get_noresume(&pdev->dev);
>> +	pm_runtime_set_active(&pdev->dev);
>> +	pm_runtime_enable(&pdev->dev);
>> +
> 
> You need to cleanup in error paths (put/disable).

There are none though, this function always returns success past this point.

>>   	if (port) {
>> +		pm_runtime_get_sync(&dev->dev);
> 
> 1. You need to check return status.
> 2. Why do you need to resume the device here?

As Rafael mentioned, this is basically disabling PM so the device is 
enabled when not bound (which seems to be expected behavior). Not sure 
what I'd do if the resume fails... this is the remove path after all, 
it's not like we're doing anything else with the device at this point.

>> +
>>   		s3c24xx_serial_cpufreq_deregister(to_ourport(port));
>>   		uart_remove_one_port(&s3c24xx_uart_drv, port);
>> +
>> +		pm_runtime_disable(&dev->dev);
> 
> Why disabling it only if port!=NULL? Can remove() be called if
> platform_set_drvdata() was not?

Good question, I'm not entirely sure why these code paths have a check 
for NULL there. They were already there, do you happen to know why? To 
me it sounds like remove would only be called if probe succeeds, at 
which point drvdata should always be set.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 6/7] tty: serial: samsung_tty: Support runtime PM

[Krzysztof Kozlowski]

On 11/10/2021 07:32, Hector Martin wrote:
>>> +
>>>   		s3c24xx_serial_cpufreq_deregister(to_ourport(port));
>>>   		uart_remove_one_port(&s3c24xx_uart_drv, port);
>>> +
>>> +		pm_runtime_disable(&dev->dev);
>>
>> Why disabling it only if port!=NULL? Can remove() be called if
>> platform_set_drvdata() was not?
> 
> Good question, I'm not entirely sure why these code paths have a check 
> for NULL there. They were already there, do you happen to know why? To 
> me it sounds like remove would only be called if probe succeeds, at 
> which point drvdata should always be set.
> 

Exactly, anyway it is not part of your patch, so no problem.


Best regards,
Krzysztof

Re: [PATCH 6/7] tty: serial: samsung_tty: Support runtime PM

[Johan Hovold]

On Mon, Oct 11, 2021 at 02:32:29PM +0900, Hector Martin wrote:
> On 06/10/2021 16.43, Krzysztof Kozlowski wrote:
> > On 05/10/2021 17:59, Hector Martin wrote:

> >>   	if (port) {
> >> +		pm_runtime_get_sync(&dev->dev);
> > 
> > 1. You need to check return status.
> > 2. Why do you need to resume the device here?
> 
> As Rafael mentioned, this is basically disabling PM so the device is 
> enabled when not bound (which seems to be expected behavior). Not sure 
> what I'd do if the resume fails... this is the remove path after all, 
> it's not like we're doing anything else with the device at this point.

You still need to disable the clocks before returning from remove().
Consider what happens when the driver is rebound otherwise.

Johan