| Message ID | 20240216005756.762712-9-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
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@quicinc.com> > Reviewed-by: Bjorn Andersson <andersson@kernel.org> > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > 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
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; }; 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) { 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); + } } static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) { + int i; + dwc3_qcom_enable_wakeup_irq(qcom->qusb2_phy_irq, 0); /* @@ -414,22 +416,24 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) * disconnect and remote wakeup. When no device is connected, configure both * DP and DM lines as rising edge to detect HS/HS/LS device connect scenario. */ + for (i = 0; i < qcom->num_ports; i++) { + qcom->port_info[i].usb2_speed = dwc3_qcom_read_usb2_speed(qcom, i); + if (qcom->port_info[i].usb2_speed == USB_SPEED_LOW) { + dwc3_qcom_enable_wakeup_irq(qcom->port_info[i].dm_hs_phy_irq, + IRQ_TYPE_EDGE_FALLING); + } else if ((qcom->port_info[i].usb2_speed == USB_SPEED_HIGH) || + (qcom->port_info[i].usb2_speed == USB_SPEED_FULL)) { + dwc3_qcom_enable_wakeup_irq(qcom->port_info[i].dp_hs_phy_irq, + IRQ_TYPE_EDGE_FALLING); + } else { + dwc3_qcom_enable_wakeup_irq(qcom->port_info[i].dp_hs_phy_irq, + IRQ_TYPE_EDGE_RISING); + dwc3_qcom_enable_wakeup_irq(qcom->port_info[i].dm_hs_phy_irq, + IRQ_TYPE_EDGE_RISING); + } - if (qcom->usb2_speed == USB_SPEED_LOW) { - dwc3_qcom_enable_wakeup_irq(qcom->port_info[0].dm_hs_phy_irq, - IRQ_TYPE_EDGE_FALLING); - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || - (qcom->usb2_speed == USB_SPEED_FULL)) { - dwc3_qcom_enable_wakeup_irq(qcom->port_info[0].dp_hs_phy_irq, - IRQ_TYPE_EDGE_FALLING); - } else { - dwc3_qcom_enable_wakeup_irq(qcom->port_info[0].dp_hs_phy_irq, - IRQ_TYPE_EDGE_RISING); - dwc3_qcom_enable_wakeup_irq(qcom->port_info[0].dm_hs_phy_irq, - IRQ_TYPE_EDGE_RISING); + dwc3_qcom_enable_wakeup_irq(qcom->port_info[i].ss_phy_irq, 0); } - - dwc3_qcom_enable_wakeup_irq(qcom->port_info[0].ss_phy_irq, 0); } 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); + if (dwc3_qcom_is_host(qcom) && wakeup) dwc3_qcom_enable_interrupts(qcom); - } qcom->is_suspended = true;