Re: [RFC] ACPI: PCC: Support shared interrupt for multiple subspaces
From: Robbie King
Date: Fri Nov 04 2022 - 11:04:46 EST
On 10/31/2022 10:49 PM, lihuisong (C) wrote:
在 2022/10/31 18:40, Sudeep Holla 写道:
On Fri, Oct 28, 2022 at 03:55:54PM +0800, lihuisong (C) wrote:
在 2022/10/27 23:53, Sudeep Holla 写道:
On Sun, Oct 16, 2022 at 11:40:43AM +0800, Huisong Li wrote:
As ACPI protocol descripted, if interrupts are level, a GSIV may
be shared by multiple subspaces, but each one must have unique
platform interrupt ack preserve and ack set masks. Therefore, need
set to shared interrupt for types that can distinguish interrupt
response channel if platform interrupt mode is level triggered.
The distinguishing point isn't definitely command complete register.
Because the two status values of command complete indicate that
there is no interrupt in a subspace('1' means subspace is free for
use, and '0' means platform is processing the command). On the whole,
the platform interrupt ack register is more suitable for this role.
As ACPI protocol said, If the subspace does support interrupts, and
these are level, this register must be supplied. And is used to clear
the interrupt by using a read, modify, write sequence. This register
is a 'WR' register, the bit corresponding to the subspace is '1' when
the command is completed, or is '0'.
Therefore, register shared interrupt for multiple subspaces if support
platform interrupt ack register and interrupts are level, and read the
ack register to ensure the idle or unfinished command channels to
quickly return IRQ_NONE.
Signed-off-by: Huisong Li <lihuisong@xxxxxxxxxx>
---
drivers/mailbox/pcc.c | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3c2bc0ca454c..86c6cc44c73d 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;
+ u8 plat_irq_trigger;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
@@ -236,6 +237,15 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
int ret;
pchan = chan->con_priv;
+ ret = pcc_chan_reg_read(&pchan->plat_irq_ack, &val);
+ if (ret)
+ return IRQ_NONE;
+ /* Irq ack GAS exist and check if this interrupt has the channel. */
+ if (pchan->plat_irq_ack.gas) {
+ val &= pchan->plat_irq_ack.set_mask;
I am not sure if the above is correct. The spec doesn't specify that the
set_mask can be used to detect if the interrupt belongs to this channel.
We need clarification to use those bits.
Yes, the spec only say that the interrupt ack register is used to clear the
interrupt by using a read, modify, write sequence. But the processing
of PCC driver is as follows:
Irq Ack Register = (Irq Ack Register & Preserve_mask) | Set_mask
The set_mask is using to clear the interrupt of this channel by using OR
operation. And it should be write '1' to the corresponding bit of the
channel
to clear interrupt. So I think it is ok to use set_mask to detect if the
interrupt belongs to this channel.
The problem is it can be write-only register and reads as always zero.
But it seems that it must be a read/write register according to the ACPI spec.
I know a platform with such a behaviour.
Can you tell me which platform?
This triggered be that I have a patch to address this. I will try to search
and share, but IIRC I had a flag set when the doorbell was rung to track
which channel or when to expect the irq. I will dig that up.
Looking forward to your patch.😁
Please find below. I am not convinced yet to have extra flag for checking if
the channel is in use. The other idea I had is to use the Generic Communications
Channel Shared Memory Region Status Field in particular Command Complete
field. I haven't tried it yet and hence the reason for not posting the patch.
Let me know if the idea looks sane, so that I can try something and share
I don't think it is feasible. From the spec, the Command Complete field in the Generic
Communications Channel Shared Memory Region Status Field for type1/2 is similar to
the Command Complete Check Register in Master Slave Communications Channel Shared
Memory Region for type3/4.
it. I may not have a setup handy to test and may need sometime to test though.
Regards,
Sudeep
-->8
>From 6dd9ad4f3a11dc9b97d308e70b544337c4169803 Mon Sep 17 00:00:00 2001
From: Sudeep Holla <sudeep.holla@xxxxxxx>
Date: Thu, 27 Oct 2022 21:51:39 +0100
Subject: [PATCH] ACPI: PCC: Support shared level triggered interrupt for
multiple subspaces
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 after the hardirq handler
finished.
after --> until , right?
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, we need a way to identify
if the given channel is in use and expecting the interrupt.
Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
---
drivers/mailbox/pcc.c | 36 +++++++++++++++++++++++++++++++++---
1 file changed, 33 insertions(+), 3 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3c2bc0ca454c..a61528c874a2 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -91,6 +91,8 @@ struct pcc_chan_reg {
* @cmd_update: PCC register bundle for the command complete update register
* @error: PCC register bundle for the error status register
* @plat_irq: platform interrupt
+ * @plat_irq_flags: platform interrupt flags
+ * @chan_in_use: flag indicating whether the channel is in use or not
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -100,6 +102,8 @@ struct pcc_chan_info {
struct pcc_chan_reg cmd_update;
struct pcc_chan_reg error;
int plat_irq;
+ unsigned int plat_irq_flags;
+ bool chan_in_use;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
@@ -221,6 +225,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
@@ -237,6 +247,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
pchan = chan->con_priv;
+ if (!pchan->chan_in_use)
+ return IRQ_NONE;
+
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
if (ret)
return IRQ_NONE;
@@ -262,6 +275,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
mbox_chan_received_data(chan, NULL);
This flag should be set to false when the Error status register indicates that the channel has an error.
what do you think?
+ pchan->chan_in_use = false;
Maybe need add following logic?
if (pchan->plat_irq_ack.gas)
pchan->chan_in_use = false;
+
return IRQ_HANDLED;
}
@@ -310,9 +325,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);
@@ -374,7 +392,11 @@ 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);
+ if (!ret)
+ pchan->chan_in_use = true;
+
+ return ret;
}
static const struct mbox_chan_ops pcc_chan_ops = {
@@ -458,6 +480,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;
@@ -478,6 +502,12 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
"PLAT IRQ ACK");
}
+ if (pcc_chan_plat_irq_can_be_shared(pchan) &&
+ !pchan->plat_irq_ack.gas) {
+ pr_err("PCC subspace has level IRQ with no ACK register\n");
+ return -EINVAL;
+ }
+
return ret;
}
--
2.38.1
Hi Sudeep,
ACPI spec requested that the Irq Ack Register is a read/write register. From this point,
only using this register supports for detecting if the interrupt is for a given channel
as my patch implemented. But If we need consider the platform whose Irq Ack Register is
write-only register, the chan_in_use flag in your patch looks good to me.
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.
I think the RDN2 may provide an example of a write only interrupt acknowledge mechanism
mentioned by Sudeep.
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.
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.
Regards,
Robbie
Regards,
Huisong
.