> From: Lucas Stach [mailto:l.stach@xxxxxxxxxxxxxx] > Sent: Tuesday, January 22, 2019 6:59 PM > > Am Dienstag, den 22.01.2019, 10:39 +0000 schrieb Aisheng Dong: > > > > > From: Lucas Stach [mailto:l.stach@xxxxxxxxxxxxxx] > > > 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 > > If this is always in multiples of 32, the only change we need to make to the > driver is to fix DT binding and interpretation of the "fsl,irq-groups" property to > be in multiples of 32. > > This means i.MX8MQ DCSS irqsteer would need to change to 2 irq-groups, but > as this isn't used upstream yet we can still do this change without breaking too > much stuff and I would rather correct this now than keeping a DT binding > around that doesn't match the HW. > We want to avoid using of irq-groups as it's wrong. Stick to HW parameters, only channel number and interrupts number should be used. > > 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. > > I suppose that the OR group size at that point is always 64 input IRQs per > output IRQ, right? So with NINT32 == 1 you end up with 1 output IRQ, but for > NINT32 == 3 you get 2 output IRQs, correct? Yes, that's right. > > > > 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. > > Let's do it right with the first patch. This doesn't seem like a big change. > We can do it. > > > > > > > > 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. > > With the current binding, what keeps us from describing such a multi- channel > irqsteer with multiple DT nodes and have multiple driver instances? I don't see > why we would need to mix this all into one driver instance. > So for your above > example, something like: > > interrupt-controller@32e2d000 { > compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";> > reg = <0x32e2d000 0x1000>; > interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>; > fsl,channel = <0>; > }; > > interrupt-controller@32e2e000 { > compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";> > reg = <0x32e2e000 0x1000>; > interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>; > fsl,channel = <1>; > }; > Because from HW point of view, it IS actually one IRQSTEER module with multi channels supported. So I feel describe each channel into several nodes seems violate the HW a bit. That why I made the former dts binding as an example. Another point is that there's only one physical CHANCTL register shared with multi channels. However, each channel seems use a mirror CAHNCTRL register in its separate register space to enable the channel. But needs care about overwrite others. (Got this information after discussing with IC guys, still not verified) > > 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. > > The thing is, if we want to even try to keep DT stability we need to understand > how this HW block can be used and how we can describe this in the DT. > Yes, that's right. But the binding is already there, so we can fix them one by one without breaking the stability. Regards Dong Aisheng > Regards, > Lucas
