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....@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? _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot