[PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Kuppuswamy Sathyanarayanan]

Mapping entire GCR mem region in this driver creates
mem region request conflict in sub devices that depend
on PMC. This creates driver probe failure in devices like
iTC0_wdt and telemetry device.

Currently this driver only need memory mapping for
s0ix counter registers. So this patch fixes this issue
by requesting memory mapping for only the s0ix counter mem
region.

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

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 0651d47..2b8a090 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -58,8 +58,8 @@
 #define IPC_READ_BUFFER		0x90
 
 /* PMC Global Control Registers */
-#define GCR_TELEM_DEEP_S0IX_OFFSET	0x1078
-#define GCR_TELEM_SHLW_S0IX_OFFSET	0x1080
+#define GCR_TELEM_DEEP_S0IX_OFFSET	0x0
+#define GCR_TELEM_SHLW_S0IX_OFFSET	0x8
 
 /* Residency with clock rate at 19.2MHz to usecs */
 #define S0IX_RESIDENCY_IN_USECS(d, s)		\
@@ -84,6 +84,8 @@
 #define PLAT_RESOURCE_IPC_SIZE		0x1000
 #define PLAT_RESOURCE_GCR_OFFSET	0x1008
 #define PLAT_RESOURCE_GCR_SIZE		0x1000
+#define PLAT_RESOURCE_GCR_S0IX_OFFSET	0x1078
+#define PLAT_RESOURCE_GCR_S0IX_SIZE	12
 #define PLAT_RESOURCE_BIOS_DATA_INDEX	1
 #define PLAT_RESOURCE_BIOS_IFACE_INDEX	2
 #define PLAT_RESOURCE_TELEM_SSRAM_INDEX	3
@@ -130,6 +132,11 @@ static struct intel_pmc_ipc_dev {
 	int gcr_size;
 	bool has_gcr_regs;
 
+	/* s0ix counters */
+	resource_size_t gcr_s0ix_start;
+	int gcr_s0ix_size;
+	void __iomem *gcr_s0ix_base;
+
 	/* punit */
 	struct platform_device *punit_dev;
 
@@ -194,9 +201,9 @@ static inline u32 ipc_data_readl(u32 offset)
 	return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
 }
 
-static inline u64 gcr_data_readq(u32 offset)
+static inline u64 gcr_s0ix_data_readq(u32 offset)
 {
-	return readq(ipcdev.ipc_base + offset);
+	return readq(ipcdev.gcr_s0ix_base + offset);
 }
 
 static int intel_pmc_ipc_check_status(void)
@@ -732,7 +739,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to get ipc resource\n");
 		return -ENXIO;
 	}
-	size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;
+	size = PLAT_RESOURCE_IPC_SIZE;
 
 	if (!request_mem_region(res->start, size, pdev->name)) {
 		dev_err(&pdev->dev, "Failed to request ipc resource\n");
@@ -748,8 +755,36 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 
 	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
+
 	dev_info(&pdev->dev, "ipc res: %pR\n", res);
 
+	/* request s0ix counter reg memory */
+	ipcdev.gcr_s0ix_start = res->start + PLAT_RESOURCE_GCR_S0IX_OFFSET;
+	ipcdev.gcr_s0ix_size = PLAT_RESOURCE_GCR_S0IX_SIZE;
+
+	if (!request_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size,
+				"ipc_gcr_s0ix")) {
+		dev_err(&pdev->dev, "Failed to request s0ix mem region\n");
+		iounmap(ipcdev.ipc_base);
+		release_mem_region(res->start, size);
+		return -EBUSY;
+	}
+
+	addr = ioremap_nocache(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
+	if (!addr) {
+		dev_err(&pdev->dev, "s0ix I/O memory remapping failed\n");
+		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
+		iounmap(ipcdev.ipc_base);
+		release_mem_region(res->start, size);
+		return -ENOMEM;
+	}
+
+	ipcdev.gcr_s0ix_base = addr;
+
+	dev_info(&pdev->dev, "s0ix mem region 0x%llx-0x%llx\n",
+		 ipcdev.gcr_s0ix_start,
+		 (ipcdev.gcr_s0ix_start + ipcdev.gcr_s0ix_size - 1));
+
 	ipcdev.telem_res_inval = 0;
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_TELEM_SSRAM_INDEX);
@@ -782,8 +817,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_s0ix_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
+	shlw = gcr_s0ix_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
 
 	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
 
@@ -851,12 +886,13 @@ static int ipc_plat_probe(struct platform_device *pdev)
 	platform_device_unregister(ipcdev.telemetry_dev);
 err_device:
 	iounmap(ipcdev.ipc_base);
+	iounmap(ipcdev.gcr_s0ix_base);
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_IPC_INDEX);
 	if (res) {
+		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
 		release_mem_region(res->start,
-				   PLAT_RESOURCE_IPC_SIZE +
-				   PLAT_RESOURCE_GCR_SIZE);
+				   PLAT_RESOURCE_IPC_SIZE);
 	}
 	return ret;
 }
@@ -871,12 +907,13 @@ static int ipc_plat_remove(struct platform_device *pdev)
 	platform_device_unregister(ipcdev.punit_dev);
 	platform_device_unregister(ipcdev.telemetry_dev);
 	iounmap(ipcdev.ipc_base);
+	iounmap(ipcdev.gcr_s0ix_base);
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_IPC_INDEX);
 	if (res) {
+		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
 		release_mem_region(res->start,
-				   PLAT_RESOURCE_IPC_SIZE +
-				   PLAT_RESOURCE_GCR_SIZE);
+				   PLAT_RESOURCE_IPC_SIZE);
 	}
 	ipcdev.dev = NULL;
 	return 0;
-- 
2.7.4

[PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Kuppuswamy Sathyanarayanan]

Mapping entire GCR mem region in this driver creates
mem region request conflict in sub devices that depend
on PMC. This creates driver probe failure in devices like
iTC0_wdt and telemetry device.

Currently this driver only need memory mapping for
s0ix counter registers. So this patch fixes this issue
by requesting memory mapping for only the s0ix counter mem
region.

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

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 0651d47..2b8a090 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -58,8 +58,8 @@
 #define IPC_READ_BUFFER		0x90
 
 /* PMC Global Control Registers */
-#define GCR_TELEM_DEEP_S0IX_OFFSET	0x1078
-#define GCR_TELEM_SHLW_S0IX_OFFSET	0x1080
+#define GCR_TELEM_DEEP_S0IX_OFFSET	0x0
+#define GCR_TELEM_SHLW_S0IX_OFFSET	0x8
 
 /* Residency with clock rate at 19.2MHz to usecs */
 #define S0IX_RESIDENCY_IN_USECS(d, s)		\
@@ -84,6 +84,8 @@
 #define PLAT_RESOURCE_IPC_SIZE		0x1000
 #define PLAT_RESOURCE_GCR_OFFSET	0x1008
 #define PLAT_RESOURCE_GCR_SIZE		0x1000
+#define PLAT_RESOURCE_GCR_S0IX_OFFSET	0x1078
+#define PLAT_RESOURCE_GCR_S0IX_SIZE	12
 #define PLAT_RESOURCE_BIOS_DATA_INDEX	1
 #define PLAT_RESOURCE_BIOS_IFACE_INDEX	2
 #define PLAT_RESOURCE_TELEM_SSRAM_INDEX	3
@@ -130,6 +132,11 @@ static struct intel_pmc_ipc_dev {
 	int gcr_size;
 	bool has_gcr_regs;
 
+	/* s0ix counters */
+	resource_size_t gcr_s0ix_start;
+	int gcr_s0ix_size;
+	void __iomem *gcr_s0ix_base;
+
 	/* punit */
 	struct platform_device *punit_dev;
 
@@ -194,9 +201,9 @@ static inline u32 ipc_data_readl(u32 offset)
 	return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
 }
 
-static inline u64 gcr_data_readq(u32 offset)
+static inline u64 gcr_s0ix_data_readq(u32 offset)
 {
-	return readq(ipcdev.ipc_base + offset);
+	return readq(ipcdev.gcr_s0ix_base + offset);
 }
 
 static int intel_pmc_ipc_check_status(void)
@@ -732,7 +739,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to get ipc resource\n");
 		return -ENXIO;
 	}
-	size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;
+	size = PLAT_RESOURCE_IPC_SIZE;
 
 	if (!request_mem_region(res->start, size, pdev->name)) {
 		dev_err(&pdev->dev, "Failed to request ipc resource\n");
@@ -748,8 +755,36 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 
 	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
+
 	dev_info(&pdev->dev, "ipc res: %pR\n", res);
 
+	/* request s0ix counter reg memory */
+	ipcdev.gcr_s0ix_start = res->start + PLAT_RESOURCE_GCR_S0IX_OFFSET;
+	ipcdev.gcr_s0ix_size = PLAT_RESOURCE_GCR_S0IX_SIZE;
+
+	if (!request_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size,
+				"ipc_gcr_s0ix")) {
+		dev_err(&pdev->dev, "Failed to request s0ix mem region\n");
+		iounmap(ipcdev.ipc_base);
+		release_mem_region(res->start, size);
+		return -EBUSY;
+	}
+
+	addr = ioremap_nocache(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
+	if (!addr) {
+		dev_err(&pdev->dev, "s0ix I/O memory remapping failed\n");
+		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
+		iounmap(ipcdev.ipc_base);
+		release_mem_region(res->start, size);
+		return -ENOMEM;
+	}
+
+	ipcdev.gcr_s0ix_base = addr;
+
+	dev_info(&pdev->dev, "s0ix mem region 0x%llx-0x%llx\n",
+		 ipcdev.gcr_s0ix_start,
+		 (ipcdev.gcr_s0ix_start + ipcdev.gcr_s0ix_size - 1));
+
 	ipcdev.telem_res_inval = 0;
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_TELEM_SSRAM_INDEX);
@@ -782,8 +817,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_s0ix_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
+	shlw = gcr_s0ix_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
 
 	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
 
@@ -851,12 +886,13 @@ static int ipc_plat_probe(struct platform_device *pdev)
 	platform_device_unregister(ipcdev.telemetry_dev);
 err_device:
 	iounmap(ipcdev.ipc_base);
+	iounmap(ipcdev.gcr_s0ix_base);
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_IPC_INDEX);
 	if (res) {
+		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
 		release_mem_region(res->start,
-				   PLAT_RESOURCE_IPC_SIZE +
-				   PLAT_RESOURCE_GCR_SIZE);
+				   PLAT_RESOURCE_IPC_SIZE);
 	}
 	return ret;
 }
@@ -871,12 +907,13 @@ static int ipc_plat_remove(struct platform_device *pdev)
 	platform_device_unregister(ipcdev.punit_dev);
 	platform_device_unregister(ipcdev.telemetry_dev);
 	iounmap(ipcdev.ipc_base);
+	iounmap(ipcdev.gcr_s0ix_base);
 	res = platform_get_resource(pdev, IORESOURCE_MEM,
 				    PLAT_RESOURCE_IPC_INDEX);
 	if (res) {
+		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
 		release_mem_region(res->start,
-				   PLAT_RESOURCE_IPC_SIZE +
-				   PLAT_RESOURCE_GCR_SIZE);
+				   PLAT_RESOURCE_IPC_SIZE);
 	}
 	ipcdev.dev = NULL;
 	return 0;
-- 
2.7.4

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Rajneesh Bhardwaj]

On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Mapping entire GCR mem region in this driver creates
> mem region request conflict in sub devices that depend
> on PMC. This creates driver probe failure in devices like
> iTC0_wdt and telemetry device.

While this patch might fix the issue for now but IMHO its not taking the
right approch. I guess we need some guidance here from the maintainers but
please do consider the below explaination for why we shoud not take this
approch to fix WDT issue. Telemetry driver has no issues while loading since
its not using any register in the GCR region.

PMC on BXT/APL platforms has contiguous IPC and GCR regions. PMC_IPC driver
maps the entire IPC and GCR region. It would be inefficient to map and unmap
each time we want to use another register present in IPC or GCR spaces.

iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
(Offset: 0x1008) and this falls in GCR space. The iTCO_WDT driver fails to
load because it can't request mem region for the resources already claimed
by PMC_IPC driver. However, ioremap would still work here and WDT driver
would load just fine.

So, IMHO the problem lies elsewhere and we should find a way to handle this
better in iTCO_WDT driver.

The IPC and GCR resources belong to PMC and should be claimed by the PMC
driver rightfully and should not be reclaimed by iTCO_WDT or any other
driver.


> 
> Currently this driver only need memory mapping for
> s0ix counter registers. So this patch fixes this issue
> by requesting memory mapping for only the s0ix counter mem
> region.

How about exposing a new API in PMC_IPC driver which can be used for reading
the desired GCR register and it can be used by iTCO_WDT instead of
requesting mem regions and remapping?

> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_ipc.c | 59 +++++++++++++++++++++++++++++-------
>  1 file changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 0651d47..2b8a090 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -58,8 +58,8 @@
>  #define IPC_READ_BUFFER		0x90
>  
>  /* PMC Global Control Registers */
> -#define GCR_TELEM_DEEP_S0IX_OFFSET	0x1078
> -#define GCR_TELEM_SHLW_S0IX_OFFSET	0x1080
> +#define GCR_TELEM_DEEP_S0IX_OFFSET	0x0
> +#define GCR_TELEM_SHLW_S0IX_OFFSET	0x8
>  
>  /* Residency with clock rate at 19.2MHz to usecs */
>  #define S0IX_RESIDENCY_IN_USECS(d, s)		\
> @@ -84,6 +84,8 @@
>  #define PLAT_RESOURCE_IPC_SIZE		0x1000
>  #define PLAT_RESOURCE_GCR_OFFSET	0x1008
>  #define PLAT_RESOURCE_GCR_SIZE		0x1000
> +#define PLAT_RESOURCE_GCR_S0IX_OFFSET	0x1078
> +#define PLAT_RESOURCE_GCR_S0IX_SIZE	12
>  #define PLAT_RESOURCE_BIOS_DATA_INDEX	1
>  #define PLAT_RESOURCE_BIOS_IFACE_INDEX	2
>  #define PLAT_RESOURCE_TELEM_SSRAM_INDEX	3
> @@ -130,6 +132,11 @@ static struct intel_pmc_ipc_dev {
>  	int gcr_size;
>  	bool has_gcr_regs;
>  
> +	/* s0ix counters */
> +	resource_size_t gcr_s0ix_start;
> +	int gcr_s0ix_size;
> +	void __iomem *gcr_s0ix_base;
> +
>  	/* punit */
>  	struct platform_device *punit_dev;
>  
> @@ -194,9 +201,9 @@ static inline u32 ipc_data_readl(u32 offset)
>  	return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
>  }
>  
> -static inline u64 gcr_data_readq(u32 offset)
> +static inline u64 gcr_s0ix_data_readq(u32 offset)
>  {
> -	return readq(ipcdev.ipc_base + offset);
> +	return readq(ipcdev.gcr_s0ix_base + offset);
>  }
>  
>  static int intel_pmc_ipc_check_status(void)
> @@ -732,7 +739,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "Failed to get ipc resource\n");
>  		return -ENXIO;
>  	}
> -	size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;
> +	size = PLAT_RESOURCE_IPC_SIZE;
>  
>  	if (!request_mem_region(res->start, size, pdev->name)) {
>  		dev_err(&pdev->dev, "Failed to request ipc resource\n");
> @@ -748,8 +755,36 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>  
>  	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
>  	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
> +
>  	dev_info(&pdev->dev, "ipc res: %pR\n", res);
>  
> +	/* request s0ix counter reg memory */
> +	ipcdev.gcr_s0ix_start = res->start + PLAT_RESOURCE_GCR_S0IX_OFFSET;
> +	ipcdev.gcr_s0ix_size = PLAT_RESOURCE_GCR_S0IX_SIZE;
> +
> +	if (!request_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size,
> +				"ipc_gcr_s0ix")) {
> +		dev_err(&pdev->dev, "Failed to request s0ix mem region\n");
> +		iounmap(ipcdev.ipc_base);
> +		release_mem_region(res->start, size);
> +		return -EBUSY;
> +	}
> +
> +	addr = ioremap_nocache(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
> +	if (!addr) {
> +		dev_err(&pdev->dev, "s0ix I/O memory remapping failed\n");
> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
> +		iounmap(ipcdev.ipc_base);
> +		release_mem_region(res->start, size);
> +		return -ENOMEM;
> +	}
> +
> +	ipcdev.gcr_s0ix_base = addr;
> +
> +	dev_info(&pdev->dev, "s0ix mem region 0x%llx-0x%llx\n",
> +		 ipcdev.gcr_s0ix_start,
> +		 (ipcdev.gcr_s0ix_start + ipcdev.gcr_s0ix_size - 1));
> +
>  	ipcdev.telem_res_inval = 0;
>  	res = platform_get_resource(pdev, IORESOURCE_MEM,
>  				    PLAT_RESOURCE_TELEM_SSRAM_INDEX);
> @@ -782,8 +817,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_s0ix_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
> +	shlw = gcr_s0ix_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
>  
>  	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
>  
> @@ -851,12 +886,13 @@ static int ipc_plat_probe(struct platform_device *pdev)
>  	platform_device_unregister(ipcdev.telemetry_dev);
>  err_device:
>  	iounmap(ipcdev.ipc_base);
> +	iounmap(ipcdev.gcr_s0ix_base);
>  	res = platform_get_resource(pdev, IORESOURCE_MEM,
>  				    PLAT_RESOURCE_IPC_INDEX);
>  	if (res) {
> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>  		release_mem_region(res->start,
> -				   PLAT_RESOURCE_IPC_SIZE +
> -				   PLAT_RESOURCE_GCR_SIZE);
> +				   PLAT_RESOURCE_IPC_SIZE);
>  	}
>  	return ret;
>  }
> @@ -871,12 +907,13 @@ static int ipc_plat_remove(struct platform_device *pdev)
>  	platform_device_unregister(ipcdev.punit_dev);
>  	platform_device_unregister(ipcdev.telemetry_dev);
>  	iounmap(ipcdev.ipc_base);
> +	iounmap(ipcdev.gcr_s0ix_base);
>  	res = platform_get_resource(pdev, IORESOURCE_MEM,
>  				    PLAT_RESOURCE_IPC_INDEX);
>  	if (res) {
> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>  		release_mem_region(res->start,
> -				   PLAT_RESOURCE_IPC_SIZE +
> -				   PLAT_RESOURCE_GCR_SIZE);
> +				   PLAT_RESOURCE_IPC_SIZE);
>  	}
>  	ipcdev.dev = NULL;
>  	return 0;
> -- 
> 2.7.4
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Rajneesh Bhardwaj]

On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Mapping entire GCR mem region in this driver creates
> mem region request conflict in sub devices that depend
> on PMC. This creates driver probe failure in devices like
> iTC0_wdt and telemetry device.

While this patch might fix the issue for now but IMHO its not taking the
right approch. I guess we need some guidance here from the maintainers but
please do consider the below explaination for why we shoud not take this
approch to fix WDT issue. Telemetry driver has no issues while loading since
its not using any register in the GCR region.

PMC on BXT/APL platforms has contiguous IPC and GCR regions. PMC_IPC driver
maps the entire IPC and GCR region. It would be inefficient to map and unmap
each time we want to use another register present in IPC or GCR spaces.

iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
(Offset: 0x1008) and this falls in GCR space. The iTCO_WDT driver fails to
load because it can't request mem region for the resources already claimed
by PMC_IPC driver. However, ioremap would still work here and WDT driver
would load just fine.

So, IMHO the problem lies elsewhere and we should find a way to handle this
better in iTCO_WDT driver.

The IPC and GCR resources belong to PMC and should be claimed by the PMC
driver rightfully and should not be reclaimed by iTCO_WDT or any other
driver.


> 
> Currently this driver only need memory mapping for
> s0ix counter registers. So this patch fixes this issue
> by requesting memory mapping for only the s0ix counter mem
> region.

How about exposing a new API in PMC_IPC driver which can be used for reading
the desired GCR register and it can be used by iTCO_WDT instead of
requesting mem regions and remapping?

> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_ipc.c | 59 +++++++++++++++++++++++++++++-------
>  1 file changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 0651d47..2b8a090 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -58,8 +58,8 @@
>  #define IPC_READ_BUFFER		0x90
>  
>  /* PMC Global Control Registers */
> -#define GCR_TELEM_DEEP_S0IX_OFFSET	0x1078
> -#define GCR_TELEM_SHLW_S0IX_OFFSET	0x1080
> +#define GCR_TELEM_DEEP_S0IX_OFFSET	0x0
> +#define GCR_TELEM_SHLW_S0IX_OFFSET	0x8
>  
>  /* Residency with clock rate at 19.2MHz to usecs */
>  #define S0IX_RESIDENCY_IN_USECS(d, s)		\
> @@ -84,6 +84,8 @@
>  #define PLAT_RESOURCE_IPC_SIZE		0x1000
>  #define PLAT_RESOURCE_GCR_OFFSET	0x1008
>  #define PLAT_RESOURCE_GCR_SIZE		0x1000
> +#define PLAT_RESOURCE_GCR_S0IX_OFFSET	0x1078
> +#define PLAT_RESOURCE_GCR_S0IX_SIZE	12
>  #define PLAT_RESOURCE_BIOS_DATA_INDEX	1
>  #define PLAT_RESOURCE_BIOS_IFACE_INDEX	2
>  #define PLAT_RESOURCE_TELEM_SSRAM_INDEX	3
> @@ -130,6 +132,11 @@ static struct intel_pmc_ipc_dev {
>  	int gcr_size;
>  	bool has_gcr_regs;
>  
> +	/* s0ix counters */
> +	resource_size_t gcr_s0ix_start;
> +	int gcr_s0ix_size;
> +	void __iomem *gcr_s0ix_base;
> +
>  	/* punit */
>  	struct platform_device *punit_dev;
>  
> @@ -194,9 +201,9 @@ static inline u32 ipc_data_readl(u32 offset)
>  	return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
>  }
>  
> -static inline u64 gcr_data_readq(u32 offset)
> +static inline u64 gcr_s0ix_data_readq(u32 offset)
>  {
> -	return readq(ipcdev.ipc_base + offset);
> +	return readq(ipcdev.gcr_s0ix_base + offset);
>  }
>  
>  static int intel_pmc_ipc_check_status(void)
> @@ -732,7 +739,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "Failed to get ipc resource\n");
>  		return -ENXIO;
>  	}
> -	size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;
> +	size = PLAT_RESOURCE_IPC_SIZE;
>  
>  	if (!request_mem_region(res->start, size, pdev->name)) {
>  		dev_err(&pdev->dev, "Failed to request ipc resource\n");
> @@ -748,8 +755,36 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>  
>  	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
>  	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
> +
>  	dev_info(&pdev->dev, "ipc res: %pR\n", res);
>  
> +	/* request s0ix counter reg memory */
> +	ipcdev.gcr_s0ix_start = res->start + PLAT_RESOURCE_GCR_S0IX_OFFSET;
> +	ipcdev.gcr_s0ix_size = PLAT_RESOURCE_GCR_S0IX_SIZE;
> +
> +	if (!request_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size,
> +				"ipc_gcr_s0ix")) {
> +		dev_err(&pdev->dev, "Failed to request s0ix mem region\n");
> +		iounmap(ipcdev.ipc_base);
> +		release_mem_region(res->start, size);
> +		return -EBUSY;
> +	}
> +
> +	addr = ioremap_nocache(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
> +	if (!addr) {
> +		dev_err(&pdev->dev, "s0ix I/O memory remapping failed\n");
> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
> +		iounmap(ipcdev.ipc_base);
> +		release_mem_region(res->start, size);
> +		return -ENOMEM;
> +	}
> +
> +	ipcdev.gcr_s0ix_base = addr;
> +
> +	dev_info(&pdev->dev, "s0ix mem region 0x%llx-0x%llx\n",
> +		 ipcdev.gcr_s0ix_start,
> +		 (ipcdev.gcr_s0ix_start + ipcdev.gcr_s0ix_size - 1));
> +
>  	ipcdev.telem_res_inval = 0;
>  	res = platform_get_resource(pdev, IORESOURCE_MEM,
>  				    PLAT_RESOURCE_TELEM_SSRAM_INDEX);
> @@ -782,8 +817,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_s0ix_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
> +	shlw = gcr_s0ix_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
>  
>  	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
>  
> @@ -851,12 +886,13 @@ static int ipc_plat_probe(struct platform_device *pdev)
>  	platform_device_unregister(ipcdev.telemetry_dev);
>  err_device:
>  	iounmap(ipcdev.ipc_base);
> +	iounmap(ipcdev.gcr_s0ix_base);
>  	res = platform_get_resource(pdev, IORESOURCE_MEM,
>  				    PLAT_RESOURCE_IPC_INDEX);
>  	if (res) {
> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>  		release_mem_region(res->start,
> -				   PLAT_RESOURCE_IPC_SIZE +
> -				   PLAT_RESOURCE_GCR_SIZE);
> +				   PLAT_RESOURCE_IPC_SIZE);
>  	}
>  	return ret;
>  }
> @@ -871,12 +907,13 @@ static int ipc_plat_remove(struct platform_device *pdev)
>  	platform_device_unregister(ipcdev.punit_dev);
>  	platform_device_unregister(ipcdev.telemetry_dev);
>  	iounmap(ipcdev.ipc_base);
> +	iounmap(ipcdev.gcr_s0ix_base);
>  	res = platform_get_resource(pdev, IORESOURCE_MEM,
>  				    PLAT_RESOURCE_IPC_INDEX);
>  	if (res) {
> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>  		release_mem_region(res->start,
> -				   PLAT_RESOURCE_IPC_SIZE +
> -				   PLAT_RESOURCE_GCR_SIZE);
> +				   PLAT_RESOURCE_IPC_SIZE);
>  	}
>  	ipcdev.dev = NULL;
>  	return 0;
> -- 
> 2.7.4
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Andy Shevchenko]

On Thu, Mar 16, 2017 at 4:52 PM, Rajneesh Bhardwaj
<rajneesh.bhardwaj@intel.com> wrote:
> On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Mapping entire GCR mem region in this driver creates
>> mem region request conflict in sub devices that depend
>> on PMC. This creates driver probe failure in devices like
>> iTC0_wdt and telemetry device.

> iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
> (Offset: 0x1008) and this falls in GCR space.

Are we talking about ACPI-enabled platform?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Andy Shevchenko]

On Thu, Mar 16, 2017 at 4:52 PM, Rajneesh Bhardwaj
<rajneesh.bhardwaj@intel.com> wrote:
> On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Mapping entire GCR mem region in this driver creates
>> mem region request conflict in sub devices that depend
>> on PMC. This creates driver probe failure in devices like
>> iTC0_wdt and telemetry device.

> iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
> (Offset: 0x1008) and this falls in GCR space.

Are we talking about ACPI-enabled platform?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 06:05:39PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 16, 2017 at 4:52 PM, Rajneesh Bhardwaj
> <rajneesh.bhardwaj@intel.com> wrote:
> > On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >> Mapping entire GCR mem region in this driver creates
> >> mem region request conflict in sub devices that depend
> >> on PMC. This creates driver probe failure in devices like
> >> iTC0_wdt and telemetry device.
> 
> > iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
> > (Offset: 0x1008) and this falls in GCR space.
> 
> Are we talking about ACPI-enabled platform?
>

IIUC, you are referring to WDT enumerated by ACPI tables (WDAT, WDRT etc) ?

On APL/BXT i think we pass the resource mapping to iTCO_WDT driver since
acpi_has_watchdog provides the required protection. For non ACPI-enabled
platforms we have this issue since iTCO_WDT driver anyway tries resource
mapping when the iTCO_version >=2.

Can someone please explain whether we can have iTCO_version >=2 for Intel
Core SoCs as well or its for Atom only?
 
> -- 
> With Best Regards,
> Andy Shevchenko

-- 
Best Regards,
Rajneesh

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 06:05:39PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 16, 2017 at 4:52 PM, Rajneesh Bhardwaj
> <rajneesh.bhardwaj@intel.com> wrote:
> > On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >> Mapping entire GCR mem region in this driver creates
> >> mem region request conflict in sub devices that depend
> >> on PMC. This creates driver probe failure in devices like
> >> iTC0_wdt and telemetry device.
> 
> > iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
> > (Offset: 0x1008) and this falls in GCR space.
> 
> Are we talking about ACPI-enabled platform?
>

IIUC, you are referring to WDT enumerated by ACPI tables (WDAT, WDRT etc) ?

On APL/BXT i think we pass the resource mapping to iTCO_WDT driver since
acpi_has_watchdog provides the required protection. For non ACPI-enabled
platforms we have this issue since iTCO_WDT driver anyway tries resource
mapping when the iTCO_version >=2.

Can someone please explain whether we can have iTCO_version >=2 for Intel
Core SoCs as well or its for Atom only?
 
> -- 
> With Best Regards,
> Andy Shevchenko

-- 
Best Regards,
Rajneesh

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Andy Shevchenko]

On Thu, Mar 16, 2017 at 8:13 PM, Rajneesh Bhardwaj
<rajneesh.bhardwaj@intel.com> wrote:
> On Thu, Mar 16, 2017 at 06:05:39PM +0200, Andy Shevchenko wrote:
>> On Thu, Mar 16, 2017 at 4:52 PM, Rajneesh Bhardwaj
>> <rajneesh.bhardwaj@intel.com> wrote:
>> > On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> >> Mapping entire GCR mem region in this driver creates
>> >> mem region request conflict in sub devices that depend
>> >> on PMC. This creates driver probe failure in devices like
>> >> iTC0_wdt and telemetry device.
>>
>> > iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
>> > (Offset: 0x1008) and this falls in GCR space.
>>
>> Are we talking about ACPI-enabled platform?

> IIUC, you are referring to WDT enumerated by ACPI tables (WDAT, WDRT etc) ?
>
> On APL/BXT i think we pass the resource mapping to iTCO_WDT driver since
> acpi_has_watchdog provides the required protection. For non ACPI-enabled
> platforms we have this issue since iTCO_WDT driver anyway tries resource
> mapping when the iTCO_version >=2.

And driver with necessary stuff should be already in upstream.

https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/tree/drivers/mfd/lpc_ich.c?h=watchdog-next#n552

Or it's not enough?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Andy Shevchenko]

On Thu, Mar 16, 2017 at 8:13 PM, Rajneesh Bhardwaj
<rajneesh.bhardwaj@intel.com> wrote:
> On Thu, Mar 16, 2017 at 06:05:39PM +0200, Andy Shevchenko wrote:
>> On Thu, Mar 16, 2017 at 4:52 PM, Rajneesh Bhardwaj
>> <rajneesh.bhardwaj@intel.com> wrote:
>> > On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> >> Mapping entire GCR mem region in this driver creates
>> >> mem region request conflict in sub devices that depend
>> >> on PMC. This creates driver probe failure in devices like
>> >> iTC0_wdt and telemetry device.
>>
>> > iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
>> > (Offset: 0x1008) and this falls in GCR space.
>>
>> Are we talking about ACPI-enabled platform?

> IIUC, you are referring to WDT enumerated by ACPI tables (WDAT, WDRT etc) ?
>
> On APL/BXT i think we pass the resource mapping to iTCO_WDT driver since
> acpi_has_watchdog provides the required protection. For non ACPI-enabled
> platforms we have this issue since iTCO_WDT driver anyway tries resource
> mapping when the iTCO_version >=2.

And driver with necessary stuff should be already in upstream.

https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/tree/drivers/mfd/lpc_ich.c?h=watchdog-next#n552

Or it's not enough?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[sathyanarayanan kuppuswamy]

Hi Andy,


On 03/16/2017 01:12 PM, Andy Shevchenko wrote:
> On Thu, Mar 16, 2017 at 8:13 PM, Rajneesh Bhardwaj
> <rajneesh.bhardwaj@intel.com> wrote:
>> On Thu, Mar 16, 2017 at 06:05:39PM +0200, Andy Shevchenko wrote:
>>> On Thu, Mar 16, 2017 at 4:52 PM, Rajneesh Bhardwaj
>>> <rajneesh.bhardwaj@intel.com> wrote:
>>>> On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
>>>>> Mapping entire GCR mem region in this driver creates
>>>>> mem region request conflict in sub devices that depend
>>>>> on PMC. This creates driver probe failure in devices like
>>>>> iTC0_wdt and telemetry device.
>>>> iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
>>>> (Offset: 0x1008) and this falls in GCR space.
>>> Are we talking about ACPI-enabled platform?
>> IIUC, you are referring to WDT enumerated by ACPI tables (WDAT, WDRT etc) ?
>>
>> On APL/BXT i think we pass the resource mapping to iTCO_WDT driver since
>> acpi_has_watchdog provides the required protection. For non ACPI-enabled
>> platforms we have this issue since iTCO_WDT driver anyway tries resource
>> mapping when the iTCO_version >=2.
> And driver with necessary stuff should be already in upstream.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/tree/drivers/mfd/lpc_ich.c?h=watchdog-next#n552
>
> Or it's not enough?
Can this driver replace intel_pmc_ipc driver ? It seems to be missing 
IPC related APIs.

Regarding iTCO watchdog section, I think this driver also passes GCR 
region as memory resource to iTCO_WDT device.
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[sathyanarayanan kuppuswamy]

Hi Andy,


On 03/16/2017 01:12 PM, Andy Shevchenko wrote:
> On Thu, Mar 16, 2017 at 8:13 PM, Rajneesh Bhardwaj
> <rajneesh.bhardwaj@intel.com> wrote:
>> On Thu, Mar 16, 2017 at 06:05:39PM +0200, Andy Shevchenko wrote:
>>> On Thu, Mar 16, 2017 at 4:52 PM, Rajneesh Bhardwaj
>>> <rajneesh.bhardwaj@intel.com> wrote:
>>>> On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
>>>>> Mapping entire GCR mem region in this driver creates
>>>>> mem region request conflict in sub devices that depend
>>>>> on PMC. This creates driver probe failure in devices like
>>>>> iTC0_wdt and telemetry device.
>>>> iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
>>>> (Offset: 0x1008) and this falls in GCR space.
>>> Are we talking about ACPI-enabled platform?
>> IIUC, you are referring to WDT enumerated by ACPI tables (WDAT, WDRT etc) ?
>>
>> On APL/BXT i think we pass the resource mapping to iTCO_WDT driver since
>> acpi_has_watchdog provides the required protection. For non ACPI-enabled
>> platforms we have this issue since iTCO_WDT driver anyway tries resource
>> mapping when the iTCO_version >=2.
> And driver with necessary stuff should be already in upstream.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/tree/drivers/mfd/lpc_ich.c?h=watchdog-next#n552
>
> Or it's not enough?
Can this driver replace intel_pmc_ipc driver ? It seems to be missing 
IPC related APIs.

Regarding iTCO watchdog section, I think this driver also passes GCR 
region as memory resource to iTCO_WDT device.
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[sathyanarayanan kuppuswamy]

Hi,


On 03/16/2017 07:52 AM, Rajneesh Bhardwaj wrote:
> On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Mapping entire GCR mem region in this driver creates
>> mem region request conflict in sub devices that depend
>> on PMC. This creates driver probe failure in devices like
>> iTC0_wdt and telemetry device.
> While this patch might fix the issue for now but IMHO its not taking the
> right approch. I guess we need some guidance here from the maintainers but
Agreed. I thought about this problem and I know this solution does not
scale well. Other way is to expose an API for GCR access and use it on
PMC dependent drivers. In this case, we should also add GCR register
address macros to PMC header file and this might need change to PMC header
file each time when some one wants to add access new register address.

Since S0ix counter access is only GCR access use case in PMC driver, I
thought  both solutions has some pros and cons.
> please do consider the below explaination for why we shoud not take this
> approch to fix WDT issue. Telemetry driver has no issues while loading since
> its not using any register in the GCR region.
> PMC on BXT/APL platforms has contiguous IPC and GCR regions. PMC_IPC driver
> maps the entire IPC and GCR region. It would be inefficient to map and unmap
> each time we want to use another register present in IPC or GCR spaces.
But I am wondering whether there will be any future GCR access changes 
in PMC driver?
If its going to be just S0ix counter access then I don't think we need 
to worry about performance here.
>
> iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
> (Offset: 0x1008) and this falls in GCR space. The iTCO_WDT driver fails to
> load because it can't request mem region for the resources already claimed
> by PMC_IPC driver. However, ioremap would still work here and WDT driver
> would load just fine.
>
> So, IMHO the problem lies elsewhere and we should find a way to handle this
> better in iTCO_WDT driver.
>
> The IPC and GCR resources belong to PMC and should be claimed by the PMC
> driver rightfully and should not be reclaimed by iTCO_WDT or any other
> driver.
iTCO_WDT , Telemtry and Punit are PMC dependent devices right ? And they
share the resources among them right ?
>
>
>> Currently this driver only need memory mapping for
>> s0ix counter registers. So this patch fixes this issue
>> by requesting memory mapping for only the s0ix counter mem
>> region.
> How about exposing a new API in PMC_IPC driver which can be used for reading
> the desired GCR register and it can be used by iTCO_WDT instead of
> requesting mem regions and remapping?
If you think in future if we might need access to more GCR space in PMC 
driver, then we need to change this solution. Please let me know. If 
yes, I will add an API as you mentioned.
>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel_pmc_ipc.c | 59 +++++++++++++++++++++++++++++-------
>>   1 file changed, 48 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
>> index 0651d47..2b8a090 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -58,8 +58,8 @@
>>   #define IPC_READ_BUFFER		0x90
>>   
>>   /* PMC Global Control Registers */
>> -#define GCR_TELEM_DEEP_S0IX_OFFSET	0x1078
>> -#define GCR_TELEM_SHLW_S0IX_OFFSET	0x1080
>> +#define GCR_TELEM_DEEP_S0IX_OFFSET	0x0
>> +#define GCR_TELEM_SHLW_S0IX_OFFSET	0x8
>>   
>>   /* Residency with clock rate at 19.2MHz to usecs */
>>   #define S0IX_RESIDENCY_IN_USECS(d, s)		\
>> @@ -84,6 +84,8 @@
>>   #define PLAT_RESOURCE_IPC_SIZE		0x1000
>>   #define PLAT_RESOURCE_GCR_OFFSET	0x1008
>>   #define PLAT_RESOURCE_GCR_SIZE		0x1000
>> +#define PLAT_RESOURCE_GCR_S0IX_OFFSET	0x1078
>> +#define PLAT_RESOURCE_GCR_S0IX_SIZE	12
>>   #define PLAT_RESOURCE_BIOS_DATA_INDEX	1
>>   #define PLAT_RESOURCE_BIOS_IFACE_INDEX	2
>>   #define PLAT_RESOURCE_TELEM_SSRAM_INDEX	3
>> @@ -130,6 +132,11 @@ static struct intel_pmc_ipc_dev {
>>   	int gcr_size;
>>   	bool has_gcr_regs;
>>   
>> +	/* s0ix counters */
>> +	resource_size_t gcr_s0ix_start;
>> +	int gcr_s0ix_size;
>> +	void __iomem *gcr_s0ix_base;
>> +
>>   	/* punit */
>>   	struct platform_device *punit_dev;
>>   
>> @@ -194,9 +201,9 @@ static inline u32 ipc_data_readl(u32 offset)
>>   	return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
>>   }
>>   
>> -static inline u64 gcr_data_readq(u32 offset)
>> +static inline u64 gcr_s0ix_data_readq(u32 offset)
>>   {
>> -	return readq(ipcdev.ipc_base + offset);
>> +	return readq(ipcdev.gcr_s0ix_base + offset);
>>   }
>>   
>>   static int intel_pmc_ipc_check_status(void)
>> @@ -732,7 +739,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>>   		dev_err(&pdev->dev, "Failed to get ipc resource\n");
>>   		return -ENXIO;
>>   	}
>> -	size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;
>> +	size = PLAT_RESOURCE_IPC_SIZE;
>>   
>>   	if (!request_mem_region(res->start, size, pdev->name)) {
>>   		dev_err(&pdev->dev, "Failed to request ipc resource\n");
>> @@ -748,8 +755,36 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>>   
>>   	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
>>   	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
>> +
>>   	dev_info(&pdev->dev, "ipc res: %pR\n", res);
>>   
>> +	/* request s0ix counter reg memory */
>> +	ipcdev.gcr_s0ix_start = res->start + PLAT_RESOURCE_GCR_S0IX_OFFSET;
>> +	ipcdev.gcr_s0ix_size = PLAT_RESOURCE_GCR_S0IX_SIZE;
>> +
>> +	if (!request_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size,
>> +				"ipc_gcr_s0ix")) {
>> +		dev_err(&pdev->dev, "Failed to request s0ix mem region\n");
>> +		iounmap(ipcdev.ipc_base);
>> +		release_mem_region(res->start, size);
>> +		return -EBUSY;
>> +	}
>> +
>> +	addr = ioremap_nocache(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>> +	if (!addr) {
>> +		dev_err(&pdev->dev, "s0ix I/O memory remapping failed\n");
>> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>> +		iounmap(ipcdev.ipc_base);
>> +		release_mem_region(res->start, size);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ipcdev.gcr_s0ix_base = addr;
>> +
>> +	dev_info(&pdev->dev, "s0ix mem region 0x%llx-0x%llx\n",
>> +		 ipcdev.gcr_s0ix_start,
>> +		 (ipcdev.gcr_s0ix_start + ipcdev.gcr_s0ix_size - 1));
>> +
>>   	ipcdev.telem_res_inval = 0;
>>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>>   				    PLAT_RESOURCE_TELEM_SSRAM_INDEX);
>> @@ -782,8 +817,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_s0ix_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
>> +	shlw = gcr_s0ix_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
>>   
>>   	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
>>   
>> @@ -851,12 +886,13 @@ static int ipc_plat_probe(struct platform_device *pdev)
>>   	platform_device_unregister(ipcdev.telemetry_dev);
>>   err_device:
>>   	iounmap(ipcdev.ipc_base);
>> +	iounmap(ipcdev.gcr_s0ix_base);
>>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>>   				    PLAT_RESOURCE_IPC_INDEX);
>>   	if (res) {
>> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>>   		release_mem_region(res->start,
>> -				   PLAT_RESOURCE_IPC_SIZE +
>> -				   PLAT_RESOURCE_GCR_SIZE);
>> +				   PLAT_RESOURCE_IPC_SIZE);
>>   	}
>>   	return ret;
>>   }
>> @@ -871,12 +907,13 @@ static int ipc_plat_remove(struct platform_device *pdev)
>>   	platform_device_unregister(ipcdev.punit_dev);
>>   	platform_device_unregister(ipcdev.telemetry_dev);
>>   	iounmap(ipcdev.ipc_base);
>> +	iounmap(ipcdev.gcr_s0ix_base);
>>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>>   				    PLAT_RESOURCE_IPC_INDEX);
>>   	if (res) {
>> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>>   		release_mem_region(res->start,
>> -				   PLAT_RESOURCE_IPC_SIZE +
>> -				   PLAT_RESOURCE_GCR_SIZE);
>> +				   PLAT_RESOURCE_IPC_SIZE);
>>   	}
>>   	ipcdev.dev = NULL;
>>   	return 0;
>> -- 
>> 2.7.4
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[sathyanarayanan kuppuswamy]

Hi,


On 03/16/2017 07:52 AM, Rajneesh Bhardwaj wrote:
> On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Mapping entire GCR mem region in this driver creates
>> mem region request conflict in sub devices that depend
>> on PMC. This creates driver probe failure in devices like
>> iTC0_wdt and telemetry device.
> While this patch might fix the issue for now but IMHO its not taking the
> right approch. I guess we need some guidance here from the maintainers but
Agreed. I thought about this problem and I know this solution does not
scale well. Other way is to expose an API for GCR access and use it on
PMC dependent drivers. In this case, we should also add GCR register
address macros to PMC header file and this might need change to PMC header
file each time when some one wants to add access new register address.

Since S0ix counter access is only GCR access use case in PMC driver, I
thought  both solutions has some pros and cons.
> please do consider the below explaination for why we shoud not take this
> approch to fix WDT issue. Telemetry driver has no issues while loading since
> its not using any register in the GCR region.
> PMC on BXT/APL platforms has contiguous IPC and GCR regions. PMC_IPC driver
> maps the entire IPC and GCR region. It would be inefficient to map and unmap
> each time we want to use another register present in IPC or GCR spaces.
But I am wondering whether there will be any future GCR access changes 
in PMC driver?
If its going to be just S0ix counter access then I don't think we need 
to worry about performance here.
>
> iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
> (Offset: 0x1008) and this falls in GCR space. The iTCO_WDT driver fails to
> load because it can't request mem region for the resources already claimed
> by PMC_IPC driver. However, ioremap would still work here and WDT driver
> would load just fine.
>
> So, IMHO the problem lies elsewhere and we should find a way to handle this
> better in iTCO_WDT driver.
>
> The IPC and GCR resources belong to PMC and should be claimed by the PMC
> driver rightfully and should not be reclaimed by iTCO_WDT or any other
> driver.
iTCO_WDT , Telemtry and Punit are PMC dependent devices right ? And they
share the resources among them right ?
>
>
>> Currently this driver only need memory mapping for
>> s0ix counter registers. So this patch fixes this issue
>> by requesting memory mapping for only the s0ix counter mem
>> region.
> How about exposing a new API in PMC_IPC driver which can be used for reading
> the desired GCR register and it can be used by iTCO_WDT instead of
> requesting mem regions and remapping?
If you think in future if we might need access to more GCR space in PMC 
driver, then we need to change this solution. Please let me know. If 
yes, I will add an API as you mentioned.
>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel_pmc_ipc.c | 59 +++++++++++++++++++++++++++++-------
>>   1 file changed, 48 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
>> index 0651d47..2b8a090 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -58,8 +58,8 @@
>>   #define IPC_READ_BUFFER		0x90
>>   
>>   /* PMC Global Control Registers */
>> -#define GCR_TELEM_DEEP_S0IX_OFFSET	0x1078
>> -#define GCR_TELEM_SHLW_S0IX_OFFSET	0x1080
>> +#define GCR_TELEM_DEEP_S0IX_OFFSET	0x0
>> +#define GCR_TELEM_SHLW_S0IX_OFFSET	0x8
>>   
>>   /* Residency with clock rate at 19.2MHz to usecs */
>>   #define S0IX_RESIDENCY_IN_USECS(d, s)		\
>> @@ -84,6 +84,8 @@
>>   #define PLAT_RESOURCE_IPC_SIZE		0x1000
>>   #define PLAT_RESOURCE_GCR_OFFSET	0x1008
>>   #define PLAT_RESOURCE_GCR_SIZE		0x1000
>> +#define PLAT_RESOURCE_GCR_S0IX_OFFSET	0x1078
>> +#define PLAT_RESOURCE_GCR_S0IX_SIZE	12
>>   #define PLAT_RESOURCE_BIOS_DATA_INDEX	1
>>   #define PLAT_RESOURCE_BIOS_IFACE_INDEX	2
>>   #define PLAT_RESOURCE_TELEM_SSRAM_INDEX	3
>> @@ -130,6 +132,11 @@ static struct intel_pmc_ipc_dev {
>>   	int gcr_size;
>>   	bool has_gcr_regs;
>>   
>> +	/* s0ix counters */
>> +	resource_size_t gcr_s0ix_start;
>> +	int gcr_s0ix_size;
>> +	void __iomem *gcr_s0ix_base;
>> +
>>   	/* punit */
>>   	struct platform_device *punit_dev;
>>   
>> @@ -194,9 +201,9 @@ static inline u32 ipc_data_readl(u32 offset)
>>   	return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
>>   }
>>   
>> -static inline u64 gcr_data_readq(u32 offset)
>> +static inline u64 gcr_s0ix_data_readq(u32 offset)
>>   {
>> -	return readq(ipcdev.ipc_base + offset);
>> +	return readq(ipcdev.gcr_s0ix_base + offset);
>>   }
>>   
>>   static int intel_pmc_ipc_check_status(void)
>> @@ -732,7 +739,7 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>>   		dev_err(&pdev->dev, "Failed to get ipc resource\n");
>>   		return -ENXIO;
>>   	}
>> -	size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;
>> +	size = PLAT_RESOURCE_IPC_SIZE;
>>   
>>   	if (!request_mem_region(res->start, size, pdev->name)) {
>>   		dev_err(&pdev->dev, "Failed to request ipc resource\n");
>> @@ -748,8 +755,36 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>>   
>>   	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
>>   	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
>> +
>>   	dev_info(&pdev->dev, "ipc res: %pR\n", res);
>>   
>> +	/* request s0ix counter reg memory */
>> +	ipcdev.gcr_s0ix_start = res->start + PLAT_RESOURCE_GCR_S0IX_OFFSET;
>> +	ipcdev.gcr_s0ix_size = PLAT_RESOURCE_GCR_S0IX_SIZE;
>> +
>> +	if (!request_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size,
>> +				"ipc_gcr_s0ix")) {
>> +		dev_err(&pdev->dev, "Failed to request s0ix mem region\n");
>> +		iounmap(ipcdev.ipc_base);
>> +		release_mem_region(res->start, size);
>> +		return -EBUSY;
>> +	}
>> +
>> +	addr = ioremap_nocache(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>> +	if (!addr) {
>> +		dev_err(&pdev->dev, "s0ix I/O memory remapping failed\n");
>> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>> +		iounmap(ipcdev.ipc_base);
>> +		release_mem_region(res->start, size);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ipcdev.gcr_s0ix_base = addr;
>> +
>> +	dev_info(&pdev->dev, "s0ix mem region 0x%llx-0x%llx\n",
>> +		 ipcdev.gcr_s0ix_start,
>> +		 (ipcdev.gcr_s0ix_start + ipcdev.gcr_s0ix_size - 1));
>> +
>>   	ipcdev.telem_res_inval = 0;
>>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>>   				    PLAT_RESOURCE_TELEM_SSRAM_INDEX);
>> @@ -782,8 +817,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_s0ix_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
>> +	shlw = gcr_s0ix_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
>>   
>>   	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
>>   
>> @@ -851,12 +886,13 @@ static int ipc_plat_probe(struct platform_device *pdev)
>>   	platform_device_unregister(ipcdev.telemetry_dev);
>>   err_device:
>>   	iounmap(ipcdev.ipc_base);
>> +	iounmap(ipcdev.gcr_s0ix_base);
>>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>>   				    PLAT_RESOURCE_IPC_INDEX);
>>   	if (res) {
>> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>>   		release_mem_region(res->start,
>> -				   PLAT_RESOURCE_IPC_SIZE +
>> -				   PLAT_RESOURCE_GCR_SIZE);
>> +				   PLAT_RESOURCE_IPC_SIZE);
>>   	}
>>   	return ret;
>>   }
>> @@ -871,12 +907,13 @@ static int ipc_plat_remove(struct platform_device *pdev)
>>   	platform_device_unregister(ipcdev.punit_dev);
>>   	platform_device_unregister(ipcdev.telemetry_dev);
>>   	iounmap(ipcdev.ipc_base);
>> +	iounmap(ipcdev.gcr_s0ix_base);
>>   	res = platform_get_resource(pdev, IORESOURCE_MEM,
>>   				    PLAT_RESOURCE_IPC_INDEX);
>>   	if (res) {
>> +		release_mem_region(ipcdev.gcr_s0ix_start, ipcdev.gcr_s0ix_size);
>>   		release_mem_region(res->start,
>> -				   PLAT_RESOURCE_IPC_SIZE +
>> -				   PLAT_RESOURCE_GCR_SIZE);
>> +				   PLAT_RESOURCE_IPC_SIZE);
>>   	}
>>   	ipcdev.dev = NULL;
>>   	return 0;
>> -- 
>> 2.7.4
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 11:50:16AM -0700, sathyanarayanan kuppuswamy wrote:
> Hi,
> 
> 
> On 03/16/2017 07:52 AM, Rajneesh Bhardwaj wrote:
> >On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >>Mapping entire GCR mem region in this driver creates
> >>mem region request conflict in sub devices that depend
> >>on PMC. This creates driver probe failure in devices like
> >>iTC0_wdt and telemetry device.
> >While this patch might fix the issue for now but IMHO its not taking the
> >right approch. I guess we need some guidance here from the maintainers but
> Agreed. I thought about this problem and I know this solution does not
> scale well. Other way is to expose an API for GCR access and use it on
> PMC dependent drivers. In this case, we should also add GCR register
> address macros to PMC header file and this might need change to PMC header
> file each time when some one wants to add access new register address.
> 
> Since S0ix counter access is only GCR access use case in PMC driver, I
> thought  both solutions has some pros and cons.
> >please do consider the below explaination for why we shoud not take this
> >approch to fix WDT issue. Telemetry driver has no issues while loading since
> >its not using any register in the GCR region.
> >PMC on BXT/APL platforms has contiguous IPC and GCR regions. PMC_IPC driver
> >maps the entire IPC and GCR region. It would be inefficient to map and unmap
> >each time we want to use another register present in IPC or GCR spaces.
> But I am wondering whether there will be any future GCR access
> changes in PMC driver?

You never know!

> If its going to be just S0ix counter access then I don't think we
> need to worry about performance here.

Ditto, its not about performance.

> >
> >iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
> >(Offset: 0x1008) and this falls in GCR space. The iTCO_WDT driver fails to
> >load because it can't request mem region for the resources already claimed
> >by PMC_IPC driver. However, ioremap would still work here and WDT driver
> >would load just fine.
> >
> >So, IMHO the problem lies elsewhere and we should find a way to handle this
> >better in iTCO_WDT driver.
> >
> >The IPC and GCR resources belong to PMC and should be claimed by the PMC
> >driver rightfully and should not be reclaimed by iTCO_WDT or any other
> >driver.
> iTCO_WDT , Telemtry and Punit are PMC dependent devices right ? And they
> share the resources among them right ?

Resources belong to PMC and hence it should own them, others share. Other
drivers request and map those resources only when PMC driver does not
already do so.

> >
> >
> >>Currently this driver only need memory mapping for
> >>s0ix counter registers. So this patch fixes this issue
> >>by requesting memory mapping for only the s0ix counter mem
> >>region.
> >How about exposing a new API in PMC_IPC driver which can be used for reading
> >the desired GCR register and it can be used by iTCO_WDT instead of
> >requesting mem regions and remapping?
> If you think in future if we might need access to more GCR space in
> PMC driver, then we need to change this solution. 
>
> Please let me know. If yes, I will add an API as you mentioned.

Yes, I think so. Hope Andy and Darren are fine with that?


> >

-- 
Best Regards,
Rajneesh

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 11:50:16AM -0700, sathyanarayanan kuppuswamy wrote:
> Hi,
> 
> 
> On 03/16/2017 07:52 AM, Rajneesh Bhardwaj wrote:
> >On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >>Mapping entire GCR mem region in this driver creates
> >>mem region request conflict in sub devices that depend
> >>on PMC. This creates driver probe failure in devices like
> >>iTC0_wdt and telemetry device.
> >While this patch might fix the issue for now but IMHO its not taking the
> >right approch. I guess we need some guidance here from the maintainers but
> Agreed. I thought about this problem and I know this solution does not
> scale well. Other way is to expose an API for GCR access and use it on
> PMC dependent drivers. In this case, we should also add GCR register
> address macros to PMC header file and this might need change to PMC header
> file each time when some one wants to add access new register address.
> 
> Since S0ix counter access is only GCR access use case in PMC driver, I
> thought  both solutions has some pros and cons.
> >please do consider the below explaination for why we shoud not take this
> >approch to fix WDT issue. Telemetry driver has no issues while loading since
> >its not using any register in the GCR region.
> >PMC on BXT/APL platforms has contiguous IPC and GCR regions. PMC_IPC driver
> >maps the entire IPC and GCR region. It would be inefficient to map and unmap
> >each time we want to use another register present in IPC or GCR spaces.
> But I am wondering whether there will be any future GCR access
> changes in PMC driver?

You never know!

> If its going to be just S0ix counter access then I don't think we
> need to worry about performance here.

Ditto, its not about performance.

> >
> >iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
> >(Offset: 0x1008) and this falls in GCR space. The iTCO_WDT driver fails to
> >load because it can't request mem region for the resources already claimed
> >by PMC_IPC driver. However, ioremap would still work here and WDT driver
> >would load just fine.
> >
> >So, IMHO the problem lies elsewhere and we should find a way to handle this
> >better in iTCO_WDT driver.
> >
> >The IPC and GCR resources belong to PMC and should be claimed by the PMC
> >driver rightfully and should not be reclaimed by iTCO_WDT or any other
> >driver.
> iTCO_WDT , Telemtry and Punit are PMC dependent devices right ? And they
> share the resources among them right ?

Resources belong to PMC and hence it should own them, others share. Other
drivers request and map those resources only when PMC driver does not
already do so.

> >
> >
> >>Currently this driver only need memory mapping for
> >>s0ix counter registers. So this patch fixes this issue
> >>by requesting memory mapping for only the s0ix counter mem
> >>region.
> >How about exposing a new API in PMC_IPC driver which can be used for reading
> >the desired GCR register and it can be used by iTCO_WDT instead of
> >requesting mem regions and remapping?
> If you think in future if we might need access to more GCR space in
> PMC driver, then we need to change this solution. 
>
> Please let me know. If yes, I will add an API as you mentioned.

Yes, I think so. Hope Andy and Darren are fine with that?


> >

-- 
Best Regards,
Rajneesh

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[sathyanarayanan kuppuswamy]

On 03/16/2017 12:20 PM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 11:50:16AM -0700, sathyanarayanan kuppuswamy wrote:
>> Hi,
>>
>>
>> On 03/16/2017 07:52 AM, Rajneesh Bhardwaj wrote:
>>> On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
>>>> Mapping entire GCR mem region in this driver creates
>>>> mem region request conflict in sub devices that depend
>>>> on PMC. This creates driver probe failure in devices like
>>>> iTC0_wdt and telemetry device.
>>> While this patch might fix the issue for now but IMHO its not taking the
>>> right approch. I guess we need some guidance here from the maintainers but
>> Agreed. I thought about this problem and I know this solution does not
>> scale well. Other way is to expose an API for GCR access and use it on
>> PMC dependent drivers. In this case, we should also add GCR register
>> address macros to PMC header file and this might need change to PMC header
>> file each time when some one wants to add access new register address.
>>
>> Since S0ix counter access is only GCR access use case in PMC driver, I
>> thought  both solutions has some pros and cons.
>>> please do consider the below explaination for why we shoud not take this
>>> approch to fix WDT issue. Telemetry driver has no issues while loading since
>>> its not using any register in the GCR region.
>>> PMC on BXT/APL platforms has contiguous IPC and GCR regions. PMC_IPC driver
>>> maps the entire IPC and GCR region. It would be inefficient to map and unmap
>>> each time we want to use another register present in IPC or GCR spaces.
>> But I am wondering whether there will be any future GCR access
>> changes in PMC driver?
> You never know!
>
>> If its going to be just S0ix counter access then I don't think we
>> need to worry about performance here.
> Ditto, its not about performance.
>
>>> iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
>>> (Offset: 0x1008) and this falls in GCR space. The iTCO_WDT driver fails to
>>> load because it can't request mem region for the resources already claimed
>>> by PMC_IPC driver. However, ioremap would still work here and WDT driver
>>> would load just fine.
>>>
>>> So, IMHO the problem lies elsewhere and we should find a way to handle this
>>> better in iTCO_WDT driver.
>>>
>>> The IPC and GCR resources belong to PMC and should be claimed by the PMC
>>> driver rightfully and should not be reclaimed by iTCO_WDT or any other
>>> driver.
>> iTCO_WDT , Telemtry and Punit are PMC dependent devices right ? And they
>> share the resources among them right ?
> Resources belong to PMC and hence it should own them, others share. Other
> drivers request and map those resources only when PMC driver does not
> already do so.
I see your point.
Correct way is to use MFD driver architecture for this device driver and 
use regmap for GCR registers.
We can get the regmap pointer in dependent device driver by getting the 
private data of parent device.
>
>>>
>>>> Currently this driver only need memory mapping for
>>>> s0ix counter registers. So this patch fixes this issue
>>>> by requesting memory mapping for only the s0ix counter mem
>>>> region.
>>> How about exposing a new API in PMC_IPC driver which can be used for reading
>>> the desired GCR register and it can be used by iTCO_WDT instead of
>>> requesting mem regions and remapping?
>> If you think in future if we might need access to more GCR space in
>> PMC driver, then we need to change this solution.
>>
>> Please let me know. If yes, I will add an API as you mentioned.
> Yes, I think so. Hope Andy and Darren are fine with that?
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[sathyanarayanan kuppuswamy]

On 03/16/2017 12:20 PM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 11:50:16AM -0700, sathyanarayanan kuppuswamy wrote:
>> Hi,
>>
>>
>> On 03/16/2017 07:52 AM, Rajneesh Bhardwaj wrote:
>>> On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
>>>> Mapping entire GCR mem region in this driver creates
>>>> mem region request conflict in sub devices that depend
>>>> on PMC. This creates driver probe failure in devices like
>>>> iTC0_wdt and telemetry device.
>>> While this patch might fix the issue for now but IMHO its not taking the
>>> right approch. I guess we need some guidance here from the maintainers but
>> Agreed. I thought about this problem and I know this solution does not
>> scale well. Other way is to expose an API for GCR access and use it on
>> PMC dependent drivers. In this case, we should also add GCR register
>> address macros to PMC header file and this might need change to PMC header
>> file each time when some one wants to add access new register address.
>>
>> Since S0ix counter access is only GCR access use case in PMC driver, I
>> thought  both solutions has some pros and cons.
>>> please do consider the below explaination for why we shoud not take this
>>> approch to fix WDT issue. Telemetry driver has no issues while loading since
>>> its not using any register in the GCR region.
>>> PMC on BXT/APL platforms has contiguous IPC and GCR regions. PMC_IPC driver
>>> maps the entire IPC and GCR region. It would be inefficient to map and unmap
>>> each time we want to use another register present in IPC or GCR spaces.
>> But I am wondering whether there will be any future GCR access
>> changes in PMC driver?
> You never know!
>
>> If its going to be just S0ix counter access then I don't think we
>> need to worry about performance here.
> Ditto, its not about performance.
>
>>> iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
>>> (Offset: 0x1008) and this falls in GCR space. The iTCO_WDT driver fails to
>>> load because it can't request mem region for the resources already claimed
>>> by PMC_IPC driver. However, ioremap would still work here and WDT driver
>>> would load just fine.
>>>
>>> So, IMHO the problem lies elsewhere and we should find a way to handle this
>>> better in iTCO_WDT driver.
>>>
>>> The IPC and GCR resources belong to PMC and should be claimed by the PMC
>>> driver rightfully and should not be reclaimed by iTCO_WDT or any other
>>> driver.
>> iTCO_WDT , Telemtry and Punit are PMC dependent devices right ? And they
>> share the resources among them right ?
> Resources belong to PMC and hence it should own them, others share. Other
> drivers request and map those resources only when PMC driver does not
> already do so.
I see your point.
Correct way is to use MFD driver architecture for this device driver and 
use regmap for GCR registers.
We can get the regmap pointer in dependent device driver by getting the 
private data of parent device.
>
>>>
>>>> Currently this driver only need memory mapping for
>>>> s0ix counter registers. So this patch fixes this issue
>>>> by requesting memory mapping for only the s0ix counter mem
>>>> region.
>>> How about exposing a new API in PMC_IPC driver which can be used for reading
>>> the desired GCR register and it can be used by iTCO_WDT instead of
>>> requesting mem regions and remapping?
>> If you think in future if we might need access to more GCR space in
>> PMC driver, then we need to change this solution.
>>
>> Please let me know. If yes, I will add an API as you mentioned.
> Yes, I think so. Hope Andy and Darren are fine with that?
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

[PATCH v2 1/4] 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 v2 1/4] 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 v2 2/4] platform/x86: intel_pmc_ipc: Add pmc gcr read/write api's

[Kuppuswamy Sathyanarayanan]

This patch adds API's to read/write 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 | 16 ++++++++++++++++
 drivers/platform/x86/intel_pmc_ipc.c | 14 ++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
index 4291b6a..017429d 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -23,6 +23,10 @@
 #define IPC_ERR_EMSECURITY		6
 #define IPC_ERR_UNSIGNEDKERNEL		7
 
+/* GCR reg offsets from gcr base*/
+#define PMC_GCR_PRSTS_REG		0x00
+#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 +35,8 @@ 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);
+u32 intel_pmc_gcr_read(u32 offset);
+void intel_pmc_gcr_write(u32 offset, u32 data);
 
 #else
 
@@ -56,6 +62,16 @@ static inline int intel_pmc_s0ix_counter_read(u64 *data)
 	return -EINVAL;
 }
 
+static inline u32 intel_pmc_gcr_read(u32 offset)
+{
+	return -EINVAL;
+}
+
+static inline void intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	return;
+}
+
 #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..12018f3 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,18 @@ static inline u64 gcr_data_readq(u32 offset)
 	return readq(ipcdev.ipc_base + offset);
 }
 
+u32 intel_pmc_gcr_read(u32 offset)
+{
+	return readl(ipcdev.gcr_mem_base + offset);
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
+
+void intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	writel(data, ipcdev.gcr_mem_base + offset);
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -747,6 +760,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 v2 2/4] platform/x86: intel_pmc_ipc: Add pmc gcr read/write api's

[Kuppuswamy Sathyanarayanan]

This patch adds API's to read/write 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 | 16 ++++++++++++++++
 drivers/platform/x86/intel_pmc_ipc.c | 14 ++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
index 4291b6a..017429d 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -23,6 +23,10 @@
 #define IPC_ERR_EMSECURITY		6
 #define IPC_ERR_UNSIGNEDKERNEL		7
 
+/* GCR reg offsets from gcr base*/
+#define PMC_GCR_PRSTS_REG		0x00
+#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 +35,8 @@ 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);
+u32 intel_pmc_gcr_read(u32 offset);
+void intel_pmc_gcr_write(u32 offset, u32 data);
 
 #else
 
@@ -56,6 +62,16 @@ static inline int intel_pmc_s0ix_counter_read(u64 *data)
 	return -EINVAL;
 }
 
+static inline u32 intel_pmc_gcr_read(u32 offset)
+{
+	return -EINVAL;
+}
+
+static inline void intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	return;
+}
+
 #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..12018f3 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,18 @@ static inline u64 gcr_data_readq(u32 offset)
 	return readq(ipcdev.ipc_base + offset);
 }
 
+u32 intel_pmc_gcr_read(u32 offset)
+{
+	return readl(ipcdev.gcr_mem_base + offset);
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
+
+void intel_pmc_gcr_write(u32 offset, u32 data)
+{
+	writel(data, ipcdev.gcr_mem_base + offset);
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -747,6 +760,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 v2 2/4] platform/x86: intel_pmc_ipc: Add pmc gcr read/write api's

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 05:41:34PM -0700, Kuppuswamy Sathyanarayanan wrote:
> This patch adds API's to read/write 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 | 16 ++++++++++++++++
>  drivers/platform/x86/intel_pmc_ipc.c | 14 ++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
> index 4291b6a..017429d 100644
> --- a/arch/x86/include/asm/intel_pmc_ipc.h
> +++ b/arch/x86/include/asm/intel_pmc_ipc.h
> @@ -23,6 +23,10 @@
>  #define IPC_ERR_EMSECURITY		6
>  #define IPC_ERR_UNSIGNEDKERNEL		7
>  
> +/* GCR reg offsets from gcr base*/
> +#define PMC_GCR_PRSTS_REG		0x00

remove.

> +#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 +35,8 @@ 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);
> +u32 intel_pmc_gcr_read(u32 offset);

consider changing the signature to read data as out param and use return
value for better error handling since exported API can be called from
anywhere in the kernel.

> +void intel_pmc_gcr_write(u32 offset, u32 data);

ditto.

>  
>  #else
>  
> @@ -56,6 +62,16 @@ static inline int intel_pmc_s0ix_counter_read(u64 *data)
>  	return -EINVAL;
>  }
>  
> +static inline u32 intel_pmc_gcr_read(u32 offset)
> +{
> +	return -EINVAL;
> +}
> +

samew as above.

> +static inline void intel_pmc_gcr_write(u32 offset, u32 data)
> +{
> +	return;
> +}
> +

ditto.

>  #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..12018f3 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,18 @@ static inline u64 gcr_data_readq(u32 offset)
>  	return readq(ipcdev.ipc_base + offset);
>  }
>  
> +u32 intel_pmc_gcr_read(u32 offset)
> +{
> +	return readl(ipcdev.gcr_mem_base + offset);
> +}

what happens when this is called with a wrong offset on IPC enabled
platforms?

> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
> +
> +void intel_pmc_gcr_write(u32 offset, u32 data)
> +{
> +	writel(data, ipcdev.gcr_mem_base + offset);
> +}

same as above.

> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
> +
>  static int intel_pmc_ipc_check_status(void)
>  {
>  	int status;
> @@ -747,6 +760,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

Re: [PATCH v2 2/4] platform/x86: intel_pmc_ipc: Add pmc gcr read/write api's

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 05:41:34PM -0700, Kuppuswamy Sathyanarayanan wrote:
> This patch adds API's to read/write 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 | 16 ++++++++++++++++
>  drivers/platform/x86/intel_pmc_ipc.c | 14 ++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
> index 4291b6a..017429d 100644
> --- a/arch/x86/include/asm/intel_pmc_ipc.h
> +++ b/arch/x86/include/asm/intel_pmc_ipc.h
> @@ -23,6 +23,10 @@
>  #define IPC_ERR_EMSECURITY		6
>  #define IPC_ERR_UNSIGNEDKERNEL		7
>  
> +/* GCR reg offsets from gcr base*/
> +#define PMC_GCR_PRSTS_REG		0x00

remove.

> +#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 +35,8 @@ 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);
> +u32 intel_pmc_gcr_read(u32 offset);

consider changing the signature to read data as out param and use return
value for better error handling since exported API can be called from
anywhere in the kernel.

> +void intel_pmc_gcr_write(u32 offset, u32 data);

ditto.

>  
>  #else
>  
> @@ -56,6 +62,16 @@ static inline int intel_pmc_s0ix_counter_read(u64 *data)
>  	return -EINVAL;
>  }
>  
> +static inline u32 intel_pmc_gcr_read(u32 offset)
> +{
> +	return -EINVAL;
> +}
> +

samew as above.

> +static inline void intel_pmc_gcr_write(u32 offset, u32 data)
> +{
> +	return;
> +}
> +

ditto.

>  #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..12018f3 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,18 @@ static inline u64 gcr_data_readq(u32 offset)
>  	return readq(ipcdev.ipc_base + offset);
>  }
>  
> +u32 intel_pmc_gcr_read(u32 offset)
> +{
> +	return readl(ipcdev.gcr_mem_base + offset);
> +}

what happens when this is called with a wrong offset on IPC enabled
platforms?

> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
> +
> +void intel_pmc_gcr_write(u32 offset, u32 data)
> +{
> +	writel(data, ipcdev.gcr_mem_base + offset);
> +}

same as above.

> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
> +
>  static int intel_pmc_ipc_check_status(void)
>  {
>  	int status;
> @@ -747,6 +760,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

Re: [PATCH v2 2/4] platform/x86: intel_pmc_ipc: Add pmc gcr read/write api's

[sathyanarayanan kuppuswamy]

Hi,


On 03/17/2017 04:26 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:34PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> This patch adds API's to read/write 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 | 16 ++++++++++++++++
>>   drivers/platform/x86/intel_pmc_ipc.c | 14 ++++++++++++++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
>> index 4291b6a..017429d 100644
>> --- a/arch/x86/include/asm/intel_pmc_ipc.h
>> +++ b/arch/x86/include/asm/intel_pmc_ipc.h
>> @@ -23,6 +23,10 @@
>>   #define IPC_ERR_EMSECURITY		6
>>   #define IPC_ERR_UNSIGNEDKERNEL		7
>>   
>> +/* GCR reg offsets from gcr base*/
>> +#define PMC_GCR_PRSTS_REG		0x00
> remove.
Will remove it in next series.
>
>> +#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 +35,8 @@ 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);
>> +u32 intel_pmc_gcr_read(u32 offset);
> consider changing the signature to read data as out param and use return
> value for better error handling since exported API can be called from
> anywhere in the kernel.
I can add check to make sure offset is within GCR range.
>
>> +void intel_pmc_gcr_write(u32 offset, u32 data);
> ditto.
Same as above.
>
>>   
>>   #else
>>   
>> @@ -56,6 +62,16 @@ static inline int intel_pmc_s0ix_counter_read(u64 *data)
>>   	return -EINVAL;
>>   }
>>   
>> +static inline u32 intel_pmc_gcr_read(u32 offset)
>> +{
>> +	return -EINVAL;
>> +}
>> +
> samew as above.
Same as above.
>
>> +static inline void intel_pmc_gcr_write(u32 offset, u32 data)
>> +{
>> +	return;
>> +}
>> +
> ditto.
Same as above.
>
>>   #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..12018f3 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,18 @@ static inline u64 gcr_data_readq(u32 offset)
>>   	return readq(ipcdev.ipc_base + offset);
>>   }
>>   
>> +u32 intel_pmc_gcr_read(u32 offset)
>> +{
>> +	return readl(ipcdev.gcr_mem_base + offset);
>> +}
> what happens when this is called with a wrong offset on IPC enabled
> platforms?
>
>> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
>> +
>> +void intel_pmc_gcr_write(u32 offset, u32 data)
>> +{
>> +	writel(data, ipcdev.gcr_mem_base + offset);
>> +}
> same as above.
>
>> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
>> +
>>   static int intel_pmc_ipc_check_status(void)
>>   {
>>   	int status;
>> @@ -747,6 +760,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
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 2/4] platform/x86: intel_pmc_ipc: Add pmc gcr read/write api's

[sathyanarayanan kuppuswamy]

Hi,


On 03/17/2017 04:26 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:34PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> This patch adds API's to read/write 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 | 16 ++++++++++++++++
>>   drivers/platform/x86/intel_pmc_ipc.c | 14 ++++++++++++++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
>> index 4291b6a..017429d 100644
>> --- a/arch/x86/include/asm/intel_pmc_ipc.h
>> +++ b/arch/x86/include/asm/intel_pmc_ipc.h
>> @@ -23,6 +23,10 @@
>>   #define IPC_ERR_EMSECURITY		6
>>   #define IPC_ERR_UNSIGNEDKERNEL		7
>>   
>> +/* GCR reg offsets from gcr base*/
>> +#define PMC_GCR_PRSTS_REG		0x00
> remove.
Will remove it in next series.
>
>> +#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 +35,8 @@ 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);
>> +u32 intel_pmc_gcr_read(u32 offset);
> consider changing the signature to read data as out param and use return
> value for better error handling since exported API can be called from
> anywhere in the kernel.
I can add check to make sure offset is within GCR range.
>
>> +void intel_pmc_gcr_write(u32 offset, u32 data);
> ditto.
Same as above.
>
>>   
>>   #else
>>   
>> @@ -56,6 +62,16 @@ static inline int intel_pmc_s0ix_counter_read(u64 *data)
>>   	return -EINVAL;
>>   }
>>   
>> +static inline u32 intel_pmc_gcr_read(u32 offset)
>> +{
>> +	return -EINVAL;
>> +}
>> +
> samew as above.
Same as above.
>
>> +static inline void intel_pmc_gcr_write(u32 offset, u32 data)
>> +{
>> +	return;
>> +}
>> +
> ditto.
Same as above.
>
>>   #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..12018f3 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,18 @@ static inline u64 gcr_data_readq(u32 offset)
>>   	return readq(ipcdev.ipc_base + offset);
>>   }
>>   
>> +u32 intel_pmc_gcr_read(u32 offset)
>> +{
>> +	return readl(ipcdev.gcr_mem_base + offset);
>> +}
> what happens when this is called with a wrong offset on IPC enabled
> platforms?
>
>> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
>> +
>> +void intel_pmc_gcr_write(u32 offset, u32 data)
>> +{
>> +	writel(data, ipcdev.gcr_mem_base + offset);
>> +}
> same as above.
>
>> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
>> +
>>   static int intel_pmc_ipc_check_status(void)
>>   {
>>   	int status;
>> @@ -747,6 +760,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
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

[PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Kuppuswamy Sathyanarayanan]

Currently, iTCO watchdog driver uses memory map to access
PMC_CFG GCR register. But the entire GCR address space is
already mapped in intel_scu_ipc driver. So remapping the
GCR register in this driver causes the mem request failure in
iTCO_wdt probe function. This patch fixes this issue by
using PMC GCR read/write API's to access PMC_CFG register.

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

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3d0abc0..31abfc5 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -68,6 +68,8 @@
 #include <linux/io.h>			/* For inb/outb/... */
 #include <linux/platform_data/itco_wdt.h>
 
+#include <asm/intel_pmc_ipc.h>
+
 #include "iTCO_vendor.h"
 
 /* Address definitions for the TCO */
@@ -94,12 +96,6 @@ struct iTCO_wdt_private {
 	unsigned int iTCO_version;
 	struct resource *tco_res;
 	struct resource *smi_res;
-	/*
-	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
-	 * or memory-mapped PMC register bit 4 (TCO version 3).
-	 */
-	struct resource *gcs_pmc_res;
-	unsigned long __iomem *gcs_pmc;
 	/* the lock for io operations */
 	spinlock_t io_lock;
 	/* the PCI-device */
@@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
 		val32 |= no_reboot_bit(p);
-		writel(val32, p->gcs_pmc);
+		intel_pmc_gcr_write(PMC_GCR_PMC_CFG_REG, val32);
 	} else if (p->iTCO_version == 1) {
 		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
 		val32 |= no_reboot_bit(p);
@@ -193,11 +189,11 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
 		val32 &= ~enable_bit;
-		writel(val32, p->gcs_pmc);
+		intel_pmc_gcr_write(PMC_GCR_PMC_CFG_REG, val32);
 
-		val32 = readl(p->gcs_pmc);
+		val32 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
 	} else if (p->iTCO_version == 1) {
 		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
 		val32 &= ~enable_bit;
@@ -428,19 +424,6 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	p->iTCO_version = pdata->version;
 	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) {
-		p->gcs_pmc_res = platform_get_resource(pdev,
-						       IORESOURCE_MEM,
-						       ICH_RES_MEM_GCS_PMC);
-		p->gcs_pmc = devm_ioremap_resource(dev, p->gcs_pmc_res);
-		if (IS_ERR(p->gcs_pmc))
-			return PTR_ERR(p->gcs_pmc);
-	}
-
 	/* Check chipset's NO_REBOOT bit */
 	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
 	    iTCO_vendor_check_noreboot_on()) {
-- 
2.7.4

[PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Kuppuswamy Sathyanarayanan]

Currently, iTCO watchdog driver uses memory map to access
PMC_CFG GCR register. But the entire GCR address space is
already mapped in intel_scu_ipc driver. So remapping the
GCR register in this driver causes the mem request failure in
iTCO_wdt probe function. This patch fixes this issue by
using PMC GCR read/write API's to access PMC_CFG register.

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

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 3d0abc0..31abfc5 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -68,6 +68,8 @@
 #include <linux/io.h>			/* For inb/outb/... */
 #include <linux/platform_data/itco_wdt.h>
 
+#include <asm/intel_pmc_ipc.h>
+
 #include "iTCO_vendor.h"
 
 /* Address definitions for the TCO */
@@ -94,12 +96,6 @@ struct iTCO_wdt_private {
 	unsigned int iTCO_version;
 	struct resource *tco_res;
 	struct resource *smi_res;
-	/*
-	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
-	 * or memory-mapped PMC register bit 4 (TCO version 3).
-	 */
-	struct resource *gcs_pmc_res;
-	unsigned long __iomem *gcs_pmc;
 	/* the lock for io operations */
 	spinlock_t io_lock;
 	/* the PCI-device */
@@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
 		val32 |= no_reboot_bit(p);
-		writel(val32, p->gcs_pmc);
+		intel_pmc_gcr_write(PMC_GCR_PMC_CFG_REG, val32);
 	} else if (p->iTCO_version == 1) {
 		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
 		val32 |= no_reboot_bit(p);
@@ -193,11 +189,11 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
 		val32 &= ~enable_bit;
-		writel(val32, p->gcs_pmc);
+		intel_pmc_gcr_write(PMC_GCR_PMC_CFG_REG, val32);
 
-		val32 = readl(p->gcs_pmc);
+		val32 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
 	} else if (p->iTCO_version == 1) {
 		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
 		val32 &= ~enable_bit;
@@ -428,19 +424,6 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	p->iTCO_version = pdata->version;
 	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) {
-		p->gcs_pmc_res = platform_get_resource(pdev,
-						       IORESOURCE_MEM,
-						       ICH_RES_MEM_GCS_PMC);
-		p->gcs_pmc = devm_ioremap_resource(dev, p->gcs_pmc_res);
-		if (IS_ERR(p->gcs_pmc))
-			return PTR_ERR(p->gcs_pmc);
-	}
-
 	/* Check chipset's NO_REBOOT bit */
 	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
 	    iTCO_vendor_check_noreboot_on()) {
-- 
2.7.4

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Currently, iTCO watchdog driver uses memory map to access
> PMC_CFG GCR register. But the entire GCR address space is
> already mapped in intel_scu_ipc driver. So remapping the

intel_pmc_ipc driver.

> GCR register in this driver causes the mem request failure in
> iTCO_wdt probe function. This patch fixes this issue by
> using PMC GCR read/write API's to access PMC_CFG register.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>  1 file changed, 7 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 3d0abc0..31abfc5 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -68,6 +68,8 @@
>  #include <linux/io.h>			/* For inb/outb/... */
>  #include <linux/platform_data/itco_wdt.h>
>  
> +#include <asm/intel_pmc_ipc.h>
> +
>  #include "iTCO_vendor.h"
>  
>  /* Address definitions for the TCO */
> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>  	unsigned int iTCO_version;
>  	struct resource *tco_res;
>  	struct resource *smi_res;
> -	/*
> -	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
> -	 * or memory-mapped PMC register bit 4 (TCO version 3).
> -	 */

Better to retain this comment elsewhere.

> -	struct resource *gcs_pmc_res;
> -	unsigned long __iomem *gcs_pmc;
>  	/* the lock for io operations */
>  	spinlock_t io_lock;
>  	/* the PCI-device */
> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);

better to have protection and error handling, discussed in v2, 2/4.

compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
it impacts core WDT functionality, need to be thoroughly tested on various
platforms.

> -- 
> 2.7.4
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Currently, iTCO watchdog driver uses memory map to access
> PMC_CFG GCR register. But the entire GCR address space is
> already mapped in intel_scu_ipc driver. So remapping the

intel_pmc_ipc driver.

> GCR register in this driver causes the mem request failure in
> iTCO_wdt probe function. This patch fixes this issue by
> using PMC GCR read/write API's to access PMC_CFG register.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>  1 file changed, 7 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 3d0abc0..31abfc5 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -68,6 +68,8 @@
>  #include <linux/io.h>			/* For inb/outb/... */
>  #include <linux/platform_data/itco_wdt.h>
>  
> +#include <asm/intel_pmc_ipc.h>
> +
>  #include "iTCO_vendor.h"
>  
>  /* Address definitions for the TCO */
> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>  	unsigned int iTCO_version;
>  	struct resource *tco_res;
>  	struct resource *smi_res;
> -	/*
> -	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
> -	 * or memory-mapped PMC register bit 4 (TCO version 3).
> -	 */

Better to retain this comment elsewhere.

> -	struct resource *gcs_pmc_res;
> -	unsigned long __iomem *gcs_pmc;
>  	/* the lock for io operations */
>  	spinlock_t io_lock;
>  	/* the PCI-device */
> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);

better to have protection and error handling, discussed in v2, 2/4.

compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
it impacts core WDT functionality, need to be thoroughly tested on various
platforms.

> -- 
> 2.7.4
> 

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

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Currently, iTCO watchdog driver uses memory map to access
> PMC_CFG GCR register. But the entire GCR address space is
> already mapped in intel_scu_ipc driver. So remapping the

intel_pmc_ipc driver.

> GCR register in this driver causes the mem request failure in
> iTCO_wdt probe function. This patch fixes this issue by
> using PMC GCR read/write API's to access PMC_CFG register.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>  1 file changed, 7 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 3d0abc0..31abfc5 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -68,6 +68,8 @@
>  #include <linux/io.h>			/* For inb/outb/... */
>  #include <linux/platform_data/itco_wdt.h>
>  
> +#include <asm/intel_pmc_ipc.h>
> +
>  #include "iTCO_vendor.h"
>  
>  /* Address definitions for the TCO */
> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>  	unsigned int iTCO_version;
>  	struct resource *tco_res;
>  	struct resource *smi_res;
> -	/*
> -	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
> -	 * or memory-mapped PMC register bit 4 (TCO version 3).
> -	 */

Better to retain this comment elsewhere.

> -	struct resource *gcs_pmc_res;
> -	unsigned long __iomem *gcs_pmc;
>  	/* the lock for io operations */
>  	spinlock_t io_lock;
>  	/* the PCI-device */
> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);

better to have protection and error handling, discussed in v2, 2/4.

compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
it impacts core WDT functionality, need to be thoroughly tested on various
platforms.

> -- 
> 2.7.4
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Guenter Roeck]

On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Currently, iTCO watchdog driver uses memory map to access
>> PMC_CFG GCR register. But the entire GCR address space is
>> already mapped in intel_scu_ipc driver. So remapping the
>
> intel_pmc_ipc driver.
>
>> GCR register in this driver causes the mem request failure in
>> iTCO_wdt probe function. This patch fixes this issue by
>> using PMC GCR read/write API's to access PMC_CFG register.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>  drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>  1 file changed, 7 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>> index 3d0abc0..31abfc5 100644
>> --- a/drivers/watchdog/iTCO_wdt.c
>> +++ b/drivers/watchdog/iTCO_wdt.c
>> @@ -68,6 +68,8 @@
>>  #include <linux/io.h>			/* For inb/outb/... */
>>  #include <linux/platform_data/itco_wdt.h>
>>
>> +#include <asm/intel_pmc_ipc.h>
>> +
>>  #include "iTCO_vendor.h"
>>
>>  /* Address definitions for the TCO */
>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>  	unsigned int iTCO_version;
>>  	struct resource *tco_res;
>>  	struct resource *smi_res;
>> -	/*
>> -	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
>> -	 * or memory-mapped PMC register bit 4 (TCO version 3).
>> -	 */
>
> Better to retain this comment elsewhere.
>
>> -	struct resource *gcs_pmc_res;
>> -	unsigned long __iomem *gcs_pmc;
>>  	/* the lock for io operations */
>>  	spinlock_t io_lock;
>>  	/* the PCI-device */
>> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
>
> better to have protection and error handling, discussed in v2, 2/4.
>
> compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
> it impacts core WDT functionality, need to be thoroughly tested on various
> platforms.
>

I don't think I (or the watchdog mailing list) was copied on the original patch.
Major immediate concern is that this introduces a dependency on external code.
The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
machines". I don't know where the function is introduced, but I hope this change
does not require the pmc_ipc code to be present on such machines for the watchdog
to work. It would be bad if it does. If it doesn't, it appears that the function
should not be declared in asm/intel_pmc_ipc.h.

Guenter

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Guenter Roeck]

On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Currently, iTCO watchdog driver uses memory map to access
>> PMC_CFG GCR register. But the entire GCR address space is
>> already mapped in intel_scu_ipc driver. So remapping the
>
> intel_pmc_ipc driver.
>
>> GCR register in this driver causes the mem request failure in
>> iTCO_wdt probe function. This patch fixes this issue by
>> using PMC GCR read/write API's to access PMC_CFG register.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>  drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>  1 file changed, 7 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>> index 3d0abc0..31abfc5 100644
>> --- a/drivers/watchdog/iTCO_wdt.c
>> +++ b/drivers/watchdog/iTCO_wdt.c
>> @@ -68,6 +68,8 @@
>>  #include <linux/io.h>			/* For inb/outb/... */
>>  #include <linux/platform_data/itco_wdt.h>
>>
>> +#include <asm/intel_pmc_ipc.h>
>> +
>>  #include "iTCO_vendor.h"
>>
>>  /* Address definitions for the TCO */
>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>  	unsigned int iTCO_version;
>>  	struct resource *tco_res;
>>  	struct resource *smi_res;
>> -	/*
>> -	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
>> -	 * or memory-mapped PMC register bit 4 (TCO version 3).
>> -	 */
>
> Better to retain this comment elsewhere.
>
>> -	struct resource *gcs_pmc_res;
>> -	unsigned long __iomem *gcs_pmc;
>>  	/* the lock for io operations */
>>  	spinlock_t io_lock;
>>  	/* the PCI-device */
>> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
>
> better to have protection and error handling, discussed in v2, 2/4.
>
> compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
> it impacts core WDT functionality, need to be thoroughly tested on various
> platforms.
>

I don't think I (or the watchdog mailing list) was copied on the original patch.
Major immediate concern is that this introduces a dependency on external code.
The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
machines". I don't know where the function is introduced, but I hope this change
does not require the pmc_ipc code to be present on such machines for the watchdog
to work. It would be bad if it does. If it doesn't, it appears that the function
should not be declared in asm/intel_pmc_ipc.h.

Guenter

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

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Guenter Roeck]

On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Currently, iTCO watchdog driver uses memory map to access
>> PMC_CFG GCR register. But the entire GCR address space is
>> already mapped in intel_scu_ipc driver. So remapping the
>
> intel_pmc_ipc driver.
>
>> GCR register in this driver causes the mem request failure in
>> iTCO_wdt probe function. This patch fixes this issue by
>> using PMC GCR read/write API's to access PMC_CFG register.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>  drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>  1 file changed, 7 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>> index 3d0abc0..31abfc5 100644
>> --- a/drivers/watchdog/iTCO_wdt.c
>> +++ b/drivers/watchdog/iTCO_wdt.c
>> @@ -68,6 +68,8 @@
>>  #include <linux/io.h>			/* For inb/outb/... */
>>  #include <linux/platform_data/itco_wdt.h>
>>
>> +#include <asm/intel_pmc_ipc.h>
>> +
>>  #include "iTCO_vendor.h"
>>
>>  /* Address definitions for the TCO */
>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>  	unsigned int iTCO_version;
>>  	struct resource *tco_res;
>>  	struct resource *smi_res;
>> -	/*
>> -	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
>> -	 * or memory-mapped PMC register bit 4 (TCO version 3).
>> -	 */
>
> Better to retain this comment elsewhere.
>
>> -	struct resource *gcs_pmc_res;
>> -	unsigned long __iomem *gcs_pmc;
>>  	/* the lock for io operations */
>>  	spinlock_t io_lock;
>>  	/* the PCI-device */
>> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
>
> better to have protection and error handling, discussed in v2, 2/4.
>
> compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
> it impacts core WDT functionality, need to be thoroughly tested on various
> platforms.
>

I don't think I (or the watchdog mailing list) was copied on the original patch.
Major immediate concern is that this introduces a dependency on external code.
The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
machines". I don't know where the function is introduced, but I hope this change
does not require the pmc_ipc code to be present on such machines for the watchdog
to work. It would be bad if it does. If it doesn't, it appears that the function
should not be declared in asm/intel_pmc_ipc.h.

Guenter

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Rajneesh Bhardwaj]

On Fri, Mar 17, 2017 at 06:40:22AM -0700, Guenter Roeck wrote:
> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> >On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >>Currently, iTCO watchdog driver uses memory map to access
> >>PMC_CFG GCR register. But the entire GCR address space is
> >>already mapped in intel_scu_ipc driver. So remapping the
> >
> >intel_pmc_ipc driver.
> >
> >>GCR register in this driver causes the mem request failure in
> >>iTCO_wdt probe function. This patch fixes this issue by
> >>using PMC GCR read/write API's to access PMC_CFG register.
> >>
> >>Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >>---
> >> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
> >> 1 file changed, 7 insertions(+), 24 deletions(-)
> >>
> >>diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> >>index 3d0abc0..31abfc5 100644
> >>--- a/drivers/watchdog/iTCO_wdt.c
> >>+++ b/drivers/watchdog/iTCO_wdt.c
> >>@@ -68,6 +68,8 @@
> >> #include <linux/io.h>			/* For inb/outb/... */
> >> #include <linux/platform_data/itco_wdt.h>
> >>
> >>+#include <asm/intel_pmc_ipc.h>
> >>+
> >> #include "iTCO_vendor.h"
> >>
> >> /* Address definitions for the TCO */
> >>@@ -94,12 +96,6 @@ struct iTCO_wdt_private {
> >> 	unsigned int iTCO_version;
> >> 	struct resource *tco_res;
> >> 	struct resource *smi_res;
> >>-	/*
> >>-	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
> >>-	 * or memory-mapped PMC register bit 4 (TCO version 3).
> >>-	 */
> >
> >Better to retain this comment elsewhere.
> >
> >>-	struct resource *gcs_pmc_res;
> >>-	unsigned long __iomem *gcs_pmc;
> >> 	/* the lock for io operations */
> >> 	spinlock_t io_lock;
> >> 	/* the PCI-device */
> >>@@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
> >
> >better to have protection and error handling, discussed in v2, 2/4.
> >
> >compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
> >it impacts core WDT functionality, need to be thoroughly tested on various
> >platforms.
> >
> 
> I don't think I (or the watchdog mailing list) was copied on the original patch.
> Major immediate concern is that this introduces a dependency on external code.
> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> machines". I don't know where the function is introduced, but I hope this change
> does not require the pmc_ipc code to be present on such machines for the watchdog
> to work. It would be bad if it does. If it doesn't, it appears that the function
> should not be declared in asm/intel_pmc_ipc.h.

Yes i added you and the wdt mailing list since you were missed on original
patch. Apparently, iTCO_wdt driver always had a dependency over PMC
resources when iTCO_version >=2. The below threads should help you give some
background on this.
https://lkml.org/lkml/2017/3/16/504
https://lkml.org/lkml/2017/2/13/282

WDT driver fails to laod since the resources its requesting were reserved by
the PMC driver since they belong to PMC GCR region. WDT driver remapping those
resources is still ok but it cant reclaim them hence the problem.

> 
> Guenter
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Rajneesh Bhardwaj]

