[PATCH 0/2] Add PRUSS UIO driver support

[PATCH 0/2] Add PRUSS UIO driver support

[Pratheesh Gangadhar]

This patch series add support for PRUSS (Programmable Real-time Unit Sub System) UIO driver in Texas Instruments DA850, AM18xx and OMAPL1-38 processors. PRUSS is programmable RISC core which can be used to implement Soft IPs (eg:- DMA, CAN, UART,SmartCard) and Industrial communications data link layers (eg:- PROFIBUS). UIO driver exposes PRUSS resources like memory and interrupts to user space application.PRUSS UIO application API can be used to control PRUs in PRUSS, setup PRU INTC, load firmware to PRUs and implement IPC between Host processor and PRUs. More information on PRUSS and UIO linux user space API available in the links below

http://processors.wiki.ti.com/index.php/Programmable_Realtime_Unit_Subsystem
http://processors.wiki.ti.com/index.php/PRU_Linux_Application_Loader
http://processors.wiki.ti.com/index.php/PRU_Linux_Application_Loader_API_Guide 

PRUSS UIO driver support patch applies on top of Linus's tree latest.
Both patches applies on top of linux-davinci tree latest. 

Pratheesh Gangadhar (2):
  PRUSS UIO driver support
  Defines DA850/AM18xx/OMAPL1-38 SOC resources used by PRUSS UIO driver

 arch/arm/mach-davinci/board-da850-evm.c    |    4 +
 arch/arm/mach-davinci/da850.c              |   35 ++++
 arch/arm/mach-davinci/devices-da8xx.c      |   73 ++++++++
 arch/arm/mach-davinci/include/mach/da8xx.h |    3 +
 drivers/uio/Kconfig                        |   10 +
 drivers/uio/Makefile                       |    1 +
 drivers/uio/uio_pruss.c                    |  250 ++++++++++++++++++++++++++++
 7 files changed, 376 insertions(+), 0 deletions(-)
 create mode 100644 drivers/uio/uio_pruss.c

[PATCH 1/2] PRUSS UIO driver support

[Pratheesh Gangadhar]

Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>

This patch implements PRUSS (Programmable Real-time Unit Sub System)
UIO driver which exports SOC resources associated with PRUSS like
I/O, memories and IRQs to user space. PRUSS is dual 32-bit RISC
processors which is efficient in performing embedded tasks that
require manipulation of packed memory mapped data structures and
efficient in handling system events that have tight real time
constraints. This driver is currently supported on Texas Instruments
DA850, AM18xx and OMAPL1-38 devices.
For example, PRUSS runs firmware for real-time critical industrial
communication data link layer and communicates with application stack
running in user space via shared memory and IRQs.
---
 drivers/uio/Kconfig     |   10 ++
 drivers/uio/Makefile    |    1 +
 drivers/uio/uio_pruss.c |  250 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 261 insertions(+), 0 deletions(-)
 create mode 100644 drivers/uio/uio_pruss.c

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index bb44079..631ffe3 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -94,4 +94,14 @@ config UIO_NETX
 	  To compile this driver as a module, choose M here; the module
 	  will be called uio_netx.
 
+config UIO_PRUSS
+	tristate "Texas Instruments PRUSS driver"
+	depends on ARCH_DAVINCI_DA850
+	default n
+	help
+	  PRUSS driver for OMAPL138/DA850/AM18XX devices
+	  PRUSS driver requires user space components
+	  To compile this driver as a module, choose M here: the module
+	  will be called uio_pruss.
+
 endif
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index 18fd818..d4dd9a5 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_UIO_AEC)	+= uio_aec.o
 obj-$(CONFIG_UIO_SERCOS3)	+= uio_sercos3.o
 obj-$(CONFIG_UIO_PCI_GENERIC)	+= uio_pci_generic.o
 obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
+obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
diff --git a/drivers/uio/uio_pruss.c b/drivers/uio/uio_pruss.c
new file mode 100644
index 0000000..5ae78a5
--- /dev/null
+++ b/drivers/uio/uio_pruss.c
@@ -0,0 +1,250 @@
+/*
+ * Programmable Real-Time Unit Sub System (PRUSS) UIO driver (uio_pruss)
+ *
+ * This driver exports PRUSS host event out interrupts and PRUSS, L3 RAM,
+ * and DDR RAM to user space for applications interacting with PRUSS firmware 
+ *
+ * Copyright (C) 2010-11 Texas Instruments Incorporated - http://www.ti.com/
+ * 
+ * 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/module.h>
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/slab.h>
+
+#define DRV_NAME "pruss"
+#define DRV_VERSION "0.50"
+
+/*
+ * Host event IRQ numbers from PRUSS 	
+ * 3 PRU_EVTOUT0 PRUSS Interrupt
+ * 4 PRU_EVTOUT1 PRUSS Interrupt
+ * 5 PRU_EVTOUT2 PRUSS Interrupt
+ * 6 PRU_EVTOUT3 PRUSS Interrupt
+ * 7 PRU_EVTOUT4 PRUSS Interrupt
+ * 8 PRU_EVTOUT5 PRUSS Interrupt
+ * 9 PRU_EVTOUT6 PRUSS Interrupt
+ * 10 PRU_EVTOUT7 PRUSS Interrupt
+*/
+
+#define MAX_PRUSS_EVTOUT_INSTANCE	(8)
+
+static struct clk *pruss_clk;
+static struct uio_info *info[MAX_PRUSS_EVTOUT_INSTANCE];
+static void *ddr_virt_addr;
+static dma_addr_t ddr_phy_addr;
+
+static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
+{
+	return IRQ_HANDLED;
+}
+
+static int __devinit pruss_probe(struct platform_device *dev)
+{
+	int ret = -ENODEV;
+	int count = 0;
+	struct resource *regs_pruram, *regs_l3ram, *regs_ddr;
+	char *string;
+
+	/* Power on PRU in case its not done as part of boot-loader */
+	pruss_clk = clk_get(&dev->dev, "pruss");
+	if (IS_ERR(pruss_clk)) {
+		dev_err(&dev->dev, "Failed to get clock\n");
+		ret = PTR_ERR(pruss_clk);
+		pruss_clk = NULL;
+		return ret;
+	} else {
+		clk_enable(pruss_clk);
+	}
+
+	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
+		info[count] = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
+		if (!info[count])
+			return -ENOMEM;
+	}
+
+	regs_pruram = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (!regs_pruram) {
+		dev_err(&dev->dev, "No memory resource specified\n");
+		goto out_free;
+	}
+
+	regs_l3ram = platform_get_resource(dev, IORESOURCE_MEM, 1);
+	if (!regs_l3ram) {
+		dev_err(&dev->dev, "No memory resource specified\n");
+		goto out_free;
+	}
+
+	regs_ddr = platform_get_resource(dev, IORESOURCE_MEM, 2);
+	if (!regs_ddr) {
+		dev_err(&dev->dev, "No memory resource specified\n");
+		goto out_free;
+	}
+	ddr_virt_addr =
+	    dma_alloc_coherent(&dev->dev, regs_ddr->end - regs_ddr->start + 1,
+			       &ddr_phy_addr, GFP_KERNEL | GFP_DMA);
+	if (!ddr_virt_addr) {
+		dev_err(&dev->dev, "Could not allocate external memory\n");
+		goto out_free;
+	}
+
+	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
+		info[count]->mem[0].addr = regs_pruram->start;
+		info[count]->mem[0].size =
+		    regs_pruram->end - regs_pruram->start + 1;
+		if (!info[count]->mem[0].addr
+		    || !(info[count]->mem[0].size - 1)) {
+			dev_err(&dev->dev, "Invalid memory resource\n");
+			break;
+		}
+		info[count]->mem[0].internal_addr =
+		    ioremap(regs_pruram->start, info[count]->mem[0].size);
+		if (!info[count]->mem[0].internal_addr) {
+			dev_err(&dev->dev,
+				"Can't remap memory address range\n");
+			break;
+		}
+		info[count]->mem[0].memtype = UIO_MEM_PHYS;
+
+		info[count]->mem[1].addr = regs_l3ram->start;
+		info[count]->mem[1].size =
+		    regs_l3ram->end - regs_l3ram->start + 1;
+		if (!info[count]->mem[1].addr
+		    || !(info[count]->mem[1].size - 1)) {
+			dev_err(&dev->dev, "Invalid memory resource\n");
+			break;
+		}
+		info[count]->mem[1].internal_addr =
+		    ioremap(regs_l3ram->start, info[count]->mem[1].size);
+		if (!info[count]->mem[1].internal_addr) {
+			dev_err(&dev->dev,
+				"Can't remap memory address range\n");
+			break;
+		}
+		info[count]->mem[1].memtype = UIO_MEM_PHYS;
+
+		info[count]->mem[2].addr = ddr_phy_addr;
+		info[count]->mem[2].size = regs_ddr->end - regs_ddr->start + 1;
+		if (!info[count]->mem[2].addr
+		    || !(info[count]->mem[2].size - 1)) {
+			dev_err(&dev->dev, "Invalid memory resource\n");
+			break;
+		}
+		info[count]->mem[2].internal_addr = ddr_virt_addr;
+		if (!info[count]->mem[2].internal_addr) {
+			dev_err(&dev->dev,
+				"Can't remap memory address range\n");
+			break;
+		}
+		info[count]->mem[2].memtype = UIO_MEM_PHYS;
+
+		string = kzalloc(20, GFP_KERNEL);
+		sprintf(string, "pruss_evt%d", count);
+		info[count]->name = string;
+		info[count]->version = "0.50";
+
+		/* Register PRUSS IRQ lines */
+		info[count]->irq = IRQ_DA8XX_EVTOUT0 + count;
+
+		info[count]->irq_flags = IRQF_SHARED;
+		info[count]->handler = pruss_handler;
+
+		ret = uio_register_device(&dev->dev, info[count]);
+
+		if (ret < 0)
+			break;
+	}
+
+	if (ret < 0) {
+		if (ddr_virt_addr)
+			dma_free_coherent(&dev->dev,
+					  regs_ddr->end - regs_ddr->start + 1,
+					  ddr_virt_addr, ddr_phy_addr);
+		while (count--) {
+			uio_unregister_device(info[count]);
+			kfree(info[count]->name);
+			iounmap(info[count]->mem[0].internal_addr);
+		}
+	} else {
+		platform_set_drvdata(dev, info);
+		return 0;
+	}
+
+out_free:
+	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++)
+		kfree(info[count]);
+
+	if (pruss_clk != NULL)
+		clk_put(pruss_clk);
+
+	return ret;
+}
+
+static int __devexit pruss_remove(struct platform_device *dev)
+{
+	int count = 0;
+	struct uio_info **info;
+
+	info = (struct uio_info **)platform_get_drvdata(dev);
+
+	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
+		uio_unregister_device(info[count]);
+		kfree(info[count]->name);
+
+	}
+	iounmap(info[0]->mem[0].internal_addr);
+	iounmap(info[0]->mem[1].internal_addr);
+	if (ddr_virt_addr)
+		dma_free_coherent(&dev->dev, info[0]->mem[2].size,
+				  info[0]->mem[2].internal_addr,
+				  info[0]->mem[2].addr);
+
+	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++)
+		kfree(info[count]);
+
+	platform_set_drvdata(dev, NULL);
+
+	if (pruss_clk != NULL)
+		clk_put(pruss_clk);
+	return 0;
+}
+
+static struct platform_driver pruss_driver = {
+	.probe = pruss_probe,
+	.remove = __devexit_p(pruss_remove),
+	.driver = {
+		   .name = DRV_NAME,
+		   .owner = THIS_MODULE,
+		   },
+};
+
+static int __init pruss_init_module(void)
+{
+	return platform_driver_register(&pruss_driver);
+}
+
+module_init(pruss_init_module);
+
+static void __exit pruss_exit_module(void)
+{
+	platform_driver_unregister(&pruss_driver);
+}
+
+module_exit(pruss_exit_module);
+
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(DRV_VERSION);
+MODULE_AUTHOR("Amit Chatterjee <amit.chatterjee@ti.com>");
+MODULE_AUTHOR("Pratheesh Gangadhar <pratheesh@ti.com>");
-- 
1.6.0.6

