[PATCH 0/2] Reset xHCI port PHY on disconnect

[PATCH 0/2] Reset xHCI port PHY on disconnect

[Srinath Mannam]

This patch set adds a quirk in xHCI driver to reset PHY of xHCI port
on its disconnect event.

This patch set is based on Linux-5.0-rc2.

Srinath Mannam (2):
  dt-bindings: usb-xhci: Add usb-phy-port-reset property
  drivers: xhci: Add quirk to reset xHCI port PHY

 Documentation/devicetree/bindings/usb/usb-xhci.txt |  1 +
 drivers/usb/core/hcd.c                             |  6 ++++++
 drivers/usb/core/phy.c                             | 21 +++++++++++++++++++++
 drivers/usb/core/phy.h                             |  1 +
 drivers/usb/host/xhci-plat.c                       |  3 +++
 drivers/usb/host/xhci-ring.c                       |  9 ++++++---
 drivers/usb/host/xhci.h                            |  1 +
 include/linux/usb/hcd.h                            |  1 +
 8 files changed, 40 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property

[Srinath Mannam]

Add usb-phy-port-reset optional property to set quirk in xhci platform
driver which forces USB port PHY reset on port disconnect event.

Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
---
 Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index fea8b15..ecbdb15 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
@@ -40,6 +40,7 @@ Optional properties:
   - usb3-lpm-capable: determines if platform is USB3 LPM capable
   - quirk-broken-port-ped: set if the controller has broken port disable mechanism
   - imod-interval-ns: default interrupt moderation interval is 5000ns
+  - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect
   - phys : see usb-hcd.txt in the current directory
 
 additionally the properties from usb-hcd.txt (in the current directory) are
-- 
2.7.4

[1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property

[Srinath Mannam]

Add usb-phy-port-reset optional property to set quirk in xhci platform
driver which forces USB port PHY reset on port disconnect event.

Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
---
 Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index fea8b15..ecbdb15 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
@@ -40,6 +40,7 @@ Optional properties:
   - usb3-lpm-capable: determines if platform is USB3 LPM capable
   - quirk-broken-port-ped: set if the controller has broken port disable mechanism
   - imod-interval-ns: default interrupt moderation interval is 5000ns
+  - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect
   - phys : see usb-hcd.txt in the current directory
 
 additionally the properties from usb-hcd.txt (in the current directory) are

[PATCH 2/2] drivers: xhci: Add quirk to reset xHCI port PHY

[Srinath Mannam]

Add a quirk to reset xHCI port PHY on port disconnect event.
Stingray USB HS PHY has an issue, that USB High Speed device detected
at Full Speed after the same port has connected to Full speed device.
This problem can be resolved with that port PHY reset on disconnect.

Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/usb/core/hcd.c       |  6 ++++++
 drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
 drivers/usb/core/phy.h       |  1 +
 drivers/usb/host/xhci-plat.c |  3 +++
 drivers/usb/host/xhci-ring.c |  9 ++++++---
 drivers/usb/host/xhci.h      |  1 +
 include/linux/usb/hcd.h      |  1 +
 7 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 015b126..e2b87a6 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
 	return hcd->driver->find_raw_port_number(hcd, port1);
 }
 
+int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
+{
+	return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
+}
+EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
+
 static int usb_hcd_request_irqs(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags)
 {
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index 38b2c77..c64767d 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
 
+int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
+{
+	struct usb_phy_roothub *roothub_entry;
+	struct list_head *head;
+	int i = 0;
+
+	if (!phy_roothub)
+		return -EINVAL;
+
+	head = &phy_roothub->list;
+
+	list_for_each_entry(roothub_entry, head, list) {
+		if (i == port)
+			return phy_reset(roothub_entry->phy);
+		i++;
+	}
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_port_reset);
+
 int usb_phy_roothub_suspend(struct device *controller_dev,
 			    struct usb_phy_roothub *phy_roothub)
 {
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index 88a3c03..e8be444 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -18,6 +18,7 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
 
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
 void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
+int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port);
 
 int usb_phy_roothub_suspend(struct device *controller_dev,
 			    struct usb_phy_roothub *phy_roothub);
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ef09cb0..5a3b486 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -289,6 +289,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
 			xhci->quirks |= XHCI_BROKEN_PORT_PED;
 
+		if (device_property_read_bool(tmpdev, "usb-phy-port-reset"))
+			xhci->quirks |= XHCI_RESET_PHY_ON_DISCONNECT;
+
 		device_property_read_u32(tmpdev, "imod-interval-ns",
 					 &xhci->imod_interval);
 	}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 40fa25c..2dc3116 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1685,9 +1685,12 @@ static void handle_port_status(struct xhci_hcd *xhci,
 
 	if (hcd->speed < HCD_USB3) {
 		xhci_test_and_clear_bit(xhci, port, PORT_PLC);
-		if ((xhci->quirks & XHCI_RESET_PLL_ON_DISCONNECT) &&
-		    (portsc & PORT_CSC) && !(portsc & PORT_CONNECT))
-			xhci_cavium_reset_phy_quirk(xhci);
+		if ((portsc & PORT_CSC) && !(portsc & PORT_CONNECT)) {
+			if (xhci->quirks & XHCI_RESET_PLL_ON_DISCONNECT)
+				xhci_cavium_reset_phy_quirk(xhci);
+			else if (xhci->quirks & XHCI_RESET_PHY_ON_DISCONNECT)
+				usb_hcd_phy_port_reset(hcd, port_id - 1);
+		}
 	}
 
 cleanup:
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 652dc36..530c5ff 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1846,6 +1846,7 @@ struct xhci_hcd {
 #define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
 #define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
 #define XHCI_SNPS_BROKEN_SUSPEND    BIT_ULL(35)
+#define XHCI_RESET_PHY_ON_DISCONNECT	BIT_ULL(36)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 7dc3a41..a5ea26f 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -456,6 +456,7 @@ extern int usb_add_hcd(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags);
 extern void usb_remove_hcd(struct usb_hcd *hcd);
 extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1);
+extern int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port);
 
 struct platform_device;
 extern void usb_hcd_platform_shutdown(struct platform_device *dev);
-- 
2.7.4

