From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Mon, 10 Nov 2014 09:01:50 +0800 Subject: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function In-Reply-To: <201411081233.39163.marex@denx.de> References: <1415322494-20415-1-git-send-email-Peng.Fan@freescale.com> <201411071317.05862.marex@denx.de> <545D96F9.8020805@freescale.com> <201411081233.39163.marex@denx.de> Message-ID: <54600E7E.5@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 On 11/8/2014 7:33 PM, Marek Vasut wrote: > On Saturday, November 08, 2014 at 05:07:21 AM, Peng Fan wrote: >> ? 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"))); > > __weak board_usb_phy_mode(int index) is fine. > >> + >> 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. > > Looks OK otherwise. > Sent out v4 patch, please review. Thanks, Peng.