From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Sat, 8 Nov 2014 12:35:14 +0800 Subject: [U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function In-Reply-To: <545D96F9.8020805@freescale.com> 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> <545D96F9.8020805@freescale.com> Message-ID: <545D9D82.3030707@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/8/2014 12:07 PM, Peng Fan ??: > > > ? 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); This should be 'type = board_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. Regards, Peng.