iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/14] PCI: brcmstb: enable PCIe for STB chips
@ 2020-05-26 19:12 Jim Quinlan via iommu
  2020-05-26 19:12 ` [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets Jim Quinlan via iommu
  2020-05-29 17:48 ` [PATCH v2 00/14] PCI: brcmstb: enable PCIe for STB chips Rob Herring
  0 siblings, 2 replies; 11+ messages in thread
From: Jim Quinlan via iommu @ 2020-05-26 19:12 UTC (permalink / raw)
  To: linux-pci, Christoph Hellwig, Nicolas Saenz Julienne,
	bcm-kernel-feedback-list, james.quinlan
  Cc: Ulf Hansson, Heikki Krogerus,
	open list:LIBATA SUBSYSTEM Serial and Parallel ATA drivers,
	Julien Grall, Rob Herring, Stefano Stabellini, Saravana Kannan,
	Rafael J. Wysocki, Alan Stern,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, Corey Minyard,
	Suzuki K Poulose, Mark Brown,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Dan Williams, Andy Shevchenko, moderated list:ARM PORT,
	Greg Kroah-Hartman, Oliver Neukum, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, open list:USB SUBSYSTEM,
	Robin Murphy

v2:
Commit: "device core: Add ability to handle multiple dma offsets"
  o Added helper func attach_dma_pfn_offset_map() in address.c (Chistoph)
  o Helpers funcs added to __phys_to_dma() & __dma_to_phys() (Christoph)
  o Added warning when multiple offsets are needed and !DMA_PFN_OFFSET_MAP
  o dev->dma_pfn_map => dev->dma_pfn_offset_map
  o s/frm/from/ for dma_pfn_offset_frm_{phys,dma}_addr() (Christoph)
  o In device.h: s/const void */const struct dma_pfn_offset_region */
  o removed 'unlikely' from unlikely(dev->dma_pfn_offset_map) since
    guarded by CONFIG_DMA_PFN_OFFSET_MAP (Christoph)
  o Since dev->dma_pfn_offset is copied in usb/core/{usb,message}.c, now
    dev->dma_pfn_offset_map is copied as well.
  o Merged two of the DMA commits into one (Christoph).

Commit "arm: dma-mapping: Invoke dma offset func if needed":
  o Use helper functions instead of #if CONFIG_DMA_PFN_OFFSET

Other commits' changes:
  o Removed need for carrying of_id var in priv (Nicolas)
  o Commit message rewordings (Bjorn)
  o Commit log messages filled to 75 chars (Bjorn)
  o devm_reset_control_get_shared())
    => devm_reset_control_get_optional_shared (Philipp)
  o Add call to reset_control_assert() in PCIe remove routines (Philipp)

v1:
This patchset expands the usefulness of the Broadcom Settop Box PCIe
controller by building upon the PCIe driver used currently by the
Raspbery Pi.  Other forms of this patchset were submitted by me years
ago and not accepted; the major sticking point was the code required
for the DMA remapping needed for the PCIe driver to work [1].

There have been many changes to the DMA and OF subsystems since that
time, making a cleaner and less intrusive patchset possible.  This
patchset implements a generalization of "dev->dma_pfn_offset", except
that instead of a single scalar offset it provides for multiple
offsets via a function which depends upon the "dma-ranges" property of
the PCIe host controller.  This is required for proper functionality
of the BrcmSTB PCIe controller and possibly some other devices.

[1] https://lore.kernel.org/linux-arm-kernel/1516058925-46522-5-git-send-email-jim2101024@gmail.com/

Jim Quinlan (14):
  PCI: brcmstb: PCIE_BRCMSTB depends on ARCH_BRCMSTB
  ata: ahci_brcm: Fix use of BCM7216 reset controller
  dt-bindings: PCI: Add bindings for more Brcmstb chips
  PCI: brcmstb: Add bcm7278 reigister info
  PCI: brcmstb: Add suspend and resume pm_ops
  PCI: brcmstb: Add bcm7278 PERST support
  PCI: brcmstb: Add control of rescal reset
  of: Include a dev param in of_dma_get_range()
  device core: Add ability to handle multiple dma offsets
  arm: dma-mapping: Invoke dma offset func if needed
  PCI: brcmstb: Set internal memory viewport sizes
  PCI: brcmstb: Accommodate MSI for older chips
  PCI: brcmstb: Set bus max burst size by chip type
  PCI: brcmstb: Add bcm7211, bcm7216, bcm7445, bcm7278 to match list

 .../bindings/pci/brcm,stb-pcie.yaml           |  40 +-
 arch/arm/include/asm/dma-mapping.h            |  13 +-
 drivers/ata/ahci_brcm.c                       |  14 +-
 drivers/of/address.c                          |  69 ++-
 drivers/of/device.c                           |   2 +-
 drivers/of/of_private.h                       |   8 +-
 drivers/pci/controller/Kconfig                |   3 +-
 drivers/pci/controller/pcie-brcmstb.c         | 408 +++++++++++++++---
 drivers/usb/core/message.c                    |   3 +
 drivers/usb/core/usb.c                        |   3 +
 include/linux/device.h                        |  10 +-
 include/linux/dma-direct.h                    |  10 +-
 include/linux/dma-mapping.h                   |  46 ++
 kernel/dma/Kconfig                            |  13 +
 14 files changed, 559 insertions(+), 83 deletions(-)

-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets
  2020-05-26 19:12 [PATCH v2 00/14] PCI: brcmstb: enable PCIe for STB chips Jim Quinlan via iommu
@ 2020-05-26 19:12 ` Jim Quinlan via iommu
  2020-05-26 20:54   ` Andy Shevchenko
  2020-05-27 15:00   ` Nicolas Saenz Julienne
  2020-05-29 17:48 ` [PATCH v2 00/14] PCI: brcmstb: enable PCIe for STB chips Rob Herring
  1 sibling, 2 replies; 11+ messages in thread
From: Jim Quinlan via iommu @ 2020-05-26 19:12 UTC (permalink / raw)
  To: linux-pci, Christoph Hellwig, Nicolas Saenz Julienne,
	bcm-kernel-feedback-list, james.quinlan
  Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
	Rafael J. Wysocki, Saravana Kannan, Corey Minyard,
	Suzuki K Poulose, Greg Kroah-Hartman, Oliver Neukum,
	Robin Murphy, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, Rob Herring, Alan Stern,
	Heikki Krogerus, Srinivas Kandagatla, Dan Williams,
	Andy Shevchenko, Frank Rowand, open list:USB SUBSYSTEM

The new field in struct device 'dma_pfn_offset_map' is used to facilitate
the use of multiple pfn offsets between cpu addrs and dma addrs.  It is
similar to 'dma_pfn_offset' except that the offset chosen depends on the
cpu or dma address involved.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/of/address.c        | 65 +++++++++++++++++++++++++++++++++++--
 drivers/usb/core/message.c  |  3 ++
 drivers/usb/core/usb.c      |  3 ++
 include/linux/device.h      | 10 +++++-
 include/linux/dma-direct.h  | 10 ++++--
 include/linux/dma-mapping.h | 46 ++++++++++++++++++++++++++
 kernel/dma/Kconfig          | 13 ++++++++
 7 files changed, 144 insertions(+), 6 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 96d8cfb14a60..a01afffcde7d 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -918,6 +918,47 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index,
 }
 EXPORT_SYMBOL(of_io_request_and_map);
 
+#ifdef CONFIG_DMA_PFN_OFFSET_MAP
+static int attach_dma_pfn_offset_map(struct device *dev,
+				     struct device_node *node, int num_ranges)
+{
+	struct of_range_parser parser;
+	struct of_range range;
+	size_t r_size = (num_ranges + 1)
+		* sizeof(struct dma_pfn_offset_region);
+	struct dma_pfn_offset_region *r;
+
+	r = devm_kzalloc(dev, r_size, GFP_KERNEL);
+	if (!r)
+		return -ENOMEM;
+	dev->dma_pfn_offset_map = r;
+	of_dma_range_parser_init(&parser, node);
+
+	/*
+	 * Record all info for DMA ranges array.  We could
+	 * just use the of_range struct, but if we did that it
+	 * would require more calculations for phys_to_dma and
+	 * dma_to_phys conversions.
+	 */
+	for_each_of_range(&parser, &range) {
+		r->cpu_beg = range.cpu_addr;
+		r->cpu_end = r->cpu_beg + range.size;
+		r->dma_beg = range.bus_addr;
+		r->dma_end = r->dma_beg + range.size;
+		r->pfn_offset = PFN_DOWN(range.cpu_addr)
+			- PFN_DOWN(range.bus_addr);
+		r++;
+	}
+	return 0;
+}
+#else
+static int attach_dma_pfn_offset_map(struct device *dev,
+				     struct device_node *node, int num_ranges)
+{
+	return 0;
+}
+#endif
+
 /**
  * of_dma_get_range - Get DMA range info
  * @dev:	device pointer; only needed for a corner case.
@@ -947,6 +988,8 @@ int of_dma_get_range(struct device *dev, struct device_node *np, u64 *dma_addr,
 	struct of_range_parser parser;
 	struct of_range range;
 	u64 dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
+	bool dma_multi_pfn_offset = false;
+	int num_ranges = 0;
 
 	while (node) {
 		ranges = of_get_property(node, "dma-ranges", &len);
@@ -977,10 +1020,19 @@ int of_dma_get_range(struct device *dev, struct device_node *np, u64 *dma_addr,
 		pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
 			 range.bus_addr, range.cpu_addr, range.size);
 
+		num_ranges++;
 		if (dma_offset && range.cpu_addr - range.bus_addr != dma_offset) {
-			pr_warn("Can't handle multiple dma-ranges with different offsets on node(%pOF)\n", node);
-			/* Don't error out as we'd break some existing DTs */
-			continue;
+			if (!IS_ENABLED(CONFIG_DMA_PFN_OFFSET_MAP)) {
+				pr_warn("Can't handle multiple dma-ranges with different offsets on node(%pOF)\n", node);
+				pr_warn("Perhaps set DMA_PFN_OFFSET_MAP=y?\n");
+				/*
+				 * Don't error out as we'd break some existing
+				 * DTs that are using configs w/o
+				 * CONFIG_DMA_PFN_OFFSET_MAP set.
+				 */
+				continue;
+			}
+			dma_multi_pfn_offset = true;
 		}
 		dma_offset = range.cpu_addr - range.bus_addr;
 
@@ -991,6 +1043,13 @@ int of_dma_get_range(struct device *dev, struct device_node *np, u64 *dma_addr,
 			dma_end = range.bus_addr + range.size;
 	}
 
+	if (dma_multi_pfn_offset) {
+		dma_offset = 0;
+		ret = attach_dma_pfn_offset_map(dev, node, num_ranges);
+		if (ret)
+			return ret;
+	}
+
 	if (dma_start >= dma_end) {
 		ret = -EINVAL;
 		pr_debug("Invalid DMA ranges configuration on node(%pOF)\n",
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 6197938dcc2d..aaa3e58f5eb4 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1960,6 +1960,9 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		 */
 		intf->dev.dma_mask = dev->dev.dma_mask;
 		intf->dev.dma_pfn_offset = dev->dev.dma_pfn_offset;
+#ifdef CONFIG_DMA_PFN_OFFSET_MAP
+		intf->dev.dma_pfn_offset_map = dev->dev.dma_pfn_offset_map;
+#endif
 		INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
 		intf->minor = -1;
 		device_initialize(&intf->dev);
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index f16c26dc079d..d2ed4d90e56e 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -612,6 +612,9 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
 	 */
 	dev->dev.dma_mask = bus->sysdev->dma_mask;
 	dev->dev.dma_pfn_offset = bus->sysdev->dma_pfn_offset;
+#ifdef CONFIG_DMA_PFN_OFFSET_MAP
+	dev->dev.dma_pfn_offset_map = bus->sysdev->dma_pfn_offset_map;
+#endif
 	set_dev_node(&dev->dev, dev_to_node(bus->sysdev));
 	dev->state = USB_STATE_ATTACHED;
 	dev->lpm_disable_count = 1;
diff --git a/include/linux/device.h b/include/linux/device.h
index ac8e37cd716a..67a240ad4fc5 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -493,6 +493,8 @@ struct dev_links_info {
  * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a smaller
  *		DMA limit than the device itself supports.
  * @dma_pfn_offset: offset of DMA memory range relatively of RAM
+ * @dma_pfn_offset_map:	Like dma_pfn_offset but used when there are multiple
+ *		pfn offsets for multiple dma-ranges.
  * @dma_parms:	A low level driver may set these to teach IOMMU code about
  * 		segment limitations.
  * @dma_pools:	Dma pools (if dma'ble device).
@@ -578,7 +580,13 @@ struct device {
 					     allocations such descriptors. */
 	u64		bus_dma_limit;	/* upstream dma constraint */
 	unsigned long	dma_pfn_offset;
-
+#ifdef CONFIG_DMA_PFN_OFFSET_MAP
+	const struct dma_pfn_offset_region *dma_pfn_offset_map;
+					/* Like dma_pfn_offset, but for
+					 * the unlikely case of multiple
+					 * offsets. If non-null, dma_pfn_offset
+					 * will be set to 0. */
+#endif
 	struct device_dma_parameters *dma_parms;
 
 	struct list_head	dma_pools;	/* dma pools (if dma'ble) */
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 24b8684aa21d..03110a57eabc 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -14,15 +14,21 @@ extern unsigned int zone_dma_bits;
 static inline dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
 	dma_addr_t dev_addr = (dma_addr_t)paddr;
+	/* The compiler should remove the 2nd term if !DMA_PFN_OFFSET_MAP */
+	unsigned long dma_pfn_offset = dev->dma_pfn_offset
+		+ dma_pfn_offset_from_phys_addr(dev, paddr);
 
-	return dev_addr - ((dma_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
+	return dev_addr - ((dma_addr_t)dma_pfn_offset << PAGE_SHIFT);
 }
 
 static inline phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t dev_addr)
 {
 	phys_addr_t paddr = (phys_addr_t)dev_addr;
+	/* The compiler should remove the 2nd term if !DMA_PFN_OFFSET_MAP */
+	unsigned long dma_pfn_offset = dev->dma_pfn_offset
+		+ dma_pfn_offset_from_dma_addr(dev, paddr);
 
-	return paddr + ((phys_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
+	return paddr + ((phys_addr_t)dma_pfn_offset << PAGE_SHIFT);
 }
 #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
 
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 330ad58fbf4d..91940bba2229 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -256,6 +256,52 @@ static inline void dma_direct_sync_sg_for_cpu(struct device *dev,
 size_t dma_direct_max_mapping_size(struct device *dev);
 
 #ifdef CONFIG_HAS_DMA
+#ifdef CONFIG_DMA_PFN_OFFSET_MAP
+struct dma_pfn_offset_region {
+	phys_addr_t	cpu_beg;
+	phys_addr_t	cpu_end;
+	dma_addr_t	dma_beg;
+	dma_addr_t	dma_end;
+	unsigned long	pfn_offset;
+};
+
+static inline unsigned long dma_pfn_offset_from_dma_addr(struct device *dev,
+							 dma_addr_t dma_addr)
+{
+	const struct dma_pfn_offset_region *m = dev->dma_pfn_offset_map;
+
+	if (m)
+		for (; m->cpu_end; m++)
+			if (dma_addr >= m->dma_beg && dma_addr < m->dma_end)
+				return m->pfn_offset;
+	return 0;
+}
+
+static inline unsigned long dma_pfn_offset_from_phys_addr(struct device *dev,
+							  phys_addr_t paddr)
+{
+	const struct dma_pfn_offset_region *m = dev->dma_pfn_offset_map;
+
+	if (m)
+		for (; m->cpu_end; m++)
+			if (paddr >= m->cpu_beg && paddr < m->cpu_end)
+				return m->pfn_offset;
+	return 0;
+}
+#else  /* CONFIG_DMA_PFN_OFFSET_MAP */
+static inline unsigned long dma_pfn_offset_from_dma_addr(struct device *dev,
+							 dma_addr_t dma_addr)
+{
+	return 0;
+}
+
+static inline unsigned long dma_pfn_offset_from_phys_addr(struct device *dev,
+							  phys_addr_t paddr)
+{
+	return 0;
+}
+#endif /* CONFIG_DMA_PFN_OFFSET_MAP */
+
 #include <asm/dma-mapping.h>
 
 static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
index 4c103a24e380..ceb7e5e8f501 100644
--- a/kernel/dma/Kconfig
+++ b/kernel/dma/Kconfig
@@ -195,3 +195,16 @@ config DMA_API_DEBUG_SG
 	  is technically out-of-spec.
 
 	  If unsure, say N.
+
+config DMA_PFN_OFFSET_MAP
+	bool "Uses a DMA range map to calculate PFN offset"
+	depends on PCIE_BRCMSTB
+	default n
+	help
+	  Some devices have a dma-range that gets converted to
+	  a dev->dma_pfn_offset value.  This option is for the
+	  atypical case of there being multiple dma-ranges requiring
+	  multiple pfn offsets, which are selected from when
+	  converting to phys to dma and vice versa.
+
+	  If unsure, say N.
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets
  2020-05-26 19:12 ` [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets Jim Quinlan via iommu
@ 2020-05-26 20:54   ` Andy Shevchenko
  2020-05-26 22:01     ` Jim Quinlan via iommu
  2020-05-27 15:00   ` Nicolas Saenz Julienne
  1 sibling, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-05-26 20:54 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Heikki Krogerus, linux-pci, Srinivas Kandagatla, Frank Rowand,
	Christoph Hellwig, Saravana Kannan, Rafael J. Wysocki,
	bcm-kernel-feedback-list, Alan Stern,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, Corey Minyard,
	Suzuki K Poulose, Rob Herring, Dan Williams, Greg Kroah-Hartman,
	Oliver Neukum, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, open list:USB SUBSYSTEM,
	Robin Murphy, Nicolas Saenz Julienne

On Tue, May 26, 2020 at 03:12:48PM -0400, Jim Quinlan wrote:
> The new field in struct device 'dma_pfn_offset_map' is used to facilitate
> the use of multiple pfn offsets between cpu addrs and dma addrs.  It is
> similar to 'dma_pfn_offset' except that the offset chosen depends on the
> cpu or dma address involved.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/of/address.c        | 65 +++++++++++++++++++++++++++++++++++--
>  drivers/usb/core/message.c  |  3 ++
>  drivers/usb/core/usb.c      |  3 ++
>  include/linux/device.h      | 10 +++++-
>  include/linux/dma-direct.h  | 10 ++++--
>  include/linux/dma-mapping.h | 46 ++++++++++++++++++++++++++
>  kernel/dma/Kconfig          | 13 ++++++++
>  7 files changed, 144 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 96d8cfb14a60..a01afffcde7d 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -918,6 +918,47 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index,
>  }
>  EXPORT_SYMBOL(of_io_request_and_map);
>  
> +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> +static int attach_dma_pfn_offset_map(struct device *dev,
> +				     struct device_node *node, int num_ranges)
> +{
> +	struct of_range_parser parser;
> +	struct of_range range;
> +	size_t r_size = (num_ranges + 1)
> +		* sizeof(struct dma_pfn_offset_region);
> +	struct dma_pfn_offset_region *r;
> +

> +	r = devm_kzalloc(dev, r_size, GFP_KERNEL);

devm_?!

Looking at r_size it should be rather kcalloc().

> +	if (!r)
> +		return -ENOMEM;
> +	dev->dma_pfn_offset_map = r;
> +	of_dma_range_parser_init(&parser, node);
> +
> +	/*
> +	 * Record all info for DMA ranges array.  We could
> +	 * just use the of_range struct, but if we did that it
> +	 * would require more calculations for phys_to_dma and
> +	 * dma_to_phys conversions.
> +	 */
> +	for_each_of_range(&parser, &range) {
> +		r->cpu_beg = range.cpu_addr;
> +		r->cpu_end = r->cpu_beg + range.size;
> +		r->dma_beg = range.bus_addr;
> +		r->dma_end = r->dma_beg + range.size;
> +		r->pfn_offset = PFN_DOWN(range.cpu_addr)
> +			- PFN_DOWN(range.bus_addr);
> +		r++;
> +	}
> +	return 0;
> +}
> +#else
> +static int attach_dma_pfn_offset_map(struct device *dev,
> +				     struct device_node *node, int num_ranges)
> +{
> +	return 0;
> +}
> +#endif
> +
>  /**
>   * of_dma_get_range - Get DMA range info
>   * @dev:	device pointer; only needed for a corner case.
> @@ -947,6 +988,8 @@ int of_dma_get_range(struct device *dev, struct device_node *np, u64 *dma_addr,
>  	struct of_range_parser parser;
>  	struct of_range range;
>  	u64 dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
> +	bool dma_multi_pfn_offset = false;
> +	int num_ranges = 0;
>  
>  	while (node) {
>  		ranges = of_get_property(node, "dma-ranges", &len);
> @@ -977,10 +1020,19 @@ int of_dma_get_range(struct device *dev, struct device_node *np, u64 *dma_addr,
>  		pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
>  			 range.bus_addr, range.cpu_addr, range.size);
>  
> +		num_ranges++;
>  		if (dma_offset && range.cpu_addr - range.bus_addr != dma_offset) {
> -			pr_warn("Can't handle multiple dma-ranges with different offsets on node(%pOF)\n", node);
> -			/* Don't error out as we'd break some existing DTs */
> -			continue;
> +			if (!IS_ENABLED(CONFIG_DMA_PFN_OFFSET_MAP)) {
> +				pr_warn("Can't handle multiple dma-ranges with different offsets on node(%pOF)\n", node);
> +				pr_warn("Perhaps set DMA_PFN_OFFSET_MAP=y?\n");
> +				/*
> +				 * Don't error out as we'd break some existing
> +				 * DTs that are using configs w/o
> +				 * CONFIG_DMA_PFN_OFFSET_MAP set.
> +				 */
> +				continue;
> +			}
> +			dma_multi_pfn_offset = true;
>  		}
>  		dma_offset = range.cpu_addr - range.bus_addr;
>  
> @@ -991,6 +1043,13 @@ int of_dma_get_range(struct device *dev, struct device_node *np, u64 *dma_addr,
>  			dma_end = range.bus_addr + range.size;
>  	}
>  
> +	if (dma_multi_pfn_offset) {
> +		dma_offset = 0;
> +		ret = attach_dma_pfn_offset_map(dev, node, num_ranges);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (dma_start >= dma_end) {
>  		ret = -EINVAL;
>  		pr_debug("Invalid DMA ranges configuration on node(%pOF)\n",
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 6197938dcc2d..aaa3e58f5eb4 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1960,6 +1960,9 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
>  		 */
>  		intf->dev.dma_mask = dev->dev.dma_mask;
>  		intf->dev.dma_pfn_offset = dev->dev.dma_pfn_offset;
> +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> +		intf->dev.dma_pfn_offset_map = dev->dev.dma_pfn_offset_map;
> +#endif
>  		INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
>  		intf->minor = -1;
>  		device_initialize(&intf->dev);
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index f16c26dc079d..d2ed4d90e56e 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -612,6 +612,9 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
>  	 */
>  	dev->dev.dma_mask = bus->sysdev->dma_mask;
>  	dev->dev.dma_pfn_offset = bus->sysdev->dma_pfn_offset;
> +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> +	dev->dev.dma_pfn_offset_map = bus->sysdev->dma_pfn_offset_map;
> +#endif
>  	set_dev_node(&dev->dev, dev_to_node(bus->sysdev));
>  	dev->state = USB_STATE_ATTACHED;
>  	dev->lpm_disable_count = 1;
> diff --git a/include/linux/device.h b/include/linux/device.h
> index ac8e37cd716a..67a240ad4fc5 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -493,6 +493,8 @@ struct dev_links_info {
>   * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a smaller
>   *		DMA limit than the device itself supports.
>   * @dma_pfn_offset: offset of DMA memory range relatively of RAM
> + * @dma_pfn_offset_map:	Like dma_pfn_offset but used when there are multiple
> + *		pfn offsets for multiple dma-ranges.
>   * @dma_parms:	A low level driver may set these to teach IOMMU code about
>   * 		segment limitations.
>   * @dma_pools:	Dma pools (if dma'ble device).
> @@ -578,7 +580,13 @@ struct device {
>  					     allocations such descriptors. */
>  	u64		bus_dma_limit;	/* upstream dma constraint */
>  	unsigned long	dma_pfn_offset;
> -
> +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> +	const struct dma_pfn_offset_region *dma_pfn_offset_map;

> +					/* Like dma_pfn_offset, but for
> +					 * the unlikely case of multiple
> +					 * offsets. If non-null, dma_pfn_offset
> +					 * will be set to 0. */

A bit harder to read comment indented too much and located after the declared variable.

> +#endif
>  	struct device_dma_parameters *dma_parms;
>  
>  	struct list_head	dma_pools;	/* dma pools (if dma'ble) */
> diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
> index 24b8684aa21d..03110a57eabc 100644
> --- a/include/linux/dma-direct.h
> +++ b/include/linux/dma-direct.h
> @@ -14,15 +14,21 @@ extern unsigned int zone_dma_bits;
>  static inline dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr)
>  {
>  	dma_addr_t dev_addr = (dma_addr_t)paddr;
> +	/* The compiler should remove the 2nd term if !DMA_PFN_OFFSET_MAP */
> +	unsigned long dma_pfn_offset = dev->dma_pfn_offset
> +		+ dma_pfn_offset_from_phys_addr(dev, paddr);
>  
> -	return dev_addr - ((dma_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
> +	return dev_addr - ((dma_addr_t)dma_pfn_offset << PAGE_SHIFT);
>  }
>  
>  static inline phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t dev_addr)
>  {
>  	phys_addr_t paddr = (phys_addr_t)dev_addr;
> +	/* The compiler should remove the 2nd term if !DMA_PFN_OFFSET_MAP */
> +	unsigned long dma_pfn_offset = dev->dma_pfn_offset
> +		+ dma_pfn_offset_from_dma_addr(dev, paddr);
>  
> -	return paddr + ((phys_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
> +	return paddr + ((phys_addr_t)dma_pfn_offset << PAGE_SHIFT);
>  }
>  #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
>  
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 330ad58fbf4d..91940bba2229 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -256,6 +256,52 @@ static inline void dma_direct_sync_sg_for_cpu(struct device *dev,
>  size_t dma_direct_max_mapping_size(struct device *dev);
>  
>  #ifdef CONFIG_HAS_DMA
> +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> +struct dma_pfn_offset_region {

> +	phys_addr_t	cpu_beg;
> +	phys_addr_t	cpu_end;
> +	dma_addr_t	dma_beg;
> +	dma_addr_t	dma_end;

Perhaps
	s,beg,start,
in above names

> +	unsigned long	pfn_offset;
> +};
> +
> +static inline unsigned long dma_pfn_offset_from_dma_addr(struct device *dev,
> +							 dma_addr_t dma_addr)
> +{
> +	const struct dma_pfn_offset_region *m = dev->dma_pfn_offset_map;

> +	if (m)
> +		for (; m->cpu_end; m++)

Why not simple

	while (m) {
		...
	}

?


> +			if (dma_addr >= m->dma_beg && dma_addr < m->dma_end)
> +				return m->pfn_offset;
> +	return 0;
> +}
> +
> +static inline unsigned long dma_pfn_offset_from_phys_addr(struct device *dev,
> +							  phys_addr_t paddr)
> +{
> +	const struct dma_pfn_offset_region *m = dev->dma_pfn_offset_map;
> +

> +	if (m)
> +		for (; m->cpu_end; m++)

Ditto.

> +			if (paddr >= m->cpu_beg && paddr < m->cpu_end)
> +				return m->pfn_offset;
> +	return 0;
> +}
> +#else  /* CONFIG_DMA_PFN_OFFSET_MAP */
> +static inline unsigned long dma_pfn_offset_from_dma_addr(struct device *dev,
> +							 dma_addr_t dma_addr)
> +{
> +	return 0;
> +}
> +
> +static inline unsigned long dma_pfn_offset_from_phys_addr(struct device *dev,
> +							  phys_addr_t paddr)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_DMA_PFN_OFFSET_MAP */
> +
>  #include <asm/dma-mapping.h>
>  
>  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
> diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
> index 4c103a24e380..ceb7e5e8f501 100644
> --- a/kernel/dma/Kconfig
> +++ b/kernel/dma/Kconfig
> @@ -195,3 +195,16 @@ config DMA_API_DEBUG_SG
>  	  is technically out-of-spec.
>  
>  	  If unsure, say N.
> +
> +config DMA_PFN_OFFSET_MAP
> +	bool "Uses a DMA range map to calculate PFN offset"
> +	depends on PCIE_BRCMSTB

> +	default n

Redundant.

> +	help
> +	  Some devices have a dma-range that gets converted to
> +	  a dev->dma_pfn_offset value.  This option is for the
> +	  atypical case of there being multiple dma-ranges requiring
> +	  multiple pfn offsets, which are selected from when
> +	  converting to phys to dma and vice versa.
> +
> +	  If unsure, say N.
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets
  2020-05-26 20:54   ` Andy Shevchenko
@ 2020-05-26 22:01     ` Jim Quinlan via iommu
  0 siblings, 0 replies; 11+ messages in thread
From: Jim Quinlan via iommu @ 2020-05-26 22:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Heikki Krogerus,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
	Srinivas Kandagatla, Frank Rowand, Christoph Hellwig,
	Saravana Kannan, Rafael J. Wysocki,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Alan Stern,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, Corey Minyard,
	Suzuki K Poulose, Rob Herring, Dan Williams, Greg Kroah-Hartman,
	Oliver Neukum, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, open list:USB SUBSYSTEM,
	Robin Murphy, Nicolas Saenz Julienne

Hello Andy,

On Tue, May 26, 2020 at 4:54 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, May 26, 2020 at 03:12:48PM -0400, Jim Quinlan wrote:
> > The new field in struct device 'dma_pfn_offset_map' is used to facilitate
> > the use of multiple pfn offsets between cpu addrs and dma addrs.  It is
> > similar to 'dma_pfn_offset' except that the offset chosen depends on the
> > cpu or dma address involved.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/of/address.c        | 65 +++++++++++++++++++++++++++++++++++--
> >  drivers/usb/core/message.c  |  3 ++
> >  drivers/usb/core/usb.c      |  3 ++
> >  include/linux/device.h      | 10 +++++-
> >  include/linux/dma-direct.h  | 10 ++++--
> >  include/linux/dma-mapping.h | 46 ++++++++++++++++++++++++++
> >  kernel/dma/Kconfig          | 13 ++++++++
> >  7 files changed, 144 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 96d8cfb14a60..a01afffcde7d 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -918,6 +918,47 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index,
> >  }
> >  EXPORT_SYMBOL(of_io_request_and_map);
> >
> > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > +static int attach_dma_pfn_offset_map(struct device *dev,
> > +                                  struct device_node *node, int num_ranges)
> > +{
> > +     struct of_range_parser parser;
> > +     struct of_range range;
> > +     size_t r_size = (num_ranges + 1)
> > +             * sizeof(struct dma_pfn_offset_region);
> > +     struct dma_pfn_offset_region *r;
> > +
>
> > +     r = devm_kzalloc(dev, r_size, GFP_KERNEL);
>
> devm_?!

Yes, otherwise if the device gets unbound/bound repeatedly then there
would be a memory leak.

>
>
> Looking at r_size it should be rather kcalloc().

Yep.
>
>
> > +     if (!r)
> > +             return -ENOMEM;
> > +     dev->dma_pfn_offset_map = r;
> > +     of_dma_range_parser_init(&parser, node);
> > +
> > +     /*
> > +      * Record all info for DMA ranges array.  We could
> > +      * just use the of_range struct, but if we did that it
> > +      * would require more calculations for phys_to_dma and
> > +      * dma_to_phys conversions.
> > +      */
> > +     for_each_of_range(&parser, &range) {
> > +             r->cpu_beg = range.cpu_addr;
> > +             r->cpu_end = r->cpu_beg + range.size;
> > +             r->dma_beg = range.bus_addr;
> > +             r->dma_end = r->dma_beg + range.size;
> > +             r->pfn_offset = PFN_DOWN(range.cpu_addr)
> > +                     - PFN_DOWN(range.bus_addr);
> > +             r++;
> > +     }
> > +     return 0;
> > +}
> > +#else
> > +static int attach_dma_pfn_offset_map(struct device *dev,
> > +                                  struct device_node *node, int num_ranges)
> > +{
> > +     return 0;
> > +}
> > +#endif
> > +
> >  /**
> >   * of_dma_get_range - Get DMA range info
> >   * @dev:     device pointer; only needed for a corner case.
> > @@ -947,6 +988,8 @@ int of_dma_get_range(struct device *dev, struct device_node *np, u64 *dma_addr,
> >       struct of_range_parser parser;
> >       struct of_range range;
> >       u64 dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
> > +     bool dma_multi_pfn_offset = false;
> > +     int num_ranges = 0;
> >
> >       while (node) {
> >               ranges = of_get_property(node, "dma-ranges", &len);
> > @@ -977,10 +1020,19 @@ int of_dma_get_range(struct device *dev, struct device_node *np, u64 *dma_addr,
> >               pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
> >                        range.bus_addr, range.cpu_addr, range.size);
> >
> > +             num_ranges++;
> >               if (dma_offset && range.cpu_addr - range.bus_addr != dma_offset) {
> > -                     pr_warn("Can't handle multiple dma-ranges with different offsets on node(%pOF)\n", node);
> > -                     /* Don't error out as we'd break some existing DTs */
> > -                     continue;
> > +                     if (!IS_ENABLED(CONFIG_DMA_PFN_OFFSET_MAP)) {
> > +                             pr_warn("Can't handle multiple dma-ranges with different offsets on node(%pOF)\n", node);
> > +                             pr_warn("Perhaps set DMA_PFN_OFFSET_MAP=y?\n");
> > +                             /*
> > +                              * Don't error out as we'd break some existing
> > +                              * DTs that are using configs w/o
> > +                              * CONFIG_DMA_PFN_OFFSET_MAP set.
> > +                              */
> > +                             continue;
> > +                     }
> > +                     dma_multi_pfn_offset = true;
> >               }
> >               dma_offset = range.cpu_addr - range.bus_addr;
> >
> > @@ -991,6 +1043,13 @@ int of_dma_get_range(struct device *dev, struct device_node *np, u64 *dma_addr,
> >                       dma_end = range.bus_addr + range.size;
> >       }
> >
> > +     if (dma_multi_pfn_offset) {
> > +             dma_offset = 0;
> > +             ret = attach_dma_pfn_offset_map(dev, node, num_ranges);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       if (dma_start >= dma_end) {
> >               ret = -EINVAL;
> >               pr_debug("Invalid DMA ranges configuration on node(%pOF)\n",
> > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > index 6197938dcc2d..aaa3e58f5eb4 100644
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -1960,6 +1960,9 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
> >                */
> >               intf->dev.dma_mask = dev->dev.dma_mask;
> >               intf->dev.dma_pfn_offset = dev->dev.dma_pfn_offset;
> > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > +             intf->dev.dma_pfn_offset_map = dev->dev.dma_pfn_offset_map;
> > +#endif
> >               INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
> >               intf->minor = -1;
> >               device_initialize(&intf->dev);
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index f16c26dc079d..d2ed4d90e56e 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -612,6 +612,9 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
> >        */
> >       dev->dev.dma_mask = bus->sysdev->dma_mask;
> >       dev->dev.dma_pfn_offset = bus->sysdev->dma_pfn_offset;
> > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > +     dev->dev.dma_pfn_offset_map = bus->sysdev->dma_pfn_offset_map;
> > +#endif
> >       set_dev_node(&dev->dev, dev_to_node(bus->sysdev));
> >       dev->state = USB_STATE_ATTACHED;
> >       dev->lpm_disable_count = 1;
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index ac8e37cd716a..67a240ad4fc5 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -493,6 +493,8 @@ struct dev_links_info {
> >   * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a smaller
> >   *           DMA limit than the device itself supports.
> >   * @dma_pfn_offset: offset of DMA memory range relatively of RAM
> > + * @dma_pfn_offset_map:      Like dma_pfn_offset but used when there are multiple
> > + *           pfn offsets for multiple dma-ranges.
> >   * @dma_parms:       A low level driver may set these to teach IOMMU code about
> >   *           segment limitations.
> >   * @dma_pools:       Dma pools (if dma'ble device).
> > @@ -578,7 +580,13 @@ struct device {
> >                                            allocations such descriptors. */
> >       u64             bus_dma_limit;  /* upstream dma constraint */
> >       unsigned long   dma_pfn_offset;
> > -
> > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > +     const struct dma_pfn_offset_region *dma_pfn_offset_map;
>
> > +                                     /* Like dma_pfn_offset, but for
> > +                                      * the unlikely case of multiple
> > +                                      * offsets. If non-null, dma_pfn_offset
> > +                                      * will be set to 0. */
>
> A bit harder to read comment indented too much and located after the declared variable.

Okay, will change. I was trying to keep the comment style of the other
variables.

> > +#endif
> >       struct device_dma_parameters *dma_parms;
> >
> >       struct list_head        dma_pools;      /* dma pools (if dma'ble) */
> > diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
> > index 24b8684aa21d..03110a57eabc 100644
> > --- a/include/linux/dma-direct.h
> > +++ b/include/linux/dma-direct.h
> > @@ -14,15 +14,21 @@ extern unsigned int zone_dma_bits;
> >  static inline dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr)
> >  {
> >       dma_addr_t dev_addr = (dma_addr_t)paddr;
> > +     /* The compiler should remove the 2nd term if !DMA_PFN_OFFSET_MAP */
> > +     unsigned long dma_pfn_offset = dev->dma_pfn_offset
> > +             + dma_pfn_offset_from_phys_addr(dev, paddr);
> >
> > -     return dev_addr - ((dma_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
> > +     return dev_addr - ((dma_addr_t)dma_pfn_offset << PAGE_SHIFT);
> >  }
> >
> >  static inline phys_addr_t __dma_to_phys(struct device *dev, dma_addr_t dev_addr)
> >  {
> >       phys_addr_t paddr = (phys_addr_t)dev_addr;
> > +     /* The compiler should remove the 2nd term if !DMA_PFN_OFFSET_MAP */
> > +     unsigned long dma_pfn_offset = dev->dma_pfn_offset
> > +             + dma_pfn_offset_from_dma_addr(dev, paddr);
> >
> > -     return paddr + ((phys_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
> > +     return paddr + ((phys_addr_t)dma_pfn_offset << PAGE_SHIFT);
> >  }
> >  #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */
> >
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index 330ad58fbf4d..91940bba2229 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -256,6 +256,52 @@ static inline void dma_direct_sync_sg_for_cpu(struct device *dev,
> >  size_t dma_direct_max_mapping_size(struct device *dev);
> >
> >  #ifdef CONFIG_HAS_DMA
> > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > +struct dma_pfn_offset_region {
>
> > +     phys_addr_t     cpu_beg;
> > +     phys_addr_t     cpu_end;
> > +     dma_addr_t      dma_beg;
> > +     dma_addr_t      dma_end;
>
> Perhaps
>         s,beg,start,
> in above names
>
Okay.

>
> > +     unsigned long   pfn_offset;
> > +};
> > +
> > +static inline unsigned long dma_pfn_offset_from_dma_addr(struct device *dev,
> > +                                                      dma_addr_t dma_addr)
> > +{
> > +     const struct dma_pfn_offset_region *m = dev->dma_pfn_offset_map;
>
> > +     if (m)
> > +             for (; m->cpu_end; m++)
>
> Why not simple
>
>         while (m) {
>                 ...
>         }
>
> ?
>
That won't work;  'm' is either null or a valid pointer to an array
which has an additional entry that is 0-filled..  If non-null, 'm'
will never turn into NULL via 'm++' and the while loop will not
terminate.
>
>
> > +                     if (dma_addr >= m->dma_beg && dma_addr < m->dma_end)
> > +                             return m->pfn_offset;
> > +     return 0;
> > +}
> > +
> > +static inline unsigned long dma_pfn_offset_from_phys_addr(struct device *dev,
> > +                                                       phys_addr_t paddr)
> > +{
> > +     const struct dma_pfn_offset_region *m = dev->dma_pfn_offset_map;
> > +
>
> > +     if (m)
> > +             for (; m->cpu_end; m++)
>
> Ditto.
>
> > +                     if (paddr >= m->cpu_beg && paddr < m->cpu_end)
> > +                             return m->pfn_offset;
> > +     return 0;
> > +}
> > +#else  /* CONFIG_DMA_PFN_OFFSET_MAP */
> > +static inline unsigned long dma_pfn_offset_from_dma_addr(struct device *dev,
> > +                                                      dma_addr_t dma_addr)
> > +{
> > +     return 0;
> > +}
> > +
> > +static inline unsigned long dma_pfn_offset_from_phys_addr(struct device *dev,
> > +                                                       phys_addr_t paddr)
> > +{
> > +     return 0;
> > +}
> > +#endif /* CONFIG_DMA_PFN_OFFSET_MAP */
> > +
> >  #include <asm/dma-mapping.h>
> >
> >  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
> > diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
> > index 4c103a24e380..ceb7e5e8f501 100644
> > --- a/kernel/dma/Kconfig
> > +++ b/kernel/dma/Kconfig
> > @@ -195,3 +195,16 @@ config DMA_API_DEBUG_SG
> >         is technically out-of-spec.
> >
> >         If unsure, say N.
> > +
> > +config DMA_PFN_OFFSET_MAP
> > +     bool "Uses a DMA range map to calculate PFN offset"
> > +     depends on PCIE_BRCMSTB
>
> > +     default n
>
> Redundant.

Okay.
>
> > +     help
> > +       Some devices have a dma-range that gets converted to
> > +       a dev->dma_pfn_offset value.  This option is for the
> > +       atypical case of there being multiple dma-ranges requiring
> > +       multiple pfn offsets, which are selected from when
> > +       converting to phys to dma and vice versa.
> > +
> > +       If unsure, say N.
> > --
> > 2.17.1
> >
>
> --
> With Best Regards,
> Andy Shevchenko


Thanks!
Jim Quinlan
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets
  2020-05-26 19:12 ` [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets Jim Quinlan via iommu
  2020-05-26 20:54   ` Andy Shevchenko
@ 2020-05-27 15:00   ` Nicolas Saenz Julienne
  2020-05-27 15:43     ` Jim Quinlan via iommu
  1 sibling, 1 reply; 11+ messages in thread
From: Nicolas Saenz Julienne @ 2020-05-27 15:00 UTC (permalink / raw)
  To: Jim Quinlan, linux-pci, Christoph Hellwig, bcm-kernel-feedback-list
  Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
	Rafael J. Wysocki, Saravana Kannan, Corey Minyard,
	Suzuki K Poulose, Greg Kroah-Hartman, Oliver Neukum,
	Robin Murphy, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, Rob Herring, Alan Stern,
	Heikki Krogerus, Srinivas Kandagatla, Dan Williams,
	Andy Shevchenko, Frank Rowand, open list:USB SUBSYSTEM


[-- Attachment #1.1: Type: text/plain, Size: 6547 bytes --]

Hi Jim,
one thing comes to mind, there is a small test suite in drivers/of/unittest.c
(specifically of_unittest_pci_dma_ranges()) you could extend it to include your
use cases.

On Tue, 2020-05-26 at 15:12 -0400, Jim Quinlan wrote:
> The new field in struct device 'dma_pfn_offset_map' is used to facilitate
> the use of multiple pfn offsets between cpu addrs and dma addrs.  It is
> similar to 'dma_pfn_offset' except that the offset chosen depends on the
> cpu or dma address involved.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/of/address.c        | 65 +++++++++++++++++++++++++++++++++++--
>  drivers/usb/core/message.c  |  3 ++
>  drivers/usb/core/usb.c      |  3 ++
>  include/linux/device.h      | 10 +++++-
>  include/linux/dma-direct.h  | 10 ++++--
>  include/linux/dma-mapping.h | 46 ++++++++++++++++++++++++++
>  kernel/dma/Kconfig          | 13 ++++++++
>  7 files changed, 144 insertions(+), 6 deletions(-)
> 

[...]

> @@ -977,10 +1020,19 @@ int of_dma_get_range(struct device *dev, struct
> device_node *np, u64 *dma_addr,
>  		pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
>  			 range.bus_addr, range.cpu_addr, range.size);
>  
> +		num_ranges++;
>  		if (dma_offset && range.cpu_addr - range.bus_addr != dma_offset)
> {
> -			pr_warn("Can't handle multiple dma-ranges with different
> offsets on node(%pOF)\n", node);
> -			/* Don't error out as we'd break some existing DTs */
> -			continue;
> +			if (!IS_ENABLED(CONFIG_DMA_PFN_OFFSET_MAP)) {
> +				pr_warn("Can't handle multiple dma-ranges with
> different offsets on node(%pOF)\n", node);
> +				pr_warn("Perhaps set DMA_PFN_OFFSET_MAP=y?\n");
> +				/*
> +				 * Don't error out as we'd break some existing
> +				 * DTs that are using configs w/o
> +				 * CONFIG_DMA_PFN_OFFSET_MAP set.
> +				 */
> +				continue;

dev->bus_dma_limit is set in of_dma_configure(), this function's caller, based
on dma_start's value (set after this continue). So you'd be effectively setting
the dev->bus_dma_limit to whatever we get from the first dma-range.

This can be troublesome depending on how the dma-ranges are setup, for example
if the first dma-range doesn't include the CMA area, in arm64 generally set as
high as possible in ZONE_DMA32, that would render it useless for
dma/{direct/swiotlb}. Again depending on the bus_dma_limit value, if smaller
than ZONE_DMA you'd be unable to allocate any DMA memory.

IMO, a solution to this calls for a revamp of dma-direct's dma_capable(): match
the target DMA memory area with each dma-range we have to see if it fits.

> +			}
> +			dma_multi_pfn_offset = true;
>  		}
>  		dma_offset = range.cpu_addr - range.bus_addr;
>  
> @@ -991,6 +1043,13 @@ int of_dma_get_range(struct device *dev, struct
> device_node *np, u64 *dma_addr,
>  			dma_end = range.bus_addr + range.size;
>  	}
>  
> +	if (dma_multi_pfn_offset) {
> +		dma_offset = 0;
> +		ret = attach_dma_pfn_offset_map(dev, node, num_ranges);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (dma_start >= dma_end) {
>  		ret = -EINVAL;
>  		pr_debug("Invalid DMA ranges configuration on node(%pOF)\n",
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 6197938dcc2d..aaa3e58f5eb4 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1960,6 +1960,9 @@ int usb_set_configuration(struct usb_device *dev, int
> configuration)
>  		 */
>  		intf->dev.dma_mask = dev->dev.dma_mask;
>  		intf->dev.dma_pfn_offset = dev->dev.dma_pfn_offset;
> +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> +		intf->dev.dma_pfn_offset_map = dev->dev.dma_pfn_offset_map;
> +#endif

Thanks for looking at this, that said, I see more instances of drivers changing
dma_pfn_offset outside of the core code. Why not doing this there too?

Also, are we 100% sure that dev->dev.dma_pfn_offset isn't going to be freed
before we're done using intf->dev? Maybe it's safer to copy the ranges?

>  		INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
>  		intf->minor = -1;
>  		device_initialize(&intf->dev);
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index f16c26dc079d..d2ed4d90e56e 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -612,6 +612,9 @@ struct usb_device *usb_alloc_dev(struct usb_device
> *parent,
>  	 */
>  	dev->dev.dma_mask = bus->sysdev->dma_mask;
>  	dev->dev.dma_pfn_offset = bus->sysdev->dma_pfn_offset;
> +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> +	dev->dev.dma_pfn_offset_map = bus->sysdev->dma_pfn_offset_map;
> +#endif
>  	set_dev_node(&dev->dev, dev_to_node(bus->sysdev));
>  	dev->state = USB_STATE_ATTACHED;
>  	dev->lpm_disable_count = 1;
> diff --git a/include/linux/device.h b/include/linux/device.h
> index ac8e37cd716a..67a240ad4fc5 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -493,6 +493,8 @@ struct dev_links_info {
>   * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a smaller
>   *		DMA limit than the device itself supports.
>   * @dma_pfn_offset: offset of DMA memory range relatively of RAM
> + * @dma_pfn_offset_map:	Like dma_pfn_offset but used when there are
> multiple
> + *		pfn offsets for multiple dma-ranges.
>   * @dma_parms:	A low level driver may set these to teach IOMMU code
> about
>   * 		segment limitations.
>   * @dma_pools:	Dma pools (if dma'ble device).
> @@ -578,7 +580,13 @@ struct device {
>  					     allocations such descriptors. */
>  	u64		bus_dma_limit;	/* upstream dma constraint */
>  	unsigned long	dma_pfn_offset;
> -
> +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> +	const struct dma_pfn_offset_region *dma_pfn_offset_map;
> +					/* Like dma_pfn_offset, but for
> +					 * the unlikely case of multiple
> +					 * offsets. If non-null, dma_pfn_offset
> +					 * will be set to 0. */
> +#endif

I'm still sad this doesn't fully replace dma_pfn_offset & bus_dma_limit. I feel
the extra logic involved in incorporating this as default isn't going to be
noticeable as far as performance is concerned to single dma-range users, and
it'd make for a nicer DMA code. Also you'd force everyone to test their changes
on the multi dma-ranges code path, as opposed to having this disabled 99.9% of
the time (hence broken every so often).

Note that I sympathize with the amount of work involved on improving that, so
better wait to hear what more knowledgeable people have to say about this :)

Regards,
Nicolas


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets
  2020-05-27 15:00   ` Nicolas Saenz Julienne
@ 2020-05-27 15:43     ` Jim Quinlan via iommu
  2020-05-27 17:01       ` Nicolas Saenz Julienne
  2020-05-29 17:34       ` Rob Herring
  0 siblings, 2 replies; 11+ messages in thread
From: Jim Quinlan via iommu @ 2020-05-27 15:43 UTC (permalink / raw)
  To: Nicolas Saenz Julienne
  Cc: open list:USB SUBSYSTEM,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
	Heikki Krogerus, Srinivas Kandagatla, Frank Rowand,
	Christoph Hellwig, Saravana Kannan, Rafael J. Wysocki,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Alan Stern,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, Corey Minyard,
	Suzuki K Poulose, Rob Herring, Dan Williams, Andy Shevchenko,
	Greg Kroah-Hartman, Oliver Neukum, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, Robin Murphy

Hi Nicolas,

On Wed, May 27, 2020 at 11:00 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Jim,
> one thing comes to mind, there is a small test suite in drivers/of/unittest.c
> (specifically of_unittest_pci_dma_ranges()) you could extend it to include your
> use cases.
Sure, will check out.
>
> On Tue, 2020-05-26 at 15:12 -0400, Jim Quinlan wrote:
> > The new field in struct device 'dma_pfn_offset_map' is used to facilitate
> > the use of multiple pfn offsets between cpu addrs and dma addrs.  It is
> > similar to 'dma_pfn_offset' except that the offset chosen depends on the
> > cpu or dma address involved.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/of/address.c        | 65 +++++++++++++++++++++++++++++++++++--
> >  drivers/usb/core/message.c  |  3 ++
> >  drivers/usb/core/usb.c      |  3 ++
> >  include/linux/device.h      | 10 +++++-
> >  include/linux/dma-direct.h  | 10 ++++--
> >  include/linux/dma-mapping.h | 46 ++++++++++++++++++++++++++
> >  kernel/dma/Kconfig          | 13 ++++++++
> >  7 files changed, 144 insertions(+), 6 deletions(-)
> >
>
> [...]
>
> > @@ -977,10 +1020,19 @@ int of_dma_get_range(struct device *dev, struct
> > device_node *np, u64 *dma_addr,
> >               pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
> >                        range.bus_addr, range.cpu_addr, range.size);
> >
> > +             num_ranges++;
> >               if (dma_offset && range.cpu_addr - range.bus_addr != dma_offset)
> > {
> > -                     pr_warn("Can't handle multiple dma-ranges with different
> > offsets on node(%pOF)\n", node);
> > -                     /* Don't error out as we'd break some existing DTs */
> > -                     continue;
> > +                     if (!IS_ENABLED(CONFIG_DMA_PFN_OFFSET_MAP)) {
> > +                             pr_warn("Can't handle multiple dma-ranges with
> > different offsets on node(%pOF)\n", node);
> > +                             pr_warn("Perhaps set DMA_PFN_OFFSET_MAP=y?\n");
> > +                             /*
> > +                              * Don't error out as we'd break some existing
> > +                              * DTs that are using configs w/o
> > +                              * CONFIG_DMA_PFN_OFFSET_MAP set.
> > +                              */
> > +                             continue;
>
> dev->bus_dma_limit is set in of_dma_configure(), this function's caller, based
> on dma_start's value (set after this continue). So you'd be effectively setting
> the dev->bus_dma_limit to whatever we get from the first dma-range.
I'm not seeing that at all.  On the  evaluation of each dma-range,
dma_start and dma_end are re-evaluated to be the lowest and highest
bus values of the  dma-ranges seen so far.  After all dma-ranges are
examined,  dev->bus_dma_limit being set to the highest.  In fact, the
current code -- ie before my commits -- already does this for multiple
dma-ranges as long as the cpu-bus offset is the same in the
dma-ranges.
>
> This can be troublesome depending on how the dma-ranges are setup, for example
> if the first dma-range doesn't include the CMA area, in arm64 generally set as
> high as possible in ZONE_DMA32, that would render it useless for
> dma/{direct/swiotlb}. Again depending on the bus_dma_limit value, if smaller
> than ZONE_DMA you'd be unable to allocate any DMA memory.
>
> IMO, a solution to this calls for a revamp of dma-direct's dma_capable(): match
> the target DMA memory area with each dma-range we have to see if it fits.
>
> > +                     }
> > +                     dma_multi_pfn_offset = true;
> >               }
> >               dma_offset = range.cpu_addr - range.bus_addr;
> >
> > @@ -991,6 +1043,13 @@ int of_dma_get_range(struct device *dev, struct
> > device_node *np, u64 *dma_addr,
> >                       dma_end = range.bus_addr + range.size;
> >       }
> >
> > +     if (dma_multi_pfn_offset) {
> > +             dma_offset = 0;
> > +             ret = attach_dma_pfn_offset_map(dev, node, num_ranges);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       if (dma_start >= dma_end) {
> >               ret = -EINVAL;
> >               pr_debug("Invalid DMA ranges configuration on node(%pOF)\n",
> > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > index 6197938dcc2d..aaa3e58f5eb4 100644
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -1960,6 +1960,9 @@ int usb_set_configuration(struct usb_device *dev, int
> > configuration)
> >                */
> >               intf->dev.dma_mask = dev->dev.dma_mask;
> >               intf->dev.dma_pfn_offset = dev->dev.dma_pfn_offset;
> > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > +             intf->dev.dma_pfn_offset_map = dev->dev.dma_pfn_offset_map;
> > +#endif
>
> Thanks for looking at this, that said, I see more instances of drivers changing
> dma_pfn_offset outside of the core code. Why not doing this there too?
>
> Also, are we 100% sure that dev->dev.dma_pfn_offset isn't going to be freed
> before we're done using intf->dev? Maybe it's safer to copy the ranges?
>
> >               INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
> >               intf->minor = -1;
> >               device_initialize(&intf->dev);
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index f16c26dc079d..d2ed4d90e56e 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -612,6 +612,9 @@ struct usb_device *usb_alloc_dev(struct usb_device
> > *parent,
> >        */
> >       dev->dev.dma_mask = bus->sysdev->dma_mask;
> >       dev->dev.dma_pfn_offset = bus->sysdev->dma_pfn_offset;
> > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > +     dev->dev.dma_pfn_offset_map = bus->sysdev->dma_pfn_offset_map;
> > +#endif
> >       set_dev_node(&dev->dev, dev_to_node(bus->sysdev));
> >       dev->state = USB_STATE_ATTACHED;
> >       dev->lpm_disable_count = 1;
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index ac8e37cd716a..67a240ad4fc5 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -493,6 +493,8 @@ struct dev_links_info {
> >   * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a smaller
> >   *           DMA limit than the device itself supports.
> >   * @dma_pfn_offset: offset of DMA memory range relatively of RAM
> > + * @dma_pfn_offset_map:      Like dma_pfn_offset but used when there are
> > multiple
> > + *           pfn offsets for multiple dma-ranges.
> >   * @dma_parms:       A low level driver may set these to teach IOMMU code
> > about
> >   *           segment limitations.
> >   * @dma_pools:       Dma pools (if dma'ble device).
> > @@ -578,7 +580,13 @@ struct device {
> >                                            allocations such descriptors. */
> >       u64             bus_dma_limit;  /* upstream dma constraint */
> >       unsigned long   dma_pfn_offset;
> > -
> > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > +     const struct dma_pfn_offset_region *dma_pfn_offset_map;
> > +                                     /* Like dma_pfn_offset, but for
> > +                                      * the unlikely case of multiple
> > +                                      * offsets. If non-null, dma_pfn_offset
> > +                                      * will be set to 0. */
> > +#endif
>
> I'm still sad this doesn't fully replace dma_pfn_offset & bus_dma_limit. I feel
> the extra logic involved in incorporating this as default isn't going to be
> noticeable as far as performance is concerned to single dma-range users, and
> it'd make for a nicer DMA code. Also you'd force everyone to test their changes
> on the multi dma-ranges code path, as opposed to having this disabled 99.9% of
> the time (hence broken every so often).
Good point.
>
> Note that I sympathize with the amount of work involved on improving that, so
> better wait to hear what more knowledgeable people have to say about this :)
Yes, I agree.  I want to avoid coding and testing one solution only to
have a different reviewer NAK it.

Many thanks,
Jim
>
> Regards,
> Nicolas
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets
  2020-05-27 15:43     ` Jim Quinlan via iommu
@ 2020-05-27 17:01       ` Nicolas Saenz Julienne
  2020-05-29 17:34       ` Rob Herring
  1 sibling, 0 replies; 11+ messages in thread
From: Nicolas Saenz Julienne @ 2020-05-27 17:01 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: open list:USB SUBSYSTEM,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
	Heikki Krogerus, Srinivas Kandagatla, Frank Rowand,
	Christoph Hellwig, Saravana Kannan, Rafael J. Wysocki,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Alan Stern,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, Corey Minyard,
	Suzuki K Poulose, Rob Herring, Dan Williams, Andy Shevchenko,
	Greg Kroah-Hartman, Oliver Neukum, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, Robin Murphy


[-- Attachment #1.1: Type: text/plain, Size: 3658 bytes --]

Hi Jim,

On Wed, 2020-05-27 at 11:43 -0400, Jim Quinlan wrote:
> Hi Nicolas,
> 
> On Wed, May 27, 2020 at 11:00 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Hi Jim,
> > one thing comes to mind, there is a small test suite in
> > drivers/of/unittest.c
> > (specifically of_unittest_pci_dma_ranges()) you could extend it to include
> > your
> > use cases.
> Sure, will check out.
> > On Tue, 2020-05-26 at 15:12 -0400, Jim Quinlan wrote:
> > > The new field in struct device 'dma_pfn_offset_map' is used to facilitate
> > > the use of multiple pfn offsets between cpu addrs and dma addrs.  It is
> > > similar to 'dma_pfn_offset' except that the offset chosen depends on the
> > > cpu or dma address involved.
> > > 
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > ---
> > >  drivers/of/address.c        | 65 +++++++++++++++++++++++++++++++++++--
> > >  drivers/usb/core/message.c  |  3 ++
> > >  drivers/usb/core/usb.c      |  3 ++
> > >  include/linux/device.h      | 10 +++++-
> > >  include/linux/dma-direct.h  | 10 ++++--
> > >  include/linux/dma-mapping.h | 46 ++++++++++++++++++++++++++
> > >  kernel/dma/Kconfig          | 13 ++++++++
> > >  7 files changed, 144 insertions(+), 6 deletions(-)
> > > 
> > 
> > [...]
> > 
> > > @@ -977,10 +1020,19 @@ int of_dma_get_range(struct device *dev, struct
> > > device_node *np, u64 *dma_addr,
> > >               pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
> > >                        range.bus_addr, range.cpu_addr, range.size);
> > > 
> > > +             num_ranges++;
> > >               if (dma_offset && range.cpu_addr - range.bus_addr !=
> > > dma_offset)
> > > {
> > > -                     pr_warn("Can't handle multiple dma-ranges with
> > > different
> > > offsets on node(%pOF)\n", node);
> > > -                     /* Don't error out as we'd break some existing DTs
> > > */
> > > -                     continue;
> > > +                     if (!IS_ENABLED(CONFIG_DMA_PFN_OFFSET_MAP)) {
> > > +                             pr_warn("Can't handle multiple dma-ranges
> > > with
> > > different offsets on node(%pOF)\n", node);
> > > +                             pr_warn("Perhaps set
> > > DMA_PFN_OFFSET_MAP=y?\n");
> > > +                             /*
> > > +                              * Don't error out as we'd break some
> > > existing
> > > +                              * DTs that are using configs w/o
> > > +                              * CONFIG_DMA_PFN_OFFSET_MAP set.
> > > +                              */
> > > +                             continue;
> > 
> > dev->bus_dma_limit is set in of_dma_configure(), this function's caller,
> > based
> > on dma_start's value (set after this continue). So you'd be effectively
> > setting
> > the dev->bus_dma_limit to whatever we get from the first dma-range.
> I'm not seeing that at all.  On the  evaluation of each dma-range,
> dma_start and dma_end are re-evaluated to be the lowest and highest
> bus values of the  dma-ranges seen so far.  After all dma-ranges are
> examined,  dev->bus_dma_limit being set to the highest.  In fact, the
> current code -- ie before my commits -- already does this for multiple
> dma-ranges as long as the cpu-bus offset is the same in the
> dma-ranges.

Sorry I got carried away, you're right.

So I understand there is an underlaying assumption that the non DMAble memory
space will always sit on top of the bus memory space, as intertwined as it
might be, so as to every phys_to_dma() call on non DMAble memory to fall above
bus_dma_limit.

Regards,
Nicolas


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets
  2020-05-27 15:43     ` Jim Quinlan via iommu
  2020-05-27 17:01       ` Nicolas Saenz Julienne
@ 2020-05-29 17:34       ` Rob Herring
  2020-05-29 17:51         ` Jim Quinlan via iommu
  1 sibling, 1 reply; 11+ messages in thread
From: Rob Herring @ 2020-05-29 17:34 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: open list:USB SUBSYSTEM,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
	Heikki Krogerus, Srinivas Kandagatla, Frank Rowand,
	Christoph Hellwig, Saravana Kannan, Rafael J. Wysocki,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Alan Stern,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, Corey Minyard,
	Suzuki K Poulose, Dan Williams, Andy Shevchenko,
	Greg Kroah-Hartman, Oliver Neukum, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, Robin Murphy,
	Nicolas Saenz Julienne

On Wed, May 27, 2020 at 9:43 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
>
> Hi Nicolas,
>
> On Wed, May 27, 2020 at 11:00 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> >
> > Hi Jim,
> > one thing comes to mind, there is a small test suite in drivers/of/unittest.c
> > (specifically of_unittest_pci_dma_ranges()) you could extend it to include your
> > use cases.
> Sure, will check out.
> >
> > On Tue, 2020-05-26 at 15:12 -0400, Jim Quinlan wrote:
> > > The new field in struct device 'dma_pfn_offset_map' is used to facilitate
> > > the use of multiple pfn offsets between cpu addrs and dma addrs.  It is
> > > similar to 'dma_pfn_offset' except that the offset chosen depends on the
> > > cpu or dma address involved.
> > >
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > ---
> > >  drivers/of/address.c        | 65 +++++++++++++++++++++++++++++++++++--
> > >  drivers/usb/core/message.c  |  3 ++
> > >  drivers/usb/core/usb.c      |  3 ++
> > >  include/linux/device.h      | 10 +++++-
> > >  include/linux/dma-direct.h  | 10 ++++--
> > >  include/linux/dma-mapping.h | 46 ++++++++++++++++++++++++++
> > >  kernel/dma/Kconfig          | 13 ++++++++
> > >  7 files changed, 144 insertions(+), 6 deletions(-)
> > >
> >
> > [...]
> >
> > > @@ -977,10 +1020,19 @@ int of_dma_get_range(struct device *dev, struct
> > > device_node *np, u64 *dma_addr,
> > >               pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
> > >                        range.bus_addr, range.cpu_addr, range.size);
> > >
> > > +             num_ranges++;
> > >               if (dma_offset && range.cpu_addr - range.bus_addr != dma_offset)
> > > {
> > > -                     pr_warn("Can't handle multiple dma-ranges with different
> > > offsets on node(%pOF)\n", node);
> > > -                     /* Don't error out as we'd break some existing DTs */
> > > -                     continue;
> > > +                     if (!IS_ENABLED(CONFIG_DMA_PFN_OFFSET_MAP)) {
> > > +                             pr_warn("Can't handle multiple dma-ranges with
> > > different offsets on node(%pOF)\n", node);
> > > +                             pr_warn("Perhaps set DMA_PFN_OFFSET_MAP=y?\n");
> > > +                             /*
> > > +                              * Don't error out as we'd break some existing
> > > +                              * DTs that are using configs w/o
> > > +                              * CONFIG_DMA_PFN_OFFSET_MAP set.
> > > +                              */
> > > +                             continue;
> >
> > dev->bus_dma_limit is set in of_dma_configure(), this function's caller, based
> > on dma_start's value (set after this continue). So you'd be effectively setting
> > the dev->bus_dma_limit to whatever we get from the first dma-range.
> I'm not seeing that at all.  On the  evaluation of each dma-range,
> dma_start and dma_end are re-evaluated to be the lowest and highest
> bus values of the  dma-ranges seen so far.  After all dma-ranges are
> examined,  dev->bus_dma_limit being set to the highest.  In fact, the
> current code -- ie before my commits -- already does this for multiple
> dma-ranges as long as the cpu-bus offset is the same in the
> dma-ranges.
> >
> > This can be troublesome depending on how the dma-ranges are setup, for example
> > if the first dma-range doesn't include the CMA area, in arm64 generally set as
> > high as possible in ZONE_DMA32, that would render it useless for
> > dma/{direct/swiotlb}. Again depending on the bus_dma_limit value, if smaller
> > than ZONE_DMA you'd be unable to allocate any DMA memory.
> >
> > IMO, a solution to this calls for a revamp of dma-direct's dma_capable(): match
> > the target DMA memory area with each dma-range we have to see if it fits.
> >
> > > +                     }
> > > +                     dma_multi_pfn_offset = true;
> > >               }
> > >               dma_offset = range.cpu_addr - range.bus_addr;
> > >
> > > @@ -991,6 +1043,13 @@ int of_dma_get_range(struct device *dev, struct
> > > device_node *np, u64 *dma_addr,
> > >                       dma_end = range.bus_addr + range.size;
> > >       }
> > >
> > > +     if (dma_multi_pfn_offset) {
> > > +             dma_offset = 0;
> > > +             ret = attach_dma_pfn_offset_map(dev, node, num_ranges);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > >       if (dma_start >= dma_end) {
> > >               ret = -EINVAL;
> > >               pr_debug("Invalid DMA ranges configuration on node(%pOF)\n",
> > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > > index 6197938dcc2d..aaa3e58f5eb4 100644
> > > --- a/drivers/usb/core/message.c
> > > +++ b/drivers/usb/core/message.c
> > > @@ -1960,6 +1960,9 @@ int usb_set_configuration(struct usb_device *dev, int
> > > configuration)
> > >                */
> > >               intf->dev.dma_mask = dev->dev.dma_mask;
> > >               intf->dev.dma_pfn_offset = dev->dev.dma_pfn_offset;
> > > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > > +             intf->dev.dma_pfn_offset_map = dev->dev.dma_pfn_offset_map;
> > > +#endif
> >
> > Thanks for looking at this, that said, I see more instances of drivers changing
> > dma_pfn_offset outside of the core code. Why not doing this there too?
> >
> > Also, are we 100% sure that dev->dev.dma_pfn_offset isn't going to be freed
> > before we're done using intf->dev? Maybe it's safer to copy the ranges?
> >
> > >               INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
> > >               intf->minor = -1;
> > >               device_initialize(&intf->dev);
> > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > index f16c26dc079d..d2ed4d90e56e 100644
> > > --- a/drivers/usb/core/usb.c
> > > +++ b/drivers/usb/core/usb.c
> > > @@ -612,6 +612,9 @@ struct usb_device *usb_alloc_dev(struct usb_device
> > > *parent,
> > >        */
> > >       dev->dev.dma_mask = bus->sysdev->dma_mask;
> > >       dev->dev.dma_pfn_offset = bus->sysdev->dma_pfn_offset;
> > > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > > +     dev->dev.dma_pfn_offset_map = bus->sysdev->dma_pfn_offset_map;
> > > +#endif
> > >       set_dev_node(&dev->dev, dev_to_node(bus->sysdev));
> > >       dev->state = USB_STATE_ATTACHED;
> > >       dev->lpm_disable_count = 1;
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index ac8e37cd716a..67a240ad4fc5 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -493,6 +493,8 @@ struct dev_links_info {
> > >   * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a smaller
> > >   *           DMA limit than the device itself supports.
> > >   * @dma_pfn_offset: offset of DMA memory range relatively of RAM
> > > + * @dma_pfn_offset_map:      Like dma_pfn_offset but used when there are
> > > multiple
> > > + *           pfn offsets for multiple dma-ranges.
> > >   * @dma_parms:       A low level driver may set these to teach IOMMU code
> > > about
> > >   *           segment limitations.
> > >   * @dma_pools:       Dma pools (if dma'ble device).
> > > @@ -578,7 +580,13 @@ struct device {
> > >                                            allocations such descriptors. */
> > >       u64             bus_dma_limit;  /* upstream dma constraint */
> > >       unsigned long   dma_pfn_offset;
> > > -
> > > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > > +     const struct dma_pfn_offset_region *dma_pfn_offset_map;
> > > +                                     /* Like dma_pfn_offset, but for
> > > +                                      * the unlikely case of multiple
> > > +                                      * offsets. If non-null, dma_pfn_offset
> > > +                                      * will be set to 0. */
> > > +#endif
> >
> > I'm still sad this doesn't fully replace dma_pfn_offset & bus_dma_limit. I feel
> > the extra logic involved in incorporating this as default isn't going to be
> > noticeable as far as performance is concerned to single dma-range users, and
> > it'd make for a nicer DMA code. Also you'd force everyone to test their changes
> > on the multi dma-ranges code path, as opposed to having this disabled 99.9% of
> > the time (hence broken every so often).
> Good point.

+1

> > Note that I sympathize with the amount of work involved on improving that, so
> > better wait to hear what more knowledgeable people have to say about this :)
> Yes, I agree.  I want to avoid coding and testing one solution only to
> have a different reviewer NAK it.

It's a pretty safe bet that everyone will prefer one code path over 2.

Rob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 00/14] PCI: brcmstb: enable PCIe for STB chips
  2020-05-26 19:12 [PATCH v2 00/14] PCI: brcmstb: enable PCIe for STB chips Jim Quinlan via iommu
  2020-05-26 19:12 ` [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets Jim Quinlan via iommu
@ 2020-05-29 17:48 ` Rob Herring
  2020-05-29 17:55   ` Jim Quinlan via iommu
  1 sibling, 1 reply; 11+ messages in thread
From: Rob Herring @ 2020-05-29 17:48 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Ulf Hansson, Heikki Krogerus, linux-pci,
	open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers),
	Julien Grall, Christoph Hellwig, Stefano Stabellini,
	Saravana Kannan, Rafael J. Wysocki, bcm-kernel-feedback-list,
	Alan Stern, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
	Corey Minyard, Suzuki K Poulose, Mark Brown,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Dan Williams, Andy Shevchenko, moderated list:ARM PORT,
	Greg Kroah-Hartman, Oliver Neukum, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, open list:USB SUBSYSTEM,
	Robin Murphy, Nicolas Saenz Julienne

On Tue, May 26, 2020 at 03:12:39PM -0400, Jim Quinlan wrote:
> v2:
> Commit: "device core: Add ability to handle multiple dma offsets"
>   o Added helper func attach_dma_pfn_offset_map() in address.c (Chistoph)
>   o Helpers funcs added to __phys_to_dma() & __dma_to_phys() (Christoph)
>   o Added warning when multiple offsets are needed and !DMA_PFN_OFFSET_MAP
>   o dev->dma_pfn_map => dev->dma_pfn_offset_map
>   o s/frm/from/ for dma_pfn_offset_frm_{phys,dma}_addr() (Christoph)
>   o In device.h: s/const void */const struct dma_pfn_offset_region */
>   o removed 'unlikely' from unlikely(dev->dma_pfn_offset_map) since
>     guarded by CONFIG_DMA_PFN_OFFSET_MAP (Christoph)
>   o Since dev->dma_pfn_offset is copied in usb/core/{usb,message}.c, now
>     dev->dma_pfn_offset_map is copied as well.
>   o Merged two of the DMA commits into one (Christoph).
> 
> Commit "arm: dma-mapping: Invoke dma offset func if needed":
>   o Use helper functions instead of #if CONFIG_DMA_PFN_OFFSET
> 
> Other commits' changes:
>   o Removed need for carrying of_id var in priv (Nicolas)
>   o Commit message rewordings (Bjorn)
>   o Commit log messages filled to 75 chars (Bjorn)
>   o devm_reset_control_get_shared())
>     => devm_reset_control_get_optional_shared (Philipp)
>   o Add call to reset_control_assert() in PCIe remove routines (Philipp)
> 
> v1:
> This patchset expands the usefulness of the Broadcom Settop Box PCIe
> controller by building upon the PCIe driver used currently by the
> Raspbery Pi.  Other forms of this patchset were submitted by me years
> ago and not accepted; the major sticking point was the code required
> for the DMA remapping needed for the PCIe driver to work [1].
> 
> There have been many changes to the DMA and OF subsystems since that
> time, making a cleaner and less intrusive patchset possible.  This
> patchset implements a generalization of "dev->dma_pfn_offset", except
> that instead of a single scalar offset it provides for multiple
> offsets via a function which depends upon the "dma-ranges" property of
> the PCIe host controller.  This is required for proper functionality
> of the BrcmSTB PCIe controller and possibly some other devices.

If you can enable the h/w support without the multiple offset support, 
then I'd split up this series. The latter part might take a bit more 
time.

Rob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets
  2020-05-29 17:34       ` Rob Herring
@ 2020-05-29 17:51         ` Jim Quinlan via iommu
  0 siblings, 0 replies; 11+ messages in thread
From: Jim Quinlan via iommu @ 2020-05-29 17:51 UTC (permalink / raw)
  To: Rob Herring
  Cc: open list:USB SUBSYSTEM,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
	Heikki Krogerus, Srinivas Kandagatla, Frank Rowand,
	Christoph Hellwig, Saravana Kannan, Rafael J. Wysocki,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Alan Stern,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, Corey Minyard,
	Suzuki K Poulose, Dan Williams, Andy Shevchenko,
	Greg Kroah-Hartman, Oliver Neukum, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, Robin Murphy,
	Nicolas Saenz Julienne

On Fri, May 29, 2020 at 1:35 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, May 27, 2020 at 9:43 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> >
> > Hi Nicolas,
> >
> > On Wed, May 27, 2020 at 11:00 AM Nicolas Saenz Julienne
> > <nsaenzjulienne@suse.de> wrote:
> > >
> > > Hi Jim,
> > > one thing comes to mind, there is a small test suite in drivers/of/unittest.c
> > > (specifically of_unittest_pci_dma_ranges()) you could extend it to include your
> > > use cases.
> > Sure, will check out.
> > >
> > > On Tue, 2020-05-26 at 15:12 -0400, Jim Quinlan wrote:
> > > > The new field in struct device 'dma_pfn_offset_map' is used to facilitate
> > > > the use of multiple pfn offsets between cpu addrs and dma addrs.  It is
> > > > similar to 'dma_pfn_offset' except that the offset chosen depends on the
> > > > cpu or dma address involved.
> > > >
> > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > > ---
> > > >  drivers/of/address.c        | 65 +++++++++++++++++++++++++++++++++++--
> > > >  drivers/usb/core/message.c  |  3 ++
> > > >  drivers/usb/core/usb.c      |  3 ++
> > > >  include/linux/device.h      | 10 +++++-
> > > >  include/linux/dma-direct.h  | 10 ++++--
> > > >  include/linux/dma-mapping.h | 46 ++++++++++++++++++++++++++
> > > >  kernel/dma/Kconfig          | 13 ++++++++
> > > >  7 files changed, 144 insertions(+), 6 deletions(-)
> > > >
> > >
> > > [...]
> > >
> > > > @@ -977,10 +1020,19 @@ int of_dma_get_range(struct device *dev, struct
> > > > device_node *np, u64 *dma_addr,
> > > >               pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
> > > >                        range.bus_addr, range.cpu_addr, range.size);
> > > >
> > > > +             num_ranges++;
> > > >               if (dma_offset && range.cpu_addr - range.bus_addr != dma_offset)
> > > > {
> > > > -                     pr_warn("Can't handle multiple dma-ranges with different
> > > > offsets on node(%pOF)\n", node);
> > > > -                     /* Don't error out as we'd break some existing DTs */
> > > > -                     continue;
> > > > +                     if (!IS_ENABLED(CONFIG_DMA_PFN_OFFSET_MAP)) {
> > > > +                             pr_warn("Can't handle multiple dma-ranges with
> > > > different offsets on node(%pOF)\n", node);
> > > > +                             pr_warn("Perhaps set DMA_PFN_OFFSET_MAP=y?\n");
> > > > +                             /*
> > > > +                              * Don't error out as we'd break some existing
> > > > +                              * DTs that are using configs w/o
> > > > +                              * CONFIG_DMA_PFN_OFFSET_MAP set.
> > > > +                              */
> > > > +                             continue;
> > >
> > > dev->bus_dma_limit is set in of_dma_configure(), this function's caller, based
> > > on dma_start's value (set after this continue). So you'd be effectively setting
> > > the dev->bus_dma_limit to whatever we get from the first dma-range.
> > I'm not seeing that at all.  On the  evaluation of each dma-range,
> > dma_start and dma_end are re-evaluated to be the lowest and highest
> > bus values of the  dma-ranges seen so far.  After all dma-ranges are
> > examined,  dev->bus_dma_limit being set to the highest.  In fact, the
> > current code -- ie before my commits -- already does this for multiple
> > dma-ranges as long as the cpu-bus offset is the same in the
> > dma-ranges.
> > >
> > > This can be troublesome depending on how the dma-ranges are setup, for example
> > > if the first dma-range doesn't include the CMA area, in arm64 generally set as
> > > high as possible in ZONE_DMA32, that would render it useless for
> > > dma/{direct/swiotlb}. Again depending on the bus_dma_limit value, if smaller
> > > than ZONE_DMA you'd be unable to allocate any DMA memory.
> > >
> > > IMO, a solution to this calls for a revamp of dma-direct's dma_capable(): match
> > > the target DMA memory area with each dma-range we have to see if it fits.
> > >
> > > > +                     }
> > > > +                     dma_multi_pfn_offset = true;
> > > >               }
> > > >               dma_offset = range.cpu_addr - range.bus_addr;
> > > >
> > > > @@ -991,6 +1043,13 @@ int of_dma_get_range(struct device *dev, struct
> > > > device_node *np, u64 *dma_addr,
> > > >                       dma_end = range.bus_addr + range.size;
> > > >       }
> > > >
> > > > +     if (dma_multi_pfn_offset) {
> > > > +             dma_offset = 0;
> > > > +             ret = attach_dma_pfn_offset_map(dev, node, num_ranges);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > >       if (dma_start >= dma_end) {
> > > >               ret = -EINVAL;
> > > >               pr_debug("Invalid DMA ranges configuration on node(%pOF)\n",
> > > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > > > index 6197938dcc2d..aaa3e58f5eb4 100644
> > > > --- a/drivers/usb/core/message.c
> > > > +++ b/drivers/usb/core/message.c
> > > > @@ -1960,6 +1960,9 @@ int usb_set_configuration(struct usb_device *dev, int
> > > > configuration)
> > > >                */
> > > >               intf->dev.dma_mask = dev->dev.dma_mask;
> > > >               intf->dev.dma_pfn_offset = dev->dev.dma_pfn_offset;
> > > > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > > > +             intf->dev.dma_pfn_offset_map = dev->dev.dma_pfn_offset_map;
> > > > +#endif
> > >
> > > Thanks for looking at this, that said, I see more instances of drivers changing
> > > dma_pfn_offset outside of the core code. Why not doing this there too?
> > >
> > > Also, are we 100% sure that dev->dev.dma_pfn_offset isn't going to be freed
> > > before we're done using intf->dev? Maybe it's safer to copy the ranges?
> > >
> > > >               INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
> > > >               intf->minor = -1;
> > > >               device_initialize(&intf->dev);
> > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > > index f16c26dc079d..d2ed4d90e56e 100644
> > > > --- a/drivers/usb/core/usb.c
> > > > +++ b/drivers/usb/core/usb.c
> > > > @@ -612,6 +612,9 @@ struct usb_device *usb_alloc_dev(struct usb_device
> > > > *parent,
> > > >        */
> > > >       dev->dev.dma_mask = bus->sysdev->dma_mask;
> > > >       dev->dev.dma_pfn_offset = bus->sysdev->dma_pfn_offset;
> > > > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > > > +     dev->dev.dma_pfn_offset_map = bus->sysdev->dma_pfn_offset_map;
> > > > +#endif
> > > >       set_dev_node(&dev->dev, dev_to_node(bus->sysdev));
> > > >       dev->state = USB_STATE_ATTACHED;
> > > >       dev->lpm_disable_count = 1;
> > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > index ac8e37cd716a..67a240ad4fc5 100644
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -493,6 +493,8 @@ struct dev_links_info {
> > > >   * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a smaller
> > > >   *           DMA limit than the device itself supports.
> > > >   * @dma_pfn_offset: offset of DMA memory range relatively of RAM
> > > > + * @dma_pfn_offset_map:      Like dma_pfn_offset but used when there are
> > > > multiple
> > > > + *           pfn offsets for multiple dma-ranges.
> > > >   * @dma_parms:       A low level driver may set these to teach IOMMU code
> > > > about
> > > >   *           segment limitations.
> > > >   * @dma_pools:       Dma pools (if dma'ble device).
> > > > @@ -578,7 +580,13 @@ struct device {
> > > >                                            allocations such descriptors. */
> > > >       u64             bus_dma_limit;  /* upstream dma constraint */
> > > >       unsigned long   dma_pfn_offset;
> > > > -
> > > > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > > > +     const struct dma_pfn_offset_region *dma_pfn_offset_map;
> > > > +                                     /* Like dma_pfn_offset, but for
> > > > +                                      * the unlikely case of multiple
> > > > +                                      * offsets. If non-null, dma_pfn_offset
> > > > +                                      * will be set to 0. */
> > > > +#endif
> > >
> > > I'm still sad this doesn't fully replace dma_pfn_offset & bus_dma_limit. I feel
> > > the extra logic involved in incorporating this as default isn't going to be
> > > noticeable as far as performance is concerned to single dma-range users, and
> > > it'd make for a nicer DMA code. Also you'd force everyone to test their changes
> > > on the multi dma-ranges code path, as opposed to having this disabled 99.9% of
> > > the time (hence broken every so often).
> > Good point.
>
> +1
>
> > > Note that I sympathize with the amount of work involved on improving that, so
> > > better wait to hear what more knowledgeable people have to say about this :)
> > Yes, I agree.  I want to avoid coding and testing one solution only to
> > have a different reviewer NAK it.
>
> It's a pretty safe bet that everyone will prefer one code path over 2.
>
> Rob
Thanks for  the input.  Will do, and send out v3 ASAP.
Thanks, Jim
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 00/14] PCI: brcmstb: enable PCIe for STB chips
  2020-05-29 17:48 ` [PATCH v2 00/14] PCI: brcmstb: enable PCIe for STB chips Rob Herring
@ 2020-05-29 17:55   ` Jim Quinlan via iommu
  0 siblings, 0 replies; 11+ messages in thread
From: Jim Quinlan via iommu @ 2020-05-29 17:55 UTC (permalink / raw)
  To: Rob Herring
  Cc: Ulf Hansson, Heikki Krogerus,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
	open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers),
	Julien Grall, Christoph Hellwig, Stefano Stabellini,
	Saravana Kannan, Rafael J. Wysocki,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Alan Stern,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE, Corey Minyard,
	Suzuki K Poulose, Mark Brown,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Dan Williams, Andy Shevchenko, moderated list:ARM PORT,
	Greg Kroah-Hartman, Oliver Neukum, open list, Wolfram Sang,
	open list:DMA MAPPING HELPERS, open list:USB SUBSYSTEM,
	Robin Murphy, Nicolas Saenz Julienne

On Fri, May 29, 2020 at 1:49 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, May 26, 2020 at 03:12:39PM -0400, Jim Quinlan wrote:
> > v2:
> > Commit: "device core: Add ability to handle multiple dma offsets"
> >   o Added helper func attach_dma_pfn_offset_map() in address.c (Chistoph)
> >   o Helpers funcs added to __phys_to_dma() & __dma_to_phys() (Christoph)
> >   o Added warning when multiple offsets are needed and !DMA_PFN_OFFSET_MAP
> >   o dev->dma_pfn_map => dev->dma_pfn_offset_map
> >   o s/frm/from/ for dma_pfn_offset_frm_{phys,dma}_addr() (Christoph)
> >   o In device.h: s/const void */const struct dma_pfn_offset_region */
> >   o removed 'unlikely' from unlikely(dev->dma_pfn_offset_map) since
> >     guarded by CONFIG_DMA_PFN_OFFSET_MAP (Christoph)
> >   o Since dev->dma_pfn_offset is copied in usb/core/{usb,message}.c, now
> >     dev->dma_pfn_offset_map is copied as well.
> >   o Merged two of the DMA commits into one (Christoph).
> >
> > Commit "arm: dma-mapping: Invoke dma offset func if needed":
> >   o Use helper functions instead of #if CONFIG_DMA_PFN_OFFSET
> >
> > Other commits' changes:
> >   o Removed need for carrying of_id var in priv (Nicolas)
> >   o Commit message rewordings (Bjorn)
> >   o Commit log messages filled to 75 chars (Bjorn)
> >   o devm_reset_control_get_shared())
> >     => devm_reset_control_get_optional_shared (Philipp)
> >   o Add call to reset_control_assert() in PCIe remove routines (Philipp)
> >
> > v1:
> > This patchset expands the usefulness of the Broadcom Settop Box PCIe
> > controller by building upon the PCIe driver used currently by the
> > Raspbery Pi.  Other forms of this patchset were submitted by me years
> > ago and not accepted; the major sticking point was the code required
> > for the DMA remapping needed for the PCIe driver to work [1].
> >
> > There have been many changes to the DMA and OF subsystems since that
> > time, making a cleaner and less intrusive patchset possible.  This
> > patchset implements a generalization of "dev->dma_pfn_offset", except
> > that instead of a single scalar offset it provides for multiple
> > offsets via a function which depends upon the "dma-ranges" property of
> > the PCIe host controller.  This is required for proper functionality
> > of the BrcmSTB PCIe controller and possibly some other devices.
>
> If you can enable the h/w support without the multiple offset support,
> then I'd split up this series. The latter part might take a bit more
> time.
>
> Rob
Unfortunately, the STB PCIe  controller depends on the multiple PFN
offset functionality.
Thanks,
Jim
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-05-29 17:55 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-26 19:12 [PATCH v2 00/14] PCI: brcmstb: enable PCIe for STB chips Jim Quinlan via iommu
2020-05-26 19:12 ` [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets Jim Quinlan via iommu
2020-05-26 20:54   ` Andy Shevchenko
2020-05-26 22:01     ` Jim Quinlan via iommu
2020-05-27 15:00   ` Nicolas Saenz Julienne
2020-05-27 15:43     ` Jim Quinlan via iommu
2020-05-27 17:01       ` Nicolas Saenz Julienne
2020-05-29 17:34       ` Rob Herring
2020-05-29 17:51         ` Jim Quinlan via iommu
2020-05-29 17:48 ` [PATCH v2 00/14] PCI: brcmstb: enable PCIe for STB chips Rob Herring
2020-05-29 17:55   ` Jim Quinlan via iommu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).