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

[Binbin Zhou]

Hi Krzysztof & Marc:

Sorry for the interruption.
As said before, we tried to use the 'interrupt-map attribute' in our
Loongson liointc dts(i), but there are some unfriendly points.
Do you have any other different suggestions?

Thanks.
Binbin

On Wed, Oct 18, 2023 at 8:33 PM Huacai Chen <chenhuacai@xxxxxxxxxx> wrote:
>
> 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