Messages in this thread | | | Date | Thu, 4 May 2023 09:35:28 +0530 | Subject | Re: [PATCH v7 3/9] usb: dwc3: core: Access XHCI address space temporarily to read port info | From | Krishna Kurapati PSSNV <> |
| |
On 5/4/2023 3:19 AM, Thinh Nguyen wrote: > On Wed, May 03, 2023, Krishna Kurapati PSSNV wrote: >> >> >> On 5/3/2023 3:11 AM, Thinh Nguyen wrote: >>> Hi, >>> >>> On Mon, May 01, 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 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 | 68 +++++++++++++++++++++++++++++++++++++++++ >>>> drivers/usb/dwc3/core.h | 58 +++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 126 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>> index 0beaab932e7d..b8ac7bcee391 100644 >>>> --- a/drivers/usb/dwc3/core.c >>>> +++ b/drivers/usb/dwc3/core.c >>>> @@ -1767,6 +1767,59 @@ static int dwc3_get_clocks(struct dwc3 *dwc) >>>> return 0; >>>> } >>>> +static int dwc3_read_port_info(struct dwc3 *dwc) >>>> +{ >>>> + void __iomem *regs; >>>> + 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. >>>> + */ >>>> + 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) { >>>> + temp = readl(regs + offset); >>>> + major_revision = XHCI_EXT_PORT_MAJOR(temp); >>>> + >>>> + temp = readl(regs + offset + 0x08); >>>> + 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, "port revision seems wrong\n"); >>> >>> Can we print this instead: >>> dev_err(dwc->dev, "Unrecognized port major revision %d\n", major_revision); >>> >>>> + 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, "inconsistency in port info\n"); >>> >>> Can we print this instead: >>> dev_err(dwc->dev, "Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp)); >>> >>>> + ret = -EINVAL; >>>> + goto unmap_reg; >>>> + } >>>> + >>>> + dev_dbg(dwc->dev, >>>> + "hs-ports: %d ss-ports: %d\n", dwc->num_usb2_ports, dwc->num_usb3_ports); >>>> + >>>> +unmap_reg: >>>> + iounmap(regs); >>>> + return ret; >>>> +} >>>> + >>>> static int dwc3_probe(struct platform_device *pdev) >>>> { >>>> struct device *dev = &pdev->dev; >>>> @@ -1774,6 +1827,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 +1897,20 @@ static int dwc3_probe(struct platform_device *pdev) >>>> goto err_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); >>>> + 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..21312703e053 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) >>>> + >>>> /* 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. >>>> * @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; >>>> + u32 num_usb2_ports; >>>> + u32 num_usb3_ports; >>> >>> can we use u8? >>> >>>> + >>>> struct phy *usb2_generic_phy; >>>> struct phy *usb3_generic_phy; >>>> @@ -1650,4 +1666,46 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc) >>>> { } >>>> #endif >>>> +/** >>>> + * 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 >>> >>> I know that this is a duplicate from the xhci driver, but can we fix the >>> kerneldoc style as in other places if we're going to keep it? >>> >> >> Hi Thinh, >> >> Isn't this same as other functions ? >> >> /** >> * <function name> - description >> * @params >> * >> */ >> >> I missed the function name in comments last time, but added it in this >> version. >> >> Checkpatch too didn't give any errors/warnings other than alignment. Can you >> help point out any other mistake in this function doc/comments so that I can >> fix it in next version. >> > > It's missing ":" after @params. > > You can use the kernel-doc script to check for documentation's > errors/warnings. > > Here's the output if you run the following command: > # <KERNEL_SRC>/scripts/kernel-doc -none -Werror -function dwc3_xhci_find_next_ext_cap drivers/usb/dwc3/core.h > > drivers/usb/dwc3/core.h:1684: warning: Function parameter or member 'base' not described in 'dwc3_xhci_find_next_ext_cap' > drivers/usb/dwc3/core.h:1684: warning: Function parameter or member 'start' not described in 'dwc3_xhci_find_next_ext_cap' > drivers/usb/dwc3/core.h:1684: warning: Function parameter or member 'id' not described in 'dwc3_xhci_find_next_ext_cap' > 3 warnings as Errors > > > This is a minor style issue. While I don't think it's strictly needed to > follow the kerneldoc style for a non-api function. But if we do follow > it, we should do it correctly. > > Thanks, > Thinh
Thanks Thinh for pointing it out. Will fix this in the next version.
Regards, Krishna,
| |