[PATCH 0/5] UIO driver for APM X-Gene QMTM

[PATCH 0/5] UIO driver for APM X-Gene QMTM

[Ankit Jindal]

This patchset enables user space access to APM X-Gene QMTM
using UIO framework.

The patchset also introduces new type UIO_MEM_PHYS_CACHE
for mem regions because APM X-Gene QMTM device supports
cache coherency with CPU caches.

Ankit Jindal (5):
  uio: Add new UIO_MEM_PHYS_CACHE type for mem regions
  Documentation: Update documentation for UIO_MEM_PHYS_CACHE
  drivers: uio: Add Xgene QMTM UIO driver
  dt-bindings: Add binding info for Xgene QMTM UIO driver
  MAINTAINERS: Add entry for APM Xgene QMTM UIO driver

 Documentation/DocBook/uio-howto.tmpl               |    3 +-
 .../devicetree/bindings/uio/uio_xgene_qmtm.txt     |   45 +++
 MAINTAINERS                                        |    7 +
 drivers/uio/Kconfig                                |    8 +
 drivers/uio/Makefile                               |    1 +
 drivers/uio/uio.c                                  |   22 +-
 drivers/uio/uio_xgene_qmtm.c                       |  289 ++++++++++++++++++++
 include/linux/uio_driver.h                         |    7 +-
 8 files changed, 369 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
 create mode 100644 drivers/uio/uio_xgene_qmtm.c

-- 
1.7.9.5

[PATCH 1/5] uio: Add new UIO_MEM_PHYS_CACHE type for mem regions

[Ankit Jindal]

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 useful if dma-capable devices are capable of maintaining coherency
with CPU caches becasue we can allow cacheable access to memory shared
between such devices and user space.

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 <ankit.jindal@linaro.org>
Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
---
 drivers/uio/uio.c          |   22 +++++++++++++---------
 include/linux/uio_driver.h |    7 ++++---
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index a673e5b..ff5b6c0 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -644,7 +644,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, bool cacheable)
 {
 	struct uio_device *idev = vma->vm_private_data;
 	int mi = uio_find_mem_index(vma);
@@ -659,7 +659,9 @@ 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);
+
+	if (!cacheable)
+		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
 	/*
 	 * We cannot use the vm_iomap_memory() helper here,
@@ -706,13 +708,15 @@ 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, false);
+	case UIO_MEM_LOGICAL:
+	case UIO_MEM_VIRTUAL:
+		return uio_mmap_logical(vma);
+	case UIO_MEM_PHYS_CACHE:
+		return uio_mmap_physical(vma, true);
+	default:
+		return -EINVAL;
 	}
 }
 
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 1ad4724..f6ea5bc 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -114,10 +114,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_NONE		0
+#define UIO_MEM_PHYS		1
 #define UIO_MEM_LOGICAL	2
-#define UIO_MEM_VIRTUAL 3
+#define UIO_MEM_VIRTUAL	3
+#define UIO_MEM_PHYS_CACHE	4
 
 /* defines for uio_port->porttype */
 #define UIO_PORT_NONE	0
-- 
1.7.9.5

[PATCH 2/5] Documentation: Update documentation for UIO_MEM_PHYS_CACHE

[Ankit Jindal]

This patch update UIO documentation for new mem region
type UIO_MEM_PHYS_CACHE.

Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
---
 Documentation/DocBook/uio-howto.tmpl |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
index bbe9c1f..49e47d4 100644
--- a/Documentation/DocBook/uio-howto.tmpl
+++ b/Documentation/DocBook/uio-howto.tmpl
@@ -530,7 +530,8 @@ the memory region, it will show up in the corresponding sysfs node.
 <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.
+<varname>UIO_MEM_VIRTUAL</varname> for virtual memory and
+<varname>UIO_MEM_PHYS_CACHE</varname> for physical cacheable memory.
 </para></listitem>
 
 <listitem><para>
-- 
1.7.9.5

[PATCH 3/5] drivers: uio: Add Xgene QMTM UIO driver

[Ankit Jindal]

The Applied Micro X-Gene SOC has on-chip QMTM (Queue manager
and Traffic manager) which is hardware based Queue or Ring
manager. This QMTM devices can be used in conjuction with
other devices such as DMA Engine, Ethernet, Security Engine,
etc to assign work to based on Queues or Rings.

This patch allows user space access to Xgene QMTM device.

Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
---
 drivers/uio/Kconfig          |    8 ++
 drivers/uio/Makefile         |    1 +
 drivers/uio/uio_xgene_qmtm.c |  289 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 298 insertions(+)
 create mode 100644 drivers/uio/uio_xgene_qmtm.c

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 5a90914..d9e46bd 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -135,4 +135,12 @@ config UIO_MF624
 
 	  If you compile this as a module, it will be called uio_mf624.
 
+config UIO_XGENE_QMTM
+	tristate "Applied Micro Xgene QMTM driver"
+	depends on OF
+	help
+	  Userspace I/O interface for the Xgene QMTM. The userspace part of
+	  this driver will be available for download from the Applied Micro
+	  web site (http://www.apm.com/).
+
 endif
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index d3218bd..633eaa0 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UIO_PCI_GENERIC)	+= uio_pci_generic.o
 obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
 obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
 obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
