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

[Sudeep Holla <sudeep.holla@arm.com>]

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@arm.com>
> > ---
> 
> 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.

--
Regards,
Sudeep