[U-Boot] [RFC PATCH 05/11] ehci: Add support for Qualcomm EHCI

[Marek Vasut <marex@denx.de>]

On Sunday, December 13, 2015 at 01:38:41 PM, Mateusz Kulikowski wrote:
> Hi,
> 
> Thanks for quick review;
> 
> On 11.12.2015 00:22, Marek Vasut wrote:
> > On Thursday, December 10, 2015 at 10:41:41 PM, Mateusz Kulikowski wrote:
> [...]
> 
> >> +
> >> +#ifndef CONFIG_USB_ULPI_VIEWPORT
> >> +#error Please enable CONFIG_USB_ULPI_VIEWPORT
> >> +#endif
> > 
> > The driver should select this in Kconfig instead of this check, right ?
> 
> That was my first attempt, but ULPI_VIEWPORT is not Kconfig option,
> and it seems it just doesn't work :(
> 
> It doesn't matter if I add it as
> select USB_ULPI_VIEWPORT
> in usb KConfig, or forcibly add CONFIG_USB_ULPI_VIEWPORT to .config

I think it should be quite easily possible to add this to USB Kconfig.

> [...]
> 
> >> +#define USB_USBCMD           (0x0140)
> > 
> > Please drop the parenthesis.
> 
> Doh, missed this one - surely will do it.
> 
> > btw. this register layout looks very similar to struct usb_ehci in
> > include/usb/ehci-fsl.h , can the header be made more universal to
> > cover your driver as well ? Then these macros here won't be needed.
> 
> You're right.. in fact contrary to what I expected, Qualcomm didn't
> implemented their own USB controller.

Well building a chip is like going to a IP block supermarket afterall ;-)

> It is designed by Chipidea, and PHY as far as I see is made by Synapsys.

All of the drivers/usb/host/ehci-{mxs,mx5,mx6,vf}.c are also chipidea
cores. MXS and MX6 have chipidea PHY too.

> I can use fsl headers with little exception that two registers are
> marked as reserved: USB_AHB_MODE (0x98) and USB_GENCONFIG_2 (0xA0)
> 
> My guess is that it's just different revision/config of IP core.
> 
> Do you think it wouldn't look awkward if I use fsl headers?

Just rename them to ehci-ci.h, that should be the quickest.

> I think once this series gets to mainline we can create shared
> driver that will support both vendors.

I'm all for that.

> I also noticed that U-Boot have ci_udc already so there is
> a chance I make device controller working pretty fast
> (but I prefer not to include it in this series yet).

Correct, that works too. I think it was also tested on some marvell chip.

> [...]
> 
> >> +	struct ulpi_viewport ulpi_vp = {.port_num = priv->ulpi_port,
> >> +					.viewport_addr = priv->ulpi_base};
> >> +
> >> +	/* Disable VBUS mimicing in the controller. */
> >> +	ulpi_write(&ulpi_vp, (u8 *)ULPI_MISC_A_CLEAR,
> > 
> > This should be a pointer to a field in struct ulpi_regs, so the (u8 *)
> > cast does not seem right. See for example ehci-zynq.c
> 
> Perhaps I misussed ulpi_viewport code in that case;
> 
> The reason is I need to access MISC_A register (0x96+) that is
> not in ulpi_regs structure - afaik it's vendor-specific.
> 
> Any hints how to tackle that properly?
> 
> I can of course duplicate ulpi code, but it probably doesn't make much
> sense.

I don't have a better suggestion, sorry. Let's keep this as-is unless
someone can come up with something better. Code duplication is not a
good idea, so we won't do that.

> [...]
> 
> >> +
> >> +	/* Stop controller. */
> >> +	writel(readl(reg) & ~USBCMD_ATTACH, reg);
> > 
> > This should use clrbits_le32() instead.
> 
> Ok
> 
> [...]
> 
> >> +	/* Wait for completion */
> >> +	while (readl(reg) & 2)
> >> +		;
> > 
> > Look at wait_for_bit() implementations in the U-Boot tree and avoid the
> > unbounded waiting here please.
> 
> Ok, btw I noticed there are 3 copies of almost the same code that does that
> :)
> 
> Perhaps I can add a patch to add this function to /lib as it seems it's
> common use case?

That'd be great, it was on my list to do it for a while, but I didn't get around 
doing that yet.

> The following drivers would benefit: ehci-msm, zynq_gem, dwc2,
> ehci-mx6, ohci-lpc32xx

Thanks!