Messages in this thread | | | Date | Fri, 23 Jun 2023 13:52:49 +0800 | Subject | Re: [PATCH net-next 1/6] platform/x86: intel_pmc_core: Add IPC mailbox accessor function and add SoC register access | From | Choong Yong Liang <> |
| |
Hi Hans,
I will discuss it with David but currently he is on vacation. It might take some time to get the final output. Thank you.
On 22/6/2023 4:18 pm, Hans de Goede wrote: > Hi, > > On 6/22/23 06:19, Choong Yong Liang wrote: >> From: "David E. Box" <david.e.box@linux.intel.com> >> >> - 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@linux.intel.com> >> Signed-off-by: Chao Qin <chao.qin@intel.com> >> Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> > > This seem to be 2 patches in 1: > > 1. Move core.h to include/linux/intel_pmc_core.h > 2. The actual adding of IPC mailbox accessor function and add SoC register access > > I wonder if you really need to move the entire core.h ? > > IMHO it would be better to just add a new header with just the bits > which you actually need to export the desired functionality. > > If you do believe that you really need to move core.h please split > this into 2 separate patches and please place the header in a x86 > specific place, e.g. : include/linux/platform_data/x86/ > > > > Also note that a somewhat big refactor, to add support for > multiple PMCs for Meteor Lake is on its way to linux-next. > > Currently this is available in my review-hans branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > Please base a next version of this on this. > > There also is the question of how to merge this. Assuming this is > ready for merging once 6.5-rc1 is out then I can merge this intel_pmc_core > change into an immutable branch and send a pull-req to the net folks > for this. > > Regards, > > Hans > > > > > > >> --- >> MAINTAINERS | 1 + >> drivers/platform/x86/intel/pmc/adl.c | 2 +- >> drivers/platform/x86/intel/pmc/cnp.c | 2 +- >> drivers/platform/x86/intel/pmc/core.c | 63 ++++++++++++++++++- >> drivers/platform/x86/intel/pmc/icl.c | 2 +- >> drivers/platform/x86/intel/pmc/mtl.c | 2 +- >> drivers/platform/x86/intel/pmc/spt.c | 2 +- >> drivers/platform/x86/intel/pmc/tgl.c | 2 +- >> .../core.h => include/linux/intel_pmc_core.h | 27 +++++++- >> 9 files changed, 95 insertions(+), 8 deletions(-) >> rename drivers/platform/x86/intel/pmc/core.h => include/linux/intel_pmc_core.h (95%) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index cb14589d14ab..bdb08a79a5f8 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -10581,6 +10581,7 @@ L: platform-driver-x86@vger.kernel.org >> S: Maintained >> F: Documentation/ABI/testing/sysfs-platform-intel-pmc >> F: drivers/platform/x86/intel/pmc/ >> +F: include/linux/intel_pmc_core* >> >> INTEL PMIC GPIO DRIVERS >> M: Andy Shevchenko <andy@kernel.org> >> diff --git a/drivers/platform/x86/intel/pmc/adl.c b/drivers/platform/x86/intel/pmc/adl.c >> index 5cbd40979f2a..b6a376c536c0 100644 >> --- a/drivers/platform/x86/intel/pmc/adl.c >> +++ b/drivers/platform/x86/intel/pmc/adl.c >> @@ -8,7 +8,7 @@ >> * >> */ >> >> -#include "core.h" >> +#include <linux/intel_pmc_core.h> >> >> /* Alder Lake: PGD PFET Enable Ack Status Register(s) bitmap */ >> const struct pmc_bit_map adl_pfear_map[] = { >> diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c >> index 7fb38815c4eb..504034cc5ec3 100644 >> --- a/drivers/platform/x86/intel/pmc/cnp.c >> +++ b/drivers/platform/x86/intel/pmc/cnp.c >> @@ -8,7 +8,7 @@ >> * >> */ >> >> -#include "core.h" >> +#include <linux/intel_pmc_core.h> >> >> /* Cannon Lake: PGD PFET Enable Ack Status Register(s) bitmap */ >> const struct pmc_bit_map cnp_pfear_map[] = { >> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c >> index da6e7206d38b..0d60763c5144 100644 >> --- a/drivers/platform/x86/intel/pmc/core.c >> +++ b/drivers/platform/x86/intel/pmc/core.c >> @@ -16,6 +16,7 @@ >> #include <linux/delay.h> >> #include <linux/dmi.h> >> #include <linux/io.h> >> +#include <linux/intel_pmc_core.h> >> #include <linux/module.h> >> #include <linux/pci.h> >> #include <linux/slab.h> >> @@ -26,7 +27,9 @@ >> #include <asm/msr.h> >> #include <asm/tsc.h> >> >> -#include "core.h" >> +#define PMC_IPCS_PARAM_COUNT 7 >> + >> +static const struct x86_cpu_id *pmc_cpu_id; >> >> /* Maximum number of modes supported by platfoms that has low power mode capability */ >> const char *pmc_lpm_modes[] = { >> @@ -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 (!pmc_cpu_id || !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_dev *pmcdev, int reg_offset) >> { >> return readl(pmcdev->regbase + reg_offset); >> @@ -1130,6 +1190,7 @@ static int pmc_core_probe(struct platform_device *pdev) >> mutex_init(&pmcdev->lock); >> core_init(pmcdev); >> >> + pmc_cpu_id = cpu_id; >> >> if (lpit_read_residency_count_address(&slp_s0_addr)) { >> pmcdev->base_addr = PMC_BASE_ADDR_DEFAULT; >> diff --git a/drivers/platform/x86/intel/pmc/icl.c b/drivers/platform/x86/intel/pmc/icl.c >> index 2f11b1a6daeb..f18048ff9382 100644 >> --- a/drivers/platform/x86/intel/pmc/icl.c >> +++ b/drivers/platform/x86/intel/pmc/icl.c >> @@ -8,7 +8,7 @@ >> * >> */ >> >> -#include "core.h" >> +#include <linux/intel_pmc_core.h> >> >> const struct pmc_bit_map icl_pfear_map[] = { >> {"RES_65", BIT(0)}, >> diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c >> index e8cc156412ce..7897f5fe9881 100644 >> --- a/drivers/platform/x86/intel/pmc/mtl.c >> +++ b/drivers/platform/x86/intel/pmc/mtl.c >> @@ -9,7 +9,7 @@ >> */ >> >> #include <linux/pci.h> >> -#include "core.h" >> +#include <linux/intel_pmc_core.h> >> >> const struct pmc_reg_map mtl_reg_map = { >> .pfear_sts = ext_tgl_pfear_map, >> diff --git a/drivers/platform/x86/intel/pmc/spt.c b/drivers/platform/x86/intel/pmc/spt.c >> index e16982236778..95ce490cf5d6 100644 >> --- a/drivers/platform/x86/intel/pmc/spt.c >> +++ b/drivers/platform/x86/intel/pmc/spt.c >> @@ -8,7 +8,7 @@ >> * >> */ >> >> -#include "core.h" >> +#include <linux/intel_pmc_core.h> >> >> const struct pmc_bit_map spt_pll_map[] = { >> {"MIPI PLL", SPT_PMC_BIT_MPHY_CMN_LANE0}, >> diff --git a/drivers/platform/x86/intel/pmc/tgl.c b/drivers/platform/x86/intel/pmc/tgl.c >> index c245ada849d0..a1719d809497 100644 >> --- a/drivers/platform/x86/intel/pmc/tgl.c >> +++ b/drivers/platform/x86/intel/pmc/tgl.c >> @@ -8,7 +8,7 @@ >> * >> */ >> >> -#include "core.h" >> +#include <linux/intel_pmc_core.h> >> >> #define ACPI_S0IX_DSM_UUID "57a6512e-3979-4e9d-9708-ff13b2508972" >> #define ACPI_GET_LOW_MODE_REGISTERS 1 >> diff --git a/drivers/platform/x86/intel/pmc/core.h b/include/linux/intel_pmc_core.h >> similarity index 95% >> rename from drivers/platform/x86/intel/pmc/core.h >> rename to include/linux/intel_pmc_core.h >> index 9ca9b9746719..82810e8b92a2 100644 >> --- a/drivers/platform/x86/intel/pmc/core.h >> +++ b/include/linux/intel_pmc_core.h >> @@ -250,7 +250,16 @@ enum ppfear_regs { >> #define MTL_LPM_STATUS_OFFSET 0x1700 >> #define MTL_LPM_LIVE_STATUS_OFFSET 0x175C >> >> -extern const char *pmc_lpm_modes[]; >> +#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]; >> +}; >> >> struct pmc_bit_map { >> const char *name; >> @@ -427,4 +436,20 @@ static const struct file_operations __name ## _fops = { \ >> .release = single_release, \ >> } >> >> +#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 /* PMC_CORE_H */ >
| |