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

[Peng Fan <B51431@freescale.com>]

? 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?
>
Regards,
Peng.