| Message ID | 0a91967d40d486bb8cccd0dcf5a817df11317cf0.1640548009.git.sander@svanheule.net |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Rework realtek-rtl IRQ driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | success |
On Sun, 26 Dec 2021 19:59:27 +0000, Sander Vanheule <sander@svanheule.net> wrote: > > Amend the binding to also require a list of parent interrupts, and an > optional mask to specify which parent is mapped to which output. > > Without this information, any driver would have to make an assumption on > which parent interrupt is connected to which output. Why should an endpoint driver care at all? > > Additionally, extend (or add) the relevant descriptions to more clearly > describe the inputs and outputs of this router. > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > --- > Since it does not properly describe the hardware, I would still really > rather get rid of "interrupt-map", even though that would mean breaking > ABI for this binding. As we've argued before [1], that is our prefered > solution, and would enable us to not carry more (hacky) code because of > a mistake with the initial submission. Again, this is too late. Broken bindings live forever. > > Vendors don't ship independent DT blobs for devices with this hardware, > so the independent devicetree/kernel upgrades issue is really rather > theoretical here. Realtek isn't driving the development of the bindings > and associated drivers for this platform. They have their SDK and seem > to care very little about proper kernel integration. Any vendor can do whatever they want. You can do the same thing if you really want to. > > Furthermore, there are currently no device descriptions in the kernel > using this binding. There are in OpenWrt, but OpenWrt firmware images > for this platform always contain both the kernel and the appended DTB, > so there's also no breakage to worry about. That's just one use case. Who knows who is using this stuff in a different context? Nobody can tell. > > [1] https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/ > > Differences with v1: > - Don't drop the "interrupt-map" property > - Add the "realtek,output-valid-mask" property > --- > .../realtek,rtl-intc.yaml | 38 ++++++++++++++++--- > 1 file changed, 33 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml > index 9e76fff20323..29014673c34e 100644 > --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml > +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml > @@ -6,6 +6,10 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# > > title: Realtek RTL SoC interrupt controller devicetree bindings > > +description: > + Interrupt router for Realtek MIPS SoCs, allowing up to 32 SoC interrupts to > + be routed to one of up to 15 parent interrupts, or left disconnected. > + > maintainers: > - Birger Koblitz <mail@birger-koblitz.de> > - Bert Vermeulen <bert@biot.com> > @@ -22,7 +26,11 @@ properties: > maxItems: 1 > > interrupts: > - maxItems: 1 > + minItems: 1 > + maxItems: 15 > + description: > + List of parent interrupts, in the order that they are connected to this > + interrupt router's outputs. Is that to support multiple SoCs? I'd expect a given SoC to have a fixed number of output interrupts. > > interrupt-controller: true > > @@ -30,7 +38,21 @@ properties: > const: 0 > > interrupt-map: > - description: Describes mapping from SoC interrupts to CPU interrupts > + description: > + List of <soc_int parent_phandle parent_args ...> tuples, where "soc_int" > + is the interrupt input line number as provided by this controller. > + "parent_phandle" and "parent_args" specify which parent interrupt this > + line should be routed to. Note that interrupt specifiers should be > + identical to the parents specified in the "interrupts" property. > + > + realtek,output-valid-mask: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Optional bit mask indicating which outputs are connected to the parent > + interrupts. The lowest set bit indicates which output line the first > + interrupt from "interrupts" is connected to, the second lowest set bit > + for the second interrupt, etc. If not provided, parent interrupts will be > + assigned sequentially to the outputs. > > required: > - compatible > @@ -39,6 +61,7 @@ required: > - interrupt-controller > - "#address-cells" > - interrupt-map > + - interrupts > > additionalProperties: false > > @@ -49,9 +72,14 @@ examples: > #interrupt-cells = <1>; > interrupt-controller; > reg = <0x3000 0x20>; > + > + interrupt-parent = <&cpuintc>; > + interrupts = <1>, <2>, <5>; > + realtek,output-valid-mask = <0x13>; What additional information does this bring? From the description above, this is all SW configurable, so why should this be described in the DT? > + > #address-cells = <0>; > interrupt-map = > - <31 &cpuintc 2>, > - <30 &cpuintc 1>, > - <29 &cpuintc 5>; > + <31 &cpuintc 2>, /* connect to cpuintc 2 via output 1 */ > + <30 &cpuintc 1>, /* connect to cpuintc 1 via output 0 */ > + <29 &cpuintc 5>; /* connect to cpuintc 5 via output 4 */ > }; My conclusion here is that, as I stated in my initial review of this series, you could completely ignore the 3rd field of the map, and let the driver decide on the mapping without any extra information. We already have plenty of crossbar-type drivers in the tree that can mux a number of input to a number of outputs and route them accordingly to a set of parent interrupts. None of this requires to be described in DT. M.
On Mon, 2021-12-27 at 11:17 +0000, Marc Zyngier wrote: > On Sun, 26 Dec 2021 19:59:27 +0000, > Sander Vanheule <sander@svanheule.net> wrote: > > > > Amend the binding to also require a list of parent interrupts, and an > > optional mask to specify which parent is mapped to which output. > > > > Without this information, any driver would have to make an assumption on > > which parent interrupt is connected to which output. > > Why should an endpoint driver care at all? Interrupt inputs to interrupt outputs are SW configurable, but outputs to parent interrupts are hard-wired and cannot be modified. "interrupt-map" defines an input to parent interrupt mapping, so it seems a piece of information is missing. This is currently provided as an assumption in the driver ("CPU IRQs (2..7) are connected to outputs (1..6)"). Input-to-output is SW configurable, so that can be put in the driver. Output-to-parent is hardware configuration, > > > > Additionally, extend (or add) the relevant descriptions to more clearly > > describe the inputs and outputs of this router. > > > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > > --- > > Since it does not properly describe the hardware, I would still really > > rather get rid of "interrupt-map", even though that would mean breaking > > ABI for this binding. As we've argued before [1], that is our prefered > > solution, and would enable us to not carry more (hacky) code because of > > a mistake with the initial submission. > > Again, this is too late. Broken bindings live forever. > > > > > Vendors don't ship independent DT blobs for devices with this hardware, > > so the independent devicetree/kernel upgrades issue is really rather > > theoretical here. Realtek isn't driving the development of the bindings > > and associated drivers for this platform. They have their SDK and seem > > to care very little about proper kernel integration. > > Any vendor can do whatever they want. You can do the same thing if you > really want to. > > > > > Furthermore, there are currently no device descriptions in the kernel > > using this binding. There are in OpenWrt, but OpenWrt firmware images > > for this platform always contain both the kernel and the appended DTB, > > so there's also no breakage to worry about. > > That's just one use case. Who knows who is using this stuff in a > different context? Nobody can tell. > > > > > [1] https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/ > > > > Differences with v1: > > - Don't drop the "interrupt-map" property > > - Add the "realtek,output-valid-mask" property > > --- > > .../realtek,rtl-intc.yaml | 38 ++++++++++++++++--- > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl- > > intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl- > > intc.yaml > > index 9e76fff20323..29014673c34e 100644 > > --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml > > +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml > > @@ -6,6 +6,10 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# > > > > title: Realtek RTL SoC interrupt controller devicetree bindings > > > > +description: > > + Interrupt router for Realtek MIPS SoCs, allowing up to 32 SoC interrupts to > > + be routed to one of up to 15 parent interrupts, or left disconnected. > > + > > maintainers: > > - Birger Koblitz <mail@birger-koblitz.de> > > - Bert Vermeulen <bert@biot.com> > > @@ -22,7 +26,11 @@ properties: > > maxItems: 1 > > > > interrupts: > > - maxItems: 1 > > + minItems: 1 > > + maxItems: 15 > > + description: > > + List of parent interrupts, in the order that they are connected to this > > + interrupt router's outputs. > > Is that to support multiple SoCs? I'd expect a given SoC to have a > fixed number of output interrupts. It is, and they do AFAICT. But all values from 1 to 15 can be written to the routing registers, so I wanted this definition to be as broad as possible. The SoCs I'm working with only connect to the six CPU HW interrupts, but I don't know what the actual limit of this interrupt hardware is, or if the outputs always connect to the MIPS CPU HW interrupts. > > > > interrupt-controller: true > > > > @@ -30,7 +38,21 @@ properties: > > const: 0 > > > > interrupt-map: > > - description: Describes mapping from SoC interrupts to CPU interrupts > > + description: > > + List of <soc_int parent_phandle parent_args ...> tuples, where "soc_int" > > + is the interrupt input line number as provided by this controller. > > + "parent_phandle" and "parent_args" specify which parent interrupt this > > + line should be routed to. Note that interrupt specifiers should be > > + identical to the parents specified in the "interrupts" property. > > + > > + realtek,output-valid-mask: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + Optional bit mask indicating which outputs are connected to the parent > > + interrupts. The lowest set bit indicates which output line the first > > + interrupt from "interrupts" is connected to, the second lowest set bit > > + for the second interrupt, etc. If not provided, parent interrupts will be > > + assigned sequentially to the outputs. > > > > required: > > - compatible > > @@ -39,6 +61,7 @@ required: > > - interrupt-controller > > - "#address-cells" > > - interrupt-map > > + - interrupts > > > > additionalProperties: false > > > > @@ -49,9 +72,14 @@ examples: > > #interrupt-cells = <1>; > > interrupt-controller; > > reg = <0x3000 0x20>; > > + > > + interrupt-parent = <&cpuintc>; > > + interrupts = <1>, <2>, <5>; > > + realtek,output-valid-mask = <0x13>; > > What additional information does this bring? From the description > above, this is all SW configurable, so why should this be described in > the DT? The hardware I currently have, has a single contiguous range of outputs hard-wired to parent interrupts, starting at the first output. I wanted to provide maximum flexibility for output connections, which I think should support sparse output connections. However, since this would be an optional property, and is currently not needed for any hardware, I suppose it could be added later when needed. Adding "interrupts" as a required property is also a no-go, I assume, since that would invalidate currently-valid DT-s. > > + > > #address-cells = <0>; > > interrupt-map = > > - <31 &cpuintc 2>, > > - <30 &cpuintc 1>, > > - <29 &cpuintc 5>; > > + <31 &cpuintc 2>, /* connect to cpuintc 2 via output 1 */ > > + <30 &cpuintc 1>, /* connect to cpuintc 1 via output 0 */ > > + <29 &cpuintc 5>; /* connect to cpuintc 5 via output 4 */ > > }; > > My conclusion here is that, as I stated in my initial review of this > series, you could completely ignore the 3rd field of the map, and let > the driver decide on the mapping without any extra information. > > We already have plenty of crossbar-type drivers in the tree that can > mux a number of input to a number of outputs and route them > accordingly to a set of parent interrupts. None of this requires to be > described in DT. I had another look and "fsl,imx-intmux" looks like what I had in mind. If I understand correctly, changing "#interrupt-cells" (to add the routing info) and the required properties (to add "interrupts"), would require a new set of compatibles, in addition to some fall-back behaviour if only the original compatible is provided. Let me give this another spin, see what I can come up with. Best, Sander
On 28/12/2021 17:21, Sander Vanheule wrote: > On Mon, 2021-12-27 at 11:17 +0000, Marc Zyngier wrote: >> On Sun, 26 Dec 2021 19:59:27 +0000, >> Sander Vanheule <sander@svanheule.net> wrote: >>> >>> Amend the binding to also require a list of parent interrupts, and an >>> optional mask to specify which parent is mapped to which output. >>> >>> Without this information, any driver would have to make an assumption on >>> which parent interrupt is connected to which output. >> >> Why should an endpoint driver care at all? > > Interrupt inputs to interrupt outputs are SW configurable, but outputs to parent > interrupts are hard-wired and cannot be modified. "interrupt-map" defines an input to > parent interrupt mapping, so it seems a piece of information is missing. This is currently > provided as an assumption in the driver ("CPU IRQs (2..7) are connected to outputs > (1..6)"). > > Input-to-output is SW configurable, so that can be put in the driver. Output-to-parent is > hardware configuration, > > >>> >>> Additionally, extend (or add) the relevant descriptions to more clearly >>> describe the inputs and outputs of this router. >>> >>> Signed-off-by: Sander Vanheule <sander@svanheule.net> >>> --- >>> Since it does not properly describe the hardware, I would still really >>> rather get rid of "interrupt-map", even though that would mean breaking >>> ABI for this binding. As we've argued before [1], that is our prefered >>> solution, and would enable us to not carry more (hacky) code because of >>> a mistake with the initial submission. >> >> Again, this is too late. Broken bindings live forever. >> >>> >>> Vendors don't ship independent DT blobs for devices with this hardware, >>> so the independent devicetree/kernel upgrades issue is really rather >>> theoretical here. Realtek isn't driving the development of the bindings >>> and associated drivers for this platform. They have their SDK and seem >>> to care very little about proper kernel integration. >> >> Any vendor can do whatever they want. You can do the same thing if you >> really want to. >> >>> >>> Furthermore, there are currently no device descriptions in the kernel >>> using this binding. There are in OpenWrt, but OpenWrt firmware images >>> for this platform always contain both the kernel and the appended DTB, >>> so there's also no breakage to worry about. >> >> That's just one use case. Who knows who is using this stuff in a >> different context? Nobody can tell. >> >>> >>> [1] https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/ >>> >>> Differences with v1: >>> - Don't drop the "interrupt-map" property >>> - Add the "realtek,output-valid-mask" property >>> --- >>> .../realtek,rtl-intc.yaml | 38 ++++++++++++++++--- >>> 1 file changed, 33 insertions(+), 5 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl- >>> intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl- >>> intc.yaml >>> index 9e76fff20323..29014673c34e 100644 >>> --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml >>> @@ -6,6 +6,10 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# >>> >>> title: Realtek RTL SoC interrupt controller devicetree bindings >>> >>> +description: >>> + Interrupt router for Realtek MIPS SoCs, allowing up to 32 SoC interrupts to >>> + be routed to one of up to 15 parent interrupts, or left disconnected. >>> + >>> maintainers: >>> - Birger Koblitz <mail@birger-koblitz.de> >>> - Bert Vermeulen <bert@biot.com> >>> @@ -22,7 +26,11 @@ properties: >>> maxItems: 1 >>> >>> interrupts: >>> - maxItems: 1 >>> + minItems: 1 >>> + maxItems: 15 >>> + description: >>> + List of parent interrupts, in the order that they are connected to this >>> + interrupt router's outputs. >> >> Is that to support multiple SoCs? I'd expect a given SoC to have a >> fixed number of output interrupts. > > It is, and they do AFAICT. But all values from 1 to 15 can be written to the routing > registers, so I wanted this definition to be as broad as possible. > > The SoCs I'm working with only connect to the six CPU HW interrupts, but I don't know what > the actual limit of this interrupt hardware is, or if the outputs always connect to the > MIPS CPU HW interrupts. > From what I know, the IRQ controller is used solely by Realtek in the RTL838x, RTL839x and RTL930x SoC families, all of them MIPS 4KEc or 34Kc with the standard 7 CPU IRQ lines. In their final RTL931x series they abandoned their custom IRQ controller and went for an InterAptiv core with a standard MIPS GIC. Multiple SoCs are supported by these SoCs via direct register access for configuration either via SPI or I2C, for which the SoCs have HW support as a slave and master (only RTL93xx). Optionally, a GPIO pin can be used for raising an IRQ between SoCs. Fast control of the switch hardware in the SoCs is done by proprietary Ethernet frames. At least for OpenWRT, only the original 7 IRQ outputs are used. Adding support for 15 does not seem to hurt, though and if necessary the driver can do additional error checking. Cheers, Birger
Hi Birger, On Tue, 2021-12-28 at 17:53 +0100, Birger Koblitz wrote: > On 28/12/2021 17:21, Sander Vanheule wrote: > > On Mon, 2021-12-27 at 11:17 +0000, Marc Zyngier wrote: > > > On Sun, 26 Dec 2021 19:59:27 +0000, > > > Sander Vanheule <sander@svanheule.net> wrote: > > > > interrupts: > > > > - maxItems: 1 > > > > + minItems: 1 > > > > + maxItems: 15 > > > > + description: > > > > + List of parent interrupts, in the order that they are connected to this > > > > + interrupt router's outputs. > > > > > > Is that to support multiple SoCs? I'd expect a given SoC to have a > > > fixed number of output interrupts. > > > > It is, and they do AFAICT. But all values from 1 to 15 can be written to the routing > > registers, so I wanted this definition to be as broad as possible. > > > > The SoCs I'm working with only connect to the six CPU HW interrupts, but I don't know > > what > > the actual limit of this interrupt hardware is, or if the outputs always connect to > > the > > MIPS CPU HW interrupts. > > > From what I know, the IRQ controller is used solely by Realtek in the RTL838x, RTL839x > and > RTL930x SoC families, all of them MIPS 4KEc or 34Kc with the standard 7 CPU IRQ lines. > In their final RTL931x series they abandoned their custom IRQ controller and went for > an InterAptiv core with a standard MIPS GIC. There is some code floating around [1] to support a few Wi-Fi SoCs (RTL8196E, RTL8197D, and RTL8197F) which appear to use the same interrupt controller. Not that it's very likely these will ever be supported property, because they contain Lexra MIPS cores. That code claims these cores can have 16 CPU interrupts, althought the non-standard interrupts are apparently not implemented in that driver. There is also mention of 64 SoC interrupts, but it looks like this can be implemented by just instantiating this driver once for each register range (given the code would be modified to get rid of some static variables). Anyway, a mostly theoretical problem. The SoCs we're targetting (RTL8380x, RTL839x, and RTL930x) use the /6/ MIPS CPU HW interrupts (the two software interrupts are not used AFAICT). [1] https://github.com/ggbruno/openwrt/blob/Realtek/target/linux/realtek/files-4.14/arch/mips/realtek/irq.c Best, Sander
diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml index 9e76fff20323..29014673c34e 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml @@ -6,6 +6,10 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# title: Realtek RTL SoC interrupt controller devicetree bindings +description: + Interrupt router for Realtek MIPS SoCs, allowing up to 32 SoC interrupts to + be routed to one of up to 15 parent interrupts, or left disconnected. + maintainers: - Birger Koblitz <mail@birger-koblitz.de> - Bert Vermeulen <bert@biot.com> @@ -22,7 +26,11 @@ properties: maxItems: 1 interrupts: - maxItems: 1 + minItems: 1 + maxItems: 15 + description: + List of parent interrupts, in the order that they are connected to this + interrupt router's outputs. interrupt-controller: true @@ -30,7 +38,21 @@ properties: const: 0 interrupt-map: - description: Describes mapping from SoC interrupts to CPU interrupts + description: + List of <soc_int parent_phandle parent_args ...> tuples, where "soc_int" + is the interrupt input line number as provided by this controller. + "parent_phandle" and "parent_args" specify which parent interrupt this + line should be routed to. Note that interrupt specifiers should be + identical to the parents specified in the "interrupts" property. + + realtek,output-valid-mask: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Optional bit mask indicating which outputs are connected to the parent + interrupts. The lowest set bit indicates which output line the first + interrupt from "interrupts" is connected to, the second lowest set bit + for the second interrupt, etc. If not provided, parent interrupts will be + assigned sequentially to the outputs. required: - compatible @@ -39,6 +61,7 @@ required: - interrupt-controller - "#address-cells" - interrupt-map + - interrupts additionalProperties: false @@ -49,9 +72,14 @@ examples: #interrupt-cells = <1>; interrupt-controller; reg = <0x3000 0x20>; + + interrupt-parent = <&cpuintc>; + interrupts = <1>, <2>, <5>; + realtek,output-valid-mask = <0x13>; + #address-cells = <0>; interrupt-map = - <31 &cpuintc 2>, - <30 &cpuintc 1>, - <29 &cpuintc 5>; + <31 &cpuintc 2>, /* connect to cpuintc 2 via output 1 */ + <30 &cpuintc 1>, /* connect to cpuintc 1 via output 0 */ + <29 &cpuintc 5>; /* connect to cpuintc 5 via output 4 */ };
Amend the binding to also require a list of parent interrupts, and an optional mask to specify which parent is mapped to which output. Without this information, any driver would have to make an assumption on which parent interrupt is connected to which output. Additionally, extend (or add) the relevant descriptions to more clearly describe the inputs and outputs of this router. Signed-off-by: Sander Vanheule <sander@svanheule.net> --- Since it does not properly describe the hardware, I would still really rather get rid of "interrupt-map", even though that would mean breaking ABI for this binding. As we've argued before [1], that is our prefered solution, and would enable us to not carry more (hacky) code because of a mistake with the initial submission. Vendors don't ship independent DT blobs for devices with this hardware, so the independent devicetree/kernel upgrades issue is really rather theoretical here. Realtek isn't driving the development of the bindings and associated drivers for this platform. They have their SDK and seem to care very little about proper kernel integration. Furthermore, there are currently no device descriptions in the kernel using this binding. There are in OpenWrt, but OpenWrt firmware images for this platform always contain both the kernel and the appended DTB, so there's also no breakage to worry about. [1] https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/ Differences with v1: - Don't drop the "interrupt-map" property - Add the "realtek,output-valid-mask" property --- .../realtek,rtl-intc.yaml | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-)