[2/2] drivers: xhci: Add quirk to reset xHCI port PHY

[Srinath Mannam]

Add a quirk to reset xHCI port PHY on port disconnect event.
Stingray USB HS PHY has an issue, that USB High Speed device detected
at Full Speed after the same port has connected to Full speed device.
This problem can be resolved with that port PHY reset on disconnect.

Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/usb/core/hcd.c       |  6 ++++++
 drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
 drivers/usb/core/phy.h       |  1 +
 drivers/usb/host/xhci-plat.c |  3 +++
 drivers/usb/host/xhci-ring.c |  9 ++++++---
 drivers/usb/host/xhci.h      |  1 +
 include/linux/usb/hcd.h      |  1 +
 7 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 015b126..e2b87a6 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
 	return hcd->driver->find_raw_port_number(hcd, port1);
 }
 
+int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
+{
+	return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
+}
+EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
+
 static int usb_hcd_request_irqs(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags)
 {
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index 38b2c77..c64767d 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
 
+int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
+{
+	struct usb_phy_roothub *roothub_entry;
+	struct list_head *head;
+	int i = 0;
+
+	if (!phy_roothub)
+		return -EINVAL;
+
+	head = &phy_roothub->list;
+
+	list_for_each_entry(roothub_entry, head, list) {
+		if (i == port)
+			return phy_reset(roothub_entry->phy);
+		i++;
+	}
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_port_reset);
+
 int usb_phy_roothub_suspend(struct device *controller_dev,
 			    struct usb_phy_roothub *phy_roothub)
 {
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index 88a3c03..e8be444 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -18,6 +18,7 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
 
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
 void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
+int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port);
 
 int usb_phy_roothub_suspend(struct device *controller_dev,
 			    struct usb_phy_roothub *phy_roothub);
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ef09cb0..5a3b486 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -289,6 +289,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
 			xhci->quirks |= XHCI_BROKEN_PORT_PED;
 
+		if (device_property_read_bool(tmpdev, "usb-phy-port-reset"))
+			xhci->quirks |= XHCI_RESET_PHY_ON_DISCONNECT;
+
 		device_property_read_u32(tmpdev, "imod-interval-ns",
 					 &xhci->imod_interval);
 	}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 40fa25c..2dc3116 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1685,9 +1685,12 @@ static void handle_port_status(struct xhci_hcd *xhci,
 
 	if (hcd->speed < HCD_USB3) {
 		xhci_test_and_clear_bit(xhci, port, PORT_PLC);
-		if ((xhci->quirks & XHCI_RESET_PLL_ON_DISCONNECT) &&
-		    (portsc & PORT_CSC) && !(portsc & PORT_CONNECT))
-			xhci_cavium_reset_phy_quirk(xhci);
+		if ((portsc & PORT_CSC) && !(portsc & PORT_CONNECT)) {
+			if (xhci->quirks & XHCI_RESET_PLL_ON_DISCONNECT)
+				xhci_cavium_reset_phy_quirk(xhci);
+			else if (xhci->quirks & XHCI_RESET_PHY_ON_DISCONNECT)
+				usb_hcd_phy_port_reset(hcd, port_id - 1);
+		}
 	}
 
 cleanup:
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 652dc36..530c5ff 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1846,6 +1846,7 @@ struct xhci_hcd {
 #define XHCI_DEFAULT_PM_RUNTIME_ALLOW	BIT_ULL(33)
 #define XHCI_RESET_PLL_ON_DISCONNECT	BIT_ULL(34)
 #define XHCI_SNPS_BROKEN_SUSPEND    BIT_ULL(35)
+#define XHCI_RESET_PHY_ON_DISCONNECT	BIT_ULL(36)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 7dc3a41..a5ea26f 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -456,6 +456,7 @@ extern int usb_add_hcd(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags);
 extern void usb_remove_hcd(struct usb_hcd *hcd);
 extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1);
+extern int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port);
 
 struct platform_device;
 extern void usb_hcd_platform_shutdown(struct platform_device *dev);

Re: [PATCH 2/2] drivers: xhci: Add quirk to reset xHCI port PHY

[Mathias Nyman]

On 05.02.2019 08:18, Srinath Mannam wrote:
> Add a quirk to reset xHCI port PHY on port disconnect event.
> Stingray USB HS PHY has an issue, that USB High Speed device detected
> at Full Speed after the same port has connected to Full speed device.
> This problem can be resolved with that port PHY reset on disconnect.
> 
> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> ---
>   drivers/usb/core/hcd.c       |  6 ++++++
>   drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
>   drivers/usb/core/phy.h       |  1 +
>   drivers/usb/host/xhci-plat.c |  3 +++
>   drivers/usb/host/xhci-ring.c |  9 ++++++---
>   drivers/usb/host/xhci.h      |  1 +
>   include/linux/usb/hcd.h      |  1 +
>   7 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 015b126..e2b87a6 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
>   	return hcd->driver->find_raw_port_number(hcd, port1);
>   }
>   
> +int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
> +{
> +	return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
> +}
> +EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
> +
>   static int usb_hcd_request_irqs(struct usb_hcd *hcd,
>   		unsigned int irqnum, unsigned long irqflags)
>   {
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 38b2c77..c64767d 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>   }
>   EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
>   
> +int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct list_head *head;
> +	int i = 0;
> +
> +	if (!phy_roothub)
> +		return -EINVAL;
> +
> +	head = &phy_roothub->list;
> +
> +	list_for_each_entry(roothub_entry, head, list) {
> +		if (i == port)
> +			return phy_reset(roothub_entry->phy);
> +		i++;
> +	}

I'm not that familiar with SoC's that have several PHYs per controller,
but this looks odd.

For the above code to work wouldn't it require that each port has their own PHY,
and the PHYs are added to the list of usb_phy_roothub is in the same order as usb ports?

Or is there something I don't understand here?

-Mathias

[2/2] drivers: xhci: Add quirk to reset xHCI port PHY

[Mathias Nyman]

On 05.02.2019 08:18, Srinath Mannam wrote:
> Add a quirk to reset xHCI port PHY on port disconnect event.
> Stingray USB HS PHY has an issue, that USB High Speed device detected
> at Full Speed after the same port has connected to Full speed device.
> This problem can be resolved with that port PHY reset on disconnect.
> 
> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> ---
>   drivers/usb/core/hcd.c       |  6 ++++++
>   drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
>   drivers/usb/core/phy.h       |  1 +
>   drivers/usb/host/xhci-plat.c |  3 +++
>   drivers/usb/host/xhci-ring.c |  9 ++++++---
>   drivers/usb/host/xhci.h      |  1 +
>   include/linux/usb/hcd.h      |  1 +
>   7 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 015b126..e2b87a6 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
>   	return hcd->driver->find_raw_port_number(hcd, port1);
>   }
>   
> +int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
> +{
> +	return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
> +}
> +EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
> +
>   static int usb_hcd_request_irqs(struct usb_hcd *hcd,
>   		unsigned int irqnum, unsigned long irqflags)
>   {
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 38b2c77..c64767d 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>   }
>   EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
>   
> +int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct list_head *head;
> +	int i = 0;
> +
> +	if (!phy_roothub)
> +		return -EINVAL;
> +
> +	head = &phy_roothub->list;
> +
> +	list_for_each_entry(roothub_entry, head, list) {
> +		if (i == port)
> +			return phy_reset(roothub_entry->phy);
> +		i++;
> +	}

I'm not that familiar with SoC's that have several PHYs per controller,
but this looks odd.

For the above code to work wouldn't it require that each port has their own PHY,
and the PHYs are added to the list of usb_phy_roothub is in the same order as usb ports?

Or is there something I don't understand here?

-Mathias

Re: [PATCH 2/2] drivers: xhci: Add quirk to reset xHCI port PHY

[Srinath Mannam]

Hi Mathias,

Thanks for review, please see my comments below inline.

On Thu, Feb 7, 2019 at 8:32 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 05.02.2019 08:18, Srinath Mannam wrote:
> > Add a quirk to reset xHCI port PHY on port disconnect event.
> > Stingray USB HS PHY has an issue, that USB High Speed device detected
> > at Full Speed after the same port has connected to Full speed device.
> > This problem can be resolved with that port PHY reset on disconnect.
> >
> > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > ---
> >   drivers/usb/core/hcd.c       |  6 ++++++
> >   drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
> >   drivers/usb/core/phy.h       |  1 +
> >   drivers/usb/host/xhci-plat.c |  3 +++
> >   drivers/usb/host/xhci-ring.c |  9 ++++++---
> >   drivers/usb/host/xhci.h      |  1 +
> >   include/linux/usb/hcd.h      |  1 +
> >   7 files changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 015b126..e2b87a6 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
> >       return hcd->driver->find_raw_port_number(hcd, port1);
> >   }
> >
> > +int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
> > +{
> > +     return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
> > +}
> > +EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
> > +
> >   static int usb_hcd_request_irqs(struct usb_hcd *hcd,
> >               unsigned int irqnum, unsigned long irqflags)
> >   {
> > diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> > index 38b2c77..c64767d 100644
> > --- a/drivers/usb/core/phy.c
> > +++ b/drivers/usb/core/phy.c
> > @@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
> >   }
> >   EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> >
> > +int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
> > +{
> > +     struct usb_phy_roothub *roothub_entry;
> > +     struct list_head *head;
> > +     int i = 0;
> > +
> > +     if (!phy_roothub)
> > +             return -EINVAL;
> > +
> > +     head = &phy_roothub->list;
> > +
> > +     list_for_each_entry(roothub_entry, head, list) {
> > +             if (i == port)
> > +                     return phy_reset(roothub_entry->phy);
> > +             i++;
> > +     }
>
> I'm not that familiar with SoC's that have several PHYs per controller,
> but this looks odd.
>
> For the above code to work wouldn't it require that each port has their own PHY,
> and the PHYs are added to the list of usb_phy_roothub is in the same order as usb ports?
>
> Or is there something I don't understand here?
>
In our SOC (Stingray), xHCI controller has three ports and each port
connected to separate PHY.
Stingray xHCI controller supports both SS and HS ports and connected
separate PHYs.
We passed PHY phandlers in xHCI DT node in the order of port numbers.
as shown below xHCI DT node.
So that all PHYs added to usb_phy_roothub are in order of port numbers.
 xhci1: usb@11000 {
                        compatible = "generic-xhci";
                        reg = <0x00011000 0x1000>;
                        interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH>;
                        phys = <&usb1_phy1>, <&usbphy2>, <&usb1_phy0>;
                        phy-names = "phy0", "phy1", "phy2";
                        dma-coherent;
                        status = "disabled";
};
But we have issue with HS PHYs, so that those PHYs are required to reset.

Regards,
Srinath.
> -Mathias
>

[2/2] drivers: xhci: Add quirk to reset xHCI port PHY

[Srinath Mannam]

Hi Mathias,

Thanks for review, please see my comments below inline.

On Thu, Feb 7, 2019 at 8:32 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 05.02.2019 08:18, Srinath Mannam wrote:
> > Add a quirk to reset xHCI port PHY on port disconnect event.
> > Stingray USB HS PHY has an issue, that USB High Speed device detected
> > at Full Speed after the same port has connected to Full speed device.
> > This problem can be resolved with that port PHY reset on disconnect.
> >
> > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > ---
> >   drivers/usb/core/hcd.c       |  6 ++++++
> >   drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
> >   drivers/usb/core/phy.h       |  1 +
> >   drivers/usb/host/xhci-plat.c |  3 +++
> >   drivers/usb/host/xhci-ring.c |  9 ++++++---
> >   drivers/usb/host/xhci.h      |  1 +
> >   include/linux/usb/hcd.h      |  1 +
> >   7 files changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 015b126..e2b87a6 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
> >       return hcd->driver->find_raw_port_number(hcd, port1);
> >   }
> >
> > +int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
> > +{
> > +     return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
> > +}
> > +EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
> > +
> >   static int usb_hcd_request_irqs(struct usb_hcd *hcd,
> >               unsigned int irqnum, unsigned long irqflags)
> >   {
> > diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> > index 38b2c77..c64767d 100644
> > --- a/drivers/usb/core/phy.c
> > +++ b/drivers/usb/core/phy.c
> > @@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
> >   }
> >   EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> >
> > +int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
> > +{
> > +     struct usb_phy_roothub *roothub_entry;
> > +     struct list_head *head;
> > +     int i = 0;
> > +
> > +     if (!phy_roothub)
> > +             return -EINVAL;
> > +
> > +     head = &phy_roothub->list;
> > +
> > +     list_for_each_entry(roothub_entry, head, list) {
> > +             if (i == port)
> > +                     return phy_reset(roothub_entry->phy);
> > +             i++;
> > +     }
>
> I'm not that familiar with SoC's that have several PHYs per controller,
> but this looks odd.
>
> For the above code to work wouldn't it require that each port has their own PHY,
> and the PHYs are added to the list of usb_phy_roothub is in the same order as usb ports?
>
> Or is there something I don't understand here?
>
In our SOC (Stingray), xHCI controller has three ports and each port
connected to separate PHY.
Stingray xHCI controller supports both SS and HS ports and connected
separate PHYs.
We passed PHY phandlers in xHCI DT node in the order of port numbers.
as shown below xHCI DT node.
So that all PHYs added to usb_phy_roothub are in order of port numbers.
 xhci1: usb@11000 {
                        compatible = "generic-xhci";
                        reg = <0x00011000 0x1000>;
                        interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH>;
                        phys = <&usb1_phy1>, <&usbphy2>, <&usb1_phy0>;
                        phy-names = "phy0", "phy1", "phy2";
                        dma-coherent;
                        status = "disabled";
};
But we have issue with HS PHYs, so that those PHYs are required to reset.

Regards,
Srinath.
> -Mathias
>

Re: [PATCH 2/2] drivers: xhci: Add quirk to reset xHCI port PHY

[Mathias Nyman]

On 07.02.2019 17:17, Srinath Mannam wrote:
> Hi Mathias,
> 
> Thanks for review, please see my comments below inline.
> 
> On Thu, Feb 7, 2019 at 8:32 PM Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>>
>> On 05.02.2019 08:18, Srinath Mannam wrote:
>>> Add a quirk to reset xHCI port PHY on port disconnect event.
>>> Stingray USB HS PHY has an issue, that USB High Speed device detected
>>> at Full Speed after the same port has connected to Full speed device.
>>> This problem can be resolved with that port PHY reset on disconnect.
>>>
>>> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
>>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>> ---
>>>    drivers/usb/core/hcd.c       |  6 ++++++
>>>    drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
>>>    drivers/usb/core/phy.h       |  1 +
>>>    drivers/usb/host/xhci-plat.c |  3 +++
>>>    drivers/usb/host/xhci-ring.c |  9 ++++++---
>>>    drivers/usb/host/xhci.h      |  1 +
>>>    include/linux/usb/hcd.h      |  1 +
>>>    7 files changed, 39 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>>> index 015b126..e2b87a6 100644
>>> --- a/drivers/usb/core/hcd.c
>>> +++ b/drivers/usb/core/hcd.c
>>> @@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
>>>        return hcd->driver->find_raw_port_number(hcd, port1);
>>>    }
>>>
>>> +int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
>>> +{
>>> +     return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
>>> +}
>>> +EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
>>> +
>>>    static int usb_hcd_request_irqs(struct usb_hcd *hcd,
>>>                unsigned int irqnum, unsigned long irqflags)
>>>    {
>>> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
>>> index 38b2c77..c64767d 100644
>>> --- a/drivers/usb/core/phy.c
>>> +++ b/drivers/usb/core/phy.c
>>> @@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>>>    }
>>>    EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
>>>
>>> +int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
>>> +{
>>> +     struct usb_phy_roothub *roothub_entry;
>>> +     struct list_head *head;
>>> +     int i = 0;
>>> +
>>> +     if (!phy_roothub)
>>> +             return -EINVAL;
>>> +
>>> +     head = &phy_roothub->list;
>>> +
>>> +     list_for_each_entry(roothub_entry, head, list) {
>>> +             if (i == port)
>>> +                     return phy_reset(roothub_entry->phy);
>>> +             i++;
>>> +     }
>>
>> I'm not that familiar with SoC's that have several PHYs per controller,
>> but this looks odd.
>>
>> For the above code to work wouldn't it require that each port has their own PHY,
>> and the PHYs are added to the list of usb_phy_roothub is in the same order as usb ports?
>>
>> Or is there something I don't understand here?
>>
> In our SOC (Stingray), xHCI controller has three ports and each port
> connected to separate PHY.
> Stingray xHCI controller supports both SS and HS ports and connected
> separate PHYs.
> We passed PHY phandlers in xHCI DT node in the order of port numbers.
> as shown below xHCI DT node.
> So that all PHYs added to usb_phy_roothub are in order of port numbers.
>   xhci1: usb@11000 {
>                          compatible = "generic-xhci";
>                          reg = <0x00011000 0x1000>;
>                          interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH>;
>                          phys = <&usb1_phy1>, <&usbphy2>, <&usb1_phy0>;
>                          phy-names = "phy0", "phy1", "phy2";
>                          dma-coherent;
>                          status = "disabled";
> };
> But we have issue with HS PHYs, so that those PHYs are required to reset.

This is very specific to your SOC.
A quick grep shows most xhci controllers have one or two PHYs in total.

The suggested usb_phy_roothub_port_reset() above will only work for your SOC.
I think it would be better to have a stingray specific quirk/workaround where you
can find the right PHY to reset based on somthing like port number and PHY name.

For a generic solution we would need a better way to map usb ports to the PHY it uses.

-Mathias

[2/2] drivers: xhci: Add quirk to reset xHCI port PHY

[Mathias Nyman]

On 07.02.2019 17:17, Srinath Mannam wrote:
> Hi Mathias,
> 
> Thanks for review, please see my comments below inline.
> 
> On Thu, Feb 7, 2019 at 8:32 PM Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>>
>> On 05.02.2019 08:18, Srinath Mannam wrote:
>>> Add a quirk to reset xHCI port PHY on port disconnect event.
>>> Stingray USB HS PHY has an issue, that USB High Speed device detected
>>> at Full Speed after the same port has connected to Full speed device.
>>> This problem can be resolved with that port PHY reset on disconnect.
>>>
>>> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
>>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>> ---
>>>    drivers/usb/core/hcd.c       |  6 ++++++
>>>    drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
>>>    drivers/usb/core/phy.h       |  1 +
>>>    drivers/usb/host/xhci-plat.c |  3 +++
>>>    drivers/usb/host/xhci-ring.c |  9 ++++++---
>>>    drivers/usb/host/xhci.h      |  1 +
>>>    include/linux/usb/hcd.h      |  1 +
>>>    7 files changed, 39 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>>> index 015b126..e2b87a6 100644
>>> --- a/drivers/usb/core/hcd.c
>>> +++ b/drivers/usb/core/hcd.c
>>> @@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
>>>        return hcd->driver->find_raw_port_number(hcd, port1);
>>>    }
>>>
>>> +int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
>>> +{
>>> +     return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
>>> +}
>>> +EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
>>> +
>>>    static int usb_hcd_request_irqs(struct usb_hcd *hcd,
>>>                unsigned int irqnum, unsigned long irqflags)
>>>    {
>>> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
>>> index 38b2c77..c64767d 100644
>>> --- a/drivers/usb/core/phy.c
>>> +++ b/drivers/usb/core/phy.c
>>> @@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>>>    }
>>>    EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
>>>
>>> +int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
>>> +{
>>> +     struct usb_phy_roothub *roothub_entry;
>>> +     struct list_head *head;
>>> +     int i = 0;
>>> +
>>> +     if (!phy_roothub)
>>> +             return -EINVAL;
>>> +
>>> +     head = &phy_roothub->list;
>>> +
>>> +     list_for_each_entry(roothub_entry, head, list) {
>>> +             if (i == port)
>>> +                     return phy_reset(roothub_entry->phy);
>>> +             i++;
>>> +     }
>>
>> I'm not that familiar with SoC's that have several PHYs per controller,
>> but this looks odd.
>>
>> For the above code to work wouldn't it require that each port has their own PHY,
>> and the PHYs are added to the list of usb_phy_roothub is in the same order as usb ports?
>>
>> Or is there something I don't understand here?
>>
> In our SOC (Stingray), xHCI controller has three ports and each port
> connected to separate PHY.
> Stingray xHCI controller supports both SS and HS ports and connected
> separate PHYs.
> We passed PHY phandlers in xHCI DT node in the order of port numbers.
> as shown below xHCI DT node.
> So that all PHYs added to usb_phy_roothub are in order of port numbers.
>   xhci1: usb@11000 {
>                          compatible = "generic-xhci";
>                          reg = <0x00011000 0x1000>;
>                          interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH>;
>                          phys = <&usb1_phy1>, <&usbphy2>, <&usb1_phy0>;
>                          phy-names = "phy0", "phy1", "phy2";
>                          dma-coherent;
>                          status = "disabled";
> };
> But we have issue with HS PHYs, so that those PHYs are required to reset.

This is very specific to your SOC.
A quick grep shows most xhci controllers have one or two PHYs in total.

The suggested usb_phy_roothub_port_reset() above will only work for your SOC.
I think it would be better to have a stingray specific quirk/workaround where you
can find the right PHY to reset based on somthing like port number and PHY name.

For a generic solution we would need a better way to map usb ports to the PHY it uses.

-Mathias

Re: [PATCH 2/2] drivers: xhci: Add quirk to reset xHCI port PHY

[Srinath Mannam]

Hi Mathias,

Thanks for comments, Please find my comments below inline.

On Fri, Feb 8, 2019 at 6:00 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 07.02.2019 17:17, Srinath Mannam wrote:
> > Hi Mathias,
> >
> > Thanks for review, please see my comments below inline.
> >
> > On Thu, Feb 7, 2019 at 8:32 PM Mathias Nyman
> > <mathias.nyman@linux.intel.com> wrote:
> >>
> >> On 05.02.2019 08:18, Srinath Mannam wrote:
> >>> Add a quirk to reset xHCI port PHY on port disconnect event.
> >>> Stingray USB HS PHY has an issue, that USB High Speed device detected
> >>> at Full Speed after the same port has connected to Full speed device.
> >>> This problem can be resolved with that port PHY reset on disconnect.
> >>>
> >>> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> >>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> >>> ---
> >>>    drivers/usb/core/hcd.c       |  6 ++++++
> >>>    drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
> >>>    drivers/usb/core/phy.h       |  1 +
> >>>    drivers/usb/host/xhci-plat.c |  3 +++
> >>>    drivers/usb/host/xhci-ring.c |  9 ++++++---
> >>>    drivers/usb/host/xhci.h      |  1 +
> >>>    include/linux/usb/hcd.h      |  1 +
> >>>    7 files changed, 39 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> >>> index 015b126..e2b87a6 100644
> >>> --- a/drivers/usb/core/hcd.c
> >>> +++ b/drivers/usb/core/hcd.c
> >>> @@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
> >>>        return hcd->driver->find_raw_port_number(hcd, port1);
> >>>    }
> >>>
> >>> +int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
> >>> +{
> >>> +     return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
> >>> +
> >>>    static int usb_hcd_request_irqs(struct usb_hcd *hcd,
> >>>                unsigned int irqnum, unsigned long irqflags)
> >>>    {
> >>> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> >>> index 38b2c77..c64767d 100644
> >>> --- a/drivers/usb/core/phy.c
> >>> +++ b/drivers/usb/core/phy.c
> >>> @@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
> >>>    }
> >>>    EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> >>>
> >>> +int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
> >>> +{
> >>> +     struct usb_phy_roothub *roothub_entry;
> >>> +     struct list_head *head;
> >>> +     int i = 0;
> >>> +
> >>> +     if (!phy_roothub)
> >>> +             return -EINVAL;
> >>> +
> >>> +     head = &phy_roothub->list;
> >>> +
> >>> +     list_for_each_entry(roothub_entry, head, list) {
> >>> +             if (i == port)
> >>> +                     return phy_reset(roothub_entry->phy);
> >>> +             i++;
> >>> +     }
> >>
> >> I'm not that familiar with SoC's that have several PHYs per controller,
> >> but this looks odd.
> >>
> >> For the above code to work wouldn't it require that each port has their own PHY,
> >> and the PHYs are added to the list of usb_phy_roothub is in the same order as usb ports?
> >>
> >> Or is there something I don't understand here?
> >>
> > In our SOC (Stingray), xHCI controller has three ports and each port
> > connected to separate PHY.
> > Stingray xHCI controller supports both SS and HS ports and connected
> > separate PHYs.
> > We passed PHY phandlers in xHCI DT node in the order of port numbers.
> > as shown below xHCI DT node.
> > So that all PHYs added to usb_phy_roothub are in order of port numbers.
> >   xhci1: usb@11000 {
> >                          compatible = "generic-xhci";
> >                          reg = <0x00011000 0x1000>;
> >                          interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH>;
> >                          phys = <&usb1_phy1>, <&usbphy2>, <&usb1_phy0>;
> >                          phy-names = "phy0", "phy1", "phy2";
> >                          dma-coherent;
> >                          status = "disabled";
> > };
> > But we have issue with HS PHYs, so that those PHYs are required to reset.
>
> This is very specific to your SOC.
> A quick grep shows most xhci controllers have one or two PHYs in total.
>
> The suggested usb_phy_roothub_port_reset() above will only work for your SOC.
Yes, and Cavium SOC also has similar issue need to reset PLL.
> I think it would be better to have a stingray specific quirk/workaround where you
> can find the right PHY to reset based on somthing like port number and PHY name.
>
The tricky part is we need to reset HS PHY after port disconnect so
that PORT to PHY translation is required.
We have two xHCI controllers in our SOC, one has two ports with two
PHYs (SS + HS) and other has three ports with three PHYs (SS + HS +
HS).
All HS PHYs have this issue, we need to reset HS PHYs after disconnect
of its corresponding port.
In existing framework all phy pointers are listed in primary hcd based
on index using "devm_of_phy_get_by_index" so can rely on index number
rather PHY name?
> For a generic solution we would need a better way to map usb ports to the PHY it uses.
Can I add phy pointer and its details in "struct xhci_port" structure
while initialization to get phy pointer from xhci_port?
>
Regards,
Srinath.



> -Mathias

[2/2] drivers: xhci: Add quirk to reset xHCI port PHY

[Srinath Mannam]

Hi Mathias,

Thanks for comments, Please find my comments below inline.

On Fri, Feb 8, 2019 at 6:00 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 07.02.2019 17:17, Srinath Mannam wrote:
> > Hi Mathias,
> >
> > Thanks for review, please see my comments below inline.
> >
> > On Thu, Feb 7, 2019 at 8:32 PM Mathias Nyman
> > <mathias.nyman@linux.intel.com> wrote:
> >>
> >> On 05.02.2019 08:18, Srinath Mannam wrote:
> >>> Add a quirk to reset xHCI port PHY on port disconnect event.
> >>> Stingray USB HS PHY has an issue, that USB High Speed device detected
> >>> at Full Speed after the same port has connected to Full speed device.
> >>> This problem can be resolved with that port PHY reset on disconnect.
> >>>
> >>> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> >>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> >>> ---
> >>>    drivers/usb/core/hcd.c       |  6 ++++++
> >>>    drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
> >>>    drivers/usb/core/phy.h       |  1 +
> >>>    drivers/usb/host/xhci-plat.c |  3 +++
> >>>    drivers/usb/host/xhci-ring.c |  9 ++++++---
> >>>    drivers/usb/host/xhci.h      |  1 +
> >>>    include/linux/usb/hcd.h      |  1 +
> >>>    7 files changed, 39 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> >>> index 015b126..e2b87a6 100644
> >>> --- a/drivers/usb/core/hcd.c
> >>> +++ b/drivers/usb/core/hcd.c
> >>> @@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
> >>>        return hcd->driver->find_raw_port_number(hcd, port1);
> >>>    }
> >>>
> >>> +int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
> >>> +{
> >>> +     return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
> >>> +
> >>>    static int usb_hcd_request_irqs(struct usb_hcd *hcd,
> >>>                unsigned int irqnum, unsigned long irqflags)
> >>>    {
> >>> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> >>> index 38b2c77..c64767d 100644
> >>> --- a/drivers/usb/core/phy.c
> >>> +++ b/drivers/usb/core/phy.c
> >>> @@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
> >>>    }
> >>>    EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> >>>
> >>> +int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
> >>> +{
> >>> +     struct usb_phy_roothub *roothub_entry;
> >>> +     struct list_head *head;
> >>> +     int i = 0;
> >>> +
> >>> +     if (!phy_roothub)
> >>> +             return -EINVAL;
> >>> +
> >>> +     head = &phy_roothub->list;
> >>> +
> >>> +     list_for_each_entry(roothub_entry, head, list) {
> >>> +             if (i == port)
> >>> +                     return phy_reset(roothub_entry->phy);
> >>> +             i++;
> >>> +     }
> >>
> >> I'm not that familiar with SoC's that have several PHYs per controller,
> >> but this looks odd.
> >>
> >> For the above code to work wouldn't it require that each port has their own PHY,
> >> and the PHYs are added to the list of usb_phy_roothub is in the same order as usb ports?
> >>
> >> Or is there something I don't understand here?
> >>
> > In our SOC (Stingray), xHCI controller has three ports and each port
> > connected to separate PHY.
> > Stingray xHCI controller supports both SS and HS ports and connected
> > separate PHYs.
> > We passed PHY phandlers in xHCI DT node in the order of port numbers.
> > as shown below xHCI DT node.
> > So that all PHYs added to usb_phy_roothub are in order of port numbers.
> >   xhci1: usb@11000 {
> >                          compatible = "generic-xhci";
> >                          reg = <0x00011000 0x1000>;
> >                          interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH>;
> >                          phys = <&usb1_phy1>, <&usbphy2>, <&usb1_phy0>;
> >                          phy-names = "phy0", "phy1", "phy2";
> >                          dma-coherent;
> >                          status = "disabled";
> > };
> > But we have issue with HS PHYs, so that those PHYs are required to reset.
>
> This is very specific to your SOC.
> A quick grep shows most xhci controllers have one or two PHYs in total.
>
> The suggested usb_phy_roothub_port_reset() above will only work for your SOC.
Yes, and Cavium SOC also has similar issue need to reset PLL.
> I think it would be better to have a stingray specific quirk/workaround where you
> can find the right PHY to reset based on somthing like port number and PHY name.
>
The tricky part is we need to reset HS PHY after port disconnect so
that PORT to PHY translation is required.
We have two xHCI controllers in our SOC, one has two ports with two
PHYs (SS + HS) and other has three ports with three PHYs (SS + HS +
HS).
All HS PHYs have this issue, we need to reset HS PHYs after disconnect
of its corresponding port.
In existing framework all phy pointers are listed in primary hcd based
on index using "devm_of_phy_get_by_index" so can rely on index number
rather PHY name?
> For a generic solution we would need a better way to map usb ports to the PHY it uses.
Can I add phy pointer and its details in "struct xhci_port" structure
while initialization to get phy pointer from xhci_port?
>
Regards,
Srinath.



> -Mathias

Re: [PATCH 1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property

[Rob Herring]

On Tue, Feb 05, 2019 at 11:48:53AM +0530, Srinath Mannam wrote:
> Add usb-phy-port-reset optional property to set quirk in xhci platform
> driver which forces USB port PHY reset on port disconnect event.
> 
> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> index fea8b15..ecbdb15 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> @@ -40,6 +40,7 @@ Optional properties:
>    - usb3-lpm-capable: determines if platform is USB3 LPM capable
>    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
>    - imod-interval-ns: default interrupt moderation interval is 5000ns
> +  - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect
>    - phys : see usb-hcd.txt in the current directory

This should be implied by the HCI or phy compatible string (depending 
on who exactly needs the quirky behavior).

Rob

[1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property

[Rob Herring]

On Tue, Feb 05, 2019 at 11:48:53AM +0530, Srinath Mannam wrote:
> Add usb-phy-port-reset optional property to set quirk in xhci platform
> driver which forces USB port PHY reset on port disconnect event.
> 
> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> index fea8b15..ecbdb15 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> @@ -40,6 +40,7 @@ Optional properties:
>    - usb3-lpm-capable: determines if platform is USB3 LPM capable
>    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
>    - imod-interval-ns: default interrupt moderation interval is 5000ns
> +  - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect
>    - phys : see usb-hcd.txt in the current directory

This should be implied by the HCI or phy compatible string (depending 
on who exactly needs the quirky behavior).

Rob

Re: [PATCH 1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property

[Srinath Mannam]

Hi Rob,
Thanks for the review, Please see my comments below in line.

Regards,
Srinath.
On Tue, Feb 26, 2019 at 3:08 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Feb 05, 2019 at 11:48:53AM +0530, Srinath Mannam wrote:
> > Add usb-phy-port-reset optional property to set quirk in xhci platform
> > driver which forces USB port PHY reset on port disconnect event.
> >
> > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > ---
> >  Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > index fea8b15..ecbdb15 100644
> > --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > @@ -40,6 +40,7 @@ Optional properties:
> >    - usb3-lpm-capable: determines if platform is USB3 LPM capable
> >    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
> >    - imod-interval-ns: default interrupt moderation interval is 5000ns
> > +  - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect
> >    - phys : see usb-hcd.txt in the current directory
>
> This should be implied by the HCI or phy compatible string (depending
> on who exactly needs the quirky behavior).
Stingray USB HS PHY connected to xHCI port has an issue, if full speed
devices connected to this port then
after all High Speed devices connected to this port are detected at
full speed instead of high speed.
So that we need to do PHY (which is connected to port) reset on xHCI
port disconnect event.
That is the reason we required to add quirk in xHCI.
>
> Rob

[1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property

[Srinath Mannam]

Hi Rob,
Thanks for the review, Please see my comments below in line.

Regards,
Srinath.
On Tue, Feb 26, 2019 at 3:08 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Feb 05, 2019 at 11:48:53AM +0530, Srinath Mannam wrote:
> > Add usb-phy-port-reset optional property to set quirk in xhci platform
> > driver which forces USB port PHY reset on port disconnect event.
> >
> > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > ---
> >  Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > index fea8b15..ecbdb15 100644
> > --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > @@ -40,6 +40,7 @@ Optional properties:
> >    - usb3-lpm-capable: determines if platform is USB3 LPM capable
> >    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
> >    - imod-interval-ns: default interrupt moderation interval is 5000ns
> > +  - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect
> >    - phys : see usb-hcd.txt in the current directory
>
> This should be implied by the HCI or phy compatible string (depending
> on who exactly needs the quirky behavior).
Stingray USB HS PHY connected to xHCI port has an issue, if full speed
devices connected to this port then
after all High Speed devices connected to this port are detected at
full speed instead of high speed.
So that we need to do PHY (which is connected to port) reset on xHCI
port disconnect event.
That is the reason we required to add quirk in xHCI.
>
> Rob

Re: [PATCH 1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property

[Rob Herring]

On Mon, Feb 25, 2019 at 10:57 PM Srinath Mannam
<srinath.mannam@broadcom.com> wrote:
>
> Hi Rob,
> Thanks for the review, Please see my comments below in line.
>
> Regards,
> Srinath.
> On Tue, Feb 26, 2019 at 3:08 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Feb 05, 2019 at 11:48:53AM +0530, Srinath Mannam wrote:
> > > Add usb-phy-port-reset optional property to set quirk in xhci platform
> > > driver which forces USB port PHY reset on port disconnect event.
> > >
> > > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > ---
> > >  Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > index fea8b15..ecbdb15 100644
> > > --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > @@ -40,6 +40,7 @@ Optional properties:
> > >    - usb3-lpm-capable: determines if platform is USB3 LPM capable
> > >    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
> > >    - imod-interval-ns: default interrupt moderation interval is 5000ns
> > > +  - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect
> > >    - phys : see usb-hcd.txt in the current directory
> >
> > This should be implied by the HCI or phy compatible string (depending
> > on who exactly needs the quirky behavior).
> Stingray USB HS PHY connected to xHCI port has an issue, if full speed
> devices connected to this port then
> after all High Speed devices connected to this port are detected at
> full speed instead of high speed.
> So that we need to do PHY (which is connected to port) reset on xHCI
> port disconnect event.
> That is the reason we required to add quirk in xHCI.

So, by looking at the xhci host and phy compatible strings (or maybe
just the phy) you can determine whether you need to reset the port or
not. All the information you need is in DT already.

Rob

[1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property

[Rob Herring]

On Mon, Feb 25, 2019 at 10:57 PM Srinath Mannam
<srinath.mannam@broadcom.com> wrote:
>
> Hi Rob,
> Thanks for the review, Please see my comments below in line.
>
> Regards,
> Srinath.
> On Tue, Feb 26, 2019 at 3:08 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Feb 05, 2019 at 11:48:53AM +0530, Srinath Mannam wrote:
> > > Add usb-phy-port-reset optional property to set quirk in xhci platform
> > > driver which forces USB port PHY reset on port disconnect event.
> > >
> > > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > ---
> > >  Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > index fea8b15..ecbdb15 100644
> > > --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > @@ -40,6 +40,7 @@ Optional properties:
> > >    - usb3-lpm-capable: determines if platform is USB3 LPM capable
> > >    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
> > >    - imod-interval-ns: default interrupt moderation interval is 5000ns
> > > +  - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect
> > >    - phys : see usb-hcd.txt in the current directory
> >
> > This should be implied by the HCI or phy compatible string (depending
> > on who exactly needs the quirky behavior).
> Stingray USB HS PHY connected to xHCI port has an issue, if full speed
> devices connected to this port then
> after all High Speed devices connected to this port are detected at
> full speed instead of high speed.
> So that we need to do PHY (which is connected to port) reset on xHCI
> port disconnect event.
> That is the reason we required to add quirk in xHCI.

So, by looking at the xhci host and phy compatible strings (or maybe
just the phy) you can determine whether you need to reset the port or
not. All the information you need is in DT already.

Rob

Re: [PATCH 1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property

[Srinath Mannam]

Hi Rob,
Thanks for the information. Please find my comments below.
Regards,
Srinath.
On Tue, Feb 26, 2019 at 11:33 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Feb 25, 2019 at 10:57 PM Srinath Mannam
> <srinath.mannam@broadcom.com> wrote:
> >
> > Hi Rob,
> > Thanks for the review, Please see my comments below in line.
> >
> > Regards,
> > Srinath.
> > On Tue, Feb 26, 2019 at 3:08 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Feb 05, 2019 at 11:48:53AM +0530, Srinath Mannam wrote:
> > > > Add usb-phy-port-reset optional property to set quirk in xhci platform
> > > > driver which forces USB port PHY reset on port disconnect event.
> > > >
> > > > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > > index fea8b15..ecbdb15 100644
> > > > --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > > @@ -40,6 +40,7 @@ Optional properties:
> > > >    - usb3-lpm-capable: determines if platform is USB3 LPM capable
> > > >    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
> > > >    - imod-interval-ns: default interrupt moderation interval is 5000ns
> > > > +  - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect
> > > >    - phys : see usb-hcd.txt in the current directory
> > >
> > > This should be implied by the HCI or phy compatible string (depending
> > > on who exactly needs the quirky behavior).
> > Stingray USB HS PHY connected to xHCI port has an issue, if full speed
> > devices connected to this port then
> > after all High Speed devices connected to this port are detected at
> > full speed instead of high speed.
> > So that we need to do PHY (which is connected to port) reset on xHCI
> > port disconnect event.
> > That is the reason we required to add quirk in xHCI.
>
> So, by looking at the xhci host and phy compatible strings (or maybe
> just the phy) you can determine whether you need to reset the port or
> not. All the information you need is in DT already.
xHCI controller in our SOC has three ports each port has one
PHY(SS/HS) connected to it.
HS PHY has to reset on its corresponding port disconnect event. port
disconnect event is
captured in xHCI host framework so, quirk has to be registered in xHCI
framework only.
But we are using "generic-xhci" generic compatible string for our xHCI
controller.
As per your advice, we will add new compatible string in xhci-plat.c
driver for our xHCI controller
and will add quirk part of that.

Thank you.

>
> Rob

[1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property

[Srinath Mannam]

Hi Rob,
Thanks for the information. Please find my comments below.
Regards,
Srinath.
On Tue, Feb 26, 2019 at 11:33 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Feb 25, 2019 at 10:57 PM Srinath Mannam
> <srinath.mannam@broadcom.com> wrote:
> >
> > Hi Rob,
> > Thanks for the review, Please see my comments below in line.
> >
> > Regards,
> > Srinath.
> > On Tue, Feb 26, 2019 at 3:08 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Feb 05, 2019 at 11:48:53AM +0530, Srinath Mannam wrote:
> > > > Add usb-phy-port-reset optional property to set quirk in xhci platform
> > > > driver which forces USB port PHY reset on port disconnect event.
> > > >
> > > > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> > > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > > index fea8b15..ecbdb15 100644
> > > > --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > > > @@ -40,6 +40,7 @@ Optional properties:
> > > >    - usb3-lpm-capable: determines if platform is USB3 LPM capable
> > > >    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
> > > >    - imod-interval-ns: default interrupt moderation interval is 5000ns
> > > > +  - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect
> > > >    - phys : see usb-hcd.txt in the current directory
> > >
> > > This should be implied by the HCI or phy compatible string (depending
> > > on who exactly needs the quirky behavior).
> > Stingray USB HS PHY connected to xHCI port has an issue, if full speed
> > devices connected to this port then
> > after all High Speed devices connected to this port are detected at
> > full speed instead of high speed.
> > So that we need to do PHY (which is connected to port) reset on xHCI
> > port disconnect event.
> > That is the reason we required to add quirk in xHCI.
>
> So, by looking at the xhci host and phy compatible strings (or maybe
> just the phy) you can determine whether you need to reset the port or
> not. All the information you need is in DT already.
xHCI controller in our SOC has three ports each port has one
PHY(SS/HS) connected to it.
HS PHY has to reset on its corresponding port disconnect event. port
disconnect event is
captured in xHCI host framework so, quirk has to be registered in xHCI
framework only.
But we are using "generic-xhci" generic compatible string for our xHCI
controller.
As per your advice, we will add new compatible string in xhci-plat.c
driver for our xHCI controller
and will add quirk part of that.

Thank you.

>
> Rob