Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> writes: > On Fri, Nov 07, 2014 at 11:47:53AM -0800, Kevin Hilman wrote: >> Ulf Hansson <ulf.hansson@xxxxxxxxxx> writes: >> >> > The initial state of the device's need_restore flag should'nt depend on >> > the current state of the PM domain. For example it should be perfectly >> > valid to attach an inactive device to a powered PM domain. >> > >> > The pm_genpd_dev_need_restore() API allow us to update the need_restore >> > flag to somewhat cope with such scenarios. Typically that should have >> > been done from drivers/buses ->probe() since it's those that put the >> > requirements on the value of the need_restore flag. >> > >> > Until recently, the Exynos SOCs were the only user of the >> > pm_genpd_dev_need_restore() API, though invoking it from a centralized >> > location while adding devices to their PM domains. >> > >> > Due to that Exynos now have swithed to the generic OF-based PM domain >> > look-up, it's no longer possible to invoke the API from a centralized >> > location. The reason is because devices are now added to their PM >> > domains during the probe sequence. >> > >> > Commit "ARM: exynos: Move to generic PM domain DT bindings" >> > did the switch for Exynos to the generic OF-based PM domain look-up, >> > but it also removed the call to pm_genpd_dev_need_restore(). This >> > caused a regression for some of the Exynos drivers. >> > >> > To handle things more properly in the generic PM domain, let's change >> > the default initial value of the need_restore flag to reflect that the >> > state is unknown. As soon as some of the runtime PM callbacks gets >> > invoked, update the initial value accordingly. >> > >> > Moreover, since the generic PM domain is verifying that all device's >> > are both runtime PM enabled and suspended, using pm_runtime_suspended() >> > while pm_genpd_poweroff() is invoked from the scheduled work, we can be >> > sure of that the PM domain won't be powering off while having active >> > devices. >> > >> > Do note that, the generic PM domain can still only know about active >> > devices which has been activated through invoking its runtime PM resume >> > callback. In other words, buses/drivers using pm_runtime_set_active() >> > during ->probe() will still suffer from a race condition, potentially >> > probing a device without having its PM domain being powered. That issue >> > will have to be solved using a different approach. >> > >> > This a log from the boot regression for Exynos5, which is being fixed in >> > this patch. >> > >> > ------------[ cut here ]------------ >> > WARNING: CPU: 0 PID: 308 at ../drivers/clk/clk.c:851 clk_disable+0x24/0x30() >> > Modules linked in: >> > CPU: 0 PID: 308 Comm: kworker/0:1 Not tainted 3.18.0-rc3-00569-gbd9449f-dirty #10 >> > Workqueue: pm pm_runtime_work >> > [<c0013c64>] (unwind_backtrace) from [<c0010dec>] (show_stack+0x10/0x14) >> > [<c0010dec>] (show_stack) from [<c03ee4cc>] (dump_stack+0x70/0xbc) >> > [<c03ee4cc>] (dump_stack) from [<c0020d34>] (warn_slowpath_common+0x64/0x88) >> > [<c0020d34>] (warn_slowpath_common) from [<c0020d74>] (warn_slowpath_null+0x1c/0x24) >> > [<c0020d74>] (warn_slowpath_null) from [<c03107b0>] (clk_disable+0x24/0x30) >> > [<c03107b0>] (clk_disable) from [<c02cc834>] (gsc_runtime_suspend+0x128/0x160) >> > [<c02cc834>] (gsc_runtime_suspend) from [<c0249024>] (pm_generic_runtime_suspend+0x2c/0x38) >> > [<c0249024>] (pm_generic_runtime_suspend) from [<c024f44c>] (pm_genpd_default_save_state+0x2c/0x8c) >> > [<c024f44c>] (pm_genpd_default_save_state) from [<c024ff2c>] (pm_genpd_poweroff+0x224/0x3ec) >> > [<c024ff2c>] (pm_genpd_poweroff) from [<c02501b4>] (pm_genpd_runtime_suspend+0x9c/0xcc) >> > [<c02501b4>] (pm_genpd_runtime_suspend) from [<c024a4f8>] (__rpm_callback+0x2c/0x60) >> > [<c024a4f8>] (__rpm_callback) from [<c024a54c>] (rpm_callback+0x20/0x74) >> > [<c024a54c>] (rpm_callback) from [<c024a930>] (rpm_suspend+0xd4/0x43c) >> > [<c024a930>] (rpm_suspend) from [<c024bbcc>] (pm_runtime_work+0x80/0x90) >> > [<c024bbcc>] (pm_runtime_work) from [<c0032a9c>] (process_one_work+0x12c/0x314) >> > [<c0032a9c>] (process_one_work) from [<c0032cf4>] (worker_thread+0x3c/0x4b0) >> > [<c0032cf4>] (worker_thread) from [<c003747c>] (kthread+0xcc/0xe8) >> > [<c003747c>] (kthread) from [<c000e738>] (ret_from_fork+0x14/0x3c) >> > ---[ end trace 40cd58bcd6988f12 ]--- >> > >> > Fixes: a4a8c2c4962bb655 (ARM: exynos: Move to generic PM domain DT bindings) >> > Reported-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx> >> > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx> >> >> Reviewed-by: Kevin Hilman <khilman@xxxxxxxxxx> >> >> And looks like we need this for v3.18-rc since it does fix the >> regression. > > I guess we do need it for 3.18, but when we are talking about 3.19, > before we make any more changes can we outline how power domains are > supposed to work? Yes, I agree, we have some things to sort out for v3.19, but as this one is a regression fix, I think it needs to be in v3.18-rc. Kevin > 1. How do we attach a device to power domain? Right now it seems that > individual buses are responsible for attaching their devices to power > domains. Should we move it into driver core (device_pm_add?) so that > device starts its life in its proper power domain? > > 2. When do we power up the devices (and the domains)? Right now devices > in ACPI power domain are powered when they are attached to the power > domain (which coincides with probing), but generic power domains do not > do that. Can we add a separate API to explicitly power up the device (and > its domain if it is powered down) and do it again, either in device core > or in individual buses. This way, regardless of runtime PM or not, we > will have devices powered on when driver tries to bind to them. If > binding fails we can power down the device. > > I've heard there is a concern about power domain bouncing up and down > during probing. Is it really a concern? Would not we run into the same > issues past booting up with aggressive PM policy? Anyway, we could > probably work around this by refusing to actually power down the domains > until we are done booting, similarly to who genpd_poweroff_unused works. > > Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |