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@xxxxxxx>
> ---
> 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@xxxxxxx>
Thanks,
Cristian
