[PATCH v1 1/2] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices

[PATCH v1 1/2] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Currently, we have lot of repetitive code in dependent device resource
allocation and device creation handling code. This logic can be improved if
we use MFD framework for dependent device creation. This patch adds this
support.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_ipc.c | 401 +++++++++++++----------------------
 1 file changed, 143 insertions(+), 258 deletions(-)

This patch was originally part of "SCU/PMC/PUNIT IPC driver clean up" series. But I have
split them into two sets because we need more thorough review on generic IPC driver
design patches (second set).

https://www.spinics.net/lists/linux-watchdog/msg12796.html

Changes since previous set version (v8):
 * Addressed comments from Heikki and cleaned up some debug messages. 

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index e03fa314..fadf721 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -20,6 +20,7 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/device.h>
+#include <linux/mfd/core.h>
 #include <linux/pm.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
@@ -88,6 +89,7 @@
 #define PLAT_RESOURCE_ISP_IFACE_INDEX	5
 #define PLAT_RESOURCE_GTD_DATA_INDEX	6
 #define PLAT_RESOURCE_GTD_IFACE_INDEX	7
+#define PLAT_RESOURCE_MEM_MAX_INDEX	8
 #define PLAT_RESOURCE_ACPI_IO_INDEX	0
 
 /*
@@ -106,8 +108,6 @@
 #define TELEM_SSRAM_SIZE		240
 #define TELEM_PMC_SSRAM_OFFSET		0x1B00
 #define TELEM_PUNIT_SSRAM_OFFSET	0x1A00
-#define TCO_PMC_OFFSET			0x8
-#define TCO_PMC_SIZE			0x4
 
 /* PMC register bit definitions */
 
@@ -124,26 +124,10 @@ static struct intel_pmc_ipc_dev {
 	int cmd;
 	struct completion cmd_complete;
 
-	/* The following PMC BARs share the same ACPI device with the IPC */
-	resource_size_t acpi_io_base;
-	int acpi_io_size;
-	struct platform_device *tco_dev;
-
 	/* gcr */
 	void __iomem *gcr_mem_base;
 	bool has_gcr_regs;
 	spinlock_t gcr_lock;
-
-	/* punit */
-	struct platform_device *punit_dev;
-
-	/* Telemetry */
-	resource_size_t telem_pmc_ssram_base;
-	resource_size_t telem_punit_ssram_base;
-	int telem_pmc_ssram_size;
-	int telem_punit_ssram_size;
-	u8 telem_res_inval;
-	struct platform_device *telemetry_dev;
 } ipcdev;
 
 static char *ipc_err_sources[] = {
@@ -508,7 +492,7 @@ static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	ret = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_pmc_ipc",
 				pmc);
 	if (ret) {
-		dev_err(&pdev->dev, "Failed to request irq\n");
+		dev_err(&pdev->dev, "Failed to request IRQ\n");
 		return ret;
 	}
 
@@ -593,44 +577,6 @@ static const struct attribute_group intel_ipc_group = {
 	.attrs = intel_ipc_attrs,
 };
 
-static struct resource punit_res_array[] = {
-	/* Punit BIOS */
-	{
-		.flags = IORESOURCE_MEM,
-	},
-	{
-		.flags = IORESOURCE_MEM,
-	},
-	/* Punit ISP */
-	{
-		.flags = IORESOURCE_MEM,
-	},
-	{
-		.flags = IORESOURCE_MEM,
-	},
-	/* Punit GTD */
-	{
-		.flags = IORESOURCE_MEM,
-	},
-	{
-		.flags = IORESOURCE_MEM,
-	},
-};
-
-#define TCO_RESOURCE_ACPI_IO		0
-#define TCO_RESOURCE_SMI_EN_IO		1
-#define TCO_RESOURCE_GCR_MEM		2
-static struct resource tco_res[] = {
-	/* ACPI - TCO */
-	{
-		.flags = IORESOURCE_IO,
-	},
-	/* ACPI - SMI */
-	{
-		.flags = IORESOURCE_IO,
-	},
-};
-
 static struct itco_wdt_platform_data tco_info = {
 	.name = "Apollo Lake SoC",
 	.version = 5,
@@ -638,234 +584,182 @@ static struct itco_wdt_platform_data tco_info = {
 	.update_no_reboot_bit = update_no_reboot_bit,
 };
 
-#define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
-#define TELEMETRY_RESOURCE_PMC_SSRAM	1
-static struct resource telemetry_res[] = {
-	/*Telemetry*/
-	{
-		.flags = IORESOURCE_MEM,
-	},
-	{
-		.flags = IORESOURCE_MEM,
-	},
-};
-
-static int ipc_create_punit_device(void)
+static int ipc_create_punit_device(struct platform_device *pdev)
 {
-	struct platform_device *pdev;
-	const struct platform_device_info pdevinfo = {
-		.parent = ipcdev.dev,
-		.name = PUNIT_DEVICE_NAME,
-		.id = -1,
-		.res = punit_res_array,
-		.num_res = ARRAY_SIZE(punit_res_array),
+	static struct resource punit_res[PLAT_RESOURCE_MEM_MAX_INDEX];
+	static struct mfd_cell punit_cell;
+	struct resource *res;
+	int ret, mindex, pindex = 0;
+
+	for (mindex = 0; mindex <= PLAT_RESOURCE_MEM_MAX_INDEX; mindex++) {
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, mindex);
+
+		switch (mindex) {
+		/* Get PUNIT resources */
+		case PLAT_RESOURCE_BIOS_DATA_INDEX:
+		case PLAT_RESOURCE_BIOS_IFACE_INDEX:
+			/* BIOS resources are required, so return error if not
+			 * available
+			 */
+			if (!res) {
+				dev_err(&pdev->dev,
+					"Failed to get PUNIT MEM resource %d\n",
+					pindex);
+				return -ENXIO;
+			}
+		case PLAT_RESOURCE_ISP_DATA_INDEX:
+		case PLAT_RESOURCE_ISP_IFACE_INDEX:
+		case PLAT_RESOURCE_GTD_DATA_INDEX:
+		case PLAT_RESOURCE_GTD_IFACE_INDEX:
+			/* if valid resource found, copy the resource to PUNIT
+			 * resource
+			 */
+			if (res)
+				memcpy(&punit_res[pindex], res, sizeof(*res));
+			punit_res[pindex].flags = IORESOURCE_MEM;
+			pindex++;
+			break;
 		};
+	}
 
-	pdev = platform_device_register_full(&pdevinfo);
-	if (IS_ERR(pdev))
-		return PTR_ERR(pdev);
+	/* Create PUNIT IPC MFD cell */
+	punit_cell.name = PUNIT_DEVICE_NAME;
+	punit_cell.num_resources = ARRAY_SIZE(punit_res);
+	punit_cell.resources = punit_res;
+	punit_cell.ignore_resource_conflicts = 1;
 
-	ipcdev.punit_dev = pdev;
+	ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, &punit_cell,
+				   1, NULL, 0, NULL);
 
-	return 0;
+	dev_dbg(&pdev->dev, "Successfully created PUNIT device\n");
+
+	return ret;
 }
 
