Messages in this thread | | | Date | Wed, 17 May 2023 08:40:45 +0530 | Subject | Re: [PATCH v8 3/9] usb: dwc3: core: Access XHCI address space temporarily to read port info | From | Krishna Kurapati PSSNV <> |
| |
On 5/16/2023 8:32 PM, Krishna Kurapati PSSNV wrote: > > > On 5/16/2023 5:41 PM, Johan Hovold wrote: >> On Sun, May 14, 2023 at 11:19:11AM +0530, 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 usb2 ports and >>> usb3 ports present on multiport controller. Each USB Port is at least HS >>> capable. >>> >>> The port info for usb2 and usb3 phy are identified as num_usb2_ports >>> and num_usb3_ports. The intention is as follows: >>> >>> Wherever we need to perform phy operations like: >>> >>> LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() >>> { >>> phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); >>> phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); >>> } >>> >>> If number of usb2 ports is 3, loop can go from index 0-2 for >>> usb2_generic_phy. 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 (2 and 3) >>> are SS capable. So instead, num_usb2_ports is used to loop around all >>> phy's (both hs and ss) for performing phy operations. If any >>> usb3_generic_phy turns out to be NULL, phy operation just bails out. >>> >>> num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up >>> phy's as we need to know how many SS capable ports are there for this. >>> >>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>> --- >>> drivers/usb/dwc3/core.c | 113 ++++++++++++++++++++++++++++++++++++++++ >>> drivers/usb/dwc3/core.h | 17 +++++- >>> 2 files changed, 129 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>> index 0beaab932e7d..e983aef1fb93 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -1767,6 +1767,104 @@ static int dwc3_get_clocks(struct dwc3 *dwc) >>> return 0; >>> } >>> +/** >>> + * dwc3_xhci_find_next_ext_cap - Find the offset of the extended >>> capabilities >>> + * with capability ID id. >>> + * >>> + * @base: PCI MMIO registers base address. >>> + * @start: address at which to start looking, (0 or HCC_PARAMS to >>> start at >>> + * beginning of list) >>> + * @id: Extended capability ID to search for, or 0 for the next >>> + * capability >>> + * >>> + * Returns the offset of the next matching extended capability >>> structure. >>> + * Some capabilities can occur several times, e.g., the >>> XHCI_EXT_CAPS_PROTOCOL, >>> + * and this provides a way to find them all. >>> + */ >>> +static int dwc3_xhci_find_next_ext_cap(void __iomem *base, u32 >>> start, int id) >>> +{ >>> + u32 val; >>> + u32 next; >>> + u32 offset; >>> + >>> + offset = start; >>> + if (!start || start == XHCI_HCC_PARAMS_OFFSET) { >>> + val = readl(base + XHCI_HCC_PARAMS_OFFSET); >>> + if (val == ~0) >>> + return 0; >>> + offset = XHCI_HCC_EXT_CAPS(val) << 2; >>> + if (!offset) >>> + return 0; >>> + } >>> + do { >>> + val = readl(base + offset); >>> + if (val == ~0) >>> + return 0; >>> + if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == >>> id)) >>> + return offset; >>> + >>> + next = XHCI_EXT_CAPS_NEXT(val); >>> + offset += next << 2; >>> + } while (next); >>> + >>> + return 0; >>> +} >> >> You should not make another copy of xhci_find_next_ext_cap(), but rather >> use it directly. >> >> We already have drivers outside of usb/host using this function so it >> should be fine to do the same for now: >> >> #include "../host/xhci-ext-caps.h" >> > Hi Johan, > > This was the approach which we followed when we first introduced the > patch [1]. But Thinh suggested to duplicate code so that we can avoid > any dependency on xhci (which seems to be right). So since its just one > function, I duplicated it here. > Hi Thinh,
Would like to know your opinion here on how to proceed further.
Regards, Krishna, > >>> +static int dwc3_read_port_info(struct dwc3 *dwc) >>> +{ >>> + void __iomem *regs; >> >> Call this one 'base' instead. >> >>> + u32 offset; >>> + u32 temp; >> >> I see that the xhci driver use 'temp' for this, but I'd prefer 'val'. >> >>> + u8 major_revision; >>> + int ret = 0; >>> + >>> + /* >>> + * Remap xHCI address space to access XHCI ext cap regs, >>> + * since it is needed to get port info. >>> + */ >>> + regs = ioremap(dwc->xhci_resources[0].start, >>> + resource_size(&dwc->xhci_resources[0])); >>> + if (IS_ERR(regs)) >>> + return PTR_ERR(regs); >>> + >>> + offset = dwc3_xhci_find_next_ext_cap(regs, 0, >>> + XHCI_EXT_CAPS_PROTOCOL); >>> + while (offset) { >> >> This would be better implemented as a do-while loop (cf. >> xdbc_reset_debug_port()). >> >>> + temp = readl(regs + offset); >>> + major_revision = XHCI_EXT_PORT_MAJOR(temp); >>> + >>> + temp = readl(regs + offset + 0x08); >> >> We should try to avoid magic constants, but I see that we already have >> cases accessing these fields like this. >> >>> + if (major_revision == 0x03) { >>> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(temp); >>> + } else if (major_revision <= 0x02) { >>> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp); >>> + } else { >>> + dev_err(dwc->dev, >>> + "Unrecognized port major revision %d\n", >>> major_revision); >> >> Please add a line break after the string. >> >> Perhaps this should be handles as in xhci core by simply warning and >> continuing instead. >> > I broke the loop and went to unmap as we are not sure what values would > be read. Any use of continuing ? > >>> + ret = -EINVAL; >>> + goto unmap_reg; >>> + } >>> + >>> + offset = dwc3_xhci_find_next_ext_cap(regs, offset, >>> + XHCI_EXT_CAPS_PROTOCOL); >>> + } >>> + >>> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); >>> + if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + >>> dwc->num_usb2_ports)) { >>> + dev_err(dwc->dev, >>> + "Mismatched reported MAXPORTS (%d)\n", >>> HCS_MAX_PORTS(temp)); >>> + ret = -EINVAL; >>> + goto unmap_reg; >>> + } >> >> Not sure this is needed either. >> >> Could this risk regressing platforms which does not have currently have >> all PHYs described in DT? >> > No, it doesn't. AFAIK, this only tells how many ports are present as per > the core consultant configuration of the device. I tried to explain what > would happen incase phy's are not present in DT in [2] & [3]. > >> You do however need to make sure that both num_usb<n>_ports is no larger >> than MAX_PORTS_SUPPORTED to avoid memory corruption when you're adding >> fixed sized arrays for the PHYs later in the series. >> >>> + >>> + dev_dbg(dwc->dev, >>> + "hs-ports: %d ss-ports: %d\n", dwc->num_usb2_ports, >>> dwc->num_usb3_ports); >> >> Use %u for unsigned values. >> >> And please try to stay within 80 columns. >> > Thanks for catching this potential bug. Will add that if check as well > in v9. > >>> + >>> +unmap_reg: >>> + iounmap(regs); >>> + return ret; >>> +} >>> + >>> static int dwc3_probe(struct platform_device *pdev) >>> { >>> struct device *dev = &pdev->dev; >>> @@ -1774,6 +1872,7 @@ static int dwc3_probe(struct platform_device >>> *pdev) >>> void __iomem *regs; >>> struct dwc3 *dwc; >>> int ret; >>> + unsigned int hw_mode; >>> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); >>> if (!dwc) >>> @@ -1843,6 +1942,20 @@ static int dwc3_probe(struct platform_device >>> *pdev) >>> goto err_disable_clks; >>> } >>> + /* >>> + * Currently DWC3 controllers that are host-only capable >>> + * support Multiport >> >> Are you missing an "only" after "Currently" above? >> > Please add a full stop. >> >>> + */ >>> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >>> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >>> + ret = dwc3_read_port_info(dwc); >>> + if (ret) >>> + goto err_disable_clks; >>> + } else { >>> + dwc->num_usb2_ports = 1; >>> + dwc->num_usb3_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 d56457c02996..d3401963bc27 100644 >>> --- a/drivers/usb/dwc3/core.h >>> +++ b/drivers/usb/dwc3/core.h >>> @@ -35,6 +35,17 @@ >>> #define DWC3_MSG_MAX 500 >>> +/* Define XHCI Extcap register offsets for getting multiport info */ >>> +#define XHCI_HCC_PARAMS_OFFSET 0x10 >>> +#define DWC3_XHCI_HCSPARAMS1 0x04 >>> +#define XHCI_EXT_CAPS_PROTOCOL 2 >>> +#define XHCI_HCC_EXT_CAPS(x) (((x) >> 16) & 0xffff) >>> +#define XHCI_EXT_CAPS_ID(x) (((x) >> 0) & 0xff) >>> +#define XHCI_EXT_CAPS_NEXT(x) (((x) >> 8) & 0xff) >>> +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) >>> +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) >>> +#define HCS_MAX_PORTS(x) (((x) >> 24) & 0x7f) >>> + >> >> You should use the xhci defines instead of these copies too. >> >>> /* Global constants */ >>> #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ >>> #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ >>> @@ -1025,6 +1036,8 @@ struct dwc3_scratchpad_array { >>> * @usb_psy: pointer to power supply interface. >>> * @usb2_phy: pointer to USB2 PHY >>> * @usb3_phy: pointer to USB3 PHY >>> + * @num_usb2_ports: number of usb2 ports. >>> + * @num_usb3_ports: number of usb3 ports. >> >> Use upper case "USBn" and drop the full stops for consistency. >> >> Please move these after the PHY structures. >> >>> * @usb2_generic_phy: pointer to USB2 PHY >>> * @usb3_generic_phy: pointer to USB3 PHY >>> * @phys_ready: flag to indicate that PHYs are ready >>> @@ -1162,6 +1175,9 @@ struct dwc3 { >>> struct usb_phy *usb2_phy; >>> struct usb_phy *usb3_phy; >>> + u8 num_usb2_ports; >>> + u8 num_usb3_ports; >>> + >>> struct phy *usb2_generic_phy; >>> struct phy *usb3_generic_phy; >>> @@ -1649,5 +1665,4 @@ static inline int dwc3_ulpi_init(struct dwc3 *dwc) >>> static inline void dwc3_ulpi_exit(struct dwc3 *dwc) >>> { } >>> #endif >>> - >> >> This is an unrelated change that should be dropped. >> >>> #endif /* __DRIVERS_USB_DWC3_CORE_H */ >> >> Johan > > [1]: > https://lore.kernel.org/all/20230310163420.7582-3-quic_kriskura@quicinc.com/ > > [2]: > https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ > > [3]: > https://lore.kernel.org/all/966c1001-6d64-9163-0c07-96595156fc8c@quicinc.com/ > > Thanks for the review and comments 🙂. Will make sure to fix them in v9. > > Regards, > Krishna,
| |