[U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function

[Peng Fan <B51431@freescale.com>]

? 11/7/2014 4:20 AM, Marek Vasut ??:
> On Wednesday, November 05, 2014 at 10:18:25 AM, Peng Fan wrote:
>> ? 11/5/2014 5:03 PM, Marek Vasut ??:
>>> On Wednesday, November 05, 2014 at 07:00:32 AM, Peng Fan wrote:
>>>> ? 11/5/2014 1:33 AM, Marek Vasut ??:
>>>>> On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote:
>>>>>> Hi Marek,
>>>>>>
>>>>>> ? 11/4/2014 7:01 PM, Marek Vasut ??:
>>>>>>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote:
>>>>>>>> ? 11/4/2014 6:33 PM, Marek Vasut ??:
>>>>>>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote:
>>>>>>>>>> Include a weak function board_ehci_usb_mode to gives board code
>>>>>>>>>> a choice.
>>>>>>>>>
>>>>>>>>> What choice?
>>>>>>>>>
>>>>>>>>>> If the board want the otg port work in host mode but not
>>>>>>>>>> device mode, this should be handled.
>>>>>>>>>
>>>>>>>>> How?
>>>>>>>>>
>>>>>>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of
>>>>>>>>> selection between device and host mode ?
>>>>>>>>
>>>>>>>> In mx6sxsabresd board, there are two usb port, one used for otg, the
>>>>>>>> other used for host. However they are connected to SOC USB
>>>>>>>> controller otg1 core and otg2 core respectively. Like following:
>>>>>>>>
>>>>>>>> OTG1 CORE <----> board otg port
>>>>>>>> OTG2 CORE <----> board host port
>>>>>>>>
>>>>>>>> However the board do not have ID pin set for board host port. If
>>>>>>>> just use usb_phy_enable, the board host port will not work, because
>>>>>>>> "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE :
>>>>>>>> USB_INIT_HOST;" will always set type with USB_INIT_DEVICE.
>>>>>>>>
>>>>>>>> Because i did not find way to handle this situation in
>>>>>>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to
>>>>>>>> let board level code handle handle 'type', if board level code want
>>>>>>>> to set it's own 'type'.
>>>>>>>
>>>>>>> This part in usb_phy_enable()
>>>>>>>
>>>>>>> 163         return val & USBPHY_CTRL_OTG_ID;
>>>>>>>
>>>>>>> should be replaced by some kind of a board-specific callback then,
>>>>>>> with default implmentation being the above (reading the phy ctrl
>>>>>>> register).
>>>>>>
>>>>>> How about using the following piece of code?
>>>>>> in ehci-mx6.c
>>>>>>
>>>>>> unsigned int __weak board_usb_phy_mode(int index, unsigned int val)
>>>>>> {
>>>>>>
>>>>>> 	return val & USBPHY_CTRL_OTG_ID;
>>>>>>
>>>>>> }
>>>>>>
>>>>>> replace "return val & USBPHY_CTRL_OTG_ID;" using "
>>>>>> return board_usb_phy_mode(index, val);"
>>>>>>
>>>>>> In board file,
>>>>>> unsigned int board_usb_phy_mode(int index, unsigned int val)
>>>>>
>>>>> Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc
>>>>> $val into the function doesn't seem like a scalable future-proof
>>>>> solution. [...]
>>>>
>>>> Passing struct usb_ehci * to board code seems exports ehci register
>>>> definition to board layer.
>>>
>>> Yeah.
>>>
>>>> How about just use
>>>> "int board_usb_phy_mode(int index)" without using 'val' or 'struct
>>>> usb_ehci *ehci'.
>>>
>>> The board part might need to read the EHCI registers though. How would
>>> the board part be able to do it if you didn't pass the *ehci in ?
>>
>> To imx6, the ID bit is in PHY ctrl reg 'USBPHYx_CTRLn', also the phy
>> regs definition are not included in "struct usb_ehci". I just think
>> expose the ehci register to board layer is not fine and
>> board_usb_phy_mode does not need this. I define this just as
>> "board_ehci_hcd_init" and "board_ehci_power". Their prototype are
>> int __weak board_ehci_hcd_init(int port);
>> int __weak board_ehci_power(int port, int on);
>>
>> My implementation is the following:
>>
>> replace "return val & USBPHY_CTRL_OTG_ID;" using "return
>> board_usb_phy_mode(index);" in usb_phy_enable
>>
>> In drivers/usb/host/ehci-mx6.c:
>> 116 int __weak board_usb_phy_mode(int index)
>> 117 {
>> 118         void __iomem *phy_reg;
>> 119         void __iomem *phy_ctrl;
>> 120         u32 val;
>> 121
>> 122         phy_reg = (void __iomem *)phy_bases[index];
>> 123         phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
>> 124
>> 125         val = __raw_readl(phy_ctrl);
>> 126
>> 127         return val & USBPHY_CTRL_OTG_ID;
>> 128 }
>>
>> In board/freescale/mx6sxsabresd/mx6sxsabresd.c:
>> 295 int board_usb_phy_mode(int port)
>> 296 {
>> 297         void __iomem *phy_reg;
>> 298         void __iomem *phy_ctrl;
>> 299         u32 val;
>> 300
>> 301         switch (port) {
>> 302         case 0:
>> 303                 phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR;
>> 304                 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL);
>> 305                 val = __raw_readl(phy_ctrl);
>> 306                 return val & USBPHY_CTRL_OTG_ID;
>> 307         case 1:
>> 308                 /* Work in HOST mode. */
>> 309                 return 0;
>> 310         }
>> 311
>> 312         /* suppress warning msg */
>> 313         return 0;
>> 314 }
>>
>> Is this piece of code fine?
>
> These ad-hoc hooks are starting to become absolute horror, but I guess
> this one (if properly documented) might just work. Let's see what will
> come out of this approach.
>
Sent out v3 patch set just now. Please review.

Thanks,
Peng.