On Fri, Mar 17, 2017 at 06:40:22AM -0700, Guenter Roeck wrote:
> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> >On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >>Currently, iTCO watchdog driver uses memory map to access
> >>PMC_CFG GCR register. But the entire GCR address space is
> >>already mapped in intel_scu_ipc driver. So remapping the
> >
> >intel_pmc_ipc driver.
> >
> >>GCR register in this driver causes the mem request failure in
> >>iTCO_wdt probe function. This patch fixes this issue by
> >>using PMC GCR read/write API's to access PMC_CFG register.
> >>
> >>Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >>---
> >> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
> >> 1 file changed, 7 insertions(+), 24 deletions(-)
> >>
> >>diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> >>index 3d0abc0..31abfc5 100644
> >>--- a/drivers/watchdog/iTCO_wdt.c
> >>+++ b/drivers/watchdog/iTCO_wdt.c
> >>@@ -68,6 +68,8 @@
> >> #include <linux/io.h>			/* For inb/outb/... */
> >> #include <linux/platform_data/itco_wdt.h>
> >>
> >>+#include <asm/intel_pmc_ipc.h>
> >>+
> >> #include "iTCO_vendor.h"
> >>
> >> /* Address definitions for the TCO */
> >>@@ -94,12 +96,6 @@ struct iTCO_wdt_private {
> >> 	unsigned int iTCO_version;
> >> 	struct resource *tco_res;
> >> 	struct resource *smi_res;
> >>-	/*
> >>-	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
> >>-	 * or memory-mapped PMC register bit 4 (TCO version 3).
> >>-	 */
> >
> >Better to retain this comment elsewhere.
> >
> >>-	struct resource *gcs_pmc_res;
> >>-	unsigned long __iomem *gcs_pmc;
> >> 	/* the lock for io operations */
> >> 	spinlock_t io_lock;
> >> 	/* the PCI-device */
> >>@@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
> >
> >better to have protection and error handling, discussed in v2, 2/4.
> >
> >compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
> >it impacts core WDT functionality, need to be thoroughly tested on various
> >platforms.
> >
> 
> I don't think I (or the watchdog mailing list) was copied on the original patch.
> Major immediate concern is that this introduces a dependency on external code.
> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> machines". I don't know where the function is introduced, but I hope this change
> does not require the pmc_ipc code to be present on such machines for the watchdog
> to work. It would be bad if it does. If it doesn't, it appears that the function
> should not be declared in asm/intel_pmc_ipc.h.

Yes i added you and the wdt mailing list since you were missed on original
patch. Apparently, iTCO_wdt driver always had a dependency over PMC
resources when iTCO_version >=2. The below threads should help you give some
background on this.
https://lkml.org/lkml/2017/3/16/504
https://lkml.org/lkml/2017/2/13/282

WDT driver fails to laod since the resources its requesting were reserved by
the PMC driver since they belong to PMC GCR region. WDT driver remapping those
resources is still ok but it cant reclaim them hence the problem.

> 
> Guenter
> 

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

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Rajneesh Bhardwaj]

On Fri, Mar 17, 2017 at 06:40:22AM -0700, Guenter Roeck wrote:
> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> >On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >>Currently, iTCO watchdog driver uses memory map to access
> >>PMC_CFG GCR register. But the entire GCR address space is
> >>already mapped in intel_scu_ipc driver. So remapping the
> >
> >intel_pmc_ipc driver.
> >
> >>GCR register in this driver causes the mem request failure in
> >>iTCO_wdt probe function. This patch fixes this issue by
> >>using PMC GCR read/write API's to access PMC_CFG register.
> >>
> >>Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >>---
> >> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
> >> 1 file changed, 7 insertions(+), 24 deletions(-)
> >>
> >>diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> >>index 3d0abc0..31abfc5 100644
> >>--- a/drivers/watchdog/iTCO_wdt.c
> >>+++ b/drivers/watchdog/iTCO_wdt.c
> >>@@ -68,6 +68,8 @@
> >> #include <linux/io.h>			/* For inb/outb/... */
> >> #include <linux/platform_data/itco_wdt.h>
> >>
> >>+#include <asm/intel_pmc_ipc.h>
> >>+
> >> #include "iTCO_vendor.h"
> >>
> >> /* Address definitions for the TCO */
> >>@@ -94,12 +96,6 @@ struct iTCO_wdt_private {
> >> 	unsigned int iTCO_version;
> >> 	struct resource *tco_res;
> >> 	struct resource *smi_res;
> >>-	/*
> >>-	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
> >>-	 * or memory-mapped PMC register bit 4 (TCO version 3).
> >>-	 */
> >
> >Better to retain this comment elsewhere.
> >
> >>-	struct resource *gcs_pmc_res;
> >>-	unsigned long __iomem *gcs_pmc;
> >> 	/* the lock for io operations */
> >> 	spinlock_t io_lock;
> >> 	/* the PCI-device */
> >>@@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
> >
> >better to have protection and error handling, discussed in v2, 2/4.
> >
> >compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
> >it impacts core WDT functionality, need to be thoroughly tested on various
> >platforms.
> >
> 
> I don't think I (or the watchdog mailing list) was copied on the original patch.
> Major immediate concern is that this introduces a dependency on external code.
> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> machines". I don't know where the function is introduced, but I hope this change
> does not require the pmc_ipc code to be present on such machines for the watchdog
> to work. It would be bad if it does. If it doesn't, it appears that the function
> should not be declared in asm/intel_pmc_ipc.h.

Yes i added you and the wdt mailing list since you were missed on original
patch. Apparently, iTCO_wdt driver always had a dependency over PMC
resources when iTCO_version >=2. The below threads should help you give some
background on this.
https://lkml.org/lkml/2017/3/16/504
https://lkml.org/lkml/2017/2/13/282

WDT driver fails to laod since the resources its requesting were reserved by
the PMC driver since they belong to PMC GCR region. WDT driver remapping those
resources is still ok but it cant reclaim them hence the problem.

> 
> Guenter
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Andy Shevchenko]

On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>> wrote:
>>>
>>> Currently, iTCO watchdog driver uses memory map to access
>>> PMC_CFG GCR register. But the entire GCR address space is
>>> already mapped in intel_scu_ipc driver. So remapping the

> I don't think I (or the watchdog mailing list) was copied on the original
> patch.
> Major immediate concern is that this introduces a dependency on external
> code.
> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> machines". I don't know where the function is introduced, but I hope this
> change
> does not require the pmc_ipc code to be present on such machines for the
> watchdog
> to work. It would be bad if it does. If it doesn't, it appears that the
> function
> should not be declared in asm/intel_pmc_ipc.h.

Agree.

I already asked once [1] to fix up the mess we have in PDx86 regarding SCU IPC.
(PMC IPC how it's called is actually just a [main] part of SCU in newer SoCs).

Rajneesh, Kuppuswamy,
please pay attention on the below.

We have two libraries doing almost the same (basics) one for old
platforms, one for new.

My vision what should be done before we go further is:
1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some library.
2. Move headers to linux/platform_data/x86 for sharing with drivers
that are supporting non-Intel / not-newest-Intel hardware.
3. Fix the mess inside the intel_pmc_ipc code (like use devm_()
helpers where it makes sense, no use of global variables, etc)

On top of that
4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details)

[1] Oops, it happened on internal mailing list Jan 27. And mentioned
publicly after in a review on some patch here.
[2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Andy Shevchenko]

On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>> wrote:
>>>
>>> Currently, iTCO watchdog driver uses memory map to access
>>> PMC_CFG GCR register. But the entire GCR address space is
>>> already mapped in intel_scu_ipc driver. So remapping the

> I don't think I (or the watchdog mailing list) was copied on the original
> patch.
> Major immediate concern is that this introduces a dependency on external
> code.
> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> machines". I don't know where the function is introduced, but I hope this
> change
> does not require the pmc_ipc code to be present on such machines for the
> watchdog
> to work. It would be bad if it does. If it doesn't, it appears that the
> function
> should not be declared in asm/intel_pmc_ipc.h.

Agree.

I already asked once [1] to fix up the mess we have in PDx86 regarding SCU IPC.
(PMC IPC how it's called is actually just a [main] part of SCU in newer SoCs).

Rajneesh, Kuppuswamy,
please pay attention on the below.

We have two libraries doing almost the same (basics) one for old
platforms, one for new.

My vision what should be done before we go further is:
1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some library.
2. Move headers to linux/platform_data/x86 for sharing with drivers
that are supporting non-Intel / not-newest-Intel hardware.
3. Fix the mess inside the intel_pmc_ipc code (like use devm_()
helpers where it makes sense, no use of global variables, etc)

On top of that
4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details)

[1] Oops, it happened on internal mailing list Jan 27. And mentioned
publicly after in a review on some patch here.
[2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html

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

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Andy Shevchenko]

On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>> wrote:
>>>
>>> Currently, iTCO watchdog driver uses memory map to access
>>> PMC_CFG GCR register. But the entire GCR address space is
>>> already mapped in intel_scu_ipc driver. So remapping the

> I don't think I (or the watchdog mailing list) was copied on the original
> patch.
> Major immediate concern is that this introduces a dependency on external
> code.
> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> machines". I don't know where the function is introduced, but I hope this
> change
> does not require the pmc_ipc code to be present on such machines for the
> watchdog
> to work. It would be bad if it does. If it doesn't, it appears that the
> function
> should not be declared in asm/intel_pmc_ipc.h.

Agree.

I already asked once [1] to fix up the mess we have in PDx86 regarding SCU IPC.
(PMC IPC how it's called is actually just a [main] part of SCU in newer SoCs).

Rajneesh, Kuppuswamy,
please pay attention on the below.

We have two libraries doing almost the same (basics) one for old
platforms, one for new.

My vision what should be done before we go further is:
1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some library.
2. Move headers to linux/platform_data/x86 for sharing with drivers
that are supporting non-Intel / not-newest-Intel hardware.
3. Fix the mess inside the intel_pmc_ipc code (like use devm_()
helpers where it makes sense, no use of global variables, etc)

On top of that
4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details)

[1] Oops, it happened on internal mailing list Jan 27. And mentioned
publicly after in a review on some patch here.
[2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

On 03/17/2017 07:25 AM, Andy Shevchenko wrote:
> On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>>> wrote:
>>>> Currently, iTCO watchdog driver uses memory map to access
>>>> PMC_CFG GCR register. But the entire GCR address space is
>>>> already mapped in intel_scu_ipc driver. So remapping the
>> I don't think I (or the watchdog mailing list) was copied on the original
>> patch.
>> Major immediate concern is that this introduces a dependency on external
>> code.
>> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
>> machines". I don't know where the function is introduced, but I hope this
>> change
>> does not require the pmc_ipc code to be present on such machines for the
>> watchdog
>> to work. It would be bad if it does. If it doesn't, it appears that the
>> function
>> should not be declared in asm/intel_pmc_ipc.h.
> Agree.
>
> I already asked once [1] to fix up the mess we have in PDx86 regarding SCU IPC.
> (PMC IPC how it's called is actually just a [main] part of SCU in newer SoCs).
>
> Rajneesh, Kuppuswamy,
> please pay attention on the below.
>
> We have two libraries doing almost the same (basics) one for old
> platforms, one for new.
>
> My vision what should be done before we go further is:
> 1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some library.
I think we should create MFD driver for PMC and remove the redundant 
resource and platform device creation codes.
Yes, there is common code in IPC implementation between scu_ipc and 
pmc_ipc code. This needs be modularized.

I can work on it and send a RFC patch for this cleanup. But it could 
take more time for merging this cleanup patch.
So I think, in the mean time, we should merge this watchdog fix first to 
remove iTCO watchdog device probe issue.
> 2. Move headers to linux/platform_data/x86 for sharing with drivers
> that are supporting non-Intel / not-newest-Intel hardware.
> 3. Fix the mess inside the intel_pmc_ipc code (like use devm_()
> helpers where it makes sense, no use of global variables, etc)
Agreed.
>
> On top of that
> 4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details)
>
> [1] Oops, it happened on internal mailing list Jan 27. And mentioned
> publicly after in a review on some patch here.
> [2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

On 03/17/2017 07:25 AM, Andy Shevchenko wrote:
> On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>>> wrote:
>>>> Currently, iTCO watchdog driver uses memory map to access
>>>> PMC_CFG GCR register. But the entire GCR address space is
>>>> already mapped in intel_scu_ipc driver. So remapping the
>> I don't think I (or the watchdog mailing list) was copied on the original
>> patch.
>> Major immediate concern is that this introduces a dependency on external
>> code.
>> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
>> machines". I don't know where the function is introduced, but I hope this
>> change
>> does not require the pmc_ipc code to be present on such machines for the
>> watchdog
>> to work. It would be bad if it does. If it doesn't, it appears that the
>> function
>> should not be declared in asm/intel_pmc_ipc.h.
> Agree.
>
> I already asked once [1] to fix up the mess we have in PDx86 regarding SCU IPC.
> (PMC IPC how it's called is actually just a [main] part of SCU in newer SoCs).
>
> Rajneesh, Kuppuswamy,
> please pay attention on the below.
>
> We have two libraries doing almost the same (basics) one for old
> platforms, one for new.
>
> My vision what should be done before we go further is:
> 1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some library.
I think we should create MFD driver for PMC and remove the redundant 
resource and platform device creation codes.
Yes, there is common code in IPC implementation between scu_ipc and 
pmc_ipc code. This needs be modularized.

I can work on it and send a RFC patch for this cleanup. But it could 
take more time for merging this cleanup patch.
So I think, in the mean time, we should merge this watchdog fix first to 
remove iTCO watchdog device probe issue.
> 2. Move headers to linux/platform_data/x86 for sharing with drivers
> that are supporting non-Intel / not-newest-Intel hardware.
> 3. Fix the mess inside the intel_pmc_ipc code (like use devm_()
> helpers where it makes sense, no use of global variables, etc)
Agreed.
>
> On top of that
> 4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details)
>
> [1] Oops, it happened on internal mailing list Jan 27. And mentioned
> publicly after in a review on some patch here.
> [2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Andy Shevchenko]

On Fri, Mar 17, 2017 at 7:37 PM, sathyanarayanan kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> On 03/17/2017 07:25 AM, Andy Shevchenko wrote:
>> On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>>>> wrote:

>> I already asked once [1] to fix up the mess we have in PDx86 regarding SCU
>> IPC.
>> (PMC IPC how it's called is actually just a [main] part of SCU in newer
>> SoCs).
>>
>> Rajneesh, Kuppuswamy,
>> please pay attention on the below.
>>
>> We have two libraries doing almost the same (basics) one for old
>> platforms, one for new.
>>
>> My vision what should be done before we go further is:
>> 1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some
>> library.
>
> I think we should create MFD driver for PMC and remove the redundant
> resource and platform device creation codes.
> Yes, there is common code in IPC implementation between scu_ipc and pmc_ipc
> code. This needs be modularized.
>
> I can work on it and send a RFC patch for this cleanup. But it could take
> more time for merging this cleanup patch.
> So I think, in the mean time, we should merge this watchdog fix first to
> remove iTCO watchdog device probe issue.

I have heard already such excuses. Let's consider this as a "Last
Chinese Warning".

So, we consider reviewing applying *already floating around* patches
in exchange to looking forward for clean up next.
Do we have a deal?

Before you are going to implement anything in the code, please, share
a document (architectural point of view) how you would see things
should be done.
Also consider to address PMC (Atom drivers) and P-Unit drivers which
are related to SCU / IPC to have some structure.

>> 2. Move headers to linux/platform_data/x86 for sharing with drivers
>> that are supporting non-Intel / not-newest-Intel hardware.
>> 3. Fix the mess inside the intel_pmc_ipc code (like use devm_()
>> helpers where it makes sense, no use of global variables, etc)
>
> Agreed.
>>
>>
>> On top of that
>> 4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details)
>>
>> [1] Oops, it happened on internal mailing list Jan 27. And mentioned
>> publicly after in a review on some patch here.
>> [2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Andy Shevchenko]

On Fri, Mar 17, 2017 at 7:37 PM, sathyanarayanan kuppuswamy
<sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> On 03/17/2017 07:25 AM, Andy Shevchenko wrote:
>> On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>>>> wrote:

>> I already asked once [1] to fix up the mess we have in PDx86 regarding SCU
>> IPC.
>> (PMC IPC how it's called is actually just a [main] part of SCU in newer
>> SoCs).
>>
>> Rajneesh, Kuppuswamy,
>> please pay attention on the below.
>>
>> We have two libraries doing almost the same (basics) one for old
>> platforms, one for new.
>>
>> My vision what should be done before we go further is:
>> 1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some
>> library.
>
> I think we should create MFD driver for PMC and remove the redundant
> resource and platform device creation codes.
> Yes, there is common code in IPC implementation between scu_ipc and pmc_ipc
> code. This needs be modularized.
>
> I can work on it and send a RFC patch for this cleanup. But it could take
> more time for merging this cleanup patch.
> So I think, in the mean time, we should merge this watchdog fix first to
> remove iTCO watchdog device probe issue.

I have heard already such excuses. Let's consider this as a "Last
Chinese Warning".

So, we consider reviewing applying *already floating around* patches
in exchange to looking forward for clean up next.
Do we have a deal?

Before you are going to implement anything in the code, please, share
a document (architectural point of view) how you would see things
should be done.
Also consider to address PMC (Atom drivers) and P-Unit drivers which
are related to SCU / IPC to have some structure.

>> 2. Move headers to linux/platform_data/x86 for sharing with drivers
>> that are supporting non-Intel / not-newest-Intel hardware.
>> 3. Fix the mess inside the intel_pmc_ipc code (like use devm_()
>> helpers where it makes sense, no use of global variables, etc)
>
> Agreed.
>>
>>
>> On top of that
>> 4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details)
>>
>> [1] Oops, it happened on internal mailing list Jan 27. And mentioned
>> publicly after in a review on some patch here.
>> [2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html

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

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Andy Shevchenko]

On Fri, Mar 17, 2017 at 7:37 PM, sathyanarayanan kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> On 03/17/2017 07:25 AM, Andy Shevchenko wrote:
>> On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>>>> wrote:

>> I already asked once [1] to fix up the mess we have in PDx86 regarding SCU
>> IPC.
>> (PMC IPC how it's called is actually just a [main] part of SCU in newer
>> SoCs).
>>
>> Rajneesh, Kuppuswamy,
>> please pay attention on the below.
>>
>> We have two libraries doing almost the same (basics) one for old
>> platforms, one for new.
>>
>> My vision what should be done before we go further is:
>> 1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some
>> library.
>
> I think we should create MFD driver for PMC and remove the redundant
> resource and platform device creation codes.
> Yes, there is common code in IPC implementation between scu_ipc and pmc_ipc
> code. This needs be modularized.
>
> I can work on it and send a RFC patch for this cleanup. But it could take
> more time for merging this cleanup patch.
> So I think, in the mean time, we should merge this watchdog fix first to
> remove iTCO watchdog device probe issue.

I have heard already such excuses. Let's consider this as a "Last
Chinese Warning".

So, we consider reviewing applying *already floating around* patches
in exchange to looking forward for clean up next.
Do we have a deal?

Before you are going to implement anything in the code, please, share
a document (architectural point of view) how you would see things
should be done.
Also consider to address PMC (Atom drivers) and P-Unit drivers which
are related to SCU / IPC to have some structure.

