Date:	Thu, 7 May 2015 14:46:15 +0200
From:	"Dr. H. Nikolaus Schaller" <hns@...delico.com>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Peter Hurley <peter@...leysoftware.com>,
	Pavel Machek <pavel@....cz>,
	List for communicating with real GTA04 owners 
	<gta04-owner@...delico.com>, NeilBrown <neil@...wn.name>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Sebastian Reichel <sre@...nel.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	Jiri Slaby <jslaby@...e.cz>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [Gta04-owner] [PATCH 0/3] tty slave device support - version 3.

Hi Mark,

Am 06.05.2015 um 19:18 schrieb Mark Rutland <mark.rutland@....com>:

> On Wed, May 06, 2015 at 05:09:20PM +0100, Dr. H. Nikolaus Schaller wrote:
>> Hi Mark,
>> 
>> Am 06.05.2015 um 16:15 schrieb Mark Rutland <mark.rutland@....com>:
>> 
>>>>>>>> No, I am not playing devil’s advocate (which would imply that I am doing this
>>>>>>>> for fun to tease the dog), but I feel I have to be the advocate of future board
>>>>>>>> designers who want to easily import an existing board DT and overwrite device
>>>>>>>> tree nodes to describe design changes, i.e. what slave device is connected to
>>>>>>>> which uart.
>>> 
>>> [...]
>>> 
>>>>> If this happens, you can move the slave device into a fragment that you
>>>>> can include under the correct node. That's trivial.
>>>> 
>>>> But less readable. And that is important as well.
>>> 
>>> I disagree. The manipulation you have to perform to override properties
>>> is at least as bad as including a file.
>> 
>> What about:
>> 
>> #include "omap3-beagle-xm.dts"
>> 
>> / {
>> 	/* HS USB Port 2 Power enable was inverted with the xM C */
>> 	hsusb2_power: hsusb2_power_reg {
>> 		enable-active-high;
>> 	};
>> };
>> 
>> compared to 
>> 
>> #include “board1.dts”
>> 
>> / {
>> 	/* slave was reconnected to uart4 */
>> 	slave {
>> 		uart = <&uart4>;
>> 	};
>> };
> 
> As I mentioned, you can easily carve up your DTS to make that work with
> includes if you really must:
> 
> /* UART0 board variant */
> #include "board.dtsi"
> &uart0 {
> 	#include "some-uart-slave.dtsi"
> };
> 
> /* UART1 board variant */
> #include "board.dtsi"
> &uart1 {
> 	#include "some-uart-slave.dtsi"
> };
> 
> If you happen to find includes ugly then you can say it's ugly, but it's
> functionally equivalent, and also means you can avoid having
> disabled/partial nodes all over the place.

Functionally equivalent would also be to copy the whole source file and
s/&uart0/&uart1/.

But this is not the best solution for the DT programmer since there is no
automatic *reuse* of common parts.

And your proposal requires 3 source files instead of 2 which deteriorates
readibility and understanding what is really going on. And if you need to
change the some-uart-slave, you have to touch a different file than for
changing some other slave.

Yes, it works, but IMHO other factors for a good design are also important.

Maybe our main difference in PoV is that I specifically want to avoid that
we force future DT programmers into “ugly” solutions (even if they work).

If you think that DT programmers have to live with what they are
given and do the best with it, we can end the discussion.

> 
> If you really wanted you could macro the label instead and have the
> slaved node in full.

Hm. I have tried to find a single example where a DT source file has an
#include *not* on top level. Or a macro that defines multiple properties
or a complete subnode.

Maybe it is a commonly used DT design pattern and I just didn’t find it
in the device trees I have checked.

> 
> [...]
> 
>>> As Neil mentioned earlier, ignore "bus". Here we're caring about a 1-1
>>> connection between master (UART) and slave (device), and it happens that
>>> in most other cases the master is actually a bus, so that's how things
>>> happen to be named.
>> 
>> So you also mean that all master-slave connections must be represented
>> by DT subnodes and everything else is not acceptable?
>> 
>> What about:
>> 
>> 	sound {
>> 		compatible = "ti,omap-twl4030";
>> 		ti,model = "omap3beagle";
>> 
>> 		ti,mcbsp = <&mcbsp2>;
>> 	};
>> 
>> Isn’t McBSP a “bus” with your definition as well? Like a “master”. And the “twl4030”
>> is the slave (codec)?
> 
> I'm nowhere near familiar enough with the audio hardware nor their
> bindings to comment on that, I'm afraid. My rough understanding was that
> the twl4030 node here was referring to the logical subsystem as a whole,
> with the mcbsp being a physical component, but that's almost certainly
> somewhat wrong.

