[PATCH v3 00/10] usb: dwc3: Refactor dwc3-generic and apply to dwc3-uniphier

[PATCH v3 00/10] usb: dwc3: Refactor dwc3-generic and apply to dwc3-uniphier

[Kunihiko Hayashi]

This series achieves refactoring of dwc3-generic.

First, dwc3-generic allows DT controller nodes to be children of glue nodes,
but outside of glue nodes.

To achieve this goal, define a glue-specific function to get controller node,
look up more reference clocks in the controller node, and initialize clocks
in children of glue node before access to the controller,

Next, this series exports the structures and functions from the driver source
to the header, and replaces dwc3-uniphier driver as one implementation using
them. This expects dwc3-generic to prevent more SoC-dependent codes.

The dwc3-uniphier has original USB node, however, tentatively added its own
node dedicated to U-Boot. After this refactoring, the driver needs to add
clock entries and PHY driver to enable them corresponding to the properties
in the original node.

PATCH 1 has been provided below.
  https://patchwork.ozlabs.org/project/uboot/patch/20221215223822.137739-1-marex@denx.de/

PATCH 2 is based on the suggested patch from Marek.

PATCH 4-5 and 9-10 have been already reviewed in the previous v1.
  https://lists.denx.de/pipermail/u-boot/2023-January/505689.html

Changes since v2:
- Add Reviewed-by: tags
- Add more details to reset-uniphier patch log
- Add error release for clock/reset initializer in phy-uniphier-usb3 patch
- Fix release year in in phy-uniphier-usb3 patch
- Enable PHY_UNIPHIER_USB3 with DWC3_UNIPHIER
- Separate devicetree patches

Changes since v1:
- Add a patch to refer to the clock in top level generic node
- Add a patch to allow controller DT node outside of glue node
- Add a patch to initialize clocks in children of glue node
- Update dwc3_glue_ops to add the function to get controller device
- Add a patch to support USB glue reset
- Add a patch to add missing PHY clocks
- Add a patch to enable the driver to enable PHY clocks and resets
- Add a patch to fix node names and missing properties
- Add a patch to switch to the original USB node in DT

Kunihiko Hayashi (9):
  usb: dwc3-generic: Allow different controller DT node pattern
  usb: dwc3-generic: Add clock initialization in child DT node
  usb: dwc3-generic: Export glue structures and functions
  usb: dwc3-generic: Add the size of regs property to glue structure
  reset: uniphier: Add USB glue reset support
  clk: uniphier: Add missing USB SS-PHY clocks
  phy: socionext: Add UniPhier USB3 PHY driver
  usb: dwc3-uniphier: Use dwc3-generic instead of xhci-dwc3
  uniphier_defconfig: Disable USB_XHCI_DWC3

Marek Vasut (1):
  usb: dwc3: Look up reference clock DT phandle in both controller DT
    nodes

 configs/uniphier_v7_defconfig             |   1 -
 configs/uniphier_v8_defconfig             |   1 -
 drivers/clk/uniphier/clk-uniphier-sys.c   |   5 +
 drivers/phy/socionext/Kconfig             |   8 ++
 drivers/phy/socionext/Makefile            |   1 +
 drivers/phy/socionext/phy-uniphier-usb3.c |  93 +++++++++++++++
 drivers/reset/reset-uniphier.c            |  78 ++++++++++++-
 drivers/usb/dwc3/Kconfig                  |   4 +-
 drivers/usb/dwc3/dwc3-generic.c           | 132 ++++++++++++++--------
 drivers/usb/dwc3/dwc3-generic.h           |  33 ++++++
 drivers/usb/dwc3/dwc3-uniphier.c          | 116 ++++++++++++-------
 11 files changed, 378 insertions(+), 94 deletions(-)
 create mode 100644 drivers/phy/socionext/phy-uniphier-usb3.c
 create mode 100644 drivers/usb/dwc3/dwc3-generic.h

-- 
2.25.1

[PATCH v3 01/10] usb: dwc3: Look up reference clock DT phandle in both controller DT nodes

[Kunihiko Hayashi]

From: Marek Vasut <marex@denx.de>

There are currently four disparate placement possibilities of DWC3
reference clock phandle in SoC DTs:
- in top level glue node, with generic subnode without clock (ZynqMP)
- in top level generic node, with no subnode (i.MX8MQ)
- in generic subnode, with other clock in top level node (i.MX8MP)
- in both top level node and generic subnode (Rockchip)

Cover all the possibilities here by looking into both nodes, start
with the top level node as that seems to be used in majority of DTs
to reference the clock.

Signed-off-by: Marek Vasut <marex@denx.de>
Acked-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/usb/dwc3/dwc3-generic.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
index 78966718d01d..42e92478f25a 100644
--- a/drivers/usb/dwc3/dwc3-generic.c
+++ b/drivers/usb/dwc3/dwc3-generic.c
@@ -68,10 +68,27 @@ static int dwc3_generic_probe(struct udevice *dev,
 #if CONFIG_IS_ENABLED(OF_CONTROL)
 	dwc3_of_parse(dwc3);
 
+	/*
+	 * There are currently four disparate placement possibilities of DWC3
+	 * reference clock phandle in SoC DTs:
+	 * - in top level glue node, with generic subnode without clock (ZynqMP)
+	 * - in top level generic node, with no subnode (i.MX8MQ)
+	 * - in generic subnode, with other clock in top level node (i.MX8MP)
+	 * - in both top level node and generic subnode (Rockchip)
+	 * Cover all the possibilities here by looking into both nodes, start
+	 * with the top level node as that seems to be used in majority of DTs
+	 * to reference the clock.
+	 */
 	node = dev_ofnode(dev->parent);
 	index = ofnode_stringlist_search(node, "clock-names", "ref");
 	if (index < 0)
 		index = ofnode_stringlist_search(node, "clock-names", "ref_clk");
+	if (index < 0) {
+		node = dev_ofnode(dev);
+		index = ofnode_stringlist_search(node, "clock-names", "ref");
+		if (index < 0)
+			index = ofnode_stringlist_search(node, "clock-names", "ref_clk");
+	}
 	if (index >= 0)
 		dwc3->ref_clk = &glue->clks.clks[index];
 #endif
-- 
2.25.1

[PATCH v3 02/10] usb: dwc3-generic: Allow different controller DT node pattern

[Kunihiko Hayashi]

The most of devicetree has the following USB node structure.
The controller node is placed as a child node of the glue node.
Current dwc3-generic driver works on this premise.

    glue {
        /* glue node */
	usb {
            /* controller node */
	};
    };

However, UniPhier original devicetree has the following USB node structure.
The controller node is separately placed from the glue node.

    usb {
        /* controller node */
    };
    glue {
        /* glue node */
    };

In dwc_glue_bind(), this patch provides .glue_get_ctrl_dev() callback to
get such a controller node and binds the driver related to the node.

If this callback isn't defined, dwc_glue_bind() looks for the controller
nodes from the child nodes, as before.

Suggested-by: Marek Vasut <marex@denx.de>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 drivers/usb/dwc3/dwc3-generic.c | 93 ++++++++++++++++++++-------------
 1 file changed, 57 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
index 42e92478f25a..e32003d68e01 100644
--- a/drivers/usb/dwc3/dwc3-generic.c
+++ b/drivers/usb/dwc3/dwc3-generic.c
@@ -276,6 +276,7 @@ U_BOOT_DRIVER(dwc3_generic_host) = {
 #endif
 
 struct dwc3_glue_ops {
+	int (*glue_get_ctrl_dev)(struct udevice *parent, ofnode *node);
 	void (*glue_configure)(struct udevice *dev, int index,
 			       enum usb_dr_mode mode);
 };
@@ -415,54 +416,74 @@ struct dwc3_glue_ops ti_ops = {
 	.glue_configure = dwc3_ti_glue_configure,
 };
 
-static int dwc3_glue_bind(struct udevice *parent)
+static int dwc3_glue_bind_common(struct udevice *parent, ofnode node)
 {
-	ofnode node;
-	int ret;
+	const char *name = ofnode_get_name(node);
+	const char *driver = NULL;
 	enum usb_dr_mode dr_mode;
+	struct udevice *dev;
+	int ret;
 
-	dr_mode = usb_get_dr_mode(dev_ofnode(parent));
-
-	ofnode_for_each_subnode(node, dev_ofnode(parent)) {
-		const char *name = ofnode_get_name(node);
-		struct udevice *dev;
-		const char *driver = NULL;
-
-		debug("%s: subnode name: %s\n", __func__, name);
+	debug("%s: subnode name: %s\n", __func__, name);
 
-		/* if the parent node doesn't have a mode check the leaf */
-		if (!dr_mode)
-			dr_mode = usb_get_dr_mode(node);
+	/* if the parent node doesn't have a mode check the leaf */
+	dr_mode = usb_get_dr_mode(dev_ofnode(parent));
+	if (!dr_mode)
+		dr_mode = usb_get_dr_mode(node);
 
-		switch (dr_mode) {
-		case USB_DR_MODE_PERIPHERAL:
-		case USB_DR_MODE_OTG:
+	switch (dr_mode) {
+	case USB_DR_MODE_PERIPHERAL:
+	case USB_DR_MODE_OTG:
 #if CONFIG_IS_ENABLED(DM_USB_GADGET)
-			debug("%s: dr_mode: OTG or Peripheral\n", __func__);
-			driver = "dwc3-generic-peripheral";
+		debug("%s: dr_mode: OTG or Peripheral\n", __func__);
+		driver = "dwc3-generic-peripheral";
 #endif
-			break;
+		break;
 #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
-		case USB_DR_MODE_HOST:
-			debug("%s: dr_mode: HOST\n", __func__);
-			driver = "dwc3-generic-host";
-			break;
+	case USB_DR_MODE_HOST:
+		debug("%s: dr_mode: HOST\n", __func__);
+		driver = "dwc3-generic-host";
+		break;
 #endif
-		default:
-			debug("%s: unsupported dr_mode\n", __func__);
-			return -ENODEV;
-		};
+	default:
+		debug("%s: unsupported dr_mode\n", __func__);
+		return -ENODEV;
+	};
 
-		if (!driver)
-			continue;
+	if (!driver)
+		return -ENXIO;
+
+	ret = device_bind_driver_to_node(parent, driver, name,
+					 node, &dev);
+	if (ret) {
+		debug("%s: not able to bind usb device mode\n",
+		      __func__);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int dwc3_glue_bind(struct udevice *parent)
+{
+	struct dwc3_glue_ops *ops = (struct dwc3_glue_ops *)dev_get_driver_data(parent);
+	ofnode node;
+	int ret;
 
-		ret = device_bind_driver_to_node(parent, driver, name,
-						 node, &dev);
-		if (ret) {
-			debug("%s: not able to bind usb device mode\n",
-			      __func__);
+	if (ops && ops->glue_get_ctrl_dev) {
+		ret = ops->glue_get_ctrl_dev(parent, &node);
+		if (ret)
+			return ret;
+
+		return dwc3_glue_bind_common(parent, node);
+	}
+
+	ofnode_for_each_subnode(node, dev_ofnode(parent)) {
+		ret = dwc3_glue_bind_common(parent, node);
+		if (ret == -ENXIO)
+			continue;
+		if (ret)
 			return ret;
-		}
 	}
 
 	return 0;
-- 
2.25.1

[PATCH v3 03/10] usb: dwc3-generic: Add clock initialization in child DT node

[Kunihiko Hayashi]

Same as the reset cotnrol, should add a clock initialization in child DT
node, if the glue node doesn't have any clocks.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 drivers/usb/dwc3/dwc3-generic.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
index e32003d68e01..8fa56e1ac173 100644
--- a/drivers/usb/dwc3/dwc3-generic.c
+++ b/drivers/usb/dwc3/dwc3-generic.c
@@ -572,6 +572,12 @@ static int dwc3_glue_probe(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	if (glue->clks.count == 0) {
+		ret = dwc3_glue_clk_init(child, glue);
+		if (ret)
+			return ret;
+	}
+
 	if (glue->resets.count == 0) {
 		ret = dwc3_glue_reset_init(child, glue);
 		if (ret)
-- 
2.25.1

[PATCH v3 04/10] usb: dwc3-generic: Export glue structures and functions

[Kunihiko Hayashi]

In order to allow external SoC-dependent glue drivers to use dwc3-generic
functions, push the glue structures and export the functions to a header
file.

The exported structures and functions are:

- struct dwc3_glue_data
- struct dwc3_glue_ops
- dwc3_glue_bind()
- dwc3_glue_probe()
- dwc3_glue_remove()

The SoC-dependent glue drivers can only define their own wrapper driver
and specify these functions. The drivers can also add their own compatible
strings and configure functions.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 drivers/usb/dwc3/dwc3-generic.c | 18 ++++--------------
 drivers/usb/dwc3/dwc3-generic.h | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 14 deletions(-)
 create mode 100644 drivers/usb/dwc3/dwc3-generic.h

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
index 8fa56e1ac173..4576390ec7cd 100644
--- a/drivers/usb/dwc3/dwc3-generic.c
+++ b/drivers/usb/dwc3/dwc3-generic.c
@@ -28,11 +28,7 @@
 #include <usb/xhci.h>
 #include <asm/gpio.h>
 
-struct dwc3_glue_data {
-	struct clk_bulk		clks;
-	struct reset_ctl_bulk	resets;
-	fdt_addr_t regs;
-};
+#include "dwc3-generic.h"
 
 struct dwc3_generic_plat {
 	fdt_addr_t base;
@@ -275,12 +271,6 @@ U_BOOT_DRIVER(dwc3_generic_host) = {
 };
 #endif
 
-struct dwc3_glue_ops {
-	int (*glue_get_ctrl_dev)(struct udevice *parent, ofnode *node);
-	void (*glue_configure)(struct udevice *dev, int index,
-			       enum usb_dr_mode mode);
-};
-
 void dwc3_imx8mp_glue_configure(struct udevice *dev, int index,
 				enum usb_dr_mode mode)
 {
@@ -464,7 +454,7 @@ static int dwc3_glue_bind_common(struct udevice *parent, ofnode node)
 	return 0;
 }
 
-static int dwc3_glue_bind(struct udevice *parent)
+int dwc3_glue_bind(struct udevice *parent)
 {
 	struct dwc3_glue_ops *ops = (struct dwc3_glue_ops *)dev_get_driver_data(parent);
 	ofnode node;
@@ -531,7 +521,7 @@ static int dwc3_glue_clk_init(struct udevice *dev,
 	return 0;
 }
 
-static int dwc3_glue_probe(struct udevice *dev)
+int dwc3_glue_probe(struct udevice *dev)
 {
 	struct dwc3_glue_ops *ops = (struct dwc3_glue_ops *)dev_get_driver_data(dev);
 	struct dwc3_glue_data *glue = dev_get_plat(dev);
@@ -597,7 +587,7 @@ static int dwc3_glue_probe(struct udevice *dev)
 	return 0;
 }
 
-static int dwc3_glue_remove(struct udevice *dev)
+int dwc3_glue_remove(struct udevice *dev)
 {
 	struct dwc3_glue_data *glue = dev_get_plat(dev);
 
diff --git a/drivers/usb/dwc3/dwc3-generic.h b/drivers/usb/dwc3/dwc3-generic.h
new file mode 100644
index 000000000000..824f678841a8
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-generic.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * dwc3-generic.h - Generic DWC3 Glue layer header
+ *
+ * Copyright (C) 2016 - 2018 Xilinx, Inc.
+ * Copyright (C) 2023 Socionext Inc.
+ */
+
+#ifndef __DRIVERS_USB_DWC3_GENERIC_H
+#define __DRIVERS_USB_DWC3_GENERIC_H
+
+#include <clk.h>
+#include <reset.h>
+#include <dwc3-uboot.h>
+
+struct dwc3_glue_data {
+	struct clk_bulk		clks;
+	struct reset_ctl_bulk	resets;
+	fdt_addr_t regs;
+};
+
+struct dwc3_glue_ops {
+	int (*glue_get_ctrl_dev)(struct udevice *parent, ofnode *node);
+	void (*glue_configure)(struct udevice *dev, int index,
+			       enum usb_dr_mode mode);
+};
+
+int dwc3_glue_bind(struct udevice *parent);
+int dwc3_glue_probe(struct udevice *dev);
+int dwc3_glue_remove(struct udevice *dev);
+
+#endif
-- 
2.25.1

[PATCH v3 05/10] usb: dwc3-generic: Add the size of regs property to glue structure

[Kunihiko Hayashi]

Add the size of regs property to the glue structure to correctly
specify the register region to map.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 drivers/usb/dwc3/dwc3-generic.c | 2 +-
 drivers/usb/dwc3/dwc3-generic.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
index 4576390ec7cd..acbf7acb1918 100644
--- a/drivers/usb/dwc3/dwc3-generic.c
+++ b/drivers/usb/dwc3/dwc3-generic.c
@@ -542,7 +542,7 @@ int dwc3_glue_probe(struct udevice *dev)
 		phy.dev = NULL;
 	}
 
-	glue->regs = dev_read_addr(dev);
+	glue->regs = dev_read_addr_size_index(dev, 0, &glue->size);
 
 	ret = dwc3_glue_clk_init(dev, glue);
 	if (ret)
diff --git a/drivers/usb/dwc3/dwc3-generic.h b/drivers/usb/dwc3/dwc3-generic.h
index 824f678841a8..40902c8923f5 100644
--- a/drivers/usb/dwc3/dwc3-generic.h
+++ b/drivers/usb/dwc3/dwc3-generic.h
@@ -17,6 +17,7 @@ struct dwc3_glue_data {
 	struct clk_bulk		clks;
 	struct reset_ctl_bulk	resets;
 	fdt_addr_t regs;
+	fdt_size_t size;
 };
 
 struct dwc3_glue_ops {
-- 
2.25.1

[PATCH v3 06/10] reset: uniphier: Add USB glue reset support

[Kunihiko Hayashi]

Add reset control support in USB glue logic. This needs to control
the external clocks and resets for the logic before accessing the
glue logic.

The USB dm tree when using dwc3-generic is the following:

   USB glue
     +-- controller       (need controller-reset)
     +-- controller-reset (need syscon-reset)
     +-- phy

The controller needs to deassert "controller-reset" in USB glue before
the controller registers are accessed. The glue needs to deassert
"syscon-reset" before the glue registers are accessed.

The glue itself doesn't have "syscon-reset", so the controller-reset
controls "syscon-reset" instead.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 drivers/reset/reset-uniphier.c | 78 +++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/reset-uniphier.c b/drivers/reset/reset-uniphier.c
index 7adae51873f3..35e3ccebd72e 100644
--- a/drivers/reset/reset-uniphier.c
+++ b/drivers/reset/reset-uniphier.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (C) 2016 Socionext Inc.
  *   Author: Masahiro Yamada <yamada.masahiro@socionext.com>
+ *   Author: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
  */
 
 #include <common.h>
@@ -9,6 +10,8 @@
 #include <log.h>
 #include <malloc.h>
 #include <reset-uclass.h>
+#include <clk.h>
+#include <reset.h>
 #include <dm/device_compat.h>
 #include <linux/bitops.h>
 #include <linux/io.h>
@@ -178,10 +181,17 @@ static const struct uniphier_reset_data uniphier_pro4_peri_reset_data[] = {
 	UNIPHIER_RESET_END,
 };
 
+/* Glue reset data */
+static const struct uniphier_reset_data uniphier_pro4_usb3_reset_data[] = {
+	UNIPHIER_RESETX(15, 0, 15)
+};
+
 /* core implementaton */
 struct uniphier_reset_priv {
 	void __iomem *base;
 	const struct uniphier_reset_data *data;
+	struct clk_bulk		clks;
+	struct reset_ctl_bulk	rsts;
 };
 
 static int uniphier_reset_update(struct reset_ctl *reset_ctl, int assert)
@@ -233,10 +243,47 @@ static const struct reset_ops uniphier_reset_ops = {
 	.rst_deassert = uniphier_reset_deassert,
 };
 
+static int uniphier_reset_rst_init(struct udevice *dev)
+{
+	struct uniphier_reset_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = reset_get_bulk(dev, &priv->rsts);
+	if (ret == -ENOSYS || ret == -ENOENT)
+		return 0;
+	else if (ret)
+		return ret;
+
+	ret = reset_deassert_bulk(&priv->rsts);
+	if (ret)
+		reset_release_bulk(&priv->rsts);
+
+	return ret;
+}
+
+static int uniphier_reset_clk_init(struct udevice *dev)
+{
+	struct uniphier_reset_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = clk_get_bulk(dev, &priv->clks);
+	if (ret == -ENOSYS || ret == -ENOENT)
+		return 0;
+	if (ret)
+		return ret;
+
+	ret = clk_enable_bulk(&priv->clks);
+	if (ret)
+		clk_release_bulk(&priv->clks);
+
+	return ret;
+}
+
 static int uniphier_reset_probe(struct udevice *dev)
 {
 	struct uniphier_reset_priv *priv = dev_get_priv(dev);
 	fdt_addr_t addr;
+	int ret;
 
 	addr = dev_read_addr(dev->parent);
 	if (addr == FDT_ADDR_T_NONE)
@@ -248,7 +295,11 @@ static int uniphier_reset_probe(struct udevice *dev)
 
 	priv->data = (void *)dev_get_driver_data(dev);
 
-	return 0;
+	ret = uniphier_reset_clk_init(dev);
+	if (ret)
+		return ret;
+
+	return uniphier_reset_rst_init(dev);
 }
 
 static const struct udevice_id uniphier_reset_match[] = {
@@ -355,6 +406,31 @@ static const struct udevice_id uniphier_reset_match[] = {
 		.compatible = "socionext,uniphier-pxs3-peri-reset",
 		.data = (ulong)uniphier_pro4_peri_reset_data,
 	},
+	/* USB glue reset */
+	{
+		.compatible = "socionext,uniphier-pro4-usb3-reset",
+		.data = (ulong)uniphier_pro4_usb3_reset_data,
+	},
+	{
+		.compatible = "socionext,uniphier-pro5-usb3-reset",
+		.data = (ulong)uniphier_pro4_usb3_reset_data,
+	},
+	{
+		.compatible = "socionext,uniphier-pxs2-usb3-reset",
+		.data = (ulong)uniphier_pro4_usb3_reset_data,
+	},
+	{
+		.compatible = "socionext,uniphier-ld20-usb3-reset",
+		.data = (ulong)uniphier_pro4_usb3_reset_data,
+	},
+	{
+		.compatible = "socionext,uniphier-pxs3-usb3-reset",
+		.data = (ulong)uniphier_pro4_usb3_reset_data,
+	},
+	{
+		.compatible = "socionext,uniphier-nx1-usb3-reset",
+		.data = (ulong)uniphier_pro4_usb3_reset_data,
+	},
 	{ /* sentinel */ }
 };
 
-- 
2.25.1

[PATCH v3 07/10] clk: uniphier: Add missing USB SS-PHY clocks

[Kunihiko Hayashi]

The USB SS-PHY needs its own clock, however, some clocks don't have
clock gates. Define missing clock entries for the PHY as reference
clock.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/clk/uniphier/clk-uniphier-sys.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/uniphier/clk-uniphier-sys.c b/drivers/clk/uniphier/clk-uniphier-sys.c
index ff5d364f5978..3b8595fe610a 100644
--- a/drivers/clk/uniphier/clk-uniphier-sys.c
+++ b/drivers/clk/uniphier/clk-uniphier-sys.c
@@ -28,7 +28,10 @@ const struct uniphier_clk_data uniphier_pxs2_sys_clk_data[] = {
 	UNIPHIER_CLK_GATE_SIMPLE(14, 0x2104, 16),	/* usb30 (Pro4, Pro5, PXs2) */
 	UNIPHIER_CLK_GATE_SIMPLE(15, 0x2104, 17),	/* usb31 (Pro4, Pro5, PXs2) */
 	UNIPHIER_CLK_GATE_SIMPLE(16, 0x2104, 19),	/* usb30-phy (PXs2) */
+	UNIPHIER_CLK_RATE(17, 25000000),		/* usb30-phy2 (PXs2) */
+	UNIPHIER_CLK_RATE(18, 25000000),		/* usb30-phy3 (PXs2) */
 	UNIPHIER_CLK_GATE_SIMPLE(20, 0x2104, 20),	/* usb31-phy (PXs2) */
+	UNIPHIER_CLK_RATE(21, 25000000),		/* usb31-phy2 (PXs2) */
 	UNIPHIER_CLK_GATE_SIMPLE(24, 0x2108, 2),	/* pcie (Pro5) */
 	{ /* sentinel */ }
 #endif
@@ -44,6 +47,8 @@ const struct uniphier_clk_data uniphier_ld20_sys_clk_data[] = {
 	UNIPHIER_CLK_GATE_SIMPLE(14, 0x210c, 14),	/* usb30 (LD20) */
 	UNIPHIER_CLK_GATE_SIMPLE(16, 0x210c, 12),	/* usb30-phy0 (LD20) */
 	UNIPHIER_CLK_GATE_SIMPLE(17, 0x210c, 13),	/* usb30-phy1 (LD20) */
+	UNIPHIER_CLK_RATE(18, 25000000),		/* usb30-phy2 (LD20) */
+	UNIPHIER_CLK_RATE(19, 25000000),		/* usb30-phy3 (LD20) */
 	UNIPHIER_CLK_GATE_SIMPLE(24, 0x210c, 4),	/* pcie */
 	{ /* sentinel */ }
 #endif
-- 
2.25.1

[PATCH v3 08/10] phy: socionext: Add UniPhier USB3 PHY driver

[Kunihiko Hayashi]

Add USB3 PHY driver support to control clocks and resets for the phy.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/phy/socionext/Kconfig             |  8 ++
 drivers/phy/socionext/Makefile            |  1 +
 drivers/phy/socionext/phy-uniphier-usb3.c | 93 +++++++++++++++++++++++
 3 files changed, 102 insertions(+)
 create mode 100644 drivers/phy/socionext/phy-uniphier-usb3.c

diff --git a/drivers/phy/socionext/Kconfig b/drivers/phy/socionext/Kconfig
index bcd579e98ec1..de87d5b0109b 100644
--- a/drivers/phy/socionext/Kconfig
+++ b/drivers/phy/socionext/Kconfig
@@ -10,3 +10,11 @@ config PHY_UNIPHIER_PCIE
 	help
 	  Enable this to support PHY implemented in PCIe controller
 	  on UniPhier SoCs.
+
+config PHY_UNIPHIER_USB3
+	bool "UniPhier USB3 PHY driver"
+	depends on PHY && ARCH_UNIPHIER
+	imply REGMAP
+	help
+	  Enable this to support PHY implemented in USB3 controller
+	  on UniPhier SoCs.
diff --git a/drivers/phy/socionext/Makefile b/drivers/phy/socionext/Makefile
index 5484360b70ff..94d3aa68cfac 100644
--- a/drivers/phy/socionext/Makefile
+++ b/drivers/phy/socionext/Makefile
@@ -4,3 +4,4 @@
 #
 
 obj-$(CONFIG_PHY_UNIPHIER_PCIE)	+= phy-uniphier-pcie.o
+obj-$(CONFIG_PHY_UNIPHIER_USB3)	+= phy-uniphier-usb3.o
diff --git a/drivers/phy/socionext/phy-uniphier-usb3.c b/drivers/phy/socionext/phy-uniphier-usb3.c
new file mode 100644
index 000000000000..ec3774569287
--- /dev/null
+++ b/drivers/phy/socionext/phy-uniphier-usb3.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * phy_uniphier_usb3.c - Socionext UniPhier Usb3 PHY driver
+ * Copyright 2019-2023 Socionext, Inc.
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <generic-phy.h>
+
+#include <clk.h>
+#include <reset.h>
+
+struct uniphier_usb3phy_priv {
+	struct clk_bulk clks;
+	struct reset_ctl_bulk rsts;
+};
+
+static int uniphier_usb3phy_init(struct phy *phy)
+{
+	struct uniphier_usb3phy_priv *priv = dev_get_priv(phy->dev);
+	int ret;
+
+	ret = clk_enable_bulk(&priv->clks);
+	if (ret)
+		goto out_clk;
+
+	ret = reset_deassert_bulk(&priv->rsts);
+	if (ret)
+		goto out_rst;
+
+	return 0;
+
+out_rst:
+	reset_release_bulk(&priv->rsts);
+out_clk:
+	clk_release_bulk(&priv->clks);
+
+	return ret;
+}
+
+static int uniphier_usb3phy_probe(struct udevice *dev)
+{
+	struct uniphier_usb3phy_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = clk_get_bulk(dev, &priv->clks);
+	if (ret) {
+		if (ret != -ENOSYS && ret != -ENOENT) {
+			printf("Failed to get clocks\n");
+			return ret;
+		}
+	}
+
+	ret = reset_get_bulk(dev, &priv->rsts);
+	if (ret) {
+		if (ret != -ENOSYS && ret != -ENOENT) {
+			printf("Failed to get resets\n");
+			clk_release_bulk(&priv->clks);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static struct phy_ops uniphier_usb3phy_ops = {
+	.init = uniphier_usb3phy_init,
+};
+
+static const struct udevice_id uniphier_usb3phy_ids[] = {
+	{ .compatible = "socionext,uniphier-pro4-usb3-ssphy" },
+	{ .compatible = "socionext,uniphier-pro5-usb3-hsphy" },
+	{ .compatible = "socionext,uniphier-pro5-usb3-ssphy" },
+	{ .compatible = "socionext,uniphier-pxs2-usb3-hsphy" },
+	{ .compatible = "socionext,uniphier-pxs2-usb3-ssphy" },
+	{ .compatible = "socionext,uniphier-ld20-usb3-hsphy" },
+	{ .compatible = "socionext,uniphier-ld20-usb3-ssphy" },
+	{ .compatible = "socionext,uniphier-pxs3-usb3-hsphy" },
+	{ .compatible = "socionext,uniphier-pxs3-usb3-ssphy" },
+	{ .compatible = "socionext,uniphier-nx1-usb3-hsphy" },
+	{ .compatible = "socionext,uniphier-nx1-usb3-ssphy" },
+	{ }
+};
+
+U_BOOT_DRIVER(uniphier_usb3_phy) = {
+	.name		= "uniphier-usb3-phy",
+	.id		= UCLASS_PHY,
+	.of_match	= uniphier_usb3phy_ids,
+	.ops		= &uniphier_usb3phy_ops,
+	.probe		= uniphier_usb3phy_probe,
+	.priv_auto      = sizeof(struct uniphier_usb3phy_priv),
+};
-- 
2.25.1

[PATCH v3 09/10] usb: dwc3-uniphier: Use dwc3-generic instead of xhci-dwc3

[Kunihiko Hayashi]

dwc3-uniphier depends on xhci-dwc3 framework, however, it is preferable
to use dwc3-generic.

This driver calls the exported dwc3-generic functions and redefine
the SoC-dependent operations to fit dwc3-generic.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 drivers/usb/dwc3/Kconfig         |   4 +-
 drivers/usb/dwc3/dwc3-uniphier.c | 116 ++++++++++++++++++++-----------
 2 files changed, 78 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index f010291d0223..7ddfa94e518d 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -55,7 +55,9 @@ config USB_DWC3_MESON_GXL
 
 config USB_DWC3_UNIPHIER
 	bool "DesignWare USB3 Host Support on UniPhier Platforms"
-	depends on ARCH_UNIPHIER && USB_XHCI_DWC3
+	depends on ARCH_UNIPHIER && USB_DWC3
+	select USB_DWC3_GENERIC
+	select PHY_UNIPHIER_USB3
 	help
 	  Support of USB2/3 functionality in Socionext UniPhier platforms.
 	  Say 'Y' here if you have one such device.
diff --git a/drivers/usb/dwc3/dwc3-uniphier.c b/drivers/usb/dwc3/dwc3-uniphier.c
index 54b52dcd66a9..ab85428a7003 100644
--- a/drivers/usb/dwc3/dwc3-uniphier.c
+++ b/drivers/usb/dwc3/dwc3-uniphier.c
@@ -4,14 +4,17 @@
  *
  * Copyright (C) 2016-2017 Socionext Inc.
  *   Author: Masahiro Yamada <yamada.masahiro@socionext.com>
+ *   Author: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
  */
 
 #include <dm.h>
-#include <dm/device_compat.h>
+#include <dm/lists.h>
 #include <linux/bitops.h>
-#include <linux/errno.h>
-#include <linux/io.h>
-#include <linux/sizes.h>
+#include <linux/usb/gadget.h>
+
+#include "core.h"
+#include "gadget.h"
+#include "dwc3-generic.h"
 
 #define UNIPHIER_PRO4_DWC3_RESET	0x40
 #define   UNIPHIER_PRO4_DWC3_RESET_XIOMMU	BIT(5)
@@ -27,8 +30,11 @@
 #define UNIPHIER_PXS2_DWC3_RESET	0x00
 #define   UNIPHIER_PXS2_DWC3_RESET_XLINK	BIT(15)
 
-static int uniphier_pro4_dwc3_init(void __iomem *regs)
+static void uniphier_pro4_dwc3_init(struct udevice *dev, int index,
+				    enum usb_dr_mode mode)
 {
+	struct dwc3_glue_data *glue = dev_get_plat(dev);
+	void *regs = map_physmem(glue->regs, glue->size, MAP_NOCACHE);
 	u32 tmp;
 
 	tmp = readl(regs + UNIPHIER_PRO4_DWC3_RESET);
@@ -36,11 +42,14 @@ static int uniphier_pro4_dwc3_init(void __iomem *regs)
 	tmp |= UNIPHIER_PRO4_DWC3_RESET_XIOMMU | UNIPHIER_PRO4_DWC3_RESET_XLINK;
 	writel(tmp, regs + UNIPHIER_PRO4_DWC3_RESET);
 
-	return 0;
+	unmap_physmem(regs, MAP_NOCACHE);
 }
 
-static int uniphier_pro5_dwc3_init(void __iomem *regs)
+static void uniphier_pro5_dwc3_init(struct udevice *dev, int index,
+				    enum usb_dr_mode mode)
 {
+	struct dwc3_glue_data *glue = dev_get_plat(dev);
+	void *regs = map_physmem(glue->regs, glue->size, MAP_NOCACHE);
 	u32 tmp;
 
 	tmp = readl(regs + UNIPHIER_PRO5_DWC3_RESET);
@@ -49,72 +58,97 @@ static int uniphier_pro5_dwc3_init(void __iomem *regs)
 	tmp |= UNIPHIER_PRO5_DWC3_RESET_XLINK | UNIPHIER_PRO5_DWC3_RESET_XIOMMU;
 	writel(tmp, regs + UNIPHIER_PRO5_DWC3_RESET);
 
-	return 0;
+	unmap_physmem(regs, MAP_NOCACHE);
 }
 
-static int uniphier_pxs2_dwc3_init(void __iomem *regs)
+static void uniphier_pxs2_dwc3_init(struct udevice *dev, int index,
+				    enum usb_dr_mode mode)
 {
+	struct dwc3_glue_data *glue = dev_get_plat(dev);
+	void *regs = map_physmem(glue->regs, glue->size, MAP_NOCACHE);
 	u32 tmp;
 
 	tmp = readl(regs + UNIPHIER_PXS2_DWC3_RESET);
 	tmp |= UNIPHIER_PXS2_DWC3_RESET_XLINK;
 	writel(tmp, regs + UNIPHIER_PXS2_DWC3_RESET);
 
-	return 0;
+	unmap_physmem(regs, MAP_NOCACHE);
 }
 
-static int uniphier_dwc3_probe(struct udevice *dev)
+static int dwc3_uniphier_glue_get_ctrl_dev(struct udevice *dev, ofnode *node)
 {
-	fdt_addr_t base;
-	void __iomem *regs;
-	int (*init)(void __iomem *regs);
-	int ret;
+	struct udevice *child;
+	const char *name;
+	ofnode subnode;
+
+	/*
+	 * "controller reset" belongs to glue logic, and it should be
+	 * accessible in .glue_configure() before access to the controller
+	 * begins.
+	 */
+	ofnode_for_each_subnode(subnode, dev_ofnode(dev)) {
+		name = ofnode_get_name(subnode);
+		if (!strncmp(name, "reset", 5))
+			device_bind_driver_to_node(dev, "uniphier-reset",
+						   name, subnode, &child);
+	}
+
+	/* Get controller node that is placed separately from the glue node */
+	*node = ofnode_by_compatible(dev_ofnode(dev->parent),
+				     "socionext,uniphier-dwc3");
 
-	base = dev_read_addr(dev);
-	if (base == FDT_ADDR_T_NONE)
-		return -EINVAL;
-
-	regs = ioremap(base, SZ_32K);
-	if (!regs)
-		return -ENOMEM;
+	return 0;
+}
 
-	init = (typeof(init))dev_get_driver_data(dev);
-	ret = init(regs);
-	if (ret)
-		dev_err(dev, "failed to init glue layer\n");
+static const struct dwc3_glue_ops uniphier_pro4_dwc3_ops = {
+	.glue_get_ctrl_dev = dwc3_uniphier_glue_get_ctrl_dev,
+	.glue_configure = uniphier_pro4_dwc3_init,
+};
 
-	iounmap(regs);
+static const struct dwc3_glue_ops uniphier_pro5_dwc3_ops = {
+	.glue_get_ctrl_dev = dwc3_uniphier_glue_get_ctrl_dev,
+	.glue_configure = uniphier_pro5_dwc3_init,
+};
 
-	return ret;
-}
+static const struct dwc3_glue_ops uniphier_pxs2_dwc3_ops = {
+	.glue_get_ctrl_dev = dwc3_uniphier_glue_get_ctrl_dev,
+	.glue_configure = uniphier_pxs2_dwc3_init,
+};
 
 static const struct udevice_id uniphier_dwc3_match[] = {
 	{
-		.compatible = "socionext,uniphier-pro4-dwc3",
-		.data = (ulong)uniphier_pro4_dwc3_init,
+		.compatible = "socionext,uniphier-pro4-dwc3-glue",
+		.data = (ulong)&uniphier_pro4_dwc3_ops,
+	},
+	{
+		.compatible = "socionext,uniphier-pro5-dwc3-glue",
+		.data = (ulong)&uniphier_pro5_dwc3_ops,
 	},
 	{
-		.compatible = "socionext,uniphier-pro5-dwc3",
-		.data = (ulong)uniphier_pro5_dwc3_init,
+		.compatible = "socionext,uniphier-pxs2-dwc3-glue",
+		.data = (ulong)&uniphier_pxs2_dwc3_ops,
 	},
 	{
-		.compatible = "socionext,uniphier-pxs2-dwc3",
-		.data = (ulong)uniphier_pxs2_dwc3_init,
+		.compatible = "socionext,uniphier-ld20-dwc3-glue",
+		.data = (ulong)&uniphier_pxs2_dwc3_ops,
 	},
 	{
-		.compatible = "socionext,uniphier-ld20-dwc3",
-		.data = (ulong)uniphier_pxs2_dwc3_init,
+		.compatible = "socionext,uniphier-pxs3-dwc3-glue",
+		.data = (ulong)&uniphier_pxs2_dwc3_ops,
 	},
 	{
-		.compatible = "socionext,uniphier-pxs3-dwc3",
-		.data = (ulong)uniphier_pxs2_dwc3_init,
+		.compatible = "socionext,uniphier-nx1-dwc3-glue",
+		.data = (ulong)&uniphier_pxs2_dwc3_ops,
 	},
 	{ /* sentinel */ }
 };
 
-U_BOOT_DRIVER(usb_xhci) = {
+U_BOOT_DRIVER(dwc3_uniphier_wrapper) = {
 	.name = "uniphier-dwc3",
 	.id = UCLASS_SIMPLE_BUS,
 	.of_match = uniphier_dwc3_match,
-	.probe = uniphier_dwc3_probe,
+	.bind = dwc3_glue_bind,
+	.probe = dwc3_glue_probe,
+	.remove = dwc3_glue_remove,
+	.plat_auto = sizeof(struct dwc3_glue_data),
 };
-- 
2.25.1

[PATCH v3 10/10] uniphier_defconfig: Disable USB_XHCI_DWC3

[Kunihiko Hayashi]

Replacing with dwc3-generic, no need USB_XHCI_DWC3 anymore.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
 configs/uniphier_v7_defconfig | 1 -
 configs/uniphier_v8_defconfig | 1 -
 2 files changed, 2 deletions(-)

diff --git a/configs/uniphier_v7_defconfig b/configs/uniphier_v7_defconfig
index d626968c76d4..03feb04b9378 100644
--- a/configs/uniphier_v7_defconfig
+++ b/configs/uniphier_v7_defconfig
@@ -82,7 +82,6 @@ CONFIG_DM_SPI=y
 CONFIG_UNIPHIER_SPI=y
 CONFIG_USB=y
 CONFIG_USB_XHCI_HCD=y
-CONFIG_USB_XHCI_DWC3=y
 CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_EHCI_GENERIC=y
 CONFIG_USB_DWC3=y
diff --git a/configs/uniphier_v8_defconfig b/configs/uniphier_v8_defconfig
index 6a0e2666cf18..ed58b5746e72 100644
--- a/configs/uniphier_v8_defconfig
+++ b/configs/uniphier_v8_defconfig
@@ -71,7 +71,6 @@ CONFIG_SYSRESET=y
 CONFIG_SYSRESET_PSCI=y
 CONFIG_USB=y
 CONFIG_USB_XHCI_HCD=y
-CONFIG_USB_XHCI_DWC3=y
 CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_EHCI_GENERIC=y
 CONFIG_USB_DWC3=y
-- 
2.25.1

Re: [PATCH v3 08/10] phy: socionext: Add UniPhier USB3 PHY driver

[Marek Vasut]

On 2/8/23 10:15, Kunihiko Hayashi wrote:

[...]

> +static int uniphier_usb3phy_init(struct phy *phy)
> +{
> +	struct uniphier_usb3phy_priv *priv = dev_get_priv(phy->dev);
> +	int ret;
> +
> +	ret = clk_enable_bulk(&priv->clks);
> +	if (ret)
> +		goto out_clk;

This should be just 'return ret;'

> +	ret = reset_deassert_bulk(&priv->rsts);
> +	if (ret)
> +		goto out_rst;

This should be goto out_rst, however ...

> +	return 0;
> +
> +out_rst:
> +	reset_release_bulk(&priv->rsts);
> +out_clk:
> +	clk_release_bulk(&priv->clks);

... the out_rst: should only do:

out_rst:
   clk_disable_bulk();
   return ret

out_clk part can be removed.

> +	return ret;
> +}
> +
> +static int uniphier_usb3phy_probe(struct udevice *dev)
> +{
> +	struct uniphier_usb3phy_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	ret = clk_get_bulk(dev, &priv->clks);
> +	if (ret) {
> +		if (ret != -ENOSYS && ret != -ENOENT) {

This can be single line:

if (ret && ret != -ENOSYS && ret != -ENOENT)
   return ret;

> +			printf("Failed to get clocks\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = reset_get_bulk(dev, &priv->rsts);
> +	if (ret) {
> +		if (ret != -ENOSYS && ret != -ENOENT) {

Same here.

> +			printf("Failed to get resets\n");
> +			clk_release_bulk(&priv->clks);

Better use goto out_clk fail path.

> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct phy_ops uniphier_usb3phy_ops = {
> +	.init = uniphier_usb3phy_init,

You should implement .exit callback too, that one should do these two steps:

reset_assert_bulk()
clk_disable_bulk()

> +};

[...]

Re: [PATCH v3 07/10] clk: uniphier: Add missing USB SS-PHY clocks

[Sean Anderson]

On 2/8/23 04:15, Kunihiko Hayashi wrote:
> The USB SS-PHY needs its own clock, however, some clocks don't have
> clock gates. Define missing clock entries for the PHY as reference
> clock.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>   drivers/clk/uniphier/clk-uniphier-sys.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/clk/uniphier/clk-uniphier-sys.c b/drivers/clk/uniphier/clk-uniphier-sys.c
> index ff5d364f5978..3b8595fe610a 100644
> --- a/drivers/clk/uniphier/clk-uniphier-sys.c
> +++ b/drivers/clk/uniphier/clk-uniphier-sys.c
> @@ -28,7 +28,10 @@ const struct uniphier_clk_data uniphier_pxs2_sys_clk_data[] = {
>   	UNIPHIER_CLK_GATE_SIMPLE(14, 0x2104, 16),	/* usb30 (Pro4, Pro5, PXs2) */
>   	UNIPHIER_CLK_GATE_SIMPLE(15, 0x2104, 17),	/* usb31 (Pro4, Pro5, PXs2) */
>   	UNIPHIER_CLK_GATE_SIMPLE(16, 0x2104, 19),	/* usb30-phy (PXs2) */
> +	UNIPHIER_CLK_RATE(17, 25000000),		/* usb30-phy2 (PXs2) */
> +	UNIPHIER_CLK_RATE(18, 25000000),		/* usb30-phy3 (PXs2) */
>   	UNIPHIER_CLK_GATE_SIMPLE(20, 0x2104, 20),	/* usb31-phy (PXs2) */
> +	UNIPHIER_CLK_RATE(21, 25000000),		/* usb31-phy2 (PXs2) */
>   	UNIPHIER_CLK_GATE_SIMPLE(24, 0x2108, 2),	/* pcie (Pro5) */
>   	{ /* sentinel */ }
>   #endif
> @@ -44,6 +47,8 @@ const struct uniphier_clk_data uniphier_ld20_sys_clk_data[] = {
>   	UNIPHIER_CLK_GATE_SIMPLE(14, 0x210c, 14),	/* usb30 (LD20) */
>   	UNIPHIER_CLK_GATE_SIMPLE(16, 0x210c, 12),	/* usb30-phy0 (LD20) */
>   	UNIPHIER_CLK_GATE_SIMPLE(17, 0x210c, 13),	/* usb30-phy1 (LD20) */
> +	UNIPHIER_CLK_RATE(18, 25000000),		/* usb30-phy2 (LD20) */
> +	UNIPHIER_CLK_RATE(19, 25000000),		/* usb30-phy3 (LD20) */
>   	UNIPHIER_CLK_GATE_SIMPLE(24, 0x210c, 4),	/* pcie */
>   	{ /* sentinel */ }
>   #endif

Acked-by: Sean Anderson <seanga2@gmail.com>

Re: [PATCH v3 08/10] phy: socionext: Add UniPhier USB3 PHY driver

[Kunihiko Hayashi]

Hi Marek,

On 2023/02/10 23:09, Marek Vasut wrote:
> On 2/8/23 10:15, Kunihiko Hayashi wrote:
> 
> [...]
> 
>> +static int uniphier_usb3phy_init(struct phy *phy)
>> +{
>> +	struct uniphier_usb3phy_priv *priv = dev_get_priv(phy->dev);
>> +	int ret;
>> +
>> +	ret = clk_enable_bulk(&priv->clks);
>> +	if (ret)
>> +		goto out_clk;
> 
> This should be just 'return ret;'
> 
>> +	ret = reset_deassert_bulk(&priv->rsts);
>> +	if (ret)
>> +		goto out_rst;
> 
> This should be goto out_rst, however ...
> 
>> +	return 0;
>> +
>> +out_rst:
>> +	reset_release_bulk(&priv->rsts);
>> +out_clk:
>> +	clk_release_bulk(&priv->clks);
> 
> ... the out_rst: should only do:
> 
> out_rst:
>     clk_disable_bulk();
>     return ret
> 
> out_clk part can be removed.

I see. These operations are unpaired.
I'll fix them.

>> +	return ret;
>> +}
>> +
>> +static int uniphier_usb3phy_probe(struct udevice *dev)
>> +{
>> +	struct uniphier_usb3phy_priv *priv = dev_get_priv(dev);
>> +	int ret;
>> +
>> +	ret = clk_get_bulk(dev, &priv->clks);
>> +	if (ret) {
>> +		if (ret != -ENOSYS && ret != -ENOENT) {
> 
> This can be single line:
> 
> if (ret && ret != -ENOSYS && ret != -ENOENT)
>     return ret;

OK, This will be fixed.

>> +			printf("Failed to get clocks\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	ret = reset_get_bulk(dev, &priv->rsts);
>> +	if (ret) {
>> +		if (ret != -ENOSYS && ret != -ENOENT) {
> 
> Same here.
> 
>> +			printf("Failed to get resets\n");
>> +			clk_release_bulk(&priv->clks);
> 
> Better use goto out_clk fail path.
> 
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct phy_ops uniphier_usb3phy_ops = {
>> +	.init = uniphier_usb3phy_init,
> 
> You should implement .exit callback too, that one should do these two steps:
> 
> reset_assert_bulk()
> clk_disable_bulk()

I think so, however, when I added .exit() and executed "usb stop;usb start",
unfortunately the command got stuck.

Currently uniphier clk doesn't support CLK_CCF and can't be nested.
I think more changes are needed.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH v3 08/10] phy: socionext: Add UniPhier USB3 PHY driver

[Marek Vasut]

On 2/13/23 04:08, Kunihiko Hayashi wrote:
> Hi Marek,

Hello Hayashi-san,

> On 2023/02/10 23:09, Marek Vasut wrote:
>> On 2/8/23 10:15, Kunihiko Hayashi wrote:
>>
>> [...]
>>
>>> +static int uniphier_usb3phy_init(struct phy *phy)
>>> +{
>>> +    struct uniphier_usb3phy_priv *priv = dev_get_priv(phy->dev);
>>> +    int ret;
>>> +
>>> +    ret = clk_enable_bulk(&priv->clks);
>>> +    if (ret)
>>> +        goto out_clk;
>>
>> This should be just 'return ret;'
>>
>>> +    ret = reset_deassert_bulk(&priv->rsts);
>>> +    if (ret)
>>> +        goto out_rst;
>>
>> This should be goto out_rst, however ...
>>
>>> +    return 0;
>>> +
>>> +out_rst:
>>> +    reset_release_bulk(&priv->rsts);
>>> +out_clk:
>>> +    clk_release_bulk(&priv->clks);
>>
>> ... the out_rst: should only do:
>>
>> out_rst:
>>     clk_disable_bulk();
>>     return ret
>>
>> out_clk part can be removed.
> 
> I see. These operations are unpaired.
> I'll fix them.

Thank you

>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static struct phy_ops uniphier_usb3phy_ops = {
>>> +    .init = uniphier_usb3phy_init,
>>
>> You should implement .exit callback too, that one should do these two 
>> steps:
>>
>> reset_assert_bulk()
>> clk_disable_bulk()
> 
> I think so, however, when I added .exit() and executed "usb stop;usb 
> start",
> unfortunately the command got stuck.
> 
> Currently uniphier clk doesn't support CLK_CCF and can't be nested.
> I think more changes are needed.

Do you know where exactly the system hangs ?

Do you expect to add the CCF support ?

If so, then add at least a FIXME code comment that the exit callback 
must be implemented, so that this is not forgotten once CCF is in place. 
I don't want this series to grow much further into some "rework 
everything at once" thing.

Re: [PATCH v3 08/10] phy: socionext: Add UniPhier USB3 PHY driver

[Kunihiko Hayashi]

Hi Marek,

Sorry for late reply.

On 2023/02/14 6:06, Marek Vasut wrote:
> On 2/13/23 04:08, Kunihiko Hayashi wrote:
>> Hi Marek,
> 
> Hello Hayashi-san,
> 

[...]

>> I think so, however, when I added .exit() and executed "usb stop;usb
>> start",
>> unfortunately the command got stuck.
>>
>> Currently uniphier clk doesn't support CLK_CCF and can't be nested.
>> I think more changes are needed.
> 
> Do you know where exactly the system hangs ?

Yes.

The controller-reset driver controls the clock/reset for the glue
in probe(). The phy driver controls the clock/reset for the glue and
the phy in init(). There is an inconsistency.

When submitting "usb stop", the phy driver disables all the clock/reset
in exit().

When submitting "usb start" again, the controller-reset driver accesses
the glue register without enabling the clock/reset, and the system hangs.

> Do you expect to add the CCF support ?

Yes, first I thought the clock needed the CCF support to count the clock
function calls, but there is no support to count the reset function calls.
I need another solution.

Currently the phy driver controls all the clock/reset in init() and exit(),
however, it should control the phy clock/reset only.

> If so, then add at least a FIXME code comment that the exit callback
> must be implemented, so that this is not forgotten once CCF is in place.
> I don't want this series to grow much further into some "rework
> everything at once" thing.

As above, I should put the glue clock/reset into probe(),
and the phy clock/reset into init() without using bulk function.

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH v3 08/10] phy: socionext: Add UniPhier USB3 PHY driver

[Marek Vasut]

On 2/16/23 17:14, Kunihiko Hayashi wrote:
> Hi Marek,

Hello Hayashi-san,

> Sorry for late reply.
> 
> On 2023/02/14 6:06, Marek Vasut wrote:
>> On 2/13/23 04:08, Kunihiko Hayashi wrote:
>>> Hi Marek,
>>
>> Hello Hayashi-san,
>>
> 
> [...]
> 
>>> I think so, however, when I added .exit() and executed "usb stop;usb
>>> start",
>>> unfortunately the command got stuck.
>>>
>>> Currently uniphier clk doesn't support CLK_CCF and can't be nested.
>>> I think more changes are needed.
>>
>> Do you know where exactly the system hangs ?
> 
> Yes.
> 
> The controller-reset driver controls the clock/reset for the glue
> in probe(). The phy driver controls the clock/reset for the glue and
> the phy in init(). There is an inconsistency.
> 
> When submitting "usb stop", the phy driver disables all the clock/reset
> in exit().
> 
> When submitting "usb start" again, the controller-reset driver accesses
> the glue register without enabling the clock/reset, and the system hangs.

Shouldn't the PHY enable its own set of clock, while the controller 
should only enable its own (different) set of clock ?

If it is the same clock and there is no refcounting, then maybe what you 
should do for now is enable/disable all the clock in the controller 
driver only, and deal with the PHY once CCF with refcounting is in place ?

>> Do you expect to add the CCF support ?
> 
> Yes, first I thought the clock needed the CCF support to count the clock
> function calls, but there is no support to count the reset function calls.
> I need another solution.
> 
> Currently the phy driver controls all the clock/reset in init() and exit(),
> however, it should control the phy clock/reset only.

If that's possible on this hardware, that would be good, let controller 
control its clock and PHY control its (different) clock.

>> If so, then add at least a FIXME code comment that the exit callback
>> must be implemented, so that this is not forgotten once CCF is in place.
>> I don't want this series to grow much further into some "rework
>> everything at once" thing.
> 
> As above, I should put the glue clock/reset into probe(),
> and the phy clock/reset into init() without using bulk function.

OK

Re: [PATCH v3 08/10] phy: socionext: Add UniPhier USB3 PHY driver

[Kunihiko Hayashi]

Hi Marek,

On 2023/02/18 4:58, Marek Vasut wrote:
> On 2/16/23 17:14, Kunihiko Hayashi wrote:
>> Hi Marek,
> 
> Hello Hayashi-san,
> 
>> Sorry for late reply.
>>
>> On 2023/02/14 6:06, Marek Vasut wrote:
>>> On 2/13/23 04:08, Kunihiko Hayashi wrote:
>>>> Hi Marek,
>>>
>>> Hello Hayashi-san,
>>>
>>
>> [...]
>>
>>>> I think so, however, when I added .exit() and executed "usb stop;usb
>>>> start",
>>>> unfortunately the command got stuck.
>>>>
>>>> Currently uniphier clk doesn't support CLK_CCF and can't be nested.
>>>> I think more changes are needed.
>>>
>>> Do you know where exactly the system hangs ?
>>
>> Yes.
>>
>> The controller-reset driver controls the clock/reset for the glue
>> in probe(). The phy driver controls the clock/reset for the glue and
>> the phy in init(). There is an inconsistency.
>>
>> When submitting "usb stop", the phy driver disables all the clock/reset
>> in exit().
>>
>> When submitting "usb start" again, the controller-reset driver accesses
>> the glue register without enabling the clock/reset, and the system hangs.
> 
> Shouldn't the PHY enable its own set of clock, while the controller
> should only enable its own (different) set of clock ?

PHY has this own clock/reset, however, PHY is in the glue logic.
So accessing to PHY also needs to enable the glue clock
(same as the controller clock).

> If it is the same clock and there is no refcounting, then maybe what you
> should do for now is enable/disable all the clock in the controller
> driver only, and deal with the PHY once CCF with refcounting is in place ?

PHY needs the glue(controller) clock and the phy clock.
After all, it's wrong to control all the clocks/resets at the same time.

In this time, I don't event think about CCF (sorry to confuse you).

>>> Do you expect to add the CCF support ?
>>
>> Yes, first I thought the clock needed the CCF support to count the clock
>> function calls, but there is no support to count the reset function calls.
>> I need another solution.
>>
>> Currently the phy driver controls all the clock/reset in init() and
>> exit(),
>> however, it should control the phy clock/reset only.
> 
> If that's possible on this hardware, that would be good, let controller
> control its clock and PHY control its (different) clock.

Yes. I'll separate these controls.

>>> If so, then add at least a FIXME code comment that the exit callback
>>> must be implemented, so that this is not forgotten once CCF is in place.
>>> I don't want this series to grow much further into some "rework
>>> everything at once" thing.
>>
>> As above, I should put the glue clock/reset into probe(),
>> and the phy clock/reset into init() without using bulk function.
> 
> OK

Thank you,

---
Best Regards
Kunihiko Hayashi