[U-Boot] [PATCH v3 2/4] USB: host: Add the USB3 host driver

[Sherry Sun]

Hi Marek,

> 
> On 8/16/19 8:10 AM, Sherry Sun wrote:
> 
> [...]
> 
> > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index
> > ac68aa2d27..cc1dfe463b 100644
> > --- a/drivers/usb/host/Kconfig
> > +++ b/drivers/usb/host/Kconfig
> > @@ -95,6 +95,15 @@ config USB_XHCI_FSL
> >  	depends on !SPL_NO_USB
> >  	help
> >  	  Enables support for the on-chip xHCI controller on NXP Layerscape
> SoCs.
> > +
> > +config USB_XHCI_IMX8
> > +	bool "XHCI support for imx8"
> 
> i.MX8 I guess ?

Yes, you are right, I have changed these to i.MX8 instead of imx8.

> 
> [...]
> 
> > diff --git a/drivers/usb/host/xhci-imx8.c
> > b/drivers/usb/host/xhci-imx8.c new file mode 100644 index
> > 0000000000..0669a05c17
> > --- /dev/null
> > +++ b/drivers/usb/host/xhci-imx8.c
> > @@ -0,0 +1,189 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2019 NXP
> > + *
> > + * NXP i.MX8 USB HOST xHCI Controller (Cadence IP)
> > + *
> > + * Author: Peter Chen <peter.chen at nxp.com>  */
> > +
> > +#include <common.h>
> > +#include <usb.h>
> > +#include <malloc.h>
> > +#include <dm.h>
> > +#include <dm/device-internal.h>
> > +#include <clk.h>
> > +#include <wait_bit.h>
> > +#include <power-domain.h>
> > +#include <generic-phy.h>
> > +#include <asm/arch/clock.h>
> > +#include <linux/compat.h>
> 
> Keep the list sorted

Okay, I have done this.

> 
> > +#include "xhci.h"
> > +
> > +/* Declare global data pointer */
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +/* Host registers */
> > +#define HCIVERSION_CAPLENGTH                  0x10000
> > +#define USBSTS                                0x10084
> > +
> > +/* None-core registers */
> > +#define USB3_CORE_CTRL1    0x00
> > +#define USB3_CORE_STATUS   0x0c
> > +#define USB3_SSPHY_STATUS  0x4c
> > +
> > +struct xhci_imx8_data {
> > +	void __iomem *usb3_ctrl_base;
> > +	void __iomem *usb3_core_base;
> > +	struct clk_bulk clks;
> > +	struct phy phy;
> > +};
> > +
> > +static struct xhci_imx8_data imx8_data;
> > +
> > +static void imx8_xhci_init(void)
> > +{
> > +	u32 tmp_data;
> > +	int ret;
> > +
> > +	tmp_data = readl(imx8_data.usb3_ctrl_base + USB3_SSPHY_STATUS);
> > +	writel(tmp_data, imx8_data.usb3_ctrl_base + USB3_SSPHY_STATUS);
> 
> Is this read-write really needed ?

I reviewed this register definition. And found the read is not needed, but the write is needed.
I have changed this read-write already.

> 
> > +	tmp_data = readl(imx8_data.usb3_ctrl_base + USB3_SSPHY_STATUS);
> > +	ret = wait_for_bit_le32(imx8_data.usb3_ctrl_base + USB3_SSPHY_STATUS,
> > +				0xf0000000, true, 100, false);
> > +	if (ret)
> > +		printf("clkvld is incorrect\n");
> 
> Shouldn't this return ret ?

Okay, have done this.
> 
> > +	clrsetbits_le32(imx8_data.usb3_ctrl_base + USB3_CORE_CTRL1,
> > +			0x7, 0x202);
> > +	clrbits_le32(imx8_data.usb3_ctrl_base + USB3_CORE_CTRL1, 1 << 26);
> > +	generic_phy_init(&imx8_data.phy);
> > +
> > +	/* clear all sw_rst */
> > +	clrbits_le32(imx8_data.usb3_ctrl_base + USB3_CORE_CTRL1, 0xFC <<
> > +24);
> > +
> > +	debug("wait xhci_power_on_ready\n");
> > +	ret = wait_for_bit_le32(imx8_data.usb3_ctrl_base + USB3_CORE_STATUS,
> > +				0x1000, true, 100, false);
> > +	if (ret)
> > +		printf("wait xhci_power_on_ready timeout\n");
> 
> return ret on failure ?
Okay, have done this.

> 
> > +	debug("xhci_power_on_ready\n");
> > +
> > +	debug("waiting CNR 0x%x\n", tmp_data);
> > +	ret = wait_for_bit_le32(imx8_data.usb3_core_base + USBSTS,
> > +				0x800, false, 100, false);
> > +	if (ret)
> > +		printf("wait CNR timeout\n");
> 
> return ret on failure ?
Okay, have done this.

> 
> > +	debug("check CNR has finished\n");
> > +}
> > +
> > +static void imx8_xhci_reset(void)
> > +{
> > +	/* Set CORE ctrl to default value, that all rst are hold */
> > +	writel(0xfc000001, imx8_data.usb3_ctrl_base + USB3_CORE_CTRL1);
> 
> What are these magic values ? Add macros for them.
Okay, have done this.

> 
> > +}
> > +
> > +static int xhci_imx8_clk_init(struct udevice *dev) {
> > +	int ret;
> > +
> > +	ret = clk_get_bulk(dev, &imx8_data.clks);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_enable_bulk(&imx8_data.clks);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int xhci_imx8_get_reg_addr(struct udevice *dev) {
> > +	imx8_data.usb3_ctrl_base =
> > +			(void __iomem *)devfdt_get_addr_index(dev, 0);
> > +	imx8_data.usb3_core_base =
> > +			(void __iomem *)devfdt_get_addr_index(dev, 4);
> > +
> > +	return 0;
> > +}
> > +
> > +static int xhci_imx8_probe(struct udevice *dev) {
> > +	struct xhci_hccr *hccr;
> > +	struct xhci_hcor *hcor;
> > +	struct udevice usbotg_dev;
> > +	struct power_domain pd;
> > +	int usbotg_off;
> > +	int ret = 0;
> > +	int len;
> > +
> > +	usbotg_off = fdtdec_lookup_phandle(gd->fdt_blob,
> > +					   dev_of_offset(dev),
> > +					   "cdns3,usb");
> > +	if (usbotg_off < 0)
> > +		return -EINVAL;
> > +	usbotg_dev.node = offset_to_ofnode(usbotg_off);
> > +	usbotg_dev.parent = dev->parent;
> > +	xhci_imx8_get_reg_addr(&usbotg_dev);
> > +
> > +#if CONFIG_IS_ENABLED(POWER_DOMAIN)
> > +	if (!power_domain_get(&usbotg_dev, &pd)) {
> > +		if (power_domain_on(&pd))
> > +			return -EINVAL;
> 
> ret = power_domain_on()
> if (ret)
>   return ret;
> 
Okay, I have done this.

Bets regards 
Sherry sun

> 
> [...]