[RFC 2/3] Runtime PM support for named power states

[RFC 2/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6906 bytes --]

This patch adds support for named power states to the PCI core and to the 
PCI USB host controller drivers.  Although the PCI power states will be 
displayed okay for other types of PCI devices, using them for state 
changes won't work -- so don't try it!

Note also that a bug in the EHCI driver will prevent you from suspending
it again immediately after resuming.  ehci_pci_resume calls ehci_run,
which sets hcd->state to HC_STATE_RUNNING, thereby making
usb_hcd_pci_suspend think the root hub is running when it isn't (or at
least, it shouldn't be).

Alan Stern



Index: usb-2.6/drivers/pci/probe.c
===================================================================
--- usb-2.6.orig/drivers/pci/probe.c
+++ usb-2.6/drivers/pci/probe.c
@@ -22,6 +22,54 @@ EXPORT_SYMBOL(pci_root_buses);
 
 LIST_HEAD(pci_devices);
 
+const char pci_name_D0[] = "D0";
+const char pci_name_D1[] = "D1";
+const char pci_name_D2[] = "D2";
+const char pci_name_D3[] = "D3";
+EXPORT_SYMBOL_GPL(pci_name_D0);
+EXPORT_SYMBOL_GPL(pci_name_D1);
+EXPORT_SYMBOL_GPL(pci_name_D2);
+EXPORT_SYMBOL_GPL(pci_name_D3);
+
+
+/**
+ * pci_name_to_state - pointer-based state name to state conversion
+ * @name: state name pointer to convert
+ *
+ * Note: The name string itself is not examined, only the pointer value.
+ * Hence @name must point to one of the four canonical strings above.
+ */
+pci_power_t pci_name_to_state(const char *name)
+{
+	if (name == pci_name_D0)
+		return PCI_D0;
+	else if (name == pci_name_D1)
+		return PCI_D1;
+	else if (name == pci_name_D2)
+		return PCI_D2;
+	else if (name == pci_name_D3)
+		return PCI_D3hot;
+	return PCI_POWER_ERROR;
+}
+EXPORT_SYMBOL_GPL(pci_name_to_state);
+
+/**
+ * pci_state_to_name -- return a static name string for a power state
+ * @state: power state to convert
+ */
+const char *pci_state_to_name(pci_power_t state)
+{
+	static const char *(names[]) = {
+		pci_name_D0, pci_name_D1, pci_name_D2,
+		"D3hot", "D3cold", "Unknown"};
+	int i = (int __force) state;
+
+	if (i < 0 || i >= sizeof(names) / sizeof(names[0]))
+		return "Error";
+	return names[i];
+}
+EXPORT_SYMBOL_GPL(pci_state_to_name);
+
 #ifdef HAVE_PCI_LEGACY
 /**
  * pci_create_legacy_files - create legacy I/O port and memory files
@@ -577,6 +625,42 @@ static void pci_read_irq(struct pci_dev 
 }
 
 /**
+ * pci_setup_device_pm - fill in the PM information of a device
+ * @dev: the device structure to fill
+ *
+ * Initialize the wakeup capability flag and the list of supported states.
+ */
+static void pci_setup_device_pm(struct pci_dev * dev)
+{
+	int pm;
+	u16 pmc;
+	int i = 0;
+
+	dev->states[i] = pci_name_D0;
+
+	/* PCI PM capable devices may be able to issue PME# (wakeup) */
+	pm = pci_find_capability(dev, PCI_CAP_ID_PM);
+	if (pm) {
+		pci_read_config_word(dev, pm + PCI_PM_PMC, &pmc);
+		if (pmc & PCI_PM_CAP_PME_MASK)
+			device_init_wakeup(&dev->dev, 1);
+
+		/* REVISIT: if (pm & PCI_PM_CAP_PME_D3cold) then
+		 * pci pm spec 1.2, section 3.2.4 says we should
+		 * init PCI_PM_CTRL_PME_{STATUS,ENABLE} ...
+		 */
+
+		if (pmc & PCI_PM_CAP_D1)
+			dev->states[++i] = pci_name_D1;
+		if (pmc & PCI_PM_CAP_D2)
+			dev->states[++i] = pci_name_D2;
+		dev->states[++i] = pci_name_D3;
+	}
+	dev->dev.power.suspend_name = dev->states[i];
+	dev->dev.power.states = dev->states;
+}
+
+/**
  * pci_setup_device - fill in class and map information of a device
  * @dev: the device structure to fill
  *
@@ -589,7 +673,6 @@ static void pci_read_irq(struct pci_dev 
 static int pci_setup_device(struct pci_dev * dev)
 {
 	u32 class;
-	u16 pm;
 
 	sprintf(pci_name(dev), "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus),
 		dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
@@ -617,19 +700,7 @@ static int pci_setup_device(struct pci_d
 		pci_read_bases(dev, 6, PCI_ROM_ADDRESS);
 		pci_read_config_word(dev, PCI_SUBSYSTEM_VENDOR_ID, &dev->subsystem_vendor);
 		pci_read_config_word(dev, PCI_SUBSYSTEM_ID, &dev->subsystem_device);
-
-		/* PCI PM capable devices may be able to issue PME# (wakeup) */
-		pm = pci_find_capability(dev, PCI_CAP_ID_PM);
-		if (pm) {
-			pci_read_config_word(dev, pm + PCI_PM_PMC, &pm);
-			if (pm & PCI_PM_CAP_PME_MASK)
-				device_init_wakeup(&dev->dev, 1);
-
-			/* REVISIT: if (pm & PCI_PM_CAP_PME_D3cold) then
-			 * pci pm spec 1.2, section 3.2.4 says we should
-			 * init PCI_PM_CTRL_PME_{STATUS,ENABLE} ...
-			 */
-		}
+		pci_setup_device_pm(dev);
 		break;
 
 	case PCI_HEADER_TYPE_BRIDGE:		    /* bridge header */
