[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 at 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 at sifive.com>
> Reviewed-by: Vincent Chen <vincent.chen at 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