[RFC-V3,1/2] mailbox: pcc: Add processing platform notification for slave subspaces
Commit Message
Currently, PCC driver doesn't support the processing of platform
notification for slave PCC subspaces because of the incomplete
communication flow.
According to ACPI specification, if platform sends a notification
to OSPM, it must clear the command complete bit and trigger platform
interrupt. OSPM needs to check whether the command complete bit is
cleared, clear platform interrupt, process command, and then set the
command complete and ring doorbell to Platform. But the current judgment
on the command complete is not applicable to type4 in pcc_mbox_irq().
This patch introduces a communication flow direction field to detect
whether the interrupt belongs to the master or slave subspace channel.
And PCC driver needs to add the phase of setting the command complete
and ring doorbell in pcc_mbox_irq() to complete type4 communication
flow after processing command from Platform.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/mailbox/pcc.c | 77 +++++++++++++++++++++++++++++++++++++++----
1 file changed, 71 insertions(+), 6 deletions(-)
Comments
Hi Huisong,
Apologies for such a long delay.
Also I would like to hear from Robbie King who I know is playing around
with this these days 😄. At minimum if this logic works for him as well.
On Sat, Dec 03, 2022 at 05:51:49PM +0800, Huisong Li wrote:
> Currently, PCC driver doesn't support the processing of platform
> notification for slave PCC subspaces because of the incomplete
> communication flow.
>
> According to ACPI specification, if platform sends a notification
> to OSPM, it must clear the command complete bit and trigger platform
> interrupt. OSPM needs to check whether the command complete bit is
> cleared, clear platform interrupt, process command, and then set the
> command complete and ring doorbell to Platform. But the current judgment
> on the command complete is not applicable to type4 in pcc_mbox_irq().
>
> This patch introduces a communication flow direction field to detect
> whether the interrupt belongs to the master or slave subspace channel.
> And PCC driver needs to add the phase of setting the command complete
> and ring doorbell in pcc_mbox_irq() to complete type4 communication
> flow after processing command from Platform.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> drivers/mailbox/pcc.c | 77 +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 71 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 105d46c9801b..ad6d0b7d50fc 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -80,6 +80,13 @@ struct pcc_chan_reg {
> u64 status_mask;
> };
>
> +enum pcc_chan_comm_flow_dir_type {
> + PCC_ONLY_OSPM_TO_PLATFORM,
> + PCC_ONLY_PLATFORM_TO_OSPM,
> + PCC_BIDIRECTIONAL,
> + PCC_DIR_UNKNOWN,
> +};
> +
> /**
> * struct pcc_chan_info - PCC channel specific information
> *
> @@ -91,6 +98,7 @@ struct pcc_chan_reg {
> * @cmd_update: PCC register bundle for the command complete update register
> * @error: PCC register bundle for the error status register
> * @plat_irq: platform interrupt
> + * @comm_flow_dir: direction of communication flow supported by the channel
> */
> struct pcc_chan_info {
> struct pcc_mbox_chan chan;
> @@ -100,12 +108,15 @@ struct pcc_chan_info {
> struct pcc_chan_reg cmd_update;
> struct pcc_chan_reg error;
> int plat_irq;
> + u8 comm_flow_dir;
I would rather just save the 'type' as read from the PCCT. We don't know
what future types might be and just identifying them by the direction of
flow of the data, it restricts the usage of this.
> };
>
> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
> static struct pcc_chan_info *chan_info;
> static int pcc_chan_count;
>
> +static int pcc_send_data(struct mbox_chan *chan, void *data);
> +
> /*
> * PCC can be used with perf critical drivers such as CPPC
> * So it makes sense to locally cache the virtual address and
> @@ -221,6 +232,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> }
>
> +static bool pcc_chan_need_rsp_irq(struct pcc_chan_info *pchan,
> + u64 cmd_complete_reg_val)
Probably rename this as pcc_chan_command_complete or something similar.
> +{
> + bool need_rsp;
> +
> + if (!pchan->cmd_complete.gas)
> + return true;
> +
> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
> +
> + switch (pchan->comm_flow_dir) {
Use the channel type instead here.
在 2023/2/6 23:39, Sudeep Holla 写道:
> Hi Huisong,
>
> Apologies for such a long delay.
>
> Also I would like to hear from Robbie King who I know is playing around
> with this these days 😄. At minimum if this logic works for him as well.
@Robbie King,
Do you use this patchset to test your requirements?
Any other problems? Can you tell us your result?
>
> On Sat, Dec 03, 2022 at 05:51:49PM +0800, Huisong Li wrote:
>> Currently, PCC driver doesn't support the processing of platform
>> notification for slave PCC subspaces because of the incomplete
>> communication flow.
>>
>> According to ACPI specification, if platform sends a notification
>> to OSPM, it must clear the command complete bit and trigger platform
>> interrupt. OSPM needs to check whether the command complete bit is
>> cleared, clear platform interrupt, process command, and then set the
>> command complete and ring doorbell to Platform. But the current judgment
>> on the command complete is not applicable to type4 in pcc_mbox_irq().
>>
>> This patch introduces a communication flow direction field to detect
>> whether the interrupt belongs to the master or slave subspace channel.
>> And PCC driver needs to add the phase of setting the command complete
>> and ring doorbell in pcc_mbox_irq() to complete type4 communication
>> flow after processing command from Platform.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> drivers/mailbox/pcc.c | 77 +++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 71 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 105d46c9801b..ad6d0b7d50fc 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -80,6 +80,13 @@ struct pcc_chan_reg {
>> u64 status_mask;
>> };
>>
>> +enum pcc_chan_comm_flow_dir_type {
>> + PCC_ONLY_OSPM_TO_PLATFORM,
>> + PCC_ONLY_PLATFORM_TO_OSPM,
>> + PCC_BIDIRECTIONAL,
>> + PCC_DIR_UNKNOWN,
>> +};
>> +
>> /**
>> * struct pcc_chan_info - PCC channel specific information
>> *
>> @@ -91,6 +98,7 @@ struct pcc_chan_reg {
>> * @cmd_update: PCC register bundle for the command complete update register
>> * @error: PCC register bundle for the error status register
>> * @plat_irq: platform interrupt
>> + * @comm_flow_dir: direction of communication flow supported by the channel
>> */
>> struct pcc_chan_info {
>> struct pcc_mbox_chan chan;
>> @@ -100,12 +108,15 @@ struct pcc_chan_info {
>> struct pcc_chan_reg cmd_update;
>> struct pcc_chan_reg error;
>> int plat_irq;
>> + u8 comm_flow_dir;
> I would rather just save the 'type' as read from the PCCT. We don't know
> what future types might be and just identifying them by the direction of
> flow of the data, it restricts the usage of this.
Ack.
>
>> };
>>
>> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>> static struct pcc_chan_info *chan_info;
>> static int pcc_chan_count;
>>
>> +static int pcc_send_data(struct mbox_chan *chan, void *data);
>> +
>> /*
>> * PCC can be used with perf critical drivers such as CPPC
>> * So it makes sense to locally cache the virtual address and
>> @@ -221,6 +232,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>> }
>>
>> +static bool pcc_chan_need_rsp_irq(struct pcc_chan_info *pchan,
>> + u64 cmd_complete_reg_val)
> Probably rename this as pcc_chan_command_complete or something similar.
Ack
>
>> +{
>> + bool need_rsp;
>> +
>> + if (!pchan->cmd_complete.gas)
>> + return true;
>> +
>> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
>> +
>> + switch (pchan->comm_flow_dir) {
> Use the channel type instead here.
Ack
On 2/6/2023 9:27 PM, lihuisong (C) wrote:
>
> 在 2023/2/6 23:39, Sudeep Holla 写道:
>> Hi Huisong,
>>
>> Apologies for such a long delay.
>>
>> Also I would like to hear from Robbie King who I know is playing around
>> with this these days 😄. At minimum if this logic works for him as well.
>
> @Robbie King,
> Do you use this patchset to test your requirements?
> Any other problems? Can you tell us your result?
>
Sorry for the delay. I have verified the two patches continue to pass the
limited stress testing I have done with earlier change sets.
>>
>> On Sat, Dec 03, 2022 at 05:51:49PM +0800, Huisong Li wrote:
>>> Currently, PCC driver doesn't support the processing of platform
>>> notification for slave PCC subspaces because of the incomplete
>>> communication flow.
>>>
>>> According to ACPI specification, if platform sends a notification
>>> to OSPM, it must clear the command complete bit and trigger platform
>>> interrupt. OSPM needs to check whether the command complete bit is
>>> cleared, clear platform interrupt, process command, and then set the
>>> command complete and ring doorbell to Platform. But the current judgment
>>> on the command complete is not applicable to type4 in pcc_mbox_irq().
>>>
>>> This patch introduces a communication flow direction field to detect
>>> whether the interrupt belongs to the master or slave subspace channel.
>>> And PCC driver needs to add the phase of setting the command complete
>>> and ring doorbell in pcc_mbox_irq() to complete type4 communication
>>> flow after processing command from Platform.
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> ---
>>> drivers/mailbox/pcc.c | 77 +++++++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 71 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index 105d46c9801b..ad6d0b7d50fc 100644
>>> --- a/drivers/mailbox/pcc.c
>>> +++ b/drivers/mailbox/pcc.c
>>> @@ -80,6 +80,13 @@ struct pcc_chan_reg {
>>> u64 status_mask;
>>> };
>>> +enum pcc_chan_comm_flow_dir_type {
>>> + PCC_ONLY_OSPM_TO_PLATFORM,
>>> + PCC_ONLY_PLATFORM_TO_OSPM,
>>> + PCC_BIDIRECTIONAL,
>>> + PCC_DIR_UNKNOWN,
>>> +};
>>> +
>>> /**
>>> * struct pcc_chan_info - PCC channel specific information
>>> *
>>> @@ -91,6 +98,7 @@ struct pcc_chan_reg {
>>> * @cmd_update: PCC register bundle for the command complete update register
>>> * @error: PCC register bundle for the error status register
>>> * @plat_irq: platform interrupt
>>> + * @comm_flow_dir: direction of communication flow supported by the channel
>>> */
>>> struct pcc_chan_info {
>>> struct pcc_mbox_chan chan;
>>> @@ -100,12 +108,15 @@ struct pcc_chan_info {
>>> struct pcc_chan_reg cmd_update;
>>> struct pcc_chan_reg error;
>>> int plat_irq;
>>> + u8 comm_flow_dir;
>> I would rather just save the 'type' as read from the PCCT. We don't know
>> what future types might be and just identifying them by the direction of
>> flow of the data, it restricts the usage of this.
> Ack.
>>
>>> };
>>> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>>> static struct pcc_chan_info *chan_info;
>>> static int pcc_chan_count;
>>> +static int pcc_send_data(struct mbox_chan *chan, void *data);
>>> +
>>> /*
>>> * PCC can be used with perf critical drivers such as CPPC
>>> * So it makes sense to locally cache the virtual address and
>>> @@ -221,6 +232,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>>> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>>> }
>>> +static bool pcc_chan_need_rsp_irq(struct pcc_chan_info *pchan,
>>> + u64 cmd_complete_reg_val)
>> Probably rename this as pcc_chan_command_complete or something similar.
> Ack
>>
>>> +{
>>> + bool need_rsp;
>>> +
>>> + if (!pchan->cmd_complete.gas)
>>> + return true;
>>> +
>>> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
>>> +
>>> + switch (pchan->comm_flow_dir) {
>> Use the channel type instead here.
> Ack
在 2023/2/14 5:18, Robbie King 写道:
> On 2/6/2023 9:27 PM, lihuisong (C) wrote:
>> 在 2023/2/6 23:39, Sudeep Holla 写道:
>>> Hi Huisong,
>>>
>>> Apologies for such a long delay.
>>>
>>> Also I would like to hear from Robbie King who I know is playing around
>>> with this these days 😄. At minimum if this logic works for him as well.
>> @Robbie King,
>> Do you use this patchset to test your requirements?
>> Any other problems? Can you tell us your result?
>>
> Sorry for the delay. I have verified the two patches continue to pass the
> limited stress testing I have done with earlier change sets.
Thanks Robbie King.
I will send a formal patch as soon as possible.
>
>>> On Sat, Dec 03, 2022 at 05:51:49PM +0800, Huisong Li wrote:
>>>> Currently, PCC driver doesn't support the processing of platform
>>>> notification for slave PCC subspaces because of the incomplete
>>>> communication flow.
>>>>
>>>> According to ACPI specification, if platform sends a notification
>>>> to OSPM, it must clear the command complete bit and trigger platform
>>>> interrupt. OSPM needs to check whether the command complete bit is
>>>> cleared, clear platform interrupt, process command, and then set the
>>>> command complete and ring doorbell to Platform. But the current judgment
>>>> on the command complete is not applicable to type4 in pcc_mbox_irq().
>>>>
>>>> This patch introduces a communication flow direction field to detect
>>>> whether the interrupt belongs to the master or slave subspace channel.
>>>> And PCC driver needs to add the phase of setting the command complete
>>>> and ring doorbell in pcc_mbox_irq() to complete type4 communication
>>>> flow after processing command from Platform.
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> ---
>>>> drivers/mailbox/pcc.c | 77 +++++++++++++++++++++++++++++++++++++++----
>>>> 1 file changed, 71 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>>> index 105d46c9801b..ad6d0b7d50fc 100644
>>>> --- a/drivers/mailbox/pcc.c
>>>> +++ b/drivers/mailbox/pcc.c
>>>> @@ -80,6 +80,13 @@ struct pcc_chan_reg {
>>>> u64 status_mask;
>>>> };
>>>> +enum pcc_chan_comm_flow_dir_type {
>>>> + PCC_ONLY_OSPM_TO_PLATFORM,
>>>> + PCC_ONLY_PLATFORM_TO_OSPM,
>>>> + PCC_BIDIRECTIONAL,
>>>> + PCC_DIR_UNKNOWN,
>>>> +};
>>>> +
>>>> /**
>>>> * struct pcc_chan_info - PCC channel specific information
>>>> *
>>>> @@ -91,6 +98,7 @@ struct pcc_chan_reg {
>>>> * @cmd_update: PCC register bundle for the command complete update register
>>>> * @error: PCC register bundle for the error status register
>>>> * @plat_irq: platform interrupt
>>>> + * @comm_flow_dir: direction of communication flow supported by the channel
>>>> */
>>>> struct pcc_chan_info {
>>>> struct pcc_mbox_chan chan;
>>>> @@ -100,12 +108,15 @@ struct pcc_chan_info {
>>>> struct pcc_chan_reg cmd_update;
>>>> struct pcc_chan_reg error;
>>>> int plat_irq;
>>>> + u8 comm_flow_dir;
>>> I would rather just save the 'type' as read from the PCCT. We don't know
>>> what future types might be and just identifying them by the direction of
>>> flow of the data, it restricts the usage of this.
>> Ack.
>>>> };
>>>> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
>>>> static struct pcc_chan_info *chan_info;
>>>> static int pcc_chan_count;
>>>> +static int pcc_send_data(struct mbox_chan *chan, void *data);
>>>> +
>>>> /*
>>>> * PCC can be used with perf critical drivers such as CPPC
>>>> * So it makes sense to locally cache the virtual address and
>>>> @@ -221,6 +232,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>>>> return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>>>> }
>>>> +static bool pcc_chan_need_rsp_irq(struct pcc_chan_info *pchan,
>>>> + u64 cmd_complete_reg_val)
>>> Probably rename this as pcc_chan_command_complete or something similar.
>> Ack
>>>> +{
>>>> + bool need_rsp;
>>>> +
>>>> + if (!pchan->cmd_complete.gas)
>>>> + return true;
>>>> +
>>>> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
>>>> +
>>>> + switch (pchan->comm_flow_dir) {
>>> Use the channel type instead here.
>> Ack
> .
@@ -80,6 +80,13 @@ struct pcc_chan_reg {
u64 status_mask;
};
+enum pcc_chan_comm_flow_dir_type {
+ PCC_ONLY_OSPM_TO_PLATFORM,
+ PCC_ONLY_PLATFORM_TO_OSPM,
+ PCC_BIDIRECTIONAL,
+ PCC_DIR_UNKNOWN,
+};
+
/**
* struct pcc_chan_info - PCC channel specific information
*
@@ -91,6 +98,7 @@ struct pcc_chan_reg {
* @cmd_update: PCC register bundle for the command complete update register
* @error: PCC register bundle for the error status register
* @plat_irq: platform interrupt
+ * @comm_flow_dir: direction of communication flow supported by the channel
*/
struct pcc_chan_info {
struct pcc_mbox_chan chan;
@@ -100,12 +108,15 @@ struct pcc_chan_info {
struct pcc_chan_reg cmd_update;
struct pcc_chan_reg error;
int plat_irq;
+ u8 comm_flow_dir;
};
#define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan)
static struct pcc_chan_info *chan_info;
static int pcc_chan_count;
+static int pcc_send_data(struct mbox_chan *chan, void *data);
+
/*
* PCC can be used with perf critical drivers such as CPPC
* So it makes sense to locally cache the virtual address and
@@ -221,6 +232,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
return acpi_register_gsi(NULL, interrupt, trigger, polarity);
}
+static bool pcc_chan_need_rsp_irq(struct pcc_chan_info *pchan,
+ u64 cmd_complete_reg_val)
+{
+ bool need_rsp;
+
+ if (!pchan->cmd_complete.gas)
+ return true;
+
+ cmd_complete_reg_val &= pchan->cmd_complete.status_mask;
+
+ switch (pchan->comm_flow_dir) {
+ case PCC_ONLY_OSPM_TO_PLATFORM:
+ /*
+ * For the communication flow from OSPM to Platform, if this
+ * channel is in use, command complete bit is 1 indicates that
+ * the executing command has been completed by Platform and OSPM
+ * needs to process response.
+ */
+ need_rsp = cmd_complete_reg_val != 0;
+ break;
+ case PCC_ONLY_PLATFORM_TO_OSPM:
+ /*
+ * For the communication flow from Platform to OSPM, if this
+ * channel is in use, command complete bit is 0 indicates that
+ * Platform is sending a notification and OSPM needs to response
+ * the interrupt to process this command.
+ */
+ need_rsp = cmd_complete_reg_val == 0;
+ break;
+ default:
+ need_rsp = true;
+ break;
+ }
+
+ return need_rsp;
+}
+
/**
* pcc_mbox_irq - PCC mailbox interrupt handler
* @irq: interrupt number
@@ -240,12 +288,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
if (ret)
return IRQ_NONE;
-
- if (val) { /* Ensure GAS exists and value is non-zero */
- val &= pchan->cmd_complete.status_mask;
- if (!val)
- return IRQ_NONE;
- }
+ if (!pcc_chan_need_rsp_irq(pchan, val))
+ return IRQ_NONE;
ret = pcc_chan_reg_read(&pchan->error, &val);
if (ret)
@@ -262,6 +306,14 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
mbox_chan_received_data(chan, NULL);
+ /*
+ * For communication flow from Platform to OSPM (like, slave subspace),
+ * need to set the command complete bit and ring doorbell after
+ * processing message.
+ */
+ if (pchan->comm_flow_dir == PCC_ONLY_PLATFORM_TO_OSPM)
+ pcc_send_data(chan, NULL);
+
return IRQ_HANDLED;
}
@@ -613,6 +665,18 @@ static int __init acpi_pcc_probe(void)
return rc;
}
+static void pcc_set_chan_comm_flow_dir(struct pcc_chan_info *pchan, u8 type)
+{
+ if (type <= ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)
+ pchan->comm_flow_dir = PCC_BIDIRECTIONAL;
+ else if (type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE)
+ pchan->comm_flow_dir = PCC_ONLY_OSPM_TO_PLATFORM;
+ else if (type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+ pchan->comm_flow_dir = PCC_ONLY_PLATFORM_TO_OSPM;
+ else
+ pchan->comm_flow_dir = PCC_DIR_UNKNOWN;
+}
+
/**
* pcc_mbox_probe - Called when we find a match for the
* PCCT platform device. This is purely used to represent
@@ -686,6 +750,7 @@ static int pcc_mbox_probe(struct platform_device *pdev)
if (rc < 0)
goto err;
}
+ pcc_set_chan_comm_flow_dir(pchan, pcct_entry->type);
rc = pcc_parse_subspace_db_reg(pchan, pcct_entry);
if (rc < 0)
goto err;