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

  As ACPI protocol descripted, if interrupts are level, a GSIV may
be shared by multiple subspaces, but each one must have unique
platform interrupt ack preserve and ack set masks. Therefore, need
set to shared interrupt for types that can distinguish interrupt
response channel if platform interrupt mode is level triggered.

The distinguishing point isn't definitely command complete register.
Because the two status values of command complete indicate that
there is no interrupt in a subspace('1' means subspace is free for
use, and '0' means platform is processing the command). On the whole,
the platform interrupt ack register is more suitable for this role.
As ACPI protocol said, If the subspace does support interrupts, and
these are level, this register must be supplied. And is used to clear
the interrupt by using a read, modify, write sequence. This register
is a 'WR' register, the bit corresponding to the subspace is '1' when
the command is completed, or is '0'.

Therefore, register shared interrupt for multiple subspaces if support
platform interrupt ack register and interrupts are level, and read the
ack register to ensure the idle or unfinished command channels to
quickly return IRQ_NONE.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/mailbox/pcc.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)
  

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 <lihuisong@huawei.com>
> ---
>   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.
  

在 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 <lihuisong@huawei.com>
> ---
>  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.
  

在 2022/10/27 23:53, Sudeep Holla 写道:
> On Sun, Oct 16, 2022 at 11:40:43AM +0800, Huisong Li wrote:
>> As ACPI protocol descripted, if interrupts are level, a GSIV may
>> be shared by multiple subspaces, but each one must have unique
>> platform interrupt ack preserve and ack set masks. Therefore, need
>> set to shared interrupt for types that can distinguish interrupt
>> response channel if platform interrupt mode is level triggered.
>>
>> The distinguishing point isn't definitely command complete register.
>> Because the two status values of command complete indicate that
>> there is no interrupt in a subspace('1' means subspace is free for
>> use, and '0' means platform is processing the command). On the whole,
>> the platform interrupt ack register is more suitable for this role.
>> As ACPI protocol said, If the subspace does support interrupts, and
>> these are level, this register must be supplied. And is used to clear
>> the interrupt by using a read, modify, write sequence. This register
>> is a 'WR' register, the bit corresponding to the subspace is '1' when
>> the command is completed, or is '0'.
>>
>> Therefore, register shared interrupt for multiple subspaces if support
>> platform interrupt ack register and interrupts are level, and read the
>> ack register to ensure the idle or unfinished command channels to
>> quickly return IRQ_NONE.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   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 <lihuisong@huawei.com>
> > > ---
> > >   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 <sudeep.holla@arm.com>
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 <sudeep.holla@arm.com>
---
 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 <lihuisong@huawei.com>
>>>> ---
>>>>    drivers/mailbox/pcc.c | 27 +++++++++++++++++++++++++--
>>>>    1 file changed, 25 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>>> index 3c2bc0ca454c..86c6cc44c73d 100644
>>>> --- a/drivers/mailbox/pcc.c
>>>> +++ b/drivers/mailbox/pcc.c
>>>> @@ -100,6 +100,7 @@ struct pcc_chan_info {
>>>>    	struct pcc_chan_reg cmd_update;
>>>>    	struct pcc_chan_reg error;
>>>>    	int plat_irq;
>>>> +	u8 plat_irq_trigger;
>>>>    };
>>>>    #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>>>> @@ -236,6 +237,15 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>>>    	int ret;
>>>>    	pchan = chan->con_priv;
>>>> +	ret = pcc_chan_reg_read(&pchan->plat_irq_ack, &val);
>>>> +	if (ret)
>>>> +		return IRQ_NONE;
>>>> +	/* Irq ack GAS exist and check if this interrupt has the channel. */
>>>> +	if (pchan->plat_irq_ack.gas) {
>>>> +		val &= pchan->plat_irq_ack.set_mask;
>>> I am not sure if the above is correct. The spec doesn't specify that the
>>> set_mask can be used to detect if the interrupt belongs to this channel.
>>> We need clarification to use those bits.
>> Yes, the spec only say that the interrupt ack register is used to clear the
>> interrupt by using a read, modify, write sequence. But the processing
>> of PCC driver is as follows:
>> Irq Ack Register = (Irq Ack Register & Preserve_mask) | Set_mask
>>
>> The set_mask is using to clear the interrupt of this channel by using OR
>> operation. And it should be write '1' to the corresponding bit of the
>> channel
>> to clear interrupt. So I think it is ok to use set_mask to detect if the
>> interrupt belongs to this channel.
> The problem is it can be write-only register and reads as always zero.
But it seems that it must be a read/write register according to the ACPI 
spec.
> I know a platform with such a behaviour.
Can you tell me which platform?
>
>>> This triggered be that I have a patch to address this. I will try to search
>>> and share, but IIRC I had a flag set when the doorbell was rung to track
>>> which channel or when to expect the irq. I will dig that up.
>> Looking forward to your patch.😁
> Please find below. I am not convinced yet to have extra flag for checking if
> the channel is in use. The other idea I had is to use the Generic Communications
> Channel Shared Memory Region Status Field in particular Command Complete
> field. I haven't tried it yet and hence the reason for not posting the patch.
> Let me know if the idea looks sane, so that I can try something and share
I don't think it is feasible. From the spec, the Command Complete field 
in the Generic
Communications Channel Shared Memory Region Status Field for type1/2 is 
similar to
the Command Complete Check Register in Master Slave Communications 
Channel Shared
Memory Region for type3/4.
> it. I may not have a setup handy to test and may need sometime to test though.
>
> Regards,
> Sudeep
>
> -->8
> >From 6dd9ad4f3a11dc9b97d308e70b544337c4169803 Mon Sep 17 00:00:00 2001
> From: Sudeep Holla <sudeep.holla@arm.com>
> Date: Thu, 27 Oct 2022 21:51:39 +0100
> Subject: [PATCH] ACPI: PCC: Support shared level triggered interrupt for
>   multiple subspaces
>
> If the platform acknowledge interrupt is level triggered, then it can
> be shared by multiple subspaces provided each one has a unique platform
> interrupt ack preserve and ack set masks.
>
> If it can be shared, then we can request the irq with IRQF_SHARED and
> IRQF_ONESHOT flags. The first one indicating it can be shared and the
> latter one to keep the interrupt disabled after the hardirq handler
> finished.
after --> until , right?
>
> Further, since there is no way to detect if the interrupt is for a given
> channel as the interrupt ack preserve and ack set masks are for clearing
> the interrupt and not for reading the status, we need a way to identify
> if the given channel is in use and expecting the interrupt.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   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 <lihuisong@huawei.com>
>>>>> ---
>>>>>    drivers/mailbox/pcc.c | 27 +++++++++++++++++++++++++--
>>>>>    1 file changed, 25 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>>>> index 3c2bc0ca454c..86c6cc44c73d 100644
>>>>> --- a/drivers/mailbox/pcc.c
>>>>> +++ b/drivers/mailbox/pcc.c
>>>>> @@ -100,6 +100,7 @@ struct pcc_chan_info {
>>>>>        struct pcc_chan_reg cmd_update;
>>>>>        struct pcc_chan_reg error;
>>>>>        int plat_irq;
>>>>> +    u8 plat_irq_trigger;
>>>>>    };
>>>>>    #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>>>>> @@ -236,6 +237,15 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>>>>        int ret;
>>>>>        pchan = chan->con_priv;
>>>>> +    ret = pcc_chan_reg_read(&pchan->plat_irq_ack, &val);
>>>>> +    if (ret)
>>>>> +        return IRQ_NONE;
>>>>> +    /* Irq ack GAS exist and check if this interrupt has the channel. */
>>>>> +    if (pchan->plat_irq_ack.gas) {
>>>>> +        val &= pchan->plat_irq_ack.set_mask;
>>>> I am not sure if the above is correct. The spec doesn't specify that the
>>>> set_mask can be used to detect if the interrupt belongs to this channel.
>>>> We need clarification to use those bits.
>>> Yes, the spec only say that the interrupt ack register is used to clear the
>>> interrupt by using a read, modify, write sequence. But the processing
>>> of PCC driver is as follows:
>>> Irq Ack Register = (Irq Ack Register & Preserve_mask) | Set_mask
>>>
>>> The set_mask is using to clear the interrupt of this channel by using OR
>>> operation. And it should be write '1' to the corresponding bit of the
>>> channel
>>> to clear interrupt. So I think it is ok to use set_mask to detect if the
>>> interrupt belongs to this channel.
>> The problem is it can be write-only register and reads as always zero.
> But it seems that it must be a read/write register according to the ACPI spec.
>> I know a platform with such a behaviour.
> Can you tell me which platform?
>>
>>>> This triggered be that I have a patch to address this. I will try to search
>>>> and share, but IIRC I had a flag set when the doorbell was rung to track
>>>> which channel or when to expect the irq. I will dig that up.
>>> Looking forward to your patch.😁
>> Please find below. I am not convinced yet to have extra flag for checking if
>> the channel is in use. The other idea I had is to use the Generic Communications
>> Channel Shared Memory Region Status Field in particular Command Complete
>> field. I haven't tried it yet and hence the reason for not posting the patch.
>> Let me know if the idea looks sane, so that I can try something and share
> I don't think it is feasible. From the spec, the Command Complete field in the Generic
> Communications Channel Shared Memory Region Status Field for type1/2 is similar to
> the Command Complete Check Register in Master Slave Communications Channel Shared
> Memory Region for type3/4.
>> it. I may not have a setup handy to test and may need sometime to test though.
>>
>> Regards,
>> Sudeep
>>
>> -->8
>> >From 6dd9ad4f3a11dc9b97d308e70b544337c4169803 Mon Sep 17 00:00:00 2001
>> From: Sudeep Holla <sudeep.holla@arm.com>
>> Date: Thu, 27 Oct 2022 21:51:39 +0100
>> Subject: [PATCH] ACPI: PCC: Support shared level triggered interrupt for
>>   multiple subspaces
>>
>> If the platform acknowledge interrupt is level triggered, then it can
>> be shared by multiple subspaces provided each one has a unique platform
>> interrupt ack preserve and ack set masks.
>>
>> If it can be shared, then we can request the irq with IRQF_SHARED and
>> IRQF_ONESHOT flags. The first one indicating it can be shared and the
>> latter one to keep the interrupt disabled after the hardirq handler
>> finished.
> after --> until , right?
>>
>> Further, since there is no way to detect if the interrupt is for a given
>> channel as the interrupt ack preserve and ack set masks are for clearing
>> the interrupt and not for reading the status, we need a way to identify
>> if the given channel is in use and expecting the interrupt.
>>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>   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 :).
  

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                        ^
------                        ^
  

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

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

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.
  

在 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?
  

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(-)
>
  

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: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 <robbiek@xsightlabs.com>

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
  

在 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 <robbiek@xsightlabs.com>
>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Thanks for your review.😁

@Robbie King, can you give us some feedback?
Looking forward to you reply.
  

在 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 <robbiek@xsightlabs.com>
>>
>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> 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) <lihuisong@huawei.com> 
Sent: Sunday, April 9, 2023 9:27 PM
To: Sudeep Holla <sudeep.holla@arm.com>; Robbie King <robbiek@xsightlabs.com>
Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org; rafael.j.wysocki@intel.com; wanghuiqiang@huawei.com; zhangzekun11@huawei.com; wangxiongfeng2@huawei.com; tanxiaofei@huawei.com; guohanjun@huawei.com; xiexiuqi@huawei.com; wangkefeng.wang@huawei.com; huangdaode@huawei.com
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 
>> <robbiek@xsightlabs.com>
>>
>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> 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) <lihuisong@huawei.com>
> Sent: Sunday, April 9, 2023 9:27 PM
> To: Sudeep Holla <sudeep.holla@arm.com>; Robbie King <robbiek@xsightlabs.com>
> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org; rafael.j.wysocki@intel.com; wanghuiqiang@huawei.com; zhangzekun11@huawei.com; wangxiongfeng2@huawei.com; tanxiaofei@huawei.com; guohanjun@huawei.com; xiexiuqi@huawei.com; wangkefeng.wang@huawei.com; huangdaode@huawei.com
> 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
>>> <robbiek@xsightlabs.com>
>>>
>>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>> 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) <lihuisong@huawei.com> 
Sent: Thursday, April 13, 2023 9:05 PM
To: Robbie King <robbiek@xsightlabs.com>; Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org; rafael.j.wysocki@intel.com; wanghuiqiang@huawei.com; zhangzekun11@huawei.com; wangxiongfeng2@huawei.com; tanxiaofei@huawei.com; guohanjun@huawei.com; xiexiuqi@huawei.com; wangkefeng.wang@huawei.com; huangdaode@huawei.com
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) <lihuisong@huawei.com>
> Sent: Sunday, April 9, 2023 9:27 PM
> To: Sudeep Holla <sudeep.holla@arm.com>; Robbie King 
> <robbiek@xsightlabs.com>
> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; 
> rafael@kernel.org; rafael.j.wysocki@intel.com; 
> wanghuiqiang@huawei.com; zhangzekun11@huawei.com; 
> wangxiongfeng2@huawei.com; tanxiaofei@huawei.com; 
> guohanjun@huawei.com; xiexiuqi@huawei.com; wangkefeng.wang@huawei.com; 
> huangdaode@huawei.com
> 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 
>>> <robbiek@xsightlabs.com>
>>>
>>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>> 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) <lihuisong@huawei.com>
> Sent: Thursday, April 13, 2023 9:05 PM
> To: Robbie King <robbiek@xsightlabs.com>; Sudeep Holla <sudeep.holla@arm.com>
> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org; rafael.j.wysocki@intel.com; wanghuiqiang@huawei.com; zhangzekun11@huawei.com; wangxiongfeng2@huawei.com; tanxiaofei@huawei.com; guohanjun@huawei.com; xiexiuqi@huawei.com; wangkefeng.wang@huawei.com; huangdaode@huawei.com
> 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) <lihuisong@huawei.com>
>> Sent: Sunday, April 9, 2023 9:27 PM
>> To: Sudeep Holla <sudeep.holla@arm.com>; Robbie King
>> <robbiek@xsightlabs.com>
>> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
>> rafael@kernel.org; rafael.j.wysocki@intel.com;
>> wanghuiqiang@huawei.com; zhangzekun11@huawei.com;
>> wangxiongfeng2@huawei.com; tanxiaofei@huawei.com;
>> guohanjun@huawei.com; xiexiuqi@huawei.com; wangkefeng.wang@huawei.com;
>> huangdaode@huawei.com
>> 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
>>>> <robbiek@xsightlabs.com>
>>>>
>>>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>>> 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.
  

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

在 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 ?
  

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 <sudeep.holla@arm.com>
  

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 ?
>
  

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.
  

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 <guohanjun@huawei.com>
  

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

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,

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

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