Index: usb-2.6/include/linux/pci.h
===================================================================
--- usb-2.6.orig/include/linux/pci.h
+++ usb-2.6/include/linux/pci.h
@@ -78,6 +78,14 @@ typedef int __bitwise pci_power_t;
 #define PCI_UNKNOWN	((pci_power_t __force) 5)
 #define PCI_POWER_ERROR	((pci_power_t __force) -1)
 
+extern const char pci_name_D0[];
+extern const char pci_name_D1[];
+extern const char pci_name_D2[];
+extern const char pci_name_D3[];
+
+extern pci_power_t pci_name_to_state(const char *name);
+extern const char *pci_state_to_name(pci_power_t state);
+
 /*
  * The pci_dev structure is used to describe PCI devices.
  */
@@ -109,6 +117,7 @@ struct pci_dev {
 	pci_power_t     current_state;  /* Current operating state. In ACPI-speak,
 					   this is D0-D3, D0 being fully functional,
 					   and D3 being off. */
+	const char	*(states[5]);	/* Names for supported states */
 
 	struct	device	dev;		/* Generic device interface */
 
Index: usb-2.6/drivers/usb/core/hcd-pci.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/hcd-pci.c
+++ usb-2.6/drivers/usb/core/hcd-pci.c
@@ -196,6 +196,7 @@ int usb_hcd_pci_suspend (struct pci_dev 
 	struct usb_hcd		*hcd;
 	int			retval = 0;
 	int			has_pci_pm;
+	pci_power_t		level = PCI_D3hot;
 
 	hcd = pci_get_drvdata(dev);
 
@@ -261,19 +262,23 @@ int usb_hcd_pci_suspend (struct pci_dev 
 		 * PCI_D3 (but not PCI_D1 or PCI_D2) is allowed to reset
 		 * some device state (e.g. as part of clock reinit).
 		 */
-		retval = pci_set_power_state (dev, PCI_D3hot);
+		if (message.event == PM_EVENT_RUNTIME)
+			level = pci_name_to_state (message.name);
+		retval = pci_set_power_state (dev, level);
 		if (retval == 0) {
-			dev_dbg (hcd->self.controller, "--> PCI D3\n");
+			dev_dbg (hcd->self.controller, "--> PCI %s\n",
+					pci_state_to_name (level));
 
 			/* Ignore these return values.  We rely on pci code to
 			 * reject requests the hardware can't implement, rather
 			 * than coding the same thing.
 			 */
+			/* FIXME: Do we want these for runtime PM? */
 			(void) pci_enable_wake (dev, PCI_D3hot, hcd->remote_wakeup);
 			(void) pci_enable_wake (dev, PCI_D3cold, hcd->remote_wakeup);
 		} else {
-			dev_dbg (&dev->dev, "PCI D3 suspend fail, %d\n",
-					retval);
+			dev_dbg (&dev->dev, "PCI %s suspend fail, %d\n",
+					pci_state_to_name (level), retval);
 			(void) usb_hcd_pci_resume (dev);
 		}
 


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 2/3] Runtime PM support for named power states

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]

Hi!

> Index: usb-2.6/drivers/pci/probe.c
> ===================================================================
> --- usb-2.6.orig/drivers/pci/probe.c
> +++ usb-2.6/drivers/pci/probe.c
> @@ -22,6 +22,54 @@ EXPORT_SYMBOL(pci_root_buses);
>  
>  LIST_HEAD(pci_devices);
>  
> +const char pci_name_D0[] = "D0";
> +const char pci_name_D1[] = "D1";
> +const char pci_name_D2[] = "D2";
> +const char pci_name_D3[] = "D3";

Why not "D3hot"? And what about "D3cold"?

> +/**
> + * pci_state_to_name -- return a static name string for a power state
> + * @state: power state to convert
> + */
> +const char *pci_state_to_name(pci_power_t state)
> +{
> +	static const char *(names[]) = {
> +		pci_name_D0, pci_name_D1, pci_name_D2,
> +		"D3hot", "D3cold", "Unknown"};

} should go on separate line.

> +	int i = (int __force) state;
> +
> +	if (i < 0 || i >= sizeof(names) / sizeof(names[0]))
> +		return "Error";

Why the "Unknown" in the array, then?

> @@ -109,6 +117,7 @@ struct pci_dev {
>  	pci_power_t     current_state;  /* Current operating state. In ACPI-speak,
>  					   this is D0-D3, D0 being fully functional,
>  					   and D3 being off. */
> +	const char	*(states[5]);	/* Names for supported states */

That constant looks strange here...
								Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 2/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2956 bytes --]

On Sun, 2 Oct 2005, Pavel Machek wrote:

> Hi!
> 
> > Index: usb-2.6/drivers/pci/probe.c
> > ===================================================================
> > --- usb-2.6.orig/drivers/pci/probe.c
> > +++ usb-2.6/drivers/pci/probe.c
> > @@ -22,6 +22,54 @@ EXPORT_SYMBOL(pci_root_buses);
> >  
> >  LIST_HEAD(pci_devices);
> >  
> > +const char pci_name_D0[] = "D0";
> > +const char pci_name_D1[] = "D1";
> > +const char pci_name_D2[] = "D2";
> > +const char pci_name_D3[] = "D3";
> 
> Why not "D3hot"? And what about "D3cold"?

I considered these questions while writing the patch.  Not being entirely 
certain of the correct answers, I just kept things simple for now.

These names refer to device power-states in a running system.  Isn't it 
true that as long as the system is awake, PCI devices can go into D3hot 
but not D3cold?  If it is true, we might as well just call the state "D3".
Note that the pci_set_power_state() routine won't permit states beyond 
D3hot...

On the other hand, it occurred to me later that a more complete set of
names might be useful so that the pci_state_to_name routine could use
them.  And it would be desirable for pci_name_to_state to recognize all
the possible outputs from pci_state_to_name.

Remember, this patch was a first-pass, show-that-it-can-be-done sort of 
thing.

> > +/**
> > + * pci_state_to_name -- return a static name string for a power state
> > + * @state: power state to convert
> > + */
> > +const char *pci_state_to_name(pci_power_t state)
> > +{
> > +	static const char *(names[]) = {
> > +		pci_name_D0, pci_name_D1, pci_name_D2,
> > +		"D3hot", "D3cold", "Unknown"};
> 
> } should go on separate line.

Okay.

> > +	int i = (int __force) state;
> > +
> > +	if (i < 0 || i >= sizeof(names) / sizeof(names[0]))
> > +		return "Error";
> 
> Why the "Unknown" in the array, then?

Because this function looked like it might end up being more generally 
useful for PCI drivers, and the list of all pci_power_t values includes 
PCI_UNKNOWN as well as PCI_POWER_ERROR.

One of the weaknesses of this patch is that the current power state, as
stored in pcidev->dev.power.power_state.name, is not closely tied to the
value stored in pcidev->current_state.


> > @@ -109,6 +117,7 @@ struct pci_dev {
> >  	pci_power_t     current_state;  /* Current operating state. In ACPI-speak,
> >  					   this is D0-D3, D0 being fully functional,
> >  					   and D3 being off. */
> > +	const char	*(states[5]);	/* Names for supported states */
> 
> That constant looks strange here...

It does.  We can change it.  The idea was that this is a null-terminated 
array which might have to hold entries for up to four PCI power states.  
Therefore five array slots are pre-allocated.

Another possible improvement would be to make the new fields dependent 
on CONFIG_PM.  I didn't do it in order to keep the patches simple and to 
avoid adding preprocessor conditionals in the middle of subroutines.

Alan Stern


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 2/3] Runtime PM support for named power states

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 3375 bytes --]

