[PATCH v3 1/5] platform/x86: intel_pmc_ipc: fix gcr offset

[PATCH v3 1/5] platform/x86: intel_pmc_ipc: fix gcr offset

[Kuppuswamy Sathyanarayanan]

According to the PMC spec, gcr offset from ipc mem
region is 0x1000(4K). But currently this driver uses
0x1008 as gcr offset. This patch fixes this 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(-)

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 v3 2/5] 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 | 56 ++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

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..ea5579e 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -127,6 +127,7 @@ static struct intel_pmc_ipc_dev {
 
 	/* gcr */
 	resource_size_t gcr_base;
+	void __iomem *gcr_mem_base;
 	int gcr_size;
 	bool has_gcr_regs;
 
@@ -199,6 +200,60 @@ static inline u64 gcr_data_readq(u32 offset)
 	return readq(ipcdev.ipc_base + offset);
 }
 
+int intel_pmc_gcr_read(u32 offset, u32 *data)
+{
+	if (!ipcdev.has_gcr_regs)
+		return -EACCES;
+
+	if (offset > PLAT_RESOURCE_GCR_SIZE)
+		return -EINVAL;
+
+	*data = readl(ipcdev.gcr_mem_base + offset);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
+
+int intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	if (!ipcdev.has_gcr_regs)
+		return -EACCES;
+
+	if (offset > PLAT_RESOURCE_GCR_SIZE)
+		return -EINVAL;
+
+	writel(data, ipcdev.gcr_mem_base + offset);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
+
+int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
+{
+	u32 orig, tmp;
+
+	if (!ipcdev.has_gcr_regs)
+		return -EACCES;
+
+	if (offset > PLAT_RESOURCE_GCR_SIZE)
+		return -EINVAL;
+
+	orig = readl(ipcdev.gcr_mem_base + offset);
+
+	tmp = orig & ~mask;
+	tmp |= val & mask;
+
+	writel(tmp, ipcdev.gcr_mem_base + offset);
+
+	tmp = readl(ipcdev.gcr_mem_base + offset);
+
+	if ((tmp & mask) != (val & mask))
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -747,6 +802,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

Re: [PATCH v3 2/5] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's

[Rajneesh Bhardwaj]

On Fri, Mar 17, 2017 at 07:06:19PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 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 | 56 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+)
> 
> 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..ea5579e 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -127,6 +127,7 @@ static struct intel_pmc_ipc_dev {
>  
>  	/* gcr */
>  	resource_size_t gcr_base;
> +	void __iomem *gcr_mem_base;
>  	int gcr_size;
>  	bool has_gcr_regs;
>  
> @@ -199,6 +200,60 @@ static inline u64 gcr_data_readq(u32 offset)
>  	return readq(ipcdev.ipc_base + offset);
>  }
>  
> +int intel_pmc_gcr_read(u32 offset, u32 *data)
> +{
> +	if (!ipcdev.has_gcr_regs)
> +		return -EACCES;
> +
> +	if (offset > PLAT_RESOURCE_GCR_SIZE)
> +		return -EINVAL;
> +

we can have one function that checks the above conditions and to avoid
repetitions.

> +	*data = readl(ipcdev.gcr_mem_base + offset);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
> +
> +int intel_pmc_gcr_write(u32 offset, u32 data)
> +{
> +	if (!ipcdev.has_gcr_regs)
> +		return -EACCES;
> +
> +	if (offset > PLAT_RESOURCE_GCR_SIZE)
> +		return -EINVAL;
> +
> +	writel(data, ipcdev.gcr_mem_base + offset);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
> +
> +int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)

please add kernel-doc comments for this function that describes its intended
use case.

> +{
> +	u32 orig, tmp;
> +
> +	if (!ipcdev.has_gcr_regs)
> +		return -EACCES;
> +
> +	if (offset > PLAT_RESOURCE_GCR_SIZE)
> +		return -EINVAL;
> +
> +	orig = readl(ipcdev.gcr_mem_base + offset);
> +
> +	tmp = orig & ~mask;
> +	tmp |= val & mask;
> +
> +	writel(tmp, ipcdev.gcr_mem_base + offset);
> +
> +	tmp = readl(ipcdev.gcr_mem_base + offset);
> +
> +	if ((tmp & mask) != (val & mask))
> +		return -EIO;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
> +
>  static int intel_pmc_ipc_check_status(void)
>  {
>  	int status;
> @@ -747,6 +802,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
> 

-- 
Best Regards,
Rajneesh

[PATCH v3 3/5] 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>
---
 drivers/watchdog/iTCO_wdt.c            | 28 +++++++++++++++++++---------
 include/linux/platform_data/itco_wdt.h |  1 +
 2 files changed, 20 insertions(+), 9 deletions(-)

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 3d0abc0..7c34259 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -100,6 +100,8 @@ struct iTCO_wdt_private {
 	 */
 	struct resource *gcs_pmc_res;
 	unsigned long __iomem *gcs_pmc;
+	/* pmc specific api to update noreboot flag */
+	int (*update_noreboot_flag)(bool status);
 	/* the lock for io operations */
 	spinlock_t io_lock;
 	/* the PCI-device */
@@ -176,9 +178,13 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 
 	/* 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);
+		if (p->update_noreboot_flag)
+			p->update_noreboot_flag(1);
+		else {
+			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);
@@ -193,11 +199,14 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 
 	/* 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);
-
-		val32 = readl(p->gcs_pmc);
+		if (p->update_noreboot_flag)
+			return p->update_noreboot_flag(0);
+		else {
+			val32 = readl(p->gcs_pmc);
+			val32 &= ~enable_bit;
+			writel(val32, p->gcs_pmc);
+			val32 = readl(p->gcs_pmc);
+		}
 	} else if (p->iTCO_version == 1) {
 		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
 		val32 &= ~enable_bit;
@@ -426,13 +435,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	p->iTCO_version = pdata->version;
+	p->update_noreboot_flag = pdata->update_noreboot_flag;
 	p->pci_dev = to_pci_dev(dev->parent);
 
 	/*
 	 * 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 && !p->update_noreboot_flag) {
 		p->gcs_pmc_res = platform_get_resource(pdev,
 						       IORESOURCE_MEM,
 						       ICH_RES_MEM_GCS_PMC);
diff --git a/include/linux/platform_data/itco_wdt.h b/include/linux/platform_data/itco_wdt.h
index f16542c..ea1efb7 100644
--- a/include/linux/platform_data/itco_wdt.h
+++ b/include/linux/platform_data/itco_wdt.h
@@ -14,6 +14,7 @@
 struct itco_wdt_platform_data {
 	char name[32];
 	unsigned int version;
+	int (*update_noreboot_flag)(bool status);
 };
 
 #endif /* _ITCO_WDT_H_ */
-- 
2.7.4

Re: [v3,3/5] watchdog: iTCO_wdt: Add PMC specific noreboot update api

[Guenter Roeck]

On Fri, Mar 17, 2017 at 07:06:20PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 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>

Not entirely happy, but I don't have a better idea either.

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

> ---
>  drivers/watchdog/iTCO_wdt.c            | 28 +++++++++++++++++++---------
>  include/linux/platform_data/itco_wdt.h |  1 +
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> 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 3d0abc0..7c34259 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -100,6 +100,8 @@ struct iTCO_wdt_private {
>  	 */
>  	struct resource *gcs_pmc_res;
>  	unsigned long __iomem *gcs_pmc;
> +	/* pmc specific api to update noreboot flag */
> +	int (*update_noreboot_flag)(bool status);
>  	/* the lock for io operations */
>  	spinlock_t io_lock;
>  	/* the PCI-device */
> @@ -176,9 +178,13 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
>  
>  	/* 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);
> +		if (p->update_noreboot_flag)
> +			p->update_noreboot_flag(1);
> +		else {
> +			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);
> @@ -193,11 +199,14 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
>  
>  	/* 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);
> -
> -		val32 = readl(p->gcs_pmc);
> +		if (p->update_noreboot_flag)
> +			return p->update_noreboot_flag(0);
> +		else {
> +			val32 = readl(p->gcs_pmc);
> +			val32 &= ~enable_bit;
> +			writel(val32, p->gcs_pmc);
> +			val32 = readl(p->gcs_pmc);
> +		}
>  	} else if (p->iTCO_version == 1) {
>  		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
>  		val32 &= ~enable_bit;
> @@ -426,13 +435,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  
>  	p->iTCO_version = pdata->version;
> +	p->update_noreboot_flag = pdata->update_noreboot_flag;
>  	p->pci_dev = to_pci_dev(dev->parent);
>  
>  	/*
>  	 * 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 && !p->update_noreboot_flag) {
>  		p->gcs_pmc_res = platform_get_resource(pdev,
>  						       IORESOURCE_MEM,
>  						       ICH_RES_MEM_GCS_PMC);
> diff --git a/include/linux/platform_data/itco_wdt.h b/include/linux/platform_data/itco_wdt.h
> index f16542c..ea1efb7 100644
> --- a/include/linux/platform_data/itco_wdt.h
> +++ b/include/linux/platform_data/itco_wdt.h
> @@ -14,6 +14,7 @@
>  struct itco_wdt_platform_data {
>  	char name[32];
>  	unsigned int version;
> +	int (*update_noreboot_flag)(bool status);
>  };
>  
>  #endif /* _ITCO_WDT_H_ */

[PATCH v3 4/5] platform/x86: intel_pmc_ipc: Fix iTCO GCS memory mapping failure

[Kuppuswamy Sathyanarayanan]

iTCO watchdog driver need access to PMC_CFG GCR register to modify
the no reboot 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 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

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 ea5579e..0c66c11 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -126,7 +126,6 @@ static struct intel_pmc_ipc_dev {
 	struct platform_device *tco_dev;
 
 	/* gcr */
-	resource_size_t gcr_base;
 	void __iomem *gcr_mem_base;
 	int gcr_size;
 	bool has_gcr_regs;
@@ -254,6 +253,15 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
 
+static int update_noreboot_bit(bool status)
+{
+	if (status)
+		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4),
+					    BIT(4));
+	else
+		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4), 0);
+}
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -571,15 +579,12 @@ 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,
+	.update_noreboot_flag = update_noreboot_bit,
 };
 
 #define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
@@ -636,10 +641,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);
@@ -801,7 +802,6 @@ 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

Re: [PATCH v3 4/5] platform/x86: intel_pmc_ipc: Fix iTCO GCS memory mapping failure

[Rajneesh Bhardwaj]

On Fri, Mar 17, 2017 at 07:06:21PM -0700, Kuppuswamy Sathyanarayanan wrote:
> iTCO watchdog driver need access to PMC_CFG GCR register to modify
> the no reboot 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 | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> 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 ea5579e..0c66c11 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -126,7 +126,6 @@ static struct intel_pmc_ipc_dev {
>  	struct platform_device *tco_dev;
>  
>  	/* gcr */
> -	resource_size_t gcr_base;
>  	void __iomem *gcr_mem_base;
>  	int gcr_size;
>  	bool has_gcr_regs;
> @@ -254,6 +253,15 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
>  }
>  EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
>  
> +static int update_noreboot_bit(bool status)
> +{
> +	if (status)
> +		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4),
> +					    BIT(4));
> +	else
> +		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4), 0);
> +}
> +

