On Wed, Nov 15, 2023 at 7:21 AM <[email protected]> wrote:
>
> Tue, Nov 14, 2023 at 02:01:48PM +0100, Linus Walleij kirjoitti:
> > On Fri, Nov 10, 2023 at 11:21 AM Joy Chakraborty <[email protected]> wrote:
> >
> > > Apply pinctrl state from runtime framework device state transtion.
> > >
> > > Pinctrl states if defined in DT are bookmarked in device structures
> > > but they need to be explicitly applied from device driver callbacks
> > > which is boiler plate code and also not present in many drivers.
> > >
> > > If there is a specific order of setting pinctrl state with other driver
> > > actions then the device driver can choose to do it from its pm callbacks,
> > > in such a case this call will be a no-op from the pinctrl core framework
> > > since the desired pinctrl state would already be set.
> > >
> > > We could also add a Kconfig knob to enable/disable this, but I do not
> > > see a need to.
>
> Besides questionable code style (inline functions in the C file)...
Sure, I can change that.
>
> > It has a certain beauty to it does it not!
> >
> > The reason it wasn't done like this from the start was, if I recall correctly,
> > that in some cases a device needs to do the pin control state switching
> > in a special sequence with other operations, that can not be reordered,
> > i.e.:
> >
> > 1. The pin control state change is not context-free.
> >
> > 2. The order of events, i.e. context, does not necessarily match the
> > order that Linux subsystems happen to do things.
> >
> > When looking through the kernel tree I don't see that people use
> > the sleep state and idle state much, so we could very well go
> > with this, and then expect people that need special-casing to name
> > their states differently.
> >
> > What do people thing about that?
>
> ...I think the patch is incomplete(?) due to misterious ways of PM runtime
> calls. For example, in some cases we force runtime PM during system suspend
> which may have an undesired effect of the switching pin control states
> (hence glitches or some real issues with the hardware, up to hanging the
> system). Some pins may be critical to work with and shuffling their states
> in an unappropriate time can lead to a disaster.
>
> So, I would consider this change okay if and only if it will have a detailed
> research for all existing users to prove there will be no changes in the whole
> set of possible scenarious (of system sleep / resume, runtime, runtime with a
> custom ->prepare callback and so on).
>
I tried to place the calls to set the pinctrl states after driver/user
callback based on my understanding of runtime code so that existing
users do get a chance to set the state with any special sequence that
needs to be performed post which doing another call to set the state
would be ignored in the pinctrl framework.
But this only would be possible with the assumption that even in any
special sequences executed by users they set nothing but "default"
state in runtime_resume, "idle" state in runtime_idle and "'sleep"
state in their runtime suspend callbacks.
And like Andy mentions about "->prepare callback", if there are
drivers that are setting pinctrl state "default", "sleep" or "idle"
from any callback but
...
int (*runtime_suspend)(struct device *dev);
int (*runtime_resume)(struct device *dev);
int (*runtime_idle)(struct device *dev);
...
it could indeed be a problem.
I'll dig into users of pinctrl_select_sleep/default/idle and see if
there are such cases or if it could be done in some other way.
Thanks
Joy
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Thu, Nov 16, 2023 at 4:34 PM Joy Chakraborty <[email protected]> wrote:
> I tried to place the calls to set the pinctrl states after driver/user
> callback based on my understanding of runtime code so that existing
> users do get a chance to set the state with any special sequence that
> needs to be performed post which doing another call to set the state
> would be ignored in the pinctrl framework.
This makes sense. (And also is in the original commit.)
I think you should actually over-document this by also mentioning
this in the kerneldoc above each of the *_try_* callbacks so
users simply can't miss this point.
> But this only would be possible with the assumption that even in any
> special sequences executed by users they set nothing but "default"
> state in runtime_resume, "idle" state in runtime_idle and "'sleep"
> state in their runtime suspend callbacks.
> And like Andy mentions about "->prepare callback", if there are
> drivers that are setting pinctrl state "default", "sleep" or "idle"
> from any callback but
> ...
> int (*runtime_suspend)(struct device *dev);
> int (*runtime_resume)(struct device *dev);
> int (*runtime_idle)(struct device *dev);
> ...
> it could indeed be a problem.
> I'll dig into users of pinctrl_select_sleep/default/idle and see if
> there are such cases or if it could be done in some other way.
It's worth a check but I doubt much will turn up. The "idle" and
"sleep" states are simply not used much in the kernel.
Your users will likely be the first.
So which hardware target will use this?
It's immensely useful to have a good example to point at:
that device use "defaul", "sleep", "idle" the idiomatic way.
Yours,
Linus Walleij
On Fri, Nov 17, 2023 at 1:30 AM Linus Walleij <[email protected]> wrote:
>
> On Thu, Nov 16, 2023 at 4:34 PM Joy Chakraborty <[email protected]> wrote:
>
> > I tried to place the calls to set the pinctrl states after driver/user
> > callback based on my understanding of runtime code so that existing
> > users do get a chance to set the state with any special sequence that
> > needs to be performed post which doing another call to set the state
> > would be ignored in the pinctrl framework.
>
> This makes sense. (And also is in the original commit.)
>
> I think you should actually over-document this by also mentioning
> this in the kerneldoc above each of the *_try_* callbacks so
> users simply can't miss this point.
>
> > But this only would be possible with the assumption that even in any
> > special sequences executed by users they set nothing but "default"
> > state in runtime_resume, "idle" state in runtime_idle and "'sleep"
> > state in their runtime suspend callbacks.
> > And like Andy mentions about "->prepare callback", if there are
> > drivers that are setting pinctrl state "default", "sleep" or "idle"
> > from any callback but
> > ...
> > int (*runtime_suspend)(struct device *dev);
> > int (*runtime_resume)(struct device *dev);
> > int (*runtime_idle)(struct device *dev);
> > ...
> > it could indeed be a problem.
> > I'll dig into users of pinctrl_select_sleep/default/idle and see if
> > there are such cases or if it could be done in some other way.
>
> It's worth a check but I doubt much will turn up. The "idle" and
> "sleep" states are simply not used much in the kernel.
>
Right, I will look into this.
> Your users will likely be the first.
>
> So which hardware target will use this?
> It's immensely useful to have a good example to point at:
> that device use "defaul", "sleep", "idle" the idiomatic way.
I see some upstreaming activity on gs101 SOC.
I think gs101 and follow on SOCs could use this, I will find that out
and get back.
>
> Yours,
> Linus Walleij
Thanks a lot for your feedback
Joy
Joy