linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Add USB2PHY control support for Renesas RZ/V2H(P) SoC
@ 2025-03-05 12:39 Prabhakar
  2025-03-05 12:39 ` [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control Prabhakar
  2025-03-05 12:39 ` [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P) Prabhakar
  0 siblings, 2 replies; 20+ messages in thread
From: Prabhakar @ 2025-03-05 12:39 UTC (permalink / raw)
  To: Philipp Zabel, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm
  Cc: devicetree, linux-kernel, linux-renesas-soc, Prabhakar, Biju Das,
	Fabrizio Castro, Lad Prabhakar

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

This patch series adds support for the USB2PHY control on the Renesas
RZ/V2H(P) SoC. The changes include documenting the USB2PHY control
bindings and adding the USB2PHY control driver.

These changes are necessary to support the USB2.0 on the RZ/V2H(P) SoC.

v1->v2
- Dropped binding postfix in subject line for patch 1/2
- Moved accquiring the ctrl2 pin in deassert callback
- Updated ctrl_status_bits

Cheers,
Prabhakar

Lad Prabhakar (2):
  dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  reset: Add USB2PHY control driver for Renesas RZ/V2H(P)

 .../reset/renesas,rzv2h-usb2phy-ctrl.yaml     |  56 +++++
 drivers/reset/Kconfig                         |   7 +
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-rzv2h-usb2phy-ctrl.c      | 223 ++++++++++++++++++
 4 files changed, 287 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
 create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c

-- 
2.43.0


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

* [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  2025-03-05 12:39 [PATCH v2 0/2] Add USB2PHY control support for Renesas RZ/V2H(P) SoC Prabhakar
@ 2025-03-05 12:39 ` Prabhakar
  2025-03-05 16:26   ` Conor Dooley
  2025-03-06 11:15   ` Fabrizio Castro
  2025-03-05 12:39 ` [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P) Prabhakar
  1 sibling, 2 replies; 20+ messages in thread
From: Prabhakar @ 2025-03-05 12:39 UTC (permalink / raw)
  To: Philipp Zabel, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm
  Cc: devicetree, linux-kernel, linux-renesas-soc, Prabhakar, Biju Das,
	Fabrizio Castro, Lad Prabhakar

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add device tree binding document for the Renesas RZ/V2H(P) USB2PHY Control
Device. It mainly controls reset and power down of the USB2.0 PHY (for
both host and function).

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../reset/renesas,rzv2h-usb2phy-ctrl.yaml     | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml

diff --git a/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml b/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
new file mode 100644
index 000000000000..ed156a1d3eb3
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/renesas,rzv2h-usb2phy-ctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/V2H(P) USB2PHY Control
+
+maintainers:
+  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
+
+description:
+  The RZ/V2H(P) USB2PHY Control mainly controls reset and power down of the
+  USB2.0 PHY.
+
+properties:
+  compatible:
+    const: renesas,r9a09g057-usb2phy-ctrl  # RZ/V2H(P)
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  '#reset-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+  - power-domains
+  - '#reset-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/renesas,r9a09g057-cpg.h>
+
+    usbphy-ctrl@15830000 {
+        compatible = "renesas,r9a09g057-usb2phy-ctrl";
+        reg = <0x15830000 0x10000>;
+        clocks = <&cpg CPG_MOD 0xb6>;
+        resets = <&cpg 0xaf>;
+        power-domains = <&cpg>;
+        #reset-cells = <0>;
+    };
-- 
2.43.0


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

* [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
  2025-03-05 12:39 [PATCH v2 0/2] Add USB2PHY control support for Renesas RZ/V2H(P) SoC Prabhakar
  2025-03-05 12:39 ` [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control Prabhakar
@ 2025-03-05 12:39 ` Prabhakar
  2025-03-06 11:17   ` Fabrizio Castro
  2025-03-13  8:37   ` Philipp Zabel
  1 sibling, 2 replies; 20+ messages in thread
From: Prabhakar @ 2025-03-05 12:39 UTC (permalink / raw)
  To: Philipp Zabel, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm
  Cc: devicetree, linux-kernel, linux-renesas-soc, Prabhakar, Biju Das,
	Fabrizio Castro, Lad Prabhakar

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add support for the USB2PHY control driver on the Renesas RZ/V2H(P) SoC.
Make the driver handle reset and power-down operations for the USB2PHY.

Pass OF data to support future SoCs with similar USB2PHY hardware but
different register configurations. Define device-specific initialization
values and control register settings in OF data to ensure flexibility
for upcoming SoCs.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/reset/Kconfig                    |   7 +
 drivers/reset/Makefile                   |   1 +
 drivers/reset/reset-rzv2h-usb2phy-ctrl.c | 223 +++++++++++++++++++++++
 3 files changed, 231 insertions(+)
 create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 5b3abb6db248..bac08dae8905 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -218,6 +218,13 @@ config RESET_RZG2L_USBPHY_CTRL
 	  Support for USBPHY Control found on RZ/G2L family. It mainly
 	  controls reset and power down of the USB/PHY.
 
+config RESET_RZV2H_USB2PHY_CTRL
+	tristate "Renesas RZ/V2H(P) (and similar SoCs) USB2PHY control driver"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	help
+	  Support for USB2PHY Control found on the RZ/V2H(P) SoC (and similar SoCs).
+	  It mainly controls reset and power down of the USB2 PHY.
+
 config RESET_SCMI
 	tristate "Reset driver controlled via ARM SCMI interface"
 	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 677c4d1e2632..3cb3df018cf8 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
 obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
 obj-$(CONFIG_RESET_RASPBERRYPI) += reset-raspberrypi.o
 obj-$(CONFIG_RESET_RZG2L_USBPHY_CTRL) += reset-rzg2l-usbphy-ctrl.o
+obj-$(CONFIG_RESET_RZV2H_USB2PHY_CTRL) += reset-rzv2h-usb2phy-ctrl.o
 obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
 obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
 obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
diff --git a/drivers/reset/reset-rzv2h-usb2phy-ctrl.c b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
new file mode 100644
index 000000000000..a6daeaf37e1c
--- /dev/null
+++ b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2H(P) USB2PHY control driver
+ *
+ * Copyright (C) 2025 Renesas Electronics Corporation
+ */
+
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <linux/reset-controller.h>
+
+struct rzv2h_usb2phy_regval {
+	u16 reg;
+	u16 val;
+};
+
+struct rzv2h_usb2phy_data {
+	const struct rzv2h_usb2phy_regval *init_vals;
+	unsigned int init_val_count;
+
+	u16 ctrl_reg;
+	u16 ctrl_assert_val;
+	u16 ctrl_deassert_val;
+	u16 ctrl_status_bits;
+	u16 ctrl_release_val;
+
+	u16 ctrl2_reg;
+	u16 ctrl2_acquire_val;
+	u16 ctrl2_release_val;
+};
+
+struct rzv2h_usb2phy_ctrl_priv {
+	const struct rzv2h_usb2phy_data *data;
+	void __iomem *base;
+	struct device *dev;
+	struct reset_controller_dev rcdev;
+	spinlock_t lock;
+};
+
+#define rcdev_to_priv(x) container_of(x, struct rzv2h_usb2phy_ctrl_priv, rcdev)
+
+static int rzv2h_usbphy_ctrl_assert(struct reset_controller_dev *rcdev,
+				    unsigned long id)
+{
+	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
+	const struct rzv2h_usb2phy_data *data = priv->data;
+	struct device *dev = priv->dev;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		dev_err(dev, "pm_runtime_resume_and_get failed\n");
+		return ret;
+	}
+	scoped_guard(spinlock, &priv->lock) {
+		writel(data->ctrl2_acquire_val, priv->base + data->ctrl2_reg);
+		writel(data->ctrl_assert_val, priv->base + data->ctrl_reg);
+	}
+
+	/* The reset line needs to be asserted for more than 10 microseconds. */
+	udelay(11);
+	pm_runtime_put(dev);
+
+	return 0;
+}
+
+static int rzv2h_usbphy_ctrl_deassert(struct reset_controller_dev *rcdev,
+				      unsigned long id)
+{
+	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
+	const struct rzv2h_usb2phy_data *data = priv->data;
+	struct device *dev = priv->dev;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		dev_err(dev, "pm_runtime_resume_and_get failed\n");
+		return ret;
+	}
+
+	scoped_guard(spinlock, &priv->lock) {
+		writel(data->ctrl_deassert_val, priv->base + data->ctrl_reg);
+		writel(data->ctrl2_release_val, priv->base + data->ctrl2_reg);
+		writel(data->ctrl_release_val, priv->base + data->ctrl_reg);
+	}
+
+	pm_runtime_put(dev);
+
+	return 0;
+}
+
+static int rzv2h_usbphy_ctrl_status(struct reset_controller_dev *rcdev,
+				    unsigned long id)
+{
+	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
+	struct device *dev = priv->dev;
+	int ret;
+	u32 reg;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		dev_err(dev, "pm_runtime_resume_and_get failed\n");
+		return ret;
+	}
+
+	scoped_guard(spinlock, &priv->lock)
+		reg = readl(priv->base + priv->data->ctrl_reg);
+
+	pm_runtime_put(dev);
+
+	return (reg & priv->data->ctrl_status_bits) == priv->data->ctrl_status_bits;
+}
+
+static const struct reset_control_ops rzv2h_usbphy_ctrl_reset_ops = {
+	.assert = rzv2h_usbphy_ctrl_assert,
+	.deassert = rzv2h_usbphy_ctrl_deassert,
+	.status = rzv2h_usbphy_ctrl_status,
+};
+
+static int rzv2h_reset_of_xlate(struct reset_controller_dev *rcdev,
+				const struct of_phandle_args *reset_spec)
+{
+	/* No special handling needed, we have only one reset line per device */
+	return 0;
+}
+
+static int rzv2h_usb2phy_ctrl_probe(struct platform_device *pdev)
+{
+	const struct rzv2h_usb2phy_data *data;
+	struct rzv2h_usb2phy_ctrl_priv *priv;
+	struct device *dev = &pdev->dev;
+	struct reset_control *rstc;
+	int error;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return dev_err_probe(dev, -ENODEV,
+				     "failed to match device\n");
+
+	priv->data = data;
+	priv->dev = dev;
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	rstc = devm_reset_control_get_shared_deasserted(dev, NULL);
+	if (IS_ERR(rstc))
+		return dev_err_probe(dev, PTR_ERR(rstc),
+				     "failed to get deasserted reset\n");
+
+	spin_lock_init(&priv->lock);
+	dev_set_drvdata(dev, priv);
+
+	error = devm_pm_runtime_enable(dev);
+	if (error)
+		return dev_err_probe(dev, error, "Failed to enable pm_runtime\n");
+
+	error = pm_runtime_resume_and_get(dev);
+	if (error)
+		return dev_err_probe(dev, error, "pm_runtime_resume_and_get failed\n");
+
+	for (unsigned int i = 0; i < data->init_val_count; i++)
+		writel(data->init_vals[i].val, priv->base + data->init_vals[i].reg);
+
+	pm_runtime_put(dev);
+
+	priv->rcdev.ops = &rzv2h_usbphy_ctrl_reset_ops;
+	priv->rcdev.of_reset_n_cells = 0;
+	priv->rcdev.nr_resets = 1;
+	priv->rcdev.of_xlate = rzv2h_reset_of_xlate;
+	priv->rcdev.of_node = dev->of_node;
+	priv->rcdev.dev = dev;
+
+	return devm_reset_controller_register(dev, &priv->rcdev);
+}
+
+static const struct rzv2h_usb2phy_regval rzv2h_init_vals[] = {
+	{ .reg = 0xc10, .val = 0x67c },
+	{ .reg = 0xc14, .val = 0x1f },
+	{ .reg = 0x600, .val = 0x909 },
+};
+
+static const struct rzv2h_usb2phy_data rzv2h_of_data = {
+	.init_vals = rzv2h_init_vals,
+	.init_val_count = ARRAY_SIZE(rzv2h_init_vals),
+	.ctrl_reg = 0,
+	.ctrl_assert_val = 0x206,
+	.ctrl_status_bits = BIT(2),
+	.ctrl_deassert_val = 0x200,
+	.ctrl_release_val = 0x0,
+	.ctrl2_reg = 0xb04,
+	.ctrl2_acquire_val = 0x303,
+	.ctrl2_release_val = 0x3,
+};
+
+static const struct of_device_id rzv2h_usb2phy_ctrl_match_table[] = {
+	{ .compatible = "renesas,r9a09g057-usb2phy-ctrl", .data = &rzv2h_of_data },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzv2h_usb2phy_ctrl_match_table);
+
+static struct platform_driver rzv2h_usb2phy_ctrl_driver = {
+	.driver = {
+		.name		= "rzv2h_usb2phy_ctrl",
+		.of_match_table	= rzv2h_usb2phy_ctrl_match_table,
+	},
+	.probe = rzv2h_usb2phy_ctrl_probe,
+};
+module_platform_driver(rzv2h_usb2phy_ctrl_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
+MODULE_DESCRIPTION("Renesas RZ/V2H(P) USB2PHY Control");
-- 
2.43.0


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

* Re: [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  2025-03-05 12:39 ` [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control Prabhakar
@ 2025-03-05 16:26   ` Conor Dooley
  2025-03-05 21:35     ` Lad, Prabhakar
  2025-03-06 11:15   ` Fabrizio Castro
  1 sibling, 1 reply; 20+ messages in thread
From: Conor Dooley @ 2025-03-05 16:26 UTC (permalink / raw)
  To: Prabhakar
  Cc: Philipp Zabel, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm, devicetree,
	linux-kernel, linux-renesas-soc, Biju Das, Fabrizio Castro,
	Lad Prabhakar

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

On Wed, Mar 05, 2025 at 12:39:13PM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Add device tree binding document for the Renesas RZ/V2H(P) USB2PHY Control
> Device. It mainly controls reset and power down of the USB2.0 PHY (for
> both host and function).
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../reset/renesas,rzv2h-usb2phy-ctrl.yaml     | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml b/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> new file mode 100644
> index 000000000000..ed156a1d3eb3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/renesas,rzv2h-usb2phy-ctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/V2H(P) USB2PHY Control
> +
> +maintainers:
> +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> +
> +description:
> +  The RZ/V2H(P) USB2PHY Control mainly controls reset and power down of the
> +  USB2.0 PHY.
> +
> +properties:
> +  compatible:
> +    const: renesas,r9a09g057-usb2phy-ctrl  # RZ/V2H(P)
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  '#reset-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +  - power-domains
> +  - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/renesas,r9a09g057-cpg.h>
> +
> +    usbphy-ctrl@15830000 {

How come your nodename isn't "reset-controller"?
Otherwise,
Acked-by: Conor Dooley <conor.dooley@microchip.com>

> +        compatible = "renesas,r9a09g057-usb2phy-ctrl";
> +        reg = <0x15830000 0x10000>;
> +        clocks = <&cpg CPG_MOD 0xb6>;
> +        resets = <&cpg 0xaf>;
> +        power-domains = <&cpg>;
> +        #reset-cells = <0>;
> +    };
> -- 
> 2.43.0
> 

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

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

* Re: [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  2025-03-05 16:26   ` Conor Dooley
@ 2025-03-05 21:35     ` Lad, Prabhakar
  2025-03-06 16:26       ` Conor Dooley
  0 siblings, 1 reply; 20+ messages in thread
From: Lad, Prabhakar @ 2025-03-05 21:35 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Philipp Zabel, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm, devicetree,
	linux-kernel, linux-renesas-soc, Biju Das, Fabrizio Castro,
	Lad Prabhakar

Hi Conor,

Thank you for the review.

On Wed, Mar 5, 2025 at 4:26 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Mar 05, 2025 at 12:39:13PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add device tree binding document for the Renesas RZ/V2H(P) USB2PHY Control
> > Device. It mainly controls reset and power down of the USB2.0 PHY (for
> > both host and function).
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../reset/renesas,rzv2h-usb2phy-ctrl.yaml     | 56 +++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml b/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> > new file mode 100644
> > index 000000000000..ed156a1d3eb3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> > @@ -0,0 +1,56 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/reset/renesas,rzv2h-usb2phy-ctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas RZ/V2H(P) USB2PHY Control
> > +
> > +maintainers:
> > +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > +
> > +description:
> > +  The RZ/V2H(P) USB2PHY Control mainly controls reset and power down of the
> > +  USB2.0 PHY.
> > +
> > +properties:
> > +  compatible:
> > +    const: renesas,r9a09g057-usb2phy-ctrl  # RZ/V2H(P)
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  '#reset-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - resets
> > +  - power-domains
> > +  - '#reset-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/renesas,r9a09g057-cpg.h>
> > +
> > +    usbphy-ctrl@15830000 {
>
> How come your nodename isn't "reset-controller"?
This is to keep consistency with the other similar IP blocks found on
Renesas SoCs [0].

[0] https://elixir.bootlin.com/linux/v6.14-rc5/source/Documentation/devicetree/bindings/reset/renesas,rzg2l-usbphy-ctrl.yaml#L66

> Otherwise,
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>
> > +        compatible = "renesas,r9a09g057-usb2phy-ctrl";
> > +        reg = <0x15830000 0x10000>;
> > +        clocks = <&cpg CPG_MOD 0xb6>;
> > +        resets = <&cpg 0xaf>;
> > +        power-domains = <&cpg>;
> > +        #reset-cells = <0>;
> > +    };
> > --
> > 2.43.0
> >

Cheers,
Prabhakar

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

* RE: [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  2025-03-05 12:39 ` [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control Prabhakar
  2025-03-05 16:26   ` Conor Dooley
@ 2025-03-06 11:15   ` Fabrizio Castro
  1 sibling, 0 replies; 20+ messages in thread
From: Fabrizio Castro @ 2025-03-06 11:15 UTC (permalink / raw)
  To: Prabhakar, Philipp Zabel, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm
  Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, Biju Das,
	Prabhakar Mahadev Lad

> From: Prabhakar <prabhakar.csengg@gmail.com>
> Sent: 05 March 2025 12:39
> Subject: [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Add device tree binding document for the Renesas RZ/V2H(P) USB2PHY Control
> Device. It mainly controls reset and power down of the USB2.0 PHY (for
> both host and function).
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

> ---
>  .../reset/renesas,rzv2h-usb2phy-ctrl.yaml     | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> b/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> new file mode 100644
> index 000000000000..ed156a1d3eb3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/renesas,rzv2h-usb2phy-ctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/V2H(P) USB2PHY Control
> +
> +maintainers:
> +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> +
> +description:
> +  The RZ/V2H(P) USB2PHY Control mainly controls reset and power down of the
> +  USB2.0 PHY.
> +
> +properties:
> +  compatible:
> +    const: renesas,r9a09g057-usb2phy-ctrl  # RZ/V2H(P)
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  '#reset-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +  - power-domains
> +  - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/renesas,r9a09g057-cpg.h>
> +
> +    usbphy-ctrl@15830000 {
> +        compatible = "renesas,r9a09g057-usb2phy-ctrl";
> +        reg = <0x15830000 0x10000>;
> +        clocks = <&cpg CPG_MOD 0xb6>;
> +        resets = <&cpg 0xaf>;
> +        power-domains = <&cpg>;
> +        #reset-cells = <0>;
> +    };
> --
> 2.43.0


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

* RE: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
  2025-03-05 12:39 ` [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P) Prabhakar
@ 2025-03-06 11:17   ` Fabrizio Castro
  2025-03-13  8:37   ` Philipp Zabel
  1 sibling, 0 replies; 20+ messages in thread
From: Fabrizio Castro @ 2025-03-06 11:17 UTC (permalink / raw)
  To: Prabhakar, Philipp Zabel, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm
  Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, Biju Das,
	Prabhakar Mahadev Lad

> From: Prabhakar <prabhakar.csengg@gmail.com>
> Sent: 05 March 2025 12:39
> Subject: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Add support for the USB2PHY control driver on the Renesas RZ/V2H(P) SoC.
> Make the driver handle reset and power-down operations for the USB2PHY.
> 
> Pass OF data to support future SoCs with similar USB2PHY hardware but
> different register configurations. Define device-specific initialization
> values and control register settings in OF data to ensure flexibility
> for upcoming SoCs.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

> ---
>  drivers/reset/Kconfig                    |   7 +
>  drivers/reset/Makefile                   |   1 +
>  drivers/reset/reset-rzv2h-usb2phy-ctrl.c | 223 +++++++++++++++++++++++
>  3 files changed, 231 insertions(+)
>  create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 5b3abb6db248..bac08dae8905 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -218,6 +218,13 @@ config RESET_RZG2L_USBPHY_CTRL
>  	  Support for USBPHY Control found on RZ/G2L family. It mainly
>  	  controls reset and power down of the USB/PHY.
> 
> +config RESET_RZV2H_USB2PHY_CTRL
> +	tristate "Renesas RZ/V2H(P) (and similar SoCs) USB2PHY control driver"
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	help
> +	  Support for USB2PHY Control found on the RZ/V2H(P) SoC (and similar SoCs).
> +	  It mainly controls reset and power down of the USB2 PHY.
> +
>  config RESET_SCMI
>  	tristate "Reset driver controlled via ARM SCMI interface"
>  	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 677c4d1e2632..3cb3df018cf8 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
>  obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
>  obj-$(CONFIG_RESET_RASPBERRYPI) += reset-raspberrypi.o
>  obj-$(CONFIG_RESET_RZG2L_USBPHY_CTRL) += reset-rzg2l-usbphy-ctrl.o
> +obj-$(CONFIG_RESET_RZV2H_USB2PHY_CTRL) += reset-rzv2h-usb2phy-ctrl.o
>  obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
>  obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
>  obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> diff --git a/drivers/reset/reset-rzv2h-usb2phy-ctrl.c b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> new file mode 100644
> index 000000000000..a6daeaf37e1c
> --- /dev/null
> +++ b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/V2H(P) USB2PHY control driver
> + *
> + * Copyright (C) 2025 Renesas Electronics Corporation
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/reset-controller.h>
> +
> +struct rzv2h_usb2phy_regval {
> +	u16 reg;
> +	u16 val;
> +};
> +
> +struct rzv2h_usb2phy_data {
> +	const struct rzv2h_usb2phy_regval *init_vals;
> +	unsigned int init_val_count;
> +
> +	u16 ctrl_reg;
> +	u16 ctrl_assert_val;
> +	u16 ctrl_deassert_val;
> +	u16 ctrl_status_bits;
> +	u16 ctrl_release_val;
> +
> +	u16 ctrl2_reg;
> +	u16 ctrl2_acquire_val;
> +	u16 ctrl2_release_val;
> +};
> +
> +struct rzv2h_usb2phy_ctrl_priv {
> +	const struct rzv2h_usb2phy_data *data;
> +	void __iomem *base;
> +	struct device *dev;
> +	struct reset_controller_dev rcdev;
> +	spinlock_t lock;
> +};
> +
> +#define rcdev_to_priv(x) container_of(x, struct rzv2h_usb2phy_ctrl_priv, rcdev)
> +
> +static int rzv2h_usbphy_ctrl_assert(struct reset_controller_dev *rcdev,
> +				    unsigned long id)
> +{
> +	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> +	const struct rzv2h_usb2phy_data *data = priv->data;
> +	struct device *dev = priv->dev;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> +		return ret;
> +	}
> +	scoped_guard(spinlock, &priv->lock) {
> +		writel(data->ctrl2_acquire_val, priv->base + data->ctrl2_reg);
> +		writel(data->ctrl_assert_val, priv->base + data->ctrl_reg);
> +	}
> +
> +	/* The reset line needs to be asserted for more than 10 microseconds. */
> +	udelay(11);
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +}
> +
> +static int rzv2h_usbphy_ctrl_deassert(struct reset_controller_dev *rcdev,
> +				      unsigned long id)
> +{
> +	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> +	const struct rzv2h_usb2phy_data *data = priv->data;
> +	struct device *dev = priv->dev;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> +		return ret;
> +	}
> +
> +	scoped_guard(spinlock, &priv->lock) {
> +		writel(data->ctrl_deassert_val, priv->base + data->ctrl_reg);
> +		writel(data->ctrl2_release_val, priv->base + data->ctrl2_reg);
> +		writel(data->ctrl_release_val, priv->base + data->ctrl_reg);
> +	}
> +
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +}
> +
> +static int rzv2h_usbphy_ctrl_status(struct reset_controller_dev *rcdev,
> +				    unsigned long id)
> +{
> +	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> +	struct device *dev = priv->dev;
> +	int ret;
> +	u32 reg;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> +		return ret;
> +	}
> +
> +	scoped_guard(spinlock, &priv->lock)
> +		reg = readl(priv->base + priv->data->ctrl_reg);
> +
> +	pm_runtime_put(dev);
> +
> +	return (reg & priv->data->ctrl_status_bits) == priv->data->ctrl_status_bits;
> +}
> +
> +static const struct reset_control_ops rzv2h_usbphy_ctrl_reset_ops = {
> +	.assert = rzv2h_usbphy_ctrl_assert,
> +	.deassert = rzv2h_usbphy_ctrl_deassert,
> +	.status = rzv2h_usbphy_ctrl_status,
> +};
> +
> +static int rzv2h_reset_of_xlate(struct reset_controller_dev *rcdev,
> +				const struct of_phandle_args *reset_spec)
> +{
> +	/* No special handling needed, we have only one reset line per device */
> +	return 0;
> +}
> +
> +static int rzv2h_usb2phy_ctrl_probe(struct platform_device *pdev)
> +{
> +	const struct rzv2h_usb2phy_data *data;
> +	struct rzv2h_usb2phy_ctrl_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
> +	int error;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	data = of_device_get_match_data(dev);
> +	if (!data)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "failed to match device\n");
> +
> +	priv->data = data;
> +	priv->dev = dev;
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	rstc = devm_reset_control_get_shared_deasserted(dev, NULL);
> +	if (IS_ERR(rstc))
> +		return dev_err_probe(dev, PTR_ERR(rstc),
> +				     "failed to get deasserted reset\n");
> +
> +	spin_lock_init(&priv->lock);
> +	dev_set_drvdata(dev, priv);
> +
> +	error = devm_pm_runtime_enable(dev);
> +	if (error)
> +		return dev_err_probe(dev, error, "Failed to enable pm_runtime\n");
> +
> +	error = pm_runtime_resume_and_get(dev);
> +	if (error)
> +		return dev_err_probe(dev, error, "pm_runtime_resume_and_get failed\n");
> +
> +	for (unsigned int i = 0; i < data->init_val_count; i++)
> +		writel(data->init_vals[i].val, priv->base + data->init_vals[i].reg);
> +
> +	pm_runtime_put(dev);
> +
> +	priv->rcdev.ops = &rzv2h_usbphy_ctrl_reset_ops;
> +	priv->rcdev.of_reset_n_cells = 0;
> +	priv->rcdev.nr_resets = 1;
> +	priv->rcdev.of_xlate = rzv2h_reset_of_xlate;
> +	priv->rcdev.of_node = dev->of_node;
> +	priv->rcdev.dev = dev;
> +
> +	return devm_reset_controller_register(dev, &priv->rcdev);
> +}
> +
> +static const struct rzv2h_usb2phy_regval rzv2h_init_vals[] = {
> +	{ .reg = 0xc10, .val = 0x67c },
> +	{ .reg = 0xc14, .val = 0x1f },
> +	{ .reg = 0x600, .val = 0x909 },
> +};
> +
> +static const struct rzv2h_usb2phy_data rzv2h_of_data = {
> +	.init_vals = rzv2h_init_vals,
> +	.init_val_count = ARRAY_SIZE(rzv2h_init_vals),
> +	.ctrl_reg = 0,
> +	.ctrl_assert_val = 0x206,
> +	.ctrl_status_bits = BIT(2),
> +	.ctrl_deassert_val = 0x200,
> +	.ctrl_release_val = 0x0,
> +	.ctrl2_reg = 0xb04,
> +	.ctrl2_acquire_val = 0x303,
> +	.ctrl2_release_val = 0x3,
> +};
> +
> +static const struct of_device_id rzv2h_usb2phy_ctrl_match_table[] = {
> +	{ .compatible = "renesas,r9a09g057-usb2phy-ctrl", .data = &rzv2h_of_data },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rzv2h_usb2phy_ctrl_match_table);
> +
> +static struct platform_driver rzv2h_usb2phy_ctrl_driver = {
> +	.driver = {
> +		.name		= "rzv2h_usb2phy_ctrl",
> +		.of_match_table	= rzv2h_usb2phy_ctrl_match_table,
> +	},
> +	.probe = rzv2h_usb2phy_ctrl_probe,
> +};
> +module_platform_driver(rzv2h_usb2phy_ctrl_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas RZ/V2H(P) USB2PHY Control");
> --
> 2.43.0


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

* Re: [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  2025-03-05 21:35     ` Lad, Prabhakar
@ 2025-03-06 16:26       ` Conor Dooley
  2025-03-13 13:09         ` Philipp Zabel
  0 siblings, 1 reply; 20+ messages in thread
From: Conor Dooley @ 2025-03-06 16:26 UTC (permalink / raw)
  To: Lad, Prabhakar
  Cc: Philipp Zabel, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm, devicetree,
	linux-kernel, linux-renesas-soc, Biju Das, Fabrizio Castro,
	Lad Prabhakar

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

On Wed, Mar 05, 2025 at 09:35:13PM +0000, Lad, Prabhakar wrote:
> Hi Conor,
> 
> Thank you for the review.
> 
> On Wed, Mar 5, 2025 at 4:26 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Wed, Mar 05, 2025 at 12:39:13PM +0000, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Add device tree binding document for the Renesas RZ/V2H(P) USB2PHY Control
> > > Device. It mainly controls reset and power down of the USB2.0 PHY (for
> > > both host and function).
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  .../reset/renesas,rzv2h-usb2phy-ctrl.yaml     | 56 +++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml b/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> > > new file mode 100644
> > > index 000000000000..ed156a1d3eb3
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/reset/renesas,rzv2h-usb2phy-ctrl.yaml
> > > @@ -0,0 +1,56 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/reset/renesas,rzv2h-usb2phy-ctrl.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Renesas RZ/V2H(P) USB2PHY Control
> > > +
> > > +maintainers:
> > > +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > +
> > > +description:
> > > +  The RZ/V2H(P) USB2PHY Control mainly controls reset and power down of the
> > > +  USB2.0 PHY.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: renesas,r9a09g057-usb2phy-ctrl  # RZ/V2H(P)
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  resets:
> > > +    maxItems: 1
> > > +
> > > +  power-domains:
> > > +    maxItems: 1
> > > +
> > > +  '#reset-cells':
> > > +    const: 0
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - resets
> > > +  - power-domains
> > > +  - '#reset-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/renesas,r9a09g057-cpg.h>
> > > +
> > > +    usbphy-ctrl@15830000 {
> >
> > How come your nodename isn't "reset-controller"?
> This is to keep consistency with the other similar IP blocks found on
> Renesas SoCs [0].

That sounds awfully like "it was wrong before, and I want to keep using
the wrong node name"... If you're claiming to be some other class of
device, "ctrl" should really be "controller" like all the other sorts of
controllers ;)

> 
> [0] https://elixir.bootlin.com/linux/v6.14-rc5/source/Documentation/devicetree/bindings/reset/renesas,rzg2l-usbphy-ctrl.yaml#L66
> 
> > Otherwise,
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> >
> > > +        compatible = "renesas,r9a09g057-usb2phy-ctrl";
> > > +        reg = <0x15830000 0x10000>;
> > > +        clocks = <&cpg CPG_MOD 0xb6>;
> > > +        resets = <&cpg 0xaf>;
> > > +        power-domains = <&cpg>;
> > > +        #reset-cells = <0>;
> > > +    };
> > > --
> > > 2.43.0
> > >
> 
> Cheers,
> Prabhakar

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

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

* Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
  2025-03-05 12:39 ` [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P) Prabhakar
  2025-03-06 11:17   ` Fabrizio Castro
@ 2025-03-13  8:37   ` Philipp Zabel
  2025-03-13 10:14     ` Fabrizio Castro
  2025-03-13 11:23     ` Lad, Prabhakar
  1 sibling, 2 replies; 20+ messages in thread
From: Philipp Zabel @ 2025-03-13  8:37 UTC (permalink / raw)
  To: Prabhakar, Geert Uytterhoeven, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Magnus Damm
  Cc: devicetree, linux-kernel, linux-renesas-soc, Biju Das,
	Fabrizio Castro, Lad Prabhakar

On Mi, 2025-03-05 at 12:39 +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Add support for the USB2PHY control driver on the Renesas RZ/V2H(P) SoC.
> Make the driver handle reset and power-down operations for the USB2PHY.
> 
> Pass OF data to support future SoCs with similar USB2PHY hardware but
> different register configurations. Define device-specific initialization
> values and control register settings in OF data to ensure flexibility
> for upcoming SoCs.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/reset/Kconfig                    |   7 +
>  drivers/reset/Makefile                   |   1 +
>  drivers/reset/reset-rzv2h-usb2phy-ctrl.c | 223 +++++++++++++++++++++++
>  3 files changed, 231 insertions(+)
>  create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 5b3abb6db248..bac08dae8905 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -218,6 +218,13 @@ config RESET_RZG2L_USBPHY_CTRL
>  	  Support for USBPHY Control found on RZ/G2L family. It mainly
>  	  controls reset and power down of the USB/PHY.
>  
> +config RESET_RZV2H_USB2PHY_CTRL
> +	tristate "Renesas RZ/V2H(P) (and similar SoCs) USB2PHY control driver"
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	help
> +	  Support for USB2PHY Control found on the RZ/V2H(P) SoC (and similar SoCs).
> +	  It mainly controls reset and power down of the USB2 PHY.
> +
>  config RESET_SCMI
>  	tristate "Reset driver controlled via ARM SCMI interface"
>  	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 677c4d1e2632..3cb3df018cf8 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
>  obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
>  obj-$(CONFIG_RESET_RASPBERRYPI) += reset-raspberrypi.o
>  obj-$(CONFIG_RESET_RZG2L_USBPHY_CTRL) += reset-rzg2l-usbphy-ctrl.o
> +obj-$(CONFIG_RESET_RZV2H_USB2PHY_CTRL) += reset-rzv2h-usb2phy-ctrl.o
>  obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
>  obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
>  obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> diff --git a/drivers/reset/reset-rzv2h-usb2phy-ctrl.c b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> new file mode 100644
> index 000000000000..a6daeaf37e1c
> --- /dev/null
> +++ b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/V2H(P) USB2PHY control driver
> + *
> + * Copyright (C) 2025 Renesas Electronics Corporation
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/reset-controller.h>
> +
> +struct rzv2h_usb2phy_regval {
> +	u16 reg;
> +	u16 val;
> +};
> +
> +struct rzv2h_usb2phy_data {
> +	const struct rzv2h_usb2phy_regval *init_vals;
> +	unsigned int init_val_count;
> +
> +	u16 ctrl_reg;
> +	u16 ctrl_assert_val;
> +	u16 ctrl_deassert_val;
> +	u16 ctrl_status_bits;
> +	u16 ctrl_release_val;
> +
> +	u16 ctrl2_reg;
> +	u16 ctrl2_acquire_val;
> +	u16 ctrl2_release_val;
> +};
> +
> +struct rzv2h_usb2phy_ctrl_priv {
> +	const struct rzv2h_usb2phy_data *data;
> +	void __iomem *base;
> +	struct device *dev;
> +	struct reset_controller_dev rcdev;
> +	spinlock_t lock;

Lock without comment.

> +};
> +
> +#define rcdev_to_priv(x) container_of(x, struct rzv2h_usb2phy_ctrl_priv, rcdev)

I'd prefer this to be an inline function.

> +
> +static int rzv2h_usbphy_ctrl_assert(struct reset_controller_dev *rcdev,
> +				    unsigned long id)
> +{
> +	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> +	const struct rzv2h_usb2phy_data *data = priv->data;
> +	struct device *dev = priv->dev;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> +		return ret;
> +	}
> +	scoped_guard(spinlock, &priv->lock) {
> +		writel(data->ctrl2_acquire_val, priv->base + data->ctrl2_reg);

Comparing to deassert, I wonder why is there no ctrl_acquire_val?

> +		writel(data->ctrl_assert_val, priv->base + data->ctrl_reg);
> +	}
> +
> +	/* The reset line needs to be asserted for more than 10 microseconds. */
> +	udelay(11);

Could this be usleep_range() instead?

> +	pm_runtime_put(dev);
> +
> +	return 0;
> +}
> +
> +static int rzv2h_usbphy_ctrl_deassert(struct reset_controller_dev *rcdev,
> +				      unsigned long id)
> +{
> +	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> +	const struct rzv2h_usb2phy_data *data = priv->data;
> +	struct device *dev = priv->dev;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> +		return ret;
> +	}
> +
> +	scoped_guard(spinlock, &priv->lock) {
> +		writel(data->ctrl_deassert_val, priv->base + data->ctrl_reg);
> +		writel(data->ctrl2_release_val, priv->base + data->ctrl2_reg);
> +		writel(data->ctrl_release_val, priv->base + data->ctrl_reg);
> +	}
> +
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +}
> +
> +static int rzv2h_usbphy_ctrl_status(struct reset_controller_dev *rcdev,
> +				    unsigned long id)
> +{
> +	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> +	struct device *dev = priv->dev;
> +	int ret;
> +	u32 reg;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> +		return ret;
> +	}
> +
> +	scoped_guard(spinlock, &priv->lock)
> +		reg = readl(priv->base + priv->data->ctrl_reg);
> +
> +	pm_runtime_put(dev);
> +
> +	return (reg & priv->data->ctrl_status_bits) == priv->data->ctrl_status_bits;
> +}
> +
> +static const struct reset_control_ops rzv2h_usbphy_ctrl_reset_ops = {
> +	.assert = rzv2h_usbphy_ctrl_assert,
> +	.deassert = rzv2h_usbphy_ctrl_deassert,
> +	.status = rzv2h_usbphy_ctrl_status,
> +};
> +
> +static int rzv2h_reset_of_xlate(struct reset_controller_dev *rcdev,
> +				const struct of_phandle_args *reset_spec)
> +{
> +	/* No special handling needed, we have only one reset line per device */
> +	return 0;
> +}
> +
> +static int rzv2h_usb2phy_ctrl_probe(struct platform_device *pdev)
> +{
> +	const struct rzv2h_usb2phy_data *data;
> +	struct rzv2h_usb2phy_ctrl_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
> +	int error;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	data = of_device_get_match_data(dev);
> +	if (!data)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "failed to match device\n");
> +
> +	priv->data = data;
> +	priv->dev = dev;
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	rstc = devm_reset_control_get_shared_deasserted(dev, NULL);
> +	if (IS_ERR(rstc))
> +		return dev_err_probe(dev, PTR_ERR(rstc),
> +				     "failed to get deasserted reset\n");
> +
> +	spin_lock_init(&priv->lock);
> +	dev_set_drvdata(dev, priv);
> +
> +	error = devm_pm_runtime_enable(dev);
> +	if (error)
> +		return dev_err_probe(dev, error, "Failed to enable pm_runtime\n");
> +
> +	error = pm_runtime_resume_and_get(dev);
> +	if (error)
> +		return dev_err_probe(dev, error, "pm_runtime_resume_and_get failed\n");
> +
> +	for (unsigned int i = 0; i < data->init_val_count; i++)
> +		writel(data->init_vals[i].val, priv->base + data->init_vals[i].reg);
> +
> +	pm_runtime_put(dev);
> +
> +	priv->rcdev.ops = &rzv2h_usbphy_ctrl_reset_ops;
> +	priv->rcdev.of_reset_n_cells = 0;
> +	priv->rcdev.nr_resets = 1;
> +	priv->rcdev.of_xlate = rzv2h_reset_of_xlate;
> +	priv->rcdev.of_node = dev->of_node;
> +	priv->rcdev.dev = dev;
> +
> +	return devm_reset_controller_register(dev, &priv->rcdev);
> +}
> +
> +static const struct rzv2h_usb2phy_regval rzv2h_init_vals[] = {
> +	{ .reg = 0xc10, .val = 0x67c },
> +	{ .reg = 0xc14, .val = 0x1f },
> +	{ .reg = 0x600, .val = 0x909 },

What are these registers and what are those values doing?

> +};
> +
> +static const struct rzv2h_usb2phy_data rzv2h_of_data = {
> +	.init_vals = rzv2h_init_vals,
> +	.init_val_count = ARRAY_SIZE(rzv2h_init_vals),
> +	.ctrl_reg = 0,
> +	.ctrl_assert_val = 0x206,
> +	.ctrl_status_bits = BIT(2),
> +	.ctrl_deassert_val = 0x200,
> +	.ctrl_release_val = 0x0,
> +	.ctrl2_reg = 0xb04,
> +	.ctrl2_acquire_val = 0x303,
> +	.ctrl2_release_val = 0x3,

This is really opaque. I would have expected some defines for the
register bits (or fields?) at least.

> +};
> +
> +static const struct of_device_id rzv2h_usb2phy_ctrl_match_table[] = {
> +	{ .compatible = "renesas,r9a09g057-usb2phy-ctrl", .data = &rzv2h_of_data },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rzv2h_usb2phy_ctrl_match_table);
> +
> +static struct platform_driver rzv2h_usb2phy_ctrl_driver = {
> +	.driver = {
> +		.name		= "rzv2h_usb2phy_ctrl",
> +		.of_match_table	= rzv2h_usb2phy_ctrl_match_table,
> +	},
> +	.probe = rzv2h_usb2phy_ctrl_probe,
> +};
> +module_platform_driver(rzv2h_usb2phy_ctrl_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas RZ/V2H(P) USB2PHY Control");

regards
Philipp


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

* RE: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
  2025-03-13  8:37   ` Philipp Zabel
@ 2025-03-13 10:14     ` Fabrizio Castro
  2025-03-13 13:06       ` Philipp Zabel
  2025-03-13 11:23     ` Lad, Prabhakar
  1 sibling, 1 reply; 20+ messages in thread
From: Fabrizio Castro @ 2025-03-13 10:14 UTC (permalink / raw)
  To: Philipp Zabel, Prabhakar, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm
  Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, Biju Das,
	Prabhakar Mahadev Lad, Chris Paterson

Hi Philipp,

Thanks for your feedback!

> From: Philipp Zabel <p.zabel@pengutronix.de>
> Sent: 13 March 2025 08:37
> Subject: Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
> 
> On Mi, 2025-03-05 at 12:39 +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add support for the USB2PHY control driver on the Renesas RZ/V2H(P) SoC.
> > Make the driver handle reset and power-down operations for the USB2PHY.
> >
> > Pass OF data to support future SoCs with similar USB2PHY hardware but
> > different register configurations. Define device-specific initialization
> > values and control register settings in OF data to ensure flexibility
> > for upcoming SoCs.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/reset/Kconfig                    |   7 +
> >  drivers/reset/Makefile                   |   1 +
> >  drivers/reset/reset-rzv2h-usb2phy-ctrl.c | 223 +++++++++++++++++++++++
> >  3 files changed, 231 insertions(+)
> >  create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> >
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 5b3abb6db248..bac08dae8905 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -218,6 +218,13 @@ config RESET_RZG2L_USBPHY_CTRL
> >  	  Support for USBPHY Control found on RZ/G2L family. It mainly
> >  	  controls reset and power down of the USB/PHY.
> >
> > +config RESET_RZV2H_USB2PHY_CTRL
> > +	tristate "Renesas RZ/V2H(P) (and similar SoCs) USB2PHY control driver"
> > +	depends on ARCH_RENESAS || COMPILE_TEST
> > +	help
> > +	  Support for USB2PHY Control found on the RZ/V2H(P) SoC (and similar SoCs).
> > +	  It mainly controls reset and power down of the USB2 PHY.
> > +
> >  config RESET_SCMI
> >  	tristate "Reset driver controlled via ARM SCMI interface"
> >  	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index 677c4d1e2632..3cb3df018cf8 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -30,6 +30,7 @@ obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> >  obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
> >  obj-$(CONFIG_RESET_RASPBERRYPI) += reset-raspberrypi.o
> >  obj-$(CONFIG_RESET_RZG2L_USBPHY_CTRL) += reset-rzg2l-usbphy-ctrl.o
> > +obj-$(CONFIG_RESET_RZV2H_USB2PHY_CTRL) += reset-rzv2h-usb2phy-ctrl.o
> >  obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
> >  obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> >  obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> > diff --git a/drivers/reset/reset-rzv2h-usb2phy-ctrl.c b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > new file mode 100644
> > index 000000000000..a6daeaf37e1c
> > --- /dev/null
> > +++ b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > @@ -0,0 +1,223 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/V2H(P) USB2PHY control driver
> > + *
> > + * Copyright (C) 2025 Renesas Electronics Corporation
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/reset.h>
> > +#include <linux/reset-controller.h>
> > +
> > +struct rzv2h_usb2phy_regval {
> > +	u16 reg;
> > +	u16 val;
> > +};
> > +
> > +struct rzv2h_usb2phy_data {
> > +	const struct rzv2h_usb2phy_regval *init_vals;
> > +	unsigned int init_val_count;
> > +
> > +	u16 ctrl_reg;
> > +	u16 ctrl_assert_val;
> > +	u16 ctrl_deassert_val;
> > +	u16 ctrl_status_bits;
> > +	u16 ctrl_release_val;
> > +
> > +	u16 ctrl2_reg;
> > +	u16 ctrl2_acquire_val;
> > +	u16 ctrl2_release_val;
> > +};
> > +
> > +struct rzv2h_usb2phy_ctrl_priv {
> > +	const struct rzv2h_usb2phy_data *data;
> > +	void __iomem *base;
> > +	struct device *dev;
> > +	struct reset_controller_dev rcdev;
> > +	spinlock_t lock;
> 
> Lock without comment.
> 
> > +};
> > +
> > +#define rcdev_to_priv(x) container_of(x, struct rzv2h_usb2phy_ctrl_priv, rcdev)
> 
> I'd prefer this to be an inline function.
> 
> > +
> > +static int rzv2h_usbphy_ctrl_assert(struct reset_controller_dev *rcdev,
> > +				    unsigned long id)
> > +{
> > +	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> > +	const struct rzv2h_usb2phy_data *data = priv->data;
> > +	struct device *dev = priv->dev;
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret) {
> > +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> > +		return ret;
> > +	}
> > +	scoped_guard(spinlock, &priv->lock) {
> > +		writel(data->ctrl2_acquire_val, priv->base + data->ctrl2_reg);
> 
> Comparing to deassert, I wonder why is there no ctrl_acquire_val?
> 
> > +		writel(data->ctrl_assert_val, priv->base + data->ctrl_reg);
> > +	}
> > +
> > +	/* The reset line needs to be asserted for more than 10 microseconds. */
> > +	udelay(11);
> 
> Could this be usleep_range() instead?
> 
> > +	pm_runtime_put(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzv2h_usbphy_ctrl_deassert(struct reset_controller_dev *rcdev,
> > +				      unsigned long id)
> > +{
> > +	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> > +	const struct rzv2h_usb2phy_data *data = priv->data;
> > +	struct device *dev = priv->dev;
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret) {
> > +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> > +		return ret;
> > +	}
> > +
> > +	scoped_guard(spinlock, &priv->lock) {
> > +		writel(data->ctrl_deassert_val, priv->base + data->ctrl_reg);
> > +		writel(data->ctrl2_release_val, priv->base + data->ctrl2_reg);
> > +		writel(data->ctrl_release_val, priv->base + data->ctrl_reg);
> > +	}
> > +
> > +	pm_runtime_put(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzv2h_usbphy_ctrl_status(struct reset_controller_dev *rcdev,
> > +				    unsigned long id)
> > +{
> > +	struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> > +	struct device *dev = priv->dev;
> > +	int ret;
> > +	u32 reg;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret) {
> > +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> > +		return ret;
> > +	}
> > +
> > +	scoped_guard(spinlock, &priv->lock)
> > +		reg = readl(priv->base + priv->data->ctrl_reg);
> > +
> > +	pm_runtime_put(dev);
> > +
> > +	return (reg & priv->data->ctrl_status_bits) == priv->data->ctrl_status_bits;
> > +}
> > +
> > +static const struct reset_control_ops rzv2h_usbphy_ctrl_reset_ops = {
> > +	.assert = rzv2h_usbphy_ctrl_assert,
> > +	.deassert = rzv2h_usbphy_ctrl_deassert,
> > +	.status = rzv2h_usbphy_ctrl_status,
> > +};
> > +
> > +static int rzv2h_reset_of_xlate(struct reset_controller_dev *rcdev,
> > +				const struct of_phandle_args *reset_spec)
> > +{
> > +	/* No special handling needed, we have only one reset line per device */
> > +	return 0;
> > +}
> > +
> > +static int rzv2h_usb2phy_ctrl_probe(struct platform_device *pdev)
> > +{
> > +	const struct rzv2h_usb2phy_data *data;
> > +	struct rzv2h_usb2phy_ctrl_priv *priv;
> > +	struct device *dev = &pdev->dev;
> > +	struct reset_control *rstc;
> > +	int error;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	data = of_device_get_match_data(dev);
> > +	if (!data)
> > +		return dev_err_probe(dev, -ENODEV,
> > +				     "failed to match device\n");
> > +
> > +	priv->data = data;
> > +	priv->dev = dev;
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	rstc = devm_reset_control_get_shared_deasserted(dev, NULL);
> > +	if (IS_ERR(rstc))
> > +		return dev_err_probe(dev, PTR_ERR(rstc),
> > +				     "failed to get deasserted reset\n");
> > +
> > +	spin_lock_init(&priv->lock);
> > +	dev_set_drvdata(dev, priv);
> > +
> > +	error = devm_pm_runtime_enable(dev);
> > +	if (error)
> > +		return dev_err_probe(dev, error, "Failed to enable pm_runtime\n");
> > +
> > +	error = pm_runtime_resume_and_get(dev);
> > +	if (error)
> > +		return dev_err_probe(dev, error, "pm_runtime_resume_and_get failed\n");
> > +
> > +	for (unsigned int i = 0; i < data->init_val_count; i++)
> > +		writel(data->init_vals[i].val, priv->base + data->init_vals[i].reg);
> > +
> > +	pm_runtime_put(dev);
> > +
> > +	priv->rcdev.ops = &rzv2h_usbphy_ctrl_reset_ops;
> > +	priv->rcdev.of_reset_n_cells = 0;
> > +	priv->rcdev.nr_resets = 1;
> > +	priv->rcdev.of_xlate = rzv2h_reset_of_xlate;
> > +	priv->rcdev.of_node = dev->of_node;
> > +	priv->rcdev.dev = dev;
> > +
> > +	return devm_reset_controller_register(dev, &priv->rcdev);
> > +}
> > +
> > +static const struct rzv2h_usb2phy_regval rzv2h_init_vals[] = {
> > +	{ .reg = 0xc10, .val = 0x67c },
> > +	{ .reg = 0xc14, .val = 0x1f },
> > +	{ .reg = 0x600, .val = 0x909 },
> 
> What are these registers and what are those values doing?

Unfortunately, there are some licensing restrictions on this IP, this is
the best that we can do, as per the license agreement.

> 
> > +};
> > +
> > +static const struct rzv2h_usb2phy_data rzv2h_of_data = {
> > +	.init_vals = rzv2h_init_vals,
> > +	.init_val_count = ARRAY_SIZE(rzv2h_init_vals),
> > +	.ctrl_reg = 0,
> > +	.ctrl_assert_val = 0x206,
> > +	.ctrl_status_bits = BIT(2),
> > +	.ctrl_deassert_val = 0x200,
> > +	.ctrl_release_val = 0x0,
> > +	.ctrl2_reg = 0xb04,
> > +	.ctrl2_acquire_val = 0x303,
> > +	.ctrl2_release_val = 0x3,
> 
> This is really opaque. I would have expected some defines for the
> register bits (or fields?) at least.

We have the same limitations for these bits I am afraid.

Cheers,
Fab

> 
> > +};
> > +
> > +static const struct of_device_id rzv2h_usb2phy_ctrl_match_table[] = {
> > +	{ .compatible = "renesas,r9a09g057-usb2phy-ctrl", .data = &rzv2h_of_data },
> > +	{ /* Sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rzv2h_usb2phy_ctrl_match_table);
> > +
> > +static struct platform_driver rzv2h_usb2phy_ctrl_driver = {
> > +	.driver = {
> > +		.name		= "rzv2h_usb2phy_ctrl",
> > +		.of_match_table	= rzv2h_usb2phy_ctrl_match_table,
> > +	},
> > +	.probe = rzv2h_usb2phy_ctrl_probe,
> > +};
> > +module_platform_driver(rzv2h_usb2phy_ctrl_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
> > +MODULE_DESCRIPTION("Renesas RZ/V2H(P) USB2PHY Control");
> 
> regards
> Philipp


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

* Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
  2025-03-13  8:37   ` Philipp Zabel
  2025-03-13 10:14     ` Fabrizio Castro
@ 2025-03-13 11:23     ` Lad, Prabhakar
  1 sibling, 0 replies; 20+ messages in thread
From: Lad, Prabhakar @ 2025-03-13 11:23 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: Geert Uytterhoeven, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Magnus Damm, devicetree, linux-kernel,
	linux-renesas-soc, Biju Das, Fabrizio Castro, Lad Prabhakar

Hi Philipp,

Thank you for the review.

On Thu, Mar 13, 2025 at 8:37 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Mi, 2025-03-05 at 12:39 +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add support for the USB2PHY control driver on the Renesas RZ/V2H(P) SoC.
> > Make the driver handle reset and power-down operations for the USB2PHY.
> >
> > Pass OF data to support future SoCs with similar USB2PHY hardware but
> > different register configurations. Define device-specific initialization
> > values and control register settings in OF data to ensure flexibility
> > for upcoming SoCs.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/reset/Kconfig                    |   7 +
> >  drivers/reset/Makefile                   |   1 +
> >  drivers/reset/reset-rzv2h-usb2phy-ctrl.c | 223 +++++++++++++++++++++++
> >  3 files changed, 231 insertions(+)
> >  create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> >
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 5b3abb6db248..bac08dae8905 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -218,6 +218,13 @@ config RESET_RZG2L_USBPHY_CTRL
> >         Support for USBPHY Control found on RZ/G2L family. It mainly
> >         controls reset and power down of the USB/PHY.
> >
> > +config RESET_RZV2H_USB2PHY_CTRL
> > +     tristate "Renesas RZ/V2H(P) (and similar SoCs) USB2PHY control driver"
> > +     depends on ARCH_RENESAS || COMPILE_TEST
> > +     help
> > +       Support for USB2PHY Control found on the RZ/V2H(P) SoC (and similar SoCs).
> > +       It mainly controls reset and power down of the USB2 PHY.
> > +
> >  config RESET_SCMI
> >       tristate "Reset driver controlled via ARM SCMI interface"
> >       depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index 677c4d1e2632..3cb3df018cf8 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -30,6 +30,7 @@ obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> >  obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
> >  obj-$(CONFIG_RESET_RASPBERRYPI) += reset-raspberrypi.o
> >  obj-$(CONFIG_RESET_RZG2L_USBPHY_CTRL) += reset-rzg2l-usbphy-ctrl.o
> > +obj-$(CONFIG_RESET_RZV2H_USB2PHY_CTRL) += reset-rzv2h-usb2phy-ctrl.o
> >  obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
> >  obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> >  obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> > diff --git a/drivers/reset/reset-rzv2h-usb2phy-ctrl.c b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > new file mode 100644
> > index 000000000000..a6daeaf37e1c
> > --- /dev/null
> > +++ b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > @@ -0,0 +1,223 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/V2H(P) USB2PHY control driver
> > + *
> > + * Copyright (C) 2025 Renesas Electronics Corporation
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/reset.h>
> > +#include <linux/reset-controller.h>
> > +
> > +struct rzv2h_usb2phy_regval {
> > +     u16 reg;
> > +     u16 val;
> > +};
> > +
> > +struct rzv2h_usb2phy_data {
> > +     const struct rzv2h_usb2phy_regval *init_vals;
> > +     unsigned int init_val_count;
> > +
> > +     u16 ctrl_reg;
> > +     u16 ctrl_assert_val;
> > +     u16 ctrl_deassert_val;
> > +     u16 ctrl_status_bits;
> > +     u16 ctrl_release_val;
> > +
> > +     u16 ctrl2_reg;
> > +     u16 ctrl2_acquire_val;
> > +     u16 ctrl2_release_val;
> > +};
> > +
> > +struct rzv2h_usb2phy_ctrl_priv {
> > +     const struct rzv2h_usb2phy_data *data;
> > +     void __iomem *base;
> > +     struct device *dev;
> > +     struct reset_controller_dev rcdev;
> > +     spinlock_t lock;
>
> Lock without comment.
>
I will add a comment for it.

> > +};
> > +
> > +#define rcdev_to_priv(x) container_of(x, struct rzv2h_usb2phy_ctrl_priv, rcdev)
>
> I'd prefer this to be an inline function.
>
OK, I'll add a rzv2h_usbphy_rcdev_to_priv() inline function.

> > +
> > +static int rzv2h_usbphy_ctrl_assert(struct reset_controller_dev *rcdev,
> > +                                 unsigned long id)
> > +{
> > +     struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> > +     const struct rzv2h_usb2phy_data *data = priv->data;
> > +     struct device *dev = priv->dev;
> > +     int ret;
> > +
> > +     ret = pm_runtime_resume_and_get(dev);
> > +     if (ret) {
> > +             dev_err(dev, "pm_runtime_resume_and_get failed\n");
> > +             return ret;
> > +     }
> > +     scoped_guard(spinlock, &priv->lock) {
> > +             writel(data->ctrl2_acquire_val, priv->base + data->ctrl2_reg);
>
> Comparing to deassert, I wonder why is there no ctrl_acquire_val?
>
Based on the HW manual there isn't one.

> > +             writel(data->ctrl_assert_val, priv->base + data->ctrl_reg);
> > +     }
> > +
> > +     /* The reset line needs to be asserted for more than 10 microseconds. */
> > +     udelay(11);
>
> Could this be usleep_range() instead?
>
Mostly the consumers wouldn't call the assert operation in an atomic
context, so usleep_range() could be used here.

Cheers,
Prabhakar

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

* Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
  2025-03-13 10:14     ` Fabrizio Castro
@ 2025-03-13 13:06       ` Philipp Zabel
  2025-03-18 12:31         ` Fabrizio Castro
  0 siblings, 1 reply; 20+ messages in thread
From: Philipp Zabel @ 2025-03-13 13:06 UTC (permalink / raw)
  To: Fabrizio Castro, Prabhakar, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm
  Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, Biju Das,
	Prabhakar Mahadev Lad, Chris Paterson

Hi Fabrizio,

On Do, 2025-03-13 at 10:14 +0000, Fabrizio Castro wrote:
> Hi Philipp,
> 
> Thanks for your feedback!
> 
> > From: Philipp Zabel <p.zabel@pengutronix.de>
> > Sent: 13 March 2025 08:37
> > Subject: Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
> > 
> > On Mi, 2025-03-05 at 12:39 +0000, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > 
> > > Add support for the USB2PHY control driver on the Renesas RZ/V2H(P) SoC.
> > > Make the driver handle reset and power-down operations for the USB2PHY.
> > > 
> > > Pass OF data to support future SoCs with similar USB2PHY hardware but
> > > different register configurations. Define device-specific initialization
> > > values and control register settings in OF data to ensure flexibility
> > > for upcoming SoCs.
> > > 
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/reset/Kconfig                    |   7 +
> > >  drivers/reset/Makefile                   |   1 +
> > >  drivers/reset/reset-rzv2h-usb2phy-ctrl.c | 223 +++++++++++++++++++++++
> > >  3 files changed, 231 insertions(+)
> > >  create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > 
[...]
> > > diff --git a/drivers/reset/reset-rzv2h-usb2phy-ctrl.c b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > new file mode 100644
> > > index 000000000000..a6daeaf37e1c
> > > --- /dev/null
> > > +++ b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > @@ -0,0 +1,223 @@
[...]
> > > +static const struct rzv2h_usb2phy_regval rzv2h_init_vals[] = {
> > > +	{ .reg = 0xc10, .val = 0x67c },
> > > +	{ .reg = 0xc14, .val = 0x1f },
> > > +	{ .reg = 0x600, .val = 0x909 },
> > 
> > What are these registers and what are those values doing?
> 
> Unfortunately, there are some licensing restrictions on this IP, this is
> the best that we can do, as per the license agreement.

How am I expected to review this?

For now, I'll assume that these registers are not related to reset
functionality at all, and that this driver should be a phy controller
driver instead of a reset controller driver.

Can you convince me otherwise without breaking license agreements?

regards
Philipp

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

* Re: [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  2025-03-06 16:26       ` Conor Dooley
@ 2025-03-13 13:09         ` Philipp Zabel
  2025-03-13 13:17           ` Lad, Prabhakar
  0 siblings, 1 reply; 20+ messages in thread
From: Philipp Zabel @ 2025-03-13 13:09 UTC (permalink / raw)
  To: Conor Dooley, Lad, Prabhakar
  Cc: Geert Uytterhoeven, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Magnus Damm, devicetree, linux-kernel,
	linux-renesas-soc, Biju Das, Fabrizio Castro, Lad Prabhakar

On Do, 2025-03-06 at 16:26 +0000, Conor Dooley wrote:
[...]
> That sounds awfully like "it was wrong before, and I want to keep using
> the wrong node name"... If you're claiming to be some other class of
> device, "ctrl" should really be "controller" like all the other sorts of
> controllers ;)

There are "usb-phy-controller" nodes on the rcar-gen2 SoCs.

regards
Philipp

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

* Re: [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  2025-03-13 13:09         ` Philipp Zabel
@ 2025-03-13 13:17           ` Lad, Prabhakar
  2025-03-27 11:06             ` Lad, Prabhakar
  0 siblings, 1 reply; 20+ messages in thread
From: Lad, Prabhakar @ 2025-03-13 13:17 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: Conor Dooley, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm, devicetree,
	linux-kernel, linux-renesas-soc, Biju Das, Fabrizio Castro,
	Lad Prabhakar

Hi Philipp,

On Thu, Mar 13, 2025 at 1:09 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Do, 2025-03-06 at 16:26 +0000, Conor Dooley wrote:
> [...]
> > That sounds awfully like "it was wrong before, and I want to keep using
> > the wrong node name"... If you're claiming to be some other class of
> > device, "ctrl" should really be "controller" like all the other sorts of
> > controllers ;)
>
> There are "usb-phy-controller" nodes on the rcar-gen2 SoCs.
>
Ok, I will rename the node name to "usb-phy-controller".

Cheers
Prabhakar

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

* RE: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
  2025-03-13 13:06       ` Philipp Zabel
@ 2025-03-18 12:31         ` Fabrizio Castro
  2025-03-25 15:13           ` Philipp Zabel
  0 siblings, 1 reply; 20+ messages in thread
From: Fabrizio Castro @ 2025-03-18 12:31 UTC (permalink / raw)
  To: Philipp Zabel, Prabhakar, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm
  Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, Biju Das,
	Prabhakar Mahadev Lad, Chris Paterson

Hi Philipp,

Thanks for your feedback!

> From: Philipp Zabel <p.zabel@pengutronix.de>
> Sent: 13 March 2025 13:06
> Subject: Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
> 
> Hi Fabrizio,
> 
> On Do, 2025-03-13 at 10:14 +0000, Fabrizio Castro wrote:
> > Hi Philipp,
> >
> > Thanks for your feedback!
> >
> > > From: Philipp Zabel <p.zabel@pengutronix.de>
> > > Sent: 13 March 2025 08:37
> > > Subject: Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
> > >
> > > On Mi, 2025-03-05 at 12:39 +0000, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Add support for the USB2PHY control driver on the Renesas RZ/V2H(P) SoC.
> > > > Make the driver handle reset and power-down operations for the USB2PHY.
> > > >
> > > > Pass OF data to support future SoCs with similar USB2PHY hardware but
> > > > different register configurations. Define device-specific initialization
> > > > values and control register settings in OF data to ensure flexibility
> > > > for upcoming SoCs.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  drivers/reset/Kconfig                    |   7 +
> > > >  drivers/reset/Makefile                   |   1 +
> > > >  drivers/reset/reset-rzv2h-usb2phy-ctrl.c | 223 +++++++++++++++++++++++
> > > >  3 files changed, 231 insertions(+)
> > > >  create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > >
> [...]
> > > > diff --git a/drivers/reset/reset-rzv2h-usb2phy-ctrl.c b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > > new file mode 100644
> > > > index 000000000000..a6daeaf37e1c
> > > > --- /dev/null
> > > > +++ b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > > @@ -0,0 +1,223 @@
> [...]
> > > > +static const struct rzv2h_usb2phy_regval rzv2h_init_vals[] = {
> > > > +	{ .reg = 0xc10, .val = 0x67c },
> > > > +	{ .reg = 0xc14, .val = 0x1f },
> > > > +	{ .reg = 0x600, .val = 0x909 },
> > >
> > > What are these registers and what are those values doing?
> >
> > Unfortunately, there are some licensing restrictions on this IP, this is
> > the best that we can do, as per the license agreement.
> 
> How am I expected to review this?
> 
> For now, I'll assume that these registers are not related to reset
> functionality at all, and that this driver should be a phy controller
> driver instead of a reset controller driver.
> 
> Can you convince me otherwise without breaking license agreements?

Sorry about the delay, as you may have figured out, we had to double check with
the LSI team before making any statement.

We can confirm that `rzv2h_init_vals` contains the registers and corresponding
initialization values required to prepare the PHY to receive assert and deassert
requests. This is a one time only thing, done at probe.

After looking into things again, I have noticed that the probe function is missing
calling into the assert sequence, and the status of the reset is undefined, so
that's something to fix for v3 to make it initialize in asserted state.

The assert, deassert, and status operations are only touching reset related registers.
Nothing else.

Therefore we believe this should be a port reset driver.

Thanks for your patience so far, and sorry for being cryptic.

Cheers,
Fab

> 
> regards
> Philipp

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

* Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
  2025-03-18 12:31         ` Fabrizio Castro
@ 2025-03-25 15:13           ` Philipp Zabel
  2025-03-25 15:43             ` Fabrizio Castro
  0 siblings, 1 reply; 20+ messages in thread
From: Philipp Zabel @ 2025-03-25 15:13 UTC (permalink / raw)
  To: Fabrizio Castro, Prabhakar, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm
  Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, Biju Das,
	Prabhakar Mahadev Lad, Chris Paterson

Hi Fabrizio, Prabhakar,

On Di, 2025-03-18 at 12:31 +0000, Fabrizio Castro wrote:
> Hi Philipp,
> 
> Thanks for your feedback!
> 
> > From: Philipp Zabel <p.zabel@pengutronix.de>
> > Sent: 13 March 2025 13:06
> > Subject: Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
> > 
> > Hi Fabrizio,
> > 
> > On Do, 2025-03-13 at 10:14 +0000, Fabrizio Castro wrote:
> > > Hi Philipp,
> > > 
> > > Thanks for your feedback!
> > > 
> > > > From: Philipp Zabel <p.zabel@pengutronix.de>
> > > > Sent: 13 March 2025 08:37
> > > > Subject: Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
> > > > 
> > > > On Mi, 2025-03-05 at 12:39 +0000, Prabhakar wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > 
> > > > > Add support for the USB2PHY control driver on the Renesas RZ/V2H(P) SoC.
> > > > > Make the driver handle reset and power-down operations for the USB2PHY.
> > > > > 
> > > > > Pass OF data to support future SoCs with similar USB2PHY hardware but
> > > > > different register configurations. Define device-specific initialization
> > > > > values and control register settings in OF data to ensure flexibility
> > > > > for upcoming SoCs.
> > > > > 
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > >  drivers/reset/Kconfig                    |   7 +
> > > > >  drivers/reset/Makefile                   |   1 +
> > > > >  drivers/reset/reset-rzv2h-usb2phy-ctrl.c | 223 +++++++++++++++++++++++
> > > > >  3 files changed, 231 insertions(+)
> > > > >  create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > > > 
> > [...]
> > > > > diff --git a/drivers/reset/reset-rzv2h-usb2phy-ctrl.c b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > > > new file mode 100644
> > > > > index 000000000000..a6daeaf37e1c
> > > > > --- /dev/null
> > > > > +++ b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > > > @@ -0,0 +1,223 @@
> > [...]
> > > > > +static const struct rzv2h_usb2phy_regval rzv2h_init_vals[] = {
> > > > > +	{ .reg = 0xc10, .val = 0x67c },
> > > > > +	{ .reg = 0xc14, .val = 0x1f },
> > > > > +	{ .reg = 0x600, .val = 0x909 },
> > > > 
> > > > What are these registers and what are those values doing?
> > > 
> > > Unfortunately, there are some licensing restrictions on this IP, this is
> > > the best that we can do, as per the license agreement.
> > 
> > How am I expected to review this?
> > 
> > For now, I'll assume that these registers are not related to reset
> > functionality at all, and that this driver should be a phy controller
> > driver instead of a reset controller driver.
> > 
> > Can you convince me otherwise without breaking license agreements?
> 
> Sorry about the delay, as you may have figured out, we had to double check with
> the LSI team before making any statement.
> 
> We can confirm that `rzv2h_init_vals` contains the registers and corresponding
> initialization values required to prepare the PHY to receive assert and deassert
> requests. This is a one time only thing, done at probe.

Thank you. Please document this in a comment next to the
rzv2h_init_vals[] table.

> After looking into things again, I have noticed that the probe function is missing
> calling into the assert sequence, and the status of the reset is undefined, so
> that's something to fix for v3 to make it initialize in asserted state.
> 
> The assert, deassert, and status operations are only touching reset related registers.
> Nothing else.
>
> Therefore we believe this should be a port reset driver.
>
> Thanks for your patience so far, and sorry for being cryptic.

Let's go ahead with this driver. I'd be happy about a MAINTAINERS entry
for it.

regards
Philipp

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

* RE: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
  2025-03-25 15:13           ` Philipp Zabel
@ 2025-03-25 15:43             ` Fabrizio Castro
  0 siblings, 0 replies; 20+ messages in thread
From: Fabrizio Castro @ 2025-03-25 15:43 UTC (permalink / raw)
  To: Philipp Zabel, Prabhakar, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm
  Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, Biju Das,
	Prabhakar Mahadev Lad, Chris Paterson

Hi Philipp,

Thanks for your feedback!

> From: Philipp Zabel <p.zabel@pengutronix.de>
> Sent: 25 March 2025 15:13
> Subject: Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
> 
> Hi Fabrizio, Prabhakar,
> 
> On Di, 2025-03-18 at 12:31 +0000, Fabrizio Castro wrote:
> > Hi Philipp,
> >
> > Thanks for your feedback!
> >
> > > From: Philipp Zabel <p.zabel@pengutronix.de>
> > > Sent: 13 March 2025 13:06
> > > Subject: Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
> > >
> > > Hi Fabrizio,
> > >
> > > On Do, 2025-03-13 at 10:14 +0000, Fabrizio Castro wrote:
> > > > Hi Philipp,
> > > >
> > > > Thanks for your feedback!
> > > >
> > > > > From: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > Sent: 13 March 2025 08:37
> > > > > Subject: Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
> > > > >
> > > > > On Mi, 2025-03-05 at 12:39 +0000, Prabhakar wrote:
> > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > >
> > > > > > Add support for the USB2PHY control driver on the Renesas RZ/V2H(P) SoC.
> > > > > > Make the driver handle reset and power-down operations for the USB2PHY.
> > > > > >
> > > > > > Pass OF data to support future SoCs with similar USB2PHY hardware but
> > > > > > different register configurations. Define device-specific initialization
> > > > > > values and control register settings in OF data to ensure flexibility
> > > > > > for upcoming SoCs.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > ---
> > > > > >  drivers/reset/Kconfig                    |   7 +
> > > > > >  drivers/reset/Makefile                   |   1 +
> > > > > >  drivers/reset/reset-rzv2h-usb2phy-ctrl.c | 223 +++++++++++++++++++++++
> > > > > >  3 files changed, 231 insertions(+)
> > > > > >  create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > > > >
> > > [...]
> > > > > > diff --git a/drivers/reset/reset-rzv2h-usb2phy-ctrl.c b/drivers/reset/reset-rzv2h-usb2phy-
> ctrl.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..a6daeaf37e1c
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > > > > > @@ -0,0 +1,223 @@
> > > [...]
> > > > > > +static const struct rzv2h_usb2phy_regval rzv2h_init_vals[] = {
> > > > > > +	{ .reg = 0xc10, .val = 0x67c },
> > > > > > +	{ .reg = 0xc14, .val = 0x1f },
> > > > > > +	{ .reg = 0x600, .val = 0x909 },
> > > > >
> > > > > What are these registers and what are those values doing?
> > > >
> > > > Unfortunately, there are some licensing restrictions on this IP, this is
> > > > the best that we can do, as per the license agreement.
> > >
> > > How am I expected to review this?
> > >
> > > For now, I'll assume that these registers are not related to reset
> > > functionality at all, and that this driver should be a phy controller
> > > driver instead of a reset controller driver.
> > >
> > > Can you convince me otherwise without breaking license agreements?
> >
> > Sorry about the delay, as you may have figured out, we had to double check with
> > the LSI team before making any statement.
> >
> > We can confirm that `rzv2h_init_vals` contains the registers and corresponding
> > initialization values required to prepare the PHY to receive assert and deassert
> > requests. This is a one time only thing, done at probe.
> 
> Thank you. Please document this in a comment next to the
> rzv2h_init_vals[] table.

Will do, thank you.

> 
> > After looking into things again, I have noticed that the probe function is missing
> > calling into the assert sequence, and the status of the reset is undefined, so
> > that's something to fix for v3 to make it initialize in asserted state.
> >
> > The assert, deassert, and status operations are only touching reset related registers.
> > Nothing else.
> >
> > Therefore we believe this should be a port reset driver.
> >
> > Thanks for your patience so far, and sorry for being cryptic.
> 
> Let's go ahead with this driver. I'd be happy about a MAINTAINERS entry
> for it.

Thank you! And we'll definitely add a MAINTAINERS entry for this.

Cheers,
Fab

> 
> regards
> Philipp

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

* Re: [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  2025-03-13 13:17           ` Lad, Prabhakar
@ 2025-03-27 11:06             ` Lad, Prabhakar
  2025-03-27 16:40               ` Conor Dooley
  0 siblings, 1 reply; 20+ messages in thread
From: Lad, Prabhakar @ 2025-03-27 11:06 UTC (permalink / raw)
  To: Philipp Zabel, Conor Dooley
  Cc: Geert Uytterhoeven, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Magnus Damm, devicetree, linux-kernel,
	linux-renesas-soc, Biju Das, Fabrizio Castro, Lad Prabhakar

Hi Philipp and Conor

On Thu, Mar 13, 2025 at 1:17 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> Hi Philipp,
>
> On Thu, Mar 13, 2025 at 1:09 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >
> > On Do, 2025-03-06 at 16:26 +0000, Conor Dooley wrote:
> > [...]
> > > That sounds awfully like "it was wrong before, and I want to keep using
> > > the wrong node name"... If you're claiming to be some other class of
> > > device, "ctrl" should really be "controller" like all the other sorts of
> > > controllers ;)
> >
> > There are "usb-phy-controller" nodes on the rcar-gen2 SoCs.
> >
> Ok, I will rename the node name to "usb-phy-controller".
>
Fyi to chime in with other reset drivers I'll rename this binding file
to `renesas,rzv2h-usb2phy-reset.yaml` and have the node named
`usb2phy-reset@15830000` in the example node.

Cheers
Prabhakar

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

* Re: [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  2025-03-27 11:06             ` Lad, Prabhakar
@ 2025-03-27 16:40               ` Conor Dooley
  2025-03-27 17:24                 ` Lad, Prabhakar
  0 siblings, 1 reply; 20+ messages in thread
From: Conor Dooley @ 2025-03-27 16:40 UTC (permalink / raw)
  To: Lad, Prabhakar
  Cc: Philipp Zabel, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm, devicetree,
	linux-kernel, linux-renesas-soc, Biju Das, Fabrizio Castro,
	Lad Prabhakar

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

On Thu, Mar 27, 2025 at 11:06:33AM +0000, Lad, Prabhakar wrote:
> Hi Philipp and Conor
> 
> On Thu, Mar 13, 2025 at 1:17 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> >
> > Hi Philipp,
> >
> > On Thu, Mar 13, 2025 at 1:09 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > >
> > > On Do, 2025-03-06 at 16:26 +0000, Conor Dooley wrote:
> > > [...]
> > > > That sounds awfully like "it was wrong before, and I want to keep using
> > > > the wrong node name"... If you're claiming to be some other class of
> > > > device, "ctrl" should really be "controller" like all the other sorts of
> > > > controllers ;)
> > >
> > > There are "usb-phy-controller" nodes on the rcar-gen2 SoCs.
> > >
> > Ok, I will rename the node name to "usb-phy-controller".
> >
> Fyi to chime in with other reset drivers I'll rename this binding file
> to `renesas,rzv2h-usb2phy-reset.yaml` and have the node named

> `usb2phy-reset@15830000` in the example node.

At that point, isn't it then "just" a reset controller with only a
single device that it resets, so "reset-controller" is the right class
of device to label it as?

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

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

* Re: [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control
  2025-03-27 16:40               ` Conor Dooley
@ 2025-03-27 17:24                 ` Lad, Prabhakar
  0 siblings, 0 replies; 20+ messages in thread
From: Lad, Prabhakar @ 2025-03-27 17:24 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Philipp Zabel, Geert Uytterhoeven, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Magnus Damm, devicetree,
	linux-kernel, linux-renesas-soc, Biju Das, Fabrizio Castro,
	Lad Prabhakar

Hi Conor,

On Thu, Mar 27, 2025 at 4:40 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Mar 27, 2025 at 11:06:33AM +0000, Lad, Prabhakar wrote:
> > Hi Philipp and Conor
> >
> > On Thu, Mar 13, 2025 at 1:17 PM Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> > >
> > > Hi Philipp,
> > >
> > > On Thu, Mar 13, 2025 at 1:09 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > >
> > > > On Do, 2025-03-06 at 16:26 +0000, Conor Dooley wrote:
> > > > [...]
> > > > > That sounds awfully like "it was wrong before, and I want to keep using
> > > > > the wrong node name"... If you're claiming to be some other class of
> > > > > device, "ctrl" should really be "controller" like all the other sorts of
> > > > > controllers ;)
> > > >
> > > > There are "usb-phy-controller" nodes on the rcar-gen2 SoCs.
> > > >
> > > Ok, I will rename the node name to "usb-phy-controller".
> > >
> > Fyi to chime in with other reset drivers I'll rename this binding file
> > to `renesas,rzv2h-usb2phy-reset.yaml` and have the node named
>
> > `usb2phy-reset@15830000` in the example node.
>
> At that point, isn't it then "just" a reset controller with only a
> single device that it resets, so "reset-controller" is the right class
> of device to label it as?
I agree, I will label it as a "reset-controller".

Cheers,
Prabhakar

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

end of thread, other threads:[~2025-03-27 17:24 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-05 12:39 [PATCH v2 0/2] Add USB2PHY control support for Renesas RZ/V2H(P) SoC Prabhakar
2025-03-05 12:39 ` [PATCH v2 1/2] dt-bindings: reset: Document RZ/V2H(P) USB2PHY Control Prabhakar
2025-03-05 16:26   ` Conor Dooley
2025-03-05 21:35     ` Lad, Prabhakar
2025-03-06 16:26       ` Conor Dooley
2025-03-13 13:09         ` Philipp Zabel
2025-03-13 13:17           ` Lad, Prabhakar
2025-03-27 11:06             ` Lad, Prabhakar
2025-03-27 16:40               ` Conor Dooley
2025-03-27 17:24                 ` Lad, Prabhakar
2025-03-06 11:15   ` Fabrizio Castro
2025-03-05 12:39 ` [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P) Prabhakar
2025-03-06 11:17   ` Fabrizio Castro
2025-03-13  8:37   ` Philipp Zabel
2025-03-13 10:14     ` Fabrizio Castro
2025-03-13 13:06       ` Philipp Zabel
2025-03-18 12:31         ` Fabrizio Castro
2025-03-25 15:13           ` Philipp Zabel
2025-03-25 15:43             ` Fabrizio Castro
2025-03-13 11:23     ` Lad, Prabhakar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).