Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Rafael J. Wysocki[171108 00:47]: > On Wednesday, November 8, 2017 1:28:24 AM CET Florian Fainelli wrote: > > On 11/07/2017 04:23 PM, Rafael J. Wysocki wrote: > > > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > > >> * Florian Fainelli [171104 17:21]: > > >>> > > >>> > > >>> On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > > > * Florian Fainelli [171103 17:04]: > > >> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > >> The pinctrl provider is losing its state, hence these two patches. > > > > > > OK > > > > > >>> Anyways, the context lost flag should be managed in the PM core for > > >>> the device, so adding linux-pm and Rafael to Cc. > > >> > > >> I don't think it's that simple but sure, why not. > > > > > > Just having bool context_lost in struct dev_pm_info would probably > > > be enough to allow drivers to deal with it. This flag could then > > > be set for a device by power domain related code that knows if > > > context got lost. > > > > Something like: if the driver sees "context_lost" set, it should > > restore > > the context to the device from memory? > > >>> > > >>> That is what is being proposed here, except that the actual mechanism > > >>> where this matters needs to be in the core pinctrl code, otherwise the > > >>> state (context) is not restored due to a check that attempts not to > > >>> (re)apply a previous state. > > >>> > > > > But the it would also need to save the context beforehand, so why not > > to > > restore it unconditionally on resume? > > >>> > > >>> That's what my original attempts did here: > > >>> > > >>> https://patchwork.kernel.org/patch/9598969/ > > >>> > > >>> but Linus rightfully requested this to be done differently, hence this > > >>> attempt now to solve it in a slightly more flexible way based on DT > > >>> properties. > > >> > > >> For runtime PM, restoring the state constantly is unnecessary and not > > >> good for battery life. The logic can be just: > > >> > > >> 1. Device driver runtime PM suspend saves the state when needed > > >> > > >> 2. Device driver runtime PM resume checks if context_lost was set by > > >>the bus or power domain code > > >> > > >> 3. If context was lost, device driver restores the state, or in some > > >>cases may need re-run the driver register init related parts > > >>to bring the driver back up, then clears the context_lost flag > > >> > > >> How about something like the following patch? So far only compile > > >> tested with CONFIG_PM enabled. If that looks like the way to go, > > >> I'll test it properly and add some comments for the functions and > > >> post a proper patch :) > > > > > > Honestly, I'm not sure. > > > > > > I'd rather have a context_lost flag to start with and see how/if > > > drivers will use that before adding any common infra for handling > > > this. > > > > I am afraid we are being slightly side tracked here on this context_loss > > flag, because the crux of the problem is not whether a driver knows or > > not when it loses state, it's more than the pinctrl core refuses to > > re-apply the same state upon resumption even when the consumer driver > > tells it to, because it has not seen a transition (consider it as a > > stale software cache of the state), and there is only one state defined. > > > > I don't particularly care how its gets solved, at the generic device > > driver model or at the pinctrl level, but I think the pinctrl code needs > > to change in that regard no matter what we do, because right now, if you > > call pinctrl_select_state() in your driver's resume function, and there > > is only one state defined, nothing happens, that's a problem. > > I see, thanks for explaining that clearly. > > Then it looks like that there needs to be a way for the "cached" state to be > invalidated and the question is what that way should be and how it is going to > be triggered. Yeah seems like that issue needs to be solved at the pinctrl level even if the drivers were using a generic state for the context lost for figuring out when to do it. Regards, Tony
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Rafael J. Wysocki [171108 00:47]: > On Wednesday, November 8, 2017 1:28:24 AM CET Florian Fainelli wrote: > > On 11/07/2017 04:23 PM, Rafael J. Wysocki wrote: > > > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > > >> * Florian Fainelli [171104 17:21]: > > >>> > > >>> > > >>> On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > > > * Florian Fainelli [171103 17:04]: > > >> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > >> The pinctrl provider is losing its state, hence these two patches. > > > > > > OK > > > > > >>> Anyways, the context lost flag should be managed in the PM core for > > >>> the device, so adding linux-pm and Rafael to Cc. > > >> > > >> I don't think it's that simple but sure, why not. > > > > > > Just having bool context_lost in struct dev_pm_info would probably > > > be enough to allow drivers to deal with it. This flag could then > > > be set for a device by power domain related code that knows if > > > context got lost. > > > > Something like: if the driver sees "context_lost" set, it should > > restore > > the context to the device from memory? > > >>> > > >>> That is what is being proposed here, except that the actual mechanism > > >>> where this matters needs to be in the core pinctrl code, otherwise the > > >>> state (context) is not restored due to a check that attempts not to > > >>> (re)apply a previous state. > > >>> > > > > But the it would also need to save the context beforehand, so why not > > to > > restore it unconditionally on resume? > > >>> > > >>> That's what my original attempts did here: > > >>> > > >>> https://patchwork.kernel.org/patch/9598969/ > > >>> > > >>> but Linus rightfully requested this to be done differently, hence this > > >>> attempt now to solve it in a slightly more flexible way based on DT > > >>> properties. > > >> > > >> For runtime PM, restoring the state constantly is unnecessary and not > > >> good for battery life. The logic can be just: > > >> > > >> 1. Device driver runtime PM suspend saves the state when needed > > >> > > >> 2. Device driver runtime PM resume checks if context_lost was set by > > >>the bus or power domain code > > >> > > >> 3. If context was lost, device driver restores the state, or in some > > >>cases may need re-run the driver register init related parts > > >>to bring the driver back up, then clears the context_lost flag > > >> > > >> How about something like the following patch? So far only compile > > >> tested with CONFIG_PM enabled. If that looks like the way to go, > > >> I'll test it properly and add some comments for the functions and > > >> post a proper patch :) > > > > > > Honestly, I'm not sure. > > > > > > I'd rather have a context_lost flag to start with and see how/if > > > drivers will use that before adding any common infra for handling > > > this. > > > > I am afraid we are being slightly side tracked here on this context_loss > > flag, because the crux of the problem is not whether a driver knows or > > not when it loses state, it's more than the pinctrl core refuses to > > re-apply the same state upon resumption even when the consumer driver > > tells it to, because it has not seen a transition (consider it as a > > stale software cache of the state), and there is only one state defined. > > > > I don't particularly care how its gets solved, at the generic device > > driver model or at the pinctrl level, but I think the pinctrl code needs > > to change in that regard no matter what we do, because right now, if you > > call pinctrl_select_state() in your driver's resume function, and there > > is only one state defined, nothing happens, that's a problem. > > I see, thanks for explaining that clearly. > > Then it looks like that there needs to be a way for the "cached" state to be > invalidated and the question is what that way should be and how it is going to > be triggered. Yeah seems like that issue needs to be solved at the pinctrl level even if the drivers were using a generic state for the context lost for figuring out when to do it. Regards, Tony
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Rafael J. Wysocki[171108 00:25]: > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > > For runtime PM, restoring the state constantly is unnecessary and not > > good for battery life. The logic can be just: > > > > 1. Device driver runtime PM suspend saves the state when needed > > > > 2. Device driver runtime PM resume checks if context_lost was set by > >the bus or power domain code > > > > 3. If context was lost, device driver restores the state, or in some > >cases may need re-run the driver register init related parts > >to bring the driver back up, then clears the context_lost flag > > > > How about something like the following patch? So far only compile > > tested with CONFIG_PM enabled. If that looks like the way to go, > > I'll test it properly and add some comments for the functions and > > post a proper patch :) > > Honestly, I'm not sure. > > I'd rather have a context_lost flag to start with and see how/if > drivers will use that before adding any common infra for handling > this. Right, I'll provide some use cases but it will be a little while. Currently it's done in non-generic way at the interconnect code for my use cases: $ git grep "\.context_offs = " arch/arm/mach-omap2/*data.c | wc -l 276 It seems that we could have genpd take care of this in a generic way with the patch I posted. Regards, Tony
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Rafael J. Wysocki [171108 00:25]: > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > > For runtime PM, restoring the state constantly is unnecessary and not > > good for battery life. The logic can be just: > > > > 1. Device driver runtime PM suspend saves the state when needed > > > > 2. Device driver runtime PM resume checks if context_lost was set by > >the bus or power domain code > > > > 3. If context was lost, device driver restores the state, or in some > >cases may need re-run the driver register init related parts > >to bring the driver back up, then clears the context_lost flag > > > > How about something like the following patch? So far only compile > > tested with CONFIG_PM enabled. If that looks like the way to go, > > I'll test it properly and add some comments for the functions and > > post a proper patch :) > > Honestly, I'm not sure. > > I'd rather have a context_lost flag to start with and see how/if > drivers will use that before adding any common infra for handling > this. Right, I'll provide some use cases but it will be a little while. Currently it's done in non-generic way at the interconnect code for my use cases: $ git grep "\.context_offs = " arch/arm/mach-omap2/*data.c | wc -l 276 It seems that we could have genpd take care of this in a generic way with the patch I posted. Regards, Tony
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On Wednesday, November 8, 2017 1:28:24 AM CET Florian Fainelli wrote: > On 11/07/2017 04:23 PM, Rafael J. Wysocki wrote: > > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > >> * Florian Fainelli[171104 17:21]: > >>> > >>> > >>> On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > > * Florian Fainelli [171103 17:04]: > >> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > >> The pinctrl provider is losing its state, hence these two patches. > > > > OK > > > >>> Anyways, the context lost flag should be managed in the PM core for > >>> the device, so adding linux-pm and Rafael to Cc. > >> > >> I don't think it's that simple but sure, why not. > > > > Just having bool context_lost in struct dev_pm_info would probably > > be enough to allow drivers to deal with it. This flag could then > > be set for a device by power domain related code that knows if > > context got lost. > > Something like: if the driver sees "context_lost" set, it should restore > the context to the device from memory? > >>> > >>> That is what is being proposed here, except that the actual mechanism > >>> where this matters needs to be in the core pinctrl code, otherwise the > >>> state (context) is not restored due to a check that attempts not to > >>> (re)apply a previous state. > >>> > > But the it would also need to save the context beforehand, so why not to > restore it unconditionally on resume? > >>> > >>> That's what my original attempts did here: > >>> > >>> https://patchwork.kernel.org/patch/9598969/ > >>> > >>> but Linus rightfully requested this to be done differently, hence this > >>> attempt now to solve it in a slightly more flexible way based on DT > >>> properties. > >> > >> For runtime PM, restoring the state constantly is unnecessary and not > >> good for battery life. The logic can be just: > >> > >> 1. Device driver runtime PM suspend saves the state when needed > >> > >> 2. Device driver runtime PM resume checks if context_lost was set by > >>the bus or power domain code > >> > >> 3. If context was lost, device driver restores the state, or in some > >>cases may need re-run the driver register init related parts > >>to bring the driver back up, then clears the context_lost flag > >> > >> How about something like the following patch? So far only compile > >> tested with CONFIG_PM enabled. If that looks like the way to go, > >> I'll test it properly and add some comments for the functions and > >> post a proper patch :) > > > > Honestly, I'm not sure. > > > > I'd rather have a context_lost flag to start with and see how/if > > drivers will use that before adding any common infra for handling > > this. > > I am afraid we are being slightly side tracked here on this context_loss > flag, because the crux of the problem is not whether a driver knows or > not when it loses state, it's more than the pinctrl core refuses to > re-apply the same state upon resumption even when the consumer driver > tells it to, because it has not seen a transition (consider it as a > stale software cache of the state), and there is only one state defined. > > I don't particularly care how its gets solved, at the generic device > driver model or at the pinctrl level, but I think the pinctrl code needs > to change in that regard no matter what we do, because right now, if you > call pinctrl_select_state() in your driver's resume function, and there > is only one state defined, nothing happens, that's a problem. I see, thanks for explaining that clearly. Then it looks like that there needs to be a way for the "cached" state to be invalidated and the question is what that way should be and how it is going to be triggered. Thanks, Rafael
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On Wednesday, November 8, 2017 1:28:24 AM CET Florian Fainelli wrote: > On 11/07/2017 04:23 PM, Rafael J. Wysocki wrote: > > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > >> * Florian Fainelli [171104 17:21]: > >>> > >>> > >>> On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > > * Florian Fainelli [171103 17:04]: > >> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > >> The pinctrl provider is losing its state, hence these two patches. > > > > OK > > > >>> Anyways, the context lost flag should be managed in the PM core for > >>> the device, so adding linux-pm and Rafael to Cc. > >> > >> I don't think it's that simple but sure, why not. > > > > Just having bool context_lost in struct dev_pm_info would probably > > be enough to allow drivers to deal with it. This flag could then > > be set for a device by power domain related code that knows if > > context got lost. > > Something like: if the driver sees "context_lost" set, it should restore > the context to the device from memory? > >>> > >>> That is what is being proposed here, except that the actual mechanism > >>> where this matters needs to be in the core pinctrl code, otherwise the > >>> state (context) is not restored due to a check that attempts not to > >>> (re)apply a previous state. > >>> > > But the it would also need to save the context beforehand, so why not to > restore it unconditionally on resume? > >>> > >>> That's what my original attempts did here: > >>> > >>> https://patchwork.kernel.org/patch/9598969/ > >>> > >>> but Linus rightfully requested this to be done differently, hence this > >>> attempt now to solve it in a slightly more flexible way based on DT > >>> properties. > >> > >> For runtime PM, restoring the state constantly is unnecessary and not > >> good for battery life. The logic can be just: > >> > >> 1. Device driver runtime PM suspend saves the state when needed > >> > >> 2. Device driver runtime PM resume checks if context_lost was set by > >>the bus or power domain code > >> > >> 3. If context was lost, device driver restores the state, or in some > >>cases may need re-run the driver register init related parts > >>to bring the driver back up, then clears the context_lost flag > >> > >> How about something like the following patch? So far only compile > >> tested with CONFIG_PM enabled. If that looks like the way to go, > >> I'll test it properly and add some comments for the functions and > >> post a proper patch :) > > > > Honestly, I'm not sure. > > > > I'd rather have a context_lost flag to start with and see how/if > > drivers will use that before adding any common infra for handling > > this. > > I am afraid we are being slightly side tracked here on this context_loss > flag, because the crux of the problem is not whether a driver knows or > not when it loses state, it's more than the pinctrl core refuses to > re-apply the same state upon resumption even when the consumer driver > tells it to, because it has not seen a transition (consider it as a > stale software cache of the state), and there is only one state defined. > > I don't particularly care how its gets solved, at the generic device > driver model or at the pinctrl level, but I think the pinctrl code needs > to change in that regard no matter what we do, because right now, if you > call pinctrl_select_state() in your driver's resume function, and there > is only one state defined, nothing happens, that's a problem. I see, thanks for explaining that clearly. Then it looks like that there needs to be a way for the "cached" state to be invalidated and the question is what that way should be and how it is going to be triggered. Thanks, Rafael
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On 11/07/2017 04:23 PM, Rafael J. Wysocki wrote: > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: >> * Florian Fainelli[171104 17:21]: >>> >>> >>> On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > * Florian Fainelli [171103 17:04]: >> On 11/03/2017 09:11 AM, Tony Lindgren wrote: >> The pinctrl provider is losing its state, hence these two patches. > > OK > >>> Anyways, the context lost flag should be managed in the PM core for >>> the device, so adding linux-pm and Rafael to Cc. >> >> I don't think it's that simple but sure, why not. > > Just having bool context_lost in struct dev_pm_info would probably > be enough to allow drivers to deal with it. This flag could then > be set for a device by power domain related code that knows if > context got lost. Something like: if the driver sees "context_lost" set, it should restore the context to the device from memory? >>> >>> That is what is being proposed here, except that the actual mechanism >>> where this matters needs to be in the core pinctrl code, otherwise the >>> state (context) is not restored due to a check that attempts not to >>> (re)apply a previous state. >>> But the it would also need to save the context beforehand, so why not to restore it unconditionally on resume? >>> >>> That's what my original attempts did here: >>> >>> https://patchwork.kernel.org/patch/9598969/ >>> >>> but Linus rightfully requested this to be done differently, hence this >>> attempt now to solve it in a slightly more flexible way based on DT >>> properties. >> >> For runtime PM, restoring the state constantly is unnecessary and not >> good for battery life. The logic can be just: >> >> 1. Device driver runtime PM suspend saves the state when needed >> >> 2. Device driver runtime PM resume checks if context_lost was set by >>the bus or power domain code >> >> 3. If context was lost, device driver restores the state, or in some >>cases may need re-run the driver register init related parts >>to bring the driver back up, then clears the context_lost flag >> >> How about something like the following patch? So far only compile >> tested with CONFIG_PM enabled. If that looks like the way to go, >> I'll test it properly and add some comments for the functions and >> post a proper patch :) > > Honestly, I'm not sure. > > I'd rather have a context_lost flag to start with and see how/if > drivers will use that before adding any common infra for handling > this. I am afraid we are being slightly side tracked here on this context_loss flag, because the crux of the problem is not whether a driver knows or not when it loses state, it's more than the pinctrl core refuses to re-apply the same state upon resumption even when the consumer driver tells it to, because it has not seen a transition (consider it as a stale software cache of the state), and there is only one state defined. I don't particularly care how its gets solved, at the generic device driver model or at the pinctrl level, but I think the pinctrl code needs to change in that regard no matter what we do, because right now, if you call pinctrl_select_state() in your driver's resume function, and there is only one state defined, nothing happens, that's a problem. -- Florian
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On 11/07/2017 04:23 PM, Rafael J. Wysocki wrote: > On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: >> * Florian Fainelli [171104 17:21]: >>> >>> >>> On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > * Florian Fainelli [171103 17:04]: >> On 11/03/2017 09:11 AM, Tony Lindgren wrote: >> The pinctrl provider is losing its state, hence these two patches. > > OK > >>> Anyways, the context lost flag should be managed in the PM core for >>> the device, so adding linux-pm and Rafael to Cc. >> >> I don't think it's that simple but sure, why not. > > Just having bool context_lost in struct dev_pm_info would probably > be enough to allow drivers to deal with it. This flag could then > be set for a device by power domain related code that knows if > context got lost. Something like: if the driver sees "context_lost" set, it should restore the context to the device from memory? >>> >>> That is what is being proposed here, except that the actual mechanism >>> where this matters needs to be in the core pinctrl code, otherwise the >>> state (context) is not restored due to a check that attempts not to >>> (re)apply a previous state. >>> But the it would also need to save the context beforehand, so why not to restore it unconditionally on resume? >>> >>> That's what my original attempts did here: >>> >>> https://patchwork.kernel.org/patch/9598969/ >>> >>> but Linus rightfully requested this to be done differently, hence this >>> attempt now to solve it in a slightly more flexible way based on DT >>> properties. >> >> For runtime PM, restoring the state constantly is unnecessary and not >> good for battery life. The logic can be just: >> >> 1. Device driver runtime PM suspend saves the state when needed >> >> 2. Device driver runtime PM resume checks if context_lost was set by >>the bus or power domain code >> >> 3. If context was lost, device driver restores the state, or in some >>cases may need re-run the driver register init related parts >>to bring the driver back up, then clears the context_lost flag >> >> How about something like the following patch? So far only compile >> tested with CONFIG_PM enabled. If that looks like the way to go, >> I'll test it properly and add some comments for the functions and >> post a proper patch :) > > Honestly, I'm not sure. > > I'd rather have a context_lost flag to start with and see how/if > drivers will use that before adding any common infra for handling > this. I am afraid we are being slightly side tracked here on this context_loss flag, because the crux of the problem is not whether a driver knows or not when it loses state, it's more than the pinctrl core refuses to re-apply the same state upon resumption even when the consumer driver tells it to, because it has not seen a transition (consider it as a stale software cache of the state), and there is only one state defined. I don't particularly care how its gets solved, at the generic device driver model or at the pinctrl level, but I think the pinctrl code needs to change in that regard no matter what we do, because right now, if you call pinctrl_select_state() in your driver's resume function, and there is only one state defined, nothing happens, that's a problem. -- Florian
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > * Florian Fainelli[171104 17:21]: > > > > > > On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > > > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > > >> * Florian Fainelli [171103 17:04]: > > >>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > >>> The pinctrl provider is losing its state, hence these two patches. > > >> > > >> OK > > >> > > Anyways, the context lost flag should be managed in the PM core for > > the device, so adding linux-pm and Rafael to Cc. > > >>> > > >>> I don't think it's that simple but sure, why not. > > >> > > >> Just having bool context_lost in struct dev_pm_info would probably > > >> be enough to allow drivers to deal with it. This flag could then > > >> be set for a device by power domain related code that knows if > > >> context got lost. > > > > > > Something like: if the driver sees "context_lost" set, it should restore > > > the context to the device from memory? > > > > That is what is being proposed here, except that the actual mechanism > > where this matters needs to be in the core pinctrl code, otherwise the > > state (context) is not restored due to a check that attempts not to > > (re)apply a previous state. > > > > > > > > But the it would also need to save the context beforehand, so why not to > > > restore it unconditionally on resume? > > > > That's what my original attempts did here: > > > > https://patchwork.kernel.org/patch/9598969/ > > > > but Linus rightfully requested this to be done differently, hence this > > attempt now to solve it in a slightly more flexible way based on DT > > properties. > > For runtime PM, restoring the state constantly is unnecessary and not > good for battery life. The logic can be just: > > 1. Device driver runtime PM suspend saves the state when needed > > 2. Device driver runtime PM resume checks if context_lost was set by >the bus or power domain code > > 3. If context was lost, device driver restores the state, or in some >cases may need re-run the driver register init related parts >to bring the driver back up, then clears the context_lost flag > > How about something like the following patch? So far only compile > tested with CONFIG_PM enabled. If that looks like the way to go, > I'll test it properly and add some comments for the functions and > post a proper patch :) Honestly, I'm not sure. I'd rather have a context_lost flag to start with and see how/if drivers will use that before adding any common infra for handling this. Thanks, Rafael
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On Tuesday, November 7, 2017 5:00:06 PM CET Tony Lindgren wrote: > * Florian Fainelli [171104 17:21]: > > > > > > On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > > > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > > >> * Florian Fainelli [171103 17:04]: > > >>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > >>> The pinctrl provider is losing its state, hence these two patches. > > >> > > >> OK > > >> > > Anyways, the context lost flag should be managed in the PM core for > > the device, so adding linux-pm and Rafael to Cc. > > >>> > > >>> I don't think it's that simple but sure, why not. > > >> > > >> Just having bool context_lost in struct dev_pm_info would probably > > >> be enough to allow drivers to deal with it. This flag could then > > >> be set for a device by power domain related code that knows if > > >> context got lost. > > > > > > Something like: if the driver sees "context_lost" set, it should restore > > > the context to the device from memory? > > > > That is what is being proposed here, except that the actual mechanism > > where this matters needs to be in the core pinctrl code, otherwise the > > state (context) is not restored due to a check that attempts not to > > (re)apply a previous state. > > > > > > > > But the it would also need to save the context beforehand, so why not to > > > restore it unconditionally on resume? > > > > That's what my original attempts did here: > > > > https://patchwork.kernel.org/patch/9598969/ > > > > but Linus rightfully requested this to be done differently, hence this > > attempt now to solve it in a slightly more flexible way based on DT > > properties. > > For runtime PM, restoring the state constantly is unnecessary and not > good for battery life. The logic can be just: > > 1. Device driver runtime PM suspend saves the state when needed > > 2. Device driver runtime PM resume checks if context_lost was set by >the bus or power domain code > > 3. If context was lost, device driver restores the state, or in some >cases may need re-run the driver register init related parts >to bring the driver back up, then clears the context_lost flag > > How about something like the following patch? So far only compile > tested with CONFIG_PM enabled. If that looks like the way to go, > I'll test it properly and add some comments for the functions and > post a proper patch :) Honestly, I'm not sure. I'd rather have a context_lost flag to start with and see how/if drivers will use that before adding any common infra for handling this. Thanks, Rafael
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Florian Fainelli[171104 17:21]: > > > On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > >> * Florian Fainelli [171103 17:04]: > >>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > >>> The pinctrl provider is losing its state, hence these two patches. > >> > >> OK > >> > Anyways, the context lost flag should be managed in the PM core for > the device, so adding linux-pm and Rafael to Cc. > >>> > >>> I don't think it's that simple but sure, why not. > >> > >> Just having bool context_lost in struct dev_pm_info would probably > >> be enough to allow drivers to deal with it. This flag could then > >> be set for a device by power domain related code that knows if > >> context got lost. > > > > Something like: if the driver sees "context_lost" set, it should restore > > the context to the device from memory? > > That is what is being proposed here, except that the actual mechanism > where this matters needs to be in the core pinctrl code, otherwise the > state (context) is not restored due to a check that attempts not to > (re)apply a previous state. > > > > > But the it would also need to save the context beforehand, so why not to > > restore it unconditionally on resume? > > That's what my original attempts did here: > > https://patchwork.kernel.org/patch/9598969/ > > but Linus rightfully requested this to be done differently, hence this > attempt now to solve it in a slightly more flexible way based on DT > properties. For runtime PM, restoring the state constantly is unnecessary and not good for battery life. The logic can be just: 1. Device driver runtime PM suspend saves the state when needed 2. Device driver runtime PM resume checks if context_lost was set by the bus or power domain code 3. If context was lost, device driver restores the state, or in some cases may need re-run the driver register init related parts to bring the driver back up, then clears the context_lost flag How about something like the following patch? So far only compile tested with CONFIG_PM enabled. If that looks like the way to go, I'll test it properly and add some comments for the functions and post a proper patch :) Regards, Tony 8< -- >From tony Mon Sep 17 00:00:00 2001 From: Tony Lindgren Date: Mon, 6 Nov 2017 07:02:54 -0800 Subject: [PATCH] RFC: PM / context: Add support for context lost handling On some hardware device drivers can lose their context in deeper idle states when the power is cut off for some power domain. As the drivers themselves may not be aware if the context was lost without reinitializing the device, we want to provide few helpers for tracking if device context was lost in a generic way. Typically the interconnect or power domain hardware knows when context was lost and we can set up a function for the consumer drivers to query the context lost state. The helpers for context lost that can be used in the following way: 1. Power domain or interconnect code registers a callback function with for a device error = dev_pm_register_context_lost_handler(dev, cb, data); if (error) ... 2. A consumer device driver saves it's state as needed in suspend or runtime PM suspend 3. A power domain is powered off during idle, and context is lost for all the devices in that power domain 4. On resume or runtime PM resume, a device driver queries the context lost state and restores the context if needed error = dev_pm_update_context_lost(dev); if (error) ... if (dev_pm_was_context_lost(dev)) restore_context(ddata); REVISIT: Compile tested only, needs testing and comments Not-Yet-Signed-off-by: Tony Lindgren --- drivers/base/power/Makefile | 1 + drivers/base/power/context.c | 84 drivers/base/power/power.h | 6 include/linux/pm.h | 1 + include/linux/pm_context.h | 51 +++ 5 files changed, 143 insertions(+) create mode 100644 drivers/base/power/context.c create mode 100644 include/linux/pm_context.h diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile --- a/drivers/base/power/Makefile +++ b/drivers/base/power/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o +obj-$(CONFIG_PM) += context.o obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o obj-$(CONFIG_PM_TRACE_RTC) += trace.o obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o diff --git a/drivers/base/power/context.c b/drivers/base/power/context.c new file mode 100644 --- /dev/null +++ b/drivers/base/power/context.c @@ -0,0 +1,84 @@ +/* + * context.c - Device context lost helper functions + * + * This program is free software; you can redistribute it and/or modify + * it under
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Charles Keepax[171104 08:38]: > On Fri, Nov 03, 2017 at 10:33:53AM -0700, Tony Lindgren wrote: > > * Florian Fainelli [171103 17:04]: > > > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > > The pinctrl provider is losing its state, hence these two patches. > > > > OK > > > > > > Anyways, the context lost flag should be managed in the PM core for > > > > the device, so adding linux-pm and Rafael to Cc. > > > > > > I don't think it's that simple but sure, why not. > > > > Just having bool context_lost in struct dev_pm_info would probably > > be enough to allow drivers to deal with it. This flag could then > > be set for a device by power domain related code that knows if > > context got lost. > > > > Anybody got better ideas? > > > > Should the provider driver not know its state will be lost since > will have had its PM ops called, and it should be aware of the > state it was in. So doesn't it just need to restore that state on > resume? Feels a bit like we are over complicating this here. > Apologies if I am missing some here. Well any driver can lose context depending on the hardare, this could be both the pinctrl provider and the pinctrl consumer drivers :) If your pinctrl provider driver knows when it lost context and knows when to restore it, then no generic code is needed necessarily for your use case. But as the drivers themselves are not aware themselves when they lost context without checking that all registers are initialized properly, I think we need something generic for tracking the context lost. Typically it's the power domain hardware that knows when context was lost. Regards, Tony
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Florian Fainelli [171104 17:21]: > > > On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > >> * Florian Fainelli [171103 17:04]: > >>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: > >>> The pinctrl provider is losing its state, hence these two patches. > >> > >> OK > >> > Anyways, the context lost flag should be managed in the PM core for > the device, so adding linux-pm and Rafael to Cc. > >>> > >>> I don't think it's that simple but sure, why not. > >> > >> Just having bool context_lost in struct dev_pm_info would probably > >> be enough to allow drivers to deal with it. This flag could then > >> be set for a device by power domain related code that knows if > >> context got lost. > > > > Something like: if the driver sees "context_lost" set, it should restore > > the context to the device from memory? > > That is what is being proposed here, except that the actual mechanism > where this matters needs to be in the core pinctrl code, otherwise the > state (context) is not restored due to a check that attempts not to > (re)apply a previous state. > > > > > But the it would also need to save the context beforehand, so why not to > > restore it unconditionally on resume? > > That's what my original attempts did here: > > https://patchwork.kernel.org/patch/9598969/ > > but Linus rightfully requested this to be done differently, hence this > attempt now to solve it in a slightly more flexible way based on DT > properties. For runtime PM, restoring the state constantly is unnecessary and not good for battery life. The logic can be just: 1. Device driver runtime PM suspend saves the state when needed 2. Device driver runtime PM resume checks if context_lost was set by the bus or power domain code 3. If context was lost, device driver restores the state, or in some cases may need re-run the driver register init related parts to bring the driver back up, then clears the context_lost flag How about something like the following patch? So far only compile tested with CONFIG_PM enabled. If that looks like the way to go, I'll test it properly and add some comments for the functions and post a proper patch :) Regards, Tony 8< -- >From tony Mon Sep 17 00:00:00 2001 From: Tony Lindgren Date: Mon, 6 Nov 2017 07:02:54 -0800 Subject: [PATCH] RFC: PM / context: Add support for context lost handling On some hardware device drivers can lose their context in deeper idle states when the power is cut off for some power domain. As the drivers themselves may not be aware if the context was lost without reinitializing the device, we want to provide few helpers for tracking if device context was lost in a generic way. Typically the interconnect or power domain hardware knows when context was lost and we can set up a function for the consumer drivers to query the context lost state. The helpers for context lost that can be used in the following way: 1. Power domain or interconnect code registers a callback function with for a device error = dev_pm_register_context_lost_handler(dev, cb, data); if (error) ... 2. A consumer device driver saves it's state as needed in suspend or runtime PM suspend 3. A power domain is powered off during idle, and context is lost for all the devices in that power domain 4. On resume or runtime PM resume, a device driver queries the context lost state and restores the context if needed error = dev_pm_update_context_lost(dev); if (error) ... if (dev_pm_was_context_lost(dev)) restore_context(ddata); REVISIT: Compile tested only, needs testing and comments Not-Yet-Signed-off-by: Tony Lindgren --- drivers/base/power/Makefile | 1 + drivers/base/power/context.c | 84 drivers/base/power/power.h | 6 include/linux/pm.h | 1 + include/linux/pm_context.h | 51 +++ 5 files changed, 143 insertions(+) create mode 100644 drivers/base/power/context.c create mode 100644 include/linux/pm_context.h diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile --- a/drivers/base/power/Makefile +++ b/drivers/base/power/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o +obj-$(CONFIG_PM) += context.o obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o obj-$(CONFIG_PM_TRACE_RTC) += trace.o obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o diff --git a/drivers/base/power/context.c b/drivers/base/power/context.c new file mode 100644 --- /dev/null +++ b/drivers/base/power/context.c @@ -0,0 +1,84 @@ +/* + * context.c - Device context lost helper functions + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Charles Keepax [171104 08:38]: > On Fri, Nov 03, 2017 at 10:33:53AM -0700, Tony Lindgren wrote: > > * Florian Fainelli [171103 17:04]: > > > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > > The pinctrl provider is losing its state, hence these two patches. > > > > OK > > > > > > Anyways, the context lost flag should be managed in the PM core for > > > > the device, so adding linux-pm and Rafael to Cc. > > > > > > I don't think it's that simple but sure, why not. > > > > Just having bool context_lost in struct dev_pm_info would probably > > be enough to allow drivers to deal with it. This flag could then > > be set for a device by power domain related code that knows if > > context got lost. > > > > Anybody got better ideas? > > > > Should the provider driver not know its state will be lost since > will have had its PM ops called, and it should be aware of the > state it was in. So doesn't it just need to restore that state on > resume? Feels a bit like we are over complicating this here. > Apologies if I am missing some here. Well any driver can lose context depending on the hardare, this could be both the pinctrl provider and the pinctrl consumer drivers :) If your pinctrl provider driver knows when it lost context and knows when to restore it, then no generic code is needed necessarily for your use case. But as the drivers themselves are not aware themselves when they lost context without checking that all registers are initialized properly, I think we need something generic for tracking the context lost. Typically it's the power domain hardware that knows when context was lost. Regards, Tony
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: >> * Florian Fainelli[171103 17:04]: >>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: >>> The pinctrl provider is losing its state, hence these two patches. >> >> OK >> Anyways, the context lost flag should be managed in the PM core for the device, so adding linux-pm and Rafael to Cc. >>> >>> I don't think it's that simple but sure, why not. >> >> Just having bool context_lost in struct dev_pm_info would probably >> be enough to allow drivers to deal with it. This flag could then >> be set for a device by power domain related code that knows if >> context got lost. > > Something like: if the driver sees "context_lost" set, it should restore > the context to the device from memory? That is what is being proposed here, except that the actual mechanism where this matters needs to be in the core pinctrl code, otherwise the state (context) is not restored due to a check that attempts not to (re)apply a previous state. > > But the it would also need to save the context beforehand, so why not to > restore it unconditionally on resume? That's what my original attempts did here: https://patchwork.kernel.org/patch/9598969/ but Linus rightfully requested this to be done differently, hence this attempt now to solve it in a slightly more flexible way based on DT properties. -- Florian
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On 11/04/2017 05:25 AM, Rafael J. Wysocki wrote: > On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: >> * Florian Fainelli [171103 17:04]: >>> On 11/03/2017 09:11 AM, Tony Lindgren wrote: >>> The pinctrl provider is losing its state, hence these two patches. >> >> OK >> Anyways, the context lost flag should be managed in the PM core for the device, so adding linux-pm and Rafael to Cc. >>> >>> I don't think it's that simple but sure, why not. >> >> Just having bool context_lost in struct dev_pm_info would probably >> be enough to allow drivers to deal with it. This flag could then >> be set for a device by power domain related code that knows if >> context got lost. > > Something like: if the driver sees "context_lost" set, it should restore > the context to the device from memory? That is what is being proposed here, except that the actual mechanism where this matters needs to be in the core pinctrl code, otherwise the state (context) is not restored due to a check that attempts not to (re)apply a previous state. > > But the it would also need to save the context beforehand, so why not to > restore it unconditionally on resume? That's what my original attempts did here: https://patchwork.kernel.org/patch/9598969/ but Linus rightfully requested this to be done differently, hence this attempt now to solve it in a slightly more flexible way based on DT properties. -- Florian
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > * Florian Fainelli[171103 17:04]: > > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > The pinctrl provider is losing its state, hence these two patches. > > OK > > > > Anyways, the context lost flag should be managed in the PM core for > > > the device, so adding linux-pm and Rafael to Cc. > > > > I don't think it's that simple but sure, why not. > > Just having bool context_lost in struct dev_pm_info would probably > be enough to allow drivers to deal with it. This flag could then > be set for a device by power domain related code that knows if > context got lost. Something like: if the driver sees "context_lost" set, it should restore the context to the device from memory? But the it would also need to save the context beforehand, so why not to restore it unconditionally on resume? Thanks, Rafael
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On Friday, November 3, 2017 6:33:53 PM CET Tony Lindgren wrote: > * Florian Fainelli [171103 17:04]: > > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > The pinctrl provider is losing its state, hence these two patches. > > OK > > > > Anyways, the context lost flag should be managed in the PM core for > > > the device, so adding linux-pm and Rafael to Cc. > > > > I don't think it's that simple but sure, why not. > > Just having bool context_lost in struct dev_pm_info would probably > be enough to allow drivers to deal with it. This flag could then > be set for a device by power domain related code that knows if > context got lost. Something like: if the driver sees "context_lost" set, it should restore the context to the device from memory? But the it would also need to save the context beforehand, so why not to restore it unconditionally on resume? Thanks, Rafael
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On Fri, Nov 03, 2017 at 10:33:53AM -0700, Tony Lindgren wrote: > * Florian Fainelli[171103 17:04]: > > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > The pinctrl provider is losing its state, hence these two patches. > > OK > > > > Anyways, the context lost flag should be managed in the PM core for > > > the device, so adding linux-pm and Rafael to Cc. > > > > I don't think it's that simple but sure, why not. > > Just having bool context_lost in struct dev_pm_info would probably > be enough to allow drivers to deal with it. This flag could then > be set for a device by power domain related code that knows if > context got lost. > > Anybody got better ideas? > Should the provider driver not know its state will be lost since will have had its PM ops called, and it should be aware of the state it was in. So doesn't it just need to restore that state on resume? Feels a bit like we are over complicating this here. Apologies if I am missing some here. Thanks, Charles
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On Fri, Nov 03, 2017 at 10:33:53AM -0700, Tony Lindgren wrote: > * Florian Fainelli [171103 17:04]: > > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > > The pinctrl provider is losing its state, hence these two patches. > > OK > > > > Anyways, the context lost flag should be managed in the PM core for > > > the device, so adding linux-pm and Rafael to Cc. > > > > I don't think it's that simple but sure, why not. > > Just having bool context_lost in struct dev_pm_info would probably > be enough to allow drivers to deal with it. This flag could then > be set for a device by power domain related code that knows if > context got lost. > > Anybody got better ideas? > Should the provider driver not know its state will be lost since will have had its PM ops called, and it should be aware of the state it was in. So doesn't it just need to restore that state on resume? Feels a bit like we are over complicating this here. Apologies if I am missing some here. Thanks, Charles
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Florian Fainelli[171103 17:04]: > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > The pinctrl provider is losing its state, hence these two patches. OK > > Anyways, the context lost flag should be managed in the PM core for > > the device, so adding linux-pm and Rafael to Cc. > > I don't think it's that simple but sure, why not. Just having bool context_lost in struct dev_pm_info would probably be enough to allow drivers to deal with it. This flag could then be set for a device by power domain related code that knows if context got lost. Anybody got better ideas? Regards, Tony
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Florian Fainelli [171103 17:04]: > On 11/03/2017 09:11 AM, Tony Lindgren wrote: > The pinctrl provider is losing its state, hence these two patches. OK > > Anyways, the context lost flag should be managed in the PM core for > > the device, so adding linux-pm and Rafael to Cc. > > I don't think it's that simple but sure, why not. Just having bool context_lost in struct dev_pm_info would probably be enough to allow drivers to deal with it. This flag could then be set for a device by power domain related code that knows if context got lost. Anybody got better ideas? Regards, Tony
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On 11/03/2017 03:37 AM, Charles Keepax wrote: > On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: >> Hello Linus, >> >> It's me again, so I have been thinking about the problem originally >> reported in: [PATCH fixes v3] pinctrl: Really force states during >> suspend/resume >> >> and other similar patches a while ago, and this new version allows a platform >> using pinctrl-single to specify whether its pins are going to lose their >> state >> during a system deep sleep. >> >> Note that this is still checked at the pinctrl_select_state() because >> consumers >> of the pinctrl API might be calling this from their suspend/resume functions >> and should not have to know whether the provider does lose its pin states. >> > > Still feels to me like it should be the providers job to the > restore the state rather than expecting the consumer to > re-request any state it had. But lets wait and see what Linus > thinks. The mechanism is generic, but the property needs to be placed at the pinctrl provider level anyways. > > Also not sure if you have seen this chain, but probably worth a > look: > > https://www.spinics.net/lists/devicetree/msg200649.html > > It is adding support to the GPIO code for controllers that can > have options to retain state across reset, not the same but > probably at least slightly related to this series. Let me take a closer look and see how much appears applicable. > > Thanks, > Charles > -- Florian
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On 11/03/2017 03:37 AM, Charles Keepax wrote: > On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: >> Hello Linus, >> >> It's me again, so I have been thinking about the problem originally >> reported in: [PATCH fixes v3] pinctrl: Really force states during >> suspend/resume >> >> and other similar patches a while ago, and this new version allows a platform >> using pinctrl-single to specify whether its pins are going to lose their >> state >> during a system deep sleep. >> >> Note that this is still checked at the pinctrl_select_state() because >> consumers >> of the pinctrl API might be calling this from their suspend/resume functions >> and should not have to know whether the provider does lose its pin states. >> > > Still feels to me like it should be the providers job to the > restore the state rather than expecting the consumer to > re-request any state it had. But lets wait and see what Linus > thinks. The mechanism is generic, but the property needs to be placed at the pinctrl provider level anyways. > > Also not sure if you have seen this chain, but probably worth a > look: > > https://www.spinics.net/lists/devicetree/msg200649.html > > It is adding support to the GPIO code for controllers that can > have options to retain state across reset, not the same but > probably at least slightly related to this series. Let me take a closer look and see how much appears applicable. > > Thanks, > Charles > -- Florian
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On 11/03/2017 09:11 AM, Tony Lindgren wrote: > * Charles Keepax[171103 10:38]: >> On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: >>> Hello Linus, >>> >>> It's me again, so I have been thinking about the problem originally >>> reported in: [PATCH fixes v3] pinctrl: Really force states during >>> suspend/resume >>> >>> and other similar patches a while ago, and this new version allows a >>> platform >>> using pinctrl-single to specify whether its pins are going to lose their >>> state >>> during a system deep sleep. >>> >>> Note that this is still checked at the pinctrl_select_state() because >>> consumers >>> of the pinctrl API might be calling this from their suspend/resume functions >>> and should not have to know whether the provider does lose its pin states. >>> >> >> Still feels to me like it should be the providers job to the >> restore the state rather than expecting the consumer to >> re-request any state it had. But lets wait and see what Linus >> thinks. > > But isn't it the consumer device losing it's state here? Or the > pinctrl provider losing it's state? The pinctrl provider is losing its state, hence these two patches. > > Anyways, the context lost flag should be managed in the PM core for > the device, so adding linux-pm and Rafael to Cc. I don't think it's that simple but sure, why not. -- Florian
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On 11/03/2017 09:11 AM, Tony Lindgren wrote: > * Charles Keepax [171103 10:38]: >> On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: >>> Hello Linus, >>> >>> It's me again, so I have been thinking about the problem originally >>> reported in: [PATCH fixes v3] pinctrl: Really force states during >>> suspend/resume >>> >>> and other similar patches a while ago, and this new version allows a >>> platform >>> using pinctrl-single to specify whether its pins are going to lose their >>> state >>> during a system deep sleep. >>> >>> Note that this is still checked at the pinctrl_select_state() because >>> consumers >>> of the pinctrl API might be calling this from their suspend/resume functions >>> and should not have to know whether the provider does lose its pin states. >>> >> >> Still feels to me like it should be the providers job to the >> restore the state rather than expecting the consumer to >> re-request any state it had. But lets wait and see what Linus >> thinks. > > But isn't it the consumer device losing it's state here? Or the > pinctrl provider losing it's state? The pinctrl provider is losing its state, hence these two patches. > > Anyways, the context lost flag should be managed in the PM core for > the device, so adding linux-pm and Rafael to Cc. I don't think it's that simple but sure, why not. -- Florian
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Charles Keepax[171103 10:38]: > On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: > > Hello Linus, > > > > It's me again, so I have been thinking about the problem originally > > reported in: [PATCH fixes v3] pinctrl: Really force states during > > suspend/resume > > > > and other similar patches a while ago, and this new version allows a > > platform > > using pinctrl-single to specify whether its pins are going to lose their > > state > > during a system deep sleep. > > > > Note that this is still checked at the pinctrl_select_state() because > > consumers > > of the pinctrl API might be calling this from their suspend/resume functions > > and should not have to know whether the provider does lose its pin states. > > > > Still feels to me like it should be the providers job to the > restore the state rather than expecting the consumer to > re-request any state it had. But lets wait and see what Linus > thinks. But isn't it the consumer device losing it's state here? Or the pinctrl provider losing it's state? Anyways, the context lost flag should be managed in the PM core for the device, so adding linux-pm and Rafael to Cc. Regards, Tony
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
* Charles Keepax [171103 10:38]: > On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: > > Hello Linus, > > > > It's me again, so I have been thinking about the problem originally > > reported in: [PATCH fixes v3] pinctrl: Really force states during > > suspend/resume > > > > and other similar patches a while ago, and this new version allows a > > platform > > using pinctrl-single to specify whether its pins are going to lose their > > state > > during a system deep sleep. > > > > Note that this is still checked at the pinctrl_select_state() because > > consumers > > of the pinctrl API might be calling this from their suspend/resume functions > > and should not have to know whether the provider does lose its pin states. > > > > Still feels to me like it should be the providers job to the > restore the state rather than expecting the consumer to > re-request any state it had. But lets wait and see what Linus > thinks. But isn't it the consumer device losing it's state here? Or the pinctrl provider losing it's state? Anyways, the context lost flag should be managed in the PM core for the device, so adding linux-pm and Rafael to Cc. Regards, Tony
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: > Hello Linus, > > It's me again, so I have been thinking about the problem originally > reported in: [PATCH fixes v3] pinctrl: Really force states during > suspend/resume > > and other similar patches a while ago, and this new version allows a platform > using pinctrl-single to specify whether its pins are going to lose their state > during a system deep sleep. > > Note that this is still checked at the pinctrl_select_state() because > consumers > of the pinctrl API might be calling this from their suspend/resume functions > and should not have to know whether the provider does lose its pin states. > Still feels to me like it should be the providers job to the restore the state rather than expecting the consumer to re-request any state it had. But lets wait and see what Linus thinks. Also not sure if you have seen this chain, but probably worth a look: https://www.spinics.net/lists/devicetree/msg200649.html It is adding support to the GPIO code for controllers that can have options to retain state across reset, not the same but probably at least slightly related to this series. Thanks, Charles
Re: [PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
On Thu, Nov 02, 2017 at 04:15:49PM -0700, Florian Fainelli wrote: > Hello Linus, > > It's me again, so I have been thinking about the problem originally > reported in: [PATCH fixes v3] pinctrl: Really force states during > suspend/resume > > and other similar patches a while ago, and this new version allows a platform > using pinctrl-single to specify whether its pins are going to lose their state > during a system deep sleep. > > Note that this is still checked at the pinctrl_select_state() because > consumers > of the pinctrl API might be calling this from their suspend/resume functions > and should not have to know whether the provider does lose its pin states. > Still feels to me like it should be the providers job to the restore the state rather than expecting the consumer to re-request any state it had. But lets wait and see what Linus thinks. Also not sure if you have seen this chain, but probably worth a look: https://www.spinics.net/lists/devicetree/msg200649.html It is adding support to the GPIO code for controllers that can have options to retain state across reset, not the same but probably at least slightly related to this series. Thanks, Charles
[PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
Hello Linus, It's me again, so I have been thinking about the problem originally reported in: [PATCH fixes v3] pinctrl: Really force states during suspend/resume and other similar patches a while ago, and this new version allows a platform using pinctrl-single to specify whether its pins are going to lose their state during a system deep sleep. Note that this is still checked at the pinctrl_select_state() because consumers of the pinctrl API might be calling this from their suspend/resume functions and should not have to know whether the provider does lose its pin states. This is against your pinctrl/for-next branch. Thanks! Changes in v2: - make the property generic and not specific to pinctrl-single Florian Fainelli (2): pinctrl: Allow a device to indicate when to force a state pinctrl: Allow indicating loss of pin states during low-power .../devicetree/bindings/pinctrl/pinctrl-bindings.txt| 4 drivers/pinctrl/core.c | 17 - drivers/pinctrl/core.h | 4 3 files changed, 24 insertions(+), 1 deletion(-) -- 2.9.3
[PATCH v2 0/2] pinctrl: Allow indicating loss of state across suspend/resume
Hello Linus, It's me again, so I have been thinking about the problem originally reported in: [PATCH fixes v3] pinctrl: Really force states during suspend/resume and other similar patches a while ago, and this new version allows a platform using pinctrl-single to specify whether its pins are going to lose their state during a system deep sleep. Note that this is still checked at the pinctrl_select_state() because consumers of the pinctrl API might be calling this from their suspend/resume functions and should not have to know whether the provider does lose its pin states. This is against your pinctrl/for-next branch. Thanks! Changes in v2: - make the property generic and not specific to pinctrl-single Florian Fainelli (2): pinctrl: Allow a device to indicate when to force a state pinctrl: Allow indicating loss of pin states during low-power .../devicetree/bindings/pinctrl/pinctrl-bindings.txt| 4 drivers/pinctrl/core.c | 17 - drivers/pinctrl/core.h | 4 3 files changed, 24 insertions(+), 1 deletion(-) -- 2.9.3