* [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
@ 2015-12-07 16:55 Qipeng Zha
2015-12-07 16:55 ` [PATCH V8 2/2] platform:x86: add Intel P-Unit mailbox IPC driver Qipeng Zha
2015-12-07 23:45 ` [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Darren Hart
0 siblings, 2 replies; 13+ messages in thread
From: Qipeng Zha @ 2015-12-07 16:55 UTC (permalink / raw)
To: platform-driver-x86; +Cc: dvhart
BIOS restructure exported memory resources for Punit
in acpi table, So update resources for Punit.
Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
drivers/platform/x86/intel_pmc_ipc.c | 142 +++++++++++++++++++++++------------
1 file changed, 96 insertions(+), 46 deletions(-)
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 28b2a12..c699950 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -65,12 +65,16 @@
#define IPC_TRIGGER_MODE_IRQ true
/* exported resources from IFWI */
-#define PLAT_RESOURCE_IPC_INDEX 0
-#define PLAT_RESOURCE_IPC_SIZE 0x1000
-#define PLAT_RESOURCE_GCR_SIZE 0x1000
-#define PLAT_RESOURCE_PUNIT_DATA_INDEX 1
-#define PLAT_RESOURCE_PUNIT_INTER_INDEX 2
-#define PLAT_RESOURCE_ACPI_IO_INDEX 0
+#define PLAT_RES_IPC_INDEX 0
+#define PLAT_RES_IPC_SIZE 0x1000
+#define PLAT_RES_GCR_SIZE 0x1000
+#define PLAT_RES_PUNIT_BIOS_DATA_INDEX 1
+#define PLAT_RES_PUNIT_BIOS_INTER_INDEX 2
+#define PLAT_RES_PUNIT_ISP_DATA_INDEX 4
+#define PLAT_RES_PUNIT_ISP_INTER_INDEX 5
+#define PLAT_RES_PUNIT_GTD_DATA_INDEX 6
+#define PLAT_RES_PUNIT_GTD_INTER_INDEX 7
+#define PLAT_RES_ACPI_IO_INDEX 0
/*
* BIOS does not create an ACPI device for each PMC function,
@@ -105,10 +109,6 @@ static struct intel_pmc_ipc_dev {
int gcr_size;
/* punit */
- resource_size_t punit_base;
- int punit_size;
- resource_size_t punit_base2;
- int punit_size2;
struct platform_device *punit_dev;
} ipcdev;
@@ -444,9 +444,22 @@ static const struct attribute_group intel_ipc_group = {
.attrs = intel_ipc_attrs,
};
-#define PUNIT_RESOURCE_INTER 1
-static struct resource punit_res[] = {
- /* Punit */
+static struct resource punit_res_array[] = {
+ /* Punit BIOS */
+ {
+ .flags = IORESOURCE_MEM,
+ },
+ {
+ .flags = IORESOURCE_MEM,
+ },
+ /* Punit ISP */
+ {
+ .flags = IORESOURCE_MEM,
+ },
+ {
+ .flags = IORESOURCE_MEM,
+ },
+ /* Punit GTD */
{
.flags = IORESOURCE_MEM,
},
@@ -481,7 +494,6 @@ static struct itco_wdt_platform_data tco_info = {
static int ipc_create_punit_device(void)
{
struct platform_device *pdev;
- struct resource *res;
int ret;
pdev = platform_device_alloc(PUNIT_DEVICE_NAME, -1);
@@ -491,17 +503,8 @@ static int ipc_create_punit_device(void)
}
pdev->dev.parent = ipcdev.dev;
-
- res = punit_res;
- res->start = ipcdev.punit_base;
- res->end = res->start + ipcdev.punit_size - 1;
-
- res = punit_res + PUNIT_RESOURCE_INTER;
- res->start = ipcdev.punit_base2;
- res->end = res->start + ipcdev.punit_size2 - 1;
-
- ret = platform_device_add_resources(pdev, punit_res,
- ARRAY_SIZE(punit_res));
+ ret = platform_device_add_resources(pdev, punit_res_array,
+ ARRAY_SIZE(punit_res_array));
if (ret) {
dev_err(ipcdev.dev, "Failed to add platform punit resources\n");
goto err;
@@ -590,12 +593,12 @@ static int ipc_create_pmc_devices(void)
static int ipc_plat_get_res(struct platform_device *pdev)
{
- struct resource *res;
+ struct resource *res, *punit_res;
void __iomem *addr;
int size;
res = platform_get_resource(pdev, IORESOURCE_IO,
- PLAT_RESOURCE_ACPI_IO_INDEX);
+ PLAT_RES_ACPI_IO_INDEX);
if (!res) {
dev_err(&pdev->dev, "Failed to get io resource\n");
return -ENXIO;
@@ -606,37 +609,84 @@ static int ipc_plat_get_res(struct platform_device *pdev)
dev_info(&pdev->dev, "io res: %llx %x\n",
(long long)res->start, (int)resource_size(res));
+ punit_res = punit_res_array;
res = platform_get_resource(pdev, IORESOURCE_MEM,
- PLAT_RESOURCE_PUNIT_DATA_INDEX);
+ PLAT_RES_PUNIT_BIOS_DATA_INDEX);
if (!res) {
- dev_err(&pdev->dev, "Failed to get punit resource\n");
+ dev_err(&pdev->dev, "Failed to get res of punit BIOS data\n");
return -ENXIO;
}
- size = resource_size(res);
- ipcdev.punit_base = res->start;
- ipcdev.punit_size = size;
- dev_info(&pdev->dev, "punit data res: %llx %x\n",
+ punit_res->start = res->start;
+ punit_res->end = res->start + resource_size(res) - 1;
+ dev_info(&pdev->dev, "punit BIOS data res: %llx %x\n",
(long long)res->start, (int)resource_size(res));
res = platform_get_resource(pdev, IORESOURCE_MEM,
- PLAT_RESOURCE_PUNIT_INTER_INDEX);
+ PLAT_RES_PUNIT_BIOS_INTER_INDEX);
if (!res) {
- dev_err(&pdev->dev, "Failed to get punit inter resource\n");
+ dev_err(&pdev->dev, "Failed to get res of punit BIOS inter\n");
return -ENXIO;
}
- size = resource_size(res);
- ipcdev.punit_base2 = res->start;
- ipcdev.punit_size2 = size;
- dev_info(&pdev->dev, "punit interface res: %llx %x\n",
+ punit_res++;
+ punit_res->start = res->start;
+ punit_res->end = res->start + resource_size(res) - 1;
+ dev_info(&pdev->dev, "punit BIOS interface res: %llx %x\n",
+ (long long)res->start, (int)resource_size(res));
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM,
+ PLAT_RES_PUNIT_ISP_DATA_INDEX);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get res of punit ISP data\n");
+ return -ENXIO;
+ }
+ punit_res++;
+ punit_res->start = res->start;
+ punit_res->end = res->start + resource_size(res) - 1;
+ dev_info(&pdev->dev, "punit ISP data res: %llx %x\n",
(long long)res->start, (int)resource_size(res));
res = platform_get_resource(pdev, IORESOURCE_MEM,
- PLAT_RESOURCE_IPC_INDEX);
+ PLAT_RES_PUNIT_ISP_INTER_INDEX);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get res of punit ISP inter\n");
+ return -ENXIO;
+ }
+ punit_res++;
+ punit_res->start = res->start;
+ punit_res->end = res->start + resource_size(res) - 1;
+ dev_info(&pdev->dev, "punit ISP interface res: %llx %x\n",
+ (long long)res->start, (int)resource_size(res));
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM,
+ PLAT_RES_PUNIT_GTD_DATA_INDEX);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get res of punit GTD data\n");
+ return -ENXIO;
+ }
+ punit_res++;
+ punit_res->start = res->start;
+ punit_res->end = res->start + resource_size(res) - 1;
+ dev_info(&pdev->dev, "punit GTD data res: %llx %x\n",
+ (long long)res->start, (int)resource_size(res));
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM,
+ PLAT_RES_PUNIT_GTD_INTER_INDEX);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get res of punit GTD inter\n");
+ return -ENXIO;
+ }
+ punit_res++;
+ punit_res->start = res->start;
+ punit_res->end = res->start + resource_size(res) - 1;
+ dev_info(&pdev->dev, "punit GTD interface res: %llx %x\n",
+ (long long)res->start, (int)resource_size(res));
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, PLAT_RES_IPC_INDEX);
if (!res) {
dev_err(&pdev->dev, "Failed to get ipc resource\n");
return -ENXIO;
}
- size = PLAT_RESOURCE_IPC_SIZE;
+ size = PLAT_RES_IPC_SIZE;
if (!request_mem_region(res->start, size, pdev->name)) {
dev_err(&pdev->dev, "Failed to request ipc resource\n");
return -EBUSY;
@@ -650,7 +700,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
ipcdev.ipc_base = addr;
ipcdev.gcr_base = res->start + size;
- ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
+ ipcdev.gcr_size = PLAT_RES_GCR_SIZE;
dev_info(&pdev->dev, "ipc res: %llx %x\n",
(long long)res->start, (int)resource_size(res));
@@ -714,9 +764,9 @@ err_irq:
err_device:
iounmap(ipcdev.ipc_base);
res = platform_get_resource(pdev, IORESOURCE_MEM,
- PLAT_RESOURCE_IPC_INDEX);
+ PLAT_RES_IPC_INDEX);
if (res)
- release_mem_region(res->start, PLAT_RESOURCE_IPC_SIZE);
+ release_mem_region(res->start, PLAT_RES_IPC_SIZE);
return ret;
}
@@ -730,9 +780,9 @@ static int ipc_plat_remove(struct platform_device *pdev)
platform_device_unregister(ipcdev.punit_dev);
iounmap(ipcdev.ipc_base);
res = platform_get_resource(pdev, IORESOURCE_MEM,
- PLAT_RESOURCE_IPC_INDEX);
+ PLAT_RES_IPC_INDEX);
if (res)
- release_mem_region(res->start, PLAT_RESOURCE_IPC_SIZE);
+ release_mem_region(res->start, PLAT_RES_IPC_SIZE);
ipcdev.dev = NULL;
return 0;
}
--
1.8.3.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V8 2/2] platform:x86: add Intel P-Unit mailbox IPC driver
2015-12-07 16:55 [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Qipeng Zha
@ 2015-12-07 16:55 ` Qipeng Zha
2015-12-07 23:45 ` [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Darren Hart
1 sibling, 0 replies; 13+ messages in thread
From: Qipeng Zha @ 2015-12-07 16:55 UTC (permalink / raw)
To: platform-driver-x86; +Cc: dvhart
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@intel.com>
---
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 | 348 +++++++++++++++++++++++++++++++++
5 files changed, 459 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@intel.com>
L: platform-driver-x86@vger.kernel.org
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@linux-mips.org>
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_CMD_BASE | 0x00)
+#define IPC_PUNIT_BIOS_VR_INTERFACE (IPC_PUNIT_BIOS_CMD_BASE | 0x01)
+#define IPC_PUNIT_BIOS_READ_PCS (IPC_PUNIT_BIOS_CMD_BASE | 0x02)
+#define IPC_PUNIT_BIOS_WRITE_PCS (IPC_PUNIT_BIOS_CMD_BASE | 0x03)
+#define IPC_PUNIT_BIOS_READ_PCU_CONFIG (IPC_PUNIT_BIOS_CMD_BASE | 0x04)
+#define IPC_PUNIT_BIOS_WRITE_PCU_CONFIG (IPC_PUNIT_BIOS_CMD_BASE | 0x05)
+#define IPC_PUNIT_BIOS_READ_PL1_SETTING (IPC_PUNIT_BIOS_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_BIOS_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_CMD_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_CMD_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_BIOS_CMD_BASE | 0x0e)
+#define IPC_PUNIT_BIOS_WRITE_TELE_TRACE (IPC_PUNIT_BIOS_CMD_BASE | 0x0f)
+#define IPC_PUNIT_BIOS_READ_TELE_EVENT (IPC_PUNIT_BIOS_CMD_BASE | 0x10)
+#define IPC_PUNIT_BIOS_WRITE_TELE_EVENT (IPC_PUNIT_BIOS_CMD_BASE | 0x11)
+#define IPC_PUNIT_BIOS_READ_MODULE_TEMP (IPC_PUNIT_BIOS_CMD_BASE | 0x12)
+#define IPC_PUNIT_BIOS_RESERVED (IPC_PUNIT_BIOS_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_BIOS_CMD_BASE | 0x16)
+#define IPC_PUNIT_BIOS_WRITE_RATIO_OVER (IPC_PUNIT_BIOS_CMD_BASE | 0x17)
+#define IPC_PUNIT_BIOS_READ_VF_GL_CTRL (IPC_PUNIT_BIOS_CMD_BASE | 0x18)
+#define IPC_PUNIT_BIOS_WRITE_VF_GL_CTRL (IPC_PUNIT_BIOS_CMD_BASE | 0x19)
+#define IPC_PUNIT_BIOS_READ_FM_SOC_TEMP_THRESH (IPC_PUNIT_BIOS_CMD_BASE | 0x1a)
+#define IPC_PUNIT_BIOS_WRITE_FM_SOC_TEMP_THRESH (IPC_PUNIT_BIOS_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_CMD_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_BASE | 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_CMD_BASE | 0x1c)
+
+/* ISP Driver => Pcode commands */
+#define IPC_PUNIT_ISPD_ZERO (IPC_PUNIT_ISPD_CMD_BASE | 0x00)
+#define IPC_PUNIT_ISPD_CONFIG (IPC_PUNIT_ISPD_CMD_BASE | 0x01)
+#define IPC_PUNIT_ISPD_GET_ISP_LTR_VAL (IPC_PUNIT_ISPD_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_ISPD_CMD_BASE | 0x04)
+#define IPC_PUNIT_ISPD_WRITE_CDYN_LEVEL (IPC_PUNIT_ISPD_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..345ad71
--- /dev/null
+++ b/drivers/platform/x86/intel_punit_ipc.c
@@ -0,0 +1,348 @@
+/*
+ * 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
+#define CMD_PARA1_SHIFT 8
+#define CMD_PARA2_SHIFT 16
+
+#define CMD_TIMEOUT_SECONDS 1
+
+enum {
+ DATA = 0,
+ INTERFACE,
+};
+
+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];
+ IPC_TYPE type;
+} IPC_DEV;
+
+static IPC_DEV *punit_ipcdev;
+
+static inline u32 ipc_read_status(IPC_DEV *ipcdev, IPC_TYPE type)
+{
+ return readl(ipcdev->base[type][INTERFACE]);
+}
+
+static inline void ipc_write_cmd(IPC_DEV *ipcdev, IPC_TYPE type, u32 cmd)
+{
+ writel(cmd, ipcdev->base[type][INTERFACE]);
+}
+
+static inline u32 ipc_read_data_low(IPC_DEV *ipcdev, IPC_TYPE type)
+{
+ return readl(ipcdev->base[type][DATA] + OFFSET_DATA_LOW);
+}
+
+static inline u32 ipc_read_data_high(IPC_DEV *ipcdev, IPC_TYPE type)
+{
+ return readl(ipcdev->base[type][DATA] + OFFSET_DATA_HIGH);
+}
+
+static inline void ipc_write_data_low(IPC_DEV *ipcdev, IPC_TYPE type, u32 data)
+{
+ writel(data, ipcdev->base[type][DATA] + OFFSET_DATA_LOW);
+}
+
+static inline void ipc_write_data_high(IPC_DEV *ipcdev, IPC_TYPE type, u32 data)
+{
+ writel(data, ipcdev->base[type][DATA] + OFFSET_DATA_HIGH);
+}
+
+static const char *ipc_err_string(int error)
+{
+ if (error == IPC_PUNIT_ERR_SUCCESS)
+ return "no error";
+ else if (error == IPC_PUNIT_ERR_INVALID_CMD)
+ return "invalid command";
+ else if (error == IPC_PUNIT_ERR_INVALID_PARAMETER)
+ return "invalid parameter";
+ else if (error == IPC_PUNIT_ERR_CMD_TIMEOUT)
+ return "command timeout";
+ else if (error == IPC_PUNIT_ERR_CMD_LOCKED)
+ return "command locked";
+ else if (error == IPC_PUNIT_ERR_INVALID_VR_ID)
+ return "invalid vr id";
+ else if (error == IPC_PUNIT_ERR_VR_ERR)
+ return "vr error";
+ else
+ return "unknown error";
+}
+
+static int intel_punit_ipc_check_status(IPC_DEV *ipcdev, IPC_TYPE type)
+{
+ int loops = CMD_TIMEOUT_SECONDS * USEC_PER_SEC;
+ int errcode;
+ int status;
+
+ if (ipcdev->irq) {
+ if (!wait_for_completion_timeout(&ipcdev->cmd_complete,
+ CMD_TIMEOUT_SECONDS * HZ)) {
+ dev_err(ipcdev->dev, "IPC timed out\n");
+ return -ETIMEDOUT;
+ }
+ } else {
+ while ((ipc_read_status(ipcdev, type) & CMD_RUN) && --loops)
+ udelay(1);
+ if (!loops) {
+ dev_err(ipcdev->dev, "IPC timed out\n");
+ return -ETIMEDOUT;
+ }
+ }
+
+ status = ipc_read_status(ipcdev, type);
+ errcode = status & CMD_ERRCODE_MASK;
+ if (errcode) {
+ dev_err(ipcdev->dev, "IPC failed: %s, IPC_STS=0x%x\n",
+ ipc_err_string(errcode), status);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+/**
+ * intel_punit_ipc_simple_command() - Simple IPC command
+ * @cmd: IPC command code.
+ * @para1: First 8bit parameter, set 0 if not used.
+ * @para2: Second 8bit parameter, set 0 if not used.
+ *
+ * Send a IPC command to P-Unit when there is no data transaction
+ *
+ * Return: IPC error code or 0 on success.
+ */
+int intel_punit_ipc_simple_command(int cmd, int para1, int para2)
+{
+ IPC_DEV *ipcdev = punit_ipcdev;
+ IPC_TYPE type;
+ u32 val;
+ int ret;
+
+ mutex_lock(&ipcdev->lock);
+
+ reinit_completion(&ipcdev->cmd_complete);
+ type = (cmd & IPC_PUNIT_CMD_TYPE_MASK) >> IPC_TYPE_OFFSET;
+
+ val = cmd & ~IPC_PUNIT_CMD_TYPE_MASK;
+ val |= CMD_RUN | para2 << CMD_PARA2_SHIFT | para1 << CMD_PARA1_SHIFT;
+ ipc_write_cmd(ipcdev, type, val);
+ ret = intel_punit_ipc_check_status(ipcdev, type);
+
+ mutex_unlock(&ipcdev->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_punit_ipc_simple_command);
+
+/**
+ * intel_punit_ipc_command() - IPC command with data and pointers
+ * @cmd: IPC command code.
+ * @para1: First 8bit parameter, set 0 if not used.
+ * @para2: Second 8bit parameter, set 0 if not used.
+ * @in: Input data, 32bit for BIOS cmd, two 32bit for GTD and ISPD.
+ * @out: Output data.
+ *
+ * Send a IPC command to P-Unit with data transaction
+ *
+ * Return: IPC error code or 0 on success.
+ */
+int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2, u32 *in, u32 *out)
+{
+ IPC_DEV *ipcdev = punit_ipcdev;
+ IPC_TYPE type;
+ u32 val;
+ int ret;
+
+ mutex_lock(&ipcdev->lock);
+
+ reinit_completion(&ipcdev->cmd_complete);
+ type = (cmd & IPC_PUNIT_CMD_TYPE_MASK) >> IPC_TYPE_OFFSET;
+ ipc_write_data_low(ipcdev, type, *in);
+
+ if (type == GTDRIVER_IPC || type == ISPDRIVER_IPC)
+ ipc_write_data_high(ipcdev, type, *++in);
+
+ val = cmd & ~IPC_PUNIT_CMD_TYPE_MASK;
+ val |= CMD_RUN | para2 << CMD_PARA2_SHIFT | para1 << CMD_PARA1_SHIFT;
+ ipc_write_cmd(ipcdev, type, val);
+
+ ret = intel_punit_ipc_check_status(ipcdev, type);
+ if (ret)
+ goto out;
+ *out = ipc_read_data_low(ipcdev, type);
+
+ if (type == GTDRIVER_IPC || type == ISPDRIVER_IPC)
+ *++out = ipc_read_data_high(ipcdev, type);
+
+out:
+ mutex_unlock(&ipcdev->lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(intel_punit_ipc_command);
+
+static irqreturn_t intel_punit_ioc(int irq, void *dev_id)
+{
+ IPC_DEV *ipcdev = (IPC_DEV *)dev_id;
+
+ complete(&ipcdev->cmd_complete);
+ return IRQ_HANDLED;
+}
+
+static int intel_punit_get_bars(struct platform_device *pdev)
+{
+ struct resource *res;
+ void __iomem *addr;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ addr = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(addr)) {
+ dev_err(&pdev->dev, "Failed to map resouce for BIOS DATA\n");
+ return PTR_ERR(addr);
+ }
+ punit_ipcdev->base[BIOS_IPC][DATA] = addr;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ addr = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(addr)) {
+ dev_err(&pdev->dev, "Failed to map resouce for BIOS inter\n");
+ return PTR_ERR(addr);
+ }
+ punit_ipcdev->base[BIOS_IPC][INTERFACE] = addr;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+ addr = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(addr)) {
+ dev_err(&pdev->dev, "Failed to map resouce for ISP DATA\n");
+ return PTR_ERR(addr);
+ }
+ punit_ipcdev->base[ISPDRIVER_IPC][DATA] = addr;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
+ addr = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(addr)) {
+ dev_err(&pdev->dev, "Failed to map resouce for ISP inter\n");
+ return PTR_ERR(addr);
+ }
+ punit_ipcdev->base[ISPDRIVER_IPC][INTERFACE] = addr;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 4);
+ addr = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(addr)) {
+ dev_err(&pdev->dev, "Failed to map resouce for GTD DATA\n");
+ return PTR_ERR(addr);
+ }
+ punit_ipcdev->base[GTDRIVER_IPC][DATA] = addr;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 5);
+ addr = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(addr)) {
+ dev_err(&pdev->dev, "Failed to map resouce for GTD inter\n");
+ return PTR_ERR(addr);
+ }
+ punit_ipcdev->base[GTDRIVER_IPC][INTERFACE] = addr;
+
+ return 0;
+}
+
+static int intel_punit_ipc_probe(struct platform_device *pdev)
+{
+ int irq, ret;
+
+ punit_ipcdev = devm_kzalloc(&pdev->dev,
+ sizeof(*punit_ipcdev), GFP_KERNEL);
+ if (!punit_ipcdev)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, punit_ipcdev);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ punit_ipcdev->irq = 0;
+ dev_warn(&pdev->dev, "Invalid IRQ, using polling mode\n");
+ } else {
+ ret = devm_request_irq(&pdev->dev, irq, intel_punit_ioc,
+ IRQF_NO_SUSPEND, "intel_punit_ipc",
+ &punit_ipcdev);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to request irq: %d\n", irq);
+ return ret;
+ }
+ punit_ipcdev->irq = irq;
+ }
+
+ ret = intel_punit_get_bars(pdev);
+ if (ret)
+ goto out;
+
+ punit_ipcdev->dev = &pdev->dev;
+ mutex_init(&punit_ipcdev->lock);
+ init_completion(&punit_ipcdev->cmd_complete);
+
+out:
+ return ret;
+}
+
+static int intel_punit_ipc_remove(struct platform_device *pdev)
+{
+ return 0;
+}
+
+static const struct acpi_device_id punit_ipc_acpi_ids[] = {
+ { "INT34D4", 0 },
+ { }
+};
+
+static struct platform_driver intel_punit_ipc_driver = {
+ .probe = intel_punit_ipc_probe,
+ .remove = intel_punit_ipc_remove,
+ .driver = {
+ .name = "intel_punit_ipc",
+ .acpi_match_table = ACPI_PTR(punit_ipc_acpi_ids),
+ },
+};
+
+static int __init intel_punit_ipc_init(void)
+{
+ return platform_driver_register(&intel_punit_ipc_driver);
+}
+
+static void __exit intel_punit_ipc_exit(void)
+{
+ platform_driver_unregister(&intel_punit_ipc_driver);
+}
+
+MODULE_AUTHOR("Zha Qipeng <qipeng.zha@intel.com>");
+MODULE_DESCRIPTION("Intel P-Unit IPC driver");
+MODULE_LICENSE("GPL v2");
+
+/* Some modules are dependent on this, so init earlier */
+fs_initcall(intel_punit_ipc_init);
+module_exit(intel_punit_ipc_exit);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-07 16:55 [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Qipeng Zha
2015-12-07 16:55 ` [PATCH V8 2/2] platform:x86: add Intel P-Unit mailbox IPC driver Qipeng Zha
@ 2015-12-07 23:45 ` Darren Hart
2015-12-08 11:10 ` Andy Shevchenko
` (2 more replies)
1 sibling, 3 replies; 13+ messages in thread
From: Darren Hart @ 2015-12-07 23:45 UTC (permalink / raw)
To: Qipeng Zha, Andy Shevchenko; +Cc: platform-driver-x86
On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
> BIOS restructure exported memory resources for Punit
> in acpi table, So update resources for Punit.
>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
Thank you for the update Qipeng. I will review shortly.
+Andriy who originally raised the concern over the ACPI resource assumptions in
the previous version. Andriy, this resource allocation looks to be a substantial
improvement to me. Do you have any further concerns?
> ---
> drivers/platform/x86/intel_pmc_ipc.c | 142 +++++++++++++++++++++++------------
> 1 file changed, 96 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 28b2a12..c699950 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -65,12 +65,16 @@
> #define IPC_TRIGGER_MODE_IRQ true
>
> /* exported resources from IFWI */
> -#define PLAT_RESOURCE_IPC_INDEX 0
> -#define PLAT_RESOURCE_IPC_SIZE 0x1000
> -#define PLAT_RESOURCE_GCR_SIZE 0x1000
> -#define PLAT_RESOURCE_PUNIT_DATA_INDEX 1
> -#define PLAT_RESOURCE_PUNIT_INTER_INDEX 2
> -#define PLAT_RESOURCE_ACPI_IO_INDEX 0
> +#define PLAT_RES_IPC_INDEX 0
> +#define PLAT_RES_IPC_SIZE 0x1000
> +#define PLAT_RES_GCR_SIZE 0x1000
> +#define PLAT_RES_PUNIT_BIOS_DATA_INDEX 1
> +#define PLAT_RES_PUNIT_BIOS_INTER_INDEX 2
> +#define PLAT_RES_PUNIT_ISP_DATA_INDEX 4
> +#define PLAT_RES_PUNIT_ISP_INTER_INDEX 5
> +#define PLAT_RES_PUNIT_GTD_DATA_INDEX 6
> +#define PLAT_RES_PUNIT_GTD_INTER_INDEX 7
> +#define PLAT_RES_ACPI_IO_INDEX 0
>
> /*
> * BIOS does not create an ACPI device for each PMC function,
> @@ -105,10 +109,6 @@ static struct intel_pmc_ipc_dev {
> int gcr_size;
>
> /* punit */
> - resource_size_t punit_base;
> - int punit_size;
> - resource_size_t punit_base2;
> - int punit_size2;
> struct platform_device *punit_dev;
> } ipcdev;
>
> @@ -444,9 +444,22 @@ static const struct attribute_group intel_ipc_group = {
> .attrs = intel_ipc_attrs,
> };
>
> -#define PUNIT_RESOURCE_INTER 1
> -static struct resource punit_res[] = {
> - /* Punit */
> +static struct resource punit_res_array[] = {
> + /* Punit BIOS */
> + {
> + .flags = IORESOURCE_MEM,
> + },
> + {
> + .flags = IORESOURCE_MEM,
> + },
> + /* Punit ISP */
> + {
> + .flags = IORESOURCE_MEM,
> + },
> + {
> + .flags = IORESOURCE_MEM,
> + },
> + /* Punit GTD */
> {
> .flags = IORESOURCE_MEM,
> },
> @@ -481,7 +494,6 @@ static struct itco_wdt_platform_data tco_info = {
> static int ipc_create_punit_device(void)
> {
> struct platform_device *pdev;
> - struct resource *res;
> int ret;
>
> pdev = platform_device_alloc(PUNIT_DEVICE_NAME, -1);
> @@ -491,17 +503,8 @@ static int ipc_create_punit_device(void)
> }
>
> pdev->dev.parent = ipcdev.dev;
> -
> - res = punit_res;
> - res->start = ipcdev.punit_base;
> - res->end = res->start + ipcdev.punit_size - 1;
> -
> - res = punit_res + PUNIT_RESOURCE_INTER;
> - res->start = ipcdev.punit_base2;
> - res->end = res->start + ipcdev.punit_size2 - 1;
> -
> - ret = platform_device_add_resources(pdev, punit_res,
> - ARRAY_SIZE(punit_res));
> + ret = platform_device_add_resources(pdev, punit_res_array,
> + ARRAY_SIZE(punit_res_array));
> if (ret) {
> dev_err(ipcdev.dev, "Failed to add platform punit resources\n");
> goto err;
> @@ -590,12 +593,12 @@ static int ipc_create_pmc_devices(void)
>
> static int ipc_plat_get_res(struct platform_device *pdev)
> {
> - struct resource *res;
> + struct resource *res, *punit_res;
> void __iomem *addr;
> int size;
>
> res = platform_get_resource(pdev, IORESOURCE_IO,
> - PLAT_RESOURCE_ACPI_IO_INDEX);
> + PLAT_RES_ACPI_IO_INDEX);
> if (!res) {
> dev_err(&pdev->dev, "Failed to get io resource\n");
> return -ENXIO;
> @@ -606,37 +609,84 @@ static int ipc_plat_get_res(struct platform_device *pdev)
> dev_info(&pdev->dev, "io res: %llx %x\n",
> (long long)res->start, (int)resource_size(res));
>
> + punit_res = punit_res_array;
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_PUNIT_DATA_INDEX);
> + PLAT_RES_PUNIT_BIOS_DATA_INDEX);
> if (!res) {
> - dev_err(&pdev->dev, "Failed to get punit resource\n");
> + dev_err(&pdev->dev, "Failed to get res of punit BIOS data\n");
> return -ENXIO;
> }
> - size = resource_size(res);
> - ipcdev.punit_base = res->start;
> - ipcdev.punit_size = size;
> - dev_info(&pdev->dev, "punit data res: %llx %x\n",
> + punit_res->start = res->start;
> + punit_res->end = res->start + resource_size(res) - 1;
> + dev_info(&pdev->dev, "punit BIOS data res: %llx %x\n",
> (long long)res->start, (int)resource_size(res));
>
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_PUNIT_INTER_INDEX);
> + PLAT_RES_PUNIT_BIOS_INTER_INDEX);
> if (!res) {
> - dev_err(&pdev->dev, "Failed to get punit inter resource\n");
> + dev_err(&pdev->dev, "Failed to get res of punit BIOS inter\n");
> return -ENXIO;
> }
> - size = resource_size(res);
> - ipcdev.punit_base2 = res->start;
> - ipcdev.punit_size2 = size;
> - dev_info(&pdev->dev, "punit interface res: %llx %x\n",
> + punit_res++;
> + punit_res->start = res->start;
> + punit_res->end = res->start + resource_size(res) - 1;
> + dev_info(&pdev->dev, "punit BIOS interface res: %llx %x\n",
> + (long long)res->start, (int)resource_size(res));
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM,
> + PLAT_RES_PUNIT_ISP_DATA_INDEX);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get res of punit ISP data\n");
> + return -ENXIO;
> + }
> + punit_res++;
> + punit_res->start = res->start;
> + punit_res->end = res->start + resource_size(res) - 1;
> + dev_info(&pdev->dev, "punit ISP data res: %llx %x\n",
> (long long)res->start, (int)resource_size(res));
>
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_IPC_INDEX);
> + PLAT_RES_PUNIT_ISP_INTER_INDEX);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get res of punit ISP inter\n");
> + return -ENXIO;
> + }
> + punit_res++;
> + punit_res->start = res->start;
> + punit_res->end = res->start + resource_size(res) - 1;
> + dev_info(&pdev->dev, "punit ISP interface res: %llx %x\n",
> + (long long)res->start, (int)resource_size(res));
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM,
> + PLAT_RES_PUNIT_GTD_DATA_INDEX);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get res of punit GTD data\n");
> + return -ENXIO;
> + }
> + punit_res++;
> + punit_res->start = res->start;
> + punit_res->end = res->start + resource_size(res) - 1;
> + dev_info(&pdev->dev, "punit GTD data res: %llx %x\n",
> + (long long)res->start, (int)resource_size(res));
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM,
> + PLAT_RES_PUNIT_GTD_INTER_INDEX);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get res of punit GTD inter\n");
> + return -ENXIO;
> + }
> + punit_res++;
> + punit_res->start = res->start;
> + punit_res->end = res->start + resource_size(res) - 1;
> + dev_info(&pdev->dev, "punit GTD interface res: %llx %x\n",
> + (long long)res->start, (int)resource_size(res));
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, PLAT_RES_IPC_INDEX);
> if (!res) {
> dev_err(&pdev->dev, "Failed to get ipc resource\n");
> return -ENXIO;
> }
> - size = PLAT_RESOURCE_IPC_SIZE;
> + size = PLAT_RES_IPC_SIZE;
> if (!request_mem_region(res->start, size, pdev->name)) {
> dev_err(&pdev->dev, "Failed to request ipc resource\n");
> return -EBUSY;
> @@ -650,7 +700,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
> ipcdev.ipc_base = addr;
>
> ipcdev.gcr_base = res->start + size;
> - ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
> + ipcdev.gcr_size = PLAT_RES_GCR_SIZE;
> dev_info(&pdev->dev, "ipc res: %llx %x\n",
> (long long)res->start, (int)resource_size(res));
>
> @@ -714,9 +764,9 @@ err_irq:
> err_device:
> iounmap(ipcdev.ipc_base);
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_IPC_INDEX);
> + PLAT_RES_IPC_INDEX);
> if (res)
> - release_mem_region(res->start, PLAT_RESOURCE_IPC_SIZE);
> + release_mem_region(res->start, PLAT_RES_IPC_SIZE);
> return ret;
> }
>
> @@ -730,9 +780,9 @@ static int ipc_plat_remove(struct platform_device *pdev)
> platform_device_unregister(ipcdev.punit_dev);
> iounmap(ipcdev.ipc_base);
> res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_IPC_INDEX);
> + PLAT_RES_IPC_INDEX);
> if (res)
> - release_mem_region(res->start, PLAT_RESOURCE_IPC_SIZE);
> + release_mem_region(res->start, PLAT_RES_IPC_SIZE);
> ipcdev.dev = NULL;
> return 0;
> }
> --
> 1.8.3.2
>
>
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-07 23:45 ` [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Darren Hart
@ 2015-12-08 11:10 ` Andy Shevchenko
2015-12-08 12:57 ` Andy Shevchenko
2015-12-08 13:19 ` Andy Shevchenko
2 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2015-12-08 11:10 UTC (permalink / raw)
To: Darren Hart, Qipeng Zha; +Cc: platform-driver-x86
On Mon, 2015-12-07 at 15:45 -0800, Darren Hart wrote:
> On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
> > BIOS restructure exported memory resources for Punit
> > in acpi table, So update resources for Punit.
> >
> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
>
> Thank you for the update Qipeng. I will review shortly.
>
> +Andriy who originally raised the concern over the ACPI resource
> assumptions in
> the previous version. Andriy, this resource allocation looks to be a
> substantial
> improvement to me. Do you have any further concerns?
I will check it later.
Hmm… I am not subscribed to platform-driver-x86@ and wasn't in Cc list,
so it might delay me response.
>
> > ---
> > drivers/platform/x86/intel_pmc_ipc.c | 142
> > +++++++++++++++++++++++------------
> > 1 file changed, 96 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel_pmc_ipc.c
> > b/drivers/platform/x86/intel_pmc_ipc.c
> > index 28b2a12..c699950 100644
> > --- a/drivers/platform/x86/intel_pmc_ipc.c
> > +++ b/drivers/platform/x86/intel_pmc_ipc.c
> > @@ -65,12 +65,16 @@
> > #define IPC_TRIGGER_MODE_IRQ true
> >
> > /* exported resources from IFWI */
> > -#define PLAT_RESOURCE_IPC_INDEX 0
> > -#define PLAT_RESOURCE_IPC_SIZE 0x1000
> > -#define PLAT_RESOURCE_GCR_SIZE 0x1000
> > -#define PLAT_RESOURCE_PUNIT_DATA_INDEX 1
> > -#define PLAT_RESOURCE_PUNIT_INTER_INDEX 2
> > -#define PLAT_RESOURCE_ACPI_IO_INDEX 0
> > +#define PLAT_RES_IPC_INDEX 0
> > +#define PLAT_RES_IPC_SIZE 0x1000
> > +#define PLAT_RES_GCR_SIZE 0x1000
> > +#define PLAT_RES_PUNIT_BIOS_DATA_INDEX 1
> > +#define PLAT_RES_PUNIT_BIOS_INTER_INDEX 2
> > +#define PLAT_RES_PUNIT_ISP_DATA_INDEX 4
> > +#define PLAT_RES_PUNIT_ISP_INTER_INDEX 5
> > +#define PLAT_RES_PUNIT_GTD_DATA_INDEX 6
> > +#define PLAT_RES_PUNIT_GTD_INTER_INDEX 7
> > +#define PLAT_RES_ACPI_IO_INDEX 0
> >
> > /*
> > * BIOS does not create an ACPI device for each PMC function,
> > @@ -105,10 +109,6 @@ static struct intel_pmc_ipc_dev {
> > int gcr_size;
> >
> > /* punit */
> > - resource_size_t punit_base;
> > - int punit_size;
> > - resource_size_t punit_base2;
> > - int punit_size2;
> > struct platform_device *punit_dev;
> > } ipcdev;
> >
> > @@ -444,9 +444,22 @@ static const struct attribute_group
> > intel_ipc_group = {
> > .attrs = intel_ipc_attrs,
> > };
> >
> > -#define PUNIT_RESOURCE_INTER 1
> > -static struct resource punit_res[] = {
> > - /* Punit */
> > +static struct resource punit_res_array[] = {
> > + /* Punit BIOS */
> > + {
> > + .flags = IORESOURCE_MEM,
> > + },
> > + {
> > + .flags = IORESOURCE_MEM,
> > + },
> > + /* Punit ISP */
> > + {
> > + .flags = IORESOURCE_MEM,
> > + },
> > + {
> > + .flags = IORESOURCE_MEM,
> > + },
> > + /* Punit GTD */
> > {
> > .flags = IORESOURCE_MEM,
> > },
> > @@ -481,7 +494,6 @@ static struct itco_wdt_platform_data tco_info =
> > {
> > static int ipc_create_punit_device(void)
> > {
> > struct platform_device *pdev;
> > - struct resource *res;
> > int ret;
> >
> > pdev = platform_device_alloc(PUNIT_DEVICE_NAME, -1);
> > @@ -491,17 +503,8 @@ static int ipc_create_punit_device(void)
> > }
> >
> > pdev->dev.parent = ipcdev.dev;
> > -
> > - res = punit_res;
> > - res->start = ipcdev.punit_base;
> > - res->end = res->start + ipcdev.punit_size - 1;
> > -
> > - res = punit_res + PUNIT_RESOURCE_INTER;
> > - res->start = ipcdev.punit_base2;
> > - res->end = res->start + ipcdev.punit_size2 - 1;
> > -
> > - ret = platform_device_add_resources(pdev, punit_res,
> > - ARRAY_SIZE(punit_res))
> > ;
> > + ret = platform_device_add_resources(pdev, punit_res_array,
> > + ARRAY_SIZE(punit_res_a
> > rray));
> > if (ret) {
> > dev_err(ipcdev.dev, "Failed to add platform punit
> > resources\n");
> > goto err;
> > @@ -590,12 +593,12 @@ static int ipc_create_pmc_devices(void)
> >
> > static int ipc_plat_get_res(struct platform_device *pdev)
> > {
> > - struct resource *res;
> > + struct resource *res, *punit_res;
> > void __iomem *addr;
> > int size;
> >
> > res = platform_get_resource(pdev, IORESOURCE_IO,
> > - PLAT_RESOURCE_ACPI_IO_INDEX);
> > + PLAT_RES_ACPI_IO_INDEX);
> > if (!res) {
> > dev_err(&pdev->dev, "Failed to get io
> > resource\n");
> > return -ENXIO;
> > @@ -606,37 +609,84 @@ static int ipc_plat_get_res(struct
> > platform_device *pdev)
> > dev_info(&pdev->dev, "io res: %llx %x\n",
> > (long long)res->start, (int)resource_size(res));
> >
> > + punit_res = punit_res_array;
> > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > - PLAT_RESOURCE_PUNIT_DATA_INDEX
> > );
> > + PLAT_RES_PUNIT_BIOS_DATA_INDEX
> > );
> > if (!res) {
> > - dev_err(&pdev->dev, "Failed to get punit
> > resource\n");
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > BIOS data\n");
> > return -ENXIO;
> > }
> > - size = resource_size(res);
> > - ipcdev.punit_base = res->start;
> > - ipcdev.punit_size = size;
> > - dev_info(&pdev->dev, "punit data res: %llx %x\n",
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit BIOS data res: %llx %x\n",
> > (long long)res->start, (int)resource_size(res));
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > - PLAT_RESOURCE_PUNIT_INTER_INDE
> > X);
> > + PLAT_RES_PUNIT_BIOS_INTER_INDE
> > X);
> > if (!res) {
> > - dev_err(&pdev->dev, "Failed to get punit inter
> > resource\n");
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > BIOS inter\n");
> > return -ENXIO;
> > }
> > - size = resource_size(res);
> > - ipcdev.punit_base2 = res->start;
> > - ipcdev.punit_size2 = size;
> > - dev_info(&pdev->dev, "punit interface res: %llx %x\n",
> > + punit_res++;
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit BIOS interface res: %llx
> > %x\n",
> > + (long long)res->start, (int)resource_size(res));
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM,
> > + PLAT_RES_PUNIT_ISP_DATA_INDEX)
> > ;
> > + if (!res) {
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > ISP data\n");
> > + return -ENXIO;
> > + }
> > + punit_res++;
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit ISP data res: %llx %x\n",
> > (long long)res->start, (int)resource_size(res));
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > - PLAT_RESOURCE_IPC_INDEX);
> > + PLAT_RES_PUNIT_ISP_INTER_INDEX
> > );
> > + if (!res) {
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > ISP inter\n");
> > + return -ENXIO;
> > + }
> > + punit_res++;
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit ISP interface res: %llx %x\n",
> > + (long long)res->start, (int)resource_size(res));
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM,
> > + PLAT_RES_PUNIT_GTD_DATA_INDEX)
> > ;
> > + if (!res) {
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > GTD data\n");
> > + return -ENXIO;
> > + }
> > + punit_res++;
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit GTD data res: %llx %x\n",
> > + (long long)res->start, (int)resource_size(res));
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM,
> > + PLAT_RES_PUNIT_GTD_INTER_INDEX
> > );
> > + if (!res) {
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > GTD inter\n");
> > + return -ENXIO;
> > + }
> > + punit_res++;
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit GTD interface res: %llx %x\n",
> > + (long long)res->start, (int)resource_size(res));
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM,
> > PLAT_RES_IPC_INDEX);
> > if (!res) {
> > dev_err(&pdev->dev, "Failed to get ipc
> > resource\n");
> > return -ENXIO;
> > }
> > - size = PLAT_RESOURCE_IPC_SIZE;
> > + size = PLAT_RES_IPC_SIZE;
> > if (!request_mem_region(res->start, size, pdev->name)) {
> > dev_err(&pdev->dev, "Failed to request ipc
> > resource\n");
> > return -EBUSY;
> > @@ -650,7 +700,7 @@ static int ipc_plat_get_res(struct
> > platform_device *pdev)
> > ipcdev.ipc_base = addr;
> >
> > ipcdev.gcr_base = res->start + size;
> > - ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
> > + ipcdev.gcr_size = PLAT_RES_GCR_SIZE;
> > dev_info(&pdev->dev, "ipc res: %llx %x\n",
> > (long long)res->start, (int)resource_size(res));
> >
> > @@ -714,9 +764,9 @@ err_irq:
> > err_device:
> > iounmap(ipcdev.ipc_base);
> > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > - PLAT_RESOURCE_IPC_INDEX);
> > + PLAT_RES_IPC_INDEX);
> > if (res)
> > - release_mem_region(res->start,
> > PLAT_RESOURCE_IPC_SIZE);
> > + release_mem_region(res->start, PLAT_RES_IPC_SIZE);
> > return ret;
> > }
> >
> > @@ -730,9 +780,9 @@ static int ipc_plat_remove(struct
> > platform_device *pdev)
> > platform_device_unregister(ipcdev.punit_dev);
> > iounmap(ipcdev.ipc_base);
> > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > - PLAT_RESOURCE_IPC_INDEX);
> > + PLAT_RES_IPC_INDEX);
> > if (res)
> > - release_mem_region(res->start,
> > PLAT_RESOURCE_IPC_SIZE);
> > + release_mem_region(res->start, PLAT_RES_IPC_SIZE);
> > ipcdev.dev = NULL;
> > return 0;
> > }
> > --
> > 1.8.3.2
> >
> >
>
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-07 23:45 ` [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Darren Hart
2015-12-08 11:10 ` Andy Shevchenko
@ 2015-12-08 12:57 ` Andy Shevchenko
2015-12-08 22:59 ` Darren Hart
2015-12-09 0:21 ` Darren Hart
2015-12-08 13:19 ` Andy Shevchenko
2 siblings, 2 replies; 13+ messages in thread
From: Andy Shevchenko @ 2015-12-08 12:57 UTC (permalink / raw)
To: Darren Hart, Qipeng Zha; +Cc: platform-driver-x86
On Mon, 2015-12-07 at 15:45 -0800, Darren Hart wrote:
> On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
> > BIOS restructure exported memory resources for Punit
> > in acpi table, So update resources for Punit.
> >
> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
>
> Thank you for the update Qipeng. I will review shortly.
>
> +Andriy who originally raised the concern over the ACPI resource
> assumptions in
> the previous version. Andriy, this resource allocation looks to be a
> substantial
> improvement to me. Do you have any further concerns?
Here I have few mostly stylish concerns.
>
> > ---
> > drivers/platform/x86/intel_pmc_ipc.c | 142
> > +++++++++++++++++++++++------------
> > 1 file changed, 96 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel_pmc_ipc.c
> > b/drivers/platform/x86/intel_pmc_ipc.c
> > index 28b2a12..c699950 100644
> > --- a/drivers/platform/x86/intel_pmc_ipc.c
> > +++ b/drivers/platform/x86/intel_pmc_ipc.c
> > @@ -65,12 +65,16 @@
> > #define IPC_TRIGGER_MODE_IRQ true
> >
> > /* exported resources from IFWI */
> > -#define PLAT_RESOURCE_IPC_INDEX 0
> > -#define PLAT_RESOURCE_IPC_SIZE 0x1000
> > -#define PLAT_RESOURCE_GCR_SIZE 0x1000
> > -#define PLAT_RESOURCE_PUNIT_DATA_INDEX 1
> > -#define PLAT_RESOURCE_PUNIT_INTER_INDEX 2
> > -#define PLAT_RESOURCE_ACPI_IO_INDEX 0
> > +#define PLAT_RES_IPC_INDEX 0
> > +#define PLAT_RES_IPC_SIZE 0x1000
> > +#define PLAT_RES_GCR_SIZE 0x1000
> > +#define PLAT_RES_PUNIT_BIOS_DATA_INDEX 1
> > +#define PLAT_RES_PUNIT_BIOS_INTER_INDEX 2
> > +#define PLAT_RES_PUNIT_ISP_DATA_INDEX 4
> > +#define PLAT_RES_PUNIT_ISP_INTER_INDEX 5
> > +#define PLAT_RES_PUNIT_GTD_DATA_INDEX 6
> > +#define PLAT_RES_PUNIT_GTD_INTER_INDEX 7
> > +#define PLAT_RES_ACPI_IO_INDEX 0
May I propose to rename a bit this one?
For me looks like PUNIT is not needed in the naming.
What about
/* Resource indexes */
#define PLAT_RESOURCE_IPC_INDEX 0
/* P-Unit */
#define PLAT_RESOURCE_BIOS_DATA_INDEX 1
…
#define PLAT_RESOURCE_GTD_INTER_INDEX 7
> >
> > /*
> > * BIOS does not create an ACPI device for each PMC function,
> > @@ -105,10 +109,6 @@ static struct intel_pmc_ipc_dev {
> > int gcr_size;
> >
> > /* punit */
> > - resource_size_t punit_base;
> > - int punit_size;
> > - resource_size_t punit_base2;
> > - int punit_size2;
> > struct platform_device *punit_dev;
> > } ipcdev;
> >
> > @@ -444,9 +444,22 @@ static const struct attribute_group
> > intel_ipc_group = {
> > .attrs = intel_ipc_attrs,
> > };
> >
> > -#define PUNIT_RESOURCE_INTER 1
> > -static struct resource punit_res[] = {
> > - /* Punit */
> > +static struct resource punit_res_array[] = {
> > + /* Punit BIOS */
> > + {
> > + .flags = IORESOURCE_MEM,
> > + },
> > + {
> > + .flags = IORESOURCE_MEM,
> > + },
> > + /* Punit ISP */
> > + {
> > + .flags = IORESOURCE_MEM,
> > + },
> > + {
> > + .flags = IORESOURCE_MEM,
> > + },
> > + /* Punit GTD */
> > {
> > .flags = IORESOURCE_MEM,
> > },
> > @@ -481,7 +494,6 @@ static struct itco_wdt_platform_data tco_info =
> > {
> > static int ipc_create_punit_device(void)
> > {
> > struct platform_device *pdev;
> > - struct resource *res;
> > int ret;
> >
> > pdev = platform_device_alloc(PUNIT_DEVICE_NAME, -1);
> > @@ -491,17 +503,8 @@ static int ipc_create_punit_device(void)
> > }
> >
> > pdev->dev.parent = ipcdev.dev;
> > -
> > - res = punit_res;
> > - res->start = ipcdev.punit_base;
> > - res->end = res->start + ipcdev.punit_size - 1;
> > -
> > - res = punit_res + PUNIT_RESOURCE_INTER;
> > - res->start = ipcdev.punit_base2;
> > - res->end = res->start + ipcdev.punit_size2 - 1;
> > -
> > - ret = platform_device_add_resources(pdev, punit_res,
> > - ARRAY_SIZE(punit_res))
> > ;
> > + ret = platform_device_add_resources(pdev, punit_res_array,
> > + ARRAY_SIZE(punit_res_a
> > rray));
> > if (ret) {
> > dev_err(ipcdev.dev, "Failed to add platform punit
> > resources\n");
> > goto err;
> > @@ -590,12 +593,12 @@ static int ipc_create_pmc_devices(void)
> >
> > static int ipc_plat_get_res(struct platform_device *pdev)
> > {
> > - struct resource *res;
> > + struct resource *res, *punit_res;
> > void __iomem *addr;
> > int size;
> >
> > res = platform_get_resource(pdev, IORESOURCE_IO,
> > - PLAT_RESOURCE_ACPI_IO_INDEX);
> > + PLAT_RES_ACPI_IO_INDEX);
> > if (!res) {
> > dev_err(&pdev->dev, "Failed to get io
> > resource\n");
> > return -ENXIO;
> > @@ -606,37 +609,84 @@ static int ipc_plat_get_res(struct
> > platform_device *pdev)
> > dev_info(&pdev->dev, "io res: %llx %x\n",
> > (long long)res->start, (int)resource_size(res));
> >
> > + punit_res = punit_res_array;
> > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > - PLAT_RESOURCE_PUNIT_DATA_INDEX
> > );
> > + PLAT_RES_PUNIT_BIOS_DATA_INDEX
> > );
> > if (!res) {
> > - dev_err(&pdev->dev, "Failed to get punit
> > resource\n");
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > BIOS data\n");
> > return -ENXIO;
> > }
> > - size = resource_size(res);
> > - ipcdev.punit_base = res->start;
> > - ipcdev.punit_size = size;
> > - dev_info(&pdev->dev, "punit data res: %llx %x\n",
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
Seems like
*punit_res = *res;
Though punit_res is assigned to punit_res_array which seems not right
to me.
If it's a member of that array we have to explicitly show the index.
> > + dev_info(&pdev->dev, "punit BIOS data res: %llx %x\n",
> > (long long)res->start, (int)resource_size(res));
%pR
(Might be another patch in the future to fix existing code to move to
%pR)
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > - PLAT_RESOURCE_PUNIT_INTER_INDE
> > X);
> > + PLAT_RES_PUNIT_BIOS_INTER_INDE
> > X);
> > if (!res) {
> > - dev_err(&pdev->dev, "Failed to get punit inter
> > resource\n");
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > BIOS inter\n");
Darren, can you improve this phrasing, I didn't get what this message
about?
> > return -ENXIO;
> > }
> > - size = resource_size(res);
> > - ipcdev.punit_base2 = res->start;
> > - ipcdev.punit_size2 = size;
> > - dev_info(&pdev->dev, "punit interface res: %llx %x\n",
> > + punit_res++;
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit BIOS interface res: %llx
> > %x\n",
> > + (long long)res->start, (int)resource_size(res));
Same comments as above.
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM,
> > + PLAT_RES_PUNIT_ISP_DATA_INDEX)
> > ;
> > + if (!res) {
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > ISP data\n");
> > + return -ENXIO;
> > + }
> > + punit_res++;
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit ISP data res: %llx %x\n",
> > (long long)res->start, (int)resource_size(res));
Ditto.
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > - PLAT_RESOURCE_IPC_INDEX);
> > + PLAT_RES_PUNIT_ISP_INTER_INDEX
> > );
> > + if (!res) {
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > ISP inter\n");
> > + return -ENXIO;
> > + }
> > + punit_res++;
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit ISP interface res: %llx %x\n",
> > + (long long)res->start, (int)resource_size(res));
Ditto.
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM,
> > + PLAT_RES_PUNIT_GTD_DATA_INDEX)
> > ;
> > + if (!res) {
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > GTD data\n");
> > + return -ENXIO;
> > + }
> > + punit_res++;
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit GTD data res: %llx %x\n",
> > + (long long)res->start, (int)resource_size(res));
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM,
> > + PLAT_RES_PUNIT_GTD_INTER_INDEX
> > );
> > + if (!res) {
> > + dev_err(&pdev->dev, "Failed to get res of punit
> > GTD inter\n");
> > + return -ENXIO;
> > + }
> > + punit_res++;
> > + punit_res->start = res->start;
> > + punit_res->end = res->start + resource_size(res) - 1;
> > + dev_info(&pdev->dev, "punit GTD interface res: %llx %x\n",
> > + (long long)res->start, (int)resource_size(res));
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM,
> > PLAT_RES_IPC_INDEX);
> > if (!res) {
> > dev_err(&pdev->dev, "Failed to get ipc
> > resource\n");
> > return -ENXIO;
> > }
> > - size = PLAT_RESOURCE_IPC_SIZE;
> > + size = PLAT_RES_IPC_SIZE;
> > if (!request_mem_region(res->start, size, pdev->name)) {
> > dev_err(&pdev->dev, "Failed to request ipc
> > resource\n");
> > return -EBUSY;
> > @@ -650,7 +700,7 @@ static int ipc_plat_get_res(struct
> > platform_device *pdev)
> > ipcdev.ipc_base = addr;
> >
> > ipcdev.gcr_base = res->start + size;
> > - ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
> > + ipcdev.gcr_size = PLAT_RES_GCR_SIZE;
> > dev_info(&pdev->dev, "ipc res: %llx %x\n",
> > (long long)res->start, (int)resource_size(res));
> >
> > @@ -714,9 +764,9 @@ err_irq:
> > err_device:
> > iounmap(ipcdev.ipc_base);
> > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > - PLAT_RESOURCE_IPC_INDEX);
> > + PLAT_RES_IPC_INDEX);
> > if (res)
> > - release_mem_region(res->start,
> > PLAT_RESOURCE_IPC_SIZE);
> > + release_mem_region(res->start, PLAT_RES_IPC_SIZE);
> > return ret;
> > }
> >
> > @@ -730,9 +780,9 @@ static int ipc_plat_remove(struct
> > platform_device *pdev)
> > platform_device_unregister(ipcdev.punit_dev);
> > iounmap(ipcdev.ipc_base);
> > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > - PLAT_RESOURCE_IPC_INDEX);
> > + PLAT_RES_IPC_INDEX);
> > if (res)
> > - release_mem_region(res->start,
> > PLAT_RESOURCE_IPC_SIZE);
> > + release_mem_region(res->start, PLAT_RES_IPC_SIZE);
> > ipcdev.dev = NULL;
> > return 0;
> > }
> > --
> > 1.8.3.2
> >
> >
>
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-07 23:45 ` [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Darren Hart
2015-12-08 11:10 ` Andy Shevchenko
2015-12-08 12:57 ` Andy Shevchenko
@ 2015-12-08 13:19 ` Andy Shevchenko
2015-12-08 23:02 ` Darren Hart
` (2 more replies)
2 siblings, 3 replies; 13+ messages in thread
From: Andy Shevchenko @ 2015-12-08 13:19 UTC (permalink / raw)
To: Darren Hart, Qipeng Zha; +Cc: platform-driver-x86
On Mon, 2015-12-07 at 15:45 -0800, Darren Hart wrote:
> On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
> > BIOS restructure exported memory resources for Punit
> > in acpi table, So update resources for Punit.
> >
> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
>
> Thank you for the update Qipeng. I will review shortly.
>
> +Andriy who originally raised the concern over the ACPI resource
> assumptions in
> the previous version. Andriy, this resource allocation looks to be a
> substantial
> improvement to me. Do you have any further concerns?
So, regarding to the second patch
1. In excerpts like following
if (IS_ERR(addr)) {
dev_err(&pdev->dev, "Failed to map resouce for BIOS
DATA\n");
return PTR_ERR(addr);
}
No need to have an error message. Core already has something to print
at that point.
2. No need to explicitly cast to / from void *.
IPC_DEV *ipcdev = (IPC_DEV *)dev_id;
Otherwise looks much better than very first version!
Thanks for an update.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-08 12:57 ` Andy Shevchenko
@ 2015-12-08 22:59 ` Darren Hart
2015-12-09 0:21 ` Darren Hart
1 sibling, 0 replies; 13+ messages in thread
From: Darren Hart @ 2015-12-08 22:59 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Qipeng Zha, platform-driver-x86
On Tue, Dec 08, 2015 at 02:57:18PM +0200, Andy Shevchenko wrote:
> On Mon, 2015-12-07 at 15:45 -0800, Darren Hart wrote:
> > On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
...
>
> > >
> > > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > > - PLAT_RESOURCE_PUNIT_INTER_INDE
> > > X);
> > > + PLAT_RES_PUNIT_BIOS_INTER_INDE
> > > X);
> > > if (!res) {
> > > - dev_err(&pdev->dev, "Failed to get punit inter
> > > resource\n");
> > > + dev_err(&pdev->dev, "Failed to get res of punit
> > > BIOS inter\n");
>
> Darren, can you improve this phrasing, I didn't get what this message
> about?
I wasn't going to mention this unless there was sure to be a v9, it seems that
is likely, so...
Qipeng, in order to ensure the message to the user is clear, it is important not
to use abbreviations (especially non-obvious ones) like "inter" and "res". Note that the user will not necessarily have the sourcecode handy for context.
I believe this message is just attempting to the user that the
platform_get_resource message failed for some reason when attempting to get the
ACPI provided resource for the BIOS interface resource. There are two resources
for each, interface and data.
Without knowing the details of the internals, I would think the following might
be a better message?
dev_err(&pdev->dev, "Failed to get punit BIOS interface platform resource\n");
Similarly for all these messages.
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-08 13:19 ` Andy Shevchenko
@ 2015-12-08 23:02 ` Darren Hart
2015-12-08 23:28 ` Darren Hart
2015-12-10 2:39 ` Zha, Qipeng
2 siblings, 0 replies; 13+ messages in thread
From: Darren Hart @ 2015-12-08 23:02 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Qipeng Zha, platform-driver-x86
On Tue, Dec 08, 2015 at 03:19:17PM +0200, Andy Shevchenko wrote:
> On Mon, 2015-12-07 at 15:45 -0800, Darren Hart wrote:
> > On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
> > > BIOS restructure exported memory resources for Punit
> > > in acpi table, So update resources for Punit.
> > >
> > > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> >
> > Thank you for the update Qipeng. I will review shortly.
> >
> > +Andriy who originally raised the concern over the ACPI resource
> > assumptions in
> > the previous version. Andriy, this resource allocation looks to be a
> > substantial
> > improvement to me. Do you have any further concerns?
>
> So, regarding to the second patch
>
> 1. In excerpts like following
>
> if (IS_ERR(addr)) {
> dev_err(&pdev->dev, "Failed to map resouce for BIOS
> DATA\n");
> return PTR_ERR(addr);
> }
>
> No need to have an error message. Core already has something to print
> at that point.
>
>
> 2. No need to explicitly cast to / from void *.
>
> IPC_DEV *ipcdev = (IPC_DEV *)dev_id;
>
>
> Otherwise looks much better than very first version!
Good points, thanks Andriy.
Qipeng. I hate to ask for one more spin, but this is good feedback, and the
error messages from the previous note should be addressed. If you can give one
more update, we can get this queued to next and still make the 4.5 merge window.
Thanks for your efforts to get to this point. Nearly there!
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-08 13:19 ` Andy Shevchenko
2015-12-08 23:02 ` Darren Hart
@ 2015-12-08 23:28 ` Darren Hart
2015-12-10 2:39 ` Zha, Qipeng
2 siblings, 0 replies; 13+ messages in thread
From: Darren Hart @ 2015-12-08 23:28 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Qipeng Zha, platform-driver-x86
On Tue, Dec 08, 2015 at 03:19:17PM +0200, Andy Shevchenko wrote:
> On Mon, 2015-12-07 at 15:45 -0800, Darren Hart wrote:
> > On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
> > > BIOS restructure exported memory resources for Punit
> > > in acpi table, So update resources for Punit.
> > >
> > > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> >
> > Thank you for the update Qipeng. I will review shortly.
> >
> > +Andriy who originally raised the concern over the ACPI resource
> > assumptions in
> > the previous version. Andriy, this resource allocation looks to be a
> > substantial
> > improvement to me. Do you have any further concerns?
>
> So, regarding to the second patch
>
> 1. In excerpts like following
>
> if (IS_ERR(addr)) {
> dev_err(&pdev->dev, "Failed to map resouce for BIOS
> DATA\n");
> return PTR_ERR(addr);
> }
>
> No need to have an error message. Core already has something to print
> at that point.
Ah, so neither the DATA nor the INTER messages are necessary. I suppose this
means you can ignore my response on better wording.
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-08 12:57 ` Andy Shevchenko
2015-12-08 22:59 ` Darren Hart
@ 2015-12-09 0:21 ` Darren Hart
2015-12-09 11:09 ` Andy Shevchenko
1 sibling, 1 reply; 13+ messages in thread
From: Darren Hart @ 2015-12-09 0:21 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Qipeng Zha, platform-driver-x86
On Tue, Dec 08, 2015 at 02:57:18PM +0200, Andy Shevchenko wrote:
> On Mon, 2015-12-07 at 15:45 -0800, Darren Hart wrote:
> > On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
> > > BIOS restructure exported memory resources for Punit
> > > in acpi table, So update resources for Punit.
> > >
> > > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> >
> > Thank you for the update Qipeng. I will review shortly.
> >
> > +Andriy who originally raised the concern over the ACPI resource
> > assumptions in
> > the previous version. Andriy, this resource allocation looks to be a
> > substantial
> > improvement to me. Do you have any further concerns?
>
> Here I have few mostly stylish concerns.
>
> >
> > > ---
> > > drivers/platform/x86/intel_pmc_ipc.c | 142
> > > +++++++++++++++++++++++------------
> > > 1 file changed, 96 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/intel_pmc_ipc.c
> > > b/drivers/platform/x86/intel_pmc_ipc.c
> > > index 28b2a12..c699950 100644
> > > --- a/drivers/platform/x86/intel_pmc_ipc.c
> > > +++ b/drivers/platform/x86/intel_pmc_ipc.c
> > > @@ -65,12 +65,16 @@
> > > #define IPC_TRIGGER_MODE_IRQ true
> > >
> > > /* exported resources from IFWI */
> > > -#define PLAT_RESOURCE_IPC_INDEX 0
> > > -#define PLAT_RESOURCE_IPC_SIZE 0x1000
> > > -#define PLAT_RESOURCE_GCR_SIZE 0x1000
> > > -#define PLAT_RESOURCE_PUNIT_DATA_INDEX 1
> > > -#define PLAT_RESOURCE_PUNIT_INTER_INDEX 2
> > > -#define PLAT_RESOURCE_ACPI_IO_INDEX 0
> > > +#define PLAT_RES_IPC_INDEX 0
> > > +#define PLAT_RES_IPC_SIZE 0x1000
> > > +#define PLAT_RES_GCR_SIZE 0x1000
> > > +#define PLAT_RES_PUNIT_BIOS_DATA_INDEX 1
> > > +#define PLAT_RES_PUNIT_BIOS_INTER_INDEX 2
> > > +#define PLAT_RES_PUNIT_ISP_DATA_INDEX 4
> > > +#define PLAT_RES_PUNIT_ISP_INTER_INDEX 5
> > > +#define PLAT_RES_PUNIT_GTD_DATA_INDEX 6
> > > +#define PLAT_RES_PUNIT_GTD_INTER_INDEX 7
> > > +#define PLAT_RES_ACPI_IO_INDEX 0
>
> May I propose to rename a bit this one?
> For me looks like PUNIT is not needed in the naming.
>
> What about
>
> /* Resource indexes */
> #define PLAT_RESOURCE_IPC_INDEX 0
> /* P-Unit */
> #define PLAT_RESOURCE_BIOS_DATA_INDEX 1
> …
>
> #define PLAT_RESOURCE_GTD_INTER_INDEX 7
Seems reasonable, Qipeng, does PUNIT provide some kind of namespacing that
relevantly separates this from PLAT_RES_IPC_INDEX for example? Is it a different
component?
I don't want to bikeshed too much over this though, certainly don't want to hold
up getting this in next over it. But as we do have some items below to address,
please consider this.
...
> > > @@ -606,37 +609,84 @@ static int ipc_plat_get_res(struct
> > > platform_device *pdev)
> > > dev_info(&pdev->dev, "io res: %llx %x\n",
> > > (long long)res->start, (int)resource_size(res));
> > >
> > > + punit_res = punit_res_array;
> > > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > > - PLAT_RESOURCE_PUNIT_DATA_INDEX
> > > );
> > > + PLAT_RES_PUNIT_BIOS_DATA_INDEX
> > > );
> > > if (!res) {
> > > - dev_err(&pdev->dev, "Failed to get punit
> > > resource\n");
> > > + dev_err(&pdev->dev, "Failed to get res of punit
> > > BIOS data\n");
> > > return -ENXIO;
> > > }
> > > - size = resource_size(res);
> > > - ipcdev.punit_base = res->start;
> > > - ipcdev.punit_size = size;
> > > - dev_info(&pdev->dev, "punit data res: %llx %x\n",
>
> > > + punit_res->start = res->start;
> > > + punit_res->end = res->start + resource_size(res) - 1;
>
> Seems like
>
> *punit_res = *res;
>
> Though punit_res is assigned to punit_res_array which seems not right
> to me.
>
> If it's a member of that array we have to explicitly show the index.
>
It seems fairly clear to me that punit_res is used to iterate through the items
of the array using pointer arithmetic, but if it could be clearer, great. What
would you prefer to see Andriy?
Unfortunatley, we can't use the defined indices for the ACPI resources as they
are neither 0 based nor sequential. This does mean that the punit driver relies
on the order the pmc driver populates this array, rather than defined indices.
This binding, however, is contained entirely within the kernel, so I'm not so
concerned as I was with the ACPI resources being assumed contiguous.
We could move the DATA, INTERFACE enums to intel_punit_ipc.h and use the
following indices without introducing new enums...
(2 * BIOS_IPC) + DATA
(2 * BIOS_IPC) + INTERFACE
(2 * GTDRIVER_IPC) + DATA
(2 * GTDRIVER_IPC) + INTERFACE
(2 * GTDRIVER_IPC) + DATA
(2 * GTDRIVER_IPC) + INTERFACE
But that's pretty horrible, so I suppose the only real solution would be to
introduce yet another set of defines:
#define PUNIT_PLAT_RES_BIOS_IPC_DATA_INDEX 0
#define PUNIT_PLAT_RES_BIOS_IPC_INTERFACE_INDEX 1
#define PUNIT_PLAT_RES_GTDRIVER_IPC_DATA_INDEX 2
#define PUNIT_PLAT_RES_GTDRIVER_IPC_INTERFACE_INDEX 3
#define PUNIT_PLAT_RES_ISPDRIVER_IPC_DATA_INDEX 4
#define PUNIT_PLAT_RES_ISPDRIVER_IPC_INTERFACE_INDEX 5
I'm not really sure this is better given the lengthy list for defines already
present.
So, if you would like to a change, please recommend what you would prefer
Andriy, because I can see the argument for either approach.
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-09 0:21 ` Darren Hart
@ 2015-12-09 11:09 ` Andy Shevchenko
2015-12-09 16:33 ` Darren Hart
0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2015-12-09 11:09 UTC (permalink / raw)
To: Darren Hart; +Cc: Qipeng Zha, platform-driver-x86
On Tue, 2015-12-08 at 16:21 -0800, Darren Hart wrote:
> On Tue, Dec 08, 2015 at 02:57:18PM +0200, Andy Shevchenko wrote:
> > On Mon, 2015-12-07 at 15:45 -0800, Darren Hart wrote:
> > > On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
> > > > BIOS restructure exported memory resources for Punit
> > > > in acpi table, So update resources for Punit.
> > > >
> > > > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> > >
> > > Thank you for the update Qipeng. I will review shortly.
> > >
> > > +Andriy who originally raised the concern over the ACPI resource
> > > assumptions in
> > > the previous version. Andriy, this resource allocation looks to
> > > be a
> > > substantial
> > > improvement to me. Do you have any further concerns?
> >
> > Here I have few mostly stylish concerns.
> >
> > >
> > > > ---
> > > > drivers/platform/x86/intel_pmc_ipc.c | 142
> > > > +++++++++++++++++++++++------------
> > > > 1 file changed, 96 insertions(+), 46 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/intel_pmc_ipc.c
> > > > b/drivers/platform/x86/intel_pmc_ipc.c
> > > > index 28b2a12..c699950 100644
> > > > --- a/drivers/platform/x86/intel_pmc_ipc.c
> > > > +++ b/drivers/platform/x86/intel_pmc_ipc.c
> > > > @@ -65,12 +65,16 @@
> > > > #define IPC_TRIGGER_MODE_IRQ true
> > > >
> > > > /* exported resources from IFWI */
> > > > -#define PLAT_RESOURCE_IPC_INDEX 0
> > > > -#define PLAT_RESOURCE_IPC_SIZE 0x1000
> > > > -#define PLAT_RESOURCE_GCR_SIZE 0x1000
> > > > -#define PLAT_RESOURCE_PUNIT_DATA_INDEX 1
> > > > -#define PLAT_RESOURCE_PUNIT_INTER_INDEX 2
> > > > -#define PLAT_RESOURCE_ACPI_IO_INDEX 0
> > > > +#define PLAT_RES_IPC_INDEX 0
> > > > +#define PLAT_RES_IPC_SIZE 0x1000
> > > > +#define PLAT_RES_GCR_SIZE 0x1000
> > > > +#define PLAT_RES_PUNIT_BIOS_DATA_INDEX 1
> > > > +#define PLAT_RES_PUNIT_BIOS_INTER_INDEX 2
> > > > +#define PLAT_RES_PUNIT_ISP_DATA_INDEX 4
> > > > +#define PLAT_RES_PUNIT_ISP_INTER_INDEX 5
> > > > +#define PLAT_RES_PUNIT_GTD_DATA_INDEX 6
> > > > +#define PLAT_RES_PUNIT_GTD_INTER_INDEX 7
> > > > +#define PLAT_RES_ACPI_IO_INDEX 0
> >
> > May I propose to rename a bit this one?
> > For me looks like PUNIT is not needed in the naming.
> >
> > What about
> >
> > /* Resource indexes */
> > #define PLAT_RESOURCE_IPC_INDEX 0
> > /* P-Unit */
> > #define PLAT_RESOURCE_BIOS_DATA_INDEX 1
> > …
> >
> > #define PLAT_RESOURCE_GTD_INTER_INDEX 7
>
>
> Seems reasonable, Qipeng, does PUNIT provide some kind of namespacing
> that
> relevantly separates this from PLAT_RES_IPC_INDEX for example? Is it
> a different
> component?
>
> I don't want to bikeshed too much over this though, certainly don't
> want to hold
> up getting this in next over it. But as we do have some items below
> to address,
> please consider this.
>
> ...
>
> > > > @@ -606,37 +609,84 @@ static int ipc_plat_get_res(struct
> > > > platform_device *pdev)
> > > > dev_info(&pdev->dev, "io res: %llx %x\n",
> > > > (long long)res->start,
> > > > (int)resource_size(res));
> > > >
> > > > + punit_res = punit_res_array;
> > > > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > > > - PLAT_RESOURCE_PUNIT_DATA_I
> > > > NDEX
> > > > );
> > > > + PLAT_RES_PUNIT_BIOS_DATA_I
> > > > NDEX
> > > > );
> > > > if (!res) {
> > > > - dev_err(&pdev->dev, "Failed to get punit
> > > > resource\n");
> > > > + dev_err(&pdev->dev, "Failed to get res of
> > > > punit
> > > > BIOS data\n");
> > > > return -ENXIO;
> > > > }
> > > > - size = resource_size(res);
> > > > - ipcdev.punit_base = res->start;
> > > > - ipcdev.punit_size = size;
> > > > - dev_info(&pdev->dev, "punit data res: %llx %x\n",
> >
> > > > + punit_res->start = res->start;
> > > > + punit_res->end = res->start + resource_size(res) - 1;
> >
> > Seems like
> >
> > *punit_res = *res;
> >
> > Though punit_res is assigned to punit_res_array which seems not
> > right
> > to me.
> >
> > If it's a member of that array we have to explicitly show the
> > index.
> >
>
> It seems fairly clear to me that punit_res is used to iterate through
> the items
> of the array using pointer arithmetic, but if it could be clearer,
> great. What
> would you prefer to see Andriy?
>
> Unfortunatley, we can't use the defined indices for the ACPI
> resources as they
> are neither 0 based nor sequential. This does mean that the punit
> driver relies
> on the order the pmc driver populates this array, rather than defined
> indices.
> This binding, however, is contained entirely within the kernel, so
> I'm not so
> concerned as I was with the ACPI resources being assumed contiguous.
>
> We could move the DATA, INTERFACE enums to intel_punit_ipc.h and use
> the
> following indices without introducing new enums...
>
> (2 * BIOS_IPC) + DATA
> (2 * BIOS_IPC) + INTERFACE
> (2 * GTDRIVER_IPC) + DATA
> (2 * GTDRIVER_IPC) + INTERFACE
> (2 * GTDRIVER_IPC) + DATA
> (2 * GTDRIVER_IPC) + INTERFACE
>
> But that's pretty horrible, so I suppose the only real solution would
> be to
> introduce yet another set of defines:
>
> #define PUNIT_PLAT_RES_BIOS_IPC_DATA_INDEX 0
> #define PUNIT_PLAT_RES_BIOS_IPC_INTERFACE_INDEX 1
> #define PUNIT_PLAT_RES_GTDRIVER_IPC_DATA_INDEX 2
> #define PUNIT_PLAT_RES_GTDRIVER_IPC_INTERFACE_INDEX 3
> #define PUNIT_PLAT_RES_ISPDRIVER_IPC_DATA_INDEX 4
> #define PUNIT_PLAT_RES_ISPDRIVER_IPC_INTERFACE_INDEX 5
>
> I'm not really sure this is better given the lengthy list for defines
> already
> present.
>
> So, if you would like to a change, please recommend what you would
> prefer
> Andriy, because I can see the argument for either approach.
For simplicity sake let's leave this as current iterative approach.
Maybe we may go with the comment before each punit_res++; line to
explain "this is index N to cover Y".
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-09 11:09 ` Andy Shevchenko
@ 2015-12-09 16:33 ` Darren Hart
0 siblings, 0 replies; 13+ messages in thread
From: Darren Hart @ 2015-12-09 16:33 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Qipeng Zha, platform-driver-x86
On Wed, Dec 09, 2015 at 01:09:51PM +0200, Andy Shevchenko wrote:
> On Tue, 2015-12-08 at 16:21 -0800, Darren Hart wrote:
> > On Tue, Dec 08, 2015 at 02:57:18PM +0200, Andy Shevchenko wrote:
> > > On Mon, 2015-12-07 at 15:45 -0800, Darren Hart wrote:
> > > > On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
> > > > > BIOS restructure exported memory resources for Punit
> > > > > in acpi table, So update resources for Punit.
> > > > >
> > > > > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> > > >
> > > > Thank you for the update Qipeng. I will review shortly.
> > > >
> > > > +Andriy who originally raised the concern over the ACPI resource
> > > > assumptions in
> > > > the previous version. Andriy, this resource allocation looks to
> > > > be a
> > > > substantial
> > > > improvement to me. Do you have any further concerns?
> > >
> > > Here I have few mostly stylish concerns.
> > >
> > > >
> > > > > ---
> > > > > drivers/platform/x86/intel_pmc_ipc.c | 142
> > > > > +++++++++++++++++++++++------------
> > > > > 1 file changed, 96 insertions(+), 46 deletions(-)
> > > > >
> > > > > diff --git a/drivers/platform/x86/intel_pmc_ipc.c
> > > > > b/drivers/platform/x86/intel_pmc_ipc.c
> > > > > index 28b2a12..c699950 100644
> > > > > --- a/drivers/platform/x86/intel_pmc_ipc.c
> > > > > +++ b/drivers/platform/x86/intel_pmc_ipc.c
> > > > > @@ -65,12 +65,16 @@
> > > > > #define IPC_TRIGGER_MODE_IRQ true
> > > > >
> > > > > /* exported resources from IFWI */
> > > > > -#define PLAT_RESOURCE_IPC_INDEX 0
> > > > > -#define PLAT_RESOURCE_IPC_SIZE 0x1000
> > > > > -#define PLAT_RESOURCE_GCR_SIZE 0x1000
> > > > > -#define PLAT_RESOURCE_PUNIT_DATA_INDEX 1
> > > > > -#define PLAT_RESOURCE_PUNIT_INTER_INDEX 2
> > > > > -#define PLAT_RESOURCE_ACPI_IO_INDEX 0
> > > > > +#define PLAT_RES_IPC_INDEX 0
> > > > > +#define PLAT_RES_IPC_SIZE 0x1000
> > > > > +#define PLAT_RES_GCR_SIZE 0x1000
> > > > > +#define PLAT_RES_PUNIT_BIOS_DATA_INDEX 1
> > > > > +#define PLAT_RES_PUNIT_BIOS_INTER_INDEX 2
> > > > > +#define PLAT_RES_PUNIT_ISP_DATA_INDEX 4
> > > > > +#define PLAT_RES_PUNIT_ISP_INTER_INDEX 5
> > > > > +#define PLAT_RES_PUNIT_GTD_DATA_INDEX 6
> > > > > +#define PLAT_RES_PUNIT_GTD_INTER_INDEX 7
> > > > > +#define PLAT_RES_ACPI_IO_INDEX 0
> > >
> > > May I propose to rename a bit this one?
> > > For me looks like PUNIT is not needed in the naming.
> > >
> > > What about
> > >
> > > /* Resource indexes */
> > > #define PLAT_RESOURCE_IPC_INDEX 0
> > > /* P-Unit */
> > > #define PLAT_RESOURCE_BIOS_DATA_INDEX 1
> > > …
> > >
> > > #define PLAT_RESOURCE_GTD_INTER_INDEX 7
> >
> >
> > Seems reasonable, Qipeng, does PUNIT provide some kind of namespacing
> > that
> > relevantly separates this from PLAT_RES_IPC_INDEX for example? Is it
> > a different
> > component?
> >
> > I don't want to bikeshed too much over this though, certainly don't
> > want to hold
> > up getting this in next over it. But as we do have some items below
> > to address,
> > please consider this.
> >
> > ...
> >
> > > > > @@ -606,37 +609,84 @@ static int ipc_plat_get_res(struct
> > > > > platform_device *pdev)
> > > > > dev_info(&pdev->dev, "io res: %llx %x\n",
> > > > > (long long)res->start,
> > > > > (int)resource_size(res));
> > > > >
> > > > > + punit_res = punit_res_array;
> > > > > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > > > > - PLAT_RESOURCE_PUNIT_DATA_I
> > > > > NDEX
> > > > > );
> > > > > + PLAT_RES_PUNIT_BIOS_DATA_I
> > > > > NDEX
> > > > > );
> > > > > if (!res) {
> > > > > - dev_err(&pdev->dev, "Failed to get punit
> > > > > resource\n");
> > > > > + dev_err(&pdev->dev, "Failed to get res of
> > > > > punit
> > > > > BIOS data\n");
> > > > > return -ENXIO;
> > > > > }
> > > > > - size = resource_size(res);
> > > > > - ipcdev.punit_base = res->start;
> > > > > - ipcdev.punit_size = size;
> > > > > - dev_info(&pdev->dev, "punit data res: %llx %x\n",
> > >
> > > > > + punit_res->start = res->start;
> > > > > + punit_res->end = res->start + resource_size(res) - 1;
> > >
> > > Seems like
> > >
> > > *punit_res = *res;
> > >
> > > Though punit_res is assigned to punit_res_array which seems not
> > > right
> > > to me.
> > >
> > > If it's a member of that array we have to explicitly show the
> > > index.
> > >
> >
> > It seems fairly clear to me that punit_res is used to iterate through
> > the items
> > of the array using pointer arithmetic, but if it could be clearer,
> > great. What
> > would you prefer to see Andriy?
> >
> > Unfortunatley, we can't use the defined indices for the ACPI
> > resources as they
> > are neither 0 based nor sequential. This does mean that the punit
> > driver relies
> > on the order the pmc driver populates this array, rather than defined
> > indices.
> > This binding, however, is contained entirely within the kernel, so
> > I'm not so
> > concerned as I was with the ACPI resources being assumed contiguous.
> >
> > We could move the DATA, INTERFACE enums to intel_punit_ipc.h and use
> > the
> > following indices without introducing new enums...
> >
> > (2 * BIOS_IPC) + DATA
> > (2 * BIOS_IPC) + INTERFACE
> > (2 * GTDRIVER_IPC) + DATA
> > (2 * GTDRIVER_IPC) + INTERFACE
> > (2 * GTDRIVER_IPC) + DATA
> > (2 * GTDRIVER_IPC) + INTERFACE
> >
> > But that's pretty horrible, so I suppose the only real solution would
> > be to
> > introduce yet another set of defines:
> >
> > #define PUNIT_PLAT_RES_BIOS_IPC_DATA_INDEX 0
> > #define PUNIT_PLAT_RES_BIOS_IPC_INTERFACE_INDEX 1
> > #define PUNIT_PLAT_RES_GTDRIVER_IPC_DATA_INDEX 2
> > #define PUNIT_PLAT_RES_GTDRIVER_IPC_INTERFACE_INDEX 3
> > #define PUNIT_PLAT_RES_ISPDRIVER_IPC_DATA_INDEX 4
> > #define PUNIT_PLAT_RES_ISPDRIVER_IPC_INTERFACE_INDEX 5
> >
> > I'm not really sure this is better given the lengthy list for defines
> > already
> > present.
> >
> > So, if you would like to a change, please recommend what you would
> > prefer
> > Andriy, because I can see the argument for either approach.
>
> For simplicity sake let's leave this as current iterative approach.
>
> Maybe we may go with the comment before each punit_res++; line to
> explain "this is index N to cover Y".
Sounds like a plan to me. Qipeng, could you include that in your final version?
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
2015-12-08 13:19 ` Andy Shevchenko
2015-12-08 23:02 ` Darren Hart
2015-12-08 23:28 ` Darren Hart
@ 2015-12-10 2:39 ` Zha, Qipeng
2 siblings, 0 replies; 13+ messages in thread
From: Zha, Qipeng @ 2015-12-10 2:39 UTC (permalink / raw)
To: Andy Shevchenko, Darren Hart; +Cc: platform-driver-x86@vger.kernel.org
>> +Andriy who originally raised the concern over the ACPI resource
>> assumptions in
>> the previous version. Andriy, this resource allocation looks to be a
>> substantial improvement to me. Do you have any further concerns?
>So, regarding to the second patch
>1. In excerpts like following
>if (IS_ERR(addr)) {
> dev_err(&pdev->dev, "Failed to map resouce for BIOS DATA\n");
> return PTR_ERR(addr);
> }
>No need to have an error message. Core already has something to print at that point.
Core error message will not show which resource is failed,
Anyway, I will made this change
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-12-10 2:39 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-07 16:55 [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Qipeng Zha
2015-12-07 16:55 ` [PATCH V8 2/2] platform:x86: add Intel P-Unit mailbox IPC driver Qipeng Zha
2015-12-07 23:45 ` [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Darren Hart
2015-12-08 11:10 ` Andy Shevchenko
2015-12-08 12:57 ` Andy Shevchenko
2015-12-08 22:59 ` Darren Hart
2015-12-09 0:21 ` Darren Hart
2015-12-09 11:09 ` Andy Shevchenko
2015-12-09 16:33 ` Darren Hart
2015-12-08 13:19 ` Andy Shevchenko
2015-12-08 23:02 ` Darren Hart
2015-12-08 23:28 ` Darren Hart
2015-12-10 2:39 ` Zha, Qipeng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.