* Re: [alsa-devel] [PATCH] usb: add USB_QUIRK_RESET_RESUME for M-Audio 49
From: Clemens Ladisch @ 2012-11-25 22:01 UTC (permalink / raw)
To: Jonathan Nieder
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, Steffen Müller,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-pm-u79uwXL29TY76Z2rM5mHXA, Olivier MATZ,
stable-u79uwXL29TY76Z2rM5mHXA, David Banks, Ralf Lang,
Oliver Neukum
In-Reply-To: <20121125212100.GE24024-fcEM2ccDkbL2nhBuCrrZHw@public.gmane.org>
Jonathan Nieder wrote:
> Some USB MIDI keyboards fail to operate after a USB autosuspend.
Make that *all* USB MIDI devices with input ports.
This is not a bug in the device, but one of the many bugs introduced
with the autosuspend code in <http://git.kernel.org/linus/88a8516a2128>.
That patch does not handle input at all, i.e., when the driver wants to
read from the device, it just doesn't take it out of suspend mode.
> A workaround is to disable USB autosuspend for these devices by
> putting AUTOSUSPEND_USBID_BLACKLIST="0763:2027" (resp. 0763:019b) in
> /etc/laptop-mode/conf.d/usb-autosuspend.conf. In the spirit of commit
> 166cb70e97bd ("usb: add USB_QUIRK_RESET_RESUME for M-Audio 88es"),
> reset the device on resume so this workaround is not needed any more.
It is not feasible to add the IDs of all USB MIDI devices.
I'm working on a fix that adds proper power management for input ports,
but this requires the driver to be reorganized a little ...
Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Rafael J. Wysocki @ 2012-11-26 0:33 UTC (permalink / raw)
To: James Bottomley
Cc: Tejun Heo, Aaron Lu, Jeff Garzik, Alan Stern, Jeff Wu, Aaron Lu,
linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <1353337611.2444.32.camel@dabdike.int.hansenpartnership.com>
On Monday, November 19, 2012 03:06:51 PM James Bottomley wrote:
> On Mon, 2012-11-19 at 06:56 -0800, Tejun Heo wrote:
> > Hey, Aaron.
> >
> > On Mon, Nov 19, 2012 at 11:09:40AM +0800, Aaron Lu wrote:
> > > > What I'm confused about is what autopm does for devices w/o zpodd.
> > > > What happens then? Is it gonna leave power on for the device and,
> > > > say, go on to suspend the controller? But, how would that work for,
> > > > say, future devices with async notification for media events?
> > >
> > > Maybe we shouldn't allow autopm for such devices?
> >
> > Yeah, maybe. It would be nice to be able to automatically power off
> > disks and ports which aren't being used tho.
> >
> > > > That said, I can't say the snooping is pretty. It's a rather nasty
> > > > thing to do. So, libata now wants information from the event polling
> > > > in block layer, but reaching for block_device from ata_devices is
> > > > nasty too. Hmmm... but aren't you already doing that to block polling
> > > > on a powered down device?
> > >
> > > I was feeling brain damaged by this for some time :-)
> > >
> > > Basically, only ATA layer is aware of the power off thing, and sr knows
> > > nothing about this(or it is not supposed to know this, at least, this is
> > > what SCSI people think) and once powered off, I do not want the poll to
> > > disturb the device, so I need to block the poll. I can't come up with
> > > another way to achieve this except this nasty way.
> > >
> > > James suggests me to keep the poll, but emulate the command. The problem
> > > with this is, the autopm for resume will kick in on each poll, so I'll
> > > need to decide if power up the ODD for this time's resume is needed in
> > > port's runtime resume callback. This made things complex and it also put
> > > too much logic in the resume callback, which is not desired. And even if
> > > I keep the ODD in powered off state by emulating this poll command, its
> > > ancestor devices will still be resumed, and I may need to do some trick
> > > in their resume callback to avoid needless power/state transitions. This
> > > doesn't feel like an elegant way to solve this either.
> > >
> > > So yes, I'm still using this _nasty_ way to make the ODD stay in powered
> > > off state as long as possible. But if there is other elegant ways to
> > > solve this, I would appreciate it and happily using it. Personally, I
> > > hope we can make sr aware of ZPODD, that would make the pain gone.
> >
> > I really think we need a way for (auto)pm and event polling to talk to
> > each other so that autopm can tell event poll to sod off while pm is
> > in effect. Trying to solve this from inside libata doesn't seem
> > right. The problem, again, seems to be figuring out which hardware
> > device maps to which block device. Hmmm... Any good ideas?
>
> I've asked the PM people several times about this, because it's a real
> problem for almost everything: PM needs some type of top to bottom
> stack view, which the layering isolation we have within storage really
> doesn't cope with well. No real suggestion has been forthcoming.
Actually, I think that the particular case in question is really special
and the fact that there's the pollig loop that user space is involved in
doesn't make things more stratightforward.
And PM really doesn't need to see things top to bottom, but the polling
needs to know what happens in the PM land. We need to be able to tell it
"from now on tell user space that there are no events here". The question
is where to put that information so that it's accessible to all parts of the
stack involved.
> The reason I think it should be emulated (in the acpi layer, not libata,
> but as long as it's not in SCSI, I'm not so fussed where it ends up) is
> because ZPODD is the software equivalent of ZPREADY, which will be done
> in hardware and will be effectively invisible to autopm in the same way
> that SCSI (and ATA) power management is mostly invisible. If we
> currently do ZPREADY emulation in the low layer (i.e. ZPODD has exact
> ZPREADY emulation), we won't care (except for flipping the sofware bit)
> whether the device support ZPODD or ZPREADY and it will all just
> work(tm). The industry expectation is that ZPODD is just a transition
> state between current power management and ZPREADY.
Well, if you poll a ZPREADY-capable drive, it will go off and on in cycles
transparently, but still it won't save as much energy as it can. We'll need
to do something about the polling in that case too, it seems.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states
From: Huang Ying @ 2012-11-26 0:43 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Greg Kroah-Hartman, Linux PM list, ACPI Devel Maling List,
Zhang Rui, Svahn, Kai, Mika Westerberg, Lan, Tianyu, Zheng, Lv,
Aaron Lu, Grant Likely
In-Reply-To: <2576372.XKUY0b7gBl@vostro.rjw.lan>
On Sun, 2012-11-25 at 15:55 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Make it possible to ask the routines used for adding/removing devices
> to/from the general ACPI PM domain, acpi_dev_pm_attach() and
> acpi_dev_pm_detach(), respectively, to change the power states of
> devices so that they are put into the full-power state automatically
> by acpi_dev_pm_attach() and into the lowest-power state available
> automatically by acpi_dev_pm_detach().
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/acpi/device_pm.c | 28 ++++++++++++++++++++++++----
> include/linux/acpi.h | 11 +++++++----
> 2 files changed, 31 insertions(+), 8 deletions(-)
>
> Index: linux/drivers/acpi/device_pm.c
> ===================================================================
> --- linux.orig/drivers/acpi/device_pm.c
> +++ linux/drivers/acpi/device_pm.c
> @@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
> /**
> * acpi_dev_pm_attach - Prepare device for ACPI power management.
> * @dev: Device to prepare.
> + * @power_on: Whether or not to power on the device.
> *
> * If @dev has a valid ACPI handle that has a valid struct acpi_device object
> * attached to it, install a wakeup notification handler for the device and
> - * add it to the general ACPI PM domain.
> + * add it to the general ACPI PM domain. If @power_on is set, the device will
> + * be put into the ACPI D0 state before the function returns.
> *
> * This assumes that the @dev's bus type uses generic power management callbacks
> * (or doesn't use any power management callbacks at all).
> @@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
> * Callers must ensure proper synchronization of this function with power
> * management callbacks.
> */
> -int acpi_dev_pm_attach(struct device *dev)
> +int acpi_dev_pm_attach(struct device *dev, bool power_on)
> {
> struct acpi_device *adev = acpi_dev_pm_get_node(dev);
>
> @@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
>
> acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
> dev->pm_domain = &acpi_general_pm_domain;
> + if (power_on) {
> + acpi_dev_pm_full_power(adev);
> + __acpi_device_run_wake(adev, false);
> + }
> return 0;
> }
> EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> @@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> /**
> * acpi_dev_pm_detach - Remove ACPI power management from the device.
> * @dev: Device to take care of.
> + * @power_off: Whether or not to try to remove power from the device.
> *
> * Remove the device from the general ACPI PM domain and remove its wakeup
> - * notifier.
> + * notifier. If @power_off is set, additionally remove power from the device if
> + * possible.
> *
> * Callers must ensure proper synchronization of this function with power
> * management callbacks.
> */
> -void acpi_dev_pm_detach(struct device *dev)
> +void acpi_dev_pm_detach(struct device *dev, bool power_off)
> {
> struct acpi_device *adev = acpi_dev_pm_get_node(dev);
>
> if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> dev->pm_domain = NULL;
> acpi_remove_pm_notifier(adev, acpi_wakeup_device);
> + if (power_off) {
> + /*
> + * If the device's PM QoS resume latency limit or flags
> + * have been exposed to user space, they have to be
> + * hidden at this point, so that they don't affect the
> + * choice of the low-power state to put the device into.
> + */
> + dev_pm_qos_hide_latency_limit(dev);
> + dev_pm_qos_hide_flags(dev);
NO_POWER_OFF flag is ignored here. Is it possible for some device (or
corresponding ACPI method) has broken D3cold implementation, so that the
user need a way to disable it?
Best Regards,
Huang Ying
> + __acpi_device_run_wake(adev, false);
> + acpi_dev_pm_low_power(dev, adev, ACPI_STATE_S0);
> + }
> }
> }
> EXPORT_SYMBOL_GPL(acpi_dev_pm_detach);
> Index: linux/include/linux/acpi.h
> ===================================================================
> --- linux.orig/include/linux/acpi.h
> +++ linux/include/linux/acpi.h
> @@ -510,11 +510,14 @@ static inline int acpi_subsys_resume_ear
> #endif
>
> #if defined(CONFIG_ACPI) && defined(CONFIG_PM)
> -int acpi_dev_pm_attach(struct device *dev);
> -int acpi_dev_pm_detach(struct device *dev);
> +int acpi_dev_pm_attach(struct device *dev, bool power_on);
> +int acpi_dev_pm_detach(struct device *dev, bool power_off);
> #else
> -static inline int acpi_dev_pm_attach(struct device *dev) { return -ENODEV; }
> -static inline void acpi_dev_pm_detach(struct device *dev) {}
> +static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
> +{
> + return -ENODEV;
> +}
> +static inline void acpi_dev_pm_detach(struct device *dev, bool power_off) {}
> #endif
>
> #ifdef CONFIG_ACPI
>
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Aaron Lu @ 2012-11-26 0:45 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: James Bottomley, Tejun Heo, Jeff Garzik, Alan Stern, Jeff Wu,
Aaron Lu, linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <35648985.61QNrr0Knq@vostro.rjw.lan>
On 11/26/2012 08:33 AM, Rafael J. Wysocki wrote:
> On Monday, November 19, 2012 03:06:51 PM James Bottomley wrote:
>> On Mon, 2012-11-19 at 06:56 -0800, Tejun Heo wrote:
>>> Hey, Aaron.
>>>
>>> On Mon, Nov 19, 2012 at 11:09:40AM +0800, Aaron Lu wrote:
>>>>> What I'm confused about is what autopm does for devices w/o zpodd.
>>>>> What happens then? Is it gonna leave power on for the device and,
>>>>> say, go on to suspend the controller? But, how would that work for,
>>>>> say, future devices with async notification for media events?
>>>>
>>>> Maybe we shouldn't allow autopm for such devices?
>>>
>>> Yeah, maybe. It would be nice to be able to automatically power off
>>> disks and ports which aren't being used tho.
>>>
>>>>> That said, I can't say the snooping is pretty. It's a rather nasty
>>>>> thing to do. So, libata now wants information from the event polling
>>>>> in block layer, but reaching for block_device from ata_devices is
>>>>> nasty too. Hmmm... but aren't you already doing that to block polling
>>>>> on a powered down device?
>>>>
>>>> I was feeling brain damaged by this for some time :-)
>>>>
>>>> Basically, only ATA layer is aware of the power off thing, and sr knows
>>>> nothing about this(or it is not supposed to know this, at least, this is
>>>> what SCSI people think) and once powered off, I do not want the poll to
>>>> disturb the device, so I need to block the poll. I can't come up with
>>>> another way to achieve this except this nasty way.
>>>>
>>>> James suggests me to keep the poll, but emulate the command. The problem
>>>> with this is, the autopm for resume will kick in on each poll, so I'll
>>>> need to decide if power up the ODD for this time's resume is needed in
>>>> port's runtime resume callback. This made things complex and it also put
>>>> too much logic in the resume callback, which is not desired. And even if
>>>> I keep the ODD in powered off state by emulating this poll command, its
>>>> ancestor devices will still be resumed, and I may need to do some trick
>>>> in their resume callback to avoid needless power/state transitions. This
>>>> doesn't feel like an elegant way to solve this either.
>>>>
>>>> So yes, I'm still using this _nasty_ way to make the ODD stay in powered
>>>> off state as long as possible. But if there is other elegant ways to
>>>> solve this, I would appreciate it and happily using it. Personally, I
>>>> hope we can make sr aware of ZPODD, that would make the pain gone.
>>>
>>> I really think we need a way for (auto)pm and event polling to talk to
>>> each other so that autopm can tell event poll to sod off while pm is
>>> in effect. Trying to solve this from inside libata doesn't seem
>>> right. The problem, again, seems to be figuring out which hardware
>>> device maps to which block device. Hmmm... Any good ideas?
>>
>> I've asked the PM people several times about this, because it's a real
>> problem for almost everything: PM needs some type of top to bottom
>> stack view, which the layering isolation we have within storage really
>> doesn't cope with well. No real suggestion has been forthcoming.
>
> Actually, I think that the particular case in question is really special
> and the fact that there's the pollig loop that user space is involved in
> doesn't make things more stratightforward.
>
> And PM really doesn't need to see things top to bottom, but the polling
> needs to know what happens in the PM land. We need to be able to tell it
> "from now on tell user space that there are no events here". The question
Actually, in newer kernels(2.6.38 later) and user space tools(udisks
version 1.0.3 on), this is no longer the case.
udisks now no longer poll for event change, and in kernel polling has
been used to notify user space about event change with uevent. So the
polling thing can be entirely controlled in kernel space.
And please take a look at the v10 patch I've sent where I tried to solve
this by introducing PM_QOS_NO_POLL flag, see if that makes sense to you.
Thanks,
Aaron
> is where to put that information so that it's accessible to all parts of the
> stack involved.
>
>> The reason I think it should be emulated (in the acpi layer, not libata,
>> but as long as it's not in SCSI, I'm not so fussed where it ends up) is
>> because ZPODD is the software equivalent of ZPREADY, which will be done
>> in hardware and will be effectively invisible to autopm in the same way
>> that SCSI (and ATA) power management is mostly invisible. If we
>> currently do ZPREADY emulation in the low layer (i.e. ZPODD has exact
>> ZPREADY emulation), we won't care (except for flipping the sofware bit)
>> whether the device support ZPODD or ZPREADY and it will all just
>> work(tm). The industry expectation is that ZPODD is just a transition
>> state between current power management and ZPREADY.
>
> Well, if you poll a ZPREADY-capable drive, it will go off and on in cycles
> transparently, but still it won't save as much energy as it can. We'll need
> to do something about the polling in that case too, it seems.
>
> Thanks,
> Rafael
>
>
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Aaron Lu @ 2012-11-26 0:48 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tejun Heo, Alan Stern, Jeff Garzik, James Bottomley, Jeff Wu,
linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <6529392.v4vtIGQYok@vostro.rjw.lan>
On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote:
> On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote:
>> On 11/20/2012 02:00 PM, Aaron Lu wrote:
>>> On 11/19/2012 10:56 PM, Tejun Heo wrote:
>>>> I really think we need a way for (auto)pm and event polling to talk to
>>>> each other so that autopm can tell event poll to sod off while pm is
>>>> in effect. Trying to solve this from inside libata doesn't seem
>>>> right. The problem, again, seems to be figuring out which hardware
>>>> device maps to which block device. Hmmm... Any good ideas?
>>>
>>> A possible way of doing this is using pm qos.
>>>
>>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we
>>> can add another one: NO_POLL, use it like the following:
>>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
>>> longer necessary. In the ZPODD's case, it should be set when the
>>> device is to be powered off;
>>> 2 Clear it when poll is necessary again. In the ZPODD's case, when power
>>> is re-gained, this flag will be cleared.
>>
>>
>>> 3 In the disk_events_workfn, check if this flag is set, if so, simply
>>> return.
>>
>> It should be, skip calling disk->fops->check_events, but still queue the
>> work for next time's poll.
>>
>> -Aaron
>>
>>>
>>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer
>>> can access it through ata_device->sdev->sdev_gendev.
>>>
>>> Is this OK?
>
> No, I don't think so. PM QoS is about telling the layer that will put the
> device into low-power states what states are to be taken into consideration.
> In this case, however, we need to tell someone else that the device has been
> turned off. Clearly, we need a way to do that, but not through PM QoS.
>
> Did you consider using pm_runtime_suspended() to check the device status?
The problem is, a device can be in runtime suspended state while still
needs to be polled...
Thanks,
Aaron
>
> Rafael
>
>
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Rafael J. Wysocki @ 2012-11-26 0:50 UTC (permalink / raw)
To: Aaron Lu
Cc: Tejun Heo, Alan Stern, Jeff Garzik, James Bottomley, Jeff Wu,
linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <50AB468D.1060902@intel.com>
On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote:
> On 11/20/2012 02:00 PM, Aaron Lu wrote:
> > On 11/19/2012 10:56 PM, Tejun Heo wrote:
> >> I really think we need a way for (auto)pm and event polling to talk to
> >> each other so that autopm can tell event poll to sod off while pm is
> >> in effect. Trying to solve this from inside libata doesn't seem
> >> right. The problem, again, seems to be figuring out which hardware
> >> device maps to which block device. Hmmm... Any good ideas?
> >
> > A possible way of doing this is using pm qos.
> >
> > We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we
> > can add another one: NO_POLL, use it like the following:
> > 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
> > longer necessary. In the ZPODD's case, it should be set when the
> > device is to be powered off;
> > 2 Clear it when poll is necessary again. In the ZPODD's case, when power
> > is re-gained, this flag will be cleared.
>
>
> > 3 In the disk_events_workfn, check if this flag is set, if so, simply
> > return.
>
> It should be, skip calling disk->fops->check_events, but still queue the
> work for next time's poll.
>
> -Aaron
>
> >
> > The disk->driverfs_dev can be used to host the pm qos flag, ATA layer
> > can access it through ata_device->sdev->sdev_gendev.
> >
> > Is this OK?
No, I don't think so. PM QoS is about telling the layer that will put the
device into low-power states what states are to be taken into consideration.
In this case, however, we need to tell someone else that the device has been
turned off. Clearly, we need a way to do that, but not through PM QoS.
Did you consider using pm_runtime_suspended() to check the device status?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states
From: Rafael J. Wysocki @ 2012-11-26 1:00 UTC (permalink / raw)
To: Huang Ying
Cc: LKML, Greg Kroah-Hartman, Linux PM list, ACPI Devel Maling List,
Zhang Rui, Svahn, Kai, Mika Westerberg, Lan, Tianyu, Zheng, Lv,
Aaron Lu, Grant Likely
In-Reply-To: <1353890590.28789.68.camel@yhuang-dev>
On Monday, November 26, 2012 08:43:10 AM Huang Ying wrote:
> On Sun, 2012-11-25 at 15:55 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Make it possible to ask the routines used for adding/removing devices
> > to/from the general ACPI PM domain, acpi_dev_pm_attach() and
> > acpi_dev_pm_detach(), respectively, to change the power states of
> > devices so that they are put into the full-power state automatically
> > by acpi_dev_pm_attach() and into the lowest-power state available
> > automatically by acpi_dev_pm_detach().
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > drivers/acpi/device_pm.c | 28 ++++++++++++++++++++++++----
> > include/linux/acpi.h | 11 +++++++----
> > 2 files changed, 31 insertions(+), 8 deletions(-)
> >
> > Index: linux/drivers/acpi/device_pm.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/device_pm.c
> > +++ linux/drivers/acpi/device_pm.c
> > @@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
> > /**
> > * acpi_dev_pm_attach - Prepare device for ACPI power management.
> > * @dev: Device to prepare.
> > + * @power_on: Whether or not to power on the device.
> > *
> > * If @dev has a valid ACPI handle that has a valid struct acpi_device object
> > * attached to it, install a wakeup notification handler for the device and
> > - * add it to the general ACPI PM domain.
> > + * add it to the general ACPI PM domain. If @power_on is set, the device will
> > + * be put into the ACPI D0 state before the function returns.
> > *
> > * This assumes that the @dev's bus type uses generic power management callbacks
> > * (or doesn't use any power management callbacks at all).
> > @@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
> > * Callers must ensure proper synchronization of this function with power
> > * management callbacks.
> > */
> > -int acpi_dev_pm_attach(struct device *dev)
> > +int acpi_dev_pm_attach(struct device *dev, bool power_on)
> > {
> > struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> >
> > @@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
> >
> > acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
> > dev->pm_domain = &acpi_general_pm_domain;
> > + if (power_on) {
> > + acpi_dev_pm_full_power(adev);
> > + __acpi_device_run_wake(adev, false);
> > + }
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > @@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > /**
> > * acpi_dev_pm_detach - Remove ACPI power management from the device.
> > * @dev: Device to take care of.
> > + * @power_off: Whether or not to try to remove power from the device.
> > *
> > * Remove the device from the general ACPI PM domain and remove its wakeup
> > - * notifier.
> > + * notifier. If @power_off is set, additionally remove power from the device if
> > + * possible.
> > *
> > * Callers must ensure proper synchronization of this function with power
> > * management callbacks.
> > */
> > -void acpi_dev_pm_detach(struct device *dev)
> > +void acpi_dev_pm_detach(struct device *dev, bool power_off)
> > {
> > struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> >
> > if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> > dev->pm_domain = NULL;
> > acpi_remove_pm_notifier(adev, acpi_wakeup_device);
> > + if (power_off) {
> > + /*
> > + * If the device's PM QoS resume latency limit or flags
> > + * have been exposed to user space, they have to be
> > + * hidden at this point, so that they don't affect the
> > + * choice of the low-power state to put the device into.
> > + */
> > + dev_pm_qos_hide_latency_limit(dev);
> > + dev_pm_qos_hide_flags(dev);
>
> NO_POWER_OFF flag is ignored here. Is it possible for some device (or
> corresponding ACPI method) has broken D3cold implementation, so that the
> user need a way to disable it?
We are removing the driver here and we haven't exposed the flags, have we?
If the driver had exposed them and then left them behind, we need to clean up
after it and that's why I added those two lines.
In the future we may decide that the flags need to be exposed by the core,
because of the broken hardware, for example, before the driver's .probe()
routine is run and in that case we obviously won't be removing them here any
more. For now, however, I think it's better to hide them here.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Rafael J. Wysocki @ 2012-11-26 1:03 UTC (permalink / raw)
To: Aaron Lu
Cc: Tejun Heo, Alan Stern, Jeff Garzik, James Bottomley, Jeff Wu,
linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <50B2BC73.5010905@intel.com>
On Monday, November 26, 2012 08:48:51 AM Aaron Lu wrote:
> On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote:
> > On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote:
> >> On 11/20/2012 02:00 PM, Aaron Lu wrote:
> >>> On 11/19/2012 10:56 PM, Tejun Heo wrote:
> >>>> I really think we need a way for (auto)pm and event polling to talk to
> >>>> each other so that autopm can tell event poll to sod off while pm is
> >>>> in effect. Trying to solve this from inside libata doesn't seem
> >>>> right. The problem, again, seems to be figuring out which hardware
> >>>> device maps to which block device. Hmmm... Any good ideas?
> >>>
> >>> A possible way of doing this is using pm qos.
> >>>
> >>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we
> >>> can add another one: NO_POLL, use it like the following:
> >>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
> >>> longer necessary. In the ZPODD's case, it should be set when the
> >>> device is to be powered off;
> >>> 2 Clear it when poll is necessary again. In the ZPODD's case, when power
> >>> is re-gained, this flag will be cleared.
> >>
> >>
> >>> 3 In the disk_events_workfn, check if this flag is set, if so, simply
> >>> return.
> >>
> >> It should be, skip calling disk->fops->check_events, but still queue the
> >> work for next time's poll.
> >>
> >> -Aaron
> >>
> >>>
> >>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer
> >>> can access it through ata_device->sdev->sdev_gendev.
> >>>
> >>> Is this OK?
> >
> > No, I don't think so. PM QoS is about telling the layer that will put the
> > device into low-power states what states are to be taken into consideration.
> > In this case, however, we need to tell someone else that the device has been
> > turned off. Clearly, we need a way to do that, but not through PM QoS.
> >
> > Did you consider using pm_runtime_suspended() to check the device status?
>
> The problem is, a device can be in runtime suspended state while still
> needs to be polled...
Well, maybe this is the problem, then? Why does it need to be polled when
suspended?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Aaron Lu @ 2012-11-26 1:05 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tejun Heo, Alan Stern, Jeff Garzik, James Bottomley, Jeff Wu,
linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <3650037.tRRn9I6XAb@vostro.rjw.lan>
On 11/26/2012 09:03 AM, Rafael J. Wysocki wrote:
> On Monday, November 26, 2012 08:48:51 AM Aaron Lu wrote:
>> On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote:
>>> On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote:
>>>> On 11/20/2012 02:00 PM, Aaron Lu wrote:
>>>>> On 11/19/2012 10:56 PM, Tejun Heo wrote:
>>>>>> I really think we need a way for (auto)pm and event polling to talk to
>>>>>> each other so that autopm can tell event poll to sod off while pm is
>>>>>> in effect. Trying to solve this from inside libata doesn't seem
>>>>>> right. The problem, again, seems to be figuring out which hardware
>>>>>> device maps to which block device. Hmmm... Any good ideas?
>>>>>
>>>>> A possible way of doing this is using pm qos.
>>>>>
>>>>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we
>>>>> can add another one: NO_POLL, use it like the following:
>>>>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
>>>>> longer necessary. In the ZPODD's case, it should be set when the
>>>>> device is to be powered off;
>>>>> 2 Clear it when poll is necessary again. In the ZPODD's case, when power
>>>>> is re-gained, this flag will be cleared.
>>>>
>>>>
>>>>> 3 In the disk_events_workfn, check if this flag is set, if so, simply
>>>>> return.
>>>>
>>>> It should be, skip calling disk->fops->check_events, but still queue the
>>>> work for next time's poll.
>>>>
>>>> -Aaron
>>>>
>>>>>
>>>>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer
>>>>> can access it through ata_device->sdev->sdev_gendev.
>>>>>
>>>>> Is this OK?
>>>
>>> No, I don't think so. PM QoS is about telling the layer that will put the
>>> device into low-power states what states are to be taken into consideration.
>>> In this case, however, we need to tell someone else that the device has been
>>> turned off. Clearly, we need a way to do that, but not through PM QoS.
>>>
>>> Did you consider using pm_runtime_suspended() to check the device status?
>>
>> The problem is, a device can be in runtime suspended state while still
>> needs to be polled...
>
> Well, maybe this is the problem, then? Why does it need to be polled when
> suspended?
For ODDs, poll is not necessary only when ZP capable ODD is powered off.
For other ODDs, poll still needs to go on.
ZP capable ODDs:
- runtime suspended, power remained(due to NO_POWER_OFF qos flag)
poll is needed
-- runtime suspended, power removed
poll is not needed
Non ZP capable ODDs:
-- runtime suspended, power remained (power will never be removed)
poll is needed
If we do not poll for the powered on case, we will lose media change
event.
Thanks,
Aaron
>
> Rafael
>
>
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Rafael J. Wysocki @ 2012-11-26 1:11 UTC (permalink / raw)
To: Aaron Lu
Cc: Tejun Heo, Alan Stern, Jeff Garzik, James Bottomley, Jeff Wu,
linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <50B2C076.3070201@intel.com>
On Monday, November 26, 2012 09:05:58 AM Aaron Lu wrote:
> On 11/26/2012 09:03 AM, Rafael J. Wysocki wrote:
> > On Monday, November 26, 2012 08:48:51 AM Aaron Lu wrote:
> >> On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote:
> >>> On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote:
> >>>> On 11/20/2012 02:00 PM, Aaron Lu wrote:
> >>>>> On 11/19/2012 10:56 PM, Tejun Heo wrote:
> >>>>>> I really think we need a way for (auto)pm and event polling to talk to
> >>>>>> each other so that autopm can tell event poll to sod off while pm is
> >>>>>> in effect. Trying to solve this from inside libata doesn't seem
> >>>>>> right. The problem, again, seems to be figuring out which hardware
> >>>>>> device maps to which block device. Hmmm... Any good ideas?
> >>>>>
> >>>>> A possible way of doing this is using pm qos.
> >>>>>
> >>>>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we
> >>>>> can add another one: NO_POLL, use it like the following:
> >>>>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
> >>>>> longer necessary. In the ZPODD's case, it should be set when the
> >>>>> device is to be powered off;
> >>>>> 2 Clear it when poll is necessary again. In the ZPODD's case, when power
> >>>>> is re-gained, this flag will be cleared.
> >>>>
> >>>>
> >>>>> 3 In the disk_events_workfn, check if this flag is set, if so, simply
> >>>>> return.
> >>>>
> >>>> It should be, skip calling disk->fops->check_events, but still queue the
> >>>> work for next time's poll.
> >>>>
> >>>> -Aaron
> >>>>
> >>>>>
> >>>>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer
> >>>>> can access it through ata_device->sdev->sdev_gendev.
> >>>>>
> >>>>> Is this OK?
> >>>
> >>> No, I don't think so. PM QoS is about telling the layer that will put the
> >>> device into low-power states what states are to be taken into consideration.
> >>> In this case, however, we need to tell someone else that the device has been
> >>> turned off. Clearly, we need a way to do that, but not through PM QoS.
> >>>
> >>> Did you consider using pm_runtime_suspended() to check the device status?
> >>
> >> The problem is, a device can be in runtime suspended state while still
> >> needs to be polled...
> >
> > Well, maybe this is the problem, then? Why does it need to be polled when
> > suspended?
>
> For ODDs, poll is not necessary only when ZP capable ODD is powered off.
> For other ODDs, poll still needs to go on.
>
> ZP capable ODDs:
> - runtime suspended, power remained(due to NO_POWER_OFF qos flag)
> poll is needed
> -- runtime suspended, power removed
> poll is not needed
>
> Non ZP capable ODDs:
> -- runtime suspended, power remained (power will never be removed)
> poll is needed
>
> If we do not poll for the powered on case, we will lose media change
> event.
But the media change event should change the status from suspended to active,
shouldn't it?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states
From: Huang Ying @ 2012-11-26 1:07 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Greg Kroah-Hartman, Linux PM list, ACPI Devel Maling List,
Zhang Rui, Svahn, Kai, Mika Westerberg, Lan, Tianyu, Zheng, Lv,
Aaron Lu, Grant Likely
In-Reply-To: <4238031.sDhWdlxYNQ@vostro.rjw.lan>
On Mon, 2012-11-26 at 02:00 +0100, Rafael J. Wysocki wrote:
> On Monday, November 26, 2012 08:43:10 AM Huang Ying wrote:
> > On Sun, 2012-11-25 at 15:55 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Make it possible to ask the routines used for adding/removing devices
> > > to/from the general ACPI PM domain, acpi_dev_pm_attach() and
> > > acpi_dev_pm_detach(), respectively, to change the power states of
> > > devices so that they are put into the full-power state automatically
> > > by acpi_dev_pm_attach() and into the lowest-power state available
> > > automatically by acpi_dev_pm_detach().
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > > drivers/acpi/device_pm.c | 28 ++++++++++++++++++++++++----
> > > include/linux/acpi.h | 11 +++++++----
> > > 2 files changed, 31 insertions(+), 8 deletions(-)
> > >
> > > Index: linux/drivers/acpi/device_pm.c
> > > ===================================================================
> > > --- linux.orig/drivers/acpi/device_pm.c
> > > +++ linux/drivers/acpi/device_pm.c
> > > @@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
> > > /**
> > > * acpi_dev_pm_attach - Prepare device for ACPI power management.
> > > * @dev: Device to prepare.
> > > + * @power_on: Whether or not to power on the device.
> > > *
> > > * If @dev has a valid ACPI handle that has a valid struct acpi_device object
> > > * attached to it, install a wakeup notification handler for the device and
> > > - * add it to the general ACPI PM domain.
> > > + * add it to the general ACPI PM domain. If @power_on is set, the device will
> > > + * be put into the ACPI D0 state before the function returns.
> > > *
> > > * This assumes that the @dev's bus type uses generic power management callbacks
> > > * (or doesn't use any power management callbacks at all).
> > > @@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
> > > * Callers must ensure proper synchronization of this function with power
> > > * management callbacks.
> > > */
> > > -int acpi_dev_pm_attach(struct device *dev)
> > > +int acpi_dev_pm_attach(struct device *dev, bool power_on)
> > > {
> > > struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > >
> > > @@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
> > >
> > > acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
> > > dev->pm_domain = &acpi_general_pm_domain;
> > > + if (power_on) {
> > > + acpi_dev_pm_full_power(adev);
> > > + __acpi_device_run_wake(adev, false);
> > > + }
> > > return 0;
> > > }
> > > EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > @@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > /**
> > > * acpi_dev_pm_detach - Remove ACPI power management from the device.
> > > * @dev: Device to take care of.
> > > + * @power_off: Whether or not to try to remove power from the device.
> > > *
> > > * Remove the device from the general ACPI PM domain and remove its wakeup
> > > - * notifier.
> > > + * notifier. If @power_off is set, additionally remove power from the device if
> > > + * possible.
> > > *
> > > * Callers must ensure proper synchronization of this function with power
> > > * management callbacks.
> > > */
> > > -void acpi_dev_pm_detach(struct device *dev)
> > > +void acpi_dev_pm_detach(struct device *dev, bool power_off)
> > > {
> > > struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > >
> > > if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> > > dev->pm_domain = NULL;
> > > acpi_remove_pm_notifier(adev, acpi_wakeup_device);
> > > + if (power_off) {
> > > + /*
> > > + * If the device's PM QoS resume latency limit or flags
> > > + * have been exposed to user space, they have to be
> > > + * hidden at this point, so that they don't affect the
> > > + * choice of the low-power state to put the device into.
> > > + */
> > > + dev_pm_qos_hide_latency_limit(dev);
> > > + dev_pm_qos_hide_flags(dev);
> >
> > NO_POWER_OFF flag is ignored here. Is it possible for some device (or
> > corresponding ACPI method) has broken D3cold implementation, so that the
> > user need a way to disable it?
>
> We are removing the driver here and we haven't exposed the flags, have we?
IMHO, the flag may be exposed by the bus instead of device driver.
Because some bus can determine whether D3cold is supported by the
device. If the sysfs file for the flag is exposed by the bus, user set
it and we silently ignore it, will it cause confusing for the user?
Best Regards,
Huang Ying
> If the driver had exposed them and then left them behind, we need to clean up
> after it and that's why I added those two lines.
>
> In the future we may decide that the flags need to be exposed by the core,
> because of the broken hardware, for example, before the driver's .probe()
> routine is run and in that case we obviously won't be removing them here any
> more. For now, however, I think it's better to hide them here.
>
> Thanks,
> Rafael
>
>
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Aaron Lu @ 2012-11-26 1:09 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tejun Heo, Alan Stern, Jeff Garzik, James Bottomley, Jeff Wu,
linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <1543239.67ZUIKlEE3@vostro.rjw.lan>
On 11/26/2012 09:11 AM, Rafael J. Wysocki wrote:
> On Monday, November 26, 2012 09:05:58 AM Aaron Lu wrote:
>> On 11/26/2012 09:03 AM, Rafael J. Wysocki wrote:
>>> On Monday, November 26, 2012 08:48:51 AM Aaron Lu wrote:
>>>> On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote:
>>>>> On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote:
>>>>>> On 11/20/2012 02:00 PM, Aaron Lu wrote:
>>>>>>> On 11/19/2012 10:56 PM, Tejun Heo wrote:
>>>>>>>> I really think we need a way for (auto)pm and event polling to talk to
>>>>>>>> each other so that autopm can tell event poll to sod off while pm is
>>>>>>>> in effect. Trying to solve this from inside libata doesn't seem
>>>>>>>> right. The problem, again, seems to be figuring out which hardware
>>>>>>>> device maps to which block device. Hmmm... Any good ideas?
>>>>>>>
>>>>>>> A possible way of doing this is using pm qos.
>>>>>>>
>>>>>>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we
>>>>>>> can add another one: NO_POLL, use it like the following:
>>>>>>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
>>>>>>> longer necessary. In the ZPODD's case, it should be set when the
>>>>>>> device is to be powered off;
>>>>>>> 2 Clear it when poll is necessary again. In the ZPODD's case, when power
>>>>>>> is re-gained, this flag will be cleared.
>>>>>>
>>>>>>
>>>>>>> 3 In the disk_events_workfn, check if this flag is set, if so, simply
>>>>>>> return.
>>>>>>
>>>>>> It should be, skip calling disk->fops->check_events, but still queue the
>>>>>> work for next time's poll.
>>>>>>
>>>>>> -Aaron
>>>>>>
>>>>>>>
>>>>>>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer
>>>>>>> can access it through ata_device->sdev->sdev_gendev.
>>>>>>>
>>>>>>> Is this OK?
>>>>>
>>>>> No, I don't think so. PM QoS is about telling the layer that will put the
>>>>> device into low-power states what states are to be taken into consideration.
>>>>> In this case, however, we need to tell someone else that the device has been
>>>>> turned off. Clearly, we need a way to do that, but not through PM QoS.
>>>>>
>>>>> Did you consider using pm_runtime_suspended() to check the device status?
>>>>
>>>> The problem is, a device can be in runtime suspended state while still
>>>> needs to be polled...
>>>
>>> Well, maybe this is the problem, then? Why does it need to be polled when
>>> suspended?
>>
>> For ODDs, poll is not necessary only when ZP capable ODD is powered off.
>> For other ODDs, poll still needs to go on.
>>
>> ZP capable ODDs:
>> - runtime suspended, power remained(due to NO_POWER_OFF qos flag)
>> poll is needed
>> -- runtime suspended, power removed
>> poll is not needed
>>
>> Non ZP capable ODDs:
>> -- runtime suspended, power remained (power will never be removed)
>> poll is needed
>>
>> If we do not poll for the powered on case, we will lose media change
>> event.
>
> But the media change event should change the status from suspended to active,
> shouldn't it?
The media change event is derived from the poll, if we stop the poll, how
can we know the event in the first place?
Thanks,
Aaron
>
> Rafael
>
>
^ permalink raw reply
* Re: [PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states
From: Rafael J. Wysocki @ 2012-11-26 1:16 UTC (permalink / raw)
To: Huang Ying
Cc: LKML, Greg Kroah-Hartman, Linux PM list, ACPI Devel Maling List,
Zhang Rui, Svahn, Kai, Mika Westerberg, Lan, Tianyu, Zheng, Lv,
Aaron Lu, Grant Likely
In-Reply-To: <1353892047.28789.82.camel@yhuang-dev>
On Monday, November 26, 2012 09:07:27 AM Huang Ying wrote:
> On Mon, 2012-11-26 at 02:00 +0100, Rafael J. Wysocki wrote:
> > On Monday, November 26, 2012 08:43:10 AM Huang Ying wrote:
> > > On Sun, 2012-11-25 at 15:55 +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Make it possible to ask the routines used for adding/removing devices
> > > > to/from the general ACPI PM domain, acpi_dev_pm_attach() and
> > > > acpi_dev_pm_detach(), respectively, to change the power states of
> > > > devices so that they are put into the full-power state automatically
> > > > by acpi_dev_pm_attach() and into the lowest-power state available
> > > > automatically by acpi_dev_pm_detach().
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > > drivers/acpi/device_pm.c | 28 ++++++++++++++++++++++++----
> > > > include/linux/acpi.h | 11 +++++++----
> > > > 2 files changed, 31 insertions(+), 8 deletions(-)
> > > >
> > > > Index: linux/drivers/acpi/device_pm.c
> > > > ===================================================================
> > > > --- linux.orig/drivers/acpi/device_pm.c
> > > > +++ linux/drivers/acpi/device_pm.c
> > > > @@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
> > > > /**
> > > > * acpi_dev_pm_attach - Prepare device for ACPI power management.
> > > > * @dev: Device to prepare.
> > > > + * @power_on: Whether or not to power on the device.
> > > > *
> > > > * If @dev has a valid ACPI handle that has a valid struct acpi_device object
> > > > * attached to it, install a wakeup notification handler for the device and
> > > > - * add it to the general ACPI PM domain.
> > > > + * add it to the general ACPI PM domain. If @power_on is set, the device will
> > > > + * be put into the ACPI D0 state before the function returns.
> > > > *
> > > > * This assumes that the @dev's bus type uses generic power management callbacks
> > > > * (or doesn't use any power management callbacks at all).
> > > > @@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
> > > > * Callers must ensure proper synchronization of this function with power
> > > > * management callbacks.
> > > > */
> > > > -int acpi_dev_pm_attach(struct device *dev)
> > > > +int acpi_dev_pm_attach(struct device *dev, bool power_on)
> > > > {
> > > > struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > > >
> > > > @@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
> > > >
> > > > acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
> > > > dev->pm_domain = &acpi_general_pm_domain;
> > > > + if (power_on) {
> > > > + acpi_dev_pm_full_power(adev);
> > > > + __acpi_device_run_wake(adev, false);
> > > > + }
> > > > return 0;
> > > > }
> > > > EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > > @@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > > /**
> > > > * acpi_dev_pm_detach - Remove ACPI power management from the device.
> > > > * @dev: Device to take care of.
> > > > + * @power_off: Whether or not to try to remove power from the device.
> > > > *
> > > > * Remove the device from the general ACPI PM domain and remove its wakeup
> > > > - * notifier.
> > > > + * notifier. If @power_off is set, additionally remove power from the device if
> > > > + * possible.
> > > > *
> > > > * Callers must ensure proper synchronization of this function with power
> > > > * management callbacks.
> > > > */
> > > > -void acpi_dev_pm_detach(struct device *dev)
> > > > +void acpi_dev_pm_detach(struct device *dev, bool power_off)
> > > > {
> > > > struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > > >
> > > > if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> > > > dev->pm_domain = NULL;
> > > > acpi_remove_pm_notifier(adev, acpi_wakeup_device);
> > > > + if (power_off) {
> > > > + /*
> > > > + * If the device's PM QoS resume latency limit or flags
> > > > + * have been exposed to user space, they have to be
> > > > + * hidden at this point, so that they don't affect the
> > > > + * choice of the low-power state to put the device into.
> > > > + */
> > > > + dev_pm_qos_hide_latency_limit(dev);
> > > > + dev_pm_qos_hide_flags(dev);
> > >
> > > NO_POWER_OFF flag is ignored here. Is it possible for some device (or
> > > corresponding ACPI method) has broken D3cold implementation, so that the
> > > user need a way to disable it?
> >
> > We are removing the driver here and we haven't exposed the flags, have we?
>
> IMHO, the flag may be exposed by the bus instead of device driver.
> Because some bus can determine whether D3cold is supported by the
> device. If the sysfs file for the flag is exposed by the bus, user set
> it and we silently ignore it, will it cause confusing for the user?
OK, put it in a different way: If the entity that calls acpi_dev_pm_attach()
exposes the flags _before_ calling it, then acpi_dev_pm_detach() should not
hide the flags. Otherwise, it should hide them. Currently, the only user
of acpi_dev_pm_attach() (that will be the platform bus type) will not expose
the flags before calling that routine, so it is OK for acpi_dev_pm_detach()
to remove them.
Is there anything I'm missing here?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Aaron Lu @ 2012-11-26 1:17 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tejun Heo, Alan Stern, Jeff Garzik, James Bottomley, Jeff Wu,
linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <1543239.67ZUIKlEE3@vostro.rjw.lan>
On 11/26/2012 09:11 AM, Rafael J. Wysocki wrote:
> On Monday, November 26, 2012 09:05:58 AM Aaron Lu wrote:
>> On 11/26/2012 09:03 AM, Rafael J. Wysocki wrote:
>>> On Monday, November 26, 2012 08:48:51 AM Aaron Lu wrote:
>>>> On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote:
>>>>> On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote:
>>>>>> On 11/20/2012 02:00 PM, Aaron Lu wrote:
>>>>>>> On 11/19/2012 10:56 PM, Tejun Heo wrote:
>>>>>>>> I really think we need a way for (auto)pm and event polling to talk to
>>>>>>>> each other so that autopm can tell event poll to sod off while pm is
>>>>>>>> in effect. Trying to solve this from inside libata doesn't seem
>>>>>>>> right. The problem, again, seems to be figuring out which hardware
>>>>>>>> device maps to which block device. Hmmm... Any good ideas?
>>>>>>>
>>>>>>> A possible way of doing this is using pm qos.
>>>>>>>
>>>>>>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we
>>>>>>> can add another one: NO_POLL, use it like the following:
>>>>>>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
>>>>>>> longer necessary. In the ZPODD's case, it should be set when the
>>>>>>> device is to be powered off;
>>>>>>> 2 Clear it when poll is necessary again. In the ZPODD's case, when power
>>>>>>> is re-gained, this flag will be cleared.
>>>>>>
>>>>>>
>>>>>>> 3 In the disk_events_workfn, check if this flag is set, if so, simply
>>>>>>> return.
>>>>>>
>>>>>> It should be, skip calling disk->fops->check_events, but still queue the
>>>>>> work for next time's poll.
>>>>>>
>>>>>> -Aaron
>>>>>>
>>>>>>>
>>>>>>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer
>>>>>>> can access it through ata_device->sdev->sdev_gendev.
>>>>>>>
>>>>>>> Is this OK?
>>>>>
>>>>> No, I don't think so. PM QoS is about telling the layer that will put the
>>>>> device into low-power states what states are to be taken into consideration.
>>>>> In this case, however, we need to tell someone else that the device has been
>>>>> turned off. Clearly, we need a way to do that, but not through PM QoS.
>>>>>
>>>>> Did you consider using pm_runtime_suspended() to check the device status?
>>>>
>>>> The problem is, a device can be in runtime suspended state while still
>>>> needs to be polled...
>>>
>>> Well, maybe this is the problem, then? Why does it need to be polled when
>>> suspended?
>>
>> For ODDs, poll is not necessary only when ZP capable ODD is powered off.
>> For other ODDs, poll still needs to go on.
>>
>> ZP capable ODDs:
>> - runtime suspended, power remained(due to NO_POWER_OFF qos flag)
>> poll is needed
>> -- runtime suspended, power removed
>> poll is not needed
>>
>> Non ZP capable ODDs:
>> -- runtime suspended, power remained (power will never be removed)
>> poll is needed
>>
>> If we do not poll for the powered on case, we will lose media change
>> event.
>
> But the media change event should change the status from suspended to active,
> shouldn't it?
We need a way PM code can tell block layer that it is not necessary to
poll the device now, and runtime suspended is not enough, so we need
another way.
Thanks,
Aaron
>
> Rafael
>
>
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Rafael J. Wysocki @ 2012-11-26 1:22 UTC (permalink / raw)
To: Aaron Lu
Cc: Tejun Heo, Alan Stern, Jeff Garzik, James Bottomley, Jeff Wu,
linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <50B2C150.9070809@intel.com>
On Monday, November 26, 2012 09:09:36 AM Aaron Lu wrote:
> On 11/26/2012 09:11 AM, Rafael J. Wysocki wrote:
> > On Monday, November 26, 2012 09:05:58 AM Aaron Lu wrote:
> >> On 11/26/2012 09:03 AM, Rafael J. Wysocki wrote:
> >>> On Monday, November 26, 2012 08:48:51 AM Aaron Lu wrote:
> >>>> On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote:
> >>>>> On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote:
> >>>>>> On 11/20/2012 02:00 PM, Aaron Lu wrote:
> >>>>>>> On 11/19/2012 10:56 PM, Tejun Heo wrote:
> >>>>>>>> I really think we need a way for (auto)pm and event polling to talk to
> >>>>>>>> each other so that autopm can tell event poll to sod off while pm is
> >>>>>>>> in effect. Trying to solve this from inside libata doesn't seem
> >>>>>>>> right. The problem, again, seems to be figuring out which hardware
> >>>>>>>> device maps to which block device. Hmmm... Any good ideas?
> >>>>>>>
> >>>>>>> A possible way of doing this is using pm qos.
> >>>>>>>
> >>>>>>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we
> >>>>>>> can add another one: NO_POLL, use it like the following:
> >>>>>>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
> >>>>>>> longer necessary. In the ZPODD's case, it should be set when the
> >>>>>>> device is to be powered off;
> >>>>>>> 2 Clear it when poll is necessary again. In the ZPODD's case, when power
> >>>>>>> is re-gained, this flag will be cleared.
> >>>>>>
> >>>>>>
> >>>>>>> 3 In the disk_events_workfn, check if this flag is set, if so, simply
> >>>>>>> return.
> >>>>>>
> >>>>>> It should be, skip calling disk->fops->check_events, but still queue the
> >>>>>> work for next time's poll.
> >>>>>>
> >>>>>> -Aaron
> >>>>>>
> >>>>>>>
> >>>>>>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer
> >>>>>>> can access it through ata_device->sdev->sdev_gendev.
> >>>>>>>
> >>>>>>> Is this OK?
> >>>>>
> >>>>> No, I don't think so. PM QoS is about telling the layer that will put the
> >>>>> device into low-power states what states are to be taken into consideration.
> >>>>> In this case, however, we need to tell someone else that the device has been
> >>>>> turned off. Clearly, we need a way to do that, but not through PM QoS.
> >>>>>
> >>>>> Did you consider using pm_runtime_suspended() to check the device status?
> >>>>
> >>>> The problem is, a device can be in runtime suspended state while still
> >>>> needs to be polled...
> >>>
> >>> Well, maybe this is the problem, then? Why does it need to be polled when
> >>> suspended?
> >>
> >> For ODDs, poll is not necessary only when ZP capable ODD is powered off.
> >> For other ODDs, poll still needs to go on.
> >>
> >> ZP capable ODDs:
> >> - runtime suspended, power remained(due to NO_POWER_OFF qos flag)
> >> poll is needed
> >> -- runtime suspended, power removed
> >> poll is not needed
> >>
> >> Non ZP capable ODDs:
> >> -- runtime suspended, power remained (power will never be removed)
> >> poll is needed
> >>
> >> If we do not poll for the powered on case, we will lose media change
> >> event.
> >
> > But the media change event should change the status from suspended to active,
> > shouldn't it?
>
> The media change event is derived from the poll, if we stop the poll, how
> can we know the event in the first place?
OK, so what you're trying to say is that if the device is not turned off
and the user opens the tray and inserts a media in there, we won't know that
that happened without polling. Is that correct?
If so, can you please remind me why we want to pretend that the device is
suspended if we want to poll it?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
From: Aaron Lu @ 2012-11-26 1:22 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tejun Heo, Alan Stern, Jeff Garzik, James Bottomley, Jeff Wu,
linux-ide, linux-pm, linux-scsi, linux-acpi
In-Reply-To: <1592766.Y3yt2Qv8zR@vostro.rjw.lan>
On 11/26/2012 09:22 AM, Rafael J. Wysocki wrote:
> On Monday, November 26, 2012 09:09:36 AM Aaron Lu wrote:
>> On 11/26/2012 09:11 AM, Rafael J. Wysocki wrote:
>>> On Monday, November 26, 2012 09:05:58 AM Aaron Lu wrote:
>>>> On 11/26/2012 09:03 AM, Rafael J. Wysocki wrote:
>>>>> On Monday, November 26, 2012 08:48:51 AM Aaron Lu wrote:
>>>>>> On 11/26/2012 08:50 AM, Rafael J. Wysocki wrote:
>>>>>>> On Tuesday, November 20, 2012 04:59:57 PM Aaron Lu wrote:
>>>>>>>> On 11/20/2012 02:00 PM, Aaron Lu wrote:
>>>>>>>>> On 11/19/2012 10:56 PM, Tejun Heo wrote:
>>>>>>>>>> I really think we need a way for (auto)pm and event polling to talk to
>>>>>>>>>> each other so that autopm can tell event poll to sod off while pm is
>>>>>>>>>> in effect. Trying to solve this from inside libata doesn't seem
>>>>>>>>>> right. The problem, again, seems to be figuring out which hardware
>>>>>>>>>> device maps to which block device. Hmmm... Any good ideas?
>>>>>>>>>
>>>>>>>>> A possible way of doing this is using pm qos.
>>>>>>>>>
>>>>>>>>> We currently have 2 pm qos flags, NO_POWER_OFF and REMOTE_WAKEUP, and we
>>>>>>>>> can add another one: NO_POLL, use it like the following:
>>>>>>>>> 1 Set the NO_POLL pm qos flag when the underlying driver thinks it is no
>>>>>>>>> longer necessary. In the ZPODD's case, it should be set when the
>>>>>>>>> device is to be powered off;
>>>>>>>>> 2 Clear it when poll is necessary again. In the ZPODD's case, when power
>>>>>>>>> is re-gained, this flag will be cleared.
>>>>>>>>
>>>>>>>>
>>>>>>>>> 3 In the disk_events_workfn, check if this flag is set, if so, simply
>>>>>>>>> return.
>>>>>>>>
>>>>>>>> It should be, skip calling disk->fops->check_events, but still queue the
>>>>>>>> work for next time's poll.
>>>>>>>>
>>>>>>>> -Aaron
>>>>>>>>
>>>>>>>>>
>>>>>>>>> The disk->driverfs_dev can be used to host the pm qos flag, ATA layer
>>>>>>>>> can access it through ata_device->sdev->sdev_gendev.
>>>>>>>>>
>>>>>>>>> Is this OK?
>>>>>>>
>>>>>>> No, I don't think so. PM QoS is about telling the layer that will put the
>>>>>>> device into low-power states what states are to be taken into consideration.
>>>>>>> In this case, however, we need to tell someone else that the device has been
>>>>>>> turned off. Clearly, we need a way to do that, but not through PM QoS.
>>>>>>>
>>>>>>> Did you consider using pm_runtime_suspended() to check the device status?
>>>>>>
>>>>>> The problem is, a device can be in runtime suspended state while still
>>>>>> needs to be polled...
>>>>>
>>>>> Well, maybe this is the problem, then? Why does it need to be polled when
>>>>> suspended?
>>>>
>>>> For ODDs, poll is not necessary only when ZP capable ODD is powered off.
>>>> For other ODDs, poll still needs to go on.
>>>>
>>>> ZP capable ODDs:
>>>> - runtime suspended, power remained(due to NO_POWER_OFF qos flag)
>>>> poll is needed
>>>> -- runtime suspended, power removed
>>>> poll is not needed
>>>>
>>>> Non ZP capable ODDs:
>>>> -- runtime suspended, power remained (power will never be removed)
>>>> poll is needed
>>>>
>>>> If we do not poll for the powered on case, we will lose media change
>>>> event.
>>>
>>> But the media change event should change the status from suspended to active,
>>> shouldn't it?
>>
>> The media change event is derived from the poll, if we stop the poll, how
>> can we know the event in the first place?
>
> OK, so what you're trying to say is that if the device is not turned off
> and the user opens the tray and inserts a media in there, we won't know that
> that happened without polling. Is that correct?
Yes.
>
> If so, can you please remind me why we want to pretend that the device is
> suspended if we want to poll it?
We do not pretend the device is suspended, it is :-)
Even though we want to poll it, we are not polling it all the time, we
still have the poll interval, where the device and its ancestor devices
can enter runtime suspended state.
The timing to idle the device is decided by SCSI sr driver, and why it
is done this way is discussed here:
http://thread.gmane.org/gmane.linux.acpi.devel/55243/focus=52703
Thanks,
Aaron
>
> Rafael
>
>
^ permalink raw reply
* Re: [PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states
From: Huang Ying @ 2012-11-26 1:25 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Greg Kroah-Hartman, Linux PM list, ACPI Devel Maling List,
Zhang Rui, Svahn, Kai, Mika Westerberg, Lan, Tianyu, Zheng, Lv,
Aaron Lu, Grant Likely
In-Reply-To: <4855631.a4dcc0Gn36@vostro.rjw.lan>
On Mon, 2012-11-26 at 02:16 +0100, Rafael J. Wysocki wrote:
> On Monday, November 26, 2012 09:07:27 AM Huang Ying wrote:
> > On Mon, 2012-11-26 at 02:00 +0100, Rafael J. Wysocki wrote:
> > > On Monday, November 26, 2012 08:43:10 AM Huang Ying wrote:
> > > > On Sun, 2012-11-25 at 15:55 +0100, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > Make it possible to ask the routines used for adding/removing devices
> > > > > to/from the general ACPI PM domain, acpi_dev_pm_attach() and
> > > > > acpi_dev_pm_detach(), respectively, to change the power states of
> > > > > devices so that they are put into the full-power state automatically
> > > > > by acpi_dev_pm_attach() and into the lowest-power state available
> > > > > automatically by acpi_dev_pm_detach().
> > > > >
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > ---
> > > > > drivers/acpi/device_pm.c | 28 ++++++++++++++++++++++++----
> > > > > include/linux/acpi.h | 11 +++++++----
> > > > > 2 files changed, 31 insertions(+), 8 deletions(-)
> > > > >
> > > > > Index: linux/drivers/acpi/device_pm.c
> > > > > ===================================================================
> > > > > --- linux.orig/drivers/acpi/device_pm.c
> > > > > +++ linux/drivers/acpi/device_pm.c
> > > > > @@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
> > > > > /**
> > > > > * acpi_dev_pm_attach - Prepare device for ACPI power management.
> > > > > * @dev: Device to prepare.
> > > > > + * @power_on: Whether or not to power on the device.
> > > > > *
> > > > > * If @dev has a valid ACPI handle that has a valid struct acpi_device object
> > > > > * attached to it, install a wakeup notification handler for the device and
> > > > > - * add it to the general ACPI PM domain.
> > > > > + * add it to the general ACPI PM domain. If @power_on is set, the device will
> > > > > + * be put into the ACPI D0 state before the function returns.
> > > > > *
> > > > > * This assumes that the @dev's bus type uses generic power management callbacks
> > > > > * (or doesn't use any power management callbacks at all).
> > > > > @@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
> > > > > * Callers must ensure proper synchronization of this function with power
> > > > > * management callbacks.
> > > > > */
> > > > > -int acpi_dev_pm_attach(struct device *dev)
> > > > > +int acpi_dev_pm_attach(struct device *dev, bool power_on)
> > > > > {
> > > > > struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > > > >
> > > > > @@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
> > > > >
> > > > > acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
> > > > > dev->pm_domain = &acpi_general_pm_domain;
> > > > > + if (power_on) {
> > > > > + acpi_dev_pm_full_power(adev);
> > > > > + __acpi_device_run_wake(adev, false);
> > > > > + }
> > > > > return 0;
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > > > @@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > > > /**
> > > > > * acpi_dev_pm_detach - Remove ACPI power management from the device.
> > > > > * @dev: Device to take care of.
> > > > > + * @power_off: Whether or not to try to remove power from the device.
> > > > > *
> > > > > * Remove the device from the general ACPI PM domain and remove its wakeup
> > > > > - * notifier.
> > > > > + * notifier. If @power_off is set, additionally remove power from the device if
> > > > > + * possible.
> > > > > *
> > > > > * Callers must ensure proper synchronization of this function with power
> > > > > * management callbacks.
> > > > > */
> > > > > -void acpi_dev_pm_detach(struct device *dev)
> > > > > +void acpi_dev_pm_detach(struct device *dev, bool power_off)
> > > > > {
> > > > > struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > > > >
> > > > > if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> > > > > dev->pm_domain = NULL;
> > > > > acpi_remove_pm_notifier(adev, acpi_wakeup_device);
> > > > > + if (power_off) {
> > > > > + /*
> > > > > + * If the device's PM QoS resume latency limit or flags
> > > > > + * have been exposed to user space, they have to be
> > > > > + * hidden at this point, so that they don't affect the
> > > > > + * choice of the low-power state to put the device into.
> > > > > + */
> > > > > + dev_pm_qos_hide_latency_limit(dev);
> > > > > + dev_pm_qos_hide_flags(dev);
> > > >
> > > > NO_POWER_OFF flag is ignored here. Is it possible for some device (or
> > > > corresponding ACPI method) has broken D3cold implementation, so that the
> > > > user need a way to disable it?
> > >
> > > We are removing the driver here and we haven't exposed the flags, have we?
> >
> > IMHO, the flag may be exposed by the bus instead of device driver.
> > Because some bus can determine whether D3cold is supported by the
> > device. If the sysfs file for the flag is exposed by the bus, user set
> > it and we silently ignore it, will it cause confusing for the user?
>
> OK, put it in a different way: If the entity that calls acpi_dev_pm_attach()
> exposes the flags _before_ calling it, then acpi_dev_pm_detach() should not
> hide the flags. Otherwise, it should hide them. Currently, the only user
> of acpi_dev_pm_attach() (that will be the platform bus type) will not expose
> the flags before calling that routine, so it is OK for acpi_dev_pm_detach()
> to remove them.
Yes. You are right. Thanks for clarification.
Best Regards,
Huang Ying
^ permalink raw reply
* Re: [PATCH 2/2] thermal: rcar: add .get_trip_type/temp and .notify support
From: Kuninori Morimoto @ 2012-11-26 1:40 UTC (permalink / raw)
To: Zhang Rui; +Cc: Simon, Magnus, linux-pm
In-Reply-To: <1353652189.2111.7.camel@rzhang1-mobl4>
Hi Zhang
Thank you for your reply
> > +static int rcar_thermal_get_trip_temp(struct thermal_zone_device *zone,
> > + int trip, unsigned long *temp)
> > +{
> > + struct rcar_thermal_priv *priv = rcar_zone_to_priv(zone);
> > +
> > + /* see rcar_thermal_get_temp() */
> > + switch (trip) {
> > + case 0: /* -45 <= temp < +45 */
> > + *temp = -45 - 1;
> > + break;
>
> does this mean you expect the thermal driver to take some action when
> the temperature is higher than -45C?
>
> > + case 1: /* +45 <= temp < +90 */
> > + *temp = 45 - 1;
> > + break;
> what do you expect to happen when the temperature is higher than 45C but
> lower than 90C?
>
> > + case 2: /* +90 <= temp < +135 */
> > + *temp = 90 - 1;
> > + break;
> > + default:
> > + dev_err(priv->dev, "rcar driver trip error\n");
> > + return -EINVAL;
> > + }
(snip)
> > static struct thermal_zone_device_ops rcar_thermal_zone_ops = {
> > - .get_temp = rcar_thermal_get_temp,
> > + .get_temp = rcar_thermal_get_temp,
> > + .get_trip_type = rcar_thermal_get_trip_type,
> > + .get_trip_temp = rcar_thermal_get_trip_temp,
> > + .notify = rcar_thermal_notify,
> > };
> >
> how does the active trip point work if you do not bind any cooling
> devices to it?
>
> > /*
> > @@ -212,8 +286,9 @@ static int rcar_thermal_probe(struct platform_device *pdev)
> > goto error_free_priv;
> > }
> >
> > - zone = thermal_zone_device_register("rcar_thermal", 0, 0, priv,
> > - &rcar_thermal_zone_ops, 0, 0);
> > + zone = thermal_zone_device_register("rcar_thermal", 4, 0, priv,
> > + &rcar_thermal_zone_ops, 0,
> > + IDLE_INTERVAL);
>
> there is no interrupt for any thermal changes on this platform,
> including critical overheat, right?
Now, we need critical shutdown only at this point.
Then, should I remove active/hot trip ?
If yes, I can send v2 patch.
BTW, does thermal frame work use 45C ? or 45*1000 mC ?
Maybe A * 1000 mC is correct ?
Best regards
---
Kuninori Morimoto
^ permalink raw reply
* Re: [PATCH 2/2] thermal: rcar: add .get_trip_type/temp and .notify support
From: Zhang Rui @ 2012-11-26 1:52 UTC (permalink / raw)
To: Kuninori Morimoto; +Cc: Simon, Magnus, linux-pm
In-Reply-To: <87k3t9w39m.wl%kuninori.morimoto.gx@renesas.com>
On Sun, 2012-11-25 at 17:40 -0800, Kuninori Morimoto wrote:
> Hi Zhang
>
> Thank you for your reply
>
> > > +static int rcar_thermal_get_trip_temp(struct thermal_zone_device *zone,
> > > + int trip, unsigned long *temp)
> > > +{
> > > + struct rcar_thermal_priv *priv = rcar_zone_to_priv(zone);
> > > +
> > > + /* see rcar_thermal_get_temp() */
> > > + switch (trip) {
> > > + case 0: /* -45 <= temp < +45 */
> > > + *temp = -45 - 1;
> > > + break;
> >
> > does this mean you expect the thermal driver to take some action when
> > the temperature is higher than -45C?
> >
> > > + case 1: /* +45 <= temp < +90 */
> > > + *temp = 45 - 1;
> > > + break;
> > what do you expect to happen when the temperature is higher than 45C but
> > lower than 90C?
> >
> > > + case 2: /* +90 <= temp < +135 */
> > > + *temp = 90 - 1;
> > > + break;
> > > + default:
> > > + dev_err(priv->dev, "rcar driver trip error\n");
> > > + return -EINVAL;
> > > + }
> (snip)
> > > static struct thermal_zone_device_ops rcar_thermal_zone_ops = {
> > > - .get_temp = rcar_thermal_get_temp,
> > > + .get_temp = rcar_thermal_get_temp,
> > > + .get_trip_type = rcar_thermal_get_trip_type,
> > > + .get_trip_temp = rcar_thermal_get_trip_temp,
> > > + .notify = rcar_thermal_notify,
> > > };
> > >
> > how does the active trip point work if you do not bind any cooling
> > devices to it?
> >
> > > /*
> > > @@ -212,8 +286,9 @@ static int rcar_thermal_probe(struct platform_device *pdev)
> > > goto error_free_priv;
> > > }
> > >
> > > - zone = thermal_zone_device_register("rcar_thermal", 0, 0, priv,
> > > - &rcar_thermal_zone_ops, 0, 0);
> > > + zone = thermal_zone_device_register("rcar_thermal", 4, 0, priv,
> > > + &rcar_thermal_zone_ops, 0,
> > > + IDLE_INTERVAL);
> >
> > there is no interrupt for any thermal changes on this platform,
> > including critical overheat, right?
>
> Now, we need critical shutdown only at this point.
> Then, should I remove active/hot trip ?
> If yes, I can send v2 patch.
>
yep, please remove active and hot trip points.
> BTW, does thermal frame work use 45C ? or 45*1000 mC ?
> Maybe A * 1000 mC is correct ?
>
I'm not sure if I understand correctly.
The unit of trip point temperature is Milli-Celsius.
thanks,
rui
^ permalink raw reply
* Re: [PATCH 0/3 v2] thermal: rcar: add critical shutdown support
From: Kuninori Morimoto @ 2012-11-26 2:30 UTC (permalink / raw)
To: Zhang Rui; +Cc: Simon, Magnus, linux-pm
In-Reply-To: <1353894734.2299.16.camel@rzhang1-mobl4>
Hi Zhang
These are v2 patches for rcar thermal driver to add critical shutdown support.
Kuninori Morimoto (3):
thermal: rcar: fixup the unit of temperature
thermal: rcar: add rcar_zone_to_priv() macro
thermal: rcar: add .get_trip_type/temp and .notify support
drivers/thermal/rcar_thermal.c | 75 +++++++++++++++++++++++++++++++++++++---
1 file changed, 70 insertions(+), 5 deletions(-)
These are based on latest rzhang/next branch
Best regards
---
Kuninori Morimoto
^ permalink raw reply
* [PATCH 1/3] thermal: rcar: fixup the unit of temperature
From: Kuninori Morimoto, Kuninori Morimoto @ 2012-11-26 2:32 UTC (permalink / raw)
To: Zhang Rui; +Cc: Simon, Magnus, linux-pm, Kuninori Morimoto
In-Reply-To: <87ip8tw0x1.wl%kuninori.morimoto.gx@renesas.com>
The unit of temperature is Milli-Celsius.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
drivers/thermal/rcar_thermal.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index 81dce23..f2678ff 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -43,6 +43,8 @@ struct rcar_thermal_priv {
u32 comp;
};
+#define MCELSIUS(temp) ((temp) * 1000)
+
/*
* basic functions
*/
@@ -169,7 +171,7 @@ static int rcar_thermal_get_temp(struct thermal_zone_device *zone,
}
}
- *temp = tmp;
+ *temp = MCELSIUS(tmp);
return 0;
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/3] thermal: rcar: add rcar_zone_to_priv() macro
From: Kuninori Morimoto, Kuninori Morimoto @ 2012-11-26 2:32 UTC (permalink / raw)
To: Zhang Rui; +Cc: Simon, Magnus, linux-pm, Kuninori Morimoto
In-Reply-To: <87ip8tw0x1.wl%kuninori.morimoto.gx@renesas.com>
This patch adds rcar_zone_to_priv()
which is a helper macro for gettign private data.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
drivers/thermal/rcar_thermal.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index f2678ff..90db951 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -44,6 +44,7 @@ struct rcar_thermal_priv {
};
#define MCELSIUS(temp) ((temp) * 1000)
+#define rcar_zone_to_priv(zone) (zone->devdata)
/*
* basic functions
@@ -98,7 +99,7 @@ static void rcar_thermal_bset(struct rcar_thermal_priv *priv, u32 reg,
static int rcar_thermal_get_temp(struct thermal_zone_device *zone,
unsigned long *temp)
{
- struct rcar_thermal_priv *priv = zone->devdata;
+ struct rcar_thermal_priv *priv = rcar_zone_to_priv(zone);
int val, min, max, tmp;
tmp = -200; /* default */
--
1.7.9.5
^ permalink raw reply related
* [PATCH 3/3] thermal: rcar: add .get_trip_type/temp and .notify support
From: Kuninori Morimoto, Kuninori Morimoto @ 2012-11-26 2:32 UTC (permalink / raw)
To: Zhang Rui; +Cc: Simon, Magnus, linux-pm, Kuninori Morimoto
In-Reply-To: <87ip8tw0x1.wl%kuninori.morimoto.gx@renesas.com>
This patch adds .get_trip_type(), .get_trip_temp(), and .notify()
on rcar_thermal_zone_ops.
Driver will try platform power OFF if it reached to
critical temperature.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
drivers/thermal/rcar_thermal.c | 68 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 65 insertions(+), 3 deletions(-)
diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index 90db951..e1aedcc 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -22,10 +22,13 @@
#include <linux/io.h>
#include <linux/module.h>
#include <linux/platform_device.h>
+#include <linux/reboot.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/thermal.h>
+#define IDLE_INTERVAL 5000
+
#define THSCR 0x2c
#define THSSR 0x30
@@ -176,8 +179,66 @@ static int rcar_thermal_get_temp(struct thermal_zone_device *zone,
return 0;
}
+static int rcar_thermal_get_trip_type(struct thermal_zone_device *zone,
+ int trip, enum thermal_trip_type *type)
+{
+ struct rcar_thermal_priv *priv = rcar_zone_to_priv(zone);
+
+ /* see rcar_thermal_get_temp() */
+ switch (trip) {
+ case 0: /* +90 <= temp < +135 */
+ *type = THERMAL_TRIP_CRITICAL;
+ break;
+ default:
+ dev_err(priv->dev, "rcar driver trip error\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int rcar_thermal_get_trip_temp(struct thermal_zone_device *zone,
+ int trip, unsigned long *temp)
+{
+ struct rcar_thermal_priv *priv = rcar_zone_to_priv(zone);
+
+ /* see rcar_thermal_get_temp() */
+ switch (trip) {
+ case 0: /* +90 <= temp < +135 */
+ *temp = MCELSIUS(90 - 1);
+ break;
+ default:
+ dev_err(priv->dev, "rcar driver trip error\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int rcar_thermal_notify(struct thermal_zone_device *zone,
+ int trip, enum thermal_trip_type type)
+{
+ struct rcar_thermal_priv *priv = rcar_zone_to_priv(zone);
+
+ switch (type) {
+ case THERMAL_TRIP_CRITICAL:
+ /* FIXME */
+ dev_warn(priv->dev,
+ "Thermal reached to critical temperature\n");
+ machine_power_off();
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
static struct thermal_zone_device_ops rcar_thermal_zone_ops = {
- .get_temp = rcar_thermal_get_temp,
+ .get_temp = rcar_thermal_get_temp,
+ .get_trip_type = rcar_thermal_get_trip_type,
+ .get_trip_temp = rcar_thermal_get_trip_temp,
+ .notify = rcar_thermal_notify,
};
/*
@@ -211,8 +272,9 @@ static int rcar_thermal_probe(struct platform_device *pdev)
return -ENOMEM;
}
- zone = thermal_zone_device_register("rcar_thermal", 0, 0, priv,
- &rcar_thermal_zone_ops, NULL, 0, 0);
+ zone = thermal_zone_device_register("rcar_thermal", 1, 0, priv,
+ &rcar_thermal_zone_ops, NULL, 0,
+ IDLE_INTERVAL);
if (IS_ERR(zone)) {
dev_err(&pdev->dev, "thermal zone device is NULL\n");
return PTR_ERR(zone);
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 1/3] thermal: rcar: fixup the unit of temperature
From: Zhang Rui @ 2012-11-26 2:59 UTC (permalink / raw)
To: Kuninori Morimoto; +Cc: Simon, Magnus, linux-pm, Kuninori Morimoto
In-Reply-To: <87haodw0v0.wl%kuninori.morimoto.gx@renesas.com>
On Sun, 2012-11-25 at 18:32 -0800, Kuninori Morimoto wrote:
> The unit of temperature is Milli-Celsius.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
applied.
thanks,
rui
> ---
> drivers/thermal/rcar_thermal.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index 81dce23..f2678ff 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -43,6 +43,8 @@ struct rcar_thermal_priv {
> u32 comp;
> };
>
> +#define MCELSIUS(temp) ((temp) * 1000)
> +
> /*
> * basic functions
> */
> @@ -169,7 +171,7 @@ static int rcar_thermal_get_temp(struct thermal_zone_device *zone,
> }
> }
>
> - *temp = tmp;
> + *temp = MCELSIUS(tmp);
> return 0;
> }
>
^ permalink raw reply
* Re: [PATCH 2/3] thermal: rcar: add rcar_zone_to_priv() macro
From: Zhang Rui @ 2012-11-26 2:59 UTC (permalink / raw)
To: Kuninori Morimoto; +Cc: Simon, Magnus, linux-pm, Kuninori Morimoto
In-Reply-To: <87fw3xw0um.wl%kuninori.morimoto.gx@renesas.com>
On Sun, 2012-11-25 at 18:32 -0800, Kuninori Morimoto wrote:
> This patch adds rcar_zone_to_priv()
> which is a helper macro for gettign private data.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
applied.
thanks,
rui
> ---
> drivers/thermal/rcar_thermal.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index f2678ff..90db951 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -44,6 +44,7 @@ struct rcar_thermal_priv {
> };
>
> #define MCELSIUS(temp) ((temp) * 1000)
> +#define rcar_zone_to_priv(zone) (zone->devdata)
>
> /*
> * basic functions
> @@ -98,7 +99,7 @@ static void rcar_thermal_bset(struct rcar_thermal_priv *priv, u32 reg,
> static int rcar_thermal_get_temp(struct thermal_zone_device *zone,
> unsigned long *temp)
> {
> - struct rcar_thermal_priv *priv = zone->devdata;
> + struct rcar_thermal_priv *priv = rcar_zone_to_priv(zone);
> int val, min, max, tmp;
>
> tmp = -200; /* default */
^ 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