[RFC 0/2] Add support for multiport controller

[RFC 0/2] 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 
patches have gone through basic sanity only.

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" defines the PHYs that it 
supports.

Looking for comments/feedback on the device tree bindings. Once the 
bindings are locked, we can further factor the code.

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

multiport {
	mp_1: mport@1 {
		usb-phys = <usb2_phy0>, <usb3_phy0>;
	};	
	mp_2: mport@2 {
		usb-phys = <usb2_phy1>, <usb3_phy1>;
	};	
};

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

 .../devicetree/bindings/usb/snps,dwc3.yaml         |  55 +++++
 drivers/usb/dwc3/core.c                            | 259 ++++++++++++++++-----
 drivers/usb/dwc3/core.h                            |   8 +-
 drivers/usb/dwc3/drd.c                             |   8 +-
 drivers/usb/dwc3/gadget.c                          |   4 +-
 5 files changed, 264 insertions(+), 70 deletions(-)

-- 
2.7.4

[RFC 1/2] dt-bindings: usb: dwc3: Add support for multiport related properties

[Harsh Agarwal]

Added support for multiport, mport, num-ssphy and num-hsphy
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         | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index f4471f8..39c61483 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -341,6 +341,35 @@ 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.
+    required:
+      - mport
+
+  num-hsphy:
+    description: Total number of HS-PHYs defined by the multiport controller.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  num-ssphy:
+    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.
+    patternProperties: "^mport@[0-9a-f]+$"
+    oneOf:
+       - required:
+         - usb-phy
+       - required:
+          - phys
+          - phy-names
+
 unevaluatedProperties: false
 
 required:
@@ -369,4 +398,30 @@ 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: mport@1 {
+          usb-phy = <&usb2_phy0>, <&usb3_phy0>;
+	};
+
+	MP_2: mport@2 {
+          usb-phy = <&usb2_phy1>, <&usb3_phy1>;
+	};
+
+	MP_3: mport@3 {
+          usb-phy = <&usb2_phy2>, <&usb_nop_phy>;
+	};
+
+	MP_4: mport@4 {
+          usb-phy = <&usb2_phy3>, <&usb_nop_phy>;
+	};
+
+      };
+    };
 ...
-- 
2.7.4

[RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller

[Harsh Agarwal]

Currently the USB driver supports only single port controller
which works with 2 PHYs at max ie HS and SS.

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

This implementation is compatible with existing glue drivers.

Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
---
 drivers/usb/dwc3/core.c   | 259 +++++++++++++++++++++++++++++++++++-----------
 drivers/usb/dwc3/core.h   |   8 +-
 drivers/usb/dwc3/drd.c    |   8 +-
 drivers/usb/dwc3/gadget.c |   4 +-
 4 files changed, 209 insertions(+), 70 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 2682469..8eb6b5b6 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -189,8 +189,8 @@ 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);
+			if (dwc->usb2_phy[0])
+				otg_set_vbus(dwc->usb2_phy[0]->otg, true);
 			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
 			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
 			if (dwc->dis_split_quirk) {
@@ -204,9 +204,13 @@ static void __dwc3_set_mode(struct work_struct *work)
 		dwc3_core_soft_reset(dwc);
 
 		dwc3_event_buffers_setup(dwc);
-
-		if (dwc->usb2_phy)
-			otg_set_vbus(dwc->usb2_phy->otg, false);
+		/*
+		 * Multiport Controller works only in host mode.
+		 * There will only be one pair of HS and SS PHY for the controller operating in
+		 * device mode.
+		 */
+		if (dwc->usb2_phy[0])
+			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
 		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
 		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
 
@@ -825,15 +829,20 @@ 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);
+	for (i = 0; i < dwc->num_hsphy; i++)
+		usb_phy_shutdown(dwc->usb2_phy[i]);
+	for (i = 0; i < dwc->num_ssphy; i++)
+		usb_phy_shutdown(dwc->usb3_phy[i]);
 	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);
+	for (i = 0; i < dwc->num_hsphy; i++)
+		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
+	for (i = 0; i < dwc->num_ssphy; i++)
+		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
 	phy_power_off(dwc->usb2_generic_phy);
 	phy_power_off(dwc->usb3_generic_phy);
 	dwc3_clk_disable(dwc);
@@ -1038,7 +1047,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
 {
 	unsigned int		hw_mode;
 	u32			reg;
-	int			ret;
+	int			ret, i;
 
 	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
 
@@ -1066,8 +1075,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		dwc->phys_ready = true;
 	}
 
-	usb_phy_init(dwc->usb2_phy);
-	usb_phy_init(dwc->usb3_phy);
+	for (i = 0; i < dwc->num_hsphy; i++)
+		usb_phy_init(dwc->usb2_phy[i]);
+	for (i = 0; i < dwc->num_ssphy; i++)
+		usb_phy_init(dwc->usb3_phy[i]);
 	ret = phy_init(dwc->usb2_generic_phy);
 	if (ret < 0)
 		goto err0a;
@@ -1112,8 +1123,10 @@ 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);
+	for (i = 0; i < dwc->num_hsphy; i++)
+		usb_phy_set_suspend(dwc->usb2_phy[i], 0);
+	for (i = 0; i < dwc->num_ssphy; i++)
+		usb_phy_set_suspend(dwc->usb3_phy[i], 0);
 	ret = phy_power_on(dwc->usb2_generic_phy);
 	if (ret < 0)
 		goto err2;
@@ -1234,12 +1247,16 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	phy_power_off(dwc->usb2_generic_phy);
 
 err2:
-	usb_phy_set_suspend(dwc->usb2_phy, 1);
-	usb_phy_set_suspend(dwc->usb3_phy, 1);
+	for (i = 0; i < dwc->num_hsphy; i++)
+		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
+	for (i = 0; i < dwc->num_ssphy; i++)
+		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
 
 err1:
-	usb_phy_shutdown(dwc->usb2_phy);
-	usb_phy_shutdown(dwc->usb3_phy);
+	for (i = 0; i < dwc->num_hsphy; i++)
+		usb_phy_shutdown(dwc->usb2_phy[i]);
+	for (i = 0; i < dwc->num_ssphy; i++)
+		usb_phy_shutdown(dwc->usb3_phy[i]);
 	phy_exit(dwc->usb2_generic_phy);
 	phy_exit(dwc->usb3_generic_phy);
 
@@ -1250,52 +1267,166 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	return ret;
 }
 
