Re: [PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

[Hanjun Guo]

On 2025/11/18 18:09, Kazuhiro Abe (Fujitsu) wrote:
> Hi Hanjun,
>
> Thanks for your comments.
>
> > > > > AGDI has two types of signaling modes: SDEI and interrupt.
> > > > > Currently, the AGDI driver only supports SDEI.
> > > > > Therefore, add support for interrupt signaling mode The interrupt
> > > > > vector is retrieved from the AGDI table, and call panic function
> > > > > when an interrupt occurs.
> > > > >
> > > > > Reviewed-by: Ilkka Koskinen<ilkka@xxxxxxxxxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Kazuhiro Abe<fj1078ii@xxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > > Hanjun, I have addressed all your comments.
> > > > > Please review them.
> > > > >
> > > > > v3->v4
> > > > >     - Add a comment to the flags member.
> > > > >     - Fix agdi_interrupt_probe.
> > > > >     - Fix agdi_interrupt_remove.
> > > > >     - Add space in struct initializsation.
> > > > >     - Delete curly braces.
> > > > Looks good to me,
> > > >
> > > > Acked-by: Hanjun Guo<guohanjun@xxxxxxxxxx>
> > > I wasn't cc'd on the original patch but I couldn't figure out why it
> > > uses IRQF_NO_AUTOEN when requesting the irq given that the first thing
> > > it does is enable it.
> > Sorry for the late reply, seems this comment was addressed.
> >
> > But when I go back to read the latest AGDI spec [0], I see the signaling mode
> > can be SDEI*and*  interrupt:
> >
> > Flags node structures 1 36 Bits [1:0]: Signaling mode:
> >
> > 2: Both SDEI and Interrupt-based signaling is supported. While an SDEI event
> > handler is registered, the platform is allowed to not generate the wired interrupt
> >
> > So which means we need to support both SDEI and interrupt when the
> > signaling mode is 2, but for now, the code is
> >
> > +	if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE)
> > +		pdata.gsiv = agdi_table->gsiv;
> > +	else
> > +		pdata.sdei_event = agdi_table->sdei_event;
> >
> > Kazuhiro, could you take a look again?
> I don't think enabling both interrupt and SDEI makes sense.
> Therefore, we need to choose one but we can't know which one is preferred for each platform.
> On the other hand, before this patch the code is:
>
>         if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE) {
>                 pr_warn("Interrupt signaling is not supported");
>                 goto err_put_table;
>         }
>
> ...and SDEI is used when mode is 2. This behavior will not be changed by this patch.
> Therefore, It seems for me this patch is reasonable for now.
> I would appreciate your thoughts on this.

I had a talk to firmware guy about this, they say even with both
supported, the platform will use the same interrupt number to
trigger, and the spec says:

While an SDEI event handler is registered, the platform is allowed to
not generate the wired interrupt

I think this patch is ready to go.

Thanks
Hanjun