On Thu, Feb 16, 2023 at 02:36:53PM +0800, Huisong Li wrote:
> If the platform acknowledge interrupt is level triggered, then it can
> be shared by multiple subspaces provided each one has a unique platform
> interrupt ack preserve and ack set masks.
>
> If it can be shared, then we can request the irq with IRQF_SHARED and
> IRQF_ONESHOT flags. The first one indicating it can be shared and the
> latter one to keep the interrupt disabled until the hardirq handler
> finished.
>
> Further, since there is no way to detect if the interrupt is for a given
> channel as the interrupt ack preserve and ack set masks are for clearing
> the interrupt and not for reading the status(in case Irq Ack register
> may be write-only on some platforms), we need a way to identify if the
> given channel is in use and expecting the interrupt.
>
> PCC type0, type1 and type5 do not support shared level triggered interrupt.
> The methods of determining whether a given channel for remaining types
> should respond to an interrupt are as follows:
> - type2: Whether the interrupt belongs to a given channel is only
> determined by the status field in Generic Communications Channel
> Shared Memory Region, which is done in rx_callback of PCC client.
> - type3: This channel checks chan_in_use flag first and then checks the
> command complete bit(value '1' indicates that the command has
> been completed).
> - type4: Platform ensure that the default value of the command complete
> bit corresponding to the type4 channel is '1'. This command
> complete bit is '0' when receive a platform notification.
>
> Signed-off-by: Huisong Li <lihuisong@xxxxxxxxxx>
> ---
> drivers/mailbox/pcc.c | 45 ++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index ecd54f049de3..04c2d73a0473 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -92,6 +92,10 @@ struct pcc_chan_reg {
> * @error: PCC register bundle for the error status register
> * @plat_irq: platform interrupt
> * @type: PCC subspace type
> + * @plat_irq_flags: platform interrupt flags
> + * @chan_in_use: flag indicating whether the channel is in use or not when use
> + * platform interrupt, and only use it for communication from OSPM
> + * to Platform, like type 3.
Also add a node that since only one transfer can occur at a time and the
mailbox takes care of locking, this flag needs no locking and is used just
to check if the interrupt needs handling when it is shared.
> */
> struct pcc_chan_info {
> struct pcc_mbox_chan chan;
> @@ -102,6 +106,8 @@ struct pcc_chan_info {
> struct pcc_chan_reg error;
> int plat_irq;
> u8 type;
> + unsigned int plat_irq_flags;
> + bool chan_in_use;
> };
>
> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
> @@ -225,6 +231,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;
> +}
> +
> static bool pcc_chan_command_complete(struct pcc_chan_info *pchan,
> u64 cmd_complete_reg_val)
> {
> @@ -277,6 +289,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
> int ret;
>
> pchan = chan->con_priv;
> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE &&
> + !pchan->chan_in_use)
> + return IRQ_NONE;
>
> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> if (ret)
> @@ -302,9 +317,13 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
> /*
> * The PCC slave subspace channel needs to set the command complete bit
> * and ring doorbell after processing message.
> + *
> + * The PCC master subspace channel clears chan_in_use to free channel.
> */
> if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> pcc_send_data(chan, NULL);
> + else if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE)
> + pchan->chan_in_use = false;
Just wondering if this has to be for type 3 only. I am trying to avoid
conditional update of this flag, can we not do it for everything except type4 ?
(I mean just in unconditional else part)
>
> return IRQ_HANDLED;
> }
> @@ -353,10 +372,13 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
> spin_unlock_irqrestore(&chan->lock, flags);
>
> if (pchan->plat_irq > 0) {
> + unsigned long irqflags;
> int rc;
>
> - 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);
> @@ -418,7 +440,17 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
> if (ret)
> return ret;
>
> - return pcc_chan_reg_read_modify_write(&pchan->db);
> + ret = pcc_chan_reg_read_modify_write(&pchan->db);
> + /*
> + * For the master subspace channel, set chan_in_use flag to true after
> + * ring doorbell, and clear this flag when the reply message is
> + * processed.
> + */
> + if (!ret && pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE &&
> + pchan->plat_irq > 0)
> + pchan->chan_in_use = true;
Ditto here(for all type except type 4?)
--
Regards,
Sudeep