+static struct usb_phy *dwc3_core_get_phy_by_handle_with_node(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_err(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;
+}
+
+static int dwc3_extract_num_phys(struct dwc3 *dwc)
+{
+	struct device_node	*ports, *port;
+
+	/* 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_hsphy = 1;
+		dwc->num_ssphy = 1;
+	} else {
+		for_each_available_child_of_node(ports, port) {
+			dwc->num_hsphy += 1;
+			dwc->num_ssphy += 1;
+		}
+	}
+	dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_hsphy, dwc->num_ssphy);
+
+	dwc->usb2_phy = devm_kzalloc(dwc->dev,
+		sizeof(*dwc->usb2_phy) * dwc->num_hsphy, GFP_KERNEL);
+	if (!dwc->usb2_phy)
+		return -ENOMEM;
+
+	dwc->usb3_phy = devm_kzalloc(dwc->dev,
+		sizeof(*dwc->usb3_phy) * dwc->num_ssphy, GFP_KERNEL);
+	if (!dwc->usb3_phy)
+		return -ENOMEM;
+
+	return 0;
+}
+
 static int dwc3_core_get_phy(struct dwc3 *dwc)
 {
 	struct device		*dev = dwc->dev;
 	struct device_node	*node = dev->of_node;
-	int ret;
+	struct device_node	*ports, *port;
 
-	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);
-	} 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);
-	}
+	int ret, i = 0;
 
-	if (IS_ERR(dwc->usb2_phy)) {
-		ret = PTR_ERR(dwc->usb2_phy);
-		if (ret == -ENXIO || ret == -ENODEV)
-			dwc->usb2_phy = NULL;
-		else
-			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
+	ret = dwc3_extract_num_phys(dwc);
+	if (ret) {
+		dev_err(dwc->dev, "Unable to extract number of PHYs\n");
+		return ret;
 	}
 
-	if (IS_ERR(dwc->usb3_phy)) {
-		ret = PTR_ERR(dwc->usb3_phy);
-		if (ret == -ENXIO || ret == -ENODEV)
-			dwc->usb3_phy = NULL;
-		else
-			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
+	/* Find if any "multiport" child is present inside DWC3*/
+	for_each_available_child_of_node(node, ports) {
+		if (!strcmp(ports->name, "multiport"))
+			break;
 	}
 
-	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;
-		else
-			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
-	}
+	if (!ports) {
+		if (node) {
+			dwc->usb2_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
+			dwc->usb3_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
+		} else {
+			dwc->usb2_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+			dwc->usb3_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
+		}
 
-	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;
-		else
-			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
+		if (IS_ERR(dwc->usb2_phy[0])) {
+			ret = PTR_ERR(dwc->usb2_phy[0]);
+			if (ret == -ENXIO || ret == -ENODEV)
+				dwc->usb2_phy[0] = NULL;
+			else
+				return dev_err_probe(dev, ret, "no usb2 phy configured\n");
+		}
+
+		if (IS_ERR(dwc->usb3_phy[0])) {
+			ret = PTR_ERR(dwc->usb3_phy[0]);
+			if (ret == -ENXIO || ret == -ENODEV)
+				dwc->usb3_phy[0] = NULL;
+			else
+				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
+		}
+
+		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;
+			else
+				return dev_err_probe(dev, ret, "no usb2 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;
+			else
+				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
+		}
+
+	} else {
+		pr_info("Multiport node found\n");
+		/* Iterate over each port of the MultiPort */
+		for_each_available_child_of_node(ports, port) {
+			dwc->usb2_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
+											0, port);
+			dwc->usb3_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
+											1, port);
+
+			if (IS_ERR(dwc->usb2_phy[i])) {
+				ret = PTR_ERR(dwc->usb2_phy[i]);
+				pr_err("usb2_phy gone %d\n", ret);
+				if (ret == -ENXIO || ret == -ENODEV)
+					dwc->usb2_phy[i] = NULL;
+				else
+					return dev_err_probe(dev, ret, "no usb2 phy configured\n");
+			}
+
+			if (IS_ERR(dwc->usb3_phy[i])) {
+				ret = PTR_ERR(dwc->usb3_phy[i]);
+				pr_err("usb3_phy gone %d\n", ret);
+				if (ret == -ENXIO || ret == -ENODEV)
+					dwc->usb3_phy[i] = NULL;
+				else
+					return dev_err_probe(dev, ret, "no usb3 phy configured\n");
+			}
+			//TODO Write Generic PHY API
+			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;
+				else
+					return dev_err_probe(dev, ret, "no usb2 phy configured\n");
+			}
+
+			//TODO Write Generic PHY API
+			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;
+				else
+					return dev_err_probe(dev, ret, "no usb3 phy configured\n");
+			}
+			i++;
+		}
 	}
 
 	return 0;
@@ -1310,8 +1441,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 	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);
+		if (dwc->usb2_phy[0])
+			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
 		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
 		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
 
@@ -1322,8 +1453,8 @@ 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);
+		if (dwc->usb2_phy[0])
+			otg_set_vbus(dwc->usb2_phy[0]->otg, true);
 		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
 		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
 
@@ -1673,7 +1804,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;
 
@@ -1838,13 +1969,17 @@ 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);
+	for (i = 0; i < dwc->num_hsphy; i++)
+		usb_phy_shutdown(dwc->usb2_phy[i]);
+	for (i = 0; i < dwc->num_ssphy; i++)
+		usb_phy_shutdown(dwc->usb3_phy[i]);
 	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);
+	for (i = 0; i < dwc->num_hsphy; i++)
+		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
+	for (i = 0; i < dwc->num_ssphy; i++)
+		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
 	phy_power_off(dwc->usb2_generic_phy);
 	phy_power_off(dwc->usb3_generic_phy);
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 81c486b..3175ed9 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_hsphy: Number of HS ports controlled by the core
+ * @num_dsphy: 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,8 +1149,10 @@ 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_hsphy;
+	u32			num_ssphy;
 
 	struct phy		*usb2_generic_phy;
 	struct phy		*usb3_generic_phy;
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 039bf24..7c9eba6 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -384,8 +384,8 @@ 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_phy[0])
+				otg_set_vbus(dwc->usb2_phy[0]->otg, true);
 			if (dwc->usb2_generic_phy)
 				phy_set_mode(dwc->usb2_generic_phy,
 					     PHY_MODE_USB_HOST);
@@ -398,8 +398,8 @@ 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_phy[0])
+			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
 		if (dwc->usb2_generic_phy)
 			phy_set_mode(dwc->usb2_generic_phy,
 				     PHY_MODE_USB_DEVICE);
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 4f54f0e..c58a67c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2870,8 +2870,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: [RFC 1/2] dt-bindings: usb: dwc3: Add support for multiport related properties

[Rob Herring]

On Thu, 19 May 2022 18:04:54 +0530, Harsh Agarwal wrote:
> Added support for multiport, mport, num-ssphy and num-hsphy
> 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         | 55 ++++++++++++++++++++++
>  1 file changed, 55 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:8: [warning] wrong indentation: expected 6 but found 7 (indentation)
./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:368:10: [warning] wrong indentation: expected 11 but found 9 (indentation)
./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:370:11: [warning] wrong indentation: expected 11 but found 10 (indentation)
./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:409:1: [error] syntax error: found character '\t' that cannot start any token (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/usb/snps,dwc3.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 52, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/main.py", line 434, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 119, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 773, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 852, in _ruamel_yaml.CParser._compose_sequence_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: while scanning a block scalar
  in "<unicode string>", line 401, column 5
found a tab character where an indentation space is expected
  in "<unicode string>", line 409, column 1
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/usb/snps,dwc3.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml
./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:  while scanning a block scalar
  in "<unicode string>", line 401, column 5
found a tab character where an indentation space is expected
  in "<unicode string>", line 409, column 1
./Documentation/devicetree/bindings/usb/amlogic,meson-g12a-usb-ctrl.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml
./Documentation/devicetree/bindings/usb/samsung,exynos-dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml
./Documentation/devicetree/bindings/usb/qcom,dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml
./Documentation/devicetree/bindings/usb/fsl,imx8mp-dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml
./Documentation/devicetree/bindings/usb/ti,keystone-dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml
./Documentation/devicetree/bindings/usb/intel,keembay-dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml
./Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: ignoring, error parsing file
make: *** [Makefile:1401: dt_binding_check] Error 2

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: [RFC 1/2] dt-bindings: usb: dwc3: Add support for multiport related properties

[Bjorn Andersson]

On Thu 19 May 05:34 PDT 2022, Harsh Agarwal wrote:

> Added support for multiport, mport, num-ssphy and num-hsphy
> properties. These properties are used to support devices having
> a multiport controller.
> 
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>

Please do run dt_binding_check on your bindings, even though they are
RFCs.

> ---
>  .../devicetree/bindings/usb/snps,dwc3.yaml         | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index f4471f8..39c61483 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -341,6 +341,35 @@ properties:
>        This port is used with the 'usb-role-switch' property  to connect the
>        dwc3 to type C connector.
>  
> +  multiport:

Why are you inventing an of_graph lookalike here?

> +    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.
> +    required:
> +      - mport
> +
> +  num-hsphy:
> +    description: Total number of HS-PHYs defined by the multiport controller.
> +    $ref: /schemas/types.yaml#/definitions/uint32

I'm expecting that you wont' have any superspeed-only ports. As such
this number would imply be the number of ports listed under the node.

> +
> +  num-ssphy:
> +    description: Total number of SS-PHYs defined by the multiport controller.
> +    $ref: /schemas/types.yaml#/definitions/uint32

Can you please explain why it's necessary to specify usb_nop_phy?
Wouldn't it be possible to omit the phy in the case of a HS-only port?
In which case this could just be calculated as well.

Regards,
Bjorn

> +
> +  mport:
> +    description: Each mport node represents one port of the multiport controller.
> +    patternProperties: "^mport@[0-9a-f]+$"
> +    oneOf:
> +       - required:
> +         - usb-phy
> +       - required:
> +          - phys
> +          - phy-names
> +
>  unevaluatedProperties: false
>  
>  required:
> @@ -369,4 +398,30 @@ 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: mport@1 {
> +          usb-phy = <&usb2_phy0>, <&usb3_phy0>;
> +	};
> +
> +	MP_2: mport@2 {
> +          usb-phy = <&usb2_phy1>, <&usb3_phy1>;
> +	};
> +
> +	MP_3: mport@3 {
> +          usb-phy = <&usb2_phy2>, <&usb_nop_phy>;
> +	};
> +
> +	MP_4: mport@4 {
> +          usb-phy = <&usb2_phy3>, <&usb_nop_phy>;
> +	};
> +
> +      };
> +    };
>  ...
> -- 
> 2.7.4
> 

Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller

[Bjorn Andersson]

On Thu 19 May 05:34 PDT 2022, Harsh Agarwal wrote:

> Currently the USB driver supports only single port controller
> which works with 2 PHYs at max ie HS and SS.
> 
> But some devices have "multiport" controller where a single
> controller supports multiple ports and each port have their own
> PHYs. Refactor PHY logic to support the same.
> 
> This implementation is compatible with existing glue drivers.
> 

I would love to see this support land, for various different devices I
have.

But can you please explain how you tested this on an upstream kernel,
given that all the Qualcomm phys are implemented in the generic phy
framework?


Also, when I spoke with Jack about this feature recently, he mentioned
that you need to update GUSB2PHYCFG(i) and GUSB3PIPECTL(i) in e.g.
dwc3_phy_config(). Is this series complete?

> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c   | 259 +++++++++++++++++++++++++++++++++++-----------
>  drivers/usb/dwc3/core.h   |   8 +-
>  drivers/usb/dwc3/drd.c    |   8 +-
>  drivers/usb/dwc3/gadget.c |   4 +-
>  4 files changed, 209 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2682469..8eb6b5b6 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -189,8 +189,8 @@ 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);
> +			if (dwc->usb2_phy[0])
> +				otg_set_vbus(dwc->usb2_phy[0]->otg, true);
>  			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>  			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>  			if (dwc->dis_split_quirk) {
> @@ -204,9 +204,13 @@ static void __dwc3_set_mode(struct work_struct *work)
>  		dwc3_core_soft_reset(dwc);
>  
>  		dwc3_event_buffers_setup(dwc);
> -
> -		if (dwc->usb2_phy)
> -			otg_set_vbus(dwc->usb2_phy->otg, false);
> +		/*
> +		 * Multiport Controller works only in host mode.
> +		 * There will only be one pair of HS and SS PHY for the controller operating in
> +		 * device mode.
> +		 */

I think this comment applies to every single place where you operate on
usb2_phy[0] only. But rather than telling the reader of the code that
multiport is valid only for host-only setups, wouldn't it be useful to
prevent the moving to device mode if multiple ports are specified?

Or am I just not finding the place where you do this?

> +		if (dwc->usb2_phy[0])
> +			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
>  		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
>  		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>  
> @@ -825,15 +829,20 @@ 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);
> +	for (i = 0; i < dwc->num_hsphy; i++)
> +		usb_phy_shutdown(dwc->usb2_phy[i]);
> +	for (i = 0; i < dwc->num_ssphy; i++)
> +		usb_phy_shutdown(dwc->usb3_phy[i]);
>  	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);
> +	for (i = 0; i < dwc->num_hsphy; i++)
> +		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
> +	for (i = 0; i < dwc->num_ssphy; i++)
> +		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
>  	phy_power_off(dwc->usb2_generic_phy);
>  	phy_power_off(dwc->usb3_generic_phy);
>  	dwc3_clk_disable(dwc);
> @@ -1038,7 +1047,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  {
>  	unsigned int		hw_mode;
>  	u32			reg;
> -	int			ret;
> +	int			ret, i;
>  
>  	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>  
> @@ -1066,8 +1075,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  		dwc->phys_ready = true;
>  	}
>  
> -	usb_phy_init(dwc->usb2_phy);
> -	usb_phy_init(dwc->usb3_phy);
> +	for (i = 0; i < dwc->num_hsphy; i++)
> +		usb_phy_init(dwc->usb2_phy[i]);
> +	for (i = 0; i < dwc->num_ssphy; i++)
> +		usb_phy_init(dwc->usb3_phy[i]);
>  	ret = phy_init(dwc->usb2_generic_phy);
>  	if (ret < 0)
>  		goto err0a;
> @@ -1112,8 +1123,10 @@ 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);
> +	for (i = 0; i < dwc->num_hsphy; i++)
> +		usb_phy_set_suspend(dwc->usb2_phy[i], 0);
> +	for (i = 0; i < dwc->num_ssphy; i++)
> +		usb_phy_set_suspend(dwc->usb3_phy[i], 0);
>  	ret = phy_power_on(dwc->usb2_generic_phy);
>  	if (ret < 0)
>  		goto err2;
> @@ -1234,12 +1247,16 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	phy_power_off(dwc->usb2_generic_phy);
>  
>  err2:
> -	usb_phy_set_suspend(dwc->usb2_phy, 1);
> -	usb_phy_set_suspend(dwc->usb3_phy, 1);
> +	for (i = 0; i < dwc->num_hsphy; i++)
> +		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
> +	for (i = 0; i < dwc->num_ssphy; i++)
> +		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
>  
>  err1:
> -	usb_phy_shutdown(dwc->usb2_phy);
> -	usb_phy_shutdown(dwc->usb3_phy);
> +	for (i = 0; i < dwc->num_hsphy; i++)
> +		usb_phy_shutdown(dwc->usb2_phy[i]);
> +	for (i = 0; i < dwc->num_ssphy; i++)
> +		usb_phy_shutdown(dwc->usb3_phy[i]);
>  	phy_exit(dwc->usb2_generic_phy);
>  	phy_exit(dwc->usb3_generic_phy);
>  
> @@ -1250,52 +1267,166 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	return ret;
>  }
>  
> +static struct usb_phy *dwc3_core_get_phy_by_handle_with_node(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_err(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;
> +}
> +
> +static int dwc3_extract_num_phys(struct dwc3 *dwc)

This doesn't extract the number, it allocates the usb2_phy and usb3_phy
arrays.

> +{
> +	struct device_node	*ports, *port;
> +
> +	/* 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 you break you need of_node_put().

> +	}
> +	if (!ports) {
> +		dwc->num_hsphy = 1;
> +		dwc->num_ssphy = 1;
> +	} else {
> +		for_each_available_child_of_node(ports, port) {
> +			dwc->num_hsphy += 1;
> +			dwc->num_ssphy += 1;
> +		}
> +	}
> +	dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_hsphy, dwc->num_ssphy);
> +
> +	dwc->usb2_phy = devm_kzalloc(dwc->dev,
> +		sizeof(*dwc->usb2_phy) * dwc->num_hsphy, GFP_KERNEL);
> +	if (!dwc->usb2_phy)
> +		return -ENOMEM;
> +
> +	dwc->usb3_phy = devm_kzalloc(dwc->dev,
> +		sizeof(*dwc->usb3_phy) * dwc->num_ssphy, GFP_KERNEL);
> +	if (!dwc->usb3_phy)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  static int dwc3_core_get_phy(struct dwc3 *dwc)
>  {
>  	struct device		*dev = dwc->dev;
>  	struct device_node	*node = dev->of_node;
> -	int ret;
> +	struct device_node	*ports, *port;
>  
> -	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);
> -	} 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);
> -	}
> +	int ret, i = 0;
>  
> -	if (IS_ERR(dwc->usb2_phy)) {
> -		ret = PTR_ERR(dwc->usb2_phy);
> -		if (ret == -ENXIO || ret == -ENODEV)
> -			dwc->usb2_phy = NULL;
> -		else
> -			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +	ret = dwc3_extract_num_phys(dwc);
> +	if (ret) {
> +		dev_err(dwc->dev, "Unable to extract number of PHYs\n");
> +		return ret;
>  	}
>  
> -	if (IS_ERR(dwc->usb3_phy)) {
> -		ret = PTR_ERR(dwc->usb3_phy);
> -		if (ret == -ENXIO || ret == -ENODEV)
> -			dwc->usb3_phy = NULL;
> -		else
> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +	/* Find if any "multiport" child is present inside DWC3*/
> +	for_each_available_child_of_node(node, ports) {
> +		if (!strcmp(ports->name, "multiport"))
> +			break;
>  	}
>  
> -	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;
> -		else
> -			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> -	}
> +	if (!ports) {
> +		if (node) {
> +			dwc->usb2_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> +			dwc->usb3_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> +		} else {
> +			dwc->usb2_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +			dwc->usb3_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> +		}
>  
> -	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;
> -		else
> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +		if (IS_ERR(dwc->usb2_phy[0])) {
> +			ret = PTR_ERR(dwc->usb2_phy[0]);
> +			if (ret == -ENXIO || ret == -ENODEV)
> +				dwc->usb2_phy[0] = NULL;
> +			else
> +				return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +		}
> +
> +		if (IS_ERR(dwc->usb3_phy[0])) {
> +			ret = PTR_ERR(dwc->usb3_phy[0]);
> +			if (ret == -ENXIO || ret == -ENODEV)
> +				dwc->usb3_phy[0] = NULL;
> +			else
> +				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +		}
> +
> +		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;
> +			else
> +				return dev_err_probe(dev, ret, "no usb2 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;
> +			else
> +				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +		}
> +
> +	} else {
> +		pr_info("Multiport node found\n");

Please remove your debug prints.

> +		/* Iterate over each port of the MultiPort */
> +		for_each_available_child_of_node(ports, port) {
> +			dwc->usb2_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
> +											0, port);
> +			dwc->usb3_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
> +											1, port);
> +
> +			if (IS_ERR(dwc->usb2_phy[i])) {
> +				ret = PTR_ERR(dwc->usb2_phy[i]);
> +				pr_err("usb2_phy gone %d\n", ret);
> +				if (ret == -ENXIO || ret == -ENODEV)
> +					dwc->usb2_phy[i] = NULL;
> +				else
> +					return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +			}
> +
> +			if (IS_ERR(dwc->usb3_phy[i])) {
> +				ret = PTR_ERR(dwc->usb3_phy[i]);
> +				pr_err("usb3_phy gone %d\n", ret);
> +				if (ret == -ENXIO || ret == -ENODEV)
> +					dwc->usb3_phy[i] = NULL;
> +				else
> +					return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +			}
> +			//TODO Write Generic PHY API

Afaict this isn't a TODO, this is just broken right now?

Regards,
Bjorn

> +			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;
> +				else
> +					return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +			}
> +
> +			//TODO Write Generic PHY API
> +			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;
> +				else
> +					return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +			}
> +			i++;
> +		}
>  	}
>  
>  	return 0;
> @@ -1310,8 +1441,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  	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);
> +		if (dwc->usb2_phy[0])
> +			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
>  		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
>  		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>  
> @@ -1322,8 +1453,8 @@ 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);
> +		if (dwc->usb2_phy[0])
> +			otg_set_vbus(dwc->usb2_phy[0]->otg, true);
>  		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>  		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>  
> @@ -1673,7 +1804,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;
>  
> @@ -1838,13 +1969,17 @@ 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);
> +	for (i = 0; i < dwc->num_hsphy; i++)
> +		usb_phy_shutdown(dwc->usb2_phy[i]);
> +	for (i = 0; i < dwc->num_ssphy; i++)
> +		usb_phy_shutdown(dwc->usb3_phy[i]);
>  	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);
> +	for (i = 0; i < dwc->num_hsphy; i++)
> +		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
> +	for (i = 0; i < dwc->num_ssphy; i++)
> +		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
>  	phy_power_off(dwc->usb2_generic_phy);
>  	phy_power_off(dwc->usb3_generic_phy);
>  
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 81c486b..3175ed9 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_hsphy: Number of HS ports controlled by the core
> + * @num_dsphy: 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,8 +1149,10 @@ 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_hsphy;
> +	u32			num_ssphy;
>  
>  	struct phy		*usb2_generic_phy;
>  	struct phy		*usb3_generic_phy;
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index 039bf24..7c9eba6 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -384,8 +384,8 @@ 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_phy[0])
> +				otg_set_vbus(dwc->usb2_phy[0]->otg, true);
>  			if (dwc->usb2_generic_phy)
>  				phy_set_mode(dwc->usb2_generic_phy,
>  					     PHY_MODE_USB_HOST);
> @@ -398,8 +398,8 @@ 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_phy[0])
> +			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
>  		if (dwc->usb2_generic_phy)
>  			phy_set_mode(dwc->usb2_generic_phy,
>  				     PHY_MODE_USB_DEVICE);
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 4f54f0e..c58a67c 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2870,8 +2870,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: [RFC 1/2] dt-bindings: usb: dwc3: Add support for multiport related properties

