Message-ID: <20160708074326.GR14675@voom.fritz.box>
Date:	Fri, 8 Jul 2016 17:43:26 +1000
From:	David Gibson <david@...son.dropbear.id.au>
To:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
Cc:	frowand.list@...il.com, robh+dt@...nel.org,
	stephen.boyd@...aro.org, broonie@...nel.org,
	Grant Likely <grant.likely@...retlab.ca>, mark.rutland@....com,
	Matt Porter <mporter@...sulko.com>, koen@...inion.thruhere.net,
	linux@...ck-us.net, marex@...x.de, wsa@...-dreams.de,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-i2c@...r.kernel.org
Subject: Re: [RFC PATCH 0/1] Portable Device Tree Connector -- conceptual

On Fri, Jul 08, 2016 at 10:26:15AM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> > On Jul 7, 2016, at 10:15 , David Gibson <david@...son.dropbear.id.au> wrote:
> > 
> > On Sat, Jul 02, 2016 at 04:55:49PM -0700, frowand.list@...il.com wrote:
> >> From: Frank Rowand <frank.rowand@...sony.com>
> >> 
> >> Hi All,
> >> 
> >> This is version 2 of this email.
> >> 
> >> Changes from version 1:
> >> 
> >>  - some rewording of the text
> >>  - removed new (theoretical) dtc directive "/connector/"
> >>  - added compatibility between mother board and daughter board
> >>  - added info on applying a single .dtbo to different connectors
> >>  - attached an RFC patch showing the required kernel changes
> >>  - changes to mother board .dts connector node:
> >>     - removed target_path property
> >>     - added connector-socket property
> >>  - changes to daughter board .dts connector node:
> >>     - added connector-plug property
> >> 
> >> 
> >> I've been trying to wrap my head around what Pantelis and Rob have written
> >> on the subject of a device tree representation of a connector for a
> >> daughter board to connect to (eg a cape or a shield) and the representation
> >> of the daughter board.  (Or any other physically pluggable object.)
> >> 
> >> After trying to make sense of what had been written (or presented via slides
> >> at a conference - thanks Pantelis!), I decided to go back to first principals
> >> of what we are trying to accomplish.  I came up with some really simple bogus
> >> examples to try to explain what my thought process is.
> >> 
> >> This is an extremely simple example to illustrate the concepts.  It is not
> >> meant to represent the complexity of a real board.
> >> 
> >> To start with, assume that the device that will eventually be on a daughter
> >> board is first soldered onto the mother board.  The mother board contains
> >> two devices connected via bus spi_1.  One device is described in the .dts
> >> file, the other is described in an included .dtsi file.
> >> Then the device tree files will look like:
> >> 
> >> $ cat board.dts
> >> /dts-v1/;
> >> 
> >> / {
> >>        #address-cells = < 1 >;
> >>        #size-cells = < 1 >;
> >> 
> >>        tree_1: soc@0 {
> >>                reg = <0x0 0x0>;
> >> 
> >>                spi_1: spi1 {
> >>                };
> >>        };
> >> 
> >> };
> >> 
> >> &spi_1 {
> >>        ethernet-switch@0 {
> >>                compatible = "micrel,ks8995m";
> >>        };
> >> };
> >> 
> >> #include "spi_codec.dtsi"
> >> 
> >> 
> >> $ cat spi_codec.dtsi
> >> &spi_1 {
> >> 	codec@1 {
> >> 		compatible = "ti,tlv320aic26";
> >> 	};
> >> };
> >> 
> >> 
> >> #----- codec chip on cape
> >> 
> >> Then suppose I move the codec chip to a cape.  Then I will have the same
> >> exact .dts and .dtsi and everything still works.
> >> 
> >> 
> >> @----- codec chip on cape, overlay
> >> 
> >> If I want to use overlays, I only have to add the version and "/plugin/",
> >> then use the '-@' flag for dtc (both for the previous board.dts and
> >> this spi_codec_overlay.dts):
> >> 
> >> $ cat spi_codec_overlay.dts
> >> /dts-v1/;
> >> 
> >> /plugin/;
> >> 
> >> &spi_1 {
> >> 	codec@1 {
> >> 		compatible = "ti,tlv320aic26";
> >> 	};
> >> };
> >> 
> >> 
> >> Pantelis pointed out that the syntax has changed to be:
> >>   /dts-v1/ /plugin/;
> >> 
> >> 
> >> #----- codec chip on cape, overlay, connector
> >> 
> >> Now we move into the realm of connectors.  My mental model of what the
> >> hardware and driver look like has not changed.  The only thing that has
> >> changed is that I want to be able to specify that the connector that
> >> the cape is plugged into has some pins that are the spi bus /soc/spi1.
> >> 
> >> The following _almost_ but not quite gets me what I want.  Note that
> >> the only thing the connector node does is provide some kind of
> >> pointer or reference to what node(s) are physically routed through
> >> the connector.  The connector node does not need to describe the pins;
> >> it only has to point to the node that describes the pins.
> >> 
> >> This example will turn out to be not sufficient.  It is a stepping
> >> stone in building my mental model.
> >> 
> >> $ cat board_with_connector.dts
> >> /dts-v1/;
> >> 
> >> / {
> >> 	#address-cells = < 1 >;
> >> 	#size-cells = < 1 >;
> >> 
> >> 	tree_1: soc@0 {
> >> 		reg = <0x0 0x0>;
> >> 
> >> 		spi_1: spi1 {
> >> 		};
> >> 	};
> >> 
> >> 	connector_1: connector_1 {
> >> 		spi1 {
> >> 			target_phandle = <&spi_1>;
> >> 		};
> >> 	};
> >> 
> >> };
> >> 
> >> &spi_1 {
> >> 	ethernet-switch@0 {
> >> 		compatible = "micrel,ks8995m";
> >> 	};
> >> };
> >> 
> >> 
> >> $ cat spi_codec_overlay_with_connector.dts
> >> /dts-v1/;
> >> 
> >> /plugin/;
> >> 
> >> &connector_1 {
> >> 	spi1 {
> >> 		codec@1 {
> >> 			compatible = "ti,tlv320aic26";
> >> 		};
> >> 	};
> >> };
> >> 
> >> 
> >> The result is that the overlay fixup for spi1 on the cape will
> >> relocate the spi1 node to /connector_1 in the host tree, so
> >> this does not solve the connector linkage yet:
> >> 
> >> -- chunk from the decompiled board_with_connector.dtb:
> >> 
> >> 	__symbols__ {
> >> 		connector_1 = "/connector_1";
> >> 	};
> >> 
> >> -- chunk from the decompiled spi_codec_overlay_with_connector.dtb:
> >> 
> >> 	fragment@0 {
> >> 		target = <0xffffffff>;
> >> 		__overlay__ {
> >> 			spi1 {
> >> 				codec@1 {
> >> 					compatible = "ti,tlv320aic26";
> >> 				};
> >> 			};
> >> 		};
> >> 	};
> >> 	__fixups__ {
> >> 		connector_1 = "/fragment@0:target:0";
> >> 	};
> >> 
> >> 
> >> After applying the overlay, the codec@1 node will be at
> >> /connector_1/spi1/codec@1.  What I want is for that node
> >> to be at /spi1/codec@1.
> >> 
> >> 
> >> 
> >> #----- magic new syntax
> >> 
> >> What I really want is some way to tell dtc that I want to do one
> >> level of dereferencing when resolving the path of device nodes
> >> contained by the connector node in the overlay dts.
> >> 
> >> Version 1 of this email suggested using dtc magic to do this extra
> >> level of dereferencing.  This version of the email has changed to
> >> have the kernel code that applies the overlay do the extra level
> >> of dereferencing.
> >> 
> >> The property "connector-socket" tells the kernel overlay code
> >> that this is a socket.  The overlay code does not actually
> >> do anything special as a result of this property; it is simply
> >> used as a sanity check that this node really is a socket.  The
> >> person writing the mother board .dts must provide the
> >> target_phandle property, which points to a node responsible for
> >> some of the pins on the connector.
> >> 
> >> The property "connector-plug" tells the kernel overlay code
> >> that each child node in the overlay corresponds to a node in the
> >> socket, and the socket will contain one property that is
> >> a phandle pointing to the node that is the target of that child
> >> node in the overlay node.
> >> 
> >> 
> >> $ cat board_with_connector_v2.dts
> >> 
> >> /dts-v1/;
> >> 
> >> / {
> >> 	#address-cells = < 1 >;
> >> 	#size-cells = < 1 >;
> >> 
> >> 	tree_1: soc@0 {
> >> 		reg = <0x0 0x0>;
> >> 
> >> 		spi_1: spi1 {
> >> 		};
> >> 	};
> >> 
> >> 	connector_1: connector_1 {
> >> 		compatible = "11-pin-accessory";
> >> 		connector-socket;
> > 
> > I don't see any advantage to allowing connectors anywhere in the tree:
> > pretty much by definition a connector is a "whole board" concept.  So
> > I think instead they should all go in a new special node under the
> > root, say /connectors.  With that done, you don't need the
> > connector-socket tag any more.
> > 
> 
> I’m fine with that. But only for the portable connector case.

Ok.

> >> 		spi1 {
> >> 			target_phandle = <&spi_1>;
> >> 		};
> >> 	};
> >> 
> >> };
> >> 
> >> &spi_1 {
> >> 	ethernet-switch@0 {
> >> 		compatible = "micrel,ks8995m";
> >> 	};
> >> };
> >> 
> >> 
> >> $ cat spi_codec_overlay_with_connector_v2.dts
> >> 
> >> /dts-v1/;
> >> 
> >> /plugin/;
> >> 
> >> &connector_1 {
> >> 	connector-plug;
> >> 	compatible = "11-pin-accessory";
> >> 
> >> 	spi1 {
> >> 		codec@1 {
> >> 			compatible = "ti,tlv320aic26";
> >> 		};
> >> 	};
> >> };
> >> 
> >> 
> >> The spi_codec_overlay_with_connector_v2.dtb __fixups__ information
> >> is unchanged from the previous example, but the kernel overlay
> >> code will do the correct extra level of dereferencing when it
> >> detects the connector-plug property in the overlay.
> >> 
> >> The one remaining piece that this patch does not provide is how
> >> the overlay manager (which does not yet exist in the mainline
> >> tree) can apply an overlay to two different targets.  That
> >> final step should be a trivial change to of_overlay_create(),
> >> adding a parameter that is a mapping of the target (or maybe
> >> even targets) in the overlay to different targets in the
> >> active device tree.
> >> 
> >> This seems like a more straight forward way to handle connectors.
> >> 
> >> First, ignoring pinctrl and pinmux, what does everyone think?
> >> 
> >> Then, the next step is whether pinctrl and pinmux work with this method.
> >> Pantelis, can you point me to a good example for
> >> 
> >>  1) an in-tree board dts file
> >>  2) an overlay file (I am assuming out of tree) that applies to the board
> >>  3) an in-tree .dtsi file that would provide the same features as
> >>     the overlay file if it was included by the board dts file
> >> 
> >> It should be easier to discuss pinctrl and pinmux with an example.
> > 
> > Hrm.. so I think you're trying to stick too close to the existing
> > overlay model.  Something I've always disliked about that model is
> > that the plugin can overlay *anywhere* in the master tree, meaning it
> > must have intimate knowledge of that tree.  Instead of using the
> > global __symbols__, there should be a set of "symbols" local to the
> > specific connector (socket), which are the *only* points which the
> > plugin is allowed to overlay or reference.
> > 
> 
> This is about the portable connector case.

Right, if I understand you correctly, that's the only case I'm
discussing here.

> We can figure out some way for local symbols for the portable
> case to work, but the global symbols for board specific overlays must
> be able to work.
> 
> There are different use cases that call for both non-portable and portable
> overlays.

Right, I assumed that.  I'm suggesting this connector format in
addition to the existing overlay format (although I think the
connector format should be preferred when possible).

> > Given that we're going to need new code to support this new connector
> > model, I think we should also fix some of the uglies in the current
> > overlay format while we're at it.
> > 
> 
> We need to keep compatibility with the old format. There are 5 million
> RPIs sold, half a million beaglebones and C.H.I.P. is coming out too.
> They all use overlays in one form or another.
> 
> That’s not counting all the custom boards that actively use them.
> 
> We have a user base now.

Sorry, I wasn't clear.  I'm not suggesting we abolish the existing
overlay format, or alter it.  Despite its flaws it has a userbase, as
you say.

But I'm saying if we're creating a new format for portable connectors,
we shouldn't inherit those flaws when we don't have to.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists