Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Mar 06, 2013 at 02:05:52PM +0100, Thomas Gleixner wrote: > On Wed, 6 Mar 2013, Simon Horman wrote: > > > On Wed, Mar 06, 2013 at 11:01:14AM +0100, Thomas Gleixner wrote: > > > On Wed, 6 Mar 2013, Simon Horman wrote: > > > > On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: > > > > > The SoCs using this driver are currently mainly used > > > > > together with regular platform devices so this driver > > > > > allows configuration via platform data to support things > > > > > like static interrupt base address. DT support will > > > > > be added incrementally in the not so distant future. > > > > > > > > Hi Thomas, > > > > > > > > I'm wondering how you would like to handle merging this driver. > > > > I can think of three options but I'm sure there are others. > > > > > > > > * You can take the patches (there is a follow-up series) yourself. > > > > * I can prepare a pull request for you > > > > * I can prepare a pull request for arm-soc with the shmobile patches > > > > that > > > > enable the driver on the r8a7779 and sh73a0. > > > > > > > > The last option is possibly the easiest. > > > > > > Correct. > > > > > > > But in that case I'd appreciate an Ack from you on this patch. > > > > > > You want to pick the V2 series, which already has my blessing: > > > > > > https://lkml.org/lkml/2013/2/26/305 > > > > > > For merging it through arm-soc you have my ack now :) > > > > Thanks, I have that. > > > > It seems that V2 adds onto this patch rather than replaces it. > > So could I also get an Ack for this patch too? > > Ah,. right. V2 is an incremental fix for V1. Yes, please add my Ack. Thanks. I have added this into a new intc-external-irq branch of the renesas tree on kernel.org and thus queued it up for v3.10. I have also added the following patches to that branch: irqchip: irqc: Add DT support irqchip: intc-irqpin: Initial DT support ARM: shmobile: Make r8a7779 INTC irqpin platform data static ARM: shmobile: Make sh73a0 INTC irqpin platform data static irqchip: Renesas IRQC driver irqchip: intc-irqpin: GPL header for platform data irqchip: intc-irqpin: Make use of devm functions irqchip: intc-irqpin: Add force comments irqchip: intc-irqpin: Cache mapped IRQ irqchip: intc-irqpin: Whitespace fixes ARM: shmobile: INTC External IRQ pin driver on r8a7779 ARM: shmobile: INTC External IRQ pin driver on sh73a0 ARM: shmobile: irq_pin() for static IRQ pin assignment The intc-external-irq branch is merged into the next branch and I expect it to appear in linux-next in the not to distant future. I have removed the topic/intc-external-irq branch from the reneas tree, the branch where these patches were being staged. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Mar 06, 2013 at 02:05:52PM +0100, Thomas Gleixner wrote: On Wed, 6 Mar 2013, Simon Horman wrote: On Wed, Mar 06, 2013 at 11:01:14AM +0100, Thomas Gleixner wrote: On Wed, 6 Mar 2013, Simon Horman wrote: On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: The SoCs using this driver are currently mainly used together with regular platform devices so this driver allows configuration via platform data to support things like static interrupt base address. DT support will be added incrementally in the not so distant future. Hi Thomas, I'm wondering how you would like to handle merging this driver. I can think of three options but I'm sure there are others. * You can take the patches (there is a follow-up series) yourself. * I can prepare a pull request for you * I can prepare a pull request for arm-soc with the shmobile patches that enable the driver on the r8a7779 and sh73a0. The last option is possibly the easiest. Correct. But in that case I'd appreciate an Ack from you on this patch. You want to pick the V2 series, which already has my blessing: https://lkml.org/lkml/2013/2/26/305 For merging it through arm-soc you have my ack now :) Thanks, I have that. It seems that V2 adds onto this patch rather than replaces it. So could I also get an Ack for this patch too? Ah,. right. V2 is an incremental fix for V1. Yes, please add my Ack. Thanks. I have added this into a new intc-external-irq branch of the renesas tree on kernel.org and thus queued it up for v3.10. I have also added the following patches to that branch: irqchip: irqc: Add DT support irqchip: intc-irqpin: Initial DT support ARM: shmobile: Make r8a7779 INTC irqpin platform data static ARM: shmobile: Make sh73a0 INTC irqpin platform data static irqchip: Renesas IRQC driver irqchip: intc-irqpin: GPL header for platform data irqchip: intc-irqpin: Make use of devm functions irqchip: intc-irqpin: Add force comments irqchip: intc-irqpin: Cache mapped IRQ irqchip: intc-irqpin: Whitespace fixes ARM: shmobile: INTC External IRQ pin driver on r8a7779 ARM: shmobile: INTC External IRQ pin driver on sh73a0 ARM: shmobile: irq_pin() for static IRQ pin assignment The intc-external-irq branch is merged into the next branch and I expect it to appear in linux-next in the not to distant future. I have removed the topic/intc-external-irq branch from the reneas tree, the branch where these patches were being staged. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, 6 Mar 2013, Simon Horman wrote: > On Wed, Mar 06, 2013 at 11:01:14AM +0100, Thomas Gleixner wrote: > > On Wed, 6 Mar 2013, Simon Horman wrote: > > > On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: > > > > The SoCs using this driver are currently mainly used > > > > together with regular platform devices so this driver > > > > allows configuration via platform data to support things > > > > like static interrupt base address. DT support will > > > > be added incrementally in the not so distant future. > > > > > > Hi Thomas, > > > > > > I'm wondering how you would like to handle merging this driver. > > > I can think of three options but I'm sure there are others. > > > > > > * You can take the patches (there is a follow-up series) yourself. > > > * I can prepare a pull request for you > > > * I can prepare a pull request for arm-soc with the shmobile patches that > > > enable the driver on the r8a7779 and sh73a0. > > > > > > The last option is possibly the easiest. > > > > Correct. > > > > > But in that case I'd appreciate an Ack from you on this patch. > > > > You want to pick the V2 series, which already has my blessing: > > > > https://lkml.org/lkml/2013/2/26/305 > > > > For merging it through arm-soc you have my ack now :) > > Thanks, I have that. > > It seems that V2 adds onto this patch rather than replaces it. > So could I also get an Ack for this patch too? Ah,. right. V2 is an incremental fix for V1. Yes, please add my Ack. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Mar 06, 2013 at 11:01:14AM +0100, Thomas Gleixner wrote: > On Wed, 6 Mar 2013, Simon Horman wrote: > > On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: > > > The SoCs using this driver are currently mainly used > > > together with regular platform devices so this driver > > > allows configuration via platform data to support things > > > like static interrupt base address. DT support will > > > be added incrementally in the not so distant future. > > > > Hi Thomas, > > > > I'm wondering how you would like to handle merging this driver. > > I can think of three options but I'm sure there are others. > > > > * You can take the patches (there is a follow-up series) yourself. > > * I can prepare a pull request for you > > * I can prepare a pull request for arm-soc with the shmobile patches that > > enable the driver on the r8a7779 and sh73a0. > > > > The last option is possibly the easiest. > > Correct. > > > But in that case I'd appreciate an Ack from you on this patch. > > You want to pick the V2 series, which already has my blessing: > > https://lkml.org/lkml/2013/2/26/305 > > For merging it through arm-soc you have my ack now :) Thanks, I have that. It seems that V2 adds onto this patch rather than replaces it. So could I also get an Ack for this patch too? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, 6 Mar 2013, Simon Horman wrote: > On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: > > The SoCs using this driver are currently mainly used > > together with regular platform devices so this driver > > allows configuration via platform data to support things > > like static interrupt base address. DT support will > > be added incrementally in the not so distant future. > > Hi Thomas, > > I'm wondering how you would like to handle merging this driver. > I can think of three options but I'm sure there are others. > > * You can take the patches (there is a follow-up series) yourself. > * I can prepare a pull request for you > * I can prepare a pull request for arm-soc with the shmobile patches that > enable the driver on the r8a7779 and sh73a0. > > The last option is possibly the easiest. Correct. > But in that case I'd appreciate an Ack from you on this patch. You want to pick the V2 series, which already has my blessing: https://lkml.org/lkml/2013/2/26/305 For merging it through arm-soc you have my ack now :) Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, 6 Mar 2013, Simon Horman wrote: On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: The SoCs using this driver are currently mainly used together with regular platform devices so this driver allows configuration via platform data to support things like static interrupt base address. DT support will be added incrementally in the not so distant future. Hi Thomas, I'm wondering how you would like to handle merging this driver. I can think of three options but I'm sure there are others. * You can take the patches (there is a follow-up series) yourself. * I can prepare a pull request for you * I can prepare a pull request for arm-soc with the shmobile patches that enable the driver on the r8a7779 and sh73a0. The last option is possibly the easiest. Correct. But in that case I'd appreciate an Ack from you on this patch. You want to pick the V2 series, which already has my blessing: https://lkml.org/lkml/2013/2/26/305 For merging it through arm-soc you have my ack now :) Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Mar 06, 2013 at 11:01:14AM +0100, Thomas Gleixner wrote: On Wed, 6 Mar 2013, Simon Horman wrote: On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: The SoCs using this driver are currently mainly used together with regular platform devices so this driver allows configuration via platform data to support things like static interrupt base address. DT support will be added incrementally in the not so distant future. Hi Thomas, I'm wondering how you would like to handle merging this driver. I can think of three options but I'm sure there are others. * You can take the patches (there is a follow-up series) yourself. * I can prepare a pull request for you * I can prepare a pull request for arm-soc with the shmobile patches that enable the driver on the r8a7779 and sh73a0. The last option is possibly the easiest. Correct. But in that case I'd appreciate an Ack from you on this patch. You want to pick the V2 series, which already has my blessing: https://lkml.org/lkml/2013/2/26/305 For merging it through arm-soc you have my ack now :) Thanks, I have that. It seems that V2 adds onto this patch rather than replaces it. So could I also get an Ack for this patch too? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, 6 Mar 2013, Simon Horman wrote: On Wed, Mar 06, 2013 at 11:01:14AM +0100, Thomas Gleixner wrote: On Wed, 6 Mar 2013, Simon Horman wrote: On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: The SoCs using this driver are currently mainly used together with regular platform devices so this driver allows configuration via platform data to support things like static interrupt base address. DT support will be added incrementally in the not so distant future. Hi Thomas, I'm wondering how you would like to handle merging this driver. I can think of three options but I'm sure there are others. * You can take the patches (there is a follow-up series) yourself. * I can prepare a pull request for you * I can prepare a pull request for arm-soc with the shmobile patches that enable the driver on the r8a7779 and sh73a0. The last option is possibly the easiest. Correct. But in that case I'd appreciate an Ack from you on this patch. You want to pick the V2 series, which already has my blessing: https://lkml.org/lkml/2013/2/26/305 For merging it through arm-soc you have my ack now :) Thanks, I have that. It seems that V2 adds onto this patch rather than replaces it. So could I also get an Ack for this patch too? Ah,. right. V2 is an incremental fix for V1. Yes, please add my Ack. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Feb 27, 2013 at 7:28 PM, Paul Mundt wrote: > On Wed, Feb 27, 2013 at 06:52:51PM +0900, Magnus Damm wrote: >> As you know, the INTC code that you are referring to is a full >> interrupt controller designed to work directly with CPU cores like SH >> and ARM. Newer ARM cores like Cortex-A9 all include the GIC both for >> IPI purpose in case of SMP and they also implement SPI functionality >> for I/O device interrupts. So the need for vendor-specific interrupt >> controller IP is almost down to nothing these days. >> > Yes, I'm aware of that. Good! >> With that in mind I do really doubt that we end up reimplementing >> sh_intc. The upcoming designs that I am aware of do not change their >> external IRQ pin hardware to force us to update this driver. What >> happens after that I'm afraid I can't say. =) >> > While I don't expect you would end up with a full reimplmentation, my > concern is more that it would just be reproducing stuff that's already in > place. IOW, perhaps it's less work to put sh_intc on a diet, and tie the > core functionality that you need for external IRQ pins in to an irqchip > there -- with the core of the old code adapted in to some sort of common > base library, rather than coming up with a new lightweight irqchip driver > and having to incrementally pile stuff on top of it later. I'm afraid that I can't really see how we would want to pile that much stuff on top of this driver in the future. Maybe we need to adjust it slightly, but that should be fairly easy. >From my point of view it all boils down to this for our ARM SoCs: 1) The driver is only suitable for older GIC-based designs coming from the Renesas side. 2) GIC-based SoCs like EMEV2 coming from ex-NEC is using different IP and different driver. 3) Older SoCs that do not make use of GIC are using the full blown INTC implementation. >> I mainly wrote this driver to support the r8a7779 SoC which can't be >> driven by the existing INTC code base. During development I decided to >> try to share the driver between multiple GIC-based SoCs like r8a7779 >> and sh73a0. The reason behind trying to share this driver between >> multiple SoCs is to remove SoC-specific hacks that can't be dealt with >> by the existing INTC code. >> > Ok, fair enough. > >> I don't really understand why you would want to use a generic table >> driven driver when you have the possibility of using a >> hardware-specific driver. > > For the same reason sh_intc was written in the first place, every CPU > subtype more or less had a similar set of interrupt controllers with > minor deviations. Those deviations were sufficient enough to make the > code pretty hairy in places, and what we have now is really more of a > subsystem than an irqchip driver. > > Having to reproduce 90% of the code when you're adding new CPUs at the > rate of 2 a month hardly makes an SoC-specific driver realistic, > especially as you just end up with identical features being broken in > subtly differnt ways on different SoCs. That being said, a lot of that is > legacy stuff and a result of no CPU team talking to any other CPU team > ever, and it seems like things have improved enough in that regard that > perhaps a hardware-specific driver won't be a complete throw-away > disaster like it was before. It's a risk either way, I just hope your > lightweight solution remains lightweight and consistent long enough that > we don't have multiple copies of slightly different drivers doing exactly > same thing spiralling out of control and turning in to a maintenance > nightmare. Yes, I agree about the history with zillions of different SoCs, and the INTC and PFC design choice. But regarding the external pins in case of GIC-based designs, this seems to be something we can reuse as a regular driver. > If sh_intc doesn't deal with the needs of the new GIC-backed SoCs then > that's of course something that has to be addressed regardless (whether > that be hacking up sh_intc or adding new hardware-specific irqchips). Ignoring things won't work, agreed. >> I suppose you are wondering why no one seems to be working on INTC DT >> support at this point. The truth is that we got very little feedback >> and development support with interrupts in general last autumn and on >> top of that our developers went their own way instead of following >> advice. >> > I assumed it was either being rewritten or had already been merged, so I > was simply surprised to hear otherwise. I will dig through the archives a > bit later and see about getting caught up. Sounds good. Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: > From: Magnus Damm > > This patch adds a driver for external IRQ pins connected > to the INTC block on recent SoCs from Renesas. > > The INTC hardware block usually contains a rather wide > range of features ranging from external IRQ pin handling > to legacy interrupt controller support. On older SoCs > the INTC is used as a general purpose interrupt controller > both for external IRQ pins and on-chip devices. > > On more recent ARM based SoCs with Cortex-A9 the main > interrupt controller is the GIC, but IRQ trigger setup > still need to happen in the INTC hardware block. > > This driver implements the glue code needed to configure > IRQ trigger and also handle mask/unmask and demux of > external IRQ pins hooked up from the INTC to the GIC. > > Tested on sh73a0 and r8a7779. The hardware varies quite > a bit with SoC model, for instance register width and > bitfield widths vary wildly. The driver requires one GIC > SPI per external IRQ pin to operate. Each driver instance > will handle up to 8 external IRQ pins. > > The SoCs using this driver are currently mainly used > together with regular platform devices so this driver > allows configuration via platform data to support things > like static interrupt base address. DT support will > be added incrementally in the not so distant future. Hi Thomas, I'm wondering how you would like to handle merging this driver. I can think of three options but I'm sure there are others. * You can take the patches (there is a follow-up series) yourself. * I can prepare a pull request for you * I can prepare a pull request for arm-soc with the shmobile patches that enable the driver on the r8a7779 and sh73a0. The last option is possibly the easiest. But in that case I'd appreciate an Ack from you on this patch. > Signed-off-by: Magnus Damm > --- > > drivers/irqchip/Kconfig |4 > drivers/irqchip/Makefile |1 > drivers/irqchip/irq-renesas-intc-irqpin.c | 464 > + > include/linux/platform_data/irq-renesas-intc-irqpin.h | 10 > 4 files changed, 479 insertions(+) > > --- 0001/drivers/irqchip/Kconfig > +++ work/drivers/irqchip/Kconfig 2013-02-18 18:28:22.0 +0900 > @@ -25,6 +25,10 @@ config ARM_VIC_NR > The maximum number of VICs available in the system, for > power management. > > +config RENESAS_INTC_IRQPIN > + bool > + select IRQ_DOMAIN > + > config VERSATILE_FPGA_IRQ > bool > select IRQ_DOMAIN > --- 0001/drivers/irqchip/Makefile > +++ work/drivers/irqchip/Makefile 2013-02-18 18:28:22.0 +0900 > @@ -5,4 +5,5 @@ obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi.o > obj-$(CONFIG_ARCH_SPEAR3XX) += spear-shirq.o > obj-$(CONFIG_ARM_GIC)+= irq-gic.o > obj-$(CONFIG_ARM_VIC)+= irq-vic.o > +obj-$(CONFIG_RENESAS_INTC_IRQPIN)+= irq-renesas-intc-irqpin.o > obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o > --- /dev/null > +++ work/drivers/irqchip/irq-renesas-intc-irqpin.c2013-02-18 > 21:06:32.0 +0900 > @@ -0,0 +1,464 @@ > +/* > + * Renesas INTC External IRQ Pin Driver > + * > + * Copyright (C) 2013 Magnus Damm > + * > + * 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; either version 2 of the License > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define INTC_IRQPIN_MAX 8 /* maximum 8 interrupts per driver instance */ > + > +#define INTC_IRQPIN_REG_SENSE 0 /* ICRn */ > +#define INTC_IRQPIN_REG_PRIO 1 /* INTPRInn */ > +#define INTC_IRQPIN_REG_SOURCE 2 /* INTREQnn */ > +#define INTC_IRQPIN_REG_MASK 3 /* INTMSKnn */ > +#define INTC_IRQPIN_REG_CLEAR 4 /* INTMSKCLRnn */ > +#define INTC_IRQPIN_REG_NR 5 > + > +/* INTC external IRQ PIN hardware register access: > + * > + * SENSE is read-write 32-bit with 2-bits or 4-bits per IRQ (*) > + * PRIO is read-write 32-bit with 4-bits per IRQ (**) > + * SOURCE is read-only 32-bit or 8-bit with 1-bit per IRQ (***) > + * MASK is write-only 32-bit or 8-bit with 1-bit per IRQ (***) > + * CLEAR is write-only 32-bit or 8-bit with 1-bit per IRQ (***) > + * > + * (*) May be accessed by more than one
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: From: Magnus Damm d...@opensource.se This patch adds a driver for external IRQ pins connected to the INTC block on recent SoCs from Renesas. The INTC hardware block usually contains a rather wide range of features ranging from external IRQ pin handling to legacy interrupt controller support. On older SoCs the INTC is used as a general purpose interrupt controller both for external IRQ pins and on-chip devices. On more recent ARM based SoCs with Cortex-A9 the main interrupt controller is the GIC, but IRQ trigger setup still need to happen in the INTC hardware block. This driver implements the glue code needed to configure IRQ trigger and also handle mask/unmask and demux of external IRQ pins hooked up from the INTC to the GIC. Tested on sh73a0 and r8a7779. The hardware varies quite a bit with SoC model, for instance register width and bitfield widths vary wildly. The driver requires one GIC SPI per external IRQ pin to operate. Each driver instance will handle up to 8 external IRQ pins. The SoCs using this driver are currently mainly used together with regular platform devices so this driver allows configuration via platform data to support things like static interrupt base address. DT support will be added incrementally in the not so distant future. Hi Thomas, I'm wondering how you would like to handle merging this driver. I can think of three options but I'm sure there are others. * You can take the patches (there is a follow-up series) yourself. * I can prepare a pull request for you * I can prepare a pull request for arm-soc with the shmobile patches that enable the driver on the r8a7779 and sh73a0. The last option is possibly the easiest. But in that case I'd appreciate an Ack from you on this patch. Signed-off-by: Magnus Damm d...@opensource.se --- drivers/irqchip/Kconfig |4 drivers/irqchip/Makefile |1 drivers/irqchip/irq-renesas-intc-irqpin.c | 464 + include/linux/platform_data/irq-renesas-intc-irqpin.h | 10 4 files changed, 479 insertions(+) --- 0001/drivers/irqchip/Kconfig +++ work/drivers/irqchip/Kconfig 2013-02-18 18:28:22.0 +0900 @@ -25,6 +25,10 @@ config ARM_VIC_NR The maximum number of VICs available in the system, for power management. +config RENESAS_INTC_IRQPIN + bool + select IRQ_DOMAIN + config VERSATILE_FPGA_IRQ bool select IRQ_DOMAIN --- 0001/drivers/irqchip/Makefile +++ work/drivers/irqchip/Makefile 2013-02-18 18:28:22.0 +0900 @@ -5,4 +5,5 @@ obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi.o obj-$(CONFIG_ARCH_SPEAR3XX) += spear-shirq.o obj-$(CONFIG_ARM_GIC)+= irq-gic.o obj-$(CONFIG_ARM_VIC)+= irq-vic.o +obj-$(CONFIG_RENESAS_INTC_IRQPIN)+= irq-renesas-intc-irqpin.o obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o --- /dev/null +++ work/drivers/irqchip/irq-renesas-intc-irqpin.c2013-02-18 21:06:32.0 +0900 @@ -0,0 +1,464 @@ +/* + * Renesas INTC External IRQ Pin Driver + * + * Copyright (C) 2013 Magnus Damm + * + * 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; either version 2 of the License + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#include linux/init.h +#include linux/platform_device.h +#include linux/spinlock.h +#include linux/interrupt.h +#include linux/ioport.h +#include linux/io.h +#include linux/irq.h +#include linux/irqdomain.h +#include linux/err.h +#include linux/slab.h +#include linux/module.h +#include linux/platform_data/irq-renesas-intc-irqpin.h + +#define INTC_IRQPIN_MAX 8 /* maximum 8 interrupts per driver instance */ + +#define INTC_IRQPIN_REG_SENSE 0 /* ICRn */ +#define INTC_IRQPIN_REG_PRIO 1 /* INTPRInn */ +#define INTC_IRQPIN_REG_SOURCE 2 /* INTREQnn */ +#define INTC_IRQPIN_REG_MASK 3 /* INTMSKnn */ +#define INTC_IRQPIN_REG_CLEAR 4 /* INTMSKCLRnn */ +#define INTC_IRQPIN_REG_NR 5 + +/* INTC external IRQ PIN hardware register access: + * + * SENSE is read-write 32-bit with 2-bits or 4-bits per IRQ (*) + * PRIO is read-write 32-bit with 4-bits per IRQ (**) + * SOURCE is read-only 32-bit or 8-bit with 1-bit per IRQ (***) + * MASK is write-only 32-bit or 8-bit with 1-bit per
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Feb 27, 2013 at 7:28 PM, Paul Mundt let...@linux-sh.org wrote: On Wed, Feb 27, 2013 at 06:52:51PM +0900, Magnus Damm wrote: As you know, the INTC code that you are referring to is a full interrupt controller designed to work directly with CPU cores like SH and ARM. Newer ARM cores like Cortex-A9 all include the GIC both for IPI purpose in case of SMP and they also implement SPI functionality for I/O device interrupts. So the need for vendor-specific interrupt controller IP is almost down to nothing these days. Yes, I'm aware of that. Good! With that in mind I do really doubt that we end up reimplementing sh_intc. The upcoming designs that I am aware of do not change their external IRQ pin hardware to force us to update this driver. What happens after that I'm afraid I can't say. =) While I don't expect you would end up with a full reimplmentation, my concern is more that it would just be reproducing stuff that's already in place. IOW, perhaps it's less work to put sh_intc on a diet, and tie the core functionality that you need for external IRQ pins in to an irqchip there -- with the core of the old code adapted in to some sort of common base library, rather than coming up with a new lightweight irqchip driver and having to incrementally pile stuff on top of it later. I'm afraid that I can't really see how we would want to pile that much stuff on top of this driver in the future. Maybe we need to adjust it slightly, but that should be fairly easy. From my point of view it all boils down to this for our ARM SoCs: 1) The driver is only suitable for older GIC-based designs coming from the Renesas side. 2) GIC-based SoCs like EMEV2 coming from ex-NEC is using different IP and different driver. 3) Older SoCs that do not make use of GIC are using the full blown INTC implementation. I mainly wrote this driver to support the r8a7779 SoC which can't be driven by the existing INTC code base. During development I decided to try to share the driver between multiple GIC-based SoCs like r8a7779 and sh73a0. The reason behind trying to share this driver between multiple SoCs is to remove SoC-specific hacks that can't be dealt with by the existing INTC code. Ok, fair enough. I don't really understand why you would want to use a generic table driven driver when you have the possibility of using a hardware-specific driver. For the same reason sh_intc was written in the first place, every CPU subtype more or less had a similar set of interrupt controllers with minor deviations. Those deviations were sufficient enough to make the code pretty hairy in places, and what we have now is really more of a subsystem than an irqchip driver. Having to reproduce 90% of the code when you're adding new CPUs at the rate of 2 a month hardly makes an SoC-specific driver realistic, especially as you just end up with identical features being broken in subtly differnt ways on different SoCs. That being said, a lot of that is legacy stuff and a result of no CPU team talking to any other CPU team ever, and it seems like things have improved enough in that regard that perhaps a hardware-specific driver won't be a complete throw-away disaster like it was before. It's a risk either way, I just hope your lightweight solution remains lightweight and consistent long enough that we don't have multiple copies of slightly different drivers doing exactly same thing spiralling out of control and turning in to a maintenance nightmare. Yes, I agree about the history with zillions of different SoCs, and the INTC and PFC design choice. But regarding the external pins in case of GIC-based designs, this seems to be something we can reuse as a regular driver. If sh_intc doesn't deal with the needs of the new GIC-backed SoCs then that's of course something that has to be addressed regardless (whether that be hacking up sh_intc or adding new hardware-specific irqchips). Ignoring things won't work, agreed. I suppose you are wondering why no one seems to be working on INTC DT support at this point. The truth is that we got very little feedback and development support with interrupts in general last autumn and on top of that our developers went their own way instead of following advice. I assumed it was either being rewritten or had already been merged, so I was simply surprised to hear otherwise. I will dig through the archives a bit later and see about getting caught up. Sounds good. Thanks, / magnus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Feb 27, 2013 at 06:52:51PM +0900, Magnus Damm wrote: > As you know, the INTC code that you are referring to is a full > interrupt controller designed to work directly with CPU cores like SH > and ARM. Newer ARM cores like Cortex-A9 all include the GIC both for > IPI purpose in case of SMP and they also implement SPI functionality > for I/O device interrupts. So the need for vendor-specific interrupt > controller IP is almost down to nothing these days. > Yes, I'm aware of that. > With that in mind I do really doubt that we end up reimplementing > sh_intc. The upcoming designs that I am aware of do not change their > external IRQ pin hardware to force us to update this driver. What > happens after that I'm afraid I can't say. =) > While I don't expect you would end up with a full reimplmentation, my concern is more that it would just be reproducing stuff that's already in place. IOW, perhaps it's less work to put sh_intc on a diet, and tie the core functionality that you need for external IRQ pins in to an irqchip there -- with the core of the old code adapted in to some sort of common base library, rather than coming up with a new lightweight irqchip driver and having to incrementally pile stuff on top of it later. > I mainly wrote this driver to support the r8a7779 SoC which can't be > driven by the existing INTC code base. During development I decided to > try to share the driver between multiple GIC-based SoCs like r8a7779 > and sh73a0. The reason behind trying to share this driver between > multiple SoCs is to remove SoC-specific hacks that can't be dealt with > by the existing INTC code. > Ok, fair enough. > I don't really understand why you would want to use a generic table > driven driver when you have the possibility of using a > hardware-specific driver. For the same reason sh_intc was written in the first place, every CPU subtype more or less had a similar set of interrupt controllers with minor deviations. Those deviations were sufficient enough to make the code pretty hairy in places, and what we have now is really more of a subsystem than an irqchip driver. Having to reproduce 90% of the code when you're adding new CPUs at the rate of 2 a month hardly makes an SoC-specific driver realistic, especially as you just end up with identical features being broken in subtly differnt ways on different SoCs. That being said, a lot of that is legacy stuff and a result of no CPU team talking to any other CPU team ever, and it seems like things have improved enough in that regard that perhaps a hardware-specific driver won't be a complete throw-away disaster like it was before. It's a risk either way, I just hope your lightweight solution remains lightweight and consistent long enough that we don't have multiple copies of slightly different drivers doing exactly same thing spiralling out of control and turning in to a maintenance nightmare. If sh_intc doesn't deal with the needs of the new GIC-backed SoCs then that's of course something that has to be addressed regardless (whether that be hacking up sh_intc or adding new hardware-specific irqchips). > I suppose you are wondering why no one seems to be working on INTC DT > support at this point. The truth is that we got very little feedback > and development support with interrupts in general last autumn and on > top of that our developers went their own way instead of following > advice. > I assumed it was either being rewritten or had already been merged, so I was simply surprised to hear otherwise. I will dig through the archives a bit later and see about getting caught up. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Feb 27, 2013 at 5:52 PM, Paul Mundt wrote: > On Wed, Feb 27, 2013 at 05:35:51PM +0900, Magnus Damm wrote: >> On Wed, Feb 27, 2013 at 5:23 PM, Paul Mundt wrote: >> > So how exactly does this interact with the existing sh_intc code? Or is >> > there some reason why you have opted to bypass it in order to implement a >> > simplified reduced-functionality version of INTC support focused only on >> > external pins? If both are used together this is going to be a nightmare >> > for locking, and it's also non-obvious how the IRQ domains on both sides >> > will interact. >> > >> > This needs a lot more explanation. >> >> Recent GIC-based SoCs do not make use of INTC for any on-chip I/O >> devices. This driver is meant to be used as a layer between the actual >> IRQ pin and the GIC. Anything else needs the full driver. The existing >> non-DT INTC driver can happily coexist with this driver like it does >> in the case of sh73a0 here: >> >> [PATCH 02/03] ARM: shmobile: INTC External IRQ pin driver on sh73a0 >> > Ok, thanks for clarifying. > > I suppose the main concern is how quickly this will simply turn in to a > deviated partial implementation of the full driver as newer SoCs begin > deviating from your simplified case, and we basically end up > reimplementing sh_intc anyways. As you know, the INTC code that you are referring to is a full interrupt controller designed to work directly with CPU cores like SH and ARM. Newer ARM cores like Cortex-A9 all include the GIC both for IPI purpose in case of SMP and they also implement SPI functionality for I/O device interrupts. So the need for vendor-specific interrupt controller IP is almost down to nothing these days. With that in mind I do really doubt that we end up reimplementing sh_intc. The upcoming designs that I am aware of do not change their external IRQ pin hardware to force us to update this driver. What happens after that I'm afraid I can't say. =) >> The driver is not meant to be used with INTC-only based systems like >> sh7372 and the SH architecture. I would be very happy if someone could >> get their shit together and fix up DT support for the common INTC >> code. This has not happened yet though. So if you know anyone with >> time to spare then feel free to suggest them to work together with >> Iwamatsu-san to get the DT version of the code reviewed together with >> Linaro. >> > I haven't heard or seen anything new on that in some time, so I assumed > the work had stalled. I'm not sure why there wasn't more effort put in to > DT support for the INTC code rather than simply coming up with a > temporary bypass shim, and I'm not sure why you think this work is > blocked by anyone (unless you're just referring to a general lack of > resources). I mainly wrote this driver to support the r8a7779 SoC which can't be driven by the existing INTC code base. During development I decided to try to share the driver between multiple GIC-based SoCs like r8a7779 and sh73a0. The reason behind trying to share this driver between multiple SoCs is to remove SoC-specific hacks that can't be dealt with by the existing INTC code. > In any event, I'm not sure what the best option for the interim is. I > suppose we can merge the irqchips until the INTC stuff catches up, but > then we are probaby going to run in to a situation where they either have > to co-exist, or the irqchips are removed and the sh_intc code has to > carry a compat shim to deal with those DT bindings. Neither of those > options seem particularly appealing to me. I don't really understand why you would want to use a generic table driven driver when you have the possibility of using a hardware-specific driver. I suppose you are wondering why no one seems to be working on INTC DT support at this point. The truth is that we got very little feedback and development support with interrupts in general last autumn and on top of that our developers went their own way instead of following advice. Anyway, I do understand your worry about what happens if the hardware starts to deviate, but judging by the future devices that I've seen we should be ok for a while. Beyond that no one can tell. Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Feb 27, 2013 at 05:35:51PM +0900, Magnus Damm wrote: > On Wed, Feb 27, 2013 at 5:23 PM, Paul Mundt wrote: > > So how exactly does this interact with the existing sh_intc code? Or is > > there some reason why you have opted to bypass it in order to implement a > > simplified reduced-functionality version of INTC support focused only on > > external pins? If both are used together this is going to be a nightmare > > for locking, and it's also non-obvious how the IRQ domains on both sides > > will interact. > > > > This needs a lot more explanation. > > Recent GIC-based SoCs do not make use of INTC for any on-chip I/O > devices. This driver is meant to be used as a layer between the actual > IRQ pin and the GIC. Anything else needs the full driver. The existing > non-DT INTC driver can happily coexist with this driver like it does > in the case of sh73a0 here: > > [PATCH 02/03] ARM: shmobile: INTC External IRQ pin driver on sh73a0 > Ok, thanks for clarifying. I suppose the main concern is how quickly this will simply turn in to a deviated partial implementation of the full driver as newer SoCs begin deviating from your simplified case, and we basically end up reimplementing sh_intc anyways. > The driver is not meant to be used with INTC-only based systems like > sh7372 and the SH architecture. I would be very happy if someone could > get their shit together and fix up DT support for the common INTC > code. This has not happened yet though. So if you know anyone with > time to spare then feel free to suggest them to work together with > Iwamatsu-san to get the DT version of the code reviewed together with > Linaro. > I haven't heard or seen anything new on that in some time, so I assumed the work had stalled. I'm not sure why there wasn't more effort put in to DT support for the INTC code rather than simply coming up with a temporary bypass shim, and I'm not sure why you think this work is blocked by anyone (unless you're just referring to a general lack of resources). In any event, I'm not sure what the best option for the interim is. I suppose we can merge the irqchips until the INTC stuff catches up, but then we are probaby going to run in to a situation where they either have to co-exist, or the irqchips are removed and the sh_intc code has to carry a compat shim to deal with those DT bindings. Neither of those options seem particularly appealing to me. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Feb 27, 2013 at 5:23 PM, Paul Mundt wrote: > On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: >> From: Magnus Damm >> >> This patch adds a driver for external IRQ pins connected >> to the INTC block on recent SoCs from Renesas. >> > So how exactly does this interact with the existing sh_intc code? Or is > there some reason why you have opted to bypass it in order to implement a > simplified reduced-functionality version of INTC support focused only on > external pins? If both are used together this is going to be a nightmare > for locking, and it's also non-obvious how the IRQ domains on both sides > will interact. > > This needs a lot more explanation. Recent GIC-based SoCs do not make use of INTC for any on-chip I/O devices. This driver is meant to be used as a layer between the actual IRQ pin and the GIC. Anything else needs the full driver. The existing non-DT INTC driver can happily coexist with this driver like it does in the case of sh73a0 here: [PATCH 02/03] ARM: shmobile: INTC External IRQ pin driver on sh73a0 The driver is not meant to be used with INTC-only based systems like sh7372 and the SH architecture. I would be very happy if someone could get their shit together and fix up DT support for the common INTC code. This has not happened yet though. So if you know anyone with time to spare then feel free to suggest them to work together with Iwamatsu-san to get the DT version of the code reviewed together with Linaro. Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: > From: Magnus Damm > > This patch adds a driver for external IRQ pins connected > to the INTC block on recent SoCs from Renesas. > So how exactly does this interact with the existing sh_intc code? Or is there some reason why you have opted to bypass it in order to implement a simplified reduced-functionality version of INTC support focused only on external pins? If both are used together this is going to be a nightmare for locking, and it's also non-obvious how the IRQ domains on both sides will interact. This needs a lot more explanation. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: From: Magnus Damm d...@opensource.se This patch adds a driver for external IRQ pins connected to the INTC block on recent SoCs from Renesas. So how exactly does this interact with the existing sh_intc code? Or is there some reason why you have opted to bypass it in order to implement a simplified reduced-functionality version of INTC support focused only on external pins? If both are used together this is going to be a nightmare for locking, and it's also non-obvious how the IRQ domains on both sides will interact. This needs a lot more explanation. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Feb 27, 2013 at 5:23 PM, Paul Mundt let...@linux-sh.org wrote: On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: From: Magnus Damm d...@opensource.se This patch adds a driver for external IRQ pins connected to the INTC block on recent SoCs from Renesas. So how exactly does this interact with the existing sh_intc code? Or is there some reason why you have opted to bypass it in order to implement a simplified reduced-functionality version of INTC support focused only on external pins? If both are used together this is going to be a nightmare for locking, and it's also non-obvious how the IRQ domains on both sides will interact. This needs a lot more explanation. Recent GIC-based SoCs do not make use of INTC for any on-chip I/O devices. This driver is meant to be used as a layer between the actual IRQ pin and the GIC. Anything else needs the full driver. The existing non-DT INTC driver can happily coexist with this driver like it does in the case of sh73a0 here: [PATCH 02/03] ARM: shmobile: INTC External IRQ pin driver on sh73a0 The driver is not meant to be used with INTC-only based systems like sh7372 and the SH architecture. I would be very happy if someone could get their shit together and fix up DT support for the common INTC code. This has not happened yet though. So if you know anyone with time to spare then feel free to suggest them to work together with Iwamatsu-san to get the DT version of the code reviewed together with Linaro. Thanks, / magnus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Feb 27, 2013 at 05:35:51PM +0900, Magnus Damm wrote: On Wed, Feb 27, 2013 at 5:23 PM, Paul Mundt let...@linux-sh.org wrote: So how exactly does this interact with the existing sh_intc code? Or is there some reason why you have opted to bypass it in order to implement a simplified reduced-functionality version of INTC support focused only on external pins? If both are used together this is going to be a nightmare for locking, and it's also non-obvious how the IRQ domains on both sides will interact. This needs a lot more explanation. Recent GIC-based SoCs do not make use of INTC for any on-chip I/O devices. This driver is meant to be used as a layer between the actual IRQ pin and the GIC. Anything else needs the full driver. The existing non-DT INTC driver can happily coexist with this driver like it does in the case of sh73a0 here: [PATCH 02/03] ARM: shmobile: INTC External IRQ pin driver on sh73a0 Ok, thanks for clarifying. I suppose the main concern is how quickly this will simply turn in to a deviated partial implementation of the full driver as newer SoCs begin deviating from your simplified case, and we basically end up reimplementing sh_intc anyways. The driver is not meant to be used with INTC-only based systems like sh7372 and the SH architecture. I would be very happy if someone could get their shit together and fix up DT support for the common INTC code. This has not happened yet though. So if you know anyone with time to spare then feel free to suggest them to work together with Iwamatsu-san to get the DT version of the code reviewed together with Linaro. I haven't heard or seen anything new on that in some time, so I assumed the work had stalled. I'm not sure why there wasn't more effort put in to DT support for the INTC code rather than simply coming up with a temporary bypass shim, and I'm not sure why you think this work is blocked by anyone (unless you're just referring to a general lack of resources). In any event, I'm not sure what the best option for the interim is. I suppose we can merge the irqchips until the INTC stuff catches up, but then we are probaby going to run in to a situation where they either have to co-exist, or the irqchips are removed and the sh_intc code has to carry a compat shim to deal with those DT bindings. Neither of those options seem particularly appealing to me. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Feb 27, 2013 at 5:52 PM, Paul Mundt let...@linux-sh.org wrote: On Wed, Feb 27, 2013 at 05:35:51PM +0900, Magnus Damm wrote: On Wed, Feb 27, 2013 at 5:23 PM, Paul Mundt let...@linux-sh.org wrote: So how exactly does this interact with the existing sh_intc code? Or is there some reason why you have opted to bypass it in order to implement a simplified reduced-functionality version of INTC support focused only on external pins? If both are used together this is going to be a nightmare for locking, and it's also non-obvious how the IRQ domains on both sides will interact. This needs a lot more explanation. Recent GIC-based SoCs do not make use of INTC for any on-chip I/O devices. This driver is meant to be used as a layer between the actual IRQ pin and the GIC. Anything else needs the full driver. The existing non-DT INTC driver can happily coexist with this driver like it does in the case of sh73a0 here: [PATCH 02/03] ARM: shmobile: INTC External IRQ pin driver on sh73a0 Ok, thanks for clarifying. I suppose the main concern is how quickly this will simply turn in to a deviated partial implementation of the full driver as newer SoCs begin deviating from your simplified case, and we basically end up reimplementing sh_intc anyways. As you know, the INTC code that you are referring to is a full interrupt controller designed to work directly with CPU cores like SH and ARM. Newer ARM cores like Cortex-A9 all include the GIC both for IPI purpose in case of SMP and they also implement SPI functionality for I/O device interrupts. So the need for vendor-specific interrupt controller IP is almost down to nothing these days. With that in mind I do really doubt that we end up reimplementing sh_intc. The upcoming designs that I am aware of do not change their external IRQ pin hardware to force us to update this driver. What happens after that I'm afraid I can't say. =) The driver is not meant to be used with INTC-only based systems like sh7372 and the SH architecture. I would be very happy if someone could get their shit together and fix up DT support for the common INTC code. This has not happened yet though. So if you know anyone with time to spare then feel free to suggest them to work together with Iwamatsu-san to get the DT version of the code reviewed together with Linaro. I haven't heard or seen anything new on that in some time, so I assumed the work had stalled. I'm not sure why there wasn't more effort put in to DT support for the INTC code rather than simply coming up with a temporary bypass shim, and I'm not sure why you think this work is blocked by anyone (unless you're just referring to a general lack of resources). I mainly wrote this driver to support the r8a7779 SoC which can't be driven by the existing INTC code base. During development I decided to try to share the driver between multiple GIC-based SoCs like r8a7779 and sh73a0. The reason behind trying to share this driver between multiple SoCs is to remove SoC-specific hacks that can't be dealt with by the existing INTC code. In any event, I'm not sure what the best option for the interim is. I suppose we can merge the irqchips until the INTC stuff catches up, but then we are probaby going to run in to a situation where they either have to co-exist, or the irqchips are removed and the sh_intc code has to carry a compat shim to deal with those DT bindings. Neither of those options seem particularly appealing to me. I don't really understand why you would want to use a generic table driven driver when you have the possibility of using a hardware-specific driver. I suppose you are wondering why no one seems to be working on INTC DT support at this point. The truth is that we got very little feedback and development support with interrupts in general last autumn and on top of that our developers went their own way instead of following advice. Anyway, I do understand your worry about what happens if the hardware starts to deviate, but judging by the future devices that I've seen we should be ok for a while. Beyond that no one can tell. Thanks, / magnus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Wed, Feb 27, 2013 at 06:52:51PM +0900, Magnus Damm wrote: As you know, the INTC code that you are referring to is a full interrupt controller designed to work directly with CPU cores like SH and ARM. Newer ARM cores like Cortex-A9 all include the GIC both for IPI purpose in case of SMP and they also implement SPI functionality for I/O device interrupts. So the need for vendor-specific interrupt controller IP is almost down to nothing these days. Yes, I'm aware of that. With that in mind I do really doubt that we end up reimplementing sh_intc. The upcoming designs that I am aware of do not change their external IRQ pin hardware to force us to update this driver. What happens after that I'm afraid I can't say. =) While I don't expect you would end up with a full reimplmentation, my concern is more that it would just be reproducing stuff that's already in place. IOW, perhaps it's less work to put sh_intc on a diet, and tie the core functionality that you need for external IRQ pins in to an irqchip there -- with the core of the old code adapted in to some sort of common base library, rather than coming up with a new lightweight irqchip driver and having to incrementally pile stuff on top of it later. I mainly wrote this driver to support the r8a7779 SoC which can't be driven by the existing INTC code base. During development I decided to try to share the driver between multiple GIC-based SoCs like r8a7779 and sh73a0. The reason behind trying to share this driver between multiple SoCs is to remove SoC-specific hacks that can't be dealt with by the existing INTC code. Ok, fair enough. I don't really understand why you would want to use a generic table driven driver when you have the possibility of using a hardware-specific driver. For the same reason sh_intc was written in the first place, every CPU subtype more or less had a similar set of interrupt controllers with minor deviations. Those deviations were sufficient enough to make the code pretty hairy in places, and what we have now is really more of a subsystem than an irqchip driver. Having to reproduce 90% of the code when you're adding new CPUs at the rate of 2 a month hardly makes an SoC-specific driver realistic, especially as you just end up with identical features being broken in subtly differnt ways on different SoCs. That being said, a lot of that is legacy stuff and a result of no CPU team talking to any other CPU team ever, and it seems like things have improved enough in that regard that perhaps a hardware-specific driver won't be a complete throw-away disaster like it was before. It's a risk either way, I just hope your lightweight solution remains lightweight and consistent long enough that we don't have multiple copies of slightly different drivers doing exactly same thing spiralling out of control and turning in to a maintenance nightmare. If sh_intc doesn't deal with the needs of the new GIC-backed SoCs then that's of course something that has to be addressed regardless (whether that be hacking up sh_intc or adding new hardware-specific irqchips). I suppose you are wondering why no one seems to be working on INTC DT support at this point. The truth is that we got very little feedback and development support with interrupts in general last autumn and on top of that our developers went their own way instead of following advice. I assumed it was either being rewritten or had already been merged, so I was simply surprised to hear otherwise. I will dig through the archives a bit later and see about getting caught up. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Tue, 19 Feb 2013, Magnus Damm wrote:
> On Tue, Feb 19, 2013 at 7:11 PM, Thomas Gleixner wrote:
> >> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
> >
> > Shouldn't the lock be part of struct intc_irqpin_priv ?
>
> Good idea, but I need to lock access to the SENSE register against
> multiple driver instances. This is not the case for PRIO. But because
> both PRIO and SENSE are accessed in the slow path I decided to be lazy
> and use the same way of locking to reduce the code size.
Fair enough.
> >> +static void intc_irqpin_irq_enable_force(struct irq_data *d)
> >> +{
> >> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> >> + int irq = p->irq[irqd_to_hwirq(d)].irq;
> >> +
> >> + intc_irqpin_irq_enable(d);
> >> + irq_get_chip(irq)->irq_unmask(irq_get_irq_data(irq));
> >> +}
> >> +
> >> +static void intc_irqpin_irq_disable_force(struct irq_data *d)
> >> +{
> >> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> >> + int irq = p->irq[irqd_to_hwirq(d)].irq;
> >> +
> >> + irq_get_chip(irq)->irq_mask(irq_get_irq_data(irq));
> >> + intc_irqpin_irq_disable(d);
> >
> > Hmm. If I understand that code correctly, then the _force functions
> > are (un)masking another interrupt line. But this happens without
> > holding irq_desc[irq]->lock. Looks unsafe at least and if correct
> > wants a big fat comment.
>
> On some SoCs the masking for some IRQs seems busted, and the only sane
> way to work around that is to (un)mask the parent interrupt. The
> parent happens to be the GIC. I will look into how to add locking.
Is there a 1:1 relationship between the intc interrupt and the GIC? If
yes and if nothing else fiddles with that particular GIC interrupt,
then you might get away without extra locking, but that wants a
comment at least.
> >> + irq_set_chip_and_handler(virq, >irq_chip, handle_level_irq);
> >> + set_irq_flags(virq, IRQF_VALID); /* kill me now */
> >
> > What needs to be killed? -ENOPARSE
>
> I'd like to not have to set this flag in my interrupt code.
Ah.
> I've written interrupt code on other architectures before, and from my
> experience only ARM requires the IRQF_VALID flag to be set because the
> ARM architecture software believes it is a special case. I may be
> behind times - I have to admit that I've not checked the latest state
> - this flag may not be needed anymore, hurray if so - but at least a
> couple of years ago it was needed in case of ARM for our shared INTC
> code (shared between sh and ARM).
>
> What is your opinion about this matter?
It provides an extra paranoia level, but I'm not sure if it's really
worth the trouble.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
Hi Thomas,
Thanks for your help with the review!
On Tue, Feb 19, 2013 at 7:11 PM, Thomas Gleixner wrote:
> Magnus,
>
> On Mon, 18 Feb 2013, Magnus Damm wrote:
>
>> +static inline unsigned long intc_irqpin_read(struct intc_irqpin_priv *p,
>> + int reg)
>> +{
>> + struct intc_irqpin_iomem *i = >iomem[reg];
>
> Newline between variable and code please.
Yes, I agree, will fix. I have been criticized for adding too many
newlines in the past, so I guess this is a good sign that I can flip
the other way now!
>> + return i->read(i->iomem);
>> +}
>> +
>> +static inline void intc_irqpin_write(struct intc_irqpin_priv *p,
>> + int reg, unsigned long data)
>> +{
>> + struct intc_irqpin_iomem *i = >iomem[reg];
>> + i->write(i->iomem, data);
>> +}
>> +
>> +static inline unsigned long intc_irqpin_hwirq_mask(struct intc_irqpin_priv
>> *p,
>> +int reg, int hw_irq)
>> +{
>> + return BIT((p->iomem[reg].width - 1) - hw_irq);
>> +}
>> +
>> +static inline void intc_irqpin_irq_write_hwirq(struct intc_irqpin_priv *p,
>> +int reg, int hw_irq)
>> +{
>> + intc_irqpin_write(p, reg, intc_irqpin_hwirq_mask(p, reg, hw_irq));
>> +}
>> +
>> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
>
> Shouldn't the lock be part of struct intc_irqpin_priv ?
Good idea, but I need to lock access to the SENSE register against
multiple driver instances. This is not the case for PRIO. But because
both PRIO and SENSE are accessed in the slow path I decided to be lazy
and use the same way of locking to reduce the code size.
>> +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
>> + int reg, int shift,
>> + int width, int value)
>> +{
>> + unsigned long flags;
>> + unsigned long tmp;
>> +
>> + raw_spin_lock_irqsave(_irqpin_lock, flags);
>> +static void intc_irqpin_dbg(struct intc_irqpin_irq *i, char *str)
>> +{
>> + dev_dbg(>p->pdev->dev, "%s (%d:%d:%d)\n",
>> + str, i->irq, i->hw_irq,
>> + irq_find_mapping(i->p->irq_domain, i->hw_irq));
>
> Do you really want to do a lookup for each debug invocation?
Uhm, maybe no. I need to check if this compiles out in case of DEBUG=n.
>> +}
>> +
>> +static void intc_irqpin_irq_enable(struct irq_data *d)
>> +{
>> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
>> + int hw_irq = irqd_to_hwirq(d);
>> +
>> + intc_irqpin_dbg(>irq[hw_irq], "enable");
>> + intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_CLEAR, hw_irq);
>> +}
>> +
>> +static void intc_irqpin_irq_disable(struct irq_data *d)
>> +{
>> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
>> + int hw_irq = irqd_to_hwirq(d);
>> +
>> + intc_irqpin_dbg(>irq[hw_irq], "disable");
>> + intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_MASK, hw_irq);
>> +}
>> +
>> +static void intc_irqpin_irq_enable_force(struct irq_data *d)
>> +{
>> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
>> + int irq = p->irq[irqd_to_hwirq(d)].irq;
>> +
>> + intc_irqpin_irq_enable(d);
>> + irq_get_chip(irq)->irq_unmask(irq_get_irq_data(irq));
>> +}
>> +
>> +static void intc_irqpin_irq_disable_force(struct irq_data *d)
>> +{
>> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
>> + int irq = p->irq[irqd_to_hwirq(d)].irq;
>> +
>> + irq_get_chip(irq)->irq_mask(irq_get_irq_data(irq));
>> + intc_irqpin_irq_disable(d);
>
> Hmm. If I understand that code correctly, then the _force functions
> are (un)masking another interrupt line. But this happens without
> holding irq_desc[irq]->lock. Looks unsafe at least and if correct
> wants a big fat comment.
On some SoCs the masking for some IRQs seems busted, and the only sane
way to work around that is to (un)mask the parent interrupt. The
parent happens to be the GIC. I will look into how to add locking.
>> +}
>> +
>> +#define INTC_IRQ_SENSE_VALID 0x10
>> +#define INTC_IRQ_SENSE(x) (x + INTC_IRQ_SENSE_VALID)
>> +
>> +static unsigned char intc_irqpin_sense[IRQ_TYPE_SENSE_MASK + 1] = {
>> + [IRQ_TYPE_EDGE_FALLING] = INTC_IRQ_SENSE(0x00),
>> + [IRQ_TYPE_EDGE_RISING] = INTC_IRQ_SENSE(0x01),
>> + [IRQ_TYPE_LEVEL_LOW] = INTC_IRQ_SENSE(0x02),
>> + [IRQ_TYPE_LEVEL_HIGH] = INTC_IRQ_SENSE(0x03),
>> + [IRQ_TYPE_EDGE_BOTH] = INTC_IRQ_SENSE(0x04),
>> +};
>> +
>> +static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type)
>> +{
>> + unsigned char value = intc_irqpin_sense[type & IRQ_TYPE_SENSE_MASK];
>> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
>> +
>> + if (!(value & INTC_IRQ_SENSE_VALID))
>> + return -EINVAL;
>> +
>> + return intc_irqpin_set_sense(p, irqd_to_hwirq(d),
>> + value ^
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
Hi Morimoto-san,
On Tue, Feb 19, 2013 at 10:04 AM, Kuninori Morimoto
wrote:
>
> Hi Magnus
>
> Thank you for this patch.
> Small comment from me :)
Sure, thanks!
>> +struct intc_irqpin_priv {
>> + struct intc_irqpin_iomem iomem[INTC_IRQPIN_REG_NR];
>> + struct intc_irqpin_irq irq[INTC_IRQPIN_MAX];
>> + struct renesas_intc_irqpin_config config;
>> + unsigned int number_of_irqs;
>> + struct platform_device *pdev;
>> + struct irq_chip irq_chip;
>> + struct irq_domain *irq_domain;
>> +};
>
> (snip)
>
>> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
>> +
>> +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
>> + int reg, int shift,
>> + int width, int value)
>> +{
>> + unsigned long flags;
>> + unsigned long tmp;
>> +
>> + raw_spin_lock_irqsave(_irqpin_lock, flags);
>> +
>> + tmp = intc_irqpin_read(p, reg);
>> + tmp &= ~(((1 << width) - 1) << shift);
>> + tmp |= value << shift;
>> + intc_irqpin_write(p, reg, tmp);
>> +
>> + raw_spin_unlock_irqrestore(_irqpin_lock, flags);
>> +}
>
> It is possible to include this spin lock into priv ?
> This local static spin lock seems not wrong, but looks strange ?
Please see this comment (that you snipped out above):
+/* INTC external IRQ PIN hardware register access:
+ *
+ * SENSE is read-write 32-bit with 2-bits or 4-bits per IRQ (*)
+ * PRIO is read-write 32-bit with 4-bits per IRQ (**)
+ * SOURCE is read-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ * MASK is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ * CLEAR is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ *
+ * (*) May be accessed by more than one driver instance - lock needed
+ * (**) Read-modify-write access by one driver instance - lock needed
+ * (***) Accessed by one driver instance only - no locking needed
+ */
Basically, the lock is used for SENSE and PRIO. SENSE may be shared
between multiple driver instances, so a lock in ->priv won't be
enough. PRIO may be locked using ->priv but both SENSE and PRIV are in
the slow path so I decided to handle both using a global lock.
>> +static int intc_irqpin_probe(struct platform_device *pdev)
>> +{
>> + struct renesas_intc_irqpin_config *pdata = pdev->dev.platform_data;
>> + struct intc_irqpin_priv *p;
>> + struct intc_irqpin_iomem *i;
>> + struct resource *io[INTC_IRQPIN_REG_NR];
>> + struct resource *irq;
>> + struct irq_chip *irq_chip;
>> + void (*enable_fn)(struct irq_data *d);
>> + void (*disable_fn)(struct irq_data *d);
>> + const char *name = dev_name(>dev);
>> + int ret;
>> + int k;
>> +
>> + p = kzalloc(sizeof(*p), GFP_KERNEL);
>
> can you use devm_kzalloc() ?
> devm_ioremap_nocache() or devm_request_and_ioremap()
> Can you use devm_request_irq()
> if you used devm_, you can remove kfree() / free_irq() / iounmap() here
I somehow knew that this would come up. Will give devm a go in V2 -
unless someone tells me such a change is going to make the driver more
painful to back port to LTSI-3.4.
>> --- /dev/null
>> +++ work/include/linux/platform_data/irq-renesas-intc-irqpin.h
>> 2013-02-18 18:28:24.0 +0900
>> @@ -0,0 +1,10 @@
>> +#ifndef __IRQ_RENESAS_INTC_IRQPIN_H__
>> +#define __IRQ_RENESAS_INTC_IRQPIN_H__
>
> GPL license signage ?
Let me check how other files in include/linux/platform_data/ handles
this. I will follow the majority.
Thanks,
/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Tue, Feb 19, 2013 at 10:03 AM, Simon Horman wrote: > On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: >> From: Magnus Damm >> >> This patch adds a driver for external IRQ pins connected >> to the INTC block on recent SoCs from Renesas. >> >> The INTC hardware block usually contains a rather wide >> range of features ranging from external IRQ pin handling >> to legacy interrupt controller support. On older SoCs >> the INTC is used as a general purpose interrupt controller >> both for external IRQ pins and on-chip devices. >> >> On more recent ARM based SoCs with Cortex-A9 the main >> interrupt controller is the GIC, but IRQ trigger setup >> still need to happen in the INTC hardware block. >> >> This driver implements the glue code needed to configure >> IRQ trigger and also handle mask/unmask and demux of >> external IRQ pins hooked up from the INTC to the GIC. >> >> Tested on sh73a0 and r8a7779. The hardware varies quite >> a bit with SoC model, for instance register width and >> bitfield widths vary wildly. The driver requires one GIC >> SPI per external IRQ pin to operate. Each driver instance >> will handle up to 8 external IRQ pins. >> >> The SoCs using this driver are currently mainly used >> together with regular platform devices so this driver >> allows configuration via platform data to support things >> like static interrupt base address. DT support will >> be added incrementally in the not so distant future. >> >> Signed-off-by: Magnus Damm > > Hi Magnus, Hi all, > > I do not expect this code to go through the renesas tree. However, in > order to provide a basis for work on renesas SoCs I have added this patch > to the topic/intc-external-irq topic branch in the reneas tree on > kernel.org and merged it into topic/all+next. > > In other words, I am not picking this series up to merge it or add it to > linux-next, rather I am storing it for reference. > > > In the course of adding the branch I noticed a 3 whitespace warnings > from git. I have highlighted them below. Thanks Simon. I will fix those up in V2! / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
Magnus,
On Mon, 18 Feb 2013, Magnus Damm wrote:
> +static inline unsigned long intc_irqpin_read(struct intc_irqpin_priv *p,
> + int reg)
> +{
> + struct intc_irqpin_iomem *i = >iomem[reg];
Newline between variable and code please.
> + return i->read(i->iomem);
> +}
> +
> +static inline void intc_irqpin_write(struct intc_irqpin_priv *p,
> + int reg, unsigned long data)
> +{
> + struct intc_irqpin_iomem *i = >iomem[reg];
> + i->write(i->iomem, data);
> +}
> +
> +static inline unsigned long intc_irqpin_hwirq_mask(struct intc_irqpin_priv
> *p,
> +int reg, int hw_irq)
> +{
> + return BIT((p->iomem[reg].width - 1) - hw_irq);
> +}
> +
> +static inline void intc_irqpin_irq_write_hwirq(struct intc_irqpin_priv *p,
> +int reg, int hw_irq)
> +{
> + intc_irqpin_write(p, reg, intc_irqpin_hwirq_mask(p, reg, hw_irq));
> +}
> +
> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
Shouldn't the lock be part of struct intc_irqpin_priv ?
> +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
> + int reg, int shift,
> + int width, int value)
> +{
> + unsigned long flags;
> + unsigned long tmp;
> +
> + raw_spin_lock_irqsave(_irqpin_lock, flags);
> +static void intc_irqpin_dbg(struct intc_irqpin_irq *i, char *str)
> +{
> + dev_dbg(>p->pdev->dev, "%s (%d:%d:%d)\n",
> + str, i->irq, i->hw_irq,
> + irq_find_mapping(i->p->irq_domain, i->hw_irq));
Do you really want to do a lookup for each debug invocation?
> +}
> +
> +static void intc_irqpin_irq_enable(struct irq_data *d)
> +{
> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> + int hw_irq = irqd_to_hwirq(d);
> +
> + intc_irqpin_dbg(>irq[hw_irq], "enable");
> + intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_CLEAR, hw_irq);
> +}
> +
> +static void intc_irqpin_irq_disable(struct irq_data *d)
> +{
> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> + int hw_irq = irqd_to_hwirq(d);
> +
> + intc_irqpin_dbg(>irq[hw_irq], "disable");
> + intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_MASK, hw_irq);
> +}
> +
> +static void intc_irqpin_irq_enable_force(struct irq_data *d)
> +{
> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> + int irq = p->irq[irqd_to_hwirq(d)].irq;
> +
> + intc_irqpin_irq_enable(d);
> + irq_get_chip(irq)->irq_unmask(irq_get_irq_data(irq));
> +}
> +
> +static void intc_irqpin_irq_disable_force(struct irq_data *d)
> +{
> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> + int irq = p->irq[irqd_to_hwirq(d)].irq;
> +
> + irq_get_chip(irq)->irq_mask(irq_get_irq_data(irq));
> + intc_irqpin_irq_disable(d);
Hmm. If I understand that code correctly, then the _force functions
are (un)masking another interrupt line. But this happens without
holding irq_desc[irq]->lock. Looks unsafe at least and if correct
wants a big fat comment.
> +}
> +
> +#define INTC_IRQ_SENSE_VALID 0x10
> +#define INTC_IRQ_SENSE(x) (x + INTC_IRQ_SENSE_VALID)
> +
> +static unsigned char intc_irqpin_sense[IRQ_TYPE_SENSE_MASK + 1] = {
> + [IRQ_TYPE_EDGE_FALLING] = INTC_IRQ_SENSE(0x00),
> + [IRQ_TYPE_EDGE_RISING] = INTC_IRQ_SENSE(0x01),
> + [IRQ_TYPE_LEVEL_LOW] = INTC_IRQ_SENSE(0x02),
> + [IRQ_TYPE_LEVEL_HIGH] = INTC_IRQ_SENSE(0x03),
> + [IRQ_TYPE_EDGE_BOTH] = INTC_IRQ_SENSE(0x04),
> +};
> +
> +static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> + unsigned char value = intc_irqpin_sense[type & IRQ_TYPE_SENSE_MASK];
> + struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +
> + if (!(value & INTC_IRQ_SENSE_VALID))
> + return -EINVAL;
> +
> + return intc_irqpin_set_sense(p, irqd_to_hwirq(d),
> + value ^ INTC_IRQ_SENSE_VALID);
> +}
> +
> +static irqreturn_t intc_irqpin_irq_handler(int irq, void *dev_id)
> +{
> + struct intc_irqpin_irq *i = dev_id;
> + struct intc_irqpin_priv *p = i->p;
> + unsigned long bit;
> +
> + intc_irqpin_dbg(i, "demux1");
> + bit = intc_irqpin_hwirq_mask(p, INTC_IRQPIN_REG_SOURCE, i->hw_irq);
> +
> + if (intc_irqpin_read(p, INTC_IRQPIN_REG_SOURCE) & bit) {
> + intc_irqpin_write(p, INTC_IRQPIN_REG_SOURCE, ~bit);
> + intc_irqpin_dbg(i, "demux2");
> + generic_handle_irq(irq_find_mapping(p->irq_domain, i->hw_irq));
Shouldn't you cache that mapping value somewhere ?
> + return IRQ_HANDLED;
> + }
> + return IRQ_NONE;
> +}
> +
> +static int intc_irqpin_irq_domain_map(struct irq_domain *h, unsigned int
> virq,
> + irq_hw_number_t hw)
> +{
> + struct intc_irqpin_priv *p = h->host_data;
> +
> +
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
Magnus,
On Mon, 18 Feb 2013, Magnus Damm wrote:
+static inline unsigned long intc_irqpin_read(struct intc_irqpin_priv *p,
+ int reg)
+{
+ struct intc_irqpin_iomem *i = p-iomem[reg];
Newline between variable and code please.
+ return i-read(i-iomem);
+}
+
+static inline void intc_irqpin_write(struct intc_irqpin_priv *p,
+ int reg, unsigned long data)
+{
+ struct intc_irqpin_iomem *i = p-iomem[reg];
+ i-write(i-iomem, data);
+}
+
+static inline unsigned long intc_irqpin_hwirq_mask(struct intc_irqpin_priv
*p,
+int reg, int hw_irq)
+{
+ return BIT((p-iomem[reg].width - 1) - hw_irq);
+}
+
+static inline void intc_irqpin_irq_write_hwirq(struct intc_irqpin_priv *p,
+int reg, int hw_irq)
+{
+ intc_irqpin_write(p, reg, intc_irqpin_hwirq_mask(p, reg, hw_irq));
+}
+
+static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
Shouldn't the lock be part of struct intc_irqpin_priv ?
+static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
+ int reg, int shift,
+ int width, int value)
+{
+ unsigned long flags;
+ unsigned long tmp;
+
+ raw_spin_lock_irqsave(intc_irqpin_lock, flags);
+static void intc_irqpin_dbg(struct intc_irqpin_irq *i, char *str)
+{
+ dev_dbg(i-p-pdev-dev, %s (%d:%d:%d)\n,
+ str, i-irq, i-hw_irq,
+ irq_find_mapping(i-p-irq_domain, i-hw_irq));
Do you really want to do a lookup for each debug invocation?
+}
+
+static void intc_irqpin_irq_enable(struct irq_data *d)
+{
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+ int hw_irq = irqd_to_hwirq(d);
+
+ intc_irqpin_dbg(p-irq[hw_irq], enable);
+ intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_CLEAR, hw_irq);
+}
+
+static void intc_irqpin_irq_disable(struct irq_data *d)
+{
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+ int hw_irq = irqd_to_hwirq(d);
+
+ intc_irqpin_dbg(p-irq[hw_irq], disable);
+ intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_MASK, hw_irq);
+}
+
+static void intc_irqpin_irq_enable_force(struct irq_data *d)
+{
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+ int irq = p-irq[irqd_to_hwirq(d)].irq;
+
+ intc_irqpin_irq_enable(d);
+ irq_get_chip(irq)-irq_unmask(irq_get_irq_data(irq));
+}
+
+static void intc_irqpin_irq_disable_force(struct irq_data *d)
+{
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+ int irq = p-irq[irqd_to_hwirq(d)].irq;
+
+ irq_get_chip(irq)-irq_mask(irq_get_irq_data(irq));
+ intc_irqpin_irq_disable(d);
Hmm. If I understand that code correctly, then the _force functions
are (un)masking another interrupt line. But this happens without
holding irq_desc[irq]-lock. Looks unsafe at least and if correct
wants a big fat comment.
+}
+
+#define INTC_IRQ_SENSE_VALID 0x10
+#define INTC_IRQ_SENSE(x) (x + INTC_IRQ_SENSE_VALID)
+
+static unsigned char intc_irqpin_sense[IRQ_TYPE_SENSE_MASK + 1] = {
+ [IRQ_TYPE_EDGE_FALLING] = INTC_IRQ_SENSE(0x00),
+ [IRQ_TYPE_EDGE_RISING] = INTC_IRQ_SENSE(0x01),
+ [IRQ_TYPE_LEVEL_LOW] = INTC_IRQ_SENSE(0x02),
+ [IRQ_TYPE_LEVEL_HIGH] = INTC_IRQ_SENSE(0x03),
+ [IRQ_TYPE_EDGE_BOTH] = INTC_IRQ_SENSE(0x04),
+};
+
+static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type)
+{
+ unsigned char value = intc_irqpin_sense[type IRQ_TYPE_SENSE_MASK];
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+
+ if (!(value INTC_IRQ_SENSE_VALID))
+ return -EINVAL;
+
+ return intc_irqpin_set_sense(p, irqd_to_hwirq(d),
+ value ^ INTC_IRQ_SENSE_VALID);
+}
+
+static irqreturn_t intc_irqpin_irq_handler(int irq, void *dev_id)
+{
+ struct intc_irqpin_irq *i = dev_id;
+ struct intc_irqpin_priv *p = i-p;
+ unsigned long bit;
+
+ intc_irqpin_dbg(i, demux1);
+ bit = intc_irqpin_hwirq_mask(p, INTC_IRQPIN_REG_SOURCE, i-hw_irq);
+
+ if (intc_irqpin_read(p, INTC_IRQPIN_REG_SOURCE) bit) {
+ intc_irqpin_write(p, INTC_IRQPIN_REG_SOURCE, ~bit);
+ intc_irqpin_dbg(i, demux2);
+ generic_handle_irq(irq_find_mapping(p-irq_domain, i-hw_irq));
Shouldn't you cache that mapping value somewhere ?
+ return IRQ_HANDLED;
+ }
+ return IRQ_NONE;
+}
+
+static int intc_irqpin_irq_domain_map(struct irq_domain *h, unsigned int
virq,
+ irq_hw_number_t hw)
+{
+ struct intc_irqpin_priv *p = h-host_data;
+
+ intc_irqpin_dbg(p-irq[hw], map);
+ irq_set_chip_data(virq, h-host_data);
+ irq_set_chip_and_handler(virq, p-irq_chip, handle_level_irq);
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Tue, Feb 19, 2013 at 10:03 AM, Simon Horman ho...@verge.net.au wrote: On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: From: Magnus Damm d...@opensource.se This patch adds a driver for external IRQ pins connected to the INTC block on recent SoCs from Renesas. The INTC hardware block usually contains a rather wide range of features ranging from external IRQ pin handling to legacy interrupt controller support. On older SoCs the INTC is used as a general purpose interrupt controller both for external IRQ pins and on-chip devices. On more recent ARM based SoCs with Cortex-A9 the main interrupt controller is the GIC, but IRQ trigger setup still need to happen in the INTC hardware block. This driver implements the glue code needed to configure IRQ trigger and also handle mask/unmask and demux of external IRQ pins hooked up from the INTC to the GIC. Tested on sh73a0 and r8a7779. The hardware varies quite a bit with SoC model, for instance register width and bitfield widths vary wildly. The driver requires one GIC SPI per external IRQ pin to operate. Each driver instance will handle up to 8 external IRQ pins. The SoCs using this driver are currently mainly used together with regular platform devices so this driver allows configuration via platform data to support things like static interrupt base address. DT support will be added incrementally in the not so distant future. Signed-off-by: Magnus Damm d...@opensource.se Hi Magnus, Hi all, I do not expect this code to go through the renesas tree. However, in order to provide a basis for work on renesas SoCs I have added this patch to the topic/intc-external-irq topic branch in the reneas tree on kernel.org and merged it into topic/all+next. In other words, I am not picking this series up to merge it or add it to linux-next, rather I am storing it for reference. In the course of adding the branch I noticed a 3 whitespace warnings from git. I have highlighted them below. Thanks Simon. I will fix those up in V2! / magnus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
Hi Morimoto-san,
On Tue, Feb 19, 2013 at 10:04 AM, Kuninori Morimoto
kuninori.morimoto...@renesas.com wrote:
Hi Magnus
Thank you for this patch.
Small comment from me :)
Sure, thanks!
+struct intc_irqpin_priv {
+ struct intc_irqpin_iomem iomem[INTC_IRQPIN_REG_NR];
+ struct intc_irqpin_irq irq[INTC_IRQPIN_MAX];
+ struct renesas_intc_irqpin_config config;
+ unsigned int number_of_irqs;
+ struct platform_device *pdev;
+ struct irq_chip irq_chip;
+ struct irq_domain *irq_domain;
+};
(snip)
+static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
+
+static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
+ int reg, int shift,
+ int width, int value)
+{
+ unsigned long flags;
+ unsigned long tmp;
+
+ raw_spin_lock_irqsave(intc_irqpin_lock, flags);
+
+ tmp = intc_irqpin_read(p, reg);
+ tmp = ~(((1 width) - 1) shift);
+ tmp |= value shift;
+ intc_irqpin_write(p, reg, tmp);
+
+ raw_spin_unlock_irqrestore(intc_irqpin_lock, flags);
+}
It is possible to include this spin lock into priv ?
This local static spin lock seems not wrong, but looks strange ?
Please see this comment (that you snipped out above):
+/* INTC external IRQ PIN hardware register access:
+ *
+ * SENSE is read-write 32-bit with 2-bits or 4-bits per IRQ (*)
+ * PRIO is read-write 32-bit with 4-bits per IRQ (**)
+ * SOURCE is read-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ * MASK is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ * CLEAR is write-only 32-bit or 8-bit with 1-bit per IRQ (***)
+ *
+ * (*) May be accessed by more than one driver instance - lock needed
+ * (**) Read-modify-write access by one driver instance - lock needed
+ * (***) Accessed by one driver instance only - no locking needed
+ */
Basically, the lock is used for SENSE and PRIO. SENSE may be shared
between multiple driver instances, so a lock in -priv won't be
enough. PRIO may be locked using -priv but both SENSE and PRIV are in
the slow path so I decided to handle both using a global lock.
+static int intc_irqpin_probe(struct platform_device *pdev)
+{
+ struct renesas_intc_irqpin_config *pdata = pdev-dev.platform_data;
+ struct intc_irqpin_priv *p;
+ struct intc_irqpin_iomem *i;
+ struct resource *io[INTC_IRQPIN_REG_NR];
+ struct resource *irq;
+ struct irq_chip *irq_chip;
+ void (*enable_fn)(struct irq_data *d);
+ void (*disable_fn)(struct irq_data *d);
+ const char *name = dev_name(pdev-dev);
+ int ret;
+ int k;
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
can you use devm_kzalloc() ?
devm_ioremap_nocache() or devm_request_and_ioremap()
Can you use devm_request_irq()
if you used devm_, you can remove kfree() / free_irq() / iounmap() here
I somehow knew that this would come up. Will give devm a go in V2 -
unless someone tells me such a change is going to make the driver more
painful to back port to LTSI-3.4.
--- /dev/null
+++ work/include/linux/platform_data/irq-renesas-intc-irqpin.h
2013-02-18 18:28:24.0 +0900
@@ -0,0 +1,10 @@
+#ifndef __IRQ_RENESAS_INTC_IRQPIN_H__
+#define __IRQ_RENESAS_INTC_IRQPIN_H__
GPL license signage ?
Let me check how other files in include/linux/platform_data/ handles
this. I will follow the majority.
Thanks,
/ magnus
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
Hi Thomas,
Thanks for your help with the review!
On Tue, Feb 19, 2013 at 7:11 PM, Thomas Gleixner t...@linutronix.de wrote:
Magnus,
On Mon, 18 Feb 2013, Magnus Damm wrote:
+static inline unsigned long intc_irqpin_read(struct intc_irqpin_priv *p,
+ int reg)
+{
+ struct intc_irqpin_iomem *i = p-iomem[reg];
Newline between variable and code please.
Yes, I agree, will fix. I have been criticized for adding too many
newlines in the past, so I guess this is a good sign that I can flip
the other way now!
+ return i-read(i-iomem);
+}
+
+static inline void intc_irqpin_write(struct intc_irqpin_priv *p,
+ int reg, unsigned long data)
+{
+ struct intc_irqpin_iomem *i = p-iomem[reg];
+ i-write(i-iomem, data);
+}
+
+static inline unsigned long intc_irqpin_hwirq_mask(struct intc_irqpin_priv
*p,
+int reg, int hw_irq)
+{
+ return BIT((p-iomem[reg].width - 1) - hw_irq);
+}
+
+static inline void intc_irqpin_irq_write_hwirq(struct intc_irqpin_priv *p,
+int reg, int hw_irq)
+{
+ intc_irqpin_write(p, reg, intc_irqpin_hwirq_mask(p, reg, hw_irq));
+}
+
+static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
Shouldn't the lock be part of struct intc_irqpin_priv ?
Good idea, but I need to lock access to the SENSE register against
multiple driver instances. This is not the case for PRIO. But because
both PRIO and SENSE are accessed in the slow path I decided to be lazy
and use the same way of locking to reduce the code size.
+static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
+ int reg, int shift,
+ int width, int value)
+{
+ unsigned long flags;
+ unsigned long tmp;
+
+ raw_spin_lock_irqsave(intc_irqpin_lock, flags);
+static void intc_irqpin_dbg(struct intc_irqpin_irq *i, char *str)
+{
+ dev_dbg(i-p-pdev-dev, %s (%d:%d:%d)\n,
+ str, i-irq, i-hw_irq,
+ irq_find_mapping(i-p-irq_domain, i-hw_irq));
Do you really want to do a lookup for each debug invocation?
Uhm, maybe no. I need to check if this compiles out in case of DEBUG=n.
+}
+
+static void intc_irqpin_irq_enable(struct irq_data *d)
+{
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+ int hw_irq = irqd_to_hwirq(d);
+
+ intc_irqpin_dbg(p-irq[hw_irq], enable);
+ intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_CLEAR, hw_irq);
+}
+
+static void intc_irqpin_irq_disable(struct irq_data *d)
+{
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+ int hw_irq = irqd_to_hwirq(d);
+
+ intc_irqpin_dbg(p-irq[hw_irq], disable);
+ intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_MASK, hw_irq);
+}
+
+static void intc_irqpin_irq_enable_force(struct irq_data *d)
+{
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+ int irq = p-irq[irqd_to_hwirq(d)].irq;
+
+ intc_irqpin_irq_enable(d);
+ irq_get_chip(irq)-irq_unmask(irq_get_irq_data(irq));
+}
+
+static void intc_irqpin_irq_disable_force(struct irq_data *d)
+{
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+ int irq = p-irq[irqd_to_hwirq(d)].irq;
+
+ irq_get_chip(irq)-irq_mask(irq_get_irq_data(irq));
+ intc_irqpin_irq_disable(d);
Hmm. If I understand that code correctly, then the _force functions
are (un)masking another interrupt line. But this happens without
holding irq_desc[irq]-lock. Looks unsafe at least and if correct
wants a big fat comment.
On some SoCs the masking for some IRQs seems busted, and the only sane
way to work around that is to (un)mask the parent interrupt. The
parent happens to be the GIC. I will look into how to add locking.
+}
+
+#define INTC_IRQ_SENSE_VALID 0x10
+#define INTC_IRQ_SENSE(x) (x + INTC_IRQ_SENSE_VALID)
+
+static unsigned char intc_irqpin_sense[IRQ_TYPE_SENSE_MASK + 1] = {
+ [IRQ_TYPE_EDGE_FALLING] = INTC_IRQ_SENSE(0x00),
+ [IRQ_TYPE_EDGE_RISING] = INTC_IRQ_SENSE(0x01),
+ [IRQ_TYPE_LEVEL_LOW] = INTC_IRQ_SENSE(0x02),
+ [IRQ_TYPE_LEVEL_HIGH] = INTC_IRQ_SENSE(0x03),
+ [IRQ_TYPE_EDGE_BOTH] = INTC_IRQ_SENSE(0x04),
+};
+
+static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type)
+{
+ unsigned char value = intc_irqpin_sense[type IRQ_TYPE_SENSE_MASK];
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+
+ if (!(value INTC_IRQ_SENSE_VALID))
+ return -EINVAL;
+
+ return intc_irqpin_set_sense(p, irqd_to_hwirq(d),
+ value ^ INTC_IRQ_SENSE_VALID);
+}
+
+static irqreturn_t intc_irqpin_irq_handler(int irq, void *dev_id)
+{
+ struct intc_irqpin_irq *i = dev_id;
+ struct intc_irqpin_priv *p = i-p;
+ unsigned long bit;
+
+
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Tue, 19 Feb 2013, Magnus Damm wrote:
On Tue, Feb 19, 2013 at 7:11 PM, Thomas Gleixner t...@linutronix.de wrote:
+static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
Shouldn't the lock be part of struct intc_irqpin_priv ?
Good idea, but I need to lock access to the SENSE register against
multiple driver instances. This is not the case for PRIO. But because
both PRIO and SENSE are accessed in the slow path I decided to be lazy
and use the same way of locking to reduce the code size.
Fair enough.
+static void intc_irqpin_irq_enable_force(struct irq_data *d)
+{
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+ int irq = p-irq[irqd_to_hwirq(d)].irq;
+
+ intc_irqpin_irq_enable(d);
+ irq_get_chip(irq)-irq_unmask(irq_get_irq_data(irq));
+}
+
+static void intc_irqpin_irq_disable_force(struct irq_data *d)
+{
+ struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
+ int irq = p-irq[irqd_to_hwirq(d)].irq;
+
+ irq_get_chip(irq)-irq_mask(irq_get_irq_data(irq));
+ intc_irqpin_irq_disable(d);
Hmm. If I understand that code correctly, then the _force functions
are (un)masking another interrupt line. But this happens without
holding irq_desc[irq]-lock. Looks unsafe at least and if correct
wants a big fat comment.
On some SoCs the masking for some IRQs seems busted, and the only sane
way to work around that is to (un)mask the parent interrupt. The
parent happens to be the GIC. I will look into how to add locking.
Is there a 1:1 relationship between the intc interrupt and the GIC? If
yes and if nothing else fiddles with that particular GIC interrupt,
then you might get away without extra locking, but that wants a
comment at least.
+ irq_set_chip_and_handler(virq, p-irq_chip, handle_level_irq);
+ set_irq_flags(virq, IRQF_VALID); /* kill me now */
What needs to be killed? -ENOPARSE
I'd like to not have to set this flag in my interrupt code.
Ah.
I've written interrupt code on other architectures before, and from my
experience only ARM requires the IRQF_VALID flag to be set because the
ARM architecture software believes it is a special case. I may be
behind times - I have to admit that I've not checked the latest state
- this flag may not be needed anymore, hurray if so - but at least a
couple of years ago it was needed in case of ARM for our shared INTC
code (shared between sh and ARM).
What is your opinion about this matter?
It provides an extra paranoia level, but I'm not sure if it's really
worth the trouble.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
Hi Magnus
Thank you for this patch.
Small comment from me :)
> +struct intc_irqpin_priv {
> + struct intc_irqpin_iomem iomem[INTC_IRQPIN_REG_NR];
> + struct intc_irqpin_irq irq[INTC_IRQPIN_MAX];
> + struct renesas_intc_irqpin_config config;
> + unsigned int number_of_irqs;
> + struct platform_device *pdev;
> + struct irq_chip irq_chip;
> + struct irq_domain *irq_domain;
> +};
(snip)
> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
> +
> +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
> + int reg, int shift,
> + int width, int value)
> +{
> + unsigned long flags;
> + unsigned long tmp;
> +
> + raw_spin_lock_irqsave(_irqpin_lock, flags);
> +
> + tmp = intc_irqpin_read(p, reg);
> + tmp &= ~(((1 << width) - 1) << shift);
> + tmp |= value << shift;
> + intc_irqpin_write(p, reg, tmp);
> +
> + raw_spin_unlock_irqrestore(_irqpin_lock, flags);
> +}
It is possible to include this spin lock into priv ?
This local static spin lock seems not wrong, but looks strange ?
> +static int intc_irqpin_probe(struct platform_device *pdev)
> +{
> + struct renesas_intc_irqpin_config *pdata = pdev->dev.platform_data;
> + struct intc_irqpin_priv *p;
> + struct intc_irqpin_iomem *i;
> + struct resource *io[INTC_IRQPIN_REG_NR];
> + struct resource *irq;
> + struct irq_chip *irq_chip;
> + void (*enable_fn)(struct irq_data *d);
> + void (*disable_fn)(struct irq_data *d);
> + const char *name = dev_name(>dev);
> + int ret;
> + int k;
> +
> + p = kzalloc(sizeof(*p), GFP_KERNEL);
can you use devm_kzalloc() ?
> + if (!p) {
> + dev_err(>dev, "failed to allocate driver data\n");
> + ret = -ENOMEM;
> + goto err0;
> + }
> +
> + /* deal with driver instance configuration */
> + if (pdata)
> + memcpy(>config, pdata, sizeof(*pdata));
> + if (!p->config.sense_bitfield_width)
> + p->config.sense_bitfield_width = 4; /* default to 4 bits */
> +
> + p->pdev = pdev;
> + platform_set_drvdata(pdev, p);
> +
> + /* get hold of manadatory IOMEM */
> + for (k = 0; k < INTC_IRQPIN_REG_NR; k++) {
> + io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k);
> + if (!io[k]) {
> + dev_err(>dev, "not enough IOMEM resources\n");
> + ret = -EINVAL;
> + goto err1;
> + }
> + }
> +
> + /* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
> + for (k = 0; k < INTC_IRQPIN_MAX; k++) {
> + irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
> + if (!irq)
> + break;
> +
> + p->irq[k].hw_irq = k;
> + p->irq[k].p = p;
> + p->irq[k].irq = irq->start;
> + }
> +
> + p->number_of_irqs = k;
> + if (p->number_of_irqs < 1) {
> + dev_err(>dev, "not enough IRQ resources\n");
> + ret = -EINVAL;
> + goto err1;
> + }
> +
> + /* ioremap IOMEM and setup read/write callbacks */
> + for (k = 0; k < INTC_IRQPIN_REG_NR; k++) {
> + i = >iomem[k];
> +
> + switch (resource_size(io[k])) {
> + case 1:
> + i->width = 8;
> + i->read = intc_irqpin_read8;
> + i->write = intc_irqpin_write8;
> + break;
> + case 4:
> + i->width = 32;
> + i->read = intc_irqpin_read32;
> + i->write = intc_irqpin_write32;
> + break;
> + default:
> + dev_err(>dev, "IOMEM size mismatch\n");
> + ret = -EINVAL;
> + goto err2;
> + }
> +
> + i->iomem = ioremap_nocache(io[k]->start, resource_size(io[k]));
devm_ioremap_nocache() or devm_request_and_ioremap()
> + if (!i->iomem) {
> + dev_err(>dev, "failed to remap IOMEM\n");
> + ret = -ENXIO;
> + goto err2;
> + }
> + }
> +
> + /* mask all interrupts using priority */
> + for (k = 0; k < p->number_of_irqs; k++)
> + intc_irqpin_mask_unmask_prio(p, k, 1);
> +
> + /* use more severe masking method if requested */
> + if (p->config.control_parent) {
> + enable_fn = intc_irqpin_irq_enable_force;
> + disable_fn = intc_irqpin_irq_disable_force;
> + } else {
> + enable_fn = intc_irqpin_irq_enable;
> + disable_fn = intc_irqpin_irq_disable;
> + }
> +
> + irq_chip = >irq_chip;
> + irq_chip->name = name;
> + irq_chip->irq_mask = disable_fn;
> + irq_chip->irq_unmask = enable_fn;
> + irq_chip->irq_enable = enable_fn;
> +
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: > From: Magnus Damm > > This patch adds a driver for external IRQ pins connected > to the INTC block on recent SoCs from Renesas. > > The INTC hardware block usually contains a rather wide > range of features ranging from external IRQ pin handling > to legacy interrupt controller support. On older SoCs > the INTC is used as a general purpose interrupt controller > both for external IRQ pins and on-chip devices. > > On more recent ARM based SoCs with Cortex-A9 the main > interrupt controller is the GIC, but IRQ trigger setup > still need to happen in the INTC hardware block. > > This driver implements the glue code needed to configure > IRQ trigger and also handle mask/unmask and demux of > external IRQ pins hooked up from the INTC to the GIC. > > Tested on sh73a0 and r8a7779. The hardware varies quite > a bit with SoC model, for instance register width and > bitfield widths vary wildly. The driver requires one GIC > SPI per external IRQ pin to operate. Each driver instance > will handle up to 8 external IRQ pins. > > The SoCs using this driver are currently mainly used > together with regular platform devices so this driver > allows configuration via platform data to support things > like static interrupt base address. DT support will > be added incrementally in the not so distant future. > > Signed-off-by: Magnus Damm Hi Magnus, Hi all, I do not expect this code to go through the renesas tree. However, in order to provide a basis for work on renesas SoCs I have added this patch to the topic/intc-external-irq topic branch in the reneas tree on kernel.org and merged it into topic/all+next. In other words, I am not picking this series up to merge it or add it to linux-next, rather I am storing it for reference. In the course of adding the branch I noticed a 3 whitespace warnings from git. I have highlighted them below. > --- > > drivers/irqchip/Kconfig |4 > drivers/irqchip/Makefile |1 > drivers/irqchip/irq-renesas-intc-irqpin.c | 464 > + > include/linux/platform_data/irq-renesas-intc-irqpin.h | 10 > 4 files changed, 479 insertions(+) > > --- 0001/drivers/irqchip/Kconfig > +++ work/drivers/irqchip/Kconfig 2013-02-18 18:28:22.0 +0900 > @@ -25,6 +25,10 @@ config ARM_VIC_NR > The maximum number of VICs available in the system, for > power management. > > +config RENESAS_INTC_IRQPIN > + bool > + select IRQ_DOMAIN > + > config VERSATILE_FPGA_IRQ > bool > select IRQ_DOMAIN > --- 0001/drivers/irqchip/Makefile > +++ work/drivers/irqchip/Makefile 2013-02-18 18:28:22.0 +0900 > @@ -5,4 +5,5 @@ obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi.o > obj-$(CONFIG_ARCH_SPEAR3XX) += spear-shirq.o > obj-$(CONFIG_ARM_GIC)+= irq-gic.o > obj-$(CONFIG_ARM_VIC)+= irq-vic.o > +obj-$(CONFIG_RENESAS_INTC_IRQPIN)+= irq-renesas-intc-irqpin.o > obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o > --- /dev/null > +++ work/drivers/irqchip/irq-renesas-intc-irqpin.c2013-02-18 > 21:06:32.0 +0900 > @@ -0,0 +1,464 @@ > +/* > + * Renesas INTC External IRQ Pin Driver > + * > + * Copyright (C) 2013 Magnus Damm > + * > + * 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; either version 2 of the License > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define INTC_IRQPIN_MAX 8 /* maximum 8 interrupts per driver instance */ > + > +#define INTC_IRQPIN_REG_SENSE 0 /* ICRn */ > +#define INTC_IRQPIN_REG_PRIO 1 /* INTPRInn */ > +#define INTC_IRQPIN_REG_SOURCE 2 /* INTREQnn */ > +#define INTC_IRQPIN_REG_MASK 3 /* INTMSKnn */ > +#define INTC_IRQPIN_REG_CLEAR 4 /* INTMSKCLRnn */ > +#define INTC_IRQPIN_REG_NR 5 > + > +/* INTC external IRQ PIN hardware register access: > + * > + * SENSE is read-write 32-bit with 2-bits or 4-bits per IRQ (*) > + * PRIO is read-write 32-bit with 4-bits per IRQ (**) > + * SOURCE is read-only 32-bit or 8-bit with 1-bit per IRQ (***) > + * MASK is write-only 32-bit or 8-bit with 1-bit per IRQ (***) > + * CLEAR is write-only 32-bit or 8-bit with 1-bit per IRQ (***) >
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
On Mon, Feb 18, 2013 at 11:28:34PM +0900, Magnus Damm wrote: From: Magnus Damm d...@opensource.se This patch adds a driver for external IRQ pins connected to the INTC block on recent SoCs from Renesas. The INTC hardware block usually contains a rather wide range of features ranging from external IRQ pin handling to legacy interrupt controller support. On older SoCs the INTC is used as a general purpose interrupt controller both for external IRQ pins and on-chip devices. On more recent ARM based SoCs with Cortex-A9 the main interrupt controller is the GIC, but IRQ trigger setup still need to happen in the INTC hardware block. This driver implements the glue code needed to configure IRQ trigger and also handle mask/unmask and demux of external IRQ pins hooked up from the INTC to the GIC. Tested on sh73a0 and r8a7779. The hardware varies quite a bit with SoC model, for instance register width and bitfield widths vary wildly. The driver requires one GIC SPI per external IRQ pin to operate. Each driver instance will handle up to 8 external IRQ pins. The SoCs using this driver are currently mainly used together with regular platform devices so this driver allows configuration via platform data to support things like static interrupt base address. DT support will be added incrementally in the not so distant future. Signed-off-by: Magnus Damm d...@opensource.se Hi Magnus, Hi all, I do not expect this code to go through the renesas tree. However, in order to provide a basis for work on renesas SoCs I have added this patch to the topic/intc-external-irq topic branch in the reneas tree on kernel.org and merged it into topic/all+next. In other words, I am not picking this series up to merge it or add it to linux-next, rather I am storing it for reference. In the course of adding the branch I noticed a 3 whitespace warnings from git. I have highlighted them below. --- drivers/irqchip/Kconfig |4 drivers/irqchip/Makefile |1 drivers/irqchip/irq-renesas-intc-irqpin.c | 464 + include/linux/platform_data/irq-renesas-intc-irqpin.h | 10 4 files changed, 479 insertions(+) --- 0001/drivers/irqchip/Kconfig +++ work/drivers/irqchip/Kconfig 2013-02-18 18:28:22.0 +0900 @@ -25,6 +25,10 @@ config ARM_VIC_NR The maximum number of VICs available in the system, for power management. +config RENESAS_INTC_IRQPIN + bool + select IRQ_DOMAIN + config VERSATILE_FPGA_IRQ bool select IRQ_DOMAIN --- 0001/drivers/irqchip/Makefile +++ work/drivers/irqchip/Makefile 2013-02-18 18:28:22.0 +0900 @@ -5,4 +5,5 @@ obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi.o obj-$(CONFIG_ARCH_SPEAR3XX) += spear-shirq.o obj-$(CONFIG_ARM_GIC)+= irq-gic.o obj-$(CONFIG_ARM_VIC)+= irq-vic.o +obj-$(CONFIG_RENESAS_INTC_IRQPIN)+= irq-renesas-intc-irqpin.o obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o --- /dev/null +++ work/drivers/irqchip/irq-renesas-intc-irqpin.c2013-02-18 21:06:32.0 +0900 @@ -0,0 +1,464 @@ +/* + * Renesas INTC External IRQ Pin Driver + * + * Copyright (C) 2013 Magnus Damm + * + * 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; either version 2 of the License + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#include linux/init.h +#include linux/platform_device.h +#include linux/spinlock.h +#include linux/interrupt.h +#include linux/ioport.h +#include linux/io.h +#include linux/irq.h +#include linux/irqdomain.h +#include linux/err.h +#include linux/slab.h +#include linux/module.h +#include linux/platform_data/irq-renesas-intc-irqpin.h + +#define INTC_IRQPIN_MAX 8 /* maximum 8 interrupts per driver instance */ + +#define INTC_IRQPIN_REG_SENSE 0 /* ICRn */ +#define INTC_IRQPIN_REG_PRIO 1 /* INTPRInn */ +#define INTC_IRQPIN_REG_SOURCE 2 /* INTREQnn */ +#define INTC_IRQPIN_REG_MASK 3 /* INTMSKnn */ +#define INTC_IRQPIN_REG_CLEAR 4 /* INTMSKCLRnn */ +#define INTC_IRQPIN_REG_NR 5 + +/* INTC external IRQ PIN hardware register access: + * + * SENSE is read-write 32-bit with 2-bits or 4-bits per IRQ (*) + * PRIO is read-write 32-bit with 4-bits per IRQ (**) + * SOURCE is read-only 32-bit or 8-bit with 1-bit per IRQ (***) + * MASK
Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver
Hi Magnus
Thank you for this patch.
Small comment from me :)
+struct intc_irqpin_priv {
+ struct intc_irqpin_iomem iomem[INTC_IRQPIN_REG_NR];
+ struct intc_irqpin_irq irq[INTC_IRQPIN_MAX];
+ struct renesas_intc_irqpin_config config;
+ unsigned int number_of_irqs;
+ struct platform_device *pdev;
+ struct irq_chip irq_chip;
+ struct irq_domain *irq_domain;
+};
(snip)
+static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */
+
+static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p,
+ int reg, int shift,
+ int width, int value)
+{
+ unsigned long flags;
+ unsigned long tmp;
+
+ raw_spin_lock_irqsave(intc_irqpin_lock, flags);
+
+ tmp = intc_irqpin_read(p, reg);
+ tmp = ~(((1 width) - 1) shift);
+ tmp |= value shift;
+ intc_irqpin_write(p, reg, tmp);
+
+ raw_spin_unlock_irqrestore(intc_irqpin_lock, flags);
+}
It is possible to include this spin lock into priv ?
This local static spin lock seems not wrong, but looks strange ?
+static int intc_irqpin_probe(struct platform_device *pdev)
+{
+ struct renesas_intc_irqpin_config *pdata = pdev-dev.platform_data;
+ struct intc_irqpin_priv *p;
+ struct intc_irqpin_iomem *i;
+ struct resource *io[INTC_IRQPIN_REG_NR];
+ struct resource *irq;
+ struct irq_chip *irq_chip;
+ void (*enable_fn)(struct irq_data *d);
+ void (*disable_fn)(struct irq_data *d);
+ const char *name = dev_name(pdev-dev);
+ int ret;
+ int k;
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
can you use devm_kzalloc() ?
+ if (!p) {
+ dev_err(pdev-dev, failed to allocate driver data\n);
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ /* deal with driver instance configuration */
+ if (pdata)
+ memcpy(p-config, pdata, sizeof(*pdata));
+ if (!p-config.sense_bitfield_width)
+ p-config.sense_bitfield_width = 4; /* default to 4 bits */
+
+ p-pdev = pdev;
+ platform_set_drvdata(pdev, p);
+
+ /* get hold of manadatory IOMEM */
+ for (k = 0; k INTC_IRQPIN_REG_NR; k++) {
+ io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k);
+ if (!io[k]) {
+ dev_err(pdev-dev, not enough IOMEM resources\n);
+ ret = -EINVAL;
+ goto err1;
+ }
+ }
+
+ /* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */
+ for (k = 0; k INTC_IRQPIN_MAX; k++) {
+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+ if (!irq)
+ break;
+
+ p-irq[k].hw_irq = k;
+ p-irq[k].p = p;
+ p-irq[k].irq = irq-start;
+ }
+
+ p-number_of_irqs = k;
+ if (p-number_of_irqs 1) {
+ dev_err(pdev-dev, not enough IRQ resources\n);
+ ret = -EINVAL;
+ goto err1;
+ }
+
+ /* ioremap IOMEM and setup read/write callbacks */
+ for (k = 0; k INTC_IRQPIN_REG_NR; k++) {
+ i = p-iomem[k];
+
+ switch (resource_size(io[k])) {
+ case 1:
+ i-width = 8;
+ i-read = intc_irqpin_read8;
+ i-write = intc_irqpin_write8;
+ break;
+ case 4:
+ i-width = 32;
+ i-read = intc_irqpin_read32;
+ i-write = intc_irqpin_write32;
+ break;
+ default:
+ dev_err(pdev-dev, IOMEM size mismatch\n);
+ ret = -EINVAL;
+ goto err2;
+ }
+
+ i-iomem = ioremap_nocache(io[k]-start, resource_size(io[k]));
devm_ioremap_nocache() or devm_request_and_ioremap()
+ if (!i-iomem) {
+ dev_err(pdev-dev, failed to remap IOMEM\n);
+ ret = -ENXIO;
+ goto err2;
+ }
+ }
+
+ /* mask all interrupts using priority */
+ for (k = 0; k p-number_of_irqs; k++)
+ intc_irqpin_mask_unmask_prio(p, k, 1);
+
+ /* use more severe masking method if requested */
+ if (p-config.control_parent) {
+ enable_fn = intc_irqpin_irq_enable_force;
+ disable_fn = intc_irqpin_irq_disable_force;
+ } else {
+ enable_fn = intc_irqpin_irq_enable;
+ disable_fn = intc_irqpin_irq_disable;
+ }
+
+ irq_chip = p-irq_chip;
+ irq_chip-name = name;
+ irq_chip-irq_mask = disable_fn;
+ irq_chip-irq_unmask = enable_fn;
+ irq_chip-irq_enable = enable_fn;
+ irq_chip-irq_disable = disable_fn;
+ irq_chip-irq_set_type = intc_irqpin_irq_set_type;
+ irq_chip-flags = IRQCHIP_SKIP_SET_WAKE;
+
+
