Add thead,c900-plic support
Subsystems (4)
v5 cover Add thead,c900-plic support
From: Guo Ren <[email protected]>
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(-)
Signature · 7 lines
2.25.1 _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
PATCH v5 1/3 dt-bindings: vendor-prefixes: add T-Head Semiconductor
From: Guo Ren <[email protected]>
Add vendor prefix for T-Head Semiconductor [1] [2]
[1] https://github.com/T-head-Semi
[2] https://www.t-head.cn/
--- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/vendor-prefixes.yaml +2 -0
@@ -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,.*":
Signature · 7 lines
2.25.1 _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor
Hi Rob,
ping? If there is no problem, could you help pick up this patch into your tree?
Quoted 31 lines · On Sun, Oct 24, 2021 at 9:33 AM <[email protected]> wrote:
From: Guo Ren <[email protected]>
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 <[email protected]>Cc Rob Herring <[email protected]>Cc Rob Herring <[email protected]>--- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+)Documentation/devicetree/bindings/vendor-prefixes.yaml +2
-1@@ -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
Signature · 9 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
Rob Herring
maintainerRe: [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor
Quoted 4 lines · Guo Ren
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.
Quoted 39 lines · On Sun, Oct 24, 2021 at 9:33 AM <[email protected]> wrote:
Quoted 29 lines
From: Guo Ren <[email protected]>
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 <[email protected]>Cc Rob Herring <[email protected]>Cc Rob Herring <[email protected]>--- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+)Documentation/devicetree/bindings/vendor-prefixes.yaml +2
-1@@ -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,.*": --Quoted 2 lines
2.25.1
--
Best Regards
Guo Ren
Quoted 1 line
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH V5 1/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor
Quoted 10 lines · Rob Herring
Quoted 4 lines · Guo Ren
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.
Quoted 41 lines
Quoted 41 lines · On Sun, Oct 24, 2021 at 9:33 AM <[email protected]> wrote:
Quoted 29 lines
From: Guo Ren <[email protected]>
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 <[email protected]>Cc Rob Herring <[email protected]>Cc Rob Herring <[email protected]>--- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+)Documentation/devicetree/bindings/vendor-prefixes.yaml +2
-1@@ -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,.*": --Quoted 2 lines
2.25.1
--
Best Regards
Guo Ren
Signature · 4 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
PATCH v5 2/3 dt-bindings: update riscv plic compatible string
In reply to [email protected]From: Guo Ren <[email protected]>
Add the compatible string "thead,c900-plic" to the riscv plic bindings to support allwinner d1 SOC which contains c906 core.
---
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(-)
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +12 -3
@@ -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 <[email protected]> - Paul Walmsley <[email protected]>
@@ -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
Signature · 7 lines
2.25.1 _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string
Quoted 38 lines · On Sun, Oct 24, 2021 at 7:03 AM <[email protected]> wrote:
From: Guo Ren <[email protected]>
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 <[email protected]>Cc Anup Patel <[email protected]>Cc Atish Patra <[email protected]>Cc Heiko Stuebner <[email protected]>Cc Rob Herring <[email protected]>Cc Rob Herring <[email protected]>Cc Palmer Dabbelt <[email protected]>---
Changes since V5:
- Add DT list
- Fixup compatible string
- Remove allwinner-d1 compatible
- makedt_binding_checkChanges since V4:
- Update description in errata style
- Update enum suggested by Anup, Heiko, SamuelChanges 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(-)Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +0
-0
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml description: · line 41
Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +4
-0@@ -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
Quoted 24 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +8
-4maintainers: - Sagar Kadam <[email protected]> - Paul Walmsley <[email protected]>@@ -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 --
Quoted 2 lines
2.25.1
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string
Quoted 51 lines · Anup Patel
Quoted 38 lines · On Sun, Oct 24, 2021 at 7:03 AM <[email protected]> wrote:
From: Guo Ren <[email protected]>
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 <[email protected]>Cc Anup Patel <[email protected]>Cc Atish Patra <[email protected]>Cc Heiko Stuebner <[email protected]>Cc Rob Herring <[email protected]>Cc Rob Herring <[email protected]>Cc Palmer Dabbelt <[email protected]>---
Changes since V5:
- Add DT list
- Fixup compatible string
- Remove allwinner-d1 compatible
- makedt_binding_checkChanges since V4:
- Update description in errata style
- Update enum suggested by Anup, Heiko, SamuelChanges 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(-)Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +0
-0Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml description: · line 41
Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +4
-0@@ -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.
Quoted 7 lines
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.
Quoted 30 lines
Regards,
AnupQuoted 26 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +8
-4maintainers: - Sagar Kadam <[email protected]> - Paul Walmsley <[email protected]>@@ -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
Signature · 4 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string
Quoted 60 lines · Guo Ren
Quoted 51 lines · Anup Patel
Quoted 38 lines · On Sun, Oct 24, 2021 at 7:03 AM <[email protected]> wrote:
From: Guo Ren <[email protected]>
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 <[email protected]>Cc Anup Patel <[email protected]>Cc Atish Patra <[email protected]>Cc Heiko Stuebner <[email protected]>Cc Rob Herring <[email protected]>Cc Rob Herring <[email protected]>Cc Palmer Dabbelt <[email protected]>---
Changes since V5:
- Add DT list
- Fixup compatible string
- Remove allwinner-d1 compatible
- makedt_binding_checkChanges since V4:
- Update description in errata style
- Update enum suggested by Anup, Heiko, SamuelChanges 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(-)Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +0
-0Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml description: · line 41
Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +4
-0@@ -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.
Quoted 7 lines
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.
Quoted 5 lines
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
Quoted 39 lines
Quoted 26 lines
Regards,
AnupQuoted 21 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +8
-3maintainers: - Sagar Kadam <[email protected]> - Paul Walmsley <[email protected]>@@ -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-plicQuoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +0
-1reg: maxItems: 1 --2.25.1
--
Best Regards
Guo Ren
Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string
Quoted 73 lines · Anup Patel
Quoted 60 lines · Guo Ren
Quoted 51 lines · Anup Patel
Quoted 38 lines · On Sun, Oct 24, 2021 at 7:03 AM <[email protected]> wrote:
From: Guo Ren <[email protected]>
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 <[email protected]>Cc Anup Patel <[email protected]>Cc Atish Patra <[email protected]>Cc Heiko Stuebner <[email protected]>Cc Rob Herring <[email protected]>Cc Rob Herring <[email protected]>Cc Palmer Dabbelt <[email protected]>---
Changes since V5:
- Add DT list
- Fixup compatible string
- Remove allwinner-d1 compatible
- makedt_binding_checkChanges since V4:
- Update description in errata style
- Update enum suggested by Anup, Heiko, SamuelChanges 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(-)Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +0
-0Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml description: · line 41
Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +4
-0@@ -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.
Quoted 7 lines
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.Quoted 5 lines
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.
Quoted 4 lines
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.
Quoted 43 lines
Regards,
AnupQuoted 39 lines
Quoted 26 lines
Regards,
AnupQuoted 21 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +8
-3maintainers: - Sagar Kadam <[email protected]> - Paul Walmsley <[email protected]>@@ -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-plicQuoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +0
-1reg: maxItems: 1 --2.25.1
--
Best Regards
Guo Ren
Signature · 4 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string
Quoted 80 lines · Guo Ren
Quoted 73 lines · Anup Patel
Quoted 60 lines · Guo Ren
Quoted 51 lines · Anup Patel
Quoted 38 lines · On Sun, Oct 24, 2021 at 7:03 AM <[email protected]> wrote:
From: Guo Ren <[email protected]>
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 <[email protected]>Cc Anup Patel <[email protected]>Cc Atish Patra <[email protected]>Cc Heiko Stuebner <[email protected]>Cc Rob Herring <[email protected]>Cc Rob Herring <[email protected]>Cc Palmer Dabbelt <[email protected]>---
Changes since V5:
- Add DT list
- Fixup compatible string
- Remove allwinner-d1 compatible
- makedt_binding_checkChanges since V4:
- Update description in errata style
- Update enum suggested by Anup, Heiko, SamuelChanges 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(-)Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +0
-0Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml description: · line 41
Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +4
-0@@ -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.
Quoted 7 lines
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.Quoted 5 lines
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.
Quoted 4 lines
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.
Quoted 7 lines
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. Addthead_plic_chipwhich 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.
Quoted 4 lines
- 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
Quoted 47 lines
Quoted 44 lines
Regards,
AnupQuoted 39 lines
Quoted 26 lines
Regards,
AnupQuoted 21 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +8
-3maintainers: - Sagar Kadam <[email protected]> - Paul Walmsley <[email protected]>@@ -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-plicQuoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +0
-1reg: maxItems: 1 --2.25.1
--
Best Regards
Guo Ren
Quoted 5 lines
--
Best Regards
Guo Ren
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string
Quoted 94 lines · Anup Patel
Quoted 80 lines · Guo Ren
Quoted 73 lines · Anup Patel
Quoted 60 lines · Guo Ren
Quoted 51 lines · Anup Patel
Quoted 38 lines · On Sun, Oct 24, 2021 at 7:03 AM <[email protected]> wrote:
From: Guo Ren <[email protected]>
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 <[email protected]>Cc Anup Patel <[email protected]>Cc Atish Patra <[email protected]>Cc Heiko Stuebner <[email protected]>Cc Rob Herring <[email protected]>Cc Rob Herring <[email protected]>Cc Palmer Dabbelt <[email protected]>---
Changes since V5:
- Add DT list
- Fixup compatible string
- Remove allwinner-d1 compatible
- makedt_binding_checkChanges since V4:
- Update description in errata style
- Update enum suggested by Anup, Heiko, SamuelChanges 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(-)Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +0
-0Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml description: · line 41
Quoted 7 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +4
-0@@ -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.
Quoted 7 lines
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.Quoted 5 lines
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.
Quoted 4 lines
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.
Quoted 7 lines
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. Addthead_plic_chipwhich 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.
Quoted 8 lines
Quoted 4 lines
- 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.
Quoted 56 lines
Regards,
AnupQuoted 52 lines
Quoted 44 lines
Regards,
AnupQuoted 39 lines
Quoted 26 lines
Regards,
AnupQuoted 21 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +8
-3maintainers: - Sagar Kadam <[email protected]> - Paul Walmsley <[email protected]>@@ -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-plicQuoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +0
-1reg: maxItems: 1 --2.25.1
--
Best Regards
Guo Ren--
Best Regards
Guo Ren
Signature · 9 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
PATCH v5 3/3 irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT
In reply to [email protected]From: Guo Ren <[email protected]>
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.
---
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(-)
drivers/irqchip/irq-sifive-plic.c +42 -2
@@ -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);
Signature · 7 lines
2.25.1 _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT
Quoted 11 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 197
Quoted 13 lines
drivers/irqchip/irq-sifive-plic.c +13
-0+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.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead,c900-plic request_threaded_irq with ONESHOT
Quoted 24 lines · Marc Zyngier
Quoted 11 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 15 lines
drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 197
Quoted 13 lines
drivers/irqchip/irq-sifive-plic.c +13
-0+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.
Quoted 5 lines
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_UNLAZYflag, 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.
Quoted 5 lines
M.--
Without deviation from the norm, progress is not possible.
Signature · 4 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
In reply to Marc ZyngierHello Marc and Guo Ren!
Quoted 33 lines · On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 12 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 9 lines
(quoted diff) +3
-0+static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler =
this_cpu_ptr(&plic_handlers); +(quoted diff) +3
-0+ if (irqd_irq_masked(d)) { + plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);(quoted diff) +1
-0+ plic_irq_mask(d);
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +2
-0+ } else { + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);(quoted diff) +3
-0+ } +} +... 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 ?
Quoted 8 lines
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_UNLAZYflag, and the interrupt would need the exact same treatment.M.
---
(quoted diff) +0 -0
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 = {
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
Quoted 48 lines · Nikita Shubin
Hello Marc and Guo Ren!
Quoted 33 lines · On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 12 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 9 lines
(quoted diff) +3
-0+static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler =
this_cpu_ptr(&plic_handlers); +(quoted diff) +3
-0+ if (irqd_irq_masked(d)) { + plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);(quoted diff) +1
-0+ plic_irq_mask(d);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +2
-0+ } else { + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);(quoted diff) +3
-0+ } +} +... it isn't obvious to me why this cannot happen on an SiFive PLIC.
Quoted 7 lines
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_eoiinstead 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.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
Quoted 56 lines · Marc Zyngier
Quoted 53 lines · Nikita Shubin
Hello Marc and Guo Ren!
Quoted 33 lines · On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 12 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 9 lines
(quoted diff) +3
-0+static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler =
this_cpu_ptr(&plic_handlers); +
+ if (irqd_irq_masked(d)) {(quoted diff) +2
-0+ plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);
+plic_irq_mask(d);Quoted 8 lines
Review
Quoted 6 lines
+ } else {
+ writel(d->hwirq,handler->hart_base+CONTEXT_CLAIM);
+ }(quoted diff) +2
-0+} +... 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.
Quoted 6 lines
Quoted 2 lines
May be these change should be propagated to
plic_irq_eoiinstead 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
Quoted 5 lines
M.--
Without deviation from the norm, progress is not possible.
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
In reply to Marc ZyngierQuoted 56 lines · Marc Zyngier
Quoted 53 lines · Nikita Shubin
Hello Marc and Guo Ren!
Quoted 33 lines · On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 12 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 9 lines
(quoted diff) +3
-0+static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler =
this_cpu_ptr(&plic_handlers); +
+ if (irqd_irq_masked(d)) {(quoted diff) +2
-0+ plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);
+plic_irq_mask(d);Quoted 8 lines
Review
Quoted 6 lines
+ } else {
+ writel(d->hwirq,handler->hart_base+CONTEXT_CLAIM);
+ }(quoted diff) +2
-0+} +... 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.
Quoted 6 lines
Quoted 2 lines
May be these change should be propagated to
plic_irq_eoiinstead 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
Quoted 1 line
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.
Quoted 5 lines
M.--
Without deviation from the norm, progress is not possible.
Signature · 4 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
Quoted 61 lines · Guo Ren
Quoted 59 lines · Marc Zyngier
Quoted 55 lines · Nikita Shubin
Hello Marc and Guo Ren!
Quoted 33 lines · On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 12 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 9 lines
(quoted diff) +3
-0+static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler =
this_cpu_ptr(&plic_handlers); +
+ if (irqd_irq_masked(d)) {(quoted diff) +2
-0+ plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);
+plic_irq_mask(d);Quoted 8 lines
Review
Quoted 6 lines
+ } else {
+ writel(d->hwirq,handler->hart_base+CONTEXT_CLAIM);
+ }(quoted diff) +2
-0+} +... 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_eoiinstead of making a new function ?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 1 line
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.
Quoted 4 lines
Quoted 2 lines
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
Quoted 19 lines
- EOI still depends on enable bit.
- Add mask/unmask bit regs to do the right thing.Quoted 5 lines
M.--
Without deviation from the norm, progress is not possible.--
Best Regards
Guo Ren
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
Quoted 66 lines · Anup Patel
Quoted 64 lines · Guo Ren
Quoted 62 lines · Marc Zyngier
Quoted 48 lines · Nikita Shubin
Hello Marc and Guo Ren!
Quoted 33 lines · On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 12 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 9 lines
(quoted diff) +3
-0+static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler =
this_cpu_ptr(&plic_handlers); +
+ if (irqd_irq_masked(d)) {(quoted diff) +2
-0+ plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);
+plic_irq_mask(d);Quoted 8 lines
Review
Quoted 6 lines
+ } else {
+ writel(d->hwirq,handler->hart_base+CONTEXT_CLAIM);
+ }(quoted diff) +2
-0+} +... it isn't obvious to me why this cannot happen on an SiFive PLIC.
Quoted 7 lines
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_eoiinstead 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.
Quoted 6 lines
Quoted 4 lines
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.
Quoted 13 lines
no sane interrupt controller would mask interrupt upon claim and this is not what RISC-V PLIC defines.
Quoted 1 line
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.
Quoted 4 lines
Quoted 2 lines
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:
Quoted 7 lines
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);
Quoted 23 lines
Regards,
AnupQuoted 19 lines
- EOI still depends on enable bit.
- Add mask/unmask bit regs to do the right thing.Quoted 5 lines
M.--
Without deviation from the norm, progress is not possible.--
Best Regards
Guo Ren
Signature · 9 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
Quoted 94 lines · Guo Ren
Quoted 66 lines · Anup Patel
Quoted 64 lines · Guo Ren
Quoted 62 lines · Marc Zyngier
Quoted 48 lines · Nikita Shubin
Hello Marc and Guo Ren!
Quoted 33 lines · On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 12 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 9 lines
(quoted diff) +3
-0+static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler =
this_cpu_ptr(&plic_handlers); +
+ if (irqd_irq_masked(d)) {(quoted diff) +2
-0+ plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);
+plic_irq_mask(d);Quoted 8 lines
Review
Quoted 6 lines
+ } else {
+ writel(d->hwirq,handler->hart_base+CONTEXT_CLAIM);
+ }(quoted diff) +2
-0+} +... it isn't obvious to me why this cannot happen on an SiFive PLIC.
Quoted 7 lines
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_eoiinstead 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.
Quoted 6 lines
Quoted 4 lines
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.
Quoted 17 lines
no sane interrupt controller would mask interrupt upon claim and this is not what RISC-V PLIC defines.
Quoted 1 line
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.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 5 lines
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
Quoted 32 lines
Quoted 24 lines
Regards,
AnupQuoted 19 lines
- EOI still depends on enable bit.
- Add mask/unmask bit regs to do the right thing.Quoted 5 lines
M.--
Without deviation from the norm, progress is not possible.--
Best Regards
Guo Ren--
Best Regards
Guo Ren
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
Quoted 111 lines · Anup Patel
Quoted 94 lines · Guo Ren
Quoted 66 lines · Anup Patel
Quoted 64 lines · Guo Ren
Quoted 62 lines · Marc Zyngier
Quoted 48 lines · Nikita Shubin
Hello Marc and Guo Ren!
Quoted 33 lines · On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 12 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 9 lines
(quoted diff) +3
-0+static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler =
this_cpu_ptr(&plic_handlers); +
+ if (irqd_irq_masked(d)) {(quoted diff) +2
-0+ plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);
+plic_irq_mask(d);Quoted 8 lines
Review
Quoted 6 lines
+ } else {
+ writel(d->hwirq,handler->hart_base+CONTEXT_CLAIM);
+ }(quoted diff) +2
-0+} +... it isn't obvious to me why this cannot happen on an SiFive PLIC.
Quoted 7 lines
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_eoiinstead 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.
Quoted 6 lines
Quoted 4 lines
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.
Quoted 17 lines
no sane interrupt controller would mask interrupt upon claim and this is not what RISC-V PLIC defines.
Quoted 1 line
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.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 1 line
Quoted 8 lines
Quoted 4 lines
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.
Quoted 36 lines
Regards,
AnupQuoted 32 lines
Quoted 24 lines
Regards,
AnupQuoted 19 lines
- EOI still depends on enable bit.
- Add mask/unmask bit regs to do the right thing.Quoted 5 lines
M.--
Without deviation from the norm, progress is not possible.--
Best Regards
Guo Ren--
Best Regards
Guo Ren
Signature · 4 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
In reply to Anup PatelQuoted 3 lines · Anup Patel
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.
Signature · 6 lines
Without deviation from the norm, progress is not possible. _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
In reply to Guo RenQuoted 61 lines · Guo Ren
Quoted 59 lines · Marc Zyngier
Quoted 55 lines · Nikita Shubin
Hello Marc and Guo Ren!
Quoted 33 lines · On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 12 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 9 lines
(quoted diff) +3
-0+static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler =
this_cpu_ptr(&plic_handlers); +
+ if (irqd_irq_masked(d)) {(quoted diff) +2
-0+ plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);
+plic_irq_mask(d);Quoted 8 lines
Review
Quoted 6 lines
+ } else {
+ writel(d->hwirq,handler->hart_base+CONTEXT_CLAIM);
+ }(quoted diff) +2
-0+} +... 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_eoiinstead of making a new function ?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 1 line
2. EOI failed when masked
This one is a PLIC architecture defect, which seems to plague everyone.
Quoted 6 lines
Quoted 2 lines
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.
Signature · 6 lines
Without deviation from the norm, progress is not possible. _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
In reply to Nikita ShubinThx Nikita,
I would fixup that globally in the plic in next version of patch.
Quoted 85 lines · Nikita Shubin
Hello Marc and Guo Ren!
Quoted 33 lines · On Mon, 25 Oct 2021 11:48:33 +0100 Marc Zyngier <[email protected]> wrote:
Quoted 12 lines · On Sun, 24 Oct 2021 02:33:03 +0100, [email protected] wrote:
From: Guo Ren <[email protected]>
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. Addthead_plic_chipwhich fix up c906-plic irq source completion problem by unmask/mask wrapper.Here is the description of Interrupt Completion in PLIC spec [1]:
Quoted 7 lines
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...
Quoted 9 lines
(quoted diff) +3
-0+static void plic_thead_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler =
this_cpu_ptr(&plic_handlers); +(quoted diff) +3
-0+ if (irqd_irq_masked(d)) { + plic_irq_unmask(d); + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);
+plic_irq_mask(d);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +2
-0+ } else { + writel(d->hwirq, handler->hart_base +
CONTEXT_CLAIM);(quoted diff) +3
-0+ } +} +... 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_eoiinstead of making a new function ?Quoted 8 lines
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_UNLAZYflag, and the interrupt would need the exact same treatment.M.---
(quoted diff) +0
-0
b/drivers/irqchip/irq-sifive-plic.cindexcf74cfa82045..259065d271ef100644 ---a/drivers/irqchip/irq-sifive-plic.c
+++b/drivers/irqchip/irq-sifive-plic.c(quoted diff) +7
-1@@ -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 = {
Signature · 4 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH V5 3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT
In reply to Nikita ShubinQuoted 2 lines · Nikita Shubin
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.
Quoted 3 lines
However
with changes proposed by Guo Ren inplic_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,
Signature · 2 lines
Vincent Pelletier GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1