-static int ipc_create_tco_device(void)
+static int ipc_create_wdt_device(struct platform_device *pdev)
 {
-	struct platform_device *pdev;
+	static struct resource wdt_ipc_res[2];
+	static struct mfd_cell wdt_cell;
 	struct resource *res;
-	const struct platform_device_info pdevinfo = {
-		.parent = ipcdev.dev,
-		.name = TCO_DEVICE_NAME,
-		.id = -1,
-		.res = tco_res,
-		.num_res = ARRAY_SIZE(tco_res),
-		.data = &tco_info,
-		.size_data = sizeof(tco_info),
-		};
+	int ret;
 
-	res = tco_res + TCO_RESOURCE_ACPI_IO;
-	res->start = ipcdev.acpi_io_base + TCO_BASE_OFFSET;
-	res->end = res->start + TCO_REGS_SIZE - 1;
+	/* If we have ACPI based watchdog use that instead, othewise create
+	 * a MFD cell for iTCO watchdog
+	 */
+	if (acpi_has_watchdog())
+		return 0;
 
-	res = tco_res + TCO_RESOURCE_SMI_EN_IO;
-	res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET;
-	res->end = res->start + SMI_EN_SIZE - 1;
+	/* Get iTCO watchdog resources */
+	res = platform_get_resource(pdev, IORESOURCE_IO,
+				    PLAT_RESOURCE_ACPI_IO_INDEX);
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get WDT resource\n");
+		return -ENXIO;
+	}
 
-	pdev = platform_device_register_full(&pdevinfo);
-	if (IS_ERR(pdev))
-		return PTR_ERR(pdev);
+	wdt_ipc_res[0].start = res->start + TCO_BASE_OFFSET;
+	wdt_ipc_res[0].end = res->start +
+		TCO_BASE_OFFSET + TCO_REGS_SIZE - 1;
+	wdt_ipc_res[0].flags = IORESOURCE_IO;
+	wdt_ipc_res[1].start = res->start + SMI_EN_OFFSET;
+	wdt_ipc_res[1].end = res->start +
+		SMI_EN_OFFSET + SMI_EN_SIZE - 1;
+	wdt_ipc_res[1].flags = IORESOURCE_IO;
 
-	ipcdev.tco_dev = pdev;
+	wdt_cell.name = TCO_DEVICE_NAME;
+	wdt_cell.platform_data = &tco_info;
+	wdt_cell.pdata_size = sizeof(tco_info);
+	wdt_cell.num_resources = ARRAY_SIZE(wdt_ipc_res);
+	wdt_cell.resources = wdt_ipc_res;
+	wdt_cell.ignore_resource_conflicts = 1;
 
-	return 0;
+	ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, &wdt_cell,
+				   1, NULL, 0, NULL);
+
+	dev_dbg(&pdev->dev, "Successfully created watchdog device\n");
+
+	return ret;
 }
 
-static int ipc_create_telemetry_device(void)
+static int ipc_create_telemetry_device(struct platform_device *pdev)
 {
-	struct platform_device *pdev;
+	static struct resource telemetry_ipc_res[2];
+	static struct mfd_cell telemetry_cell;
 	struct resource *res;
-	const struct platform_device_info pdevinfo = {
-		.parent = ipcdev.dev,
-		.name = TELEMETRY_DEVICE_NAME,
-		.id = -1,
-		.res = telemetry_res,
-		.num_res = ARRAY_SIZE(telemetry_res),
-		};
+	int ret;
 
-	res = telemetry_res + TELEMETRY_RESOURCE_PUNIT_SSRAM;
-	res->start = ipcdev.telem_punit_ssram_base;
-	res->end = res->start + ipcdev.telem_punit_ssram_size - 1;
+	/* Get telemetry resources */
+	res = platform_get_resource(pdev, IORESOURCE_MEM,
+				    PLAT_RESOURCE_TELEM_SSRAM_INDEX);
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get telemetry resource\n");
+		return -ENXIO;
+	}
 
-	res = telemetry_res + TELEMETRY_RESOURCE_PMC_SSRAM;
-	res->start = ipcdev.telem_pmc_ssram_base;
-	res->end = res->start + ipcdev.telem_pmc_ssram_size - 1;
+	telemetry_ipc_res[0].start = res->start + TELEM_PUNIT_SSRAM_OFFSET;
+	telemetry_ipc_res[0].end = res->start +
+		TELEM_PUNIT_SSRAM_OFFSET + TELEM_SSRAM_SIZE - 1;
+	telemetry_ipc_res[0].flags = IORESOURCE_MEM;
+	telemetry_ipc_res[1].start = res->start + TELEM_PMC_SSRAM_OFFSET;
+	telemetry_ipc_res[1].end = res->start +
+		TELEM_PMC_SSRAM_OFFSET + TELEM_SSRAM_SIZE - 1;
+	telemetry_ipc_res[1].flags = IORESOURCE_MEM;
 
-	pdev = platform_device_register_full(&pdevinfo);
-	if (IS_ERR(pdev))
-		return PTR_ERR(pdev);
+	telemetry_cell.name = TELEMETRY_DEVICE_NAME;
+	telemetry_cell.num_resources = ARRAY_SIZE(telemetry_ipc_res);
+	telemetry_cell.resources = telemetry_ipc_res;
+	telemetry_cell.ignore_resource_conflicts = 1;
 
-	ipcdev.telemetry_dev = pdev;
+	ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
+				   &telemetry_cell, 1, NULL, 0, NULL);
 
-	return 0;
+	dev_dbg(&pdev->dev, "Successfully created telemetry device\n");
+
+	return ret;
 }
 
-static int ipc_create_pmc_devices(void)
+static int ipc_create_pmc_devices(struct platform_device *pdev)
 {
 	int ret;
 
-	/* If we have ACPI based watchdog use that instead */
-	if (!acpi_has_watchdog()) {
-		ret = ipc_create_tco_device();
-		if (ret) {
-			dev_err(ipcdev.dev, "Failed to add tco platform device\n");
-			return ret;
-		}
-	}
+	ret = ipc_create_punit_device(pdev);
+	if (ret < 0)
+		return ret;
 
-	ret = ipc_create_punit_device();
-	if (ret) {
-		dev_err(ipcdev.dev, "Failed to add punit platform device\n");
-		platform_device_unregister(ipcdev.tco_dev);
-	}
+	ret = ipc_create_wdt_device(pdev);
+	if (ret < 0)
+		return ret;
 
-	if (!ipcdev.telem_res_inval) {
-		ret = ipc_create_telemetry_device();
-		if (ret)
-			dev_warn(ipcdev.dev,
-				"Failed to add telemetry platform device\n");
-	}
+	ret = ipc_create_telemetry_device(pdev);
+	if (ret < 0)
+		return ret;
 
-	return ret;
+	return 0;
 }
 
 static int ipc_plat_get_res(struct platform_device *pdev)
 {
-	struct resource *res, *punit_res;
+	struct resource *res;
 	void __iomem *addr;
-	int size;
-
-	res = platform_get_resource(pdev, IORESOURCE_IO,
-				    PLAT_RESOURCE_ACPI_IO_INDEX);
-	if (!res) {
-		dev_err(&pdev->dev, "Failed to get io resource\n");
-		return -ENXIO;
-	}
-	size = resource_size(res);
-	ipcdev.acpi_io_base = res->start;
-	ipcdev.acpi_io_size = size;
-	dev_info(&pdev->dev, "io res: %pR\n", res);
-
-	punit_res = punit_res_array;
-	/* This is index 0 to cover BIOS data register */
-	res = platform_get_resource(pdev, IORESOURCE_MEM,
-				    PLAT_RESOURCE_BIOS_DATA_INDEX);
-	if (!res) {
-		dev_err(&pdev->dev, "Failed to get res of punit BIOS data\n");
-		return -ENXIO;
-	}
-	*punit_res = *res;
-	dev_info(&pdev->dev, "punit BIOS data res: %pR\n", res);
-
-	/* This is index 1 to cover BIOS interface register */
-	res = platform_get_resource(pdev, IORESOURCE_MEM,
-				    PLAT_RESOURCE_BIOS_IFACE_INDEX);
-	if (!res) {
-		dev_err(&pdev->dev, "Failed to get res of punit BIOS iface\n");
-		return -ENXIO;
-	}
-	*++punit_res = *res;
-	dev_info(&pdev->dev, "punit BIOS interface res: %pR\n", res);
-
-	/* This is index 2 to cover ISP data register, optional */
-	res = platform_get_resource(pdev, IORESOURCE_MEM,
-				    PLAT_RESOURCE_ISP_DATA_INDEX);
-	++punit_res;
-	if (res) {
-		*punit_res = *res;
-		dev_info(&pdev->dev, "punit ISP data res: %pR\n", res);
-	}
-
-	/* This is index 3 to cover ISP interface register, optional */
-	res = platform_get_resource(pdev, IORESOURCE_MEM,
-				    PLAT_RESOURCE_ISP_IFACE_INDEX);
-	++punit_res;
-	if (res) {
-		*punit_res = *res;
-		dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res);
-	}
-
-	/* This is index 4 to cover GTD data register, optional */
-	res = platform_get_resource(pdev, IORESOURCE_MEM,
-				    PLAT_RESOURCE_GTD_DATA_INDEX);
-	++punit_res;
-	if (res) {
-		*punit_res = *res;
-		dev_info(&pdev->dev, "punit GTD data res: %pR\n", res);
-	}
-
-	/* This is index 5 to cover GTD interface register, optional */
-	res = platform_get_resource(pdev, IORESOURCE_MEM,
-				    PLAT_RESOURCE_GTD_IFACE_INDEX);
-	++punit_res;
-	if (res) {
-		*punit_res = *res;
-		dev_info(&pdev->dev, "punit GTD interface res: %pR\n", res);
-	}
 
+	/* Get IPC resources */
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_IPC_INDEX);
 	if (!res) {
-		dev_err(&pdev->dev, "Failed to get ipc resource\n");
+		dev_err(&pdev->dev, "Failed to get IPC resources\n");
 		return -ENXIO;
 	}
-	size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;
-	res->end = res->start + size - 1;
+
+	res->end = res->start +
+		   PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE - 1;
 
 	addr = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(addr))
 		return PTR_ERR(addr);
 
 	ipcdev.ipc_base = addr;
-
 	ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
-	dev_info(&pdev->dev, "ipc res: %pR\n", res);
-
-	ipcdev.telem_res_inval = 0;
-	res = platform_get_resource(pdev, IORESOURCE_MEM,
-				    PLAT_RESOURCE_TELEM_SSRAM_INDEX);
-	if (!res) {
-		dev_err(&pdev->dev, "Failed to get telemetry ssram resource\n");
-		ipcdev.telem_res_inval = 1;
-	} else {
-		ipcdev.telem_punit_ssram_base = res->start +
-						TELEM_PUNIT_SSRAM_OFFSET;
-		ipcdev.telem_punit_ssram_size = TELEM_SSRAM_SIZE;
-		ipcdev.telem_pmc_ssram_base = res->start +
-						TELEM_PMC_SSRAM_OFFSET;
-		ipcdev.telem_pmc_ssram_size = TELEM_SSRAM_SIZE;
-		dev_info(&pdev->dev, "telemetry ssram res: %pR\n", res);
-	}
 
 	return 0;
 }
@@ -911,7 +805,7 @@ static int ipc_plat_probe(struct platform_device *pdev)
 
 	ipcdev.irq = platform_get_irq(pdev, 0);
 	if (ipcdev.irq < 0) {
-		dev_err(&pdev->dev, "Failed to get irq\n");
+		dev_err(&pdev->dev, "Failed to get IRQ\n");
 		return -EINVAL;
 	}
 
