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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
