[U-Boot] [PATCH v3 1/3] usb:ehci-mx6 add board_usb_phy_mode function

[Marek Vasut <marex@denx.de>]

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.

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.