Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Alan Stern <stern@rowland.harvard.edu>]

On Thu, 14 May 2015, Rob Herring wrote:

> On Thu, May 14, 2015 at 9:56 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 13 May 2015, Rob Herring wrote:
> >
> >> Add platform driver for Marvell PXA1928 SOC. This is different from the
> >> mv-ehci driver in that it uses the generic phy framework, uses DT, does
> >> not use platform_data, is host only, and has a specific HSIC PHY and
> >> controller initialization handshake.
> >
> > Are those differences sufficient to make a separate source file
> > necessary?  There are plenty of other drivers that work for both DT and
> > non-DT, for example.
> 
> IMO, yes. If it were only DT support, then no it would not make sense.
> I don't disagree there are too many ehci drivers mostly varying in phy
> control.
> 
> Actually, ehci-platform is a closer match to this driver. I could use
> it perhaps and either add my custom power_on function and a match
> table entry to it or export ehci-platform functions (probe,
> power_{on,off}, and use them here. Opinion on which direction you
> prefer?

If you can use ehci-platform in place of a more specific driver, that 
would be great.  Custom power-on, power-suspend, and power-off routines 
aren't a problem.

> There's also a resume issue I have to figure out how support too. This
> is what the vendor driver does:
> 
> @@ -469,6 +470,40 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
>                         set_bit(i, &resume_needed);
>                 }
>                 ehci_writel(ehci, temp, &ehci->regs->port_status [i]);
> +
> +               /* for HSIC phy, we have already disable SE0 state when
> +                * resume back. we need do following step to pull USB
> +                * controller back. */
> +#ifdef CONFIG_USB_EHCI_MV_U2H_HSIC
> +#define PHY_TEST_GRP_0 0x10
> +
> +#define S2H_TEST_UTMI_SEL (0x1 << 7)
> +#define S2H_TEST_SUSPENDM (0x1 << 14)
> +#define S2H_TEST_TX_BITSTUFF_EN (0x1 << 29)
> +
> +               if (hcd->usb_phy->is_hsic) {
> +                       void __iomem *phy_base = hcd->usb_phy->io_priv;
> +                       unsigned int count;
> +                       mdelay(10);
> +
> +                       temp = readl(phy_base + PHY_TEST_GRP_0);
> +                       writel(temp | (S2H_TEST_UTMI_SEL
> +                                       | S2H_TEST_SUSPENDM
> +                                       | S2H_TEST_TX_BITSTUFF_EN),
> +                                       phy_base + PHY_TEST_GRP_0);
> +
> +                       count = 1000;
> +                       while (--count && (ehci_readl(ehci,
> +                               &ehci->regs->port_status[i]) & PORT_RESUME))
> +                               udelay(50);
> +                       if (count <= 0)
> +                               ehci_dbg(ehci, "resume time out\n");
> +                       writel(readl(phy_base + PHY_TEST_GRP_0)
> +                               & ~S2H_TEST_UTMI_SEL,
> +                               phy_base + PHY_TEST_GRP_0);
> +               }
> +#endif

It's not entirely clear why this is needed.  For example, why wait for 
PORT_RESUME to turn off if you never turned it on in the first place?

Anyway, it looks like this won't be so easy to integrate with 
ehci-platform.  Perhaps this could be done in the custom power-on 
routine; I don't know if that's feasible.

> >> +config USB_EHCI_MV_OF
> >> +     bool "EHCI OF support for Marvell PXA/MMP USB controller"
> >> +     depends on (ARCH_PXA || ARCH_MMP)
> >> +     select USB_EHCI_ROOT_HUB_TT
> >> +     ---help---
> >> +       Enables support for Marvell (including PXA and MMP series) on-chip
> >> +       USB SPH and OTG controller. SPH is a single port host, and it can
> >> +       only be EHCI host. OTG is controller that can switch to host mode.
> >> +       Note that this driver will not work on Marvell's other EHCI
> >> +       controller used by the EBU-type SoCs including Orion, Kirkwood,
> >> +       Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
> >> +       on-chip EHCI USB controller" for those.
> >
> > This is identical to the help text for USB_EHCI_MV.  How is a user
> > supposed to know which option to enable?
> 
> The OTG part needs to go. Perhaps I need to make it PXA1928 specific.
> All I know about Marvell IP is it is scattered all over the place, so
> determining which SOCs are the same IP is difficult. The PHYs for sure
> seem to be different on every chip as I did not find any existing
> Marvell phy drivers that match.

Obviously this is the kind of thing that a developer is supposed to
sort out, rather then making users responsible for finding the right
combination of settings for their hardware.

Alan Stern