Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Thu, May 24, 2018 at 06:32:37AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180524 09:20]:
> > On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:

> > > Well if you have some better mechanism in mind let's try it out. Short of
> > > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of 
> > > ideas
> > > right now.
> > 
> > Yeah, that would be too much of a hack and likely wouldn't work either
> > (and we really should do away with those _force calls altogether).
> > 
> > I've been thinking a bit too much about this already, but it may be
> > possible to use the pm QoS framework for this. A resume latency can be
> > set through sysfs where "n/a" is defined to mean "no latency accepted"
> > (i.e. controller remains always-on while port is open) and "0" means
> > "any latency accepted" (i.e. omap aggressive serial RPM is allowed).
> 
> Oh yeah, PM QoS might work here!

Actually, after reading a recent QoS related bug report, I realised that
a resume latency request of "n/a" is actually a third way of disabling
runtime PM, which similarly to the negative autosuspend would prevent
also a closed port from suspending.

Using a small positive resume latency for this feels like too much of a
hack, but defining a new QoS flag might still work.

Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Thu, May 24, 2018 at 06:32:37AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180524 09:20]:
> > On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:

> > > Well if you have some better mechanism in mind let's try it out. Short of
> > > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of 
> > > ideas
> > > right now.
> > 
> > Yeah, that would be too much of a hack and likely wouldn't work either
> > (and we really should do away with those _force calls altogether).
> > 
> > I've been thinking a bit too much about this already, but it may be
> > possible to use the pm QoS framework for this. A resume latency can be
> > set through sysfs where "n/a" is defined to mean "no latency accepted"
> > (i.e. controller remains always-on while port is open) and "0" means
> > "any latency accepted" (i.e. omap aggressive serial RPM is allowed).
> 
> Oh yeah, PM QoS might work here!

Actually, after reading a recent QoS related bug report, I realised that
a resume latency request of "n/a" is actually a third way of disabling
runtime PM, which similarly to the negative autosuspend would prevent
also a closed port from suspending.

Using a small positive resume latency for this feels like too much of a
hack, but defining a new QoS flag might still work.

Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold  [180524 09:20]:
> On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> > 
> > Yes the bug for closed ports needs to be fixed for sure.
> 
> I did some forensic on this and it seems this problem has "always" been
> there. Specifically, closed ports have never been runtime suspended
> unless a non-negative autosuspend delay has been set by user space since
> fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
> driver") which was merged seven years ago.
>
> So while it would certainly be nice to save some more power by default,
> this would really be a new feature rather than a bug or regression fix
> (which reduces the urgency for this issue somewhat too).

Yes it's been there since the start.

> > > > >   2. aggressive serial RPM, where the controller is allowed to
> > > > >  suspend while the port is open even though this may result in
> > > > >  lost characters when waking up on incoming data
> > > > 
> > > > In this case it seems that the only thing needed is to just
> > > > configure the autosuspend delay for the parent port. The use of
> > > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > > we should just document it. I guess we could also introduce
> > > > pm_runtime_block_autoidle_unless_configured() :)
> > > 
> > > The implications of a negative autosuspend delay are already documented
> > > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > > gets it wrong when trying to do things which aren't currently supported
> > > (and never have been).
> > > 
> > > So I still think we need a new mechanism for this.
> > 
> > Well if you have some better mechanism in mind let's try it out. Short of
> > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> > right now.
> 
> Yeah, that would be too much of a hack and likely wouldn't work either
> (and we really should do away with those _force calls altogether).
> 
> I've been thinking a bit too much about this already, but it may be
> possible to use the pm QoS framework for this. A resume latency can be
> set through sysfs where "n/a" is defined to mean "no latency accepted"
> (i.e. controller remains always-on while port is open) and "0" means
> "any latency accepted" (i.e. omap aggressive serial RPM is allowed).

Oh yeah, PM QoS might work here!

> Now, implementing this may get a little tricky as we want to be able to
> change this setting on the fly (consider consoles) and we need to figure
> out the interaction with serdev (user space should probably not be
> allowed to request a resume latency for ports used by serdev).

It sounds like Andy Shevchenko has a series of patches that just might
allow us to make this all generic for Linux serial framework. So adding
Andy to Cc, I don't think he has posted all the patches yet.

Andy, see the PM QoS comment above for console idling :)

> I'd be happy to dig into this some more, but not in my spare time I'm
> afraid.

Indeed.

> > > > > For normal ttys, we need a user-space interface for selecting between
> > > > > the two, and for serdev we may want a way to select the RPM scheme 
> > > > > from
> > > > > within the kernel.
> > > > > 
> > > > > Note that with my serdev controller runtime PM patch, serdev core 
> > > > > could
> > > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > > reference for the controller while the port is open).
> > > > 
> > > > So if your serdev controller was to set the parent autosuspend
> > > > delay on open() and set it back on close() this should work?
> > > 
> > > Is it really the job of a serdev driver to set the autosuspend delay of
> > > a parent controller? Isn't this somethings which depends on the
> > > characteristics of the controller (possibly configurable by user space)
> > > such as the cost of runtime suspending and resuming?
> > 
> > Only in some cases will the serdev driver know it's safe to configure
> > the parent controller. Configuring the parent controller from userspace
> > works just fine as we've seen for years now.
> 
> Yes, user space may override the default settings provided by the serial
> driver, but a serdev driver, in contrast, knows nothing about the
> underlying serial hardware.
> 
> > > The patch I posted works with what we have today; if a parent serial
> > > controller driver uses aggressive runtime PM by default or after having
> > > been configured through sysfs to do so.
> > 
> > Yeah let's stick with configuring the parent controller from userspace
> > for now at least.
> 
> Yep, status quo works for the time being (since this isn't a
> regression). 
> 
> > > What I'm getting at here is that the delay should be set by the serial
> > > driver implementing aggressive runtime PM. Then all we need is a
> > > mechanism to determine whether an extra RPM reference should be taken at
> > > tty open or not (configurable by user space, defaulting to yes).
> > 
> 

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold  [180524 09:20]:
> On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> > 
> > Yes the bug for closed ports needs to be fixed for sure.
> 
> I did some forensic on this and it seems this problem has "always" been
> there. Specifically, closed ports have never been runtime suspended
> unless a non-negative autosuspend delay has been set by user space since
> fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
> driver") which was merged seven years ago.
>
> So while it would certainly be nice to save some more power by default,
> this would really be a new feature rather than a bug or regression fix
> (which reduces the urgency for this issue somewhat too).

Yes it's been there since the start.

> > > > >   2. aggressive serial RPM, where the controller is allowed to
> > > > >  suspend while the port is open even though this may result in
> > > > >  lost characters when waking up on incoming data
> > > > 
> > > > In this case it seems that the only thing needed is to just
> > > > configure the autosuspend delay for the parent port. The use of
> > > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > > we should just document it. I guess we could also introduce
> > > > pm_runtime_block_autoidle_unless_configured() :)
> > > 
> > > The implications of a negative autosuspend delay are already documented
> > > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > > gets it wrong when trying to do things which aren't currently supported
> > > (and never have been).
> > > 
> > > So I still think we need a new mechanism for this.
> > 
> > Well if you have some better mechanism in mind let's try it out. Short of
> > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> > right now.
> 
> Yeah, that would be too much of a hack and likely wouldn't work either
> (and we really should do away with those _force calls altogether).
> 
> I've been thinking a bit too much about this already, but it may be
> possible to use the pm QoS framework for this. A resume latency can be
> set through sysfs where "n/a" is defined to mean "no latency accepted"
> (i.e. controller remains always-on while port is open) and "0" means
> "any latency accepted" (i.e. omap aggressive serial RPM is allowed).

Oh yeah, PM QoS might work here!

> Now, implementing this may get a little tricky as we want to be able to
> change this setting on the fly (consider consoles) and we need to figure
> out the interaction with serdev (user space should probably not be
> allowed to request a resume latency for ports used by serdev).

It sounds like Andy Shevchenko has a series of patches that just might
allow us to make this all generic for Linux serial framework. So adding
Andy to Cc, I don't think he has posted all the patches yet.

Andy, see the PM QoS comment above for console idling :)

> I'd be happy to dig into this some more, but not in my spare time I'm
> afraid.

Indeed.

> > > > > For normal ttys, we need a user-space interface for selecting between
> > > > > the two, and for serdev we may want a way to select the RPM scheme 
> > > > > from
> > > > > within the kernel.
> > > > > 
> > > > > Note that with my serdev controller runtime PM patch, serdev core 
> > > > > could
> > > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > > reference for the controller while the port is open).
> > > > 
> > > > So if your serdev controller was to set the parent autosuspend
> > > > delay on open() and set it back on close() this should work?
> > > 
> > > Is it really the job of a serdev driver to set the autosuspend delay of
> > > a parent controller? Isn't this somethings which depends on the
> > > characteristics of the controller (possibly configurable by user space)
> > > such as the cost of runtime suspending and resuming?
> > 
> > Only in some cases will the serdev driver know it's safe to configure
> > the parent controller. Configuring the parent controller from userspace
> > works just fine as we've seen for years now.
> 
> Yes, user space may override the default settings provided by the serial
> driver, but a serdev driver, in contrast, knows nothing about the
> underlying serial hardware.
> 
> > > The patch I posted works with what we have today; if a parent serial
> > > controller driver uses aggressive runtime PM by default or after having
> > > been configured through sysfs to do so.
> > 
> > Yeah let's stick with configuring the parent controller from userspace
> > for now at least.
> 
> Yep, status quo works for the time being (since this isn't a
> regression). 
> 
> > > What I'm getting at here is that the delay should be set by the serial
> > > driver implementing aggressive runtime PM. Then all we need is a
> > > mechanism to determine whether an extra RPM reference should be taken at
> > > tty open or not (configurable by user space, defaulting to yes).
> > 
> > OK yeah some 

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180521 13:50]:
> > On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold  [180517 10:12]:

> > > Well in that case we should just stick with -1 value for the
> > > autosuspend. And just do pm_runtime_put_sync_suspend() after
> > > probe and on close.
> > 
> > That won't work either as a negative autosuspend delay prevents runtime
> > suspend completely (by grabbing an extra RPM reference).
> 
> Well so negative autosuspend delay is working as documented then :)

Yes, indeed. All too well. ;)

> > > > I fail to see how we can implement this using the current toolbox. What
> > > > you're after here is really a mechanism for selecting between two
> > > > different runtime PM schemes at runtime:
> > > > 
> > > > 1. normal serial RPM, where the controller is active while the
> > > >port is open (this should be the safe default)
> > > 
> > > Agreed. And that is the case already.
> > 
> > Yes, but it's not really the case today as since omap-serial (and
> > 8250-omap) sets a negative autosuspend at probe and hence does not
> > runtime-suspend when the port is closed. So that's the long-standing bug
> > which needs fixing.
> 
> Yes the bug for closed ports needs to be fixed for sure.

I did some forensic on this and it seems this problem has "always" been
there. Specifically, closed ports have never been runtime suspended
unless a non-negative autosuspend delay has been set by user space since
fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
driver") which was merged seven years ago.

So while it would certainly be nice to save some more power by default,
this would really be a new feature rather than a bug or regression fix
(which reduces the urgency for this issue somewhat too).

> > > > 2. aggressive serial RPM, where the controller is allowed to
> > > >suspend while the port is open even though this may result in
> > > >lost characters when waking up on incoming data
> > > 
> > > In this case it seems that the only thing needed is to just
> > > configure the autosuspend delay for the parent port. The use of
> > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > we should just document it. I guess we could also introduce
> > > pm_runtime_block_autoidle_unless_configured() :)
> > 
> > The implications of a negative autosuspend delay are already documented
> > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > gets it wrong when trying to do things which aren't currently supported
> > (and never have been).
> > 
> > So I still think we need a new mechanism for this.
> 
> Well if you have some better mechanism in mind let's try it out. Short of
> sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> right now.

Yeah, that would be too much of a hack and likely wouldn't work either
(and we really should do away with those _force calls altogether).

I've been thinking a bit too much about this already, but it may be
possible to use the pm QoS framework for this. A resume latency can be
set through sysfs where "n/a" is defined to mean "no latency accepted"
(i.e. controller remains always-on while port is open) and "0" means
"any latency accepted" (i.e. omap aggressive serial RPM is allowed).

Now, implementing this may get a little tricky as we want to be able to
change this setting on the fly (consider consoles) and we need to figure
out the interaction with serdev (user space should probably not be
allowed to request a resume latency for ports used by serdev).

I'd be happy to dig into this some more, but not in my spare time I'm
afraid.

> > > > For normal ttys, we need a user-space interface for selecting between
> > > > the two, and for serdev we may want a way to select the RPM scheme from
> > > > within the kernel.
> > > > 
> > > > Note that with my serdev controller runtime PM patch, serdev core could
> > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > reference for the controller while the port is open).
> > > 
> > > So if your serdev controller was to set the parent autosuspend
> > > delay on open() and set it back on close() this should work?
> > 
> > Is it really the job of a serdev driver to set the autosuspend delay of
> > a parent controller? Isn't this somethings which depends on the
> > characteristics of the controller (possibly configurable by user space)
> > such as the cost of runtime suspending and resuming?
> 
> Only in some cases will the serdev driver know it's safe to configure
> the parent controller. Configuring the parent controller from userspace
> works just fine as we've seen for years now.

Yes, user space may override the default settings provided by the serial
driver, but a serdev driver, in contrast, knows nothing about the
underlying serial hardware.

> > 

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180521 13:50]:
> > On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold  [180517 10:12]:

> > > Well in that case we should just stick with -1 value for the
> > > autosuspend. And just do pm_runtime_put_sync_suspend() after
> > > probe and on close.
> > 
> > That won't work either as a negative autosuspend delay prevents runtime
> > suspend completely (by grabbing an extra RPM reference).
> 
> Well so negative autosuspend delay is working as documented then :)

Yes, indeed. All too well. ;)

> > > > I fail to see how we can implement this using the current toolbox. What
> > > > you're after here is really a mechanism for selecting between two
> > > > different runtime PM schemes at runtime:
> > > > 
> > > > 1. normal serial RPM, where the controller is active while the
> > > >port is open (this should be the safe default)
> > > 
> > > Agreed. And that is the case already.
> > 
> > Yes, but it's not really the case today as since omap-serial (and
> > 8250-omap) sets a negative autosuspend at probe and hence does not
> > runtime-suspend when the port is closed. So that's the long-standing bug
> > which needs fixing.
> 
> Yes the bug for closed ports needs to be fixed for sure.

I did some forensic on this and it seems this problem has "always" been
there. Specifically, closed ports have never been runtime suspended
unless a non-negative autosuspend delay has been set by user space since
fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
driver") which was merged seven years ago.

So while it would certainly be nice to save some more power by default,
this would really be a new feature rather than a bug or regression fix
(which reduces the urgency for this issue somewhat too).

> > > > 2. aggressive serial RPM, where the controller is allowed to
> > > >suspend while the port is open even though this may result in
> > > >lost characters when waking up on incoming data
> > > 
> > > In this case it seems that the only thing needed is to just
> > > configure the autosuspend delay for the parent port. The use of
> > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > we should just document it. I guess we could also introduce
> > > pm_runtime_block_autoidle_unless_configured() :)
> > 
> > The implications of a negative autosuspend delay are already documented
> > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > gets it wrong when trying to do things which aren't currently supported
> > (and never have been).
> > 
> > So I still think we need a new mechanism for this.
> 
> Well if you have some better mechanism in mind let's try it out. Short of
> sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> right now.

Yeah, that would be too much of a hack and likely wouldn't work either
(and we really should do away with those _force calls altogether).

I've been thinking a bit too much about this already, but it may be
possible to use the pm QoS framework for this. A resume latency can be
set through sysfs where "n/a" is defined to mean "no latency accepted"
(i.e. controller remains always-on while port is open) and "0" means
"any latency accepted" (i.e. omap aggressive serial RPM is allowed).

Now, implementing this may get a little tricky as we want to be able to
change this setting on the fly (consider consoles) and we need to figure
out the interaction with serdev (user space should probably not be
allowed to request a resume latency for ports used by serdev).

I'd be happy to dig into this some more, but not in my spare time I'm
afraid.

> > > > For normal ttys, we need a user-space interface for selecting between
> > > > the two, and for serdev we may want a way to select the RPM scheme from
> > > > within the kernel.
> > > > 
> > > > Note that with my serdev controller runtime PM patch, serdev core could
> > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > reference for the controller while the port is open).
> > > 
> > > So if your serdev controller was to set the parent autosuspend
> > > delay on open() and set it back on close() this should work?
> > 
> > Is it really the job of a serdev driver to set the autosuspend delay of
> > a parent controller? Isn't this somethings which depends on the
> > characteristics of the controller (possibly configurable by user space)
> > such as the cost of runtime suspending and resuming?
> 
> Only in some cases will the serdev driver know it's safe to configure
> the parent controller. Configuring the parent controller from userspace
> works just fine as we've seen for years now.

Yes, user space may override the default settings provided by the serial
driver, but a serdev driver, in contrast, knows nothing about the
underlying serial hardware.

> > The patch I posted works with what we 

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold  [180521 13:50]:
> On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> > * Johan Hovold  [180517 10:12]:
> > > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > > either as that would also prevent the device from runtime suspending
> > > just as the current negative autosuspend delay does.
> > 
> > Well in that case we should just stick with -1 value for the
> > autosuspend. And just do pm_runtime_put_sync_suspend() after
> > probe and on close.
> 
> That won't work either as a negative autosuspend delay prevents runtime
> suspend completely (by grabbing an extra RPM reference).

Well so negative autosuspend delay is working as documented then :)

> > > I fail to see how we can implement this using the current toolbox. What
> > > you're after here is really a mechanism for selecting between two
> > > different runtime PM schemes at runtime:
> > > 
> > >   1. normal serial RPM, where the controller is active while the
> > >  port is open (this should be the safe default)
> > 
> > Agreed. And that is the case already.
> 
> Yes, but it's not really the case today as since omap-serial (and
> 8250-omap) sets a negative autosuspend at probe and hence does not
> runtime-suspend when the port is closed. So that's the long-standing bug
> which needs fixing.

Yes the bug for closed ports needs to be fixed for sure.

> > >   2. aggressive serial RPM, where the controller is allowed to
> > >  suspend while the port is open even though this may result in
> > >  lost characters when waking up on incoming data
> > 
> > In this case it seems that the only thing needed is to just
> > configure the autosuspend delay for the parent port. The use of
> > -1 has been around since the start of runtime PM AFAIK, so maybe
> > we should just document it. I guess we could also introduce
> > pm_runtime_block_autoidle_unless_configured() :)
> 
> The implications of a negative autosuspend delay are already documented
> (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> gets it wrong when trying to do things which aren't currently supported
> (and never have been).
> 
> So I still think we need a new mechanism for this.

Well if you have some better mechanism in mind let's try it out. Short of
sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
right now.

> > > For normal ttys, we need a user-space interface for selecting between
> > > the two, and for serdev we may want a way to select the RPM scheme from
> > > within the kernel.
> > > 
> > > Note that with my serdev controller runtime PM patch, serdev core could
> > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > reference for the controller while the port is open).
> > 
> > So if your serdev controller was to set the parent autosuspend
> > delay on open() and set it back on close() this should work?
> 
> Is it really the job of a serdev driver to set the autosuspend delay of
> a parent controller? Isn't this somethings which depends on the
> characteristics of the controller (possibly configurable by user space)
> such as the cost of runtime suspending and resuming?

Only in some cases will the serdev driver know it's safe to configure
the parent controller. Configuring the parent controller from userspace
works just fine as we've seen for years now.

> The patch I posted works with what we have today; if a parent serial
> controller driver uses aggressive runtime PM by default or after having
> been configured through sysfs to do so.

Yeah let's stick with configuring the parent controller from userspace
for now at least.

> What I'm getting at here is that the delay should be set by the serial
> driver implementing aggressive runtime PM. Then all we need is a
> mechanism to determine whether an extra RPM reference should be taken at
> tty open or not (configurable by user space, defaulting to yes).

OK yeah some additional on/off switch seems to be missing here.

> Specifically, the serial drivers themselves would always use
> autosuspend and not have to deal with supporting the two RPM schemes
> (normal vs aggressive runtime PM).

OK. So if I understand your idea right, we could have autosuspend timeout
set to 3000ms in the 8250_omap.c but still default to RPM blocked?
Then user can enable aggressive PM via /sys as desired, right?

Regards,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold  [180521 13:50]:
> On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> > * Johan Hovold  [180517 10:12]:
> > > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > > either as that would also prevent the device from runtime suspending
> > > just as the current negative autosuspend delay does.
> > 
> > Well in that case we should just stick with -1 value for the
> > autosuspend. And just do pm_runtime_put_sync_suspend() after
> > probe and on close.
> 
> That won't work either as a negative autosuspend delay prevents runtime
> suspend completely (by grabbing an extra RPM reference).

Well so negative autosuspend delay is working as documented then :)

> > > I fail to see how we can implement this using the current toolbox. What
> > > you're after here is really a mechanism for selecting between two
> > > different runtime PM schemes at runtime:
> > > 
> > >   1. normal serial RPM, where the controller is active while the
> > >  port is open (this should be the safe default)
> > 
> > Agreed. And that is the case already.
> 
> Yes, but it's not really the case today as since omap-serial (and
> 8250-omap) sets a negative autosuspend at probe and hence does not
> runtime-suspend when the port is closed. So that's the long-standing bug
> which needs fixing.

Yes the bug for closed ports needs to be fixed for sure.

> > >   2. aggressive serial RPM, where the controller is allowed to
> > >  suspend while the port is open even though this may result in
> > >  lost characters when waking up on incoming data
> > 
> > In this case it seems that the only thing needed is to just
> > configure the autosuspend delay for the parent port. The use of
> > -1 has been around since the start of runtime PM AFAIK, so maybe
> > we should just document it. I guess we could also introduce
> > pm_runtime_block_autoidle_unless_configured() :)
> 
> The implications of a negative autosuspend delay are already documented
> (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> gets it wrong when trying to do things which aren't currently supported
> (and never have been).
> 
> So I still think we need a new mechanism for this.

Well if you have some better mechanism in mind let's try it out. Short of
sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
right now.

> > > For normal ttys, we need a user-space interface for selecting between
> > > the two, and for serdev we may want a way to select the RPM scheme from
> > > within the kernel.
> > > 
> > > Note that with my serdev controller runtime PM patch, serdev core could
> > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > reference for the controller while the port is open).
> > 
> > So if your serdev controller was to set the parent autosuspend
> > delay on open() and set it back on close() this should work?
> 
> Is it really the job of a serdev driver to set the autosuspend delay of
> a parent controller? Isn't this somethings which depends on the
> characteristics of the controller (possibly configurable by user space)
> such as the cost of runtime suspending and resuming?

Only in some cases will the serdev driver know it's safe to configure
the parent controller. Configuring the parent controller from userspace
works just fine as we've seen for years now.

> The patch I posted works with what we have today; if a parent serial
> controller driver uses aggressive runtime PM by default or after having
> been configured through sysfs to do so.

Yeah let's stick with configuring the parent controller from userspace
for now at least.

> What I'm getting at here is that the delay should be set by the serial
> driver implementing aggressive runtime PM. Then all we need is a
> mechanism to determine whether an extra RPM reference should be taken at
> tty open or not (configurable by user space, defaulting to yes).

OK yeah some additional on/off switch seems to be missing here.

> Specifically, the serial drivers themselves would always use
> autosuspend and not have to deal with supporting the two RPM schemes
> (normal vs aggressive runtime PM).

OK. So if I understand your idea right, we could have autosuspend timeout
set to 3000ms in the 8250_omap.c but still default to RPM blocked?
Then user can enable aggressive PM via /sys as desired, right?

Regards,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180517 10:12]:
> > [ Sorry about the late reply. ]
> > 
> > On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold  [180509 13:12]:
> > 
> > > > It seems we really should not be using the negative autosuspend to
> > > > configure the RPM behaviour the way these drivers do. Perhaps a new
> > > > mechanism is needed.
> > > 
> > > Hmm well simply defaulting to "on" instead of "auto" and setting the
> > > autosuspend_ms to 3000 by default might be doable. I think that way
> > > we can keep use_autosuspend() in probe. Let's hope there are no
> > > existing use cases that would break with that.
> > 
> > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > either as that would also prevent the device from runtime suspending
> > just as the current negative autosuspend delay does.
> 
> Well in that case we should just stick with -1 value for the
> autosuspend. And just do pm_runtime_put_sync_suspend() after
> probe and on close.

That won't work either as a negative autosuspend delay prevents runtime
suspend completely (by grabbing an extra RPM reference).

> > I fail to see how we can implement this using the current toolbox. What
> > you're after here is really a mechanism for selecting between two
> > different runtime PM schemes at runtime:
> > 
> > 1. normal serial RPM, where the controller is active while the
> >port is open (this should be the safe default)
> 
> Agreed. And that is the case already.

Yes, but it's not really the case today as since omap-serial (and
8250-omap) sets a negative autosuspend at probe and hence does not
runtime-suspend when the port is closed. So that's the long-standing bug
which needs fixing.

> > 2. aggressive serial RPM, where the controller is allowed to
> >suspend while the port is open even though this may result in
> >lost characters when waking up on incoming data
> 
> In this case it seems that the only thing needed is to just
> configure the autosuspend delay for the parent port. The use of
> -1 has been around since the start of runtime PM AFAIK, so maybe
> we should just document it. I guess we could also introduce
> pm_runtime_block_autoidle_unless_configured() :)

The implications of a negative autosuspend delay are already documented
(in Documentation/power/runtime_pm.txt); it's just the omap drivers that
gets it wrong when trying to do things which aren't currently supported
(and never have been).

So I still think we need a new mechanism for this.

> > For normal ttys, we need a user-space interface for selecting between
> > the two, and for serdev we may want a way to select the RPM scheme from
> > within the kernel.
> > 
> > Note that with my serdev controller runtime PM patch, serdev core could
> > always opt for aggressive PM (as by default serdev core holds an RPM
> > reference for the controller while the port is open).
> 
> So if your serdev controller was to set the parent autosuspend
> delay on open() and set it back on close() this should work?

Is it really the job of a serdev driver to set the autosuspend delay of
a parent controller? Isn't this somethings which depends on the
characteristics of the controller (possibly configurable by user space)
such as the cost of runtime suspending and resuming?

The patch I posted works with what we have today; if a parent serial
controller driver uses aggressive runtime PM by default or after having
been configured through sysfs to do so.

What I'm getting at here is that the delay should be set by the serial
driver implementing aggressive runtime PM. Then all we need is a
mechanism to determine whether an extra RPM reference should be taken at
tty open or not (configurable by user space, defaulting to yes).

Specifically, the serial drivers themselves would always use
autosuspend and not have to deal with supporting the two RPM schemes
(normal vs aggressive runtime PM).

Thanks,
Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180517 10:12]:
> > [ Sorry about the late reply. ]
> > 
> > On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold  [180509 13:12]:
> > 
> > > > It seems we really should not be using the negative autosuspend to
> > > > configure the RPM behaviour the way these drivers do. Perhaps a new
> > > > mechanism is needed.
> > > 
> > > Hmm well simply defaulting to "on" instead of "auto" and setting the
> > > autosuspend_ms to 3000 by default might be doable. I think that way
> > > we can keep use_autosuspend() in probe. Let's hope there are no
> > > existing use cases that would break with that.
> > 
> > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > either as that would also prevent the device from runtime suspending
> > just as the current negative autosuspend delay does.
> 
> Well in that case we should just stick with -1 value for the
> autosuspend. And just do pm_runtime_put_sync_suspend() after
> probe and on close.

That won't work either as a negative autosuspend delay prevents runtime
suspend completely (by grabbing an extra RPM reference).

> > I fail to see how we can implement this using the current toolbox. What
> > you're after here is really a mechanism for selecting between two
> > different runtime PM schemes at runtime:
> > 
> > 1. normal serial RPM, where the controller is active while the
> >port is open (this should be the safe default)
> 
> Agreed. And that is the case already.

Yes, but it's not really the case today as since omap-serial (and
8250-omap) sets a negative autosuspend at probe and hence does not
runtime-suspend when the port is closed. So that's the long-standing bug
which needs fixing.

> > 2. aggressive serial RPM, where the controller is allowed to
> >suspend while the port is open even though this may result in
> >lost characters when waking up on incoming data
> 
> In this case it seems that the only thing needed is to just
> configure the autosuspend delay for the parent port. The use of
> -1 has been around since the start of runtime PM AFAIK, so maybe
> we should just document it. I guess we could also introduce
> pm_runtime_block_autoidle_unless_configured() :)

The implications of a negative autosuspend delay are already documented
(in Documentation/power/runtime_pm.txt); it's just the omap drivers that
gets it wrong when trying to do things which aren't currently supported
(and never have been).

So I still think we need a new mechanism for this.

> > For normal ttys, we need a user-space interface for selecting between
> > the two, and for serdev we may want a way to select the RPM scheme from
> > within the kernel.
> > 
> > Note that with my serdev controller runtime PM patch, serdev core could
> > always opt for aggressive PM (as by default serdev core holds an RPM
> > reference for the controller while the port is open).
> 
> So if your serdev controller was to set the parent autosuspend
> delay on open() and set it back on close() this should work?

Is it really the job of a serdev driver to set the autosuspend delay of
a parent controller? Isn't this somethings which depends on the
characteristics of the controller (possibly configurable by user space)
such as the cost of runtime suspending and resuming?

The patch I posted works with what we have today; if a parent serial
controller driver uses aggressive runtime PM by default or after having
been configured through sysfs to do so.

What I'm getting at here is that the delay should be set by the serial
driver implementing aggressive runtime PM. Then all we need is a
mechanism to determine whether an extra RPM reference should be taken at
tty open or not (configurable by user space, defaulting to yes).

Specifically, the serial drivers themselves would always use
autosuspend and not have to deal with supporting the two RPM schemes
(normal vs aggressive runtime PM).

Thanks,
Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold  [180517 10:12]:
> [ Sorry about the late reply. ]
> 
> On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > * Johan Hovold  [180509 13:12]:
> 
> > > It seems we really should not be using the negative autosuspend to
> > > configure the RPM behaviour the way these drivers do. Perhaps a new
> > > mechanism is needed.
> > 
> > Hmm well simply defaulting to "on" instead of "auto" and setting the
> > autosuspend_ms to 3000 by default might be doable. I think that way
> > we can keep use_autosuspend() in probe. Let's hope there are no
> > existing use cases that would break with that.
> 
> No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> either as that would also prevent the device from runtime suspending
> just as the current negative autosuspend delay does.

Well in that case we should just stick with -1 value for the
autosuspend. And just do pm_runtime_put_sync_suspend() after
probe and on close.

> I fail to see how we can implement this using the current toolbox. What
> you're after here is really a mechanism for selecting between two
> different runtime PM schemes at runtime:
> 
>   1. normal serial RPM, where the controller is active while the
>  port is open (this should be the safe default)

Agreed. And that is the case already.

>   2. aggressive serial RPM, where the controller is allowed to
>  suspend while the port is open even though this may result in
>  lost characters when waking up on incoming data

In this case it seems that the only thing needed is to just
configure the autosuspend delay for the parent port. The use of
-1 has been around since the start of runtime PM AFAIK, so maybe
we should just document it. I guess we could also introduce
pm_runtime_block_autoidle_unless_configured() :)

> For normal ttys, we need a user-space interface for selecting between
> the two, and for serdev we may want a way to select the RPM scheme from
> within the kernel.
> 
> Note that with my serdev controller runtime PM patch, serdev core could
> always opt for aggressive PM (as by default serdev core holds an RPM
> reference for the controller while the port is open).

So if your serdev controller was to set the parent autosuspend
delay on open() and set it back on close() this should work?

Regards,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold  [180517 10:12]:
> [ Sorry about the late reply. ]
> 
> On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > * Johan Hovold  [180509 13:12]:
> 
> > > It seems we really should not be using the negative autosuspend to
> > > configure the RPM behaviour the way these drivers do. Perhaps a new
> > > mechanism is needed.
> > 
> > Hmm well simply defaulting to "on" instead of "auto" and setting the
> > autosuspend_ms to 3000 by default might be doable. I think that way
> > we can keep use_autosuspend() in probe. Let's hope there are no
> > existing use cases that would break with that.
> 
> No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> either as that would also prevent the device from runtime suspending
> just as the current negative autosuspend delay does.

Well in that case we should just stick with -1 value for the
autosuspend. And just do pm_runtime_put_sync_suspend() after
probe and on close.

> I fail to see how we can implement this using the current toolbox. What
> you're after here is really a mechanism for selecting between two
> different runtime PM schemes at runtime:
> 
>   1. normal serial RPM, where the controller is active while the
>  port is open (this should be the safe default)

Agreed. And that is the case already.

>   2. aggressive serial RPM, where the controller is allowed to
>  suspend while the port is open even though this may result in
>  lost characters when waking up on incoming data

In this case it seems that the only thing needed is to just
configure the autosuspend delay for the parent port. The use of
-1 has been around since the start of runtime PM AFAIK, so maybe
we should just document it. I guess we could also introduce
pm_runtime_block_autoidle_unless_configured() :)

> For normal ttys, we need a user-space interface for selecting between
> the two, and for serdev we may want a way to select the RPM scheme from
> within the kernel.
> 
> Note that with my serdev controller runtime PM patch, serdev core could
> always opt for aggressive PM (as by default serdev core holds an RPM
> reference for the controller while the port is open).

So if your serdev controller was to set the parent autosuspend
delay on open() and set it back on close() this should work?

Regards,

Tony

Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Johan Hovold]

On Wed, May 09, 2018 at 07:05:50AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180509 09:20]:
> > On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> > > I think using open/close for runtime pm is good enough for GPS,
> > > since it regularly sends data and draws lots of power anyways.
> > > But devices, that have an out-of-band wakeup signal can do proper
> > > runtime PM of the serial port without loosing characters.
> > 
> > Yeah, there may be some applications where this is possible. And this is
> > not the case for GPS, but not just because of a generally higher power
> > consumption, but due to the fact that we cannot afford having the first
> > message in every report burst be dropped.
> 
> Well most of the phone implementations use one or two out of band
> GPIOs to first wake the UART before any data is sent. For serdev
> this can be called from the serdev consumer write function for TX.
> For RX, the serdev consumer needs to implement an interrupt handler
> and wake up the parent UART before serdev RX.

Right, but the client driver would then wake the parent controller when
expecting RX, while waking its own (serial-attached) device when wanting
to do TX.

Just for the record, we also cleared this up here:

https://marc.info/?l=linux-kernel=152604434504868=2

> > > Note, that OMAP does not reach deep idle states with active
> > > serial port. This is not acceptable for low power devices.
> > 
> > Sure, but note that OMAP is the only serial driver which currently
> > implements this kind of aggressive runtime PM (besides a couple of
> > usb-serial drivers). This means that a serdev driver can never rely on
> > this being the case, and therefore needs to be restrictive about how
> > long the port is kept open if it cares about power at all.
> 
> Well by default we don't allow lossy UART. It needs to be manually
> configured via /sys for the timeout. With serdev, this can all be
> done with no /sys configuration needed for the cases with GPIO
> wake irqs :)

Indeed, but serdev client drivers must also work with serial drivers
which do not implement this kind of aggressive runtime PM, and then the
only way to save power is to close the port when not in use.

Thanks,
Johan

Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Johan Hovold]

On Wed, May 09, 2018 at 07:05:50AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180509 09:20]:
> > On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> > > I think using open/close for runtime pm is good enough for GPS,
> > > since it regularly sends data and draws lots of power anyways.
> > > But devices, that have an out-of-band wakeup signal can do proper
> > > runtime PM of the serial port without loosing characters.
> > 
> > Yeah, there may be some applications where this is possible. And this is
> > not the case for GPS, but not just because of a generally higher power
> > consumption, but due to the fact that we cannot afford having the first
> > message in every report burst be dropped.
> 
> Well most of the phone implementations use one or two out of band
> GPIOs to first wake the UART before any data is sent. For serdev
> this can be called from the serdev consumer write function for TX.
> For RX, the serdev consumer needs to implement an interrupt handler
> and wake up the parent UART before serdev RX.

Right, but the client driver would then wake the parent controller when
expecting RX, while waking its own (serial-attached) device when wanting
to do TX.

Just for the record, we also cleared this up here:

https://marc.info/?l=linux-kernel=152604434504868=2

> > > Note, that OMAP does not reach deep idle states with active
> > > serial port. This is not acceptable for low power devices.
> > 
> > Sure, but note that OMAP is the only serial driver which currently
> > implements this kind of aggressive runtime PM (besides a couple of
> > usb-serial drivers). This means that a serdev driver can never rely on
> > this being the case, and therefore needs to be restrictive about how
> > long the port is kept open if it cares about power at all.
> 
> Well by default we don't allow lossy UART. It needs to be manually
> configured via /sys for the timeout. With serdev, this can all be
> done with no /sys configuration needed for the cases with GPIO
> wake irqs :)

Indeed, but serdev client drivers must also work with serial drivers
which do not implement this kind of aggressive runtime PM, and then the
only way to save power is to close the port when not in use.

Thanks,
Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

[ Sorry about the late reply. ]

On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180509 13:12]:

> > It seems we really should not be using the negative autosuspend to
> > configure the RPM behaviour the way these drivers do. Perhaps a new
> > mechanism is needed.
> 
> Hmm well simply defaulting to "on" instead of "auto" and setting the
> autosuspend_ms to 3000 by default might be doable. I think that way
> we can keep use_autosuspend() in probe. Let's hope there are no
> existing use cases that would break with that.

No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
either as that would also prevent the device from runtime suspending
just as the current negative autosuspend delay does.

I fail to see how we can implement this using the current toolbox. What
you're after here is really a mechanism for selecting between two
different runtime PM schemes at runtime:

1. normal serial RPM, where the controller is active while the
   port is open (this should be the safe default)

2. aggressive serial RPM, where the controller is allowed to
   suspend while the port is open even though this may result in
   lost characters when waking up on incoming data

For normal ttys, we need a user-space interface for selecting between
the two, and for serdev we may want a way to select the RPM scheme from
within the kernel.

Note that with my serdev controller runtime PM patch, serdev core could
always opt for aggressive PM (as by default serdev core holds an RPM
reference for the controller while the port is open).

Thanks,
Johan

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

[ Sorry about the late reply. ]

On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180509 13:12]:

> > It seems we really should not be using the negative autosuspend to
> > configure the RPM behaviour the way these drivers do. Perhaps a new
> > mechanism is needed.
> 
> Hmm well simply defaulting to "on" instead of "auto" and setting the
> autosuspend_ms to 3000 by default might be doable. I think that way
> we can keep use_autosuspend() in probe. Let's hope there are no
> existing use cases that would break with that.

No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
either as that would also prevent the device from runtime suspending
just as the current negative autosuspend delay does.

I fail to see how we can implement this using the current toolbox. What
you're after here is really a mechanism for selecting between two
different runtime PM schemes at runtime:

1. normal serial RPM, where the controller is active while the
   port is open (this should be the safe default)

2. aggressive serial RPM, where the controller is allowed to
   suspend while the port is open even though this may result in
   lost characters when waking up on incoming data

For normal ttys, we need a user-space interface for selecting between
the two, and for serdev we may want a way to select the RPM scheme from
within the kernel.

Note that with my serdev controller runtime PM patch, serdev core could
always opt for aggressive PM (as by default serdev core holds an RPM
reference for the controller while the port is open).

Thanks,
Johan

Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Tony Lindgren]

* Johan Hovold  [180509 09:20]:
> On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> > I think using open/close for runtime pm is good enough for GPS,
> > since it regularly sends data and draws lots of power anyways.
> > But devices, that have an out-of-band wakeup signal can do proper
> > runtime PM of the serial port without loosing characters.
> 
> Yeah, there may be some applications where this is possible. And this is
> not the case for GPS, but not just because of a generally higher power
> consumption, but due to the fact that we cannot afford having the first
> message in every report burst be dropped.

Well most of the phone implementations use one or two out of band
GPIOs to first wake the UART before any data is sent. For serdev
this can be called from the serdev consumer write function for TX.
For RX, the serdev consumer needs to implement an interrupt handler
and wake up the parent UART before serdev RX.

> > Note, that OMAP does not reach deep idle states with active
> > serial port. This is not acceptable for low power devices.
> 
> Sure, but note that OMAP is the only serial driver which currently
> implements this kind of aggressive runtime PM (besides a couple of
> usb-serial drivers). This means that a serdev driver can never rely on
> this being the case, and therefore needs to be restrictive about how
> long the port is kept open if it cares about power at all.

Well by default we don't allow lossy UART. It needs to be manually
configured via /sys for the timeout. With serdev, this can all be
done with no /sys configuration needed for the cases with GPIO
wake irqs :)

Regards,

Tony

Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Tony Lindgren]

* Johan Hovold  [180509 09:20]:
> On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> > I think using open/close for runtime pm is good enough for GPS,
> > since it regularly sends data and draws lots of power anyways.
> > But devices, that have an out-of-band wakeup signal can do proper
> > runtime PM of the serial port without loosing characters.
> 
> Yeah, there may be some applications where this is possible. And this is
> not the case for GPS, but not just because of a generally higher power
> consumption, but due to the fact that we cannot afford having the first
> message in every report burst be dropped.

Well most of the phone implementations use one or two out of band
GPIOs to first wake the UART before any data is sent. For serdev
this can be called from the serdev consumer write function for TX.
For RX, the serdev consumer needs to implement an interrupt handler
and wake up the parent UART before serdev RX.

> > Note, that OMAP does not reach deep idle states with active
> > serial port. This is not acceptable for low power devices.
> 
> Sure, but note that OMAP is the only serial driver which currently
> implements this kind of aggressive runtime PM (besides a couple of
> usb-serial drivers). This means that a serdev driver can never rely on
> this being the case, and therefore needs to be restrictive about how
> long the port is kept open if it cares about power at all.

Well by default we don't allow lossy UART. It needs to be manually
configured via /sys for the timeout. With serdev, this can all be
done with no /sys configuration needed for the cases with GPIO
wake irqs :)

Regards,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold  [180509 13:12]:
> While this seems to fix the idling of closed ports here too, I'm not
> sure we can move use_autosuspend to startup() like this.
> 
> First, this flag should be set before registering the tty so that udev
> can be used to update the attributes.
> 
> Second, this prevents setting the autosuspend delay through sysfs when
> the port is closed (when autosuspend is disabled).

Yeah I noticed that too yesterday.

> It seems we really should not be using the negative autosuspend to
> configure the RPM behaviour the way these drivers do. Perhaps a new
> mechanism is needed.

Hmm well simply defaulting to "on" instead of "auto" and setting the
autosuspend_ms to 3000 by default might be doable. I think that way
we can keep use_autosuspend() in probe. Let's hope there are no
existing use cases that would break with that.

> But I'm afraid I don't have time to look at this today.

Sure np thanks,

Tony

Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Tony Lindgren]

* Johan Hovold  [180509 13:12]:
> While this seems to fix the idling of closed ports here too, I'm not
> sure we can move use_autosuspend to startup() like this.
> 
> First, this flag should be set before registering the tty so that udev
> can be used to update the attributes.
> 
> Second, this prevents setting the autosuspend delay through sysfs when
> the port is closed (when autosuspend is disabled).

Yeah I noticed that too yesterday.

> It seems we really should not be using the negative autosuspend to
> configure the RPM behaviour the way these drivers do. Perhaps a new
> mechanism is needed.

Hmm well simply defaulting to "on" instead of "auto" and setting the
autosuspend_ms to 3000 by default might be doable. I think that way
we can keep use_autosuspend() in probe. Let's hope there are no
existing use cases that would break with that.

> But I'm afraid I don't have time to look at this today.

Sure np thanks,

Tony

OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

[ Updating subject and adding linux-serial, linux-pm and linux-omap. ]

On Tue, May 08, 2018 at 09:49:04AM -0700, Tony Lindgren wrote:
> * Tony Lindgren  [180508 08:54]:
> > * Tony Lindgren  [180508 08:47]:
> > > * Tony Lindgren  [180508 08:22]:
> > > > * Johan Hovold  [180508 07:00]:
> > > > > With the negative autosuspend set in both omap drivers probe 
> > > > > functions,
> > > > > this is the expected behaviour. Which I think we must fix.
> > > > 
> > > > Yes indeed. I've been using my script for years now and have
> > > > completely missed the fact that the unused ports are not idled
> > > > at all on start-up when unused.
> > > 
> > > This might be all that's needed, care to try it and if it works
> > > I'll send out two separate proper patches?
> > > 
> > > I'm seeing this now on my bbb after temporarily disabling my
> > > UART idle init script:
> > > 
> > > # rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
> > > 0x44e004b4 = 0x0002
> > > 
> > > # rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
> > > 0x44e0006c = 0x0003
> > > 0x44e00070 = 0x0003
> > > 0x44e00074 = 0x0003
> > > 0x44e00078 = 0x0003
> > > 
> > > # rwmem -s32 0x44e00038   # uart 6 on l4_per
> > > 0x44e00038 = 0x0003
> > 
> > Hmm I forgot to remove status = "disabled" from the other ports,
> > still not idling the unused ports with the patch below it seems.
> 
> No need to enable/disable autosuspend except in startup and shutdown
> I think. The patch below works for me, now includes removal of the
> status = "disabled" flags too. Only tested with 8250_omap on bbb
> so far.
> 
> I wonder if other places still need fixing for autosuspend
> like console support?

