Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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