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

[Peng Fan <B51431@freescale.com>]

? 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")));
+
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.

Regards,
Peng.