Re: [PATCH 06/13] mailbox: pcc: Add pcc_mbox_chan structure to hold shared memory region info

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

On Wed, Jul 14, 2021 at 07:18:43PM +0100, Cristian Marussi wrote:
> On Thu, Jul 08, 2021 at 07:08:44PM +0100, Sudeep Holla wrote:
> > Currently PCC mailbox controller sets con_priv in each channel to hold
> > the pointer to pcct subspace entry it corresponds to. The mailbox uses
> 
> nit: s/uses/users
> 
> > will then fetch this pointer from the channel descriptor they get when
> > they request for the channel. Using that pointer they then parse the
> > pcct entry again to fetch all the information about shared memory region.
> > 
> > In order to remove individual users of PCC mailbox parsing the PCCT
> > subspace entries to fetch same information, let us consolidate the same
> > in pcc mailbox controller by parsing all the shared memory region
> > information into a structure that can also hold the mbox_chan pointer it
> > represent.
> > 
> > This can then be used as main PCC mailbox channel pointer that we can
> > return as part of pcc_mbox_request_channel instead of standard mailbox
> > channel pointer.
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/mailbox/pcc.c | 27 +++++++++++++++++++++++++++
> >  include/acpi/pcc.h    |  9 +++++++++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > index 5f19bee71c04..affde0995d52 100644
> > --- a/drivers/mailbox/pcc.c
> > +++ b/drivers/mailbox/pcc.c
> > @@ -67,11 +67,13 @@ static struct mbox_chan *pcc_mbox_channels;
> >  /**
> >   * struct pcc_chan_info - PCC channel specific information
> >   *
> > + * @chan: PCC channel information with Shared Memory Region info
> >   * @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 {
> > +	struct pcc_mbox_chan chan;
> >  	void __iomem *db_vaddr;
> >  	void __iomem *db_ack_vaddr;
> >  	int db_irq;
> > @@ -469,6 +471,27 @@ static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> >  						  db_reg->bit_width / 8);
> >  }
> >  
> > +/**
> > + * pcc_parse_subspace_shmem - Parse the PCC Shared Memory Region information
> > + *
> > + * @pchan: Pointer to the PCC channel info structure.
> > + * @pcct_entry: Pointer to the ACPI subtable header.
> > + *
> > + */
> > +static void pcc_parse_subspace_shmem(struct pcc_chan_info *pchan,
> > +				     struct acpi_subtable_header *pcct_entry)
> > +{
> > +	struct acpi_pcct_subspace *pcct_ss;
> > +
> > +	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
> > +
> > +	pchan->chan.shmem_base_addr = pcct_ss->base_address;
> > +	pchan->chan.shmem_size = pcct_ss->length;
> > +	pchan->chan.latency = pcct_ss->latency;
> > +	pchan->chan.max_access_rate = pcct_ss->max_access_rate;
> > +	pchan->chan.min_turnaround_time = pcct_ss->min_turnaround_time;
> > +}
> > +
>
> Out of curiosity this ACPI provided latency/max_access/turnaround_time
> are supposed to be considered and/or enforced where ? by the clients
> using this controller ?
>

Ideally enforced, if not at-least considered by the clients using this
PCC channel. I do agree it results in duplication and at the discretion
of the clients. I am thinking if we can provide some kind of helpers/
accessors to achieve that. Very rough idea still as mentioned in the
cover letter, not looked at all users in details. Some drivers map the
shared memory as normal/cached while majority expects it to be device/IO.
That is one difference to consider while we try to consolidate shmem
accesses. Happy to hear opinions here 😄.

--
Regards,
Sudeep