Hi,
The series of patches implement binding physical devices with ACPI
devices. With it, device drivers can utilize methods provided by
firmware (ACPI). These patches are against 2.6.10, please give your
comments.
Thanks,
Shaohua
On Tue, 2005-01-04 at 21:50, Li Shaohua wrote:
> Hi,
> The series of patches implement binding physical devices with ACPI
> devices. With it, device drivers can utilize methods provided by
> firmware (ACPI). These patches are against 2.6.10, please give your
> comments.
I think we'll save some significant power when ACPI D-states
are invoked for the devices that supply them. While many
PCI devices support PCI power management and thus don't need/use
ACPI D-states, I think it will be even more important to add
this functionality to the legacy devices which never have
PCI power management and thus always depend on ACPI for D-states.
It looks like some device drivers scribble on dev->platform_data;
and we need to fix those drivers before deploying this patch.
Alternatively, we could add a new field to struct device,
but then we'd probably never get rid of it...
I'm a little unformforable with platform_notify
and platform_notify_remove available as globals.
We should probably BUG_ON if we
find them set before ACPI writes on them.
We'll need to update this patch to handle erroneous
but real platforms that don't have _BBN
by using _CRS to get PCI bus number.
Unclear to me if/how this association of ACPI capability
should be reflected in the sysfs device tree.
thanks,
-Len
On Thu, 2005-01-06 at 12:00, Len Brown wrote:
> I think we'll save some significant power when ACPI D-states
> are invoked for the devices that supply them. While many
> PCI devices support PCI power management and thus don't need/use
> ACPI D-states, I think it will be even more important to add
> this functionality to the legacy devices which never have
> PCI power management and thus always depend on ACPI for D-states.
It's easy to link the PNP devices with the ACPI devices with the ACPIPNP
driver. But the PNP driver core hasn't similar routines as
'pnp_set_power_state'. Unclear if the PNP drivers support
suspend/resume.
> It looks like some device drivers scribble on dev->platform_data;
> and we need to fix those drivers before deploying this patch.
> Alternatively, we could add a new field to struct device,
> but then we'd probably never get rid of it...
Yep, this is a big problem. According to the comments in the source
file, it's designed for firmware such as ACPI, but some drivers misused
it. A search shows there are many such drivers. Fixing the drivers is a
pain for me.
> I'm a little unformforable with platform_notify
> and platform_notify_remove available as globals.
> We should probably BUG_ON if we
> find them set before ACPI writes on them.
I will fix it.
> We'll need to update this patch to handle erroneous
> but real platforms that don't have _BBN
> by using _CRS to get PCI bus number.
That's ok.
> Unclear to me if/how this association of ACPI capability
> should be reflected in the sysfs device tree.
Possibly both the physical devices and ACPI devices have a link in sysfs
to pointer partners.
Thanks,
Shaohua
Hi!
> > It looks like some device drivers scribble on dev->platform_data;
> > and we need to fix those drivers before deploying this patch.
> > Alternatively, we could add a new field to struct device,
> > but then we'd probably never get rid of it...
> Yep, this is a big problem. According to the comments in the source
> file, it's designed for firmware such as ACPI, but some drivers misused
> it. A search shows there are many such drivers. Fixing the drivers is a
> pain for me.
It is easy: remove the field for release or two, then readd it with
your patch.
Pavel
--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
On Wed, 2005-01-05 at 10:50, Li Shaohua wrote:
> Hi,
> The series of patches implement binding physical devices with ACPI
> devices. With it, device drivers can utilize methods provided by
> firmware (ACPI). These patches are against 2.6.10, please give your
> comments.
Hi,
This is updated patches according to latest discussion.
Changes from last one:
1. introduce new field 'firmware_data' in 'struct device', since people
complain rename 'platform_data. Greg, could you please check if the
comments I added in 'struct device' are correct?
2. align to Pavel's latest PCI state convention work.
3. Some cleanups and add more comments.
One issue is 'platform_pci_choose_state' doesn't get called, it should
be after Pavel updates the parameter of 'pci_choose_state'
Thanks,
Shaohua
Hi!
> > The series of patches implement binding physical devices with ACPI
> > devices. With it, device drivers can utilize methods provided by
> > firmware (ACPI). These patches are against 2.6.10, please give your
> > comments.
> This is updated patches according to latest discussion.
> Changes from last one:
> 1. introduce new field 'firmware_data' in 'struct device', since people
> complain rename 'platform_data. Greg, could you please check if the
> comments I added in 'struct device' are correct?
> 2. align to Pavel's latest PCI state convention work.
> 3. Some cleanups and add more comments.
> One issue is 'platform_pci_choose_state' doesn't get called, it should
> be after Pavel updates the parameter of 'pci_choose_state'
diff -puN drivers/pci/pci.c~acpi-pci-get-suspend-state-callback
drivers/pci/pci.c
--- 2.5/drivers/pci/pci.c~acpi-pci-get-suspend-state-callback
2005-01-17 12:54:05.357547072 +0800
+++ 2.5-root/drivers/pci/pci.c 2005-01-17 13:08:50.835933896 +0800
@@ -317,6 +317,7 @@ pci_set_power_state(struct pci_dev *dev,
* Returns PCI power state suitable for given device and given system
* message.
*/
+int (*platform_pci_choose_state)(struct pci_dev *, pm_message_t) = 0;
pci_power_t pci_choose_state(struct pci_dev *dev, u32 state)
{
Perhaps you want this to be "= NULL"?
> @@ -208,6 +209,25 @@ acpi_status pci_osc_control_set(u32 flag
> }
> EXPORT_SYMBOL(pci_osc_control_set);
>
> +static int acpi_pci_choose_state(struct pci_dev *pdev,
> + pm_message_t state)
> +{
> + char dstate_str[] = "_S0D";
> + acpi_status status;
> + unsigned long val;
> + struct device *dev = &pdev->dev;
> +
> + /* state is PM_SUSPEND_* */
> + if ((state >= PM_SUSPEND_MAX) || !DEVICE_ACPI_HANDLE(dev))
> + return -EINVAL;
> + dstate_str[2] += (int __force)state;
When I'm done, you will not be able to just retype state to
integer... Perhaps you want to do pci_choose_state first; that gets
you pci_power_t and that one *is* okay to retype to int?
[And you will be shielded from my patches, too ;-) ]
Pavel
--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
On Mon, 2005-01-17 at 19:28, Pavel Machek wrote:
> Hi!
>
> > > The series of patches implement binding physical devices with ACPI
> > > devices. With it, device drivers can utilize methods provided by
> > > firmware (ACPI). These patches are against 2.6.10, please give your
> > > comments.
>
> > This is updated patches according to latest discussion.
> > Changes from last one:
> > 1. introduce new field 'firmware_data' in 'struct device', since people
> > complain rename 'platform_data. Greg, could you please check if the
> > comments I added in 'struct device' are correct?
> > 2. align to Pavel's latest PCI state convention work.
> > 3. Some cleanups and add more comments.
> > One issue is 'platform_pci_choose_state' doesn't get called, it should
> > be after Pavel updates the parameter of 'pci_choose_state'
>
> diff -puN drivers/pci/pci.c~acpi-pci-get-suspend-state-callback
> drivers/pci/pci.c
> --- 2.5/drivers/pci/pci.c~acpi-pci-get-suspend-state-callback
> 2005-01-17 12:54:05.357547072 +0800
> +++ 2.5-root/drivers/pci/pci.c 2005-01-17 13:08:50.835933896 +0800
> @@ -317,6 +317,7 @@ pci_set_power_state(struct pci_dev *dev,
> * Returns PCI power state suitable for given device and given system
> * message.
> */
> +int (*platform_pci_choose_state)(struct pci_dev *, pm_message_t) = 0;
>
> pci_power_t pci_choose_state(struct pci_dev *dev, u32 state)
> {
>
> Perhaps you want this to be "= NULL"?
I must be in sleep :). I will fix it soon.
>
>
> > @@ -208,6 +209,25 @@ acpi_status pci_osc_control_set(u32 flag
> > }
> > EXPORT_SYMBOL(pci_osc_control_set);
> >
> > +static int acpi_pci_choose_state(struct pci_dev *pdev,
> > + pm_message_t state)
> > +{
> > + char dstate_str[] = "_S0D";
> > + acpi_status status;
> > + unsigned long val;
> > + struct device *dev = &pdev->dev;
> > +
> > + /* state is PM_SUSPEND_* */
> > + if ((state >= PM_SUSPEND_MAX) || !DEVICE_ACPI_HANDLE(dev))
> > + return -EINVAL;
> > + dstate_str[2] += (int __force)state;
>
> When I'm done, you will not be able to just retype state to
> integer... Perhaps you want to do pci_choose_state first; that gets
> you pci_power_t and that one *is* okay to retype to int?
Firmware possibly will can't return a useful suspend state (Either
firmware doesn't define such device or evaluation failed), that's why I
return an int. I suppose pci_choose_state will do something:
ret = firmware_pci_choose_state(dev, state);
if (ret >= 0)
pci_state = ret;
switch(pci_state) {
case 0: return PCI_D0;
.....
}
Thanks,
Shaohua