| Message ID | 20240808081412.24553-1-yongxuan.wang@sifive.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] riscv-aplic: manually set pending for the pre-existing interrupts | expand |
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
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
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
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;