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

[Marek Vasut]

On 8/20/19 5:24 PM, Sherry Sun wrote:
> Hi Marek,

Hi,

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

Look the registers up by name then, each entry should have it's own name
in the DT. git grep for "reg-names" .

[...]

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

CCing Jean, since I think he did solve similar topic for his platform.

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

How so ? The controller is left running until you call .remove()

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

You want to tear the controller down before booting kernel, e.g. to
prevent it from doing some rogue DMA into memory, while the kernel
starts up. This might mess the system up.