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@xxxxxxx>
> > >
> > > arm64 architecture handles correctly 64bit DMAs and can enable support
> > > for 64bit EHCI host controllers.
> > >
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
> > > Signed-off-by: Ryan Harkin <ryan.harkin@xxxxxxxxxx>
> > > Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> >
> > 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
> > > + */
> > > if (!dma_set_mask(hcd->self.controller, DMA_BIT_MASK(64)))
> > > ehci_info(ehci, "enabled 64bit DMA\n");
> > > +#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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
