| Message ID | 20231007154806.605-7-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On Sat, Oct 07, 2023 at 09:18:02PM +0530, Krishna Kurapati wrote: > 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@quicinc.com> > --- > 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() instead 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. > + 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; Johan
On 10/23/2023 9:17 PM, Johan Hovold wrote: > On Sat, Oct 07, 2023 at 09:18:02PM +0530, Krishna Kurapati wrote: >> 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@quicinc.com> >> --- >> 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. > 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. > But perhaps this should be done using usb_hub_for_each_child() instead > 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. > Either ways is fine. We would have qcom->num_ports to determine how many speeds we can read. >> */ >> #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. With your suggestion, yes, this can be refactored to be readable. > > [ 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. ] I don't want to add support for this right away [1]. I would like to keep hs_phy_irq outside the loop for now. > >> 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. Either way is fine by me as mentioned above. Will udapte code accordingly. > >> + 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; [1]: https://lore.kernel.org/all/fb5e5e1d-520c-4cbc-adde-f30e853421a1@quicinc.com/ Regards, Krishna,
On Mon, Oct 23, 2023 at 10:57:04PM +0530, Krishna Kurapati PSSNV wrote: > On 10/23/2023 9:17 PM, Johan Hovold wrote: > > On Sat, Oct 07, 2023 at 09:18:02PM +0530, Krishna Kurapati wrote: > >> 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@quicinc.com> > >> --- > >> -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. > > > 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. It does not make sense to keep only half the comment after your update as it is a suggestion for how one could go about and generalise this for multiport, which is what you are now doing. > > But perhaps this should be done using usb_hub_for_each_child() instead > > 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. > > > Either ways is fine. We would have qcom->num_ports to determine how many > speeds we can read. That's not the point. I'm referring to which alternative is less computationally expensive and allows for a clean implementation. Please do try to figure it out yourself. > >> */ > >> #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. > > With your suggestion, yes, this can be refactored to be readable. > > > > > [ 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. ] > > I don't want to add support for this right away [1]. I would like to > keep hs_phy_irq outside the loop for now. No. Stop trying to take shortcuts. Again, this is upstream, not Qualcomm's vendor kernel. Johan
On 10/24/2023 12:40 PM, Johan Hovold wrote: >>> >>> This comment no longer makes sense with your current implementation. >>> >> 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. > > It does not make sense to keep only half the comment after your update > as it is a suggestion for how one could go about and generalise this for > multiport, which is what you are now doing. > Thanks for explanation. Will update the comments. >>> But perhaps this should be done using usb_hub_for_each_child() instead >>> 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. >>> >> Either ways is fine. We would have qcom->num_ports to determine how many >> speeds we can read. > > That's not the point. I'm referring to which alternative is less > computationally expensive and allows for a clean implementation. > > Please do try to figure it out yourself. > I don't think its much of a difference: while (loop over num_ports) { read_usb2_speed() } read_usb2_speed() { while (loop over num_ports) { hub api to read speed. } } The second one would avoid calling read_usb2_speed multiple times. Will take that path. >>> >>> [ 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. ] >> >> I don't want to add support for this right away [1]. I would like to >> keep hs_phy_irq outside the loop for now. > > No. Stop trying to take shortcuts. Again, this is upstream, not > Qualcomm's vendor kernel. > I don't think it is a shortcut. The reason I said I would keep it out of loop is I know why we need DP/DM/SS IRQ's during wakeup. The wakeup signals come in as rising/falling edges in high speed on DP/DM lines and LFPS terminations come on SS lines. So we need these 3 interrupts for sure in wakeup context. hs_phy_irq is not mandatory for wakeup. Any particular reason why it is needed to add driver support for hs_phy_irq's of multiport now ? May be I am missing something. If there is any reason why we need to add it now, I would try to learn and see if it has any side effects (like generating spurious wakeup's) and if nothing, I would add it back to port structure. Regards, Krishna,
On Tue, Oct 24, 2023 at 02:11:31PM +0530, Krishna Kurapati PSSNV wrote: > On 10/24/2023 12:40 PM, Johan Hovold wrote: > >>> But perhaps this should be done using usb_hub_for_each_child() instead > >>> 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. > >>> > >> Either ways is fine. We would have qcom->num_ports to determine how many > >> speeds we can read. > > > > That's not the point. I'm referring to which alternative is less > > computationally expensive and allows for a clean implementation. > > > > Please do try to figure it out yourself. > > > I don't think its much of a difference: > > while (loop over num_ports) { > read_usb2_speed() > } > > read_usb2_speed() { > while (loop over num_ports) { > hub api to read speed. > } > } > > The second one would avoid calling read_usb2_speed multiple times. Will > take that path. You need to look at the implementation of usb_hub_for_each_child() and usb_hub_find_child() to determine that, which you now forced me to do; and yes, you're right, this shouldn't matter from an efficiency standpoint. > >>> [ 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. ] > >> > >> I don't want to add support for this right away [1]. I would like to > >> keep hs_phy_irq outside the loop for now. > > > > No. Stop trying to take shortcuts. Again, this is upstream, not > > Qualcomm's vendor kernel. > > I don't think it is a shortcut. > > The reason I said I would keep it out of loop is I know why we need > DP/DM/SS IRQ's during wakeup. The wakeup signals come in as > rising/falling edges in high speed on DP/DM lines and LFPS terminations > come on SS lines. It is a shortcut as this interrupt is per-port and some SoC's already use it. So you're making a mess of the implementation for no good reason. > So we need these 3 interrupts for sure in wakeup context. > hs_phy_irq is not mandatory for wakeup. Any particular reason why it is > needed to add driver support for hs_phy_irq's of multiport now ? May be > I am missing something. If there is any reason why we need to add it > now, I would try to learn and see if it has any side effects (like > generating spurious wakeup's) and if nothing, I would add it back to > port structure. As I've mentioned a few times now, the hs_phy_irq is already used by a few SoC's so you can't just pretend it doesn't exist and mess up the implementation for no good reason. Just find out how it is used and why only some Qualcomm SoC's use it currently. It appears to be used in parallel with the DP/DM interrupts, and it has been there from the start: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver") Sure, the wakeup implementation was incomplete and broken for a long time, but I'm not going to let you continue this practise of pushing incomplete hacks upstream which someone else will eventually be forced to clean up. You have the documentation, just use it. Johan
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]; 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) { 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(). */ #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]); + } } static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) { + int i; + dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq, 0); /* @@ -413,22 +418,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. */ - - if (qcom->usb2_speed == USB_SPEED_LOW) { - dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0], + for (i = 0; i < qcom->num_ports; i++) { + qcom->usb2_speed[i] = dwc3_qcom_read_usb2_speed(qcom, i); + if (qcom->usb2_speed[i] == USB_SPEED_LOW) { + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i], IRQ_TYPE_EDGE_FALLING); - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || - (qcom->usb2_speed == USB_SPEED_FULL)) { - dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0], + } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) || + (qcom->usb2_speed[i] == USB_SPEED_FULL)) { + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i], IRQ_TYPE_EDGE_FALLING); - } else { - dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0], + } else { + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i], IRQ_TYPE_EDGE_RISING); - dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0], + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i], IRQ_TYPE_EDGE_RISING); - } + } - dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0], 0); + dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i], 0); + } } 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); + if (dwc3_qcom_is_host(qcom) && wakeup) dwc3_qcom_enable_interrupts(qcom); - } qcom->is_suspended = true;
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@quicinc.com> --- drivers/usb/dwc3/dwc3-qcom.c | 65 +++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 30 deletions(-)