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