I think bit 4 will hold good only when TCO version is 3. What about the
other cases? Tested it on APL and it worked fine.

>  static int intel_pmc_ipc_check_status(void)
>  {
>  	int status;
> @@ -571,15 +579,12 @@ 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,
> +	.update_noreboot_flag = update_noreboot_bit,
>  };
>  
>  #define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
> @@ -636,10 +641,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);
> @@ -801,7 +802,6 @@ 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
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH v3 4/5] platform/x86: intel_pmc_ipc: Fix iTCO GCS memory mapping failure

[sathyanarayanan kuppuswamy]

On 03/31/2017 07:01 AM, Rajneesh Bhardwaj wrote:
> On Fri, Mar 17, 2017 at 07:06:21PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> iTCO watchdog driver need access to PMC_CFG GCR register to modify
>> the no reboot 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 | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> 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 ea5579e..0c66c11 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -126,7 +126,6 @@ static struct intel_pmc_ipc_dev {
>>   	struct platform_device *tco_dev;
>>   
>>   	/* gcr */
>> -	resource_size_t gcr_base;
>>   	void __iomem *gcr_mem_base;
>>   	int gcr_size;
>>   	bool has_gcr_regs;
>> @@ -254,6 +253,15 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
>>   }
>>   EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
>>   
>> +static int update_noreboot_bit(bool status)
>> +{
>> +	if (status)
>> +		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4),
>> +					    BIT(4));
>> +	else
>> +		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4), 0);
>> +}
>> +
> I think bit 4 will hold good only when TCO version is 3. What about the
> other cases? Tested it on APL and it worked fine.

Setting bit 4 will work for both version 3 and 5. Currently this driver 
only handles wdt device creation for APL platform. For APL, WDT version 
is 5 and hence this fix should work fine.

For other cases, currently iTCO_wdt driver still uses the memory mapped 
reboot setting.
>
>>   static int intel_pmc_ipc_check_status(void)
>>   {
>>   	int status;
>> @@ -571,15 +579,12 @@ 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,
>> +	.update_noreboot_flag = update_noreboot_bit,
>>   };
>>   
>>   #define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
>> @@ -636,10 +641,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);
>> @@ -801,7 +802,6 @@ 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
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

[PATCH v3 5/5] 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>
---
 arch/x86/include/asm/intel_pmc_ipc.h |  2 ++
 drivers/platform/x86/intel_pmc_ipc.c | 10 +++-------
 2 files changed, 5 insertions(+), 7 deletions(-)

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 0c66c11..54d5254 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)		\
 ({						\
@@ -196,7 +192,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);
 }
 
 int intel_pmc_gcr_read(u32 offset, u32 *data)
@@ -838,8 +834,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 v3 5/5] platform/x86: intel_pmc_ipc: use gcr mem base for S0ix counter read

[Rajneesh Bhardwaj]

On Fri, Mar 17, 2017 at 07:06:22PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 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.
>

looks fine to me.
 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.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(-)
> 
> 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 0c66c11..54d5254 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)		\
>  ({						\
> @@ -196,7 +192,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);
>  }
>  
>  int intel_pmc_gcr_read(u32 offset, u32 *data)
> @@ -838,8 +834,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
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH v3 5/5] platform/x86: intel_pmc_ipc: use gcr mem base for S0ix counter read

[Shanth Murthy]

On Fri, Mar 17, 2017 at 07:06:22PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 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.
> 

tested and verified, looks good to me.

> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.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(-)
> 
> 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 0c66c11..54d5254 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)		\
>  ({						\
> @@ -196,7 +192,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);
>  }
>  
>  int intel_pmc_gcr_read(u32 offset, u32 *data)
> @@ -838,8 +834,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 v3 1/5] platform/x86: intel_pmc_ipc: fix gcr offset

[Rajneesh Bhardwaj]

On Fri, Mar 17, 2017 at 07:06:18PM -0700, Kuppuswamy Sathyanarayanan wrote:
> According to the PMC spec, gcr offset from ipc mem

Which spec? We can just use generic terms for BXT/APL PMC.

> region is 0x1000(4K). But currently this driver uses
> 0x1008 as gcr offset. This patch fixes this issue.
>

Patch is good but i feel it's better to have little more explanation in the
commit message since the subject looks very similar to one old patch that
seems to have created the issue being fixed by this one.

Have a look at:
intel_pmc_ipc: Fix GCR register base address and length
 
> 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(-)
> 
> 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
> 

-- 
Best Regards,
Rajneesh

[PATCH v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset

[Kuppuswamy Sathyanarayanan]

According to Broxton APL PMC spec, gcr mem region starts
at offset 0x1000 from ipc mem base address. In this driver,
PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
memory region from IPC mem region. So we should use 0x1000(4K)
as GCR offset. But currently this driver uses 0x1008 as GCT
offset.This patch fixes this 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 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 v4 2/5] 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 | 95 ++++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+)

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..bc95bf2 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -127,6 +127,7 @@ static struct intel_pmc_ipc_dev {
 
 	/* gcr */
 	resource_size_t gcr_base;
+	void __iomem *gcr_mem_base;
 	int gcr_size;
 	bool has_gcr_regs;
 
@@ -199,6 +200,99 @@ 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;
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0)
+		return ret;
+
+	*data = readl(ipcdev.gcr_mem_base + offset);
+
+	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;
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0)
+		return ret;
+
+	writel(data, ipcdev.gcr_mem_base + offset);
+
+	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 orig, tmp;
+
+	tmp = is_gcr_valid(offset);
+	if (tmp < 0)
+		return tmp;
+
+	orig = readl(ipcdev.gcr_mem_base + offset);
+
+	tmp = orig & ~mask;
+	tmp |= val & mask;
+
+	writel(tmp, ipcdev.gcr_mem_base + offset);
+
+	tmp = readl(ipcdev.gcr_mem_base + offset);
+
+	if ((tmp & mask) != (val & mask))
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -747,6 +841,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

Re: [PATCH v4 2/5] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's

[Andy Shevchenko]

On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> This patch adds API's to read/write/update PMC GC registers.
> PMC dependent devices like iTCO_WDT, Telemetry has requirement

iTCO_wdt

> to acces GCR registers. These API's can be used for this
> purpose.

> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c

> +static inline int is_gcr_valid(u32 offset)

Pointer to ipcdev should be a parameter to this function.

> +{
> +       if (!ipcdev.has_gcr_regs)
> +               return -EACCES;
> +
> +       if (offset > PLAT_RESOURCE_GCR_SIZE)
> +               return -EINVAL;
> +
> +       return 0;
> +}

> +/**
> + * 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

-> * @mask and @val.

You would need to refresh how to use kernel doc.

> + *
> + * Return:     negative value on error or 0 on success.
> + */

With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/5] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's

[Sathyanarayanan Kuppuswamy Natarajan]

Hi Andy,

Thanks for your comments.

On Sun, Apr 2, 2017 at 6:58 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> This patch adds API's to read/write/update PMC GC registers.
>> PMC dependent devices like iTCO_WDT, Telemetry has requirement
>
> iTCO_wdt
will fix it in next version.
>
>> to acces GCR registers. These API's can be used for this
>> purpose.
>
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>
>> +static inline int is_gcr_valid(u32 offset)
>
> Pointer to ipcdev should be a parameter to this function.

But ipcdev is a static variable, visible across this file. So there is
no point in passing it as parameter.

I just noticed that I am not holding the mutex lock in these
functions. I will fix it in next version.

>
>> +{
>> +       if (!ipcdev.has_gcr_regs)
>> +               return -EACCES;
>> +
>> +       if (offset > PLAT_RESOURCE_GCR_SIZE)
>> +               return -EINVAL;
>> +
>> +       return 0;
>> +}
>
>> +/**
>> + * 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
>
> -> * @mask and @val.
>
> You would need to refresh how to use kernel doc.
-:) will fix it in next version.
>
>> + *
>> + * Return:     negative value on error or 0 on success.
>> + */
>
> With Best Regards,
> Andy Shevchenko



-- 
--

Sathya

Re: [PATCH v4 2/5] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's

[Andy Shevchenko]

On Mon, Apr 3, 2017 at 4:51 AM, Sathyanarayanan Kuppuswamy Natarajan
<sathyaosid@gmail.com> wrote:

>>> +static inline int is_gcr_valid(u32 offset)
>>
>> Pointer to ipcdev should be a parameter to this function.
>
> But ipcdev is a static variable, visible across this file. So there is
> no point in passing it as parameter.

That's one of refactoring needed for this library. You need to pass a
pointer to all internal functions and where it's possible to avoid
reference to a global variable.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/5] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's

[sathyanarayanan kuppuswamy]

Hi Andy,


On 04/04/2017 06:23 AM, Andy Shevchenko wrote:
> On Mon, Apr 3, 2017 at 4:51 AM, Sathyanarayanan Kuppuswamy Natarajan
> <sathyaosid@gmail.com> wrote:
>
>>>> +static inline int is_gcr_valid(u32 offset)
>>> Pointer to ipcdev should be a parameter to this function.
>> But ipcdev is a static variable, visible across this file. So there is
>> no point in passing it as parameter.
> That's one of refactoring needed for this library. You need to pass a
> pointer to all internal functions and where it's possible to avoid
> reference to a global variable.
Yes, I have included this into the list of issues that needs to fixed 
during the refactoring effort.
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

