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

[Hans de Goede]

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.

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 .

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 */