[PATCH v7 1/6] platform/x86: intel_pmc_ipc: fix gcr offset

[PATCH v7 1/6] platform/x86: intel_pmc_ipc: fix gcr offset

[Kuppuswamy Sathyanarayanan]

According to Broxton APL spec, PMC MIMO resources for Global Control
Registers(GCR) are located at 4K(0x1000) offset from IPC base address.
In this driver, PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
region base address from IPC base address and its current value of
0x1008 is incorrect because it points to location for PMC_CFG register
and not the GCR base address itself.

GCR Base = IPC1 Base + 0x1000.

This patch fixes this offset issue.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_ipc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Changes since v6:
 *  Updated commit message

Changes since v5:
 *  None

Changes since v4:
 *  None

Changes since v3:
 * Updated the commit history

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 0651d47..0a33592 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -82,7 +82,7 @@
 /* exported resources from IFWI */
 #define PLAT_RESOURCE_IPC_INDEX		0
 #define PLAT_RESOURCE_IPC_SIZE		0x1000
-#define PLAT_RESOURCE_GCR_OFFSET	0x1008
+#define PLAT_RESOURCE_GCR_OFFSET	0x1000
 #define PLAT_RESOURCE_GCR_SIZE		0x1000
 #define PLAT_RESOURCE_BIOS_DATA_INDEX	1
 #define PLAT_RESOURCE_BIOS_IFACE_INDEX	2
-- 
2.7.4

[PATCH v7 1/6] platform/x86: intel_pmc_ipc: fix gcr offset

[Kuppuswamy Sathyanarayanan]

According to Broxton APL spec, PMC MIMO resources for Global Control
Registers(GCR) are located at 4K(0x1000) offset from IPC base address.
In this driver, PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
region base address from IPC base address and its current value of
0x1008 is incorrect because it points to location for PMC_CFG register
and not the GCR base address itself.

GCR Base = IPC1 Base + 0x1000.

This patch fixes this offset issue.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_ipc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Changes since v6:
 *  Updated commit message

Changes since v5:
 *  None

Changes since v4:
 *  None

Changes since v3:
 * Updated the commit history

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 0651d47..0a33592 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -82,7 +82,7 @@
 /* exported resources from IFWI */
 #define PLAT_RESOURCE_IPC_INDEX		0
 #define PLAT_RESOURCE_IPC_SIZE		0x1000
-#define PLAT_RESOURCE_GCR_OFFSET	0x1008
+#define PLAT_RESOURCE_GCR_OFFSET	0x1000
 #define PLAT_RESOURCE_GCR_SIZE		0x1000
 #define PLAT_RESOURCE_BIOS_DATA_INDEX	1
 #define PLAT_RESOURCE_BIOS_IFACE_INDEX	2
-- 
2.7.4

[PATCH v7 2/6] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's

[Kuppuswamy Sathyanarayanan]

This patch adds API's to read/write/update PMC GC registers.
PMC dependent devices like iTCO_wdt, Telemetry has requirement
to acces GCR registers. These API's can be used for this
purpose.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/include/asm/intel_pmc_ipc.h |  21 +++++++
 drivers/platform/x86/intel_pmc_ipc.c | 115 +++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+)

Changes since v6:
 * None

Changes since v5:
 * Removed redundant temp variable in intel_pmc_gcr_update().
 * Changed label name from "gcr_update_err" to "gcr_ipc_unlock"
 * Fixed some style issues.

Changes since v4:
 * Fixed style issue in commit history
 * Added mutex locks in read/write/update API's.

Changes since v3:
 * Added usage comments for read/write/update api
 * Created a helper function to handle GCR related range checks.

Changes since v2:
 * Removed unused reg offset from header file.
 * Modified read/write api's signatures for better error handling
 * Added function for bit level update of gcr register.

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
index 4291b6a..8402efe 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -23,6 +23,9 @@
 #define IPC_ERR_EMSECURITY		6
 #define IPC_ERR_UNSIGNEDKERNEL		7
 
+/* GCR reg offsets from gcr base*/
+#define PMC_GCR_PMC_CFG_REG		0x08
+
 #if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
 
 int intel_pmc_ipc_simple_command(int cmd, int sub);
@@ -31,6 +34,9 @@ int intel_pmc_ipc_raw_cmd(u32 cmd, u32 sub, u8 *in, u32 inlen,
 int intel_pmc_ipc_command(u32 cmd, u32 sub, u8 *in, u32 inlen,
 		u32 *out, u32 outlen);
 int intel_pmc_s0ix_counter_read(u64 *data);
+int intel_pmc_gcr_read(u32 offset, u32 *data);
+int intel_pmc_gcr_write(u32 offset, u32 data);
+int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val);
 
 #else
 
@@ -56,6 +62,21 @@ static inline int intel_pmc_s0ix_counter_read(u64 *data)
 	return -EINVAL;
 }
 
