Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1614040 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/5] platform/x86: intel_pmc_ipc: fix gcr offset Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> - 2017-03-31 15:40 +0200
    [PATCH v4 3/5] watchdog: iTCO_wdt: Add PMC specific noreboot update api Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-01 01:40 +0200
      Re: [PATCH v4 3/5] watchdog: iTCO_wdt: Add PMC specific noreboot  update api Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-02 16:10 +0200
        Re: [PATCH v4 3/5] watchdog: iTCO_wdt: Add PMC specific noreboot  update api Sathyanarayanan Kuppuswamy Natarajan <sathyaosid@gmail.com> - 2017-04-03 04:00 +0200
    [PATCH v4 2/5] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-01 01:40 +0200
      Re: [PATCH v4 2/5] platform/x86: intel_pmc_ipc: Add pmc gcr  read/write/update api's Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-02 16:00 +0200
        Re: [PATCH v4 2/5] platform/x86: intel_pmc_ipc: Add pmc gcr  read/write/update api's Sathyanarayanan Kuppuswamy Natarajan <sathyaosid@gmail.com> - 2017-04-03 04:00 +0200
          Re: [PATCH v4 2/5] platform/x86: intel_pmc_ipc: Add pmc gcr  read/write/update api's Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-04 15:30 +0200
            Re: [PATCH v4 2/5] platform/x86: intel_pmc_ipc: Add pmc gcr  read/write/update api's sathyanarayanan kuppuswamy          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-04 22:30 +0200
    [PATCH v4 5/5] platform/x86: intel_pmc_ipc: use gcr mem base for S0ix counter read Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-01 01:40 +0200
    [PATCH v4 4/5] platform/x86: intel_pmc_ipc: Fix iTCO GCS memory mapping failure Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-01 01:40 +0200
      Re: [PATCH v4 4/5] platform/x86: intel_pmc_ipc: Fix iTCO GCS memory  mapping failure Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-02 16:20 +0200
        Re: [PATCH v4 4/5] platform/x86: intel_pmc_ipc: Fix iTCO GCS memory  mapping failure Sathyanarayanan Kuppuswamy Natarajan <sathyaosid@gmail.com> - 2017-04-03 04:00 +0200
    [PATCH v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-01 01:40 +0200
      Re: [PATCH v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-02 16:20 +0200
        Re: [PATCH v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset Sathyanarayanan Kuppuswamy Natarajan <sathyaosid@gmail.com> - 2017-04-03 04:00 +0200
          [PATCH v5 4/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot calls Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-04 02:30 +0200
            Re: [PATCH v5 4/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot  calls Guenter Roeck <linux@roeck-us.net> - 2017-04-04 05:30 +0200
            Re: [PATCH v5 4/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot calls Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-04 16:00 +0200
          [PATCH v5 2/6] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-04 02:30 +0200
            Re: [PATCH v5 2/6] platform/x86: intel_pmc_ipc: Add pmc gcr  read/write/update api's Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-04 16:00 +0200
              Re: [PATCH v5 2/6] platform/x86: intel_pmc_ipc: Add pmc gcr  read/write/update api's sathyanarayanan kuppuswamy          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-05 00:20 +0200
          [PATCH v5 6/6] platform/x86: intel_pmc_ipc: use gcr mem base for S0ix counter read Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-04 02:30 +0200
            Re: [PATCH v5 6/6] platform/x86: intel_pmc_ipc: use gcr mem base for  S0ix counter read Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-04 16:00 +0200
              Re: [PATCH v5 6/6] platform/x86: intel_pmc_ipc: use gcr mem base for  S0ix counter read sathyanarayanan kuppuswamy          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-05 00:20 +0200
          [PATCH v5 1/6] platform/x86: intel_pmc_ipc: fix gcr offset Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-04 02:30 +0200
          [PATCH v5 3/6] watchdog: iTCO_wdt: Add PMC specific noreboot update api Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-04 02:30 +0200
            Re: [PATCH v5 3/6] watchdog: iTCO_wdt: Add PMC specific noreboot  update api Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-04 15:50 +0200
          [PATCH v5 5/6] platform/x86: intel_pmc_ipc: Fix iTCO_wdt GCS memory mapping failure Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-04 02:30 +0200
            Re: [PATCH v5 5/6] platform/x86: intel_pmc_ipc: Fix iTCO_wdt GCS  memory mapping failure Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-04 16:00 +0200
          Re: [PATCH v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-04 15:30 +0200
            Re: [PATCH v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset sathyanarayanan kuppuswamy          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-04 23:40 +0200
              [PATCH v6 6/6] platform/x86: intel_pmc_ipc: use gcr mem base for S0ix counter read Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-06 01:00 +0200
              [PATCH v6 5/6] platform/x86: intel_pmc_ipc: Fix iTCO_wdt GCS memory mapping failure Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-06 01:00 +0200
                Re: [PATCH v6 5/6] platform/x86: intel_pmc_ipc: Fix iTCO_wdt GCS  memory mapping failure Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-06 23:40 +0200
              [PATCH v6 3/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot_bit functions Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-06 01:00 +0200
              [PATCH v6 2/6] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-06 01:00 +0200
              [PATCH v6 4/6] watchdog: iTCO_wdt: Add PMC specific noreboot update api Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-06 01:00 +0200
                Re: [PATCH v6 4/6] watchdog: iTCO_wdt: Add PMC specific noreboot  update api Guenter Roeck <linux@roeck-us.net> - 2017-04-06 13:50 +0200
              [PATCH v6 1/6] platform/x86: intel_pmc_ipc: fix gcr offset Kuppuswamy Sathyanarayanan          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-04-06 01:00 +0200
                Re: [PATCH v6 1/6] platform/x86: intel_pmc_ipc: fix gcr offset Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> - 2017-04-06 17:20 +0200

Page 1 of 3  [1] 2 3  Next page →

#1614040 — Re: [PATCH v3 1/5] platform/x86: intel_pmc_ipc: fix gcr offset

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

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

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

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

Have a look at:
intel_pmc_ipc: Fix GCR register base address and length
 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/platform/x86/intel_pmc_ipc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 0651d47..0a33592 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -82,7 +82,7 @@
>  /* exported resources from IFWI */
>  #define PLAT_RESOURCE_IPC_INDEX		0
>  #define PLAT_RESOURCE_IPC_SIZE		0x1000
> -#define PLAT_RESOURCE_GCR_OFFSET	0x1008
> +#define PLAT_RESOURCE_GCR_OFFSET	0x1000
>  #define PLAT_RESOURCE_GCR_SIZE		0x1000
>  #define PLAT_RESOURCE_BIOS_DATA_INDEX	1
>  #define PLAT_RESOURCE_BIOS_IFACE_INDEX	2
> -- 
> 2.7.4
> 

-- 
Best Regards,
Rajneesh

[toc] | [next] | [standalone]

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

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

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

Changes since v3:
 * Rebased on top of latest.

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

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

[toc] | [prev] | [next] | [standalone]

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

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

SoCs.

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

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

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

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

PMC
API

> +       int (*update_noreboot_flag)(bool status);
>         /* the lock for io operations */
>         spinlock_t io_lock;
>         /* the PCI-device */
> @@ -176,9 +178,13 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
>
>         /* Set the NO_REBOOT bit: this disables reboots */
>         if (p->iTCO_version >= 2) {
> -               val32 = readl(p->gcs_pmc);
> -               val32 |= no_reboot_bit(p);
> -               writel(val32, p->gcs_pmc);
> +               if (p->update_noreboot_flag)

> +                       p->update_noreboot_flag(1);

1 -> true for sake of consistency.

> +               else {
> +                       val32 = readl(p->gcs_pmc);
> +                       val32 |= no_reboot_bit(p);
> +                       writel(val32, p->gcs_pmc);
> +               }
>         } else if (p->iTCO_version == 1) {
>                 pci_read_config_dword(p->pci_dev, 0xd4, &val32);
>                 val32 |= no_reboot_bit(p);
> @@ -193,11 +199,14 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
>
>         /* Unset the NO_REBOOT bit: this enables reboots */
>         if (p->iTCO_version >= 2) {
> -               val32 = readl(p->gcs_pmc);
> -               val32 &= ~enable_bit;
> -               writel(val32, p->gcs_pmc);
> -
> -               val32 = readl(p->gcs_pmc);
> +               if (p->update_noreboot_flag)

> +                       return p->update_noreboot_flag(0);

0 -> false.

> +               else {

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

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

Helpers split might be done as a preparatory separate patch.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

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

Hi,

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



-- 
--

Sathya

[toc] | [prev] | [next] | [standalone]

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

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

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

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

iTCO_wdt

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

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

> +static inline int is_gcr_valid(u32 offset)

Pointer to ipcdev should be a parameter to this function.

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

> +/**
> + * intel_pmc_gcr_update() - Update PMC GCR register bits
> + * @offset:    offset of GCR register from GCR address base
> + * @mask:      bit mask for update operation
> + * @val:       update value
> + *

> + * Updates the bits of given GCR register as specified by
> + * mask and val

-> * @mask and @val.

You would need to refresh how to use kernel doc.

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

With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

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

Hi Andy,

Thanks for your comments.

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

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

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

>
>> +{
>> +       if (!ipcdev.has_gcr_regs)
>> +               return -EACCES;
>> +
>> +       if (offset > PLAT_RESOURCE_GCR_SIZE)
>> +               return -EINVAL;
>> +
>> +       return 0;
>> +}
>
>> +/**
>> + * intel_pmc_gcr_update() - Update PMC GCR register bits
>> + * @offset:    offset of GCR register from GCR address base
>> + * @mask:      bit mask for update operation
>> + * @val:       update value
>> + *
>
>> + * Updates the bits of given GCR register as specified by
>> + * mask and val
>
> -> * @mask and @val.
>
> You would need to refresh how to use kernel doc.
-:) will fix it in next version.
>
>> + *
>> + * Return:     negative value on error or 0 on success.
>> + */
>
> With Best Regards,
> Andy Shevchenko



-- 
--

Sathya

[toc] | [prev] | [next] | [standalone]

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

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

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

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

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

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

Hi Andy,


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

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

[toc] | [prev] | [next] | [standalone]

#1614328 — [PATCH v4 5/5] platform/x86: intel_pmc_ipc: use gcr mem base for S0ix counter read

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

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

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

Changes since v2: 
 * Added support for update_noreboot_bit api.

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index bc95bf2..3f3ee50 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -126,7 +126,6 @@ static struct intel_pmc_ipc_dev {
 	struct platform_device *tco_dev;
 
 	/* gcr */
-	resource_size_t gcr_base;
 	void __iomem *gcr_mem_base;
 	int gcr_size;
 	bool has_gcr_regs;
@@ -293,6 +292,15 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
 
+static int update_noreboot_bit(bool status)
+{
+	if (status)
+		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4),
+					    BIT(4));
+	else
+		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4), 0);
+}
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -610,15 +618,12 @@ static struct resource tco_res[] = {
 	{
 		.flags = IORESOURCE_IO,
 	},
-	/* GCS */
-	{
-		.flags = IORESOURCE_MEM,
-	},
 };
 
 static struct itco_wdt_platform_data tco_info = {
 	.name = "Apollo Lake SoC",
 	.version = 5,
+	.update_noreboot_flag = update_noreboot_bit,
 };
 
 #define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
@@ -675,10 +680,6 @@ static int ipc_create_tco_device(void)
 	res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET;
 	res->end = res->start + SMI_EN_SIZE - 1;
 
-	res = tco_res + TCO_RESOURCE_GCR_MEM;
-	res->start = ipcdev.gcr_base + TCO_PMC_OFFSET;
-	res->end = res->start + TCO_PMC_SIZE - 1;
-
 	pdev = platform_device_register_full(&pdevinfo);
 	if (IS_ERR(pdev))
 		return PTR_ERR(pdev);
@@ -840,7 +841,6 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 	}
 	ipcdev.ipc_base = addr;
 
-	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
 	dev_info(&pdev->dev, "ipc res: %pR\n", res);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

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

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

iTCO_wdt or use above in the rest of the series.

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

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


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

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

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

> +       else

Redundant.


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

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

Hi Andy,



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

will go with iTCO_wdt.

>
>> the no reboot setting. Currently, this is done by passing PMC_CFG reg
>> address as memory resource to watchdog driver and allowing it directly
>> modify the PMC_CFG register. But currently PMC driver also has
>> requirement to memory map the entire GCR register space in this driver.
>> This causes mem request failure in watchdog driver. So this patch fixes
>> this issue by adding api to update noreboot flag and passes them
>> to watchdog driver via platform data.
>
>
>> +static int update_noreboot_bit(bool status)
>> +{
>> +       if (status)
>
>> +               return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG, BIT(4),
>> +                                           BIT(4));
>
> BIT(4) is a magic. Moreover, it's used as mask and value here, you
> might introduce temporary variables for better understanding what is
> what.
> As an example you may look at drivers/acpi/acpi_lpss.c (code related
> to IOSF MBI interaction).

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

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



-- 
--

Sathya

[toc] | [prev] | [next] | [standalone]

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

According to Broxton APL PMC spec, gcr mem region starts
at offset 0x1000 from ipc mem base address. In this driver,
PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
memory region from IPC mem region. So we should use 0x1000(4K)
as GCR offset. But currently this driver uses 0x1008 as GCT
offset.This patch fixes this issue.

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

Changes since v3:
 * Updated the commit history

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

[toc] | [prev] | [next] | [standalone]

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

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


So, if I apply this one independently, would it fix an existin issue?

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



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

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

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

On Sun, Apr 2, 2017 at 7:11 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> According to Broxton APL PMC spec, gcr mem region starts
>> at offset 0x1000 from ipc mem base address. In this driver,
>> PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR
>> memory region from IPC mem region. So we should use 0x1000(4K)
>> as GCR offset. But currently this driver uses 0x1008 as GCT
>> offset.This patch fixes this issue.
>
>
> So, if I apply this one independently, would it fix an existin issue?
>
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>  drivers/platform/x86/intel_pmc_ipc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Changes since v3:
>>  * Updated the commit history
>>
>> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
>> index 0651d47..0a33592 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -82,7 +82,7 @@
>>  /* exported resources from IFWI */
>>  #define PLAT_RESOURCE_IPC_INDEX                0
>>  #define PLAT_RESOURCE_IPC_SIZE         0x1000
>> -#define PLAT_RESOURCE_GCR_OFFSET       0x1008
>> +#define PLAT_RESOURCE_GCR_OFFSET       0x1000
>>  #define PLAT_RESOURCE_GCR_SIZE         0x1000
>>  #define PLAT_RESOURCE_BIOS_DATA_INDEX  1
>>  #define PLAT_RESOURCE_BIOS_IFACE_INDEX 2
>> --
>> 2.7.4
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
--

Sathya

[toc] | [prev] | [next] | [standalone]

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

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

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

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 521ae95..cddfa00 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -172,50 +172,35 @@ static inline u32 no_reboot_bit(struct iTCO_wdt_private *p)
 	return enable_bit;
 }
 
-static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
+static int iTCO_wdt_update_no_reboot_flag(struct iTCO_wdt_private *p,
+					  bool status)
 {
-	u32 val32;
+	u32 val32 = 0, newval32 = 0;
 
-	/* Set the NO_REBOOT bit: this disables reboots */
 	if (p->iTCO_version >= 2) {
 		if (p->update_noreboot_flag)
-			p->update_noreboot_flag(true);
+			return p->update_noreboot_flag(status);
 		else {
 			val32 = readl(p->gcs_pmc);
-			val32 |= no_reboot_bit(p);
-			writel(val32, p->gcs_pmc);
-		}
-	} else if (p->iTCO_version == 1) {
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-		val32 |= no_reboot_bit(p);
-		pci_write_config_dword(p->pci_dev, 0xd4, val32);
-	}
-}
-
-static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
-{
-	u32 enable_bit = no_reboot_bit(p);
-	u32 val32 = 0;
+			if (status)
+				val32 |= no_reboot_bit(p);
+			else
+				val32 &= ~no_reboot_bit(p);
 
-	/* Unset the NO_REBOOT bit: this enables reboots */
-	if (p->iTCO_version >= 2) {
-		if (p->update_noreboot_flag)
-			return p->update_noreboot_flag(false);
-		else {
-			val32 = readl(p->gcs_pmc);
-			val32 &= ~enable_bit;
 			writel(val32, p->gcs_pmc);
-			val32 = readl(p->gcs_pmc);
+			newval32 = readl(p->gcs_pmc);
 		}
 	} else if (p->iTCO_version == 1) {
 		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
-		val32 &= ~enable_bit;
+		if (status)
+			val32 |= no_reboot_bit(p);
+		else
+			val32 &= ~no_reboot_bit(p);
 		pci_write_config_dword(p->pci_dev, 0xd4, val32);
-
-		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
+		pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
 	}
 
-	if (val32 & enable_bit)
+	if (val32 != newval32)
 		return -EIO;
 
 	return 0;
@@ -231,7 +216,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
 	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
 
 	/* disable chipset's NO_REBOOT bit */
-	if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
+	if (iTCO_wdt_update_no_reboot_flag(p, false)) {
 		spin_unlock(&p->io_lock);
 		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
 		return -EIO;
@@ -272,7 +257,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
 	val = inw(TCO1_CNT(p));
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	iTCO_wdt_set_NO_REBOOT_bit(p);
+	iTCO_wdt_update_no_reboot_flag(p, true);
 
 	spin_unlock(&p->io_lock);
 
@@ -452,14 +437,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	}
 
 	/* Check chipset's NO_REBOOT bit */
-	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
+	if (iTCO_wdt_update_no_reboot_flag(p, false) &&
 	    iTCO_vendor_check_noreboot_on()) {
 		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
 		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
 	}
 
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
-	iTCO_wdt_set_NO_REBOOT_bit(p);
+	iTCO_wdt_update_no_reboot_flag(p, true);
 
 	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
 	if (!devm_request_region(dev, p->smi_res->start,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1615685 — Re: [PATCH v5 4/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot calls

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

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

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

Why not stick with bit ?

> +					  bool status)

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

>  {
> -	u32 val32;
> +	u32 val32 = 0, newval32 = 0;
>
> -	/* Set the NO_REBOOT bit: this disables reboots */
>  	if (p->iTCO_version >= 2) {
>  		if (p->update_noreboot_flag)
> -			p->update_noreboot_flag(true);
> +			return p->update_noreboot_flag(status);
>  		else {
>  			val32 = readl(p->gcs_pmc);
> -			val32 |= no_reboot_bit(p);
> -			writel(val32, p->gcs_pmc);
> -		}
> -	} else if (p->iTCO_version == 1) {
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 |= no_reboot_bit(p);
> -		pci_write_config_dword(p->pci_dev, 0xd4, val32);
> -	}
> -}
> -
> -static int iTCO_wdt_unset_NO_REBOOT_bit(struct iTCO_wdt_private *p)
> -{
> -	u32 enable_bit = no_reboot_bit(p);
> -	u32 val32 = 0;
> +			if (status)
> +				val32 |= no_reboot_bit(p);
> +			else
> +				val32 &= ~no_reboot_bit(p);
>
> -	/* Unset the NO_REBOOT bit: this enables reboots */
> -	if (p->iTCO_version >= 2) {
> -		if (p->update_noreboot_flag)
> -			return p->update_noreboot_flag(false);
> -		else {
> -			val32 = readl(p->gcs_pmc);
> -			val32 &= ~enable_bit;
>  			writel(val32, p->gcs_pmc);
> -			val32 = readl(p->gcs_pmc);
> +			newval32 = readl(p->gcs_pmc);
>  		}
>  	} else if (p->iTCO_version == 1) {
>  		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> -		val32 &= ~enable_bit;
> +		if (status)
> +			val32 |= no_reboot_bit(p);
> +		else
> +			val32 &= ~no_reboot_bit(p);
>  		pci_write_config_dword(p->pci_dev, 0xd4, val32);
> -
> -		pci_read_config_dword(p->pci_dev, 0xd4, &val32);
> +		pci_read_config_dword(p->pci_dev, 0xd4, &newval32);
>  	}
>
> -	if (val32 & enable_bit)
> +	if (val32 != newval32)
>  		return -EIO;
>
>  	return 0;
> @@ -231,7 +216,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
>  	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
>
>  	/* disable chipset's NO_REBOOT bit */
> -	if (iTCO_wdt_unset_NO_REBOOT_bit(p)) {
> +	if (iTCO_wdt_update_no_reboot_flag(p, false)) {
>  		spin_unlock(&p->io_lock);
>  		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
>  		return -EIO;
> @@ -272,7 +257,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
>  	val = inw(TCO1_CNT(p));
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	iTCO_wdt_set_NO_REBOOT_bit(p);
> +	iTCO_wdt_update_no_reboot_flag(p, true);
>
>  	spin_unlock(&p->io_lock);
>
> @@ -452,14 +437,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	}
>
>  	/* Check chipset's NO_REBOOT bit */
> -	if (iTCO_wdt_unset_NO_REBOOT_bit(p) &&
> +	if (iTCO_wdt_update_no_reboot_flag(p, false) &&
>  	    iTCO_vendor_check_noreboot_on()) {
>  		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
>  		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
>  	}
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	iTCO_wdt_set_NO_REBOOT_bit(p);
> +	iTCO_wdt_update_no_reboot_flag(p, true);
>
>  	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
>  	if (!devm_request_region(dev, p->smi_res->start,
>

[toc] | [prev] | [next] | [standalone]

#1616046 — Re: [PATCH v5 4/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot calls

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

Something similar should go before patch 3/6.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

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

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

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

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

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

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

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web