| Message ID | 20230821061315.3416836-1-zhoubinbin@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] dt-bindings: interrupt-controller: loongson,liointc: Fix warnings about liointc-2.0 | expand |
Hi Krzysztof: Thanks for your detailed reply. On Tue, Aug 22, 2023 at 1:44 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 21/08/2023 08:13, Binbin Zhou 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. > > No, why? Parent? This does not make sense. This was noted in the v1 patch discussion. The liointc driver now gets the parent interrupt via of_irq_get_byname(), so I think the "interrupt-names" should be "required". https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-loongson-liointc.c?h=v6.5-rc6#n345 static const char *const parent_names[] = {"int0", "int1", "int2", "int3"}; for (i = 0; i < LIOINTC_NUM_PARENT; i++) { parent_irq[i] = of_irq_get_byname(node, parent_names[i]); if (parent_irq[i] > 0) have_parent = TRUE; } if (!have_parent) return -ENODEV; > > > > > 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". > > Why? What the pattern has to do with anything in routing or not routing > something? First of all, interrupt routing is configurable and each intx handles up to 32 interrupt sources. int0-int3 you can choose a single one or a combination of multiple ones, as long as the intx chosen matches the parent interrupt and is not duplicated: Parent interrupt --> intx 2-->int0 3-->int1 4-->int2 5-->int3 As: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/boot/dts/loongson/loongson64g-package.dtsi?h=v6.5-rc6#n24 In addition, if there are 64 interrupt sources, such as the mips Loongson-2K1000, and we need two dts nodes to describe the interrupt routing, then there is bound to be a node without "int0". https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi?h=v6.5-rc6#n60 According to the current dt-binding rule, if the node does not have "int0", there will be a dts_check warning, which is not in line with our original intention. > > > > > 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 <zhoubinbin@loongson.cn> > > --- > > 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/20230815084713.1627520-1-zhoubinbin@loongson.cn/ > > > > .../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 > > - minItems: 3 > > + reg: true > > No. Constraints must be here. May I ask a question: Since different compatibles require different minItems/minItems for the attribute, this writeup of defining the attribute to be true first and then defining the specific value in an if-else statement is not recommended? > > > > > - reg-names: > > - items: > > - - const: main > > - - const: isr0 > > - - const: isr1 > > + reg-names: true > > No, keep at least min/maxItems here. > > > > > 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 > > I don't see reason behind it. > > > > > '#interrupt-cells': > > const: 2 > > @@ -69,32 +61,41 @@ required: > > - compatible > > - reg > > - interrupts > > + - interrupt-names > > Why? You are doing multiple things at once, without proper explanation. Maybe this patch does too many things... There are actually 3 things here, as stated in the commit message, and since they are all about liointc-2.0 dts-check warnings, I put them in a patch. > > > - 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 > > Srsly, why this is moved here from the top? It does not make sense. In liointc-2.0, we need to deal with two dts nodes, and the setting and routing registers are not contiguous, so the driver needs "reg-names" to get the corresponding register mapping. So I put all this in the liointc-2.0 section. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-loongson-liointc.c?h=v6.5-rc6#n225 if (revision > 1) { for (i = 0; i < LIOINTC_NUM_CORES; i++) { int index = of_property_match_string(node, "reg-names", core_reg_names[i]); if (index < 0) continue; priv->core_isr[i] = of_iomap(node, index); } if (!priv->core_isr[0]) goto out_iounmap; } I referenced other dt-binding writeups and thought this would be clearer. Is this if-else style not recommended? Should I keep the v1 patch writeup? https://lore.kernel.org/all/20230815084713.1627520-1-zhoubinbin@loongson.cn/ Thanks. Binbin > > > + required: > > + - reg-names > > + else: > > + properties: > > + reg: > > + maxItems: 1 > > so reg-names can be "pink-pony"? > > Best regards, > Krzysztof >
On 24/08/2023 13:32, Binbin Zhou wrote: > Hi Krzysztof: > > Thanks for your detailed reply. > > On Tue, Aug 22, 2023 at 4:30 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 22/08/2023 10:13, Binbin Zhou wrote: >>> Hi Krzysztof: >>> >>> Thanks for your detailed reply. >>> >>> On Tue, Aug 22, 2023 at 1:44 PM Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 21/08/2023 08:13, Binbin Zhou 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. >>>> >>>> No, why? Parent? This does not make sense. >>> >>> This was noted in the v1 patch discussion. The liointc driver now gets >>> the parent interrupt via of_irq_get_byname(), so I think the >>> "interrupt-names" should be "required". >> >> of_irq_get_byname() does not give you parent interrupt, but the >> interrupt. Why do you need parent interrupt and what is it? >> >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-loongson-liointc.c?h=v6.5-rc6#n345 >>> >>> static const char *const parent_names[] = {"int0", "int1", "int2", "int3"}; >>> >>> for (i = 0; i < LIOINTC_NUM_PARENT; i++) { >>> parent_irq[i] = of_irq_get_byname(node, parent_names[i]); >>> if (parent_irq[i] > 0) >>> have_parent = TRUE; >>> } >>> if (!have_parent) >>> return -ENODEV; >> >> How requiring parents interrupt is related to other changes in this >> file? One logical change, one patch. > > Yes, that was my mistake, whether or not the interrupt-names need to > be "required" is another issue. It does not cause a check warning. > I'll think about it some more. >> >> Anyway why did you do it and take it by names? Names here are basically >> useless if they match indices, so just get interrupt by indices. > > There is a match between interrupts, interrupt names and interrupt maps: > > interrupt->interrupt name->interrupt map > 2->int0->int_map[0] > 3->int1->int_map[1] > 4->int2->int_map[2] > 5->int3->int_map[3] > > As part of the 2k1000 liointc1 node: > > liointc1: interrupt-controller@1fe11440 { > .... > interrupt-parent = <&cpuintc>; > interrupts = <3>; > interrupt-names = "int1"; > > loongson,parent_int_map = <0x00000000>, /* int0 */ How did you sneak this property? The version - v2 - which was reviewed by Rob: https://lore.kernel.org/all/20190905144316.12527-7-jiaxun.yang@flygoat.com/ did not have it. Now v3 suddenly appears with Rob's review and this property: https://lore.kernel.org/all/20200112081416.722218-4-jiaxun.yang@flygoat.com/ Please help me understand this property appeared there and how did you get it reviewed? > <0xffffffff>, /* int1 */ > <0x00000000>, /* int2 */ > <0x00000000>; /* int3 */ So now you will keep bringing more hacks for a hacky property. No, this cannot go on. Best regards, Krzysztof
在 2023/8/25 20:56, Krzysztof Kozlowski 写道: [...] > How did you sneak this property? The version - v2 - which was reviewed > by Rob: > https://lore.kernel.org/all/20190905144316.12527-7-jiaxun.yang@flygoat.com/ > did not have it. > > Now v3 suddenly appears with Rob's review and this property: > https://lore.kernel.org/all/20200112081416.722218-4-jiaxun.yang@flygoat.com/ > > Please help me understand this property appeared there and how did you > get it reviewed? Hi all, It has been some years since this series was merged. My vague memory tells me there was some off-list discussion made in IRC with linux-arch folks and IRQ folks to come up with this binding design. In this case I guess I forgot to drop Rob's R-b tag when updating this patch between reversions. I apologize for any inconvenience this may have caused. > >> <0xffffffff>, /* int1 */ >> <0x00000000>, /* int2 */ >> <0x00000000>; /* int3 */ > So now you will keep bringing more hacks for a hacky property. No, this > cannot go on. 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. A little bit background about this property, LIOINTC can route a interrupt to any of 4 upstream core interrupt pins. Downstream interrupt devicies should not care about which pin the interrupt go but we want to leave a knob in devicetree for performance tuning. So we designed such property that use masks corresponding to each upsteam interrupt pins to tell where should a interrupt go. Thnaks - Jiaxun > > Best regards, > Krzysztof >
On Wed, 30 Aug 2023 04:59:20 +0100, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: > > > > 在 2023/8/25 20:56, Krzysztof Kozlowski 写道: > [...] > > How did you sneak this property? The version - v2 - which was reviewed > > by Rob: > > https://lore.kernel.org/all/20190905144316.12527-7-jiaxun.yang@flygoat.com/ > > did not have it. > > > > Now v3 suddenly appears with Rob's review and this property: > > https://lore.kernel.org/all/20200112081416.722218-4-jiaxun.yang@flygoat.com/ > > > > Please help me understand this property appeared there and how did you > > get it reviewed? > Hi all, > > It has been some years since this series was merged. > My vague memory tells me there was some off-list discussion made in IRC with > linux-arch folks and IRQ folks to come up with this binding design. > > In this case I guess I forgot to drop Rob's R-b tag when updating this patch > between reversions. I apologize for any inconvenience this may have caused. > > > > >> <0xffffffff>, /* int1 */ > >> <0x00000000>, /* int2 */ > >> <0x00000000>; /* int3 */ > > So now you will keep bringing more hacks for a hacky property. No, this > > cannot go on. > > 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. As for the offending property, it has no place here either. DT is not the place where you put "performance knobs". M.
On 30/08/2023 05:59, Jiaxun Yang wrote: > > > 在 2023/8/25 20:56, Krzysztof Kozlowski 写道: > [...] >> How did you sneak this property? The version - v2 - which was reviewed >> by Rob: >> https://lore.kernel.org/all/20190905144316.12527-7-jiaxun.yang@flygoat.com/ >> did not have it. >> >> Now v3 suddenly appears with Rob's review and this property: >> https://lore.kernel.org/all/20200112081416.722218-4-jiaxun.yang@flygoat.com/ >> >> Please help me understand this property appeared there and how did you >> get it reviewed? > Hi all, > > It has been some years since this series was merged. > My vague memory tells me there was some off-list discussion made in IRC with > linux-arch folks and IRQ folks to come up with this binding design. We would not suggest you property which in the name has underscores and duplicates interrupt-map property. > > In this case I guess I forgot to drop Rob's R-b tag when updating this patch > between reversions. I apologize for any inconvenience this may have caused. > >> >>> <0xffffffff>, /* int1 */ >>> <0x00000000>, /* int2 */ >>> <0x00000000>; /* int3 */ >> So now you will keep bringing more hacks for a hacky property. No, this >> cannot go on. > > 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. > > A little bit background about this property, LIOINTC can route a > interrupt to any of > 4 upstream core interrupt pins. Downstream interrupt devicies should not > care about > which pin the interrupt go but we want to leave a knob in devicetree for > performance > tuning. So we designed such property that use masks corresponding to > each upsteam > interrupt pins to tell where should a interrupt go. Best regards, Krzysztof
在 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 <jiaxun.yang@flygoat.com> 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 Mon, 16 Oct 2023 at 13:26, Binbin Zhou <zhoubb.aaron@gmail.com> 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 <jonas.gorski@gmail.com> wrote: > > On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <zhoubb.aaron@gmail.com> 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 <zhoubb.aaron@gmail.com> wrote: > > On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <jonas.gorski@gmail.com> wrote: > > > > On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <zhoubb.aaron@gmail.com> 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 <jonas.gorski@gmail.com> wrote: > > On Wed, 18 Oct 2023 at 08:58, Binbin Zhou <zhoubb.aaron@gmail.com> wrote: > > > > On Tue, Oct 17, 2023 at 9:05 PM Jonas Gorski <jonas.gorski@gmail.com> wrote: > > > > > > On Mon, 16 Oct 2023 at 13:26, Binbin Zhou <zhoubb.aaron@gmail.com> 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 <zhoubb.aaron@gmail.com> 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.
Hi, Krzysztof On Wed, Oct 25, 2023 at 3:16 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 25/10/2023 03:56, Huacai Chen wrote: > > Hi, Krzysztof, > > > > On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <maz@kernel.org> wrote: > >> > >> On Fri, 20 Oct 2023 10:51:35 +0100, > >> Binbin Zhou <zhoubb.aaron@gmail.com> 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 Sun, Oct 29, 2023 at 1:42 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 26/10/2023 09:19, Huacai Chen wrote: > > Hi, Krzysztof > > > > On Wed, Oct 25, 2023 at 3:16 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 25/10/2023 03:56, Huacai Chen wrote: > >>> Hi, Krzysztof, > >>> > >>> On Fri, Oct 20, 2023 at 8:18 PM Marc Zyngier <maz@kernel.org> wrote: > >>>> > >>>> On Fri, 20 Oct 2023 10:51:35 +0100, > >>>> Binbin Zhou <zhoubb.aaron@gmail.com> 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 >
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 */ - }; ...
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 <zhoubinbin@loongson.cn> --- 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/20230815084713.1627520-1-zhoubinbin@loongson.cn/ .../loongson,liointc.yaml | 74 +++++++++---------- 1 file changed, 37 insertions(+), 37 deletions(-)