[Pavan Kondeti]

Hi Harsh,

On Thu, May 19, 2022 at 06:04:54PM +0530, Harsh Agarwal wrote:
> Added support for multiport, mport, num-ssphy and num-hsphy
> 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         | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index f4471f8..39c61483 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -341,6 +341,35 @@ 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.
> +    required:
> +      - mport
> +
> +  num-hsphy:
> +    description: Total number of HS-PHYs defined by the multiport controller.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  num-ssphy:
> +    description: Total number of SS-PHYs defined by the multiport controller.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
Do we need this properties at all? Atleast your next patch in this series is
not parsing those properties. The idea I believe is to maintain the same
usb-phy / phys semantics as of today. i.e we expect first PHY to be USB2 PHY
and 2nd PHY to be USB3 PHY. Obviously, we need to make sure that all ports
defined under multiport node are passing PHYs without any holes. For example,
if the controller has 3 ports and passing phys for 1st and 3rd port is not
acceptible. In any case we need to know the number of HS and SS PHYs so that 
the GUSB2PHYCFG/GUSB3PIPECTL are configured correctly, irrespective of how we
handle phy(s) in this node.

Can you please clarify on the need for num-hsphy and num-ssphy and what
happens for USB2 only ports?

> +  mport:
> +    description: Each mport node represents one port of the multiport controller.
> +    patternProperties: "^mport@[0-9a-f]+$"
> +    oneOf:
> +       - required:
> +         - usb-phy
> +       - required:
> +          - phys
> +          - phy-names
> +

Thanks,
Pavan

Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller

[Pavan Kondeti]

Hi Harsh,

I know that device tree bindings are not locked yet. So feel free to
address/respon to these comments after the bindings are locked.

On Thu, May 19, 2022 at 06:04:55PM +0530, Harsh Agarwal wrote:
> Currently the USB driver supports only single port controller
> which works with 2 PHYs at max ie HS and SS.
> 
> But some devices have "multiport" controller where a single
> controller supports multiple ports and each port have their own
> PHYs. Refactor PHY logic to support the same.
> 
> This implementation is compatible with existing glue drivers.
> 
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c   | 259 +++++++++++++++++++++++++++++++++++-----------
>  drivers/usb/dwc3/core.h   |   8 +-
>  drivers/usb/dwc3/drd.c    |   8 +-
>  drivers/usb/dwc3/gadget.c |   4 +-
>  4 files changed, 209 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2682469..8eb6b5b6 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -189,8 +189,8 @@ 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);
> +			if (dwc->usb2_phy[0])
> +				otg_set_vbus(dwc->usb2_phy[0]->otg, true);
>  			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>  			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>  			if (dwc->dis_split_quirk) {
> @@ -204,9 +204,13 @@ static void __dwc3_set_mode(struct work_struct *work)
>  		dwc3_core_soft_reset(dwc);
>  
>  		dwc3_event_buffers_setup(dwc);
> -
> -		if (dwc->usb2_phy)
> -			otg_set_vbus(dwc->usb2_phy->otg, false);
> +		/*
> +		 * Multiport Controller works only in host mode.
> +		 * There will only be one pair of HS and SS PHY for the controller operating in
> +		 * device mode.
> +		 */
> +		if (dwc->usb2_phy[0])
> +			otg_set_vbus(dwc->usb2_phy[0]->otg, false);

Fair enough. As per my understanding, the DWC3 controller can't keep one port in
device mode and another port in host mode since GCTL[12:13] bits which control
the port capability direction are controller specific but not port specific.
So it is okay to assume that the controller operating in device mode will only
have one USB2 PHY.

Your comment needs some correction though. We don't need both HS and SS PHY.
having just HS PHY is sufficient, in fact the HS PHY is also optional hence
the NULL check here.

>  		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
>  		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>  

<snip>

> @@ -1250,52 +1267,166 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	return ret;
>  }
>  
> +static struct usb_phy *dwc3_core_get_phy_by_handle_with_node(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_err(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;
> +}
> +
> +static int dwc3_extract_num_phys(struct dwc3 *dwc)
> +{
> +	struct device_node	*ports, *port;
> +
> +	/* Find if any "multiport" child is present inside DWC3*/

nit pick. space after DWC3.

> +	for_each_available_child_of_node(dwc->dev->of_node, ports) {
> +		if (!strcmp(ports->name, "multiport"))
> +			break;
> +	}
> +	if (!ports) {
> +		dwc->num_hsphy = 1;
> +		dwc->num_ssphy = 1;
> +	} else {
> +		for_each_available_child_of_node(ports, port) {
> +			dwc->num_hsphy += 1;
> +			dwc->num_ssphy += 1;
> +		}
> +	}

Would of_get_child_count() work?

I understand that we need to count the number of HS and SS PHY here for two
reasons here.

1. To allocate USB/Generic PHY structures inside dwc3.
2. We need to loop around the phy count to write into GUSB2PHYCFG/GUSB3PIPECTL
registers.

However, is it not possible that a port is HS only and not have any SS PHY
associated. Incrementing dwc->num_ssphy is not entirely correct for every
child. We need to increment dwc->num_ssphy only when get the correct phandle
for SS PHY.

It may be easier to allocate instances as per the child count but increment
dwc->num_ssphy as and when we process phys.

> +	dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_hsphy, dwc->num_ssphy);
> +
> +	dwc->usb2_phy = devm_kzalloc(dwc->dev,
> +		sizeof(*dwc->usb2_phy) * dwc->num_hsphy, GFP_KERNEL);
> +	if (!dwc->usb2_phy)
> +		return -ENOMEM;
> +
> +	dwc->usb3_phy = devm_kzalloc(dwc->dev,
> +		sizeof(*dwc->usb3_phy) * dwc->num_ssphy, GFP_KERNEL);
> +	if (!dwc->usb3_phy)
> +		return -ENOMEM;
> +
> +	return 0;
> +}

