Darren, Andy Great thanks your kind, professional review. Just sent out v10, I sent twice since I don't receive patch set 2 after first time. They are same, sorry for the trouble. Best wishes Qipeng -----Original Message----- From: platform-driver-x86-owner@xxxxxxxxxxxxxxx [mailto:platform-driver-x86-owner@xxxxxxxxxxxxxxx] On Behalf Of Darren Hart Sent: Friday, December 11, 2015 7:02 AM To: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> Cc: Zha, Qipeng <qipeng.zha@xxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx Subject: Re: [PATCH V9 2/2] platform:x86: add Intel P-Unit mailbox IPC driver On Thu, Dec 10, 2015 at 01:23:05PM +0200, Andy Shevchenko wrote: > On Fri, 2015-12-11 at 02:21 +0800, Qipeng Zha wrote: > > This driver provides support for P-Unit mailbox IPC on Intel > > platforms. > > The heart of the P-Unit is the Foxton microcontroller and its > > firmware, which provide mailbox interface for power management > > usage. > > > > Signed-off-by: Qipeng Zha <qipeng.zha@xxxxxxxxx> > > Couple of nitpicks below, otherwise finally my tag > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > > > > --- > > change in v9 > > remove non-necessary cast for void pointer; > > remove redundant error messages. > > > > change in v8 > > Change the way to get three IPCs's resources due to udpated acpi > > entries, > > > > In the new acpi resources layout, two entries for each IPC, one for > > data and one for interface. > > > > change in v7 > > Update ipc_err_string()'s return type to "const char *" from "char > > *"; > > Add terminator in acpi id array. > > > > change in v6 > > Update header file; > > Update interface of lowlevel register access; > > Restructure implementation of two command functions; > > Save IPC private data pointer to pdev's priv; > > > > change in v5 > > Fix commend style in header file; > > Replace EINVAL with ENODEV in stub functions; > > Replace ipc_err_sources array with ipc_err_string function; > > Correct comments: "if invalid" -> "if not used"; > > Propagate return error of devm_request_irq. > > > > change in v4 > > Fix two code style issues: make /* as a whole line and replace > > "return ret" with "goto out"; > > Replace -EINVAL with -ENXIO for failures due to resource. > > > > change in v3 > > Fix compile issue in intel_punit_ipc.h, it happened when built-in > > and the header file is included in other source file. > > > > change in v2 > > Fix issues in code style and comments; > > Remove "default y" in Kconfig; > > Remove some header files; > > Replace request_mem_region with devm_request_mem_region, and same > > for request_irq; > > Change to use prefix of IPC_PUNIT_ to define commands. > > --- > > MAINTAINERS | 4 +- > > arch/x86/include/asm/intel_punit_ipc.h | 101 ++++++++++ > > drivers/platform/x86/Kconfig | 6 + > > drivers/platform/x86/Makefile | 1 + > > drivers/platform/x86/intel_punit_ipc.c | 336 > > +++++++++++++++++++++++++++++++++ > > 5 files changed, 447 insertions(+), 1 deletion(-) > > create mode 100644 arch/x86/include/asm/intel_punit_ipc.h > > create mode 100644 drivers/platform/x86/intel_punit_ipc.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS index 5192700..333a022 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -5683,12 +5683,14 @@ F: drivers/dma/mic_x100_dma.c > > F: drivers/dma/mic_x100_dma.h > > F Documentation/mic/ > > > > -INTEL PMC IPC DRIVER > > +INTEL PMC/P-Unit IPC DRIVER > > M: Zha Qipeng<qipeng.zha@xxxxxxxxx> > > L: platform-driver-x86@xxxxxxxxxxxxxxx > > S: Maintained > > F: drivers/platform/x86/intel_pmc_ipc.c > > +F: drivers/platform/x86/intel_punit_ipc.c > > F: arch/x86/include/asm/intel_pmc_ipc.h > > +F: arch/x86/include/asm/intel_punit_ipc.h > > > > IOC3 ETHERNET DRIVER > > M: Ralf Baechle <ralf@xxxxxxxxxxxxxx> > > diff --git a/arch/x86/include/asm/intel_punit_ipc.h > > b/arch/x86/include/asm/intel_punit_ipc.h > > new file mode 100644 > > index 0000000..201eb9d > > --- /dev/null > > +++ b/arch/x86/include/asm/intel_punit_ipc.h > > @@ -0,0 +1,101 @@ > > +#ifndef _ASM_X86_INTEL_PUNIT_IPC_H_ #define > > +_ASM_X86_INTEL_PUNIT_IPC_H_ > > + > > +/* > > + * Three types of 8bit P-Unit IPC commands are supported, > > + * bit[7:6]: [00]: BIOS; [01]: GTD; [10]: ISPD. > > + */ > > +typedef enum { > > + BIOS_IPC = 0, > > + GTDRIVER_IPC, > > + ISPDRIVER_IPC, > > + RESERVED_IPC, > > +} IPC_TYPE; > > + > > +#define IPC_TYPE_OFFSET 6 > > +#define IPC_PUNIT_BIOS_CMD_BASE (BIOS_IPC << > > IPC_TYPE_OFFSET) > > +#define IPC_PUNIT_GTD_CMD_BASE (GTDDRIVER_IPC << > > IPC_TYPE_OFFSET) > > +#define IPC_PUNIT_ISPD_CMD_BASE (ISPDRIVER_IPC << > > IPC_TYPE_OFFSET) > > +#define IPC_PUNIT_CMD_TYPE_MASK (RESERVED_IPC << > > IPC_TYPE_OFFSET) > > + > > +/* BIOS => Pcode commands */ > > +#define IPC_PUNIT_BIOS_ZERO (IPC_PUNIT_BIOS_C > > MD_BASE | 0x00) > > +#define IPC_PUNIT_BIOS_VR_INTERFACE (IPC_PUNIT_BIOS_C > > MD_BASE | 0x01) > > +#define IPC_PUNIT_BIOS_READ_PCS (IPC_PUNIT_BI > > OS_CMD_BASE | 0x02) > > +#define IPC_PUNIT_BIOS_WRITE_PCS (IPC_PUNIT_BIOS_CMD_ > > BASE | 0x03) > > +#define IPC_PUNIT_BIOS_READ_PCU_CONFIG (IPC_PUNIT_BIO > > S_CMD_BASE | 0x04) > > +#define IPC_PUNIT_BIOS_WRITE_PCU_CONFIG (IPC_PUNIT_BI > > OS_CMD_BASE | 0x05) > > +#define IPC_PUNIT_BIOS_READ_PL1_SETTING (IPC_PUNIT_BI > > OS_CMD_BASE | 0x06) > > +#define IPC_PUNIT_BIOS_WRITE_PL1_SETTING (IPC_PUNIT_BIOS_CMD_ > > BASE | 0x07) > > +#define IPC_PUNIT_BIOS_TRIGGER_VDD_RAM (IPC_PUNIT_BIO > > S_CMD_BASE | 0x08) > > +#define IPC_PUNIT_BIOS_READ_TELE_INFO (IPC_PUNIT_BIOS > > _CMD_BASE | 0x09) > > +#define IPC_PUNIT_BIOS_READ_TELE_TRACE_CTRL (IPC_PUNIT_BIOS_C > > MD_BASE | 0x0a) > > +#define IPC_PUNIT_BIOS_WRITE_TELE_TRACE_CTRL (IPC_PUNIT_BIOS_ > > CMD_BASE | 0x0b) > > +#define IPC_PUNIT_BIOS_READ_TELE_EVENT_CTRL (IPC_PUNIT_BIOS_C > > MD_BASE | 0x0c) > > +#define IPC_PUNIT_BIOS_WRITE_TELE_EVENT_CTRL (IPC_PUNIT_BIOS_ > > CMD_BASE | 0x0d) > > +#define IPC_PUNIT_BIOS_READ_TELE_TRACE (IPC_PUNIT_BIO > > S_CMD_BASE | 0x0e) > > +#define IPC_PUNIT_BIOS_WRITE_TELE_TRACE (IPC_PUNIT_BI > > OS_CMD_BASE | 0x0f) > > +#define IPC_PUNIT_BIOS_READ_TELE_EVENT (IPC_PUNIT_BIO > > S_CMD_BASE | 0x10) > > +#define IPC_PUNIT_BIOS_WRITE_TELE_EVENT (IPC_PUNIT_BI > > OS_CMD_BASE | 0x11) > > +#define IPC_PUNIT_BIOS_READ_MODULE_TEMP (IPC_PUNIT_BI > > OS_CMD_BASE | 0x12) > > +#define IPC_PUNIT_BIOS_RESERVED (IPC_PUNIT_BI > > OS_CMD_BASE | 0x13) > > +#define IPC_PUNIT_BIOS_READ_VOLTAGE_OVER (IPC_PUNIT_BIOS_CMD_ > > BASE | 0x14) > > +#define IPC_PUNIT_BIOS_WRITE_VOLTAGE_OVER (IPC_PUNIT_BIOS_CMD > > _BASE | 0x15) > > +#define IPC_PUNIT_BIOS_READ_RATIO_OVER (IPC_PUNIT_BIO > > S_CMD_BASE | 0x16) > > +#define IPC_PUNIT_BIOS_WRITE_RATIO_OVER (IPC_PUNIT_BI > > OS_CMD_BASE | 0x17) > > +#define IPC_PUNIT_BIOS_READ_VF_GL_CTRL (IPC_PUNIT_BIO > > S_CMD_BASE | 0x18) > > +#define IPC_PUNIT_BIOS_WRITE_VF_GL_CTRL (IPC_PUNIT_BI > > OS_CMD_BASE | 0x19) > > +#define IPC_PUNIT_BIOS_READ_FM_SOC_TEMP_THRESH (IPC_PUNIT_BIO > > S_CMD_BASE | 0x1a) > > +#define IPC_PUNIT_BIOS_WRITE_FM_SOC_TEMP_THRESH (IPC_PUNIT_BI > > OS_CMD_BASE | 0x1b) > > + > > +/* GT Driver => Pcode commands */ > > +#define IPC_PUNIT_GTD_ZERO (IPC_PUNIT_GTD_CMD > > _BASE | 0x00) > > +#define IPC_PUNIT_GTD_CONFIG (IPC_PUNIT_GTD_C > > MD_BASE | 0x01) > > +#define IPC_PUNIT_GTD_READ_ICCP_LIC_CDYN_SCAL (IPC_PUNIT_GTD_ > > CMD_BASE | 0x02) > > +#define IPC_PUNIT_GTD_WRITE_ICCP_LIC_CDYN_SCAL (IPC_PUNIT_GTD > > _CMD_BASE | 0x03) > > +#define IPC_PUNIT_GTD_GET_WM_VAL (IPC_PUNIT_GTD_CMD_B > > ASE | 0x06) > > +#define IPC_PUNIT_GTD_WRITE_CONFIG_WISHREQ (IPC_PUNIT_GTD_CMD > > _BASE | 0x07) > > +#define IPC_PUNIT_GTD_READ_REQ_DUTY_CYCLE (IPC_PUNIT_GTD_CMD_ > > BASE | 0x16) > > +#define IPC_PUNIT_GTD_DIS_VOL_FREQ_CHG_REQUEST (IPC_PUNIT_GTD > > _CMD_BASE | 0x17) > > +#define IPC_PUNIT_GTD_DYNA_DUTY_CYCLE_CTRL (IPC_PUNIT_GTD_CMD > > _BASE | 0x1a) > > +#define IPC_PUNIT_GTD_DYNA_DUTY_CYCLE_TUNING (IPC_PUNIT_GTD_C > > MD_BASE | 0x1c) > > + > > +/* ISP Driver => Pcode commands */ > > +#define IPC_PUNIT_ISPD_ZERO (IPC_PUNIT_ISPD_C > > MD_BASE | 0x00) > > +#define IPC_PUNIT_ISPD_CONFIG (IPC_PUNIT_ISPD > > _CMD_BASE | 0x01) > > +#define IPC_PUNIT_ISPD_GET_ISP_LTR_VAL (IPC_PUNIT_ISP > > D_CMD_BASE | 0x02) > > +#define IPC_PUNIT_ISPD_ACCESS_IU_FREQ_BOUNDS (IPC_PUNIT_ISPD_ > > CMD_BASE | 0x03) > > +#define IPC_PUNIT_ISPD_READ_CDYN_LEVEL (IPC_PUNIT_ISP > > D_CMD_BASE | 0x04) > > +#define IPC_PUNIT_ISPD_WRITE_CDYN_LEVEL (IPC_PUNIT_IS > > PD_CMD_BASE | 0x05) > > + > > +/* Error codes */ > > +#define IPC_PUNIT_ERR_SUCCESS 0 > > +#define IPC_PUNIT_ERR_INVALID_CMD 1 > > +#define IPC_PUNIT_ERR_INVALID_PARAMETER 2 > > +#define IPC_PUNIT_ERR_CMD_TIMEOUT 3 > > +#define IPC_PUNIT_ERR_CMD_LOCKED 4 > > +#define IPC_PUNIT_ERR_INVALID_VR_ID 5 > > +#define IPC_PUNIT_ERR_VR_ERR 6 > > + > > +#if IS_ENABLED(CONFIG_INTEL_PUNIT_IPC) > > + > > +int intel_punit_ipc_simple_command(int cmd, int para1, int para2); > > +int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2, u32 *in, > > u32 *out); > > + > > +#else > > + > > +static inline int intel_punit_ipc_simple_command(int cmd, > > + int para1, int > > para2) > > +{ > > + return -ENODEV; > > +} > > + > > +static inline int intel_punit_ipc_command(u32 cmd, u32 para1, u32 > > para2, > > + u32 *in, u32 *out) > > +{ > > + return -ENODEV; > > +} > > + > > +#endif /* CONFIG_INTEL_PUNIT_IPC */ > > + > > +#endif > > diff --git a/drivers/platform/x86/Kconfig > > b/drivers/platform/x86/Kconfig index 1089eaa..148ff88 100644 > > --- a/drivers/platform/x86/Kconfig > > +++ b/drivers/platform/x86/Kconfig > > @@ -944,4 +944,10 @@ config SURFACE_PRO3_BUTTON > > depends on ACPI && INPUT > > ---help--- > > This driver handles the power/home/volume buttons on the > > Microsoft Surface Pro 3 tablet. > > + > > +config INTEL_PUNIT_IPC > > + tristate "Intel P-Unit IPC Driver" > > + ---help--- > > + This driver provides support for Intel P-Unit Mailbox IPC > > mechanism, > > + which is used to bridge the communications between kernel > > and P-Unit. > > endif # X86_PLATFORM_DEVICES > > diff --git a/drivers/platform/x86/Makefile > > b/drivers/platform/x86/Makefile index 3ca78a3..5ee5425 100644 > > --- a/drivers/platform/x86/Makefile > > +++ b/drivers/platform/x86/Makefile > > @@ -62,3 +62,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o > > obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o > > obj-$(CONFIG_INTEL_PMC_IPC) += intel_pmc_ipc.o > > obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o > > +obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o > > diff --git a/drivers/platform/x86/intel_punit_ipc.c > > b/drivers/platform/x86/intel_punit_ipc.c > > new file mode 100644 > > index 0000000..e6354a7 > > --- /dev/null > > +++ b/drivers/platform/x86/intel_punit_ipc.c > > @@ -0,0 +1,336 @@ > > +/* > > + * Driver for the Intel P-Unit Mailbox IPC mechanism > > + * > > + * (C) Copyright 2015 Intel Corporation > > + * > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > + as > > + * published by the Free Software Foundation. > > + * > > + * The heart of the P-Unit is the Foxton microcontroller and its > > firmware, > > + * which provide mailbox interface for power management usage. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/acpi.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/interrupt.h> > > +#include <linux/platform_device.h> > > +#include <asm/intel_punit_ipc.h> > > + > > +/* IPC Mailbox registers */ > > +#define OFFSET_DATA_LOW 0x0 > > +#define OFFSET_DATA_HIGH 0x4 > > +/* bit field of interface register */ > > +#define CMD_RUN (1 << 31) > > +#define CMD_ERRCODE_MASK 0xFF > > #include <linux/bitops.h> > > …BIT(31) > …GENMASK(7,0) > > > +#define CMD_PARA1_SHIFT 8 > > +#define CMD_PARA2_SHIFT 16 > > + > > +#define CMD_TIMEOUT_SECONDS 1 > > + > > +enum { > > + DATA = 0, > > + INTERFACE, > > BASE_DATA > BASE_IFACE (or BASE_INTERFACE if you prefer, though better to be in > align across those two drivers) BASE_MAX > > > > +}; > > + > > +typedef struct { > > + struct device *dev; > > + struct mutex lock; > > + int irq; > > + struct completion cmd_complete; > > + /* base of interface and data registers */ > > + void __iomem *base[RESERVED_IPC][INTERFACE + 1]; > > …[][BASE_MAX]; > These are solid improvements. Qipeng, one more spin please and we can have this in next by the weekend. I'm personally much more confident in this driver. Thanks to Andriy for his careful review, and thank you for sticking with it. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html ��.n��������+%������w��{.n������_���v��z����n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
