Since commit f4dee5d8e1fa ("dt-bindings: interrupt-controller: Add
Loongson-2K1000 LIOINTC"), the loongson liointc supports configuring
routes for 64-bit interrupt sources.
For liointc-2.0, we need to define two liointc nodes in dts, one for
"0-31" interrupt sources and the other for "32-63" interrupt sources.
This applies to mips Loongson-2K1000.
Unfortunately, there are some warnings about "loongson,liointc-2.0":
1. "interrupt-names" should be "required", the driver gets the parent
interrupts through it.
2. Since not all CPUs are multicore, e.g. Loongson-2K0500 is a
single-core CPU, there is no core1-related registers. So "reg" and
"reg-names" should be set to "minItems 2".
3. Routing interrupts from "int0" is a common solution in practice, but
theoretically there is no such requirement, as long as conflicts are
avoided. So "interrupt-names" should be defined by "pattern".
This fixes dtbs_check warning:
DTC_CHK arch/mips/boot/dts/loongson/loongson64_2core_2k1000.dtb
arch/mips/boot/dts/loongson/loongson64_2core_2k1000.dtb: interrupt-controller@1fe11440: interrupt-names:0: 'int0' was expected
From schema: Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
arch/mips/boot/dts/loongson/loongson64_2core_2k1000.dtb: interrupt-controller@1fe11440: Unevaluated properties are not allowed ('interrupt-names' was unexpected)
From schema: Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
Fixes: f4dee5d8e1fa ("dt-bindings: interrupt-controller: Add Loongson-2K1000 LIOINTC")
Signed-off-by: Binbin Zhou <[email protected]>
---
V2:
1. Update commit message;
2. "interruprt-names" should be "required", the driver gets the parent
interrupts through it;
3. Add more descriptions to explain the rationale for multiple nodes;
4. Rewrite if-else statements.
Link to V1:
https://lore.kernel.org/all/[email protected]/
.../loongson,liointc.yaml | 74 +++++++++----------
1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml b/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
index 00b570c82903..f695d3a75ddf 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
@@ -11,11 +11,11 @@ maintainers:
description: |
This interrupt controller is found in the Loongson-3 family of chips and
- Loongson-2K1000 chip, as the primary package interrupt controller which
+ Loongson-2K series chips, as the primary package interrupt controller which
can route local I/O interrupt to interrupt lines of cores.
-
-allOf:
- - $ref: /schemas/interrupt-controller.yaml#
+ In particular, the Loongson-2K1000/2K0500 has 64 interrupt sources that we
+ need to describe with two dts nodes. One for interrupt sources "0-31" and
+ the other for interrupt sources "32-63".
properties:
compatible:
@@ -24,15 +24,9 @@ properties:
- loongson,liointc-1.0a
- loongson,liointc-2.0
- reg:
- minItems: 1
- maxItems: 3
+ reg: true
- reg-names:
- items:
- - const: main
- - const: isr0
- - const: isr1
+ reg-names: true
interrupt-controller: true
@@ -45,11 +39,9 @@ properties:
interrupt-names:
description: List of names for the parent interrupts.
items:
- - const: int0
- - const: int1
- - const: int2
- - const: int3
+ pattern: int[0-3]
minItems: 1
+ maxItems: 4
'#interrupt-cells':
const: 2
@@ -69,32 +61,41 @@ required:
- compatible
- reg
- interrupts
+ - interrupt-names
- interrupt-controller
- '#interrupt-cells'
- loongson,parent_int_map
-
unevaluatedProperties: false
-if:
- properties:
- compatible:
- contains:
- enum:
- - loongson,liointc-2.0
-
-then:
- properties:
- reg:
- minItems: 3
-
- required:
- - reg-names
-
-else:
- properties:
- reg:
- maxItems: 1
+allOf:
+ - $ref: /schemas/interrupt-controller.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - loongson,liointc-2.0
+ then:
+ properties:
+ reg:
+ minItems: 2
+ items:
+ - description: Interrupt routing registers.
+ - description: Low/high 32-bit interrupt status routed to core0.
+ - description: Low/high 32-bit interrupt status routed to core1.
+ reg-names:
+ minItems: 2
+ items:
+ - const: main
+ - const: isr0
+ - const: isr1
+ required:
+ - reg-names
+ else:
+ properties:
+ reg:
+ maxItems: 1
examples:
- |
@@ -113,7 +114,6 @@ examples:
<0x0f000000>, /* int1 */
<0x00000000>, /* int2 */
<0x00000000>; /* int3 */
-
};
...
--
2.39.3
Reviewed-by: Huacai Chen <[email protected]>
On Mon, Aug 21, 2023 at 2:13 PM Binbin Zhou <[email protected]> wrote:
>
> Since commit f4dee5d8e1fa ("dt-bindings: interrupt-controller: Add
> Loongson-2K1000 LIOINTC"), the loongson liointc supports configuring
> routes for 64-bit interrupt sources.
>
> For liointc-2.0, we need to define two liointc nodes in dts, one for
> "0-31" interrupt sources and the other for "32-63" interrupt sources.
> This applies to mips Loongson-2K1000.
>
> Unfortunately, there are some warnings about "loongson,liointc-2.0":
> 1. "interrupt-names" should be "required", the driver gets the parent
> interrupts through it.
>
> 2. Since not all CPUs are multicore, e.g. Loongson-2K0500 is a
> single-core CPU, there is no core1-related registers. So "reg" and
> "reg-names" should be set to "minItems 2".
>
> 3. Routing interrupts from "int0" is a common solution in practice, but
> theoretically there is no such requirement, as long as conflicts are
> avoided. So "interrupt-names" should be defined by "pattern".
>
> This fixes dtbs_check warning:
>
> DTC_CHK arch/mips/boot/dts/loongson/loongson64_2core_2k1000.dtb
> arch/mips/boot/dts/loongson/loongson64_2core_2k1000.dtb: interrupt-controller@1fe11440: interrupt-names:0: 'int0' was expected
> From schema: Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
> arch/mips/boot/dts/loongson/loongson64_2core_2k1000.dtb: interrupt-controller@1fe11440: Unevaluated properties are not allowed ('interrupt-names' was unexpected)
> From schema: Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
>
> Fixes: f4dee5d8e1fa ("dt-bindings: interrupt-controller: Add Loongson-2K1000 LIOINTC")
> Signed-off-by: Binbin Zhou <[email protected]>
> ---
> V2:
> 1. Update commit message;
> 2. "interruprt-names" should be "required", the driver gets the parent
> interrupts through it;
> 3. Add more descriptions to explain the rationale for multiple nodes;
> 4. Rewrite if-else statements.
>
> Link to V1:
> https://lore.kernel.org/all/[email protected]/
>
> .../loongson,liointc.yaml | 74 +++++++++----------
> 1 file changed, 37 insertions(+), 37 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml b/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
> index 00b570c82903..f695d3a75ddf 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/loongson,liointc.yaml
> @@ -11,11 +11,11 @@ maintainers:
>
> description: |
> This interrupt controller is found in the Loongson-3 family of chips and
> - Loongson-2K1000 chip, as the primary package interrupt controller which
> + Loongson-2K series chips, as the primary package interrupt controller which
> can route local I/O interrupt to interrupt lines of cores.
> -
> -allOf:
> - - $ref: /schemas/interrupt-controller.yaml#
> + In particular, the Loongson-2K1000/2K0500 has 64 interrupt sources that we
> + need to describe with two dts nodes. One for interrupt sources "0-31" and
> + the other for interrupt sources "32-63".
>
> properties:
> compatible:
> @@ -24,15 +24,9 @@ properties:
> - loongson,liointc-1.0a
> - loongson,liointc-2.0
>
> - reg:
> - minItems: 1
> - maxItems: 3
> + reg: true
>
> - reg-names:
> - items:
> - - const: main
> - - const: isr0
> - - const: isr1
> + reg-names: true
>
> interrupt-controller: true
>
> @@ -45,11 +39,9 @@ properties:
> interrupt-names:
> description: List of names for the parent interrupts.
> items:
> - - const: int0
> - - const: int1
> - - const: int2
> - - const: int3
> + pattern: int[0-3]
> minItems: 1
> + maxItems: 4
>
> '#interrupt-cells':
> const: 2
> @@ -69,32 +61,41 @@ required:
> - compatible
> - reg
> - interrupts
> + - interrupt-names
> - interrupt-controller
> - '#interrupt-cells'
> - loongson,parent_int_map
>
> -
> unevaluatedProperties: false
>
> -if:
> - properties:
> - compatible:
> - contains:
> - enum:
> - - loongson,liointc-2.0
> -
> -then:
> - properties:
> - reg:
> - minItems: 3
> -
> - required:
> - - reg-names
> -
> -else:
> - properties:
> - reg:
> - maxItems: 1
> +allOf:
> + - $ref: /schemas/interrupt-controller.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - loongson,liointc-2.0
> + then:
> + properties:
> + reg:
> + minItems: 2
> + items:
> + - description: Interrupt routing registers.
> + - description: Low/high 32-bit interrupt status routed to core0.
> + - description: Low/high 32-bit interrupt status routed to core1.
> + reg-names:
> + minItems: 2
> + items:
> + - const: main
> + - const: isr0
> + - const: isr1
> + required:
> + - reg-names
> + else:
> + properties:
> + reg:
> + maxItems: 1
>
> examples:
> - |
> @@ -113,7 +114,6 @@ examples:
> <0x0f000000>, /* int1 */
> <0x00000000>, /* int2 */
> <0x00000000>; /* int3 */
> -
> };
>
> ...
> --
> 2.39.3
>
在 2023/8/30 21:44, Marc Zyngier 写道:
[...]
>> What's the best way, in your opinion, to overhaul this property? As we don't
>> really care backward compatibility of DTBs on those systems we can
>> just redesign it.
> You may not care about backward compatibility, but I do. We don't
> break existing systems, full stop.
Ah it won't break any existing system. Sorry for not giving enough insight
into the platform in previous reply. As for Loongson64 all DTBs are built
into kernel binary. So as long as binding are changed together with all DTS
in tree we won't break any system.
> As for the offending property, it has no place here either. DT is not
> the place where you put "performance knobs".
Hmm, I can see various bindings with vendor prefix exposing device
configurations. If we seen this interrupt routing as a device configuration
I don't think it's against devicetree design philosophy.
Thanks
- Jiaxun
>
> M.
>
在 2023/8/30 22:35, Krzysztof Kozlowski 写道:
>> What's the best way, in your opinion, to overhaul this property? As we don't
>> really care backward compatibility of DTBs on those systems we can just
>> redesign it.
> Deprecate the property in the bindings, allow driver to work with or
> without it and finally drop it entirely from DTS.
I'd love to have such configuration flexibility so I'd be sad to see it go.
+ Huacai and Binbin, what's your opinion?
If dropping such functionality in kernel is a must go, we can hardcode
to route all downstream interrupt to the first pin that passed to DT.
Thanks
- Jiaxun
> Best regards,
> Krzysztof
>
Each IRQ source of liointc may be routed to different IRQ source of
cpuintc, for implementing this, bit mapping between liointc and cpuintc
is required, and the mapping information is used for liointc IRQ routing
register setting. It seems that interrupt-map can not pass the mapping
to driver in current driver/of code, so the mapping is used in a
non-standard way(of cause, underscore style may be changed in dts and
driver).
IMO, hardcode routing completely in driver is not flexible and not
recommended, and if possible, keep current map unchanged please.
Jianmin
Thanks.
On 2023/8/31 上午9:47, Binbin Zhou wrote:
> cc Jianmin Lv.
>
> Hi all:
>
> Jianmin knows Loongson interrupt controllers well, he may have other
> suggestions.
>
> Thanks.
> Binbin
>
> On Wed, Aug 30, 2023 at 11:31 PM Jiaxun Yang <[email protected]> wrote:
>>
>>
>> 在 2023/8/30 22:35, Krzysztof Kozlowski 写道:
>>>> What's the best way, in your opinion, to overhaul this property? As we don't
>>>> really care backward compatibility of DTBs on those systems we can just
>>>> redesign it.
>>> Deprecate the property in the bindings, allow driver to work with or
>>> without it and finally drop it entirely from DTS.
>> I'd love to have such configuration flexibility so I'd be sad to see it go.
>> + Huacai and Binbin, what's your opinion?
>>
>> If dropping such functionality in kernel is a must go, we can hardcode
>> to route all downstream interrupt to the first pin that passed to DT.
>>
>> Thanks
>> - Jiaxun
>>> Best regards,
>>> Krzysztof
>>>
On Wed, 30 Aug 2023 16:25:48 +0100,
Jiaxun Yang <[email protected]> wrote:
>
>
>
> 在 2023/8/30 21:44, Marc Zyngier 写道:
> [...]
> >> What's the best way, in your opinion, to overhaul this property? As we don't
> >> really care backward compatibility of DTBs on those systems we can
> >> just redesign it.
> > You may not care about backward compatibility, but I do. We don't
> > break existing systems, full stop.
> Ah it won't break any existing system. Sorry for not giving enough insight
> into the platform in previous reply. As for Loongson64 all DTBs are built
> into kernel binary. So as long as binding are changed together with all DTS
> in tree we won't break any system.
This is factually wrong. QEMU produces a DT for Loongarch at runtime.
So no, you're not allowed to just drop bindings on the floor. They
stay forever.
> > As for the offending property, it has no place here either. DT is not
> > the place where you put "performance knobs".
> Hmm, I can see various bindings with vendor prefix exposing device
> configurations. If we seen this interrupt routing as a device configuration
> I don't think it's against devicetree design philosophy.
Just because we have tons of crap in the device trees doesn't give you
a license to be just as bad.
M.
--
Without deviation from the norm, progress is not possible.
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?
Thanks.
Binbin
On Mon, Sep 4, 2023 at 2:54 PM Marc Zyngier <[email protected]> wrote:
>
> On Wed, 30 Aug 2023 16:25:48 +0100,
> Jiaxun Yang <[email protected]> wrote:
> >
> >
> >
> > 在 2023/8/30 21:44, Marc Zyngier 写道:
> > [...]
> > >> What's the best way, in your opinion, to overhaul this property? As we don't
> > >> really care backward compatibility of DTBs on those systems we can
> > >> just redesign it.
> > > You may not care about backward compatibility, but I do. We don't
> > > break existing systems, full stop.
> > Ah it won't break any existing system. Sorry for not giving enough insight
> > into the platform in previous reply. As for Loongson64 all DTBs are built
> > into kernel binary. So as long as binding are changed together with all DTS
> > in tree we won't break any system.
>
> This is factually wrong. QEMU produces a DT for Loongarch at runtime.
> So no, you're not allowed to just drop bindings on the floor. They
> stay forever.
>
> > > As for the offending property, it has no place here either. DT is not
> > > the place where you put "performance knobs".
> > Hmm, I can see various bindings with vendor prefix exposing device
> > configurations. If we seen this interrupt routing as a device configuration
> > I don't think it's against devicetree design philosophy.
>
> Just because we have tons of crap in the device trees doesn't give you
> a license to be just as bad.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <[email protected]> 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.
Best Regards,
Jonas
On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <[email protected]> wrote:
>
> On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <[email protected]> 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).
Thanks.
Binbin
>
> Best Regards,
> Jonas
On Wed, 18 Oct 2023 at 08:58, Binbin Zhou <[email protected]> wrote:
>
> On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <[email protected]> wrote:
> >
> > On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <[email protected]> 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?
Best Regards,
Jonas
Hi, Jonas,
On Wed, Oct 18, 2023 at 5:38 PM Jonas Gorski <[email protected]> wrote:
>
> On Wed, 18 Oct 2023 at 08:58, Binbin Zhou <[email protected]> wrote:
> >
> > On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <[email protected]> wrote:
> > >
> > > On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <[email protected]> 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
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 <[email protected]> wrote:
>
> Hi, Jonas,
>
> On Wed, Oct 18, 2023 at 5:38 PM Jonas Gorski <[email protected]> wrote:
> >
> > On Wed, 18 Oct 2023 at 08:58, Binbin Zhou <[email protected]> wrote:
> > >
> > > On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <[email protected]> wrote:
> > > >
> > > > On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <[email protected]> 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
On Fri, 20 Oct 2023 10:51:35 +0100,
Binbin Zhou <[email protected]> wrote:
>
> 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?
I don't have any suggestion, but if you are still thinking of adding
some extra crap to the of_irq_imap_abusers[] array, the answer is a
firm 'NO'.
M.
--
Without deviation from the norm, progress is not possible.
Hi, Krzysztof,
On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <[email protected]> wrote:
>
> On Fri, 20 Oct 2023 10:51:35 +0100,
> Binbin Zhou <[email protected]> wrote:
> >
> > 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?
>
> I don't have any suggestion, but if you are still thinking of adding
> some extra crap to the of_irq_imap_abusers[] array, the answer is a
> firm 'NO'.
Excuse me, but as described before, 'interrupt-map' cannot be used for
liointc unless adding it to of_irq_imap_abusers[], can we still use
'parent_int_map' in this case? Or just change it to 'parent-int-map'
to satisfy the naming style?
Huacai
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
On 25/10/2023 03:56, Huacai Chen wrote:
> Hi, Krzysztof,
>
> On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <[email protected]> wrote:
>>
>> On Fri, 20 Oct 2023 10:51:35 +0100,
>> Binbin Zhou <[email protected]> wrote:
>>>
>>> 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?
>>
>> I don't have any suggestion, but if you are still thinking of adding
>> some extra crap to the of_irq_imap_abusers[] array, the answer is a
>> firm 'NO'.
> Excuse me, but as described before, 'interrupt-map' cannot be used for
> liointc unless adding it to of_irq_imap_abusers[], can we still use
> 'parent_int_map' in this case? Or just change it to 'parent-int-map'
> to satisfy the naming style?
Why do you respond to me? You received firm 'NO' about
of_irq_imap_abusers, so how adhering to naming style or violating naming
style has anything to do with it?
Best regards,
Krzysztof
Hi, Krzysztof
On Wed, Oct 25, 2023 at 3:16 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 25/10/2023 03:56, Huacai Chen wrote:
> > Hi, Krzysztof,
> >
> > On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <[email protected]> wrote:
> >>
> >> On Fri, 20 Oct 2023 10:51:35 +0100,
> >> Binbin Zhou <[email protected]> wrote:
> >>>
> >>> 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?
> >>
> >> I don't have any suggestion, but if you are still thinking of adding
> >> some extra crap to the of_irq_imap_abusers[] array, the answer is a
> >> firm 'NO'.
> > Excuse me, but as described before, 'interrupt-map' cannot be used for
> > liointc unless adding it to of_irq_imap_abusers[], can we still use
> > 'parent_int_map' in this case? Or just change it to 'parent-int-map'
> > to satisfy the naming style?
>
> Why do you respond to me? You received firm 'NO' about
> of_irq_imap_abusers, so how adhering to naming style or violating naming
> style has anything to do with it?
I'm sorry but of_irq_imap_abusers is to make 'interrupt-map' to work,
without of_irq_imap_abusers we can only use the existing
'parent_int_map'. We need your response because we want to know
whether you can accept the existing method since the other approach
has received 'NO'. And, changing 'parent_int_map' to 'parent-int-map'
can be a little better, at least it satisfies the naming style.
Huacai
>
> Best regards,
> Krzysztof
>
On 26/10/2023 09:19, Huacai Chen wrote:
> Hi, Krzysztof
>
> On Wed, Oct 25, 2023 at 3:16 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 25/10/2023 03:56, Huacai Chen wrote:
>>> Hi, Krzysztof,
>>>
>>> On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <[email protected]> wrote:
>>>>
>>>> On Fri, 20 Oct 2023 10:51:35 +0100,
>>>> Binbin Zhou <[email protected]> wrote:
>>>>>
>>>>> 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?
>>>>
>>>> I don't have any suggestion, but if you are still thinking of adding
>>>> some extra crap to the of_irq_imap_abusers[] array, the answer is a
>>>> firm 'NO'.
>>> Excuse me, but as described before, 'interrupt-map' cannot be used for
>>> liointc unless adding it to of_irq_imap_abusers[], can we still use
>>> 'parent_int_map' in this case? Or just change it to 'parent-int-map'
>>> to satisfy the naming style?
>>
>> Why do you respond to me? You received firm 'NO' about
>> of_irq_imap_abusers, so how adhering to naming style or violating naming
>> style has anything to do with it?
> I'm sorry but of_irq_imap_abusers is to make 'interrupt-map' to work,
> without of_irq_imap_abusers we can only use the existing
> 'parent_int_map'. We need your response because we want to know
> whether you can accept the existing method since the other approach
> has received 'NO'. And, changing 'parent_int_map' to 'parent-int-map'
> can be a little better, at least it satisfies the naming style.
Indeed, interrupt-map might not fit here. I don't know whether your
custom property - purely for runtime performance purpose - will be
accepted. Initial description of this field suggested that it is OS
policy, not hardware choice. But sure, propose something with
justification, so we can review it. The proposal must not break ABI, so
you must support both parent_int_map and parent-int-map (or whatever we
call it) properties. The first we will probably deprecate.
The way this property was sneaked into kernel bypassing review is still
disappointing.
Best regards,
Krzysztof
On Sun, Oct 29, 2023 at 1:42 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 26/10/2023 09:19, Huacai Chen wrote:
> > Hi, Krzysztof
> >
> > On Wed, Oct 25, 2023 at 3:16 PM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 25/10/2023 03:56, Huacai Chen wrote:
> >>> Hi, Krzysztof,
> >>>
> >>> On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <[email protected]> wrote:
> >>>>
> >>>> On Fri, 20 Oct 2023 10:51:35 +0100,
> >>>> Binbin Zhou <[email protected]> wrote:
> >>>>>
> >>>>> 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?
> >>>>
> >>>> I don't have any suggestion, but if you are still thinking of adding
> >>>> some extra crap to the of_irq_imap_abusers[] array, the answer is a
> >>>> firm 'NO'.
> >>> Excuse me, but as described before, 'interrupt-map' cannot be used for
> >>> liointc unless adding it to of_irq_imap_abusers[], can we still use
> >>> 'parent_int_map' in this case? Or just change it to 'parent-int-map'
> >>> to satisfy the naming style?
> >>
> >> Why do you respond to me? You received firm 'NO' about
> >> of_irq_imap_abusers, so how adhering to naming style or violating naming
> >> style has anything to do with it?
> > I'm sorry but of_irq_imap_abusers is to make 'interrupt-map' to work,
> > without of_irq_imap_abusers we can only use the existing
> > 'parent_int_map'. We need your response because we want to know
> > whether you can accept the existing method since the other approach
> > has received 'NO'. And, changing 'parent_int_map' to 'parent-int-map'
> > can be a little better, at least it satisfies the naming style.
>
> Indeed, interrupt-map might not fit here. I don't know whether your
> custom property - purely for runtime performance purpose - will be
> accepted. Initial description of this field suggested that it is OS
> policy, not hardware choice. But sure, propose something with
> justification, so we can review it. The proposal must not break ABI, so
> you must support both parent_int_map and parent-int-map (or whatever we
> call it) properties. The first we will probably deprecate.
>
Hi Krzysztof:
Thanks a lot for your reply and suggestion!
I'll try to split the change points into separate patches in the next
version, it might be better understood.
Thanks.
Binbin
> The way this property was sneaked into kernel bypassing review is still
> disappointing.
>
> Best regards,
> Krzysztof
>