[PATCH] platform:x86: add Intel Punit mailbox IPC driver

[PATCH] platform:x86: add Intel Punit mailbox IPC driver

[Qipeng Zha]

This driver provides support for Punit mailbox IPC on Intel platforms.
The heart of the Punit is the Foxton microcontroller and its firmware,
which provide mailbox interface for power management usage.

Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
 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 | 388 +++++++++++++++++++++++++++++++++
 4 files changed, 496 insertions(+)
 create mode 100644 arch/x86/include/asm/intel_punit_ipc.h
 create mode 100644 drivers/platform/x86/intel_punit_ipc.c

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..08e3e14
--- /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_
+
+/* Commands supported on GLM core, are handled by different bars,
+ * unify these commands together here
+ */
+#define IPC_BIOS_PUNIT_CMD_BASE				0x00
+#define IPC_GTD_PUNIT_CMD_BASE				0x20
+#define IPC_ISPD_PUNIT_CMD_BASE				0x40
+
+/* BIOS => Pcode commands */
+#define IPC_BIOS_PUNIT_CMD_ZERO			(IPC_BIOS_PUNIT_CMD_BASE + 0x00)
+#define IPC_BIOS_PUNIT_CMD_VR_INTERFACE		(IPC_BIOS_PUNIT_CMD_BASE + 0x01)
+#define IPC_BIOS_PUNIT_CMD_READ_PCS		(IPC_BIOS_PUNIT_CMD_BASE + 0x02)
+#define IPC_BIOS_PUNIT_CMD_WRITE_PCS		(IPC_BIOS_PUNIT_CMD_BASE + 0x03)
+#define IPC_BIOS_PUNIT_CMD_READ_PCU_CONFIG	(IPC_BIOS_PUNIT_CMD_BASE + 0x04)
+#define IPC_BIOS_PUNIT_CMD_WRITE_PCU_CONFIG	(IPC_BIOS_PUNIT_CMD_BASE + 0x05)
+#define IPC_BIOS_PUNIT_CMD_READ_PL1_SETTING	(IPC_BIOS_PUNIT_CMD_BASE + 0x06)
+#define IPC_BIOS_PUNIT_CMD_WRITE_PL1_SETTING	(IPC_BIOS_PUNIT_CMD_BASE + 0x07)
+#define IPC_BIOS_PUNIT_CMD_TRIGGER_VDD_RAM	(IPC_BIOS_PUNIT_CMD_BASE + 0x08)
+#define IPC_BIOS_PUNIT_CMD_READ_TELE_INFO	(IPC_BIOS_PUNIT_CMD_BASE + 0x09)
+#define IPC_BIOS_PUNIT_CMD_READ_TELE_TRACE_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x0a)
+#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_TRACE_CTRL \
+						(IPC_BIOS_PUNIT_CMD_BASE + 0x0b)
+#define IPC_BIOS_PUNIT_CMD_READ_TELE_EVENT_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x0c)
+#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_EVENT_CTRL \
+						(IPC_BIOS_PUNIT_CMD_BASE + 0x0d)
+#define IPC_BIOS_PUNIT_CMD_READ_TELE_TRACE	(IPC_BIOS_PUNIT_CMD_BASE + 0x0e)
+#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_TRACE	(IPC_BIOS_PUNIT_CMD_BASE + 0x0f)
+#define IPC_BIOS_PUNIT_CMD_READ_TELE_EVENT	(IPC_BIOS_PUNIT_CMD_BASE + 0x10)
+#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_EVENT	(IPC_BIOS_PUNIT_CMD_BASE + 0x11)
+#define IPC_BIOS_PUNIT_CMD_READ_MODULE_TEMP	(IPC_BIOS_PUNIT_CMD_BASE + 0x12)
+#define IPC_BIOS_PUNIT_CMD_RESERVED		(IPC_BIOS_PUNIT_CMD_BASE + 0x13)
+#define IPC_BIOS_PUNIT_CMD_READ_VOLTAGE_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x14)
+#define IPC_BIOS_PUNIT_CMD_WRITE_VOLTAGE_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x15)
+#define IPC_BIOS_PUNIT_CMD_READ_RATIO_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x16)
+#define IPC_BIOS_PUNIT_CMD_WRITE_RATIO_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x17)
+#define IPC_BIOS_PUNIT_CMD_READ_VF_GL_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x18)
+#define IPC_BIOS_PUNIT_CMD_WRITE_VF_GL_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x19)
+#define IPC_BIOS_PUNIT_CMD_READ_FM_SOC_TEMP_THRESH \
+						(IPC_BIOS_PUNIT_CMD_BASE + 0x1a)
+#define IPC_BIOS_PUNIT_CMD_WRITE_FM_SOC_TEMP_THRESH \
+						(IPC_BIOS_PUNIT_CMD_BASE + 0x1b)
+
+/*GT Driver => Pcode commands*/
+#define IPC_GTD_PUNIT_CMD_ZERO			(IPC_GTD_PUNIT_CMD_BASE + 0x00)
+#define IPC_GTD_PUNIT_CMD_CONFIG		(IPC_GTD_PUNIT_CMD_BASE + 0x01)
+#define IPC_GTD_PUNIT_CMD_READ_ICCP_LIC_CDYN_SCAL \
+						(IPC_GTD_PUNIT_CMD_BASE + 0x02)
+#define IPC_GTD_PUNIT_CMD_WRITE_ICCP_LIC_CDYN_SCAL \
+						(IPC_GTD_PUNIT_CMD_BASE + 0x03)
+#define IPC_GTD_PUNIT_CMD_GET_WM_VAL		(IPC_GTD_PUNIT_CMD_BASE + 0x06)
+#define IPC_GTD_PUNIT_CMD_WRITE_CONFIG_WISHREQ	(IPC_GTD_PUNIT_CMD_BASE + 0x07)
+#define IPC_GTD_PUNIT_CMD_READ_REQ_DUTY_CYCLE	(IPC_GTD_PUNIT_CMD_BASE + 0x16)
+#define IPC_GTD_PUNIT_CMD_DIS_VOL_FREQ_CHANGE_REQUEST \
+						(IPC_GTD_PUNIT_CMD_BASE + 0x17)
+#define IPC_GTD_PUNIT_CMD_DYNA_DUTY_CYCLE_CTRL	(IPC_GTD_PUNIT_CMD_BASE + 0x1a)
+#define IPC_GTD_PUNIT_CMD_DYNA_DUTY_CYCLE_TUNING \
+						(IPC_GTD_PUNIT_CMD_BASE + 0x1c)
+
+/* ISP Driver => Pcode commands */
+#define IPC_ISPD_PUNIT_CMD_ZERO			(IPC_ISPD_PUNIT_CMD_BASE + 0x00)
+#define IPC_ISPD_PUNIT_CMD_CONFIG		(IPC_ISPD_PUNIT_CMD_BASE + 0x01)
+#define IPC_ISPD_PUNIT_CMD_GET_ISP_LTR_VAL	(IPC_ISPD_PUNIT_CMD_BASE + 0x02)
+#define IPC_ISPD_PUNIT_CMD_ACCESS_IU_FREQ_BOUNDS \
+						(IPC_ISPD_PUNIT_CMD_BASE + 0x03)
+#define IPC_ISPD_PUNIT_CMD_READ_CDYN_LEVEL	(IPC_ISPD_PUNIT_CMD_BASE + 0x04)
+#define IPC_ISPD_PUNIT_CMD_WRITE_CDYN_LEVEL	(IPC_ISPD_PUNIT_CMD_BASE + 0x05)
+#define IPC_ISPD_PUNIT_CMD_MAX			(IPC_ISPD_PUNIT_CMD_BASE + 0x06)
+
+/* Error codes */
+#define IPC_ERR_SUCCESS				0
+#define IPC_ERR_INVALID_CMD			1
+#define IPC_ERR_INVALID_PARAMETER		2
+#define IPC_ERR_CMD_TIMEOUT			3
+#define IPC_ERR_CMD_LOCKED			4
+#define IPC_ERR_INVALID_VR_ID			5
+#define IPC_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 intline int intel_punit_ipc_simple_command(int cmd,
+						  int para1, int para2);
+{
+	return -EINVAL;
+}
+
+static inline int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2,
+					  u32 *in, u32 *out);
+{
+	return -EINVAL;
+}
+
+#endif /*CONFIG_INTEL_PUNIT_IPC*/
+
+#endif
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 346f1fd..42ada9b 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 PUNIT IPC Driver"
+	default y
+	---help---
+	  IPC is used to bridge the communications between kernel and PUNIT
+
 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..78cb794
--- /dev/null
+++ b/drivers/platform/x86/intel_punit_ipc.c
@@ -0,0 +1,388 @@
+/*
+ * intel_punit_ipc.c: Driver for the Intel Punit 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 Punit is the Foxton microcontroller and its firmware,
+ * which provide mailbox interface for power management usage.
+ */
+
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/pm.h>
+#include <linux/acpi.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/bitops.h>
+#include <linux/atomic.h>
+#include <linux/notifier.h>
+#include <linux/suspend.h>
+#include <linux/platform_device.h>
+#include <asm/intel_punit_ipc.h>
+
+/* Mailbox registers */
+#define MAILBOX_DATA_LOW		0x0
+#define MAILBOX_INTERFACE		0x4
+#define		CMD_RUN			(1 << 31)
+#define		CMD_ERRCODE_MASK	0xFF
+#define		CMD_PARA1_SHIFT		8
+#define		CMD_PARA2_SHIFT		16
+#define		CMD_MASK		0xFF
+#define MAILBOX_DATA_HIGH		0x8
+
+#define MAILBOX_REGISTER_SPACE		0x10
+
+#define CMD_TIMEOUT_SECONDS		3
+
+/* Three external mailbox */
+enum mailbox_type {
+	BIOS_MAILBOX,
+	GTDRIVER_MAILBOX,
+	ISPDRIVER_MAILBOX,
+	RESERVED_MAILBOX,
+};
+
+struct intel_punit_ipc_controller {
+	struct platform_device *pdev;
+	struct mutex lock;
+	void __iomem *base[RESERVED_MAILBOX];
+	struct completion cmd_complete;
+	int irq;
+
+	int cmd;
+	enum mailbox_type type;
+};
+
+static char *ipc_err_sources[] = {
+	[IPC_ERR_SUCCESS] =
+		"no error",
+	[IPC_ERR_INVALID_CMD] =
+		"invalid command",
+	[IPC_ERR_INVALID_PARAMETER] =
+		"invalid parameter",
+	[IPC_ERR_CMD_TIMEOUT] =
+		"command timeout",
+	[IPC_ERR_CMD_LOCKED] =
+		"command locked",
+	[IPC_ERR_INVALID_VR_ID] =
+		"invalid vr id",
+	[IPC_ERR_VR_ERR] =
+		"vr error",
+};
+
+static struct intel_punit_ipc_controller ipcdev;
+
+static inline u32 ipc_read_status(void)
+{
+	return readl(ipcdev.base[ipcdev.type] + MAILBOX_INTERFACE);
+}
+
+static inline void ipc_write_cmd(u32 cmd)
+{
+	writel(cmd, ipcdev.base[ipcdev.type] + MAILBOX_INTERFACE);
+}
+
+static inline u32 ipc_read_data_low(void)
+{
+	return readl(ipcdev.base[ipcdev.type] + MAILBOX_DATA_LOW);
+}
+
+static inline u32 ipc_read_data_high(void)
+{
+	return readl(ipcdev.base[ipcdev.type] + MAILBOX_DATA_HIGH);
+}
+
+static inline void ipc_write_data_low(u32 data)
+{
+	writel(data, ipcdev.base[ipcdev.type] + MAILBOX_DATA_LOW);
+}
+
+static inline void ipc_write_data_high(u32 data)
+{
+	writel(data, ipcdev.base[ipcdev.type] + MAILBOX_DATA_HIGH);
+}
+
+static int intel_punit_init_cmd(u32 cmd)
+{
+	cmd &= CMD_MASK;
+	if (cmd < IPC_BIOS_PUNIT_CMD_BASE)
+		return -EINVAL;
+	else if (cmd < IPC_GTD_PUNIT_CMD_BASE)
+		ipcdev.type = BIOS_MAILBOX;
+	else if (cmd < IPC_ISPD_PUNIT_CMD_BASE)
+		ipcdev.type = GTDRIVER_MAILBOX;
+	else if (cmd < IPC_ISPD_PUNIT_CMD_MAX)
+		ipcdev.type = ISPDRIVER_MAILBOX;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+
+static int intel_punit_ipc_send_command(u32 cmd)
+{
+	int ret;
+
+	ret = intel_punit_init_cmd(cmd);
+	if (ret)
+		return ret;
+	ipcdev.cmd = cmd;
+	reinit_completion(&ipcdev.cmd_complete);
+	ipc_write_cmd(cmd);
+	return 0;
+}
+
+static int intel_punit_ipc_check_status(void)
+{
+	int loops = CMD_TIMEOUT_SECONDS * USEC_PER_SEC;
+	int ret = 0;
+	int status;
+	int errcode;
+
+	if (ipcdev.irq) {
+		if (0 == wait_for_completion_timeout(
+				&ipcdev.cmd_complete,
+				CMD_TIMEOUT_SECONDS * HZ)) {
+			dev_err(&ipcdev.pdev->dev,
+				"IPC timed out, IPC_CMD=0x%x\n", ipcdev.cmd);
+			return -ETIMEDOUT;
+		}
+	} else {
+		while ((ipc_read_status() & CMD_RUN) && --loops)
+			udelay(1);
+		if (!loops) {
+			dev_err(&ipcdev.pdev->dev,
+				"IPC timed out, IPC_CMD=0x%x\n", ipcdev.cmd);
+			return -ETIMEDOUT;
+		}
+	}
+
+	status = ipc_read_status();
+	errcode = status & CMD_ERRCODE_MASK;
+	if (errcode) {
+		ret = -EIO;
+		if (errcode < ARRAY_SIZE(ipc_err_sources))
+			dev_err(&ipcdev.pdev->dev,
+				"IPC failed: %s, IPC_STS=0x%x, IPC_CMD=0x%x\n",
+				ipc_err_sources[errcode], status, ipcdev.cmd);
+		else
+			dev_err(&ipcdev.pdev->dev,
+				"IPC failed: unknown err,STS=0x%x, CMD=0x%x\n",
+				status, ipcdev.cmd);
+	}
+
+	return ret;
+}
+
+/**
+ * intel_punit_ipc_simple_command() - Simple IPC command
+ * @cmd:	IPC command code.
+ * @para1:	First 8bit parameter, set 0 if invalid.
+ * @para2:	Second 8bit parameter, set 0 if invalid.
+ *
+ * Send a IPC command to Punit when there is no data transaction
+ *
+ * Return:	an IPC error code or 0 on success.
+ */
+int intel_punit_ipc_simple_command(int cmd, int para1, int para2)
+{
+	int ret;
+
+	mutex_lock(&ipcdev.lock);
+	ret = intel_punit_ipc_send_command(CMD_RUN |
+					   para2 << CMD_PARA2_SHIFT |
+					   para1 << CMD_PARA1_SHIFT | cmd);
+	if (ret)
+		goto out;
+	ret = intel_punit_ipc_check_status();
+out:
+	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 invalid.
+ * @para2:	Second 8bit parameter, set 0 if invalid.
+ * @in:		Input data, 32bit for BIOS cmd, two 32bit for GTD and ISPD.
+ * @out:	Output data.
+ *
+ * Send a IPC command to Punit with data transaction
+ *
+ * Return:	an IPC error code or 0 on success.
+ */
+int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2, u32 *in, u32 *out)
+{
+	int ret;
+
+	mutex_lock(&ipcdev.lock);
+	ipc_write_data_low(*in);
+	if (ipcdev.type == GTDRIVER_MAILBOX ||
+			ipcdev.type == ISPDRIVER_MAILBOX) {
+		in++;
+		ipc_write_data_high(*in);
+	}
+	ret = intel_punit_ipc_send_command(CMD_RUN |
+					   para2 << CMD_PARA2_SHIFT |
+					   para1 << CMD_PARA1_SHIFT | cmd);
+	if (ret)
+		goto out;
+	ret = intel_punit_ipc_check_status();
+	*out = ipc_read_data_low();
+	if (ipcdev.type == GTDRIVER_MAILBOX ||
+			ipcdev.type == ISPDRIVER_MAILBOX) {
+		out++;
+		*out = ipc_read_data_high();
+	}
+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)
+{
+	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;
+	int ret;
+
+	res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res0) {
+		dev_err(&pdev->dev, "Failed to get iomem resource\n");
+		return -EINVAL;
+	}
+	size = resource_size(res0);
+	if (!request_mem_region(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 -EINVAL;
+	}
+	size = resource_size(res1);
+	if (!request_mem_region(res1->start, size, pdev->name)) {
+		dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
+		ret = -EBUSY;
+		goto err_res1;
+	}
+
+	addr = ioremap_nocache(res0->start,
+			       resource_size(res0) + resource_size(res1));
+	if (!addr) {
+		dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
+		ret = -ENOMEM;
+		goto err_map;
+	}
+	ipcdev.base[BIOS_MAILBOX] = addr;
+	addr += MAILBOX_REGISTER_SPACE;
+	ipcdev.base[GTDRIVER_MAILBOX] = addr;
+	addr += MAILBOX_REGISTER_SPACE;
+	ipcdev.base[ISPDRIVER_MAILBOX] = addr;
+
+	return 0;
+
+err_map:
+	release_mem_region(res1->start, resource_size(res1));
+err_res1:
+	release_mem_region(res0->start, resource_size(res0));
+	return ret;
+}
+
+static int intel_punit_ipc_probe(struct platform_device *pdev)
+{
+	int irq;
+	int ret;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		ipcdev.irq = 0;
+		dev_warn(&pdev->dev, "Could not get irq number\n");
+	} else {
+		if (request_irq(irq, intel_punit_ioc, IRQF_NO_SUSPEND,
+				"intel_punit_ipc", &ipcdev)) {
+			dev_err(&pdev->dev, "request irq %d\n", irq);
+			return -EBUSY;
+		}
+		ipcdev.irq = irq;
+	}
+
+	ret = intel_punit_get_bars(pdev);
+	if (ret) {
+		if (ipcdev.irq)
+			free_irq(ipcdev.irq, &ipcdev);
+		return ret;
+	}
+
+	ipcdev.pdev = pdev;
+	mutex_init(&ipcdev.lock);
+	init_completion(&ipcdev.cmd_complete);
+	return ret;
+}
+
+static int intel_punit_ipc_remove(struct platform_device *pdev)
+{
+	struct resource *res;
+
+	if (ipcdev.irq)
+		free_irq(ipcdev.irq, &ipcdev);
+	iounmap(ipcdev.base[BIOS_MAILBOX]);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res)
+		release_mem_region(res->start, resource_size(res));
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res)
+		release_mem_region(res->start, resource_size(res));
+	return 0;
+}
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id punit_ipc_acpi_ids[] = {
+	{"INT34D4", 0}
+};
+#endif
+
+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 Punit IPC driver");
+MODULE_LICENSE("GPL");
+
+/* 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

Re: [PATCH] platform:x86: add Intel Punit mailbox IPC driver

[Darren Hart]

On Fri, Jul 31, 2015 at 11:58:56PM +0800, Qipeng Zha wrote:
> This driver provides support for Punit mailbox IPC on Intel platforms.
> The heart of the Punit is the Foxton microcontroller and its firmware,
> which provide mailbox interface for power management usage.
> 

Mika, Andriy:

I'm traveling over the next few days and could use some help ensuring this new
driver receives some additional review. If you can, I'd appreciate your
thoughts.

> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> ---
>  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 | 388 +++++++++++++++++++++++++++++++++
>  4 files changed, 496 insertions(+)
>  create mode 100644 arch/x86/include/asm/intel_punit_ipc.h
>  create mode 100644 drivers/platform/x86/intel_punit_ipc.c
> 
> 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..08e3e14
> --- /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_
> +
> +/* Commands supported on GLM core, are handled by different bars,
> + * unify these commands together here
> + */
> +#define IPC_BIOS_PUNIT_CMD_BASE				0x00
> +#define IPC_GTD_PUNIT_CMD_BASE				0x20
> +#define IPC_ISPD_PUNIT_CMD_BASE				0x40
> +
> +/* BIOS => Pcode commands */
> +#define IPC_BIOS_PUNIT_CMD_ZERO			(IPC_BIOS_PUNIT_CMD_BASE + 0x00)
> +#define IPC_BIOS_PUNIT_CMD_VR_INTERFACE		(IPC_BIOS_PUNIT_CMD_BASE + 0x01)
> +#define IPC_BIOS_PUNIT_CMD_READ_PCS		(IPC_BIOS_PUNIT_CMD_BASE + 0x02)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_PCS		(IPC_BIOS_PUNIT_CMD_BASE + 0x03)
> +#define IPC_BIOS_PUNIT_CMD_READ_PCU_CONFIG	(IPC_BIOS_PUNIT_CMD_BASE + 0x04)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_PCU_CONFIG	(IPC_BIOS_PUNIT_CMD_BASE + 0x05)
> +#define IPC_BIOS_PUNIT_CMD_READ_PL1_SETTING	(IPC_BIOS_PUNIT_CMD_BASE + 0x06)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_PL1_SETTING	(IPC_BIOS_PUNIT_CMD_BASE + 0x07)
> +#define IPC_BIOS_PUNIT_CMD_TRIGGER_VDD_RAM	(IPC_BIOS_PUNIT_CMD_BASE + 0x08)
> +#define IPC_BIOS_PUNIT_CMD_READ_TELE_INFO	(IPC_BIOS_PUNIT_CMD_BASE + 0x09)
> +#define IPC_BIOS_PUNIT_CMD_READ_TELE_TRACE_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x0a)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_TRACE_CTRL \
> +						(IPC_BIOS_PUNIT_CMD_BASE + 0x0b)
> +#define IPC_BIOS_PUNIT_CMD_READ_TELE_EVENT_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x0c)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_EVENT_CTRL \
> +						(IPC_BIOS_PUNIT_CMD_BASE + 0x0d)
> +#define IPC_BIOS_PUNIT_CMD_READ_TELE_TRACE	(IPC_BIOS_PUNIT_CMD_BASE + 0x0e)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_TRACE	(IPC_BIOS_PUNIT_CMD_BASE + 0x0f)
> +#define IPC_BIOS_PUNIT_CMD_READ_TELE_EVENT	(IPC_BIOS_PUNIT_CMD_BASE + 0x10)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_EVENT	(IPC_BIOS_PUNIT_CMD_BASE + 0x11)
> +#define IPC_BIOS_PUNIT_CMD_READ_MODULE_TEMP	(IPC_BIOS_PUNIT_CMD_BASE + 0x12)
> +#define IPC_BIOS_PUNIT_CMD_RESERVED		(IPC_BIOS_PUNIT_CMD_BASE + 0x13)
> +#define IPC_BIOS_PUNIT_CMD_READ_VOLTAGE_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x14)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_VOLTAGE_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x15)
> +#define IPC_BIOS_PUNIT_CMD_READ_RATIO_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x16)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_RATIO_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x17)
> +#define IPC_BIOS_PUNIT_CMD_READ_VF_GL_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x18)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_VF_GL_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x19)
> +#define IPC_BIOS_PUNIT_CMD_READ_FM_SOC_TEMP_THRESH \
> +						(IPC_BIOS_PUNIT_CMD_BASE + 0x1a)
> +#define IPC_BIOS_PUNIT_CMD_WRITE_FM_SOC_TEMP_THRESH \
> +						(IPC_BIOS_PUNIT_CMD_BASE + 0x1b)
> +
> +/*GT Driver => Pcode commands*/
> +#define IPC_GTD_PUNIT_CMD_ZERO			(IPC_GTD_PUNIT_CMD_BASE + 0x00)
> +#define IPC_GTD_PUNIT_CMD_CONFIG		(IPC_GTD_PUNIT_CMD_BASE + 0x01)
> +#define IPC_GTD_PUNIT_CMD_READ_ICCP_LIC_CDYN_SCAL \
> +						(IPC_GTD_PUNIT_CMD_BASE + 0x02)
> +#define IPC_GTD_PUNIT_CMD_WRITE_ICCP_LIC_CDYN_SCAL \
> +						(IPC_GTD_PUNIT_CMD_BASE + 0x03)
> +#define IPC_GTD_PUNIT_CMD_GET_WM_VAL		(IPC_GTD_PUNIT_CMD_BASE + 0x06)
> +#define IPC_GTD_PUNIT_CMD_WRITE_CONFIG_WISHREQ	(IPC_GTD_PUNIT_CMD_BASE + 0x07)
> +#define IPC_GTD_PUNIT_CMD_READ_REQ_DUTY_CYCLE	(IPC_GTD_PUNIT_CMD_BASE + 0x16)
> +#define IPC_GTD_PUNIT_CMD_DIS_VOL_FREQ_CHANGE_REQUEST \
> +						(IPC_GTD_PUNIT_CMD_BASE + 0x17)
> +#define IPC_GTD_PUNIT_CMD_DYNA_DUTY_CYCLE_CTRL	(IPC_GTD_PUNIT_CMD_BASE + 0x1a)
> +#define IPC_GTD_PUNIT_CMD_DYNA_DUTY_CYCLE_TUNING \
> +						(IPC_GTD_PUNIT_CMD_BASE + 0x1c)
> +
> +/* ISP Driver => Pcode commands */
> +#define IPC_ISPD_PUNIT_CMD_ZERO			(IPC_ISPD_PUNIT_CMD_BASE + 0x00)
> +#define IPC_ISPD_PUNIT_CMD_CONFIG		(IPC_ISPD_PUNIT_CMD_BASE + 0x01)
> +#define IPC_ISPD_PUNIT_CMD_GET_ISP_LTR_VAL	(IPC_ISPD_PUNIT_CMD_BASE + 0x02)
> +#define IPC_ISPD_PUNIT_CMD_ACCESS_IU_FREQ_BOUNDS \
> +						(IPC_ISPD_PUNIT_CMD_BASE + 0x03)
> +#define IPC_ISPD_PUNIT_CMD_READ_CDYN_LEVEL	(IPC_ISPD_PUNIT_CMD_BASE + 0x04)
> +#define IPC_ISPD_PUNIT_CMD_WRITE_CDYN_LEVEL	(IPC_ISPD_PUNIT_CMD_BASE + 0x05)
> +#define IPC_ISPD_PUNIT_CMD_MAX			(IPC_ISPD_PUNIT_CMD_BASE + 0x06)
> +
> +/* Error codes */
> +#define IPC_ERR_SUCCESS				0
> +#define IPC_ERR_INVALID_CMD			1
> +#define IPC_ERR_INVALID_PARAMETER		2
> +#define IPC_ERR_CMD_TIMEOUT			3
> +#define IPC_ERR_CMD_LOCKED			4
> +#define IPC_ERR_INVALID_VR_ID			5
> +#define IPC_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 intline int intel_punit_ipc_simple_command(int cmd,
> +						  int para1, int para2);
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2,
> +					  u32 *in, u32 *out);
> +{
> +	return -EINVAL;
> +}
> +
> +#endif /*CONFIG_INTEL_PUNIT_IPC*/
> +
> +#endif
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 346f1fd..42ada9b 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 PUNIT IPC Driver"
> +	default y
> +	---help---
> +	  IPC is used to bridge the communications between kernel and PUNIT
> +
>  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..78cb794
> --- /dev/null
> +++ b/drivers/platform/x86/intel_punit_ipc.c
> @@ -0,0 +1,388 @@
> +/*
> + * intel_punit_ipc.c: Driver for the Intel Punit 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 Punit is the Foxton microcontroller and its firmware,
> + * which provide mailbox interface for power management usage.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/pm.h>
> +#include <linux/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/bitops.h>
> +#include <linux/atomic.h>
> +#include <linux/notifier.h>
> +#include <linux/suspend.h>
> +#include <linux/platform_device.h>
> +#include <asm/intel_punit_ipc.h>
> +
> +/* Mailbox registers */
> +#define MAILBOX_DATA_LOW		0x0
> +#define MAILBOX_INTERFACE		0x4
> +#define		CMD_RUN			(1 << 31)
> +#define		CMD_ERRCODE_MASK	0xFF
> +#define		CMD_PARA1_SHIFT		8
> +#define		CMD_PARA2_SHIFT		16
> +#define		CMD_MASK		0xFF
> +#define MAILBOX_DATA_HIGH		0x8
> +
> +#define MAILBOX_REGISTER_SPACE		0x10
> +
> +#define CMD_TIMEOUT_SECONDS		3
> +
> +/* Three external mailbox */
> +enum mailbox_type {
> +	BIOS_MAILBOX,
> +	GTDRIVER_MAILBOX,
> +	ISPDRIVER_MAILBOX,
> +	RESERVED_MAILBOX,
> +};
> +
> +struct intel_punit_ipc_controller {
> +	struct platform_device *pdev;
> +	struct mutex lock;
> +	void __iomem *base[RESERVED_MAILBOX];
> +	struct completion cmd_complete;
> +	int irq;
> +
> +	int cmd;
> +	enum mailbox_type type;
> +};
> +
> +static char *ipc_err_sources[] = {
> +	[IPC_ERR_SUCCESS] =
> +		"no error",
> +	[IPC_ERR_INVALID_CMD] =
> +		"invalid command",
> +	[IPC_ERR_INVALID_PARAMETER] =
> +		"invalid parameter",
> +	[IPC_ERR_CMD_TIMEOUT] =
> +		"command timeout",
> +	[IPC_ERR_CMD_LOCKED] =
> +		"command locked",
> +	[IPC_ERR_INVALID_VR_ID] =
> +		"invalid vr id",
> +	[IPC_ERR_VR_ERR] =
> +		"vr error",
> +};
> +
> +static struct intel_punit_ipc_controller ipcdev;
> +
> +static inline u32 ipc_read_status(void)
> +{
> +	return readl(ipcdev.base[ipcdev.type] + MAILBOX_INTERFACE);
> +}
> +
> +static inline void ipc_write_cmd(u32 cmd)
> +{
> +	writel(cmd, ipcdev.base[ipcdev.type] + MAILBOX_INTERFACE);
> +}
> +
> +static inline u32 ipc_read_data_low(void)
> +{
> +	return readl(ipcdev.base[ipcdev.type] + MAILBOX_DATA_LOW);
> +}
> +
> +static inline u32 ipc_read_data_high(void)
> +{
> +	return readl(ipcdev.base[ipcdev.type] + MAILBOX_DATA_HIGH);
> +}
> +
> +static inline void ipc_write_data_low(u32 data)
> +{
> +	writel(data, ipcdev.base[ipcdev.type] + MAILBOX_DATA_LOW);
> +}
> +
> +static inline void ipc_write_data_high(u32 data)
> +{
> +	writel(data, ipcdev.base[ipcdev.type] + MAILBOX_DATA_HIGH);
> +}
> +
> +static int intel_punit_init_cmd(u32 cmd)
> +{
> +	cmd &= CMD_MASK;
> +	if (cmd < IPC_BIOS_PUNIT_CMD_BASE)
> +		return -EINVAL;
> +	else if (cmd < IPC_GTD_PUNIT_CMD_BASE)
> +		ipcdev.type = BIOS_MAILBOX;
> +	else if (cmd < IPC_ISPD_PUNIT_CMD_BASE)
> +		ipcdev.type = GTDRIVER_MAILBOX;
> +	else if (cmd < IPC_ISPD_PUNIT_CMD_MAX)
> +		ipcdev.type = ISPDRIVER_MAILBOX;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int intel_punit_ipc_send_command(u32 cmd)
> +{
> +	int ret;
> +
> +	ret = intel_punit_init_cmd(cmd);
> +	if (ret)
> +		return ret;
> +	ipcdev.cmd = cmd;
> +	reinit_completion(&ipcdev.cmd_complete);
> +	ipc_write_cmd(cmd);
> +	return 0;
> +}
> +
> +static int intel_punit_ipc_check_status(void)
> +{
> +	int loops = CMD_TIMEOUT_SECONDS * USEC_PER_SEC;
> +	int ret = 0;
> +	int status;
> +	int errcode;
> +
> +	if (ipcdev.irq) {
> +		if (0 == wait_for_completion_timeout(
> +				&ipcdev.cmd_complete,
> +				CMD_TIMEOUT_SECONDS * HZ)) {
> +			dev_err(&ipcdev.pdev->dev,
> +				"IPC timed out, IPC_CMD=0x%x\n", ipcdev.cmd);
> +			return -ETIMEDOUT;
> +		}
> +	} else {
> +		while ((ipc_read_status() & CMD_RUN) && --loops)
> +			udelay(1);
> +		if (!loops) {
> +			dev_err(&ipcdev.pdev->dev,
> +				"IPC timed out, IPC_CMD=0x%x\n", ipcdev.cmd);
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	status = ipc_read_status();
> +	errcode = status & CMD_ERRCODE_MASK;
> +	if (errcode) {
> +		ret = -EIO;
> +		if (errcode < ARRAY_SIZE(ipc_err_sources))
> +			dev_err(&ipcdev.pdev->dev,
> +				"IPC failed: %s, IPC_STS=0x%x, IPC_CMD=0x%x\n",
> +				ipc_err_sources[errcode], status, ipcdev.cmd);
> +		else
> +			dev_err(&ipcdev.pdev->dev,
> +				"IPC failed: unknown err,STS=0x%x, CMD=0x%x\n",
> +				status, ipcdev.cmd);
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * intel_punit_ipc_simple_command() - Simple IPC command
> + * @cmd:	IPC command code.
> + * @para1:	First 8bit parameter, set 0 if invalid.
> + * @para2:	Second 8bit parameter, set 0 if invalid.
> + *
> + * Send a IPC command to Punit when there is no data transaction
> + *
> + * Return:	an IPC error code or 0 on success.
> + */
> +int intel_punit_ipc_simple_command(int cmd, int para1, int para2)
> +{
> +	int ret;
> +
> +	mutex_lock(&ipcdev.lock);
> +	ret = intel_punit_ipc_send_command(CMD_RUN |
> +					   para2 << CMD_PARA2_SHIFT |
> +					   para1 << CMD_PARA1_SHIFT | cmd);
> +	if (ret)
> +		goto out;
> +	ret = intel_punit_ipc_check_status();
> +out:
> +	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 invalid.
> + * @para2:	Second 8bit parameter, set 0 if invalid.
> + * @in:		Input data, 32bit for BIOS cmd, two 32bit for GTD and ISPD.
> + * @out:	Output data.
> + *
> + * Send a IPC command to Punit with data transaction
> + *
> + * Return:	an IPC error code or 0 on success.
> + */
> +int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2, u32 *in, u32 *out)
> +{
> +	int ret;
> +
> +	mutex_lock(&ipcdev.lock);
> +	ipc_write_data_low(*in);
> +	if (ipcdev.type == GTDRIVER_MAILBOX ||
> +			ipcdev.type == ISPDRIVER_MAILBOX) {
> +		in++;
> +		ipc_write_data_high(*in);
> +	}
> +	ret = intel_punit_ipc_send_command(CMD_RUN |
> +					   para2 << CMD_PARA2_SHIFT |
> +					   para1 << CMD_PARA1_SHIFT | cmd);
> +	if (ret)
> +		goto out;
> +	ret = intel_punit_ipc_check_status();
> +	*out = ipc_read_data_low();
> +	if (ipcdev.type == GTDRIVER_MAILBOX ||
> +			ipcdev.type == ISPDRIVER_MAILBOX) {
> +		out++;
> +		*out = ipc_read_data_high();
> +	}
> +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)
> +{
> +	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;
> +	int ret;
> +
> +	res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res0) {
> +		dev_err(&pdev->dev, "Failed to get iomem resource\n");
> +		return -EINVAL;
> +	}
> +	size = resource_size(res0);
> +	if (!request_mem_region(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 -EINVAL;
> +	}
> +	size = resource_size(res1);
> +	if (!request_mem_region(res1->start, size, pdev->name)) {
> +		dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
> +		ret = -EBUSY;
> +		goto err_res1;
> +	}
> +
> +	addr = ioremap_nocache(res0->start,
> +			       resource_size(res0) + resource_size(res1));
> +	if (!addr) {
> +		dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
> +		ret = -ENOMEM;
> +		goto err_map;
> +	}
> +	ipcdev.base[BIOS_MAILBOX] = addr;
> +	addr += MAILBOX_REGISTER_SPACE;
> +	ipcdev.base[GTDRIVER_MAILBOX] = addr;
> +	addr += MAILBOX_REGISTER_SPACE;
> +	ipcdev.base[ISPDRIVER_MAILBOX] = addr;
> +
> +	return 0;
> +
> +err_map:
> +	release_mem_region(res1->start, resource_size(res1));
> +err_res1:
> +	release_mem_region(res0->start, resource_size(res0));
> +	return ret;
> +}
> +
> +static int intel_punit_ipc_probe(struct platform_device *pdev)
> +{
> +	int irq;
> +	int ret;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		ipcdev.irq = 0;
> +		dev_warn(&pdev->dev, "Could not get irq number\n");
> +	} else {
> +		if (request_irq(irq, intel_punit_ioc, IRQF_NO_SUSPEND,
> +				"intel_punit_ipc", &ipcdev)) {
> +			dev_err(&pdev->dev, "request irq %d\n", irq);
> +			return -EBUSY;
> +		}
> +		ipcdev.irq = irq;
> +	}
> +
> +	ret = intel_punit_get_bars(pdev);
> +	if (ret) {
> +		if (ipcdev.irq)
> +			free_irq(ipcdev.irq, &ipcdev);
> +		return ret;
> +	}
> +
> +	ipcdev.pdev = pdev;
> +	mutex_init(&ipcdev.lock);
> +	init_completion(&ipcdev.cmd_complete);
> +	return ret;
> +}
> +
> +static int intel_punit_ipc_remove(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +
> +	if (ipcdev.irq)
> +		free_irq(ipcdev.irq, &ipcdev);
> +	iounmap(ipcdev.base[BIOS_MAILBOX]);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (res)
> +		release_mem_region(res->start, resource_size(res));
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (res)
> +		release_mem_region(res->start, resource_size(res));
> +	return 0;
> +}
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id punit_ipc_acpi_ids[] = {
> +	{"INT34D4", 0}
> +};
> +#endif
> +
> +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 Punit IPC driver");
> +MODULE_LICENSE("GPL");
> +
> +/* 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
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] platform:x86: add Intel Punit mailbox IPC driver

[Mika Westerberg]

On Wed, Aug 05, 2015 at 02:25:13PM -0700, Darren Hart wrote:
> On Fri, Jul 31, 2015 at 11:58:56PM +0800, Qipeng Zha wrote:
> > This driver provides support for Punit mailbox IPC on Intel platforms.
> > The heart of the Punit is the Foxton microcontroller and its firmware,
> > which provide mailbox interface for power management usage.
> > 
> 
> Mika, Andriy:
> 
> I'm traveling over the next few days and could use some help ensuring this new
> driver receives some additional review. If you can, I'd appreciate your
> thoughts.

Sure, no problem.

> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> > ---
> >  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 | 388 +++++++++++++++++++++++++++++++++
> >  4 files changed, 496 insertions(+)
> >  create mode 100644 arch/x86/include/asm/intel_punit_ipc.h
> >  create mode 100644 drivers/platform/x86/intel_punit_ipc.c
> > 
> > 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..08e3e14
> > --- /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_
> > +
> > +/* Commands supported on GLM core, are handled by different bars,
> > + * unify these commands together here
> > + */
> > +#define IPC_BIOS_PUNIT_CMD_BASE				0x00
> > +#define IPC_GTD_PUNIT_CMD_BASE				0x20
> > +#define IPC_ISPD_PUNIT_CMD_BASE				0x40
> > +
> > +/* BIOS => Pcode commands */
> > +#define IPC_BIOS_PUNIT_CMD_ZERO			(IPC_BIOS_PUNIT_CMD_BASE + 0x00)
> > +#define IPC_BIOS_PUNIT_CMD_VR_INTERFACE		(IPC_BIOS_PUNIT_CMD_BASE + 0x01)
> > +#define IPC_BIOS_PUNIT_CMD_READ_PCS		(IPC_BIOS_PUNIT_CMD_BASE + 0x02)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_PCS		(IPC_BIOS_PUNIT_CMD_BASE + 0x03)
> > +#define IPC_BIOS_PUNIT_CMD_READ_PCU_CONFIG	(IPC_BIOS_PUNIT_CMD_BASE + 0x04)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_PCU_CONFIG	(IPC_BIOS_PUNIT_CMD_BASE + 0x05)
> > +#define IPC_BIOS_PUNIT_CMD_READ_PL1_SETTING	(IPC_BIOS_PUNIT_CMD_BASE + 0x06)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_PL1_SETTING	(IPC_BIOS_PUNIT_CMD_BASE + 0x07)
> > +#define IPC_BIOS_PUNIT_CMD_TRIGGER_VDD_RAM	(IPC_BIOS_PUNIT_CMD_BASE + 0x08)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_INFO	(IPC_BIOS_PUNIT_CMD_BASE + 0x09)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_TRACE_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x0a)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_TRACE_CTRL \
> > +						(IPC_BIOS_PUNIT_CMD_BASE + 0x0b)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_EVENT_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x0c)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_EVENT_CTRL \
> > +						(IPC_BIOS_PUNIT_CMD_BASE + 0x0d)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_TRACE	(IPC_BIOS_PUNIT_CMD_BASE + 0x0e)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_TRACE	(IPC_BIOS_PUNIT_CMD_BASE + 0x0f)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_EVENT	(IPC_BIOS_PUNIT_CMD_BASE + 0x10)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_EVENT	(IPC_BIOS_PUNIT_CMD_BASE + 0x11)
> > +#define IPC_BIOS_PUNIT_CMD_READ_MODULE_TEMP	(IPC_BIOS_PUNIT_CMD_BASE + 0x12)
> > +#define IPC_BIOS_PUNIT_CMD_RESERVED		(IPC_BIOS_PUNIT_CMD_BASE + 0x13)
> > +#define IPC_BIOS_PUNIT_CMD_READ_VOLTAGE_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x14)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_VOLTAGE_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x15)
> > +#define IPC_BIOS_PUNIT_CMD_READ_RATIO_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x16)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_RATIO_OVER	(IPC_BIOS_PUNIT_CMD_BASE + 0x17)
> > +#define IPC_BIOS_PUNIT_CMD_READ_VF_GL_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x18)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_VF_GL_CTRL	(IPC_BIOS_PUNIT_CMD_BASE + 0x19)
> > +#define IPC_BIOS_PUNIT_CMD_READ_FM_SOC_TEMP_THRESH \
> > +						(IPC_BIOS_PUNIT_CMD_BASE + 0x1a)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_FM_SOC_TEMP_THRESH \
> > +						(IPC_BIOS_PUNIT_CMD_BASE + 0x1b)
> > +
> > +/*GT Driver => Pcode commands*/
> > +#define IPC_GTD_PUNIT_CMD_ZERO			(IPC_GTD_PUNIT_CMD_BASE + 0x00)
> > +#define IPC_GTD_PUNIT_CMD_CONFIG		(IPC_GTD_PUNIT_CMD_BASE + 0x01)
> > +#define IPC_GTD_PUNIT_CMD_READ_ICCP_LIC_CDYN_SCAL \
> > +						(IPC_GTD_PUNIT_CMD_BASE + 0x02)
> > +#define IPC_GTD_PUNIT_CMD_WRITE_ICCP_LIC_CDYN_SCAL \
> > +						(IPC_GTD_PUNIT_CMD_BASE + 0x03)
> > +#define IPC_GTD_PUNIT_CMD_GET_WM_VAL		(IPC_GTD_PUNIT_CMD_BASE + 0x06)
> > +#define IPC_GTD_PUNIT_CMD_WRITE_CONFIG_WISHREQ	(IPC_GTD_PUNIT_CMD_BASE + 0x07)
> > +#define IPC_GTD_PUNIT_CMD_READ_REQ_DUTY_CYCLE	(IPC_GTD_PUNIT_CMD_BASE + 0x16)
> > +#define IPC_GTD_PUNIT_CMD_DIS_VOL_FREQ_CHANGE_REQUEST \
> > +						(IPC_GTD_PUNIT_CMD_BASE + 0x17)
> > +#define IPC_GTD_PUNIT_CMD_DYNA_DUTY_CYCLE_CTRL	(IPC_GTD_PUNIT_CMD_BASE + 0x1a)
> > +#define IPC_GTD_PUNIT_CMD_DYNA_DUTY_CYCLE_TUNING \
> > +						(IPC_GTD_PUNIT_CMD_BASE + 0x1c)
> > +
> > +/* ISP Driver => Pcode commands */
> > +#define IPC_ISPD_PUNIT_CMD_ZERO			(IPC_ISPD_PUNIT_CMD_BASE + 0x00)
> > +#define IPC_ISPD_PUNIT_CMD_CONFIG		(IPC_ISPD_PUNIT_CMD_BASE + 0x01)
> > +#define IPC_ISPD_PUNIT_CMD_GET_ISP_LTR_VAL	(IPC_ISPD_PUNIT_CMD_BASE + 0x02)
> > +#define IPC_ISPD_PUNIT_CMD_ACCESS_IU_FREQ_BOUNDS \
> > +						(IPC_ISPD_PUNIT_CMD_BASE + 0x03)
> > +#define IPC_ISPD_PUNIT_CMD_READ_CDYN_LEVEL	(IPC_ISPD_PUNIT_CMD_BASE + 0x04)
> > +#define IPC_ISPD_PUNIT_CMD_WRITE_CDYN_LEVEL	(IPC_ISPD_PUNIT_CMD_BASE + 0x05)
> > +#define IPC_ISPD_PUNIT_CMD_MAX			(IPC_ISPD_PUNIT_CMD_BASE + 0x06)
> > +
> > +/* Error codes */
> > +#define IPC_ERR_SUCCESS				0
> > +#define IPC_ERR_INVALID_CMD			1
> > +#define IPC_ERR_INVALID_PARAMETER		2
> > +#define IPC_ERR_CMD_TIMEOUT			3
> > +#define IPC_ERR_CMD_LOCKED			4
> > +#define IPC_ERR_INVALID_VR_ID			5
> > +#define IPC_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);

So is there going to be in-kernel user for this driver?

> > +
> > +#else
> > +
> > +static intline int intel_punit_ipc_simple_command(int cmd,
> > +						  int para1, int para2);
> > +{
> > +	return -EINVAL;
> > +}
> > +
> > +static inline int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2,
> > +					  u32 *in, u32 *out);
> > +{
> > +	return -EINVAL;
> > +}
> > +
> > +#endif /*CONFIG_INTEL_PUNIT_IPC*/
> > +
> > +#endif
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 346f1fd..42ada9b 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 PUNIT IPC Driver"
> > +	default y

I'm sure it does not need to be enabled by default.

> > +	---help---
> > +	  IPC is used to bridge the communications between kernel and PUNIT
> > +
> >  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..78cb794
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_punit_ipc.c
> > @@ -0,0 +1,388 @@
> > +/*
> > + * intel_punit_ipc.c: Driver for the Intel Punit Mailbox IPC mechanism

Drop the file name here.

> > + *
> > + * (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 Punit is the Foxton microcontroller and its firmware,
> > + * which provide mailbox interface for power management usage.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/device.h>
> > +#include <linux/pm.h>
> > +#include <linux/acpi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/bitops.h>
> > +#include <linux/atomic.h>
> > +#include <linux/notifier.h>
> > +#include <linux/suspend.h>
> > +#include <linux/platform_device.h>

Are you sure you need all the above headers?

> > +#include <asm/intel_punit_ipc.h>
> > +
> > +/* Mailbox registers */
> > +#define MAILBOX_DATA_LOW		0x0
> > +#define MAILBOX_INTERFACE		0x4
> > +#define		CMD_RUN			(1 << 31)
> > +#define		CMD_ERRCODE_MASK	0xFF
> > +#define		CMD_PARA1_SHIFT		8
> > +#define		CMD_PARA2_SHIFT		16
> > +#define		CMD_MASK		0xFF
> > +#define MAILBOX_DATA_HIGH		0x8
> > +
> > +#define MAILBOX_REGISTER_SPACE		0x10
> > +
> > +#define CMD_TIMEOUT_SECONDS		3
> > +
> > +/* Three external mailbox */
> > +enum mailbox_type {
> > +	BIOS_MAILBOX,
> > +	GTDRIVER_MAILBOX,
> > +	ISPDRIVER_MAILBOX,
> > +	RESERVED_MAILBOX,
> > +};
> > +
> > +struct intel_punit_ipc_controller {
> > +	struct platform_device *pdev;
> > +	struct mutex lock;
> > +	void __iomem *base[RESERVED_MAILBOX];
> > +	struct completion cmd_complete;
> > +	int irq;
> > +
> > +	int cmd;
> > +	enum mailbox_type type;
> > +};
> > +
> > +static char *ipc_err_sources[] = {
> > +	[IPC_ERR_SUCCESS] =
> > +		"no error",
> > +	[IPC_ERR_INVALID_CMD] =
> > +		"invalid command",
> > +	[IPC_ERR_INVALID_PARAMETER] =
> > +		"invalid parameter",
> > +	[IPC_ERR_CMD_TIMEOUT] =
> > +		"command timeout",
> > +	[IPC_ERR_CMD_LOCKED] =
> > +		"command locked",
> > +	[IPC_ERR_INVALID_VR_ID] =
> > +		"invalid vr id",
> > +	[IPC_ERR_VR_ERR] =
> > +		"vr error",
> > +};
> > +
> > +static struct intel_punit_ipc_controller ipcdev;

Why not to allocate the private structure at probe time?

> > +
> > +static inline u32 ipc_read_status(void)
> > +{
> > +	return readl(ipcdev.base[ipcdev.type] + MAILBOX_INTERFACE);
> > +}
> > +
> > +static inline void ipc_write_cmd(u32 cmd)
> > +{
> > +	writel(cmd, ipcdev.base[ipcdev.type] + MAILBOX_INTERFACE);
> > +}
> > +
> > +static inline u32 ipc_read_data_low(void)
> > +{
> > +	return readl(ipcdev.base[ipcdev.type] + MAILBOX_DATA_LOW);
> > +}
> > +
> > +static inline u32 ipc_read_data_high(void)
> > +{
> > +	return readl(ipcdev.base[ipcdev.type] + MAILBOX_DATA_HIGH);
> > +}
> > +
> > +static inline void ipc_write_data_low(u32 data)
> > +{
> > +	writel(data, ipcdev.base[ipcdev.type] + MAILBOX_DATA_LOW);
> > +}
> > +
> > +static inline void ipc_write_data_high(u32 data)
> > +{
> > +	writel(data, ipcdev.base[ipcdev.type] + MAILBOX_DATA_HIGH);
> > +}
> > +
> > +static int intel_punit_init_cmd(u32 cmd)
> > +{
> > +	cmd &= CMD_MASK;
> > +	if (cmd < IPC_BIOS_PUNIT_CMD_BASE)
> > +		return -EINVAL;
> > +	else if (cmd < IPC_GTD_PUNIT_CMD_BASE)
> > +		ipcdev.type = BIOS_MAILBOX;
> > +	else if (cmd < IPC_ISPD_PUNIT_CMD_BASE)
> > +		ipcdev.type = GTDRIVER_MAILBOX;
> > +	else if (cmd < IPC_ISPD_PUNIT_CMD_MAX)
> > +		ipcdev.type = ISPDRIVER_MAILBOX;
> > +	else
> > +		return -EINVAL;
> > +
> > +	return 0;

	return -EINVAL and drop else


> > +}
> > +
> > +static int intel_punit_ipc_send_command(u32 cmd)
> > +{
> > +	int ret;
> > +
> > +	ret = intel_punit_init_cmd(cmd);
> > +	if (ret)
> > +		return ret;
> > +	ipcdev.cmd = cmd;
> > +	reinit_completion(&ipcdev.cmd_complete);
> > +	ipc_write_cmd(cmd);
> > +	return 0;
> > +}
> > +
> > +static int intel_punit_ipc_check_status(void)
> > +{
> > +	int loops = CMD_TIMEOUT_SECONDS * USEC_PER_SEC;
> > +	int ret = 0;
> > +	int status;
> > +	int errcode;
> > +
> > +	if (ipcdev.irq) {
> > +		if (0 == wait_for_completion_timeout(

		if (!wait_for_completion_timeout(

> > +				&ipcdev.cmd_complete,
> > +				CMD_TIMEOUT_SECONDS * HZ)) {
> > +			dev_err(&ipcdev.pdev->dev,
> > +				"IPC timed out, IPC_CMD=0x%x\n", ipcdev.cmd);
> > +			return -ETIMEDOUT;
> > +		}
> > +	} else {
> > +		while ((ipc_read_status() & CMD_RUN) && --loops)
> > +			udelay(1);
> > +		if (!loops) {
> > +			dev_err(&ipcdev.pdev->dev,
> > +				"IPC timed out, IPC_CMD=0x%x\n", ipcdev.cmd);
> > +			return -ETIMEDOUT;
> > +		}
> > +	}
> > +
> > +	status = ipc_read_status();
> > +	errcode = status & CMD_ERRCODE_MASK;
> > +	if (errcode) {
> > +		ret = -EIO;
> > +		if (errcode < ARRAY_SIZE(ipc_err_sources))
> > +			dev_err(&ipcdev.pdev->dev,
> > +				"IPC failed: %s, IPC_STS=0x%x, IPC_CMD=0x%x\n",
> > +				ipc_err_sources[errcode], status, ipcdev.cmd);
> > +		else
> > +			dev_err(&ipcdev.pdev->dev,
> > +				"IPC failed: unknown err,STS=0x%x, CMD=0x%x\n",
> > +				status, ipcdev.cmd);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * intel_punit_ipc_simple_command() - Simple IPC command
> > + * @cmd:	IPC command code.
> > + * @para1:	First 8bit parameter, set 0 if invalid.
> > + * @para2:	Second 8bit parameter, set 0 if invalid.
> > + *
> > + * Send a IPC command to Punit when there is no data transaction
> > + *
> > + * Return:	an IPC error code or 0 on success.
> > + */
> > +int intel_punit_ipc_simple_command(int cmd, int para1, int para2)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&ipcdev.lock);
> > +	ret = intel_punit_ipc_send_command(CMD_RUN |
> > +					   para2 << CMD_PARA2_SHIFT |
> > +					   para1 << CMD_PARA1_SHIFT | cmd);
> > +	if (ret)
> > +		goto out;
> > +	ret = intel_punit_ipc_check_status();
> > +out:
> > +	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 invalid.
> > + * @para2:	Second 8bit parameter, set 0 if invalid.
> > + * @in:		Input data, 32bit for BIOS cmd, two 32bit for GTD and ISPD.
> > + * @out:	Output data.
> > + *
> > + * Send a IPC command to Punit with data transaction
> > + *
> > + * Return:	an IPC error code or 0 on success.
> > + */
> > +int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2, u32 *in, u32 *out)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&ipcdev.lock);
> > +	ipc_write_data_low(*in);
> > +	if (ipcdev.type == GTDRIVER_MAILBOX ||
> > +			ipcdev.type == ISPDRIVER_MAILBOX) {
> > +		in++;
> > +		ipc_write_data_high(*in);
> > +	}
> > +	ret = intel_punit_ipc_send_command(CMD_RUN |
> > +					   para2 << CMD_PARA2_SHIFT |
> > +					   para1 << CMD_PARA1_SHIFT | cmd);
> > +	if (ret)
> > +		goto out;
> > +	ret = intel_punit_ipc_check_status();
> > +	*out = ipc_read_data_low();
> > +	if (ipcdev.type == GTDRIVER_MAILBOX ||
> > +			ipcdev.type == ISPDRIVER_MAILBOX) {
> > +		out++;
> > +		*out = ipc_read_data_high();
> > +	}
> > +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)
> > +{
> > +	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;
> > +	int ret;
> > +
> > +	res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res0) {
> > +		dev_err(&pdev->dev, "Failed to get iomem resource\n");
> > +		return -EINVAL;
> > +	}
> > +	size = resource_size(res0);
> > +	if (!request_mem_region(res0->start, size, pdev->name)) {
> > +		dev_err(&pdev->dev, "Failed to request iomem resouce\n");
> > +		return -EBUSY;
> > +	}

Why not use devm_ here, like:

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	addr = devm_ioremap_resource(&pdev->dev, res);
	if (IS_ERR(addr))
		return PTR_ERR(addr);

> > +
> > +	res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	if (!res1) {
> > +		dev_err(&pdev->dev, "Failed to get iomem resource1\n");
> > +		return -EINVAL;
> > +	}
> > +	size = resource_size(res1);
> > +	if (!request_mem_region(res1->start, size, pdev->name)) {
> > +		dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
> > +		ret = -EBUSY;
> > +		goto err_res1;
> > +	}
> > +
> > +	addr = ioremap_nocache(res0->start,
> > +			       resource_size(res0) + resource_size(res1));
> > +	if (!addr) {
> > +		dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
> > +		ret = -ENOMEM;
> > +		goto err_map;
> > +	}
> > +	ipcdev.base[BIOS_MAILBOX] = addr;
> > +	addr += MAILBOX_REGISTER_SPACE;
> > +	ipcdev.base[GTDRIVER_MAILBOX] = addr;
> > +	addr += MAILBOX_REGISTER_SPACE;
> > +	ipcdev.base[ISPDRIVER_MAILBOX] = addr;
> > +
> > +	return 0;
> > +
> > +err_map:
> > +	release_mem_region(res1->start, resource_size(res1));
> > +err_res1:
> > +	release_mem_region(res0->start, resource_size(res0));
> > +	return ret;
> > +}
> > +
> > +static int intel_punit_ipc_probe(struct platform_device *pdev)
> > +{
> > +	int irq;
> > +	int ret;

Looks better if you do

	int irq, ret;

> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		ipcdev.irq = 0;
> > +		dev_warn(&pdev->dev, "Could not get irq number\n");
> > +	} else {
> > +		if (request_irq(irq, intel_punit_ioc, IRQF_NO_SUSPEND,
> > +				"intel_punit_ipc", &ipcdev)) {
> > +			dev_err(&pdev->dev, "request irq %d\n", irq);
> > +			return -EBUSY;
> > +		}
> > +		ipcdev.irq = irq;
> > +	}

devm_request_irq()?

> > +
> > +	ret = intel_punit_get_bars(pdev);
> > +	if (ret) {
> > +		if (ipcdev.irq)
> > +			free_irq(ipcdev.irq, &ipcdev);

Then you don't need all this...

> > +		return ret;
> > +	}
> > +
> > +	ipcdev.pdev = pdev;
> > +	mutex_init(&ipcdev.lock);
> > +	init_completion(&ipcdev.cmd_complete);
> > +	return ret;
> > +}
> > +
> > +static int intel_punit_ipc_remove(struct platform_device *pdev)
> > +{
> > +	struct resource *res;
> > +
> > +	if (ipcdev.irq)
> > +		free_irq(ipcdev.irq, &ipcdev);
> > +	iounmap(ipcdev.base[BIOS_MAILBOX]);
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (res)
> > +		release_mem_region(res->start, resource_size(res));
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	if (res)
> > +		release_mem_region(res->start, resource_size(res));

...nor this.

> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id punit_ipc_acpi_ids[] = {
> > +	{"INT34D4", 0}
> > +};
> > +#endif
> > +
> > +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 Punit IPC driver");
> > +MODULE_LICENSE("GPL");

GPLv2

> > +
> > +/* 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
> > 
> > 
> 
> -- 
> Darren Hart
> Intel Open Source Technology Center

Re: [PATCH] platform:x86: add Intel Punit mailbox IPC driver

[Shevchenko, Andriy]

[Cc: Matt to discuss iTCO thing found in one of the drivers]

On Wed, 2015-08-05 at 14:25 -0700, Darren Hart wrote:
> On Fri, Jul 31, 2015 at 11:58:56PM +0800, Qipeng Zha wrote:
> > This driver provides support for Punit mailbox IPC on Intel 
> > platforms.
> > The heart of the Punit is the Foxton microcontroller and its 
> > firmware,
> > which provide mailbox interface for power management usage.
> > 
> 
> Mika, Andriy:
> 
> I'm traveling over the next few days and could use some help ensuring 
> this new
> driver receives some additional review. If you can, I'd appreciate 
> your
> thoughts.

Yes, will do right now.


First of all we have many things here and there related to power
control devices on various Intel hardware.

arch/x86/platform/atom/pmc_atom.c
arch/x86/platform/atom/punit_atom_debug.c
drivers/mfd/intel_soc_pmic_core.c
drivers/platform/x86/intel_pmc_ipc.c
drivers/platform/x86/intel_pmic_gpio.c

So, what I see is the menagerie of drivers spread in the sources tree.

And on top of that we have IOSF mailbox implementation.

So, the main questions are:
 - Does any of above correlate with what you are doing in this driver?
 - Is PUnit accessible via IOSF mailbox?
 - Would it be generic driver for many Intel platforms or only for few?

Also the name of the module is intel_punit_ipc which is somehow used in
intel_pmc_ipc.c.

Matt, by the way intel_pmc_ipc.c module creates iTCO device (LPC bus).
Does it look correct?

> 
> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> > ---
> >  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 | 388 
> > +++++++++++++++++++++++++++++++++
> >  4 files changed, 496 insertions(+)
> >  create mode 100644 arch/x86/include/asm/intel_punit_ipc.h
> >  create mode 100644 drivers/platform/x86/intel_punit_ipc.c
> > 
> > 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..08e3e14
> > --- /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_
> > +
> > +/* Commands supported on GLM core, are handled by different bars,
> > + * unify these commands together here
> > + */
> > +#define IPC_BIOS_PUNIT_CMD_BASE				0x
> > 00
> > +#define IPC_GTD_PUNIT_CMD_BASE				0x20
> > +#define IPC_ISPD_PUNIT_CMD_BASE				0x
> > 40

What about using prefix IPC_PUNIT_ everywhere in this driver?

> > +
> > +/* BIOS => Pcode commands */
> > +#define IPC_BIOS_PUNIT_CMD_ZERO			(IPC_BIOS_
> > PUNIT_CMD_BASE + 0x00)

Can we just provide a plain number per each command?

Moreover, do we need all commands to be present in the first place?
Any users for them all?

> > +#define IPC_BIOS_PUNIT_CMD_VR_INTERFACE		(IPC_BIOS_
> > PUNIT_CMD_BASE + 0x01)
> > +#define IPC_BIOS_PUNIT_CMD_READ_PCS		(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x02)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_PCS		(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x03)
> > +#define IPC_BIOS_PUNIT_CMD_READ_PCU_CONFIG	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x04)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_PCU_CONFIG	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x05)
> > +#define IPC_BIOS_PUNIT_CMD_READ_PL1_SETTING	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x06)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_PL1_SETTING	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x07)
> > +#define IPC_BIOS_PUNIT_CMD_TRIGGER_VDD_RAM	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x08)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_INFO	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x09)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_TRACE_CTRL	(IPC_BIOS_
> > PUNIT_CMD_BASE + 0x0a)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_TRACE_CTRL \
> > +						(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x0b)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_EVENT_CTRL	(IPC_BIOS_
> > PUNIT_CMD_BASE + 0x0c)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_EVENT_CTRL \
> > +						(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x0d)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_TRACE	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x0e)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_TRACE	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x0f)
> > +#define IPC_BIOS_PUNIT_CMD_READ_TELE_EVENT	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x10)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_TELE_EVENT	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x11)
> > +#define IPC_BIOS_PUNIT_CMD_READ_MODULE_TEMP	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x12)
> > +#define IPC_BIOS_PUNIT_CMD_RESERVED		(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x13)
> > +#define IPC_BIOS_PUNIT_CMD_READ_VOLTAGE_OVER	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x14)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_VOLTAGE_OVER	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x15)
> > +#define IPC_BIOS_PUNIT_CMD_READ_RATIO_OVER	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x16)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_RATIO_OVER	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x17)
> > +#define IPC_BIOS_PUNIT_CMD_READ_VF_GL_CTRL	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x18)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_VF_GL_CTRL	(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x19)
> > +#define IPC_BIOS_PUNIT_CMD_READ_FM_SOC_TEMP_THRESH \
> > +						(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x1a)
> > +#define IPC_BIOS_PUNIT_CMD_WRITE_FM_SOC_TEMP_THRESH \
> > +						(IPC_BIOS_PUNIT_CM
> > D_BASE + 0x1b)
> > +
> > +/*GT Driver => Pcode commands*/
> > +#define IPC_GTD_PUNIT_CMD_ZERO			(IPC_GTD_PUNIT_CMD
> > _BASE + 0x00)
> > +#define IPC_GTD_PUNIT_CMD_CONFIG		(IPC_GTD_PUNIT_CMD
> > _BASE + 0x01)
> > +#define IPC_GTD_PUNIT_CMD_READ_ICCP_LIC_CDYN_SCAL \
> > +						(IPC_GTD_PUNIT_CMD
> > _BASE + 0x02)
> > +#define IPC_GTD_PUNIT_CMD_WRITE_ICCP_LIC_CDYN_SCAL \
> > +						(IPC_GTD_PUNIT_CMD
> > _BASE + 0x03)
> > +#define IPC_GTD_PUNIT_CMD_GET_WM_VAL		(IPC_GTD_PUNIT_CMD
> > _BASE + 0x06)
> > +#define IPC_GTD_PUNIT_CMD_WRITE_CONFIG_WISHREQ	(IPC_GTD_PUNIT_CMD
> > _BASE + 0x07)
> > +#define IPC_GTD_PUNIT_CMD_READ_REQ_DUTY_CYCLE	(IPC_GTD_PUNIT_CMD
> > _BASE + 0x16)
> > +#define IPC_GTD_PUNIT_CMD_DIS_VOL_FREQ_CHANGE_REQUEST \
> > +						(IPC_GTD_PUNIT_CMD
> > _BASE + 0x17)
> > +#define IPC_GTD_PUNIT_CMD_DYNA_DUTY_CYCLE_CTRL	(IPC_GTD_PUNIT_CMD
> > _BASE + 0x1a)
> > +#define IPC_GTD_PUNIT_CMD_DYNA_DUTY_CYCLE_TUNING \
> > +						(IPC_GTD_PUNIT_CMD
> > _BASE + 0x1c)
> > +
> > +/* ISP Driver => Pcode commands */
> > +#define IPC_ISPD_PUNIT_CMD_ZERO			(IPC_ISPD_
> > PUNIT_CMD_BASE + 0x00)
> > +#define IPC_ISPD_PUNIT_CMD_CONFIG		(IPC_ISPD_PUNIT_CM
> > D_BASE + 0x01)
> > +#define IPC_ISPD_PUNIT_CMD_GET_ISP_LTR_VAL	(IPC_ISPD_PUNIT_CM
> > D_BASE + 0x02)
> > +#define IPC_ISPD_PUNIT_CMD_ACCESS_IU_FREQ_BOUNDS \
> > +						(IPC_ISPD_PUNIT_CM
> > D_BASE + 0x03)
> > +#define IPC_ISPD_PUNIT_CMD_READ_CDYN_LEVEL	(IPC_ISPD_PUNIT_CM
> > D_BASE + 0x04)
> > +#define IPC_ISPD_PUNIT_CMD_WRITE_CDYN_LEVEL	(IPC_ISPD_PUNIT_CM
> > D_BASE + 0x05)
> > +#define IPC_ISPD_PUNIT_CMD_MAX			(IPC_ISPD_PUNIT_CM
> > D_BASE + 0x06)
> > +
> > +/* Error codes */
> > +#define IPC_ERR_SUCCESS				0
> > +#define IPC_ERR_INVALID_CMD			1
> > +#define IPC_ERR_INVALID_PARAMETER		2
> > +#define IPC_ERR_CMD_TIMEOUT			3
> > +#define IPC_ERR_CMD_LOCKED			4
> > +#define IPC_ERR_INVALID_VR_ID			5
> > +#define IPC_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 intline int intel_punit_ipc_simple_command(int cmd,
> > +						  int para1, int 


Seems was never compiled with disabled option.

> > para2);
> > +{
> > +	return -EINVAL;
> > +}
> > +
> > +static inline int intel_punit_ipc_command(u32 cmd, u32 para1, u32 
> > para2,
> > +					  u32 *in, u32 *out);
> > +{
> > +	return -EINVAL;
> > +}
> > +
> > +#endif /*CONFIG_INTEL_PUNIT_IPC*/
> > +
> > +#endif
> > diff --git a/drivers/platform/x86/Kconfig 
> > b/drivers/platform/x86/Kconfig
> > index 346f1fd..42ada9b 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 PUNIT IPC Driver"

PUNIT -> P-Unit ?

> > +	default y
> > +	---help---
> > +	  IPC is used to bridge the communications between kernel 
> > and PUNIT

Ditto.

> > +
> >  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..78cb794
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_punit_ipc.c
> > @@ -0,0 +1,388 @@
> > +/*
> > + * intel_punit_ipc.c: Driver for the Intel Punit 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 Punit is the Foxton microcontroller and its 


Ditto.

> > firmware,
> > + * which provide mailbox interface for power management usage.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/device.h>
> > +#include <linux/pm.h>
> > +#include <linux/acpi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/bitops.h>
> > +#include <linux/atomic.h>
> > +#include <linux/notifier.h>
> > +#include <linux/suspend.h>
> > +#include <linux/platform_device.h>
> > +#include <asm/intel_punit_ipc.h>
> > +
> > +/* Mailbox registers */
> > +#define MAILBOX_DATA_LOW		0x0
> > +#define MAILBOX_INTERFACE		0x4
> > +#define		CMD_RUN			(1 << 31)
> > +#define		CMD_ERRCODE_MASK	0xFF
> > +#define		CMD_PARA1_SHIFT		8
> > +#define		CMD_PARA2_SHIFT		16
> > +#define		CMD_MASK		0xFF
> > +#define MAILBOX_DATA_HIGH		0x8
> > +
> > +#define MAILBOX_REGISTER_SPACE		0x10
> > +
> > +#define CMD_TIMEOUT_SECONDS		3
> > +
> > +/* Three external mailbox */
> > +enum mailbox_type {
> > +	BIOS_MAILBOX,
> > +	GTDRIVER_MAILBOX,
> > +	ISPDRIVER_MAILBOX,
> > +	RESERVED_MAILBOX,
> > +};
> > +
> > +struct intel_punit_ipc_controller {
> > +	struct platform_device *pdev;
> > +	struct mutex lock;
> > +	void __iomem *base[RESERVED_MAILBOX];

Why not to use explicitly one field per mailbox?
And therefore remove enum.

> > +	struct completion cmd_complete;
> > +	int irq;
> > +
> > +	int cmd;
> > +	enum mailbox_type type;
> > +};
> > +
> > +static char *ipc_err_sources[] = {
> > +	[IPC_ERR_SUCCESS] =
> > +		"no error",
> > +	[IPC_ERR_INVALID_CMD] =
> > +		"invalid command",
> > +	[IPC_ERR_INVALID_PARAMETER] =
> > +		"invalid parameter",
> > +	[IPC_ERR_CMD_TIMEOUT] =
> > +		"command timeout",
> > +	[IPC_ERR_CMD_LOCKED] =
> > +		"command locked",
> > +	[IPC_ERR_INVALID_VR_ID] =
> > +		"invalid vr id",
> > +	[IPC_ERR_VR_ERR] =
> > +		"vr error",
> > +};
> > +
> > +static struct intel_punit_ipc_controller ipcdev;
> > +
> > +static inline u32 ipc_read_status(void)
> > +{
> > +	return readl(ipcdev.base[ipcdev.type] + 
> > MAILBOX_INTERFACE);
> > +}
> > +
> > +static inline void ipc_write_cmd(u32 cmd)
> > +{
> > +	writel(cmd, ipcdev.base[ipcdev.type] + MAILBOX_INTERFACE);
> > +}
> > +
> > +static inline u32 ipc_read_data_low(void)
> > +{
> > +	return readl(ipcdev.base[ipcdev.type] + MAILBOX_DATA_LOW);
> > +}
> > +
> > +static inline u32 ipc_read_data_high(void)
> > +{
> > +	return readl(ipcdev.base[ipcdev.type] + 
> > MAILBOX_DATA_HIGH);
> > +}
> > +
> > +static inline void ipc_write_data_low(u32 data)
> > +{
> > +	writel(data, ipcdev.base[ipcdev.type] + MAILBOX_DATA_LOW);
> > +}
> > +
> > +static inline void ipc_write_data_high(u32 data)
> > +{
> > +	writel(data, ipcdev.base[ipcdev.type] + 
> > MAILBOX_DATA_HIGH);
> > +}
> > +
> > +static int intel_punit_init_cmd(u32 cmd)
> > +{
> > +	cmd &= CMD_MASK;
> > +	if (cmd < IPC_BIOS_PUNIT_CMD_BASE)
> > +		return -EINVAL;
> > +	else if (cmd < IPC_GTD_PUNIT_CMD_BASE)
> > +		ipcdev.type = BIOS_MAILBOX;
> > +	else if (cmd < IPC_ISPD_PUNIT_CMD_BASE)
> > +		ipcdev.type = GTDRIVER_MAILBOX;
> > +	else if (cmd < IPC_ISPD_PUNIT_CMD_MAX)
> > +		ipcdev.type = ISPDRIVER_MAILBOX;
> > +	else
> > +		return -EINVAL;

It seems you call this each time you want to send a command.
It would be nicer without ipcdev.type since you create an artificial
type in the same code, so you already know the way how to do that. And
there is more confusion since your type is permanently mutating.

> > +
> > +	return 0;
> > +}
> > +
> > +static int intel_punit_ipc_send_command(u32 cmd)
> > +{
> > +	int ret;
> > +
> > +	ret = intel_punit_init_cmd(cmd);
> > +	if (ret)
> > +		return ret;
> > +	ipcdev.cmd = cmd;
> > +	reinit_completion(&ipcdev.cmd_complete);
> > +	ipc_write_cmd(cmd);
> > +	return 0;
> > +}
> > +
> > +static int intel_punit_ipc_check_status(void)
> > +{
> > +	int loops = CMD_TIMEOUT_SECONDS * USEC_PER_SEC;
> > +	int ret = 0;
> > +	int status;
> > +	int errcode;
> > +
> > +	if (ipcdev.irq) {
> > +		if (0 == wait_for_completion_timeout(
> > +				&ipcdev.cmd_complete,

> > +				CMD_TIMEOUT_SECONDS * HZ)) {

Can you use pattern
if (func() == const) instead?

> > +			dev_err(&ipcdev.pdev->dev,
> > +				"IPC timed out, IPC_CMD=0x%x\n", 
> > ipcdev.cmd);
> > +			return -ETIMEDOUT;
> > +		}
> > +	} else {
> > +		while ((ipc_read_status() & CMD_RUN) && --loops)
> > +			udelay(1);
> > +		if (!loops) {
> > +			dev_err(&ipcdev.pdev->dev,
> > +				"IPC timed out, IPC_CMD=0x%x\n", 
> > ipcdev.cmd);
> > +			return -ETIMEDOUT;
> > +		}
> > +	}
> > +
> > +	status = ipc_read_status();
> > +	errcode = status & CMD_ERRCODE_MASK;
> > +	if (errcode) {
> > +		ret = -EIO;
> > +		if (errcode < ARRAY_SIZE(ipc_err_sources))
> > +			dev_err(&ipcdev.pdev->dev,
> > +				"IPC failed: %s, IPC_STS=0x%x, 
> > IPC_CMD=0x%x\n",
> > +				ipc_err_sources[errcode], status, 
> > ipcdev.cmd);
> > +		else
> > +			dev_err(&ipcdev.pdev->dev,
> > +				"IPC failed: unknown err,STS=0x%x, 
> > CMD=0x%x\n",
> > +				status, ipcdev.cmd);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * intel_punit_ipc_simple_command() - Simple IPC command
> > + * @cmd:	IPC command code.
> > + * @para1:	First 8bit parameter, set 0 if invalid.
> > + * @para2:	Second 8bit parameter, set 0 if invalid.
> > + *
> > + * Send a IPC command to Punit when there is no data transaction
> > + *
> > + * Return:	an IPC error code or 0 on success.
> > + */
> > +int intel_punit_ipc_simple_command(int cmd, int para1, int para2)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&ipcdev.lock);
> > +	ret = intel_punit_ipc_send_command(CMD_RUN |
> > +					   para2 << 
> > CMD_PARA2_SHIFT |
> > +					   para1 << 
> > CMD_PARA1_SHIFT | cmd);
> > +	if (ret)
> > +		goto out;
> > +	ret = intel_punit_ipc_check_status();
> > +out:
> > +	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 invalid.
> > + * @para2:	Second 8bit parameter, set 0 if invalid.
> > + * @in:		Input data, 32bit for BIOS cmd, two 32bit 
> > for GTD and ISPD.
> > + * @out:	Output data.
> > + *
> > + * Send a IPC command to Punit with data transaction
> > + *
> > + * Return:	an IPC error code or 0 on success.
> > + */
> > +int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2, u32 
> > *in, u32 *out)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&ipcdev.lock);
> > +	ipc_write_data_low(*in);
> > +	if (ipcdev.type == GTDRIVER_MAILBOX ||
> > +			ipcdev.type == ISPDRIVER_MAILBOX) {
> > +		in++;
> > +		ipc_write_data_high(*in);

*++in

> > +	}
> > +	ret = intel_punit_ipc_send_command(CMD_RUN |
> > +					   para2 << 
> > CMD_PARA2_SHIFT |
> > +					   para1 << 
> > CMD_PARA1_SHIFT | cmd);
> > +	if (ret)
> > +		goto out;
> > +	ret = intel_punit_ipc_check_status();
> > +	*out = ipc_read_data_low();
> > +	if (ipcdev.type == GTDRIVER_MAILBOX ||
> > +			ipcdev.type == ISPDRIVER_MAILBOX) {
> > +		out++;
> > +		*out = ipc_read_data_high();

*++out

> > +	}
> > +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)
> > +{
> > +	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;
> > +	int ret;
> > +
> > +	res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res0) {
> > +		dev_err(&pdev->dev, "Failed to get iomem 
> > resource\n");
> > +		return -EINVAL;
> > +	}
> > +	size = resource_size(res0);
> > +	if (!request_mem_region(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 -EINVAL;
> > +	}
> > +	size = resource_size(res1);
> > +	if (!request_mem_region(res1->start, size, pdev->name)) {
> > +		dev_err(&pdev->dev, "Failed to request iomem 
> > resouce1\n");
> > +		ret = -EBUSY;
> > +		goto err_res1;
> > +	}
> > +
> > +	addr = ioremap_nocache(res0->start,
> > +			       resource_size(res0) + 
> > resource_size(res1));
> > +	if (!addr) {
> > +		dev_err(&pdev->dev, "Failed to ioremap ipc 
> > base\n");
> > +		ret = -ENOMEM;
> > +		goto err_map;
> > +	}
> > +	ipcdev.base[BIOS_MAILBOX] = addr;
> > +	addr += MAILBOX_REGISTER_SPACE;
> > +	ipcdev.base[GTDRIVER_MAILBOX] = addr;
> > +	addr += MAILBOX_REGISTER_SPACE;
> > +	ipcdev.base[ISPDRIVER_MAILBOX] = addr;

Wait, is this MFD or just you logically split it. If the latter is
true, then might be no need to differentiate addresses.

> > +
> > +	return 0;
> > +
> > +err_map:
> > +	release_mem_region(res1->start, resource_size(res1));
> > +err_res1:
> > +	release_mem_region(res0->start, resource_size(res0));
> > +	return ret;
> > +}
> > +
> > +static int intel_punit_ipc_probe(struct platform_device *pdev)
> > +{
> > +	int irq;
> > +	int ret;
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		ipcdev.irq = 0;
> > +		dev_warn(&pdev->dev, "Could not get irq 
> > number\n");
> > +	} else {
> > +		if (request_irq(irq, intel_punit_ioc, 
> > IRQF_NO_SUSPEND,
> > +				"intel_punit_ipc", &ipcdev)) {
> > +			dev_err(&pdev->dev, "request irq %d\n", 
> > irq);
> > +			return -EBUSY;
> > +		}
> > +		ipcdev.irq = irq;
> > +	}
> > +
> > +	ret = intel_punit_get_bars(pdev);
> > +	if (ret) {
> > +		if (ipcdev.irq)
> > +			free_irq(ipcdev.irq, &ipcdev);
> > +		return ret;
> > +	}
> > +
> > +	ipcdev.pdev = pdev;
> > +	mutex_init(&ipcdev.lock);
> > +	init_completion(&ipcdev.cmd_complete);
> > +	return ret;
> > +}
> > +
> > +static int intel_punit_ipc_remove(struct platform_device *pdev)
> > +{
> > +	struct resource *res;
> > +
> > +	if (ipcdev.irq)
> > +		free_irq(ipcdev.irq, &ipcdev);
> > +	iounmap(ipcdev.base[BIOS_MAILBOX]);
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (res)
> > +		release_mem_region(res->start, 
> > resource_size(res));
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	if (res)
> > +		release_mem_region(res->start, 
> > resource_size(res));
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_ACPI

No need to have #ifdef here.

> > +static const struct acpi_device_id punit_ipc_acpi_ids[] = {
> > +	{"INT34D4", 0}
> > +};
> > +#endif
> > +
> > +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 Punit IPC driver");
> > +MODULE_LICENSE("GPL");
> > +
> > +/* Some modules are dependent on this, so init earlier */

So, initcalls make sense only for built-in drivers. Which one will
require that?

> > +fs_initcall(intel_punit_ipc_init);
> > +module_exit(intel_punit_ipc_exit);
> > -- 
> > 1.8.3.2
> > 
> > 
> 

-- 
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.

Re: [PATCH] platform:x86: add Intel Punit mailbox IPC driver

[Matt Fleming]

On Mon, 2015-08-10 at 16:04 +0100, Shevchenko, Andriy wrote:
> 
> Matt, by the way intel_pmc_ipc.c module creates iTCO device (LPC bus).
> Does it look correct?

Yeah, it looks OK to me. Note there are changes queued up in Lee Jones'
tree for changing the lpc_ich_info into itco_wdt_platform_data.

Btw, is there a reason that intel_pmc_ipc doesn't use the mfd_* API
since it's concerned with multiple functions?

Re: [PATCH] platform:x86: add Intel Punit mailbox IPC driver

[Darren Hart]

On Tue, Aug 18, 2015 at 04:50:56PM +0100, Matt Fleming wrote:
> On Mon, 2015-08-10 at 16:04 +0100, Shevchenko, Andriy wrote:
> > 
> > Matt, by the way intel_pmc_ipc.c module creates iTCO device (LPC bus).
> > Does it look correct?
> 
> Yeah, it looks OK to me. Note there are changes queued up in Lee Jones'
> tree for changing the lpc_ich_info into itco_wdt_platform_data.
> 
> Btw, is there a reason that intel_pmc_ipc doesn't use the mfd_* API
> since it's concerned with multiple functions?

Qipeng?

I saw the v2 come in, but didn't see this question addressed in the changelog.

-- 
Darren Hart
Intel Open Source Technology Center

RE: [PATCH] platform:x86: add Intel Punit mailbox IPC driver

[Zha, Qipeng]

On Tue, Aug 18, 2015 at 04:50:56PM +0100, Matt Fleming wrote:
> On Mon, 2015-08-10 at 16:04 +0100, Shevchenko, Andriy wrote:
> > 
> > Matt, by the way intel_pmc_ipc.c module creates iTCO device (LPC bus).
> > Does it look correct?
> 
> Yeah, it looks OK to me. Note there are changes queued up in Lee Jones'
> tree for changing the lpc_ich_info into itco_wdt_platform_data.
> 
> Btw, is there a reason that intel_pmc_ipc doesn't use the mfd_* API 
> since it's concerned with multiple functions?

> Qipeng?

> I saw the v2 come in, but didn't see this question addressed in the changelog.

When design P-unit driver, we got feedback from BIOS that there will allocate a dedicate
ACPI id for P-unit controller, and finally BIOS decide not do that, just put some necessary 
Resource in PMC acpi table.
After that, we got request to do same thing for iTco.

I agree it's better to update pmc driver to use mfd_add_devices to create punit and iTco device.

--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] platform:x86: add Intel Punit mailbox IPC driver

[Darren Hart]

On Wed, Aug 26, 2015 at 08:22:33AM +0000, Zha, Qipeng wrote:
> On Tue, Aug 18, 2015 at 04:50:56PM +0100, Matt Fleming wrote:
> > On Mon, 2015-08-10 at 16:04 +0100, Shevchenko, Andriy wrote:
> > > 
> > > Matt, by the way intel_pmc_ipc.c module creates iTCO device (LPC bus).
> > > Does it look correct?
> > 
> > Yeah, it looks OK to me. Note there are changes queued up in Lee Jones'
> > tree for changing the lpc_ich_info into itco_wdt_platform_data.
> > 
> > Btw, is there a reason that intel_pmc_ipc doesn't use the mfd_* API 
> > since it's concerned with multiple functions?
> 
> > Qipeng?
> 
> > I saw the v2 come in, but didn't see this question addressed in the changelog.
> 
> When design P-unit driver, we got feedback from BIOS that there will allocate a dedicate
> ACPI id for P-unit controller, and finally BIOS decide not do that, just put some necessary 
> Resource in PMC acpi table.
> After that, we got request to do same thing for iTco.
> 
> I agree it's better to update pmc driver to use mfd_add_devices to create punit and iTco device.

Does this mean there sill be a v4 I should be waiting for?

-- 
Darren Hart
Intel Open Source Technology Center