[PATCH 2/2] Defines DA850/AM18xx/OMAPL1-38 SOC resources used by PRUSS UIO driver

[Pratheesh Gangadhar]

Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>

This patch defines PRUSS, ECAP clocks, memory and IRQ resources
used by PRUSS UIO driver in DA850/AM18xx/OMAPL1-38 devices. UIO
driver exports 64K I/O region of PRUSS, 128KB L3 RAM and 256KB
DDR buffer to user space. PRUSS has 8 host event interrupt lines
mapped to IRQ_DA8XX_EVTOUT0..7 of ARM9 INTC.These in conjunction
with shared memory can be used to implement IPC between ARM9 and
PRUSS.
---
 arch/arm/mach-davinci/board-da850-evm.c    |    4 ++
 arch/arm/mach-davinci/da850.c              |   35 +++++++++++++
 arch/arm/mach-davinci/devices-da8xx.c      |   73 ++++++++++++++++++++++++++++
 arch/arm/mach-davinci/include/mach/da8xx.h |    3 +
 4 files changed, 115 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 11f986b..ddcbac8 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -1077,6 +1077,10 @@ static __init void da850_evm_init(void)
 		pr_warning("da850_evm_init: i2c0 registration failed: %d\n",
 				ret);
 
+	ret = da8xx_register_pruss();
+       if (ret)
+                pr_warning("da850_evm_init: pruss registration failed: %d\n",
+                                 ret);
 
 	ret = da8xx_register_watchdog();
 	if (ret)
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 3443d97..0096d4f 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -238,6 +238,13 @@ static struct clk tptc2_clk = {
 	.flags		= ALWAYS_ENABLED,
 };
 
