On Thu, 14 May 2015, Rob Herring wrote:
> On Thu, May 14, 2015 at 9:56 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
