* Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver
From: Zhang, Rui @ 2012-09-29 13:37 UTC (permalink / raw)
To: Alan Cox
Cc: Grant Likely, LKML, linux-acpi@vger.kernel.org, linux-i2c,
Dirk Brandewie, linux-pm
In-Reply-To: <20120928135401.6781d134@pyramind.ukuu.org.uk>
> -----Original Message-----
> From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk]
> Sent: Friday, September 28, 2012 8:54 PM
> To: Zhang, Rui
> Cc: LKML; linux-pm; linux-i2c; linux-acpi@vger.kernel.org; Len, Brown;
> Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> Subject: Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller
> enumeration driver
> Importance: High
>
> On Fri, 28 Sep 2012 15:40:32 +0800
> Zhang Rui <rui.zhang@intel.com> wrote:
>
> > >From 6077a62f2865201ab6727ca7d628ee5e43aa57e1 Mon Sep 17 00:00:00
> > >2001
> > From: Zhang Rui <rui.zhang@intel.com>
> > Date: Fri, 24 Aug 2012 15:18:25 +0800
> > Subject: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller
> > enumeration driver
> >
> > This driver is able to
> > 1) enumerate I2C controller via ACPI namespace
> > and register it as a platform device.
> > 2) enumerate I2C slave devices via ACPI namespace.
>
> Will this also trigger and work with the ACPI4 based devices that seem
> to have I²C tables that Linux currently doesn't understand (eq some
> GMA600/Oaktrail platforms) ?
I'm not aware of this. I think the answer is probably "No"
because these PNPids are pretty new.
But we can check FADT.revision to make the driver work
on ACPI5 platforms only.
Thanks,
rui
^ permalink raw reply
* Re: [RFC PATCH 1/6] Introduce acpi_match_device_id().
From: Mika Westerberg @ 2012-10-01 6:37 UTC (permalink / raw)
To: Zhang, Rui
Cc: LKML, linux-pm, linux-i2c, linux-acpi@vger.kernel.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <744357E9AAD1214791ACBA4B0B90926322C156@SHSMSX101.ccr.corp.intel.com>
On Sat, Sep 29, 2012 at 01:31:52PM +0000, Zhang, Rui wrote:
>
> > > +{
> > > + acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
> >
> > If the device is not bound to an ACPI handle this will return NULL. And
> > I don't see you doing that in this series meaning that..
> >
>
>
> You're right, I should set pdev->archdata.acpi_handle to the I2C
> controller in i2c_root.c.
There already is an API for that - check drivers/acpi/glue.c.
^ permalink raw reply
* Re: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal
From: Mika Westerberg @ 2012-10-01 6:44 UTC (permalink / raw)
To: Zhang Rui
Cc: LKML, linux-pm, linux-i2c,
linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <1348817863.10877.320.camel-fuY85erJQUO75v1z/vFq2g@public.gmane.org>
On Fri, Sep 28, 2012 at 03:37:43PM +0800, Zhang Rui wrote:
>
> the main idea is that, for Serial Buses like I2C and SPI, we enumerate
> the controller as a platform device, and then enumerate the slaves via
> i2c/spi_register_board_info. And then, when the controller is really
> probed and enabled in the platform driver, the SPI/I2C bus code will
> enumerate I2C/SPI slaves automatically.
> And for the other devices, we will enumerate all of them as platform
> devices, which is not covered in this patch set yet.
Can you show some example how we could use this new code for example with
an existing I2C/SPI slave driver? Let's say the device uses few GPIOs, one
for interrupt and other for triggering firmware download. In addition to
that it needs a special parameters that can be extracted running the "_DSM"
method of the device.
Normally the driver would get this stuff from the platform data or from
Device Tree but how it is done with these patches?
^ permalink raw reply
* Re: [RFC PATCH 2/6] Introduce ACPI style match in platform_match
From: Mika Westerberg @ 2012-10-01 6:47 UTC (permalink / raw)
To: Zhang Rui
Cc: LKML, linux-pm, linux-i2c, linux-acpi@vger.kernel.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <1348817955.10877.322.camel@rui.sh.intel.com>
On Fri, Sep 28, 2012 at 03:39:15PM +0800, Zhang Rui wrote:
> >From 5d7ecd12c2994b8c5905d52718c2870c3b62746e Mon Sep 17 00:00:00 2001
> From: Zhang Rui <rui.zhang@intel.com>
> Date: Fri, 28 Sep 2012 14:51:03 +0800
> Subject: [RFC PATCH 2/6] Introduce ACPI style match in platform_match
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/base/platform.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index a1a7225..90e64c6f 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -20,6 +20,7 @@
> #include <linux/err.h>
> #include <linux/slab.h>
> #include <linux/pm_runtime.h>
> +#include <linux/acpi.h>
>
> #include "base.h"
>
> @@ -635,6 +636,13 @@ static const struct platform_device_id *platform_match_id(
> struct platform_device *pdev)
> {
> while (id->name[0]) {
> +#ifdef CONFIG_ACPI
I don't think the above is needed as you stub the acpi_match_device_id()
out when !CONFIG_ACPI.
How about I2C and SPI slave devices?
> + /* attempt ACPI style match */
> + if (acpi_match_device_id(&pdev->dev, id->name) == 0) {
> + pdev->id_entry = id;
> + return id;
> + }
> +#endif
> if (strcmp(pdev->name, id->name) == 0) {
> pdev->id_entry = id;
> return id;
> --
> 1.7.7.6
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver
From: Mika Westerberg @ 2012-10-01 6:55 UTC (permalink / raw)
To: Zhang Rui
Cc: LKML, linux-pm, linux-i2c,
linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <1348818032.10877.325.camel-fuY85erJQUO75v1z/vFq2g@public.gmane.org>
On Fri, Sep 28, 2012 at 03:40:32PM +0800, Zhang Rui wrote:
> +acpi_status __init i2c_enumerate_slave(acpi_handle handle, u32 level,
> + void *data, void **return_value)
> +{
> + int result;
> + acpi_status status;
> + struct acpi_buffer buffer;
> + struct acpi_resource *resource;
> + struct acpi_resource_gpio *gpio;
> + struct acpi_resource_i2c_serialbus *i2c;
> + int i;
> + struct acpi_i2c_root *root = data;
> + struct i2c_board_info info;
> + struct acpi_device *device;
> +
> + if (acpi_bus_get_device(handle, &device))
> + return AE_OK;
> +
> + status = acpi_get_current_resources(handle, &buffer);
> + if (ACPI_FAILURE(status)) {
> + dev_err(&device->dev, "Failed to get ACPI resources\n");
> + return AE_OK;
> + }
> +
> + for (i = 0; i < buffer.length; i += sizeof(struct acpi_resource)) {
> + resource = (struct acpi_resource *)(buffer.pointer + i);
> +
> + switch (resource->type) {
> + case ACPI_RESOURCE_TYPE_GPIO:
> + gpio = &resource->data.gpio;
> +
> + if (gpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT) {
> + result =
> + acpi_device_get_gpio_irq
> + (gpio->resource_source.string_ptr,
> + gpio->pin_table[0], &info.irq);
acpi_device_get_gpio_irq() is not defined in this patch series?
Also you need to do the gpio_request()/gpio_to_irq() things somewhere. Are
they handled in acpi_device_get_gpio_irq()?
How about GpioIo resources?
> + if (result)
> + dev_err(&device->dev,
> + "Failed to get IRQ\n");
> + }
> + break;
> + case ACPI_RESOURCE_TYPE_SERIAL_BUS:
> + i2c = &resource->data.i2c_serial_bus;
> +
> + info.addr = i2c->slave_address;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + add_slave(root, &info);
> +
> + kfree(buffer.pointer);
> + return AE_OK;
> +}
> +
> +static int __devinit acpi_i2c_root_add(struct acpi_device *device)
> +{
> + acpi_status status;
> + struct acpi_i2c_root *root;
> + struct resource *resources;
> + int result;
> +
> + if (!device->pnp.unique_id) {
> + dev_err(&device->dev,
> + "Unsupported ACPI I2C controller. No UID\n");
Where does this restriction come from? As far as I understand UID is
optional.
> + return -ENODEV;
> + }
> +
> + root = kzalloc(sizeof(struct acpi_i2c_root), GFP_KERNEL);
> + if (!root)
> + return -ENOMEM;
> +
> + root->device = device;
> +
> + kstrtoint(device->pnp.unique_id, 10, &root->busnum);
> +
> + /* enumerate I2C controller */
> + root->pdev =
> + platform_device_alloc(acpi_device_hid(device), root->busnum);
> + if (!root->pdev) {
> + dev_err(&device->dev, "Failed to alloc platform device\n");
> + goto err;
> + }
> +
> + result = acpi_get_generic_resources(device, &resources);
> + if (result < 0) {
> + dev_err(&device->dev, "Failed to get resources\n");
> + goto err;
> + }
> +
> + platform_device_add_resources(root->pdev, resources, result);
> + platform_device_add(root->pdev);
> +
> + /* enumerate I2C slave devices */
> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, root->device->handle, 1,
> + i2c_enumerate_slave, NULL, root, NULL);
> +
> + if (ACPI_FAILURE(status)) {
> + dev_err(&root->device->dev, "i2c ACPI namespace walk error!\n");
> + kfree(root);
> + return -ENODEV;
> + }
> +
> + register_slaves(root);
> +
> + return 0;
> +err:
> + kfree(root);
> + return -ENODEV;
> +}
^ permalink raw reply
* RE: [RFC PATCH 1/6] Introduce acpi_match_device_id().
From: Zhang, Rui @ 2012-10-01 13:56 UTC (permalink / raw)
To: Mika Westerberg
Cc: LKML, linux-pm, linux-i2c,
linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <20121001063753.GE15548-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org]
> Sent: Monday, October 01, 2012 2:38 PM
> To: Zhang, Rui
> Cc: LKML; linux-pm; linux-i2c; linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Len, Brown;
> Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> Subject: Re: [RFC PATCH 1/6] Introduce acpi_match_device_id().
> Importance: High
>
> On Sat, Sep 29, 2012 at 01:31:52PM +0000, Zhang, Rui wrote:
> >
> > > > +{
> > > > + acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
> > >
> > > If the device is not bound to an ACPI handle this will return NULL.
> > > And I don't see you doing that in this series meaning that..
> > >
> >
> >
> > You're right, I should set pdev->archdata.acpi_handle to the I2C
> > controller in i2c_root.c.
>
> There already is an API for that - check drivers/acpi/glue.c.
Do you mean acpi_bind?
acpi_bind_one will bind the physical device node to the ACPI device,
But for the i2c controller ACPI device, the physical node is the I2C
adapter in the I2C bus.
If we introduce I2C bus ACPI binding, which is what we're doing now,
the i2c_adapter->dev.archdata.acpi_handle will be set to the ACPI
i2c controller handle by acpi_bind_one.
Thanks,
rui
^ permalink raw reply
* RE: [RFC PATCH 2/6] Introduce ACPI style match in platform_match
From: Zhang, Rui @ 2012-10-01 14:00 UTC (permalink / raw)
To: Mika Westerberg
Cc: LKML, linux-pm, linux-i2c, linux-acpi@vger.kernel.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <20121001064709.GG15548@intel.com>
> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Mika Westerberg
> Sent: Monday, October 01, 2012 2:47 PM
> To: Zhang, Rui
> Cc: LKML; linux-pm; linux-i2c; linux-acpi@vger.kernel.org; Len, Brown;
> Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> Subject: Re: [RFC PATCH 2/6] Introduce ACPI style match in
> platform_match
> Importance: High
>
> On Fri, Sep 28, 2012 at 03:39:15PM +0800, Zhang Rui wrote:
> > >From 5d7ecd12c2994b8c5905d52718c2870c3b62746e Mon Sep 17 00:00:00
> > >2001
> > From: Zhang Rui <rui.zhang@intel.com>
> > Date: Fri, 28 Sep 2012 14:51:03 +0800
> > Subject: [RFC PATCH 2/6] Introduce ACPI style match in platform_match
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> > drivers/base/platform.c | 8 ++++++++
> > 1 files changed, 8 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c index
> > a1a7225..90e64c6f 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -20,6 +20,7 @@
> > #include <linux/err.h>
> > #include <linux/slab.h>
> > #include <linux/pm_runtime.h>
> > +#include <linux/acpi.h>
> >
> > #include "base.h"
> >
> > @@ -635,6 +636,13 @@ static const struct platform_device_id
> *platform_match_id(
> > struct platform_device *pdev)
> > {
> > while (id->name[0]) {
> > +#ifdef CONFIG_ACPI
>
> I don't think the above is needed as you stub the acpi_match_device_id()
> out when !CONFIG_ACPI.
>
You're right, I'll remove this.
> How about I2C and SPI slave devices?
>
We're introduce the I2C/SPI bus ACPI binding, and the i2c/spi bus .match
method can be redirected to acpi callbacks.
Thanks,
rui
^ permalink raw reply
* RE: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver
From: Zhang, Rui @ 2012-10-01 14:19 UTC (permalink / raw)
To: Mika Westerberg
Cc: LKML, linux-pm, linux-i2c, linux-acpi@vger.kernel.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <20121001065526.GH15548@intel.com>
> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org [mailto:linux-i2c-
> owner@vger.kernel.org] On Behalf Of Mika Westerberg
> Sent: Monday, October 01, 2012 2:55 PM
> To: Zhang, Rui
> Cc: LKML; linux-pm; linux-i2c; linux-acpi@vger.kernel.org; Len, Brown;
> Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> Subject: Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller
> enumeration driver
> Importance: High
>
> On Fri, Sep 28, 2012 at 03:40:32PM +0800, Zhang Rui wrote:
> > +acpi_status __init i2c_enumerate_slave(acpi_handle handle, u32 level,
> > + void *data, void **return_value) {
> > + int result;
> > + acpi_status status;
> > + struct acpi_buffer buffer;
> > + struct acpi_resource *resource;
> > + struct acpi_resource_gpio *gpio;
> > + struct acpi_resource_i2c_serialbus *i2c;
> > + int i;
> > + struct acpi_i2c_root *root = data;
> > + struct i2c_board_info info;
> > + struct acpi_device *device;
> > +
> > + if (acpi_bus_get_device(handle, &device))
> > + return AE_OK;
> > +
> > + status = acpi_get_current_resources(handle, &buffer);
> > + if (ACPI_FAILURE(status)) {
> > + dev_err(&device->dev, "Failed to get ACPI resources\n");
> > + return AE_OK;
> > + }
> > +
> > + for (i = 0; i < buffer.length; i += sizeof(struct acpi_resource))
> {
> > + resource = (struct acpi_resource *)(buffer.pointer + i);
> > +
> > + switch (resource->type) {
> > + case ACPI_RESOURCE_TYPE_GPIO:
> > + gpio = &resource->data.gpio;
> > +
> > + if (gpio->connection_type ==
> ACPI_RESOURCE_GPIO_TYPE_INT) {
> > + result =
> > + acpi_device_get_gpio_irq
> > + (gpio->resource_source.string_ptr,
> > + gpio->pin_table[0], &info.irq);
>
> acpi_device_get_gpio_irq() is not defined in this patch series?
>
ACPI GPIO controller patch has already been sent out, but in ACPI mailing list only.
> Also you need to do the gpio_request()/gpio_to_irq() things somewhere.
> Are they handled in acpi_device_get_gpio_irq()?
>
Yep.
> How about GpioIo resources?
>
This is not covered in this patch set, but will be in the next patch set.
> > + if (result)
> > + dev_err(&device->dev,
> > + "Failed to get IRQ\n");
> > + }
> > + break;
> > + case ACPI_RESOURCE_TYPE_SERIAL_BUS:
> > + i2c = &resource->data.i2c_serial_bus;
> > +
> > + info.addr = i2c->slave_address;
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > +
> > + add_slave(root, &info);
> > +
> > + kfree(buffer.pointer);
> > + return AE_OK;
> > +}
> > +
> > +static int __devinit acpi_i2c_root_add(struct acpi_device *device) {
> > + acpi_status status;
> > + struct acpi_i2c_root *root;
> > + struct resource *resources;
> > + int result;
> > +
> > + if (!device->pnp.unique_id) {
> > + dev_err(&device->dev,
> > + "Unsupported ACPI I2C controller. No UID\n");
>
> Where does this restriction come from? As far as I understand UID is
> optional.
>
_UID is optional.
But it seems to be required for SPB buses that need ACPI device enumeration.
At least this is true for the ACPI 5 compatible ACPI tables I have.
Thanks,
rui
^ permalink raw reply
* RE: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal
From: Zhang, Rui @ 2012-10-01 14:30 UTC (permalink / raw)
To: Mika Westerberg
Cc: LKML, linux-pm, linux-i2c, linux-acpi@vger.kernel.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <20121001064433.GF15548@intel.com>
> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: Monday, October 01, 2012 2:45 PM
> To: Zhang, Rui
> Cc: LKML; linux-pm; linux-i2c; linux-acpi@vger.kernel.org; Len, Brown;
> Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> Subject: Re: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal
> Importance: High
>
> On Fri, Sep 28, 2012 at 03:37:43PM +0800, Zhang Rui wrote:
> >
> > the main idea is that, for Serial Buses like I2C and SPI, we
> enumerate
> > the controller as a platform device, and then enumerate the slaves
> via
> > i2c/spi_register_board_info. And then, when the controller is really
> > probed and enabled in the platform driver, the SPI/I2C bus code will
> > enumerate I2C/SPI slaves automatically.
> > And for the other devices, we will enumerate all of them as platform
> > devices, which is not covered in this patch set yet.
>
> Can you show some example how we could use this new code for example
> with an existing I2C/SPI slave driver?
This is just prototype patch set that I want to illustrate my idea
on ACPI device enumeration, to get more thoughts on this.
So no example driver so far.
> Let's say the device uses few
> GPIOs, one for interrupt and other for triggering firmware download. In
> addition to that it needs a special parameters that can be extracted
> running the "_DSM" method of the device.
>
> Normally the driver would get this stuff from the platform data or from
> Device Tree but how it is done with these patches?
Can you show me an example driver that gets the special parameters from Device Tree?
Thanks,
rui
^ permalink raw reply
* Re: [RFC PATCH 1/6] Introduce acpi_match_device_id().
From: Mika Westerberg @ 2012-10-02 6:10 UTC (permalink / raw)
To: Zhang, Rui
Cc: LKML, linux-pm, linux-i2c, linux-acpi@vger.kernel.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <744357E9AAD1214791ACBA4B0B90926322D93C@SHSMSX101.ccr.corp.intel.com>
On Mon, Oct 01, 2012 at 01:56:17PM +0000, Zhang, Rui wrote:
>
>
> > -----Original Message-----
> > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> > Sent: Monday, October 01, 2012 2:38 PM
> > To: Zhang, Rui
> > Cc: LKML; linux-pm; linux-i2c; linux-acpi@vger.kernel.org; Len, Brown;
> > Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> > Subject: Re: [RFC PATCH 1/6] Introduce acpi_match_device_id().
> > Importance: High
> >
> > On Sat, Sep 29, 2012 at 01:31:52PM +0000, Zhang, Rui wrote:
> > >
> > > > > +{
> > > > > + acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
> > > >
> > > > If the device is not bound to an ACPI handle this will return NULL.
> > > > And I don't see you doing that in this series meaning that..
> > > >
> > >
> > >
> > > You're right, I should set pdev->archdata.acpi_handle to the I2C
> > > controller in i2c_root.c.
> >
> > There already is an API for that - check drivers/acpi/glue.c.
>
> Do you mean acpi_bind?
No, I mean register_acpi_bus_type(). You can see an example of this looking
PCI, USB, .., drivers.
^ permalink raw reply
* Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver
From: Mika Westerberg @ 2012-10-02 6:12 UTC (permalink / raw)
To: Zhang, Rui
Cc: LKML, linux-pm, linux-i2c, linux-acpi@vger.kernel.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <744357E9AAD1214791ACBA4B0B90926322D970@SHSMSX101.ccr.corp.intel.com>
On Mon, Oct 01, 2012 at 02:19:49PM +0000, Zhang, Rui wrote:
>
>
> > -----Original Message-----
> > From: linux-i2c-owner@vger.kernel.org [mailto:linux-i2c-
> > owner@vger.kernel.org] On Behalf Of Mika Westerberg
> > Sent: Monday, October 01, 2012 2:55 PM
> > To: Zhang, Rui
> > Cc: LKML; linux-pm; linux-i2c; linux-acpi@vger.kernel.org; Len, Brown;
> > Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> > Subject: Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller
> > enumeration driver
> > Importance: High
> >
> > On Fri, Sep 28, 2012 at 03:40:32PM +0800, Zhang Rui wrote:
> > > +acpi_status __init i2c_enumerate_slave(acpi_handle handle, u32 level,
> > > + void *data, void **return_value) {
> > > + int result;
> > > + acpi_status status;
> > > + struct acpi_buffer buffer;
> > > + struct acpi_resource *resource;
> > > + struct acpi_resource_gpio *gpio;
> > > + struct acpi_resource_i2c_serialbus *i2c;
> > > + int i;
> > > + struct acpi_i2c_root *root = data;
> > > + struct i2c_board_info info;
> > > + struct acpi_device *device;
> > > +
> > > + if (acpi_bus_get_device(handle, &device))
> > > + return AE_OK;
> > > +
> > > + status = acpi_get_current_resources(handle, &buffer);
> > > + if (ACPI_FAILURE(status)) {
> > > + dev_err(&device->dev, "Failed to get ACPI resources\n");
> > > + return AE_OK;
> > > + }
> > > +
> > > + for (i = 0; i < buffer.length; i += sizeof(struct acpi_resource))
> > {
> > > + resource = (struct acpi_resource *)(buffer.pointer + i);
> > > +
> > > + switch (resource->type) {
> > > + case ACPI_RESOURCE_TYPE_GPIO:
> > > + gpio = &resource->data.gpio;
> > > +
> > > + if (gpio->connection_type ==
> > ACPI_RESOURCE_GPIO_TYPE_INT) {
> > > + result =
> > > + acpi_device_get_gpio_irq
> > > + (gpio->resource_source.string_ptr,
> > > + gpio->pin_table[0], &info.irq);
> >
> > acpi_device_get_gpio_irq() is not defined in this patch series?
> >
> ACPI GPIO controller patch has already been sent out, but in ACPI mailing list only.
It would have been good idea to mention this in the cover letter at least.
>
> > Also you need to do the gpio_request()/gpio_to_irq() things somewhere.
> > Are they handled in acpi_device_get_gpio_irq()?
> >
> Yep.
>
> > How about GpioIo resources?
> >
> This is not covered in this patch set, but will be in the next patch set.
>
> > > + if (result)
> > > + dev_err(&device->dev,
> > > + "Failed to get IRQ\n");
> > > + }
> > > + break;
> > > + case ACPI_RESOURCE_TYPE_SERIAL_BUS:
> > > + i2c = &resource->data.i2c_serial_bus;
> > > +
> > > + info.addr = i2c->slave_address;
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > + }
> > > +
> > > + add_slave(root, &info);
> > > +
> > > + kfree(buffer.pointer);
> > > + return AE_OK;
> > > +}
> > > +
> > > +static int __devinit acpi_i2c_root_add(struct acpi_device *device) {
> > > + acpi_status status;
> > > + struct acpi_i2c_root *root;
> > > + struct resource *resources;
> > > + int result;
> > > +
> > > + if (!device->pnp.unique_id) {
> > > + dev_err(&device->dev,
> > > + "Unsupported ACPI I2C controller. No UID\n");
> >
> > Where does this restriction come from? As far as I understand UID is
> > optional.
> >
>
> _UID is optional.
> But it seems to be required for SPB buses that need ACPI device enumeration.
> At least this is true for the ACPI 5 compatible ACPI tables I have.
Yes but if some vendor is not setting it (as it is optional) you still want
your code to work, right?
^ permalink raw reply
* Re: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal
From: Mika Westerberg @ 2012-10-02 6:19 UTC (permalink / raw)
To: Zhang, Rui
Cc: LKML, linux-pm, linux-i2c,
linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <744357E9AAD1214791ACBA4B0B90926322D989-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>
On Mon, Oct 01, 2012 at 02:30:00PM +0000, Zhang, Rui wrote:
>
>
> > -----Original Message-----
> > From: Mika Westerberg [mailto:mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org]
> > Sent: Monday, October 01, 2012 2:45 PM
> > To: Zhang, Rui
> > Cc: LKML; linux-pm; linux-i2c; linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Len, Brown;
> > Rafael J. Wysocki; Grant Likely; Dirk Brandewie
> > Subject: Re: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal
> > Importance: High
> >
> > On Fri, Sep 28, 2012 at 03:37:43PM +0800, Zhang Rui wrote:
> > >
> > > the main idea is that, for Serial Buses like I2C and SPI, we
> > enumerate
> > > the controller as a platform device, and then enumerate the slaves
> > via
> > > i2c/spi_register_board_info. And then, when the controller is really
> > > probed and enabled in the platform driver, the SPI/I2C bus code will
> > > enumerate I2C/SPI slaves automatically.
> > > And for the other devices, we will enumerate all of them as platform
> > > devices, which is not covered in this patch set yet.
> >
> > Can you show some example how we could use this new code for example
> > with an existing I2C/SPI slave driver?
>
> This is just prototype patch set that I want to illustrate my idea
> on ACPI device enumeration, to get more thoughts on this.
> So no example driver so far.
But surely you have thought how this should be done? Otherwise we only see
a part of the solution here. What if this enumeration you introduce here
doesn't play well when I2C/SPI are added?
>
> > Let's say the device uses few
> > GPIOs, one for interrupt and other for triggering firmware download. In
> > addition to that it needs a special parameters that can be extracted
> > running the "_DSM" method of the device.
> >
> > Normally the driver would get this stuff from the platform data or from
> > Device Tree but how it is done with these patches?
>
> Can you show me an example driver that gets the special parameters from Device Tree?
drivers/misc/eeprom/at25.c but there are much more if you search for DT
enabled drivers.
Now, I don't know what is the proper way to pass parameters in ACPI but
"_DSM" seems to be one that is being used.
^ permalink raw reply
* Re: [RFC PATCH 3/6] ACPI: introduce acpi_get_generic_resources
From: Bjorn Helgaas @ 2012-10-05 15:36 UTC (permalink / raw)
To: Zhang Rui
Cc: LKML, linux-pm, linux-i2c, linux-acpi@vger.kernel.org, Len, Brown,
Rafael J. Wysocki, Grant Likely, Dirk Brandewie
In-Reply-To: <1348818006.10877.323.camel@rui.sh.intel.com>
On Fri, Sep 28, 2012 at 1:40 AM, Zhang Rui <rui.zhang@intel.com> wrote:
> From 9a851d177794129a89f720c7122cb39fd163126b Mon Sep 17 00:00:00 2001
> From: Zhang Rui <rui.zhang@intel.com>
> Date: Fri, 28 Sep 2012 08:34:05 +0800
> Subject: [RFC PATCH 3/6] ACPI: introduce acpi_get_generic_resources
>
> Introduce acpi_get_generic_resources() to convert
> ACPI style resources to struct resource.
This seems obviously similar to drivers/pnp/pnpacpi/rsparser.c, but
only handles a few of the resource types. Do you plan to extend this
to support everything rsparser.c supports? Do you envision replacing
the current PNPACPI/PNPBIOS/ISAPNP stuff with something built on what
you're doing here? We already have two ways for drivers to bind to
ACPI devices (pnp_register_driver() and acpi_bus_register_driver()).
I hope we are moving toward *one* way rather than three ways :)
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/acpi/Makefile | 1 +
> drivers/acpi/resource.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++
> include/acpi/acpi_bus.h | 1 +
> 3 files changed, 167 insertions(+), 0 deletions(-)
> create mode 100644 drivers/acpi/resource.c
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 6b1d535..4b65608 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -46,6 +46,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
> ifdef CONFIG_ACPI_VIDEO
> acpi-y += video_detect.o
> endif
> +acpi-y += resource.o
>
> # These are (potentially) separate modules
> obj-$(CONFIG_ACPI_AC) += ac.o
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> new file mode 100644
> index 0000000..30a5204
> --- /dev/null
> +++ b/drivers/acpi/resource.c
> @@ -0,0 +1,165 @@
> +/*
> + * resource.c -- convert ACPI resource to generic resource
> + *
> + * Copyright (c) 2012 Zhang Rui <rui.zhang@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + */
> +#include <linux/kernel.h>
> +#include <linux/export.h>
> +#include <linux/acpi.h>
> +
> +static int irq_flags(int triggering, int polarity, int sharable)
> +{
> + int flags;
> +
> + if (triggering == ACPI_LEVEL_SENSITIVE) {
> + if (polarity == ACPI_ACTIVE_LOW)
> + flags = IORESOURCE_IRQ_LOWLEVEL;
> + else
> + flags = IORESOURCE_IRQ_HIGHLEVEL;
> + } else {
> + if (polarity == ACPI_ACTIVE_LOW)
> + flags = IORESOURCE_IRQ_LOWEDGE;
> + else
> + flags = IORESOURCE_IRQ_HIGHEDGE;
> + }
> +
> + if (sharable == ACPI_SHARED)
> + flags |= IORESOURCE_IRQ_SHAREABLE;
> +
> + return flags;
> +}
> +
> +static void acpi_get_irq_resource(struct acpi_resource *res,
> + struct resource *resource)
> +{
> + struct acpi_resource_irq *irq = &res->data.irq;
> + int t, p;
> +
> + if (irq->interrupt_count == 0)
> + resource->flags = IORESOURCE_DISABLED;
> +
> + if (!acpi_get_override_irq(irq->interrupts[0], &t, &p)) {
> + t = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
> + p = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> +
> + if (irq->triggering != t || irq->polarity != p) {
> + irq->triggering = t;
> + irq->polarity = p;
> + }
> + }
> +
> + resource->flags =
> + irq_flags(irq->triggering, irq->polarity, irq->sharable);
> + resource->flags |= IORESOURCE_IRQ;
> + resource->start = irq->interrupts[0];
> + resource->end = irq->interrupts[0];
> +}
> +
> +static void acpi_get_extended_irq_resource(struct acpi_resource *res,
> + struct resource *resource)
> +{
> + struct acpi_resource_extended_irq *irq = &res->data.extended_irq;
> + int t, p;
> +
> + if (irq->interrupt_count == 0)
> + resource->flags = IORESOURCE_DISABLED;
> +
> + if (!acpi_get_override_irq(irq->interrupts[0], &t, &p)) {
> + t = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
> + p = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> +
> + if (irq->triggering != t || irq->polarity != p) {
> + irq->triggering = t;
> + irq->polarity = p;
> + }
> + }
> +
> + resource->flags =
> + irq_flags(irq->triggering, irq->polarity, irq->sharable);
> + resource->flags |= IORESOURCE_IRQ;
> + resource->start = irq->interrupts[0];
> + resource->end = irq->interrupts[0];
> +}
> +
> +static void acpi_get_mem_resource(struct acpi_resource *res,
> + struct resource *resource)
> +{
> + struct acpi_resource_fixed_memory32 *mem = &res->data.fixed_memory32;
> +
> + if (mem->address_length == 0)
> + resource->flags |= IORESOURCE_DISABLED;
> + if (mem->write_protect == ACPI_READ_WRITE_MEMORY)
> + resource->flags |= IORESOURCE_MEM_WRITEABLE;
> +
> + resource->flags |= IORESOURCE_MEM;
> + resource->start = mem->address;
> + resource->end = mem->address + mem->address_length - 1;
> +}
> +
> +int acpi_get_generic_resources(struct acpi_device *device,
> + struct resource **resources)
> +{
> + acpi_status status;
> + struct acpi_buffer buffer;
> + struct acpi_resource *res;
> + struct resource *p;
> + int res_count;
> + int i;
> +
> + status = acpi_get_current_resources(device->handle, &buffer);
> + if (ACPI_FAILURE(status)) {
> + dev_err(&device->dev, "can't get ACPI resources\n");
> + return -EINVAL;
> + }
> +
> + res_count = (buffer.length - 1) / sizeof(struct acpi_resource) - 1;
> + p = kzalloc(sizeof(struct resource) * res_count, GFP_KERNEL);
> + if (!resources) {
> + kfree(buffer.pointer);
> + return -ENOMEM;
> + }
> +
> + res = (struct acpi_resource *)buffer.pointer;
> + i = 0;
> +
> + while (res->type != ACPI_RESOURCE_TYPE_END_TAG) {
> +
> + switch (res->type) {
> + case ACPI_RESOURCE_TYPE_IRQ:
> + acpi_get_irq_resource(res, &p[i]);
> + break;
> + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> + acpi_get_extended_irq_resource(res, &p[i]);
> + break;
> + case ACPI_RESOURCE_TYPE_MEMORY32:
> + acpi_get_mem_resource(res, &p[i]);
> + break;
> + default:
> + i--;
> + break;
> + }
> + i++;
> + res = ACPI_NEXT_RESOURCE(res);
> + }
> +
> + /* Get rid of unsupported ACPI resources */
> + if (i != res_count)
> + p =
> + kmemdup(p, sizeof(struct resource) * i, GFP_KERNEL);
> +
> + *resources = p;
> + kfree(buffer.pointer);
> + return i;
> +}
> +
> +EXPORT_SYMBOL(acpi_get_generic_resources);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 8b5b124..a4f8eaf 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -381,6 +381,7 @@ int acpi_match_device_ids(struct acpi_device *device,
> int acpi_match_device_id(const struct device *, const char *);
> int acpi_create_dir(struct acpi_device *);
> void acpi_remove_dir(struct acpi_device *);
> +int acpi_get_generic_resources(struct acpi_device *, struct resource **);
>
> /*
> * Bind physical devices with ACPI devices
> --
> 1.7.7.6
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [REGRESSION]: hibernation issues on 3.5.4/3.5.5
From: Andrew Watts @ 2012-10-08 5:30 UTC (permalink / raw)
To: linux-kernel, linux-pm
Hello.
I am experiencing problems with hibernation (echo disk >
/sys/power/state) on both 3.5.4 and 3.5.5. Note: Problem did not
exist up through 3.5.2 (I skipped 3.5.3).
Upon resume from hibernation, the % loading progress indicator
appears. At some point the image stops loading and the system
reverts to a standard cold-boot sequence.
Logs (attached below) reveal it is erroring out with "Invalid LZO
uncompressed length".
Many thanks for your help.
~ Andy
=======
[ 2.354392] PM: Checking hibernation image partition /dev/hda2
[ 2.354414] PM: Hibernation image partition 3:2 present
[ 2.354415] PM: Looking for hibernation image.
[ 2.379577] PM: Image signature found, resuming
[ 2.406288] PM: Marking nosave pages: [mem 0x0009f000-0x000fffff]
[ 2.406294] PM: Basic memory bitmaps created
[ 2.406295] PM: Preparing processes for restore.
[ 2.406298] Freezing user space processes ... (elapsed 0.00 seconds) done.
[ 2.406604] PM: Loading hibernation image.
[ 3.162702] PM: Using 1 thread(s) for decompression.
[ 3.162702] PM: Loading and decompressing image data (161994 pages) ... ^H^H^H^H 0%%^H^H^H^H 1%%^H^H^H^H 2%%^H^H^H^H 3%%^H^H^H^H 4%%^H^H^H^H 5%%^H^H^H^H 6%%^H^H^H^H 7%%^H^H^H^H 8%%^H^H^H^H 9%%^H^H^H^H 10%%^H^H^H^H 11%%^H^H^H^H 12%%^H^H^H^H 13%%^H^H^H^H 14%%^H^H^H^H 15%%^H^H^H^H 16%%^H^H^H^H 17%%^H^H^H^H 18%%^H^H^H^H 19%%^H^H^H^H 20%%^H^H^H^H 21%%^H^H^H^H 22%%^H^H^H^H 23%%^H^H^H^H 24%%^H^H^H^H 25%%^H^H^H^H 26%%^H^H^H^H 27%%^H^H^H^H 28%%^H^H^H^H 29%%^H^H^H^H 30%%^H^H^H^H 31%%^H^H^H^H 32%%^H^H^H^H 33%%^H^H^H^H 34%%^H^H^H^H 35%%^H^H^H^H 36%%^H^H^H^H 37%%^H^H^H^H 38%%^H^H^H^H 39%%^H^H^H^H 40%%^H^H^H^H 41%%^H^H^H^H 42%%^H^H^H^H 43%%^H^H^H^H 44%%^H^H^H^H 45%%^H^H^H^H 46%%^H^H^H^H 47%%^H^H^H^H 48%%^H^H^H^H 49%%^H^H^H^H 50%%^H^H^H^H 51%%^H^H^H^H 52%%^H^H^H^H 53%%^H^H^H^H 54%%^H^H^H
^H 55%%^H^H^H^H 56%%^H^H^H^H 57%%^H^H^H^H 58%%^H^H^H^H 59%%^H^H^H^H 60%%^H^H^H^H 61%%^H^H^H^H 62%%
[ 7.730534] PM: Invalid LZO uncompressed length
[ 7.730743]
[ 7.730880] PM: Read 647976 kbytes in 4.56 seconds (142.10 MB/s)
[ 7.731616] PM: Error -1 resuming
[ 8.224207] PM: Failed to load hibernation image, recovering.
[ 8.279520] Restarting tasks ... done.
[ 8.279783] PM: Basic memory bitmaps freed
[ 8.306034] PM: Hibernation image not present or could not be loaded.
^ permalink raw reply
* Tracing # of rtpm transitions
From: Sundar @ 2012-10-12 13:51 UTC (permalink / raw)
To: Alan Stern, Rafael J. Wysocki; +Cc: linux-pm, vivek.thakkar
<I sent an another email from sundar.iyer@intel.com, but I think it
hasnt got through>
Hi,
We need to count/trace the runtime PM transitions for all components.
Is this small snippet of change valid? This can be extended to include
a transition to active state, get exposed in sysfs, controls to reset
the value like other existing sysfs entries.
Or is the wakeup_count the correct means to figure out the resume events?
diff --git a/drivers/base/power/runtime.c
b/drivers/base/power/runtime.c index 1f339d5..233f8f5 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -52,6 +52,8 @@ static void __update_runtime_status(struct device
*dev, enum rpm_status status)
update_pm_runtime_accounting(dev);
trace_runtime_pm_status(dev, status);
dev->power.runtime_status = status;
+ if (status == RPM_SUSPENDED)
+ atomic_inc(&dev->power.transition_suspend_count);
}
/**
siyer5@pnp-buildsrv-4:~/ICS/hardware/intel/linux-2.6$ git diff
include/linux/pm.h diff --git a/include/linux/pm.h
b/include/linux/pm.h index 411e4f4..076a0e3 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -442,6 +442,7 @@ struct dev_pm_info {
wait_queue_head_t wait_queue;
atomic_t usage_count;
atomic_t child_count;
+ atomic_t suspend_transition_count;
unsigned int disable_depth:3;
unsigned int ignore_children:1;
unsigned int idle_notification:1;
Cheers!
--
---------
The views expressed in this email are personal and do not necessarily
echo my employers.
^ permalink raw reply
* Re: Subject: [PATCH] drivers-core: move device_pm_remove behind bus_remove_device
From: Yanmin Zhang @ 2012-10-15 7:39 UTC (permalink / raw)
To: Zhang, LongX, Alan Stern
Cc: linux-kernel@vger.kernel.org, linux-pm@lists.linux-foundation.org,
rjw@sisk.pl
In-Reply-To: <F7B8FD780A346D46A0042F5C63B06AE775B2B0@SHSMSX102.ccr.corp.intel.com>
On Fri, 2012-09-21 at 01:58 +0000, Zhang, LongX wrote:
> From: LongX Zhang <longx.zhang@intel.com>
>
> device_pm_remove will call pm_runtime_remove which would disable
> runtime PM of the device. After that pm_runtime_get* or
> pm_runtime_put* will be ingored. So if we disable the runtime PM
> before device really be removed, drivers' _remove callback may
> access HW even pm_runtime_get* fails. That is bad.
The background about the patch: We hit an hang issue when removing a mmc
device on Medfield Android phone by sysfs interface.
Consider below call sequence when removing a device:
device_del => device_pm_remove
=> class_intf->remove_dev(dev, class_intf) => pm_runtime_get_sync/put_sync
=> bus_remove_device => device_release_driver => pm_runtime_get_sync/put_sync
remove_dev might call pm_runtime_get_sync/put_sync.
Then, generic device_release_driver also calls pm_runtime_get_sync/put_sync.
Since device_del => device_pm_remove firstly, later _get_sync wouldn't really
wake up the device.
I git log -p to find the patch which moves the calling to device_pm_remove ahead.
It's below patch:
commit 775b64d2b6ca37697de925f70799c710aab5849a
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date: Sat Jan 12 20:40:46 2008 +0100
PM: Acquire device locks on suspend
This patch reorganizes the way suspend and resume notifications are
sent to drivers. The major changes are that now the PM core acquires
every device semaphore before calling the methods, and calls to
device_add() during suspends will fail, while calls to device_del()
during suspends will block.
It also provides a way to safely remove a suspended device with the
help of the PM core, by using the device_pm_schedule_removal() callback
introduced specifically for this purpose, and updates two drivers (msr
and cpuid) that need to use it.
As device_pm_schedule_removal is deleted by another patch, we need also revert
other parts of the patch, i.e. move the calling of device_pm_remove after
the calling to bus_remove_device.
>
> Signed-off-by: LongX Zhang <longx.zhang@intel.com>
> ---
> drivers/base/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 5e6e00b..81ea7f2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1169,7 +1169,6 @@ void device_del(struct device *dev)
> if (dev->bus)
> blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> BUS_NOTIFY_DEL_DEVICE, dev);
> - device_pm_remove(dev);
> dpm_sysfs_remove(dev);
> if (parent)
> klist_del(&dev->p->knode_parent);
> @@ -1194,6 +1193,7 @@ void device_del(struct device *dev)
> device_remove_file(dev, &uevent_attr);
> device_remove_attrs(dev);
> bus_remove_device(dev);
> + device_pm_remove(dev);
> driver_deferred_probe_del(dev);
>
> /*
^ permalink raw reply
* Re: Subject: [PATCH] drivers-core: move device_pm_remove behind bus_remove_device
From: Rafael J. Wysocki @ 2012-10-15 20:59 UTC (permalink / raw)
To: yanmin_zhang
Cc: Zhang, LongX, linux-kernel@vger.kernel.org,
linux-pm@lists.linux-foundation.org
In-Reply-To: <1350286789.2207.6.camel@ymzhang.sh.intel.com>
On Monday 15 of October 2012 15:39:49 Yanmin Zhang wrote:
> On Fri, 2012-09-21 at 01:58 +0000, Zhang, LongX wrote:
> > From: LongX Zhang <longx.zhang@intel.com>
> >
> > device_pm_remove will call pm_runtime_remove which would disable
> > runtime PM of the device. After that pm_runtime_get* or
> > pm_runtime_put* will be ingored. So if we disable the runtime PM
> > before device really be removed, drivers' _remove callback may
> > access HW even pm_runtime_get* fails. That is bad.
> The background about the patch: We hit an hang issue when removing a mmc
> device on Medfield Android phone by sysfs interface.
>
> Consider below call sequence when removing a device:
> device_del => device_pm_remove
> => class_intf->remove_dev(dev, class_intf) => pm_runtime_get_sync/put_sync
> => bus_remove_device => device_release_driver => pm_runtime_get_sync/put_sync
>
> remove_dev might call pm_runtime_get_sync/put_sync.
> Then, generic device_release_driver also calls pm_runtime_get_sync/put_sync.
> Since device_del => device_pm_remove firstly, later _get_sync wouldn't really
> wake up the device.
>
> I git log -p to find the patch which moves the calling to device_pm_remove ahead.
> It's below patch:
>
> commit 775b64d2b6ca37697de925f70799c710aab5849a
> Author: Rafael J. Wysocki <rjw@sisk.pl>
> Date: Sat Jan 12 20:40:46 2008 +0100
>
> PM: Acquire device locks on suspend
>
> This patch reorganizes the way suspend and resume notifications are
> sent to drivers. The major changes are that now the PM core acquires
> every device semaphore before calling the methods, and calls to
> device_add() during suspends will fail, while calls to device_del()
> during suspends will block.
>
> It also provides a way to safely remove a suspended device with the
> help of the PM core, by using the device_pm_schedule_removal() callback
> introduced specifically for this purpose, and updates two drivers (msr
> and cpuid) that need to use it.
>
>
> As device_pm_schedule_removal is deleted by another patch, we need also revert
> other parts of the patch, i.e. move the calling of device_pm_remove after
> the calling to bus_remove_device.
>
> >
> > Signed-off-by: LongX Zhang <longx.zhang@intel.com>
Can you please resend the patch? I lost the track of it unfortunately.
Thanks,
Rafael
> > ---
> > drivers/base/core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 5e6e00b..81ea7f2 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1169,7 +1169,6 @@ void device_del(struct device *dev)
> > if (dev->bus)
> > blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> > BUS_NOTIFY_DEL_DEVICE, dev);
> > - device_pm_remove(dev);
> > dpm_sysfs_remove(dev);
> > if (parent)
> > klist_del(&dev->p->knode_parent);
> > @@ -1194,6 +1193,7 @@ void device_del(struct device *dev)
> > device_remove_file(dev, &uevent_attr);
> > device_remove_attrs(dev);
> > bus_remove_device(dev);
> > + device_pm_remove(dev);
> > driver_deferred_probe_del(dev);
> >
> > /*
>
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* [PATCH v2] drivers-core: move the calling to device_pm_remove behind the calling to bus_remove_device
From: zhanglong @ 2012-10-16 15:28 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org
Cc: Rafael J. Wysocki, yanmin_zhang, Alan Stern,
linux-pm@lists.linux-foundation.org
We hit an hang issue when removing a mmc device on Medfield Android phone by sysfs interface.
device_pm_remove will call pm_runtime_remove which would disable
runtime PM of the device. After that pm_runtime_get* or
pm_runtime_put* will be ignored. So if we disable the runtime PM
before device really be removed, drivers' _remove callback may
access HW even pm_runtime_get* fails. That is bad.
Consider below call sequence when removing a device:
device_del => device_pm_remove
=> class_intf->remove_dev(dev, class_intf) => pm_runtime_get_sync/put_sync
=> bus_remove_device => device_release_driver => pm_runtime_get_sync/put_sync
remove_dev might call pm_runtime_get_sync/put_sync.
Then, generic device_release_driver also calls pm_runtime_get_sync/put_sync.
Since device_del => device_pm_remove firstly, later _get_sync wouldn't really wake up the device.
I git log -p to find the patch which moves the calling to device_pm_remove ahead.
It's below patch:
commit 775b64d2b6ca37697de925f70799c710aab5849a
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date: Sat Jan 12 20:40:46 2008 +0100
PM: Acquire device locks on suspend
This patch reorganizes the way suspend and resume notifications are
sent to drivers. The major changes are that now the PM core acquires
every device semaphore before calling the methods, and calls to
device_add() during suspends will fail, while calls to device_del()
during suspends will block.
It also provides a way to safely remove a suspended device with the
help of the PM core, by using the device_pm_schedule_removal() callback
introduced specifically for this purpose, and updates two drivers (msr
and cpuid) that need to use it.
As device_pm_schedule_removal is deleted by another patch, we need also revert other parts of the patch,
i.e. move the calling of device_pm_remove after the calling to bus_remove_device.
Signed-off-by: LongX Zhang <longx.zhang@intel.com>
---
drivers/base/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index abea76c..150a415 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1180,7 +1180,6 @@ void device_del(struct device *dev)
if (dev->bus)
blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
BUS_NOTIFY_DEL_DEVICE, dev);
- device_pm_remove(dev);
dpm_sysfs_remove(dev);
if (parent)
klist_del(&dev->p->knode_parent);
@@ -1205,6 +1204,7 @@ void device_del(struct device *dev)
device_remove_file(dev, &uevent_attr);
device_remove_attrs(dev);
bus_remove_device(dev);
+ device_pm_remove(dev);
driver_deferred_probe_del(dev);
/* Notify the platform of the removal, in case they
--
1.7.10
^ permalink raw reply related
* Re: Subject: [PATCH] drivers-core: move device_pm_remove behind bus_remove_device
From: Yanmin Zhang @ 2012-10-18 0:21 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Zhang, LongX, Alan Stern, linux-kernel@vger.kernel.org,
linux-pm@lists.linux-foundation.org
In-Reply-To: <2338561.PDRRozbqko@vostro.rjw.lan>
On Mon, 2012-10-15 at 22:59 +0200, Rafael J. Wysocki wrote:
> On Monday 15 of October 2012 15:39:49 Yanmin Zhang wrote:
> > On Fri, 2012-09-21 at 01:58 +0000, Zhang, LongX wrote:
> > > From: LongX Zhang <longx.zhang@intel.com>
> > >
> > > device_pm_remove will call pm_runtime_remove which would disable
> > > runtime PM of the device. After that pm_runtime_get* or
> > > pm_runtime_put* will be ingored. So if we disable the runtime PM
> > > before device really be removed, drivers' _remove callback may
> > > access HW even pm_runtime_get* fails. That is bad.
> > The background about the patch: We hit an hang issue when removing a mmc
> > device on Medfield Android phone by sysfs interface.
> >
> > Consider below call sequence when removing a device:
> > device_del => device_pm_remove
> > => class_intf->remove_dev(dev, class_intf) => pm_runtime_get_sync/put_sync
> > => bus_remove_device => device_release_driver => pm_runtime_get_sync/put_sync
> >
> > remove_dev might call pm_runtime_get_sync/put_sync.
> > Then, generic device_release_driver also calls pm_runtime_get_sync/put_sync.
> > Since device_del => device_pm_remove firstly, later _get_sync wouldn't really
> > wake up the device.
> >
> > I git log -p to find the patch which moves the calling to device_pm_remove ahead.
> > It's below patch:
> >
> > commit 775b64d2b6ca37697de925f70799c710aab5849a
> > Author: Rafael J. Wysocki <rjw@sisk.pl>
> > Date: Sat Jan 12 20:40:46 2008 +0100
> >
> > PM: Acquire device locks on suspend
> >
> > This patch reorganizes the way suspend and resume notifications are
> > sent to drivers. The major changes are that now the PM core acquires
> > every device semaphore before calling the methods, and calls to
> > device_add() during suspends will fail, while calls to device_del()
> > during suspends will block.
> >
> > It also provides a way to safely remove a suspended device with the
> > help of the PM core, by using the device_pm_schedule_removal() callback
> > introduced specifically for this purpose, and updates two drivers (msr
> > and cpuid) that need to use it.
> >
> >
> > As device_pm_schedule_removal is deleted by another patch, we need also revert
> > other parts of the patch, i.e. move the calling of device_pm_remove after
> > the calling to bus_remove_device.
> >
> > >
> > > Signed-off-by: LongX Zhang <longx.zhang@intel.com>
>
> Can you please resend the patch? I lost the track of it unfortunately.
Long sent V2 out. See https://lkml.org/lkml/2012/10/16/594.
^ permalink raw reply
* Your mail to linux-pm-request@vger.kernel.org
From: linux-pm-request @ 2012-10-20 15:32 UTC (permalink / raw)
To: gll-linux-pm2
In-Reply-To: <87txtpqjfn.fsf@topper.koldfront.dk>
This pre-recorded message is being sent in response to your recent
email to linux-pm-request@vger.kernel.org.
All routine administrative requests (including subscriptions and
unsubscriptions) concerning this mailing list are handled by an
automated server. Please read this message carefully to find the
information relevant to you.
SUBSCRIBING
===========
To subscribe to linux-pm, send the following in the body (not
the subject line) of an email message to "Majordomo@vger.kernel.org":
subscribe linux-pm
This will subscribe the account from which you send the message to
the linux-pm list.
If you wish to subscribe another address instead (such as a local
redistribution list), you can use a command of the form:
subscribe linux-pm other-address@your_site.your_net
UNSUBSCRIBING
=============
To unsubscribe from linux-pm, send the following in the body (not
the subject line) of an email message to "Majordomo@vger.kernel.org":
unsubscribe linux-pm
This will unsubscribe the account from which you send the message.
If you are subscribed with some other address, you'll have to send
a command of the following form instead:
unsubscribe linux-pm other-address@your_site.your_net
If you don't know what address you are subscribed with, you can send
the following command to see who else is on the list (assuming that
information isn't designated "private" by the owner of the list):
who linux-pm
If you want to search non-private lists at this server, you can do that
by sending a command like:
which string
This will return a list of all entries on all lists that contain "string".
HELP
====
To find out more about the automated server and the commands it
understands, send the following command to "Majordomo@vger.kernel.org":
help
If you feel you need to reach a human, send email to:
linux-pm-approval@vger.kernel.org
^ permalink raw reply
* Re: [PATCH 1/5] Thermal: do bind operation after thermal zone or cooling device register returns.
From: Francesco Lavra @ 2012-10-21 10:05 UTC (permalink / raw)
To: hongbo.zhang
Cc: linaro-dev, linux-kernel, linux-pm, STEricsson_nomadik_linux,
kernel, linaro-kernel, hongbo.zhang, patches, amit.kachhap
In-Reply-To: <1350387889-15324-2-git-send-email-hongbo.zhang@linaro.com>
Hi,
On 10/16/2012 01:44 PM, hongbo.zhang wrote:
> From: "hongbo.zhang" <hongbo.zhang@linaro.com>
>
> In the previous bind function, cdev->get_max_state(cdev, &max_state) is called
> before the registration function finishes, but at this moment, the parameter
> cdev at thermal driver layer isn't ready--it will get ready only after its
> registration, so the the get_max_state callback cannot tell the max_state
> according to the cdev input.
> This problem can be fixed by separating the bind operation out of registration
> and doing it when registration completely finished.
When thermal_cooling_device_register() is called, the thermal framework
assumes the cooling device is "ready", i.e. all of its ops callbacks
return meaningful results. If the cooling device is not ready at this
point, then this is a bug in the code that registers it.
Specifically, the faulty code in your case is in the cpufreq cooling
implementation, where the cooling device is registered before being
added to the internal list of cpufreq cooling devices. So, IMHO the fix
is needed there.
--
Francesco
^ permalink raw reply
* intel_idle/parameters/max_cstate read only
From: Alon Bar-Lev @ 2012-10-21 13:55 UTC (permalink / raw)
To: Linux PM list
Hi,
In the old acpi module, the max_cstate could have been changed at
runtime via the sysfs.
The intel_idle does not provide this functionality.
# ls -la /sys/module/intel_idle/parameters/max_cstate
-r--r--r--. 1 root root 4096 Oct 21 15:55
/sys/module/intel_idle/parameters/max_cstate
Is that on purpose? Is there any way to achieve the same at runtime?
Regards,
Alon Bar-Lev.
^ permalink raw reply
* Re: [PATCH 5/5] Thermal: Add ST-Ericsson db8500 thermal dirver.
From: Francesco Lavra @ 2012-10-21 15:01 UTC (permalink / raw)
To: hongbo.zhang
Cc: linaro-dev, linux-kernel, linux-pm, STEricsson_nomadik_linux,
kernel, linaro-kernel, hongbo.zhang, patches
In-Reply-To: <1350387889-15324-6-git-send-email-hongbo.zhang@linaro.com>
Hi Hongbo,
On 10/16/2012 01:44 PM, hongbo.zhang wrote:
> From: "hongbo.zhang" <hongbo.zhang@linaro.com>
>
> This diver is based on the thermal management framework in thermal_sys.c.
> A thermal zone device is created with the trip points to which cooling
> devices can be bound, the current cooling device is cpufreq, e.g. CPU
> frequency is clipped down to cool the CPU, and other cooling devices can
> be added and bound to the trip points dynamically.
> The platform specific PRCMU interrupts are used to active thermal update
> when trip points are reached.
[...]
> diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c
> new file mode 100644
> index 0000000..bb065d4
> --- /dev/null
> +++ b/drivers/thermal/db8500_cpufreq_cooling.c
> @@ -0,0 +1,118 @@
> +/*
> + * db8500_cpufreq_cooling.c - db8500 cpufreq works as cooling device.
> + *
> + * Copyright (C) 2012 ST-Ericsson
> + * Copyright (C) 2012 Linaro Ltd.
> + *
> + * Author: Hongbo Zhang <hognbo.zhang@stericsson.com>
Your e-mail address is misspelled :)
[...]
> diff --git a/drivers/thermal/db8500_thermal.c b/drivers/thermal/db8500_thermal.c
> new file mode 100644
> index 0000000..34dcc52
> --- /dev/null
> +++ b/drivers/thermal/db8500_thermal.c
> @@ -0,0 +1,507 @@
> +/*
> + * db8500_thermal.c - db8500 Thermal Management Implementation
> + *
> + * Copyright (C) 2012 ST-Ericsson
> + * Copyright (C) 2012 Linaro Ltd.
> + *
> + * Author: Hongbo Zhang <hognbo.zhang@stericsson.com>
Misspelled address
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +#include <linux/cpu_cooling.h>
> +#include <linux/mfd/dbx500-prcmu.h>
> +#include <linux/platform_data/db8500_thermal.h>
> +
> +#define PRCMU_DEFAULT_MEASURE_TIME 0xFFF
> +#define PRCMU_DEFAULT_LOW_TEMP 0
> +
> +struct db8500_thermal_zone {
> + struct thermal_zone_device *therm_dev;
> + struct mutex th_lock;
> + struct platform_device *thsens_pdev;
This member is set in db8500_thermal_probe(), but is never used. I would
suggest removing it.
> + struct work_struct therm_work;
> + struct db8500_thsens_platform_data *trip_tab;
> + enum thermal_device_mode mode;
> + enum thermal_trend trend;
> + unsigned long cur_temp_pseudo;
> + unsigned int cur_index;
> + int low_irq;
> + int high_irq;
Same story as thsens_pdev, low_irq and high_irq are set in
db8500_thermal_probe(), but are never used. Should be removed.
> +};
> +
> +/* Callback to bind cooling device to thermal zone */
> +static int db8500_cdev_bind(struct thermal_zone_device *thermal,
> + struct thermal_cooling_device *cdev)
> +{
> + struct db8500_thermal_zone *pzone;
> + struct db8500_thsens_platform_data *ptrips;
> + char *cdev_name;
> + unsigned long max_state, upper, lower;
> + int i, j, ret;
> +
> + pzone = (struct db8500_thermal_zone *)thermal->devdata;
> + ptrips = pzone->trip_tab;
> +
> + if (!cdev->type)
> + return -EINVAL;
cdev->type is an array, not a simple pointer, so it cannot be NULL.
> +
> + ret = -ENODEV;
> + for (i = 0; i < ptrips->num_trips; i++)
> + for (j = 0; j < COOLING_DEV_MAX; j++) {
> + cdev_name = ptrips->trip_points[i].cooling_dev_name[j];
> + if (!cdev_name)
> + continue;
> +
> + if (strcmp(cdev_name, cdev->type))
> + continue;
> +
> + cdev->ops->get_max_state(cdev, &max_state);
> + upper = (i > max_state) ? max_state : i;
> + lower = (i > max_state) ? max_state : i;
You may want to merge these two lines: upper = lower = ...
> +
> + ret = thermal_zone_bind_cooling_device(thermal, i,
> + cdev, upper, lower);
> + if (ret)
> + pr_err("Error binding cooling device.\n");
> + else
> + pr_info("Cdev %s bound.\n", cdev->type);
> + }
> +
> + return ret;
> +}
> +
> +/* Callback to unbind cooling device from thermal zone */
> +static int db8500_cdev_unbind(struct thermal_zone_device *thermal,
> + struct thermal_cooling_device *cdev)
> +{
> + struct db8500_thermal_zone *pzone;
> + struct db8500_thsens_platform_data *ptrips;
> + char *cdev_name;
> + int i, j, ret;
> +
> + pzone = (struct db8500_thermal_zone *)thermal->devdata;
> + ptrips = pzone->trip_tab;
> +
> + if (!cdev->type)
> + return -EINVAL;
cdev->type cannot be NULL.
> +
> + ret = -ENODEV;
> + for (i = 0; i < ptrips->num_trips; i++)
> + for (j = 0; j < COOLING_DEV_MAX; j++) {
> + cdev_name = ptrips->trip_points[i].cooling_dev_name[j];
> + if (!cdev_name)
> + continue;
> +
> + if (strcmp(cdev_name, cdev->type))
> + continue;
> +
> + ret = thermal_zone_unbind_cooling_device(
> + thermal, i, cdev);
> + if (ret)
> + pr_err("Error unbinding cooling device.\n");
> + else
> + pr_info("Cdev %s unbound.\n", cdev->type);
> + }
> +
> + return ret;
> +}
> +
> +/* Callback to get current temperature */
> +static int db8500_sys_get_temp(struct thermal_zone_device *thermal,
> + unsigned long *temp)
> +{
> + struct db8500_thermal_zone *pzone;
> + pzone = (struct db8500_thermal_zone *)thermal->devdata;
> +
> + /* TODO: There is no PRCMU interface to get temperature data currently,
> + so a pseudo temperature is returned , it works for the thermal framework
> + and this will be fixed when the PRCMU interface is available */
> + *temp = pzone->cur_temp_pseudo;
> +
> + return 0;
> +}
> +
> +/* Callback to get temperature changing trend */
> +static int db8500_sys_get_trend(struct thermal_zone_device *thermal,
> + int trip, enum thermal_trend *trend)
> +{
> + struct db8500_thermal_zone *pzone;
> + pzone = (struct db8500_thermal_zone *)thermal->devdata;
> +
> + *trend = pzone->trend;
> +
> + return 0;
> +}
> +
> +/* Callback to get thermal zone mode */
> +static int db8500_sys_get_mode(struct thermal_zone_device *thermal,
> + enum thermal_device_mode *mode)
> +{
> + struct db8500_thermal_zone *pzone;
> + pzone = (struct db8500_thermal_zone *)thermal->devdata;
> +
> + mutex_lock(&pzone->th_lock);
> + *mode = pzone->mode;
> + mutex_unlock(&pzone->th_lock);
> +
> + return 0;
> +}
> +
> +/* Callback to set thermal zone mode */
> +static int db8500_sys_set_mode(struct thermal_zone_device *thermal,
> + enum thermal_device_mode mode)
> +{
> + struct db8500_thermal_zone *pzone;
> + struct thermal_zone_device *pthdev;
> +
> + pzone = (struct db8500_thermal_zone *)thermal->devdata;
> + pthdev = pzone->therm_dev;
> +
> + if (!pthdev) {
> + pr_err("Thermal zone not registered.\n");
> + return 0;
> + }
If this function is called, you are sure the thermal zone has been
registered.
> +
> + mutex_lock(&pzone->th_lock);
> +
> + pzone->mode = mode;
> +
> + if (mode == THERMAL_DEVICE_ENABLED)
> + schedule_work(&pzone->therm_work);
> +
> + mutex_unlock(&pzone->th_lock);
> +
> + return 0;
> +}
> +
> +/* Callback to get trip point type */
> +static int db8500_sys_get_trip_type(struct thermal_zone_device *thermal,
> + int trip, enum thermal_trip_type *type)
> +{
> + struct db8500_thermal_zone *pzone;
> + struct db8500_thsens_platform_data *ptrips;
> +
> + pzone = (struct db8500_thermal_zone *)thermal->devdata;
> + ptrips = pzone->trip_tab;
> +
> + if (trip >= ptrips->num_trips)
> + return -EINVAL;
> +
> + *type = ptrips->trip_points[trip].type;
> +
> + return 0;
> +}
> +
> +/* Callback to get trip point temperature */
> +static int db8500_sys_get_trip_temp(struct thermal_zone_device *thermal,
> + int trip, unsigned long *temp)
> +{
> + struct db8500_thermal_zone *pzone;
> + struct db8500_thsens_platform_data *ptrips;
> +
> + pzone = (struct db8500_thermal_zone *)thermal->devdata;
> + ptrips = pzone->trip_tab;
> +
> + if (trip >= ptrips->num_trips)
> + return -EINVAL;
> +
> + *temp = ptrips->trip_points[trip].temp;
> +
> + return 0;
> +}
> +
> +/* Callback to get critical trip point temperature */
> +static int db8500_sys_get_crit_temp(struct thermal_zone_device *thermal,
> + unsigned long *temp)
> +{
> + struct db8500_thermal_zone *pzone;
> + struct db8500_thsens_platform_data *ptrips;
> + int i;
> +
> + pzone = (struct db8500_thermal_zone *)thermal->devdata;
> + ptrips = pzone->trip_tab;
> +
> + for (i = (ptrips->num_trips - 1); i > 0; i--) {
> + if (ptrips->trip_points[i].type == THERMAL_TRIP_CRITICAL) {
> + *temp = ptrips->trip_points[i].temp;
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static struct thermal_zone_device_ops thdev_ops = {
> + .bind = db8500_cdev_bind,
> + .unbind = db8500_cdev_unbind,
> + .get_temp = db8500_sys_get_temp,
> + .get_trend = db8500_sys_get_trend,
> + .get_mode = db8500_sys_get_mode,
> + .set_mode = db8500_sys_set_mode,
> + .get_trip_type = db8500_sys_get_trip_type,
> + .get_trip_temp = db8500_sys_get_trip_temp,
> + .get_crit_temp = db8500_sys_get_crit_temp,
> +};
> +
> +static irqreturn_t prcmu_low_irq_handler(int irq, void *irq_data)
> +{
> + struct db8500_thermal_zone *pzone = irq_data;
> + struct db8500_thsens_platform_data *ptrips;
> + unsigned long next_low, next_high;
> + unsigned int idx;
> +
> + ptrips = pzone->trip_tab;
> + idx = pzone->cur_index;
> + if (unlikely(idx == 0))
> + /* Meaningless for thermal management, ignoring it */
> + return IRQ_HANDLED;
> +
> + if (idx == 1) {
> + next_high = ptrips->trip_points[0].temp;
> + next_low = PRCMU_DEFAULT_LOW_TEMP;
> + } else {
> + next_high = ptrips->trip_points[idx-1].temp;
> + next_low = ptrips->trip_points[idx-2].temp;
> + }
> +
> + pzone->cur_index -= 1;
> + pzone->cur_temp_pseudo = (next_high + next_low)/2;
> +
> + prcmu_stop_temp_sense();
> + prcmu_config_hotmon((u8)(next_low/1000), (u8)(next_high/1000));
> + prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
> +
> + pr_debug("PRCMU set max %ld, set min %ld\n", next_high, next_low);
> +
> + pzone->trend = THERMAL_TREND_DROPPING;
> + schedule_work(&pzone->therm_work);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t prcmu_high_irq_handler(int irq, void *irq_data)
> +{
> + struct db8500_thermal_zone *pzone = irq_data;
> + struct db8500_thsens_platform_data *ptrips;
> + unsigned long next_low, next_high;
> + unsigned int idx;
> +
> + ptrips = pzone->trip_tab;
> + idx = pzone->cur_index;
> +
> + if (idx < ptrips->num_trips - 1) {
> + next_high = ptrips->trip_points[idx+1].temp;
> + next_low = ptrips->trip_points[idx].temp;
> +
> + pzone->cur_index += 1;
> + pzone->cur_temp_pseudo = (next_high + next_low)/2;
> +
> + prcmu_stop_temp_sense();
> + prcmu_config_hotmon((u8)(next_low/1000), (u8)(next_high/1000));
> + prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
> +
> + pr_debug("PRCMU set max %ld, min %ld\n", next_high, next_low);
> + }
> +
> + if (idx == ptrips->num_trips - 1)
} else if ()
> + pzone->cur_temp_pseudo = ptrips->trip_points[idx].temp + 1;
> +
> + pzone->trend = THERMAL_TREND_RAISING;
> + schedule_work(&pzone->therm_work);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void db8500_thermal_work(struct work_struct *work)
> +{
> + enum thermal_device_mode cur_mode;
> + struct db8500_thermal_zone *pzone;
> +
> + pzone = container_of(work, struct db8500_thermal_zone, therm_work);
> +
> + mutex_lock(&pzone->th_lock);
> + cur_mode = pzone->mode;
> + mutex_unlock(&pzone->th_lock);
> +
> + if (cur_mode == THERMAL_DEVICE_DISABLED) {
> + pr_warn("Warning: thermal function disabled.\n");
> + return;
> + }
> +
> + thermal_zone_device_update(pzone->therm_dev);
> + pr_debug("db8500_thermal_work finished.\n");
> +}
> +
> +static int __devinit db8500_thermal_probe(struct platform_device *pdev)
> +{
> + struct db8500_thermal_zone *pzone = NULL;
> + struct db8500_thsens_platform_data *ptrips;
> + int low_irq, high_irq, ret = 0;
> + unsigned long dft_low, dft_high;
> +
> + pzone = devm_kzalloc(&pdev->dev,
> + sizeof(struct db8500_thermal_zone), GFP_KERNEL);
> + if (!pzone)
> + return -ENOMEM;
> +
> + pzone->thsens_pdev = pdev;
> +
> + low_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_LOW");
> + if (low_irq < 0) {
> + pr_err("Get IRQ_HOTMON_LOW failed.\n");
> + return low_irq;
> + }
> +
> + ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,
> + prcmu_low_irq_handler,
> + IRQF_NO_SUSPEND | IRQF_ONESHOT, "dbx500_temp_low", pzone);
> + if (ret < 0) {
> + pr_err("Failed to allocate temp low irq.\n");
> + return ret;
> + }
> +
> + high_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_HIGH");
> + if (high_irq < 0) {
> + pr_err("Get IRQ_HOTMON_HIGH failed.\n");
> + return high_irq;
> + }
> +
> + ret = devm_request_threaded_irq(&pdev->dev, high_irq, NULL,
> + prcmu_high_irq_handler,
> + IRQF_NO_SUSPEND | IRQF_ONESHOT, "dbx500_temp_high", pzone);
> + if (ret < 0) {
> + pr_err("Failed to allocate temp high irq.\n");
> + return ret;
> + }
> +
> + pzone->low_irq = low_irq;
> + pzone->high_irq = high_irq;
> +
> + pzone->mode = THERMAL_DEVICE_DISABLED;
> +
> + mutex_init(&pzone->th_lock);
> +
> + INIT_WORK(&pzone->therm_work, db8500_thermal_work);
> +
> + ptrips = pdev->dev.platform_data;
> + pzone->trip_tab = ptrips;
> +
> + pzone->therm_dev = thermal_zone_device_register("db8500_thermal_zone",
> + ptrips->num_trips, 0, pzone, &thdev_ops, 0, 0);
> +
> + if (IS_ERR(pzone->therm_dev)) {
> + pr_err("Failed to register thermal zone device\n");
> + return PTR_ERR(pzone->therm_dev);
> + }
> +
> + dft_low = PRCMU_DEFAULT_LOW_TEMP;
> + dft_high = ptrips->trip_points[0].temp;
> +
> + prcmu_stop_temp_sense();
> + prcmu_config_hotmon((u8)(dft_low/1000), (u8)(dft_high/1000));
> + prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
> +
> + pzone->cur_index = 0;
> + pzone->cur_temp_pseudo = (dft_low + dft_high)/2;
> + pzone->trend = THERMAL_TREND_STABLE;
All the stuff from prcmu_stop_temp_sense() up to this line can race with
the irq handlers, I would suggest doing it before requesting the irqs.
> + pzone->mode = THERMAL_DEVICE_ENABLED;
Shouldn't this be protected by pzone->th_lock? Otherwise it should be
set before the thermal zone is registered.
> +
> + platform_set_drvdata(pdev, pzone);
> +
> + return 0;
> +}
> +
> +static int __devexit db8500_thermal_remove(struct platform_device *pdev)
> +{
> + struct db8500_thermal_zone *pzone;
> + pzone = platform_get_drvdata(pdev);
> +
> + cancel_work_sync(&pzone->therm_work);
> +
> + if (pzone->therm_dev)
> + thermal_zone_device_unregister(pzone->therm_dev);
> +
> + return 0;
> +}
mutex_destroy() should be called on pzone->th_lock
> +
> +static int db8500_thermal_suspend(struct platform_device *pdev,
> + pm_message_t state)
> +{
> + struct db8500_thermal_zone *pzone;
> + pzone = platform_get_drvdata(pdev);
> +
> + flush_work_sync(&pzone->therm_work);
> + prcmu_stop_temp_sense();
> +
> + return 0;
> +}
> +
> +static int db8500_thermal_resume(struct platform_device *pdev)
> +{
> + struct db8500_thermal_zone *pzone;
> + struct db8500_thsens_platform_data *ptrips;
> + unsigned long dft_low, dft_high;
> +
> + pzone = platform_get_drvdata(pdev);
> + ptrips = pzone->trip_tab;
> + dft_low = PRCMU_DEFAULT_LOW_TEMP;
> + dft_high = ptrips->trip_points[0].temp;
> +
> + prcmu_config_hotmon((u8)(dft_low/1000), (u8)(dft_high/1000));
> + prcmu_start_temp_sense(PRCMU_DEFAULT_MEASURE_TIME);
Shouldn't cur_index and cur_temp_pseudo be updated as well? You may want
to define a helper function with all the code shared by irq handlers
(both high and low), probe and resume.
> +
> + return 0;
> +}
[...]
> diff --git a/include/linux/platform_data/db8500_thermal.h b/include/linux/platform_data/db8500_thermal.h
> new file mode 100644
> index 0000000..0b6d164
> --- /dev/null
> +++ b/include/linux/platform_data/db8500_thermal.h
> @@ -0,0 +1,39 @@
> +/*
> + * db8500_thermal.h - db8500 Thermal Management Implementation
> + *
> + * Copyright (C) 2012 ST-Ericsson
> + * Copyright (C) 2012 Linaro Ltd.
> + *
> + * Author: Hongbo Zhang <hognbo.zhang@stericsson.com>
Misspelled address
--
Francesco
^ permalink raw reply
* [REGRESSION] 3.7-rc2 kernel BUG at kernel/power/snapshot.c:517!
From: Maciej Rutecki @ 2012-10-21 15:12 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: lenb, rjw, linux-acpi, pavel, linux-pm
Error: kernel BUG at kernel/power/snapshot.c:517!
Last known good: 3.6.1
Bad: 3.7-rc2
Steps to reproduce:
1. Boot system with 3.7-rc2 kernel
2. Try suspend to disk (s2ram works OK)
System dies on message:
http://mrutecki.pl/download/kernel/3.7-rc2/swinka/DSCN0280.jpg
lspci:
http://mrutecki.pl/download/kernel/3.7-rc2/swinka/lspci.txt
dmesg:
http://mrutecki.pl/download/kernel/3.7-rc2/swinka/dmesg-3.7-rc2.txt
--
Maciej Rutecki
http://www.mrutecki.pl
^ permalink raw reply
* Re: [RFC PATCH 4/4] sdio: pm: set device's power state after driver runtime suspended it
From: Rafael J. Wysocki @ 2012-10-21 19:57 UTC (permalink / raw)
To: Aaron Lu; +Cc: Chris Ball, linux-mmc, linux-pm, linux-acpi, Aaron Lu
In-Reply-To: <20121020071539.GB4798@localhost.localdomain>
On Saturday 20 of October 2012 15:15:41 Aaron Lu wrote:
> On Fri, Oct 19, 2012 at 08:08:38PM +0200, Rafael J. Wysocki wrote:
> > On Friday 19 of October 2012 01:39:25 Rafael J. Wysocki wrote:
> > > On Friday 12 of October 2012 11:12:41 Aaron Lu wrote:
> > > > In sdio bus level runtime callback function, after call the driver's
> > > > runtime suspend callback, we will check if the device supports a
> > > > platform level power management, and if so, a proper power state is
> > > > chosen by the corresponding platform callback and then set.
> > > >
> > > > Platform level runtime wakeup is also set, if device is enabled for
> > > > runtime wakeup by its driver, it will be armed the ability to generate
> > > > a wakeup event by the platform.
> > > >
> > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > > ---
> > > > drivers/mmc/core/sdio_bus.c | 49 +++++++++++++++++++++++++++++++++++++++++++--
> > > > 1 file changed, 47 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> > > > index aaec9e2..d83dea8 100644
> > > > --- a/drivers/mmc/core/sdio_bus.c
> > > > +++ b/drivers/mmc/core/sdio_bus.c
> > > > @@ -23,6 +23,7 @@
> > > >
> > > > #include "sdio_cis.h"
> > > > #include "sdio_bus.h"
> > > > +#include "sdio.h"
> > > > #include "sdio_acpi.h"
> > > >
> > > > /* show configuration fields */
> > > > @@ -194,10 +195,54 @@ static int sdio_bus_remove(struct device *dev)
> > > > }
> > > >
> > > > #ifdef CONFIG_PM
> > > > +
> > > > +static int sdio_bus_runtime_suspend(struct device *dev)
> > > > +{
> > > > + int ret;
> > > > + sdio_power_t state;
> > > > +
> > > > + ret = pm_generic_runtime_suspend(dev);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + if (!platform_sdio_power_manageable(dev))
> > > > + goto out;
> > > > +
> > > > + platform_sdio_run_wake(dev, true);
> > > > +
> > > > + state = platform_sdio_choose_power_state(dev);
> > > > + if (state == SDIO_POWER_ERROR) {
> > > > + ret = -EIO;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + ret = platform_sdio_set_power_state(dev, state);
> > > > +
> > > > +out:
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int sdio_bus_runtime_resume(struct device *dev)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + if (platform_sdio_power_manageable(dev)) {
> > > > + platform_sdio_run_wake(dev, false);
> > > > + ret = platform_sdio_set_power_state(dev, SDIO_D0);
> > > > + if (ret)
> > > > + goto out;
> > > > + }
> > > > +
> > > > + ret = pm_generic_runtime_resume(dev);
> > > > +
> > > > +out:
> > > > + return ret;
> > > > +}
> > > > +
> > >
> > > Most likely we will need to make analogous changes for other bus types that
> > > don't support power management natively, like platform, SPI, I2C etc. In all
> > > of them the _runtime_suspend() and _runtime_resume() routine will look
> > > almost exactly the same except for the platform_sdio_ prefix.
> > >
> > > For this reason, I think it would be better to simply define two functions
> > > acpi_pm_runtime_suspend() and acpi_pm_runtime_resume() that will do all of
> > > the ACPI-specific operations related to runtime suspend/resume. Then, we
> > > will be able to use these functions for all of the bus types in question
> > > in the same way (we may also need to add analogous functions for system
> > > suspend/resume handling).
> >
> > Something like in the (totally untested) patch below.
>
> Looks good to me.
> I'll test the code and put it into v2 of the patchset with your
> sign-off, is it OK?
I'd rather do it a bit differently in the signed-off version (I'm working
on these patches, they should be ready around Tuesday), but if you can test
it in its current form, that'd be useful too.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox