Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On Tue, Jul 30, 2013 at 12:01 AM, Stephen Warren wrote: > I was thinking more about people writing the device trees that define > these states; they need to explicitly make the choice re: overlapping > states or independent states. We should not plan to obsolete any current > usage of overlapping states since that will mean an incompatible change > to the DT ABI (deprecate yes so that no more usage is added, but the > kernel should still support the old way). This is another reason to not group and encode explicitly the pins that remain unchanged during state transitions. I prefer that either: - when we build up the state containers in the subsystem, we identify overlapping pins and encode them in the state container somehow or: - when transitioning from state A -> state B we identify ovelapping pins or groups of pins and do not touch them by making calls down to the driver ->free() and ->request() callback pair. or: the pinctrl-single.c driver in it's callbacks like ->enable() ->disable(), ->request(), ->free() internally short cuts from its knowledge of such pin shortcuts and no other drivers are affected (nor helped) by this optimization. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On Tue, Jul 30, 2013 at 12:01 AM, Stephen Warren swar...@wwwdotorg.org wrote: I was thinking more about people writing the device trees that define these states; they need to explicitly make the choice re: overlapping states or independent states. We should not plan to obsolete any current usage of overlapping states since that will mean an incompatible change to the DT ABI (deprecate yes so that no more usage is added, but the kernel should still support the old way). This is another reason to not group and encode explicitly the pins that remain unchanged during state transitions. I prefer that either: - when we build up the state containers in the subsystem, we identify overlapping pins and encode them in the state container somehow or: - when transitioning from state A - state B we identify ovelapping pins or groups of pins and do not touch them by making calls down to the driver -free() and -request() callback pair. or: the pinctrl-single.c driver in it's callbacks like -enable() -disable(), -request(), -free() internally short cuts from its knowledge of such pin shortcuts and no other drivers are affected (nor helped) by this optimization. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/29/2013 03:21 AM, Tony Lindgren wrote: > * Stephen Warren [130719 12:05]: >> On 07/19/2013 01:39 AM, Tony Lindgren wrote: >>> >>> I think the only sane way to deal with this is to make the I2C controller >>> to show up as two separate I2C controller instances. Then use runtime >>> PM to save and restore the state for each instance, and locking between >>> the two driver instances. >>> >>> For the pin muxing part, I'd do this: >>> >>> i2c1 instance i2c2 instance notes >>> default_state 0 pins 0 pins (or dedicated >>> pins only) >>> active_stateall pinsalls pins >>> idle_state safe mode safe mode >>> >>> Then when i2c1 instance is done, it's runtime PM suspend muxes the pins >>> to safe mode, or some nop state. Then when i2c2 instance is woken, it's >>> runtime PM resume muxes pins to i2c2. >> >> I see two issues with that approach: >> >> 1) Runtime PM doesn't always put a device into an idle state as soon as >> its work is done. Sometimes (always?) there is a delay between when the >> device is last used and when the HW is actually made idle so that if the >> device is re-activated quickly, the kernel hasn't wasted time making it >> idle then active again. You'd have to force that delay to complete when >> switching between the two virtual controller devices. > > There is the autosuspend timeout for delayed transitions, but I think > runtime PM already has calls for making the state change immediate also. True, but I /think/ that then you could never use the APIs that perform delayed idle, since you'd always need to force immediate idle to guarantee you could always immediately switch to the other state/virtual-controller. >> 2) This requires two separate device objects for the two I2C instances. >> I guess you could have the driver for the one I2C mux node end up >> instantiating two child devices for this purpose, and hence make that >> happen without needing to change the DT ABI. However, that sure feels >> complex... > > Yes but you will also automatically get quite a bit of sanity to your > I2C driver code rather than trying to handle the two separate instances > within the driver alone. Consider things like scanning the I2C buses for > devices and just dev_dbg(). The I2C mux is already very simple. IIRC, it instantiates a separate "struct i2c_adapter" for each "virtual" I2C controller. It actually looks like each of those already embeds its own "struct device", so perhaps this issue is a little moot. However, we'd have to find some way of redirecting pinctrl requests from those child devices to the top-level I2C mux struct device itself, since that's what the pinctrl mapping table entries are defined for. It seems much simpler to just leave the pinctrl stuff controlled by that top-level device, rather than pushing it down to the child virtual devices/adapters. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/29/2013 03:05 AM, Tony Lindgren wrote: > * Stephen Warren [130719 11:59]: >> On 07/19/2013 01:29 AM, Tony Lindgren wrote: >>> >>> I'd vote for keeping the existing behaviour with pinctrl_select_state() >>> when no active state is defined. >> >> Yes, I think that will work, since the active state cannot exist before >> this new scheme is in place. > > Right. > >> But, this needs to be very clearly spell out in the DT binding >> documentation: If you have states default/idle/sleep, they're complete >> alternatives, whereas if you have states default/active/idle/sleep, the >> latter 3 are alternatives that build on top of the first. I foresee mass >> confusion, but perhaps I'm being pessimistic. > > I'm hoping we can automate the runtime PM handling with default/active/idle > completely from the consumer driver point of view. And then when that's > working, we can probably deprecate any runtime PM related handling using > pinctr_select_state() and print warnings. And we can also improve the > documentation so no new users will use the default/idle/sleep for runtime > PM unless they really want to. I was thinking more about people writing the device trees that define these states; they need to explicitly make the choice re: overlapping states or independent states. We should not plan to obsolete any current usage of overlapping states since that will mean an incompatible change to the DT ABI (deprecate yes so that no more usage is added, but the kernel should still support the old way). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Linus Walleij [130722 16:14]: > On Tue, Jul 16, 2013 at 11:05 AM, Tony Lindgren wrote: > > > To toggle dynamic states, let's add the optional active state in > > addition to the static default state. Then if the optional active > > state is defined, we can require that idle and sleep states cover > > the same pingroups as the active state. > > OK... > > > Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() > > to use instead of pinctrl_select() to avoid breaking existing users. > > > > With pinctrl_check_dynamic() we can check that idle and sleep states > > match the active state for pingroups during init, and don't need to > > do it during runtime. > > I do not understand why this complexity need to be exposed outside > of the subsystem. Unfortunately it's mostly to deal with supporting the current behaviour of pinctrl_select_state() which is not quite suitable for runtime PM. > pinctrl_select_state() and the PM accessors are enough IMO. Why > should say a driver care about whether it is dynamic or not? I think we can make this all transparent to the consumer drivers for runtime PM. Basically drivers/base/pinctrl.c needs these for the checks because of the current pinctrl_select_state(). > Surely the checking and different paths for static/dynamic configurations > can be an intrinsic detail of the pinctrl subsystem, by adding flags and > members to private structs like struct pinctrl itself in worst case. I'll take a look if we can bury more things inside the pinctrl subsystem. > So I'm not buying into this, it looks like it is making things complicated > for consumers outside the subsystem for no reason. I don't think the consumer drivers eventually need to do much anything ideally. We're missing runtime PM related set_irq_wake() but that's a minor detail as we can initially keep the runtime PM related wake-up events always enabled. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren [130719 12:05]: > On 07/19/2013 01:39 AM, Tony Lindgren wrote: > > > > I think the only sane way to deal with this is to make the I2C controller > > to show up as two separate I2C controller instances. Then use runtime > > PM to save and restore the state for each instance, and locking between > > the two driver instances. > > > > For the pin muxing part, I'd do this: > > > > i2c1 instance i2c2 instance notes > > default_state 0 pins 0 pins (or dedicated > > pins only) > > active_stateall pinsalls pins > > idle_state safe mode safe mode > > > > Then when i2c1 instance is done, it's runtime PM suspend muxes the pins > > to safe mode, or some nop state. Then when i2c2 instance is woken, it's > > runtime PM resume muxes pins to i2c2. > > I see two issues with that approach: > > 1) Runtime PM doesn't always put a device into an idle state as soon as > its work is done. Sometimes (always?) there is a delay between when the > device is last used and when the HW is actually made idle so that if the > device is re-activated quickly, the kernel hasn't wasted time making it > idle then active again. You'd have to force that delay to complete when > switching between the two virtual controller devices. There is the autosuspend timeout for delayed transitions, but I think runtime PM already has calls for making the state change immediate also. > 2) This requires two separate device objects for the two I2C instances. > I guess you could have the driver for the one I2C mux node end up > instantiating two child devices for this purpose, and hence make that > happen without needing to change the DT ABI. However, that sure feels > complex... Yes but you will also automatically get quite a bit of sanity to your I2C driver code rather than trying to handle the two separate instances within the driver alone. Consider things like scanning the I2C buses for devices and just dev_dbg(). > I wonder if it wouldn't be better to have active/idle/sleep as > "modifiers" to the state name rather than alternatives, so you end up > with states named: > > default > nobus:active > nobus:idle > nobus:sleep > bus0:active > bus0:idle > bus0:sleep > bus1:active > bus1:idle > bus1:sleep > > Of course, if you continue on with that approach (i.e. you add more > sub-fields each separated by a colon), you end up with a huge > combinatorial mess of state names. Right :) Sooner or later we'll have some totally messed up piece of hardware pop up that multiplexes one controller across a large number number buses.. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren [130719 12:10]: > On 07/19/2013 04:29 AM, Grygorii Strashko wrote: > > > > First of all, I'd like to mention that these patches do *not* connect > > pinctrl to PM runtime, so until driver will call pinctrl_select_state() > > or pinctrl_pm_select_*() there will be no pins state changes. > > Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be > called automatically by the runtime PM core, so that we don't need to > write code to do this in every single driver, just like we moved the > call to pinctrl_select_state(default) into the device core so that we > didn't have to make every device do that manually? Yes I think we can make it all automatic. So far it seems that the last missing piece was Linus' suggestion of making it mostly happen using irqchip with calls to pinctrl so consumer drivers may not need to do anything. > > (As result, i2c-mux is not good example, seems:)) > > As such, I think all situations are good examples, because a generic > feature has to work in all cases. Yes we need to support both runtime PM, and more complex cases of sharing pins between devices for example that are not runtime PM related. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren [130719 11:59]: > On 07/19/2013 01:29 AM, Tony Lindgren wrote: > > > > I'd vote for keeping the existing behaviour with pinctrl_select_state() > > when no active state is defined. > > Yes, I think that will work, since the active state cannot exist before > this new scheme is in place. Right. > But, this needs to be very clearly spell out in the DT binding > documentation: If you have states default/idle/sleep, they're complete > alternatives, whereas if you have states default/active/idle/sleep, the > latter 3 are alternatives that build on top of the first. I foresee mass > confusion, but perhaps I'm being pessimistic. I'm hoping we can automate the runtime PM handling with default/active/idle completely from the consumer driver point of view. And then when that's working, we can probably deprecate any runtime PM related handling using pinctr_select_state() and print warnings. And we can also improve the documentation so no new users will use the default/idle/sleep for runtime PM unless they really want to. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren swar...@wwwdotorg.org [130719 11:59]: On 07/19/2013 01:29 AM, Tony Lindgren wrote: I'd vote for keeping the existing behaviour with pinctrl_select_state() when no active state is defined. Yes, I think that will work, since the active state cannot exist before this new scheme is in place. Right. But, this needs to be very clearly spell out in the DT binding documentation: If you have states default/idle/sleep, they're complete alternatives, whereas if you have states default/active/idle/sleep, the latter 3 are alternatives that build on top of the first. I foresee mass confusion, but perhaps I'm being pessimistic. I'm hoping we can automate the runtime PM handling with default/active/idle completely from the consumer driver point of view. And then when that's working, we can probably deprecate any runtime PM related handling using pinctr_select_state() and print warnings. And we can also improve the documentation so no new users will use the default/idle/sleep for runtime PM unless they really want to. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren swar...@wwwdotorg.org [130719 12:10]: On 07/19/2013 04:29 AM, Grygorii Strashko wrote: First of all, I'd like to mention that these patches do *not* connect pinctrl to PM runtime, so until driver will call pinctrl_select_state() or pinctrl_pm_select_*() there will be no pins state changes. Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be called automatically by the runtime PM core, so that we don't need to write code to do this in every single driver, just like we moved the call to pinctrl_select_state(default) into the device core so that we didn't have to make every device do that manually? Yes I think we can make it all automatic. So far it seems that the last missing piece was Linus' suggestion of making it mostly happen using irqchip with calls to pinctrl so consumer drivers may not need to do anything. (As result, i2c-mux is not good example, seems:)) As such, I think all situations are good examples, because a generic feature has to work in all cases. Yes we need to support both runtime PM, and more complex cases of sharing pins between devices for example that are not runtime PM related. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren swar...@wwwdotorg.org [130719 12:05]: On 07/19/2013 01:39 AM, Tony Lindgren wrote: I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances. For the pin muxing part, I'd do this: i2c1 instance i2c2 instance notes default_state 0 pins 0 pins (or dedicated pins only) active_stateall pinsalls pins idle_state safe mode safe mode Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2. I see two issues with that approach: 1) Runtime PM doesn't always put a device into an idle state as soon as its work is done. Sometimes (always?) there is a delay between when the device is last used and when the HW is actually made idle so that if the device is re-activated quickly, the kernel hasn't wasted time making it idle then active again. You'd have to force that delay to complete when switching between the two virtual controller devices. There is the autosuspend timeout for delayed transitions, but I think runtime PM already has calls for making the state change immediate also. 2) This requires two separate device objects for the two I2C instances. I guess you could have the driver for the one I2C mux node end up instantiating two child devices for this purpose, and hence make that happen without needing to change the DT ABI. However, that sure feels complex... Yes but you will also automatically get quite a bit of sanity to your I2C driver code rather than trying to handle the two separate instances within the driver alone. Consider things like scanning the I2C buses for devices and just dev_dbg(). I wonder if it wouldn't be better to have active/idle/sleep as modifiers to the state name rather than alternatives, so you end up with states named: default nobus:active nobus:idle nobus:sleep bus0:active bus0:idle bus0:sleep bus1:active bus1:idle bus1:sleep Of course, if you continue on with that approach (i.e. you add more sub-fields each separated by a colon), you end up with a huge combinatorial mess of state names. Right :) Sooner or later we'll have some totally messed up piece of hardware pop up that multiplexes one controller across a large number number buses.. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Linus Walleij linus.wall...@linaro.org [130722 16:14]: On Tue, Jul 16, 2013 at 11:05 AM, Tony Lindgren t...@atomide.com wrote: To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state. OK... Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users. With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime. I do not understand why this complexity need to be exposed outside of the subsystem. Unfortunately it's mostly to deal with supporting the current behaviour of pinctrl_select_state() which is not quite suitable for runtime PM. pinctrl_select_state() and the PM accessors are enough IMO. Why should say a driver care about whether it is dynamic or not? I think we can make this all transparent to the consumer drivers for runtime PM. Basically drivers/base/pinctrl.c needs these for the checks because of the current pinctrl_select_state(). Surely the checking and different paths for static/dynamic configurations can be an intrinsic detail of the pinctrl subsystem, by adding flags and members to private structs like struct pinctrl itself in worst case. I'll take a look if we can bury more things inside the pinctrl subsystem. So I'm not buying into this, it looks like it is making things complicated for consumers outside the subsystem for no reason. I don't think the consumer drivers eventually need to do much anything ideally. We're missing runtime PM related set_irq_wake() but that's a minor detail as we can initially keep the runtime PM related wake-up events always enabled. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/29/2013 03:05 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130719 11:59]: On 07/19/2013 01:29 AM, Tony Lindgren wrote: I'd vote for keeping the existing behaviour with pinctrl_select_state() when no active state is defined. Yes, I think that will work, since the active state cannot exist before this new scheme is in place. Right. But, this needs to be very clearly spell out in the DT binding documentation: If you have states default/idle/sleep, they're complete alternatives, whereas if you have states default/active/idle/sleep, the latter 3 are alternatives that build on top of the first. I foresee mass confusion, but perhaps I'm being pessimistic. I'm hoping we can automate the runtime PM handling with default/active/idle completely from the consumer driver point of view. And then when that's working, we can probably deprecate any runtime PM related handling using pinctr_select_state() and print warnings. And we can also improve the documentation so no new users will use the default/idle/sleep for runtime PM unless they really want to. I was thinking more about people writing the device trees that define these states; they need to explicitly make the choice re: overlapping states or independent states. We should not plan to obsolete any current usage of overlapping states since that will mean an incompatible change to the DT ABI (deprecate yes so that no more usage is added, but the kernel should still support the old way). -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/29/2013 03:21 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130719 12:05]: On 07/19/2013 01:39 AM, Tony Lindgren wrote: I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances. For the pin muxing part, I'd do this: i2c1 instance i2c2 instance notes default_state 0 pins 0 pins (or dedicated pins only) active_stateall pinsalls pins idle_state safe mode safe mode Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2. I see two issues with that approach: 1) Runtime PM doesn't always put a device into an idle state as soon as its work is done. Sometimes (always?) there is a delay between when the device is last used and when the HW is actually made idle so that if the device is re-activated quickly, the kernel hasn't wasted time making it idle then active again. You'd have to force that delay to complete when switching between the two virtual controller devices. There is the autosuspend timeout for delayed transitions, but I think runtime PM already has calls for making the state change immediate also. True, but I /think/ that then you could never use the APIs that perform delayed idle, since you'd always need to force immediate idle to guarantee you could always immediately switch to the other state/virtual-controller. 2) This requires two separate device objects for the two I2C instances. I guess you could have the driver for the one I2C mux node end up instantiating two child devices for this purpose, and hence make that happen without needing to change the DT ABI. However, that sure feels complex... Yes but you will also automatically get quite a bit of sanity to your I2C driver code rather than trying to handle the two separate instances within the driver alone. Consider things like scanning the I2C buses for devices and just dev_dbg(). The I2C mux is already very simple. IIRC, it instantiates a separate struct i2c_adapter for each virtual I2C controller. It actually looks like each of those already embeds its own struct device, so perhaps this issue is a little moot. However, we'd have to find some way of redirecting pinctrl requests from those child devices to the top-level I2C mux struct device itself, since that's what the pinctrl mapping table entries are defined for. It seems much simpler to just leave the pinctrl stuff controlled by that top-level device, rather than pushing it down to the child virtual devices/adapters. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On Fri, Jul 19, 2013 at 9:03 PM, Stephen Warren wrote: > On 07/19/2013 04:29 AM, Grygorii Strashko wrote: >> First of all, I'd like to mention that these patches do *not* connect >> pinctrl to PM runtime, so until driver will call pinctrl_select_state() >> or pinctrl_pm_select_*() there will be no pins state changes. > > Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be > called automatically by the runtime PM core, Nah I had no such complete ambitions, just factoring around. There are examples, such as deactivating a TTY from userspace, that should result in the pins going to sleep while it may have nothing to do with runtime PM. > so that we don't need to > write code to do this in every single driver, just like we moved the > call to pinctrl_select_state(default) into the device core so that we > didn't have to make every device do that manually? I am pretty convinced that if this dynamic muxing stuff shall be implemented, it should be a pinctrl subsystem intrinsic optimization detail and should not be exposed to the outside with all these extra functions at all. It looks overly complicated to me. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On Tue, Jul 16, 2013 at 11:05 AM, Tony Lindgren wrote: > To toggle dynamic states, let's add the optional active state in > addition to the static default state. Then if the optional active > state is defined, we can require that idle and sleep states cover > the same pingroups as the active state. OK... > Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() > to use instead of pinctrl_select() to avoid breaking existing users. > > With pinctrl_check_dynamic() we can check that idle and sleep states > match the active state for pingroups during init, and don't need to > do it during runtime. I do not understand why this complexity need to be exposed outside of the subsystem. pinctrl_select_state() and the PM accessors are enough IMO. Why should say a driver care about whether it is dynamic or not? Surely the checking and different paths for static/dynamic configurations can be an intrinsic detail of the pinctrl subsystem, by adding flags and members to private structs like struct pinctrl itself in worst case. So I'm not buying into this, it looks like it is making things complicated for consumers outside the subsystem for no reason. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On Tue, Jul 16, 2013 at 11:05 AM, Tony Lindgren t...@atomide.com wrote: To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state. OK... Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users. With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime. I do not understand why this complexity need to be exposed outside of the subsystem. pinctrl_select_state() and the PM accessors are enough IMO. Why should say a driver care about whether it is dynamic or not? Surely the checking and different paths for static/dynamic configurations can be an intrinsic detail of the pinctrl subsystem, by adding flags and members to private structs like struct pinctrl itself in worst case. So I'm not buying into this, it looks like it is making things complicated for consumers outside the subsystem for no reason. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On Fri, Jul 19, 2013 at 9:03 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 07/19/2013 04:29 AM, Grygorii Strashko wrote: First of all, I'd like to mention that these patches do *not* connect pinctrl to PM runtime, so until driver will call pinctrl_select_state() or pinctrl_pm_select_*() there will be no pins state changes. Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be called automatically by the runtime PM core, Nah I had no such complete ambitions, just factoring around. There are examples, such as deactivating a TTY from userspace, that should result in the pins going to sleep while it may have nothing to do with runtime PM. so that we don't need to write code to do this in every single driver, just like we moved the call to pinctrl_select_state(default) into the device core so that we didn't have to make every device do that manually? I am pretty convinced that if this dynamic muxing stuff shall be implemented, it should be a pinctrl subsystem intrinsic optimization detail and should not be exposed to the outside with all these extra functions at all. It looks overly complicated to me. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/19/2013 01:29 AM, Tony Lindgren wrote: > * Stephen Warren [130718 12:27]: >> On 07/18/2013 01:25 AM, Tony Lindgren wrote: >>> * Stephen Warren [130717 14:21]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: > To toggle dynamic states, let's add the optional active state in > addition to the static default state. Then if the optional active > state is defined, we can require that idle and sleep states cover > the same pingroups as the active state. > > Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() > to use instead of pinctrl_select() to avoid breaking existing users. > > With pinctrl_check_dynamic() we can check that idle and sleep states > match the active state for pingroups during init, and don't need to > do it during runtime. > > Then with the states pre-validated, pinctrl_select_dynamic() can > just toggle between the dynamic states without extra checks. > > Note that pinctr_select_state() still has valid use cases, such as > changing states when the pins can be shared between two drivers > and don't necessarily cover the same pingroups. For dynamic runtime > toggling of pin states, we should eventually always use just > pinctrl_select_dynamic(). >> > @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device > *dev, struct pinctrl_state *sta > return 0; > if (IS_ERR(state)) > return 0; /* No such state */ > - ret = pinctrl_select_state(pins->p, state); > + > + /* Configured for proper dynamic muxing? */ > + if (!IS_ERR(dev->pins->active_state)) > + ret = pinctrl_select_dynamic(pins->p, state); > + else > + ret = pinctrl_select_state(pins->p, state); Hmmm. I'm not quite sure this is right... Surely this function should just do nothing if the dynamic states aren't defined? The system should just, and only, be in the "default" state and never do anything else. >>> >>> This keeps the existing behaviour. We should be able to drop the >>> call to pinctrl_select_state() here after the existing use in >>> drivers has been fixed. >> >> How many DT-based systems already have multiple of default/idle/sleep >> states defined in DT? Right now, since the kernel code uses >> pinctrl_select_state() to switch between those, the state definitions >> need to define /all/ pins used by those states, not just the dynamic >> ones. However, with this series in place, the default state should only >> include the static pins, and the active/idle/sleep states should only >> include the dynamic pins. That's a change to the DT bindings, since it >> changes how the DT must be written, and causes older DTs to be >> incompatible with newer kernels and vice-versa. > > Well we can keep the current behaviour with pinctrl_select_state() around > as long as needed. For the legacy cases, there is no active state defined > and we fall back to pinctrl_select_state(). > >> So, do we just ignore this DT ABI change again, or insist on doing it in >> some compatible way? DT ABI-ness is a PITA:-( > > I'd vote for keeping the existing behaviour with pinctrl_select_state() > when no active state is defined. Yes, I think that will work, since the active state cannot exist before this new scheme is in place. But, this needs to be very clearly spell out in the DT binding documentation: If you have states default/idle/sleep, they're complete alternatives, whereas if you have states default/active/idle/sleep, the latter 3 are alternatives that build on top of the first. I foresee mass confusion, but perhaps I'm being pessimistic. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/19/2013 01:39 AM, Tony Lindgren wrote: > * Stephen Warren [130718 12:33]: >> On 07/18/2013 01:36 AM, Tony Lindgren wrote: >>> * Stephen Warren [130717 14:30]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: >> ... Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? >>> >>> The idea is that runtime PM will never touch the basic mux settings >>> at all. The "default" state should be considered a static state >>> that is claimed during driver probe, and released when the driver >>> is unloaded. This is typically let's say 90% of the pins for any >>> device. >>> >>> For runtime PM, we can just toggle the PM related pinctrl states as >>> they have been verified to match the active state during init. >>> >>> So I don't see why pinctrl-I2C would have to do anything specific. >>> All that is required is that the pins are grouped for the consumer >>> driver, and we can provide some automated checks on the states for >>> runtime PM. >> >> So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C >> buses: >> >> a) bus 1: I2C controller is muxed out onto one set of pins. >> >> b) bus 2: I2C controller is muxed out onto another set of pins. >> >> Now, the system could go idle in either of those 2 states, and then >> later need to return to one of those states. I just don't see how that >> would work, since the runtime PM code in this series switches to *an* >> active state when the device becomes non-idle. If the definition of the >> idle state switches the mux function for both sets of pins to some >> idle/quiescent value, then you'd need to do different reprogramming when >> going back to "the" active state; I guess the system would need to >> remember which state was active before switching to idle, then switch >> back to that same state rather than hard-coding the active state name as >> "active"... > > I think the only sane way to deal with this is to make the I2C controller > to show up as two separate I2C controller instances. Then use runtime > PM to save and restore the state for each instance, and locking between > the two driver instances. > > For the pin muxing part, I'd do this: > > i2c1 instance i2c2 instance notes > default_state 0 pins 0 pins (or dedicated pins only) > active_state all pinsalls pins > idle_statesafe mode safe mode > > Then when i2c1 instance is done, it's runtime PM suspend muxes the pins > to safe mode, or some nop state. Then when i2c2 instance is woken, it's > runtime PM resume muxes pins to i2c2. I see two issues with that approach: 1) Runtime PM doesn't always put a device into an idle state as soon as its work is done. Sometimes (always?) there is a delay between when the device is last used and when the HW is actually made idle so that if the device is re-activated quickly, the kernel hasn't wasted time making it idle then active again. You'd have to force that delay to complete when switching between the two virtual controller devices. 2) This requires two separate device objects for the two I2C instances. I guess you could have the driver for the one I2C mux node end up instantiating two child devices for this purpose, and hence make that happen without needing to change the DT ABI. However, that sure feels complex... I wonder if it wouldn't be better to have active/idle/sleep as "modifiers" to the state name rather than alternatives, so you end up with states named: default nobus:active nobus:idle nobus:sleep bus0:active bus0:idle bus0:sleep bus1:active bus1:idle bus1:sleep Of course, if you continue on with that approach (i.e. you add more sub-fields each separated by a colon), you end up with a huge combinatorial mess of state names. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/19/2013 04:29 AM, Grygorii Strashko wrote: > Hi Tony, Stephen > > On 07/19/2013 10:39 AM, Tony Lindgren wrote: >> * Stephen Warren [130718 12:33]: >>> On 07/18/2013 01:36 AM, Tony Lindgren wrote: * Stephen Warren [130717 14:30]: > On 07/16/2013 03:05 AM, Tony Lindgren wrote: >>> ... > Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime > PM? Does the mux setting select which states are used for runtime > PM, or > does runtime PM override the basic mux setting, or must the pincrl-I2C > mux manually implement custom runtime-PM/pinctrl interaction since > there's no generic answer to those questions? How many more custom > exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The "default" state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. >>> >>> So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C >>> buses: >>> >>> a) bus 1: I2C controller is muxed out onto one set of pins. >>> >>> b) bus 2: I2C controller is muxed out onto another set of pins. >>> >>> Now, the system could go idle in either of those 2 states, and then >>> later need to return to one of those states. I just don't see how that >>> would work, since the runtime PM code in this series switches to *an* >>> active state when the device becomes non-idle. If the definition of the >>> idle state switches the mux function for both sets of pins to some >>> idle/quiescent value, then you'd need to do different reprogramming when >>> going back to "the" active state; I guess the system would need to >>> remember which state was active before switching to idle, then switch >>> back to that same state rather than hard-coding the active state name as >>> "active"... >> >> I think the only sane way to deal with this is to make the I2C controller >> to show up as two separate I2C controller instances. Then use runtime >> PM to save and restore the state for each instance, and locking between >> the two driver instances. >> >> For the pin muxing part, I'd do this: >> >> i2c1 instancei2c2 instancenotes >> default_state0 pins0 pins(or dedicated pins only) >> active_stateall pinsalls pins >> idle_statesafe modesafe mode >> >> Then when i2c1 instance is done, it's runtime PM suspend muxes the pins >> to safe mode, or some nop state. Then when i2c2 instance is woken, it's >> runtime PM resume muxes pins to i2c2. > > First of all, I'd like to mention that these patches do *not* connect > pinctrl to PM runtime, so until driver will call pinctrl_select_state() > or pinctrl_pm_select_*() there will be no pins state changes. Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be called automatically by the runtime PM core, so that we don't need to write code to do this in every single driver, just like we moved the call to pinctrl_select_state(default) into the device core so that we didn't have to make every device do that manually? > (As result, i2c-mux is not good example, seems:)) As such, I think all situations are good examples, because a generic feature has to work in all cases. The description you gave of the behavioural changes this patch creates seems accurate at a quick glance. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
Hi Tony, Stephen
On 07/19/2013 10:39 AM, Tony Lindgren wrote:
* Stephen Warren [130718 12:33]:
On 07/18/2013 01:36 AM, Tony Lindgren wrote:
* Stephen Warren [130717 14:30]:
On 07/16/2013 03:05 AM, Tony Lindgren wrote:
...
Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime
PM? Does the mux setting select which states are used for runtime PM, or
does runtime PM override the basic mux setting, or must the pincrl-I2C
mux manually implement custom runtime-PM/pinctrl interaction since
there's no generic answer to those questions? How many more custom
exceptions will there be?
The idea is that runtime PM will never touch the basic mux settings
at all. The "default" state should be considered a static state
that is claimed during driver probe, and released when the driver
is unloaded. This is typically let's say 90% of the pins for any
device.
For runtime PM, we can just toggle the PM related pinctrl states as
they have been verified to match the active state during init.
So I don't see why pinctrl-I2C would have to do anything specific.
All that is required is that the pins are grouped for the consumer
driver, and we can provide some automated checks on the states for
runtime PM.
So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C
buses:
a) bus 1: I2C controller is muxed out onto one set of pins.
b) bus 2: I2C controller is muxed out onto another set of pins.
Now, the system could go idle in either of those 2 states, and then
later need to return to one of those states. I just don't see how that
would work, since the runtime PM code in this series switches to *an*
active state when the device becomes non-idle. If the definition of the
idle state switches the mux function for both sets of pins to some
idle/quiescent value, then you'd need to do different reprogramming when
going back to "the" active state; I guess the system would need to
remember which state was active before switching to idle, then switch
back to that same state rather than hard-coding the active state name as
"active"...
I think the only sane way to deal with this is to make the I2C controller
to show up as two separate I2C controller instances. Then use runtime
PM to save and restore the state for each instance, and locking between
the two driver instances.
For the pin muxing part, I'd do this:
i2c1 instance i2c2 instance notes
default_state 0 pins 0 pins (or dedicated pins only)
active_stateall pinsalls pins
idle_state safe mode safe mode
Then when i2c1 instance is done, it's runtime PM suspend muxes the pins
to safe mode, or some nop state. Then when i2c2 instance is woken, it's
runtime PM resume muxes pins to i2c2.
First of all, I'd like to mention that these patches do *not* connect
pinctrl to PM runtime, so until driver will call pinctrl_select_state()
or pinctrl_pm_select_*() there will be no pins state changes.
(As result, i2c-mux is not good example, seems:))
And I think, some sort of summary need to be done to explain how system
will behave after these patches in comparison to how it was before:
1) Device has pins states defined and driver uses pinctrl_select_state
(lets say legacy mode):
- "default" - no changes
- "default"+"idle"/"sleep" - no changes
- "default" + any other states - no chages
- "default"+"active" + "idle"/"sleep" - behavior will be *changed*
pinctrl_bind_pins() will do:
a) pinctrl_select_state("default")
b) pinctrl_select_dynamic("active")
but, driver uses pinctrl_select_state() to change pins state during
its work -- Is it ok?
2) Device has pins states defined and driver uses pinctrl_pm_select_*()
API only:
- if no "active" defined - behavior will be the same as for legacy mode
- "default"+"active" + "idle"/"sleep" - will behave as explained in doc
3) Device has pins states defined and driver uses
pinctrl_pm_select_*() API and pinctrl_select_state() simultaneously:
- if no "active" defined - behavior will be the same as for legacy mode
- "default"+"active" + "idle"/"sleep" + "stateX"
How it will work if during suspend driver will do:??
if (conditionY)
pinctrl_select_state(stateX);
pinctrl_pm_select_sleep_state("sleep")
Is it valid?
How it will work if during runtime resuming driver will do:??
if (conditionY)
pinctrl_select_state(stateX);
pinctrl_pm_select_active_state("active");
Is it valid?
Any way, if driver don't want support introduced default/common behavior
- all it need is to not define "active" state.
Uh, hope it will be useful and I have correct understandings here :)
Regards,
Tony
Regards,
- grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren [130718 12:33]: > On 07/18/2013 01:36 AM, Tony Lindgren wrote: > > * Stephen Warren [130717 14:30]: > >> On 07/16/2013 03:05 AM, Tony Lindgren wrote: > ... > >> Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime > >> PM? Does the mux setting select which states are used for runtime PM, or > >> does runtime PM override the basic mux setting, or must the pincrl-I2C > >> mux manually implement custom runtime-PM/pinctrl interaction since > >> there's no generic answer to those questions? How many more custom > >> exceptions will there be? > > > > The idea is that runtime PM will never touch the basic mux settings > > at all. The "default" state should be considered a static state > > that is claimed during driver probe, and released when the driver > > is unloaded. This is typically let's say 90% of the pins for any > > device. > > > > For runtime PM, we can just toggle the PM related pinctrl states as > > they have been verified to match the active state during init. > > > > So I don't see why pinctrl-I2C would have to do anything specific. > > All that is required is that the pins are grouped for the consumer > > driver, and we can provide some automated checks on the states for > > runtime PM. > > So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C > buses: > > a) bus 1: I2C controller is muxed out onto one set of pins. > > b) bus 2: I2C controller is muxed out onto another set of pins. > > Now, the system could go idle in either of those 2 states, and then > later need to return to one of those states. I just don't see how that > would work, since the runtime PM code in this series switches to *an* > active state when the device becomes non-idle. If the definition of the > idle state switches the mux function for both sets of pins to some > idle/quiescent value, then you'd need to do different reprogramming when > going back to "the" active state; I guess the system would need to > remember which state was active before switching to idle, then switch > back to that same state rather than hard-coding the active state name as > "active"... I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances. For the pin muxing part, I'd do this: i2c1 instance i2c2 instance notes default_state 0 pins 0 pins (or dedicated pins only) active_stateall pinsalls pins idle_state safe mode safe mode Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren [130718 12:27]: > On 07/18/2013 01:25 AM, Tony Lindgren wrote: > > * Stephen Warren [130717 14:21]: > >> On 07/16/2013 03:05 AM, Tony Lindgren wrote: > >>> To toggle dynamic states, let's add the optional active state in > >>> addition to the static default state. Then if the optional active > >>> state is defined, we can require that idle and sleep states cover > >>> the same pingroups as the active state. > >>> > >>> Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() > >>> to use instead of pinctrl_select() to avoid breaking existing users. > >>> > >>> With pinctrl_check_dynamic() we can check that idle and sleep states > >>> match the active state for pingroups during init, and don't need to > >>> do it during runtime. > >>> > >>> Then with the states pre-validated, pinctrl_select_dynamic() can > >>> just toggle between the dynamic states without extra checks. > >>> > >>> Note that pinctr_select_state() still has valid use cases, such as > >>> changing states when the pins can be shared between two drivers > >>> and don't necessarily cover the same pingroups. For dynamic runtime > >>> toggling of pin states, we should eventually always use just > >>> pinctrl_select_dynamic(). > > >>> @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device > >>> *dev, struct pinctrl_state *sta > >>> return 0; > >>> if (IS_ERR(state)) > >>> return 0; /* No such state */ > >>> - ret = pinctrl_select_state(pins->p, state); > >>> + > >>> + /* Configured for proper dynamic muxing? */ > >>> + if (!IS_ERR(dev->pins->active_state)) > >>> + ret = pinctrl_select_dynamic(pins->p, state); > >>> + else > >>> + ret = pinctrl_select_state(pins->p, state); > >> > >> Hmmm. I'm not quite sure this is right... Surely this function should > >> just do nothing if the dynamic states aren't defined? The system should > >> just, and only, be in the "default" state and never do anything else. > > > > This keeps the existing behaviour. We should be able to drop the > > call to pinctrl_select_state() here after the existing use in > > drivers has been fixed. > > How many DT-based systems already have multiple of default/idle/sleep > states defined in DT? Right now, since the kernel code uses > pinctrl_select_state() to switch between those, the state definitions > need to define /all/ pins used by those states, not just the dynamic > ones. However, with this series in place, the default state should only > include the static pins, and the active/idle/sleep states should only > include the dynamic pins. That's a change to the DT bindings, since it > changes how the DT must be written, and causes older DTs to be > incompatible with newer kernels and vice-versa. Well we can keep the current behaviour with pinctrl_select_state() around as long as needed. For the legacy cases, there is no active state defined and we fall back to pinctrl_select_state(). > So, do we just ignore this DT ABI change again, or insist on doing it in > some compatible way? DT ABI-ness is a PITA:-( I'd vote for keeping the existing behaviour with pinctrl_select_state() when no active state is defined. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren swar...@wwwdotorg.org [130718 12:27]: On 07/18/2013 01:25 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:21]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state. Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users. With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime. Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks. Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic(). @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta return 0; if (IS_ERR(state)) return 0; /* No such state */ - ret = pinctrl_select_state(pins-p, state); + + /* Configured for proper dynamic muxing? */ + if (!IS_ERR(dev-pins-active_state)) + ret = pinctrl_select_dynamic(pins-p, state); + else + ret = pinctrl_select_state(pins-p, state); Hmmm. I'm not quite sure this is right... Surely this function should just do nothing if the dynamic states aren't defined? The system should just, and only, be in the default state and never do anything else. This keeps the existing behaviour. We should be able to drop the call to pinctrl_select_state() here after the existing use in drivers has been fixed. How many DT-based systems already have multiple of default/idle/sleep states defined in DT? Right now, since the kernel code uses pinctrl_select_state() to switch between those, the state definitions need to define /all/ pins used by those states, not just the dynamic ones. However, with this series in place, the default state should only include the static pins, and the active/idle/sleep states should only include the dynamic pins. That's a change to the DT bindings, since it changes how the DT must be written, and causes older DTs to be incompatible with newer kernels and vice-versa. Well we can keep the current behaviour with pinctrl_select_state() around as long as needed. For the legacy cases, there is no active state defined and we fall back to pinctrl_select_state(). So, do we just ignore this DT ABI change again, or insist on doing it in some compatible way? DT ABI-ness is a PITA:-( I'd vote for keeping the existing behaviour with pinctrl_select_state() when no active state is defined. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren swar...@wwwdotorg.org [130718 12:33]: On 07/18/2013 01:36 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:30]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: ... Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The default state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses: a) bus 1: I2C controller is muxed out onto one set of pins. b) bus 2: I2C controller is muxed out onto another set of pins. Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to the active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as active... I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances. For the pin muxing part, I'd do this: i2c1 instance i2c2 instance notes default_state 0 pins 0 pins (or dedicated pins only) active_stateall pinsalls pins idle_state safe mode safe mode Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
Hi Tony, Stephen On 07/19/2013 10:39 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130718 12:33]: On 07/18/2013 01:36 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:30]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: ... Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The default state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses: a) bus 1: I2C controller is muxed out onto one set of pins. b) bus 2: I2C controller is muxed out onto another set of pins. Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to the active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as active... I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances. For the pin muxing part, I'd do this: i2c1 instance i2c2 instance notes default_state 0 pins 0 pins (or dedicated pins only) active_stateall pinsalls pins idle_state safe mode safe mode Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2. First of all, I'd like to mention that these patches do *not* connect pinctrl to PM runtime, so until driver will call pinctrl_select_state() or pinctrl_pm_select_*() there will be no pins state changes. (As result, i2c-mux is not good example, seems:)) And I think, some sort of summary need to be done to explain how system will behave after these patches in comparison to how it was before: 1) Device has pins states defined and driver uses pinctrl_select_state (lets say legacy mode): - default - no changes - default+idle/sleep - no changes - default + any other states - no chages - default+active + idle/sleep - behavior will be *changed* pinctrl_bind_pins() will do: a) pinctrl_select_state(default) b) pinctrl_select_dynamic(active) but, driver uses pinctrl_select_state() to change pins state during its work -- Is it ok? 2) Device has pins states defined and driver uses pinctrl_pm_select_*() API only: - if no active defined - behavior will be the same as for legacy mode - default+active + idle/sleep - will behave as explained in doc 3) Device has pins states defined and driver uses pinctrl_pm_select_*() API and pinctrl_select_state() simultaneously: - if no active defined - behavior will be the same as for legacy mode - default+active + idle/sleep + stateX How it will work if during suspend driver will do:?? if (conditionY) pinctrl_select_state(stateX); pinctrl_pm_select_sleep_state(sleep) Is it valid? How it will work if during runtime resuming driver will do:?? if (conditionY) pinctrl_select_state(stateX); pinctrl_pm_select_active_state(active); Is it valid? Any way, if driver don't want support introduced default/common behavior - all it need is to not define active state. Uh, hope it will be useful and I have correct understandings here :) Regards, Tony Regards, - grygorii -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/19/2013 01:39 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130718 12:33]: On 07/18/2013 01:36 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:30]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: ... Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The default state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses: a) bus 1: I2C controller is muxed out onto one set of pins. b) bus 2: I2C controller is muxed out onto another set of pins. Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to the active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as active... I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances. For the pin muxing part, I'd do this: i2c1 instance i2c2 instance notes default_state 0 pins 0 pins (or dedicated pins only) active_state all pinsalls pins idle_statesafe mode safe mode Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2. I see two issues with that approach: 1) Runtime PM doesn't always put a device into an idle state as soon as its work is done. Sometimes (always?) there is a delay between when the device is last used and when the HW is actually made idle so that if the device is re-activated quickly, the kernel hasn't wasted time making it idle then active again. You'd have to force that delay to complete when switching between the two virtual controller devices. 2) This requires two separate device objects for the two I2C instances. I guess you could have the driver for the one I2C mux node end up instantiating two child devices for this purpose, and hence make that happen without needing to change the DT ABI. However, that sure feels complex... I wonder if it wouldn't be better to have active/idle/sleep as modifiers to the state name rather than alternatives, so you end up with states named: default nobus:active nobus:idle nobus:sleep bus0:active bus0:idle bus0:sleep bus1:active bus1:idle bus1:sleep Of course, if you continue on with that approach (i.e. you add more sub-fields each separated by a colon), you end up with a huge combinatorial mess of state names. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/19/2013 01:29 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130718 12:27]: On 07/18/2013 01:25 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:21]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state. Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users. With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime. Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks. Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic(). @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta return 0; if (IS_ERR(state)) return 0; /* No such state */ - ret = pinctrl_select_state(pins-p, state); + + /* Configured for proper dynamic muxing? */ + if (!IS_ERR(dev-pins-active_state)) + ret = pinctrl_select_dynamic(pins-p, state); + else + ret = pinctrl_select_state(pins-p, state); Hmmm. I'm not quite sure this is right... Surely this function should just do nothing if the dynamic states aren't defined? The system should just, and only, be in the default state and never do anything else. This keeps the existing behaviour. We should be able to drop the call to pinctrl_select_state() here after the existing use in drivers has been fixed. How many DT-based systems already have multiple of default/idle/sleep states defined in DT? Right now, since the kernel code uses pinctrl_select_state() to switch between those, the state definitions need to define /all/ pins used by those states, not just the dynamic ones. However, with this series in place, the default state should only include the static pins, and the active/idle/sleep states should only include the dynamic pins. That's a change to the DT bindings, since it changes how the DT must be written, and causes older DTs to be incompatible with newer kernels and vice-versa. Well we can keep the current behaviour with pinctrl_select_state() around as long as needed. For the legacy cases, there is no active state defined and we fall back to pinctrl_select_state(). So, do we just ignore this DT ABI change again, or insist on doing it in some compatible way? DT ABI-ness is a PITA:-( I'd vote for keeping the existing behaviour with pinctrl_select_state() when no active state is defined. Yes, I think that will work, since the active state cannot exist before this new scheme is in place. But, this needs to be very clearly spell out in the DT binding documentation: If you have states default/idle/sleep, they're complete alternatives, whereas if you have states default/active/idle/sleep, the latter 3 are alternatives that build on top of the first. I foresee mass confusion, but perhaps I'm being pessimistic. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/19/2013 04:29 AM, Grygorii Strashko wrote: Hi Tony, Stephen On 07/19/2013 10:39 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130718 12:33]: On 07/18/2013 01:36 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:30]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: ... Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The default state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses: a) bus 1: I2C controller is muxed out onto one set of pins. b) bus 2: I2C controller is muxed out onto another set of pins. Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to the active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as active... I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances. For the pin muxing part, I'd do this: i2c1 instancei2c2 instancenotes default_state0 pins0 pins(or dedicated pins only) active_stateall pinsalls pins idle_statesafe modesafe mode Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2. First of all, I'd like to mention that these patches do *not* connect pinctrl to PM runtime, so until driver will call pinctrl_select_state() or pinctrl_pm_select_*() there will be no pins state changes. Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be called automatically by the runtime PM core, so that we don't need to write code to do this in every single driver, just like we moved the call to pinctrl_select_state(default) into the device core so that we didn't have to make every device do that manually? (As result, i2c-mux is not good example, seems:)) As such, I think all situations are good examples, because a generic feature has to work in all cases. The description you gave of the behavioural changes this patch creates seems accurate at a quick glance. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/18/2013 01:36 AM, Tony Lindgren wrote: > * Stephen Warren [130717 14:30]: >> On 07/16/2013 03:05 AM, Tony Lindgren wrote: ... >> Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime >> PM? Does the mux setting select which states are used for runtime PM, or >> does runtime PM override the basic mux setting, or must the pincrl-I2C >> mux manually implement custom runtime-PM/pinctrl interaction since >> there's no generic answer to those questions? How many more custom >> exceptions will there be? > > The idea is that runtime PM will never touch the basic mux settings > at all. The "default" state should be considered a static state > that is claimed during driver probe, and released when the driver > is unloaded. This is typically let's say 90% of the pins for any > device. > > For runtime PM, we can just toggle the PM related pinctrl states as > they have been verified to match the active state during init. > > So I don't see why pinctrl-I2C would have to do anything specific. > All that is required is that the pins are grouped for the consumer > driver, and we can provide some automated checks on the states for > runtime PM. So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses: a) bus 1: I2C controller is muxed out onto one set of pins. b) bus 2: I2C controller is muxed out onto another set of pins. Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to "the" active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as "active"... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/18/2013 01:25 AM, Tony Lindgren wrote: > * Stephen Warren [130717 14:21]: >> On 07/16/2013 03:05 AM, Tony Lindgren wrote: >>> To toggle dynamic states, let's add the optional active state in >>> addition to the static default state. Then if the optional active >>> state is defined, we can require that idle and sleep states cover >>> the same pingroups as the active state. >>> >>> Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() >>> to use instead of pinctrl_select() to avoid breaking existing users. >>> >>> With pinctrl_check_dynamic() we can check that idle and sleep states >>> match the active state for pingroups during init, and don't need to >>> do it during runtime. >>> >>> Then with the states pre-validated, pinctrl_select_dynamic() can >>> just toggle between the dynamic states without extra checks. >>> >>> Note that pinctr_select_state() still has valid use cases, such as >>> changing states when the pins can be shared between two drivers >>> and don't necessarily cover the same pingroups. For dynamic runtime >>> toggling of pin states, we should eventually always use just >>> pinctrl_select_dynamic(). >>> @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device >>> *dev, struct pinctrl_state *sta >>> return 0; >>> if (IS_ERR(state)) >>> return 0; /* No such state */ >>> - ret = pinctrl_select_state(pins->p, state); >>> + >>> + /* Configured for proper dynamic muxing? */ >>> + if (!IS_ERR(dev->pins->active_state)) >>> + ret = pinctrl_select_dynamic(pins->p, state); >>> + else >>> + ret = pinctrl_select_state(pins->p, state); >> >> Hmmm. I'm not quite sure this is right... Surely this function should >> just do nothing if the dynamic states aren't defined? The system should >> just, and only, be in the "default" state and never do anything else. > > This keeps the existing behaviour. We should be able to drop the > call to pinctrl_select_state() here after the existing use in > drivers has been fixed. How many DT-based systems already have multiple of default/idle/sleep states defined in DT? Right now, since the kernel code uses pinctrl_select_state() to switch between those, the state definitions need to define /all/ pins used by those states, not just the dynamic ones. However, with this series in place, the default state should only include the static pins, and the active/idle/sleep states should only include the dynamic pins. That's a change to the DT bindings, since it changes how the DT must be written, and causes older DTs to be incompatible with newer kernels and vice-versa. So, do we just ignore this DT ABI change again, or insist on doing it in some compatible way? DT ABI-ness is a PITA:-( -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/4] pinctrl: Add support for additional dynamic states
To toggle dynamic states, let's add the optional active state in
addition to the static default state. Then if the optional active
state is defined, we can require that idle and sleep states cover
the same pingroups as the active state.
Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
to use instead of pinctrl_select() to avoid breaking existing users.
With pinctrl_check_dynamic() we can check that idle and sleep states
match the active state for pingroups during init, and don't need to
do it during runtime.
Then with the states pre-validated, pinctrl_select_dynamic() can
just toggle between the dynamic states without extra checks.
Note that pinctr_select_state() still has valid use cases, such as
changing states when the pins can be shared between two drivers
and don't necessarily cover the same pingroups. For dynamic runtime
toggling of pin states, we should eventually always use just
pinctrl_select_dynamic().
Cc: Felipe Balbi
Cc: Grygorii Strashko
Cc: Stephen Warren
Signed-off-by: Tony Lindgren
---
Documentation/pinctrl.txt | 77 ++-
drivers/pinctrl/core.c| 226 ++---
include/linux/pinctrl/consumer.h | 46 +++
include/linux/pinctrl/devinfo.h |4 +
include/linux/pinctrl/pinctrl-state.h | 15 ++
5 files changed, 342 insertions(+), 26 deletions(-)
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 052e13a..0477ec5 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -1283,12 +1283,77 @@ This gives the exact same result as the above
construction.
Runtime pinmuxing
=
-It is possible to mux a certain function in and out at runtime, say to move
-an SPI port from one set of pins to another set of pins. Say for example for
-spi0 in the example above, we expose two different groups of pins for the same
-function, but with different named in the mapping as described under
-"Advanced mapping" above. So that for an SPI device, we have two states named
-"pos-A" and "pos-B".
+Typically runtime pinmuxing is done for runtime PM to mux a device RX pin
+to GPIO for wake-up events. And In some cases a shared RX/TX pin may need to
+be toggled. Sometimes pins can be also shared between two device drivers.
+
+In most cases runtime pinmuxing only is needed for some pins on a device, and
+most pins can be static. To avoid continuously having to check for all the
+pins of a device, pinctrl framework supports grouping device pin states to
+static and dynamic states.
+
+Most devices only need to use the static default state. If a devices has a
+need for runtime pinmuxing, the device must define the static default state,
+and then the optional dynamic states using the following rules:
+
+1. The default dynamic states are in addition to the static default state.
+
+2. The default dynamic states are active, sleep and idle, or active and idle.
+
+3. At least active and idle, or active and sleep states must be defined.
+
+4. Out of the dynamic pin states, only one of the active, sleep or idle states
+ can be active at a time.
+
+5. The dynamic pin states must all toggle the same pins to avoid unnecessary
+ checks during runtime.
+
+6. The pinctrl consumer driver must use pinctrl_select_dynamic() instead of
+ pinctrl_select_state() to toggle between the dynamic states.
+
+For example, to mux a serial driver RX pin to GPIO for runtime PM wake-up
+events, something like the following can be done. Most of the work is done
+by the pinctrl framework as long as the default, active and idle states are
+properly defined.
+
+#include
+
+serial_foo_probe()
+{
+ ...
+ if (!pinctrl_pm_is_idle_state_error())
+ /* Set driver specific flags for runtime PM */
+ ...
+}
+
+static int serial_foo_runtime_suspend(struct device *dev)
+{
+ ...
+ res = pinctrl_pm_select_idle_state(dev);
+ if (ret < 0)
+ return ret;
+ ...
+}
+
+static int serial_foo_runtime_resume(struct device *dev)
+{
+ ...
+ res = pinctrl_pm_select_active_state(dev);
+ if (ret < 0)
+ return ret;
+ ...
+}
+
+Currently only runtime muxing for runtime PM has been optimized as that can
+happen at high rate every time when idling and unidling devices. For less
+latency critical runtime remuxing it is possible to use pinctrl_select_state()
+as described below.
+
+For example, it is possible to move an SPI port from one set of pins to another
+set of pins. Say for example for spi0 in the example above, we expose two
+different groups of pins for the same function, but with different named in
+the mapping as described under "Advanced mapping" above. So that for an SPI
+device, we have two states named "pos-A" and "pos-B".
This snippet first muxes the function in the pins defined by group A, enables
it, disables and releases it, and muxes it in on the pins defined by group B:
diff --git
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Tony Lindgren [130718 00:31]:
> * Stephen Warren [130717 14:21]:
> > On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> > > +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> > > + struct pinctrl_state *st2)
> > > +{
> > > + struct pinctrl_setting *s1, *s2;
> > > +
> > > + list_for_each_entry(s1, >settings, node) {
> > ...
> > > + list_for_each_entry(s2, >settings, node) {
> > ...
> > > + if (pctldev1 != pctldev2) {
> > > + dev_dbg(dev, "pctldev must be the same for
> > > states\n");
> > > + return -EINVAL;
> > > + }
> >
> > I don't think we should require that; it's perfectly legal at the moment
> > for some device's pinctrl configuration to include settings from
> > multiple different pin controllers.
>
> Yes that's fine for pinctrl_select(), but let's not do that for
> runtime muxing.
Hmm reading this again, you're right, there should not be anything
preventing mixing multiple controllers as long as the states match.
> > > + for (i = 0; i < num_pins1; i++) {
> > > + int pin1 = pins1[i];
> > > +
> > > + for (j = 0; j < num_pins2; j++) {
> > > + int pin2 = pins2[j];
> > > +
> > > + if (pin1 == pin2) {
> > > + found++;
> > > + break;
> > > + }
> > > + }
> > > + }
> > > +
> > > + if (found != num_pins1) {
> >
> > I guess this make sure that every entry in the dynamic state is in the
> > state state, but not vice-versa; the static state can affect more stuff
> > than the dynamic state?
> >
> > If so, shouldn't that check be if (found != num_pins2)?
>
> The check is that idle_state and sleep_state pins must match the
> active_state pins. This is intentionally different from the current
> pinctrl_select() logic.
And here things will get messed up if the order of settings is diffrent..
So yes, pinctrl_check_dynamic() needs more work.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren [130717 14:30]: > On 07/16/2013 03:05 AM, Tony Lindgren wrote: > > To toggle dynamic states, let's add the optional active state in > > addition to the static default state. Then if the optional active > > state is defined, we can require that idle and sleep states cover > > the same pingroups as the active state. > > > > Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() > > to use instead of pinctrl_select() to avoid breaking existing users. > > > > With pinctrl_check_dynamic() we can check that idle and sleep states > > match the active state for pingroups during init, and don't need to > > do it during runtime. > > > > Then with the states pre-validated, pinctrl_select_dynamic() can > > just toggle between the dynamic states without extra checks. > > > > Note that pinctr_select_state() still has valid use cases, such as > > changing states when the pins can be shared between two drivers > > and don't necessarily cover the same pingroups. For dynamic runtime > > toggling of pin states, we should eventually always use just > > pinctrl_select_dynamic(). > > Something in this series should edit Documentation/pinctrl.txt to > explain the philosophy behind the static/dynamic state split. That > philosophy and/or semantics are more important to review than the code... Sure, I'll write up something on that. > Related to that, I'm worried that using pinctrl_select_state() and > pinctrl_select_dynamic() appear to be mutually-exclusive options here. Not currently, but eventually I think that's a good idea. We should use pinctrl_select_state() only during init time eventually because of the diffing of states it does. > Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime > PM? Does the mux setting select which states are used for runtime PM, or > does runtime PM override the basic mux setting, or must the pincrl-I2C > mux manually implement custom runtime-PM/pinctrl interaction since > there's no generic answer to those questions? How many more custom > exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The "default" state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren [130717 14:21]:
> On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> > To toggle dynamic states, let's add the optional active state in
> > addition to the static default state. Then if the optional active
> > state is defined, we can require that idle and sleep states cover
> > the same pingroups as the active state.
> >
> > Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
> > to use instead of pinctrl_select() to avoid breaking existing users.
> >
> > With pinctrl_check_dynamic() we can check that idle and sleep states
> > match the active state for pingroups during init, and don't need to
> > do it during runtime.
> >
> > Then with the states pre-validated, pinctrl_select_dynamic() can
> > just toggle between the dynamic states without extra checks.
> >
> > Note that pinctr_select_state() still has valid use cases, such as
> > changing states when the pins can be shared between two drivers
> > and don't necessarily cover the same pingroups. For dynamic runtime
> > toggling of pin states, we should eventually always use just
> > pinctrl_select_dynamic().
>
> > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>
> > @@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
> > list_for_each_entry_safe(setting, n2, >settings, node) {
> > pinctrl_free_setting(state == p->state[PINCTRL_STATIC],
> > setting);
> > + pinctrl_free_setting(state == p->state[PINCTRL_DYNAMIC],
> > +setting);
>
> This will cause pinmux_free_setting() or pinconf_free_setting() to be
> called twice on the setting object. Right now they don't do anything,
> but if they start to kfree() some dynamically-allocated data that's
> stored within the setting, that'll cause problems. I would suggest a
> loop body something more like:
>
> bool disable_setting = state == XXX || state == YYY;
> pinctrl_free_setting(disable_setting, setting);
OK good point, I'll check it.
> > +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> > + struct pinctrl_state *st2)
> > +{
> > + struct pinctrl_setting *s1, *s2;
> > +
> > + list_for_each_entry(s1, >settings, node) {
> ...
> > + list_for_each_entry(s2, >settings, node) {
> ...
> > + if (pctldev1 != pctldev2) {
> > + dev_dbg(dev, "pctldev must be the same for
> > states\n");
> > + return -EINVAL;
> > + }
>
> I don't think we should require that; it's perfectly legal at the moment
> for some device's pinctrl configuration to include settings from
> multiple different pin controllers.
Yes that's fine for pinctrl_select(), but let's not do that for
runtime muxing.
Here we want to do a one-time check during init to make sure that
if active_state is defined, then the idle_state and sleep_state
must match active_state for pins.
This way we can avoid checking things over and over again during
runtime like pinctrl_select() is currently doing.
> > + for (i = 0; i < num_pins1; i++) {
> > + int pin1 = pins1[i];
> > +
> > + for (j = 0; j < num_pins2; j++) {
> > + int pin2 = pins2[j];
> > +
> > + if (pin1 == pin2) {
> > + found++;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if (found != num_pins1) {
>
> I guess this make sure that every entry in the dynamic state is in the
> state state, but not vice-versa; the static state can affect more stuff
> than the dynamic state?
>
> If so, shouldn't that check be if (found != num_pins2)?
The check is that idle_state and sleep_state pins must match the
active_state pins. This is intentionally different from the current
pinctrl_select() logic.
> > +int pinctrl_select_dynamic(struct pinctrl *p, struct pinctrl_state *state)
>
> The body of this function is rather duplicated with pinctrl_select().
Well we may be able to make pinctrl_select() use this too, I'll take
a look. Initially I'd rather not start messing with pinctrl_select()
to avoid breaking existing users.
> > @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device
> > *dev, struct pinctrl_state *sta
> > return 0;
> > if (IS_ERR(state))
> > return 0; /* No such state */
> > - ret = pinctrl_select_state(pins->p, state);
> > +
> > + /* Configured for proper dynamic muxing? */
>
> I don't think "proper" is correct here; it's just fine not to have any
> dynamic configuration.
Right, that's the most common case. I'll drop the "proper".
> > + if (!IS_ERR(dev->pins->active_state))
> > +
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren swar...@wwwdotorg.org [130717 14:21]:
On 07/16/2013 03:05 AM, Tony Lindgren wrote:
To toggle dynamic states, let's add the optional active state in
addition to the static default state. Then if the optional active
state is defined, we can require that idle and sleep states cover
the same pingroups as the active state.
Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
to use instead of pinctrl_select() to avoid breaking existing users.
With pinctrl_check_dynamic() we can check that idle and sleep states
match the active state for pingroups during init, and don't need to
do it during runtime.
Then with the states pre-validated, pinctrl_select_dynamic() can
just toggle between the dynamic states without extra checks.
Note that pinctr_select_state() still has valid use cases, such as
changing states when the pins can be shared between two drivers
and don't necessarily cover the same pingroups. For dynamic runtime
toggling of pin states, we should eventually always use just
pinctrl_select_dynamic().
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
@@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
list_for_each_entry_safe(setting, n2, state-settings, node) {
pinctrl_free_setting(state == p-state[PINCTRL_STATIC],
setting);
+ pinctrl_free_setting(state == p-state[PINCTRL_DYNAMIC],
+setting);
This will cause pinmux_free_setting() or pinconf_free_setting() to be
called twice on the setting object. Right now they don't do anything,
but if they start to kfree() some dynamically-allocated data that's
stored within the setting, that'll cause problems. I would suggest a
loop body something more like:
bool disable_setting = state == XXX || state == YYY;
pinctrl_free_setting(disable_setting, setting);
OK good point, I'll check it.
+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+ struct pinctrl_state *st2)
+{
+ struct pinctrl_setting *s1, *s2;
+
+ list_for_each_entry(s1, st1-settings, node) {
...
+ list_for_each_entry(s2, st2-settings, node) {
...
+ if (pctldev1 != pctldev2) {
+ dev_dbg(dev, pctldev must be the same for
states\n);
+ return -EINVAL;
+ }
I don't think we should require that; it's perfectly legal at the moment
for some device's pinctrl configuration to include settings from
multiple different pin controllers.
Yes that's fine for pinctrl_select(), but let's not do that for
runtime muxing.
Here we want to do a one-time check during init to make sure that
if active_state is defined, then the idle_state and sleep_state
must match active_state for pins.
This way we can avoid checking things over and over again during
runtime like pinctrl_select() is currently doing.
+ for (i = 0; i num_pins1; i++) {
+ int pin1 = pins1[i];
+
+ for (j = 0; j num_pins2; j++) {
+ int pin2 = pins2[j];
+
+ if (pin1 == pin2) {
+ found++;
+ break;
+ }
+ }
+ }
+
+ if (found != num_pins1) {
I guess this make sure that every entry in the dynamic state is in the
state state, but not vice-versa; the static state can affect more stuff
than the dynamic state?
If so, shouldn't that check be if (found != num_pins2)?
The check is that idle_state and sleep_state pins must match the
active_state pins. This is intentionally different from the current
pinctrl_select() logic.
+int pinctrl_select_dynamic(struct pinctrl *p, struct pinctrl_state *state)
The body of this function is rather duplicated with pinctrl_select().
Well we may be able to make pinctrl_select() use this too, I'll take
a look. Initially I'd rather not start messing with pinctrl_select()
to avoid breaking existing users.
@@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device
*dev, struct pinctrl_state *sta
return 0;
if (IS_ERR(state))
return 0; /* No such state */
- ret = pinctrl_select_state(pins-p, state);
+
+ /* Configured for proper dynamic muxing? */
I don't think proper is correct here; it's just fine not to have any
dynamic configuration.
Right, that's the most common case. I'll drop the proper.
+ if (!IS_ERR(dev-pins-active_state))
+ ret = pinctrl_select_dynamic(pins-p, state);
+ else
+ ret = pinctrl_select_state(pins-p, state);
Hmmm. I'm not quite sure
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Stephen Warren swar...@wwwdotorg.org [130717 14:30]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state. Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users. With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime. Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks. Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic(). Something in this series should edit Documentation/pinctrl.txt to explain the philosophy behind the static/dynamic state split. That philosophy and/or semantics are more important to review than the code... Sure, I'll write up something on that. Related to that, I'm worried that using pinctrl_select_state() and pinctrl_select_dynamic() appear to be mutually-exclusive options here. Not currently, but eventually I think that's a good idea. We should use pinctrl_select_state() only during init time eventually because of the diffing of states it does. Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The default state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Tony Lindgren t...@atomide.com [130718 00:31]:
* Stephen Warren swar...@wwwdotorg.org [130717 14:21]:
On 07/16/2013 03:05 AM, Tony Lindgren wrote:
+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+ struct pinctrl_state *st2)
+{
+ struct pinctrl_setting *s1, *s2;
+
+ list_for_each_entry(s1, st1-settings, node) {
...
+ list_for_each_entry(s2, st2-settings, node) {
...
+ if (pctldev1 != pctldev2) {
+ dev_dbg(dev, pctldev must be the same for
states\n);
+ return -EINVAL;
+ }
I don't think we should require that; it's perfectly legal at the moment
for some device's pinctrl configuration to include settings from
multiple different pin controllers.
Yes that's fine for pinctrl_select(), but let's not do that for
runtime muxing.
Hmm reading this again, you're right, there should not be anything
preventing mixing multiple controllers as long as the states match.
+ for (i = 0; i num_pins1; i++) {
+ int pin1 = pins1[i];
+
+ for (j = 0; j num_pins2; j++) {
+ int pin2 = pins2[j];
+
+ if (pin1 == pin2) {
+ found++;
+ break;
+ }
+ }
+ }
+
+ if (found != num_pins1) {
I guess this make sure that every entry in the dynamic state is in the
state state, but not vice-versa; the static state can affect more stuff
than the dynamic state?
If so, shouldn't that check be if (found != num_pins2)?
The check is that idle_state and sleep_state pins must match the
active_state pins. This is intentionally different from the current
pinctrl_select() logic.
And here things will get messed up if the order of settings is diffrent..
So yes, pinctrl_check_dynamic() needs more work.
Regards,
Tony
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/4] pinctrl: Add support for additional dynamic states
To toggle dynamic states, let's add the optional active state in
addition to the static default state. Then if the optional active
state is defined, we can require that idle and sleep states cover
the same pingroups as the active state.
Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
to use instead of pinctrl_select() to avoid breaking existing users.
With pinctrl_check_dynamic() we can check that idle and sleep states
match the active state for pingroups during init, and don't need to
do it during runtime.
Then with the states pre-validated, pinctrl_select_dynamic() can
just toggle between the dynamic states without extra checks.
Note that pinctr_select_state() still has valid use cases, such as
changing states when the pins can be shared between two drivers
and don't necessarily cover the same pingroups. For dynamic runtime
toggling of pin states, we should eventually always use just
pinctrl_select_dynamic().
Cc: Felipe Balbi ba...@ti.com
Cc: Grygorii Strashko grygorii.stras...@ti.com
Cc: Stephen Warren swar...@wwwdotorg.org
Signed-off-by: Tony Lindgren t...@atomide.com
---
Documentation/pinctrl.txt | 77 ++-
drivers/pinctrl/core.c| 226 ++---
include/linux/pinctrl/consumer.h | 46 +++
include/linux/pinctrl/devinfo.h |4 +
include/linux/pinctrl/pinctrl-state.h | 15 ++
5 files changed, 342 insertions(+), 26 deletions(-)
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 052e13a..0477ec5 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -1283,12 +1283,77 @@ This gives the exact same result as the above
construction.
Runtime pinmuxing
=
-It is possible to mux a certain function in and out at runtime, say to move
-an SPI port from one set of pins to another set of pins. Say for example for
-spi0 in the example above, we expose two different groups of pins for the same
-function, but with different named in the mapping as described under
-Advanced mapping above. So that for an SPI device, we have two states named
-pos-A and pos-B.
+Typically runtime pinmuxing is done for runtime PM to mux a device RX pin
+to GPIO for wake-up events. And In some cases a shared RX/TX pin may need to
+be toggled. Sometimes pins can be also shared between two device drivers.
+
+In most cases runtime pinmuxing only is needed for some pins on a device, and
+most pins can be static. To avoid continuously having to check for all the
+pins of a device, pinctrl framework supports grouping device pin states to
+static and dynamic states.
+
+Most devices only need to use the static default state. If a devices has a
+need for runtime pinmuxing, the device must define the static default state,
+and then the optional dynamic states using the following rules:
+
+1. The default dynamic states are in addition to the static default state.
+
+2. The default dynamic states are active, sleep and idle, or active and idle.
+
+3. At least active and idle, or active and sleep states must be defined.
+
+4. Out of the dynamic pin states, only one of the active, sleep or idle states
+ can be active at a time.
+
+5. The dynamic pin states must all toggle the same pins to avoid unnecessary
+ checks during runtime.
+
+6. The pinctrl consumer driver must use pinctrl_select_dynamic() instead of
+ pinctrl_select_state() to toggle between the dynamic states.
+
+For example, to mux a serial driver RX pin to GPIO for runtime PM wake-up
+events, something like the following can be done. Most of the work is done
+by the pinctrl framework as long as the default, active and idle states are
+properly defined.
+
+#include linux/pinctrl/consumer.h
+
+serial_foo_probe()
+{
+ ...
+ if (!pinctrl_pm_is_idle_state_error())
+ /* Set driver specific flags for runtime PM */
+ ...
+}
+
+static int serial_foo_runtime_suspend(struct device *dev)
+{
+ ...
+ res = pinctrl_pm_select_idle_state(dev);
+ if (ret 0)
+ return ret;
+ ...
+}
+
+static int serial_foo_runtime_resume(struct device *dev)
+{
+ ...
+ res = pinctrl_pm_select_active_state(dev);
+ if (ret 0)
+ return ret;
+ ...
+}
+
+Currently only runtime muxing for runtime PM has been optimized as that can
+happen at high rate every time when idling and unidling devices. For less
+latency critical runtime remuxing it is possible to use pinctrl_select_state()
+as described below.
+
+For example, it is possible to move an SPI port from one set of pins to another
+set of pins. Say for example for spi0 in the example above, we expose two
+different groups of pins for the same function, but with different named in
+the mapping as described under Advanced mapping above. So that for an SPI
+device, we have two states named pos-A and pos-B.
This snippet first muxes the function in the pins defined by group A, enables
it, disables and
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/18/2013 01:25 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:21]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state. Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users. With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime. Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks. Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic(). @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta return 0; if (IS_ERR(state)) return 0; /* No such state */ - ret = pinctrl_select_state(pins-p, state); + + /* Configured for proper dynamic muxing? */ + if (!IS_ERR(dev-pins-active_state)) + ret = pinctrl_select_dynamic(pins-p, state); + else + ret = pinctrl_select_state(pins-p, state); Hmmm. I'm not quite sure this is right... Surely this function should just do nothing if the dynamic states aren't defined? The system should just, and only, be in the default state and never do anything else. This keeps the existing behaviour. We should be able to drop the call to pinctrl_select_state() here after the existing use in drivers has been fixed. How many DT-based systems already have multiple of default/idle/sleep states defined in DT? Right now, since the kernel code uses pinctrl_select_state() to switch between those, the state definitions need to define /all/ pins used by those states, not just the dynamic ones. However, with this series in place, the default state should only include the static pins, and the active/idle/sleep states should only include the dynamic pins. That's a change to the DT bindings, since it changes how the DT must be written, and causes older DTs to be incompatible with newer kernels and vice-versa. So, do we just ignore this DT ABI change again, or insist on doing it in some compatible way? DT ABI-ness is a PITA:-( -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/18/2013 01:36 AM, Tony Lindgren wrote: * Stephen Warren swar...@wwwdotorg.org [130717 14:30]: On 07/16/2013 03:05 AM, Tony Lindgren wrote: ... Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? The idea is that runtime PM will never touch the basic mux settings at all. The default state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device. For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init. So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM. So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses: a) bus 1: I2C controller is muxed out onto one set of pins. b) bus 2: I2C controller is muxed out onto another set of pins. Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to the active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as active... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/16/2013 03:05 AM, Tony Lindgren wrote: > To toggle dynamic states, let's add the optional active state in > addition to the static default state. Then if the optional active > state is defined, we can require that idle and sleep states cover > the same pingroups as the active state. > > Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() > to use instead of pinctrl_select() to avoid breaking existing users. > > With pinctrl_check_dynamic() we can check that idle and sleep states > match the active state for pingroups during init, and don't need to > do it during runtime. > > Then with the states pre-validated, pinctrl_select_dynamic() can > just toggle between the dynamic states without extra checks. > > Note that pinctr_select_state() still has valid use cases, such as > changing states when the pins can be shared between two drivers > and don't necessarily cover the same pingroups. For dynamic runtime > toggling of pin states, we should eventually always use just > pinctrl_select_dynamic(). Something in this series should edit Documentation/pinctrl.txt to explain the philosophy behind the static/dynamic state split. That philosophy and/or semantics are more important to review than the code... Related to that, I'm worried that using pinctrl_select_state() and pinctrl_select_dynamic() appear to be mutually-exclusive options here. Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/16/2013 03:05 AM, Tony Lindgren wrote:
> To toggle dynamic states, let's add the optional active state in
> addition to the static default state. Then if the optional active
> state is defined, we can require that idle and sleep states cover
> the same pingroups as the active state.
>
> Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
> to use instead of pinctrl_select() to avoid breaking existing users.
>
> With pinctrl_check_dynamic() we can check that idle and sleep states
> match the active state for pingroups during init, and don't need to
> do it during runtime.
>
> Then with the states pre-validated, pinctrl_select_dynamic() can
> just toggle between the dynamic states without extra checks.
>
> Note that pinctr_select_state() still has valid use cases, such as
> changing states when the pins can be shared between two drivers
> and don't necessarily cover the same pingroups. For dynamic runtime
> toggling of pin states, we should eventually always use just
> pinctrl_select_dynamic().
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> @@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
> list_for_each_entry_safe(setting, n2, >settings, node) {
> pinctrl_free_setting(state == p->state[PINCTRL_STATIC],
>setting);
> + pinctrl_free_setting(state == p->state[PINCTRL_DYNAMIC],
> + setting);
This will cause pinmux_free_setting() or pinconf_free_setting() to be
called twice on the setting object. Right now they don't do anything,
but if they start to kfree() some dynamically-allocated data that's
stored within the setting, that'll cause problems. I would suggest a
loop body something more like:
bool disable_setting = state == XXX || state == YYY;
pinctrl_free_setting(disable_setting, setting);
> +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> + struct pinctrl_state *st2)
> +{
> + struct pinctrl_setting *s1, *s2;
> +
> + list_for_each_entry(s1, >settings, node) {
...
> + list_for_each_entry(s2, >settings, node) {
...
> + if (pctldev1 != pctldev2) {
> + dev_dbg(dev, "pctldev must be the same for
> states\n");
> + return -EINVAL;
> + }
I don't think we should require that; it's perfectly legal at the moment
for some device's pinctrl configuration to include settings from
multiple different pin controllers.
> + for (i = 0; i < num_pins1; i++) {
> + int pin1 = pins1[i];
> +
> + for (j = 0; j < num_pins2; j++) {
> + int pin2 = pins2[j];
> +
> + if (pin1 == pin2) {
> + found++;
> + break;
> + }
> + }
> + }
> +
> + if (found != num_pins1) {
I guess this make sure that every entry in the dynamic state is in the
state state, but not vice-versa; the static state can affect more stuff
than the dynamic state?
If so, shouldn't that check be if (found != num_pins2)?
> +int pinctrl_select_dynamic(struct pinctrl *p, struct pinctrl_state *state)
The body of this function is rather duplicated with pinctrl_select().
> @@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev,
> struct pinctrl_state *sta
> return 0;
> if (IS_ERR(state))
> return 0; /* No such state */
> - ret = pinctrl_select_state(pins->p, state);
> +
> + /* Configured for proper dynamic muxing? */
I don't think "proper" is correct here; it's just fine not to have any
dynamic configuration.
> + if (!IS_ERR(dev->pins->active_state))
> + ret = pinctrl_select_dynamic(pins->p, state);
> + else
> + ret = pinctrl_select_state(pins->p, state);
Hmmm. I'm not quite sure this is right... Surely this function should
just do nothing if the dynamic states aren't defined? The system should
just, and only, be in the "default" state and never do anything else.
Looking back at patch 1, I see the are
pinctrl_pm_select_{default,sleep,idle}_state(). Shouldn't that list be
active/sleep/idle, since I assume default is that "static" state, and
the other three are the "dynamic" states?
> +static int pinctrl_pm_check_state(struct device *dev,
> + struct pinctrl_state *state)
Naming this pinctrl_pm_is_state_error() or pinctrl_pm_is_state_ok()
might give more clue what its return value is expected to be.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/16/2013 03:05 AM, Tony Lindgren wrote:
To toggle dynamic states, let's add the optional active state in
addition to the static default state. Then if the optional active
state is defined, we can require that idle and sleep states cover
the same pingroups as the active state.
Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
to use instead of pinctrl_select() to avoid breaking existing users.
With pinctrl_check_dynamic() we can check that idle and sleep states
match the active state for pingroups during init, and don't need to
do it during runtime.
Then with the states pre-validated, pinctrl_select_dynamic() can
just toggle between the dynamic states without extra checks.
Note that pinctr_select_state() still has valid use cases, such as
changing states when the pins can be shared between two drivers
and don't necessarily cover the same pingroups. For dynamic runtime
toggling of pin states, we should eventually always use just
pinctrl_select_dynamic().
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
@@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
list_for_each_entry_safe(setting, n2, state-settings, node) {
pinctrl_free_setting(state == p-state[PINCTRL_STATIC],
setting);
+ pinctrl_free_setting(state == p-state[PINCTRL_DYNAMIC],
+ setting);
This will cause pinmux_free_setting() or pinconf_free_setting() to be
called twice on the setting object. Right now they don't do anything,
but if they start to kfree() some dynamically-allocated data that's
stored within the setting, that'll cause problems. I would suggest a
loop body something more like:
bool disable_setting = state == XXX || state == YYY;
pinctrl_free_setting(disable_setting, setting);
+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+ struct pinctrl_state *st2)
+{
+ struct pinctrl_setting *s1, *s2;
+
+ list_for_each_entry(s1, st1-settings, node) {
...
+ list_for_each_entry(s2, st2-settings, node) {
...
+ if (pctldev1 != pctldev2) {
+ dev_dbg(dev, pctldev must be the same for
states\n);
+ return -EINVAL;
+ }
I don't think we should require that; it's perfectly legal at the moment
for some device's pinctrl configuration to include settings from
multiple different pin controllers.
+ for (i = 0; i num_pins1; i++) {
+ int pin1 = pins1[i];
+
+ for (j = 0; j num_pins2; j++) {
+ int pin2 = pins2[j];
+
+ if (pin1 == pin2) {
+ found++;
+ break;
+ }
+ }
+ }
+
+ if (found != num_pins1) {
I guess this make sure that every entry in the dynamic state is in the
state state, but not vice-versa; the static state can affect more stuff
than the dynamic state?
If so, shouldn't that check be if (found != num_pins2)?
+int pinctrl_select_dynamic(struct pinctrl *p, struct pinctrl_state *state)
The body of this function is rather duplicated with pinctrl_select().
@@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev,
struct pinctrl_state *sta
return 0;
if (IS_ERR(state))
return 0; /* No such state */
- ret = pinctrl_select_state(pins-p, state);
+
+ /* Configured for proper dynamic muxing? */
I don't think proper is correct here; it's just fine not to have any
dynamic configuration.
+ if (!IS_ERR(dev-pins-active_state))
+ ret = pinctrl_select_dynamic(pins-p, state);
+ else
+ ret = pinctrl_select_state(pins-p, state);
Hmmm. I'm not quite sure this is right... Surely this function should
just do nothing if the dynamic states aren't defined? The system should
just, and only, be in the default state and never do anything else.
Looking back at patch 1, I see the are
pinctrl_pm_select_{default,sleep,idle}_state(). Shouldn't that list be
active/sleep/idle, since I assume default is that static state, and
the other three are the dynamic states?
+static int pinctrl_pm_check_state(struct device *dev,
+ struct pinctrl_state *state)
Naming this pinctrl_pm_is_state_error() or pinctrl_pm_is_state_ok()
might give more clue what its return value is expected to be.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
On 07/16/2013 03:05 AM, Tony Lindgren wrote: To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state. Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users. With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime. Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks. Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic(). Something in this series should edit Documentation/pinctrl.txt to explain the philosophy behind the static/dynamic state split. That philosophy and/or semantics are more important to review than the code... Related to that, I'm worried that using pinctrl_select_state() and pinctrl_select_dynamic() appear to be mutually-exclusive options here. Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Felipe Balbi [130716 02:42]:
> Hi,
>
> On Tue, Jul 16, 2013 at 02:05:36AM -0700, Tony Lindgren wrote:
> > +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> > + struct pinctrl_state *st2)
> > +{
> > + struct pinctrl_setting *s1, *s2;
> > +
> > + list_for_each_entry(s1, >settings, node) {
> > + struct pinctrl_dev *pctldev1;
> > + const struct pinctrl_ops *pctlops1;
> > + const unsigned *pins1;
> > + unsigned num_pins1;
> > + int res;
> > +
> > + if (s1->type != PIN_MAP_TYPE_MUX_GROUP)
> > + continue;
> > +
> > + pctldev1 = s1->pctldev;
> > + pctlops1 = pctldev1->desc->pctlops;
> > + res = pctlops1->get_group_pins(pctldev1, s1->data.mux.group,
> > + , _pins1);
> > + if (res) {
> > + dev_dbg(dev, "could not get state1 group pins\n");
> > + return -EINVAL;
> > + }
> > +
> > + list_for_each_entry(s2, >settings, node) {
> > + struct pinctrl_dev *pctldev2;
> > + const struct pinctrl_ops *pctlops2;
> > + const unsigned *pins2;
> > + unsigned num_pins2;
> > + int i, j, found = 0;
> > +
> > + if (s2->type != PIN_MAP_TYPE_MUX_GROUP)
> > + continue;
> > +
> > + pctldev2 = s2->pctldev;
> > + if (pctldev1 != pctldev2) {
> > + dev_dbg(dev, "pctldev must be the same for
> > states\n");
> > + return -EINVAL;
> > + }
> > + pctlops2 = pctldev2->desc->pctlops;
> > + res = pctlops2->get_group_pins(pctldev2,
> > + s2->data.mux.group,
> > + , _pins2);
> > + if (res) {
> > + dev_dbg(dev, "could not get state2 group
> > pins\n");
> > + return -EINVAL;
> > + }
> > +
> > + for (i = 0; i < num_pins1; i++) {
> > + int pin1 = pins1[i];
> > +
> > + for (j = 0; j < num_pins2; j++) {
> > + int pin2 = pins2[j];
> > +
> > + if (pin1 == pin2) {
> > + found++;
> > + break;
> > + }
> > + }
> > + }
>
> 4 levels of nested loops ? Isn't this way too much ? OTOH, it points to
> the fact that, perhaps, a list isn't the best data structure for
> pinctrl ??
This check is only done during init to avoid constantly diffing the
pins during runtime like we currently do. And it's only done for the
pins that need to change during runtime for wake-up events etc. So
that's typically few to ten pins per device that need to be checked.
I agree there are things to improve for the data structures,
but that's a different patch set.
> Or perhaps you could just assume that if num_pins1 == num_pins2 it's
> enough ? But that will, likely, leave some uncovered corners...
Yes I considered that, but checking for the number of pins is not
enough. It's best to check them properly during init.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
Hi,
On Tue, Jul 16, 2013 at 02:05:36AM -0700, Tony Lindgren wrote:
> +int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
> + struct pinctrl_state *st2)
> +{
> + struct pinctrl_setting *s1, *s2;
> +
> + list_for_each_entry(s1, >settings, node) {
> + struct pinctrl_dev *pctldev1;
> + const struct pinctrl_ops *pctlops1;
> + const unsigned *pins1;
> + unsigned num_pins1;
> + int res;
> +
> + if (s1->type != PIN_MAP_TYPE_MUX_GROUP)
> + continue;
> +
> + pctldev1 = s1->pctldev;
> + pctlops1 = pctldev1->desc->pctlops;
> + res = pctlops1->get_group_pins(pctldev1, s1->data.mux.group,
> +, _pins1);
> + if (res) {
> + dev_dbg(dev, "could not get state1 group pins\n");
> + return -EINVAL;
> + }
> +
> + list_for_each_entry(s2, >settings, node) {
> + struct pinctrl_dev *pctldev2;
> + const struct pinctrl_ops *pctlops2;
> + const unsigned *pins2;
> + unsigned num_pins2;
> + int i, j, found = 0;
> +
> + if (s2->type != PIN_MAP_TYPE_MUX_GROUP)
> + continue;
> +
> + pctldev2 = s2->pctldev;
> + if (pctldev1 != pctldev2) {
> + dev_dbg(dev, "pctldev must be the same for
> states\n");
> + return -EINVAL;
> + }
> + pctlops2 = pctldev2->desc->pctlops;
> + res = pctlops2->get_group_pins(pctldev2,
> +s2->data.mux.group,
> +, _pins2);
> + if (res) {
> + dev_dbg(dev, "could not get state2 group
> pins\n");
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < num_pins1; i++) {
> + int pin1 = pins1[i];
> +
> + for (j = 0; j < num_pins2; j++) {
> + int pin2 = pins2[j];
> +
> + if (pin1 == pin2) {
> + found++;
> + break;
> + }
> + }
> + }
4 levels of nested loops ? Isn't this way too much ? OTOH, it points to
the fact that, perhaps, a list isn't the best data structure for
pinctrl ??
Or perhaps you could just assume that if num_pins1 == num_pins2 it's
enough ? But that will, likely, leave some uncovered corners...
--
balbi
signature.asc
Description: Digital signature
[PATCH 3/4] pinctrl: Add support for additional dynamic states
To toggle dynamic states, let's add the optional active state in
addition to the static default state. Then if the optional active
state is defined, we can require that idle and sleep states cover
the same pingroups as the active state.
Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
to use instead of pinctrl_select() to avoid breaking existing users.
With pinctrl_check_dynamic() we can check that idle and sleep states
match the active state for pingroups during init, and don't need to
do it during runtime.
Then with the states pre-validated, pinctrl_select_dynamic() can
just toggle between the dynamic states without extra checks.
Note that pinctr_select_state() still has valid use cases, such as
changing states when the pins can be shared between two drivers
and don't necessarily cover the same pingroups. For dynamic runtime
toggling of pin states, we should eventually always use just
pinctrl_select_dynamic().
Cc: Stephen Warren
Signed-off-by: Tony Lindgren
---
drivers/pinctrl/core.c| 189 -
include/linux/pinctrl/consumer.h | 46
include/linux/pinctrl/devinfo.h |4 +
include/linux/pinctrl/pinctrl-state.h | 15 ++-
4 files changed, 249 insertions(+), 5 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 8da11d5..4f58a97 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
list_for_each_entry_safe(setting, n2, >settings, node) {
pinctrl_free_setting(state == p->state[PINCTRL_STATIC],
setting);
+ pinctrl_free_setting(state == p->state[PINCTRL_DYNAMIC],
+setting);
list_del(>node);
kfree(setting);
}
@@ -1041,6 +1043,134 @@ unapply_new_state:
}
EXPORT_SYMBOL_GPL(pinctrl_select_state);
+/**
+ * pinctrl_check_dynamic() - compare two states for the pins
+ * @dev: pinctrl consumer device pointer
+ * @st1: state handle
+ * @st2: state handle
+ *
+ * This function checks that the group pins match between the two
+ * states to avoid runtime checking. Use this to check dynamic pin
+ * states during init.
+ */
+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+ struct pinctrl_state *st2)
+{
+ struct pinctrl_setting *s1, *s2;
+
+ list_for_each_entry(s1, >settings, node) {
+ struct pinctrl_dev *pctldev1;
+ const struct pinctrl_ops *pctlops1;
+ const unsigned *pins1;
+ unsigned num_pins1;
+ int res;
+
+ if (s1->type != PIN_MAP_TYPE_MUX_GROUP)
+ continue;
+
+ pctldev1 = s1->pctldev;
+ pctlops1 = pctldev1->desc->pctlops;
+ res = pctlops1->get_group_pins(pctldev1, s1->data.mux.group,
+ , _pins1);
+ if (res) {
+ dev_dbg(dev, "could not get state1 group pins\n");
+ return -EINVAL;
+ }
+
+ list_for_each_entry(s2, >settings, node) {
+ struct pinctrl_dev *pctldev2;
+ const struct pinctrl_ops *pctlops2;
+ const unsigned *pins2;
+ unsigned num_pins2;
+ int i, j, found = 0;
+
+ if (s2->type != PIN_MAP_TYPE_MUX_GROUP)
+ continue;
+
+ pctldev2 = s2->pctldev;
+ if (pctldev1 != pctldev2) {
+ dev_dbg(dev, "pctldev must be the same for
states\n");
+ return -EINVAL;
+ }
+ pctlops2 = pctldev2->desc->pctlops;
+ res = pctlops2->get_group_pins(pctldev2,
+ s2->data.mux.group,
+ , _pins2);
+ if (res) {
+ dev_dbg(dev, "could not get state2 group
pins\n");
+ return -EINVAL;
+ }
+
+ for (i = 0; i < num_pins1; i++) {
+ int pin1 = pins1[i];
+
+ for (j = 0; j < num_pins2; j++) {
+ int pin2 = pins2[j];
+
+ if (pin1 == pin2) {
+ found++;
+ break;
+ }
+ }
+ }
+
+ if (found != num_pins1) {
+
[PATCH 3/4] pinctrl: Add support for additional dynamic states
To toggle dynamic states, let's add the optional active state in
addition to the static default state. Then if the optional active
state is defined, we can require that idle and sleep states cover
the same pingroups as the active state.
Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic()
to use instead of pinctrl_select() to avoid breaking existing users.
With pinctrl_check_dynamic() we can check that idle and sleep states
match the active state for pingroups during init, and don't need to
do it during runtime.
Then with the states pre-validated, pinctrl_select_dynamic() can
just toggle between the dynamic states without extra checks.
Note that pinctr_select_state() still has valid use cases, such as
changing states when the pins can be shared between two drivers
and don't necessarily cover the same pingroups. For dynamic runtime
toggling of pin states, we should eventually always use just
pinctrl_select_dynamic().
Cc: Stephen Warren swar...@wwwdotorg.org
Signed-off-by: Tony Lindgren t...@atomide.com
---
drivers/pinctrl/core.c| 189 -
include/linux/pinctrl/consumer.h | 46
include/linux/pinctrl/devinfo.h |4 +
include/linux/pinctrl/pinctrl-state.h | 15 ++-
4 files changed, 249 insertions(+), 5 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 8da11d5..4f58a97 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
list_for_each_entry_safe(setting, n2, state-settings, node) {
pinctrl_free_setting(state == p-state[PINCTRL_STATIC],
setting);
+ pinctrl_free_setting(state == p-state[PINCTRL_DYNAMIC],
+setting);
list_del(setting-node);
kfree(setting);
}
@@ -1041,6 +1043,134 @@ unapply_new_state:
}
EXPORT_SYMBOL_GPL(pinctrl_select_state);
+/**
+ * pinctrl_check_dynamic() - compare two states for the pins
+ * @dev: pinctrl consumer device pointer
+ * @st1: state handle
+ * @st2: state handle
+ *
+ * This function checks that the group pins match between the two
+ * states to avoid runtime checking. Use this to check dynamic pin
+ * states during init.
+ */
+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+ struct pinctrl_state *st2)
+{
+ struct pinctrl_setting *s1, *s2;
+
+ list_for_each_entry(s1, st1-settings, node) {
+ struct pinctrl_dev *pctldev1;
+ const struct pinctrl_ops *pctlops1;
+ const unsigned *pins1;
+ unsigned num_pins1;
+ int res;
+
+ if (s1-type != PIN_MAP_TYPE_MUX_GROUP)
+ continue;
+
+ pctldev1 = s1-pctldev;
+ pctlops1 = pctldev1-desc-pctlops;
+ res = pctlops1-get_group_pins(pctldev1, s1-data.mux.group,
+ pins1, num_pins1);
+ if (res) {
+ dev_dbg(dev, could not get state1 group pins\n);
+ return -EINVAL;
+ }
+
+ list_for_each_entry(s2, st2-settings, node) {
+ struct pinctrl_dev *pctldev2;
+ const struct pinctrl_ops *pctlops2;
+ const unsigned *pins2;
+ unsigned num_pins2;
+ int i, j, found = 0;
+
+ if (s2-type != PIN_MAP_TYPE_MUX_GROUP)
+ continue;
+
+ pctldev2 = s2-pctldev;
+ if (pctldev1 != pctldev2) {
+ dev_dbg(dev, pctldev must be the same for
states\n);
+ return -EINVAL;
+ }
+ pctlops2 = pctldev2-desc-pctlops;
+ res = pctlops2-get_group_pins(pctldev2,
+ s2-data.mux.group,
+ pins2, num_pins2);
+ if (res) {
+ dev_dbg(dev, could not get state2 group
pins\n);
+ return -EINVAL;
+ }
+
+ for (i = 0; i num_pins1; i++) {
+ int pin1 = pins1[i];
+
+ for (j = 0; j num_pins2; j++) {
+ int pin2 = pins2[j];
+
+ if (pin1 == pin2) {
+ found++;
+ break;
+ }
+ }
+ }
+
+
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
Hi,
On Tue, Jul 16, 2013 at 02:05:36AM -0700, Tony Lindgren wrote:
+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+ struct pinctrl_state *st2)
+{
+ struct pinctrl_setting *s1, *s2;
+
+ list_for_each_entry(s1, st1-settings, node) {
+ struct pinctrl_dev *pctldev1;
+ const struct pinctrl_ops *pctlops1;
+ const unsigned *pins1;
+ unsigned num_pins1;
+ int res;
+
+ if (s1-type != PIN_MAP_TYPE_MUX_GROUP)
+ continue;
+
+ pctldev1 = s1-pctldev;
+ pctlops1 = pctldev1-desc-pctlops;
+ res = pctlops1-get_group_pins(pctldev1, s1-data.mux.group,
+pins1, num_pins1);
+ if (res) {
+ dev_dbg(dev, could not get state1 group pins\n);
+ return -EINVAL;
+ }
+
+ list_for_each_entry(s2, st2-settings, node) {
+ struct pinctrl_dev *pctldev2;
+ const struct pinctrl_ops *pctlops2;
+ const unsigned *pins2;
+ unsigned num_pins2;
+ int i, j, found = 0;
+
+ if (s2-type != PIN_MAP_TYPE_MUX_GROUP)
+ continue;
+
+ pctldev2 = s2-pctldev;
+ if (pctldev1 != pctldev2) {
+ dev_dbg(dev, pctldev must be the same for
states\n);
+ return -EINVAL;
+ }
+ pctlops2 = pctldev2-desc-pctlops;
+ res = pctlops2-get_group_pins(pctldev2,
+s2-data.mux.group,
+pins2, num_pins2);
+ if (res) {
+ dev_dbg(dev, could not get state2 group
pins\n);
+ return -EINVAL;
+ }
+
+ for (i = 0; i num_pins1; i++) {
+ int pin1 = pins1[i];
+
+ for (j = 0; j num_pins2; j++) {
+ int pin2 = pins2[j];
+
+ if (pin1 == pin2) {
+ found++;
+ break;
+ }
+ }
+ }
4 levels of nested loops ? Isn't this way too much ? OTOH, it points to
the fact that, perhaps, a list isn't the best data structure for
pinctrl ??
Or perhaps you could just assume that if num_pins1 == num_pins2 it's
enough ? But that will, likely, leave some uncovered corners...
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 3/4] pinctrl: Add support for additional dynamic states
* Felipe Balbi ba...@ti.com [130716 02:42]:
Hi,
On Tue, Jul 16, 2013 at 02:05:36AM -0700, Tony Lindgren wrote:
+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+ struct pinctrl_state *st2)
+{
+ struct pinctrl_setting *s1, *s2;
+
+ list_for_each_entry(s1, st1-settings, node) {
+ struct pinctrl_dev *pctldev1;
+ const struct pinctrl_ops *pctlops1;
+ const unsigned *pins1;
+ unsigned num_pins1;
+ int res;
+
+ if (s1-type != PIN_MAP_TYPE_MUX_GROUP)
+ continue;
+
+ pctldev1 = s1-pctldev;
+ pctlops1 = pctldev1-desc-pctlops;
+ res = pctlops1-get_group_pins(pctldev1, s1-data.mux.group,
+ pins1, num_pins1);
+ if (res) {
+ dev_dbg(dev, could not get state1 group pins\n);
+ return -EINVAL;
+ }
+
+ list_for_each_entry(s2, st2-settings, node) {
+ struct pinctrl_dev *pctldev2;
+ const struct pinctrl_ops *pctlops2;
+ const unsigned *pins2;
+ unsigned num_pins2;
+ int i, j, found = 0;
+
+ if (s2-type != PIN_MAP_TYPE_MUX_GROUP)
+ continue;
+
+ pctldev2 = s2-pctldev;
+ if (pctldev1 != pctldev2) {
+ dev_dbg(dev, pctldev must be the same for
states\n);
+ return -EINVAL;
+ }
+ pctlops2 = pctldev2-desc-pctlops;
+ res = pctlops2-get_group_pins(pctldev2,
+ s2-data.mux.group,
+ pins2, num_pins2);
+ if (res) {
+ dev_dbg(dev, could not get state2 group
pins\n);
+ return -EINVAL;
+ }
+
+ for (i = 0; i num_pins1; i++) {
+ int pin1 = pins1[i];
+
+ for (j = 0; j num_pins2; j++) {
+ int pin2 = pins2[j];
+
+ if (pin1 == pin2) {
+ found++;
+ break;
+ }
+ }
+ }
4 levels of nested loops ? Isn't this way too much ? OTOH, it points to
the fact that, perhaps, a list isn't the best data structure for
pinctrl ??
This check is only done during init to avoid constantly diffing the
pins during runtime like we currently do. And it's only done for the
pins that need to change during runtime for wake-up events etc. So
that's typically few to ten pins per device that need to be checked.
I agree there are things to improve for the data structures,
but that's a different patch set.
Or perhaps you could just assume that if num_pins1 == num_pins2 it's
enough ? But that will, likely, leave some uncovered corners...
Yes I considered that, but checking for the number of pins is not
enough. It's best to check them properly during init.
Regards,
Tony
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
