From: Tony Lindgren <tony@atomide.com> To: Linus Walleij <linus.walleij@linaro.org> Cc: Kevin Hilman <khilman@linaro.org>, Chris Ball <cjb@laptop.org>, Balaji T K <balajitk@ti.com>, Andreas Fenkart <afenkart@gmail.com>, "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>, Linux-OMAP <linux-omap@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Mark Brown <broonie@kernel.org>, Dmitry Torokhov <dmitry.torokhov@gmail.com> Subject: Re: [PATCH 3/4] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime Date: Wed, 12 Jun 2013 06:21:44 -0700 [thread overview] Message-ID: <20130612132144.GP8164@atomide.com> (raw) In-Reply-To: <CACRpkdaEjLzXfA3HX5mra0Mjheia4e2O3kbvikeZQRsNczS7kg@mail.gmail.com> * Linus Walleij <linus.walleij@linaro.org> [130611 01:00]: > On Mon, Jun 10, 2013 at 6:23 PM, Tony Lindgren <tony@atomide.com> wrote: > > > We only should remux the pins that need remuxing as that's done > > every time hitting idle. So I think we should have the following > > default groups: > > > > default Static pins that don't change, no need to remux > > configured in consumer driver probe like we already > > do > > > > active Optional dynamic pins remuxed for runtime, can be > > configured and selected in consumer driver probe. > > The consumer driver may also want to select this > > state in PM runtime resume. > > > > idle Optional dynamic pins remuxed for idle. The consumer > > driver may also want to select this state in PM > > runtime suspend depending on device_can_wakeup() > > and driver specific needs. > > The one thing I don't understand is why a driver would select the > active state in probe(), unless it's a driver that does not support > runtime PM. (But maybe that's what you mean.) Yes you're right, there should not be any need to select active state in probe, that should be selected by PM runtime. > Compare this to <linus/pinctrl/pinctrl-state.h>: > > /** > * @PINCTRL_STATE_DEFAULT: the state the pinctrl handle shall be put > * into as default, usually this means the pins are up and ready to > * be used by the device driver. This state is commonly used by > * hogs to configure muxing and pins at boot, and also as a state > * to go into when returning from sleep and idle in > * .pm_runtime_resume() or ordinary .resume() for example. > * @PINCTRL_STATE_IDLE: the state the pinctrl handle shall be put into > * when the pins are idle. This is a state where the system is relaxed > * but not fully sleeping - some power may be on but clocks gated for > * example. Could typically be set from a pm_runtime_suspend() or > * pm_runtime_idle() operation. > * @PINCTRL_STATE_SLEEP: the state the pinctrl handle shall be put into > * when the pins are sleeping. This is a state where the system is in > * its lowest sleep state. Could typically be set from an > * ordinary .suspend() function. > */ > #define PINCTRL_STATE_DEFAULT "default" > #define PINCTRL_STATE_IDLE "idle" > #define PINCTRL_STATE_SLEEP "sleep" > > The way I currently use these in e.g. > drivers/spi/spi-pl022 is: > > probe: > -> default > > runtime_suspend: > -> idle > > runtime_resume: > -> default > > suspend: > -> sleep > > resume: > -> default > -> idle > > Notice that we go to default then idle on probe and > runtime resume. This is because the idle state is > optional (as is the sleep state). > > So I guess if we should extend this terminology to match > what you are using for the OMAP it would rather be like > this: > > probe: > -> default > > runtime_suspend: > -> idle > > runtime_resume: > -> default > -> active At least for omaps, there's no need to select default in runtime_resume as the default pins stay that way. > suspend: > -> sleep For omaps, we would just select idle pins again in the suspend case. > resume: > -> default > -> idle And for omaps, there's no need to select default in resume either. Just selecting active would do the trick for resume. So for omaps, the sequence would be: probe: -> default (typically all device pins except rx pin) runtime_suspend: suspend: -> idle (remux rx pin from device to gpio input for wake) runtime_resume: resume: -> active (remux rx pin from gpio input to device) > Just one more optional "active" state in runtime resume. > Correct? Yes the "active" is needed, but "sleep" would be unused for omaps. > If we can agree on this I will add the active state to the > state table and add a container in the core for this as well > as pinctrl_pm_select_active_state() so we can skip all the > pointless boilerplate also in the OMAP drivers, plus increase > the readability and portability quite a bit. Sounds good to me as long as we don't always need to select the default pins over and over in PM runtime_resume. > >> However in this case I *suspect* that what you really want > >> to do it to rename the state called "default" to "sleep" > >> (it appears the default state is sleepy) and then rename > >> the "active" state to "default" (as this is the defined semantic > >> meaning of "default" from <linux/pinctrl/pinctrl-state.h>. > > > > The idle state above could also be called sleep instead of idle > > if you prefer that. > > No I think we should reserve that name for the pin state > associated with suspend(). Let's leave it like this. OK > > I think the confusion is caused by the fact that we need three > > mux groups, not just two :) The toggling between active and idle > > is the hotpath as that can potentially happen for multiple drivers > > every time we enter and exit idle. > > Actually we have the same thing, it's just that our "default" > and "active" are the same thing. But it seems we need to > add your granularity to this. Well the difference seems to be that you need to remux all the device pins for runtime_suspend and resume while in most of the cases I know of only one device pins needs to be toggled and the rest can be selected in driver probe. Regards, Tony
WARNING: multiple messages have this Message-ID (diff)
From: tony@atomide.com (Tony Lindgren) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 3/4] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime Date: Wed, 12 Jun 2013 06:21:44 -0700 [thread overview] Message-ID: <20130612132144.GP8164@atomide.com> (raw) In-Reply-To: <CACRpkdaEjLzXfA3HX5mra0Mjheia4e2O3kbvikeZQRsNczS7kg@mail.gmail.com> * Linus Walleij <linus.walleij@linaro.org> [130611 01:00]: > On Mon, Jun 10, 2013 at 6:23 PM, Tony Lindgren <tony@atomide.com> wrote: > > > We only should remux the pins that need remuxing as that's done > > every time hitting idle. So I think we should have the following > > default groups: > > > > default Static pins that don't change, no need to remux > > configured in consumer driver probe like we already > > do > > > > active Optional dynamic pins remuxed for runtime, can be > > configured and selected in consumer driver probe. > > The consumer driver may also want to select this > > state in PM runtime resume. > > > > idle Optional dynamic pins remuxed for idle. The consumer > > driver may also want to select this state in PM > > runtime suspend depending on device_can_wakeup() > > and driver specific needs. > > The one thing I don't understand is why a driver would select the > active state in probe(), unless it's a driver that does not support > runtime PM. (But maybe that's what you mean.) Yes you're right, there should not be any need to select active state in probe, that should be selected by PM runtime. > Compare this to <linus/pinctrl/pinctrl-state.h>: > > /** > * @PINCTRL_STATE_DEFAULT: the state the pinctrl handle shall be put > * into as default, usually this means the pins are up and ready to > * be used by the device driver. This state is commonly used by > * hogs to configure muxing and pins at boot, and also as a state > * to go into when returning from sleep and idle in > * .pm_runtime_resume() or ordinary .resume() for example. > * @PINCTRL_STATE_IDLE: the state the pinctrl handle shall be put into > * when the pins are idle. This is a state where the system is relaxed > * but not fully sleeping - some power may be on but clocks gated for > * example. Could typically be set from a pm_runtime_suspend() or > * pm_runtime_idle() operation. > * @PINCTRL_STATE_SLEEP: the state the pinctrl handle shall be put into > * when the pins are sleeping. This is a state where the system is in > * its lowest sleep state. Could typically be set from an > * ordinary .suspend() function. > */ > #define PINCTRL_STATE_DEFAULT "default" > #define PINCTRL_STATE_IDLE "idle" > #define PINCTRL_STATE_SLEEP "sleep" > > The way I currently use these in e.g. > drivers/spi/spi-pl022 is: > > probe: > -> default > > runtime_suspend: > -> idle > > runtime_resume: > -> default > > suspend: > -> sleep > > resume: > -> default > -> idle > > Notice that we go to default then idle on probe and > runtime resume. This is because the idle state is > optional (as is the sleep state). > > So I guess if we should extend this terminology to match > what you are using for the OMAP it would rather be like > this: > > probe: > -> default > > runtime_suspend: > -> idle > > runtime_resume: > -> default > -> active At least for omaps, there's no need to select default in runtime_resume as the default pins stay that way. > suspend: > -> sleep For omaps, we would just select idle pins again in the suspend case. > resume: > -> default > -> idle And for omaps, there's no need to select default in resume either. Just selecting active would do the trick for resume. So for omaps, the sequence would be: probe: -> default (typically all device pins except rx pin) runtime_suspend: suspend: -> idle (remux rx pin from device to gpio input for wake) runtime_resume: resume: -> active (remux rx pin from gpio input to device) > Just one more optional "active" state in runtime resume. > Correct? Yes the "active" is needed, but "sleep" would be unused for omaps. > If we can agree on this I will add the active state to the > state table and add a container in the core for this as well > as pinctrl_pm_select_active_state() so we can skip all the > pointless boilerplate also in the OMAP drivers, plus increase > the readability and portability quite a bit. Sounds good to me as long as we don't always need to select the default pins over and over in PM runtime_resume. > >> However in this case I *suspect* that what you really want > >> to do it to rename the state called "default" to "sleep" > >> (it appears the default state is sleepy) and then rename > >> the "active" state to "default" (as this is the defined semantic > >> meaning of "default" from <linux/pinctrl/pinctrl-state.h>. > > > > The idle state above could also be called sleep instead of idle > > if you prefer that. > > No I think we should reserve that name for the pin state > associated with suspend(). Let's leave it like this. OK > > I think the confusion is caused by the fact that we need three > > mux groups, not just two :) The toggling between active and idle > > is the hotpath as that can potentially happen for multiple drivers > > every time we enter and exit idle. > > Actually we have the same thing, it's just that our "default" > and "active" are the same thing. But it seems we need to > add your granularity to this. Well the difference seems to be that you need to remux all the device pins for runtime_suspend and resume while in most of the cases I know of only one device pins needs to be toggled and the rest can be selected in driver probe. Regards, Tony
next prev parent reply other threads:[~2013-06-12 13:21 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-06-07 21:49 [PATCH 0/4] Updated omap_hsmmc SDIO and remuxing patches Tony Lindgren 2013-06-07 21:49 ` Tony Lindgren 2013-06-07 21:49 ` [PATCH 1/4] mmc: omap_hsmmc: Fix context save and restore for DT Tony Lindgren 2013-06-07 21:49 ` Tony Lindgren 2013-06-08 4:25 ` Felipe Balbi 2013-06-08 4:25 ` Felipe Balbi 2013-06-08 15:02 ` Tony Lindgren 2013-06-08 15:02 ` Tony Lindgren 2013-06-07 21:49 ` [PATCH 2/4] mmc: omap_hsmmc: Enable SDIO IRQ using a GPIO in idle mode Tony Lindgren 2013-06-07 21:49 ` Tony Lindgren 2013-06-14 7:37 ` Tony Lindgren 2013-06-14 7:37 ` Tony Lindgren 2013-06-14 11:50 ` Ulf Hansson 2013-06-14 11:50 ` Ulf Hansson 2013-06-20 7:24 ` Tony Lindgren 2013-06-20 7:24 ` Tony Lindgren 2013-07-08 9:02 ` Felipe Balbi 2013-07-08 9:02 ` Felipe Balbi 2013-06-07 21:49 ` [PATCH 3/4] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime Tony Lindgren 2013-06-07 21:49 ` Tony Lindgren 2013-06-10 16:03 ` Linus Walleij 2013-06-10 16:03 ` Linus Walleij 2013-06-10 16:23 ` Tony Lindgren 2013-06-10 16:23 ` Tony Lindgren 2013-06-11 7:54 ` Linus Walleij 2013-06-11 7:54 ` Linus Walleij 2013-06-12 13:21 ` Tony Lindgren [this message] 2013-06-12 13:21 ` Tony Lindgren 2013-06-14 7:40 ` Tony Lindgren 2013-06-14 7:40 ` Tony Lindgren 2013-06-07 21:50 ` [PATCH 4/4] mmc: omap_hsmmc: debugfs entries for GPIO and SDIO mode Tony Lindgren 2013-06-07 21:50 ` Tony Lindgren
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20130612132144.GP8164@atomide.com \ --to=tony@atomide.com \ --cc=afenkart@gmail.com \ --cc=balajitk@ti.com \ --cc=broonie@kernel.org \ --cc=cjb@laptop.org \ --cc=dmitry.torokhov@gmail.com \ --cc=khilman@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-mmc@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.