+static struct clk pruss_clk = {
+	.name		= "pruss",
+	.parent		= &pll0_sysclk2,
+	.lpsc		= DA8XX_LPSC0_DMAX,
+	.flags		= ALWAYS_ENABLED,
+};
+
 static struct clk uart0_clk = {
 	.name		= "uart0",
 	.parent		= &pll0_sysclk2,
@@ -359,6 +366,30 @@ static struct clk usb20_clk = {
 	.gpsc		= 1,
 };
 
+static struct clk ecap0_clk = {
+	.name		= "ecap0",
+	.parent		= &pll0_sysclk2,
+	.lpsc		= DA8XX_LPSC1_ECAP,
+	.flags		= DA850_CLK_ASYNC3,
+	.gpsc		= 1,
+};
+
+static struct clk ecap1_clk = {
+	.name		= "ecap1",
+	.parent		= &pll0_sysclk2,
+	.lpsc		= DA8XX_LPSC1_ECAP,
+	.flags		= DA850_CLK_ASYNC3,
+	.gpsc		= 1,
+};
+
+static struct clk ecap2_clk = {
+	.name		= "ecap2",
+	.parent		= &pll0_sysclk2,
+	.lpsc		= DA8XX_LPSC1_ECAP,
+	.flags		= DA850_CLK_ASYNC3,
+	.gpsc		= 1,
+};
+
 static struct clk_lookup da850_clks[] = {
 	CLK(NULL,		"ref",		&ref_clk),
 	CLK(NULL,		"pll0",		&pll0_clk),
@@ -386,6 +417,7 @@ static struct clk_lookup da850_clks[] = {
 	CLK(NULL,		"tptc1",	&tptc1_clk),
 	CLK(NULL,		"tpcc1",	&tpcc1_clk),
 	CLK(NULL,		"tptc2",	&tptc2_clk),
+	CLK(NULL,		"pruss",	&pruss_clk),
 	CLK(NULL,		"uart0",	&uart0_clk),
 	CLK(NULL,		"uart1",	&uart1_clk),
 	CLK(NULL,		"uart2",	&uart2_clk),
@@ -403,6 +435,9 @@ static struct clk_lookup da850_clks[] = {
 	CLK(NULL,		"aemif",	&aemif_clk),
 	CLK(NULL,		"usb11",	&usb11_clk),
 	CLK(NULL,		"usb20",	&usb20_clk),
+	CLK(NULL,		"ecap0",	&ecap0_clk),
+	CLK(NULL,		"ecap1",	&ecap1_clk),
+	CLK(NULL,		"ecap2",	&ecap2_clk),
 	CLK(NULL,		NULL,		NULL),
 };
 
diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
index beda8a4..4ea3d1f 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -725,3 +725,76 @@ int __init da8xx_register_cpuidle(void)
 
 	return platform_device_register(&da8xx_cpuidle_device);
 }
+static struct resource pruss_resources[] = {
+	[0] = {
+		.start  = DA8XX_PRUSS_BASE,
+		.end    = DA8XX_PRUSS_BASE + SZ_64K - 1,
+		.flags  = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start  = DA8XX_L3RAM_BASE,
+		.end    = DA8XX_L3RAM_BASE + SZ_128K - 1,
+		.flags  = IORESOURCE_MEM,
+	},
+	[2] = {
+		.start  = 0,
+		.end    = SZ_256K - 1,
+		.flags  = IORESOURCE_MEM,
+	},
+
+	[3] = {
+		.start  = IRQ_DA8XX_EVTOUT0,
+		.end    = IRQ_DA8XX_EVTOUT0,
+		.flags  = IORESOURCE_IRQ,
+	},
+	[4] = {
+		.start  = IRQ_DA8XX_EVTOUT1,
+		.end    = IRQ_DA8XX_EVTOUT1,
+		.flags  = IORESOURCE_IRQ,
+	},
+	[5] = {
+		.start  = IRQ_DA8XX_EVTOUT2,
+		.end    = IRQ_DA8XX_EVTOUT2,
+		.flags  = IORESOURCE_IRQ,
+	},
+	[6] = {
+		.start  = IRQ_DA8XX_EVTOUT3,
+		.end    = IRQ_DA8XX_EVTOUT3,
+		.flags  = IORESOURCE_IRQ,
+	},
+	[7] = {
+		.start  = IRQ_DA8XX_EVTOUT4,
+		.end    = IRQ_DA8XX_EVTOUT4,
+		.flags  = IORESOURCE_IRQ,
+	},
+	[8] = {
+		.start  = IRQ_DA8XX_EVTOUT5,
+		.end    = IRQ_DA8XX_EVTOUT5,
+		.flags  = IORESOURCE_IRQ,
+	},
+	[9] = {
+		.start  = IRQ_DA8XX_EVTOUT6,
+		.end    = IRQ_DA8XX_EVTOUT6,
+		.flags  = IORESOURCE_IRQ,
+	},
+	[10] = {
+		.start  = IRQ_DA8XX_EVTOUT7,
+		.end    = IRQ_DA8XX_EVTOUT7,
+		.flags  = IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device pruss_device = {
+	.name			= "pruss",
+	.id			= 0,
+	.num_resources		= ARRAY_SIZE(pruss_resources),
+	.resource		= pruss_resources,
+	.dev	 =	{
+		.coherent_dma_mask = 0xffffffff,
+	}
+};
+
+int __init da8xx_register_pruss()
+{
+	return platform_device_register(&pruss_device);
+}
diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
index cfcb223..3ed6ee0 100644
--- a/arch/arm/mach-davinci/include/mach/da8xx.h
+++ b/arch/arm/mach-davinci/include/mach/da8xx.h
@@ -60,6 +60,7 @@ extern unsigned int da850_max_speed;
 #define DA8XX_PLL0_BASE		0x01c11000
 #define DA8XX_TIMER64P0_BASE	0x01c20000
 #define DA8XX_TIMER64P1_BASE	0x01c21000
+#define DA8XX_PRUSS_BASE	0x01c30000
 #define DA8XX_GPIO_BASE		0x01e26000
 #define DA8XX_PSC1_BASE		0x01e27000
 #define DA8XX_LCD_CNTRL_BASE	0x01e13000
@@ -68,6 +69,7 @@ extern unsigned int da850_max_speed;
 #define DA8XX_AEMIF_CS2_BASE	0x60000000
 #define DA8XX_AEMIF_CS3_BASE	0x62000000
 #define DA8XX_AEMIF_CTL_BASE	0x68000000
+#define DA8XX_L3RAM_BASE	0x80000000
 #define DA8XX_DDR2_CTL_BASE	0xb0000000
 #define DA8XX_ARM_RAM_BASE	0xffff0000
 
@@ -90,6 +92,7 @@ int da850_register_cpufreq(char *async_clk);
 int da8xx_register_cpuidle(void);
 void __iomem * __init da8xx_get_mem_ctlr(void);
 int da850_register_pm(struct platform_device *pdev);
+int da8xx_register_pruss(void);
 
 extern struct platform_device da8xx_serial_device;
 extern struct emac_platform_data da8xx_emac_pdata;
-- 
1.6.0.6

[PATCH 2/2] Defines DA850/AM18xx/OMAPL1-38 SOC resources used by PRUSS UIO driver

[Sergei Shtylyov]

Hello.

Pratheesh Gangadhar wrote:

> Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>

    Signoff should follow the change log, not precede it. "From:" line may 
precede the change log.

> This patch defines PRUSS, ECAP clocks, memory and IRQ resources
> used by PRUSS UIO driver in DA850/AM18xx/OMAPL1-38 devices. UIO
> driver exports 64K I/O region of PRUSS, 128KB L3 RAM and 256KB
> DDR buffer to user space. PRUSS has 8 host event interrupt lines
> mapped to IRQ_DA8XX_EVTOUT0..7 of ARM9 INTC.These in conjunction
> with shared memory can be used to implement IPC between ARM9 and
> PRUSS.

> diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> index 11f986b..ddcbac8 100644
> --- a/arch/arm/mach-davinci/board-da850-evm.c
> +++ b/arch/arm/mach-davinci/board-da850-evm.c
> @@ -1077,6 +1077,10 @@ static __init void da850_evm_init(void)
>  		pr_warning("da850_evm_init: i2c0 registration failed: %d\n",
>  				ret);
>  
> +	ret = da8xx_register_pruss();
> +       if (ret)
> +                pr_warning("da850_evm_init: pruss registration failed: %d\n",
> +                                 ret);
>  
>  	ret = da8xx_register_watchdog();
>  	if (ret)

    I'd put this into a pacth of its own, to have the SoC level changes 
separated from board-level changes...

> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index 3443d97..0096d4f 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -238,6 +238,13 @@ static struct clk tptc2_clk = {
>  	.flags		= ALWAYS_ENABLED,
>  };
>  
> +static struct clk pruss_clk = {
> +	.name		= "pruss",
> +	.parent		= &pll0_sysclk2,
> +	.lpsc		= DA8XX_LPSC0_DMAX,

    We have patch submitted by TI that renames this #define to DA8XX_LPSC0_PRUSS...

> +	.flags		= ALWAYS_ENABLED,
> +};
> +

    We already have patch submitted by TI that defines this clock. It would be 
nice if you coordinated your efforts to avoid such conflicts.
    Again, I'd put the part adding the clocks into a patch of its own.

> diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
> index beda8a4..4ea3d1f 100644
> --- a/arch/arm/mach-davinci/devices-da8xx.c
> +++ b/arch/arm/mach-davinci/devices-da8xx.c
> @@ -725,3 +725,76 @@ int __init da8xx_register_cpuidle(void)
>  
>  	return platform_device_register(&da8xx_cpuidle_device);
>  }
> +static struct resource pruss_resources[] = {
> +	[0] = {
> +		.start  = DA8XX_PRUSS_BASE,
> +		.end    = DA8XX_PRUSS_BASE + SZ_64K - 1,
> +		.flags  = IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start  = DA8XX_L3RAM_BASE,
> +		.end    = DA8XX_L3RAM_BASE + SZ_128K - 1,
> +		.flags  = IORESOURCE_MEM,

    Are you going to use all SRAM?

> +	},
> +	[2] = {
> +		.start  = 0,
> +		.end    = SZ_256K - 1,
> +		.flags  = IORESOURCE_MEM,
> +	},
> +
> +	[3] = {
> +		.start  = IRQ_DA8XX_EVTOUT0,
> +		.end    = IRQ_DA8XX_EVTOUT0,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[4] = {
> +		.start  = IRQ_DA8XX_EVTOUT1,
> +		.end    = IRQ_DA8XX_EVTOUT1,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[5] = {
> +		.start  = IRQ_DA8XX_EVTOUT2,
> +		.end    = IRQ_DA8XX_EVTOUT2,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[6] = {
> +		.start  = IRQ_DA8XX_EVTOUT3,
> +		.end    = IRQ_DA8XX_EVTOUT3,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[7] = {
> +		.start  = IRQ_DA8XX_EVTOUT4,
> +		.end    = IRQ_DA8XX_EVTOUT4,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[8] = {
> +		.start  = IRQ_DA8XX_EVTOUT5,
> +		.end    = IRQ_DA8XX_EVTOUT5,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[9] = {
> +		.start  = IRQ_DA8XX_EVTOUT6,
> +		.end    = IRQ_DA8XX_EVTOUT6,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +	[10] = {
> +		.start  = IRQ_DA8XX_EVTOUT7,
> +		.end    = IRQ_DA8XX_EVTOUT7,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct platform_device pruss_device = {
> +	.name			= "pruss",
> +	.id			= 0,
> +	.num_resources		= ARRAY_SIZE(pruss_resources),
> +	.resource		= pruss_resources,
> +	.dev	 =	{
> +		.coherent_dma_mask = 0xffffffff,
> +	}
> +};
> +
> +int __init da8xx_register_pruss()
> +{
> +	return platform_device_register(&pruss_device);
> +}

    Again, there's TI patch that adds this device already...

WBR, Sergei

[PATCH 2/2] Defines DA850/AM18xx/OMAPL1-38 SOC resources used by PRUSS UIO driver

[TK, Pratheesh Gangadhar]

Hi,

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov at mvista.com]
> Sent: Monday, February 21, 2011 10:40 PM
> To: TK, Pratheesh Gangadhar
> Cc: davinci-linux-open-source at linux.davincidsp.com; gregkh at suse.de;
> Chatterjee, Amit; hjk at linutronix.de; linux-kernel at vger.kernel.org; linux-
> arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 2/2] Defines DA850/AM18xx/OMAPL1-38 SOC resources used
> by PRUSS UIO driver
> 
> Hello.
> 
> Pratheesh Gangadhar wrote:
> 
> > Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>
> 
>     Signoff should follow the change log, not precede it. "From:" line may
> precede the change log.
Fixed this in the new version.
> 
> > This patch defines PRUSS, ECAP clocks, memory and IRQ resources
> > used by PRUSS UIO driver in DA850/AM18xx/OMAPL1-38 devices. UIO
> > driver exports 64K I/O region of PRUSS, 128KB L3 RAM and 256KB
> > DDR buffer to user space. PRUSS has 8 host event interrupt lines
> > mapped to IRQ_DA8XX_EVTOUT0..7 of ARM9 INTC.These in conjunction
> > with shared memory can be used to implement IPC between ARM9 and
> > PRUSS.
> 
> > diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-
> davinci/board-da850-evm.c
> > index 11f986b..ddcbac8 100644
> > --- a/arch/arm/mach-davinci/board-da850-evm.c
> > +++ b/arch/arm/mach-davinci/board-da850-evm.c
> > @@ -1077,6 +1077,10 @@ static __init void da850_evm_init(void)
> >  		pr_warning("da850_evm_init: i2c0 registration failed: %d\n",
> >  				ret);
> >
> > +	ret = da8xx_register_pruss();
> > +       if (ret)
> > +                pr_warning("da850_evm_init: pruss registration failed:
> %d\n",
> > +                                 ret);
> >
> >  	ret = da8xx_register_watchdog();
> >  	if (ret)
> 
>     I'd put this into a pacth of its own, to have the SoC level changes
> separated from board-level changes...

Ok. I will do this; I need to make this change for hawkboard as well.
> 
> > diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-
> davinci/da850.c
> > index 3443d97..0096d4f 100644
> > --- a/arch/arm/mach-davinci/da850.c
> > +++ b/arch/arm/mach-davinci/da850.c
> > @@ -238,6 +238,13 @@ static struct clk tptc2_clk = {
> >  	.flags		= ALWAYS_ENABLED,
> >  };
> >
> > +static struct clk pruss_clk = {
> > +	.name		= "pruss",
> > +	.parent		= &pll0_sysclk2,
> > +	.lpsc		= DA8XX_LPSC0_DMAX,
> 
>     We have patch submitted by TI that renames this #define to
> DA8XX_LPSC0_PRUSS...
> 
> > +	.flags		= ALWAYS_ENABLED,
> > +};
> > +
> 
>     We already have patch submitted by TI that defines this clock. It
> would be
> nice if you coordinated your efforts to avoid such conflicts.
>     Again, I'd put the part adding the clocks into a patch of its own.

I will try to sync with that patch. 
> 
> > diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-
> davinci/devices-da8xx.c
> > index beda8a4..4ea3d1f 100644
> > --- a/arch/arm/mach-davinci/devices-da8xx.c
> > +++ b/arch/arm/mach-davinci/devices-da8xx.c
> > @@ -725,3 +725,76 @@ int __init da8xx_register_cpuidle(void)
> >
> >  	return platform_device_register(&da8xx_cpuidle_device);
> >  }
> > +static struct resource pruss_resources[] = {
> > +	[0] = {
> > +		.start  = DA8XX_PRUSS_BASE,
> > +		.end    = DA8XX_PRUSS_BASE + SZ_64K - 1,
> > +		.flags  = IORESOURCE_MEM,
> > +	},
> > +	[1] = {
> > +		.start  = DA8XX_L3RAM_BASE,
> > +		.end    = DA8XX_L3RAM_BASE + SZ_128K - 1,
> > +		.flags  = IORESOURCE_MEM,
> 
>     Are you going to use all SRAM?
Its application dependent - we don't use more than 10KB at this point.
> 
> > +	},
> > +	[2] = {
> > +		.start  = 0,
> > +		.end    = SZ_256K - 1,
> > +		.flags  = IORESOURCE_MEM,
> > +	},
> > +
> > +	[3] = {
> > +		.start  = IRQ_DA8XX_EVTOUT0,
> > +		.end    = IRQ_DA8XX_EVTOUT0,
> > +		.flags  = IORESOURCE_IRQ,
> > +	},
> > +	[4] = {
> > +		.start  = IRQ_DA8XX_EVTOUT1,
> > +		.end    = IRQ_DA8XX_EVTOUT1,
> > +		.flags  = IORESOURCE_IRQ,
> > +	},
> > +	[5] = {
> > +		.start  = IRQ_DA8XX_EVTOUT2,
> > +		.end    = IRQ_DA8XX_EVTOUT2,
> > +		.flags  = IORESOURCE_IRQ,
> > +	},
> > +	[6] = {
> > +		.start  = IRQ_DA8XX_EVTOUT3,
> > +		.end    = IRQ_DA8XX_EVTOUT3,
> > +		.flags  = IORESOURCE_IRQ,
> > +	},
> > +	[7] = {
> > +		.start  = IRQ_DA8XX_EVTOUT4,
> > +		.end    = IRQ_DA8XX_EVTOUT4,
> > +		.flags  = IORESOURCE_IRQ,
> > +	},
> > +	[8] = {
> > +		.start  = IRQ_DA8XX_EVTOUT5,
> > +		.end    = IRQ_DA8XX_EVTOUT5,
> > +		.flags  = IORESOURCE_IRQ,
> > +	},
> > +	[9] = {
> > +		.start  = IRQ_DA8XX_EVTOUT6,
> > +		.end    = IRQ_DA8XX_EVTOUT6,
> > +		.flags  = IORESOURCE_IRQ,
> > +	},
> > +	[10] = {
> > +		.start  = IRQ_DA8XX_EVTOUT7,
> > +		.end    = IRQ_DA8XX_EVTOUT7,
> > +		.flags  = IORESOURCE_IRQ,
> > +	},
> > +};
> > +
> > +static struct platform_device pruss_device = {
> > +	.name			= "pruss",
> > +	.id			= 0,
> > +	.num_resources		= ARRAY_SIZE(pruss_resources),
> > +	.resource		= pruss_resources,
> > +	.dev	 =	{
> > +		.coherent_dma_mask = 0xffffffff,
> > +	}
> > +};
> > +
> > +int __init da8xx_register_pruss()
> > +{
> > +	return platform_device_register(&pruss_device);
> > +}
> 
>     Again, there's TI patch that adds this device already...
I will check this.


Thanks,
Pratheesh

[PATCH 1/2] PRUSS UIO driver support

[Arnd Bergmann]

On Friday 18 February 2011, Pratheesh Gangadhar wrote:
> Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>
> 
> This patch implements PRUSS (Programmable Real-time Unit Sub System)
> UIO driver which exports SOC resources associated with PRUSS like
> I/O, memories and IRQs to user space. PRUSS is dual 32-bit RISC
> processors which is efficient in performing embedded tasks that
> require manipulation of packed memory mapped data structures and
> efficient in handling system events that have tight real time
> constraints. This driver is currently supported on Texas Instruments
> DA850, AM18xx and OMAPL1-38 devices.
> For example, PRUSS runs firmware for real-time critical industrial
> communication data link layer and communicates with application stack
> running in user space via shared memory and IRQs.

Looks basically ok, but there are two limitations that I see that you
might consider fixing.

Oh, and you should put the Signed-off-by statement below the changelog,
not above it, but that has nothing to do with the code.

> +/*
> + * Host event IRQ numbers from PRUSS 	
> + * 3 PRU_EVTOUT0 PRUSS Interrupt
> + * 4 PRU_EVTOUT1 PRUSS Interrupt
> + * 5 PRU_EVTOUT2 PRUSS Interrupt
> + * 6 PRU_EVTOUT3 PRUSS Interrupt
> + * 7 PRU_EVTOUT4 PRUSS Interrupt
> + * 8 PRU_EVTOUT5 PRUSS Interrupt
> + * 9 PRU_EVTOUT6 PRUSS Interrupt
> + * 10 PRU_EVTOUT7 PRUSS Interrupt
> +*/
> +
> +#define MAX_PRUSS_EVTOUT_INSTANCE	(8)
> +
> +static struct clk *pruss_clk;
> +static struct uio_info *info[MAX_PRUSS_EVTOUT_INSTANCE];
> +static void *ddr_virt_addr;
> +static dma_addr_t ddr_phy_addr;

By making all of these static variables, you limit youself to
a single PRUSS instance in the system. It's generally better
to write device drivers in a way that makes it possible to
have multiple instances, e.g. by moving these four variables
into the 'priv' part of struct uio_info.

> +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> +{
> +	return IRQ_HANDLED;
> +}

An empty interrupt handler is rather pointless. I guess you really
notify user space when the interrupt handler gets called, as this
is the main point of a UIO driver as far as I understand it.

	Arnd

[PATCH 1/2] PRUSS UIO driver support

[Thomas Gleixner]

On Fri, 18 Feb 2011, Arnd Bergmann wrote:
> On Friday 18 February 2011, Pratheesh Gangadhar wrote:
> > Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>
> > +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> > +{
> > +	return IRQ_HANDLED;
> > +}
> 
> An empty interrupt handler is rather pointless. I guess you really
> notify user space when the interrupt handler gets called, as this
> is the main point of a UIO driver as far as I understand it.

The UIO core code does this for you when the driver handler returns
IRQ_HANDLED, but the empty handler raises a different questions:

Is the interrupt edge triggerd or how do you avoid an irq storm here?
Usually UIO drivers are requested to mask the interrupt in the device
itself.

Thanks,

	tglx

[PATCH 1/2] PRUSS UIO driver support

[Arnd Bergmann]

On Friday 18 February 2011, Thomas Gleixner wrote:
> On Fri, 18 Feb 2011, Arnd Bergmann wrote:
> > On Friday 18 February 2011, Pratheesh Gangadhar wrote:
> > > Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>
> > > +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> > > +{
> > > +   return IRQ_HANDLED;
> > > +}
> > 
> > An empty interrupt handler is rather pointless. I guess you really
> > notify user space when the interrupt handler gets called, as this
> > is the main point of a UIO driver as far as I understand it.
> 
> The UIO core code does this for you when the driver handler returns
> IRQ_HANDLED

Ah, right.

> but the empty handler raises a different questions:
> 
> Is the interrupt edge triggerd or how do you avoid an irq storm here?
> Usually UIO drivers are requested to mask the interrupt in the device
> itself.

If it's edge triggered, it should not advertise IRQF_SHARED, right?

	Arnd

[PATCH 1/2] PRUSS UIO driver support

[Thomas Gleixner]

On Fri, 18 Feb 2011, Arnd Bergmann wrote:

> On Friday 18 February 2011, Thomas Gleixner wrote:
> > On Fri, 18 Feb 2011, Arnd Bergmann wrote:
> > > On Friday 18 February 2011, Pratheesh Gangadhar wrote:
> > > > Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>
> > > > +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> > > > +{
> > > > +   return IRQ_HANDLED;
> > > > +}
> > > 
> > > An empty interrupt handler is rather pointless. I guess you really
> > > notify user space when the interrupt handler gets called, as this
> > > is the main point of a UIO driver as far as I understand it.
> > 
> > The UIO core code does this for you when the driver handler returns
> > IRQ_HANDLED
> 
> Ah, right.
> 
> > but the empty handler raises a different questions:
> > 
> > Is the interrupt edge triggerd or how do you avoid an irq storm here?
> > Usually UIO drivers are requested to mask the interrupt in the device
> > itself.
> 
> If it's edge triggered, it should not advertise IRQF_SHARED, right?

Nope. And the handler needs a fat comment why this works.

Thanks,

	tglx

[PATCH 1/2] PRUSS UIO driver support

[TK, Pratheesh Gangadhar]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx at linutronix.de]
> Sent: Friday, February 18, 2011 10:33 PM
> To: Arnd Bergmann
> Cc: linux-arm-kernel at lists.infradead.org; TK, Pratheesh Gangadhar;
> davinci-linux-open-source at linux.davincidsp.com; gregkh at suse.de;
> Chatterjee, Amit; Hans J. Koch; LKML
> Subject: Re: [PATCH 1/2] PRUSS UIO driver support
> 
> On Fri, 18 Feb 2011, Arnd Bergmann wrote:
> 
> > On Friday 18 February 2011, Thomas Gleixner wrote:
> > > On Fri, 18 Feb 2011, Arnd Bergmann wrote:
> > > > On Friday 18 February 2011, Pratheesh Gangadhar wrote:
> > > > > Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>
> > > > > +static irqreturn_t pruss_handler(int irq, struct uio_info
> *dev_info)
> > > > > +{
> > > > > +   return IRQ_HANDLED;
> > > > > +}
> > > >
> > > > An empty interrupt handler is rather pointless. I guess you really
> > > > notify user space when the interrupt handler gets called, as this
> > > > is the main point of a UIO driver as far as I understand it.
> > >
> > > The UIO core code does this for you when the driver handler returns
> > > IRQ_HANDLED
> >
> > Ah, right.
> >
> > > but the empty handler raises a different questions:
> > >
> > > Is the interrupt edge triggerd or how do you avoid an irq storm here?
> > > Usually UIO drivers are requested to mask the interrupt in the device
> > > itself.
> >
> > If it's edge triggered, it should not advertise IRQF_SHARED, right?
> 
> Nope. And the handler needs a fat comment why this works.

For my understanding - if the interrupt is not shared and not level triggered - is this okay to have empty handler? In this specific case, these interrupt lines are internal to SOC and hooked to ARM INTC from PRUSS. PRUSS has another INTC to handle system events to PRUSS as well as to generate system events to host ARM. These generated events are used for IPC between user application and PRU firmware and for async notifications from PRU firmware to user space. I don't see a reason to make it shared as we have 8 lines available for use. As mentioned before ARM INTC interrupt processing logic converts interrupts to active high pulses.

I also looked at the interrupt handling in existing UIO drivers


static irqreturn_t my_uio_handler(int irq, struct uio_info *dev_info)
{
        if (no interrupt is enabled and no interrupt is active) /For shared interrupt handling
                return IRQ_NONE;

        disable interrupt; // For level triggered interrupts 
        return IRQ_HANDLED;
}

It's not clear how and where interrupts are re-enabled. Is this expected to be done from user space?

Uio_secos3.c has an irqcontrol function to enable/disable interrupts. Is this the recommended approach?

Thanks and Regards,
Pratheesh

[PATCH 1/2] PRUSS UIO driver support

[Hans J. Koch]

On Sat, Feb 19, 2011 at 09:10:23PM +0530, TK, Pratheesh Gangadhar wrote:
> 
> For my understanding - if the interrupt is not shared and not level triggered -
> is this okay to have empty handler?

Greg already said he won't accept that. And I'm quite sure these interrupts
are level triggered, since that is the default on arch/omap.

E.g. in arch/arm/mach-omap1/irq.c, a loop sets all irqs to level triggered
handling: set_irq_handler(j, handle_level_irq); (line 234)

> In this specific case, these interrupt lines are internal to SOC and hooked to ARM INTC from PRUSS. PRUSS has another INTC to handle system events to PRUSS as well as to generate system events to host ARM. These generated events are used for IPC between user application and PRU firmware and for async notifications from PRU firmware to user space. I don't see a reason to make it shared as we have 8 lines available for use. As mentioned before ARM INTC interrupt processing logic converts interrupts to active high pulses.

What's a "pulse triggered interrupt"? I know level and edge triggered ones.

> 
> I also looked at the interrupt handling in existing UIO drivers
> 
> 
> static irqreturn_t my_uio_handler(int irq, struct uio_info *dev_info)
> {
>         if (no interrupt is enabled and no interrupt is active) /For shared interrupt handling
>                 return IRQ_NONE;
> 
>         disable interrupt; // For level triggered interrupts 
>         return IRQ_HANDLED;
> }
> 
> It's not clear how and where interrupts are re-enabled. Is this expected to be done from user space?

That's the normal case, yes. You re-enable the interrupts by accessing the irq
mask register of your chip.

> 
> Uio_secos3.c has an irqcontrol function to enable/disable interrupts. Is this the recommended approach?

No. That is a workaround for broken hardware. You need it if and only if your
chip 1) has several internal irq sources and 2) there's no possibility for
the kernel to mask the interrupt in the chip without loosing the information
which irq source actually triggered the interrupt.

If that is the case, the kernel will disable the irq line in some other way
without touching the chip. Userspace then needs a way to re-enable irqs,
and that's where the irqcontrol function is used.

Normally, you shouldn't need it.

Thanks,
Hans

[PATCH 1/2] PRUSS UIO driver support

[TK, Pratheesh Gangadhar]

Hi Hans,

> -----Original Message-----
> From: Hans J. Koch [mailto:hjk at hansjkoch.de]
> Sent: Sunday, February 20, 2011 12:04 AM
> To: TK, Pratheesh Gangadhar
> Cc: Thomas Gleixner; Arnd Bergmann; linux-arm-kernel at lists.infradead.org;
> davinci-linux-open-source at linux.davincidsp.com; gregkh at suse.de;
> Chatterjee, Amit; Hans J. Koch; LKML
> Subject: Re: [PATCH 1/2] PRUSS UIO driver support
> 
> On Sat, Feb 19, 2011 at 09:10:23PM +0530, TK, Pratheesh Gangadhar wrote:
> >
> > For my understanding - if the interrupt is not shared and not level
> triggered -
> > is this okay to have empty handler?
> 
> Greg already said he won't accept that. And I'm quite sure these
> interrupts
> are level triggered, since that is the default on arch/omap.
> 
> E.g. in arch/arm/mach-omap1/irq.c, a loop sets all irqs to level triggered
> handling: set_irq_handler(j, handle_level_irq); (line 234)

You should be looking at arch/arm/mach-davinci/irq.c and all interrupts except IRQ_TINT1_TINT34 is set to edge trigger mode (line 160).
> 
> > In this specific case, these interrupt lines are internal to SOC and
> hooked to ARM INTC from PRUSS. PRUSS has another INTC to handle system
> events to PRUSS as well as to generate system events to host ARM. These
> generated events are used for IPC between user application and PRU
> firmware and for async notifications from PRU firmware to user space. I
> don't see a reason to make it shared as we have 8 lines available for use.
> As mentioned before ARM INTC interrupt processing logic converts
> interrupts to active high pulses.
> 
> What's a "pulse triggered interrupt"? I know level and edge triggered
> ones.

Basically it's same as edge triggered.

Thanks,
Pratheesh

[PATCH 1/2] PRUSS UIO driver support

[Hans J. Koch]

On Mon, Feb 21, 2011 at 09:27:47AM +0530, TK, Pratheesh Gangadhar wrote:
> Hi Hans,
> 
> > -----Original Message-----
> > From: Hans J. Koch [mailto:hjk at hansjkoch.de]
> > Sent: Sunday, February 20, 2011 12:04 AM
> > To: TK, Pratheesh Gangadhar
> > Cc: Thomas Gleixner; Arnd Bergmann; linux-arm-kernel at lists.infradead.org;
> > davinci-linux-open-source at linux.davincidsp.com; gregkh at suse.de;
> > Chatterjee, Amit; Hans J. Koch; LKML
> > Subject: Re: [PATCH 1/2] PRUSS UIO driver support
> > 
> > On Sat, Feb 19, 2011 at 09:10:23PM +0530, TK, Pratheesh Gangadhar wrote:
> > >
> > > For my understanding - if the interrupt is not shared and not level
> > triggered -
> > > is this okay to have empty handler?
> > 
> > Greg already said he won't accept that. And I'm quite sure these
> > interrupts
> > are level triggered, since that is the default on arch/omap.
> > 
> > E.g. in arch/arm/mach-omap1/irq.c, a loop sets all irqs to level triggered
> > handling: set_irq_handler(j, handle_level_irq); (line 234)
> 
> You should be looking at arch/arm/mach-davinci/irq.c

Hmpf. I alway get lost in the directory structure of TI SoCs...
At least I learned that OMAP3 is in the omap2 directory :)

> and all interrupts except IRQ_TINT1_TINT34 is set to edge trigger mode (line 160).

Is that really needed? Level triggered interrupts should certainly be
preferred if possible.

Thanks,
Hans

[PATCH 1/2] PRUSS UIO driver support

[Thomas Gleixner]

On Mon, 21 Feb 2011, Hans J. Koch wrote:
> On Mon, Feb 21, 2011 at 09:27:47AM +0530, TK, Pratheesh Gangadhar wrote:
> > and all interrupts except IRQ_TINT1_TINT34 is set to edge trigger mode (line 160).
> 
> Is that really needed? Level triggered interrupts should certainly be
> preferred if possible.

Nonsense. If you have a sane interrupt controller which latches the
edge transitions even when masked, edge type interrupts are perfectly
fine.

Thanks,

	tglx

[PATCH 1/2] PRUSS UIO driver support

[Sergei Shtylyov]

Hello.

Hans J. Koch wrote:

>>> -----Original Message-----
>>> From: Hans J. Koch [mailto:hjk at hansjkoch.de]
>>> Sent: Sunday, February 20, 2011 12:04 AM
>>> To: TK, Pratheesh Gangadhar
>>> Cc: Thomas Gleixner; Arnd Bergmann; linux-arm-kernel at lists.infradead.org;
>>> davinci-linux-open-source at linux.davincidsp.com; gregkh at suse.de;
>>> Chatterjee, Amit; Hans J. Koch; LKML
>>> Subject: Re: [PATCH 1/2] PRUSS UIO driver support

>>> On Sat, Feb 19, 2011 at 09:10:23PM +0530, TK, Pratheesh Gangadhar wrote:
>>>> For my understanding - if the interrupt is not shared and not level triggered -
>>>> is this okay to have empty handler?

>>> Greg already said he won't accept that. And I'm quite sure these
>>> interrupts
>>> are level triggered, since that is the default on arch/omap.

>>> E.g. in arch/arm/mach-omap1/irq.c, a loop sets all irqs to level triggered
>>> handling: set_irq_handler(j, handle_level_irq); (line 234)

>> You should be looking at arch/arm/mach-davinci/irq.c

    Into arch/arm/mach-davinci/cp_intc.c actually as we're not talking about a 
"real" DaVincis here, but about OMAP-L138.

> Hmpf. I alway get lost in the directory structure of TI SoCs...
> At least I learned that OMAP3 is in the omap2 directory :)

    It's probably even more convoluted with DaVinci and friends. :-)

>> and all interrupts except IRQ_TINT1_TINT34 is set to edge trigger mode (line 160).

> Is that really needed? Level triggered interrupts should certainly be
> preferred if possible.

    Don't look there, this code is for "real" DaVincis only.

> Thanks,
> Hans

WBR, Sergei

[PATCH 1/2] PRUSS UIO driver support

[TK, Pratheesh Gangadhar]

Hi,

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov at mvista.com]
> Sent: Tuesday, February 22, 2011 1:23 AM
> To: Hans J. Koch
> Cc: TK, Pratheesh Gangadhar; davinci-linux-open-
> source at linux.davincidsp.com; Arnd Bergmann; Chatterjee, Amit;
> gregkh at suse.de; LKML; Thomas Gleixner; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 1/2] PRUSS UIO driver support
> 
> Hello.
> 
> Hans J. Koch wrote:
> 
> >>> -----Original Message-----
> >>> From: Hans J. Koch [mailto:hjk at hansjkoch.de]
> >>> Sent: Sunday, February 20, 2011 12:04 AM
> >>> To: TK, Pratheesh Gangadhar
> >>> Cc: Thomas Gleixner; Arnd Bergmann; linux-arm-
> kernel at lists.infradead.org;
> >>> davinci-linux-open-source at linux.davincidsp.com; gregkh at suse.de;
> >>> Chatterjee, Amit; Hans J. Koch; LKML
> >>> Subject: Re: [PATCH 1/2] PRUSS UIO driver support
> 
> >>> On Sat, Feb 19, 2011 at 09:10:23PM +0530, TK, Pratheesh Gangadhar
> wrote:
> >>>> For my understanding - if the interrupt is not shared and not level
> triggered -
> >>>> is this okay to have empty handler?
> 
> >>> Greg already said he won't accept that. And I'm quite sure these
> >>> interrupts
> >>> are level triggered, since that is the default on arch/omap.
> 
> >>> E.g. in arch/arm/mach-omap1/irq.c, a loop sets all irqs to level
> triggered
> >>> handling: set_irq_handler(j, handle_level_irq); (line 234)
> 
> >> You should be looking at arch/arm/mach-davinci/irq.c
> 
>     Into arch/arm/mach-davinci/cp_intc.c actually as we're not talking
> about a
> "real" DaVincis here, but about OMAP-L138.
> 
> > Hmpf. I alway get lost in the directory structure of TI SoCs...
> > At least I learned that OMAP3 is in the omap2 directory :)
> 
>     It's probably even more convoluted with DaVinci and friends. :-)
> 
> >> and all interrupts except IRQ_TINT1_TINT34 is set to edge trigger mode
> (line 160).
> 
> > Is that really needed? Level triggered interrupts should certainly be
> > preferred if possible.
> 
>     Don't look there, this code is for "real" DaVincis only.
> 

Thanks for correcting. Interrupts are configured as edge triggered here as well.

Thanks and Regards,
Pratheesh

[PATCH 1/2] PRUSS UIO driver support

[TK, Pratheesh Gangadhar]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: Friday, February 18, 2011 9:14 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: TK, Pratheesh Gangadhar; davinci-linux-open-
> source at linux.davincidsp.com; gregkh at suse.de; Chatterjee, Amit;
> hjk at linutronix.de; linux-kernel at vger.kernel.org
> Subject: Re: [PATCH 1/2] PRUSS UIO driver support
> 
> On Friday 18 February 2011, Pratheesh Gangadhar wrote:
> > Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>
> >
> > This patch implements PRUSS (Programmable Real-time Unit Sub System)
> > UIO driver which exports SOC resources associated with PRUSS like
> > I/O, memories and IRQs to user space. PRUSS is dual 32-bit RISC
> > processors which is efficient in performing embedded tasks that
> > require manipulation of packed memory mapped data structures and
> > efficient in handling system events that have tight real time
> > constraints. This driver is currently supported on Texas Instruments
> > DA850, AM18xx and OMAPL1-38 devices.
> > For example, PRUSS runs firmware for real-time critical industrial
> > communication data link layer and communicates with application stack
> > running in user space via shared memory and IRQs.
> 
> Looks basically ok, but there are two limitations that I see that you
> might consider fixing.
> 
> Oh, and you should put the Signed-off-by statement below the changelog,
> not above it, but that has nothing to do with the code.
> 
> > +/*
> > + * Host event IRQ numbers from PRUSS
> > + * 3 PRU_EVTOUT0 PRUSS Interrupt
> > + * 4 PRU_EVTOUT1 PRUSS Interrupt
> > + * 5 PRU_EVTOUT2 PRUSS Interrupt
> > + * 6 PRU_EVTOUT3 PRUSS Interrupt
> > + * 7 PRU_EVTOUT4 PRUSS Interrupt
> > + * 8 PRU_EVTOUT5 PRUSS Interrupt
> > + * 9 PRU_EVTOUT6 PRUSS Interrupt
> > + * 10 PRU_EVTOUT7 PRUSS Interrupt
> > +*/
> > +
> > +#define MAX_PRUSS_EVTOUT_INSTANCE	(8)
> > +
> > +static struct clk *pruss_clk;
> > +static struct uio_info *info[MAX_PRUSS_EVTOUT_INSTANCE];
> > +static void *ddr_virt_addr;
> > +static dma_addr_t ddr_phy_addr;
> 
> By making all of these static variables, you limit youself to
> a single PRUSS instance in the system. It's generally better
> to write device drivers in a way that makes it possible to
> have multiple instances, e.g. by moving these four variables
> into the 'priv' part of struct uio_info.

Ok, I agree with making them non-static. Making them part of uio_info might
not be desired. PRUSS_EVOUT_INSTANCE is not same as PRUSS instances. Each
PRUSS can have up to 8 event out interrupt lines to host ARM.
> 
> > +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> > +{
> > +	return IRQ_HANDLED;
> > +}
> 
> An empty interrupt handler is rather pointless. I guess you really
> notify user space when the interrupt handler gets called, as this
> is the main point of a UIO driver as far as I understand it.
> 
As discussed in the later E-mails, UIO core takes care of this.
Will cover this in following responses.

Thanks,
Pratheesh

[PATCH 1/2] PRUSS UIO driver support

[Hans J. Koch]

On Fri, Feb 18, 2011 at 08:35:29PM +0530, Pratheesh Gangadhar wrote:
> Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>
> 
> This patch implements PRUSS (Programmable Real-time Unit Sub System)
> UIO driver which exports SOC resources associated with PRUSS like
> I/O, memories and IRQs to user space. PRUSS is dual 32-bit RISC
> processors which is efficient in performing embedded tasks that
> require manipulation of packed memory mapped data structures and
> efficient in handling system events that have tight real time
> constraints. This driver is currently supported on Texas Instruments
> DA850, AM18xx and OMAPL1-38 devices.
> For example, PRUSS runs firmware for real-time critical industrial
> communication data link layer and communicates with application stack
> running in user space via shared memory and IRQs.

I see a few issues, comments below.

Thanks,
Hans

> ---
>  drivers/uio/Kconfig     |   10 ++
>  drivers/uio/Makefile    |    1 +
>  drivers/uio/uio_pruss.c |  250 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 261 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/uio/uio_pruss.c
> 
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index bb44079..631ffe3 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -94,4 +94,14 @@ config UIO_NETX
>  	  To compile this driver as a module, choose M here; the module
>  	  will be called uio_netx.
>  
> +config UIO_PRUSS
> +	tristate "Texas Instruments PRUSS driver"
> +	depends on ARCH_DAVINCI_DA850
> +	default n

That line is unneccessary, "n" is already the default.

> +	help
> +	  PRUSS driver for OMAPL138/DA850/AM18XX devices
> +	  PRUSS driver requires user space components
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called uio_pruss.
> +
>  endif
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index 18fd818..d4dd9a5 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_UIO_AEC)	+= uio_aec.o
>  obj-$(CONFIG_UIO_SERCOS3)	+= uio_sercos3.o
>  obj-$(CONFIG_UIO_PCI_GENERIC)	+= uio_pci_generic.o
>  obj-$(CONFIG_UIO_NETX)	+= uio_netx.o
> +obj-$(CONFIG_UIO_PRUSS)         += uio_pruss.o
> diff --git a/drivers/uio/uio_pruss.c b/drivers/uio/uio_pruss.c
> new file mode 100644
> index 0000000..5ae78a5
> --- /dev/null
> +++ b/drivers/uio/uio_pruss.c
> @@ -0,0 +1,250 @@
> +/*
> + * Programmable Real-Time Unit Sub System (PRUSS) UIO driver (uio_pruss)
> + *
> + * This driver exports PRUSS host event out interrupts and PRUSS, L3 RAM,
> + * and DDR RAM to user space for applications interacting with PRUSS firmware 
> + *
> + * Copyright (C) 2010-11 Texas Instruments Incorporated - http://www.ti.com/
> + * 
> + * 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/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/uio_driver.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/slab.h>
> +
> +#define DRV_NAME "pruss"
> +#define DRV_VERSION "0.50"
> +
> +/*
> + * Host event IRQ numbers from PRUSS 	
> + * 3 PRU_EVTOUT0 PRUSS Interrupt
> + * 4 PRU_EVTOUT1 PRUSS Interrupt
> + * 5 PRU_EVTOUT2 PRUSS Interrupt
> + * 6 PRU_EVTOUT3 PRUSS Interrupt
> + * 7 PRU_EVTOUT4 PRUSS Interrupt
> + * 8 PRU_EVTOUT5 PRUSS Interrupt
> + * 9 PRU_EVTOUT6 PRUSS Interrupt
> + * 10 PRU_EVTOUT7 PRUSS Interrupt
> +*/
> +
> +#define MAX_PRUSS_EVTOUT_INSTANCE	(8)

The brackets are not needed.

> +
> +static struct clk *pruss_clk;
> +static struct uio_info *info[MAX_PRUSS_EVTOUT_INSTANCE];

is it really neccessary to allocate that statically?

> +static void *ddr_virt_addr;
> +static dma_addr_t ddr_phy_addr;
> +
> +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> +{
> +	return IRQ_HANDLED;
> +}

ROTFL. That reminds me of an old story. The last time I wrote this, and
Greg dared to post it, we received this reply:

http://marc.info/?l=linux-kernel&m=116604101232144&w=2

So, if you really have a _very_ good reason why this _always_ works on
_any_ DA850 board, add a comment that explains why. Otherwise the whole
patch set will be doomed.

> +
> +static int __devinit pruss_probe(struct platform_device *dev)
> +{
> +	int ret = -ENODEV;
> +	int count = 0;
> +	struct resource *regs_pruram, *regs_l3ram, *regs_ddr;
> +	char *string;
> +
> +	/* Power on PRU in case its not done as part of boot-loader */
> +	pruss_clk = clk_get(&dev->dev, "pruss");
> +	if (IS_ERR(pruss_clk)) {
> +		dev_err(&dev->dev, "Failed to get clock\n");
> +		ret = PTR_ERR(pruss_clk);
> +		pruss_clk = NULL;
> +		return ret;
> +	} else {
> +		clk_enable(pruss_clk);
> +	}
> +
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
> +		info[count] = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
> +		if (!info[count])
> +			return -ENOMEM;
> +	}
> +
> +	regs_pruram = platform_get_resource(dev, IORESOURCE_MEM, 0);
> +	if (!regs_pruram) {
> +		dev_err(&dev->dev, "No memory resource specified\n");
> +		goto out_free;
> +	}
> +
> +	regs_l3ram = platform_get_resource(dev, IORESOURCE_MEM, 1);
> +	if (!regs_l3ram) {
> +		dev_err(&dev->dev, "No memory resource specified\n");
> +		goto out_free;
> +	}
> +
> +	regs_ddr = platform_get_resource(dev, IORESOURCE_MEM, 2);
> +	if (!regs_ddr) {
> +		dev_err(&dev->dev, "No memory resource specified\n");
> +		goto out_free;
> +	}
> +	ddr_virt_addr =
> +	    dma_alloc_coherent(&dev->dev, regs_ddr->end - regs_ddr->start + 1,
> +			       &ddr_phy_addr, GFP_KERNEL | GFP_DMA);
> +	if (!ddr_virt_addr) {
> +		dev_err(&dev->dev, "Could not allocate external memory\n");
> +		goto out_free;
> +	}
> +
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
> +		info[count]->mem[0].addr = regs_pruram->start;
> +		info[count]->mem[0].size =
> +		    regs_pruram->end - regs_pruram->start + 1;
> +		if (!info[count]->mem[0].addr
> +		    || !(info[count]->mem[0].size - 1)) {

That size check looks fishy. If somebody forgot to set the size it's OK ?

> +			dev_err(&dev->dev, "Invalid memory resource\n");
> +			break;
> +		}
> +		info[count]->mem[0].internal_addr =
> +		    ioremap(regs_pruram->start, info[count]->mem[0].size);
> +		if (!info[count]->mem[0].internal_addr) {
> +			dev_err(&dev->dev,
> +				"Can't remap memory address range\n");
> +			break;
> +		}
> +		info[count]->mem[0].memtype = UIO_MEM_PHYS;
> +
> +		info[count]->mem[1].addr = regs_l3ram->start;
> +		info[count]->mem[1].size =
> +		    regs_l3ram->end - regs_l3ram->start + 1;
> +		if (!info[count]->mem[1].addr
> +		    || !(info[count]->mem[1].size - 1)) {
> +			dev_err(&dev->dev, "Invalid memory resource\n");
> +			break;
> +		}
> +		info[count]->mem[1].internal_addr =
> +		    ioremap(regs_l3ram->start, info[count]->mem[1].size);
> +		if (!info[count]->mem[1].internal_addr) {
> +			dev_err(&dev->dev,
> +				"Can't remap memory address range\n");
> +			break;
> +		}
> +		info[count]->mem[1].memtype = UIO_MEM_PHYS;
> +
> +		info[count]->mem[2].addr = ddr_phy_addr;
> +		info[count]->mem[2].size = regs_ddr->end - regs_ddr->start + 1;
> +		if (!info[count]->mem[2].addr
> +		    || !(info[count]->mem[2].size - 1)) {
> +			dev_err(&dev->dev, "Invalid memory resource\n");
> +			break;
> +		}
> +		info[count]->mem[2].internal_addr = ddr_virt_addr;
> +		if (!info[count]->mem[2].internal_addr) {
> +			dev_err(&dev->dev,
> +				"Can't remap memory address range\n");
> +			break;
> +		}
> +		info[count]->mem[2].memtype = UIO_MEM_PHYS;
> +
> +		string = kzalloc(20, GFP_KERNEL);
> +		sprintf(string, "pruss_evt%d", count);
> +		info[count]->name = string;
> +		info[count]->version = "0.50";
> +
> +		/* Register PRUSS IRQ lines */
> +		info[count]->irq = IRQ_DA8XX_EVTOUT0 + count;
> +
> +		info[count]->irq_flags = IRQF_SHARED;

How do you handle shared interrupts with the handler above?

> +		info[count]->handler = pruss_handler;

And how do you make sure your interrupts are not level triggered? The
handler above will hang for level triggered interrupts.

> +
> +		ret = uio_register_device(&dev->dev, info[count]);
> +
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	if (ret < 0) {
> +		if (ddr_virt_addr)
> +			dma_free_coherent(&dev->dev,
> +					  regs_ddr->end - regs_ddr->start + 1,
> +					  ddr_virt_addr, ddr_phy_addr);
> +		while (count--) {
> +			uio_unregister_device(info[count]);
> +			kfree(info[count]->name);
> +			iounmap(info[count]->mem[0].internal_addr);
> +		}
> +	} else {
> +		platform_set_drvdata(dev, info);
> +		return 0;
> +	}
> +
> +out_free:
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++)
> +		kfree(info[count]);
> +
> +	if (pruss_clk != NULL)
> +		clk_put(pruss_clk);
> +
> +	return ret;
> +}
> +
> +static int __devexit pruss_remove(struct platform_device *dev)
> +{
> +	int count = 0;
> +	struct uio_info **info;
> +
> +	info = (struct uio_info **)platform_get_drvdata(dev);
> +
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
> +		uio_unregister_device(info[count]);
> +		kfree(info[count]->name);
> +
> +	}
> +	iounmap(info[0]->mem[0].internal_addr);
> +	iounmap(info[0]->mem[1].internal_addr);
> +	if (ddr_virt_addr)
> +		dma_free_coherent(&dev->dev, info[0]->mem[2].size,
> +				  info[0]->mem[2].internal_addr,
> +				  info[0]->mem[2].addr);
> +
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++)
> +		kfree(info[count]);
> +
> +	platform_set_drvdata(dev, NULL);
> +
> +	if (pruss_clk != NULL)
> +		clk_put(pruss_clk);
> +	return 0;
> +}
> +
> +static struct platform_driver pruss_driver = {
> +	.probe = pruss_probe,
> +	.remove = __devexit_p(pruss_remove),
> +	.driver = {
> +		   .name = DRV_NAME,
> +		   .owner = THIS_MODULE,
> +		   },
> +};
> +
> +static int __init pruss_init_module(void)
> +{
> +	return platform_driver_register(&pruss_driver);
> +}
> +
> +module_init(pruss_init_module);
> +
> +static void __exit pruss_exit_module(void)
> +{
> +	platform_driver_unregister(&pruss_driver);
> +}
> +
> +module_exit(pruss_exit_module);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_AUTHOR("Amit Chatterjee <amit.chatterjee@ti.com>");
> +MODULE_AUTHOR("Pratheesh Gangadhar <pratheesh@ti.com>");
> -- 
> 1.6.0.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[PATCH 1/2] PRUSS UIO driver support

[Greg KH]

On Fri, Feb 18, 2011 at 05:31:47PM +0100, Hans J. Koch wrote:
> On Fri, Feb 18, 2011 at 08:35:29PM +0530, Pratheesh Gangadhar wrote:
> > Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>

As noted by others, this needs to go at the end of the changelog
comment.

Also, always run your patches through the scripts/checkpatch.pl script
and fix the warnings and errors it finds.  To not do so is just rude as
you are asking us to do the basic review work that you yourself did not
do in the first place.

> > +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> > +{
> > +	return IRQ_HANDLED;
> > +}
> 
> ROTFL. That reminds me of an old story. The last time I wrote this, and
> Greg dared to post it, we received this reply:
> 
> http://marc.info/?l=linux-kernel&m=116604101232144&w=2
> 
> So, if you really have a _very_ good reason why this _always_ works on
> _any_ DA850 board, add a comment that explains why. Otherwise the whole
> patch set will be doomed.

Nope, this whole patch set is doomed if this isn't fixed, I'm not going
to accept it no matter how much you want to try to say this is ok on
this hardware.

thanks,

greg k-h

[PATCH 1/2] PRUSS UIO driver support

[TK, Pratheesh Gangadhar]

> -----Original Message-----
> From: Greg KH [mailto:gregkh at suse.de]
> Sent: Friday, February 18, 2011 10:36 PM
> To: TK, Pratheesh Gangadhar
> Cc: Hans J. Koch; davinci-linux-open-source at linux.davincidsp.com;
> Chatterjee, Amit; linux-kernel at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 1/2] PRUSS UIO driver support
> 
> On Fri, Feb 18, 2011 at 05:31:47PM +0100, Hans J. Koch wrote:
> > On Fri, Feb 18, 2011 at 08:35:29PM +0530, Pratheesh Gangadhar wrote:
> > > Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>
> 
> As noted by others, this needs to go at the end of the changelog
> comment.
> 
> Also, always run your patches through the scripts/checkpatch.pl script
> and fix the warnings and errors it finds.  To not do so is just rude as
> you are asking us to do the basic review work that you yourself did not
> do in the first place.

I actually ran checkpatch multiple times, but made some more changes before sending and missed to run again. Sorry about this. Will make sure these issues are fixed in the next version.
> 
> > > +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> > > +{
> > > +	return IRQ_HANDLED;
> > > +}
> >
> > ROTFL. That reminds me of an old story. The last time I wrote this, and
> > Greg dared to post it, we received this reply:
> >
> > http://marc.info/?l=linux-kernel&m=116604101232144&w=2
> >
> > So, if you really have a _very_ good reason why this _always_ works on
> > _any_ DA850 board, add a comment that explains why. Otherwise the whole
> > patch set will be doomed.
> 
> Nope, this whole patch set is doomed if this isn't fixed, I'm not going
> to accept it no matter how much you want to try to say this is ok on
> this hardware.
Okay. I had some queries in the previous mail. Will fix this once I am clear about the options.

Thanks,
Pratheesh

[PATCH 1/2] PRUSS UIO driver support

[TK, Pratheesh Gangadhar]

> -----Original Message-----
> From: Hans J. Koch [mailto:hjk at hansjkoch.de]
> Sent: Friday, February 18, 2011 10:02 PM
> To: TK, Pratheesh Gangadhar
> Cc: davinci-linux-open-source at linux.davincidsp.com; hjk at hansjkoch.de;
> gregkh at suse.de; Chatterjee, Amit; linux-kernel at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 1/2] PRUSS UIO driver support
> 
> On Fri, Feb 18, 2011 at 08:35:29PM +0530, Pratheesh Gangadhar wrote:
> > Signed-off-by: Pratheesh Gangadhar <pratheesh@ti.com>
> >
> > This patch implements PRUSS (Programmable Real-time Unit Sub System)
> > UIO driver which exports SOC resources associated with PRUSS like
> > I/O, memories and IRQs to user space. PRUSS is dual 32-bit RISC
> > processors which is efficient in performing embedded tasks that
> > require manipulation of packed memory mapped data structures and
> > efficient in handling system events that have tight real time
> > constraints. This driver is currently supported on Texas Instruments
> > DA850, AM18xx and OMAPL1-38 devices.
> > For example, PRUSS runs firmware for real-time critical industrial
> > communication data link layer and communicates with application stack
> > running in user space via shared memory and IRQs.
> 
> I see a few issues, comments below.
> 
> Thanks,
> Hans
> 
> > ---
> >  drivers/uio/Kconfig     |   10 ++
> >  drivers/uio/Makefile    |    1 +
> >  drivers/uio/uio_pruss.c |  250
> +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 261 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/uio/uio_pruss.c
> >
> > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> > index bb44079..631ffe3 100644
> > --- a/drivers/uio/Kconfig
> > +++ b/drivers/uio/Kconfig
> > @@ -94,4 +94,14 @@ config UIO_NETX
> >  	  To compile this driver as a module, choose M here; the module
> >  	  will be called uio_netx.
> >
> > +config UIO_PRUSS
> > +	tristate "Texas Instruments PRUSS driver"
> > +	depends on ARCH_DAVINCI_DA850
> > +	default n
> 
> That line is unneccessary, "n" is already the default.
> 
Ok, will fix in the next version.
> > +	help
> > +	  PRUSS driver for OMAPL138/DA850/AM18XX devices
> > +	  PRUSS driver requires user space components
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called uio_pruss.
> > +
> >  endif
> > +#define MAX_PRUSS_EVTOUT_INSTANCE	(8)
> 
> The brackets are not needed.
> 
Will fix in the next version.
> > +
> > +static struct clk *pruss_clk;
> > +static struct uio_info *info[MAX_PRUSS_EVTOUT_INSTANCE];
> 
> is it really neccessary to allocate that statically?
> 
> > +static void *ddr_virt_addr;
> > +static dma_addr_t ddr_phy_addr;
> > +
Agree - not necessary - will fix.

