[PATCH v3 0/3] Add support for multiport controller

[PATCH v3 0/3] Add support for multiport controller

[Harsh Agarwal]

Currently the DWC3 driver supports only single port controller which 
requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
DWC3 controller with multiple ports that can operate in host mode. Some of
the port supports both SS+HS and other port supports only HS mode.

This change refactors the PHY logic to support multiport controller. The 
implementation has been tested with Generic PHYs as well.

For any multiport controller we would define a new node "multiport" inside
dwc3 and then add subsequent "mport" nodes inside it for individual ports
that it supports. Now each individual "mport" node defines their own PHYs.

e.g.
Consider a Dual port controller where each port supports HS+SS 

multiport {
	mp_1: mport1 {
		usb-phy = <usb2_phy0>, <usb3_phy0>;
        /* Can define Generic PHYs also */  
	};	
	mp_2: mport2 {
		usb-phy = <usb2_phy1>, <usb3_phy1>;
	};	

Changes in v3:
Incase any PHY init fails, then clear/exit the PHYs that
are already initialized.

Changes in v2:
Changed dwc3_count_phys to return the number of PHY Phandles in the node.
This will be used now in dwc3_extract_num_phys to increment num_usb2_phy 
and num_usb3_phy.
Added new parameter "ss_idx" in dwc3_core_get_phy_ny_node and changed its
structure such that the first half is for HS-PHY and second half is for
SS-PHY.
In dwc3_core_get_phy, for multiport controller, only if SS-PHY phandle is
present, pass proper SS_IDX else pass -1.

Harsh Agarwal (3):
  dt-bindings: usb: dwc3: Add support for multiport related properties
  usb: phy: Add devm_of_usb_get_phy_by_phandle
  usb: dwc3: Refactor PHY logic to support Multiport Controller

 .../devicetree/bindings/usb/snps,dwc3.yaml         |  53 +++
 drivers/usb/dwc3/core.c                            | 429 +++++++++++++++------
 drivers/usb/dwc3/core.h                            |  12 +-
 drivers/usb/dwc3/drd.c                             |  16 +-
 drivers/usb/dwc3/gadget.c                          |   4 +-
 drivers/usb/phy/phy.c                              |  34 ++
 include/linux/usb/phy.h                            |   8 +
 7 files changed, 426 insertions(+), 130 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] dt-bindings: usb: dwc3: Add support for multiport related properties

[Harsh Agarwal]

Added support for multiport, mport, num_usb2_phy and num_usb3_phy
properties. These properties are used to support devices having
a multiport controller.

Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
---
 .../devicetree/bindings/usb/snps,dwc3.yaml         | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index d41265b..9332fa2 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -343,6 +343,32 @@ properties:
       This port is used with the 'usb-role-switch' property  to connect the
       dwc3 to type C connector.
 
+  multiport:
+    description:
+      If a single USB controller supports multiple ports, then it's referred to as
+      a multiport controller. Each port of the multiport controller can support
+      either High Speed or Super Speed or both and have their own PHY phandles. Each
+      port is represented by "mport" node and all the "mport" nodes are grouped
+      together inside the "multiport" node where individual "mport" node defines the
+      PHYs supported by that port.
+
+  num_usb2_phy:
+    description: Total number of HS-PHYs defined by the multiport controller.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  num_usb3_phy:
+    description: Total number of SS-PHYs defined by the multiport controller.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  mport:
+    description: Each mport node represents one port of the multiport controller.
+    oneOf:
+      - required:
+        - usb-phy
+      - required:
+        - phys
+        - phy-names
+
 unevaluatedProperties: false
 
 required:
@@ -371,4 +397,31 @@ examples:
       snps,dis_u2_susphy_quirk;
       snps,dis_enblslpm_quirk;
     };
+  - |
+    usb@4a000000 {
+      compatible = "snps,dwc3";
+      reg = <0x4a000000 0xcfff>;
+      interrupts = <0 92 4>;
+
+      multiport {
+
+        MP_1: mport1 {
+          usb-phy = <&usb2_phy0>, <&usb3_phy0>;
+          /* Can define Generic PHYs also */
+        };
+
+        MP_2: mport2 {
+          usb-phy = <&usb2_phy1>, <&usb3_phy1>;
+        };
+
+        MP_3: mport3 {
+          usb-phy = <&usb2_phy2>;
+        };
+
+        MP_4: mport4 {
+          usb-phy = <&usb2_phy3>;
+        };
+
+      };
+    };
 ...
-- 
2.7.4

[PATCH v3 2/3] usb: phy: Add devm_of_usb_get_phy_by_phandle

[Harsh Agarwal]

Adding support for devm_of_usb_get_phy_by_phandle which allows
us to get PHY phandles of a device declared inside lookup_node.

Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
---
 drivers/usb/phy/phy.c   | 34 ++++++++++++++++++++++++++++++++++
 include/linux/usb/phy.h |  8 ++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
index 1b24492..0843757 100644
--- a/drivers/usb/phy/phy.c
+++ b/drivers/usb/phy/phy.c
@@ -615,6 +615,40 @@ struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
 EXPORT_SYMBOL_GPL(devm_usb_get_phy_by_phandle);
 
 /**
+ * devm_of_usb_get_phy_by_phandle - find the USB PHY by phandle in lookup_node
+ * @dev: device that requests this phy
+ * @phandle: name of the property holding the phy phandle value
+ * @index: the index of the phy
+ * @lookup_node: The node to search for PHY phandles.
+ *
+ * Returns the phy driver associated with the given phandle value,
+ * after getting a refcount to it, -ENODEV if there is no such phy or
+ * -EPROBE_DEFER if there is a phandle to the phy, but the device is
+ * not yet loaded. While at that, it also associates the device with
+ * the phy using devres. On driver detach, release function is invoked
+ * on the devres data, then, devres data is freed.
+ *
+ * For use by USB host and peripheral drivers.
+ */
+struct usb_phy *devm_of_usb_get_phy_by_phandle(struct device *dev,
+	const char *phandle, u8 index, struct device_node *lookup_node)
+{
+	struct device_node *node;
+	struct usb_phy	*phy;
+
+	node = of_parse_phandle(lookup_node, phandle, index);
+	if (!node) {
+		dev_dbg(dev, "failed to get %s phandle in %pOF node\n", phandle,
+			dev->of_node);
+		return ERR_PTR(-ENODEV);
+	}
+	phy = devm_usb_get_phy_by_node(dev, node, NULL);
+	of_node_put(node);
+	return phy;
+}
+EXPORT_SYMBOL_GPL(devm_of_usb_get_phy_by_phandle);
+
+/**
  * devm_usb_put_phy - release the USB PHY
  * @dev: device that wants to release this phy
  * @phy: the phy returned by devm_usb_get_phy()
diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
index e4de6bc..5685b68 100644
--- a/include/linux/usb/phy.h
+++ b/include/linux/usb/phy.h
@@ -220,6 +220,8 @@ extern struct usb_phy *devm_usb_get_phy(struct device *dev,
 	enum usb_phy_type type);
 extern struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
 	const char *phandle, u8 index);
+extern struct usb_phy *devm_of_usb_get_phy_by_phandle(struct device *dev,
+	const char *phandle, u8 index, struct device_node *lookup_node);
 extern struct usb_phy *devm_usb_get_phy_by_node(struct device *dev,
 	struct device_node *node, struct notifier_block *nb);
 extern void usb_put_phy(struct usb_phy *);
@@ -249,6 +251,12 @@ static inline struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
 	return ERR_PTR(-ENXIO);
 }
 
+static inline struct usb_phy *devm_of_usb_get_phy_by_phandle(struct device *dev,
+	const char *phandle, u8 index, struct device_node *lookup_node)
+{
+	return ERR_PTR(-ENXIO);
+}
+
 static inline struct usb_phy *devm_usb_get_phy_by_node(struct device *dev,
 	struct device_node *node, struct notifier_block *nb)
 {
-- 
2.7.4

[PATCH v3 3/3] usb: dwc3: Refactor PHY logic to support Multiport Controller

[Harsh Agarwal]

Currently the DWC3 driver supports only single port controller
which requires at most 2 PHYs ie HS and SS PHYs.

But some SOCs have a "multiport" USB DWC3 controller where a
single controller supports multiple ports and each port have
their own PHYs. Refactor PHY logic to support the same.

Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
---
 drivers/usb/dwc3/core.c   | 429 +++++++++++++++++++++++++++++++++-------------
 drivers/usb/dwc3/core.h   |  12 +-
 drivers/usb/dwc3/drd.c    |  16 +-
 drivers/usb/dwc3/gadget.c |   4 +-
 4 files changed, 331 insertions(+), 130 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5734219..b221915 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
 {
 	struct dwc3 *dwc = work_to_dwc(work);
 	unsigned long flags;
-	int ret;
+	int i, ret;
 	u32 reg;
 
 	mutex_lock(&dwc->mutex);
@@ -189,10 +189,13 @@ static void __dwc3_set_mode(struct work_struct *work)
 		if (ret) {
 			dev_err(dwc->dev, "failed to initialize host\n");
 		} else {
-			if (dwc->usb2_phy)
-				otg_set_vbus(dwc->usb2_phy->otg, true);
-			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
-			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
+			for (i = 0; i < dwc->num_usb2_phy; i++) {
+				if (dwc->usb2_phy[i])
+					otg_set_vbus(dwc->usb2_phy[i]->otg, true);
+				phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
+			}
+			for (i = 0; i < dwc->num_usb3_phy; i++)
+				phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
 			if (dwc->dis_split_quirk) {
 				reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
 				reg |= DWC3_GUCTL3_SPLITDISABLE;
@@ -205,10 +208,10 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 		dwc3_event_buffers_setup(dwc);
 
-		if (dwc->usb2_phy)
-			otg_set_vbus(dwc->usb2_phy->otg, false);
-		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
-		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
+		if (dwc->usb2_phy[0])
+			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
+		phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
+		phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);
 
 		ret = dwc3_gadget_init(dwc);
 		if (ret)
@@ -656,6 +659,7 @@ static int dwc3_core_ulpi_init(struct dwc3 *dwc)
  */
 static int dwc3_phy_setup(struct dwc3 *dwc)
 {
+	int i;
 	unsigned int hw_mode;
 	u32 reg;
 
@@ -716,7 +720,8 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 	if (dwc->dis_del_phy_power_chg_quirk)
 		reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
 
-	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
+	for (i = 0; i < dwc->num_usb3_phy; i++)
+		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), reg);
 
 	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
 
