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