> > +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> > +{
> > +	return IRQ_HANDLED;
> > +}
> 
> ROTFL. That reminds me of an old story. The last time I wrote this, and
> Greg dared to post it, we received this reply:
> 
> http://marc.info/?l=linux-kernel&m=116604101232144&w=2
> 
> So, if you really have a _very_ good reason why this _always_ works on
> _any_ DA850 board, add a comment that explains why. Otherwise the whole
> patch set will be doomed.
> 
It always worked for me during the tests on the h/w. So did not bother to dig into the details then. From AM1808/AM1810 ARM Microprocessor System Reference Guide (http://focus.ti.com/general/docs/lit/getliterature.tsp?literatureNumber=sprugm9a&fileType=pdf), section 11.3.1 Interrupt processing
The interrupt processing block does the following tasks:
	-Synchronization of slower and asynchronous interrupts
	- Conversion of polarity to active high
	- Conversion of interrupt type to pulse interrupts
After the processing block, all interrupts will be active-high pulses

Interrupt processing is the first step in INTC h/w which maps system interrupts to ARM (host) interrupts (FIQ, IRQ). 

However I am willing to clean this up to meet the kernel guidelines and good practices... 
> > +
> > +static int __devinit pruss_probe(struct platform_device *dev)
> > +{
> > +	int ret = -ENODEV;
> > +	int count = 0;
> > +	struct resource *regs_pruram, *regs_l3ram, *regs_ddr;
> > +	char *string;
> > +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
> > +		info[count]->mem[0].addr = regs_pruram->start;
> > +		info[count]->mem[0].size =
> > +		    regs_pruram->end - regs_pruram->start + 1;
> > +		if (!info[count]->mem[0].addr
> > +		    || !(info[count]->mem[0].size - 1)) {
> 
> That size check looks fishy. If somebody forgot to set the size it's OK ?
> 
size is set just previous line, right? If regs_prum_ram->end== regs_prum_ram->start - then there is something wrong... 

> > +			dev_err(&dev->dev, "Invalid memory resource\n");
> > +			break;
> > +		}
> > +		info[count]->mem[0].internal_addr =
> > +		    ioremap(regs_pruram->start, info[count]->mem[0].size);
> > +		if (!info[count]->mem[0].internal_addr) {
> > +			dev_err(&dev->dev,
> > +				"Can't remap memory address range\n");
> > +			break;
> > +		}
> > +		/* Register PRUSS IRQ lines */
> > +		info[count]->irq = IRQ_DA8XX_EVTOUT0 + count;
> > +
> > +		info[count]->irq_flags = IRQF_SHARED;
> 
> How do you handle shared interrupts with the handler above?
> 
> > +		info[count]->handler = pruss_handler;
> 
> And how do you make sure your interrupts are not level triggered? The
> handler above will hang for level triggered interrupts.

As mentioned above interrupt controller (ARM INTC) converts interrupt type to pulse. After required processing is complete - user space handler clears the interrupt from PRUSS.

Thanks,
Pratheesh

[PATCH 1/2] PRUSS UIO driver support

[Thomas Gleixner]

On Fri, 18 Feb 2011, Pratheesh Gangadhar wrote:
> +/*
> + * Host event IRQ numbers from PRUSS 	
> + * 3 PRU_EVTOUT0 PRUSS Interrupt
> + * 4 PRU_EVTOUT1 PRUSS Interrupt
> + * 5 PRU_EVTOUT2 PRUSS Interrupt
> + * 6 PRU_EVTOUT3 PRUSS Interrupt
> + * 7 PRU_EVTOUT4 PRUSS Interrupt
> + * 8 PRU_EVTOUT5 PRUSS Interrupt
> + * 9 PRU_EVTOUT6 PRUSS Interrupt
> + * 10 PRU_EVTOUT7 PRUSS Interrupt
> +*/
> +
> +#define MAX_PRUSS_EVTOUT_INSTANCE	(8)
> +
> +static struct clk *pruss_clk;
> +static struct uio_info *info[MAX_PRUSS_EVTOUT_INSTANCE];
> +static void *ddr_virt_addr;
> +static dma_addr_t ddr_phy_addr;
> +
> +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> +{
> +	return IRQ_HANDLED;

  See other mail.

> +}
> +
> +static int __devinit pruss_probe(struct platform_device *dev)
> +{
> +	int ret = -ENODEV;
> +	int count = 0;

  Please make this one line.

> +	struct resource *regs_pruram, *regs_l3ram, *regs_ddr;
> +	char *string;
> +
> +	/* Power on PRU in case its not done as part of boot-loader */
> +	pruss_clk = clk_get(&dev->dev, "pruss");
> +	if (IS_ERR(pruss_clk)) {
> +		dev_err(&dev->dev, "Failed to get clock\n");
> +		ret = PTR_ERR(pruss_clk);
> +		pruss_clk = NULL;
> +		return ret;
> +	} else {
> +		clk_enable(pruss_clk);
> +	}
> +
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
> +		info[count] = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
> +		if (!info[count])
> +			return -ENOMEM;

  That leaks memory in case of count > 0

  And this whole loop is crap:

  struct uio_info *info = kzalloc(sizeof(struct uio_info) * MAX_PRUSS_EVTOUT_INSTANCE,
  	 	  	  	  GFP_KERNEL);

  perhaps ?		

> +	}
> +
> +	regs_pruram = platform_get_resource(dev, IORESOURCE_MEM, 0);
> +	if (!regs_pruram) {
> +		dev_err(&dev->dev, "No memory resource specified\n");
> +		goto out_free;
> +	}
> +
> +	regs_l3ram = platform_get_resource(dev, IORESOURCE_MEM, 1);
> +	if (!regs_l3ram) {
> +		dev_err(&dev->dev, "No memory resource specified\n");
> +		goto out_free;
> +	}
> +
> +	regs_ddr = platform_get_resource(dev, IORESOURCE_MEM, 2);
> +	if (!regs_ddr) {
> +		dev_err(&dev->dev, "No memory resource specified\n");
> +		goto out_free;
> +	}
> +	ddr_virt_addr =
> +	    dma_alloc_coherent(&dev->dev, regs_ddr->end - regs_ddr->start + 1,
> +			       &ddr_phy_addr, GFP_KERNEL | GFP_DMA);
> +	if (!ddr_virt_addr) {
> +		dev_err(&dev->dev, "Could not allocate external memory\n");
> +		goto out_free;
> +	}
> +
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
> +		info[count]->mem[0].addr = regs_pruram->start;
> +		info[count]->mem[0].size =
> +		    regs_pruram->end - regs_pruram->start + 1;
> +		if (!info[count]->mem[0].addr
> +		    || !(info[count]->mem[0].size - 1)) {
> +			dev_err(&dev->dev, "Invalid memory resource\n");
> +			break;

  Is this break intentional ? Assume you have registered one uio
  instance already then ret = 0 and you fall into the good path below.

> +		}
> +		info[count]->mem[0].internal_addr =
> +		    ioremap(regs_pruram->start, info[count]->mem[0].size);
> +		if (!info[count]->mem[0].internal_addr) {
> +			dev_err(&dev->dev,
> +				"Can't remap memory address range\n");
> +			break;

  Ditto

> +		}
> +		info[count]->mem[0].memtype = UIO_MEM_PHYS;
> +
> +		info[count]->mem[1].addr = regs_l3ram->start;
> +		info[count]->mem[1].size =
> +		    regs_l3ram->end - regs_l3ram->start + 1;
> +		if (!info[count]->mem[1].addr
> +		    || !(info[count]->mem[1].size - 1)) {
> +			dev_err(&dev->dev, "Invalid memory resource\n");
> +			break;
> +		}
> +		info[count]->mem[1].internal_addr =
> +		    ioremap(regs_l3ram->start, info[count]->mem[1].size);
> +		if (!info[count]->mem[1].internal_addr) {
> +			dev_err(&dev->dev,
> +				"Can't remap memory address range\n");
> +			break;
> +		}
> +		info[count]->mem[1].memtype = UIO_MEM_PHYS;
> +
> +		info[count]->mem[2].addr = ddr_phy_addr;
> +		info[count]->mem[2].size = regs_ddr->end - regs_ddr->start + 1;
> +		if (!info[count]->mem[2].addr
> +		    || !(info[count]->mem[2].size - 1)) {
> +			dev_err(&dev->dev, "Invalid memory resource\n");
> +			break;

  This is silly. If ddr_virt_addr == NULL you never reach that code.


> +		}
> +		info[count]->mem[2].internal_addr = ddr_virt_addr;
> +		if (!info[count]->mem[2].internal_addr) {
> +			dev_err(&dev->dev,
> +				"Can't remap memory address range\n");

  This is silly. If ddr_virt_addr == NULL you never reach that code.

> +			break;
> +		}
> +		info[count]->mem[2].memtype = UIO_MEM_PHYS;
> +
> +		string = kzalloc(20, GFP_KERNEL);
> +		sprintf(string, "pruss_evt%d", count);
> +		info[count]->name = string;
> +		info[count]->version = "0.50";
> +
> +		/* Register PRUSS IRQ lines */
> +		info[count]->irq = IRQ_DA8XX_EVTOUT0 + count;
> +
> +		info[count]->irq_flags = IRQF_SHARED;

  Huch. That can be shared ? Then you must in the interrupt handler

  1) check whether the interrupt is originated from your device
  2) mask at the device level.

> +		info[count]->handler = pruss_handler;
> +
> +		ret = uio_register_device(&dev->dev, info[count]);
> +
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	if (ret < 0) {
> +		if (ddr_virt_addr)
> +			dma_free_coherent(&dev->dev,
> +					  regs_ddr->end - regs_ddr->start + 1,
> +					  ddr_virt_addr, ddr_phy_addr);
> +		while (count--) {
> +			uio_unregister_device(info[count]);
> +			kfree(info[count]->name);
> +			iounmap(info[count]->mem[0].internal_addr);

  Please move that section below the return 0 and use goto out_uio;
  instead of break;

  This is real horrible.

> +		}
> +	} else {
> +		platform_set_drvdata(dev, info);
> +		return 0;
> +	}
> +
> +out_free:
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++)
> +		kfree(info[count]);
> +
> +	if (pruss_clk != NULL)
> +		clk_put(pruss_clk);
> +
> +	return ret;
> +}
> +
> +static int __devexit pruss_remove(struct platform_device *dev)
> +{
> +	int count = 0;
> +	struct uio_info **info;
> +
> +	info = (struct uio_info **)platform_get_drvdata(dev);
> +
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
> +		uio_unregister_device(info[count]);
> +		kfree(info[count]->name);

  In the above error exit path you do:

		iounmap(info[count]->mem[0].internal_addr);

  And in both cases you dont unmap info[count]->mem[1].internal_addr

  Why do you have those kernel mappings at all if you do not access
  the device from this code ?

> +
> +	}
> +	iounmap(info[0]->mem[0].internal_addr);
> +	iounmap(info[0]->mem[1].internal_addr);

  Sigh

> +	if (ddr_virt_addr)
> +		dma_free_coherent(&dev->dev, info[0]->mem[2].size,
> +				  info[0]->mem[2].internal_addr,
> +				  info[0]->mem[2].addr);
> +
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++)
> +		kfree(info[count]);
> +
> +	platform_set_drvdata(dev, NULL);
> +
> +	if (pruss_clk != NULL)
> +		clk_put(pruss_clk);
> +	return 0;

So you have basically the same cleanup code twice with different bugs.
Please avoid this kind of mistake which will happen with duplicated
code. The right way to do that is creating a cleanup function and call
them from both places.

You can call uio_unregister_device on a non registered info struct as
well. So all it takes is:

	if (ddr_virt_addr)
		dma_free_coherent(&dev->dev, info[0]->mem[2].size,
				  info[0]->mem[2].internal_addr,
				  info[0]->mem[2].addr);

	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
      	  	if (!info[count])
			break;

		uio_unregister_device(info[count]);
		kfree(info[count]->name);
		...
		kfree(info[count]);
		info[count] = NULL; 
        }

	platform_set_drvdata(dev, NULL);

	if (pruss_clk)
		clk_put(pruss_clk);

Thanks,

	tglx

[PATCH 1/2] PRUSS UIO driver support

