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

[Peng Fan]

在 11/8/2014 12:07 PM, Peng Fan 写道:
>
>
> 在 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);
This should be 'type = board_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.
Regards,
Peng.