Message ID | 20180820230011.25368-1-palmer@sifive.com |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | dt-bindings: riscv,cpu-intc: Cleanups from a missed review | expand |
On 8/20/18 4:01 PM, Palmer Dabbelt wrote: > I managed to miss one of Rob's code reviews on the mailing list > <http://lists.infradead.org/pipermail/linux-riscv/2018-August/001139.html>. > The patch has already been merged, so I'm submitting a fixup. > > Sorry! > > Fixes: b67bc7cb4088 ("dt-bindings: interrupt-controller: RISC-V local interrupt controller") > Cc: Rob Herring <robh@kernel.org> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Karsten Merker <merker@debian.org> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > --- > .../bindings/interrupt-controller/riscv,cpu-intc.txt | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt > index b0a8af51c388..265b223cd978 100644 > --- a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt > @@ -11,7 +11,7 @@ The RISC-V supervisor ISA manual specifies three interrupt sources that are > attached to every HLIC: software interrupts, the timer interrupt, and external > interrupts. Software interrupts are used to send IPIs between cores. The > timer interrupt comes from an architecturally mandated real-time timer that is > -controller via Supervisor Binary Interface (SBI) calls and CSR reads. External > +controlled via Supervisor Binary Interface (SBI) calls and CSR reads. External > interrupts connect all other device interrupts to the HLIC, which are routed > via the platform-level interrupt controller (PLIC). > > @@ -25,7 +25,15 @@ in the system. > > Required properties: > - compatible : "riscv,cpu-intc" Since this is a fix up patch, we should update the compatible string with the sifive specific one as well. no? Regards, Atish > -- #interrupt-cells : should be <1> > +- #interrupt-cells : should be <1>. The interrupt sources are defined by the > + RISC-V supervisor ISA manual, with only the following three interrupts being > + defined for supervisor mode: > + - Source 1 is the supervisor software interrupt, which can be sent by an SBI > + call and is reserved for use by software. > + - Source 5 is the supervisor timer interrupt, which can be configured by > + SBI calls and implements a one-shot timer. > + - Source 9 is the supervisor external interrupt, which chains to all other > + device interrupts. > - interrupt-controller : Identifies the node as an interrupt controller > > Furthermore, this interrupt-controller MUST be embedded inside the cpu > @@ -38,7 +46,7 @@ An example device tree entry for a HLIC is show below. > ... > cpu1-intc: interrupt-controller { > #interrupt-cells = <1>; > - compatible = "riscv,cpu-intc", "sifive,fu540-c000-cpu-intc"; > + compatible = "sifive,fu540-c000-cpu-intc", "riscv,cpu-intc"; > interrupt-controller; > }; > }; >
On Mon, Aug 20, 2018 at 6:10 PM Atish Patra <atish.patra@wdc.com> wrote: > > On 8/20/18 4:01 PM, Palmer Dabbelt wrote: > > I managed to miss one of Rob's code reviews on the mailing list > > <http://lists.infradead.org/pipermail/linux-riscv/2018-August/001139.html>. > > The patch has already been merged, so I'm submitting a fixup. > > > > Sorry! > > > > Fixes: b67bc7cb4088 ("dt-bindings: interrupt-controller: RISC-V local interrupt controller") > > Cc: Rob Herring <robh@kernel.org> > > Cc: Christoph Hellwig <hch@infradead.org> > > Cc: Karsten Merker <merker@debian.org> > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > --- > > .../bindings/interrupt-controller/riscv,cpu-intc.txt | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt > > index b0a8af51c388..265b223cd978 100644 > > --- a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt > > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt > > @@ -11,7 +11,7 @@ The RISC-V supervisor ISA manual specifies three interrupt sources that are > > attached to every HLIC: software interrupts, the timer interrupt, and external > > interrupts. Software interrupts are used to send IPIs between cores. The > > timer interrupt comes from an architecturally mandated real-time timer that is > > -controller via Supervisor Binary Interface (SBI) calls and CSR reads. External > > +controlled via Supervisor Binary Interface (SBI) calls and CSR reads. External > > interrupts connect all other device interrupts to the HLIC, which are routed > > via the platform-level interrupt controller (PLIC). > > > > @@ -25,7 +25,15 @@ in the system. > > > > Required properties: > > - compatible : "riscv,cpu-intc" > > Since this is a fix up patch, we should update the compatible string > with the sifive specific one as well. no? I think it is fine as is if my understanding is correct. Given this is part of the RISC-V spec(s), then using 'riscv' here for riscv,cpu-intc is fine. It was only the PLIC which didn't have any standard definition that I had issue with. Plus, with the SoC specific string, I'm not too worried about what the fallback is. However, sifive,fu540-c000-cpu-intc does need to be documented. Putting it in the example is not documenting it. Rob
On Tue, 21 Aug 2018 05:59:03 PDT (-0700), robh@kernel.org wrote: > On Mon, Aug 20, 2018 at 6:10 PM Atish Patra <atish.patra@wdc.com> wrote: >> >> On 8/20/18 4:01 PM, Palmer Dabbelt wrote: >> > I managed to miss one of Rob's code reviews on the mailing list >> > <http://lists.infradead.org/pipermail/linux-riscv/2018-August/001139.html>. >> > The patch has already been merged, so I'm submitting a fixup. >> > >> > Sorry! >> > >> > Fixes: b67bc7cb4088 ("dt-bindings: interrupt-controller: RISC-V local interrupt controller") >> > Cc: Rob Herring <robh@kernel.org> >> > Cc: Christoph Hellwig <hch@infradead.org> >> > Cc: Karsten Merker <merker@debian.org> >> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> > --- >> > .../bindings/interrupt-controller/riscv,cpu-intc.txt | 14 +++++++++++--- >> > 1 file changed, 11 insertions(+), 3 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt >> > index b0a8af51c388..265b223cd978 100644 >> > --- a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt >> > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt >> > @@ -11,7 +11,7 @@ The RISC-V supervisor ISA manual specifies three interrupt sources that are >> > attached to every HLIC: software interrupts, the timer interrupt, and external >> > interrupts. Software interrupts are used to send IPIs between cores. The >> > timer interrupt comes from an architecturally mandated real-time timer that is >> > -controller via Supervisor Binary Interface (SBI) calls and CSR reads. External >> > +controlled via Supervisor Binary Interface (SBI) calls and CSR reads. External >> > interrupts connect all other device interrupts to the HLIC, which are routed >> > via the platform-level interrupt controller (PLIC). >> > >> > @@ -25,7 +25,15 @@ in the system. >> > >> > Required properties: >> > - compatible : "riscv,cpu-intc" >> >> Since this is a fix up patch, we should update the compatible string >> with the sifive specific one as well. no? > > I think it is fine as is if my understanding is correct. Given this is > part of the RISC-V spec(s), then using 'riscv' here for riscv,cpu-intc > is fine. It was only the PLIC which didn't have any standard > definition that I had issue with. Plus, with the SoC specific string, > I'm not too worried about what the fallback is. I agree. > However, sifive,fu540-c000-cpu-intc does need to be documented. > Putting it in the example is not documenting it. Makes sense. I wrote up something quickly diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt index 265b223cd978..30c23fba9676 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt @@ -24,7 +24,14 @@ a PLIC interrupt property will typically list the HLICs for all present HARTs in the system. Required properties: -- compatible : "riscv,cpu-intc" +- compatible : should be one of + "sifive,fu540-c000-cpu-intc" + and must contain "riscv,cpu-intc". The intent here is that each + implementation of the standard RISC-V interrupt controller contains a unique + compatible string in addition to the standard string "riscv,cpu-intc". This + allows the specific implementation of the interrupt controller to be + differentiated from others in case of yet to be discovered + incompatibilities between the extant implementations. - #interrupt-cells : should be <1>. The interrupt sources are defined by the RISC-V supervisor ISA manual, with only the following three interrupts being defined for supervisor mode: but it's very much a first pass as I managed to confuse myself while writing the documentation. The central issue here is that the "riscv,cpu-intc" on the fu540-c000 is actually provided by our machine-mode firmware, so I'm not sure calling this "sifive,fu540-c000-cpu-intc" is actually the right thing to do here. It should really be called something like "ENTITY,bbl-cpu-intc-537ae11ae506" -- where that's a git hash of the BBL used, and I'm not sure what "ENTITY" should be (bbl is part of the RISC-V GitHub, but it's not part of any RISC-V spec). That naming scheme is a nightmare, though, so I don't want to suggest it :). There's then the additional wrinkle where, in the current BBL implementation, we just use the machine-mode hardware trap delegation mechanism to directly provide most of the relevant traps to Linux without actually invoking any software. As a result, that means any as-of-yet undiscovered bugs present in that particular hardware path will end up manifesting in Linux. The trouble then is that if we discover one of these bugs we'd want to fix it in the machine-mode monitor, so the actual behavior of the interrupt controller that Linux sees would end up depending on both the hardware implementation as well as the machine-mode monitor's implementation. The resulting naming scheme seems crazy. Maybe the right thing to do here is to just call this something like compatible = "sifive,fu540-c000-cpu-intc-0", "riscv,cpu-intc"; which would result in a patch that looks like diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt index 265b223cd978..36a6a06dcac4 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt @@ -24,7 +24,17 @@ a PLIC interrupt property will typically list the HLICs for all present HARTs in the system. Required properties: -- compatible : "riscv,cpu-intc" +- compatible : should be one of + "sifive,fu540-c000-cpu-intc-0" + and must contain "riscv,cpu-intc". The intent here is that each + implementation of the standard RISC-V interrupt controller contains a unique + compatible string in addition to the standard string "riscv,cpu-intc". This + allows the specific implementation of the interrupt controller to be + differentiated from others in case of yet to be discovered + incompatibilities between the extant implementations. In this case, we + indicate the exact interrupt controller implementation by providing + "fu540-c000" as the hardware implementation's identifier and "0" as the + software implementation's identifier. - #interrupt-cells : should be <1>. The interrupt sources are defined by the RISC-V supervisor ISA manual, with only the following three interrupts being defined for supervisor mode: @@ -46,7 +56,7 @@ An example device tree entry for a HLIC is show below. ... cpu1-intc: interrupt-controller { #interrupt-cells = <1>; - compatible = "sifive,fu540-c000-cpu-intc", "riscv,cpu-intc"; + compatible = "sifive,fu540-c000-cpu-intc-0", "riscv,cpu-intc"; interrupt-controller; }; }; With the idea being that at least we can figure out if we end up with a bug that can be worked around in software. > Rob Thanks for all the help!
diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt index b0a8af51c388..265b223cd978 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt @@ -11,7 +11,7 @@ The RISC-V supervisor ISA manual specifies three interrupt sources that are attached to every HLIC: software interrupts, the timer interrupt, and external interrupts. Software interrupts are used to send IPIs between cores. The timer interrupt comes from an architecturally mandated real-time timer that is -controller via Supervisor Binary Interface (SBI) calls and CSR reads. External +controlled via Supervisor Binary Interface (SBI) calls and CSR reads. External interrupts connect all other device interrupts to the HLIC, which are routed via the platform-level interrupt controller (PLIC). @@ -25,7 +25,15 @@ in the system. Required properties: - compatible : "riscv,cpu-intc" -- #interrupt-cells : should be <1> +- #interrupt-cells : should be <1>. The interrupt sources are defined by the + RISC-V supervisor ISA manual, with only the following three interrupts being + defined for supervisor mode: + - Source 1 is the supervisor software interrupt, which can be sent by an SBI + call and is reserved for use by software. + - Source 5 is the supervisor timer interrupt, which can be configured by + SBI calls and implements a one-shot timer. + - Source 9 is the supervisor external interrupt, which chains to all other + device interrupts. - interrupt-controller : Identifies the node as an interrupt controller Furthermore, this interrupt-controller MUST be embedded inside the cpu @@ -38,7 +46,7 @@ An example device tree entry for a HLIC is show below. ... cpu1-intc: interrupt-controller { #interrupt-cells = <1>; - compatible = "riscv,cpu-intc", "sifive,fu540-c000-cpu-intc"; + compatible = "sifive,fu540-c000-cpu-intc", "riscv,cpu-intc"; interrupt-controller; }; };
I managed to miss one of Rob's code reviews on the mailing list <http://lists.infradead.org/pipermail/linux-riscv/2018-August/001139.html>. The patch has already been merged, so I'm submitting a fixup. Sorry! Fixes: b67bc7cb4088 ("dt-bindings: interrupt-controller: RISC-V local interrupt controller") Cc: Rob Herring <robh@kernel.org> Cc: Christoph Hellwig <hch@infradead.org> Cc: Karsten Merker <merker@debian.org> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> --- .../bindings/interrupt-controller/riscv,cpu-intc.txt | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)