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

[Mateusz Kulikowski]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

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

[...]
>> +#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.

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

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?

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

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).

[...]
>> +	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.


[...]
>> +
>> +	/* 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?

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


Regards,
Mateusz
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJWbWbJAAoJELvtohmVtQzBvEEH/iwqONAqWwqQzpUR4izzZ97Y
CAEUWi4GacxwUVt0vZMcK5KV0sRJVP947daMxVkNoDWWkpREuPby+OecFe3mk7iJ
cJzTAlYs/OOIkGBuza2wkfaxXq0AItpn2lBF/Vwe8u5hFGSPgYY0quek8SKma6NQ
rtNFVdc+4+pgGMy1Pl8Fym9UXOJ/aVv806+XS34UrgGSsnv5qWudRiT3HA0ZR38A
VPzgRXs+kIwVAhPe2AlXW0K8w/ipaEF41qAMvHUdXopi0h4Tgsc2QEijC0sIQmBf
kSM6gvzYq+gFOJifxcEt3EJj6hOQ4U7nEOi/PqtjBl3BsTw6IdUWLdakCVzQq1I=
=eUF2
-----END PGP SIGNATURE-----