From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Sat, 8 Nov 2014 12:07:21 +0800 Subject: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function In-Reply-To: <201411071317.05862.marex@denx.de> References: <1415322494-20415-1-git-send-email-Peng.Fan@freescale.com> <201411071209.06824.marex@denx.de> <545CB0EF.9010106@freescale.com> <201411071317.05862.marex@denx.de> Message-ID: <545D96F9.8020805@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 8:17 PM, Marek Vasut ??: > On Friday, November 07, 2014 at 12:45:51 PM, Peng Fan wrote: >> ? 11/7/2014 7:09 PM, Marek Vasut ??: >>> On Friday, November 07, 2014 at 12:03:30 PM, Peng Fan wrote: >>> >>> [...] >>> >>>>>> @@ -160,7 +174,7 @@ static int usb_phy_enable(int index, struct >>>>>> usb_ehci *ehci) val |= (USBPHY_CTRL_ENUTMILEVEL2 | >>>>>> USBPHY_CTRL_ENUTMILEVEL3); >>>>>> >>>>>> __raw_writel(val, phy_ctrl); >>>>>> >>>>>> - return val & USBPHY_CTRL_OTG_ID; >>>>>> + return board_usb_phy_mode(index); >>>>> >>>>> This should be called from ehci_hcd_init() right after >>>>> usb_phy_enable(). Afterall, the mode detection has nothing to do with >>>>> the PHY enabling. >>>> >>>> This back to what I did in patch v2. right after usb_phy_enable(), just >>>> paste that piece of code here: >>>> >>>> The weak function: >>>> +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type) >>>> +{ >>>> + return 0; >>>> +} >>>> + >>>> >>>> type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : >>>> USB_INIT_HOST; >>>> >>>> + board_usb_phy_mode(index, &type); >>>> + >>> >>> The usb_phy_enable() should not return the PHY mode at all though. >>> It should be the board_usb_phy_mode() which adjusts the PHY type. >>> The usb_phy_enable() should return just a success/failure return >>> value. >> >> ok. got it. >> >>>> What need to do is to let board can modify the `type` like following: >>>> +int board_usb_phy_mode(int port, enum usb_init_type *type) >>>> +{ >>>> + if (port == 1) >>>> + /* port1 works in HOST Mode */ >>>> + *type = USB_INIT_HOST; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> This is the way that I did in patch v2. If this is fine, I'll resent >>>> this patch set. >>> >>> It should really explicitly set it, not modify it, see above. >> >> I have an idea about this patch: >> 1. usb_phy_enable will not be touched. >> 2. replace "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : >> USB_INIT_HOST;" with "usb_phy_enable(index, ehci)". >> 3. right after usb_phy_enable, add this line "type = >> board_usb_phy_mode(index)" or "type = board_usb_phy_mode((struct usb_phy >> *)PHY_ADDRESS)". Here I also think pass phy register definition to board >> level code is not fine just as what we talked about passing ehci struct >> to board level code in patch v2. >> 4. in ehci-mx6.c, implement the weak function "int __weak >> board_usb_phy_mode(xxx)", and it's return value is the mode, HOST or >> DEVICE. If the board code want to implement this function, just return >> what the board want. >> >> After all, this patch may looks like this: >> In ehci-mx6.c >> +int __weak board_usb_phy_mode(int port) >> +{ >> + void __iomem *phy_reg; >> + void __iomem *phy_ctrl; >> + u32 val; >> + >> + phy_reg = (void __iomem *)phy_bases[port]; >> + phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL); >> + >> + val = __raw_readl(phy_ctrl); >> + >> + return val & USBPHY_CTRL_OTG_ID; >> +} >> + >> >> - type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST; >> + usb_phy_enable(index, ehci); >> + type = board_usb_phy_mode(index); >> >> in board code, which is not in this patch, just list here: >> +int board_usb_phy_mode(int port) >> +{ >> + if (port == 1) >> + return USB_INIT_HOST; >> + else >> + return USB_INIT_DEVICE; >> +} >> I just want to keep it simple and do not want to touch usb phy register >> in board code. >> >> Any ideas? > > This seems OKish for all but the part where usb_phy_enable() shouldn't be > touched. The return value of usb_phy_enable() should really be a regular > return code, not the PHY mode. > ok. I'll fix this. > You can also still implement a function to query a PHY for it's mode, so you > don't need to explicitly read the USBPHY_CTRL_OTG_ID in the board code. > I am not sure whether this following way is fine or not. +int board_usb_phy_mode(int index) + __attribute__((weak, alias("usb_phy_mode"))); + in usb_phy_mode, query a PHY for it's mode. And righter after usb_phy_enable in ehci-mx6.c. - type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST; + usb_phy_enable(index, ehci); + type = usb_phy_mode(index); usb_phy_enable return 0 but not return val & USBPHY_CTRL_OTG_ID. There is no status bit for query enabled or not, so just return 0. In board file: int board_usb_phy_mode(int port) { if (port == 1) return USB_INIT_HOST; else return usb_phy_mode(port); } I think this is better way then previous patch, but i did not find where to put the usb_phy_mode prototype type, since board file will use it. Regards, Peng.