* [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
@ 2015-10-09 8:49 Qipeng Zha
2015-10-09 13:33 ` Shevchenko, Andriy
0 siblings, 1 reply; 20+ messages in thread
From: Qipeng Zha @ 2015-10-09 8:49 UTC (permalink / raw)
To: platform-driver-x86
Cc: dvhart, qipeng.zha, andriy.shevchenko, mika.westerberg
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 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 13ac861..cf71387 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5207,12 +5207,14 @@ F: include/uapi/linux/mei.h
F: drivers/misc/mei/*
F: Documentation/misc-devices/mei/*
-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 346f1fd..9948369 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -891,4 +891,10 @@ config INTEL_PMC_IPC
The PMC is an ARC processor which defines IPC commands for communication
with other entities in the CPU.
+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 1051372..eea765f 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -59,3 +59,4 @@ obj-$(CONFIG_INTEL_SMARTCONNECT) += intel-smartconnect.o
obj-$(CONFIG_PVPANIC) += pvpanic.o
obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
obj-$(CONFIG_INTEL_PMC_IPC) += intel_pmc_ipc.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..c78ab57
--- /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 DATA_LOW 0x0
+#define INTERFACE 0x4
+#define CMD_RUN (1 << 31)
+#define CMD_ERRCODE_MASK 0xFF
+#define CMD_PARA1_SHIFT 8
+#define CMD_PARA2_SHIFT 16
+#define DATA_HIGH 0x8
+
+#define MAILBOX_REGISTER_SPACE 0x10
+#define CMD_TIMEOUT_SECONDS 1
+
+typedef struct {
+ struct device *dev;
+ struct mutex lock;
+ int irq;
+ struct completion cmd_complete;
+ void __iomem *base[RESERVED_IPC];
+ 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_LOW);
+}
+
+static inline u32 ipc_read_data_high(IPC_DEV *ipcdev, IPC_TYPE type)
+{
+ return readl(ipcdev->base[type] + DATA_HIGH);
+}
+
+static inline void ipc_write_data_low(IPC_DEV *ipcdev, IPC_TYPE type, u32 data)
+{
+ writel(data, ipcdev->base[type] + DATA_LOW);
+}
+
+static inline void ipc_write_data_high(IPC_DEV *ipcdev, IPC_TYPE type, u32 data)
+{
+ writel(data, ipcdev->base[type] + 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 *res0, *res1;
+ void __iomem *addr;
+ int size;
+
+ res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res0) {
+ dev_err(&pdev->dev, "Failed to get iomem resource\n");
+ return -ENXIO;
+ }
+ size = resource_size(res0);
+ if (!devm_request_mem_region(&pdev->dev, res0->start,
+ size, pdev->name)) {
+ dev_err(&pdev->dev, "Failed to request iomem resouce\n");
+ return -EBUSY;
+ }
+
+ res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res1) {
+ dev_err(&pdev->dev, "Failed to get iomem resource1\n");
+ return -ENXIO;
+ }
+ size = resource_size(res1);
+ if (!devm_request_mem_region(&pdev->dev, res1->start,
+ size, pdev->name)) {
+ dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
+ return -EBUSY;
+ }
+
+ addr = ioremap_nocache(res0->start,
+ resource_size(res0) + resource_size(res1));
+ if (!addr) {
+ dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
+ return -ENOMEM;
+ }
+ punit_ipcdev->base[BIOS_IPC] = addr;
+ addr += MAILBOX_REGISTER_SPACE;
+ punit_ipcdev->base[GTDRIVER_IPC] = addr;
+ addr += MAILBOX_REGISTER_SPACE;
+ punit_ipcdev->base[ISPDRIVER_IPC] = 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)
+{
+ IPC_DEV *ipcdev = platform_get_drvdata(pdev);
+
+ iounmap(ipcdev->base[BIOS_IPC]);
+
+ 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] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-09 8:49 [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver Qipeng Zha
@ 2015-10-09 13:33 ` Shevchenko, Andriy
2015-10-10 3:07 ` Zha, Qipeng
0 siblings, 1 reply; 20+ messages in thread
From: Shevchenko, Andriy @ 2015-10-09 13:33 UTC (permalink / raw)
To: platform-driver-x86, Zha, Qipeng; +Cc: dvhart, Westerberg, Mika
On Fri, 2015-10-09 at 16:49 +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.
Everything is quite okay, except this BAR thingy.
Can you provide a DSDT excerpt for the device to see what is there?
I can't find such device (by ACPI id) in the tables of the accessible
hardware in our lab.
>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
>
> ---
> 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 13ac861..cf71387 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5207,12 +5207,14 @@ F: include/uapi/linux/mei.h
> F: drivers/misc/mei/*
> F: Documentation/misc-devices/mei/*
>
> -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_B
> IOS_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_B
> IOS_CMD_BASE | 0x05)
> +#define IPC_PUNIT_BIOS_READ_PL1_SETTING (IPC_PUNIT_B
> IOS_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_B
> IOS_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_B
> IOS_CMD_BASE | 0x11)
> +#define IPC_PUNIT_BIOS_READ_MODULE_TEMP (IPC_PUNIT_B
> IOS_CMD_BASE | 0x12)
> +#define IPC_PUNIT_BIOS_RESERVED (IPC_PUNIT_B
> IOS_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_B
> IOS_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_B
> IOS_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_B
> IOS_CMD_BASE | 0x1b)
> +
> +/* GT Driver => Pcode commands */
> +#define IPC_PUNIT_GTD_ZERO (IPC_PUNIT_GTD_CMD_B
> ASE | 0x00)
> +#define IPC_PUNIT_GTD_CONFIG (IPC_PUNIT_GTD_CMD_B
> ASE | 0x01)
> +#define IPC_PUNIT_GTD_READ_ICCP_LIC_CDYN_SCAL (IPC_PUNIT_GTD_CMD_B
> ASE | 0x02)
> +#define IPC_PUNIT_GTD_WRITE_ICCP_LIC_CDYN_SCAL (IPC_PUNIT_GTD_CMD_B
> ASE | 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_B
> ASE | 0x07)
> +#define IPC_PUNIT_GTD_READ_REQ_DUTY_CYCLE (IPC_PUNIT_GTD_CMD_B
> ASE | 0x16)
> +#define IPC_PUNIT_GTD_DIS_VOL_FREQ_CHG_REQUEST (IPC_PUNIT_GTD_CMD_B
> ASE | 0x17)
> +#define IPC_PUNIT_GTD_DYNA_DUTY_CYCLE_CTRL (IPC_PUNIT_GTD_CMD_B
> ASE | 0x1a)
> +#define IPC_PUNIT_GTD_DYNA_DUTY_CYCLE_TUNING (IPC_PUNIT_GTD_CMD_B
> ASE | 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_I
> SPD_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 346f1fd..9948369 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -891,4 +891,10 @@ config INTEL_PMC_IPC
> The PMC is an ARC processor which defines IPC commands for
> communication
> with other entities in the CPU.
>
> +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 1051372..eea765f 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_INTEL_SMARTCONNECT) += intel
> -smartconnect.o
> obj-$(CONFIG_PVPANIC) += pvpanic.o
> obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
> obj-$(CONFIG_INTEL_PMC_IPC) += intel_pmc_ipc.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..c78ab57
> --- /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 DATA_LOW 0x0
> +#define INTERFACE 0x4
> +#define CMD_RUN (1 << 31)
> +#define CMD_ERRCODE_MASK 0xFF
> +#define CMD_PARA1_SHIFT 8
> +#define CMD_PARA2_SHIFT 16
> +#define DATA_HIGH 0x8
> +
> +#define MAILBOX_REGISTER_SPACE 0x10
> +#define CMD_TIMEOUT_SECONDS 1
> +
> +typedef struct {
> + struct device *dev;
> + struct mutex lock;
> + int irq;
> + struct completion cmd_complete;
> + void __iomem *base[RESERVED_IPC];
> + 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_LOW);
> +}
> +
> +static inline u32 ipc_read_data_high(IPC_DEV *ipcdev, IPC_TYPE type)
> +{
> + return readl(ipcdev->base[type] + DATA_HIGH);
> +}
> +
> +static inline void ipc_write_data_low(IPC_DEV *ipcdev, IPC_TYPE
> type, u32 data)
> +{
> + writel(data, ipcdev->base[type] + DATA_LOW);
> +}
> +
> +static inline void ipc_write_data_high(IPC_DEV *ipcdev, IPC_TYPE
> type, u32 data)
> +{
> + writel(data, ipcdev->base[type] + 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 *res0, *res1;
> + void __iomem *addr;
> + int size;
> +
> + res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res0) {
> + dev_err(&pdev->dev, "Failed to get iomem
> resource\n");
> + return -ENXIO;
> + }
> + size = resource_size(res0);
> + if (!devm_request_mem_region(&pdev->dev, res0->start,
> + size, pdev->name)) {
> + dev_err(&pdev->dev, "Failed to request iomem
> resouce\n");
> + return -EBUSY;
> + }
> +
> + res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res1) {
> + dev_err(&pdev->dev, "Failed to get iomem
> resource1\n");
> + return -ENXIO;
> + }
> + size = resource_size(res1);
> + if (!devm_request_mem_region(&pdev->dev, res1->start,
> + size, pdev->name)) {
> + dev_err(&pdev->dev, "Failed to request iomem
> resouce1\n");
> + return -EBUSY;
> + }
> +
> + addr = ioremap_nocache(res0->start,
> + resource_size(res0) +
> resource_size(res1));
> + if (!addr) {
> + dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
> + return -ENOMEM;
> + }
> + punit_ipcdev->base[BIOS_IPC] = addr;
> + addr += MAILBOX_REGISTER_SPACE;
> + punit_ipcdev->base[GTDRIVER_IPC] = addr;
> + addr += MAILBOX_REGISTER_SPACE;
> + punit_ipcdev->base[ISPDRIVER_IPC] = 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)
> +{
> + IPC_DEV *ipcdev = platform_get_drvdata(pdev);
> +
> + iounmap(ipcdev->base[BIOS_IPC]);
> +
> + 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);
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-09 13:33 ` Shevchenko, Andriy
@ 2015-10-10 3:07 ` Zha, Qipeng
2015-10-15 5:16 ` Darren Hart
2015-10-15 10:35 ` Shevchenko, Andriy
0 siblings, 2 replies; 20+ messages in thread
From: Zha, Qipeng @ 2015-10-10 3:07 UTC (permalink / raw)
To: Shevchenko, Andriy, platform-driver-x86; +Cc: dvhart, Westerberg, Mika
>Everything is quite okay, except this BAR thingy.
>Can you provide a DSDT excerpt for the device to see what is there?
>I can't find such device (by ACPI id) in the tables of the accessible hardware in our lab.
Please check below acpi device definition from BIOS.
Punit device is created in pmc driver, since BIOS finally reject to create a separate device for Punit.
Scope (\_SB) {
Device(IPC1)
{
…
Name (RBUF, ResourceTemplate ()
{
Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0) // IPC1 Bar
// Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) // SSRAM
Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS mailbox Data
Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
IO (Decode16, 0x400, 0x480, 0x4, 0x80) // ACPI IO Base address
Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, , , ) {40} // IPC1 IRQ
})
…
}
}//end scope
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-10 3:07 ` Zha, Qipeng
@ 2015-10-15 5:16 ` Darren Hart
2015-10-21 12:51 ` Darren Hart
2015-10-15 10:35 ` Shevchenko, Andriy
1 sibling, 1 reply; 20+ messages in thread
From: Darren Hart @ 2015-10-15 5:16 UTC (permalink / raw)
To: Zha, Qipeng; +Cc: Shevchenko, Andriy, platform-driver-x86, Westerberg, Mika
On Sat, Oct 10, 2015 at 03:07:53AM +0000, Zha, Qipeng wrote:
>
> >Everything is quite okay, except this BAR thingy.
>
> >Can you provide a DSDT excerpt for the device to see what is there?
>
> >I can't find such device (by ACPI id) in the tables of the accessible hardware in our lab.
>
> Please check below acpi device definition from BIOS.
> Punit device is created in pmc driver, since BIOS finally reject to create a separate device for Punit.
>
Qipeng, sorry for the delay. I've been under a rather absurd load. I'd like to
close on this so we can get this into -next ASAP. Thank you for posting the
DSDT.
Help us parse what this means so we're all seeing the same thing. I see a total
of 3 memory addresses and the following IO base address. Unfortunately, the
comments don't map directly to the driver code, so I need to piece this
together.
The code in question from the driver is:
#define MAILBOX_REGISTER_SPACE 0x10
+static int intel_punit_get_bars(struct platform_device *pdev)
+{
+ struct resource *res0, *res1;
+ void __iomem *addr;
+ int size;
+
+ res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res0) {
+ dev_err(&pdev->dev, "Failed to get iomem resource\n");
+ return -ENXIO;
+ }
+ size = resource_size(res0);
+ if (!devm_request_mem_region(&pdev->dev, res0->start,
+ size, pdev->name)) {
+ dev_err(&pdev->dev, "Failed to request iomem resouce\n");
+ return -EBUSY;
+ }
+
+ res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res1) {
+ dev_err(&pdev->dev, "Failed to get iomem resource1\n");
+ return -ENXIO;
+ }
+ size = resource_size(res1);
+ if (!devm_request_mem_region(&pdev->dev, res1->start,
+ size, pdev->name)) {
+ dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
+ return -EBUSY;
+ }
+
+ addr = ioremap_nocache(res0->start,
+ resource_size(res0) + resource_size(res1));
+ if (!addr) {
+ dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
+ return -ENOMEM;
+ }
+ punit_ipcdev->base[BIOS_IPC] = addr;
+ addr += MAILBOX_REGISTER_SPACE;
+ punit_ipcdev->base[GTDRIVER_IPC] = addr;
+ addr += MAILBOX_REGISTER_SPACE;
+ punit_ipcdev->base[ISPDRIVER_IPC] = addr;
+
+ return 0;
+}
> Scope (\_SB) {
> Device(IPC1)
> {
> …
> Name (RBUF, ResourceTemplate ()
> {
> Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0) // IPC1 Bar
size: 0x1000
IPC1 BAR, I presume this maps to res0 in the driver?
Then the start of this 4096 byte region is assigned by:
punit_ipcdev->base[BIOS_IPC] = addr;
And everything else assigned by intel_punit_get_bards is contained within this
addr since MAILBOX_REGISTER_SPACE is only 0x10.
Do I have that correct?
> // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) // SSRAM
> Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS mailbox Data
size: 0x1000
And this would be res1 in the driver?
> Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
size: 0x1000
This seems odd, especially with the comments in the DSD. A more natural mapping would have been each of the Memory32Fixed macros mapping to the BIOS_IPC, GTDRIVER_IPC, and ISPDRIVER_IPC BARs in the driver. This would indeed be the case if MAILBOX_REGISTER_SPACE was 0x1000 instead of 0x10. Also, if that were the case, then those BARs should be set by using the mapped addr for the 3 separate resources - which is what Andy was getting at.
Qipeng, I assume I misinterpretted something above. Can you point out where I've got this wrong and how the driver is doing the only thing it can given the resources provided by the DSDT?
Thanks,
> IO (Decode16, 0x400, 0x480, 0x4, 0x80) // ACPI IO Base address
> Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, , , ) {40} // IPC1 IRQ
> })
>
> …
> }
> }//end scope
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-10 3:07 ` Zha, Qipeng
2015-10-15 5:16 ` Darren Hart
@ 2015-10-15 10:35 ` Shevchenko, Andriy
2015-10-15 10:39 ` Shevchenko, Andriy
1 sibling, 1 reply; 20+ messages in thread
From: Shevchenko, Andriy @ 2015-10-15 10:35 UTC (permalink / raw)
To: platform-driver-x86, Zha, Qipeng; +Cc: dvhart, Westerberg, Mika
On Sat, 2015-10-10 at 03:07 +0000, Zha, Qipeng wrote:
> >
> > Everything is quite okay, except this BAR thingy.
>
> > Can you provide a DSDT excerpt for the device to see what is there?
>
> > I can't find such device (by ACPI id) in the tables of the
> > accessible hardware in our lab.
>
> Please check below acpi device definition from BIOS.
> Punit device is created in pmc driver, since BIOS finally reject to
> create a separate device for Punit.
Thank you for mention this one. It's unfortunately a show stopper for
using this module as a driver (you can't assign two drivers to the same
device). You have to convert is to a library.
Moreover, I briefly looked at the intel_pmc_ipc and it should be
refactored a in a few ways: a) split to core part, PCI driver, and ACPI
driver, b) improved regarding to comments you got in this review (many
comments are applied to what we have there).
Darren, your opinion?
>
> Scope (\_SB) {
> Device(IPC1)
> {
> …
> Name (RBUF, ResourceTemplate ()
> {
> Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0) //
> IPC1 Bar
> // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) //
> SSRAM
> Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) //
> PUnit BIOS mailbox Data
> Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) //
> PUnit BIOS mailbox Interface and GTD/ISPD mailbox
> IO (Decode16, 0x400, 0x480, 0x4, 0x80) //
> ACPI IO Base address
> Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, , ,
> ) {40} // IPC1 IRQ
> })
>
> …
> }
> }//end scope
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-15 10:35 ` Shevchenko, Andriy
@ 2015-10-15 10:39 ` Shevchenko, Andriy
2015-10-15 15:29 ` Darren Hart
0 siblings, 1 reply; 20+ messages in thread
From: Shevchenko, Andriy @ 2015-10-15 10:39 UTC (permalink / raw)
To: platform-driver-x86, Zha, Qipeng; +Cc: dvhart, Westerberg, Mika
On Thu, 2015-10-15 at 13:35 +0300, Andy Shevchenko wrote:
> On Sat, 2015-10-10 at 03:07 +0000, Zha, Qipeng wrote:
> > >
> > > Everything is quite okay, except this BAR thingy.
> >
> > > Can you provide a DSDT excerpt for the device to see what is
> > > there?
> >
> > > I can't find such device (by ACPI id) in the tables of the
> > > accessible hardware in our lab.
> >
> > Please check below acpi device definition from BIOS.
> > Punit device is created in pmc driver, since BIOS finally reject to
> >
> > create a separate device for Punit.
>
> Thank you for mention this one. It's unfortunately a show stopper for
> using this module as a driver (you can't assign two drivers to the
> same
> device). You have to convert is to a library.
Oh, I'm sorry, I really missed that the IDs are different.
So, discard this part.
>
> Moreover, I briefly looked at the intel_pmc_ipc and it should be
> refactored a in a few ways: a) split to core part, PCI driver, and
> ACPI
> driver, b) improved regarding to comments you got in this review
> (many
> comments are applied to what we have there).
>
> Darren, your opinion?
This by the way still valid.
>
> >
> > Scope (\_SB) {
> > Device(IPC1)
> > {
> > …
> > Name (RBUF, ResourceTemplate ()
> > {
> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0)
> > //
> > IPC1 Bar
> > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1)
> > //
> > SSRAM
> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT)
> > //
> > PUnit BIOS mailbox Data
> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF)
> > //
> > PUnit BIOS mailbox Interface and GTD/ISPD mailbox
> > IO (Decode16, 0x400, 0x480, 0x4, 0x80)
> > //
> > ACPI IO Base address
> > Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,
> > ,
> > ) {40} // IPC1 IRQ
> > })
> >
> > …
> > }
> > }//end scope
>
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-15 10:39 ` Shevchenko, Andriy
@ 2015-10-15 15:29 ` Darren Hart
2015-10-15 15:36 ` Shevchenko, Andriy
0 siblings, 1 reply; 20+ messages in thread
From: Darren Hart @ 2015-10-15 15:29 UTC (permalink / raw)
To: Shevchenko, Andriy; +Cc: platform-driver-x86, Zha, Qipeng, Westerberg, Mika
On Thu, Oct 15, 2015 at 10:39:35AM +0000, Shevchenko, Andriy wrote:
> On Thu, 2015-10-15 at 13:35 +0300, Andy Shevchenko wrote:
> > On Sat, 2015-10-10 at 03:07 +0000, Zha, Qipeng wrote:
> > > >
> > > > Everything is quite okay, except this BAR thingy.
> > >
> > > > Can you provide a DSDT excerpt for the device to see what is
> > > > there?
> > >
> > > > I can't find such device (by ACPI id) in the tables of the
> > > > accessible hardware in our lab.
> > >
> > > Please check below acpi device definition from BIOS.
> > > Punit device is created in pmc driver, since BIOS finally reject to
> > >
> > > create a separate device for Punit.
> >
> > Thank you for mention this one. It's unfortunately a show stopper for
> > using this module as a driver (you can't assign two drivers to the
> > same
> > device). You have to convert is to a library.
>
> Oh, I'm sorry, I really missed that the IDs are different.
> So, discard this part.
>
> >
> > Moreover, I briefly looked at the intel_pmc_ipc and it should be
> > refactored a in a few ways: a) split to core part, PCI driver, and
> > ACPI
> > driver, b) improved regarding to comments you got in this review
> > (many
> > comments are applied to what we have there).
> >
> > Darren, your opinion?
>
> This by the way still valid.
I'm a bit confused. Are you just bringing up that the intel_pmc_ipc driver could
also be improved? Or are you sugesting that this driver needs to be refactored
together with the intel_pmc_ipc driver?
With respect to the intel_punit_ipc driver, I've tried to parse/map the DSDT to
the BAR mapping in the driver which you raised concerns about. Can you review
that bit and help us come to a conclusion on that since you asked for the DSDT?
(See separate response to Qipeng on that).
Thanks,
>
> >
> > >
> > > Scope (\_SB) {
> > > Device(IPC1)
> > > {
> > > …
> > > Name (RBUF, ResourceTemplate ()
> > > {
> > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0)
> > > //
> > > IPC1 Bar
> > > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1)
> > > //
> > > SSRAM
> > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT)
> > > //
> > > PUnit BIOS mailbox Data
> > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF)
> > > //
> > > PUnit BIOS mailbox Interface and GTD/ISPD mailbox
> > > IO (Decode16, 0x400, 0x480, 0x4, 0x80)
> > > //
> > > ACPI IO Base address
> > > Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,
> > > ,
> > > ) {40} // IPC1 IRQ
> > > })
> > >
> > > …
> > > }
> > > }//end scope
> >
>
> --
> Andy Shevchenko <andriy.shevchenko@intel.com>
> Intel Finland Oy
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki
> Business Identity Code: 0357606 - 4
> Domiciled in Helsinki
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-15 15:29 ` Darren Hart
@ 2015-10-15 15:36 ` Shevchenko, Andriy
0 siblings, 0 replies; 20+ messages in thread
From: Shevchenko, Andriy @ 2015-10-15 15:36 UTC (permalink / raw)
To: dvhart; +Cc: platform-driver-x86, Zha, Qipeng, Westerberg, Mika
On Thu, 2015-10-15 at 08:29 -0700, Darren Hart wrote:
> On Thu, Oct 15, 2015 at 10:39:35AM +0000, Shevchenko, Andriy wrote:
> > On Thu, 2015-10-15 at 13:35 +0300, Andy Shevchenko wrote:
> > > On Sat, 2015-10-10 at 03:07 +0000, Zha, Qipeng wrote:
> > > > >
> > > > > Everything is quite okay, except this BAR thingy.
> > > >
> > > > > Can you provide a DSDT excerpt for the device to see what is
> > > > > there?
> > > >
> > > > > I can't find such device (by ACPI id) in the tables of the
> > > > > accessible hardware in our lab.
> > > >
> > > > Please check below acpi device definition from BIOS.
> > > > Punit device is created in pmc driver, since BIOS finally
> > > > reject to
> > > >
> > > > create a separate device for Punit.
> > >
> > >
> > > Moreover, I briefly looked at the intel_pmc_ipc and it should be
> > > refactored a in a few ways: a) split to core part, PCI driver,
> > > and
> > > ACPI
> > > driver, b) improved regarding to comments you got in this review
> > > (many
> > > comments are applied to what we have there).
> > >
> > > Darren, your opinion?
> >
> > This by the way still valid.
>
> I'm a bit confused.
Sorry for this.
> Are you just bringing up that the intel_pmc_ipc driver could
> also be improved?
>
That is, but for future. It's not a concern right now.
>
> With respect to the intel_punit_ipc driver, I've tried to parse/map
> the DSDT to
> the BAR mapping in the driver which you raised concerns about. Can
> you review
> that bit and help us come to a conclusion on that since you asked for
> the DSDT?
> (See separate response to Qipeng on that).
Yes, I'm follow the discussion. I would like to understand what those
three resources for? And there is one IO (IO resource) for unclear
reason.
>
> Thanks,
>
> >
> > >
> > > >
> > > > Scope (\_SB) {
> > > > Device(IPC1)
> > > > {
> > > > …
> > > > Name (RBUF, ResourceTemplate ()
> > > > {
> > > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0)
> > > >
> > > > //
> > > > IPC1 Bar
> > > > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000,
> > > > BAR1)
> > > > //
> > > > SSRAM
> > > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT)
> > > >
> > > > //
> > > > PUnit BIOS mailbox Data
> > > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF)
> > > >
> > > > //
> > > > PUnit BIOS mailbox Interface and GTD/ISPD mailbox
> > > > IO (Decode16, 0x400, 0x480, 0x4, 0x80)
> > > >
> > > > //
> > > > ACPI IO Base address
> > > > Interrupt (ResourceConsumer, Level, ActiveLow,
> > > > Exclusive, ,
> > > > ,
> > > > ) {40} // IPC1 IRQ
> > > > })
> > > >
> > > > …
> > > > }
> > > > }//end scope
> > >
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-15 5:16 ` Darren Hart
@ 2015-10-21 12:51 ` Darren Hart
2015-10-22 1:01 ` Zha, Qipeng
0 siblings, 1 reply; 20+ messages in thread
From: Darren Hart @ 2015-10-21 12:51 UTC (permalink / raw)
To: Zha, Qipeng; +Cc: Shevchenko, Andriy, platform-driver-x86, Westerberg, Mika
Qipeng, can you comment on my understanding of the DSDT and the driver?
On Wed, Oct 14, 2015 at 10:16:06PM -0700, Darren Hart wrote:
> On Sat, Oct 10, 2015 at 03:07:53AM +0000, Zha, Qipeng wrote:
> >
> > >Everything is quite okay, except this BAR thingy.
> >
> > >Can you provide a DSDT excerpt for the device to see what is there?
> >
> > >I can't find such device (by ACPI id) in the tables of the accessible hardware in our lab.
> >
> > Please check below acpi device definition from BIOS.
> > Punit device is created in pmc driver, since BIOS finally reject to create a separate device for Punit.
> >
>
> Qipeng, sorry for the delay. I've been under a rather absurd load. I'd like to
> close on this so we can get this into -next ASAP. Thank you for posting the
> DSDT.
>
> Help us parse what this means so we're all seeing the same thing. I see a total
> of 3 memory addresses and the following IO base address. Unfortunately, the
> comments don't map directly to the driver code, so I need to piece this
> together.
>
> The code in question from the driver is:
>
> #define MAILBOX_REGISTER_SPACE 0x10
>
> +static int intel_punit_get_bars(struct platform_device *pdev)
> +{
> + struct resource *res0, *res1;
> + void __iomem *addr;
> + int size;
> +
> + res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res0) {
> + dev_err(&pdev->dev, "Failed to get iomem resource\n");
> + return -ENXIO;
> + }
> + size = resource_size(res0);
> + if (!devm_request_mem_region(&pdev->dev, res0->start,
> + size, pdev->name)) {
> + dev_err(&pdev->dev, "Failed to request iomem resouce\n");
> + return -EBUSY;
> + }
> +
> + res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res1) {
> + dev_err(&pdev->dev, "Failed to get iomem resource1\n");
> + return -ENXIO;
> + }
> + size = resource_size(res1);
> + if (!devm_request_mem_region(&pdev->dev, res1->start,
> + size, pdev->name)) {
> + dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
> + return -EBUSY;
> + }
> +
> + addr = ioremap_nocache(res0->start,
> + resource_size(res0) + resource_size(res1));
> + if (!addr) {
> + dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
> + return -ENOMEM;
> + }
> + punit_ipcdev->base[BIOS_IPC] = addr;
> + addr += MAILBOX_REGISTER_SPACE;
> + punit_ipcdev->base[GTDRIVER_IPC] = addr;
> + addr += MAILBOX_REGISTER_SPACE;
> + punit_ipcdev->base[ISPDRIVER_IPC] = addr;
> +
> + return 0;
> +}
>
>
> > Scope (\_SB) {
> > Device(IPC1)
> > {
> > …
> > Name (RBUF, ResourceTemplate ()
> > {
> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0) // IPC1 Bar
>
> size: 0x1000
> IPC1 BAR, I presume this maps to res0 in the driver?
>
> Then the start of this 4096 byte region is assigned by:
>
> punit_ipcdev->base[BIOS_IPC] = addr;
>
> And everything else assigned by intel_punit_get_bards is contained within this
> addr since MAILBOX_REGISTER_SPACE is only 0x10.
>
> Do I have that correct?
>
> > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) // SSRAM
> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS mailbox Data
>
> size: 0x1000
> And this would be res1 in the driver?
>
> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
>
> size: 0x1000
>
> This seems odd, especially with the comments in the DSD. A more natural mapping would have been each of the Memory32Fixed macros mapping to the BIOS_IPC, GTDRIVER_IPC, and ISPDRIVER_IPC BARs in the driver. This would indeed be the case if MAILBOX_REGISTER_SPACE was 0x1000 instead of 0x10. Also, if that were the case, then those BARs should be set by using the mapped addr for the 3 separate resources - which is what Andy was getting at.
>
> Qipeng, I assume I misinterpretted something above. Can you point out where I've got this wrong and how the driver is doing the only thing it can given the resources provided by the DSDT?
>
> Thanks,
>
>
> > IO (Decode16, 0x400, 0x480, 0x4, 0x80) // ACPI IO Base address
> > Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, , , ) {40} // IPC1 IRQ
> > })
> >
> > …
> > }
> > }//end scope
>
> --
> Darren Hart
> Intel Open Source Technology Center
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-21 12:51 ` Darren Hart
@ 2015-10-22 1:01 ` Zha, Qipeng
2015-10-22 8:04 ` Darren Hart
0 siblings, 1 reply; 20+ messages in thread
From: Zha, Qipeng @ 2015-10-22 1:01 UTC (permalink / raw)
To: Darren Hart; +Cc: Shevchenko, Andriy, platform-driver-x86, Westerberg, Mika
>>Qipeng, can you comment on my understanding of the DSDT and the driver?
>> > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) // SSRAM
>> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS mailbox Data
>> size: 0x1000
>> And this would be res1 in the driver?
>> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
When boot up, BIOS will rewrite the size of these entries, actually the size for this entry will change to 4B, not the default 0x1000.
This is real strange implementation for us, as before mentioned, BIOS implement like this to make it compatible for wos driver.
On Wed, Oct 14, 2015 at 10:16:06PM -0700, Darren Hart wrote:
> On Sat, Oct 10, 2015 at 03:07:53AM +0000, Zha, Qipeng wrote:
> >
> > >Everything is quite okay, except this BAR thingy.
> >
> > >Can you provide a DSDT excerpt for the device to see what is there?
> >
> > >I can't find such device (by ACPI id) in the tables of the accessible hardware in our lab.
> >
> > Please check below acpi device definition from BIOS.
> > Punit device is created in pmc driver, since BIOS finally reject to create a separate device for Punit.
> >
>
> Qipeng, sorry for the delay. I've been under a rather absurd load. I'd
> like to close on this so we can get this into -next ASAP. Thank you
> for posting the DSDT.
>
> Help us parse what this means so we're all seeing the same thing. I
> see a total of 3 memory addresses and the following IO base address.
> Unfortunately, the comments don't map directly to the driver code, so
> I need to piece this together.
>
> The code in question from the driver is:
>
> #define MAILBOX_REGISTER_SPACE 0x10
>
> +static int intel_punit_get_bars(struct platform_device *pdev) {
> + struct resource *res0, *res1;
> + void __iomem *addr;
> + int size;
> +
> + res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res0) {
> + dev_err(&pdev->dev, "Failed to get iomem resource\n");
> + return -ENXIO;
> + }
> + size = resource_size(res0);
> + if (!devm_request_mem_region(&pdev->dev, res0->start,
> + size, pdev->name)) {
> + dev_err(&pdev->dev, "Failed to request iomem resouce\n");
> + return -EBUSY;
> + }
> +
> + res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res1) {
> + dev_err(&pdev->dev, "Failed to get iomem resource1\n");
> + return -ENXIO;
> + }
> + size = resource_size(res1);
> + if (!devm_request_mem_region(&pdev->dev, res1->start,
> + size, pdev->name)) {
> + dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
> + return -EBUSY;
> + }
> +
> + addr = ioremap_nocache(res0->start,
> + resource_size(res0) + resource_size(res1));
> + if (!addr) {
> + dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
> + return -ENOMEM;
> + }
> + punit_ipcdev->base[BIOS_IPC] = addr;
> + addr += MAILBOX_REGISTER_SPACE;
> + punit_ipcdev->base[GTDRIVER_IPC] = addr;
> + addr += MAILBOX_REGISTER_SPACE;
> + punit_ipcdev->base[ISPDRIVER_IPC] = addr;
> +
> + return 0;
> +}
>
>
> > Scope (\_SB) {
> > Device(IPC1)
> > {
> > …
> > Name (RBUF, ResourceTemplate ()
> > {
> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0) // IPC1 Bar
>
> size: 0x1000
> IPC1 BAR, I presume this maps to res0 in the driver?
>
> Then the start of this 4096 byte region is assigned by:
>
> punit_ipcdev->base[BIOS_IPC] = addr;
>
> And everything else assigned by intel_punit_get_bards is contained
> within this addr since MAILBOX_REGISTER_SPACE is only 0x10.
>
> Do I have that correct?
>
> > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) // SSRAM
> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS mailbox Data
>
> size: 0x1000
> And this would be res1 in the driver?
>
> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
>
> size: 0x1000
>
> This seems odd, especially with the comments in the DSD. A more natural mapping would have been each of the Memory32Fixed macros mapping to the BIOS_IPC, GTDRIVER_IPC, and ISPDRIVER_IPC BARs in the driver. This would indeed be the case if MAILBOX_REGISTER_SPACE was 0x1000 instead of 0x10. Also, if that were the case, then those BARs should be set by using the mapped addr for the 3 separate resources - which is what Andy was getting at.
>
> Qipeng, I assume I misinterpretted something above. Can you point out where I've got this wrong and how the driver is doing the only thing it can given the resources provided by the DSDT?
>
> Thanks,
>
>
> > IO (Decode16, 0x400, 0x480, 0x4, 0x80) // ACPI IO Base address
> > Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, , , ) {40} // IPC1 IRQ
> > })
> >
> > …
> > }
> > }//end scope
>
> --
> Darren Hart
> Intel Open Source Technology Center
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-22 1:01 ` Zha, Qipeng
@ 2015-10-22 8:04 ` Darren Hart
2015-10-22 8:35 ` Shevchenko, Andriy
2015-10-22 15:18 ` Zha, Qipeng
0 siblings, 2 replies; 20+ messages in thread
From: Darren Hart @ 2015-10-22 8:04 UTC (permalink / raw)
To: Zha, Qipeng, Rafael Wysocki
Cc: Shevchenko, Andriy, platform-driver-x86, Westerberg, Mika
On Thu, Oct 22, 2015 at 01:01:32AM +0000, Zha, Qipeng wrote:
> >>Qipeng, can you comment on my understanding of the DSDT and the driver?
> >> > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) // SSRAM
> >> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS mailbox Data
> >> size: 0x1000
> >> And this would be res1 in the driver?
> >> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
>
> When boot up, BIOS will rewrite the size of these entries, actually the size for this entry will change to 4B, not the default 0x1000.
> This is real strange implementation for us, as before mentioned, BIOS implement like this to make it compatible for wos driver.
>
+Rafael - we're having some trouble mapping the ACPI declared resources to the
driver code using them. Qipeng has indicated that the BIOS team is doing
something rather bizarre to meet some requirement from the WOS drivers. Can you
have a look at this thread? I haven't been able to map the ACPI resources to the
driver resources in a way that makes sense to me, and this is holding up merging
the driver.
Qipeng, this is very strange indeed. How does BIOS change this in a way that
doesn't change the resources ACPI reports to the OS?
Is the ASL fragment you provided collected from a running Linux system using
acpidump? If so, then the resources ACPI advertises to the OS are not actually
available... and that can't be acceptable.
Qipeng, I want to get this driver merged, but we have to do the due diligence to
understand how these resources are being used. I've asked several questions to
try and clarify this, but your responses have been very short and do not fully
address the questions. I need your help to fully describe what is going on with
the BARs before I can sign this off and ask Linus to merge it.
Thanks,
Darren
> On Wed, Oct 14, 2015 at 10:16:06PM -0700, Darren Hart wrote:
> > On Sat, Oct 10, 2015 at 03:07:53AM +0000, Zha, Qipeng wrote:
> > >
> > > >Everything is quite okay, except this BAR thingy.
> > >
> > > >Can you provide a DSDT excerpt for the device to see what is there?
> > >
> > > >I can't find such device (by ACPI id) in the tables of the accessible hardware in our lab.
> > >
> > > Please check below acpi device definition from BIOS.
> > > Punit device is created in pmc driver, since BIOS finally reject to create a separate device for Punit.
> > >
> >
> > Qipeng, sorry for the delay. I've been under a rather absurd load. I'd
> > like to close on this so we can get this into -next ASAP. Thank you
> > for posting the DSDT.
> >
> > Help us parse what this means so we're all seeing the same thing. I
> > see a total of 3 memory addresses and the following IO base address.
> > Unfortunately, the comments don't map directly to the driver code, so
> > I need to piece this together.
> >
> > The code in question from the driver is:
> >
> > #define MAILBOX_REGISTER_SPACE 0x10
> >
> > +static int intel_punit_get_bars(struct platform_device *pdev) {
> > + struct resource *res0, *res1;
> > + void __iomem *addr;
> > + int size;
> > +
> > + res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res0) {
> > + dev_err(&pdev->dev, "Failed to get iomem resource\n");
> > + return -ENXIO;
> > + }
> > + size = resource_size(res0);
> > + if (!devm_request_mem_region(&pdev->dev, res0->start,
> > + size, pdev->name)) {
> > + dev_err(&pdev->dev, "Failed to request iomem resouce\n");
> > + return -EBUSY;
> > + }
> > +
> > + res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!res1) {
> > + dev_err(&pdev->dev, "Failed to get iomem resource1\n");
> > + return -ENXIO;
> > + }
> > + size = resource_size(res1);
> > + if (!devm_request_mem_region(&pdev->dev, res1->start,
> > + size, pdev->name)) {
> > + dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
> > + return -EBUSY;
> > + }
> > +
> > + addr = ioremap_nocache(res0->start,
> > + resource_size(res0) + resource_size(res1));
> > + if (!addr) {
> > + dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
> > + return -ENOMEM;
> > + }
> > + punit_ipcdev->base[BIOS_IPC] = addr;
> > + addr += MAILBOX_REGISTER_SPACE;
> > + punit_ipcdev->base[GTDRIVER_IPC] = addr;
> > + addr += MAILBOX_REGISTER_SPACE;
> > + punit_ipcdev->base[ISPDRIVER_IPC] = addr;
> > +
> > + return 0;
> > +}
> >
> >
> > > Scope (\_SB) {
> > > Device(IPC1)
> > > {
> > > …
> > > Name (RBUF, ResourceTemplate ()
> > > {
> > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0) // IPC1 Bar
> >
> > size: 0x1000
> > IPC1 BAR, I presume this maps to res0 in the driver?
> >
> > Then the start of this 4096 byte region is assigned by:
> >
> > punit_ipcdev->base[BIOS_IPC] = addr;
> >
> > And everything else assigned by intel_punit_get_bards is contained
> > within this addr since MAILBOX_REGISTER_SPACE is only 0x10.
> >
> > Do I have that correct?
> >
> > > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) // SSRAM
> > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS mailbox Data
> >
> > size: 0x1000
> > And this would be res1 in the driver?
> >
> > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
> >
> > size: 0x1000
> >
> > This seems odd, especially with the comments in the DSD. A more natural mapping would have been each of the Memory32Fixed macros mapping to the BIOS_IPC, GTDRIVER_IPC, and ISPDRIVER_IPC BARs in the driver. This would indeed be the case if MAILBOX_REGISTER_SPACE was 0x1000 instead of 0x10. Also, if that were the case, then those BARs should be set by using the mapped addr for the 3 separate resources - which is what Andy was getting at.
> >
> > Qipeng, I assume I misinterpretted something above. Can you point out where I've got this wrong and how the driver is doing the only thing it can given the resources provided by the DSDT?
> >
> > Thanks,
> >
> >
> > > IO (Decode16, 0x400, 0x480, 0x4, 0x80) // ACPI IO Base address
> > > Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, , , ) {40} // IPC1 IRQ
> > > })
> > >
> > > …
> > > }
> > > }//end scope
> >
> > --
> > Darren Hart
> > Intel Open Source Technology Center
>
> --
> Darren Hart
> Intel Open Source Technology Center
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-22 8:04 ` Darren Hart
@ 2015-10-22 8:35 ` Shevchenko, Andriy
2015-10-22 15:18 ` Zha, Qipeng
1 sibling, 0 replies; 20+ messages in thread
From: Shevchenko, Andriy @ 2015-10-22 8:35 UTC (permalink / raw)
To: Darren Hart, Zha, Qipeng, Rafael Wysocki
Cc: platform-driver-x86, Westerberg, Mika
> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Thursday, October 22, 2015 11:05 AM
> To: Zha, Qipeng; Rafael Wysocki
> Cc: Shevchenko, Andriy; platform-driver-x86@vger.kernel.org; Westerberg,
> Mika
> Subject: Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
>
> On Thu, Oct 22, 2015 at 01:01:32AM +0000, Zha, Qipeng wrote:
> > >>Qipeng, can you comment on my understanding of the DSDT and the
> driver?
> > >> > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) //
> SSRAM
> > >> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) //
> PUnit BIOS mailbox Data
> > >> size: 0x1000
> > >> And this would be res1 in the driver?
> > >> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) //
> PUnit BIOS mailbox Interface and GTD/ISPD mailbox
> >
> > When boot up, BIOS will rewrite the size of these entries, actually the size
> for this entry will change to 4B, not the default 0x1000.
> > This is real strange implementation for us, as before mentioned, BIOS
> implement like this to make it compatible for wos driver.
> >
>
> +Rafael - we're having some trouble mapping the ACPI declared resources
> +to the
> driver code using them. Qipeng has indicated that the BIOS team is doing
> something rather bizarre to meet some requirement from the WOS drivers.
> Can you have a look at this thread? I haven't been able to map the ACPI
> resources to the driver resources in a way that makes sense to me, and this is
> holding up merging the driver.
>
> Qipeng, this is very strange indeed. How does BIOS change this in a way that
> doesn't change the resources ACPI reports to the OS?
>
> Is the ASL fragment you provided collected from a running Linux system
> using acpidump? If so, then the resources ACPI advertises to the OS are not
> actually available... and that can't be acceptable.
+1. Please confirm that the excerpt has been copied from running Linux OS machine, otherwise please provide the *real* (run-time) excerpt from DSDT using either acpidump or manually copied from /sys/firmware/acpi/tables/DSDT (IIRC).
>
> Qipeng, I want to get this driver merged, but we have to do the due diligence
> to understand how these resources are being used. I've asked several
> questions to try and clarify this, but your responses have been very short and
> do not fully address the questions. I need your help to fully describe what is
> going on with the BARs before I can sign this off and ask Linus to merge it.
--
Andy Shevchenko <andriy.shevchenko@intel.com>
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-22 8:04 ` Darren Hart
2015-10-22 8:35 ` Shevchenko, Andriy
@ 2015-10-22 15:18 ` Zha, Qipeng
2015-10-22 15:43 ` Darren Hart
1 sibling, 1 reply; 20+ messages in thread
From: Zha, Qipeng @ 2015-10-22 15:18 UTC (permalink / raw)
To: Darren Hart, Rafael Wysocki
Cc: Shevchenko, Andriy, platform-driver-x86, Westerberg, Mika, Xue, Gavin
Darren, Andriy:
Thanks for your kind review, try to make clear as below.
+Gavin, Our BIOS developer.
On Thu, Oct 22, 2015 at 01:01:32AM +0000, Zha, Qipeng wrote:
> >>Qipeng, can you comment on my understanding of the DSDT and the driver?
> >> > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) // SSRAM
> >> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS mailbox Data
> >> size: 0x1000
> >> And this would be res1 in the driver?
> >> > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
>
> When boot up, BIOS will rewrite the size of these entries, actually the size for this entry will change to 4B, not the default 0x1000.
> This is real strange implementation for us, as before mentioned, BIOS implement like this to make it compatible for wos driver.
>
>+Rafael - we're having some trouble mapping the ACPI declared resources
>+to the
>driver code using them. Qipeng has indicated that the BIOS team is doing something rather bizarre to meet some requirement from the WOS drivers. Can you have a look at this thread? I haven't been able to map the ACPI resources to the driver resources in a way that makes sense to me, and this is holding up merging the driver.
>Qipeng, this is very strange indeed. How does BIOS change this in a way that doesn't change the resources ACPI reports to the OS?
>Is the ASL fragment you provided collected from a running Linux system using acpidump? If so, then the resources ACPI advertises to the OS are not actually available... and that can't be acceptable.
>Qipeng, I want to get this driver merged, but we have to do the due diligence to understand how these resources are being used. I've asked several questions to try and clarify this, but your responses have been very short and do not fully address the questions. I need your help to fully describe what is going on with the BARs before I can sign this off and ask Linus to merge it.
>Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS mailbox Data
This is map to res0
> Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
This is map to res1
These two entries are firstly parsed in pmc driver firstly and reported to punit driver.
>Is the ASL fragment you provided collected from a running Linux system using acpidump? If so, then the resources ACPI advertises to the OS are not actually available... and that can't be acceptable.
This is real ASL code I got from BIOS team and validated on bxt platform.
Confirmed BIOS runtime code will update "acpi table" in memory before switch to os, for those resource which need to get/read from hardware dynamically, in this case, MINF and MDAT is set in runtime when boot BIOS and size of res0 is set as 4B.
> On Wed, Oct 14, 2015 at 10:16:06PM -0700, Darren Hart wrote:
> > On Sat, Oct 10, 2015 at 03:07:53AM +0000, Zha, Qipeng wrote:
> > >
> > > >Everything is quite okay, except this BAR thingy.
> > >
> > > >Can you provide a DSDT excerpt for the device to see what is there?
> > >
> > > >I can't find such device (by ACPI id) in the tables of the accessible hardware in our lab.
> > >
> > > Please check below acpi device definition from BIOS.
> > > Punit device is created in pmc driver, since BIOS finally reject to create a separate device for Punit.
> > >
> >
> > Qipeng, sorry for the delay. I've been under a rather absurd load.
> > I'd like to close on this so we can get this into -next ASAP. Thank
> > you for posting the DSDT.
> >
> > Help us parse what this means so we're all seeing the same thing. I
> > see a total of 3 memory addresses and the following IO base address.
> > Unfortunately, the comments don't map directly to the driver code,
> > so I need to piece this together.
> >
> > The code in question from the driver is:
> >
> > #define MAILBOX_REGISTER_SPACE 0x10
> >
> > +static int intel_punit_get_bars(struct platform_device *pdev) {
> > + struct resource *res0, *res1;
> > + void __iomem *addr;
> > + int size;
> > +
> > + res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res0) {
> > + dev_err(&pdev->dev, "Failed to get iomem resource\n");
> > + return -ENXIO;
> > + }
> > + size = resource_size(res0);
> > + if (!devm_request_mem_region(&pdev->dev, res0->start,
> > + size, pdev->name)) {
> > + dev_err(&pdev->dev, "Failed to request iomem resouce\n");
> > + return -EBUSY;
> > + }
> > +
> > + res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!res1) {
> > + dev_err(&pdev->dev, "Failed to get iomem resource1\n");
> > + return -ENXIO;
> > + }
> > + size = resource_size(res1);
> > + if (!devm_request_mem_region(&pdev->dev, res1->start,
> > + size, pdev->name)) {
> > + dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
> > + return -EBUSY;
> > + }
> > +
> > + addr = ioremap_nocache(res0->start,
> > + resource_size(res0) + resource_size(res1));
> > + if (!addr) {
> > + dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
> > + return -ENOMEM;
> > + }
> > + punit_ipcdev->base[BIOS_IPC] = addr;
> > + addr += MAILBOX_REGISTER_SPACE;
> > + punit_ipcdev->base[GTDRIVER_IPC] = addr;
> > + addr += MAILBOX_REGISTER_SPACE;
> > + punit_ipcdev->base[ISPDRIVER_IPC] = addr;
> > +
> > + return 0;
> > +}
> >
> >
> > > Scope (\_SB) {
> > > Device(IPC1)
> > > {
> > > …
> > > Name (RBUF, ResourceTemplate ()
> > > {
> > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0) // IPC1 Bar
> >
> > size: 0x1000
> > IPC1 BAR, I presume this maps to res0 in the driver?
> >
> > Then the start of this 4096 byte region is assigned by:
> >
> > punit_ipcdev->base[BIOS_IPC] = addr;
> >
> > And everything else assigned by intel_punit_get_bards is contained
> > within this addr since MAILBOX_REGISTER_SPACE is only 0x10.
> >
> > Do I have that correct?
> >
> > > // Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) // SSRAM
> > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS mailbox Data
> >
> > size: 0x1000
> > And this would be res1 in the driver?
> >
> > > Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
> >
> > size: 0x1000
> >
> > This seems odd, especially with the comments in the DSD. A more natural mapping would have been each of the Memory32Fixed macros mapping to the BIOS_IPC, GTDRIVER_IPC, and ISPDRIVER_IPC BARs in the driver. This would indeed be the case if MAILBOX_REGISTER_SPACE was 0x1000 instead of 0x10. Also, if that were the case, then those BARs should be set by using the mapped addr for the 3 separate resources - which is what Andy was getting at.
> >
> > Qipeng, I assume I misinterpretted something above. Can you point out where I've got this wrong and how the driver is doing the only thing it can given the resources provided by the DSDT?
> >
> > Thanks,
> >
> >
> > > IO (Decode16, 0x400, 0x480, 0x4, 0x80) // ACPI IO Base address
> > > Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, , , ) {40} // IPC1 IRQ
> > > })
> > >
> > > …
> > > }
> > > }//end scope
> >
> > --
> > Darren Hart
> > Intel Open Source Technology Center
>
> --
> Darren Hart
> Intel Open Source Technology Center
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-22 15:18 ` Zha, Qipeng
@ 2015-10-22 15:43 ` Darren Hart
2015-10-24 13:35 ` Rafael J. Wysocki
2015-10-26 8:51 ` Zha, Qipeng
0 siblings, 2 replies; 20+ messages in thread
From: Darren Hart @ 2015-10-22 15:43 UTC (permalink / raw)
To: Zha, Qipeng
Cc: Rafael Wysocki, Shevchenko, Andriy, platform-driver-x86,
Westerberg, Mika, Xue, Gavin
On Thu, Oct 22, 2015 at 03:18:16PM +0000, Zha, Qipeng wrote:
> >Is the ASL fragment you provided collected from a running Linux system using
> >acpidump? If so, then the resources ACPI advertises to the OS are not
> >actually available... and that can't be acceptable.
> This is real ASL code I got from BIOS team and validated on bxt platform.
> Confirmed BIOS runtime code will update "acpi table" in memory before switch
> to os, for those resource which need to get/read from hardware dynamically,
> in this case, MINF and MDAT is set in runtime when boot BIOS and size of res0
> is set as 4B.
So the ASL you provided was not what the Linux kernel is seeing, correct?
Can you please provide a DSDT disassembly from the running Linux system please,
such as:
# cp /sys/firmware/acpi/tables/DSDT DSDT.dat
# iasl -d DSDT.dat
Then find this device in DSDT.dsl and paste it here please.
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-22 15:43 ` Darren Hart
@ 2015-10-24 13:35 ` Rafael J. Wysocki
2015-10-26 8:51 ` Zha, Qipeng
1 sibling, 0 replies; 20+ messages in thread
From: Rafael J. Wysocki @ 2015-10-24 13:35 UTC (permalink / raw)
To: Darren Hart, Zha, Qipeng
Cc: Shevchenko, Andriy, platform-driver-x86, Westerberg, Mika, Xue, Gavin
On Thursday, October 22, 2015 05:43:07 PM Darren Hart wrote:
> On Thu, Oct 22, 2015 at 03:18:16PM +0000, Zha, Qipeng wrote:
> > >Is the ASL fragment you provided collected from a running Linux system using
> > >acpidump? If so, then the resources ACPI advertises to the OS are not
> > >actually available... and that can't be acceptable.
> > This is real ASL code I got from BIOS team and validated on bxt platform.
> > Confirmed BIOS runtime code will update "acpi table" in memory before switch
> > to os, for those resource which need to get/read from hardware dynamically,
> > in this case, MINF and MDAT is set in runtime when boot BIOS and size of res0
> > is set as 4B.
>
> So the ASL you provided was not what the Linux kernel is seeing, correct?
>
> Can you please provide a DSDT disassembly from the running Linux system please,
> such as:
>
> # cp /sys/firmware/acpi/tables/DSDT DSDT.dat
> # iasl -d DSDT.dat
>
> Then find this device in DSDT.dsl and paste it here please.
Or even better send the full output of acpidump from that system (when running).
Thanks,
Rafael
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-22 15:43 ` Darren Hart
2015-10-24 13:35 ` Rafael J. Wysocki
@ 2015-10-26 8:51 ` Zha, Qipeng
2015-10-27 12:30 ` Shevchenko, Andriy
1 sibling, 1 reply; 20+ messages in thread
From: Zha, Qipeng @ 2015-10-26 8:51 UTC (permalink / raw)
To: Darren Hart
Cc: Rafael Wysocki, Shevchenko, Andriy, platform-driver-x86,
Westerberg, Mika, Xue, Gavin
>So the ASL you provided was not what the Linux kernel is seeing, correct?
>Can you please provide a DSDT disassembly from the running Linux system please, such as:
># cp /sys/firmware/acpi/tables/DSDT DSDT.dat # iasl -d DSDT.dat
>Then find this device in DSDT.dsl and paste it here please.
Sorry for late feedback, got dsdt from lab machine since my board got broken,
Please check.
Scope (\_SB)
{
Device (IPC1)
{
Name (_ADR, Zero) // _ADR: Address
Name (_HID, "INT34D2") // _HID: Hardware ID
Name (_CID, "INT34D2") // _CID: Compatible ID
Name (_DDN, "Intel(R) IPCI controller ") // _DDN: DOS Device Name
Name (_UID, One) // _UID: Unique ID
Name (RBUF, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
0x00000000, // Address Base
0x00002000, // Address Length
_Y08)
Memory32Fixed (ReadWrite,
0x00000000, // Address Base
0x00000004, // Address Length
_Y09)
Memory32Fixed (ReadWrite,
0x00000000, // Address Base
0x00000040, // Address Length
_Y0A)
IO (Decode16,
0x0400, // Range Minimum
0x0480, // Range Maximum
0x04, // Alignment
0x80, // Length
)
Memory32Fixed (ReadWrite,
0x00000000, // Address Base
0x00002000, // Address Length
_Y0B)
Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, )
{
0x00000028,
}
})
Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
{
CreateDWordField (RBUF, \_SB.IPC1._Y08._BAS, B0BA) // _BAS: Base Address
CreateDWordField (RBUF, \_SB.IPC1._Y08._LEN, B0LN) // _LEN: Length
Store (DD1A, B0BA)
Store (DD1L, B0LN)
CreateDWordField (RBUF, \_SB.IPC1._Y09._BAS, BM01) // _BAS: Base Address
CreateDWordField (RBUF, \_SB.IPC1._Y09._LEN, BML1) // _LEN: Length
CreateDWordField (RBUF, \_SB.IPC1._Y0A._BAS, BM02) // _BAS: Base Address
CreateDWordField (RBUF, \_SB.IPC1._Y0A._LEN, BML2) // _LEN: Length
Store (BMDA, BM01)
Store (0x04, BML1)
Store (BMIA, BM02)
Store (0x40, BML2)
CreateDWordField (RBUF, \_SB.IPC1._Y0B._BAS, B1BA) // _BAS: Base Address
CreateDWordField (RBUF, \_SB.IPC1._Y0B._LEN, B1LN) // _LEN: Length
Store (DD3A, B1BA)
Store (DD3L, B1LN)
Return (RBUF)
}
Method (_STA, 0, NotSerialized) // _STA: Status
{
Return (0x0F)
}
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-26 8:51 ` Zha, Qipeng
@ 2015-10-27 12:30 ` Shevchenko, Andriy
2015-10-28 1:27 ` Darren Hart
0 siblings, 1 reply; 20+ messages in thread
From: Shevchenko, Andriy @ 2015-10-27 12:30 UTC (permalink / raw)
To: Zha, Qipeng, dvhart
Cc: Xue, Gavin, platform-driver-x86, rjw, Westerberg, Mika
On Mon, 2015-10-26 at 08:51 +0000, Zha, Qipeng wrote:
> > So the ASL you provided was not what the Linux kernel is seeing,
> > correct?
>
> > Can you please provide a DSDT disassembly from the running Linux
> > system please, such as:
>
> > # cp /sys/firmware/acpi/tables/DSDT DSDT.dat # iasl -d DSDT.dat
>
> > Then find this device in DSDT.dsl and paste it here please.
>
> Sorry for late feedback, got dsdt from lab machine since my board
> got broken,
> Please check.
>
> Scope (\_SB)
> {
> Device (IPC1)
> {
> Name (_ADR, Zero) // _ADR: Address
> Name (_HID, "INT34D2") // _HID: Hardware ID
> Name (_CID, "INT34D2") // _CID: Compatible ID
> Name (_DDN, "Intel(R) IPCI controller ") // _DDN:
> DOS Device Name
> Name (_UID, One) // _UID: Unique ID
> Name (RBUF, ResourceTemplate ()
> {
> Memory32Fixed (ReadWrite,
> 0x00000000, // Address Base
> 0x00002000, // Address Length
> _Y08)
> Memory32Fixed (ReadWrite,
> 0x00000000, // Address Base
> 0x00000004, // Address Length
> _Y09)
> Memory32Fixed (ReadWrite,
> 0x00000000, // Address Base
> 0x00000040, // Address Length
> _Y0A)
> IO (Decode16,
> 0x0400, // Range Minimum
> 0x0480, // Range Maximum
> 0x04, // Alignment
> 0x80, // Length
> )
> Memory32Fixed (ReadWrite,
> 0x00000000, // Address Base
> 0x00002000, // Address Length
> _Y0B)
> Interrupt (ResourceConsumer, Level, ActiveLow,
> Exclusive, ,, )
> {
> 0x00000028,
> }
> })
> Method (_CRS, 0, NotSerialized) // _CRS: Current
> Resource Settings
> {
> CreateDWordField (RBUF, \_SB.IPC1._Y08._BAS,
> B0BA) // _BAS: Base Address
> CreateDWordField (RBUF, \_SB.IPC1._Y08._LEN,
> B0LN) // _LEN: Length
> Store (DD1A, B0BA)
> Store (DD1L, B0LN)
dd1a, dd1l has been stored to Resource 0
> CreateDWordField (RBUF, \_SB.IPC1._Y09._BAS,
> BM01) // _BAS: Base Address
> CreateDWordField (RBUF, \_SB.IPC1._Y09._LEN,
> BML1) // _LEN: Length
> CreateDWordField (RBUF, \_SB.IPC1._Y0A._BAS,
> BM02) // _BAS: Base Address
> CreateDWordField (RBUF, \_SB.IPC1._Y0A._LEN,
> BML2) // _LEN: Length
> Store (BMDA, BM01)
> Store (0x04, BML1)
> Store (BMIA, BM02)
> Store (0x40, BML2)
bmda, 0x04 -> Resource 1
bmia, 0x40 -> Resource 2
> CreateDWordField (RBUF, \_SB.IPC1._Y0B._BAS,
> B1BA) // _BAS: Base Address
> CreateDWordField (RBUF, \_SB.IPC1._Y0B._LEN,
> B1LN) // _LEN: Length
> Store (DD3A, B1BA)
> Store (DD3L, B1LN)
dd3a, dd3l -> Resource 3
Can you create a temporary method in the ->probe() of this driver to
iterate over resources and print them out? Or tell the values BIOS set
per values dd1a, dd1l, dd3a, dd3l, bmda, bmia.
AFAIU you are using only resources 0 and 1. Can you put here small
description of what each resource is meant for? (I guess couple of them
to P-Unit, and couple related to what you are trying to get, right?)
Also, it's not clear the purpose of IO resource. What is for?
> Return (RBUF)
> }
>
> Method (_STA, 0, NotSerialized) // _STA: Status
> {
> Return (0x0F)
> }
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-27 12:30 ` Shevchenko, Andriy
@ 2015-10-28 1:27 ` Darren Hart
2015-10-30 6:11 ` Zha, Qipeng
0 siblings, 1 reply; 20+ messages in thread
From: Darren Hart @ 2015-10-28 1:27 UTC (permalink / raw)
To: Shevchenko, Andriy
Cc: Zha, Qipeng, Xue, Gavin, platform-driver-x86, rjw, Westerberg, Mika
On Tue, Oct 27, 2015 at 12:30:31PM +0000, Shevchenko, Andriy wrote:
> On Mon, 2015-10-26 at 08:51 +0000, Zha, Qipeng wrote:
> > > So the ASL you provided was not what the Linux kernel is seeing,
> > > correct?
> >
> > > Can you please provide a DSDT disassembly from the running Linux
> > > system please, such as:
> >
> > > # cp /sys/firmware/acpi/tables/DSDT DSDT.dat # iasl -d DSDT.dat
> >
> > > Then find this device in DSDT.dsl and paste it here please.
> >
> > Sorry for late feedback, got dsdt from lab machine since my board
> > got broken,
> > Please check.
> >
> > Scope (\_SB)
> > {
> > Device (IPC1)
> > {
> > Name (_ADR, Zero) // _ADR: Address
> > Name (_HID, "INT34D2") // _HID: Hardware ID
> > Name (_CID, "INT34D2") // _CID: Compatible ID
> > Name (_DDN, "Intel(R) IPCI controller ") // _DDN:
> > DOS Device Name
> > Name (_UID, One) // _UID: Unique ID
> > Name (RBUF, ResourceTemplate ()
> > {
> > Memory32Fixed (ReadWrite,
> > 0x00000000, // Address Base
> > 0x00002000, // Address Length
> > _Y08)
> > Memory32Fixed (ReadWrite,
> > 0x00000000, // Address Base
> > 0x00000004, // Address Length
> > _Y09)
> > Memory32Fixed (ReadWrite,
> > 0x00000000, // Address Base
> > 0x00000040, // Address Length
> > _Y0A)
> > IO (Decode16,
> > 0x0400, // Range Minimum
> > 0x0480, // Range Maximum
> > 0x04, // Alignment
> > 0x80, // Length
> > )
> > Memory32Fixed (ReadWrite,
> > 0x00000000, // Address Base
> > 0x00002000, // Address Length
> > _Y0B)
> > Interrupt (ResourceConsumer, Level, ActiveLow,
> > Exclusive, ,, )
> > {
> > 0x00000028,
> > }
> > })
> > Method (_CRS, 0, NotSerialized) // _CRS: Current
> > Resource Settings
> > {
> > CreateDWordField (RBUF, \_SB.IPC1._Y08._BAS,
> > B0BA) // _BAS: Base Address
> > CreateDWordField (RBUF, \_SB.IPC1._Y08._LEN,
> > B0LN) // _LEN: Length
> > Store (DD1A, B0BA)
> > Store (DD1L, B0LN)
>
> dd1a, dd1l has been stored to Resource 0
>
> > CreateDWordField (RBUF, \_SB.IPC1._Y09._BAS,
> > BM01) // _BAS: Base Address
> > CreateDWordField (RBUF, \_SB.IPC1._Y09._LEN,
> > BML1) // _LEN: Length
> > CreateDWordField (RBUF, \_SB.IPC1._Y0A._BAS,
> > BM02) // _BAS: Base Address
> > CreateDWordField (RBUF, \_SB.IPC1._Y0A._LEN,
> > BML2) // _LEN: Length
> > Store (BMDA, BM01)
> > Store (0x04, BML1)
> > Store (BMIA, BM02)
> > Store (0x40, BML2)
>
> bmda, 0x04 -> Resource 1
> bmia, 0x40 -> Resource 2
>
> > CreateDWordField (RBUF, \_SB.IPC1._Y0B._BAS,
> > B1BA) // _BAS: Base Address
> > CreateDWordField (RBUF, \_SB.IPC1._Y0B._LEN,
> > B1LN) // _LEN: Length
> > Store (DD3A, B1BA)
> > Store (DD3L, B1LN)
>
> dd3a, dd3l -> Resource 3
>
>
> Can you create a temporary method in the ->probe() of this driver to
> iterate over resources and print them out? Or tell the values BIOS set
> per values dd1a, dd1l, dd3a, dd3l, bmda, bmia.
>
> AFAIU you are using only resources 0 and 1. Can you put here small
> description of what each resource is meant for? (I guess couple of them
> to P-Unit, and couple related to what you are trying to get, right?)
While the driver code merges the ranges of res0 and res1:
line 244-245:
addr = ioremap_nocache(res0->start,
resource_size(res0) + resource_size(res1));
It appears that we're using very little of this:
punit_ipcdev->base[BIOS_IPC] = addr;
addr += MAILBOX_REGISTER_SPACE;
punit_ipcdev->base[GTDRIVER_IPC] = addr;
addr += MAILBOX_REGISTER_SPACE;
punit_ipcdev->base[ISPDRIVER_IPC] = addr;
MAILBOX_REGISTER_SPACE is 0x10, but I don't know how long ISPDRIVER_IPC is
expected to be, but it apears to be 4 bytes. This would mean we're using a total
of 0x24 starting res0->start. res0 has a length of 0x2000 per the ASL above, and
we're only referencing 0x24 of it.
Why, then, do we merge the lengths of res0 and res1, presumably to 0x2004, when
we only use 0x24?
Or, am I misreading this?
Also, Qipeng, you mentioned earlier that the firmware reported a length of 0x4B
I believe? I don't see that in this ASL.
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-28 1:27 ` Darren Hart
@ 2015-10-30 6:11 ` Zha, Qipeng
2015-10-30 9:56 ` Shevchenko, Andriy
0 siblings, 1 reply; 20+ messages in thread
From: Zha, Qipeng @ 2015-10-30 6:11 UTC (permalink / raw)
To: Darren Hart, Shevchenko, Andriy
Cc: Xue, Gavin, platform-driver-x86, rjw, Westerberg, Mika
> On Mon, 2015-10-26 at 08:51 +0000, Zha, Qipeng wrote:
> > > So the ASL you provided was not what the Linux kernel is seeing,
> > > correct?
> >
> > > Can you please provide a DSDT disassembly from the running Linux
> > > system please, such as:
> >
> > > # cp /sys/firmware/acpi/tables/DSDT DSDT.dat # iasl -d DSDT.dat
> >
> > > Then find this device in DSDT.dsl and paste it here please.
> >
> > Sorry for late feedback, got dsdt from lab machine since my board
> > got broken, Please check.
> >
> > Scope (\_SB)
> > {
> > Device (IPC1)
> > {
> > Name (_ADR, Zero) // _ADR: Address
> > Name (_HID, "INT34D2") // _HID: Hardware ID
> > Name (_CID, "INT34D2") // _CID: Compatible ID
> > Name (_DDN, "Intel(R) IPCI controller ") // _DDN:
> > DOS Device Name
> > Name (_UID, One) // _UID: Unique ID
> > Name (RBUF, ResourceTemplate ()
> > {
> > Memory32Fixed (ReadWrite,
> > 0x00000000, // Address Base
> > 0x00002000, // Address Length
> > _Y08)
> > Memory32Fixed (ReadWrite,
> > 0x00000000, // Address Base
> > 0x00000004, // Address Length
> > _Y09)
> > Memory32Fixed (ReadWrite,
> > 0x00000000, // Address Base
> > 0x00000040, // Address Length
> > _Y0A)
> > IO (Decode16,
> > 0x0400, // Range Minimum
> > 0x0480, // Range Maximum
> > 0x04, // Alignment
> > 0x80, // Length
> > )
> > Memory32Fixed (ReadWrite,
> > 0x00000000, // Address Base
> > 0x00002000, // Address Length
> > _Y0B)
> > Interrupt (ResourceConsumer, Level, ActiveLow,
> > Exclusive, ,, )
> > {
> > 0x00000028,
> > }
> > })
> > Method (_CRS, 0, NotSerialized) // _CRS: Current
> > Resource Settings
> > {
> > CreateDWordField (RBUF, \_SB.IPC1._Y08._BAS,
> > B0BA) // _BAS: Base Address
> > CreateDWordField (RBUF, \_SB.IPC1._Y08._LEN,
> > B0LN) // _LEN: Length
> > Store (DD1A, B0BA)
> > Store (DD1L, B0LN)
>
> dd1a, dd1l has been stored to Resource 0
>
> > CreateDWordField (RBUF, \_SB.IPC1._Y09._BAS,
> > BM01) // _BAS: Base Address
> > CreateDWordField (RBUF, \_SB.IPC1._Y09._LEN,
> > BML1) // _LEN: Length
> > CreateDWordField (RBUF, \_SB.IPC1._Y0A._BAS,
> > BM02) // _BAS: Base Address
> > CreateDWordField (RBUF, \_SB.IPC1._Y0A._LEN,
> > BML2) // _LEN: Length
> > Store (BMDA, BM01)
> > Store (0x04, BML1)
> > Store (BMIA, BM02)
> > Store (0x40, BML2)
>
> bmda, 0x04 -> Resource 1
> bmia, 0x40 -> Resource 2
>
> > CreateDWordField (RBUF, \_SB.IPC1._Y0B._BAS,
> > B1BA) // _BAS: Base Address
> > CreateDWordField (RBUF, \_SB.IPC1._Y0B._LEN,
> > B1LN) // _LEN: Length
> > Store (DD3A, B1BA)
> > Store (DD3L, B1LN)
>
> dd3a, dd3l -> Resource 3
>
>
> Can you create a temporary method in the ->probe() of this driver to
> iterate over resources and print them out? Or tell the values BIOS set
> per values dd1a, dd1l, dd3a, dd3l, bmda, bmia.
>
> AFAIU you are using only resources 0 and 1. Can you put here small
> description of what each resource is meant for? (I guess couple of
> them to P-Unit, and couple related to what you are trying to get,
> right?)
>While the driver code merges the ranges of res0 and res1:
>line 244-245:
addr = ioremap_nocache(res0->start,
resource_size(res0) + resource_size(res1));
>It appears that we're using very little of this:
> punit_ipcdev->base[BIOS_IPC] = addr;
> addr += MAILBOX_REGISTER_SPACE;
> punit_ipcdev->base[GTDRIVER_IPC] = addr;
> addr += MAILBOX_REGISTER_SPACE;
> punit_ipcdev->base[ISPDRIVER_IPC] = addr;
>MAILBOX_REGISTER_SPACE is 0x10, but I don't know how long ISPDRIVER_IPC is expected to be, but it apears to be 4 bytes. This would mean we're using a total of 0x24 starting res0->start. res0 has a length of 0x2000 per the ASL above, and we're only referencing 0x24 of it.
>Why, then, do we merge the lengths of res0 and res1, presumably to 0x2004, when we only use 0x24?
>Or, am I misreading this?
>Also, Qipeng, you mentioned earlier that the firmware reported a length of 0x4B I believe? I don't see that in this ASL.
Memory32Fixed (ReadWrite,
0x00000000, // Address Base
0x00002000, // Address Length
_Y08)
dd1a, dd1l -> Resource 0
This is PMC controller memory space, not related to Punit.
bmda, 0x04 -> Resource 1
bmia, 0x40 -> Resource 2
These two are for Punit memory space, and bmia = bmda + 4,
They are mapped to res0,res1 in Punit driver, size of res0 is set as 4B(Store (0x04, BML1)).
Other resource are for other PMC functions, not related to Punit either.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver
2015-10-30 6:11 ` Zha, Qipeng
@ 2015-10-30 9:56 ` Shevchenko, Andriy
0 siblings, 0 replies; 20+ messages in thread
From: Shevchenko, Andriy @ 2015-10-30 9:56 UTC (permalink / raw)
To: Zha, Qipeng, dvhart
Cc: Westerberg, Mika, platform-driver-x86, Xue, Gavin, rjw
On Fri, 2015-10-30 at 06:11 +0000, Zha, Qipeng wrote:
> > On Mon, 2015-10-26 at 08:51 +0000, Zha, Qipeng wrote:
> > > > So the ASL you provided was not what the Linux kernel is
> > > > seeing,
> > > > correct?
> > >
> > > > Can you please provide a DSDT disassembly from the running
> > > > Linux
> > > > system please, such as:
> > >
> > > > # cp /sys/firmware/acpi/tables/DSDT DSDT.dat # iasl -d DSDT.dat
> > >
> > > > Then find this device in DSDT.dsl and paste it here please.
> > >
> > > Sorry for late feedback, got dsdt from lab machine since my
> > > board
> > > got broken, Please check.
> > >
> > > Scope (\_SB)
> > > {
> > > Device (IPC1)
> > > {
> > > Name (_ADR, Zero) // _ADR: Address
> > > Name (_HID, "INT34D2") // _HID: Hardware ID
> > > Name (_CID, "INT34D2") // _CID: Compatible ID
> > > Name (_DDN, "Intel(R) IPCI controller ") //
> > > _DDN:
> > > DOS Device Name
> > > Name (_UID, One) // _UID: Unique ID
> > > Name (RBUF, ResourceTemplate ()
> > > {
> > > Memory32Fixed (ReadWrite,
> > > 0x00000000, // Address Base
> > > 0x00002000, // Address Length
> > > _Y08)
> > > Memory32Fixed (ReadWrite,
> > > 0x00000000, // Address Base
> > > 0x00000004, // Address Length
> > > _Y09)
> > > Memory32Fixed (ReadWrite,
> > > 0x00000000, // Address Base
> > > 0x00000040, // Address Length
> > > _Y0A)
> > > IO (Decode16,
> > > 0x0400, // Range Minimum
> > > 0x0480, // Range Maximum
> > > 0x04, // Alignment
> > > 0x80, // Length
> > > )
> > > Memory32Fixed (ReadWrite,
> > > 0x00000000, // Address Base
> > > 0x00002000, // Address Length
> > > _Y0B)
> > > Interrupt (ResourceConsumer, Level,
> > > ActiveLow,
> > > Exclusive, ,, )
> > > {
> > > 0x00000028,
> > > }
> > > })
> > > Method (_CRS, 0, NotSerialized) // _CRS: Current
> > > Resource Settings
> > > {
> > > CreateDWordField (RBUF, \_SB.IPC1._Y08._BAS,
> > > B0BA) // _BAS: Base Address
> > > CreateDWordField (RBUF, \_SB.IPC1._Y08._LEN,
> > > B0LN) // _LEN: Length
> > > Store (DD1A, B0BA)
> > > Store (DD1L, B0LN)
> >
> > dd1a, dd1l has been stored to Resource 0
> >
> > > CreateDWordField (RBUF, \_SB.IPC1._Y09._BAS,
> > > BM01) // _BAS: Base Address
> > > CreateDWordField (RBUF, \_SB.IPC1._Y09._LEN,
> > > BML1) // _LEN: Length
> > > CreateDWordField (RBUF, \_SB.IPC1._Y0A._BAS,
> > > BM02) // _BAS: Base Address
> > > CreateDWordField (RBUF, \_SB.IPC1._Y0A._LEN,
> > > BML2) // _LEN: Length
> > > Store (BMDA, BM01)
> > > Store (0x04, BML1)
> > > Store (BMIA, BM02)
> > > Store (0x40, BML2)
> >
> > bmda, 0x04 -> Resource 1
> > bmia, 0x40 -> Resource 2
> >
> > > CreateDWordField (RBUF, \_SB.IPC1._Y0B._BAS,
> > > B1BA) // _BAS: Base Address
> > > CreateDWordField (RBUF, \_SB.IPC1._Y0B._LEN,
> > > B1LN) // _LEN: Length
> > > Store (DD3A, B1BA)
> > > Store (DD3L, B1LN)
> >
> > dd3a, dd3l -> Resource 3
> >
> >
> > Can you create a temporary method in the ->probe() of this driver
> > to
> > iterate over resources and print them out? Or tell the values BIOS
> > set
> > per values dd1a, dd1l, dd3a, dd3l, bmda, bmia.
> >
> > AFAIU you are using only resources 0 and 1. Can you put here small
> > description of what each resource is meant for? (I guess couple of
> > them to P-Unit, and couple related to what you are trying to get,
> > right?)
>
> > While the driver code merges the ranges of res0 and res1:
>
> > line 244-245:
> addr = ioremap_nocache(res0->start,
> resource_size(res0) +
> resource_size(res1));
>
> > It appears that we're using very little of this:
>
> > punit_ipcdev->base[BIOS_IPC] = addr;
> > addr += MAILBOX_REGISTER_SPACE;
> > punit_ipcdev->base[GTDRIVER_IPC] = addr;
> > addr += MAILBOX_REGISTER_SPACE;
> > punit_ipcdev->base[ISPDRIVER_IPC] = addr;
>
> > MAILBOX_REGISTER_SPACE is 0x10, but I don't know how long
> > ISPDRIVER_IPC is expected to be, but it apears to be 4 bytes. This
> > would mean we're using a total of 0x24 starting res0->start. res0
> > has a length of 0x2000 per the ASL above, and we're only
> > referencing 0x24 of it.
>
> > Why, then, do we merge the lengths of res0 and res1, presumably to
> > 0x2004, when we only use 0x24?
>
> > Or, am I misreading this?
>
> > Also, Qipeng, you mentioned earlier that the firmware reported a
> > length of 0x4B I believe? I don't see that in this ASL.
>
>
> Memory32Fixed (ReadWrite,
> 0x00000000, // Address Base
> 0x00002000, // Address Length
> _Y08)
> dd1a, dd1l -> Resource 0
> This is PMC controller memory space, not related to Punit.
>
> bmda, 0x04 -> Resource 1
> bmia, 0x40 -> Resource 2
> These two are for Punit memory space, and bmia = bmda + 4,
> They are mapped to res0,res1 in Punit driver, size of res0 is set as
> 4B(Store (0x04, BML1)).
>
> Other resource are for other PMC functions, not related to Punit
> either.
Are the values top secret? Can you please provide all numbers,
including not relevant?
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2015-10-30 9:58 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-09 8:49 [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver Qipeng Zha
2015-10-09 13:33 ` Shevchenko, Andriy
2015-10-10 3:07 ` Zha, Qipeng
2015-10-15 5:16 ` Darren Hart
2015-10-21 12:51 ` Darren Hart
2015-10-22 1:01 ` Zha, Qipeng
2015-10-22 8:04 ` Darren Hart
2015-10-22 8:35 ` Shevchenko, Andriy
2015-10-22 15:18 ` Zha, Qipeng
2015-10-22 15:43 ` Darren Hart
2015-10-24 13:35 ` Rafael J. Wysocki
2015-10-26 8:51 ` Zha, Qipeng
2015-10-27 12:30 ` Shevchenko, Andriy
2015-10-28 1:27 ` Darren Hart
2015-10-30 6:11 ` Zha, Qipeng
2015-10-30 9:56 ` Shevchenko, Andriy
2015-10-15 10:35 ` Shevchenko, Andriy
2015-10-15 10:39 ` Shevchenko, Andriy
2015-10-15 15:29 ` Darren Hart
2015-10-15 15:36 ` Shevchenko, Andriy
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.