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 2 of 3 — ← Prev page 1 [2] 3  Next page →

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

On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> This patch adds API's to read/write/update PMC GC registers.
> PMC dependent devices like iTCO_wdt, Telemetry has requirement
> to acces GCR registers. These API's can be used for this
> purpose.

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

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

> +       void __iomem *gcr_mem_base;
>         int gcr_size;

Rearrange those lines to make __iomem pointer latter.

> +static inline int is_gcr_valid(u32 offset)

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

> +/**
> + * intel_pmc_gcr_write() - Write PMC GCR register
> + * @offset:    offset of GCR register from GCR address base
> + * @data:      register update value
> + *
> + * Writes the PMC GCR register of given offset with given
> + * value

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

> +/**
> + * intel_pmc_gcr_update() - Update PMC GCR register bits
> + * @offset:    offset of GCR register from GCR address base
> + * @mask:      bit mask for update operation
> + * @val:       update value
> + *
> + * Updates the bits of given GCR register as specified by
> + * @mask and @val

Ditto.

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

> +       u32 orig, tmp;

One of them is redundant.

I would go just with
u32 value;

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

> +

Redundant.

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

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

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

> +gcr_update_err:

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

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

-- 
With Best Regards,
Andy Shevchenko

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

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

Hi Andy,


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

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

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

#1615632 — [PATCH v5 6/6] 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 v4:
 * Rebased on top of latest changes.

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

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

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

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

On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> To maintain the uniformity in accessing GCR registers, this patch
> modifies the S0ix counter read function to use GCR address base
> instead of ipc address base.
>

This one looks good.

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

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

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

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

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

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

> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> Tested-by: Shanth Murthy <shanth.murthy@intel.com>
> ---
>  arch/x86/include/asm/intel_pmc_ipc.h |  2 ++
>  drivers/platform/x86/intel_pmc_ipc.c | 10 +++-------
>  2 files changed, 5 insertions(+), 7 deletions(-)
>
> Changes since v4:
>  * Rebased on top of latest changes.
>
> Changes since v3:
>  * Rebased on top of latest changes.
>
> diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
> index 8402efe..fac89eb 100644
> --- a/arch/x86/include/asm/intel_pmc_ipc.h
> +++ b/arch/x86/include/asm/intel_pmc_ipc.h
> @@ -25,6 +25,8 @@
>
>  /* GCR reg offsets from gcr base*/
>  #define PMC_GCR_PMC_CFG_REG            0x08
> +#define PMC_GCR_TELEM_DEEP_S0IX_REG    0x78
> +#define PMC_GCR_TELEM_SHLW_S0IX_REG    0x80
>
>  #if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
>
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 3d0d6f17..b61f569 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -57,10 +57,6 @@
>  #define IPC_WRITE_BUFFER       0x80
>  #define IPC_READ_BUFFER                0x90
>
> -/* PMC Global Control Registers */
> -#define GCR_TELEM_DEEP_S0IX_OFFSET     0x1078
> -#define GCR_TELEM_SHLW_S0IX_OFFSET     0x1080
> -
>  /* Residency with clock rate at 19.2MHz to usecs */
>  #define S0IX_RESIDENCY_IN_USECS(d, s)          \
>  ({                                             \
> @@ -203,7 +199,7 @@ static inline u32 ipc_data_readl(u32 offset)
>
>  static inline u64 gcr_data_readq(u32 offset)
>  {
> -       return readq(ipcdev.ipc_base + offset);
> +       return readq(ipcdev.gcr_mem_base + offset);
>  }
>
>  static inline int is_gcr_valid(u32 offset)
> @@ -906,8 +902,8 @@ int intel_pmc_s0ix_counter_read(u64 *data)
>         if (!ipcdev.has_gcr_regs)
>                 return -EACCES;
>
> -       deep = gcr_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
> -       shlw = gcr_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
> +       deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);
> +       shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);
>
>         *data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
>
> --
> 2.7.4
>



-- 
With Best Regards,
Andy Shevchenko

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

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

Hi Andy,


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

For #2, myself and Rajnessh created a initial proposal for refactoring 
task and we will share it with you in few days.
>
> Like we agreed it would be last series I'm going to apply without
> refactoring done.
:) Its our deal. Thanks.
>
> --- 8< --- 8< ---
>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Reviewed-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
>> Tested-by: Shanth Murthy <shanth.murthy@intel.com>
>> ---
>>   arch/x86/include/asm/intel_pmc_ipc.h |  2 ++
>>   drivers/platform/x86/intel_pmc_ipc.c | 10 +++-------
>>   2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> Changes since v4:
>>   * Rebased on top of latest changes.
>>
>> Changes since v3:
>>   * Rebased on top of latest changes.
>>
>> diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
>> index 8402efe..fac89eb 100644
>> --- a/arch/x86/include/asm/intel_pmc_ipc.h
>> +++ b/arch/x86/include/asm/intel_pmc_ipc.h
>> @@ -25,6 +25,8 @@
>>
>>   /* GCR reg offsets from gcr base*/
>>   #define PMC_GCR_PMC_CFG_REG            0x08
>> +#define PMC_GCR_TELEM_DEEP_S0IX_REG    0x78
>> +#define PMC_GCR_TELEM_SHLW_S0IX_REG    0x80
>>
>>   #if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
>>
>> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
>> index 3d0d6f17..b61f569 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -57,10 +57,6 @@
>>   #define IPC_WRITE_BUFFER       0x80
>>   #define IPC_READ_BUFFER                0x90
>>
>> -/* PMC Global Control Registers */
>> -#define GCR_TELEM_DEEP_S0IX_OFFSET     0x1078
>> -#define GCR_TELEM_SHLW_S0IX_OFFSET     0x1080
>> -
>>   /* Residency with clock rate at 19.2MHz to usecs */
>>   #define S0IX_RESIDENCY_IN_USECS(d, s)          \
>>   ({                                             \
>> @@ -203,7 +199,7 @@ static inline u32 ipc_data_readl(u32 offset)
>>
>>   static inline u64 gcr_data_readq(u32 offset)
>>   {
>> -       return readq(ipcdev.ipc_base + offset);
>> +       return readq(ipcdev.gcr_mem_base + offset);
>>   }
>>
>>   static inline int is_gcr_valid(u32 offset)
>> @@ -906,8 +902,8 @@ int intel_pmc_s0ix_counter_read(u64 *data)
>>          if (!ipcdev.has_gcr_regs)
>>                  return -EACCES;
>>
>> -       deep = gcr_data_readq(GCR_TELEM_DEEP_S0IX_OFFSET);
>> -       shlw = gcr_data_readq(GCR_TELEM_SHLW_S0IX_OFFSET);
>> +       deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);
>> +       shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);
>>
>>          *data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
>>
>> --
>> 2.7.4
>>
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

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

#1615634 — [PATCH v5 1/6] 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 v4:
 *  None

Changes since v3:
 * Updated the commit history

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

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

#1615635 — [PATCH v5 3/6] 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 v4:
 * Fixed some style issues.
 * used false for 0 and true for 1 in update_noreboot_flag function.

Changes since v3:
 * Rebased on top of latest.

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

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

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

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

On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> In some SoCs, setting noreboot bit needs modification to
> PMC GC registers. But not all PMC drivers allow other drivers
> to memory map their GC region. This could create mem request
> conflict in watchdog driver. So this patch adds facility to allow
> PMC drivers to pass noreboot update function to watchdog
> drivers via platform data.

