Re: [PATCH 03/13] mailbox: pcc: Refactor all PCC channel information into a structure

[Cristian Marussi]

On Thu, Jul 15, 2021 at 12:27:10PM +0100, Sudeep Holla wrote:
> On Wed, Jul 14, 2021 at 05:54:34PM +0100, Cristian Marussi wrote:
> > On Thu, Jul 08, 2021 at 07:08:41PM +0100, Sudeep Holla wrote:
> > > Currently all the PCC channel specific information are stored/maintained
> > > in global individual arrays for each of those information. It is not
> > > scalable and not clean if we have to stash more channel specific
> > > information. Couple of reasons to stash more information are to extend
> > > the support to Type 3/4 PCCT subspace and also to avoid accessing the
> > > PCCT table entries themselves each time we need the information.
> > > 
> > > This patch moves all those PCC channel specific information into a
> > > separate structure pcc_chan_info.
> > > 
> > > Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
> > > ---
> > 
> > Hi Sudeep,
> > 
> > >  drivers/mailbox/pcc.c | 106 +++++++++++++++++++++---------------------
> > >  1 file changed, 53 insertions(+), 53 deletions(-)
> > > 
> > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > > index 23391e224a68..c5f481a615b0 100644
> > > --- a/drivers/mailbox/pcc.c
> > > +++ b/drivers/mailbox/pcc.c
> > > @@ -64,12 +64,20 @@
> > >  
> > >  static struct mbox_chan *pcc_mbox_channels;
> > >  
> > > -/* Array of cached virtual address for doorbell registers */
> > > -static void __iomem **pcc_doorbell_vaddr;
> > > -/* Array of cached virtual address for doorbell ack registers */
> > > -static void __iomem **pcc_doorbell_ack_vaddr;
> > > -/* Array of doorbell interrupts */
> > > -static int *pcc_doorbell_irq;
> > > +/**
> > > + * struct pcc_chan_info - PCC channel specific information
> > > + *
> > > + * @db_vaddr: cached virtual address for doorbell register
> > > + * @db_ack_vaddr: cached virtual address for doorbell ack register
> > > + * @db_irq: doorbell interrupt
> > > + */
> > > +struct pcc_chan_info {
> > > +	void __iomem *db_vaddr;
> > > +	void __iomem *db_ack_vaddr;
> > > +	int db_irq;
> > > +};
> >
> > Given that this db_irq represents the optional completion interrupt that is
> > used platform-->OSPM to signal command completions and/or notifications/
> > delayed_responses and it is mentioned indeed in ACPI 6.4 as "Platform
> > Interrupt" and also referred in this driver as such somewherelse, is it not
> > misleading to call it then here "doorbell interrupt" since the "doorbell" in
> > this context is usually the interrupt that goes the other way around
> > OSPM-->Platform and is indeed handled by a different set of dedicated Doorbell
> > registers ? (that are indeed called Doorbell throughout this driver down below
> > ...but I understand this was the nomenclature used also before in this driver)
> >
> 
> Exactly, I share your thoughts and I completely agree. I didn't want to change
> it in this patch as that would mix 2 different change and makes it hard to
> review. I assume you might have already seen the 8/13 which renames this
> before we add more such registers in later patches.
> 

Yes indeed, but db_irq is not renamed even later when db_vaddr/db_ack_addr are
consolidated and renamed. So that confused me even more :D

Thanks,
Cristian
> --
> Regards,
> Sudeep