[U-Boot] [PATCH 3/3] imx: mx6sllevk: add usb support

[Marek Vasut]

On 12/22/2016 09:55 AM, Peng Fan wrote:
> 
> 
>>>>>>>>>>> +#define USB_OTHERREGS_OFFSET   0x800
>>>>>>>>>>> +#define UCTRL_PWR_POL          (1 << 9)
>>>>>>>>>>> +
>>>>>>>>>>> +int board_ehci_hcd_init(int port) {
>>>>>>>>>>> +	u32 *usbnc_usb_ctrl;
>>>>>>>>>>> +
>>>>>>>>>>> +	if (port > 1)
>>>>>>>>>>> +		return -EINVAL;
>>>>>>>>>>> +
>>>>>>>>>>> +	usbnc_usb_ctrl = (u32 *)(USB_BASE_ADDR +
>>>>>> USB_OTHERREGS_OFFSET
>>>>>>>>>> +
>>>>>>>>>>> +				 port * 4);
>>>>>>>>>>> +
>>>>>>>>>>> +	/* Set Power polarity */
>>>>>>>>>>> +	setbits_le32(usbnc_usb_ctrl, UCTRL_PWR_POL);
>>>>>>>>>>> +	return 0;
>>>>>>>>>>> +}
>>>>>>>>>>
>>>>>>>>>> Is this function similar to what usb_oc_config() does ?
>>>>>>>>>
>>>>>>>>> No, this bit is not for overcurrent. According to RM, this is
>>>>>>>>> OTG Power Polarity This bit should be set according to power
>>>>>>>>> switch's enable
>>>>>>>> polarity.
>>>>>>>>> 1 Power switch has an active-high enable input
>>>>>>>>> 0 Power switch has an active-low enable input
>>>>>>>>>
>>>>>>>>> This is board specific.
>>>>>>>>
>>>>>>>> Great, except it should also be part of the driver , the same way
>>>>>>>> as polarity is configured, yes ?
>>>>>>>
>>>>>>> I just found that there is code for mx7,
>>>>>>> http://lists.denx.de/pipermail/u-boot/2016-July/260321.html
>>>>>>>
>>>>>>> Then do you agree I add such piece code in usb_power_config for mx6?
>>>>>>> "
>>>>>>> #ifdef CONFIG_MXC_USB_OTG_HACTIVE
>>>>>>>         setbits_le32(ctrl, UCTRL_PWR_POL); #else
>>>>>>>         clrbits_le32(ctrl, UCTRL_PWR_POL); #endif "
>>>>>>
>>>>>> Didn't you add DT support in a patch sent like ... yesterday ? :)
>>>>>> Just use DT property instead of ifdef.
>>>>>
>>>>> There is no property for otg power polatiry in upstream Linux. I
>>>>> would not like to introduce one in U-Boot. We can drop the ifdef
>>>>> when there is an
>>>> property in upstream. What do you think?
>>>>
>>>> So uh, how can the USB work in mainline Linux on that board ?
>>>
>>> Confirmed with IC, if using gpio to control vbus power, no need to configure
>> this bit.
>>> Since in dts, we are using gpio regulator to control vbus, this piece
>>> code in patch 3/3 can be discarded. I'll drop it in V2.
>>
>> Hum, OK. Let's revisit this when we need this then ?
> 
> Take i.MX6SLL as an example,
> The pin key_col4 can be configured with the two functions.
> [1] MX6_PAD_KEY_COL4__USB_OTG1_PWR 
> [2] MX6_PAD_KEY_COL4__GPIO4_IO00
> 
> In fsl i.mx uboot code, the common way is to use [1] and configure the polarity to enable the vbus power.
> But in kernel, it is configured as [2] and use gpio regulator to enable the vbus power.
> 
> This is board specific. There is no upstream property for this bit. When there is an upstream property for this,
> We could add it in probe function for those that use [1] in dts pinmux.
> 
> Now I am using the same dts from kernel which this pinmux configured as [2], so this piece code is not needed for now.

Well, can you submit a proposal with the new prop for devicetree@ ?
It can then be improved in linux too ...

-- 
Best regards,
Marek Vasut