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

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]

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 <johan@kernel.org> [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-johan@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(&serdev->dev);
}

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

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]