[PATCH 0/8] Cache-coherent DMA access using UIO

[PATCH 0/8] Cache-coherent DMA access using UIO

[Anup Patel]

The goal of this patchset is to improve UIO framework and UIO dmem
driver to allow cache-coherent DMA accesses from user-space.

This patchset is based on two previous patchsets:
1) [PATCH v5 0/6] UIO driver for APM X-Gene QMTM
(Refer, http://www.spinics.net/lists/devicetree/msg58244.html)
2) [PATCH 0/4] Fix and extend uio_dmem_genirq
(Refer, https://lkml.org/lkml/2016/5/17/141)

We have adopted only patch0-3 of patchset1 which was abandoned
long time back. We have taken care of last few unaddressed comments
on these patches.

The patchset2 is quite recent has been adopted entirely. We have
taken care review comments on these patches too.

This patchset is based on v4.7-rc7 tag and it is available in uio-v1
branch of https://github.com/Broadcom/arm64-linux.git

Ankit Jindal (3):
  uio: code style cleanup
  uio: Add new UIO_MEM_PHYS_CACHE type for mem regions
  Documentation: Update documentation for UIO_MEM_PHYS_CACHE and
    UIO_MEM_DEVICE

Anup Patel (2):
  uio: Add new UIO_MEM_DEVICE type for mem regions
  uio: Use new memtypes in uio_dmem_genirq

Jan Viktorin (3):
  uio: fix dmem_region_start computation
  uio: UIO_IRQ_NONE is a valid option for uioinfo->irq
  uio: bind uio_dmem_genirq via OF

 Documentation/DocBook/uio-howto.tmpl |   6 +-
 drivers/uio/uio.c                    |  32 +++++---
 drivers/uio/uio_dmem_genirq.c        | 141 +++++++++++++++++++++++++----------
 include/linux/uio_driver.h           |  10 ++-
 4 files changed, 136 insertions(+), 53 deletions(-)

-- 
1.9.1

[PATCH 1/8] uio: code style cleanup

[Anup Patel]

From: Ankit Jindal <thatsjindal@gmail.com>

This patch fixes the indentation of switch-case block in uio driver.

Signed-off-by: Ankit Jindal <thatsjindal@gmail.com>
Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/uio/uio.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index fba021f..f2729b7 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -703,13 +703,13 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	}
 
 	switch (idev->info->mem[mi].memtype) {
-		case UIO_MEM_PHYS:
-			return uio_mmap_physical(vma);
-		case UIO_MEM_LOGICAL:
-		case UIO_MEM_VIRTUAL:
-			return uio_mmap_logical(vma);
-		default:
-			return -EINVAL;
+	case UIO_MEM_PHYS:
+		return uio_mmap_physical(vma);
+	case UIO_MEM_LOGICAL:
+	case UIO_MEM_VIRTUAL:
+		return uio_mmap_logical(vma);
+	default:
+		return -EINVAL;
 	}
 }
 
-- 
1.9.1

[PATCH 2/8] uio: Add new UIO_MEM_PHYS_CACHE type for mem regions

[Anup Patel]

From: Ankit Jindal <thatsjindal@gmail.com>

Currently, three types of mem regions are supported: UIO_MEM_PHYS,
UIO_MEM_LOGICAL and UIO_MEM_VIRTUAL. Among these UIO_MEM_PHYS helps
UIO driver export physcial memory to user space as non-cacheable
user memory. Typcially memory-mapped registers of a device are exported
to user space as UIO_MEM_PHYS type mem region. The UIO_MEM_PHYS type
is not efficient if dma-capable devices are capable of maintaining
coherency with CPU caches.

This patch adds new type UIO_MEM_PHYS_CACHE for mem regions to enable
cacheable access to physical memory from user space.

Signed-off-by: Ankit Jindal <thatsjindal@gmail.com>
Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/uio/uio.c          | 16 +++++++++++++---
 include/linux/uio_driver.h |  9 +++++----
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index f2729b7..0e53076 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -641,7 +641,7 @@ static const struct vm_operations_struct uio_physical_vm_ops = {
 #endif
 };
 
-static int uio_mmap_physical(struct vm_area_struct *vma)
+static int uio_mmap_physical(struct vm_area_struct *vma, int memtype)
 {
 	struct uio_device *idev = vma->vm_private_data;
 	int mi = uio_find_mem_index(vma);
@@ -656,7 +656,16 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
 		return -EINVAL;
 
 	vma->vm_ops = &uio_physical_vm_ops;
-	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	switch (memtype) {
+	case UIO_MEM_PHYS:
+		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+		break;
+	case UIO_MEM_PHYS_CACHE:
+		/* Do nothing. */
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	/*
 	 * We cannot use the vm_iomap_memory() helper here,
@@ -704,7 +713,8 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 
 	switch (idev->info->mem[mi].memtype) {
 	case UIO_MEM_PHYS:
-		return uio_mmap_physical(vma);
+	case UIO_MEM_PHYS_CACHE:
+		return uio_mmap_physical(vma, idev->info->mem[mi].memtype);
 	case UIO_MEM_LOGICAL:
 	case UIO_MEM_VIRTUAL:
 		return uio_mmap_logical(vma);
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 32c0e83..31359aee 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -124,10 +124,11 @@ extern void uio_event_notify(struct uio_info *info);
 #define UIO_IRQ_NONE	0
 
 /* defines for uio_mem->memtype */
-#define UIO_MEM_NONE	0
-#define UIO_MEM_PHYS	1
-#define UIO_MEM_LOGICAL	2
-#define UIO_MEM_VIRTUAL 3
+#define UIO_MEM_NONE		0
+#define UIO_MEM_PHYS		1
+#define UIO_MEM_LOGICAL		2
+#define UIO_MEM_VIRTUAL		3
+#define UIO_MEM_PHYS_CACHE	4
 
 /* defines for uio_port->porttype */
 #define UIO_PORT_NONE	0
-- 
1.9.1

[PATCH 3/8] uio: Add new UIO_MEM_DEVICE type for mem regions

[Anup Patel]

On ARM64, the MMU supports special memory attributes for device
memory/registers. Due to this we have pgprot_device() provided
by asm/pgtable.h of arch/arm64.

On architectures that do not have special MMU attribute for device
memory/registers, the asm-generic/pgtable.h maps pgprot_device()
to pgprot_noncached().

This patch introduces a new UIO mem region type UIO_MEM_DEVICE to
represent device registers/memory. The UIO device drivers should
prefer this new UIO mem region type for memory mapped device registers.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/uio/uio.c          | 4 ++++
 include/linux/uio_driver.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 0e53076..a00990c 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -663,6 +663,9 @@ static int uio_mmap_physical(struct vm_area_struct *vma, int memtype)
 	case UIO_MEM_PHYS_CACHE:
 		/* Do nothing. */
 		break;
+	case UIO_MEM_DEVICE:
+		vma->vm_page_prot = pgprot_device(vma->vm_page_prot);
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -714,6 +717,7 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	switch (idev->info->mem[mi].memtype) {
 	case UIO_MEM_PHYS:
 	case UIO_MEM_PHYS_CACHE:
+	case UIO_MEM_DEVICE:
 		return uio_mmap_physical(vma, idev->info->mem[mi].memtype);
 	case UIO_MEM_LOGICAL:
 	case UIO_MEM_VIRTUAL:
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 31359aee..7349f95 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -129,6 +129,7 @@ extern void uio_event_notify(struct uio_info *info);
 #define UIO_MEM_LOGICAL		2
 #define UIO_MEM_VIRTUAL		3
 #define UIO_MEM_PHYS_CACHE	4
+#define UIO_MEM_DEVICE		5
 
 /* defines for uio_port->porttype */
 #define UIO_PORT_NONE	0
-- 
1.9.1

[PATCH 4/8] Documentation: Update documentation for UIO_MEM_PHYS_CACHE and UIO_MEM_DEVICE

[Anup Patel]

From: Ankit Jindal <thatsjindal@gmail.com>

This patch updates UIO documentation for new mem region
types UIO_MEM_PHYS_CACHE and UIO_MEM_DEVICE.

Signed-off-by: Ankit Jindal <thatsjindal@gmail.com>
Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 Documentation/DocBook/uio-howto.tmpl | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
index cd0e452..de9dafe 100644
--- a/Documentation/DocBook/uio-howto.tmpl
+++ b/Documentation/DocBook/uio-howto.tmpl
@@ -529,8 +529,10 @@ the memory region, it will show up in the corresponding sysfs node.
 <varname>int memtype</varname>: Required if the mapping is used. Set this to
 <varname>UIO_MEM_PHYS</varname> if you you have physical memory on your
 card to be mapped. Use <varname>UIO_MEM_LOGICAL</varname> for logical
-memory (e.g. allocated with <function>kmalloc()</function>). There's also
-<varname>UIO_MEM_VIRTUAL</varname> for virtual memory.
+memory (e.g. allocated with <function>kmalloc()</function>). There are also
+<varname>UIO_MEM_VIRTUAL</varname> for virtual memory,
+<varname>UIO_MEM_PHYS_CACHE</varname> for cacheable physical memory and,
+<varname>UIO_MEM_DEVICE</varname> for memory mapped device registers.
 </para></listitem>
 
 <listitem><para>
-- 
1.9.1

[PATCH 5/8] uio: fix dmem_region_start computation

[Anup Patel]

From: Jan Viktorin <viktorin@rehivetech.com>

The variable i contains a total number of resources (including
IORESOURCE_IRQ). However, we want the dmem_region_start to point
after the last resource of type IORESOURCE_MEM. The original behaviour
leads (very likely) to skipping several UIO mapping regions and makes
them useless. Fix this by computing dmem_region_start from the uiomem
which points to the last used UIO mapping.

Fixes: 0a0c3b5a24bd ("Add new uio device for dynamic memory allocation")

Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
---
 drivers/uio/uio_dmem_genirq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
index 915facb..e1134a4 100644
--- a/drivers/uio/uio_dmem_genirq.c
+++ b/drivers/uio/uio_dmem_genirq.c
@@ -229,7 +229,7 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
 		++uiomem;
 	}
 
-	priv->dmem_region_start = i;
+	priv->dmem_region_start = uiomem - &uioinfo->mem[0];
 	priv->num_dmem_regions = pdata->num_dynamic_regions;
 
 	for (i = 0; i < pdata->num_dynamic_regions; ++i) {
-- 
1.9.1

[PATCH 6/8] uio: UIO_IRQ_NONE is a valid option for uioinfo->irq

[Anup Patel]

From: Jan Viktorin <viktorin@rehivetech.com>

We can simplify handling of platform_get_irq into one place as it is
acceptable to see UIO_IRQ_NONE instead of a valid IRQ number. Some
devices don't have or don't need any interrupt to be handled. The
same change has been already done for uio_pdrv_genirq.

Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
---
 drivers/uio/uio_dmem_genirq.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
index e1134a4..a4d6d81 100644
--- a/drivers/uio/uio_dmem_genirq.c
+++ b/drivers/uio/uio_dmem_genirq.c
@@ -154,8 +154,6 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
 	int i;
 
 	if (pdev->dev.of_node) {
-		int irq;
-
 		/* alloc uioinfo for one device */
 		uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
 		if (!uioinfo) {
@@ -165,13 +163,6 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
 		}
 		uioinfo->name = pdev->dev.of_node->name;
 		uioinfo->version = "devicetree";
-
-		/* Multiple IRQs are not supported */
-		irq = platform_get_irq(pdev, 0);
-		if (irq == -ENXIO)
-			uioinfo->irq = UIO_IRQ_NONE;
-		else
-			uioinfo->irq = irq;
 	}
 
 	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
@@ -200,14 +191,18 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
 	priv->pdev = pdev;
 	mutex_init(&priv->alloc_lock);
 
+	/* Multiple IRQs are not supported */
 	if (!uioinfo->irq) {
 		ret = platform_get_irq(pdev, 0);
-		if (ret < 0) {
+		uioinfo->irq = ret;
+		if (ret == -ENXIO && pdev->dev.of_node)
+			uioinfo->irq = UIO_IRQ_NONE;
+		else if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get IRQ\n");
 			goto bad1;
 		}
-		uioinfo->irq = ret;
 	}
+
 	uiomem = &uioinfo->mem[0];
 
 	for (i = 0; i < pdev->num_resources; ++i) {
-- 
1.9.1

[PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Anup Patel]

From: Jan Viktorin <viktorin@rehivetech.com>

The uio_dmem_genirq works in a similar ways as uio_pdrv_genirq now.

It accepts the of_id module parameter to specify UIO compatible
string as module parameter. There are few other module parameters
to specify DT property name for number bits in DMA mask and details
about dynamic regions.

Following are the newly added module parameters:
1) of_id: The UIO compatible string to be used for DT probing
2) of_dma_bits_prot: This is name of OF property which will be
used to specify number of DMA mask bits in UIO DT node.
3) of_dmem_count_prop: This is name of OF property which will be
used to specify number of dynamic regions in UIO DT node.
4) of_dmem_sizes_prop: This is name of OF property which will be
used to specify sizes of each dynamic regions in UIO DT node.

Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/uio/uio_dmem_genirq.c | 113 +++++++++++++++++++++++++++++++++---------
 1 file changed, 89 insertions(+), 24 deletions(-)

diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
index a4d6d81..0a0cc19 100644
--- a/drivers/uio/uio_dmem_genirq.c
+++ b/drivers/uio/uio_dmem_genirq.c
@@ -144,46 +144,107 @@ static int uio_dmem_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
 	return 0;
 }
 
+static char uio_of_dma_bits_prop[128] = "uio,dma-bits";
+static char uio_of_dmem_count_prop[128] = "uio,number-of-dynamic-regions";
+static char uio_of_dmem_sizes_prop[128] = "uio,dynamic-regions-sizes";
+
+static int uio_dmem_genirq_alloc_platdata(struct platform_device *pdev)
+{
+	struct uio_dmem_genirq_pdata pdata;
+	u32 dma_bits, regions;
+	u32 sizes[MAX_UIO_MAPS];
+	int ret;
+
+	memset(&pdata, 0, sizeof(pdata));
+
+	ret = of_property_read_u32(pdev->dev.of_node,
+				   uio_of_dma_bits_prop, &dma_bits);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Missing property %s\n", uio_of_dma_bits_prop);
+		return ret;
+	}
+	if (dma_bits > 64)
+		dma_bits = 64;
+
+	dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(dma_bits));
+
+	ret = of_property_read_u32(pdev->dev.of_node,
+				   uio_of_dmem_count_prop, &regions);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Missing property %s\n", uio_of_dmem_count_prop);
+		return ret;
+	}
+
+	regions = min_t(u32, regions, MAX_UIO_MAPS);
+
+	ret = of_property_read_u32_array(pdev->dev.of_node,
+					 uio_of_dmem_sizes_prop, sizes,
+					 regions);
+	if (ret) {
+		dev_err(&pdev->dev, "Missing or invalid property %s\n",
+			uio_of_dmem_sizes_prop);
+		return ret;
+	}
+
+	pdata.dynamic_region_sizes = devm_kzalloc(&pdev->dev,
+			sizeof(*pdata.dynamic_region_sizes) *
+			pdata.num_dynamic_regions, GFP_KERNEL);
+	if (!pdata.dynamic_region_sizes) {
+		dev_err(&pdev->dev, "Unable to alloc dynamic_region_sizes\n");
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	pdata.num_dynamic_regions = regions;
+	while (regions--)
+		pdata.dynamic_region_sizes[regions] = sizes[regions];
+
+	pdata.uioinfo.name = pdev->dev.of_node->name;
+	pdata.uioinfo.version = "devicetree";
+
+	return platform_device_add_data(pdev, &pdata, sizeof(pdata));
+}
+
 static int uio_dmem_genirq_probe(struct platform_device *pdev)
 {
-	struct uio_dmem_genirq_pdata *pdata = dev_get_platdata(&pdev->dev);
-	struct uio_info *uioinfo = &pdata->uioinfo;
+	struct uio_dmem_genirq_pdata *pdata;
+	struct uio_info *uioinfo;
 	struct uio_dmem_genirq_platdata *priv;
 	struct uio_mem *uiomem;
 	int ret = -EINVAL;
 	int i;
 
 	if (pdev->dev.of_node) {
-		/* alloc uioinfo for one device */
-		uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
-		if (!uioinfo) {
-			ret = -ENOMEM;
-			dev_err(&pdev->dev, "unable to kmalloc\n");
+		ret = uio_dmem_genirq_alloc_platdata(pdev);
+		if (ret)
 			goto bad2;
-		}
-		uioinfo->name = pdev->dev.of_node->name;
-		uioinfo->version = "devicetree";
 	}
 
+	pdata = dev_get_platdata(&pdev->dev);
+	uioinfo = &pdata->uioinfo;
+
 	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
 		dev_err(&pdev->dev, "missing platform_data\n");
-		goto bad0;
+		goto bad2;
 	}
 
 	if (uioinfo->handler || uioinfo->irqcontrol ||
 	    uioinfo->irq_flags & IRQF_SHARED) {
 		dev_err(&pdev->dev, "interrupt configuration error\n");
-		goto bad0;
+		goto bad2;
 	}
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
 		ret = -ENOMEM;
 		dev_err(&pdev->dev, "unable to kmalloc\n");
-		goto bad0;
+		goto bad2;
 	}
 
-	dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
+	if (!pdev->dev.of_node)
+		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
 
 	priv->uioinfo = uioinfo;
 	spin_lock_init(&priv->lock);
@@ -278,10 +339,6 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
 	return 0;
  bad1:
 	kfree(priv);
- bad0:
-	/* kfree uioinfo for OF */
-	if (pdev->dev.of_node)
-		kfree(uioinfo);
  bad2:
 	return ret;
 }
@@ -296,10 +353,6 @@ static int uio_dmem_genirq_remove(struct platform_device *pdev)
 	priv->uioinfo->handler = NULL;
 	priv->uioinfo->irqcontrol = NULL;
 
-	/* kfree uioinfo for OF */
-	if (pdev->dev.of_node)
-		kfree(priv->uioinfo);
-
 	kfree(priv);
 	return 0;
 }
@@ -327,10 +380,22 @@ static const struct dev_pm_ops uio_dmem_genirq_dev_pm_ops = {
 };
 
 #ifdef CONFIG_OF
-static const struct of_device_id uio_of_genirq_match[] = {
-	{ /* empty for now */ },
+static struct of_device_id uio_of_genirq_match[] = {
+	{ /* This is filled with module_parm */ },
+	{ /* end of list */ },
 };
 MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
+module_param_string(of_id, uio_of_genirq_match[0].compatible, 128, 0);
+MODULE_PARM_DESC(of_id, "Openfirmware id of the device to be handled by uio");
+module_param_string(of_dma_bits_prop, uio_of_dma_bits_prop, 128, 0);
+MODULE_PARM_DESC(of_dma_bits_prop,
+		 "Openfirmware property for number of bits in DMA mask");
+module_param_string(of_dmem_count_prop, uio_of_dmem_count_prop, 128, 0);
+MODULE_PARM_DESC(of_dmem_count_prop,
+		 "Openfirmware property for dynamic region count");
+module_param_string(of_dmem_sizes_prop, uio_of_dmem_sizes_prop, 128, 0);
+MODULE_PARM_DESC(of_dmem_sizes_prop,
+		 "Openfirmware property for dynamic region sizes");
 #endif
 
 static struct platform_driver uio_dmem_genirq = {
-- 
1.9.1

[PATCH 8/8] uio: Use new memtypes in uio_dmem_genirq

[Anup Patel]

This patch extends uio_dmem_genirq driver to use recently
added memtypes as follows:
1. Use UIO_MEM_DEVICE memtype for MEM resources
2. Use UIO_MEM_PHYS_CACHE memtype for dynamic regions
when UIO DT node is marked as DMA coherent.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/uio/uio_dmem_genirq.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
index 0a0cc19..4bd6f31 100644
--- a/drivers/uio/uio_dmem_genirq.c
+++ b/drivers/uio/uio_dmem_genirq.c
@@ -279,7 +279,7 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
 			break;
 		}
 
-		uiomem->memtype = UIO_MEM_PHYS;
+		uiomem->memtype = UIO_MEM_DEVICE;
 		uiomem->addr = r->start;
 		uiomem->size = resource_size(r);
 		++uiomem;
@@ -295,7 +295,12 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
 					" dynamic and fixed memory regions.\n");
 			break;
 		}
-		uiomem->memtype = UIO_MEM_PHYS;
+
+		if (pdev->dev.of_node &&
+		    of_dma_is_coherent(pdev->dev.of_node))
+			uiomem->memtype = UIO_MEM_PHYS_CACHE;
+		else
+			uiomem->memtype = UIO_MEM_PHYS;
 		uiomem->addr = DMEM_MAP_ERROR;
 		uiomem->size = pdata->dynamic_region_sizes[i];
 		++uiomem;
-- 
1.9.1

[PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Russell King - ARM Linux]

On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
> +static int uio_dmem_genirq_alloc_platdata(struct platform_device *pdev)
> +{
> +	struct uio_dmem_genirq_pdata pdata;
> +	u32 dma_bits, regions;
> +	u32 sizes[MAX_UIO_MAPS];
> +	int ret;
> +
> +	memset(&pdata, 0, sizeof(pdata));
> +
> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   uio_of_dma_bits_prop, &dma_bits);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Missing property %s\n", uio_of_dma_bits_prop);
> +		return ret;
> +	}
> +	if (dma_bits > 64)
> +		dma_bits = 64;
> +
> +	dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(dma_bits));

You really need to check the return value from this: this function
negotiates with the architecture, and if 64-bit DMA is not supported,
then the call will fail and you as a driver are expected to fall back
to 32-bit DMA only.

In that case, you're expected to call the same function with a 32-bit
mask, and if that fails, you're supposed to then decide that DMA is
not possible.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Anup Patel]

On Fri, Jul 15, 2016 at 3:15 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
>> +static int uio_dmem_genirq_alloc_platdata(struct platform_device *pdev)
>> +{
>> +     struct uio_dmem_genirq_pdata pdata;
>> +     u32 dma_bits, regions;
>> +     u32 sizes[MAX_UIO_MAPS];
>> +     int ret;
>> +
>> +     memset(&pdata, 0, sizeof(pdata));
>> +
>> +     ret = of_property_read_u32(pdev->dev.of_node,
>> +                                uio_of_dma_bits_prop, &dma_bits);
>> +     if (ret) {
>> +             dev_err(&pdev->dev,
>> +                     "Missing property %s\n", uio_of_dma_bits_prop);
>> +             return ret;
>> +     }
>> +     if (dma_bits > 64)
>> +             dma_bits = 64;
>> +
>> +     dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(dma_bits));
>
> You really need to check the return value from this: this function
> negotiates with the architecture, and if 64-bit DMA is not supported,
> then the call will fail and you as a driver are expected to fall back
> to 32-bit DMA only.
>
> In that case, you're expected to call the same function with a 32-bit
> mask, and if that fails, you're supposed to then decide that DMA is
> not possible.
>

Thanks for pointing out.

I will fix this as-per your suggestion in next revision.

Regards,
Anup

[PATCH 5/8] uio: fix dmem_region_start computation

[Greg Kroah-Hartman]

On Fri, Jul 15, 2016 at 02:34:00PM +0530, Anup Patel wrote:
> From: Jan Viktorin <viktorin@rehivetech.com>
> 
> The variable i contains a total number of resources (including
> IORESOURCE_IRQ). However, we want the dmem_region_start to point
> after the last resource of type IORESOURCE_MEM. The original behaviour
> leads (very likely) to skipping several UIO mapping regions and makes
> them useless. Fix this by computing dmem_region_start from the uiomem
> which points to the last used UIO mapping.
> 
> Fixes: 0a0c3b5a24bd ("Add new uio device for dynamic memory allocation")
> 
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> ---
>  drivers/uio/uio_dmem_genirq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Why isn't this patch first in the series, with a stable marking, and
your signed off on it (you are forwarding it on to me, so you need to
add your mark to it as well, I can't take it otherwise.)

> diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
> index 915facb..e1134a4 100644
> --- a/drivers/uio/uio_dmem_genirq.c
> +++ b/drivers/uio/uio_dmem_genirq.c
> @@ -229,7 +229,7 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
>  		++uiomem;
>  	}
>  
> -	priv->dmem_region_start = i;
> +	priv->dmem_region_start = uiomem - &uioinfo->mem[0];

Are you sure about this?  It doesn't look correct at first glance, I'm
loath to take this without a bunch of testing.  Were you able to test
this out to verify it doesn't break working hardware?

thanks,

greg k-h

[PATCH 5/8] uio: fix dmem_region_start computation

[Jan Viktorin]

On Fri, 15 Jul 2016 20:32:24 +0900
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Fri, Jul 15, 2016 at 02:34:00PM +0530, Anup Patel wrote:
> > From: Jan Viktorin <viktorin@rehivetech.com>
> > 
> > The variable i contains a total number of resources (including
> > IORESOURCE_IRQ). However, we want the dmem_region_start to point
> > after the last resource of type IORESOURCE_MEM. The original behaviour
> > leads (very likely) to skipping several UIO mapping regions and makes
> > them useless. Fix this by computing dmem_region_start from the uiomem
> > which points to the last used UIO mapping.
> > 
> > Fixes: 0a0c3b5a24bd ("Add new uio device for dynamic memory allocation")
> > 
> > Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> > ---
> >  drivers/uio/uio_dmem_genirq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)  
> 
> Why isn't this patch first in the series, with a stable marking, and
> your signed off on it (you are forwarding it on to me, so you need to
> add your mark to it as well, I can't take it otherwise.)
> 
> > diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
> > index 915facb..e1134a4 100644
> > --- a/drivers/uio/uio_dmem_genirq.c
> > +++ b/drivers/uio/uio_dmem_genirq.c
> > @@ -229,7 +229,7 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
> >  		++uiomem;
> >  	}
> >  
> > -	priv->dmem_region_start = i;
> > +	priv->dmem_region_start = uiomem - &uioinfo->mem[0];  
> 
> Are you sure about this?  It doesn't look correct at first glance, I'm

The loop over resources counts all resources in _i_ and mem resources in _uiomem_
pointer. Thus, the dmem_region_start cannot be derived from _i_ but from _uiomem_.

> loath to take this without a bunch of testing.  Were you able to test
> this out to verify it doesn't break working hardware?

This is a good point, however, what is the working hardware? I could not find
any application of the uio_dmem_genirq anywhere online. Any example, nothing.

I am afraid that nobody is using more then 1 memory resource with this driver
and so nobody could discover this to be a bug. I was working with more then 2
resources.

I'd be glad if somebody else can test it on any other working setup.

Regards
Jan

> 
> thanks,
> 
> greg k-h

[PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Mark Rutland]

[adding devicetree list]

On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
> From: Jan Viktorin <viktorin@rehivetech.com>
> 
> The uio_dmem_genirq works in a similar ways as uio_pdrv_genirq now.
> 
> It accepts the of_id module parameter to specify UIO compatible
> string as module parameter. There are few other module parameters
> to specify DT property name for number bits in DMA mask and details
> about dynamic regions.
> 
> Following are the newly added module parameters:
> 1) of_id: The UIO compatible string to be used for DT probing
> 2) of_dma_bits_prot: This is name of OF property which will be
> used to specify number of DMA mask bits in UIO DT node.
> 3) of_dmem_count_prop: This is name of OF property which will be
> used to specify number of dynamic regions in UIO DT node.
> 4) of_dmem_sizes_prop: This is name of OF property which will be
> used to specify sizes of each dynamic regions in UIO DT node.
> 
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> ---
>  drivers/uio/uio_dmem_genirq.c | 113 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 89 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
> index a4d6d81..0a0cc19 100644
> --- a/drivers/uio/uio_dmem_genirq.c
> +++ b/drivers/uio/uio_dmem_genirq.c
> @@ -144,46 +144,107 @@ static int uio_dmem_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>  	return 0;
>  }
>  
> +static char uio_of_dma_bits_prop[128] = "uio,dma-bits";
> +static char uio_of_dmem_count_prop[128] = "uio,number-of-dynamic-regions";
> +static char uio_of_dmem_sizes_prop[128] = "uio,dynamic-regions-sizes";

What are these proeprties? What is a "dynamic region" in hardware terms?

Why must they be in the DT, and why are the *property names* arbitrarily
overridable as module parameters? What exactly do you expect there to be
in a DT?

DT bindings need documentation, but there was none as part of this series.

I do not think this makes sense from a DT perspective.

Thanks,
Mark.

> +
> +static int uio_dmem_genirq_alloc_platdata(struct platform_device *pdev)
> +{
> +	struct uio_dmem_genirq_pdata pdata;
> +	u32 dma_bits, regions;
> +	u32 sizes[MAX_UIO_MAPS];
> +	int ret;
> +
> +	memset(&pdata, 0, sizeof(pdata));
> +
> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   uio_of_dma_bits_prop, &dma_bits);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Missing property %s\n", uio_of_dma_bits_prop);
> +		return ret;
> +	}
> +	if (dma_bits > 64)
> +		dma_bits = 64;
> +
> +	dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(dma_bits));
> +
> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   uio_of_dmem_count_prop, &regions);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Missing property %s\n", uio_of_dmem_count_prop);
> +		return ret;
> +	}
> +
> +	regions = min_t(u32, regions, MAX_UIO_MAPS);
> +
> +	ret = of_property_read_u32_array(pdev->dev.of_node,
> +					 uio_of_dmem_sizes_prop, sizes,
> +					 regions);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Missing or invalid property %s\n",
> +			uio_of_dmem_sizes_prop);
> +		return ret;
> +	}
> +
> +	pdata.dynamic_region_sizes = devm_kzalloc(&pdev->dev,
> +			sizeof(*pdata.dynamic_region_sizes) *
> +			pdata.num_dynamic_regions, GFP_KERNEL);
> +	if (!pdata.dynamic_region_sizes) {
> +		dev_err(&pdev->dev, "Unable to alloc dynamic_region_sizes\n");
> +		ret = -ENOMEM;
> +		return ret;
> +	}
> +
> +	pdata.num_dynamic_regions = regions;
> +	while (regions--)
> +		pdata.dynamic_region_sizes[regions] = sizes[regions];
> +
> +	pdata.uioinfo.name = pdev->dev.of_node->name;
> +	pdata.uioinfo.version = "devicetree";
> +
> +	return platform_device_add_data(pdev, &pdata, sizeof(pdata));
> +}
> +
>  static int uio_dmem_genirq_probe(struct platform_device *pdev)
>  {
> -	struct uio_dmem_genirq_pdata *pdata = dev_get_platdata(&pdev->dev);
> -	struct uio_info *uioinfo = &pdata->uioinfo;
> +	struct uio_dmem_genirq_pdata *pdata;
> +	struct uio_info *uioinfo;
>  	struct uio_dmem_genirq_platdata *priv;
>  	struct uio_mem *uiomem;
>  	int ret = -EINVAL;
>  	int i;
>  
>  	if (pdev->dev.of_node) {
> -		/* alloc uioinfo for one device */
> -		uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
> -		if (!uioinfo) {
> -			ret = -ENOMEM;
> -			dev_err(&pdev->dev, "unable to kmalloc\n");
> +		ret = uio_dmem_genirq_alloc_platdata(pdev);
> +		if (ret)
>  			goto bad2;
> -		}
> -		uioinfo->name = pdev->dev.of_node->name;
> -		uioinfo->version = "devicetree";
>  	}
>  
> +	pdata = dev_get_platdata(&pdev->dev);
> +	uioinfo = &pdata->uioinfo;
> +
>  	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
>  		dev_err(&pdev->dev, "missing platform_data\n");
> -		goto bad0;
> +		goto bad2;
>  	}
>  
>  	if (uioinfo->handler || uioinfo->irqcontrol ||
>  	    uioinfo->irq_flags & IRQF_SHARED) {
>  		dev_err(&pdev->dev, "interrupt configuration error\n");
> -		goto bad0;
> +		goto bad2;
>  	}
>  
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv) {
>  		ret = -ENOMEM;
>  		dev_err(&pdev->dev, "unable to kmalloc\n");
> -		goto bad0;
> +		goto bad2;
>  	}
>  
> -	dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	if (!pdev->dev.of_node)
> +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
>  
>  	priv->uioinfo = uioinfo;
>  	spin_lock_init(&priv->lock);
> @@ -278,10 +339,6 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
>  	return 0;
>   bad1:
>  	kfree(priv);
> - bad0:
> -	/* kfree uioinfo for OF */
> -	if (pdev->dev.of_node)
> -		kfree(uioinfo);
>   bad2:
>  	return ret;
>  }
> @@ -296,10 +353,6 @@ static int uio_dmem_genirq_remove(struct platform_device *pdev)
>  	priv->uioinfo->handler = NULL;
>  	priv->uioinfo->irqcontrol = NULL;
>  
> -	/* kfree uioinfo for OF */
> -	if (pdev->dev.of_node)
> -		kfree(priv->uioinfo);
> -
>  	kfree(priv);
>  	return 0;
>  }
> @@ -327,10 +380,22 @@ static const struct dev_pm_ops uio_dmem_genirq_dev_pm_ops = {
>  };
>  
>  #ifdef CONFIG_OF
> -static const struct of_device_id uio_of_genirq_match[] = {
> -	{ /* empty for now */ },
> +static struct of_device_id uio_of_genirq_match[] = {
> +	{ /* This is filled with module_parm */ },
> +	{ /* end of list */ },
>  };
>  MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
> +module_param_string(of_id, uio_of_genirq_match[0].compatible, 128, 0);
> +MODULE_PARM_DESC(of_id, "Openfirmware id of the device to be handled by uio");
> +module_param_string(of_dma_bits_prop, uio_of_dma_bits_prop, 128, 0);
> +MODULE_PARM_DESC(of_dma_bits_prop,
> +		 "Openfirmware property for number of bits in DMA mask");
> +module_param_string(of_dmem_count_prop, uio_of_dmem_count_prop, 128, 0);
> +MODULE_PARM_DESC(of_dmem_count_prop,
> +		 "Openfirmware property for dynamic region count");
> +module_param_string(of_dmem_sizes_prop, uio_of_dmem_sizes_prop, 128, 0);
> +MODULE_PARM_DESC(of_dmem_sizes_prop,
> +		 "Openfirmware property for dynamic region sizes");
>  #endif
>  
>  static struct platform_driver uio_dmem_genirq = {
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Anup Patel]

On Fri, Jul 15, 2016 at 6:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> [adding devicetree list]
>
> On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
>> From: Jan Viktorin <viktorin@rehivetech.com>
>>
>> The uio_dmem_genirq works in a similar ways as uio_pdrv_genirq now.
>>
>> It accepts the of_id module parameter to specify UIO compatible
>> string as module parameter. There are few other module parameters
>> to specify DT property name for number bits in DMA mask and details
>> about dynamic regions.
>>
>> Following are the newly added module parameters:
>> 1) of_id: The UIO compatible string to be used for DT probing
>> 2) of_dma_bits_prot: This is name of OF property which will be
>> used to specify number of DMA mask bits in UIO DT node.
>> 3) of_dmem_count_prop: This is name of OF property which will be
>> used to specify number of dynamic regions in UIO DT node.
>> 4) of_dmem_sizes_prop: This is name of OF property which will be
>> used to specify sizes of each dynamic regions in UIO DT node.
>>
>> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>> ---
>>  drivers/uio/uio_dmem_genirq.c | 113 +++++++++++++++++++++++++++++++++---------
>>  1 file changed, 89 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
>> index a4d6d81..0a0cc19 100644
>> --- a/drivers/uio/uio_dmem_genirq.c
>> +++ b/drivers/uio/uio_dmem_genirq.c
>> @@ -144,46 +144,107 @@ static int uio_dmem_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>>       return 0;
>>  }
>>
>> +static char uio_of_dma_bits_prop[128] = "uio,dma-bits";
>> +static char uio_of_dmem_count_prop[128] = "uio,number-of-dynamic-regions";
>> +static char uio_of_dmem_sizes_prop[128] = "uio,dynamic-regions-sizes";
>
> What are these proeprties? What is a "dynamic region" in hardware terms?

"Dynamic region" is DMAble memory alloced using dma_alloc_xxx() APIs
for the device.

For DMA-capable devices accessed from user-space, we need DMAble
memory available to user-space. Sometime such devices are also
cache-coherent (or IO-coherent) so in such case user-space will need
cacheable DMAble memory.

Number of "Dynamic regions" required by UIO dmem device depends
on the type of HW device hence we describe number of "Dynamic regions"
and size of "Dynamic regions" via DT attributes.

>
> Why must they be in the DT, and why are the *property names* arbitrarily
> overridable as module parameters? What exactly do you expect there to be
> in a DT?

They must be in DT for platform devices created using DT probing. The
number of "Dynamic regions" required by UIO device will depend on the
nature of device. We cannot have same number and sizes of "Dynamic
regions" for different UIO dmem devices.

The UIO dmem driver is a generic driver and not specific to any type of
HW. In fact, UIO dmem driver is generic enough to access variety of
platform devices from user-space using UIO framework.

Based on this Rob had previously suggested to not have any (or enforce
any) DT bindings for UIO dmem driver.

Refer, https://lkml.org/lkml/2016/5/18/457
Link to original Jan's patchset is https://lkml.org/lkml/2016/5/17/141

We had two options:
1) Get details of "Dynamic regions" via module parameter but this
would mean using same number and sizes of "Dynamic regions" for
all UIO dmem devices.
2) Pass names of DT attributes used by UIO dmem driver as
module parameters.

>
> DT bindings need documentation, but there was none as part of this series.
>
> I do not think this makes sense from a DT perspective.
>

As explained above, we are not fixing DT compatible string and
DT attributes names for UIO dmem driver instead we pass all
these as module parameters (preferable via kernel args or at
time of module insertion). Due to this we don't require DT bindings
document.

Regards,
Anup

[PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Mark Rutland]

On Fri, Jul 15, 2016 at 09:57:28PM +0530, Anup Patel wrote:
> On Fri, Jul 15, 2016 at 6:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
> >> +static char uio_of_dma_bits_prop[128] = "uio,dma-bits";
> >> +static char uio_of_dmem_count_prop[128] = "uio,number-of-dynamic-regions";
> >> +static char uio_of_dmem_sizes_prop[128] = "uio,dynamic-regions-sizes";
> >
> > What are these proeprties? What is a "dynamic region" in hardware terms?
> 
> "Dynamic region" is DMAble memory alloced using dma_alloc_xxx() APIs
> for the device.
> 
> For DMA-capable devices accessed from user-space, we need DMAble
> memory available to user-space. Sometime such devices are also
> cache-coherent (or IO-coherent) so in such case user-space will need
> cacheable DMAble memory.
> 
> Number of "Dynamic regions" required by UIO dmem device depends
> on the type of HW device hence we describe number of "Dynamic regions"
> and size of "Dynamic regions" via DT attributes.

So this is something akin to the number of DMA channels a device might
use.

Either that's implicit knowledge of the device (if fixed), which doesn't
need to be in the DT, or a real property that should be described
explicitly in the device binding, that has nothing to do with UIO.

In general, a device which you might want to use with UIO may not
require such properties, so this is imposing a requirement on DTBs
purely for the sake of userspace software.

> > Why must they be in the DT, and why are the *property names* arbitrarily
> > overridable as module parameters? What exactly do you expect there to be
> > in a DT?
> 
> They must be in DT for platform devices created using DT probing. The
> number of "Dynamic regions" required by UIO device will depend on the
> nature of device. We cannot have same number and sizes of "Dynamic
> regions" for different UIO dmem devices.

If the devices are so different, they should not be handled by the same
driver that has no knowledge of those differences. Platform VFIO has the
same problem, and requires device-specific reset drivers.

> The UIO dmem driver is a generic driver and not specific to any type of
> HW. In fact, UIO dmem driver is generic enough to access variety of
> platform devices from user-space using UIO framework.
> 
> Based on this Rob had previously suggested to not have any (or enforce
> any) DT bindings for UIO dmem driver.
> 
> Refer, https://lkml.org/lkml/2016/5/18/457

I don't follow. Here Rob stated:

  DT describes h/w. UIO is not a h/w block, so this does not belong in DT. A
  UIO vs. kernel driver is purely a kernel decision which shouldn't
  require a DT change.

  The properties should be part of match data for a compatible string that
  needs them set. Or if they can be defined in a way that is actually a
  property of the h/w, then it would be acceptible. You'd still need to
  define compatible strings that the properties apply to.

Above, you have come up with a default set of property names which you
go looking for, and you expect a class of property, even if you don't
expect specific names. That constitutes a binding, and a poorly-defined
one at that.

Per my reading, Rob's point in https://lkml.org/lkml/2016/5/23/539 was
that providing the *values* as module parameters was fine, not providing
the *property names*.

> Link to original Jan's patchset is https://lkml.org/lkml/2016/5/17/141
> 
> We had two options:
> 1) Get details of "Dynamic regions" via module parameter but this
> would mean using same number and sizes of "Dynamic regions" for
> all UIO dmem devices.
> 2) Pass names of DT attributes used by UIO dmem driver as
> module parameters.
> 
> > DT bindings need documentation, but there was none as part of this series.
> >
> > I do not think this makes sense from a DT perspective.
> 
> As explained above, we are not fixing DT compatible string and
> DT attributes names for UIO dmem driver instead we pass all
> these as module parameters (preferable via kernel args or at
> time of module insertion). Due to this we don't require DT bindings
> document.

If you're looking for properties in the DT, you're assuming a binding of
some sort, regardless of whether you expect particular names.

To be clear, NAK to this as it stands.

Thanks,
Mark.

[PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Anup Patel]

On Fri, Jul 15, 2016 at 10:22 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jul 15, 2016 at 09:57:28PM +0530, Anup Patel wrote:
>> On Fri, Jul 15, 2016 at 6:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
>> >> +static char uio_of_dma_bits_prop[128] = "uio,dma-bits";
>> >> +static char uio_of_dmem_count_prop[128] = "uio,number-of-dynamic-regions";
>> >> +static char uio_of_dmem_sizes_prop[128] = "uio,dynamic-regions-sizes";
>> >
>> > What are these proeprties? What is a "dynamic region" in hardware terms?
>>
>> "Dynamic region" is DMAble memory alloced using dma_alloc_xxx() APIs
>> for the device.
>>
>> For DMA-capable devices accessed from user-space, we need DMAble
>> memory available to user-space. Sometime such devices are also
>> cache-coherent (or IO-coherent) so in such case user-space will need
>> cacheable DMAble memory.
>>
>> Number of "Dynamic regions" required by UIO dmem device depends
>> on the type of HW device hence we describe number of "Dynamic regions"
>> and size of "Dynamic regions" via DT attributes.
>
> So this is something akin to the number of DMA channels a device might
> use.
>
> Either that's implicit knowledge of the device (if fixed), which doesn't
> need to be in the DT, or a real property that should be described
> explicitly in the device binding, that has nothing to do with UIO.
>
> In general, a device which you might want to use with UIO may not
> require such properties, so this is imposing a requirement on DTBs
> purely for the sake of userspace software.
>
>> > Why must they be in the DT, and why are the *property names* arbitrarily
>> > overridable as module parameters? What exactly do you expect there to be
>> > in a DT?
>>
>> They must be in DT for platform devices created using DT probing. The
>> number of "Dynamic regions" required by UIO device will depend on the
>> nature of device. We cannot have same number and sizes of "Dynamic
>> regions" for different UIO dmem devices.
>
> If the devices are so different, they should not be handled by the same
> driver that has no knowledge of those differences. Platform VFIO has the
> same problem, and requires device-specific reset drivers.
>
>> The UIO dmem driver is a generic driver and not specific to any type of
>> HW. In fact, UIO dmem driver is generic enough to access variety of
>> platform devices from user-space using UIO framework.
>>
>> Based on this Rob had previously suggested to not have any (or enforce
>> any) DT bindings for UIO dmem driver.
>>
>> Refer, https://lkml.org/lkml/2016/5/18/457
>
> I don't follow. Here Rob stated:
>
>   DT describes h/w. UIO is not a h/w block, so this does not belong in DT. A
>   UIO vs. kernel driver is purely a kernel decision which shouldn't
>   require a DT change.
>
>   The properties should be part of match data for a compatible string that
>   needs them set. Or if they can be defined in a way that is actually a
>   property of the h/w, then it would be acceptible. You'd still need to
>   define compatible strings that the properties apply to.
>
> Above, you have come up with a default set of property names which you
> go looking for, and you expect a class of property, even if you don't
> expect specific names. That constitutes a binding, and a poorly-defined
> one at that.
>
> Per my reading, Rob's point in https://lkml.org/lkml/2016/5/23/539 was
> that providing the *values* as module parameters was fine, not providing
> the *property names*.
>
>> Link to original Jan's patchset is https://lkml.org/lkml/2016/5/17/141
>>
>> We had two options:
>> 1) Get details of "Dynamic regions" via module parameter but this
>> would mean using same number and sizes of "Dynamic regions" for
>> all UIO dmem devices.
>> 2) Pass names of DT attributes used by UIO dmem driver as
>> module parameters.
>>
>> > DT bindings need documentation, but there was none as part of this series.
>> >
>> > I do not think this makes sense from a DT perspective.
>>
>> As explained above, we are not fixing DT compatible string and
>> DT attributes names for UIO dmem driver instead we pass all
>> these as module parameters (preferable via kernel args or at
>> time of module insertion). Due to this we don't require DT bindings
>> document.
>
> If you're looking for properties in the DT, you're assuming a binding of
> some sort, regardless of whether you expect particular names.
>
> To be clear, NAK to this as it stands.

I think if we are not enforcing any DT bindings by passing compatible
and property names as module parameters then we don't need any
DT bindings document. We can certainly remove default property
names and enforce UIO dmem users to always pass this as module
parameters.

Anyways, you have already NAKed this so I will just get all dynamic
region info from module parameters only.

Regards,
Anup