@@ -921,47 +815,38 @@ static int ipc_plat_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = ipc_create_pmc_devices();
+	ret = ipc_create_pmc_devices(pdev);
 	if (ret) {
-		dev_err(&pdev->dev, "Failed to create pmc devices\n");
+		dev_err(&pdev->dev, "Failed to create PMC devices\n");
 		return ret;
 	}
 
-	if (devm_request_irq(&pdev->dev, ipcdev.irq, ioc, IRQF_NO_SUSPEND,
-			     "intel_pmc_ipc", &ipcdev)) {
-		dev_err(&pdev->dev, "Failed to request irq\n");
-		ret = -EBUSY;
-		goto err_irq;
+	ret = devm_request_irq(&pdev->dev, ipcdev.irq, ioc, IRQF_NO_SUSPEND,
+			       "intel_pmc_ipc", &ipcdev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request IRQ\n");
+		return ret;
 	}
 
 	ret = sysfs_create_group(&pdev->dev.kobj, &intel_ipc_group);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to create sysfs group %d\n",
 			ret);
-		goto err_sys;
+		devm_free_irq(&pdev->dev, ipcdev.irq, &ipcdev);
+		return ret;
 	}
 
 	ipcdev.has_gcr_regs = true;
 
 	return 0;
-err_sys:
-	devm_free_irq(&pdev->dev, ipcdev.irq, &ipcdev);
-err_irq:
-	platform_device_unregister(ipcdev.tco_dev);
-	platform_device_unregister(ipcdev.punit_dev);
-	platform_device_unregister(ipcdev.telemetry_dev);
-
-	return ret;
 }
 
 static int ipc_plat_remove(struct platform_device *pdev)
 {
 	sysfs_remove_group(&pdev->dev.kobj, &intel_ipc_group);
 	devm_free_irq(&pdev->dev, ipcdev.irq, &ipcdev);
-	platform_device_unregister(ipcdev.tco_dev);
-	platform_device_unregister(ipcdev.punit_dev);
-	platform_device_unregister(ipcdev.telemetry_dev);
 	ipcdev.dev = NULL;
+
 	return 0;
 }
 
@@ -980,12 +865,12 @@ static int __init intel_pmc_ipc_init(void)
 
 	ret = platform_driver_register(&ipc_plat_driver);
 	if (ret) {
-		pr_err("Failed to register PMC ipc platform driver\n");
+		pr_err("Failed to register PMC IPC platform driver\n");
 		return ret;
 	}
 	ret = pci_register_driver(&ipc_pci_driver);
 	if (ret) {
-		pr_err("Failed to register PMC ipc pci driver\n");
+		pr_err("Failed to register PMC IPC PCI driver\n");
 		platform_driver_unregister(&ipc_plat_driver);
 		return ret;
 	}
-- 
2.7.4

[PATCH v1 2/2] platform/x86: intel_pmc_ipc: Use regmap calls for GCR updates

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

This patch adds support for regmap based implementation for GCR
read/write/update APIs.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/platform/x86/Kconfig         |   1 +
 drivers/platform/x86/intel_pmc_ipc.c | 124 +++++++++++++----------------------
 2 files changed, 46 insertions(+), 79 deletions(-)

This patch was originally part of "SCU/PMC/PUNIT IPC driver clean up" series. But I have
split them into two sets because we need more thorough review on generic IPC driver
design patches (second set).

https://www.spinics.net/lists/linux-watchdog/msg12796.html

Changes since previous set version (v8):
 * Removed gcr_mem_base variable.

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 2c745e8..ac618fd 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1087,6 +1087,7 @@ config PVPANIC
 config INTEL_PMC_IPC
 	tristate "Intel PMC IPC Driver"
 	depends on ACPI
+	select REGMAP_MMIO
 	---help---
 	This driver provides support for PMC control on some Intel platforms.
 	The PMC is an ARC processor which defines IPC commands for communication
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index fadf721..650144a 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -35,6 +35,7 @@
 #include <linux/acpi.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/spinlock.h>
+#include <linux/regmap.h>
 
 #include <asm/intel_pmc_ipc.h>
 
@@ -125,9 +126,7 @@ static struct intel_pmc_ipc_dev {
 	struct completion cmd_complete;
 
 	/* gcr */
-	void __iomem *gcr_mem_base;
-	bool has_gcr_regs;
-	spinlock_t gcr_lock;
+	struct regmap *gcr_regs;
 } ipcdev;
 
 static char *ipc_err_sources[] = {
@@ -149,6 +148,15 @@ static char *ipc_err_sources[] = {
 		"Unsigned kernel",
 };
 
+static struct regmap_config gcr_regmap_config = {
+	.name = "intel_pmc_gcr",
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+	.max_register = PLAT_RESOURCE_GCR_SIZE,
+};
+
 /* Prevent concurrent calls to the PMC */
 static DEFINE_MUTEX(ipclock);
 
@@ -182,21 +190,6 @@ static inline u32 ipc_data_readl(u32 offset)
 	return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
 }
 
-static inline u64 gcr_data_readq(u32 offset)
-{
-	return readq(ipcdev.gcr_mem_base + offset);
-}
-
-static inline int is_gcr_valid(u32 offset)
-{
-	if (!ipcdev.has_gcr_regs)
-		return -EACCES;
-
-	if (offset > PLAT_RESOURCE_GCR_SIZE)
-		return -EINVAL;
-
-	return 0;
-}
 
 /**
  * intel_pmc_gcr_read() - Read PMC GCR register
@@ -209,21 +202,12 @@ static inline int is_gcr_valid(u32 offset)
  */
 int intel_pmc_gcr_read(u32 offset, u32 *data)
 {
-	int ret;
-
-	spin_lock(&ipcdev.gcr_lock);
-
-	ret = is_gcr_valid(offset);
-	if (ret < 0) {
-		spin_unlock(&ipcdev.gcr_lock);
-		return ret;
-	}
-
-	*data = readl(ipcdev.gcr_mem_base + offset);
+	struct intel_pmc_ipc_dev *pmc = &ipcdev;
 
-	spin_unlock(&ipcdev.gcr_lock);
+	if (!pmc->gcr_regs)
+		return -EACCES;
 
-	return 0;
+	return regmap_read(pmc->gcr_regs, offset, data);
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
 
@@ -239,21 +223,12 @@ EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
  */
 int intel_pmc_gcr_write(u32 offset, u32 data)
 {
-	int ret;
-
-	spin_lock(&ipcdev.gcr_lock);
-
-	ret = is_gcr_valid(offset);
-	if (ret < 0) {
-		spin_unlock(&ipcdev.gcr_lock);
-		return ret;
-	}
-
-	writel(data, ipcdev.gcr_mem_base + offset);
+	struct intel_pmc_ipc_dev *pmc = &ipcdev;
 
-	spin_unlock(&ipcdev.gcr_lock);
+	if (!pmc->gcr_regs)
+		return -EACCES;
 
-	return 0;
+	return regmap_write(pmc->gcr_regs, offset, data);
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
 
@@ -270,33 +245,12 @@ EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
  */
 int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
 {
-	u32 new_val;
-	int ret = 0;
-
-	spin_lock(&ipcdev.gcr_lock);
-
-	ret = is_gcr_valid(offset);
-	if (ret < 0)
-		goto gcr_ipc_unlock;
-
-	new_val = readl(ipcdev.gcr_mem_base + offset);
-
-	new_val &= ~mask;
-	new_val |= val & mask;
-
-	writel(new_val, ipcdev.gcr_mem_base + offset);
-
-	new_val = readl(ipcdev.gcr_mem_base + offset);
+	struct intel_pmc_ipc_dev *pmc = &ipcdev;
 
-	/* check whether the bit update is successful */
-	if ((new_val & mask) != (val & mask)) {
-		ret = -EIO;
-		goto gcr_ipc_unlock;
-	}
+	if (!pmc->gcr_regs)
+		return -EACCES;
 
-gcr_ipc_unlock:
-	spin_unlock(&ipcdev.gcr_lock);
-	return ret;
+	return regmap_update_bits(pmc->gcr_regs, offset, mask, val);
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
 
@@ -475,8 +429,6 @@ static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	pmc->irq_mode = IPC_TRIGGER_MODE_IRQ;
 
-	spin_lock_init(&ipcdev.gcr_lock);
-
 	ret = pcim_enable_device(pdev);
 	if (ret)
 		return ret;
@@ -759,7 +711,6 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 		return PTR_ERR(addr);
 
 	ipcdev.ipc_base = addr;
-	ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
 
 	return 0;
 }
@@ -772,17 +723,26 @@ static int ipc_plat_get_res(struct platform_device *pdev)
  */
 int intel_pmc_s0ix_counter_read(u64 *data)
 {
+	struct intel_pmc_ipc_dev *pmc = &ipcdev;
 	u64 deep, shlw;
+	int ret;
 
-	if (!ipcdev.has_gcr_regs)
+	if (!pmc->gcr_regs)
 		return -EACCES;
 
-	deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);
-	shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);
+	ret = regmap_bulk_read(pmc->gcr_regs, PMC_GCR_TELEM_DEEP_S0IX_REG,
+			       &deep, 2);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(pmc->gcr_regs, PMC_GCR_TELEM_SHLW_S0IX_REG,
+			       &shlw, 2);
+	if (ret)
+		return ret;
 
 	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(intel_pmc_s0ix_counter_read);
 
@@ -801,7 +761,6 @@ static int ipc_plat_probe(struct platform_device *pdev)
 	ipcdev.dev = &pdev->dev;
 	ipcdev.irq_mode = IPC_TRIGGER_MODE_IRQ;
 	init_completion(&ipcdev.cmd_complete);
-	spin_lock_init(&ipcdev.gcr_lock);
 
 	ipcdev.irq = platform_get_irq(pdev, 0);
 	if (ipcdev.irq < 0) {
@@ -815,6 +774,15 @@ static int ipc_plat_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* GCR base address is at ipc_base + PLAT_RESOURCE_GCR_OFFSET */
+	ipcdev.gcr_regs = devm_regmap_init_mmio_clk(ipcdev.dev, NULL,
+				ipcdev.ipc_base + PLAT_RESOURCE_GCR_OFFSET,
+				&gcr_regmap_config);
+	if (IS_ERR(ipcdev.gcr_regs)) {
+		dev_err(ipcdev.dev, "gcr_regs regmap init failed\n");
+		return PTR_ERR(ipcdev.gcr_regs);
+	}
+
 	ret = ipc_create_pmc_devices(pdev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to create PMC devices\n");
@@ -836,8 +804,6 @@ static int ipc_plat_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ipcdev.has_gcr_regs = true;
-
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH v1 1/2] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices

[Andy Shevchenko]

On Fri, Jan 26, 2018 at 12:53 AM,
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Currently, we have lot of repetitive code in dependent device resource
> allocation and device creation handling code. This logic can be improved if
> we use MFD framework for dependent device creation. This patch adds this
> support.

Thanks for an update. My comments below.

> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

First of all, I barely remember what I did to this patch. In any case
this one is redundant since it will have mine when I push it to our
repo.

> @@ -508,7 +492,7 @@ static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>         ret = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_pmc_ipc",
>                                 pmc);
>         if (ret) {
> -               dev_err(&pdev->dev, "Failed to request irq\n");
> +               dev_err(&pdev->dev, "Failed to request IRQ\n");
>                 return ret;
>         }

Split this kind of changes in a separate patch.

> +static int ipc_create_punit_device(struct platform_device *pdev)
>  {
> +       static struct resource punit_res[PLAT_RESOURCE_MEM_MAX_INDEX];
> +       static struct mfd_cell punit_cell;
> +       struct resource *res;
> +       int ret, mindex, pindex = 0;
> +
> +       for (mindex = 0; mindex <= PLAT_RESOURCE_MEM_MAX_INDEX; mindex++) {

'<=' ??? (Why = is here?)

> +               res = platform_get_resource(pdev, IORESOURCE_MEM, mindex);
> +
> +               switch (mindex) {
> +               /* Get PUNIT resources */
> +               case PLAT_RESOURCE_BIOS_DATA_INDEX:
> +               case PLAT_RESOURCE_BIOS_IFACE_INDEX:

> +                       /* BIOS resources are required, so return error if not
> +                        * available
> +                        */

It's not the network subsystem, please, do a proper style for
multi-line comments.

> +                       if (!res) {

Would the following work for you?

if (res)
 break;
     dev_err(&pdev->dev, "Failed to get PUNIT MEM resource %d\n", pindex);
     return -ENXIO;
case ...:
...
if (res)
 break;
default:
 continue;

memcpy(...);
...

> +                               dev_err(&pdev->dev,
> +                                       "Failed to get PUNIT MEM resource %d\n",
> +                                       pindex);
> +                               return -ENXIO;
> +                       }
> +               case PLAT_RESOURCE_ISP_DATA_INDEX:
> +               case PLAT_RESOURCE_ISP_IFACE_INDEX:
> +               case PLAT_RESOURCE_GTD_DATA_INDEX:
> +               case PLAT_RESOURCE_GTD_IFACE_INDEX:
> +                       /* if valid resource found, copy the resource to PUNIT
> +                        * resource
> +                        */
> +                       if (res)
> +                               memcpy(&punit_res[pindex], res, sizeof(*res));
> +                       punit_res[pindex].flags = IORESOURCE_MEM;
> +                       pindex++;
> +                       break;
>                 };
> +       }

> +       ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, &punit_cell,
> +                                  1, NULL, 0, NULL);
>
> +       dev_dbg(&pdev->dev, "Successfully created PUNIT device\n");
>

Wrong. If ret is not 0 the message is misleading.
Just remove it.

Same for the rest cases.

> +       return ret;
>  }

