* [PATCH V5 0/3] Add thead,c900-plic support @ 2021-10-24 1:33 guoren 2021-10-24 1:33 ` [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor guoren ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: guoren @ 2021-10-24 1:33 UTC (permalink / raw) To: guoren, anup, atish.patra, maz, tglx, palmer, heiko, robh Cc: linux-kernel, linux-riscv, Guo Ren From: Guo Ren <guoren@linux.alibaba.com> Add the compatible string "thead,c900-plic" to the riscv plic bindings to support allwinner d1 SOC which contains c906 core. Changes since V5: - Move back to mask/unmask - Fixup the problem in eoi callback - Remove allwinner,sun20i-d1 IRQCHIP_DECLARE - Rewrite comment log - Add DT list - Fixup compatible string - Remove allwinner-d1 compatible - make dt_binding_check - Add T-head vendor-prefixes Changes since V4: - Update description in errata style - Update enum suggested by Anup, Heiko, Samuel - Update comment by Anup - Add cover-letter Changes since V3: - Rename "c9xx" to "c900" - Add thead,c900-plic in the description section - Add sifive_plic_chip and thead_plic_chip for difference Changes since V2: - Add a separate compatible string "thead,c9xx-plic" - set irq_mask/unmask of "plic_chip" to NULL and point irq_enable/disable of "plic_chip" to plic_irq_mask/unmask - Add a detailed comment block in plic_init() about the differences in Claim/Completion process of RISC-V PLIC and C9xx PLIC. Guo Ren (3): dt-bindings: vendor-prefixes: add T-Head Semiconductor dt-bindings: update riscv plic compatible string irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT .../sifive,plic-1.0.0.yaml | 15 +++++-- .../devicetree/bindings/vendor-prefixes.yaml | 2 + drivers/irqchip/irq-sifive-plic.c | 44 ++++++++++++++++++- 3 files changed, 56 insertions(+), 5 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor 2021-10-24 1:33 [PATCH V5 0/3] Add thead,c900-plic support guoren @ 2021-10-24 1:33 ` guoren 2021-11-02 2:21 ` Guo Ren 2021-10-24 1:33 ` [PATCH V5 2/3] dt-bindings: update riscv plic compatible string guoren 2021-10-24 1:33 ` [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT guoren 2 siblings, 1 reply; 27+ messages in thread From: guoren @ 2021-10-24 1:33 UTC (permalink / raw) To: guoren, anup, atish.patra, maz, tglx, palmer, heiko, robh Cc: linux-kernel, linux-riscv, Guo Ren, Rob Herring From: Guo Ren <guoren@linux.alibaba.com> Add vendor prefix for T-Head Semiconductor [1] [2] [1] https://github.com/T-head-Semi [2] https://www.t-head.cn/ Signed-off-by: Guo Ren <guoren@linux.alibaba.com> Cc: Rob Herring <robh@kernel.org> Cc: Rob Herring <robh+dt@kernel.org> --- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index a867f7102c35..f532a8830693 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -1169,6 +1169,8 @@ patternProperties: description: Terasic Inc. "^tfc,.*": description: Three Five Corp + "^thead,.*": + description: T-Head Semiconductor Co., Ltd. "^thine,.*": description: THine Electronics, Inc. "^thingyjp,.*": -- 2.25.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor 2021-10-24 1:33 ` [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor guoren @ 2021-11-02 2:21 ` Guo Ren 2021-11-02 12:59 ` Rob Herring 0 siblings, 1 reply; 27+ messages in thread From: Guo Ren @ 2021-11-02 2:21 UTC (permalink / raw) To: Guo Ren, Rob Herring Cc: Linux Kernel Mailing List, linux-riscv, Guo Ren, Heiko Stübner Hi Rob, ping? If there is no problem, could you help pick up this patch into your tree? On Sun, Oct 24, 2021 at 9:33 AM <guoren@kernel.org> wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > Add vendor prefix for T-Head Semiconductor [1] [2] > > [1] https://github.com/T-head-Semi > [2] https://www.t-head.cn/ > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Cc: Rob Herring <robh@kernel.org> > Cc: Rob Herring <robh+dt@kernel.org> > --- > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml > index a867f7102c35..f532a8830693 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml > @@ -1169,6 +1169,8 @@ patternProperties: > description: Terasic Inc. > "^tfc,.*": > description: Three Five Corp > + "^thead,.*": > + description: T-Head Semiconductor Co., Ltd. > "^thine,.*": > description: THine Electronics, Inc. > "^thingyjp,.*": > -- > 2.25.1 > -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor 2021-11-02 2:21 ` Guo Ren @ 2021-11-02 12:59 ` Rob Herring 2021-11-03 1:52 ` Guo Ren 0 siblings, 1 reply; 27+ messages in thread From: Rob Herring @ 2021-11-02 12:59 UTC (permalink / raw) To: Guo Ren Cc: Linux Kernel Mailing List, linux-riscv, Guo Ren, Heiko Stübner On Mon, Nov 1, 2021 at 9:21 PM Guo Ren <guoren@kernel.org> wrote: > > Hi Rob, > > ping? If there is no problem, could you help pick up this patch into your tree? Generally DT patches go with the rest of the series. And you need to send them to the DT list so that automated checks run and they are in my review queue. > > On Sun, Oct 24, 2021 at 9:33 AM <guoren@kernel.org> wrote: > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > Add vendor prefix for T-Head Semiconductor [1] [2] > > > > [1] https://github.com/T-head-Semi > > [2] https://www.t-head.cn/ > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Cc: Rob Herring <robh@kernel.org> > > Cc: Rob Herring <robh+dt@kernel.org> > > --- > > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml > > index a867f7102c35..f532a8830693 100644 > > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml > > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml > > @@ -1169,6 +1169,8 @@ patternProperties: > > description: Terasic Inc. > > "^tfc,.*": > > description: Three Five Corp > > + "^thead,.*": > > + description: T-Head Semiconductor Co., Ltd. > > "^thine,.*": > > description: THine Electronics, Inc. > > "^thingyjp,.*": > > -- > > 2.25.1 > > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor 2021-11-02 12:59 ` Rob Herring @ 2021-11-03 1:52 ` Guo Ren 0 siblings, 0 replies; 27+ messages in thread From: Guo Ren @ 2021-11-03 1:52 UTC (permalink / raw) To: Rob Herring Cc: Linux Kernel Mailing List, linux-riscv, Guo Ren, Heiko Stübner On Tue, Nov 2, 2021 at 8:59 PM Rob Herring <robh@kernel.org> wrote: > > On Mon, Nov 1, 2021 at 9:21 PM Guo Ren <guoren@kernel.org> wrote: > > > > Hi Rob, > > > > ping? If there is no problem, could you help pick up this patch into your tree? > > Generally DT patches go with the rest of the series. And you need to > send them to the DT list so that automated checks run and they are in > my review queue. This patch would be separated from these patch sets. I would send it to you separately next. > > > > > On Sun, Oct 24, 2021 at 9:33 AM <guoren@kernel.org> wrote: > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > Add vendor prefix for T-Head Semiconductor [1] [2] > > > > > > [1] https://github.com/T-head-Semi > > > [2] https://www.t-head.cn/ > > > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > Cc: Rob Herring <robh@kernel.org> > > > Cc: Rob Herring <robh+dt@kernel.org> > > > --- > > > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml > > > index a867f7102c35..f532a8830693 100644 > > > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml > > > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml > > > @@ -1169,6 +1169,8 @@ patternProperties: > > > description: Terasic Inc. > > > "^tfc,.*": > > > description: Three Five Corp > > > + "^thead,.*": > > > + description: T-Head Semiconductor Co., Ltd. > > > "^thine,.*": > > > description: THine Electronics, Inc. > > > "^thingyjp,.*": > > > -- > > > 2.25.1 > > > > > > > > > -- > > Best Regards > > Guo Ren > > > > ML: https://lore.kernel.org/linux-csky/ -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH V5 2/3] dt-bindings: update riscv plic compatible string 2021-10-24 1:33 [PATCH V5 0/3] Add thead,c900-plic support guoren 2021-10-24 1:33 ` [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor guoren @ 2021-10-24 1:33 ` guoren 2021-10-24 7:35 ` Anup Patel 2021-10-24 1:33 ` [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT guoren 2 siblings, 1 reply; 27+ messages in thread From: guoren @ 2021-10-24 1:33 UTC (permalink / raw) To: guoren, anup, atish.patra, maz, tglx, palmer, heiko, robh Cc: linux-kernel, linux-riscv, Guo Ren, Rob Herring, Palmer Dabbelt From: Guo Ren <guoren@linux.alibaba.com> Add the compatible string "thead,c900-plic" to the riscv plic bindings to support allwinner d1 SOC which contains c906 core. Signed-off-by: Guo Ren <guoren@linux.alibaba.com> Cc: Anup Patel <anup@brainfault.org> Cc: Atish Patra <atish.patra@wdc.com> Cc: Heiko Stuebner <heiko@sntech.de> Cc: Rob Herring <robh@kernel.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Palmer Dabbelt <palmerdabbelt@google.com> --- Changes since V5: - Add DT list - Fixup compatible string - Remove allwinner-d1 compatible - make dt_binding_check Changes since V4: - Update description in errata style - Update enum suggested by Anup, Heiko, Samuel Changes since V3: - Rename "c9xx" to "c900" - Add thead,c900-plic in the description section --- .../interrupt-controller/sifive,plic-1.0.0.yaml | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml index 08d5a57ce00f..18b97bfd7954 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml @@ -35,6 +35,10 @@ description: contains a specific memory layout, which is documented in chapter 8 of the SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>. + The thead,c900-plic couldn't complete masked irq source which has been disabled in + enable register. Add thead_plic_chip which fix up c906-plic irq source completion + problem by unmask/mask wrapper. + maintainers: - Sagar Kadam <sagar.kadam@sifive.com> - Paul Walmsley <paul.walmsley@sifive.com> @@ -42,11 +46,16 @@ maintainers: properties: compatible: - items: + oneOf: + - items: - enum: - - sifive,fu540-c000-plic - - canaan,k210-plic + - sifive,fu540-c000-plic + - canaan,k210-plic - const: sifive,plic-1.0.0 + - items: + - enum: + - allwinner,sun20i-d1-plic + - const: thead,c900-plic reg: maxItems: 1 -- 2.25.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string 2021-10-24 1:33 ` [PATCH V5 2/3] dt-bindings: update riscv plic compatible string guoren @ 2021-10-24 7:35 ` Anup Patel 2021-10-24 9:01 ` Guo Ren 0 siblings, 1 reply; 27+ messages in thread From: Anup Patel @ 2021-10-24 7:35 UTC (permalink / raw) To: Guo Ren Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren, Rob Herring, Palmer Dabbelt On Sun, Oct 24, 2021 at 7:03 AM <guoren@kernel.org> wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > Add the compatible string "thead,c900-plic" to the riscv plic > bindings to support allwinner d1 SOC which contains c906 core. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Cc: Anup Patel <anup@brainfault.org> > Cc: Atish Patra <atish.patra@wdc.com> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Rob Herring <robh@kernel.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Palmer Dabbelt <palmerdabbelt@google.com> > > --- > > Changes since V5: > - Add DT list > - Fixup compatible string > - Remove allwinner-d1 compatible > - make dt_binding_check > > Changes since V4: > - Update description in errata style > - Update enum suggested by Anup, Heiko, Samuel > > Changes since V3: > - Rename "c9xx" to "c900" > - Add thead,c900-plic in the description section > --- > .../interrupt-controller/sifive,plic-1.0.0.yaml | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > index 08d5a57ce00f..18b97bfd7954 100644 > --- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > @@ -35,6 +35,10 @@ description: > contains a specific memory layout, which is documented in chapter 8 of the > SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>. > > + The thead,c900-plic couldn't complete masked irq source which has been disabled in > + enable register. Add thead_plic_chip which fix up c906-plic irq source completion > + problem by unmask/mask wrapper. > + This is an incomplete description about how T-HEAD PLIC is different from RISC-V PLIC. I would suggest the following: The T-HEAD C9xx SoC implements a modified/custom T-HEAD PLIC specification which will mask current IRQ upon read to CLAIM register and will unmask the IRQ upon write to CLAIM register. The thead,c900-plic compatible string represents the custom T-HEAD PLIC specification. Regards, Anup > maintainers: > - Sagar Kadam <sagar.kadam@sifive.com> > - Paul Walmsley <paul.walmsley@sifive.com> > @@ -42,11 +46,16 @@ maintainers: > > properties: > compatible: > - items: > + oneOf: > + - items: > - enum: > - - sifive,fu540-c000-plic > - - canaan,k210-plic > + - sifive,fu540-c000-plic > + - canaan,k210-plic > - const: sifive,plic-1.0.0 > + - items: > + - enum: > + - allwinner,sun20i-d1-plic > + - const: thead,c900-plic > > reg: > maxItems: 1 > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string 2021-10-24 7:35 ` Anup Patel @ 2021-10-24 9:01 ` Guo Ren 2021-10-24 9:18 ` Anup Patel 0 siblings, 1 reply; 27+ messages in thread From: Guo Ren @ 2021-10-24 9:01 UTC (permalink / raw) To: Anup Patel Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren, Rob Herring, Palmer Dabbelt On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <anup@brainfault.org> wrote: > > On Sun, Oct 24, 2021 at 7:03 AM <guoren@kernel.org> wrote: > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > Add the compatible string "thead,c900-plic" to the riscv plic > > bindings to support allwinner d1 SOC which contains c906 core. > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Cc: Anup Patel <anup@brainfault.org> > > Cc: Atish Patra <atish.patra@wdc.com> > > Cc: Heiko Stuebner <heiko@sntech.de> > > Cc: Rob Herring <robh@kernel.org> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Palmer Dabbelt <palmerdabbelt@google.com> > > > > --- > > > > Changes since V5: > > - Add DT list > > - Fixup compatible string > > - Remove allwinner-d1 compatible > > - make dt_binding_check > > > > Changes since V4: > > - Update description in errata style > > - Update enum suggested by Anup, Heiko, Samuel > > > > Changes since V3: > > - Rename "c9xx" to "c900" > > - Add thead,c900-plic in the description section > > --- > > .../interrupt-controller/sifive,plic-1.0.0.yaml | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > index 08d5a57ce00f..18b97bfd7954 100644 > > --- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > @@ -35,6 +35,10 @@ description: > > contains a specific memory layout, which is documented in chapter 8 of the > > SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>. > > > > + The thead,c900-plic couldn't complete masked irq source which has been disabled in > > + enable register. Add thead_plic_chip which fix up c906-plic irq source completion > > + problem by unmask/mask wrapper. > > + > > This is an incomplete description about how T-HEAD PLIC is different from > RISC-V PLIC. > > I would suggest the following: > > The T-HEAD C9xx SoC implements a modified/custom T-HEAD PLIC specification > which will mask current IRQ upon read to CLAIM register and will unmask the IRQ > upon write to CLAIM register. The thead,c900-plic compatible string > represents the > custom T-HEAD PLIC specification. The patch fixup the problem that when "thead,c900-plic" couldn't complete masked irq source which has been disabled. This patch is different from the last one in that there is no relationship with the auto-mask feature. > > Regards, > Anup > > > maintainers: > > - Sagar Kadam <sagar.kadam@sifive.com> > > - Paul Walmsley <paul.walmsley@sifive.com> > > @@ -42,11 +46,16 @@ maintainers: > > > > properties: > > compatible: > > - items: > > + oneOf: > > + - items: > > - enum: > > - - sifive,fu540-c000-plic > > - - canaan,k210-plic > > + - sifive,fu540-c000-plic > > + - canaan,k210-plic > > - const: sifive,plic-1.0.0 > > + - items: > > + - enum: > > + - allwinner,sun20i-d1-plic > > + - const: thead,c900-plic > > > > reg: > > maxItems: 1 > > -- > > 2.25.1 > > -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string 2021-10-24 9:01 ` Guo Ren @ 2021-10-24 9:18 ` Anup Patel 2021-10-24 9:35 ` Guo Ren 0 siblings, 1 reply; 27+ messages in thread From: Anup Patel @ 2021-10-24 9:18 UTC (permalink / raw) To: Guo Ren Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren, Rob Herring, Palmer Dabbelt On Sun, Oct 24, 2021 at 2:31 PM Guo Ren <guoren@kernel.org> wrote: > > On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <anup@brainfault.org> wrote: > > > > On Sun, Oct 24, 2021 at 7:03 AM <guoren@kernel.org> wrote: > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > Add the compatible string "thead,c900-plic" to the riscv plic > > > bindings to support allwinner d1 SOC which contains c906 core. > > > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > Cc: Anup Patel <anup@brainfault.org> > > > Cc: Atish Patra <atish.patra@wdc.com> > > > Cc: Heiko Stuebner <heiko@sntech.de> > > > Cc: Rob Herring <robh@kernel.org> > > > Cc: Rob Herring <robh+dt@kernel.org> > > > Cc: Palmer Dabbelt <palmerdabbelt@google.com> > > > > > > --- > > > > > > Changes since V5: > > > - Add DT list > > > - Fixup compatible string > > > - Remove allwinner-d1 compatible > > > - make dt_binding_check > > > > > > Changes since V4: > > > - Update description in errata style > > > - Update enum suggested by Anup, Heiko, Samuel > > > > > > Changes since V3: > > > - Rename "c9xx" to "c900" > > > - Add thead,c900-plic in the description section > > > --- > > > .../interrupt-controller/sifive,plic-1.0.0.yaml | 15 ++++++++++++--- > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > index 08d5a57ce00f..18b97bfd7954 100644 > > > --- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > @@ -35,6 +35,10 @@ description: > > > contains a specific memory layout, which is documented in chapter 8 of the > > > SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>. > > > > > > + The thead,c900-plic couldn't complete masked irq source which has been disabled in > > > + enable register. Add thead_plic_chip which fix up c906-plic irq source completion > > > + problem by unmask/mask wrapper. > > > + > > > > This is an incomplete description about how T-HEAD PLIC is different from > > RISC-V PLIC. > > > > I would suggest the following: > > > > The T-HEAD C9xx SoC implements a modified/custom T-HEAD PLIC specification > > which will mask current IRQ upon read to CLAIM register and will unmask the IRQ > > upon write to CLAIM register. The thead,c900-plic compatible string > > represents the > > custom T-HEAD PLIC specification. > The patch fixup the problem that when "thead,c900-plic" couldn't > complete masked irq source which has been disabled. > > This patch is different from the last one in that there is no > relationship with the auto-mask feature. This patch adds compatible string for T-HEAD PLIC so it should describe how T-HEAD PLIC is different from RISC-V PLIC. The DT bindings document describes HW and not the software work-around implemented using DT bindings. Your irqchip patch uses T-HEAD PLIC compatible string to implement a work-around. In other words, this patch is different from the irqchip patch. Regards, Anup > > > > > Regards, > > Anup > > > > > maintainers: > > > - Sagar Kadam <sagar.kadam@sifive.com> > > > - Paul Walmsley <paul.walmsley@sifive.com> > > > @@ -42,11 +46,16 @@ maintainers: > > > > > > properties: > > > compatible: > > > - items: > > > + oneOf: > > > + - items: > > > - enum: > > > - - sifive,fu540-c000-plic > > > - - canaan,k210-plic > > > + - sifive,fu540-c000-plic > > > + - canaan,k210-plic > > > - const: sifive,plic-1.0.0 > > > + - items: > > > + - enum: > > > + - allwinner,sun20i-d1-plic > > > + - const: thead,c900-plic > > > > > > reg: > > > maxItems: 1 > > > -- > > > 2.25.1 > > > > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string 2021-10-24 9:18 ` Anup Patel @ 2021-10-24 9:35 ` Guo Ren 2021-10-24 9:52 ` Anup Patel 0 siblings, 1 reply; 27+ messages in thread From: Guo Ren @ 2021-10-24 9:35 UTC (permalink / raw) To: Anup Patel Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren, Rob Herring, Palmer Dabbelt On Sun, Oct 24, 2021 at 5:18 PM Anup Patel <anup@brainfault.org> wrote: > > On Sun, Oct 24, 2021 at 2:31 PM Guo Ren <guoren@kernel.org> wrote: > > > > On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <anup@brainfault.org> wrote: > > > > > > On Sun, Oct 24, 2021 at 7:03 AM <guoren@kernel.org> wrote: > > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > Add the compatible string "thead,c900-plic" to the riscv plic > > > > bindings to support allwinner d1 SOC which contains c906 core. > > > > > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > > Cc: Anup Patel <anup@brainfault.org> > > > > Cc: Atish Patra <atish.patra@wdc.com> > > > > Cc: Heiko Stuebner <heiko@sntech.de> > > > > Cc: Rob Herring <robh@kernel.org> > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > > Cc: Palmer Dabbelt <palmerdabbelt@google.com> > > > > > > > > --- > > > > > > > > Changes since V5: > > > > - Add DT list > > > > - Fixup compatible string > > > > - Remove allwinner-d1 compatible > > > > - make dt_binding_check > > > > > > > > Changes since V4: > > > > - Update description in errata style > > > > - Update enum suggested by Anup, Heiko, Samuel > > > > > > > > Changes since V3: > > > > - Rename "c9xx" to "c900" > > > > - Add thead,c900-plic in the description section > > > > --- > > > > .../interrupt-controller/sifive,plic-1.0.0.yaml | 15 ++++++++++++--- > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > > index 08d5a57ce00f..18b97bfd7954 100644 > > > > --- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > > @@ -35,6 +35,10 @@ description: > > > > contains a specific memory layout, which is documented in chapter 8 of the > > > > SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>. > > > > > > > > + The thead,c900-plic couldn't complete masked irq source which has been disabled in > > > > + enable register. Add thead_plic_chip which fix up c906-plic irq source completion > > > > + problem by unmask/mask wrapper. > > > > + > > > > > > This is an incomplete description about how T-HEAD PLIC is different from > > > RISC-V PLIC. > > > > > > I would suggest the following: > > > > > > The T-HEAD C9xx SoC implements a modified/custom T-HEAD PLIC specification > > > which will mask current IRQ upon read to CLAIM register and will unmask the IRQ > > > upon write to CLAIM register. The thead,c900-plic compatible string > > > represents the > > > custom T-HEAD PLIC specification. > > The patch fixup the problem that when "thead,c900-plic" couldn't > > complete masked irq source which has been disabled. > > > > This patch is different from the last one in that there is no > > relationship with the auto-mask feature. > > This patch adds compatible string for T-HEAD PLIC so it > should describe how T-HEAD PLIC is different from RISC-V > PLIC. The DT bindings document describes HW and not > the software work-around implemented using DT bindings. > > Your irqchip patch uses T-HEAD PLIC compatible string to > implement a work-around. > > In other words, this patch is different from the irqchip patch. How about below: The thead,c900-plic compatible string represents the custom T-HEAD PLIC specification. - It couldn't complete masked irq source which has been disabled in enable register. Add thead_plic_chip which fix up c906-plic irq source completion problem by unmask/mask wrapper. - It implements a modified/custom T-HEAD PLIC specification which will mask current IRQ upon read to CLAIM register and will unmask the IRQ upon write to CLAIM register. But the feature wasn't utilized by software. > > Regards, > Anup > > > > > > > > > Regards, > > > Anup > > > > > > > maintainers: > > > > - Sagar Kadam <sagar.kadam@sifive.com> > > > > - Paul Walmsley <paul.walmsley@sifive.com> > > > > @@ -42,11 +46,16 @@ maintainers: > > > > > > > > properties: > > > > compatible: > > > > - items: > > > > + oneOf: > > > > + - items: > > > > - enum: > > > > - - sifive,fu540-c000-plic > > > > - - canaan,k210-plic > > > > + - sifive,fu540-c000-plic > > > > + - canaan,k210-plic > > > > - const: sifive,plic-1.0.0 > > > > + - items: > > > > + - enum: > > > > + - allwinner,sun20i-d1-plic > > > > + - const: thead,c900-plic > > > > > > > > reg: > > > > maxItems: 1 > > > > -- > > > > 2.25.1 > > > > > > > > > > > > -- > > Best Regards > > Guo Ren > > > > ML: https://lore.kernel.org/linux-csky/ -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string 2021-10-24 9:35 ` Guo Ren @ 2021-10-24 9:52 ` Anup Patel 2021-10-24 10:04 ` Guo Ren 0 siblings, 1 reply; 27+ messages in thread From: Anup Patel @ 2021-10-24 9:52 UTC (permalink / raw) To: Guo Ren Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren, Rob Herring, Palmer Dabbelt On Sun, Oct 24, 2021 at 3:05 PM Guo Ren <guoren@kernel.org> wrote: > > On Sun, Oct 24, 2021 at 5:18 PM Anup Patel <anup@brainfault.org> wrote: > > > > On Sun, Oct 24, 2021 at 2:31 PM Guo Ren <guoren@kernel.org> wrote: > > > > > > On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <anup@brainfault.org> wrote: > > > > > > > > On Sun, Oct 24, 2021 at 7:03 AM <guoren@kernel.org> wrote: > > > > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > Add the compatible string "thead,c900-plic" to the riscv plic > > > > > bindings to support allwinner d1 SOC which contains c906 core. > > > > > > > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > > > Cc: Anup Patel <anup@brainfault.org> > > > > > Cc: Atish Patra <atish.patra@wdc.com> > > > > > Cc: Heiko Stuebner <heiko@sntech.de> > > > > > Cc: Rob Herring <robh@kernel.org> > > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > > > Cc: Palmer Dabbelt <palmerdabbelt@google.com> > > > > > > > > > > --- > > > > > > > > > > Changes since V5: > > > > > - Add DT list > > > > > - Fixup compatible string > > > > > - Remove allwinner-d1 compatible > > > > > - make dt_binding_check > > > > > > > > > > Changes since V4: > > > > > - Update description in errata style > > > > > - Update enum suggested by Anup, Heiko, Samuel > > > > > > > > > > Changes since V3: > > > > > - Rename "c9xx" to "c900" > > > > > - Add thead,c900-plic in the description section > > > > > --- > > > > > .../interrupt-controller/sifive,plic-1.0.0.yaml | 15 ++++++++++++--- > > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > > > index 08d5a57ce00f..18b97bfd7954 100644 > > > > > --- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > > > @@ -35,6 +35,10 @@ description: > > > > > contains a specific memory layout, which is documented in chapter 8 of the > > > > > SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>. > > > > > > > > > > + The thead,c900-plic couldn't complete masked irq source which has been disabled in > > > > > + enable register. Add thead_plic_chip which fix up c906-plic irq source completion > > > > > + problem by unmask/mask wrapper. > > > > > + > > > > > > > > This is an incomplete description about how T-HEAD PLIC is different from > > > > RISC-V PLIC. > > > > > > > > I would suggest the following: > > > > > > > > The T-HEAD C9xx SoC implements a modified/custom T-HEAD PLIC specification > > > > which will mask current IRQ upon read to CLAIM register and will unmask the IRQ > > > > upon write to CLAIM register. The thead,c900-plic compatible string > > > > represents the > > > > custom T-HEAD PLIC specification. > > > The patch fixup the problem that when "thead,c900-plic" couldn't > > > complete masked irq source which has been disabled. > > > > > > This patch is different from the last one in that there is no > > > relationship with the auto-mask feature. > > > > This patch adds compatible string for T-HEAD PLIC so it > > should describe how T-HEAD PLIC is different from RISC-V > > PLIC. The DT bindings document describes HW and not > > the software work-around implemented using DT bindings. > > > > Your irqchip patch uses T-HEAD PLIC compatible string to > > implement a work-around. > > > > In other words, this patch is different from the irqchip patch. > > How about below: > > The thead,c900-plic compatible string represents the custom T-HEAD > PLIC specification. > - It couldn't complete masked irq source which has been disabled in > enable register. Add thead_plic_chip which fix up c906-plic irq source > completion problem by unmask/mask wrapper. This first bullet is not required because it describes how it is used in irqchip driver to fix issues. This info has to go in your driver fix patch. > - It implements a modified/custom T-HEAD PLIC specification which > will mask current IRQ upon read to CLAIM register and will unmask the > IRQ upon write to CLAIM register. But the feature wasn't utilized by > software. Please don't advertise non-compliance with RISC-V PLIC spec as feature. What I had suggest before seems better. Regards, Anup > > > > > Regards, > > Anup > > > > > > > > > > > > > Regards, > > > > Anup > > > > > > > > > maintainers: > > > > > - Sagar Kadam <sagar.kadam@sifive.com> > > > > > - Paul Walmsley <paul.walmsley@sifive.com> > > > > > @@ -42,11 +46,16 @@ maintainers: > > > > > > > > > > properties: > > > > > compatible: > > > > > - items: > > > > > + oneOf: > > > > > + - items: > > > > > - enum: > > > > > - - sifive,fu540-c000-plic > > > > > - - canaan,k210-plic > > > > > + - sifive,fu540-c000-plic > > > > > + - canaan,k210-plic > > > > > - const: sifive,plic-1.0.0 > > > > > + - items: > > > > > + - enum: > > > > > + - allwinner,sun20i-d1-plic > > > > > + - const: thead,c900-plic > > > > > > > > > > reg: > > > > > maxItems: 1 > > > > > -- > > > > > 2.25.1 > > > > > > > > > > > > > > > > > -- > > > Best Regards > > > Guo Ren > > > > > > ML: https://lore.kernel.org/linux-csky/ > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string 2021-10-24 9:52 ` Anup Patel @ 2021-10-24 10:04 ` Guo Ren 0 siblings, 0 replies; 27+ messages in thread From: Guo Ren @ 2021-10-24 10:04 UTC (permalink / raw) To: Anup Patel Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren, Rob Herring, Palmer Dabbelt On Sun, Oct 24, 2021 at 5:53 PM Anup Patel <anup@brainfault.org> wrote: > > On Sun, Oct 24, 2021 at 3:05 PM Guo Ren <guoren@kernel.org> wrote: > > > > On Sun, Oct 24, 2021 at 5:18 PM Anup Patel <anup@brainfault.org> wrote: > > > > > > On Sun, Oct 24, 2021 at 2:31 PM Guo Ren <guoren@kernel.org> wrote: > > > > > > > > On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <anup@brainfault.org> wrote: > > > > > > > > > > On Sun, Oct 24, 2021 at 7:03 AM <guoren@kernel.org> wrote: > > > > > > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > > > Add the compatible string "thead,c900-plic" to the riscv plic > > > > > > bindings to support allwinner d1 SOC which contains c906 core. > > > > > > > > > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > > > > Cc: Anup Patel <anup@brainfault.org> > > > > > > Cc: Atish Patra <atish.patra@wdc.com> > > > > > > Cc: Heiko Stuebner <heiko@sntech.de> > > > > > > Cc: Rob Herring <robh@kernel.org> > > > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > > > > Cc: Palmer Dabbelt <palmerdabbelt@google.com> > > > > > > > > > > > > --- > > > > > > > > > > > > Changes since V5: > > > > > > - Add DT list > > > > > > - Fixup compatible string > > > > > > - Remove allwinner-d1 compatible > > > > > > - make dt_binding_check > > > > > > > > > > > > Changes since V4: > > > > > > - Update description in errata style > > > > > > - Update enum suggested by Anup, Heiko, Samuel > > > > > > > > > > > > Changes since V3: > > > > > > - Rename "c9xx" to "c900" > > > > > > - Add thead,c900-plic in the description section > > > > > > --- > > > > > > .../interrupt-controller/sifive,plic-1.0.0.yaml | 15 ++++++++++++--- > > > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > > > > index 08d5a57ce00f..18b97bfd7954 100644 > > > > > > --- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml > > > > > > @@ -35,6 +35,10 @@ description: > > > > > > contains a specific memory layout, which is documented in chapter 8 of the > > > > > > SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>. > > > > > > > > > > > > + The thead,c900-plic couldn't complete masked irq source which has been disabled in > > > > > > + enable register. Add thead_plic_chip which fix up c906-plic irq source completion > > > > > > + problem by unmask/mask wrapper. > > > > > > + > > > > > > > > > > This is an incomplete description about how T-HEAD PLIC is different from > > > > > RISC-V PLIC. > > > > > > > > > > I would suggest the following: > > > > > > > > > > The T-HEAD C9xx SoC implements a modified/custom T-HEAD PLIC specification > > > > > which will mask current IRQ upon read to CLAIM register and will unmask the IRQ > > > > > upon write to CLAIM register. The thead,c900-plic compatible string > > > > > represents the > > > > > custom T-HEAD PLIC specification. > > > > The patch fixup the problem that when "thead,c900-plic" couldn't > > > > complete masked irq source which has been disabled. > > > > > > > > This patch is different from the last one in that there is no > > > > relationship with the auto-mask feature. > > > > > > This patch adds compatible string for T-HEAD PLIC so it > > > should describe how T-HEAD PLIC is different from RISC-V > > > PLIC. The DT bindings document describes HW and not > > > the software work-around implemented using DT bindings. > > > > > > Your irqchip patch uses T-HEAD PLIC compatible string to > > > implement a work-around. > > > > > > In other words, this patch is different from the irqchip patch. > > > > How about below: > > > > The thead,c900-plic compatible string represents the custom T-HEAD > > PLIC specification. > > - It couldn't complete masked irq source which has been disabled in > > enable register. Add thead_plic_chip which fix up c906-plic irq source > > completion problem by unmask/mask wrapper. > > This first bullet is not required because it describes how it is used > in irqchip driver to fix issues. This info has to go in your driver fix > patch. > > > - It implements a modified/custom T-HEAD PLIC specification which > > will mask current IRQ upon read to CLAIM register and will unmask the > > IRQ upon write to CLAIM register. But the feature wasn't utilized by > > software. > > Please don't advertise non-compliance with RISC-V PLIC spec as feature. > > What I had suggest before seems better. Okay, just put these in the description in the next version. The T-HEAD C9xx SoC implements a modified/custom T-HEAD PLIC specification which will mask current IRQ upon read to CLAIM register and will unmask the IRQ upon write to CLAIM register. The thead,c900-plic compatible string represents the custom T-HEAD PLIC specification. > > Regards, > Anup > > > > > > > > > Regards, > > > Anup > > > > > > > > > > > > > > > > > Regards, > > > > > Anup > > > > > > > > > > > maintainers: > > > > > > - Sagar Kadam <sagar.kadam@sifive.com> > > > > > > - Paul Walmsley <paul.walmsley@sifive.com> > > > > > > @@ -42,11 +46,16 @@ maintainers: > > > > > > > > > > > > properties: > > > > > > compatible: > > > > > > - items: > > > > > > + oneOf: > > > > > > + - items: > > > > > > - enum: > > > > > > - - sifive,fu540-c000-plic > > > > > > - - canaan,k210-plic > > > > > > + - sifive,fu540-c000-plic > > > > > > + - canaan,k210-plic > > > > > > - const: sifive,plic-1.0.0 > > > > > > + - items: > > > > > > + - enum: > > > > > > + - allwinner,sun20i-d1-plic > > > > > > + - const: thead,c900-plic > > > > > > > > > > > > reg: > > > > > > maxItems: 1 > > > > > > -- > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > > > > -- > > > > Best Regards > > > > Guo Ren > > > > > > > > ML: https://lore.kernel.org/linux-csky/ > > > > > > > > -- > > Best Regards > > Guo Ren > > > > ML: https://lore.kernel.org/linux-csky/ -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT 2021-10-24 1:33 [PATCH V5 0/3] Add thead,c900-plic support guoren 2021-10-24 1:33 ` [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor guoren 2021-10-24 1:33 ` [PATCH V5 2/3] dt-bindings: update riscv plic compatible string guoren @ 2021-10-24 1:33 ` guoren 2021-10-25 10:48 ` Marc Zyngier 2 siblings, 1 reply; 27+ messages in thread From: guoren @ 2021-10-24 1:33 UTC (permalink / raw) To: guoren, anup, atish.patra, maz, tglx, palmer, heiko, robh Cc: linux-kernel, linux-riscv, Guo Ren From: Guo Ren <guoren@linux.alibaba.com> When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver, only the first interrupt could be handled, and continue irq is blocked by hw. Because the thead,c900-plic couldn't complete masked irq source which has been disabled in enable register. Add thead_plic_chip which fix up c906-plic irq source completion problem by unmask/mask wrapper. Here is the description of Interrupt Completion in PLIC spec [1]: The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ completion is silently ignored. [1] https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adoc Signed-off-by: Guo Ren <guoren@linux.alibaba.com> Cc: Anup Patel <anup@brainfault.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Marc Zyngier <maz@kernel.org> Cc: Palmer Dabbelt <palmer@dabbelt.com> Cc: Atish Patra <atish.patra@wdc.com> --- Changes since V5: - Move back to mask/unmask - Fixup the problem in eoi callback - Remove allwinner,sun20i-d1 IRQCHIP_DECLARE - Rewrite comment log Changes since V4: - Update comment by Anup Changes since V3: - Rename "c9xx" to "c900" - Add sifive_plic_chip and thead_plic_chip for difference Changes since V2: - Add a separate compatible string "thead,c9xx-plic" - set irq_mask/unmask of "plic_chip" to NULL and point irq_enable/disable of "plic_chip" to plic_irq_mask/unmask - Add a detailed comment block in plic_init() about the differences in Claim/Completion process of RISC-V PLIC and C9xx PLIC. --- drivers/irqchip/irq-sifive-plic.c | 44 +++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index cf74cfa82045..7c64a7ab56f3 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -166,7 +166,7 @@ static void plic_irq_eoi(struct irq_data *d) writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); } -static struct irq_chip plic_chip = { +static struct irq_chip sifive_plic_chip = { .name = "SiFive PLIC", .irq_mask = plic_irq_mask, .irq_unmask = plic_irq_unmask, @@ -176,12 +176,43 @@ static struct irq_chip plic_chip = { #endif }; +/* + * Current C9xx PLIC can't complete interrupt when the interrupt + * source is currently disabled for the target. Re-enable the + * interrupt source by unmasking to let hw irq source completion + * succeed. + */ +static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler = this_cpu_ptr(&plic_handlers); + + if (irqd_irq_masked(d)) { + plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); + plic_irq_mask(d); + } else { + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); + } +} + +static struct irq_chip thead_plic_chip = { + .name = "T-Head PLIC", + .irq_mask = plic_irq_mask, + .irq_unmask = plic_irq_unmask, + .irq_eoi = plic_thead_irq_eoi, +#ifdef CONFIG_SMP + .irq_set_affinity = plic_set_affinity, +#endif +}; + +static struct irq_chip *def_plic_chip = &sifive_plic_chip; + static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq) { struct plic_priv *priv = d->host_data; - irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data, + irq_domain_set_info(d, irq, hwirq, def_plic_chip, d->host_data, handle_fasteoi_irq, NULL, NULL); irq_set_noprobe(irq); irq_set_affinity(irq, &priv->lmask); @@ -390,5 +421,14 @@ static int __init plic_init(struct device_node *node, return error; } +static int __init thead_c900_plic_init(struct device_node *node, + struct device_node *parent) +{ + def_plic_chip = &thead_plic_chip; + + return plic_init(node, parent); +} + IRQCHIP_DECLARE(sifive_plic, "sifive,plic-1.0.0", plic_init); IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */ +IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", thead_c900_plic_init); -- 2.25.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT 2021-10-24 1:33 ` [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT guoren @ 2021-10-25 10:48 ` Marc Zyngier 2021-10-25 13:33 ` Guo Ren 2021-10-28 10:55 ` [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic " Nikita Shubin 0 siblings, 2 replies; 27+ messages in thread From: Marc Zyngier @ 2021-10-25 10:48 UTC (permalink / raw) To: guoren Cc: anup, atish.patra, tglx, palmer, heiko, robh, linux-kernel, linux-riscv, Guo Ren On Sun, 24 Oct 2021 02:33:03 +0100, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver, > only the first interrupt could be handled, and continue irq is blocked by > hw. Because the thead,c900-plic couldn't complete masked irq source which > has been disabled in enable register. Add thead_plic_chip which fix up > c906-plic irq source completion problem by unmask/mask wrapper. > > Here is the description of Interrupt Completion in PLIC spec [1]: > > The PLIC signals it has completed executing an interrupt handler by > writing the interrupt ID it received from the claim to the claim/complete > register. The PLIC does not check whether the completion ID is the same > as the last claim ID for that target. If the completion ID does not match > an interrupt source that is currently enabled for the target, the > ^^ ^^^^^^^^^ ^^^^^^^ > completion is silently ignored. Given this bit of the spec... > +static void plic_thead_irq_eoi(struct irq_data *d) > +{ > + struct plic_handler *handler = this_cpu_ptr(&plic_handlers); > + > + if (irqd_irq_masked(d)) { > + plic_irq_unmask(d); > + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); > + plic_irq_mask(d); > + } else { > + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); > + } > +} > + ... it isn't obvious to me why this cannot happen on an SiFive PLIC. And it isn't only for threaded interrupts in oneshot mode. Any driver can mask an interrupt from its handler after having set the IRQ_DISABLE_UNLAZY flag, and the interrupt would need the exact same treatment. M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT 2021-10-25 10:48 ` Marc Zyngier @ 2021-10-25 13:33 ` Guo Ren 2021-10-28 10:55 ` [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic " Nikita Shubin 1 sibling, 0 replies; 27+ messages in thread From: Guo Ren @ 2021-10-25 13:33 UTC (permalink / raw) To: Marc Zyngier Cc: Anup Patel, Atish Patra, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, Linux Kernel Mailing List, linux-riscv, Guo Ren On Mon, Oct 25, 2021 at 6:48 PM Marc Zyngier <maz@kernel.org> wrote: > > On Sun, 24 Oct 2021 02:33:03 +0100, > guoren@kernel.org wrote: > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver, > > only the first interrupt could be handled, and continue irq is blocked by > > hw. Because the thead,c900-plic couldn't complete masked irq source which > > has been disabled in enable register. Add thead_plic_chip which fix up > > c906-plic irq source completion problem by unmask/mask wrapper. > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > The PLIC signals it has completed executing an interrupt handler by > > writing the interrupt ID it received from the claim to the claim/complete > > register. The PLIC does not check whether the completion ID is the same > > as the last claim ID for that target. If the completion ID does not match > > an interrupt source that is currently enabled for the target, the > > ^^ ^^^^^^^^^ ^^^^^^^ > > completion is silently ignored. > > Given this bit of the spec... > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > +{ > > + struct plic_handler *handler = this_cpu_ptr(&plic_handlers); > > + > > + if (irqd_irq_masked(d)) { > > + plic_irq_unmask(d); > > + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); > > + plic_irq_mask(d); > > + } else { > > + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); > > + } > > +} > > + > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. I'm not sure about SiFive PLIC. Maybe they didn't follow that to implement. > > And it isn't only for threaded interrupts in oneshot mode. Any driver > can mask an interrupt from its handler after having set the > IRQ_DISABLE_UNLAZY flag, and the interrupt would need the exact same > treatment. Thx for mentioned here, and I'll add it in the comment of next version patch. > > M. > > -- > Without deviation from the norm, progress is not possible. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-10-25 10:48 ` Marc Zyngier 2021-10-25 13:33 ` Guo Ren @ 2021-10-28 10:55 ` Nikita Shubin 2021-10-28 14:58 ` Marc Zyngier ` (2 more replies) 1 sibling, 3 replies; 27+ messages in thread From: Nikita Shubin @ 2021-10-28 10:55 UTC (permalink / raw) To: Marc Zyngier Cc: guoren, anup, atish.patra, tglx, palmer, heiko, robh, linux-kernel, linux-riscv, Guo Ren Hello Marc and Guo Ren! On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <maz@kernel.org> wrote: > On Sun, 24 Oct 2021 02:33:03 +0100, > guoren@kernel.org wrote: > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the > > driver, only the first interrupt could be handled, and continue irq > > is blocked by hw. Because the thead,c900-plic couldn't complete > > masked irq source which has been disabled in enable register. Add > > thead_plic_chip which fix up c906-plic irq source completion > > problem by unmask/mask wrapper. > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > The PLIC signals it has completed executing an interrupt handler by > > writing the interrupt ID it received from the claim to the > > claim/complete register. The PLIC does not check whether the > > completion ID is the same as the last claim ID for that target. If > > the completion ID does not match an interrupt source that is > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ > > completion is silently ignored. > > Given this bit of the spec... > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > +{ > > + struct plic_handler *handler = > > this_cpu_ptr(&plic_handlers); + > > + if (irqd_irq_masked(d)) { > > + plic_irq_unmask(d); > > + writel(d->hwirq, handler->hart_base + > > CONTEXT_CLAIM); > > + plic_irq_mask(d); > > + } else { > > + writel(d->hwirq, handler->hart_base + > > CONTEXT_CLAIM); > > + } > > +} > > + > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. This indeed happens with SiFive PLIC. I am currently tinkering with da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However with changes proposed by Guo Ren in plic_thead_irq_eoi, everything begins to work fine. May be these change should be propagated to plic_irq_eoi instead of making a new function ? > > And it isn't only for threaded interrupts in oneshot mode. Any driver > can mask an interrupt from its handler after having set the > IRQ_DISABLE_UNLAZY flag, and the interrupt would need the exact same > treatment. > > M. > --- diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index cf74cfa82045..259065d271ef 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -163,7 +163,13 @@ static void plic_irq_eoi(struct irq_data *d) { struct plic_handler *handler = this_cpu_ptr(&plic_handlers); - writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); + if (irqd_irq_masked(d)) { + plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); + plic_irq_mask(d); + } else { + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); + } } static struct irq_chip plic_chip = { ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-10-28 10:55 ` [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic " Nikita Shubin @ 2021-10-28 14:58 ` Marc Zyngier 2021-10-30 10:27 ` Anup Patel 2021-11-01 2:20 ` Guo Ren 2021-11-01 2:00 ` Guo Ren 2021-11-01 5:11 ` Vincent Pelletier 2 siblings, 2 replies; 27+ messages in thread From: Marc Zyngier @ 2021-10-28 14:58 UTC (permalink / raw) To: Nikita Shubin Cc: guoren, anup, atish.patra, tglx, palmer, heiko, robh, linux-kernel, linux-riscv, Guo Ren On Thu, 28 Oct 2021 11:55:23 +0100, Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > Hello Marc and Guo Ren! > > On Mon, 25 Oct 2021 11:48:33 +0100 > Marc Zyngier <maz@kernel.org> wrote: > > > On Sun, 24 Oct 2021 02:33:03 +0100, > > guoren@kernel.org wrote: > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the > > > driver, only the first interrupt could be handled, and continue irq > > > is blocked by hw. Because the thead,c900-plic couldn't complete > > > masked irq source which has been disabled in enable register. Add > > > thead_plic_chip which fix up c906-plic irq source completion > > > problem by unmask/mask wrapper. > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > > > The PLIC signals it has completed executing an interrupt handler by > > > writing the interrupt ID it received from the claim to the > > > claim/complete register. The PLIC does not check whether the > > > completion ID is the same as the last claim ID for that target. If > > > the completion ID does not match an interrupt source that is > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ > > > completion is silently ignored. > > > > Given this bit of the spec... > > > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > > +{ > > > + struct plic_handler *handler = > > > this_cpu_ptr(&plic_handlers); + > > > + if (irqd_irq_masked(d)) { > > > + plic_irq_unmask(d); > > > + writel(d->hwirq, handler->hart_base + > > > CONTEXT_CLAIM); > > > + plic_irq_mask(d); > > > + } else { > > > + writel(d->hwirq, handler->hart_base + > > > CONTEXT_CLAIM); > > > + } > > > +} > > > + > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. > > This indeed happens with SiFive PLIC. I am currently tinkering with > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything > begins to work fine. > > May be these change should be propagated to plic_irq_eoi instead of > making a new function ? That's my impression too. I think the T-Head defect is pretty much immaterial when you consider how 'interesting' the PLIC architecture is. Conflating EOI and masking really is a misfeature... M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-10-28 14:58 ` Marc Zyngier @ 2021-10-30 10:27 ` Anup Patel 2021-11-01 2:20 ` Guo Ren 1 sibling, 0 replies; 27+ messages in thread From: Anup Patel @ 2021-10-30 10:27 UTC (permalink / raw) To: Marc Zyngier Cc: Nikita Shubin, Guo Ren, Atish Patra, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren On Thu, Oct 28, 2021 at 8:28 PM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 28 Oct 2021 11:55:23 +0100, > Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > > > Hello Marc and Guo Ren! > > > > On Mon, 25 Oct 2021 11:48:33 +0100 > > Marc Zyngier <maz@kernel.org> wrote: > > > > > On Sun, 24 Oct 2021 02:33:03 +0100, > > > guoren@kernel.org wrote: > > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the > > > > driver, only the first interrupt could be handled, and continue irq > > > > is blocked by hw. Because the thead,c900-plic couldn't complete > > > > masked irq source which has been disabled in enable register. Add > > > > thead_plic_chip which fix up c906-plic irq source completion > > > > problem by unmask/mask wrapper. > > > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > > > > > The PLIC signals it has completed executing an interrupt handler by > > > > writing the interrupt ID it received from the claim to the > > > > claim/complete register. The PLIC does not check whether the > > > > completion ID is the same as the last claim ID for that target. If > > > > the completion ID does not match an interrupt source that is > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ > > > > completion is silently ignored. > > > > > > Given this bit of the spec... > > > > > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > > > +{ > > > > + struct plic_handler *handler = > > > > this_cpu_ptr(&plic_handlers); + > > > > + if (irqd_irq_masked(d)) { > > > > + plic_irq_unmask(d); > > > > + writel(d->hwirq, handler->hart_base + > > > > CONTEXT_CLAIM); > > > > + plic_irq_mask(d); > > > > + } else { > > > > + writel(d->hwirq, handler->hart_base + > > > > CONTEXT_CLAIM); > > > > + } > > > > +} > > > > + > > > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. > > > > This indeed happens with SiFive PLIC. I am currently tinkering with > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything > > begins to work fine. > > > > May be these change should be propagated to plic_irq_eoi instead of > > making a new function ? > > That's my impression too. I think the T-Head defect is pretty much > immaterial when you consider how 'interesting' the PLIC architecture > is. Conflating EOI and masking really is a misfeature... Unfortunately, the PLIC implementation is the same across existing boards (except T-HEAD) so this issue is there on all existing boards. I double checked the upcoming RISC-V AIA specification and this problem is not there in RISC-V AIA because the interrupt claim/completion is different. (Refer, https://github.com/riscv/riscv-aia/releases/download/0.2-draft.27/riscv-interrupts-027.pdf) I plan to send-out RFC PATCH for AIA soon so that we get early feedback from everyone on LKML. Regards, Anup > > M. > > -- > Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-10-28 14:58 ` Marc Zyngier 2021-10-30 10:27 ` Anup Patel @ 2021-11-01 2:20 ` Guo Ren 2021-11-01 2:53 ` Anup Patel 2021-11-01 9:25 ` Marc Zyngier 1 sibling, 2 replies; 27+ messages in thread From: Guo Ren @ 2021-11-01 2:20 UTC (permalink / raw) To: Marc Zyngier Cc: Nikita Shubin, Anup Patel, Atish Patra, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, Linux Kernel Mailing List, linux-riscv, Guo Ren On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 28 Oct 2021 11:55:23 +0100, > Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > > > Hello Marc and Guo Ren! > > > > On Mon, 25 Oct 2021 11:48:33 +0100 > > Marc Zyngier <maz@kernel.org> wrote: > > > > > On Sun, 24 Oct 2021 02:33:03 +0100, > > > guoren@kernel.org wrote: > > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the > > > > driver, only the first interrupt could be handled, and continue irq > > > > is blocked by hw. Because the thead,c900-plic couldn't complete > > > > masked irq source which has been disabled in enable register. Add > > > > thead_plic_chip which fix up c906-plic irq source completion > > > > problem by unmask/mask wrapper. > > > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > > > > > The PLIC signals it has completed executing an interrupt handler by > > > > writing the interrupt ID it received from the claim to the > > > > claim/complete register. The PLIC does not check whether the > > > > completion ID is the same as the last claim ID for that target. If > > > > the completion ID does not match an interrupt source that is > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ > > > > completion is silently ignored. > > > > > > Given this bit of the spec... > > > > > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > > > +{ > > > > + struct plic_handler *handler = > > > > this_cpu_ptr(&plic_handlers); + > > > > + if (irqd_irq_masked(d)) { > > > > + plic_irq_unmask(d); > > > > + writel(d->hwirq, handler->hart_base + > > > > CONTEXT_CLAIM); > > > > + plic_irq_mask(d); > > > > + } else { > > > > + writel(d->hwirq, handler->hart_base + > > > > CONTEXT_CLAIM); > > > > + } > > > > +} > > > > + > > > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. > > > > This indeed happens with SiFive PLIC. I am currently tinkering with > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything > > begins to work fine. > > > > May be these change should be propagated to plic_irq_eoi instead of > > making a new function ? > > That's my impression too. I think the T-Head defect is pretty much > immaterial when you consider how 'interesting' the PLIC architecture > is. Which is the "T-Head defect" you mentioned here? 1. Auto masking with claim + complete (I don't think it's a defect, right? May I add a new patch to utilize the feature to decrease a little duplicate mask/unmask operations in the future?) 2. EOI failed when masked > Conflating EOI and masking really is a misfeature... I think the problem is riscv PLIC reuse enable bit as mask bit. I recommend separating them. That means: - EOI still depends on enable bit. - Add mask/unmask bit regs to do the right thing. > > M. > > -- > Without deviation from the norm, progress is not possible. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-11-01 2:20 ` Guo Ren @ 2021-11-01 2:53 ` Anup Patel 2021-11-01 3:57 ` Guo Ren 2021-11-01 9:25 ` Marc Zyngier 1 sibling, 1 reply; 27+ messages in thread From: Anup Patel @ 2021-11-01 2:53 UTC (permalink / raw) To: Guo Ren Cc: Marc Zyngier, Nikita Shubin, Atish Patra, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, Linux Kernel Mailing List, linux-riscv, Guo Ren On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <guoren@kernel.org> wrote: > > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Thu, 28 Oct 2021 11:55:23 +0100, > > Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > > > > > Hello Marc and Guo Ren! > > > > > > On Mon, 25 Oct 2021 11:48:33 +0100 > > > Marc Zyngier <maz@kernel.org> wrote: > > > > > > > On Sun, 24 Oct 2021 02:33:03 +0100, > > > > guoren@kernel.org wrote: > > > > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the > > > > > driver, only the first interrupt could be handled, and continue irq > > > > > is blocked by hw. Because the thead,c900-plic couldn't complete > > > > > masked irq source which has been disabled in enable register. Add > > > > > thead_plic_chip which fix up c906-plic irq source completion > > > > > problem by unmask/mask wrapper. > > > > > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > > > > > > > The PLIC signals it has completed executing an interrupt handler by > > > > > writing the interrupt ID it received from the claim to the > > > > > claim/complete register. The PLIC does not check whether the > > > > > completion ID is the same as the last claim ID for that target. If > > > > > the completion ID does not match an interrupt source that is > > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ > > > > > completion is silently ignored. > > > > > > > > Given this bit of the spec... > > > > > > > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > > > > +{ > > > > > + struct plic_handler *handler = > > > > > this_cpu_ptr(&plic_handlers); + > > > > > + if (irqd_irq_masked(d)) { > > > > > + plic_irq_unmask(d); > > > > > + writel(d->hwirq, handler->hart_base + > > > > > CONTEXT_CLAIM); > > > > > + plic_irq_mask(d); > > > > > + } else { > > > > > + writel(d->hwirq, handler->hart_base + > > > > > CONTEXT_CLAIM); > > > > > + } > > > > > +} > > > > > + > > > > > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. > > > > > > This indeed happens with SiFive PLIC. I am currently tinkering with > > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However > > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything > > > begins to work fine. > > > > > > May be these change should be propagated to plic_irq_eoi instead of > > > making a new function ? > > > > That's my impression too. I think the T-Head defect is pretty much > > immaterial when you consider how 'interesting' the PLIC architecture > > is. > Which is the "T-Head defect" you mentioned here? > 1. Auto masking with claim + complete (I don't think it's a defect, > right? May I add a new patch to utilize the feature to decrease a > little duplicate mask/unmask operations in the future?) This is definitely a defect and non-compliance for T-HEAD because no sane interrupt controller would mask interrupt upon claim and this is not what RISC-V PLIC defines. > 2. EOI failed when masked This defect exists for both RISC-V PLIC and T-HEAD PLIC because of the way interrupt completion is defined. > > > Conflating EOI and masking really is a misfeature... > I think the problem is riscv PLIC reuse enable bit as mask bit. I > recommend separating them. That means: There are no per-interrupt mask bits. We only have per-context and per-interrupt enable bits which is used to provide mask/unmask functionality expected by the irqchip framework. I don't see how this is a problem for RISC-V PLIC. The only real issue with RISC-V PLIC is the fact the interrupt completion will be ignored for a masked interrupt which is what Marc is pointing at. Regards, Anup > - EOI still depends on enable bit. > - Add mask/unmask bit regs to do the right thing. > > > > > M. > > > > -- > > Without deviation from the norm, progress is not possible. > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-11-01 2:53 ` Anup Patel @ 2021-11-01 3:57 ` Guo Ren 2021-11-01 4:27 ` Anup Patel 0 siblings, 1 reply; 27+ messages in thread From: Guo Ren @ 2021-11-01 3:57 UTC (permalink / raw) To: Anup Patel Cc: Marc Zyngier, Nikita Shubin, Atish Patra, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, Linux Kernel Mailing List, linux-riscv, Guo Ren On Mon, Nov 1, 2021 at 10:53 AM Anup Patel <anup@brainfault.org> wrote: > > On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <guoren@kernel.org> wrote: > > > > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Thu, 28 Oct 2021 11:55:23 +0100, > > > Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > > > > > > > Hello Marc and Guo Ren! > > > > > > > > On Mon, 25 Oct 2021 11:48:33 +0100 > > > > Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > On Sun, 24 Oct 2021 02:33:03 +0100, > > > > > guoren@kernel.org wrote: > > > > > > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the > > > > > > driver, only the first interrupt could be handled, and continue irq > > > > > > is blocked by hw. Because the thead,c900-plic couldn't complete > > > > > > masked irq source which has been disabled in enable register. Add > > > > > > thead_plic_chip which fix up c906-plic irq source completion > > > > > > problem by unmask/mask wrapper. > > > > > > > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > > > > > > > > > The PLIC signals it has completed executing an interrupt handler by > > > > > > writing the interrupt ID it received from the claim to the > > > > > > claim/complete register. The PLIC does not check whether the > > > > > > completion ID is the same as the last claim ID for that target. If > > > > > > the completion ID does not match an interrupt source that is > > > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ > > > > > > completion is silently ignored. > > > > > > > > > > Given this bit of the spec... > > > > > > > > > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > > > > > +{ > > > > > > + struct plic_handler *handler = > > > > > > this_cpu_ptr(&plic_handlers); + > > > > > > + if (irqd_irq_masked(d)) { > > > > > > + plic_irq_unmask(d); > > > > > > + writel(d->hwirq, handler->hart_base + > > > > > > CONTEXT_CLAIM); > > > > > > + plic_irq_mask(d); > > > > > > + } else { > > > > > > + writel(d->hwirq, handler->hart_base + > > > > > > CONTEXT_CLAIM); > > > > > > + } > > > > > > +} > > > > > > + > > > > > > > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. > > > > > > > > This indeed happens with SiFive PLIC. I am currently tinkering with > > > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However > > > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything > > > > begins to work fine. > > > > > > > > May be these change should be propagated to plic_irq_eoi instead of > > > > making a new function ? > > > > > > That's my impression too. I think the T-Head defect is pretty much > > > immaterial when you consider how 'interesting' the PLIC architecture > > > is. > > Which is the "T-Head defect" you mentioned here? > > 1. Auto masking with claim + complete (I don't think it's a defect, > > right? May I add a new patch to utilize the feature to decrease a > > little duplicate mask/unmask operations in the future?) > > This is definitely a defect and non-compliance for T-HEAD because I just agree with non-compliance, but what's the defect of auto-masking? If somebody could explain, I'm very grateful. > no sane interrupt controller would mask interrupt upon claim and this > is not what RISC-V PLIC defines. > > > 2. EOI failed when masked > > This defect exists for both RISC-V PLIC and T-HEAD PLIC > because of the way interrupt completion is defined. > > > > > > Conflating EOI and masking really is a misfeature... > > I think the problem is riscv PLIC reuse enable bit as mask bit. I > > recommend separating them. That means: > > There are no per-interrupt mask bits. We only have per-context > and per-interrupt enable bits which is used to provide mask/unmask > functionality expected by the irqchip framework. > > I don't see how this is a problem for RISC-V PLIC. The only real > issue with RISC-V PLIC is the fact the interrupt completion will be > ignored for a masked interrupt which is what Marc is pointing at. So you are not considering add per-interrupt mask bits to solve the above problem, right? I don't think you would keep below codes in AIA eoi. + plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); + plic_irq_mask(d); > > Regards, > Anup > > > - EOI still depends on enable bit. > > - Add mask/unmask bit regs to do the right thing. > > > > > > > > > > > M. > > > > > > -- > > > Without deviation from the norm, progress is not possible. > > > > > > > > -- > > Best Regards > > Guo Ren > > > > ML: https://lore.kernel.org/linux-csky/ -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-11-01 3:57 ` Guo Ren @ 2021-11-01 4:27 ` Anup Patel 2021-11-01 7:56 ` Guo Ren 2021-11-01 9:27 ` Marc Zyngier 0 siblings, 2 replies; 27+ messages in thread From: Anup Patel @ 2021-11-01 4:27 UTC (permalink / raw) To: Guo Ren Cc: Marc Zyngier, Nikita Shubin, Atish Patra, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, Linux Kernel Mailing List, linux-riscv, Guo Ren On Mon, Nov 1, 2021 at 9:27 AM Guo Ren <guoren@kernel.org> wrote: > > On Mon, Nov 1, 2021 at 10:53 AM Anup Patel <anup@brainfault.org> wrote: > > > > On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <guoren@kernel.org> wrote: > > > > > > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Thu, 28 Oct 2021 11:55:23 +0100, > > > > Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > > > > > > > > > Hello Marc and Guo Ren! > > > > > > > > > > On Mon, 25 Oct 2021 11:48:33 +0100 > > > > > Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > On Sun, 24 Oct 2021 02:33:03 +0100, > > > > > > guoren@kernel.org wrote: > > > > > > > > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the > > > > > > > driver, only the first interrupt could be handled, and continue irq > > > > > > > is blocked by hw. Because the thead,c900-plic couldn't complete > > > > > > > masked irq source which has been disabled in enable register. Add > > > > > > > thead_plic_chip which fix up c906-plic irq source completion > > > > > > > problem by unmask/mask wrapper. > > > > > > > > > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > > > > > > > > > > > The PLIC signals it has completed executing an interrupt handler by > > > > > > > writing the interrupt ID it received from the claim to the > > > > > > > claim/complete register. The PLIC does not check whether the > > > > > > > completion ID is the same as the last claim ID for that target. If > > > > > > > the completion ID does not match an interrupt source that is > > > > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ > > > > > > > completion is silently ignored. > > > > > > > > > > > > Given this bit of the spec... > > > > > > > > > > > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > > > > > > +{ > > > > > > > + struct plic_handler *handler = > > > > > > > this_cpu_ptr(&plic_handlers); + > > > > > > > + if (irqd_irq_masked(d)) { > > > > > > > + plic_irq_unmask(d); > > > > > > > + writel(d->hwirq, handler->hart_base + > > > > > > > CONTEXT_CLAIM); > > > > > > > + plic_irq_mask(d); > > > > > > > + } else { > > > > > > > + writel(d->hwirq, handler->hart_base + > > > > > > > CONTEXT_CLAIM); > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > > > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. > > > > > > > > > > This indeed happens with SiFive PLIC. I am currently tinkering with > > > > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However > > > > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything > > > > > begins to work fine. > > > > > > > > > > May be these change should be propagated to plic_irq_eoi instead of > > > > > making a new function ? > > > > > > > > That's my impression too. I think the T-Head defect is pretty much > > > > immaterial when you consider how 'interesting' the PLIC architecture > > > > is. > > > Which is the "T-Head defect" you mentioned here? > > > 1. Auto masking with claim + complete (I don't think it's a defect, > > > right? May I add a new patch to utilize the feature to decrease a > > > little duplicate mask/unmask operations in the future?) > > > > This is definitely a defect and non-compliance for T-HEAD because > I just agree with non-compliance, but what's the defect of > auto-masking? If somebody could explain, I'm very grateful. > > > no sane interrupt controller would mask interrupt upon claim and this > > is not what RISC-V PLIC defines. > > > > > 2. EOI failed when masked > > > > This defect exists for both RISC-V PLIC and T-HEAD PLIC > > because of the way interrupt completion is defined. > > > > > > > > > Conflating EOI and masking really is a misfeature... > > > I think the problem is riscv PLIC reuse enable bit as mask bit. I > > > recommend separating them. That means: > > > > There are no per-interrupt mask bits. We only have per-context > > and per-interrupt enable bits which is used to provide mask/unmask > > functionality expected by the irqchip framework. > > > > I don't see how this is a problem for RISC-V PLIC. The only real > > issue with RISC-V PLIC is the fact the interrupt completion will be > > ignored for a masked interrupt which is what Marc is pointing at. > So you are not considering add per-interrupt mask bits to solve the > above problem, right? The RISC-V PLIC has several limitations and also lacks a lot of features hence it's marked as deprecated in RISC-V platform specs and will be removed eventually from RISC-V platform specs. The RISC-V AIA will totally replace RISC-V PLIC going forward. In fact, RISC-V AIA APLIC addresses all limitations of RISC-V PLIC along with new features additions. > > I don't think you would keep below codes in AIA eoi. > + plic_irq_unmask(d); > + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); > + plic_irq_mask(d); Like I mentioned previously, the AIA APLIC is very different from the PLIC so we don't need this mask/unmask dance over there. It has global per-interrupt enable bits in AIA APLIC which is different from PLIC. Regards, Anup > > > > > Regards, > > Anup > > > > > - EOI still depends on enable bit. > > > - Add mask/unmask bit regs to do the right thing. > > > > > > > > > > > > > > > > > M. > > > > > > > > -- > > > > Without deviation from the norm, progress is not possible. > > > > > > > > > > > > -- > > > Best Regards > > > Guo Ren > > > > > > ML: https://lore.kernel.org/linux-csky/ > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-11-01 4:27 ` Anup Patel @ 2021-11-01 7:56 ` Guo Ren 2021-11-01 9:27 ` Marc Zyngier 1 sibling, 0 replies; 27+ messages in thread From: Guo Ren @ 2021-11-01 7:56 UTC (permalink / raw) To: Anup Patel Cc: Marc Zyngier, Nikita Shubin, Atish Patra, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, Linux Kernel Mailing List, linux-riscv, Guo Ren On Mon, Nov 1, 2021 at 12:28 PM Anup Patel <anup@brainfault.org> wrote: > > On Mon, Nov 1, 2021 at 9:27 AM Guo Ren <guoren@kernel.org> wrote: > > > > On Mon, Nov 1, 2021 at 10:53 AM Anup Patel <anup@brainfault.org> wrote: > > > > > > On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <guoren@kernel.org> wrote: > > > > > > > > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Thu, 28 Oct 2021 11:55:23 +0100, > > > > > Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > > > > > > > > > > > Hello Marc and Guo Ren! > > > > > > > > > > > > On Mon, 25 Oct 2021 11:48:33 +0100 > > > > > > Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > > > On Sun, 24 Oct 2021 02:33:03 +0100, > > > > > > > guoren@kernel.org wrote: > > > > > > > > > > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the > > > > > > > > driver, only the first interrupt could be handled, and continue irq > > > > > > > > is blocked by hw. Because the thead,c900-plic couldn't complete > > > > > > > > masked irq source which has been disabled in enable register. Add > > > > > > > > thead_plic_chip which fix up c906-plic irq source completion > > > > > > > > problem by unmask/mask wrapper. > > > > > > > > > > > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > > > > > > > > > > > > > The PLIC signals it has completed executing an interrupt handler by > > > > > > > > writing the interrupt ID it received from the claim to the > > > > > > > > claim/complete register. The PLIC does not check whether the > > > > > > > > completion ID is the same as the last claim ID for that target. If > > > > > > > > the completion ID does not match an interrupt source that is > > > > > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ > > > > > > > > completion is silently ignored. > > > > > > > > > > > > > > Given this bit of the spec... > > > > > > > > > > > > > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > > > > > > > +{ > > > > > > > > + struct plic_handler *handler = > > > > > > > > this_cpu_ptr(&plic_handlers); + > > > > > > > > + if (irqd_irq_masked(d)) { > > > > > > > > + plic_irq_unmask(d); > > > > > > > > + writel(d->hwirq, handler->hart_base + > > > > > > > > CONTEXT_CLAIM); > > > > > > > > + plic_irq_mask(d); > > > > > > > > + } else { > > > > > > > > + writel(d->hwirq, handler->hart_base + > > > > > > > > CONTEXT_CLAIM); > > > > > > > > + } > > > > > > > > +} > > > > > > > > + > > > > > > > > > > > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. > > > > > > > > > > > > This indeed happens with SiFive PLIC. I am currently tinkering with > > > > > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However > > > > > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything > > > > > > begins to work fine. > > > > > > > > > > > > May be these change should be propagated to plic_irq_eoi instead of > > > > > > making a new function ? > > > > > > > > > > That's my impression too. I think the T-Head defect is pretty much > > > > > immaterial when you consider how 'interesting' the PLIC architecture > > > > > is. > > > > Which is the "T-Head defect" you mentioned here? > > > > 1. Auto masking with claim + complete (I don't think it's a defect, > > > > right? May I add a new patch to utilize the feature to decrease a > > > > little duplicate mask/unmask operations in the future?) > > > > > > This is definitely a defect and non-compliance for T-HEAD because > > I just agree with non-compliance, but what's the defect of > > auto-masking? If somebody could explain, I'm very grateful. > > > > > no sane interrupt controller would mask interrupt upon claim and this > > > is not what RISC-V PLIC defines. > > > > > > > 2. EOI failed when masked > > > > > > This defect exists for both RISC-V PLIC and T-HEAD PLIC > > > because of the way interrupt completion is defined. > > > > > > > > > > > > Conflating EOI and masking really is a misfeature... > > > > I think the problem is riscv PLIC reuse enable bit as mask bit. I > > > > recommend separating them. That means: > > > > > > There are no per-interrupt mask bits. We only have per-context > > > and per-interrupt enable bits which is used to provide mask/unmask > > > functionality expected by the irqchip framework. > > > > > > I don't see how this is a problem for RISC-V PLIC. The only real > > > issue with RISC-V PLIC is the fact the interrupt completion will be > > > ignored for a masked interrupt which is what Marc is pointing at. > > So you are not considering add per-interrupt mask bits to solve the > > above problem, right? > > The RISC-V PLIC has several limitations and also lacks a lot of features > hence it's marked as deprecated in RISC-V platform specs and will be > removed eventually from RISC-V platform specs. > > The RISC-V AIA will totally replace RISC-V PLIC going forward. In fact, > RISC-V AIA APLIC addresses all limitations of RISC-V PLIC along with > new features additions. > > > > > I don't think you would keep below codes in AIA eoi. > > + plic_irq_unmask(d); > > + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); > > + plic_irq_mask(d); > > Like I mentioned previously, the AIA APLIC is very different from the > PLIC so we don't need this mask/unmask dance over there. It has global > per-interrupt enable bits in AIA APLIC which is different from PLIC. The ”global per-interrupt enable bits“ is okay. > > Regards, > Anup > > > > > > > > > Regards, > > > Anup > > > > > > > - EOI still depends on enable bit. > > > > - Add mask/unmask bit regs to do the right thing. > > > > > > > > > > > > > > > > > > > > > > > M. > > > > > > > > > > -- > > > > > Without deviation from the norm, progress is not possible. > > > > > > > > > > > > > > > > -- > > > > Best Regards > > > > Guo Ren > > > > > > > > ML: https://lore.kernel.org/linux-csky/ > > > > > > > > -- > > Best Regards > > Guo Ren > > > > ML: https://lore.kernel.org/linux-csky/ -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-11-01 4:27 ` Anup Patel 2021-11-01 7:56 ` Guo Ren @ 2021-11-01 9:27 ` Marc Zyngier 1 sibling, 0 replies; 27+ messages in thread From: Marc Zyngier @ 2021-11-01 9:27 UTC (permalink / raw) To: Anup Patel Cc: Guo Ren, Nikita Shubin, Atish Patra, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, Linux Kernel Mailing List, linux-riscv, Guo Ren On Mon, 01 Nov 2021 04:27:50 +0000, Anup Patel <anup@brainfault.org> wrote: > The RISC-V AIA will totally replace RISC-V PLIC going forward. In fact, > RISC-V AIA APLIC addresses all limitations of RISC-V PLIC along with > new features additions. Instead of arguing about yet another piece of RISC-V vapourware, how about you guys propose a patch that would actually make the *current* HW work to some extent? M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-11-01 2:20 ` Guo Ren 2021-11-01 2:53 ` Anup Patel @ 2021-11-01 9:25 ` Marc Zyngier 1 sibling, 0 replies; 27+ messages in thread From: Marc Zyngier @ 2021-11-01 9:25 UTC (permalink / raw) To: Guo Ren Cc: Nikita Shubin, Anup Patel, Atish Patra, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, Linux Kernel Mailing List, linux-riscv, Guo Ren On Mon, 01 Nov 2021 02:20:21 +0000, Guo Ren <guoren@kernel.org> wrote: > > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Thu, 28 Oct 2021 11:55:23 +0100, > > Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > > > > > Hello Marc and Guo Ren! > > > > > > On Mon, 25 Oct 2021 11:48:33 +0100 > > > Marc Zyngier <maz@kernel.org> wrote: > > > > > > > On Sun, 24 Oct 2021 02:33:03 +0100, > > > > guoren@kernel.org wrote: > > > > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the > > > > > driver, only the first interrupt could be handled, and continue irq > > > > > is blocked by hw. Because the thead,c900-plic couldn't complete > > > > > masked irq source which has been disabled in enable register. Add > > > > > thead_plic_chip which fix up c906-plic irq source completion > > > > > problem by unmask/mask wrapper. > > > > > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > > > > > > > The PLIC signals it has completed executing an interrupt handler by > > > > > writing the interrupt ID it received from the claim to the > > > > > claim/complete register. The PLIC does not check whether the > > > > > completion ID is the same as the last claim ID for that target. If > > > > > the completion ID does not match an interrupt source that is > > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ > > > > > completion is silently ignored. > > > > > > > > Given this bit of the spec... > > > > > > > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > > > > +{ > > > > > + struct plic_handler *handler = > > > > > this_cpu_ptr(&plic_handlers); + > > > > > + if (irqd_irq_masked(d)) { > > > > > + plic_irq_unmask(d); > > > > > + writel(d->hwirq, handler->hart_base + > > > > > CONTEXT_CLAIM); > > > > > + plic_irq_mask(d); > > > > > + } else { > > > > > + writel(d->hwirq, handler->hart_base + > > > > > CONTEXT_CLAIM); > > > > > + } > > > > > +} > > > > > + > > > > > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. > > > > > > This indeed happens with SiFive PLIC. I am currently tinkering with > > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However > > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything > > > begins to work fine. > > > > > > May be these change should be propagated to plic_irq_eoi instead of > > > making a new function ? > > > > That's my impression too. I think the T-Head defect is pretty much > > immaterial when you consider how 'interesting' the PLIC architecture > > is. > Which is the "T-Head defect" you mentioned here? > 1. Auto masking with claim + complete (I don't think it's a defect, > right? May I add a new patch to utilize the feature to decrease a > little duplicate mask/unmask operations in the future?) That *is* a T-Head defect. It may not be material for Linux, but being a departure from the spec, it is a bug, clear and simple. IMHO, either you implement the spec to the letter, or you don't. If you deviate, this is something else. > 2. EOI failed when masked This one is a PLIC architecture defect, which seems to plague everyone. > > > Conflating EOI and masking really is a misfeature... > I think the problem is riscv PLIC reuse enable bit as mask bit. I > recommend separating them. That means: > - EOI still depends on enable bit. > - Add mask/unmask bit regs to do the right thing. Maybe, but that's not the problem at hand. I suggest you move architectural discussions to a separate thread, and keep this thread for fixing the mess that plagues existing users. M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-10-28 10:55 ` [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic " Nikita Shubin 2021-10-28 14:58 ` Marc Zyngier @ 2021-11-01 2:00 ` Guo Ren 2021-11-01 5:11 ` Vincent Pelletier 2 siblings, 0 replies; 27+ messages in thread From: Guo Ren @ 2021-11-01 2:00 UTC (permalink / raw) To: Nikita Shubin Cc: Marc Zyngier, Anup Patel, Atish Patra, Thomas Gleixner, Palmer Dabbelt, Heiko Stübner, Rob Herring, Linux Kernel Mailing List, linux-riscv, Guo Ren Thx Nikita, I would fixup that globally in the plic in next version of patch. On Thu, Oct 28, 2021 at 7:01 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > Hello Marc and Guo Ren! > > On Mon, 25 Oct 2021 11:48:33 +0100 > Marc Zyngier <maz@kernel.org> wrote: > > > On Sun, 24 Oct 2021 02:33:03 +0100, > > guoren@kernel.org wrote: > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the > > > driver, only the first interrupt could be handled, and continue irq > > > is blocked by hw. Because the thead,c900-plic couldn't complete > > > masked irq source which has been disabled in enable register. Add > > > thead_plic_chip which fix up c906-plic irq source completion > > > problem by unmask/mask wrapper. > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]: > > > > > > The PLIC signals it has completed executing an interrupt handler by > > > writing the interrupt ID it received from the claim to the > > > claim/complete register. The PLIC does not check whether the > > > completion ID is the same as the last claim ID for that target. If > > > the completion ID does not match an interrupt source that is > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ > > > completion is silently ignored. > > > > Given this bit of the spec... > > > > > +static void plic_thead_irq_eoi(struct irq_data *d) > > > +{ > > > + struct plic_handler *handler = > > > this_cpu_ptr(&plic_handlers); + > > > + if (irqd_irq_masked(d)) { > > > + plic_irq_unmask(d); > > > + writel(d->hwirq, handler->hart_base + > > > CONTEXT_CLAIM); > > > + plic_irq_mask(d); > > > + } else { > > > + writel(d->hwirq, handler->hart_base + > > > CONTEXT_CLAIM); > > > + } > > > +} > > > + > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC. > > This indeed happens with SiFive PLIC. I am currently tinkering with > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything > begins to work fine. > > May be these change should be propagated to plic_irq_eoi instead of > making a new function ? > > > > > And it isn't only for threaded interrupts in oneshot mode. Any driver > > can mask an interrupt from its handler after having set the > > IRQ_DISABLE_UNLAZY flag, and the interrupt would need the exact same > > treatment. > > > > M. > > > > --- > diff --git a/drivers/irqchip/irq-sifive-plic.c > b/drivers/irqchip/irq-sifive-plic.c index cf74cfa82045..259065d271ef > 100644 --- a/drivers/irqchip/irq-sifive-plic.c > +++ b/drivers/irqchip/irq-sifive-plic.c > @@ -163,7 +163,13 @@ static void plic_irq_eoi(struct irq_data *d) > { > struct plic_handler *handler = this_cpu_ptr(&plic_handlers); > > - writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); > + if (irqd_irq_masked(d)) { > + plic_irq_unmask(d); > + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); > + plic_irq_mask(d); > + } else { > + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); > + } > } > > static struct irq_chip plic_chip = { -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT 2021-10-28 10:55 ` [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic " Nikita Shubin 2021-10-28 14:58 ` Marc Zyngier 2021-11-01 2:00 ` Guo Ren @ 2021-11-01 5:11 ` Vincent Pelletier 2 siblings, 0 replies; 27+ messages in thread From: Vincent Pelletier @ 2021-11-01 5:11 UTC (permalink / raw) To: Nikita Shubin Cc: Marc Zyngier, guoren, anup, atish.patra, tglx, palmer, heiko, robh, linux-kernel, linux-riscv, Guo Ren On Thu, 28 Oct 2021 13:55:23 +0300, Nikita Shubin <nikita.shubin@maquefel.me> wrote: > This indeed happens with SiFive PLIC. I am currently tinkering with > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. Happy to see someone else having this issue. I hit this issue in July and tried to get feedback, but nothing happened and I gave up: http://lists.infradead.org/pipermail/linux-riscv/2021-July/007441.html My uneducated guess, by spying on the registers behind the kernel's back (see the python script I attached), was that this could be specific to level-signalled interrupts, where the IRQ re-triggers in the PLIC right after being cleared but after being unbound from any hart. Then the "IRQ pending" flag is set (causing the IRQ edge which would normally trigger interrupt handling in associated hart) without anything noticing, so it will never be cleared and never be handled. > However > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything > begins to work fine. Great news, so this issue will be fixed in a better way than my RFC. RFC which can hence be discarded in patchwork, I believe: https://patchwork.kernel.org/project/linux-riscv/patch/8c36c1a28ce63b5120765fd3c636944bfec8bee9.1625882423.git.plr.vincent@gmail.com/ (I'm not sure if I can do it myself) Regards, -- Vincent Pelletier GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1 ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2021-11-03 1:52 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-24 1:33 [PATCH V5 0/3] Add thead,c900-plic support guoren 2021-10-24 1:33 ` [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor guoren 2021-11-02 2:21 ` Guo Ren 2021-11-02 12:59 ` Rob Herring 2021-11-03 1:52 ` Guo Ren 2021-10-24 1:33 ` [PATCH V5 2/3] dt-bindings: update riscv plic compatible string guoren 2021-10-24 7:35 ` Anup Patel 2021-10-24 9:01 ` Guo Ren 2021-10-24 9:18 ` Anup Patel 2021-10-24 9:35 ` Guo Ren 2021-10-24 9:52 ` Anup Patel 2021-10-24 10:04 ` Guo Ren 2021-10-24 1:33 ` [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT guoren 2021-10-25 10:48 ` Marc Zyngier 2021-10-25 13:33 ` Guo Ren 2021-10-28 10:55 ` [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic " Nikita Shubin 2021-10-28 14:58 ` Marc Zyngier 2021-10-30 10:27 ` Anup Patel 2021-11-01 2:20 ` Guo Ren 2021-11-01 2:53 ` Anup Patel 2021-11-01 3:57 ` Guo Ren 2021-11-01 4:27 ` Anup Patel 2021-11-01 7:56 ` Guo Ren 2021-11-01 9:27 ` Marc Zyngier 2021-11-01 9:25 ` Marc Zyngier 2021-11-01 2:00 ` Guo Ren 2021-11-01 5:11 ` Vincent Pelletier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).