While this seems to fix the idling of closed ports here too, I'm not
sure we can move use_autosuspend to startup() like this.

First, this flag should be set before registering the tty so that udev
can be used to update the attributes.

Second, this prevents setting the autosuspend delay through sysfs when
the port is closed (when autosuspend is disabled).

It seems we really should not be using the negative autosuspend to
configure the RPM behaviour the way these drivers do. Perhaps a new
mechanism is needed.

But I'm afraid I don't have time to look at this today.

Thanks,
Johan

> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -605,6 +605,7 @@ static int omap_8250_startup(struct uart_port *port)
>   return ret;
>   }
>  
> + pm_runtime_use_autosuspend(port->dev);
>   pm_runtime_get_sync(port->dev);
>  
>   up->mcr = 0;
> @@ -685,8 +686,8 @@ static void omap_8250_shutdown(struct uart_port *port)
>   serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
>   serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>  
> - pm_runtime_mark_last_busy(port->dev);
> - pm_runtime_put_autosuspend(port->dev);
> + pm_runtime_dont_use_autosuspend(port->dev);
> + pm_runtime_put_sync(port->dev);
>   free_irq(port->irq, port);
>   dev_pm_clear_wake_irq(port->dev);
>  }
> @@ -1226,7 +1227,7 @@ static int omap8250_probe(struct platform_device *pdev)
>   spin_lock_init(>rx_dma_lock);
>  
>   device_init_wakeup(>dev, true);
> - pm_runtime_use_autosuspend(>dev);
> + /* See omap_8250_startup and shutdown for autosuspend */
>   pm_runtime_set_autosuspend_delay(>dev, -1);
>  
>   pm_runtime_irq_safe(>dev);
> @@ -1265,8 +1266,8 @@ static int omap8250_probe(struct platform_device *pdev)
>   }
>   priv->line = ret;
>   platform_set_drvdata(pdev, priv);
> - pm_runtime_mark_last_busy(>dev);
> - pm_runtime_put_autosuspend(>dev);
> + pm_runtime_put_sync(>dev);
> +
>   return 0;
>  err:

OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))

[Johan Hovold]

[ Updating subject and adding linux-serial, linux-pm and linux-omap. ]

On Tue, May 08, 2018 at 09:49:04AM -0700, Tony Lindgren wrote:
> * Tony Lindgren  [180508 08:54]:
> > * Tony Lindgren  [180508 08:47]:
> > > * Tony Lindgren  [180508 08:22]:
> > > > * Johan Hovold  [180508 07:00]:
> > > > > With the negative autosuspend set in both omap drivers probe 
> > > > > functions,
> > > > > this is the expected behaviour. Which I think we must fix.
> > > > 
> > > > Yes indeed. I've been using my script for years now and have
> > > > completely missed the fact that the unused ports are not idled
> > > > at all on start-up when unused.
> > > 
> > > This might be all that's needed, care to try it and if it works
> > > I'll send out two separate proper patches?
> > > 
> > > I'm seeing this now on my bbb after temporarily disabling my
> > > UART idle init script:
> > > 
> > > # rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
> > > 0x44e004b4 = 0x0002
> > > 
> > > # rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
> > > 0x44e0006c = 0x0003
> > > 0x44e00070 = 0x0003
> > > 0x44e00074 = 0x0003
> > > 0x44e00078 = 0x0003
> > > 
> > > # rwmem -s32 0x44e00038   # uart 6 on l4_per
> > > 0x44e00038 = 0x0003
> > 
> > Hmm I forgot to remove status = "disabled" from the other ports,
> > still not idling the unused ports with the patch below it seems.
> 
> No need to enable/disable autosuspend except in startup and shutdown
> I think. The patch below works for me, now includes removal of the
> status = "disabled" flags too. Only tested with 8250_omap on bbb
> so far.
> 
> I wonder if other places still need fixing for autosuspend
> like console support?

While this seems to fix the idling of closed ports here too, I'm not
sure we can move use_autosuspend to startup() like this.

First, this flag should be set before registering the tty so that udev
can be used to update the attributes.

Second, this prevents setting the autosuspend delay through sysfs when
the port is closed (when autosuspend is disabled).

It seems we really should not be using the negative autosuspend to
configure the RPM behaviour the way these drivers do. Perhaps a new
mechanism is needed.

But I'm afraid I don't have time to look at this today.

Thanks,
Johan

> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -605,6 +605,7 @@ static int omap_8250_startup(struct uart_port *port)
>   return ret;
>   }
>  
> + pm_runtime_use_autosuspend(port->dev);
>   pm_runtime_get_sync(port->dev);
>  
>   up->mcr = 0;
> @@ -685,8 +686,8 @@ static void omap_8250_shutdown(struct uart_port *port)
>   serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
>   serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>  
> - pm_runtime_mark_last_busy(port->dev);
> - pm_runtime_put_autosuspend(port->dev);
> + pm_runtime_dont_use_autosuspend(port->dev);
> + pm_runtime_put_sync(port->dev);
>   free_irq(port->irq, port);
>   dev_pm_clear_wake_irq(port->dev);
>  }
> @@ -1226,7 +1227,7 @@ static int omap8250_probe(struct platform_device *pdev)
>   spin_lock_init(>rx_dma_lock);
>  
>   device_init_wakeup(>dev, true);
> - pm_runtime_use_autosuspend(>dev);
> + /* See omap_8250_startup and shutdown for autosuspend */
>   pm_runtime_set_autosuspend_delay(>dev, -1);
>  
>   pm_runtime_irq_safe(>dev);
> @@ -1265,8 +1266,8 @@ static int omap8250_probe(struct platform_device *pdev)
>   }
>   priv->line = ret;
>   platform_set_drvdata(pdev, priv);
> - pm_runtime_mark_last_busy(>dev);
> - pm_runtime_put_autosuspend(>dev);
> + pm_runtime_put_sync(>dev);
> +
>   return 0;
>  err:

Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Johan Hovold]

On Wed, May 09, 2018 at 11:18:31AM +0200, Johan Hovold wrote:

> I've cooked up a patch which I'll be sending as a reply to this mail.

Forgot to add the in-reply-to header of course. For the interested, the
patch can be found here:

https://lkml.kernel.org/r/20180509094419.13470-1-jo...@kernel.org

Johan

Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Johan Hovold]

On Wed, May 09, 2018 at 11:18:31AM +0200, Johan Hovold wrote:

> I've cooked up a patch which I'll be sending as a reply to this mail.

Forgot to add the in-reply-to header of course. For the interested, the
patch can be found here:

https://lkml.kernel.org/r/20180509094419.13470-1-jo...@kernel.org

Johan

Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Johan Hovold]

[ Changing the subject line as this is thread is no longer about DT
  bindings.

  Also adding linux-serial and linux-pm while keeping some context. ]

On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, May 07, 2018 at 06:34:39PM +0200, Johan Hovold wrote:
> > On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold  [180507 03:03]:
> > > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > > 
> > > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > > us to enable runtime PM by default (once implemented), since we know
> > > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > > with sideband wakeup gpios).
> > > > 
> > > > I'm not sure we want generic runtime-pm support for the controllers in
> > > > the sense that the slave device state is always reflected by the serial
> > > > controller. Similar as for i2c and spi, we really only want to keep the
> > > > controller active when we are doing I/O, but we may want to keep a
> > > > client active for longer.
> > > 
> > > Yeah i2c seems to do the right thing where the bus takes care
> > > of runtime PM.
> > 
> > Yeah, but since serial is async in contrast to i2c/spi, we may not be
> > able to push this entirely into core. The serdev drivers may need to
> > indicate when they expect or need to do I/O by opening and closing the
> > port. And this is how I implemented these first couple of gnss drivers.
> >
> > Also note that most serial driver do not do runtime pm while the port is
> > open (as OMAP does), and doing so also has the drawbacks of lost
> > characters etc. as Sebastian mentioned.
> 
> I think using open/close for runtime pm is good enough for GPS,
> since it regularly sends data and draws lots of power anyways.
> But devices, that have an out-of-band wakeup signal can do proper
> runtime PM of the serial port without loosing characters.

Yeah, there may be some applications where this is possible. And this is
not the case for GPS, but not just because of a generally higher power
consumption, but due to the fact that we cannot afford having the first
message in every report burst be dropped.

> Note, that OMAP does not reach deep idle states with active
> serial port. This is not acceptable for low power devices.

Sure, but note that OMAP is the only serial driver which currently
implements this kind of aggressive runtime PM (besides a couple of
usb-serial drivers). This means that a serdev driver can never rely on
this being the case, and therefore needs to be restrictive about how
long the port is kept open if it cares about power at all.

> > > > Take the u-blox driver in this series for example. As I'm using runtime
> > > > PM to manage device power, user-space can chose to prevent the receiver
> > > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > > in setups without a backup battery (by means of the power/control
> > > > attribute).
> > > 
> > > Sorry I don't seem to have that one, care to paste the subject
> > > line of that patch?
> > 
> > "[PATCH 5/7] gnss: add driver for u-blox receivers"
> > 
> > https://lkml.kernel.org/r/20180424163458.11947-6-jo...@kernel.org
> > 
> > > > Note that serdev not enabling runtime pm for controllers is roughly
> > > > equivalent to setting the .ignore_children flag, which is what we do for
> > > > i2c and spi controller, and possibly what we want here too.
> 
> For I2C/SPI this works, since receive operations are initiated by
> the controller. Unfortunately its harder to implement for async
> serial. But I agree, that we may want to have runtime PM for the
> serdev client and .ignore_children is the way to go.

It's really about adding runtime PM support to serdev controllers.
Serdev client drivers can already use runtime PM (as mentioned above).

The ignore_children flag would then allow the controller RPM state to be
managed independently of the child/slave device state when more fine
grained RPM control is desired.

> I think the client API should allow two things:
> 
> 1. minimal runtime PM support: The controller is runtime enabled
> on serdev open and disabled on serdev close. This may be enough
> for some clients and useful for writing new drivers.

Right, and we already have something like this today by means of how the
serial driver implement runtime PM (i.e. they are generally active while
the port is open).

> 2. full runtime PM support: The controller is sleeping by default
> even with serdev open. The calls to write/change port settings/...
> automatically enables the device, similar to i2c/spi. But there
> must be additional functions to enable/disable runtime PM based
> on a wakeup gpio or similar out-of-band information. It may be
> enough to just provide:
> 
> int serdev_pm_runtime_get_sync(struct serdev_device *serdev) {

Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)

[Johan Hovold]

[ Changing the subject line as this is thread is no longer about DT
  bindings.

  Also adding linux-serial and linux-pm while keeping some context. ]

On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, May 07, 2018 at 06:34:39PM +0200, Johan Hovold wrote:
> > On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold  [180507 03:03]:
> > > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > > 
> > > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > > us to enable runtime PM by default (once implemented), since we know
> > > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > > with sideband wakeup gpios).
> > > > 
> > > > I'm not sure we want generic runtime-pm support for the controllers in
> > > > the sense that the slave device state is always reflected by the serial
> > > > controller. Similar as for i2c and spi, we really only want to keep the
> > > > controller active when we are doing I/O, but we may want to keep a
> > > > client active for longer.
> > > 
> > > Yeah i2c seems to do the right thing where the bus takes care
> > > of runtime PM.
> > 
> > Yeah, but since serial is async in contrast to i2c/spi, we may not be
> > able to push this entirely into core. The serdev drivers may need to
> > indicate when they expect or need to do I/O by opening and closing the
> > port. And this is how I implemented these first couple of gnss drivers.
> >
> > Also note that most serial driver do not do runtime pm while the port is
> > open (as OMAP does), and doing so also has the drawbacks of lost
> > characters etc. as Sebastian mentioned.
> 
> I think using open/close for runtime pm is good enough for GPS,
> since it regularly sends data and draws lots of power anyways.
> But devices, that have an out-of-band wakeup signal can do proper
> runtime PM of the serial port without loosing characters.

Yeah, there may be some applications where this is possible. And this is
not the case for GPS, but not just because of a generally higher power
consumption, but due to the fact that we cannot afford having the first
message in every report burst be dropped.

> Note, that OMAP does not reach deep idle states with active
> serial port. This is not acceptable for low power devices.

Sure, but note that OMAP is the only serial driver which currently
implements this kind of aggressive runtime PM (besides a couple of
usb-serial drivers). This means that a serdev driver can never rely on
this being the case, and therefore needs to be restrictive about how
long the port is kept open if it cares about power at all.

> > > > Take the u-blox driver in this series for example. As I'm using runtime
> > > > PM to manage device power, user-space can chose to prevent the receiver
> > > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > > in setups without a backup battery (by means of the power/control
> > > > attribute).
> > > 
> > > Sorry I don't seem to have that one, care to paste the subject
> > > line of that patch?
> > 
> > "[PATCH 5/7] gnss: add driver for u-blox receivers"
> > 
> > https://lkml.kernel.org/r/20180424163458.11947-6-jo...@kernel.org
> > 
> > > > Note that serdev not enabling runtime pm for controllers is roughly
> > > > equivalent to setting the .ignore_children flag, which is what we do for
> > > > i2c and spi controller, and possibly what we want here too.
> 
> For I2C/SPI this works, since receive operations are initiated by
> the controller. Unfortunately its harder to implement for async
> serial. But I agree, that we may want to have runtime PM for the
> serdev client and .ignore_children is the way to go.

It's really about adding runtime PM support to serdev controllers.
Serdev client drivers can already use runtime PM (as mentioned above).

The ignore_children flag would then allow the controller RPM state to be
managed independently of the child/slave device state when more fine
grained RPM control is desired.

> I think the client API should allow two things:
> 
> 1. minimal runtime PM support: The controller is runtime enabled
> on serdev open and disabled on serdev close. This may be enough
> for some clients and useful for writing new drivers.

Right, and we already have something like this today by means of how the
serial driver implement runtime PM (i.e. they are generally active while
the port is open).

> 2. full runtime PM support: The controller is sleeping by default
> even with serdev open. The calls to write/change port settings/...
> automatically enables the device, similar to i2c/spi. But there
> must be additional functions to enable/disable runtime PM based
> on a wakeup gpio or similar out-of-band information. It may be
> enough to just provide:
> 
> int serdev_pm_runtime_get_sync(struct serdev_device *serdev) {
> 

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Tony Lindgren  [180508 08:54]:
> * Tony Lindgren  [180508 08:47]:
> > * Tony Lindgren  [180508 08:22]:
> > > * Johan Hovold  [180508 07:00]:
> > > > With the negative autosuspend set in both omap drivers probe functions,
> > > > this is the expected behaviour. Which I think we must fix.
> > > 
> > > Yes indeed. I've been using my script for years now and have
> > > completely missed the fact that the unused ports are not idled
> > > at all on start-up when unused.
> > 
> > This might be all that's needed, care to try it and if it works
> > I'll send out two separate proper patches?
> > 
> > I'm seeing this now on my bbb after temporarily disabling my
> > UART idle init script:
> > 
> > # rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
> > 0x44e004b4 = 0x0002
> > 
> > # rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
> > 0x44e0006c = 0x0003
> > 0x44e00070 = 0x0003
> > 0x44e00074 = 0x0003
> > 0x44e00078 = 0x0003
> > 
> > # rwmem -s32 0x44e00038   # uart 6 on l4_per
> > 0x44e00038 = 0x0003
> 
> Hmm I forgot to remove status = "disabled" from the other ports,
> still not idling the unused ports with the patch below it seems.

No need to enable/disable autosuspend except in startup and shutdown
I think. The patch below works for me, now includes removal of the
status = "disabled" flags too. Only tested with 8250_omap on bbb
so far.

I wonder if other places still need fixing for autosuspend
like console support?

Regards,

Tony

8< ---
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -347,7 +347,6 @@
clock-frequency = <4800>;
reg = <0x44e09000 0x2000>;
interrupts = <72>;
-   status = "disabled";
dmas = < 26 0>, < 27 0>;
dma-names = "tx", "rx";
};
@@ -358,7 +357,6 @@
clock-frequency = <4800>;
reg = <0x48022000 0x2000>;
interrupts = <73>;
-   status = "disabled";
dmas = < 28 0>, < 29 0>;
dma-names = "tx", "rx";
};
@@ -369,7 +367,6 @@
clock-frequency = <4800>;
reg = <0x48024000 0x2000>;
interrupts = <74>;
-   status = "disabled";
dmas = < 30 0>, < 31 0>;
dma-names = "tx", "rx";
};
@@ -380,7 +377,6 @@
clock-frequency = <4800>;
reg = <0x481a6000 0x2000>;
interrupts = <44>;
-   status = "disabled";
};
 
uart4: serial@481a8000 {
@@ -389,7 +385,6 @@
clock-frequency = <4800>;
reg = <0x481a8000 0x2000>;
interrupts = <45>;
-   status = "disabled";
};
 
uart5: serial@481aa000 {
@@ -398,7 +393,6 @@
clock-frequency = <4800>;
reg = <0x481aa000 0x2000>;
interrupts = <46>;
-   status = "disabled";
};
 
i2c0: i2c@44e0b000 {
diff --git a/drivers/tty/serial/8250/8250_omap.c 
b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -605,6 +605,7 @@ static int omap_8250_startup(struct uart_port *port)
return ret;
}
 
+   pm_runtime_use_autosuspend(port->dev);
pm_runtime_get_sync(port->dev);
 
up->mcr = 0;
@@ -685,8 +686,8 @@ static void omap_8250_shutdown(struct uart_port *port)
serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
 
-   pm_runtime_mark_last_busy(port->dev);
-   pm_runtime_put_autosuspend(port->dev);
+   pm_runtime_dont_use_autosuspend(port->dev);
+   pm_runtime_put_sync(port->dev);
free_irq(port->irq, port);
dev_pm_clear_wake_irq(port->dev);
 }
@@ -1226,7 +1227,7 @@ static int omap8250_probe(struct platform_device *pdev)
spin_lock_init(>rx_dma_lock);
 
device_init_wakeup(>dev, true);
-   pm_runtime_use_autosuspend(>dev);
+   /* See omap_8250_startup and shutdown for autosuspend */
pm_runtime_set_autosuspend_delay(>dev, -1);
 
pm_runtime_irq_safe(>dev);
@@ -1265,8 +1266,8 @@ static int omap8250_probe(struct platform_device *pdev)
}
priv->line = ret;
platform_set_drvdata(pdev, priv);
-   pm_runtime_mark_last_busy(>dev);
-   

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Tony Lindgren  [180508 08:54]:
> * Tony Lindgren  [180508 08:47]:
> > * Tony Lindgren  [180508 08:22]:
> > > * Johan Hovold  [180508 07:00]:
> > > > With the negative autosuspend set in both omap drivers probe functions,
> > > > this is the expected behaviour. Which I think we must fix.
> > > 
> > > Yes indeed. I've been using my script for years now and have
> > > completely missed the fact that the unused ports are not idled
> > > at all on start-up when unused.
> > 
> > This might be all that's needed, care to try it and if it works
> > I'll send out two separate proper patches?
> > 
> > I'm seeing this now on my bbb after temporarily disabling my
> > UART idle init script:
> > 
> > # rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
> > 0x44e004b4 = 0x0002
> > 
> > # rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
> > 0x44e0006c = 0x0003
> > 0x44e00070 = 0x0003
> > 0x44e00074 = 0x0003
> > 0x44e00078 = 0x0003
> > 
> > # rwmem -s32 0x44e00038   # uart 6 on l4_per
> > 0x44e00038 = 0x0003
> 
> Hmm I forgot to remove status = "disabled" from the other ports,
> still not idling the unused ports with the patch below it seems.

No need to enable/disable autosuspend except in startup and shutdown
I think. The patch below works for me, now includes removal of the
status = "disabled" flags too. Only tested with 8250_omap on bbb
so far.

I wonder if other places still need fixing for autosuspend
like console support?

Regards,

Tony

8< ---
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -347,7 +347,6 @@
clock-frequency = <4800>;
reg = <0x44e09000 0x2000>;
interrupts = <72>;
-   status = "disabled";
dmas = < 26 0>, < 27 0>;
dma-names = "tx", "rx";
};
@@ -358,7 +357,6 @@
clock-frequency = <4800>;
reg = <0x48022000 0x2000>;
interrupts = <73>;
-   status = "disabled";
dmas = < 28 0>, < 29 0>;
dma-names = "tx", "rx";
};
@@ -369,7 +367,6 @@
clock-frequency = <4800>;
reg = <0x48024000 0x2000>;
interrupts = <74>;
-   status = "disabled";
dmas = < 30 0>, < 31 0>;
dma-names = "tx", "rx";
};
@@ -380,7 +377,6 @@
clock-frequency = <4800>;
reg = <0x481a6000 0x2000>;
interrupts = <44>;
-   status = "disabled";
};
 
uart4: serial@481a8000 {
@@ -389,7 +385,6 @@
clock-frequency = <4800>;
reg = <0x481a8000 0x2000>;
interrupts = <45>;
-   status = "disabled";
};
 
uart5: serial@481aa000 {
@@ -398,7 +393,6 @@
clock-frequency = <4800>;
reg = <0x481aa000 0x2000>;
interrupts = <46>;
-   status = "disabled";
};
 
i2c0: i2c@44e0b000 {
diff --git a/drivers/tty/serial/8250/8250_omap.c 
b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -605,6 +605,7 @@ static int omap_8250_startup(struct uart_port *port)
return ret;
}
 
+   pm_runtime_use_autosuspend(port->dev);
pm_runtime_get_sync(port->dev);
 
up->mcr = 0;
@@ -685,8 +686,8 @@ static void omap_8250_shutdown(struct uart_port *port)
serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
 
-   pm_runtime_mark_last_busy(port->dev);
-   pm_runtime_put_autosuspend(port->dev);
+   pm_runtime_dont_use_autosuspend(port->dev);
+   pm_runtime_put_sync(port->dev);
free_irq(port->irq, port);
dev_pm_clear_wake_irq(port->dev);
 }
@@ -1226,7 +1227,7 @@ static int omap8250_probe(struct platform_device *pdev)
spin_lock_init(>rx_dma_lock);
 
device_init_wakeup(>dev, true);
-   pm_runtime_use_autosuspend(>dev);
+   /* See omap_8250_startup and shutdown for autosuspend */
pm_runtime_set_autosuspend_delay(>dev, -1);
 
pm_runtime_irq_safe(>dev);
@@ -1265,8 +1266,8 @@ static int omap8250_probe(struct platform_device *pdev)
}
priv->line = ret;
platform_set_drvdata(pdev, priv);
-   pm_runtime_mark_last_busy(>dev);
-   pm_runtime_put_autosuspend(>dev);
+   pm_runtime_put_sync(>dev);
+

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Sebastian Reichel]

Hi,

On Mon, May 07, 2018 at 06:34:39PM +0200, Johan Hovold wrote:
> On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > * Johan Hovold  [180507 03:03]:
> > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > 
> > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > us to enable runtime PM by default (once implemented), since we know
> > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > with sideband wakeup gpios).
> > > 
> > > I'm not sure we want generic runtime-pm support for the controllers in
> > > the sense that the slave device state is always reflected by the serial
> > > controller. Similar as for i2c and spi, we really only want to keep the
> > > controller active when we are doing I/O, but we may want to keep a
> > > client active for longer.
> > 
> > Yeah i2c seems to do the right thing where the bus takes care
> > of runtime PM.
> 
> Yeah, but since serial is async in contrast to i2c/spi, we may not be
> able to push this entirely into core. The serdev drivers may need to
> indicate when they expect or need to do I/O by opening and closing the
> port. And this is how I implemented these first couple of gnss drivers.
>
> Also note that most serial driver do not do runtime pm while the port is
> open (as OMAP does), and doing so also has the drawbacks of lost
> characters etc. as Sebastian mentioned.

I think using open/close for runtime pm is good enough for GPS,
since it regularly sends data and draws lots of power anyways.
But devices, that have an out-of-band wakeup signal can do proper
runtime PM of the serial port without loosing characters.

Note, that OMAP does not reach deep idle states with active
serial port. This is not acceptable for low power devices.

> > > Take the u-blox driver in this series for example. As I'm using runtime
> > > PM to manage device power, user-space can chose to prevent the receiver
> > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > in setups without a backup battery (by means of the power/control
> > > attribute).
> > 
> > Sorry I don't seem to have that one, care to paste the subject
> > line of that patch?
> 
>   "[PATCH 5/7] gnss: add driver for u-blox receivers"
> 
>   https://lkml.kernel.org/r/20180424163458.11947-6-jo...@kernel.org
> 
> > > Note that serdev not enabling runtime pm for controllers is roughly
> > > equivalent to setting the .ignore_children flag, which is what we do for
> > > i2c and spi controller, and possibly what we want here too.

For I2C/SPI this works, since receive operations are initiated by
the controller. Unfortunately its harder to implement for async
serial. But I agree, that we may want to have runtime PM for the
serdev client and .ignore_children is the way to go.

I think the client API should allow two things:

1. minimal runtime PM support: The controller is runtime enabled
on serdev open and disabled on serdev close. This may be enough
for some clients and useful for writing new drivers.

2. full runtime PM support: The controller is sleeping by default
even with serdev open. The calls to write/change port settings/...
automatically enables the device, similar to i2c/spi. But there
must be additional functions to enable/disable runtime PM based
on a wakeup gpio or similar out-of-band information. It may be
enough to just provide:

int serdev_pm_runtime_get_sync(struct serdev_device *serdev) {
pm_runtime_get_sync(>dev);
}

int serdev_pm_runtime_put_autosuspend(struct serdev_device *serdev) {
pm_runtime_put_autosuspend(>dev);
}

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Sebastian Reichel]

Hi,

On Mon, May 07, 2018 at 06:34:39PM +0200, Johan Hovold wrote:
> On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > * Johan Hovold  [180507 03:03]:
> > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > 
> > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > us to enable runtime PM by default (once implemented), since we know
> > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > with sideband wakeup gpios).
> > > 
> > > I'm not sure we want generic runtime-pm support for the controllers in
> > > the sense that the slave device state is always reflected by the serial
> > > controller. Similar as for i2c and spi, we really only want to keep the
> > > controller active when we are doing I/O, but we may want to keep a
> > > client active for longer.
> > 
> > Yeah i2c seems to do the right thing where the bus takes care
> > of runtime PM.
> 
> Yeah, but since serial is async in contrast to i2c/spi, we may not be
> able to push this entirely into core. The serdev drivers may need to
> indicate when they expect or need to do I/O by opening and closing the
> port. And this is how I implemented these first couple of gnss drivers.
>
> Also note that most serial driver do not do runtime pm while the port is
> open (as OMAP does), and doing so also has the drawbacks of lost
> characters etc. as Sebastian mentioned.

I think using open/close for runtime pm is good enough for GPS,
since it regularly sends data and draws lots of power anyways.
But devices, that have an out-of-band wakeup signal can do proper
runtime PM of the serial port without loosing characters.

Note, that OMAP does not reach deep idle states with active
serial port. This is not acceptable for low power devices.

> > > Take the u-blox driver in this series for example. As I'm using runtime
> > > PM to manage device power, user-space can chose to prevent the receiver
> > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > in setups without a backup battery (by means of the power/control
> > > attribute).
> > 
> > Sorry I don't seem to have that one, care to paste the subject
> > line of that patch?
> 
>   "[PATCH 5/7] gnss: add driver for u-blox receivers"
> 
>   https://lkml.kernel.org/r/20180424163458.11947-6-jo...@kernel.org
> 
> > > Note that serdev not enabling runtime pm for controllers is roughly
> > > equivalent to setting the .ignore_children flag, which is what we do for
> > > i2c and spi controller, and possibly what we want here too.

For I2C/SPI this works, since receive operations are initiated by
the controller. Unfortunately its harder to implement for async
serial. But I agree, that we may want to have runtime PM for the
serdev client and .ignore_children is the way to go.

I think the client API should allow two things:

1. minimal runtime PM support: The controller is runtime enabled
on serdev open and disabled on serdev close. This may be enough
for some clients and useful for writing new drivers.

2. full runtime PM support: The controller is sleeping by default
even with serdev open. The calls to write/change port settings/...
automatically enables the device, similar to i2c/spi. But there
must be additional functions to enable/disable runtime PM based
on a wakeup gpio or similar out-of-band information. It may be
enough to just provide:

int serdev_pm_runtime_get_sync(struct serdev_device *serdev) {
pm_runtime_get_sync(>dev);
}

int serdev_pm_runtime_put_autosuspend(struct serdev_device *serdev) {
pm_runtime_put_autosuspend(>dev);
}

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Tony Lindgren  [180508 08:47]:
> * Tony Lindgren  [180508 08:22]:
> > * Johan Hovold  [180508 07:00]:
> > > With the negative autosuspend set in both omap drivers probe functions,
> > > this is the expected behaviour. Which I think we must fix.
> > 
> > Yes indeed. I've been using my script for years now and have
> > completely missed the fact that the unused ports are not idled
> > at all on start-up when unused.
> 
> This might be all that's needed, care to try it and if it works
> I'll send out two separate proper patches?
> 
> I'm seeing this now on my bbb after temporarily disabling my
> UART idle init script:
> 
> # rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
> 0x44e004b4 = 0x0002
> 
> # rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
> 0x44e0006c = 0x0003
> 0x44e00070 = 0x0003
> 0x44e00074 = 0x0003
> 0x44e00078 = 0x0003
> 
> # rwmem -s32 0x44e00038   # uart 6 on l4_per
> 0x44e00038 = 0x0003

Hmm I forgot to remove status = "disabled" from the other ports,
still not idling the unused ports with the patch below it seems.

Regards,

Tony

> 8< --
> diff --git a/drivers/tty/serial/8250/8250_omap.c 
> b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -685,8 +685,7 @@ static void omap_8250_shutdown(struct uart_port *port)
>   serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
>   serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>  
> - pm_runtime_mark_last_busy(port->dev);
> - pm_runtime_put_autosuspend(port->dev);
> + pm_runtime_put_sync(port->dev);
>   free_irq(port->irq, port);
>   dev_pm_clear_wake_irq(port->dev);
>  }
> @@ -1265,8 +1264,7 @@ static int omap8250_probe(struct platform_device *pdev)
>   }
>   priv->line = ret;
>   platform_set_drvdata(pdev, priv);
> - pm_runtime_mark_last_busy(>dev);
> - pm_runtime_put_autosuspend(>dev);
> + pm_runtime_put_sync(>dev);
>   return 0;
>  err:
>   pm_runtime_dont_use_autosuspend(>dev);
> diff --git a/drivers/tty/serial/omap-serial.c 
> b/drivers/tty/serial/omap-serial.c
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -821,8 +821,7 @@ static void serial_omap_shutdown(struct uart_port *port)
>   if (serial_in(up, UART_LSR) & UART_LSR_DR)
>   (void) serial_in(up, UART_RX);
>  
> - pm_runtime_mark_last_busy(up->dev);
> - pm_runtime_put_autosuspend(up->dev);
> + pm_runtime_put_sync(up->dev);
>   free_irq(up->port.irq, up);
>   dev_pm_clear_wake_irq(up->dev);
>  }
> @@ -1751,8 +1750,7 @@ static int serial_omap_probe(struct platform_device 
> *pdev)
>   if (ret != 0)
>   goto err_add_port;
>  
> - pm_runtime_mark_last_busy(up->dev);
> - pm_runtime_put_autosuspend(up->dev);
> + pm_runtime_put_sync(>dev);
>   return 0;
>  
>  err_add_port:
> -- 
> 2.17.0

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Tony Lindgren  [180508 08:47]:
> * Tony Lindgren  [180508 08:22]:
> > * Johan Hovold  [180508 07:00]:
> > > With the negative autosuspend set in both omap drivers probe functions,
> > > this is the expected behaviour. Which I think we must fix.
> > 
> > Yes indeed. I've been using my script for years now and have
> > completely missed the fact that the unused ports are not idled
> > at all on start-up when unused.
> 
> This might be all that's needed, care to try it and if it works
> I'll send out two separate proper patches?
> 
> I'm seeing this now on my bbb after temporarily disabling my
> UART idle init script:
> 
> # rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
> 0x44e004b4 = 0x0002
> 
> # rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
> 0x44e0006c = 0x0003
> 0x44e00070 = 0x0003
> 0x44e00074 = 0x0003
> 0x44e00078 = 0x0003
> 
> # rwmem -s32 0x44e00038   # uart 6 on l4_per
> 0x44e00038 = 0x0003

Hmm I forgot to remove status = "disabled" from the other ports,
still not idling the unused ports with the patch below it seems.

Regards,

Tony

> 8< --
> diff --git a/drivers/tty/serial/8250/8250_omap.c 
> b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -685,8 +685,7 @@ static void omap_8250_shutdown(struct uart_port *port)
>   serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
>   serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>  
> - pm_runtime_mark_last_busy(port->dev);
> - pm_runtime_put_autosuspend(port->dev);
> + pm_runtime_put_sync(port->dev);
>   free_irq(port->irq, port);
>   dev_pm_clear_wake_irq(port->dev);
>  }
> @@ -1265,8 +1264,7 @@ static int omap8250_probe(struct platform_device *pdev)
>   }
>   priv->line = ret;
>   platform_set_drvdata(pdev, priv);
> - pm_runtime_mark_last_busy(>dev);
> - pm_runtime_put_autosuspend(>dev);
> + pm_runtime_put_sync(>dev);
>   return 0;
>  err:
>   pm_runtime_dont_use_autosuspend(>dev);
> diff --git a/drivers/tty/serial/omap-serial.c 
> b/drivers/tty/serial/omap-serial.c
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -821,8 +821,7 @@ static void serial_omap_shutdown(struct uart_port *port)
>   if (serial_in(up, UART_LSR) & UART_LSR_DR)
>   (void) serial_in(up, UART_RX);
>  
> - pm_runtime_mark_last_busy(up->dev);
> - pm_runtime_put_autosuspend(up->dev);
> + pm_runtime_put_sync(up->dev);
>   free_irq(up->port.irq, up);
>   dev_pm_clear_wake_irq(up->dev);
>  }
> @@ -1751,8 +1750,7 @@ static int serial_omap_probe(struct platform_device 
> *pdev)
>   if (ret != 0)
>   goto err_add_port;
>  
> - pm_runtime_mark_last_busy(up->dev);
> - pm_runtime_put_autosuspend(up->dev);
> + pm_runtime_put_sync(>dev);
>   return 0;
>  
>  err_add_port:
> -- 
> 2.17.0

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Tony Lindgren  [180508 08:22]:
> * Johan Hovold  [180508 07:00]:
> > With the negative autosuspend set in both omap drivers probe functions,
> > this is the expected behaviour. Which I think we must fix.
> 
> Yes indeed. I've been using my script for years now and have
> completely missed the fact that the unused ports are not idled
> at all on start-up when unused.

This might be all that's needed, care to try it and if it works
I'll send out two separate proper patches?

I'm seeing this now on my bbb after temporarily disabling my
UART idle init script:

# rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
0x44e004b4 = 0x0002

# rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
0x44e0006c = 0x0003
0x44e00070 = 0x0003
0x44e00074 = 0x0003
0x44e00078 = 0x0003

# rwmem -s32 0x44e00038   # uart 6 on l4_per
0x44e00038 = 0x0003

Regards,

Tony

8< --
diff --git a/drivers/tty/serial/8250/8250_omap.c 
b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -685,8 +685,7 @@ static void omap_8250_shutdown(struct uart_port *port)
serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
 
-   pm_runtime_mark_last_busy(port->dev);
-   pm_runtime_put_autosuspend(port->dev);
+   pm_runtime_put_sync(port->dev);
free_irq(port->irq, port);
dev_pm_clear_wake_irq(port->dev);
 }
@@ -1265,8 +1264,7 @@ static int omap8250_probe(struct platform_device *pdev)
}
priv->line = ret;
platform_set_drvdata(pdev, priv);
-   pm_runtime_mark_last_busy(>dev);
-   pm_runtime_put_autosuspend(>dev);
+   pm_runtime_put_sync(>dev);
return 0;
 err:
pm_runtime_dont_use_autosuspend(>dev);
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -821,8 +821,7 @@ static void serial_omap_shutdown(struct uart_port *port)
if (serial_in(up, UART_LSR) & UART_LSR_DR)
(void) serial_in(up, UART_RX);
 
-   pm_runtime_mark_last_busy(up->dev);
-   pm_runtime_put_autosuspend(up->dev);
+   pm_runtime_put_sync(up->dev);
free_irq(up->port.irq, up);
dev_pm_clear_wake_irq(up->dev);
 }
@@ -1751,8 +1750,7 @@ static int serial_omap_probe(struct platform_device *pdev)
if (ret != 0)
goto err_add_port;
 
-   pm_runtime_mark_last_busy(up->dev);
-   pm_runtime_put_autosuspend(up->dev);
+   pm_runtime_put_sync(>dev);
return 0;
 
 err_add_port:
-- 
2.17.0

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Tony Lindgren  [180508 08:22]:
> * Johan Hovold  [180508 07:00]:
> > With the negative autosuspend set in both omap drivers probe functions,
> > this is the expected behaviour. Which I think we must fix.
> 
> Yes indeed. I've been using my script for years now and have
> completely missed the fact that the unused ports are not idled
> at all on start-up when unused.

This might be all that's needed, care to try it and if it works
I'll send out two separate proper patches?

I'm seeing this now on my bbb after temporarily disabling my
UART idle init script:

# rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
0x44e004b4 = 0x0002

# rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
0x44e0006c = 0x0003
0x44e00070 = 0x0003
0x44e00074 = 0x0003
0x44e00078 = 0x0003

# rwmem -s32 0x44e00038   # uart 6 on l4_per
0x44e00038 = 0x0003

Regards,

Tony

8< --
diff --git a/drivers/tty/serial/8250/8250_omap.c 
b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -685,8 +685,7 @@ static void omap_8250_shutdown(struct uart_port *port)
serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
 
-   pm_runtime_mark_last_busy(port->dev);
-   pm_runtime_put_autosuspend(port->dev);
+   pm_runtime_put_sync(port->dev);
free_irq(port->irq, port);
dev_pm_clear_wake_irq(port->dev);
 }
@@ -1265,8 +1264,7 @@ static int omap8250_probe(struct platform_device *pdev)
}
priv->line = ret;
platform_set_drvdata(pdev, priv);
-   pm_runtime_mark_last_busy(>dev);
-   pm_runtime_put_autosuspend(>dev);
+   pm_runtime_put_sync(>dev);
return 0;
 err:
pm_runtime_dont_use_autosuspend(>dev);
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -821,8 +821,7 @@ static void serial_omap_shutdown(struct uart_port *port)
if (serial_in(up, UART_LSR) & UART_LSR_DR)
(void) serial_in(up, UART_RX);
 
-   pm_runtime_mark_last_busy(up->dev);
-   pm_runtime_put_autosuspend(up->dev);
+   pm_runtime_put_sync(up->dev);
free_irq(up->port.irq, up);
dev_pm_clear_wake_irq(up->dev);
 }
@@ -1751,8 +1750,7 @@ static int serial_omap_probe(struct platform_device *pdev)
if (ret != 0)
goto err_add_port;
 
-   pm_runtime_mark_last_busy(up->dev);
-   pm_runtime_put_autosuspend(up->dev);
+   pm_runtime_put_sync(>dev);
return 0;
 
 err_add_port:
-- 
2.17.0

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Johan Hovold  [180508 07:00]:
> On Mon, May 07, 2018 at 10:50:32AM -0700, Tony Lindgren wrote:
> > I don't know, they are there for the ports that don't have any
> > serdev device. But if there is a serdev child node, the sysfs
> > disappear for the 8250 port like it's /dev/ttyS* entry. That is
> > even with no hci_serdev.ko loaded :)
> 
> It sounds to me like you have a udev rule that's matching on TTY devices
> and setting an autosuspend timeout that allows the controller to runtime
> suspend. Is that so?

Oh you are right, I do have an old init script that idles the UARTs.

And that script is looking for tty[SO]*. Updating it to use
/sys/bus/platform/devices fixes the issue I was seeing:

uarts=$(find /sys/bus/platform/devices/4*.serial/power/ -type d)
for uart in $uarts; do
echo -n 1000 > $uart/autosuspend_delay_ms
echo -n enabled > $uart/wakeup
echo -n auto > $uart/control
done

The above also idles the console so beware though.

> But the point is, we really don't want the runtime PM behaviour to be
> dependent on the presence of such rules. The serial controllers should
> always be idle when not in use (unless overridden by user space).

I agree, and that seems broken right now.

> > Do you have also a /dev/ttyO* entry created for the serdev port?
> 
> No, serdev claims the port and no tty device is created.

And this is no longer the issue when using /sys/bus/platform/devices
instead of tty[SO]* in my init script :)

> > > I'd say the omap drivers are broken; the controller should definitely
> > > idle when the port is closed (whether using serdev or not) without
> > > having to fiddle with sysfs.
> > 
> > This is happening for the non-serdev ports for sure. FYI, there is
> > one patch needed for omap4 to idle unused ports that I posted
> > few days ago:
> > 
> > [PATCHv3] serial: 8250: omap: Fix idling of clocks for unused uarts
> > 
> > But the serdev port is never idled, even if unused.
> 
> With the negative autosuspend set in both omap drivers probe functions,
> this is the expected behaviour. Which I think we must fix.

Yes indeed. I've been using my script for years now and have
completely missed the fact that the unused ports are not idled
at all on start-up when unused.

> > Can you check your serdev port clkctrl reg with rwmem or similar
> > tool when it's idle?
> > 
> > You can do it with:
> > 
> > rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
> > rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
> > rwmem -s32 0x44e00038   # uart 6 on l4_per
> > 
> > And here's what I have on my bbb with 8250_omap:
> > 
> > 0x44e004b4 = 0x0002
> > 0x44e0006c = 0x0003
> > 0x44e00070 = 0x0003
> > 0x44e00074 = 0x0003
> > 0x44e00078 = 0x0003
> > 0x44e00038 = 0x0003
> > 
> > So all disabled except for the console UART.
> 
> On BBB with omap-serial I have:
> 
> 0x44e004b4 = 0x2  (uart 1, console, open)
> 0x44e0006c = 0x2  (uart 2, serdev, closed)
> 0x44e00070 = 0x3  (uart 3, disabled in DT)
> 0x44e00074 = 0x3  (uart 4, disabled in DT)
> 0x44e00078 = 0x2  (uart 5, serdev, closed)
> 0x44e00038 = 0x2  (uart 6, ttyO5, closed)
> 
> So no enabled and closed port is idle, whether using serdev or not.

OK so we really have currently just one PM related bug where
unused UARTs should be idled on init. And then the serdev PM
can be handled the way you're doing it and tested with the
above rwmem.

Hmm maybe all we need for serdev runtime PM is to set the parent
UART autoidle timeout to immediate (0 instead of -1) from the serdev
child driver to allow the serdev driver manage the PM.

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Johan Hovold  [180508 07:00]:
> On Mon, May 07, 2018 at 10:50:32AM -0700, Tony Lindgren wrote:
> > I don't know, they are there for the ports that don't have any
> > serdev device. But if there is a serdev child node, the sysfs
> > disappear for the 8250 port like it's /dev/ttyS* entry. That is
> > even with no hci_serdev.ko loaded :)
> 
> It sounds to me like you have a udev rule that's matching on TTY devices
> and setting an autosuspend timeout that allows the controller to runtime
> suspend. Is that so?

Oh you are right, I do have an old init script that idles the UARTs.

And that script is looking for tty[SO]*. Updating it to use
/sys/bus/platform/devices fixes the issue I was seeing:

uarts=$(find /sys/bus/platform/devices/4*.serial/power/ -type d)
for uart in $uarts; do
echo -n 1000 > $uart/autosuspend_delay_ms
echo -n enabled > $uart/wakeup
echo -n auto > $uart/control
done

The above also idles the console so beware though.

> But the point is, we really don't want the runtime PM behaviour to be
> dependent on the presence of such rules. The serial controllers should
> always be idle when not in use (unless overridden by user space).

I agree, and that seems broken right now.

> > Do you have also a /dev/ttyO* entry created for the serdev port?
> 
> No, serdev claims the port and no tty device is created.

And this is no longer the issue when using /sys/bus/platform/devices
instead of tty[SO]* in my init script :)

> > > I'd say the omap drivers are broken; the controller should definitely
> > > idle when the port is closed (whether using serdev or not) without
> > > having to fiddle with sysfs.
> > 
> > This is happening for the non-serdev ports for sure. FYI, there is
> > one patch needed for omap4 to idle unused ports that I posted
> > few days ago:
> > 
> > [PATCHv3] serial: 8250: omap: Fix idling of clocks for unused uarts
> > 
> > But the serdev port is never idled, even if unused.
> 
> With the negative autosuspend set in both omap drivers probe functions,
> this is the expected behaviour. Which I think we must fix.

Yes indeed. I've been using my script for years now and have
completely missed the fact that the unused ports are not idled
at all on start-up when unused.

> > Can you check your serdev port clkctrl reg with rwmem or similar
> > tool when it's idle?
> > 
> > You can do it with:
> > 
> > rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
> > rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
> > rwmem -s32 0x44e00038   # uart 6 on l4_per
> > 
> > And here's what I have on my bbb with 8250_omap:
> > 
> > 0x44e004b4 = 0x0002
> > 0x44e0006c = 0x0003
> > 0x44e00070 = 0x0003
> > 0x44e00074 = 0x0003
> > 0x44e00078 = 0x0003
> > 0x44e00038 = 0x0003
> > 
> > So all disabled except for the console UART.
> 
> On BBB with omap-serial I have:
> 
> 0x44e004b4 = 0x2  (uart 1, console, open)
> 0x44e0006c = 0x2  (uart 2, serdev, closed)
> 0x44e00070 = 0x3  (uart 3, disabled in DT)
> 0x44e00074 = 0x3  (uart 4, disabled in DT)
> 0x44e00078 = 0x2  (uart 5, serdev, closed)
> 0x44e00038 = 0x2  (uart 6, ttyO5, closed)
> 
> So no enabled and closed port is idle, whether using serdev or not.

OK so we really have currently just one PM related bug where
unused UARTs should be idled on init. And then the serdev PM
can be handled the way you're doing it and tested with the
above rwmem.

Hmm maybe all we need for serdev runtime PM is to set the parent
UART autoidle timeout to immediate (0 instead of -1) from the serdev
child driver to allow the serdev driver manage the PM.

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Mon, May 07, 2018 at 10:50:32AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180507 16:36]:
> > On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > > Hi,
> > > 
> > > * Johan Hovold  [180507 03:03]:
> > > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:

> > > > Note that serdev not enabling runtime pm for controllers is roughly
> > > > equivalent to setting the .ignore_children flag, which is what we do for
> > > > i2c and spi controller, and possibly what we want here too.
> > > 
> > > We currently don't idle serdev at all even if not in use. What
> > > I noticed is if I have these in my .config:
> > > 
> > > CONFIG_SERIAL_DEV_BUS=y
> > > CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
> > > 
> > > And no hci_serdev.ko driver loaded, then the 8250 port still stays
> > > active and there are no sysfs entries to idle it.
> > 
> > Sounds like the 8250_omap driver is doing something funky. Why would
> > there not be any sysfs attributes to control runtime pm?
> 
> I don't know, they are there for the ports that don't have any
> serdev device. But if there is a serdev child node, the sysfs
> disappear for the 8250 port like it's /dev/ttyS* entry. That is
> even with no hci_serdev.ko loaded :)

It sounds to me like you have a udev rule that's matching on TTY devices
and setting an autosuspend timeout that allows the controller to runtime
suspend. Is that so?

With serdev such a rule would no longer be sufficient as it would no
longer configure all controllers. You can always set the autosuspend
directly through the platform device node, for example:

/sys/bus/platform/devices/481aa000.serial

But the point is, we really don't want the runtime PM behaviour to be
dependent on the presence of such rules. The serial controllers should
always be idle when not in use (unless overridden by user space).

> > > Are you seeing this with your series?
> > 
> > I'm using omap-serial (on BBB) and like 8250_omap, the driver disables
> > runtime pm at probe by setting a negative autosuspend timeout.
> 
> Hmm I though we now have both 8250_omap and serial-omap behave the
> same way for PM.

It looks like they've been implemented the same way (e.g. the negative
autosuspend timeout).

> > Changing this through sysfs causes the serial controller to runtime
> > suspend, but something is not right in my setup as it doesn't wake up on
> > incoming data.
> 
> Do you have also a /dev/ttyO* entry created for the serdev port?

No, serdev claims the port and no tty device is created.

> > I'd say the omap drivers are broken; the controller should definitely
> > idle when the port is closed (whether using serdev or not) without
> > having to fiddle with sysfs.
> 
> This is happening for the non-serdev ports for sure. FYI, there is
> one patch needed for omap4 to idle unused ports that I posted
> few days ago:
> 
> [PATCHv3] serial: 8250: omap: Fix idling of clocks for unused uarts
> 
> But the serdev port is never idled, even if unused.

With the negative autosuspend set in both omap drivers probe functions,
this is the expected behaviour. Which I think we must fix.

> Can you check your serdev port clkctrl reg with rwmem or similar
> tool when it's idle?
> 
> You can do it with:
> 
> rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
> rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
> rwmem -s32 0x44e00038   # uart 6 on l4_per
> 
> And here's what I have on my bbb with 8250_omap:
> 
> 0x44e004b4 = 0x0002
> 0x44e0006c = 0x0003
> 0x44e00070 = 0x0003
> 0x44e00074 = 0x0003
> 0x44e00078 = 0x0003
> 0x44e00038 = 0x0003
> 
> So all disabled except for the console UART.

On BBB with omap-serial I have:

0x44e004b4 = 0x2(uart 1, console, open)
0x44e0006c = 0x2(uart 2, serdev, closed)
0x44e00070 = 0x3(uart 3, disabled in DT)
0x44e00074 = 0x3(uart 4, disabled in DT)
0x44e00078 = 0x2(uart 5, serdev, closed)
0x44e00038 = 0x2(uart 6, ttyO5, closed)

So no enabled and closed port is idle, whether using serdev or not.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Mon, May 07, 2018 at 10:50:32AM -0700, Tony Lindgren wrote:
> * Johan Hovold  [180507 16:36]:
> > On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > > Hi,
> > > 
> > > * Johan Hovold  [180507 03:03]:
> > > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:

> > > > Note that serdev not enabling runtime pm for controllers is roughly
> > > > equivalent to setting the .ignore_children flag, which is what we do for
> > > > i2c and spi controller, and possibly what we want here too.
> > > 
> > > We currently don't idle serdev at all even if not in use. What
> > > I noticed is if I have these in my .config:
> > > 
> > > CONFIG_SERIAL_DEV_BUS=y
> > > CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
> > > 
> > > And no hci_serdev.ko driver loaded, then the 8250 port still stays
> > > active and there are no sysfs entries to idle it.
> > 
> > Sounds like the 8250_omap driver is doing something funky. Why would
> > there not be any sysfs attributes to control runtime pm?
> 
> I don't know, they are there for the ports that don't have any
> serdev device. But if there is a serdev child node, the sysfs
> disappear for the 8250 port like it's /dev/ttyS* entry. That is
> even with no hci_serdev.ko loaded :)

It sounds to me like you have a udev rule that's matching on TTY devices
and setting an autosuspend timeout that allows the controller to runtime
suspend. Is that so?

With serdev such a rule would no longer be sufficient as it would no
longer configure all controllers. You can always set the autosuspend
directly through the platform device node, for example:

/sys/bus/platform/devices/481aa000.serial

But the point is, we really don't want the runtime PM behaviour to be
dependent on the presence of such rules. The serial controllers should
always be idle when not in use (unless overridden by user space).

> > > Are you seeing this with your series?
> > 
> > I'm using omap-serial (on BBB) and like 8250_omap, the driver disables
> > runtime pm at probe by setting a negative autosuspend timeout.
> 
> Hmm I though we now have both 8250_omap and serial-omap behave the
> same way for PM.

It looks like they've been implemented the same way (e.g. the negative
autosuspend timeout).

> > Changing this through sysfs causes the serial controller to runtime
> > suspend, but something is not right in my setup as it doesn't wake up on
> > incoming data.
> 
> Do you have also a /dev/ttyO* entry created for the serdev port?

No, serdev claims the port and no tty device is created.

> > I'd say the omap drivers are broken; the controller should definitely
> > idle when the port is closed (whether using serdev or not) without
> > having to fiddle with sysfs.
> 
> This is happening for the non-serdev ports for sure. FYI, there is
> one patch needed for omap4 to idle unused ports that I posted
> few days ago:
> 
> [PATCHv3] serial: 8250: omap: Fix idling of clocks for unused uarts
> 
> But the serdev port is never idled, even if unused.

With the negative autosuspend set in both omap drivers probe functions,
this is the expected behaviour. Which I think we must fix.

> Can you check your serdev port clkctrl reg with rwmem or similar
> tool when it's idle?
> 
> You can do it with:
> 
> rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
> rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
> rwmem -s32 0x44e00038   # uart 6 on l4_per
> 
> And here's what I have on my bbb with 8250_omap:
> 
> 0x44e004b4 = 0x0002
> 0x44e0006c = 0x0003
> 0x44e00070 = 0x0003
> 0x44e00074 = 0x0003
> 0x44e00078 = 0x0003
> 0x44e00038 = 0x0003
> 
> So all disabled except for the console UART.

On BBB with omap-serial I have:

0x44e004b4 = 0x2(uart 1, console, open)
0x44e0006c = 0x2(uart 2, serdev, closed)
0x44e00070 = 0x3(uart 3, disabled in DT)
0x44e00074 = 0x3(uart 4, disabled in DT)
0x44e00078 = 0x2(uart 5, serdev, closed)
0x44e00038 = 0x2(uart 6, ttyO5, closed)

So no enabled and closed port is idle, whether using serdev or not.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Johan Hovold  [180507 16:36]:
> On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > Hi,
> > 
> > * Johan Hovold  [180507 03:03]:
> > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > 
> > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > us to enable runtime PM by default (once implemented), since we know
> > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > with sideband wakeup gpios).
> > > 
> > > I'm not sure we want generic runtime-pm support for the controllers in
> > > the sense that the slave device state is always reflected by the serial
> > > controller. Similar as for i2c and spi, we really only want to keep the
> > > controller active when we are doing I/O, but we may want to keep a
> > > client active for longer.
> > 
> > Yeah i2c seems to do the right thing where the bus takes care
> > of runtime PM.
> 
> Yeah, but since serial is async in contrast to i2c/spi, we may not be
> able to push this entirely into core. The serdev drivers may need to
> indicate when they expect or need to do I/O by opening and closing the
> port. And this is how I implemented these first couple of gnss drivers.

OK

> Also note that most serial driver do not do runtime pm while the port is
> open (as OMAP does), and doing so also has the drawbacks of lost
> characters etc. as Sebastian mentioned.

Yes serdev seems really nice for the oob wake gpios :)

> > > Take the u-blox driver in this series for example. As I'm using runtime
> > > PM to manage device power, user-space can chose to prevent the receiver
> > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > in setups without a backup battery (by means of the power/control
> > > attribute).
> > 
> > Sorry I don't seem to have that one, care to paste the subject
> > line of that patch?
> 
>   "[PATCH 5/7] gnss: add driver for u-blox receivers"
> 
>   https://lkml.kernel.org/r/20180424163458.11947-6-jo...@kernel.org

Thanks will take a look.

> > > Note that serdev not enabling runtime pm for controllers is roughly
> > > equivalent to setting the .ignore_children flag, which is what we do for
> > > i2c and spi controller, and possibly what we want here too.
> > 
> > We currently don't idle serdev at all even if not in use. What
> > I noticed is if I have these in my .config:
> > 
> > CONFIG_SERIAL_DEV_BUS=y
> > CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
> > 
> > And no hci_serdev.ko driver loaded, then the 8250 port still stays
> > active and there are no sysfs entries to idle it.
> 
> Sounds like the 8250_omap driver is doing something funky. Why would
> there not be any sysfs attributes to control runtime pm?

I don't know, they are there for the ports that don't have any
serdev device. But if there is a serdev child node, the sysfs
disappear for the 8250 port like it's /dev/ttyS* entry. That is
even with no hci_serdev.ko loaded :)

> > Are you seeing this with your series?
> 
> I'm using omap-serial (on BBB) and like 8250_omap, the driver disables
> runtime pm at probe by setting a negative autosuspend timeout.

Hmm I though we now have both 8250_omap and serial-omap behave the
same way for PM.

> Changing this through sysfs causes the serial controller to runtime
> suspend, but something is not right in my setup as it doesn't wake up on
> incoming data.

Do you have also a /dev/ttyO* entry created for the serdev port?

> I'd say the omap drivers are broken; the controller should definitely
> idle when the port is closed (whether using serdev or not) without
> having to fiddle with sysfs.

This is happening for the non-serdev ports for sure. FYI, there is
one patch needed for omap4 to idle unused ports that I posted
few days ago:

[PATCHv3] serial: 8250: omap: Fix idling of clocks for unused uarts

But the serdev port is never idled, even if unused.

Can you check your serdev port clkctrl reg with rwmem or similar
tool when it's idle?

You can do it with:

rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
rwmem -s32 0x44e00038   # uart 6 on l4_per

And here's what I have on my bbb with 8250_omap:

0x44e004b4 = 0x0002
0x44e0006c = 0x0003
0x44e00070 = 0x0003
0x44e00074 = 0x0003
0x44e00078 = 0x0003
0x44e00038 = 0x0003

So all disabled except for the console UART.

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Johan Hovold  [180507 16:36]:
> On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > Hi,
> > 
> > * Johan Hovold  [180507 03:03]:
> > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > 
> > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > us to enable runtime PM by default (once implemented), since we know
> > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > with sideband wakeup gpios).
> > > 
> > > I'm not sure we want generic runtime-pm support for the controllers in
> > > the sense that the slave device state is always reflected by the serial
> > > controller. Similar as for i2c and spi, we really only want to keep the
> > > controller active when we are doing I/O, but we may want to keep a
> > > client active for longer.
> > 
> > Yeah i2c seems to do the right thing where the bus takes care
> > of runtime PM.
> 
> Yeah, but since serial is async in contrast to i2c/spi, we may not be
> able to push this entirely into core. The serdev drivers may need to
> indicate when they expect or need to do I/O by opening and closing the
> port. And this is how I implemented these first couple of gnss drivers.

OK

> Also note that most serial driver do not do runtime pm while the port is
> open (as OMAP does), and doing so also has the drawbacks of lost
> characters etc. as Sebastian mentioned.

Yes serdev seems really nice for the oob wake gpios :)

> > > Take the u-blox driver in this series for example. As I'm using runtime
> > > PM to manage device power, user-space can chose to prevent the receiver
> > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > in setups without a backup battery (by means of the power/control
> > > attribute).
> > 
> > Sorry I don't seem to have that one, care to paste the subject
> > line of that patch?
> 
>   "[PATCH 5/7] gnss: add driver for u-blox receivers"
> 
>   https://lkml.kernel.org/r/20180424163458.11947-6-jo...@kernel.org

Thanks will take a look.

> > > Note that serdev not enabling runtime pm for controllers is roughly
> > > equivalent to setting the .ignore_children flag, which is what we do for
> > > i2c and spi controller, and possibly what we want here too.
> > 
> > We currently don't idle serdev at all even if not in use. What
> > I noticed is if I have these in my .config:
> > 
> > CONFIG_SERIAL_DEV_BUS=y
> > CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
> > 
> > And no hci_serdev.ko driver loaded, then the 8250 port still stays
> > active and there are no sysfs entries to idle it.
> 
> Sounds like the 8250_omap driver is doing something funky. Why would
> there not be any sysfs attributes to control runtime pm?

I don't know, they are there for the ports that don't have any
serdev device. But if there is a serdev child node, the sysfs
disappear for the 8250 port like it's /dev/ttyS* entry. That is
even with no hci_serdev.ko loaded :)

> > Are you seeing this with your series?
> 
> I'm using omap-serial (on BBB) and like 8250_omap, the driver disables
> runtime pm at probe by setting a negative autosuspend timeout.

Hmm I though we now have both 8250_omap and serial-omap behave the
same way for PM.

> Changing this through sysfs causes the serial controller to runtime
> suspend, but something is not right in my setup as it doesn't wake up on
> incoming data.

Do you have also a /dev/ttyO* entry created for the serdev port?

> I'd say the omap drivers are broken; the controller should definitely
> idle when the port is closed (whether using serdev or not) without
> having to fiddle with sysfs.

This is happening for the non-serdev ports for sure. FYI, there is
one patch needed for omap4 to idle unused ports that I posted
few days ago:

[PATCHv3] serial: 8250: omap: Fix idling of clocks for unused uarts

But the serdev port is never idled, even if unused.

Can you check your serdev port clkctrl reg with rwmem or similar
tool when it's idle?

You can do it with:

rwmem -s32 0x44e004b4   # uart 1 on l4_wkup
rwmem -s32 0x44e0006c+0x10  # uart 2 - 5 on l4_per
rwmem -s32 0x44e00038   # uart 6 on l4_per

And here's what I have on my bbb with 8250_omap:

0x44e004b4 = 0x0002
0x44e0006c = 0x0003
0x44e00070 = 0x0003
0x44e00074 = 0x0003
0x44e00078 = 0x0003
0x44e00038 = 0x0003

So all disabled except for the console UART.

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> Hi,
> 
> * Johan Hovold  [180507 03:03]:
> > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > 
> > > Having said all of this, serdev does not yet support runtime PM (at
> > > all). Tony is currently looking into it. Fortunately serdev allows
> > > us to enable runtime PM by default (once implemented), since we know
> > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > with sideband wakeup gpios).
> > 
> > I'm not sure we want generic runtime-pm support for the controllers in
> > the sense that the slave device state is always reflected by the serial
> > controller. Similar as for i2c and spi, we really only want to keep the
> > controller active when we are doing I/O, but we may want to keep a
> > client active for longer.
> 
> Yeah i2c seems to do the right thing where the bus takes care
> of runtime PM.

Yeah, but since serial is async in contrast to i2c/spi, we may not be
able to push this entirely into core. The serdev drivers may need to
indicate when they expect or need to do I/O by opening and closing the
port. And this is how I implemented these first couple of gnss drivers.

Also note that most serial driver do not do runtime pm while the port is
open (as OMAP does), and doing so also has the drawbacks of lost
characters etc. as Sebastian mentioned.

> > Take the u-blox driver in this series for example. As I'm using runtime
> > PM to manage device power, user-space can chose to prevent the receiver
> > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > in setups without a backup battery (by means of the power/control
> > attribute).
> 
> Sorry I don't seem to have that one, care to paste the subject
> line of that patch?

"[PATCH 5/7] gnss: add driver for u-blox receivers"

https://lkml.kernel.org/r/20180424163458.11947-6-jo...@kernel.org

> > Note that serdev not enabling runtime pm for controllers is roughly
> > equivalent to setting the .ignore_children flag, which is what we do for
> > i2c and spi controller, and possibly what we want here too.
> 
> We currently don't idle serdev at all even if not in use. What
> I noticed is if I have these in my .config:
> 
> CONFIG_SERIAL_DEV_BUS=y
> CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
> 
> And no hci_serdev.ko driver loaded, then the 8250 port still stays
> active and there are no sysfs entries to idle it.

Sounds like the 8250_omap driver is doing something funky. Why would
there not be any sysfs attributes to control runtime pm?

> Are you seeing this with your series?

I'm using omap-serial (on BBB) and like 8250_omap, the driver disables
runtime pm at probe by setting a negative autosuspend timeout.

Changing this through sysfs causes the serial controller to runtime
suspend, but something is not right in my setup as it doesn't wake up on
incoming data.

I'd say the omap drivers are broken; the controller should definitely
idle when the port is closed (whether using serdev or not) without
having to fiddle with sysfs.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> Hi,
> 
> * Johan Hovold  [180507 03:03]:
> > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > 
> > > Having said all of this, serdev does not yet support runtime PM (at
> > > all). Tony is currently looking into it. Fortunately serdev allows
> > > us to enable runtime PM by default (once implemented), since we know
> > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > with sideband wakeup gpios).
> > 
> > I'm not sure we want generic runtime-pm support for the controllers in
> > the sense that the slave device state is always reflected by the serial
> > controller. Similar as for i2c and spi, we really only want to keep the
> > controller active when we are doing I/O, but we may want to keep a
> > client active for longer.
> 
> Yeah i2c seems to do the right thing where the bus takes care
> of runtime PM.

Yeah, but since serial is async in contrast to i2c/spi, we may not be
able to push this entirely into core. The serdev drivers may need to
indicate when they expect or need to do I/O by opening and closing the
port. And this is how I implemented these first couple of gnss drivers.

Also note that most serial driver do not do runtime pm while the port is
open (as OMAP does), and doing so also has the drawbacks of lost
characters etc. as Sebastian mentioned.

> > Take the u-blox driver in this series for example. As I'm using runtime
> > PM to manage device power, user-space can chose to prevent the receiver
> > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > in setups without a backup battery (by means of the power/control
> > attribute).
> 
> Sorry I don't seem to have that one, care to paste the subject
> line of that patch?

"[PATCH 5/7] gnss: add driver for u-blox receivers"

https://lkml.kernel.org/r/20180424163458.11947-6-jo...@kernel.org

> > Note that serdev not enabling runtime pm for controllers is roughly
> > equivalent to setting the .ignore_children flag, which is what we do for
> > i2c and spi controller, and possibly what we want here too.
> 
> We currently don't idle serdev at all even if not in use. What
> I noticed is if I have these in my .config:
> 
> CONFIG_SERIAL_DEV_BUS=y
> CONFIG_SERIAL_DEV_CTRL_TTYPORT=y
> 
> And no hci_serdev.ko driver loaded, then the 8250 port still stays
> active and there are no sysfs entries to idle it.

Sounds like the 8250_omap driver is doing something funky. Why would
there not be any sysfs attributes to control runtime pm?

> Are you seeing this with your series?

I'm using omap-serial (on BBB) and like 8250_omap, the driver disables
runtime pm at probe by setting a negative autosuspend timeout.

Changing this through sysfs causes the serial controller to runtime
suspend, but something is not right in my setup as it doesn't wake up on
incoming data.

I'd say the omap drivers are broken; the controller should definitely
idle when the port is closed (whether using serdev or not) without
having to fiddle with sysfs.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

Hi,

* Johan Hovold  [180507 03:03]:
> On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> 
> > Having said all of this, serdev does not yet support runtime PM (at
> > all). Tony is currently looking into it. Fortunately serdev allows
> > us to enable runtime PM by default (once implemented), since we know
> > the remote side and can (hopefully) avoid losing characters (i.e.
> > with sideband wakeup gpios).
> 
> I'm not sure we want generic runtime-pm support for the controllers in
> the sense that the slave device state is always reflected by the serial
> controller. Similar as for i2c and spi, we really only want to keep the
> controller active when we are doing I/O, but we may want to keep a
> client active for longer.

Yeah i2c seems to do the right thing where the bus takes care
of runtime PM.

> Take the u-blox driver in this series for example. As I'm using runtime
> PM to manage device power, user-space can chose to prevent the receiver
> from runtime suspending in order to avoid lengthy (re-)acquisition times
> in setups without a backup battery (by means of the power/control
> attribute).

Sorry I don't seem to have that one, care to paste the subject
line of that patch?

> Note that serdev not enabling runtime pm for controllers is roughly
> equivalent to setting the .ignore_children flag, which is what we do for
> i2c and spi controller, and possibly what we want here too.