@@ -730,7 +735,8 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 		} else if (dwc->hsphy_interface &&
 				!strncmp(dwc->hsphy_interface, "ulpi", 4)) {
 			reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
-			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+			for (i = 0; i < dwc->num_usb2_phy; i++)
+				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
 		} else {
 			/* Relying on default value. */
 			if (!(reg & DWC3_GUSB2PHYCFG_ULPI_UTMI))
@@ -787,7 +793,8 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 	if (dwc->dis_u2_freeclk_exists_quirk)
 		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
 
-	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+	for (i = 0; i < dwc->num_usb2_phy; i++)
+		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
 
 	return 0;
 }
@@ -826,17 +833,23 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
 
 static void dwc3_core_exit(struct dwc3 *dwc)
 {
+	int i;
 	dwc3_event_buffers_cleanup(dwc);
 
-	usb_phy_shutdown(dwc->usb2_phy);
-	usb_phy_shutdown(dwc->usb3_phy);
-	phy_exit(dwc->usb2_generic_phy);
-	phy_exit(dwc->usb3_generic_phy);
+	for (i = 0; i < dwc->num_usb2_phy; i++) {
+		usb_phy_shutdown(dwc->usb2_phy[i]);
+		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
+		phy_exit(dwc->usb2_generic_phy[i]);
+		phy_power_off(dwc->usb2_generic_phy[i]);
+	}
+
+	for (i = 0; i < dwc->num_usb3_phy; i++) {
+		usb_phy_shutdown(dwc->usb3_phy[i]);
+		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
+		phy_exit(dwc->usb3_generic_phy[i]);
+		phy_power_off(dwc->usb3_generic_phy[i]);
+	}
 
-	usb_phy_set_suspend(dwc->usb2_phy, 1);
-	usb_phy_set_suspend(dwc->usb3_phy, 1);
-	phy_power_off(dwc->usb2_generic_phy);
-	phy_power_off(dwc->usb3_generic_phy);
 	dwc3_clk_disable(dwc);
 	reset_control_assert(dwc->reset);
 }
@@ -1039,7 +1052,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
 {
 	unsigned int		hw_mode;
 	u32			reg;
-	int			ret;
+	int			ret, i, j;
 
 	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
 
@@ -1067,16 +1080,50 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		dwc->phys_ready = true;
 	}
 
-	usb_phy_init(dwc->usb2_phy);
-	usb_phy_init(dwc->usb3_phy);
-	ret = phy_init(dwc->usb2_generic_phy);
-	if (ret < 0)
-		goto err0a;
+	for (i = 0; i < dwc->num_usb2_phy; i++) {
+		ret = usb_phy_init(dwc->usb2_phy[i]);
+		if (ret < 0) {
+			/* clean up prior initialized HS PHYs */
+			for (j = 0; j < i; j++)
+				usb_phy_shutdown(dwc->usb2_phy[j]);
+			goto err0a;
+		}
+	}
 
-	ret = phy_init(dwc->usb3_generic_phy);
-	if (ret < 0) {
-		phy_exit(dwc->usb2_generic_phy);
-		goto err0a;
+	for (i = 0; i < dwc->num_usb3_phy; i++) {
+		ret = usb_phy_init(dwc->usb3_phy[i]);
+		if (ret < 0) {
+			/* clean up prior initialized SS PHYs */
+			for (j = 0; j < i; j++)
+				usb_phy_shutdown(dwc->usb3_phy[j]);
+			/* clean up prior initialized HS PHYs */
+			for (i = 0; i < dwc->num_usb2_phy; i++)
+				usb_phy_shutdown(dwc->usb2_phy[i]);
+			goto err0a;
+		}
+	}
+
+	for (i = 0; i < dwc->num_usb2_phy; i++) {
+		ret = phy_init(dwc->usb2_generic_phy[i]);
+		if (ret < 0) {
+			/* clean up prior initialized HS PHYs */
+			for (j = 0; j < i; j++)
+				phy_exit(dwc->usb2_generic_phy[j]);
+			goto err0a;
+		}
+	}
+
+	for (i = 0; i < dwc->num_usb3_phy; i++) {
+		ret = phy_init(dwc->usb3_generic_phy[i]);
+		if (ret < 0) {
+			/* clean up prior initialized SS PHYs */
+			for (j = 0; j < i; j++)
+				phy_exit(dwc->usb3_generic_phy[j]);
+			/* clean up prior initialized HS PHYs */
+			for (i = 0; i < dwc->num_usb2_phy; i++)
+				phy_exit(dwc->usb2_generic_phy[i]);
+			goto err0a;
+		}
 	}
 
 	ret = dwc3_core_soft_reset(dwc);
@@ -1086,15 +1133,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD &&
 	    !DWC3_VER_IS_WITHIN(DWC3, ANY, 194A)) {
 		if (!dwc->dis_u3_susphy_quirk) {
-			reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
-			reg |= DWC3_GUSB3PIPECTL_SUSPHY;
-			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
+			for (i = 0; i < dwc->num_usb3_phy; i++) {
+				reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i));
+				reg |= DWC3_GUSB3PIPECTL_SUSPHY;
+				dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), reg);
+			}
 		}
 
 		if (!dwc->dis_u2_susphy_quirk) {
-			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
-			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+			for (i = 0; i < dwc->num_usb2_phy; i++) {
+				reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
+				reg |= DWC3_GUSB2PHYCFG_SUSPHY;
+				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
+			}
 		}
 	}
 
@@ -1113,15 +1164,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
 
 	dwc3_set_incr_burst_type(dwc);
 
-	usb_phy_set_suspend(dwc->usb2_phy, 0);
-	usb_phy_set_suspend(dwc->usb3_phy, 0);
-	ret = phy_power_on(dwc->usb2_generic_phy);
-	if (ret < 0)
-		goto err2;
+	for (i = 0; i < dwc->num_usb2_phy; i++) {
+		usb_phy_set_suspend(dwc->usb2_phy[i], 0);
+		ret = phy_power_on(dwc->usb2_generic_phy[i]);
+		if (ret < 0)
+			goto err2;
+	}
 
