| Message ID | 20230216063653.1995-2-lihuisong@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | mailbox: pcc: Support platform notification for type4 and shared interrupt | expand |
On Thu, Feb 16, 2023 at 02:36:52PM +0800, Huisong Li wrote: > Currently, PCC driver doesn't support the processing of platform > notification for slave PCC subspaces because of the incomplete > communication flow. > > According to ACPI specification, if platform sends a notification > to OSPM, it must clear the command complete bit and trigger platform > interrupt. OSPM needs to check whether the command complete bit is > cleared, clear platform interrupt, process command, and then set the > command complete and ring doorbell to Platform. But the current judgment > on the command complete is not applicable to type4 in pcc_mbox_irq(). > > This patch determines whether the PCC driver needs to respond to the > interrupt of the channel with the master or slave subspace based on > the command complete register. And PCC driver needs to add the phase > of setting the command complete and ring doorbell in pcc_mbox_irq() > to complete type4 communication flow after processing command from > Platform. > I would prefer to reword this a little bit: " mailbox: pcc: Add support for platform notification handling Currently, PCC driver doesn't support the processing of platform notification for type 4 PCC subspaces. According to ACPI specification, if platform sends a notification to OSPM, it must clear the command complete bit and trigger platform interrupt. OSPM needs to check whether the command complete bit is cleared, clear platform interrupt, process command, and then set the command complete and ring doorbell to the Platform. Let us stash the value of the pcc type and use the same while processing the interrupt of the channel. We also need to set the command complete bit and ring doorbell in the interrupt handler for the type 4 channel to complete the communication flow after processing the notification from the Platform. " > Signed-off-by: Huisong Li <lihuisong@huawei.com> > --- > drivers/mailbox/pcc.c | 57 ++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 51 insertions(+), 6 deletions(-) > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > index 105d46c9801b..ecd54f049de3 100644 > --- a/drivers/mailbox/pcc.c > +++ b/drivers/mailbox/pcc.c > @@ -91,6 +91,7 @@ struct pcc_chan_reg { > * @cmd_update: PCC register bundle for the command complete update register > * @error: PCC register bundle for the error status register > * @plat_irq: platform interrupt > + * @type: PCC subspace type > */ > struct pcc_chan_info { > struct pcc_mbox_chan chan; > @@ -100,12 +101,15 @@ struct pcc_chan_info { > struct pcc_chan_reg cmd_update; > struct pcc_chan_reg error; > int plat_irq; > + u8 type; > }; > > #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan) > static struct pcc_chan_info *chan_info; > static int pcc_chan_count; > > +static int pcc_send_data(struct mbox_chan *chan, void *data); > + > /* > * PCC can be used with perf critical drivers such as CPPC > * So it makes sense to locally cache the virtual address and > @@ -221,6 +225,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags) > return acpi_register_gsi(NULL, interrupt, trigger, polarity); > } > > +static bool pcc_chan_command_complete(struct pcc_chan_info *pchan, > + u64 cmd_complete_reg_val) > +{ > + bool complete; > + > + if (!pchan->cmd_complete.gas) > + return true; > + > + cmd_complete_reg_val &= pchan->cmd_complete.status_mask; > + > + switch (pchan->type) { > + case ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE: > + /* > + * If this channel with the PCC master subspace is in use, > + * the command complete bit is 1 indicates that the executing > + * command has been completed by Platform and OSPM needs to > + * process the response. > + */ > + complete = cmd_complete_reg_val != 0; > + break; > + case ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE: > + /* > + * If this channel with the PCC slave subspace is in use, > + * the command complete bit is 0 indicates that Platform is > + * sending a notification and OSPM needs to response the > + * interrupt to process this command. > + */ > + complete = cmd_complete_reg_val == 0; > + break; > + default: > + complete = true; > + break; > + } > + > + return complete; > +} > + > /** > * pcc_mbox_irq - PCC mailbox interrupt handler > * @irq: interrupt number > @@ -240,12 +281,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); > if (ret) > return IRQ_NONE; > - > - if (val) { /* Ensure GAS exists and value is non-zero */ > - val &= pchan->cmd_complete.status_mask; > - if (!val) > - return IRQ_NONE; > - } > + if (!pcc_chan_command_complete(pchan, val)) > + return IRQ_NONE; > Can we simplify the above 2 chunks like below ? Does that work for you ? I see we already check for presence of complete complete check and update registers for type3/4, we can avoid it I think. Let me know if you spot any issue with this. Regards, Sudeep --->8 @@ -221,6 +225,28 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags) return acpi_register_gsi(NULL, interrupt, trigger, polarity); } +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) +{ + u64 val; + int ret; + + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); + if (ret) + return false; + + val &= pchan->cmd_complete.status_mask; + + /* + * If this is PCC slave subspace channel, then the command complete + * bit 0 indicates that Platform is sending a notification and OSPM + * needs to respond this interrupt to process this command. + */ + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) + return !val; + else + return !!val; +} + /** * pcc_mbox_irq - PCC mailbox interrupt handler * @irq: interrupt number @@ -237,16 +263,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) pchan = chan->con_priv; - ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); - if (ret) + if (!pcc_mbox_cmd_complete_check(pchan)) return IRQ_NONE; - if (val) { /* Ensure GAS exists and value is non-zero */ - val &= pchan->cmd_complete.status_mask; - if (!val) - return IRQ_NONE; - } - ret = pcc_chan_reg_read(&pchan->error, &val); if (ret)
在 2023/3/1 21:24, Sudeep Holla 写道: > On Thu, Feb 16, 2023 at 02:36:52PM +0800, Huisong Li wrote: >> Currently, PCC driver doesn't support the processing of platform >> notification for slave PCC subspaces because of the incomplete >> communication flow. >> >> According to ACPI specification, if platform sends a notification >> to OSPM, it must clear the command complete bit and trigger platform >> interrupt. OSPM needs to check whether the command complete bit is >> cleared, clear platform interrupt, process command, and then set the >> command complete and ring doorbell to Platform. But the current judgment >> on the command complete is not applicable to type4 in pcc_mbox_irq(). >> >> This patch determines whether the PCC driver needs to respond to the >> interrupt of the channel with the master or slave subspace based on >> the command complete register. And PCC driver needs to add the phase >> of setting the command complete and ring doorbell in pcc_mbox_irq() >> to complete type4 communication flow after processing command from >> Platform. >> > I would prefer to reword this a little bit: > > " > mailbox: pcc: Add support for platform notification handling > > Currently, PCC driver doesn't support the processing of platform > notification for type 4 PCC subspaces. > > According to ACPI specification, if platform sends a notification > to OSPM, it must clear the command complete bit and trigger platform > interrupt. OSPM needs to check whether the command complete bit is > cleared, clear platform interrupt, process command, and then set the > command complete and ring doorbell to the Platform. > > Let us stash the value of the pcc type and use the same while processing > the interrupt of the channel. We also need to set the command complete > bit and ring doorbell in the interrupt handler for the type 4 channel to > complete the communication flow after processing the notification from > the Platform. > " Thanks for your rewording. >> Signed-off-by: Huisong Li <lihuisong@huawei.com> >> --- >> drivers/mailbox/pcc.c | 57 ++++++++++++++++++++++++++++++++++++++----- >> 1 file changed, 51 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c >> index 105d46c9801b..ecd54f049de3 100644 >> --- a/drivers/mailbox/pcc.c >> +++ b/drivers/mailbox/pcc.c >> @@ -91,6 +91,7 @@ struct pcc_chan_reg { >> * @cmd_update: PCC register bundle for the command complete update register >> * @error: PCC register bundle for the error status register >> * @plat_irq: platform interrupt >> + * @type: PCC subspace type >> */ >> struct pcc_chan_info { >> struct pcc_mbox_chan chan; >> @@ -100,12 +101,15 @@ struct pcc_chan_info { >> struct pcc_chan_reg cmd_update; >> struct pcc_chan_reg error; >> int plat_irq; >> + u8 type; >> }; >> >> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan) >> static struct pcc_chan_info *chan_info; >> static int pcc_chan_count; >> >> +static int pcc_send_data(struct mbox_chan *chan, void *data); >> + >> /* >> * PCC can be used with perf critical drivers such as CPPC >> * So it makes sense to locally cache the virtual address and >> @@ -221,6 +225,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags) >> return acpi_register_gsi(NULL, interrupt, trigger, polarity); >> } >> >> +static bool pcc_chan_command_complete(struct pcc_chan_info *pchan, >> + u64 cmd_complete_reg_val) >> +{ >> + bool complete; >> + >> + if (!pchan->cmd_complete.gas) >> + return true; >> + >> + cmd_complete_reg_val &= pchan->cmd_complete.status_mask; >> + >> + switch (pchan->type) { >> + case ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE: >> + /* >> + * If this channel with the PCC master subspace is in use, >> + * the command complete bit is 1 indicates that the executing >> + * command has been completed by Platform and OSPM needs to >> + * process the response. >> + */ >> + complete = cmd_complete_reg_val != 0; >> + break; >> + case ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE: >> + /* >> + * If this channel with the PCC slave subspace is in use, >> + * the command complete bit is 0 indicates that Platform is >> + * sending a notification and OSPM needs to response the >> + * interrupt to process this command. >> + */ >> + complete = cmd_complete_reg_val == 0; >> + break; >> + default: >> + complete = true; >> + break; >> + } >> + >> + return complete; >> +} >> + >> /** >> * pcc_mbox_irq - PCC mailbox interrupt handler >> * @irq: interrupt number >> @@ -240,12 +281,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) >> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); >> if (ret) >> return IRQ_NONE; >> - >> - if (val) { /* Ensure GAS exists and value is non-zero */ >> - val &= pchan->cmd_complete.status_mask; >> - if (!val) >> - return IRQ_NONE; >> - } >> + if (!pcc_chan_command_complete(pchan, val)) >> + return IRQ_NONE; >> > Can we simplify the above 2 chunks like below ? Does that work for you ? > I see we already check for presence of complete complete check and update > registers for type3/4, we can avoid it I think. Let me know if you spot > any issue with this. Agree using a function to read cmd_complete and check it. But it has a probelm, details as below. > > Regards, > Sudeep > > --->8 > > @@ -221,6 +225,28 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags) > return acpi_register_gsi(NULL, interrupt, trigger, polarity); > } > > +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) > +{ > + u64 val; > + int ret; > + > + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); > + if (ret) > + return false; > + we indeed already check if cmd_complete register is exist. IMO, it can simply the code logic and reduce the risk of problems if we return true here for the type without this register. what do you think? > + val &= pchan->cmd_complete.status_mask; > + > + /* > + * If this is PCC slave subspace channel, then the command complete > + * bit 0 indicates that Platform is sending a notification and OSPM > + * needs to respond this interrupt to process this command. > + */ > + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) > + return !val; > + else > + return !!val; This else branch is not applicable to type 3. type 3 will cannot respond interrupt. > +} > + > /** > * pcc_mbox_irq - PCC mailbox interrupt handler > * @irq: interrupt number > @@ -237,16 +263,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > > pchan = chan->con_priv; > > - ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); > - if (ret) > + if (!pcc_mbox_cmd_complete_check(pchan)) > return IRQ_NONE; > > - if (val) { /* Ensure GAS exists and value is non-zero */ > - val &= pchan->cmd_complete.status_mask; > - if (!val) > - return IRQ_NONE; > - } > - > ret = pcc_chan_reg_read(&pchan->error, &val); > if (ret) > > > .
On Thu, Mar 02, 2023 at 09:57:35AM +0800, lihuisong (C) wrote: > > 在 2023/3/1 21:24, Sudeep Holla 写道: [...] > > > > +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) > > +{ > > + u64 val; > > + int ret; > > + > > + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); > > + if (ret) > > + return false; > > + > > we indeed already check if cmd_complete register is exist. > IMO, it can simply the code logic and reduce the risk of problems if we > return true here for the type without this register. > what do you think? > IIUC, your concern is about returning true for type 4 when the register doesn't exist, right ? I am saying it won't happen as we bail out if there is no GAS register from pcc_chan_reg_init(). Or am I missing something here ? > > + val &= pchan->cmd_complete.status_mask; > > + > > + /* > > + * If this is PCC slave subspace channel, then the command complete > > + * bit 0 indicates that Platform is sending a notification and OSPM > > + * needs to respond this interrupt to process this command. > > + */ > > + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) > > + return !val; > > + else > > + return !!val; > This else branch is not applicable to type 3. type 3 will cannot respond > interrupt. Sorry I don't understand what you mean by that.
在 2023/3/2 21:52, Sudeep Holla 写道: > On Thu, Mar 02, 2023 at 09:57:35AM +0800, lihuisong (C) wrote: >> 在 2023/3/1 21:24, Sudeep Holla 写道: > [...] > >>> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) >>> +{ >>> + u64 val; >>> + int ret; >>> + >>> + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); >>> + if (ret) >>> + return false; >>> + >> we indeed already check if cmd_complete register is exist. >> IMO, it can simply the code logic and reduce the risk of problems if we >> return true here for the type without this register. >> what do you think? >> > IIUC, your concern is about returning true for type 4 when the register > doesn't exist, right ? Return true in advance for the type without the cmd_complete register. If support the register, we judge if the channel should respond the interrupt based on the value of cmd_complete, like bellow. -->8 +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) +{ + u64 val; + int ret; + + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); + if (ret) + return false; + + if (!pchan->cmd_complete.gas) + return true; + + /* + * Judge if the channel respond the interrupt based on the value of + * command complete. + */ + val &= pchan->cmd_complete.status_mask; + /* + * If this is PCC slave subspace channel, then the command complete + * bit 0 indicates that Platform is sending a notification and OSPM + * needs to respond this interrupt to process this command. + */ + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) + return !val; + else + return !!val; +} > I am saying it won't happen as we bail out if there is no GAS register > from pcc_chan_reg_init(). Or am I missing something here ? Yes, what you say is also ok. Just wondering if it is better to simply the logic. >>> + val &= pchan->cmd_complete.status_mask; >>> + >>> + /* >>> + * If this is PCC slave subspace channel, then the command complete >>> + * bit 0 indicates that Platform is sending a notification and OSPM >>> + * needs to respond this interrupt to process this command. >>> + */ >>> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) >>> + return !val; >>> + else >>> + return !!val; >> This else branch is not applicable to type 3. type 3 will cannot respond >> interrupt. > Sorry I don't understand what you mean by that. Sorry for my mistake. I meant that the type2 channel always return false in this function and never respond the interrupt if no check for the GAS register. Because the 'val' for the type without the register is zero. >
On Fri, Mar 03, 2023 at 09:50:00AM +0800, lihuisong (C) wrote: > > 在 2023/3/2 21:52, Sudeep Holla 写道: > > On Thu, Mar 02, 2023 at 09:57:35AM +0800, lihuisong (C) wrote: > > > 在 2023/3/1 21:24, Sudeep Holla 写道: > > [...] > > > > > > +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) > > > > +{ > > > > + u64 val; > > > > + int ret; > > > > + > > > > + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); > > > > + if (ret) > > > > + return false; > > > > + > > > we indeed already check if cmd_complete register is exist. > > > IMO, it can simply the code logic and reduce the risk of problems if we > > > return true here for the type without this register. > > > what do you think? > > > > > IIUC, your concern is about returning true for type 4 when the register > > doesn't exist, right ? > Return true in advance for the type without the cmd_complete register. > If support the register, we judge if the channel should respond the > interrupt based on the value of cmd_complete, like bellow. Right, sorry for missing that. > > -->8 > +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) > +{ > + u64 val; > + int ret; > + > + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); > + if (ret) > + return false; > + > + if (!pchan->cmd_complete.gas) > + return true; > + Yes we need the above check. > + /* > + * Judge if the channel respond the interrupt based on the value of > + * command complete. > + */ > + val &= pchan->cmd_complete.status_mask; > + /* > + * If this is PCC slave subspace channel, then the command complete > + * bit 0 indicates that Platform is sending a notification and OSPM > + * needs to respond this interrupt to process this command. > + */ > + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) > + return !val; > + else > + return !!val; > +} > > I am saying it won't happen as we bail out if there is no GAS register > > from pcc_chan_reg_init(). Or am I missing something here ? > Yes, what you say is also ok. Just wondering if it is better to simply the > logic. Understood now. > > > > + val &= pchan->cmd_complete.status_mask; > > > > + > > > > + /* > > > > + * If this is PCC slave subspace channel, then the command complete > > > > + * bit 0 indicates that Platform is sending a notification and OSPM > > > > + * needs to respond this interrupt to process this command. > > > > + */ > > > > + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) > > > > + return !val; > > > > + else > > > > + return !!val; > > > This else branch is not applicable to type 3. type 3 will cannot respond > > > interrupt. > > Sorry I don't understand what you mean by that. > Sorry for my mistake. > I meant that the type2 channel always return false in this function and > never respond the interrupt if no check for the GAS register. > Because the 'val' for the type without the register is zero. > > Agreed as mentioned above, we need to bail out with true return if no GAS is found. -- Regards, Sudeep
在 2023/3/3 19:07, Sudeep Holla 写道: > On Fri, Mar 03, 2023 at 09:50:00AM +0800, lihuisong (C) wrote: >> 在 2023/3/2 21:52, Sudeep Holla 写道: >>> On Thu, Mar 02, 2023 at 09:57:35AM +0800, lihuisong (C) wrote: >>>> 在 2023/3/1 21:24, Sudeep Holla 写道: >>> [...] >>> >>>>> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) >>>>> +{ >>>>> + u64 val; >>>>> + int ret; >>>>> + >>>>> + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); >>>>> + if (ret) >>>>> + return false; >>>>> + >>>> we indeed already check if cmd_complete register is exist. >>>> IMO, it can simply the code logic and reduce the risk of problems if we >>>> return true here for the type without this register. >>>> what do you think? >>>> >>> IIUC, your concern is about returning true for type 4 when the register >>> doesn't exist, right ? >> Return true in advance for the type without the cmd_complete register. >> If support the register, we judge if the channel should respond the >> interrupt based on the value of cmd_complete, like bellow. > Right, sorry for missing that. > >> -->8 >> +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan) >> +{ >> + u64 val; >> + int ret; >> + >> + ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); >> + if (ret) >> + return false; >> + >> + if (!pchan->cmd_complete.gas) >> + return true; >> + > Yes we need the above check. > >> + /* >> + * Judge if the channel respond the interrupt based on the value of >> + * command complete. >> + */ >> + val &= pchan->cmd_complete.status_mask; >> + /* >> + * If this is PCC slave subspace channel, then the command complete >> + * bit 0 indicates that Platform is sending a notification and OSPM >> + * needs to respond this interrupt to process this command. >> + */ >> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) >> + return !val; >> + else >> + return !!val; >> +} >>> I am saying it won't happen as we bail out if there is no GAS register >>> from pcc_chan_reg_init(). Or am I missing something here ? >> Yes, what you say is also ok. Just wondering if it is better to simply the >> logic. > Understood now. > >>>>> + val &= pchan->cmd_complete.status_mask; >>>>> + >>>>> + /* >>>>> + * If this is PCC slave subspace channel, then the command complete >>>>> + * bit 0 indicates that Platform is sending a notification and OSPM >>>>> + * needs to respond this interrupt to process this command. >>>>> + */ >>>>> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) >>>>> + return !val; >>>>> + else >>>>> + return !!val; >>>> This else branch is not applicable to type 3. type 3 will cannot respond >>>> interrupt. >>> Sorry I don't understand what you mean by that. >> Sorry for my mistake. >> I meant that the type2 channel always return false in this function and >> never respond the interrupt if no check for the GAS register. >> Because the 'val' for the type without the register is zero. > Agreed as mentioned above, we need to bail out with true return if no GAS is > found. > Ok, I will fix it as mentioned above. > > .
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index 105d46c9801b..ecd54f049de3 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -91,6 +91,7 @@ struct pcc_chan_reg { * @cmd_update: PCC register bundle for the command complete update register * @error: PCC register bundle for the error status register * @plat_irq: platform interrupt + * @type: PCC subspace type */ struct pcc_chan_info { struct pcc_mbox_chan chan; @@ -100,12 +101,15 @@ struct pcc_chan_info { struct pcc_chan_reg cmd_update; struct pcc_chan_reg error; int plat_irq; + u8 type; }; #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan) static struct pcc_chan_info *chan_info; static int pcc_chan_count; +static int pcc_send_data(struct mbox_chan *chan, void *data); + /* * PCC can be used with perf critical drivers such as CPPC * So it makes sense to locally cache the virtual address and @@ -221,6 +225,43 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags) return acpi_register_gsi(NULL, interrupt, trigger, polarity); } +static bool pcc_chan_command_complete(struct pcc_chan_info *pchan, + u64 cmd_complete_reg_val) +{ + bool complete; + + if (!pchan->cmd_complete.gas) + return true; + + cmd_complete_reg_val &= pchan->cmd_complete.status_mask; + + switch (pchan->type) { + case ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE: + /* + * If this channel with the PCC master subspace is in use, + * the command complete bit is 1 indicates that the executing + * command has been completed by Platform and OSPM needs to + * process the response. + */ + complete = cmd_complete_reg_val != 0; + break; + case ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE: + /* + * If this channel with the PCC slave subspace is in use, + * the command complete bit is 0 indicates that Platform is + * sending a notification and OSPM needs to response the + * interrupt to process this command. + */ + complete = cmd_complete_reg_val == 0; + break; + default: + complete = true; + break; + } + + return complete; +} + /** * pcc_mbox_irq - PCC mailbox interrupt handler * @irq: interrupt number @@ -240,12 +281,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); if (ret) return IRQ_NONE; - - if (val) { /* Ensure GAS exists and value is non-zero */ - val &= pchan->cmd_complete.status_mask; - if (!val) - return IRQ_NONE; - } + if (!pcc_chan_command_complete(pchan, val)) + return IRQ_NONE; ret = pcc_chan_reg_read(&pchan->error, &val); if (ret) @@ -262,6 +299,13 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) mbox_chan_received_data(chan, NULL); + /* + * The PCC slave subspace channel needs to set the command complete bit + * and ring doorbell after processing message. + */ + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) + pcc_send_data(chan, NULL); + return IRQ_HANDLED; } @@ -692,6 +736,7 @@ static int pcc_mbox_probe(struct platform_device *pdev) pcc_parse_subspace_shmem(pchan, pcct_entry); + pchan->type = pcct_entry->type; pcct_entry = (struct acpi_subtable_header *) ((unsigned long) pcct_entry + pcct_entry->length); }
Currently, PCC driver doesn't support the processing of platform notification for slave PCC subspaces because of the incomplete communication flow. According to ACPI specification, if platform sends a notification to OSPM, it must clear the command complete bit and trigger platform interrupt. OSPM needs to check whether the command complete bit is cleared, clear platform interrupt, process command, and then set the command complete and ring doorbell to Platform. But the current judgment on the command complete is not applicable to type4 in pcc_mbox_irq(). This patch determines whether the PCC driver needs to respond to the interrupt of the channel with the master or slave subspace based on the command complete register. And PCC driver needs to add the phase of setting the command complete and ring doorbell in pcc_mbox_irq() to complete type4 communication flow after processing command from Platform. Signed-off-by: Huisong Li <lihuisong@huawei.com> --- drivers/mailbox/pcc.c | 57 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 6 deletions(-)