We currently don't idle serdev at all even if not in use. What
I noticed is if I have these in my .config:

CONFIG_SERIAL_DEV_BUS=y
CONFIG_SERIAL_DEV_CTRL_TTYPORT=y

And no hci_serdev.ko driver loaded, then the 8250 port still stays
active and there are no sysfs entries to idle it.

Are you seeing this with your series?

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

Hi,

* Johan Hovold  [180507 03:03]:
> On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> 
> > Having said all of this, serdev does not yet support runtime PM (at
> > all). Tony is currently looking into it. Fortunately serdev allows
> > us to enable runtime PM by default (once implemented), since we know
> > the remote side and can (hopefully) avoid losing characters (i.e.
> > with sideband wakeup gpios).
> 
> I'm not sure we want generic runtime-pm support for the controllers in
> the sense that the slave device state is always reflected by the serial
> controller. Similar as for i2c and spi, we really only want to keep the
> controller active when we are doing I/O, but we may want to keep a
> client active for longer.

Yeah i2c seems to do the right thing where the bus takes care
of runtime PM.

> Take the u-blox driver in this series for example. As I'm using runtime
> PM to manage device power, user-space can chose to prevent the receiver
> from runtime suspending in order to avoid lengthy (re-)acquisition times
> in setups without a backup battery (by means of the power/control
> attribute).

Sorry I don't seem to have that one, care to paste the subject
line of that patch?

> Note that serdev not enabling runtime pm for controllers is roughly
> equivalent to setting the .ignore_children flag, which is what we do for
> i2c and spi controller, and possibly what we want here too.

We currently don't idle serdev at all even if not in use. What
I noticed is if I have these in my .config:

CONFIG_SERIAL_DEV_BUS=y
CONFIG_SERIAL_DEV_CTRL_TTYPORT=y

And no hci_serdev.ko driver loaded, then the 8250 port still stays
active and there are no sysfs entries to idle it.

Are you seeing this with your series?

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Fri, May 04, 2018 at 02:04:15PM +0200, H. Nikolaus Schaller wrote:

> It should be possible to cover this by a timer that is started
> in this case. If there is serdev data received after assuming the module
> is turned off, the driver has detected the wrong case - and can safely
> close the serdev until we want to have it powered on again.
> 
> If there is no response after turing off, the module power state is already
> in sync and we can close the serdev as well - after the timeout (let's say
> 30 seconds). Then, the serdev UART can idle. We should open the serdev
> and start this timer also in the probe function to catch an initially wrong
> state.

Right, the only thing that worried me about that was that we cannot
really delay system suspend for 30 seconds even if a somewhat shorter
delay should be probably be sufficient (still a number of seconds).

Configuring the serial controller as a wakeup source which aborts
suspend or resumes if the driver gets it wrong might be preferred to
draining the battery in suspend however.

Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Fri, May 04, 2018 at 02:04:15PM +0200, H. Nikolaus Schaller wrote:

> It should be possible to cover this by a timer that is started
> in this case. If there is serdev data received after assuming the module
> is turned off, the driver has detected the wrong case - and can safely
> close the serdev until we want to have it powered on again.
> 
> If there is no response after turing off, the module power state is already
> in sync and we can close the serdev as well - after the timeout (let's say
> 30 seconds). Then, the serdev UART can idle. We should open the serdev
> and start this timer also in the probe function to catch an initially wrong
> state.

Right, the only thing that worried me about that was that we cannot
really delay system suspend for 30 seconds even if a somewhat shorter
delay should be probably be sufficient (still a number of seconds).

Configuring the serial controller as a wakeup source which aborts
suspend or resumes if the driver gets it wrong might be preferred to
draining the battery in suspend however.

Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:

> Having said all of this, serdev does not yet support runtime PM (at
> all). Tony is currently looking into it. Fortunately serdev allows
> us to enable runtime PM by default (once implemented), since we know
> the remote side and can (hopefully) avoid losing characters (i.e.
> with sideband wakeup gpios).

I'm not sure we want generic runtime-pm support for the controllers in
the sense that the slave device state is always reflected by the serial
controller. Similar as for i2c and spi, we really only want to keep the
controller active when we are doing I/O, but we may want to keep a
client active for longer.

Take the u-blox driver in this series for example. As I'm using runtime
PM to manage device power, user-space can chose to prevent the receiver
from runtime suspending in order to avoid lengthy (re-)acquisition times
in setups without a backup battery (by means of the power/control
attribute).

Note that serdev not enabling runtime pm for controllers is roughly
equivalent to setting the .ignore_children flag, which is what we do for
i2c and spi controller, and possibly what we want here too.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:

> Having said all of this, serdev does not yet support runtime PM (at
> all). Tony is currently looking into it. Fortunately serdev allows
> us to enable runtime PM by default (once implemented), since we know
> the remote side and can (hopefully) avoid losing characters (i.e.
> with sideband wakeup gpios).

I'm not sure we want generic runtime-pm support for the controllers in
the sense that the slave device state is always reflected by the serial
controller. Similar as for i2c and spi, we really only want to keep the
controller active when we are doing I/O, but we may want to keep a
client active for longer.

Take the u-blox driver in this series for example. As I'm using runtime
PM to manage device power, user-space can chose to prevent the receiver
from runtime suspending in order to avoid lengthy (re-)acquisition times
in setups without a backup battery (by means of the power/control
attribute).

Note that serdev not enabling runtime pm for controllers is roughly
equivalent to setting the .ignore_children flag, which is what we do for
i2c and spi controller, and possibly what we want here too.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Thu, May 03, 2018 at 11:35:21AM +0200, H. Nikolaus Schaller wrote:

> I have realized that the w2sg0004 is an exception (although a Sirf chip)
> that it does not provide a WAKEUP signal. And another significant
> difference is that we have to keep the serdev UART enabled even if there
> is no user-space client. Otherwise we are not able to detect unexpected
> activity. So we unfortunately can't move serdev open/close into the .open
> and .close ops but need to open it in probe.

As have also been discussed elsewhere in this thread it may be possible,
and it is definitely desirable, to only keep the port open when really
needed. But given the complexity of implementing this, starting with a
simpler and less power efficient method for sirf-chips without WAKEUP
may be acceptable.

> Therefore, it is in my opinion still better to have a separate driver for
> the w2sg0004 instead of hacking the support of this chip into your WAKEUP
> capable sirfstar driver. So I suggest that you make WAKEUP a required
> property.

I disagree. The sirf driver subsumes your particular wi2wi module and
configurations without WAKEUP are described by the datasheets for other
modules as well.

> We had faced a comparable decision last year with the ov9650 and ov9655 camera
> sensors which are almost the same. But not same enough to integrate both into
> a single driver.

But here we are talking about two configuration for the same chip (even
if your particular chip only supports one).

Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Thu, May 03, 2018 at 11:35:21AM +0200, H. Nikolaus Schaller wrote:

> I have realized that the w2sg0004 is an exception (although a Sirf chip)
> that it does not provide a WAKEUP signal. And another significant
> difference is that we have to keep the serdev UART enabled even if there
> is no user-space client. Otherwise we are not able to detect unexpected
> activity. So we unfortunately can't move serdev open/close into the .open
> and .close ops but need to open it in probe.

As have also been discussed elsewhere in this thread it may be possible,
and it is definitely desirable, to only keep the port open when really
needed. But given the complexity of implementing this, starting with a
simpler and less power efficient method for sirf-chips without WAKEUP
may be acceptable.

> Therefore, it is in my opinion still better to have a separate driver for
> the w2sg0004 instead of hacking the support of this chip into your WAKEUP
> capable sirfstar driver. So I suggest that you make WAKEUP a required
> property.

I disagree. The sirf driver subsumes your particular wi2wi module and
configurations without WAKEUP are described by the datasheets for other
modules as well.

> We had faced a comparable decision last year with the ov9650 and ov9655 camera
> sensors which are almost the same. But not same enough to integrate both into
> a single driver.

But here we are talking about two configuration for the same chip (even
if your particular chip only supports one).

Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Wed, May 02, 2018 at 08:16:11AM -0500, Rob Herring wrote:
> On Wed, May 2, 2018 at 3:16 AM, Johan Hovold  wrote:
> > On Tue, May 01, 2018 at 09:05:42AM -0500, Rob Herring wrote:
> >> On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold  wrote:
> >> > On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
> >> >> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
> >> >> > Add binding for u-blox GNSS receivers.

> >> >> > +Required Properties:
> >> >> > +
> >> >> > +- compatible   : Must be one of
> >> >> > +
> >> >> > +   "u-blox,neo-8"
> >> >> > +   "u-blox,neo-m8"
> >> >> > +
> >> >> > +- vcc-supply   : Main voltage regulator (VCC)
> >> >>
> >> >> What about V_BCKP?

> > ...my point was that in case there's no backup battery, you don't want
> > to enable vcc (via v_bckp) at probe (and instead have the device cold
> > boot whenever it's used).
> 
> Wouldn't that result in very long acquisition times? I guess I was
> thinking Vcc would be always on when running and V_bckp was just for
> suspend.

Yes, the acquisition times would certainly be longer, but for a GNSS
receiver which is rarely used that might still be preferred. And since
I'm using runtime pm to manage the supply, this policy decision can be
deferred to user space and controlled through the power/control
attribute.

> > Hence, the driver would need to check if the v_bckp-supply is identical
> > to vcc and not enable the former at probe in that case (i.e. similar to
> > if no v_bckp had been specified and we considered it optional).
> 
> I guess if that's the intended operation, then making it optional is fine.

Ok.

> >> > Take a look at the sirf binding; vendors use different names for their
> >> > timepulse pins and in that case I added the actual pin names (1PPS, TM)
> >> > in parenthesis after the description.
> >> >
> >> > Note that I mentioned "timepulse-gpios" in the generic binding with the
> >> > intent of trying to enforce a generic name for pins with such a
> >> > function (similarly for "enable-gpios", which I guess is already
> >> > established).
> >>
> >> Yes, I think that's good.
> >>
> >> Though with the enable-gpios I was debating the name for sirfstar a
> >> bit because it isn't the normal drive it active to enable, but rather
> >> a pulse to enable or disable.
> >
> > I had some concerns along those lines as well, and if you agree I'll
> > change the property name to on_off-gpios (or onoff-gpios) which matches
> > the vendor data sheets for this pin, and which I think would be better.
> 
> Okay, just add a vendor prefix. And note that using '_' is discouraged.

Ok, I'll name it "sirf,onoff-gpios".

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Wed, May 02, 2018 at 08:16:11AM -0500, Rob Herring wrote:
> On Wed, May 2, 2018 at 3:16 AM, Johan Hovold  wrote:
> > On Tue, May 01, 2018 at 09:05:42AM -0500, Rob Herring wrote:
> >> On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold  wrote:
> >> > On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
> >> >> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
> >> >> > Add binding for u-blox GNSS receivers.

> >> >> > +Required Properties:
> >> >> > +
> >> >> > +- compatible   : Must be one of
> >> >> > +
> >> >> > +   "u-blox,neo-8"
> >> >> > +   "u-blox,neo-m8"
> >> >> > +
> >> >> > +- vcc-supply   : Main voltage regulator (VCC)
> >> >>
> >> >> What about V_BCKP?

> > ...my point was that in case there's no backup battery, you don't want
> > to enable vcc (via v_bckp) at probe (and instead have the device cold
> > boot whenever it's used).
> 
> Wouldn't that result in very long acquisition times? I guess I was
> thinking Vcc would be always on when running and V_bckp was just for
> suspend.

Yes, the acquisition times would certainly be longer, but for a GNSS
receiver which is rarely used that might still be preferred. And since
I'm using runtime pm to manage the supply, this policy decision can be
deferred to user space and controlled through the power/control
attribute.

> > Hence, the driver would need to check if the v_bckp-supply is identical
> > to vcc and not enable the former at probe in that case (i.e. similar to
> > if no v_bckp had been specified and we considered it optional).
> 
> I guess if that's the intended operation, then making it optional is fine.

Ok.

> >> > Take a look at the sirf binding; vendors use different names for their
> >> > timepulse pins and in that case I added the actual pin names (1PPS, TM)
> >> > in parenthesis after the description.
> >> >
> >> > Note that I mentioned "timepulse-gpios" in the generic binding with the
> >> > intent of trying to enforce a generic name for pins with such a
> >> > function (similarly for "enable-gpios", which I guess is already
> >> > established).
> >>
> >> Yes, I think that's good.
> >>
> >> Though with the enable-gpios I was debating the name for sirfstar a
> >> bit because it isn't the normal drive it active to enable, but rather
> >> a pulse to enable or disable.
> >
> > I had some concerns along those lines as well, and if you agree I'll
> > change the property name to on_off-gpios (or onoff-gpios) which matches
> > the vendor data sheets for this pin, and which I think would be better.
> 
> Okay, just add a vendor prefix. And note that using '_' is discouraged.

Ok, I'll name it "sirf,onoff-gpios".

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Sebastian Reichel  [180504 13:40]:
> Hi,
> 
> On Fri, May 04, 2018 at 02:04:15PM +0200, H. Nikolaus Schaller wrote:
> > > Am 04.05.2018 um 13:42 schrieb Sebastian Reichel :
> > >> I think it does not need much more (if at all) than a gpio controller on
> > >> the OMAP3 chip (I think the clocks are active anyways for use by the 
> > >> other
> > >> UARTs).
> > >> 
> > >> We had proposed years ago to reprogram the UART RX pin by pinmux-states
> > >> into an interrupt gpio but that was rejected because it was not general
> > >> enough and ugly in the device tree (an rx-gpios record where the rx-line
> > >> is already connected to the UART-rx).
> > >> 
> > >> Then we did experiment with tapping the UART driver and finally the
> > >> serdev API was developed to solve this problem. Hence we use it now this
> > >> way.
> > > 
> > > Having any UART active on OMAP results in the SoC not entering
> > > idle/off wasting energy. For normal (i.e. not connected to a peripheral)
> > > TTYs you can enable runtime autosuspend and configure the RX pin as
> > > wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
> > > will lose the first few characters (since the serial device needs some
> > > time to wakeup). This is for example supported by the N900 uart3
> > > (debug uart):
> > > 
> > > $ grep -A4 " {" arch/arm/boot/dts/omap3-n900.dts 
> > >  {
> > >   interrupts-extended = < 74 _pmx_core OMAP3_UART3_RX>;
> > >   pinctrl-names = "default";
> > >   pinctrl-0 = <_pins>;
> > > };
> > > 
> > > To get it working, you also need to enable autosuspend for the tty
> > > in userspace (echo 3000 
> > > /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
> > > This is not enabled by default due to the character loss characteristic
> > > during wakeup.
> > > 
> > > Having said all of this, serdev does not yet support runtime PM (at
> > > all). Tony is currently looking into it. Fortunately serdev allows
> > > us to enable runtime PM by default (once implemented), since we know
> > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > with sideband wakeup gpios).
> > 
> > thanks for explaining this! We originally had similar thoughts when
> > proposing a w2sg0004 driver for the first time (years ago), but we can
> > not accept loosing characters...
> > 
> > Now, I could imagine a potential solution. The key situation why we keep
> > the serdev open and listening is if the driver did try to turn the module
> > off, but in fact did turn it on (because it was not in sync).
> >
> > It should be possible to cover this by a timer that is started
> > in this case. If there is serdev data received after assuming the module
> > is turned off, the driver has detected the wrong case - and can safely
> > close the serdev until we want to have it powered on again.
> > 
> > If there is no response after turing off, the module power state is already
> > in sync and we can close the serdev as well - after the timeout (let's say
> > 30 seconds). Then, the serdev UART can idle. We should open the serdev
> > and start this timer also in the probe function to catch an initially wrong
> > state.
> 
> That sounds like a good plan.

I don't know the details, but sounds like you should might be able to
just the timer that comes with pm_runtime_use_autosuspend() :)

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Tony Lindgren]

* Sebastian Reichel  [180504 13:40]:
> Hi,
> 
> On Fri, May 04, 2018 at 02:04:15PM +0200, H. Nikolaus Schaller wrote:
> > > Am 04.05.2018 um 13:42 schrieb Sebastian Reichel :
> > >> I think it does not need much more (if at all) than a gpio controller on
> > >> the OMAP3 chip (I think the clocks are active anyways for use by the 
> > >> other
> > >> UARTs).
> > >> 
> > >> We had proposed years ago to reprogram the UART RX pin by pinmux-states
> > >> into an interrupt gpio but that was rejected because it was not general
> > >> enough and ugly in the device tree (an rx-gpios record where the rx-line
> > >> is already connected to the UART-rx).
> > >> 
> > >> Then we did experiment with tapping the UART driver and finally the
> > >> serdev API was developed to solve this problem. Hence we use it now this
> > >> way.
> > > 
> > > Having any UART active on OMAP results in the SoC not entering
> > > idle/off wasting energy. For normal (i.e. not connected to a peripheral)
> > > TTYs you can enable runtime autosuspend and configure the RX pin as
> > > wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
> > > will lose the first few characters (since the serial device needs some
> > > time to wakeup). This is for example supported by the N900 uart3
> > > (debug uart):
> > > 
> > > $ grep -A4 " {" arch/arm/boot/dts/omap3-n900.dts 
> > >  {
> > >   interrupts-extended = < 74 _pmx_core OMAP3_UART3_RX>;
> > >   pinctrl-names = "default";
> > >   pinctrl-0 = <_pins>;
> > > };
> > > 
> > > To get it working, you also need to enable autosuspend for the tty
> > > in userspace (echo 3000 
> > > /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
> > > This is not enabled by default due to the character loss characteristic
> > > during wakeup.
> > > 
> > > Having said all of this, serdev does not yet support runtime PM (at
> > > all). Tony is currently looking into it. Fortunately serdev allows
> > > us to enable runtime PM by default (once implemented), since we know
> > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > with sideband wakeup gpios).
> > 
> > thanks for explaining this! We originally had similar thoughts when
> > proposing a w2sg0004 driver for the first time (years ago), but we can
> > not accept loosing characters...
> > 
> > Now, I could imagine a potential solution. The key situation why we keep
> > the serdev open and listening is if the driver did try to turn the module
> > off, but in fact did turn it on (because it was not in sync).
> >
> > It should be possible to cover this by a timer that is started
> > in this case. If there is serdev data received after assuming the module
> > is turned off, the driver has detected the wrong case - and can safely
> > close the serdev until we want to have it powered on again.
> > 
> > If there is no response after turing off, the module power state is already
> > in sync and we can close the serdev as well - after the timeout (let's say
> > 30 seconds). Then, the serdev UART can idle. We should open the serdev
> > and start this timer also in the probe function to catch an initially wrong
> > state.
> 
> That sounds like a good plan.

I don't know the details, but sounds like you should might be able to
just the timer that comes with pm_runtime_use_autosuspend() :)

Regards,

Tony

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Sebastian Reichel]

Hi,

On Fri, May 04, 2018 at 02:04:15PM +0200, H. Nikolaus Schaller wrote:
> > Am 04.05.2018 um 13:42 schrieb Sebastian Reichel :
> >> I think it does not need much more (if at all) than a gpio controller on
> >> the OMAP3 chip (I think the clocks are active anyways for use by the other
> >> UARTs).
> >> 
> >> We had proposed years ago to reprogram the UART RX pin by pinmux-states
> >> into an interrupt gpio but that was rejected because it was not general
> >> enough and ugly in the device tree (an rx-gpios record where the rx-line
> >> is already connected to the UART-rx).
> >> 
> >> Then we did experiment with tapping the UART driver and finally the
> >> serdev API was developed to solve this problem. Hence we use it now this
> >> way.
> > 
> > Having any UART active on OMAP results in the SoC not entering
> > idle/off wasting energy. For normal (i.e. not connected to a peripheral)
> > TTYs you can enable runtime autosuspend and configure the RX pin as
> > wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
> > will lose the first few characters (since the serial device needs some
> > time to wakeup). This is for example supported by the N900 uart3
> > (debug uart):
> > 
> > $ grep -A4 " {" arch/arm/boot/dts/omap3-n900.dts 
> >  {
> > interrupts-extended = < 74 _pmx_core OMAP3_UART3_RX>;
> > pinctrl-names = "default";
> > pinctrl-0 = <_pins>;
> > };
> > 
> > To get it working, you also need to enable autosuspend for the tty
> > in userspace (echo 3000 
> > /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
> > This is not enabled by default due to the character loss characteristic
> > during wakeup.
> > 
> > Having said all of this, serdev does not yet support runtime PM (at
> > all). Tony is currently looking into it. Fortunately serdev allows
> > us to enable runtime PM by default (once implemented), since we know
> > the remote side and can (hopefully) avoid losing characters (i.e.
> > with sideband wakeup gpios).
> 
> thanks for explaining this! We originally had similar thoughts when
> proposing a w2sg0004 driver for the first time (years ago), but we can
> not accept loosing characters...
> 
> Now, I could imagine a potential solution. The key situation why we keep
> the serdev open and listening is if the driver did try to turn the module
> off, but in fact did turn it on (because it was not in sync).
>
> It should be possible to cover this by a timer that is started
> in this case. If there is serdev data received after assuming the module
> is turned off, the driver has detected the wrong case - and can safely
> close the serdev until we want to have it powered on again.
> 
> If there is no response after turing off, the module power state is already
> in sync and we can close the serdev as well - after the timeout (let's say
> 30 seconds). Then, the serdev UART can idle. We should open the serdev
> and start this timer also in the probe function to catch an initially wrong
> state.

That sounds like a good plan.

> But I think we should focus on the basics of this driver first. Then it can
> be optimized later on.

Definitely.

-- Sebastian.


signature.asc
Description: PGP signature

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Sebastian Reichel]

Hi,

On Fri, May 04, 2018 at 02:04:15PM +0200, H. Nikolaus Schaller wrote:
> > Am 04.05.2018 um 13:42 schrieb Sebastian Reichel :
> >> I think it does not need much more (if at all) than a gpio controller on
> >> the OMAP3 chip (I think the clocks are active anyways for use by the other
> >> UARTs).
> >> 
> >> We had proposed years ago to reprogram the UART RX pin by pinmux-states
> >> into an interrupt gpio but that was rejected because it was not general
> >> enough and ugly in the device tree (an rx-gpios record where the rx-line
> >> is already connected to the UART-rx).
> >> 
> >> Then we did experiment with tapping the UART driver and finally the
> >> serdev API was developed to solve this problem. Hence we use it now this
> >> way.
> > 
> > Having any UART active on OMAP results in the SoC not entering
> > idle/off wasting energy. For normal (i.e. not connected to a peripheral)
> > TTYs you can enable runtime autosuspend and configure the RX pin as
> > wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
> > will lose the first few characters (since the serial device needs some
> > time to wakeup). This is for example supported by the N900 uart3
> > (debug uart):
> > 
> > $ grep -A4 " {" arch/arm/boot/dts/omap3-n900.dts 
> >  {
> > interrupts-extended = < 74 _pmx_core OMAP3_UART3_RX>;
> > pinctrl-names = "default";
> > pinctrl-0 = <_pins>;
> > };
> > 
> > To get it working, you also need to enable autosuspend for the tty
> > in userspace (echo 3000 
> > /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
> > This is not enabled by default due to the character loss characteristic
> > during wakeup.
> > 
> > Having said all of this, serdev does not yet support runtime PM (at
> > all). Tony is currently looking into it. Fortunately serdev allows
> > us to enable runtime PM by default (once implemented), since we know
> > the remote side and can (hopefully) avoid losing characters (i.e.
> > with sideband wakeup gpios).
> 
> thanks for explaining this! We originally had similar thoughts when
> proposing a w2sg0004 driver for the first time (years ago), but we can
> not accept loosing characters...
> 
> Now, I could imagine a potential solution. The key situation why we keep
> the serdev open and listening is if the driver did try to turn the module
> off, but in fact did turn it on (because it was not in sync).
>
> It should be possible to cover this by a timer that is started
> in this case. If there is serdev data received after assuming the module
> is turned off, the driver has detected the wrong case - and can safely
> close the serdev until we want to have it powered on again.
> 
> If there is no response after turing off, the module power state is already
> in sync and we can close the serdev as well - after the timeout (let's say
> 30 seconds). Then, the serdev UART can idle. We should open the serdev
> and start this timer also in the probe function to catch an initially wrong
> state.

That sounds like a good plan.

> But I think we should focus on the basics of this driver first. Then it can
> be optimized later on.

Definitely.

-- Sebastian.


signature.asc
Description: PGP signature

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[H. Nikolaus Schaller]

Hi Sebastian,

> Am 04.05.2018 um 13:42 schrieb Sebastian Reichel :
> 
> [+cc Tony]
> 
> Hi,
> 
>> I think it does not need much more (if at all) than a gpio controller on
>> the OMAP3 chip (I think the clocks are active anyways for use by the other
>> UARTs).
>> 
>> We had proposed years ago to reprogram the UART RX pin by pinmux-states
>> into an interrupt gpio but that was rejected because it was not general
>> enough and ugly in the device tree (an rx-gpios record where the rx-line
>> is already connected to the UART-rx).
>> 
>> Then we did experiment with tapping the UART driver and finally the
>> serdev API was developed to solve this problem. Hence we use it now this
>> way.
> 
> Having any UART active on OMAP results in the SoC not entering
> idle/off wasting energy. For normal (i.e. not connected to a peripheral)
> TTYs you can enable runtime autosuspend and configure the RX pin as
> wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
> will lose the first few characters (since the serial device needs some
> time to wakeup). This is for example supported by the N900 uart3
> (debug uart):
> 
> $ grep -A4 " {" arch/arm/boot/dts/omap3-n900.dts 
>  {
>   interrupts-extended = < 74 _pmx_core OMAP3_UART3_RX>;
>   pinctrl-names = "default";
>   pinctrl-0 = <_pins>;
> };
> 
> To get it working, you also need to enable autosuspend for the tty
> in userspace (echo 3000 
> /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
> This is not enabled by default due to the character loss characteristic
> during wakeup.
> 
> Having said all of this, serdev does not yet support runtime PM (at
> all). Tony is currently looking into it. Fortunately serdev allows
> us to enable runtime PM by default (once implemented), since we know
> the remote side and can (hopefully) avoid losing characters (i.e.
> with sideband wakeup gpios).

thanks for explaining this! We originally had similar thoughts when
proposing a w2sg0004 driver for the first time (years ago), but we can
not accept loosing characters...

Now, I could imagine a potential solution. The key situation why we keep
the serdev open and listening is if the driver did try to turn the module
off, but in fact did turn it on (because it was not in sync).

It should be possible to cover this by a timer that is started
in this case. If there is serdev data received after assuming the module
is turned off, the driver has detected the wrong case - and can safely
close the serdev until we want to have it powered on again.

If there is no response after turing off, the module power state is already
in sync and we can close the serdev as well - after the timeout (let's say
30 seconds). Then, the serdev UART can idle. We should open the serdev
and start this timer also in the probe function to catch an initially wrong
state.

But I think we should focus on the basics of this driver first. Then it can
be optimized later on.

BR and thanks,
Nikolaus

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[H. Nikolaus Schaller]

Hi Sebastian,

> Am 04.05.2018 um 13:42 schrieb Sebastian Reichel :
> 
> [+cc Tony]
> 
> Hi,
> 
>> I think it does not need much more (if at all) than a gpio controller on
>> the OMAP3 chip (I think the clocks are active anyways for use by the other
>> UARTs).
>> 
>> We had proposed years ago to reprogram the UART RX pin by pinmux-states
>> into an interrupt gpio but that was rejected because it was not general
>> enough and ugly in the device tree (an rx-gpios record where the rx-line
>> is already connected to the UART-rx).
>> 
>> Then we did experiment with tapping the UART driver and finally the
>> serdev API was developed to solve this problem. Hence we use it now this
>> way.
> 
> Having any UART active on OMAP results in the SoC not entering
> idle/off wasting energy. For normal (i.e. not connected to a peripheral)
> TTYs you can enable runtime autosuspend and configure the RX pin as
> wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
> will lose the first few characters (since the serial device needs some
> time to wakeup). This is for example supported by the N900 uart3
> (debug uart):
> 
> $ grep -A4 " {" arch/arm/boot/dts/omap3-n900.dts 
>  {
>   interrupts-extended = < 74 _pmx_core OMAP3_UART3_RX>;
>   pinctrl-names = "default";
>   pinctrl-0 = <_pins>;
> };
> 
> To get it working, you also need to enable autosuspend for the tty
> in userspace (echo 3000 
> /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
> This is not enabled by default due to the character loss characteristic
> during wakeup.
> 
> Having said all of this, serdev does not yet support runtime PM (at
> all). Tony is currently looking into it. Fortunately serdev allows
> us to enable runtime PM by default (once implemented), since we know
> the remote side and can (hopefully) avoid losing characters (i.e.
> with sideband wakeup gpios).

thanks for explaining this! We originally had similar thoughts when
proposing a w2sg0004 driver for the first time (years ago), but we can
not accept loosing characters...

Now, I could imagine a potential solution. The key situation why we keep
the serdev open and listening is if the driver did try to turn the module
off, but in fact did turn it on (because it was not in sync).

It should be possible to cover this by a timer that is started
in this case. If there is serdev data received after assuming the module
is turned off, the driver has detected the wrong case - and can safely
close the serdev until we want to have it powered on again.

If there is no response after turing off, the module power state is already
in sync and we can close the serdev as well - after the timeout (let's say
30 seconds). Then, the serdev UART can idle. We should open the serdev
and start this timer also in the probe function to catch an initially wrong
state.

But I think we should focus on the basics of this driver first. Then it can
be optimized later on.

BR and thanks,
Nikolaus

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Sebastian Reichel]

[+cc Tony]

Hi,

On Fri, May 04, 2018 at 07:16:00AM +0200, H. Nikolaus Schaller wrote:
> > Am 03.05.2018 um 20:50 schrieb Andreas Kemnade :
> > On Thu, 3 May 2018 11:35:21 +0200
> > H. Nikolaus Schaller  wrote:
> > 
> >> I have realized that the w2sg0004 is an exception (although a Sirf chip)
> >> that it does not provide a WAKEUP signal. And another significant
> >> difference is that we have to keep the serdev UART enabled even if there
> >> is no user-space client. Otherwise we are not able to detect unexpected
> >> activity. So we unfortunately can't move serdev open/close into the .open
> >> and .close ops but need to open it in probe.
> >> 
> > how much power does it use to keep the uart enabled? Or should it
> > better be reprogrammed as gpio?
> 
> I think it does not need much more (if at all) than a gpio controller on
> the OMAP3 chip (I think the clocks are active anyways for use by the other
> UARTs).
>
> We had proposed years ago to reprogram the UART RX pin by pinmux-states
> into an interrupt gpio but that was rejected because it was not general
> enough and ugly in the device tree (an rx-gpios record where the rx-line
> is already connected to the UART-rx).
> 
> Then we did experiment with tapping the UART driver and finally the
> serdev API was developed to solve this problem. Hence we use it now this
> way.

Having any UART active on OMAP results in the SoC not entering
idle/off wasting energy. For normal (i.e. not connected to a peripheral)
TTYs you can enable runtime autosuspend and configure the RX pin as
wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
will lose the first few characters (since the serial device needs some
time to wakeup). This is for example supported by the N900 uart3
(debug uart):

$ grep -A4 " {" arch/arm/boot/dts/omap3-n900.dts 
 {
interrupts-extended = < 74 _pmx_core OMAP3_UART3_RX>;
pinctrl-names = "default";
pinctrl-0 = <_pins>;
};

To get it working, you also need to enable autosuspend for the tty
in userspace (echo 3000 /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
This is not enabled by default due to the character loss characteristic
during wakeup.

Having said all of this, serdev does not yet support runtime PM (at
all). Tony is currently looking into it. Fortunately serdev allows
us to enable runtime PM by default (once implemented), since we know
the remote side and can (hopefully) avoid losing characters (i.e.
with sideband wakeup gpios).

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Sebastian Reichel]

[+cc Tony]

Hi,

On Fri, May 04, 2018 at 07:16:00AM +0200, H. Nikolaus Schaller wrote:
> > Am 03.05.2018 um 20:50 schrieb Andreas Kemnade :
> > On Thu, 3 May 2018 11:35:21 +0200
> > H. Nikolaus Schaller  wrote:
> > 
> >> I have realized that the w2sg0004 is an exception (although a Sirf chip)
> >> that it does not provide a WAKEUP signal. And another significant
> >> difference is that we have to keep the serdev UART enabled even if there
> >> is no user-space client. Otherwise we are not able to detect unexpected
> >> activity. So we unfortunately can't move serdev open/close into the .open
> >> and .close ops but need to open it in probe.
> >> 
> > how much power does it use to keep the uart enabled? Or should it
> > better be reprogrammed as gpio?
> 
> I think it does not need much more (if at all) than a gpio controller on
> the OMAP3 chip (I think the clocks are active anyways for use by the other
> UARTs).
>
> We had proposed years ago to reprogram the UART RX pin by pinmux-states
> into an interrupt gpio but that was rejected because it was not general
> enough and ugly in the device tree (an rx-gpios record where the rx-line
> is already connected to the UART-rx).
> 
> Then we did experiment with tapping the UART driver and finally the
> serdev API was developed to solve this problem. Hence we use it now this
> way.

Having any UART active on OMAP results in the SoC not entering
idle/off wasting energy. For normal (i.e. not connected to a peripheral)
TTYs you can enable runtime autosuspend and configure the RX pin as
wakeup interrupt. This will wakeup the TTY on incoming traffic, but you
will lose the first few characters (since the serial device needs some
time to wakeup). This is for example supported by the N900 uart3
(debug uart):

$ grep -A4 " {" arch/arm/boot/dts/omap3-n900.dts 
 {
interrupts-extended = < 74 _pmx_core OMAP3_UART3_RX>;
pinctrl-names = "default";
pinctrl-0 = <_pins>;
};

To get it working, you also need to enable autosuspend for the tty
in userspace (echo 3000 /sys/class/tty/ttyS2/device/power/autosuspend_delay_ms).
This is not enabled by default due to the character loss characteristic
during wakeup.

Having said all of this, serdev does not yet support runtime PM (at
all). Tony is currently looking into it. Fortunately serdev allows
us to enable runtime PM by default (once implemented), since we know
the remote side and can (hopefully) avoid losing characters (i.e.
with sideband wakeup gpios).

-- Sebastian


signature.asc
Description: PGP signature

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[H. Nikolaus Schaller]

Hi Andreas,

> Am 03.05.2018 um 20:50 schrieb Andreas Kemnade :
> 
> On Thu, 3 May 2018 11:35:21 +0200
> H. Nikolaus Schaller  wrote:
> 
> 
>> I have realized that the w2sg0004 is an exception (although a Sirf chip)
>> that it does not provide a WAKEUP signal. And another significant
>> difference is that we have to keep the serdev UART enabled even if there
>> is no user-space client. Otherwise we are not able to detect unexpected
>> activity. So we unfortunately can't move serdev open/close into the .open
>> and .close ops but need to open it in probe.
>> 
> how much power does it use to keep the uart enabled? Or should it
> better be reprogrammed as gpio?

I think it does not need much more (if at all) than a gpio controller on
the OMAP3 chip (I think the clocks are active anyways for use by the other
UARTs).

We had proposed years ago to reprogram the UART RX pin by pinmux-states
into an interrupt gpio but that was rejected because it was not general
enough and ugly in the device tree (an rx-gpios record where the rx-line
is already connected to the UART-rx).

Then we did experiment with tapping the UART driver and finally the
serdev API was developed to solve this problem. Hence we use it now this
way.

BR and thanks,
Nikolaus




signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[H. Nikolaus Schaller]

Hi Andreas,

> Am 03.05.2018 um 20:50 schrieb Andreas Kemnade :
> 
> On Thu, 3 May 2018 11:35:21 +0200
> H. Nikolaus Schaller  wrote:
> 
> 
>> I have realized that the w2sg0004 is an exception (although a Sirf chip)
>> that it does not provide a WAKEUP signal. And another significant
>> difference is that we have to keep the serdev UART enabled even if there
>> is no user-space client. Otherwise we are not able to detect unexpected
>> activity. So we unfortunately can't move serdev open/close into the .open
>> and .close ops but need to open it in probe.
>> 
> how much power does it use to keep the uart enabled? Or should it
> better be reprogrammed as gpio?

I think it does not need much more (if at all) than a gpio controller on
the OMAP3 chip (I think the clocks are active anyways for use by the other
UARTs).

We had proposed years ago to reprogram the UART RX pin by pinmux-states
into an interrupt gpio but that was rejected because it was not general
enough and ugly in the device tree (an rx-gpios record where the rx-line
is already connected to the UART-rx).

Then we did experiment with tapping the UART driver and finally the
serdev API was developed to solve this problem. Hence we use it now this
way.

BR and thanks,
Nikolaus




signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Andreas Kemnade]

On Thu, 3 May 2018 11:35:21 +0200
H. Nikolaus Schaller  wrote:


> I have realized that the w2sg0004 is an exception (although a Sirf chip)
> that it does not provide a WAKEUP signal. And another significant
> difference is that we have to keep the serdev UART enabled even if there
> is no user-space client. Otherwise we are not able to detect unexpected
> activity. So we unfortunately can't move serdev open/close into the .open
> and .close ops but need to open it in probe.
> 
how much power does it use to keep the uart enabled? Or should it
better be reprogrammed as gpio?

Regards,
Andreas


pgpvIMkv890i2.pgp
Description: OpenPGP digital signature

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Andreas Kemnade]

On Thu, 3 May 2018 11:35:21 +0200
H. Nikolaus Schaller  wrote:


> I have realized that the w2sg0004 is an exception (although a Sirf chip)
> that it does not provide a WAKEUP signal. And another significant
> difference is that we have to keep the serdev UART enabled even if there
> is no user-space client. Otherwise we are not able to detect unexpected
> activity. So we unfortunately can't move serdev open/close into the .open
> and .close ops but need to open it in probe.
> 
how much power does it use to keep the uart enabled? Or should it
better be reprogrammed as gpio?

Regards,
Andreas


