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 <[email protected]>
---
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;
+ if (val == 0)
+ return IRQ_NONE;
+ }
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
if (ret)
@@ -309,10 +319,21 @@ 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);
+ /*
+ * As ACPI protocol descripted, if interrupts are level, a GSIV
+ * may be shared by multiple subspaces.
+ * Therefore, register shared interrupt for multiple subspaces
+ * if support platform interrupt ack register and interrupts
+ * are level.
+ */
+ irqflags = (pchan->plat_irq_ack.gas &&
+ pchan->plat_irq_trigger == ACPI_LEVEL_SENSITIVE) ?
+ IRQF_SHARED : 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);
@@ -457,6 +478,8 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
pcct_ss->platform_interrupt);
return -EINVAL;
}
+ pchan->plat_irq_trigger = (pcct_ss->flags & ACPI_PCCT_INTERRUPT_MODE) ?
+ ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
--
2.33.0
Kindly ping.
在 2022/10/16 11:40, Huisong Li 写道:
> 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 <[email protected]>
> ---
> 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;
> + if (val == 0)
> + return IRQ_NONE;
> + }
>
> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> if (ret)
> @@ -309,10 +319,21 @@ 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);
> + /*
> + * As ACPI protocol descripted, if interrupts are level, a GSIV
> + * may be shared by multiple subspaces.
> + * Therefore, register shared interrupt for multiple subspaces
> + * if support platform interrupt ack register and interrupts
> + * are level.
> + */
> + irqflags = (pchan->plat_irq_ack.gas &&
> + pchan->plat_irq_trigger == ACPI_LEVEL_SENSITIVE) ?
> + IRQF_SHARED : 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);
> @@ -457,6 +478,8 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
> pcct_ss->platform_interrupt);
> return -EINVAL;
> }
> + pchan->plat_irq_trigger = (pcct_ss->flags & ACPI_PCCT_INTERRUPT_MODE) ?
> + ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
>
> if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
> struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
On Wed, Oct 26, 2022 at 02:10:13PM +0800, lihuisong (C) wrote:
> Kindly ping.
>
Sorry for the delay, I will take a look ASAP. I wanted to refer the spec
before I review this.
--
Regards,
Sudeep
在 2022/10/27 19:10, Sudeep Holla 写道:
> On Wed, Oct 26, 2022 at 02:10:13PM +0800, lihuisong (C) wrote:
>> Kindly ping.
>>
> Sorry for the delay, I will take a look ASAP. I wanted to refer the spec
> before I review this.
Thanks, Sudee. Looking forward your reply.
>
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 <[email protected]>
> ---
> 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.
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.
> + if (val == 0)
> + return IRQ_NONE;
> + }
>
> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> if (ret)
> @@ -309,10 +319,21 @@ 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);
> + /*
> + * As ACPI protocol descripted, if interrupts are level, a GSIV
> + * may be shared by multiple subspaces.
> + * Therefore, register shared interrupt for multiple subspaces
> + * if support platform interrupt ack register and interrupts
> + * are level.
> + */
> + irqflags = (pchan->plat_irq_ack.gas &&
> + pchan->plat_irq_trigger == ACPI_LEVEL_SENSITIVE) ?
> + IRQF_SHARED : 0;
We can hide all the details in a macro or oneline function that returns if
the interrupt can be shared. Also since this is threaded interrupt, you may
need to keep it disabled until the thread handler is run.
--
Regards,
Sudeep
在 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 <[email protected]>
>> ---
>> 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.
>
> 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.????
>
>> + if (val == 0)
>> + return IRQ_NONE;
>> + }
>>
>> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
>> if (ret)
>> @@ -309,10 +319,21 @@ 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);
>> + /*
>> + * As ACPI protocol descripted, if interrupts are level, a GSIV
>> + * may be shared by multiple subspaces.
>> + * Therefore, register shared interrupt for multiple subspaces
>> + * if support platform interrupt ack register and interrupts
>> + * are level.
>> + */
>> + irqflags = (pchan->plat_irq_ack.gas &&
>> + pchan->plat_irq_trigger == ACPI_LEVEL_SENSITIVE) ?
>> + IRQF_SHARED : 0;
> We can hide all the details in a macro or oneline function that returns if
Ack
> the interrupt can be shared. Also since this is threaded interrupt, you may
> need to keep it disabled until the thread handler is run.
'it' means 'interrupt', right? If it is, I don't understand why it needs to
be disabled. The irq handlers under this irq number are called serially when
the interrupt is triggered.
>
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 <[email protected]>
> > > ---
> > > 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.
I know a platform with such a behaviour.
> > 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
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 <[email protected]>
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.
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 <[email protected]>
---
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);
+ 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
在 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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.
Regards,
Huisong
>
>
>
> .
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 <[email protected]>
>>>>> ---
>>>>> 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 <[email protected]>
>> 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 <[email protected]>
>> ---
>> 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
>>
>>
>>
>> .
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 ?
> 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 :).
--
Regards,
Sudeep
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.
>
>> 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;
在 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;
>
>
> .
On 11/7/2022 1:24 AM, lihuisong (C) wrote:
>
> 在 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.
>
Hi Huisong, apologies, I see your point now concerning multiple subspaces.
I have started stress testing where I continuously generate both requests
and notifications as quickly as possible, and unfortunately found an issue
even with the original chan_in_use patch. I first had to modify the patch
to get the type 4 channel notifications to function at all, essentially
ignoring the chan_in_use flag for that channel. With that change, I still
hit my original stress issue, where the pcc_mbox_irq function did not
correctly ignore an interrupt for the type 3 channel.
The issue occurs when a request from AP to SCP over the type 3 channel is
outstanding, and simultaneously the SCP initiates a notification over the
type 4 channel. Since the two channels share an interrupt, both handlers
are invoked.
I've tried to draw out the state of the channel status "free" bits along
with the AP and SCP function calls involved.
type 3
------
(1)pcc.c:pcc_send_data()
| (5) mailbox.c:mbox_chan_receive_data()
_______v (4)pcc.c:pcc_mbox_irq()
free \_________________________________________
^
type 4 ^
------ ^
_____________________
free \_____________________________
^ ^
| |
(2)mod_smt.c:smt_transmit() |
|
(3)mod_mhu2.c:raise_interrupt()
The sequence of events are:
1) OS initiates request to SCP by clearing FREE in status and ringing SCP doorbell
2) SCP initiates notification by filling shared memory and clearing FREE in status
3) SCP notifies OS by ringing OS doorbell
4) OS first invokes interrupt handler for type 3 channel
At this step, the issue is that "val" from reading status (i.e. CommandCompleteCheck)
is zero (SCP has not responded yet) so the code below falls through and continues
to processes the interrupt as if the request has been acknowledged by the SCP.
if (val) { /* Ensure GAS exists and value is non-zero */
val &= pchan->cmd_complete.status_mask;
if (!val)
return IRQ_NONE;
}
The chan_in_use flag does not address this because the channel is indeed in use.
5) ACPI:PCC client kernel module is incorrectly notified that response data is
available
I added the following fix (applied on top of Sudeep's original patch for clarity)
for the issue above which solved the stress test issue. I've changed the interrupt
handler to explicitly verify that the status value matches the mask for type 3
interrupts before acknowledging them. Conversely, a type 4 channel verifies that
the status value does *not* match the mask, since in this case we are functioning
as the recipient, not the initiator.
One concern is that since this fundamentally changes handling of the channel status,
that existing platforms could be impacted.
For reference, here are my Pcct.aslc tables:
{
EFI_ACPI_6_4_PCCT_SUBSPACE_TYPE_3_EXTENDED_PCC,
...
ARM_GAS32(0x06000004ULL), // CommandCompleteCheckRegister
0x00000001ULL, // CommandCompleteCheckMask
ARM_GAS32(0x06000004ULL), // CommandCompleteUpdateRegister
0xFFFFFFFEULL, // CommandCompleteUpdatePreserve
0x00000000ULL, // CommandCompleteUpdateSet
ARM_GAS32(0x06000004ULL), // ErrorStatusRegister
0x00000002ULL, // ErrorStatusMask
},
{
EFI_ACPI_6_4_PCCT_SUBSPACE_TYPE_4_EXTENDED_PCC,
...
ARM_GAS32(0x06000084ULL), // CommandCompleteCheckRegister
0x00000001ULL, // CommandCompleteCheckMask
ARM_GAS32(0x06000084ULL), // CommandCompleteUpdateRegister
0xFFFFFFFEULL, // CommandCompleteUpdatePreserve
0x00000001ULL, // CommandCompleteUpdateSet
ARM_GAS32(0x06000084ULL), // ErrorStatusRegister
0x00000002ULL, // ErrorStatusMask
},
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index f8febc4f3270..a7dfcb5503ff 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -93,6 +93,8 @@ struct pcc_chan_reg {
* @plat_irq: platform interrupt
* @plat_irq_flags: platform interrupt flags
* @chan_in_use: flag indicating whether the channel is in use or not
+ * @is_controller: flow of data on the channel is controlled locally
+ * (as opposed to notifications which originate remotely)
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -104,6 +106,7 @@ struct pcc_chan_info {
int plat_irq;
unsigned int plat_irq_flags;
bool chan_in_use;
+ bool is_controller;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
@@ -243,22 +246,32 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
struct pcc_chan_info *pchan;
struct mbox_chan *chan = p;
u64 val;
+ u64 cmp;
int ret;
pchan = chan->con_priv;
- if (!pchan->chan_in_use)
+ if (pchan->is_controller && !pchan->chan_in_use)
return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
if (ret)
return IRQ_NONE;
- if (val) { /* Ensure GAS exists and value is non-zero */
- val &= pchan->cmd_complete.status_mask;
- if (!val)
- return IRQ_NONE;
- }
+ /*
+ * When we control data flow on the channel, we expect
+ * to see the mask bit(s) set by the remote to indicate
+ * the presence of a valid response. When we do not
+ * control the flow (i.e. type 4) the opposite is true.
+ */
+ if (pchan->is_controller)
+ cmp = pchan->cmd_complete.status_mask;
+ else
+ cmp = 0;
+
+ val &= pchan->cmd_complete.status_mask;
+ if (cmp != val)
+ return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->error, &val);
if (ret)
@@ -704,6 +717,9 @@ static int pcc_mbox_probe(struct platform_device *pdev)
pcc_mbox_channels[i].con_priv = pchan;
pchan->chan.mchan = &pcc_mbox_channels[i];
+ pchan->is_controller =
+ (pcct_entry->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE);
+
if (pcct_entry->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE &&
!pcc_mbox_ctrl->txdone_irq) {
pr_err("Plaform Interrupt flag must be set to 1");
在 2022/11/18 2:09, Robbie King 写道:
> On 11/7/2022 1:24 AM, lihuisong (C) wrote:
>>
>> 在 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.
>>
>
> Hi Huisong, apologies, I see your point now concerning multiple
> subspaces.
>
> I have started stress testing where I continuously generate both requests
> and notifications as quickly as possible, and unfortunately found an
> issue
> even with the original chan_in_use patch. I first had to modify the
> patch
> to get the type 4 channel notifications to function at all, essentially
> ignoring the chan_in_use flag for that channel. With that change, I
> still
> hit my original stress issue, where the pcc_mbox_irq function did not
> correctly ignore an interrupt for the type 3 channel.
>
> The issue occurs when a request from AP to SCP over the type 3 channel is
> outstanding, and simultaneously the SCP initiates a notification over the
> type 4 channel. Since the two channels share an interrupt, both handlers
> are invoked.
>
> I've tried to draw out the state of the channel status "free" bits along
> with the AP and SCP function calls involved.
>
> type 3
> ------
>
> (1)pcc.c:pcc_send_data()
> | (5) mailbox.c:mbox_chan_receive_data()
> _______v (4)pcc.c:pcc_mbox_irq()
> free \_________________________________________
>
> ^
> type 4 ^
> ------ ^
> _____________________
> free \_____________________________
> ^ ^
> | |
> (2)mod_smt.c:smt_transmit() |
> |
> (3)mod_mhu2.c:raise_interrupt()
>
> The sequence of events are:
>
> 1) OS initiates request to SCP by clearing FREE in status and ringing
> SCP doorbell
> 2) SCP initiates notification by filling shared memory and clearing
> FREE in status
> 3) SCP notifies OS by ringing OS doorbell
> 4) OS first invokes interrupt handler for type 3 channel
>
> At this step, the issue is that "val" from reading status (i.e.
> CommandCompleteCheck)
> is zero (SCP has not responded yet) so the code below falls through
> and continues
> to processes the interrupt as if the request has been acknowledged
> by the SCP.
>
> if (val) { /* Ensure GAS exists and value is non-zero */
> val &= pchan->cmd_complete.status_mask;
> if (!val)
> return IRQ_NONE;
> }
>
> The chan_in_use flag does not address this because the channel is
> indeed in use.
>
> 5) ACPI:PCC client kernel module is incorrectly notified that response
> data is
> available
Indeed, chan_in_use flag is invalid for type4.
> I added the following fix (applied on top of Sudeep's original patch
> for clarity)
> for the issue above which solved the stress test issue. I've changed
> the interrupt
> handler to explicitly verify that the status value matches the mask
> for type 3
> interrupts before acknowledging them. Conversely, a type 4 channel
> verifies that
> the status value does *not* match the mask, since in this case we are
> functioning
> as the recipient, not the initiator.
>
> One concern is that since this fundamentally changes handling of the
> channel status,
> that existing platforms could be impacted.
[snip]
>
> + /*
> + * When we control data flow on the channel, we expect
> + * to see the mask bit(s) set by the remote to indicate
> + * the presence of a valid response. When we do not
> + * control the flow (i.e. type 4) the opposite is true.
> + */
> + if (pchan->is_controller)
> + cmp = pchan->cmd_complete.status_mask;
> + else
> + cmp = 0;
> +
> + val &= pchan->cmd_complete.status_mask;
> + if (cmp != val)
> + return IRQ_NONE;
>
We don't have to use the pchan->cmd_complete.status_mask as above.
For the communication from AP to SCP, if this channel is in use, command
complete bit is 1 indicates that the command being executed has been
completed.
For the communication from SCP to AP, if this channel is in use, command
complete bit is 0 indicates that the bit has been cleared and OS should
response the interrupt.
So you concern should be gone if we do as following approach:
"
val &= pchan->cmd_complete.status_mask;
need_rsp_irq = pchan->is_controller ? val != 0 : val == 0;
if (!need_rsp_irq)
return IRQ_NONE
"
This may depend on the default value of the command complete register
for each channel(must be 1, means that the channel is free for use).
It is ok for type3 because of chan_in_use flag, while something needs
to do in platform or OS for type4.
> ret = pcc_chan_reg_read(&pchan->error, &val);
> if (ret)
> @@ -704,6 +717,9 @@ static int pcc_mbox_probe(struct platform_device
> *pdev)
> pcc_mbox_channels[i].con_priv = pchan;
> pchan->chan.mchan = &pcc_mbox_channels[i];
>
> + pchan->is_controller =
> + (pcct_entry->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE);
> +
This definition does not apply to all types because type1 and type2
support bidirectional communication.
> if (pcct_entry->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE &&
> !pcc_mbox_ctrl->txdone_irq) {
> pr_err("Plaform Interrupt flag must be set to 1");
>
I put all points we discussed into the following modifcation.
Robbie, can you try it again for type 4 and see what else needs to be
done?
Regards,
Huisong
--> all modifcations:
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3c2bc0ca454c..320aab6cf733 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -80,6 +80,13 @@ struct pcc_chan_reg {
u64 status_mask;
};
+enum pcc_chan_mesg_dir {
+ PCC_ONLY_AP_TO_SCP,
+ PCC_ONLY_SCP_TO_AP,
+ PCC_BIDIRECTIONAL,
+ PCC_DIR_UNKNOWN,
+};
+
/**
* struct pcc_chan_info - PCC channel specific information
*
@@ -91,6 +98,10 @@ 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
when use
+ * platform interrupt
+ * @mesg_dir: direction of message transmission supported by the channel
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -100,6 +111,9 @@ 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;
+ u8 mesg_dir;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
@@ -221,6 +235,47 @@ 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_need_rsp_irq(struct pcc_chan_info *pchan,
+ u64 cmd_complete_reg_val)
+{
+ bool need_rsp;
+
+ if (!pchan->cmd_complete.gas)
+ return true;
+
+ cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
+
+ switch (pchan->mesg_dir) {
+ case PCC_ONLY_AP_TO_SCP:
+ /*
+ * For the communication from AP to SCP, if this channel
is in
+ * use, command complete bit is 1 indicates that the command
+ * being executed has been completed.
+ */
+ need_rsp = cmd_complete_reg_val != 0;
+ break;
+ case PCC_ONLY_SCP_TO_AP:
+ /*
+ * For the communication from SCP to AP， if this channel
is in
+ * use, command complete bit is 0 indicates that the bit has
+ * been cleared and AP should response the interrupt.
+ */
+ need_rsp = cmd_complete_reg_val == 0;
+ break;
+ default:
+ need_rsp = true;
+ break;
+ }
+
+ return need_rsp;
+}
+
/**
* pcc_mbox_irq - PCC mailbox interrupt handler
* @irq: interrupt number
@@ -232,37 +287,47 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
{
struct pcc_chan_info *pchan;
struct mbox_chan *chan = p;
+ static irqreturn_t rc;
u64 val;
int ret;
pchan = chan->con_priv;
+ if (pchan->mesg_dir == PCC_ONLY_AP_TO_SCP && !pchan->chan_in_use)
+ return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
if (ret)
return IRQ_NONE;
+ if (!pcc_chan_need_rsp_irq(pchan, val))
+ return IRQ_NONE;
- if (val) { /* Ensure GAS exists and value is non-zero */
- val &= pchan->cmd_complete.status_mask;
- if (!val)
- return IRQ_NONE;
+ ret = pcc_chan_reg_read(&pchan->error, &val);
+ if (ret) {
+ rc = IRQ_NONE;
+ goto out;
}
- ret = pcc_chan_reg_read(&pchan->error, &val);
- if (ret)
- return IRQ_NONE;
val &= pchan->error.status_mask;
if (val) {
val &= ~pchan->error.status_mask;
pcc_chan_reg_write(&pchan->error, val);
- return IRQ_NONE;
+ rc = IRQ_NONE;
+ goto out;
}
- if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
- return IRQ_NONE;
+ if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) {
+ rc = IRQ_NONE;
+ goto out;
+ }
mbox_chan_received_data(chan, NULL);
+ rc = IRQ_HANDLED;
- return IRQ_HANDLED;
+out:
+ if (pchan->cmd_complete.gas)
+ pchan->chan_in_use = false;
+
+ return rc;
}
/**
@@ -309,10 +374,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);
@@ -374,7 +442,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->plat_irq > 0)
+ pchan->chan_in_use = true;
+
+ return ret;
}
static const struct mbox_chan_ops pcc_chan_ops = {
@@ -457,6 +529,7 @@ static int pcc_parse_subspace_irq(struct
pcc_chan_info *pchan,
pcct_ss->platform_interrupt);
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 +551,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;
}
@@ -613,6 +692,18 @@ static int __init acpi_pcc_probe(void)
return rc;
}
+static void pcc_set_chan_mesg_dir(struct pcc_chan_info *pchan, u8 type)
+{
+ if (type <= ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)
+ pchan->mesg_dir = PCC_BIDIRECTIONAL;
+ else if (type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE)
+ pchan->mesg_dir = PCC_ONLY_AP_TO_SCP;
+ else if (type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ pchan->mesg_dir = PCC_ONLY_SCP_TO_AP;
+ else
+ pchan->mesg_dir = PCC_DIR_UNKNOWN;
+}
+
/**
* pcc_mbox_probe - Called when we find a match for the
* PCCT platform device. This is purely used to represent
@@ -680,6 +771,7 @@ static int pcc_mbox_probe(struct platform_device *pdev)
rc = -EINVAL;
goto err;
}
+ pcc_set_chan_mesg_dir(pchan, pcct_entry->type);
if (pcc_mbox_ctrl->txdone_irq) {
rc = pcc_parse_subspace_irq(pchan, pcct_entry);
> .
On 11/19/2022 2:32 AM, lihuisong (C) wrote:
>
> 在 2022/11/18 2:09, Robbie King 写道:
>> On 11/7/2022 1:24 AM, lihuisong (C) wrote:
>>>
>>> 在 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.
>>>
>>
>> Hi Huisong, apologies, I see your point now concerning multiple subspaces.
>>
>> I have started stress testing where I continuously generate both requests
>> and notifications as quickly as possible, and unfortunately found an issue
>> even with the original chan_in_use patch. I first had to modify the patch
>> to get the type 4 channel notifications to function at all, essentially
>> ignoring the chan_in_use flag for that channel. With that change, I still
>> hit my original stress issue, where the pcc_mbox_irq function did not
>> correctly ignore an interrupt for the type 3 channel.
>>
>> The issue occurs when a request from AP to SCP over the type 3 channel is
>> outstanding, and simultaneously the SCP initiates a notification over the
>> type 4 channel. Since the two channels share an interrupt, both handlers
>> are invoked.
>>
>> I've tried to draw out the state of the channel status "free" bits along
>> with the AP and SCP function calls involved.
>>
>> type 3
>> ------
>>
>> (1)pcc.c:pcc_send_data()
>> | (5) mailbox.c:mbox_chan_receive_data()
>> _______v (4)pcc.c:pcc_mbox_irq()
>> free \_________________________________________
>>
>> ^
>> type 4 ^
>> ------ ^
>> _____________________
>> free \_____________________________
>> ^ ^
>> | |
>> (2)mod_smt.c:smt_transmit() |
>> |
>> (3)mod_mhu2.c:raise_interrupt()
>>
>> The sequence of events are:
>>
>> 1) OS initiates request to SCP by clearing FREE in status and ringing SCP doorbell
>> 2) SCP initiates notification by filling shared memory and clearing FREE in status
>> 3) SCP notifies OS by ringing OS doorbell
>> 4) OS first invokes interrupt handler for type 3 channel
>>
>> At this step, the issue is that "val" from reading status (i.e. CommandCompleteCheck)
>> is zero (SCP has not responded yet) so the code below falls through and continues
>> to processes the interrupt as if the request has been acknowledged by the SCP.
>>
>> if (val) { /* Ensure GAS exists and value is non-zero */
>> val &= pchan->cmd_complete.status_mask;
>> if (!val)
>> return IRQ_NONE;
>> }
>>
>> The chan_in_use flag does not address this because the channel is indeed in use.
>>
>> 5) ACPI:PCC client kernel module is incorrectly notified that response data is
>> available
> Indeed, chan_in_use flag is invalid for type4.
Thinking about this some more, I believe there is a need for the chan_in_use flag
for the type 4 channels. If there are multiple SCP to AP channels sharing an
interrupt, and the PCC client code chooses to acknowledge the transfer from
process level (i.e. call mbox_send outside of the mbox_chan_received_data callback),
then I believe a window exists where the callback could be invoked twice for the
same SCP to AP channel. I've attached a diff.
>> I added the following fix (applied on top of Sudeep's original patch for clarity)
>> for the issue above which solved the stress test issue. I've changed the interrupt
>> handler to explicitly verify that the status value matches the mask for type 3
>> interrupts before acknowledging them. Conversely, a type 4 channel verifies that
>> the status value does *not* match the mask, since in this case we are functioning
>> as the recipient, not the initiator.
>>
>> One concern is that since this fundamentally changes handling of the channel status,
>> that existing platforms could be impacted.
> [snip]
>>
>> + /*
>> + * When we control data flow on the channel, we expect
>> + * to see the mask bit(s) set by the remote to indicate
>> + * the presence of a valid response. When we do not
>> + * control the flow (i.e. type 4) the opposite is true.
>> + */
>> + if (pchan->is_controller)
>> + cmp = pchan->cmd_complete.status_mask;
>> + else
>> + cmp = 0;
>> +
>> + val &= pchan->cmd_complete.status_mask;
>> + if (cmp != val)
>> + return IRQ_NONE;
>>
> We don't have to use the pchan->cmd_complete.status_mask as above.
>
> For the communication from AP to SCP, if this channel is in use, command
> complete bit is 1 indicates that the command being executed has been
> completed.
> For the communication from SCP to AP, if this channel is in use, command
> complete bit is 0 indicates that the bit has been cleared and OS should
> response the interrupt.
>
> So you concern should be gone if we do as following approach:
> "
> val &= pchan->cmd_complete.status_mask;
> need_rsp_irq = pchan->is_controller ? val != 0 : val == 0;
> if (!need_rsp_irq)
> return IRQ_NONE
> "
>
> This may depend on the default value of the command complete register
> for each channel(must be 1, means that the channel is free for use).
> It is ok for type3 because of chan_in_use flag, while something needs
> to do in platform or OS for type4.
>> ret = pcc_chan_reg_read(&pchan->error, &val);
>> if (ret)
>> @@ -704,6 +717,9 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>> pcc_mbox_channels[i].con_priv = pchan;
>> pchan->chan.mchan = &pcc_mbox_channels[i];
>>
>> + pchan->is_controller =
>> + (pcct_entry->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE);
>> +
> This definition does not apply to all types because type1 and type2
> support bidirectional communication.
>
>> if (pcct_entry->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE &&
>> !pcc_mbox_ctrl->txdone_irq) {
>> pr_err("Plaform Interrupt flag must be set to 1");
>>
>
> I put all points we discussed into the following modifcation.
> Robbie, can you try it again for type 4 and see what else needs to be
> done?
>
> Regards,
> Huisong
>
Thanks Huisong, I ran my current stress test scenario against your diff
with no issues (I did have to manually merge due to a tabs to spaces issue
which may be totally on my end, still investigating).
Here is the proposed change to support chan_in_use for type 4 (which I've
also successfully tested with). I think I have solved the tabs to spaces
issue for my sent messages, apologies if that's not the case.
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 057e00ee83c8..d4fcc913a9a8 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -292,7 +292,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
int ret;
pchan = chan->con_priv;
- if (pchan->mesg_dir == PCC_ONLY_AP_TO_SCP && !pchan->chan_in_use)
+ if (!pchan->chan_in_use)
return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
@@ -320,8 +320,16 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
goto out;
}
+ /*
+ * Clearing in_use before RX callback allows calls to mbox_send
+ * (which sets in_use) within the callback so SCP to AP channels
+ * can acknowledge transfer within IRQ context
+ */
+ if (pchan->cmd_complete.gas)
+ pchan->chan_in_use = false;
+
mbox_chan_received_data(chan, NULL);
- rc = IRQ_HANDLED;
+ return IRQ_HANDLED;
out:
if (pchan->cmd_complete.gas)
@@ -772,6 +780,8 @@ static int pcc_mbox_probe(struct platform_device *pdev)
goto err;
}
pcc_set_chan_mesg_dir(pchan, pcct_entry->type);
+ if (pchan->mesg_dir == PCC_ONLY_SCP_TO_AP)
+ pchan->chan_in_use = true;
if (pcc_mbox_ctrl->txdone_irq) {
rc = pcc_parse_subspace_irq(pchan, pcct_entry);
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, we need a way to identify
if the given channel is in use and expecting the interrupt.
The way and differences of identification interrupt of each types for a
given channel are as follows:
1) type0, type1 and type5: do not support shared level triggered interrupt.
2) type2: whether the interrupt belongs to a given channel is detected
based on the status field in Generic Communications Channel
Shared Memory Region during calling rx_callback in PCC client
code.
3) type3: use the command complete register and chan_in_use flag to control
4) type4: use the command complete register and need to set the
corresponding bit of salve subspace to 1 by default in platform.
Signed-off-by: Huisong Li <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
Signed-off-by: Robbie King <[email protected]>
---
-v2: don't use platform interrupt ack register to identify if the given
channel should respond interrupt.
---
drivers/mailbox/pcc.c | 130 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 116 insertions(+), 14 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3c2bc0ca454c..674e214d64d1 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -80,6 +80,13 @@ struct pcc_chan_reg {
u64 status_mask;
};
+enum pcc_chan_mesg_dir {
+ PCC_ONLY_AP_TO_SCP,
+ PCC_ONLY_SCP_TO_AP,
+ PCC_BIDIRECTIONAL,
+ PCC_DIR_UNKNOWN,
+};
+
/**
* struct pcc_chan_info - PCC channel specific information
*
@@ -91,6 +98,10 @@ 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 when use
+ * platform interrupt, and only use it for PCC_ONLY_AP_TO_SCP
+ * @mesg_dir: direction of message transmission supported by the channel
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -100,12 +111,17 @@ 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;
+ u8 mesg_dir;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
static struct pcc_chan_info *chan_info;
static int pcc_chan_count;
+static int pcc_send_data(struct mbox_chan *chan, void *data);
+
/*
* PCC can be used with perf critical drivers such as CPPC
* So it makes sense to locally cache the virtual address and
@@ -221,6 +237,47 @@ 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_need_rsp_irq(struct pcc_chan_info *pchan,
+ u64 cmd_complete_reg_val)
+{
+ bool need_rsp;
+
+ if (!pchan->cmd_complete.gas)
+ return true;
+
+ cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
+
+ switch (pchan->mesg_dir) {
+ case PCC_ONLY_AP_TO_SCP:
+ /*
+ * For the communication from AP to SCP, if this channel is in
+ * use, command complete bit is 1 indicates that the command
+ * being executed has been completed.
+ */
+ need_rsp = cmd_complete_reg_val != 0;
+ break;
+ case PCC_ONLY_SCP_TO_AP:
+ /*
+ * For the communication from SCP to AP， if this channel is in
+ * use, command complete bit is 0 indicates that the bit has
+ * been cleared and AP should response the interrupt.
+ */
+ need_rsp = cmd_complete_reg_val == 0;
+ break;
+ default:
+ need_rsp = true;
+ break;
+ }
+
+ return need_rsp;
+}
+
/**
* pcc_mbox_irq - PCC mailbox interrupt handler
* @irq: interrupt number
@@ -232,37 +289,54 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
{
struct pcc_chan_info *pchan;
struct mbox_chan *chan = p;
+ static irqreturn_t rc;
u64 val;
int ret;
pchan = chan->con_priv;
+ if (pchan->mesg_dir == PCC_ONLY_AP_TO_SCP && !pchan->chan_in_use)
+ return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
if (ret)
return IRQ_NONE;
+ if (!pcc_chan_need_rsp_irq(pchan, val))
+ return IRQ_NONE;
- if (val) { /* Ensure GAS exists and value is non-zero */
- val &= pchan->cmd_complete.status_mask;
- if (!val)
- return IRQ_NONE;
+ ret = pcc_chan_reg_read(&pchan->error, &val);
+ if (ret) {
+ rc = IRQ_NONE;
+ goto out;
}
- ret = pcc_chan_reg_read(&pchan->error, &val);
- if (ret)
- return IRQ_NONE;
val &= pchan->error.status_mask;
if (val) {
val &= ~pchan->error.status_mask;
pcc_chan_reg_write(&pchan->error, val);
- return IRQ_NONE;
+ rc = IRQ_NONE;
+ goto out;
}
- if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
- return IRQ_NONE;
+ if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) {
+ rc = IRQ_NONE;
+ goto out;
+ }
mbox_chan_received_data(chan, NULL);
+ /*
+ * For slave subspace, need to set the command complete bit and ring
+ * doorbell after processing message.
+ */
+ if (pchan->mesg_dir == PCC_ONLY_SCP_TO_AP)
+ pcc_send_data(chan, NULL);
+
+ rc = IRQ_HANDLED;
- return IRQ_HANDLED;
+out:
+ if (pchan->mesg_dir == PCC_ONLY_AP_TO_SCP)
+ pchan->chan_in_use = false;
+
+ return rc;
}
/**
@@ -309,10 +383,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);
@@ -374,7 +451,12 @@ 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->mesg_dir == PCC_ONLY_AP_TO_SCP &&
+ pchan->plat_irq > 0)
+ pchan->chan_in_use = true;
+
+ return ret;
}
static const struct mbox_chan_ops pcc_chan_ops = {
@@ -457,6 +539,7 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
pcct_ss->platform_interrupt);
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 +561,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;
}
@@ -613,6 +702,18 @@ static int __init acpi_pcc_probe(void)
return rc;
}
+static void pcc_set_chan_mesg_dir(struct pcc_chan_info *pchan, u8 type)
+{
+ if (type <= ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)
+ pchan->mesg_dir = PCC_BIDIRECTIONAL;
+ else if (type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE)
+ pchan->mesg_dir = PCC_ONLY_AP_TO_SCP;
+ else if (type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ pchan->mesg_dir = PCC_ONLY_AP_TO_SCP;
+ else
+ pchan->mesg_dir = PCC_DIR_UNKNOWN;
+}
+
/**
* pcc_mbox_probe - Called when we find a match for the
* PCCT platform device. This is purely used to represent
@@ -680,6 +781,7 @@ static int pcc_mbox_probe(struct platform_device *pdev)
rc = -EINVAL;
goto err;
}
+ pcc_set_chan_mesg_dir(pchan, pcct_entry->type);
if (pcc_mbox_ctrl->txdone_irq) {
rc = pcc_parse_subspace_irq(pchan, pcct_entry);
--
2.33.0
在 2022/11/22 0:59, Robbie King 写道:
> On 11/19/2022 2:32 AM, lihuisong (C) wrote:
>> 在 2022/11/18 2:09, Robbie King 写道:
>>> On 11/7/2022 1:24 AM, lihuisong (C) wrote:
>>>> 在 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.
>>>>
>>> Hi Huisong, apologies, I see your point now concerning multiple subspaces.
>>>
>>> I have started stress testing where I continuously generate both requests
>>> and notifications as quickly as possible, and unfortunately found an issue
>>> even with the original chan_in_use patch. I first had to modify the patch
>>> to get the type 4 channel notifications to function at all, essentially
>>> ignoring the chan_in_use flag for that channel. With that change, I still
>>> hit my original stress issue, where the pcc_mbox_irq function did not
>>> correctly ignore an interrupt for the type 3 channel.
>>>
>>> The issue occurs when a request from AP to SCP over the type 3 channel is
>>> outstanding, and simultaneously the SCP initiates a notification over the
>>> type 4 channel. Since the two channels share an interrupt, both handlers
>>> are invoked.
>>>
>>> I've tried to draw out the state of the channel status "free" bits along
>>> with the AP and SCP function calls involved.
>>>
>>> type 3
>>> ------
>>>
>>> (1)pcc.c:pcc_send_data()
>>> | (5) mailbox.c:mbox_chan_receive_data()
>>> _______v (4)pcc.c:pcc_mbox_irq()
>>> free \_________________________________________
>>>
>>> ^
>>> type 4 ^
>>> ------ ^
>>> _____________________
>>> free \_____________________________
>>> ^ ^
>>> | |
>>> (2)mod_smt.c:smt_transmit() |
>>> |
>>> (3)mod_mhu2.c:raise_interrupt()
>>>
>>> The sequence of events are:
>>>
>>> 1) OS initiates request to SCP by clearing FREE in status and ringing SCP doorbell
>>> 2) SCP initiates notification by filling shared memory and clearing FREE in status
>>> 3) SCP notifies OS by ringing OS doorbell
>>> 4) OS first invokes interrupt handler for type 3 channel
>>>
>>> At this step, the issue is that "val" from reading status (i.e. CommandCompleteCheck)
>>> is zero (SCP has not responded yet) so the code below falls through and continues
>>> to processes the interrupt as if the request has been acknowledged by the SCP.
>>>
>>> if (val) { /* Ensure GAS exists and value is non-zero */
>>> val &= pchan->cmd_complete.status_mask;
>>> if (!val)
>>> return IRQ_NONE;
>>> }
>>>
>>> The chan_in_use flag does not address this because the channel is indeed in use.
>>>
>>> 5) ACPI:PCC client kernel module is incorrectly notified that response data is
>>> available
>> Indeed, chan_in_use flag is invalid for type4.
> Thinking about this some more, I believe there is a need for the chan_in_use flag
> for the type 4 channels. If there are multiple SCP to AP channels sharing an
> interrupt, and the PCC client code chooses to acknowledge the transfer from
> process level (i.e. call mbox_send outside of the mbox_chan_received_data callback),
> then I believe a window exists where the callback could be invoked twice for the
> same SCP to AP channel. I've attached a diff.
I don't understand your concern. type4 need to set command complete bit and
ring doorbell after processing mbox_chan_received_data callback. I think it
is ok without chan_in_use flag.
Here's what I think:
For type4, set the command complete bit to 1 by default in platform.
Clear the command complete when do platform notification.
If a given channel detects the command complete bit is 0, it should respond
the interrupt, otherwise there is nothing to do.
I put all points we discussed into the RFC V2 patch. Let's go to V2 to
discuss.
>
>>> I added the following fix (applied on top of Sudeep's original patch for clarity)
>>> for the issue above which solved the stress test issue. I've changed the interrupt
>>> handler to explicitly verify that the status value matches the mask for type 3
>>> interrupts before acknowledging them. Conversely, a type 4 channel verifies that
>>> the status value does *not* match the mask, since in this case we are functioning
>>> as the recipient, not the initiator.
>>>
>>> One concern is that since this fundamentally changes handling of the channel status,
>>> that existing platforms could be impacted.
>> [snip]
>>> + /*
>>> + * When we control data flow on the channel, we expect
>>> + * to see the mask bit(s) set by the remote to indicate
>>> + * the presence of a valid response. When we do not
>>> + * control the flow (i.e. type 4) the opposite is true.
>>> + */
>>> + if (pchan->is_controller)
>>> + cmp = pchan->cmd_complete.status_mask;
>>> + else
>>> + cmp = 0;
>>> +
>>> + val &= pchan->cmd_complete.status_mask;
>>> + if (cmp != val)
>>> + return IRQ_NONE;
>>>
>> We don't have to use the pchan->cmd_complete.status_mask as above.
>>
>> For the communication from AP to SCP, if this channel is in use, command
>> complete bit is 1 indicates that the command being executed has been
>> completed.
>> For the communication from SCP to AP, if this channel is in use, command
>> complete bit is 0 indicates that the bit has been cleared and OS should
>> response the interrupt.
>>
>> So you concern should be gone if we do as following approach:
>> "
>> val &= pchan->cmd_complete.status_mask;
>> need_rsp_irq = pchan->is_controller ? val != 0 : val == 0;
>> if (!need_rsp_irq)
>> return IRQ_NONE
>> "
>>
>> This may depend on the default value of the command complete register
>> for each channel(must be 1, means that the channel is free for use).
>> It is ok for type3 because of chan_in_use flag, while something needs
>> to do in platform or OS for type4.
>>> ret = pcc_chan_reg_read(&pchan->error, &val);
>>> if (ret)
>>> @@ -704,6 +717,9 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>>> pcc_mbox_channels[i].con_priv = pchan;
>>> pchan->chan.mchan = &pcc_mbox_channels[i];
>>>
>>> + pchan->is_controller =
>>> + (pcct_entry->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE);
>>> +
>> This definition does not apply to all types because type1 and type2
>> support bidirectional communication.
>>
>>> if (pcct_entry->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE &&
>>> !pcc_mbox_ctrl->txdone_irq) {
>>> pr_err("Plaform Interrupt flag must be set to 1");
>>>
>> I put all points we discussed into the following modifcation.
>> Robbie, can you try it again for type 4 and see what else needs to be
>> done?
>>
>> Regards,
>> Huisong
>>
> Thanks Huisong, I ran my current stress test scenario against your diff
> with no issues (I did have to manually merge due to a tabs to spaces issue
> which may be totally on my end, still investigating).
>
> Here is the proposed change to support chan_in_use for type 4 (which I've
> also successfully tested with). I think I have solved the tabs to spaces
> issue for my sent messages, apologies if that's not the case.
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 057e00ee83c8..d4fcc913a9a8 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -292,7 +292,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
> int ret;
>
> pchan = chan->con_priv;
> - if (pchan->mesg_dir == PCC_ONLY_AP_TO_SCP && !pchan->chan_in_use)
> + if (!pchan->chan_in_use)
> return IRQ_NONE;
>
> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> @@ -320,8 +320,16 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
> goto out;
> }
>
> + /*
> + * Clearing in_use before RX callback allows calls to mbox_send
> + * (which sets in_use) within the callback so SCP to AP channels
> + * can acknowledge transfer within IRQ context
> + */
> + if (pchan->cmd_complete.gas)
> + pchan->chan_in_use = false;
> +
> mbox_chan_received_data(chan, NULL);
> - rc = IRQ_HANDLED;
> + return IRQ_HANDLED;
>
> out:
> if (pchan->cmd_complete.gas)
> @@ -772,6 +780,8 @@ static int pcc_mbox_probe(struct platform_device *pdev)
> goto err;
> }
> pcc_set_chan_mesg_dir(pchan, pcct_entry->type);
> + if (pchan->mesg_dir == PCC_ONLY_SCP_TO_AP)
> + pchan->chan_in_use = true;
>
> if (pcc_mbox_ctrl->txdone_irq) {
> rc = pcc_parse_subspace_irq(pchan, pcct_entry);
>
> .
On Tue, Nov 22, 2022 at 11:30:51AM +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, we need a way to identify
> if the given channel is in use and expecting the interrupt.
>
> The way and differences of identification interrupt of each types for a
> given channel are as follows:
> 1) type0, type1 and type5: do not support shared level triggered interrupt.
> 2) type2: whether the interrupt belongs to a given channel is detected
> based on the status field in Generic Communications Channel
> Shared Memory Region during calling rx_callback in PCC client
> code.
> 3) type3: use the command complete register and chan_in_use flag to control
> 4) type4: use the command complete register and need to set the
> corresponding bit of salve subspace to 1 by default in platform.
>
> Signed-off-by: Huisong Li <[email protected]>
> Signed-off-by: Sudeep Holla <[email protected]>
While I am aware that there are parts of this patch that I have suggested or
was part of the discussion, it doesn't mean you can add my sign-off without
my consent. You have introduced new things here which I haven't seen or
agreed to, so this sign-off is completely meaningless and wrong. Please
don't do that in the future.
> Signed-off-by: Robbie King <[email protected]>
> ---
> -v2: don't use platform interrupt ack register to identify if the given
> channel should respond interrupt.
>
> ---
> drivers/mailbox/pcc.c | 130 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 116 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 3c2bc0ca454c..674e214d64d1 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -80,6 +80,13 @@ struct pcc_chan_reg {
> u64 status_mask;
> };
>
> +enum pcc_chan_mesg_dir {
> + PCC_ONLY_AP_TO_SCP,
> + PCC_ONLY_SCP_TO_AP,
AP and SCP sounds very specific to your platform. The ACPI PCC spec doesn't
talk about these or use these terminology IIUC. You need to refer AP as OSPM
and SCP as platform.
> + PCC_BIDIRECTIONAL,
Again I need to check about this in the specification.
> + PCC_DIR_UNKNOWN,
> +};
> +
> /**
> * struct pcc_chan_info - PCC channel specific information
> *
> @@ -91,6 +98,10 @@ 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 when use
> + * platform interrupt, and only use it for PCC_ONLY_AP_TO_SCP
> + * @mesg_dir: direction of message transmission supported by the channel
> */
> struct pcc_chan_info {
> struct pcc_mbox_chan chan;
> @@ -100,12 +111,17 @@ 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;
> + u8 mesg_dir;
> };
>
> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
> static struct pcc_chan_info *chan_info;
> static int pcc_chan_count;
>
> +static int pcc_send_data(struct mbox_chan *chan, void *data);
> +
> /*
> * PCC can be used with perf critical drivers such as CPPC
> * So it makes sense to locally cache the virtual address and
> @@ -221,6 +237,47 @@ 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_need_rsp_irq(struct pcc_chan_info *pchan,
> + u64 cmd_complete_reg_val)
> +{
> + bool need_rsp;
> +
> + if (!pchan->cmd_complete.gas)
> + return true;
> +
> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
> +
> + switch (pchan->mesg_dir) {
> + case PCC_ONLY_AP_TO_SCP:
> + /*
> + * For the communication from AP to SCP, if this channel is in
> + * use, command complete bit is 1 indicates that the command
> + * being executed has been completed.
> + */
> + need_rsp = cmd_complete_reg_val != 0;
> + break;
> + case PCC_ONLY_SCP_TO_AP:
> + /*
> + * For the communication from SCP to AP， if this channel is in
> + * use, command complete bit is 0 indicates that the bit has
> + * been cleared and AP should response the interrupt.
> + */
> + need_rsp = cmd_complete_reg_val == 0;
> + break;
> + default:
> + need_rsp = true;
> + break;
> + }
> +
> + return need_rsp;
> +}
> +
> /**
> * pcc_mbox_irq - PCC mailbox interrupt handler
> * @irq: interrupt number
> @@ -232,37 +289,54 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
> {
> struct pcc_chan_info *pchan;
> struct mbox_chan *chan = p;
> + static irqreturn_t rc;
> u64 val;
> int ret;
>
> pchan = chan->con_priv;
> + if (pchan->mesg_dir == PCC_ONLY_AP_TO_SCP && !pchan->chan_in_use)
> + return IRQ_NONE;
>
> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> if (ret)
> return IRQ_NONE;
> + if (!pcc_chan_need_rsp_irq(pchan, val))
> + return IRQ_NONE;
>
Not sure the login in pcc_chan_need_rsp_irq works for type1/2 channels
or am I missing something.
> - if (val) { /* Ensure GAS exists and value is non-zero */
> - val &= pchan->cmd_complete.status_mask;
> - if (!val)
> - return IRQ_NONE;
> + ret = pcc_chan_reg_read(&pchan->error, &val);
> + if (ret) {
> + rc = IRQ_NONE;
> + goto out;
> }
>
> - ret = pcc_chan_reg_read(&pchan->error, &val);
> - if (ret)
> - return IRQ_NONE;
> val &= pchan->error.status_mask;
> if (val) {
> val &= ~pchan->error.status_mask;
> pcc_chan_reg_write(&pchan->error, val);
> - return IRQ_NONE;
> + rc = IRQ_NONE;
> + goto out;
> }
>
> - if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
> - return IRQ_NONE;
> + if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) {
> + rc = IRQ_NONE;
> + goto out;
> + }
>
> mbox_chan_received_data(chan, NULL);
> + /*
> + * For slave subspace, need to set the command complete bit and ring
> + * doorbell after processing message.
> + */
> + if (pchan->mesg_dir == PCC_ONLY_SCP_TO_AP)
> + pcc_send_data(chan, NULL);
> +
> + rc = IRQ_HANDLED;
>
Also I think it is better to split the support into 2 different patches.
Add type 4 channel interrupt handling support and then handle interrupt
sharing or vice-versa. I am struggling to follow this. I would also avoid
goto in a interrupt handler unless absolutely necessary.
--
Regards,
Sudeep
在 2022/11/22 21:46, Sudeep Holla 写道:
> On Tue, Nov 22, 2022 at 11:30:51AM +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, we need a way to identify
>> if the given channel is in use and expecting the interrupt.
>>
>> The way and differences of identification interrupt of each types for a
>> given channel are as follows:
>> 1) type0, type1 and type5: do not support shared level triggered interrupt.
>> 2) type2: whether the interrupt belongs to a given channel is detected
>> based on the status field in Generic Communications Channel
>> Shared Memory Region during calling rx_callback in PCC client
>> code.
>> 3) type3: use the command complete register and chan_in_use flag to control
>> 4) type4: use the command complete register and need to set the
>> corresponding bit of salve subspace to 1 by default in platform.
>>
>> Signed-off-by: Huisong Li <[email protected]>
>> Signed-off-by: Sudeep Holla <[email protected]>
> While I am aware that there are parts of this patch that I have suggested or
> was part of the discussion, it doesn't mean you can add my sign-off without
> my consent. You have introduced new things here which I haven't seen or
> agreed to, so this sign-off is completely meaningless and wrong. Please
> don't do that in the future.
Sorry, I got it. I'll remove this sign-off. Thank you for reminding.
>
>> Signed-off-by: Robbie King <[email protected]>
>> ---
>> -v2: don't use platform interrupt ack register to identify if the given
>> channel should respond interrupt.
>>
>> ---
>> drivers/mailbox/pcc.c | 130 +++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 116 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 3c2bc0ca454c..674e214d64d1 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -80,6 +80,13 @@ struct pcc_chan_reg {
>> u64 status_mask;
>> };
>>
>> +enum pcc_chan_mesg_dir {
>> + PCC_ONLY_AP_TO_SCP,
>> + PCC_ONLY_SCP_TO_AP,
> AP and SCP sounds very specific to your platform. The ACPI PCC spec doesn't
> talk about these or use these terminology IIUC. You need to refer AP as OSPM
> and SCP as platform.
ok
>
>> + PCC_BIDIRECTIONAL,
> Again I need to check about this in the specification.
>
>> + PCC_DIR_UNKNOWN,
>> +};
>> +
>> /**
>> * struct pcc_chan_info - PCC channel specific information
>> *
>> @@ -91,6 +98,10 @@ 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 when use
>> + * platform interrupt, and only use it for PCC_ONLY_AP_TO_SCP
>> + * @mesg_dir: direction of message transmission supported by the channel
>> */
>> struct pcc_chan_info {
>> struct pcc_mbox_chan chan;
>> @@ -100,12 +111,17 @@ 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;
>> + u8 mesg_dir;
>> };
>>
>> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>> static struct pcc_chan_info *chan_info;
>> static int pcc_chan_count;
>>
>> +static int pcc_send_data(struct mbox_chan *chan, void *data);
>> +
>> /*
>> * PCC can be used with perf critical drivers such as CPPC
>> * So it makes sense to locally cache the virtual address and
>> @@ -221,6 +237,47 @@ 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_need_rsp_irq(struct pcc_chan_info *pchan,
>> + u64 cmd_complete_reg_val)
>> +{
>> + bool need_rsp;
>> +
>> + if (!pchan->cmd_complete.gas)
>> + return true;
>> +
>> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
>> +
>> + switch (pchan->mesg_dir) {
>> + case PCC_ONLY_AP_TO_SCP:
>> + /*
>> + * For the communication from AP to SCP, if this channel is in
>> + * use, command complete bit is 1 indicates that the command
>> + * being executed has been completed.
>> + */
>> + need_rsp = cmd_complete_reg_val != 0;
>> + break;
>> + case PCC_ONLY_SCP_TO_AP:
>> + /*
>> + * For the communication from SCP to AP， if this channel is in
>> + * use, command complete bit is 0 indicates that the bit has
>> + * been cleared and AP should response the interrupt.
>> + */
>> + need_rsp = cmd_complete_reg_val == 0;
>> + break;
>> + default:
>> + need_rsp = true;
>> + break;
>> + }
>> +
>> + return need_rsp;
>> +}
>> +
>> /**
>> * pcc_mbox_irq - PCC mailbox interrupt handler
>> * @irq: interrupt number
>> @@ -232,37 +289,54 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>> {
>> struct pcc_chan_info *pchan;
>> struct mbox_chan *chan = p;
>> + static irqreturn_t rc;
>> u64 val;
>> int ret;
>>
>> pchan = chan->con_priv;
>> + if (pchan->mesg_dir == PCC_ONLY_AP_TO_SCP && !pchan->chan_in_use)
>> + return IRQ_NONE;
>>
>> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
>> if (ret)
>> return IRQ_NONE;
>> + if (!pcc_chan_need_rsp_irq(pchan, val))
>> + return IRQ_NONE;
>>
> Not sure the login in pcc_chan_need_rsp_irq works for type1/2 channels
> or am I missing something.
First of all, type 1 subspaces do not support a level triggered platform
interrupt
as no method is provided to clear the interrupt. Secondly, because there
is no
command complete register, pcc_chan_need_rsp_irq return true for type2
channels.
and the interrupt for type2 channels have to be identified in the
rx_callback of
PCC client. Generally, the rx_callback of PCC client based on type2
channel should
do it. I found that some drivers already do that, such as,
xgene-slimpro-hwmon and
xgene-slimpro-i2c.
>
>> - if (val) { /* Ensure GAS exists and value is non-zero */
>> - val &= pchan->cmd_complete.status_mask;
>> - if (!val)
>> - return IRQ_NONE;
>> + ret = pcc_chan_reg_read(&pchan->error, &val);
>> + if (ret) {
>> + rc = IRQ_NONE;
>> + goto out;
>> }
>>
>> - ret = pcc_chan_reg_read(&pchan->error, &val);
>> - if (ret)
>> - return IRQ_NONE;
>> val &= pchan->error.status_mask;
>> if (val) {
>> val &= ~pchan->error.status_mask;
>> pcc_chan_reg_write(&pchan->error, val);
>> - return IRQ_NONE;
>> + rc = IRQ_NONE;
>> + goto out;
>> }
>>
>> - if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
>> - return IRQ_NONE;
>> + if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack)) {
>> + rc = IRQ_NONE;
>> + goto out;
>> + }
>>
>> mbox_chan_received_data(chan, NULL);
>> + /*
>> + * For slave subspace, need to set the command complete bit and ring
>> + * doorbell after processing message.
>> + */
>> + if (pchan->mesg_dir == PCC_ONLY_SCP_TO_AP)
>> + pcc_send_data(chan, NULL);
>> +
>> + rc = IRQ_HANDLED;
>>
> Also I think it is better to split the support into 2 different patches.
> Add type 4 channel interrupt handling support and then handle interrupt
> sharing or vice-versa. I am struggling to follow this. I would also avoid
> goto in a interrupt handler unless absolutely necessary.
Yes. I will split it in next version.
I'm actually struggling with what value this interrupt handler should
return when
fail to read/write register if a channel need to respond the interrupt
and what to
do for the chan_in_use flag. What do you think?
>
PCC supports processing platform notification for slave subspaces and
shared interrupt for multiple subspaces.
---
-v3: split V2 into two patches.
-v2: don't use platform interrupt ack register to identify if the given
channel should respond interrupt.
Huisong Li (2):
mailbox: pcc: Add processing platform notification for slave subspaces
mailbox: pcc: Support shared interrupt for multiple subspaces
drivers/mailbox/pcc.c | 123 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 114 insertions(+), 9 deletions(-)
--
2.33.0
Currently, PCC driver doesn't support the processing of platform
notification for slave PCC subspaces because of the incomplete
communication flow.
According to ACPI specification, if platform sends a notification
to OSPM, it must clear the command complete bit and trigger platform
interrupt. OSPM needs to check whether the command complete bit is
cleared, clear platform interrupt, process command, and then set the
command complete and ring doorbell to Platform. But the current judgment
on the command complete is not applicable to type4 in pcc_mbox_irq().
This patch introduces a communication flow direction field to detect
whether the interrupt belongs to the master or slave subspace channel.
And PCC driver needs to add the phase of setting the command complete
and ring doorbell in pcc_mbox_irq() to complete type4 communication
flow after processing command from Platform.
Signed-off-by: Huisong Li <[email protected]>
---
drivers/mailbox/pcc.c | 77 +++++++++++++++++++++++++++++++++++++++----
1 file changed, 71 insertions(+), 6 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 105d46c9801b..ad6d0b7d50fc 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -80,6 +80,13 @@ struct pcc_chan_reg {
u64 status_mask;
};
+enum pcc_chan_comm_flow_dir_type {
+ PCC_ONLY_OSPM_TO_PLATFORM,
+ PCC_ONLY_PLATFORM_TO_OSPM,
+ PCC_BIDIRECTIONAL,
+ PCC_DIR_UNKNOWN,
+};
+
/**
* struct pcc_chan_info - PCC channel specific information
*
@@ -91,6 +98,7 @@ 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
+ * @comm_flow_dir: direction of communication flow supported by the channel
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -100,12 +108,15 @@ struct pcc_chan_info {
struct pcc_chan_reg cmd_update;
struct pcc_chan_reg error;
int plat_irq;
+ u8 comm_flow_dir;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
static struct pcc_chan_info *chan_info;
static int pcc_chan_count;
+static int pcc_send_data(struct mbox_chan *chan, void *data);
+
/*
* PCC can be used with perf critical drivers such as CPPC
* So it makes sense to locally cache the virtual address and
@@ -221,6 +232,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
return acpi_register_gsi(NULL, interrupt, trigger, polarity);
}
+static bool pcc_chan_need_rsp_irq(struct pcc_chan_info *pchan,
+ u64 cmd_complete_reg_val)
+{
+ bool need_rsp;
+
+ if (!pchan->cmd_complete.gas)
+ return true;
+
+ cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
+
+ switch (pchan->comm_flow_dir) {
+ case PCC_ONLY_OSPM_TO_PLATFORM:
+ /*
+ * For the communication flow from OSPM to Platform, if this
+ * channel is in use, command complete bit is 1 indicates that
+ * the executing command has been completed by Platform and OSPM
+ * needs to process response.
+ */
+ need_rsp = cmd_complete_reg_val != 0;
+ break;
+ case PCC_ONLY_PLATFORM_TO_OSPM:
+ /*
+ * For the communication flow from Platform to OSPM, if this
+ * channel is in use, command complete bit is 0 indicates that
+ * Platform is sending a notification and OSPM needs to response
+ * the interrupt to process this command.
+ */
+ need_rsp = cmd_complete_reg_val == 0;
+ break;
+ default:
+ need_rsp = true;
+ break;
+ }
+
+ return need_rsp;
+}
+
/**
* pcc_mbox_irq - PCC mailbox interrupt handler
* @irq: interrupt number
@@ -240,12 +288,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
if (ret)
return IRQ_NONE;
-
- if (val) { /* Ensure GAS exists and value is non-zero */
- val &= pchan->cmd_complete.status_mask;
- if (!val)
- return IRQ_NONE;
- }
+ if (!pcc_chan_need_rsp_irq(pchan, val))
+ return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->error, &val);
if (ret)
@@ -262,6 +306,14 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
mbox_chan_received_data(chan, NULL);
+ /*
+ * For communication flow from Platform to OSPM (like, slave subspace),
+ * need to set the command complete bit and ring doorbell after
+ * processing message.
+ */
+ if (pchan->comm_flow_dir == PCC_ONLY_PLATFORM_TO_OSPM)
+ pcc_send_data(chan, NULL);
+
return IRQ_HANDLED;
}
@@ -613,6 +665,18 @@ static int __init acpi_pcc_probe(void)
return rc;
}
+static void pcc_set_chan_comm_flow_dir(struct pcc_chan_info *pchan, u8 type)
+{
+ if (type <= ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)
+ pchan->comm_flow_dir = PCC_BIDIRECTIONAL;
+ else if (type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE)
+ pchan->comm_flow_dir = PCC_ONLY_OSPM_TO_PLATFORM;
+ else if (type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ pchan->comm_flow_dir = PCC_ONLY_PLATFORM_TO_OSPM;
+ else
+ pchan->comm_flow_dir = PCC_DIR_UNKNOWN;
+}
+
/**
* pcc_mbox_probe - Called when we find a match for the
* PCCT platform device. This is purely used to represent
@@ -686,6 +750,7 @@ static int pcc_mbox_probe(struct platform_device *pdev)
if (rc < 0)
goto err;
}
+ pcc_set_chan_comm_flow_dir(pchan, pcct_entry->type);
rc = pcc_parse_subspace_db_reg(pchan, pcct_entry);
if (rc < 0)
goto err;
--
2.33.0
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 <[email protected]>
---
drivers/mailbox/pcc.c | 46 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 43 insertions(+), 3 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index ad6d0b7d50fc..90e9cc324081 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -99,6 +99,10 @@ enum pcc_chan_comm_flow_dir_type {
* @error: PCC register bundle for the error status register
* @plat_irq: platform interrupt
* @comm_flow_dir: direction of communication flow supported by the channel
+ * @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.
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -109,6 +113,8 @@ struct pcc_chan_info {
struct pcc_chan_reg error;
int plat_irq;
u8 comm_flow_dir;
+ unsigned int plat_irq_flags;
+ bool chan_in_use;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
@@ -232,6 +238,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_need_rsp_irq(struct pcc_chan_info *pchan,
u64 cmd_complete_reg_val)
{
@@ -284,6 +296,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
int ret;
pchan = chan->con_priv;
+ if (pchan->comm_flow_dir == PCC_ONLY_OSPM_TO_PLATFORM &&
+ !pchan->chan_in_use)
+ return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
if (ret)
@@ -310,9 +325,14 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
* For communication flow from Platform to OSPM (like, slave subspace),
* need to set the command complete bit and ring doorbell after
* processing message.
+ *
+ * For communication flow from OSPM to Platform, clear chan_in_use flag
+ * to free this channel.
*/
if (pchan->comm_flow_dir == PCC_ONLY_PLATFORM_TO_OSPM)
pcc_send_data(chan, NULL);
+ else if (pchan->comm_flow_dir == PCC_ONLY_OSPM_TO_PLATFORM)
+ pchan->chan_in_use = false;
return IRQ_HANDLED;
}
@@ -361,10 +381,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);
@@ -426,7 +449,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 communication flow from OSPM to Platform, set chan_in_use flag
+ * of this channel to true after ring doorbell, and clear this flag
+ * when the reply message is processed.
+ */
+ if (!ret && pchan->comm_flow_dir == PCC_ONLY_OSPM_TO_PLATFORM &&
+ pchan->plat_irq > 0)
+ pchan->chan_in_use = true;
+
+ return ret;
}
static const struct mbox_chan_ops pcc_chan_ops = {
@@ -509,6 +542,7 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
pcct_ss->platform_interrupt);
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;
@@ -530,6 +564,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.33.0
Hi Sudeep,
Can you take a look? This patch series works well for type3 and type4.
/Huisong Li
在 2022/12/3 17:51, Huisong Li 写道:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
>
> ---
> -v3: split V2 into two patches.
> -v2: don't use platform interrupt ack register to identify if the given
> channel should respond interrupt.
>
> Huisong Li (2):
> mailbox: pcc: Add processing platform notification for slave subspaces
> mailbox: pcc: Support shared interrupt for multiple subspaces
>
> drivers/mailbox/pcc.c | 123 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 114 insertions(+), 9 deletions(-)
>
Kindly ping...
在 2022/12/14 10:57, lihuisong (C) 写道:
> Hi Sudeep,
>
> Can you take a look? This patch series works well for type3 and type4.
>
> /Huisong Li
>
> 在 2022/12/3 17:51, Huisong Li 写道:
>> PCC supports processing platform notification for slave subspaces and
>> shared interrupt for multiple subspaces.
>>
>> ---
>> -v3: split V2 into two patches.
>> -v2: don't use platform interrupt ack register to identify if the
>> given
>> channel should respond interrupt.
>>
>> Huisong Li (2):
>> mailbox: pcc: Add processing platform notification for slave
>> subspaces
>> mailbox: pcc: Support shared interrupt for multiple subspaces
>>
>> drivers/mailbox/pcc.c | 123 ++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 114 insertions(+), 9 deletions(-)
>>
> .
On Wed, Dec 14, 2022 at 10:57:51AM +0800, lihuisong (C) wrote:
> Hi Sudeep,
>
> Can you take a look? This patch series works well for type3 and type4.
>
Hi Huisong Li,
Sorry for the delay. I have been away and will be for some more time.
I will take a look at this ASAP. Thanks for your patience.
--
Regards,
Sudeep
在 2023/1/4 19:06, Sudeep Holla 写道:
> On Wed, Dec 14, 2022 at 10:57:51AM +0800, lihuisong (C) wrote:
>> Hi Sudeep,
>>
>> Can you take a look? This patch series works well for type3 and type4.
>>
> Hi Huisong Li,
>
> Sorry for the delay. I have been away and will be for some more time.
> I will take a look at this ASAP. Thanks for your patience.
Thank you for your advance notice. Looking forward to your comments.????
在 2023/1/5 9:14, lihuisong (C) 写道:
>
> 在 2023/1/4 19:06, Sudeep Holla 写道:
>> On Wed, Dec 14, 2022 at 10:57:51AM +0800, lihuisong (C) wrote:
>>> Hi Sudeep,
>>>
>>> Can you take a look? This patch series works well for type3 and type4.
>>>
>> Hi Huisong Li,
>>
>> Sorry for the delay. I have been away and will be for some more time.
>> I will take a look at this ASAP. Thanks for your patience.
> Thank you for your advance notice. Looking forward to your comments.????
>
> .
Hi Sudeep,
Can you take a look at this patch series?
Hi Huisong,
Apologies for such a long delay.
Also I would like to hear from Robbie King who I know is playing around
with this these days ????. At minimum if this logic works for him as well.
On Sat, Dec 03, 2022 at 05:51:49PM +0800, Huisong Li wrote:
> Currently, PCC driver doesn't support the processing of platform
> notification for slave PCC subspaces because of the incomplete
> communication flow.
>
> According to ACPI specification, if platform sends a notification
> to OSPM, it must clear the command complete bit and trigger platform
> interrupt. OSPM needs to check whether the command complete bit is
> cleared, clear platform interrupt, process command, and then set the
> command complete and ring doorbell to Platform. But the current judgment
> on the command complete is not applicable to type4 in pcc_mbox_irq().
>
> This patch introduces a communication flow direction field to detect
> whether the interrupt belongs to the master or slave subspace channel.
> And PCC driver needs to add the phase of setting the command complete
> and ring doorbell in pcc_mbox_irq() to complete type4 communication
> flow after processing command from Platform.
>
> Signed-off-by: Huisong Li <[email protected]>
> ---
> drivers/mailbox/pcc.c | 77 +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 71 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 105d46c9801b..ad6d0b7d50fc 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -80,6 +80,13 @@ struct pcc_chan_reg {
> u64 status_mask;
> };
>
> +enum pcc_chan_comm_flow_dir_type {
> + PCC_ONLY_OSPM_TO_PLATFORM,
> + PCC_ONLY_PLATFORM_TO_OSPM,
> + PCC_BIDIRECTIONAL,
> + PCC_DIR_UNKNOWN,
> +};
> +
> /**
> * struct pcc_chan_info - PCC channel specific information
> *
> @@ -91,6 +98,7 @@ 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
> + * @comm_flow_dir: direction of communication flow supported by the channel
> */
> struct pcc_chan_info {
> struct pcc_mbox_chan chan;
> @@ -100,12 +108,15 @@ struct pcc_chan_info {
> struct pcc_chan_reg cmd_update;
> struct pcc_chan_reg error;
> int plat_irq;
> + u8 comm_flow_dir;
I would rather just save the 'type' as read from the PCCT. We don't know
what future types might be and just identifying them by the direction of
flow of the data, it restricts the usage of this.
> };
>
> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
> static struct pcc_chan_info *chan_info;
> static int pcc_chan_count;
>
> +static int pcc_send_data(struct mbox_chan *chan, void *data);
> +
> /*
> * PCC can be used with perf critical drivers such as CPPC
> * So it makes sense to locally cache the virtual address and
> @@ -221,6 +232,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> }
>
> +static bool pcc_chan_need_rsp_irq(struct pcc_chan_info *pchan,
> + u64 cmd_complete_reg_val)
Probably rename this as pcc_chan_command_complete or something similar.
> +{
> + bool need_rsp;
> +
> + if (!pchan->cmd_complete.gas)
> + return true;
> +
> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
> +
> + switch (pchan->comm_flow_dir) {
Use the channel type instead here.
--
Regards,
Sudeep
在 2023/2/6 23:39, Sudeep Holla 写道:
> Hi Huisong,
>
> Apologies for such a long delay.
>
> Also I would like to hear from Robbie King who I know is playing around
> with this these days ????. At minimum if this logic works for him as well.
@Robbie King,
Do you use this patchset to test your requirements?
Any other problems? Can you tell us your result?
>
> On Sat, Dec 03, 2022 at 05:51:49PM +0800, Huisong Li wrote:
>> Currently, PCC driver doesn't support the processing of platform
>> notification for slave PCC subspaces because of the incomplete
>> communication flow.
>>
>> According to ACPI specification, if platform sends a notification
>> to OSPM, it must clear the command complete bit and trigger platform
>> interrupt. OSPM needs to check whether the command complete bit is
>> cleared, clear platform interrupt, process command, and then set the
>> command complete and ring doorbell to Platform. But the current judgment
>> on the command complete is not applicable to type4 in pcc_mbox_irq().
>>
>> This patch introduces a communication flow direction field to detect
>> whether the interrupt belongs to the master or slave subspace channel.
>> And PCC driver needs to add the phase of setting the command complete
>> and ring doorbell in pcc_mbox_irq() to complete type4 communication
>> flow after processing command from Platform.
>>
>> Signed-off-by: Huisong Li <[email protected]>
>> ---
>> drivers/mailbox/pcc.c | 77 +++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 71 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 105d46c9801b..ad6d0b7d50fc 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -80,6 +80,13 @@ struct pcc_chan_reg {
>> u64 status_mask;
>> };
>>
>> +enum pcc_chan_comm_flow_dir_type {
>> + PCC_ONLY_OSPM_TO_PLATFORM,
>> + PCC_ONLY_PLATFORM_TO_OSPM,
>> + PCC_BIDIRECTIONAL,
>> + PCC_DIR_UNKNOWN,
>> +};
>> +
>> /**
>> * struct pcc_chan_info - PCC channel specific information
>> *
>> @@ -91,6 +98,7 @@ 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
>> + * @comm_flow_dir: direction of communication flow supported by the channel
>> */
>> struct pcc_chan_info {
>> struct pcc_mbox_chan chan;
>> @@ -100,12 +108,15 @@ struct pcc_chan_info {
>> struct pcc_chan_reg cmd_update;
>> struct pcc_chan_reg error;
>> int plat_irq;
>> + u8 comm_flow_dir;
> I would rather just save the 'type' as read from the PCCT. We don't know
> what future types might be and just identifying them by the direction of
> flow of the data, it restricts the usage of this.
Ack.
>
>> };
>>
>> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>> static struct pcc_chan_info *chan_info;
>> static int pcc_chan_count;
>>
>> +static int pcc_send_data(struct mbox_chan *chan, void *data);
>> +
>> /*
>> * PCC can be used with perf critical drivers such as CPPC
>> * So it makes sense to locally cache the virtual address and
>> @@ -221,6 +232,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>> }
>>
>> +static bool pcc_chan_need_rsp_irq(struct pcc_chan_info *pchan,
>> + u64 cmd_complete_reg_val)
> Probably rename this as pcc_chan_command_complete or something similar.
Ack
>
>> +{
>> + bool need_rsp;
>> +
>> + if (!pchan->cmd_complete.gas)
>> + return true;
>> +
>> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
>> +
>> + switch (pchan->comm_flow_dir) {
> Use the channel type instead here.
Ack
On 2/6/2023 9:27 PM, lihuisong (C) wrote:
>
> 在 2023/2/6 23:39, Sudeep Holla 写道:
>> Hi Huisong,
>>
>> Apologies for such a long delay.
>>
>> Also I would like to hear from Robbie King who I know is playing around
>> with this these days ????. At minimum if this logic works for him as well.
>
> @Robbie King,
> Do you use this patchset to test your requirements?
> Any other problems? Can you tell us your result?
>
Sorry for the delay. I have verified the two patches continue to pass the
limited stress testing I have done with earlier change sets.
>>
>> On Sat, Dec 03, 2022 at 05:51:49PM +0800, Huisong Li wrote:
>>> Currently, PCC driver doesn't support the processing of platform
>>> notification for slave PCC subspaces because of the incomplete
>>> communication flow.
>>>
>>> According to ACPI specification, if platform sends a notification
>>> to OSPM, it must clear the command complete bit and trigger platform
>>> interrupt. OSPM needs to check whether the command complete bit is
>>> cleared, clear platform interrupt, process command, and then set the
>>> command complete and ring doorbell to Platform. But the current judgment
>>> on the command complete is not applicable to type4 in pcc_mbox_irq().
>>>
>>> This patch introduces a communication flow direction field to detect
>>> whether the interrupt belongs to the master or slave subspace channel.
>>> And PCC driver needs to add the phase of setting the command complete
>>> and ring doorbell in pcc_mbox_irq() to complete type4 communication
>>> flow after processing command from Platform.
>>>
>>> Signed-off-by: Huisong Li <[email protected]>
>>> ---
>>> drivers/mailbox/pcc.c | 77 +++++++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 71 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index 105d46c9801b..ad6d0b7d50fc 100644
>>> --- a/drivers/mailbox/pcc.c
>>> +++ b/drivers/mailbox/pcc.c
>>> @@ -80,6 +80,13 @@ struct pcc_chan_reg {
>>> u64 status_mask;
>>> };
>>> +enum pcc_chan_comm_flow_dir_type {
>>> + PCC_ONLY_OSPM_TO_PLATFORM,
>>> + PCC_ONLY_PLATFORM_TO_OSPM,
>>> + PCC_BIDIRECTIONAL,
>>> + PCC_DIR_UNKNOWN,
>>> +};
>>> +
>>> /**
>>> * struct pcc_chan_info - PCC channel specific information
>>> *
>>> @@ -91,6 +98,7 @@ 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
>>> + * @comm_flow_dir: direction of communication flow supported by the channel
>>> */
>>> struct pcc_chan_info {
>>> struct pcc_mbox_chan chan;
>>> @@ -100,12 +108,15 @@ struct pcc_chan_info {
>>> struct pcc_chan_reg cmd_update;
>>> struct pcc_chan_reg error;
>>> int plat_irq;
>>> + u8 comm_flow_dir;
>> I would rather just save the 'type' as read from the PCCT. We don't know
>> what future types might be and just identifying them by the direction of
>> flow of the data, it restricts the usage of this.
> Ack.
>>
>>> };
>>> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>>> static struct pcc_chan_info *chan_info;
>>> static int pcc_chan_count;
>>> +static int pcc_send_data(struct mbox_chan *chan, void *data);
>>> +
>>> /*
>>> * PCC can be used with perf critical drivers such as CPPC
>>> * So it makes sense to locally cache the virtual address and
>>> @@ -221,6 +232,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>>> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>>> }
>>> +static bool pcc_chan_need_rsp_irq(struct pcc_chan_info *pchan,
>>> + u64 cmd_complete_reg_val)
>> Probably rename this as pcc_chan_command_complete or something similar.
> Ack
>>
>>> +{
>>> + bool need_rsp;
>>> +
>>> + if (!pchan->cmd_complete.gas)
>>> + return true;
>>> +
>>> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
>>> +
>>> + switch (pchan->comm_flow_dir) {
>> Use the channel type instead here.
> Ack
在 2023/2/14 5:18, Robbie King 写道:
> On 2/6/2023 9:27 PM, lihuisong (C) wrote:
>> 在 2023/2/6 23:39, Sudeep Holla 写道:
>>> Hi Huisong,
>>>
>>> Apologies for such a long delay.
>>>
>>> Also I would like to hear from Robbie King who I know is playing around
>>> with this these days ????. At minimum if this logic works for him as well.
>> @Robbie King,
>> Do you use this patchset to test your requirements?
>> Any other problems? Can you tell us your result?
>>
> Sorry for the delay. I have verified the two patches continue to pass the
> limited stress testing I have done with earlier change sets.
Thanks Robbie King.
I will send a formal patch as soon as possible.
>
>>> On Sat, Dec 03, 2022 at 05:51:49PM +0800, Huisong Li wrote:
>>>> Currently, PCC driver doesn't support the processing of platform
>>>> notification for slave PCC subspaces because of the incomplete
>>>> communication flow.
>>>>
>>>> According to ACPI specification, if platform sends a notification
>>>> to OSPM, it must clear the command complete bit and trigger platform
>>>> interrupt. OSPM needs to check whether the command complete bit is
>>>> cleared, clear platform interrupt, process command, and then set the
>>>> command complete and ring doorbell to Platform. But the current judgment
>>>> on the command complete is not applicable to type4 in pcc_mbox_irq().
>>>>
>>>> This patch introduces a communication flow direction field to detect
>>>> whether the interrupt belongs to the master or slave subspace channel.
>>>> And PCC driver needs to add the phase of setting the command complete
>>>> and ring doorbell in pcc_mbox_irq() to complete type4 communication
>>>> flow after processing command from Platform.
>>>>
>>>> Signed-off-by: Huisong Li <[email protected]>
>>>> ---
>>>> drivers/mailbox/pcc.c | 77 +++++++++++++++++++++++++++++++++++++++----
>>>> 1 file changed, 71 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>>> index 105d46c9801b..ad6d0b7d50fc 100644
>>>> --- a/drivers/mailbox/pcc.c
>>>> +++ b/drivers/mailbox/pcc.c
>>>> @@ -80,6 +80,13 @@ struct pcc_chan_reg {
>>>> u64 status_mask;
>>>> };
>>>> +enum pcc_chan_comm_flow_dir_type {
>>>> + PCC_ONLY_OSPM_TO_PLATFORM,
>>>> + PCC_ONLY_PLATFORM_TO_OSPM,
>>>> + PCC_BIDIRECTIONAL,
>>>> + PCC_DIR_UNKNOWN,
>>>> +};
>>>> +
>>>> /**
>>>> * struct pcc_chan_info - PCC channel specific information
>>>> *
>>>> @@ -91,6 +98,7 @@ 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
>>>> + * @comm_flow_dir: direction of communication flow supported by the channel
>>>> */
>>>> struct pcc_chan_info {
>>>> struct pcc_mbox_chan chan;
>>>> @@ -100,12 +108,15 @@ struct pcc_chan_info {
>>>> struct pcc_chan_reg cmd_update;
>>>> struct pcc_chan_reg error;
>>>> int plat_irq;
>>>> + u8 comm_flow_dir;
>>> I would rather just save the 'type' as read from the PCCT. We don't know
>>> what future types might be and just identifying them by the direction of
>>> flow of the data, it restricts the usage of this.
>> Ack.
>>>> };
>>>> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>>>> static struct pcc_chan_info *chan_info;
>>>> static int pcc_chan_count;
>>>> +static int pcc_send_data(struct mbox_chan *chan, void *data);
>>>> +
>>>> /*
>>>> * PCC can be used with perf critical drivers such as CPPC
>>>> * So it makes sense to locally cache the virtual address and
>>>> @@ -221,6 +232,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>>>> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>>>> }
>>>> +static bool pcc_chan_need_rsp_irq(struct pcc_chan_info *pchan,
>>>> + u64 cmd_complete_reg_val)
>>> Probably rename this as pcc_chan_command_complete or something similar.
>> Ack
>>>> +{
>>>> + bool need_rsp;
>>>> +
>>>> + if (!pchan->cmd_complete.gas)
>>>> + return true;
>>>> +
>>>> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
>>>> +
>>>> + switch (pchan->comm_flow_dir) {
>>> Use the channel type instead here.
>> Ack
> .
PCC supports processing platform notification for slave subspaces and
shared interrupt for multiple subspaces.
---
-v1: using subspace type to replace comm_flow_dir in patch [1/2]
-rfc-v3: split V2 into two patches.
-rfc-v2: don't use platform interrupt ack register to identify if the given
channel should respond interrupt.
Huisong Li (2):
mailbox: pcc: Add processing platform notification for slave subspaces
mailbox: pcc: Support shared interrupt for multiple subspaces
drivers/mailbox/pcc.c | 102 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 93 insertions(+), 9 deletions(-)
--
2.33.0
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 <[email protected]>
---
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.
*/
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;
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;
+
+ return ret;
}
static const struct mbox_chan_ops pcc_chan_ops = {
@@ -501,6 +533,7 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
pcct_ss->platform_interrupt);
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;
@@ -522,6 +555,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.33.0
Currently, PCC driver doesn't support the processing of platform
notification for slave PCC subspaces because of the incomplete
communication flow.
According to ACPI specification, if platform sends a notification
to OSPM, it must clear the command complete bit and trigger platform
interrupt. OSPM needs to check whether the command complete bit is
cleared, clear platform interrupt, process command, and then set the
command complete and ring doorbell to Platform. But the current judgment
on the command complete is not applicable to type4 in pcc_mbox_irq().
This patch determines whether the PCC driver needs to respond to the
interrupt of the channel with the master or slave subspace based on
the command complete register. And PCC driver needs to add the phase
of setting the command complete and ring doorbell in pcc_mbox_irq()
to complete type4 communication flow after processing command from
Platform.
Signed-off-by: Huisong Li <[email protected]>
---
drivers/mailbox/pcc.c | 57 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 51 insertions(+), 6 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 105d46c9801b..ecd54f049de3 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -91,6 +91,7 @@ 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
+ * @type: PCC subspace type
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -100,12 +101,15 @@ struct pcc_chan_info {
struct pcc_chan_reg cmd_update;
struct pcc_chan_reg error;
int plat_irq;
+ u8 type;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
static struct pcc_chan_info *chan_info;
static int pcc_chan_count;
+static int pcc_send_data(struct mbox_chan *chan, void *data);
+
/*
* PCC can be used with perf critical drivers such as CPPC
* So it makes sense to locally cache the virtual address and
@@ -221,6 +225,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
return acpi_register_gsi(NULL, interrupt, trigger, polarity);
}
+static bool pcc_chan_command_complete(struct pcc_chan_info *pchan,
+ u64 cmd_complete_reg_val)
+{
+ bool complete;
+
+ if (!pchan->cmd_complete.gas)
+ return true;
+
+ cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
+
+ switch (pchan->type) {
+ case ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE:
+ /*
+ * If this channel with the PCC master subspace is in use,
+ * the command complete bit is 1 indicates that the executing
+ * command has been completed by Platform and OSPM needs to
+ * process the response.
+ */
+ complete = cmd_complete_reg_val != 0;
+ break;
+ case ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE:
+ /*
+ * If this channel with the PCC slave subspace is in use,
+ * the command complete bit is 0 indicates that Platform is
+ * sending a notification and OSPM needs to response the
+ * interrupt to process this command.
+ */
+ complete = cmd_complete_reg_val == 0;
+ break;
+ default:
+ complete = true;
+ break;
+ }
+
+ return complete;
+}
+
/**
* pcc_mbox_irq - PCC mailbox interrupt handler
* @irq: interrupt number
@@ -240,12 +281,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
if (ret)
return IRQ_NONE;
-
- if (val) { /* Ensure GAS exists and value is non-zero */
- val &= pchan->cmd_complete.status_mask;
- if (!val)
- return IRQ_NONE;
- }
+ if (!pcc_chan_command_complete(pchan, val))
+ return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->error, &val);
if (ret)
@@ -262,6 +299,13 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
mbox_chan_received_data(chan, NULL);
+ /*
+ * The PCC slave subspace channel needs to set the command complete bit
+ * and ring doorbell after processing message.
+ */
+ if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ pcc_send_data(chan, NULL);
+
return IRQ_HANDLED;
}
@@ -692,6 +736,7 @@ static int pcc_mbox_probe(struct platform_device *pdev)
pcc_parse_subspace_shmem(pchan, pcct_entry);
+ pchan->type = pcct_entry->type;
pcct_entry = (struct acpi_subtable_header *)
((unsigned long) pcct_entry + pcct_entry->length);
}
--
2.33.0
Kindly ping.
在 2023/2/16 14:36, Huisong Li 写道:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
>
> ---
> -v1: using subspace type to replace comm_flow_dir in patch [1/2]
> -rfc-v3: split V2 into two patches.
> -rfc-v2: don't use platform interrupt ack register to identify if the given
> channel should respond interrupt.
>
> Huisong Li (2):
> mailbox: pcc: Add processing platform notification for slave subspaces
> mailbox: pcc: Support shared interrupt for multiple subspaces
>
> drivers/mailbox/pcc.c | 102 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 93 insertions(+), 9 deletions(-)
>
On Thu, Feb 16, 2023 at 02:36:52PM +0800, Huisong Li wrote:
> Currently, PCC driver doesn't support the processing of platform
> notification for slave PCC subspaces because of the incomplete
> communication flow.
>
> According to ACPI specification, if platform sends a notification
> to OSPM, it must clear the command complete bit and trigger platform
> interrupt. OSPM needs to check whether the command complete bit is
> cleared, clear platform interrupt, process command, and then set the
> command complete and ring doorbell to Platform. But the current judgment
> on the command complete is not applicable to type4 in pcc_mbox_irq().
>
> This patch determines whether the PCC driver needs to respond to the
> interrupt of the channel with the master or slave subspace based on
> the command complete register. And PCC driver needs to add the phase
> of setting the command complete and ring doorbell in pcc_mbox_irq()
> to complete type4 communication flow after processing command from
> Platform.
>
I would prefer to reword this a little bit:
"
mailbox: pcc: Add support for platform notification handling
Currently, PCC driver doesn't support the processing of platform
notification for type 4 PCC subspaces.
According to ACPI specification, if platform sends a notification
to OSPM, it must clear the command complete bit and trigger platform
interrupt. OSPM needs to check whether the command complete bit is
cleared, clear platform interrupt, process command, and then set the
command complete and ring doorbell to the Platform.
Let us stash the value of the pcc type and use the same while processing
the interrupt of the channel. We also need to set the command complete
bit and ring doorbell in the interrupt handler for the type 4 channel to
complete the communication flow after processing the notification from
the Platform.
"
> Signed-off-by: Huisong Li <[email protected]>
> ---
> drivers/mailbox/pcc.c | 57 ++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 105d46c9801b..ecd54f049de3 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -91,6 +91,7 @@ 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
> + * @type: PCC subspace type
> */
> struct pcc_chan_info {
> struct pcc_mbox_chan chan;
> @@ -100,12 +101,15 @@ struct pcc_chan_info {
> struct pcc_chan_reg cmd_update;
> struct pcc_chan_reg error;
> int plat_irq;
> + u8 type;
> };
>
> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
> static struct pcc_chan_info *chan_info;
> static int pcc_chan_count;
>
> +static int pcc_send_data(struct mbox_chan *chan, void *data);
> +
> /*
> * PCC can be used with perf critical drivers such as CPPC
> * So it makes sense to locally cache the virtual address and
> @@ -221,6 +225,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> }
>
> +static bool pcc_chan_command_complete(struct pcc_chan_info *pchan,
> + u64 cmd_complete_reg_val)
> +{
> + bool complete;
> +
> + if (!pchan->cmd_complete.gas)
> + return true;
> +
> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
> +
> + switch (pchan->type) {
> + case ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE:
> + /*
> + * If this channel with the PCC master subspace is in use,
> + * the command complete bit is 1 indicates that the executing
> + * command has been completed by Platform and OSPM needs to
> + * process the response.
> + */
> + complete = cmd_complete_reg_val != 0;
> + break;
> + case ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE:
> + /*
> + * If this channel with the PCC slave subspace is in use,
> + * the command complete bit is 0 indicates that Platform is
> + * sending a notification and OSPM needs to response the
> + * interrupt to process this command.
> + */
> + complete = cmd_complete_reg_val == 0;
> + break;
> + default:
> + complete = true;
> + break;
> + }
> +
> + return complete;
> +}
> +
> /**
> * pcc_mbox_irq - PCC mailbox interrupt handler
> * @irq: interrupt number
> @@ -240,12 +281,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> if (ret)
> return IRQ_NONE;
> -
> - if (val) { /* Ensure GAS exists and value is non-zero */
> - val &= pchan->cmd_complete.status_mask;
> - if (!val)
> - return IRQ_NONE;
> - }
> + if (!pcc_chan_command_complete(pchan, val))
> + return IRQ_NONE;
>
Can we simplify the above 2 chunks like below ? Does that work for you ?
I see we already check for presence of complete complete check and update
registers for type3/4, we can avoid it I think. Let me know if you spot
any issue with this.
Regards,
Sudeep
--->8
@@ -221,6 +225,28 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
return acpi_register_gsi(NULL, interrupt, trigger, polarity);
}
+static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
+{
+ u64 val;
+ int ret;
+
+ ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
+ if (ret)
+ return false;
+
+ val &= pchan->cmd_complete.status_mask;
+
+ /*
+ * If this is PCC slave subspace channel, then the command complete
+ * bit 0 indicates that Platform is sending a notification and OSPM
+ * needs to respond this interrupt to process this command.
+ */
+ if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ return !val;
+ else
+ return !!val;
+}
+
/**
* pcc_mbox_irq - PCC mailbox interrupt handler
* @irq: interrupt number
@@ -237,16 +263,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
pchan = chan->con_priv;
- ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
- if (ret)
+ if (!pcc_mbox_cmd_complete_check(pchan))
return IRQ_NONE;
- if (val) { /* Ensure GAS exists and value is non-zero */
- val &= pchan->cmd_complete.status_mask;
- if (!val)
- return IRQ_NONE;
- }
-
ret = pcc_chan_reg_read(&pchan->error, &val);
if (ret)
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 <[email protected]>
> ---
> 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
在 2023/3/1 21:24, Sudeep Holla 写道:
> On Thu, Feb 16, 2023 at 02:36:52PM +0800, Huisong Li wrote:
>> Currently, PCC driver doesn't support the processing of platform
>> notification for slave PCC subspaces because of the incomplete
>> communication flow.
>>
>> According to ACPI specification, if platform sends a notification
>> to OSPM, it must clear the command complete bit and trigger platform
>> interrupt. OSPM needs to check whether the command complete bit is
>> cleared, clear platform interrupt, process command, and then set the
>> command complete and ring doorbell to Platform. But the current judgment
>> on the command complete is not applicable to type4 in pcc_mbox_irq().
>>
>> This patch determines whether the PCC driver needs to respond to the
>> interrupt of the channel with the master or slave subspace based on
>> the command complete register. And PCC driver needs to add the phase
>> of setting the command complete and ring doorbell in pcc_mbox_irq()
>> to complete type4 communication flow after processing command from
>> Platform.
>>
> I would prefer to reword this a little bit:
>
> "
> mailbox: pcc: Add support for platform notification handling
>
> Currently, PCC driver doesn't support the processing of platform
> notification for type 4 PCC subspaces.
>
> According to ACPI specification, if platform sends a notification
> to OSPM, it must clear the command complete bit and trigger platform
> interrupt. OSPM needs to check whether the command complete bit is
> cleared, clear platform interrupt, process command, and then set the
> command complete and ring doorbell to the Platform.
>
> Let us stash the value of the pcc type and use the same while processing
> the interrupt of the channel. We also need to set the command complete
> bit and ring doorbell in the interrupt handler for the type 4 channel to
> complete the communication flow after processing the notification from
> the Platform.
> "
Thanks for your rewording.
>> Signed-off-by: Huisong Li <[email protected]>
>> ---
>> drivers/mailbox/pcc.c | 57 ++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 51 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 105d46c9801b..ecd54f049de3 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -91,6 +91,7 @@ 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
>> + * @type: PCC subspace type
>> */
>> struct pcc_chan_info {
>> struct pcc_mbox_chan chan;
>> @@ -100,12 +101,15 @@ struct pcc_chan_info {
>> struct pcc_chan_reg cmd_update;
>> struct pcc_chan_reg error;
>> int plat_irq;
>> + u8 type;
>> };
>>
>> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>> static struct pcc_chan_info *chan_info;
>> static int pcc_chan_count;
>>
>> +static int pcc_send_data(struct mbox_chan *chan, void *data);
>> +
>> /*
>> * PCC can be used with perf critical drivers such as CPPC
>> * So it makes sense to locally cache the virtual address and
>> @@ -221,6 +225,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>> }
>>
>> +static bool pcc_chan_command_complete(struct pcc_chan_info *pchan,
>> + u64 cmd_complete_reg_val)
>> +{
>> + bool complete;
>> +
>> + if (!pchan->cmd_complete.gas)
>> + return true;
>> +
>> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
>> +
>> + switch (pchan->type) {
>> + case ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE:
>> + /*
>> + * If this channel with the PCC master subspace is in use,
>> + * the command complete bit is 1 indicates that the executing
>> + * command has been completed by Platform and OSPM needs to
>> + * process the response.
>> + */
>> + complete = cmd_complete_reg_val != 0;
>> + break;
>> + case ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE:
>> + /*
>> + * If this channel with the PCC slave subspace is in use,
>> + * the command complete bit is 0 indicates that Platform is
>> + * sending a notification and OSPM needs to response the
>> + * interrupt to process this command.
>> + */
>> + complete = cmd_complete_reg_val == 0;
>> + break;
>> + default:
>> + complete = true;
>> + break;
>> + }
>> +
>> + return complete;
>> +}
>> +
>> /**
>> * pcc_mbox_irq - PCC mailbox interrupt handler
>> * @irq: interrupt number
>> @@ -240,12 +281,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
>> if (ret)
>> return IRQ_NONE;
>> -
>> - if (val) { /* Ensure GAS exists and value is non-zero */
>> - val &= pchan->cmd_complete.status_mask;
>> - if (!val)
>> - return IRQ_NONE;
>> - }
>> + if (!pcc_chan_command_complete(pchan, val))
>> + return IRQ_NONE;
>>
> Can we simplify the above 2 chunks like below ? Does that work for you ?
> I see we already check for presence of complete complete check and update
> registers for type3/4, we can avoid it I think. Let me know if you spot
> any issue with this.
Agree using a function to read cmd_complete and check it. But it has a
probelm, details as below.
>
> Regards,
> Sudeep
>
> --->8
>
> @@ -221,6 +225,28 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> }
>
> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
> +{
> + u64 val;
> + int ret;
> +
> + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> + if (ret)
> + return false;
> +
we indeed already check if cmd_complete register is exist.
IMO, it can simply the code logic and reduce the risk of problems if we return true here for the type without this register.
what do you think?
> + val &= pchan->cmd_complete.status_mask;
> +
> + /*
> + * If this is PCC slave subspace channel, then the command complete
> + * bit 0 indicates that Platform is sending a notification and OSPM
> + * needs to respond this interrupt to process this command.
> + */
> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> + return !val;
> + else
> + return !!val;
This else branch is not applicable to type 3. type 3 will cannot respond
interrupt.
> +}
> +
> /**
> * pcc_mbox_irq - PCC mailbox interrupt handler
> * @irq: interrupt number
> @@ -237,16 +263,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>
> pchan = chan->con_priv;
>
> - ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> - if (ret)
> + if (!pcc_mbox_cmd_complete_check(pchan))
> return IRQ_NONE;
>
> - if (val) { /* Ensure GAS exists and value is non-zero */
> - val &= pchan->cmd_complete.status_mask;
> - if (!val)
> - return IRQ_NONE;
> - }
> -
> ret = pcc_chan_reg_read(&pchan->error, &val);
> if (ret)
>
>
> .
在 2023/3/1 21:36, Sudeep Holla 写道:
> 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 <[email protected]>
>> ---
>> 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.
Add a per-channel lock. Is this your mean?
>
>> */
>> 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)
But type2 do not need this flag.
For types no need this flag, it is always hard to understand and
redundant design.
If no this condition, we don't know what is the impact on the furture
types.
>
>> 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?)
Above is my concern.
>
On Thu, Mar 02, 2023 at 09:57:35AM +0800, lihuisong (C) wrote:
>
> 在 2023/3/1 21:24, Sudeep Holla 写道:
[...]
> >
> > +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
> > +{
> > + u64 val;
> > + int ret;
> > +
> > + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> > + if (ret)
> > + return false;
> > +
>
> we indeed already check if cmd_complete register is exist.
> IMO, it can simply the code logic and reduce the risk of problems if we
> return true here for the type without this register.
> what do you think?
>
IIUC, your concern is about returning true for type 4 when the register
doesn't exist, right ?
I am saying it won't happen as we bail out if there is no GAS register
from pcc_chan_reg_init(). Or am I missing something here ?
> > + val &= pchan->cmd_complete.status_mask;
> > +
> > + /*
> > + * If this is PCC slave subspace channel, then the command complete
> > + * bit 0 indicates that Platform is sending a notification and OSPM
> > + * needs to respond this interrupt to process this command.
> > + */
> > + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> > + return !val;
> > + else
> > + return !!val;
> This else branch is not applicable to type 3. type 3 will cannot respond
> interrupt.
Sorry I don't understand what you mean by that.
--
Regards,
Sudeep
On Thu, Mar 02, 2023 at 10:17:07AM +0800, lihuisong (C) wrote:
>
> 在 2023/3/1 21:36, Sudeep Holla 写道:
> > 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 <[email protected]>
> > > ---
> > > 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.
> Add a per-channel lock. Is this your mean?
No. I meant a comment saying it is not need since only one transfer can occur
at a time and mailbox takes care of locking. So chan_in_use can be accessed
without a lock.
> >
> > > */
> > > 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)
> But type2 do not need this flag.
Yes
> For types no need this flag, it is always hard to understand and redundant
> design.
But does it matter ? You can even support shared interrupt for type 1&2.
They support level interrupt, so we can add them too. I understand you can
test only type 3, but this driver caters for all and the code must be generic
as much as possible. I don't see any point in check for type 3 only. Only
type 4 is slave and operates quite opposite compared to other types and makes
sense to handle it differently.
> If no this condition, we don't know what is the impact on the furture types.
We can add/extend the check if necessary while adding the support for that
in the future.
--
Regards,
Sudeep
在 2023/3/2 21:52, Sudeep Holla 写道:
> On Thu, Mar 02, 2023 at 09:57:35AM +0800, lihuisong (C) wrote:
>> 在 2023/3/1 21:24, Sudeep Holla 写道:
> [...]
>
>>> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
>>> +{
>>> + u64 val;
>>> + int ret;
>>> +
>>> + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
>>> + if (ret)
>>> + return false;
>>> +
>> we indeed already check if cmd_complete register is exist.
>> IMO, it can simply the code logic and reduce the risk of problems if we
>> return true here for the type without this register.
>> what do you think?
>>
> IIUC, your concern is about returning true for type 4 when the register
> doesn't exist, right ?
Return true in advance for the type without the cmd_complete register.
If support the register, we judge if the channel should respond the
interrupt based on the value of cmd_complete, like bellow.
-->8
+static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
+{
+ u64 val;
+ int ret;
+
+ ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
+ if (ret)
+ return false;
+
+ if (!pchan->cmd_complete.gas)
+ return true;
+
+ /*
+ * Judge if the channel respond the interrupt based on the value of
+ * command complete.
+ */
+ val &= pchan->cmd_complete.status_mask;
+ /*
+ * If this is PCC slave subspace channel, then the command complete
+ * bit 0 indicates that Platform is sending a notification and OSPM
+ * needs to respond this interrupt to process this command.
+ */
+ if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ return !val;
+ else
+ return !!val;
+}
> I am saying it won't happen as we bail out if there is no GAS register
> from pcc_chan_reg_init(). Or am I missing something here ?
Yes, what you say is also ok. Just wondering if it is better to simply
the logic.
>>> + val &= pchan->cmd_complete.status_mask;
>>> +
>>> + /*
>>> + * If this is PCC slave subspace channel, then the command complete
>>> + * bit 0 indicates that Platform is sending a notification and OSPM
>>> + * needs to respond this interrupt to process this command.
>>> + */
>>> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
>>> + return !val;
>>> + else
>>> + return !!val;
>> This else branch is not applicable to type 3. type 3 will cannot respond
>> interrupt.
> Sorry I don't understand what you mean by that.
Sorry for my mistake.
I meant that the type2 channel always return false in this function and
never respond the interrupt if no check for the GAS register.
Because the 'val' for the type without the register is zero.
>
On Fri, Mar 03, 2023 at 09:50:00AM +0800, lihuisong (C) wrote:
>
> 在 2023/3/2 21:52, Sudeep Holla 写道:
> > On Thu, Mar 02, 2023 at 09:57:35AM +0800, lihuisong (C) wrote:
> > > 在 2023/3/1 21:24, Sudeep Holla 写道:
> > [...]
> >
> > > > +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
> > > > +{
> > > > + u64 val;
> > > > + int ret;
> > > > +
> > > > + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> > > > + if (ret)
> > > > + return false;
> > > > +
> > > we indeed already check if cmd_complete register is exist.
> > > IMO, it can simply the code logic and reduce the risk of problems if we
> > > return true here for the type without this register.
> > > what do you think?
> > >
> > IIUC, your concern is about returning true for type 4 when the register
> > doesn't exist, right ?
> Return true in advance for the type without the cmd_complete register.
> If support the register, we judge if the channel should respond the
> interrupt based on the value of cmd_complete, like bellow.
Right, sorry for missing that.
>
> -->8
> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
> +{
> + u64 val;
> + int ret;
> +
> + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> + if (ret)
> + return false;
> +
> + if (!pchan->cmd_complete.gas)
> + return true;
> +
Yes we need the above check.
> + /*
> + * Judge if the channel respond the interrupt based on the value of
> + * command complete.
> + */
> + val &= pchan->cmd_complete.status_mask;
> + /*
> + * If this is PCC slave subspace channel, then the command complete
> + * bit 0 indicates that Platform is sending a notification and OSPM
> + * needs to respond this interrupt to process this command.
> + */
> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> + return !val;
> + else
> + return !!val;
> +}
> > I am saying it won't happen as we bail out if there is no GAS register
> > from pcc_chan_reg_init(). Or am I missing something here ?
> Yes, what you say is also ok. Just wondering if it is better to simply the
> logic.
Understood now.
> > > > + val &= pchan->cmd_complete.status_mask;
> > > > +
> > > > + /*
> > > > + * If this is PCC slave subspace channel, then the command complete
> > > > + * bit 0 indicates that Platform is sending a notification and OSPM
> > > > + * needs to respond this interrupt to process this command.
> > > > + */
> > > > + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> > > > + return !val;
> > > > + else
> > > > + return !!val;
> > > This else branch is not applicable to type 3. type 3 will cannot respond
> > > interrupt.
> > Sorry I don't understand what you mean by that.
> Sorry for my mistake.
> I meant that the type2 channel always return false in this function and
> never respond the interrupt if no check for the GAS register.
> Because the 'val' for the type without the register is zero.
> >
Agreed as mentioned above, we need to bail out with true return if no GAS is
found.
--
Regards,
Sudeep
On Fri, Mar 03, 2023 at 02:33:49PM +0800, lihuisong (C) wrote:
> Sorry for my resend. Because I found that my last reply email is not in the
> thread of this patch. I guess it may be send failed.
>
> 在 2023/3/2 22:02, Sudeep Holla 写道:
> > No. I meant a comment saying it is not need since only one transfer can occur
> > at a time and mailbox takes care of locking. So chan_in_use can be accessed
> > without a lock.
> Got it. Agreed.
Thanks
> > > For types no need this flag, it is always hard to understand and redundant
> > > design.
> > But does it matter ? You can even support shared interrupt for type 1&2.
> BTW, type 1 subspaces do not support a level triggered platform interrupt as
> no method is provided to clear the interrupt.
Agreed but there is no harm using the flag, you can add a comment that it is
useful only if shared interrupts are supported. That will imply it is dummy
for type 1. I am avoiding too many type unnecessary checks especially in IRQ
handler.
> > They support level interrupt, so we can add them too. I understand you can
> > test only type 3, but this driver caters for all and the code must be generic
> > as much as possible. I don't see any point in check for type 3 only. Only
> I understand what you do.
> But type 2 also supports the communication flow from OSPM to Platfrom.
> In this case, this flag will get in the way of type 2.
>
How ?
> Whether the interrupt belongs to a type2 channel is only determined by
> the status field in Generic Communications Channel Shared Memory Region,
> which is done in rx_callback of PCC client.
Agreed, but do you see any issue using the flag even if it acts as dummy ?
--
Regards,
Sudeep
在 2023/3/3 19:14, Sudeep Holla 写道:
> On Fri, Mar 03, 2023 at 02:33:49PM +0800, lihuisong (C) wrote:
>> Sorry for my resend. Because I found that my last reply email is not in the
>> thread of this patch. I guess it may be send failed.
>>
>> 在 2023/3/2 22:02, Sudeep Holla 写道:
>>> No. I meant a comment saying it is not need since only one transfer can occur
>>> at a time and mailbox takes care of locking. So chan_in_use can be accessed
>>> without a lock.
>> Got it. Agreed.
> Thanks
already modify this comment as below.
>
>>>> For types no need this flag, it is always hard to understand and redundant
>>>> design.
>>> But does it matter ? You can even support shared interrupt for type 1&2.
>> BTW, type 1 subspaces do not support a level triggered platform interrupt as
>> no method is provided to clear the interrupt.
> Agreed but there is no harm using the flag, you can add a comment that it is
> useful only if shared interrupts are supported. That will imply it is dummy
> for type 1. I am avoiding too many type unnecessary checks especially in IRQ
> handler.
Understood.
>
>>> They support level interrupt, so we can add them too. I understand you can
>>> test only type 3, but this driver caters for all and the code must be generic
>>> as much as possible. I don't see any point in check for type 3 only. Only
>> I understand what you do.
>> But type 2 also supports the communication flow from OSPM to Platfrom.
>> In this case, this flag will get in the way of type 2.
>>
> How ?
It should be ok if all types except for type 3 do not check this flag in
interrupt handle.
Namely, these types consider it as dummy, and do not use it, anywhere,
Right?
>
>> Whether the interrupt belongs to a type2 channel is only determined by
>> the status field in Generic Communications Channel Shared Memory Region,
>> which is done in rx_callback of PCC client.
> Agreed, but do you see any issue using the flag even if it acts as dummy ?
I think it can work well if these types completely ignore this flag, like below.
what do you think?
-->8
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index ecd54f049de3..14405e99193d 100755
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -92,6 +92,13 @@ 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: this flag is used just to check if the interrupt needs
+ * handling when it is shared. Since only one transfer can
occur
+ * at a time and mailbox takes care of locking, this flag
can be
+ * accessed without a lock. Note: the type only support the
+ * communication from OSPM to Platform, like type3, use it, and
+ * other types completely ignore it.
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -102,6 +109,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 +234,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 +292,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 +320,12 @@ 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);
+ pchan->chan_in_use = false;
return IRQ_HANDLED;
}
@@ -353,10 +374,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 +442,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->plat_irq > 0)
+ pchan->chan_in_use = true;
+
+ return ret;
}
static const struct mbox_chan_ops pcc_chan_ops = {
@@ -501,6 +529,7 @@ static int pcc_parse_subspace_irq(struct
pcc_chan_info *pchan,
pcct_ss->platform_interrupt);
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;
@@ -522,6 +551,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;
}
在 2023/3/3 19:07, Sudeep Holla 写道:
> On Fri, Mar 03, 2023 at 09:50:00AM +0800, lihuisong (C) wrote:
>> 在 2023/3/2 21:52, Sudeep Holla 写道:
>>> On Thu, Mar 02, 2023 at 09:57:35AM +0800, lihuisong (C) wrote:
>>>> 在 2023/3/1 21:24, Sudeep Holla 写道:
>>> [...]
>>>
>>>>> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
>>>>> +{
>>>>> + u64 val;
>>>>> + int ret;
>>>>> +
>>>>> + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
>>>>> + if (ret)
>>>>> + return false;
>>>>> +
>>>> we indeed already check if cmd_complete register is exist.
>>>> IMO, it can simply the code logic and reduce the risk of problems if we
>>>> return true here for the type without this register.
>>>> what do you think?
>>>>
>>> IIUC, your concern is about returning true for type 4 when the register
>>> doesn't exist, right ?
>> Return true in advance for the type without the cmd_complete register.
>> If support the register, we judge if the channel should respond the
>> interrupt based on the value of cmd_complete, like bellow.
> Right, sorry for missing that.
>
>> -->8
>> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
>> +{
>> + u64 val;
>> + int ret;
>> +
>> + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
>> + if (ret)
>> + return false;
>> +
>> + if (!pchan->cmd_complete.gas)
>> + return true;
>> +
> Yes we need the above check.
>
>> + /*
>> + * Judge if the channel respond the interrupt based on the value of
>> + * command complete.
>> + */
>> + val &= pchan->cmd_complete.status_mask;
>> + /*
>> + * If this is PCC slave subspace channel, then the command complete
>> + * bit 0 indicates that Platform is sending a notification and OSPM
>> + * needs to respond this interrupt to process this command.
>> + */
>> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
>> + return !val;
>> + else
>> + return !!val;
>> +}
>>> I am saying it won't happen as we bail out if there is no GAS register
>>> from pcc_chan_reg_init(). Or am I missing something here ?
>> Yes, what you say is also ok. Just wondering if it is better to simply the
>> logic.
> Understood now.
>
>>>>> + val &= pchan->cmd_complete.status_mask;
>>>>> +
>>>>> + /*
>>>>> + * If this is PCC slave subspace channel, then the command complete
>>>>> + * bit 0 indicates that Platform is sending a notification and OSPM
>>>>> + * needs to respond this interrupt to process this command.
>>>>> + */
>>>>> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
>>>>> + return !val;
>>>>> + else
>>>>> + return !!val;
>>>> This else branch is not applicable to type 3. type 3 will cannot respond
>>>> interrupt.
>>> Sorry I don't understand what you mean by that.
>> Sorry for my mistake.
>> I meant that the type2 channel always return false in this function and
>> never respond the interrupt if no check for the GAS register.
>> Because the 'val' for the type without the register is zero.
> Agreed as mentioned above, we need to bail out with true return if no GAS is
> found.
>
Ok, I will fix it as mentioned above.
>
> .
On Sat, Mar 04, 2023 at 05:47:28PM +0800, lihuisong (C) wrote:
>
> 在 2023/3/3 19:14, Sudeep Holla 写道:
> > On Fri, Mar 03, 2023 at 02:33:49PM +0800, lihuisong (C) wrote:
> > > Sorry for my resend. Because I found that my last reply email is not in the
> > > thread of this patch. I guess it may be send failed.
> > >
> > > 在 2023/3/2 22:02, Sudeep Holla 写道:
> > > > No. I meant a comment saying it is not need since only one transfer can occur
> > > > at a time and mailbox takes care of locking. So chan_in_use can be accessed
> > > > without a lock.
> > > Got it. Agreed.
> > Thanks
> already modify this comment as below.
> >
> > > > > For types no need this flag, it is always hard to understand and redundant
> > > > > design.
> > > > But does it matter ? You can even support shared interrupt for type 1&2.
> > > BTW, type 1 subspaces do not support a level triggered platform interrupt as
> > > no method is provided to clear the interrupt.
> > Agreed but there is no harm using the flag, you can add a comment that it is
> > useful only if shared interrupts are supported. That will imply it is dummy
> > for type 1. I am avoiding too many type unnecessary checks especially in IRQ
> > handler.
>
> Understood.
>
> >
> > > > They support level interrupt, so we can add them too. I understand you can
> > > > test only type 3, but this driver caters for all and the code must be generic
> > > > as much as possible. I don't see any point in check for type 3 only. Only
> > > I understand what you do.
> > > But type 2 also supports the communication flow from OSPM to Platfrom.
> > > In this case, this flag will get in the way of type 2.
> > >
> > How ?
> It should be ok if all types except for type 3 do not check this flag in
> interrupt handle.
> Namely, these types consider it as dummy, and do not use it, anywhere,
> Right?
> >
> > > Whether the interrupt belongs to a type2 channel is only determined by
> > > the status field in Generic Communications Channel Shared Memory Region,
> > > which is done in rx_callback of PCC client.
> > Agreed, but do you see any issue using the flag even if it acts as dummy ?
>
> I think it can work well if these types completely ignore this flag, like below.
> what do you think?
>
> -->8
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index ecd54f049de3..14405e99193d 100755
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -92,6 +92,13 @@ 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: this flag is used just to check if the interrupt needs
> + * handling when it is shared. Since only one transfer can
> occur
> + * at a time and mailbox takes care of locking, this flag can
> be
> + * accessed without a lock. Note: the type only support the
> + * communication from OSPM to Platform, like type3, use it, and
> + * other types completely ignore it.
> */
> struct pcc_chan_info {
> struct pcc_mbox_chan chan;
> @@ -102,6 +109,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 +234,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 +292,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)
I would have avoided the type check above but I understand your concern
so let us keep it like this for now.
Please submit non-RFC patch as some maintainers may not look at RFC.
--
Regards,
Sudeep
在 2023/3/11 4:14, Sudeep Holla 写道:
> On Sat, Mar 04, 2023 at 05:47:28PM +0800, lihuisong (C) wrote:
>> 在 2023/3/3 19:14, Sudeep Holla 写道:
>>> On Fri, Mar 03, 2023 at 02:33:49PM +0800, lihuisong (C) wrote:
>>>> Sorry for my resend. Because I found that my last reply email is not in the
>>>> thread of this patch. I guess it may be send failed.
>>>>
>>>> 在 2023/3/2 22:02, Sudeep Holla 写道:
>>>>> No. I meant a comment saying it is not need since only one transfer can occur
>>>>> at a time and mailbox takes care of locking. So chan_in_use can be accessed
>>>>> without a lock.
>>>> Got it. Agreed.
>>> Thanks
>> already modify this comment as below.
>>>>>> For types no need this flag, it is always hard to understand and redundant
>>>>>> design.
>>>>> But does it matter ? You can even support shared interrupt for type 1&2.
>>>> BTW, type 1 subspaces do not support a level triggered platform interrupt as
>>>> no method is provided to clear the interrupt.
>>> Agreed but there is no harm using the flag, you can add a comment that it is
>>> useful only if shared interrupts are supported. That will imply it is dummy
>>> for type 1. I am avoiding too many type unnecessary checks especially in IRQ
>>> handler.
>> Understood.
>>
>>>>> They support level interrupt, so we can add them too. I understand you can
>>>>> test only type 3, but this driver caters for all and the code must be generic
>>>>> as much as possible. I don't see any point in check for type 3 only. Only
>>>> I understand what you do.
>>>> But type 2 also supports the communication flow from OSPM to Platfrom.
>>>> In this case, this flag will get in the way of type 2.
>>>>
>>> How ?
>> It should be ok if all types except for type 3 do not check this flag in
>> interrupt handle.
>> Namely, these types consider it as dummy, and do not use it, anywhere,
>> Right?
>>>> Whether the interrupt belongs to a type2 channel is only determined by
>>>> the status field in Generic Communications Channel Shared Memory Region,
>>>> which is done in rx_callback of PCC client.
>>> Agreed, but do you see any issue using the flag even if it acts as dummy ?
>> I think it can work well if these types completely ignore this flag, like below.
>> what do you think?
>>
>> -->8
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index ecd54f049de3..14405e99193d 100755
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -92,6 +92,13 @@ 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: this flag is used just to check if the interrupt needs
>> + * handling when it is shared. Since only one transfer can
>> occur
>> + * at a time and mailbox takes care of locking, this flag can
>> be
>> + * accessed without a lock. Note: the type only support the
>> + * communication from OSPM to Platform, like type3, use it, and
>> + * other types completely ignore it.
>> */
>> struct pcc_chan_info {
>> struct pcc_mbox_chan chan;
>> @@ -102,6 +109,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 +234,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 +292,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)
> I would have avoided the type check above but I understand your concern
> so let us keep it like this for now.
Thanks for your unstanding.
>
> Please submit non-RFC patch as some maintainers may not look at RFC.
I will send V2 ASAP.
>
PCC supports processing platform notification for slave subspaces and
shared interrupt for multiple subspaces.
---
-v2: extract cmd complete code into a function.
unrelated types regard chan_in_use as dummy.
-v1: using subspace type to replace comm_flow_dir in patch [1/2]
-rfc-v3: split V2 into two patches.
-rfc-v2: don't use platform interrupt ack register to identify if the given
channel should respond interrupt.
Huisong Li (2):
mailbox: pcc: Add support for platform notification handling
mailbox: pcc: Support shared interrupt for multiple subspaces
drivers/mailbox/pcc.c | 90 +++++++++++++++++++++++++++++++++++++------
1 file changed, 79 insertions(+), 11 deletions(-)
--
2.33.0
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.
The new field, 'chan_in_use' is used by the type only support the
communication from OSPM to Platform (like type3) and should be completely
ignored by other types so as to avoid too many type unnecessary checks in
IRQ handler.
Signed-off-by: Huisong Li <[email protected]>
---
drivers/mailbox/pcc.c | 42 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index a0a87c480d8b..7804dcf72646 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -92,6 +92,13 @@ 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: this flag is used just to check if the interrupt needs
+ * handling when it is shared. Since only one transfer can occur
+ * at a time and mailbox takes care of locking, this flag can be
+ * accessed without a lock. Note: the type only support the
+ * communication from OSPM to Platform, like type3, use it, and
+ * other types completely ignore it.
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -102,6 +109,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 +234,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_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
{
u64 val;
@@ -268,6 +283,10 @@ 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;
+
if (!pcc_mbox_cmd_complete_check(pchan))
return IRQ_NONE;
@@ -289,9 +308,12 @@ 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);
+ pchan->chan_in_use = false;
return IRQ_HANDLED;
}
@@ -340,10 +362,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);
@@ -405,7 +430,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->plat_irq > 0)
+ pchan->chan_in_use = true;
+
+ return ret;
}
static const struct mbox_chan_ops pcc_chan_ops = {
@@ -488,6 +517,7 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
pcct_ss->platform_interrupt);
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;
@@ -509,6 +539,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.33.0
Currently, PCC driver doesn't support the processing of platform
notification for type 4 PCC subspaces.
According to ACPI specification, if platform sends a notification
to OSPM, it must clear the command complete bit and trigger platform
interrupt. OSPM needs to check whether the command complete bit is
cleared, clear platform interrupt, process command, and then set the
command complete and ring doorbell to the Platform.
Let us stash the value of the pcc type and use the same while processing
the interrupt of the channel. We also need to set the command complete
bit and ring doorbell in the interrupt handler for the type 4 channel to
complete the communication flow after processing the notification from
the Platform.
Signed-off-by: Huisong Li <[email protected]>
---
drivers/mailbox/pcc.c | 50 +++++++++++++++++++++++++++++++++++--------
1 file changed, 41 insertions(+), 9 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 105d46c9801b..a0a87c480d8b 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -91,6 +91,7 @@ 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
+ * @type: PCC subspace type
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -100,12 +101,15 @@ struct pcc_chan_info {
struct pcc_chan_reg cmd_update;
struct pcc_chan_reg error;
int plat_irq;
+ u8 type;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
static struct pcc_chan_info *chan_info;
static int pcc_chan_count;
+static int pcc_send_data(struct mbox_chan *chan, void *data);
+
/*
* PCC can be used with perf critical drivers such as CPPC
* So it makes sense to locally cache the virtual address and
@@ -221,6 +225,34 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
return acpi_register_gsi(NULL, interrupt, trigger, polarity);
}
+static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
+{
+ u64 val;
+ int ret;
+
+ ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
+ if (ret)
+ return false;
+
+ if (!pchan->cmd_complete.gas)
+ return true;
+
+ /*
+ * Judge if the channel respond the interrupt based on the value of
+ * command complete.
+ */
+ val &= pchan->cmd_complete.status_mask;
+ /*
+ * If this is PCC slave subspace channel, and the command complete
+ * bit 0 indicates that Platform is sending a notification and OSPM
+ * needs to respond this interrupt to process this command.
+ */
+ if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ return !val;
+
+ return !!val;
+}
+
/**
* pcc_mbox_irq - PCC mailbox interrupt handler
* @irq: interrupt number
@@ -236,17 +268,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
int ret;
pchan = chan->con_priv;
-
- ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
- if (ret)
+ if (!pcc_mbox_cmd_complete_check(pchan))
return IRQ_NONE;
- if (val) { /* Ensure GAS exists and value is non-zero */
- val &= pchan->cmd_complete.status_mask;
- if (!val)
- return IRQ_NONE;
- }
-
ret = pcc_chan_reg_read(&pchan->error, &val);
if (ret)
return IRQ_NONE;
@@ -262,6 +286,13 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
mbox_chan_received_data(chan, NULL);
+ /*
+ * The PCC slave subspace channel needs to set the command complete bit
+ * and ring doorbell after processing message.
+ */
+ if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ pcc_send_data(chan, NULL);
+
return IRQ_HANDLED;
}
@@ -692,6 +723,7 @@ static int pcc_mbox_probe(struct platform_device *pdev)
pcc_parse_subspace_shmem(pchan, pcct_entry);
+ pchan->type = pcct_entry->type;
pcct_entry = (struct acpi_subtable_header *)
((unsigned long) pcct_entry + pcct_entry->length);
}
--
2.33.0
Kindly ping.
在 2023/3/14 19:11, Huisong Li 写道:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
>
> ---
> -v2: extract cmd complete code into a function.
> unrelated types regard chan_in_use as dummy.
> -v1: using subspace type to replace comm_flow_dir in patch [1/2]
> -rfc-v3: split V2 into two patches.
> -rfc-v2: don't use platform interrupt ack register to identify if the given
> channel should respond interrupt.
>
> Huisong Li (2):
> mailbox: pcc: Add support for platform notification handling
> mailbox: pcc: Support shared interrupt for multiple subspaces
>
> drivers/mailbox/pcc.c | 90 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 79 insertions(+), 11 deletions(-)
>
On Tue, Mar 14, 2023 at 07:11:34PM +0800, Huisong Li wrote:
> Currently, PCC driver doesn't support the processing of platform
> notification for type 4 PCC subspaces.
>
> According to ACPI specification, if platform sends a notification
> to OSPM, it must clear the command complete bit and trigger platform
> interrupt. OSPM needs to check whether the command complete bit is
> cleared, clear platform interrupt, process command, and then set the
> command complete and ring doorbell to the Platform.
>
> Let us stash the value of the pcc type and use the same while processing
> the interrupt of the channel. We also need to set the command complete
> bit and ring doorbell in the interrupt handler for the type 4 channel to
> complete the communication flow after processing the notification from
> the Platform.
>
> Signed-off-by: Huisong Li <[email protected]>
> ---
> drivers/mailbox/pcc.c | 50 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 41 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 105d46c9801b..a0a87c480d8b 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -91,6 +91,7 @@ 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
> + * @type: PCC subspace type
> */
> struct pcc_chan_info {
> struct pcc_mbox_chan chan;
> @@ -100,12 +101,15 @@ struct pcc_chan_info {
> struct pcc_chan_reg cmd_update;
> struct pcc_chan_reg error;
> int plat_irq;
> + u8 type;
> };
>
> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
> static struct pcc_chan_info *chan_info;
> static int pcc_chan_count;
>
> +static int pcc_send_data(struct mbox_chan *chan, void *data);
> +
> /*
> * PCC can be used with perf critical drivers such as CPPC
> * So it makes sense to locally cache the virtual address and
> @@ -221,6 +225,34 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> }
>
> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
> +{
> + u64 val;
> + int ret;
> +
> + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> + if (ret)
> + return false;
> +
> + if (!pchan->cmd_complete.gas)
> + return true;
> +
> + /*
> + * Judge if the channel respond the interrupt based on the value of
> + * command complete.
> + */
> + val &= pchan->cmd_complete.status_mask;
[super nit] Would prefer an blank line here.
--
Regards,
Sudeep
On Tue, Mar 14, 2023 at 07:11:33PM +0800, Huisong Li wrote:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
>
Other than a super minor nit in the patch 1/2, this looks good to me.
It would be good if we can get tested-by from
Robbie King <[email protected]>
Reviewed-by: Sudeep Holla <[email protected]>
--
Regards,
Sudeep
在 2023/3/27 19:30, Sudeep Holla 写道:
> On Tue, Mar 14, 2023 at 07:11:34PM +0800, Huisong Li wrote:
>> Currently, PCC driver doesn't support the processing of platform
>> notification for type 4 PCC subspaces.
>>
>> According to ACPI specification, if platform sends a notification
>> to OSPM, it must clear the command complete bit and trigger platform
>> interrupt. OSPM needs to check whether the command complete bit is
>> cleared, clear platform interrupt, process command, and then set the
>> command complete and ring doorbell to the Platform.
>>
>> Let us stash the value of the pcc type and use the same while processing
>> the interrupt of the channel. We also need to set the command complete
>> bit and ring doorbell in the interrupt handler for the type 4 channel to
>> complete the communication flow after processing the notification from
>> the Platform.
>>
>> Signed-off-by: Huisong Li <[email protected]>
>> ---
>> drivers/mailbox/pcc.c | 50 +++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 41 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 105d46c9801b..a0a87c480d8b 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -91,6 +91,7 @@ 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
>> + * @type: PCC subspace type
>> */
>> struct pcc_chan_info {
>> struct pcc_mbox_chan chan;
>> @@ -100,12 +101,15 @@ struct pcc_chan_info {
>> struct pcc_chan_reg cmd_update;
>> struct pcc_chan_reg error;
>> int plat_irq;
>> + u8 type;
>> };
>>
>> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>> static struct pcc_chan_info *chan_info;
>> static int pcc_chan_count;
>>
>> +static int pcc_send_data(struct mbox_chan *chan, void *data);
>> +
>> /*
>> * PCC can be used with perf critical drivers such as CPPC
>> * So it makes sense to locally cache the virtual address and
>> @@ -221,6 +225,34 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>> }
>>
>> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
>> +{
>> + u64 val;
>> + int ret;
>> +
>> + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
>> + if (ret)
>> + return false;
>> +
>> + if (!pchan->cmd_complete.gas)
>> + return true;
>> +
>> + /*
>> + * Judge if the channel respond the interrupt based on the value of
>> + * command complete.
>> + */
>> + val &= pchan->cmd_complete.status_mask;
> [super nit] Would prefer an blank line here.
>
Yeah, it would be better if there is an blank line here.
Is it necessary to send v3 for this?
在 2023/3/27 19:33, Sudeep Holla 写道:
> On Tue, Mar 14, 2023 at 07:11:33PM +0800, Huisong Li wrote:
>> PCC supports processing platform notification for slave subspaces and
>> shared interrupt for multiple subspaces.
>>
> Other than a super minor nit in the patch 1/2, this looks good to me.
> It would be good if we can get tested-by from
> Robbie King <[email protected]>
>
> Reviewed-by: Sudeep Holla <[email protected]>
Thanks for your review.????
@Robbie King, can you give us some feedback?
Looking forward to you reply.
On Mon, Mar 27, 2023 at 08:25:04PM +0800, lihuisong (C) wrote:
>
> 在 2023/3/27 19:30, Sudeep Holla 写道:
> > On Tue, Mar 14, 2023 at 07:11:34PM +0800, Huisong Li wrote:
> > > Currently, PCC driver doesn't support the processing of platform
> > > notification for type 4 PCC subspaces.
> > >
> > > According to ACPI specification, if platform sends a notification
> > > to OSPM, it must clear the command complete bit and trigger platform
> > > interrupt. OSPM needs to check whether the command complete bit is
> > > cleared, clear platform interrupt, process command, and then set the
> > > command complete and ring doorbell to the Platform.
> > >
> > > Let us stash the value of the pcc type and use the same while processing
> > > the interrupt of the channel. We also need to set the command complete
> > > bit and ring doorbell in the interrupt handler for the type 4 channel to
> > > complete the communication flow after processing the notification from
> > > the Platform.
> > >
> > > Signed-off-by: Huisong Li <[email protected]>
> > > ---
> > > drivers/mailbox/pcc.c | 50 +++++++++++++++++++++++++++++++++++--------
> > > 1 file changed, 41 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > > index 105d46c9801b..a0a87c480d8b 100644
> > > --- a/drivers/mailbox/pcc.c
> > > +++ b/drivers/mailbox/pcc.c
> > > @@ -91,6 +91,7 @@ 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
> > > + * @type: PCC subspace type
> > > */
> > > struct pcc_chan_info {
> > > struct pcc_mbox_chan chan;
> > > @@ -100,12 +101,15 @@ struct pcc_chan_info {
> > > struct pcc_chan_reg cmd_update;
> > > struct pcc_chan_reg error;
> > > int plat_irq;
> > > + u8 type;
> > > };
> > > #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
> > > static struct pcc_chan_info *chan_info;
> > > static int pcc_chan_count;
> > > +static int pcc_send_data(struct mbox_chan *chan, void *data);
> > > +
> > > /*
> > > * PCC can be used with perf critical drivers such as CPPC
> > > * So it makes sense to locally cache the virtual address and
> > > @@ -221,6 +225,34 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
> > > return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> > > }
> > > +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
> > > +{
> > > + u64 val;
> > > + int ret;
> > > +
> > > + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> > > + if (ret)
> > > + return false;
> > > +
> > > + if (!pchan->cmd_complete.gas)
> > > + return true;
> > > +
> > > + /*
> > > + * Judge if the channel respond the interrupt based on the value of
> > > + * command complete.
> > > + */
> > > + val &= pchan->cmd_complete.status_mask;
> > [super nit] Would prefer an blank line here.
> >
> Yeah, it would be better if there is an blank line here.
> Is it necessary to send v3 for this?
Let us see once you have Robbie's tested-by. Depending on who picks up we can
check with them.
--
Regards,
Sudeep
在 2023/3/27 20:31, lihuisong (C) 写道:
>
> 在 2023/3/27 19:33, Sudeep Holla 写道:
>> On Tue, Mar 14, 2023 at 07:11:33PM +0800, Huisong Li wrote:
>>> PCC supports processing platform notification for slave subspaces and
>>> shared interrupt for multiple subspaces.
>>>
>> Other than a super minor nit in the patch 1/2, this looks good to me.
>> It would be good if we can get tested-by from
>> Robbie King <[email protected]>
>>
>> Reviewed-by: Sudeep Holla <[email protected]>
> Thanks for your review.????
>
> @Robbie King, can you give us some feedback?
> Looking forward to you reply.
>
>
@Robbie King, kindly ping.
Apologies, missed earlier emails. Will make this a priority for the week.
-----Original Message-----
From: lihuisong (C) <[email protected]>
Sent: Sunday, April 9, 2023 9:27 PM
To: Sudeep Holla <[email protected]>; Robbie King <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v2 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt
在 2023/3/27 20:31, lihuisong (C) 写道:
>
> 在 2023/3/27 19:33, Sudeep Holla 写道:
>> On Tue, Mar 14, 2023 at 07:11:33PM +0800, Huisong Li wrote:
>>> PCC supports processing platform notification for slave subspaces
>>> and shared interrupt for multiple subspaces.
>>>
>> Other than a super minor nit in the patch 1/2, this looks good to me.
>> It would be good if we can get tested-by from Robbie King
>> <[email protected]>
>>
>> Reviewed-by: Sudeep Holla <[email protected]>
> Thanks for your review.????
>
> @Robbie King, can you give us some feedback?
> Looking forward to you reply.
>
>
@Robbie King, kindly ping.
在 2023/4/11 22:47, Robbie King 写道:
> Apologies, missed earlier emails. Will make this a priority for the week.
Thanks. Looking forward to you reply.
>
> -----Original Message-----
> From: lihuisong (C) <[email protected]>
> Sent: Sunday, April 9, 2023 9:27 PM
> To: Sudeep Holla <[email protected]>; Robbie King <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH v2 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt
>
>
> 在 2023/3/27 20:31, lihuisong (C) 写道:
>> 在 2023/3/27 19:33, Sudeep Holla 写道:
>>> On Tue, Mar 14, 2023 at 07:11:33PM +0800, Huisong Li wrote:
>>>> PCC supports processing platform notification for slave subspaces
>>>> and shared interrupt for multiple subspaces.
>>>>
>>> Other than a super minor nit in the patch 1/2, this looks good to me.
>>> It would be good if we can get tested-by from Robbie King
>>> <[email protected]>
>>>
>>> Reviewed-by: Sudeep Holla <[email protected]>
>> Thanks for your review.????
>>
>> @Robbie King, can you give us some feedback?
>> Looking forward to you reply.
>>
>>
> @Robbie King, kindly ping.
Sorry for the delay. I ran my simple stress test against the patch set and
saw no issues. For the record it is by no means a thorough regression, but it
has illuminated issues in the past.
The test itself uses a "heartbeat" module in the SCP firmware that generates
notifications at a programmable interval. The stress test is simply generating
these heartbeats (SCP to AP notifications) while also generating protocol version
queries (AP to SCP). The notifications are sequence numbered to verify none are
lost, however SCP to AP notification support does not support SCP generating
notifications faster than the AP can process them, so the heartbeat rate must be
reasonably slow (on the order of 10s of millliseconds).
-----Original Message-----
From: lihuisong (C) <[email protected]>
Sent: Thursday, April 13, 2023 9:05 PM
To: Robbie King <[email protected]>; Sudeep Holla <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v2 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt
在 2023/4/11 22:47, Robbie King 写道:
> Apologies, missed earlier emails. Will make this a priority for the week.
Thanks. Looking forward to you reply.
>
> -----Original Message-----
> From: lihuisong (C) <[email protected]>
> Sent: Sunday, April 9, 2023 9:27 PM
> To: Sudeep Holla <[email protected]>; Robbie King
> <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v2 0/2] mailbox: pcc: Support platform
> notification for type4 and shared interrupt
>
>
> 在 2023/3/27 20:31, lihuisong (C) 写道:
>> 在 2023/3/27 19:33, Sudeep Holla 写道:
>>> On Tue, Mar 14, 2023 at 07:11:33PM +0800, Huisong Li wrote:
>>>> PCC supports processing platform notification for slave subspaces
>>>> and shared interrupt for multiple subspaces.
>>>>
>>> Other than a super minor nit in the patch 1/2, this looks good to me.
>>> It would be good if we can get tested-by from Robbie King
>>> <[email protected]>
>>>
>>> Reviewed-by: Sudeep Holla <[email protected]>
>> Thanks for your review.????
>>
>> @Robbie King, can you give us some feedback?
>> Looking forward to you reply.
>>
>>
> @Robbie King, kindly ping.
在 2023/4/14 21:48, Robbie King 写道:
> Sorry for the delay. I ran my simple stress test against the patch set and
> saw no issues. For the record it is by no means a thorough regression, but it
> has illuminated issues in the past.
Thanks for your testing.
>
> The test itself uses a "heartbeat" module in the SCP firmware that generates
> notifications at a programmable interval. The stress test is simply generating
> these heartbeats (SCP to AP notifications) while also generating protocol version
> queries (AP to SCP). The notifications are sequence numbered to verify none are
> lost, however SCP to AP notification support does not support SCP generating
> notifications faster than the AP can process them, so the heartbeat rate must be
> reasonably slow (on the order of 10s of millliseconds).
I understand your concern. I think this doesn't get int the way of what
we are doing.
My stress tests were also run in type3 and type4 concurrent scenarios.
There were two drivers using type3 to send command looply on platform.
In the firmware terminal window,
there were two channels for type4 to generate notifications from
platform at the 1ms(even shorter) interval.
I didn't find anything issues in this stress after running a couple of
hours.
@Robbie King and @Sudeep, what do you think of my test?
>
> -----Original Message-----
> From: lihuisong (C) <[email protected]>
> Sent: Thursday, April 13, 2023 9:05 PM
> To: Robbie King <[email protected]>; Sudeep Holla <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH v2 0/2] mailbox: pcc: Support platform notification for type4 and shared interrupt
>
>
> 在 2023/4/11 22:47, Robbie King 写道:
>> Apologies, missed earlier emails. Will make this a priority for the week.
> Thanks. Looking forward to you reply.
>> -----Original Message-----
>> From: lihuisong (C) <[email protected]>
>> Sent: Sunday, April 9, 2023 9:27 PM
>> To: Sudeep Holla <[email protected]>; Robbie King
>> <[email protected]>
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH v2 0/2] mailbox: pcc: Support platform
>> notification for type4 and shared interrupt
>>
>>
>> 在 2023/3/27 20:31, lihuisong (C) 写道:
>>> 在 2023/3/27 19:33, Sudeep Holla 写道:
>>>> On Tue, Mar 14, 2023 at 07:11:33PM +0800, Huisong Li wrote:
>>>>> PCC supports processing platform notification for slave subspaces
>>>>> and shared interrupt for multiple subspaces.
>>>>>
>>>> Other than a super minor nit in the patch 1/2, this looks good to me.
>>>> It would be good if we can get tested-by from Robbie King
>>>> <[email protected]>
>>>>
>>>> Reviewed-by: Sudeep Holla <[email protected]>
>>> Thanks for your review.????
>>>
>>> @Robbie King, can you give us some feedback?
>>> Looking forward to you reply.
>>>
>>>
>> @Robbie King, kindly ping.
On Tue, Apr 18, 2023 at 10:21:54AM +0800, lihuisong (C) wrote:
>
> 在 2023/4/14 21:48, Robbie King 写道:
> > Sorry for the delay. I ran my simple stress test against the patch set and
> > saw no issues. For the record it is by no means a thorough regression, but it
> > has illuminated issues in the past.
> Thanks for your testing.
> >
> > The test itself uses a "heartbeat" module in the SCP firmware that generates
> > notifications at a programmable interval. The stress test is simply generating
> > these heartbeats (SCP to AP notifications) while also generating protocol version
> > queries (AP to SCP). The notifications are sequence numbered to verify none are
> > lost, however SCP to AP notification support does not support SCP generating
> > notifications faster than the AP can process them, so the heartbeat rate must be
> > reasonably slow (on the order of 10s of millliseconds).
> I understand your concern. I think this doesn't get int the way of what we
> are doing.
>
> My stress tests were also run in type3 and type4 concurrent scenarios.
> There were two drivers using type3 to send command looply on platform.
> In the firmware terminal window,
> there were two channels for type4 to generate notifications from platform at
> the 1ms(even shorter) interval.
> I didn't find anything issues in this stress after running a couple of
> hours.
>
> @Robbie King and @Sudeep, what do you think of my test?
>
IMO if there is a need to have this driver changes upstream, then it is good
enough test as it is the best that can be done at this time. We can always fix
the bugs or extend to new use-cases in the future.
Since it is merge window next week, it is quite late now. But sometimes
Rafael picks up additional patches late. So please post v3 even if there
are no changes with my reviewed-by and Robbie's tested-by so that I can ask
Rafael to pick it up.
--
Regards,
Sudeep
在 2023/4/21 18:55, Sudeep Holla 写道:
> On Tue, Apr 18, 2023 at 10:21:54AM +0800, lihuisong (C) wrote:
>> 在 2023/4/14 21:48, Robbie King 写道:
>>> Sorry for the delay. I ran my simple stress test against the patch set and
>>> saw no issues. For the record it is by no means a thorough regression, but it
>>> has illuminated issues in the past.
>> Thanks for your testing.
>>> The test itself uses a "heartbeat" module in the SCP firmware that generates
>>> notifications at a programmable interval. The stress test is simply generating
>>> these heartbeats (SCP to AP notifications) while also generating protocol version
>>> queries (AP to SCP). The notifications are sequence numbered to verify none are
>>> lost, however SCP to AP notification support does not support SCP generating
>>> notifications faster than the AP can process them, so the heartbeat rate must be
>>> reasonably slow (on the order of 10s of millliseconds).
>> I understand your concern. I think this doesn't get int the way of what we
>> are doing.
>>
>> My stress tests were also run in type3 and type4 concurrent scenarios.
>> There were two drivers using type3 to send command looply on platform.
>> In the firmware terminal window,
>> there were two channels for type4 to generate notifications from platform at
>> the 1ms(even shorter) interval.
>> I didn't find anything issues in this stress after running a couple of
>> hours.
>>
>> @Robbie King and @Sudeep, what do you think of my test?
>>
> IMO if there is a need to have this driver changes upstream, then it is good
> enough test as it is the best that can be done at this time. We can always fix
> the bugs or extend to new use-cases in the future.
>
> Since it is merge window next week, it is quite late now. But sometimes
> Rafael picks up additional patches late. So please post v3 even if there
> are no changes with my reviewed-by and Robbie's tested-by so that I can ask
> Rafael to pick it up.
All right. patch 2/2 needs to be updated because of the following patch:
76d4adacd52e ("mailbox: pcc: Use mbox_bind_client")
v3 will be sent ASAP.
>
PCC supports processing platform notification for slave subspaces and
shared interrupt for multiple subspaces.
---
-v3: update requesting shared irq code due to pcc using mbox_bind_client.
-v2: extract cmd complete code into a function.
unrelated types regard chan_in_use as dummy.
-v1: using subspace type to replace comm_flow_dir in patch [1/2]
-rfc-v3: split V2 into two patches.
-rfc-v2: don't use platform interrupt ack register to identify if the given
channel should respond interrupt.
Huisong Li (2):
mailbox: pcc: Add support for platform notification handling
mailbox: pcc: Support shared interrupt for multiple subspaces
drivers/mailbox/pcc.c | 91 +++++++++++++++++++++++++++++++++++++------
1 file changed, 80 insertions(+), 11 deletions(-)
--
2.22.0
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.
The new field, 'chan_in_use' is used by the type only support the
communication from OSPM to Platform (like type3) and should be completely
ignored by other types so as to avoid too many type unnecessary checks in
IRQ handler.
Signed-off-by: Huisong Li <[email protected]>
---
drivers/mailbox/pcc.c | 43 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 40 insertions(+), 3 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 80310b48bfb6..94885e411085 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -92,6 +92,13 @@ 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: this flag is used just to check if the interrupt needs
+ * handling when it is shared. Since only one transfer can occur
+ * at a time and mailbox takes care of locking, this flag can be
+ * accessed without a lock. Note: the type only support the
+ * communication from OSPM to Platform, like type3, use it, and
+ * other types completely ignore it.
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -102,6 +109,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 +234,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_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
{
u64 val;
@@ -242,6 +257,7 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
* command complete.
*/
val &= pchan->cmd_complete.status_mask;
+
/*
* If this is PCC slave subspace channel, and the command complete
* bit 0 indicates that Platform is sending a notification and OSPM
@@ -268,6 +284,10 @@ 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;
+
if (!pcc_mbox_cmd_complete_check(pchan))
return IRQ_NONE;
@@ -289,9 +309,12 @@ 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);
+ pchan->chan_in_use = false;
return IRQ_HANDLED;
}
@@ -371,7 +394,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->plat_irq > 0)
+ pchan->chan_in_use = true;
+
+ return ret;
}
/**
@@ -384,11 +411,14 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
static int pcc_startup(struct mbox_chan *chan)
{
struct pcc_chan_info *pchan = chan->con_priv;
+ unsigned long irqflags;
int rc;
if (pchan->plat_irq > 0) {
- rc = devm_request_irq(chan->mbox->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(chan->mbox->dev, pchan->plat_irq, pcc_mbox_irq,
+ irqflags, MBOX_IRQ_NAME, chan);
if (unlikely(rc)) {
dev_err(chan->mbox->dev, "failed to register PCC interrupt %d\n",
pchan->plat_irq);
@@ -494,6 +524,7 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
pcct_ss->platform_interrupt);
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;
@@ -515,6 +546,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.22.0
Currently, PCC driver doesn't support the processing of platform
notification for type 4 PCC subspaces.
According to ACPI specification, if platform sends a notification
to OSPM, it must clear the command complete bit and trigger platform
interrupt. OSPM needs to check whether the command complete bit is
cleared, clear platform interrupt, process command, and then set the
command complete and ring doorbell to the Platform.
Let us stash the value of the pcc type and use the same while processing
the interrupt of the channel. We also need to set the command complete
bit and ring doorbell in the interrupt handler for the type 4 channel to
complete the communication flow after processing the notification from
the Platform.
Signed-off-by: Huisong Li <[email protected]>
---
drivers/mailbox/pcc.c | 50 +++++++++++++++++++++++++++++++++++--------
1 file changed, 41 insertions(+), 9 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index a44d4b3e5beb..80310b48bfb6 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -91,6 +91,7 @@ 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
+ * @type: PCC subspace type
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -100,12 +101,15 @@ struct pcc_chan_info {
struct pcc_chan_reg cmd_update;
struct pcc_chan_reg error;
int plat_irq;
+ u8 type;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
static struct pcc_chan_info *chan_info;
static int pcc_chan_count;
+static int pcc_send_data(struct mbox_chan *chan, void *data);
+
/*
* PCC can be used with perf critical drivers such as CPPC
* So it makes sense to locally cache the virtual address and
@@ -221,6 +225,34 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
return acpi_register_gsi(NULL, interrupt, trigger, polarity);
}
+static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
+{
+ u64 val;
+ int ret;
+
+ ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
+ if (ret)
+ return false;
+
+ if (!pchan->cmd_complete.gas)
+ return true;
+
+ /*
+ * Judge if the channel respond the interrupt based on the value of
+ * command complete.
+ */
+ val &= pchan->cmd_complete.status_mask;
+ /*
+ * If this is PCC slave subspace channel, and the command complete
+ * bit 0 indicates that Platform is sending a notification and OSPM
+ * needs to respond this interrupt to process this command.
+ */
+ if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ return !val;
+
+ return !!val;
+}
+
/**
* pcc_mbox_irq - PCC mailbox interrupt handler
* @irq: interrupt number
@@ -236,17 +268,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
int ret;
pchan = chan->con_priv;
-
- ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
- if (ret)
+ if (!pcc_mbox_cmd_complete_check(pchan))
return IRQ_NONE;
- if (val) { /* Ensure GAS exists and value is non-zero */
- val &= pchan->cmd_complete.status_mask;
- if (!val)
- return IRQ_NONE;
- }
-
ret = pcc_chan_reg_read(&pchan->error, &val);
if (ret)
return IRQ_NONE;
@@ -262,6 +286,13 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
mbox_chan_received_data(chan, NULL);
+ /*
+ * The PCC slave subspace channel needs to set the command complete bit
+ * and ring doorbell after processing message.
+ */
+ if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ pcc_send_data(chan, NULL);
+
return IRQ_HANDLED;
}
@@ -698,6 +729,7 @@ static int pcc_mbox_probe(struct platform_device *pdev)
pcc_parse_subspace_shmem(pchan, pcct_entry);
+ pchan->type = pcct_entry->type;
pcct_entry = (struct acpi_subtable_header *)
((unsigned long) pcct_entry + pcct_entry->length);
}
--
2.22.0
在 2023/4/21 18:55, Sudeep Holla 写道:
> On Tue, Apr 18, 2023 at 10:21:54AM +0800, lihuisong (C) wrote:
>> 在 2023/4/14 21:48, Robbie King 写道:
>>> Sorry for the delay. I ran my simple stress test against the patch set and
>>> saw no issues. For the record it is by no means a thorough regression, but it
>>> has illuminated issues in the past.
>> Thanks for your testing.
>>> The test itself uses a "heartbeat" module in the SCP firmware that generates
>>> notifications at a programmable interval. The stress test is simply generating
>>> these heartbeats (SCP to AP notifications) while also generating protocol version
>>> queries (AP to SCP). The notifications are sequence numbered to verify none are
>>> lost, however SCP to AP notification support does not support SCP generating
>>> notifications faster than the AP can process them, so the heartbeat rate must be
>>> reasonably slow (on the order of 10s of millliseconds).
>> I understand your concern. I think this doesn't get int the way of what we
>> are doing.
>>
>> My stress tests were also run in type3 and type4 concurrent scenarios.
>> There were two drivers using type3 to send command looply on platform.
>> In the firmware terminal window,
>> there were two channels for type4 to generate notifications from platform at
>> the 1ms(even shorter) interval.
>> I didn't find anything issues in this stress after running a couple of
>> hours.
>>
>> @Robbie King and @Sudeep, what do you think of my test?
>>
> IMO if there is a need to have this driver changes upstream, then it is good
> enough test as it is the best that can be done at this time. We can always fix
> the bugs or extend to new use-cases in the future.
>
> Since it is merge window next week, it is quite late now. But sometimes
> Rafael picks up additional patches late. So please post v3 even if there
> are no changes with my reviewed-by and Robbie's tested-by so that I can ask
> Rafael to pick it up.
Hi Robbie and Sudeep,
v3 has been sent.
Can you take a look at this series again?
Looking forward to your reply.
>
Hi Rafael,
On Sun, Apr 23, 2023 at 07:03:33PM +0800, Huisong Li wrote:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
Can you please take this via your tree ? It has been reviewed and
tested now. If it is too late for this merge window, are you happy to
pick it up if posted at -rc1 for next merge window ?
--
Regards,
Sudeep
Hi Sudeep,
Can you add Reviewed-by again?
I forgot to do it when send v3.
There is no the 'Reviewed-by' tag on patchwork now.
I'm not sure if this affects patch merging.
/Huisong
在 2023/4/23 19:03, Huisong Li 写道:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
>
> ---
> -v3: update requesting shared irq code due to pcc using mbox_bind_client.
> -v2: extract cmd complete code into a function.
> unrelated types regard chan_in_use as dummy.
> -v1: using subspace type to replace comm_flow_dir in patch [1/2]
> -rfc-v3: split V2 into two patches.
> -rfc-v2: don't use platform interrupt ack register to identify if the given
> channel should respond interrupt.
>
> Huisong Li (2):
> mailbox: pcc: Add support for platform notification handling
> mailbox: pcc: Support shared interrupt for multiple subspaces
>
> drivers/mailbox/pcc.c | 91 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 80 insertions(+), 11 deletions(-)
>
On Sun, Apr 23, 2023 at 07:03:33PM +0800, Huisong Li wrote:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
>
> ---
> -v3: update requesting shared irq code due to pcc using mbox_bind_client.
> -v2: extract cmd complete code into a function.
> unrelated types regard chan_in_use as dummy.
> -v1: using subspace type to replace comm_flow_dir in patch [1/2]
> -rfc-v3: split V2 into two patches.
> -rfc-v2: don't use platform interrupt ack register to identify if the given
> channel should respond interrupt.
>
> Huisong Li (2):
> mailbox: pcc: Add support for platform notification handling
> mailbox: pcc: Support shared interrupt for multiple subspaces
>
Reviewed-by: Sudeep Holla <[email protected]>
--
Regards,
Sudeep
Hi Rafael,
This series have been discussed, reviewed and tested.
Can you take a look at this series?
/Huisong
在 2023/4/25 22:41, Sudeep Holla 写道:
> Hi Rafael,
>
> On Sun, Apr 23, 2023 at 07:03:33PM +0800, Huisong Li wrote:
>> PCC supports processing platform notification for slave subspaces and
>> shared interrupt for multiple subspaces.
> Can you please take this via your tree ? It has been reviewed and
> tested now. If it is too late for this merge window, are you happy to
> pick it up if posted at -rc1 for next merge window ?
>
PCC supports processing platform notification for slave subspaces and
shared interrupt for multiple subspaces.
---
-v4: add Reviewed-by.
-v3: update requesting shared irq code due to pcc using mbox_bind_client.
-v2: extract cmd complete code into a function.
unrelated types regard chan_in_use as dummy.
-v1: using subspace type to replace comm_flow_dir in patch [1/2]
-rfc-v3: split V2 into two patches.
-rfc-v2: don't use platform interrupt ack register to identify if the given
channel should respond interrupt.
Huisong Li (2):
mailbox: pcc: Add support for platform notification handling
mailbox: pcc: Support shared interrupt for multiple subspaces
drivers/mailbox/pcc.c | 91 +++++++++++++++++++++++++++++++++++++------
1 file changed, 80 insertions(+), 11 deletions(-)
--
2.22.0
Hi Rafael,
On Tue, Jun 13, 2023 at 08:57:26PM +0800, Huisong Li wrote:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
>
Can you pick up these patches please ?
It missed last merge window narrowly as I didn't want to push it that late.
So it would be good to get this in this time around.
--
Regards,
Sudeep
On 2023/6/13 20:57, Huisong Li wrote:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
>
> ---
> -v4: add Reviewed-by.
> -v3: update requesting shared irq code due to pcc using mbox_bind_client.
> -v2: extract cmd complete code into a function.
> unrelated types regard chan_in_use as dummy.
> -v1: using subspace type to replace comm_flow_dir in patch [1/2]
> -rfc-v3: split V2 into two patches.
> -rfc-v2: don't use platform interrupt ack register to identify if the given
> channel should respond interrupt.
>
> Huisong Li (2):
> mailbox: pcc: Add support for platform notification handling
> mailbox: pcc: Support shared interrupt for multiple subspaces
>
> drivers/mailbox/pcc.c | 91 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 80 insertions(+), 11 deletions(-)
>
Reviewed-by: Hanjun Guo <[email protected]>
Hi Rafael,
Can you merge this series if it is ok for you.
They've already missed two merge windows.????
Regards,
Huisong
在 2023/6/14 23:58, Sudeep Holla 写道:
> Hi Rafael,
>
> On Tue, Jun 13, 2023 at 08:57:26PM +0800, Huisong Li wrote:
>> PCC supports processing platform notification for slave subspaces and
>> shared interrupt for multiple subspaces.
>>
> Can you pick up these patches please ?
> It missed last merge window narrowly as I didn't want to push it that late.
> So it would be good to get this in this time around.
>
Kindly ping.
在 2023/7/14 14:39, lihuisong (C) 写道:
> Hi Rafael,
>
> Can you merge this series if it is ok for you.
> They've already missed two merge windows.????
>
> Regards,
> Huisong
>
>
> 在 2023/6/14 23:58, Sudeep Holla 写道:
>> Hi Rafael,
>>
>> On Tue, Jun 13, 2023 at 08:57:26PM +0800, Huisong Li wrote:
>>> PCC supports processing platform notification for slave subspaces and
>>> shared interrupt for multiple subspaces.
>>>
>> Can you pick up these patches please ?
>> It missed last merge window narrowly as I didn't want to push it that
>> late.
>> So it would be good to get this in this time around.
>>
> .
PCC supports processing platform notification for slave subspaces and
shared interrupt for multiple subspaces.
---
-v4: add Reviewed-by.
-v3: update requesting shared irq code due to pcc using mbox_bind_client.
-v2: extract cmd complete code into a function.
unrelated types regard chan_in_use as dummy.
-v1: using subspace type to replace comm_flow_dir in patch [1/2]
-rfc-v3: split V2 into two patches.
-rfc-v2: don't use platform interrupt ack register to identify if the given
channel should respond interrupt.
Huisong Li (2):
mailbox: pcc: Add support for platform notification handling
mailbox: pcc: Support shared interrupt for multiple subspaces
drivers/mailbox/pcc.c | 91 +++++++++++++++++++++++++++++++++++++------
1 file changed, 80 insertions(+), 11 deletions(-)
--
2.22.0
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.
The new field, 'chan_in_use' is used by the type only support the
communication from OSPM to Platform (like type3) and should be completely
ignored by other types so as to avoid too many type unnecessary checks in
IRQ handler.
Signed-off-by: Huisong Li <[email protected]>
Reviewed-by: Sudeep Holla <[email protected]>
Reviewed-by: Hanjun Guo <[email protected]>
---
drivers/mailbox/pcc.c | 43 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 40 insertions(+), 3 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 80310b48bfb6..94885e411085 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -92,6 +92,13 @@ 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: this flag is used just to check if the interrupt needs
+ * handling when it is shared. Since only one transfer can occur
+ * at a time and mailbox takes care of locking, this flag can be
+ * accessed without a lock. Note: the type only support the
+ * communication from OSPM to Platform, like type3, use it, and
+ * other types completely ignore it.
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -102,6 +109,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 +234,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_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
{
u64 val;
@@ -242,6 +257,7 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
* command complete.
*/
val &= pchan->cmd_complete.status_mask;
+
/*
* If this is PCC slave subspace channel, and the command complete
* bit 0 indicates that Platform is sending a notification and OSPM
@@ -268,6 +284,10 @@ 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;
+
if (!pcc_mbox_cmd_complete_check(pchan))
return IRQ_NONE;
@@ -289,9 +309,12 @@ 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);
+ pchan->chan_in_use = false;
return IRQ_HANDLED;
}
@@ -371,7 +394,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->plat_irq > 0)
+ pchan->chan_in_use = true;
+
+ return ret;
}
/**
@@ -384,11 +411,14 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
static int pcc_startup(struct mbox_chan *chan)
{
struct pcc_chan_info *pchan = chan->con_priv;
+ unsigned long irqflags;
int rc;
if (pchan->plat_irq > 0) {
- rc = devm_request_irq(chan->mbox->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(chan->mbox->dev, pchan->plat_irq, pcc_mbox_irq,
+ irqflags, MBOX_IRQ_NAME, chan);
if (unlikely(rc)) {
dev_err(chan->mbox->dev, "failed to register PCC interrupt %d\n",
pchan->plat_irq);
@@ -494,6 +524,7 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
pcct_ss->platform_interrupt);
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;
@@ -515,6 +546,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.22.0
Hi Rafael,
On Tue, Aug 01, 2023 at 02:38:25PM +0800, Huisong Li wrote:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
>
These changes have missed last 2 merge window. Let us know if you can pull
this for v6.6 or you prefer to route this via mailbox.
--
Regards,
Sudeep
Hi Rafael,
kindly ping.
在 2023/8/1 17:38, Sudeep Holla 写道:
> Hi Rafael,
>
> On Tue, Aug 01, 2023 at 02:38:25PM +0800, Huisong Li wrote:
>> PCC supports processing platform notification for slave subspaces and
>> shared interrupt for multiple subspaces.
>>
> These changes have missed last 2 merge window. Let us know if you can pull
> this for v6.6 or you prefer to route this via mailbox.
>
Hi Rafael,
Can you take a look at this series?
All be done. This series have missed two merge window.
在 2023/8/9 19:44, lihuisong (C) 写道:
> Hi Rafael,
>
> kindly ping.
>
>
> 在 2023/8/1 17:38, Sudeep Holla 写道:
>> Hi Rafael,
>>
>> On Tue, Aug 01, 2023 at 02:38:25PM +0800, Huisong Li wrote:
>>> PCC supports processing platform notification for slave subspaces and
>>> shared interrupt for multiple subspaces.
>>>
>> These changes have missed last 2 merge window. Let us know if you can
>> pull
>> this for v6.6 or you prefer to route this via mailbox.
>>
> .
kindly ping.
在 2023/8/17 14:50, lihuisong (C) 写道:
> Hi Rafael,
>
> Can you take a look at this series?
> All be done. This series have missed two merge window.
>
>
> 在 2023/8/9 19:44, lihuisong (C) 写道:
>> Hi Rafael,
>>
>> kindly ping.
>>
>>
>> 在 2023/8/1 17:38, Sudeep Holla 写道:
>>> Hi Rafael,
>>>
>>> On Tue, Aug 01, 2023 at 02:38:25PM +0800, Huisong Li wrote:
>>>> PCC supports processing platform notification for slave subspaces and
>>>> shared interrupt for multiple subspaces.
>>>>
>>> These changes have missed last 2 merge window. Let us know if you
>>> can pull
>>> this for v6.6 or you prefer to route this via mailbox.
>>>
>> .
>
> .
Hi Rafael,
On Tue, Aug 01, 2023 at 10:38:31AM +0100, Sudeep Holla wrote:
> Hi Rafael,
>
> On Tue, Aug 01, 2023 at 02:38:25PM +0800, Huisong Li wrote:
> > PCC supports processing platform notification for slave subspaces and
> > shared interrupt for multiple subspaces.
> >
>
> These changes have missed last 2 merge window. Let us know if you can pull
> this for v6.6 or you prefer to route this via mailbox.
>
Sorry for the nag. Do you prefer pull request for this ? Since it was just 2
patch I didn't add that option before. Let me know.
--
Regards,
Sudeep
On Tue, 01 Aug 2023 14:38:25 +0800, Huisong Li wrote:
> PCC supports processing platform notification for slave subspaces and
> shared interrupt for multiple subspaces.
>
Applied to sudeep.holla/linux (for-next/pcc/updates), thanks!
[1/2] mailbox: pcc: Add support for platform notification handling
https://git.kernel.org/sudeep.holla/c/60c40b06fa68
[2/2] mailbox: pcc: Support shared interrupt for multiple subspaces
https://git.kernel.org/sudeep.holla/c/3db174e478cb
--
Regards,
Sudeep