On Thu, 15 May 2014, Liviu Dudau wrote:
On Thu, May 15, 2014 at 03:11:48PM +0100, Alan Stern wrote:
On Wed, 14 May 2014, Mark Brown wrote:
From: Liviu Dudau Liviu.Dudau@arm.com
arm64 architecture handles correctly 64bit DMAs and can enable support for 64bit EHCI host controllers.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com Signed-off-by: Ryan Harkin ryan.harkin@linaro.org Signed-off-by: Mark Brown broonie@linaro.org
Did you folks tested this for all sorts of host controllers? I have no way to verify that it works, and last I heard, many (or even most) controllers don't work right with 64-bit DMA.
I have tested it with a host controller that is capable of 64-bit DMA and without this change it doesn't work.
What do you mean it doesn't work? Can't the host controller use 32-bit DMA?
At the moment it is the only known USB host controller enabled for arm64. And 64-bit DMA works fine on arm64.
What do you mean? What happens if you plug in a PCI card containing, say, a Sony EHCI host controller on an arm64 system?
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 81cda09b47e3..e704d403beae 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -590,11 +590,17 @@ static int ehci_run (struct usb_hcd *hcd) */ hcc_params = ehci_readl(ehci, &ehci->caps->hcc_params); if (HCC_64BIT_ADDR(hcc_params)) {
ehci_writel(ehci, 0, &ehci->regs->segment);-#if 0 -// this is deeply broken on almost all architectures +#if CONFIG_ARM64
ehci_writel(ehci, ehci->periodic_dma >> 32,&ehci->regs->segment);/** this is deeply broken on almost all architectures* but arm64 can use it so enable it*/+#else
ehci_writel(ehci, 0, &ehci->regs->segment);It's silly to put this line in a separate #else section. The upper 32 bits of ehci->periodic_dma are bound to be 0 anyway, because it was allocated before the DMA mask was changed.
Well, I don't want to enable 64-bit DMA for *all* the platforms, so there needs to be an #else.
No, there doesn't. Just leave the
ehci_writel(ehci, 0, &ehci->regs->segment);
line above the "#if CONFIG_ARM64", the way it is in the original code, and get rid of
ehci_writel(ehci, ehci->periodic_dma >> 32, &ehci->regs->segment);
While it is true that ehci->periodic_dma variable has the top 32 bits zero, that cannot be guaranteed to be true for the physical register holding that value, so I guess the write is not superfluous.
That's why you can write 0 to the register instead of writing ehci->periodic_dma >> 32.
Don't forget, the controller uses that same ehci->regs->segment register for ehci->qtd_pool, ehci->qh_pool, ehci->itd_pool, and ehci->sitd_pool as well as ehci->periodic. If those DMA pools were allocated in different regions of memory (that is, regions whose upper 32 bits were different), the controller wouldn't be able to access them. The only way to insure they will all be allocated in the same 4-GB region is if they are allocated in the first 4 GB.
Alan Stern