+obj-$(CONFIG_UIO_XGENE_QMTM)	+= uio_xgene_qmtm.o
diff --git a/drivers/uio/uio_xgene_qmtm.c b/drivers/uio/uio_xgene_qmtm.c
new file mode 100644
index 0000000..895a385
--- /dev/null
+++ b/drivers/uio/uio_xgene_qmtm.c
@@ -0,0 +1,289 @@
+/*
+ * Xgene Queue Manager Traffic Manager (QMTM) UIO driver (uio_xgene_qmtm)
+ *
+ * This driver exports QMTM CSRs, Fabric and memory for queues to user-space
+ *
+ * Copyright (C) 2014 Applied Micro - http://www.apm.com/
+ * Copyright (C) 2014 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/slab.h>
+#include <linux/of_platform.h>
+
+#define DRV_NAME "qmtm_uio"
+#define DRV_VERSION "1.0"
+
+#define QMTM_CONFIG			0x00000004
+#define QMTM_SRST			0x0000c200
+#define QMTM_CLKEN			0x0000c208
+#define QMTM_CFG_MEM_RAM_SHUTDOWN	0x0000d070
+
+#define QMTM_DEFAULT_QSIZE		0x00010000
+
+struct uio_qmtm_dev {
+	struct uio_info *info;
+	struct clk *qmtm_clk;
+};
+
+/* QMTM CSR read/write routine */
+static inline void qmtm_csr_write(struct uio_qmtm_dev *qmtm_dev, u32 offset,
+		u32 data)
+{
+	void __iomem *addr = (u8 *)qmtm_dev->info->mem[0].internal_addr +
+		offset;
+
+	writel(data, addr);
+}
+
+static inline u32 qmtm_csr_read(struct uio_qmtm_dev *qmtm_dev, u32 offset)
+{
+	void __iomem *addr = (u8 *)qmtm_dev->info->mem[0].internal_addr +
+		offset;
+
+	return readl(addr);
+}
+
+static int qmtm_reset(struct uio_qmtm_dev *qmtm_dev)
+{
+	u32 val;
+	int wait = 1000;
+
+	/* get device out of reset */
+	qmtm_csr_write(qmtm_dev, QMTM_CLKEN, 3);
+	qmtm_csr_write(qmtm_dev, QMTM_SRST, 3);
+	udelay(1000);
+	qmtm_csr_write(qmtm_dev, QMTM_SRST, 0);
+	qmtm_csr_write(qmtm_dev, QMTM_CFG_MEM_RAM_SHUTDOWN, 0);
+
+	/* check whether device is out of reset or not */
+	do {
+		val = qmtm_csr_read(qmtm_dev, QMTM_CFG_MEM_RAM_SHUTDOWN);
+
+		if (!wait--)
+			return -1;
+		udelay(1);
+	} while (val == 0xffffffff);
+
+	return 0;
+}
+
+static int qmtm_open(struct uio_info *info, struct inode *inode)
+{
+	struct uio_qmtm_dev *qmtm_dev = info->priv;
+
+	/* Enable QPcore */
+	qmtm_csr_write(qmtm_dev, QMTM_CONFIG, 0x80000000);
+
+	return 0;
+}
+
+static int qmtm_release(struct uio_info *info, struct inode *inode)
+{
+	struct uio_qmtm_dev *qmtm_dev = info->priv;
+
+	/* Disable QPcore */
+	qmtm_csr_write(qmtm_dev, QMTM_CONFIG, 0);
+
+	return 0;
+}
+
+static void qmtm_cleanup(struct platform_device *pdev,
+		struct uio_qmtm_dev *qmtm_dev)
+{
+	struct uio_info *info = qmtm_dev->info;
+
+	uio_unregister_device(info);
+
+	kfree(info->name);
+
+	if (!IS_ERR(info->mem[0].internal_addr))
+		devm_iounmap(&pdev->dev, info->mem[0].internal_addr);
+
+	kfree(info);
+	clk_put(qmtm_dev->qmtm_clk);
+	kfree(qmtm_dev);
+}
+
+static int qmtm_probe(struct platform_device *pdev)
+{
+	struct uio_info *info;
+	struct uio_qmtm_dev *qmtm_dev;
+	struct resource *csr;
+	struct resource *fabric;
+	struct resource *qpool;
+	unsigned int num_queues;
+	unsigned int devid;
+	int ret = -ENODEV;
+
+	qmtm_dev = kzalloc(sizeof(struct uio_qmtm_dev), GFP_KERNEL);
+	if (!qmtm_dev)
+		return -ENOMEM;
+
+	qmtm_dev->info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!qmtm_dev->info) {
+		kfree(qmtm_dev);
+		return -ENOMEM;
+	}
+
+	/* Power on qmtm in case its not done as part of boot-loader */
+	qmtm_dev->qmtm_clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(qmtm_dev->qmtm_clk)) {
+		dev_err(&pdev->dev, "Failed to get clock\n");
+		ret = PTR_ERR(qmtm_dev->qmtm_clk);
+		kfree(qmtm_dev->info);
+		kfree(qmtm_dev);
+		return ret;
+	} else {
+		clk_prepare_enable(qmtm_dev->qmtm_clk);
+	}
+
+	csr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!csr) {
+		dev_err(&pdev->dev, "No QMTM CSR resource specified\n");
+		goto out_free;
+	}
+
+	if (!csr->start) {
+		dev_err(&pdev->dev, "Invalid CSR resource\n");
+		goto out_free;
+	}
+
+	fabric = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!fabric) {
+		dev_err(&pdev->dev, "No QMTM Fabric resource specified\n");
+		goto out_free;
+	}
+
+	if (!fabric->start) {
+		dev_err(&pdev->dev, "Invalid Fabric resource\n");
+		goto out_free;
+	}
+
+	qpool = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	if (!qpool) {
+		dev_err(&pdev->dev, "No QMTM Qpool resource specified\n");
+		goto out_free;
+	}
+
+	if (!qpool->start) {
+		dev_err(&pdev->dev, "Invalid Qpool resource\n");
+		goto out_free;
+	}
+
+	ret = of_property_read_u32(pdev->dev.of_node, "num_queues",
+			&num_queues);
+
+	if (ret < 0) {
+		dev_err(&pdev->dev, "No num_queues resource specified\n");
+		goto out_free;
+	}
+
+	/* check whether sufficient memory is provided for the given queues */
+	if (!((num_queues * QMTM_DEFAULT_QSIZE) <= resource_size(qpool))) {
+		dev_err(&pdev->dev, "Insufficient Qpool for the given queues\n");
+		goto out_free;
+	}
+
+	ret = of_property_read_u32(pdev->dev.of_node, "devid", &devid);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "No devid resource specified\n");
+		goto out_free;
+	}
+
+	info = qmtm_dev->info;
+	info->mem[0].name = "csr";
+	info->mem[0].addr = csr->start;
+	info->mem[0].size = resource_size(csr);
+	info->mem[0].memtype = UIO_MEM_PHYS;
+	info->mem[0].internal_addr = devm_ioremap_resource(&pdev->dev, csr);
+
+	if (IS_ERR(info->mem[0].internal_addr)) {
+		dev_err(&pdev->dev, "Failed to ioremap CSR region\n");
+		goto out_free;
+	}
+
+	info->mem[1].name = "fabric";
+	info->mem[1].addr = fabric->start;
+	info->mem[1].size = resource_size(fabric);
+	info->mem[1].memtype = UIO_MEM_PHYS;
+
+	info->mem[2].name = "qpool";
+	info->mem[2].addr = qpool->start;
+	info->mem[2].size = resource_size(qpool);
+	info->mem[2].memtype = UIO_MEM_PHYS_CACHE;
+
+	info->name = kasprintf(GFP_KERNEL, "qmtm%d", devid);
+	info->version = DRV_VERSION;
+
+	info->priv = qmtm_dev;
+	info->open = qmtm_open;
+	info->release = qmtm_release;
+
+	/* get the qmtm out of reset */
+	ret = qmtm_reset(qmtm_dev);
+	if (ret < 0)
+		goto out_free;
+
+	/* register with uio framework */
+	ret = uio_register_device(&pdev->dev, info);
+	if (ret < 0)
+		goto out_free;
+
+	dev_info(&pdev->dev, "%s registered as UIO device.\n", info->name);
+
+	platform_set_drvdata(pdev, qmtm_dev);
+	return 0;
+
+out_free:
+	qmtm_cleanup(pdev, qmtm_dev);
+	return ret;
+}
+
+static int qmtm_remove(struct platform_device *pdev)
+{
+	struct uio_qmtm_dev *qmtm_dev = platform_get_drvdata(pdev);
+
+	qmtm_cleanup(pdev, qmtm_dev);
+	return 0;
+}
+
+static struct of_device_id qmtm_match[] = {
+	{.compatible = "apm,xgene-qmtm-uio",},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, qmtm_match);
+
+static struct platform_driver qmtm_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = qmtm_match,
+	},
+	.probe = qmtm_probe,
+	.remove = qmtm_remove,
+};
+
+module_platform_driver(qmtm_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(DRV_VERSION);
+MODULE_AUTHOR("Ankit Jindal <ankit.jindal@linaro.org>");
+MODULE_AUTHOR("Tushar Jagad <tushar.jagad@linaro.org>");
-- 
1.7.9.5

[PATCH 4/5] dt-bindings: Add binding info for Xgene QMTM UIO driver

[Ankit Jindal]

This patch adds device tree binding documentation for
Xgene QMTM UIO driver.

Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
---
 .../devicetree/bindings/uio/uio_xgene_qmtm.txt     |   45 ++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt

diff --git a/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
new file mode 100644
index 0000000..b71831b
--- /dev/null
+++ b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
@@ -0,0 +1,45 @@
+APM X-Gene QMTM UIO nodes
+
+QMTM UIO nodes are defined for user space access to on-chip QMTM device
+on APM X-Gene SOC using UIO framework.
+
+Required properties:
+- compatible: Should be "apm,xgene-qmtm-uio"
+- reg: Address and length of the register set for the device. It contains the
+  information of registers in the same order as described by reg-names.
+- reg-names: Should contain the register set names
+  - "csr": QMTM control and status register address space.
+  - "fabric": QMTM memory mapped access to queue states.
+  - "qpool": Memory location for creating QMTM queues. This could be some
+    SRAM or reserved portion of RAM. It is expected that size and location
+    of qpool memory will be configurable via bootloader.
+- clocks: Reference to the clock entry.
+- num_queues: Number of queues under this QMTM device.
+- devid: QMTM identification number for the system having multiple QMTM devices
+
+Optional properties:
+- status: Should be "ok" or "disabled" for enabled/disabled. Default is "ok".
+
+Example:
+	qmtm1clk: qmtmclk@1f20c000 {
+		compatible = "apm,xgene-device-clock";
+		clock-output-names = "qmtm1clk";
+		status = "ok";
+	};
+
+	qmtm1_uio: qmtm_uio@1f200000 {
+		compatible = "apm,xgene-qmtm-uio";
+		status = "disabled";
+		reg =  <0x0 0x1f200000 0x0 0x10000
+			0x0 0x1b000000 0x0 0x400000
+			0x0 0x00000000 0x0 0x0>;/* filled by bootloader */
+		reg-names = "csr", "fabric", "qpool";
+		clocks = <&qmtm1clk 0>;
+		num_queues = <0x400>;
+		devid = <1>;
+	};
+
+	/* Board-specific peripheral configurations */
+	&qmtm1_uio {
+		status = "ok";
+	};
-- 
1.7.9.5

[PATCH 5/5] MAINTAINERS: Add entry for APM Xgene QMTM UIO driver

[Ankit Jindal]

Add entry to maintainer list for APM Xgene QMTM UIO driver.

Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
---
 MAINTAINERS |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5e7866a..138663f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -727,6 +727,13 @@ S:	Supported
 F:	drivers/net/ethernet/apm/xgene/
 F:	Documentation/devicetree/bindings/net/apm-xgene-enet.txt
 
+APPLIED MICRO (APM) X-GENE QMTM UIO DRIVER
+M:	Ankit Jindal <ankit.jindal@linaro.org>
+M:	Tushar Jagad <tushar.jagad@linaro.org>
+S:	Supported
+F:	drivers/uio/uio_xgene_qmtm.c
+F:	Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
+
 APTINA CAMERA SENSOR PLL
 M:	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
 L:	linux-media@vger.kernel.org
-- 
1.7.9.5

Re: [PATCH 4/5] dt-bindings: Add binding info for Xgene QMTM UIO driver

[Mark Rutland]

On Tue, Sep 09, 2014 at 10:56:58AM +0100, Ankit Jindal wrote:
> This patch adds device tree binding documentation for
> Xgene QMTM UIO driver.
> 
> Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
> Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
> ---
>  .../devicetree/bindings/uio/uio_xgene_qmtm.txt     |   45 ++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
> 
> diff --git a/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
> new file mode 100644
> index 0000000..b71831b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
> @@ -0,0 +1,45 @@
> +APM X-Gene QMTM UIO nodes
> +
> +QMTM UIO nodes are defined for user space access to on-chip QMTM device
> +on APM X-Gene SOC using UIO framework.
> +

Userspace vs kernel space has precisely _nothing_ to do with a HW
description.

This doesn't describe at all what the device is (e.g. what does QMTM
stand for, what is it used for?).

NAK.

Please ensure you Cc the devicetree list (devicetree@vger.kernel.org) in
future.

> +Required properties:
> +- compatible: Should be "apm,xgene-qmtm-uio"

This should definitely not have "uio" in the name.

> +- reg: Address and length of the register set for the device. It contains the
> +  information of registers in the same order as described by reg-names.
> +- reg-names: Should contain the register set names
> +  - "csr": QMTM control and status register address space.
> +  - "fabric": QMTM memory mapped access to queue states.

These look ok, I guess.

> +  - "qpool": Memory location for creating QMTM queues. This could be some
> +    SRAM or reserved portion of RAM. It is expected that size and location
> +    of qpool memory will be configurable via bootloader.

I don't follow why this should be described in a reg entry. This is not
a property of the device; this is a separate resource being assigned to
the device.

Why can the kernel not allocate this dynamically?

If you need a specific fixed pool, use the reserved-memory bindings.

> +- clocks: Reference to the clock entry.

There is only one clock input to the IP block?

> +- num_queues: Number of queues under this QMTM device.

s/_/-/, property names should use '-' rather than '_'.

> +- devid: QMTM identification number for the system having multiple QMTM devices

What exactly is this used for? Why is the reg entry not sufficient?

> +Optional properties:
> +- status: Should be "ok" or "disabled" for enabled/disabled. Default is "ok".

This is so standard I don't see the point in documenting it.

Mark.

Re: [PATCH 1/5] uio: Add new UIO_MEM_PHYS_CACHE type for mem regions

[Greg Kroah-Hartman]

On Tue, Sep 09, 2014 at 03:26:55PM +0530, Ankit Jindal wrote:
> 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 useful if dma-capable devices are capable of maintaining coherency
> with CPU caches becasue we can allow cacheable access to memory shared
> between such devices and user space.
> 
> 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 <ankit.jindal@linaro.org>
> Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
> ---
>  drivers/uio/uio.c          |   22 +++++++++++++---------
>  include/linux/uio_driver.h |    7 ++++---
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index a673e5b..ff5b6c0 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -644,7 +644,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, bool cacheable)
>  {
>  	struct uio_device *idev = vma->vm_private_data;
>  	int mi = uio_find_mem_index(vma);
> @@ -659,7 +659,9 @@ 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);
> +
> +	if (!cacheable)
> +		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>  
>  	/*
>  	 * We cannot use the vm_iomap_memory() helper here,
> @@ -706,13 +708,15 @@ 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, false);
> +	case UIO_MEM_LOGICAL:
> +	case UIO_MEM_VIRTUAL:
> +		return uio_mmap_logical(vma);
> +	case UIO_MEM_PHYS_CACHE:
> +		return uio_mmap_physical(vma, true);
> +	default:
> +		return -EINVAL;

Please don't do code style cleanups in the same patch you are adding new
functionality.  I can't take this because of this, sorry.

greg k-h

Re: [PATCH 1/5] uio: Add new UIO_MEM_PHYS_CACHE type for mem regions

[Ankit Jindal]

On 10 September 2014 00:59, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Sep 09, 2014 at 03:26:55PM +0530, Ankit Jindal wrote:
>> 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 useful if dma-capable devices are capable of maintaining coherency
>> with CPU caches becasue we can allow cacheable access to memory shared
>> between such devices and user space.
>>
>> 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 <ankit.jindal@linaro.org>
>> Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
>> ---
>>  drivers/uio/uio.c          |   22 +++++++++++++---------
>>  include/linux/uio_driver.h |    7 ++++---
>>  2 files changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> index a673e5b..ff5b6c0 100644
>> --- a/drivers/uio/uio.c
>> +++ b/drivers/uio/uio.c
>> @@ -644,7 +644,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, bool cacheable)
>>  {
>>       struct uio_device *idev = vma->vm_private_data;
>>       int mi = uio_find_mem_index(vma);
>> @@ -659,7 +659,9 @@ 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);
>> +
>> +     if (!cacheable)
>> +             vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>>
>>       /*
>>        * We cannot use the vm_iomap_memory() helper here,
>> @@ -706,13 +708,15 @@ 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, false);
>> +     case UIO_MEM_LOGICAL:
>> +     case UIO_MEM_VIRTUAL:
>> +             return uio_mmap_logical(vma);
>> +     case UIO_MEM_PHYS_CACHE:
>> +             return uio_mmap_physical(vma, true);
>> +     default:
>> +             return -EINVAL;
>
> Please don't do code style cleanups in the same patch you are adding new
> functionality.  I can't take this because of this, sorry.

Sure, I will separate out the patch for code style cleanups.

>
> greg k-h

Thanks,
Ankit

Re: [PATCH 4/5] dt-bindings: Add binding info for Xgene QMTM UIO driver

[Ankit Jindal]

On 9 September 2014 16:23, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Sep 09, 2014 at 10:56:58AM +0100, Ankit Jindal wrote:
>> This patch adds device tree binding documentation for
>> Xgene QMTM UIO driver.
>>
>> Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
>> Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
>> ---
>>  .../devicetree/bindings/uio/uio_xgene_qmtm.txt     |   45 ++++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
>> new file mode 100644
>> index 0000000..b71831b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/uio/uio_xgene_qmtm.txt
>> @@ -0,0 +1,45 @@
>> +APM X-Gene QMTM UIO nodes
>> +
>> +QMTM UIO nodes are defined for user space access to on-chip QMTM device
>> +on APM X-Gene SOC using UIO framework.
>> +
>
> Userspace vs kernel space has precisely _nothing_ to do with a HW
> description.
>
> This doesn't describe at all what the device is (e.g. what does QMTM
> stand for, what is it used for?).
>
> NAK.

QMTM stands for queue manager and traffic manager. It is device managing
the hardware queues. It also implements QoS among hardware queues hence
term "traffic" manager is present in its name.

Sure, I will add more info about the device in the next revision.

The primary purpose of QMTM UIO driver is to expose QMTM HW to
user space so that frameworks like ODP (OpenDataPlane) can use
this from user space.

>
> Please ensure you Cc the devicetree list (devicetree@vger.kernel.org) in
> future.

Sure, will do.

>
>> +Required properties:
>> +- compatible: Should be "apm,xgene-qmtm-uio"
>
> This should definitely not have "uio" in the name.

I added "uio" in compatible string because in future APM might
add separate driver for accessing QMTM from kernel space.

Can you suggest better compatible strings for this UIO driver?

>
>> +- reg: Address and length of the register set for the device. It contains the
>> +  information of registers in the same order as described by reg-names.
>> +- reg-names: Should contain the register set names
>> +  - "csr": QMTM control and status register address space.
>> +  - "fabric": QMTM memory mapped access to queue states.
>
> These look ok, I guess.
>
>> +  - "qpool": Memory location for creating QMTM queues. This could be some
>> +    SRAM or reserved portion of RAM. It is expected that size and location
>> +    of qpool memory will be configurable via bootloader.
>
> I don't follow why this should be described in a reg entry. This is not
> a property of the device; this is a separate resource being assigned to
> the device.

Ok, I will remove it from reg and add "qpool-addr" and "qpool-size"
attributes to
point the location of qpool.

>
> Why can the kernel not allocate this dynamically?

For faster access, this can be on-chip memory as well.

>
> If you need a specific fixed pool, use the reserved-memory bindings.

reserved-memory only apply to RAM, it does not apply to on-chip
SRAMs. Right ?

>
>> +- clocks: Reference to the clock entry.
>
> There is only one clock input to the IP block?

Yes.

>
>> +- num_queues: Number of queues under this QMTM device.
>
> s/_/-/, property names should use '-' rather than '_'.

Sure, will rename it in next revision.

>
>> +- devid: QMTM identification number for the system having multiple QMTM devices
>
> What exactly is this used for? Why is the reg entry not sufficient?

When there are multiple QMTM devices, the devid is used to form a
unique id (a tuple of queue number and device id) for the queues
belonging to this device. Unfortunately, we don't have any register
providing the devid of the device.

>
>> +Optional properties:
>> +- status: Should be "ok" or "disabled" for enabled/disabled. Default is "ok".
>
> This is so standard I don't see the point in documenting it.

Ok, I will remove it in next revision.

>
> Mark.

Thanks,
Ankit

Re: [PATCH 2/5] Documentation: Update documentation for UIO_MEM_PHYS_CACHE

[Russell King - ARM Linux]

On Tue, Sep 09, 2014 at 03:26:56PM +0530, Ankit Jindal wrote:
> This patch update UIO documentation for new mem region
> type UIO_MEM_PHYS_CACHE.
> 
> Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
> Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
> ---
>  Documentation/DocBook/uio-howto.tmpl |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
> index bbe9c1f..49e47d4 100644
> --- a/Documentation/DocBook/uio-howto.tmpl
> +++ b/Documentation/DocBook/uio-howto.tmpl
> @@ -530,7 +530,8 @@ the memory region, it will show up in the corresponding sysfs node.
>  <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.
> +<varname>UIO_MEM_VIRTUAL</varname> for virtual memory and
> +<varname>UIO_MEM_PHYS_CACHE</varname> for physical cacheable memory.

When I read this, I wondered what "physical cacheable memory" was.
Then I found that what you're doing with this is mapping physical
memory into userspace with cacheable attributes.

So, to avoid confusion, this should be "for physical memory, which
will be cacheably mapped" or similar.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/5] drivers: uio: Add Xgene QMTM UIO driver

[Russell King - ARM Linux]

On Tue, Sep 09, 2014 at 03:26:57PM +0530, Ankit Jindal wrote:
> diff --git a/drivers/uio/uio_xgene_qmtm.c b/drivers/uio/uio_xgene_qmtm.c
...
> +/* QMTM CSR read/write routine */
> +static inline void qmtm_csr_write(struct uio_qmtm_dev *qmtm_dev, u32 offset,
> +		u32 data)
> +{
> +	void __iomem *addr = (u8 *)qmtm_dev->info->mem[0].internal_addr +
> +		offset;
> +
> +	writel(data, addr);

Why not...
	void __iomem *base = qmtm_dev->info->mem[0].internal_addr;

	writel(data, addr + offset);

We permit void pointer arithmetic in the kernel.

> +static int qmtm_reset(struct uio_qmtm_dev *qmtm_dev)
> +{
...
> +	/* check whether device is out of reset or not */
> +	do {
> +		val = qmtm_csr_read(qmtm_dev, QMTM_CFG_MEM_RAM_SHUTDOWN);
> +
> +		if (!wait--)
> +			return -1;
> +		udelay(1);
> +	} while (val == 0xffffffff);

There's two points about the above:

1. The loop is buggy.  A correct implementation would:
	- test for the success condition
	- on success, break out of the loop
	- decrement the timeout, and break out of the loop if we have timed out
	- delay the required delay
	- repeat

   Note that this means that we will always check for success before
   deciding whether we've failed or not, /and/ without penalty of an
   unnecessary delay (as you have.)

2. returning -1 here is not on.  This value can (via your probe
   function) be propagated to userspace, where it will be interpreted
   as an -EPERM error.  Wouldn't a proper errno be better?

> +static void qmtm_cleanup(struct platform_device *pdev,
> +		struct uio_qmtm_dev *qmtm_dev)
> +{
> +	struct uio_info *info = qmtm_dev->info;
> +
> +	uio_unregister_device(info);
> +
> +	kfree(info->name);
> +
> +	if (!IS_ERR(info->mem[0].internal_addr))
> +		devm_iounmap(&pdev->dev, info->mem[0].internal_addr);

So what if we hit a failure at

	platform_get_resource(pdev, IORESOURCE_MEM, 0);

in the probe function below, and call this function.  The purpose of the
devm_* stuff is to avoid these kinds of error-cleanup errors by automating
that stuff.  devm_* APIs record the resource allocations against the
device, and when the probe function fails, or the driver is unbound, the
devm_* resources claimed in the probe function are freed.

> +
> +	kfree(info);
> +	clk_put(qmtm_dev->qmtm_clk);
> +	kfree(qmtm_dev);

All of the above, with the exception of uio_unregister_device() and the
missing clk_unprepare_disable() can be left to the devm_* stuff to deal
with, if you'd used the devm_* functions in the probe function.

> +}
> +
> +static int qmtm_probe(struct platform_device *pdev)
> +{
> +	struct uio_info *info;
> +	struct uio_qmtm_dev *qmtm_dev;
> +	struct resource *csr;
> +	struct resource *fabric;
> +	struct resource *qpool;
> +	unsigned int num_queues;
> +	unsigned int devid;
> +	int ret = -ENODEV;

Probably a bad idea to use a standard value.  You probably have some
error codes you've forgotten to propagate.

> +
> +	qmtm_dev = kzalloc(sizeof(struct uio_qmtm_dev), GFP_KERNEL);

devm_kzalloc

> +	if (!qmtm_dev)
> +		return -ENOMEM;
> +
> +	qmtm_dev->info = kzalloc(sizeof(*info), GFP_KERNEL);

devm_kzalloc

> +	if (!qmtm_dev->info) {
> +		kfree(qmtm_dev);

No need for this free.

> +		return -ENOMEM;
> +	}
> +
> +	/* Power on qmtm in case its not done as part of boot-loader */
> +	qmtm_dev->qmtm_clk = clk_get(&pdev->dev, NULL);

devm_clk_get

> +	if (IS_ERR(qmtm_dev->qmtm_clk)) {
> +		dev_err(&pdev->dev, "Failed to get clock\n");
> +		ret = PTR_ERR(qmtm_dev->qmtm_clk);
> +		kfree(qmtm_dev->info);
> +		kfree(qmtm_dev);

Both kfree's can be removed.

> +		return ret;
> +	} else {
> +		clk_prepare_enable(qmtm_dev->qmtm_clk);
> +	}
> +
> +	csr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!csr) {
> +		dev_err(&pdev->dev, "No QMTM CSR resource specified\n");
> +		goto out_free;
> +	}
> +
> +	if (!csr->start) {
> +		dev_err(&pdev->dev, "Invalid CSR resource\n");
> +		goto out_free;
> +	}
> +
> +	fabric = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!fabric) {
> +		dev_err(&pdev->dev, "No QMTM Fabric resource specified\n");
> +		goto out_free;
> +	}
> +
> +	if (!fabric->start) {
> +		dev_err(&pdev->dev, "Invalid Fabric resource\n");
> +		goto out_free;
> +	}
> +
> +	qpool = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	if (!qpool) {
> +		dev_err(&pdev->dev, "No QMTM Qpool resource specified\n");
> +		goto out_free;
> +	}
> +
> +	if (!qpool->start) {
> +		dev_err(&pdev->dev, "Invalid Qpool resource\n");
> +		goto out_free;
> +	}
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "num_queues",
> +			&num_queues);

You may wish to consider checking that you have a pdev->dev.of_node and
cleanly error if you don't.

> +
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "No num_queues resource specified\n");
> +		goto out_free;
> +	}
> +
> +	/* check whether sufficient memory is provided for the given queues */
> +	if (!((num_queues * QMTM_DEFAULT_QSIZE) <= resource_size(qpool))) {
> +		dev_err(&pdev->dev, "Insufficient Qpool for the given queues\n");
> +		goto out_free;
> +	}
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "devid", &devid);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "No devid resource specified\n");
> +		goto out_free;
> +	}
> +
> +	info = qmtm_dev->info;
> +	info->mem[0].name = "csr";
> +	info->mem[0].addr = csr->start;
> +	info->mem[0].size = resource_size(csr);
> +	info->mem[0].memtype = UIO_MEM_PHYS;
> +	info->mem[0].internal_addr = devm_ioremap_resource(&pdev->dev, csr);
> +
> +	if (IS_ERR(info->mem[0].internal_addr)) {
> +		dev_err(&pdev->dev, "Failed to ioremap CSR region\n");

How about printing the error code, and propagating that error code to your
caller?

> +		goto out_free;
> +	}
> +
> +	info->mem[1].name = "fabric";
> +	info->mem[1].addr = fabric->start;
> +	info->mem[1].size = resource_size(fabric);
> +	info->mem[1].memtype = UIO_MEM_PHYS;
> +
> +	info->mem[2].name = "qpool";
> +	info->mem[2].addr = qpool->start;
> +	info->mem[2].size = resource_size(qpool);
> +	info->mem[2].memtype = UIO_MEM_PHYS_CACHE;
> +
> +	info->name = kasprintf(GFP_KERNEL, "qmtm%d", devid);

devm_kasprintf

> +	info->version = DRV_VERSION;
> +
> +	info->priv = qmtm_dev;
> +	info->open = qmtm_open;
> +	info->release = qmtm_release;
> +
> +	/* get the qmtm out of reset */
> +	ret = qmtm_reset(qmtm_dev);
> +	if (ret < 0)
> +		goto out_free;

Here you propagate that -1 value out of your probe function to userspace.
If you want to do this, please choose a reasonable error code, rather
than -EPERM.

> +
> +	/* register with uio framework */
> +	ret = uio_register_device(&pdev->dev, info);
> +	if (ret < 0)
> +		goto out_free;
> +
> +	dev_info(&pdev->dev, "%s registered as UIO device.\n", info->name);

Does this printk serve a useful purpose?  Most other UIO drivers don't
bother printing anything, though if you do print something, it may be
useful to mention which uio device it is associated with.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/5] Documentation: Update documentation for UIO_MEM_PHYS_CACHE

[Ankit Jindal]

On 15 September 2014 01:31, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Sep 09, 2014 at 03:26:56PM +0530, Ankit Jindal wrote:
>> This patch update UIO documentation for new mem region
>> type UIO_MEM_PHYS_CACHE.
>>
>> Signed-off-by: Ankit Jindal <ankit.jindal@linaro.org>
>> Signed-off-by: Tushar Jagad <tushar.jagad@linaro.org>
>> ---
>>  Documentation/DocBook/uio-howto.tmpl |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/DocBook/uio-howto.tmpl b/Documentation/DocBook/uio-howto.tmpl
>> index bbe9c1f..49e47d4 100644
>> --- a/Documentation/DocBook/uio-howto.tmpl
>> +++ b/Documentation/DocBook/uio-howto.tmpl
>> @@ -530,7 +530,8 @@ the memory region, it will show up in the corresponding sysfs node.
>>  <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.
>> +<varname>UIO_MEM_VIRTUAL</varname> for virtual memory and
>> +<varname>UIO_MEM_PHYS_CACHE</varname> for physical cacheable memory.
>
> When I read this, I wondered what "physical cacheable memory" was.
> Then I found that what you're doing with this is mapping physical
> memory into userspace with cacheable attributes.
>
> So, to avoid confusion, this should be "for physical memory, which
> will be cacheably mapped" or similar.

Thanks, will frame better in next version.

Thanks,
Ankit

Re: [PATCH 3/5] drivers: uio: Add Xgene QMTM UIO driver

[Ankit Jindal]

On 15 September 2014 01:53, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Sep 09, 2014 at 03:26:57PM +0530, Ankit Jindal wrote:
>> diff --git a/drivers/uio/uio_xgene_qmtm.c b/drivers/uio/uio_xgene_qmtm.c
> ...
>> +/* QMTM CSR read/write routine */
>> +static inline void qmtm_csr_write(struct uio_qmtm_dev *qmtm_dev, u32 offset,
>> +             u32 data)
>> +{
>> +     void __iomem *addr = (u8 *)qmtm_dev->info->mem[0].internal_addr +
>> +             offset;
>> +
>> +     writel(data, addr);
>
> Why not...
>         void __iomem *base = qmtm_dev->info->mem[0].internal_addr;
>
>         writel(data, addr + offset);
>
> We permit void pointer arithmetic in the kernel.
>
>> +static int qmtm_reset(struct uio_qmtm_dev *qmtm_dev)
>> +{
> ...
>> +     /* check whether device is out of reset or not */
>> +     do {
>> +             val = qmtm_csr_read(qmtm_dev, QMTM_CFG_MEM_RAM_SHUTDOWN);
>> +
>> +             if (!wait--)
>> +                     return -1;
>> +             udelay(1);
>> +     } while (val == 0xffffffff);
>
> There's two points about the above:
>
> 1. The loop is buggy.  A correct implementation would:
>         - test for the success condition
>         - on success, break out of the loop
>         - decrement the timeout, and break out of the loop if we have timed out
>         - delay the required delay
>         - repeat
>
>    Note that this means that we will always check for success before
>    deciding whether we've failed or not, /and/ without penalty of an
>    unnecessary delay (as you have.)
>
> 2. returning -1 here is not on.  This value can (via your probe
>    function) be propagated to userspace, where it will be interpreted
>    as an -EPERM error.  Wouldn't a proper errno be better?
>
>> +static void qmtm_cleanup(struct platform_device *pdev,
>> +             struct uio_qmtm_dev *qmtm_dev)
>> +{
>> +     struct uio_info *info = qmtm_dev->info;
>> +
>> +     uio_unregister_device(info);
>> +
>> +     kfree(info->name);
>> +
>> +     if (!IS_ERR(info->mem[0].internal_addr))
>> +             devm_iounmap(&pdev->dev, info->mem[0].internal_addr);
>
> So what if we hit a failure at
>
>         platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> in the probe function below, and call this function.  The purpose of the
> devm_* stuff is to avoid these kinds of error-cleanup errors by automating
> that stuff.  devm_* APIs record the resource allocations against the
> device, and when the probe function fails, or the driver is unbound, the
> devm_* resources claimed in the probe function are freed.
>
>> +
>> +     kfree(info);
>> +     clk_put(qmtm_dev->qmtm_clk);
>> +     kfree(qmtm_dev);
>
> All of the above, with the exception of uio_unregister_device() and the
> missing clk_unprepare_disable() can be left to the devm_* stuff to deal
> with, if you'd used the devm_* functions in the probe function.
>
>> +}
>> +
>> +static int qmtm_probe(struct platform_device *pdev)
>> +{
>> +     struct uio_info *info;
>> +     struct uio_qmtm_dev *qmtm_dev;
>> +     struct resource *csr;
>> +     struct resource *fabric;
>> +     struct resource *qpool;
>> +     unsigned int num_queues;
>> +     unsigned int devid;
>> +     int ret = -ENODEV;
>
> Probably a bad idea to use a standard value.  You probably have some
> error codes you've forgotten to propagate.
>
>> +
>> +     qmtm_dev = kzalloc(sizeof(struct uio_qmtm_dev), GFP_KERNEL);
>
> devm_kzalloc
>
>> +     if (!qmtm_dev)
>> +             return -ENOMEM;
>> +
>> +     qmtm_dev->info = kzalloc(sizeof(*info), GFP_KERNEL);
>
> devm_kzalloc
>
>> +     if (!qmtm_dev->info) {
>> +             kfree(qmtm_dev);
>
> No need for this free.
>
>> +             return -ENOMEM;
>> +     }
>> +
>> +     /* Power on qmtm in case its not done as part of boot-loader */
>> +     qmtm_dev->qmtm_clk = clk_get(&pdev->dev, NULL);
>
> devm_clk_get
>
>> +     if (IS_ERR(qmtm_dev->qmtm_clk)) {
>> +             dev_err(&pdev->dev, "Failed to get clock\n");
>> +             ret = PTR_ERR(qmtm_dev->qmtm_clk);
>> +             kfree(qmtm_dev->info);
>> +             kfree(qmtm_dev);
>
> Both kfree's can be removed.
>
>> +             return ret;
>> +     } else {
>> +             clk_prepare_enable(qmtm_dev->qmtm_clk);
>> +     }
>> +
>> +     csr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     if (!csr) {
>> +             dev_err(&pdev->dev, "No QMTM CSR resource specified\n");
>> +             goto out_free;
>> +     }
>> +
>> +     if (!csr->start) {
>> +             dev_err(&pdev->dev, "Invalid CSR resource\n");
>> +             goto out_free;
>> +     }
>> +
>> +     fabric = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +     if (!fabric) {
>> +             dev_err(&pdev->dev, "No QMTM Fabric resource specified\n");
>> +             goto out_free;
>> +     }
>> +
>> +     if (!fabric->start) {
>> +             dev_err(&pdev->dev, "Invalid Fabric resource\n");
>> +             goto out_free;
>> +     }
>> +
>> +     qpool = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> +     if (!qpool) {
>> +             dev_err(&pdev->dev, "No QMTM Qpool resource specified\n");
>> +             goto out_free;
>> +     }
>> +
>> +     if (!qpool->start) {
>> +             dev_err(&pdev->dev, "Invalid Qpool resource\n");
>> +             goto out_free;
>> +     }
>> +
>> +     ret = of_property_read_u32(pdev->dev.of_node, "num_queues",
>> +                     &num_queues);
>
> You may wish to consider checking that you have a pdev->dev.of_node and
> cleanly error if you don't.
>
>> +
>> +     if (ret < 0) {
>> +             dev_err(&pdev->dev, "No num_queues resource specified\n");
>> +             goto out_free;
>> +     }
>> +
>> +     /* check whether sufficient memory is provided for the given queues */
>> +     if (!((num_queues * QMTM_DEFAULT_QSIZE) <= resource_size(qpool))) {
>> +             dev_err(&pdev->dev, "Insufficient Qpool for the given queues\n");
>> +             goto out_free;
>> +     }
>> +
>> +     ret = of_property_read_u32(pdev->dev.of_node, "devid", &devid);
>> +     if (ret < 0) {
>> +             dev_err(&pdev->dev, "No devid resource specified\n");
>> +             goto out_free;
>> +     }
>> +
>> +     info = qmtm_dev->info;
>> +     info->mem[0].name = "csr";
>> +     info->mem[0].addr = csr->start;
>> +     info->mem[0].size = resource_size(csr);
>> +     info->mem[0].memtype = UIO_MEM_PHYS;
>> +     info->mem[0].internal_addr = devm_ioremap_resource(&pdev->dev, csr);
>> +
>> +     if (IS_ERR(info->mem[0].internal_addr)) {
>> +             dev_err(&pdev->dev, "Failed to ioremap CSR region\n");
>
> How about printing the error code, and propagating that error code to your
> caller?
>
>> +             goto out_free;
>> +     }
>> +
>> +     info->mem[1].name = "fabric";
>> +     info->mem[1].addr = fabric->start;
>> +     info->mem[1].size = resource_size(fabric);
>> +     info->mem[1].memtype = UIO_MEM_PHYS;
>> +
>> +     info->mem[2].name = "qpool";
>> +     info->mem[2].addr = qpool->start;
>> +     info->mem[2].size = resource_size(qpool);
>> +     info->mem[2].memtype = UIO_MEM_PHYS_CACHE;
>> +
>> +     info->name = kasprintf(GFP_KERNEL, "qmtm%d", devid);
>
> devm_kasprintf
>
>> +     info->version = DRV_VERSION;
>> +
>> +     info->priv = qmtm_dev;
>> +     info->open = qmtm_open;
>> +     info->release = qmtm_release;
>> +
>> +     /* get the qmtm out of reset */
>> +     ret = qmtm_reset(qmtm_dev);
>> +     if (ret < 0)
>> +             goto out_free;
>
> Here you propagate that -1 value out of your probe function to userspace.
> If you want to do this, please choose a reasonable error code, rather
> than -EPERM.
>
>> +
>> +     /* register with uio framework */
>> +     ret = uio_register_device(&pdev->dev, info);
>> +     if (ret < 0)
>> +             goto out_free;
>> +
>> +     dev_info(&pdev->dev, "%s registered as UIO device.\n", info->name);
>
> Does this printk serve a useful purpose?  Most other UIO drivers don't
> bother printing anything, though if you do print something, it may be
> useful to mention which uio device it is associated with.
>
Thanks, will incorporate all the suggested changes in next version.

Thanks,
Ankit