-----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----- _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot