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

[Sherry Sun]

Hi Marek,

> 
> On 8/20/19 10:31 AM, Sherry Sun wrote:
> > Hi Marek,
> >
> >>
> >> On 8/19/19 8:10 AM, Sherry Sun wrote:
> >>> Add the USB3 host driver for NXP imx8 platform, and the cadence IP
> >>> is in it. The USB3 host driver support DM mode, it will probe USB3
> >>> host node in dts.
> >>>
> >>> Signed-off-by: Sherry Sun <sherry.sun at nxp.com>
> >>
> >> [...]
> >>
> >>> +static void 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); }
> >>
> >> Inline this.
> >>
> >> Also, look at drivers/mtd/renesas_rpc_hf.c to handle both 32bit and
> >> 64bit DTs with multiple "reg" entries.
> >>
> >
> > Actually, I don't think it's needed to change the way getting reg in dts.
> > For two reasons:
> > 1. This xhci-imx8 driver is only target on imx8 platform which all use 64bit
> dts.
> > 2. devfdt_get_addr_index() can handle both 32/64 bits dts too.
> 
> By hard-coding this "4" offset, you assume the DT is using 64bit addressing,
> that's wrong, so please fix it. The iMX8 does support aarch32, right ? So
> someone might pass in 32bit DT and this would break.

Sorry, maybe there are some misunderstandings in the code.
There five reg elements in dts node, which are none_core_regs/ xhci_regs/ dev_regs/ phy_regs/ otg_regs.
devfdt_get_addr_index(dev, 4) only want to get the fifth reg value, instead of meanning 64bit address.

586     usbotg3: usb3 at 5b110000 {
587         compatible = "cdns,usb3";
588         reg = <0x0 0x5B110000 0x0 0x10000>,
589             <0x0 0x5B130000 0x0 0x10000>,
590             <0x0 0x5B140000 0x0 0x10000>,
591             <0x0 0x5B160000 0x0 0x40000>,
592             <0x0 0x5B120000 0x0 0x10000>;

> 
> >> [...]
> >>
> >>> +static const struct udevice_id xhci_usb_ids[] = {
> >>> +	{ .compatible = "cdns,usb3-host", },
> >>
> >> https://lore.ker
> >>
> nel.org%2Fpatchwork%2Fpatch%2F1059917%2F&data=02%7C01%7Cshe
> >>
> rry.sun%40nxp.com%7C70e116ce152a4060dbd508d724987b54%7C686ea1d
> >>
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637018109631327748&sd
> >>
> ata=GzX5KNQ8m0CCeBa9tPSV%2BOEwwuSsxDdAiJd1T6or8P0%3D&reser
> >> ved=0 would suggest that
> >> cdns,usb3-1.0.0 is the compatible. But I might be wrong.
> >
> > This patch has not been accepted by the kernel, right?
> > So I think maybe we can do this change to synchronize with kernel
> > after the cdns3 kernel patch been accepted.
> 
> Or, we can start with at least reasonably OK DT compatible right away to
> reasonably avoid ABI breakage.

Sure, will change the compatible from "cdns,usb3" to "cdns,usb3-1.0.0".

> 
> > And note that in uboot, one dts node can only bind with one driver, so
> > we keep the cdns3 node for usb gadget driver, then add a usb host node
> > for
> > xhci-imx8 driver in *-uboot.dtsi. so here is no need to change the host driver
> compatible.
> > But the compatible in gadget driver should be changed later.
> 
> We should try avoiding ABI breaks in DT.

But the cdns3 usb gaget driver and host driver in different uclass need two dt nodes to bind with.
And the compatible of the two node cannot be same.
So for gadget driver, the compatible may use "cdns,usb3-1.0.0", for host driver, the compatible will use "cdns,usb3-1.0.0-host".
What do you think about it?

> 
> >>> +	{ }
> >>> +};
> >>> +
> >>> +U_BOOT_DRIVER(xhci_imx8) = {
> >>> +	.name	= "xhci_imx8",
> >>> +	.id	= UCLASS_USB,
> >>> +	.of_match = xhci_usb_ids,
> >>> +	.probe = xhci_imx8_probe,
> >>> +	.remove = xhci_imx8_remove,
> >>> +	.ops	= &xhci_usb_ops,
> >>> +	.platdata_auto_alloc_size = sizeof(struct usb_platdata),
> >>> +	.priv_auto_alloc_size = sizeof(struct xhci_ctrl),
> >>> +	.flags	= DM_FLAG_ALLOC_PRIV_DMA,
> >>
> >> I think you also need DM_FLAG_OS_PREPARE to trigger .remove before
> >> booting Linux, but I might be wrong. Please check that.
> >
> > When use usb stop command to stop the usb host driver,
> > device_remove(bus, DM_REMOVE_NORMAL) is called by usb_stop(), so
> > normally we don’t need the DM_FLAG_OS_PREPARE flag.
> > But considering some special circumstances, maybe add this flag is helpful.
> 
> I'm not talking about "usb stop", I am talking about booting Linux.
> That's when then .remove should be called to tear down the driver. Is it?

I know what you mean. The only function of DM_FLAG_OS_PREPARE 
is to call .remove before booting kernel, to make sure the device is 
removed.
But normally, the host device is removed every time after we use it.
So even the flag is set, the .remove won't be called again before booting kernel
because the device is checked to be removed already.
Add the flag just make sure the device will be removed before booting kernel.
Do you think so?

Best regard
Sherry sun