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

[Peng Fan <B51431@freescale.com>]

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.