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