>         /* Set the NO_REBOOT bit: this disables reboots */
>         if (p->iTCO_version >= 2) {
> -               val32 = readl(p->gcs_pmc);
> -               val32 |= no_reboot_bit(p);
> -               writel(val32, p->gcs_pmc);
> +               if (p->update_noreboot_flag)
> +                       p->update_noreboot_flag(true);
> +               else {
> +                       val32 = readl(p->gcs_pmc);
> +                       val32 |= no_reboot_bit(p);
> +                       writel(val32, p->gcs_pmc);
> +               }

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

-- 
With Best Regards,
Andy Shevchenko

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

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

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

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

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

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

Changes since v2: 
 * Added support for update_noreboot_bit api.

diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index 8b7fef0..3d0d6f17 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -112,6 +112,13 @@
 #define TCO_PMC_OFFSET			0x8
 #define TCO_PMC_SIZE			0x4
 
+/* PMC register bit definitions */
+
+/* PMC_CFG_REG bit masks */
+#define PMC_CFG_NO_REBOOT_MASK		BIT(4)
+#define PMC_CFG_NO_REBOOT_ENABLE	BIT(4)
+#define PMC_CFG_NO_REBOOT_DISABLE	0
+
 static struct intel_pmc_ipc_dev {
 	struct device *dev;
 	void __iomem *ipc_base;
@@ -126,7 +133,6 @@ static struct intel_pmc_ipc_dev {
 	struct platform_device *tco_dev;
 
 	/* gcr */
-	resource_size_t gcr_base;
 	void __iomem *gcr_mem_base;
 	int gcr_size;
 	bool has_gcr_regs;
@@ -312,6 +318,18 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
 }
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
 
+static int update_noreboot_bit(bool status)
+{
+	if (status)
+		return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
+					    PMC_CFG_NO_REBOOT_MASK,
+					    PMC_CFG_NO_REBOOT_ENABLE);
+
+	return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
+				    PMC_CFG_NO_REBOOT_MASK,
+				    PMC_CFG_NO_REBOOT_DISABLE);
+}
+
 static int intel_pmc_ipc_check_status(void)
 {
 	int status;
@@ -629,15 +647,12 @@ static struct resource tco_res[] = {
 	{
 		.flags = IORESOURCE_IO,
 	},
-	/* GCS */
-	{
-		.flags = IORESOURCE_MEM,
-	},
 };
 
 static struct itco_wdt_platform_data tco_info = {
 	.name = "Apollo Lake SoC",
 	.version = 5,
+	.update_noreboot_flag = update_noreboot_bit,
 };
 
 #define TELEMETRY_RESOURCE_PUNIT_SSRAM	0
@@ -694,10 +709,6 @@ static int ipc_create_tco_device(void)
 	res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET;
 	res->end = res->start + SMI_EN_SIZE - 1;
 
-	res = tco_res + TCO_RESOURCE_GCR_MEM;
-	res->start = ipcdev.gcr_base + TCO_PMC_OFFSET;
-	res->end = res->start + TCO_PMC_SIZE - 1;
-
 	pdev = platform_device_register_full(&pdevinfo);
 	if (IS_ERR(pdev))
 		return PTR_ERR(pdev);
@@ -859,7 +870,6 @@ static int ipc_plat_get_res(struct platform_device *pdev)
 	}
 	ipcdev.ipc_base = addr;
 
-	ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
 	ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
 	dev_info(&pdev->dev, "ipc res: %pR\n", res);
-- 
2.7.4

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

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

On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> iTCO_wdt driver need access to PMC_CFG GCR register to modify the
> noreboot setting. Currently, this is done by passing PMC_CFG reg
> address as memory resource to watchdog driver and allowing it directly
> modify the PMC_CFG register. But currently PMC driver also has
> requirement to memory map the entire GCR register space in this driver.
> This causes mem request failure in watchdog driver. So this patch fixes
> this issue by adding api to update noreboot flag and passes them

api -> API

> to watchdog driver via platform data.

This one looks good.

> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 8b7fef0..3d0d6f17 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -112,6 +112,13 @@
>  #define TCO_PMC_OFFSET                 0x8
>  #define TCO_PMC_SIZE                   0x4
>
> +/* PMC register bit definitions */
> +
> +/* PMC_CFG_REG bit masks */
> +#define PMC_CFG_NO_REBOOT_MASK         BIT(4)
> +#define PMC_CFG_NO_REBOOT_ENABLE       BIT(4)
> +#define PMC_CFG_NO_REBOOT_DISABLE      0
> +
>  static struct intel_pmc_ipc_dev {
>         struct device *dev;
>         void __iomem *ipc_base;
> @@ -126,7 +133,6 @@ static struct intel_pmc_ipc_dev {
>         struct platform_device *tco_dev;
>
>         /* gcr */
> -       resource_size_t gcr_base;
>         void __iomem *gcr_mem_base;
>         int gcr_size;
>         bool has_gcr_regs;
> @@ -312,6 +318,18 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
>  }
>  EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
>
> +static int update_noreboot_bit(bool status)
> +{
> +       if (status)
> +               return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
> +                                           PMC_CFG_NO_REBOOT_MASK,
> +                                           PMC_CFG_NO_REBOOT_ENABLE);
> +
> +       return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
> +                                   PMC_CFG_NO_REBOOT_MASK,
> +                                   PMC_CFG_NO_REBOOT_DISABLE);
> +}
> +
>  static int intel_pmc_ipc_check_status(void)
>  {
>         int status;
> @@ -629,15 +647,12 @@ static struct resource tco_res[] = {
>         {
>                 .flags = IORESOURCE_IO,
>         },
> -       /* GCS */
> -       {
> -               .flags = IORESOURCE_MEM,
> -       },
>  };
>
>  static struct itco_wdt_platform_data tco_info = {
>         .name = "Apollo Lake SoC",
>         .version = 5,
> +       .update_noreboot_flag = update_noreboot_bit,
>  };
>
>  #define TELEMETRY_RESOURCE_PUNIT_SSRAM 0
> @@ -694,10 +709,6 @@ static int ipc_create_tco_device(void)
>         res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET;
>         res->end = res->start + SMI_EN_SIZE - 1;
>
> -       res = tco_res + TCO_RESOURCE_GCR_MEM;
> -       res->start = ipcdev.gcr_base + TCO_PMC_OFFSET;
> -       res->end = res->start + TCO_PMC_SIZE - 1;
> -
>         pdev = platform_device_register_full(&pdevinfo);
>         if (IS_ERR(pdev))
>                 return PTR_ERR(pdev);
> @@ -859,7 +870,6 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>         }
>         ipcdev.ipc_base = addr;
>
> -       ipcdev.gcr_base = res->start + PLAT_RESOURCE_GCR_OFFSET;
>         ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
>         ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE;
>         dev_info(&pdev->dev, "ipc res: %pR\n", res);

-- 
With Best Regards,
Andy Shevchenko

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

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

Please, STOP top-posting.

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

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

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



-- 
With Best Regards,
Andy Shevchenko

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

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

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

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

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

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

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

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

#1617411 — [PATCH v6 6/6] 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(-)

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

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

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

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

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

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

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

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

Changes since v2: 
 * Added support for update_noreboot_bit api.

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

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

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

On Thu, Apr 6, 2017 at 1:54 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> iTCO_wdt driver need access to PMC_CFG GCR register to modify the
> noreboot setting. Currently, this is done by passing PMC_CFG reg
> address as memory resource to watchdog driver and allowing it directly
> modify the PMC_CFG register. But currently PMC driver also has
> requirement to memory map the entire GCR register space in this driver.
> This causes mem request failure in watchdog driver. So this patch fixes
> this issue by adding API to update noreboot flag and passes them
> to watchdog driver via platform data.

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

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

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

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

u32 value = set ? PMC_CFG_NO_REBOOT_ENABLE : PMC_CFG_NO_REBOOT_DISABLE;

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

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

-- 
With Best Regards,
Andy Shevchenko

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

#1617413 — [PATCH v6 3/6] watchdog: iTCO_wdt: cleanup set/unset no_reboot_bit functions

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

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

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

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

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

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

#1617414 — [PATCH v6 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 | 115 +++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+)

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

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

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

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

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

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

#1617416 — [PATCH v6 4/6] 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            | 20 ++++++++++++++------
 include/linux/platform_data/itco_wdt.h |  4 ++++
 2 files changed, 18 insertions(+), 6 deletions(-)

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

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

Changes since v3:
 * Rebased on top of latest.

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

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

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

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

