Messages in this thread | | | Date | Tue, 14 Mar 2023 09:05:00 +0800 | Subject | Re: [PATCH 2/2] mailbox: pcc: Support shared interrupt for multiple subspaces | From | "lihuisong (C)" <> |
| |
在 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. >
| |