On Fri, Feb 16, 2024 at 06:27:55AM +0530, Krishna Kurapati wrote: > DWC3 Qcom wrapper currently supports only wakeup configuration > for single port controllers. Read speed of each port connected > to the controller and enable wakeup for each of them accordingly. > > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx> > Reviewed-by: Bjorn Andersson <andersson@xxxxxxxxxx> > Acked-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> > --- > drivers/usb/dwc3/dwc3-qcom.c | 72 ++++++++++++++++++------------------ > 1 file changed, 37 insertions(+), 35 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index a20d63a791bd..572dc3fdae12 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -78,6 +78,7 @@ struct dwc3_qcom_port { > int dp_hs_phy_irq; > int dm_hs_phy_irq; > int ss_phy_irq; > + enum usb_device_speed usb2_speed; You need to remove the corresponding, and now unused, field from struct dwc3_qcom as well. > }; > > struct dwc3_qcom { > @@ -336,7 +337,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) As I mentioned, there's no need for a line break after the first parameter as this is a function definition (e.g. Linus as expressed a preference for this as it makes functions easier to grep for). > { > struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > struct usb_device *udev; > @@ -347,14 +349,8 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > */ > hcd = platform_get_drvdata(dwc->xhci); > > - /* > - * 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. > - */ > #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 > @@ -387,23 +383,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->qusb2_phy_irq); > > - if (qcom->usb2_speed == USB_SPEED_LOW) { > - dwc3_qcom_disable_wakeup_irq(qcom->port_info[0].dm_hs_phy_irq); > - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || > - (qcom->usb2_speed == USB_SPEED_FULL)) { > - dwc3_qcom_disable_wakeup_irq(qcom->port_info[0].dp_hs_phy_irq); > - } else { > - dwc3_qcom_disable_wakeup_irq(qcom->port_info[0].dp_hs_phy_irq); > - dwc3_qcom_disable_wakeup_irq(qcom->port_info[0].dm_hs_phy_irq); > - } > + for (i = 0; i < qcom->num_ports; i++) { > + if (qcom->port_info[i].usb2_speed == USB_SPEED_LOW) { > + dwc3_qcom_disable_wakeup_irq(qcom->port_info[i].dm_hs_phy_irq); > + } else if ((qcom->port_info[i].usb2_speed == USB_SPEED_HIGH) || > + (qcom->port_info[i].usb2_speed == USB_SPEED_FULL)) { > + dwc3_qcom_disable_wakeup_irq(qcom->port_info[i].dp_hs_phy_irq); > + } else { > + dwc3_qcom_disable_wakeup_irq(qcom->port_info[i].dp_hs_phy_irq); > + dwc3_qcom_disable_wakeup_irq(qcom->port_info[i].dm_hs_phy_irq); > + } > > - dwc3_qcom_disable_wakeup_irq(qcom->port_info[0].ss_phy_irq); > + dwc3_qcom_disable_wakeup_irq(qcom->port_info[i].ss_phy_irq); > + } As I already commented on v13, this should be a per-port helper rather than special casing qusb2_phy_irq and a for loop for the other interrupts: 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. > } > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > @@ -455,10 +459,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); And again, as I said for v13: So just let this function update the usb2 speed for all ports unless there are reasons not to. rather than hide it away in an odd for loop in dwc3_qcom_enable_interrupts(). > + if (dwc3_qcom_is_host(qcom) && wakeup) > dwc3_qcom_enable_interrupts(qcom); > - } > > qcom->is_suspended = true; Johan