[PATCH 0/5] Driver support for RZ/V2M PWC

[PATCH 0/5] Driver support for RZ/V2M PWC

[Fabrizio Castro]

The PWC IP found in the RZ/V2M family of chips fits the Multi-Function
Device (MFD) model quite well, and comes with the below capabilities:
* external power supply on/off sequence generation
* on/off signal generation for the LPDDR4 core power supply (LPVDD)
* key input signals processing
* general-purpose output pins

This series introduces a driver to address GPIO support, and a driver
to address power off support, alongside the corresponding dt-bindings.

Thanks,
Fab

Fabrizio Castro (5):
  dt-bindings: gpio: Add RZ/V2M PWC GPIO driver bindings
  dt-bindings: power: reset: Add RZ/V2M PWC Power OFF bindings
  dt-bindings: mfd: Add RZ/V2M PWC global registers bindings
  gpio: Add support for Renesas RZ/V2M PWC
  power: reset: Add new driver for RZ/V2M PWC poweroff

 .../bindings/gpio/renesas,rzv2m-pwc-gpio.yaml |  62 +++++++++
 .../bindings/mfd/renesas,rzv2m-pwc.yaml       |  70 ++++++++++
 .../reset/renesas,rzv2m-pwc-poweroff.yaml     |  48 +++++++
 drivers/gpio/Kconfig                          |   8 ++
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-rzv2m-pwc.c                 | 123 ++++++++++++++++++
 drivers/power/reset/Kconfig                   |  10 ++
 drivers/power/reset/Makefile                  |   1 +
 drivers/power/reset/rzv2m-pwc-poweroff.c      |  81 ++++++++++++
 9 files changed, 404 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml
 create mode 100644 Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-poweroff.yaml
 create mode 100644 drivers/gpio/gpio-rzv2m-pwc.c
 create mode 100644 drivers/power/reset/rzv2m-pwc-poweroff.c

-- 
2.34.1

[PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver bindings

[Fabrizio Castro]

Add dt-bindings document for the RZ/V2M PWC GPIO driver.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 .../bindings/gpio/renesas,rzv2m-pwc-gpio.yaml | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
new file mode 100644
index 000000000000..ecc034d53259
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/renesas,rzv2m-pwc-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/V2M External Power Sequence Controller (PWC) GPIO
+
+description: |+
+  The PWC IP found in the RZ/V2M family of chips comes with General-Purpose
+  Output pins, alongside the below functions
+    - external power supply on/off sequence generation
+    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
+    - key input signals processing
+  This node uses syscon to map the register used to control the GPIOs
+  (the register map is retrieved from the parent dt-node), and the node should
+  be represented as a sub node of a "syscon", "simple-mfd" node.
+
+maintainers:
+  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,r9a09g011-pwc-gpio # RZ/V2M
+          - renesas,r9a09g055-pwc-gpio # RZ/V2MA
+      - const: renesas,rzv2m-pwc-gpio
+
+  offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Offset in the register map for controlling the GPIOs (in bytes).
+
+  regmap:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Phandle to the register map node.
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+required:
+  - compatible
+  - regmap
+  - offset
+  - gpio-controller
+  - '#gpio-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio {
+            compatible = "renesas,r9a09g011-pwc-gpio",
+                         "renesas,rzv2m-pwc-gpio";
+            regmap = <&regmapnode>;
+            offset = <0x80>;
+            gpio-controller;
+            #gpio-cells = <2>;
+    };
-- 
2.34.1

[PATCH 2/5] dt-bindings: power: reset: Add RZ/V2M PWC Power OFF bindings

[Fabrizio Castro]

Add dt-bindings document for the RZ/V2M PWC Power OFF driver.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 .../reset/renesas,rzv2m-pwc-poweroff.yaml     | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-poweroff.yaml

diff --git a/Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-poweroff.yaml
new file mode 100644
index 000000000000..12456e3e93e6
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-poweroff.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/reset/renesas,rzv2m-pwc-poweroff.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/V2M External Power Sequence Controller (PWC) Power OFF
+
+description: |+
+  The PWC IP found in the RZ/V2M family of chips comes with the below
+  capabilities
+    - external power supply on/off sequence generation
+    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
+    - key input signals processing
+    - general-purpose output pins
+  This node uses syscon to map the registers relevant to Power OFF (the
+  register map is retrieved from the parent dt-node), and the node should be
+  represented as a sub node of a "syscon", "simple-mfd" node.
+
+maintainers:
+  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,r9a09g011-pwc-poweroff # RZ/V2M
+          - renesas,r9a09g055-pwc-poweroff # RZ/V2MA
+      - const: renesas,rzv2m-pwc-poweroff
+
+  regmap:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      Phandle to the register map node.
+
+required:
+  - compatible
+  - regmap
+
+additionalProperties: false
+
+examples:
+  - |
+    poweroff {
+            compatible = "renesas,r9a09g011-pwc-poweroff",
+                         "renesas,rzv2m-pwc-poweroff";
+            regmap = <&regmapnode>;
+    };
-- 
2.34.1

[PATCH 3/5] dt-bindings: mfd: Add RZ/V2M PWC global registers bindings

[Fabrizio Castro]

The RZ/V2M PWC is a multi-function device, and its software
support relies on "syscon" and "simple-mfd".
Add the dt-bindings for the top level device tree node.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 .../bindings/mfd/renesas,rzv2m-pwc.yaml       | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml

diff --git a/Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml b/Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml
new file mode 100644
index 000000000000..a7e180bfbd83
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/renesas,rzv2m-pwc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/V2M External Power Sequence Controller (PWC)
+
+description: |+
+  The PWC IP found in the RZ/V2M family of chips comes with the below
+  capabilities
+    - external power supply on/off sequence generation
+    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
+    - key input signals processing
+    - general-purpose output pins
+
+maintainers:
+  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,r9a09g011-pwc # RZ/V2M
+          - renesas,r9a09g055-pwc # RZ/V2MA
+      - const: renesas,rzv2m-pwc
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+  gpio:
+    type: object
+    $ref: /schemas/gpio/renesas,rzv2m-pwc-gpio.yaml#
+    description: General-Purpose Output pins controller.
+
+  poweroff:
+    type: object
+    $ref: /schemas/power/reset/renesas,rzv2m-pwc-poweroff.yaml#
+    description: Power OFF controller.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    pwc: pwc@a3700000 {
+            compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc", "syscon",
+                         "simple-mfd";
+            reg = <0xa3700000 0x800>;
+
+            gpio {
+                    compatible = "renesas,r9a09g011-pwc-gpio",
+                                 "renesas,rzv2m-pwc-gpio";
+                    regmap = <&pwc>;
+                    offset = <0x80>;
+                    gpio-controller;
+                    #gpio-cells = <2>;
+            };
+
+            poweroff {
+                    compatible = "renesas,r9a09g011-pwc-poweroff",
+                                 "renesas,rzv2m-pwc-poweroff";
+                    regmap = <&pwc>;
+            };
+    };
-- 
2.34.1

[PATCH 4/5] gpio: Add support for Renesas RZ/V2M PWC

[Fabrizio Castro]

The RZ/V2M SoC contains an External Power Sequence Controller (PWC)
module. This module provides an external power supply on/off sequence,
on/off signal for the LPDDR4 core power supply, control signals for
external I/O power supplies of the SD host interfaces, and key input
signals.
PWC is essentially a Multi-Function Device (MFD).

The driver just implements the control signals for external I/O
power supplies of the SD host interfaces as gpios, and it relies on
syscon and simple-mfd.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/gpio/Kconfig          |   8 +++
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpio-rzv2m-pwc.c | 123 ++++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+)
 create mode 100644 drivers/gpio/gpio-rzv2m-pwc.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e6ebc4c90a5d..e016919b9643 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -553,6 +553,14 @@ config GPIO_ROCKCHIP
 	help
 	  Say yes here to support GPIO on Rockchip SoCs.
 
+config GPIO_RZV2M_PWC
+	tristate "Renesas RZ/V2M PWC GPIO support"
+	depends on MFD_SYSCON
+	depends on ARCH_R9A09G011 || COMPILE_TEST
+	help
+	  Say yes here to support the External Power Sequence Controller (PWC)
+	  GPIO controller driver for RZ/V2M devices.
+
 config GPIO_SAMA5D2_PIOBU
 	tristate "SAMA5D2 PIOBU GPIO support"
 	depends on MFD_SYSCON
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 3462a138764a..5f655684603f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -132,6 +132,7 @@ obj-$(CONFIG_GPIO_RDC321X)		+= gpio-rdc321x.o
 obj-$(CONFIG_GPIO_REALTEK_OTTO)		+= gpio-realtek-otto.o
 obj-$(CONFIG_GPIO_REG)			+= gpio-reg.o
 obj-$(CONFIG_GPIO_ROCKCHIP)	+= gpio-rockchip.o
+obj-$(CONFIG_GPIO_RZV2M_PWC)		+= gpio-rzv2m-pwc.o
 obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
 obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
diff --git a/drivers/gpio/gpio-rzv2m-pwc.c b/drivers/gpio/gpio-rzv2m-pwc.c
new file mode 100644
index 000000000000..672d868cb8c9
--- /dev/null
+++ b/drivers/gpio/gpio-rzv2m-pwc.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Renesas Electronics Corporation
+ *
+ * GPIO driver for Renesas RZ/V2M External Power Sequence Controller (PWC)
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+
+struct rzv2m_pwc_gpio_priv {
+	struct gpio_chip gp;
+	int offset;
+	struct regmap *regmap;
+	DECLARE_BITMAP(ch_en_bits, 2);
+};
+
+static void rzv2m_pwc_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			       int value)
+{
+	struct rzv2m_pwc_gpio_priv *priv = gpiochip_get_data(chip);
+	u32 reg;
+
+	/* BIT 16 enables write to BIT 0, and BIT 17 enables write to BIT 1 */
+	reg = BIT(offset + 16);
+	if (value)
+		reg |= BIT(offset);
+
+	regmap_write(priv->regmap, priv->offset, reg);
+
+	if (value)
+		set_bit(offset, priv->ch_en_bits);
+	else
+		clear_bit(offset, priv->ch_en_bits);
+}
+
+static int rzv2m_pwc_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rzv2m_pwc_gpio_priv *priv = gpiochip_get_data(chip);
+
+	return test_bit(offset, priv->ch_en_bits);
+}
+
+static int rzv2m_pwc_gpio_direction_output(struct gpio_chip *gc,
+					   unsigned int nr, int value)
+{
+	if (nr > 1)
+		return -EINVAL;
+
+	rzv2m_pwc_gpio_set(gc, nr, value);
+
+	return 0;
+}
+
+static const struct gpio_chip rzv2m_pwc_gc = {
+	.label = "rzv2m_pwc_gpio",
+	.owner = THIS_MODULE,
+	.get = rzv2m_pwc_gpio_get,
+	.set = rzv2m_pwc_gpio_set,
+	.direction_output = rzv2m_pwc_gpio_direction_output,
+	.can_sleep = false,
+	.ngpio = 2,
+	.base = -1,
+};
+
+static int rzv2m_pwc_gpio_probe(struct platform_device *pdev)
+{
+	struct rzv2m_pwc_gpio_priv *priv;
+	int err;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+						       "regmap");
+
+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->regmap),
+				     "Can't find regmap property");
+
+	err = of_property_read_u32(pdev->dev.of_node, "offset", &priv->offset);
+	if (err)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "Can't find offset property");
+
+	/*
+	 * The register used by this driver cannot be read, therefore set the
+	 * outputs to their default values and initialize priv->ch_en_bits accordingly.
+	 * BIT 16 enables write to BIT 0, BIT 17 enables write to BIT 1, and the
+	 * default value of both BIT 0 and BIT 1 is 0.
+	 */
+	regmap_write(priv->regmap, priv->offset, BIT(17) | BIT(16));
+	bitmap_zero(priv->ch_en_bits, 2);
+
+	priv->gp = rzv2m_pwc_gc;
+	priv->gp.parent = pdev->dev.parent;
+	priv->gp.fwnode = dev_fwnode(&pdev->dev);
+
+	return devm_gpiochip_add_data(&pdev->dev, &priv->gp, priv);
+}
+
+static const struct of_device_id rzv2m_pwc_gpio_of_match[] = {
+	{ .compatible = "renesas,rzv2m-pwc-gpio" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzv2m_pwc_gpio_of_match);
+
+static struct platform_driver rzv2m_pwc_gpio_driver = {
+	.probe = rzv2m_pwc_gpio_probe,
+	.driver = {
+		.name = "rzv2m_pwc_gpio",
+		.of_match_table = of_match_ptr(rzv2m_pwc_gpio_of_match),
+	},
+};
+module_platform_driver(rzv2m_pwc_gpio_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Fabrizio Castro <castro.fabrizio.jz@renesas.com>");
+MODULE_DESCRIPTION("Renesas RZ/V2M PWC GPIO");
-- 
2.34.1

[PATCH 5/5] power: reset: Add new driver for RZ/V2M PWC poweroff

[Fabrizio Castro]

The RZ/V2M PWC IP controls external power supplies and therefore
can turn the power supplies off when powering down the system.

PWC is essentially a Multi-Function Device (MFD), and this driver
relies on syscon and simple-mfd to integrate within the larger
scheme of things.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/power/reset/Kconfig              | 10 +++
 drivers/power/reset/Makefile             |  1 +
 drivers/power/reset/rzv2m-pwc-poweroff.c | 81 ++++++++++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100644 drivers/power/reset/rzv2m-pwc-poweroff.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index a8c46ba5878f..9f7c9ed1a36e 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -303,4 +303,14 @@ config POWER_MLXBF
 	help
 	  This driver supports reset or low power mode handling for Mellanox BlueField.
 
+config POWER_RESET_RZV2M_PWC
+	tristate "Renesas RZ/V2M PWC Power OFF"
+	depends on MFD_SYSCON
+	depends on ARCH_R9A09G011 || COMPILE_TEST
+	help
+	  The RZ/V2M PWC IP controls external power supplies and therefore can
+	  turn the power supplies off when powering down the system.
+	  Enable this driver when PWC is in control of the system power supplies
+	  and it's the preferred way to shutdown the system.
+
 endif
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 0a39424fc558..f05a8abff2eb 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -36,3 +36,4 @@ obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
 obj-$(CONFIG_POWER_RESET_SC27XX) += sc27xx-poweroff.o
 obj-$(CONFIG_NVMEM_REBOOT_MODE) += nvmem-reboot-mode.o
 obj-$(CONFIG_POWER_MLXBF) += pwr-mlxbf.o
+obj-$(CONFIG_POWER_RESET_RZV2M_PWC) += rzv2m-pwc-poweroff.o
diff --git a/drivers/power/reset/rzv2m-pwc-poweroff.c b/drivers/power/reset/rzv2m-pwc-poweroff.c
new file mode 100644
index 000000000000..e9bd16e65b6a
--- /dev/null
+++ b/drivers/power/reset/rzv2m-pwc-poweroff.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Renesas Electronics Corporation
+ *
+ * Reset driver for Renesas RZ/V2M External Power Sequence Controller (PWC)
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/regmap.h>
+
+#define PWC_PWCRST			0x00
+#define PWC_PWCCKEN			0x04
+#define PWC_PWCCTL			0x50
+
+#define PWC_PWCRST_RSTSOFTAX		0x1
+#define PWC_PWCCKEN_ENGCKMAIN		0x1
+#define PWC_PWCCTL_PWOFF		0x1
+
+struct rzv2m_pwc_poweroff_priv {
+	struct regmap *regmap;
+	struct device *dev;
+};
+
+static int rzv2m_pwc_poweroff(struct sys_off_data *data)
+{
+	struct rzv2m_pwc_poweroff_priv *priv =
+		(struct rzv2m_pwc_poweroff_priv *)data->cb_data;
+
+	regmap_write(priv->regmap, PWC_PWCRST, PWC_PWCRST_RSTSOFTAX);
+	regmap_write(priv->regmap, PWC_PWCCKEN, PWC_PWCCKEN_ENGCKMAIN);
+	regmap_write(priv->regmap, PWC_PWCCTL, PWC_PWCCTL_PWOFF);
+
+	mdelay(150);
+
+	dev_err(priv->dev, "Failed to power off the system");
+
+	return NOTIFY_DONE;
+}
+
+static int rzv2m_pwc_poweroff_probe(struct platform_device *pdev)
+{
+	struct rzv2m_pwc_poweroff_priv *priv;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+						       "regmap");
+
+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->regmap),
+				     "Can't find regmap property");
+
+	priv->dev = &pdev->dev;
+
+	return devm_register_power_off_handler(&pdev->dev, rzv2m_pwc_poweroff,
+					       priv);
+}
+
+static const struct of_device_id rzv2m_pwc_poweroff_of_match[] = {
+	{ .compatible = "renesas,rzv2m-pwc-poweroff" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzv2m_pwc_poweroff_of_match);
+
+static struct platform_driver rzv2m_pwc_poweroff_driver = {
+	.probe = rzv2m_pwc_poweroff_probe,
+	.driver = {
+		.name = "rzv2m_pwc_poweroff",
+		.of_match_table = of_match_ptr(rzv2m_pwc_poweroff_of_match),
+	},
+};
+module_platform_driver(rzv2m_pwc_poweroff_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Fabrizio Castro <castro.fabrizio.jz@renesas.com>");
+MODULE_DESCRIPTION("Renesas RZ/V2M PWC power OFF driver");
-- 
2.34.1

Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver bindings

[Rob Herring]

On Tue, Dec 13, 2022 at 10:43:06PM +0000, Fabrizio Castro wrote:
> Add dt-bindings document for the RZ/V2M PWC GPIO driver.

Bindings are for h/w blocks/devices, not a specific driver.

> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> ---
>  .../bindings/gpio/renesas,rzv2m-pwc-gpio.yaml | 62 +++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
> new file mode 100644
> index 000000000000..ecc034d53259
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/renesas,rzv2m-pwc-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/V2M External Power Sequence Controller (PWC) GPIO
> +
> +description: |+
> +  The PWC IP found in the RZ/V2M family of chips comes with General-Purpose
> +  Output pins, alongside the below functions
> +    - external power supply on/off sequence generation
> +    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
> +    - key input signals processing
> +  This node uses syscon to map the register used to control the GPIOs
> +  (the register map is retrieved from the parent dt-node), and the node should
> +  be represented as a sub node of a "syscon", "simple-mfd" node.
> +
> +maintainers:
> +  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - renesas,r9a09g011-pwc-gpio # RZ/V2M
> +          - renesas,r9a09g055-pwc-gpio # RZ/V2MA
> +      - const: renesas,rzv2m-pwc-gpio
> +
> +  offset:

Too generic of a name. We want any given property name (globally) to 
have 1 type. With the below comment, this should be replaced with 'reg' 
instead if you have child nodes.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Offset in the register map for controlling the GPIOs (in bytes).
> +
> +  regmap:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle to the register map node.

Looks like GPIO is a sub-function of some other block. Define the 
binding for that entire block. GPIO can be either either a function of 
that node (just add GPIO provider properties) or you can have GPIO child 
nodes. Depends on what the entire block looks like to decide. Do you 
have multiple instances of the GPIO block would be one reason to have 
child nodes.

> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +required:
> +  - compatible
> +  - regmap
> +  - offset
> +  - gpio-controller
> +  - '#gpio-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio {
> +            compatible = "renesas,r9a09g011-pwc-gpio",
> +                         "renesas,rzv2m-pwc-gpio";
> +            regmap = <&regmapnode>;
> +            offset = <0x80>;
> +            gpio-controller;
> +            #gpio-cells = <2>;
> +    };
> -- 
> 2.34.1
> 
> 

Re: [PATCH 3/5] dt-bindings: mfd: Add RZ/V2M PWC global registers bindings

[Rob Herring]

On Tue, Dec 13, 2022 at 10:43:08PM +0000, Fabrizio Castro wrote:
> The RZ/V2M PWC is a multi-function device, and its software
> support relies on "syscon" and "simple-mfd".
> Add the dt-bindings for the top level device tree node.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> ---
>  .../bindings/mfd/renesas,rzv2m-pwc.yaml       | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml b/Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml
> new file mode 100644
> index 000000000000..a7e180bfbd83
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/renesas,rzv2m-pwc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/V2M External Power Sequence Controller (PWC)
> +
> +description: |+
> +  The PWC IP found in the RZ/V2M family of chips comes with the below
> +  capabilities
> +    - external power supply on/off sequence generation
> +    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
> +    - key input signals processing
> +    - general-purpose output pins
> +
> +maintainers:
> +  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - renesas,r9a09g011-pwc # RZ/V2M
> +          - renesas,r9a09g055-pwc # RZ/V2MA
> +      - const: renesas,rzv2m-pwc
> +      - const: syscon
> +      - const: simple-mfd
> +
> +  reg:
> +    maxItems: 1
> +
> +  gpio:
> +    type: object
> +    $ref: /schemas/gpio/renesas,rzv2m-pwc-gpio.yaml#
> +    description: General-Purpose Output pins controller.
> +
> +  poweroff:
> +    type: object
> +    $ref: /schemas/power/reset/renesas,rzv2m-pwc-poweroff.yaml#
> +    description: Power OFF controller.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pwc: pwc@a3700000 {
> +            compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc", "syscon",
> +                         "simple-mfd";
> +            reg = <0xa3700000 0x800>;
> +
> +            gpio {
> +                    compatible = "renesas,r9a09g011-pwc-gpio",
> +                                 "renesas,rzv2m-pwc-gpio";
> +                    regmap = <&pwc>;
> +                    offset = <0x80>;
> +                    gpio-controller;
> +                    #gpio-cells = <2>;
> +            };
> +
> +            poweroff {
> +                    compatible = "renesas,r9a09g011-pwc-poweroff",
> +                                 "renesas,rzv2m-pwc-poweroff";
> +                    regmap = <&pwc>;

Why does this need to be a child node? There aren't any resources for 
it. 'regmap' is just the parent node.

Assuming this binding is complete, I don't think you need any child 
nodes. A single node can have multiple providers.

Rob

RE: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver bindings

[Fabrizio Castro]

Hi Rob,

Thanks for your feedback!

> From: Rob Herring <robh@kernel.org>
> Sent: 14 December 2022 16:11
> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver
> bindings
> 
> On Tue, Dec 13, 2022 at 10:43:06PM +0000, Fabrizio Castro wrote:
> > Add dt-bindings document for the RZ/V2M PWC GPIO driver.
> 
> Bindings are for h/w blocks/devices, not a specific driver.

Apologies, I will reword the changelog in v2.

> 
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > ---
> >  .../bindings/gpio/renesas,rzv2m-pwc-gpio.yaml | 62 +++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
> gpio.yaml b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
> gpio.yaml
> > new file mode 100644
> > index 000000000000..ecc034d53259
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
> > @@ -0,0 +1,62 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre
> e.org%2Fschemas%2Fgpio%2Frenesas%2Crzv2m-pwc-
> gpio.yaml%23&amp;data=05%7C01%7Cfabrizio.castro.jz%40renesas.com%7C603623c
> 766f4421b85bd08daddedcb8c%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638
> 066310628408926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz
> IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=o46ncDZK8YK5HYJ
> ZYDXuq3yfEA34vnaxEsIDBlcroc0%3D&amp;reserved=0
> > +$schema:
> https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre
> e.org%2Fmeta-
> schemas%2Fcore.yaml%23&amp;data=05%7C01%7Cfabrizio.castro.jz%40renesas.com
> %7C603623c766f4421b85bd08daddedcb8c%7C53d82571da1947e49cb4625a166a4a2a%7C0
> %7C0%7C638066310628408926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=VoWvV
> pW782DVH2zdTKIesyzqm6sjiFyacbl833%2BjRis%3D&amp;reserved=0
> > +
> > +title: Renesas RZ/V2M External Power Sequence Controller (PWC) GPIO
> > +
> > +description: |+
> > +  The PWC IP found in the RZ/V2M family of chips comes with General-
> Purpose
> > +  Output pins, alongside the below functions
> > +    - external power supply on/off sequence generation
> > +    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
> > +    - key input signals processing
> > +  This node uses syscon to map the register used to control the GPIOs
> > +  (the register map is retrieved from the parent dt-node), and the node
> should
> > +  be represented as a sub node of a "syscon", "simple-mfd" node.
> > +
> > +maintainers:
> > +  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - renesas,r9a09g011-pwc-gpio # RZ/V2M
> > +          - renesas,r9a09g055-pwc-gpio # RZ/V2MA
> > +      - const: renesas,rzv2m-pwc-gpio
> > +
> > +  offset:
> 
> Too generic of a name. We want any given property name (globally) to
> have 1 type. With the below comment, this should be replaced with 'reg'
> instead if you have child nodes.

My understanding is that syscon subnodes normally use this name for exactly
the same purpose, for example:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/power/reset/syscon-poweroff.yaml#L27
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml#L30

What am I missing?

> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      Offset in the register map for controlling the GPIOs (in bytes).
> > +
> > +  regmap:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Phandle to the register map node.
> 
> Looks like GPIO is a sub-function of some other block. Define the
> binding for that entire block.

That's defined in patch 3 from this series.
I have sent it as patch 3 because that document references:
* /schemas/gpio/renesas,rzv2m-pwc-gpio.yaml
* /schemas/power/reset/renesas,rzv2m-pwc-poweroff.yaml
Which are defined in this patch and in patch 2 in the series.

Do you want me to move patch 3 to patch 1 in v2?

> GPIO can be either either a function of
> that node (just add GPIO provider properties) or you can have GPIO child
> nodes. Depends on what the entire block looks like to decide. Do you
> have multiple instances of the GPIO block would be one reason to have
> child nodes.

From a pure HW point of view, this GPIO block is contained inside the PWC block
(as PWC is basically a MFD device), and it only deals with 2 General-Purpose
Output pins, both controlled by 1 (and the same) register, therefore the GPIO
block is only 1 child.

If possible, I would like to keep the functionality offered by the sub-blocks of
PWC contained in separated drivers and DT nodes (either non-child nodes or child
nodes).

My understanding is that simple-mfd will automatically probe the children of the
simple-mfd node, and also hierarchically it makes sense to me to have the DT nodes
of the PWC sub-blocks as children of the "syscon", "simple-mfd" node. I have found
other instances of this same architecture in the kernel already (plenty from NXP/Freescale),
for example:
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mm.dtsi#L585
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mn.dtsi#L586
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mp.dtsi#L451
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mq.dtsi#L616
https://github.com/torvalds/linux/blob/master/arch/mips/boot/dts/mti/sead3.dts#L93
etc...

Something like the below could also work, but I don't think it would represent the
HW accurately:
pwc: pwc@a3700000 {
	compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc",
		     "syscon", "simple-mfd";
	reg = <0 0xa3700000 0 0x800>;
};

pwc-gpio {
	compatible = "renesas,r9a09g011-pwc-gpio",
		     "renesas,rzv2m-pwc-gpio";
	regmap = <&pwc>;
	gpio-controller;
	#gpio-cells = <2>;
};

pwc-poweroff {
	compatible = "renesas,r9a09g011-pwc-poweroff",
		     "renesas,rzv2m-pwc-poweroff";
	regmap = <&pwc>;
};


I think the below describes things better:
pwc: pwc@a3700000 {
	compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc",
		     "syscon", "simple-mfd";
	reg = <0 0xa3700000 0 0x800>;

	gpio {
		compatible = "renesas,r9a09g011-pwc-gpio",
			     "renesas,rzv2m-pwc-gpio";
		regmap = <&pwc>;
		offset = <0x80>;
		gpio-controller;
		#gpio-cells = <2>;
	};

	poweroff {
		compatible = "renesas,r9a09g011-pwc-poweroff",
			     "renesas,rzv2m-pwc-poweroff";
		regmap = <&pwc>;
	};
};

Do you think the bindings I have sent out are causing confusion here?
What else can I improve?

Thanks,
Fab

> 
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +required:
> > +  - compatible
> > +  - regmap
> > +  - offset
> > +  - gpio-controller
> > +  - '#gpio-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    gpio {
> > +            compatible = "renesas,r9a09g011-pwc-gpio",
> > +                         "renesas,rzv2m-pwc-gpio";
> > +            regmap = <&regmapnode>;
> > +            offset = <0x80>;
> > +            gpio-controller;
> > +            #gpio-cells = <2>;
> > +    };
> > --
> > 2.34.1
> >
> >

Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver bindings

[Krzysztof Kozlowski]

On 14/12/2022 19:26, Fabrizio Castro wrote:
> Hi Rob,
> 
> Thanks for your feedback!
> 
>> From: Rob Herring <robh@kernel.org>
>> Sent: 14 December 2022 16:11
>> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
>> Subject: Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver
>> bindings
>>
>> On Tue, Dec 13, 2022 at 10:43:06PM +0000, Fabrizio Castro wrote:
>>> Add dt-bindings document for the RZ/V2M PWC GPIO driver.
>>
>> Bindings are for h/w blocks/devices, not a specific driver.
> 
> Apologies, I will reword the changelog in v2.
> 
>>
>>>
>>> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
>>> ---
>>>  .../bindings/gpio/renesas,rzv2m-pwc-gpio.yaml | 62 +++++++++++++++++++
>>>  1 file changed, 62 insertions(+)
>>>  create mode 100644
>> Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
>> gpio.yaml b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
>> gpio.yaml
>>> new file mode 100644
>>> index 000000000000..ecc034d53259
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
>>> @@ -0,0 +1,62 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id:
>> https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre
>> e.org%2Fschemas%2Fgpio%2Frenesas%2Crzv2m-pwc-
>> gpio.yaml%23&amp;data=05%7C01%7Cfabrizio.castro.jz%40renesas.com%7C603623c
>> 766f4421b85bd08daddedcb8c%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638
>> 066310628408926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz
>> IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=o46ncDZK8YK5HYJ
>> ZYDXuq3yfEA34vnaxEsIDBlcroc0%3D&amp;reserved=0
>>> +$schema:
>> https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre
>> e.org%2Fmeta-
>> schemas%2Fcore.yaml%23&amp;data=05%7C01%7Cfabrizio.castro.jz%40renesas.com
>> %7C603623c766f4421b85bd08daddedcb8c%7C53d82571da1947e49cb4625a166a4a2a%7C0
>> %7C0%7C638066310628408926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
>> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=VoWvV
>> pW782DVH2zdTKIesyzqm6sjiFyacbl833%2BjRis%3D&amp;reserved=0
>>> +
>>> +title: Renesas RZ/V2M External Power Sequence Controller (PWC) GPIO
>>> +
>>> +description: |+
>>> +  The PWC IP found in the RZ/V2M family of chips comes with General-
>> Purpose
>>> +  Output pins, alongside the below functions
>>> +    - external power supply on/off sequence generation
>>> +    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
>>> +    - key input signals processing
>>> +  This node uses syscon to map the register used to control the GPIOs
>>> +  (the register map is retrieved from the parent dt-node), and the node
>> should
>>> +  be represented as a sub node of a "syscon", "simple-mfd" node.
>>> +
>>> +maintainers:
>>> +  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - renesas,r9a09g011-pwc-gpio # RZ/V2M
>>> +          - renesas,r9a09g055-pwc-gpio # RZ/V2MA
>>> +      - const: renesas,rzv2m-pwc-gpio
>>> +
>>> +  offset:
>>
>> Too generic of a name. We want any given property name (globally) to
>> have 1 type. With the below comment, this should be replaced with 'reg'
>> instead if you have child nodes.
> 
> My understanding is that syscon subnodes normally use this name for exactly
> the same purpose, for example:
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/power/reset/syscon-poweroff.yaml#L27
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml#L30
> 
> What am I missing?

These are generic drivers, so they need offset as they do not match any
specific programming model. You are not making a generic device. Address
offsets are not suitable in most cases for DTS. There are of course
exceptions so you can present reasons why this one is exception.
> 
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description: |
>>> +      Offset in the register map for controlling the GPIOs (in bytes).
>>> +
>>> +  regmap:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: Phandle to the register map node.
>>
>> Looks like GPIO is a sub-function of some other block. Define the
>> binding for that entire block.
> 
> That's defined in patch 3 from this series.
> I have sent it as patch 3 because that document references:
> * /schemas/gpio/renesas,rzv2m-pwc-gpio.yaml
> * /schemas/power/reset/renesas,rzv2m-pwc-poweroff.yaml
> Which are defined in this patch and in patch 2 in the series.
> 
> Do you want me to move patch 3 to patch 1 in v2?

We do not want regmap, but proper definition of entire hardware.

> 
>> GPIO can be either either a function of
>> that node (just add GPIO provider properties) or you can have GPIO child
>> nodes. Depends on what the entire block looks like to decide. Do you
>> have multiple instances of the GPIO block would be one reason to have
>> child nodes.
> 
> From a pure HW point of view, this GPIO block is contained inside the PWC block
> (as PWC is basically a MFD device), and it only deals with 2 General-Purpose
> Output pins, both controlled by 1 (and the same) register, therefore the GPIO
> block is only 1 child.
> 
> If possible, I would like to keep the functionality offered by the sub-blocks of
> PWC contained in separated drivers and DT nodes (either non-child nodes or child
> nodes).

Driver do not matter for bindings. We talk about regmap field which - as
you explained above - is not needed.


> 
> My understanding is that simple-mfd will automatically probe the children of the
> simple-mfd node, and also hierarchically it makes sense to me to have the DT nodes
> of the PWC sub-blocks as children of the "syscon", "simple-mfd" node. I have found
> other instances of this same architecture in the kernel already (plenty from NXP/Freescale),
> for example:

I don't understand. You do not have here simple-mfd and it still does
not explain Rob's comment and regmap.

> https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mm.dtsi#L585
> https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mn.dtsi#L586
> https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mp.dtsi#L451
> https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mq.dtsi#L616
> https://github.com/torvalds/linux/blob/master/arch/mips/boot/dts/mti/sead3.dts#L93
> etc...
> 
> Something like the below could also work, but I don't think it would represent the
> HW accurately:
> pwc: pwc@a3700000 {
> 	compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc",
> 		     "syscon", "simple-mfd";
> 	reg = <0 0xa3700000 0 0x800>;
> };
> 
> pwc-gpio {
> 	compatible = "renesas,r9a09g011-pwc-gpio",
> 		     "renesas,rzv2m-pwc-gpio";
> 	regmap = <&pwc>;
> 	gpio-controller;
> 	#gpio-cells = <2>;
> };
> 
> pwc-poweroff {
> 	compatible = "renesas,r9a09g011-pwc-poweroff",
> 		     "renesas,rzv2m-pwc-poweroff";
> 	regmap = <&pwc>;
> };
> 
> 
> I think the below describes things better:
> pwc: pwc@a3700000 {
> 	compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc",
> 		     "syscon", "simple-mfd";
> 	reg = <0 0xa3700000 0 0x800>;
> 
> 	gpio {
> 		compatible = "renesas,r9a09g011-pwc-gpio",
> 			     "renesas,rzv2m-pwc-gpio";
> 		regmap = <&pwc>;

You speak about two different things. So again - drop regmap. You do not
need it.

> 		offset = <0x80>;
> 		gpio-controller;
> 		#gpio-cells = <2>;
> 	};
> 
> 	poweroff {
> 		compatible = "renesas,r9a09g011-pwc-poweroff",
> 			     "renesas,rzv2m-pwc-poweroff";
> 		regmap = <&pwc>;

Drop regmap.

> 	};
> };
> 

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: power: reset: Add RZ/V2M PWC Power OFF bindings

[Krzysztof Kozlowski]

On 13/12/2022 23:43, Fabrizio Castro wrote:
> Add dt-bindings document for the RZ/V2M PWC Power OFF driver.

Drop driver.

Subject: drop second, redundant "bindings".

> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> ---
>  .../reset/renesas,rzv2m-pwc-poweroff.yaml     | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-poweroff.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-poweroff.yaml
> new file mode 100644
> index 000000000000..12456e3e93e6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-poweroff.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/reset/renesas,rzv2m-pwc-poweroff.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/V2M External Power Sequence Controller (PWC) Power OFF
> +
> +description: |+
> +  The PWC IP found in the RZ/V2M family of chips comes with the below
> +  capabilities
> +    - external power supply on/off sequence generation
> +    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
> +    - key input signals processing
> +    - general-purpose output pins
> +  This node uses syscon to map the registers relevant to Power OFF (the
> +  register map is retrieved from the parent dt-node), and the node should be
> +  represented as a sub node of a "syscon", "simple-mfd" node.
> +
> +maintainers:
> +  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - renesas,r9a09g011-pwc-poweroff # RZ/V2M
> +          - renesas,r9a09g055-pwc-poweroff # RZ/V2MA
> +      - const: renesas,rzv2m-pwc-poweroff
> +
> +  regmap:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: |
> +      Phandle to the register map node.

This also has to go.

> +
> +required:
> +  - compatible
> +  - regmap
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    poweroff {
> +            compatible = "renesas,r9a09g011-pwc-poweroff",

Use 4 spaces for example indentation.

> +                         "renesas,rzv2m-pwc-poweroff";
> +            regmap = <&regmapnode>;
> +    };

Best regards,
Krzysztof

RE: [PATCH 3/5] dt-bindings: mfd: Add RZ/V2M PWC global registers bindings

[Fabrizio Castro]

Hi Rob,

Thanks for the feeback.

> From: Rob Herring <robh@kernel.org>
> Sent: 14 December 2022 16:16
> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH 3/5] dt-bindings: mfd: Add RZ/V2M PWC global registers
> bindings
> 
> On Tue, Dec 13, 2022 at 10:43:08PM +0000, Fabrizio Castro wrote:
> > The RZ/V2M PWC is a multi-function device, and its software
> > support relies on "syscon" and "simple-mfd".
> > Add the dt-bindings for the top level device tree node.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > ---
> >  .../bindings/mfd/renesas,rzv2m-pwc.yaml       | 70 +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/renesas,rzv2m-
> pwc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/renesas,rzv2m-
> pwc.yaml b/Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml
> > new file mode 100644
> > index 000000000000..a7e180bfbd83
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/renesas,rzv2m-pwc.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +
> > +title: Renesas RZ/V2M External Power Sequence Controller (PWC)
> > +
> > +description: |+
> > +  The PWC IP found in the RZ/V2M family of chips comes with the below
> > +  capabilities
> > +    - external power supply on/off sequence generation
> > +    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
> > +    - key input signals processing
> > +    - general-purpose output pins
> > +
> > +maintainers:
> > +  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - renesas,r9a09g011-pwc # RZ/V2M
> > +          - renesas,r9a09g055-pwc # RZ/V2MA
> > +      - const: renesas,rzv2m-pwc
> > +      - const: syscon
> > +      - const: simple-mfd
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  gpio:
> > +    type: object
> > +    $ref: /schemas/gpio/renesas,rzv2m-pwc-gpio.yaml#
> > +    description: General-Purpose Output pins controller.
> > +
> > +  poweroff:
> > +    type: object
> > +    $ref: /schemas/power/reset/renesas,rzv2m-pwc-poweroff.yaml#
> > +    description: Power OFF controller.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    pwc: pwc@a3700000 {
> > +            compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc",
> "syscon",
> > +                         "simple-mfd";
> > +            reg = <0xa3700000 0x800>;
> > +
> > +            gpio {
> > +                    compatible = "renesas,r9a09g011-pwc-gpio",
> > +                                 "renesas,rzv2m-pwc-gpio";
> > +                    regmap = <&pwc>;
> > +                    offset = <0x80>;
> > +                    gpio-controller;
> > +                    #gpio-cells = <2>;
> > +            };
> > +
> > +            poweroff {
> > +                    compatible = "renesas,r9a09g011-pwc-poweroff",
> > +                                 "renesas,rzv2m-pwc-poweroff";
> > +                    regmap = <&pwc>;
> 
> Why does this need to be a child node? There aren't any resources for
> it. 'regmap' is just the parent node.
> 
> Assuming this binding is complete, I don't think you need any child
> nodes. A single node can have multiple providers.

Alright, then I'll just put everything the device needs into a single
node. I'll send v2 based on the below snippet:

    pwc@a3700000 {
      compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc";
      reg = <0xa3700000 0x800>;
      gpio-controller;
      #gpio-cells = <2>;
      renesas,rzv2m-pwc-power;
    };

Thanks,
Fab

> 
> Rob

RE: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver bindings

[Fabrizio Castro]

Hi Rob,

Thanks for the feedback.

> From: Rob Herring <robh@kernel.org>
> Sent: 14 December 2022 16:11
> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver
> bindings
> 
> On Tue, Dec 13, 2022 at 10:43:06PM +0000, Fabrizio Castro wrote:
> > Add dt-bindings document for the RZ/V2M PWC GPIO driver.
> 
> Bindings are for h/w blocks/devices, not a specific driver.

Right, I'll be more careful next time.

> 
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > ---
> >  .../bindings/gpio/renesas,rzv2m-pwc-gpio.yaml | 62 +++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
> gpio.yaml b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
> gpio.yaml
> > new file mode 100644
> > index 000000000000..ecc034d53259
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
> > @@ -0,0 +1,62 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +
> > +title: Renesas RZ/V2M External Power Sequence Controller (PWC) GPIO
> > +
> > +description: |+
> > +  The PWC IP found in the RZ/V2M family of chips comes with General-
> Purpose
> > +  Output pins, alongside the below functions
> > +    - external power supply on/off sequence generation
> > +    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
> > +    - key input signals processing
> > +  This node uses syscon to map the register used to control the GPIOs
> > +  (the register map is retrieved from the parent dt-node), and the node
> should
> > +  be represented as a sub node of a "syscon", "simple-mfd" node.
> > +
> > +maintainers:
> > +  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - renesas,r9a09g011-pwc-gpio # RZ/V2M
> > +          - renesas,r9a09g055-pwc-gpio # RZ/V2MA
> > +      - const: renesas,rzv2m-pwc-gpio
> > +
> > +  offset:
> 
> Too generic of a name. We want any given property name (globally) to
> have 1 type. With the below comment, this should be replaced with 'reg'
> instead if you have child nodes.

I'll take it out

> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      Offset in the register map for controlling the GPIOs (in bytes).
> > +
> > +  regmap:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Phandle to the register map node.
> 
> Looks like GPIO is a sub-function of some other block. Define the
> binding for that entire block. GPIO can be either either a function of
> that node (just add GPIO provider properties) or you can have GPIO child
> nodes. Depends on what the entire block looks like to decide. Do you
> have multiple instances of the GPIO block would be one reason to have
> child nodes.

I'll take out this child node.

Thanks,
Fab

> 
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +required:
> > +  - compatible
> > +  - regmap
> > +  - offset
> > +  - gpio-controller
> > +  - '#gpio-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    gpio {
> > +            compatible = "renesas,r9a09g011-pwc-gpio",
> > +                         "renesas,rzv2m-pwc-gpio";
> > +            regmap = <&regmapnode>;
> > +            offset = <0x80>;
> > +            gpio-controller;
> > +            #gpio-cells = <2>;
> > +    };
> > --
> > 2.34.1
> >
> >

RE: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver bindings

[Fabrizio Castro]

Hi Krzysztof,

Thanks for your feedback.

> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: 15 December 2022 09:49
> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Rob Herring
> <robh@kernel.org>
> Subject: Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver
> bindings
> 
> On 14/12/2022 19:26, Fabrizio Castro wrote:
> > Hi Rob,
> >
> > Thanks for your feedback!
> >
> >> From: Rob Herring <robh@kernel.org>
> >> Sent: 14 December 2022 16:11
> >> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> >> Subject: Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver
> >> bindings
> >>
> >> On Tue, Dec 13, 2022 at 10:43:06PM +0000, Fabrizio Castro wrote:
> >>> Add dt-bindings document for the RZ/V2M PWC GPIO driver.
> >>
> >> Bindings are for h/w blocks/devices, not a specific driver.
> >
> > Apologies, I will reword the changelog in v2.
> >
> >>
> >>>
> >>> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> >>> ---
> >>>  .../bindings/gpio/renesas,rzv2m-pwc-gpio.yaml | 62
> +++++++++++++++++++
> >>>  1 file changed, 62 insertions(+)
> >>>  create mode 100644
> >> Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
> >> gpio.yaml b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
> >> gpio.yaml
> >>> new file mode 100644
> >>> index 000000000000..ecc034d53259
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-
> gpio.yaml
> >>> @@ -0,0 +1,62 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +
> >>> +title: Renesas RZ/V2M External Power Sequence Controller (PWC) GPIO
> >>> +
> >>> +description: |+
> >>> +  The PWC IP found in the RZ/V2M family of chips comes with General-
> >> Purpose
> >>> +  Output pins, alongside the below functions
> >>> +    - external power supply on/off sequence generation
> >>> +    - on/off signal generation for the LPDDR4 core power supply
> (LPVDD)
> >>> +    - key input signals processing
> >>> +  This node uses syscon to map the register used to control the GPIOs
> >>> +  (the register map is retrieved from the parent dt-node), and the
> node
> >> should
> >>> +  be represented as a sub node of a "syscon", "simple-mfd" node.
> >>> +
> >>> +maintainers:
> >>> +  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - enum:
> >>> +          - renesas,r9a09g011-pwc-gpio # RZ/V2M
> >>> +          - renesas,r9a09g055-pwc-gpio # RZ/V2MA
> >>> +      - const: renesas,rzv2m-pwc-gpio
> >>> +
> >>> +  offset:
> >>
> >> Too generic of a name. We want any given property name (globally) to
> >> have 1 type. With the below comment, this should be replaced with 'reg'
> >> instead if you have child nodes.
> >
> > My understanding is that syscon subnodes normally use this name for
> exactly
> > the same purpose, for example:
> >
> >
> > What am I missing?
> 
> These are generic drivers, so they need offset as they do not match any
> specific programming model. You are not making a generic device. Address
> offsets are not suitable in most cases for DTS. There are of course
> exceptions so you can present reasons why this one is exception.

Thanks for the explanation

> >
> >>
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    description: |
> >>> +      Offset in the register map for controlling the GPIOs (in
> bytes).
> >>> +
> >>> +  regmap:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description: Phandle to the register map node.
> >>
> >> Looks like GPIO is a sub-function of some other block. Define the
> >> binding for that entire block.
> >
> > That's defined in patch 3 from this series.
> > I have sent it as patch 3 because that document references:
> > * /schemas/gpio/renesas,rzv2m-pwc-gpio.yaml
> > * /schemas/power/reset/renesas,rzv2m-pwc-poweroff.yaml
> > Which are defined in this patch and in patch 2 in the series.
> >
> > Do you want me to move patch 3 to patch 1 in v2?
> 
> We do not want regmap, but proper definition of entire hardware.

Will do. I'll drop regmap.

> 
> >
> >> GPIO can be either either a function of
> >> that node (just add GPIO provider properties) or you can have GPIO
> child
> >> nodes. Depends on what the entire block looks like to decide. Do you
> >> have multiple instances of the GPIO block would be one reason to have
> >> child nodes.
> >
> > From a pure HW point of view, this GPIO block is contained inside the
> PWC block
> > (as PWC is basically a MFD device), and it only deals with 2 General-
> Purpose
> > Output pins, both controlled by 1 (and the same) register, therefore the
> GPIO
> > block is only 1 child.
> >
> > If possible, I would like to keep the functionality offered by the sub-
> blocks of
> > PWC contained in separated drivers and DT nodes (either non-child nodes
> or child
> > nodes).
> 
> Driver do not matter for bindings. We talk about regmap field which - as
> you explained above - is not needed.

Okay, I'll rework, and I'll send v2.

Thanks,
Fab

> 
> 
> >
> > My understanding is that simple-mfd will automatically probe the
> children of the
> > simple-mfd node, and also hierarchically it makes sense to me to have
> the DT nodes
> > of the PWC sub-blocks as children of the "syscon", "simple-mfd" node. I
> have found
> > other instances of this same architecture in the kernel already (plenty
> from NXP/Freescale),
> > for example:
> 
> I don't understand. You do not have here simple-mfd and it still does
> not explain Rob's comment and regmap.
> 
> >
> > etc...
> >
> > Something like the below could also work, but I don't think it would
> represent the
> > HW accurately:
> > pwc: pwc@a3700000 {
> > 	compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc",
> > 		     "syscon", "simple-mfd";
> > 	reg = <0 0xa3700000 0 0x800>;
> > };
> >
> > pwc-gpio {
> > 	compatible = "renesas,r9a09g011-pwc-gpio",
> > 		     "renesas,rzv2m-pwc-gpio";
> > 	regmap = <&pwc>;
> > 	gpio-controller;
> > 	#gpio-cells = <2>;
> > };
> >
> > pwc-poweroff {
> > 	compatible = "renesas,r9a09g011-pwc-poweroff",
> > 		     "renesas,rzv2m-pwc-poweroff";
> > 	regmap = <&pwc>;
> > };
> >
> >
> > I think the below describes things better:
> > pwc: pwc@a3700000 {
> > 	compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc",
> > 		     "syscon", "simple-mfd";
> > 	reg = <0 0xa3700000 0 0x800>;
> >
> > 	gpio {
> > 		compatible = "renesas,r9a09g011-pwc-gpio",
> > 			     "renesas,rzv2m-pwc-gpio";
> > 		regmap = <&pwc>;
> 
> You speak about two different things. So again - drop regmap. You do not
> need it.
> 
> > 		offset = <0x80>;
> > 		gpio-controller;
> > 		#gpio-cells = <2>;
> > 	};
> >
> > 	poweroff {
> > 		compatible = "renesas,r9a09g011-pwc-poweroff",
> > 			     "renesas,rzv2m-pwc-poweroff";
> > 		regmap = <&pwc>;
> 
> Drop regmap.
> 
> > 	};
> > };
> >
> 
> Best regards,
> Krzysztof

RE: [PATCH 2/5] dt-bindings: power: reset: Add RZ/V2M PWC Power OFF bindings

[Fabrizio Castro]

Hi Krzysztof,

Thanks for your feedback.

> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: 15 December 2022 09:50
> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>;
> Subject: Re: [PATCH 2/5] dt-bindings: power: reset: Add RZ/V2M PWC Power
> OFF bindings
> 
> On 13/12/2022 23:43, Fabrizio Castro wrote:
> > Add dt-bindings document for the RZ/V2M PWC Power OFF driver.
> 
> Drop driver.
> 
> Subject: drop second, redundant "bindings".

Thanks

> 
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > ---
> >  .../reset/renesas,rzv2m-pwc-poweroff.yaml     | 48 +++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-
> poweroff.yaml
> >
> > diff --git
> a/Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-
> poweroff.yaml
> b/Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-
> poweroff.yaml
> > new file mode 100644
> > index 000000000000..12456e3e93e6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/reset/renesas,rzv2m-pwc-
> poweroff.yaml
> > @@ -0,0 +1,48 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +
> > +title: Renesas RZ/V2M External Power Sequence Controller (PWC) Power
> OFF
> > +
> > +description: |+
> > +  The PWC IP found in the RZ/V2M family of chips comes with the below
> > +  capabilities
> > +    - external power supply on/off sequence generation
> > +    - on/off signal generation for the LPDDR4 core power supply (LPVDD)
> > +    - key input signals processing
> > +    - general-purpose output pins
> > +  This node uses syscon to map the registers relevant to Power OFF (the
> > +  register map is retrieved from the parent dt-node), and the node
> should be
> > +  represented as a sub node of a "syscon", "simple-mfd" node.
> > +
> > +maintainers:
> > +  - Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - renesas,r9a09g011-pwc-poweroff # RZ/V2M
> > +          - renesas,r9a09g055-pwc-poweroff # RZ/V2MA
> > +      - const: renesas,rzv2m-pwc-poweroff
> > +
> > +  regmap:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: |
> > +      Phandle to the register map node.
> 
> This also has to go.

It'll go

> 
> > +
> > +required:
> > +  - compatible
> > +  - regmap
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    poweroff {
> > +            compatible = "renesas,r9a09g011-pwc-poweroff",
> 
> Use 4 spaces for example indentation.

Doh, I'll be more careful next time.

Thanks,
Fab

> 
> > +                         "renesas,rzv2m-pwc-poweroff";
> > +            regmap = <&regmapnode>;
> > +    };
> 
> Best regards,
> Krzysztof

Re: [PATCH 4/5] gpio: Add support for Renesas RZ/V2M PWC

[Bartosz Golaszewski]

On Tue, Dec 13, 2022 at 11:43 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
>
> The RZ/V2M SoC contains an External Power Sequence Controller (PWC)
> module. This module provides an external power supply on/off sequence,
> on/off signal for the LPDDR4 core power supply, control signals for
> external I/O power supplies of the SD host interfaces, and key input
> signals.
> PWC is essentially a Multi-Function Device (MFD).
>
> The driver just implements the control signals for external I/O
> power supplies of the SD host interfaces as gpios, and it relies on
> syscon and simple-mfd.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> ---
>  drivers/gpio/Kconfig          |   8 +++
>  drivers/gpio/Makefile         |   1 +
>  drivers/gpio/gpio-rzv2m-pwc.c | 123 ++++++++++++++++++++++++++++++++++
>  3 files changed, 132 insertions(+)
>  create mode 100644 drivers/gpio/gpio-rzv2m-pwc.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index e6ebc4c90a5d..e016919b9643 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -553,6 +553,14 @@ config GPIO_ROCKCHIP
>         help
>           Say yes here to support GPIO on Rockchip SoCs.
>
> +config GPIO_RZV2M_PWC
> +       tristate "Renesas RZ/V2M PWC GPIO support"
> +       depends on MFD_SYSCON
> +       depends on ARCH_R9A09G011 || COMPILE_TEST
> +       help
> +         Say yes here to support the External Power Sequence Controller (PWC)
> +         GPIO controller driver for RZ/V2M devices.
> +
>  config GPIO_SAMA5D2_PIOBU
>         tristate "SAMA5D2 PIOBU GPIO support"
>         depends on MFD_SYSCON
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 3462a138764a..5f655684603f 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -132,6 +132,7 @@ obj-$(CONFIG_GPIO_RDC321X)          += gpio-rdc321x.o
>  obj-$(CONFIG_GPIO_REALTEK_OTTO)                += gpio-realtek-otto.o
>  obj-$(CONFIG_GPIO_REG)                 += gpio-reg.o
>  obj-$(CONFIG_GPIO_ROCKCHIP)    += gpio-rockchip.o
> +obj-$(CONFIG_GPIO_RZV2M_PWC)           += gpio-rzv2m-pwc.o
>  obj-$(CONFIG_ARCH_SA1100)              += gpio-sa1100.o
>  obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)       += gpio-sama5d2-piobu.o
>  obj-$(CONFIG_GPIO_SCH311X)             += gpio-sch311x.o
> diff --git a/drivers/gpio/gpio-rzv2m-pwc.c b/drivers/gpio/gpio-rzv2m-pwc.c
> new file mode 100644
> index 000000000000..672d868cb8c9
> --- /dev/null
> +++ b/drivers/gpio/gpio-rzv2m-pwc.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Renesas Electronics Corporation
> + *
> + * GPIO driver for Renesas RZ/V2M External Power Sequence Controller (PWC)
> + */
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/spinlock.h>
> +
> +struct rzv2m_pwc_gpio_priv {
> +       struct gpio_chip gp;
> +       int offset;
> +       struct regmap *regmap;
> +       DECLARE_BITMAP(ch_en_bits, 2);
> +};
> +
> +static void rzv2m_pwc_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +                              int value)
> +{
> +       struct rzv2m_pwc_gpio_priv *priv = gpiochip_get_data(chip);
> +       u32 reg;
> +
> +       /* BIT 16 enables write to BIT 0, and BIT 17 enables write to BIT 1 */
> +       reg = BIT(offset + 16);
> +       if (value)
> +               reg |= BIT(offset);
> +
> +       regmap_write(priv->regmap, priv->offset, reg);
> +
> +       if (value)
> +               set_bit(offset, priv->ch_en_bits);
> +       else
> +               clear_bit(offset, priv->ch_en_bits);

You can use assign_bit() here and pass value to it.

> +}
> +
> +static int rzv2m_pwc_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct rzv2m_pwc_gpio_priv *priv = gpiochip_get_data(chip);
> +
> +       return test_bit(offset, priv->ch_en_bits);
> +}
> +
> +static int rzv2m_pwc_gpio_direction_output(struct gpio_chip *gc,
> +                                          unsigned int nr, int value)
> +{
> +       if (nr > 1)
> +               return -EINVAL;
> +
> +       rzv2m_pwc_gpio_set(gc, nr, value);
> +
> +       return 0;
> +}
> +
> +static const struct gpio_chip rzv2m_pwc_gc = {
> +       .label = "rzv2m_pwc_gpio",
> +       .owner = THIS_MODULE,
> +       .get = rzv2m_pwc_gpio_get,
> +       .set = rzv2m_pwc_gpio_set,
> +       .direction_output = rzv2m_pwc_gpio_direction_output,
> +       .can_sleep = false,
> +       .ngpio = 2,
> +       .base = -1,
> +};
> +
> +static int rzv2m_pwc_gpio_probe(struct platform_device *pdev)
> +{
> +       struct rzv2m_pwc_gpio_priv *priv;
> +       int err;
> +
> +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +                                                      "regmap");
> +
> +       if (IS_ERR(priv->regmap))
> +               return dev_err_probe(&pdev->dev, PTR_ERR(priv->regmap),
> +                                    "Can't find regmap property");
> +
> +       err = of_property_read_u32(pdev->dev.of_node, "offset", &priv->offset);

Please don't use OF APIs in drivers anymore, use
device_property_read_u32() instead.

Otherwise looks pretty good!

Bart

> +       if (err)
> +               return dev_err_probe(&pdev->dev, -EINVAL,
> +                                    "Can't find offset property");
> +
> +       /*
> +        * The register used by this driver cannot be read, therefore set the
> +        * outputs to their default values and initialize priv->ch_en_bits accordingly.
> +        * BIT 16 enables write to BIT 0, BIT 17 enables write to BIT 1, and the
> +        * default value of both BIT 0 and BIT 1 is 0.
> +        */
> +       regmap_write(priv->regmap, priv->offset, BIT(17) | BIT(16));
> +       bitmap_zero(priv->ch_en_bits, 2);
> +
> +       priv->gp = rzv2m_pwc_gc;
> +       priv->gp.parent = pdev->dev.parent;
> +       priv->gp.fwnode = dev_fwnode(&pdev->dev);
> +
> +       return devm_gpiochip_add_data(&pdev->dev, &priv->gp, priv);
> +}
> +
> +static const struct of_device_id rzv2m_pwc_gpio_of_match[] = {
> +       { .compatible = "renesas,rzv2m-pwc-gpio" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rzv2m_pwc_gpio_of_match);
> +
> +static struct platform_driver rzv2m_pwc_gpio_driver = {
> +       .probe = rzv2m_pwc_gpio_probe,
> +       .driver = {
> +               .name = "rzv2m_pwc_gpio",
> +               .of_match_table = of_match_ptr(rzv2m_pwc_gpio_of_match),
> +       },
> +};
> +module_platform_driver(rzv2m_pwc_gpio_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Fabrizio Castro <castro.fabrizio.jz@renesas.com>");
> +MODULE_DESCRIPTION("Renesas RZ/V2M PWC GPIO");
> --
> 2.34.1
>

RE: [PATCH 4/5] gpio: Add support for Renesas RZ/V2M PWC

[Fabrizio Castro]

Hi Bartosz,

Thanks for your feedback!

> 
> On Tue, Dec 13, 2022 at 11:43 PM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> >
> > The RZ/V2M SoC contains an External Power Sequence Controller (PWC)
> > module. This module provides an external power supply on/off sequence,
> > on/off signal for the LPDDR4 core power supply, control signals for
> > external I/O power supplies of the SD host interfaces, and key input
> > signals.
> > PWC is essentially a Multi-Function Device (MFD).
> >
> > The driver just implements the control signals for external I/O
> > power supplies of the SD host interfaces as gpios, and it relies on
> > syscon and simple-mfd.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > ---
> >  drivers/gpio/Kconfig          |   8 +++
> >  drivers/gpio/Makefile         |   1 +
> >  drivers/gpio/gpio-rzv2m-pwc.c | 123 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 132 insertions(+)
> >  create mode 100644 drivers/gpio/gpio-rzv2m-pwc.c
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index e6ebc4c90a5d..e016919b9643 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -553,6 +553,14 @@ config GPIO_ROCKCHIP
> >         help
> >           Say yes here to support GPIO on Rockchip SoCs.
> >
> > +config GPIO_RZV2M_PWC
> > +       tristate "Renesas RZ/V2M PWC GPIO support"
> > +       depends on MFD_SYSCON
> > +       depends on ARCH_R9A09G011 || COMPILE_TEST
> > +       help
> > +         Say yes here to support the External Power Sequence Controller
> (PWC)
> > +         GPIO controller driver for RZ/V2M devices.
> > +
> >  config GPIO_SAMA5D2_PIOBU
> >         tristate "SAMA5D2 PIOBU GPIO support"
> >         depends on MFD_SYSCON
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 3462a138764a..5f655684603f 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -132,6 +132,7 @@ obj-$(CONFIG_GPIO_RDC321X)          += gpio-
> rdc321x.o
> >  obj-$(CONFIG_GPIO_REALTEK_OTTO)                += gpio-realtek-otto.o
> >  obj-$(CONFIG_GPIO_REG)                 += gpio-reg.o
> >  obj-$(CONFIG_GPIO_ROCKCHIP)    += gpio-rockchip.o
> > +obj-$(CONFIG_GPIO_RZV2M_PWC)           += gpio-rzv2m-pwc.o
> >  obj-$(CONFIG_ARCH_SA1100)              += gpio-sa1100.o
> >  obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)       += gpio-sama5d2-piobu.o
> >  obj-$(CONFIG_GPIO_SCH311X)             += gpio-sch311x.o
> > diff --git a/drivers/gpio/gpio-rzv2m-pwc.c b/drivers/gpio/gpio-rzv2m-
> pwc.c
> > new file mode 100644
> > index 000000000000..672d868cb8c9
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-rzv2m-pwc.c
> > @@ -0,0 +1,123 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2022 Renesas Electronics Corporation
> > + *
> > + * GPIO driver for Renesas RZ/V2M External Power Sequence Controller
> (PWC)
> > + */
> > +
> > +#include <linux/gpio/driver.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spinlock.h>
> > +
> > +struct rzv2m_pwc_gpio_priv {
> > +       struct gpio_chip gp;
> > +       int offset;
> > +       struct regmap *regmap;
> > +       DECLARE_BITMAP(ch_en_bits, 2);
> > +};
> > +
> > +static void rzv2m_pwc_gpio_set(struct gpio_chip *chip, unsigned int
> offset,
> > +                              int value)
> > +{
> > +       struct rzv2m_pwc_gpio_priv *priv = gpiochip_get_data(chip);
> > +       u32 reg;
> > +
> > +       /* BIT 16 enables write to BIT 0, and BIT 17 enables write to
> BIT 1 */
> > +       reg = BIT(offset + 16);
> > +       if (value)
> > +               reg |= BIT(offset);
> > +
> > +       regmap_write(priv->regmap, priv->offset, reg);
> > +
> > +       if (value)
> > +               set_bit(offset, priv->ch_en_bits);
> > +       else
> > +               clear_bit(offset, priv->ch_en_bits);
> 
> You can use assign_bit() here and pass value to it.

Good point, I will fix it in v3.

> 
> > +}
> > +
> > +static int rzv2m_pwc_gpio_get(struct gpio_chip *chip, unsigned int
> offset)
> > +{
> > +       struct rzv2m_pwc_gpio_priv *priv = gpiochip_get_data(chip);
> > +
> > +       return test_bit(offset, priv->ch_en_bits);
> > +}
> > +
> > +static int rzv2m_pwc_gpio_direction_output(struct gpio_chip *gc,
> > +                                          unsigned int nr, int value)
> > +{
> > +       if (nr > 1)
> > +               return -EINVAL;
> > +
> > +       rzv2m_pwc_gpio_set(gc, nr, value);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct gpio_chip rzv2m_pwc_gc = {
> > +       .label = "rzv2m_pwc_gpio",
> > +       .owner = THIS_MODULE,
> > +       .get = rzv2m_pwc_gpio_get,
> > +       .set = rzv2m_pwc_gpio_set,
> > +       .direction_output = rzv2m_pwc_gpio_direction_output,
> > +       .can_sleep = false,
> > +       .ngpio = 2,
> > +       .base = -1,
> > +};
> > +
> > +static int rzv2m_pwc_gpio_probe(struct platform_device *pdev)
> > +{
> > +       struct rzv2m_pwc_gpio_priv *priv;
> > +       int err;
> > +
> > +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       priv->regmap = syscon_regmap_lookup_by_phandle(pdev-
> >dev.of_node,
> > +                                                      "regmap");
> > +
> > +       if (IS_ERR(priv->regmap))
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(priv->regmap),
> > +                                    "Can't find regmap property");
> > +
> > +       err = of_property_read_u32(pdev->dev.of_node, "offset", &priv-
> >offset);
> 
> Please don't use OF APIs in drivers anymore, use
> device_property_read_u32() instead.


Gotcha, I'll fix it in v3.

Thanks!
Fab

> 
> Otherwise looks pretty good!
> 
> Bart
> 
> > +       if (err)
> > +               return dev_err_probe(&pdev->dev, -EINVAL,
> > +                                    "Can't find offset property");
> > +
> > +       /*
> > +        * The register used by this driver cannot be read, therefore
> set the
> > +        * outputs to their default values and initialize priv-
> >ch_en_bits accordingly.
> > +        * BIT 16 enables write to BIT 0, BIT 17 enables write to BIT 1,
> and the
> > +        * default value of both BIT 0 and BIT 1 is 0.
> > +        */
> > +       regmap_write(priv->regmap, priv->offset, BIT(17) | BIT(16));
> > +       bitmap_zero(priv->ch_en_bits, 2);
> > +
> > +       priv->gp = rzv2m_pwc_gc;
> > +       priv->gp.parent = pdev->dev.parent;
> > +       priv->gp.fwnode = dev_fwnode(&pdev->dev);
> > +
> > +       return devm_gpiochip_add_data(&pdev->dev, &priv->gp, priv);
> > +}
> > +
> > +static const struct of_device_id rzv2m_pwc_gpio_of_match[] = {
> > +       { .compatible = "renesas,rzv2m-pwc-gpio" },
> > +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rzv2m_pwc_gpio_of_match);
> > +
> > +static struct platform_driver rzv2m_pwc_gpio_driver = {
> > +       .probe = rzv2m_pwc_gpio_probe,
> > +       .driver = {
> > +               .name = "rzv2m_pwc_gpio",
> > +               .of_match_table = of_match_ptr(rzv2m_pwc_gpio_of_match),
> > +       },
> > +};
> > +module_platform_driver(rzv2m_pwc_gpio_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Fabrizio Castro <castro.fabrizio.jz@renesas.com>");
> > +MODULE_DESCRIPTION("Renesas RZ/V2M PWC GPIO");
> > --
> > 2.34.1
> >