Re: [PATCH v2] dt-bindings: interrupt-controller: loongson,liointc: Fix warnings about liointc-2.0

[Huacai Chen]

Hi, Jonas,

On Wed, Oct 18, 2023 at 5:38 PM Jonas Gorski <jonas.gorski@xxxxxxxxx> wrote:
>
> On Wed, 18 Oct 2023 at 08:58, Binbin Zhou <zhoubb.aaron@xxxxxxxxx> wrote:
> >
> > On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <jonas.gorski@xxxxxxxxx> wrote:
> > >
> > > On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <zhoubb.aaron@xxxxxxxxx> wrote:
> > > >
> > > > Hi all:
> > > >
> > > > Sorry, it's been a while since the last discussion.
> > > >
> > > > Previously, Krzysztof suggested using the standard "interrupt-map"
> > > > attribute instead of the "loongson,parent_int_map" attribute, which I
> > > > tried to implement, but the downside of this approach seems to be
> > > > obvious.
> > > >
> > > > First of all, let me explain again the interrupt routing of the
> > > > loongson liointc.
> > > > For example, the Loongson-2K1000 has 64 interrupt sources, each with
> > > > the following 8-bit interrupt routing registers (main regs attribute
> > > > in dts):
> > > >
> > > > +----+-------------------------------------------------------------------+
> > > > | bit  | description
> > > >             |
> > > > +----+-------------------------------------------------------------------+
> > > > | 3:0 | Processor core to route                                           |
> > > > | 7:4 | Routed processor core interrupt pins (INT0--INT3) |
> > > > +-----+------------------------------------------------------------------+
> > > >
> > > > The "loongson,parent_int_map" attribute is to describe the routed
> > > > interrupt pins to cpuintc.
> > > >
> > > > However, the "interrupt-map" attribute is not supposed to be used for
> > > > interrupt controller in the normal case. Though since commit
> > > > 041284181226 ("of/irq: Allow matching of an interrupt-map local to an
> > > > interrupt controller"), the "interrupt-map" attribute can be used in
> > > > interrupt controller nodes. Some interrupt controllers were found not
> > > > to work properly later, so in commit de4adddcbcc2 ("of/irq: Add a
> > > > quirk for controllers with their own definition of interrupt-map"), a
> > > > quirk was added for these interrupt controllers. As we can see from
> > > > the commit message, this is a bad solution in itself.
> > > >
> > > > Similarly, if we choose to use the "interrupt-map" attribute in the
> > > > interrupt controller, we have to use this unfriendly solution (quirk).
> > > > Because we hope of_irq_parse_raw() stops at the liointc level rather
> > > > than goto its parent level.
> > > >
> > > > So, I don't think it's a good choice to use a bad solution as a replacement.
> > > >
> > > > Do you have any other ideas?
> > >
> > > Assuming this is changeable at runtime, this sounds to me like this
> > > mapping/routing could easily be exposed as irqchip cpu affinity. Then
> > > userspace can apply all the performance optimizations it wants (and
> > > can easily update them without fiddling with the kernel/dts).
> > >
> > > And then there would be no need to hardcode/describe it in the dts(i) at all.
> >
> > Hi Jonas:
> >
> > Thanks for your reply.
> >
> > It is possible that my non-detailed explanation caused your misunderstanding.
> > Allow me to explain again about the interrupt routing register above,
> > which we know is divided into two parts:
> >
> > +----+-------------------------------------------------------------------+
> > | bit  | description |
> > +----+-------------------------------------------------------------------+
> > | 3:0 | Processor core to route                                           |
> > | 7:4 | Routed processor core interrupt pins (INT0--INT3) |
> > +-----+------------------------------------------------------------------+
> >
> > The first part "processor core" will be set to "boot_cpu_id" in the
> > driver, which we assume is fixed and we don't need to care about it
> > here.
> > What we care about is the second part "mapping of device interrupts to
> > processor interrupt pins", which is what we want to describe in
> > dts(i).
> >
> > Let's take the Loongson-2K1000 as an example again, it has 64
> > interrupt sources as inputs and 4 processor core interrupt pins as
> > outputs.
> > The sketch is shown below:
> >
> > Device Interrupts           Interrupt Pins
> >                  +-------------+
> >          0---->|                |--> INT0
> >         ...       | Mapping |--> INT1
> >         ...       |                |--> INT2
> >         63--->|                |--> INT3
> >                  +-------------+
> >
> > Therefore, this mapping relationship cannot be changed at runtime and
> > needs to be hardcoded/described in dts(i).
>
> But that's only a driver/description limitation, not an actual
> physical limitation, right? In theory you could reroute them as much
> as you want as long as you keep the kernel up-to-date about the
> current routing (via whatever means).
>
> Anyway, I guess you want to use the routed interrupt pin to identify
> different irq controller blocks.
>
> Can't the interrupt pin be inferred from the parent interrupt? If your
> parent (hw) irq is two, route everything to INT0 etc? Or alternatively
> use the name of the parent interrupt?
Let me make things clear and ignore those irrelevant information.
1, Our liointc controller has 32 inputs from downstream interrupt
sources and 4 outputs to the parent irqchip, the "routing" here means
which input go to which output.
2, We use 'parent_int_map' to describe the boot-time routing in dts
previously, but Krzysztof suggests us to use 'interrupt-map' instead.
3, When we rework our driver to use 'interrupt-map', we found that
this property is not supposed to be used in a regular irqchip (it is
usually used in a pcie port which is also act as an irqchip).
4, If we still want to use 'interrupt-map' to describe the routing in
liointc, we should make of_irq_parse_raw() stop at the liointc level
rather than go to its parent level, because the hwirq is provided by
liointc, not its parent. To archive this goal, we should add liointc
to the quirk list.
5, So, for our liointc driver we have two choices: 1) still use the
'parent_int_map' property; 2) use 'interrupt-map' property and add
liointc to the quirk list. We prefer the first ourselves, but
Krzysztof may give us a best solution.

Huacai

>
> Best Regards,
> Jonas