[RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The sa1111 platform is one of the two remaining users of the old Arm
specific "dmabounce" code, which is an earlier implementation of the
generic swiotlb.

Linus Walleij submitted a patch that removes dmabounce support from
the ixp4xx, and I had a look at the other user, which is the sa1111
companion chip.

Looking at how dmabounce is used, I could narrow it down to one driver
one one machine:

 - dmabounce is only initialized on assabet and pfs168, but not on
   any other sa1100 or pxa platform using sa1111.

 - pfs168 is not supported in mainline Linux.

 - only the OHCI and audio devices on sa1111 support DMA

 - There is no audio driver for this hardware

In the OHCI code, I noticed that two other platforms already have
a local bounce buffer support in the form of the "local_mem"
allocator. Specifically, TMIO and SM501 use this on a few other ARM
boards with 16KB or 128KB of local SRAM that can be accessed from the
OHCI and from the CPU.

While this is not the same problem as on sa1111, I could not find a
reason why we can't re-use the existing implementation but replace the
physical SRAM address mapping with a locally allocated DMA buffer.

There are two main downsides:

 - rather than using a dynamically sized pool, this buffer needs
   to be allocated at probe time using a fixed size. Without
   having any idea of what it should be, I picked a size of
   64KB, which is between what the other two OHCI front-ends use
   in their SRAM. If anyone has a better idea what that size
   is reasonable, this can be trivially changed.

 - Previously, only USB transfers to the second memory bank
   on Assabet needed to go through the bounce buffer, now all
   of them do, which may impact runtime performance, depending
   on what type of device is attached.

On the upside, the local_mem support uses write-combining
buffers, which should be a bit faster for transfers to the device
compared to normal uncached coherent memory as used in dmabounce.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I don't have this hardware, so the patch is not tested at all.
---
 arch/arm/common/Kconfig        |  1 -
 arch/arm/common/sa1111.c       | 64 ----------------------------------
 drivers/usb/core/hcd.c         | 17 +++++++--
 drivers/usb/host/ohci-sa1111.c | 16 +++++++++
 4 files changed, 30 insertions(+), 68 deletions(-)

diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig
index c8e198631d41..286ea014c015 100644
--- a/arch/arm/common/Kconfig
+++ b/arch/arm/common/Kconfig
@@ -1,7 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 config SA1111
 	bool
-	select DMABOUNCE if !ARCH_PXA
 
 config DMABOUNCE
 	bool
diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
index 7df003b149c6..a00915883f78 100644
--- a/arch/arm/common/sa1111.c
+++ b/arch/arm/common/sa1111.c
@@ -1391,70 +1391,9 @@ void sa1111_driver_unregister(struct sa1111_driver *driver)
 }
 EXPORT_SYMBOL(sa1111_driver_unregister);
 
-#ifdef CONFIG_DMABOUNCE
-/*
- * According to the "Intel StrongARM SA-1111 Microprocessor Companion
- * Chip Specification Update" (June 2000), erratum #7, there is a
- * significant bug in the SA1111 SDRAM shared memory controller.  If
- * an access to a region of memory above 1MB relative to the bank base,
- * it is important that address bit 10 _NOT_ be asserted. Depending
- * on the configuration of the RAM, bit 10 may correspond to one
- * of several different (processor-relative) address bits.
- *
- * This routine only identifies whether or not a given DMA address
- * is susceptible to the bug.
- *
- * This should only get called for sa1111_device types due to the
- * way we configure our device dma_masks.
- */
-static int sa1111_needs_bounce(struct device *dev, dma_addr_t addr, size_t size)
-{
-	/*
-	 * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
-	 * User's Guide" mentions that jumpers R51 and R52 control the
-	 * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
-	 * SDRAM bank 1 on Neponset). The default configuration selects
-	 * Assabet, so any address in bank 1 is necessarily invalid.
-	 */
-	return (machine_is_assabet() || machine_is_pfs168()) &&
-		(addr >= 0xc8000000 || (addr + size) >= 0xc8000000);
-}
-
-static int sa1111_notifier_call(struct notifier_block *n, unsigned long action,
-	void *data)
-{
-	struct sa1111_dev *dev = to_sa1111_device(data);
-
-	switch (action) {
-	case BUS_NOTIFY_ADD_DEVICE:
-		if (dev->dev.dma_mask && dev->dma_mask < 0xffffffffUL) {
-			int ret = dmabounce_register_dev(&dev->dev, 1024, 4096,
-					sa1111_needs_bounce);
-			if (ret)
-				dev_err(&dev->dev, "failed to register with dmabounce: %d\n", ret);
-		}
-		break;
-
-	case BUS_NOTIFY_DEL_DEVICE:
-		if (dev->dev.dma_mask && dev->dma_mask < 0xffffffffUL)
-			dmabounce_unregister_dev(&dev->dev);
-		break;
-	}
-	return NOTIFY_OK;
-}
-
-static struct notifier_block sa1111_bus_notifier = {
-	.notifier_call = sa1111_notifier_call,
-};
-#endif
-
 static int __init sa1111_init(void)
 {
 	int ret = bus_register(&sa1111_bus_type);
-#ifdef CONFIG_DMABOUNCE
-	if (ret == 0)
-		bus_register_notifier(&sa1111_bus_type, &sa1111_bus_notifier);
-#endif
 	if (ret == 0)
 		platform_driver_register(&sa1111_device_driver);
 	return ret;
@@ -1463,9 +1402,6 @@ static int __init sa1111_init(void)
 static void __exit sa1111_exit(void)
 {
 	platform_driver_unregister(&sa1111_device_driver);
-#ifdef CONFIG_DMABOUNCE
-	bus_unregister_notifier(&sa1111_bus_type, &sa1111_bus_notifier);
-#endif
 	bus_unregister(&sa1111_bus_type);
 }
 
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 3c7c64ff3c0a..5f2fa46c7958 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1260,7 +1260,8 @@ void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb)
 EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
 
 /*
- * Some usb host controllers can only perform dma using a small SRAM area.
+ * Some usb host controllers can only perform dma using a small SRAM area,
+ * or that have restrictions in addressable DRAM.
  * The usb core itself is however optimized for host controllers that can dma
  * using regular system memory - like pci devices doing bus mastering.
  *
@@ -3095,8 +3096,18 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
 	if (IS_ERR(hcd->localmem_pool))
 		return PTR_ERR(hcd->localmem_pool);
 
-	local_mem = devm_memremap(hcd->self.sysdev, phys_addr,
-				  size, MEMREMAP_WC);
+	/*
+	 * if a physical SRAM address was passed, map it, otherwise
+	 * allocate system memory as a buffer.
+	 */
+	if (phys_addr)
+		local_mem = devm_memremap(hcd->self.sysdev, phys_addr,
+					  size, MEMREMAP_WC);
+	else
+		local_mem = dmam_alloc_attrs(hcd->self.sysdev, size, &dma,
+					     GFP_KERNEL,
+					     DMA_ATTR_WRITE_COMBINE);
+
 	if (IS_ERR(local_mem))
 		return PTR_ERR(local_mem);
 
diff --git a/drivers/usb/host/ohci-sa1111.c b/drivers/usb/host/ohci-sa1111.c
index 137f66f6977f..488033f2e144 100644
--- a/drivers/usb/host/ohci-sa1111.c
+++ b/drivers/usb/host/ohci-sa1111.c
@@ -206,6 +206,22 @@ static int ohci_hcd_sa1111_probe(struct sa1111_dev *dev)
 		goto err1;
 	}
 
+	/*
+	 * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
+	 * User's Guide" mentions that jumpers R51 and R52 control the
+	 * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
+	 * SDRAM bank 1 on Neponset). The default configuration selects
+	 * Assabet, so any address in bank 1 is necessarily invalid.
+	 *
+	 * As a workaround, use a bounce buffer in addressable memory
+	 * as local_mem.
+	 */
+	if (machine_is_assabet()) {
+		ret = usb_hcd_setup_local_mem(hcd, 0, 0, SZ_64K);
+		if (ret)
+			goto out_err1;
+	}
+
 	if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {
 		dev_dbg(&dev->dev, "request_mem_region failed\n");
 		ret = -EBUSY;
-- 
2.29.2

[RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The sa1111 platform is one of the two remaining users of the old Arm
specific "dmabounce" code, which is an earlier implementation of the
generic swiotlb.

Linus Walleij submitted a patch that removes dmabounce support from
the ixp4xx, and I had a look at the other user, which is the sa1111
companion chip.

Looking at how dmabounce is used, I could narrow it down to one driver
one one machine:

 - dmabounce is only initialized on assabet and pfs168, but not on
   any other sa1100 or pxa platform using sa1111.

 - pfs168 is not supported in mainline Linux.

 - only the OHCI and audio devices on sa1111 support DMA

 - There is no audio driver for this hardware

In the OHCI code, I noticed that two other platforms already have
a local bounce buffer support in the form of the "local_mem"
allocator. Specifically, TMIO and SM501 use this on a few other ARM
boards with 16KB or 128KB of local SRAM that can be accessed from the
OHCI and from the CPU.

While this is not the same problem as on sa1111, I could not find a
reason why we can't re-use the existing implementation but replace the
physical SRAM address mapping with a locally allocated DMA buffer.

There are two main downsides:

 - rather than using a dynamically sized pool, this buffer needs
   to be allocated at probe time using a fixed size. Without
   having any idea of what it should be, I picked a size of
   64KB, which is between what the other two OHCI front-ends use
   in their SRAM. If anyone has a better idea what that size
   is reasonable, this can be trivially changed.

 - Previously, only USB transfers to the second memory bank
   on Assabet needed to go through the bounce buffer, now all
   of them do, which may impact runtime performance, depending
   on what type of device is attached.

On the upside, the local_mem support uses write-combining
buffers, which should be a bit faster for transfers to the device
compared to normal uncached coherent memory as used in dmabounce.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I don't have this hardware, so the patch is not tested at all.
---
 arch/arm/common/Kconfig        |  1 -
 arch/arm/common/sa1111.c       | 64 ----------------------------------
 drivers/usb/core/hcd.c         | 17 +++++++--
 drivers/usb/host/ohci-sa1111.c | 16 +++++++++
 4 files changed, 30 insertions(+), 68 deletions(-)

diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig
index c8e198631d41..286ea014c015 100644
--- a/arch/arm/common/Kconfig
+++ b/arch/arm/common/Kconfig
@@ -1,7 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 config SA1111
 	bool
-	select DMABOUNCE if !ARCH_PXA
 
 config DMABOUNCE
 	bool
diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
index 7df003b149c6..a00915883f78 100644
--- a/arch/arm/common/sa1111.c
+++ b/arch/arm/common/sa1111.c
@@ -1391,70 +1391,9 @@ void sa1111_driver_unregister(struct sa1111_driver *driver)
 }
 EXPORT_SYMBOL(sa1111_driver_unregister);
 
-#ifdef CONFIG_DMABOUNCE
-/*
- * According to the "Intel StrongARM SA-1111 Microprocessor Companion
- * Chip Specification Update" (June 2000), erratum #7, there is a
- * significant bug in the SA1111 SDRAM shared memory controller.  If
- * an access to a region of memory above 1MB relative to the bank base,
- * it is important that address bit 10 _NOT_ be asserted. Depending
- * on the configuration of the RAM, bit 10 may correspond to one
- * of several different (processor-relative) address bits.
- *
- * This routine only identifies whether or not a given DMA address
- * is susceptible to the bug.
- *
- * This should only get called for sa1111_device types due to the
- * way we configure our device dma_masks.
- */
-static int sa1111_needs_bounce(struct device *dev, dma_addr_t addr, size_t size)
-{
-	/*
-	 * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
-	 * User's Guide" mentions that jumpers R51 and R52 control the
-	 * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
-	 * SDRAM bank 1 on Neponset). The default configuration selects
-	 * Assabet, so any address in bank 1 is necessarily invalid.
-	 */
-	return (machine_is_assabet() || machine_is_pfs168()) &&
-		(addr >= 0xc8000000 || (addr + size) >= 0xc8000000);
-}
-
-static int sa1111_notifier_call(struct notifier_block *n, unsigned long action,
-	void *data)
-{
-	struct sa1111_dev *dev = to_sa1111_device(data);
-
-	switch (action) {
-	case BUS_NOTIFY_ADD_DEVICE:
-		if (dev->dev.dma_mask && dev->dma_mask < 0xffffffffUL) {
-			int ret = dmabounce_register_dev(&dev->dev, 1024, 4096,
-					sa1111_needs_bounce);
-			if (ret)
-				dev_err(&dev->dev, "failed to register with dmabounce: %d\n", ret);
-		}
-		break;
-
-	case BUS_NOTIFY_DEL_DEVICE:
-		if (dev->dev.dma_mask && dev->dma_mask < 0xffffffffUL)
-			dmabounce_unregister_dev(&dev->dev);
-		break;
-	}
-	return NOTIFY_OK;
-}
-
-static struct notifier_block sa1111_bus_notifier = {
-	.notifier_call = sa1111_notifier_call,
-};
-#endif
-
 static int __init sa1111_init(void)
 {
 	int ret = bus_register(&sa1111_bus_type);
-#ifdef CONFIG_DMABOUNCE
-	if (ret == 0)
-		bus_register_notifier(&sa1111_bus_type, &sa1111_bus_notifier);
-#endif
 	if (ret == 0)
 		platform_driver_register(&sa1111_device_driver);
 	return ret;
@@ -1463,9 +1402,6 @@ static int __init sa1111_init(void)
 static void __exit sa1111_exit(void)
 {
 	platform_driver_unregister(&sa1111_device_driver);
-#ifdef CONFIG_DMABOUNCE
-	bus_unregister_notifier(&sa1111_bus_type, &sa1111_bus_notifier);
-#endif
 	bus_unregister(&sa1111_bus_type);
 }
 
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 3c7c64ff3c0a..5f2fa46c7958 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1260,7 +1260,8 @@ void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb)
 EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
 
 /*
- * Some usb host controllers can only perform dma using a small SRAM area.
+ * Some usb host controllers can only perform dma using a small SRAM area,
+ * or that have restrictions in addressable DRAM.
  * The usb core itself is however optimized for host controllers that can dma
  * using regular system memory - like pci devices doing bus mastering.
  *
@@ -3095,8 +3096,18 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
 	if (IS_ERR(hcd->localmem_pool))
 		return PTR_ERR(hcd->localmem_pool);
 
-	local_mem = devm_memremap(hcd->self.sysdev, phys_addr,
-				  size, MEMREMAP_WC);
+	/*
+	 * if a physical SRAM address was passed, map it, otherwise
+	 * allocate system memory as a buffer.
+	 */
+	if (phys_addr)
+		local_mem = devm_memremap(hcd->self.sysdev, phys_addr,
+					  size, MEMREMAP_WC);
+	else
+		local_mem = dmam_alloc_attrs(hcd->self.sysdev, size, &dma,
+					     GFP_KERNEL,
+					     DMA_ATTR_WRITE_COMBINE);
+
 	if (IS_ERR(local_mem))
 		return PTR_ERR(local_mem);
 
diff --git a/drivers/usb/host/ohci-sa1111.c b/drivers/usb/host/ohci-sa1111.c
index 137f66f6977f..488033f2e144 100644
--- a/drivers/usb/host/ohci-sa1111.c
+++ b/drivers/usb/host/ohci-sa1111.c
@@ -206,6 +206,22 @@ static int ohci_hcd_sa1111_probe(struct sa1111_dev *dev)
 		goto err1;
 	}
 
+	/*
+	 * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
+	 * User's Guide" mentions that jumpers R51 and R52 control the
+	 * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
+	 * SDRAM bank 1 on Neponset). The default configuration selects
+	 * Assabet, so any address in bank 1 is necessarily invalid.
+	 *
+	 * As a workaround, use a bounce buffer in addressable memory
+	 * as local_mem.
+	 */
+	if (machine_is_assabet()) {
+		ret = usb_hcd_setup_local_mem(hcd, 0, 0, SZ_64K);
+		if (ret)
+			goto out_err1;
+	}
+
 	if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {
 		dev_dbg(&dev->dev, "request_mem_region failed\n");
 		ret = -EBUSY;
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Alan Stern]

On Tue, Feb 01, 2022 at 04:02:48PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The sa1111 platform is one of the two remaining users of the old Arm
> specific "dmabounce" code, which is an earlier implementation of the
> generic swiotlb.
> 
> Linus Walleij submitted a patch that removes dmabounce support from
> the ixp4xx, and I had a look at the other user, which is the sa1111
> companion chip.
> 
> Looking at how dmabounce is used, I could narrow it down to one driver
> one one machine:
> 
>  - dmabounce is only initialized on assabet and pfs168, but not on
>    any other sa1100 or pxa platform using sa1111.
> 
>  - pfs168 is not supported in mainline Linux.
> 
>  - only the OHCI and audio devices on sa1111 support DMA
> 
>  - There is no audio driver for this hardware
> 
> In the OHCI code, I noticed that two other platforms already have
> a local bounce buffer support in the form of the "local_mem"
> allocator. Specifically, TMIO and SM501 use this on a few other ARM
> boards with 16KB or 128KB of local SRAM that can be accessed from the
> OHCI and from the CPU.
> 
> While this is not the same problem as on sa1111, I could not find a
> reason why we can't re-use the existing implementation but replace the
> physical SRAM address mapping with a locally allocated DMA buffer.
> 
> There are two main downsides:
> 
>  - rather than using a dynamically sized pool, this buffer needs
>    to be allocated at probe time using a fixed size. Without
>    having any idea of what it should be, I picked a size of
>    64KB, which is between what the other two OHCI front-ends use
>    in their SRAM. If anyone has a better idea what that size
>    is reasonable, this can be trivially changed.
> 
>  - Previously, only USB transfers to the second memory bank
>    on Assabet needed to go through the bounce buffer, now all
>    of them do, which may impact runtime performance, depending
>    on what type of device is attached.
> 
> On the upside, the local_mem support uses write-combining
> buffers, which should be a bit faster for transfers to the device
> compared to normal uncached coherent memory as used in dmabounce.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I don't have this hardware, so the patch is not tested at all.


> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 3c7c64ff3c0a..5f2fa46c7958 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1260,7 +1260,8 @@ void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb)
>  EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
>  
>  /*
> - * Some usb host controllers can only perform dma using a small SRAM area.
> + * Some usb host controllers can only perform dma using a small SRAM area,
> + * or that have restrictions in addressable DRAM.

s/that //
s/in/on/

Otherwise the USB parts of this look okay to me.  I don't have suitable 
hardware to test either.  (I wonder if anyone is still using this 
platform...)

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Alan Stern]

On Tue, Feb 01, 2022 at 04:02:48PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The sa1111 platform is one of the two remaining users of the old Arm
> specific "dmabounce" code, which is an earlier implementation of the
> generic swiotlb.
> 
> Linus Walleij submitted a patch that removes dmabounce support from
> the ixp4xx, and I had a look at the other user, which is the sa1111
> companion chip.
> 
> Looking at how dmabounce is used, I could narrow it down to one driver
> one one machine:
> 
>  - dmabounce is only initialized on assabet and pfs168, but not on
>    any other sa1100 or pxa platform using sa1111.
> 
>  - pfs168 is not supported in mainline Linux.
> 
>  - only the OHCI and audio devices on sa1111 support DMA
> 
>  - There is no audio driver for this hardware
> 
> In the OHCI code, I noticed that two other platforms already have
> a local bounce buffer support in the form of the "local_mem"
> allocator. Specifically, TMIO and SM501 use this on a few other ARM
> boards with 16KB or 128KB of local SRAM that can be accessed from the
> OHCI and from the CPU.
> 
> While this is not the same problem as on sa1111, I could not find a
> reason why we can't re-use the existing implementation but replace the
> physical SRAM address mapping with a locally allocated DMA buffer.
> 
> There are two main downsides:
> 
>  - rather than using a dynamically sized pool, this buffer needs
>    to be allocated at probe time using a fixed size. Without
>    having any idea of what it should be, I picked a size of
>    64KB, which is between what the other two OHCI front-ends use
>    in their SRAM. If anyone has a better idea what that size
>    is reasonable, this can be trivially changed.
> 
>  - Previously, only USB transfers to the second memory bank
>    on Assabet needed to go through the bounce buffer, now all
>    of them do, which may impact runtime performance, depending
>    on what type of device is attached.
> 
> On the upside, the local_mem support uses write-combining
> buffers, which should be a bit faster for transfers to the device
> compared to normal uncached coherent memory as used in dmabounce.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I don't have this hardware, so the patch is not tested at all.


> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 3c7c64ff3c0a..5f2fa46c7958 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1260,7 +1260,8 @@ void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb)
>  EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
>  
>  /*
> - * Some usb host controllers can only perform dma using a small SRAM area.
> + * Some usb host controllers can only perform dma using a small SRAM area,
> + * or that have restrictions in addressable DRAM.

s/that //
s/in/on/

Otherwise the USB parts of this look okay to me.  I don't have suitable 
hardware to test either.  (I wonder if anyone is still using this 
platform...)

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Arnd Bergmann]

On Tue, Feb 1, 2022 at 4:31 PM Alan Stern <stern@rowland.harvard.edu> wrote:

> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 3c7c64ff3c0a..5f2fa46c7958 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -1260,7 +1260,8 @@ void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb)
> >  EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
> >
> >  /*
> > - * Some usb host controllers can only perform dma using a small SRAM area.
> > + * Some usb host controllers can only perform dma using a small SRAM area,
> > + * or that have restrictions in addressable DRAM.
>
> s/that //
> s/in/on/

Fixed now.

> Otherwise the USB parts of this look okay to me.  I don't have suitable
> hardware to test either.  (I wonder if anyone is still using this
> platform...)

I assumed Russell was still using the Assabet, but his last upstream
commits for sa1100 are from 2016 (merged in 2019), so that may have
changed in the meantime.

> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Thanks!

       Arnd

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Arnd Bergmann]

On Tue, Feb 1, 2022 at 4:31 PM Alan Stern <stern@rowland.harvard.edu> wrote:

> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 3c7c64ff3c0a..5f2fa46c7958 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -1260,7 +1260,8 @@ void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb)
> >  EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
> >
> >  /*
> > - * Some usb host controllers can only perform dma using a small SRAM area.
> > + * Some usb host controllers can only perform dma using a small SRAM area,
> > + * or that have restrictions in addressable DRAM.
>
> s/that //
> s/in/on/

Fixed now.

> Otherwise the USB parts of this look okay to me.  I don't have suitable
> hardware to test either.  (I wonder if anyone is still using this
> platform...)

I assumed Russell was still using the Assabet, but his last upstream
commits for sa1100 are from 2016 (merged in 2019), so that may have
changed in the meantime.

> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Thanks!

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Robin Murphy]

On 2022-02-01 15:02, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The sa1111 platform is one of the two remaining users of the old Arm
> specific "dmabounce" code, which is an earlier implementation of the
> generic swiotlb.
> 
> Linus Walleij submitted a patch that removes dmabounce support from
> the ixp4xx, and I had a look at the other user, which is the sa1111
> companion chip.
> 
> Looking at how dmabounce is used, I could narrow it down to one driver
> one one machine:
> 
>   - dmabounce is only initialized on assabet and pfs168, but not on
>     any other sa1100 or pxa platform using sa1111.

Hmm, my reading of it was different. AFAICS it should affect all 
platforms with CONFIG_ARCH_SA1100 + CONFIG_SA1111 - the bus notifier 
from sa1111_init() will initialise dmabounce for everything where 
sa1111_configure_smc() has punched a hole in the DMA mask to handle the 
addressing erratum. sa1111_needs_bounce() looks to be a further 
consideration for platforms where DMA *additionally* cannot target an 
entire bank of memory at all.

>   - pfs168 is not supported in mainline Linux.
> 
>   - only the OHCI and audio devices on sa1111 support DMA
> 
>   - There is no audio driver for this hardware
> 
> In the OHCI code, I noticed that two other platforms already have
> a local bounce buffer support in the form of the "local_mem"
> allocator. Specifically, TMIO and SM501 use this on a few other ARM
> boards with 16KB or 128KB of local SRAM that can be accessed from the
> OHCI and from the CPU.
> 
> While this is not the same problem as on sa1111, I could not find a
> reason why we can't re-use the existing implementation but replace the
> physical SRAM address mapping with a locally allocated DMA buffer.
> 
> There are two main downsides:
> 
>   - rather than using a dynamically sized pool, this buffer needs
>     to be allocated at probe time using a fixed size. Without
>     having any idea of what it should be, I picked a size of
>     64KB, which is between what the other two OHCI front-ends use
>     in their SRAM. If anyone has a better idea what that size
>     is reasonable, this can be trivially changed.
> 
>   - Previously, only USB transfers to the second memory bank
>     on Assabet needed to go through the bounce buffer, now all
>     of them do, which may impact runtime performance, depending
>     on what type of device is attached.

As above, bouncing should also happen for every other 1/8/16/32MB 
(depending on configuration) of memory, see the "Figure out if we need 
to bounce from the DMA mask" case in needs_bounce().

Thanks,
Robin.

> On the upside, the local_mem support uses write-combining
> buffers, which should be a bit faster for transfers to the device
> compared to normal uncached coherent memory as used in dmabounce.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I don't have this hardware, so the patch is not tested at all.
> ---
>   arch/arm/common/Kconfig        |  1 -
>   arch/arm/common/sa1111.c       | 64 ----------------------------------
>   drivers/usb/core/hcd.c         | 17 +++++++--
>   drivers/usb/host/ohci-sa1111.c | 16 +++++++++
>   4 files changed, 30 insertions(+), 68 deletions(-)
> 
> diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig
> index c8e198631d41..286ea014c015 100644
> --- a/arch/arm/common/Kconfig
> +++ b/arch/arm/common/Kconfig
> @@ -1,7 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0
>   config SA1111
>   	bool
> -	select DMABOUNCE if !ARCH_PXA
>   
>   config DMABOUNCE
>   	bool
> diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
> index 7df003b149c6..a00915883f78 100644
> --- a/arch/arm/common/sa1111.c
> +++ b/arch/arm/common/sa1111.c
> @@ -1391,70 +1391,9 @@ void sa1111_driver_unregister(struct sa1111_driver *driver)
>   }
>   EXPORT_SYMBOL(sa1111_driver_unregister);
>   
> -#ifdef CONFIG_DMABOUNCE
> -/*
> - * According to the "Intel StrongARM SA-1111 Microprocessor Companion
> - * Chip Specification Update" (June 2000), erratum #7, there is a
> - * significant bug in the SA1111 SDRAM shared memory controller.  If
> - * an access to a region of memory above 1MB relative to the bank base,
> - * it is important that address bit 10 _NOT_ be asserted. Depending
> - * on the configuration of the RAM, bit 10 may correspond to one
> - * of several different (processor-relative) address bits.
> - *
> - * This routine only identifies whether or not a given DMA address
> - * is susceptible to the bug.
> - *
> - * This should only get called for sa1111_device types due to the
> - * way we configure our device dma_masks.
> - */
> -static int sa1111_needs_bounce(struct device *dev, dma_addr_t addr, size_t size)
> -{
> -	/*
> -	 * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
> -	 * User's Guide" mentions that jumpers R51 and R52 control the
> -	 * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
> -	 * SDRAM bank 1 on Neponset). The default configuration selects
> -	 * Assabet, so any address in bank 1 is necessarily invalid.
> -	 */
> -	return (machine_is_assabet() || machine_is_pfs168()) &&
> -		(addr >= 0xc8000000 || (addr + size) >= 0xc8000000);
> -}
> -
> -static int sa1111_notifier_call(struct notifier_block *n, unsigned long action,
> -	void *data)
> -{
> -	struct sa1111_dev *dev = to_sa1111_device(data);
> -
> -	switch (action) {
> -	case BUS_NOTIFY_ADD_DEVICE:
> -		if (dev->dev.dma_mask && dev->dma_mask < 0xffffffffUL) {
> -			int ret = dmabounce_register_dev(&dev->dev, 1024, 4096,
> -					sa1111_needs_bounce);
> -			if (ret)
> -				dev_err(&dev->dev, "failed to register with dmabounce: %d\n", ret);
> -		}
> -		break;
> -
> -	case BUS_NOTIFY_DEL_DEVICE:
> -		if (dev->dev.dma_mask && dev->dma_mask < 0xffffffffUL)
> -			dmabounce_unregister_dev(&dev->dev);
> -		break;
> -	}
> -	return NOTIFY_OK;
> -}
> -
> -static struct notifier_block sa1111_bus_notifier = {
> -	.notifier_call = sa1111_notifier_call,
> -};
> -#endif
> -
>   static int __init sa1111_init(void)
>   {
>   	int ret = bus_register(&sa1111_bus_type);
> -#ifdef CONFIG_DMABOUNCE
> -	if (ret == 0)
> -		bus_register_notifier(&sa1111_bus_type, &sa1111_bus_notifier);
> -#endif
>   	if (ret == 0)
>   		platform_driver_register(&sa1111_device_driver);
>   	return ret;
> @@ -1463,9 +1402,6 @@ static int __init sa1111_init(void)
>   static void __exit sa1111_exit(void)
>   {
>   	platform_driver_unregister(&sa1111_device_driver);
> -#ifdef CONFIG_DMABOUNCE
> -	bus_unregister_notifier(&sa1111_bus_type, &sa1111_bus_notifier);
> -#endif
>   	bus_unregister(&sa1111_bus_type);
>   }
>   
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 3c7c64ff3c0a..5f2fa46c7958 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1260,7 +1260,8 @@ void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb)
>   EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
>   
>   /*
> - * Some usb host controllers can only perform dma using a small SRAM area.
> + * Some usb host controllers can only perform dma using a small SRAM area,
> + * or that have restrictions in addressable DRAM.
>    * The usb core itself is however optimized for host controllers that can dma
>    * using regular system memory - like pci devices doing bus mastering.
>    *
> @@ -3095,8 +3096,18 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
>   	if (IS_ERR(hcd->localmem_pool))
>   		return PTR_ERR(hcd->localmem_pool);
>   
> -	local_mem = devm_memremap(hcd->self.sysdev, phys_addr,
> -				  size, MEMREMAP_WC);
> +	/*
> +	 * if a physical SRAM address was passed, map it, otherwise
> +	 * allocate system memory as a buffer.
> +	 */
> +	if (phys_addr)
> +		local_mem = devm_memremap(hcd->self.sysdev, phys_addr,
> +					  size, MEMREMAP_WC);
> +	else
> +		local_mem = dmam_alloc_attrs(hcd->self.sysdev, size, &dma,
> +					     GFP_KERNEL,
> +					     DMA_ATTR_WRITE_COMBINE);
> +
>   	if (IS_ERR(local_mem))
>   		return PTR_ERR(local_mem);
>   
> diff --git a/drivers/usb/host/ohci-sa1111.c b/drivers/usb/host/ohci-sa1111.c
> index 137f66f6977f..488033f2e144 100644
> --- a/drivers/usb/host/ohci-sa1111.c
> +++ b/drivers/usb/host/ohci-sa1111.c
> @@ -206,6 +206,22 @@ static int ohci_hcd_sa1111_probe(struct sa1111_dev *dev)
>   		goto err1;
>   	}
>   
> +	/*
> +	 * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
> +	 * User's Guide" mentions that jumpers R51 and R52 control the
> +	 * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
> +	 * SDRAM bank 1 on Neponset). The default configuration selects
> +	 * Assabet, so any address in bank 1 is necessarily invalid.
> +	 *
> +	 * As a workaround, use a bounce buffer in addressable memory
> +	 * as local_mem.
> +	 */
> +	if (machine_is_assabet()) {
> +		ret = usb_hcd_setup_local_mem(hcd, 0, 0, SZ_64K);
> +		if (ret)
> +			goto out_err1;
> +	}
> +
>   	if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {
>   		dev_dbg(&dev->dev, "request_mem_region failed\n");
>   		ret = -EBUSY;

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Robin Murphy]

On 2022-02-01 15:02, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The sa1111 platform is one of the two remaining users of the old Arm
> specific "dmabounce" code, which is an earlier implementation of the
> generic swiotlb.
> 
> Linus Walleij submitted a patch that removes dmabounce support from
> the ixp4xx, and I had a look at the other user, which is the sa1111
> companion chip.
> 
> Looking at how dmabounce is used, I could narrow it down to one driver
> one one machine:
> 
>   - dmabounce is only initialized on assabet and pfs168, but not on
>     any other sa1100 or pxa platform using sa1111.

Hmm, my reading of it was different. AFAICS it should affect all 
platforms with CONFIG_ARCH_SA1100 + CONFIG_SA1111 - the bus notifier 
from sa1111_init() will initialise dmabounce for everything where 
sa1111_configure_smc() has punched a hole in the DMA mask to handle the 
addressing erratum. sa1111_needs_bounce() looks to be a further 
consideration for platforms where DMA *additionally* cannot target an 
entire bank of memory at all.

>   - pfs168 is not supported in mainline Linux.
> 
>   - only the OHCI and audio devices on sa1111 support DMA
> 
>   - There is no audio driver for this hardware
> 
> In the OHCI code, I noticed that two other platforms already have
> a local bounce buffer support in the form of the "local_mem"
> allocator. Specifically, TMIO and SM501 use this on a few other ARM
> boards with 16KB or 128KB of local SRAM that can be accessed from the
> OHCI and from the CPU.
> 
> While this is not the same problem as on sa1111, I could not find a
> reason why we can't re-use the existing implementation but replace the
> physical SRAM address mapping with a locally allocated DMA buffer.
> 
> There are two main downsides:
> 
>   - rather than using a dynamically sized pool, this buffer needs
>     to be allocated at probe time using a fixed size. Without
>     having any idea of what it should be, I picked a size of
>     64KB, which is between what the other two OHCI front-ends use
>     in their SRAM. If anyone has a better idea what that size
>     is reasonable, this can be trivially changed.
> 
>   - Previously, only USB transfers to the second memory bank
>     on Assabet needed to go through the bounce buffer, now all
>     of them do, which may impact runtime performance, depending
>     on what type of device is attached.

As above, bouncing should also happen for every other 1/8/16/32MB 
(depending on configuration) of memory, see the "Figure out if we need 
to bounce from the DMA mask" case in needs_bounce().

Thanks,
Robin.

> On the upside, the local_mem support uses write-combining
> buffers, which should be a bit faster for transfers to the device
> compared to normal uncached coherent memory as used in dmabounce.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I don't have this hardware, so the patch is not tested at all.
> ---
>   arch/arm/common/Kconfig        |  1 -
>   arch/arm/common/sa1111.c       | 64 ----------------------------------
>   drivers/usb/core/hcd.c         | 17 +++++++--
>   drivers/usb/host/ohci-sa1111.c | 16 +++++++++
>   4 files changed, 30 insertions(+), 68 deletions(-)
> 
> diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig
> index c8e198631d41..286ea014c015 100644
> --- a/arch/arm/common/Kconfig
> +++ b/arch/arm/common/Kconfig
> @@ -1,7 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0
>   config SA1111
>   	bool
> -	select DMABOUNCE if !ARCH_PXA
>   
>   config DMABOUNCE
>   	bool
> diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
> index 7df003b149c6..a00915883f78 100644
> --- a/arch/arm/common/sa1111.c
> +++ b/arch/arm/common/sa1111.c
> @@ -1391,70 +1391,9 @@ void sa1111_driver_unregister(struct sa1111_driver *driver)
>   }
>   EXPORT_SYMBOL(sa1111_driver_unregister);
>   
> -#ifdef CONFIG_DMABOUNCE
> -/*
> - * According to the "Intel StrongARM SA-1111 Microprocessor Companion
> - * Chip Specification Update" (June 2000), erratum #7, there is a
> - * significant bug in the SA1111 SDRAM shared memory controller.  If
> - * an access to a region of memory above 1MB relative to the bank base,
> - * it is important that address bit 10 _NOT_ be asserted. Depending
> - * on the configuration of the RAM, bit 10 may correspond to one
> - * of several different (processor-relative) address bits.
> - *
> - * This routine only identifies whether or not a given DMA address
> - * is susceptible to the bug.
> - *
> - * This should only get called for sa1111_device types due to the
> - * way we configure our device dma_masks.
> - */
> -static int sa1111_needs_bounce(struct device *dev, dma_addr_t addr, size_t size)
> -{
> -	/*
> -	 * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
> -	 * User's Guide" mentions that jumpers R51 and R52 control the
> -	 * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
> -	 * SDRAM bank 1 on Neponset). The default configuration selects
> -	 * Assabet, so any address in bank 1 is necessarily invalid.
> -	 */
> -	return (machine_is_assabet() || machine_is_pfs168()) &&
> -		(addr >= 0xc8000000 || (addr + size) >= 0xc8000000);
> -}
> -
> -static int sa1111_notifier_call(struct notifier_block *n, unsigned long action,
> -	void *data)
> -{
> -	struct sa1111_dev *dev = to_sa1111_device(data);
> -
> -	switch (action) {
> -	case BUS_NOTIFY_ADD_DEVICE:
> -		if (dev->dev.dma_mask && dev->dma_mask < 0xffffffffUL) {
> -			int ret = dmabounce_register_dev(&dev->dev, 1024, 4096,
> -					sa1111_needs_bounce);
> -			if (ret)
> -				dev_err(&dev->dev, "failed to register with dmabounce: %d\n", ret);
> -		}
> -		break;
> -
> -	case BUS_NOTIFY_DEL_DEVICE:
> -		if (dev->dev.dma_mask && dev->dma_mask < 0xffffffffUL)
> -			dmabounce_unregister_dev(&dev->dev);
> -		break;
> -	}
> -	return NOTIFY_OK;
> -}
> -
> -static struct notifier_block sa1111_bus_notifier = {
> -	.notifier_call = sa1111_notifier_call,
> -};
> -#endif
> -
>   static int __init sa1111_init(void)
>   {
>   	int ret = bus_register(&sa1111_bus_type);
> -#ifdef CONFIG_DMABOUNCE
> -	if (ret == 0)
> -		bus_register_notifier(&sa1111_bus_type, &sa1111_bus_notifier);
> -#endif
>   	if (ret == 0)
>   		platform_driver_register(&sa1111_device_driver);
>   	return ret;
> @@ -1463,9 +1402,6 @@ static int __init sa1111_init(void)
>   static void __exit sa1111_exit(void)
>   {
>   	platform_driver_unregister(&sa1111_device_driver);
> -#ifdef CONFIG_DMABOUNCE
> -	bus_unregister_notifier(&sa1111_bus_type, &sa1111_bus_notifier);
> -#endif
>   	bus_unregister(&sa1111_bus_type);
>   }
>   
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 3c7c64ff3c0a..5f2fa46c7958 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1260,7 +1260,8 @@ void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb)
>   EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
>   
>   /*
> - * Some usb host controllers can only perform dma using a small SRAM area.
> + * Some usb host controllers can only perform dma using a small SRAM area,
> + * or that have restrictions in addressable DRAM.
>    * The usb core itself is however optimized for host controllers that can dma
>    * using regular system memory - like pci devices doing bus mastering.
>    *
> @@ -3095,8 +3096,18 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
>   	if (IS_ERR(hcd->localmem_pool))
>   		return PTR_ERR(hcd->localmem_pool);
>   
> -	local_mem = devm_memremap(hcd->self.sysdev, phys_addr,
> -				  size, MEMREMAP_WC);
> +	/*
> +	 * if a physical SRAM address was passed, map it, otherwise
> +	 * allocate system memory as a buffer.
> +	 */
> +	if (phys_addr)
> +		local_mem = devm_memremap(hcd->self.sysdev, phys_addr,
> +					  size, MEMREMAP_WC);
> +	else
> +		local_mem = dmam_alloc_attrs(hcd->self.sysdev, size, &dma,
> +					     GFP_KERNEL,
> +					     DMA_ATTR_WRITE_COMBINE);
> +
>   	if (IS_ERR(local_mem))
>   		return PTR_ERR(local_mem);
>   
> diff --git a/drivers/usb/host/ohci-sa1111.c b/drivers/usb/host/ohci-sa1111.c
> index 137f66f6977f..488033f2e144 100644
> --- a/drivers/usb/host/ohci-sa1111.c
> +++ b/drivers/usb/host/ohci-sa1111.c
> @@ -206,6 +206,22 @@ static int ohci_hcd_sa1111_probe(struct sa1111_dev *dev)
>   		goto err1;
>   	}
>   
> +	/*
> +	 * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
> +	 * User's Guide" mentions that jumpers R51 and R52 control the
> +	 * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
> +	 * SDRAM bank 1 on Neponset). The default configuration selects
> +	 * Assabet, so any address in bank 1 is necessarily invalid.
> +	 *
> +	 * As a workaround, use a bounce buffer in addressable memory
> +	 * as local_mem.
> +	 */
> +	if (machine_is_assabet()) {
> +		ret = usb_hcd_setup_local_mem(hcd, 0, 0, SZ_64K);
> +		if (ret)
> +			goto out_err1;
> +	}
> +
>   	if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {
>   		dev_dbg(&dev->dev, "request_mem_region failed\n");
>   		ret = -EBUSY;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Russell King (Oracle)]

On Tue, Feb 01, 2022 at 05:10:38PM +0000, Robin Murphy wrote:
> On 2022-02-01 15:02, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > The sa1111 platform is one of the two remaining users of the old Arm
> > specific "dmabounce" code, which is an earlier implementation of the
> > generic swiotlb.
> > 
> > Linus Walleij submitted a patch that removes dmabounce support from
> > the ixp4xx, and I had a look at the other user, which is the sa1111
> > companion chip.
> > 
> > Looking at how dmabounce is used, I could narrow it down to one driver
> > one one machine:
> > 
> >   - dmabounce is only initialized on assabet and pfs168, but not on
> >     any other sa1100 or pxa platform using sa1111.
> 
> Hmm, my reading of it was different. AFAICS it should affect all platforms
> with CONFIG_ARCH_SA1100 + CONFIG_SA1111 - the bus notifier from
> sa1111_init() will initialise dmabounce for everything where
> sa1111_configure_smc() has punched a hole in the DMA mask to handle the
> addressing erratum. sa1111_needs_bounce() looks to be a further
> consideration for platforms where DMA *additionally* cannot target an entire
> bank of memory at all.

Correct. The SA1111 companion can only access one SDRAM bank, whereas
the SA1110 SoC can address up to four SDRAM banks. On platforms where
there is only one bank of SDRAM, there is no issue. However, on the
Assabet there is one SDRAM bank, and on the Neponset daughter board
with the SA1111, there is a second bank. As explained in the commentry,
the SA1111 can be hardware-configured via resistive jumpers to access
either bank, but we only support the factory-shipped configuration,
which is bank 0 (the lowest addressable bank.)

The SA1111 also has an issue that one of its address lines doesn't
behave correctly, and depending on the SDRAM columns/rows, this
punches multiple holes in the SDRAM address space it can access,
which is what the sa1111_dma_mask[] array is about, and we end up
with every alternate megabyte of physical address space being
inaccessible.

The DMA mask, along with the logic in dmabounce (which truely uses the
DMA mask as, erm, a *mask* rather than the misnamed *limit* that it
has been) know about both of these issues.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Russell King (Oracle)]

On Tue, Feb 01, 2022 at 05:10:38PM +0000, Robin Murphy wrote:
> On 2022-02-01 15:02, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > The sa1111 platform is one of the two remaining users of the old Arm
> > specific "dmabounce" code, which is an earlier implementation of the
> > generic swiotlb.
> > 
> > Linus Walleij submitted a patch that removes dmabounce support from
> > the ixp4xx, and I had a look at the other user, which is the sa1111
> > companion chip.
> > 
> > Looking at how dmabounce is used, I could narrow it down to one driver
> > one one machine:
> > 
> >   - dmabounce is only initialized on assabet and pfs168, but not on
> >     any other sa1100 or pxa platform using sa1111.
> 
> Hmm, my reading of it was different. AFAICS it should affect all platforms
> with CONFIG_ARCH_SA1100 + CONFIG_SA1111 - the bus notifier from
> sa1111_init() will initialise dmabounce for everything where
> sa1111_configure_smc() has punched a hole in the DMA mask to handle the
> addressing erratum. sa1111_needs_bounce() looks to be a further
> consideration for platforms where DMA *additionally* cannot target an entire
> bank of memory at all.

Correct. The SA1111 companion can only access one SDRAM bank, whereas
the SA1110 SoC can address up to four SDRAM banks. On platforms where
there is only one bank of SDRAM, there is no issue. However, on the
Assabet there is one SDRAM bank, and on the Neponset daughter board
with the SA1111, there is a second bank. As explained in the commentry,
the SA1111 can be hardware-configured via resistive jumpers to access
either bank, but we only support the factory-shipped configuration,
which is bank 0 (the lowest addressable bank.)

The SA1111 also has an issue that one of its address lines doesn't
behave correctly, and depending on the SDRAM columns/rows, this
punches multiple holes in the SDRAM address space it can access,
which is what the sa1111_dma_mask[] array is about, and we end up
with every alternate megabyte of physical address space being
inaccessible.

The DMA mask, along with the logic in dmabounce (which truely uses the
DMA mask as, erm, a *mask* rather than the misnamed *limit* that it
has been) know about both of these issues.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Arnd Bergmann]

On Tue, Feb 1, 2022 at 6:48 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
> On Tue, Feb 01, 2022 at 05:10:38PM +0000, Robin Murphy wrote:
> >
> > Hmm, my reading of it was different. AFAICS it should affect all platforms
> > with CONFIG_ARCH_SA1100 + CONFIG_SA1111 - the bus notifier from
> > sa1111_init() will initialise dmabounce for everything where
> > sa1111_configure_smc() has punched a hole in the DMA mask to handle the
> > addressing erratum. sa1111_needs_bounce() looks to be a further
> > consideration for platforms where DMA *additionally* cannot target an entire
> > bank of memory at all.
>
> Correct. The SA1111 companion can only access one SDRAM bank, whereas
> the SA1110 SoC can address up to four SDRAM banks. On platforms where
> there is only one bank of SDRAM, there is no issue. However, on the
> Assabet there is one SDRAM bank, and on the Neponset daughter board
> with the SA1111, there is a second bank. As explained in the commentry,
> the SA1111 can be hardware-configured via resistive jumpers to access
> either bank, but we only support the factory-shipped configuration,
> which is bank 0 (the lowest addressable bank.)

Ok, so this is the part that I think my patch gets right.

> The SA1111 also has an issue that one of its address lines doesn't
> behave correctly, and depending on the SDRAM columns/rows, this
> punches multiple holes in the SDRAM address space it can access,
> which is what the sa1111_dma_mask[] array is about, and we end up
> with every alternate megabyte of physical address space being
> inaccessible.
>
> The DMA mask, along with the logic in dmabounce (which truely uses the
> DMA mask as, erm, a *mask* rather than the misnamed *limit* that it
> has been) know about both of these issues.

while this part would not work if dma_alloc_flags() ends up getting
memory that is not accessible. At the minimum I need to drop the
machine_is_assabet() check and always allocate a safe buffer to
back hcd->local_mem regardless of the machine.

After reading through the dmabounce code again, my interpretation
is that the safe buffer it uses for bounces ultimately relies on
dma_alloc_coherent() allocating physical pages using GFP_DMA,
which in turn is sized to 1MB on the machines that need it.

If I'm not missing something else, using dmam_alloc_flags() in the
local_mem code should work with the same address restrictions, so
I hope I only need to update the changelog text plus the trivial change
below.

         Arnd

@@ -207,6 +207,14 @@ static int ohci_hcd_sa1111_probe(struct sa1111_dev *dev)
        }

        /*
+        * According to the "Intel StrongARM SA-1111 Microprocessor Companion
+        * Chip Specification Update" (June 2000), erratum #7, there is a
+        * significant bug in the SA1111 SDRAM shared memory controller.  If
+        * an access to a region of memory above 1MB relative to the bank base,
+        * it is important that address bit 10 _NOT_ be asserted. Depending
+        * on the configuration of the RAM, bit 10 may correspond to one
+        * of several different (processor-relative) address bits.
+        *
         * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
         * User's Guide" mentions that jumpers R51 and R52 control the
         * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
@@ -214,13 +222,14 @@ static int ohci_hcd_sa1111_probe(struct sa1111_dev *dev)
         * Assabet, so any address in bank 1 is necessarily invalid.
         *
         * As a workaround, use a bounce buffer in addressable memory
-        * as local_mem.
+        * as local_mem, relying on ZONE_DMA to provide an area that
+        * fits within the above constraints.
+        *
+        * SZ_64K is an estimate for what size this might need.
         */
-       if (machine_is_assabet()) {
-               ret = usb_hcd_setup_local_mem(hcd, 0, 0, SZ_64K);
-               if (ret)
-                       goto out_err1;
-       }
+       ret = usb_hcd_setup_local_mem(hcd, 0, 0, SZ_64K);
+       if (ret)
+               goto out_err1;

        if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {
                dev_dbg(&dev->dev, "request_mem_region failed\n");

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Arnd Bergmann]

On Tue, Feb 1, 2022 at 6:48 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
> On Tue, Feb 01, 2022 at 05:10:38PM +0000, Robin Murphy wrote:
> >
> > Hmm, my reading of it was different. AFAICS it should affect all platforms
> > with CONFIG_ARCH_SA1100 + CONFIG_SA1111 - the bus notifier from
> > sa1111_init() will initialise dmabounce for everything where
> > sa1111_configure_smc() has punched a hole in the DMA mask to handle the
> > addressing erratum. sa1111_needs_bounce() looks to be a further
> > consideration for platforms where DMA *additionally* cannot target an entire
> > bank of memory at all.
>
> Correct. The SA1111 companion can only access one SDRAM bank, whereas
> the SA1110 SoC can address up to four SDRAM banks. On platforms where
> there is only one bank of SDRAM, there is no issue. However, on the
> Assabet there is one SDRAM bank, and on the Neponset daughter board
> with the SA1111, there is a second bank. As explained in the commentry,
> the SA1111 can be hardware-configured via resistive jumpers to access
> either bank, but we only support the factory-shipped configuration,
> which is bank 0 (the lowest addressable bank.)

Ok, so this is the part that I think my patch gets right.

> The SA1111 also has an issue that one of its address lines doesn't
> behave correctly, and depending on the SDRAM columns/rows, this
> punches multiple holes in the SDRAM address space it can access,
> which is what the sa1111_dma_mask[] array is about, and we end up
> with every alternate megabyte of physical address space being
> inaccessible.
>
> The DMA mask, along with the logic in dmabounce (which truely uses the
> DMA mask as, erm, a *mask* rather than the misnamed *limit* that it
> has been) know about both of these issues.

while this part would not work if dma_alloc_flags() ends up getting
memory that is not accessible. At the minimum I need to drop the
machine_is_assabet() check and always allocate a safe buffer to
back hcd->local_mem regardless of the machine.

After reading through the dmabounce code again, my interpretation
is that the safe buffer it uses for bounces ultimately relies on
dma_alloc_coherent() allocating physical pages using GFP_DMA,
which in turn is sized to 1MB on the machines that need it.

If I'm not missing something else, using dmam_alloc_flags() in the
local_mem code should work with the same address restrictions, so
I hope I only need to update the changelog text plus the trivial change
below.

         Arnd

@@ -207,6 +207,14 @@ static int ohci_hcd_sa1111_probe(struct sa1111_dev *dev)
        }

        /*
+        * According to the "Intel StrongARM SA-1111 Microprocessor Companion
+        * Chip Specification Update" (June 2000), erratum #7, there is a
+        * significant bug in the SA1111 SDRAM shared memory controller.  If
+        * an access to a region of memory above 1MB relative to the bank base,
+        * it is important that address bit 10 _NOT_ be asserted. Depending
+        * on the configuration of the RAM, bit 10 may correspond to one
+        * of several different (processor-relative) address bits.
+        *
         * Section 4.6 of the "Intel StrongARM SA-1111 Development Module
         * User's Guide" mentions that jumpers R51 and R52 control the
         * target of SA-1111 DMA (either SDRAM bank 0 on Assabet, or
@@ -214,13 +222,14 @@ static int ohci_hcd_sa1111_probe(struct sa1111_dev *dev)
         * Assabet, so any address in bank 1 is necessarily invalid.
         *
         * As a workaround, use a bounce buffer in addressable memory
-        * as local_mem.
+        * as local_mem, relying on ZONE_DMA to provide an area that
+        * fits within the above constraints.
+        *
+        * SZ_64K is an estimate for what size this might need.
         */
-       if (machine_is_assabet()) {
-               ret = usb_hcd_setup_local_mem(hcd, 0, 0, SZ_64K);
-               if (ret)
-                       goto out_err1;
-       }
+       ret = usb_hcd_setup_local_mem(hcd, 0, 0, SZ_64K);
+       if (ret)
+               goto out_err1;

        if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {
                dev_dbg(&dev->dev, "request_mem_region failed\n");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Arnd Bergmann]

On Wed, Feb 2, 2022 at 12:10 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> while this part would not work if dma_alloc_flags() ends up getting
> memory that is not accessible. At the minimum I need to drop the
> machine_is_assabet() check and always allocate a safe buffer to
> back hcd->local_mem regardless of the machine.

I double-checked this to ensure we don't have to check for sa1100
vs pxa instead, and I found that all three sa1100 platforms with sa1111
(assabet/neponset, badge4, jornada720) enable DMA, but the one
pxa machine (lubbock) has the DMA mask set to 0 and won't
be able to use OHCI anyway.

       Arnd

Re: [RFC] ARM: sa1100/assabet: move dmabounce hack to ohci driver

[Arnd Bergmann]

On Wed, Feb 2, 2022 at 12:10 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> while this part would not work if dma_alloc_flags() ends up getting
> memory that is not accessible. At the minimum I need to drop the
> machine_is_assabet() check and always allocate a safe buffer to
> back hcd->local_mem regardless of the machine.

I double-checked this to ensure we don't have to check for sa1100
vs pxa instead, and I found that all three sa1100 platforms with sa1111
(assabet/neponset, badge4, jornada720) enable DMA, but the one
pxa machine (lubbock) has the DMA mask set to 0 and won't
be able to use OHCI anyway.

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel