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!