Re: [PATCH v6 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
Hi Andy, Sorry for the delay in response. I will do what you suggest in your email. I do have one response to one of your comments below. On Thu, Jul 2, 2020 at 4:43 AM Andy Shevchenko wrote: > > On Wed, Jul 01, 2020 at 05:21:38PM -0400, Jim Quinlan wrote: > > The new field 'dma_range_map' in struct device is used to facilitate the > > use of single or multiple offsets between mapping regions of cpu addrs and > > dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only > > capable of holding a single uniform offset and had no region bounds > > checking. > > > > The function of_dma_get_range() has been modified so that it takes a single > > argument -- the device node -- and returns a map, NULL, or an error code. > > The map is an array that holds the information regarding the DMA regions. > > Each range entry contains the address offset, the cpu_start address, the > > dma_start address, and the size of the region. > > > > of_dma_configure() is the typical manner to set range offsets but there are > > a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel > > driver code. These cases now invoke the function > > dma_attach_offset_range(dev, cpu_addr, dma_addr, size). > > ... > > > + if (dev && dev->dma_range_map) > > + pfn -= (unsigned long)PFN_DOWN(dma_offset_from_phys_addr(dev, > > PFN_PHYS(pfn))); > > Instead of casting use PHYS_PFN() and it will be consistent with latter in > the same line. > > > + if (dev && dev->dma_range_map) > > + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, > > addr)); > > Ditto. > > ... > > > + dev_err(dev, "set dma_offset%08llx%s\n", > > KEYSTONE_HIGH_PHYS_START > > + - KEYSTONE_LOW_PHYS_START, ret ? " failed" : ""); > > Please, avoid such indentation. > Better split it to the three lines, argument per line (expect dev which will > go > on the first one). > > This applies to all similar places. > > ... > > > unsigned long pfn = (dma_handle >> PAGE_SHIFT); > > PHYS_PFN() / PFN_DOWN() ? > > > + if (!WARN_ON(!dev) && dev->dma_range_map) > > + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, > > dma_handle)); > > PHYS_PFN() ? > > ... > > > + r = kcalloc(num_ranges + 1, sizeof(struct bus_dma_region), > > GFP_KERNEL); > > sizeof(*r) ? > > > + if (!r) > > + return ERR_PTR(-ENOMEM); > > ... > > > + ret = IS_ERR(map) ? PTR_ERR(map) : 0; > > PTR_ERR_OR_ZERO() > > ... > > > + /* We want the offset map to be device-managed, so alloc & > > copy */ > > + dev->dma_range_map = devm_kcalloc(dev, num_ranges + 1, > > sizeof(*r), > > + GFP_KERNEL); > > The question is how many times per device lifetime this can be called? Someone else questioned this. There are two cases that come to mind: our overnight test which load/unloads the RC driver over and over, and a customer that does an unbind/bind of the RC driver when their EP is hung and wants to restart. Both cases are atypical and are weak arguments to justify the second copy. I will drop the copy. Thanks, Jim Quinlan Broadcom STB > > ... > > > > + if (!dev->dma_range_map) > > + return -ENOMEM; > > + memcpy((void *)dev->dma_range_map, map, sizeof(*r) * > > num_ranges + 1); > > If it's continuous, perhaps kmemdup() ? > > ... > > > + rc = IS_ERR(map) ? PTR_ERR(map) : 0; > > PTR_ERR_OR_ZERO() > > ... > > > + = dma_offset_from_phys_addr(dev, > > PFN_PHYS(mem->pfn_base)); > > + > > + return (dma_addr_t)PFN_PHYS(mem->pfn_base) - dma_offset; > > Looking at this more, I think you need to introduce in the same header (pfn.h) > something like: > > #define PFN_DMA_ADDR() > #define DMA_ADDR_PFN() > > -- > With Best Regards, > Andy Shevchenko > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
On Wed, Jul 01, 2020 at 05:21:38PM -0400, Jim Quinlan wrote: > The new field 'dma_range_map' in struct device is used to facilitate the > use of single or multiple offsets between mapping regions of cpu addrs and > dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only > capable of holding a single uniform offset and had no region bounds > checking. > > The function of_dma_get_range() has been modified so that it takes a single > argument -- the device node -- and returns a map, NULL, or an error code. > The map is an array that holds the information regarding the DMA regions. > Each range entry contains the address offset, the cpu_start address, the > dma_start address, and the size of the region. > > of_dma_configure() is the typical manner to set range offsets but there are > a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel > driver code. These cases now invoke the function > dma_attach_offset_range(dev, cpu_addr, dma_addr, size). ... > + if (dev && dev->dma_range_map) > + pfn -= (unsigned long)PFN_DOWN(dma_offset_from_phys_addr(dev, > PFN_PHYS(pfn))); Instead of casting use PHYS_PFN() and it will be consistent with latter in the same line. > + if (dev && dev->dma_range_map) > + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, > addr)); Ditto. ... > + dev_err(dev, "set dma_offset%08llx%s\n", > KEYSTONE_HIGH_PHYS_START > + - KEYSTONE_LOW_PHYS_START, ret ? " failed" : ""); Please, avoid such indentation. Better split it to the three lines, argument per line (expect dev which will go on the first one). This applies to all similar places. ... > unsigned long pfn = (dma_handle >> PAGE_SHIFT); PHYS_PFN() / PFN_DOWN() ? > + if (!WARN_ON(!dev) && dev->dma_range_map) > + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, > dma_handle)); PHYS_PFN() ? ... > + r = kcalloc(num_ranges + 1, sizeof(struct bus_dma_region), GFP_KERNEL); sizeof(*r) ? > + if (!r) > + return ERR_PTR(-ENOMEM); ... > + ret = IS_ERR(map) ? PTR_ERR(map) : 0; PTR_ERR_OR_ZERO() ... > + /* We want the offset map to be device-managed, so alloc & copy > */ > + dev->dma_range_map = devm_kcalloc(dev, num_ranges + 1, > sizeof(*r), > + GFP_KERNEL); The question is how many times per device lifetime this can be called? ... > + if (!dev->dma_range_map) > + return -ENOMEM; > + memcpy((void *)dev->dma_range_map, map, sizeof(*r) * num_ranges > + 1); If it's continuous, perhaps kmemdup() ? ... > + rc = IS_ERR(map) ? PTR_ERR(map) : 0; PTR_ERR_OR_ZERO() ... > + = dma_offset_from_phys_addr(dev, > PFN_PHYS(mem->pfn_base)); > + > + return (dma_addr_t)PFN_PHYS(mem->pfn_base) - dma_offset; Looking at this more, I think you need to introduce in the same header (pfn.h) something like: #define PFN_DMA_ADDR() #define DMA_ADDR_PFN() -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v6 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
The new field 'dma_range_map' in struct device is used to facilitate the
use of single or multiple offsets between mapping regions of cpu addrs and
dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only
capable of holding a single uniform offset and had no region bounds
checking.
The function of_dma_get_range() has been modified so that it takes a single
argument -- the device node -- and returns a map, NULL, or an error code.
The map is an array that holds the information regarding the DMA regions.
Each range entry contains the address offset, the cpu_start address, the
dma_start address, and the size of the region.
of_dma_configure() is the typical manner to set range offsets but there are
a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel
driver code. These cases now invoke the function
dma_attach_offset_range(dev, cpu_addr, dma_addr, size).
Signed-off-by: Jim Quinlan
---
arch/arm/include/asm/dma-mapping.h| 9 +-
arch/arm/mach-keystone/keystone.c | 17 ++--
arch/sh/drivers/pci/pcie-sh7786.c | 9 +-
arch/sh/kernel/dma-coherent.c | 14 +--
arch/x86/pci/sta2x11-fixup.c | 7 +-
drivers/acpi/arm64/iort.c | 5 +-
drivers/gpu/drm/sun4i/sun4i_backend.c | 7 +-
drivers/iommu/io-pgtable-arm.c| 2 +-
.../platform/sunxi/sun4i-csi/sun4i_csi.c | 6 +-
.../platform/sunxi/sun6i-csi/sun6i_csi.c | 5 +-
drivers/of/address.c | 95 ++-
drivers/of/device.c | 50 ++
drivers/of/of_private.h | 9 +-
drivers/of/unittest.c | 35 +--
drivers/remoteproc/remoteproc_core.c | 2 +-
.../staging/media/sunxi/cedrus/cedrus_hw.c| 8 +-
drivers/usb/core/message.c| 4 +-
drivers/usb/core/usb.c| 2 +-
include/linux/device.h| 4 +-
include/linux/dma-direct.h| 10 +-
include/linux/dma-mapping.h | 37
kernel/dma/coherent.c | 11 ++-
kernel/dma/mapping.c | 53 +++
23 files changed, 279 insertions(+), 122 deletions(-)
diff --git a/arch/arm/include/asm/dma-mapping.h
b/arch/arm/include/asm/dma-mapping.h
index bdd80ddbca34..b6796383f205 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -35,8 +35,9 @@ static inline const struct dma_map_ops
*get_arch_dma_ops(struct bus_type *bus)
#ifndef __arch_pfn_to_dma
static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
{
- if (dev)
- pfn -= dev->dma_pfn_offset;
+ if (dev && dev->dma_range_map)
+ pfn -= (unsigned long)PFN_DOWN(dma_offset_from_phys_addr(dev,
PFN_PHYS(pfn)));
+
return (dma_addr_t)__pfn_to_bus(pfn);
}
@@ -44,8 +45,8 @@ static inline unsigned long dma_to_pfn(struct device *dev,
dma_addr_t addr)
{
unsigned long pfn = __bus_to_pfn(addr);
- if (dev)
- pfn += dev->dma_pfn_offset;
+ if (dev && dev->dma_range_map)
+ pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev,
addr));
return pfn;
}
diff --git a/arch/arm/mach-keystone/keystone.c
b/arch/arm/mach-keystone/keystone.c
index 638808c4e122..a499ece6f054 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -8,6 +8,7 @@
*/
#include
#include
+#include
#include
#include
#include
@@ -24,8 +25,6 @@
#include "keystone.h"
-static unsigned long keystone_dma_pfn_offset __read_mostly;
-
static int keystone_platform_notifier(struct notifier_block *nb,
unsigned long event, void *data)
{
@@ -38,9 +37,12 @@ static int keystone_platform_notifier(struct notifier_block
*nb,
return NOTIFY_BAD;
if (!dev->of_node) {
- dev->dma_pfn_offset = keystone_dma_pfn_offset;
- dev_err(dev, "set dma_pfn_offset%08lx\n",
- dev->dma_pfn_offset);
+ int ret = dma_attach_offset_range(dev, KEYSTONE_HIGH_PHYS_START,
+ KEYSTONE_LOW_PHYS_START,
+ KEYSTONE_HIGH_PHYS_SIZE);
+ dev_err(dev, "set dma_offset%08llx%s\n",
KEYSTONE_HIGH_PHYS_START
+ - KEYSTONE_LOW_PHYS_START, ret ? " failed" : "");
+
}
return NOTIFY_OK;
}
@@ -51,11 +53,8 @@ static struct notifier_block platform_nb = {
static void __init keystone_init(void)
{
- if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) {
- keystone_dma_pfn_offset = PFN_DOWN(KEYSTONE_HIGH_PHYS_START -
- KEYSTONE_LOW_PHYS_START);
+ if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START)
