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

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

On Thu, Jul 15, 2021 at 01:50:48PM +0100, Cristian Marussi wrote:
> 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@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.
> >
>
> 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
>

Yikes! That was not intentional. Since I re-ordered some of the changes
after making them originally, I relied on regex to get it right and ease
the rebasing which I know I failed terribly. I will fix that too.

--
Regards,
Sudeep