Messages in this thread Patch in this message | | | Date | Sat, 11 Mar 2023 08:34:04 +0530 | Subject | Re: [PATCH 2/8] usb: dwc3: core: Access XHCI address space temporarily to read port info | From | Krishna Kurapati PSSNV <> |
| |
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
Regards, Krishna,
>>> + * @num_ss_ports: Indicates the number of USB ports present on HW >>> that are >>> + * SS Capable >>> * @usb2_generic_phy: pointer to USB2 PHY >>> * @usb3_generic_phy: pointer to USB3 PHY >>> * @phys_ready: flag to indicate that PHYs are ready >>> @@ -1158,6 +1165,8 @@ struct dwc3 { >>> struct usb_phy *usb2_phy; >>> struct usb_phy *usb3_phy; >>> + u32 num_ports; >>> + u32 num_ss_ports; >>> struct phy *usb2_generic_phy; >>> struct phy *usb3_generic_phy; >>> -- >>> 2.39.0 >>> >> >> Thanks, >> Thinh
| |