Re: [RFC] ACPI: PCC: Support shared interrupt for multiple subspaces

[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@xxxxxxxxxx>
> > > ---
> > >   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.😁
> >

Please find below. I am not convinced yet to have extra flag for checking if
the channel is in use. The other idea I had is to use the Generic Communications
Channel Shared Memory Region Status Field in particular Command Complete
field. I haven't tried it yet and hence the reason for not posting the patch.
Let me know if the idea looks sane, so that I can try something and share
it. I may not have a setup handy to test and may need sometime to test though.

Regards,
Sudeep

-->8