[PATCH v4 3/5] 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            | 28 +++++++++++++++++++---------
 include/linux/platform_data/itco_wdt.h |  1 +
 2 files changed, 20 insertions(+), 9 deletions(-)

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 3d0abc0..7c34259 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -100,6 +100,8 @@ struct iTCO_wdt_private {
 	 */
 	struct resource *gcs_pmc_res;
 	unsigned long __iomem *gcs_pmc;
+	/* pmc specific api to update noreboot flag */
+	int (*update_noreboot_flag)(bool status);
 	/* the lock for io operations */
 	spinlock_t io_lock;
 	/* the PCI-device */
@@ -176,9 +178,13 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 
 	/* 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);
+		if (p->update_noreboot_flag)
+			p->update_noreboot_flag(1);
+		else {
+			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);
@@ -193,11 +199,14 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 
 	/* 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);
-
-		val32 = readl(p->gcs_pmc);
+		if (p->update_noreboot_flag)
+			return p->update_noreboot_flag(0);
+		else {
+			val32 = readl(p->gcs_pmc);
+			val32 &= ~enable_bit;
+			writel(val32, p->gcs_pmc);
+			val32 = readl(p->gcs_pmc);
+		}
 	} else if (p->iTCO_version == 1) {
 		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
 		val32 &= ~enable_bit;
@@ -426,13 +435,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	p->iTCO_version = pdata->version;
+	p->update_noreboot_flag = pdata->update_noreboot_flag;
 	p->pci_dev = to_pci_dev(dev->parent);
 
 	/*
 	 * 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 && !p->update_noreboot_flag) {
 		p->gcs_pmc_res = platform_get_resource(pdev,
 						       IORESOURCE_MEM,
 						       ICH_RES_MEM_GCS_PMC);
diff --git a/include/linux/platform_data/itco_wdt.h b/include/linux/platform_data/itco_wdt.h
index f16542c..ea1efb7 100644
--- a/include/linux/platform_data/itco_wdt.h
+++ b/include/linux/platform_data/itco_wdt.h
@@ -14,6 +14,7 @@
 struct itco_wdt_platform_data {
 	char name[32];
 	unsigned int version;
+	int (*update_noreboot_flag)(bool status);
 };
 
 #endif /* _ITCO_WDT_H_ */
-- 
2.7.4

Re: [PATCH v4 3/5] watchdog: iTCO_wdt: Add PMC specific noreboot update api

[Andy Shevchenko]

On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> In some SOCs, setting noreboot bit needs modification to

SoCs.

Perhaps you can create a wikipage to share with your team what style
issues usually needs to be addressed.
One of them is a proper capitalization in abbreviations / code names.

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

> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -100,6 +100,8 @@ struct iTCO_wdt_private {
>          */
>         struct resource *gcs_pmc_res;
>         unsigned long __iomem *gcs_pmc;

> +       /* pmc specific api to update noreboot flag */

PMC
API

> +       int (*update_noreboot_flag)(bool status);
>         /* the lock for io operations */
>         spinlock_t io_lock;
>         /* the PCI-device */
> @@ -176,9 +178,13 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
>
>         /* 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);
> +               if (p->update_noreboot_flag)

> +                       p->update_noreboot_flag(1);

1 -> true for sake of consistency.

> +               else {
> +                       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);
> @@ -193,11 +199,14 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
>
>         /* 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);
> -
> -               val32 = readl(p->gcs_pmc);
> +               if (p->update_noreboot_flag)

> +                       return p->update_noreboot_flag(0);

0 -> false.

> +               else {

> +                       val32 = readl(p->gcs_pmc);
> +                       val32 &= ~enable_bit;
> +                       writel(val32, p->gcs_pmc);
> +                       val32 = readl(p->gcs_pmc);

This and similar above code might be split to a helper and you may
assign it once. In such case you will not need a special flag anymore.

Helpers split might be done as a preparatory separate patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/5] watchdog: iTCO_wdt: Add PMC specific noreboot update api

[Sathyanarayanan Kuppuswamy Natarajan]

Hi,

On Sun, Apr 2, 2017 at 7:04 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> In some SOCs, setting noreboot bit needs modification to
>
> SoCs.
>
> Perhaps you can create a wikipage to share with your team what style
> issues usually needs to be addressed.
> One of them is a proper capitalization in abbreviations / code names.
>
>> 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.
>
>> --- a/drivers/watchdog/iTCO_wdt.c
>> +++ b/drivers/watchdog/iTCO_wdt.c
>> @@ -100,6 +100,8 @@ struct iTCO_wdt_private {
>>          */
>>         struct resource *gcs_pmc_res;
>>         unsigned long __iomem *gcs_pmc;
>
>> +       /* pmc specific api to update noreboot flag */
>
> PMC
> API
will fix in next version.
>
>> +       int (*update_noreboot_flag)(bool status);
>>         /* the lock for io operations */
>>         spinlock_t io_lock;
>>         /* the PCI-device */
>> @@ -176,9 +178,13 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
>>
>>         /* 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);
>> +               if (p->update_noreboot_flag)
>
>> +                       p->update_noreboot_flag(1);
>
> 1 -> true for sake of consistency.
ditto
>
>> +               else {
>> +                       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);
>> @@ -193,11 +199,14 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
>>
>>         /* 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);
>> -
>> -               val32 = readl(p->gcs_pmc);
>> +               if (p->update_noreboot_flag)
>
>> +                       return p->update_noreboot_flag(0);
>
> 0 -> false.
ditto
>
>> +               else {
>
>> +                       val32 = readl(p->gcs_pmc);
>> +                       val32 &= ~enable_bit;
>> +                       writel(val32, p->gcs_pmc);
>> +                       val32 = readl(p->gcs_pmc);
>
> This and similar above code might be split to a helper and you may
> assign it once. In such case you will not need a special flag anymore.
>
> Helpers split might be done as a preparatory separate patch.
Yes, will create a patch for fixing it.
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
--

Sathya

[PATCH v4 4/5] platform/x86: intel_pmc_ipc: Fix iTCO GCS memory mapping failure

[Kuppuswamy Sathyanarayanan]

iTCO watchdog driver need access to PMC_CFG GCR register to modify
the no reboot 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 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

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 bc95bf2..3f3ee50 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -126,7 +126,6 @@ static struct intel_pmc_ipc_dev {
 	struct platform_device *tco_dev;
 
 	/* gcr */
-	resource_size_t gcr_base;
 	void __iomem *gcr_mem_base;
 	int gcr_size;
 	bool has_gcr_regs;
@@ -293,6 +292,15 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
 
+static int update_noreboot_bit(bool status)
+{
+	if (status)
+		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4),
+					    BIT(4));
+	else
+		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4), 0);
+}
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -610,15 +618,12 @@ 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,
+	.update_noreboot_flag = update_noreboot_bit,
 };
 
 #define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
@@ -675,10 +680,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);
@@ -840,7 +841,6 @@ 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

Re: [PATCH v4 4/5] platform/x86: intel_pmc_ipc: Fix iTCO GCS memory mapping failure

[Andy Shevchenko]

On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> iTCO watchdog driver need access to PMC_CFG GCR register to modify

iTCO_wdt or use above in the rest of the series.

So, choose one and use it everywhere in your patch series.

> the no reboot 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.


> +static int update_noreboot_bit(bool status)
> +{
> +       if (status)

> +               return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4),
> +                                           BIT(4));

BIT(4) is a magic. Moreover, it's used as mask and value here, you
might introduce temporary variables for better understanding what is
what.
As an example you may look at drivers/acpi/acpi_lpss.c (code related
to IOSF MBI interaction).

> +       else

Redundant.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/5] platform/x86: intel_pmc_ipc: Fix iTCO GCS memory mapping failure

[Sathyanarayanan Kuppuswamy Natarajan]

Hi Andy,



On Sun, Apr 2, 2017 at 7:10 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> iTCO watchdog driver need access to PMC_CFG GCR register to modify
>
> iTCO_wdt or use above in the rest of the series.
>
> So, choose one and use it everywhere in your patch series.

will go with iTCO_wdt.

>
>> the no reboot 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.
>
>
>> +static int update_noreboot_bit(bool status)
>> +{
>> +       if (status)
>
>> +               return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4),
>> +                                           BIT(4));
>
> BIT(4) is a magic. Moreover, it's used as mask and value here, you
> might introduce temporary variables for better understanding what is
> what.
> As an example you may look at drivers/acpi/acpi_lpss.c (code related
> to IOSF MBI interaction).

I will create macros with some meaningful name to explain what BIT(4)
represents.

>
>> +       else
>
> Redundant.
>
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
--

Sathya

[PATCH v4 5/5] 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 v3:
 * Rebased on top of latest changes.

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 3f3ee50..c7b5517 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)		\
 ({						\
@@ -196,7 +192,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)
@@ -877,8 +873,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 v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset

[Andy Shevchenko]

On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> According to Broxton APL PMC spec, gcr mem region starts
> at offset 0x1000 from ipc mem base address. In this driver,
> PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
> memory region from IPC mem region. So we should use 0x1000(4K)
> as GCR offset. But currently this driver uses 0x1008 as GCT
> offset.This patch fixes this issue.


So, if I apply this one independently, would it fix an existin 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 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 v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset

[Sathyanarayanan Kuppuswamy Natarajan]

Yes, just applying this patch will fix the existing offset issue.

On Sun, Apr 2, 2017 at 7:11 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> According to Broxton APL PMC spec, gcr mem region starts
>> at offset 0x1000 from ipc mem base address. In this driver,
>> PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
>> memory region from IPC mem region. So we should use 0x1000(4K)
>> as GCR offset. But currently this driver uses 0x1008 as GCT
>> offset.This patch fixes this issue.
>
>
> So, if I apply this one independently, would it fix an existin 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 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



-- 
--

Sathya

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

[Kuppuswamy Sathyanarayanan]

According to Broxton APL PMC spec, gcr mem region starts
at offset 0x1000 from ipc mem base address. In this driver,
PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
memory region from IPC mem region. So we should use 0x1000(4K)
as GCR offset. But currently this driver uses 0x1008 as GCT
offset.This patch fixes this 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 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 v5 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 | 114 +++++++++++++++++++++++++++++++++++
 2 files changed, 135 insertions(+)

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..8b7fef0 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -127,6 +127,7 @@ static struct intel_pmc_ipc_dev {
 
 	/* gcr */
 	resource_size_t gcr_base;
+	void __iomem *gcr_mem_base;
 	int gcr_size;
 	bool has_gcr_regs;
 
@@ -199,6 +200,118 @@ 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 orig, tmp;
+	int ret = 0;
+
+	mutex_lock(&ipclock);
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0)
+		goto gcr_update_err;
+
+	orig = readl(ipcdev.gcr_mem_base + offset);
+
+	tmp = orig & ~mask;
+	tmp |= val & mask;
+
+	writel(tmp, ipcdev.gcr_mem_base + offset);
+
+	tmp = readl(ipcdev.gcr_mem_base + offset);
+
+	if ((tmp & mask) != (val & mask)) {
+		ret = -EIO;
+		goto gcr_update_err;
+	}
+
+gcr_update_err:
+	mutex_unlock(&ipclock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -747,6 +860,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

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

[Andy Shevchenko]

On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 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.

> +/* GCR reg offsets from gcr base*/
> +#define PMC_GCR_PMC_CFG_REG            0x08
> +

> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -127,6 +127,7 @@ static struct intel_pmc_ipc_dev {
>
>         /* gcr */
>         resource_size_t gcr_base;

> +       void __iomem *gcr_mem_base;
>         int gcr_size;

Rearrange those lines to make __iomem pointer latter.

> +static inline int is_gcr_valid(u32 offset)

Same comment as previously. It should take a pointer to struct
intel_pmc_ipc_dev as a parameter.

> +/**
> + * 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

You have to use proper punctuation in the sentences in full Description section.

> +/**
> + * 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

Ditto.

> +int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
> +{

> +       u32 orig, tmp;

One of them is redundant.

I would go just with
u32 value;

> +       writel(tmp, ipcdev.gcr_mem_base + offset);
> +
> +       tmp = readl(ipcdev.gcr_mem_base + offset);

> +

Redundant.

> +       if ((tmp & mask) != (val & mask)) {

Why?! It the a case when writel() will fail? Needs to be commented.

> +               ret = -EIO;
> +               goto gcr_update_err;
> +       }
> +

> +gcr_update_err:

The keyword  'unlock' is missed in the label name.

> +       mutex_unlock(&ipclock);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);

-- 
With Best Regards,
Andy Shevchenko

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

[sathyanarayanan kuppuswamy]

Hi Andy,


On 04/04/2017 06:53 AM, Andy Shevchenko wrote:
> On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> 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.
>> +/* GCR reg offsets from gcr base*/
>> +#define PMC_GCR_PMC_CFG_REG            0x08
>> +
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -127,6 +127,7 @@ static struct intel_pmc_ipc_dev {
>>
>>          /* gcr */
>>          resource_size_t gcr_base;
>> +       void __iomem *gcr_mem_base;
>>          int gcr_size;
> Rearrange those lines to make __iomem pointer latter.
Will do it in next version. But the gcr_base resource pointer will be 
removed in next dependent patch. So I don't think it matters in the end.
>
>> +static inline int is_gcr_valid(u32 offset)
> Same comment as previously. It should take a pointer to struct
> intel_pmc_ipc_dev as a parameter.
This needs the driver cleanup. We can do it in refactoring patch.
>
>> +/**
>> + * 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
> You have to use proper punctuation in the sentences in full Description section.
Will fix it in next version.
>
>> +/**
>> + * 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
> Ditto.
>
>> +int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
>> +{
>> +       u32 orig, tmp;
> One of them is redundant.
>
> I would go just with
> u32 value;
Will fix it in next version.
>
>> +       writel(tmp, ipcdev.gcr_mem_base + offset);
>> +
>> +       tmp = readl(ipcdev.gcr_mem_base + offset);
>> +
> Redundant.
>
>> +       if ((tmp & mask) != (val & mask)) {
> Why?! It the a case when writel() will fail? Needs to be commented.
Yes, I am checking whether the update is successful or  not. I can add a 
comment there to explain it.
>
>> +               ret = -EIO;
>> +               goto gcr_update_err;
>> +       }
>> +
>> +gcr_update_err:
> The keyword  'unlock' is missed in the label name.
"goto" to this label is only used when there is an error in update 
operation. Do you think we should still rename it to gcr_ipc_unlock ?
>
>> +       mutex_unlock(&ipclock);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

[PATCH v5 3/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            | 28 +++++++++++++++++++---------
 include/linux/platform_data/itco_wdt.h |  1 +
 2 files changed, 20 insertions(+), 9 deletions(-)

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 3d0abc0..521ae95 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -100,6 +100,8 @@ struct iTCO_wdt_private {
 	 */
 	struct resource *gcs_pmc_res;
 	unsigned long __iomem *gcs_pmc;
+	/* PMC specific API to update noreboot flag */
+	int (*update_noreboot_flag)(bool status);
 	/* the lock for io operations */
 	spinlock_t io_lock;
 	/* the PCI-device */
@@ -176,9 +178,13 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 
 	/* 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);
+		if (p->update_noreboot_flag)
+			p->update_noreboot_flag(true);
+		else {
+			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);
@@ -193,11 +199,14 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
 
 	/* 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);
-
-		val32 = readl(p->gcs_pmc);
+		if (p->update_noreboot_flag)
+			return p->update_noreboot_flag(false);
+		else {
+			val32 = readl(p->gcs_pmc);
+			val32 &= ~enable_bit;
+			writel(val32, p->gcs_pmc);
+			val32 = readl(p->gcs_pmc);
+		}
 	} else if (p->iTCO_version == 1) {
 		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
 		val32 &= ~enable_bit;
@@ -426,13 +435,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	p->iTCO_version = pdata->version;
+	p->update_noreboot_flag = pdata->update_noreboot_flag;
 	p->pci_dev = to_pci_dev(dev->parent);
 
 	/*
 	 * 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 && !p->update_noreboot_flag) {
 		p->gcs_pmc_res = platform_get_resource(pdev,
 						       IORESOURCE_MEM,
 						       ICH_RES_MEM_GCS_PMC);
diff --git a/include/linux/platform_data/itco_wdt.h b/include/linux/platform_data/itco_wdt.h
index f16542c..ea1efb7 100644
--- a/include/linux/platform_data/itco_wdt.h
+++ b/include/linux/platform_data/itco_wdt.h
@@ -14,6 +14,7 @@
 struct itco_wdt_platform_data {
 	char name[32];
 	unsigned int version;
+	int (*update_noreboot_flag)(bool status);
 };
 
 #endif /* _ITCO_WDT_H_ */
-- 
2.7.4

Re: [PATCH v5 3/6] watchdog: iTCO_wdt: Add PMC specific noreboot update api

[Andy Shevchenko]

On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 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.

>         /* 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);
> +               if (p->update_noreboot_flag)
> +                       p->update_noreboot_flag(true);
> +               else {
> +                       val32 = readl(p->gcs_pmc);
> +                       val32 |= no_reboot_bit(p);
> +                       writel(val32, p->gcs_pmc);
> +               }

What I meant is to introduce helper(s) first and then fix this. Yes, I
understand it might increase work for backporting (if even needed),
but I still think it worth doing.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 4/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot calls

[Kuppuswamy Sathyanarayanan]

Both iTCO_wdt_unset_NO_REBOOT_bit() and iTCO_wdt_unset_NO_REBOOT_bit()
functions has lot of common code between them. So merging these two
functions would remove these unnecessary code duplications. This patch
fixes this issue by creating single update function to handle both
set/unset functionalities.

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

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 521ae95..cddfa00 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -172,50 +172,35 @@ 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 iTCO_wdt_update_no_reboot_flag(struct iTCO_wdt_private *p,
+					  bool status)
 {
-	u32 val32;
+	u32 val32 = 0, newval32 = 0;
 
-	/* Set the NO_REBOOT bit: this disables reboots */
 	if (p->iTCO_version >= 2) {
 		if (p->update_noreboot_flag)
-			p->update_noreboot_flag(true);
+			return p->update_noreboot_flag(status);
 		else {
 			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);
-	}
-}
-
-static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
-{
-	u32 enable_bit = no_reboot_bit(p);
-	u32 val32 = 0;
+			if (status)
+				val32 |= no_reboot_bit(p);
+			else
+				val32 &= ~no_reboot_bit(p);
 
-	/* Unset the NO_REBOOT bit: this enables reboots */
-	if (p->iTCO_version >= 2) {
-		if (p->update_noreboot_flag)
-			return p->update_noreboot_flag(false);
-		else {
-			val32 = readl(p->gcs_pmc);
-			val32 &= ~enable_bit;
 			writel(val32, p->gcs_pmc);
-			val32 = readl(p->gcs_pmc);
+			newval32 = readl(p->gcs_pmc);
 		}
 	} else if (p->iTCO_version == 1) {
 		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-		val32 &= ~enable_bit;
+		if (status)
+			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, &val32);
+		pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
 	}
 
-	if (val32 & enable_bit)
+	if (val32 != newval32)
 		return -EIO;
 
 	return 0;
@@ -231,7 +216,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 (iTCO_wdt_update_no_reboot_flag(p, false)) {
 		spin_unlock(&p->io_lock);
 		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
 		return -EIO;
@@ -272,7 +257,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);
+	iTCO_wdt_update_no_reboot_flag(p, true);
 
 	spin_unlock(&p->io_lock);
 
@@ -452,14 +437,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 (iTCO_wdt_update_no_reboot_flag(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);
+	iTCO_wdt_update_no_reboot_flag(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 v5 4/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot calls

[Guenter Roeck]

On 04/03/2017 05:24 PM, Kuppuswamy Sathyanarayanan wrote:
> Both iTCO_wdt_unset_NO_REBOOT_bit() and iTCO_wdt_unset_NO_REBOOT_bit()

I think you mean s/set/unset/ once.

> functions has lot of common code between them. So merging these two
> functions would remove these unnecessary code duplications. This patch
> fixes this issue by creating single update function to handle both
> set/unset functionalities.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/watchdog/iTCO_wdt.c | 53 ++++++++++++++++-----------------------------
>  1 file changed, 19 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 521ae95..cddfa00 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -172,50 +172,35 @@ 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 iTCO_wdt_update_no_reboot_flag(struct iTCO_wdt_private *p,

Why not stick with bit ?

> +					  bool status)

I think 'set' would be better than 'status'.

>  {
> -	u32 val32;
> +	u32 val32 = 0, newval32 = 0;
>
> -	/* Set the NO_REBOOT bit: this disables reboots */
>  	if (p->iTCO_version >= 2) {
>  		if (p->update_noreboot_flag)
> -			p->update_noreboot_flag(true);
> +			return p->update_noreboot_flag(status);
>  		else {
>  			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);
> -	}
> -}
> -
> -static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
> -{
> -	u32 enable_bit = no_reboot_bit(p);
> -	u32 val32 = 0;
> +			if (status)
> +				val32 |= no_reboot_bit(p);
> +			else
> +				val32 &= ~no_reboot_bit(p);
>
> -	/* Unset the NO_REBOOT bit: this enables reboots */
> -	if (p->iTCO_version >= 2) {
> -		if (p->update_noreboot_flag)
> -			return p->update_noreboot_flag(false);
> -		else {
> -			val32 = readl(p->gcs_pmc);
> -			val32 &= ~enable_bit;
>  			writel(val32, p->gcs_pmc);
> -			val32 = readl(p->gcs_pmc);
> +			newval32 = readl(p->gcs_pmc);
>  		}
>  	} else if (p->iTCO_version == 1) {
>  		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 &= ~enable_bit;
> +		if (status)
> +			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, &val32);
> +		pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
>  	}
>
> -	if (val32 & enable_bit)
> +	if (val32 != newval32)
>  		return -EIO;
>
>  	return 0;
> @@ -231,7 +216,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 (iTCO_wdt_update_no_reboot_flag(p, false)) {
>  		spin_unlock(&p->io_lock);
>  		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
>  		return -EIO;
> @@ -272,7 +257,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);
> +	iTCO_wdt_update_no_reboot_flag(p, true);
>
>  	spin_unlock(&p->io_lock);
>
> @@ -452,14 +437,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 (iTCO_wdt_update_no_reboot_flag(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);
> +	iTCO_wdt_update_no_reboot_flag(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 v5 4/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot calls

[Andy Shevchenko]

On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> Both iTCO_wdt_unset_NO_REBOOT_bit() and iTCO_wdt_unset_NO_REBOOT_bit()
> functions has lot of common code between them. So merging these two
> functions would remove these unnecessary code duplications. This patch
> fixes this issue by creating single update function to handle both
> set/unset functionalities.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Something similar should go before patch 3/6.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 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 | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

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 8b7fef0..3d0d6f17 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		BIT(4)
+#define PMC_CFG_NO_REBOOT_ENABLE	BIT(4)
+#define PMC_CFG_NO_REBOOT_DISABLE	0
+
 static struct intel_pmc_ipc_dev {
 	struct device *dev;
 	void __iomem *ipc_base;
@@ -126,7 +133,6 @@ static struct intel_pmc_ipc_dev {
 	struct platform_device *tco_dev;
 
 	/* gcr */
-	resource_size_t gcr_base;
 	void __iomem *gcr_mem_base;
 	int gcr_size;
 	bool has_gcr_regs;
@@ -312,6 +318,18 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
 
+static int update_noreboot_bit(bool status)
+{
+	if (status)
+		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
+					    PMC_CFG_NO_REBOOT_MASK,
+					    PMC_CFG_NO_REBOOT_ENABLE);
+
+	return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
+				    PMC_CFG_NO_REBOOT_MASK,
+				    PMC_CFG_NO_REBOOT_DISABLE);
+}
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -629,15 +647,12 @@ 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,
+	.update_noreboot_flag = update_noreboot_bit,
 };
 
 #define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
@@ -694,10 +709,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);
@@ -859,7 +870,6 @@ 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

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

[Andy Shevchenko]

On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 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

api -> API

> to watchdog driver via platform data.

This one looks good.

> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 8b7fef0..3d0d6f17 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         BIT(4)
> +#define PMC_CFG_NO_REBOOT_ENABLE       BIT(4)
> +#define PMC_CFG_NO_REBOOT_DISABLE      0
> +
>  static struct intel_pmc_ipc_dev {
>         struct device *dev;
>         void __iomem *ipc_base;
> @@ -126,7 +133,6 @@ static struct intel_pmc_ipc_dev {
>         struct platform_device *tco_dev;
>
>         /* gcr */
> -       resource_size_t gcr_base;
>         void __iomem *gcr_mem_base;
>         int gcr_size;
>         bool has_gcr_regs;
> @@ -312,6 +318,18 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
>  }
>  EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
>
> +static int update_noreboot_bit(bool status)
> +{
> +       if (status)
> +               return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
> +                                           PMC_CFG_NO_REBOOT_MASK,
> +                                           PMC_CFG_NO_REBOOT_ENABLE);
> +
> +       return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
> +                                   PMC_CFG_NO_REBOOT_MASK,
> +                                   PMC_CFG_NO_REBOOT_DISABLE);
> +}
> +
>  static int intel_pmc_ipc_check_status(void)
>  {
>         int status;
> @@ -629,15 +647,12 @@ 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,
> +       .update_noreboot_flag = update_noreboot_bit,
>  };
>
>  #define TELEMETRY_RESOURCE_PUNIT_SSRAM 0
> @@ -694,10 +709,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);
> @@ -859,7 +870,6 @@ 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);

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 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 v4:
 * Rebased on top of latest changes.

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

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 3d0d6f17..b61f569 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)		\
 ({						\
@@ -203,7 +199,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)
@@ -906,8 +902,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 v5 6/6] platform/x86: intel_pmc_ipc: use gcr mem base for S0ix counter read

[Andy Shevchenko]

On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 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.
>

This one looks good.

--- 8< --- 8< ---

Overall, I do not like this spreading interface where we have more and
more functions which use some global variable without clear
understanding of device / initialization dependencies.

Better approach would be something using opaque pointers and API which
takes it as an input.
I didn't investigate yet about something similar to UCLASS_SYSCON
(this is nice model of dependency used in U-Boot) in Linux kernel, it
would be cool to use it.

So, the priority number #2 (number #1 is to fix interrupt handling for
Whiskey Cove) is to refactor this all mess.