[TK, Pratheesh Gangadhar]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx at linutronix.de]
> Sent: Friday, February 18, 2011 10:22 PM
> To: TK, Pratheesh Gangadhar
> Cc: davinci-linux-open-source at linux.davincidsp.com; Hans J. Koch;
> gregkh at suse.de; Chatterjee, Amit; LKML; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 1/2] PRUSS UIO driver support
> 
> On Fri, 18 Feb 2011, Pratheesh Gangadhar wrote:
> > +/*
> > + * Host event IRQ numbers from PRUSS
> > + * 3 PRU_EVTOUT0 PRUSS Interrupt
> > + * 4 PRU_EVTOUT1 PRUSS Interrupt
> > + * 5 PRU_EVTOUT2 PRUSS Interrupt
> > + * 6 PRU_EVTOUT3 PRUSS Interrupt
> > + * 7 PRU_EVTOUT4 PRUSS Interrupt
> > + * 8 PRU_EVTOUT5 PRUSS Interrupt
> > + * 9 PRU_EVTOUT6 PRUSS Interrupt
> > + * 10 PRU_EVTOUT7 PRUSS Interrupt
> > +*/
> > +
> > +#define MAX_PRUSS_EVTOUT_INSTANCE	(8)
> > +
> > +static struct clk *pruss_clk;
> > +static struct uio_info *info[MAX_PRUSS_EVTOUT_INSTANCE];
> > +static void *ddr_virt_addr;
> > +static dma_addr_t ddr_phy_addr;
> > +
> > +static irqreturn_t pruss_handler(int irq, struct uio_info *dev_info)
> > +{
> > +	return IRQ_HANDLED;
> 
>   See other mail.
> 
I responded in the other mail.

> > +}
> > +
> > +static int __devinit pruss_probe(struct platform_device *dev)
> > +{
> > +	int ret = -ENODEV;
> > +	int count = 0;
> 
>   Please make this one line.
> 
Sure, will do.
> > +	struct resource *regs_pruram, *regs_l3ram, *regs_ddr;
> > +	char *string;
> > +
> > +	/* Power on PRU in case its not done as part of boot-loader */
> > +	pruss_clk = clk_get(&dev->dev, "pruss");
> > +	if (IS_ERR(pruss_clk)) {
> > +		dev_err(&dev->dev, "Failed to get clock\n");
> > +		ret = PTR_ERR(pruss_clk);
> > +		pruss_clk = NULL;
> > +		return ret;
> > +	} else {
> > +		clk_enable(pruss_clk);
> > +	}
> > +
> > +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
> > +		info[count] = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
> > +		if (!info[count])
> > +			return -ENOMEM;
> 
>   That leaks memory in case of count > 0
> 
Correct...
>   And this whole loop is crap:
> 
>   struct uio_info *info = kzalloc(sizeof(struct uio_info) *
> MAX_PRUSS_EVTOUT_INSTANCE,
>   	 	  	  	  GFP_KERNEL);
> 
>   perhaps ?
> 
Will do.
> > +	}
> > +
> > +	regs_pruram = platform_get_resource(dev, IORESOURCE_MEM, 0);
> > +	if (!regs_pruram) {
> > +		dev_err(&dev->dev, "No memory resource specified\n");
> > +		goto out_free;
> > +	}
> > +
> > +	regs_l3ram = platform_get_resource(dev, IORESOURCE_MEM, 1);
> > +	if (!regs_l3ram) {
> > +		dev_err(&dev->dev, "No memory resource specified\n");
> > +		goto out_free;
> > +	}
> > +
> > +	regs_ddr = platform_get_resource(dev, IORESOURCE_MEM, 2);
> > +	if (!regs_ddr) {
> > +		dev_err(&dev->dev, "No memory resource specified\n");
> > +		goto out_free;
> > +	}
> > +	ddr_virt_addr =
> > +	    dma_alloc_coherent(&dev->dev, regs_ddr->end - regs_ddr->start +
> 1,
> > +			       &ddr_phy_addr, GFP_KERNEL | GFP_DMA);
> > +	if (!ddr_virt_addr) {
> > +		dev_err(&dev->dev, "Could not allocate external memory\n");
> > +		goto out_free;
> > +	}
> > +
> > +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
> > +		info[count]->mem[0].addr = regs_pruram->start;
> > +		info[count]->mem[0].size =
> > +		    regs_pruram->end - regs_pruram->start + 1;
> > +		if (!info[count]->mem[0].addr
> > +		    || !(info[count]->mem[0].size - 1)) {
> > +			dev_err(&dev->dev, "Invalid memory resource\n");
> > +			break;
> 
>   Is this break intentional ? Assume you have registered one uio
>   instance already then ret = 0 and you fall into the good path below.
> 
This is a bug. Will fix...
> > +		}
> > +		info[count]->mem[0].internal_addr =
> > +		    ioremap(regs_pruram->start, info[count]->mem[0].size);
> > +		if (!info[count]->mem[0].internal_addr) {
> > +			dev_err(&dev->dev,
> > +				"Can't remap memory address range\n");
> > +			break;
> 
>   Ditto
> 
> > +		}
> > +		info[count]->mem[0].memtype = UIO_MEM_PHYS;
> > +
> > +		info[count]->mem[1].addr = regs_l3ram->start;
> > +		info[count]->mem[1].size =
> > +		    regs_l3ram->end - regs_l3ram->start + 1;
> > +		if (!info[count]->mem[1].addr
> > +		    || !(info[count]->mem[1].size - 1)) {
> > +			dev_err(&dev->dev, "Invalid memory resource\n");
> > +			break;
> > +		}
> > +		info[count]->mem[1].internal_addr =
> > +		    ioremap(regs_l3ram->start, info[count]->mem[1].size);
> > +		if (!info[count]->mem[1].internal_addr) {
> > +			dev_err(&dev->dev,
> > +				"Can't remap memory address range\n");
> > +			break;
> > +		}
> > +		info[count]->mem[1].memtype = UIO_MEM_PHYS;
> > +
> > +		info[count]->mem[2].addr = ddr_phy_addr;
> > +		info[count]->mem[2].size = regs_ddr->end - regs_ddr->start +
> 1;
> > +		if (!info[count]->mem[2].addr
> > +		    || !(info[count]->mem[2].size - 1)) {
> > +			dev_err(&dev->dev, "Invalid memory resource\n");
> > +			break;
> 
>   This is silly. If ddr_virt_addr == NULL you never reach that code.
> 
Right... Will fix.
> 
> > +		}
> > +		info[count]->mem[2].internal_addr = ddr_virt_addr;
> > +		if (!info[count]->mem[2].internal_addr) {
> > +			dev_err(&dev->dev,
> > +				"Can't remap memory address range\n");
> 
>   This is silly. If ddr_virt_addr == NULL you never reach that code.
> 
Right... Will fix.
> > +			break;
> > +		}
> > +		info[count]->mem[2].memtype = UIO_MEM_PHYS;
> > +
> > +		string = kzalloc(20, GFP_KERNEL);
> > +		sprintf(string, "pruss_evt%d", count);
> > +		info[count]->name = string;
> > +		info[count]->version = "0.50";
> > +
> > +		/* Register PRUSS IRQ lines */
> > +		info[count]->irq = IRQ_DA8XX_EVTOUT0 + count;
> > +
> > +		info[count]->irq_flags = IRQF_SHARED;
> 
>   Huch. That can be shared ? Then you must in the interrupt handler
> 
>   1) check whether the interrupt is originated from your device
>   2) mask at the device level.
This interrupt is not shared. Must admit I am newbie to linux interrupt framework. 
> 
> > +		info[count]->handler = pruss_handler;
> > +
> > +		ret = uio_register_device(&dev->dev, info[count]);
> > +
> > +		if (ret < 0)
> > +			break;
> > +	}
> > +
> > +	if (ret < 0) {
> > +		if (ddr_virt_addr)
> > +			dma_free_coherent(&dev->dev,
> > +					  regs_ddr->end - regs_ddr->start + 1,
> > +					  ddr_virt_addr, ddr_phy_addr);
> > +		while (count--) {
> > +			uio_unregister_device(info[count]);
> > +			kfree(info[count]->name);
> > +			iounmap(info[count]->mem[0].internal_addr);
> 
>   Please move that section below the return 0 and use goto out_uio;
>   instead of break;
> 
>   This is real horrible.
> 
Ok, will do.
> > +		}
> > +	} else {
> > +		platform_set_drvdata(dev, info);
> > +		return 0;
> > +	}
> > +
> > +out_free:
> > +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++)
> > +		kfree(info[count]);
> > +
> > +	if (pruss_clk != NULL)
> > +		clk_put(pruss_clk);
> > +
> > +	return ret;
> > +}
> > +
> > +static int __devexit pruss_remove(struct platform_device *dev)
> > +{
> > +	int count = 0;
> > +	struct uio_info **info;
> > +
> > +	info = (struct uio_info **)platform_get_drvdata(dev);
> > +
> > +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
> > +		uio_unregister_device(info[count]);
> > +		kfree(info[count]->name);
> 
>   In the above error exit path you do:
> 
> 		iounmap(info[count]->mem[0].internal_addr);
> 
>   And in both cases you dont unmap info[count]->mem[1].internal_addr
> 
>   Why do you have those kernel mappings at all if you do not access
>   the device from this code ?
> 
I got this from existing UIO drivers which I used as reference. Should have paid more attention as this h/w is quite different...Was under the impression that ioremap is necessary for the user space access to those memory regions.
> > +
> > +	}
> > +	iounmap(info[0]->mem[0].internal_addr);
> > +	iounmap(info[0]->mem[1].internal_addr);
> 
>   Sigh
> 
> > +	if (ddr_virt_addr)
> > +		dma_free_coherent(&dev->dev, info[0]->mem[2].size,
> > +				  info[0]->mem[2].internal_addr,
> > +				  info[0]->mem[2].addr);
> > +
> > +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++)
> > +		kfree(info[count]);
> > +
> > +	platform_set_drvdata(dev, NULL);
> > +
> > +	if (pruss_clk != NULL)
> > +		clk_put(pruss_clk);
> > +	return 0;
> 
> So you have basically the same cleanup code twice with different bugs.
> Please avoid this kind of mistake which will happen with duplicated
> code. The right way to do that is creating a cleanup function and call
> them from both places.
> 
Agree and will fix this.
> You can call uio_unregister_device on a non registered info struct as
> well. So all it takes is:
> 
> 	if (ddr_virt_addr)
> 		dma_free_coherent(&dev->dev, info[0]->mem[2].size,
> 				  info[0]->mem[2].internal_addr,
> 				  info[0]->mem[2].addr);
> 
> 	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++) {
>       	  	if (!info[count])
> 			break;
> 
> 		uio_unregister_device(info[count]);
> 		kfree(info[count]->name);
> 		...
> 		kfree(info[count]);
> 		info[count] = NULL;
>         }
> 
> 	platform_set_drvdata(dev, NULL);
> 
> 	if (pruss_clk)
> 		clk_put(pruss_clk);
> 
Thanks a lot for a detailed review. I will address these comments in the next revision of the patch... 

Thanks,
Pratheesh

[PATCH 1/2] PRUSS UIO driver support

[Russell King - ARM Linux]

On Fri, Feb 18, 2011 at 08:35:29PM +0530, Pratheesh Gangadhar wrote:
> +static int __devinit pruss_probe(struct platform_device *dev)
> +{
> +	int ret = -ENODEV;
> +	int count = 0;
> +	struct resource *regs_pruram, *regs_l3ram, *regs_ddr;
> +	char *string;
> +
> +	/* Power on PRU in case its not done as part of boot-loader */
> +	pruss_clk = clk_get(&dev->dev, "pruss");
> +	if (IS_ERR(pruss_clk)) {
> +		dev_err(&dev->dev, "Failed to get clock\n");
> +		ret = PTR_ERR(pruss_clk);
> +		pruss_clk = NULL;

Delete this line.

...
> +out_free:
> +	for (count = 0; count < MAX_PRUSS_EVTOUT_INSTANCE; count++)
> +		kfree(info[count]);
> +
> +	if (pruss_clk != NULL)

	if (!IS_ERR(pruss_clk))

> +		clk_put(pruss_clk);
...
> +static int __devexit pruss_remove(struct platform_device *dev)
...
> +	platform_set_drvdata(dev, NULL);
> +
> +	if (pruss_clk != NULL)

	if (!IS_ERR(pruss_clk))

> +		clk_put(pruss_clk);