Re: [PATCH 14/14] ACPI: CPPC: Simplify PCC shared memory region handling

[Rafael J. Wysocki]

On Mon, Mar 3, 2025 at 11:53 AM Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>
> The PCC driver now handles mapping and unmapping of shared memory
> areas as part of pcc_mbox_{request,free}_channel(). Without these before,
> this ACPI CPPC driver did handling of those mappings like several
> other PCC mailbox client drivers.
>
> There were redundant operations, leading to unnecessary code. Maintaining
> the consistency across these driver was harder due to scattered handling
> of shmem.
>
> Just use the mapped shmem and remove all redundant operations from this
> driver.
>
> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>

Acked-by: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>

> ---
>  drivers/acpi/cppc_acpi.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index f193e713825ac24203ece5f94d6cf99dd4724ce4..d972157a79b6ade2f3738c90128e8692141b3ee5 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -47,7 +47,6 @@
>
>  struct cppc_pcc_data {
>         struct pcc_mbox_chan *pcc_channel;
> -       void __iomem *pcc_comm_addr;
>         bool pcc_channel_acquired;
>         unsigned int deadline_us;
>         unsigned int pcc_mpar, pcc_mrtt, pcc_nominal;
> @@ -95,7 +94,7 @@ static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
>  static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>
>  /* pcc mapped address + header size + offset within PCC subspace */
> -#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
> +#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_channel->shmem + \
>                                                 0x8 + (offs))
>
>  /* Check if a CPC register is in PCC */
> @@ -223,7 +222,7 @@ static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
>         int ret, status;
>         struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>         struct acpi_pcct_shared_memory __iomem *generic_comm_base =
> -               pcc_ss_data->pcc_comm_addr;
> +                                       pcc_ss_data->pcc_channel->shmem;
>
>         if (!pcc_ss_data->platform_owns_pcc)
>                 return 0;
> @@ -258,7 +257,7 @@ static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
>         int ret = -EIO, i;
>         struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>         struct acpi_pcct_shared_memory __iomem *generic_comm_base =
> -               pcc_ss_data->pcc_comm_addr;
> +                                       pcc_ss_data->pcc_channel->shmem;
>         unsigned int time_delta;
>
>         /*
> @@ -571,15 +570,6 @@ static int register_pcc_channel(int pcc_ss_idx)
>                 pcc_data[pcc_ss_idx]->pcc_mpar = pcc_chan->max_access_rate;
>                 pcc_data[pcc_ss_idx]->pcc_nominal = pcc_chan->latency;
>
> -               pcc_data[pcc_ss_idx]->pcc_comm_addr =
> -                       acpi_os_ioremap(pcc_chan->shmem_base_addr,
> -                                       pcc_chan->shmem_size);
> -               if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
> -                       pr_err("Failed to ioremap PCC comm region mem for %d\n",
> -                              pcc_ss_idx);
> -                       return -ENOMEM;
> -               }
> -
>                 /* Set flag so that we don't come here for each CPU. */
>                 pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
>         }
>
> --
> 2.34.1
>