>> 2. Move headers to linux/platform_data/x86 for sharing with drivers
>> that are supporting non-Intel / not-newest-Intel hardware.
>> 3. Fix the mess inside the intel_pmc_ipc code (like use devm_()
>> helpers where it makes sense, no use of global variables, etc)
>
> Agreed.
>>
>>
>> On top of that
>> 4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details)
>>
>> [1] Oops, it happened on internal mailing list Jan 27. And mentioned
>> publicly after in a review on some patch here.
>> [2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

Hi Andy,


On 03/17/2017 11:38 AM, Andy Shevchenko wrote:
> On Fri, Mar 17, 2017 at 7:37 PM, sathyanarayanan kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> On 03/17/2017 07:25 AM, Andy Shevchenko wrote:
>>> On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>>>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>>>>> wrote:
>>> I already asked once [1] to fix up the mess we have in PDx86 regarding SCU
>>> IPC.
>>> (PMC IPC how it's called is actually just a [main] part of SCU in newer
>>> SoCs).
>>>
>>> Rajneesh, Kuppuswamy,
>>> please pay attention on the below.
>>>
>>> We have two libraries doing almost the same (basics) one for old
>>> platforms, one for new.
>>>
>>> My vision what should be done before we go further is:
>>> 1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some
>>> library.
>> I think we should create MFD driver for PMC and remove the redundant
>> resource and platform device creation codes.
>> Yes, there is common code in IPC implementation between scu_ipc and pmc_ipc
>> code. This needs be modularized.
>>
>> I can work on it and send a RFC patch for this cleanup. But it could take
>> more time for merging this cleanup patch.
>> So I think, in the mean time, we should merge this watchdog fix first to
>> remove iTCO watchdog device probe issue.
> I have heard already such excuses. Let's consider this as a "Last
> Chinese Warning".
>
> So, we consider reviewing applying *already floating around* patches
> in exchange to looking forward for clean up next.
> Do we have a deal?
Deal.
>
> Before you are going to implement anything in the code, please, share
> a document (architectural point of view) how you would see things
> should be done.
> Also consider to address PMC (Atom drivers) and P-Unit drivers which
> are related to SCU / IPC to have some structure.
Will send out the design document summarizing the issues we want to 
solve and a proposed design model.
>
>>> 2. Move headers to linux/platform_data/x86 for sharing with drivers
>>> that are supporting non-Intel / not-newest-Intel hardware.
>>> 3. Fix the mess inside the intel_pmc_ipc code (like use devm_()
>>> helpers where it makes sense, no use of global variables, etc)
>> Agreed.
>>>
>>> On top of that
>>> 4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details)
>>>
>>> [1] Oops, it happened on internal mailing list Jan 27. And mentioned
>>> publicly after in a review on some patch here.
>>> [2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

Hi Andy,


On 03/17/2017 11:38 AM, Andy Shevchenko wrote:
> On Fri, Mar 17, 2017 at 7:37 PM, sathyanarayanan kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> On 03/17/2017 07:25 AM, Andy Shevchenko wrote:
>>> On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>>>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>>>>> wrote:
>>> I already asked once [1] to fix up the mess we have in PDx86 regarding SCU
>>> IPC.
>>> (PMC IPC how it's called is actually just a [main] part of SCU in newer
>>> SoCs).
>>>
>>> Rajneesh, Kuppuswamy,
>>> please pay attention on the below.
>>>
>>> We have two libraries doing almost the same (basics) one for old
>>> platforms, one for new.
>>>
>>> My vision what should be done before we go further is:
>>> 1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some
>>> library.
>> I think we should create MFD driver for PMC and remove the redundant
>> resource and platform device creation codes.
>> Yes, there is common code in IPC implementation between scu_ipc and pmc_ipc
>> code. This needs be modularized.
>>
>> I can work on it and send a RFC patch for this cleanup. But it could take
>> more time for merging this cleanup patch.
>> So I think, in the mean time, we should merge this watchdog fix first to
>> remove iTCO watchdog device probe issue.
> I have heard already such excuses. Let's consider this as a "Last
> Chinese Warning".
>
> So, we consider reviewing applying *already floating around* patches
> in exchange to looking forward for clean up next.
> Do we have a deal?
Deal.
>
> Before you are going to implement anything in the code, please, share
> a document (architectural point of view) how you would see things
> should be done.
> Also consider to address PMC (Atom drivers) and P-Unit drivers which
> are related to SCU / IPC to have some structure.
Will send out the design document summarizing the issues we want to 
solve and a proposed design model.
>
>>> 2. Move headers to linux/platform_data/x86 for sharing with drivers
>>> that are supporting non-Intel / not-newest-Intel hardware.
>>> 3. Fix the mess inside the intel_pmc_ipc code (like use devm_()
>>> helpers where it makes sense, no use of global variables, etc)
>> Agreed.
>>>
>>> On top of that
>>> 4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details)
>>>
>>> [1] Oops, it happened on internal mailing list Jan 27. And mentioned
>>> publicly after in a review on some patch here.
>>> [2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

On 03/17/2017 06:40 AM, Guenter Roeck wrote:
> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan 
>> wrote:
>>> Currently, iTCO watchdog driver uses memory map to access
>>> PMC_CFG GCR register. But the entire GCR address space is
>>> already mapped in intel_scu_ipc driver. So remapping the
>>
>> intel_pmc_ipc driver.
>>
>>> GCR register in this driver causes the mem request failure in
>>> iTCO_wdt probe function. This patch fixes this issue by
>>> using PMC GCR read/write API's to access PMC_CFG register.
>>>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan 
>>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> ---
>>>  drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>>  1 file changed, 7 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>>> index 3d0abc0..31abfc5 100644
>>> --- a/drivers/watchdog/iTCO_wdt.c
>>> +++ b/drivers/watchdog/iTCO_wdt.c
>>> @@ -68,6 +68,8 @@
>>>  #include <linux/io.h>            /* For inb/outb/... */
>>>  #include <linux/platform_data/itco_wdt.h>
>>>
>>> +#include <asm/intel_pmc_ipc.h>
>>> +
>>>  #include "iTCO_vendor.h"
>>>
>>>  /* Address definitions for the TCO */
>>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>>      unsigned int iTCO_version;
>>>      struct resource *tco_res;
>>>      struct resource *smi_res;
>>> -    /*
>>> -     * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO 
>>> version 2),
>>> -     * or memory-mapped PMC register bit 4 (TCO version 3).
>>> -     */
>>
>> Better to retain this comment elsewhere.
>>
>>> -    struct resource *gcs_pmc_res;
>>> -    unsigned long __iomem *gcs_pmc;
>>>      /* the lock for io operations */
>>>      spinlock_t io_lock;
>>>      /* the PCI-device */
>>> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
>>
>> better to have protection and error handling, discussed in v2, 2/4.
>>
>> compiled and tested this on APL and i see iTCO_WDT driver loads fine. 
>> Since
>> it impacts core WDT functionality, need to be thoroughly tested on 
>> various
>> platforms.
>>
>
> I don't think I (or the watchdog mailing list) was copied on the 
> original patch.
Sorry. Its my mistake. I will fix it in next series update.
> Major immediate concern is that this introduces a dependency on 
> external code.
> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> machines". I don't know where the function is introduced, but I hope 
> this change
> does not require the pmc_ipc code to be present on such machines for 
> the watchdog
> to work. It would be bad if it does. If it doesn't, it appears that 
> the function
> should not be declared in asm/intel_pmc_ipc.h.
It should not create any compile time dependency with INTEL_PMC_IPC 
config option. If INTEL_PMC_IPC_CONFIG is disabled, we use
empty definitions for these calls defined in asm/intel_pmc_ipc.h

But iTCO_wdt driver already has runtime dependency with INTEL_PMIC_IPC 
if its version iTCO_version >= 2.

>
> Guenter
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

On 03/17/2017 06:40 AM, Guenter Roeck wrote:
> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan 
>> wrote:
>>> Currently, iTCO watchdog driver uses memory map to access
>>> PMC_CFG GCR register. But the entire GCR address space is
>>> already mapped in intel_scu_ipc driver. So remapping the
>>
>> intel_pmc_ipc driver.
>>
>>> GCR register in this driver causes the mem request failure in
>>> iTCO_wdt probe function. This patch fixes this issue by
>>> using PMC GCR read/write API's to access PMC_CFG register.
>>>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan 
>>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> ---
>>>  drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>>  1 file changed, 7 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>>> index 3d0abc0..31abfc5 100644
>>> --- a/drivers/watchdog/iTCO_wdt.c
>>> +++ b/drivers/watchdog/iTCO_wdt.c
>>> @@ -68,6 +68,8 @@
>>>  #include <linux/io.h>            /* For inb/outb/... */
>>>  #include <linux/platform_data/itco_wdt.h>
>>>
>>> +#include <asm/intel_pmc_ipc.h>
>>> +
>>>  #include "iTCO_vendor.h"
>>>
>>>  /* Address definitions for the TCO */
>>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>>      unsigned int iTCO_version;
>>>      struct resource *tco_res;
>>>      struct resource *smi_res;
>>> -    /*
>>> -     * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO 
>>> version 2),
>>> -     * or memory-mapped PMC register bit 4 (TCO version 3).
>>> -     */
>>
>> Better to retain this comment elsewhere.
>>
>>> -    struct resource *gcs_pmc_res;
>>> -    unsigned long __iomem *gcs_pmc;
>>>      /* the lock for io operations */
>>>      spinlock_t io_lock;
>>>      /* the PCI-device */
>>> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
>>
>> better to have protection and error handling, discussed in v2, 2/4.
>>
>> compiled and tested this on APL and i see iTCO_WDT driver loads fine. 
>> Since
>> it impacts core WDT functionality, need to be thoroughly tested on 
>> various
>> platforms.
>>
>
> I don't think I (or the watchdog mailing list) was copied on the 
> original patch.
Sorry. Its my mistake. I will fix it in next series update.
> Major immediate concern is that this introduces a dependency on 
> external code.
> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> machines". I don't know where the function is introduced, but I hope 
> this change
> does not require the pmc_ipc code to be present on such machines for 
> the watchdog
> to work. It would be bad if it does. If it doesn't, it appears that 
> the function
> should not be declared in asm/intel_pmc_ipc.h.
It should not create any compile time dependency with INTEL_PMC_IPC 
config option. If INTEL_PMC_IPC_CONFIG is disabled, we use
empty definitions for these calls defined in asm/intel_pmc_ipc.h

But iTCO_wdt driver already has runtime dependency with INTEL_PMIC_IPC 
if its version iTCO_version >= 2.

>
> Guenter
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Guenter Roeck]

On Fri, Mar 17, 2017 at 10:24:35AM -0700, sathyanarayanan kuppuswamy wrote:
> 
> 
> On 03/17/2017 06:40 AM, Guenter Roeck wrote:
> >On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> >>On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
> >>wrote:
> >>>Currently, iTCO watchdog driver uses memory map to access
> >>>PMC_CFG GCR register. But the entire GCR address space is
> >>>already mapped in intel_scu_ipc driver. So remapping the
> >>
> >>intel_pmc_ipc driver.
> >>
> >>>GCR register in this driver causes the mem request failure in
> >>>iTCO_wdt probe function. This patch fixes this issue by
> >>>using PMC GCR read/write API's to access PMC_CFG register.
> >>>
> >>>Signed-off-by: Kuppuswamy Sathyanarayanan
> >>><sathyanarayanan.kuppuswamy@linux.intel.com>
> >>>---
> >>> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
> >>> 1 file changed, 7 insertions(+), 24 deletions(-)
> >>>
> >>>diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> >>>index 3d0abc0..31abfc5 100644
> >>>--- a/drivers/watchdog/iTCO_wdt.c
> >>>+++ b/drivers/watchdog/iTCO_wdt.c
> >>>@@ -68,6 +68,8 @@
> >>> #include <linux/io.h>            /* For inb/outb/... */
> >>> #include <linux/platform_data/itco_wdt.h>
> >>>
> >>>+#include <asm/intel_pmc_ipc.h>
> >>>+
> >>> #include "iTCO_vendor.h"
> >>>
> >>> /* Address definitions for the TCO */
> >>>@@ -94,12 +96,6 @@ struct iTCO_wdt_private {
> >>>     unsigned int iTCO_version;
> >>>     struct resource *tco_res;
> >>>     struct resource *smi_res;
> >>>-    /*
> >>>-     * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO
> >>>version 2),
> >>>-     * or memory-mapped PMC register bit 4 (TCO version 3).
> >>>-     */
> >>
> >>Better to retain this comment elsewhere.
> >>
> >>>-    struct resource *gcs_pmc_res;
> >>>-    unsigned long __iomem *gcs_pmc;
> >>>     /* the lock for io operations */
> >>>     spinlock_t io_lock;
> >>>     /* the PCI-device */
> >>>@@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
> >>
> >>better to have protection and error handling, discussed in v2, 2/4.
> >>
> >>compiled and tested this on APL and i see iTCO_WDT driver loads fine.
> >>Since
> >>it impacts core WDT functionality, need to be thoroughly tested on
> >>various
> >>platforms.
> >>
> >
> >I don't think I (or the watchdog mailing list) was copied on the original
> >patch.
> Sorry. Its my mistake. I will fix it in next series update.
> >Major immediate concern is that this introduces a dependency on external
> >code.
> >The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> >machines". I don't know where the function is introduced, but I hope this
> >change
> >does not require the pmc_ipc code to be present on such machines for the
> >watchdog
> >to work. It would be bad if it does. If it doesn't, it appears that the
> >function
> >should not be declared in asm/intel_pmc_ipc.h.
> It should not create any compile time dependency with INTEL_PMC_IPC config
> option. If INTEL_PMC_IPC_CONFIG is disabled, we use
> empty definitions for these calls defined in asm/intel_pmc_ipc.h
> 

So the watchdog driver would get an error if CONFIG_INTEL_PMC_IPC
is not defined ? And that is supposed to be acceptable ?

> But iTCO_wdt driver already has runtime dependency with INTEL_PMIC_IPC if
> its version iTCO_version >= 2.
> 
Unless I am missing something, there is no explicit dependency. AFAICS
the watchdog driver works just fine if INTEL_PMIC_IPC is not enabled,
and/or if it is built as module and the module is not loaded.

Maybe you mean that the watchdog driver doesn't load if the INTEL_PMIC_IPC
driver is loaded. That would be a bug, not a dependency.

Guenter

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[Guenter Roeck]

On Fri, Mar 17, 2017 at 10:24:35AM -0700, sathyanarayanan kuppuswamy wrote:
> 
> 
> On 03/17/2017 06:40 AM, Guenter Roeck wrote:
> >On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> >>On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
> >>wrote:
> >>>Currently, iTCO watchdog driver uses memory map to access
> >>>PMC_CFG GCR register. But the entire GCR address space is
> >>>already mapped in intel_scu_ipc driver. So remapping the
> >>
> >>intel_pmc_ipc driver.
> >>
> >>>GCR register in this driver causes the mem request failure in
> >>>iTCO_wdt probe function. This patch fixes this issue by
> >>>using PMC GCR read/write API's to access PMC_CFG register.
> >>>
> >>>Signed-off-by: Kuppuswamy Sathyanarayanan
> >>><sathyanarayanan.kuppuswamy@linux.intel.com>
> >>>---
> >>> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
> >>> 1 file changed, 7 insertions(+), 24 deletions(-)
> >>>
> >>>diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> >>>index 3d0abc0..31abfc5 100644
> >>>--- a/drivers/watchdog/iTCO_wdt.c
> >>>+++ b/drivers/watchdog/iTCO_wdt.c
> >>>@@ -68,6 +68,8 @@
> >>> #include <linux/io.h>            /* For inb/outb/... */
> >>> #include <linux/platform_data/itco_wdt.h>
> >>>
> >>>+#include <asm/intel_pmc_ipc.h>
> >>>+
> >>> #include "iTCO_vendor.h"
> >>>
> >>> /* Address definitions for the TCO */
> >>>@@ -94,12 +96,6 @@ struct iTCO_wdt_private {
> >>>     unsigned int iTCO_version;
> >>>     struct resource *tco_res;
> >>>     struct resource *smi_res;
> >>>-    /*
> >>>-     * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO
> >>>version 2),
> >>>-     * or memory-mapped PMC register bit 4 (TCO version 3).
> >>>-     */
> >>
> >>Better to retain this comment elsewhere.
> >>
> >>>-    struct resource *gcs_pmc_res;
> >>>-    unsigned long __iomem *gcs_pmc;
> >>>     /* the lock for io operations */
> >>>     spinlock_t io_lock;
> >>>     /* the PCI-device */
> >>>@@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
> >>
> >>better to have protection and error handling, discussed in v2, 2/4.
> >>
> >>compiled and tested this on APL and i see iTCO_WDT driver loads fine.
> >>Since
> >>it impacts core WDT functionality, need to be thoroughly tested on
> >>various
> >>platforms.
> >>
> >
> >I don't think I (or the watchdog mailing list) was copied on the original
> >patch.
> Sorry. Its my mistake. I will fix it in next series update.
> >Major immediate concern is that this introduces a dependency on external
> >code.
> >The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> >machines". I don't know where the function is introduced, but I hope this
> >change
> >does not require the pmc_ipc code to be present on such machines for the
> >watchdog
> >to work. It would be bad if it does. If it doesn't, it appears that the
> >function
> >should not be declared in asm/intel_pmc_ipc.h.
> It should not create any compile time dependency with INTEL_PMC_IPC config
> option. If INTEL_PMC_IPC_CONFIG is disabled, we use
> empty definitions for these calls defined in asm/intel_pmc_ipc.h
> 

So the watchdog driver would get an error if CONFIG_INTEL_PMC_IPC
is not defined ? And that is supposed to be acceptable ?

> But iTCO_wdt driver already has runtime dependency with INTEL_PMIC_IPC if
> its version iTCO_version >= 2.
> 
Unless I am missing something, there is no explicit dependency. AFAICS
the watchdog driver works just fine if INTEL_PMIC_IPC is not enabled,
and/or if it is built as module and the module is not loaded.

Maybe you mean that the watchdog driver doesn't load if the INTEL_PMIC_IPC
driver is loaded. That would be a bug, not a dependency.

Guenter

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

On 03/17/2017 10:50 AM, Guenter Roeck wrote:
> On Fri, Mar 17, 2017 at 10:24:35AM -0700, sathyanarayanan kuppuswamy wrote:
>>
>> On 03/17/2017 06:40 AM, Guenter Roeck wrote:
>>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>>>> wrote:
>>>>> Currently, iTCO watchdog driver uses memory map to access
>>>>> PMC_CFG GCR register. But the entire GCR address space is
>>>>> already mapped in intel_scu_ipc driver. So remapping the
>>>> intel_pmc_ipc driver.
>>>>
>>>>> GCR register in this driver causes the mem request failure in
>>>>> iTCO_wdt probe function. This patch fixes this issue by
>>>>> using PMC GCR read/write API's to access PMC_CFG register.
>>>>>
>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>>>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>> ---
>>>>> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>>>> 1 file changed, 7 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>>>>> index 3d0abc0..31abfc5 100644
>>>>> --- a/drivers/watchdog/iTCO_wdt.c
>>>>> +++ b/drivers/watchdog/iTCO_wdt.c
>>>>> @@ -68,6 +68,8 @@
>>>>> #include <linux/io.h>            /* For inb/outb/... */
>>>>> #include <linux/platform_data/itco_wdt.h>
>>>>>
>>>>> +#include <asm/intel_pmc_ipc.h>
>>>>> +
>>>>> #include "iTCO_vendor.h"
>>>>>
>>>>> /* Address definitions for the TCO */
>>>>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>>>>      unsigned int iTCO_version;
>>>>>      struct resource *tco_res;
>>>>>      struct resource *smi_res;
>>>>> -    /*
>>>>> -     * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO
>>>>> version 2),
>>>>> -     * or memory-mapped PMC register bit 4 (TCO version 3).
>>>>> -     */
>>>> Better to retain this comment elsewhere.
>>>>
>>>>> -    struct resource *gcs_pmc_res;
>>>>> -    unsigned long __iomem *gcs_pmc;
>>>>>      /* the lock for io operations */
>>>>>      spinlock_t io_lock;
>>>>>      /* the PCI-device */
>>>>> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
>>>> better to have protection and error handling, discussed in v2, 2/4.
>>>>
>>>> compiled and tested this on APL and i see iTCO_WDT driver loads fine.
>>>> Since
>>>> it impacts core WDT functionality, need to be thoroughly tested on
>>>> various
>>>> platforms.
>>>>
>>> I don't think I (or the watchdog mailing list) was copied on the original
>>> patch.
>> Sorry. Its my mistake. I will fix it in next series update.
>>> Major immediate concern is that this introduces a dependency on external
>>> code.
>>> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
>>> machines". I don't know where the function is introduced, but I hope this
>>> change
>>> does not require the pmc_ipc code to be present on such machines for the
>>> watchdog
>>> to work. It would be bad if it does. If it doesn't, it appears that the
>>> function
>>> should not be declared in asm/intel_pmc_ipc.h.
>> It should not create any compile time dependency with INTEL_PMC_IPC config
>> option. If INTEL_PMC_IPC_CONFIG is disabled, we use
>> empty definitions for these calls defined in asm/intel_pmc_ipc.h
>>
> So the watchdog driver would get an error if CONFIG_INTEL_PMC_IPC
> is not defined ? And that is supposed to be acceptable ?
Sorry, It looks like I missed your point in your previous email.  I 
thought that gcs_pmc mem resource will be used only if watchdog device 
is enumerated by intel_pmc_ipc.c

After reviewing the code again, I found out this iTCO_wdt driver can 
also be enumerated by drivers/mfd/lpc_ich.c and 
drivers/i2c/busses/i2c-i801.c. Both these drivers pass the GCS as memory 
resource. So using intel_pmc_ipc specific calls will break watchdog 
functionality if its enumerated by any device other than intel_pmic_ipc.c

May be I should add a flag for ipc case in itco_wdt_platform_data and 
handle this as a special case.
>
>> But iTCO_wdt driver already has runtime dependency with INTEL_PMIC_IPC if
>> its version iTCO_version >= 2.
>>
> Unless I am missing something, there is no explicit dependency. AFAICS
> the watchdog driver works just fine if INTEL_PMIC_IPC is not enabled,
> and/or if it is built as module and the module is not loaded.
>
> Maybe you mean that the watchdog driver doesn't load if the INTEL_PMIC_IPC
> driver is loaded. That would be a bug, not a dependency.
>
> Guenter
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

On 03/17/2017 10:50 AM, Guenter Roeck wrote:
> On Fri, Mar 17, 2017 at 10:24:35AM -0700, sathyanarayanan kuppuswamy wrote:
>>
>> On 03/17/2017 06:40 AM, Guenter Roeck wrote:
>>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>>>> wrote:
>>>>> Currently, iTCO watchdog driver uses memory map to access
>>>>> PMC_CFG GCR register. But the entire GCR address space is
>>>>> already mapped in intel_scu_ipc driver. So remapping the
>>>> intel_pmc_ipc driver.
>>>>
>>>>> GCR register in this driver causes the mem request failure in
>>>>> iTCO_wdt probe function. This patch fixes this issue by
>>>>> using PMC GCR read/write API's to access PMC_CFG register.
>>>>>
>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>>>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>> ---
>>>>> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>>>> 1 file changed, 7 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>>>>> index 3d0abc0..31abfc5 100644
>>>>> --- a/drivers/watchdog/iTCO_wdt.c
>>>>> +++ b/drivers/watchdog/iTCO_wdt.c
>>>>> @@ -68,6 +68,8 @@
>>>>> #include <linux/io.h>            /* For inb/outb/... */
>>>>> #include <linux/platform_data/itco_wdt.h>
>>>>>
>>>>> +#include <asm/intel_pmc_ipc.h>
>>>>> +
>>>>> #include "iTCO_vendor.h"
>>>>>
>>>>> /* Address definitions for the TCO */
>>>>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>>>>      unsigned int iTCO_version;
>>>>>      struct resource *tco_res;
>>>>>      struct resource *smi_res;
>>>>> -    /*
>>>>> -     * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO
>>>>> version 2),
>>>>> -     * or memory-mapped PMC register bit 4 (TCO version 3).
>>>>> -     */
>>>> Better to retain this comment elsewhere.
>>>>
>>>>> -    struct resource *gcs_pmc_res;
>>>>> -    unsigned long __iomem *gcs_pmc;
>>>>>      /* the lock for io operations */
>>>>>      spinlock_t io_lock;
>>>>>      /* the PCI-device */
>>>>> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
>>>> better to have protection and error handling, discussed in v2, 2/4.
>>>>
>>>> compiled and tested this on APL and i see iTCO_WDT driver loads fine.
>>>> Since
>>>> it impacts core WDT functionality, need to be thoroughly tested on
>>>> various
>>>> platforms.
>>>>
>>> I don't think I (or the watchdog mailing list) was copied on the original
>>> patch.
>> Sorry. Its my mistake. I will fix it in next series update.
>>> Major immediate concern is that this introduces a dependency on external
>>> code.
>>> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
>>> machines". I don't know where the function is introduced, but I hope this
>>> change
>>> does not require the pmc_ipc code to be present on such machines for the
>>> watchdog
>>> to work. It would be bad if it does. If it doesn't, it appears that the
>>> function
>>> should not be declared in asm/intel_pmc_ipc.h.
>> It should not create any compile time dependency with INTEL_PMC_IPC config
>> option. If INTEL_PMC_IPC_CONFIG is disabled, we use
>> empty definitions for these calls defined in asm/intel_pmc_ipc.h
>>
> So the watchdog driver would get an error if CONFIG_INTEL_PMC_IPC
> is not defined ? And that is supposed to be acceptable ?
Sorry, It looks like I missed your point in your previous email.  I 
thought that gcs_pmc mem resource will be used only if watchdog device 
is enumerated by intel_pmc_ipc.c

After reviewing the code again, I found out this iTCO_wdt driver can 
also be enumerated by drivers/mfd/lpc_ich.c and 
drivers/i2c/busses/i2c-i801.c. Both these drivers pass the GCS as memory 
resource. So using intel_pmc_ipc specific calls will break watchdog 
functionality if its enumerated by any device other than intel_pmic_ipc.c

May be I should add a flag for ipc case in itco_wdt_platform_data and 
handle this as a special case.
>
>> But iTCO_wdt driver already has runtime dependency with INTEL_PMIC_IPC if
>> its version iTCO_version >= 2.
>>
> Unless I am missing something, there is no explicit dependency. AFAICS
> the watchdog driver works just fine if INTEL_PMIC_IPC is not enabled,
> and/or if it is built as module and the module is not loaded.
>
> Maybe you mean that the watchdog driver doesn't load if the INTEL_PMIC_IPC
> driver is loaded. That would be a bug, not a dependency.
>
> Guenter
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

Hi Rajneesh,


On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Currently, iTCO watchdog driver uses memory map to access
>> PMC_CFG GCR register. But the entire GCR address space is
>> already mapped in intel_scu_ipc driver. So remapping the
> intel_pmc_ipc driver.
Will fix it in next series.
>
>> GCR register in this driver causes the mem request failure in
>> iTCO_wdt probe function. This patch fixes this issue by
>> using PMC GCR read/write API's to access PMC_CFG register.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>   1 file changed, 7 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>> index 3d0abc0..31abfc5 100644
>> --- a/drivers/watchdog/iTCO_wdt.c
>> +++ b/drivers/watchdog/iTCO_wdt.c
>> @@ -68,6 +68,8 @@
>>   #include <linux/io.h>			/* For inb/outb/... */
>>   #include <linux/platform_data/itco_wdt.h>
>>   
>> +#include <asm/intel_pmc_ipc.h>
>> +
>>   #include "iTCO_vendor.h"
>>   
>>   /* Address definitions for the TCO */
>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>   	unsigned int iTCO_version;
>>   	struct resource *tco_res;
>>   	struct resource *smi_res;
>> -	/*
>> -	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
>> -	 * or memory-mapped PMC register bit 4 (TCO version 3).
>> -	 */
> Better to retain this comment elsewhere.
no_reboot_bit() function might be better place for this comment.
>
>> -	struct resource *gcs_pmc_res;
>> -	unsigned long __iomem *gcs_pmc;
>>   	/* the lock for io operations */
>>   	spinlock_t io_lock;
>>   	/* the PCI-device */
>> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
> better to have protection and error handling, discussed in v2, 2/4.
>
> compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
> it impacts core WDT functionality, need to be thoroughly tested on various
> platforms.
I have tested it in Joule and it works fine. it would be nice if some 
one can verify it in non-atom platforms.
>
>> -- 
>> 2.7.4
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

Hi Rajneesh,


On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Currently, iTCO watchdog driver uses memory map to access
>> PMC_CFG GCR register. But the entire GCR address space is
>> already mapped in intel_scu_ipc driver. So remapping the
> intel_pmc_ipc driver.
Will fix it in next series.
>
>> GCR register in this driver causes the mem request failure in
>> iTCO_wdt probe function. This patch fixes this issue by
>> using PMC GCR read/write API's to access PMC_CFG register.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>   drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>   1 file changed, 7 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>> index 3d0abc0..31abfc5 100644
>> --- a/drivers/watchdog/iTCO_wdt.c
>> +++ b/drivers/watchdog/iTCO_wdt.c
>> @@ -68,6 +68,8 @@
>>   #include <linux/io.h>			/* For inb/outb/... */
>>   #include <linux/platform_data/itco_wdt.h>
>>   
>> +#include <asm/intel_pmc_ipc.h>
>> +
>>   #include "iTCO_vendor.h"
>>   
>>   /* Address definitions for the TCO */
>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>   	unsigned int iTCO_version;
>>   	struct resource *tco_res;
>>   	struct resource *smi_res;
>> -	/*
>> -	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
>> -	 * or memory-mapped PMC register bit 4 (TCO version 3).
>> -	 */
> Better to retain this comment elsewhere.
no_reboot_bit() function might be better place for this comment.
>
>> -	struct resource *gcs_pmc_res;
>> -	unsigned long __iomem *gcs_pmc;
>>   	/* the lock for io operations */
>>   	spinlock_t io_lock;
>>   	/* the PCI-device */
>> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
> better to have protection and error handling, discussed in v2, 2/4.
>
> compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
> it impacts core WDT functionality, need to be thoroughly tested on various
> platforms.
I have tested it in Joule and it works fine. it would be nice if some 
one can verify it in non-atom platforms.
>
>> -- 
>> 2.7.4
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

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

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[sathyanarayanan kuppuswamy]

Hi Rajneesh,


On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Currently, iTCO watchdog driver uses memory map to access
>> PMC_CFG GCR register. But the entire GCR address space is
>> already mapped in intel_scu_ipc driver. So remapping the
> intel_pmc_ipc driver.
Will fix it in next series.
>
>> GCR register in this driver causes the mem request failure in
>> iTCO_wdt probe function. This patch fixes this issue by
>> using PMC GCR read/write API's to access PMC_CFG register.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>   1 file changed, 7 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>> index 3d0abc0..31abfc5 100644
>> --- a/drivers/watchdog/iTCO_wdt.c
>> +++ b/drivers/watchdog/iTCO_wdt.c
>> @@ -68,6 +68,8 @@
>>   #include <linux/io.h>			/* For inb/outb/... */
>>   #include <linux/platform_data/itco_wdt.h>
>>   
>> +#include <asm/intel_pmc_ipc.h>
>> +
>>   #include "iTCO_vendor.h"
>>   
>>   /* Address definitions for the TCO */
>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>   	unsigned int iTCO_version;
>>   	struct resource *tco_res;
>>   	struct resource *smi_res;
>> -	/*
>> -	 * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
>> -	 * or memory-mapped PMC register bit 4 (TCO version 3).
>> -	 */
> Better to retain this comment elsewhere.
no_reboot_bit() function might be better place for this comment.
>
>> -	struct resource *gcs_pmc_res;
>> -	unsigned long __iomem *gcs_pmc;
>>   	/* the lock for io operations */
>>   	spinlock_t io_lock;
>>   	/* the PCI-device */
>> @@ -176,9 +172,9 @@ 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 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
> better to have protection and error handling, discussed in v2, 2/4.
>
> compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
> it impacts core WDT functionality, need to be thoroughly tested on various
> platforms.
I have tested it in Joule and it works fine. it would be nice if some 
one can verify it in non-atom platforms.
>
>> -- 
>> 2.7.4
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

Hi Kuppuswamy,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc3 next-20170310]
[cannot apply to platform-drivers-x86/for-next tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kuppuswamy-Sathyanarayanan/platform-x86-intel_pmc_ipc-fix-gcr-offset/20170320-032638
config: x86_64-randconfig-h0-03200816 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `iTCO_wdt_unset_NO_REBOOT_bit':
>> iTCO_wdt.c:(.text+0x3b6f0f): undefined reference to `intel_pmc_gcr_read'
>> iTCO_wdt.c:(.text+0x3b6f24): undefined reference to `intel_pmc_gcr_write'
   iTCO_wdt.c:(.text+0x3b6f2e): undefined reference to `intel_pmc_gcr_read'
   drivers/built-in.o: In function `iTCO_wdt_set_NO_REBOOT_bit':
   iTCO_wdt.c:(.text+0x3b729e): undefined reference to `intel_pmc_gcr_read'
   iTCO_wdt.c:(.text+0x3b72d1): undefined reference to `intel_pmc_gcr_write'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30932 bytes --]

Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

Hi Kuppuswamy,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc3 next-20170310]
[cannot apply to platform-drivers-x86/for-next tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kuppuswamy-Sathyanarayanan/platform-x86-intel_pmc_ipc-fix-gcr-offset/20170320-032638
config: x86_64-randconfig-h0-03200816 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `iTCO_wdt_unset_NO_REBOOT_bit':
>> iTCO_wdt.c:(.text+0x3b6f0f): undefined reference to `intel_pmc_gcr_read'
>> iTCO_wdt.c:(.text+0x3b6f24): undefined reference to `intel_pmc_gcr_write'
   iTCO_wdt.c:(.text+0x3b6f2e): undefined reference to `intel_pmc_gcr_read'
   drivers/built-in.o: In function `iTCO_wdt_set_NO_REBOOT_bit':
   iTCO_wdt.c:(.text+0x3b729e): undefined reference to `intel_pmc_gcr_read'
   iTCO_wdt.c:(.text+0x3b72d1): undefined reference to `intel_pmc_gcr_write'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30932 bytes --]

[PATCH v2 4/4] platform/x86: intel_pmc_ipc: remove iTCO GCR mem resource

[Kuppuswamy Sathyanarayanan]

This patch removes the unused iTCO GCR memory resource

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

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 12018f3..e9e1d62 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;
@@ -529,10 +528,6 @@ static struct resource tco_res[] = {
 	{
 		.flags = IORESOURCE_IO,
 	},
-	/* GCS */
-	{
-		.flags = IORESOURCE_MEM,
-	},
 };
 
 static struct itco_wdt_platform_data tco_info = {
@@ -594,10 +589,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);
@@ -759,7 +750,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

[PATCH v2 4/4] platform/x86: intel_pmc_ipc: remove iTCO GCR mem resource

[Kuppuswamy Sathyanarayanan]

This patch removes the unused iTCO GCR memory resource

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

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 12018f3..e9e1d62 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;
@@ -529,10 +528,6 @@ static struct resource tco_res[] = {
 	{
 		.flags = IORESOURCE_IO,
 	},
-	/* GCS */
-	{
-		.flags = IORESOURCE_MEM,
-	},
 };
 
 static struct itco_wdt_platform_data tco_info = {
@@ -594,10 +589,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);
@@ -759,7 +750,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 v2 4/4] platform/x86: intel_pmc_ipc: remove iTCO GCR mem resource

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 05:41:36PM -0700, Kuppuswamy Sathyanarayanan wrote:
> This patch removes the unused iTCO GCR memory resource
>

Looks fine to me.
 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_ipc.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 12018f3..e9e1d62 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;
> @@ -529,10 +528,6 @@ static struct resource tco_res[] = {
>  	{
>  		.flags = IORESOURCE_IO,
>  	},
> -	/* GCS */
> -	{
> -		.flags = IORESOURCE_MEM,
> -	},
>  };
>  
>  static struct itco_wdt_platform_data tco_info = {
> @@ -594,10 +589,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);
> @@ -759,7 +750,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 v2 4/4] platform/x86: intel_pmc_ipc: remove iTCO GCR mem resource

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 05:41:36PM -0700, Kuppuswamy Sathyanarayanan wrote:
> This patch removes the unused iTCO GCR memory resource
>

Looks fine to me.
 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_ipc.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 12018f3..e9e1d62 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;
> @@ -529,10 +528,6 @@ static struct resource tco_res[] = {
>  	{
>  		.flags = IORESOURCE_IO,
>  	},
> -	/* GCS */
> -	{
> -		.flags = IORESOURCE_MEM,
> -	},
>  };
>  
>  static struct itco_wdt_platform_data tco_info = {
> @@ -594,10 +589,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);
> @@ -759,7 +750,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 v2 1/4] platform/x86: intel_pmc_ipc: fix gcr offset

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 05:41:33PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 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.
>

This one is fine and was one of the WIP patches. This now enables further
cleanup and we should re-align GCR_TELEM_DEEP_S0IX_OFFSET from gcr_base.

CC: Shanth Murthy <shanth.murthy@intel.com>
 
> 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

Re: [PATCH v2 1/4] platform/x86: intel_pmc_ipc: fix gcr offset

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 05:41:33PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 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.
>

This one is fine and was one of the WIP patches. This now enables further
cleanup and we should re-align GCR_TELEM_DEEP_S0IX_OFFSET from gcr_base.

CC: Shanth Murthy <shanth.murthy@intel.com>
 
> 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

Re: [PATCH v2 1/4] platform/x86: intel_pmc_ipc: fix gcr offset

[sathyanarayanan kuppuswamy]

On 03/17/2017 04:13 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:33PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> 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.
>>
> This one is fine and was one of the WIP patches. This now enables further
> cleanup and we should re-align GCR_TELEM_DEEP_S0IX_OFFSET from gcr_base.
Since S0IX_OFFSET currently does not use GCR_OFFSET as base, I think 
that change is irrelevant to this fix.
I can submit another patch for S0IX_OFFSET cleanup.
>
> CC: Shanth Murthy <shanth.murthy@intel.com>
>   
>> 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
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v2 1/4] platform/x86: intel_pmc_ipc: fix gcr offset

[sathyanarayanan kuppuswamy]

On 03/17/2017 04:13 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:33PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> 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.
>>
> This one is fine and was one of the WIP patches. This now enables further
> cleanup and we should re-align GCR_TELEM_DEEP_S0IX_OFFSET from gcr_base.
Since S0IX_OFFSET currently does not use GCR_OFFSET as base, I think 
that change is irrelevant to this fix.
I can submit another patch for S0IX_OFFSET cleanup.
>
> CC: Shanth Murthy <shanth.murthy@intel.com>
>   
>> 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
>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer