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(-)
--
2.25.1
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(+)
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
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
- 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 <[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
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.
[1] https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adoc
Signed-off-by: Guo Ren <[email protected]>
Cc: Anup Patel <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Atish Patra <[email protected]>
---
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
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
> - 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 <[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
>
On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <[email protected]> wrote:
>
> 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
> > - 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 <[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
> >
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
On Sun, Oct 24, 2021 at 2:31 PM Guo Ren <[email protected]> wrote:
>
> On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <[email protected]> wrote:
> >
> > 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
> > > - 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 <[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
> > >
>
>
>
> --
> Best Regards
> Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/
On Sun, Oct 24, 2021 at 5:18 PM Anup Patel <[email protected]> wrote:
>
> On Sun, Oct 24, 2021 at 2:31 PM Guo Ren <[email protected]> wrote:
> >
> > On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <[email protected]> wrote:
> > >
> > > 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
> > > > - 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 <[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
> > > >
> >
> >
> >
> > --
> > Best Regards
> > Guo Ren
> >
> > ML: https://lore.kernel.org/linux-csky/
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
On Sun, Oct 24, 2021 at 3:05 PM Guo Ren <[email protected]> wrote:
>
> On Sun, Oct 24, 2021 at 5:18 PM Anup Patel <[email protected]> wrote:
> >
> > On Sun, Oct 24, 2021 at 2:31 PM Guo Ren <[email protected]> wrote:
> > >
> > > On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <[email protected]> wrote:
> > > >
> > > > 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
> > > > > - 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 <[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
> > > > >
> > >
> > >
> > >
> > > --
> > > Best Regards
> > > Guo Ren
> > >
> > > ML: https://lore.kernel.org/linux-csky/
>
>
>
> --
> Best Regards
> Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/
On Sun, Oct 24, 2021 at 5:53 PM Anup Patel <[email protected]> wrote:
>
> On Sun, Oct 24, 2021 at 3:05 PM Guo Ren <[email protected]> wrote:
> >
> > On Sun, Oct 24, 2021 at 5:18 PM Anup Patel <[email protected]> wrote:
> > >
> > > On Sun, Oct 24, 2021 at 2:31 PM Guo Ren <[email protected]> wrote:
> > > >
> > > > On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <[email protected]> wrote:
> > > > >
> > > > > 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
> > > > > > - 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 <[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
> > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > 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/
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. 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.
On Mon, Oct 25, 2021 at 6:48 PM Marc Zyngier <[email protected]> wrote:
>
> 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. 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/
Hello Marc and Guo Ren!
On Mon, 25 Oct 2021 11:48:33 +0100
Marc Zyngier <[email protected]> wrote:
> 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. 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 = {
On Thu, 28 Oct 2021 11:55:23 +0100,
Nikita Shubin <[email protected]> wrote:
>
> Hello Marc and Guo Ren!
>
> On Mon, 25 Oct 2021 11:48:33 +0100
> Marc Zyngier <[email protected]> wrote:
>
> > 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. 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.
On Thu, Oct 28, 2021 at 8:28 PM Marc Zyngier <[email protected]> wrote:
>
> On Thu, 28 Oct 2021 11:55:23 +0100,
> Nikita Shubin <[email protected]> wrote:
> >
> > Hello Marc and Guo Ren!
> >
> > On Mon, 25 Oct 2021 11:48:33 +0100
> > Marc Zyngier <[email protected]> wrote:
> >
> > > 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. 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.
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 <[email protected]> wrote:
>
> Hello Marc and Guo Ren!
>
> On Mon, 25 Oct 2021 11:48:33 +0100
> Marc Zyngier <[email protected]> wrote:
>
> > 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. 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/
On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[email protected]> wrote:
>
> On Thu, 28 Oct 2021 11:55:23 +0100,
> Nikita Shubin <[email protected]> wrote:
> >
> > Hello Marc and Guo Ren!
> >
> > On Mon, 25 Oct 2021 11:48:33 +0100
> > Marc Zyngier <[email protected]> wrote:
> >
> > > 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. 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/
On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <[email protected]> wrote:
>
> On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[email protected]> wrote:
> >
> > On Thu, 28 Oct 2021 11:55:23 +0100,
> > Nikita Shubin <[email protected]> wrote:
> > >
> > > Hello Marc and Guo Ren!
> > >
> > > On Mon, 25 Oct 2021 11:48:33 +0100
> > > Marc Zyngier <[email protected]> wrote:
> > >
> > > > 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. 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/
On Mon, Nov 1, 2021 at 10:53 AM Anup Patel <[email protected]> wrote:
>
> On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <[email protected]> wrote:
> >
> > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[email protected]> wrote:
> > >
> > > On Thu, 28 Oct 2021 11:55:23 +0100,
> > > Nikita Shubin <[email protected]> wrote:
> > > >
> > > > Hello Marc and Guo Ren!
> > > >
> > > > On Mon, 25 Oct 2021 11:48:33 +0100
> > > > Marc Zyngier <[email protected]> wrote:
> > > >
> > > > > 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. 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/
On Mon, Nov 1, 2021 at 9:27 AM Guo Ren <[email protected]> wrote:
>
> On Mon, Nov 1, 2021 at 10:53 AM Anup Patel <[email protected]> wrote:
> >
> > On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <[email protected]> wrote:
> > >
> > > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[email protected]> wrote:
> > > >
> > > > On Thu, 28 Oct 2021 11:55:23 +0100,
> > > > Nikita Shubin <[email protected]> wrote:
> > > > >
> > > > > Hello Marc and Guo Ren!
> > > > >
> > > > > On Mon, 25 Oct 2021 11:48:33 +0100
> > > > > Marc Zyngier <[email protected]> wrote:
> > > > >
> > > > > > 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. 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/
On Thu, 28 Oct 2021 13:55:23 +0300, Nikita Shubin <[email protected]> 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
On Mon, Nov 1, 2021 at 12:28 PM Anup Patel <[email protected]> wrote:
>
> On Mon, Nov 1, 2021 at 9:27 AM Guo Ren <[email protected]> wrote:
> >
> > On Mon, Nov 1, 2021 at 10:53 AM Anup Patel <[email protected]> wrote:
> > >
> > > On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <[email protected]> wrote:
> > > >
> > > > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[email protected]> wrote:
> > > > >
> > > > > On Thu, 28 Oct 2021 11:55:23 +0100,
> > > > > Nikita Shubin <[email protected]> wrote:
> > > > > >
> > > > > > Hello Marc and Guo Ren!
> > > > > >
> > > > > > On Mon, 25 Oct 2021 11:48:33 +0100
> > > > > > Marc Zyngier <[email protected]> wrote:
> > > > > >
> > > > > > > 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. 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/
On Mon, 01 Nov 2021 02:20:21 +0000,
Guo Ren <[email protected]> wrote:
>
> On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[email protected]> wrote:
> >
> > On Thu, 28 Oct 2021 11:55:23 +0100,
> > Nikita Shubin <[email protected]> wrote:
> > >
> > > Hello Marc and Guo Ren!
> > >
> > > On Mon, 25 Oct 2021 11:48:33 +0100
> > > Marc Zyngier <[email protected]> wrote:
> > >
> > > > 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. 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.
On Mon, 01 Nov 2021 04:27:50 +0000,
Anup Patel <[email protected]> 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.
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 <[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(+)
>
> 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/
On Mon, Nov 1, 2021 at 9:21 PM Guo Ren <[email protected]> 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 <[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(+)
> >
> > 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/
On Tue, Nov 2, 2021 at 8:59 PM Rob Herring <[email protected]> wrote:
>
> On Mon, Nov 1, 2021 at 9:21 PM Guo Ren <[email protected]> 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 <[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(+)
> > >
> > > 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/