* [PATCH 1/2] PRUSS UIO driver support
From: Greg KH @ 2011-02-18 17:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110218163147.GD4684@local>
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
^ permalink raw reply
* [PATCH 1/2] PRUSS UIO driver support
From: Thomas Gleixner @ 2011-02-18 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201102181731.05644.arnd@arndb.de>
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
^ permalink raw reply
* [PATCH 1/2] PRUSS UIO driver support
From: Thomas Gleixner @ 2011-02-18 16:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1298041530-26855-2-git-send-email-pratheesh@ti.com>
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
^ permalink raw reply
* [patch-v2.6.39 6/7] OMAP4430: hwmod data: Adding USBOTG
From: Cousson, Benoit @ 2011-02-18 16:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110218154135.GF11151@legolas.emea.dhcp.ti.com>
On 2/18/2011 4:41 PM, Balbi, Felipe wrote:
> Hi all,
>
> On Fri, Feb 18, 2011 at 03:11:11PM +0100, Cousson, Benoit wrote:
>> On 2/17/2011 7:18 PM, Tony Lindgren wrote:
>>> * Cousson, Benoit<b-cousson@ti.com> [110217 09:45]:
>>>>
>>>> Done, boot tested, and that does remove the warning about the
>>>> missing opt clock you had with the previous data.
>>>>
>>>> git://gitorious.org/omap-pm/linux.git for_2.6.39/omap4_hwmod_data
>>>>
>>>> Just let me know if it does break anything else.
>>>
>>> Pulled into omap-for-linus. Also now merged are the devel-hwspinlock
>>> and devel-mcspi branches.
>>
>> McSPI OMAP4 hwmod data are missing a couple of flags that break the boot for the moment.
>>
>> Govindraj should rebase on the omap4_hwmod_data branch and add the missing rev and dev_attr for the McSPI.
>>
>> Unfortunately, even with that patch omap-for-linus boot hangs after the following trace:
>>
>> [ 0.875091] omap_device: omap2_mcspi.1: new worst case activate latency 0: 30517
>> [ 0.886657] usbcore: registered new interface driver usbfs
>> [ 0.892822] usbcore: registered new interface driver hub
>> [ 0.898681] usbcore: registered new device driver usb
>> [ 0.904815] omap_i2c omap_i2c.1: bus 1 rev4.0 at 400 kHz
>
> I tested your branch merged with mine and it was working fine. What else
> is on omap-for-linus ? There are only 28 patches on that branch, a
> bisect would be quick ?
This patch is at least fixing the McSPI crash. I still don't have a clue
about the final hang :-(
Benoit
---
From b2190f0d339c9d843eb5e370d0db8b7090fbcfab Mon Sep 17 00:00:00 2001
From: Benoit Cousson <b-cousson@ti.com>
Date: Fri, 18 Feb 2011 14:01:06 +0100
Subject: [PATCH] OMAP4: hwmod data: Add rev and dev_attr fields in McSPI
- Add a rev attribute to identify various McSPI IP version.
- Add a dev_attr structure to provide the number of chipselect
supported by the instance.
Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Govindraj.R <govindraj.raja@ti.com>
---
arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 26
++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 84e795c..182aa79 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -24,6 +24,7 @@
#include <plat/cpu.h>
#include <plat/gpio.h>
#include <plat/dma.h>
+#include <plat/mcspi.h>
#include "omap_hwmod_common_data.h"
@@ -3114,6 +3115,7 @@ static struct omap_hwmod_class_sysconfig
omap44xx_mcspi_sysc = {
static struct omap_hwmod_class omap44xx_mcspi_hwmod_class = {
.name = "mcspi",
.sysc = &omap44xx_mcspi_sysc,
+ .rev = OMAP4_MCSPI_REV,
};
/* mcspi1 */
@@ -3156,6 +3158,11 @@ static struct omap_hwmod_ocp_if
*omap44xx_mcspi1_slaves[] = {
&omap44xx_l4_per__mcspi1,
};
+/* mcspi1 dev_attr */
+static struct omap2_mcspi_dev_attr mcspi1_dev_attr = {
+ .num_chipselect = 4,
+};
+
static struct omap_hwmod omap44xx_mcspi1_hwmod = {
.name = "mcspi1",
.class = &omap44xx_mcspi_hwmod_class,
@@ -3169,6 +3176,7 @@ static struct omap_hwmod omap44xx_mcspi1_hwmod = {
.clkctrl_reg = OMAP4430_CM_L4PER_MCSPI1_CLKCTRL,
},
},
+ .dev_attr = &mcspi1_dev_attr,
.slaves = omap44xx_mcspi1_slaves,
.slaves_cnt = ARRAY_SIZE(omap44xx_mcspi1_slaves),
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
@@ -3210,6 +3218,11 @@ static struct omap_hwmod_ocp_if
*omap44xx_mcspi2_slaves[] = {
&omap44xx_l4_per__mcspi2,
};
+/* mcspi2 dev_attr */
+static struct omap2_mcspi_dev_attr mcspi2_dev_attr = {
+ .num_chipselect = 2,
+};
+
static struct omap_hwmod omap44xx_mcspi2_hwmod = {
.name = "mcspi2",
.class = &omap44xx_mcspi_hwmod_class,
@@ -3223,6 +3236,7 @@ static struct omap_hwmod omap44xx_mcspi2_hwmod = {
.clkctrl_reg = OMAP4430_CM_L4PER_MCSPI2_CLKCTRL,
},
},
+ .dev_attr = &mcspi2_dev_attr,
.slaves = omap44xx_mcspi2_slaves,
.slaves_cnt = ARRAY_SIZE(omap44xx_mcspi2_slaves),
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
@@ -3264,6 +3278,11 @@ static struct omap_hwmod_ocp_if
*omap44xx_mcspi3_slaves[] = {
&omap44xx_l4_per__mcspi3,
};
+/* mcspi3 dev_attr */
+static struct omap2_mcspi_dev_attr mcspi3_dev_attr = {
+ .num_chipselect = 2,
+};
+
static struct omap_hwmod omap44xx_mcspi3_hwmod = {
.name = "mcspi3",
.class = &omap44xx_mcspi_hwmod_class,
@@ -3277,6 +3296,7 @@ static struct omap_hwmod omap44xx_mcspi3_hwmod = {
.clkctrl_reg = OMAP4430_CM_L4PER_MCSPI3_CLKCTRL,
},
},
+ .dev_attr = &mcspi3_dev_attr,
.slaves = omap44xx_mcspi3_slaves,
.slaves_cnt = ARRAY_SIZE(omap44xx_mcspi3_slaves),
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
@@ -3316,6 +3336,11 @@ static struct omap_hwmod_ocp_if
*omap44xx_mcspi4_slaves[] = {
&omap44xx_l4_per__mcspi4,
};
+/* mcspi4 dev_attr */
+static struct omap2_mcspi_dev_attr mcspi4_dev_attr = {
+ .num_chipselect = 1,
+};
+
static struct omap_hwmod omap44xx_mcspi4_hwmod = {
.name = "mcspi4",
.class = &omap44xx_mcspi_hwmod_class,
@@ -3329,6 +3354,7 @@ static struct omap_hwmod omap44xx_mcspi4_hwmod = {
.clkctrl_reg = OMAP4430_CM_L4PER_MCSPI4_CLKCTRL,
},
},
+ .dev_attr = &mcspi4_dev_attr,
.slaves = omap44xx_mcspi4_slaves,
.slaves_cnt = ARRAY_SIZE(omap44xx_mcspi4_slaves),
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
--
1.7.0.4
^ permalink raw reply related
* [PATCH 1/2] PRUSS UIO driver support
From: Hans J. Koch @ 2011-02-18 16:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1298041530-26855-2-git-send-email-pratheesh@ti.com>
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/
>
^ permalink raw reply
* [PATCH 1/2] PRUSS UIO driver support
From: Arnd Bergmann @ 2011-02-18 16:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <alpine.LFD.2.00.1102181713480.2701@localhost6.localdomain6>
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
^ permalink raw reply
* [PATCH 1/2] PRUSS UIO driver support
From: Thomas Gleixner @ 2011-02-18 16:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201102181644.17634.arnd@arndb.de>
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
^ permalink raw reply
* [RFC] MMC: error handling improvements
From: Brian Swetland @ 2011-02-18 16:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AANLkTin64W=VQP+EomU3PK-Ye=Nz_SP+DOLxgT__4fkQ@mail.gmail.com>
On Fri, Feb 18, 2011 at 5:03 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/2/17 Brian Swetland <swetland@google.com>:
>> On Wed, Feb 16, 2011 at 1:01 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> 2011/2/16 David Brown <davidb@codeaurora.org>:
>
>>>> The SDCC block is shared between
>>>> the modem processor and the processor running Linux. ?If the driver
>>>> doesn't go through the DMA engine, which coordinates this, the registers
>>>> will be stomped on by the other CPU whenever it decides to access it's
>>>> parts of the flash device.
>>> (...)
>>>
>>> At the same time what you're saying sounds very weird:
>>> the ios handler in mmc_sdcc does not request any DMA
>>> channel before messing with the hardware, it simply just write
>>> into registers very much in the style of mmci.c. Wouldn't that
>>> disturb any simultaneous access to the MMC from another
>>> CPU?
>>
>> (...)
>> The way register access for the SDCC is synchronized between these two
>> cores is by using a locking primitive built into the MSM DMA
>> controller. ?If you don't use this indirect access model (via DMA
>> transactions to the registers), you end up tripping over the baseband
>> or the other way 'round. ?It's not fun.
>
> Wherever is that synchronized in the DMA controller?
> I don't get it because the code is so very similar.
>
> When the ios does this:
> msmsdcc_writel(host, clk, MMCICLOCK);
Sorry, I'm wrong. I was thinking of the MSM NAND driver which does
have a funky DMA interlock access control scheme where the DMA engine
is use to arbitrate register access between the apps and modem cores.
You are correct -- there's no special magic in the SDCC driver.
If this can be handled by a common driver with a quirks flag for
msm-specific oddities, that sounds quite sane to me.
Brian
^ permalink raw reply
* [patch-v2.6.39 6/7] OMAP4430: hwmod data: Adding USBOTG
From: Cousson, Benoit @ 2011-02-18 15:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110218154135.GF11151@legolas.emea.dhcp.ti.com>
Hi Felipe,
On 2/18/2011 4:41 PM, Balbi, Felipe wrote:
> Hi all,
>
> On Fri, Feb 18, 2011 at 03:11:11PM +0100, Cousson, Benoit wrote:
>> On 2/17/2011 7:18 PM, Tony Lindgren wrote:
>>> * Cousson, Benoit<b-cousson@ti.com> [110217 09:45]:
>>>>
>>>> Done, boot tested, and that does remove the warning about the
>>>> missing opt clock you had with the previous data.
>>>>
>>>> git://gitorious.org/omap-pm/linux.git for_2.6.39/omap4_hwmod_data
>>>>
>>>> Just let me know if it does break anything else.
>>>
>>> Pulled into omap-for-linus. Also now merged are the devel-hwspinlock
>>> and devel-mcspi branches.
>>
>> McSPI OMAP4 hwmod data are missing a couple of flags that break the boot for the moment.
>>
>> Govindraj should rebase on the omap4_hwmod_data branch and add the missing rev and dev_attr for the McSPI.
>>
>> Unfortunately, even with that patch omap-for-linus boot hangs after the following trace:
>>
>> [ 0.875091] omap_device: omap2_mcspi.1: new worst case activate latency 0: 30517
>> [ 0.886657] usbcore: registered new interface driver usbfs
>> [ 0.892822] usbcore: registered new interface driver hub
>> [ 0.898681] usbcore: registered new device driver usb
>> [ 0.904815] omap_i2c omap_i2c.1: bus 1 rev4.0 at 400 kHz
>
> I tested your branch merged with mine and it was working fine. What else
> is on omap-for-linus ? There are only 28 patches on that branch, a
> bisect would be quick ?
This is what I observed too. There are some early stuff in it + some TI816X... I do not know what is going wrong.
Please note that there are a little bit more (47) stuff added but they are below the -rc5 tag... The bisect will be longer :-)
+ [HEAD^2] OMAP: runtime: McSPI driver runtime conversion
+ [HEAD^2^] OMAP: devices: Modify McSPI device to adapt to hwmod framework
+ [HEAD^2~2] OMAP3: hwmod data: Add McSPI
+ [HEAD^2~3] OMAP2430: hwmod data: Add McSPI
+ [HEAD^2~4] OMAP2420: hwmod data: Add McSPI
+ [HEAD^] omap: add hwspinlock device
+ [HEAD~2] drivers: hwspinlock: add OMAP implementation
+ [HEAD~3] drivers: hwspinlock: add framework
+ [HEAD~4^2] OMAP4: hwmod data: Add USBOTG
+ [HEAD~4^2^] OMAP4: hwmod data: Add AESS, McPDM, bandgap, counter_32k, MMC, KBD, ISS & IPU
+ [HEAD~4^2~2] OMAP4: hwmod data: Add McBSP
+ [HEAD~4^2~3] OMAP4: hwmod data: Add DMIC
+ [HEAD~4^2~4] OMAP4: hwmod data: Add mailbox
+ [HEAD~4^2~5] OMAP4: hwmod data: Add DSS, DISPC, DSI1&2, RFBI, HDMI and VENC
+ [HEAD~4^2~6] OMAP4: hwmod data: Add timer
+ [HEAD~4^2~7] OMAP4: hwmod data: Add McSPI
+ [HEAD~4^2~8] OMAP4: hwmod data: Add hwspinlock
+ [HEAD~5^3] TI816X: Update to use init_early
+ [HEAD~5^3^] TI816X: Add low level debug support
+ [HEAD~5^3~2] TI816X: Create board support and enable build for TI816X EVM
+ [HEAD~5^3~3] TI816X: Update common OMAP machine specific sources
+ [HEAD~5^3~4] TI816X: Update common omap platform files
+ [HEAD~5^3~5] omap: hwmod: Populate _mpu_rt_va later on in omap_hwmod_late_init
+ [HEAD~5^3~6] omap2+: Fix omap_serial_early_init to work with init_early hook
+ [HEAD~5^3~7] omap2+: Make omap_hwmod_late_init into core_initcall
+ [HEAD~5^3~8] ARM: OMAP2: use early init hook
+ [HEAD~5^2] omap: McBSP: Remove unused audio macros in mcbsp.h
+ [HEAD~5^2^] wip: fix section mismatches in omap1_defconfig
+ [HEAD~5^2~2] ARM: omap: move omap_board_config_kernel to .init.data
+ [HEAD~5^2~3] ARM: omap: move omap_get_config et al. to .init.text
+ [HEAD~5^2~4] ARM: omap1/nokia770: mark some functions __init
+ [HEAD~5^2~5] arm: mach-omap1: board-voiceblue: add missing include
+ [HEAD~5^2~6] ARM: OMAP: Allow platforms to hook reset cleanly
+ [HEAD~5^2~7] arm: mach-omap1: board-h3: make nand_platdata static
+ [HEAD~5^2~8] arm: mach-omap1: board-htcherald: make htcpld_chips and htcpld_pfdata static
+ [HEAD~5^2~9] arm: mach-omap1: board-innovator: make innovator_mmc_init() static
+ [HEAD~5^2~10] arm: mach-omap1: board-h2: make h2_nand_platdata static
+ [HEAD~5^2~11] arm: plat-omap: dma: make omap_dma_in_1510_mode() static
+ [HEAD~5^2~12] arm: omap2: irq: fix compile warning:
+ [HEAD~5^2~13] arm: omap1: fix a bunch of section mismatches
+ [HEAD~5^2~14] arm: omap1: fix compile warnings
+ [HEAD~5^2~15] arm: omap1: fix compile warning
+ [HEAD~5^2~16] arm: omap: i2c: fix compile warning
+ [HEAD~5^2~17] omap: Start using CONFIG_SOC_OMAP
++ [85e2efb] Linux 2.6.38-rc5
Regards,
Benoit
^ permalink raw reply
* [PATCH 1/2] PRUSS UIO driver support
From: Arnd Bergmann @ 2011-02-18 15:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1298041530-26855-2-git-send-email-pratheesh@ti.com>
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
^ permalink raw reply
* [patch-v2.6.39 6/7] OMAP4430: hwmod data: Adding USBOTG
From: Felipe Balbi @ 2011-02-18 15:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4D5E7DFF.7020609@ti.com>
Hi all,
On Fri, Feb 18, 2011 at 03:11:11PM +0100, Cousson, Benoit wrote:
> On 2/17/2011 7:18 PM, Tony Lindgren wrote:
> > * Cousson, Benoit<b-cousson@ti.com> [110217 09:45]:
> >>
> >> Done, boot tested, and that does remove the warning about the
> >> missing opt clock you had with the previous data.
> >>
> >> git://gitorious.org/omap-pm/linux.git for_2.6.39/omap4_hwmod_data
> >>
> >> Just let me know if it does break anything else.
> >
> > Pulled into omap-for-linus. Also now merged are the devel-hwspinlock
> > and devel-mcspi branches.
>
> McSPI OMAP4 hwmod data are missing a couple of flags that break the boot for the moment.
>
> Govindraj should rebase on the omap4_hwmod_data branch and add the missing rev and dev_attr for the McSPI.
>
> Unfortunately, even with that patch omap-for-linus boot hangs after the following trace:
>
> [ 0.875091] omap_device: omap2_mcspi.1: new worst case activate latency 0: 30517
> [ 0.886657] usbcore: registered new interface driver usbfs
> [ 0.892822] usbcore: registered new interface driver hub
> [ 0.898681] usbcore: registered new device driver usb
> [ 0.904815] omap_i2c omap_i2c.1: bus 1 rev4.0 at 400 kHz
I tested your branch merged with mine and it was working fine. What else
is on omap-for-linus ? There are only 28 patches on that branch, a
bisect would be quick ?
--
balbi
^ permalink raw reply
* [PATCH] ARM: pmu: avoid setting IRQ affinity on UP systems
From: Will Deacon @ 2011-02-18 15:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110217162421.GC941@pulham.picochip.com>
Hi Jamie,
> On Tue, Feb 15, 2011 at 12:11:39PM +0000, Will Deacon wrote:
> > Now that we can execute a CONFIG_SMP kernel on a uniprocessor system,
> > extra care has to be taken in the PMU IRQ affinity setting code to
> > ensure that we don't always fail to initialise.
> >
> > This patch changes the CPU PMU initialisation code so that when we
> > only have a single IRQ, whose affinity can not be changed at the
> > controller, we report success (0) rather than -EINVAL.
> >
> > Cc: Jamie Iles <jamie@jamieiles.com>
> > Reported-by: Avik Sil <avik.sil@linaro.org>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
>
> Look good!
>
> Acked-by: Jamie Iles <jamie@jamieiles.com>
Cheers. I've submitted this as 6742/1 with a minor change (deleted the out:
label as it's no longer used).
Will
^ permalink raw reply
* [PATCH v2 09/13] can: pruss CAN driver.
From: Arnd Bergmann @ 2011-02-18 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1297435892-28278-10-git-send-email-subhasish@mistralsolutions.com>
On Friday 11 February 2011, Subhasish Ghosh wrote:
> This patch adds support for the CAN device emulated on PRUSS.
Hi Subhasish,
This is a detailed walk through the can driver. The pruss_can.c
file mostly looks good, there are very tiny changes that I'm
suggesting to improve the code. I assume that you wrote that file.
The pruss_can_api.c is a bit of a mess and looks like it was copied
from some other code base and just barely changed to follow Linux
coding style. I can tell from the main driver file that you can do
better than that.
My recommendation for that would be to throw it away and reimplement
the few parts that you actually need, in proper coding style.
You can also try to fix the file according to the comments I give
you below, but I assume that would be signficantly more work.
Moving everything into one file also makes things easier to read
here and lets you identifer more quickly what is unused.
Arnd
> +#ifdef __CAN_DEBUG
> +#define __can_debug(fmt, args...) printk(KERN_DEBUG "can_debug: " fmt, ## args)
> +#else
> +#define __can_debug(fmt, args...)
> +#endif
> +#define __can_err(fmt, args...) printk(KERN_ERR "can_err: " fmt, ## args)
Better use the existing dev_dbg() and dev_err() macros that provide the
same functionality in a more standard way. You already use them in
some places, as I noticed.
If you don't have a way to pass a meaningful device, you can use
pr_debug/pr_err.
> +void omapl_pru_can_rx_wQ(struct work_struct *work)
> +{
This is only used in the same file, so better make it static.
> + if (-1 == pru_can_get_intr_status(priv->dev, &priv->can_rx_hndl))
> + return;
Don't make up your own return values, just use the standard error codes,
e.g. -EIO or -EAGAIN, whatever fits.
The more common way to write the comparison would be the other way round,
if (pru_can_get_intr_status(priv->dev, &priv->can_rx_hndl) == -EAGAIN)
return;
or, simpler
if (pru_can_get_intr_status(priv->dev, &priv->can_rx_hndl))
return;
> +irqreturn_t omapl_tx_can_intr(int irq, void *dev_id)
> +{
This also should be static
> + for (bit_set = 0; ((priv->can_tx_hndl.u32interruptstatus & 0xFF)
> + >> bit_set != 0); bit_set++)
> + ;
bit_set = fls(priv->can_tx_hndl.u32interruptstatus & 0xFF); ?
> +irqreturn_t omapl_rx_can_intr(int irq, void *dev_id)
static
> diff --git a/drivers/net/can/da8xx_pruss/pruss_can_api.c b/drivers/net/can/da8xx_pruss/pruss_can_api.c
> new file mode 100644
> index 0000000..2f7438a
> --- /dev/null
> +++ b/drivers/net/can/da8xx_pruss/pruss_can_api.c
A lot of code in this file seems to be unused. Is that right?
I would suggest adding only the code that is actually being
used. If you add more functionality later, you can always
add back the low-level functions, but dead code usually
turns into broken code quickly.
> +static can_emu_drv_inst gstr_can_inst[ecanmaxinst];
This is global data and probably needs some for of locking
> +/*
> + * pru_can_set_brp() Updates the BRP register of PRU0
> + * and PRU1 of OMAP L138. This API will be called by the
> + * Application to updtae the BRP register of PRU0 and PRU1
> + *
> + * param u16bitrateprescaler The can bus bitrate
> + * prescaler value be set
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_set_brp(struct device *dev, u16 u16bitrateprescaler)
> +{
unused.
> + u32 u32offset;
> +
> + if (u16bitrateprescaler > 255) {
> + return -1;
> + }
non-standard error code. It also doesn't match the comment, which
claims it is SUCCESS or FAILURE, both of which are (rightfully)
not defined.
> + u32offset = (PRU_CAN_RX_CLOCK_BRP_REGISTER);
> + pruss_writel(dev, u32offset, (u32 *) &u16bitrateprescaler, 1);
> +
> + u32offset = (PRU_CAN_TX_CLOCK_BRP_REGISTER);
> + pruss_writel(dev, u32offset, (u32 *) &u16bitrateprescaler, 1);
You pass a 32 bit pointer to a 16 bit local variable here.
This has an undefined effect, and if you build this code on
a big-endian platform, it cannot possibly do anything good.
pruss_writel() is defined in a funny way if it takes a thirty-two bit
input argument by reference, rather than by value. What is going
on there?
> +s16 pru_can_set_bit_timing(struct device *dev,
> + can_bit_timing_consts *pstrbittiming)
unused.
> + u32 u32offset;
> + u32 u32serregister;
It's a bit silly to put the name of the type into the name
of a variable. You already spell it out in the definition.
> +s16 pru_can_write_data_to_mailbox(struct device *dev,
> + can_emu_app_hndl *pstremuapphndl)
> +{
> + s16 s16subrtnretval;
> + u32 u32offset;
> +
> + if (pstremuapphndl == NULL) {
> + return -1;
> + }
nonstandard error code. Also, why the heck is type function
return type s16 when the only possible return values are 0
and -1? Just make this an int.
> + switch ((u8) pstremuapphndl->ecanmailboxnumber) {
> + case 0:
> + u32offset = (PRU_CAN_TX_MAILBOX0);
> + break;
> + case 1:
> + u32offset = (PRU_CAN_TX_MAILBOX1);
> + break;
> + case 2:
> + u32offset = (PRU_CAN_TX_MAILBOX2);
> + break;
> + case 3:
> + u32offset = (PRU_CAN_TX_MAILBOX3);
> + break;
> + case 4:
> + u32offset = (PRU_CAN_TX_MAILBOX4);
> + break;
> + case 5:
> + u32offset = (PRU_CAN_TX_MAILBOX5);
> + break;
> + case 6:
> + u32offset = (PRU_CAN_TX_MAILBOX6);
> + break;
> + case 7:
> + u32offset = (PRU_CAN_TX_MAILBOX7);
> + break;
> + default:
> + return -1;
> + }
Lovely switch statement. I'm sure you find a better way to express this ;-)
> + s16subrtnretval = pruss_writel(dev, u32offset,
> + (u32 *) &(pstremuapphndl->strcanmailbox), 4);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
> + return 0;
> +}
return pruss_writel(...) ?
> +
> +/*
> + * pru_can_get_intr_status()
> + * Gets the interrupts status register value.
> + * This API will be called by the Application
> + * to get the interrupts status register value
> + *
> + * param u8prunumber PRU number for which IntStatusReg
> + * has to be read
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_get_intr_status(struct device *dev,
> + can_emu_app_hndl *pstremuapphndl)
> +{
> + u32 u32offset;
> + s16 s16subrtnretval = -1;
> +
> + if (pstremuapphndl == NULL) {
> + return -1;
> + }
In every function you check that pstremuapphndl is present. This seems
rather pointless. How about just making sure you never pass a NULL
value to these functions? That should not be hard at all from the
high-level driver.
> + if (pstremuapphndl->u8prunumber == DA8XX_PRUCORE_1) {
> + u32offset = (PRU_CAN_TX_INTERRUPT_STATUS_REGISTER);
> + } else if (pstremuapphndl->u8prunumber == DA8XX_PRUCORE_0) {
> + u32offset = (PRU_CAN_RX_INTERRUPT_STATUS_REGISTER);
> + } else {
> + return -1;
> + }
> +
> + s16subrtnretval = pruss_readl(dev, u32offset,
> + (u32 *) &pstremuapphndl->u32interruptstatus, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
You can also get rid of all these {} braces around one-line statements.
> +/*
> + * pru_can_get_mailbox_status() Gets the mailbox status
> + * register value. This API will be called by the Application
> + * to get the mailbox status register value
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_get_mailbox_status(struct device *dev,
> + can_emu_app_hndl *pstremuapphndl)
unused.
> +/*
> + * pru_can_config_mode_set() Sets the timing value
> + * for data transfer. This API will be called by the Application
> + * to set timing valus for data transfer
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_config_mode_set(struct device *dev, bool bconfigmodeflag)
unused.
> + u32offset = (PRU_CAN_TX_GLOBAL_CONTROL_REGISTER & 0xFFFF);
> + u32value = 0x00000000;
> + s16subrtnretval = pruss_writel(dev, u32offset, (u32 *) &u32value, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
> +
> + u32offset = (PRU_CAN_TX_GLOBAL_STATUS_REGISTER & 0xFFFF);
> + u32value = 0x00000040;
> + s16subrtnretval = pruss_writel(dev, u32offset, (u32 *) &u32value, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
> + u32offset = (PRU_CAN_RX_GLOBAL_STATUS_REGISTER & 0xFFFF);
> + u32value = 0x00000040;
> + s16subrtnretval = pruss_writel(dev, u32offset, (u32 *) &u32value, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
<skipping 50 (!) more of these>
After the third time of writing the same code, you should have noticed that
there is some duplication involved that can trivially be reduced. A good
way to express the same would be a table with the contents:
static struct pru_can_register_init {
u16 offset;
u32 value;
} = {
{ PRU_CAN_TX_GLOBAL_CONTROL_REGISTER, 0, },
{ PRU_CAN_TX_GLOBAL_STATUS_REGISTER, 0x40, },
{ PRU_CAN_RX_GLOBAL_STATUS_REGISTER, 0x40, },
...
};
> +
> +
> +/*
> + * pru_can_emu_open() Opens the can emu for
> + * application to use. This API will be called by the Application
> + * to Open the can emu for application to use.
> + *
> + * param pstremuapphndl Pointer to application handler
> + * structure
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_emu_open(struct device *dev, can_emu_app_hndl *pstremuapphndl)
unused.
> +/*
> + * brief pru_can_emu_close() Closes the can emu for other
> + * applications to use. This API will be called by the Application to Close
> + * the can emu for other applications to use
> + *
> + * param pstremuapphndl Pointer to application handler structure
> + *
> + * return SUCCESS or FAILURE
> + */
> +s16 pru_can_emu_close(struct device *dev, can_emu_app_hndl *pstremuapphndl)
unused
> +s16 pru_can_emu_sreset(struct device *dev)
> +{
> + return 0;
> +}
pointless.
> +s16 pru_can_tx(struct device *dev, u8 u8mailboxnumber, u8 u8prunumber)
> +{
> + u32 u32offset = 0;
> + u32 u32value = 0;
> + s16 s16subrtnretval = -1;
> +
> + if (DA8XX_PRUCORE_1 == u8prunumber) {
> + switch (u8mailboxnumber) {
> + case 0:
> + u32offset = (PRU_CAN_TX_MAILBOX0_STATUS_REGISTER & 0xFFFF);
> + u32value = 0x00000080;
> + s16subrtnretval = pruss_writel(dev, u32offset,
> + (u32 *) &u32value, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
> + break;
> + case 1:
> + u32offset = (PRU_CAN_TX_MAILBOX1_STATUS_REGISTER & 0xFFFF);
> + u32value = 0x00000080;
> + s16subrtnretval = pruss_writel(dev, u32offset,
> + (u32 *) &u32value, 1);
> + if (s16subrtnretval == -1) {
> + return -1;
> + }
> + break;
> + case 2:
> + u32offset = (PRU_CAN_TX_MAILBOX2_STATUS_REGISTER & 0xFFFF);
Another pointless switch statement.
> +s16 pru_can_mask_ints(struct device *dev, u32 int_mask)
> +{
> + return 0;
> +}
> +
> +int pru_can_get_error_cnt(struct device *dev, u8 u8prunumber)
> +{
> + return 0;
> +}
useless.
> +int pru_can_get_intc_status(struct device *dev)
> +{
> + u32 u32offset = 0;
> + u32 u32getvalue = 0;
> + u32 u32clrvalue = 0;
> +
> + u32offset = (PRUSS_INTC_STATCLRINT1 & 0xFFFF);
> + pruss_readl(dev, u32offset, (u32 *) &u32getvalue, 1);
> +
> + if (u32getvalue & 4)
> + u32clrvalue = 34; /* CLR Event 34 */
> +
> + if (u32getvalue & 2)
> + u32clrvalue = 33; /* CLR Event 33 */
> +
> + if (u32clrvalue) {
> + u32offset = (PRUSS_INTC_STATIDXCLR & 0xFFFF);
> + pruss_writel(dev, u32offset, &u32clrvalue, 1);
> + } else
> + return -1;
Could the controller signal both event 34 and 33 simultaneously?
The only user of this function looks at the individual bits
of the return value again, which looks wrong for all possible
return values here.
> +#ifndef _PRU_CAN_API_H_
> +#define CAN_BIT_TIMINGS (0x273)
> +
> +/* Timer Clock is sourced from DDR freq (PLL1 SYS CLK 2) */
> +#define TIMER_CLK_FREQ 132000000
> +
> +#define TIMER_SETUP_DELAY 14
> +#define GPIO_SETUP_DELAY 150
> +
> +#define CAN_RX_PRU_0 PRUSS_NUM0
> +#define CAN_TX_PRU_1 PRUSS_NUM1
> +
> +/* Number of Instruction in the Delay loop */
> +#define DELAY_LOOP_LENGTH 2
> +
> +#define PRU1_BASE_ADDR 0x2000
> +
> +#define PRU_CAN_TX_GLOBAL_CONTROL_REGISTER (PRU1_BASE_ADDR)
> +#define PRU_CAN_TX_GLOBAL_STATUS_REGISTER (PRU1_BASE_ADDR + 0x04)
> +#define PRU_CAN_TX_INTERRUPT_MASK_REGISTER (PRU1_BASE_ADDR + 0x08)
> +#define PRU_CAN_TX_INTERRUPT_STATUS_REGISTER (PRU1_BASE_ADDR + 0x0C)
> +#define PRU_CAN_TX_MAILBOX0_STATUS_REGISTER (PRU1_BASE_ADDR + 0x10)
> +#define PRU_CAN_TX_MAILBOX1_STATUS_REGISTER (PRU1_BASE_ADDR + 0x14)
The header file should be used for interfaces between the two .c files,
don't mix that with hardware specific definitions. Sometimes you may want
to have register number lists in a header, if that list is going to be
used in multiple places. In this case, there is just one user, so better
move all those definitions over there.
> +typedef enum {
> + ecaninst0 = 0,
> + ecaninst1,
> + ecanmaxinst
> +} can_instance_enum;
> +
> +typedef enum {
> + ecanmailbox0 = 0,
> + ecanmailbox1,
> + ecanmailbox2,
> + ecanmailbox3,
> + ecanmailbox4,
> + ecanmailbox5,
> + ecanmailbox6,
> + ecanmailbox7
> +} can_mailbox_number;
> +
> +typedef enum {
> + ecandirectioninit = 0,
> + ecantransmit,
> + ecanreceive
> +} can_transfer_direction;
The values are all unused, you only use the typedefs.
IMHO it would be more sensible to just pass these as unsigned int
or u32 values, but if you prefer, there is no reason to just do
typedef u32 can_mailbox_number;
etc.
> +typedef struct {
> + u16 u16extendedidentifier;
> + u16 u16baseidentifier;
> + u8 u8data7;
> + u8 u8data6;
> + u8 u8data5;
> + u8 u8data4;
> + u8 u8data3;
> + u8 u8data2;
> + u8 u8data1;
> + u8 u8data0;
> + u16 u16datalength;
> + u16 u16crc;
> +} can_mail_box_structure;
Please don't use typedef for complex data structures, and learn about
better naming of identifiers. I would suggest writing this as
struct pru_can_mailbox {
u16 ext_id;
u16 base_id;
u8 data[8]; /* note: reverse order */
u16 len;
u16 crc;
};
Sames rules apply to the other structures.
Arnd
^ permalink raw reply
* platform/i2c busses: pm runtime and system sleep
From: Alan Stern @ 2011-02-18 15:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AANLkTinsPqFcodH0w7LQeFEY+amodNH4CneRCRhhbKaz@mail.gmail.com>
On Fri, 18 Feb 2011, Rabin Vincent wrote:
> How about something like the below? If we have something like this, we
> can just switch platform to GENERIC_PM_OPS and add the
> pm_runtime_want_interaction() (or something better named)
Yes, please find a better name! Maybe something starting with
"generic_" to indicate that this applies only to devices using the
GENERIC_PM_OPS.
> call to the
> i2c and spi drivers using runtime PM.
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 42615b4..2b8a099 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1069,6 +1069,30 @@ void pm_runtime_allow(struct device *dev)
> EXPORT_SYMBOL_GPL(pm_runtime_allow);
>
> /**
> + * pm_runtime_want_interaction - Enable interaction between system sleep
> + * and runtime PM callbacks at the bus/subsystem
> + * level.
> + * @dev: Device to handle
> + *
> + * Set the power.want_interaction flage, which tells the generic PM subsystem
> + * ops that the following actions should be done during system suspend/resume:
> + *
> + * - If the device has been runtime suspended, the driver's
> + * suspend() handler will not be invoked.
> + *
> + * - If the device has a resume() pm callback, and the resume()
> + * callback returns success on system resume, the device's
> + * runtime PM status will be set to active.
> + */
This last part is normally true for all devices. If you don't want it
to hold when want_interaction isn't set, you should add a good
explanation to sections 6 and 7 in Documentation/power/runtime.txt.
> +void pm_runtime_want_interaction(struct device *dev)
> +{
> + spin_lock_irq(&dev->power.lock);
> + dev->power.want_interaction = 1;
> + spin_unlock_irq(&dev->power.lock);
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_want_interaction);
> +
> +/**
> * pm_runtime_no_callbacks - Ignore run-time PM callbacks for a device.
> * @dev: Device to handle.
> *
Don't forget that you also need to describe these things in
Documentation/power/runtime.txt.
Alan Stern
^ permalink raw reply
* [PATCH 2/2] Defines DA850/AM18xx/OMAPL1-38 SOC resources used by PRUSS UIO driver
From: Pratheesh Gangadhar @ 2011-02-18 15:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1298041530-26855-2-git-send-email-pratheesh@ti.com>
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
^ permalink raw reply related
* [PATCH 1/2] PRUSS UIO driver support
From: Pratheesh Gangadhar @ 2011-02-18 15:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1298041530-26855-1-git-send-email-pratheesh@ti.com>
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
^ permalink raw reply related
* [PATCH 0/2] Add PRUSS UIO driver support
From: Pratheesh Gangadhar @ 2011-02-18 15:05 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply
* [PATCH 2/3] OMAP2+: hwmod: Fix what _init_clock returns
From: Cousson, Benoit @ 2011-02-18 14:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1297858285-7056-3-git-send-email-rnayak@ti.com>
On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> _init_clock always returns 0 and does
> not propogate the error (in case of failure)
> back to the caller, causing _init_clocks to
> fail silently.
>
> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> ---
> arch/arm/mach-omap2/omap_hwmod.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index cd9dcde..960461f 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -926,7 +926,7 @@ static int _init_clocks(struct omap_hwmod *oh, void *data)
> if (!ret)
> oh->_state = _HWMOD_STATE_CLKS_INITED;
>
> - return 0;
> + return ret;
> }
>
> /**
This is correct and that makes kerneldoc accurate : "Returns ... a
non-zero error on failure."
I'll queue it for 2.6.39.
Thanks,
Benoit
^ permalink raw reply
* [PATCH 0/3] OMAP2+ hwmod fixes
From: Cousson, Benoit @ 2011-02-18 14:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <6cc4b2788ff1e15c22987d5db91f0395@mail.gmail.com>
Hi Rajendra,
On 2/16/2011 2:43 PM, Nayak, Rajendra wrote:
> Hi Benoit,
>
>> -----Original Message-----
>> From: Cousson, Benoit [mailto:b-cousson at ti.com]
>> Sent: Wednesday, February 16, 2011 6:37 PM
>> To: Nayak, Rajendra
>> Cc: linux-omap at vger.kernel.org; paul at pwsan.com;
> linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
>>
>> Hi Rajendra,
>>
>> On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
>>> This series fixes some hwmod api return values
>>> and also adds some state checks.
>>> The hwmod iterator functions are made to
>>> continue and not break if one of the
>>> callback functions ends up with an error.
>>
>> By doing that, you change the behavior of this function.
>> I'm not sure I fully understand why.
>> Could you elaborate on the use case?
>
> Since these functions iterate over all hwmods
> calling a common callback function, there might
> be cases wherein the callback function for *some*
> hwmods might fail. For instance, if you run through
> all hwmods and try to clear the context registers
> for all of them, for some hwmods which might
> not have context registers the callback function
> might return a -EINVAL, however that should not
> stop you from attempting to clear the context
> registers for the rest of the hwmods which have
> them and abort the function midway, no?
> This is more hypothetical, however the real usecase
> that prompted me with this patch was when I
> had some wrong state check in _setup function,
> and the iterator would stop with the first failure
> and not even attempt to setup the rest of the
> hwmods.
Yeah, but by using that function you implicitly accept that an error
will break the loop, so the function you pass to the iterator should be
written for that. Meaning if you do not want to exit on error you should
not report an error.
>> To avoid that behavior in the past, I was just returning
>> 0 in case of failure to avoid stopping the iteration.
>> It looks like you do not want to stop the iteration but still
>> retrieve the error.
>> I do not see in this series what you plan to do with the
>> error at the end of the iteration.
>
> Most users of these iterators would just use the non-zero
> return value to throw an error/warning out stating there
> were failures running through all the callback functions.
> That does not change with this patch, and they can still
> do it.
Except that now, the iterator is not able anymore to stop on error if
needed potentially.
My point is that you are changing the behavior of this function without
maintaining the legacy.
So maybe creating a new iterator is a better approach.
Even is this feature is not used today I have some secret plan for this
behavior in the near future :-)
But my initial comment is still valid, if you do not care about the
final error status after the iteration, you'd better not return any
error at the beginning.
This was the behavior or the _setup until now.
Regards,
Benoit
^ permalink raw reply
* [PATCH v2 13/13] tty: pruss SUART driver
From: Alan Cox @ 2011-02-18 14:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <90BDE50764124CA8AAD560BB1E92C062@subhasishg>
On Fri, 18 Feb 2011 19:17:38 +0530
"Subhasish Ghosh" <subhasish@mistralsolutions.com> wrote:
> Hello,
>
> Regarding the semaphore to mutex migration.
> We are using down_trylock in interrupt context,
> mutex_trylock cannot be used in interrupt context, so we cannot use mutex in
> our driver.
Then you probably need to rework your locking. Best bet might be to fix
all the other stuff and report the driver, and people can think about the
locking problem.
Alan
^ permalink raw reply
* [PATCH V3 3/4] ARM: Xilinx: base header files and assembly macros
From: John Linn @ 2011-02-18 14:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110218090625.GA3203@pulham.picochip.com>
> -----Original Message-----
> From: John Linn
> Sent: Friday, February 18, 2011 7:00 AM
> To: 'Jamie Iles'
> Cc: linux-arm-kernel at lists.infradead.org;
linux-kernel at vger.kernel.org; linux at arm.linux.org.uk;
> catalin.marinas at arm.com; glikely at secretlab.ca; arnd at arndb.de
> Subject: RE: [PATCH V3 3/4] ARM: Xilinx: base header files and
assembly macros
>
> > -----Original Message-----
> > From: Jamie Iles [mailto:jamie at jamieiles.com]
> > Sent: Friday, February 18, 2011 2:06 AM
> > To: John Linn
> > Cc: Jamie Iles; linux-arm-kernel at lists.infradead.org;
linux-kernel at vger.kernel.org;
> > linux at arm.linux.org.uk; catalin.marinas at arm.com;
glikely at secretlab.ca; arnd at arndb.de
> > Subject: Re: [PATCH V3 3/4] ARM: Xilinx: base header files and
assembly macros
> >
> > On Thu, Feb 17, 2011 at 06:19:09PM -0700, John Linn wrote:
> > > > > +#define PHYS_OFFSET 0x0
> > > >
> > > > This should be PLAT_PHYS_OFFSET and could do with being
surrounded
> > > with
> > > > UL() e.g.
> > > >
> > > > #define PLAT_PHYS_OFFSET UL(0x00000000)
> > >
> > > This doesn't build with 2.6.38-rc5, am I missing something?
> > >
> > > PHYS_OFFSET is still needed from what I can tell, but maybe I
overlooked
> > > something.
> >
> > The change is queued up in Russell's devel branch for the dynamic
p2v
> > and will be in 2.6.39. For a patch series like this it's probably
worth
> > developing off of linux-next[1]. This contains all of the trees
that
> > will be mereged into the next kernel so carries the bigger changes
like
> > dynamic p2v.
>
> My hope was to get into 2.6.38 and maybe that was not realistic. If
it's too late
> for 2.6.38 then I'll revise the patch for this branch.
I guess it was too early and I wasn't thinking clearly there with
2.6.38.
You were right on 2.6.39. Sounds like I should switch to this other
branch
to develop against.
Thanks,
John
>
> Any thoughts Russell based on where we are?
>
> Thanks for the insight.
> John
>
> >
> > Jamie
> >
> > 1. git://git.kernel.org/pub/scm/linux/kernel/git/sfr/linux-next.git
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply
* [patch-v2.6.39 6/7] OMAP4430: hwmod data: Adding USBOTG
From: Cousson, Benoit @ 2011-02-18 14:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110217181849.GV20795@atomide.com>
Hi Tony
On 2/17/2011 7:18 PM, Tony Lindgren wrote:
> * Cousson, Benoit<b-cousson@ti.com> [110217 09:45]:
>>
>> Done, boot tested, and that does remove the warning about the
>> missing opt clock you had with the previous data.
>>
>> git://gitorious.org/omap-pm/linux.git for_2.6.39/omap4_hwmod_data
>>
>> Just let me know if it does break anything else.
>
> Pulled into omap-for-linus. Also now merged are the devel-hwspinlock
> and devel-mcspi branches.
McSPI OMAP4 hwmod data are missing a couple of flags that break the boot for the moment.
Govindraj should rebase on the omap4_hwmod_data branch and add the missing rev and dev_attr for the McSPI.
Unfortunately, even with that patch omap-for-linus boot hangs after the following trace:
[ 0.875091] omap_device: omap2_mcspi.1: new worst case activate latency 0: 30517
[ 0.886657] usbcore: registered new interface driver usbfs
[ 0.892822] usbcore: registered new interface driver hub
[ 0.898681] usbcore: registered new device driver usb
[ 0.904815] omap_i2c omap_i2c.1: bus 1 rev4.0 at 400 kHz
Regards,
Benoit
^ permalink raw reply
* [PATCH V3 3/4] ARM: Xilinx: base header files and assembly macros
From: John Linn @ 2011-02-18 13:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110218090625.GA3203@pulham.picochip.com>
> -----Original Message-----
> From: Jamie Iles [mailto:jamie at jamieiles.com]
> Sent: Friday, February 18, 2011 2:06 AM
> To: John Linn
> Cc: Jamie Iles; linux-arm-kernel at lists.infradead.org;
linux-kernel at vger.kernel.org;
> linux at arm.linux.org.uk; catalin.marinas at arm.com; glikely at secretlab.ca;
arnd at arndb.de
> Subject: Re: [PATCH V3 3/4] ARM: Xilinx: base header files and
assembly macros
>
> On Thu, Feb 17, 2011 at 06:19:09PM -0700, John Linn wrote:
> > > > +#define PHYS_OFFSET 0x0
> > >
> > > This should be PLAT_PHYS_OFFSET and could do with being surrounded
> > with
> > > UL() e.g.
> > >
> > > #define PLAT_PHYS_OFFSET UL(0x00000000)
> >
> > This doesn't build with 2.6.38-rc5, am I missing something?
> >
> > PHYS_OFFSET is still needed from what I can tell, but maybe I
overlooked
> > something.
>
> The change is queued up in Russell's devel branch for the dynamic p2v
> and will be in 2.6.39. For a patch series like this it's probably
worth
> developing off of linux-next[1]. This contains all of the trees that
> will be mereged into the next kernel so carries the bigger changes
like
> dynamic p2v.
My hope was to get into 2.6.38 and maybe that was not realistic. If
it's too late
for 2.6.38 then I'll revise the patch for this branch.
Any thoughts Russell based on where we are?
Thanks for the insight.
John
>
> Jamie
>
> 1. git://git.kernel.org/pub/scm/linux/kernel/git/sfr/linux-next.git
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply
* [RFC/PATCH] OMAP: Fix section mismatch
From: Uwe Kleine-König @ 2011-02-18 13:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1298034953-21016-1-git-send-email-weber@corscience.de>
Hello,
[added cpufreq at vger.kernel.org and Dave to Cc:]
On Fri, Feb 18, 2011 at 02:15:53PM +0100, Thomas Weber wrote:
> When compiling linux-omap with 'make CONFIG_DEBUG_SECTION_MISMATCH=y'
> the following output occurs:
>
> WARNING: arch/arm/plat-omap/built-in.o(.data+0x6e8): Section mismatch in
> reference from the variable omap_driver to the function
> .init.text:omap_cpu_init()
> The variable omap_driver references
> the function __init omap_cpu_init()
> If the reference is valid then annotate the
> variable with __init* or __refdata (see linux/init.h) or name the
> variable:
> *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console,
>
> This patch fixes this by adding __refdata to the omap_driver variable.
>
> Signed-off-by: Thomas Weber <weber@corscience.de>
> ---
> arch/arm/plat-omap/cpu-omap.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/plat-omap/cpu-omap.c b/arch/arm/plat-omap/cpu-omap.c
> index 11c54ec..17fdd5f 100644
> --- a/arch/arm/plat-omap/cpu-omap.c
> +++ b/arch/arm/plat-omap/cpu-omap.c
> @@ -144,7 +144,7 @@ static struct freq_attr *omap_cpufreq_attr[] = {
> NULL,
> };
>
> -static struct cpufreq_driver omap_driver = {
> +static struct cpufreq_driver omap_driver __refdata = {
> .flags = CPUFREQ_STICKY,
> .verify = omap_verify_speed,
> .target = omap_target,
This is correct for a single case. But actually the better fix would be
to remove the .init member of struct cpufreq_driver and change the
prototype of cpufreq_register_driver from
int (*)(struct cpufreq_driver *)
to
int (*)(struct cpufreq_driver *, int (*)(struct cpufreq_policy *))
and pass the init function directly to it.
This way you don't need __refdata (which should be an exception) and you
don't waste memory for a pointer that is never used after
cpufreq_register_driver returns.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH v2 13/13] tty: pruss SUART driver
From: Subhasish Ghosh @ 2011-02-18 13:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110211162814.6ff274f1@lxorguk.ukuu.org.uk>
Hello,
Regarding the semaphore to mutex migration.
We are using down_trylock in interrupt context,
mutex_trylock cannot be used in interrupt context, so we cannot use mutex in
our driver.
--------------------------------------------------
From: "Alan Cox" <alan@lxorguk.ukuu.org.uk>
Sent: Friday, February 11, 2011 9:58 PM
To: "Subhasish Ghosh" <subhasish@mistralsolutions.com>
Cc: <davinci-linux-open-source@linux.davincidsp.com>;
<linux-arm-kernel@lists.infradead.org>; <m-watkins@ti.com>;
<nsekhar@ti.com>; <sachi@mistralsolutions.com>; "Greg Kroah-Hartman"
<gregkh@suse.de>; "open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 13/13] tty: pruss SUART driver
> Don't see why it needs its own sub-directory
>
>
>
>> +#ifdef __SUART_DEBUG
>> +#define __suart_debug(fmt, args...) \
>> + printk(KERN_DEBUG "suart_debug: " fmt, ## args)
>> +#else
>> +#define __suart_debug(fmt, args...)
>> +#endif
>> +
>> +#define __suart_err(fmt, args...) printk(KERN_ERR "suart_err: " fmt, ##
>> args)
>
> Use dev_dbg/dev_err/pr_debug/pr_err
>
>
>> +static void omapl_pru_tx_chars(struct omapl_pru_suart *soft_uart, u32
>> uart_no)
>> +{
>> + struct circ_buf *xmit = &soft_uart->port[uart_no].state->xmit;
>> + struct device *dev = soft_uart->dev;
>> + s32 count = 0;
>> +
>> + if (!(suart_get_duplex(soft_uart, uart_no) & ePRU_SUART_HALF_TX))
>> + return;
>> +
>> + if (down_trylock(&soft_uart->port_sem[uart_no]))
>> + return;
>
> Please use a mutex not a semaphore. We are trying to get almost all the
> kernel using mutexes as it is needed for hard real time.
>
>
>> + pru_softuart_read_data(dev, &soft_uart->suart_hdl[uart_no],
>> + suart_data, data_len + 1,
>> + &data_len_read);
>> +
>> + for (i = 0; i <= data_len_read; i++) {
>> + soft_uart->port[uart_no].icount.rx++;
>> + /* check for sys rq */
>> + if (uart_handle_sysrq_char
>> + (&soft_uart->port[uart_no], suart_data))
>> + continue;
>> + }
>> + /* update the tty data structure */
>> + tty_insert_flip_string(tty, suart_data, data_len_read);
>
> You can avoid a copy here by using tty_prepare_flip_string()
>
> Also please use the tty_port_tty_get/tty_kref_put interfaces so the tty
> refcounting is right
>
>
>> +static void pruss_suart_set_termios(struct uart_port *port,
>> + struct ktermios *termios,
>> + struct ktermios *old)
>> +{
>> + struct omapl_pru_suart *soft_uart =
>> + container_of(port, struct omapl_pru_suart, port[port->line]);
>> + struct device *dev = soft_uart->dev;
>> + u8 cval = 0;
>> + unsigned long flags = 0;
>> + u32 baud = 0;
>> + u32 old_csize = old ? old->c_cflag & CSIZE : CS8;
>> +
>> +/*
>> + * Do not allow unsupported configurations to be set
>> + */
>> + if (1) {
>> + termios->c_cflag &= ~(HUPCL | CRTSCTS | CMSPAR | CSTOPB
>> + | PARENB | PARODD | CMSPAR);
>> + termios->c_cflag |= CLOCAL;
>
> No. CLOCAL and HUPCL are user side flags. Apps expect to able to set them
> even if in fact they have no effect, so leave those two alone. The rest
> is fine.
>
>
>> +/*
>> + * Characteres to ignore
>
> Typo
>
>
>
>> diff --git a/drivers/tty/serial/da8xx_pruss/pruss_suart_api.c
>> b/drivers/tty/serial/da8xx_pruss/pruss_suart_api.c
>> new file mode 100644
>> index 0000000..d809dd3
>> --- /dev/null
>> +++ b/drivers/tty/serial/da8xx_pruss/pruss_suart_api.c
>> @@ -0,0 +1,2350 @@
>> +/*
>> + * Copyright (C) 2010 Texas Instruments Incorporated
>> + * Author: Jitendra Kumar <jitendra@mistralsolutions.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/types.h>
>> +#include <linux/mfd/pruss/da8xx_pru.h>
>> +#include <linux/io.h>
>> +#include <mach/hardware.h>
>> +#include "pruss_suart_api.h"
>> +#include "pruss_suart_regs.h"
>> +#include "pruss_suart_board.h"
>> +#include "pruss_suart_utils.h"
>> +#include "pruss_suart_err.h"
>> +
>> +static u8 g_uart_statu_table[8];
> Can you lose the g_, its a windows naming convention we dont use
>
>
>> +s16 pru_softuart_open(suart_handle h_suart)
>> +{
>
> If you just used normal integers you could surely make this routine
> trivial and remove all the duplication in the switches
>
>> + s16 status = PRU_SUART_SUCCESS;
>
> And please stick to Linux error codes
>
>
>> +/* suart instance close routine */
>> +s16 pru_softuart_close(suart_handle h_uart)
>> +{
>> + s16 status = SUART_SUCCESS;
>> +
>> + if (h_uart == NULL) {
>> + return PRU_SUART_ERR_HANDLE_INVALID;
>
> Which is never checked. Far better to use WARN_ON and the like for such
> cases - or if like this one they don't appear to be possible to simply
> delete them
>
>> + if ((tx_clk_divisor > 385) || (tx_clk_divisor == 0))
>> + return SUART_INVALID_CLKDIVISOR;
>> + if ((rx_clk_divisor > 385) || (rx_clk_divisor == 0))
>> + return SUART_INVALID_CLKDIVISOR;
>
> [minor] Lots of excess brackets
>
>
>> + u32offset = (u32) (PRUSS_INTC_CHANMAP9 & 0xFFFF);
>> + u32value = PRU_INTC_CHAN_34_SYSEVT_36_39;
>> + s16retval = pruss_writel(dev, u32offset, (u32 *)&u32value, 1);
>> + if (-1 == s16retval)
>> + return -1;
>
> If you fixed the API here you'd be able to just write
>
> if (pruss_writel(dev, PRUSS_INTC_CHANMAP9 & 0xFFFF,
> PRU_INTC_BLAH)
>
> or similar which would clean up a lot of messy code and shrink it
> dramatically. Given you have lots of series of writes some kind of
>
> pruss_writel_multi(dev, array, len)
>
> that took an array of addr/value pairs might also clean up a ton of code
> and turn it into trivial tables
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox