* Re: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again
From: Steven Rostedt @ 2012-04-13 15:29 UTC (permalink / raw)
To: Alan Stern
Cc: Andrey Rahmatullin, jrnieder-Re5JQEeQqe8AvxtiuMwx3w,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <Pine.LNX.4.44L0.1204131008010.1185-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 921 bytes --]
On Fri, 2012-04-13 at 10:10 -0400, Alan Stern wrote:
> On Thu, 12 Apr 2012, Alan Stern wrote:
>
> > Tomorrow I'll look through the code more carefully; with those changes
> > there really is very little difference between the two cases. Things
> > like mmio mappings and IRQs requested; they shouldn't cause a crash.
>
> Let's start with some basic information. First, before suspending,
> what shows up in /sys/kernel/debug/usb/devices?
Attached (after gzipping it).
>
> Second, if you do
>
> echo 0 >/sys/bus/usb/devices/usb1/bConfigurationValue
This never returned. I did the echo, and the command locked up.
(process 3077)
cat /proc/3077/wchan
usb_disconnect
-- Steve
> echo 0 >/sys/bus/usb/devices/usb2/bConfigurationValue
>
> before suspending (using a vanilla kernel and no script), does it
> change the behavior? I expect it won't, but we should check just to be
> thorough.
>
> Alan Stern
[-- Attachment #2: devices.gz --]
[-- Type: application/x-gzip, Size: 841 bytes --]
^ permalink raw reply
* Re: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again
From: Steven Rostedt @ 2012-04-13 15:32 UTC (permalink / raw)
To: Alan Stern
Cc: Andrey Rahmatullin, jrnieder-Re5JQEeQqe8AvxtiuMwx3w,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <1334330949.23924.360.camel-f9ZlEuEWxVcI6MkJdU+c8EEOCMrvLtNR@public.gmane.org>
On Fri, 2012-04-13 at 11:29 -0400, Steven Rostedt wrote:
> >
> > Second, if you do
> >
> > echo 0 >/sys/bus/usb/devices/usb1/bConfigurationValue
>
> This never returned. I did the echo, and the command locked up.
>
> (process 3077)
>
> cat /proc/3077/wchan
> usb_disconnect
>
A sysrq-t gives this:
[ 4468.680789] bash D ffff88022837d7d0 0 3077 3071 0x00000004
[ 4468.680792] ffff88022c0f7af8 0000000000000086 0000000000000001 ffff88022837d4c0
[ 4468.680796] ffff88022c0f7fd8 000000000000000a ffff88022c0f7fd8 ffff88022c0f7fd8
[ 4468.680800] ffff88022837f810 ffff88022837d4c0 ffff88022837d4c0 ffff88022c0f7fd8
[ 4468.680804] Call Trace:
[ 4468.680807] [<ffffffff813f5a3f>] schedule+0x3f/0x60
[ 4468.680810] [<ffffffff813f6974>] __mutex_lock_slowpath+0x104/0x1a0
[ 4468.680813] [<ffffffff813f64a2>] mutex_lock+0x22/0x40
[ 4468.680821] [<ffffffffa00111a6>] usb_disconnect+0x96/0x140 [usbcore]
[ 4468.680830] [<ffffffffa001118c>] usb_disconnect+0x7c/0x140 [usbcore]
[ 4468.680837] [<ffffffffa00112b0>] hub_quiesce+0x60/0xc0 [usbcore]
[ 4468.680844] [<ffffffffa001147c>] hub_disconnect+0x7c/0x100 [usbcore]
[ 4468.680853] [<ffffffffa001b092>] usb_unbind_interface+0x52/0x180 [usbcore]
[ 4468.680861] [<ffffffff812b650c>] __device_release_driver+0x7c/0xe0
[ 4468.680864] [<ffffffff812b659c>] device_release_driver+0x2c/0x40
[ 4468.680867] [<ffffffff812b6048>] bus_remove_device+0x78/0xb0
[ 4468.680870] [<ffffffff812b338d>] device_del+0x12d/0x1b0
[ 4468.680878] [<ffffffffa0018def>] usb_disable_device+0xaf/0x1d0 [usbcore]
[ 4468.680886] [<ffffffffa0019778>] usb_set_configuration+0x278/0x6d0 [usbcore]
[ 4468.680890] [<ffffffff8120f448>] ? sscanf+0x38/0x40
[ 4468.680898] [<ffffffffa001e5ca>] set_bConfigurationValue+0x6a/0x90 [usbcore]
[ 4468.680901] [<ffffffff812b24b8>] dev_attr_store+0x18/0x30
[ 4468.680905] [<ffffffff811a81d2>] sysfs_write_file+0xe2/0x170
[ 4468.680909] [<ffffffff81139673>] vfs_write+0xb3/0x180
[ 4468.680913] [<ffffffff8113999a>] sys_write+0x4a/0x90
[ 4468.680916] [<ffffffff813fe7ab>] system_call_fastpath+0x16/0x1b
-- Steve
--
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: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again
From: Steven Rostedt @ 2012-04-13 15:35 UTC (permalink / raw)
To: Alan Stern
Cc: Andrey Rahmatullin, jrnieder-Re5JQEeQqe8AvxtiuMwx3w,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <1334331148.23924.361.camel-f9ZlEuEWxVcI6MkJdU+c8EEOCMrvLtNR@public.gmane.org>
On Fri, 2012-04-13 at 11:32 -0400, Steven Rostedt wrote:
> On Fri, 2012-04-13 at 11:29 -0400, Steven Rostedt wrote:
>
> > >
> > > Second, if you do
> > >
> > > echo 0 >/sys/bus/usb/devices/usb1/bConfigurationValue
> >
> > This never returned. I did the echo, and the command locked up.
> >
> > (process 3077)
> >
> > cat /proc/3077/wchan
> > usb_disconnect
> >
Oh, and the "non modified" kernel I used was 3.1.4. The modified ones
was 3.2.5 (instead of removing the changes, I just used this one).
-- Steve
--
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: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again
From: Alan Stern @ 2012-04-13 15:42 UTC (permalink / raw)
To: Steven Rostedt
Cc: Andrey Rahmatullin, jrnieder-Re5JQEeQqe8AvxtiuMwx3w,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <1334330949.23924.360.camel-f9ZlEuEWxVcI6MkJdU+c8EEOCMrvLtNR@public.gmane.org>
On Fri, 13 Apr 2012, Steven Rostedt wrote:
> On Fri, 2012-04-13 at 10:10 -0400, Alan Stern wrote:
> > On Thu, 12 Apr 2012, Alan Stern wrote:
> >
> > > Tomorrow I'll look through the code more carefully; with those changes
> > > there really is very little difference between the two cases. Things
> > > like mmio mappings and IRQs requested; they shouldn't cause a crash.
> >
> > Let's start with some basic information. First, before suspending,
> > what shows up in /sys/kernel/debug/usb/devices?
>
> Attached (after gzipping it).
Okay, you've got Bluetooth device and a USB webcam, in addition to the
internal "rate-matching" hub on each EHCI bus.
> > Second, if you do
> >
> > echo 0 >/sys/bus/usb/devices/usb1/bConfigurationValue
>
> This never returned. I did the echo, and the command locked up.
>
> (process 3077)
>
> cat /proc/3077/wchan
> usb_disconnect
I just did the same thing here, with the same result. I'm trying to
track it down now. It may take a while...
Alan Stern
--
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: ehci_hcd related S3 lockup on ASUS laptops, again
From: Alan Stern @ 2012-04-13 21:04 UTC (permalink / raw)
To: Steven Rostedt; +Cc: jrnieder, Andrey Rahmatullin, linux-pm, USB list
In-Reply-To: <1334330949.23924.360.camel@gandalf.stny.rr.com>
On Fri, 13 Apr 2012, Steven Rostedt wrote:
> > Second, if you do
> >
> > echo 0 >/sys/bus/usb/devices/usb1/bConfigurationValue
>
> This never returned. I did the echo, and the command locked up.
>
> (process 3077)
>
> cat /proc/3077/wchan
> usb_disconnect
I found the reason for the deadlock. The patch below will work around
the problem for now, although it's not a correct fix. You'll still get
some complaints from lockdep.
Alan Stern
Index: usb-3.4/drivers/usb/core/hub.c
===================================================================
--- usb-3.4.orig/drivers/usb/core/hub.c
+++ usb-3.4/drivers/usb/core/hub.c
@@ -1667,7 +1667,7 @@ void usb_disconnect(struct usb_device **
{
struct usb_device *udev = *pdev;
int i;
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+// struct usb_hcd *hcd = bus_to_hcd(udev->bus);
/* mark the device as inactive, so any further urb submissions for
* this device (and any of its children) will fail immediately.
@@ -1690,9 +1690,9 @@ void usb_disconnect(struct usb_device **
* so that the hardware is now fully quiesced.
*/
dev_dbg (&udev->dev, "unregistering device\n");
- mutex_lock(hcd->bandwidth_mutex);
+// mutex_lock(hcd->bandwidth_mutex);
usb_disable_device(udev, 0);
- mutex_unlock(hcd->bandwidth_mutex);
+// mutex_unlock(hcd->bandwidth_mutex);
usb_hcd_synchronize_unlinks(udev);
usb_remove_ep_devs(&udev->ep0);
^ permalink raw reply
* Re: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again
From: Andrey Rahmatullin @ 2012-04-13 22:43 UTC (permalink / raw)
To: Alan Stern
Cc: Steven Rostedt, jrnieder-Re5JQEeQqe8AvxtiuMwx3w,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <Pine.LNX.4.44L0.1204131008010.1185-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 407 bytes --]
On Fri, Apr 13, 2012 at 10:10:30AM -0400, Alan Stern wrote:
> Second, if you do
>
> echo 0 >/sys/bus/usb/devices/usb1/bConfigurationValue
> echo 0 >/sys/bus/usb/devices/usb2/bConfigurationValue
>
> before suspending (using a vanilla kernel and no script), does it
> change the behavior? I expect it won't, but we should check just to be
> thorough.
No, it still locks up.
--
WBR, wRAR
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH V2 0/6] thermal: exynos: Add kernel thermal support for exynos platform
From: Zhang Rui @ 2012-04-16 2:07 UTC (permalink / raw)
To: Amit Kachhap
Cc: linux-samsung-soc, linaro-dev, patches, linux-kernel, lm-sensors,
linux-acpi, linux-pm
In-Reply-To: <CAK44p20Bw9QACRqJc_s4Ri8E46OKOD9O_XpC2Gtd6avSy50dJQ@mail.gmail.com>
On 三, 2012-04-11 at 18:17 +0530, Amit Kachhap wrote:
> Hi Rui,
>
> Thanks for looking into the patches.
>
> On 10 April 2012 06:28, Zhang Rui <rui.zhang@intel.com> wrote:
> > Hi, Amit,
> >
> > On 三, 2012-04-04 at 10:02 +0530, Amit Kachhap wrote:
> >> Hi Len/Rui,
> >>
> >> Any comment or feedback from your side about the status of this patch?
> >> Is it merge-able or major re-work is needed? I have fixed most of the
> >> comments in this patchset and currently working on some of the minor
> >> comments received and will submit them shortly.
> >>
> > Sorry for the late response.
> >
> > First of all, it makes sense to me to introduce the generic cpufrq
> > cooling implementation.
> ok thanks
> > But I still have some questions.
> > I think the key reason why THERMAL_TRIP_STATE_INSTANCE is introduced is
> > that the MONIROR_ZONE and WARN_ZONE on exynos4 can not fit into the
> > current passive handling in the generic thermal layer well, right?
> > e.g. there is no tc1/tc2 on exynos4.
> >
> > If yes, is it possible that we can enhance the passive cooling to
> > support the generic processor cooling?
> > say, introduce another way to throttle the processor in
> > thermal_zone_device_passive when tc1 and tc2 are not available?
>
> I agree that this new trip type code can be moved into passive trip
> type when tc1 and tc2 are 0. but this is special type of cooling
> devices behaviour where only instances of the same cooling device is
> binded to a trip point. The order of mapping is the only
> differentiating criteria and there are some checks used to implement
> this like
> 1) The trip points should be in ascending order.(This is missing in my
> original patch, so I added below)
> 2) The set_cur_state has to be called for the exact temp range so
> get_cur_state(&state) and set_cur_state(state ++/state--) logic is not
> possible.
> 3) set_cur_state is called as set_cur_state(cdev_instance).
Do you really need two THERMAL_TRIP_STATE_INSTANCE trip points?
I'm not sure if my understanding is right, but IMO, we can have one
THERMAL_TRIP_STATE_INSTANCE only, for both MONIROR_ZONE and WARN_ZONE,
and the trip temperature equals MONIROR_ZONE.
The cpufreq cooling device will work in the same way, no?
thanks,
rui
> There is a chance that people might confuse that these checks are
> applicable for passive trip types also which is not the case here.
>
> @@ -1187,6 +1228,21 @@ struct thermal_zone_device
> *thermal_zone_device_register(char *type,
> tz->ops->get_trip_type(tz, count, &trip_type);
> if (trip_type == THERMAL_TRIP_PASSIVE)
> passive = 1;
> + /*
> + * For THERMAL_TRIP_STATE_INSTANCE trips, thermal zone should
> + * be in ascending order.
> + */
> + if (trip_type == THERMAL_TRIP_STATE_INSTANCE) {
> + tz->ops->get_trip_temp(tz, count, &trip_temp);
> + if (first_trip_temp == 0)
> + first_trip_temp = trip_temp;
> + else if (first_trip_temp < trip_temp)
> + first_trip_temp = trip_temp;
> + else if (first_trip_temp > trip_temp) {
> + pr_warn("Zone trip points should be in
> ascending order\n");
> + goto unregister;
> + }
> + }
> }
>
> if (!passive)
>
> Anyway there is other alternative where this new trip type is not
> needed and I can just use the existing trip type THERMAL_TRIP_ACTIVE
> and create 2 separate cooling devices for MONITOR_ZONE and WARN_ZONE.
> I had thought to make this generic and just to manage with 1 cooling
> device.
> What is your view?
>
> Thanks,
> Amit Daniel
>
>
> >
> > thanks,
> > rui
> >
> >> Regards,
> >> Amit Daniel
> >>
> >> On 19 March 2012 11:47, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> >> > Changes since V1:
> >> > *Moved the sensor driver to driver/thermal folder from driver/hwmon folder
> >> > as suggested by Mark Brown and Guenter Roeck
> >> > *Added notifier support to notify the registered drivers of any cpu cooling
> >> > action. The driver can modify the default cooling behaviour(eg set different
> >> > max clip frequency).
> >> > *The percentage based frequency replaced with absolute clipped frequency.
> >> > *Some more conditional checks when setting max frequency.
> >> > *Renamed the new trip type THERMAL_TRIP_STATE_ACTIVE to
> >> > THERMAL_TRIP_STATE_INSTANCE
> >> > *Many review comments from R, Durgadoss <durgadoss.r@intel.com> and
> >> > eduardo.valentin@ti.com implemented.
> >> > *Removed cooling stats through debugfs patch
> >> > *The V1 based can be found here,
> >> > https://lkml.org/lkml/2012/2/22/123
> >> > http://lkml.org/lkml/2012/3/3/32
> >> >
> >> > Changes since RFC:
> >> > *Changed the cpu cooling registration/unregistration API's to instance based
> >> > *Changed the STATE_ACTIVE trip type to pass correct instance id
> >> > *Adding support to restore back the policy->max_freq after doing frequency
> >> > clipping.
> >> > *Moved the trip cooling stats from sysfs node to debugfs node as suggested
> >> > by Greg KH greg@kroah.com
> >> > *Incorporated several review comments from eduardo.valentin@ti.com
> >> > *Moved the Temperature sensor driver from driver/hwmon/ to driver/mfd
> >> > as discussed with Guenter Roeck <guenter.roeck@ericsson.com> and
> >> > Donggeun Kim <dg77.kim@samsung.com> (https://lkml.org/lkml/2012/1/5/7)
> >> > *Some changes according to the changes in common cpu cooling APIs
> >> > *The RFC based patches can be found here,
> >> > https://lkml.org/lkml/2011/12/13/186
> >> > https://lkml.org/lkml/2011/12/21/169
> >> >
> >> >
> >> > Brief Description:
> >> >
> >> > 1) The generic cooling devices code is placed inside driver/thermal/* as
> >> > placing inside acpi folder will need un-necessary enabling of acpi code. This
> >> > codes is architecture independent.
> >> >
> >> > 2) This patchset adds a new trip type THERMAL_TRIP_STATE_INSTANCE which passes
> >> > cooling device instance number and may be helpful for cpufreq cooling devices
> >> > to take the correct cooling action. This trip type avoids the temperature
> >> > comparision check again inside the cooling handler.
> >> >
> >> > 3) This patchset adds generic cpu cooling low level implementation through
> >> > frequency clipping and cpu hotplug. In future, other cpu related cooling
> >> > devices may be added here. An ACPI version of this already exists
> >> > (drivers/acpi/processor_thermal.c). But this will be useful for platforms
> >> > like ARM using the generic thermal interface along with the generic cpu
> >> > cooling devices. The cooling device registration API's return cooling device
> >> > pointers which can be easily binded with the thermal zone trip points.
> >> > The important APIs exposed are,
> >> > a)struct thermal_cooling_device *cpufreq_cooling_register(
> >> > struct freq_clip_table *tab_ptr, unsigned int tab_size,
> >> > const struct cpumask *mask_val)
> >> > b)void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
> >> >
> >> > 4) Samsung exynos platform thermal implementation is done using the generic
> >> > cpu cooling APIs and the new trip type. The temperature sensor driver present
> >> > in the hwmon folder(registered as hwmon driver) is moved to thermal folder
> >> > and registered as a thermal driver.
> >> >
> >> > All this patchset is based on Kernel version 3.3-rc7
> >> >
> >> > A simple data/control flow diagrams is shown below,
> >> >
> >> > Core Linux thermal <-----> Exynos thermal interface <----- Temperature Sensor
> >> > | |
> >> > \|/ |
> >> > Cpufreq cooling device <---------------
> >> >
> >> >
> >> > Amit Daniel Kachhap (6):
> >> > thermal: Add a new trip type to use cooling device instance number
> >> > thermal: Add generic cpufreq cooling implementation
> >> > thermal: Add generic cpuhotplug cooling implementation
> >> > hwmon: exynos4: Move thermal sensor driver to driver/thermal
> >> > directory
> >> > thermal: exynos4: Register the tmu sensor with the kernel thermal
> >> > layer
> >> > ARM: exynos4: Add thermal sensor driver platform device support
> >> >
> >> > Documentation/hwmon/exynos4_tmu | 81 ---
> >> > Documentation/thermal/cpu-cooling-api.txt | 76 +++
> >> > Documentation/thermal/exynos4_tmu | 52 ++
> >> > Documentation/thermal/sysfs-api.txt | 4 +-
> >> > arch/arm/mach-exynos/Kconfig | 11 +
> >> > arch/arm/mach-exynos/Makefile | 1 +
> >> > arch/arm/mach-exynos/clock.c | 4 +
> >> > arch/arm/mach-exynos/dev-tmu.c | 39 ++
> >> > arch/arm/mach-exynos/include/mach/irqs.h | 2 +
> >> > arch/arm/mach-exynos/include/mach/map.h | 1 +
> >> > arch/arm/mach-exynos/mach-origen.c | 1 +
> >> > arch/arm/plat-samsung/include/plat/devs.h | 1 +
> >> > drivers/hwmon/Kconfig | 10 -
> >> > drivers/hwmon/Makefile | 1 -
> >> > drivers/hwmon/exynos4_tmu.c | 514 -------------------
> >> > drivers/thermal/Kconfig | 21 +
> >> > drivers/thermal/Makefile | 2 +
> >> > drivers/thermal/cpu_cooling.c | 529 +++++++++++++++++++
> >> > drivers/thermal/exynos4_thermal.c | 790 +++++++++++++++++++++++++++++
> >> > drivers/thermal/thermal_sys.c | 45 ++-
> >> > include/linux/cpu_cooling.h | 78 +++
> >> > include/linux/platform_data/exynos4_tmu.h | 7 +
> >> > include/linux/thermal.h | 1 +
> >> > 23 files changed, 1660 insertions(+), 611 deletions(-)
> >> > delete mode 100644 Documentation/hwmon/exynos4_tmu
> >> > create mode 100644 Documentation/thermal/cpu-cooling-api.txt
> >> > create mode 100644 Documentation/thermal/exynos4_tmu
> >> > create mode 100644 arch/arm/mach-exynos/dev-tmu.c
> >> > delete mode 100644 drivers/hwmon/exynos4_tmu.c
> >> > create mode 100644 drivers/thermal/cpu_cooling.c
> >> > create mode 100644 drivers/thermal/exynos4_thermal.c
> >> > create mode 100644 include/linux/cpu_cooling.h
> >> >
> >
> >
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* RE: [PATCH V4]mmc: remove MMC bus legacy suspend/resume method
From: Hebbar, Gururaja @ 2012-04-16 5:46 UTC (permalink / raw)
To: Chuanxiao Dong, linux-mmc@vger.kernel.org,
linux-pm@lists.linux-foundation.org
Cc: cjb@laptop.org, ulf.hansson@stericsson.com
In-Reply-To: <20120411115438.GA303@intel.com>
On Wed, Apr 11, 2012 at 17:24:38, Chuanxiao Dong wrote:
> MMC bus is using legacy suspend/resume method, which is not compatible if
> runtime pm callbacks are used. In this scenario, MMC bus suspend/resume
> callbacks cannot be called when system entering S3. So change to use the new
> defined dev_pm_ops for system sleeping mode
>
> Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
> ---
> Changes in v2:
> use SET_SYSTEM_SLEEP_PM_OPS to define sleep callbacks as Rafael
> suggested
>
> Changes in v3:
> remove NULL pointer define for runtime callbacks when PM_RUNTIME is not
> selected, as Ulf Hansson suggested
>
> Changes in v4:
> fix the warning when disable CONFIG_PM_RUNTIME
Tested on AM335x Platform. Solves major issue/crash reported at
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg65425.html
Tested-by: Hebbar, Gururaja <gururaja.hebbar@ti.com>
>
> drivers/mmc/card/block.c | 2 +-
> drivers/mmc/core/bus.c | 24 ++++++++----------------
> include/linux/mmc/card.h | 2 +-
> 3 files changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index f2020d3..3582c03 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1826,7 +1826,7 @@ static void mmc_blk_remove(struct mmc_card *card)
> }
>
> #ifdef CONFIG_PM
> -static int mmc_blk_suspend(struct mmc_card *card, pm_message_t state)
> +static int mmc_blk_suspend(struct mmc_card *card)
> {
> struct mmc_blk_data *part_md;
> struct mmc_blk_data *md = mmc_get_drvdata(card);
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 5d011a3..aad8516 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -122,14 +122,14 @@ static int mmc_bus_remove(struct device *dev)
> return 0;
> }
>
> -static int mmc_bus_suspend(struct device *dev, pm_message_t state)
> +static int mmc_bus_suspend(struct device *dev)
> {
> struct mmc_driver *drv = to_mmc_driver(dev->driver);
> struct mmc_card *card = mmc_dev_to_card(dev);
> int ret = 0;
>
> if (dev->driver && drv->suspend)
> - ret = drv->suspend(card, state);
> + ret = drv->suspend(card);
> return ret;
> }
>
> @@ -165,20 +165,14 @@ static int mmc_runtime_idle(struct device *dev)
> return pm_runtime_suspend(dev);
> }
>
> +#endif /* !CONFIG_PM_RUNTIME */
> +
> static const struct dev_pm_ops mmc_bus_pm_ops = {
> - .runtime_suspend = mmc_runtime_suspend,
> - .runtime_resume = mmc_runtime_resume,
> - .runtime_idle = mmc_runtime_idle,
> + SET_RUNTIME_PM_OPS(mmc_runtime_suspend, mmc_runtime_resume,
> + mmc_runtime_idle)
> + SET_SYSTEM_SLEEP_PM_OPS(mmc_bus_suspend, mmc_bus_resume)
> };
>
> -#define MMC_PM_OPS_PTR (&mmc_bus_pm_ops)
> -
> -#else /* !CONFIG_PM_RUNTIME */
> -
> -#define MMC_PM_OPS_PTR NULL
> -
> -#endif /* !CONFIG_PM_RUNTIME */
> -
> static struct bus_type mmc_bus_type = {
> .name = "mmc",
> .dev_attrs = mmc_dev_attrs,
> @@ -186,9 +180,7 @@ static struct bus_type mmc_bus_type = {
> .uevent = mmc_bus_uevent,
> .probe = mmc_bus_probe,
> .remove = mmc_bus_remove,
> - .suspend = mmc_bus_suspend,
> - .resume = mmc_bus_resume,
> - .pm = MMC_PM_OPS_PTR,
> + .pm = &mmc_bus_pm_ops,
> };
>
> int mmc_register_bus(void)
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 1a1ca71..c984c9a 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -480,7 +480,7 @@ struct mmc_driver {
> struct device_driver drv;
> int (*probe)(struct mmc_card *);
> void (*remove)(struct mmc_card *);
> - int (*suspend)(struct mmc_card *, pm_message_t);
> + int (*suspend)(struct mmc_card *);
> int (*resume)(struct mmc_card *);
> };
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
Gururaja
^ permalink raw reply
* RE: [PATCH V4]mmc: remove MMC bus legacy suspend/resume method
From: Hebbar, Gururaja @ 2012-04-16 5:47 UTC (permalink / raw)
To: Chris Ball, Ulf Hansson
Cc: Chuanxiao Dong, linux-mmc@vger.kernel.org,
linux-pm@lists.linux-foundation.org
In-Reply-To: <87y5q2s4yo.fsf@laptop.org>
On Wed, Apr 11, 2012 at 20:14:47, Chris Ball wrote:
> Hi,
>
> On Wed, Apr 11 2012, Ulf Hansson wrote:
> > On 04/11/2012 01:54 PM, Chuanxiao Dong wrote:
> >> MMC bus is using legacy suspend/resume method, which is not compatible if
> >> runtime pm callbacks are used. In this scenario, MMC bus suspend/resume
> >> callbacks cannot be called when system entering S3. So change to use the new
> >> defined dev_pm_ops for system sleeping mode
> >>
> >> Signed-off-by: Chuanxiao Dong<chuanxiao.dong@intel.com>
> >> ---
> >> Changes in v2:
> >> use SET_SYSTEM_SLEEP_PM_OPS to define sleep callbacks as Rafael
> >> suggested
> >>
> >> Changes in v3:
> >> remove NULL pointer define for runtime callbacks when PM_RUNTIME is not
> >> selected, as Ulf Hansson suggested
> >>
> >> Changes in v4:
> >> fix the warning when disable CONFIG_PM_RUNTIME
> >>
> >
> > Looks good to me! Acked-by: Ulf Hansson <ulf.hansson@stericsson.com>
>
> Thanks! Pushed to mmc-next for 3.4.
Can you consider my Tested-by for this patch.
Tested on AM335x Platform. Solves major issue/crash reported at
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg65425.html
Tested-by: Hebbar, Gururaja <gururaja.hebbar@ti.com>
>
> - Chris.
> --
> Chris Ball <cjb@laptop.org> <http://printf.net/>
> One Laptop Per Child
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
Gururaja
^ permalink raw reply
* Re: Regarding Devfreq
From: Satendra... @ 2012-04-16 7:02 UTC (permalink / raw)
To: MyungJoo Ham; +Cc: linux-pm
In-Reply-To: <CAJ0PZbRWgS_Vcx9mXqT=mO+oCXxgJwL1JOnDb_9uyU=p3a9tbA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 9171 bytes --]
Hi Ham,
I have used jiffies to calculate the busy time and total time for a module
(made of 'n' functions say).
I have added a bew structure:
typedef struct devfreq_perf_counters {
unsigned long func_entry_jiffies; /* jiffies at the enrty of a func
*/
unsigned long start_jiffies; /* starting jiffies for every
busy/total \
load cal in the last x
seconds*/
unsigned long local_jiffies; /* last jiffies count */
unsigned long busy_jiffies; /* total execution jiffies count in the
last \
x seconds */
}devfreq_counters;
This structure's instance can be included in the platform_data(or private
data) of the
driver which has to register itself with devfreq. This way while doing
set_drv_data, access
to these counters could be available all the time.
Now there is one initialization function :
void devfreq_counters_init(void *data)
{
devfreq_counters **dc = &data->dpc;
*dc = kzalloc(sizeof (devfreq_counters), GFP_KERNEL);
(*dc)->func_entry_jiffies = 0UL;
(*dc)->local_jiffies = 0UL;
(*dc)->busy_jiffies = 0UL;
(*dc)->start_jiffies = jiffies;
return;
}
This function initializes the counters and is called at the end of driver
initialiation function.
Now there is a list of APIs as shown below:
#define devfreq_jiffies_start(dc) \
dc->func_entry_jiffies = jiffies
#define devfreq_jiffies_end(dc) do { \
dc->local_jiffies = (jiffies)-(dc->func_entry_jiffies);\
dc->busy_jiffies += dc->local_jiffies;\
}while(0)
#define busy_jiffies_in_usec(dc) \
jiffies_to_usecs((dc)->busy_jiffies)
#define busy_plus_not_busy_jiffies_in_usec(dc) \
jiffies_to_usecs(((jiffies) - (dc)->start_jiffies))
#define devfreq_reset_counters(dc) do { \
dc->func_entry_jiffies = 0UL; \
dc->local_jiffies = 0UL; \
dc->busy_jiffies = 0UL; \
dc->start_jiffies = jiffies;\
}while(0)
Description:
- devfreq_jiffies_start(dc): Will be called in the start of the function
(this is one of the functions
which we want to consider in measuring the busy time of the driver)
- devfreq_jiffies_end(dc): Will be called at the end of that function
- busy_jiffies_in_usec(dc): Will return jiffies for which all considered
(for busy time) driver's
functions were busy
- busy_plus_not_busy_jiffies_in_usec(dc): Total jiffies i.e. busy time +
not busy time of driver
- devfreq_reset_counters(dc): obvious
I wanted the simple solution to busy and load and i thought about this.
This solution hasn't been verified
yet.
Ham, can you please provide your views on it.
Thanks,
Satendra
On 29 March 2012 07:44, MyungJoo Ham <myungjoo.ham@gmail.com> wrote:
> 2012/3/28 Satendra... <satendra.pratap@gmail.com>
> >
> > Hi Ham,
> >
> > In the below struct :
> > struct devfreq_dev_status {
> > /* both since the last measure */
> > unsigned long total_time;
> > unsigned long busy_time;
> > unsigned long current_frequency;
> > void *private_data;
> > };
> >
> > How to calculate total_time/busy_time? Does it require hardware support
> > (may be some performance counters)?
> >
> > Thanks,
> > Satendra
>
> It is decided by each devfreq device driver, which is why
> get_dev_status() is to be provided by the devfreq device driver.
>
> If the device has performance counters, then, it's great, but
> performance counters are not mandatory.
>
> Here goes a list of mechanisms to fetch busy/total time:
> - Performance counters
> - Measure the time between "operation start" and "operation end" and
> accumulate the time (getting busy time. probably by ktime?)
> - Measure the idle time (CPUIDLE/CPUFREQ does this)
> - Count the number of operation and calculate the operational time
> based on the number.
> - and so on.
>
> As you can see in the list, you don't need a hardware support such as
> performance counters.
>
>
>
> Cheers!
> MyungJoo.
>
> >
> >
> > On 21 March 2012 15:49, 함명주 <myungjoo.ham@samsung.com> wrote:
> >>
> >> Hi Stendra,
> >>
> >>
> >>
> >> - Bascially, you can determine the voltage based on frequency; thus, we
> >> do not need to determint voltages at DVFS framework. It is corresponding
> >> device driver's responsibility even when we have AVS features. Thus,
> yes,
> >> the target callback needs to control both frequency and voltage (and
> >> anything else required to change the frequency/voltage)
> >>
> >>
> >>
> >> - I recommend to use regulator framework to control regulators unless
> you
> >> really really need to ignore regulator framework. Don't reinvent the
> wheel.
> >>
> >>
> >>
> >> - The three steps you've mentioned are correct.
> >>
> >>
> >>
> >> - The mailing list is opened to all. You are welcomed to use it (CC'ed
> >> linux-pm) and just CC needed people.
> >>
> >>
> >>
> >>
> >>
> >> Cheers!
> >>
> >> MyungJoo.
> >>
> >>
> >>
> >> ------- Original Message -------
> >>
> >> Sender : Satendra...<satendra.pratap@gmail.com>
> >>
> >> Date : 2012-03-21 19:03 (GMT+09:00)
> >>
> >> Title : Re: Re: Regarding Devfreq
> >>
> >>
> >>
> >> Hi Ham,
> >>
> >> Thank you very much for your reply. I will try not to disturb you much.
> >> In Devfreq I dont see any API related to voltage scaling. Is it the
> >> "->target" function's (in struct devfreq_dev_profile)
> >> responsibility to change the voltage as well with frequency?
> >> Do we really need to register our regulators using regulator framework
> of
> >> linux for voltage scaling?
> >>
> >> For a clear understanding I need to do following to use devfreq (I am
> >> sorry as I may be verifying it again):
> >> - define an instance of "struct devfreq_dev_profile" and provide
> >> implementations of target, get_dev_status and exit callbacks
> >> - Implement our choice of governer or use any one from already
> >> implemented ones.
> >> - call "devfreq_add_device" function from our driver's probe function.
> >>
> >> is that it? or do we need to do something else also?
> >>
> >> Thanks,
> >> Satendra
> >>
> >>
> >> On 21 March 2012 15:14, 함명주 <myungjoo.ham@samsung.com> wrote:
> >>>
> >>> Hello Satendra,
> >>>
> >>>
> >>>
> >>> - For Devfreq, OPP is optional though recommended for easier
> >>> implementation. You can still implement all the needed things without
> OPP.
> >>> OPP is just a simple data structure to store pairs of voltage and
> frequency.
> >>>
> >>>
> >>>
> >>> - Yes, you need to call devfreq_add_device() and supply the required
> >>> data. You can implement your own governor or use one of predefined
> >>> governors. Runtime replacement of governors like CPUfreq is "TODO" for
> now.
> >>>
> >>>
> >>>
> >>> - For example, you can see /drivers/devfreq/exynos4_bus.c. GPU, Display
> >>> devfreq drivers are under development in other companies (ARM).
> >>>
> >>>
> >>>
> >>> Cheers!
> >>>
> >>> MyungJoo.
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> ------- Original Message -------
> >>>
> >>> Sender : Satendra...<satendra.pratap@gmail.com>
> >>>
> >>> Date : 2012-03-21 17:31 (GMT+09:00)
> >>>
> >>> Title : Re: Regarding Devfreq
> >>>
> >>>
> >>>
> >>> Hi Ham,
> >>>
> >>> I have studied DVFS and what I feel is that we need to use OPP and
> >>> Voltage regulator interfaces also to
> >>> maintain Optimum Performance Points and to provide APIs to change the
> >>> voltage.
> >>> What I feel is that to use DVFS every driver has to
> >>> call devfreq_add_device function to register that device to
> >>> the devfreq framework. And in order to do that we have to
> >>> implement devfreq_dev_profile and our choice of governer.
> >>>
> >>> I appreciate your help.
> >>>
> >>> Thanks,
> >>> Satendra
> >>>
> >>> On 20 March 2012 12:16, Satendra... <satendra.pratap@gmail.com> wrote:
> >>>>
> >>>> Hi Ham,
> >>>>
> >>>> We are working on a new SoC for our new product and would want to use
> >>>> your Devfreq
> >>>> framework for our devices. Would you please let me know any other
> >>>> implementation which
> >>>> uses devfreq ? so that we could take that as a reference.
> >>>> Or we would be the first one to start?
> >>>>
> >>>> Thanks,
> >>>> Satendra
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> --
> >>>
> >>> MyungJoo Ham (함명주), PHD
> >>>
> >>> System S/W Lab, S/W Platform Team, Software Center
> >>> Samsung Electronics
> >>> Cell: +82-10-6714-2858
> >>>
> >>>
> >>
> >>
> >>
> >>
> >>
> >>
> >> --
> >>
> >> MyungJoo Ham (함명주), PHD
> >>
> >> System S/W Lab, S/W Platform Team, Software Center
> >> Samsung Electronics
> >> Cell: +82-10-6714-2858
> >>
> >>
> >
> >
> >
> > _______________________________________________
> > linux-pm mailing list
> > linux-pm@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
>
>
>
>
> --
> MyungJoo Ham, Ph.D.
> System S/W Lab, S/W Center, Samsung Electronics
>
[-- Attachment #1.2: Type: text/html, Size: 13272 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Devfreq Simple On Demand Governor
From: Satendra... @ 2012-04-16 10:07 UTC (permalink / raw)
To: linux-pm; +Cc: myungjoo.ham
[-- Attachment #1.1: Type: text/plain, Size: 767 bytes --]
Hi Ham,
I was looking at the simple on demand governor's code and have got few
queries:
- In the code below to check for overflow, why 24 and 7 has been taken?
/* Prevent overflow */
if (stat.busy_time >= (1 << 24) || stat.total_time >= (1 << 24)) {
stat.busy_time >>= 7;
stat.total_time >>= 7;
}
- Also for maximum frequency UINT_MAX is returned which may not be suited
for some devices.
- One more query, whats the unit (usecs, nsecs etc) used for busy and load
time. Has it got nothing to do with units
and its mere the percentage of busy time?
- If we want to use simple on demand governor then do we need to modify
yours to suit our needs? I mean you have
just provided a framework?
Thanks,
Satendra
[-- Attachment #1.2: Type: text/html, Size: 1032 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: Regarding Devfreq
From: MyungJoo Ham @ 2012-04-16 10:19 UTC (permalink / raw)
To: Satendra...; +Cc: linux-pm
In-Reply-To: <CAChS5kNA8CyQuHPJ+HjJ21NMVB5vddBjJvSXQF+KcLbN4v8mXg@mail.gmail.com>
On Mon, Apr 16, 2012 at 4:02 PM, Satendra... <satendra.pratap@gmail.com> wrote:
> Hi Ham,
>
> I have used jiffies to calculate the busy time and total time for a module
> (made of 'n' functions say).
> I have added a bew structure:
Hi.
In my ARM SoC systems, one jiffy is 5ms. I don't think other systems
won't vary too much (should be around several hundred us to several
ms). Are you sure that jiffy has enough granularity for your system?
If you are counting the length spent in a function call, shouldn't
nsecs be usec? (ktime?). And for saving such values, the
"private_data" is intended for individual devfreq drivers
communicating with governors specific to the drivers. Devfreq->data is
intended for individual devfreq governors. Thus, yes, platform-data is
the right place for devfreq driver internal.
Cheers!
MyungJoo.
>
> typedef struct devfreq_perf_counters {
> unsigned long func_entry_jiffies; /* jiffies at the enrty of a func
> */
> unsigned long start_jiffies; /* starting jiffies for every
> busy/total \
> load cal in the last x
> seconds*/
> unsigned long local_jiffies; /* last jiffies count */
> unsigned long busy_jiffies; /* total execution jiffies count in the
> last \
> x seconds */
> }devfreq_counters;
>
> This structure's instance can be included in the platform_data(or private
> data) of the
> driver which has to register itself with devfreq. This way while doing
> set_drv_data, access
> to these counters could be available all the time.
>
> Now there is one initialization function :
>
> void devfreq_counters_init(void *data)
> {
> devfreq_counters **dc = &data->dpc;
>
> *dc = kzalloc(sizeof (devfreq_counters), GFP_KERNEL);
>
> (*dc)->func_entry_jiffies = 0UL;
> (*dc)->local_jiffies = 0UL;
> (*dc)->busy_jiffies = 0UL;
> (*dc)->start_jiffies = jiffies;
>
> return;
> }
>
> This function initializes the counters and is called at the end of driver
> initialiation function.
>
> Now there is a list of APIs as shown below:
>
> #define devfreq_jiffies_start(dc) \
> dc->func_entry_jiffies = jiffies
>
> #define devfreq_jiffies_end(dc) do { \
> dc->local_jiffies = (jiffies)-(dc->func_entry_jiffies);\
> dc->busy_jiffies += dc->local_jiffies;\
> }while(0)
>
> #define busy_jiffies_in_usec(dc) \
> jiffies_to_usecs((dc)->busy_jiffies)
>
> #define busy_plus_not_busy_jiffies_in_usec(dc) \
> jiffies_to_usecs(((jiffies) - (dc)->start_jiffies))
>
> #define devfreq_reset_counters(dc) do { \
> dc->func_entry_jiffies = 0UL; \
> dc->local_jiffies = 0UL; \
> dc->busy_jiffies = 0UL; \
> dc->start_jiffies = jiffies;\
> }while(0)
>
> Description:
> - devfreq_jiffies_start(dc): Will be called in the start of the function
> (this is one of the functions
> which we want to consider in measuring the busy time of the driver)
> - devfreq_jiffies_end(dc): Will be called at the end of that function
> - busy_jiffies_in_usec(dc): Will return jiffies for which all considered
> (for busy time) driver's
> functions were busy
> - busy_plus_not_busy_jiffies_in_usec(dc): Total jiffies i.e. busy time + not
> busy time of driver
> - devfreq_reset_counters(dc): obvious
>
> I wanted the simple solution to busy and load and i thought about this. This
> solution hasn't been verified
> yet.
>
> Ham, can you please provide your views on it.
>
> Thanks,
> Satendra
>
>
> On 29 March 2012 07:44, MyungJoo Ham <myungjoo.ham@gmail.com> wrote:
>>
>> 2012/3/28 Satendra... <satendra.pratap@gmail.com>
>> >
>> > Hi Ham,
>> >
>> > In the below struct :
>> > struct devfreq_dev_status {
>> > /* both since the last measure */
>> > unsigned long total_time;
>> > unsigned long busy_time;
>> > unsigned long current_frequency;
>> > void *private_data;
>> > };
>> >
>> > How to calculate total_time/busy_time? Does it require hardware support
>> > (may be some performance counters)?
>> >
>> > Thanks,
>> > Satendra
>>
>> It is decided by each devfreq device driver, which is why
>> get_dev_status() is to be provided by the devfreq device driver.
>>
>> If the device has performance counters, then, it's great, but
>> performance counters are not mandatory.
>>
>> Here goes a list of mechanisms to fetch busy/total time:
>> - Performance counters
>> - Measure the time between "operation start" and "operation end" and
>> accumulate the time (getting busy time. probably by ktime?)
>> - Measure the idle time (CPUIDLE/CPUFREQ does this)
>> - Count the number of operation and calculate the operational time
>> based on the number.
>> - and so on.
>>
>> As you can see in the list, you don't need a hardware support such as
>> performance counters.
>>
>>
>>
>> Cheers!
>> MyungJoo.
>>
>> >
>> >
>> > On 21 March 2012 15:49, 함명주 <myungjoo.ham@samsung.com> wrote:
>> >>
>> >> Hi Stendra,
>> >>
>> >>
>> >>
>> >> - Bascially, you can determine the voltage based on frequency; thus, we
>> >> do not need to determint voltages at DVFS framework. It is
>> >> corresponding
>> >> device driver's responsibility even when we have AVS features. Thus,
>> >> yes,
>> >> the target callback needs to control both frequency and voltage (and
>> >> anything else required to change the frequency/voltage)
>> >>
>> >>
>> >>
>> >> - I recommend to use regulator framework to control regulators unless
>> >> you
>> >> really really need to ignore regulator framework. Don't reinvent the
>> >> wheel.
>> >>
>> >>
>> >>
>> >> - The three steps you've mentioned are correct.
>> >>
>> >>
>> >>
>> >> - The mailing list is opened to all. You are welcomed to use it (CC'ed
>> >> linux-pm) and just CC needed people.
>> >>
>> >>
>> >>
>> >>
>> >>
>> >> Cheers!
>> >>
>> >> MyungJoo.
>> >>
>> >>
>> >>
>> >> ------- Original Message -------
>> >>
>> >> Sender : Satendra...<satendra.pratap@gmail.com>
>> >>
>> >> Date : 2012-03-21 19:03 (GMT+09:00)
>> >>
>> >> Title : Re: Re: Regarding Devfreq
>> >>
>> >>
>> >>
>> >> Hi Ham,
>> >>
>> >> Thank you very much for your reply. I will try not to disturb you much.
>> >> In Devfreq I dont see any API related to voltage scaling. Is it the
>> >> "->target" function's (in struct devfreq_dev_profile)
>> >> responsibility to change the voltage as well with frequency?
>> >> Do we really need to register our regulators using regulator framework
>> >> of
>> >> linux for voltage scaling?
>> >>
>> >> For a clear understanding I need to do following to use devfreq (I am
>> >> sorry as I may be verifying it again):
>> >> - define an instance of "struct devfreq_dev_profile" and provide
>> >> implementations of target, get_dev_status and exit callbacks
>> >> - Implement our choice of governer or use any one from already
>> >> implemented ones.
>> >> - call "devfreq_add_device" function from our driver's probe function.
>> >>
>> >> is that it? or do we need to do something else also?
>> >>
>> >> Thanks,
>> >> Satendra
>> >>
>> >>
>> >> On 21 March 2012 15:14, 함명주 <myungjoo.ham@samsung.com> wrote:
>> >>>
>> >>> Hello Satendra,
>> >>>
>> >>>
>> >>>
>> >>> - For Devfreq, OPP is optional though recommended for easier
>> >>> implementation. You can still implement all the needed things without
>> >>> OPP.
>> >>> OPP is just a simple data structure to store pairs of voltage and
>> >>> frequency.
>> >>>
>> >>>
>> >>>
>> >>> - Yes, you need to call devfreq_add_device() and supply the required
>> >>> data. You can implement your own governor or use one of predefined
>> >>> governors. Runtime replacement of governors like CPUfreq is "TODO" for
>> >>> now.
>> >>>
>> >>>
>> >>>
>> >>> - For example, you can see /drivers/devfreq/exynos4_bus.c. GPU,
>> >>> Display
>> >>> devfreq drivers are under development in other companies (ARM).
>> >>>
>> >>>
>> >>>
>> >>> Cheers!
>> >>>
>> >>> MyungJoo.
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>> ------- Original Message -------
>> >>>
>> >>> Sender : Satendra...<satendra.pratap@gmail.com>
>> >>>
>> >>> Date : 2012-03-21 17:31 (GMT+09:00)
>> >>>
>> >>> Title : Re: Regarding Devfreq
>> >>>
>> >>>
>> >>>
>> >>> Hi Ham,
>> >>>
>> >>> I have studied DVFS and what I feel is that we need to use OPP and
>> >>> Voltage regulator interfaces also to
>> >>> maintain Optimum Performance Points and to provide APIs to change the
>> >>> voltage.
>> >>> What I feel is that to use DVFS every driver has to
>> >>> call devfreq_add_device function to register that device to
>> >>> the devfreq framework. And in order to do that we have to
>> >>> implement devfreq_dev_profile and our choice of governer.
>> >>>
>> >>> I appreciate your help.
>> >>>
>> >>> Thanks,
>> >>> Satendra
>> >>>
>> >>> On 20 March 2012 12:16, Satendra... <satendra.pratap@gmail.com> wrote:
>> >>>>
>> >>>> Hi Ham,
>> >>>>
>> >>>> We are working on a new SoC for our new product and would want to use
>> >>>> your Devfreq
>> >>>> framework for our devices. Would you please let me know any other
>> >>>> implementation which
>> >>>> uses devfreq ? so that we could take that as a reference.
>> >>>> Or we would be the first one to start?
>> >>>>
>> >>>> Thanks,
>> >>>> Satendra
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>> --
>> >>>
>> >>> MyungJoo Ham (함명주), PHD
>> >>>
>> >>> System S/W Lab, S/W Platform Team, Software Center
>> >>> Samsung Electronics
>> >>> Cell: +82-10-6714-2858
>> >>>
>> >>>
>> >>
>> >>
>> >>
>> >>
>> >>
>> >>
>> >> --
>> >>
>> >> MyungJoo Ham (함명주), PHD
>> >>
>> >> System S/W Lab, S/W Platform Team, Software Center
>> >> Samsung Electronics
>> >> Cell: +82-10-6714-2858
>> >>
>> >>
>> >
>> >
>> >
>> > _______________________________________________
>> > linux-pm mailing list
>> > linux-pm@lists.linux-foundation.org
>> > https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
>>
>>
>>
>>
>> --
>> MyungJoo Ham, Ph.D.
>> System S/W Lab, S/W Center, Samsung Electronics
>
>
--
MyungJoo Ham, Ph.D.
System S/W Lab, S/W Center, Samsung Electronics
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: Regarding Devfreq
From: MyungJoo Ham @ 2012-04-16 12:10 UTC (permalink / raw)
To: Satendra...; +Cc: linux-pm
In-Reply-To: <CAJ0PZbRfHz1BqMrJHi2FD+d51nx86hyr79M96pJ-ZCKoUYEFTQ@mail.gmail.com>
On Mon, Apr 16, 2012 at 7:19 PM, MyungJoo Ham <myungjoo.ham@gmail.com> wrote:
> On Mon, Apr 16, 2012 at 4:02 PM, Satendra... <satendra.pratap@gmail.com> wrote:
>> Hi Ham,
>>
>> I have used jiffies to calculate the busy time and total time for a module
>> (made of 'n' functions say).
>> I have added a bew structure:
>
> Hi.
>
> In my ARM SoC systems, one jiffy is 5ms. I don't think other systems
> won't vary too much (should be around several hundred us to several
> ms). Are you sure that jiffy has enough granularity for your system?
> If you are counting the length spent in a function call, shouldn't
> nsecs be usec? (ktime?). And for saving such values, the
> "private_data" is intended for individual devfreq drivers
> communicating with governors specific to the drivers. Devfreq->data is
> intended for individual devfreq governors. Thus, yes, platform-data is
> the right place for devfreq driver internal.
>
> Cheers!
> MyungJoo.
>
Ah.. and one missing point:
Please consider the concurrency. You may have multiple instance of
function calls in your device driver. And please do not block them by
simply adding locks there if this performance counting is the only
reason to make them mutually exclusive. This is another reason to
count nsecs (ktime) locally in the function and add the end-start
(local variable) into the aggregation variable (lock here only or use
atomic operators).
Cheers!
MyungJoo.
>>
>> typedef struct devfreq_perf_counters {
>> unsigned long func_entry_jiffies; /* jiffies at the enrty of a func
>> */
>> unsigned long start_jiffies; /* starting jiffies for every
>> busy/total \
>> load cal in the last x
>> seconds*/
>> unsigned long local_jiffies; /* last jiffies count */
>> unsigned long busy_jiffies; /* total execution jiffies count in the
>> last \
>> x seconds */
>> }devfreq_counters;
>>
>> This structure's instance can be included in the platform_data(or private
>> data) of the
>> driver which has to register itself with devfreq. This way while doing
>> set_drv_data, access
>> to these counters could be available all the time.
>>
>> Now there is one initialization function :
>>
>> void devfreq_counters_init(void *data)
>> {
>> devfreq_counters **dc = &data->dpc;
>>
>> *dc = kzalloc(sizeof (devfreq_counters), GFP_KERNEL);
>>
>> (*dc)->func_entry_jiffies = 0UL;
>> (*dc)->local_jiffies = 0UL;
>> (*dc)->busy_jiffies = 0UL;
>> (*dc)->start_jiffies = jiffies;
>>
>> return;
>> }
>>
>> This function initializes the counters and is called at the end of driver
>> initialiation function.
>>
>> Now there is a list of APIs as shown below:
>>
>> #define devfreq_jiffies_start(dc) \
>> dc->func_entry_jiffies = jiffies
>>
>> #define devfreq_jiffies_end(dc) do { \
>> dc->local_jiffies = (jiffies)-(dc->func_entry_jiffies);\
>> dc->busy_jiffies += dc->local_jiffies;\
>> }while(0)
>>
>> #define busy_jiffies_in_usec(dc) \
>> jiffies_to_usecs((dc)->busy_jiffies)
>>
>> #define busy_plus_not_busy_jiffies_in_usec(dc) \
>> jiffies_to_usecs(((jiffies) - (dc)->start_jiffies))
>>
>> #define devfreq_reset_counters(dc) do { \
>> dc->func_entry_jiffies = 0UL; \
>> dc->local_jiffies = 0UL; \
>> dc->busy_jiffies = 0UL; \
>> dc->start_jiffies = jiffies;\
>> }while(0)
>>
>> Description:
>> - devfreq_jiffies_start(dc): Will be called in the start of the function
>> (this is one of the functions
>> which we want to consider in measuring the busy time of the driver)
>> - devfreq_jiffies_end(dc): Will be called at the end of that function
>> - busy_jiffies_in_usec(dc): Will return jiffies for which all considered
>> (for busy time) driver's
>> functions were busy
>> - busy_plus_not_busy_jiffies_in_usec(dc): Total jiffies i.e. busy time + not
>> busy time of driver
>> - devfreq_reset_counters(dc): obvious
>>
>> I wanted the simple solution to busy and load and i thought about this. This
>> solution hasn't been verified
>> yet.
>>
>> Ham, can you please provide your views on it.
>>
>> Thanks,
>> Satendra
>>
>>
>> On 29 March 2012 07:44, MyungJoo Ham <myungjoo.ham@gmail.com> wrote:
>>>
>>> 2012/3/28 Satendra... <satendra.pratap@gmail.com>
>>> >
>>> > Hi Ham,
>>> >
>>> > In the below struct :
>>> > struct devfreq_dev_status {
>>> > /* both since the last measure */
>>> > unsigned long total_time;
>>> > unsigned long busy_time;
>>> > unsigned long current_frequency;
>>> > void *private_data;
>>> > };
>>> >
>>> > How to calculate total_time/busy_time? Does it require hardware support
>>> > (may be some performance counters)?
>>> >
>>> > Thanks,
>>> > Satendra
>>>
>>> It is decided by each devfreq device driver, which is why
>>> get_dev_status() is to be provided by the devfreq device driver.
>>>
>>> If the device has performance counters, then, it's great, but
>>> performance counters are not mandatory.
>>>
>>> Here goes a list of mechanisms to fetch busy/total time:
>>> - Performance counters
>>> - Measure the time between "operation start" and "operation end" and
>>> accumulate the time (getting busy time. probably by ktime?)
>>> - Measure the idle time (CPUIDLE/CPUFREQ does this)
>>> - Count the number of operation and calculate the operational time
>>> based on the number.
>>> - and so on.
>>>
>>> As you can see in the list, you don't need a hardware support such as
>>> performance counters.
>>>
>>>
>>>
>>> Cheers!
>>> MyungJoo.
>>>
>>> >
>>> >
>>> > On 21 March 2012 15:49, 함명주 <myungjoo.ham@samsung.com> wrote:
>>> >>
>>> >> Hi Stendra,
>>> >>
>>> >>
>>> >>
>>> >> - Bascially, you can determine the voltage based on frequency; thus, we
>>> >> do not need to determint voltages at DVFS framework. It is
>>> >> corresponding
>>> >> device driver's responsibility even when we have AVS features. Thus,
>>> >> yes,
>>> >> the target callback needs to control both frequency and voltage (and
>>> >> anything else required to change the frequency/voltage)
>>> >>
>>> >>
>>> >>
>>> >> - I recommend to use regulator framework to control regulators unless
>>> >> you
>>> >> really really need to ignore regulator framework. Don't reinvent the
>>> >> wheel.
>>> >>
>>> >>
>>> >>
>>> >> - The three steps you've mentioned are correct.
>>> >>
>>> >>
>>> >>
>>> >> - The mailing list is opened to all. You are welcomed to use it (CC'ed
>>> >> linux-pm) and just CC needed people.
>>> >>
>>> >>
>>> >>
>>> >>
>>> >>
>>> >> Cheers!
>>> >>
>>> >> MyungJoo.
>>> >>
>>> >>
>>> >>
>>> >> ------- Original Message -------
>>> >>
>>> >> Sender : Satendra...<satendra.pratap@gmail.com>
>>> >>
>>> >> Date : 2012-03-21 19:03 (GMT+09:00)
>>> >>
>>> >> Title : Re: Re: Regarding Devfreq
>>> >>
>>> >>
>>> >>
>>> >> Hi Ham,
>>> >>
>>> >> Thank you very much for your reply. I will try not to disturb you much.
>>> >> In Devfreq I dont see any API related to voltage scaling. Is it the
>>> >> "->target" function's (in struct devfreq_dev_profile)
>>> >> responsibility to change the voltage as well with frequency?
>>> >> Do we really need to register our regulators using regulator framework
>>> >> of
>>> >> linux for voltage scaling?
>>> >>
>>> >> For a clear understanding I need to do following to use devfreq (I am
>>> >> sorry as I may be verifying it again):
>>> >> - define an instance of "struct devfreq_dev_profile" and provide
>>> >> implementations of target, get_dev_status and exit callbacks
>>> >> - Implement our choice of governer or use any one from already
>>> >> implemented ones.
>>> >> - call "devfreq_add_device" function from our driver's probe function.
>>> >>
>>> >> is that it? or do we need to do something else also?
>>> >>
>>> >> Thanks,
>>> >> Satendra
>>> >>
>>> >>
>>> >> On 21 March 2012 15:14, 함명주 <myungjoo.ham@samsung.com> wrote:
>>> >>>
>>> >>> Hello Satendra,
>>> >>>
>>> >>>
>>> >>>
>>> >>> - For Devfreq, OPP is optional though recommended for easier
>>> >>> implementation. You can still implement all the needed things without
>>> >>> OPP.
>>> >>> OPP is just a simple data structure to store pairs of voltage and
>>> >>> frequency.
>>> >>>
>>> >>>
>>> >>>
>>> >>> - Yes, you need to call devfreq_add_device() and supply the required
>>> >>> data. You can implement your own governor or use one of predefined
>>> >>> governors. Runtime replacement of governors like CPUfreq is "TODO" for
>>> >>> now.
>>> >>>
>>> >>>
>>> >>>
>>> >>> - For example, you can see /drivers/devfreq/exynos4_bus.c. GPU,
>>> >>> Display
>>> >>> devfreq drivers are under development in other companies (ARM).
>>> >>>
>>> >>>
>>> >>>
>>> >>> Cheers!
>>> >>>
>>> >>> MyungJoo.
>>> >>>
>>> >>>
>>> >>>
>>> >>>
>>> >>>
>>> >>> ------- Original Message -------
>>> >>>
>>> >>> Sender : Satendra...<satendra.pratap@gmail.com>
>>> >>>
>>> >>> Date : 2012-03-21 17:31 (GMT+09:00)
>>> >>>
>>> >>> Title : Re: Regarding Devfreq
>>> >>>
>>> >>>
>>> >>>
>>> >>> Hi Ham,
>>> >>>
>>> >>> I have studied DVFS and what I feel is that we need to use OPP and
>>> >>> Voltage regulator interfaces also to
>>> >>> maintain Optimum Performance Points and to provide APIs to change the
>>> >>> voltage.
>>> >>> What I feel is that to use DVFS every driver has to
>>> >>> call devfreq_add_device function to register that device to
>>> >>> the devfreq framework. And in order to do that we have to
>>> >>> implement devfreq_dev_profile and our choice of governer.
>>> >>>
>>> >>> I appreciate your help.
>>> >>>
>>> >>> Thanks,
>>> >>> Satendra
>>> >>>
>>> >>> On 20 March 2012 12:16, Satendra... <satendra.pratap@gmail.com> wrote:
>>> >>>>
>>> >>>> Hi Ham,
>>> >>>>
>>> >>>> We are working on a new SoC for our new product and would want to use
>>> >>>> your Devfreq
>>> >>>> framework for our devices. Would you please let me know any other
>>> >>>> implementation which
>>> >>>> uses devfreq ? so that we could take that as a reference.
>>> >>>> Or we would be the first one to start?
>>> >>>>
>>> >>>> Thanks,
>>> >>>> Satendra
>>> >>>
>>> >>>
>>> >>>
>>> >>>
>>> >>>
>>> >>>
>>> >>> --
>>> >>>
>>> >>> MyungJoo Ham (함명주), PHD
>>> >>>
>>> >>> System S/W Lab, S/W Platform Team, Software Center
>>> >>> Samsung Electronics
>>> >>> Cell: +82-10-6714-2858
>>> >>>
>>> >>>
>>> >>
>>> >>
>>> >>
>>> >>
>>> >>
>>> >>
>>> >> --
>>> >>
>>> >> MyungJoo Ham (함명주), PHD
>>> >>
>>> >> System S/W Lab, S/W Platform Team, Software Center
>>> >> Samsung Electronics
>>> >> Cell: +82-10-6714-2858
>>> >>
>>> >>
>>> >
>>> >
>>> >
>>> > _______________________________________________
>>> > linux-pm mailing list
>>> > linux-pm@lists.linux-foundation.org
>>> > https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
>>>
>>>
>>>
>>>
>>> --
>>> MyungJoo Ham, Ph.D.
>>> System S/W Lab, S/W Center, Samsung Electronics
>>
>>
>
>
>
> --
> MyungJoo Ham, Ph.D.
> System S/W Lab, S/W Center, Samsung Electronics
--
MyungJoo Ham, Ph.D.
System S/W Lab, S/W Center, Samsung Electronics
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: Devfreq Simple On Demand Governor
From: MyungJoo Ham @ 2012-04-16 14:16 UTC (permalink / raw)
To: Satendra...; +Cc: linux-pm
In-Reply-To: <CAChS5kNS6DUq9W-R0XkQ8ksojUAbsLy5opz1eKb5iaa0D895cw@mail.gmail.com>
On Mon, Apr 16, 2012 at 7:07 PM, Satendra... <satendra.pratap@gmail.com> wrote:
> Hi Ham,
>
> I was looking at the simple on demand governor's code and have got few
> queries:
> - In the code below to check for overflow, why 24 and 7 has been taken?
> /* Prevent overflow */
> if (stat.busy_time >= (1 << 24) || stat.total_time >= (1 << 24)) {
> stat.busy_time >>= 7;
> stat.total_time >>= 7;
> }
It is because 1<<7 >= 100 and 1<<6 < 100. (To get percentage, we are
multiplying 100).
>
> - Also for maximum frequency UINT_MAX is returned which may not be suited
> for some devices.
The frequency given to devfreq driver is "recommended frequency", not
the "exact frequency required".
Thus, the driver is required to choose
1. frequency that is at least (but closest) or same with the
"recommended frequency"
and if that's impossible,
2. frequency that is closest to the "recommended frequency".
Thus, if it's "UINT_MAX", then it should choose the maximum frequency.
>
> - One more query, whats the unit (usecs, nsecs etc) used for busy and load
> time. Has it got nothing to do with units
> and its mere the percentage of busy time?
The unit is up to the device driver. It only needs to provide
"reasonable" values in u32 to the infrastructure. It could be usec,
nsec, #ops, or anything else.
I've mentioned it only because jiffy does not seem to be "reasonable";
jiffy is too rough. You may even get 0 jiffies for 100 operations
while you may get 1 jiffy for 1 operation in the same system.
In the governors, maybe busy / total ratio can be used as in
simple_ondemand. And you should give values that can support some fair
granularity there, which jiffy cannot.
>
> - If we want to use simple on demand governor then do we need to modify
> yours to suit our needs? I mean you have
> just provided a framework?
If you want to use THE simple_ondemand for your device, you only need
to create a devfreq driver. You don't need to modify the governor or
the framework/.
>
> Thanks,
> Satendra
Cheers!
MyungJoo.
--
MyungJoo Ham, Ph.D.
System S/W Lab, S/W Center, Samsung Electronics
^ permalink raw reply
* Re: ehci_hcd related S3 lockup on ASUS laptops, again
From: Alan Stern @ 2012-04-16 20:07 UTC (permalink / raw)
To: Andrey Rahmatullin; +Cc: jrnieder, linux-pm, USB list, Steven Rostedt
In-Reply-To: <20120413224336.GA5585@belkar.wrar.name>
On Sat, 14 Apr 2012, Andrey Rahmatullin wrote:
> On Fri, Apr 13, 2012 at 10:10:30AM -0400, Alan Stern wrote:
> > Second, if you do
> >
> > echo 0 >/sys/bus/usb/devices/usb1/bConfigurationValue
> > echo 0 >/sys/bus/usb/devices/usb2/bConfigurationValue
> >
> > before suspending (using a vanilla kernel and no script), does it
> > change the behavior? I expect it won't, but we should check just to be
> > thorough.
> No, it still locks up.
Okay.
This is my next attempt to make the driver-bound and driver-unbound
suspend paths as similar as possible. Apply this to a vanilla kernel;
it is based on 3.4-rc2 plus a few unrelated changes. See what happens
when you suspend without unbinding ehci-hcd -- and be sure to include
"no_console_suspend" on the boot command line beforehand.
These changes will prevent the driver from working after resume
(assuming the computer survives that long). That's all right; all I
care about is whether the computer _does_ resume.
Alan Stern
Index: usb-3.4/drivers/pci/pci-driver.c
===================================================================
--- usb-3.4.orig/drivers/pci/pci-driver.c
+++ usb-3.4/drivers/pci/pci-driver.c
@@ -713,6 +713,8 @@ static int pci_pm_suspend_noirq(struct d
if (!pm) {
pci_save_state(pci_dev);
+ pci_prepare_to_sleep(pci_dev);
+ pci_pm_set_unknown_state(pci_dev);
return 0;
}
Index: usb-3.4/drivers/pci/pci.c
===================================================================
--- usb-3.4.orig/drivers/pci/pci.c
+++ usb-3.4/drivers/pci/pci.c
@@ -1348,6 +1348,7 @@ void pci_disable_enabled_device(struct p
if (pci_is_enabled(dev))
do_pci_disable_device(dev);
}
+EXPORT_SYMBOL(pci_disable_enabled_device);
/**
* pci_disable_device - Disable PCI device after use
@@ -1710,6 +1711,7 @@ pci_power_t pci_target_state(struct pci_
*/
int pci_prepare_to_sleep(struct pci_dev *dev)
{
+ pci_power_t cur_state = dev->current_state;
pci_power_t target_state = pci_target_state(dev);
int error;
@@ -1723,6 +1725,8 @@ int pci_prepare_to_sleep(struct pci_dev
if (error)
pci_enable_wake(dev, target_state, false);
+ dev_info(&dev->dev, "cur %d target %d error %d\n", cur_state,
+ target_state, error);
return error;
}
Index: usb-3.4/drivers/usb/core/hcd-pci.c
===================================================================
--- usb-3.4.orig/drivers/usb/core/hcd-pci.c
+++ usb-3.4/drivers/usb/core/hcd-pci.c
@@ -381,6 +381,8 @@ static int check_root_hub_suspended(stru
}
#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME)
+extern void pci_disable_enabled_device(struct pci_dev *);
+
static int suspend_common(struct device *dev, bool do_wakeup)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -427,12 +429,17 @@ static int suspend_common(struct device
if (!hcd->msix_enabled)
synchronize_irq(pci_dev->irq);
+ free_irq(hcd->irq, hcd);
+ iounmap(hcd->regs);
+ pci_disable_device(pci_dev);
+
/* Downstream ports from this root hub should already be quiesced, so
* there will be no DMA activity. Now we can shut down the upstream
* link (except maybe for PME# resume signaling). We'll enter a
* low power state during suspend_noirq, if the hardware allows.
*/
- pci_disable_device(pci_dev);
+ pci_disable_enabled_device(pci_dev);
+ pci_dev->current_state = PCI_UNKNOWN;
return retval;
}
Index: usb-3.4/drivers/usb/host/ehci-pci.c
===================================================================
--- usb-3.4.orig/drivers/usb/host/ehci-pci.c
+++ usb-3.4/drivers/usb/host/ehci-pci.c
@@ -342,7 +342,6 @@ static int ehci_pci_suspend(struct usb_h
* mark HW unaccessible. The PM and USB cores make sure that
* the root hub is either suspended or stopped.
*/
- ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup);
spin_lock_irqsave (&ehci->lock, flags);
ehci_writel(ehci, 0, &ehci->regs->intr_enable);
(void)ehci_readl(ehci, &ehci->regs->intr_enable);
@@ -350,6 +349,9 @@ static int ehci_pci_suspend(struct usb_h
clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
spin_unlock_irqrestore (&ehci->lock, flags);
+ ehci_silence_controller(ehci);
+ ehci_reset(ehci);
+
// could save FLADJ in case of Vaux power loss
// ... we'd only use it to handle clock skew
^ permalink raw reply
* Re: ehci_hcd related S3 lockup on ASUS laptops, again
From: Andrey Rahmatullin @ 2012-04-16 21:19 UTC (permalink / raw)
To: Alan Stern; +Cc: jrnieder, linux-pm, USB list, Steven Rostedt
In-Reply-To: <Pine.LNX.4.44L0.1204161558220.1408-100000@iolanthe.rowland.org>
[-- Attachment #1.1: Type: text/plain, Size: 4325 bytes --]
On Mon, Apr 16, 2012 at 04:07:10PM -0400, Alan Stern wrote:
> This is my next attempt to make the driver-bound and driver-unbound
> suspend paths as similar as possible. Apply this to a vanilla kernel;
> it is based on 3.4-rc2 plus a few unrelated changes. See what happens
> when you suspend without unbinding ehci-hcd -- and be sure to include
> "no_console_suspend" on the boot command line beforehand.
It suspends and resumes but the screen is not enabled after resume and the
network doesn't seem to work either.
> These changes will prevent the driver from working after resume
> (assuming the computer survives that long). That's all right; all I
> care about is whether the computer _does_ resume.
>
> Alan Stern
>
>
>
>
> Index: usb-3.4/drivers/pci/pci-driver.c
> ===================================================================
> --- usb-3.4.orig/drivers/pci/pci-driver.c
> +++ usb-3.4/drivers/pci/pci-driver.c
> @@ -713,6 +713,8 @@ static int pci_pm_suspend_noirq(struct d
>
> if (!pm) {
> pci_save_state(pci_dev);
> + pci_prepare_to_sleep(pci_dev);
> + pci_pm_set_unknown_state(pci_dev);
> return 0;
> }
>
> Index: usb-3.4/drivers/pci/pci.c
> ===================================================================
> --- usb-3.4.orig/drivers/pci/pci.c
> +++ usb-3.4/drivers/pci/pci.c
> @@ -1348,6 +1348,7 @@ void pci_disable_enabled_device(struct p
> if (pci_is_enabled(dev))
> do_pci_disable_device(dev);
> }
> +EXPORT_SYMBOL(pci_disable_enabled_device);
>
> /**
> * pci_disable_device - Disable PCI device after use
> @@ -1710,6 +1711,7 @@ pci_power_t pci_target_state(struct pci_
> */
> int pci_prepare_to_sleep(struct pci_dev *dev)
> {
> + pci_power_t cur_state = dev->current_state;
> pci_power_t target_state = pci_target_state(dev);
> int error;
>
> @@ -1723,6 +1725,8 @@ int pci_prepare_to_sleep(struct pci_dev
> if (error)
> pci_enable_wake(dev, target_state, false);
>
> + dev_info(&dev->dev, "cur %d target %d error %d\n", cur_state,
> + target_state, error);
> return error;
> }
>
> Index: usb-3.4/drivers/usb/core/hcd-pci.c
> ===================================================================
> --- usb-3.4.orig/drivers/usb/core/hcd-pci.c
> +++ usb-3.4/drivers/usb/core/hcd-pci.c
> @@ -381,6 +381,8 @@ static int check_root_hub_suspended(stru
> }
>
> #if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME)
> +extern void pci_disable_enabled_device(struct pci_dev *);
> +
> static int suspend_common(struct device *dev, bool do_wakeup)
> {
> struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -427,12 +429,17 @@ static int suspend_common(struct device
> if (!hcd->msix_enabled)
> synchronize_irq(pci_dev->irq);
>
> + free_irq(hcd->irq, hcd);
> + iounmap(hcd->regs);
> + pci_disable_device(pci_dev);
> +
> /* Downstream ports from this root hub should already be quiesced, so
> * there will be no DMA activity. Now we can shut down the upstream
> * link (except maybe for PME# resume signaling). We'll enter a
> * low power state during suspend_noirq, if the hardware allows.
> */
> - pci_disable_device(pci_dev);
> + pci_disable_enabled_device(pci_dev);
> + pci_dev->current_state = PCI_UNKNOWN;
> return retval;
> }
>
> Index: usb-3.4/drivers/usb/host/ehci-pci.c
> ===================================================================
> --- usb-3.4.orig/drivers/usb/host/ehci-pci.c
> +++ usb-3.4/drivers/usb/host/ehci-pci.c
> @@ -342,7 +342,6 @@ static int ehci_pci_suspend(struct usb_h
> * mark HW unaccessible. The PM and USB cores make sure that
> * the root hub is either suspended or stopped.
> */
> - ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup);
> spin_lock_irqsave (&ehci->lock, flags);
> ehci_writel(ehci, 0, &ehci->regs->intr_enable);
> (void)ehci_readl(ehci, &ehci->regs->intr_enable);
> @@ -350,6 +349,9 @@ static int ehci_pci_suspend(struct usb_h
> clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> spin_unlock_irqrestore (&ehci->lock, flags);
>
> + ehci_silence_controller(ehci);
> + ehci_reset(ehci);
> +
> // could save FLADJ in case of Vaux power loss
> // ... we'd only use it to handle clock skew
>
>
>
--
WBR, wRAR
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: Regarding Devfreq
From: Satendra... @ 2012-04-17 9:28 UTC (permalink / raw)
To: MyungJoo Ham; +Cc: linux-pm
In-Reply-To: <CAJ0PZbQPHMNAw459PwSF5fVc0kRN2J_cp7mbtAGwOBt-_-3JWQ@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 15066 bytes --]
Hi Ham,
On 16 April 2012 17:40, MyungJoo Ham <myungjoo.ham@gmail.com> wrote:
> On Mon, Apr 16, 2012 at 7:19 PM, MyungJoo Ham <myungjoo.ham@gmail.com>
> wrote:
> > On Mon, Apr 16, 2012 at 4:02 PM, Satendra... <satendra.pratap@gmail.com>
> wrote:
> >> Hi Ham,
> >>
> >> I have used jiffies to calculate the busy time and total time for a
> module
> >> (made of 'n' functions say).
> >> I have added a bew structure:
> >
> > Hi.
> >
> > In my ARM SoC systems, one jiffy is 5ms. I don't think other systems
> > won't vary too much (should be around several hundred us to several
> > ms). Are you sure that jiffy has enough granularity for your system?
> > If you are counting the length spent in a function call, shouldn't
> > nsecs be usec? (ktime?). And for saving such values, the
> > "private_data" is intended for individual devfreq drivers
> > communicating with governors specific to the drivers. Devfreq->data is
> > intended for individual devfreq governors. Thus, yes, platform-data is
> > the right place for devfreq driver internal.
> >
> > Cheers!
> > MyungJoo.
> >
>
> Ah.. and one missing point:
>
> Please consider the concurrency. You may have multiple instance of
> function calls in your device driver. And please do not block them by
> simply adding locks there if this performance counting is the only
> reason to make them mutually exclusive. This is another reason to
> count nsecs (ktime) locally in the function and add the end-start
> (local variable) into the aggregation variable (lock here only or use
> atomic operators).
>
> Cheers!
> MyungJoo.
>
>
Yes thats true that we will have multiple instances of function calls but
every instance of driver
will have its own copy of counters which can be accessed through platform
data.
So AFAICS we need not to worry about concurrency issue because no instance
will touch the
counters of another instance. Please correct if i am wrong.
Below is the code using ktime API:
typedef struct devfreq_perf_counters {
#ifdef DEVFREQ_USE_KTIME
ktime_t fentry_time; /* jiffies at the enrty of a func */
ktime_t start_time; /* starting jiffies for every busy/total \
load cal in the last x seconds*/
ktime_t prev_count; /* last jiffies count */
s64 busy_time; /* total execution jiffies count in the last \
x seconds */
#else
unsigned long fentry_time; /* jiffies at the enrty of a func */
unsigned long start_time; /* starting jiffies for every busy/total \
load cal in the last x
seconds*/
unsigned long prev_count; /* last jiffies count */
unsigned long busy_time; /* total execution jiffies count in the
last \
x seconds */
#endif
}devfreq_counters;
#ifdef DEVFREQ_USE_KTIME
/* using ktime */
#define devfreq_func_start(dc) do {\
(dc)->fentry_time = ktime_get();\
}while(0)
#define devfreq_func_end(dc) do {\
(dc)->prev_count = ktime_get();\
(dc)->busy_time += ktime_to_ns(ktime_sub((dc)->prev_count, \
(dc)->fentry_time));\
}while(0)
#define module_busy_time(dc) ((dc)->busy_time)
#define module_busy_plus_not_busy_time(dc) \
ktime_to_ns(ktime_sub(ktime_get(), (dc)->start_time))
#define devfreq_reset_counters(dc) do { \
(dc)->start_time = ktime_get();\
}while(0)
#else
#define devfreq_func_start(dc) \
(dc)->fentry_time = jiffies
#define devfreq_fun_end(dc) do { \
(dc)->prev_count = (jiffies)-((dc)->fentry_time);\
(dc)->busy_time += (dc)->prev_count;\
}while(0)
#define module_busy_time(dc) \
jiffies_to_usecs((dc)->busy_time)
#define module_busy_plus_not_busy_time(dc) \
jiffies_to_usecs(((jiffies) - (dc)->start_time))
#define devfreq_reset_counters(dc) do { \
(dc)->fentry_time = 0UL; \
(dc)->prev_count = 0UL; \
(dc)->busy_time = 0UL; \
(dc)->start_time = jiffies;\
}while(0)
#endif
Thanks,
Satendra
> >>
> >> typedef struct devfreq_perf_counters {
> >> unsigned long func_entry_jiffies; /* jiffies at the enrty of a
> func
> >> */
> >> unsigned long start_jiffies; /* starting jiffies for every
> >> busy/total \
> >> load cal in the last x
> >> seconds*/
> >> unsigned long local_jiffies; /* last jiffies count */
> >> unsigned long busy_jiffies; /* total execution jiffies count in
> the
> >> last \
> >> x seconds */
> >> }devfreq_counters;
> >>
> >> This structure's instance can be included in the platform_data(or
> private
> >> data) of the
> >> driver which has to register itself with devfreq. This way while doing
> >> set_drv_data, access
> >> to these counters could be available all the time.
> >>
> >> Now there is one initialization function :
> >>
> >> void devfreq_counters_init(void *data)
> >> {
> >> devfreq_counters **dc = &data->dpc;
> >>
> >> *dc = kzalloc(sizeof (devfreq_counters), GFP_KERNEL);
> >>
> >> (*dc)->func_entry_jiffies = 0UL;
> >> (*dc)->local_jiffies = 0UL;
> >> (*dc)->busy_jiffies = 0UL;
> >> (*dc)->start_jiffies = jiffies;
> >>
> >> return;
> >> }
> >>
> >> This function initializes the counters and is called at the end of
> driver
> >> initialiation function.
> >>
> >> Now there is a list of APIs as shown below:
> >>
> >> #define devfreq_jiffies_start(dc) \
> >> dc->func_entry_jiffies = jiffies
> >>
> >> #define devfreq_jiffies_end(dc) do { \
> >> dc->local_jiffies = (jiffies)-(dc->func_entry_jiffies);\
> >> dc->busy_jiffies += dc->local_jiffies;\
> >> }while(0)
> >>
> >> #define busy_jiffies_in_usec(dc) \
> >> jiffies_to_usecs((dc)->busy_jiffies)
> >>
> >> #define busy_plus_not_busy_jiffies_in_usec(dc) \
> >> jiffies_to_usecs(((jiffies) - (dc)->start_jiffies))
> >>
> >> #define devfreq_reset_counters(dc) do { \
> >> dc->func_entry_jiffies = 0UL; \
> >> dc->local_jiffies = 0UL; \
> >> dc->busy_jiffies = 0UL; \
> >> dc->start_jiffies = jiffies;\
> >> }while(0)
> >>
> >> Description:
> >> - devfreq_jiffies_start(dc): Will be called in the start of the function
> >> (this is one of the functions
> >> which we want to consider in measuring the busy time of the driver)
> >> - devfreq_jiffies_end(dc): Will be called at the end of that function
> >> - busy_jiffies_in_usec(dc): Will return jiffies for which all considered
> >> (for busy time) driver's
> >> functions were busy
> >> - busy_plus_not_busy_jiffies_in_usec(dc): Total jiffies i.e. busy time
> + not
> >> busy time of driver
> >> - devfreq_reset_counters(dc): obvious
> >>
> >> I wanted the simple solution to busy and load and i thought about this.
> This
> >> solution hasn't been verified
> >> yet.
> >>
> >> Ham, can you please provide your views on it.
> >>
> >> Thanks,
> >> Satendra
> >>
> >>
> >> On 29 March 2012 07:44, MyungJoo Ham <myungjoo.ham@gmail.com> wrote:
> >>>
> >>> 2012/3/28 Satendra... <satendra.pratap@gmail.com>
> >>> >
> >>> > Hi Ham,
> >>> >
> >>> > In the below struct :
> >>> > struct devfreq_dev_status {
> >>> > /* both since the last measure */
> >>> > unsigned long total_time;
> >>> > unsigned long busy_time;
> >>> > unsigned long current_frequency;
> >>> > void *private_data;
> >>> > };
> >>> >
> >>> > How to calculate total_time/busy_time? Does it require hardware
> support
> >>> > (may be some performance counters)?
> >>> >
> >>> > Thanks,
> >>> > Satendra
> >>>
> >>> It is decided by each devfreq device driver, which is why
> >>> get_dev_status() is to be provided by the devfreq device driver.
> >>>
> >>> If the device has performance counters, then, it's great, but
> >>> performance counters are not mandatory.
> >>>
> >>> Here goes a list of mechanisms to fetch busy/total time:
> >>> - Performance counters
> >>> - Measure the time between "operation start" and "operation end" and
> >>> accumulate the time (getting busy time. probably by ktime?)
> >>> - Measure the idle time (CPUIDLE/CPUFREQ does this)
> >>> - Count the number of operation and calculate the operational time
> >>> based on the number.
> >>> - and so on.
> >>>
> >>> As you can see in the list, you don't need a hardware support such as
> >>> performance counters.
> >>>
> >>>
> >>>
> >>> Cheers!
> >>> MyungJoo.
> >>>
> >>> >
> >>> >
> >>> > On 21 March 2012 15:49, 함명주 <myungjoo.ham@samsung.com> wrote:
> >>> >>
> >>> >> Hi Stendra,
> >>> >>
> >>> >>
> >>> >>
> >>> >> - Bascially, you can determine the voltage based on frequency;
> thus, we
> >>> >> do not need to determint voltages at DVFS framework. It is
> >>> >> corresponding
> >>> >> device driver's responsibility even when we have AVS features. Thus,
> >>> >> yes,
> >>> >> the target callback needs to control both frequency and voltage (and
> >>> >> anything else required to change the frequency/voltage)
> >>> >>
> >>> >>
> >>> >>
> >>> >> - I recommend to use regulator framework to control regulators
> unless
> >>> >> you
> >>> >> really really need to ignore regulator framework. Don't reinvent the
> >>> >> wheel.
> >>> >>
> >>> >>
> >>> >>
> >>> >> - The three steps you've mentioned are correct.
> >>> >>
> >>> >>
> >>> >>
> >>> >> - The mailing list is opened to all. You are welcomed to use it
> (CC'ed
> >>> >> linux-pm) and just CC needed people.
> >>> >>
> >>> >>
> >>> >>
> >>> >>
> >>> >>
> >>> >> Cheers!
> >>> >>
> >>> >> MyungJoo.
> >>> >>
> >>> >>
> >>> >>
> >>> >> ------- Original Message -------
> >>> >>
> >>> >> Sender : Satendra...<satendra.pratap@gmail.com>
> >>> >>
> >>> >> Date : 2012-03-21 19:03 (GMT+09:00)
> >>> >>
> >>> >> Title : Re: Re: Regarding Devfreq
> >>> >>
> >>> >>
> >>> >>
> >>> >> Hi Ham,
> >>> >>
> >>> >> Thank you very much for your reply. I will try not to disturb you
> much.
> >>> >> In Devfreq I dont see any API related to voltage scaling. Is it the
> >>> >> "->target" function's (in struct devfreq_dev_profile)
> >>> >> responsibility to change the voltage as well with frequency?
> >>> >> Do we really need to register our regulators using regulator
> framework
> >>> >> of
> >>> >> linux for voltage scaling?
> >>> >>
> >>> >> For a clear understanding I need to do following to use devfreq (I
> am
> >>> >> sorry as I may be verifying it again):
> >>> >> - define an instance of "struct devfreq_dev_profile" and provide
> >>> >> implementations of target, get_dev_status and exit callbacks
> >>> >> - Implement our choice of governer or use any one from already
> >>> >> implemented ones.
> >>> >> - call "devfreq_add_device" function from our driver's probe
> function.
> >>> >>
> >>> >> is that it? or do we need to do something else also?
> >>> >>
> >>> >> Thanks,
> >>> >> Satendra
> >>> >>
> >>> >>
> >>> >> On 21 March 2012 15:14, 함명주 <myungjoo.ham@samsung.com> wrote:
> >>> >>>
> >>> >>> Hello Satendra,
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>> - For Devfreq, OPP is optional though recommended for easier
> >>> >>> implementation. You can still implement all the needed things
> without
> >>> >>> OPP.
> >>> >>> OPP is just a simple data structure to store pairs of voltage and
> >>> >>> frequency.
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>> - Yes, you need to call devfreq_add_device() and supply the
> required
> >>> >>> data. You can implement your own governor or use one of predefined
> >>> >>> governors. Runtime replacement of governors like CPUfreq is "TODO"
> for
> >>> >>> now.
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>> - For example, you can see /drivers/devfreq/exynos4_bus.c. GPU,
> >>> >>> Display
> >>> >>> devfreq drivers are under development in other companies (ARM).
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>> Cheers!
> >>> >>>
> >>> >>> MyungJoo.
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>> ------- Original Message -------
> >>> >>>
> >>> >>> Sender : Satendra...<satendra.pratap@gmail.com>
> >>> >>>
> >>> >>> Date : 2012-03-21 17:31 (GMT+09:00)
> >>> >>>
> >>> >>> Title : Re: Regarding Devfreq
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>> Hi Ham,
> >>> >>>
> >>> >>> I have studied DVFS and what I feel is that we need to use OPP and
> >>> >>> Voltage regulator interfaces also to
> >>> >>> maintain Optimum Performance Points and to provide APIs to change
> the
> >>> >>> voltage.
> >>> >>> What I feel is that to use DVFS every driver has to
> >>> >>> call devfreq_add_device function to register that device to
> >>> >>> the devfreq framework. And in order to do that we have to
> >>> >>> implement devfreq_dev_profile and our choice of governer.
> >>> >>>
> >>> >>> I appreciate your help.
> >>> >>>
> >>> >>> Thanks,
> >>> >>> Satendra
> >>> >>>
> >>> >>> On 20 March 2012 12:16, Satendra... <satendra.pratap@gmail.com>
> wrote:
> >>> >>>>
> >>> >>>> Hi Ham,
> >>> >>>>
> >>> >>>> We are working on a new SoC for our new product and would want to
> use
> >>> >>>> your Devfreq
> >>> >>>> framework for our devices. Would you please let me know any other
> >>> >>>> implementation which
> >>> >>>> uses devfreq ? so that we could take that as a reference.
> >>> >>>> Or we would be the first one to start?
> >>> >>>>
> >>> >>>> Thanks,
> >>> >>>> Satendra
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>>
> >>> >>> --
> >>> >>>
> >>> >>> MyungJoo Ham (함명주), PHD
> >>> >>>
> >>> >>> System S/W Lab, S/W Platform Team, Software Center
> >>> >>> Samsung Electronics
> >>> >>> Cell: +82-10-6714-2858
> >>> >>>
> >>> >>>
> >>> >>
> >>> >>
> >>> >>
> >>> >>
> >>> >>
> >>> >>
> >>> >> --
> >>> >>
> >>> >> MyungJoo Ham (함명주), PHD
> >>> >>
> >>> >> System S/W Lab, S/W Platform Team, Software Center
> >>> >> Samsung Electronics
> >>> >> Cell: +82-10-6714-2858
> >>> >>
> >>> >>
> >>> >
> >>> >
> >>> >
> >>> > _______________________________________________
> >>> > linux-pm mailing list
> >>> > linux-pm@lists.linux-foundation.org
> >>> > https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
> >>>
> >>>
> >>>
> >>>
> >>> --
> >>> MyungJoo Ham, Ph.D.
> >>> System S/W Lab, S/W Center, Samsung Electronics
> >>
> >>
> >
> >
> >
> > --
> > MyungJoo Ham, Ph.D.
> > System S/W Lab, S/W Center, Samsung Electronics
>
>
>
> --
> MyungJoo Ham, Ph.D.
> System S/W Lab, S/W Center, Samsung Electronics
>
[-- Attachment #1.2: Type: text/html, Size: 22473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: Devfreq Simple On Demand Governor
From: Satendra... @ 2012-04-17 10:19 UTC (permalink / raw)
To: MyungJoo Ham; +Cc: linux-pm
In-Reply-To: <CAJ0PZbQB3USyV65KNERn01yF-KFgi9o7yRJquoOc0iBWotv6PA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 2752 bytes --]
Hi Ham,
Thanks for your replies. Below are my thoughts.
On 16 April 2012 19:46, MyungJoo Ham <myungjoo.ham@gmail.com> wrote:
> On Mon, Apr 16, 2012 at 7:07 PM, Satendra... <satendra.pratap@gmail.com>
> wrote:
> > Hi Ham,
> >
> > I was looking at the simple on demand governor's code and have got few
> > queries:
> > - In the code below to check for overflow, why 24 and 7 has been taken?
> > /* Prevent overflow */
> > if (stat.busy_time >= (1 << 24) || stat.total_time >= (1 << 24))
> {
> > stat.busy_time >>= 7;
> > stat.total_time >>= 7;
> > }
>
> It is because 1<<7 >= 100 and 1<<6 < 100. (To get percentage, we are
> multiplying 100).
>
> ok. but what about 24? Is it assumed that no one will be allowed to
execute more than 2^(24-7) units (nsec, msecs etc)?
Is 2^(24-7) = 128 * 1024 units (nsecs, msecs etc) the default time slice of
kernel to any code?
> >
> > - Also for maximum frequency UINT_MAX is returned which may not be suited
> > for some devices.
>
> The frequency given to devfreq driver is "recommended frequency", not
> the "exact frequency required".
>
> Thus, the driver is required to choose
> 1. frequency that is at least (but closest) or same with the
> "recommended frequency"
> and if that's impossible,
> 2. frequency that is closest to the "recommended frequency".
>
> Thus, if it's "UINT_MAX", then it should choose the maximum frequency.
>
>
You mean we have to check for UINT_MAX in our code and if its so then set
device at its maximum frequency.
> >
> > - One more query, whats the unit (usecs, nsecs etc) used for busy and
> load
> > time. Has it got nothing to do with units
> > and its mere the percentage of busy time?
>
> The unit is up to the device driver. It only needs to provide
> "reasonable" values in u32 to the infrastructure. It could be usec,
> nsec, #ops, or anything else.
> I've mentioned it only because jiffy does not seem to be "reasonable";
> jiffy is too rough. You may even get 0 jiffies for 100 operations
> while you may get 1 jiffy for 1 operation in the same system.
>
> In the governors, maybe busy / total ratio can be used as in
> simple_ondemand. And you should give values that can support some fair
> granularity there, which jiffy cannot.
>
> >
> > - If we want to use simple on demand governor then do we need to modify
> > yours to suit our needs? I mean you have
> > just provided a framework?
>
> If you want to use THE simple_ondemand for your device, you only need
> to create a devfreq driver. You don't need to modify the governor or
> the framework/.
>
> >
> > Thanks,
> > Satendra
>
>
> Cheers!
> MyungJoo.
>
> --
> MyungJoo Ham, Ph.D.
> System S/W Lab, S/W Center, Samsung Electronics
>
Thanks,
Satendra
[-- Attachment #1.2: Type: text/html, Size: 3984 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: ehci_hcd related S3 lockup on ASUS laptops, again
From: Alan Stern @ 2012-04-17 15:11 UTC (permalink / raw)
To: Andrey Rahmatullin; +Cc: jrnieder, linux-pm, USB list, Steven Rostedt
In-Reply-To: <20120416211943.GE11484@belkar.wrar.name>
On Tue, 17 Apr 2012, Andrey Rahmatullin wrote:
> On Mon, Apr 16, 2012 at 04:07:10PM -0400, Alan Stern wrote:
> > This is my next attempt to make the driver-bound and driver-unbound
> > suspend paths as similar as possible. Apply this to a vanilla kernel;
> > it is based on 3.4-rc2 plus a few unrelated changes. See what happens
> > when you suspend without unbinding ehci-hcd -- and be sure to include
> > "no_console_suspend" on the boot command line beforehand.
> It suspends and resumes but the screen is not enabled after resume and the
> network doesn't seem to work either.
Well, at least it's progress. It's possible that those other problems
were caused by the rather drastic things the patch does.
Just to make sure, did you test the patch with the script installed
(that is, with ehci-hcd unbound)? I assume that will work normally.
Moving on, the next thing is to remove changes from the patch, one at a
time, until we find one that prevents the system from resuming. So,
testing at each step, please remove from the patch:
1. This change in hcd-pci.c:
+ pci_dev->current_state = PCI_UNKNOWN;
2. This change in ehci-pci.c:
+ ehci_silence_controller(ehci);
3. This change in hcd-pci.c:
+ pci_disable_device(pci_dev);
4. This change in hcd-pci.c:
+ iounmap(hcd->regs);
5. This change in hcd-pci.c:
+ free_irq(hcd->irq, hcd);
6. This change in hcd-pci.c:
- pci_disable_device(pci_dev);
+ pci_disable_enabled_device(pci_dev);
7. This change in ehci-pci.c:
- ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup);
Once all those things have been removed, the patch should be the same
as one you tried earlier, which did crash the machine.
Alan Stern
^ permalink raw reply
* Re: ehci_hcd related S3 lockup on ASUS laptops, again
From: Andrey Rahmatullin @ 2012-04-17 16:25 UTC (permalink / raw)
To: Alan Stern; +Cc: jrnieder, linux-pm, USB list, Steven Rostedt
In-Reply-To: <Pine.LNX.4.44L0.1204171057470.1364-100000@iolanthe.rowland.org>
[-- Attachment #1.1: Type: text/plain, Size: 1200 bytes --]
On Tue, Apr 17, 2012 at 11:11:04AM -0400, Alan Stern wrote:
> Just to make sure, did you test the patch with the script installed
> (that is, with ehci-hcd unbound)? I assume that will work normally.
It works normally, yes.
>
> Moving on, the next thing is to remove changes from the patch, one at a
> time, until we find one that prevents the system from resuming. So,
> testing at each step, please remove from the patch:
>
> 1. This change in hcd-pci.c:
> + pci_dev->current_state = PCI_UNKNOWN;
It locks up if this line is removed.
> 2. This change in ehci-pci.c:
> + ehci_silence_controller(ehci);
>
> 3. This change in hcd-pci.c:
> + pci_disable_device(pci_dev);
>
> 4. This change in hcd-pci.c:
> + iounmap(hcd->regs);
>
> 5. This change in hcd-pci.c:
> + free_irq(hcd->irq, hcd);
>
> 6. This change in hcd-pci.c:
> - pci_disable_device(pci_dev);
> + pci_disable_enabled_device(pci_dev);
>
> 7. This change in ehci-pci.c:
> - ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup);
>
> Once all those things have been removed, the patch should be the same
> as one you tried earlier, which did crash the machine.
--
WBR, wRAR
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: ehci_hcd related S3 lockup on ASUS laptops, again
From: Alan Stern @ 2012-04-17 16:58 UTC (permalink / raw)
To: Andrey Rahmatullin; +Cc: jrnieder, linux-pm, USB list, Steven Rostedt
In-Reply-To: <20120417162534.GL11484@belkar.wrar.name>
On Tue, 17 Apr 2012, Andrey Rahmatullin wrote:
> On Tue, Apr 17, 2012 at 11:11:04AM -0400, Alan Stern wrote:
> > Just to make sure, did you test the patch with the script installed
> > (that is, with ehci-hcd unbound)? I assume that will work normally.
> It works normally, yes.
>
> >
> > Moving on, the next thing is to remove changes from the patch, one at a
> > time, until we find one that prevents the system from resuming. So,
> > testing at each step, please remove from the patch:
> >
> > 1. This change in hcd-pci.c:
> > + pci_dev->current_state = PCI_UNKNOWN;
> It locks up if this line is removed.
Very bizarre indeed. All right, leave that line in place and try
removing the other changes, one at a time. In fact, let's add one more
thing to remove from the patch
> > 2. This change in ehci-pci.c:
> > + ehci_silence_controller(ehci);
> >
> > 3. This change in hcd-pci.c:
> > + pci_disable_device(pci_dev);
> >
> > 4. This change in hcd-pci.c:
> > + iounmap(hcd->regs);
> >
> > 5. This change in hcd-pci.c:
> > + free_irq(hcd->irq, hcd);
> >
> > 6. This change in hcd-pci.c:
> > - pci_disable_device(pci_dev);
> > + pci_disable_enabled_device(pci_dev);
> >
> > 7. This change in ehci-pci.c:
> > - ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup);
8. This change in ehci-pci.c:
+ ehci_reset(ehci);
If you manage to reach the end of this list, you'll essentially be
doing a normal suspend (except for the PCI_UNKNOWN part).
Alan Stern
^ permalink raw reply
* Re: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again
From: Andrey Rahmatullin @ 2012-04-17 17:51 UTC (permalink / raw)
To: Alan Stern
Cc: Steven Rostedt, jrnieder-Re5JQEeQqe8AvxtiuMwx3w,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <Pine.LNX.4.44L0.1204171251330.1364-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1215 bytes --]
On Tue, Apr 17, 2012 at 12:58:03PM -0400, Alan Stern wrote:
> > > 1. This change in hcd-pci.c:
> > > + pci_dev->current_state = PCI_UNKNOWN;
> > It locks up if this line is removed.
>
> Very bizarre indeed. All right, leave that line in place and try
> removing the other changes, one at a time. In fact, let's add one more
> thing to remove from the patch
>
> > > 2. This change in ehci-pci.c:
> > > + ehci_silence_controller(ehci);
> > >
> > > 3. This change in hcd-pci.c:
> > > + pci_disable_device(pci_dev);
> > >
> > > 4. This change in hcd-pci.c:
> > > + iounmap(hcd->regs);
> > >
> > > 5. This change in hcd-pci.c:
> > > + free_irq(hcd->irq, hcd);
> > >
> > > 6. This change in hcd-pci.c:
> > > - pci_disable_device(pci_dev);
> > > + pci_disable_enabled_device(pci_dev);
> > >
> > > 7. This change in ehci-pci.c:
> > > - ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup);
>
> 8. This change in ehci-pci.c:
> + ehci_reset(ehci);
>
> If you manage to reach the end of this list, you'll essentially be
> doing a normal suspend (except for the PCI_UNKNOWN part).
It works without changes #2..8. I'm attaching the resulting patch.
--
WBR, wRAR
[-- Attachment #1.2: current.patch --]
[-- Type: text/x-diff, Size: 3465 bytes --]
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 6b54b23..8a9edf4 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -713,6 +713,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
if (!pm) {
pci_save_state(pci_dev);
+ pci_prepare_to_sleep(pci_dev);
+ pci_pm_set_unknown_state(pci_dev);
return 0;
}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8156744..3111105 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1348,6 +1348,7 @@ void pci_disable_enabled_device(struct pci_dev *dev)
if (pci_is_enabled(dev))
do_pci_disable_device(dev);
}
+EXPORT_SYMBOL(pci_disable_enabled_device);
/**
* pci_disable_device - Disable PCI device after use
@@ -1710,6 +1711,7 @@ pci_power_t pci_target_state(struct pci_dev *dev)
*/
int pci_prepare_to_sleep(struct pci_dev *dev)
{
+ pci_power_t cur_state = dev->current_state;
pci_power_t target_state = pci_target_state(dev);
int error;
@@ -1723,6 +1725,8 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
if (error)
pci_enable_wake(dev, target_state, false);
+ dev_info(&dev->dev, "cur %d target %d error %d\n", cur_state,
+ target_state, error);
return error;
}
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 622b4a4..d7dc939 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -381,6 +381,8 @@ static int check_root_hub_suspended(struct device *dev)
}
#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME)
+extern void pci_disable_enabled_device(struct pci_dev *);
+
static int suspend_common(struct device *dev, bool do_wakeup)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -427,12 +429,18 @@ static int suspend_common(struct device *dev, bool do_wakeup)
if (!hcd->msix_enabled)
synchronize_irq(pci_dev->irq);
+// free_irq(hcd->irq, hcd); // 5
+// iounmap(hcd->regs); // 4
+// pci_disable_device(pci_dev); // 3
+
/* Downstream ports from this root hub should already be quiesced, so
* there will be no DMA activity. Now we can shut down the upstream
* link (except maybe for PME# resume signaling). We'll enter a
* low power state during suspend_noirq, if the hardware allows.
*/
+// pci_disable_enabled_device(pci_dev); // 6
pci_disable_device(pci_dev);
+ pci_dev->current_state = PCI_UNKNOWN; // 1
return retval;
}
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 01bb7241d..5fbfc3e 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -342,7 +342,7 @@ static int ehci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
* mark HW unaccessible. The PM and USB cores make sure that
* the root hub is either suspended or stopped.
*/
- ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup);
+ ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup); // 7
spin_lock_irqsave (&ehci->lock, flags);
ehci_writel(ehci, 0, &ehci->regs->intr_enable);
(void)ehci_readl(ehci, &ehci->regs->intr_enable);
@@ -350,6 +350,9 @@ static int ehci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
spin_unlock_irqrestore (&ehci->lock, flags);
+// ehci_silence_controller(ehci); // 2
+// ehci_reset(ehci); // 8
+
// could save FLADJ in case of Vaux power loss
// ... we'd only use it to handle clock skew
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again
From: Alan Stern @ 2012-04-17 18:26 UTC (permalink / raw)
To: Andrey Rahmatullin
Cc: Steven Rostedt, jrnieder-Re5JQEeQqe8AvxtiuMwx3w,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <20120417175122.GM11484-hAV9HEAGFNe6YibBOCjzsw@public.gmane.org>
On Tue, 17 Apr 2012, Andrey Rahmatullin wrote:
> > If you manage to reach the end of this list, you'll essentially be
> > doing a normal suspend (except for the PCI_UNKNOWN part).
> It works without changes #2..8. I'm attaching the resulting patch.
Wow. Okay, I have boiled this down to a single patch. You should
try this both with and without unbinding ehci-hcd, and post the dmesg
log output that it generates in the two cases.
Alan Stern
Index: usb-3.4/drivers/pci/pci-driver.c
===================================================================
--- usb-3.4.orig/drivers/pci/pci-driver.c
+++ usb-3.4/drivers/pci/pci-driver.c
@@ -713,6 +713,8 @@ static int pci_pm_suspend_noirq(struct d
if (!pm) {
pci_save_state(pci_dev);
+ pci_prepare_to_sleep(pci_dev);
+ pci_pm_set_unknown_state(pci_dev);
return 0;
}
Index: usb-3.4/drivers/pci/pci.c
===================================================================
--- usb-3.4.orig/drivers/pci/pci.c
+++ usb-3.4/drivers/pci/pci.c
@@ -1710,6 +1710,7 @@ pci_power_t pci_target_state(struct pci_
*/
int pci_prepare_to_sleep(struct pci_dev *dev)
{
+ pci_power_t cur_state = dev->current_state;
pci_power_t target_state = pci_target_state(dev);
int error;
@@ -1723,6 +1724,8 @@ int pci_prepare_to_sleep(struct pci_dev
if (error)
pci_enable_wake(dev, target_state, false);
+ dev_info(&dev->dev, "cur %d target %d error %d\n", cur_state,
+ target_state, error);
return error;
}
Index: usb-3.4/drivers/usb/core/hcd-pci.c
===================================================================
--- usb-3.4.orig/drivers/usb/core/hcd-pci.c
+++ usb-3.4/drivers/usb/core/hcd-pci.c
@@ -433,6 +433,7 @@ static int suspend_common(struct device
* low power state during suspend_noirq, if the hardware allows.
*/
pci_disable_device(pci_dev);
+ pci_dev->current_state = PCI_UNKNOWN;
return retval;
}
--
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: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again
From: Andrey Rahmatullin @ 2012-04-17 18:51 UTC (permalink / raw)
To: Alan Stern
Cc: Steven Rostedt, jrnieder-Re5JQEeQqe8AvxtiuMwx3w,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <Pine.LNX.4.44L0.1204171423310.1163-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]
On Tue, Apr 17, 2012 at 02:26:24PM -0400, Alan Stern wrote:
> > > If you manage to reach the end of this list, you'll essentially be
> > > doing a normal suspend (except for the PCI_UNKNOWN part).
> > It works without changes #2..8. I'm attaching the resulting patch.
>
> Wow. Okay, I have boiled this down to a single patch. You should
> try this both with and without unbinding ehci-hcd, and post the dmesg
> log output that it generates in the two cases.
Unbound:
pci 0000:00:1d.0: wake-up capability enabled by ACPI
pci 0000:00:1d.0: Refused to change power state, currently in D0
pci 0000:00:1d.0: cur 5 target 3 error 0
pci 0000:00:1a.0: wake-up capability enabled by ACPI
pci 0000:00:1a.0: Refused to change power state, currently in D0
pci 0000:00:1a.0: cur 5 target 3 error 0
Bound:
ehci_hcd 0000:00:1d.0: wake-up capability enabled by ACPI
ehci_hcd 0000:00:1d.0: Refused to change power state, currently in D0
ehci_hcd 0000:00:1d.0: cur 5 target 3 error 0
ehci_hcd 0000:00:1a.0: wake-up capability enabled by ACPI
ehci_hcd 0000:00:1a.0: Refused to change power state, currently in D0
ehci_hcd 0000:00:1a.0: cur 5 target 3 error 0
--
WBR, wRAR
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again
From: Alan Stern @ 2012-04-17 19:20 UTC (permalink / raw)
To: Andrey Rahmatullin
Cc: Steven Rostedt, jrnieder-Re5JQEeQqe8AvxtiuMwx3w,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <20120417185149.GO11484-hAV9HEAGFNe6YibBOCjzsw@public.gmane.org>
On Wed, 18 Apr 2012, Andrey Rahmatullin wrote:
> > Wow. Okay, I have boiled this down to a single patch. You should
> > try this both with and without unbinding ehci-hcd, and post the dmesg
> > log output that it generates in the two cases.
>
> Unbound:
>
> pci 0000:00:1d.0: wake-up capability enabled by ACPI
> pci 0000:00:1d.0: Refused to change power state, currently in D0
> pci 0000:00:1d.0: cur 5 target 3 error 0
> pci 0000:00:1a.0: wake-up capability enabled by ACPI
> pci 0000:00:1a.0: Refused to change power state, currently in D0
> pci 0000:00:1a.0: cur 5 target 3 error 0
>
> Bound:
>
> ehci_hcd 0000:00:1d.0: wake-up capability enabled by ACPI
> ehci_hcd 0000:00:1d.0: Refused to change power state, currently in D0
> ehci_hcd 0000:00:1d.0: cur 5 target 3 error 0
> ehci_hcd 0000:00:1a.0: wake-up capability enabled by ACPI
> ehci_hcd 0000:00:1a.0: Refused to change power state, currently in D0
> ehci_hcd 0000:00:1a.0: cur 5 target 3 error 0
Very good. The two behaviors are the same.
Now if you modify the patch by removing the change to hcd-pci.c, which
will leave the EHCI code exactly the same as in the vanilla kernel, and
set the pm_test value to "platform", what does the dmesg log show in
the two cases?
Steven reported that the power state does get set to D3hot; he did not
get the "Refused to change power state" lines.
I have a strong feeling that your computer crashes during suspend
whenever the EHCI controllers are in a low-power state. We'll see.
Alan Stern
--
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
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