Re: [RFC v2 6/6] platform/x86: intel_pmc_ipc: Use generic Intel IPC device calls

2017-09-06 Thread Andy Shevchenko 
On Wed, Sep 6, 2017 at 8:27 AM, Kuppuswamy, Sathyanarayanan
 wrote:
> On 9/5/2017 12:38 AM, Lee Jones wrote:
>> On Sat, 02 Sep 2017, sathyanarayanan.kuppusw...@linux.intel.com wrote:

>> I'm a bit concerned by the API.
>
> This is not a new change. Even before refactoring this driver, we have been
> using u32 bit variable to pass the DPTR and SPTR address.
>>
>>Any reason why you're not using
>> pointers for addresses?
>
> I think the main reason is,  this is the expected format defined by SCU/PMC
> spec. According to the spec document, SPTR and DPTR registers are used to
> program the 32 bit SRAM address from which the PMC/SCU firmware can
> read/write the data of an IPC command. if we are not using SPTR or DPTR, we
> need to leave the value at zero.

It's an effect, the cause is that the system controllers which are
used in Intel SoCs are 32-bit microprocessors and they can't address
more. That's why SRAM is placed in 32-bit address space.
And thus, the SCU protocol defines fixed width parameters for
addresses. So, it means we can't use any address outside of 4G space,
iow 32-bit wide.

>> pointers, you should be using NULL, instead of 0.


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC v2 6/6] platform/x86: intel_pmc_ipc: Use generic Intel IPC device calls

2017-09-06 Thread Andy Shevchenko 
On Wed, Sep 6, 2017 at 8:27 AM, Kuppuswamy, Sathyanarayanan
 wrote:
> On 9/5/2017 12:38 AM, Lee Jones wrote:
>> On Sat, 02 Sep 2017, sathyanarayanan.kuppusw...@linux.intel.com wrote:

>> I'm a bit concerned by the API.
>
> This is not a new change. Even before refactoring this driver, we have been
> using u32 bit variable to pass the DPTR and SPTR address.
>>
>>Any reason why you're not using
>> pointers for addresses?
>
> I think the main reason is,  this is the expected format defined by SCU/PMC
> spec. According to the spec document, SPTR and DPTR registers are used to
> program the 32 bit SRAM address from which the PMC/SCU firmware can
> read/write the data of an IPC command. if we are not using SPTR or DPTR, we
> need to leave the value at zero.

It's an effect, the cause is that the system controllers which are
used in Intel SoCs are 32-bit microprocessors and they can't address
more. That's why SRAM is placed in 32-bit address space.
And thus, the SCU protocol defines fixed width parameters for
addresses. So, it means we can't use any address outside of 4G space,
iow 32-bit wide.

>> pointers, you should be using NULL, instead of 0.


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC v2 6/6] platform/x86: intel_pmc_ipc: Use generic Intel IPC device calls

2017-09-05 Thread Kuppuswamy, Sathyanarayanan 

Hi Lee,


On 9/5/2017 12:38 AM, Lee Jones wrote:

On Sat, 02 Sep 2017, sathyanarayanan.kuppusw...@linux.intel.com wrote:


From: Kuppuswamy Sathyanarayanan 

Removed redundant IPC helper functions and refactored the driver to use
generic IPC device driver APIs.

This patch also cleans-up PMC IPC user drivers to use APIs provided
by generic IPC driver.

Signed-off-by: Kuppuswamy Sathyanarayanan 

---
  arch/x86/include/asm/intel_pmc_ipc.h  |  37 +--
  drivers/mfd/intel_soc_pmic_bxtwc.c|  24 +-
  include/linux/mfd/intel_soc_pmic.h|   2 +

I'm a bit concerned by the API.
This is not a new change. Even before refactoring this driver, we have 
been using u32 bit variable to pass the DPTR and SPTR address.

   Any reason why you're not using
pointers for addresses?
I think the main reason is,  this is the expected format defined by 
SCU/PMC spec. According to the spec document, SPTR and DPTR registers 
are used to program the 32 bit SRAM address from which the PMC/SCU 
firmware can read/write the data of an IPC command. if we are not using 
SPTR or DPTR, we need to leave the value at zero.

pointers, you should be using NULL, instead of 0.


  drivers/platform/x86/intel_pmc_ipc.c  | 364 +-
  drivers/platform/x86/intel_telemetry_pltdrv.c | 114 
  5 files changed, 215 insertions(+), 326 deletions(-)

Changes since v1:
  * Removed custom APIs.
  * Cleaned up PMC IPC user drivers to use APIs provided by generic
IPC driver.

Re: [RFC v2 6/6] platform/x86: intel_pmc_ipc: Use generic Intel IPC device calls

2017-09-05 Thread Kuppuswamy, Sathyanarayanan 

Hi Lee,


On 9/5/2017 12:38 AM, Lee Jones wrote:

On Sat, 02 Sep 2017, sathyanarayanan.kuppusw...@linux.intel.com wrote:


From: Kuppuswamy Sathyanarayanan 

Removed redundant IPC helper functions and refactored the driver to use
generic IPC device driver APIs.

This patch also cleans-up PMC IPC user drivers to use APIs provided
by generic IPC driver.

Signed-off-by: Kuppuswamy Sathyanarayanan 

---
  arch/x86/include/asm/intel_pmc_ipc.h  |  37 +--
  drivers/mfd/intel_soc_pmic_bxtwc.c|  24 +-
  include/linux/mfd/intel_soc_pmic.h|   2 +

I'm a bit concerned by the API.
This is not a new change. Even before refactoring this driver, we have 
been using u32 bit variable to pass the DPTR and SPTR address.

   Any reason why you're not using
pointers for addresses?
I think the main reason is,  this is the expected format defined by 
SCU/PMC spec. According to the spec document, SPTR and DPTR registers 
are used to program the 32 bit SRAM address from which the PMC/SCU 
firmware can read/write the data of an IPC command. if we are not using 
SPTR or DPTR, we need to leave the value at zero.

pointers, you should be using NULL, instead of 0.


  drivers/platform/x86/intel_pmc_ipc.c  | 364 +-
  drivers/platform/x86/intel_telemetry_pltdrv.c | 114 
  5 files changed, 215 insertions(+), 326 deletions(-)

Changes since v1:
  * Removed custom APIs.
  * Cleaned up PMC IPC user drivers to use APIs provided by generic
IPC driver.

Re: [RFC v2 6/6] platform/x86: intel_pmc_ipc: Use generic Intel IPC device calls

2017-09-05 Thread Lee Jones 
On Sat, 02 Sep 2017, sathyanarayanan.kuppusw...@linux.intel.com wrote:

> From: Kuppuswamy Sathyanarayanan 
> 
> Removed redundant IPC helper functions and refactored the driver to use
> generic IPC device driver APIs.
> 
> This patch also cleans-up PMC IPC user drivers to use APIs provided
> by generic IPC driver.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan 
> 
> ---
>  arch/x86/include/asm/intel_pmc_ipc.h  |  37 +--

>  drivers/mfd/intel_soc_pmic_bxtwc.c|  24 +-
>  include/linux/mfd/intel_soc_pmic.h|   2 +

I'm a bit concerned by the API.  Any reason why you're not using
pointers for addresses?  And for the arguments where you are using
pointers, you should be using NULL, instead of 0.

>  drivers/platform/x86/intel_pmc_ipc.c  | 364 
> +-
>  drivers/platform/x86/intel_telemetry_pltdrv.c | 114 
>  5 files changed, 215 insertions(+), 326 deletions(-)
> 
> Changes since v1:
>  * Removed custom APIs.
>  * Cleaned up PMC IPC user drivers to use APIs provided by generic
>IPC driver.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC v2 6/6] platform/x86: intel_pmc_ipc: Use generic Intel IPC device calls

2017-09-05 Thread Lee Jones 
On Sat, 02 Sep 2017, sathyanarayanan.kuppusw...@linux.intel.com wrote:

> From: Kuppuswamy Sathyanarayanan 
> 
> Removed redundant IPC helper functions and refactored the driver to use
> generic IPC device driver APIs.
> 
> This patch also cleans-up PMC IPC user drivers to use APIs provided
> by generic IPC driver.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan 
> 
> ---
>  arch/x86/include/asm/intel_pmc_ipc.h  |  37 +--

>  drivers/mfd/intel_soc_pmic_bxtwc.c|  24 +-
>  include/linux/mfd/intel_soc_pmic.h|   2 +

I'm a bit concerned by the API.  Any reason why you're not using
pointers for addresses?  And for the arguments where you are using
pointers, you should be using NULL, instead of 0.

