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