Re: [RFC] ACPI: PCC: Support shared interrupt for multiple subspaces

[lihuisong (C)]

在 2022/11/4 23:39, Robbie King 写道:
> On 11/4/2022 11:15 AM, Sudeep Holla wrote:
> > On Fri, Nov 04, 2022 at 11:04:22AM -0400, Robbie King wrote:
> > > Hello Huisong, your raising of the shared interrupt issue is very 
> > > timely, I
> > > am working to implement "Extended PCC subspaces (types 3 and 4)" 
> > > using PCC
> > > on the ARM RDN2 reference platform as a proof of concept, and 
> > > encountered
> > > this issue as well.  FWIW, I am currently testing using Sudeep's 
> > > patch with
> > > the "chan_in_use" flag removed, and so far have not encountered any 
> > > issues.
> >
> > Interesting, do you mean the patch I post in this thread but without the
> > whole chan_in_use flag ?
>
> That's right, diff I'm running with is attached to end of message.
Hello Robbie, In multiple subspaces scenario, there is a problem
that OS doesn't know which channel should respond to the interrupt
if no this chan_in_use flag. If you have not not encountered any
issues in this case, it may be related to your register settings.

@Sudeep, what shoud we do next?
> > > I think the RDN2 may provide an example of a write only interrupt
> > > acknowledge mechanism mentioned by Sudeep.
> >
> > Yes.
> >
> > > The RDN2 reference design uses the MHUv2 IP for the doorbell 
> > > mechanism.  If
> > > my implementation is correct (and it quite possibly is not), 
> > > acknowledging
> > > the DB interrupt from the platform is accomplished by writing a 1 to 
> > > the
> > > appropriate bit in the receiver channel window CH_CLR register, 
> > > which is
> > > documented as:
> > >
> > >    Channel flag clear.
> > >    Write 0b1 to a bit clears the corresponding bit in the CH_ST and 
> > > CH_ST_MSK.
> > >    Writing 0b0 has no effect.
> > >    Each bit always reads as 0b0.
> >
> > Correct, on this MHUv[1-2], it is write only register and it reads zero.
> > So basically you will ignore the interrupt if we apply the logic Huisong
> > proposed initially.
> >
> > > in the "Arm Corstone SSE-700 Subsystem Technical Reference Manual".
> > >
> > > Apologies if I am off in the weeds here as I have only been working 
> > > with
> > > PCC/SCMI for a very short period of time.
> >
> > Good to know info :).
>
> It helps that your linux / firmware code is easy to follow! :)
>
> One other minor issue I encountered was that a NULL GAS (all zeros) 
> doesn't
> seem to be supported by pcc_chan_reg_init, may be a good opportunity 
> for me
> to submit my first RFC...
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index ed18936b8ce6..3fa7335d15b0 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -100,6 +100,7 @@ struct pcc_chan_info {
>         struct pcc_chan_reg cmd_update;
>         struct pcc_chan_reg error;
>         int plat_irq;
> +       unsigned int plat_irq_flags;
>  };
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index ed18936b8ce6..3fa7335d15b0 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -100,6 +100,7 @@ struct pcc_chan_info {
>         struct pcc_chan_reg cmd_update;
>         struct pcc_chan_reg error;
>         int plat_irq;
> +       unsigned int plat_irq_flags;
>  };
>
>  #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
> @@ -221,6 +222,12 @@ static int pcc_map_interrupt(u32 interrupt, u32 
> flags)
>         return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>  }
>
> +static bool pcc_chan_plat_irq_can_be_shared(struct pcc_chan_info *pchan)
> +{
> +       return (pchan->plat_irq_flags & ACPI_PCCT_INTERRUPT_MODE) ==
> +               ACPI_LEVEL_SENSITIVE;
> +}
> +
>  /**
>   * pcc_mbox_irq - PCC mailbox interrupt handler
>   * @irq:       interrupt number
> @@ -310,9 +317,12 @@ pcc_mbox_request_channel(struct mbox_client *cl, 
> int subspace_id)
>
>         if (pchan->plat_irq > 0) {
>                 int rc;
> +               unsigned long irqflags;
>
> -               rc = devm_request_irq(dev, pchan->plat_irq, 
> pcc_mbox_irq, 0,
> -                                     MBOX_IRQ_NAME, chan);
> +               irqflags = pcc_chan_plat_irq_can_be_shared(pchan) ?
> +                           IRQF_SHARED | IRQF_ONESHOT : 0;
> +               rc = devm_request_irq(dev, pchan->plat_irq, pcc_mbox_irq,
> +                                     irqflags, MBOX_IRQ_NAME, chan);
>                 if (unlikely(rc)) {
>                         dev_err(dev, "failed to register PCC interrupt 
> %d\n",
>                                 pchan->plat_irq);
> @@ -458,6 +468,8 @@ static int pcc_parse_subspace_irq(struct 
> pcc_chan_info *pchan,
>                 return -EINVAL;
>         }
>
> +       pchan->plat_irq_flags = pcct_ss->flags;
> +
>         if (pcct_ss->header.type == 
> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
>                 struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void 
> *)pcct_ss;
>
>
> .