[PATCH 1/1] riscv-aplic: manually set pending for the pre-existing interrupts

[PATCH 1/1] riscv-aplic: manually set pending for the pre-existing interrupts

[Yong-Xuan Wang]

The section 4.5.2 of the RISC-V AIA specification says that any write
to a sourcecfg register of an APLIC might (or might not) cause the
corresponding interrupt-pending bit to be set to one if the rectified
input value is high (= 1) under the new source mode.

If an interrupt is asserted before the driver configs its interrupt
type to APLIC, it's pending bit will not be set except a relevant
write to a setip or setipnum register. When we write the interrupt
type to sourcecfg register, if the APLIC device doesn't check and
update the pending bit, the interrupt might never becomes pending.

For the level interrupts forwarded by MSI, we can manually set the
pending bit if the interrupts have been asserted before the interrupt
type configuration.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
---
 drivers/irqchip/irq-riscv-aplic-main.c |  4 ++++
 drivers/irqchip/irq-riscv-aplic-main.h |  1 +
 drivers/irqchip/irq-riscv-aplic-msi.c  | 17 +++++++++++------
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
index 28dd175b5764..46c44d96123c 100644
--- a/drivers/irqchip/irq-riscv-aplic-main.c
+++ b/drivers/irqchip/irq-riscv-aplic-main.c
@@ -58,6 +58,10 @@ int aplic_irq_set_type(struct irq_data *d, unsigned int type)
 	sourcecfg += (d->hwirq - 1) * sizeof(u32);
 	writel(val, sourcecfg);
 
+	/* manually set pending for the asserting interrupts */
+	if (!priv->nr_idcs)
+		aplic_retrigger_asserting_irq(d);
+
 	return 0;
 }
 
diff --git a/drivers/irqchip/irq-riscv-aplic-main.h b/drivers/irqchip/irq-riscv-aplic-main.h
index 4393927d8c80..c2be66f379b1 100644
--- a/drivers/irqchip/irq-riscv-aplic-main.h
+++ b/drivers/irqchip/irq-riscv-aplic-main.h
@@ -34,6 +34,7 @@ struct aplic_priv {
 
 void aplic_irq_unmask(struct irq_data *d);
 void aplic_irq_mask(struct irq_data *d);
+void aplic_retrigger_asserting_irq(struct irq_data *d);
 int aplic_irq_set_type(struct irq_data *d, unsigned int type);
 int aplic_irqdomain_translate(struct irq_fwspec *fwspec, u32 gsi_base,
 			      unsigned long *hwirq, unsigned int *type);
diff --git a/drivers/irqchip/irq-riscv-aplic-msi.c b/drivers/irqchip/irq-riscv-aplic-msi.c
index 028444af48bd..eaf4d730a49a 100644
--- a/drivers/irqchip/irq-riscv-aplic-msi.c
+++ b/drivers/irqchip/irq-riscv-aplic-msi.c
@@ -32,15 +32,10 @@ static void aplic_msi_irq_unmask(struct irq_data *d)
 	aplic_irq_unmask(d);
 }
 
-static void aplic_msi_irq_eoi(struct irq_data *d)
+void aplic_retrigger_asserting_irq(struct irq_data *d)
 {
 	struct aplic_priv *priv = irq_data_get_irq_chip_data(d);
 
-	/*
-	 * EOI handling is required only for level-triggered interrupts
-	 * when APLIC is in MSI mode.
-	 */
-
 	switch (irqd_get_trigger_type(d)) {
 	case IRQ_TYPE_LEVEL_LOW:
 	case IRQ_TYPE_LEVEL_HIGH:
@@ -59,6 +54,16 @@ static void aplic_msi_irq_eoi(struct irq_data *d)
 	}
 }
 
+static void aplic_msi_irq_eoi(struct irq_data *d)
+{
+	/*
+	 * EOI handling is required only for level-triggered interrupts
+	 * when APLIC is in MSI mode.
+	 */
+
+	aplic_retrigger_asserting_irq(d);
+}
+
 static void aplic_msi_write_msg(struct irq_data *d, struct msi_msg *msg)
 {
 	unsigned int group_index, hart_index, guest_index, val;
-- 
2.17.1

Re: [PATCH 1/1] riscv-aplic: manually set pending for the pre-existing interrupts

[Anup Patel]

More appropriate patch subject should be:

irqchip: riscv-aplic: retrigger interrupt in MSI mode upon write to
sourcecfg register

On Thu, Aug 8, 2024 at 1:44 PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
>
> The section 4.5.2 of the RISC-V AIA specification says that any write
> to a sourcecfg register of an APLIC might (or might not) cause the
> corresponding interrupt-pending bit to be set to one if the rectified
> input value is high (= 1) under the new source mode.

Add quotes around the text from RISC-V AIA specification.

>
> If an interrupt is asserted before the driver configs its interrupt
> type to APLIC, it's pending bit will not be set except a relevant
> write to a setip or setipnum register. When we write the interrupt
> type to sourcecfg register, if the APLIC device doesn't check and
> update the pending bit, the interrupt might never becomes pending.

The second sentence above can be re-written as follows:

When interrupt type is changed in sourcecfg register, the APLIC
device might not set the corresponding pending bit, the interrupt
might never become pending.

>
> For the level interrupts forwarded by MSI, we can manually set the
> pending bit if the interrupts have been asserted before the interrupt
> type configuration.

The above sentence can be re-writtern as follows:

To handle sourcecfg register changes for level-triggered interrupts in
MSI mode, manually set the pending bit for retriggering interrupt if it
was already asserted.

>
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  drivers/irqchip/irq-riscv-aplic-main.c |  4 ++++
>  drivers/irqchip/irq-riscv-aplic-main.h |  1 +
>  drivers/irqchip/irq-riscv-aplic-msi.c  | 17 +++++++++++------
>  3 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
> index 28dd175b5764..46c44d96123c 100644
> --- a/drivers/irqchip/irq-riscv-aplic-main.c
> +++ b/drivers/irqchip/irq-riscv-aplic-main.c
> @@ -58,6 +58,10 @@ int aplic_irq_set_type(struct irq_data *d, unsigned int type)
>         sourcecfg += (d->hwirq - 1) * sizeof(u32);
>         writel(val, sourcecfg);
>
> +       /* manually set pending for the asserting interrupts */
> +       if (!priv->nr_idcs)
> +               aplic_retrigger_asserting_irq(d);
> +

This is not the right place. See below.

>         return 0;
>  }
>
> diff --git a/drivers/irqchip/irq-riscv-aplic-main.h b/drivers/irqchip/irq-riscv-aplic-main.h
> index 4393927d8c80..c2be66f379b1 100644
> --- a/drivers/irqchip/irq-riscv-aplic-main.h
> +++ b/drivers/irqchip/irq-riscv-aplic-main.h
> @@ -34,6 +34,7 @@ struct aplic_priv {
>
>  void aplic_irq_unmask(struct irq_data *d);
>  void aplic_irq_mask(struct irq_data *d);
> +void aplic_retrigger_asserting_irq(struct irq_data *d);
>  int aplic_irq_set_type(struct irq_data *d, unsigned int type);
>  int aplic_irqdomain_translate(struct irq_fwspec *fwspec, u32 gsi_base,
>                               unsigned long *hwirq, unsigned int *type);
> diff --git a/drivers/irqchip/irq-riscv-aplic-msi.c b/drivers/irqchip/irq-riscv-aplic-msi.c
> index 028444af48bd..eaf4d730a49a 100644
> --- a/drivers/irqchip/irq-riscv-aplic-msi.c
> +++ b/drivers/irqchip/irq-riscv-aplic-msi.c
> @@ -32,15 +32,10 @@ static void aplic_msi_irq_unmask(struct irq_data *d)
>         aplic_irq_unmask(d);
>  }
>
> -static void aplic_msi_irq_eoi(struct irq_data *d)
> +void aplic_retrigger_asserting_irq(struct irq_data *d)

This needs to be a static function because we don't require
this for APLIC in direct mode.

Also, rename it to aplic_msi_irq_retrigger_level().

>  {
>         struct aplic_priv *priv = irq_data_get_irq_chip_data(d);
>
> -       /*
> -        * EOI handling is required only for level-triggered interrupts
> -        * when APLIC is in MSI mode.
> -        */
> -
>         switch (irqd_get_trigger_type(d)) {
>         case IRQ_TYPE_LEVEL_LOW:
>         case IRQ_TYPE_LEVEL_HIGH:
> @@ -59,6 +54,16 @@ static void aplic_msi_irq_eoi(struct irq_data *d)
>         }
>  }
>
> +static void aplic_msi_irq_eoi(struct irq_data *d)
> +{
> +       /*
> +        * EOI handling is required only for level-triggered interrupts
> +        * when APLIC is in MSI mode.
> +        */
> +
> +       aplic_retrigger_asserting_irq(d);
> +}
> +

Define APLIC MSI mode specific irq_set_type() like below:

int aplic_msi_irq_set_type(struct irq_data *d, unsigned int type)
{
       int rc;

       rc = aplic_irq_set_type(d, type);
       if (rc)
              return rc;

       /*
        * Updating sourcecfg register for level-triggered interrupts
        * requires interrupt retriggering when APLIC in MSI mode.
        */
       aplic_msi_irq_retrigger_level(d);
       return 0;
}

>  static void aplic_msi_write_msg(struct irq_data *d, struct msi_msg *msg)
>  {
>         unsigned int group_index, hart_index, guest_index, val;
> --
> 2.17.1
>

Regards,
Anup

Re: [PATCH 1/1] riscv-aplic: manually set pending for the pre-existing interrupts

[Thomas Gleixner]

On Thu, Aug 08 2024 at 18:56, Anup Patel wrote:
> More appropriate patch subject should be:
>
> irqchip: riscv-aplic: retrigger interrupt in MSI mode upon write to
> sourcecfg register

And the correct one would be:

    irqchip/riscv-aplic: Retrigger MSI interrupt on source configuration

1) The prefix is correct

2) Sentence starts with a uppercase letter

3) It uses understandable words. sourcecfg is a implementation detail
   which is irrelevant for the high level overview of a changelog
   subject, which is visible in the short log.

> On Thu, Aug 8, 2024 at 1:44 PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
>>
>> The section 4.5.2 of the RISC-V AIA specification says that any write
>> to a sourcecfg register of an APLIC might (or might not) cause the
>> corresponding interrupt-pending bit to be set to one if the rectified
>> input value is high (= 1) under the new source mode.
>
> Add quotes around the text from RISC-V AIA specification.
>
>>
>> If an interrupt is asserted before the driver configs its interrupt
>> type to APLIC, it's pending bit will not be set except a relevant
>> write to a setip or setipnum register. When we write the interrupt
>> type to sourcecfg register, if the APLIC device doesn't check and
>> update the pending bit, the interrupt might never becomes pending.
>
> The second sentence above can be re-written as follows:
>
> When interrupt type is changed in sourcecfg register, the APLIC

the interrupt type ... in the source....

> device might not set the corresponding pending bit, the interrupt

bit , so the ...

> might never become pending.

>
> Define APLIC MSI mode specific irq_set_type() like below:
>
> int aplic_msi_irq_set_type(struct irq_data *d, unsigned int type)

static :)

> {
>        int rc;
>
>        rc = aplic_irq_set_type(d, type);

         int rc = aplic...

>        if (rc)
>               return rc;
>
>        /*
>         * Updating sourcecfg register for level-triggered interrupts
>         * requires interrupt retriggering when APLIC in MSI mode.

APLIC is in ....

>         */
>        aplic_msi_irq_retrigger_level(d);

Thanks,

        tglx

Re: [PATCH 1/1] riscv-aplic: manually set pending for the pre-existing interrupts

[Yong-Xuan Wang]

Hi Anup and Thomas,

On Thu, Aug 8, 2024 at 10:34 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Thu, Aug 08 2024 at 18:56, Anup Patel wrote:
> > More appropriate patch subject should be:
> >
> > irqchip: riscv-aplic: retrigger interrupt in MSI mode upon write to
> > sourcecfg register
>
> And the correct one would be:
>
>     irqchip/riscv-aplic: Retrigger MSI interrupt on source configuration
>
> 1) The prefix is correct
>
> 2) Sentence starts with a uppercase letter
>
> 3) It uses understandable words. sourcecfg is a implementation detail
>    which is irrelevant for the high level overview of a changelog
>    subject, which is visible in the short log.
>
> > On Thu, Aug 8, 2024 at 1:44 PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
> >>
> >> The section 4.5.2 of the RISC-V AIA specification says that any write
> >> to a sourcecfg register of an APLIC might (or might not) cause the
> >> corresponding interrupt-pending bit to be set to one if the rectified
> >> input value is high (= 1) under the new source mode.
> >
> > Add quotes around the text from RISC-V AIA specification.
> >
> >>
> >> If an interrupt is asserted before the driver configs its interrupt
> >> type to APLIC, it's pending bit will not be set except a relevant
> >> write to a setip or setipnum register. When we write the interrupt
> >> type to sourcecfg register, if the APLIC device doesn't check and
> >> update the pending bit, the interrupt might never becomes pending.
> >
> > The second sentence above can be re-written as follows:
> >
> > When interrupt type is changed in sourcecfg register, the APLIC
>
> the interrupt type ... in the source....
>
> > device might not set the corresponding pending bit, the interrupt
>
> bit , so the ...
>
> > might never become pending.
>
> >
> > Define APLIC MSI mode specific irq_set_type() like below:
> >
> > int aplic_msi_irq_set_type(struct irq_data *d, unsigned int type)
>
> static :)
>
> > {
> >        int rc;
> >
> >        rc = aplic_irq_set_type(d, type);
>
>          int rc = aplic...
>
> >        if (rc)
> >               return rc;
> >
> >        /*
> >         * Updating sourcecfg register for level-triggered interrupts
> >         * requires interrupt retriggering when APLIC in MSI mode.
>
> APLIC is in ....
>
> >         */
> >        aplic_msi_irq_retrigger_level(d);
>
> Thanks,
>
>         tglx

Thanks a lot! I will fix them in the next version.

Regards,
Yong-Xuan