we have to allocate sufficient instances for generic PHY as well. I understand
that this is a RFC patch at this point but mentioning here so that you plan
accordingly.

> +
>  static int dwc3_core_get_phy(struct dwc3 *dwc)
>  {
>  	struct device		*dev = dwc->dev;
>  	struct device_node	*node = dev->of_node;
> -	int ret;
> +	struct device_node	*ports, *port;
>  
> -	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);
> -	} 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);
> -	}
> +	int ret, i = 0;
>  
> -	if (IS_ERR(dwc->usb2_phy)) {
> -		ret = PTR_ERR(dwc->usb2_phy);
> -		if (ret == -ENXIO || ret == -ENODEV)
> -			dwc->usb2_phy = NULL;
> -		else
> -			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +	ret = dwc3_extract_num_phys(dwc);
> +	if (ret) {
> +		dev_err(dwc->dev, "Unable to extract number of PHYs\n");
> +		return ret;
>  	}
>  
> -	if (IS_ERR(dwc->usb3_phy)) {
> -		ret = PTR_ERR(dwc->usb3_phy);
> -		if (ret == -ENXIO || ret == -ENODEV)
> -			dwc->usb3_phy = NULL;
> -		else
> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +	/* Find if any "multiport" child is present inside DWC3*/
> +	for_each_available_child_of_node(node, ports) {
> +		if (!strcmp(ports->name, "multiport"))
> +			break;
>  	}
>  
> -	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;
> -		else
> -			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> -	}
> +	if (!ports) {
> +		if (node) {
> +			dwc->usb2_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> +			dwc->usb3_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> +		} else {
> +			dwc->usb2_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +			dwc->usb3_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> +		}
>  
> -	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;
> -		else
> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +		if (IS_ERR(dwc->usb2_phy[0])) {
> +			ret = PTR_ERR(dwc->usb2_phy[0]);
> +			if (ret == -ENXIO || ret == -ENODEV)
> +				dwc->usb2_phy[0] = NULL;
> +			else
> +				return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +		}
> +
> +		if (IS_ERR(dwc->usb3_phy[0])) {
> +			ret = PTR_ERR(dwc->usb3_phy[0]);
> +			if (ret == -ENXIO || ret == -ENODEV)
> +				dwc->usb3_phy[0] = NULL;
> +			else
> +				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +		}
> +
> +		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;
> +			else
> +				return dev_err_probe(dev, ret, "no usb2 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;
> +			else
> +				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +		}
> +

Would be good to write a common wrapper that gets the USB PHY phandles
and works for each mport (if present) or the fallback case. That wrapper
function should take required arguments and look for usb2/usb3 phy as well as
generic phy.

> +	} else {
> +		pr_info("Multiport node found\n");
> +		/* Iterate over each port of the MultiPort */
> +		for_each_available_child_of_node(ports, port) {
> +			dwc->usb2_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
> +											0, port);
> +			dwc->usb3_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
> +											1, port);
> +
> +			if (IS_ERR(dwc->usb2_phy[i])) {
> +				ret = PTR_ERR(dwc->usb2_phy[i]);
> +				pr_err("usb2_phy gone %d\n", ret);
> +				if (ret == -ENXIO || ret == -ENODEV)
> +					dwc->usb2_phy[i] = NULL;
> +				else
> +					return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +			}
> +
> +			if (IS_ERR(dwc->usb3_phy[i])) {
> +				ret = PTR_ERR(dwc->usb3_phy[i]);
> +				pr_err("usb3_phy gone %d\n", ret);
> +				if (ret == -ENXIO || ret == -ENODEV)
> +					dwc->usb3_phy[i] = NULL;
> +				else
> +					return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +			}
> +			//TODO Write Generic PHY API
> +			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;
> +				else
> +					return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +			}
> +
> +			//TODO Write Generic PHY API
> +			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;
> +				else
> +					return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +			}
> +			i++;
> +		}
>  	}
>  
>  	return 0;

<snip>

> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 81c486b..3175ed9 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_hsphy: Number of HS ports controlled by the core
> + * @num_dsphy: 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,8 +1149,10 @@ 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_hsphy;
> +	u32			num_ssphy;
>  
>  	struct phy		*usb2_generic_phy;
>  	struct phy		*usb3_generic_phy;

The generic phy also needs to be supported/handled.

Thanks,
Pavan

Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller

[Pavan Kondeti]

On Thu, May 19, 2022 at 06:04:55PM +0530, Harsh Agarwal wrote:
> Currently the USB driver supports only single port controller
> which works with 2 PHYs at max ie HS and SS.
> 
> But some devices have "multiport" controller where a single
> controller supports multiple ports and each port have their own
> PHYs. Refactor PHY logic to support the same.
> 
> This implementation is compatible with existing glue drivers.
> 
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>

<snip>

> +static struct usb_phy *dwc3_core_get_phy_by_handle_with_node(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_err(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;
> +}
> +

we have devm_of_phy_get() API that takes both device and device_node (could be
other than device-of_node). This API you have to use for the generic PHY, so
we can have a similar wrapper with devm_of_usb_get_phy_by_phandle() which
takes device and device_node as arguments.

> +static int dwc3_extract_num_phys(struct dwc3 *dwc)
> +{
> +	struct device_node	*ports, *port;
> +
> +	/* 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_hsphy = 1;
> +		dwc->num_ssphy = 1;
> +	} else {
> +		for_each_available_child_of_node(ports, port) {
> +			dwc->num_hsphy += 1;
> +			dwc->num_ssphy += 1;
> +		}
> +	}
> +	dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_hsphy, dwc->num_ssphy);
> +
> +	dwc->usb2_phy = devm_kzalloc(dwc->dev,
> +		sizeof(*dwc->usb2_phy) * dwc->num_hsphy, GFP_KERNEL);
> +	if (!dwc->usb2_phy)
> +		return -ENOMEM;
> +
> +	dwc->usb3_phy = devm_kzalloc(dwc->dev,
> +		sizeof(*dwc->usb3_phy) * dwc->num_ssphy, GFP_KERNEL);
> +	if (!dwc->usb3_phy)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  static int dwc3_core_get_phy(struct dwc3 *dwc)
>  {
>  	struct device		*dev = dwc->dev;
>  	struct device_node	*node = dev->of_node;
> -	int ret;
> +	struct device_node	*ports, *port;
>  
> -	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);
> -	} 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);
> -	}
> +	int ret, i = 0;
>  
> -	if (IS_ERR(dwc->usb2_phy)) {
> -		ret = PTR_ERR(dwc->usb2_phy);
> -		if (ret == -ENXIO || ret == -ENODEV)
> -			dwc->usb2_phy = NULL;
> -		else
> -			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +	ret = dwc3_extract_num_phys(dwc);
> +	if (ret) {
> +		dev_err(dwc->dev, "Unable to extract number of PHYs\n");
> +		return ret;
>  	}
>  
> -	if (IS_ERR(dwc->usb3_phy)) {
> -		ret = PTR_ERR(dwc->usb3_phy);
> -		if (ret == -ENXIO || ret == -ENODEV)
> -			dwc->usb3_phy = NULL;
> -		else
> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +	/* Find if any "multiport" child is present inside DWC3*/
> +	for_each_available_child_of_node(node, ports) {
> +		if (!strcmp(ports->name, "multiport"))
> +			break;
>  	}
>  
> -	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;
> -		else
> -			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> -	}
> +	if (!ports) {
> +		if (node) {
> +			dwc->usb2_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> +			dwc->usb3_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> +		} else {
> +			dwc->usb2_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +			dwc->usb3_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> +		}
>  
> -	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;
> -		else
> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +		if (IS_ERR(dwc->usb2_phy[0])) {
> +			ret = PTR_ERR(dwc->usb2_phy[0]);
> +			if (ret == -ENXIO || ret == -ENODEV)
> +				dwc->usb2_phy[0] = NULL;
> +			else
> +				return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +		}
> +
> +		if (IS_ERR(dwc->usb3_phy[0])) {
> +			ret = PTR_ERR(dwc->usb3_phy[0]);
> +			if (ret == -ENXIO || ret == -ENODEV)
> +				dwc->usb3_phy[0] = NULL;
> +			else
> +				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +		}
> +
> +		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;
> +			else
> +				return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +		}

Like I said above, devm_of_phy_get() is what needs to be used if we want to
re-use the same block of code that works for existing and multiport case.

Thanks,
Pavan

Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller

[Harsh Agarwal]

On 5/21/2022 8:47 AM, Bjorn Andersson wrote:
> On Thu 19 May 05:34 PDT 2022, Harsh Agarwal wrote:
>
>> Currently the USB driver supports only single port controller
>> which works with 2 PHYs at max ie HS and SS.
>>
>> But some devices have "multiport" controller where a single
>> controller supports multiple ports and each port have their own
>> PHYs. Refactor PHY logic to support the same.
>>
>> This implementation is compatible with existing glue drivers.
>>
> I would love to see this support land, for various different devices I
> have.
>
> But can you please explain how you tested this on an upstream kernel,
> given that all the Qualcomm phys are implemented in the generic phy
> framework?
>
>
> Also, when I spoke with Jack about this feature recently, he mentioned
> that you need to update GUSB2PHYCFG(i) and GUSB3PIPECTL(i) in e.g.
> dwc3_phy_config(). Is this series complete?

The idea here was to make the core driver compatible with multiport and
with the existing single port controller.
We wanted to get feedback on the current approach.
Ofcourse the code needs refactoring.

Yes for multiport we need to accomodate for GUSB2PHYCFG(i) and 
GUSB3PIPECTL(i).
Will add those changes as well.

>
>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.c   | 259 +++++++++++++++++++++++++++++++++++-----------
>>   drivers/usb/dwc3/core.h   |   8 +-
>>   drivers/usb/dwc3/drd.c    |   8 +-
>>   drivers/usb/dwc3/gadget.c |   4 +-
>>   4 files changed, 209 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 2682469..8eb6b5b6 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -189,8 +189,8 @@ 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);
>> +			if (dwc->usb2_phy[0])
>> +				otg_set_vbus(dwc->usb2_phy[0]->otg, true);
>>   			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>>   			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>>   			if (dwc->dis_split_quirk) {
>> @@ -204,9 +204,13 @@ static void __dwc3_set_mode(struct work_struct *work)
>>   		dwc3_core_soft_reset(dwc);
>>   
>>   		dwc3_event_buffers_setup(dwc);
>> -
>> -		if (dwc->usb2_phy)
>> -			otg_set_vbus(dwc->usb2_phy->otg, false);
>> +		/*
>> +		 * Multiport Controller works only in host mode.
>> +		 * There will only be one pair of HS and SS PHY for the controller operating in
>> +		 * device mode.
>> +		 */
> I think this comment applies to every single place where you operate on
> usb2_phy[0] only. But rather than telling the reader of the code that
> multiport is valid only for host-only setups, wouldn't it be useful to
> prevent the moving to device mode if multiple ports are specified?
>
> Or am I just not finding the place where you do this?
Yeah  maybe adding comment just in one place might be Odd.
Multiport are host mode only, so transitioning to device mode will lead 
to crash or will not be permitted.
We can add checks in dwc3_probe to bail out if it's multiport controller 
and dr_mode is device/OTG.
>
>> +		if (dwc->usb2_phy[0])
>> +			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
>>   		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
>>   		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>>   
>> @@ -825,15 +829,20 @@ 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);
>> +	for (i = 0; i < dwc->num_hsphy; i++)
>> +		usb_phy_shutdown(dwc->usb2_phy[i]);
>> +	for (i = 0; i < dwc->num_ssphy; i++)
>> +		usb_phy_shutdown(dwc->usb3_phy[i]);
>>   	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);
>> +	for (i = 0; i < dwc->num_hsphy; i++)
>> +		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
>> +	for (i = 0; i < dwc->num_ssphy; i++)
>> +		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
>>   	phy_power_off(dwc->usb2_generic_phy);
>>   	phy_power_off(dwc->usb3_generic_phy);
>>   	dwc3_clk_disable(dwc);
>> @@ -1038,7 +1047,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   {
>>   	unsigned int		hw_mode;
>>   	u32			reg;
>> -	int			ret;
>> +	int			ret, i;
>>   
>>   	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>>   
>> @@ -1066,8 +1075,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   		dwc->phys_ready = true;
>>   	}
>>   
>> -	usb_phy_init(dwc->usb2_phy);
>> -	usb_phy_init(dwc->usb3_phy);
>> +	for (i = 0; i < dwc->num_hsphy; i++)
>> +		usb_phy_init(dwc->usb2_phy[i]);
>> +	for (i = 0; i < dwc->num_ssphy; i++)
>> +		usb_phy_init(dwc->usb3_phy[i]);
>>   	ret = phy_init(dwc->usb2_generic_phy);
>>   	if (ret < 0)
>>   		goto err0a;
>> @@ -1112,8 +1123,10 @@ 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);
>> +	for (i = 0; i < dwc->num_hsphy; i++)
>> +		usb_phy_set_suspend(dwc->usb2_phy[i], 0);
>> +	for (i = 0; i < dwc->num_ssphy; i++)
>> +		usb_phy_set_suspend(dwc->usb3_phy[i], 0);
>>   	ret = phy_power_on(dwc->usb2_generic_phy);
>>   	if (ret < 0)
>>   		goto err2;
>> @@ -1234,12 +1247,16 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   	phy_power_off(dwc->usb2_generic_phy);
>>   
>>   err2:
>> -	usb_phy_set_suspend(dwc->usb2_phy, 1);
>> -	usb_phy_set_suspend(dwc->usb3_phy, 1);
>> +	for (i = 0; i < dwc->num_hsphy; i++)
>> +		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
>> +	for (i = 0; i < dwc->num_ssphy; i++)
>> +		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
>>   
>>   err1:
>> -	usb_phy_shutdown(dwc->usb2_phy);
>> -	usb_phy_shutdown(dwc->usb3_phy);
>> +	for (i = 0; i < dwc->num_hsphy; i++)
>> +		usb_phy_shutdown(dwc->usb2_phy[i]);
>> +	for (i = 0; i < dwc->num_ssphy; i++)
>> +		usb_phy_shutdown(dwc->usb3_phy[i]);
>>   	phy_exit(dwc->usb2_generic_phy);
>>   	phy_exit(dwc->usb3_generic_phy);
>>   
>> @@ -1250,52 +1267,166 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   	return ret;
>>   }
>>   
>> +static struct usb_phy *dwc3_core_get_phy_by_handle_with_node(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_err(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;
>> +}
>> +
>> +static int dwc3_extract_num_phys(struct dwc3 *dwc)
> This doesn't extract the number, it allocates the usb2_phy and usb3_phy
> arrays.

I will push another change where for every "mport" node if the number of 
USB Phy phandle is 1 then increment only num_hsphy.
If it's 2 then increase both num_hsphy, num_ssphy .
Assumption : Every port will have a HSPHY phandle.

>
>> +{
>> +	struct device_node	*ports, *port;
>> +
>> +	/* 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 you break you need of_node_put().
>
>> +	}
>> +	if (!ports) {
>> +		dwc->num_hsphy = 1;
>> +		dwc->num_ssphy = 1;
>> +	} else {
>> +		for_each_available_child_of_node(ports, port) {
>> +			dwc->num_hsphy += 1;
>> +			dwc->num_ssphy += 1;
>> +		}
>> +	}
>> +	dev_info(dwc->dev, "Num of HS and SS PHY are %u %u\n", dwc->num_hsphy, dwc->num_ssphy);
>> +
>> +	dwc->usb2_phy = devm_kzalloc(dwc->dev,
>> +		sizeof(*dwc->usb2_phy) * dwc->num_hsphy, GFP_KERNEL);
>> +	if (!dwc->usb2_phy)
>> +		return -ENOMEM;
>> +
>> +	dwc->usb3_phy = devm_kzalloc(dwc->dev,
>> +		sizeof(*dwc->usb3_phy) * dwc->num_ssphy, GFP_KERNEL);
>> +	if (!dwc->usb3_phy)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>>   static int dwc3_core_get_phy(struct dwc3 *dwc)
>>   {
>>   	struct device		*dev = dwc->dev;
>>   	struct device_node	*node = dev->of_node;
>> -	int ret;
>> +	struct device_node	*ports, *port;
>>   
>> -	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);
>> -	} 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);
>> -	}
>> +	int ret, i = 0;
>>   
>> -	if (IS_ERR(dwc->usb2_phy)) {
>> -		ret = PTR_ERR(dwc->usb2_phy);
>> -		if (ret == -ENXIO || ret == -ENODEV)
>> -			dwc->usb2_phy = NULL;
>> -		else
>> -			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> +	ret = dwc3_extract_num_phys(dwc);
>> +	if (ret) {
>> +		dev_err(dwc->dev, "Unable to extract number of PHYs\n");
>> +		return ret;
>>   	}
>>   
>> -	if (IS_ERR(dwc->usb3_phy)) {
>> -		ret = PTR_ERR(dwc->usb3_phy);
>> -		if (ret == -ENXIO || ret == -ENODEV)
>> -			dwc->usb3_phy = NULL;
>> -		else
>> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> +	/* Find if any "multiport" child is present inside DWC3*/
>> +	for_each_available_child_of_node(node, ports) {
>> +		if (!strcmp(ports->name, "multiport"))
>> +			break;
>>   	}
>>   
>> -	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;
>> -		else
>> -			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> -	}
>> +	if (!ports) {
>> +		if (node) {
>> +			dwc->usb2_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
>> +			dwc->usb3_phy[0] = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
>> +		} else {
>> +			dwc->usb2_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>> +			dwc->usb3_phy[0] = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>> +		}
>>   
>> -	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;
>> -		else
>> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> +		if (IS_ERR(dwc->usb2_phy[0])) {
>> +			ret = PTR_ERR(dwc->usb2_phy[0]);
>> +			if (ret == -ENXIO || ret == -ENODEV)
>> +				dwc->usb2_phy[0] = NULL;
>> +			else
>> +				return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> +		}
>> +
>> +		if (IS_ERR(dwc->usb3_phy[0])) {
>> +			ret = PTR_ERR(dwc->usb3_phy[0]);
>> +			if (ret == -ENXIO || ret == -ENODEV)
>> +				dwc->usb3_phy[0] = NULL;
>> +			else
>> +				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> +		}
>> +
>> +		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;
>> +			else
>> +				return dev_err_probe(dev, ret, "no usb2 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;
>> +			else
>> +				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> +		}
>> +
>> +	} else {
>> +		pr_info("Multiport node found\n");
> Please remove your debug prints.
Sure
>
>> +		/* Iterate over each port of the MultiPort */
>> +		for_each_available_child_of_node(ports, port) {
>> +			dwc->usb2_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
>> +											0, port);
>> +			dwc->usb3_phy[i] = dwc3_core_get_phy_by_handle_with_node(dev, "usb-phy",
>> +											1, port);
>> +
>> +			if (IS_ERR(dwc->usb2_phy[i])) {
>> +				ret = PTR_ERR(dwc->usb2_phy[i]);
>> +				pr_err("usb2_phy gone %d\n", ret);
>> +				if (ret == -ENXIO || ret == -ENODEV)
>> +					dwc->usb2_phy[i] = NULL;
>> +				else
>> +					return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> +			}
>> +
>> +			if (IS_ERR(dwc->usb3_phy[i])) {
>> +				ret = PTR_ERR(dwc->usb3_phy[i]);
>> +				pr_err("usb3_phy gone %d\n", ret);
>> +				if (ret == -ENXIO || ret == -ENODEV)
>> +					dwc->usb3_phy[i] = NULL;
>> +				else
>> +					return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> +			}
>> +			//TODO Write Generic PHY API
> Afaict this isn't a TODO, this is just broken right now?

I have tested these changes against "usb-phy" and not the generic PHY 
approach.
We have "devm_of_get_phy" which can be used for Generic PHY support.
Thanks Pavan for suggesting this.
I will check this on upstream device.

>
> Regards,
> Bjorn
>
>> +			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;
>> +				else
>> +					return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> +			}
>> +
>> +			//TODO Write Generic PHY API
>> +			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;
>> +				else
>> +					return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> +			}
>> +			i++;
>> +		}
>>   	}
>>   
>>   	return 0;
>> @@ -1310,8 +1441,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>   	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);
>> +		if (dwc->usb2_phy[0])
>> +			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
>>   		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
>>   		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>>   
>> @@ -1322,8 +1453,8 @@ 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);
>> +		if (dwc->usb2_phy[0])
>> +			otg_set_vbus(dwc->usb2_phy[0]->otg, true);
>>   		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>>   		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>>   
>> @@ -1673,7 +1804,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;
>>   
>> @@ -1838,13 +1969,17 @@ 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);
>> +	for (i = 0; i < dwc->num_hsphy; i++)
>> +		usb_phy_shutdown(dwc->usb2_phy[i]);
>> +	for (i = 0; i < dwc->num_ssphy; i++)
>> +		usb_phy_shutdown(dwc->usb3_phy[i]);
>>   	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);
>> +	for (i = 0; i < dwc->num_hsphy; i++)
>> +		usb_phy_set_suspend(dwc->usb2_phy[i], 1);
>> +	for (i = 0; i < dwc->num_ssphy; i++)
>> +		usb_phy_set_suspend(dwc->usb3_phy[i], 1);
>>   	phy_power_off(dwc->usb2_generic_phy);
>>   	phy_power_off(dwc->usb3_generic_phy);
>>   
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 81c486b..3175ed9 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_hsphy: Number of HS ports controlled by the core
>> + * @num_dsphy: 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,8 +1149,10 @@ 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_hsphy;
>> +	u32			num_ssphy;
>>   
>>   	struct phy		*usb2_generic_phy;
>>   	struct phy		*usb3_generic_phy;
>> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
>> index 039bf24..7c9eba6 100644
>> --- a/drivers/usb/dwc3/drd.c
>> +++ b/drivers/usb/dwc3/drd.c
>> @@ -384,8 +384,8 @@ 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_phy[0])
>> +				otg_set_vbus(dwc->usb2_phy[0]->otg, true);
>>   			if (dwc->usb2_generic_phy)
>>   				phy_set_mode(dwc->usb2_generic_phy,
>>   					     PHY_MODE_USB_HOST);
>> @@ -398,8 +398,8 @@ 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_phy[0])
>> +			otg_set_vbus(dwc->usb2_phy[0]->otg, false);
>>   		if (dwc->usb2_generic_phy)
>>   			phy_set_mode(dwc->usb2_generic_phy,
>>   				     PHY_MODE_USB_DEVICE);
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 4f54f0e..c58a67c 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2870,8 +2870,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: [RFC 1/2] dt-bindings: usb: dwc3: Add support for multiport related properties

