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