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

["lihuisong (C)" <lihuisong@huawei.com>]

在 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;
>
>
> .