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

[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.