On Sat, Oct 07, 2023 at 09:18:02PM +0530, Krishna Kurapati wrote:Can you help elaborate on your comment ? Do you mean that this API doesn't get speed on all ports, but this has to be called in a loop to get all the port speeds ? In that sense, I agree, I can change the comments here.
Currently wakeup is supported by only single port controllers. Read speed
of each port and accordingly enable IRQ's for those ports.
Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
---
drivers/usb/dwc3/dwc3-qcom.c | 65 +++++++++++++++++++-----------------
1 file changed, 35 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 863892284146..651b9775a0c2 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -90,7 +90,7 @@ struct dwc3_qcom {
*/
int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
int hs_phy_irq;
- enum usb_device_speed usb2_speed;
+ enum usb_device_speed usb2_speed[DWC3_MAX_PORTS];
This also belongs in a new port structure.
struct extcon_dev *edev;
struct extcon_dev *host_edev;
@@ -335,7 +335,8 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
return dwc->xhci;
}
-static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
+static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom,
+ int port_index)
No need for line break (since it's a function definition).
{
struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
struct usb_device *udev;
@@ -348,12 +349,10 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
/*
* It is possible to query the speed of all children of
- * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code
- * currently supports only 1 port per controller. So
- * this is sufficient.
+ * USB2.0 root hub via usb_hub_for_each_child().
This comment no longer makes sense with your current implementation.
But perhaps this should be done using usb_hub_for_each_child() insteadEither ways is fine. We would have qcom->num_ports to determine how many speeds we can read.
as that may be more efficient. Then you use this function to read out
the speed for all the ports in go (and store it in the port structures I
mentioned). Please determine which alternative is best.
*/
#ifdef CONFIG_USB
- udev = usb_hub_find_child(hcd->self.root_hub, 1);
+ udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
#else
udev = NULL;
#endif
@@ -386,23 +385,29 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
{
+ int i;
+
dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
- if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
- } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
- (qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
- } else {
- dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
- dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
- }
+ for (i = 0; i < qcom->num_ports; i++) {
+ if (qcom->usb2_speed[i] == USB_SPEED_LOW) {
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]);
+ } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) ||
+ (qcom->usb2_speed[i] == USB_SPEED_FULL)) {
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]);
+ } else {
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]);
+ }
- dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i]);
+ }
}
The above is hardly readable, partly because of the 2d array that I
think you should drop, and partly because you add the port loop here
instead of in the caller.
A lot of these functions should become port operation where you either
pass in a port structure directly or possibly a port index as I've
mentioned before.
[ I realise that the confusion around hs_phy_irq may be partly to blame
for this but since that one is also a per-port interrupt, that's no
longer an issue. ]
static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
@@ -454,10 +461,8 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
* The role is stable during suspend as role switching is done from a
* freezable workqueue.
*/
- if (dwc3_qcom_is_host(qcom) && wakeup) {
- qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
So just let this function update the usb2 speed for all ports unless
there are reasons not to.
[1]: https://lore.kernel.org/all/fb5e5e1d-520c-4cbc-adde-f30e853421a1@xxxxxxxxxxx/
+ if (dwc3_qcom_is_host(qcom) && wakeup)
dwc3_qcom_enable_interrupts(qcom);
And then iterate over the ports and enable the interrupts here as you
did above for the pwr_evnt_irqs.
- }
qcom->is_suspended = true;