Messages in this thread | | | Date | Mon, 3 Jul 2023 00:18:14 +0530 | Subject | Re: [PATCH v9 03/10] usb: dwc3: core: Access XHCI address space temporarily to read port info | From | Krishna Kurapati PSSNV <> |
| |
On 6/27/2023 5:15 PM, Johan Hovold wrote: > On Wed, Jun 21, 2023 at 10:06:21AM +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 | 62 +++++++++++++++++++++++++++++++++++++++++ >> drivers/usb/dwc3/core.h | 9 ++++++ >> 2 files changed, 71 insertions(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index f6689b731718..32ec05fc242b 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -39,6 +39,7 @@ >> #include "io.h" >> >> #include "debug.h" >> +#include "../host/xhci-ext-caps.h" >> >> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ >> >> @@ -1767,6 +1768,52 @@ static int dwc3_get_clocks(struct dwc3 *dwc) >> return 0; >> } >> >> +static int dwc3_read_port_info(struct dwc3 *dwc) >> +{ >> + void __iomem *base; >> + u8 major_revision; >> + u32 offset = 0; >> + int ret = 0; > > ret is never modified, so drop and return 0 unconditionally below. > > You can add it back later in the series when you start using it. > >> + u32 val; >> + >> + /* >> + * Remap xHCI address space to access XHCI ext cap regs, >> + * since it is needed to get port info. >> + */ >> + base = ioremap(dwc->xhci_resources[0].start, >> + resource_size(&dwc->xhci_resources[0])); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + do { >> + offset = xhci_find_next_ext_cap(base, offset, >> + XHCI_EXT_CAPS_PROTOCOL); >> + > > You can drop this newline. > >> + if (!offset) >> + break; >> + >> + val = readl(base + offset); >> + major_revision = XHCI_EXT_PORT_MAJOR(val); >> + >> + val = readl(base + offset + 0x08); >> + if (major_revision == 0x03) { >> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val); >> + } else if (major_revision <= 0x02) { >> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val); >> + } else { >> + dev_err(dwc->dev, >> + "Unrecognized port major revision %d\n", >> + major_revision); >> + } >> + } while (1); >> + >> + dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", >> + dwc->num_usb2_ports, dwc->num_usb3_ports); >> + >> + iounmap(base); > > Nit: I'd add a newline here. > >> + return ret; >> +} >> + >> static int dwc3_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -1774,6 +1821,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) >> @@ -1854,6 +1902,20 @@ static int dwc3_probe(struct platform_device *pdev) >> goto err_disable_clks; >> } >> >> + /* >> + * Currently only 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); >> + 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 8b1295e4dcdd..42fb17aa66fa 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -33,6 +33,10 @@ >> >> #include <linux/power_supply.h> >> >> +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) >> +#define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) >> +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > > Again, don't copy defines from xhci. > > Looks like these should be moved to the xhci-ext-caps.h header along > with struct xhci_protocol_caps. > Can't we just give them an exception ? Modifying xhci stuff doesn't sound good. Can I just rename them and keep them here ?
Regards, Krishna,
>> + >> #define DWC3_MSG_MAX 500 >> >> /* Global constants */ >> @@ -1029,6 +1033,8 @@ struct dwc3_scratchpad_array { >> * @usb3_phy: pointer to USB3 PHY >> * @usb2_generic_phy: pointer to USB2 PHY >> * @usb3_generic_phy: pointer to USB3 PHY >> + * @num_usb2_ports: number of USB2 ports. >> + * @num_usb3_ports: number of USB3 ports. > > Again, please drop the full stops ('.'). > > Johan
| |