On Wed, Jan 25, 2023, Krishna Kurapati PSSNV wrote:
>
>
> On 1/21/2023 4:14 AM, Thinh Nguyen wrote:
> >
> > This becomes rather more complicated because the user can skip certain
> > port in the DT. We have access to the host registers. Can we just
> > temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port
> > capability before handing control over to the xHCI driver. We would be
> > able to get the num_ports and num_ss_ports then.
> >
> > Similarly, the xhci driver doesn't care whether the user skips certain
> > port in the DT, it only checks and operates based on the capability
> > registers.
> >
> > If we have the exact num_ports and num_ss_ports, we can be sure the
> > setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid.
> >
>
> Hi Thinh,
>
> Thanks for the suggestion. Is the following diff / implementation good
> enough ? I Wanted to get it clarified from upstream as I am using
> *ioremap/iounmap* directly instead of *devm_* API's
>
> I tested it and it works fine on SA8295P. Will do some further testing on
> other devices as well.
>
>
> +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
> +{
> + void __iomem *regs;
> + struct resource dwc_res;
> + unsigned int hw_mode;
> + u32 offset;
> + u32 temp;
> + u8 major_revision;
> + u8 minor_revision;
> +
> + /*
> + * Request memory region including xHCI regs,
> + * since it is needed to get port info
> + */
> + dwc_res = *res;
> + dwc_res.start += 0;
> +
> + regs = ioremap(dwc_res.start, resource_size(&dwc_res));
> + if (IS_ERR(regs)) {
> + return PTR_ERR(regs);
> + }
We don't need to ioremap the whole region. Just do it for
the xhci_resources[0]
> +
> + /*
> + * If the controller is not host-only, then it must be a
> + * single port controller.
> + */
> + temp = readl(regs + DWC3_GHWPARAMS0);
> + hw_mode = DWC3_GHWPARAMS0_MODE(temp);
> + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
> + dwc->num_ports = 1;
> + dwc->num_ss_ports = 1;
> + return 0;
> + }
This check should be done before we get into this function.
> +
> + 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);;
> + minor_revision = XHCI_EXT_PORT_MINOR(temp);
We probably don't need minor revision.
> +
> + 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, "revision gone wrong\n");
> + return -EINVAL;
> + }
> +
> + offset = xhci_find_next_ext_cap(regs, offset,
> + XHCI_EXT_CAPS_PROTOCOL);
> + }
> +
> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1_OFFSET);
> + if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
> + dev_err(dwc->dev, "inconsistency in port info\n");
> + return -EINVAL;
> + }
> +
> + dev_info(dwc->dev, "num_ports: %d, num_ss_ports: %d\n",
> dwc->num_ports, dwc->num_ss_ports);
> + iounmap(regs);
Make sure to iounmap on all the early return/error cases.
> + return 0;
> +}
> +
> static int dwc3_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -1912,6 +1964,10 @@ static int dwc3_probe(struct platform_device *pdev)
> dwc->xhci_resources[0].flags = res->flags;
> dwc->xhci_resources[0].name = res->name;
>
> + ret = dwc3_read_port_info(dwc, res);
This should be called after some initializations to make sure some
clocks are enabled. Otherwise some devices may not able to access the
registers. Preferably after dwc3_cache_hwparams() but before
dwc3_core_init().
> + if (ret)
> + return ret;
> +
> /*
> * Request memory region but exclude xHCI regs,
> * since it will be requested by the xhci-plat driver.
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2f82eda9d44f..8535425b81d4 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -38,6 +38,9 @@
> /* Numer of ports supported by a multiport controller */
> #define MAX_PORTS_SUPPORTED 4
>
> +/* XHCI Reg constants */
> +#define DWC3_XHCI_HCSPARAMS1_OFFSET 0x04
Change to DWC3_XHCI_HCSPARAMS1
> +
> /* Global constants */
> #define DWC3_PULL_UP_TIMEOUT 500 /* ms */
> #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */
>
>
>
> Please let me know if this would be acceptable.
>
It looks fine to me. Please review the comments and remmove debug
prints and any other cleanup for a proper patch.
Thanks,
Thinh