pgpvIMkv890i2.pgp
Description: OpenPGP digital signature

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Wed, May 2, 2018 at 3:16 AM, Johan Hovold  wrote:
> On Tue, May 01, 2018 at 09:05:42AM -0500, Rob Herring wrote:
>> On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold  wrote:
>> > On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
>> >> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
>> >> > Add binding for u-blox GNSS receivers.
>> >> >
>> >> > Note that the u-blox product names encodes form factor (e.g. "neo"),
>> >> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
>> >> > chipset is used for the compatible strings (for now).
>> >> >
>> >> > Signed-off-by: Johan Hovold 
>> >> > ---
>> >> >  .../devicetree/bindings/gnss/u-blox.txt   | 31 +++
>> >> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>> >> >  2 files changed, 32 insertions(+)
>> >> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt 
>> >> > b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> > new file mode 100644
>> >> > index ..bb54b83a177f
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> > @@ -0,0 +1,31 @@
>> >> > +u-blox GNSS Receiver DT binding
>> >> > +
>> >> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB 
>> >> > interfaces.
>> >> > +
>> >> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
>> >> > +properties.
>> >> > +
>> >> > +Required Properties:
>> >> > +
>> >> > +- compatible   : Must be one of
>> >> > +
>> >> > +   "u-blox,neo-8"
>> >> > +   "u-blox,neo-m8"
>> >> > +
>> >> > +- vcc-supply   : Main voltage regulator (VCC)
>> >>
>> >> What about V_BCKP?
>> >
>> > That's the backup supply for for the RTC and batter-backed RAM. In
>> > configurations where a battery is not used it should be connected to
>> > VCC.
>> >
>> > How would you model that? I can enable a vbckp regulator at probe, but
>> > what if someone then accurately describes the corresponding pin as being
>> > connected to VCC?
>>
>> You mean how to model a battery? It would just be a 'regulator'
>> because the regulator binding covers any supply really.
>>
>> Then you just set both rails to the same supply phandle.
>
> Yes, but...
>
>> > I guess we can check if the regulators are identical,
>> > and then just have the driver ignore V_BKUP. Knowing whether there is
>> > a (hopefully charged) battery connected could be useful.
>>
>> Regulators are ref counted, so just enable it twice. Or the driver can
>> just ignore it until it supports battery backup.
>
> ...my point was that in case there's no backup battery, you don't want
> to enable vcc (via v_bckp) at probe (and instead have the device cold
> boot whenever it's used).

Wouldn't that result in very long acquisition times? I guess I was
thinking Vcc would be always on when running and V_bckp was just for
suspend.

> Hence, the driver would need to check if the v_bckp-supply is identical
> to vcc and not enable the former at probe in that case (i.e. similar to
> if no v_bckp had been specified and we considered it optional).

I guess if that's the intended operation, then making it optional is fine.

Rob

>
>> >> > +- timepulse-gpios  : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
>> >>
>> >> Why the 3rd "TIMEPULSE"?
>> >
>> > That's the pin name, which in this case is identical to the property
>> > name, so I'll drop it here.
>>
>> Then what is the 2nd "Timepulse"?
>
> That's the generic function name.
>
>> Maybe just a "pin name: X" prefix so it is clear.
>
> For u-blox devices, where property-, function- and pin name coincide, I
> could just change this to:
>
> +- timepulse-gpios  : Timepulse GPIO
>
> and then for the sirfstar binding, which will be used by devices from
> multiple vendors which have decided to name their pins differently, I
> can add a "pin name: " prefix for clarity?

Sounds good.

>> > Take a look at the sirf binding; vendors use different names for their
>> > timepulse pins and in that case I added the actual pin names (1PPS, TM)
>> > in parenthesis after the description.
>> >
>> > Note that I mentioned "timepulse-gpios" in the generic binding with the
>> > intent of trying to enforce a generic name for pins with such a
>> > function (similarly for "enable-gpios", which I guess is already
>> > established).
>>
>> Yes, I think that's good.
>>
>> Though with the enable-gpios I was debating the name for sirfstar a
>> bit because it isn't the normal drive it active to enable, but rather
>> a pulse to enable or disable.
>
> I had some concerns along those lines as well, and if you agree I'll
> change the property name to on_off-gpios (or onoff-gpios) which matches
> the vendor data sheets for this pin, and which I think would be better.

Okay, just add a vendor prefix. And note that using '_' is discouraged.

Rob

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Wed, May 2, 2018 at 3:16 AM, Johan Hovold  wrote:
> On Tue, May 01, 2018 at 09:05:42AM -0500, Rob Herring wrote:
>> On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold  wrote:
>> > On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
>> >> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
>> >> > Add binding for u-blox GNSS receivers.
>> >> >
>> >> > Note that the u-blox product names encodes form factor (e.g. "neo"),
>> >> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
>> >> > chipset is used for the compatible strings (for now).
>> >> >
>> >> > Signed-off-by: Johan Hovold 
>> >> > ---
>> >> >  .../devicetree/bindings/gnss/u-blox.txt   | 31 +++
>> >> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>> >> >  2 files changed, 32 insertions(+)
>> >> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt 
>> >> > b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> > new file mode 100644
>> >> > index ..bb54b83a177f
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> >> > @@ -0,0 +1,31 @@
>> >> > +u-blox GNSS Receiver DT binding
>> >> > +
>> >> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB 
>> >> > interfaces.
>> >> > +
>> >> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
>> >> > +properties.
>> >> > +
>> >> > +Required Properties:
>> >> > +
>> >> > +- compatible   : Must be one of
>> >> > +
>> >> > +   "u-blox,neo-8"
>> >> > +   "u-blox,neo-m8"
>> >> > +
>> >> > +- vcc-supply   : Main voltage regulator (VCC)
>> >>
>> >> What about V_BCKP?
>> >
>> > That's the backup supply for for the RTC and batter-backed RAM. In
>> > configurations where a battery is not used it should be connected to
>> > VCC.
>> >
>> > How would you model that? I can enable a vbckp regulator at probe, but
>> > what if someone then accurately describes the corresponding pin as being
>> > connected to VCC?
>>
>> You mean how to model a battery? It would just be a 'regulator'
>> because the regulator binding covers any supply really.
>>
>> Then you just set both rails to the same supply phandle.
>
> Yes, but...
>
>> > I guess we can check if the regulators are identical,
>> > and then just have the driver ignore V_BKUP. Knowing whether there is
>> > a (hopefully charged) battery connected could be useful.
>>
>> Regulators are ref counted, so just enable it twice. Or the driver can
>> just ignore it until it supports battery backup.
>
> ...my point was that in case there's no backup battery, you don't want
> to enable vcc (via v_bckp) at probe (and instead have the device cold
> boot whenever it's used).

Wouldn't that result in very long acquisition times? I guess I was
thinking Vcc would be always on when running and V_bckp was just for
suspend.

> Hence, the driver would need to check if the v_bckp-supply is identical
> to vcc and not enable the former at probe in that case (i.e. similar to
> if no v_bckp had been specified and we considered it optional).

I guess if that's the intended operation, then making it optional is fine.

Rob

>
>> >> > +- timepulse-gpios  : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
>> >>
>> >> Why the 3rd "TIMEPULSE"?
>> >
>> > That's the pin name, which in this case is identical to the property
>> > name, so I'll drop it here.
>>
>> Then what is the 2nd "Timepulse"?
>
> That's the generic function name.
>
>> Maybe just a "pin name: X" prefix so it is clear.
>
> For u-blox devices, where property-, function- and pin name coincide, I
> could just change this to:
>
> +- timepulse-gpios  : Timepulse GPIO
>
> and then for the sirfstar binding, which will be used by devices from
> multiple vendors which have decided to name their pins differently, I
> can add a "pin name: " prefix for clarity?

Sounds good.

>> > Take a look at the sirf binding; vendors use different names for their
>> > timepulse pins and in that case I added the actual pin names (1PPS, TM)
>> > in parenthesis after the description.
>> >
>> > Note that I mentioned "timepulse-gpios" in the generic binding with the
>> > intent of trying to enforce a generic name for pins with such a
>> > function (similarly for "enable-gpios", which I guess is already
>> > established).
>>
>> Yes, I think that's good.
>>
>> Though with the enable-gpios I was debating the name for sirfstar a
>> bit because it isn't the normal drive it active to enable, but rather
>> a pulse to enable or disable.
>
> I had some concerns along those lines as well, and if you agree I'll
> change the property name to on_off-gpios (or onoff-gpios) which matches
> the vendor data sheets for this pin, and which I think would be better.

Okay, just add a vendor prefix. And note that using '_' is discouraged.

Rob

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Tue, May 01, 2018 at 09:05:42AM -0500, Rob Herring wrote:
> On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold  wrote:
> > On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
> >> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
> >> > Add binding for u-blox GNSS receivers.
> >> >
> >> > Note that the u-blox product names encodes form factor (e.g. "neo"),
> >> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> >> > chipset is used for the compatible strings (for now).
> >> >
> >> > Signed-off-by: Johan Hovold 
> >> > ---
> >> >  .../devicetree/bindings/gnss/u-blox.txt   | 31 +++
> >> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
> >> >  2 files changed, 32 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt 
> >> > b/Documentation/devicetree/bindings/gnss/u-blox.txt
> >> > new file mode 100644
> >> > index ..bb54b83a177f
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> >> > @@ -0,0 +1,31 @@
> >> > +u-blox GNSS Receiver DT binding
> >> > +
> >> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB 
> >> > interfaces.
> >> > +
> >> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> >> > +properties.
> >> > +
> >> > +Required Properties:
> >> > +
> >> > +- compatible   : Must be one of
> >> > +
> >> > +   "u-blox,neo-8"
> >> > +   "u-blox,neo-m8"
> >> > +
> >> > +- vcc-supply   : Main voltage regulator (VCC)
> >>
> >> What about V_BCKP?
> >
> > That's the backup supply for for the RTC and batter-backed RAM. In
> > configurations where a battery is not used it should be connected to
> > VCC.
> >
> > How would you model that? I can enable a vbckp regulator at probe, but
> > what if someone then accurately describes the corresponding pin as being
> > connected to VCC?
> 
> You mean how to model a battery? It would just be a 'regulator'
> because the regulator binding covers any supply really.
> 
> Then you just set both rails to the same supply phandle.

Yes, but...

> > I guess we can check if the regulators are identical,
> > and then just have the driver ignore V_BKUP. Knowing whether there is
> > a (hopefully charged) battery connected could be useful.
> 
> Regulators are ref counted, so just enable it twice. Or the driver can
> just ignore it until it supports battery backup.

...my point was that in case there's no backup battery, you don't want
to enable vcc (via v_bckp) at probe (and instead have the device cold
boot whenever it's used).

Hence, the driver would need to check if the v_bckp-supply is identical
to vcc and not enable the former at probe in that case (i.e. similar to
if no v_bckp had been specified and we considered it optional).

> >> > +- timepulse-gpios  : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
> >>
> >> Why the 3rd "TIMEPULSE"?
> >
> > That's the pin name, which in this case is identical to the property
> > name, so I'll drop it here.
> 
> Then what is the 2nd "Timepulse"?

That's the generic function name.

> Maybe just a "pin name: X" prefix so it is clear.

For u-blox devices, where property-, function- and pin name coincide, I
could just change this to:

+- timepulse-gpios  : Timepulse GPIO

and then for the sirfstar binding, which will be used by devices from
multiple vendors which have decided to name their pins differently, I
can add a "pin name: " prefix for clarity?

> > Take a look at the sirf binding; vendors use different names for their
> > timepulse pins and in that case I added the actual pin names (1PPS, TM)
> > in parenthesis after the description.
> >
> > Note that I mentioned "timepulse-gpios" in the generic binding with the
> > intent of trying to enforce a generic name for pins with such a
> > function (similarly for "enable-gpios", which I guess is already
> > established).
> 
> Yes, I think that's good.
> 
> Though with the enable-gpios I was debating the name for sirfstar a
> bit because it isn't the normal drive it active to enable, but rather
> a pulse to enable or disable.

I had some concerns along those lines as well, and if you agree I'll
change the property name to on_off-gpios (or onoff-gpios) which matches
the vendor data sheets for this pin, and which I think would be better.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Tue, May 01, 2018 at 09:05:42AM -0500, Rob Herring wrote:
> On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold  wrote:
> > On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
> >> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
> >> > Add binding for u-blox GNSS receivers.
> >> >
> >> > Note that the u-blox product names encodes form factor (e.g. "neo"),
> >> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> >> > chipset is used for the compatible strings (for now).
> >> >
> >> > Signed-off-by: Johan Hovold 
> >> > ---
> >> >  .../devicetree/bindings/gnss/u-blox.txt   | 31 +++
> >> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
> >> >  2 files changed, 32 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt 
> >> > b/Documentation/devicetree/bindings/gnss/u-blox.txt
> >> > new file mode 100644
> >> > index ..bb54b83a177f
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> >> > @@ -0,0 +1,31 @@
> >> > +u-blox GNSS Receiver DT binding
> >> > +
> >> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB 
> >> > interfaces.
> >> > +
> >> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> >> > +properties.
> >> > +
> >> > +Required Properties:
> >> > +
> >> > +- compatible   : Must be one of
> >> > +
> >> > +   "u-blox,neo-8"
> >> > +   "u-blox,neo-m8"
> >> > +
> >> > +- vcc-supply   : Main voltage regulator (VCC)
> >>
> >> What about V_BCKP?
> >
> > That's the backup supply for for the RTC and batter-backed RAM. In
> > configurations where a battery is not used it should be connected to
> > VCC.
> >
> > How would you model that? I can enable a vbckp regulator at probe, but
> > what if someone then accurately describes the corresponding pin as being
> > connected to VCC?
> 
> You mean how to model a battery? It would just be a 'regulator'
> because the regulator binding covers any supply really.
> 
> Then you just set both rails to the same supply phandle.

Yes, but...

> > I guess we can check if the regulators are identical,
> > and then just have the driver ignore V_BKUP. Knowing whether there is
> > a (hopefully charged) battery connected could be useful.
> 
> Regulators are ref counted, so just enable it twice. Or the driver can
> just ignore it until it supports battery backup.

...my point was that in case there's no backup battery, you don't want
to enable vcc (via v_bckp) at probe (and instead have the device cold
boot whenever it's used).

Hence, the driver would need to check if the v_bckp-supply is identical
to vcc and not enable the former at probe in that case (i.e. similar to
if no v_bckp had been specified and we considered it optional).

> >> > +- timepulse-gpios  : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
> >>
> >> Why the 3rd "TIMEPULSE"?
> >
> > That's the pin name, which in this case is identical to the property
> > name, so I'll drop it here.
> 
> Then what is the 2nd "Timepulse"?

That's the generic function name.

> Maybe just a "pin name: X" prefix so it is clear.

For u-blox devices, where property-, function- and pin name coincide, I
could just change this to:

+- timepulse-gpios  : Timepulse GPIO

and then for the sirfstar binding, which will be used by devices from
multiple vendors which have decided to name their pins differently, I
can add a "pin name: " prefix for clarity?

> > Take a look at the sirf binding; vendors use different names for their
> > timepulse pins and in that case I added the actual pin names (1PPS, TM)
> > in parenthesis after the description.
> >
> > Note that I mentioned "timepulse-gpios" in the generic binding with the
> > intent of trying to enforce a generic name for pins with such a
> > function (similarly for "enable-gpios", which I guess is already
> > established).
> 
> Yes, I think that's good.
> 
> Though with the enable-gpios I was debating the name for sirfstar a
> bit because it isn't the normal drive it active to enable, but rather
> a pulse to enable or disable.

I had some concerns along those lines as well, and if you agree I'll
change the property name to on_off-gpios (or onoff-gpios) which matches
the vendor data sheets for this pin, and which I think would be better.

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold  wrote:
> On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
>> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
>> > Add binding for u-blox GNSS receivers.
>> >
>> > Note that the u-blox product names encodes form factor (e.g. "neo"),
>> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
>> > chipset is used for the compatible strings (for now).
>> >
>> > Signed-off-by: Johan Hovold 
>> > ---
>> >  .../devicetree/bindings/gnss/u-blox.txt   | 31 +++
>> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>> >  2 files changed, 32 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt 
>> > b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> > new file mode 100644
>> > index ..bb54b83a177f
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> > @@ -0,0 +1,31 @@
>> > +u-blox GNSS Receiver DT binding
>> > +
>> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
>> > +
>> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
>> > +properties.
>> > +
>> > +Required Properties:
>> > +
>> > +- compatible   : Must be one of
>> > +
>> > +   "u-blox,neo-8"
>> > +   "u-blox,neo-m8"
>> > +
>> > +- vcc-supply   : Main voltage regulator (VCC)
>>
>> What about V_BCKP?
>
> That's the backup supply for for the RTC and batter-backed RAM. In
> configurations where a battery is not used it should be connected to
> VCC.
>
> How would you model that? I can enable a vbckp regulator at probe, but
> what if someone then accurately describes the corresponding pin as being
> connected to VCC?

You mean how to model a battery? It would just be a 'regulator'
because the regulator binding covers any supply really.

Then you just set both rails to the same supply phandle.

> I guess we can check if the regulators are identical,
> and then just have the driver ignore V_BKUP. Knowing whether there is
> a (hopefully charged) battery connected could be useful.

Regulators are ref counted, so just enable it twice. Or the driver can
just ignore it until it supports battery backup.

> I can't seem to find any other bindings that describe battery supplies,
> but I'll add it here.

There must be some. But then the battery and charger bindings are not
in the best shape.

>> > +
>> > +Optional Properties:
>>
>> reg is required if using I2C, SPI, or USB.
>
> I'll add that (even if there is no driver support for these yet).
>
>> Datasheet also shows an interrupt pin.
>
> Not used currently, but I'll add it to the binding as well.
>
>> > +
>> > +- timepulse-gpios  : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
>>
>> Why the 3rd "TIMEPULSE"?
>
> That's the pin name, which in this case is identical to the property
> name, so I'll drop it here.

Then what is the 2nd "Timepulse"?

Maybe just a "pin name: X" prefix so it is clear.

> Take a look at the sirf binding; vendors use different names for their
> timepulse pins and in that case I added the actual pin names (1PPS, TM)
> in parenthesis after the description.
>
> Note that I mentioned "timepulse-gpios" in the generic binding with the
> intent of trying to enforce a generic name for pins with such a
> function (similarly for "enable-gpios", which I guess is already
> established).

Yes, I think that's good.

Though with the enable-gpios I was debating the name for sirfstar a
bit because it isn't the normal drive it active to enable, but rather
a pulse to enable or disable.

Rob

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Thu, Apr 26, 2018 at 4:10 AM, Johan Hovold  wrote:
> On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
>> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
>> > Add binding for u-blox GNSS receivers.
>> >
>> > Note that the u-blox product names encodes form factor (e.g. "neo"),
>> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
>> > chipset is used for the compatible strings (for now).
>> >
>> > Signed-off-by: Johan Hovold 
>> > ---
>> >  .../devicetree/bindings/gnss/u-blox.txt   | 31 +++
>> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>> >  2 files changed, 32 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt 
>> > b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> > new file mode 100644
>> > index ..bb54b83a177f
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
>> > @@ -0,0 +1,31 @@
>> > +u-blox GNSS Receiver DT binding
>> > +
>> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
>> > +
>> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
>> > +properties.
>> > +
>> > +Required Properties:
>> > +
>> > +- compatible   : Must be one of
>> > +
>> > +   "u-blox,neo-8"
>> > +   "u-blox,neo-m8"
>> > +
>> > +- vcc-supply   : Main voltage regulator (VCC)
>>
>> What about V_BCKP?
>
> That's the backup supply for for the RTC and batter-backed RAM. In
> configurations where a battery is not used it should be connected to
> VCC.
>
> How would you model that? I can enable a vbckp regulator at probe, but
> what if someone then accurately describes the corresponding pin as being
> connected to VCC?

You mean how to model a battery? It would just be a 'regulator'
because the regulator binding covers any supply really.

Then you just set both rails to the same supply phandle.

> I guess we can check if the regulators are identical,
> and then just have the driver ignore V_BKUP. Knowing whether there is
> a (hopefully charged) battery connected could be useful.

Regulators are ref counted, so just enable it twice. Or the driver can
just ignore it until it supports battery backup.

> I can't seem to find any other bindings that describe battery supplies,
> but I'll add it here.

There must be some. But then the battery and charger bindings are not
in the best shape.

>> > +
>> > +Optional Properties:
>>
>> reg is required if using I2C, SPI, or USB.
>
> I'll add that (even if there is no driver support for these yet).
>
>> Datasheet also shows an interrupt pin.
>
> Not used currently, but I'll add it to the binding as well.
>
>> > +
>> > +- timepulse-gpios  : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
>>
>> Why the 3rd "TIMEPULSE"?
>
> That's the pin name, which in this case is identical to the property
> name, so I'll drop it here.

Then what is the 2nd "Timepulse"?

Maybe just a "pin name: X" prefix so it is clear.

> Take a look at the sirf binding; vendors use different names for their
> timepulse pins and in that case I added the actual pin names (1PPS, TM)
> in parenthesis after the description.
>
> Note that I mentioned "timepulse-gpios" in the generic binding with the
> intent of trying to enforce a generic name for pins with such a
> function (similarly for "enable-gpios", which I guess is already
> established).

Yes, I think that's good.

Though with the enable-gpios I was debating the name for sirfstar a
bit because it isn't the normal drive it active to enable, but rather
a pulse to enable or disable.

Rob

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
> > Add binding for u-blox GNSS receivers.
> >
> > Note that the u-blox product names encodes form factor (e.g. "neo"),
> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> > chipset is used for the compatible strings (for now).
> >
> > Signed-off-by: Johan Hovold 
> > ---
> >  .../devicetree/bindings/gnss/u-blox.txt   | 31 +++
> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
> >  2 files changed, 32 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt 
> > b/Documentation/devicetree/bindings/gnss/u-blox.txt
> > new file mode 100644
> > index ..bb54b83a177f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> > @@ -0,0 +1,31 @@
> > +u-blox GNSS Receiver DT binding
> > +
> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
> > +
> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> > +properties.
> > +
> > +Required Properties:
> > +
> > +- compatible   : Must be one of
> > +
> > +   "u-blox,neo-8"
> > +   "u-blox,neo-m8"
> > +
> > +- vcc-supply   : Main voltage regulator (VCC)
> 
> What about V_BCKP?

That's the backup supply for for the RTC and batter-backed RAM. In
configurations where a battery is not used it should be connected to
VCC.

How would you model that? I can enable a vbckp regulator at probe, but
what if someone then accurately describes the corresponding pin as being
connected to VCC? I guess we can check if the regulators are identical,
and then just have the driver ignore V_BKUP. Knowing whether there is
a (hopefully charged) battery connected could be useful.

I can't seem to find any other bindings that describe battery supplies,
but I'll add it here.

> > +
> > +Optional Properties:
> 
> reg is required if using I2C, SPI, or USB.

I'll add that (even if there is no driver support for these yet).

> Datasheet also shows an interrupt pin.

Not used currently, but I'll add it to the binding as well.

> > +
> > +- timepulse-gpios  : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
> 
> Why the 3rd "TIMEPULSE"?

That's the pin name, which in this case is identical to the property
name, so I'll drop it here.

Take a look at the sirf binding; vendors use different names for their
timepulse pins and in that case I added the actual pin names (1PPS, TM)
in parenthesis after the description.

Note that I mentioned "timepulse-gpios" in the generic binding with the
intent of trying to enforce a generic name for pins with such a
function (similarly for "enable-gpios", which I guess is already
established).

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Johan Hovold]

On Wed, Apr 25, 2018 at 01:16:58PM -0500, Rob Herring wrote:
> On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
> > Add binding for u-blox GNSS receivers.
> >
> > Note that the u-blox product names encodes form factor (e.g. "neo"),
> > chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> > chipset is used for the compatible strings (for now).
> >
> > Signed-off-by: Johan Hovold 
> > ---
> >  .../devicetree/bindings/gnss/u-blox.txt   | 31 +++
> >  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
> >  2 files changed, 32 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt 
> > b/Documentation/devicetree/bindings/gnss/u-blox.txt
> > new file mode 100644
> > index ..bb54b83a177f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> > @@ -0,0 +1,31 @@
> > +u-blox GNSS Receiver DT binding
> > +
> > +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
> > +
> > +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> > +properties.
> > +
> > +Required Properties:
> > +
> > +- compatible   : Must be one of
> > +
> > +   "u-blox,neo-8"
> > +   "u-blox,neo-m8"
> > +
> > +- vcc-supply   : Main voltage regulator (VCC)
> 
> What about V_BCKP?

That's the backup supply for for the RTC and batter-backed RAM. In
configurations where a battery is not used it should be connected to
VCC.

How would you model that? I can enable a vbckp regulator at probe, but
what if someone then accurately describes the corresponding pin as being
connected to VCC? I guess we can check if the regulators are identical,
and then just have the driver ignore V_BKUP. Knowing whether there is
a (hopefully charged) battery connected could be useful.

I can't seem to find any other bindings that describe battery supplies,
but I'll add it here.

> > +
> > +Optional Properties:
> 
> reg is required if using I2C, SPI, or USB.

I'll add that (even if there is no driver support for these yet).

> Datasheet also shows an interrupt pin.

Not used currently, but I'll add it to the binding as well.

> > +
> > +- timepulse-gpios  : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)
> 
> Why the 3rd "TIMEPULSE"?

That's the pin name, which in this case is identical to the property
name, so I'll drop it here.

Take a look at the sirf binding; vendors use different names for their
timepulse pins and in that case I added the actual pin names (1PPS, TM)
in parenthesis after the description.

Note that I mentioned "timepulse-gpios" in the generic binding with the
intent of trying to enforce a generic name for pins with such a
function (similarly for "enable-gpios", which I guess is already
established).

Thanks,
Johan

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
> Add binding for u-blox GNSS receivers.
>
> Note that the u-blox product names encodes form factor (e.g. "neo"),
> chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> chipset is used for the compatible strings (for now).
>
> Signed-off-by: Johan Hovold 
> ---
>  .../devicetree/bindings/gnss/u-blox.txt   | 31 +++
>  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>  2 files changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>
> diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt 
> b/Documentation/devicetree/bindings/gnss/u-blox.txt
> new file mode 100644
> index ..bb54b83a177f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> @@ -0,0 +1,31 @@
> +u-blox GNSS Receiver DT binding
> +
> +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
> +
> +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> +properties.
> +
> +Required Properties:
> +
> +- compatible   : Must be one of
> +
> +   "u-blox,neo-8"
> +   "u-blox,neo-m8"
> +
> +- vcc-supply   : Main voltage regulator (VCC)

What about V_BCKP?

> +
> +Optional Properties:

reg is required if using I2C, SPI, or USB.

Datasheet also shows an interrupt pin.

> +
> +- timepulse-gpios  : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)

Why the 3rd "TIMEPULSE"?

> +
> +Example:
> +
> +serial@1234 {
> +   compatible = "ns16550a";
> +
> +   gnss {
> +   compatible = "u-blox,neo-8";
> +
> +   vcc-supply = <_reg>;
> +   };
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index b5f978a4cac6..2128dfdf73f1 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -374,6 +374,7 @@ tronsmart   Tronsmart
>  truly  Truly Semiconductors Limited
>  tsdTheobroma Systems Design und Consulting GmbH
>  tyan   Tyan Computer Corporation
> +u-blox u-blox
>  ucrobotics uCRobotics
>  ubnt   Ubiquiti Networks
>  udoo   Udoo
> --
> 2.17.0
>

Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding

[Rob Herring]

On Tue, Apr 24, 2018 at 11:34 AM, Johan Hovold  wrote:
> Add binding for u-blox GNSS receivers.
>
> Note that the u-blox product names encodes form factor (e.g. "neo"),
> chipset (e.g. "8") and variant (e.g. "q"), but that only formfactor and
> chipset is used for the compatible strings (for now).
>
> Signed-off-by: Johan Hovold 
> ---
>  .../devicetree/bindings/gnss/u-blox.txt   | 31 +++
>  .../devicetree/bindings/vendor-prefixes.txt   |  1 +
>  2 files changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gnss/u-blox.txt
>
> diff --git a/Documentation/devicetree/bindings/gnss/u-blox.txt 
> b/Documentation/devicetree/bindings/gnss/u-blox.txt
> new file mode 100644
> index ..bb54b83a177f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gnss/u-blox.txt
> @@ -0,0 +1,31 @@
> +u-blox GNSS Receiver DT binding
> +
> +The u-blox GNSS receivers can use UART, DDC (I2C), SPI and USB interfaces.
> +
> +Please see Documentation/devicetree/bindings/gnss/gnss.txt for generic
> +properties.
> +
> +Required Properties:
> +
> +- compatible   : Must be one of
> +
> +   "u-blox,neo-8"
> +   "u-blox,neo-m8"
> +
> +- vcc-supply   : Main voltage regulator (VCC)

What about V_BCKP?

> +
> +Optional Properties:

reg is required if using I2C, SPI, or USB.

Datasheet also shows an interrupt pin.

> +
> +- timepulse-gpios  : Timepulse (e.g. 1PPS) GPIO (TIMEPULSE)

Why the 3rd "TIMEPULSE"?

> +
> +Example:
> +
> +serial@1234 {
> +   compatible = "ns16550a";
> +
> +   gnss {
> +   compatible = "u-blox,neo-8";
> +
> +   vcc-supply = <_reg>;
> +   };
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index b5f978a4cac6..2128dfdf73f1 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -374,6 +374,7 @@ tronsmart   Tronsmart
>  truly  Truly Semiconductors Limited
>  tsdTheobroma Systems Design und Consulting GmbH
>  tyan   Tyan Computer Corporation
> +u-blox u-blox
>  ucrobotics uCRobotics
>  ubnt   Ubiquiti Networks
>  udoo   Udoo
> --
> 2.17.0
>