Re: [PATCH v11 06/13] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV]

On 9/1/2023 6:43 AM, Wesley Cheng wrote:
> Hi Krishna,
>
> >           if (dwc->usb2_phy)
> >               otg_set_vbus(dwc->usb2_phy->otg, false);
> > -        phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
> > -        phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
> > +        phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
> > +        phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);
>
> Throughout this patch, you are looping across all PHYs irrespective of 
> if we are in device mode or not.  This is the only exception where you 
> are setting only PHY index 0 (for both SS and HS PHYs).  Do you think we 
> should also only modify PHY index#0 for other PHY related sequences?
Hi Wesley,

 Multiport controllers are host only capable currently. So if the 
GHWPARAMS indicate we are DRD/peripheral capable, we set 
num_usb2/3_ports to "1" unconditionally. So there would not be any 
looping necessary here.

> >           if (ret)
> > @@ -1804,9 +1887,12 @@ static int dwc3_read_port_info(struct dwc3 *dwc)
> >       dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
> >               dwc->num_usb2_ports, dwc->num_usb3_ports);
> > -
> >       iounmap(base);
> > +    if ((dwc->num_usb2_ports > DWC3_MAX_PORTS) ||
> > +        (dwc->num_usb3_ports > DWC3_MAX_PORTS))
> > +        return -ENOMEM;
> > +
>
> Shouldn't this be more applicable to be included in patch#4 in this series?
The read_port_info function was only initially intended to read only 
port count and later the macro was added. So the check was put here.

Regards,
Krishna,