RE: [PATCH 4/4] irq: imx: irqsteer: add multi output interrupts support

From: Aisheng Dong <aisheng.dong@nxp.com>
To: Lucas Stach <l.stach@pengutronix.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	Marc Zyngier <marc.zyngier@arm.com>
Subject: RE: [PATCH 4/4] irq: imx: irqsteer: add multi output interrupts support
Date: Tue, 22 Jan 2019 10:39:11 +0000	[thread overview]
Message-ID: <AM0PR04MB42111FE266342A2C2EAECC3580980@AM0PR04MB4211.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <1547806960.2626.9.camel@pengutronix.de>

> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Friday, January 18, 2019 6:23 PM
[...]
> > > This has been discussed when upstreaming the driver. The controller
> > > may support multiple output IRQs, but only one them is actually used
> > > depending on the CHANCTRL config. There is no use in hooking up all
> > > the output IRQs in DT, if only one of them is actually used. Some of
> > > the outputs may not even be visible to the Linux system, but may
> > > belong to a Cortex M4 subsystem. All of those configurations can be
> > > described in DT by changing the upstream interrupt and "fsl,channel" in a
> coherent way.
> > >
> > > Please correct me if my understanding is totally wrong.
> >
> > I'm afraid your understanding of CHAN seems wrong.
> > (Binding doc of that property needs change as well).
> >
> > On QXP DC SS, the IRQSTEER supports 512 interrupts with 8 interrupt
> > output Conntected to GIC.
> > The current driver does not support it as it assumes only one interrupt
> output used.
> 
> Okay, so let's take a step back. The description in the QXP RM is actually better
> than what I've seen until now. Still it's totally confusing that the "channel"
> terminology used with different meanings in docs. Let's try to avoid this as
> much as possible.
> 
> So to get things straight: Each irqsteer controller has a number of IRQ groups.
> All the input IRQs of one group are ORed together to form on output IRQ.
> Depending on the SoC integration, a group can contain 32 or
> 64 IRQs, where DCSS irqsteer on MX8M and the big 512 input controllers on
> QXP and QM both use 64 IRQs per group. You are claiming that the smaller
> controllers on both QXP am QM have only 32 IRQs per group, right?
> 
> So the only change that is needed is that the driver needs to know the number
> of input IRQs per group, with a default of 64 to not break DT compatibility.
> 

Not exactly.
from HW point of view , there're two parameters during IRQSTEER integration.
For example,
DC in QXP:
parameter  IRQCHAN		=  1; 	//Number of IRQ Channels/Slots
parameter  NINT32		=  8;	//Number of interrupts in multiple of 32

MIPI CSI in MQ:
Parameter  IRQCHAN		= 1
Parameter  NINT32		= 1

You will see no group concept used here. Only channel number and interrupts number.
The group is an IP internal concept that ORed a group of 64 interrupts into an output
interrupt. But it may also only use 32 interrupts in the same group.

> Also if the connection between IRQ group and output IRQ is fixed, the driver
> should be more clever about handling the chained IRQ. If you know which of
> the upstream IRQs fired you only need to look at the 32 or 64 IRQ status
> registers of that specific group, not all of them.

Yes, that's right.
I planned to do that later with a separate patch before.

> 
> Can you please clarify what the CHANCTRL setting changes in this setup?
> 

IRQsteer supports up to 5 separate CAHNNELS which each of them supports up
to 512 interrupts. CHANCTL is used to enable those respective CHAN output interrupts.
e.g.
1~8 output interrupts of CHAN0.

One notable thing is the each channel has a separate address space.
That means the chan1 reg address is not the one we specified in default reg property.
So the correct dts may be like for multi channels cases.
interrupt-controller@32e2d000 {
        compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";
        reg = <0x32e2d000 0x1000>,
              <0x32e2e000 0x1000>,
              <0x32e2f000 0x1000>;
              ...
        reg-names = "ch0", "ch1", "ch2", ...;
        interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
        fsl,irqs-per-chan= <64>;
        interrupt-controller;
        #interrupt-cells = <2>; //cell 0: chan index cell 2: interrupt number
};
This makes the things quite complicated.

In reality, we still don't have such using cases so far as as multi channels usually
are used to deliver the interrupts to different cores,
e.g. M4, SCU, or DSP, A core don't handle it.
So I did not change it currently as it's another story.
This patch series mainly aims to add support for 32 or 512 interrupts channel and multiple
Outputs for a single CHANNEL case.

Regards
Dong Aisheng

> Regards,
> Lucas