[PATCH v2 00/15] Add initial USB support for the Renesas RZ/G3S SoC

[PATCH v2 00/15] Add initial USB support for the Renesas RZ/G3S SoC

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series adds initial USB support for the Renesas RZ/G3S SoC.

Series is split as follows:
- patches 01-05/15	- add SYSC driver support; this is necessary for USB
			  PHY as the USB PHY driver need to touch a register in
			  the SYSC address space, in the initialization phase
- patch 06/15		- updates the USBHS documentation for RZ/G3S
- patches 07-10/15	- updates the USB PHY support to handle the SYSC
			  USB PWRRDY signal. Along with it a fix for the
			  DT bindings and one for the PHY driver were
			  added; fixes are RZ/G3S USB related
- patch 11/15		- document the USB PHY Ctrl support
- patches 12-15/15	- add device tree support

Merge strategy, if any:
- patches 01-05/15,12-14/15 can go through Renesas tree
- patch 06/14 can go though USB tree
- patches 07-10/14 can go through PHY tree
- patch 11/15 can go though reset controller tree

Thank you,
Claudiu Beznea

Changes in v2:
- dropped v1 patches already applied
- added fixes patches (07/14 and 09/14)
- dropped the approach of handling the USB PWRRDY though a reset controller
  driver and introduced the signal concept for the SYSC driver; because
  of this, most of the work done in v1 was dropped
- per patch changes are listed in individual patches, if any

Christophe JAILLET (1):
  phy: renesas: rcar-gen3-usb2: Fix an error handling path in
    rcar_gen3_phy_usb2_probe()

Claudiu Beznea (14):
  dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add
    #renesas,sysc-signal-cells
  soc: renesas: Add SYSC driver for Renesas RZ family
  soc: renesas: rz-sysc: Enable SYSC driver for RZ/G3S
  soc: renesas: rz-sysc: Add SoC detection support
  soc: renesas: rz-sysc: Move RZ/G3S SoC detection to the SYSC driver
  dt-bindings: usb: renesas,usbhs: Document RZ/G3S SoC
  dt-bindings: phy: renesas,usb2-phy: Mark resets as required for RZ/G3S
  dt-bindings: phy: renesas,usb2-phy: Add renesas,sysc-signal
  phy: renesas: rcar-gen3-usb2: Add support for PWRRDY
  dt-bindings: reset: renesas,rzg2l-usbphy-ctrl: Document RZ/G3S support
  arm64: dts: renesas: Add #renesas,sysc-signal-cells to system
    controller node
  arm64: dts: renesas: r9a08g045: Enable the system controller
  arm64: dts: renesas: r9a08g045: Add USB support
  arm64: dts: renesas: rzg3s-smarc: Enable USB support

 .../bindings/phy/renesas,usb2-phy.yaml        |  26 +-
 .../reset/renesas,rzg2l-usbphy-ctrl.yaml      |   1 +
 .../soc/renesas/renesas,rzg2l-sysc.yaml       |  23 +-
 .../bindings/usb/renesas,usbhs.yaml           |   2 +
 arch/arm64/boot/dts/renesas/r9a07g043.dtsi    |   3 +-
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi    |   3 +-
 arch/arm64/boot/dts/renesas/r9a07g054.dtsi    |   3 +-
 arch/arm64/boot/dts/renesas/r9a08g045.dtsi    | 123 +++++-
 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi  |  57 +++
 drivers/phy/renesas/phy-rcar-gen3-usb2.c      |  77 +++-
 drivers/soc/renesas/Kconfig                   |   8 +
 drivers/soc/renesas/Makefile                  |   2 +
 drivers/soc/renesas/r9a08g045-sysc.c          |  43 +++
 drivers/soc/renesas/renesas-soc.c             |  12 -
 drivers/soc/renesas/rz-sysc.c                 | 350 ++++++++++++++++++
 drivers/soc/renesas/rz-sysc.h                 |  70 ++++
 16 files changed, 778 insertions(+), 25 deletions(-)
 create mode 100644 drivers/soc/renesas/r9a08g045-sysc.c
 create mode 100644 drivers/soc/renesas/rz-sysc.c
 create mode 100644 drivers/soc/renesas/rz-sysc.h

-- 
2.39.2

[PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The RZ/G3S system controller (SYSC) has registers to control signals that
are routed to various IPs. These signals must be controlled during
configuration of the respective IPs. One such signal is the USB PWRRDY,
which connects the SYSC and the USB PHY. This signal must to be controlled
before and after the power to the USB PHY is turned off/on.

Other similar signals include the following (according to the RZ/G3S
hardware manual):

* PCIe:
- ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
- PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
  register
- MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register

* SPI:
- SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
  register

* I2C/I3C:
- af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
  (x=0..3)
- af_bypass I3C signal controlled through SYS_I3C_CFG register

* Ethernet:
- FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
  registers (x=0..1)

Add #renesas,sysc-signal-cells DT property to allow different SYSC signals
consumers to manage these signals.

The goal is to enable consumers to specify the required access data for
these signals (through device tree) and let their respective drivers
control these signals via the syscon regmap provided by the system
controller driver. For example, the USB PHY will describe this relation
using the following DT property:

usb2_phy1: usb-phy@11e30200 {
	// ...
	renesas,sysc-signal = <&sysc 0xd70 0x1>;
	// ...
};

Along with it, add the syscon to the compatible list as it will be
requested by the consumer drivers. The syscon was added to the rest of
system controller variants as these are similar with RZ/G3S and can
benefit from the implementation proposed in this series.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new


 .../soc/renesas/renesas,rzg2l-sysc.yaml       | 23 ++++++++++++++-----
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
index 4386b2c3fa4d..90f827e8de3e 100644
--- a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
+++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
@@ -19,11 +19,13 @@ description:
 
 properties:
   compatible:
-    enum:
-      - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five
-      - renesas,r9a07g044-sysc # RZ/G2{L,LC}
-      - renesas,r9a07g054-sysc # RZ/V2L
-      - renesas,r9a08g045-sysc # RZ/G3S
+    items:
+      - enum:
+          - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five
+          - renesas,r9a07g044-sysc # RZ/G2{L,LC}
+          - renesas,r9a07g054-sysc # RZ/V2L
+          - renesas,r9a08g045-sysc # RZ/G3S
+      - const: syscon
 
   reg:
     maxItems: 1
@@ -42,9 +44,17 @@ properties:
       - const: cm33stbyr_int
       - const: ca55_deny
 
+  "#renesas,sysc-signal-cells":
+    description:
+      The number of cells needed to configure a SYSC controlled signal. First
+      cell specifies the SYSC offset of the configuration register, second cell
+      specifies the bitmask in register.
+    const: 2
+
 required:
   - compatible
   - reg
+  - "#renesas,sysc-signal-cells"
 
 additionalProperties: false
 
@@ -53,7 +63,7 @@ examples:
     #include <dt-bindings/interrupt-controller/arm-gic.h>
 
     sysc: system-controller@11020000 {
-            compatible = "renesas,r9a07g044-sysc";
+            compatible = "renesas,r9a07g044-sysc", "syscon";
             reg = <0x11020000 0x10000>;
             interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>,
                          <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
@@ -61,4 +71,5 @@ examples:
                          <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
             interrupt-names = "lpm_int", "ca55stbydone_int", "cm33stbyr_int",
                               "ca55_deny";
+            #renesas,sysc-signal-cells = <2>;
     };
-- 
2.39.2

[PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The RZ/G3S system controller (SYSC) has various registers that control
signals specific to individual IPs. IP drivers must control these signals
at different configuration phases.

Add SYSC driver that allows individual SYSC consumers to control these
signals. The SYSC driver exports a syscon regmap enabling IP drivers to
use a specific SYSC offset and mask from the device tree, which can then be
accessed through regmap_update_bits().

Currently, the SYSC driver provides control to the USB PWRRDY signal, which
is routed to the USB PHY. This signal needs to be managed before or after
powering the USB PHY off or on.

Other SYSC signals candidates (as exposed in the the hardware manual of the
RZ/G3S SoC) include:

* PCIe:
- ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
- PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
  register
- MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register

* SPI:
- SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
  register

* I2C/I3C:
- af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
  (x=0..3)
- af_bypass I3C signal controlled through SYS_I3C_CFG register

* Ethernet:
- FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
  registers (x=0..1)

As different Renesas RZ SoC shares most of the SYSC functionalities
available on the RZ/G3S SoC, the driver if formed of a SYSC core
part and a SoC specific part allowing individual SYSC SoC to provide
functionalities to the SYSC core.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Change in v2:
- this was patch 04/16 in v1
- dropped the initial approach proposed in v1 where a with a reset
  controller driver was proposed to handle the USB PWRRDY signal
- implemented it with syscon regmap and the SYSC signal concept
  (introduced in this patch)

 drivers/soc/renesas/Kconfig          |   7 +
 drivers/soc/renesas/Makefile         |   2 +
 drivers/soc/renesas/r9a08g045-sysc.c |  31 +++
 drivers/soc/renesas/rz-sysc.c        | 286 +++++++++++++++++++++++++++
 drivers/soc/renesas/rz-sysc.h        |  52 +++++
 5 files changed, 378 insertions(+)
 create mode 100644 drivers/soc/renesas/r9a08g045-sysc.c
 create mode 100644 drivers/soc/renesas/rz-sysc.c
 create mode 100644 drivers/soc/renesas/rz-sysc.h

diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig
index 9f7fe02310b9..0686c3ad9e27 100644
--- a/drivers/soc/renesas/Kconfig
+++ b/drivers/soc/renesas/Kconfig
@@ -378,4 +378,11 @@ config PWC_RZV2M
 config RST_RCAR
 	bool "Reset Controller support for R-Car" if COMPILE_TEST
 
+config SYSC_RZ
+	bool "System controller for RZ SoCs" if COMPILE_TEST
+
+config SYSC_R9A08G045
+	bool "Renesas RZ/G3S System controller support" if COMPILE_TEST
+	select SYSC_RZ
+
 endif # SOC_RENESAS
diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
index 734f8f8cefa4..8cd139b3dd0a 100644
--- a/drivers/soc/renesas/Makefile
+++ b/drivers/soc/renesas/Makefile
@@ -6,7 +6,9 @@ obj-$(CONFIG_SOC_RENESAS)	+= renesas-soc.o
 ifdef CONFIG_SMP
 obj-$(CONFIG_ARCH_R9A06G032)	+= r9a06g032-smp.o
 endif
+obj-$(CONFIG_SYSC_R9A08G045)	+= r9a08g045-sysc.o
 
 # Family
 obj-$(CONFIG_PWC_RZV2M)		+= pwc-rzv2m.o
 obj-$(CONFIG_RST_RCAR)		+= rcar-rst.o
+obj-$(CONFIG_SYSC_RZ)		+= rz-sysc.o
diff --git a/drivers/soc/renesas/r9a08g045-sysc.c b/drivers/soc/renesas/r9a08g045-sysc.c
new file mode 100644
index 000000000000..ceea738aee72
--- /dev/null
+++ b/drivers/soc/renesas/r9a08g045-sysc.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RZ/G3S System controller driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/init.h>
+
+#include "rz-sysc.h"
+
+#define SYS_USB_PWRRDY		0xd70
+#define SYS_USB_PWRRDY_PWRRDY_N	BIT(0)
+#define SYS_MAX_REG		0xe20
+
+static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
+	{
+		.name = "usb-pwrrdy",
+		.offset = SYS_USB_PWRRDY,
+		.mask = SYS_USB_PWRRDY_PWRRDY_N,
+		.refcnt_incr_val = 0
+	}
+};
+
+const struct rz_sysc_init_data rzg3s_sysc_init_data = {
+	.signals_init_data = rzg3s_sysc_signals_init_data,
+	.num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
+	.max_register_offset = SYS_MAX_REG,
+};
diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c
new file mode 100644
index 000000000000..dc0edacd7170
--- /dev/null
+++ b/drivers/soc/renesas/rz-sysc.c
@@ -0,0 +1,286 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RZ System controller driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#include <linux/dcache.h>
+#include <linux/debugfs.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/refcount.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+
+#include "rz-sysc.h"
+
+/**
+ * struct rz_sysc - RZ SYSC private data structure
+ * @base: SYSC base address
+ * @dev: SYSC device pointer
+ * @signals: SYSC signals
+ * @num_signals: number of SYSC signals
+ */
+struct rz_sysc {
+	void __iomem *base;
+	struct device *dev;
+	struct rz_sysc_signal *signals;
+	u8 num_signals;
+};
+
+static int rz_sysc_reg_read(void *context, unsigned int off, unsigned int *val)
+{
+	struct rz_sysc *sysc = context;
+
+	*val = readl(sysc->base + off);
+
+	return 0;
+}
+
+static struct rz_sysc_signal *rz_sysc_off_to_signal(struct rz_sysc *sysc, unsigned int offset,
+						    unsigned int mask)
+{
+	struct rz_sysc_signal *signals = sysc->signals;
+
+	for (u32 i = 0; i < sysc->num_signals; i++) {
+		if (signals[i].init_data->offset != offset)
+			continue;
+
+		/*
+		 * In case mask == 0 we just return the signal data w/o checking the mask.
+		 * This is useful when calling through rz_sysc_reg_write() to check
+		 * if the requested setting is for a mapped signal or not.
+		 */
+		if (mask) {
+			if (signals[i].init_data->mask == mask)
+				return &signals[i];
+		} else {
+			return &signals[i];
+		}
+	}
+
+	return NULL;
+}
+
+static int rz_sysc_reg_update_bits(void *context, unsigned int off,
+				   unsigned int mask, unsigned int val)
+{
+	struct rz_sysc *sysc = context;
+	struct rz_sysc_signal *signal;
+	bool update = false;
+
+	signal = rz_sysc_off_to_signal(sysc, off, mask);
+	if (signal) {
+		if (signal->init_data->refcnt_incr_val == val) {
+			if (!refcount_read(&signal->refcnt)) {
+				refcount_set(&signal->refcnt, 1);
+				update = true;
+			} else {
+				refcount_inc(&signal->refcnt);
+			}
+		} else {
+			update = refcount_dec_and_test(&signal->refcnt);
+		}
+	} else {
+		update = true;
+	}
+
+	if (update) {
+		u32 tmp;
+
+		tmp = readl(sysc->base + off);
+		tmp &= ~mask;
+		tmp |= val & mask;
+		writel(tmp, sysc->base + off);
+	}
+
+	return 0;
+}
+
+static int rz_sysc_reg_write(void *context, unsigned int off, unsigned int val)
+{
+	struct rz_sysc *sysc = context;
+	struct rz_sysc_signal *signal;
+
+	/*
+	 * Force using regmap_update_bits() for signals to have reference counter
+	 * per individual signal in case there are multiple signals controlled
+	 * through the same register.
+	 */
+	signal = rz_sysc_off_to_signal(sysc, off, 0);
+	if (signal) {
+		dev_err(sysc->dev,
+			"regmap_write() not allowed on register controlling a signal. Use regmap_update_bits()!");
+		return -EOPNOTSUPP;
+	}
+
+	writel(val, sysc->base + off);
+
+	return 0;
+}
+
+static bool rz_sysc_writeable_reg(struct device *dev, unsigned int off)
+{
+	struct rz_sysc *sysc = dev_get_drvdata(dev);
+	struct rz_sysc_signal *signal;
+
+	/* Any register containing a signal is writeable. */
+	signal = rz_sysc_off_to_signal(sysc, off, 0);
+	if (signal)
+		return true;
+
+	return false;
+}
+
+static bool rz_sysc_readable_reg(struct device *dev, unsigned int off)
+{
+	struct rz_sysc *sysc = dev_get_drvdata(dev);
+	struct rz_sysc_signal *signal;
+
+	/* Any register containing a signal is readable. */
+	signal = rz_sysc_off_to_signal(sysc, off, 0);
+	if (signal)
+		return true;
+
+	return false;
+}
+
+static int rz_sysc_signals_show(struct seq_file *s, void *what)
+{
+	struct rz_sysc *sysc = s->private;
+
+	seq_printf(s, "%-20s Enable count\n", "Signal");
+	seq_printf(s, "%-20s ------------\n", "--------------------");
+
+	for (u8 i = 0; i < sysc->num_signals; i++) {
+		seq_printf(s, "%-20s %d\n", sysc->signals[i].init_data->name,
+			   refcount_read(&sysc->signals[i].refcnt));
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(rz_sysc_signals);
+
+static void rz_sysc_debugfs_remove(void *data)
+{
+	debugfs_remove_recursive(data);
+}
+
+static int rz_sysc_signals_init(struct rz_sysc *sysc,
+				const struct rz_sysc_signal_init_data *init_data,
+				u32 num_signals)
+{
+	struct dentry *root;
+	int ret;
+
+	sysc->signals = devm_kcalloc(sysc->dev, num_signals, sizeof(*sysc->signals),
+				     GFP_KERNEL);
+	if (!sysc->signals)
+		return -ENOMEM;
+
+	for (u32 i = 0; i < num_signals; i++) {
+		struct rz_sysc_signal_init_data *id;
+
+		id = devm_kzalloc(sysc->dev, sizeof(*id), GFP_KERNEL);
+		if (!id)
+			return -ENOMEM;
+
+		id->name = devm_kstrdup(sysc->dev, init_data->name, GFP_KERNEL);
+		if (!id->name)
+			return -ENOMEM;
+
+		id->offset = init_data->offset;
+		id->mask = init_data->mask;
+		id->refcnt_incr_val = init_data->refcnt_incr_val;
+
+		sysc->signals[i].init_data = id;
+		refcount_set(&sysc->signals[i].refcnt, 0);
+	}
+
+	sysc->num_signals = num_signals;
+
+	root = debugfs_create_dir("renesas-rz-sysc", NULL);
+	ret = devm_add_action_or_reset(sysc->dev, rz_sysc_debugfs_remove, root);
+	if (ret)
+		return ret;
+	debugfs_create_file("signals", 0444, root, sysc, &rz_sysc_signals_fops);
+
+	return 0;
+}
+
+static struct regmap_config rz_sysc_regmap = {
+	.name = "rz_sysc_regs",
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+	.reg_read = rz_sysc_reg_read,
+	.reg_write = rz_sysc_reg_write,
+	.reg_update_bits = rz_sysc_reg_update_bits,
+	.writeable_reg = rz_sysc_writeable_reg,
+	.readable_reg = rz_sysc_readable_reg,
+};
+
+static const struct of_device_id rz_sysc_match[] = {
+#ifdef CONFIG_SYSC_R9A08G045
+	{ .compatible = "renesas,r9a08g045-sysc", .data = &rzg3s_sysc_init_data },
+#endif
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rz_sysc_match);
+
+static int rz_sysc_probe(struct platform_device *pdev)
+{
+	const struct rz_sysc_init_data *data;
+	struct device *dev = &pdev->dev;
+	struct rz_sysc *sysc;
+	struct regmap *regmap;
+	int ret;
+
+	data = device_get_match_data(dev);
+	if (!data || !data->max_register_offset)
+		return -EINVAL;
+
+	sysc = devm_kzalloc(dev, sizeof(*sysc), GFP_KERNEL);
+	if (!sysc)
+		return -ENOMEM;
+
+	sysc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(sysc->base))
+		return PTR_ERR(sysc->base);
+
+	sysc->dev = dev;
+
+	ret = rz_sysc_signals_init(sysc, data->signals_init_data, data->num_signals);
+	if (ret)
+		return ret;
+
+	dev_set_drvdata(dev, sysc);
+	rz_sysc_regmap.max_register = data->max_register_offset;
+	regmap = devm_regmap_init(dev, NULL, sysc, &rz_sysc_regmap);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return of_syscon_register_regmap(dev->of_node, regmap);
+}
+
+static struct platform_driver rz_sysc_driver = {
+	.driver = {
+		.name = "renesas-rz-sysc",
+		.of_match_table = rz_sysc_match
+	},
+	.probe = rz_sysc_probe
+};
+
+static int __init rz_sysc_init(void)
+{
+	return platform_driver_register(&rz_sysc_driver);
+}
+subsys_initcall(rz_sysc_init);
+
+MODULE_DESCRIPTION("Renesas RZ System Controller Driver");
+MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/soc/renesas/rz-sysc.h b/drivers/soc/renesas/rz-sysc.h
new file mode 100644
index 000000000000..bb850310c931
--- /dev/null
+++ b/drivers/soc/renesas/rz-sysc.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Renesas RZ System Controller
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#ifndef __SOC_RENESAS_RZ_SYSC_H__
+#define __SOC_RENESAS_RZ_SYSC_H__
+
+#include <linux/refcount.h>
+#include <linux/types.h>
+
+/**
+ * struct rz_sysc_signal_init_data - RZ SYSC signals init data
+ * @name: signal name
+ * @offset: register offset controling this signal
+ * @mask: bitmask in register specific to this signal
+ * @refcnt_incr_val: increment refcnt when setting this value
+ */
+struct rz_sysc_signal_init_data {
+	const char *name;
+	u32 offset;
+	u32 mask;
+	u32 refcnt_incr_val;
+};
+
+/**
+ * struct rz_sysc_signal - RZ SYSC signals
+ * @init_data: signals initialization data
+ * @refcnt: reference counter
+ */
+struct rz_sysc_signal {
+	const struct rz_sysc_signal_init_data *init_data;
+	refcount_t refcnt;
+};
+
+/**
+ * struct rz_sysc_init_data - RZ SYSC initialization data
+ * @signals_init_data: RZ SYSC signals initialization data
+ * @num_signals: number of SYSC signals
+ * @max_register_offset: Maximum SYSC register offset to be used by the regmap config
+ */
+struct rz_sysc_init_data {
+	const struct rz_sysc_signal_init_data *signals_init_data;
+	u32 num_signals;
+	u32 max_register_offset;
+};
+
+extern const struct rz_sysc_init_data rzg3s_sysc_init_data;
+
+#endif /* __SOC_RENESAS_RZ_SYSC_H__ */
-- 
2.39.2

[PATCH v2 03/15] soc: renesas: rz-sysc: Enable SYSC driver for RZ/G3S

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Enable SYSC driver for RZ/G3S. This is necessary for USB support.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new

 drivers/soc/renesas/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig
index 0686c3ad9e27..c8065f25ee53 100644
--- a/drivers/soc/renesas/Kconfig
+++ b/drivers/soc/renesas/Kconfig
@@ -334,6 +334,7 @@ config ARCH_R9A07G054
 config ARCH_R9A08G045
 	bool "ARM64 Platform support for RZ/G3S"
 	select ARCH_RZG2L
+	select SYSC_R9A08G045
 	help
 	  This enables support for the Renesas RZ/G3S SoC variants.
 
-- 
2.39.2

[PATCH v2 04/15] soc: renesas: rz-sysc: Add SoC detection support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The RZ SYSC controller has registers that keep the SoC ID data. Add
driver support to retrieve the SoC ID and register a SoC driver.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- this was patch 05/16 in v1
- changed patch title and description
- added SoC initialization code in its own function
- addressed the review comments
- introduced struct rz_sysc_soc_id_init_data and adjusted the code
  accordingly
- dropped the RZ/G3S SoC detection code (it will be introduced in
  a separate patch)

 drivers/soc/renesas/rz-sysc.c | 72 +++++++++++++++++++++++++++++++++--
 drivers/soc/renesas/rz-sysc.h | 18 +++++++++
 2 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c
index dc0edacd7170..d34d295831b8 100644
--- a/drivers/soc/renesas/rz-sysc.c
+++ b/drivers/soc/renesas/rz-sysc.c
@@ -14,9 +14,12 @@
 #include <linux/refcount.h>
 #include <linux/regmap.h>
 #include <linux/seq_file.h>
+#include <linux/sys_soc.h>
 
 #include "rz-sysc.h"
 
+#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
+
 /**
  * struct rz_sysc - RZ SYSC private data structure
  * @base: SYSC base address
@@ -211,6 +214,59 @@ static int rz_sysc_signals_init(struct rz_sysc *sysc,
 	return 0;
 }
 
+static int rz_sysc_soc_init(struct rz_sysc *sysc, const struct of_device_id *match)
+{
+	const struct rz_sysc_init_data *sysc_data = match->data;
+	const struct rz_sysc_soc_id_init_data *soc_data = sysc_data->soc_id_init_data;
+	struct soc_device_attribute *soc_dev_attr;
+	const char *soc_id_start, *soc_id_end;
+	u32 val, revision, specific_id;
+	struct soc_device *soc_dev;
+	char soc_id[32] = {0};
+	u8 size;
+
+	if (!soc_data || !soc_data->family || !soc_data->offset ||
+	    !soc_data->revision_mask)
+		return -EINVAL;
+
+	soc_id_start = strchr(match->compatible, ',') + 1;
+	soc_id_end = strchr(match->compatible, '-');
+	size = soc_id_end - soc_id_start;
+	if (size > 32)
+		size = 32;
+	strscpy(soc_id, soc_id_start, size);
+
+	soc_dev_attr = devm_kzalloc(sysc->dev, sizeof(*soc_dev_attr), GFP_KERNEL);
+	if (!soc_dev_attr)
+		return -ENOMEM;
+
+	soc_dev_attr->family = soc_data->family;
+	soc_dev_attr->soc_id = devm_kstrdup(sysc->dev, soc_id, GFP_KERNEL);
+	if (!soc_dev_attr->soc_id)
+		return -ENOMEM;
+
+	val = readl(sysc->base + soc_data->offset);
+	revision = field_get(soc_data->revision_mask, val);
+	specific_id = field_get(soc_data->specific_id_mask, val);
+	soc_dev_attr->revision = devm_kasprintf(sysc->dev, GFP_KERNEL, "%u", revision);
+	if (!soc_dev_attr->revision)
+		return -ENOMEM;
+
+	if (soc_data->id && specific_id != soc_data->id) {
+		dev_warn(sysc->dev, "SoC mismatch (product = 0x%x)\n", specific_id);
+		return -ENODEV;
+	}
+
+	dev_info(sysc->dev, "Detected Renesas %s %s Rev %s\n", soc_dev_attr->family,
+		 soc_dev_attr->soc_id, soc_dev_attr->revision);
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev))
+		return PTR_ERR(soc_dev);
+
+	return 0;
+}
+
 static struct regmap_config rz_sysc_regmap = {
 	.name = "rz_sysc_regs",
 	.reg_bits = 32,
@@ -235,14 +291,15 @@ MODULE_DEVICE_TABLE(of, rz_sysc_match);
 static int rz_sysc_probe(struct platform_device *pdev)
 {
 	const struct rz_sysc_init_data *data;
+	const struct of_device_id *match;
 	struct device *dev = &pdev->dev;
-	struct rz_sysc *sysc;
 	struct regmap *regmap;
+	struct rz_sysc *sysc;
 	int ret;
 
-	data = device_get_match_data(dev);
-	if (!data || !data->max_register_offset)
-		return -EINVAL;
+	match = of_match_node(rz_sysc_match, dev->of_node);
+	if (!match || !match->data)
+		return -ENODEV;
 
 	sysc = devm_kzalloc(dev, sizeof(*sysc), GFP_KERNEL);
 	if (!sysc)
@@ -253,6 +310,13 @@ static int rz_sysc_probe(struct platform_device *pdev)
 		return PTR_ERR(sysc->base);
 
 	sysc->dev = dev;
+	ret = rz_sysc_soc_init(sysc, match);
+	if (ret)
+		return ret;
+
+	data = match->data;
+	if (!data->max_register_offset)
+		return -EINVAL;
 
 	ret = rz_sysc_signals_init(sysc, data->signals_init_data, data->num_signals);
 	if (ret)
diff --git a/drivers/soc/renesas/rz-sysc.h b/drivers/soc/renesas/rz-sysc.h
index bb850310c931..babca9c743c7 100644
--- a/drivers/soc/renesas/rz-sysc.h
+++ b/drivers/soc/renesas/rz-sysc.h
@@ -35,13 +35,31 @@ struct rz_sysc_signal {
 	refcount_t refcnt;
 };
 
+/**
+ * struct rz_syc_soc_id_init_data - RZ SYSC SoC identification initialization data
+ * @family: RZ SoC family
+ * @id: RZ SoC expected ID
+ * @offset: SYSC SoC ID register offset
+ * @revision_mask: SYSC SoC ID revision mask
+ * @specific_id_mask: SYSC SoC ID specific ID mask
+ */
+struct rz_sysc_soc_id_init_data {
+	const char * const family;
+	u32 id;
+	u32 offset;
+	u32 revision_mask;
+	u32 specific_id_mask;
+};
+
 /**
  * struct rz_sysc_init_data - RZ SYSC initialization data
+ * @soc_id_init_data: RZ SYSC SoC ID initialization data
  * @signals_init_data: RZ SYSC signals initialization data
  * @num_signals: number of SYSC signals
  * @max_register_offset: Maximum SYSC register offset to be used by the regmap config
  */
 struct rz_sysc_init_data {
+	const struct rz_sysc_soc_id_init_data *soc_id_init_data;
 	const struct rz_sysc_signal_init_data *signals_init_data;
 	u32 num_signals;
 	u32 max_register_offset;
-- 
2.39.2

[PATCH v2 05/15] soc: renesas: rz-sysc: Move RZ/G3S SoC detection to the SYSC driver

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Now that we have SoC detection in the RZ SYSC driver, move the RZ/G3S
SoC detection to it. The SYSC provides SoC ID in its own registers.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- this was handled though patch 05/16 in v1 
- provide SoC specific init data through the SoC specific driver

 drivers/soc/renesas/r9a08g045-sysc.c | 12 ++++++++++++
 drivers/soc/renesas/renesas-soc.c    | 12 ------------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/soc/renesas/r9a08g045-sysc.c b/drivers/soc/renesas/r9a08g045-sysc.c
index ceea738aee72..81970db300b2 100644
--- a/drivers/soc/renesas/r9a08g045-sysc.c
+++ b/drivers/soc/renesas/r9a08g045-sysc.c
@@ -11,6 +11,9 @@
 
 #include "rz-sysc.h"
 
+#define SYS_LSI_DEVID		0xa04
+#define SYS_LSI_DEVID_REV	GENMASK(31, 28)
+#define SYS_LSI_DEVID_SPECIFIC	GENMASK(27, 0)
 #define SYS_USB_PWRRDY		0xd70
 #define SYS_USB_PWRRDY_PWRRDY_N	BIT(0)
 #define SYS_MAX_REG		0xe20
@@ -24,7 +27,16 @@ static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __in
 	}
 };
 
+static const struct rz_sysc_soc_id_init_data rzg3s_sysc_soc_id_init_data __initconst = {
+	.family = "RZ/G3S",
+	.id = 0x85e0447,
+	.offset = SYS_LSI_DEVID,
+	.revision_mask = SYS_LSI_DEVID_REV,
+	.specific_id_mask = SYS_LSI_DEVID_SPECIFIC
+};
+
 const struct rz_sysc_init_data rzg3s_sysc_init_data = {
+	.soc_id_init_data = &rzg3s_sysc_soc_id_init_data,
 	.signals_init_data = rzg3s_sysc_signals_init_data,
 	.num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
 	.max_register_offset = SYS_MAX_REG,
diff --git a/drivers/soc/renesas/renesas-soc.c b/drivers/soc/renesas/renesas-soc.c
index 172d59e6fbcf..425d9037dcd0 100644
--- a/drivers/soc/renesas/renesas-soc.c
+++ b/drivers/soc/renesas/renesas-soc.c
@@ -71,10 +71,6 @@ static const struct renesas_family fam_rzg2ul __initconst __maybe_unused = {
 	.name	= "RZ/G2UL",
 };
 
-static const struct renesas_family fam_rzg3s __initconst __maybe_unused = {
-	.name	= "RZ/G3S",
-};
-
 static const struct renesas_family fam_rzv2h __initconst __maybe_unused = {
 	.name	= "RZ/V2H",
 };
@@ -176,11 +172,6 @@ static const struct renesas_soc soc_rz_g2ul __initconst __maybe_unused = {
 	.id     = 0x8450447,
 };
 
-static const struct renesas_soc soc_rz_g3s __initconst __maybe_unused = {
-	.family = &fam_rzg3s,
-	.id	= 0x85e0447,
-};
-
 static const struct renesas_soc soc_rz_v2h __initconst __maybe_unused = {
 	.family = &fam_rzv2h,
 	.id     = 0x847a447,
@@ -410,9 +401,6 @@ static const struct of_device_id renesas_socs[] __initconst __maybe_unused = {
 #ifdef CONFIG_ARCH_R9A07G054
 	{ .compatible = "renesas,r9a07g054",	.data = &soc_rz_v2l },
 #endif
-#ifdef CONFIG_ARCH_R9A08G045
-	{ .compatible = "renesas,r9a08g045",	.data = &soc_rz_g3s },
-#endif
 #ifdef CONFIG_ARCH_R9A09G011
 	{ .compatible = "renesas,r9a09g011",	.data = &soc_rz_v2m },
 #endif
-- 
2.39.2

[PATCH v2 06/15] dt-bindings: usb: renesas,usbhs: Document RZ/G3S SoC

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The USBHS IP block on RZ/G3S SoC is identitcal to the one found on the
RZ/G2L device. Document the RZ/G3S USBHS IP block.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- this was patch 09/16 in v1
- collected tags

 Documentation/devicetree/bindings/usb/renesas,usbhs.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml b/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml
index b23ef29bf794..980f325341d4 100644
--- a/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml
+++ b/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml
@@ -26,6 +26,7 @@ properties:
               - renesas,usbhs-r9a07g043 # RZ/G2UL and RZ/Five
               - renesas,usbhs-r9a07g044 # RZ/G2{L,LC}
               - renesas,usbhs-r9a07g054 # RZ/V2L
+              - renesas,usbhs-r9a08g045 # RZ/G3S
           - const: renesas,rzg2l-usbhs
 
       - items:
@@ -130,6 +131,7 @@ allOf:
               - renesas,usbhs-r9a07g043
               - renesas,usbhs-r9a07g044
               - renesas,usbhs-r9a07g054
+              - renesas,usbhs-r9a08g045
     then:
       properties:
         interrupts:
-- 
2.39.2

[PATCH v2 07/15] dt-bindings: phy: renesas,usb2-phy: Mark resets as required for RZ/G3S

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The reset lines are mandatory for the Renesas RZ/G3S platform and must be
explicitly defined in device tree.

Fixes: f3c849855114 ("dt-bindings: phy: renesas,usb2-phy: Document RZ/G3S phy bindings")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new


 Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml b/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
index af275cea3456..2babd200bd98 100644
--- a/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
@@ -105,7 +105,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: renesas,rzg2l-usb2-phy
+            enum:
+              - renesas,rzg2l-usb2-phy
+              - renesas,usb2-phy-r9a08g045
     then:
       required:
         - resets
-- 
2.39.2

[PATCH v2 08/15] dt-bindings: phy: renesas,usb2-phy: Add renesas,sysc-signal

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

On the Renesas RZ/G3S SoC, the USB PHY receives a signal from the system
controller that need to be de-asserted/asserted when power is turned
on/off. This signal, called PWRRDY, is controlled through a specific
register in the system controller memory space.

Add the renesas,sysc-signal DT property to describe the relation b/w the
system controller and the USB PHY on the Renesas RZ/G3S. This property
provides a phandle to the system controller, along with the offset within
the system controller memory space that manages the signal and a bitmask
that indicates the specific bits required to control the signal.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new


 .../bindings/phy/renesas,usb2-phy.yaml        | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml b/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
index 2babd200bd98..3b8dcacc3740 100644
--- a/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
@@ -85,6 +85,16 @@ properties:
 
   dr_mode: true
 
+  renesas,sysc-signal:
+    description: System controller phandle, specifying the register
+      offset and bitmask associated with a specific system controller signal
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: system controller phandle
+          - description: register offset associated with a signal
+          - description: register bitmask associated with a signal
+
 if:
   properties:
     compatible:
@@ -112,6 +122,18 @@ allOf:
       required:
         - resets
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,usb2-phy-r9a08g045
+    then:
+      required:
+        - renesas,sysc-signal
+    else:
+      properties:
+        renesas,sysc-signal: false
+
 additionalProperties: false
 
 examples:
-- 
2.39.2

[PATCH v2 09/15] phy: renesas: rcar-gen3-usb2: Fix an error handling path in rcar_gen3_phy_usb2_probe()

[Claudiu]

From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

If an error occurs after the reset_control_deassert(),
reset_control_assert() must be called, as already done in the remove
function.

Use devm_add_action_or_reset() to add the missing call and simplify the
.remove() function accordingly.

Fixes: 4eae16375357 ("phy: renesas: rcar-gen3-usb2: Add support to initialize the bus")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
[claudiu.beznea: removed "struct reset_control *rstc = data;" from
 rcar_gen3_reset_assert()]
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new; re-spinned the Christophe's work at
  https://lore.kernel.org/all/TYCPR01MB113329930BA5E2149C9BE2A1986672@TYCPR01MB11332.jpnprd01.prod.outlook.com/


 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 775f4f973a6c..59f74aa993ac 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -668,6 +668,11 @@ static enum usb_dr_mode rcar_gen3_get_dr_mode(struct device_node *np)
 	return candidate;
 }
 
+static void rcar_gen3_reset_assert(void *data)
+{
+	reset_control_assert(data);
+}
+
 static int rcar_gen3_phy_usb2_init_bus(struct rcar_gen3_chan *channel)
 {
 	struct device *dev = channel->dev;
@@ -686,6 +691,11 @@ static int rcar_gen3_phy_usb2_init_bus(struct rcar_gen3_chan *channel)
 	if (ret)
 		goto rpm_put;
 
+	ret = devm_add_action_or_reset(dev, rcar_gen3_reset_assert,
+				       channel->rstc);
+	if (ret)
+		goto rpm_put;
+
 	val = readl(channel->base + USB2_AHB_BUS_CTR);
 	val &= ~USB2_AHB_BUS_CTR_MBL_MASK;
 	val |= USB2_AHB_BUS_CTR_MBL_INCR4;
@@ -815,7 +825,6 @@ static void rcar_gen3_phy_usb2_remove(struct platform_device *pdev)
 	if (channel->is_otg_channel)
 		device_remove_file(&pdev->dev, &dev_attr_role);
 
-	reset_control_assert(channel->rstc);
 	pm_runtime_disable(&pdev->dev);
 };
 
-- 
2.39.2

[PATCH v2 10/15] phy: renesas: rcar-gen3-usb2: Add support for PWRRDY

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

On the Renesas RZ/G3S SoC, the USB PHY has an input signal called PWRRDY.
This signal is managed by the system controller and must be de-asserted
after powering on the area where USB PHY resides and asserted before
powering it off.

The connection b/w the system controller and the USB PHY is implemented
through the renesas,sysc-signal device tree property. This property
specifies the register offset and the bitmask required to control the
signal. The system controller exports the syscon regmap, and the read/write
access to the memory area of the PWRRDY signal is reference-counted, as the
same system controller signal is connected to both RZ/G3S USB PHYs.

Add support for the PWRRDY signal control.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new


 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 66 ++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 59f74aa993ac..84459755adf5 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -12,12 +12,14 @@
 #include <linux/extcon-provider.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/string.h>
@@ -111,6 +113,12 @@ struct rcar_gen3_phy {
 	bool powered;
 };
 
+struct rcar_gen3_pwrrdy {
+	struct regmap *regmap;
+	u32 offset;
+	u32 mask;
+};
+
 struct rcar_gen3_chan {
 	void __iomem *base;
 	struct device *dev;	/* platform_device's device */
@@ -118,6 +126,7 @@ struct rcar_gen3_chan {
 	struct rcar_gen3_phy rphys[NUM_OF_PHYS];
 	struct regulator *vbus;
 	struct reset_control *rstc;
+	struct rcar_gen3_pwrrdy *pwrrdy;
 	struct work_struct work;
 	struct mutex lock;	/* protects rphys[...].powered */
 	enum usb_dr_mode dr_mode;
@@ -133,6 +142,7 @@ struct rcar_gen3_phy_drv_data {
 	const struct phy_ops *phy_usb2_ops;
 	bool no_adp_ctrl;
 	bool init_bus;
+	bool pwrrdy;
 };
 
 /*
@@ -587,6 +597,7 @@ static const struct rcar_gen3_phy_drv_data rz_g3s_phy_usb2_data = {
 	.phy_usb2_ops = &rcar_gen3_phy_usb2_ops,
 	.no_adp_ctrl = true,
 	.init_bus = true,
+	.pwrrdy = true,
 };
 
 static const struct of_device_id rcar_gen3_phy_usb2_match_table[] = {
@@ -707,6 +718,55 @@ static int rcar_gen3_phy_usb2_init_bus(struct rcar_gen3_chan *channel)
 	return ret;
 }
 
+static void rcar_gen3_phy_usb2_set_pwrrdy(struct rcar_gen3_chan *channel, bool power_on)
+{
+	struct rcar_gen3_pwrrdy *pwrrdy = channel->pwrrdy;
+
+	/* N/A on this platform. */
+	if (!pwrrdy)
+		return;
+
+	regmap_update_bits(pwrrdy->regmap, pwrrdy->offset, pwrrdy->mask, !power_on);
+}
+
+static void rcar_gen3_phy_usb2_pwrrdy_off(void *data)
+{
+	rcar_gen3_phy_usb2_set_pwrrdy(data, false);
+}
+
+static int rcar_gen3_phy_usb2_init_pwrrdy(struct rcar_gen3_chan *channel)
+{
+	struct device *dev = channel->dev;
+	struct rcar_gen3_pwrrdy *pwrrdy;
+	struct of_phandle_args args;
+	int ret;
+
+	pwrrdy = devm_kzalloc(dev, sizeof(*pwrrdy), GFP_KERNEL);
+	if (!pwrrdy)
+		return -ENOMEM;
+
+	ret = of_parse_phandle_with_args(dev->of_node, "renesas,sysc-signal",
+					 "#renesas,sysc-signal-cells", 0, &args);
+	if (ret)
+		return ret;
+
+	pwrrdy->regmap = syscon_node_to_regmap(args.np);
+	pwrrdy->offset = args.args[0];
+	pwrrdy->mask = args.args[1];
+
+	of_node_put(args.np);
+
+	if (IS_ERR(pwrrdy->regmap))
+		return PTR_ERR(pwrrdy->regmap);
+
+	channel->pwrrdy = pwrrdy;
+
+	/* Power it ON. */
+	rcar_gen3_phy_usb2_set_pwrrdy(channel, true);
+
+	return devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_pwrrdy_off, channel);
+}
+
 static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 {
 	const struct rcar_gen3_phy_drv_data *phy_data;
@@ -763,6 +823,12 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, channel);
 	channel->dev = dev;
 
+	if (phy_data->pwrrdy) {
+		ret = rcar_gen3_phy_usb2_init_pwrrdy(channel);
+		if (ret)
+			goto error;
+	}
+
 	if (phy_data->init_bus) {
 		ret = rcar_gen3_phy_usb2_init_bus(channel);
 		if (ret)
-- 
2.39.2

[PATCH v2 11/15] dt-bindings: reset: renesas,rzg2l-usbphy-ctrl: Document RZ/G3S support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S USB PHY control block is similar with the one found on
the Renesas RZ/G2L. Add documentation for it.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new

 .../devicetree/bindings/reset/renesas,rzg2l-usbphy-ctrl.yaml     | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/reset/renesas,rzg2l-usbphy-ctrl.yaml b/Documentation/devicetree/bindings/reset/renesas,rzg2l-usbphy-ctrl.yaml
index b0b20af15313..ae59c2dcadbf 100644
--- a/Documentation/devicetree/bindings/reset/renesas,rzg2l-usbphy-ctrl.yaml
+++ b/Documentation/devicetree/bindings/reset/renesas,rzg2l-usbphy-ctrl.yaml
@@ -20,6 +20,7 @@ properties:
           - renesas,r9a07g043-usbphy-ctrl # RZ/G2UL and RZ/Five
           - renesas,r9a07g044-usbphy-ctrl # RZ/G2{L,LC}
           - renesas,r9a07g054-usbphy-ctrl # RZ/V2L
+          - renesas,r9a08g045-usbphy-ctrl # RZ/G3S
       - const: renesas,rzg2l-usbphy-ctrl
 
   reg:
-- 
2.39.2

[PATCH v2 12/15] arm64: dts: renesas: Add #renesas,sysc-signal-cells to system controller node

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The system controller on RZ/G3S can provide control access to its signals.
To enable this, add the #renesas,sysc-signal-cells DT property. Consumers
can use the renesas,sysc-signal DT property to reference the specific SYSC
signal that needs to be controlled.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new


 arch/arm64/boot/dts/renesas/r9a07g043.dtsi | 3 ++-
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 3 ++-
 arch/arm64/boot/dts/renesas/r9a07g054.dtsi | 3 ++-
 arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
index 593c66b27ad1..2ebb951e6a39 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
@@ -585,8 +585,9 @@ cpg: clock-controller@11010000 {
 		};
 
 		sysc: system-controller@11020000 {
-			compatible = "renesas,r9a07g043-sysc";
+			compatible = "renesas,r9a07g043-sysc", "syscon";
 			reg = <0 0x11020000 0 0x10000>;
+			#renesas,sysc-signal-cells = <2>;
 			status = "disabled";
 		};
 
diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 6b1c77cd8261..9dd229cbf288 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -877,7 +877,7 @@ cpg: clock-controller@11010000 {
 		};
 
 		sysc: system-controller@11020000 {
-			compatible = "renesas,r9a07g044-sysc";
+			compatible = "renesas,r9a07g044-sysc", "syscon";
 			reg = <0 0x11020000 0 0x10000>;
 			interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
@@ -885,6 +885,7 @@ sysc: system-controller@11020000 {
 				     <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "lpm_int", "ca55stbydone_int",
 					  "cm33stbyr_int", "ca55_deny";
+			#renesas,sysc-signal-cells = <2>;
 			status = "disabled";
 		};
 
diff --git a/arch/arm64/boot/dts/renesas/r9a07g054.dtsi b/arch/arm64/boot/dts/renesas/r9a07g054.dtsi
index 01f59914dd09..31550b8c3143 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g054.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g054.dtsi
@@ -884,7 +884,7 @@ cpg: clock-controller@11010000 {
 		};
 
 		sysc: system-controller@11020000 {
-			compatible = "renesas,r9a07g054-sysc";
+			compatible = "renesas,r9a07g054-sysc", "syscon";
 			reg = <0 0x11020000 0 0x10000>;
 			interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
@@ -892,6 +892,7 @@ sysc: system-controller@11020000 {
 				     <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "lpm_int", "ca55stbydone_int",
 					  "cm33stbyr_int", "ca55_deny";
+			#renesas,sysc-signal-cells = <2>;
 			status = "disabled";
 		};
 
diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
index be8a0a768c65..169561386f35 100644
--- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
@@ -198,7 +198,7 @@ cpg: clock-controller@11010000 {
 		};
 
 		sysc: system-controller@11020000 {
-			compatible = "renesas,r9a08g045-sysc";
+			compatible = "renesas,r9a08g045-sysc", "syscon";
 			reg = <0 0x11020000 0 0x10000>;
 			interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
@@ -206,6 +206,7 @@ sysc: system-controller@11020000 {
 				     <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "lpm_int", "ca55stbydone_int",
 					  "cm33stbyr_int", "ca55_deny";
+			#renesas,sysc-signal-cells = <2>;
 			status = "disabled";
 		};
 
-- 
2.39.2

[PATCH v2 13/15] arm64: dts: renesas: r9a08g045: Enable the system controller

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Enable the system controller. It is needed for USB and SoC
identification.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new

 arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
index 169561386f35..89cf57eb8389 100644
--- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
@@ -207,7 +207,6 @@ sysc: system-controller@11020000 {
 			interrupt-names = "lpm_int", "ca55stbydone_int",
 					  "cm33stbyr_int", "ca55_deny";
 			#renesas,sysc-signal-cells = <2>;
-			status = "disabled";
 		};
 
 		pinctrl: pinctrl@11030000 {
-- 
2.39.2

[PATCH v2 14/15] arm64: dts: renesas: r9a08g045: Add USB support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Add USB nodes for the Renesas RZ/G3S SoC. This consists of PHY reset,
host and device support.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- this was patch 14/16 in v1
- added renesas,sysc-signal properties to USB PHYs
- collected tags
- Geert: I kept your tag; please let me know if you consider otherwise 

 arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 119 +++++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
index 89cf57eb8389..6ce94bbecfa6 100644
--- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
@@ -417,6 +417,125 @@ eth1: ethernet@11c40000 {
 			status = "disabled";
 		};
 
+		phyrst: usbphy-ctrl@11e00000 {
+			compatible = "renesas,r9a08g045-usbphy-ctrl", "renesas,rzg2l-usbphy-ctrl";
+			reg = <0 0x11e00000 0 0x10000>;
+			clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>;
+			resets = <&cpg R9A08G045_USB_PRESETN>;
+			power-domains = <&cpg>;
+			#reset-cells = <1>;
+			status = "disabled";
+
+			usb0_vbus_otg: regulator-vbus {
+				regulator-name = "vbus";
+			};
+		};
+
+		ohci0: usb@11e10000 {
+			compatible = "generic-ohci";
+			reg = <0 0x11e10000 0 0x100>;
+			interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>,
+				 <&cpg CPG_MOD R9A08G045_USB_U2H0_HCLK>;
+			resets = <&phyrst 0>,
+				 <&cpg R9A08G045_USB_U2H0_HRESETN>;
+			phys = <&usb2_phy0 1>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		ehci0: usb@11e10100 {
+			compatible = "generic-ehci";
+			reg = <0 0x11e10100 0 0x100>;
+			interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>,
+				 <&cpg CPG_MOD R9A08G045_USB_U2H0_HCLK>;
+			resets = <&phyrst 0>,
+				 <&cpg R9A08G045_USB_U2H0_HRESETN>;
+			phys = <&usb2_phy0 2>;
+			phy-names = "usb";
+			companion = <&ohci0>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		usb2_phy0: usb-phy@11e10200 {
+			compatible = "renesas,usb2-phy-r9a08g045";
+			reg = <0 0x11e10200 0 0x700>;
+			interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>,
+				 <&cpg CPG_MOD R9A08G045_USB_U2H0_HCLK>;
+			resets = <&phyrst 0>,
+				 <&cpg R9A08G045_USB_U2H0_HRESETN>;
+			#phy-cells = <1>;
+			power-domains = <&cpg>;
+			renesas,sysc-signal = <&sysc 0xd70 0x1>;
+			status = "disabled";
+		};
+
+		hsusb: usb@11e20000 {
+			compatible = "renesas,usbhs-r9a08g045",
+				     "renesas,rzg2l-usbhs";
+			reg = <0 0x11e20000 0 0x10000>;
+			interrupts = <GIC_SPI 85 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>,
+				 <&cpg CPG_MOD R9A08G045_USB_U2P_EXR_CPUCLK>;
+			resets = <&phyrst 0>,
+				 <&cpg R9A08G045_USB_U2P_EXL_SYSRST>;
+			renesas,buswait = <7>;
+			phys = <&usb2_phy0 3>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		ohci1: usb@11e30000 {
+			compatible = "generic-ohci";
+			reg = <0 0x11e30000 0 0x100>;
+			interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>,
+				 <&cpg CPG_MOD R9A08G045_USB_U2H1_HCLK>;
+			resets = <&phyrst 1>,
+				 <&cpg R9A08G045_USB_U2H1_HRESETN>;
+			phys = <&usb2_phy1 1>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		ehci1: usb@11e30100 {
+			compatible = "generic-ehci";
+			reg = <0 0x11e30100 0 0x100>;
+			interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>,
+				 <&cpg CPG_MOD R9A08G045_USB_U2H1_HCLK>;
+			resets = <&phyrst 1>,
+				 <&cpg R9A08G045_USB_U2H1_HRESETN>;
+			phys = <&usb2_phy1 2>;
+			phy-names = "usb";
+			companion = <&ohci1>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		usb2_phy1: usb-phy@11e30200 {
+			compatible = "renesas,usb2-phy-r9a08g045";
+			reg = <0 0x11e30200 0 0x700>;
+			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>,
+				 <&cpg CPG_MOD R9A08G045_USB_U2H1_HCLK>;
+			resets = <&phyrst 1>,
+				 <&cpg R9A08G045_USB_U2H1_HRESETN>;
+			#phy-cells = <1>;
+			power-domains = <&cpg>;
+			renesas,sysc-signal = <&sysc 0xd70 0x1>;
+			status = "disabled";
+		};
+
 		gic: interrupt-controller@12400000 {
 			compatible = "arm,gic-v3";
 			#interrupt-cells = <3>;
-- 
2.39.2

[PATCH v2 15/15] arm64: dts: renesas: rzg3s-smarc: Enable USB support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Enable USB support (host, device, USB PHYs).

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- this was patch 15/16 in v1:
- dropped sysc enablement as it is now done in SoC dtsi file

 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi | 57 ++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
index 4509151344c4..84523e771ebf 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
@@ -64,12 +64,35 @@ vccq_sdhi1: regulator-vccq-sdhi1 {
 	};
 };
 
+&ehci0 {
+	dr_mode = "otg";
+	status = "okay";
+};
+
+&ehci1 {
+	status = "okay";
+};
+
+&hsusb {
+	dr_mode = "otg";
+	status = "okay";
+};
+
 &i2c0 {
 	status = "okay";
 
 	clock-frequency = <1000000>;
 };
 
+&ohci0 {
+	dr_mode = "otg";
+	status = "okay";
+};
+
+&ohci1 {
+	status = "okay";
+};
+
 &pinctrl {
 	key-1-gpio-hog {
 		gpio-hog;
@@ -128,6 +151,27 @@ cd {
 			pinmux = <RZG2L_PORT_PINMUX(0, 2, 1)>; /* SD1_CD */
 		};
 	};
+
+	usb0_pins: usb0 {
+		peri {
+			pinmux = <RZG2L_PORT_PINMUX(5, 0, 1)>, /* VBUS */
+				 <RZG2L_PORT_PINMUX(5, 2, 1)>; /* OVC */
+		};
+
+		otg {
+			pinmux = <RZG2L_PORT_PINMUX(5, 3, 1)>; /* OTG_ID */
+			bias-pull-up;
+		};
+	};
+
+	usb1_pins: usb1 {
+		pinmux = <RZG2L_PORT_PINMUX(5, 4, 5)>, /* OVC */
+			 <RZG2L_PORT_PINMUX(6, 0, 1)>; /* VBUS */
+	};
+};
+
+&phyrst {
+	status = "okay";
 };
 
 &scif0 {
@@ -148,3 +192,16 @@ &sdhi1 {
 	max-frequency = <125000000>;
 	status = "okay";
 };
+
+&usb2_phy0 {
+	pinctrl-0 = <&usb0_pins>;
+	pinctrl-names = "default";
+	vbus-supply = <&usb0_vbus_otg>;
+	status = "okay";
+};
+
+&usb2_phy1 {
+	pinctrl-0 = <&usb1_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+};
-- 
2.39.2

RE: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

[Biju Das]

Hi Claudiu,

Thanks for the patch.

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: 26 November 2024 09:21
> Subject: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The RZ/G3S system controller (SYSC) has various registers that control signals specific to individual
> IPs. IP drivers must control these signals at different configuration phases.
> 
> Add SYSC driver that allows individual SYSC consumers to control these signals. The SYSC driver
> exports a syscon regmap enabling IP drivers to use a specific SYSC offset and mask from the device
> tree, which can then be accessed through regmap_update_bits().
> 
> Currently, the SYSC driver provides control to the USB PWRRDY signal, which is routed to the USB PHY.
> This signal needs to be managed before or after powering the USB PHY off or on.
> 
> Other SYSC signals candidates (as exposed in the the hardware manual of the RZ/G3S SoC) include:
> 
> * PCIe:
> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>   register
> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> 
> * SPI:
> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>   register
> 
> * I2C/I3C:
> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>   (x=0..3)
> - af_bypass I3C signal controlled through SYS_I3C_CFG register
> 
> * Ethernet:
> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>   registers (x=0..1)
> 
> As different Renesas RZ SoC shares most of the SYSC functionalities available on the RZ/G3S SoC, the
> driver if formed of a SYSC core part and a SoC specific part allowing individual SYSC SoC to provide
> functionalities to the SYSC core.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Cheers,
Biju

> ---
> 
> Change in v2:
> - this was patch 04/16 in v1
> - dropped the initial approach proposed in v1 where a with a reset
>   controller driver was proposed to handle the USB PWRRDY signal
> - implemented it with syscon regmap and the SYSC signal concept
>   (introduced in this patch)
> 
>  drivers/soc/renesas/Kconfig          |   7 +
>  drivers/soc/renesas/Makefile         |   2 +
>  drivers/soc/renesas/r9a08g045-sysc.c |  31 +++
>  drivers/soc/renesas/rz-sysc.c        | 286 +++++++++++++++++++++++++++
>  drivers/soc/renesas/rz-sysc.h        |  52 +++++
>  5 files changed, 378 insertions(+)
>  create mode 100644 drivers/soc/renesas/r9a08g045-sysc.c
>  create mode 100644 drivers/soc/renesas/rz-sysc.c  create mode 100644 drivers/soc/renesas/rz-sysc.h
> 
> diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig index
> 9f7fe02310b9..0686c3ad9e27 100644
> --- a/drivers/soc/renesas/Kconfig
> +++ b/drivers/soc/renesas/Kconfig
> @@ -378,4 +378,11 @@ config PWC_RZV2M
>  config RST_RCAR
>  	bool "Reset Controller support for R-Car" if COMPILE_TEST
> 
> +config SYSC_RZ
> +	bool "System controller for RZ SoCs" if COMPILE_TEST
> +
> +config SYSC_R9A08G045
> +	bool "Renesas RZ/G3S System controller support" if COMPILE_TEST
> +	select SYSC_RZ
> +
>  endif # SOC_RENESAS
> diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile index
> 734f8f8cefa4..8cd139b3dd0a 100644
> --- a/drivers/soc/renesas/Makefile
> +++ b/drivers/soc/renesas/Makefile
> @@ -6,7 +6,9 @@ obj-$(CONFIG_SOC_RENESAS)	+= renesas-soc.o
>  ifdef CONFIG_SMP
>  obj-$(CONFIG_ARCH_R9A06G032)	+= r9a06g032-smp.o
>  endif
> +obj-$(CONFIG_SYSC_R9A08G045)	+= r9a08g045-sysc.o
> 
>  # Family
>  obj-$(CONFIG_PWC_RZV2M)		+= pwc-rzv2m.o
>  obj-$(CONFIG_RST_RCAR)		+= rcar-rst.o
> +obj-$(CONFIG_SYSC_RZ)		+= rz-sysc.o
> diff --git a/drivers/soc/renesas/r9a08g045-sysc.c b/drivers/soc/renesas/r9a08g045-sysc.c
> new file mode 100644
> index 000000000000..ceea738aee72
> --- /dev/null
> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RZ/G3S System controller driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/init.h>
> +
> +#include "rz-sysc.h"
> +
> +#define SYS_USB_PWRRDY		0xd70
> +#define SYS_USB_PWRRDY_PWRRDY_N	BIT(0)
> +#define SYS_MAX_REG		0xe20
> +
> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
> +	{
> +		.name = "usb-pwrrdy",
> +		.offset = SYS_USB_PWRRDY,
> +		.mask = SYS_USB_PWRRDY_PWRRDY_N,
> +		.refcnt_incr_val = 0
> +	}
> +};
> +
> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {
> +	.signals_init_data = rzg3s_sysc_signals_init_data,
> +	.num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
> +	.max_register_offset = SYS_MAX_REG,
> +};
> diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c new file mode 100644 index
> 000000000000..dc0edacd7170
> --- /dev/null
> +++ b/drivers/soc/renesas/rz-sysc.c
> @@ -0,0 +1,286 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RZ System controller driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/dcache.h>
> +#include <linux/debugfs.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/refcount.h>
> +#include <linux/regmap.h>
> +#include <linux/seq_file.h>
> +
> +#include "rz-sysc.h"
> +
> +/**
> + * struct rz_sysc - RZ SYSC private data structure
> + * @base: SYSC base address
> + * @dev: SYSC device pointer
> + * @signals: SYSC signals
> + * @num_signals: number of SYSC signals  */ struct rz_sysc {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct rz_sysc_signal *signals;
> +	u8 num_signals;
> +};
> +
> +static int rz_sysc_reg_read(void *context, unsigned int off, unsigned
> +int *val) {
> +	struct rz_sysc *sysc = context;
> +
> +	*val = readl(sysc->base + off);
> +
> +	return 0;
> +}
> +
> +static struct rz_sysc_signal *rz_sysc_off_to_signal(struct rz_sysc *sysc, unsigned int offset,
> +						    unsigned int mask)
> +{
> +	struct rz_sysc_signal *signals = sysc->signals;
> +
> +	for (u32 i = 0; i < sysc->num_signals; i++) {
> +		if (signals[i].init_data->offset != offset)
> +			continue;
> +
> +		/*
> +		 * In case mask == 0 we just return the signal data w/o checking the mask.
> +		 * This is useful when calling through rz_sysc_reg_write() to check
> +		 * if the requested setting is for a mapped signal or not.
> +		 */
> +		if (mask) {
> +			if (signals[i].init_data->mask == mask)
> +				return &signals[i];
> +		} else {
> +			return &signals[i];
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static int rz_sysc_reg_update_bits(void *context, unsigned int off,
> +				   unsigned int mask, unsigned int val) {
> +	struct rz_sysc *sysc = context;
> +	struct rz_sysc_signal *signal;
> +	bool update = false;
> +
> +	signal = rz_sysc_off_to_signal(sysc, off, mask);
> +	if (signal) {
> +		if (signal->init_data->refcnt_incr_val == val) {
> +			if (!refcount_read(&signal->refcnt)) {
> +				refcount_set(&signal->refcnt, 1);
> +				update = true;
> +			} else {
> +				refcount_inc(&signal->refcnt);
> +			}
> +		} else {
> +			update = refcount_dec_and_test(&signal->refcnt);
> +		}
> +	} else {
> +		update = true;
> +	}
> +
> +	if (update) {
> +		u32 tmp;
> +
> +		tmp = readl(sysc->base + off);
> +		tmp &= ~mask;
> +		tmp |= val & mask;
> +		writel(tmp, sysc->base + off);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rz_sysc_reg_write(void *context, unsigned int off, unsigned
> +int val) {
> +	struct rz_sysc *sysc = context;
> +	struct rz_sysc_signal *signal;
> +
> +	/*
> +	 * Force using regmap_update_bits() for signals to have reference counter
> +	 * per individual signal in case there are multiple signals controlled
> +	 * through the same register.
> +	 */
> +	signal = rz_sysc_off_to_signal(sysc, off, 0);
> +	if (signal) {
> +		dev_err(sysc->dev,
> +			"regmap_write() not allowed on register controlling a signal. Use
> regmap_update_bits()!");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	writel(val, sysc->base + off);
> +
> +	return 0;
> +}
> +
> +static bool rz_sysc_writeable_reg(struct device *dev, unsigned int off)
> +{
> +	struct rz_sysc *sysc = dev_get_drvdata(dev);
> +	struct rz_sysc_signal *signal;
> +
> +	/* Any register containing a signal is writeable. */
> +	signal = rz_sysc_off_to_signal(sysc, off, 0);
> +	if (signal)
> +		return true;
> +
> +	return false;
> +}
> +
> +static bool rz_sysc_readable_reg(struct device *dev, unsigned int off)
> +{
> +	struct rz_sysc *sysc = dev_get_drvdata(dev);
> +	struct rz_sysc_signal *signal;
> +
> +	/* Any register containing a signal is readable. */
> +	signal = rz_sysc_off_to_signal(sysc, off, 0);
> +	if (signal)
> +		return true;
> +
> +	return false;
> +}
> +
> +static int rz_sysc_signals_show(struct seq_file *s, void *what) {
> +	struct rz_sysc *sysc = s->private;
> +
> +	seq_printf(s, "%-20s Enable count\n", "Signal");
> +	seq_printf(s, "%-20s ------------\n", "--------------------");
> +
> +	for (u8 i = 0; i < sysc->num_signals; i++) {
> +		seq_printf(s, "%-20s %d\n", sysc->signals[i].init_data->name,
> +			   refcount_read(&sysc->signals[i].refcnt));
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(rz_sysc_signals);
> +
> +static void rz_sysc_debugfs_remove(void *data) {
> +	debugfs_remove_recursive(data);
> +}
> +
> +static int rz_sysc_signals_init(struct rz_sysc *sysc,
> +				const struct rz_sysc_signal_init_data *init_data,
> +				u32 num_signals)
> +{
> +	struct dentry *root;
> +	int ret;
> +
> +	sysc->signals = devm_kcalloc(sysc->dev, num_signals, sizeof(*sysc->signals),
> +				     GFP_KERNEL);
> +	if (!sysc->signals)
> +		return -ENOMEM;
> +
> +	for (u32 i = 0; i < num_signals; i++) {
> +		struct rz_sysc_signal_init_data *id;
> +
> +		id = devm_kzalloc(sysc->dev, sizeof(*id), GFP_KERNEL);
> +		if (!id)
> +			return -ENOMEM;
> +
> +		id->name = devm_kstrdup(sysc->dev, init_data->name, GFP_KERNEL);
> +		if (!id->name)
> +			return -ENOMEM;
> +
> +		id->offset = init_data->offset;
> +		id->mask = init_data->mask;
> +		id->refcnt_incr_val = init_data->refcnt_incr_val;
> +
> +		sysc->signals[i].init_data = id;
> +		refcount_set(&sysc->signals[i].refcnt, 0);
> +	}
> +
> +	sysc->num_signals = num_signals;
> +
> +	root = debugfs_create_dir("renesas-rz-sysc", NULL);
> +	ret = devm_add_action_or_reset(sysc->dev, rz_sysc_debugfs_remove, root);
> +	if (ret)
> +		return ret;
> +	debugfs_create_file("signals", 0444, root, sysc,
> +&rz_sysc_signals_fops);
> +
> +	return 0;
> +}
> +
> +static struct regmap_config rz_sysc_regmap = {
> +	.name = "rz_sysc_regs",
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.fast_io = true,
> +	.reg_read = rz_sysc_reg_read,
> +	.reg_write = rz_sysc_reg_write,
> +	.reg_update_bits = rz_sysc_reg_update_bits,
> +	.writeable_reg = rz_sysc_writeable_reg,
> +	.readable_reg = rz_sysc_readable_reg,
> +};
> +
> +static const struct of_device_id rz_sysc_match[] = { #ifdef
> +CONFIG_SYSC_R9A08G045
> +	{ .compatible = "renesas,r9a08g045-sysc", .data =
> +&rzg3s_sysc_init_data }, #endif
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rz_sysc_match);
> +
> +static int rz_sysc_probe(struct platform_device *pdev) {
> +	const struct rz_sysc_init_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct rz_sysc *sysc;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	data = device_get_match_data(dev);
> +	if (!data || !data->max_register_offset)
> +		return -EINVAL;
> +
> +	sysc = devm_kzalloc(dev, sizeof(*sysc), GFP_KERNEL);
> +	if (!sysc)
> +		return -ENOMEM;
> +
> +	sysc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(sysc->base))
> +		return PTR_ERR(sysc->base);
> +
> +	sysc->dev = dev;
> +
> +	ret = rz_sysc_signals_init(sysc, data->signals_init_data, data->num_signals);
> +	if (ret)
> +		return ret;
> +
> +	dev_set_drvdata(dev, sysc);
> +	rz_sysc_regmap.max_register = data->max_register_offset;
> +	regmap = devm_regmap_init(dev, NULL, sysc, &rz_sysc_regmap);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	return of_syscon_register_regmap(dev->of_node, regmap); }
> +
> +static struct platform_driver rz_sysc_driver = {
> +	.driver = {
> +		.name = "renesas-rz-sysc",
> +		.of_match_table = rz_sysc_match
> +	},
> +	.probe = rz_sysc_probe
> +};
> +
> +static int __init rz_sysc_init(void)
> +{
> +	return platform_driver_register(&rz_sysc_driver);
> +}
> +subsys_initcall(rz_sysc_init);
> +
> +MODULE_DESCRIPTION("Renesas RZ System Controller Driver");
> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/soc/renesas/rz-sysc.h b/drivers/soc/renesas/rz-sysc.h new file mode 100644 index
> 000000000000..bb850310c931
> --- /dev/null
> +++ b/drivers/soc/renesas/rz-sysc.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Renesas RZ System Controller
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#ifndef __SOC_RENESAS_RZ_SYSC_H__
> +#define __SOC_RENESAS_RZ_SYSC_H__
> +
> +#include <linux/refcount.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct rz_sysc_signal_init_data - RZ SYSC signals init data
> + * @name: signal name
> + * @offset: register offset controling this signal
> + * @mask: bitmask in register specific to this signal
> + * @refcnt_incr_val: increment refcnt when setting this value  */
> +struct rz_sysc_signal_init_data {
> +	const char *name;
> +	u32 offset;
> +	u32 mask;
> +	u32 refcnt_incr_val;
> +};
> +
> +/**
> + * struct rz_sysc_signal - RZ SYSC signals
> + * @init_data: signals initialization data
> + * @refcnt: reference counter
> + */
> +struct rz_sysc_signal {
> +	const struct rz_sysc_signal_init_data *init_data;
> +	refcount_t refcnt;
> +};
> +
> +/**
> + * struct rz_sysc_init_data - RZ SYSC initialization data
> + * @signals_init_data: RZ SYSC signals initialization data
> + * @num_signals: number of SYSC signals
> + * @max_register_offset: Maximum SYSC register offset to be used by the
> +regmap config  */ struct rz_sysc_init_data {
> +	const struct rz_sysc_signal_init_data *signals_init_data;
> +	u32 num_signals;
> +	u32 max_register_offset;
> +};
> +
> +extern const struct rz_sysc_init_data rzg3s_sysc_init_data;
> +
> +#endif /* __SOC_RENESAS_RZ_SYSC_H__ */
> --
> 2.39.2
> 

RE: [PATCH v2 04/15] soc: renesas: rz-sysc: Add SoC detection support

[Biju Das]

Hi Claudiu,

Thanks for the patch.

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: 26 November 2024 09:21
> Subject: [PATCH v2 04/15] soc: renesas: rz-sysc: Add SoC detection support
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The RZ SYSC controller has registers that keep the SoC ID data. Add driver support to retrieve the SoC
> ID and register a SoC driver.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Cheers,
Biju

> ---
> 
> Changes in v2:
> - this was patch 05/16 in v1
> - changed patch title and description
> - added SoC initialization code in its own function
> - addressed the review comments
> - introduced struct rz_sysc_soc_id_init_data and adjusted the code
>   accordingly
> - dropped the RZ/G3S SoC detection code (it will be introduced in
>   a separate patch)
> 
>  drivers/soc/renesas/rz-sysc.c | 72 +++++++++++++++++++++++++++++++++--
>  drivers/soc/renesas/rz-sysc.h | 18 +++++++++
>  2 files changed, 86 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c index
> dc0edacd7170..d34d295831b8 100644
> --- a/drivers/soc/renesas/rz-sysc.c
> +++ b/drivers/soc/renesas/rz-sysc.c
> @@ -14,9 +14,12 @@
>  #include <linux/refcount.h>
>  #include <linux/regmap.h>
>  #include <linux/seq_file.h>
> +#include <linux/sys_soc.h>
> 
>  #include "rz-sysc.h"
> 
> +#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> +
>  /**
>   * struct rz_sysc - RZ SYSC private data structure
>   * @base: SYSC base address
> @@ -211,6 +214,59 @@ static int rz_sysc_signals_init(struct rz_sysc *sysc,
>  	return 0;
>  }
> 
> +static int rz_sysc_soc_init(struct rz_sysc *sysc, const struct
> +of_device_id *match) {
> +	const struct rz_sysc_init_data *sysc_data = match->data;
> +	const struct rz_sysc_soc_id_init_data *soc_data = sysc_data->soc_id_init_data;
> +	struct soc_device_attribute *soc_dev_attr;
> +	const char *soc_id_start, *soc_id_end;
> +	u32 val, revision, specific_id;
> +	struct soc_device *soc_dev;
> +	char soc_id[32] = {0};
> +	u8 size;
> +
> +	if (!soc_data || !soc_data->family || !soc_data->offset ||
> +	    !soc_data->revision_mask)
> +		return -EINVAL;
> +
> +	soc_id_start = strchr(match->compatible, ',') + 1;
> +	soc_id_end = strchr(match->compatible, '-');
> +	size = soc_id_end - soc_id_start;
> +	if (size > 32)
> +		size = 32;
> +	strscpy(soc_id, soc_id_start, size);
> +
> +	soc_dev_attr = devm_kzalloc(sysc->dev, sizeof(*soc_dev_attr), GFP_KERNEL);
> +	if (!soc_dev_attr)
> +		return -ENOMEM;
> +
> +	soc_dev_attr->family = soc_data->family;
> +	soc_dev_attr->soc_id = devm_kstrdup(sysc->dev, soc_id, GFP_KERNEL);
> +	if (!soc_dev_attr->soc_id)
> +		return -ENOMEM;
> +
> +	val = readl(sysc->base + soc_data->offset);
> +	revision = field_get(soc_data->revision_mask, val);
> +	specific_id = field_get(soc_data->specific_id_mask, val);
> +	soc_dev_attr->revision = devm_kasprintf(sysc->dev, GFP_KERNEL, "%u", revision);
> +	if (!soc_dev_attr->revision)
> +		return -ENOMEM;
> +
> +	if (soc_data->id && specific_id != soc_data->id) {
> +		dev_warn(sysc->dev, "SoC mismatch (product = 0x%x)\n", specific_id);
> +		return -ENODEV;
> +	}
> +
> +	dev_info(sysc->dev, "Detected Renesas %s %s Rev %s\n", soc_dev_attr->family,
> +		 soc_dev_attr->soc_id, soc_dev_attr->revision);
> +
> +	soc_dev = soc_device_register(soc_dev_attr);
> +	if (IS_ERR(soc_dev))
> +		return PTR_ERR(soc_dev);
> +
> +	return 0;
> +}
> +
>  static struct regmap_config rz_sysc_regmap = {
>  	.name = "rz_sysc_regs",
>  	.reg_bits = 32,
> @@ -235,14 +291,15 @@ MODULE_DEVICE_TABLE(of, rz_sysc_match);  static int rz_sysc_probe(struct
> platform_device *pdev)  {
>  	const struct rz_sysc_init_data *data;
> +	const struct of_device_id *match;
>  	struct device *dev = &pdev->dev;
> -	struct rz_sysc *sysc;
>  	struct regmap *regmap;
> +	struct rz_sysc *sysc;
>  	int ret;
> 
> -	data = device_get_match_data(dev);
> -	if (!data || !data->max_register_offset)
> -		return -EINVAL;
> +	match = of_match_node(rz_sysc_match, dev->of_node);
> +	if (!match || !match->data)
> +		return -ENODEV;
> 
>  	sysc = devm_kzalloc(dev, sizeof(*sysc), GFP_KERNEL);
>  	if (!sysc)
> @@ -253,6 +310,13 @@ static int rz_sysc_probe(struct platform_device *pdev)
>  		return PTR_ERR(sysc->base);
> 
>  	sysc->dev = dev;
> +	ret = rz_sysc_soc_init(sysc, match);
> +	if (ret)
> +		return ret;
> +
> +	data = match->data;
> +	if (!data->max_register_offset)
> +		return -EINVAL;
> 
>  	ret = rz_sysc_signals_init(sysc, data->signals_init_data, data->num_signals);
>  	if (ret)
> diff --git a/drivers/soc/renesas/rz-sysc.h b/drivers/soc/renesas/rz-sysc.h index
> bb850310c931..babca9c743c7 100644
> --- a/drivers/soc/renesas/rz-sysc.h
> +++ b/drivers/soc/renesas/rz-sysc.h
> @@ -35,13 +35,31 @@ struct rz_sysc_signal {
>  	refcount_t refcnt;
>  };
> 
> +/**
> + * struct rz_syc_soc_id_init_data - RZ SYSC SoC identification
> +initialization data
> + * @family: RZ SoC family
> + * @id: RZ SoC expected ID
> + * @offset: SYSC SoC ID register offset
> + * @revision_mask: SYSC SoC ID revision mask
> + * @specific_id_mask: SYSC SoC ID specific ID mask  */ struct
> +rz_sysc_soc_id_init_data {
> +	const char * const family;
> +	u32 id;
> +	u32 offset;
> +	u32 revision_mask;
> +	u32 specific_id_mask;
> +};
> +
>  /**
>   * struct rz_sysc_init_data - RZ SYSC initialization data
> + * @soc_id_init_data: RZ SYSC SoC ID initialization data
>   * @signals_init_data: RZ SYSC signals initialization data
>   * @num_signals: number of SYSC signals
>   * @max_register_offset: Maximum SYSC register offset to be used by the regmap config
>   */
>  struct rz_sysc_init_data {
> +	const struct rz_sysc_soc_id_init_data *soc_id_init_data;
>  	const struct rz_sysc_signal_init_data *signals_init_data;
>  	u32 num_signals;
>  	u32 max_register_offset;
> --
> 2.39.2
> 

Re: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

[Geert Uytterhoeven]

Hi Biju,

On Tue, Nov 26, 2024 at 10:28 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > -----Original Message-----
> > From: Claudiu <claudiu.beznea@tuxon.dev>
> > Sent: 26 November 2024 09:21
> > Subject: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family
> >
> > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> > The RZ/G3S system controller (SYSC) has various registers that control signals specific to individual
> > IPs. IP drivers must control these signals at different configuration phases.
> >
> > Add SYSC driver that allows individual SYSC consumers to control these signals. The SYSC driver
> > exports a syscon regmap enabling IP drivers to use a specific SYSC offset and mask from the device
> > tree, which can then be accessed through regmap_update_bits().
> >
> > Currently, the SYSC driver provides control to the USB PWRRDY signal, which is routed to the USB PHY.
> > This signal needs to be managed before or after powering the USB PHY off or on.
> >
> > Other SYSC signals candidates (as exposed in the the hardware manual of the RZ/G3S SoC) include:
> >
> > * PCIe:
> > - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> > - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
> >   register
> > - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> >
> > * SPI:
> > - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
> >   register
> >
> > * I2C/I3C:
> > - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
> >   (x=0..3)
> > - af_bypass I3C signal controlled through SYS_I3C_CFG register
> >
> > * Ethernet:
> > - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
> >   registers (x=0..1)
> >
> > As different Renesas RZ SoC shares most of the SYSC functionalities available on the RZ/G3S SoC, the
> > driver if formed of a SYSC core part and a SoC specific part allowing individual SYSC SoC to provide
> > functionalities to the SYSC core.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your review!

> > ---
> >
> > Change in v2:
> > - this was patch 04/16 in v1
> > - dropped the initial approach proposed in v1 where a with a reset
> >   controller driver was proposed to handle the USB PWRRDY signal
> > - implemented it with syscon regmap and the SYSC signal concept
> >   (introduced in this patch)

[...]

When reviewing, please trim your response, so other people don't have
to scroll through hundreds of lines of quoted text, to find any other
comments (if any).

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

[Biju Das]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 26 November 2024 13:42
> Subject: Re: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family
> 
> Hi Biju,
> 
> On Tue, Nov 26, 2024 at 10:28 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > -----Original Message-----
> > > From: Claudiu <claudiu.beznea@tuxon.dev>
> > > Sent: 26 November 2024 09:21
> > > Subject: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas
> > > RZ family
> > >
> > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >
> > > The RZ/G3S system controller (SYSC) has various registers that
> > > control signals specific to individual IPs. IP drivers must control these signals at different
> configuration phases.
> > >
> > > Add SYSC driver that allows individual SYSC consumers to control
> > > these signals. The SYSC driver exports a syscon regmap enabling IP
> > > drivers to use a specific SYSC offset and mask from the device tree, which can then be accessed
> through regmap_update_bits().
> > >
> > > Currently, the SYSC driver provides control to the USB PWRRDY signal, which is routed to the USB
> PHY.
> > > This signal needs to be managed before or after powering the USB PHY off or on.
> > >
> > > Other SYSC signals candidates (as exposed in the the hardware manual of the RZ/G3S SoC) include:
> > >
> > > * PCIe:
> > > - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> > > - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
> > >   register
> > > - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> > >
> > > * SPI:
> > > - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
> > >   register
> > >
> > > * I2C/I3C:
> > > - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
> > >   (x=0..3)
> > > - af_bypass I3C signal controlled through SYS_I3C_CFG register
> > >
> > > * Ethernet:
> > > - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
> > >   registers (x=0..1)
> > >
> > > As different Renesas RZ SoC shares most of the SYSC functionalities
> > > available on the RZ/G3S SoC, the driver if formed of a SYSC core
> > > part and a SoC specific part allowing individual SYSC SoC to provide functionalities to the SYSC
> core.
> > >
> > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your review!
> 
> > > ---
> > >
> > > Change in v2:
> > > - this was patch 04/16 in v1
> > > - dropped the initial approach proposed in v1 where a with a reset
> > >   controller driver was proposed to handle the USB PWRRDY signal
> > > - implemented it with syscon regmap and the SYSC signal concept
> > >   (introduced in this patch)
> 
> [...]
> 
> When reviewing, please trim your response, so other people don't have to scroll through hundreds of
> lines of quoted text, to find any other comments (if any).

Sorry for that. Will take care next time.

Cheers,
Biju

Re: [PATCH v2 11/15] dt-bindings: reset: renesas,rzg2l-usbphy-ctrl: Document RZ/G3S support

[Conor Dooley]

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

On Tue, Nov 26, 2024 at 11:20:46AM +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The Renesas RZ/G3S USB PHY control block is similar with the one found on
> the Renesas RZ/G2L. Add documentation for it.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

Re: [PATCH v2 07/15] dt-bindings: phy: renesas,usb2-phy: Mark resets as required for RZ/G3S

[Conor Dooley]

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

On Tue, Nov 26, 2024 at 11:20:42AM +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The reset lines are mandatory for the Renesas RZ/G3S platform and must be
> explicitly defined in device tree.
> 
> Fixes: f3c849855114 ("dt-bindings: phy: renesas,usb2-phy: Document RZ/G3S phy bindings")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

Re: [PATCH v2 10/15] phy: renesas: rcar-gen3-usb2: Add support for PWRRDY

[Geert Uytterhoeven]

Hi Claudiu,

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> On the Renesas RZ/G3S SoC, the USB PHY has an input signal called PWRRDY.
> This signal is managed by the system controller and must be de-asserted
> after powering on the area where USB PHY resides and asserted before
> powering it off.
>
> The connection b/w the system controller and the USB PHY is implemented
> through the renesas,sysc-signal device tree property. This property
> specifies the register offset and the bitmask required to control the
> signal. The system controller exports the syscon regmap, and the read/write
> access to the memory area of the PWRRDY signal is reference-counted, as the
> same system controller signal is connected to both RZ/G3S USB PHYs.
>
> Add support for the PWRRDY signal control.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -707,6 +718,55 @@ static int rcar_gen3_phy_usb2_init_bus(struct rcar_gen3_chan *channel)
>         return ret;
>  }
>
> +static void rcar_gen3_phy_usb2_set_pwrrdy(struct rcar_gen3_chan *channel, bool power_on)
> +{
> +       struct rcar_gen3_pwrrdy *pwrrdy = channel->pwrrdy;
> +
> +       /* N/A on this platform. */
> +       if (!pwrrdy)
> +               return;

This cannot happen?

> +
> +       regmap_update_bits(pwrrdy->regmap, pwrrdy->offset, pwrrdy->mask, !power_on);
> +}
> +
> +static void rcar_gen3_phy_usb2_pwrrdy_off(void *data)
> +{
> +       rcar_gen3_phy_usb2_set_pwrrdy(data, false);
> +}
> +
> +static int rcar_gen3_phy_usb2_init_pwrrdy(struct rcar_gen3_chan *channel)
> +{
> +       struct device *dev = channel->dev;
> +       struct rcar_gen3_pwrrdy *pwrrdy;
> +       struct of_phandle_args args;
> +       int ret;
> +
> +       pwrrdy = devm_kzalloc(dev, sizeof(*pwrrdy), GFP_KERNEL);
> +       if (!pwrrdy)
> +               return -ENOMEM;
> +
> +       ret = of_parse_phandle_with_args(dev->of_node, "renesas,sysc-signal",
> +                                        "#renesas,sysc-signal-cells", 0, &args);
> +       if (ret)
> +               return ret;
> +
> +       pwrrdy->regmap = syscon_node_to_regmap(args.np);
> +       pwrrdy->offset = args.args[0];
> +       pwrrdy->mask = args.args[1];
> +
> +       of_node_put(args.np);
> +
> +       if (IS_ERR(pwrrdy->regmap))
> +               return PTR_ERR(pwrrdy->regmap);
> +
> +       channel->pwrrdy = pwrrdy;
> +
> +       /* Power it ON. */
> +       rcar_gen3_phy_usb2_set_pwrrdy(channel, true);
> +
> +       return devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_pwrrdy_off, channel);
> +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 12/15] arm64: dts: renesas: Add #renesas,sysc-signal-cells to system controller node

[Geert Uytterhoeven]

Hi Claudiu,

Thanks for your patch!

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The system controller on RZ/G3S can provide control access to its signals.

Actually all SoCs from the "RZ/G2L" family...

> To enable this, add the #renesas,sysc-signal-cells DT property. Consumers
> can use the renesas,sysc-signal DT property to reference the specific SYSC
> signal that needs to be controlled.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v2:
> - none; this patch is new
>
>
>  arch/arm64/boot/dts/renesas/r9a07g043.dtsi | 3 ++-
>  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 3 ++-
>  arch/arm64/boot/dts/renesas/r9a07g054.dtsi | 3 ++-
>  arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 3 ++-

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

[Geert Uytterhoeven]

Hi Claudiu,

Thanks for your patch!

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The RZ/G3S system controller (SYSC) has various registers that control
> signals specific to individual IPs. IP drivers must control these signals
> at different configuration phases.
>
> Add SYSC driver that allows individual SYSC consumers to control these
> signals. The SYSC driver exports a syscon regmap enabling IP drivers to
> use a specific SYSC offset and mask from the device tree, which can then be
> accessed through regmap_update_bits().
>
> Currently, the SYSC driver provides control to the USB PWRRDY signal, which
> is routed to the USB PHY. This signal needs to be managed before or after
> powering the USB PHY off or on.
>
> Other SYSC signals candidates (as exposed in the the hardware manual of the

s/the the/the/

> RZ/G3S SoC) include:
>
> * PCIe:
> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>   register
> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>
> * SPI:
> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>   register
>
> * I2C/I3C:
> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>   (x=0..3)
> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>
> * Ethernet:
> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>   registers (x=0..1)
>
> As different Renesas RZ SoC shares most of the SYSC functionalities
> available on the RZ/G3S SoC, the driver if formed of a SYSC core
> part and a SoC specific part allowing individual SYSC SoC to provide
> functionalities to the SYSC core.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

> --- /dev/null
> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RZ/G3S System controller driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/init.h>
> +
> +#include "rz-sysc.h"
> +
> +#define SYS_USB_PWRRDY         0xd70
> +#define SYS_USB_PWRRDY_PWRRDY_N        BIT(0)
> +#define SYS_MAX_REG            0xe20
> +
> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {

This is marked __initconst...

> +       {
> +               .name = "usb-pwrrdy",
> +               .offset = SYS_USB_PWRRDY,
> +               .mask = SYS_USB_PWRRDY_PWRRDY_N,
> +               .refcnt_incr_val = 0
> +       }
> +};
> +
> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {

... but this is not __init, causing a section mismatch.

> +       .signals_init_data = rzg3s_sysc_signals_init_data,
> +       .num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
> +       .max_register_offset = SYS_MAX_REG,
> +};

> --- /dev/null
> +++ b/drivers/soc/renesas/rz-sysc.c

> +/**
> + * struct rz_sysc - RZ SYSC private data structure
> + * @base: SYSC base address
> + * @dev: SYSC device pointer
> + * @signals: SYSC signals
> + * @num_signals: number of SYSC signals
> + */
> +struct rz_sysc {
> +       void __iomem *base;
> +       struct device *dev;
> +       struct rz_sysc_signal *signals;
> +       u8 num_signals;

You could change signals to a flexible array at the end, tag it with
__counted_by(num_signals), and allocate space for both struct rz_sysc
and the signals array using struct_size(), reducing the number of
allocations.

> +};

> +static struct rz_sysc_signal *rz_sysc_off_to_signal(struct rz_sysc *sysc, unsigned int offset,
> +                                                   unsigned int mask)
> +{
> +       struct rz_sysc_signal *signals = sysc->signals;
> +
> +       for (u32 i = 0; i < sysc->num_signals; i++) {

s/u32/unsigned int/

> +               if (signals[i].init_data->offset != offset)
> +                       continue;
> +
> +               /*
> +                * In case mask == 0 we just return the signal data w/o checking the mask.
> +                * This is useful when calling through rz_sysc_reg_write() to check
> +                * if the requested setting is for a mapped signal or not.
> +                */
> +               if (mask) {
> +                       if (signals[i].init_data->mask == mask)
> +                               return &signals[i];
> +               } else {
> +                       return &signals[i];
> +               }

if (!mask || signals[i].init_data->mask == mask)
        return &signals[i];

> +       }
> +
> +       return NULL;
> +}
> +
> +static int rz_sysc_reg_update_bits(void *context, unsigned int off,
> +                                  unsigned int mask, unsigned int val)
> +{
> +       struct rz_sysc *sysc = context;
> +       struct rz_sysc_signal *signal;
> +       bool update = false;
> +
> +       signal = rz_sysc_off_to_signal(sysc, off, mask);
> +       if (signal) {
> +               if (signal->init_data->refcnt_incr_val == val) {
> +                       if (!refcount_read(&signal->refcnt)) {
> +                               refcount_set(&signal->refcnt, 1);
> +                               update = true;
> +                       } else {
> +                               refcount_inc(&signal->refcnt);
> +                       }
> +               } else {
> +                       update = refcount_dec_and_test(&signal->refcnt);
> +               }
> +       } else {
> +               update = true;
> +       }

You could reduce indentation/number of lines by reordering the logic:

    if (!signal) {
            update = true;
    } else if (signal->init_data->refcnt_incr_val != val) {
            update = refcount_dec_and_test(&signal->refcnt);
    } else if (!refcount_read(&signal->refcnt)) {
            refcount_set(&signal->refcnt, 1);
            update = true;
    } else {
            refcount_inc(&signal->refcnt);
    }

> +
> +       if (update) {
> +               u32 tmp;
> +
> +               tmp = readl(sysc->base + off);
> +               tmp &= ~mask;
> +               tmp |= val & mask;
> +               writel(tmp, sysc->base + off);
> +       }
> +
> +       return 0;
> +}
> +
> +static int rz_sysc_reg_write(void *context, unsigned int off, unsigned int val)
> +{
> +       struct rz_sysc *sysc = context;
> +       struct rz_sysc_signal *signal;
> +
> +       /*
> +        * Force using regmap_update_bits() for signals to have reference counter
> +        * per individual signal in case there are multiple signals controlled
> +        * through the same register.
> +        */
> +       signal = rz_sysc_off_to_signal(sysc, off, 0);
> +       if (signal) {
> +               dev_err(sysc->dev,
> +                       "regmap_write() not allowed on register controlling a signal. Use regmap_update_bits()!");
> +               return -EOPNOTSUPP;
> +       }
> +

Can you ever get here, given rz_sysc_writeable_reg() below would have
returned false? If not, is there any point in having this function?

> +       writel(val, sysc->base + off);
> +
> +       return 0;
> +}
> +
> +static bool rz_sysc_writeable_reg(struct device *dev, unsigned int off)
> +{
> +       struct rz_sysc *sysc = dev_get_drvdata(dev);
> +       struct rz_sysc_signal *signal;
> +
> +       /* Any register containing a signal is writeable. */
> +       signal = rz_sysc_off_to_signal(sysc, off, 0);
> +       if (signal)
> +               return true;
> +
> +       return false;
> +}

> +static int rz_sysc_signals_show(struct seq_file *s, void *what)
> +{
> +       struct rz_sysc *sysc = s->private;
> +
> +       seq_printf(s, "%-20s Enable count\n", "Signal");
> +       seq_printf(s, "%-20s ------------\n", "--------------------");
> +
> +       for (u8 i = 0; i < sysc->num_signals; i++) {
> +               seq_printf(s, "%-20s %d\n", sysc->signals[i].init_data->name,
> +                          refcount_read(&sysc->signals[i].refcnt));
> +       }
> +
> +       return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(rz_sysc_signals);

What is the use-case for this? Just (initial) debugging?

> +
> +static void rz_sysc_debugfs_remove(void *data)
> +{
> +       debugfs_remove_recursive(data);
> +}
> +
> +static int rz_sysc_signals_init(struct rz_sysc *sysc,
> +                               const struct rz_sysc_signal_init_data *init_data,
> +                               u32 num_signals)
> +{
> +       struct dentry *root;
> +       int ret;
> +
> +       sysc->signals = devm_kcalloc(sysc->dev, num_signals, sizeof(*sysc->signals),
> +                                    GFP_KERNEL);
> +       if (!sysc->signals)
> +               return -ENOMEM;
> +
> +       for (u32 i = 0; i < num_signals; i++) {

unsigned int

> +               struct rz_sysc_signal_init_data *id;
> +
> +               id = devm_kzalloc(sysc->dev, sizeof(*id), GFP_KERNEL);
> +               if (!id)
> +                       return -ENOMEM;
> +
> +               id->name = devm_kstrdup(sysc->dev, init_data->name, GFP_KERNEL);
> +               if (!id->name)
> +                       return -ENOMEM;
> +
> +               id->offset = init_data->offset;
> +               id->mask = init_data->mask;
> +               id->refcnt_incr_val = init_data->refcnt_incr_val;
> +
> +               sysc->signals[i].init_data = id;
> +               refcount_set(&sysc->signals[i].refcnt, 0);
> +       }
> +
> +       sysc->num_signals = num_signals;
> +
> +       root = debugfs_create_dir("renesas-rz-sysc", NULL);
> +       ret = devm_add_action_or_reset(sysc->dev, rz_sysc_debugfs_remove, root);
> +       if (ret)
> +               return ret;
> +       debugfs_create_file("signals", 0444, root, sysc, &rz_sysc_signals_fops);
> +
> +       return 0;
> +}

> --- /dev/null
> +++ b/drivers/soc/renesas/rz-sysc.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Renesas RZ System Controller
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#ifndef __SOC_RENESAS_RZ_SYSC_H__
> +#define __SOC_RENESAS_RZ_SYSC_H__
> +
> +#include <linux/refcount.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct rz_sysc_signal_init_data - RZ SYSC signals init data
> + * @name: signal name
> + * @offset: register offset controling this signal
> + * @mask: bitmask in register specific to this signal
> + * @refcnt_incr_val: increment refcnt when setting this value
> + */
> +struct rz_sysc_signal_init_data {
> +       const char *name;
> +       u32 offset;
> +       u32 mask;
> +       u32 refcnt_incr_val;
> +};
> +
> +/**
> + * struct rz_sysc_signal - RZ SYSC signals
> + * @init_data: signals initialization data
> + * @refcnt: reference counter
> + */
> +struct rz_sysc_signal {
> +       const struct rz_sysc_signal_init_data *init_data;

Can't you just embed struct rz_sysc_signal_init_data?
That way you could allocate the rz_sysc_signal and
rz_sysc_signal_init_data structures in a single allocation.

> +       refcount_t refcnt;
> +};

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells

[Geert Uytterhoeven]

Hi Claudiu,

CC Ulf

Thanks for your patch!

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The RZ/G3S system controller (SYSC) has registers to control signals that
> are routed to various IPs. These signals must be controlled during
> configuration of the respective IPs. One such signal is the USB PWRRDY,
> which connects the SYSC and the USB PHY. This signal must to be controlled
> before and after the power to the USB PHY is turned off/on.
>
> Other similar signals include the following (according to the RZ/G3S
> hardware manual):
>
> * PCIe:
> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>   register
> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>
> * SPI:
> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>   register
>
> * I2C/I3C:
> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>   (x=0..3)
> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>
> * Ethernet:
> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>   registers (x=0..1)
>
> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals
> consumers to manage these signals.
>
> The goal is to enable consumers to specify the required access data for
> these signals (through device tree) and let their respective drivers
> control these signals via the syscon regmap provided by the system
> controller driver. For example, the USB PHY will describe this relation
> using the following DT property:
>
> usb2_phy1: usb-phy@11e30200 {
>         // ...
>         renesas,sysc-signal = <&sysc 0xd70 0x1>;
>         // ...
> };

IIUIC, the consumer driver will  appear to control the SYSC bits
directly, but due to the use of custom validating regmap accessors
and reference counting in the SYSC driver, this is safe?
The extra safety requires duplicating the register bits in both DT
and the SYSC driver.
Both usb-phy nodes on RZG3S use the same renesas,sysc-signal, so the
reference counting is indeed needed.  They are in different power
domains, could that be an issue w.r.t. ordering?

I am not a big fan of describing register bits in DT, but for the other
SYSC users you list above, syscon+regmap seems to be a valid solution.
For USB and PCIe control, the situation is different. I more liked the
approach with "reset IDs" you had in v1, as it abstracts the DT
description from the register bits, and the USB and PCIe reset bits use
a different polarity (on RZ/G3S). If future SoC integration changes
the polarity, you have to handle that in the consumer (USB or PCIe)
driver, too.  Unfortunately such "reset IDs" are only suitable for
use with the reset or pmdomain frameworks, which didn't survive the
earlier discussions.

One other option would be to let SYSC expose regulators?
While that would work for USB and PCIe control, we would still need
syscon+regmap for the other bits.

So the more I think about it, the more I like your (clever) solution...

> Along with it, add the syscon to the compatible list as it will be
> requested by the consumer drivers. The syscon was added to the rest of
> system controller variants as these are similar with RZ/G3S and can
> benefit from the implementation proposed in this series.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells

[Claudiu Beznea]

Hi, Geert,

On 28.11.2024 17:46, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> CC Ulf
> 
> Thanks for your patch!
> 
> On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The RZ/G3S system controller (SYSC) has registers to control signals that
>> are routed to various IPs. These signals must be controlled during
>> configuration of the respective IPs. One such signal is the USB PWRRDY,
>> which connects the SYSC and the USB PHY. This signal must to be controlled
>> before and after the power to the USB PHY is turned off/on.
>>
>> Other similar signals include the following (according to the RZ/G3S
>> hardware manual):
>>
>> * PCIe:
>> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
>> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>>   register
>> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>>
>> * SPI:
>> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>>   register
>>
>> * I2C/I3C:
>> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>>   (x=0..3)
>> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>>
>> * Ethernet:
>> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>>   registers (x=0..1)
>>
>> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals
>> consumers to manage these signals.
>>
>> The goal is to enable consumers to specify the required access data for
>> these signals (through device tree) and let their respective drivers
>> control these signals via the syscon regmap provided by the system
>> controller driver. For example, the USB PHY will describe this relation
>> using the following DT property:
>>
>> usb2_phy1: usb-phy@11e30200 {
>>         // ...
>>         renesas,sysc-signal = <&sysc 0xd70 0x1>;
>>         // ...
>> };
> 
> IIUIC, the consumer driver will  appear to control the SYSC bits
> directly, but due to the use of custom validating regmap accessors
> and reference counting in the SYSC driver, this is safe?

I'm not sure I fully understand the safety concern.

> The extra safety requires duplicating the register bits in both DT
> and the SYSC driver.

One other option I saw was to have common defines for registers that could
have been shared b/w driver and DTSes. But it looked better to me the way
it has been presented in this series.

> Both usb-phy nodes on RZG3S use the same renesas,sysc-signal, so the
> reference counting is indeed needed.  They are in different power
> domains, could that be an issue w.r.t. ordering?

In chapter "32.4.2.1 USB/PHY related pins", section "When either Port1 or
Port2 is unused" of the RZ/G3S HW manual it is mentioned "Since USB_VDD18 /
USB_VDD33 are common to 2 Port PHY, it is necessary to supply power even
when one of the
 ports is not in use".

(From the discussions w/ the internal HW team) The PWRRDY is an (software
controlled) indicator to the USB PHY that power supply is ready.

From that and [1] I get that both PHYs are powered by the same regulators
(USB_VDD18/USB_VDD33) and the USB PWRRDY signal need to be set before/after
the USB PHY power off/on. Because of this I consider the order doesn't matter.

[1] https://gcdnb.pbrd.co/images/0a1zYBFZXZVb.png

> 
> I am not a big fan of describing register bits in DT,

+1

> but for the other
> SYSC users you list above, syscon+regmap seems to be a valid solution.
> For USB and PCIe control, the situation is different. I more liked the
> approach with "reset IDs" you had in v1, as it abstracts the DT
> description from the register bits,

+1

> and the USB and PCIe reset bits use
> a different polarity (on RZ/G3S). If future SoC integration changes
> the polarity, you have to handle that in the consumer (USB or PCIe)
> driver, too.

That's true. The idea of this implementation was that the consumer would
know what they need to set for themselves on the SYSC side.

> Unfortunately such "reset IDs" are only suitable for
> use with the reset or pmdomain frameworks, which didn't survive the
> earlier discussions.
> 
> One other option would be to let SYSC expose regulators?

We can try, but we can hit the wall again, as the PWRRDY signal is not a
regulator either. From the internal HW discussion is an indicator (software
controlled) that the power to the USB PHY is ready.

> While that would work for USB and PCIe control, we would still need
> syscon+regmap for the other bits.

That is true.

> 
> So the more I think about it, the more I like your (clever) solution...
> 
>> Along with it, add the syscon to the compatible list as it will be
>> requested by the consumer drivers. The syscon was added to the rest of
>> system controller variants as these are similar with RZ/G3S and can
>> benefit from the implementation proposed in this series.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells

[Geert Uytterhoeven]

Hi Claudiu,

On Fri, Nov 29, 2024 at 9:21 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 28.11.2024 17:46, Geert Uytterhoeven wrote:
> > On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> The RZ/G3S system controller (SYSC) has registers to control signals that
> >> are routed to various IPs. These signals must be controlled during
> >> configuration of the respective IPs. One such signal is the USB PWRRDY,
> >> which connects the SYSC and the USB PHY. This signal must to be controlled
> >> before and after the power to the USB PHY is turned off/on.
> >>
> >> Other similar signals include the following (according to the RZ/G3S
> >> hardware manual):
> >>
> >> * PCIe:
> >> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> >> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
> >>   register
> >> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> >>
> >> * SPI:
> >> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
> >>   register
> >>
> >> * I2C/I3C:
> >> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
> >>   (x=0..3)
> >> - af_bypass I3C signal controlled through SYS_I3C_CFG register
> >>
> >> * Ethernet:
> >> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
> >>   registers (x=0..1)
> >>
> >> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals
> >> consumers to manage these signals.
> >>
> >> The goal is to enable consumers to specify the required access data for
> >> these signals (through device tree) and let their respective drivers
> >> control these signals via the syscon regmap provided by the system
> >> controller driver. For example, the USB PHY will describe this relation
> >> using the following DT property:
> >>
> >> usb2_phy1: usb-phy@11e30200 {
> >>         // ...
> >>         renesas,sysc-signal = <&sysc 0xd70 0x1>;
> >>         // ...
> >> };
> >
> > IIUIC, the consumer driver will  appear to control the SYSC bits
> > directly, but due to the use of custom validating regmap accessors
> > and reference counting in the SYSC driver, this is safe?
>
> I'm not sure I fully understand the safety concern.

Sorry for my bad expression, this was more like a rhetorical question.
I meant that it is safe because:
  1. Consumers cannot perform arbitrary register accesses,
  2. The reference counting guarantees correct operation, despite
      both usb-phy nodes using the same renesas,sysc-signal.

So everything is fine.

> > The extra safety requires duplicating the register bits in both DT
> > and the SYSC driver.
>
> One other option I saw was to have common defines for registers that could
> have been shared b/w driver and DTSes. But it looked better to me the way
> it has been presented in this series.
>
> > Both usb-phy nodes on RZG3S use the same renesas,sysc-signal, so the
> > reference counting is indeed needed.  They are in different power
> > domains, could that be an issue w.r.t. ordering?
>
> In chapter "32.4.2.1 USB/PHY related pins", section "When either Port1 or
> Port2 is unused" of the RZ/G3S HW manual it is mentioned "Since USB_VDD18 /
> USB_VDD33 are common to 2 Port PHY, it is necessary to supply power even
> when one of the
>  ports is not in use".

Does that mean you have to power the other PHY on through the
CPG_BUS_PERI_COM_MSTOP register, too?
(I know you haven't added R9A08G045_PD_USBx to the USB nodes yet,
 as #power-domain-cells is still 0).

> (From the discussions w/ the internal HW team) The PWRRDY is an (software
> controlled) indicator to the USB PHY that power supply is ready.
>
> From that and [1] I get that both PHYs are powered by the same regulators
> (USB_VDD18/USB_VDD33) and the USB PWRRDY signal need to be set before/after
> the USB PHY power off/on. Because of this I consider the order doesn't matter.
>
> [1] https://gcdnb.pbrd.co/images/0a1zYBFZXZVb.png

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

[Claudiu Beznea]

Hi, Geert,

On 28.11.2024 17:24, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> Thanks for your patch!
> 
> On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The RZ/G3S system controller (SYSC) has various registers that control
>> signals specific to individual IPs. IP drivers must control these signals
>> at different configuration phases.
>>
>> Add SYSC driver that allows individual SYSC consumers to control these
>> signals. The SYSC driver exports a syscon regmap enabling IP drivers to
>> use a specific SYSC offset and mask from the device tree, which can then be
>> accessed through regmap_update_bits().
>>
>> Currently, the SYSC driver provides control to the USB PWRRDY signal, which
>> is routed to the USB PHY. This signal needs to be managed before or after
>> powering the USB PHY off or on.
>>
>> Other SYSC signals candidates (as exposed in the the hardware manual of the
> 
> s/the the/the/
> 
>> RZ/G3S SoC) include:
>>
>> * PCIe:
>> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
>> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>>   register
>> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>>
>> * SPI:
>> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>>   register
>>
>> * I2C/I3C:
>> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>>   (x=0..3)
>> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>>
>> * Ethernet:
>> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>>   registers (x=0..1)
>>
>> As different Renesas RZ SoC shares most of the SYSC functionalities
>> available on the RZ/G3S SoC, the driver if formed of a SYSC core
>> part and a SoC specific part allowing individual SYSC SoC to provide
>> functionalities to the SYSC core.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
>> --- /dev/null
>> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * RZ/G3S System controller driver
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/array_size.h>
>> +#include <linux/bits.h>
>> +#include <linux/init.h>
>> +
>> +#include "rz-sysc.h"
>> +
>> +#define SYS_USB_PWRRDY         0xd70
>> +#define SYS_USB_PWRRDY_PWRRDY_N        BIT(0)
>> +#define SYS_MAX_REG            0xe20
>> +
>> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
> 
> This is marked __initconst...
> 
>> +       {
>> +               .name = "usb-pwrrdy",
>> +               .offset = SYS_USB_PWRRDY,
>> +               .mask = SYS_USB_PWRRDY_PWRRDY_N,
>> +               .refcnt_incr_val = 0
>> +       }
>> +};
>> +
>> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {
> 
> ... but this is not __init, causing a section mismatch.

Do you know if there is a way to detect this?

> 
>> +       .signals_init_data = rzg3s_sysc_signals_init_data,
>> +       .num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
>> +       .max_register_offset = SYS_MAX_REG,
>> +};
> 
>> --- /dev/null
>> +++ b/drivers/soc/renesas/rz-sysc.c
> 
>> +/**
>> + * struct rz_sysc - RZ SYSC private data structure
>> + * @base: SYSC base address
>> + * @dev: SYSC device pointer
>> + * @signals: SYSC signals
>> + * @num_signals: number of SYSC signals
>> + */
>> +struct rz_sysc {
>> +       void __iomem *base;
>> +       struct device *dev;
>> +       struct rz_sysc_signal *signals;
>> +       u8 num_signals;
> 
> You could change signals to a flexible array at the end, tag it with
> __counted_by(num_signals), and allocate space for both struct rz_sysc
> and the signals array using struct_size(), reducing the number of
> allocations.

I'll look into this.

> 
>> +};
> 
>> +static struct rz_sysc_signal *rz_sysc_off_to_signal(struct rz_sysc *sysc, unsigned int offset,
>> +                                                   unsigned int mask)
>> +{
>> +       struct rz_sysc_signal *signals = sysc->signals;
>> +
>> +       for (u32 i = 0; i < sysc->num_signals; i++) {
> 
> s/u32/unsigned int/
> 
>> +               if (signals[i].init_data->offset != offset)
>> +                       continue;
>> +
>> +               /*
>> +                * In case mask == 0 we just return the signal data w/o checking the mask.
>> +                * This is useful when calling through rz_sysc_reg_write() to check
>> +                * if the requested setting is for a mapped signal or not.
>> +                */
>> +               if (mask) {
>> +                       if (signals[i].init_data->mask == mask)
>> +                               return &signals[i];
>> +               } else {
>> +                       return &signals[i];
>> +               }
> 
> if (!mask || signals[i].init_data->mask == mask)
>         return &signals[i];

Looks better, indeed!

> 
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> +static int rz_sysc_reg_update_bits(void *context, unsigned int off,
>> +                                  unsigned int mask, unsigned int val)
>> +{
>> +       struct rz_sysc *sysc = context;
>> +       struct rz_sysc_signal *signal;
>> +       bool update = false;
>> +
>> +       signal = rz_sysc_off_to_signal(sysc, off, mask);
>> +       if (signal) {
>> +               if (signal->init_data->refcnt_incr_val == val) {
>> +                       if (!refcount_read(&signal->refcnt)) {
>> +                               refcount_set(&signal->refcnt, 1);
>> +                               update = true;
>> +                       } else {
>> +                               refcount_inc(&signal->refcnt);
>> +                       }
>> +               } else {
>> +                       update = refcount_dec_and_test(&signal->refcnt);
>> +               }
>> +       } else {
>> +               update = true;
>> +       }
> 
> You could reduce indentation/number of lines by reordering the logic:
> 
>     if (!signal) {
>             update = true;
>     } else if (signal->init_data->refcnt_incr_val != val) {
>             update = refcount_dec_and_test(&signal->refcnt);
>     } else if (!refcount_read(&signal->refcnt)) {
>             refcount_set(&signal->refcnt, 1);
>             update = true;
>     } else {
>             refcount_inc(&signal->refcnt);
>     }

Looks better, indeed!

> 
>> +
>> +       if (update) {
>> +               u32 tmp;
>> +
>> +               tmp = readl(sysc->base + off);
>> +               tmp &= ~mask;
>> +               tmp |= val & mask;
>> +               writel(tmp, sysc->base + off);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int rz_sysc_reg_write(void *context, unsigned int off, unsigned int val)
>> +{
>> +       struct rz_sysc *sysc = context;
>> +       struct rz_sysc_signal *signal;
>> +
>> +       /*
>> +        * Force using regmap_update_bits() for signals to have reference counter
>> +        * per individual signal in case there are multiple signals controlled
>> +        * through the same register.
>> +        */
>> +       signal = rz_sysc_off_to_signal(sysc, off, 0);
>> +       if (signal) {
>> +               dev_err(sysc->dev,
>> +                       "regmap_write() not allowed on register controlling a signal. Use regmap_update_bits()!");
>> +               return -EOPNOTSUPP;
>> +       }
>> +
> 
> Can you ever get here, given rz_sysc_writeable_reg() below would have
> returned false? 

Yes, in case the user wants to access a register having a signal associated.

If user does this:

regmap_write(regmap, signal_offset, value);

The rz_sysc_writeable_reg() returns true (as it checks only the signal
offset but not the mask) and the rz_sysc_reg_write() reach this point. If
we allow it to continue we can break reference counting for the existing
implemented signals, or, if there are multiple signals handled thorough the
same register, we can break other signals.

> If not, is there any point in having this function?

I chose to have it for (currently unexisting) other use cases of the syscon
+ regmap exported by this driver. For the moment, it is not necessary,
indeed. I can drop it if you prefer.

> 
>> +       writel(val, sysc->base + off);
>> +
>> +       return 0;
>> +}
>> +
>> +static bool rz_sysc_writeable_reg(struct device *dev, unsigned int off)
>> +{
>> +       struct rz_sysc *sysc = dev_get_drvdata(dev);
>> +       struct rz_sysc_signal *signal;
>> +
>> +       /* Any register containing a signal is writeable. */
>> +       signal = rz_sysc_off_to_signal(sysc, off, 0);
>> +       if (signal)
>> +               return true;
>> +
>> +       return false;
>> +}
> 
>> +static int rz_sysc_signals_show(struct seq_file *s, void *what)
>> +{
>> +       struct rz_sysc *sysc = s->private;
>> +
>> +       seq_printf(s, "%-20s Enable count\n", "Signal");
>> +       seq_printf(s, "%-20s ------------\n", "--------------------");
>> +
>> +       for (u8 i = 0; i < sysc->num_signals; i++) {
>> +               seq_printf(s, "%-20s %d\n", sysc->signals[i].init_data->name,
>> +                          refcount_read(&sysc->signals[i].refcnt));
>> +       }
>> +
>> +       return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(rz_sysc_signals);
> 
> What is the use-case for this? Just (initial) debugging?

Only debugging.

> 
>> +
>> +static void rz_sysc_debugfs_remove(void *data)
>> +{
>> +       debugfs_remove_recursive(data);
>> +}
>> +
>> +static int rz_sysc_signals_init(struct rz_sysc *sysc,
>> +                               const struct rz_sysc_signal_init_data *init_data,
>> +                               u32 num_signals)
>> +{
>> +       struct dentry *root;
>> +       int ret;
>> +
>> +       sysc->signals = devm_kcalloc(sysc->dev, num_signals, sizeof(*sysc->signals),
>> +                                    GFP_KERNEL);
>> +       if (!sysc->signals)
>> +               return -ENOMEM;
>> +
>> +       for (u32 i = 0; i < num_signals; i++) {
> 
> unsigned int
> 
>> +               struct rz_sysc_signal_init_data *id;
>> +
>> +               id = devm_kzalloc(sysc->dev, sizeof(*id), GFP_KERNEL);
>> +               if (!id)
>> +                       return -ENOMEM;
>> +
>> +               id->name = devm_kstrdup(sysc->dev, init_data->name, GFP_KERNEL);
>> +               if (!id->name)
>> +                       return -ENOMEM;
>> +
>> +               id->offset = init_data->offset;
>> +               id->mask = init_data->mask;
>> +               id->refcnt_incr_val = init_data->refcnt_incr_val;
>> +
>> +               sysc->signals[i].init_data = id;
>> +               refcount_set(&sysc->signals[i].refcnt, 0);
>> +       }
>> +
>> +       sysc->num_signals = num_signals;
>> +
>> +       root = debugfs_create_dir("renesas-rz-sysc", NULL);
>> +       ret = devm_add_action_or_reset(sysc->dev, rz_sysc_debugfs_remove, root);
>> +       if (ret)
>> +               return ret;
>> +       debugfs_create_file("signals", 0444, root, sysc, &rz_sysc_signals_fops);
>> +
>> +       return 0;
>> +}
> 
>> --- /dev/null
>> +++ b/drivers/soc/renesas/rz-sysc.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Renesas RZ System Controller
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#ifndef __SOC_RENESAS_RZ_SYSC_H__
>> +#define __SOC_RENESAS_RZ_SYSC_H__
>> +
>> +#include <linux/refcount.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * struct rz_sysc_signal_init_data - RZ SYSC signals init data
>> + * @name: signal name
>> + * @offset: register offset controling this signal
>> + * @mask: bitmask in register specific to this signal
>> + * @refcnt_incr_val: increment refcnt when setting this value
>> + */
>> +struct rz_sysc_signal_init_data {
>> +       const char *name;
>> +       u32 offset;
>> +       u32 mask;
>> +       u32 refcnt_incr_val;
>> +};
>> +
>> +/**
>> + * struct rz_sysc_signal - RZ SYSC signals
>> + * @init_data: signals initialization data
>> + * @refcnt: reference counter
>> + */
>> +struct rz_sysc_signal {
>> +       const struct rz_sysc_signal_init_data *init_data;
> 
> Can't you just embed struct rz_sysc_signal_init_data?

Meaning to have directly the members of struct rz_sysc_signal_init_data
here or to drop the const qualifier along with __initconst on
rzg3s_sysc_signals_init_data[]  and re-use the platfom data w/o allocate
new memory?

Thank you for your review,
Claudiu


> That way you could allocate the rz_sysc_signal and
> rz_sysc_signal_init_data structures in a single allocation.
> 
>> +       refcount_t refcnt;
>> +};
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

[Geert Uytterhoeven]

Hi Claudiu,

On Fri, Nov 29, 2024 at 9:48 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 28.11.2024 17:24, Geert Uytterhoeven wrote:
> > On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> The RZ/G3S system controller (SYSC) has various registers that control
> >> signals specific to individual IPs. IP drivers must control these signals
> >> at different configuration phases.
> >>
> >> Add SYSC driver that allows individual SYSC consumers to control these
> >> signals. The SYSC driver exports a syscon regmap enabling IP drivers to
> >> use a specific SYSC offset and mask from the device tree, which can then be
> >> accessed through regmap_update_bits().
> >>
> >> Currently, the SYSC driver provides control to the USB PWRRDY signal, which
> >> is routed to the USB PHY. This signal needs to be managed before or after
> >> powering the USB PHY off or on.
> >>
> >> Other SYSC signals candidates (as exposed in the the hardware manual of the
> >>
> >> * PCIe:
> >> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> >> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
> >>   register
> >> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> >>
> >> * SPI:
> >> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
> >>   register
> >>
> >> * I2C/I3C:
> >> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
> >>   (x=0..3)
> >> - af_bypass I3C signal controlled through SYS_I3C_CFG register
> >>
> >> * Ethernet:
> >> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
> >>   registers (x=0..1)
> >>
> >> As different Renesas RZ SoC shares most of the SYSC functionalities
> >> available on the RZ/G3S SoC, the driver if formed of a SYSC core
> >> part and a SoC specific part allowing individual SYSC SoC to provide
> >> functionalities to the SYSC core.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> >> --- /dev/null
> >> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
> >> @@ -0,0 +1,31 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * RZ/G3S System controller driver
> >> + *
> >> + * Copyright (C) 2024 Renesas Electronics Corp.
> >> + */
> >> +
> >> +#include <linux/array_size.h>
> >> +#include <linux/bits.h>
> >> +#include <linux/init.h>
> >> +
> >> +#include "rz-sysc.h"
> >> +
> >> +#define SYS_USB_PWRRDY         0xd70
> >> +#define SYS_USB_PWRRDY_PWRRDY_N        BIT(0)
> >> +#define SYS_MAX_REG            0xe20
> >> +
> >> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
> >
> > This is marked __initconst...
> >
> >> +       {
> >> +               .name = "usb-pwrrdy",
> >> +               .offset = SYS_USB_PWRRDY,
> >> +               .mask = SYS_USB_PWRRDY_PWRRDY_N,
> >> +               .refcnt_incr_val = 0
> >> +       }
> >> +};
> >> +
> >> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {
> >
> > ... but this is not __init, causing a section mismatch.
>
> Do you know if there is a way to detect this?

The kernel should tell you during the build...

>
> >
> >> +       .signals_init_data = rzg3s_sysc_signals_init_data,
> >> +       .num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
> >> +       .max_register_offset = SYS_MAX_REG,
> >> +};
> >
> >> --- /dev/null
> >> +++ b/drivers/soc/renesas/rz-sysc.c
> >
> >> +/**
> >> + * struct rz_sysc - RZ SYSC private data structure
> >> + * @base: SYSC base address
> >> + * @dev: SYSC device pointer
> >> + * @signals: SYSC signals
> >> + * @num_signals: number of SYSC signals
> >> + */
> >> +struct rz_sysc {
> >> +       void __iomem *base;
> >> +       struct device *dev;
> >> +       struct rz_sysc_signal *signals;
> >> +       u8 num_signals;
> >
> > You could change signals to a flexible array at the end, tag it with
> > __counted_by(num_signals), and allocate space for both struct rz_sysc
> > and the signals array using struct_size(), reducing the number of
> > allocations.
>
> I'll look into this.

> >> --- /dev/null
> >> +++ b/drivers/soc/renesas/rz-sysc.h
> >> @@ -0,0 +1,52 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * Renesas RZ System Controller
> >> + *
> >> + * Copyright (C) 2024 Renesas Electronics Corp.
> >> + */
> >> +
> >> +#ifndef __SOC_RENESAS_RZ_SYSC_H__
> >> +#define __SOC_RENESAS_RZ_SYSC_H__
> >> +
> >> +#include <linux/refcount.h>
> >> +#include <linux/types.h>
> >> +
> >> +/**
> >> + * struct rz_sysc_signal_init_data - RZ SYSC signals init data
> >> + * @name: signal name
> >> + * @offset: register offset controling this signal
> >> + * @mask: bitmask in register specific to this signal
> >> + * @refcnt_incr_val: increment refcnt when setting this value
> >> + */
> >> +struct rz_sysc_signal_init_data {
> >> +       const char *name;
> >> +       u32 offset;
> >> +       u32 mask;
> >> +       u32 refcnt_incr_val;
> >> +};
> >> +
> >> +/**
> >> + * struct rz_sysc_signal - RZ SYSC signals
> >> + * @init_data: signals initialization data
> >> + * @refcnt: reference counter
> >> + */
> >> +struct rz_sysc_signal {
> >> +       const struct rz_sysc_signal_init_data *init_data;
> >
> > Can't you just embed struct rz_sysc_signal_init_data?
>
> Meaning to have directly the members of struct rz_sysc_signal_init_data
> here or to drop the const qualifier along with __initconst on
> rzg3s_sysc_signals_init_data[]  and re-use the platfom data w/o allocate
> new memory?

I mean

    struct rz_sysc_signal {
          struct rz_sysc_signal_init_data init_data;
          ...
    };

Currently you allocate rz_sysc_signal_init_data separately.
When embedded, it will be part of rz_sysc, cfr. above.

> > That way you could allocate the rz_sysc_signal and
> > rz_sysc_signal_init_data structures in a single allocation.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 10/15] phy: renesas: rcar-gen3-usb2: Add support for PWRRDY

[Claudiu Beznea]

Hi, Geert,

On 28.11.2024 17:07, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> On the Renesas RZ/G3S SoC, the USB PHY has an input signal called PWRRDY.
>> This signal is managed by the system controller and must be de-asserted
>> after powering on the area where USB PHY resides and asserted before
>> powering it off.
>>
>> The connection b/w the system controller and the USB PHY is implemented
>> through the renesas,sysc-signal device tree property. This property
>> specifies the register offset and the bitmask required to control the
>> signal. The system controller exports the syscon regmap, and the read/write
>> access to the memory area of the PWRRDY signal is reference-counted, as the
>> same system controller signal is connected to both RZ/G3S USB PHYs.
>>
>> Add support for the PWRRDY signal control.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>> @@ -707,6 +718,55 @@ static int rcar_gen3_phy_usb2_init_bus(struct rcar_gen3_chan *channel)
>>         return ret;
>>  }
>>
>> +static void rcar_gen3_phy_usb2_set_pwrrdy(struct rcar_gen3_chan *channel, bool power_on)
>> +{
>> +       struct rcar_gen3_pwrrdy *pwrrdy = channel->pwrrdy;
>> +
>> +       /* N/A on this platform. */
>> +       if (!pwrrdy)
>> +               return;
> 
> This cannot happen?

You're right, currently it can't happen.

It might be useful for the suspend to RAM support (that will be posted
after initial support will be integrated) to have this function called
unconditionally on suspend/resume APIs.

I can drop it if it's preferred.

Thank you for  your review,
Claudiu

> 
>> +
>> +       regmap_update_bits(pwrrdy->regmap, pwrrdy->offset, pwrrdy->mask, !power_on);
>> +}
>> +
>> +static void rcar_gen3_phy_usb2_pwrrdy_off(void *data)
>> +{
>> +       rcar_gen3_phy_usb2_set_pwrrdy(data, false);
>> +}
>> +
>> +static int rcar_gen3_phy_usb2_init_pwrrdy(struct rcar_gen3_chan *channel)
>> +{
>> +       struct device *dev = channel->dev;
>> +       struct rcar_gen3_pwrrdy *pwrrdy;
>> +       struct of_phandle_args args;
>> +       int ret;
>> +
>> +       pwrrdy = devm_kzalloc(dev, sizeof(*pwrrdy), GFP_KERNEL);
>> +       if (!pwrrdy)
>> +               return -ENOMEM;
>> +
>> +       ret = of_parse_phandle_with_args(dev->of_node, "renesas,sysc-signal",
>> +                                        "#renesas,sysc-signal-cells", 0, &args);
>> +       if (ret)
>> +               return ret;
>> +
>> +       pwrrdy->regmap = syscon_node_to_regmap(args.np);
>> +       pwrrdy->offset = args.args[0];
>> +       pwrrdy->mask = args.args[1];
>> +
>> +       of_node_put(args.np);
>> +
>> +       if (IS_ERR(pwrrdy->regmap))
>> +               return PTR_ERR(pwrrdy->regmap);
>> +
>> +       channel->pwrrdy = pwrrdy;
>> +
>> +       /* Power it ON. */
>> +       rcar_gen3_phy_usb2_set_pwrrdy(channel, true);
>> +
>> +       return devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_pwrrdy_off, channel);
>> +}
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

[Claudiu Beznea]

On 29.11.2024 10:54, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, Nov 29, 2024 at 9:48 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 28.11.2024 17:24, Geert Uytterhoeven wrote:
>>> On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> The RZ/G3S system controller (SYSC) has various registers that control
>>>> signals specific to individual IPs. IP drivers must control these signals
>>>> at different configuration phases.
>>>>
>>>> Add SYSC driver that allows individual SYSC consumers to control these
>>>> signals. The SYSC driver exports a syscon regmap enabling IP drivers to
>>>> use a specific SYSC offset and mask from the device tree, which can then be
>>>> accessed through regmap_update_bits().
>>>>
>>>> Currently, the SYSC driver provides control to the USB PWRRDY signal, which
>>>> is routed to the USB PHY. This signal needs to be managed before or after
>>>> powering the USB PHY off or on.
>>>>
>>>> Other SYSC signals candidates (as exposed in the the hardware manual of the
>>>>
>>>> * PCIe:
>>>> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
>>>> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>>>>   register
>>>> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>>>>
>>>> * SPI:
>>>> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>>>>   register
>>>>
>>>> * I2C/I3C:
>>>> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>>>>   (x=0..3)
>>>> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>>>>
>>>> * Ethernet:
>>>> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>>>>   registers (x=0..1)
>>>>
>>>> As different Renesas RZ SoC shares most of the SYSC functionalities
>>>> available on the RZ/G3S SoC, the driver if formed of a SYSC core
>>>> part and a SoC specific part allowing individual SYSC SoC to provide
>>>> functionalities to the SYSC core.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>>> --- /dev/null
>>>> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
>>>> @@ -0,0 +1,31 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * RZ/G3S System controller driver
>>>> + *
>>>> + * Copyright (C) 2024 Renesas Electronics Corp.
>>>> + */
>>>> +
>>>> +#include <linux/array_size.h>
>>>> +#include <linux/bits.h>
>>>> +#include <linux/init.h>
>>>> +
>>>> +#include "rz-sysc.h"
>>>> +
>>>> +#define SYS_USB_PWRRDY         0xd70
>>>> +#define SYS_USB_PWRRDY_PWRRDY_N        BIT(0)
>>>> +#define SYS_MAX_REG            0xe20
>>>> +
>>>> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
>>>
>>> This is marked __initconst...
>>>
>>>> +       {
>>>> +               .name = "usb-pwrrdy",
>>>> +               .offset = SYS_USB_PWRRDY,
>>>> +               .mask = SYS_USB_PWRRDY_PWRRDY_N,
>>>> +               .refcnt_incr_val = 0
>>>> +       }
>>>> +};
>>>> +
>>>> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {
>>>
>>> ... but this is not __init, causing a section mismatch.
>>
>> Do you know if there is a way to detect this?
> 
> The kernel should tell you during the build...

I'll look carefully, I haven't noticed it. Thank you!

> 
>>
>>>
>>>> +       .signals_init_data = rzg3s_sysc_signals_init_data,
>>>> +       .num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
>>>> +       .max_register_offset = SYS_MAX_REG,
>>>> +};
>>>
>>>> --- /dev/null
>>>> +++ b/drivers/soc/renesas/rz-sysc.c
>>>
>>>> +/**
>>>> + * struct rz_sysc - RZ SYSC private data structure
>>>> + * @base: SYSC base address
>>>> + * @dev: SYSC device pointer
>>>> + * @signals: SYSC signals
>>>> + * @num_signals: number of SYSC signals
>>>> + */
>>>> +struct rz_sysc {
>>>> +       void __iomem *base;
>>>> +       struct device *dev;
>>>> +       struct rz_sysc_signal *signals;
>>>> +       u8 num_signals;
>>>
>>> You could change signals to a flexible array at the end, tag it with
>>> __counted_by(num_signals), and allocate space for both struct rz_sysc
>>> and the signals array using struct_size(), reducing the number of
>>> allocations.
>>
>> I'll look into this.
> 
>>>> --- /dev/null
>>>> +++ b/drivers/soc/renesas/rz-sysc.h
>>>> @@ -0,0 +1,52 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * Renesas RZ System Controller
>>>> + *
>>>> + * Copyright (C) 2024 Renesas Electronics Corp.
>>>> + */
>>>> +
>>>> +#ifndef __SOC_RENESAS_RZ_SYSC_H__
>>>> +#define __SOC_RENESAS_RZ_SYSC_H__
>>>> +
>>>> +#include <linux/refcount.h>
>>>> +#include <linux/types.h>
>>>> +
>>>> +/**
>>>> + * struct rz_sysc_signal_init_data - RZ SYSC signals init data
>>>> + * @name: signal name
>>>> + * @offset: register offset controling this signal
>>>> + * @mask: bitmask in register specific to this signal
>>>> + * @refcnt_incr_val: increment refcnt when setting this value
>>>> + */
>>>> +struct rz_sysc_signal_init_data {
>>>> +       const char *name;
>>>> +       u32 offset;
>>>> +       u32 mask;
>>>> +       u32 refcnt_incr_val;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct rz_sysc_signal - RZ SYSC signals
>>>> + * @init_data: signals initialization data
>>>> + * @refcnt: reference counter
>>>> + */
>>>> +struct rz_sysc_signal {
>>>> +       const struct rz_sysc_signal_init_data *init_data;
>>>
>>> Can't you just embed struct rz_sysc_signal_init_data?
>>
>> Meaning to have directly the members of struct rz_sysc_signal_init_data
>> here or to drop the const qualifier along with __initconst on
>> rzg3s_sysc_signals_init_data[]  and re-use the platfom data w/o allocate
>> new memory?
> 
> I mean
> 
>     struct rz_sysc_signal {
>           struct rz_sysc_signal_init_data init_data;
>           ...
>     };
> 
> Currently you allocate rz_sysc_signal_init_data separately.
> When embedded, it will be part of rz_sysc, cfr. above.

Ah, your right. I initially had this as a pointer and re-used the init data
(rzg3s_sysc_signals_init_data[], w/o having __initconst qualifier for it).
I dropped that approach but missed to drop the pointer here.

Thank you,
Claudiu

> 
>>> That way you could allocate the rz_sysc_signal and
>>> rz_sysc_signal_init_data structures in a single allocation.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

[Geert Uytterhoeven]

On Fri, Nov 29, 2024 at 9:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Nov 29, 2024 at 9:48 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > On 28.11.2024 17:24, Geert Uytterhoeven wrote:
> > > On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >>
> > >> The RZ/G3S system controller (SYSC) has various registers that control
> > >> signals specific to individual IPs. IP drivers must control these signals
> > >> at different configuration phases.
> > >>
> > >> Add SYSC driver that allows individual SYSC consumers to control these
> > >> signals. The SYSC driver exports a syscon regmap enabling IP drivers to
> > >> use a specific SYSC offset and mask from the device tree, which can then be
> > >> accessed through regmap_update_bits().
> > >>
> > >> Currently, the SYSC driver provides control to the USB PWRRDY signal, which
> > >> is routed to the USB PHY. This signal needs to be managed before or after
> > >> powering the USB PHY off or on.
> > >>
> > >> Other SYSC signals candidates (as exposed in the the hardware manual of the
> > >>
> > >> * PCIe:
> > >> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> > >> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
> > >>   register
> > >> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> > >>
> > >> * SPI:
> > >> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
> > >>   register
> > >>
> > >> * I2C/I3C:
> > >> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
> > >>   (x=0..3)
> > >> - af_bypass I3C signal controlled through SYS_I3C_CFG register
> > >>
> > >> * Ethernet:
> > >> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
> > >>   registers (x=0..1)
> > >>
> > >> As different Renesas RZ SoC shares most of the SYSC functionalities
> > >> available on the RZ/G3S SoC, the driver if formed of a SYSC core
> > >> part and a SoC specific part allowing individual SYSC SoC to provide
> > >> functionalities to the SYSC core.
> > >>
> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >
> > >> --- /dev/null
> > >> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
> > >> @@ -0,0 +1,31 @@
> > >> +// SPDX-License-Identifier: GPL-2.0
> > >> +/*
> > >> + * RZ/G3S System controller driver
> > >> + *
> > >> + * Copyright (C) 2024 Renesas Electronics Corp.
> > >> + */
> > >> +
> > >> +#include <linux/array_size.h>
> > >> +#include <linux/bits.h>
> > >> +#include <linux/init.h>
> > >> +
> > >> +#include "rz-sysc.h"
> > >> +
> > >> +#define SYS_USB_PWRRDY         0xd70
> > >> +#define SYS_USB_PWRRDY_PWRRDY_N        BIT(0)
> > >> +#define SYS_MAX_REG            0xe20
> > >> +
> > >> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
> > >
> > > This is marked __initconst...
> > >
> > >> +       {
> > >> +               .name = "usb-pwrrdy",
> > >> +               .offset = SYS_USB_PWRRDY,
> > >> +               .mask = SYS_USB_PWRRDY_PWRRDY_N,
> > >> +               .refcnt_incr_val = 0
> > >> +       }
> > >> +};
> > >> +
> > >> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {
> > >
> > > ... but this is not __init, causing a section mismatch.
> >
> > Do you know if there is a way to detect this?
>
> The kernel should tell you during the build...

Sorry, I hit send too early; I was still verifying this...
And it indeed doesn't trigger, strange...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells

[Claudiu Beznea]

Hi, Geert,

On 29.11.2024 10:38, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, Nov 29, 2024 at 9:21 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 28.11.2024 17:46, Geert Uytterhoeven wrote:
>>> On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> The RZ/G3S system controller (SYSC) has registers to control signals that
>>>> are routed to various IPs. These signals must be controlled during
>>>> configuration of the respective IPs. One such signal is the USB PWRRDY,
>>>> which connects the SYSC and the USB PHY. This signal must to be controlled
>>>> before and after the power to the USB PHY is turned off/on.
>>>>
>>>> Other similar signals include the following (according to the RZ/G3S
>>>> hardware manual):
>>>>
>>>> * PCIe:
>>>> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
>>>> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>>>>   register
>>>> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>>>>
>>>> * SPI:
>>>> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>>>>   register
>>>>
>>>> * I2C/I3C:
>>>> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>>>>   (x=0..3)
>>>> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>>>>
>>>> * Ethernet:
>>>> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>>>>   registers (x=0..1)
>>>>
>>>> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals
>>>> consumers to manage these signals.
>>>>
>>>> The goal is to enable consumers to specify the required access data for
>>>> these signals (through device tree) and let their respective drivers
>>>> control these signals via the syscon regmap provided by the system
>>>> controller driver. For example, the USB PHY will describe this relation
>>>> using the following DT property:
>>>>
>>>> usb2_phy1: usb-phy@11e30200 {
>>>>         // ...
>>>>         renesas,sysc-signal = <&sysc 0xd70 0x1>;
>>>>         // ...
>>>> };
>>>
>>> IIUIC, the consumer driver will  appear to control the SYSC bits
>>> directly, but due to the use of custom validating regmap accessors
>>> and reference counting in the SYSC driver, this is safe?
>>
>> I'm not sure I fully understand the safety concern.
> 
> Sorry for my bad expression, this was more like a rhetorical question.
> I meant that it is safe because:
>   1. Consumers cannot perform arbitrary register accesses,
>   2. The reference counting guarantees correct operation, despite
>       both usb-phy nodes using the same renesas,sysc-signal.
> 
> So everything is fine.
> 
>>> The extra safety requires duplicating the register bits in both DT
>>> and the SYSC driver.
>>
>> One other option I saw was to have common defines for registers that could
>> have been shared b/w driver and DTSes. But it looked better to me the way
>> it has been presented in this series.
>>
>>> Both usb-phy nodes on RZG3S use the same renesas,sysc-signal, so the
>>> reference counting is indeed needed.  They are in different power
>>> domains, could that be an issue w.r.t. ordering?
>>
>> In chapter "32.4.2.1 USB/PHY related pins", section "When either Port1 or
>> Port2 is unused" of the RZ/G3S HW manual it is mentioned "Since USB_VDD18 /
>> USB_VDD33 are common to 2 Port PHY, it is necessary to supply power even
>> when one of the
>>  ports is not in use".
> 
> Does that mean you have to power the other PHY on through the
> CPG_BUS_PERI_COM_MSTOP register, too?

I don't know at the moment if there is hard requirement b/w USB PWRRDY and
the USB PHY CPG MSTOP bit. I'll have to ask further internally.

Thank you,
Claudiu

> (I know you haven't added R9A08G045_PD_USBx to the USB nodes yet,
>  as #power-domain-cells is still 0).
> 
>> (From the discussions w/ the internal HW team) The PWRRDY is an (software
>> controlled) indicator to the USB PHY that power supply is ready.
>>
>> From that and [1] I get that both PHYs are powered by the same regulators
>> (USB_VDD18/USB_VDD33) and the USB PWRRDY signal need to be set before/after
>> the USB PHY power off/on. Because of this I consider the order doesn't matter.
>>
>> [1] https://gcdnb.pbrd.co/images/0a1zYBFZXZVb.png
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v2 07/15] dt-bindings: phy: renesas,usb2-phy: Mark resets as required for RZ/G3S

[Geert Uytterhoeven]

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The reset lines are mandatory for the Renesas RZ/G3S platform and must be
> explicitly defined in device tree.
>
> Fixes: f3c849855114 ("dt-bindings: phy: renesas,usb2-phy: Document RZ/G3S phy bindings")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 03/15] soc: renesas: rz-sysc: Enable SYSC driver for RZ/G3S

[Geert Uytterhoeven]

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Enable SYSC driver for RZ/G3S. This is necessary for USB support.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 04/15] soc: renesas: rz-sysc: Add SoC detection support

[Geert Uytterhoeven]

Hi Claudiu,

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The RZ SYSC controller has registers that keep the SoC ID data. Add
> driver support to retrieve the SoC ID and register a SoC driver.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v2:
> - this was patch 05/16 in v1
> - changed patch title and description
> - added SoC initialization code in its own function
> - addressed the review comments
> - introduced struct rz_sysc_soc_id_init_data and adjusted the code
>   accordingly
> - dropped the RZ/G3S SoC detection code (it will be introduced in
>   a separate patch)

Thanks for your patch!

> --- a/drivers/soc/renesas/rz-sysc.c
> +++ b/drivers/soc/renesas/rz-sysc.c
> @@ -211,6 +214,59 @@ static int rz_sysc_signals_init(struct rz_sysc *sysc,
>         return 0;
>  }
>
> +static int rz_sysc_soc_init(struct rz_sysc *sysc, const struct of_device_id *match)
> +{
> +       const struct rz_sysc_init_data *sysc_data = match->data;
> +       const struct rz_sysc_soc_id_init_data *soc_data = sysc_data->soc_id_init_data;
> +       struct soc_device_attribute *soc_dev_attr;
> +       const char *soc_id_start, *soc_id_end;
> +       u32 val, revision, specific_id;
> +       struct soc_device *soc_dev;
> +       char soc_id[32] = {0};
> +       u8 size;

unsigned int (or size_t?)

> +
> +       if (!soc_data || !soc_data->family || !soc_data->offset ||
> +           !soc_data->revision_mask)
> +               return -EINVAL;

Cannot happen?

> +
> +       soc_id_start = strchr(match->compatible, ',') + 1;
> +       soc_id_end = strchr(match->compatible, '-');
> +       size = soc_id_end - soc_id_start;
> +       if (size > 32)
> +               size = 32;

sizeof(soc_id) instead of hardcoded 32

> +       strscpy(soc_id, soc_id_start, size);

Doesn't this wipe the last character, as strscpy() always
NUL-terminates the destination buffer?

> +
> +       soc_dev_attr = devm_kzalloc(sysc->dev, sizeof(*soc_dev_attr), GFP_KERNEL);
> +       if (!soc_dev_attr)
> +               return -ENOMEM;
> +
> +       soc_dev_attr->family = soc_data->family;

Shouldn't you duplicate family? It's in __initconst, hence freed later.

> +       soc_dev_attr->soc_id = devm_kstrdup(sysc->dev, soc_id, GFP_KERNEL);
> +       if (!soc_dev_attr->soc_id)
> +               return -ENOMEM;
> +
> +       val = readl(sysc->base + soc_data->offset);
> +       revision = field_get(soc_data->revision_mask, val);
> +       specific_id = field_get(soc_data->specific_id_mask, val);
> +       soc_dev_attr->revision = devm_kasprintf(sysc->dev, GFP_KERNEL, "%u", revision);
> +       if (!soc_dev_attr->revision)
> +               return -ENOMEM;
> +
> +       if (soc_data->id && specific_id != soc_data->id) {
> +               dev_warn(sysc->dev, "SoC mismatch (product = 0x%x)\n", specific_id);
> +               return -ENODEV;
> +       }
> +
> +       dev_info(sysc->dev, "Detected Renesas %s %s Rev %s\n", soc_dev_attr->family,
> +                soc_dev_attr->soc_id, soc_dev_attr->revision);
> +
> +       soc_dev = soc_device_register(soc_dev_attr);
> +       if (IS_ERR(soc_dev))
> +               return PTR_ERR(soc_dev);
> +
> +       return 0;
> +}
> +
>  static struct regmap_config rz_sysc_regmap = {
>         .name = "rz_sysc_regs",
>         .reg_bits = 32,
> @@ -235,14 +291,15 @@ MODULE_DEVICE_TABLE(of, rz_sysc_match);
>  static int rz_sysc_probe(struct platform_device *pdev)
>  {
>         const struct rz_sysc_init_data *data;
> +       const struct of_device_id *match;
>         struct device *dev = &pdev->dev;
> -       struct rz_sysc *sysc;
>         struct regmap *regmap;
> +       struct rz_sysc *sysc;
>         int ret;
>
> -       data = device_get_match_data(dev);
> -       if (!data || !data->max_register_offset)
> -               return -EINVAL;
> +       match = of_match_node(rz_sysc_match, dev->of_node);
> +       if (!match || !match->data)

!match->data cannot happen

> +               return -ENODEV;
>
>         sysc = devm_kzalloc(dev, sizeof(*sysc), GFP_KERNEL);
>         if (!sysc)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 05/15] soc: renesas: rz-sysc: Move RZ/G3S SoC detection to the SYSC driver

[Geert Uytterhoeven]

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Now that we have SoC detection in the RZ SYSC driver, move the RZ/G3S
> SoC detection to it. The SYSC provides SoC ID in its own registers.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v2:
> - this was handled though patch 05/16 in v1
> - provide SoC specific init data through the SoC specific driver

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 08/15] dt-bindings: phy: renesas,usb2-phy: Add renesas,sysc-signal

[Geert Uytterhoeven]

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> On the Renesas RZ/G3S SoC, the USB PHY receives a signal from the system
> controller that need to be de-asserted/asserted when power is turned
> on/off. This signal, called PWRRDY, is controlled through a specific
> register in the system controller memory space.
>
> Add the renesas,sysc-signal DT property to describe the relation b/w the
> system controller and the USB PHY on the Renesas RZ/G3S. This property
> provides a phandle to the system controller, along with the offset within
> the system controller memory space that manages the signal and a bitmask
> that indicates the specific bits required to control the signal.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 09/15] phy: renesas: rcar-gen3-usb2: Fix an error handling path in rcar_gen3_phy_usb2_probe()

[Geert Uytterhoeven]

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>
> If an error occurs after the reset_control_deassert(),
> reset_control_assert() must be called, as already done in the remove
> function.
>
> Use devm_add_action_or_reset() to add the missing call and simplify the
> .remove() function accordingly.
>
> Fixes: 4eae16375357 ("phy: renesas: rcar-gen3-usb2: Add support to initialize the bus")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> [claudiu.beznea: removed "struct reset_control *rstc = data;" from
>  rcar_gen3_reset_assert()]
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 11/15] dt-bindings: reset: renesas,rzg2l-usbphy-ctrl: Document RZ/G3S support

[Geert Uytterhoeven]

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The Renesas RZ/G3S USB PHY control block is similar with the one found on
> the Renesas RZ/G2L. Add documentation for it.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 12/15] arm64: dts: renesas: Add #renesas,sysc-signal-cells to system controller node

[Geert Uytterhoeven]

On Thu, Nov 28, 2024 at 4:17 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> > The system controller on RZ/G3S can provide control access to its signals.
>
> Actually all SoCs from the "RZ/G2L" family...
>
> > To enable this, add the #renesas,sysc-signal-cells DT property. Consumers
> > can use the renesas,sysc-signal DT property to reference the specific SYSC
> > signal that needs to be controlled.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

With the patch description updated:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 13/15] arm64: dts: renesas: r9a08g045: Enable the system controller

[Geert Uytterhoeven]

Hi Claudiu,

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Enable the system controller. It is needed for USB and SoC
> identification.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
> @@ -207,7 +207,6 @@ sysc: system-controller@11020000 {
>                         interrupt-names = "lpm_int", "ca55stbydone_int",
>                                           "cm33stbyr_int", "ca55_deny";
>                         #renesas,sysc-signal-cells = <2>;
> -                       status = "disabled";
>                 };

Please enable it on all members of the RZ/G2L family, and combine
this with "[PATCH v2 12/15]".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 14/15] arm64: dts: renesas: r9a08g045: Add USB support

[Geert Uytterhoeven]

Hi Claudiu,

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Add USB nodes for the Renesas RZ/G3S SoC. This consists of PHY reset,
> host and device support.
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v2:
> - this was patch 14/16 in v1
> - added renesas,sysc-signal properties to USB PHYs
> - collected tags
> - Geert: I kept your tag; please let me know if you consider otherwise

Thanks for your patch!

> --- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
> @@ -417,6 +417,125 @@ eth1: ethernet@11c40000 {
>                         status = "disabled";
>                 };
>
> +               phyrst: usbphy-ctrl@11e00000 {
[...]
> +               ohci0: usb@11e10000 {
[...]
> +               ehci0: usb@11e10100 {
[...]
> +               usb2_phy0: usb-phy@11e10200 {
[...]
> +               hsusb: usb@11e20000 {
[...]
> +               ohci1: usb@11e30000 {
[...]
> +               ehci1: usb@11e30100 {
[...]
> +               usb2_phy1: usb-phy@11e30200 {

Please keep similar nodes together, i.e.

    phyrst: usbphy-ctrl@11e00000 {
    ohci0: usb@11e10000 {
    ohci1: usb@11e30000 {
    ehci0: usb@11e10100 {
    ehci1: usb@11e30100 {
    usb2_phy0: usb-phy@11e10200 {
    usb2_phy1: usb-phy@11e30200 {
    hsusb: usb@11e20000 {

After that, you can easily compare with similar SoCs:

    soc-dts-diff arch/arm64/boot/dts/renesas/r9a0{7g044,8g045}.dtsi

https://github.com/geertu/linux-scripts/blob/master/soc-dts-diff

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 15/15] arm64: dts: renesas: rzg3s-smarc: Enable USB support

[Geert Uytterhoeven]

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Enable USB support (host, device, USB PHYs).
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v2:
> - this was patch 15/16 in v1:
> - dropped sysc enablement as it is now done in SoC dtsi file

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells

[Rob Herring]

On Tue, Nov 26, 2024 at 11:20:36AM +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The RZ/G3S system controller (SYSC) has registers to control signals that
> are routed to various IPs. These signals must be controlled during
> configuration of the respective IPs. One such signal is the USB PWRRDY,
> which connects the SYSC and the USB PHY. This signal must to be controlled
> before and after the power to the USB PHY is turned off/on.
> 
> Other similar signals include the following (according to the RZ/G3S
> hardware manual):
> 
> * PCIe:
> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>   register
> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> 
> * SPI:
> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>   register
> 
> * I2C/I3C:
> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>   (x=0..3)
> - af_bypass I3C signal controlled through SYS_I3C_CFG register
> 
> * Ethernet:
> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>   registers (x=0..1)
> 
> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals
> consumers to manage these signals.
> 
> The goal is to enable consumers to specify the required access data for
> these signals (through device tree) and let their respective drivers
> control these signals via the syscon regmap provided by the system
> controller driver. For example, the USB PHY will describe this relation
> using the following DT property:
> 
> usb2_phy1: usb-phy@11e30200 {
> 	// ...
> 	renesas,sysc-signal = <&sysc 0xd70 0x1>;
> 	// ...
> };
> 
> Along with it, add the syscon to the compatible list as it will be
> requested by the consumer drivers. The syscon was added to the rest of
> system controller variants as these are similar with RZ/G3S and can
> benefit from the implementation proposed in this series.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this patch is new
> 
> 
>  .../soc/renesas/renesas,rzg2l-sysc.yaml       | 23 ++++++++++++++-----
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
> index 4386b2c3fa4d..90f827e8de3e 100644
> --- a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
> +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
> @@ -19,11 +19,13 @@ description:
>  
>  properties:
>    compatible:
> -    enum:
> -      - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five
> -      - renesas,r9a07g044-sysc # RZ/G2{L,LC}
> -      - renesas,r9a07g054-sysc # RZ/V2L
> -      - renesas,r9a08g045-sysc # RZ/G3S
> +    items:
> +      - enum:
> +          - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five
> +          - renesas,r9a07g044-sysc # RZ/G2{L,LC}
> +          - renesas,r9a07g054-sysc # RZ/V2L
> +          - renesas,r9a08g045-sysc # RZ/G3S
> +      - const: syscon
>  
>    reg:
>      maxItems: 1
> @@ -42,9 +44,17 @@ properties:
>        - const: cm33stbyr_int
>        - const: ca55_deny
>  
> +  "#renesas,sysc-signal-cells":
> +    description:
> +      The number of cells needed to configure a SYSC controlled signal. First
> +      cell specifies the SYSC offset of the configuration register, second cell
> +      specifies the bitmask in register.
> +    const: 2

If there's only one possible value, then just fix the size in the users. 
We don't need #foo-cells until things are really generic. Plus patch 
8 already ignores this based on the schema. And there's implications to 
defining them. For example, the pattern is that the consumer property 
name is renesas,sysc-signals, not renesas,sysc-signal.

Maybe someone wants to create a 'h/w (signal) control' subsystem (and 
binding) that is just 'read, assert, or deassert a h/w signal'. Perhaps 
even the reset subsystem could be morphed into that as I think there 
would be a lot of overlap. Maybe that would cut down on a lot of these 
syscon phandle properties. I would find that a lot more acceptable than 
the generic 'syscons' and '#syscon-cells' binding that was proposed at 
some point.


> +
>  required:
>    - compatible
>    - reg
> +  - "#renesas,sysc-signal-cells"

New required properties are an ABI break.

>  
>  additionalProperties: false
>  
> @@ -53,7 +63,7 @@ examples:
>      #include <dt-bindings/interrupt-controller/arm-gic.h>
>  
>      sysc: system-controller@11020000 {
> -            compatible = "renesas,r9a07g044-sysc";
> +            compatible = "renesas,r9a07g044-sysc", "syscon";

What happens on a new kernel and a DT without this change?

Rob

Re: [PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells

[Claudiu Beznea]

Hi, Rob,

On 10.12.2024 20:45, Rob Herring wrote:
> On Tue, Nov 26, 2024 at 11:20:36AM +0200, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The RZ/G3S system controller (SYSC) has registers to control signals that
>> are routed to various IPs. These signals must be controlled during
>> configuration of the respective IPs. One such signal is the USB PWRRDY,
>> which connects the SYSC and the USB PHY. This signal must to be controlled
>> before and after the power to the USB PHY is turned off/on.
>>
>> Other similar signals include the following (according to the RZ/G3S
>> hardware manual):
>>
>> * PCIe:
>> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
>> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>>   register
>> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>>
>> * SPI:
>> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>>   register
>>
>> * I2C/I3C:
>> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>>   (x=0..3)
>> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>>
>> * Ethernet:
>> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>>   registers (x=0..1)
>>
>> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals
>> consumers to manage these signals.
>>
>> The goal is to enable consumers to specify the required access data for
>> these signals (through device tree) and let their respective drivers
>> control these signals via the syscon regmap provided by the system
>> controller driver. For example, the USB PHY will describe this relation
>> using the following DT property:
>>
>> usb2_phy1: usb-phy@11e30200 {
>> 	// ...
>> 	renesas,sysc-signal = <&sysc 0xd70 0x1>;
>> 	// ...
>> };
>>
>> Along with it, add the syscon to the compatible list as it will be
>> requested by the consumer drivers. The syscon was added to the rest of
>> system controller variants as these are similar with RZ/G3S and can
>> benefit from the implementation proposed in this series.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this patch is new
>>
>>
>>  .../soc/renesas/renesas,rzg2l-sysc.yaml       | 23 ++++++++++++++-----
>>  1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
>> index 4386b2c3fa4d..90f827e8de3e 100644
>> --- a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
>> +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
>> @@ -19,11 +19,13 @@ description:
>>  
>>  properties:
>>    compatible:
>> -    enum:
>> -      - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five
>> -      - renesas,r9a07g044-sysc # RZ/G2{L,LC}
>> -      - renesas,r9a07g054-sysc # RZ/V2L
>> -      - renesas,r9a08g045-sysc # RZ/G3S
>> +    items:
>> +      - enum:
>> +          - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five
>> +          - renesas,r9a07g044-sysc # RZ/G2{L,LC}
>> +          - renesas,r9a07g054-sysc # RZ/V2L
>> +          - renesas,r9a08g045-sysc # RZ/G3S
>> +      - const: syscon
>>  
>>    reg:
>>      maxItems: 1
>> @@ -42,9 +44,17 @@ properties:
>>        - const: cm33stbyr_int
>>        - const: ca55_deny
>>  
>> +  "#renesas,sysc-signal-cells":
>> +    description:
>> +      The number of cells needed to configure a SYSC controlled signal. First
>> +      cell specifies the SYSC offset of the configuration register, second cell
>> +      specifies the bitmask in register.
>> +    const: 2
> 
> If there's only one possible value, then just fix the size in the users. 
> We don't need #foo-cells until things are really generic. Plus patch 
> 8 already ignores this based on the schema. And there's implications to 
> defining them. For example, the pattern is that the consumer property 
> name is renesas,sysc-signals, not renesas,sysc-signal.

OK, I'll fix the size in users.

> 
> Maybe someone wants to create a 'h/w (signal) control' subsystem (and 
> binding) that is just 'read, assert, or deassert a h/w signal'. Perhaps 

Until then, is it OK for you to keep it as proposed here?

> even the reset subsystem could be morphed into that as I think there 
> would be a lot of overlap. 

The USB PWRRDY signal handling has been initially implemented though a
reset controller driver but, after discussion with Philipp it has been
concluded that it should be handled differently, since it is not a reset
signal.

> Maybe that would cut down on a lot of these 
> syscon phandle properties. I would find that a lot more acceptable than 
> the generic 'syscons' and '#syscon-cells' binding that was proposed at 
> some point.
> 
> 
>> +
>>  required:
>>    - compatible
>>    - reg
>> +  - "#renesas,sysc-signal-cells"
> 
> New required properties are an ABI break.

I've added it as in the old DTs the system-controller node is disabled.
With that, do you consider it OK to keep it?

> 
>>  
>>  additionalProperties: false
>>  
>> @@ -53,7 +63,7 @@ examples:
>>      #include <dt-bindings/interrupt-controller/arm-gic.h>
>>  
>>      sysc: system-controller@11020000 {
>> -            compatible = "renesas,r9a07g044-sysc";
>> +            compatible = "renesas,r9a07g044-sysc", "syscon";
> 
> What happens on a new kernel and a DT without this change?

The older DT have the system-controller node disabled, thus nothing will be
probed for it.

Thank you for your review,
Claudiu

> 
> Rob

Re: [PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells

[Rob Herring]

On Wed, Dec 11, 2024 at 6:23 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Rob,
>
> On 10.12.2024 20:45, Rob Herring wrote:
> > On Tue, Nov 26, 2024 at 11:20:36AM +0200, Claudiu wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> The RZ/G3S system controller (SYSC) has registers to control signals that
> >> are routed to various IPs. These signals must be controlled during
> >> configuration of the respective IPs. One such signal is the USB PWRRDY,
> >> which connects the SYSC and the USB PHY. This signal must to be controlled
> >> before and after the power to the USB PHY is turned off/on.
> >>
> >> Other similar signals include the following (according to the RZ/G3S
> >> hardware manual):
> >>
> >> * PCIe:
> >> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> >> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
> >>   register
> >> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> >>
> >> * SPI:
> >> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
> >>   register
> >>
> >> * I2C/I3C:
> >> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
> >>   (x=0..3)
> >> - af_bypass I3C signal controlled through SYS_I3C_CFG register
> >>
> >> * Ethernet:
> >> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
> >>   registers (x=0..1)
> >>
> >> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals
> >> consumers to manage these signals.
> >>
> >> The goal is to enable consumers to specify the required access data for
> >> these signals (through device tree) and let their respective drivers
> >> control these signals via the syscon regmap provided by the system
> >> controller driver. For example, the USB PHY will describe this relation
> >> using the following DT property:
> >>
> >> usb2_phy1: usb-phy@11e30200 {
> >>      // ...
> >>      renesas,sysc-signal = <&sysc 0xd70 0x1>;
> >>      // ...
> >> };
> >>
> >> Along with it, add the syscon to the compatible list as it will be
> >> requested by the consumer drivers. The syscon was added to the rest of
> >> system controller variants as these are similar with RZ/G3S and can
> >> benefit from the implementation proposed in this series.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>
> >> Changes in v2:
> >> - none; this patch is new
> >>
> >>
> >>  .../soc/renesas/renesas,rzg2l-sysc.yaml       | 23 ++++++++++++++-----
> >>  1 file changed, 17 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
> >> index 4386b2c3fa4d..90f827e8de3e 100644
> >> --- a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
> >> +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
> >> @@ -19,11 +19,13 @@ description:
> >>
> >>  properties:
> >>    compatible:
> >> -    enum:
> >> -      - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five
> >> -      - renesas,r9a07g044-sysc # RZ/G2{L,LC}
> >> -      - renesas,r9a07g054-sysc # RZ/V2L
> >> -      - renesas,r9a08g045-sysc # RZ/G3S
> >> +    items:
> >> +      - enum:
> >> +          - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five
> >> +          - renesas,r9a07g044-sysc # RZ/G2{L,LC}
> >> +          - renesas,r9a07g054-sysc # RZ/V2L
> >> +          - renesas,r9a08g045-sysc # RZ/G3S
> >> +      - const: syscon
> >>
> >>    reg:
> >>      maxItems: 1
> >> @@ -42,9 +44,17 @@ properties:
> >>        - const: cm33stbyr_int
> >>        - const: ca55_deny
> >>
> >> +  "#renesas,sysc-signal-cells":
> >> +    description:
> >> +      The number of cells needed to configure a SYSC controlled signal. First
> >> +      cell specifies the SYSC offset of the configuration register, second cell
> >> +      specifies the bitmask in register.
> >> +    const: 2
> >
> > If there's only one possible value, then just fix the size in the users.
> > We don't need #foo-cells until things are really generic. Plus patch
> > 8 already ignores this based on the schema. And there's implications to
> > defining them. For example, the pattern is that the consumer property
> > name is renesas,sysc-signals, not renesas,sysc-signal.
>
> OK, I'll fix the size in users.

You already did for the one in this series.

> >
> > Maybe someone wants to create a 'h/w (signal) control' subsystem (and
> > binding) that is just 'read, assert, or deassert a h/w signal'. Perhaps
>
> Until then, is it OK for you to keep it as proposed here?

Yes.

> > even the reset subsystem could be morphed into that as I think there
> > would be a lot of overlap.
>
> The USB PWRRDY signal handling has been initially implemented though a
> reset controller driver but, after discussion with Philipp it has been
> concluded that it should be handled differently, since it is not a reset
> signal.

Every reset is a signal, but every signal is not a reset.

> > Maybe that would cut down on a lot of these
> > syscon phandle properties. I would find that a lot more acceptable than
> > the generic 'syscons' and '#syscon-cells' binding that was proposed at
> > some point.
> >
> >
> >> +
> >>  required:
> >>    - compatible
> >>    - reg
> >> +  - "#renesas,sysc-signal-cells"
> >
> > New required properties are an ABI break.
>
> I've added it as in the old DTs the system-controller node is disabled.

Ok, so it depends if the consumers treat this node as required or not.
Or maybe they are all disabled too.

> With that, do you consider it OK to keep it?

No, as we're dropping the property aren't we?

Rob

Re: [PATCH v2 01/15] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Add #renesas,sysc-signal-cells

[Claudiu Beznea]

On 11.12.2024 14:46, Rob Herring wrote:
> On Wed, Dec 11, 2024 at 6:23 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>
>> Hi, Rob,
>>
>> On 10.12.2024 20:45, Rob Herring wrote:
>>> On Tue, Nov 26, 2024 at 11:20:36AM +0200, Claudiu wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> The RZ/G3S system controller (SYSC) has registers to control signals that
>>>> are routed to various IPs. These signals must be controlled during
>>>> configuration of the respective IPs. One such signal is the USB PWRRDY,
>>>> which connects the SYSC and the USB PHY. This signal must to be controlled
>>>> before and after the power to the USB PHY is turned off/on.
>>>>
>>>> Other similar signals include the following (according to the RZ/G3S
>>>> hardware manual):
>>>>
>>>> * PCIe:
>>>> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
>>>> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>>>>   register
>>>> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>>>>
>>>> * SPI:
>>>> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>>>>   register
>>>>
>>>> * I2C/I3C:
>>>> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>>>>   (x=0..3)
>>>> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>>>>
>>>> * Ethernet:
>>>> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>>>>   registers (x=0..1)
>>>>
>>>> Add #renesas,sysc-signal-cells DT property to allow different SYSC signals
>>>> consumers to manage these signals.
>>>>
>>>> The goal is to enable consumers to specify the required access data for
>>>> these signals (through device tree) and let their respective drivers
>>>> control these signals via the syscon regmap provided by the system
>>>> controller driver. For example, the USB PHY will describe this relation
>>>> using the following DT property:
>>>>
>>>> usb2_phy1: usb-phy@11e30200 {
>>>>      // ...
>>>>      renesas,sysc-signal = <&sysc 0xd70 0x1>;
>>>>      // ...
>>>> };
>>>>
>>>> Along with it, add the syscon to the compatible list as it will be
>>>> requested by the consumer drivers. The syscon was added to the rest of
>>>> system controller variants as these are similar with RZ/G3S and can
>>>> benefit from the implementation proposed in this series.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - none; this patch is new
>>>>
>>>>
>>>>  .../soc/renesas/renesas,rzg2l-sysc.yaml       | 23 ++++++++++++++-----
>>>>  1 file changed, 17 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
>>>> index 4386b2c3fa4d..90f827e8de3e 100644
>>>> --- a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
>>>> +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
>>>> @@ -19,11 +19,13 @@ description:
>>>>
>>>>  properties:
>>>>    compatible:
>>>> -    enum:
>>>> -      - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five
>>>> -      - renesas,r9a07g044-sysc # RZ/G2{L,LC}
>>>> -      - renesas,r9a07g054-sysc # RZ/V2L
>>>> -      - renesas,r9a08g045-sysc # RZ/G3S
>>>> +    items:
>>>> +      - enum:
>>>> +          - renesas,r9a07g043-sysc # RZ/G2UL and RZ/Five
>>>> +          - renesas,r9a07g044-sysc # RZ/G2{L,LC}
>>>> +          - renesas,r9a07g054-sysc # RZ/V2L
>>>> +          - renesas,r9a08g045-sysc # RZ/G3S
>>>> +      - const: syscon
>>>>
>>>>    reg:
>>>>      maxItems: 1
>>>> @@ -42,9 +44,17 @@ properties:
>>>>        - const: cm33stbyr_int
>>>>        - const: ca55_deny
>>>>
>>>> +  "#renesas,sysc-signal-cells":
>>>> +    description:
>>>> +      The number of cells needed to configure a SYSC controlled signal. First
>>>> +      cell specifies the SYSC offset of the configuration register, second cell
>>>> +      specifies the bitmask in register.
>>>> +    const: 2
>>>
>>> If there's only one possible value, then just fix the size in the users.
>>> We don't need #foo-cells until things are really generic. Plus patch
>>> 8 already ignores this based on the schema. And there's implications to
>>> defining them. For example, the pattern is that the consumer property
>>> name is renesas,sysc-signals, not renesas,sysc-signal.
>>
>> OK, I'll fix the size in users.
> 
> You already did for the one in this series.
> 
>>>
>>> Maybe someone wants to create a 'h/w (signal) control' subsystem (and
>>> binding) that is just 'read, assert, or deassert a h/w signal'. Perhaps
>>
>> Until then, is it OK for you to keep it as proposed here?
> 
> Yes.
> 
>>> even the reset subsystem could be morphed into that as I think there
>>> would be a lot of overlap.
>>
>> The USB PWRRDY signal handling has been initially implemented though a
>> reset controller driver but, after discussion with Philipp it has been
>> concluded that it should be handled differently, since it is not a reset
>> signal.
> 
> Every reset is a signal, but every signal is not a reset.
> 
>>> Maybe that would cut down on a lot of these
>>> syscon phandle properties. I would find that a lot more acceptable than
>>> the generic 'syscons' and '#syscon-cells' binding that was proposed at
>>> some point.
>>>
>>>
>>>> +
>>>>  required:
>>>>    - compatible
>>>>    - reg
>>>> +  - "#renesas,sysc-signal-cells"
>>>
>>> New required properties are an ABI break.
>>
>> I've added it as in the old DTs the system-controller node is disabled.
> 
> Ok, so it depends if the consumers treat this node as required or not.

The only current consumer is the the RZ/G3S USB PHY which is added along
with this series.

> Or maybe they are all disabled too.
> 
>> With that, do you consider it OK to keep it?
> 
> No, as we're dropping the property aren't we?

You're right! Stupid question from me, sorry.

Thank you for your review,
Claudiu

> 
> Rob