usb: ehci: Enable support for 64bit EHCI host controllers in arm64

Message ID 1400099448-24222-1-git-send-email-broonie@kernel.org
State New
Headers show

Commit Message

Mark Brown May 14, 2014, 8:30 p.m.
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>
---
 drivers/usb/host/ehci-hcd.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Alan Stern May 15, 2014, 2:11 p.m. | #1
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
Liviu Dudau May 15, 2014, 3:17 p.m. | #2
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
> 
>
Mark Brown May 15, 2014, 3:23 p.m. | #3
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.
Alan Stern May 15, 2014, 3:36 p.m. | #4
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
Alan Stern May 15, 2014, 3:38 p.m. | #5
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
Liviu Dudau May 15, 2014, 4:53 p.m. | #6
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
> 
>
Alan Stern May 15, 2014, 6:21 p.m. | #7
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
Catalin Marinas May 16, 2014, 12:55 p.m. | #8
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?
Liviu Dudau May 16, 2014, 1:21 p.m. | #9
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

Patch

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
 	}