[U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

[Peng Fan]

在 11/7/2014 7:09 PM, Marek Vasut 写道:
> On Friday, November 07, 2014 at 12:03:30 PM, Peng Fan wrote:
>
> [...]
>
>>>> @@ -160,7 +174,7 @@ static int usb_phy_enable(int index, struct usb_ehci
>>>> *ehci) val |= (USBPHY_CTRL_ENUTMILEVEL2 | USBPHY_CTRL_ENUTMILEVEL3);
>>>>
>>>>    	__raw_writel(val, phy_ctrl);
>>>>
>>>> -	return val & USBPHY_CTRL_OTG_ID;
>>>> +	return board_usb_phy_mode(index);
>>>
>>> This should be called from ehci_hcd_init() right after usb_phy_enable().
>>> Afterall, the mode detection has nothing to do with the PHY enabling.
>>
>> This back to what I did in patch v2. right after usb_phy_enable(), just
>> paste that piece of code here:
>>
>> The weak function:
>> +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type)
>> +{
>> +       return 0;
>> +}
>> +
>>
>>           type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
>> USB_INIT_HOST;
>>
>> +       board_usb_phy_mode(index, &type);
>> +
>
> The usb_phy_enable() should not return the PHY mode at all though.
> It should be the board_usb_phy_mode() which adjusts the PHY type.
> The usb_phy_enable() should return just a success/failure return
> value.
>
ok. got it.
>> What need to do is to let board can modify the `type` like following:
>> +int board_usb_phy_mode(int port, enum usb_init_type *type)
>> +{
>> +	if (port == 1)
>> +       /* port1 works in HOST Mode */
>> +       	*type = USB_INIT_HOST;
>> +
>> +       return 0;
>> +}
>> +
>> This is the way that I did in patch v2. If this is fine, I'll resent
>> this patch set.
>
> It should really explicitly set it, not modify it, see above.
>
I have an idea about this patch:
1. usb_phy_enable will not be touched.
2. replace "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : 
USB_INIT_HOST;" with "usb_phy_enable(index, ehci)".
3. right after usb_phy_enable, add this line "type = 
board_usb_phy_mode(index)" or "type = board_usb_phy_mode((struct usb_phy 
*)PHY_ADDRESS)". Here I also think pass phy register definition to board 
level code is not fine just as what we talked about passing ehci struct 
to board level code in patch v2.
4. in ehci-mx6.c, implement the weak function "int __weak 
board_usb_phy_mode(xxx)", and it's return value is the mode, HOST or 
DEVICE. If the board code want to implement this function, just return 
what the board want.

After all, this patch may looks like this:
In ehci-mx6.c
+int __weak board_usb_phy_mode(int port)
+{
+       void __iomem *phy_reg;
+       void __iomem *phy_ctrl;
+       u32 val;
+
+       phy_reg = (void __iomem *)phy_bases[port];
+       phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
+
+       val = __raw_readl(phy_ctrl);
+
+       return val & USBPHY_CTRL_OTG_ID;
+}
+

- type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;
+ usb_phy_enable(index, ehci);
+ type = board_usb_phy_mode(index);

in board code, which is not in this patch, just list here:
+int board_usb_phy_mode(int port)
+{
+	if (port == 1)
+		return USB_INIT_HOST;
+	else
+		return USB_INIT_DEVICE;
+}
I just want to keep it simple and do not want to touch usb phy register 
in board code.

Any ideas?
> [...]
>
Regards,
Peng.