| Message ID | 20221016034043.52227-1-lihuisong@huawei.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] ACPI: PCC: Support shared interrupt for multiple subspaces | expand |
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.
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.
在 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.
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.
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); > > .
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;
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(-)