Like we agreed it would be last series I'm going to apply without
refactoring done.

--- 8< --- 8< ---

> 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 v4:
>  * Rebased on top of latest changes.
>
> Changes since v3:
>  * Rebased on top of latest changes.
>
> 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 3d0d6f17..b61f569 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)          \
>  ({                                             \
> @@ -203,7 +199,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)
> @@ -906,8 +902,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
>



-- 
With Best Regards,
Andy Shevchenko

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

[sathyanarayanan kuppuswamy]

Hi Andy,


On 04/04/2017 06:51 AM, Andy Shevchenko wrote:
> On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> 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.
>>
> This one looks good.
>
> --- 8< --- 8< ---
>
> Overall, I do not like this spreading interface where we have more and
> more functions which use some global variable without clear
> understanding of device / initialization dependencies.
Agreed. This is added to list of issues that need to be fixed during the 
refactoring effort.
>
> Better approach would be something using opaque pointers and API which
> takes it as an input.
> I didn't investigate yet about something similar to UCLASS_SYSCON
> (this is nice model of dependency used in U-Boot) in Linux kernel, it
> would be cool to use it.
Will look into this and see whether we can use this design in our code..
>
> So, the priority number #2 (number #1 is to fix interrupt handling for
> Whiskey Cove) is to refactor this all mess.
I have created patches to fix #1, I am in process of testing them. Once 
I am satisfied , I will send it for review.

