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

[Cristian Marussi <cristian.marussi@arm.com>]

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 ?

>  /**
>   * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
>   *
> @@ -536,6 +559,8 @@ static int __init acpi_pcc_probe(void)
>  		struct pcc_chan_info *pchan = chan_info + i;
>  		pcc_mbox_channels[i].con_priv = pcct_entry;
>  
> +		pchan->chan.mchan = &pcc_mbox_channels[i];
> +
>  		if (pcc_mbox_ctrl.txdone_irq) {
>  			rc = pcc_parse_subspace_irq(pchan, pcct_entry);
>  			if (rc < 0)
> @@ -543,6 +568,8 @@ static int __init acpi_pcc_probe(void)
>  		}
>  		pcc_parse_subspace_db_reg(pchan, pcct_entry);
>  
> +		pcc_parse_subspace_shmem(pchan, pcct_entry);
> +
>  		pcct_entry = (struct acpi_subtable_header *)
>  			((unsigned long) pcct_entry + pcct_entry->length);
>  	}
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 4dec4ed138cd..5e510a6b8052 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -9,6 +9,15 @@
>  #include <linux/mailbox_controller.h>
>  #include <linux/mailbox_client.h>
>  
> +struct pcc_mbox_chan {
> +	struct mbox_chan *mchan;
> +	u64 shmem_base_addr;
> +	u64 shmem_size;
> +	u32 latency;
> +	u32 max_access_rate;
> +	u16 min_turnaround_time;
> +};
> +
>  #define MAX_PCC_SUBSPACES	256
>  #ifdef CONFIG_PCC
>  extern struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
> -- 
> 2.25.1


Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian