| Message ID | 20230216063653.1995-1-lihuisong@huawei.com |
|---|---|
| Headers | show |
| Series | mailbox: pcc: Support platform notification for type4 and shared interrupt | expand |
Kindly ping. 在 2023/2/16 14:36, Huisong Li 写道: > PCC supports processing platform notification for slave subspaces and > shared interrupt for multiple subspaces. > > --- > -v1: using subspace type to replace comm_flow_dir in patch [1/2] > -rfc-v3: split V2 into two patches. > -rfc-v2: don't use platform interrupt ack register to identify if the given > channel should respond interrupt. > > Huisong Li (2): > mailbox: pcc: Add processing platform notification for slave subspaces > mailbox: pcc: Support shared interrupt for multiple subspaces > > drivers/mailbox/pcc.c | 102 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 93 insertions(+), 9 deletions(-) >
On Thu, Feb 16, 2023 at 02:36:53PM +0800, Huisong Li wrote: > If the platform acknowledge interrupt is level triggered, then it can > be shared by multiple subspaces provided each one has a unique platform > interrupt ack preserve and ack set masks. > > If it can be shared, then we can request the irq with IRQF_SHARED and > IRQF_ONESHOT flags. The first one indicating it can be shared and the > latter one to keep the interrupt disabled until the hardirq handler > finished. > > Further, since there is no way to detect if the interrupt is for a given > channel as the interrupt ack preserve and ack set masks are for clearing > the interrupt and not for reading the status(in case Irq Ack register > may be write-only on some platforms), we need a way to identify if the > given channel is in use and expecting the interrupt. > > PCC type0, type1 and type5 do not support shared level triggered interrupt. > The methods of determining whether a given channel for remaining types > should respond to an interrupt are as follows: > - type2: Whether the interrupt belongs to a given channel is only > determined by the status field in Generic Communications Channel > Shared Memory Region, which is done in rx_callback of PCC client. > - type3: This channel checks chan_in_use flag first and then checks the > command complete bit(value '1' indicates that the command has > been completed). > - type4: Platform ensure that the default value of the command complete > bit corresponding to the type4 channel is '1'. This command > complete bit is '0' when receive a platform notification. > > Signed-off-by: Huisong Li <lihuisong@huawei.com> > --- > drivers/mailbox/pcc.c | 45 ++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 42 insertions(+), 3 deletions(-) > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > index ecd54f049de3..04c2d73a0473 100644 > --- a/drivers/mailbox/pcc.c > +++ b/drivers/mailbox/pcc.c > @@ -92,6 +92,10 @@ struct pcc_chan_reg { > * @error: PCC register bundle for the error status register > * @plat_irq: platform interrupt > * @type: PCC subspace type > + * @plat_irq_flags: platform interrupt flags > + * @chan_in_use: flag indicating whether the channel is in use or not when use > + * platform interrupt, and only use it for communication from OSPM > + * to Platform, like type 3. Also add a node that since only one transfer can occur at a time and the mailbox takes care of locking, this flag needs no locking and is used just to check if the interrupt needs handling when it is shared. > */ > struct pcc_chan_info { > struct pcc_mbox_chan chan; > @@ -102,6 +106,8 @@ struct pcc_chan_info { > struct pcc_chan_reg error; > int plat_irq; > u8 type; > + unsigned int plat_irq_flags; > + bool chan_in_use; > }; > > #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan) > @@ -225,6 +231,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; > +} > + > static bool pcc_chan_command_complete(struct pcc_chan_info *pchan, > u64 cmd_complete_reg_val) > { > @@ -277,6 +289,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > int ret; > > pchan = chan->con_priv; > + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE && > + !pchan->chan_in_use) > + return IRQ_NONE; > > ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); > if (ret) > @@ -302,9 +317,13 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > /* > * The PCC slave subspace channel needs to set the command complete bit > * and ring doorbell after processing message. > + * > + * The PCC master subspace channel clears chan_in_use to free channel. > */ > if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) > pcc_send_data(chan, NULL); > + else if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE) > + pchan->chan_in_use = false; Just wondering if this has to be for type 3 only. I am trying to avoid conditional update of this flag, can we not do it for everything except type4 ? (I mean just in unconditional else part) > > return IRQ_HANDLED; > } > @@ -353,10 +372,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); > @@ -418,7 +440,17 @@ 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); > + /* > + * For the master subspace channel, set chan_in_use flag to true after > + * ring doorbell, and clear this flag when the reply message is > + * processed. > + */ > + if (!ret && pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE && > + pchan->plat_irq > 0) > + pchan->chan_in_use = true; Ditto here(for all type except type 4?)
在 2023/3/1 21:36, Sudeep Holla 写道: > On Thu, Feb 16, 2023 at 02:36:53PM +0800, Huisong Li wrote: >> If the platform acknowledge interrupt is level triggered, then it can >> be shared by multiple subspaces provided each one has a unique platform >> interrupt ack preserve and ack set masks. >> >> If it can be shared, then we can request the irq with IRQF_SHARED and >> IRQF_ONESHOT flags. The first one indicating it can be shared and the >> latter one to keep the interrupt disabled until the hardirq handler >> finished. >> >> Further, since there is no way to detect if the interrupt is for a given >> channel as the interrupt ack preserve and ack set masks are for clearing >> the interrupt and not for reading the status(in case Irq Ack register >> may be write-only on some platforms), we need a way to identify if the >> given channel is in use and expecting the interrupt. >> >> PCC type0, type1 and type5 do not support shared level triggered interrupt. >> The methods of determining whether a given channel for remaining types >> should respond to an interrupt are as follows: >> - type2: Whether the interrupt belongs to a given channel is only >> determined by the status field in Generic Communications Channel >> Shared Memory Region, which is done in rx_callback of PCC client. >> - type3: This channel checks chan_in_use flag first and then checks the >> command complete bit(value '1' indicates that the command has >> been completed). >> - type4: Platform ensure that the default value of the command complete >> bit corresponding to the type4 channel is '1'. This command >> complete bit is '0' when receive a platform notification. >> >> Signed-off-by: Huisong Li <lihuisong@huawei.com> >> --- >> drivers/mailbox/pcc.c | 45 ++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 42 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c >> index ecd54f049de3..04c2d73a0473 100644 >> --- a/drivers/mailbox/pcc.c >> +++ b/drivers/mailbox/pcc.c >> @@ -92,6 +92,10 @@ struct pcc_chan_reg { >> * @error: PCC register bundle for the error status register >> * @plat_irq: platform interrupt >> * @type: PCC subspace type >> + * @plat_irq_flags: platform interrupt flags >> + * @chan_in_use: flag indicating whether the channel is in use or not when use >> + * platform interrupt, and only use it for communication from OSPM >> + * to Platform, like type 3. > Also add a node that since only one transfer can occur at a time and the > mailbox takes care of locking, this flag needs no locking and is used just > to check if the interrupt needs handling when it is shared. Add a per-channel lock. Is this your mean? > >> */ >> struct pcc_chan_info { >> struct pcc_mbox_chan chan; >> @@ -102,6 +106,8 @@ struct pcc_chan_info { >> struct pcc_chan_reg error; >> int plat_irq; >> u8 type; >> + unsigned int plat_irq_flags; >> + bool chan_in_use; >> }; >> >> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan) >> @@ -225,6 +231,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; >> +} >> + >> static bool pcc_chan_command_complete(struct pcc_chan_info *pchan, >> u64 cmd_complete_reg_val) >> { >> @@ -277,6 +289,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) >> int ret; >> >> pchan = chan->con_priv; >> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE && >> + !pchan->chan_in_use) >> + return IRQ_NONE; >> >> ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); >> if (ret) >> @@ -302,9 +317,13 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) >> /* >> * The PCC slave subspace channel needs to set the command complete bit >> * and ring doorbell after processing message. >> + * >> + * The PCC master subspace channel clears chan_in_use to free channel. >> */ >> if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) >> pcc_send_data(chan, NULL); >> + else if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE) >> + pchan->chan_in_use = false; > Just wondering if this has to be for type 3 only. I am trying to avoid > conditional update of this flag, can we not do it for everything except type4 ? > (I mean just in unconditional else part) But type2 do not need this flag. For types no need this flag, it is always hard to understand and redundant design. If no this condition, we don't know what is the impact on the furture types. > >> return IRQ_HANDLED; >> } >> @@ -353,10 +372,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); >> @@ -418,7 +440,17 @@ 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); >> + /* >> + * For the master subspace channel, set chan_in_use flag to true after >> + * ring doorbell, and clear this flag when the reply message is >> + * processed. >> + */ >> + if (!ret && pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE && >> + pchan->plat_irq > 0) >> + pchan->chan_in_use = true; > Ditto here(for all type except type 4?) Above is my concern. >
On Thu, Mar 02, 2023 at 10:17:07AM +0800, lihuisong (C) wrote: > > 在 2023/3/1 21:36, Sudeep Holla 写道: > > On Thu, Feb 16, 2023 at 02:36:53PM +0800, Huisong Li wrote: > > > If the platform acknowledge interrupt is level triggered, then it can > > > be shared by multiple subspaces provided each one has a unique platform > > > interrupt ack preserve and ack set masks. > > > > > > If it can be shared, then we can request the irq with IRQF_SHARED and > > > IRQF_ONESHOT flags. The first one indicating it can be shared and the > > > latter one to keep the interrupt disabled until the hardirq handler > > > finished. > > > > > > Further, since there is no way to detect if the interrupt is for a given > > > channel as the interrupt ack preserve and ack set masks are for clearing > > > the interrupt and not for reading the status(in case Irq Ack register > > > may be write-only on some platforms), we need a way to identify if the > > > given channel is in use and expecting the interrupt. > > > > > > PCC type0, type1 and type5 do not support shared level triggered interrupt. > > > The methods of determining whether a given channel for remaining types > > > should respond to an interrupt are as follows: > > > - type2: Whether the interrupt belongs to a given channel is only > > > determined by the status field in Generic Communications Channel > > > Shared Memory Region, which is done in rx_callback of PCC client. > > > - type3: This channel checks chan_in_use flag first and then checks the > > > command complete bit(value '1' indicates that the command has > > > been completed). > > > - type4: Platform ensure that the default value of the command complete > > > bit corresponding to the type4 channel is '1'. This command > > > complete bit is '0' when receive a platform notification. > > > > > > Signed-off-by: Huisong Li <lihuisong@huawei.com> > > > --- > > > drivers/mailbox/pcc.c | 45 ++++++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 42 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > > > index ecd54f049de3..04c2d73a0473 100644 > > > --- a/drivers/mailbox/pcc.c > > > +++ b/drivers/mailbox/pcc.c > > > @@ -92,6 +92,10 @@ struct pcc_chan_reg { > > > * @error: PCC register bundle for the error status register > > > * @plat_irq: platform interrupt > > > * @type: PCC subspace type > > > + * @plat_irq_flags: platform interrupt flags > > > + * @chan_in_use: flag indicating whether the channel is in use or not when use > > > + * platform interrupt, and only use it for communication from OSPM > > > + * to Platform, like type 3. > > Also add a node that since only one transfer can occur at a time and the > > mailbox takes care of locking, this flag needs no locking and is used just > > to check if the interrupt needs handling when it is shared. > Add a per-channel lock. Is this your mean? No. I meant a comment saying it is not need since only one transfer can occur at a time and mailbox takes care of locking. So chan_in_use can be accessed without a lock. > > > > > */ > > > struct pcc_chan_info { > > > struct pcc_mbox_chan chan; > > > @@ -102,6 +106,8 @@ struct pcc_chan_info { > > > struct pcc_chan_reg error; > > > int plat_irq; > > > u8 type; > > > + unsigned int plat_irq_flags; > > > + bool chan_in_use; > > > }; > > > #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan) > > > @@ -225,6 +231,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; > > > +} > > > + > > > static bool pcc_chan_command_complete(struct pcc_chan_info *pchan, > > > u64 cmd_complete_reg_val) > > > { > > > @@ -277,6 +289,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > > > int ret; > > > pchan = chan->con_priv; > > > + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE && > > > + !pchan->chan_in_use) > > > + return IRQ_NONE; > > > ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); > > > if (ret) > > > @@ -302,9 +317,13 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > > > /* > > > * The PCC slave subspace channel needs to set the command complete bit > > > * and ring doorbell after processing message. > > > + * > > > + * The PCC master subspace channel clears chan_in_use to free channel. > > > */ > > > if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) > > > pcc_send_data(chan, NULL); > > > + else if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE) > > > + pchan->chan_in_use = false; > > Just wondering if this has to be for type 3 only. I am trying to avoid > > conditional update of this flag, can we not do it for everything except type4 ? > > (I mean just in unconditional else part) > But type2 do not need this flag. Yes > For types no need this flag, it is always hard to understand and redundant > design. But does it matter ? You can even support shared interrupt for type 1&2. They support level interrupt, so we can add them too. I understand you can test only type 3, but this driver caters for all and the code must be generic as much as possible. I don't see any point in check for type 3 only. Only type 4 is slave and operates quite opposite compared to other types and makes sense to handle it differently. > If no this condition, we don't know what is the impact on the furture types. We can add/extend the check if necessary while adding the support for that in the future. -- Regards, Sudeep
On Fri, Mar 03, 2023 at 02:33:49PM +0800, lihuisong (C) wrote: > Sorry for my resend. Because I found that my last reply email is not in the > thread of this patch. I guess it may be send failed. > > 在 2023/3/2 22:02, Sudeep Holla 写道: > > No. I meant a comment saying it is not need since only one transfer can occur > > at a time and mailbox takes care of locking. So chan_in_use can be accessed > > without a lock. > Got it. Agreed. Thanks > > > For types no need this flag, it is always hard to understand and redundant > > > design. > > But does it matter ? You can even support shared interrupt for type 1&2. > BTW, type 1 subspaces do not support a level triggered platform interrupt as > no method is provided to clear the interrupt. Agreed but there is no harm using the flag, you can add a comment that it is useful only if shared interrupts are supported. That will imply it is dummy for type 1. I am avoiding too many type unnecessary checks especially in IRQ handler. > > They support level interrupt, so we can add them too. I understand you can > > test only type 3, but this driver caters for all and the code must be generic > > as much as possible. I don't see any point in check for type 3 only. Only > I understand what you do. > But type 2 also supports the communication flow from OSPM to Platfrom. > In this case, this flag will get in the way of type 2. > How ? > Whether the interrupt belongs to a type2 channel is only determined by > the status field in Generic Communications Channel Shared Memory Region, > which is done in rx_callback of PCC client. Agreed, but do you see any issue using the flag even if it acts as dummy ? -- Regards, Sudeep
在 2023/3/3 19:14, Sudeep Holla 写道: > On Fri, Mar 03, 2023 at 02:33:49PM +0800, lihuisong (C) wrote: >> Sorry for my resend. Because I found that my last reply email is not in the >> thread of this patch. I guess it may be send failed. >> >> 在 2023/3/2 22:02, Sudeep Holla 写道: >>> No. I meant a comment saying it is not need since only one transfer can occur >>> at a time and mailbox takes care of locking. So chan_in_use can be accessed >>> without a lock. >> Got it. Agreed. > Thanks already modify this comment as below. > >>>> For types no need this flag, it is always hard to understand and redundant >>>> design. >>> But does it matter ? You can even support shared interrupt for type 1&2. >> BTW, type 1 subspaces do not support a level triggered platform interrupt as >> no method is provided to clear the interrupt. > Agreed but there is no harm using the flag, you can add a comment that it is > useful only if shared interrupts are supported. That will imply it is dummy > for type 1. I am avoiding too many type unnecessary checks especially in IRQ > handler. Understood. > >>> They support level interrupt, so we can add them too. I understand you can >>> test only type 3, but this driver caters for all and the code must be generic >>> as much as possible. I don't see any point in check for type 3 only. Only >> I understand what you do. >> But type 2 also supports the communication flow from OSPM to Platfrom. >> In this case, this flag will get in the way of type 2. >> > How ? It should be ok if all types except for type 3 do not check this flag in interrupt handle. Namely, these types consider it as dummy, and do not use it, anywhere, Right? > >> Whether the interrupt belongs to a type2 channel is only determined by >> the status field in Generic Communications Channel Shared Memory Region, >> which is done in rx_callback of PCC client. > Agreed, but do you see any issue using the flag even if it acts as dummy ? I think it can work well if these types completely ignore this flag, like below. what do you think? -->8 diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index ecd54f049de3..14405e99193d 100755 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -92,6 +92,13 @@ struct pcc_chan_reg { * @error: PCC register bundle for the error status register * @plat_irq: platform interrupt * @type: PCC subspace type + * @plat_irq_flags: platform interrupt flags + * @chan_in_use: this flag is used just to check if the interrupt needs + * handling when it is shared. Since only one transfer can occur + * at a time and mailbox takes care of locking, this flag can be + * accessed without a lock. Note: the type only support the + * communication from OSPM to Platform, like type3, use it, and + * other types completely ignore it. */ struct pcc_chan_info { struct pcc_mbox_chan chan; @@ -102,6 +109,8 @@ struct pcc_chan_info { struct pcc_chan_reg error; int plat_irq; u8 type; + unsigned int plat_irq_flags; + bool chan_in_use; }; #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan) @@ -225,6 +234,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; +} + static bool pcc_chan_command_complete(struct pcc_chan_info *pchan, u64 cmd_complete_reg_val) { @@ -277,6 +292,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) int ret; pchan = chan->con_priv; + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE && + !pchan->chan_in_use) + return IRQ_NONE; ret = pcc_chan_reg_read(&pchan->cmd_complete, &val); if (ret) @@ -302,9 +320,12 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) /* * The PCC slave subspace channel needs to set the command complete bit * and ring doorbell after processing message. + * + * The PCC master subspace channel clears chan_in_use to free channel. */ if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE) pcc_send_data(chan, NULL); + pchan->chan_in_use = false; return IRQ_HANDLED; } @@ -353,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); @@ -418,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 = { @@ -501,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; @@ -522,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; }
On Sat, Mar 04, 2023 at 05:47:28PM +0800, lihuisong (C) wrote: > > 在 2023/3/3 19:14, Sudeep Holla 写道: > > On Fri, Mar 03, 2023 at 02:33:49PM +0800, lihuisong (C) wrote: > > > Sorry for my resend. Because I found that my last reply email is not in the > > > thread of this patch. I guess it may be send failed. > > > > > > 在 2023/3/2 22:02, Sudeep Holla 写道: > > > > No. I meant a comment saying it is not need since only one transfer can occur > > > > at a time and mailbox takes care of locking. So chan_in_use can be accessed > > > > without a lock. > > > Got it. Agreed. > > Thanks > already modify this comment as below. > > > > > > > For types no need this flag, it is always hard to understand and redundant > > > > > design. > > > > But does it matter ? You can even support shared interrupt for type 1&2. > > > BTW, type 1 subspaces do not support a level triggered platform interrupt as > > > no method is provided to clear the interrupt. > > Agreed but there is no harm using the flag, you can add a comment that it is > > useful only if shared interrupts are supported. That will imply it is dummy > > for type 1. I am avoiding too many type unnecessary checks especially in IRQ > > handler. > > Understood. > > > > > > > They support level interrupt, so we can add them too. I understand you can > > > > test only type 3, but this driver caters for all and the code must be generic > > > > as much as possible. I don't see any point in check for type 3 only. Only > > > I understand what you do. > > > But type 2 also supports the communication flow from OSPM to Platfrom. > > > In this case, this flag will get in the way of type 2. > > > > > How ? > It should be ok if all types except for type 3 do not check this flag in > interrupt handle. > Namely, these types consider it as dummy, and do not use it, anywhere, > Right? > > > > > Whether the interrupt belongs to a type2 channel is only determined by > > > the status field in Generic Communications Channel Shared Memory Region, > > > which is done in rx_callback of PCC client. > > Agreed, but do you see any issue using the flag even if it acts as dummy ? > > I think it can work well if these types completely ignore this flag, like below. > what do you think? > > -->8 > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > index ecd54f049de3..14405e99193d 100755 > --- a/drivers/mailbox/pcc.c > +++ b/drivers/mailbox/pcc.c > @@ -92,6 +92,13 @@ struct pcc_chan_reg { > * @error: PCC register bundle for the error status register > * @plat_irq: platform interrupt > * @type: PCC subspace type > + * @plat_irq_flags: platform interrupt flags > + * @chan_in_use: this flag is used just to check if the interrupt needs > + * handling when it is shared. Since only one transfer can > occur > + * at a time and mailbox takes care of locking, this flag can > be > + * accessed without a lock. Note: the type only support the > + * communication from OSPM to Platform, like type3, use it, and > + * other types completely ignore it. > */ > struct pcc_chan_info { > struct pcc_mbox_chan chan; > @@ -102,6 +109,8 @@ struct pcc_chan_info { > struct pcc_chan_reg error; > int plat_irq; > u8 type; > + unsigned int plat_irq_flags; > + bool chan_in_use; > }; > > #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan) > @@ -225,6 +234,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; > +} > + > static bool pcc_chan_command_complete(struct pcc_chan_info *pchan, > u64 cmd_complete_reg_val) > { > @@ -277,6 +292,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) > int ret; > > pchan = chan->con_priv; > + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE && > + !pchan->chan_in_use) I would have avoided the type check above but I understand your concern so let us keep it like this for now. Please submit non-RFC patch as some maintainers may not look at RFC.
在 2023/3/11 4:14, Sudeep Holla 写道: > On Sat, Mar 04, 2023 at 05:47:28PM +0800, lihuisong (C) wrote: >> 在 2023/3/3 19:14, Sudeep Holla 写道: >>> On Fri, Mar 03, 2023 at 02:33:49PM +0800, lihuisong (C) wrote: >>>> Sorry for my resend. Because I found that my last reply email is not in the >>>> thread of this patch. I guess it may be send failed. >>>> >>>> 在 2023/3/2 22:02, Sudeep Holla 写道: >>>>> No. I meant a comment saying it is not need since only one transfer can occur >>>>> at a time and mailbox takes care of locking. So chan_in_use can be accessed >>>>> without a lock. >>>> Got it. Agreed. >>> Thanks >> already modify this comment as below. >>>>>> For types no need this flag, it is always hard to understand and redundant >>>>>> design. >>>>> But does it matter ? You can even support shared interrupt for type 1&2. >>>> BTW, type 1 subspaces do not support a level triggered platform interrupt as >>>> no method is provided to clear the interrupt. >>> Agreed but there is no harm using the flag, you can add a comment that it is >>> useful only if shared interrupts are supported. That will imply it is dummy >>> for type 1. I am avoiding too many type unnecessary checks especially in IRQ >>> handler. >> Understood. >> >>>>> They support level interrupt, so we can add them too. I understand you can >>>>> test only type 3, but this driver caters for all and the code must be generic >>>>> as much as possible. I don't see any point in check for type 3 only. Only >>>> I understand what you do. >>>> But type 2 also supports the communication flow from OSPM to Platfrom. >>>> In this case, this flag will get in the way of type 2. >>>> >>> How ? >> It should be ok if all types except for type 3 do not check this flag in >> interrupt handle. >> Namely, these types consider it as dummy, and do not use it, anywhere, >> Right? >>>> Whether the interrupt belongs to a type2 channel is only determined by >>>> the status field in Generic Communications Channel Shared Memory Region, >>>> which is done in rx_callback of PCC client. >>> Agreed, but do you see any issue using the flag even if it acts as dummy ? >> I think it can work well if these types completely ignore this flag, like below. >> what do you think? >> >> -->8 >> >> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c >> index ecd54f049de3..14405e99193d 100755 >> --- a/drivers/mailbox/pcc.c >> +++ b/drivers/mailbox/pcc.c >> @@ -92,6 +92,13 @@ struct pcc_chan_reg { >> * @error: PCC register bundle for the error status register >> * @plat_irq: platform interrupt >> * @type: PCC subspace type >> + * @plat_irq_flags: platform interrupt flags >> + * @chan_in_use: this flag is used just to check if the interrupt needs >> + * handling when it is shared. Since only one transfer can >> occur >> + * at a time and mailbox takes care of locking, this flag can >> be >> + * accessed without a lock. Note: the type only support the >> + * communication from OSPM to Platform, like type3, use it, and >> + * other types completely ignore it. >> */ >> struct pcc_chan_info { >> struct pcc_mbox_chan chan; >> @@ -102,6 +109,8 @@ struct pcc_chan_info { >> struct pcc_chan_reg error; >> int plat_irq; >> u8 type; >> + unsigned int plat_irq_flags; >> + bool chan_in_use; >> }; >> >> #define to_pcc_chan_info(c) container_of(c, struct pcc_chan_info, chan) >> @@ -225,6 +234,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; >> +} >> + >> static bool pcc_chan_command_complete(struct pcc_chan_info *pchan, >> u64 cmd_complete_reg_val) >> { >> @@ -277,6 +292,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) >> int ret; >> >> pchan = chan->con_priv; >> + if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE && >> + !pchan->chan_in_use) > I would have avoided the type check above but I understand your concern > so let us keep it like this for now. Thanks for your unstanding. > > Please submit non-RFC patch as some maintainers may not look at RFC. I will send V2 ASAP. >
PCC supports processing platform notification for slave subspaces and shared interrupt for multiple subspaces. --- -v1: using subspace type to replace comm_flow_dir in patch [1/2] -rfc-v3: split V2 into two patches. -rfc-v2: don't use platform interrupt ack register to identify if the given channel should respond interrupt. Huisong Li (2): mailbox: pcc: Add processing platform notification for slave subspaces mailbox: pcc: Support shared interrupt for multiple subspaces drivers/mailbox/pcc.c | 102 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 93 insertions(+), 9 deletions(-)