For #2, myself and Rajnessh created a initial proposal for refactoring 
task and we will share it with you in few days.
>
> Like we agreed it would be last series I'm going to apply without
> refactoring done.
:) Its our deal. Thanks.
>
> --- 8< --- 8< ---
>
>> 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 v4:
>>   * Rebased on top of latest changes.
>>
>> Changes since v3:
>>   * Rebased on top of latest changes.
>>
>> 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 3d0d6f17..b61f569 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)          \
>>   ({                                             \
>> @@ -203,7 +199,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)
>> @@ -906,8 +902,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
>>
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset

[Andy Shevchenko]

Please, STOP top-posting.

On Mon, Apr 3, 2017 at 4:51 AM, Sathyanarayanan Kuppuswamy Natarajan
<sathyaosid@gmail.com> wrote:
> Yes, just applying this patch will fix the existing offset issue.

Then the question how it was tested in both cases (before this change and now) ?

>
> On Sun, Apr 2, 2017 at 7:11 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>> According to Broxton APL PMC spec, gcr mem region starts
>>> at offset 0x1000 from ipc mem base address. In this driver,
>>> PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
>>> memory region from IPC mem region. So we should use 0x1000(4K)
>>> as GCR offset. But currently this driver uses 0x1008 as GCT
>>> offset.This patch fixes this issue.
>>
>>
>> So, if I apply this one independently, would it fix an existin 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 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
>
>
>
> --
> --
>
> Sathya



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset

[sathyanarayanan kuppuswamy]

On 04/04/2017 06:25 AM, Andy Shevchenko wrote:
> Please, STOP top-posting.
>
> On Mon, Apr 3, 2017 at 4:51 AM, Sathyanarayanan Kuppuswamy Natarajan
> <sathyaosid@gmail.com> wrote:
>> Yes, just applying this patch will fix the existing offset issue.
> Then the question how it was tested in both cases (before this change and now) ?
iTCO_wdt is the only driver which uses this offset when calculating the 
address of PMC_CFG for setting/unsetting the no-reboot bit.

Without this change, instead of modifying the PMC_CFG(offset:0x1008) 
register, iTCO_wdt was modifying the address with offset 0x1010.
According to spec, offset 0x1010 is unused memory address. So modifying 
that area did not create any errors, but  its not functionally correct.

So with or without this change you will not see any error message. But 
its creating the functional issue of not modifying the no-reboot bit.

Currently, I tested this fix by duping the GCR registers and verified 
whether the no-reboot bit is modified or not.
>
>> On Sun, Apr 2, 2017 at 7:11 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>> According to Broxton APL PMC spec, gcr mem region starts
>>>> at offset 0x1000 from ipc mem base address. In this driver,
>>>> PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
>>>> memory region from IPC mem region. So we should use 0x1000(4K)
>>>> as GCR offset. But currently this driver uses 0x1008 as GCT
>>>> offset.This patch fixes this issue.
>>>
>>> So, if I apply this one independently, would it fix an existin 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 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
>>
>>
>> --
>> --
>>
>> Sathya
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

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

[Kuppuswamy Sathyanarayanan]

According to Broxton APL PMC spec, gcr mem region starts
at offset 0x1000 from ipc mem base address. In this driver,
PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
memory region from IPC mem region. So we should use 0x1000(4K)
as GCR offset. But currently this driver uses 0x1008 as GCT
offset.This patch fixes this 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 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 v6 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 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 v6 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 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..d8768a2 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 inline 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 inline 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 inline 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;
 }
 
