[PATCH v1 1/1] x86/platform/intel-mid: Run PWRMU command immediately

[PATCH v1 1/1] x86/platform/intel-mid: Run PWRMU command immediately

[Andy Shevchenko]

On some firmwares we have to tell how exactly we want the command to be run.
The default case for now is to run it immediately.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/platform/intel-mid/pwr.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/intel-mid/pwr.c b/arch/x86/platform/intel-mid/pwr.c
index c901a34..0548741 100644
--- a/arch/x86/platform/intel-mid/pwr.c
+++ b/arch/x86/platform/intel-mid/pwr.c
@@ -44,6 +44,10 @@
 /* Bits in PM_CMD */
 #define PM_CMD_CMD(x)		((x) << 0)
 #define PM_CMD_IOC		(1 << 8)
+#define PM_CMD_CM_NOP		(0 << 9)
+#define PM_CMD_CM_IMMEDIATE	(1 << 9)
+#define PM_CMD_CM_DELAY		(2 << 9)
+#define PM_CMD_CM_TRIGGER	(3 << 9)
 #define PM_CMD_D3cold		(1 << 21)
 
 /* List of commands */
@@ -137,7 +141,7 @@ static int mid_pwr_wait(struct mid_pwr *pwr)
 
 static int mid_pwr_wait_for_cmd(struct mid_pwr *pwr, u8 cmd)
 {
-	writel(PM_CMD_CMD(cmd), pwr->regs + PM_CMD);
+	writel(PM_CMD_CMD(cmd) | PM_CMD_CM_IMMEDIATE, pwr->regs + PM_CMD);
 	return mid_pwr_wait(pwr);
 }
 
-- 
2.8.1

Re: [PATCH v1 1/1] x86/platform/intel-mid: Run PWRMU command immediately

[Ingo Molnar]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On some firmwares we have to tell how exactly we want the command to be run.
> The default case for now is to run it immediately.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/platform/intel-mid/pwr.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/intel-mid/pwr.c b/arch/x86/platform/intel-mid/pwr.c
> index c901a34..0548741 100644
> --- a/arch/x86/platform/intel-mid/pwr.c
> +++ b/arch/x86/platform/intel-mid/pwr.c
> @@ -44,6 +44,10 @@
>  /* Bits in PM_CMD */
>  #define PM_CMD_CMD(x)		((x) << 0)
>  #define PM_CMD_IOC		(1 << 8)
> +#define PM_CMD_CM_NOP		(0 << 9)
> +#define PM_CMD_CM_IMMEDIATE	(1 << 9)
> +#define PM_CMD_CM_DELAY		(2 << 9)
> +#define PM_CMD_CM_TRIGGER	(3 << 9)
>  #define PM_CMD_D3cold		(1 << 21)
>  
>  /* List of commands */
> @@ -137,7 +141,7 @@ static int mid_pwr_wait(struct mid_pwr *pwr)
>  
>  static int mid_pwr_wait_for_cmd(struct mid_pwr *pwr, u8 cmd)
>  {
> -	writel(PM_CMD_CMD(cmd), pwr->regs + PM_CMD);
> +	writel(PM_CMD_CMD(cmd) | PM_CMD_CM_IMMEDIATE, pwr->regs + PM_CMD);
>  	return mid_pwr_wait(pwr);
>  }

Does this fix a bug? If yes then please also add that to the changelog: what are 
the symptoms of the bug - how does a user notice, etc.

Thanks,

	Ingo

Re: [PATCH v1 1/1] x86/platform/intel-mid: Run PWRMU command immediately

[Andy Shevchenko]

On Thu, 2016-08-18 at 12:52 +0200, Ingo Molnar wrote:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > 
> > On some firmwares we have to tell how exactly we want the command to
> > be run.
> > The default case for now is to run it immediately.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  arch/x86/platform/intel-mid/pwr.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/platform/intel-mid/pwr.c
> > b/arch/x86/platform/intel-mid/pwr.c
> > index c901a34..0548741 100644
> > --- a/arch/x86/platform/intel-mid/pwr.c
> > +++ b/arch/x86/platform/intel-mid/pwr.c
> > @@ -44,6 +44,10 @@
> >  /* Bits in PM_CMD */
> >  #define PM_CMD_CMD(x)		((x) << 0)
> >  #define PM_CMD_IOC		(1 << 8)
> > +#define PM_CMD_CM_NOP		(0 << 9)
> > +#define PM_CMD_CM_IMMEDIATE	(1 << 9)
> > +#define PM_CMD_CM_DELAY		(2 << 9)
> > +#define PM_CMD_CM_TRIGGER	(3 << 9)
> >  #define PM_CMD_D3cold		(1 << 21)
> >  
> >  /* List of commands */
> > @@ -137,7 +141,7 @@ static int mid_pwr_wait(struct mid_pwr *pwr)
> >  
> >  static int mid_pwr_wait_for_cmd(struct mid_pwr *pwr, u8 cmd)
> >  {
> > -	writel(PM_CMD_CMD(cmd), pwr->regs + PM_CMD);
> > +	writel(PM_CMD_CMD(cmd) | PM_CMD_CM_IMMEDIATE, pwr->regs +
> > PM_CMD);
> >  	return mid_pwr_wait(pwr);
> >  }
> 
> Does this fix a bug? If yes then please also add that to the
> changelog: what are 
> the symptoms of the bug - how does a user notice, etc.

Unfortunately I have no firmware (I have knowledge of) to test this. On
the board I have, i.e. Intel Edison, everything works either way. On the
other hand the official BSP code has magic number 0x2201 to set, where
bits [15:13] indeed has no meaning to firmware, but the rest is
meaningful. So, I could conclude it *might* fix a bug.

[15:13] MODE_ID
Numeric ID associated with the given mode from an OSPM perspective.
Value not interpreted by firmware. Upon successful completion of this
command, this value should be reflected in the PM_STS.MODE_ID field

Taking above to the consideration what would you advise me?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/1] x86/platform/intel-mid: Run PWRMU command immediately

[Ingo Molnar]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, 2016-08-18 at 12:52 +0200, Ingo Molnar wrote:
> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > 
> > > 
> > > On some firmwares we have to tell how exactly we want the command to
> > > be run.
> > > The default case for now is to run it immediately.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  arch/x86/platform/intel-mid/pwr.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/platform/intel-mid/pwr.c
> > > b/arch/x86/platform/intel-mid/pwr.c
> > > index c901a34..0548741 100644
> > > --- a/arch/x86/platform/intel-mid/pwr.c
> > > +++ b/arch/x86/platform/intel-mid/pwr.c
> > > @@ -44,6 +44,10 @@
> > >  /* Bits in PM_CMD */
> > >  #define PM_CMD_CMD(x)		((x) << 0)
> > >  #define PM_CMD_IOC		(1 << 8)
> > > +#define PM_CMD_CM_NOP		(0 << 9)
> > > +#define PM_CMD_CM_IMMEDIATE	(1 << 9)
> > > +#define PM_CMD_CM_DELAY		(2 << 9)
> > > +#define PM_CMD_CM_TRIGGER	(3 << 9)
> > >  #define PM_CMD_D3cold		(1 << 21)
> > >  
> > >  /* List of commands */
> > > @@ -137,7 +141,7 @@ static int mid_pwr_wait(struct mid_pwr *pwr)
> > >  
> > >  static int mid_pwr_wait_for_cmd(struct mid_pwr *pwr, u8 cmd)
> > >  {
> > > -	writel(PM_CMD_CMD(cmd), pwr->regs + PM_CMD);
> > > +	writel(PM_CMD_CMD(cmd) | PM_CMD_CM_IMMEDIATE, pwr->regs +
> > > PM_CMD);
> > >  	return mid_pwr_wait(pwr);
> > >  }
> > 
> > Does this fix a bug? If yes then please also add that to the
> > changelog: what are 
> > the symptoms of the bug - how does a user notice, etc.
> 
> Unfortunately I have no firmware (I have knowledge of) to test this. On
> the board I have, i.e. Intel Edison, everything works either way. On the
> other hand the official BSP code has magic number 0x2201 to set, where
> bits [15:13] indeed has no meaning to firmware, but the rest is
> meaningful. So, I could conclude it *might* fix a bug.
> 
> [15:13] MODE_ID
> Numeric ID associated with the given mode from an OSPM perspective.
> Value not interpreted by firmware. Upon successful completion of this
> command, this value should be reflected in the PM_STS.MODE_ID field
> 
> Taking above to the consideration what would you advise me?

"This appears to be a safer approach based on the documentation." is good enough 
justification, IMHO. So if you update the changelog with this information it's 
fine to me!

Thanks,

	Ingo