> +static int ipc_create_wdt_device(struct platform_device *pdev)
>  {
> +       static struct resource wdt_ipc_res[2];
> +       static struct mfd_cell wdt_cell;
>         struct resource *res;
> +       int ret;
>
> +       /* If we have ACPI based watchdog use that instead, othewise create
> +        * a MFD cell for iTCO watchdog
> +        */

Style.

> +       if (acpi_has_watchdog())
> +               return 0;

> +       ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, &wdt_cell,
> +                                  1, NULL, 0, NULL);
> +
> +       dev_dbg(&pdev->dev, "Successfully created watchdog device\n");
> +
> +       return ret;

What Heikki meant is to fill cells by those helper functions and call
mfd_add_devices() only once.

See lpc_ich.c as an example.

>  }

> +static int ipc_create_pmc_devices(struct platform_device *pdev)
>  {
>         int ret;
>
> +       ret = ipc_create_punit_device(pdev);
> +       if (ret < 0)
> +               return ret;

Is it fatal? (Hint: it's quite likely not)

> +       ret = ipc_create_wdt_device(pdev);
> +       if (ret < 0)
> +               return ret;

Is it fatal?

> +       ret = ipc_create_telemetry_device(pdev);
> +       if (ret < 0)
> +               return ret;

Is it fatal?

> +       return 0;
>  }

> +       ret = devm_request_irq(&pdev->dev, ipcdev.irq, ioc, IRQF_NO_SUSPEND,
> +                              "intel_pmc_ipc", &ipcdev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to request IRQ\n");
> +               return ret;
>         }
>
>         ret = sysfs_create_group(&pdev->dev.kobj, &intel_ipc_group);
>         if (ret) {
>                 dev_err(&pdev->dev, "Failed to create sysfs group %d\n",
>                         ret);
> -               goto err_sys;

> +               devm_free_irq(&pdev->dev, ipcdev.irq, &ipcdev);

Why do you need this one?

> +               return ret;
>         }
>
>         ipcdev.has_gcr_regs = true;
>
>         return 0;
>  }

And to the main question, what this is doing in PDx86 now? There
should be a patch to move it under drivers/mfd.

In _any case_ I need an Ack from Lee.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] platform/x86: intel_pmc_ipc: Use regmap calls for GCR updates

[Andy Shevchenko]

On Fri, Jan 26, 2018 at 12:53 AM,
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> This patch adds support for regmap based implementation for GCR
> read/write/update APIs.
>

Thanks. On the first glance this one looks okay.

> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/platform/x86/Kconfig         |   1 +
>  drivers/platform/x86/intel_pmc_ipc.c | 124 +++++++++++++----------------------
>  2 files changed, 46 insertions(+), 79 deletions(-)
>
> This patch was originally part of "SCU/PMC/PUNIT IPC driver clean up" series. But I have
> split them into two sets because we need more thorough review on generic IPC driver
> design patches (second set).
>
> https://www.spinics.net/lists/linux-watchdog/msg12796.html
>
> Changes since previous set version (v8):
>  * Removed gcr_mem_base variable.
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 2c745e8..ac618fd 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1087,6 +1087,7 @@ config PVPANIC
>  config INTEL_PMC_IPC
>         tristate "Intel PMC IPC Driver"
>         depends on ACPI
> +       select REGMAP_MMIO
>         ---help---
>         This driver provides support for PMC control on some Intel platforms.
>         The PMC is an ARC processor which defines IPC commands for communication
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index fadf721..650144a 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -35,6 +35,7 @@
>  #include <linux/acpi.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/spinlock.h>
> +#include <linux/regmap.h>
>
>  #include <asm/intel_pmc_ipc.h>
>
> @@ -125,9 +126,7 @@ static struct intel_pmc_ipc_dev {
>         struct completion cmd_complete;
>
>         /* gcr */
> -       void __iomem *gcr_mem_base;
> -       bool has_gcr_regs;
> -       spinlock_t gcr_lock;
> +       struct regmap *gcr_regs;
>  } ipcdev;
>
>  static char *ipc_err_sources[] = {
> @@ -149,6 +148,15 @@ static char *ipc_err_sources[] = {
>                 "Unsigned kernel",
>  };
>
> +static struct regmap_config gcr_regmap_config = {
> +       .name = "intel_pmc_gcr",
> +       .reg_bits = 32,
> +       .reg_stride = 4,
> +       .val_bits = 32,
> +       .fast_io = true,
> +       .max_register = PLAT_RESOURCE_GCR_SIZE,
> +};
> +
>  /* Prevent concurrent calls to the PMC */
>  static DEFINE_MUTEX(ipclock);
>
> @@ -182,21 +190,6 @@ static inline u32 ipc_data_readl(u32 offset)
>         return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
>  }
>
> -static inline u64 gcr_data_readq(u32 offset)
> -{
> -       return readq(ipcdev.gcr_mem_base + offset);
> -}
> -
> -static inline int is_gcr_valid(u32 offset)
> -{
> -       if (!ipcdev.has_gcr_regs)
> -               return -EACCES;
> -
> -       if (offset > PLAT_RESOURCE_GCR_SIZE)
> -               return -EINVAL;
> -
> -       return 0;
> -}
>
>  /**
>   * intel_pmc_gcr_read() - Read PMC GCR register
> @@ -209,21 +202,12 @@ static inline int is_gcr_valid(u32 offset)
>   */
>  int intel_pmc_gcr_read(u32 offset, u32 *data)
>  {
> -       int ret;
> -
> -       spin_lock(&ipcdev.gcr_lock);
> -
> -       ret = is_gcr_valid(offset);
> -       if (ret < 0) {
> -               spin_unlock(&ipcdev.gcr_lock);
> -               return ret;
> -       }
> -
> -       *data = readl(ipcdev.gcr_mem_base + offset);
> +       struct intel_pmc_ipc_dev *pmc = &ipcdev;
>
> -       spin_unlock(&ipcdev.gcr_lock);
> +       if (!pmc->gcr_regs)
> +               return -EACCES;
>
> -       return 0;
> +       return regmap_read(pmc->gcr_regs, offset, data);
>  }
>  EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
>
> @@ -239,21 +223,12 @@ EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
>   */
>  int intel_pmc_gcr_write(u32 offset, u32 data)
>  {
> -       int ret;
> -
> -       spin_lock(&ipcdev.gcr_lock);
> -
> -       ret = is_gcr_valid(offset);
> -       if (ret < 0) {
> -               spin_unlock(&ipcdev.gcr_lock);
> -               return ret;
> -       }
> -
> -       writel(data, ipcdev.gcr_mem_base + offset);
> +       struct intel_pmc_ipc_dev *pmc = &ipcdev;
>
> -       spin_unlock(&ipcdev.gcr_lock);
> +       if (!pmc->gcr_regs)
> +               return -EACCES;
>
> -       return 0;
> +       return regmap_write(pmc->gcr_regs, offset, data);
>  }
>  EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
>
> @@ -270,33 +245,12 @@ EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
>   */
>  int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
>  {
> -       u32 new_val;
> -       int ret = 0;
> -
> -       spin_lock(&ipcdev.gcr_lock);
> -
> -       ret = is_gcr_valid(offset);
> -       if (ret < 0)
> -               goto gcr_ipc_unlock;
> -
> -       new_val = readl(ipcdev.gcr_mem_base + offset);
> -
> -       new_val &= ~mask;
> -       new_val |= val & mask;
> -
> -       writel(new_val, ipcdev.gcr_mem_base + offset);
> -
> -       new_val = readl(ipcdev.gcr_mem_base + offset);
> +       struct intel_pmc_ipc_dev *pmc = &ipcdev;
>
> -       /* check whether the bit update is successful */
> -       if ((new_val & mask) != (val & mask)) {
> -               ret = -EIO;
> -               goto gcr_ipc_unlock;
> -       }
> +       if (!pmc->gcr_regs)
> +               return -EACCES;
>
> -gcr_ipc_unlock:
> -       spin_unlock(&ipcdev.gcr_lock);
> -       return ret;
> +       return regmap_update_bits(pmc->gcr_regs, offset, mask, val);
>  }
>  EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
>
> @@ -475,8 +429,6 @@ static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>
>         pmc->irq_mode = IPC_TRIGGER_MODE_IRQ;
>
> -       spin_lock_init(&ipcdev.gcr_lock);
> -
>         ret = pcim_enable_device(pdev);
>         if (ret)
>                 return ret;
> @@ -759,7 +711,6 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>                 return PTR_ERR(addr);
>
>         ipcdev.ipc_base = addr;
> -       ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
>
>         return 0;
>  }
> @@ -772,17 +723,26 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>   */
>  int intel_pmc_s0ix_counter_read(u64 *data)
>  {
> +       struct intel_pmc_ipc_dev *pmc = &ipcdev;
>         u64 deep, shlw;
> +       int ret;
>
> -       if (!ipcdev.has_gcr_regs)
> +       if (!pmc->gcr_regs)
>                 return -EACCES;
>
> -       deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);
> -       shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);
> +       ret = regmap_bulk_read(pmc->gcr_regs, PMC_GCR_TELEM_DEEP_S0IX_REG,
> +                              &deep, 2);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_bulk_read(pmc->gcr_regs, PMC_GCR_TELEM_SHLW_S0IX_REG,
> +                              &shlw, 2);
> +       if (ret)
> +               return ret;
>
>         *data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
>
> -       return 0;
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(intel_pmc_s0ix_counter_read);
>
> @@ -801,7 +761,6 @@ static int ipc_plat_probe(struct platform_device *pdev)
>         ipcdev.dev = &pdev->dev;
>         ipcdev.irq_mode = IPC_TRIGGER_MODE_IRQ;
>         init_completion(&ipcdev.cmd_complete);
> -       spin_lock_init(&ipcdev.gcr_lock);
>
>         ipcdev.irq = platform_get_irq(pdev, 0);
>         if (ipcdev.irq < 0) {
> @@ -815,6 +774,15 @@ static int ipc_plat_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       /* GCR base address is at ipc_base + PLAT_RESOURCE_GCR_OFFSET */
> +       ipcdev.gcr_regs = devm_regmap_init_mmio_clk(ipcdev.dev, NULL,
> +                               ipcdev.ipc_base + PLAT_RESOURCE_GCR_OFFSET,
> +                               &gcr_regmap_config);
> +       if (IS_ERR(ipcdev.gcr_regs)) {
> +               dev_err(ipcdev.dev, "gcr_regs regmap init failed\n");
> +               return PTR_ERR(ipcdev.gcr_regs);
> +       }
> +
>         ret = ipc_create_pmc_devices(pdev);
>         if (ret) {
>                 dev_err(&pdev->dev, "Failed to create PMC devices\n");
> @@ -836,8 +804,6 @@ static int ipc_plat_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> -       ipcdev.has_gcr_regs = true;
> -
>         return 0;
>  }
>
> --
> 2.7.4
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices

[sathya]

Hi Andy,

Thanks for the review.

On 01/26/2018 08:17 AM, Andy Shevchenko wrote:
> On Fri, Jan 26, 2018 at 12:53 AM,
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> Currently, we have lot of repetitive code in dependent device resource
>> allocation and device creation handling code. This logic can be improved if
>> we use MFD framework for dependent device creation. This patch adds this
>> support.
> Thanks for an update. My comments below.
>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> First of all, I barely remember what I did to this patch.
Sorry, I know I took a long break. But its over now. I will be active in 
coming months.
> In any case
> this one is redundant since it will have mine when I push it to our
> repo.
>
>> @@ -508,7 +492,7 @@ static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>          ret = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_pmc_ipc",
>>                                  pmc);
>>          if (ret) {
>> -               dev_err(&pdev->dev, "Failed to request irq\n");
>> +               dev_err(&pdev->dev, "Failed to request IRQ\n");
Will split these renames into a separate patch.
>>                  return ret;
>>          }
> Split this kind of changes in a separate patch.
>
>> +static int ipc_create_punit_device(struct platform_device *pdev)
>>   {
>> +       static struct resource punit_res[PLAT_RESOURCE_MEM_MAX_INDEX];
>> +       static struct mfd_cell punit_cell;
>> +       struct resource *res;
>> +       int ret, mindex, pindex = 0;
>> +
>> +       for (mindex = 0; mindex <= PLAT_RESOURCE_MEM_MAX_INDEX; mindex++) {
> '<=' ??? (Why = is here?)
Good catch. It should be only <. I will fix it in next release.
>
>> +               res = platform_get_resource(pdev, IORESOURCE_MEM, mindex);
>> +
>> +               switch (mindex) {
>> +               /* Get PUNIT resources */
>> +               case PLAT_RESOURCE_BIOS_DATA_INDEX:
>> +               case PLAT_RESOURCE_BIOS_IFACE_INDEX:
>> +                       /* BIOS resources are required, so return error if not
>> +                        * available
>> +                        */
> It's not the network subsystem, please, do a proper style for
> multi-line comments.
Will fix it in next release.
>
>> +                       if (!res) {
> Would the following work for you?
>
> if (res)
>   break;
>       dev_err(&pdev->dev, "Failed to get PUNIT MEM resource %d\n", pindex);
>       return -ENXIO;
> case ...:
> ...
> if (res)
>   break;
> default:
>   continue;
>
> memcpy(...);
> ...
If you move memcpy outside the switch statement, then it will be called 
for cases (non punit cases) like PLAT_RESOURCE_TELEM_SSRAM_INDEX or 
PLAT_RESOURCE_ACPI_IO_INDEX which is logically incorrect.
>
>> +                               dev_err(&pdev->dev,
>> +                                       "Failed to get PUNIT MEM resource %d\n",
>> +                                       pindex);
>> +                               return -ENXIO;
>> +                       }
>> +               case PLAT_RESOURCE_ISP_DATA_INDEX:
>> +               case PLAT_RESOURCE_ISP_IFACE_INDEX:
>> +               case PLAT_RESOURCE_GTD_DATA_INDEX:
>> +               case PLAT_RESOURCE_GTD_IFACE_INDEX:
>> +                       /* if valid resource found, copy the resource to PUNIT
>> +                        * resource
>> +                        */
>> +                       if (res)
>> +                               memcpy(&punit_res[pindex], res, sizeof(*res));
>> +                       punit_res[pindex].flags = IORESOURCE_MEM;
>> +                       pindex++;
>> +                       break;
>>                  };
>> +       }
>> +       ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, &punit_cell,
>> +                                  1, NULL, 0, NULL);
>>
>> +       dev_dbg(&pdev->dev, "Successfully created PUNIT device\n");
>>
> Wrong. If ret is not 0 the message is misleading.
> Just remove it.
>
> Same for the rest cases.
I will remove it.
>
>> +       return ret;
>>   }
>> +static int ipc_create_wdt_device(struct platform_device *pdev)
>>   {
>> +       static struct resource wdt_ipc_res[2];
>> +       static struct mfd_cell wdt_cell;
>>          struct resource *res;
>> +       int ret;
>>
>> +       /* If we have ACPI based watchdog use that instead, othewise create
>> +        * a MFD cell for iTCO watchdog
>> +        */
> Style.
Got it. I will be fixed in next version.
>
>> +       if (acpi_has_watchdog())
>> +               return 0;
>> +       ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, &wdt_cell,
>> +                                  1, NULL, 0, NULL);
>> +
>> +       dev_dbg(&pdev->dev, "Successfully created watchdog device\n");
>> +
>> +       return ret;
> What Heikki meant is to fill cells by those helper functions and call
> mfd_add_devices() only once.
Ok. It will be fixed in next version.

