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

[Marek Vasut]

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.

>> [...]
>>
>>> +static const struct udevice_id xhci_usb_ids[] = {
>>> +	{ .compatible = "cdns,usb3-host", },
>>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.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.

> 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.

>>> +	{ }
>>> +};
>>> +
>>> +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?