| Message ID | 1371826990-25820-1-git-send-email-grygorii.strashko@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 21, 2013 at 5:03 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > Hence, add pinctrl handling in omap_device core: > 1) on PM runtime resume > - switch pinctrl state to "default" (todo: "active") > 2) on PM runtime suspend > - switch pinctrl state to "idle" > 3) during system wide suspend > - switch pinctrl state to "sleep" or "idle" if omap_device core disables device > - switch pinctrl state to "sleep" if device is disabled already > 4) during system wide resume > - switch pinctrl state to "default" (todo: "active") if omap_device core has > disabled device during suspend > - switch pinctrl state to "idle" if device was already disabled before suspend I don't understand step 4. I get the creeps about whether the system is runtime suspended or runtime resumed when we come to resume proper, so I need Kevin to have a look at this. Apart from that it looks good. Stephen and Tony are trying to figure out the details of whether "active" is necessary or not in a related thread I think. Yours, Linus Walleij
Hi Grygorii, Grygorii Strashko <grygorii.strashko@ti.com> writes: > Before switching to DT pinctrl states of OMAP IPs have been handled by hwmod > framework. After switching to DT-boot the pinctrl handling was dropped from > hwmod framework and, as it was recommended, OMAP IP's drivers have to be updated > to handle pinctrl states by itself using pinctrl_pm_select_xx() helpers > (see http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173514.html) > > But this is not right for OMAP2+ SoC where real IPs state is controlled > by omap_device core which enables/disables modules & clocks actually. > > For example, if OMAP I2C driver will handle pinctrl state during system wide > suspend the following issue may occure: > - suspend_noirq - I2C device can be still active because of PM auto-suspend > |-_od_suspend_noirq > |- omap_i2c_suspend_noirq > |- PINs state set to SLEEP > |- pm_generic_runtime_suspend > |- omap_i2c_runtime_suspend() > |- PINs state set to IDLE <--- *oops* PINs state is IDLE and not SLEEP > |- omap_device_idle() > |- omap_hwmod_idle() > |- _idle() > |- disbale module (sysc&clocks) > > - resume_noirq - I2C was active before suspend > |-_od_resume_noirq > |- omap_hwmod_enable() > |- _enable() > |- enable module (sysc&clocks) > |- pm_generic_runtime_resume > |- omap_i2c_runtime_resume() > |- PINs state set to DEFAULT <--- !!!! > |- omap_i2c_resume_noirq > |- PINs state set to DEFAULT > |- PINs state set to IDLE <--- *big oops* we have active module and its > PINs state is IDLE > (see https://patchwork.kernel.org/patch/2642101/) > > Of course, everything can be handled by adding a tons of code in ecah driver to > check PM state of device and override default behavior of omap_device core, but > this not good. > > Hence, add pinctrl handling in omap_device core: > 1) on PM runtime resume > - switch pinctrl state to "default" (todo: "active") > 2) on PM runtime suspend > - switch pinctrl state to "idle" > 3) during system wide suspend > - switch pinctrl state to "sleep" or "idle" if omap_device core disables device > - switch pinctrl state to "sleep" if device is disabled already > 4) during system wide resume > - switch pinctrl state to "default" (todo: "active") if omap_device core has > disabled device during suspend > - switch pinctrl state to "idle" if device was already disabled before suspend > > This will enable pinctrl for all OMAP2+ IP's drivers by default - > no changes in code is needed and only DT data will need to be updated > (add "default", "active", "idle", "sleep" states). > > This will enable pinctrl handling for all OMAP2+ drivers by default - > no changes in code is needed and only DT data will need to be updated > (add "default", "active", "idle", "sleep" states). > > Related discussions: > - [3/3] i2c: nomadik: use pinctrl PM helpers > https://patchwork.kernel.org/patch/2670291/ > - mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime > https://patchwork.kernel.org/patch/2690191/ > - [PATCH 00/11] drivers: Add Pinctrl PM support > https://lkml.org/lkml/2013/5/31/210 > > CC: Hebbar Gururaja <gururaja.hebbar@ti.com> > CC: Linus Walleij <linus.walleij@linaro.org> > CC: linux-arm-kernel@lists.infradead.org > CC: linux-omap@vger.kernel.org > CC: linux-kernel@vger.kernel.org > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > Hi Kevin, Tony, > > I've verified this patch on OMAP4 SDP board: > - PM runtime for I2C4, UART2, UART3 > - suspend/resume with I2C4, UART2, UART3 > > seems it works and pinctrl states have been updated as expected. Thanks for working on this. I agree with the approach, and much prefer this to boiler plate code throughout the drivers. I suggest we wait until the dust settles on the active/default thread before taking this further, but have no objections to the approach. Kevin
* Linus Walleij <linus.walleij@linaro.org> [130624 05:13]: > On Fri, Jun 21, 2013 at 5:03 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > > > Hence, add pinctrl handling in omap_device core: > > 1) on PM runtime resume > > - switch pinctrl state to "default" (todo: "active") > > 2) on PM runtime suspend > > - switch pinctrl state to "idle" > > 3) during system wide suspend > > - switch pinctrl state to "sleep" or "idle" if omap_device core disables device > > - switch pinctrl state to "sleep" if device is disabled already Do you need a separate setting for "idle" and "sleep", or are they the same? > > 4) during system wide resume > > - switch pinctrl state to "default" (todo: "active") if omap_device core has > > disabled device during suspend > > - switch pinctrl state to "idle" if device was already disabled before suspend > > I don't understand step 4. > > I get the creeps about whether the system is runtime suspended > or runtime resumed when we come to resume proper, so I need > Kevin to have a look at this. > > Apart from that it looks good. > > Stephen and Tony are trying to figure out the details of whether "active" > is necessary or not in a related thread I think. Yes we should have that sorted out over next few weeks, so let's just wait a little while on all these dynamic remuxing patches to avoid churn. Regards, Tony
Hi All, On 06/25/2013 09:58 AM, Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [130624 05:13]: >> On Fri, Jun 21, 2013 at 5:03 PM, Grygorii Strashko >> <grygorii.strashko@ti.com> wrote: >> >>> Hence, add pinctrl handling in omap_device core: >>> 1) on PM runtime resume >>> - switch pinctrl state to "default" (todo: "active") >>> 2) on PM runtime suspend >>> - switch pinctrl state to "idle" >>> 3) during system wide suspend >>> - switch pinctrl state to "sleep" or "idle" if omap_device core disables device >>> - switch pinctrl state to "sleep" if device is disabled already > Do you need a separate setting for "idle" and "sleep", or are > they the same? > >>> 4) during system wide resume >>> - switch pinctrl state to "default" (todo: "active") if omap_device core has >>> disabled device during suspend >>> - switch pinctrl state to "idle" if device was already disabled before suspend >> I don't understand step 4. >> >> I get the creeps about whether the system is runtime suspended >> or runtime resumed when we come to resume proper, so I need >> Kevin to have a look at this. >> >> Apart from that it looks good. >> >> Stephen and Tony are trying to figure out the details of whether "active" >> is necessary or not in a related thread I think. Thanks for your comments. I've prepared diagram to illustrate how does this work: + | | .probe() | +-----v--------+ | | | default | | | +----+--+------+ | | pm_runtime_get() | | pm_runtime_put() +------------------+ +------------------+ | | +------v--------+ pm_runtime_put() +-------v-------+ | +-----------------------> | +-----------> Active | pm_runtime_get() | Idle <--------------+ | | <-----------------------+ | | | +-------+-------+ +-------+-------+ | | |.suspend_noirq() |.suspend_noirq() | | | | | | | | | | | | | | | | | | +-------v-------+ +-------v--------+ | | | | | | | +-----------+ Sleep ------+ | Sleep +-------------+ .resume_noirq() | | | | |.resume_noirq() +-------|-------+ | +----------------+ | Idle | +-------------+ The "Sleep" pinctrl state is optional - if "sleep" state isn't defined then "Idle" pinctrl state will be used during suspend. As per above pinctrl state transition diagram - I think, that: - if "idle" or "sleep" states are defined then "active" state have to be defined too. So, pinctrl core should detect such situation and generate a warning. From my point of view, we should have "active" state - it would allow to avoid additional runtime overhead and handle properly the case when drivers are used as loadable modules. In this case, after boot, device's pinctrl registers can have HW default values (after reset) or can be configured to some safe (off) state from board file (board's "default" configuration). Driver module will activate its PINs when loaded and need to restore safe (off) PINs state when unloaded. As result, it seems, we should have "off" state which can be used when driver's module is removed. So, final list of default pnctrl states may be defined as "default", "active", "idle", "sleep", "off": - "active", "idle", "sleep": will be handled by omap_device core - "default", "off": will be handled by driver itself (or Device core). > Yes we should have that sorted out over next few weeks, so let's > just wait a little while on all these dynamic remuxing patches > to avoid churn. > > Regards, > > Tony Regards, - grygorii
Picture format fixed. Sorry for that. On 06/26/2013 04:20 PM, Grygorii Strashko wrote: > Hi All, > > On 06/25/2013 09:58 AM, Tony Lindgren wrote: >> * Linus Walleij <linus.walleij@linaro.org> [130624 05:13]: >>> On Fri, Jun 21, 2013 at 5:03 PM, Grygorii Strashko >>> <grygorii.strashko@ti.com> wrote: >>> >>>> Hence, add pinctrl handling in omap_device core: >>>> 1) on PM runtime resume >>>> - switch pinctrl state to "default" (todo: "active") >>>> 2) on PM runtime suspend >>>> - switch pinctrl state to "idle" >>>> 3) during system wide suspend >>>> - switch pinctrl state to "sleep" or "idle" if omap_device core >>>> disables device >>>> - switch pinctrl state to "sleep" if device is disabled already >> Do you need a separate setting for "idle" and "sleep", or are >> they the same? >> >>>> 4) during system wide resume >>>> - switch pinctrl state to "default" (todo: "active") if omap_device >>>> core has >>>> disabled device during suspend >>>> - switch pinctrl state to "idle" if device was already disabled >>>> before suspend >>> I don't understand step 4. >>> >>> I get the creeps about whether the system is runtime suspended >>> or runtime resumed when we come to resume proper, so I need >>> Kevin to have a look at this. >>> >>> Apart from that it looks good. >>> >>> Stephen and Tony are trying to figure out the details of whether >>> "active" >>> is necessary or not in a related thread I think. > Thanks for your comments. > > I've prepared diagram to illustrate how does this work: > + > | > | .probe() > | > +-----v--------+ > | | > | default | > | | > +----+--+------+ > | | > pm_runtime_get()| | pm_runtime_put() > +----------------+ +------------+ > | | > +------v------+ pm_runtime_put()+-------v-----+ > | +-----------------> | > +-----------> Active |pm_runtime_get() | Idle <----------+ > | | <-----------------+ | | > | +-------+-----+ +-------+-----+ | > | |.suspend_noirq() |.suspend_noirq()| > | | | | > | | | | > | | | | > | | | | > | +-------v-----+ +-------v-----+ | > | | | | | | > +-----------+ Sleep ------+ | Sleep +----------+ > .resume_noirq() | | | | |.resume_noirq() > +-------|-----+ | +-------------+ > | Idle | > +-----------+ > > The "Sleep" pinctrl state is optional - if "sleep" state isn't defined > then "Idle" pinctrl state will be used during suspend. > > As per above pinctrl state transition diagram - I think, that: > - if "idle" or "sleep" states are defined then "active" state have to be > defined too. > So, pinctrl core should detect such situation and generate a warning. > > From my point of view, we should have "active" state - it would allow > to avoid additional > runtime overhead and handle properly the case when drivers are used as > loadable modules. > In this case, after boot, device's pinctrl registers can have HW default > values (after reset) or > can be configured to some safe (off) state from board file (board's > "default" configuration). > Driver module will activate its PINs when loaded and need to restore > safe (off) PINs state when > unloaded. > As result, it seems, we should have "off" state which can be used when > driver's module is removed. > > So, final list of default pnctrl states may be defined as "default", > "active", "idle", "sleep", "off": > - "active", "idle", "sleep": will be handled by omap_device core > - "default", "off": will be handled by driver itself (or Device core). > > > >> Yes we should have that sorted out over next few weeks, so let's >> just wait a little while on all these dynamic remuxing patches >> to avoid churn. >> >> Regards, >> >> Tony > > Regards, > - grygorii > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 26, 2013 at 3:20 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > The "Sleep" pinctrl state is optional - if "sleep" state isn't defined > then "Idle" pinctrl state will be used during suspend. Why? If we have a clear cut semantic that "idle" is for runtime suspend, why should it be a fallback for suspend? You do realize that can just be turned around (as common suspend is more widely implemented than runtime suspend) so that we could say that if "idle" does not exist, we go to "sleep" in runtime suspend. > So, final list of default pnctrl states may be defined as "default", > "active", "idle", "sleep", "off": > - "active", "idle", "sleep": will be handled by omap_device core > - "default", "off": will be handled by driver itself (or Device core). Currently the pinctrl system combines what is called "default" and "active" into one, assuming that all devices shall come up in the active state. Also we haven't seen a device that need some "off" state that is different from "sleep". If you want to drive this state list home you have to give a *real world example*. I want to see a *real* example, for a device and it's pins, that define totally different things for these states, as a rationale. Else we are just defining states to make nice figures or mental maps and that is not helpful for drivers writers. Yours, Linus Walleij
* Linus Walleij <linus.walleij@linaro.org> [130626 12:37]: > On Wed, Jun 26, 2013 at 3:20 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > > > The "Sleep" pinctrl state is optional - if "sleep" state isn't defined > > then "Idle" pinctrl state will be used during suspend. > > Why? If we have a clear cut semantic that "idle" is for runtime > suspend, why should it be a fallback for suspend? > > You do realize that can just be turned around (as common suspend > is more widely implemented than runtime suspend) so that we > could say that if "idle" does not exist, we go to "sleep" in > runtime suspend. > > > So, final list of default pnctrl states may be defined as "default", > > "active", "idle", "sleep", "off": > > - "active", "idle", "sleep": will be handled by omap_device core > > - "default", "off": will be handled by driver itself (or Device core). > > Currently the pinctrl system combines what is called "default" > and "active" into one, assuming that all devices shall come up > in the active state. > > Also we haven't seen a device that need some "off" state that > is different from "sleep". Right, this is what I've been wondering too. Do we really need "idle", "sleep", "off"? Or is "idle" enough? Or "idle" and "sleep"? Only one of the should be set at a time, but it would be nice to handle most cases in a generic way in drivers/base/pinctrl.c. > If you want to drive this state list home you have to give a > *real world example*. > > I want to see a *real* example, for a device and it's pins, > that define totally different things for these states, as a > rationale. Yeah me too :) > Else we are just defining states to make nice figures or mental > maps and that is not helpful for drivers writers. Regards, Tony
Hi Linus, On 06/26/2013 10:31 PM, Linus Walleij wrote: > On Wed, Jun 26, 2013 at 3:20 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > >> The "Sleep" pinctrl state is optional - if "sleep" state isn't defined >> then "Idle" pinctrl state will be used during suspend. > > Why? If we have a clear cut semantic that "idle" is for runtime > suspend, why should it be a fallback for suspend? Seems, some misunderstanding is here :) This is OMAP specific - for OMAP the "Idle" state is typically used, and "sleep" state is rather optional. Historically, three states have been defined for OMAP: - "enabled" - "idle" - "off" Most of OMAP drivers use Runtime PM only to manage their states - but, any calls to pm_runtime_xxx() helpers are redirected to *OMAP device* framework which physically configures power state/clocks/pins of device. It's the usual situation, when the device is in "Active/ON" state when system is going to suspend, because PM runtime *do not allow* to disable device during system wide suspend. To workaround this, the *OMAP device* framework disables all active devices forcibly at .suspend_noirq() stage - and, at this moment, it should apply "Idle" or "Sleep" configuration on PINs. I need to note here that "Sleep" state is/will be *not* used during PM runtime operation - it's very specific. "off" state is used very rare now - and for the cases when device driver is removed or device need to completely switched off (HSI omap_hsi.c and RPROC remoteproc.c). I think, In the future the OMAP pinctrl configurations would be manged in more flexible way then now (thanks to "pinctrl PM helpers" and you;)) - "Idle" state will be splitted to "Idle"/"sleep" - "default" state will be splitted to "default"/"active" > > You do realize that can just be turned around (as common suspend > is more widely implemented than runtime suspend) so that we > could say that if "idle" does not exist, we go to "sleep" in > runtime suspend. > >> So, final list of default pnctrl states may be defined as "default", >> "active", "idle", "sleep", "off": >> - "active", "idle", "sleep": will be handled by omap_device core >> - "default", "off": will be handled by driver itself (or Device core). > > Currently the pinctrl system combines what is called "default" > and "active" into one, assuming that all devices shall come up > in the active state. > > Also we haven't seen a device that need some "off" state that > is different from "sleep". > > If you want to drive this state list home you have to give a > *real world example*. > > I want to see a *real* example, for a device and it's pins, > that define totally different things for these states, as a > rationale. > > Else we are just defining states to make nice figures or mental > maps and that is not helpful for drivers writers. OK. Please pay attention to the following example (taken from TI Android product kernel 3.4 - need to have the same in mainline kernel): static const struct omap_device_pad port1_phy_pads[] __initconst = { { .name = "usbb1_ulpitll_stp.usbb1_ulpiphy_stp", .flags = OMAP_DEVICE_PAD_REMUX, .enable = OMAP_PIN_OUTPUT | OMAP_MUX_MODE4, .idle = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE7, .off = OMAP_PIN_OFF_OUTPUT_LOW | OMAP_MUX_MODE7, }, { .name = "usbb1_ulpitll_clk.usbb1_ulpiphy_clk", .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, { .name = "usbb1_ulpitll_dir.usbb1_ulpiphy_dir", .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, { .name = "usbb1_ulpitll_nxt.usbb1_ulpiphy_nxt", .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, { .name = "usbb1_ulpitll_dat0.usbb1_ulpiphy_dat0", .flags = OMAP_DEVICE_PAD_REMUX |OMAP_DEVICE_PAD_WAKEUP, .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, .idle = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, { .name = "usbb1_ulpitll_dat1.usbb1_ulpiphy_dat1", .flags = OMAP_DEVICE_PAD_REMUX |OMAP_DEVICE_PAD_WAKEUP, .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, .idle = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, { .name = "usbb1_ulpitll_dat2.usbb1_ulpiphy_dat2", .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, { .name = "usbb1_ulpitll_dat3.usbb1_ulpiphy_dat3", .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, { .name = "usbb1_ulpitll_dat4.usbb1_ulpiphy_dat4", .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, { .name = "usbb1_ulpitll_dat5.usbb1_ulpiphy_dat5", .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, { .name = "usbb1_ulpitll_dat6.usbb1_ulpiphy_dat6", .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, { .name = "usbb1_ulpitll_dat7.usbb1_ulpiphy_dat7", .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, }, }; As you can see, from 12 pins only 3 pins need to be reconfigured while switching from "active"->"idle" states and back (and as I mentioned above for OMAP "idle" == "sleep" now). Regarding "OFF" state: OMAP mux HW defines special state for unused pins which is selected by default after reset and need to be selected when device isn't used, for example: _MUXMODE - Functional multiplexing selection for pad 0x0: Select usbb1_hsic_data 0x3: Select gpio_96 0x7: Select safe_mode <<--- pin unused In opposite, in "sleep" state some pins need to be reconfigured in special way to avoid glitches/support wake up/follow HW requirements: { .name = "usbb1_ulpitll_dat0.usbb1_ulpiphy_dat0", .flags = OMAP_DEVICE_PAD_REMUX |OMAP_DEVICE_PAD_WAKEUP, .enable = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, <<- "active" PIN is input+pulldown .idle = OMAP_PIN_INPUT_PULLDOWN | OMAP_MUX_MODE4, <<- "idle/sleep" PIN is input+pulldown+wakeup_en }, - or - { .name = "uart2_rts.uart2_rts", .flags = OMAP_DEVICE_PAD_REMUX, .enable = OMAP_PIN_OUTPUT | OMAP_MUX_MODE0, <<- "active" PIN is output .idle = OMAP_PIN_OFF_INPUT_PULLUP | OMAP_MUX_MODE7, <<- "idle/sleep" PIN is input+pullup+safe }, More over, OAMP mux HW allow to define special state for PIN which will be applied in suspend/OFF mode (OFF-mode deepest possible PM state for OMAP) automatically. X_OFFMODEPULLTYPESELECT OffMode mode pullup/down selection for pad X_OFFMODEPULLUDENABLE OffMode mode pullup/down enable for pad X_OFFMODEOUTVALUE OffMode mode output value for pad X_OFFMODEOUTENABLE OffMode mode output enable value for pad X_OFFMODEENABLE OffMode mode override control for pad > > Yours, > Linus Walleij > Regards, -grygorii
* Grygorii Strashko <grygorii.strashko@ti.com> [130627 07:12]: > > As you can see, from 12 pins only 3 pins need to be reconfigured > while switching from "active"->"idle" states and back (and as I > mentioned above for OMAP "idle" == "sleep" now). > > Regarding "OFF" state: > OMAP mux HW defines special state for unused pins which is selected > by default after reset and need to be selected when device isn't > used, for example: > _MUXMODE - Functional multiplexing selection for pad > 0x0: Select usbb1_hsic_data > 0x3: Select gpio_96 > 0x7: Select safe_mode <<--- pin unused The off mode bits can be enabled continuously, the mux hardware automatically sets them. So sounds like you don't need any separate "idle" "sleep" and "off" states, the following should do: "default" (or "static") static pins that don't need to be touched after consumer driver probe "active" dynamic pins that are not a subset of "default" needed for runtime; these pins are the same as "idle" below, but with different muxing or pinconf device runtime "idle" dynamic pins that are not a subset of "default" needed for various idle modes; these pins are the same as "active" above, but with different muxing or pinconf for various idle states Can you please confirm that these named modes are enough for your needs? If your hardware does not have specific off mode bits, then I can understand that you may need one mor state "off". Regards, Tony
On 06/27/2013 05:45 PM, Tony Lindgren wrote: > * Grygorii Strashko <grygorii.strashko@ti.com> [130627 07:12]: >> >> As you can see, from 12 pins only 3 pins need to be reconfigured >> while switching from "active"->"idle" states and back (and as I >> mentioned above for OMAP "idle" == "sleep" now). >> >> Regarding "OFF" state: >> OMAP mux HW defines special state for unused pins which is selected >> by default after reset and need to be selected when device isn't >> used, for example: >> _MUXMODE - Functional multiplexing selection for pad >> 0x0: Select usbb1_hsic_data >> 0x3: Select gpio_96 >> 0x7: Select safe_mode <<--- pin unused > > The off mode bits can be enabled continuously, the mux hardware > automatically sets them. So sounds like you don't need any > separate "idle" "sleep" and "off" states, the following should > do: > > "default" (or "static") static pins that don't need to be touched > after consumer driver probe > > "active" dynamic pins that are not a subset of > "default" needed for runtime; these pins > are the same as "idle" below, but with > different muxing or pinconf device > runtime > > "idle" dynamic pins that are not a subset of > "default" needed for various idle modes; > these pins are the same as "active" above, > but with different muxing or pinconf for > various idle states > > Can you please confirm that these named modes are enough for > your needs? Confirm ) > > If your hardware does not have specific off mode bits, then > I can understand that you may need one mor state "off". > > Regards, > > Tony > Regards, -grygorii
On Thu, Jun 27, 2013 at 4:01 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > I think, In the future the OMAP pinctrl configurations would be manged in > more flexible way then now (thanks to "pinctrl PM helpers" and you;)) > - "Idle" state will be splitted to "Idle"/"sleep" > - "default" state will be splitted to "default"/"active" OK so the first ones we already have so the discussion is now down to adding the "active" state (and pinctrl_pm* helper function). I guess we need a patch set prepared which adds the "active" state and helper function as the first patch, i.e. this: http://marc.info/?l=linux-kernel&m=137094012703340&w=2 Can I have your ACK on this patch? I do not want to add the state unless there is a clear consumer, so it needs to go in with the first patch to a device driver that uses pinctrl_pm_select_active_state() which will be a good demonstration on its use and utility. (And a point to object and suggest other ways to do the same thing...) Yours, Linus Walleij
On Thu, Jun 27, 2013 at 4:45 PM, Tony Lindgren <tony@atomide.com> wrote: > The off mode bits can be enabled continuously, the mux hardware > automatically sets them. So sounds like you don't need any > separate "idle" "sleep" and "off" states, the following should > do: > > "default" (or "static") static pins that don't need to be touched > after consumer driver probe Please use "default" for this. We cannot shoehorn OMAP lingo into the core kernel and expect everyone to accept that... > "active" dynamic pins that are not a subset of > "default" needed for runtime; these pins > are the same as "idle" below, but with > different muxing or pinconf device > runtime > > "idle" dynamic pins that are not a subset of > "default" needed for various idle modes; > these pins are the same as "active" above, > but with different muxing or pinconf for > various idle states The misunderstanding earlier was that it was assumed that "active" == "default", just with another name. If the above is true with an actual driver patch calling *both* pinctrl_pm_select_default_state() *and* pinctrl_pm_select_active_state() in different runpaths I see no reason to reject it. But if it turns out that the drivers always use either "default" or "active" and never both I consider it a pure naming convention and will not accept the "active" state. Now it's time for patches :-) Yours, Linus Walleij
* Linus Walleij <linus.walleij@linaro.org> [130710 13:42]: > On Thu, Jun 27, 2013 at 4:01 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > > > I think, In the future the OMAP pinctrl configurations would be manged in > > more flexible way then now (thanks to "pinctrl PM helpers" and you;)) > > - "Idle" state will be splitted to "Idle"/"sleep" > > - "default" state will be splitted to "default"/"active" > > OK so the first ones we already have so the discussion is now down > to adding the "active" state (and pinctrl_pm* helper function). I think I have a patchset ready for pinctrl to allow multiple simulaneous states as discussed, need to test it first though. Should be able to post it on Friday hopefully. > I guess we need a patch set prepared which adds the "active" state > and helper function as the first patch, i.e. this: > http://marc.info/?l=linux-kernel&m=137094012703340&w=2 > Can I have your ACK on this patch? Hmm I have gone a bit further with the drivers/base/pinctrl.c in my set where if active state is defined then sleep and idle states must match active state for the pingroups to avoid constantly checking those sets during runtime. Then the pinctrl_pm_select_active_state() does not actually have anything to do with PM in the rx/tx case, so we should rename that. It's pretty close, but before we can apply that we need the changes I have to allow multiple simultaneous states. I suggest wait just few days on that patch. > I do not want to add the state unless there is a clear consumer, > so it needs to go in with the first patch to a device driver that uses > pinctrl_pm_select_active_state() which will be a good demonstration > on its use and utility. (And a point to object and suggest other ways > to do the same thing...) Right, we have quite a few consumers with omaps for that as am33xx requires remuxing wake-up events for all drivers AFAIK. The MMC SDIO pin remuxing is probably closest one ready. Regards, Tony
Hi Linus, On 07/10/2013 11:36 PM, Linus Walleij wrote: > On Thu, Jun 27, 2013 at 4:01 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > >> I think, In the future the OMAP pinctrl configurations would be manged in >> more flexible way then now (thanks to "pinctrl PM helpers" and you;)) >> - "Idle" state will be splitted to "Idle"/"sleep" >> - "default" state will be splitted to "default"/"active" > > OK so the first ones we already have so the discussion is now down > to adding the "active" state (and pinctrl_pm* helper function). > > I guess we need a patch set prepared which adds the "active" state > and helper function as the first patch, i.e. this: > http://marc.info/?l=linux-kernel&m=137094012703340&w=2 > Can I have your ACK on this patch? I've verified combination of [PATCH] drivers: pinctrl: add active state to core [RFC] ARM: OMAP2+: omap_device: add pinctrl handling on OMAP4 SDP board+UARTs and going to send patches on Monday. So, Acked/Tested-by: Grygorii Strashko <grygorii.strashko@ti.com> (I can't reply directly to thread - not signed to those lists :)) What is the best branch to base my patches on? > > I do not want to add the state unless there is a clear consumer, > so it needs to go in with the first patch to a device driver that uses > pinctrl_pm_select_active_state() which will be a good demonstration > on its use and utility. (And a point to object and suggest other ways > to do the same thing...) > > Yours, > Linus Walleij > Regards, -grygorii
On Fri, Jul 12, 2013 at 5:36 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > On 07/10/2013 11:36 PM, Linus Walleij wrote: >> >> I guess we need a patch set prepared which adds the "active" state >> and helper function as the first patch, i.e. this: >> http://marc.info/?l=linux-kernel&m=137094012703340&w=2 >> Can I have your ACK on this patch? > > I've verified combination of > [PATCH] drivers: pinctrl: add active state to core > [RFC] ARM: OMAP2+: omap_device: add pinctrl handling > on OMAP4 SDP board+UARTs and going to send patches on Monday. > > So, Acked/Tested-by: Grygorii Strashko <grygorii.strashko@ti.com> > (I can't reply directly to thread - not signed to those lists :)) > > What is the best branch to base my patches on? I suggest you coordinate that with Tony. He can take my patch with your ACK if it helps, but it seems he's already up to optimizing the pinctrl core :-) Yours, Linus Walleij
diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c index e37feb2..d8cf17b 100644 --- a/arch/arm/mach-omap2/omap_device.c +++ b/arch/arm/mach-omap2/omap_device.c @@ -39,6 +39,7 @@ #include "soc.h" #include "omap_device.h" #include "omap_hwmod.h" +#include "iomap.h" /* Private functions */ @@ -585,8 +586,10 @@ static int _od_runtime_suspend(struct device *dev) ret = pm_generic_runtime_suspend(dev); - if (!ret) + if (!ret) { omap_device_idle(pdev); + pinctrl_pm_select_idle_state(dev); + } return ret; } @@ -595,12 +598,26 @@ static int _od_runtime_resume(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); + /* TODO: should be replaced to pinctrl_pm_select_active_state() */ + pinctrl_pm_select_default_state(dev); + omap_device_enable(pdev); return pm_generic_runtime_resume(dev); } #endif +void _od_suspend_sel_pinctrl_state(struct device *dev) +{ + if (!dev->pins) + return; + /* try to select *deepest* pinctrl state */ + if (IS_ERR(dev->pins->sleep_state)) + pinctrl_pm_select_idle_state(dev); + else + pinctrl_pm_select_sleep_state(dev); +} + #ifdef CONFIG_SUSPEND static int _od_suspend_noirq(struct device *dev) { @@ -613,12 +630,22 @@ static int _od_suspend_noirq(struct device *dev) return 0; ret = pm_generic_suspend_noirq(dev); + if (!ret) { + if (!pm_runtime_status_suspended(dev)) { + if (pm_generic_runtime_suspend(dev) == 0) { + if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) { + omap_device_idle(pdev); + _od_suspend_sel_pinctrl_state(dev); + } + } - if (!ret && !pm_runtime_status_suspended(dev)) { - if (pm_generic_runtime_suspend(dev) == 0) { - if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) - omap_device_idle(pdev); od->flags |= OMAP_DEVICE_SUSPENDED; + } else { + /* + * "idle" pinctrl state already applied - + * try to set "sleep" state + */ + pinctrl_pm_select_sleep_state(dev); } } @@ -633,9 +660,15 @@ static int _od_resume_noirq(struct device *dev) if ((od->flags & OMAP_DEVICE_SUSPENDED) && !pm_runtime_status_suspended(dev)) { od->flags &= ~OMAP_DEVICE_SUSPENDED; - if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) + if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)) { omap_device_enable(pdev); + /* TODO: should be replaced to pinctrl_pm_select_active_state() */ + pinctrl_pm_select_default_state(dev); + } pm_generic_runtime_resume(dev); + } else if (!pm_runtime_status_suspended(dev)) { + /* switch back to "idle" pinctrl state */ + pinctrl_pm_select_idle_state(dev); } return pm_generic_resume_noirq(dev);
Before switching to DT pinctrl states of OMAP IPs have been handled by hwmod framework. After switching to DT-boot the pinctrl handling was dropped from hwmod framework and, as it was recommended, OMAP IP's drivers have to be updated to handle pinctrl states by itself using pinctrl_pm_select_xx() helpers (see http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173514.html) But this is not right for OMAP2+ SoC where real IPs state is controlled by omap_device core which enables/disables modules & clocks actually. For example, if OMAP I2C driver will handle pinctrl state during system wide suspend the following issue may occure: - suspend_noirq - I2C device can be still active because of PM auto-suspend |-_od_suspend_noirq |- omap_i2c_suspend_noirq |- PINs state set to SLEEP |- pm_generic_runtime_suspend |- omap_i2c_runtime_suspend() |- PINs state set to IDLE <--- *oops* PINs state is IDLE and not SLEEP |- omap_device_idle() |- omap_hwmod_idle() |- _idle() |- disbale module (sysc&clocks) - resume_noirq - I2C was active before suspend |-_od_resume_noirq |- omap_hwmod_enable() |- _enable() |- enable module (sysc&clocks) |- pm_generic_runtime_resume |- omap_i2c_runtime_resume() |- PINs state set to DEFAULT <--- !!!! |- omap_i2c_resume_noirq |- PINs state set to DEFAULT |- PINs state set to IDLE <--- *big oops* we have active module and its PINs state is IDLE (see https://patchwork.kernel.org/patch/2642101/) Of course, everything can be handled by adding a tons of code in ecah driver to check PM state of device and override default behavior of omap_device core, but this not good. Hence, add pinctrl handling in omap_device core: 1) on PM runtime resume - switch pinctrl state to "default" (todo: "active") 2) on PM runtime suspend - switch pinctrl state to "idle" 3) during system wide suspend - switch pinctrl state to "sleep" or "idle" if omap_device core disables device - switch pinctrl state to "sleep" if device is disabled already 4) during system wide resume - switch pinctrl state to "default" (todo: "active") if omap_device core has disabled device during suspend - switch pinctrl state to "idle" if device was already disabled before suspend This will enable pinctrl for all OMAP2+ IP's drivers by default - no changes in code is needed and only DT data will need to be updated (add "default", "active", "idle", "sleep" states). This will enable pinctrl handling for all OMAP2+ drivers by default - no changes in code is needed and only DT data will need to be updated (add "default", "active", "idle", "sleep" states). Related discussions: - [3/3] i2c: nomadik: use pinctrl PM helpers https://patchwork.kernel.org/patch/2670291/ - mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime https://patchwork.kernel.org/patch/2690191/ - [PATCH 00/11] drivers: Add Pinctrl PM support https://lkml.org/lkml/2013/5/31/210 CC: Hebbar Gururaja <gururaja.hebbar@ti.com> CC: Linus Walleij <linus.walleij@linaro.org> CC: linux-arm-kernel@lists.infradead.org CC: linux-omap@vger.kernel.org CC: linux-kernel@vger.kernel.org Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- Hi Kevin, Tony, I've verified this patch on OMAP4 SDP board: - PM runtime for I2C4, UART2, UART3 - suspend/resume with I2C4, UART2, UART3 seems it works and pinctrl states have been updated as expected. TODO: - need to be rebased when support for "active" state will be added. - need to be rebased when "Omap serial cleanup" patch series will be merged https://lkml.org/lkml/2013/4/26/503 based on top of: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git 849aa58 Add linux-next specific files for 20130620 Best regards - grygorii arch/arm/mach-omap2/omap_device.c | 45 ++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 6 deletions(-)