Re: [RESEND PATCH v2 4/4] dt-bindings: add bindings for mediatek intpol

Re: [RESEND PATCH v2 4/4] dt-bindings: add bindings for mediatek intpol

[Matthias Brugger]

2014-08-13 4:11 GMT+02:00 Joe.C <srv_yingjoe.chen@mediatek.com>:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
>
> Add binding documentation for Mediatek SoC GIC interrupt polarity extension.
>
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  .../bindings/interrupt-controller/mediatek,intpol.txt    | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt

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.

>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> new file mode 100644
> index 0000000..16ea372
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> @@ -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@10200100 {
> +               compatible = "mediatek,mt6577-intpol";
> +               reg = <0x10200100 0x1c>;
> +       };
> --
> 1.8.1.1.dirty
>



-- 
motzblog.wordpress.com

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Marc Zyngier]

Hi Joe,

On 13/08/14 03:11, Joe.C wrote:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
> 
> 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 <yingjoe.chen@mediatek.com>
> ---
>  drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 57d165e..66485ab 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  	u32 confoff = (gicirq / 16) * 4;
>  	bool enabled = false;
>  	u32 val;
> +	int ret = 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)
> +			goto out;
> +	} else if (type != IRQ_TYPE_LEVEL_HIGH &&
> +		type != IRQ_TYPE_EDGE_RISING) {
> +			ret = -EINVAL;
> +			goto out;
> +	}
>  
>  	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
> +	   polarity extension in gic_arch_extn.irq_set_type. If arch
> +	   doesn't support polarity extension, the check above will reject
> +	   improper type. */
> +	if (type & IRQ_TYPE_LEVEL_MASK)
>  		val &= ~confmask;
> -	else if (type == IRQ_TYPE_EDGE_RISING)
> +	else if (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);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int gic_retrigger(struct irq_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...

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Jan Lübbe]

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 |

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Marc Zyngier]

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:
>> 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...

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Marc Zyngier wrote:
> > 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

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Marc Zyngier wrote:
> I very much like that kind of approach. Stacking domains seems to solve
> a number of issues at once:
> 
> - NVIDIA's gic extension
> - TI's crossbar
> - ARM's GICv2m
> - Mediatek's glorified line inverter
> - ... and probably the next madness that's going to land tomorrow

Its probably already there you just don't know about it yet :)
 
> I haven't completly groked the way we're going to allocate domains and
> irq_data structures, but this is definitely something worth

All we have for now is a rough idea and some pseudo code in my lengthy
reply to Jiang, but it shouldn't be hard to implement something
useful.

Thanks,

	tglx

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Mark Rutland]

On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote:
> [Adding Jiang Liu to the spam-fest]
> 
> Hi Thomas,
> 
> On Wed, Aug 27 2014 at  1:23:48 pm BST, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Wed, 27 Aug 2014, Marc Zyngier wrote:
> >> > 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
> 
> Ah, very interesting. Thanks for pointing that out.
> 
> > 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?
> 
> I very much like that kind of approach. Stacking domains seems to solve
> a number of issues at once:
> 
> - NVIDIA's gic extension
> - TI's crossbar
> - ARM's GICv2m
> - Mediatek's glorified line inverter
> - ... and probably the next madness that's going to land tomorrow
> 
> I haven't completly groked the way we're going to allocate domains and
> irq_data structures, but this is definitely something worth
> investigating. I'm not too worried about the indirection either - at
> least we end up with maintainable code.
> 
> We need to work out how to drive the whole stacking from a DT
> perspective: Mark, any idea?

Describing the lines the magic irqchip has to its parent irqchip is
simple, the standard interrupts property can handle that:

parent: parent {
	...
	#interrupt-cells = <1>;
};

magic {
	...
	interrupt-parent = <&parent>;
	interrupts = <3>, <6>, <17>, <2>;
	#interrupt-cells = <1>;
};

The harder part is describing the configuration of each interrupt to the
magic irqchip (e.g. edge vs level triggered), which is inseparable form
the line identifier in the interrupt-specifier.

If there interrupts don't share the same configuration then I'm not sure
how we describe that in the DT. That's especially problematic if the
assignment of parent line is dynamic (as with the crossbar iirc).

Mark.

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Mark Rutland wrote:
> On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote:
> > We need to work out how to drive the whole stacking from a DT
> > perspective: Mark, any idea?
> 
> Describing the lines the magic irqchip has to its parent irqchip is
> simple, the standard interrupts property can handle that:
> 
> parent: parent {
> 	...
> 	#interrupt-cells = <1>;
> };
> 
> magic {
> 	...
> 	interrupt-parent = <&parent>;
> 	interrupts = <3>, <6>, <17>, <2>;
> 	#interrupt-cells = <1>;
> };
> 
> The harder part is describing the configuration of each interrupt to the
> magic irqchip (e.g. edge vs level triggered), which is inseparable form
> the line identifier in the interrupt-specifier.

Do we really need to decribe that at the this level?

You have the parent ARMGIC and the maGIC with a specified (or even
dynamic) routing of the maGIC lines to the ARMGIC.

Now the device which uses a maGIC interrupt specifies the interrupt
type at the maGIC.

So the type setter function of the maGIC configures the maGIC hardware
in such a way that the output to the ARMGIC results in a type which
the ARMGIC can handle and calls the type setter function of the maGIC
with that type.

I don't think you need to decribe this in the magic/parent relation
ship at all. maGIC should know what kind of output it can provide to
ARMGIC so that should just work, unless I'm missing something.

> If there interrupts don't share the same configuration then I'm not sure
> how we describe that in the DT. That's especially problematic if the
> assignment of parent line is dynamic (as with the crossbar iirc).

Why is this an issue?

There are two ways to solve that:

1) You can do a fully dynamic allocation at the parent level, which of
   course requires that the whole parent range is dynamically
   managed. That's what we are planning for the MSI/Remap/Vector
   stacking

2) You define the irq lines at the parent which are available for
   dynamic stacking and let the allocation code hand one out.

3) You tell the maGIC controller which lines of the parent are
   available for it, so it can only stack onto those lines and instead
   of letting the parent allocate a free one, the maGIC needs to map
   its stuff to one of those ARMGIC lines.

Which one to chose depends on the particular maGIC incarnation, but
its not a big issue to select one ...

Thanks,

	tglx