The McBSP is a 4-wire serial interface controller sitting on the OMAP chip.
It is accessed by MMIO and DMA. The twl4030 is the other end and the data
that is transferred is PCM audio to/from speakers/microphone.

So this node defines: there is a sound slave that is a twl4030 and the PCM interface
is the mcbsp2 of the OMAP SoC. Exactly the same pattern as I want to describe
a GPS slave that is connected to uart1 of the OMAP SoC.

> 
>> &usb_otg_hs {
>> 	interface-type = <0>;
>> 	usb-phy = <&usb2_phy>;
>> 	phys = <&usb2_phy>;
>> 	phy-names = "usb2-phy";
>> 	mode = <3>;
>> 	power = <50>;
>> };
>> 
>> Isn’t a PHY interface (e.g. ULPI-PHY) a “slave” connected to a 12 wire ULPI “bus” as well?
> 
> Not necessarily. If you took a look at the bindings you'd notice that
> many PHYs have MMIO interfaces for configuration which we have to poke.
> Those MMIO interfaces live on an MMIO bus so we describe those and link
> to the phy by phandle reference.

Yes, that is the reason why it can only be solved by a phandle, because the “main interface”
of the PHY chip is sometimes the MMIO bus, but could also be I2C or SPI.

In the case of osb_otg_hs I have copied, it is not. The PHY is *only* connected through
the ULPI wires and all register programming goes through that interface. It is not connected to
the MMIO bus at all.

> 
> A given device could operate as a PHY to multiple controllers, hence
> phy-cells, and hence in general a PHY cannot be considered a slave
> device.
> 
>> At least in this two areas everything done so far appears to contradict your argument.
> 
> Not really. You've found bindings with a different idiom, but those seem
> to be organised as they are for reasons which don't appear to apply to
> UART slave devices as you describe them.

In my view I2C and SPI also follow a different idiom (addressable, multiple slaves)
which does not appear to apply for an UART slave device.

> 
>> This is for me the blueprint how it should be done for UART slaves like any point-to-point
>> multi-wire interfaces (question not even discussed: does UART-to-UART have clear master
>> and slave roles? Isn’t the connected device the “master”? but I don’t want to broaden the
>> discussion again).
>> 
>> This is basically why I keep this discussion open, because it is not that obvious
>> that Neil’s proposal is right and mine is wrong.
>> 
>>> 
>>>>> for other
>>>>> interfaces like SPI and I2C. I do not see a compelling reason to do
>>>>> otherwise for devices
> 
> [...]
> 
>>>>> hanging off of UARTs -- doing so would make things
>>>>> less straightforward because you have a fundamentally different idiom.
>>>> 
>>>> Yes, my proposal is fundamentally different from I2C and SPI practice, but
>>>> it is the same that is heavily used for e.g. GPIOs and Regulators.
>>> 
>>> Well, those cases are somewhat distinct, and I'd say that UART slaves
>>> are much closer to SPI/I2C devices than GPIOs or regulators.
>> 
>> As shown above they are IMHO closer to McBSP and ULPI-PHY (and some
>> other interfaces).
> 
> As above, I disagree.
> 
>>> Let's say I have a GPIO described via a phandle. That GPIO is actually
>>> owned by some GPIO controller whose control interface is sat on an MMIO
>>> bus. What we're describing with the phandle is use of the GPIO, but not
>>> the main interface for interactive with the GPIO, which is the MMIO
>>> interface of the controller.
>> 
>> Right. For the UART we do the same: the UART controller is connected
>> to the MMIO and (in my proposal) the phandle describes the use of the UART
>> (to connect to some slave device). Exactly the same situation.
> 
> Except that your fundamental interface to the device is via the UART,
> which is not typically the case for things like regulators and/or GPIOs.
> If I want a regulator to do something, I ask it to do so via the
> controller's MMIO interface. If you want the device to do something, you
> ask it to do so via the UART. 

I also ask the UART controller through the MMIO to send bytes to the device.
For a GPIO it is just a single bit that is “sent” or “received”.

And, I do not ask the device to do something through UART. I exchange
data. And only if the specific device has it I use some escape mechanisms
(like AT commands) to do control. If a vt52 is connected I only send characters
and receive keyboard codes.

So I don’t think that being able to control the slave is a common property
of all UART-slave connections.

BTW: the GPS chip we use is doing something without any instructions. It starts
to send NMEA records right after power on. It would even be useful with a unidirectional
connection from GPS to UART. Hence there are no commands that need to be
sent through UART. Rather it just needs some GPIO to be activated or powered down
when /dev/tty is opened/closed.

So one could argue that in this real case (which we try to solve in an acceptable way
since 2 years) the fundamental/main interface (control command interface) of this chip
is the power on/off line. And UART isn’t involved at all. Or just as an “activity detector”.

But that is on the lowest detail level.

> 
>>> In the case of UART slave devices the control interface is attached to
>>> the UART, and effectively the slave sits on the UART's "bus". We could
>>> refer to it from elsewhere by phandle, but its canonical parent should
>>> be the UART, as that’s what its main interface is attached to.
>> 
>> What is the “main interface” of some device? Why should it have a special
>> role in DT? I have doubts that it is useful to declare some interface as “main”,
>> unless it is a MMIO bus connection.
>> 
>> There are e.g. chips with multi-interfaces like a twl4030. They have 2 I2C busses, 2 PCM
>> “busses”, an ULIP-PHY and some GPIOs.
> 
> I think that would already be covered as an I2C device with two I2C IDs,
> much like we would cater for an MMIO-interfaced PHY with two MMIO
> control register regions

Yes, as long as multiple interfaces fall into the same category.

> 
>> Or some 3G/4G modems which have
>> USB, UART (both useable for AT commands in parallel!), PCM and some GPIOs.
> 
> Of these I would expect that the USB or UART inerfaces would be the main
> ones,

Both are equally valid to be the “main" interface (because you can use both to send
AT commands).

> though I would expect that we'd require two separate nodes which
> we would have to link up.

And how would you do that without phandles?

Fortunately this is an easy to solve problem since USB has enumeration and the
USB interface does not need to be described in DT.

> 
>> Which interface is “main”? For the twl4030 it happens that one of the I2C interfaces
>> is chosen (because it allows to access the registers inside the chip).
> 
> Because it happens to be the “main” interface ;)

It depends on the angle of viewing onto the chip components, to see it as the main interface.

If you look from “how can the kernel access registers”, it is. If you look from a
sound (PCM payload) perspective the PCM interface would be “main”.

But I agree that a “main interface” should be defined as the interface that allows
the most broad control the chip. I.e. it should not be defined by “biggest payload
bandwidth” or similar.

> 
>> Now you might say: yes, the registers of some uart connected device can
>> be accessed through the uart as well. But usually there is a higher level
>> protocol (AT commands) that give some sort of “register addressing”. But
>> that is a different level than using I2C to access e.g. the gpio controller in the twl4030.
>> 
>> I just want to say that requiring and focussing on a “main” interface of a peripheral
>> chip may lead to troubles.
> 
> I don't disagree that it falls apart in some cases. However, that's not
> the general case, and it applies to other interface types too, so I
> don't think it should matter in the context of this discussions.
> 
>>>> From my DT designer PoV I would say the UART exists in some SoC
>>>> (independently of a device being connected) and then, a device is connected
>>>> to the UART. Hence the proposal of adding this connection link to the device’s
>>>> node and not making the device a subnode. And having the device driver do
>>>> power management and only ask the uart/tty driver to be notified about open()
>>>> and close() events. How power is managed in detail is then not any part of the
>>>> tty/serial drivers.
>>> 
>>> The way that the power management interfaces are organised within Linux
>>> is orthogonal to the way we describe things in the DT.
>> 
>> Agreed. And therefore power management registration, notifications etc.
>> are to be hidden by the drivers and should not be an argument to say “with
>> subnodes it is easier to implement and probing is done in the right order”.
> 
> While generally we shouldn't treat OS internals as an argument for DT
> organisation, laying things out in a sensible manner for
> discoverabilitiy is somewhat different from stating that the run-time
> use of a device is fundamentally tied to its description in the DT.
> 
> We _could_ list all nodes in a flat list with cross references instead
> of having a tree. But it would be horrible for _any_ OS to deal with, so
> we don’t do that.

Agreed.

“Discoverability” is something I have not yet thought about. For I2C it is
important to tell the I2C controller at which address to look for devices
(so that it does not have to scan all of them). This is most easily achieved
by making the (potential) slaves subnodes of the i2c bus controller and
nicely fits with the <reg> property subnodes usually have.

UART slaves are static. Or simply respond/don’t respond. So there is
no well defined discovery mechanism involved and therefore the slave
does not require to be a subnode of the uart to support descovery.

BTW: there is a solution to the probe sequence issue:
1. each UART registers itself in some uart map (phandle -> driver instance)
    as soon as probed successfully
2. a slave driver checks if it can find its attached uart in the list
3. if not -> -EPROBE_DEFER
4. if yes, it can register with the uart driver to receive tty open/close notifications


But mainly we do not agree on priority and importance of criteria when evaluating
the two solutions that are on the table.

We can continue with examples and counter examples to identify the implications
(for Driver developers and for DT developers) of choosing either one.

It is a very complex problem without a single criterion that makes one solution
better than the other. Hence I think the discussion is necessary.

So should we continue or does someone decide?

BR and thanks,
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