Re: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport

[Johan Hovold]

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