| Message ID | 1400099448-24222-1-git-send-email-broonie@kernel.org |
|---|---|
| State | New |
| Headers | show |
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. > 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. > #endif > } Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. At the moment it is the only known USB host controller enabled for arm64. And 64-bit DMA works fine on arm64. > > > 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. 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. Best regards, Liviu > > > #endif > > } > > Alan Stern > >
On Thu, May 15, 2014 at 04:17:44PM +0100, Liviu Dudau wrote: > On Thu, May 15, 2014 at 03:11:48PM +0100, Alan Stern wrote: > > On Wed, 14 May 2014, Mark Brown wrote: > > 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. At the moment it is the only known > USB host controller enabled for arm64. And 64-bit DMA works fine on arm64. We should probably conditionalise the device configuration on dma_set_mask() succeeding - any device that can't handle 64 bit DMA should be set up to constrain things appropriately.
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 > > > + */ > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 15 May 2014, Mark Brown wrote: > On Thu, May 15, 2014 at 04:17:44PM +0100, Liviu Dudau wrote: > > On Thu, May 15, 2014 at 03:11:48PM +0100, Alan Stern wrote: > > > On Wed, 14 May 2014, Mark Brown wrote: > > > > 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. At the moment it is the only known > > USB host controller enabled for arm64. And 64-bit DMA works fine on arm64. > > We should probably conditionalise the device configuration on > dma_set_mask() succeeding - any device that can't handle 64 bit DMA > should be set up to constrain things appropriately. Last I heard, there were EHCI controllers that claimed to handle 64-bit DMA but got it wrong. I don't know to what extent this may still be true. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 15, 2014 at 04:36:25PM +0100, Alan Stern wrote: > 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? It could if arm64 would restrict the DMA addresses to 32-bit, but it doesn't and I end up on my platform with USB DMA buffers allocated >4GB address. > > > 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? I don't have one, so I don't know for sure. I will try to find a PCI card that can do 32-bit and 64-bit DMA. > > > > > 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); Actually I need this line because my period_dma addresses have top 32-bit values non-zero. > > > 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. My platform creates all the USB buffers outside the 4GB zone. Need to go back to the code to understand if that is due to design or misconfiguration. Best regards, Liviu > > Alan Stern > >
On Thu, 15 May 2014, Liviu Dudau wrote: > On Thu, May 15, 2014 at 04:36:25PM +0100, Alan Stern wrote: > > 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? > > It could if arm64 would restrict the DMA addresses to 32-bit, but it doesn't > and I end up on my platform with USB DMA buffers allocated >4GB address. > > > > > > 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? > > I don't have one, so I don't know for sure. I will try to find a PCI card that > can do 32-bit and 64-bit DMA. What about a PCI card that can only do 32-bit DMA? > > 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); > > Actually I need this line because my period_dma addresses have top 32-bit > values non-zero. That seems like a bug. > > 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. > > My platform creates all the USB buffers outside the 4GB zone. Need to go back > to the code to understand if that is due to design or misconfiguration. If the platform doesn't guarantee that all those pools and buffers will lie in the same 4-GB region, it's a misconfiguration. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 15, 2014 at 05:53:53PM +0100, Liviu Dudau wrote: > On Thu, May 15, 2014 at 04:36:25PM +0100, Alan Stern wrote: > > 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: > > > > > arm64 architecture handles correctly 64bit DMAs and can enable support > > > > > for 64bit EHCI host controllers. > > > > > > > > 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? > > It could if arm64 would restrict the DMA addresses to 32-bit, but it doesn't > and I end up on my platform with USB DMA buffers allocated >4GB address. dma_alloc_coherent() on arm64 should return 32-bit addresses if the coherent_dma_mask is set to 32-bit. Which kernel version is this?
On Fri, May 16, 2014 at 01:55:01PM +0100, Catalin Marinas wrote: > On Thu, May 15, 2014 at 05:53:53PM +0100, Liviu Dudau wrote: > > On Thu, May 15, 2014 at 04:36:25PM +0100, Alan Stern wrote: > > > 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: > > > > > > arm64 architecture handles correctly 64bit DMAs and can enable support > > > > > > for 64bit EHCI host controllers. > > > > > > > > > > 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? > > > > It could if arm64 would restrict the DMA addresses to 32-bit, but it doesn't > > and I end up on my platform with USB DMA buffers allocated >4GB address. > > dma_alloc_coherent() on arm64 should return 32-bit addresses if the > coherent_dma_mask is set to 32-bit. Which kernel version is this? The initial patch was created before the removal of DMA32_ZONE from arm64. I need to revisit the latest mainline kernel on my board to determine if this patch is still needed. Best regards, Liviu > > -- > Catalin
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); #endif }