diff mbox series

[v2,1/2] mailbox: pcc: Add support for platform notification handling

Message ID 20230314111135.16520-2-lihuisong@huawei.com
State Superseded
Headers show
Series mailbox: pcc: Support platform notification for type4 and shared interrupt | expand

Commit Message

lihuisong (C) March 14, 2023, 11:11 a.m. UTC
Currently, PCC driver doesn't support the processing of platform
notification for type 4 PCC subspaces.

According to ACPI specification, if platform sends a notification
to OSPM, it must clear the command complete bit and trigger platform
interrupt. OSPM needs to check whether the command complete bit is
cleared, clear platform interrupt, process command, and then set the
command complete and ring doorbell to the Platform.

Let us stash the value of the pcc type and use the same while processing
the interrupt of the channel. We also need to set the command complete
bit and ring doorbell in the interrupt handler for the type 4 channel to
complete the communication flow after processing the notification from
the Platform.

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

Comments

Sudeep Holla March 27, 2023, 11:30 a.m. UTC | #1
On Tue, Mar 14, 2023 at 07:11:34PM +0800, Huisong Li wrote:
> Currently, PCC driver doesn't support the processing of platform
> notification for type 4 PCC subspaces.
> 
> According to ACPI specification, if platform sends a notification
> to OSPM, it must clear the command complete bit and trigger platform
> interrupt. OSPM needs to check whether the command complete bit is
> cleared, clear platform interrupt, process command, and then set the
> command complete and ring doorbell to the Platform.
> 
> Let us stash the value of the pcc type and use the same while processing
> the interrupt of the channel. We also need to set the command complete
> bit and ring doorbell in the interrupt handler for the type 4 channel to
> complete the communication flow after processing the notification from
> the Platform.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  drivers/mailbox/pcc.c | 50 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 105d46c9801b..a0a87c480d8b 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -91,6 +91,7 @@ struct pcc_chan_reg {
>   * @cmd_update: PCC register bundle for the command complete update register
>   * @error: PCC register bundle for the error status register
>   * @plat_irq: platform interrupt
> + * @type: PCC subspace type
>   */
>  struct pcc_chan_info {
>  	struct pcc_mbox_chan chan;
> @@ -100,12 +101,15 @@ struct pcc_chan_info {
>  	struct pcc_chan_reg cmd_update;
>  	struct pcc_chan_reg error;
>  	int plat_irq;
> +	u8 type;
>  };
>  
>  #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>  static struct pcc_chan_info *chan_info;
>  static int pcc_chan_count;
>  
> +static int pcc_send_data(struct mbox_chan *chan, void *data);
> +
>  /*
>   * PCC can be used with perf critical drivers such as CPPC
>   * So it makes sense to locally cache the virtual address and
> @@ -221,6 +225,34 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>  	return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>  }
>  
> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
> +{
> +	u64 val;
> +	int ret;
> +
> +	ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> +	if (ret)
> +		return false;
> +
> +	if (!pchan->cmd_complete.gas)
> +		return true;
> +
> +	/*
> +	 * Judge if the channel respond the interrupt based on the value of
> +	 * command complete.
> +	 */
> +	val &= pchan->cmd_complete.status_mask;

[super nit] Would prefer an blank line here.

--
Regards,
Sudeep
lihuisong (C) March 27, 2023, 12:25 p.m. UTC | #2
在 2023/3/27 19:30, Sudeep Holla 写道:
> On Tue, Mar 14, 2023 at 07:11:34PM +0800, Huisong Li wrote:
>> Currently, PCC driver doesn't support the processing of platform
>> notification for type 4 PCC subspaces.
>>
>> According to ACPI specification, if platform sends a notification
>> to OSPM, it must clear the command complete bit and trigger platform
>> interrupt. OSPM needs to check whether the command complete bit is
>> cleared, clear platform interrupt, process command, and then set the
>> command complete and ring doorbell to the Platform.
>>
>> Let us stash the value of the pcc type and use the same while processing
>> the interrupt of the channel. We also need to set the command complete
>> bit and ring doorbell in the interrupt handler for the type 4 channel to
>> complete the communication flow after processing the notification from
>> the Platform.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/mailbox/pcc.c | 50 +++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 41 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 105d46c9801b..a0a87c480d8b 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -91,6 +91,7 @@ struct pcc_chan_reg {
>>    * @cmd_update: PCC register bundle for the command complete update register
>>    * @error: PCC register bundle for the error status register
>>    * @plat_irq: platform interrupt
>> + * @type: PCC subspace type
>>    */
>>   struct pcc_chan_info {
>>   	struct pcc_mbox_chan chan;
>> @@ -100,12 +101,15 @@ struct pcc_chan_info {
>>   	struct pcc_chan_reg cmd_update;
>>   	struct pcc_chan_reg error;
>>   	int plat_irq;
>> +	u8 type;
>>   };
>>   
>>   #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>>   static struct pcc_chan_info *chan_info;
>>   static int pcc_chan_count;
>>   
>> +static int pcc_send_data(struct mbox_chan *chan, void *data);
>> +
>>   /*
>>    * PCC can be used with perf critical drivers such as CPPC
>>    * So it makes sense to locally cache the virtual address and
>> @@ -221,6 +225,34 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>>   	return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>>   }
>>   
>> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
>> +{
>> +	u64 val;
>> +	int ret;
>> +
>> +	ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
>> +	if (ret)
>> +		return false;
>> +
>> +	if (!pchan->cmd_complete.gas)
>> +		return true;
>> +
>> +	/*
>> +	 * Judge if the channel respond the interrupt based on the value of
>> +	 * command complete.
>> +	 */
>> +	val &= pchan->cmd_complete.status_mask;
> [super nit] Would prefer an blank line here.
>
Yeah, it would be better if there is an blank line here.
Is it necessary to send v3 for this?
Sudeep Holla March 27, 2023, 1:27 p.m. UTC | #3
On Mon, Mar 27, 2023 at 08:25:04PM +0800, lihuisong (C) wrote:
> 
> 在 2023/3/27 19:30, Sudeep Holla 写道:
> > On Tue, Mar 14, 2023 at 07:11:34PM +0800, Huisong Li wrote:
> > > Currently, PCC driver doesn't support the processing of platform
> > > notification for type 4 PCC subspaces.
> > > 
> > > According to ACPI specification, if platform sends a notification
> > > to OSPM, it must clear the command complete bit and trigger platform
> > > interrupt. OSPM needs to check whether the command complete bit is
> > > cleared, clear platform interrupt, process command, and then set the
> > > command complete and ring doorbell to the Platform.
> > > 
> > > Let us stash the value of the pcc type and use the same while processing
> > > the interrupt of the channel. We also need to set the command complete
> > > bit and ring doorbell in the interrupt handler for the type 4 channel to
> > > complete the communication flow after processing the notification from
> > > the Platform.
> > > 
> > > Signed-off-by: Huisong Li <lihuisong@huawei.com>
> > > ---
> > >   drivers/mailbox/pcc.c | 50 +++++++++++++++++++++++++++++++++++--------
> > >   1 file changed, 41 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > > index 105d46c9801b..a0a87c480d8b 100644
> > > --- a/drivers/mailbox/pcc.c
> > > +++ b/drivers/mailbox/pcc.c
> > > @@ -91,6 +91,7 @@ struct pcc_chan_reg {
> > >    * @cmd_update: PCC register bundle for the command complete update register
> > >    * @error: PCC register bundle for the error status register
> > >    * @plat_irq: platform interrupt
> > > + * @type: PCC subspace type
> > >    */
> > >   struct pcc_chan_info {
> > >   	struct pcc_mbox_chan chan;
> > > @@ -100,12 +101,15 @@ struct pcc_chan_info {
> > >   	struct pcc_chan_reg cmd_update;
> > >   	struct pcc_chan_reg error;
> > >   	int plat_irq;
> > > +	u8 type;
> > >   };
> > >   #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
> > >   static struct pcc_chan_info *chan_info;
> > >   static int pcc_chan_count;
> > > +static int pcc_send_data(struct mbox_chan *chan, void *data);
> > > +
> > >   /*
> > >    * PCC can be used with perf critical drivers such as CPPC
> > >    * So it makes sense to locally cache the virtual address and
> > > @@ -221,6 +225,34 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
> > >   	return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> > >   }
> > > +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
> > > +{
> > > +	u64 val;
> > > +	int ret;
> > > +
> > > +	ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> > > +	if (ret)
> > > +		return false;
> > > +
> > > +	if (!pchan->cmd_complete.gas)
> > > +		return true;
> > > +
> > > +	/*
> > > +	 * Judge if the channel respond the interrupt based on the value of
> > > +	 * command complete.
> > > +	 */
> > > +	val &= pchan->cmd_complete.status_mask;
> > [super nit] Would prefer an blank line here.
> > 
> Yeah, it would be better if there is an blank line here.
> Is it necessary to send v3 for this?

Let us see once you have Robbie's tested-by. Depending on who picks up we can
check with them.
diff mbox series

Patch

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 105d46c9801b..a0a87c480d8b 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -91,6 +91,7 @@  struct pcc_chan_reg {
  * @cmd_update: PCC register bundle for the command complete update register
  * @error: PCC register bundle for the error status register
  * @plat_irq: platform interrupt
+ * @type: PCC subspace type
  */
 struct pcc_chan_info {
 	struct pcc_mbox_chan chan;
@@ -100,12 +101,15 @@  struct pcc_chan_info {
 	struct pcc_chan_reg cmd_update;
 	struct pcc_chan_reg error;
 	int plat_irq;
+	u8 type;
 };
 
 #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
 static struct pcc_chan_info *chan_info;
 static int pcc_chan_count;
 
+static int pcc_send_data(struct mbox_chan *chan, void *data);
+
 /*
  * PCC can be used with perf critical drivers such as CPPC
  * So it makes sense to locally cache the virtual address and
@@ -221,6 +225,34 @@  static int pcc_map_interrupt(u32 interrupt, u32 flags)
 	return acpi_register_gsi(NULL, interrupt, trigger, polarity);
 }
 
+static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
+{
+	u64 val;
+	int ret;
+
+	ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
+	if (ret)
+		return false;
+
+	if (!pchan->cmd_complete.gas)
+		return true;
+
+	/*
+	 * Judge if the channel respond the interrupt based on the value of
+	 * command complete.
+	 */
+	val &= pchan->cmd_complete.status_mask;
+	/*
+	 * If this is PCC slave subspace channel, and the command complete
+	 * bit 0 indicates that Platform is sending a notification and OSPM
+	 * needs to respond this interrupt to process this command.
+	 */
+	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+		return !val;
+
+	return !!val;
+}
+
 /**
  * pcc_mbox_irq - PCC mailbox interrupt handler
  * @irq:	interrupt number
@@ -236,17 +268,9 @@  static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	int ret;
 
 	pchan = chan->con_priv;
-
-	ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
-	if (ret)
+	if (!pcc_mbox_cmd_complete_check(pchan))
 		return IRQ_NONE;
 
-	if (val) { /* Ensure GAS exists and value is non-zero */
-		val &= pchan->cmd_complete.status_mask;
-		if (!val)
-			return IRQ_NONE;
-	}
-
 	ret = pcc_chan_reg_read(&pchan->error, &val);
 	if (ret)
 		return IRQ_NONE;
@@ -262,6 +286,13 @@  static irqreturn_t pcc_mbox_irq(int irq, void *p)
 
 	mbox_chan_received_data(chan, NULL);
 
+	/*
+	 * The PCC slave subspace channel needs to set the command complete bit
+	 * and ring doorbell after processing message.
+	 */
+	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+		pcc_send_data(chan, NULL);
+
 	return IRQ_HANDLED;
 }
 
@@ -692,6 +723,7 @@  static int pcc_mbox_probe(struct platform_device *pdev)
 
 		pcc_parse_subspace_shmem(pchan, pcct_entry);
 
+		pchan->type = pcct_entry->type;
 		pcct_entry = (struct acpi_subtable_header *)
 			((unsigned long) pcct_entry + pcct_entry->length);
 	}