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

[Marek Vasut <marex@denx.de>]

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.