[PATCH 4/5] usb: xhci: Add Qualcomm DWC3 xHCI driver

[Robert Marko]

On Wed, Aug 19, 2020 at 9:34 PM Marek Vasut <marex at denx.de> wrote:
>
> On 8/19/20 8:22 PM, Tom Rini wrote:
> [...]
> >> +#include <common.h>
> >> +#include <dm.h>
> >> +#include <usb.h>
> >> +#include <linux/compat.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/usb/dwc3.h>
> >> +#include <usb/xhci.h>
>
> Please keep the list sorted.
>
> >> +/* Declare global data pointer */
> >> +DECLARE_GLOBAL_DATA_PTR;
> >> +
> >> +struct ipq_xhci_platdata {
> >> +    fdt_addr_t hcd_base;
> >> +    unsigned int rst_ctrl;
> >> +    unsigned int hs_only;
>
> bool ...
>
> >> +};
> >> +
> >> +struct ipq_xhci {
> >> +    struct ipq_xhci_platdata usb_plat;
> >> +    struct xhci_ctrl ctrl;
> >> +    struct udevice* dev;
> >> +    struct xhci_hccr *hcd;
> >> +    struct dwc3 *dwc3_reg;
> >> +};
> >> +
> >> +void ipq_reset_usb_phy(void *data)
> >> +{
> >> +    unsigned int gcc_rst_ctrl;
> >> +    struct ipq_xhci_platdata *platdata;
> >> +    struct ipq_xhci *ipq = (struct ipq_xhci *)data;
>
> Pass struct ipg_xhci pointer in instead of void *.
>
> >> +    platdata = dev_get_platdata(ipq->dev);
> >> +    if (platdata == NULL) {
> >> +            printf("Error: %s Failed\n", __func__);
>
> dev_err() here.
>
> >> +            return;
> >> +    }
>
> Shouldn't this be part of a PHY driver ?
>
> >> +    gcc_rst_ctrl = platdata->rst_ctrl;
> >> +
> >> +    if (gcc_rst_ctrl) {
> >> +            /* assert HS PHY POR reset */
> >> +            setbits_le32(gcc_rst_ctrl, 0x10);
> >> +            mdelay(10);
>
> Does it really need such lengthy delays here ?
>
> >> +            /* assert HS PHY SRIF reset */
> >> +            setbits_le32(gcc_rst_ctrl, 0x4);
> >> +            mdelay(10);
> >> +
> >> +            /* deassert HS PHY SRIF reset and program HS PHY registers */
> >> +            clrbits_le32(gcc_rst_ctrl, 0x4);
> >> +            mdelay(10);
> >> +
> >> +            /* de-assert USB3 HS PHY POR reset */
> >> +            clrbits_le32(gcc_rst_ctrl, 0x10);
>
> This BIT(4) should likely be #define'd as a macro , same for the others.
>
> >> +            mdelay(10);
> >> +
> >> +            if (!platdata->hs_only) {
> >> +                    /* assert SS PHY POR reset */
> >> +                    setbits_le32(gcc_rst_ctrl, 0x20);
> >> +                    mdelay(10);
> >> +
> >> +                    /* deassert SS PHY POR reset */
> >> +                    clrbits_le32(gcc_rst_ctrl, 0x20);
> >> +            }
> >> +    }
> >> +}
> >> +
> >> +static int ipq_xhci_core_init(struct ipq_xhci *ipq)
> >> +{
> >> +    int ret = 0;
> >> +
> >> +    ipq_reset_usb_phy((void *)ipq);
> >> +
> >> +    ret = dwc3_core_init(ipq->dwc3_reg);
> >> +    if (ret) {
> >> +            debug("%s:failed to initialize core\n", __func__);
>
> dev_dbg()
>
> >> +            return ret;
> >> +    }
> >> +
> >> +    /* We are hard-coding DWC3 core to Host Mode */
> >> +    dwc3_set_mode(ipq->dwc3_reg, DWC3_GCTL_PRTCAP_HOST);
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +static void ipq_xhci_core_exit(struct ipq_xhci *ipq)
> >> +{
>
> Is some code missing here ?
>
> >> +}
> >> +
> >> +static int xhci_usb_remove(struct udevice *dev)
> >> +{
> >> +    int ret;
> >> +    ret = xhci_deregister(dev);
> >> +
> >> +    if (ret != 0) {
> >> +            debug("%s:xhci deregistration failed\n", __func__);
>
> dev_dbg()
>
> >> +            return ret;
> >> +    }
> >> +
> >> +    ipq_xhci_core_exit(dev_get_priv(dev));
> >> +
> >> +    return 0;
>
> return ipg_xhci_core_exit() to propagate the error value (if any).
>
> >> +}
>
> [...]

Hi Marek,
thanks for the review.

I have moved the USB PHY-s into a separate driver as Linux does.
I have also dropped this custom driver and moved to use of the DWC3
generic glue one,
that way nodes and everything will match Linux as much as possible.
Unfortunately, it will most likely take a while for me to send a v2 of
this patchset as USB3.0 capable port is not working.
USB2.0 one is working fine and detecting the TI CC2540 I have
connected to it, but USB3.0 external Type-A port won't detect
USB mass storage or WLAN adapters at all.

Do you maybe have advice on how to debug this?

Regards
Robert