From patchwork Fri Jul 7 13:03:06 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Rafael J. Wysocki" X-Patchwork-Id: 9830205 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 86546602CA for ; Fri, 7 Jul 2017 13:11:25 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7807628654 for ; Fri, 7 Jul 2017 13:11:25 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6C13628657; Fri, 7 Jul 2017 13:11:25 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 0C6F32867D for ; Fri, 7 Jul 2017 13:11:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=3b4JptI0NrAz5e1Qn/xwWSw9e8gGYTqt8nkljfqwD48=; b=JOao/4Uw8vSP4W 4Y/D/J4ZxQsVv+HD3/snEcCwujfpHzf9U5sBdznE0JrpPYsHNGUMIB0ehA+mkccizctTVP41OZO0m FzmPUD+FPZyvqmEd8NlP4CciYKy+z1f+jFXysyFwCiiKT4MxeGlBvvrd5EYgda0vlNZiDYT3nrfLn CXXG2z6Sjj9bcC+TFHEEHY72bfkjmbkeXsFZGvHt4EBV85I4JorebH8ym6sIJP2u542sHQ06aVoIC +zZPzB8RhRzjds5gS23cOzSCBnY9PfHo7L/0ygo+ylw74m7umZlJ2tJwuUZtLuCbriJ8EcDjgVw6O 4w5O0Ga0bwDQC/2ACopA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dTT2Q-0007Lh-Ru; Fri, 07 Jul 2017 13:11:14 +0000 Received: from cloudserver094114.home.net.pl ([79.96.170.134]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1dTT2I-0006ZZ-24 for linux-arm-kernel@lists.infradead.org; Fri, 07 Jul 2017 13:11:10 +0000 Received: from 79.184.252.2.ipv4.supernova.orange.pl (79.184.252.2) (HELO aspire.rjw.lan) by serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer 0.82) id 5a1a6aa5af96ab9f; Fri, 7 Jul 2017 15:10:41 +0200 From: "Rafael J. Wysocki" To: Peter Chen Subject: Re: [PATCH v16 2/7] power: add power sequence library Date: Fri, 07 Jul 2017 15:03:06 +0200 Message-ID: <2042394.b4eZkzrJ5f@aspire.rjw.lan> User-Agent: KMail/4.14.10 (Linux/4.12.0-rc1+; KDE/4.14.9; x86_64; ; ) In-Reply-To: <20170707080107.GC21909@b29397-desktop> References: <1498027328-25078-1-git-send-email-peter.chen@nxp.com> <2629938.X3D3UTyHBr@aspire.rjw.lan> <20170707080107.GC21909@b29397-desktop> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170707_061106_570759_532DDAB1 X-CRM114-Status: GOOD ( 33.52 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, Peter Chen , ulf.hansson@linaro.org, heiko@sntech.de, stephen.boyd@linaro.org, frank.li@nxp.com, sre@kernel.org, gary.bisson@boundarydevices.com, festevam@gmail.com, stillcompiling@gmail.com, arnd@arndb.de, dbaryshkov@gmail.com, vaibhav.hiremath@linaro.org, krzk@kernel.org, mka@chromium.org, stern@rowland.harvard.edu, devicetree@vger.kernel.org, mail@maciej.szmigiero.name, pawel.moll@arm.com, linux-pm@vger.kernel.org, s.hauer@pengutronix.de, troy.kisky@boundarydevices.com, robh+dt@kernel.org, linux-arm-kernel@lists.infradead.org, hverkuil@xs4all.nl, oscar@naiandei.net, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, broonie@kernel.org, p.zabel@pengutronix.de, shawnguo@kernel.org, jun.li@nxp.com Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Friday, July 07, 2017 04:01:07 PM Peter Chen wrote: > On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote: > > > > > > - Can I write new code for it or I need to depend on something? > > > > There is nothing this code needs to depend on AFAICS, but there are existing > > solutions in this problem space (ACPI power management, genpd), so it needs to > > be careful enough about possible overlaps etc. > > > > > I find there is already "power state" concept at documentation. > > > Documentation/ABI/testing/sysfs-devices-power_state > > > > This is ACPI-specific and only in sysfs directories representing ACPI device > > objects (which aren't physical devices). > > > > Anyway, since ACPI covers the problem space you are working in already, > > your code has to be mutually exclusive with it. > > > > > - If I can write the new code for it, except the problems I want > > > to fix, are there any other use cases I need to consider? > > > > I would start simple and focus on the particular problem at hand, that is > > devices with two power states ("on" and "off") where the "on" state > > depends on a number of clocks and/or GPIOs. Still, I'd also avoid making > > design choices that might prevent it from being extended in the future > > if need be. > > > > One major problem I can see is how to "attach" the power states framework > > to a particular device (once we have discovered that it should be used with > > that device). > > > > For bus types that don't do power management of their own you could follow > > ACPI (and genpd) and provide a PM domain for this purpose, but bus types > > doing their own PM (like USB) will probably need to be treated differently. > > In those cases the bus type code will have to know that it should call some > > helpers to switch power states of devices. > > > > After thinking more, using a power state framework is seems too heavy > for this use case. This use case is just do some clock and gpio > operations before device is created, and do some put operations > after device is deleted. We just need some helpers in one structure > (called "power sequence" or "power state") for this purpose. > > For the use case, the clock and gpio operation can be done after device > is created, the power domain is more suitable. There is a problem with PM domains that they only provide hooks for runtime PM and system suspend/resume (including hibernation) and not for generic "power up" and "power down" operations that may need to be carried out at probe time before the runtime PM framework can be used (and analogously at remove time). I would consider starting with the patch below or similar. Then you can define something like POWER_STATE_SEQUENCE type for your case and basically use almost what you have already with it, except that struct pwrsec_generic will now become struct power_state_sequence and struct power_state_info will be embedded in it instead of struct pwrsec. The major comceptual difference is that ->power_up and ->power_down are now available at the level of the device that needs the power sequence and pm_device_power_up/down() can be used wherever necessary (in the code, in a bus type, in a controller driver or even in the driver for this particular device). Most likely you will need a PM domain in addition to that, but mostly to avoid code duplication. And in the future other types of power state definitions may be hooked up to that, including ACPI etc. Signed-off-by: Rafael J. Wysocki --- drivers/base/power/common.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/pm.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) Index: linux-pm/include/linux/pm.h =================================================================== --- linux-pm.orig/include/linux/pm.h +++ linux-pm/include/linux/pm.h @@ -550,6 +550,30 @@ struct pm_subsys_data { #endif }; +enum power_state_type { + POWER_STATE_GENERIC = 0, +}; + +/** + * struct power_state_info - information related to device power states. + * + * @type: Power states definition type. + * @power_up: Device power up method. + * @power_down: Device power down method. + * + * @power_up is expected to put the device into a power state in which it can + * be operated by software (it doesn't have to be the full power state in + * principle as long as the device will respond to all software accesses in + * this state) and @power_down is expected to put the device into the lowest + * power state the device can be put into given all of the applicable + * constraints and limitations (it may not mean completely off). + */ +struct power_state_info { + enum power_state_type type; + int (*power_up)(struct device *dev); + int (*power_down)(struct device *dev); +}; + struct dev_pm_info { pm_message_t power_state; unsigned int can_wakeup:1; @@ -600,6 +624,7 @@ struct dev_pm_info { unsigned long active_jiffies; unsigned long suspended_jiffies; unsigned long accounting_timestamp; + struct power_state_info *state; #endif struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */ void (*set_latency_tolerance)(struct device *, s32); @@ -610,6 +635,14 @@ extern void update_pm_runtime_accounting extern int dev_pm_get_subsys_data(struct device *dev); extern void dev_pm_put_subsys_data(struct device *dev); +#ifdef CONFIG_PM +extern int pm_device_power_up(struct device *dev); +extern int pm_device_power_down(struct device *dev); +#else +static inline int pm_device_power_up(struct device *dev) { return 0; } +static inline int pm_device_power_down(struct device *dev) { return 0; } +#endif /* CONFIG_PM */ + /** * struct dev_pm_domain - power management domain representation. * Index: linux-pm/drivers/base/power/common.c =================================================================== --- linux-pm.orig/drivers/base/power/common.c +++ linux-pm/drivers/base/power/common.c @@ -152,3 +152,38 @@ void dev_pm_domain_set(struct device *de device_pm_check_callbacks(dev); } EXPORT_SYMBOL_GPL(dev_pm_domain_set); + +/** + * pm_device_power_up - Power up a device using the power states framework. + * @dev: Device to power up. + * + * Put the device into a power state in which it will respond to all software + * accesses (that may not mean maximum power) using the callback provided + * through the device power state framework, if present. + */ +int pm_device_power_up(struct device *dev) +{ + if (dev->power.state && dev->power.state->power_up) + return dev->power.state->power_up(dev); + + return 0; +} +EXPORT_SYMBOL_GPL(pm_device_power_up); + +/** + * pm_device_power_down - Power down a device using the power states framework. + * @dev: Device to power down. + * + * Put the device into the lowest power state it can be put into given the + * applicable constraints and limitations (that may not mean maximum power) + * using the callback provided through the device power state framework, if + * present. + */ +int pm_device_power_down(struct device *dev) +{ + if (dev->power.state && dev->power.state->power_down) + return dev->power.state->power_down(dev); + + return 0; +} +EXPORT_SYMBOL_GPL(pm_device_power_down);