Re: [PATCH 2/8] usb: dwc3: core: Access XHCI address space temporarily to read port info

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Sat, Mar 11, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 3/11/2023 8:24 AM, Krishna Kurapati PSSNV wrote:
> > 
> > 
> > On 3/11/2023 5:25 AM, Thinh Nguyen wrote:
> > > On Fri, Mar 10, 2023, Krishna Kurapati wrote:
> > > > Currently host-only capable DWC3 controllers support Multiport.
> > > > Temporarily
> > > > map XHCI address space for host-only controllers and parse XHCI Extended
> > > > Capabilities registers to read number of physical usb ports
> > > > connected to the
> > > > multiport controller (presuming each port is at least HS
> > > > capable) and extract
> > > > info on how many of these ports are Super Speed capable.
> > > > 
> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > ---
> > > >   drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++
> > > >   drivers/usb/dwc3/core.h |  9 +++++
> > > >   2 files changed, 84 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 476b63618511..076c0f8a4441 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -37,6 +37,7 @@
> > > >   #include "core.h"
> > > >   #include "gadget.h"
> > > >   #include "io.h"
> > > > +#include "../host/xhci.h"
> > > 
> > > I think better to duplicate some of the logic in dwc3 driver and avoid
> > > any direct dependency with the xhci driver.
> > > 
> > > >   #include "debug.h"
> > > > @@ -1750,6 +1751,65 @@ static struct extcon_dev
> > > > *dwc3_get_extcon(struct dwc3 *dwc)
> > > >       return edev;
> > > >   }
> > > > +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
> > > > +{
> > > > +    void __iomem        *regs;
> > > > +    struct resource         dwc_res;
> > > > +    u32            offset;
> > > > +    u32            temp;
> > > > +    u8            major_revision;
> > > > +    int            ret = 0;
> > > > +
> > > > +    /*
> > > > +     * Remap xHCI address space to access XHCI ext cap regs,
> > > > +     * since it is needed to get port info.
> > > > +     */
> > > > +    dwc_res = *res;
> > > > +    dwc_res.start += 0;
> > > > +    dwc_res.end = dwc->xhci_resources[0].start +
> > > > +                DWC3_XHCI_REGS_END;
> > > 
> > > Isn't dwc->xhci_resources[0] already setup at this point? Can we use
> > > dwc->xhci_resources[0] directly without copy the setting in dwc_res?
> > > 
> > > > +
> > > > +    regs = ioremap(dwc_res.start, resource_size(&dwc_res));
> > > > +    if (IS_ERR(regs))
> > > > +        return PTR_ERR(regs);
> > > > +
> > > > +    offset = xhci_find_next_ext_cap(regs, 0,
> > > > +                    XHCI_EXT_CAPS_PROTOCOL);
> > > > +    while (offset) {
> > > > +        temp = readl(regs + offset);
> > > > +        major_revision = XHCI_EXT_PORT_MAJOR(temp);
> > > > +
> > > > +        temp = readl(regs + offset + 0x08);
> > > > +        if (major_revision == 0x03) {
> > > > +            dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
> > > > +        } else if (major_revision <= 0x02) {
> > > > +            dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
> > > > +        } else {
> > > > +            dev_err(dwc->dev, "port revision seems wrong\n");
> > > > +            ret = -EINVAL;
> > > > +            goto unmap_reg;
> > > > +        }
> > > > +
> > > > +        offset = xhci_find_next_ext_cap(regs, offset,
> > > > +                        XHCI_EXT_CAPS_PROTOCOL);
> > > > +    }
> > > > +
> > > > +    temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
> > > > +    if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
> > > > +        dev_err(dwc->dev, "inconsistency in port info\n");
> > > > +        ret = -EINVAL;
> > > > +        goto unmap_reg;
> > > > +    }
> > > > +
> > > > +    dev_info(dwc->dev,
> > > > +        "num-ports: %d ss-capable: %d\n", dwc->num_ports,
> > > > dwc->num_ss_ports);
> > > 
> > > The end user doesn't need to know this info. This should be a debug
> > > message. Perhaps it can be a tracepoint if needed?
> > > 
> > > > +
> > > > +unmap_reg:
> > > > +    iounmap(regs);
> > > > +    return ret;
> > > > +}
> > > > +
> > > >   static int dwc3_probe(struct platform_device *pdev)
> > > >   {
> > > >       struct device        *dev = &pdev->dev;
> > > > @@ -1757,6 +1817,7 @@ static int dwc3_probe(struct
> > > > platform_device *pdev)
> > > >       struct dwc3        *dwc;
> > > >       int            ret;
> > > > +    unsigned int        hw_mode;
> > > >       void __iomem        *regs;
> > > > @@ -1880,6 +1941,20 @@ static int dwc3_probe(struct
> > > > platform_device *pdev)
> > > >               goto disable_clks;
> > > >       }
> > > > +    /*
> > > > +     * Currently DWC3 controllers that are host-only capable
> > > > +     * support Multiport.
> > > > +     */
> > > > +    hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> > > > +    if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
> > > > +        ret = dwc3_read_port_info(dwc, res);
> > > > +        if (ret)
> > > > +            goto disable_clks;
> > > > +    } else {
> > > > +        dwc->num_ports = 1;
> > > > +        dwc->num_ss_ports = 1;
> > > > +    }
> > > > +
> > > >       spin_lock_init(&dwc->lock);
> > > >       mutex_init(&dwc->mutex);
> > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > > > index 582ebd9cf9c2..74386d6a0277 100644
> > > > --- a/drivers/usb/dwc3/core.h
> > > > +++ b/drivers/usb/dwc3/core.h
> > > > @@ -35,6 +35,9 @@
> > > >   #define DWC3_MSG_MAX    500
> > > > +/* XHCI Reg constants */
> > > > +#define DWC3_XHCI_HCSPARAMS1    0x04
> > > > +
> > > >   /* Global constants */
> > > >   #define DWC3_PULL_UP_TIMEOUT    500    /* ms */
> > > >   #define DWC3_BOUNCE_SIZE    1024    /* size of a superspeed bulk */
> > > > @@ -1023,6 +1026,10 @@ struct dwc3_scratchpad_array {
> > > >    * @usb_psy: pointer to power supply interface.
> > > >    * @usb2_phy: pointer to USB2 PHY
> > > >    * @usb3_phy: pointer to USB3 PHY
> > > > + * @num_ports: Indicates the number of physical USB ports present on HW
> > > > + *        presuming each port is at least HS capable
> > > 
> > > This isn't the number of physical USB ports right? That's the number of
> > > usb2 ports the controller is configured with right?. Perhaps we can use
> > > num_usb2_ports and num_usb3_ports?
> > > 
> > Hi Thinh,
> > 
> >    Yes, naming this might have created a little confusion.
> > num_ports is supposed to indicate number of usb2 ports in the controller.
> > 
> > Incase of sa8295 (4 port controller with first two ports having ss
> > capability), num_ports would be 4 and num_ss_ports would be 2. (and not
> > 6 as what num_ports usually sounds).
> > I can rename them accordingly in the next version and update the
> > description as well.
> > 
> > Regards,
> > Krishna,
> > 
> Hi Thinh,
> 
> One reason I didn't mention something like num_hs_ports and sticked to
> num_ports is because in core driver, wherever we need to do phy operations
> like:
> 
> for (i = 0; i < num_ports; i++)
> {
> 	phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
> 	phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
> }
> 
> The intention is as follows:
> If number of usb2 ports is 4, the loop can go from 0-3 and its fine.
> If number of usb3-ports is 2, we don't know for sure, if the first 2 ports
> are SS capable or some other ports like (3 and 4) are SS capable.
> So instead, I looped all phy operations around all usb2_generic_phy's and
> usb3_generic_phy's. If they are NULL, we just bail out inside phy operation.
> 
> While doing so, looping SS Phy operations around num_usb2_ports didn't sound
> good. From code view, it would be like we are looping usb3_phy ops around
> num_usb2_ports value (logically it is still correct as each port is atleast
> HS capable). So to avoid this, I named the variable as num_ports instead of
> num_usb2_ports
> 

Hi Krishna,

I think it's clearer if add this note along with using num_usb2_ports.
We just need to note this once and I think it's sufficient.

Thanks,
Thinh