[Harsh Agarwal]

On 5/21/2022 8:35 AM, Bjorn Andersson wrote:
> On Thu 19 May 05:34 PDT 2022, Harsh Agarwal wrote:
>
>> Added support for multiport, mport, num-ssphy and num-hsphy
>> properties. These properties are used to support devices having
>> a multiport controller.
>>
>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> Please do run dt_binding_check on your bindings, even though they are
> RFCs.
There was some dt_binding package issue so I could not run it.
Thought to check this later since this is RFC.
Sure, I will check this.
>
>> ---
>>   .../devicetree/bindings/usb/snps,dwc3.yaml         | 55 ++++++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> index f4471f8..39c61483 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -341,6 +341,35 @@ properties:
>>         This port is used with the 'usb-role-switch' property  to connect the
>>         dwc3 to type C connector.
>>   
>> +  multiport:
> Why are you inventing an of_graph lookalike here?

Not really an of_graph lookalike, here we just wanted to add "multiport" 
node which would have "mport" nodes for every port that it supports.
We can treat this "multiport" like an object encapsulating other "mport" 
objects.

>
>> +    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.
>> +    required:
>> +      - mport
>> +
>> +  num-hsphy:
>> +    description: Total number of HS-PHYs defined by the multiport controller.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> I'm expecting that you wont' have any superspeed-only ports. As such
> this number would imply be the number of ports listed under the node.