>  drivers/platform/x86/intel_pmc_ipc.c  | 364 
> +-
>  drivers/platform/x86/intel_telemetry_pltdrv.c | 114 
>  5 files changed, 215 insertions(+), 326 deletions(-)
> 
> Changes since v1:
>  * Removed custom APIs.
>  * Cleaned up PMC IPC user drivers to use APIs provided by generic
>IPC driver.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[RFC v2 6/6] platform/x86: intel_pmc_ipc: Use generic Intel IPC device calls

2017-09-02 Thread sathyanarayanan . kuppuswamy 
From: Kuppuswamy Sathyanarayanan 

Removed redundant IPC helper functions and refactored the driver to use
generic IPC device driver APIs.

This patch also cleans-up PMC IPC user drivers to use APIs provided
by generic IPC driver.

Signed-off-by: Kuppuswamy Sathyanarayanan 

---
 arch/x86/include/asm/intel_pmc_ipc.h  |  37 +--
 drivers/mfd/intel_soc_pmic_bxtwc.c|  24 +-
 drivers/platform/x86/intel_pmc_ipc.c  | 364 +-
 drivers/platform/x86/intel_telemetry_pltdrv.c | 114 
 include/linux/mfd/intel_soc_pmic.h|   2 +
 5 files changed, 215 insertions(+), 326 deletions(-)

Changes since v1:
 * Removed custom APIs.
 * Cleaned up PMC IPC user drivers to use APIs provided by generic
   IPC driver.

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h 
b/arch/x86/include/asm/intel_pmc_ipc.h
index fac89eb..9fc7c3c 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -1,10 +1,15 @@
 #ifndef _ASM_X86_INTEL_PMC_IPC_H_
 #define  _ASM_X86_INTEL_PMC_IPC_H_
 
+#include 
+
+#define INTEL_PMC_IPC_DEV  "intel_pmc_ipc"
+#define PMC_PARAM_LEN  2
+
 /* Commands */
 #define PMC_IPC_PMIC_ACCESS0xFF
-#definePMC_IPC_PMIC_ACCESS_READ0x0
-#definePMC_IPC_PMIC_ACCESS_WRITE   0x1
+#definePMC_IPC_PMIC_ACCESS_READ0x0
+#definePMC_IPC_PMIC_ACCESS_WRITE   0x1
 #define PMC_IPC_USB_PWR_CTRL   0xF0
 #define PMC_IPC_PMIC_BLACKLIST_SEL 0xEF
 #define PMC_IPC_PHY_CONFIG 0xEE
@@ -28,13 +33,14 @@
 #define PMC_GCR_TELEM_DEEP_S0IX_REG0x78
 #define PMC_GCR_TELEM_SHLW_S0IX_REG0x80
 
+static inline void pmc_cmd_init(u32 *cmd, u32 param1, u32 param2)
+{
+   cmd[0] = param1;
+   cmd[1] = param2;
+}
+
 #if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
 
-int intel_pmc_ipc_simple_command(int cmd, int sub);
-int intel_pmc_ipc_raw_cmd(u32 cmd, u32 sub, u8 *in, u32 inlen,
-   u32 *out, u32 outlen, u32 dptr, u32 sptr);
-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);
@@ -42,23 +48,6 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val);
 
 #else
 
-static inline int intel_pmc_ipc_simple_command(int cmd, int sub)
-{
-   return -EINVAL;
-}
-
-static inline int intel_pmc_ipc_raw_cmd(u32 cmd, u32 sub, u8 *in, u32 inlen,
-   u32 *out, u32 outlen, u32 dptr, u32 sptr)
-{
-   return -EINVAL;
-}
-
-static inline int intel_pmc_ipc_command(u32 cmd, u32 sub, u8 *in, u32 inlen,
-   u32 *out, u32 outlen)
-{
-   return -EINVAL;
-}
-
 static inline int intel_pmc_s0ix_counter_read(u64 *data)
 {
return -EINVAL;
diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c 
b/drivers/mfd/intel_soc_pmic_bxtwc.c
index 15bc052..368cbe2 100644
--- a/drivers/mfd/intel_soc_pmic_bxtwc.c
+++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
@@ -268,9 +268,11 @@ static int regmap_ipc_byte_reg_read(void *context, 
unsigned int reg,
 {
int ret;
int i2c_addr;
-   u8 ipc_in[2];
-   u8 ipc_out[4];
+   u8 ipc_in[2] = {0};
+   u8 ipc_out[4] = {0};
struct intel_soc_pmic *pmic = context;
+   u32 cmd[PMC_PARAM_LEN] = {PMC_IPC_PMIC_ACCESS,
+   PMC_IPC_PMIC_ACCESS_READ};
 
if (!pmic)
return -EINVAL;
@@ -284,9 +286,8 @@ static int regmap_ipc_byte_reg_read(void *context, unsigned 
int reg,
 
ipc_in[0] = reg;
ipc_in[1] = i2c_addr;
-   ret = intel_pmc_ipc_command(PMC_IPC_PMIC_ACCESS,
-   PMC_IPC_PMIC_ACCESS_READ,
-   ipc_in, sizeof(ipc_in), (u32 *)ipc_out, 1);
+   ret = ipc_dev_raw_cmd(pmic->ipc_dev, cmd, PMC_PARAM_LEN, (u32 *)ipc_in,
+   1, (u32 *)ipc_out, 1, 0, 0);
if (ret) {
dev_err(pmic->dev, "Failed to read from PMIC\n");
return ret;
@@ -301,8 +302,10 @@ static int regmap_ipc_byte_reg_write(void *context, 
unsigned int reg,
 {
int ret;
int i2c_addr;
-   u8 ipc_in[3];
+   u8 ipc_in[3] = {0};
struct intel_soc_pmic *pmic = context;
+   u32 cmd[PMC_PARAM_LEN] = {PMC_IPC_PMIC_ACCESS,
+   PMC_IPC_PMIC_ACCESS_WRITE};
 
if (!pmic)
return -EINVAL;
@@ -317,9 +320,8 @@ static int regmap_ipc_byte_reg_write(void *context, 
unsigned int reg,
ipc_in[0] = reg;
ipc_in[1] = i2c_addr;
ipc_in[2] = val;
-   ret = intel_pmc_ipc_command(PMC_IPC_PMIC_ACCESS,
-   PMC_IPC_PMIC_ACCESS_WRITE,
-   ipc_in, sizeof(ipc_in), NULL, 0);
+   ret = ipc_dev_raw_cmd(pmic->ipc_dev, cmd, PMC_PARAM_LEN, (u32 *)ipc_in,
+   1,

[RFC v2 6/6] platform/x86: intel_pmc_ipc: Use generic Intel IPC device calls

2017-09-02 Thread sathyanarayanan . kuppuswamy 
From: Kuppuswamy Sathyanarayanan 

Removed redundant IPC helper functions and refactored the driver to use
generic IPC device driver APIs.

This patch also cleans-up PMC IPC user drivers to use APIs provided
by generic IPC driver.

Signed-off-by: Kuppuswamy Sathyanarayanan 

---
 arch/x86/include/asm/intel_pmc_ipc.h  |  37 +--
 drivers/mfd/intel_soc_pmic_bxtwc.c|  24 +-
 drivers/platform/x86/intel_pmc_ipc.c  | 364 +-
 drivers/platform/x86/intel_telemetry_pltdrv.c | 114 
 include/linux/mfd/intel_soc_pmic.h|   2 +
 5 files changed, 215 insertions(+), 326 deletions(-)

Changes since v1:
 * Removed custom APIs.
 * Cleaned up PMC IPC user drivers to use APIs provided by generic
   IPC driver.

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h 
b/arch/x86/include/asm/intel_pmc_ipc.h
index fac89eb..9fc7c3c 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -1,10 +1,15 @@
 #ifndef _ASM_X86_INTEL_PMC_IPC_H_
 #define  _ASM_X86_INTEL_PMC_IPC_H_
 
+#include 
+
+#define INTEL_PMC_IPC_DEV  "intel_pmc_ipc"
+#define PMC_PARAM_LEN  2
+
 /* Commands */
 #define PMC_IPC_PMIC_ACCESS0xFF
-#definePMC_IPC_PMIC_ACCESS_READ0x0
-#definePMC_IPC_PMIC_ACCESS_WRITE   0x1
+#definePMC_IPC_PMIC_ACCESS_READ0x0
+#definePMC_IPC_PMIC_ACCESS_WRITE   0x1
 #define PMC_IPC_USB_PWR_CTRL   0xF0
 #define PMC_IPC_PMIC_BLACKLIST_SEL 0xEF
 #define PMC_IPC_PHY_CONFIG 0xEE
@@ -28,13 +33,14 @@
 #define PMC_GCR_TELEM_DEEP_S0IX_REG0x78
 #define PMC_GCR_TELEM_SHLW_S0IX_REG0x80
 
+static inline void pmc_cmd_init(u32 *cmd, u32 param1, u32 param2)
+{
+   cmd[0] = param1;
+   cmd[1] = param2;
+}
+
 #if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
 
-int intel_pmc_ipc_simple_command(int cmd, int sub);
-int intel_pmc_ipc_raw_cmd(u32 cmd, u32 sub, u8 *in, u32 inlen,
-   u32 *out, u32 outlen, u32 dptr, u32 sptr);
-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);
@@ -42,23 +48,6 @@ int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val);
 
 #else
 
-static inline int intel_pmc_ipc_simple_command(int cmd, int sub)
-{
-   return -EINVAL;
-}
-
-static inline int intel_pmc_ipc_raw_cmd(u32 cmd, u32 sub, u8 *in, u32 inlen,
-   u32 *out, u32 outlen, u32 dptr, u32 sptr)
-{
-   return -EINVAL;
-}
-
-static inline int intel_pmc_ipc_command(u32 cmd, u32 sub, u8 *in, u32 inlen,
-   u32 *out, u32 outlen)
-{
-   return -EINVAL;
-}
-
 static inline int intel_pmc_s0ix_counter_read(u64 *data)
 {
return -EINVAL;
diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c 
b/drivers/mfd/intel_soc_pmic_bxtwc.c
index 15bc052..368cbe2 100644
--- a/drivers/mfd/intel_soc_pmic_bxtwc.c
+++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
@@ -268,9 +268,11 @@ static int regmap_ipc_byte_reg_read(void *context, 
unsigned int reg,
 {
int ret;
int i2c_addr;
-   u8 ipc_in[2];
-   u8 ipc_out[4];
+   u8 ipc_in[2] = {0};
+   u8 ipc_out[4] = {0};
struct intel_soc_pmic *pmic = context;
+   u32 cmd[PMC_PARAM_LEN] = {PMC_IPC_PMIC_ACCESS,
+   PMC_IPC_PMIC_ACCESS_READ};
 
if (!pmic)
return -EINVAL;
@@ -284,9 +286,8 @@ static int regmap_ipc_byte_reg_read(void *context, unsigned 
int reg,
 
ipc_in[0] = reg;
ipc_in[1] = i2c_addr;
-   ret = intel_pmc_ipc_command(PMC_IPC_PMIC_ACCESS,
-   PMC_IPC_PMIC_ACCESS_READ,
-   ipc_in, sizeof(ipc_in), (u32 *)ipc_out, 1);
+   ret = ipc_dev_raw_cmd(pmic->ipc_dev, cmd, PMC_PARAM_LEN, (u32 *)ipc_in,
+   1, (u32 *)ipc_out, 1, 0, 0);
if (ret) {
dev_err(pmic->dev, "Failed to read from PMIC\n");
return ret;
@@ -301,8 +302,10 @@ static int regmap_ipc_byte_reg_write(void *context, 
unsigned int reg,
 {
int ret;
int i2c_addr;
-   u8 ipc_in[3];
+   u8 ipc_in[3] = {0};
struct intel_soc_pmic *pmic = context;
+   u32 cmd[PMC_PARAM_LEN] = {PMC_IPC_PMIC_ACCESS,
+   PMC_IPC_PMIC_ACCESS_WRITE};
 
if (!pmic)
return -EINVAL;
@@ -317,9 +320,8 @@ static int regmap_ipc_byte_reg_write(void *context, 
unsigned int reg,
ipc_in[0] = reg;
ipc_in[1] = i2c_addr;
ipc_in[2] = val;
-   ret = intel_pmc_ipc_command(PMC_IPC_PMIC_ACCESS,
-   PMC_IPC_PMIC_ACCESS_WRITE,
-   ipc_in, sizeof(ipc_in), NULL, 0);
+   ret = ipc_dev_raw_cmd(pmic->ipc_dev, cmd, PMC_PARAM_LEN, (u32 *)ipc_in,
+   1, NULL, 0, 0, 0);
if (ret) {
dev_err(pmic->dev, "Failed to write to