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@xxxxxxxxxxx>
> >> ---
> >> -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