Yes we can say that num_hsphy is equal to the number of "mport" nodes as 
every node will
have a HSPHY.

>
>> +
>> +  num-ssphy:
>> +    description: Total number of SS-PHYs defined by the multiport controller.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> Can you please explain why it's necessary to specify usb_nop_phy?
> Wouldn't it be possible to omit the phy in the case of a HS-only port?
> In which case this could just be calculated as well.
Yes we can ignore usb_nop_phy. It's not really necessary at places where 
we don't have a proper PHY in place.
>
> Regards,
> Bjorn
>
>> +
>> +  mport:
>> +    description: Each mport node represents one port of the multiport controller.
>> +    patternProperties: "^mport@[0-9a-f]+$"
>> +    oneOf:
>> +       - required:
>> +         - usb-phy
>> +       - required:
>> +          - phys
>> +          - phy-names
>> +
>>   unevaluatedProperties: false
>>   
>>   required:
>> @@ -369,4 +398,30 @@ 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: mport@1 {
>> +          usb-phy = <&usb2_phy0>, <&usb3_phy0>;
>> +	};
>> +
>> +	MP_2: mport@2 {
>> +          usb-phy = <&usb2_phy1>, <&usb3_phy1>;
>> +	};
>> +
>> +	MP_3: mport@3 {
>> +          usb-phy = <&usb2_phy2>, <&usb_nop_phy>;
>> +	};
>> +
>> +	MP_4: mport@4 {
>> +          usb-phy = <&usb2_phy3>, <&usb_nop_phy>;
>> +	};
>> +
>> +      };
>> +    };
>>   ...
>> -- 
>> 2.7.4
>>

Re: [RFC 1/2] dt-bindings: usb: dwc3: Add support for multiport related properties

[Rob Herring]

On Thu, May 19, 2022 at 06:04:54PM +0530, Harsh Agarwal wrote:
> Added support for multiport, mport, num-ssphy and num-hsphy
> 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         | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index f4471f8..39c61483 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -341,6 +341,35 @@ 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.
> +    required:
> +      - mport
> +
> +  num-hsphy:
> +    description: Total number of HS-PHYs defined by the multiport controller.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  num-ssphy:
> +    description: Total number of SS-PHYs defined by the multiport controller.
> +    $ref: /schemas/types.yaml#/definitions/uint32

These seem redundant. Can't you count ports/phys?

> +
> +  mport:
> +    description: Each mport node represents one port of the multiport controller.
> +    patternProperties: "^mport@[0-9a-f]+$"

Think about how the USB device binding fits into this. A hub vs. 
multiple root ports doesn't seem much different.

Rob

Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller

[Brian Masney]

On Thu, May 19, 2022 at 06:04:55PM +0530, Harsh Agarwal wrote:
> -	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;
> -		else
> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +		if (IS_ERR(dwc->usb2_phy[0])) {
> +			ret = PTR_ERR(dwc->usb2_phy[0]);
> +			if (ret == -ENXIO || ret == -ENODEV)
> +				dwc->usb2_phy[0] = NULL;
> +			else
> +				return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +		}
> +
> +		if (IS_ERR(dwc->usb3_phy[0])) {
> +			ret = PTR_ERR(dwc->usb3_phy[0]);
> +			if (ret == -ENXIO || ret == -ENODEV)
> +				dwc->usb3_phy[0] = NULL;
> +			else
> +				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> +		}
> +
> +		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;
> +			else
> +				return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> +		}

I know that this block is a copy and paste move from above, but is the
ENOSYS check really needed? It looks like the phy_get() only returns
-ENODEV.

> @@ -1147,8 +1149,10 @@ 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_hsphy;
> +	u32			num_ssphy;

Rename num_hsphy / num_ssphy to num_usb2_phy and num_usb3_phy so this is
easier to audit.

Brian

Re: [RFC 2/2] usb: dwc3: Refactor PHY logic to support Multiport Controller

[Harsh Agarwal]

On 5/23/2022 9:40 PM, Brian Masney wrote:
> On Thu, May 19, 2022 at 06:04:55PM +0530, Harsh Agarwal wrote:
>> -	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;
>> -		else
>> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> +		if (IS_ERR(dwc->usb2_phy[0])) {
>> +			ret = PTR_ERR(dwc->usb2_phy[0]);
>> +			if (ret == -ENXIO || ret == -ENODEV)
>> +				dwc->usb2_phy[0] = NULL;
>> +			else
>> +				return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> +		}
>> +
>> +		if (IS_ERR(dwc->usb3_phy[0])) {
>> +			ret = PTR_ERR(dwc->usb3_phy[0]);
>> +			if (ret == -ENXIO || ret == -ENODEV)
>> +				dwc->usb3_phy[0] = NULL;
>> +			else
>> +				return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> +		}
>> +
>> +		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;
>> +			else
>> +				return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> +		}
> I know that this block is a copy and paste move from above, but is the
> ENOSYS check really needed? It looks like the phy_get() only returns
> -ENODEV.
sure I got ENOSYS removed in my RFC V2 patch. This was present by 
default, so I did not change it earlier.
>> @@ -1147,8 +1149,10 @@ 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_hsphy;
>> +	u32			num_ssphy;
> Rename num_hsphy / num_ssphy to num_usb2_phy and num_usb3_phy so this is
> easier to audit.
Okay will change this in my next Patch.
>
> Brian
>

Re: [RFC 1/2] dt-bindings: usb: dwc3: Add support for multiport related properties

[Harsh Agarwal]

On 5/23/2022 5:55 PM, Rob Herring wrote:
> On Thu, May 19, 2022 at 06:04:54PM +0530, Harsh Agarwal wrote:
>> Added support for multiport, mport, num-ssphy and num-hsphy
>> 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         | 55 ++++++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> index f4471f8..39c61483 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -341,6 +341,35 @@ 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.
>> +    required:
>> +      - mport
>> +
>> +  num-hsphy:
>> +    description: Total number of HS-PHYs defined by the multiport controller.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  num-ssphy:
>> +    description: Total number of SS-PHYs defined by the multiport controller.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> These seem redundant. Can't you count ports/phys?

Counting ports will not suffice as there could be devices which has 
different numbers of HS
and SS PHYs. Consider a case like a Quadport multiport controller where 
we have the 1st & 2nd
port supporting SS and 3rd & 4th port supporting HS only. So we would 
need the proper number
of HS and SS PHYs.
Currently in the RFC v2 we are calculating the exact number of PHYS 
supported by each port
by counting the phandles we have defined.

>
>> +
>> +  mport:
>> +    description: Each mport node represents one port of the multiport controller.
>> +    patternProperties: "^mport@[0-9a-f]+$"
> Think about how the USB device binding fits into this. A hub vs.
> multiple root ports doesn't seem much different.
A hub will have multiple ports but only one HS and SS PHYs at the root 
port supporting it.
With Multiport Controller, each port will have its own PHY [whether HS 
only or both HS & SS]
>
> Rob