Re: [Gta04-owner] [PATCH 0/4] UART slave device support - version 4

[H. Nikolaus Schaller]

Hi Alan,

Am 17.01.2016 um 20:38 schrieb One Thousand Gnomes <gnomes@xxxxxxxxxxxxxxxxxxx>:

>> 3. mechanism to open/close the UART by the peer driver (for power management of the
>> UART), even if it there is a user-space tty client for the same UART which might or might
>> not be open at any time. Manage that in an optimal way.
> 
> Power management doesn't have to go via the uart layer. You can even
> manage it via the existing sysfs power interfaces (and some Android
> devices do in fact do exactly that either by controlling the uart pm or
> using a gpio line). Not pretty either but means people are only peeing
> in their own backyard. What goes into the kernel costs *everyone*, what
> goes in device user space costs only the perpetrator.
> 
> It would help to rewind and understand what your needs are not what your
> implementation as it stands is. Why can't you just use hciattach like
> everyone else ? Improving the PM logic by refining the existing hci
> ldisc to be power smart so everyone benefits from any improvement might
> be a useful other discussion.

I have not counted how often I have explained that, but I am happy to do it
again.

* the GTA04 device is an open hackable smartphone platform where power saving is highest priority
* the platform includes the kernel, but we don't want to prescribe any specific user space (i.e. we can't assume that a specific daemon exists or can't modify it)
* the wi2wi,w2sg0084 chip is a GPS chip (it does not understand HCI protocol, it just sends NMEA records if powered on)
* we want to provide NMEA data through /dev/ttyO1 (it uses an OMAP UART) because a tty port is the most common interface for GPS devices (e.g. a bluetooth GPS mouse is also presented as a tty)
* we want the chip to automatically power up as soon (but not before) as any gps client opens /dev/ttyO1 (or activates the DTR mctrl)
* we want the chip to automatically power down if no process uses /dev/ttyO1 any more

The standard logic of GPS daemons and applications is to receive
NMEA records through some serial /dev/tty.

Please tell me how power on/off management can be done without intercepting
somewhere in the kernel that /dev/ttyO1 is opened/closed (which is not the same
as suspend/resume).

Secondly, the chip has a very special logic that it may end up in the opposite
power state than the kernel driver thinks. Especially after boot it simply can not
know the state. The chip might be powered up/send records or might not.

A driver can only detect such a discrepancy if it thinks the GPS chip is powered off,
but there is still data coming through the UART.

Please tell me how this situation can be detected without monitoring the data stream
in the kernel going to /dev/ttyO1 - from the UART behind it - even if /dev/ttyO1 is closed.

This are *our* requirements.

Other people think that our approach helps to solve their driver architecture as well
and have added their requirements on top. This is why I attempt to make the API
more general than just for our own use-cases.

> 
> The 8686 is already working in serial mode  with no kernel hackery on
> other boards.

You appear to mix the chips we are talking about. We have the w2sg0084 gps
chip and a w2cbw003, which is a combo of an 8686 and a CSR BT serial device.

Both chips need somehow to be powered on or off if not used. Ideally automatically
at the moment no user space client is using them any more. For the bluetooth side
the moment to power off is when a hciattach is killed.

Other 8686 boards appear not to have such critical power restrictions and then they
just leave power on.

> 
>> For the GPS chip I am only interested in mctrl and if characters are received. But
>> I still want them to arrive at user space through a standard tty interface. So a solution
>> that does not interwork and cooperate with the tty layer is not a useable solution for
>> my requirements.
> 
> Why does it matter how they arrive so long as your user space can
> interpret it correctly. Or is your user space some proprietary blob you
> can't change ?

No, the opposite: it can be any open source application, which by principle
could be changed in any way. But we can't because we as the hardware
platform+kernel developers have no (and don't want to have) control over
the the user space. So we have to fit into the standards of the user space.

> 
> Them arriving by a tty interface is fine - no issue with that providing
> it doesn't need to mess up core code.

You appear to assume that the tty port is always open and there is
a background daemon running.

Here, we need to solve the problem to power down the chip if NO
tty port is open any more.

We simply don't see a solution for this outside the kernel and inside without
touching the UART code. Although there have been many very skilled
developers involved in the past 3 years to get what we need into the
mainline kernel. There had been proposals for approx. 3 or 4 different
architectures which were all rejected for good reasons and the current
one is the one which tries to overcome all known objections and of
course raises new questions.

> 
>> The reason appears to sit here:
>> 
>> http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L2725
>> 
>> This means that as soon as some UART is successfully probed, a new tty
>> interface is created for it. So we simply have to optionally disable it. This is
>> described by requirement 6.
> 
> You can just report EBUSY in your open method. You don't need to touch
> the serial core layer. It's quite sufficient to do
> 
> 	if (busy)
> 		return -EBUSY;
> 
> at the top of your uart open method.

Hm. I am not writing a new UART driver or touch them. I am using the existing
ones (omap-serial). They all call this uart_add_one_port() in their probe() function.

If I would modify just omap-serial, people would for sure complain that the solution
is not generic enough.

> 
>> UART drivers we have and work with any of them. This is why I think
>> serial-core and struct uart_port is the right level. Even if it looks like a hack.
> 
> Your "looks like" instinct is IMHO bang on - looks like a hack, is a
> hack.

It is not *my* instinct that says "looks like". It was:

1. meant to be read as "Even if it looks like a hack to the uninformed reader of the patch"
2. it was said by some reviewer

For me it is not a hack, it is a well thought logical consequence of the requirements
combined with the current code architecture as it exists (which often looks like a hack to me)
and the goal to make changes as non-intrusive as possible.

> It's also unnecessary as you can hide it in your open method if you
> must do that.

Creating tty ports is not *my* open method. It is part of the way all system
UARTs are initialized. So I can't handle this case in *my* open method, because
I have none that is tty_port related.

> 
>> So please wait until I have updated the patch set of our proposal. Then you
>> can see that we do not directly touch the tty layer.
> 
> The uart layer is part of the tty layer. It's just a glue library to make
> writing some tty drivers a bit easier.

Yes, and exactly that is helpful to solve this problem. We attach only to the
glue layer and everything is done.

> It's tied deeply to the tty_port
> implementation. One day it might even cease to exist replaced by more
> generic tty_port helpers.

Is there a concrete plan to change that? Is anyone working on it now?

If not, I would not worry about this, because it is not specific to the problem
we want to solve today (to be precise: since 3 years).

But even if it is changed, there must always be some glue between UARTs
and tty_port helpers. So it can't disappear - it can only change in structure..

> The tty layer is also an *abstract* concept. There is no real world tie
> between physical collections of shift registers that dribble bits to one
> another and tty devices in the kernel. You only need a tty driver for
> certain types of user interaction.

And we need it to process the GPS data by standard applications and tools.

BR,
Nikolaus