Date:	Sun, 2 Nov 2014 11:15:05 +0100
From:	"Dr. H. Nikolaus Schaller" <hns@...delico.com>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Marek Belisko <marek@...delico.com>,
	"arnd@...db.de" <arnd@...db.de>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	Pawel Moll <Pawel.Moll@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"galak@...eaurora.org" <galak@...eaurora.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	NeilBrown <neilb@...e.de>, linux-serial@...r.kernel.org
Subject: Re: [PATCH 2/2] Documentation: devicetree: Add bindings for Wi2Wi w2sg0004 gps

ping (questions for directions at the end of the mail).

Am 24.10.2014 um 11:32 schrieb Dr. H. Nikolaus Schaller <hns@...delico.com>:

> 
> Am 20.10.2014 um 19:26 schrieb Dr. H. Nikolaus Schaller <hns@...delico.com>:
> 
>> Hi,
>> 
>> Am 20.10.2014 um 11:35 schrieb Mark Rutland <mark.rutland@....com>:
>> 
>>> Hi,
>>> 
>>> On Fri, Oct 17, 2014 at 08:55:50PM +0100, Dr. H. Nikolaus Schaller wrote:
>>>> 
>>>> Am 17.10.2014 um 13:00 schrieb Mark Rutland <mark.rutland@....com>:
>>>> 
>>>>> On Fri, Oct 17, 2014 at 11:16:42AM +0100, Dr. H. Nikolaus Schaller wrote:
>>>>>> 
>>>>>> Am 17.10.2014 um 11:37 schrieb Mark Rutland <mark.rutland@....com>:
>>>>>> 
>>>>>>> On Thu, Oct 16, 2014 at 09:26:23PM +0100, Marek Belisko wrote:
>>>>>>>> Signed-off-by: H. Nikolaus Schaller <hns@...delico.com>
>>>>>>>> Signed-off-by: Marek Belisko <marek@...delico.com>
>>>>>>>> ---
>>>>>>>> .../devicetree/bindings/misc/wi2wi,w2sg0004.txt    | 44 ++++++++++++++++++++++
>>>>>>>> 1 file changed, 44 insertions(+)
>>>>>>>> create mode 100644 Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>>>> 
>>>>>>>> diff --git a/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..e144441
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/misc/wi2wi,w2sg0004.txt
>>>>>>>> @@ -0,0 +1,44 @@
>>>>>>>> +Wi2Wi GPS module connected through UART
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible: wi2wi,w2sg0004 or wi2wi,w2sg0084
>>>>>>>> +- pinctrl: specify two states (default and monitor). One is the default (UART) mode
>>>>>>>> +  and the other is for monitoring the RX line by an interrupt
>>>>>>>> +- on-off-gpio: the GPIO that controls the module's on-off toggle input
>>>>>>>> +
>>>>>>>> +Optional properties:
>>>>>>>> +- lna-suppy: an (optional) LNA regulator that is enabled together with the GPS receiver
>>>>>>>> +
>>>>>>>> +example:
>>>>>>>> +
>>>>>>>> +        gps_receiver: w2sg0004 {
>>>>>>>> +                compatible = "wi2wi,w2sg0004";
>>>>>>> 
>>>>>>> I couldn't spot "wi2wi" in
>>>>>>> Documentation/devicetree/bindings/vendor-prefixes.txt (in mainline).
>>>>>>> 
>>>>>>> Could you please add it?
>>>>>>> 
>>>>>>>> +                gpio-controller;
>>>>>>>> +                #gpio-cells = <2>;
>>>>>>> 
>>>>>>> As far as I can see, these properties aren't necessary. This only
>>>>>>> consumes a GPIO, it doesn't provide any.
>>>>>> 
>>>>>> Well, it provides one GPIO. Sort of a "virtual“ GPIO. It is needed so that 
>>>>>> it can be wired up to the DTR gpio of the UART driver (unfortunately this
>>>>>> patch was reverted some months ago from mainline and we will reintroduce
>>>>>> it soon).
>>>>> 
>>>>> If this GPIO doesn't really exist, then it's a Linux internal
>>>>> implementation detail rather than a description of the hardware, and
>>>>> doesn't really belong in the DT.
>>>> 
>>>> Hm. 
>>>> 
>>>> Let’s describe it differently.
>>>> 
>>>> I can see the Linux driver module as a simple software simulation for a
>>>> piece of hardware that could have been connected between the UART
>>>> and the GPS chip.
>>>> 
>>>> Basically it is a pulse-generator and a flip-flop to detect data flow on the RX
>>>> wire. This could be implemented by an external FPGA or uController. Or as
>>>> it is done on our board for saving hardware by a mix of the main CPU hardware
>>>> (Pinmux + GPIO + IRQ) and a kernel driver.
>>>> 
>>>> The best of course would have been if the w2sg0004 would have a physical
>>>> „enable“ GPIO (instead of that on-off control input).
>>>> 
>>>> Then we would hook up that enable to some physical GPIO of the CPU
>>>> and simply refer to it as e.g. <&gpio4 12>. And would not even need a driver
>>>> for it (unless we want to make rfkill gps work).
>>>> 
>>>> Therefore the driver we suggest provides an additional gpio controller with a
>>>> single GPIO so that we can write <&w2sg 0> to refer to this virtual gpio.
>>>> 
>>>> So in fact we try to wrap a non-optimal chip design by the driver and make it
>>>> appear as a standard GPIO interface to the DT and user space and whoever
>>>> needs simply to enable/disable the GPS chip.
>>> 
>>> The fact remains that this does not accurately represent the hardware,
>>> and is unnecessarily strongly tied to a particular UART design (where
>>> the DTR line is a separate UART).
>> 
>> I don’t get that it is tied to a particular UART design (except that our DTR
>> patch affects to omap-serial only).
>> 
>> Let’s describe the facts:
>> 
>> The chip has this interface:
>> 
>> GPS-TX (output)
>> GPS-RX (input)
>> ON/OFF (input) - to toggle the power state of the chip
>> 
>> They are connected to an OMAP3 UART2 as
>> 
>> UART2-TX > GPS-RX
>> UART2-RX <- GPS-TX
>> GPIO145 -> ON/OFF
>> 
>> That’s it.
>> 
>> If the chip (or any other serial GPS receiver like a Garmin) were connected
>> through real RS232 we would have
>> 
>> UART2-TX -> level shifter -> cable -> level shifter -> GPS-RX
>> UART2-RX <- level shifter <- cable <- level shifter <- GPS-TX
>> DTR-GPIO -> level shifter (DTR line) -> cable -> level shifter -> power-management logic -> ON/OFF
>> 
>> But because it is connected directly, we need to implement the power-management
>> logic between the DTR-GPIO and GPIO145:
>> 
>> DTR-GPIO -> driver -> GPIO145 -> ON/OFF
>> 
>> To correctly determine the state we need to tap the RX signal before
>> it enters the UART2-RX (it is pinmuxed with GPIO147):
>> 
>> UART2-RX <——+
>>                              OMAP3 pinmux <- OMAP3 pad <- GPS-TX
>>  GPIO147 <——+
>> 
>> So if we describe the driver as a box we have
>> 
>> inputs from user space:
>> * rfkill for GPS
>> * a control that is activated by opening /dev/tty
>> 
>> outputs to system:
>> * a control to switch the pinmux between RX and GPIO (interrupt)
>> * a control to external hardware
>> 
>>> 
>>>>> It sounds like what we actually need is the ability to describe devices
>>>>> attached to UARTs.
>>>> 
>>>> Hm. The purpose of the driver is power control of the chip. Not the serial
>>>> interface.
>>> 
>>> I'm not sure I follow your point. By describing the device as attached
>>> to the UART, the kernel can figure out that when said UART is accessed
>>> the attached device needs to be on (and must be poked as necessary).
>> 
>> Why do we need to describe this? It is all about controlling the GPIO145,
>> GPIO147 and pinmux. But not RX or TX.
>> 
>> So the driver does not need to know anything about UARTs (and if we
>> connect it to UART3 we only have to specify different GPIOs).
>> 
>> Only our board specific dtb connects the UART to the “virtual” GPIO
>> provided by the driver.
>> 
>> This also brings up a minor problem: on OMAP3 some UART RX lines can
>> be pinmuxed with different GPIOs. So putting the driver under some UART2
>> node doesn’t uniquely define the GPIO number to monitor RX and might
>> become a mess.
>> 
>>> 
>>> The power management logic for the device can stay in the device driver,
>>> and the power management logic for the UART can stay in the UART driver.
>> 
>> Here I can’t follow you. What power management does an UART driver provide?
>> 
>>> Neither would need to know about each other's internal details
>>> (e.g.GPIOs, for DTR or otherwise).
>> 
>> Yes, that is our goal and in our solution they don’t need to make assumptions
>> about each other. We just define in the DT: this GTA04 board has UART2-DTR
>> connected to the W2SG-GPIO# - instead of OMAP3-GPIO145.
>> 
>> In our solution the UART driver knows that there could be a DTR GPIO (similar to
>> a RTS GPIO which is already provided by the omap-serial RS484 bindings). Which
>> could be connected to some GPIO which drives the real DTR wire of a RS232 level
>> shifter.
>> 
>>> 
>>>>> Then you could have a mechanism whereby the UART
>>>>> driver can notify the other device driver regarding events (e.g. the
>>>>> UART being opened for access), or the other driver could claim ownership
>>>>> of the UART and expose its own interface to userspace.
>>>>> 
>>>>> That would be independent of the particular UART or other device, and
>>>>> the only description necessary in the DT would be an accurate
>>>>> representation of the way the hardware is wired.
>>>>> 
>>>>> There are a few ways that could be done, but I suspect the simplest is
>>>>> to just have the device as a sub-node of the UART, like we do for SPI or
>>>>> I2C buses:
>>>>> 
>>>>> 	serial@f00 {
>>>>> 		compatible = "vendor,uart";
>>>>> 		reg = <0xf00 0x100>;
>>>>> 		...
>>>>> 
>>>>> 		gps {
>>>>> 			compatible = "wi2wi,w2sg0004";
>>>>> 			...
>>>>> 		};
>>>>> 	};
>>>>> 
>>>>> That wouldn't work for devices with multiple UART connections. Do those
>>>>> exist, and are they common in configurations where out-of-band
>>>>> management is necessary (e.g. regulators, clocks)?
>>>> 
>>>> UARTs are usually point to point interfaces and not busses. So there is
>>>> no need to describe the interface.
>>> 
>>> I don't follow. You have a device which seems to require management
>>> kernel-side.
>> 
>> Yes, power on/off.
>> 
>>> Rather than describing the interface, you've described a
>>> fictitious relationship between the GPS device and the UART's DTR GPIO,
>>> and you’ve described a fictitious GPIO to hand to the UART driver.
>> 
>> Yes. Because the UART driver would generally expect a GPIO (if a GPIO driven
>> DTR hardware line is available on the connector).
>> 
>>> This
>>> is how you have linked the two in order to get the behaviour you want.
>> 
>> Exactly.
>> 
>>> 
>>> So it _is_ necessary to describe the interface. Rather than describing
>>> that interface at a high level you've chosen to hack together a set of
>>> fake relationships to work around the lack of ability to describe said
>>> interface.
>> 
>> The power control interface is a single GPIO from UART to the driver.
>> The other end is an interface from the driver to the pinmux of the OMAP
>> and the chip. So it is a “middleman”. But I think this is already clear.
>> 
>>> 
>>>> And I would speculate that in most cases they simply go to some
>>>> connector and therefore no connected chip that needs to be described
>>>> in the DT at all. Because it has a user-space driver (e.g. AT
>>>> commands) and no kernel driver.
>>> 
>>> In the case where no driver is necessary I agree that no description is
>>> necessary, though the description could be exposed in a helpful way to
>>> userspace to describe what’s attached to which UARTs.
>> 
>> That is usually done by configuring the gpsd process.
>> 
>>> 
>>> However, in this case you do have a kernel driver (even if basic), and
>>> it requires some knowledge of the relationship between the device and
>>> the UART in order to function.
>> 
>> Yes. And that is what we want to provide by the dtb file. That one should describe
>> the relationship.
>> 
>>> 
>>>> But we have no idea how such a solution could be implemented or tested.
>>> 
>>> I would disagree on that point, given I provided a high level
>>> description of how this could be implemented.
>> 
>> We do understand how you suggest the bindings should look like, but we have
>> no idea how that translates into code.
>> 
>> And we do not want to touch the general serial drivers, just the omap-serial
>> (because the solutions does not work without an UART behind a pinmux with a
>> GPIO with interrupt capabilities).
>> 
>> We simply have no experience modifying serial drivers (Linux is too big that anyone
>> can know all areas).
>> 
>> Our experience is limited to re-submitting the DTR-GPIO-control patch by Neil Brown
>> and making it read DT properties instead of board file.
>> 
>> So your approach needs a lot of help to really implement it. The question is who
>> will be doing it.
>> 
>>> 
>>>> If someone adds that to the serial drivers, we would be happy to use it,
>>>> but unless such a thing exists, I think our solution is quite simple and isolated
>>>> into this single driver and also uses existing standard interfaces (gpios, pinmux).
>>> 
>>> I would argue that this _abuses_ standard interfaces, as you have one
>>> device driver fiddling with resources logically owned by another.
>>> 
>>>>>> The reason to solve it that way is that we did not want to have a direct link
>>>>>> between this driver and any serial drivers or other mechanisms how drivers
>>>>>> can detect that their serial port (/dev/tty*) is opened.
>>>>>> 
>>>>>> It is used to power down the w2sg GPS chip if no user space process is
>>>>>> accessing its serial port (or de-asserts DTR through tcsetattr/ioctl).
>>>>>> 
>>>>>>> 
>>>>>>>> +
>>>>>>>> +                pinctrl-names = "default", "monitor";
>>>>>>>> +                pinctrl-0 = <&uart2_pins>;
>>>>>>>> +                pinctrl-1 = <&uart2_rx_irq_pins>;
>>>>>>>> +
>>>>>>>> +                interrupt-parent = <&gpio5>;
>>>>>>>> +                interrupts = <19 IRQ_TYPE_EDGE_FALLING>;  /* GPIO_147: RX - trigger on arrival of start bit */
>>>>>>> 
>>>>>>> While interrupts is a standard property, please describe above how many
>>>>>>> you expect and what their logical function is.
>>>>>>> 
>>>>>>> The only part I'm confused about is how the link to the UART is
>>>>>>> described. I assume I'm just ignorant of some existing pattern.
>>>>>> 
>>>>>> The serial link itself is not described at all because it is assumed to be a „must have“.
>>>>> 
>>>>> Huh? So it's a "must have" that you "don't have" in the DT?
>>>> 
>>>> Yes. The DT does not describe everything. Only those things that need
>>>> a kernel driver. And UARTs usually have user-space drivers (e.g. gpsd,
>>>> gsmd, pppd) and ioctl/tcsetattr.
>>> 
>>> The DT should describe the static portions of the hardware. Typically we
>>> only have devices with kernelspace drivers described simply because
>>> that's the way people have built DTs. Whether or not you have a
>>> kernelspace driver can change over time, the organisation of the
>>> hardware cannot.
>> 
>> So far none of the DT I have seen describe what is connected to the UART.
>> Even for boards which use HCI to communicate with a Bluetooth module.
>> 
>>> 
>>>>> I think that the relationship is being described incorrectly in the DT,
>>>>> and I think that there is a more general problem that needs to be
>>>>> addressed in order to make this case work.
>>>>> 
>>>>>> The driver only needs to monitor the RX line and needs to switch it between UART and
>>>>>> GPIO/IRQ mode. So this monitoring switch is described (with two different pinctrl states).
>>>>> 
>>>>> While this particular driver only needs that at this point in time,
>>>>> that's not necessarily true of drivers for similar devices, nor is it
>>>>> necessarily true if we need to add additional features to this driver.
>>>> 
>>>> Which features are you thinking of to add to this driver? And do
>>>> similar devices exist at all? Since we have not found any, we have
>>>> declared it as a "misc“ driver.
>>> 
>>> I don't have any particular feature in mind.
>>> 
>>> I am not immediately aware of other serial devices which require
>>> out-of-band management in the same way, though we have a vaguely similar
>>> case with SDIO devices which must be powered up before they appear on
>>> the bus.
>> 
>> AFAIK, they are usually descibed by a regulator that is enabled/disabled by
>> the SDIO driver. And the regulator is usually defined as a gpio-regulator to
>> drive an GPIO.
>> 
>> And I think the SDIO driver has a reset-gpio (which can also be interpreted
>> as a disable/power-down).
>> 
>> So such interface drivers simply expect that they can power-control dependent
>> devices through a regulator or a simple gpio.
>> 
>>> In that case I believe the intent is to describe them in the DT
>>> under the bus.
>> 
>> Or would it be ok to allow a regulator property for the serial interface? Then
>> our driver would become a w2sg0004-regulator driver similar to a gpio-regulator
>> but with a state machine that knows how to pulse the gpio until power is applied
>> to the GPS receiver internals.
>> 
>>> 
>>>>> Describing the relationship leaves a lot more freedom to improve things
>>>>> without having to update every DTB.
>>>>> 
>>>>>> We know that it is a little tricky to control this chip correctly - and we think this solution
>>>>>> is the most general (no direct dependency on the serial line, and just to pinmux states
>>>>>> and an interrupt).
>>>>> 
>>>>> I think that the rough approach I sketched out above is more general,
>>>>> and I think that you must describe the relationship with the serial
>>>>> line.
>>>>> 
>>>>> It's not clear to me whether the interrupt you describe is attached to
>>>>> the GPS, or if this is logically part of the UART.
>>>> 
>>>> The interrupt is needed to simulate the glue logic connected between
>>>> UART and GPS.
>>>> 
>>>> The output signal comes from the GPS module and goes to some pad
>>>> of the OMAP3 SoC. This pad can be either multiplexed into the UART RX
>>>> input or into a GPIO bank of the OMAP. That GPIO controller can generate
>>>> the interrupt on incoming data (when none is expected).
>>>> 
>>>> Therefore it is a GPS-generated interrupt and has nothing to do with
>>>> the UART.
>>> 
>>> Ok. When does the GPS device raise this interrupt?
>> 
>> If the driver assumes the GPS receiver is turned off, it disconnedts the RX
>> wire from the UART to a GPIO by using two different pinmux states.
>> 
>> If the GPIO raises an interrupt, the driver knows that the GPS module is not
>> turned off, because the driver has lost knowledge about its state. This is not
>> an UART related function but OMAP pinmux and GPIO.
>> 
>> I am not sure if that could even be implemented at all by a UART dependent
>> driver (since the UART is shut down and does not interrupt).
>> 
>>> 
>>> Thanks,
>>> Mark.
>> 
>> Thanks as well - it is a fruitful discussion (even if it becomes lengthy because
>> I might have repeated a lot of things that are already clear. Please accept an apology).
>> 
>> I think we just disagree about implementing a version that “works” in a quite specific
>> setup (there are necessary assumptions about OMAP pinmux and omap-serial) with
>> existing APIs versus a general one that might need a lot of changes outside the driver
>> and introduce new APIs.
>> 
>> BR,
>> Nikolaus
>> 
> 
> After re-thinking what we have discussed so far (and considering other recommendations
> I have received off-list) I currently see these options/questions:
> 
> * who could modify the serial drivers and APIs so that this driver can become
>  a subnode (“bus client”) of an UART? How long does it take to become available?
> 
> * would it be an option to rename it to “gpio-w2sg0004” to better describe that it
>  provides a (virtual) gpio?
> 
> * would it be an option to simply rename the driver to “w2sg0004-power”
>  to make clear that it is not at all related to the UART data communication
>  channel but only controlling the power of the w2sg0004 chip?
> 
> * both?
> 
> * or would it be acceptable if this is a regulator driver that controls the LDO
>  inside this chip? I.e. describe it as a “w2sg0004-regulator”?
> 
> This would be very similar to the function of a “gpio-regulator”: convert
> “regulator state” into a gpio state. But here convert “on/off” into some impulses.
> 
> And to resolve uncertainty about the real LDO state it would be able to monitor
> some feedback interrupt and handle an (optional) pinmux toggle.
> 
> This would mean that we do not even need to mention anything about UARTs
> in the driver bindings.
> 
> Rather it would just say: the driver can monitor a (gpio) interrupt line to
> know if the attached device is active (when it is not expected, e.g. after boot).
> 
> Since this monitor gpio is sometimes multiplexed with other features of the SoC,
> the driver can also switch between two pinctrl states (default and monitor).
> 
> BR,
> Nikolaus
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists