[PATCH 00/03][RFC] Reusable UIO Platform Driver

[PATCH 00/03][RFC] Reusable UIO Platform Driver

[Magnus Damm]

These patches implement a reusable UIO platform driver. This driver
can be used to export hardware to user space, as long as the device
is a) memory mapped and b) equipped with an unique IRQ.

The uio_platform driver gets all information through platform data,
including address window information and IRQ number. The driver also
supports assigning a contiguous piece of memory to each instance.
This may be useful when the exported hardware blocks can bus master
but requires physically contiguous memory.

There are not many surprises in the code if you are familiar with UIO,
except for the interrupt handling. All UIO kernel drivers that I've
seen so far have hardware specific interrupt acknowledge code in the
interrupt handler. The uio_platform driver is different.

The interrupt handling code in uio_platform assumes the device is the
only user of the assigned interrupt. This may be rare in the PC world
but for SuperH almost all interrupt sources are unique. Having an
unique interrupt for the device allows the code to use disable_irq()
and enable_irq() in kernel space and leave actual interrupt acknowledge
to user space. That way we have no hardware specific code in the kernel.

Interrupts are of course serviced in kernel space by the uio_platform
driver, but the uio_platform interrupt handler will simply disable the
IRQ until next read() or poll() syscall. The uio_platform kernel driver
assumes that the user space driver has taken care of acknowledging
the interrupt before doing read() and waiting for events again. If no
acknowledge has happened then an interrupt will occur again (since it's
still pending) and the kernel interrupt handler will disable the IRQ
again and unblock the user space process.

The last patch contains SuperH specific code that exports various
multimedia acceleration blocks to userspace. The following processors
and hardware blocks are exported for now:

sh7343: VPU
sh7722: VPU, VEU
sh7723: VPU, VEU, VEU

If anyone is interested then I have a proof of concept vidix driver
for mplayer that is interfacing using UIO to the VEU on a sh7722 to
provide accelerated color space conversion and stretching.

[PATCH 01/03] uio: Add enable_irq() callback
[PATCH 02/03] uio: Add uio_platform driver
[PATCH 03/03] sh: Export sh7343/sh7722/sh7723 VPU/VEU blocks

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 arch/sh/kernel/cpu/sh4a/setup-sh7343.c |   32 ++++++
 arch/sh/kernel/cpu/sh4a/setup-sh7722.c |   63 ++++++++++++
 arch/sh/kernel/cpu/sh4a/setup-sh7723.c |   94 ++++++++++++++++++
 drivers/uio/Kconfig                    |   10 +
 drivers/uio/Makefile                   |    1 
 drivers/uio/uio.c                      |    6 +
 drivers/uio/uio_platform.c             |  161 ++++++++++++++++++++++++++++++++
 include/linux/uio_driver.h             |    1 
 include/linux/uio_platform.h           |   10 +
 9 files changed, 378 insertions(+)

[PATCH 01/03] uio: Add enable_irq() callback

[Magnus Damm]

Add enable_irq() callback to struct uio_info. This callback is needed by
the uio_platform driver so interrupts can be enabled before blocking.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 drivers/uio/uio.c          |    6 ++++++
 include/linux/uio_driver.h |    1 +
 2 files changed, 7 insertions(+)

--- 0001/drivers/uio/uio.c
+++ work/drivers/uio/uio.c	2008-05-19 14:52:08.000000000 +0900
@@ -365,6 +365,9 @@ static unsigned int uio_poll(struct file
 	if (idev->info->irq == UIO_IRQ_NONE)
 		return -EIO;
 
+	if (idev->info->enable_irq)
+		idev->info->enable_irq(idev->info);
+
 	poll_wait(filep, &idev->wait, wait);
 	if (listener->event_count != atomic_read(&idev->event))
 		return POLLIN | POLLRDNORM;
@@ -391,6 +394,9 @@ static ssize_t uio_read(struct file *fil
 	do {
 		set_current_state(TASK_INTERRUPTIBLE);
 
+		if (idev->info->enable_irq)
+			idev->info->enable_irq(idev->info);
+
 		event_count = atomic_read(&idev->event);
 		if (event_count != listener->event_count) {
 			if (copy_to_user(buf, &event_count, count))
--- 0001/include/linux/uio_driver.h
+++ work/include/linux/uio_driver.h	2008-05-19 14:52:08.000000000 +0900
@@ -64,6 +64,7 @@ struct uio_info {
 	void			*priv;
 	irqreturn_t (*handler)(int irq, struct uio_info *dev_info);
 	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
+	void (*enable_irq)(struct uio_info *info);
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
 };

[PATCH 02/03] uio: Add uio_platform driver

[Magnus Damm]

This patch implements a reusable UIO platform driver. Only memory mapped
hardware devices with unique IRQs are supported.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Tested using the VEU on a SuperH sh7722.

 drivers/uio/Kconfig          |   10 ++
 drivers/uio/Makefile         |    1 
 drivers/uio/uio_platform.c   |  161 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/uio_platform.h |   10 ++
 4 files changed, 182 insertions(+)

--- 0001/drivers/uio/Kconfig
+++ work/drivers/uio/Kconfig	2008-05-20 17:20:18.000000000 +0900
@@ -39,4 +39,14 @@ config UIO_SMX
 
 	  If you compile this as a module, it will be called uio_smx.
 
+config UIO_PLATFORM
+	tristate "Userspace I/O Platform driver"
+	depends on UIO
+	default n
+	help
+	  Reusable userspace IO interface for memory mapped devices
+	  equipped with an unique IRQ. IRQ sharing is not supported.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called uio_platform.
 endif
--- 0001/drivers/uio/Makefile
+++ work/drivers/uio/Makefile	2008-05-20 17:20:18.000000000 +0900
@@ -1,3 +1,4 @@
 obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
 obj-$(CONFIG_UIO_SMX)	+= uio_smx.o
+obj-$(CONFIG_UIO_PLATFORM)	+= uio_platform.o
--- /dev/null
+++ work/drivers/uio/uio_platform.c	2008-05-20 18:06:08.000000000 +0900
@@ -0,0 +1,161 @@
+/*
+ * Userspace I/O Platform Driver
+ *
+ * Copyright(C) 2008 Magnus Damm
+ *
+ * Platform data driven UIO kernel glue, only memory mapped I/O devices
+ * with unique IRQs are supported.
+ *
+ * Licensed under the GPLv2 only.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/uio_platform.h>
+#include <linux/bitops.h>
+#include <linux/dma-mapping.h>
+#include <linux/mm.h>
+
+struct uio_platform_priv {
+	struct uio_info info;
+	unsigned long irq_disabled;
+};
+
+#define to_priv(uinfo) container_of(uinfo, struct uio_platform_priv, info)
+
+static irqreturn_t uio_platform_handler(int irq, struct uio_info *dev_info)
+{
+	struct uio_platform_priv *priv = to_priv(dev_info);
+
+	/* Since we are the only user of this interrupt, just disable
+	 * it and remember the state so we can enable it later.
+	 */
+
+	disable_irq(irq);
+	set_bit(0, &priv->irq_disabled);
+	return IRQ_HANDLED;
+}
+
+static void uio_platform_enable_irq(struct uio_info *dev_info)
+{
+	struct uio_platform_priv *priv = to_priv(dev_info);
+
+	if (test_and_clear_bit(0, &priv->irq_disabled))
+		enable_irq(dev_info->irq);
+}
+
+static int uio_platform_probe(struct platform_device *dev)
+{
+	struct uio_platform_info *platform_info = dev->dev.platform_data;
+	struct uio_platform_priv *priv;
+	struct uio_info *info;
+	struct resource	*r;
+	int ret, k;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	info = &priv->info;
+
+	for (k = 0; k < MAX_UIO_MAPS; k++) {
+		r = platform_get_resource(dev, IORESOURCE_MEM, k);
+		if (r == NULL)
+			break;
+
+		info->mem[k].memtype = UIO_MEM_PHYS;
+		info->mem[k].addr = r->start;
+		info->mem[k].size = r->end - r->start + 1;
+	}
+
+	if (platform_info->memsize && k < MAX_UIO_MAPS) {
+		dma_addr_t dma_handle;
+		void *buf;
+
+		buf = dma_alloc_coherent(&dev->dev, platform_info->memsize,
+					 &dma_handle, GFP_KERNEL);
+		if (buf == NULL) {
+			kfree(priv);
+			return -ENOMEM;
+		}
+
+		info->mem[k].memtype = UIO_MEM_PHYS;
+		info->mem[k].addr = dma_handle;
+		info->mem[k].internal_addr = buf;
+		info->mem[k].size = platform_info->memsize;
+
+		memset(buf, 0, info->mem[k].size);
+	}
+
+	info->name = platform_info->name;
+	info->version = platform_info->version;
+	info->irq = platform_get_irq(dev, 0);
+	info->irq_flags = IRQF_DISABLED;
+	info->handler = uio_platform_handler;
+	info->enable_irq = uio_platform_enable_irq;
+
+	platform_set_drvdata(dev, info);
+
+	ret = uio_register_device(&dev->dev, info);
+	if (ret)
+		goto err;
+
+	return 0;
+ err:
+	for (k = 0; k < MAX_UIO_MAPS; k++)
+		if (info->mem[k].memtype == UIO_MEM_PHYS)
+			dma_free_coherent(&dev->dev, info->mem[k].size,
+					  info->mem[k].internal_addr,
+					  info->mem[k].addr);
+	kfree(priv);
+	return ret;
+}
+
+static int uio_platform_remove(struct platform_device *dev)
+{
+	struct uio_info *info = platform_get_drvdata(dev);
+	struct uio_platform_priv *priv = to_priv(info);
+	int k;
+
+	uio_unregister_device(&priv->info);
+
+	for (k = 0; k < MAX_UIO_MAPS; k++)
+		if (info->mem[k].memtype == UIO_MEM_PHYS)
+			dma_free_coherent(&dev->dev, info->mem[k].size,
+					  info->mem[k].internal_addr,
+					  info->mem[k].addr);
+
+	kfree(priv);
+	return 0;
+}
+
+static struct platform_driver uio_platform_drv = {
+	.probe		= uio_platform_probe,
+	.remove		= uio_platform_remove,
+	.driver		= {
+		.name	= "uio-platform",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init uio_platform_init(void)
+{
+	return platform_driver_register(&uio_platform_drv);
+}
+module_init(uio_platform_init);
+
+static void __exit uio_platform_exit(void)
+{
+	platform_driver_unregister(&uio_platform_drv);
+}
+module_exit(uio_platform_exit);
+
+MODULE_DESCRIPTION("UIO Platform Driver");
+MODULE_AUTHOR("Magnus Damm <damm@opensource.se>");
+MODULE_LICENSE("GPL v2");
--- /dev/null
+++ work/include/linux/uio_platform.h	2008-05-20 17:20:18.000000000 +0900
@@ -0,0 +1,10 @@
+#ifndef __LINUX_UIO_PLATFORM_H__
+#define __LINUX_UIO_PLATFORM_H__
+
+struct uio_platform_info {
+	char *name;
+	char *version;
+	unsigned long memsize;
+};
+
+#endif /* __LINUX_UIO_PLATFORM_H__ */

[PATCH 03/03] sh: Export sh7343/sh7722/sh7723 VPU/VEU blocks

[Magnus Damm]

This patch exports the following SuperH hardware to user space:

sh7343: VPU
sh7722: VPU, VEU
sh7723: VPU, VEU, VEU

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 arch/sh/kernel/cpu/sh4a/setup-sh7343.c |   32 ++++++++++
 arch/sh/kernel/cpu/sh4a/setup-sh7722.c |   63 +++++++++++++++++++++
 arch/sh/kernel/cpu/sh4a/setup-sh7723.c |   94 ++++++++++++++++++++++++++++++++
 3 files changed, 189 insertions(+)

--- 0001/arch/sh/kernel/cpu/sh4a/setup-sh7343.c
+++ work/arch/sh/kernel/cpu/sh4a/setup-sh7343.c	2008-05-20 17:19:05.000000000 +0900
@@ -11,6 +11,37 @@
 #include <linux/init.h>
 #include <linux/serial.h>
 #include <linux/serial_sci.h>
+#include <linux/uio_platform.h>
+
+static struct uio_platform_info vpu_platform_data = {
+	.name = "VPU",
+	.version = "0.0.1",
+	.memsize = 1 << 20,
+};
+
+static struct resource vpu_resources[] = {
+	[0] = {
+		.name	= "VPU",
+		.start	= 0xfe900000,
+		.end	= 0xfe9022ec,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= 60,
+		.end	= 60,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device vpu_device = {
+	.name		= "uio-platform",
+	.id		= 0,
+	.dev = {
+		.platform_data	= &vpu_platform_data,
+	},
+	.resource	= vpu_resources,
+	.num_resources	= ARRAY_SIZE(vpu_resources),
+};
 
 static struct plat_sci_port sci_platform_data[] = {
 	{
@@ -33,6 +64,7 @@ static struct platform_device sci_device
 
 static struct platform_device *sh7343_devices[] __initdata = {
 	&sci_device,
+	&vpu_device,
 };
 
 static int __init sh7343_devices_setup(void)
--- 0001/arch/sh/kernel/cpu/sh4a/setup-sh7722.c
+++ work/arch/sh/kernel/cpu/sh4a/setup-sh7722.c	2008-05-20 17:19:05.000000000 +0900
@@ -12,6 +12,7 @@
 #include <linux/serial.h>
 #include <linux/serial_sci.h>
 #include <linux/mm.h>
+#include <linux/uio_platform.h>
 #include <asm/mmzone.h>
 
 static struct resource usbf_resources[] = {
@@ -59,6 +60,66 @@ static struct platform_device iic_device
 	.resource       = iic_resources,
 };
 
+static struct uio_platform_info veu_platform_data = {
+	.name = "VEU",
+	.version = "0.0.1",
+	.memsize = 2 << 20,
+};
+
+static struct resource veu_resources[] = {
+	[0] = {
+		.name	= "VEU",
+		.start	= 0xfe920000,
+		.end	= 0xfe9200b7,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= 54,
+		.end	= 54,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device veu_device = {
+	.name		= "uio-platform",
+	.id		= 0,
+	.dev = {
+		.platform_data	= &veu_platform_data,
+	},
+	.resource	= veu_resources,
+	.num_resources	= ARRAY_SIZE(veu_resources),
+};
+
+static struct uio_platform_info vpu_platform_data = {
+	.name = "VPU",
+	.version = "0.0.1",
+	.memsize = 1 << 20,
+};
+
+static struct resource vpu_resources[] = {
+	[0] = {
+		.name	= "VPU",
+		.start	= 0xfe900000,
+		.end	= 0xfe9022ec,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= 60,
+		.end	= 60,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device vpu_device = {
+	.name		= "uio-platform",
+	.id		= 1,
+	.dev = {
+		.platform_data	= &vpu_platform_data,
+	},
+	.resource	= vpu_resources,
+	.num_resources	= ARRAY_SIZE(vpu_resources),
+};
+
 static struct plat_sci_port sci_platform_data[] = {
 	{
 		.mapbase	= 0xffe00000,
@@ -94,6 +155,8 @@ static struct platform_device sci_device
 static struct platform_device *sh7722_devices[] __initdata = {
 	&usbf_device,
 	&iic_device,
+	&veu_device,
+	&vpu_device,
 	&sci_device,
 };
 
--- 0001/arch/sh/kernel/cpu/sh4a/setup-sh7723.c
+++ work/arch/sh/kernel/cpu/sh4a/setup-sh7723.c	2008-05-20 17:19:05.000000000 +0900
@@ -12,6 +12,7 @@
 #include <linux/serial.h>
 #include <linux/mm.h>
 #include <linux/serial_sci.h>
+#include <linux/uio_platform.h>
 #include <asm/mmzone.h>
 
 static struct plat_sci_port sci_platform_data[] = {
@@ -73,9 +74,102 @@ static struct platform_device rtc_device
 	.resource	= rtc_resources,
 };
 
+static struct uio_platform_info veu0_platform_data = {
+	.name = "VEU",
+	.version = "0.0.1",
+	.memsize = 2 << 20,
+};
+
+static struct resource veu0_resources[] = {
+	[0] = {
+		.name	= "VEU0",
+		.start	= 0xfe920000,
+		.end	= 0xfe92027c,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= 54,
+		.end	= 54,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device veu0_device = {
+	.name		= "uio-platform",
+	.id		= 0,
+	.dev = {
+		.platform_data	= &veu0_platform_data,
+	},
+	.resource	= veu0_resources,
+	.num_resources	= ARRAY_SIZE(veu0_resources),
+};
+
+static struct uio_platform_info veu1_platform_data = {
+	.name = "VEU",
+	.version = "0.0.1",
+	.memsize = 2 << 20,
+};
+
+static struct resource veu1_resources[] = {
+	[0] = {
+		.name	= "VEU1",
+		.start	= 0xfe924000,
+		.end	= 0xfe92427c,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= 27,
+		.end	= 27,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device veu1_device = {
+	.name		= "uio-platform",
+	.id		= 1,
+	.dev = {
+		.platform_data	= &veu1_platform_data,
+	},
+	.resource	= veu1_resources,
+	.num_resources	= ARRAY_SIZE(veu1_resources),
+};
+
+static struct uio_platform_info vpu_platform_data = {
+	.name = "VPU",
+	.version = "0.0.1",
+	.memsize = 1 << 20,
+};
+
+static struct resource vpu_resources[] = {
+	[0] = {
+		.name	= "VPU",
+		.start	= 0xfe900000,
+		.end	= 0xfe902808,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= 60,
+		.end	= 60,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device vpu_device = {
+	.name		= "uio-platform",
+	.id		= 2,
+	.dev = {
+		.platform_data	= &vpu_platform_data,
+	},
+	.resource	= vpu_resources,
+	.num_resources	= ARRAY_SIZE(vpu_resources),
+};
+
 static struct platform_device *sh7723_devices[] __initdata = {
 	&sci_device,
 	&rtc_device,
+	&vpu_device,
+	&veu0_device,
+	&veu1_device,
 };
 
 static int __init sh7723_devices_setup(void)

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Hans J. Koch]

On Tue, May 20, 2008 at 07:51:32PM +0900, Magnus Damm wrote:

Hi Magnus,

> These patches implement a reusable UIO platform driver. 

Uwe Kleine-Koenig already submitted such a framework:

http://lkml.org/lkml/2008/5/20/94

It's his third version, and it looks good. I presume you didn't know
about his work. The main difference is that he leaves interrupt handling
to platform code. That might look strange (it did to me first), but it
has the advantage that you can put hardware dependent stuff in your
board support (which depends on hardware anyway).

Could you have a look at his patch and tell me if that does what you
need?

> This driver
> can be used to export hardware to user space, as long as the device
> is a) memory mapped and b) equipped with an unique IRQ.
> 
> The uio_platform driver gets all information through platform data,
> including address window information and IRQ number. The driver also
> supports assigning a contiguous piece of memory to each instance.
> This may be useful when the exported hardware blocks can bus master
> but requires physically contiguous memory.
> 
> There are not many surprises in the code if you are familiar with UIO,
> except for the interrupt handling. All UIO kernel drivers that I've
> seen so far have hardware specific interrupt acknowledge code in the
> interrupt handler. The uio_platform driver is different.
> 
> The interrupt handling code in uio_platform assumes the device is the
> only user of the assigned interrupt. 

Uwe's approach doesn't have this limitation.

> This may be rare in the PC world
> but for SuperH almost all interrupt sources are unique. Having an
> unique interrupt for the device allows the code to use disable_irq()
> and enable_irq() in kernel space and leave actual interrupt acknowledge
> to user space. That way we have no hardware specific code in the kernel.
> 
> Interrupts are of course serviced in kernel space by the uio_platform
> driver, but the uio_platform interrupt handler will simply disable the
> IRQ until next read() or poll() syscall. The uio_platform kernel driver
> assumes that the user space driver has taken care of acknowledging
> the interrupt before doing read() and waiting for events again. If no
> acknowledge has happened then an interrupt will occur again (since it's
> still pending) and the kernel interrupt handler will disable the IRQ
> again and unblock the user space process.
> 
> The last patch contains SuperH specific code that exports various
> multimedia acceleration blocks to userspace. The following processors
> and hardware blocks are exported for now:
> 
> sh7343: VPU
> sh7722: VPU, VEU
> sh7723: VPU, VEU, VEU
> 
> If anyone is interested then I have a proof of concept vidix driver
> for mplayer that is interfacing using UIO to the VEU on a sh7722 to
> provide accelerated color space conversion and stretching.

This sounds quite interesting. Unfortunately, I'm not familiar with the
SuperH architecture. Could you also do this with Uwe's approach?

I'm about to sign-off Uwe's patch, and we'll possibly have that in
mainline soon. I don't mind having a second "generic platform" driver,
but you'll need to have good technical arguments.

Thanks,
Hans

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Magnus Damm]

Hi Hans!

On Wed, May 21, 2008 at 6:07 AM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Tue, May 20, 2008 at 07:51:32PM +0900, Magnus Damm wrote:
>> These patches implement a reusable UIO platform driver.
>
> Uwe Kleine-Koenig already submitted such a framework:
>
> http://lkml.org/lkml/2008/5/20/94
>
> It's his third version, and it looks good. I presume you didn't know
> about his work. The main difference is that he leaves interrupt handling
> to platform code. That might look strange (it did to me first), but it
> has the advantage that you can put hardware dependent stuff in your
> board support (which depends on hardware anyway).

I was not aware of this driver, thanks for the pointer!

> Could you have a look at his patch and tell me if that does what you
> need?

The uio_pdrv driver doesn't do what I need at this point, though I may
be able to extend it with the following:
- Interrupt enable/disable code
- Physically contiguous memory support

The interrupt code may be placed in the board/cpu code, but I need to
share that code between multiple UIO driver instances. We want to use
the same UIO driver for many different processor models and hardware
blocks. Extending uio_pdrv driver with a chunk of physically
contiguous memory isn't a big deal though.

To be frank, I have my doubts in adding an extra forwarding-only
platform layer on top of UIO compared to using uio_register_device()
directly from the board code. I like that the platform layer is using
struct resource and handles resource ranges for us automatically, but
wouldn't it make more sense to extend the UIO core to always use
struct resource instead of struct uio_mem? I'd be happy to help out -
just point me in the right direction.

>> The interrupt handling code in uio_platform assumes the device is the
>> only user of the assigned interrupt.
>
> Uwe's approach doesn't have this limitation.

True, but the uio_pdrv driver is choosing to not deal with interrupts
at all. I'd like to have shared interrupt handling code. With my
driver, you just feed it io memory window parameters and an interrupt
number and off you go. No need for any callbacks.

>> If anyone is interested then I have a proof of concept vidix driver
>> for mplayer that is interfacing using UIO to the VEU on a sh7722 to
>> provide accelerated color space conversion and stretching.
>
> This sounds quite interesting. Unfortunately, I'm not familiar with the
> SuperH architecture. Could you also do this with Uwe's approach?

I could handle things by extending Uwe's uio_pdrv driver, but I still
need the enable_irq() callback. I wonder if it makes sense to let the
two drivers coexist side by side, since they are solving different
problems. I can rename my driver to uio_pdrv_unique_irq or something,
or maybe uio_superh.c. I dislike the latter since my driver doesn't do
anything SuperH specific and that I suspect it can be useful for other
architectures as well.

> I'm about to sign-off Uwe's patch, and we'll possibly have that in
> mainline soon. I don't mind having a second "generic platform" driver,
> but you'll need to have good technical arguments.

I'd like to have this driver upstream as well, and sharing the code
with the uio_pdrv driver is one way, but I suspect that adding another
reusable layer on top of that driver will just complicate things. The
uio-specific part of my patches is less than 200 lines of code. From
the top of my head I can think of at least 10 different SuperH
hardware devices that can reuse this driver.

Please let me know what you prefer and I'll update the code and repost.

Thanks for your help!

/ magnus

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Uwe Kleine-König]

Hello Magnus,

Magnus Damm wrote:
> Hi Hans!
> 
> On Wed, May 21, 2008 at 6:07 AM, Hans J. Koch <hjk@linutronix.de> wrote:
> > On Tue, May 20, 2008 at 07:51:32PM +0900, Magnus Damm wrote:
> >> These patches implement a reusable UIO platform driver.
> >
> > Uwe Kleine-Koenig already submitted such a framework:
> >
> > http://lkml.org/lkml/2008/5/20/94
> >
> > It's his third version, and it looks good. I presume you didn't know
> > about his work. The main difference is that he leaves interrupt handling
> > to platform code. That might look strange (it did to me first), but it
> > has the advantage that you can put hardware dependent stuff in your
> > board support (which depends on hardware anyway).
> 
> I was not aware of this driver, thanks for the pointer!
> 
> > Could you have a look at his patch and tell me if that does what you
> > need?
> 
> The uio_pdrv driver doesn't do what I need at this point, though I may
> be able to extend it with the following:
> - Interrupt enable/disable code
> - Physically contiguous memory support
> 
> The interrupt code may be placed in the board/cpu code, but I need to
> share that code between multiple UIO driver instances. We want to use
> the same UIO driver for many different processor models and hardware
> blocks.
What about adding uio_platform_handler (with a different name) to
uio_pdrv.c?
OTOH I don't see why you want to disable the irq.  Can you describe the
reason?

>         Extending uio_pdrv driver with a chunk of physically
> contiguous memory isn't a big deal though.
I wonder how you use that memory.  Isn't it just some kind of shared
memory?  If so, why not use normal shared memory?  Do you really need
that?

> To be frank, I have my doubts in adding an extra forwarding-only
> platform layer on top of UIO compared to using uio_register_device()
> directly from the board code. I like that the platform layer is using
> struct resource and handles resource ranges for us automatically, but
> wouldn't it make more sense to extend the UIO core to always use
> struct resource instead of struct uio_mem? I'd be happy to help out -
> just point me in the right direction.
That alone doesn't help.  You need a struct device to register a uio
device.  So a platform device is the straight forward approach.
 
> >> The interrupt handling code in uio_platform assumes the device is the
> >> only user of the assigned interrupt.
> >
> > Uwe's approach doesn't have this limitation.
> 
> True, but the uio_pdrv driver is choosing to not deal with interrupts
> at all. I'd like to have shared interrupt handling code. With my
> driver, you just feed it io memory window parameters and an interrupt
> number and off you go. No need for any callbacks.
In my eyes this isn't completly correct.  Just the way you specify your
handler is a bit different.  You can pass a handler via platform data
with my driver, too.
 
BTW, you don't need "depends on UIO" (because it's in a if UIO/endif
block) and "default n" (as this is the default anyhow).  See also 
http://thread.gmane.org/gmane.linux.kernel/663884/focus=683097

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Paul Mundt]

On Wed, May 21, 2008 at 08:49:38AM +0200, Uwe Kleine-K?nig wrote:
> Magnus Damm wrote:
> >         Extending uio_pdrv driver with a chunk of physically
> > contiguous memory isn't a big deal though.
> I wonder how you use that memory.  Isn't it just some kind of shared
> memory?  If so, why not use normal shared memory?  Do you really need
> that?
> 
Physically contiguous memory is a real requirement, especially for DMA.
I'm not sure what's confusing about that?

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Uwe Kleine-König]

Paul Mundt wrote:
> On Wed, May 21, 2008 at 08:49:38AM +0200, Uwe Kleine-K?nig wrote:
> > Magnus Damm wrote:
> > >         Extending uio_pdrv driver with a chunk of physically
> > > contiguous memory isn't a big deal though.
> > I wonder how you use that memory.  Isn't it just some kind of shared
> > memory?  If so, why not use normal shared memory?  Do you really need
> > that?
> > 
> Physically contiguous memory is a real requirement, especially for DMA.
> I'm not sure what's confusing about that?
I got that, yes.  The problem is I don't see how you can use it for DMA.
The physical address is stored in info->mem[$last].internal_addr and if
there is a way to access that variable from user space, I don't see it
and would appretiate a hint.  Sorry for not expressing my concern more
clear at the first go.  I hope it's understandable now.

@Magnus: Maybe you can provide the userspace part of the driver?
How is that mapping used there?

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Magnus Damm]

Hi Uwe!

Thanks for your email.

On Wed, May 21, 2008 at 3:49 PM, Uwe Kleine-König
<Uwe.Kleine-Koenig@digi.com> wrote:
> Magnus Damm wrote:
>> The uio_pdrv driver doesn't do what I need at this point, though I may
>> be able to extend it with the following:
>> - Interrupt enable/disable code
>> - Physically contiguous memory support
>>
>> The interrupt code may be placed in the board/cpu code, but I need to
>> share that code between multiple UIO driver instances. We want to use
>> the same UIO driver for many different processor models and hardware
>> blocks.
> What about adding uio_platform_handler (with a different name) to
> uio_pdrv.c?

I'm not sure if it will help. What would such function do? Please explain.

> OTOH I don't see why you want to disable the irq.  Can you describe the
> reason?

Most UIO kernel drivers today contain hardware device specific code to
acknowledge interrupts. In other words, most UIO interrupt handlers
touches some device specific bits so the interrupt gets deasserted.

My uio_platform driver handles interrupts in a different way. The
kernel UIO driver is not aware of the hardware device specific method
to acknowledge the interrupt, instead it simply disables the interrupt
and notifies user space which instead will acknowledge the interrupt.
Next time a read() or poll() call gets made, the interrupt is enabled
again.

This allows us to export a hardware device to user space and allow
user space to handle interrupts without knowing in kernel space how to
acknowledge interrupts.

>>         Extending uio_pdrv driver with a chunk of physically
>> contiguous memory isn't a big deal though.
> I wonder how you use that memory.  Isn't it just some kind of shared
> memory?  If so, why not use normal shared memory?  Do you really need
> that?

Yes, I need that to give the exported hardware device some physically
contiguous memory for DMA. At this point our hardware is missing IOMMU
so physically contiguous memory is needed.

>> To be frank, I have my doubts in adding an extra forwarding-only
>> platform layer on top of UIO compared to using uio_register_device()
>> directly from the board code. I like that the platform layer is using
>> struct resource and handles resource ranges for us automatically, but
>> wouldn't it make more sense to extend the UIO core to always use
>> struct resource instead of struct uio_mem? I'd be happy to help out -
>> just point me in the right direction.
> That alone doesn't help.  You need a struct device to register a uio
> device.  So a platform device is the straight forward approach.

I don't mind that you are using platform devices. Actually, I think
platform devices are great. We use them for all sorts of things on the
SuperH architecture. I'm trying to suggest that maybe it's a good idea
to change the UIO core code to use struct resource instead of struct
uio_mem. Or maybe that's not a good idea, I'm not sure.

>> >> The interrupt handling code in uio_platform assumes the device is the
>> >> only user of the assigned interrupt.
>> >
>> > Uwe's approach doesn't have this limitation.
>>
>> True, but the uio_pdrv driver is choosing to not deal with interrupts
>> at all. I'd like to have shared interrupt handling code. With my
>> driver, you just feed it io memory window parameters and an interrupt
>> number and off you go. No need for any callbacks.
> In my eyes this isn't completly correct.  Just the way you specify your
> handler is a bit different.  You can pass a handler via platform data
> with my driver, too.

I don't want to pass any handler. All devices share the same interrupt
handler, the only thing that differs between multiple uio_platform
devices on one system is memory window information and irq number.

> BTW, you don't need "depends on UIO" (because it's in a if UIO/endif
> block) and "default n" (as this is the default anyhow).  See also
> http://thread.gmane.org/gmane.linux.kernel/663884/focus=683097

Ah, thanks for pointing that out!

Thank you for your feedback!

/ magnus

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Magnus Damm]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

On Wed, May 21, 2008 at 5:05 PM, Uwe Kleine-König
<Uwe.Kleine-Koenig@digi.com> wrote:
> @Magnus: Maybe you can provide the userspace part of the driver?
> How is that mapping used there?

[Added Matsubara-san as CC]

Sure, here is a little test program. Have a look at "uio_mem". The
"address" member contains the physical address that can be used for
bus mastering DMA. Compare that to "iomem" which is the pointer to the
virtual memory area in user space.

Hope this helps!

/ magnus

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: veu_lab.c --]
[-- Type: text/x-csrc; name=veu_lab.c, Size: 7353 bytes --]

/*
 * sh7722 VEU scaling example using UIO - 20080521 Magnus Damm
 *
 * Derived from
 * "Simple tick timer through Sky TMU UIO driver" by Katsuya Matsubara
 */
#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/time.h>
#include <sys/ioctl.h>
#include <fcntl.h>
#include <linux/fb.h>

static int fgets_with_openclose(char *fname, char *buf, size_t maxlen) {
	FILE *fp;

	if ((fp = fopen(fname, "r")) != NULL) {
		fgets(buf, maxlen, fp);
		fclose(fp);
		return strlen(buf);
	} else {
		return -1;
	}
}

struct uio_device {
  char *name;
  char *path;
  int fd;
};

#define MAXUIOIDS  100
#define MAXNAMELEN 256

static int locate_uio_device(char *name, struct uio_device *udp)
{
	char fname[MAXNAMELEN], buf[MAXNAMELEN];
	int uio_id;

	for (uio_id = 0; uio_id < MAXUIOIDS; uio_id++) {
		sprintf(fname, "/sys/class/uio/uio%d/name", uio_id);
		if (fgets_with_openclose(fname, buf, MAXNAMELEN) < 0)
			continue;
		if (strncmp(name, buf, strlen(name)) == 0)
			break;
	}

	if (uio_id >= MAXUIOIDS)
		return -1;

	udp->name = strdup(buf);
	udp->path = strdup(fname);
	udp->path[strlen(udp->path) - 4] = '\0';

	sprintf(buf, "/dev/uio%d", uio_id);
	udp->fd = open(buf, O_RDWR|O_SYNC);

	if (udp->fd < 0) {
		perror("open");
		return -1;
	}

	return 0;
}

struct uio_map {
  unsigned long address;
  unsigned long size;
  void *iomem;
};

static int setup_uio_map(struct uio_device *udp, int nr, struct uio_map *ump)
{
	char fname[MAXNAMELEN], buf[MAXNAMELEN];
 
	sprintf(fname, "%s/maps/map%d/addr", udp->path, nr);
	if (fgets_with_openclose(fname, buf, MAXNAMELEN) <= 0)
		return -1;

	ump->address = strtoul(buf, NULL, 0);

	sprintf(fname, "%s/maps/map%d/size", udp->path, nr);
	if (fgets_with_openclose(fname, buf, MAXNAMELEN) <= 0)
		return -1;

	ump->size = strtoul(buf, NULL, 0);

	ump->iomem = mmap(0, ump->size,
			  PROT_READ|PROT_WRITE, MAP_SHARED,
			  udp->fd, nr * getpagesize());

	if (ump->iomem == MAP_FAILED)
		return -1;

	return 0;
}

struct fb_info {
  unsigned long width;
  unsigned long height;
  unsigned long bpp;
  unsigned long line_length;

  unsigned long address;
  unsigned long size;
};

static int get_fb_info(char *device, struct fb_info *fip)
{
	struct fb_var_screeninfo vinfo;
	struct fb_fix_screeninfo finfo;
	int fd;

	fd = open(device, O_RDWR);
	if (fd < 0) {
		perror("open");
		return -1;
	}

	if (ioctl(fd, FBIOGET_VSCREENINFO, &vinfo) == -1) {
		perror("ioctl(FBIOGET_VSCREENINFO)");
		return -1;
	}

	fip->width = vinfo.xres;
	fip->height = vinfo.yres;
	fip->bpp = vinfo.bits_per_pixel;

	if (ioctl(fd, FBIOGET_FSCREENINFO, &finfo) == -1) {
		perror("ioctl(FBIOGET_FSCREENINFO)");
		return -1;
	}

	fip->address = finfo.smem_start;
	fip->size = finfo.smem_len;
	fip->line_length = finfo.line_length;
	
	close(fd);
	return 0;
}

#define VESTR 0x00 /* start register */
#define VESWR 0x10 /* src: line length */
#define VESSR 0x14 /* src: image size */
#define VSAYR 0x18 /* src: y/rgb plane address */
#define VSACR 0x1c /* src: c plane address */
#define VBSSR 0x20 /* bundle mode register */
#define VEDWR 0x30 /* dst: line length */
#define VDAYR 0x34 /* dst: y/rgb plane address */
#define VDACR 0x38 /* dst: c plane address */
#define VTRCR 0x50 /* transform control */
#define VRFCR 0x54 /* resize scale */
#define VRFSR 0x58 /* resize clip */
#define VENHR 0x5c /* enhance */
#define VFMCR 0x70 /* filter mode */ 
#define VVTCR 0x74 /* lowpass vertical */
#define VHTCR 0x78 /* lowpass horizontal */
#define VAPCR 0x80 /* color match */
#define VECCR 0x84 /* color replace */
#define VAFXR 0x90 /* fixed mode */
#define VSWPR 0x94 /* swap */
#define VEIER 0xa0 /* interrupt mask */
#define VEVTR 0xa4 /* interrupt event */
#define VSTAR 0xb0 /* status */
#define VBSRR 0xb4 /* reset */

static unsigned long read_reg(struct uio_map *ump, int reg_nr)
{
	  volatile unsigned long *reg = ump->iomem + reg_nr;

	  return *reg;
}

static void write_reg(struct uio_map *ump, unsigned long value, int reg_nr)
{
	  volatile unsigned long *reg = ump->iomem + reg_nr;

	  *reg = value;
}

static void set_scale(struct uio_map *ump, int vertical,
		      int size_in, int size_out)
{
	unsigned long fixpoint, mant, frac, value;

	/* calculate FRAC and MANT */

	fixpoint = (4096 * (size_in - 1)) / (size_out + 1);
	mant = fixpoint / 4096;
	frac = fixpoint - (mant * 4096);

	if (frac & 0x07) {
		frac &= ~0x07;

		if (size_out > size_in)
			frac -= 8; /* round down if scaling up */
		else
			frac += 8; /* round up if scaling down */
	}

	/* set scale */
	
	value = read_reg(ump, VRFCR);
	
	if (vertical) {
		value &= ~0xffff0000;
  		value |= ((mant << 12) | frac) << 16;
	}
	else {
		value &= ~0xffff;
  		value |= (mant << 12) | frac;
	}
	
	write_reg(ump, value, VRFCR);

	/* set clip */

	value = read_reg(ump, VRFSR);

	if (vertical) {
		value &= ~0xffff0000;
  		value |= size_out << 16;
	}
	else {
		value &= ~0xffff;
  		value |= size_out;
	}
	
	write_reg(ump, value, VRFSR);
}

static int foo(void)
{
	struct fb_info fbi;
	struct uio_device uio_dev;
	struct uio_map uio_mmio, uio_mem;
	unsigned long addr;
	int src_w, src_h, src_bpp;
	int dst_w, dst_h;
	int ret;

	ret = get_fb_info("/dev/fb0", &fbi);
	if (ret < 0)
		return ret;

	printf("using %lux%lu %ld bpp framebuffer at 0x%08lx (0x%08lx).\n",
	       fbi.width, fbi.height, fbi.bpp, fbi.address, fbi.size);

	ret = locate_uio_device("VEU", &uio_dev);
	if (ret < 0)
		return ret;
	
	printf("found matching UIO device at %s\n", uio_dev.path);

	ret = setup_uio_map(&uio_dev, 0, &uio_mmio);
	if (ret < 0)
		return ret;

	ret = setup_uio_map(&uio_dev, 1, &uio_mem);
	if (ret < 0)
		return ret;

	printf("VSTAR = 0x%lx\n", read_reg(&uio_mmio, VSTAR));
	printf("VEVTR = 0x%lx\n", read_reg(&uio_mmio, VEVTR));

	/* reset VEU */

	printf("VBSRR = 0x%lx\n", read_reg(&uio_mmio, VBSRR));
	write_reg(&uio_mmio, 0x100, VBSRR);
	printf("VBSRR(2) = 0x%lx\n", read_reg(&uio_mmio, VBSRR));

	/* destination: get from fb */

	dst_w = fbi.width;
	dst_h = fbi.height;

	/* source bitmap */

	src_w = 32;
	src_h = 32;
	src_bpp = 16;

 new_color:

	{
	  struct timeval tv;
	  int k;
	  unsigned char *buf = uio_mem.iomem;

	  gettimeofday(&tv, NULL);

	  for (k = 0; k < (src_w * src_h * (src_bpp / 8)); k++)
		buf[k] = k ^ (tv.tv_sec + tv.tv_usec);
	}

	addr = fbi.address;
	addr += (fbi.bpp / 8) * ((fbi.width - dst_w) / 2);
	addr += fbi.line_length * ((fbi.height - dst_h) / 2);

 again:
	write_reg(&uio_mmio, src_w * (src_bpp / 8), VESWR);
	write_reg(&uio_mmio, src_w | (src_h << 16), VESSR);
	write_reg(&uio_mmio, uio_mem.address, VSAYR);
	write_reg(&uio_mmio, 0, VSACR); /* unused in case of RGB */
	write_reg(&uio_mmio, 0, VBSSR); /* not using bundle mode */

	write_reg(&uio_mmio, fbi.line_length, VEDWR);
	write_reg(&uio_mmio, addr, VDAYR);
	write_reg(&uio_mmio, 0, VDACR); /* unused in case of RGB */

	write_reg(&uio_mmio, (6 << 16) | (3 << 8) | 1, VTRCR);

	set_scale(&uio_mmio, 0, src_w, dst_w);
	set_scale(&uio_mmio, 1, src_h, dst_h);

	write_reg(&uio_mmio, 1, VEIER); /* enable interrupt */
	write_reg(&uio_mmio, 1, VESTR);

	/* Wait for an interrupt */
	{
		unsigned long n_pending;
		read(uio_dev.fd, &n_pending, sizeof(u_long));
	}

	write_reg(&uio_mmio, 0x100, VEVTR); /* ack int, write 0 to bit 0 */

	goto again;
	goto new_color;
} 

int main(int argc, char ** argv)
{
	foo();

	return 0;
}

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Uwe Kleine-König]

Hello,

Magnus Damm wrote:
> On Wed, May 21, 2008 at 5:05 PM, Uwe Kleine-König
> <Uwe.Kleine-Koenig@digi.com> wrote:
> > @Magnus: Maybe you can provide the userspace part of the driver?
> > How is that mapping used there?
> 
> [Added Matsubara-san as CC]
> 
> Sure, here is a little test program. Have a look at "uio_mem". The
> "address" member contains the physical address that can be used for
> bus mastering DMA. Compare that to "iomem" which is the pointer to the
> virtual memory area in user space.
> 
> Hope this helps!
Yes it does.  I thought the physical address is stored in internal_addr
and the virtual in addr, but it's the other way round.  Thanks.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Uwe Kleine-König]

Magnus Damm wrote:
> Hi Uwe!
> 
> Thanks for your email.
> 
> On Wed, May 21, 2008 at 3:49 PM, Uwe Kleine-König
> <Uwe.Kleine-Koenig@digi.com> wrote:
> > Magnus Damm wrote:
> >> The uio_pdrv driver doesn't do what I need at this point, though I may
> >> be able to extend it with the following:
> >> - Interrupt enable/disable code
> >> - Physically contiguous memory support
> >>
> >> The interrupt code may be placed in the board/cpu code, but I need to
> >> share that code between multiple UIO driver instances. We want to use
> >> the same UIO driver for many different processor models and hardware
> >> blocks.
> > What about adding uio_platform_handler (with a different name) to
> > uio_pdrv.c?
> 
> I'm not sure if it will help. What would such function do? Please explain.
Just add irq_disabled to struct uio_platdata and define

	irqreturn_t uio_pdrv_disirq(int irq, struct uio_info *dev_info)
	{
		struct uio_platdata *pdata = container_of(dev_info, struct uio_platdata, uio_info);

		disable_irq(irq);
		pdata->irq_disabled = 1;
		return IRQ_HANDLED;
	}
	EXPORT_SYMBOL(uio_pdrv_disirq);

	void uio_pdrv_enirq(struct uio_info *dev_info)
	{
		...
	}
	EXPORT_SYMBOL(uio_pdrv_enirq);

and then you can do

	info->handler = uio_pdrv_disirq;
	info->enable_irq = uio_pdrv_enirq;

in the arch specific code.  I just realize that you need to compile UIO
statically then :-(

IMHO something like prep_read_poll is a better name than enable_irq for
the new member of uio_info.

> > OTOH I don't see why you want to disable the irq.  Can you describe the
> > reason?
> 
> Most UIO kernel drivers today contain hardware device specific code to
> acknowledge interrupts. In other words, most UIO interrupt handlers
> touches some device specific bits so the interrupt gets deasserted.
> 
> My uio_platform driver handles interrupts in a different way. The
> kernel UIO driver is not aware of the hardware device specific method
> to acknowledge the interrupt, instead it simply disables the interrupt
> and notifies user space which instead will acknowledge the interrupt.
> Next time a read() or poll() call gets made, the interrupt is enabled
> again.
> 
> This allows us to export a hardware device to user space and allow
> user space to handle interrupts without knowing in kernel space how to
> acknowledge interrupts.
OK, got it.  The down-side is that you can only get a single interrupt
between two calls to read() (or poll()).  So you might or might not
loose information.  And you might run into problems if your device or
your interrupt goes berserk as your handler always returns IRQ_HANDLED.
With a functional handler you can rely on existing mechanisms in the
kernel.
 
> >> To be frank, I have my doubts in adding an extra forwarding-only
> >> platform layer on top of UIO compared to using uio_register_device()
> >> directly from the board code. I like that the platform layer is using
> >> struct resource and handles resource ranges for us automatically, but
> >> wouldn't it make more sense to extend the UIO core to always use
> >> struct resource instead of struct uio_mem? I'd be happy to help out -
> >> just point me in the right direction.
> > That alone doesn't help.  You need a struct device to register a uio
> > device.  So a platform device is the straight forward approach.
> 
> I don't mind that you are using platform devices. Actually, I think
> platform devices are great. We use them for all sorts of things on the
> SuperH architecture. I'm trying to suggest that maybe it's a good idea
> to change the UIO core code to use struct resource instead of struct
> uio_mem. Or maybe that's not a good idea, I'm not sure.
struct resource alone doesn't provide enough information.  At least
memtype is needed.  And you don't need the pointers *parent, *sibling,
*child, so in my eyes it's fine to have a dedicated structure for uio.
 
> >> >> The interrupt handling code in uio_platform assumes the device is the
> >> >> only user of the assigned interrupt.
> >> >
> >> > Uwe's approach doesn't have this limitation.
> >>
> >> True, but the uio_pdrv driver is choosing to not deal with interrupts
> >> at all. I'd like to have shared interrupt handling code. With my
> >> driver, you just feed it io memory window parameters and an interrupt
> >> number and off you go. No need for any callbacks.
> > In my eyes this isn't completly correct.  Just the way you specify your
> > handler is a bit different.  You can pass a handler via platform data
> > with my driver, too.
> 
> I don't want to pass any handler. All devices share the same interrupt
> handler, the only thing that differs between multiple uio_platform
> devices on one system is memory window information and irq number.
See above.  That would be the cost to share code with "my" driver.

All in all I'm not conviced that it's a good idea to use the irq_disable
trick to save acking in kernel space.  This doesn't need to stop you
doing it that way of course.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Magnus Damm]

On Wed, May 21, 2008 at 6:25 PM, Uwe Kleine-König
<Uwe.Kleine-Koenig@digi.com> wrote:
> Magnus Damm wrote:
>> On Wed, May 21, 2008 at 3:49 PM, Uwe Kleine-König
>> > What about adding uio_platform_handler (with a different name) to
>> > uio_pdrv.c?
>>
>> I'm not sure if it will help. What would such function do? Please explain.
> Just add irq_disabled to struct uio_platdata and define
>
>        irqreturn_t uio_pdrv_disirq(int irq, struct uio_info *dev_info)
>        {
>                struct uio_platdata *pdata = container_of(dev_info, struct uio_platdata, uio_info);
>
>                disable_irq(irq);
>                pdata->irq_disabled = 1;
>                return IRQ_HANDLED;
>        }
>        EXPORT_SYMBOL(uio_pdrv_disirq);
>
>        void uio_pdrv_enirq(struct uio_info *dev_info)
>        {
>                ...
>        }
>        EXPORT_SYMBOL(uio_pdrv_enirq);
>
> and then you can do
>
>        info->handler = uio_pdrv_disirq;
>        info->enable_irq = uio_pdrv_enirq;
>
> in the arch specific code.  I just realize that you need to compile UIO
> statically then :-(

I understand now. Thanks for the clear description.

What about letting the uio_pdrv code override info->handler and
info->enable_irq with the above functions if info->handler is NULL?
That would be one step closer to a shared driver in my opinion. And it
would remove the need for symbol exports and solve the
static-compile-only issue.

The physically contiguous memory issue still needs to be solved
somehow though. What about using struct resouce flagged as
IORESOURCE_DMA to pass the amount of memory that should be allocated?

> IMHO something like prep_read_poll is a better name than enable_irq for
> the new member of uio_info.

That's fine.

>> > OTOH I don't see why you want to disable the irq.  Can you describe the
>> > reason?
>>
>> Most UIO kernel drivers today contain hardware device specific code to
>> acknowledge interrupts. In other words, most UIO interrupt handlers
>> touches some device specific bits so the interrupt gets deasserted.
>>
>> My uio_platform driver handles interrupts in a different way. The
>> kernel UIO driver is not aware of the hardware device specific method
>> to acknowledge the interrupt, instead it simply disables the interrupt
>> and notifies user space which instead will acknowledge the interrupt.
>> Next time a read() or poll() call gets made, the interrupt is enabled
>> again.
>>
>> This allows us to export a hardware device to user space and allow
>> user space to handle interrupts without knowing in kernel space how to
>> acknowledge interrupts.
> OK, got it.  The down-side is that you can only get a single interrupt
> between two calls to read() (or poll()).  So you might or might not
> loose information.  And you might run into problems if your device or
> your interrupt goes berserk as your handler always returns IRQ_HANDLED.
> With a functional handler you can rely on existing mechanisms in the
> kernel.

I agree that I only get a single interrupt, but I'm not agreeing
regarding the problems. =)

In my mind, disabling interrupts and acking them from user space only
leads to increased interrupt latencies. People may dislike increased
interrupt latencies, but if so they shouldn't have their driver in
user space. And you may of course choose to ack interrupts in kernel
space and queue information there which user space later reads out.
But that sounds more like a specialized kernel driver. And that is not
what i'm trying to do.

Regarding loosing information, if your hardware device can't cope with
long latencies and drops things on the floor then improve your
latency, increase buffer size or design better hardware. Also, I don't
think the interrupt can go berserk since it will be disabled directly
by the interrupt handler.

>> >> To be frank, I have my doubts in adding an extra forwarding-only
>> >> platform layer on top of UIO compared to using uio_register_device()
>> >> directly from the board code. I like that the platform layer is using
>> >> struct resource and handles resource ranges for us automatically, but
>> >> wouldn't it make more sense to extend the UIO core to always use
>> >> struct resource instead of struct uio_mem? I'd be happy to help out -
>> >> just point me in the right direction.
>> > That alone doesn't help.  You need a struct device to register a uio
>> > device.  So a platform device is the straight forward approach.
>>
>> I don't mind that you are using platform devices. Actually, I think
>> platform devices are great. We use them for all sorts of things on the
>> SuperH architecture. I'm trying to suggest that maybe it's a good idea
>> to change the UIO core code to use struct resource instead of struct
>> uio_mem. Or maybe that's not a good idea, I'm not sure.
> struct resource alone doesn't provide enough information.  At least
> memtype is needed.  And you don't need the pointers *parent, *sibling,
> *child, so in my eyes it's fine to have a dedicated structure for uio.

Maybe the flags member of struct resource together with IORESOURCE_xxx
can be used instead of memtype. But there is no point in changing
things just for the sake of it, so it is fine as-is in my opinion.

Thank you!

/ magnus

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Uwe Kleine-König]

Magnus Damm wrote:
> On Wed, May 21, 2008 at 6:25 PM, Uwe Kleine-König
> <Uwe.Kleine-Koenig@digi.com> wrote:
> > Magnus Damm wrote:
> >> On Wed, May 21, 2008 at 3:49 PM, Uwe Kleine-König
> >> > What about adding uio_platform_handler (with a different name) to
> >> > uio_pdrv.c?
> >>
> >> I'm not sure if it will help. What would such function do? Please explain.
> > Just add irq_disabled to struct uio_platdata and define
> >
> >        irqreturn_t uio_pdrv_disirq(int irq, struct uio_info *dev_info)
> >        {
> >                struct uio_platdata *pdata = container_of(dev_info, struct uio_platdata, uio_info);
> >
> >                disable_irq(irq);
> >                pdata->irq_disabled = 1;
> >                return IRQ_HANDLED;
> >        }
> >        EXPORT_SYMBOL(uio_pdrv_disirq);
> >
> >        void uio_pdrv_enirq(struct uio_info *dev_info)
> >        {
> >                ...
> >        }
> >        EXPORT_SYMBOL(uio_pdrv_enirq);
> >
> > and then you can do
> >
> >        info->handler = uio_pdrv_disirq;
> >        info->enable_irq = uio_pdrv_enirq;
> >
> > in the arch specific code.  I just realize that you need to compile UIO
> > statically then :-(
> 
> I understand now. Thanks for the clear description.
> 
> What about letting the uio_pdrv code override info->handler and
> info->enable_irq with the above functions if info->handler is NULL?
... if both info->handler and info->prep_read_poll are NULL and
info->irq >= 0.

> That would be one step closer to a shared driver in my opinion. And it
> would remove the need for symbol exports and solve the
> static-compile-only issue.
> 
> The physically contiguous memory issue still needs to be solved
> somehow though. What about using struct resouce flagged as
> IORESOURCE_DMA to pass the amount of memory that should be allocated?
I'm not sure that solving that problem in uio_pdrv is the right
approach.  Other uio drivers might have the same problem, so better
allow the userspace driver to allocate some memory in a more generic
way?
 
> >> My uio_platform driver handles interrupts in a different way. The
> >> kernel UIO driver is not aware of the hardware device specific method
> >> to acknowledge the interrupt, instead it simply disables the interrupt
> >> and notifies user space which instead will acknowledge the interrupt.
> >> Next time a read() or poll() call gets made, the interrupt is enabled
> >> again.
> >>
> >> This allows us to export a hardware device to user space and allow
> >> user space to handle interrupts without knowing in kernel space how to
> >> acknowledge interrupts.
> > OK, got it.  The down-side is that you can only get a single interrupt
> > between two calls to read() (or poll()).  So you might or might not
> > loose information.  And you might run into problems if your device or
> > your interrupt goes berserk as your handler always returns IRQ_HANDLED.
> > With a functional handler you can rely on existing mechanisms in the
> > kernel.
> 
> I agree that I only get a single interrupt, but I'm not agreeing
> regarding the problems. =)
> 
> In my mind, disabling interrupts and acking them from user space only
> leads to increased interrupt latencies. People may dislike increased
> interrupt latencies, but if so they shouldn't have their driver in
> user space. And you may of course choose to ack interrupts in kernel
> space and queue information there which user space later reads out.
> But that sounds more like a specialized kernel driver. And that is not
> what i'm trying to do.
> 
> Regarding loosing information, if your hardware device can't cope with
> long latencies and drops things on the floor then improve your
> latency, increase buffer size or design better hardware. Also, I don't
> think the interrupt can go berserk since it will be disabled directly
> by the interrupt handler.
Assume your irq is stuck at its active level.  Normally the irq is
then disabled after some time.  You can handle that in your userspace
driver, but with acking in kernel space and returning IRQ_NONE or
IRQ_HANDLED you get it for free.  Nevertheless, go on.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Magnus Damm]

On Wed, May 21, 2008 at 8:04 PM, Uwe Kleine-König
<Uwe.Kleine-Koenig@digi.com> wrote:
> Magnus Damm wrote:
>> What about letting the uio_pdrv code override info->handler and
>> info->enable_irq with the above functions if info->handler is NULL?
> ... if both info->handler and info->prep_read_poll are NULL and
> info->irq >= 0.

Even better! =)

>> The physically contiguous memory issue still needs to be solved
>> somehow though. What about using struct resouce flagged as
>> IORESOURCE_DMA to pass the amount of memory that should be allocated?
> I'm not sure that solving that problem in uio_pdrv is the right
> approach.  Other uio drivers might have the same problem, so better
> allow the userspace driver to allocate some memory in a more generic
> way?

I don't think there is any generic way for a user space driver to
allocate physically contiguous memory. If such way exists then we
should use that instead of course. Recommendations anyone?

>> Regarding loosing information, if your hardware device can't cope with
>> long latencies and drops things on the floor then improve your
>> latency, increase buffer size or design better hardware. Also, I don't
>> think the interrupt can go berserk since it will be disabled directly
>> by the interrupt handler.
> Assume your irq is stuck at its active level.  Normally the irq is
> then disabled after some time.  You can handle that in your userspace
> driver, but with acking in kernel space and returning IRQ_NONE or
> IRQ_HANDLED you get it for free.  Nevertheless, go on.

Ok, so normally if the irq is stuck as asserted then it gets disabled
after some time. In my case it gets disabled directly so see it as a
feature. =)

Would you like to fold in the irq_handler and irq_enable function in
your patch, or would you like me to make a patch that fits on top of
your latest version?

Thanks for your help!

/ magnus

Re: [PATCH 01/03] uio: Add enable_irq() callback

[Magnus Damm]

On Tue, May 20, 2008 at 7:51 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> Add enable_irq() callback to struct uio_info. This callback is needed by
> the uio_platform driver so interrupts can be enabled before blocking.

We can most likely use a single uio platform driver if this patch is
acceptable. Any NAKs?

Thanks,

/ magnus

Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

[Uwe Kleine-König]

Magnus Damm wrote:
> Would you like to fold in the irq_handler and irq_enable function in
> your patch, or would you like me to make a patch that fits on top of
> your latest version?
I would prefer the latter, because my patch already has acks and is
complete as such.  Moreover your suggestion needs the irq_enable patch.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 01/03] uio: Add enable_irq() callback

[Hans J. Koch]

On Wed, May 21, 2008 at 08:58:24PM +0900, Magnus Damm wrote:
> On Tue, May 20, 2008 at 7:51 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> > Add enable_irq() callback to struct uio_info. This callback is needed by
> > the uio_platform driver so interrupts can be enabled before blocking.
> 
> We can most likely use a single uio platform driver if this patch is
> acceptable. Any NAKs?

Yes. Your approach only allows enabling interrupts, but not disabling
them. And I don't like that it is not possible for generic userspace
tools to find out if a UIO device has this auto-irq-enabling capability
or not.

I just posted a patch that allows enabling _and_ disabling of irqs from
userspace by writing 0 or 1 to /dev/uioX. I've CCed you, could you
please test? If this doesn't do what you need, please let me know.

Thanks,
Hans

Re: [PATCH 01/03] uio: Add enable_irq() callback

[Magnus Damm]

Hi Hans,

On Fri, May 23, 2008 at 5:18 AM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Wed, May 21, 2008 at 08:58:24PM +0900, Magnus Damm wrote:
>> On Tue, May 20, 2008 at 7:51 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> > Add enable_irq() callback to struct uio_info. This callback is needed by
>> > the uio_platform driver so interrupts can be enabled before blocking.
>>
>> We can most likely use a single uio platform driver if this patch is
>> acceptable. Any NAKs?
>
> Yes. Your approach only allows enabling interrupts, but not disabling
> them. And I don't like that it is not possible for generic userspace
> tools to find out if a UIO device has this auto-irq-enabling capability
> or not.

Thanks for your effort. I understand that you need to enable and
disable interrupts from user space, but that's a bit different from
what I want to do. I just want interrupts to be enabled before I do
read() or poll().

Also, adding the capability of disabling and enabling interrupts from
user space seems a bit error prone to me. Mainly since user space then
needs to know that the interrupt handler in kernel space can cope with
such changes. Not such a clean interface IMO. OTOH you may need that
to cope with some broken hardware.

But why does user space need to know if the auto-irq-enabling function
is there or not? If the user is interfacing to the wrong kernel UIO
driver with wrong behavior then he has obviously done something wrong.
Knowing if auto-irq-enabling is there from user space isn't going to
save users from themselves. They can and will mix and match things in
all sorts of wrong ways anyway.

> I just posted a patch that allows enabling _and_ disabling of irqs from
> userspace by writing 0 or 1 to /dev/uioX. I've CCed you, could you
> please test? If this doesn't do what you need, please let me know.

I'm sure your patch or the ioctl suggestion both allow re-enabling
interrupts from user space. That's great, but both of them add extra
syscall overhead compared to my suggestion. They also make the user
space interface in user space part of the driver a bit more
complicated.

I do understand that you don't want to mess up your UIO kernel
callbacks by introducing just merging new ones all the time. OTOH, my
patch is just a few lines. Is introducing one extra syscall good
enough performance wise?

Thanks for your help!

/ magnus

Re: [PATCH 01/03] uio: Add enable_irq() callback

[Hans J. Koch]

Am Fri, 23 May 2008 10:24:42 +0900
schrieb "Magnus Damm" <magnus.damm@gmail.com>:

> Hi Hans,
> 
> On Fri, May 23, 2008 at 5:18 AM, Hans J. Koch <hjk@linutronix.de>
> wrote:
> > On Wed, May 21, 2008 at 08:58:24PM +0900, Magnus Damm wrote:
> >> On Tue, May 20, 2008 at 7:51 PM, Magnus Damm
> >> <magnus.damm@gmail.com> wrote:
> >> > Add enable_irq() callback to struct uio_info. This callback is
> >> > needed by the uio_platform driver so interrupts can be enabled
> >> > before blocking.
> >>
> >> We can most likely use a single uio platform driver if this patch
> >> is acceptable. Any NAKs?
> >
> > Yes. Your approach only allows enabling interrupts, but not
> > disabling them. And I don't like that it is not possible for
> > generic userspace tools to find out if a UIO device has this
> > auto-irq-enabling capability or not.
> 
> Thanks for your effort. I understand that you need to enable and
> disable interrupts from user space, but that's a bit different from
> what I want to do. I just want interrupts to be enabled before I do
> read() or poll().

Are you sure this works cleanly? You usually do a read immediately
after poll, so you might get two interrupts when waiting with select().

> 
> Also, adding the capability of disabling and enabling interrupts from
> user space seems a bit error prone to me. Mainly since user space then
> needs to know that the interrupt handler in kernel space can cope with
> such changes. 

No, it doesn't. If the kernel driver doesn't implement the irqcontrol()
handler, write attempts will return an error.

> Not such a clean interface IMO. OTOH you may need that
> to cope with some broken hardware.

All of this talk is _only_ about broken hardware. Decent hardware has
seperate IRQ mask and status registers, in which case userspace has no
problems at all to deal with several internal interrupt sources of the
chip. We only need all this for chips where it is not possible to mask
an IRQ through a mappable register or (more often) where acknowledging
the interrupt also clears the status register so that userspace has no
way of knowing what the source of the interrupt was. The latter applies
only to chips with more than one internal irq source.

> 
> But why does user space need to know if the auto-irq-enabling function
> is there or not?

It doesn't, I just find it nice to have such a flag displayed in lsuio.

> If the user is interfacing to the wrong kernel UIO
> driver with wrong behavior then he has obviously done something wrong.
> Knowing if auto-irq-enabling is there from user space isn't going to
> save users from themselves. They can and will mix and match things in
> all sorts of wrong ways anyway.

Agreed.

> 
> > I just posted a patch that allows enabling _and_ disabling of irqs
> > from userspace by writing 0 or 1 to /dev/uioX. I've CCed you, could
> > you please test? If this doesn't do what you need, please let me
> > know.
> 
> I'm sure your patch or the ioctl suggestion both allow re-enabling
> interrupts from user space. That's great, but both of them add extra
> syscall overhead compared to my suggestion. They also make the user
> space interface in user space part of the driver a bit more
> complicated.

UIO deals with two things, device memory and interrupts. We have mmap()
for mem and read() for waiting for an irq. This looks clean and logical
to me:

1) mmap() => device memory
2) read()/write() => device irqs

> 
> I do understand that you don't want to mess up your UIO kernel
> callbacks by introducing just merging new ones all the time. OTOH, my
> patch is just a few lines. Is introducing one extra syscall good
> enough performance wise?

This doesn't seem to be a problem, really. This write() is straight
forward, without any wait queues etc, so what?

Thanks,
Hans