Re: [PATCH net-next v2 1/5] platform/x86: intel_pmc_core: Add IPC mailbox accessor function and add SoC register access

[David E. Box]

Hi Hans,

On Mon, 2023-08-07 at 13:02 +0200, Hans de Goede wrote:
> > Hi David,
> > 
> > On 8/4/23 10:45, Choong Yong Liang wrote:
> > > > From: "David E. Box" <david.e.box@xxxxxxxxxxxxxxx>
> > > > 
> > > > - Exports intel_pmc_core_ipc() for host access to the PMC IPC mailbox
> > > > - Add support to use IPC command allows host to access SoC registers
> > > > through PMC firmware that are otherwise inaccessible to the host due to
> > > > security policies.
> > > > 
> > > > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Chao Qin <chao.qin@xxxxxxxxx>
> > > > Signed-off-by: Choong Yong Liang <yong.liang.choong@xxxxxxxxxxxxxxx>
> > 
> > The new exported intel_pmc_core_ipc() function does not seem to
> > depend on any existing PMC code.
> > 
> > IMHO it would be better to put this in a new .c file under
> > arch/x86/platform/intel/ this is where similar helpers like
> > the iosf_mbi functions also live.
> > 
> > This also avoids Kconfig complications. Currently the
> > drivers/platform/x86/intel/pmc/core.c code is only
> > build if CONFIG_X86_PLATFORM_DEVICES and
> > CONFIG_INTEL_PMC_CORE are both set. So if a driver
> > wants to make sure this is enabled by selecting them
> > then it needs to select both.

Yeah, makes sense. This is an old patch. Once upon a time the PMC driver was
going to use the IPC to access some registers but we were able to get them from
elsewhere. The patch was brought back for the TSN use case. But you're correct
that arch/x86/platform/intel makes more sense if the function is to be exported
now and doesn't require to PMC driver to discover the interface. We'll do that.

> > 
> > Talking about Kconfig:
> > 
> > #if IS_ENABLED(CONFIG_INTEL_PMC_CORE)
> > int intel_pmc_core_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf);
> > #else
> > static inline int intel_pmc_core_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf)
> > {
> >         return -ENODEV;
> > }
> > #endif /* CONFIG_INTEL_PMC_CORE */
> > 
> > Notice that CONFIG_INTEL_PMC_CORE is a tristate, so pmc might be build as a
> > > module where as a consumer of intel_pmc_core_ipc() might end up builtin in
> > > which case this will not work without extra Kconfig protection. And if you
> > are > going to add extra Kconfig you might just as well select or depend on
> > > INTEL_PMC_CORE and drop the #if .

Sure. Thanks.

David

> > 
> > Regards,
> > 
> > Hans
> > 
> > 
> > 
> > 
> > 
> > 
> > > > ---
> > > >  MAINTAINERS                                   |  1 +
> > > >  drivers/platform/x86/intel/pmc/core.c         | 60 +++++++++++++++++++
> > > >  .../linux/platform_data/x86/intel_pmc_core.h  | 41 +++++++++++++
> > > >  3 files changed, 102 insertions(+)
> > > >  create mode 100644 include/linux/platform_data/x86/intel_pmc_core.h
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 069e176d607a..8a034dee9da9 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -10648,6 +10648,7 @@ L:      platform-driver-x86@xxxxxxxxxxxxxxx
> > > >  S:     Maintained
> > > >  F:     Documentation/ABI/testing/sysfs-platform-intel-pmc
> > > >  F:     drivers/platform/x86/intel/pmc/
> > > > +F:     linux/platform_data/x86/intel_pmc_core.h
> > > >  
> > > >  INTEL PMIC GPIO DRIVERS
> > > >  M:     Andy Shevchenko <andy@xxxxxxxxxx>
> > > > diff --git a/drivers/platform/x86/intel/pmc/core.c > >
> > > > b/drivers/platform/x86/intel/pmc/core.c
> > > > index 5a36b3f77bc5..6fb1b0f453d8 100644
> > > > --- a/drivers/platform/x86/intel/pmc/core.c
> > > > +++ b/drivers/platform/x86/intel/pmc/core.c
> > > > @@ -20,6 +20,7 @@
> > > >  #include <linux/pci.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/suspend.h>
> > > > +#include <linux/platform_data/x86/intel_pmc_core.h>
> > > >  
> > > >  #include <asm/cpu_device_id.h>
> > > >  #include <asm/intel-family.h>
> > > > @@ -28,6 +29,8 @@
> > > >  
> > > >  #include "core.h"
> > > >  
> > > > +#define PMC_IPCS_PARAM_COUNT           7
> > > > +
> > > >  /* Maximum number of modes supported by platfoms that has low power
> > > > mode > > capability */
> > > >  const char *pmc_lpm_modes[] = {
> > > >         "S0i2.0",
> > > > @@ -53,6 +56,63 @@ const struct pmc_bit_map msr_map[] = {
> > > >         {}
> > > >  };
> > > >  
> > > > +int intel_pmc_core_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf)
> > > > +{
> > > > +       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > > > +       union acpi_object params[PMC_IPCS_PARAM_COUNT] = {
> > > > +               {.type = ACPI_TYPE_INTEGER,},
> > > > +               {.type = ACPI_TYPE_INTEGER,},
> > > > +               {.type = ACPI_TYPE_INTEGER,},
> > > > +               {.type = ACPI_TYPE_INTEGER,},
> > > > +               {.type = ACPI_TYPE_INTEGER,},
> > > > +               {.type = ACPI_TYPE_INTEGER,},
> > > > +               {.type = ACPI_TYPE_INTEGER,},
> > > > +       };
> > > > +       struct acpi_object_list arg_list = { PMC_IPCS_PARAM_COUNT,
> > > > params };
> > > > +       union acpi_object *obj;
> > > > +       int status;
> > > > +
> > > > +       if (!ipc_cmd || !rbuf)
> > > > +               return -EINVAL;
> > > > +
> > > > +       /*
> > > > +        * 0: IPC Command
> > > > +        * 1: IPC Sub Command
> > > > +        * 2: Size
> > > > +        * 3-6: Write Buffer for offset
> > > > +        */
> > > > +       params[0].integer.value = ipc_cmd->cmd;
> > > > +       params[1].integer.value = ipc_cmd->sub_cmd;
> > > > +       params[2].integer.value = ipc_cmd->size;
> > > > +       params[3].integer.value = ipc_cmd->wbuf[0];
> > > > +       params[4].integer.value = ipc_cmd->wbuf[1];
> > > > +       params[5].integer.value = ipc_cmd->wbuf[2];
> > > > +       params[6].integer.value = ipc_cmd->wbuf[3];
> > > > +
> > > > +       status = acpi_evaluate_object(NULL, "\\IPCS", &arg_list,
> > > > &buffer);
> > > > +       if (ACPI_FAILURE(status))
> > > > +               return -ENODEV;
> > > > +
> > > > +       obj = buffer.pointer;
> > > > +       /* Check if the number of elements in package is 5 */
> > > > +       if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count
> > > > == > > 5) {
> > > > +               const union acpi_object *objs = obj->package.elements;
> > > > +
> > > > +               if ((u8)objs[0].integer.value != 0)
> > > > +                       return -EINVAL;
> > > > +
> > > > +               rbuf[0] = objs[1].integer.value;
> > > > +               rbuf[1] = objs[2].integer.value;
> > > > +               rbuf[2] = objs[3].integer.value;
> > > > +               rbuf[3] = objs[4].integer.value;
> > > > +       } else {
> > > > +               return -EINVAL;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(intel_pmc_core_ipc);
> > > > +
> > > >  static inline u32 pmc_core_reg_read(struct pmc *pmc, int reg_offset)
> > > >  {
> > > >         return readl(pmc->regbase + reg_offset);
> > > > diff --git a/include/linux/platform_data/x86/intel_pmc_core.h > >
> > > > b/include/linux/platform_data/x86/intel_pmc_core.h
> > > > new file mode 100644
> > > > index 000000000000..9bb3394fedcf
> > > > --- /dev/null
> > > > +++ b/include/linux/platform_data/x86/intel_pmc_core.h
> > > > @@ -0,0 +1,41 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/*
> > > > + * Intel Core SoC Power Management Controller Header File
> > > > + *
> > > > + * Copyright (c) 2023, Intel Corporation.
> > > > + * All Rights Reserved.
> > > > + *
> > > > + * Authors: Choong Yong Liang <yong.liang.choong@xxxxxxxxxxxxxxx>
> > > > + *          David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > > > + */
> > > > +#ifndef INTEL_PMC_CORE_H
> > > > +#define INTEL_PMC_CORE_H
> > > > +
> > > > +#define IPC_SOC_REGISTER_ACCESS                        0xAA
> > > > +#define IPC_SOC_SUB_CMD_READ                   0x00
> > > > +#define IPC_SOC_SUB_CMD_WRITE                  0x01
> > > > +
> > > > +struct pmc_ipc_cmd {
> > > > +       u32 cmd;
> > > > +       u32 sub_cmd;
> > > > +       u32 size;
> > > > +       u32 wbuf[4];
> > > > +};
> > > > +
> > > > +#if IS_ENABLED(CONFIG_INTEL_PMC_CORE)
> > > > +/**
> > > > + * intel_pmc_core_ipc() - PMC IPC Mailbox accessor
> > > > + * @ipc_cmd:  struct pmc_ipc_cmd prepared with input to send
> > > > + * @rbuf:     Allocated u32[4] array for returned IPC data
> > > > + *
> > > > + * Return: 0 on success. Non-zero on mailbox error
> > > > + */
> > > > +int intel_pmc_core_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf);
> > > > +#else
> > > > +static inline int intel_pmc_core_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 >
> > > > > *rbuf)
> > > > +{
> > > > +       return -ENODEV;
> > > > +}
> > > > +#endif /* CONFIG_INTEL_PMC_CORE */
> > > > +
> > > > +#endif /* INTEL_PMC_CORE_H */
> >