From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Wed, 5 Nov 2014 17:18:25 +0800 Subject: [U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function In-Reply-To: <201411051003.42245.marex@denx.de> References: <1415087402-26007-1-git-send-email-Peng.Fan@freescale.com> <201411041833.28138.marex@denx.de> <5459BD00.1060006@freescale.com> <201411051003.42245.marex@denx.de> Message-ID: <5459EB61.6010202@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de ? 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.