+static inline int intel_pmc_gcr_read(u32 offset, u32 *data)
+{
+	return -EINVAL;
+}
+
+static inline int intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	return -EINVAL;
+}
+
+static inline int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
+{
+	return -EINVAL;
+}
+
 #endif /*CONFIG_INTEL_PMC_IPC*/
 
 #endif
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 0a33592..a0c773b 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -128,6 +128,7 @@ static struct intel_pmc_ipc_dev {
 	/* gcr */
 	resource_size_t gcr_base;
 	int gcr_size;
+	void __iomem *gcr_mem_base;
 	bool has_gcr_regs;
 
 	/* punit */
@@ -199,6 +200,119 @@ static inline u64 gcr_data_readq(u32 offset)
 	return readq(ipcdev.ipc_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
+ * @offset:	offset of GCR register from GCR address base
+ * @data:	data pointer for storing the register output
+ *
+ * Reads the PMC GCR register of given offset.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_read(u32 offset, u32 *data)
+{
+	int ret;
+
+	mutex_lock(&ipclock);
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0) {
+		mutex_unlock(&ipclock);
+		return ret;
+	}
+
+	*data = readl(ipcdev.gcr_mem_base + offset);
+
+	mutex_unlock(&ipclock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
+
+/**
+ * intel_pmc_gcr_write() - Write PMC GCR register
+ * @offset:	offset of GCR register from GCR address base
+ * @data:	register update value
+ *
+ * Writes the PMC GCR register of given offset with given
+ * value.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	int ret;
+
+	mutex_lock(&ipclock);
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0) {
+		mutex_unlock(&ipclock);
+		return ret;
+	}
+
+	writel(data, ipcdev.gcr_mem_base + offset);
+
+	mutex_unlock(&ipclock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
+
+/**
+ * intel_pmc_gcr_update() - Update PMC GCR register bits
+ * @offset:	offset of GCR register from GCR address base
+ * @mask:	bit mask for update operation
+ * @val:	update value
+ *
+ * Updates the bits of given GCR register as specified by
+ * @mask and @val.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
+{
+	u32 new_val;
+	int ret = 0;
+
+	mutex_lock(&ipclock);
+
+	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);
+
+	/* check whether the bit update is successful */
+	if ((new_val & mask) != (val & mask)) {
+		ret = -EIO;
+		goto gcr_ipc_unlock;
+	}
+
+gcr_ipc_unlock:
+	mutex_unlock(&ipclock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -747,6 +861,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 	ipcdev.ipc_base = addr;
 
 	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
+	ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
 	dev_info(&pdev->dev, "ipc res: %pR\n", res);
 
-- 
2.7.4

[PATCH v7 2/6] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's

[Kuppuswamy Sathyanarayanan]

This patch adds API's to read/write/update PMC GC registers.
PMC dependent devices like iTCO_wdt, Telemetry has requirement
to acces GCR registers. These API's can be used for this
purpose.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 arch/x86/include/asm/intel_pmc_ipc.h |  21 +++++++
 drivers/platform/x86/intel_pmc_ipc.c | 115 +++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+)

Changes since v6:
 * None

Changes since v5:
 * Removed redundant temp variable in intel_pmc_gcr_update().
 * Changed label name from "gcr_update_err" to "gcr_ipc_unlock"
 * Fixed some style issues.

Changes since v4:
 * Fixed style issue in commit history
 * Added mutex locks in read/write/update API's.

Changes since v3:
 * Added usage comments for read/write/update api
 * Created a helper function to handle GCR related range checks.

Changes since v2:
 * Removed unused reg offset from header file.
 * Modified read/write api's signatures for better error handling
 * Added function for bit level update of gcr register.

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
index 4291b6a..8402efe 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -23,6 +23,9 @@
 #define IPC_ERR_EMSECURITY		6
 #define IPC_ERR_UNSIGNEDKERNEL		7
 
+/* GCR reg offsets from gcr base*/
+#define PMC_GCR_PMC_CFG_REG		0x08
+
 #if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
 
 int intel_pmc_ipc_simple_command(int cmd, int sub);
@@ -31,6 +34,9 @@ int intel_pmc_ipc_raw_cmd(u32 cmd, u32 sub, u8 *in, u32 inlen,
 int intel_pmc_ipc_command(u32 cmd, u32 sub, u8 *in, u32 inlen,
 		u32 *out, u32 outlen);
 int intel_pmc_s0ix_counter_read(u64 *data);
+int intel_pmc_gcr_read(u32 offset, u32 *data);
+int intel_pmc_gcr_write(u32 offset, u32 data);
+int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val);
 
 #else
 
@@ -56,6 +62,21 @@ static inline int intel_pmc_s0ix_counter_read(u64 *data)
 	return -EINVAL;
 }
 
+static inline int intel_pmc_gcr_read(u32 offset, u32 *data)
+{
+	return -EINVAL;
+}
+
+static inline int intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	return -EINVAL;
+}
+
+static inline int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
+{
+	return -EINVAL;
+}
+
 #endif /*CONFIG_INTEL_PMC_IPC*/
 
 #endif
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 0a33592..a0c773b 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -128,6 +128,7 @@ static struct intel_pmc_ipc_dev {
 	/* gcr */
 	resource_size_t gcr_base;
 	int gcr_size;
+	void __iomem *gcr_mem_base;
 	bool has_gcr_regs;
 
 	/* punit */
@@ -199,6 +200,119 @@ static inline u64 gcr_data_readq(u32 offset)
 	return readq(ipcdev.ipc_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
+ * @offset:	offset of GCR register from GCR address base
+ * @data:	data pointer for storing the register output
+ *
+ * Reads the PMC GCR register of given offset.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_read(u32 offset, u32 *data)
+{
+	int ret;
+
+	mutex_lock(&ipclock);
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0) {
+		mutex_unlock(&ipclock);
+		return ret;
+	}
+
+	*data = readl(ipcdev.gcr_mem_base + offset);
+
+	mutex_unlock(&ipclock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
+
+/**
+ * intel_pmc_gcr_write() - Write PMC GCR register
+ * @offset:	offset of GCR register from GCR address base
+ * @data:	register update value
+ *
+ * Writes the PMC GCR register of given offset with given
+ * value.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	int ret;
+
+	mutex_lock(&ipclock);
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0) {
+		mutex_unlock(&ipclock);
+		return ret;
+	}
+
+	writel(data, ipcdev.gcr_mem_base + offset);
+
+	mutex_unlock(&ipclock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
+
+/**
+ * intel_pmc_gcr_update() - Update PMC GCR register bits
+ * @offset:	offset of GCR register from GCR address base
+ * @mask:	bit mask for update operation
+ * @val:	update value
+ *
+ * Updates the bits of given GCR register as specified by
+ * @mask and @val.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
+{
+	u32 new_val;
+	int ret = 0;
+
+	mutex_lock(&ipclock);
+
+	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);
+
+	/* check whether the bit update is successful */
+	if ((new_val & mask) != (val & mask)) {
+		ret = -EIO;
+		goto gcr_ipc_unlock;
+	}
+
+gcr_ipc_unlock:
+	mutex_unlock(&ipclock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -747,6 +861,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 	ipcdev.ipc_base = addr;
 
 	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
+	ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
 	dev_info(&pdev->dev, "ipc res: %pR\n", res);
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 2/6] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's

[Kuppuswamy Sathyanarayanan]

This patch adds API's to read/write/update PMC GC registers.
PMC dependent devices like iTCO_wdt, Telemetry has requirement
to acces GCR registers. These API's can be used for this
purpose.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/include/asm/intel_pmc_ipc.h |  21 +++++++
 drivers/platform/x86/intel_pmc_ipc.c | 115 +++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+)

Changes since v6:
 * None

Changes since v5:
 * Removed redundant temp variable in intel_pmc_gcr_update().
 * Changed label name from "gcr_update_err" to "gcr_ipc_unlock"
 * Fixed some style issues.

Changes since v4:
 * Fixed style issue in commit history
 * Added mutex locks in read/write/update API's.

Changes since v3:
 * Added usage comments for read/write/update api
 * Created a helper function to handle GCR related range checks.

Changes since v2:
 * Removed unused reg offset from header file.
 * Modified read/write api's signatures for better error handling
 * Added function for bit level update of gcr register.

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
index 4291b6a..8402efe 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -23,6 +23,9 @@
 #define IPC_ERR_EMSECURITY		6
 #define IPC_ERR_UNSIGNEDKERNEL		7
 
+/* GCR reg offsets from gcr base*/
+#define PMC_GCR_PMC_CFG_REG		0x08
+
 #if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
 
 int intel_pmc_ipc_simple_command(int cmd, int sub);
@@ -31,6 +34,9 @@ int intel_pmc_ipc_raw_cmd(u32 cmd, u32 sub, u8 *in, u32 inlen,
 int intel_pmc_ipc_command(u32 cmd, u32 sub, u8 *in, u32 inlen,
 		u32 *out, u32 outlen);
 int intel_pmc_s0ix_counter_read(u64 *data);
+int intel_pmc_gcr_read(u32 offset, u32 *data);
+int intel_pmc_gcr_write(u32 offset, u32 data);
+int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val);
 
 #else
 
@@ -56,6 +62,21 @@ static inline int intel_pmc_s0ix_counter_read(u64 *data)
 	return -EINVAL;
 }
 
+static inline int intel_pmc_gcr_read(u32 offset, u32 *data)
+{
+	return -EINVAL;
+}
+
+static inline int intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	return -EINVAL;
+}
+
+static inline int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
+{
+	return -EINVAL;
+}
+
 #endif /*CONFIG_INTEL_PMC_IPC*/
 
 #endif
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 0a33592..a0c773b 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -128,6 +128,7 @@ static struct intel_pmc_ipc_dev {
 	/* gcr */
 	resource_size_t gcr_base;
 	int gcr_size;
+	void __iomem *gcr_mem_base;
 	bool has_gcr_regs;
 
 	/* punit */
@@ -199,6 +200,119 @@ static inline u64 gcr_data_readq(u32 offset)
 	return readq(ipcdev.ipc_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
+ * @offset:	offset of GCR register from GCR address base
+ * @data:	data pointer for storing the register output
+ *
+ * Reads the PMC GCR register of given offset.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_read(u32 offset, u32 *data)
+{
+	int ret;
+
+	mutex_lock(&ipclock);
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0) {
+		mutex_unlock(&ipclock);
+		return ret;
+	}
+
+	*data = readl(ipcdev.gcr_mem_base + offset);
+
+	mutex_unlock(&ipclock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
+
+/**
+ * intel_pmc_gcr_write() - Write PMC GCR register
+ * @offset:	offset of GCR register from GCR address base
+ * @data:	register update value
+ *
+ * Writes the PMC GCR register of given offset with given
+ * value.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	int ret;
+
+	mutex_lock(&ipclock);
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0) {
+		mutex_unlock(&ipclock);
+		return ret;
+	}
+
+	writel(data, ipcdev.gcr_mem_base + offset);
+
+	mutex_unlock(&ipclock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
+
+/**
+ * intel_pmc_gcr_update() - Update PMC GCR register bits
+ * @offset:	offset of GCR register from GCR address base
+ * @mask:	bit mask for update operation
+ * @val:	update value
+ *
+ * Updates the bits of given GCR register as specified by
+ * @mask and @val.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
+{
+	u32 new_val;
+	int ret = 0;
+
+	mutex_lock(&ipclock);
+
+	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);
+
+	/* check whether the bit update is successful */
+	if ((new_val & mask) != (val & mask)) {
+		ret = -EIO;
+		goto gcr_ipc_unlock;
+	}
+
+gcr_ipc_unlock:
+	mutex_unlock(&ipclock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -747,6 +861,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 	ipcdev.ipc_base = addr;
 
 	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
+	ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
 	dev_info(&pdev->dev, "ipc res: %pR\n", res);
 
-- 
2.7.4

[PATCH v7 3/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot_bit functions

[Kuppuswamy Sathyanarayanan]

iTCO_wdt no_reboot_bit set/unset functions has lot of common code between
them. So merging these two functions into a single update function would
remove these unnecessary code duplications. This patch fixes this issue
by creating a no_reboot_bit update function to handle both set/unset
functions.

Also checking for iTCO version every time you make no_reboot_bit set/unset
call is inefficient and makes the code look complex. This can be improved
by performing this check once during device probe and selecting the
appropriate no_reboot_bit update function. This patch fixes this issue
by splitting the update function into multiple helper functions.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/watchdog/iTCO_wdt.c | 83 +++++++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 33 deletions(-)

Changes since v6:
 * make iTCO_wdt_no_reboot_bit_setup() static
 * remove inline specifier for update_no_reboot_bit helper functions.

Changes since v5:
 * Changed update function argument name from "status" to "set".
 * Fixed commit history.
 * Changed update function name to use "bit" instead of "flag"
 * Addressed Andy's comment on creating multiple helper functions.

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3d0abc0..a629933 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -106,6 +106,8 @@ struct iTCO_wdt_private {
 	struct pci_dev *pci_dev;
 	/* whether or not the watchdog has been suspended */
 	bool suspended;
+	/* no reboot update function pointer */
+	int (*update_no_reboot_bit)(void *p, bool set);
 };
 
 /* module parameters */
@@ -170,48 +172,61 @@ static inline u32 no_reboot_bit(struct iTCO_wdt_private *p)
 	return enable_bit;
 }
 
-static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
+static int update_no_reboot_bit_def(void *priv, bool set)
 {
-	u32 val32;
-
-	/* Set the NO_REBOOT bit: this disables reboots */
-	if (p->iTCO_version >= 2) {
-		val32 = readl(p->gcs_pmc);
-		val32 |= no_reboot_bit(p);
-		writel(val32, p->gcs_pmc);
-	} else if (p->iTCO_version == 1) {
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-		val32 |= no_reboot_bit(p);
-		pci_write_config_dword(p->pci_dev, 0xd4, val32);
-	}
+	return 0;
 }
 
-static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
+static int update_no_reboot_bit_pci(void *priv, bool set)
 {
-	u32 enable_bit = no_reboot_bit(p);
-	u32 val32 = 0;
+	struct iTCO_wdt_private *p = priv;
+	u32 val32 = 0, newval32 = 0;
 
-	/* Unset the NO_REBOOT bit: this enables reboots */
-	if (p->iTCO_version >= 2) {
-		val32 = readl(p->gcs_pmc);
-		val32 &= ~enable_bit;
-		writel(val32, p->gcs_pmc);
+	pci_read_config_dword(p->pci_dev, 0xd4, &val32);
+	if (set)
+		val32 |= no_reboot_bit(p);
+	else
+		val32 &= ~no_reboot_bit(p);
+	pci_write_config_dword(p->pci_dev, 0xd4, val32);
+	pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
 
-		val32 = readl(p->gcs_pmc);
-	} else if (p->iTCO_version == 1) {
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-		val32 &= ~enable_bit;
-		pci_write_config_dword(p->pci_dev, 0xd4, val32);
+	/* make sure the update is successful */
+	if (val32 != newval32)
+		return -EIO;
 
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-	}
+	return 0;
+}
+
+static int update_no_reboot_bit_mem(void *priv, bool set)
+{
+	struct iTCO_wdt_private *p = priv;
+	u32 val32 = 0, newval32 = 0;
+
+	val32 = readl(p->gcs_pmc);
+	if (set)
+		val32 |= no_reboot_bit(p);
+	else
+		val32 &= ~no_reboot_bit(p);
+	writel(val32, p->gcs_pmc);
+	newval32 = readl(p->gcs_pmc);
 
-	if (val32 & enable_bit)
+	/* make sure the update is successful */
+	if (val32 != newval32)
 		return -EIO;
 
 	return 0;
 }
 
+static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
+{
+	if (p->iTCO_version >= 2)
+		p->update_no_reboot_bit = update_no_reboot_bit_mem;
+	else if (p->iTCO_version == 1)
+		p->update_no_reboot_bit = update_no_reboot_bit_pci;
+	else
+		p->update_no_reboot_bit = update_no_reboot_bit_def;
+}
+
 static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 {
 	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
@@ -222,7 +237,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
 
 	/* disable chipset's NO_REBOOT bit */
-	if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
+	if (p->update_no_reboot_bit(p, false)) {
 		spin_unlock(&p->io_lock);
 		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
 		return -EIO;
@@ -263,7 +278,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
 	val = inw(TCO1_CNT(p));
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	iTCO_wdt_set_NO_REBOOT_bit(p);
+	p->update_no_reboot_bit(p, true);
 
 	spin_unlock(&p->io_lock);
 
@@ -428,6 +443,8 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	p->iTCO_version = pdata->version;
 	p->pci_dev = to_pci_dev(dev->parent);
 
+	iTCO_wdt_no_reboot_bit_setup(p);
+
 	/*
 	 * Get the Memory-Mapped GCS or PMC register, we need it for the
 	 * NO_REBOOT flag (TCO v2 and v3).
@@ -442,14 +459,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	}
 
 	/* Check chipset's NO_REBOOT bit */
-	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
+	if (p->update_no_reboot_bit(p, false) &&
 	    iTCO_vendor_check_noreboot_on()) {
 		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
 		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
 	}
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	iTCO_wdt_set_NO_REBOOT_bit(p);
+	p->update_no_reboot_bit(p, true);
 
 	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
 	if (!devm_request_region(dev, p->smi_res->start,
-- 
2.7.4

[PATCH v7 3/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot_bit functions

[Kuppuswamy Sathyanarayanan]

iTCO_wdt no_reboot_bit set/unset functions has lot of common code between
them. So merging these two functions into a single update function would
remove these unnecessary code duplications. This patch fixes this issue
by creating a no_reboot_bit update function to handle both set/unset
functions.

Also checking for iTCO version every time you make no_reboot_bit set/unset
call is inefficient and makes the code look complex. This can be improved
by performing this check once during device probe and selecting the
appropriate no_reboot_bit update function. This patch fixes this issue
by splitting the update function into multiple helper functions.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/watchdog/iTCO_wdt.c | 83 +++++++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 33 deletions(-)

Changes since v6:
 * make iTCO_wdt_no_reboot_bit_setup() static
 * remove inline specifier for update_no_reboot_bit helper functions.

Changes since v5:
 * Changed update function argument name from "status" to "set".
 * Fixed commit history.
 * Changed update function name to use "bit" instead of "flag"
 * Addressed Andy's comment on creating multiple helper functions.

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3d0abc0..a629933 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -106,6 +106,8 @@ struct iTCO_wdt_private {
 	struct pci_dev *pci_dev;
 	/* whether or not the watchdog has been suspended */
 	bool suspended;
+	/* no reboot update function pointer */
+	int (*update_no_reboot_bit)(void *p, bool set);
 };
 
 /* module parameters */
@@ -170,48 +172,61 @@ static inline u32 no_reboot_bit(struct iTCO_wdt_private *p)
 	return enable_bit;
 }
 
-static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
+static int update_no_reboot_bit_def(void *priv, bool set)
 {
-	u32 val32;
-
-	/* Set the NO_REBOOT bit: this disables reboots */
-	if (p->iTCO_version >= 2) {
-		val32 = readl(p->gcs_pmc);
-		val32 |= no_reboot_bit(p);
-		writel(val32, p->gcs_pmc);
-	} else if (p->iTCO_version == 1) {
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-		val32 |= no_reboot_bit(p);
-		pci_write_config_dword(p->pci_dev, 0xd4, val32);
-	}
+	return 0;
 }
 
-static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
+static int update_no_reboot_bit_pci(void *priv, bool set)
 {
-	u32 enable_bit = no_reboot_bit(p);
-	u32 val32 = 0;
+	struct iTCO_wdt_private *p = priv;
+	u32 val32 = 0, newval32 = 0;
 
-	/* Unset the NO_REBOOT bit: this enables reboots */
-	if (p->iTCO_version >= 2) {
-		val32 = readl(p->gcs_pmc);
-		val32 &= ~enable_bit;
-		writel(val32, p->gcs_pmc);
+	pci_read_config_dword(p->pci_dev, 0xd4, &val32);
+	if (set)
+		val32 |= no_reboot_bit(p);
+	else
+		val32 &= ~no_reboot_bit(p);
+	pci_write_config_dword(p->pci_dev, 0xd4, val32);
+	pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
 
-		val32 = readl(p->gcs_pmc);
-	} else if (p->iTCO_version == 1) {
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-		val32 &= ~enable_bit;
-		pci_write_config_dword(p->pci_dev, 0xd4, val32);
+	/* make sure the update is successful */
+	if (val32 != newval32)
+		return -EIO;
 
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-	}
+	return 0;
+}
+
+static int update_no_reboot_bit_mem(void *priv, bool set)
+{
+	struct iTCO_wdt_private *p = priv;
+	u32 val32 = 0, newval32 = 0;
+
+	val32 = readl(p->gcs_pmc);
+	if (set)
+		val32 |= no_reboot_bit(p);
+	else
+		val32 &= ~no_reboot_bit(p);
+	writel(val32, p->gcs_pmc);
+	newval32 = readl(p->gcs_pmc);
 
-	if (val32 & enable_bit)
+	/* make sure the update is successful */
+	if (val32 != newval32)
 		return -EIO;
 
 	return 0;
 }
 
+static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
+{
+	if (p->iTCO_version >= 2)
+		p->update_no_reboot_bit = update_no_reboot_bit_mem;
+	else if (p->iTCO_version == 1)
+		p->update_no_reboot_bit = update_no_reboot_bit_pci;
+	else
+		p->update_no_reboot_bit = update_no_reboot_bit_def;
+}
+
 static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 {
 	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
@@ -222,7 +237,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
 
 	/* disable chipset's NO_REBOOT bit */
-	if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
+	if (p->update_no_reboot_bit(p, false)) {
 		spin_unlock(&p->io_lock);
 		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
 		return -EIO;
@@ -263,7 +278,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
 	val = inw(TCO1_CNT(p));
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	iTCO_wdt_set_NO_REBOOT_bit(p);
+	p->update_no_reboot_bit(p, true);
 
 	spin_unlock(&p->io_lock);
 
@@ -428,6 +443,8 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	p->iTCO_version = pdata->version;
 	p->pci_dev = to_pci_dev(dev->parent);
 
+	iTCO_wdt_no_reboot_bit_setup(p);
+
 	/*
 	 * Get the Memory-Mapped GCS or PMC register, we need it for the
 	 * NO_REBOOT flag (TCO v2 and v3).
@@ -442,14 +459,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	}
 
 	/* Check chipset's NO_REBOOT bit */
-	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
+	if (p->update_no_reboot_bit(p, false) &&
 	    iTCO_vendor_check_noreboot_on()) {
 		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
 		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
 	}
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	iTCO_wdt_set_NO_REBOOT_bit(p);
+	p->update_no_reboot_bit(p, true);
 
 	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
 	if (!devm_request_region(dev, p->smi_res->start,
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 3/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot_bit functions

[Kuppuswamy Sathyanarayanan]

iTCO_wdt no_reboot_bit set/unset functions has lot of common code between
them. So merging these two functions into a single update function would
remove these unnecessary code duplications. This patch fixes this issue
by creating a no_reboot_bit update function to handle both set/unset
functions.

Also checking for iTCO version every time you make no_reboot_bit set/unset
call is inefficient and makes the code look complex. This can be improved
by performing this check once during device probe and selecting the
appropriate no_reboot_bit update function. This patch fixes this issue
by splitting the update function into multiple helper functions.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/watchdog/iTCO_wdt.c | 83 +++++++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 33 deletions(-)

Changes since v6:
 * make iTCO_wdt_no_reboot_bit_setup() static
 * remove inline specifier for update_no_reboot_bit helper functions.

Changes since v5:
 * Changed update function argument name from "status" to "set".
 * Fixed commit history.
 * Changed update function name to use "bit" instead of "flag"
 * Addressed Andy's comment on creating multiple helper functions.

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3d0abc0..a629933 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -106,6 +106,8 @@ struct iTCO_wdt_private {
 	struct pci_dev *pci_dev;
 	/* whether or not the watchdog has been suspended */
 	bool suspended;
+	/* no reboot update function pointer */
+	int (*update_no_reboot_bit)(void *p, bool set);
 };
 
 /* module parameters */
@@ -170,48 +172,61 @@ static inline u32 no_reboot_bit(struct iTCO_wdt_private *p)
 	return enable_bit;
 }
 
-static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
+static int update_no_reboot_bit_def(void *priv, bool set)
 {
-	u32 val32;
-
-	/* Set the NO_REBOOT bit: this disables reboots */
-	if (p->iTCO_version >= 2) {
-		val32 = readl(p->gcs_pmc);
-		val32 |= no_reboot_bit(p);
-		writel(val32, p->gcs_pmc);
-	} else if (p->iTCO_version == 1) {
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-		val32 |= no_reboot_bit(p);
-		pci_write_config_dword(p->pci_dev, 0xd4, val32);
-	}
+	return 0;
 }
 
-static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
+static int update_no_reboot_bit_pci(void *priv, bool set)
 {
-	u32 enable_bit = no_reboot_bit(p);
-	u32 val32 = 0;
+	struct iTCO_wdt_private *p = priv;
+	u32 val32 = 0, newval32 = 0;
 
-	/* Unset the NO_REBOOT bit: this enables reboots */
-	if (p->iTCO_version >= 2) {
-		val32 = readl(p->gcs_pmc);
-		val32 &= ~enable_bit;
-		writel(val32, p->gcs_pmc);
+	pci_read_config_dword(p->pci_dev, 0xd4, &val32);
+	if (set)
+		val32 |= no_reboot_bit(p);
+	else
+		val32 &= ~no_reboot_bit(p);
+	pci_write_config_dword(p->pci_dev, 0xd4, val32);
+	pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
 
-		val32 = readl(p->gcs_pmc);
-	} else if (p->iTCO_version == 1) {
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-		val32 &= ~enable_bit;
-		pci_write_config_dword(p->pci_dev, 0xd4, val32);
+	/* make sure the update is successful */
+	if (val32 != newval32)
+		return -EIO;
 
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-	}
+	return 0;
+}
+
+static int update_no_reboot_bit_mem(void *priv, bool set)
+{
+	struct iTCO_wdt_private *p = priv;
+	u32 val32 = 0, newval32 = 0;
+
+	val32 = readl(p->gcs_pmc);
+	if (set)
+		val32 |= no_reboot_bit(p);
+	else
+		val32 &= ~no_reboot_bit(p);
+	writel(val32, p->gcs_pmc);
+	newval32 = readl(p->gcs_pmc);
 
-	if (val32 & enable_bit)
+	/* make sure the update is successful */
+	if (val32 != newval32)
 		return -EIO;
 
 	return 0;
 }
 
+static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
+{
+	if (p->iTCO_version >= 2)
+		p->update_no_reboot_bit = update_no_reboot_bit_mem;
+	else if (p->iTCO_version == 1)
+		p->update_no_reboot_bit = update_no_reboot_bit_pci;
+	else
+		p->update_no_reboot_bit = update_no_reboot_bit_def;
+}
+
 static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 {
 	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
@@ -222,7 +237,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
 
 	/* disable chipset's NO_REBOOT bit */
-	if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
+	if (p->update_no_reboot_bit(p, false)) {
 		spin_unlock(&p->io_lock);
 		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
 		return -EIO;
@@ -263,7 +278,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
 	val = inw(TCO1_CNT(p));
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	iTCO_wdt_set_NO_REBOOT_bit(p);
+	p->update_no_reboot_bit(p, true);
 
 	spin_unlock(&p->io_lock);
 
@@ -428,6 +443,8 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	p->iTCO_version = pdata->version;
 	p->pci_dev = to_pci_dev(dev->parent);
 
+	iTCO_wdt_no_reboot_bit_setup(p);
+
 	/*
 	 * Get the Memory-Mapped GCS or PMC register, we need it for the
 	 * NO_REBOOT flag (TCO v2 and v3).
@@ -442,14 +459,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	}
 
 	/* Check chipset's NO_REBOOT bit */
-	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
+	if (p->update_no_reboot_bit(p, false) &&
 	    iTCO_vendor_check_noreboot_on()) {
 		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
 		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
 	}
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	iTCO_wdt_set_NO_REBOOT_bit(p);
+	p->update_no_reboot_bit(p, true);
 
 	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
 	if (!devm_request_region(dev, p->smi_res->start,
-- 
2.7.4

Re: [PATCH v7 3/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot_bit functions

[Guenter Roeck]

On 04/09/2017 03:00 PM, Kuppuswamy Sathyanarayanan wrote:
> iTCO_wdt no_reboot_bit set/unset functions has lot of common code between
> them. So merging these two functions into a single update function would
> remove these unnecessary code duplications. This patch fixes this issue
> by creating a no_reboot_bit update function to handle both set/unset
> functions.
>
> Also checking for iTCO version every time you make no_reboot_bit set/unset
> call is inefficient and makes the code look complex. This can be improved
> by performing this check once during device probe and selecting the
> appropriate no_reboot_bit update function. This patch fixes this issue
> by splitting the update function into multiple helper functions.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/iTCO_wdt.c | 83 +++++++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 33 deletions(-)
>
> Changes since v6:
>  * make iTCO_wdt_no_reboot_bit_setup() static
>  * remove inline specifier for update_no_reboot_bit helper functions.
>
> Changes since v5:
>  * Changed update function argument name from "status" to "set".
>  * Fixed commit history.
>  * Changed update function name to use "bit" instead of "flag"
>  * Addressed Andy's comment on creating multiple helper functions.
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 3d0abc0..a629933 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -106,6 +106,8 @@ struct iTCO_wdt_private {
>  	struct pci_dev *pci_dev;
>  	/* whether or not the watchdog has been suspended */
>  	bool suspended;
> +	/* no reboot update function pointer */
> +	int (*update_no_reboot_bit)(void *p, bool set);
>  };
>
>  /* module parameters */
> @@ -170,48 +172,61 @@ static inline u32 no_reboot_bit(struct iTCO_wdt_private *p)
>  	return enable_bit;
>  }
>
> -static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
> +static int update_no_reboot_bit_def(void *priv, bool set)
>  {
> -	u32 val32;
> -
> -	/* Set the NO_REBOOT bit: this disables reboots */
> -	if (p->iTCO_version >= 2) {
> -		val32 = readl(p->gcs_pmc);
> -		val32 |= no_reboot_bit(p);
> -		writel(val32, p->gcs_pmc);
> -	} else if (p->iTCO_version == 1) {
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 |= no_reboot_bit(p);
> -		pci_write_config_dword(p->pci_dev, 0xd4, val32);
> -	}
> +	return 0;
>  }
>
> -static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
> +static int update_no_reboot_bit_pci(void *priv, bool set)
>  {
> -	u32 enable_bit = no_reboot_bit(p);
> -	u32 val32 = 0;
> +	struct iTCO_wdt_private *p = priv;
> +	u32 val32 = 0, newval32 = 0;
>
> -	/* Unset the NO_REBOOT bit: this enables reboots */
> -	if (p->iTCO_version >= 2) {
> -		val32 = readl(p->gcs_pmc);
> -		val32 &= ~enable_bit;
> -		writel(val32, p->gcs_pmc);
> +	pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> +	if (set)
> +		val32 |= no_reboot_bit(p);
> +	else
> +		val32 &= ~no_reboot_bit(p);
> +	pci_write_config_dword(p->pci_dev, 0xd4, val32);
> +	pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
>
> -		val32 = readl(p->gcs_pmc);
> -	} else if (p->iTCO_version == 1) {
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 &= ~enable_bit;
> -		pci_write_config_dword(p->pci_dev, 0xd4, val32);
> +	/* make sure the update is successful */
> +	if (val32 != newval32)
> +		return -EIO;
>
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -	}
> +	return 0;
> +}
> +
> +static int update_no_reboot_bit_mem(void *priv, bool set)
> +{
> +	struct iTCO_wdt_private *p = priv;
> +	u32 val32 = 0, newval32 = 0;
> +
> +	val32 = readl(p->gcs_pmc);
> +	if (set)
> +		val32 |= no_reboot_bit(p);
> +	else
> +		val32 &= ~no_reboot_bit(p);
> +	writel(val32, p->gcs_pmc);
> +	newval32 = readl(p->gcs_pmc);
>
> -	if (val32 & enable_bit)
> +	/* make sure the update is successful */
> +	if (val32 != newval32)
>  		return -EIO;
>
>  	return 0;
>  }
>
> +static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
> +{
> +	if (p->iTCO_version >= 2)
> +		p->update_no_reboot_bit = update_no_reboot_bit_mem;
> +	else if (p->iTCO_version == 1)
> +		p->update_no_reboot_bit = update_no_reboot_bit_pci;
> +	else
> +		p->update_no_reboot_bit = update_no_reboot_bit_def;
> +}
> +
>  static int iTCO_wdt_start(struct watchdog_device *wd_dev)
>  {
>  	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
> @@ -222,7 +237,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
>  	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
>
>  	/* disable chipset's NO_REBOOT bit */
> -	if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
> +	if (p->update_no_reboot_bit(p, false)) {
>  		spin_unlock(&p->io_lock);
>  		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
>  		return -EIO;
> @@ -263,7 +278,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
>  	val = inw(TCO1_CNT(p));
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	iTCO_wdt_set_NO_REBOOT_bit(p);
> +	p->update_no_reboot_bit(p, true);
>
>  	spin_unlock(&p->io_lock);
>
> @@ -428,6 +443,8 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	p->iTCO_version = pdata->version;
>  	p->pci_dev = to_pci_dev(dev->parent);
>
> +	iTCO_wdt_no_reboot_bit_setup(p);
> +
>  	/*
>  	 * Get the Memory-Mapped GCS or PMC register, we need it for the
>  	 * NO_REBOOT flag (TCO v2 and v3).
> @@ -442,14 +459,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	}
>
>  	/* Check chipset's NO_REBOOT bit */
> -	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
> +	if (p->update_no_reboot_bit(p, false) &&
>  	    iTCO_vendor_check_noreboot_on()) {
>  		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
>  		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
>  	}
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	iTCO_wdt_set_NO_REBOOT_bit(p);
> +	p->update_no_reboot_bit(p, true);
>
>  	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
>  	if (!devm_request_region(dev, p->smi_res->start,
>

Re: [PATCH v7 3/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot_bit functions

[Guenter Roeck]

On 04/09/2017 03:00 PM, Kuppuswamy Sathyanarayanan wrote:
> iTCO_wdt no_reboot_bit set/unset functions has lot of common code between
> them. So merging these two functions into a single update function would
> remove these unnecessary code duplications. This patch fixes this issue
> by creating a no_reboot_bit update function to handle both set/unset
> functions.
>
> Also checking for iTCO version every time you make no_reboot_bit set/unset
> call is inefficient and makes the code look complex. This can be improved
> by performing this check once during device probe and selecting the
> appropriate no_reboot_bit update function. This patch fixes this issue
> by splitting the update function into multiple helper functions.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/iTCO_wdt.c | 83 +++++++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 33 deletions(-)
>
> Changes since v6:
>  * make iTCO_wdt_no_reboot_bit_setup() static
>  * remove inline specifier for update_no_reboot_bit helper functions.
>
> Changes since v5:
>  * Changed update function argument name from "status" to "set".
>  * Fixed commit history.
>  * Changed update function name to use "bit" instead of "flag"
>  * Addressed Andy's comment on creating multiple helper functions.
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 3d0abc0..a629933 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -106,6 +106,8 @@ struct iTCO_wdt_private {
>  	struct pci_dev *pci_dev;
>  	/* whether or not the watchdog has been suspended */
>  	bool suspended;
> +	/* no reboot update function pointer */
> +	int (*update_no_reboot_bit)(void *p, bool set);
>  };
>
>  /* module parameters */
> @@ -170,48 +172,61 @@ static inline u32 no_reboot_bit(struct iTCO_wdt_private *p)
>  	return enable_bit;
>  }
>
> -static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
> +static int update_no_reboot_bit_def(void *priv, bool set)
>  {
> -	u32 val32;
> -
> -	/* Set the NO_REBOOT bit: this disables reboots */
> -	if (p->iTCO_version >= 2) {
> -		val32 = readl(p->gcs_pmc);
> -		val32 |= no_reboot_bit(p);
> -		writel(val32, p->gcs_pmc);
> -	} else if (p->iTCO_version == 1) {
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 |= no_reboot_bit(p);
> -		pci_write_config_dword(p->pci_dev, 0xd4, val32);
> -	}
> +	return 0;
>  }
>
> -static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
> +static int update_no_reboot_bit_pci(void *priv, bool set)
>  {
> -	u32 enable_bit = no_reboot_bit(p);
> -	u32 val32 = 0;
> +	struct iTCO_wdt_private *p = priv;
> +	u32 val32 = 0, newval32 = 0;
>
> -	/* Unset the NO_REBOOT bit: this enables reboots */
> -	if (p->iTCO_version >= 2) {
> -		val32 = readl(p->gcs_pmc);
> -		val32 &= ~enable_bit;
> -		writel(val32, p->gcs_pmc);
> +	pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> +	if (set)
> +		val32 |= no_reboot_bit(p);
> +	else
> +		val32 &= ~no_reboot_bit(p);
> +	pci_write_config_dword(p->pci_dev, 0xd4, val32);
> +	pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
>
> -		val32 = readl(p->gcs_pmc);
> -	} else if (p->iTCO_version == 1) {
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 &= ~enable_bit;
> -		pci_write_config_dword(p->pci_dev, 0xd4, val32);
> +	/* make sure the update is successful */
> +	if (val32 != newval32)
> +		return -EIO;
>
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -	}
> +	return 0;
> +}
> +
> +static int update_no_reboot_bit_mem(void *priv, bool set)
> +{
> +	struct iTCO_wdt_private *p = priv;
> +	u32 val32 = 0, newval32 = 0;
> +
> +	val32 = readl(p->gcs_pmc);
> +	if (set)
> +		val32 |= no_reboot_bit(p);
> +	else
> +		val32 &= ~no_reboot_bit(p);
> +	writel(val32, p->gcs_pmc);
> +	newval32 = readl(p->gcs_pmc);
>
> -	if (val32 & enable_bit)
> +	/* make sure the update is successful */
> +	if (val32 != newval32)
>  		return -EIO;
>
>  	return 0;
>  }
>
> +static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
> +{
> +	if (p->iTCO_version >= 2)
> +		p->update_no_reboot_bit = update_no_reboot_bit_mem;
> +	else if (p->iTCO_version == 1)
> +		p->update_no_reboot_bit = update_no_reboot_bit_pci;
> +	else
> +		p->update_no_reboot_bit = update_no_reboot_bit_def;
> +}
> +
>  static int iTCO_wdt_start(struct watchdog_device *wd_dev)
>  {
>  	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
> @@ -222,7 +237,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
>  	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
>
>  	/* disable chipset's NO_REBOOT bit */
> -	if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
> +	if (p->update_no_reboot_bit(p, false)) {
>  		spin_unlock(&p->io_lock);
>  		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
>  		return -EIO;
> @@ -263,7 +278,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
>  	val = inw(TCO1_CNT(p));
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	iTCO_wdt_set_NO_REBOOT_bit(p);
> +	p->update_no_reboot_bit(p, true);
>
>  	spin_unlock(&p->io_lock);
>
> @@ -428,6 +443,8 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	p->iTCO_version = pdata->version;
>  	p->pci_dev = to_pci_dev(dev->parent);
>
> +	iTCO_wdt_no_reboot_bit_setup(p);
> +
>  	/*
>  	 * Get the Memory-Mapped GCS or PMC register, we need it for the
>  	 * NO_REBOOT flag (TCO v2 and v3).
> @@ -442,14 +459,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	}
>
>  	/* Check chipset's NO_REBOOT bit */
> -	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
> +	if (p->update_no_reboot_bit(p, false) &&
>  	    iTCO_vendor_check_noreboot_on()) {
>  		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
>  		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
>  	}
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	iTCO_wdt_set_NO_REBOOT_bit(p);
> +	p->update_no_reboot_bit(p, true);
>
>  	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
>  	if (!devm_request_region(dev, p->smi_res->start,
>

Re: [PATCH v7 3/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot_bit functions

[Guenter Roeck]

On 04/09/2017 03:00 PM, Kuppuswamy Sathyanarayanan wrote:
> iTCO_wdt no_reboot_bit set/unset functions has lot of common code between
> them. So merging these two functions into a single update function would
> remove these unnecessary code duplications. This patch fixes this issue
> by creating a no_reboot_bit update function to handle both set/unset
> functions.
>
> Also checking for iTCO version every time you make no_reboot_bit set/unset
> call is inefficient and makes the code look complex. This can be improved
> by performing this check once during device probe and selecting the
> appropriate no_reboot_bit update function. This patch fixes this issue
> by splitting the update function into multiple helper functions.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/iTCO_wdt.c | 83 +++++++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 33 deletions(-)
>
> Changes since v6:
>  * make iTCO_wdt_no_reboot_bit_setup() static
>  * remove inline specifier for update_no_reboot_bit helper functions.
>
> Changes since v5:
>  * Changed update function argument name from "status" to "set".
>  * Fixed commit history.
>  * Changed update function name to use "bit" instead of "flag"
>  * Addressed Andy's comment on creating multiple helper functions.
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 3d0abc0..a629933 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -106,6 +106,8 @@ struct iTCO_wdt_private {
>  	struct pci_dev *pci_dev;
>  	/* whether or not the watchdog has been suspended */
>  	bool suspended;
> +	/* no reboot update function pointer */
> +	int (*update_no_reboot_bit)(void *p, bool set);
>  };
>
>  /* module parameters */
> @@ -170,48 +172,61 @@ static inline u32 no_reboot_bit(struct iTCO_wdt_private *p)
>  	return enable_bit;
>  }
>
> -static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
> +static int update_no_reboot_bit_def(void *priv, bool set)
>  {
> -	u32 val32;
> -
> -	/* Set the NO_REBOOT bit: this disables reboots */
> -	if (p->iTCO_version >= 2) {
> -		val32 = readl(p->gcs_pmc);
> -		val32 |= no_reboot_bit(p);
> -		writel(val32, p->gcs_pmc);
> -	} else if (p->iTCO_version == 1) {
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 |= no_reboot_bit(p);
> -		pci_write_config_dword(p->pci_dev, 0xd4, val32);
> -	}
> +	return 0;
>  }
>
> -static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
> +static int update_no_reboot_bit_pci(void *priv, bool set)
>  {
> -	u32 enable_bit = no_reboot_bit(p);
> -	u32 val32 = 0;
> +	struct iTCO_wdt_private *p = priv;
> +	u32 val32 = 0, newval32 = 0;
>
> -	/* Unset the NO_REBOOT bit: this enables reboots */
> -	if (p->iTCO_version >= 2) {
> -		val32 = readl(p->gcs_pmc);
> -		val32 &= ~enable_bit;
> -		writel(val32, p->gcs_pmc);
> +	pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> +	if (set)
> +		val32 |= no_reboot_bit(p);
> +	else
> +		val32 &= ~no_reboot_bit(p);
> +	pci_write_config_dword(p->pci_dev, 0xd4, val32);
> +	pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
>
> -		val32 = readl(p->gcs_pmc);
> -	} else if (p->iTCO_version == 1) {
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 &= ~enable_bit;
> -		pci_write_config_dword(p->pci_dev, 0xd4, val32);
> +	/* make sure the update is successful */
> +	if (val32 != newval32)
> +		return -EIO;
>
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -	}
> +	return 0;
> +}
> +
> +static int update_no_reboot_bit_mem(void *priv, bool set)
> +{
> +	struct iTCO_wdt_private *p = priv;
> +	u32 val32 = 0, newval32 = 0;
> +
> +	val32 = readl(p->gcs_pmc);
> +	if (set)
> +		val32 |= no_reboot_bit(p);
> +	else
> +		val32 &= ~no_reboot_bit(p);
> +	writel(val32, p->gcs_pmc);
> +	newval32 = readl(p->gcs_pmc);
>
> -	if (val32 & enable_bit)
> +	/* make sure the update is successful */
> +	if (val32 != newval32)
>  		return -EIO;
>
>  	return 0;
>  }
>
> +static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
> +{
> +	if (p->iTCO_version >= 2)
> +		p->update_no_reboot_bit = update_no_reboot_bit_mem;
> +	else if (p->iTCO_version == 1)
> +		p->update_no_reboot_bit = update_no_reboot_bit_pci;
> +	else
> +		p->update_no_reboot_bit = update_no_reboot_bit_def;
> +}
> +
>  static int iTCO_wdt_start(struct watchdog_device *wd_dev)
>  {
>  	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
> @@ -222,7 +237,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
>  	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
>
>  	/* disable chipset's NO_REBOOT bit */
> -	if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
> +	if (p->update_no_reboot_bit(p, false)) {
>  		spin_unlock(&p->io_lock);
>  		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
>  		return -EIO;
> @@ -263,7 +278,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
>  	val = inw(TCO1_CNT(p));
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	iTCO_wdt_set_NO_REBOOT_bit(p);
> +	p->update_no_reboot_bit(p, true);
>
>  	spin_unlock(&p->io_lock);
>
> @@ -428,6 +443,8 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	p->iTCO_version = pdata->version;
>  	p->pci_dev = to_pci_dev(dev->parent);
>
> +	iTCO_wdt_no_reboot_bit_setup(p);
> +
>  	/*
>  	 * Get the Memory-Mapped GCS or PMC register, we need it for the
>  	 * NO_REBOOT flag (TCO v2 and v3).
> @@ -442,14 +459,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	}
>
>  	/* Check chipset's NO_REBOOT bit */
> -	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
> +	if (p->update_no_reboot_bit(p, false) &&
>  	    iTCO_vendor_check_noreboot_on()) {
>  		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
>  		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
>  	}
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	iTCO_wdt_set_NO_REBOOT_bit(p);
> +	p->update_no_reboot_bit(p, true);
>
>  	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
>  	if (!devm_request_region(dev, p->smi_res->start,
>

Re: [PATCH v7 3/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot_bit functions

[Guenter Roeck]

On 04/09/2017 03:00 PM, Kuppuswamy Sathyanarayanan wrote:
> iTCO_wdt no_reboot_bit set/unset functions has lot of common code between
> them. So merging these two functions into a single update function would
> remove these unnecessary code duplications. This patch fixes this issue
> by creating a no_reboot_bit update function to handle both set/unset
> functions.
>
> Also checking for iTCO version every time you make no_reboot_bit set/unset
> call is inefficient and makes the code look complex. This can be improved
> by performing this check once during device probe and selecting the
> appropriate no_reboot_bit update function. This patch fixes this issue
> by splitting the update function into multiple helper functions.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/iTCO_wdt.c | 83 +++++++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 33 deletions(-)
>
> Changes since v6:
>  * make iTCO_wdt_no_reboot_bit_setup() static
>  * remove inline specifier for update_no_reboot_bit helper functions.
>
> Changes since v5:
>  * Changed update function argument name from "status" to "set".
>  * Fixed commit history.
>  * Changed update function name to use "bit" instead of "flag"
>  * Addressed Andy's comment on creating multiple helper functions.
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 3d0abc0..a629933 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -106,6 +106,8 @@ struct iTCO_wdt_private {
>  	struct pci_dev *pci_dev;
>  	/* whether or not the watchdog has been suspended */
>  	bool suspended;
> +	/* no reboot update function pointer */
> +	int (*update_no_reboot_bit)(void *p, bool set);
>  };
>
>  /* module parameters */
> @@ -170,48 +172,61 @@ static inline u32 no_reboot_bit(struct iTCO_wdt_private *p)
>  	return enable_bit;
>  }
>
> -static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
> +static int update_no_reboot_bit_def(void *priv, bool set)
>  {
> -	u32 val32;
> -
> -	/* Set the NO_REBOOT bit: this disables reboots */
> -	if (p->iTCO_version >= 2) {
> -		val32 = readl(p->gcs_pmc);
> -		val32 |= no_reboot_bit(p);
> -		writel(val32, p->gcs_pmc);
> -	} else if (p->iTCO_version == 1) {
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 |= no_reboot_bit(p);
> -		pci_write_config_dword(p->pci_dev, 0xd4, val32);
> -	}
> +	return 0;
>  }
>
> -static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
> +static int update_no_reboot_bit_pci(void *priv, bool set)
>  {
> -	u32 enable_bit = no_reboot_bit(p);
> -	u32 val32 = 0;
> +	struct iTCO_wdt_private *p = priv;
> +	u32 val32 = 0, newval32 = 0;
>
> -	/* Unset the NO_REBOOT bit: this enables reboots */
> -	if (p->iTCO_version >= 2) {
> -		val32 = readl(p->gcs_pmc);
> -		val32 &= ~enable_bit;
> -		writel(val32, p->gcs_pmc);
> +	pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> +	if (set)
> +		val32 |= no_reboot_bit(p);
> +	else
> +		val32 &= ~no_reboot_bit(p);
> +	pci_write_config_dword(p->pci_dev, 0xd4, val32);
> +	pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
>
> -		val32 = readl(p->gcs_pmc);
> -	} else if (p->iTCO_version == 1) {
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 &= ~enable_bit;
> -		pci_write_config_dword(p->pci_dev, 0xd4, val32);
> +	/* make sure the update is successful */
> +	if (val32 != newval32)
> +		return -EIO;
>
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -	}
> +	return 0;
> +}
> +
> +static int update_no_reboot_bit_mem(void *priv, bool set)
> +{
> +	struct iTCO_wdt_private *p = priv;
> +	u32 val32 = 0, newval32 = 0;
> +
> +	val32 = readl(p->gcs_pmc);
> +	if (set)
> +		val32 |= no_reboot_bit(p);
> +	else
> +		val32 &= ~no_reboot_bit(p);
> +	writel(val32, p->gcs_pmc);
> +	newval32 = readl(p->gcs_pmc);
>
> -	if (val32 & enable_bit)
> +	/* make sure the update is successful */
> +	if (val32 != newval32)
>  		return -EIO;
>
>  	return 0;
>  }
>
> +static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
> +{
> +	if (p->iTCO_version >= 2)
> +		p->update_no_reboot_bit = update_no_reboot_bit_mem;
> +	else if (p->iTCO_version == 1)
> +		p->update_no_reboot_bit = update_no_reboot_bit_pci;
> +	else
> +		p->update_no_reboot_bit = update_no_reboot_bit_def;
> +}
> +
>  static int iTCO_wdt_start(struct watchdog_device *wd_dev)
>  {
>  	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
> @@ -222,7 +237,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
>  	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
>
>  	/* disable chipset's NO_REBOOT bit */
> -	if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
> +	if (p->update_no_reboot_bit(p, false)) {
>  		spin_unlock(&p->io_lock);
>  		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
>  		return -EIO;
> @@ -263,7 +278,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
>  	val = inw(TCO1_CNT(p));
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	iTCO_wdt_set_NO_REBOOT_bit(p);
> +	p->update_no_reboot_bit(p, true);
>
>  	spin_unlock(&p->io_lock);
>
> @@ -428,6 +443,8 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	p->iTCO_version = pdata->version;
>  	p->pci_dev = to_pci_dev(dev->parent);
>
> +	iTCO_wdt_no_reboot_bit_setup(p);
> +
>  	/*
>  	 * Get the Memory-Mapped GCS or PMC register, we need it for the
>  	 * NO_REBOOT flag (TCO v2 and v3).
> @@ -442,14 +459,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	}
>
>  	/* Check chipset's NO_REBOOT bit */
> -	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
> +	if (p->update_no_reboot_bit(p, false) &&
>  	    iTCO_vendor_check_noreboot_on()) {
>  		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
>  		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
>  	}
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	iTCO_wdt_set_NO_REBOOT_bit(p);
> +	p->update_no_reboot_bit(p, true);
>
>  	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
>  	if (!devm_request_region(dev, p->smi_res->start,
>

[PATCH v7 4/6] watchdog: iTCO_wdt: Add PMC specific noreboot update api

[Kuppuswamy Sathyanarayanan]

In some SoCs, setting noreboot bit needs modification to
PMC GC registers. But not all PMC drivers allow other drivers
to memory map their GC region. This could create mem request
conflict in watchdog driver. So this patch adds facility to allow
PMC drivers to pass noreboot update function to watchdog
drivers via platform data.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/iTCO_wdt.c            | 25 ++++++++++++++++++-------
 include/linux/platform_data/itco_wdt.h |  4 ++++
 2 files changed, 22 insertions(+), 7 deletions(-)

Changes since v6:
 * Moved pdata no_reboot_bit initialization to iTCO_wdt_no_reboot_bit_setup

Changes since v5:
 * Added priv_data to pass private data to no_reboot_bit update function.
 * Changed update_noreboot_flag() to update_no_reboot_bit

Changes since v4:
 * Fixed some style issues.
 * used false for 0 and true for 1 in update_noreboot_flag function.

Changes since v3:
 * Rebased on top of latest.

Changes since v2: 
 * Removed use of PMC API's directly in watchdog driver.
 * Added update_noreboot_flag to handle no IPC PMC compatibility
   issue mentioned by Guenter.

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index a629933..347f038 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -106,6 +106,8 @@ struct iTCO_wdt_private {
 	struct pci_dev *pci_dev;
 	/* whether or not the watchdog has been suspended */
 	bool suspended;
+	/* no reboot API private data */
+	void *no_reboot_priv;
 	/* no reboot update function pointer */
 	int (*update_no_reboot_bit)(void *p, bool set);
 };
@@ -217,14 +219,23 @@ static int update_no_reboot_bit_mem(void *priv, bool set)
 	return 0;
 }
 
-static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
+static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p,
+		struct itco_wdt_platform_data *pdata)
 {
+	if (pdata->update_no_reboot_bit) {
+		p->update_no_reboot_bit = pdata->update_no_reboot_bit;
+		p->no_reboot_priv = pdata->no_reboot_priv;
+		return;
+	}
+
 	if (p->iTCO_version >= 2)
 		p->update_no_reboot_bit = update_no_reboot_bit_mem;
 	else if (p->iTCO_version == 1)
 		p->update_no_reboot_bit = update_no_reboot_bit_pci;
 	else
 		p->update_no_reboot_bit = update_no_reboot_bit_def;
+
+	p->no_reboot_priv = p;
 }
 
 static int iTCO_wdt_start(struct watchdog_device *wd_dev)
@@ -237,7 +248,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
 
 	/* disable chipset's NO_REBOOT bit */
-	if (p->update_no_reboot_bit(p, false)) {
+	if (p->update_no_reboot_bit(p->no_reboot_priv, false)) {
 		spin_unlock(&p->io_lock);
 		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
 		return -EIO;
@@ -278,7 +289,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
 	val = inw(TCO1_CNT(p));
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	p->update_no_reboot_bit(p, true);
+	p->update_no_reboot_bit(p->no_reboot_priv, true);
 
 	spin_unlock(&p->io_lock);
 
@@ -443,13 +454,13 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	p->iTCO_version = pdata->version;
 	p->pci_dev = to_pci_dev(dev->parent);
 
-	iTCO_wdt_no_reboot_bit_setup(p);
+	iTCO_wdt_no_reboot_bit_setup(p, pdata);
 
 	/*
 	 * Get the Memory-Mapped GCS or PMC register, we need it for the
 	 * NO_REBOOT flag (TCO v2 and v3).
 	 */
-	if (p->iTCO_version >= 2) {
+	if (p->iTCO_version >= 2 && !pdata->update_no_reboot_bit) {
 		p->gcs_pmc_res = platform_get_resource(pdev,
 						       IORESOURCE_MEM,
 						       ICH_RES_MEM_GCS_PMC);
@@ -459,14 +470,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	}
 
 	/* Check chipset's NO_REBOOT bit */
-	if (p->update_no_reboot_bit(p, false) &&
+	if (p->update_no_reboot_bit(p->no_reboot_priv, false) &&
 	    iTCO_vendor_check_noreboot_on()) {
 		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
 		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
 	}
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	p->update_no_reboot_bit(p, true);
+	p->update_no_reboot_bit(p->no_reboot_priv, true);
 
 	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
 	if (!devm_request_region(dev, p->smi_res->start,
diff --git a/include/linux/platform_data/itco_wdt.h b/include/linux/platform_data/itco_wdt.h
index f16542c..0e95527 100644
--- a/include/linux/platform_data/itco_wdt.h
+++ b/include/linux/platform_data/itco_wdt.h
@@ -14,6 +14,10 @@
 struct itco_wdt_platform_data {
 	char name[32];
 	unsigned int version;
+	/* private data to be passed to update_no_reboot_bit API */
+	void *no_reboot_priv;
+	/* pointer for platform specific no reboot update function */
+	int (*update_no_reboot_bit)(void *priv, bool set);
 };
 
 #endif /* _ITCO_WDT_H_ */
-- 
2.7.4

[PATCH v7 4/6] watchdog: iTCO_wdt: Add PMC specific noreboot update api

[Kuppuswamy Sathyanarayanan]

In some SoCs, setting noreboot bit needs modification to
PMC GC registers. But not all PMC drivers allow other drivers
to memory map their GC region. This could create mem request
conflict in watchdog driver. So this patch adds facility to allow
PMC drivers to pass noreboot update function to watchdog
drivers via platform data.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/iTCO_wdt.c            | 25 ++++++++++++++++++-------
 include/linux/platform_data/itco_wdt.h |  4 ++++
 2 files changed, 22 insertions(+), 7 deletions(-)

Changes since v6:
 * Moved pdata no_reboot_bit initialization to iTCO_wdt_no_reboot_bit_setup

Changes since v5:
 * Added priv_data to pass private data to no_reboot_bit update function.
 * Changed update_noreboot_flag() to update_no_reboot_bit

Changes since v4:
 * Fixed some style issues.
 * used false for 0 and true for 1 in update_noreboot_flag function.

Changes since v3:
 * Rebased on top of latest.

Changes since v2: 
 * Removed use of PMC API's directly in watchdog driver.
 * Added update_noreboot_flag to handle no IPC PMC compatibility
   issue mentioned by Guenter.

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index a629933..347f038 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -106,6 +106,8 @@ struct iTCO_wdt_private {
 	struct pci_dev *pci_dev;
 	/* whether or not the watchdog has been suspended */
 	bool suspended;
+	/* no reboot API private data */
+	void *no_reboot_priv;
 	/* no reboot update function pointer */
 	int (*update_no_reboot_bit)(void *p, bool set);
 };
@@ -217,14 +219,23 @@ static int update_no_reboot_bit_mem(void *priv, bool set)
 	return 0;
 }
 
-static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
+static void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p,
+		struct itco_wdt_platform_data *pdata)
 {
+	if (pdata->update_no_reboot_bit) {
+		p->update_no_reboot_bit = pdata->update_no_reboot_bit;
+		p->no_reboot_priv = pdata->no_reboot_priv;
+		return;
+	}
+
 	if (p->iTCO_version >= 2)
 		p->update_no_reboot_bit = update_no_reboot_bit_mem;
 	else if (p->iTCO_version == 1)
 		p->update_no_reboot_bit = update_no_reboot_bit_pci;
 	else
 		p->update_no_reboot_bit = update_no_reboot_bit_def;
+
+	p->no_reboot_priv = p;
 }
 
 static int iTCO_wdt_start(struct watchdog_device *wd_dev)
@@ -237,7 +248,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
 
 	/* disable chipset's NO_REBOOT bit */
-	if (p->update_no_reboot_bit(p, false)) {
+	if (p->update_no_reboot_bit(p->no_reboot_priv, false)) {
 		spin_unlock(&p->io_lock);
 		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
 		return -EIO;
@@ -278,7 +289,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
 	val = inw(TCO1_CNT(p));
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	p->update_no_reboot_bit(p, true);
+	p->update_no_reboot_bit(p->no_reboot_priv, true);
 
 	spin_unlock(&p->io_lock);
 
@@ -443,13 +454,13 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	p->iTCO_version = pdata->version;
 	p->pci_dev = to_pci_dev(dev->parent);
 
-	iTCO_wdt_no_reboot_bit_setup(p);
+	iTCO_wdt_no_reboot_bit_setup(p, pdata);
 
 	/*
 	 * Get the Memory-Mapped GCS or PMC register, we need it for the
 	 * NO_REBOOT flag (TCO v2 and v3).
 	 */
-	if (p->iTCO_version >= 2) {
+	if (p->iTCO_version >= 2 && !pdata->update_no_reboot_bit) {
 		p->gcs_pmc_res = platform_get_resource(pdev,
 						       IORESOURCE_MEM,
 						       ICH_RES_MEM_GCS_PMC);
@@ -459,14 +470,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	}
 
 	/* Check chipset's NO_REBOOT bit */
-	if (p->update_no_reboot_bit(p, false) &&
+	if (p->update_no_reboot_bit(p->no_reboot_priv, false) &&
 	    iTCO_vendor_check_noreboot_on()) {
 		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
 		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
 	}
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	p->update_no_reboot_bit(p, true);
+	p->update_no_reboot_bit(p->no_reboot_priv, true);
 
 	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
 	if (!devm_request_region(dev, p->smi_res->start,
diff --git a/include/linux/platform_data/itco_wdt.h b/include/linux/platform_data/itco_wdt.h
index f16542c..0e95527 100644
--- a/include/linux/platform_data/itco_wdt.h
+++ b/include/linux/platform_data/itco_wdt.h
@@ -14,6 +14,10 @@
 struct itco_wdt_platform_data {
 	char name[32];
 	unsigned int version;
+	/* private data to be passed to update_no_reboot_bit API */
+	void *no_reboot_priv;
+	/* pointer for platform specific no reboot update function */
+	int (*update_no_reboot_bit)(void *priv, bool set);
 };
 
 #endif /* _ITCO_WDT_H_ */
-- 
2.7.4

[PATCH v7 5/6] platform/x86: intel_pmc_ipc: Fix iTCO_wdt GCS memory mapping failure

[Kuppuswamy Sathyanarayanan]

iTCO_wdt driver need access to PMC_CFG GCR register to modify the
noreboot setting. Currently, this is done by passing PMC_CFG reg
address as memory resource to watchdog driver and allowing it directly
modify the PMC_CFG register. But currently PMC driver also has
requirement to memory map the entire GCR register space in this driver.
This causes mem request failure in watchdog driver. So this patch fixes
this issue by adding API to update noreboot flag and passes them
to watchdog driver via platform data.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_ipc.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

Changes since v6:
 * Replace BIT(4) with (1 << 4)
 * Optimized update_no_reboot_bit implementation.

Changes since v5:
 * Fixed some style issues in commit history.
 * removed unused variable gcr_size from intel_pmc_ipc_dev

Changes since v4:
 * Fixed some style issues in commit history.
 * Used macros instead of BIT() calls.

Changes since v3:
 * Rebased on top of latest changes.

Changes since v2: 
 * Added support for update_noreboot_bit api.

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index a0c773b..0a39b0f 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -112,6 +112,13 @@
 #define TCO_PMC_OFFSET			0x8
 #define TCO_PMC_SIZE			0x4
 
+/* PMC register bit definitions */
+
+/* PMC_CFG_REG bit masks */
+#define PMC_CFG_NO_REBOOT_MASK		(1 << 4)
+#define PMC_CFG_NO_REBOOT_EN		(1 << 4)
+#define PMC_CFG_NO_REBOOT_DIS		(0 << 4)
+
 static struct intel_pmc_ipc_dev {
 	struct device *dev;
 	void __iomem *ipc_base;
@@ -126,8 +133,6 @@ static struct intel_pmc_ipc_dev {
 	struct platform_device *tco_dev;
 
 	/* gcr */
-	resource_size_t gcr_base;
-	int gcr_size;
 	void __iomem *gcr_mem_base;
 	bool has_gcr_regs;
 
@@ -313,6 +318,14 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
 
+static int update_no_reboot_bit(void *priv, bool set)
+{
+	u32 value = set ? PMC_CFG_NO_REBOOT_EN : PMC_CFG_NO_REBOOT_DIS;
+
+	return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
+				    PMC_CFG_NO_REBOOT_MASK, value);
+}
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -630,15 +643,13 @@ static struct resource tco_res[] = {
 	{
 		.flags = IORESOURCE_IO,
 	},
-	/* GCS */
-	{
-		.flags = IORESOURCE_MEM,
-	},
 };
 
 static struct itco_wdt_platform_data tco_info = {
 	.name = "Apollo Lake SoC",
 	.version = 5,
+	.no_reboot_priv = &ipcdev,
+	.update_no_reboot_bit = update_no_reboot_bit,
 };
 
 #define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
@@ -695,10 +706,6 @@ static int ipc_create_tco_device(void)
 	res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET;
 	res->end = res->start + SMI_EN_SIZE - 1;
 
-	res = tco_res + TCO_RESOURCE_GCR_MEM;
-	res->start = ipcdev.gcr_base + TCO_PMC_OFFSET;
-	res->end = res->start + TCO_PMC_SIZE - 1;
-
 	pdev = platform_device_register_full(&pdevinfo);
 	if (IS_ERR(pdev))
 		return PTR_ERR(pdev);
@@ -860,9 +867,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 	}
 	ipcdev.ipc_base = addr;
 
-	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
-	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
 	dev_info(&pdev->dev, "ipc res: %pR\n", res);
 
 	ipcdev.telem_res_inval = 0;
-- 
2.7.4

[PATCH v7 5/6] platform/x86: intel_pmc_ipc: Fix iTCO_wdt GCS memory mapping failure

[Kuppuswamy Sathyanarayanan]

iTCO_wdt driver need access to PMC_CFG GCR register to modify the
noreboot setting. Currently, this is done by passing PMC_CFG reg
address as memory resource to watchdog driver and allowing it directly
modify the PMC_CFG register. But currently PMC driver also has
requirement to memory map the entire GCR register space in this driver.
This causes mem request failure in watchdog driver. So this patch fixes
this issue by adding API to update noreboot flag and passes them
to watchdog driver via platform data.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/platform/x86/intel_pmc_ipc.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

Changes since v6:
 * Replace BIT(4) with (1 << 4)
 * Optimized update_no_reboot_bit implementation.

Changes since v5:
 * Fixed some style issues in commit history.
 * removed unused variable gcr_size from intel_pmc_ipc_dev

Changes since v4:
 * Fixed some style issues in commit history.
 * Used macros instead of BIT() calls.

Changes since v3:
 * Rebased on top of latest changes.

Changes since v2: 
 * Added support for update_noreboot_bit api.

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index a0c773b..0a39b0f 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -112,6 +112,13 @@
 #define TCO_PMC_OFFSET			0x8
 #define TCO_PMC_SIZE			0x4
 
+/* PMC register bit definitions */
+
+/* PMC_CFG_REG bit masks */
+#define PMC_CFG_NO_REBOOT_MASK		(1 << 4)
+#define PMC_CFG_NO_REBOOT_EN		(1 << 4)
+#define PMC_CFG_NO_REBOOT_DIS		(0 << 4)
+
 static struct intel_pmc_ipc_dev {
 	struct device *dev;
 	void __iomem *ipc_base;
@@ -126,8 +133,6 @@ static struct intel_pmc_ipc_dev {
 	struct platform_device *tco_dev;
 
 	/* gcr */
-	resource_size_t gcr_base;
-	int gcr_size;
 	void __iomem *gcr_mem_base;
 	bool has_gcr_regs;
 
@@ -313,6 +318,14 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
 
+static int update_no_reboot_bit(void *priv, bool set)
+{
+	u32 value = set ? PMC_CFG_NO_REBOOT_EN : PMC_CFG_NO_REBOOT_DIS;
+
+	return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
+				    PMC_CFG_NO_REBOOT_MASK, value);
+}
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -630,15 +643,13 @@ static struct resource tco_res[] = {
 	{
 		.flags = IORESOURCE_IO,
 	},
-	/* GCS */
-	{
-		.flags = IORESOURCE_MEM,
-	},
 };
 
 static struct itco_wdt_platform_data tco_info = {
 	.name = "Apollo Lake SoC",
 	.version = 5,
+	.no_reboot_priv = &ipcdev,
+	.update_no_reboot_bit = update_no_reboot_bit,
 };
 
 #define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
@@ -695,10 +706,6 @@ static int ipc_create_tco_device(void)
 	res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET;
 	res->end = res->start + SMI_EN_SIZE - 1;
 
-	res = tco_res + TCO_RESOURCE_GCR_MEM;
-	res->start = ipcdev.gcr_base + TCO_PMC_OFFSET;
-	res->end = res->start + TCO_PMC_SIZE - 1;
-
 	pdev = platform_device_register_full(&pdevinfo);
 	if (IS_ERR(pdev))
 		return PTR_ERR(pdev);
@@ -860,9 +867,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 	}
 	ipcdev.ipc_base = addr;
 
-	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
-	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
 	dev_info(&pdev->dev, "ipc res: %pR\n", res);
 
 	ipcdev.telem_res_inval = 0;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 5/6] platform/x86: intel_pmc_ipc: Fix iTCO_wdt GCS memory mapping failure

[Kuppuswamy Sathyanarayanan]

iTCO_wdt driver need access to PMC_CFG GCR register to modify the
noreboot setting. Currently, this is done by passing PMC_CFG reg
address as memory resource to watchdog driver and allowing it directly
modify the PMC_CFG register. But currently PMC driver also has
requirement to memory map the entire GCR register space in this driver.
This causes mem request failure in watchdog driver. So this patch fixes
this issue by adding API to update noreboot flag and passes them
to watchdog driver via platform data.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/platform/x86/intel_pmc_ipc.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

Changes since v6:
 * Replace BIT(4) with (1 << 4)
 * Optimized update_no_reboot_bit implementation.

Changes since v5:
 * Fixed some style issues in commit history.
 * removed unused variable gcr_size from intel_pmc_ipc_dev

Changes since v4:
 * Fixed some style issues in commit history.
 * Used macros instead of BIT() calls.

Changes since v3:
 * Rebased on top of latest changes.

Changes since v2: 
 * Added support for update_noreboot_bit api.

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index a0c773b..0a39b0f 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -112,6 +112,13 @@
 #define TCO_PMC_OFFSET			0x8
 #define TCO_PMC_SIZE			0x4
 
+/* PMC register bit definitions */
+
+/* PMC_CFG_REG bit masks */
+#define PMC_CFG_NO_REBOOT_MASK		(1 << 4)
+#define PMC_CFG_NO_REBOOT_EN		(1 << 4)
+#define PMC_CFG_NO_REBOOT_DIS		(0 << 4)
+
 static struct intel_pmc_ipc_dev {
 	struct device *dev;
 	void __iomem *ipc_base;
@@ -126,8 +133,6 @@ static struct intel_pmc_ipc_dev {
 	struct platform_device *tco_dev;
 
 	/* gcr */
-	resource_size_t gcr_base;
-	int gcr_size;
 	void __iomem *gcr_mem_base;
 	bool has_gcr_regs;
 
@@ -313,6 +318,14 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
 
+static int update_no_reboot_bit(void *priv, bool set)
+{
+	u32 value = set ? PMC_CFG_NO_REBOOT_EN : PMC_CFG_NO_REBOOT_DIS;
+
+	return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
+				    PMC_CFG_NO_REBOOT_MASK, value);
+}
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -630,15 +643,13 @@ static struct resource tco_res[] = {
 	{
 		.flags = IORESOURCE_IO,
 	},
-	/* GCS */
-	{
-		.flags = IORESOURCE_MEM,
-	},
 };
 
 static struct itco_wdt_platform_data tco_info = {
 	.name = "Apollo Lake SoC",
 	.version = 5,
+	.no_reboot_priv = &ipcdev,
+	.update_no_reboot_bit = update_no_reboot_bit,
 };
 
 #define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
@@ -695,10 +706,6 @@ static int ipc_create_tco_device(void)
 	res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET;
 	res->end = res->start + SMI_EN_SIZE - 1;
 
-	res = tco_res + TCO_RESOURCE_GCR_MEM;
-	res->start = ipcdev.gcr_base + TCO_PMC_OFFSET;
-	res->end = res->start + TCO_PMC_SIZE - 1;
-
 	pdev = platform_device_register_full(&pdevinfo);
 	if (IS_ERR(pdev))
 		return PTR_ERR(pdev);
@@ -860,9 +867,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 	}
 	ipcdev.ipc_base = addr;
 
-	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
-	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
 	dev_info(&pdev->dev, "ipc res: %pR\n", res);
 
 	ipcdev.telem_res_inval = 0;
-- 
2.7.4

[PATCH v7 6/6] platform/x86: intel_pmc_ipc: use gcr mem base for S0ix counter read

[Kuppuswamy Sathyanarayanan]

To maintain the uniformity in accessing GCR registers, this patch
modifies the S0ix counter read function to use GCR address base
instead of ipc address base.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
Tested-by: Shanth Murthy <shanth.murthy@intel.com>
---
 arch/x86/include/asm/intel_pmc_ipc.h |  2 ++
 drivers/platform/x86/intel_pmc_ipc.c | 10 +++-------
 2 files changed, 5 insertions(+), 7 deletions(-)

Changes since v6:
 * None

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
index 8402efe..fac89eb 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -25,6 +25,8 @@
 
 /* GCR reg offsets from gcr base*/
 #define PMC_GCR_PMC_CFG_REG		0x08
+#define PMC_GCR_TELEM_DEEP_S0IX_REG	0x78
+#define PMC_GCR_TELEM_SHLW_S0IX_REG	0x80
 
 #if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
 
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 0a39b0f..e4d4dfe 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -57,10 +57,6 @@
 #define IPC_WRITE_BUFFER	0x80
 #define IPC_READ_BUFFER		0x90
 
-/* PMC Global Control Registers */
-#define GCR_TELEM_DEEP_S0IX_OFFSET	0x1078
-#define GCR_TELEM_SHLW_S0IX_OFFSET	0x1080
-
 /* Residency with clock rate at 19.2MHz to usecs */
 #define S0IX_RESIDENCY_IN_USECS(d, s)		\
 ({						\
@@ -202,7 +198,7 @@ static inline u32 ipc_data_readl(u32 offset)
 
 static inline u64 gcr_data_readq(u32 offset)
 {
-	return readq(ipcdev.ipc_base + offset);
+	return readq(ipcdev.gcr_mem_base + offset);
 }
 
 static inline int is_gcr_valid(u32 offset)
@@ -902,8 +898,8 @@ int intel_pmc_s0ix_counter_read(u64 *data)
 	if (!ipcdev.has_gcr_regs)
 		return -EACCES;
 
-	deep = gcr_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
-	shlw = gcr_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
+	deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);
+	shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);
 
 	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
 
-- 
2.7.4

[PATCH v7 6/6] platform/x86: intel_pmc_ipc: use gcr mem base for S0ix counter read

[Kuppuswamy Sathyanarayanan]

To maintain the uniformity in accessing GCR registers, this patch
modifies the S0ix counter read function to use GCR address base
instead of ipc address base.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Reviewed-by: Rajneesh Bhardwaj <rajneesh.bhardwaj-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Tested-by: Shanth Murthy <shanth.murthy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/include/asm/intel_pmc_ipc.h |  2 ++
 drivers/platform/x86/intel_pmc_ipc.c | 10 +++-------
 2 files changed, 5 insertions(+), 7 deletions(-)

Changes since v6:
 * None

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
index 8402efe..fac89eb 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -25,6 +25,8 @@
 
 /* GCR reg offsets from gcr base*/
 #define PMC_GCR_PMC_CFG_REG		0x08
+#define PMC_GCR_TELEM_DEEP_S0IX_REG	0x78
+#define PMC_GCR_TELEM_SHLW_S0IX_REG	0x80
 
 #if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
 
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 0a39b0f..e4d4dfe 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -57,10 +57,6 @@
 #define IPC_WRITE_BUFFER	0x80
 #define IPC_READ_BUFFER		0x90
 
-/* PMC Global Control Registers */
-#define GCR_TELEM_DEEP_S0IX_OFFSET	0x1078
-#define GCR_TELEM_SHLW_S0IX_OFFSET	0x1080
-
 /* Residency with clock rate at 19.2MHz to usecs */
 #define S0IX_RESIDENCY_IN_USECS(d, s)		\
 ({						\
@@ -202,7 +198,7 @@ static inline u32 ipc_data_readl(u32 offset)
 
 static inline u64 gcr_data_readq(u32 offset)
 {
-	return readq(ipcdev.ipc_base + offset);
+	return readq(ipcdev.gcr_mem_base + offset);
 }
 
 static inline int is_gcr_valid(u32 offset)
@@ -902,8 +898,8 @@ int intel_pmc_s0ix_counter_read(u64 *data)
 	if (!ipcdev.has_gcr_regs)
 		return -EACCES;
 
-	deep = gcr_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
-	shlw = gcr_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
+	deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);
+	shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);
 
 	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 6/6] platform/x86: intel_pmc_ipc: use gcr mem base for S0ix counter read

[Kuppuswamy Sathyanarayanan]

To maintain the uniformity in accessing GCR registers, this patch
modifies the S0ix counter read function to use GCR address base
instead of ipc address base.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
Tested-by: Shanth Murthy <shanth.murthy@intel.com>
---
 arch/x86/include/asm/intel_pmc_ipc.h |  2 ++
 drivers/platform/x86/intel_pmc_ipc.c | 10 +++-------
 2 files changed, 5 insertions(+), 7 deletions(-)

Changes since v6:
 * None

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
index 8402efe..fac89eb 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -25,6 +25,8 @@
 
 /* GCR reg offsets from gcr base*/
 #define PMC_GCR_PMC_CFG_REG		0x08
+#define PMC_GCR_TELEM_DEEP_S0IX_REG	0x78
+#define PMC_GCR_TELEM_SHLW_S0IX_REG	0x80
 
 #if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
 
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 0a39b0f..e4d4dfe 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -57,10 +57,6 @@
 #define IPC_WRITE_BUFFER	0x80
 #define IPC_READ_BUFFER		0x90
 
-/* PMC Global Control Registers */
-#define GCR_TELEM_DEEP_S0IX_OFFSET	0x1078
-#define GCR_TELEM_SHLW_S0IX_OFFSET	0x1080
-
 /* Residency with clock rate at 19.2MHz to usecs */
 #define S0IX_RESIDENCY_IN_USECS(d, s)		\
 ({						\
@@ -202,7 +198,7 @@ static inline u32 ipc_data_readl(u32 offset)
 
 static inline u64 gcr_data_readq(u32 offset)
 {
-	return readq(ipcdev.ipc_base + offset);
+	return readq(ipcdev.gcr_mem_base + offset);
 }
 
 static inline int is_gcr_valid(u32 offset)
@@ -902,8 +898,8 @@ int intel_pmc_s0ix_counter_read(u64 *data)
 	if (!ipcdev.has_gcr_regs)
 		return -EACCES;
 
-	deep = gcr_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
-	shlw = gcr_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
+	deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);
+	shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);
 
 	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
 
-- 
2.7.4

Re: [PATCH v7 1/6] platform/x86: intel_pmc_ipc: fix gcr offset

[Andy Shevchenko]

On Mon, Apr 10, 2017 at 1:00 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> According to Broxton APL spec, PMC MIMO resources for Global Control
> Registers(GCR) are located at 4K(0x1000) offset from IPC base address.
> In this driver, PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
> region base address from IPC base address and its current value of
> 0x1008 is incorrect because it points to location for PMC_CFG register
> and not the GCR base address itself.
>
> GCR Base = IPC1 Base + 0x1000.
>
> This patch fixes this offset issue.

The whole series has been pushed to testing, thanks.

>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_ipc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Changes since v6:
>  *  Updated commit message
>
> Changes since v5:
>  *  None
>
> Changes since v4:
>  *  None
>
> Changes since v3:
>  * Updated the commit history
>
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 0651d47..0a33592 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -82,7 +82,7 @@
>  /* exported resources from IFWI */
>  #define PLAT_RESOURCE_IPC_INDEX                0
>  #define PLAT_RESOURCE_IPC_SIZE         0x1000
> -#define PLAT_RESOURCE_GCR_OFFSET       0x1008
> +#define PLAT_RESOURCE_GCR_OFFSET       0x1000
>  #define PLAT_RESOURCE_GCR_SIZE         0x1000
>  #define PLAT_RESOURCE_BIOS_DATA_INDEX  1
>  #define PLAT_RESOURCE_BIOS_IFACE_INDEX 2
> --
> 2.7.4
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/6] platform/x86: intel_pmc_ipc: fix gcr offset

[Andy Shevchenko]

On Mon, Apr 10, 2017 at 1:00 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> According to Broxton APL spec, PMC MIMO resources for Global Control
> Registers(GCR) are located at 4K(0x1000) offset from IPC base address.
> In this driver, PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
> region base address from IPC base address and its current value of
> 0x1008 is incorrect because it points to location for PMC_CFG register
> and not the GCR base address itself.
>
> GCR Base = IPC1 Base + 0x1000.
>
> This patch fixes this offset issue.

The whole series has been pushed to testing, thanks.

>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_ipc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Changes since v6:
>  *  Updated commit message
>
> Changes since v5:
>  *  None
>
> Changes since v4:
>  *  None
>
> Changes since v3:
>  * Updated the commit history
>
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 0651d47..0a33592 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -82,7 +82,7 @@
>  /* exported resources from IFWI */
>  #define PLAT_RESOURCE_IPC_INDEX                0
>  #define PLAT_RESOURCE_IPC_SIZE         0x1000
> -#define PLAT_RESOURCE_GCR_OFFSET       0x1008
> +#define PLAT_RESOURCE_GCR_OFFSET       0x1000
>  #define PLAT_RESOURCE_GCR_SIZE         0x1000
>  #define PLAT_RESOURCE_BIOS_DATA_INDEX  1
>  #define PLAT_RESOURCE_BIOS_IFACE_INDEX 2
> --
> 2.7.4
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/6] platform/x86: intel_pmc_ipc: fix gcr offset

[sathyanarayanan kuppuswamy]

On 04/25/2017 06:29 AM, Andy Shevchenko wrote:
> On Mon, Apr 10, 2017 at 1:00 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> According to Broxton APL spec, PMC MIMO resources for Global Control
>> Registers(GCR) are located at 4K(0x1000) offset from IPC base address.
>> In this driver, PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
>> region base address from IPC base address and its current value of
>> 0x1008 is incorrect because it points to location for PMC_CFG register
>> and not the GCR base address itself.
>>
>> GCR Base = IPC1 Base + 0x1000.
>>
>> This patch fixes this offset issue.
> The whole series has been pushed to testing, thanks.
Thanks Andy.
>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel_pmc_ipc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Changes since v6:
>>   *  Updated commit message
>>
>> Changes since v5:
>>   *  None
>>
>> Changes since v4:
>>   *  None
>>
>> Changes since v3:
>>   * Updated the commit history
>>
>> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
>> index 0651d47..0a33592 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -82,7 +82,7 @@
>>   /* exported resources from IFWI */
>>   #define PLAT_RESOURCE_IPC_INDEX                0
>>   #define PLAT_RESOURCE_IPC_SIZE         0x1000
>> -#define PLAT_RESOURCE_GCR_OFFSET       0x1008
>> +#define PLAT_RESOURCE_GCR_OFFSET       0x1000
>>   #define PLAT_RESOURCE_GCR_SIZE         0x1000
>>   #define PLAT_RESOURCE_BIOS_DATA_INDEX  1
>>   #define PLAT_RESOURCE_BIOS_IFACE_INDEX 2
>> --
>> 2.7.4
>>
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v7 1/6] platform/x86: intel_pmc_ipc: fix gcr offset

[sathyanarayanan kuppuswamy]

On 04/25/2017 06:29 AM, Andy Shevchenko wrote:
> On Mon, Apr 10, 2017 at 1:00 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> According to Broxton APL spec, PMC MIMO resources for Global Control
>> Registers(GCR) are located at 4K(0x1000) offset from IPC base address.
>> In this driver, PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
>> region base address from IPC base address and its current value of
>> 0x1008 is incorrect because it points to location for PMC_CFG register
>> and not the GCR base address itself.
>>
>> GCR Base = IPC1 Base + 0x1000.
>>
>> This patch fixes this offset issue.
> The whole series has been pushed to testing, thanks.
Thanks Andy.
>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel_pmc_ipc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Changes since v6:
>>   *  Updated commit message
>>
>> Changes since v5:
>>   *  None
>>
>> Changes since v4:
>>   *  None
>>
>> Changes since v3:
>>   * Updated the commit history
>>
>> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
>> index 0651d47..0a33592 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -82,7 +82,7 @@
>>   /* exported resources from IFWI */
>>   #define PLAT_RESOURCE_IPC_INDEX                0
>>   #define PLAT_RESOURCE_IPC_SIZE         0x1000
>> -#define PLAT_RESOURCE_GCR_OFFSET       0x1008
>> +#define PLAT_RESOURCE_GCR_OFFSET       0x1000
>>   #define PLAT_RESOURCE_GCR_SIZE         0x1000
>>   #define PLAT_RESOURCE_BIOS_DATA_INDEX  1
>>   #define PLAT_RESOURCE_BIOS_IFACE_INDEX 2
>> --
>> 2.7.4
>>
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer