From: Marc Zyngier <maz@kernel.org>
To: Samuel Holland <samuel@sholland.org>, guoren@kernel.org
Cc: anup@brainfault.org, atish.patra@wdc.com, tglx@linutronix.de,
palmer@dabbelt.com, heiko@sntech.de, robh@kernel.org,
linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
Guo Ren <guoren@linux.alibaba.com>
Subject: Re: [PATCH V4 1/3] irqchip/sifive-plic: Add thead,c900-plic support
Date: Mon, 18 Oct 2021 08:21:19 +0100 [thread overview]
Message-ID: <f850af365f2ac77af79ec59f92e6434a@kernel.org> (raw)
In-Reply-To: <8be1bdbd-365d-cd28-79d7-b924908f9e39@sholland.org>
On 2021-10-18 06:17, Samuel Holland wrote:
> On 10/15/21 10:21 PM, guoren@kernel.org wrote:
>> From: Guo Ren <guoren@linux.alibaba.com>
>>
>> 1) The irq_mask/unmask() is used by handle_fasteoi_irq() is mostly
Drop this useless numbering.
>> for ONESHOT irqs and there is no limitation in the RISC-V PLIC driver
>> due to use of irq_mask/unmask() callbacks. In fact, a lot of irqchip
>> drivers using handle_fasteoi_irq() also implement irq_mask/unmask().
This paragraph doesn't provide any useful information in the context
of this patch. That's at best cover-letter material.
>> 2) The C9xx PLIC does not comply with the interrupt claim/completion
>> process defined by the RISC-V PLIC specification because C9xx PLIC
>> will mask an IRQ when it is claimed by PLIC driver (i.e. readl(claim)
>> and the IRQ will be unmasked upon completion by PLIC driver (i.e.
>> writel(claim). This behaviour breaks the handling of IRQS_ONESHOT by
>> the generic handle_fasteoi_irq() used in the PLIC driver.
>>
>> 3) This patch adds an errata fix for IRQS_ONESHOT handling on
s/fix/workaround/
>> C9xx PLIC by using irq_enable/disable() callbacks instead of
>> irq_mask/unmask().
From Documentation/process/submitting-patches.rst:
<quote>
Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.
</quote>
>>
>> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
>> Cc: Anup Patel <anup@brainfault.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>> Cc: Atish Patra <atish.patra@wdc.com>
>>
>> ---
>>
>> Changes since 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 | 34
>> +++++++++++++++++++++++++++++--
>> 1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-sifive-plic.c
>> b/drivers/irqchip/irq-sifive-plic.c
>> index cf74cfa82045..960b29d02070 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,32 @@ static struct irq_chip plic_chip = {
>> #endif
>> };
>>
>> +/*
>> + * The C9xx PLIC does not comply with the interrupt claim/completion
>> + * process defined by the RISC-V PLIC specification because C9xx PLIC
>> + * will mask an IRQ when it is claimed by PLIC driver (i.e.
>> readl(claim)
>> + * and the IRQ will be unmasked upon completion by PLIC driver (i.e.
>> + * writel(claim). This behaviour breaks the handling of IRQS_ONESHOT
>> by
>> + * the generic handle_fasteoi_irq() used in the PLIC driver.
>> + */
>> +static struct irq_chip thead_plic_chip = {
>> + .name = "T-Head PLIC",
>> + .irq_disable = plic_irq_mask,
>> + .irq_enable = plic_irq_unmask,
>> + .irq_eoi = plic_irq_eoi,
>> +#ifdef CONFIG_SMP
>> + .irq_set_affinity = plic_set_affinity,
>> +#endif
> I tested this, and it doesn't work. Without IRQCHIP_EOI_THREADED,
> .irq_eoi is called at the end of the hard IRQ handler. This unmasks the
> IRQ before the irqthread has a chance to run, so it causes an interrupt
> storm for any threaded level IRQ (I saw this happen for sun8i_thermal).
>
> With IRQCHIP_EOI_THREADED, .irq_eoi is delayed until after the
> irqthread
> runs. This is good. Except that the call to unmask_threaded_irq() is
> inside a check for IRQD_IRQ_MASKED. And IRQD_IRQ_MASKED will never be
> set because .irq_mask is NULL. So the end result is that the IRQ is
> never EOI'd and is masked permanently.
>
> If you set .flags = IRQCHIP_EOI_THREADED, and additionally set
> .irq_mask
> and .irq_unmask to a dummy function that does nothing, the IRQ core
> will
> properly set/unset IRQD_IRQ_MASKED, and the IRQs will flow as expected.
> But adding dummy functions seems not so ideal, so I am not sure if this
> is the best solution.
This series is totally broken indeed, because it assumes that
enable/disable are a substitute to mask/unmask. Nothing could be further
from the truth. mask/unmask must be implemented, and enable/disable
supplement them if the HW requires something different at startup time.
If you have an 'automask' behaviour and yet the HW doesn't record this
in a separate bit, then you need to track this by yourself in the
irq_eoi() callback instead. I guess that you would skip the write to
the CLAIM register in this case, though I have no idea whether this
breaks
the HW interrupt state or not.
There is an example of this in the Apple AIC driver.
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2021-10-18 7:21 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-16 3:21 [PATCH V4 0/3] irqchip: riscv: Add thead,c900-plic support guoren
2021-10-16 3:21 ` [PATCH V4 1/3] irqchip/sifive-plic: " guoren
2021-10-18 5:17 ` Samuel Holland
2021-10-18 5:40 ` Anup Patel
2021-10-18 7:05 ` Guo Ren
2021-10-18 7:21 ` Marc Zyngier [this message]
2021-10-19 9:33 ` Guo Ren
2021-10-19 10:18 ` Marc Zyngier
2021-10-19 13:27 ` Guo Ren
2021-10-20 13:34 ` Marc Zyngier
2021-10-20 14:19 ` Guo Ren
2021-10-20 14:59 ` Darius Rad
2021-10-20 16:18 ` Anup Patel
2021-10-20 18:01 ` Darius Rad
2021-10-21 8:47 ` Anup Patel
2021-10-20 14:33 ` Anup Patel
2021-10-20 15:08 ` Marc Zyngier
2021-10-20 16:08 ` Anup Patel
2021-10-20 16:48 ` Marc Zyngier
2021-10-21 8:52 ` Anup Patel
2021-10-21 1:46 ` Guo Ren
2021-10-21 2:00 ` Guo Ren
2021-10-21 8:33 ` Marc Zyngier
2021-10-21 9:43 ` Guo Ren
2021-10-16 3:21 ` [PATCH V4 2/3] dt-bindings: update riscv plic compatible string guoren
2021-10-16 7:07 ` Andreas Schwab
2021-10-16 9:16 ` Guo Ren
2021-10-16 10:34 ` Heiko Stuebner
2021-10-16 12:56 ` Guo Ren
2021-10-16 16:31 ` Heiko Stuebner
2021-10-20 12:15 ` Guo Ren
2021-10-18 12:02 ` Rob Herring
2021-10-19 0:55 ` Guo Ren
2021-10-16 3:22 ` [PATCH V4 3/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor guoren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f850af365f2ac77af79ec59f92e6434a@kernel.org \
--to=maz@kernel.org \
--cc=anup@brainfault.org \
--cc=atish.patra@wdc.com \
--cc=guoren@kernel.org \
--cc=guoren@linux.alibaba.com \
--cc=heiko@sntech.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=robh@kernel.org \
--cc=samuel@sholland.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).