[Sorry, resend to include all maintainers.]
This series add support for mediatek GIC interrupt polarity extension.
Several components in mediatek SoC have low level triggered interrupt and
require this support.
This also correct 6589 timer irq polarity. Previous version works because
6589 boot loader already set correct polarity for timer interrupt.
The patch set is based on Matthias's Mediatek basic support for v3.17 [1].
v2:
- Make mt6589.dtsi changes as a separate commit as Matthias suggest.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272561.html
Joe.C (4):
irqchip: gic: Change irq type check when extension is
arm: mediatek: Add support for GIC interrupt polarity
arm: mediatek: Add intpol in mt6589.dtsi
dt-bindings: add bindings for mediatek intpol
Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt | 16 ++
arch/arm/boot/dts/mt6589.dtsi | 7 -
arch/arm/mach-mediatek/Makefile | 2
arch/arm/mach-mediatek/common.h | 19 +++
arch/arm/mach-mediatek/intpol.c | 61 ++++++++++
arch/arm/mach-mediatek/mediatek.c | 10 +
drivers/irqchip/irq-gic.c | 27 ++--
7 files changed, 131 insertions(+), 11 deletions(-)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
create mode 100644 arch/arm/mach-mediatek/common.h
create mode 100644 arch/arm/mach-mediatek/intpol.c
From: "Joe.C" <redacted>
GIC supports the combination with external extensions. But this
is not reflected in the checks of the interrupt type flag.
This patch allows interrupt types other than the one supported by GIC,
if an architecture extension is present and supports them.
Signed-off-by: Joe.C <redacted>
---
drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
@@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)u32confoff=(gicirq/16)*4;boolenabled=false;u32val;+intret=0;/* Interrupt configuration for SGIs can't be changed */if(gicirq<16)return-EINVAL;-if(type!=IRQ_TYPE_LEVEL_HIGH&&type!=IRQ_TYPE_EDGE_RISING)-return-EINVAL;-raw_spin_lock(&irq_controller_lock);-if(gic_arch_extn.irq_set_type)-gic_arch_extn.irq_set_type(d,type);+if(gic_arch_extn.irq_set_type){+ret=gic_arch_extn.irq_set_type(d,type);+if(ret)+gotoout;+}elseif(type!=IRQ_TYPE_LEVEL_HIGH&&+type!=IRQ_TYPE_EDGE_RISING){+ret=-EINVAL;+gotoout;+}val=readl_relaxed(base+GIC_DIST_CONFIG+confoff);-if(type==IRQ_TYPE_LEVEL_HIGH)+/* Check for both edge and level here, so we can support GIC irq+polarityextensioningic_arch_extn.irq_set_type.Ifarch+doesn'tsupportpolarityextension,thecheckabovewillreject+impropertype.*/+if(type&IRQ_TYPE_LEVEL_MASK)val&=~confmask;-elseif(type==IRQ_TYPE_EDGE_RISING)+elseif(type&IRQ_TYPE_EDGE_BOTH)val|=confmask;/*
@@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type)if(enabled)writel_relaxed(enablemask,base+GIC_DIST_ENABLE_SET+enableoff);-+out:raw_spin_unlock(&irq_controller_lock);-return0;+returnret;}staticintgic_retrigger(structirq_data*d)
@@ -0,0 +1,61 @@+/*+*Thisfilecontainscommoncodethatisintendedtobeusedacross+*boardssothatit'snotreplicated.+*+*Copyright(C)2014MediatekInc.+*+*ThissoftwareislicensedunderthetermsoftheGNUGeneralPublic+*Licenseversion2,aspublishedbytheFreeSoftwareFoundation,and+*maybecopied,distributed,andmodifiedunderthoseterms.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,+*butWITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.Seethe+*GNUGeneralPublicLicenseformoredetails.+*/+#include<linux/of_address.h>+#include<linux/io.h>+#include<linux/irq.h>+#include<linux/irqchip/arm-gic.h>++#define GIC_HW_IRQ_BASE 32+#define INT_POL_INDEX(a) ((a) - GIC_HW_IRQ_BASE)++staticvoid__iomem*int_pol_base;++staticintmtk_int_pol_set_type(structirq_data*d,unsignedinttype)+{+unsignedintirq=d->hwirq;+u32offset,reg_index,value;++offset=INT_POL_INDEX(irq)&0x1F;+reg_index=INT_POL_INDEX(irq)>>5;++/* This arch extension was called with irq_controller_lock held,+sotheread-modify-writewillbeatomic*/+value=readl(int_pol_base+reg_index*4);+if(type==IRQ_TYPE_LEVEL_LOW||type==IRQ_TYPE_EDGE_FALLING)+value|=(1<<offset);+else+value&=~(1<<offset);+writel(value,int_pol_base+reg_index*4);++return0;+}++voidinit_intpol(void)+{+structdevice_node*node;++node=of_find_compatible_node(NULL,NULL,"mediatek,mt6577-intpol");+if(!node)+return;++int_pol_base=of_io_request_and_map(node,0,"intpol");+if(IS_ERR(int_pol_base)){+pr_warn("Can't get resource\n");+return;+}++gic_arch_extn.irq_set_type=mtk_int_pol_set_type;+}
From: "Joe.C" <redacted>
Add intpol settings for mt6589.
This also correct timer interrupt flag setting. The old setting
works because 6589 boot loader already set polarity for time
interrupt. Without intpol support, the setting was not changed
so gic can get the irq correctly.
Signed-off-by: Joe.C <redacted>
---
arch/arm/boot/dts/mt6589.dtsi | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
@@ -0,0 +1,16 @@+Mediatek 65xx/81xx GIC interrupt polarity extension++Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,+these can be used as GIC interrupt polarity extension.++Required properties:+- compatible: Compatible property value should be "mediatek,mt6577-intpol"++- reg: Physical base address of the int pol registers and length of memory+ mapped region.++Example:+ intpol: intpol at 10200100 {+ compatible = "mediatek,mt6577-intpol";+ reg = <0x10200100 0x1c>;+ };
I think the place for the file should be in
Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt as
it is a interrupt-controller extension of the mediatek architecture.
@@ -0,0 +1,16 @@+Mediatek 65xx/81xx GIC interrupt polarity extension++Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,+these can be used as GIC interrupt polarity extension.++Required properties:+- compatible: Compatible property value should be "mediatek,mt6577-intpol"++- reg: Physical base address of the int pol registers and length of memory+ mapped region.++Example:+ intpol: intpol at 10200100 {+ compatible = "mediatek,mt6577-intpol";+ reg = <0x10200100 0x1c>;+ };--
Hi, Rob,
Would you help to review this DT bindings and give us some
suggestions? Matthias suggests moving this to
Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt
I think that's a better place for this. What do you think?
Joe.C
On Thu, 2014-08-21 at 17:02 +0200, Matthias Brugger wrote:
I think the place for the file should be in
Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt as
it is a interrupt-controller extension of the mediatek architecture.
@@ -0,0 +1,16 @@+Mediatek 65xx/81xx GIC interrupt polarity extension++Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,+these can be used as GIC interrupt polarity extension.++Required properties:+- compatible: Compatible property value should be "mediatek,mt6577-intpol"++- reg: Physical base address of the int pol registers and length of memory+ mapped region.++Example:+ intpol: intpol at 10200100 {+ compatible = "mediatek,mt6577-intpol";+ reg = <0x10200100 0x1c>;+ };--
Hi, ARM soc maintainers,
Please help to review this series, I think this should go through
ARM soc tree.
We plan to upstream drivers for MT8135. MT8135 is a big/little soc
featuring 2 CA7 + 2 CA15, and sharing many similar IP components with
mt65xx. Many components of 65xx series & 8135 require this support.
Joe.C
On Wed, 2014-08-13 at 10:11 +0800, Joe.C wrote:
[Sorry, resend to include all maintainers.]
This series add support for mediatek GIC interrupt polarity extension.
Several components in mediatek SoC have low level triggered interrupt and
require this support.
This also correct 6589 timer irq polarity. Previous version works because
6589 boot loader already set correct polarity for timer interrupt.
The patch set is based on Matthias's Mediatek basic support for v3.17 [1].
v2:
- Make mt6589.dtsi changes as a separate commit as Matthias suggest.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272561.html
Joe.C (4):
irqchip: gic: Change irq type check when extension is
arm: mediatek: Add support for GIC interrupt polarity
arm: mediatek: Add intpol in mt6589.dtsi
dt-bindings: add bindings for mediatek intpol
Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt | 16 ++
arch/arm/boot/dts/mt6589.dtsi | 7 -
arch/arm/mach-mediatek/Makefile | 2
arch/arm/mach-mediatek/common.h | 19 +++
arch/arm/mach-mediatek/intpol.c | 61 ++++++++++
arch/arm/mach-mediatek/mediatek.c | 10 +
drivers/irqchip/irq-gic.c | 27 ++--
7 files changed, 131 insertions(+), 11 deletions(-)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
create mode 100644 arch/arm/mach-mediatek/common.h
create mode 100644 arch/arm/mach-mediatek/intpol.c
From: Marc Zyngier <hidden> Date: 2014-08-22 11:09:48
Hi Joe,
On 13/08/14 03:11, Joe.C wrote:
quoted hunk
From: "Joe.C" <redacted>
GIC supports the combination with external extensions. But this
is not reflected in the checks of the interrupt type flag.
This patch allows interrupt types other than the one supported by GIC,
if an architecture extension is present and supports them.
Signed-off-by: Joe.C <redacted>
---
drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
@@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)u32confoff=(gicirq/16)*4;boolenabled=false;u32val;+intret=0;/* Interrupt configuration for SGIs can't be changed */if(gicirq<16)return-EINVAL;-if(type!=IRQ_TYPE_LEVEL_HIGH&&type!=IRQ_TYPE_EDGE_RISING)-return-EINVAL;-raw_spin_lock(&irq_controller_lock);-if(gic_arch_extn.irq_set_type)-gic_arch_extn.irq_set_type(d,type);+if(gic_arch_extn.irq_set_type){+ret=gic_arch_extn.irq_set_type(d,type);+if(ret)+gotoout;+}elseif(type!=IRQ_TYPE_LEVEL_HIGH&&+type!=IRQ_TYPE_EDGE_RISING){+ret=-EINVAL;+gotoout;+}val=readl_relaxed(base+GIC_DIST_CONFIG+confoff);-if(type==IRQ_TYPE_LEVEL_HIGH)+/* Check for both edge and level here, so we can support GIC irq+polarityextensioningic_arch_extn.irq_set_type.Ifarch+doesn'tsupportpolarityextension,thecheckabovewillreject+impropertype.*/+if(type&IRQ_TYPE_LEVEL_MASK)val&=~confmask;-elseif(type==IRQ_TYPE_EDGE_RISING)+elseif(type&IRQ_TYPE_EDGE_BOTH)val|=confmask;/*
@@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type)if(enabled)writel_relaxed(enablemask,base+GIC_DIST_ENABLE_SET+enableoff);-+out:raw_spin_unlock(&irq_controller_lock);-return0;+returnret;}staticintgic_retrigger(structirq_data*d)
You're really abusing the gic_arch_extn feature. I know this is
tempting, but this is pushing it a bit too far.
This feature exist for one particular reason: if your GIC is in the same
power-domain as the CPUs, it will go down as well when you suspend the
system, hence being enable to wake the CPU up. You then need a shadow
interrupt controller to take over. This is exactly why we call the hook
on every GIC-related operation.
Here, you're using it to program something that sits between the device
and the GIC. This is a separate block, with its own hardware
configuration, that modifies the interrupt signal. This should be
reflected in the device-tree and the code paths.
You can probably model this as a separate irqchip for the few interrupts
that require this, or have it configured at boot time (assuming the
configuration never changes).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Hi,
Thanks for your suggestions.
On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
Hi Joe,
On 13/08/14 03:11, Joe.C wrote:
quoted
From: "Joe.C" <redacted>
GIC supports the combination with external extensions. But this
is not reflected in the checks of the interrupt type flag.
This patch allows interrupt types other than the one supported by GIC,
if an architecture extension is present and supports them.
Signed-off-by: Joe.C <redacted>
---
drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
@@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)u32confoff=(gicirq/16)*4;boolenabled=false;u32val;+intret=0;/* Interrupt configuration for SGIs can't be changed */if(gicirq<16)return-EINVAL;-if(type!=IRQ_TYPE_LEVEL_HIGH&&type!=IRQ_TYPE_EDGE_RISING)-return-EINVAL;-raw_spin_lock(&irq_controller_lock);-if(gic_arch_extn.irq_set_type)-gic_arch_extn.irq_set_type(d,type);+if(gic_arch_extn.irq_set_type){+ret=gic_arch_extn.irq_set_type(d,type);+if(ret)+gotoout;+}elseif(type!=IRQ_TYPE_LEVEL_HIGH&&+type!=IRQ_TYPE_EDGE_RISING){+ret=-EINVAL;+gotoout;+}val=readl_relaxed(base+GIC_DIST_CONFIG+confoff);-if(type==IRQ_TYPE_LEVEL_HIGH)+/* Check for both edge and level here, so we can support GIC irq+polarityextensioningic_arch_extn.irq_set_type.Ifarch+doesn'tsupportpolarityextension,thecheckabovewillreject+impropertype.*/+if(type&IRQ_TYPE_LEVEL_MASK)val&=~confmask;-elseif(type==IRQ_TYPE_EDGE_RISING)+elseif(type&IRQ_TYPE_EDGE_BOTH)val|=confmask;/*
@@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type)if(enabled)writel_relaxed(enablemask,base+GIC_DIST_ENABLE_SET+enableoff);-+out:raw_spin_unlock(&irq_controller_lock);-return0;+returnret;}staticintgic_retrigger(structirq_data*d)
You're really abusing the gic_arch_extn feature. I know this is
tempting, but this is pushing it a bit too far.
This feature exist for one particular reason: if your GIC is in the same
power-domain as the CPUs, it will go down as well when you suspend the
system, hence being enable to wake the CPU up. You then need a shadow
interrupt controller to take over. This is exactly why we call the hook
on every GIC-related operation.
Actually we are doing this too, it is called SYS_CIRQ in our IC, and
we'll need to support that in the future. This intpol is part of CIRQ
function block. Does it make more senses if I add skeleton for CIRQ
support, and implement intpol inside it?
Here, you're using it to program something that sits between the device
and the GIC. This is a separate block, with its own hardware
configuration, that modifies the interrupt signal. This should be
reflected in the device-tree and the code paths.
You can probably model this as a separate irqchip for the few interrupts
that require this, or have it configured at boot time (assuming the
configuration never changes).
The boot loader did setup interrupt polarity for those used in boot
loader, but not all of them.
Datasheet lists components irqs as low active, so I think it makes more
sense to allow driver to use IRQF_TRIGGER_LOW instead of having them to
notice GIC only support high active and use IRQF_TRIGGER_HIGH. This
rule out configure it at boot loader or kernel init irq time.
If I implement this as a separate irqchip, I need to reuse most gic
irqchip functions. Without changing irq-gic.c to make them global,
I can only think of hack like this:
gic_init_bases(..) /* init gic */
gic_chip = irq_get_chip(0); /* to get gic irq_chip */
org_gic_set_type = gic_chip->irq_set_type;
gic_chip->irq_set_type = mt_irq_polarity_set_type;
and calling original gic_set_type from mt_irq_polarity_set_type with irq
type fixup.
Joe.C
From: Jan Lübbe <jlu@pengutronix.de> Date: 2014-08-27 09:57:14
Marc,
On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
Here, you're using it to program something that sits between the
device and the GIC. This is a separate block, with its own hardware
configuration, that modifies the interrupt signal. This should be
reflected in the device-tree and the code paths.
You can probably model this as a separate irqchip for the few
interrupts that require this, or have it configured at boot time
(assuming the configuration never changes).
It seems to me that using a separate irqchip for a simple inverter would
add the run-time overhead of passing through wrapper functions on every
IRQ. Do you have an idea how this could be avoided without using the
gic_arch_extn feature?
As in the DT the actual IRQ polarity should be used, simply configuring
the HW IRQ polarity in the bootloader is not enough without telling the
GIC driver which polarity is supported on which IRQ, right?
Regards,
Jan
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
From: Marc Zyngier <hidden> Date: 2014-08-27 10:36:42
Hi Jan,
On 27/08/14 10:55, Jan L?bbe wrote:
Marc,
On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
quoted
Here, you're using it to program something that sits between the
device and the GIC. This is a separate block, with its own hardware
configuration, that modifies the interrupt signal. This should be
reflected in the device-tree and the code paths.
You can probably model this as a separate irqchip for the few
interrupts that require this, or have it configured at boot time
(assuming the configuration never changes).
It seems to me that using a separate irqchip for a simple inverter would
add the run-time overhead of passing through wrapper functions on every
IRQ. Do you have an idea how this could be avoided without using the
gic_arch_extn feature?
Well, from the rather vague description, it could be slightly more than
a simple inverter, like being able to generate interrupts on both rising
and falling edges. Sorry, but this is not the GIC as ARM has architected it.
Yes, the additional irqchip adds some overhead. But the DT has to
reflect the fact that there is something on the interrupt path that does
some form of conversion.
As in the DT the actual IRQ polarity should be used, simply configuring
the HW IRQ polarity in the bootloader is not enough without telling the
GIC driver which polarity is supported on which IRQ, right?
Looking a bit closer at things, what you describe in DT is the IRQ
polarity the interrupt controller sees. Nothing else should interpret
that field.
So it is legal (IMO) to have a device with an interrupt specifier
describing a rising edge interrupt, and yet have the device generating a
falling edge, with Mediatek's special sauce doing the conversion in between.
Something will have to configure the polarity widget though, but that
can be left outside of the GIC.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
From: Thomas Gleixner <hidden> Date: 2014-08-27 12:24:31
On Wed, 27 Aug 2014, Marc Zyngier wrote:
quoted
As in the DT the actual IRQ polarity should be used, simply configuring
the HW IRQ polarity in the bootloader is not enough without telling the
GIC driver which polarity is supported on which IRQ, right?
Looking a bit closer at things, what you describe in DT is the IRQ
polarity the interrupt controller sees. Nothing else should interpret
that field.
So it is legal (IMO) to have a device with an interrupt specifier
describing a rising edge interrupt, and yet have the device generating a
falling edge, with Mediatek's special sauce doing the conversion in between.
Something will have to configure the polarity widget though, but that
can be left outside of the GIC.
This seems to become a popular topic and it looks like the whole GIC
extension thing is going to explode sooner than later.
We are currently discussing hierarchical irq domains to solve a
different issue in x86 land. See the related discussion here:
https://lkml.org/lkml/2014/8/1/67
Now looking at these GIC plus extra sauce problems, I wonder whether
this wouldn't be solvable in a similar way. If you look at it from the
HW perspective you have:
--------- ---------
---| MAGIC |------|ARM GIC|
---| |------| |
---| |------| |
---| |------| |
---| |------| |
--------- ---------
The MAGIC interrupt controller only provides functionality which is
not available from the ARM architected GIC but maps 1:1 to the ARM GIC
interrupt lines. So it looks like a variation to the more dynamic
mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86.
The idea is to have two irq domains: magic_domain and armgic_domain.
The magic_domain is the frontend facing the devices and the
armgic_domain is the parent domain. This is also reflected in
hierarchical data structures, i.e. irq_desc->irq_data will get a new
field parent_data, which points to the irq_data of the parent
interrupt controller, which is allocated separately when the interrupt
line is instantiated.
So in the above case the hotpath ack/eoi functions of the irq chip
associated to the device interrupt, i.e. magic_chip, would do:
irq_ack(struct irq_data *d)
{
struct irq_data *pd = d->parent_data;
pd->chip->irq_ack(pd);
}
Granted, that's another level of indirection, but this is going to be
more efficient than a boatload of conditional hooks in the GIC code
proper. Not to talk about the maintainability of the whole maze.
The irq_set_type() function would do:
irq_set_type(struct irq_data *d, int type)
{
struct irq_data *pd = d->parent_data;
gic_type = set_magic_chip_type(d, type);
return pd->chip->irq_set_type(d, gic_type);
}
Switching to this allows to completely avoid the gazillion of hooks in
the gic code and should work nicely with multiplatform kernels by
simpling hooking up the domain parent relation ship to the proper
magic domain or leave the armgic as the direct device interface in
case the SoC does not have the magic chip in front of the arm gic.
Thoughts?
Thanks,
tglx