All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops
@ 2016-10-06  6:24 Lukas Wunner
  2016-10-06  6:24 ` [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook Lukas Wunner
  2016-10-07 20:55 ` [PATCH 0/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops Andy Shevchenko
  0 siblings, 2 replies; 11+ messages in thread
From: Lukas Wunner @ 2016-10-06  6:24 UTC (permalink / raw)
  To: Andy Shevchenko, Bjorn Helgaas; +Cc: linux-pci, x86

Somehow it went completely under my radar that v4.8 added another
struct pci_platform_pm_ops for Intel Mobile Internet Devices.

There's a commit queued for v4.9 on Bjorn's pci/pm branch which adds
a mandatory ->get_power hook to pci_platform_pm_ops and implements
it for acpi_pci_platform_pm, but not mid_pci_platform_pm.

The following patch fixes that.  It's not a build issue, the missing
hook in mid_pci_platform_pm only becomes a problem at runtime.

@Andy: Could you look over this and provide an ack?  I do not have
an Intel MID so I was only able to compile-test the patch.

The ->get_power hook is currently only called to update a device's
current_state after resume (both at runtime and after system sleep)
and after changing its power state using the platform in
pci_platform_power_transition().  The result of the ->get_power hook
is ignored unless it's D3cold.  Since intel-mid PCI devices can only
be suspended to D3hot, the return value is irrelevant on this platform,
the hook just needs to be present to make pci_set_platform_pm() happy.

If you want to test the patch, you need to apply it either on Bjorn's
pci/pm branch or linux-next:
http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm

Thanks,

Lukas

Lukas Wunner (1):
  x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook

 arch/x86/include/asm/intel-mid.h  |  1 +
 arch/x86/platform/intel-mid/pwr.c | 19 +++++++++++++++++++
 drivers/pci/pci-mid.c             |  6 ++++++
 3 files changed, 26 insertions(+)

-- 
2.9.3


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook
  2016-10-06  6:24 [PATCH 0/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops Lukas Wunner
@ 2016-10-06  6:24 ` Lukas Wunner
  2016-10-07 20:55   ` Andy Shevchenko
  2016-10-07 20:55 ` [PATCH 0/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops Andy Shevchenko
  1 sibling, 1 reply; 11+ messages in thread
From: Lukas Wunner @ 2016-10-06  6:24 UTC (permalink / raw)
  To: Andy Shevchenko, Bjorn Helgaas; +Cc: linux-pci, x86

Commit cc7cc02bada8 ("PCI: Query platform firmware for device power
state") augmented struct pci_platform_pm_ops with a ->get_power hook and
implemented it for acpi_pci_platform_pm, the only pci_platform_pm_ops
existing till v4.7.

However v4.8 introduced another pci_platform_pm_ops for Intel Mobile
Internet Devices with commit 5823d0893ec2 ("x86/platform/intel-mid: Add
Power Management Unit driver").  It is missing the ->get_power hook,
which is fatal since pci_set_platform_pm() enforces its presence.

Retrofit mid_pci_platform_pm with the missing callback to fix the
breakage.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: x86@kernel.org
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 arch/x86/include/asm/intel-mid.h  |  1 +
 arch/x86/platform/intel-mid/pwr.c | 19 +++++++++++++++++++
 drivers/pci/pci-mid.c             |  6 ++++++
 3 files changed, 26 insertions(+)

diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h
index 9d6b097..4511c10 100644
--- a/arch/x86/include/asm/intel-mid.h
+++ b/arch/x86/include/asm/intel-mid.h
@@ -17,6 +17,7 @@
 
 extern int intel_mid_pci_init(void);
 extern int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state);
+extern pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev);
 
 #define INTEL_MID_PWR_LSS_OFFSET	4
 #define INTEL_MID_PWR_LSS_TYPE		(1 << 7)
diff --git a/arch/x86/platform/intel-mid/pwr.c b/arch/x86/platform/intel-mid/pwr.c
index c901a34..214bc99 100644
--- a/arch/x86/platform/intel-mid/pwr.c
+++ b/arch/x86/platform/intel-mid/pwr.c
@@ -260,6 +260,25 @@ int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state)
 }
 EXPORT_SYMBOL_GPL(intel_mid_pci_set_power_state);
 
+pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev)
+{
+	struct mid_pwr *pwr = midpwr;
+	int id, reg, bit;
+	u32 power;
+
+	if (!pwr || !pwr->available)
+		return PCI_UNKNOWN;
+
+	id = intel_mid_pwr_get_lss_id(pdev);
+	if (id < 0)
+		return PCI_UNKNOWN;
+
+	reg = (id * LSS_PWS_BITS) / 32;
+	bit = (id * LSS_PWS_BITS) % 32;
+	power = mid_pwr_get_state(pwr, reg);
+	return (power >> bit) & 3;
+}
+
 int intel_mid_pwr_get_lss_id(struct pci_dev *pdev)
 {
 	int vndr;
diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c
index c878aa7..a8b52dc 100644
--- a/drivers/pci/pci-mid.c
+++ b/drivers/pci/pci-mid.c
@@ -29,6 +29,11 @@ static int mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state)
 	return intel_mid_pci_set_power_state(pdev, state);
 }
 
+static pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
+{
+	return intel_mid_pci_get_power_state(pdev);
+}
+
 static pci_power_t mid_pci_choose_state(struct pci_dev *pdev)
 {
 	return PCI_D3hot;
@@ -52,6 +57,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev)
 static struct pci_platform_pm_ops mid_pci_platform_pm = {
 	.is_manageable	= mid_pci_power_manageable,
 	.set_state	= mid_pci_set_power_state,
+	.get_state	= mid_pci_get_power_state,
 	.choose_state	= mid_pci_choose_state,
 	.sleep_wake	= mid_pci_sleep_wake,
 	.run_wake	= mid_pci_run_wake,
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook
  2016-10-06  6:24 ` [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook Lukas Wunner
@ 2016-10-07 20:55   ` Andy Shevchenko
  2016-10-08 13:49     ` Lukas Wunner
  2016-10-09 10:46     ` Lukas Wunner
  0 siblings, 2 replies; 11+ messages in thread
From: Andy Shevchenko @ 2016-10-07 20:55 UTC (permalink / raw)
  To: Lukas Wunner, Bjorn Helgaas; +Cc: linux-pci, x86

On Thu, 2016-10-06 at 08:24 +0200, Lukas Wunner wrote:
> Commit cc7cc02bada8 ("PCI: Query platform firmware for device power
> state") augmented struct pci_platform_pm_ops with a ->get_power hook
> and
> implemented it for acpi_pci_platform_pm, the only pci_platform_pm_ops
> existing till v4.7.
> 
> However v4.8 introduced another pci_platform_pm_ops for Intel Mobile
> Internet Devices with commit 5823d0893ec2 ("x86/platform/intel-mid:
> Add
> Power Management Unit driver").  It is missing the ->get_power hook,
> which is fatal since pci_set_platform_pm() enforces its presence.
> 
> Retrofit mid_pci_platform_pm with the missing callback to fix the
> breakage.

Check comments below.

> --- a/arch/x86/platform/intel-mid/pwr.c
> +++ b/arch/x86/platform/intel-mid/pwr.c
> @@ -260,6 +260,25 @@ int intel_mid_pci_set_power_state(struct pci_dev
> *pdev, pci_power_t state)
>  }
>  EXPORT_SYMBOL_GPL(intel_mid_pci_set_power_state);
>  
> +pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev)
> +{
> +	struct mid_pwr *pwr = midpwr;
> +	int id, reg, bit;
> +	u32 power;
> +
> +	if (!pwr || !pwr->available)
> +		return PCI_UNKNOWN;

I'm not sure it's a right value. In arch/x86/pci/intel_mid_pci.c we
assign D3hot to PMCSR for all devices. I dunno how to proceed here,
though your case works for me.

> +
> +	id = intel_mid_pwr_get_lss_id(pdev);
> +	if (id < 0)
> +		return PCI_UNKNOWN;

Similar here, not all PCI devices are using PWRMU (or P-Unit, which
support is absent) and might be AON, or be controllable via PMCSR only.

> +
> +	reg = (id * LSS_PWS_BITS) / 32;
> +	bit = (id * LSS_PWS_BITS) % 32;
> +	power = mid_pwr_get_state(pwr, reg);
> +	return (power >> bit) & 3;

Don't add sparse warnings:

        return (__force pci_power_t)((power >> bit) & 3);

> +}

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops
  2016-10-06  6:24 [PATCH 0/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops Lukas Wunner
  2016-10-06  6:24 ` [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook Lukas Wunner
@ 2016-10-07 20:55 ` Andy Shevchenko
  1 sibling, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2016-10-07 20:55 UTC (permalink / raw)
  To: Lukas Wunner, Bjorn Helgaas; +Cc: linux-pci, x86

On Thu, 2016-10-06 at 08:24 +0200, Lukas Wunner wrote:
> Somehow it went completely under my radar that v4.8 added another
> struct pci_platform_pm_ops for Intel Mobile Internet Devices.
> 
> There's a commit queued for v4.9 on Bjorn's pci/pm branch which adds
> a mandatory ->get_power hook to pci_platform_pm_ops and implements
> it for acpi_pci_platform_pm, but not mid_pci_platform_pm.
> 
> The following patch fixes that.  It's not a build issue, the missing
> hook in mid_pci_platform_pm only becomes a problem at runtime.
> 
> @Andy: Could you look over this and provide an ack?  I do not have
> an Intel MID so I was only able to compile-test the patch.

Without this I got nothing but constant reboot.

Acked-and-tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Nevertheless please address my comments.

> The ->get_power hook is currently only called to update a device's
> current_state after resume (both at runtime and after system sleep)
> and after changing its power state using the platform in
> pci_platform_power_transition().  The result of the ->get_power hook
> is ignored unless it's D3cold.  Since intel-mid PCI devices can only
> be suspended to D3hot,

Strictly speaking platform supports it, but we don't use these feature.

>  the return value is irrelevant on this platform,
> the hook just needs to be present to make pci_set_platform_pm() happy.
> 
> If you want to test the patch, you need to apply it either on Bjorn's
> pci/pm branch or linux-next:
> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci
> /pm

I'm using linux-next.


> 
> Thanks,
> 
> Lukas
> 
> Lukas Wunner (1):
>   x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power
> hook
> 
>  arch/x86/include/asm/intel-mid.h  |  1 +
>  arch/x86/platform/intel-mid/pwr.c | 19 +++++++++++++++++++
>  drivers/pci/pci-mid.c             |  6 ++++++
>  3 files changed, 26 insertions(+)
> 

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook
  2016-10-07 20:55   ` Andy Shevchenko
@ 2016-10-08 13:49     ` Lukas Wunner
  2016-10-09 12:26       ` Andy Shevchenko
  2016-10-09 10:46     ` Lukas Wunner
  1 sibling, 1 reply; 11+ messages in thread
From: Lukas Wunner @ 2016-10-08 13:49 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Bjorn Helgaas, linux-pci, x86

Hi Andy,

thanks a lot for testing the patch!

On Fri, Oct 07, 2016 at 11:55:34PM +0300, Andy Shevchenko wrote:
> On Thu, 2016-10-06 at 08:24 +0200, Lukas Wunner wrote:
> > +pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev)
> > +{
> > +	struct mid_pwr *pwr = midpwr;
> > +	int id, reg, bit;
> > +	u32 power;
> > +
> > +	if (!pwr || !pwr->available)
> > +		return PCI_UNKNOWN;
> 
> I'm not sure it's a right value. In arch/x86/pci/intel_mid_pci.c we
> assign D3hot to PMCSR for all devices. I dunno how to proceed here,
> though your case works for me.

Generally returning PCI_UNKNOWN from the ->get_state hook is fine
if the platform fails to determine the state.  The ACPI equivalent
acpi_pci_get_power_state() does this as well.

> 
> > +
> > +	id = intel_mid_pwr_get_lss_id(pdev);
> > +	if (id < 0)
> > +		return PCI_UNKNOWN;
> 
> Similar here, not all PCI devices are using PWRMU (or P-Unit, which
> support is absent) and might be AON, or be controllable via PMCSR only.

Hm, then mid_pci_power_manageable() is broken, you let it return true
unconditionally.  I'm not familiar at all with Intel MID devices, do
you have a way to clearly identify if a PCI device is power managed
by the PWRMU versus PMCSR?  My guess is that at the very least,
intel_mid_pwr_get_lss_id() needs to be called and false needs to
be returned by mid_pci_power_manageable() if the return value is
negative.

As said it's not a problem for the single existing caller of
mid_pci_get_power_state(), which is pci_target_state(), since it
only honors the return value if it's PCI_D3cold. Otherwise it
defers to the PMCSR.  The rationale is that reading the PMCSR is
usually the most reliable way to determine the power state, but
D3cold cannot be determined by reading the PMCSR.

What you say sounds to me like you need to amend the callbacks
in mid_pci_platform_pm to differentiate between PWRMU-manageable
versus non-PWRMU-manageable devices, but that's not specific to
the single ->get_state callback added by this commit and can thus
go into a separate commit.

> 
> > +
> > +	reg = (id * LSS_PWS_BITS) / 32;
> > +	bit = (id * LSS_PWS_BITS) % 32;
> > +	power = mid_pwr_get_state(pwr, reg);
> > +	return (power >> bit) & 3;
> 
> Don't add sparse warnings:
> 
>         return (__force pci_power_t)((power >> bit) & 3);

Okay, I'll fix this in v2.

Thanks again,

Lukas

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook
  2016-10-07 20:55   ` Andy Shevchenko
  2016-10-08 13:49     ` Lukas Wunner
@ 2016-10-09 10:46     ` Lukas Wunner
  2016-10-09 11:57       ` Andy Shevchenko
  1 sibling, 1 reply; 11+ messages in thread
From: Lukas Wunner @ 2016-10-09 10:46 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Bjorn Helgaas, linux-pci, x86

On Fri, Oct 07, 2016 at 11:55:34PM +0300, Andy Shevchenko wrote:
> On Thu, 2016-10-06 at 08:24 +0200, Lukas Wunner wrote:
> > +
> > +	reg = (id * LSS_PWS_BITS) / 32;
> > +	bit = (id * LSS_PWS_BITS) % 32;
> > +	power = mid_pwr_get_state(pwr, reg);
> > +	return (power >> bit) & 3;
> 
> Don't add sparse warnings:
> 
>         return (__force pci_power_t)((power >> bit) & 3);

I do not get any different sparse warnings with or without the cast
despite using -Wsparse-all.  This is with sparse 0.5.0 as included in
Debian stretch.

With which options and sparse version did you manage to get new warnings?

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook
  2016-10-09 10:46     ` Lukas Wunner
@ 2016-10-09 11:57       ` Andy Shevchenko
  2016-10-09 12:49         ` Lukas Wunner
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2016-10-09 11:57 UTC (permalink / raw)
  To: Lukas Wunner; +Cc: Bjorn Helgaas, linux-pci, x86

On Sun, 2016-10-09 at 12:46 +0200, Lukas Wunner wrote:
> On Fri, Oct 07, 2016 at 11:55:34PM +0300, Andy Shevchenko wrote:
> > 
> > On Thu, 2016-10-06 at 08:24 +0200, Lukas Wunner wrote:
> > > 
> > > +
> > > +	reg = (id * LSS_PWS_BITS) / 32;
> > > +	bit = (id * LSS_PWS_BITS) % 32;
> > > +	power = mid_pwr_get_state(pwr, reg);
> > > +	return (power >> bit) & 3;
> > 
> > Don't add sparse warnings:
> > 
> >         return (__force pci_power_t)((power >> bit) & 3);
> 
> I do not get any different sparse warnings with or without the cast
> despite using -Wsparse-all.  This is with sparse 0.5.0 as included in
> Debian stretch.
> 
> With which options and sparse version did you manage to get new
> warnings?

$ sparse --version
v0.5.0

$ make C=1 CF=-D__CHECK_ENDIAN__ W=1 -j64 

Warning itself:

arch/x86/platform/intel-mid/pwr.c:305:31: warning: incorrect type in
return expression (different base types)
arch/x86/platform/intel-mid/pwr.c:305:31:    expected restricted
pci_power_t
arch/x86/platform/intel-mid/pwr.c:305:31:    got unsigned int


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook
  2016-10-08 13:49     ` Lukas Wunner
@ 2016-10-09 12:26       ` Andy Shevchenko
  2016-10-09 15:03         ` Lukas Wunner
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2016-10-09 12:26 UTC (permalink / raw)
  To: Lukas Wunner; +Cc: Bjorn Helgaas, linux-pci, x86

On Sat, 2016-10-08 at 15:49 +0200, Lukas Wunner wrote:
> Hi Andy,
> 
> thanks a lot for testing the patch!
> 
> On Fri, Oct 07, 2016 at 11:55:34PM +0300, Andy Shevchenko wrote:
> > 
> > On Thu, 2016-10-06 at 08:24 +0200, Lukas Wunner wrote:
> > > 
> > > +pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev)
> > > +{
> > > +	struct mid_pwr *pwr = midpwr;
> > > +	int id, reg, bit;
> > > +	u32 power;
> > > +
> > > +	if (!pwr || !pwr->available)
> > > +		return PCI_UNKNOWN;
> > 
> > I'm not sure it's a right value. In arch/x86/pci/intel_mid_pci.c we
> > assign D3hot to PMCSR for all devices. I dunno how to proceed here,
> > though your case works for me.
> 
> Generally returning PCI_UNKNOWN from the ->get_state hook is fine
> if the platform fails to determine the state.  The ACPI equivalent
> acpi_pci_get_power_state() does this as well.
> 
> > 
> > 
> > > 
> > > +
> > > +	id = intel_mid_pwr_get_lss_id(pdev);
> > > +	if (id < 0)
> > > +		return PCI_UNKNOWN;
> > 
> > Similar here, not all PCI devices are using PWRMU (or P-Unit, which
> > support is absent) and might be AON, or be controllable via PMCSR
> > only.
> 
> Hm, then mid_pci_power_manageable() is broken, you let it return true
> unconditionally.

Till now it works for me and others who tested my code / patches, I
would say "might be broken". I don't have [access to] all possible
documentation that shed a light on this, so, let's stick on your version
for now until some bug appears.

>   I'm not familiar at all with Intel MID devices, do
> you have a way to clearly identify if a PCI device is power managed
> by the PWRMU versus PMCSR?  My guess is that at the very least,
> intel_mid_pwr_get_lss_id() needs to be called and false needs to
> be returned by mid_pci_power_manageable() if the return value is
> negative.

PCI bus is kinda fake on those platforms (which implement SFI) and don't
always follow [PCI] specification. The vendor register represents so
called Logical Subsystem ID of the device in question. Some of them are
related to PWRMU, some to P-Unit, some just has no such register.

In PWRMU/P-Unit cases PMCSR is present and writing to it is needed.
For the rest I don't remember what is actual state, perhaps writing is
just ignored and OS reads D0 all the time from it.

And I don't remember if P-Unit manageable devices require anything else
than plain update of PMCSR.

> As said it's not a problem for the single existing caller of
> mid_pci_get_power_state(), which is pci_target_state(), since it
> only honors the return value if it's PCI_D3cold. Otherwise it
> defers to the PMCSR.  The rationale is that reading the PMCSR is
> usually the most reliable way to determine the power state, but
> D3cold cannot be determined by reading the PMCSR.

Yep.

> 
> What you say sounds to me like you need to amend the callbacks
> in mid_pci_platform_pm to differentiate between PWRMU-manageable
> versus non-PWRMU-manageable devices, but that's not specific to
> the single ->get_state callback added by this commit and can thus
> go into a separate commit.

See above.

So, from PWRMU code the function behaves correctly for now. Whenever P-
Unit support will be added the code will have to be modified
accordingly.

P.S. Btw, Asus Zenfone 2 and Lenovo K80m phones are based on Intel Moorefield which is, AFAIK, latest [widely distributed] successor of Intel MID devices. I dunno if anyone inside or outside Intel ever tried [fresh] upstream kernels there.

> 
> > 
> > 
> > > 
> > > +
> > > +	reg = (id * LSS_PWS_BITS) / 32;
> > > +	bit = (id * LSS_PWS_BITS) % 32;
> > > +	power = mid_pwr_get_state(pwr, reg);
> > > +	return (power >> bit) & 3;
> > 
> > Don't add sparse warnings:
> > 
> >         return (__force pci_power_t)((power >> bit) & 3);
> 
> Okay, I'll fix this in v2.
> 
> Thanks again,
> 
> Lukas

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook
  2016-10-09 11:57       ` Andy Shevchenko
@ 2016-10-09 12:49         ` Lukas Wunner
  0 siblings, 0 replies; 11+ messages in thread
From: Lukas Wunner @ 2016-10-09 12:49 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Bjorn Helgaas, linux-pci, x86

On Sun, Oct 09, 2016 at 02:57:29PM +0300, Andy Shevchenko wrote:
> On Sun, 2016-10-09 at 12:46 +0200, Lukas Wunner wrote:
> > On Fri, Oct 07, 2016 at 11:55:34PM +0300, Andy Shevchenko wrote:
> > > 
> > > On Thu, 2016-10-06 at 08:24 +0200, Lukas Wunner wrote:
> > > > 
> > > > +
> > > > +	reg = (id * LSS_PWS_BITS) / 32;
> > > > +	bit = (id * LSS_PWS_BITS) % 32;
> > > > +	power = mid_pwr_get_state(pwr, reg);
> > > > +	return (power >> bit) & 3;
> > > 
> > > Don't add sparse warnings:
> > > 
> > >         return (__force pci_power_t)((power >> bit) & 3);
> > 
> > I do not get any different sparse warnings with or without the cast
> > despite using -Wsparse-all.  This is with sparse 0.5.0 as included in
> > Debian stretch.
> > 
> > With which options and sparse version did you manage to get new
> > warnings?
> 
> $ sparse --version
> v0.5.0
> 
> $ make C=1 CF=-D__CHECK_ENDIAN__ W=1 -j64 
> 
> Warning itself:
> 
> arch/x86/platform/intel-mid/pwr.c:305:31: warning: incorrect type in
> return expression (different base types)
> arch/x86/platform/intel-mid/pwr.c:305:31:    expected restricted
> pci_power_t
> arch/x86/platform/intel-mid/pwr.c:305:31:    got unsigned int

Okay, -D__CHECK_ENDIAN__ was necessary to reproduce the warning.
Thanks for replying so quickly, much appreciated.

Lukas

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook
  2016-10-09 12:26       ` Andy Shevchenko
@ 2016-10-09 15:03         ` Lukas Wunner
  2016-10-10 10:54           ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Lukas Wunner @ 2016-10-09 15:03 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Bjorn Helgaas, linux-pci, x86

On Sun, Oct 09, 2016 at 03:26:37PM +0300, Andy Shevchenko wrote:
> On Sat, 2016-10-08 at 15:49 +0200, Lukas Wunner wrote:
> >   I'm not familiar at all with Intel MID devices, do
> > you have a way to clearly identify if a PCI device is power managed
> > by the PWRMU versus PMCSR?  My guess is that at the very least,
> > intel_mid_pwr_get_lss_id() needs to be called and false needs to
> > be returned by mid_pci_power_manageable() if the return value is
> > negative.
> 
> PCI bus is kinda fake on those platforms (which implement SFI) and don't
> always follow [PCI] specification. The vendor register represents so
> called Logical Subsystem ID of the device in question. Some of them are
> related to PWRMU, some to P-Unit, some just has no such register.
> 
> In PWRMU/P-Unit cases PMCSR is present and writing to it is needed.
> For the rest I don't remember what is actual state, perhaps writing is
> just ignored and OS reads D0 all the time from it.

For devices with PM mechanisms not covered by the standard PCI code
(or ACPI platform code), a common pattern is to assign them a struct
dev_pm_domain using dev_pm_domain_set().  Then on suspend you'd
invoke the PCI bus suspend hook and afterwards ask the PWRMU to
power down.  On resume you'd first power up using the PWRMU, then
call the PCI bus resume hook.  See drivers/gpu/vga/vga_switcheroo.c:
vga_switcheroo_init_domain_pm_ops() for an example.  This would have
been an alternative approach to introducing a new PCI platform for
these devices.  I'm not sure which approach is more suitable, it's
just something that crossed my mind when looking at this.

Best regards,

Lukas

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook
  2016-10-09 15:03         ` Lukas Wunner
@ 2016-10-10 10:54           ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2016-10-10 10:54 UTC (permalink / raw)
  To: Lukas Wunner; +Cc: Bjorn Helgaas, linux-pci, x86

On Sun, 2016-10-09 at 17:03 +0200, Lukas Wunner wrote:
> On Sun, Oct 09, 2016 at 03:26:37PM +0300, Andy Shevchenko wrote:
> > 
> > On Sat, 2016-10-08 at 15:49 +0200, Lukas Wunner wrote:
> > > 
> > >   I'm not familiar at all with Intel MID devices, do
> > > you have a way to clearly identify if a PCI device is power
> > > managed
> > > by the PWRMU versus PMCSR?  My guess is that at the very least,
> > > intel_mid_pwr_get_lss_id() needs to be called and false needs to
> > > be returned by mid_pci_power_manageable() if the return value is
> > > negative.
> > 
> > PCI bus is kinda fake on those platforms (which implement SFI) and
> > don't
> > always follow [PCI] specification. The vendor register represents so
> > called Logical Subsystem ID of the device in question. Some of them
> > are
> > related to PWRMU, some to P-Unit, some just has no such register.
> > 
> > In PWRMU/P-Unit cases PMCSR is present and writing to it is needed.
> > For the rest I don't remember what is actual state, perhaps writing
> > is
> > just ignored and OS reads D0 all the time from it.
> 
> For devices with PM mechanisms not covered by the standard PCI code
> (or ACPI platform code), a common pattern is to assign them a struct
> dev_pm_domain using dev_pm_domain_set().  Then on suspend you'd
> invoke the PCI bus suspend hook and afterwards ask the PWRMU to
> power down.  On resume you'd first power up using the PWRMU, then
> call the PCI bus resume hook.  See drivers/gpu/vga/vga_switcheroo.c:
> vga_switcheroo_init_domain_pm_ops() for an example.  This would have
> been an alternative approach to introducing a new PCI platform for
> these devices.  I'm not sure which approach is more suitable, it's
> just something that crossed my mind when looking at this.

Looks like individual drivers shall decide how to behave, right? We
would actually like to avoid to touch any of the device driver in
question.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-10-10 10:54 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-06  6:24 [PATCH 0/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops Lukas Wunner
2016-10-06  6:24 ` [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook Lukas Wunner
2016-10-07 20:55   ` Andy Shevchenko
2016-10-08 13:49     ` Lukas Wunner
2016-10-09 12:26       ` Andy Shevchenko
2016-10-09 15:03         ` Lukas Wunner
2016-10-10 10:54           ` Andy Shevchenko
2016-10-09 10:46     ` Lukas Wunner
2016-10-09 11:57       ` Andy Shevchenko
2016-10-09 12:49         ` Lukas Wunner
2016-10-07 20:55 ` [PATCH 0/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.