I could not find the actual BUG reported by Heikki. So I did not 
understand the reason behind his proposal.
>
> See lpc_ich.c as an example.
>
>>   }
>> +static int ipc_create_pmc_devices(struct platform_device *pdev)
>>   {
>>          int ret;
>>
>> +       ret = ipc_create_punit_device(pdev);
>> +       if (ret < 0)
>> +               return ret;
> Is it fatal? (Hint: it's quite likely not)
Logically not. But this logic exist in originally driver. I did not want 
to change the behavior without knowing the full details. Let me know 
your opinion.
>
>> +       ret = ipc_create_wdt_device(pdev);
>> +       if (ret < 0)
>> +               return ret;
> Is it fatal?
Same as above.
>
>> +       ret = ipc_create_telemetry_device(pdev);
>> +       if (ret < 0)
>> +               return ret;
> Is it fatal?
Same as above.
>
>> +       return 0;
>>   }
>> +       ret = devm_request_irq(&pdev->dev, ipcdev.irq, ioc, IRQF_NO_SUSPEND,
>> +                              "intel_pmc_ipc", &ipcdev);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "Failed to request IRQ\n");
>> +               return ret;
>>          }
>>
>>          ret = sysfs_create_group(&pdev->dev.kobj, &intel_ipc_group);
>>          if (ret) {
>>                  dev_err(&pdev->dev, "Failed to create sysfs group %d\n",
>>                          ret);
>> -               goto err_sys;
>> +               devm_free_irq(&pdev->dev, ipcdev.irq, &ipcdev);
> Why do you need this one?
This was added by you in one of the previous submissions.

I think you added it because we have a separate device remove function 
in this driver, and not explicitly freeing IRQ could mess up the 
resource cleanup order.
>
>> +               return ret;
>>          }
>>
>>          ipcdev.has_gcr_regs = true;
>>
>>          return 0;
>>   }
> And to the main question, what this is doing in PDx86 now? There
> should be a patch to move it under drivers/mfd.
Drivers like drivers/platform/chrome/cros_ec_dev.c already use MFD calls 
outside MFD framework. I am not sure what is the norm.

If you agree with the move, I will submit a patch for it.
>
> In _any case_ I need an Ack from Lee.
>