-	ret = phy_power_on(dwc->usb3_generic_phy);
-	if (ret < 0)
-		goto err3;
+	for (i = 0; i < dwc->num_usb3_phy; i++) {
+		usb_phy_set_suspend(dwc->usb3_phy[i], 0);
+		ret = phy_power_on(dwc->usb3_generic_phy[i]);
+		if (ret < 0)
+			goto err3;
+	}
 
 	ret = dwc3_event_buffers_setup(dwc);
 	if (ret) {
@@ -1229,20 +1284,29 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	return 0;
 
 err4:
-	phy_power_off(dwc->usb3_generic_phy);
+	for (i = 0; i < dwc->num_usb3_phy; i++)
+		phy_power_off(dwc->usb3_generic_phy[i]);
 
 err3:
-	phy_power_off(dwc->usb2_generic_phy);
+	for (i = 0; i < dwc->num_usb2_phy; i++)
+		phy_power_off(dwc->usb2_generic_phy[i]);
 
 err2:
-	usb_phy_set_suspend(dwc->usb2_phy, 1);
-	usb_phy_set_suspend(dwc->usb3_phy, 1);
+	for (i = 0; i < dwc->num_usb2_phy; i++)
+		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
+	for (i = 0; i < dwc->num_usb3_phy; i++)
+		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
 
 err1:
-	usb_phy_shutdown(dwc->usb2_phy);
-	usb_phy_shutdown(dwc->usb3_phy);
-	phy_exit(dwc->usb2_generic_phy);
-	phy_exit(dwc->usb3_generic_phy);
+	for (i = 0; i < dwc->num_usb2_phy; i++) {
+		usb_phy_shutdown(dwc->usb2_phy[i]);
+		phy_exit(dwc->usb2_generic_phy[i]);
+	}
+
+	for (i = 0; i < dwc->num_usb3_phy; i++) {
+		usb_phy_shutdown(dwc->usb3_phy[i]);
+		phy_exit(dwc->usb3_generic_phy[i]);
+	}
 
 err0a:
 	dwc3_ulpi_exit(dwc);
@@ -1251,53 +1315,169 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	return ret;
 }
 
-static int dwc3_core_get_phy(struct dwc3 *dwc)
+static int dwc3_count_phys(struct dwc3 *dwc, struct device_node *lookup_node)
+{
+	int count;
+
+	count = of_count_phandle_with_args(lookup_node, "phys", NULL);
+
+	if (count == -ENOENT)
+		count = of_count_phandle_with_args(lookup_node, "usb-phy", NULL);
+
+	return count;
+}
+
+static int dwc3_extract_num_phys(struct dwc3 *dwc)
+{
+	struct device_node	*ports, *port;
+	int			ret;
+
+	/* Find if any "multiport" child is present inside DWC3 */
+	for_each_available_child_of_node(dwc->dev->of_node, ports) {
+		if (!strcmp(ports->name, "multiport"))
+			break;
+	}
+	if (!ports) {
+		dwc->num_usb2_phy = 1;
+		dwc->num_usb3_phy = 1;
+	} else {
+		for_each_available_child_of_node(ports, port) {
+			ret  = dwc3_count_phys(dwc, port);
+			if (ret == 1) {
+				dwc->num_usb2_phy++;
+			} else if (ret == 2) {
+				dwc->num_usb2_phy++;
+				dwc->num_usb3_phy++;
+			} else {
+				of_node_put(port);
+				return ret;
+			}
+		}
+	}
+	dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_usb2_phy,
+									dwc->num_usb3_phy);
+
+	dwc->usb2_phy = devm_kzalloc(dwc->dev,
+		sizeof(*dwc->usb2_phy) * dwc->num_usb2_phy, GFP_KERNEL);
+	if (!dwc->usb2_phy)
+		return -ENOMEM;
+
+	dwc->usb3_phy = devm_kzalloc(dwc->dev,
+		sizeof(*dwc->usb3_phy) * dwc->num_usb3_phy, GFP_KERNEL);
+	if (!dwc->usb3_phy)
+		return -ENOMEM;
+
+	dwc->usb2_generic_phy = devm_kzalloc(dwc->dev,
+		sizeof(*dwc->usb2_generic_phy) * dwc->num_usb2_phy, GFP_KERNEL);
+	if (!dwc->usb2_generic_phy)
+		return -ENOMEM;
+
+	dwc->usb3_generic_phy = devm_kzalloc(dwc->dev,
+		sizeof(*dwc->usb3_generic_phy) * dwc->num_usb3_phy, GFP_KERNEL);
+	if (!dwc->usb3_generic_phy)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int dwc3_core_get_phy_by_node(struct dwc3 *dwc,
+		struct device_node *lookup_node, int i, int ss_idx)
 {
 	struct device		*dev = dwc->dev;
-	struct device_node	*node = dev->of_node;
-	int ret;
+	int			ret;
 
-	if (node) {
-		dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
-		dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
+	if (lookup_node) {
+		dwc->usb2_phy[i] = devm_of_usb_get_phy_by_phandle(dev,
+								"usb-phy", 0, lookup_node);
 	} else {
-		dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
-		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
+		dwc->usb2_phy[i] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
 	}
 
-	if (IS_ERR(dwc->usb2_phy)) {
-		ret = PTR_ERR(dwc->usb2_phy);
+	if (IS_ERR(dwc->usb2_phy[i])) {
+		ret = PTR_ERR(dwc->usb2_phy[i]);
 		if (ret == -ENXIO || ret == -ENODEV)
-			dwc->usb2_phy = NULL;
+			dwc->usb2_phy[i] = NULL;
 		else
 			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
 	}
 
-	if (IS_ERR(dwc->usb3_phy)) {
-		ret = PTR_ERR(dwc->usb3_phy);
-		if (ret == -ENXIO || ret == -ENODEV)
-			dwc->usb3_phy = NULL;
+	dwc->usb2_generic_phy[i] = devm_of_phy_get(dev, lookup_node, "usb2-phy");
+	if (IS_ERR(dwc->usb2_generic_phy[i])) {
+		ret = PTR_ERR(dwc->usb2_generic_phy[i]);
+		if (ret == -ENODEV)
+			dwc->usb2_generic_phy[i] = NULL;
 		else
-			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
+			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
+	}
+
+	/* If SS-PHY not present in this lookup-node, then return */
+	if (ss_idx == -1)
+		return 0;
+
+	if (lookup_node) {
+		dwc->usb3_phy[ss_idx] = devm_of_usb_get_phy_by_phandle(dev,
+								"usb-phy", 1, lookup_node);
+	} else {
+		dwc->usb3_phy[ss_idx] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
 	}
 
-	dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
-	if (IS_ERR(dwc->usb2_generic_phy)) {
-		ret = PTR_ERR(dwc->usb2_generic_phy);
-		if (ret == -ENOSYS || ret == -ENODEV)
-			dwc->usb2_generic_phy = NULL;
+	if (IS_ERR(dwc->usb3_phy[ss_idx])) {
+		ret = PTR_ERR(dwc->usb3_phy[ss_idx]);
+		if (ret == -ENXIO || ret == -ENODEV)
+			dwc->usb3_phy[ss_idx] = NULL;
 		else
-			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
+			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
 	}
 
-	dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
-	if (IS_ERR(dwc->usb3_generic_phy)) {
-		ret = PTR_ERR(dwc->usb3_generic_phy);
-		if (ret == -ENOSYS || ret == -ENODEV)
-			dwc->usb3_generic_phy = NULL;
+	dwc->usb3_generic_phy[ss_idx] = devm_of_phy_get(dev, lookup_node, "usb3-phy");
+	if (IS_ERR(dwc->usb3_generic_phy[ss_idx])) {
+		ret = PTR_ERR(dwc->usb3_generic_phy[ss_idx]);
+		if (ret == -ENODEV)
+			dwc->usb3_generic_phy[ss_idx] = NULL;
 		else
 			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
 	}
+	return 0;
+}
+
+static int dwc3_core_get_phy(struct dwc3 *dwc)
+{
+	struct device		*dev = dwc->dev;
+	struct device_node	*node = dev->of_node;
+	struct device_node	*ports, *port;
+	int ret, i = 0, j = 0;
+
+	ret = dwc3_extract_num_phys(dwc);
+	if (ret) {
+		dev_err(dwc->dev, "Unable to extract number of PHYs\n");
+		return ret;
+	}
+
+	/* Find if any "multiport" child is present inside DWC3 */
+	for_each_available_child_of_node(node, ports) {
+		if (!strcmp(ports->name, "multiport"))
+			break;
+	}
+
+	if (!ports) {
+		ret = dwc3_core_get_phy_by_node(dwc, node, 0, 0);
+		if (ret)
+			return ret;
+	} else {
+		for_each_available_child_of_node(ports, port) {
+			if (dwc3_count_phys(dwc, port) == 2)
+				ret = dwc3_core_get_phy_by_node(dwc, port, i++, j++);
+			else if (dwc3_count_phys(dwc, port) == 1)
+				ret = dwc3_core_get_phy_by_node(dwc, port, i++, -1);
+			else
+				continue;
+
+			if (ret) {
+				of_node_put(port);
+				return ret;
+			}
+		}
+	}
 
 	return 0;
 }
@@ -1305,16 +1485,16 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
 static int dwc3_core_init_mode(struct dwc3 *dwc)
 {
 	struct device *dev = dwc->dev;
-	int ret;
+	int i, ret;
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
 		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
 
-		if (dwc->usb2_phy)
-			otg_set_vbus(dwc->usb2_phy->otg, false);
-		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
-		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
+		if (dwc->usb2_phy[0])
+			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
+		phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
+		phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);
 
 		ret = dwc3_gadget_init(dwc);
 		if (ret)
@@ -1323,10 +1503,15 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 	case USB_DR_MODE_HOST:
 		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
 
-		if (dwc->usb2_phy)
-			otg_set_vbus(dwc->usb2_phy->otg, true);
-		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
-		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
+		for (i = 0; i < dwc->num_usb3_phy; i++) {
+			if (dwc->usb2_phy[i])
+				otg_set_vbus(dwc->usb2_phy[i]->otg, true);
+			phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
+		}
+
+
+		for (i = 0; i < dwc->num_usb3_phy; i++)
+			phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
 
 		ret = dwc3_host_init(dwc);
 		if (ret)
@@ -1674,7 +1859,7 @@ static int dwc3_probe(struct platform_device *pdev)
 	struct resource		*res, dwc_res;
 	struct dwc3		*dwc;
 
-	int			ret;
+	int			ret, i;
 
 	void __iomem		*regs;
 
@@ -1839,15 +2024,18 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc3_debugfs_exit(dwc);
 	dwc3_event_buffers_cleanup(dwc);
 
-	usb_phy_shutdown(dwc->usb2_phy);
-	usb_phy_shutdown(dwc->usb3_phy);
-	phy_exit(dwc->usb2_generic_phy);
-	phy_exit(dwc->usb3_generic_phy);
-
-	usb_phy_set_suspend(dwc->usb2_phy, 1);
-	usb_phy_set_suspend(dwc->usb3_phy, 1);
-	phy_power_off(dwc->usb2_generic_phy);
-	phy_power_off(dwc->usb3_generic_phy);
+	for (i = 0; i < dwc->num_usb2_phy; i++) {
+		usb_phy_shutdown(dwc->usb2_phy[i]);
+		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
+		phy_exit(dwc->usb2_generic_phy[i]);
+		phy_power_off(dwc->usb2_generic_phy[i]);
+	}
+	for (i = 0; i < dwc->num_usb3_phy; i++) {
+		usb_phy_shutdown(dwc->usb3_phy[i]);
+		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
+		phy_exit(dwc->usb3_generic_phy[i]);
+		phy_power_off(dwc->usb3_generic_phy[i]);
+	}
 
 	dwc3_ulpi_exit(dwc);
 
@@ -1929,6 +2117,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
 
 static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 {
+	int i;
 	unsigned long	flags;
 	u32 reg;
 
@@ -1951,17 +2140,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		/* Let controller to suspend HSPHY before PHY driver suspends */
 		if (dwc->dis_u2_susphy_quirk ||
 		    dwc->dis_enblslpm_quirk) {
-			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-			reg |=  DWC3_GUSB2PHYCFG_ENBLSLPM |
-				DWC3_GUSB2PHYCFG_SUSPHY;
-			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
-
-			/* Give some time for USB2 PHY to suspend */
-			usleep_range(5000, 6000);
+			for (i = 0; i < dwc->num_usb2_phy; i++) {
+				reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
+				reg |=  DWC3_GUSB2PHYCFG_ENBLSLPM |
+					DWC3_GUSB2PHYCFG_SUSPHY;
+				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
+
+				/* Give some time for USB2 PHY to suspend */
+				usleep_range(5000, 6000);
+			}
 		}
 
-		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
-		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
+		for (i = 0; i < dwc->num_usb2_phy; i++)
+			phy_pm_runtime_put_sync(dwc->usb2_generic_phy[i]);
+		for (i = 0; i < dwc->num_usb3_phy; i++)
+			phy_pm_runtime_put_sync(dwc->usb3_generic_phy[i]);
 		break;
 	case DWC3_GCTL_PRTCAP_OTG:
 		/* do nothing during runtime_suspend */
@@ -1989,7 +2182,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 {
 	unsigned long	flags;
-	int		ret;
+	int		i, ret;
 	u32		reg;
 
 	switch (dwc->current_dr_role) {
@@ -2012,17 +2205,21 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 			break;
 		}
 		/* Restore GUSB2PHYCFG bits that were modified in suspend */
-		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-		if (dwc->dis_u2_susphy_quirk)
-			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+		for (i = 0; i < dwc->num_usb2_phy; i++) {
+			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
+			if (dwc->dis_u2_susphy_quirk)
+				reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
 
-		if (dwc->dis_enblslpm_quirk)
-			reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
+			if (dwc->dis_enblslpm_quirk)
+				reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
 
-		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
+		}
 
-		phy_pm_runtime_get_sync(dwc->usb2_generic_phy);
-		phy_pm_runtime_get_sync(dwc->usb3_generic_phy);
+		for (i = 0; i < dwc->num_usb2_phy; i++)
+			phy_pm_runtime_get_sync(dwc->usb2_generic_phy[i]);
+		for (i = 0; i < dwc->num_usb3_phy; i++)
+			phy_pm_runtime_get_sync(dwc->usb3_generic_phy[i]);
 		break;
 	case DWC3_GCTL_PRTCAP_OTG:
 		/* nothing to do on runtime_resume */
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 81c486b..c858689 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1020,6 +1020,8 @@ struct dwc3_scratchpad_array {
  * @usb_psy: pointer to power supply interface.
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
+ * @num_usb2_phy: Number of HS ports controlled by the core
+ * @num_usb3_phy: Number of SS ports controlled by the core
  * @usb2_generic_phy: pointer to USB2 PHY
  * @usb3_generic_phy: pointer to USB3 PHY
  * @phys_ready: flag to indicate that PHYs are ready
@@ -1147,11 +1149,13 @@ struct dwc3 {
 
 	struct reset_control	*reset;
 
-	struct usb_phy		*usb2_phy;
-	struct usb_phy		*usb3_phy;
+	struct usb_phy		**usb2_phy;
+	struct usb_phy		**usb3_phy;
+	u32			num_usb2_phy;
+	u32			num_usb3_phy;
 
-	struct phy		*usb2_generic_phy;
-	struct phy		*usb3_generic_phy;
+	struct phy		**usb2_generic_phy;
+	struct phy		**usb3_generic_phy;
 
 	bool			phys_ready;
 
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 039bf24..404643f 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -384,10 +384,10 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
 		if (ret) {
 			dev_err(dwc->dev, "failed to initialize host\n");
 		} else {
-			if (dwc->usb2_phy)
-				otg_set_vbus(dwc->usb2_phy->otg, true);
-			if (dwc->usb2_generic_phy)
-				phy_set_mode(dwc->usb2_generic_phy,
+			if (dwc->usb2_phy[0])
+				otg_set_vbus(dwc->usb2_phy[0]->otg, true);
+			if (dwc->usb2_generic_phy[0])
+				phy_set_mode(dwc->usb2_generic_phy[0],
 					     PHY_MODE_USB_HOST);
 		}
 		break;
@@ -398,10 +398,10 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
 		dwc3_event_buffers_setup(dwc);
 		spin_unlock_irqrestore(&dwc->lock, flags);
 
-		if (dwc->usb2_phy)
-			otg_set_vbus(dwc->usb2_phy->otg, false);
-		if (dwc->usb2_generic_phy)
-			phy_set_mode(dwc->usb2_generic_phy,
+		if (dwc->usb2_phy[0])
+			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
+		if (dwc->usb2_generic_phy[0])
+			phy_set_mode(dwc->usb2_generic_phy[0],
 				     PHY_MODE_USB_DEVICE);
 		ret = dwc3_gadget_init(dwc);
 		if (ret)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 00427d1..e3b2a17 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2872,8 +2872,8 @@ static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA)
 	union power_supply_propval	val = {0};
 	int				ret;
 
-	if (dwc->usb2_phy)
-		return usb_phy_set_power(dwc->usb2_phy, mA);
+	if (dwc->usb2_phy[0])
+		return usb_phy_set_power(dwc->usb2_phy[0], mA);
 
 	if (!dwc->usb_psy)
 		return -EOPNOTSUPP;
-- 
2.7.4

Re: [PATCH v3 1/3] dt-bindings: usb: dwc3: Add support for multiport related properties

[Rob Herring]

On Wed, 08 Jun 2022 23:06:25 +0530, Harsh Agarwal wrote:
> Added support for multiport, mport, num_usb2_phy and num_usb3_phy
> properties. These properties are used to support devices having
> a multiport controller.
> 
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> ---
>  .../devicetree/bindings/usb/snps,dwc3.yaml         | 53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 

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

yamllint warnings/errors:
./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:367:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:369:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

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

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

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

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 1/3] dt-bindings: usb: dwc3: Add support for multiport related properties

[Rob Herring]

On Wed, Jun 08, 2022 at 11:06:25PM +0530, Harsh Agarwal wrote:
> Added support for multiport, mport, num_usb2_phy and num_usb3_phy
> properties. These properties are used to support devices having
> a multiport controller.
> 
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> ---
>  .../devicetree/bindings/usb/snps,dwc3.yaml         | 53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index d41265b..9332fa2 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -343,6 +343,32 @@ properties:
>        This port is used with the 'usb-role-switch' property  to connect the
>        dwc3 to type C connector.
>  
> +  multiport:

Again, I don't think this is going to play well if you need to describe 
USB devices in your DT. For example, a USB hub with additional DT 
properties.

> +    description:
> +      If a single USB controller supports multiple ports, then it's referred to as
> +      a multiport controller. Each port of the multiport controller can support
> +      either High Speed or Super Speed or both and have their own PHY phandles. Each
> +      port is represented by "mport" node and all the "mport" nodes are grouped
> +      together inside the "multiport" node where individual "mport" node defines the
> +      PHYs supported by that port.
> +
> +  num_usb2_phy:
> +    description: Total number of HS-PHYs defined by the multiport controller.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  num_usb3_phy:
> +    description: Total number of SS-PHYs defined by the multiport controller.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  mport:
> +    description: Each mport node represents one port of the multiport controller.
> +    oneOf:
> +      - required:
> +        - usb-phy

This is deprecated. Why are you adding it?

> +      - required:
> +        - phys
> +        - phy-names

Other multi port USB hosts just have a list of phys. Why can't you just 
use phy-names to identify each phy:

phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs", 
  "port3-hs";

Rob

Re: [PATCH v3 1/3] dt-bindings: usb: dwc3: Add support for multiport related properties

[Harsh Agarwal]

On 6/9/2022 9:08 PM, Rob Herring wrote:
> On Wed, Jun 08, 2022 at 11:06:25PM +0530, Harsh Agarwal wrote:
>> Added support for multiport, mport, num_usb2_phy and num_usb3_phy
>> properties. These properties are used to support devices having
>> a multiport controller.
>>
>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>> ---
>>   .../devicetree/bindings/usb/snps,dwc3.yaml         | 53 ++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> index d41265b..9332fa2 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -343,6 +343,32 @@ properties:
>>         This port is used with the 'usb-role-switch' property  to connect the
>>         dwc3 to type C connector.
>>   
>> +  multiport:
> Again, I don't think this is going to play well if you need to describe
> USB devices in your DT. For example, a USB hub with additional DT
> properties.
Thanks for the review Rob.
Can you please explain why would one want to describe a USB hub in 
device tree ?
IF USB hub is attached to a root port , it would be enumerated by the 
SW. I am not clear how DT is coming
into picture. Even if there was a scenario to add DT properties for a 
hub, then this multiport node would be like a nop
as it just helps us to get the PHY phandles in a proper way.
Do you feel we still might have a problem with multiport node ?
>> +    description:
>> +      If a single USB controller supports multiple ports, then it's referred to as
>> +      a multiport controller. Each port of the multiport controller can support
>> +      either High Speed or Super Speed or both and have their own PHY phandles. Each
>> +      port is represented by "mport" node and all the "mport" nodes are grouped
>> +      together inside the "multiport" node where individual "mport" node defines the
>> +      PHYs supported by that port.
>> +
>> +  num_usb2_phy:
>> +    description: Total number of HS-PHYs defined by the multiport controller.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  num_usb3_phy:
>> +    description: Total number of SS-PHYs defined by the multiport controller.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  mport:
>> +    description: Each mport node represents one port of the multiport controller.
>> +    oneOf:
>> +      - required:
>> +        - usb-phy
> This is deprecated. Why are you adding it?
Do you mean "usb-phy" is deprecated ?
Internally we use usb-phy with our downstream GLUE driver
>
>> +      - required:
>> +        - phys
>> +        - phy-names
> Other multi port USB hosts just have a list of phys. Why can't you just
> use phy-names to identify each phy:
>
> phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs",
>    "port3-hs";
With the above method we would have to do some kind of string parsing on 
the phy-names to get the HS and SS PHYs as we need to cater to different 
combinations of Ports ( some support HS+SS , other supports SS only).
So one challenge here is with the "usb-phy". There we directly define 
the phy phandles and that might/might-not have proper sub-strings. eg 
USB_QMP_PHY . So extracting PHYS could be tricky if the phy-handle does 
not have proper substring like "SS" "HS" etc.
We cannot break existing implementation and so we thought of going with 
the "multiport" node approach, listing below some flexibility :

1. Better representation of the PHYs and it's relation with a port.
2. Here for each port we pick the first PHY as HS and 2nd PHY as SS as 
we have been doing traditionally.
So for "usb-phy" we need not care how the PHY handles are named.

3. It's future proof incase we need to add additional properties 
specific to a port. We can just add those properties inside MP_1 or MP_2 
etc.
Though nothing like this has yet been implemented.

Also agree that there are multiple ways to approach this problem and 
that's why we had RFC tag earlier to get feedback on the same.
> Rob

Re: [PATCH v3 1/3] dt-bindings: usb: dwc3: Add support for multiport related properties

[Rob Herring]

On Fri, Jun 10, 2022 at 05:25:25PM +0530, Harsh Agarwal wrote:
> 
> On 6/9/2022 9:08 PM, Rob Herring wrote:
> > On Wed, Jun 08, 2022 at 11:06:25PM +0530, Harsh Agarwal wrote:
> > > Added support for multiport, mport, num_usb2_phy and num_usb3_phy
> > > properties. These properties are used to support devices having
> > > a multiport controller.
> > > 
> > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > ---
> > >   .../devicetree/bindings/usb/snps,dwc3.yaml         | 53 ++++++++++++++++++++++
> > >   1 file changed, 53 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > index d41265b..9332fa2 100644
> > > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > @@ -343,6 +343,32 @@ properties:
> > >         This port is used with the 'usb-role-switch' property  to connect the
> > >         dwc3 to type C connector.
> > > +  multiport:
> > Again, I don't think this is going to play well if you need to describe
> > USB devices in your DT. For example, a USB hub with additional DT
> > properties.
> Thanks for the review Rob.
> Can you please explain why would one want to describe a USB hub in device
> tree ?

Because someone soldered a hub on the board and then connected extra 
things like resets, GPIOs, supplies which are all outside of standard 
USB. It's quite common...

There's some flavors of Beagle boards that have a USB ethernet on board. 
Guess what, they skipped out on a eeprom and so the device and a MAC 
address has to be described in DT (if you want a stable MAC addr).

> IF USB hub is attached to a root port , it would be enumerated by the SW. I
> am not clear how DT is coming
> into picture. Even if there was a scenario to add DT properties for a hub,
> then this multiport node would be like a nop
> as it just helps us to get the PHY phandles in a proper way.

It won't be enumerated by the SW if it has to be powered on first using 
non-standard resources.

> Do you feel we still might have a problem with multiport node ?

A board design could have a hub or device on any or all your ports.

> > > +    description:
> > > +      If a single USB controller supports multiple ports, then it's referred to as
> > > +      a multiport controller. Each port of the multiport controller can support
> > > +      either High Speed or Super Speed or both and have their own PHY phandles. Each
> > > +      port is represented by "mport" node and all the "mport" nodes are grouped
> > > +      together inside the "multiport" node where individual "mport" node defines the
> > > +      PHYs supported by that port.
> > > +
> > > +  num_usb2_phy:
> > > +    description: Total number of HS-PHYs defined by the multiport controller.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  num_usb3_phy:
> > > +    description: Total number of SS-PHYs defined by the multiport controller.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  mport:
> > > +    description: Each mport node represents one port of the multiport controller.
> > > +    oneOf:
> > > +      - required:
> > > +        - usb-phy
> > This is deprecated. Why are you adding it?
> Do you mean "usb-phy" is deprecated ?

It is replaced by 'phys'. Any new user should use 'phys'.

> Internally we use usb-phy with our downstream GLUE driver

Upstream does not care about that.

> > 
> > > +      - required:
> > > +        - phys
> > > +        - phy-names
> > Other multi port USB hosts just have a list of phys. Why can't you just
> > use phy-names to identify each phy:
> > 
> > phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs",
> >    "port3-hs";
> With the above method we would have to do some kind of string parsing on the
> phy-names to get the HS and SS PHYs as we need to cater to different
> combinations of Ports ( some support HS+SS , other supports SS only).

You are doing string parsing anyways to get the child nodes and 
properties.

> So one challenge here is with the "usb-phy". There we directly define the
> phy phandles and that might/might-not have proper sub-strings. eg
> USB_QMP_PHY . So extracting PHYS could be tricky if the phy-handle does not
> have proper substring like "SS" "HS" etc.

The schema can and should enforce that you have the proper strings.

> We cannot break existing implementation and so we thought of going with the
> "multiport" node approach, listing below some flexibility :

How would this break?

> 1. Better representation of the PHYs and it's relation with a port.
> 2. Here for each port we pick the first PHY as HS and 2nd PHY as SS as we
> have been doing traditionally.
> So for "usb-phy" we need not care how the PHY handles are named.
> 
> 3. It's future proof incase we need to add additional properties specific to
> a port. We can just add those properties inside MP_1 or MP_2 etc.
> Though nothing like this has yet been implemented.

Then you have to consider how the standard USB device binding fits into 
this, and it needs to work for anyone with multiple ports. The 
usb-hcd.yaml schema already defines that child nodes represent a USB 
device attached to a port on the host. 'reg' is the port number.

Rob

Re: [PATCH v3 1/3] dt-bindings: usb: dwc3: Add support for multiport related properties

[Harsh Agarwal]

On 6/10/2022 10:52 PM, Rob Herring wrote:
> On Fri, Jun 10, 2022 at 05:25:25PM +0530, Harsh Agarwal wrote:
>> On 6/9/2022 9:08 PM, Rob Herring wrote:
>>> On Wed, Jun 08, 2022 at 11:06:25PM +0530, Harsh Agarwal wrote:
>>>> Added support for multiport, mport, num_usb2_phy and num_usb3_phy
>>>> properties. These properties are used to support devices having
>>>> a multiport controller.
>>>>
>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>> ---
>>>>    .../devicetree/bindings/usb/snps,dwc3.yaml         | 53 ++++++++++++++++++++++
>>>>    1 file changed, 53 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>> index d41265b..9332fa2 100644
>>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>> @@ -343,6 +343,32 @@ properties:
>>>>          This port is used with the 'usb-role-switch' property  to connect the
>>>>          dwc3 to type C connector.
>>>> +  multiport:
>>> Again, I don't think this is going to play well if you need to describe
>>> USB devices in your DT. For example, a USB hub with additional DT
>>> properties.
>> Thanks for the review Rob.
>> Can you please explain why would one want to describe a USB hub in device
>> tree ?
> Because someone soldered a hub on the board and then connected extra
> things like resets, GPIOs, supplies which are all outside of standard
> USB. It's quite common...
>
> There's some flavors of Beagle boards that have a USB ethernet on board.
> Guess what, they skipped out on a eeprom and so the device and a MAC
> address has to be described in DT (if you want a stable MAC addr).
>
>> IF USB hub is attached to a root port , it would be enumerated by the SW. I
>> am not clear how DT is coming
>> into picture. Even if there was a scenario to add DT properties for a hub,
>> then this multiport node would be like a nop
>> as it just helps us to get the PHY phandles in a proper way.
> It won't be enumerated by the SW if it has to be powered on first using
> non-standard resources.
>
>> Do you feel we still might have a problem with multiport node ?
> A board design could have a hub or device on any or all your ports.
>
>>>> +    description:
>>>> +      If a single USB controller supports multiple ports, then it's referred to as
>>>> +      a multiport controller. Each port of the multiport controller can support
>>>> +      either High Speed or Super Speed or both and have their own PHY phandles. Each
>>>> +      port is represented by "mport" node and all the "mport" nodes are grouped
>>>> +      together inside the "multiport" node where individual "mport" node defines the
>>>> +      PHYs supported by that port.
>>>> +
>>>> +  num_usb2_phy:
>>>> +    description: Total number of HS-PHYs defined by the multiport controller.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +
>>>> +  num_usb3_phy:
>>>> +    description: Total number of SS-PHYs defined by the multiport controller.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +
>>>> +  mport:
>>>> +    description: Each mport node represents one port of the multiport controller.
>>>> +    oneOf:
>>>> +      - required:
>>>> +        - usb-phy
>>> This is deprecated. Why are you adding it?
>> Do you mean "usb-phy" is deprecated ?
> It is replaced by 'phys'. Any new user should use 'phys'.
>
>> Internally we use usb-phy with our downstream GLUE driver
> Upstream does not care about that.
>
>>>> +      - required:
>>>> +        - phys
>>>> +        - phy-names
>>> Other multi port USB hosts just have a list of phys. Why can't you just
>>> use phy-names to identify each phy:
>>>
>>> phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs",
>>>     "port3-hs";
>> With the above method we would have to do some kind of string parsing on the
>> phy-names to get the HS and SS PHYs as we need to cater to different
>> combinations of Ports ( some support HS+SS , other supports SS only).
> You are doing string parsing anyways to get the child nodes and
> properties.
>
>> So one challenge here is with the "usb-phy". There we directly define the
>> phy phandles and that might/might-not have proper sub-strings. eg
>> USB_QMP_PHY . So extracting PHYS could be tricky if the phy-handle does not
>> have proper substring like "SS" "HS" etc.
> The schema can and should enforce that you have the proper strings.
Hi Rob,
Apologies for replying late.

I get your concern. Yes we can remove the "multiport" node and instead 
define the
USB phy phandles all in one place. Still I would need to add support for 
both generic-phy and
usb-phy framework as downstream many vendors are using "usb-phy" and 
it's supported by ACK as well.
This would not regress anything with Generic PHY.

@Greg can you please comment as ACK has support for usb-phy framework.

Now coming to implementation, let's consider a 4 port USB multiport 
controller having
4 HS PHYs and 2 SS PHYs.  We can have two approaches here

#1 -> If we could mandate using "HS" or "SS" as substring in
phy-names or usb-phy, then we can calculate number of HS and SS phy and 
also get
corresponding PHY nodes. Only concern here is that downstream vendors 
might need
to change their existing usb-phy names and add proper substring if they 
are not doing so ;

phy = <&usb-hs-phy>,<&usb-ss-phy>,
       <&usb-hs-phy1>, <&usb-ss-phy1>,
       <&usb-hs-phy2>, <&usb-hs-phy3>;

phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs",
    "port3-hs";


OR


#2-> We could mandate defining the USB phy in HS - SS pairs.
For ports that has only HS PHY, we would need to define usb_nop_phy in 
SS place.
Then we can calculate the number of HS & SS phys and get corresponding
PHY nodes by using simple fact that HS phy would be defined at odd places &
SS phy defined at even. Here substrings are not mandated.

phy = <&usb-hs-phy>,<&usb-qmp-phy>,
       <&usb-hs-phy1>, <&usb-qmp-phy1>,
       <&usb-hs-phy2>, <&usb_nop_phy>
       <&usb-hs-phy3>, <&usb_nop_phy>;

phy-names = "port0-hs", "port0-ss",
	    "port1-hs", "port1-ss",
	    "port2-hs", "usb-nop",
	    "port3-hs", "usb-nop";


Please let me know if you prefer any approach here or have any suggestions.

>   
>
>> We cannot break existing implementation and so we thought of going with the
>> "multiport" node approach, listing below some flexibility :
> How would this break?
>
>> 1. Better representation of the PHYs and it's relation with a port.
>> 2. Here for each port we pick the first PHY as HS and 2nd PHY as SS as we
>> have been doing traditionally.
>> So for "usb-phy" we need not care how the PHY handles are named.
>>
>> 3. It's future proof incase we need to add additional properties specific to
>> a port. We can just add those properties inside MP_1 or MP_2 etc.
>> Though nothing like this has yet been implemented.
> Then you have to consider how the standard USB device binding fits into
> this, and it needs to work for anyone with multiple ports. The
> usb-hcd.yaml schema already defines that child nodes represent a USB
> device attached to a port on the host. 'reg' is the port number.
>
> Rob

Re: [PATCH v3 1/3] dt-bindings: usb: dwc3: Add support for multiport related properties

[Rob Herring]

On Mon, Jun 27, 2022 at 06:36:53PM +0530, Harsh Agarwal wrote:
> 
> On 6/10/2022 10:52 PM, Rob Herring wrote:
> > On Fri, Jun 10, 2022 at 05:25:25PM +0530, Harsh Agarwal wrote:
> > > On 6/9/2022 9:08 PM, Rob Herring wrote:
> > > > On Wed, Jun 08, 2022 at 11:06:25PM +0530, Harsh Agarwal wrote:
> > > > > Added support for multiport, mport, num_usb2_phy and num_usb3_phy
> > > > > properties. These properties are used to support devices having
> > > > > a multiport controller.
> > > > > 
> > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > > > ---
> > > > >    .../devicetree/bindings/usb/snps,dwc3.yaml         | 53 ++++++++++++++++++++++
> > > > >    1 file changed, 53 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > > > index d41265b..9332fa2 100644
> > > > > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > > > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > > > @@ -343,6 +343,32 @@ properties:
> > > > >          This port is used with the 'usb-role-switch' property  to connect the
> > > > >          dwc3 to type C connector.
> > > > > +  multiport:
> > > > Again, I don't think this is going to play well if you need to describe
> > > > USB devices in your DT. For example, a USB hub with additional DT
> > > > properties.
> > > Thanks for the review Rob.
> > > Can you please explain why would one want to describe a USB hub in device
> > > tree ?
> > Because someone soldered a hub on the board and then connected extra
> > things like resets, GPIOs, supplies which are all outside of standard
> > USB. It's quite common...
> > 
> > There's some flavors of Beagle boards that have a USB ethernet on board.
> > Guess what, they skipped out on a eeprom and so the device and a MAC
> > address has to be described in DT (if you want a stable MAC addr).
> > 
> > > IF USB hub is attached to a root port , it would be enumerated by the SW. I
> > > am not clear how DT is coming
> > > into picture. Even if there was a scenario to add DT properties for a hub,
> > > then this multiport node would be like a nop
> > > as it just helps us to get the PHY phandles in a proper way.
> > It won't be enumerated by the SW if it has to be powered on first using
> > non-standard resources.
> > 
> > > Do you feel we still might have a problem with multiport node ?
> > A board design could have a hub or device on any or all your ports.
> > 
> > > > > +    description:
> > > > > +      If a single USB controller supports multiple ports, then it's referred to as
> > > > > +      a multiport controller. Each port of the multiport controller can support
> > > > > +      either High Speed or Super Speed or both and have their own PHY phandles. Each
> > > > > +      port is represented by "mport" node and all the "mport" nodes are grouped
> > > > > +      together inside the "multiport" node where individual "mport" node defines the
> > > > > +      PHYs supported by that port.
> > > > > +
> > > > > +  num_usb2_phy:
> > > > > +    description: Total number of HS-PHYs defined by the multiport controller.
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +
> > > > > +  num_usb3_phy:
> > > > > +    description: Total number of SS-PHYs defined by the multiport controller.
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +
> > > > > +  mport:
> > > > > +    description: Each mport node represents one port of the multiport controller.
> > > > > +    oneOf:
> > > > > +      - required:
> > > > > +        - usb-phy
> > > > This is deprecated. Why are you adding it?
> > > Do you mean "usb-phy" is deprecated ?
> > It is replaced by 'phys'. Any new user should use 'phys'.
> > 
> > > Internally we use usb-phy with our downstream GLUE driver
> > Upstream does not care about that.
> > 
> > > > > +      - required:
> > > > > +        - phys
> > > > > +        - phy-names
> > > > Other multi port USB hosts just have a list of phys. Why can't you just
> > > > use phy-names to identify each phy:
> > > > 
> > > > phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs",
> > > >     "port3-hs";
> > > With the above method we would have to do some kind of string parsing on the
> > > phy-names to get the HS and SS PHYs as we need to cater to different
> > > combinations of Ports ( some support HS+SS , other supports SS only).
> > You are doing string parsing anyways to get the child nodes and
> > properties.
> > 
> > > So one challenge here is with the "usb-phy". There we directly define the
> > > phy phandles and that might/might-not have proper sub-strings. eg
> > > USB_QMP_PHY . So extracting PHYS could be tricky if the phy-handle does not
> > > have proper substring like "SS" "HS" etc.
> > The schema can and should enforce that you have the proper strings.
> Hi Rob,
> Apologies for replying late.
> 
> I get your concern. Yes we can remove the "multiport" node and instead
> define the
> USB phy phandles all in one place. Still I would need to add support for
> both generic-phy and
> usb-phy framework as downstream many vendors are using "usb-phy" and it's
> supported by ACK as well.

Upstream is not concerned with downstream. The generic PHY has replaced 
usb-phy for many years now.

Furthermore, if downstream was using documented bindings, then we 
wouldn't be having this conversation.

> This would not regress anything with Generic PHY.
> 
> @Greg can you please comment as ACK has support for usb-phy framework.
> 
> Now coming to implementation, let's consider a 4 port USB multiport
> controller having
> 4 HS PHYs and 2 SS PHYs.  We can have two approaches here
> 
> #1 -> If we could mandate using "HS" or "SS" as substring in
> phy-names or usb-phy, then we can calculate number of HS and SS phy and also
> get
> corresponding PHY nodes. Only concern here is that downstream vendors might
> need
> to change their existing usb-phy names and add proper substring if they are
> not doing so ;
> 
> phy = <&usb-hs-phy>,<&usb-ss-phy>,
>       <&usb-hs-phy1>, <&usb-ss-phy1>,
>       <&usb-hs-phy2>, <&usb-hs-phy3>;
> 
> phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs",
>    "port3-hs";
> 
> 
> OR
> 
> 
> #2-> We could mandate defining the USB phy in HS - SS pairs.
> For ports that has only HS PHY, we would need to define usb_nop_phy in SS
> place.
> Then we can calculate the number of HS & SS phys and get corresponding
> PHY nodes by using simple fact that HS phy would be defined at odd places &
> SS phy defined at even. Here substrings are not mandated.
> 
> phy = <&usb-hs-phy>,<&usb-qmp-phy>,
>       <&usb-hs-phy1>, <&usb-qmp-phy1>,
>       <&usb-hs-phy2>, <&usb_nop_phy>
>       <&usb-hs-phy3>, <&usb_nop_phy>;
> 
> phy-names = "port0-hs", "port0-ss",
> 	    "port1-hs", "port1-ss",
> 	    "port2-hs", "usb-nop",
> 	    "port3-hs", "usb-nop";

The whole reason for -names is to avoid something like this with filler 
entries. So I prefer #1 as I suggested.

Rob

Re: [PATCH v3 1/3] dt-bindings: usb: dwc3: Add support for multiport related properties

[Krishna Kurapati PSSNV]

Attempt to revive this thread.

Hi Rob,

   Apologies for the delay in pursuing this thread further. Neither 
Harsh nor me were able to pursue this since July because of some other 
work load.

On 7/7/2022 3:39 AM, Rob Herring wrote:
> On Mon, Jun 27, 2022 at 06:36:53PM +0530, Harsh Agarwal wrote:
>>
>> On 6/10/2022 10:52 PM, Rob Herring wrote:
>>> On Fri, Jun 10, 2022 at 05:25:25PM +0530, Harsh Agarwal wrote:
>>>> On 6/9/2022 9:08 PM, Rob Herring wrote:
>>>>> On Wed, Jun 08, 2022 at 11:06:25PM +0530, Harsh Agarwal wrote:
>>>>>> Added support for multiport, mport, num_usb2_phy and num_usb3_phy
>>>>>> properties. These properties are used to support devices having
>>>>>> a multiport controller.
>>>>>>
>>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>>> ---
>>>>>>     .../devicetree/bindings/usb/snps,dwc3.yaml         | 53 ++++++++++++++++++++++
>>>>>>     1 file changed, 53 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>>>> index d41265b..9332fa2 100644
>>>>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>>>> @@ -343,6 +343,32 @@ properties:
>>>>>>           This port is used with the 'usb-role-switch' property  to connect the
>>>>>>           dwc3 to type C connector.
>>>>>> +  multiport:
>>>>> Again, I don't think this is going to play well if you need to describe
>>>>> USB devices in your DT. For example, a USB hub with additional DT
>>>>> properties.
>>>> Thanks for the review Rob.
>>>> Can you please explain why would one want to describe a USB hub in device
>>>> tree ?
>>> Because someone soldered a hub on the board and then connected extra
>>> things like resets, GPIOs, supplies which are all outside of standard
>>> USB. It's quite common...
>>>
>>> There's some flavors of Beagle boards that have a USB ethernet on board.
>>> Guess what, they skipped out on a eeprom and so the device and a MAC
>>> address has to be described in DT (if you want a stable MAC addr).
>>>
>>>> IF USB hub is attached to a root port , it would be enumerated by the SW. I
>>>> am not clear how DT is coming
>>>> into picture. Even if there was a scenario to add DT properties for a hub,
>>>> then this multiport node would be like a nop
>>>> as it just helps us to get the PHY phandles in a proper way.
>>> It won't be enumerated by the SW if it has to be powered on first using
>>> non-standard resources.
>>>
>>>> Do you feel we still might have a problem with multiport node ?
>>> A board design could have a hub or device on any or all your ports.
>>>
>>>>>> +    description:
>>>>>> +      If a single USB controller supports multiple ports, then it's referred to as
>>>>>> +      a multiport controller. Each port of the multiport controller can support
>>>>>> +      either High Speed or Super Speed or both and have their own PHY phandles. Each
>>>>>> +      port is represented by "mport" node and all the "mport" nodes are grouped
>>>>>> +      together inside the "multiport" node where individual "mport" node defines the
>>>>>> +      PHYs supported by that port.
>>>>>> +
>>>>>> +  num_usb2_phy:
>>>>>> +    description: Total number of HS-PHYs defined by the multiport controller.
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +
>>>>>> +  num_usb3_phy:
>>>>>> +    description: Total number of SS-PHYs defined by the multiport controller.
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +
>>>>>> +  mport:
>>>>>> +    description: Each mport node represents one port of the multiport controller.
>>>>>> +    oneOf:
>>>>>> +      - required:
>>>>>> +        - usb-phy
>>>>> This is deprecated. Why are you adding it?
>>>> Do you mean "usb-phy" is deprecated ?
>>> It is replaced by 'phys'. Any new user should use 'phys'.
>>>
>>>> Internally we use usb-phy with our downstream GLUE driver
>>> Upstream does not care about that.
>>>
>>>>>> +      - required:
>>>>>> +        - phys
>>>>>> +        - phy-names
>>>>> Other multi port USB hosts just have a list of phys. Why can't you just
>>>>> use phy-names to identify each phy:
>>>>>
>>>>> phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs",
>>>>>      "port3-hs";
>>>> With the above method we would have to do some kind of string parsing on the
>>>> phy-names to get the HS and SS PHYs as we need to cater to different
>>>> combinations of Ports ( some support HS+SS , other supports SS only).
>>> You are doing string parsing anyways to get the child nodes and
>>> properties.
>>>
>>>> So one challenge here is with the "usb-phy". There we directly define the
>>>> phy phandles and that might/might-not have proper sub-strings. eg
>>>> USB_QMP_PHY . So extracting PHYS could be tricky if the phy-handle does not
>>>> have proper substring like "SS" "HS" etc.
>>> The schema can and should enforce that you have the proper strings.
>> Hi Rob,
>> Apologies for replying late.
>>
>> I get your concern. Yes we can remove the "multiport" node and instead
>> define the
>> USB phy phandles all in one place. Still I would need to add support for
>> both generic-phy and
>> usb-phy framework as downstream many vendors are using "usb-phy" and it's
>> supported by ACK as well.
> 
> Upstream is not concerned with downstream. The generic PHY has replaced
> usb-phy for many years now.
> 
> Furthermore, if downstream was using documented bindings, then we
> wouldn't be having this conversation.
> 

If we are concentrating only on generic phy's do we need to refrain from 
making any changes to downstream phy part of the driver code in core.c 
as done in this RFC series ? I wanted to update this series after 
addressing review comments. If we don't need to, then I can revert 
changes done to dwc->usb2->phy and dwc->usb3_phy in this series.

>> This would not regress anything with Generic PHY.
>>
>> @Greg can you please comment as ACK has support for usb-phy framework.
>>
>> Now coming to implementation, let's consider a 4 port USB multiport
>> controller having
>> 4 HS PHYs and 2 SS PHYs.  We can have two approaches here
>>
>> #1 -> If we could mandate using "HS" or "SS" as substring in
>> phy-names or usb-phy, then we can calculate number of HS and SS phy and also
>> get
>> corresponding PHY nodes. Only concern here is that downstream vendors might
>> need
>> to change their existing usb-phy names and add proper substring if they are
>> not doing so ;
>>
>> phy = <&usb-hs-phy>,<&usb-ss-phy>,
>>        <&usb-hs-phy1>, <&usb-ss-phy1>,
>>        <&usb-hs-phy2>, <&usb-hs-phy3>;
>>
>> phy-names = "port0-hs", "port0-ss", "port1-hs", "port1-ss", "port2-hs",
>>     "port3-hs";
>>
>>
>> OR
>>
>>
>> #2-> We could mandate defining the USB phy in HS - SS pairs.
>> For ports that has only HS PHY, we would need to define usb_nop_phy in SS
>> place.
>> Then we can calculate the number of HS & SS phys and get corresponding
>> PHY nodes by using simple fact that HS phy would be defined at odd places &
>> SS phy defined at even. Here substrings are not mandated.
>>
>> phy = <&usb-hs-phy>,<&usb-qmp-phy>,
>>        <&usb-hs-phy1>, <&usb-qmp-phy1>,
>>        <&usb-hs-phy2>, <&usb_nop_phy>
>>        <&usb-hs-phy3>, <&usb_nop_phy>;
>>
>> phy-names = "port0-hs", "port0-ss",
>> 	    "port1-hs", "port1-ss",
>> 	    "port2-hs", "usb-nop",
>> 	    "port3-hs", "usb-nop";
> 
> The whole reason for -names is to avoid something like this with filler
> entries. So I prefer #1 as I suggested.
> 
Thanks for the review. How about we do the following:
Assuming we have 3 ports where first port is HS+SS capable and the other 
two are only HS capable. We can implement phys and phy-names as :

phy = <&usb-hs-phy1>,<&usb-ss-phy1>,
	<&usb-hs-phy2>, <&usb-hs-phy3>,

phy-names = "usb2-phy-port0", "usb3-phy-port0",
		"usb2-phy-port1", "usb2-phy-port2";

Since the driver code mandates that the phy-names are supposed to be 
"usb2-phy" and "usb3-phy", we can be sure that the first part of every 
phy name starts with "usb2-phy" or "usb3-phy" and we can append -portX 
at end of each phy name to differentiate them as shown in the above example.

In the driver code we can iterate over each of the phy-names property 
and string compare them with "usb2-phy" and "usb3-phy" to identify 
whether it is HS/SS. That way even if we have only one-port the code 
would still hold good.

Let me know if that approach would be fine.

Once again, sorry for delaying this thread.

Regards,
Krishna,

> Rob