From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Fri, 7 Nov 2014 09:10:28 +0800 Subject: [U-Boot] [PATCH v2 1/3] usb:ehci-mx6 add board_ehci_usb_mode function In-Reply-To: <201411062120.22547.marex@denx.de> References: <1415087402-26007-1-git-send-email-Peng.Fan@freescale.com> <201411051003.42245.marex@denx.de> <5459EB61.6010202@freescale.com> <201411062120.22547.marex@denx.de> Message-ID: <545C1C04.7090108@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/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.