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

Peng Fan
2014-11-10T01:01:50Z

On 11/8/2014 7:33 PM, Marek Vasut wrote:
> On Saturday, November 08, 2014 at 05:07:21 AM, Peng Fan wrote:
>> ? 11/7/2014 8:17 PM, Marek Vasut ??:
>>> On Friday, November 07, 2014 at 12:45:51 PM, Peng Fan wrote:
>>>> ? 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?
>>>
>>> This seems OKish for all but the part where usb_phy_enable() shouldn't be
>>> touched. The return value of usb_phy_enable() should really be a regular
>>> return code, not the PHY mode.
>>
>> ok. I'll fix this.
>>
>>> You can also still implement a function to query a PHY for it's mode, so
>>> you don't need to explicitly read the USBPHY_CTRL_OTG_ID in the board
>>> code.
>>
>> I am not sure whether this following way is fine or not.
>> +int board_usb_phy_mode(int index)
>> + __attribute__((weak, alias("usb_phy_mode")));
>
> __weak board_usb_phy_mode(int index) is fine.
>
>> +
>> in usb_phy_mode, query a PHY for it's mode.
>>
>> And righter after usb_phy_enable in ehci-mx6.c.
>> - type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
>> USB_INIT_HOST;
>> + usb_phy_enable(index, ehci);
>> + type = usb_phy_mode(index);
>>
>> usb_phy_enable return 0 but not return val & USBPHY_CTRL_OTG_ID. There
>> is no status bit for query enabled or not, so just return 0.
>>
>> In board file:
>> int board_usb_phy_mode(int port)
>> {
>> if (port == 1)
>> return USB_INIT_HOST;
>> else
>> return usb_phy_mode(port);
>> }
>>
>> I think this is better way then previous patch, but i did not find where
>> to put the usb_phy_mode prototype type, since board file will use it.
>
> Looks OK otherwise.
>
Sent out v4 patch, please review.

Thanks,
Peng.