+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 v6 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            | 20 ++++++++++++++------
 include/linux/platform_data/itco_wdt.h |  4 ++++
 2 files changed, 18 insertions(+), 6 deletions(-)

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 d8768a2..3f00029 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);
 };
@@ -225,6 +227,8 @@ void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
 		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 +241,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 +282,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);
 
@@ -442,14 +446,18 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 
 	p->iTCO_version = pdata->version;
 	p->pci_dev = to_pci_dev(dev->parent);
+	p->no_reboot_priv = pdata->no_reboot_priv;
 
-	iTCO_wdt_no_reboot_bit_setup(p);
+	if (pdata->update_no_reboot_bit)
+		p->update_no_reboot_bit = pdata->update_no_reboot_bit;
+	else
+		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).
 	 */
-	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 +467,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

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

[Guenter Roeck]

On 04/05/2017 03:54 PM, Kuppuswamy Sathyanarayanan wrote:
> 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            | 20 ++++++++++++++------
>  include/linux/platform_data/itco_wdt.h |  4 ++++
>  2 files changed, 18 insertions(+), 6 deletions(-)
>
> 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 d8768a2..3f00029 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);
>  };
> @@ -225,6 +227,8 @@ void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
>  		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 +241,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 +282,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);
>
> @@ -442,14 +446,18 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>
>  	p->iTCO_version = pdata->version;
>  	p->pci_dev = to_pci_dev(dev->parent);
> +	p->no_reboot_priv = pdata->no_reboot_priv;
>

This is really only for the if case below, and would be wrong otherwise.

> -	iTCO_wdt_no_reboot_bit_setup(p);
> +	if (pdata->update_no_reboot_bit)
> +		p->update_no_reboot_bit = pdata->update_no_reboot_bit;

Can you move that initialization here ? Or, even better, pass pdata to
iTCO_wdt_no_reboot_bit_setup() and initialize it there.

> +	else
> +		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).
>  	 */
> -	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 +467,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_ */
>

[PATCH v6 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 | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

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..173940f 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		BIT(4)
+#define PMC_CFG_NO_REBOOT_ENABLE	BIT(4)
+#define PMC_CFG_NO_REBOOT_DISABLE	0
+
 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,18 @@ 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)
+{
+	if (set)
+		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
+					    PMC_CFG_NO_REBOOT_MASK,
+					    PMC_CFG_NO_REBOOT_ENABLE);
+
+	return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
+				    PMC_CFG_NO_REBOOT_MASK,
+				    PMC_CFG_NO_REBOOT_DISABLE);
+}
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -630,15 +647,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 +710,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 +871,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

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

[Andy Shevchenko]

On Thu, Apr 6, 2017 at 1:54 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 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.

> +/* PMC_CFG_REG bit masks */
> +#define PMC_CFG_NO_REBOOT_MASK         BIT(4)

> +#define PMC_CFG_NO_REBOOT_ENABLE       BIT(4)
> +#define PMC_CFG_NO_REBOOT_DISABLE      0

I would go for those like
(1 << 4)
and
(0 << 4)
which for my opinion is better to get a picture.

> +static int update_no_reboot_bit(void *priv, bool set)
> +{
> +       if (set)
> +               return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
> +                                           PMC_CFG_NO_REBOOT_MASK,
> +                                           PMC_CFG_NO_REBOOT_ENABLE);
> +
> +       return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
> +                                   PMC_CFG_NO_REBOOT_MASK,
> +                                   PMC_CFG_NO_REBOOT_DISABLE);

u32 value = set ? PMC_CFG_NO_REBOOT_ENABLE : PMC_CFG_NO_REBOOT_DISABLE;

(also you may consider to use just *_EN and *_DIS)

return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
                                   PMC_CFG_NO_REBOOT_MASK,
                                   value);
> +}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v6 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(-)

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 173940f..3c9de18 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)
@@ -906,8 +902,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 v6 1/6] platform/x86: intel_pmc_ipc: fix gcr offset

[Rajneesh Bhardwaj]

On Wed, Apr 05, 2017 at 03:54:19PM -0700, Kuppuswamy Sathyanarayanan wrote:
> According to Broxton APL PMC spec, gcr mem region starts
> at offset 0x1000 from ipc mem base address. In this driver,
> PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
> memory region from IPC mem region. So we should use 0x1000(4K)
> as GCR offset. But currently this driver uses 0x1008 as GCT

Typo, GCT -> GCR. I think we still need to maintain consistency in the
commit message. Consider the below message and update if you'd like to.

On BXT SoC the PMC MMIO resources for the Global Control Registers (GCR) are
located at 4k offset from the IPC1 base. The PLAT_RESOURCE_GCR_OFFSET macro
used in this driver is misleading as 0x1008 is the location for PMC_CFG
register and not the GCR Base itself. 

GCR Base = IPC1 Base + 0x1000.

This patch updates the GCR Base address correctly.

> offset.This patch fixes this 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 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
> 

-- 
Best Regards,
Rajneesh