Re: [PATCH v2 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Andreas Kemnade <andreas@kemnade.info>]

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

Hi Johan,

On Thu, 10 Jan 2019 13:10:38 +0100
Johan Hovold <johan@kernel.org> wrote:

> Please change the commit summary to something more descriptive like
> 
> 	gnss: sirf: add support for configurations without wakeup signal
> 
> On Sun, Dec 09, 2018 at 08:51:47PM +0100, Andreas Kemnade wrote:
> > Some Wi2Wi devices do not have a wakeup output, so device state can
> > only be indirectly detected by looking whether there is communitcation
> > over the serial lines.  
> 
> > Additionally it checks for the initial state of the device during
> > probing to ensure it is off.  
> 
> Is this really needed? If so, it should probably go in a separate patch.
> 
Well, it is the main motivation for the new try of upstreaming this again.
You know the loooong history...
It has several times messed up my power consumption statistics. As I try
to test patches on top of mainline, this has often led to false alarms
regarding pm bugs in other drivers.

We could also come from another kernel here via kexec having the device in
another state. 

And why we do not want to check for uncommon things here? We e.g. do multiple tries
for changing power state. 

> > Timeout values need to be increased, because the reaction on serial line
> > is slower and are in line  with previous patches by  
> 
> I don't think this is an accurate description, but more on that below.
> 
I do not think so, but I do not have a too strong opinion here.

> > Neil Brown <neilb@suse.de> and  H. Nikolaus Schaller <hns@goldelico.com>.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > Changes in v2:
> >  - style cleanup
> >  - do not keep serdev open just because runtime is active,
> >    only when needed (gnss device is opened or state is changed)
> >  - clearer timeout semantics
> > 
> >  drivers/gnss/sirf.c | 114 +++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 91 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> > index ba663de1db49..c64369494afb 100644
> > --- a/drivers/gnss/sirf.c
> > +++ b/drivers/gnss/sirf.c
> > @@ -23,8 +23,13 @@
> >  
> >  #define SIRF_BOOT_DELAY			500
> >  #define SIRF_ON_OFF_PULSE_TIME		100
> > +/* activate till reaction of wakeup pin */
> >  #define SIRF_ACTIVATE_TIMEOUT		200
> > +/* activate till reception of data */
> > +#define SIRF_ACTIVATE_TILL_OUTPUT_TIMEOUT	1000
> >  #define SIRF_HIBERNATE_TIMEOUT		200
> > +/* If no data arrives for this time, we expect the chip to be off. */
> > +#define SIRF_MIN_SILENCE	1000  
> 
> You only need to add one new define for the no-wakeup case and it should
> reflect the report cycle (e.g. name it SIRF_NOWAKEUP_REPORT_CYCLE).
> Specifically, it is the time we must wait in order to infer that a
> device has failed to be activated, or succeeded to hibernate.
> 
GPS chips will have usually some boot messages. So it is not the
"send nmea data set every X seconds" for the wakeup case, it is
another situation deserving IMHO another name.

> In pseudo code we have:
> 
>   activate:
>    - toggle on-off
>    - wait(data-received, ACTIVATE_TIMEOUT + REPORT_CYCLE)
>      - reception: success 

Note: we can also get the goodbye/shutdown message from the chip here
so there are chances of a false success, but since we initially power down,
we will rule out wrong state here.

>      - timeout: failure
> 
>   hibernate:
>    - toggle on-off
>    - sleep(HIBERNATE_TIMEOUT)
we could also additionally check here for 
   if (last_bytes_received == GOODBYE_MSG)
or alternatively check for
   if (!BOOTUP_MSG_RECEIVED)
     - return success

>    - wait(data-received, REPORT_CYCLE)
>      - reception: failure
>      - timeout: success
> 
> A problem with your implementation, is that you assume a report cycle
> of one second, but this need to be the case. Judging from a quick look
> at the sirf protocols (sirf nmea and sirf binary) report cycles can be
> configured to be as long as 30s (sirf binary) or even 255s (sirf nmea).
> [ To make things worse these numbers can be even higher in some
> low-power modes it seems. ]
> 
As far as I see we will only might have a problem if 
  - those settings are nonvolatile (or we come in with those
    settings on another way)
  - or a state change lateron fails which we cannot properly detect


> Adding just a one-second timeout (the minimum supported cycle length)
> seems way too low, even if whatever value we choose will be reflected
> directly in the time it takes to hibernate (suspend) the device.
> 
> And since this is configurable at runtime, the only safe value would be
> the maximum one...
> 
> Perhaps we can say that no-wakeup support depends on having reasonably
> short report cycles, but this needs to be documented.
> 
Where should it be documented? Comment in code?
devicetree bindings?

So in general I see these possibilities:
1. just document tho problem with low cycle length requirement
   lateron we might improve the solution

2. do the data parsing like above just for the first or/and last bytes
   if these things really come reliable, we would catch the low-power
   corner cases like only data reports if you move around.
   I have to do some research here.

3. monitor serial output in off state and send a pulse if data arrives
  this would require the serial device to do aggressive power saving
  in that time(you send an example patch for that) or giving a gpio interrupt
  coming from the rx line to the gnss driver.
  Those things look like more restructuring work (or having a separate driver
  which is not that practical for further extensions) and would not
  catch low-power modes

My personal opinion here
is
first 1.
later improve to 2. (and do some more tests in regards of 2.)

Just for the general things first.

Regards,
Andreas

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