On 04/05/2017 03:54 PM, Kuppuswamy Sathyanarayanan wrote:
> In some SoCs, setting noreboot bit needs modification to
> PMC GC registers. But not all PMC drivers allow other drivers
> to memory map their GC region. This could create mem request
> conflict in watchdog driver. So this patch adds facility to allow
> PMC drivers to pass noreboot update function to watchdog
> drivers via platform data.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/watchdog/iTCO_wdt.c            | 20 ++++++++++++++------
>  include/linux/platform_data/itco_wdt.h |  4 ++++
>  2 files changed, 18 insertions(+), 6 deletions(-)
>
> Changes since v5:
>  * Added priv_data to pass private data to no_reboot_bit update function.
>  * Changed update_noreboot_flag() to update_no_reboot_bit
>
> Changes since v4:
>  * Fixed some style issues.
>  * used false for 0 and true for 1 in update_noreboot_flag function.
>
> Changes since v3:
>  * Rebased on top of latest.
>
> Changes since v2:
>  * Removed use of PMC API's directly in watchdog driver.
>  * Added update_noreboot_flag to handle no IPC PMC compatibility
>    issue mentioned by Guenter.
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index d8768a2..3f00029 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -106,6 +106,8 @@ struct iTCO_wdt_private {
>  	struct pci_dev *pci_dev;
>  	/* whether or not the watchdog has been suspended */
>  	bool suspended;
> +	/* no reboot API private data */
> +	void *no_reboot_priv;
>  	/* no reboot update function pointer */
>  	int (*update_no_reboot_bit)(void *p, bool set);
>  };
> @@ -225,6 +227,8 @@ void iTCO_wdt_no_reboot_bit_setup(struct iTCO_wdt_private *p)
>  		p->update_no_reboot_bit = update_no_reboot_bit_pci;
>  	else
>  		p->update_no_reboot_bit = update_no_reboot_bit_def;
> +
> +	p->no_reboot_priv = p;
>  }
>
>  static int iTCO_wdt_start(struct watchdog_device *wd_dev)
> @@ -237,7 +241,7 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
>  	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
>
>  	/* disable chipset's NO_REBOOT bit */
> -	if (p->update_no_reboot_bit(p, false)) {
> +	if (p->update_no_reboot_bit(p->no_reboot_priv, false)) {
>  		spin_unlock(&p->io_lock);
>  		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
>  		return -EIO;
> @@ -278,7 +282,7 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
>  	val = inw(TCO1_CNT(p));
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	p->update_no_reboot_bit(p, true);
> +	p->update_no_reboot_bit(p->no_reboot_priv, true);
>
>  	spin_unlock(&p->io_lock);
>
> @@ -442,14 +446,18 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>
>  	p->iTCO_version = pdata->version;
>  	p->pci_dev = to_pci_dev(dev->parent);
> +	p->no_reboot_priv = pdata->no_reboot_priv;
>

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

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

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

> +	else
> +		iTCO_wdt_no_reboot_bit_setup(p);
>
>  	/*
>  	 * Get the Memory-Mapped GCS or PMC register, we need it for the
>  	 * NO_REBOOT flag (TCO v2 and v3).
>  	 */
> -	if (p->iTCO_version >= 2) {
> +	if (p->iTCO_version >= 2 && !pdata->update_no_reboot_bit) {
>  		p->gcs_pmc_res = platform_get_resource(pdev,
>  						       IORESOURCE_MEM,
>  						       ICH_RES_MEM_GCS_PMC);
> @@ -459,14 +467,14 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	}
>
>  	/* Check chipset's NO_REBOOT bit */
> -	if (p->update_no_reboot_bit(p, false) &&
> +	if (p->update_no_reboot_bit(p->no_reboot_priv, false) &&
>  	    iTCO_vendor_check_noreboot_on()) {
>  		pr_info("unable to reset NO_REBOOT flag, device disabled by hardware/BIOS\n");
>  		return -ENODEV;	/* Cannot reset NO_REBOOT bit */
>  	}
>
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> -	p->update_no_reboot_bit(p, true);
> +	p->update_no_reboot_bit(p->no_reboot_priv, true);
>
>  	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
>  	if (!devm_request_region(dev, p->smi_res->start,
> diff --git a/include/linux/platform_data/itco_wdt.h b/include/linux/platform_data/itco_wdt.h
> index f16542c..0e95527 100644
> --- a/include/linux/platform_data/itco_wdt.h
> +++ b/include/linux/platform_data/itco_wdt.h
> @@ -14,6 +14,10 @@
>  struct itco_wdt_platform_data {
>  	char name[32];
>  	unsigned int version;
> +	/* private data to be passed to update_no_reboot_bit API */
> +	void *no_reboot_priv;
> +	/* pointer for platform specific no reboot update function */
> +	int (*update_no_reboot_bit)(void *priv, bool set);
>  };
>
>  #endif /* _ITCO_WDT_H_ */
>

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

#1617417 — [PATCH v6 1/6] 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 v5:
 *  None

Changes since v4:
 *  None

Changes since v3:
 * Updated the commit history

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

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

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web