In the PCI init:

> @@ -589,7 +673,6 @@ static void pci_read_irq(struct pci_dev 
>  static int pci_setup_device(struct pci_dev * dev)
>  {
>  	u32 class;
> -	u16 pm;
>  
>  	sprintf(pci_name(dev), "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus),
>  		dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> @@ -617,19 +700,7 @@ static int pci_setup_device(struct pci_d
>  		pci_read_bases(dev, 6, PCI_ROM_ADDRESS);
>  		pci_read_config_word(dev, PCI_SUBSYSTEM_VENDOR_ID, &dev->subsystem_vendor);
>  		pci_read_config_word(dev, PCI_SUBSYSTEM_ID, &dev->subsystem_device);
> -
> -		/* PCI PM capable devices may be able to issue PME# (wakeup) */
> -		pm = pci_find_capability(dev, PCI_CAP_ID_PM);
> -		if (pm) {
> -			pci_read_config_word(dev, pm + PCI_PM_PMC, &pm);

It's worth noting that Andrew has run into problems with that bit of
the wakeup patch ... it seems that some PPC systems (G5?) get unhappy.

It's not a general "can't read capabilities" issue, since that was done
in pci_cfg_space_size() shortly before pci_setup_device() gets called.

Though it's unclear to me (G5-less) whether that's an issue with that
patch or with some other PPC changes.  There was some mainline breakage
for PPC reported at the same time, which could easily be the same issue;
someone with a G5 should investigate.  (Benjamin?  Or maybe Paul, who
ISTR was reworking PPC support for PCI.)


> -			if (pm & PCI_PM_CAP_PME_MASK)
> -				device_init_wakeup(&dev->dev, 1);
> -
> -			/* REVISIT: if (pm & PCI_PM_CAP_PME_D3cold) then
> -			 * pci pm spec 1.2, section 3.2.4 says we should
> -			 * init PCI_PM_CTRL_PME_{STATUS,ENABLE} ...
> -			 */
> -		}
> +		pci_setup_device_pm(dev);
>  		break;
>  
>  	case PCI_HEADER_TYPE_BRIDGE:		    /* bridge header */


And in the USB PCI glue:

> @@ -261,19 +262,23 @@ int usb_hcd_pci_suspend (struct pci_dev 
>  		 * PCI_D3 (but not PCI_D1 or PCI_D2) is allowed to reset
>  		 * some device state (e.g. as part of clock reinit).
>  		 */
> -		retval = pci_set_power_state (dev, PCI_D3hot);
> +		if (message.event == PM_EVENT_RUNTIME)
> +			level = pci_name_to_state (message.name);
> +		retval = pci_set_power_state (dev, level);
>  		if (retval == 0) {
> -			dev_dbg (hcd->self.controller, "--> PCI D3\n");
> +			dev_dbg (hcd->self.controller, "--> PCI %s\n",
> +					pci_state_to_name (level));
>  
>  			/* Ignore these return values.  We rely on pci code to
>  			 * reject requests the hardware can't implement, rather
>  			 * than coding the same thing.
>  			 */
> +			/* FIXME: Do we want these for runtime PM? */

Actually, the line below should use "level" not "PCI_D3hot", since the
reason to enable wake from D3hot is that D3hot was the target state.

And if the target isn't D3hot, it may not be necessary to enable wake
from D3cold ... the reason to enable wake from D3cold is that it's
not currently knowable if system suspend is going to morph the D3hot
into a D3cold.

IMO it's probably fair to ignore D3cold except if the target is D3hot.

>  			(void) pci_enable_wake (dev, PCI_D3hot, hcd->remote_wakeup);
>  			(void) pci_enable_wake (dev, PCI_D3cold, hcd->remote_wakeup);
>  		} else {
> -			dev_dbg (&dev->dev, "PCI D3 suspend fail, %d\n",
> -					retval);
> +			dev_dbg (&dev->dev, "PCI %s suspend fail, %d\n",
> +					pci_state_to_name (level), retval);
>  			(void) usb_hcd_pci_resume (dev);
>  		}
>  
>


- Dave





[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 2/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 708 bytes --]

On Mon, 3 Oct 2005, David Brownell wrote:

> Actually, the line below should use "level" not "PCI_D3hot", since the
> reason to enable wake from D3hot is that D3hot was the target state.
> 
> And if the target isn't D3hot, it may not be necessary to enable wake
> from D3cold ... the reason to enable wake from D3cold is that it's
> not currently knowable if system suspend is going to morph the D3hot
> into a D3cold.
> 
> IMO it's probably fair to ignore D3cold except if the target is D3hot.

And for now we probably want to ignore the whole thing for runtime 
suspends, since Linux doesn't properly handle PME signals.  Unless the 
system is going to sleep, we should avoid generating them.

Alan Stern


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 2/3] Runtime PM support for named power states

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

> > Actually, the line below should use "level" not "PCI_D3hot", since the
> > reason to enable wake from D3hot is that D3hot was the target state.
> > 
> > And if the target isn't D3hot, it may not be necessary to enable wake
> > from D3cold ... the reason to enable wake from D3cold is that it's
> > not currently knowable if system suspend is going to morph the D3hot
> > into a D3cold.
> > 
> > IMO it's probably fair to ignore D3cold except if the target is D3hot.
>
> And for now we probably want to ignore the whole thing for runtime 
> suspends, since Linux doesn't properly handle PME signals.  Unless the 
> system is going to sleep, we should avoid generating them.

I'd rather avoid creating special cases like that though; leave the
wakeup stuff in and it'll continue to be ignored ... until Linux properly
handles runtime PME# signals.  And if the system enters a sleep state
(like S1 or S3) the "right thing" should just happen...

I don't know how (or whether!) ACPI handles PME# at runtime, but it'd
be trivial to have a timer watch for devices whose config space reports
they're signaling PME#.  Not the prettiest solution; but until ACPI does
better, it could certainly make everything work consistently.

- Dave



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 2/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1624 bytes --]

On Mon, 3 Oct 2005, David Brownell wrote:

> > > Actually, the line below should use "level" not "PCI_D3hot", since the
> > > reason to enable wake from D3hot is that D3hot was the target state.
> > > 
> > > And if the target isn't D3hot, it may not be necessary to enable wake
> > > from D3cold ... the reason to enable wake from D3cold is that it's
> > > not currently knowable if system suspend is going to morph the D3hot
> > > into a D3cold.
> > > 
> > > IMO it's probably fair to ignore D3cold except if the target is D3hot.
> >
> > And for now we probably want to ignore the whole thing for runtime 
> > suspends, since Linux doesn't properly handle PME signals.  Unless the 
> > system is going to sleep, we should avoid generating them.
> 
> I'd rather avoid creating special cases like that though; leave the
> wakeup stuff in and it'll continue to be ignored ... until Linux properly
> handles runtime PME# signals.  And if the system enters a sleep state
> (like S1 or S3) the "right thing" should just happen...
> 
> I don't know how (or whether!) ACPI handles PME# at runtime, but it'd
> be trivial to have a timer watch for devices whose config space reports
> they're signaling PME#.  Not the prettiest solution; but until ACPI does
> better, it could certainly make everything work consistently.

Okay, the next version of the patch will enable wakeup for whatever level 
we change to, plus D3cold if we are changing to D3hot.

But if you want a watchdog routine for PME#, you'll have to write it!  I
don't want to spend the necessary time figuring out the inner workings of
the PCI core.  :-)

Alan Stern


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]