portable device tree connector -- problem statement

portable device tree connector -- problem statement

[Frank Rowand]

Hi Pantelis,

As I have been trying to understand the proposal for a portable device
tree connector, I am starting to appreciate the possible complexity of
the problem you are trying to solve.  I suspect that the details of the
problem space are something you know a lot about and take for granted.
On the other hand, I have no previous detailed knowledge of the beagle
family.

This means that I do not have a functional requirements model to
evaluate proposals against.

The following might or might not be correct, but it is the type
of information that would help create a functional requirements
model:

1) There are several different beaglebone boards
    - beaglebone, several versions  (does not have P8, P9,
      so not relevant?)
    - beaglebone (at least 6 revisions)
    - beaglebone black (at least 10 revisions)
    - beaglebone green (adds Grove connectors)
    - could be others, I don't know

2) The different beaglebones all have two expansion
   connectors?
    - all connectors have the same physical specification?
    - the pinout of the same connectors (P8 and P9)
      - is the same on all beaglebones?
      - is different on different beaglebones?
      - there is a small number of different pinouts?
      - there is a large number of different pinouts?
    - for bones with the same pinout:
      - the pins are routed to different function blocks on the
        SOC because different bones may have different SOCs?
        - the different functional blocks are compatible or not?
      - the pins are routed to different function blocks on the
        SOC, even though different bones have the same SOC,
        because that is a design decision by the bone designer?
    - for bones with a different pinout:
      - is there an overlap with the pinout of other bones, with
        just some of the pins having a different definition?

There are a lot of potential functional requirements based on
the above questions.  This is not meant to be anywhere near
exhaustive, just illustrative.

1) Provide an interface definition that allows a single cape
P8 definition to work on different versions of host board,
where the pinout is consistent across the bones, and the
routing of signals between the SOC (and other chips) to
the P8 connector is consistent.

2) Provide an interface definition that allows a single cape
   P8 definition to work on different versions of host board,
   where the pinout is consistent across the bones, but the
   routing of signals between the SOC (and other chips) to
   the P8 connector differs.

   1a) Same as "1)", but for P9.
   1b) Same as "1)", but for both P8 and P9.

3) Provide an interface definition that allows a single cape
   P8 definition that only uses a specified subset of the P8
   pins to work on different versions of the host board, where
   the pinout of the specified subset is consistent across the
   bones.

   2a) Same as "2)", but for P9.
   2b) Same as "2)", but for both P8 and P9.

4) Provide an interface definition that requires a different
   cape definition for different versions of host board.

Again, there are more possible variations on what functional
requirements might be useful to support in a portable device
tree connector.  I was just trying to provide a flavor of
what I am looking for.

Is the problem statement and an explanation of the underlying
constraints that lead to the problem statement available?

Thanks,

Frank

Re: portable device tree connector -- problem statement

[Mark Brown]

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

On Mon, Jul 04, 2016 at 01:58:53PM -0700, Frank Rowand wrote:

> On the other hand, I have no previous detailed knowledge of the beagle
> family.

This is in no way specific to the BeagleBones, there's plenty of other
boards out there with similar setups like the Raspberry Pi and its
derivatives.  

>     - for bones with the same pinout:
>       - the pins are routed to different function blocks on the
>         SOC because different bones may have different SOCs?
>         - the different functional blocks are compatible or not?

This is the general case, there will be a substantial level of
compatibility between different base boards by virtue of the pinouts
being the same but obviously there will be some variation in the
specifics (and even where that exists it may not be enough to be visible
at the DT level for the most part).  That said there will doubtless be
some plug in modules that want to rely on the specifics of a given base
board rather than remain compatible with general users of the interface.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: portable device tree connector -- problem statement

[Mark Brown]

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

On Mon, Jul 04, 2016 at 01:58:53PM -0700, Frank Rowand wrote:

> On the other hand, I have no previous detailed knowledge of the beagle
> family.

This is in no way specific to the BeagleBones, there's plenty of other
boards out there with similar setups like the Raspberry Pi and its
derivatives.  

>     - for bones with the same pinout:
>       - the pins are routed to different function blocks on the
>         SOC because different bones may have different SOCs?
>         - the different functional blocks are compatible or not?

This is the general case, there will be a substantial level of
compatibility between different base boards by virtue of the pinouts
being the same but obviously there will be some variation in the
specifics (and even where that exists it may not be enough to be visible
at the DT level for the most part).  That said there will doubtless be
some plug in modules that want to rely on the specifics of a given base
board rather than remain compatible with general users of the interface.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: portable device tree connector -- problem statement

[Frank Rowand]

On 07/05/16 01:31, Mark Brown wrote:
> On Mon, Jul 04, 2016 at 01:58:53PM -0700, Frank Rowand wrote:
> 
>> On the other hand, I have no previous detailed knowledge of the beagle
>> family.
> 
> This is in no way specific to the BeagleBones, there's plenty of other
> boards out there with similar setups like the Raspberry Pi and its
> derivatives.

Yes, absolutely.  I'm just picking on the beaglebones because that is
what Pantelis has recently used for examples.  (He has mentioned other
connector types and expansion boards in his presentations.)

And we need to think beyond beaglebone, pi, arduino, and grove 
type of connectors.

Some other connectors that are obvious are pci and possibly usb.


>>     - for bones with the same pinout:
>>       - the pins are routed to different function blocks on the
>>         SOC because different bones may have different SOCs?
>>         - the different functional blocks are compatible or not?
> 
> This is the general case, there will be a substantial level of
> compatibility between different base boards by virtue of the pinouts
> being the same but obviously there will be some variation in the
> specifics (and even where that exists it may not be enough to be visible
> at the DT level for the most part).  That said there will doubtless be
> some plug in modules that want to rely on the specifics of a given base
> board rather than remain compatible with general users of the interface.
> 

Re: portable device tree connector -- problem statement

[Pantelis Antoniou]

Hi Frank,

> On Jul 5, 2016, at 17:24 , Frank Rowand <frowand.list@gmail.com> wrote:
> 
> On 07/05/16 01:31, Mark Brown wrote:
>> On Mon, Jul 04, 2016 at 01:58:53PM -0700, Frank Rowand wrote:
>> 
>>> On the other hand, I have no previous detailed knowledge of the beagle
>>> family.
>> 
>> This is in no way specific to the BeagleBones, there's plenty of other
>> boards out there with similar setups like the Raspberry Pi and its
>> derivatives.
> 
> Yes, absolutely.  I'm just picking on the beaglebones because that is
> what Pantelis has recently used for examples.  (He has mentioned other
> connector types and expansion boards in his presentations.)
> 
> And we need to think beyond beaglebone, pi, arduino, and grove 
> type of connectors.
> 
> Some other connectors that are obvious are pci and possibly usb.
> 

Yes, in fact a growing number of platforms come with discoverable
PCI/USB boards which provide the busses and interfaces that
non-discoverable boards are plugged in.

Think an i2c-bus on pci-e boards on which a number of I2C peripherals
are located. The Vendor/Product IDs are the same for all these
expansions boards since the h/w designers do not want to spend money
on even a dip-switch or EEPROM for the IDs.

> 
>>>    - for bones with the same pinout:
>>>      - the pins are routed to different function blocks on the
>>>        SOC because different bones may have different SOCs?
>>>        - the different functional blocks are compatible or not?
>> 
>> This is the general case, there will be a substantial level of
>> compatibility between different base boards by virtue of the pinouts
>> being the same but obviously there will be some variation in the
>> specifics (and even where that exists it may not be enough to be visible
>> at the DT level for the most part).  That said there will doubtless be
>> some plug in modules that want to rely on the specifics of a given base
>> board rather than remain compatible with general users of the interface.
>> 
> 

Regards

— Pantelis

Re: portable device tree connector -- problem statement

[Pantelis Antoniou]

Hi Mark,

> On Jul 5, 2016, at 11:31 , Mark Brown <broonie@kernel.org> wrote:
> 
> On Mon, Jul 04, 2016 at 01:58:53PM -0700, Frank Rowand wrote:
> 
>> On the other hand, I have no previous detailed knowledge of the beagle
>> family.
> 
> This is in no way specific to the BeagleBones, there's plenty of other
> boards out there with similar setups like the Raspberry Pi and its
> derivatives.  
> 

There are a lot of custom vendor boards that use it.
We need to handle custom board too.

>>    - for bones with the same pinout:
>>      - the pins are routed to different function blocks on the
>>        SOC because different bones may have different SOCs?
>>        - the different functional blocks are compatible or not?
> 
> This is the general case, there will be a substantial level of
> compatibility between different base boards by virtue of the pinouts
> being the same but obviously there will be some variation in the
> specifics (and even where that exists it may not be enough to be visible
> at the DT level for the most part).  That said there will doubtless be
> some plug in modules that want to rely on the specifics of a given base
> board rather than remain compatible with general users of the interface.

Even for plug in modules that need a specific board it is typical that new
SoCs/boards appear in the future that are backwards compatible.

Regards

— Pantelis

DT connectors, thoughts

[David Gibson]

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

Hi,

Here's some of my thoughts on how a connector format for the DT could
be done.  Sorry it's taken longer than I hoped - I've been pretty
swamped in my day job.

This is pretty early thoughts, but gives an outline of the approach I
prefer.

So.. start with an example of a board DT including a widget socket,
which contains pins for an MMIO bus, an i2c bus and 2 interrupt lines.

/dts-v1/;

/ {
	compatible = "foo,oldboard";
	ranges;
	soc@... {
		ranges;
		mmio: mmio-bus@... {
			#address-cells = <2>;
			#size-cells = <2>;
			ranges;
		};
		i2c: i2c@... {
		};
		intc: intc@... {
			#interrupt-cells = <2>;
		};
	};

	connectors {
		widget1 {
			compatible = "foo,widget-socket";
			w1_irqs: irqs {
				interrupt-controller;
				#address-cells = <0>;
				#interrupt-cells = <1>;
				interrupt-map-mask = <0xffffffff>;
				interrupt-map = <
					0 &intc 7 0
					1 &intc 8 0
				>;
			};
			aliases = {
				i2c = &i2c;
				intc = &w1_irqs;
				mmio = &mmio;
			};
		};
	};
};

Note that the symbols are local to the connector, and explicitly
listed, rather than including all labels in the tree.  This is to
enforce (or at the very least encourage) plugins to only access those
parts of the base tree.

Note also the use of an interrupt nexus node contained within the
connector to control which irqs the socketed device can use.  I think
this needs some work to properly handle unit addresses, but hope
that's enough to give the rough idea.

So, what does the thing that goes in the socket look like?  I'm
thinking some new dts syntax like this:

/dts-v1/;

/plugin/ foo,widget-socket {
	compatible = "foo,whirligig-widget";
};

&i2c {
	whirligig-controller@... {
		...
		interrupt-parent = <&widget-irqs>;
		interrupts = <0>;
	};
};

Use of the /plugin/ keyword is rather different from existing
practice, so we may want a new one instead.

The idea is that this would be compiled to something like:

/dts-v1/;

/ {
	socket-type = "foo,widget-socket";
	compatible = "foo,whirligig-widget";

	fragment@0 {
		target-alias = "i2c";
		__overlay__ {
			whirligig-controller@... {
				...
				interrupt-parent = <0xffffffff>;
				interrupts = <0>;
			};
		};
	};
	__phandle_fixups__ {
		/* These are (path, property, offset) tuples) */
		widget-irqs =
			"/fragment@0/__overlay__/whirligig-controller@...",
			"interrupt-parent", <0>;
	};
};


Suppose then there's a new version of the board.  This extends the
widget socket in a backwards compatible way, but there are now two
interchangeable sockets, and they're wired up to different irqs and
i2c lines on the baseboard:

/dts-v1/;

/ {
	compatible = "foo,newboard";
	ranges;
	soc@... {
		ranges;	
		mmio: mmio-bus@... {
			#address-cells = <2>;
			#size-cells = <2>;
			ranges;
		};
		i2c0: i2c@... {
		};
		i2c1: i2c@... {
		};
		intc: intc@... {
		};
	};

	connectors {
		widget1 {
			compatible = "foo,widget-socket-v2", "foo,widget-socket";
			w1_irqs: irqs {
				interrupt-controller;
				#address-cells = <0>;
				#interrupt-cells = <1>;
				interrupt-map-mask = <0xffffffff>;
				interrupt-map = <
					0 &intc 17 0
					1 &intc 8 0
				>;
			};
			aliases = {
				i2c = &i2c0;
				intc = &w1_irqs;
				mmio = &mmio;
			};
		};
		widget2 {
			compatible = "foo,widget-socket-v2", "foo,widget-socket";
			w2_irqs: irqs {
				interrupt-controller;
				#address-cells = <0>;
				#interrupt-cells = <1>;
				interrupt-map-mask = <0xffffffff>;
				interrupt-map = <
					0 &intc 9 0
					1 &intc 10 0
				>;
			};
			aliases = {
				i2c = &i2c1;
				widget-irqs = &w2_irqs;
				mmio = &mmio;
			};
		};
	};
};


A socketed device could also have it's own connectors - the contrived
example below has a little 256 byte mmio space (maybe some sort of LPC
thingy?):


/dts-v1/;

/plugin/ foo,widget-socket-v2 {
	compatible = "foo,superduper-widget};

	connectors {
		compatible = "foo,super-socket";
		aliases {
			superbus = &superbus;
		};	
	};
};

&mmio {
	superbus: super-bridge@100000000 {
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0x0  0xabcd0000 0x12345600  0x100>;
	};
};

&i2c {
	super-controller@... {
		...
	};
	duper-controller@... {
	};
};

Thoughts?


-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: DT connectors, thoughts

[Pantelis Antoniou]

Hi David,

Spent some time looking at this, and it looks like it’s going to the right direction.

Comments inline.

> On Jul 18, 2016, at 17:20 , David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> Hi,
> 
> Here's some of my thoughts on how a connector format for the DT could
> be done.  Sorry it's taken longer than I hoped - I've been pretty
> swamped in my day job.
> 
> This is pretty early thoughts, but gives an outline of the approach I
> prefer.
> 
> So.. start with an example of a board DT including a widget socket,
> which contains pins for an MMIO bus, an i2c bus and 2 interrupt lines.
> 
> /dts-v1/;
> 
> / {
> 	compatible = "foo,oldboard";
> 	ranges;
> 	soc@... {
> 		ranges;
> 		mmio: mmio-bus@... {
> 			#address-cells = <2>;
> 			#size-cells = <2>;
> 			ranges;
> 		};

MMIO busses are going the way of the dodo and we have serious problems
handling them in linux in a connector (and a portable manner).
While we have drivers for GPMC devices we don’t have an in kernel framework
for handling them.

A single address range does not contain enough information to program a GPMC interface
with all the timings and chip select options. It might be possible to declare a
pre-define memory window on the connector, but it’s use on a real system might
be limited.

I think it’s best we focus on standard busses like i2c/spi/i2s/mmc and gpios and
interrupts for now.

> 		i2c: i2c@... {
> 		};
> 		intc: intc@... {
> 			#interrupt-cells = <2>;
> 		};
> 	};
> 
> 	connectors {
> 		widget1 {
> 			compatible = "foo,widget-socket";
> 			w1_irqs: irqs {
> 				interrupt-controller;
> 				#address-cells = <0>;
> 				#interrupt-cells = <1>;
> 				interrupt-map-mask = <0xffffffff>;
> 				interrupt-map = <
> 					0 &intc 7 0
> 					1 &intc 8 0
> 				>;
> 			};

This is fine. We need an interrupt controller node.

In a similar manner we need GPIOs too for every GPIO option on the
connector. Could we fold this in the same node?

> 			aliases = {
> 				i2c = &i2c;
> 				intc = &w1_irqs;
> 				mmio = &mmio;
> 			};
> 		};
> 	};
> };
> 
> Note that the symbols are local to the connector, and explicitly
> listed, rather than including all labels in the tree.  This is to
> enforce (or at the very least encourage) plugins to only access those
> parts of the base tree.
> 
> Note also the use of an interrupt nexus node contained within the
> connector to control which irqs the socketed device can use.  I think
> this needs some work to properly handle unit addresses, but hope
> that's enough to give the rough idea.
> 
> So, what does the thing that goes in the socket look like?  I'm
> thinking some new dts syntax like this:
> 
> /dts-v1/;
> 
> /plugin/ foo,widget-socket {
> 	compatible = "foo,whirligig-widget";
> };
> 
> &i2c {
> 	whirligig-controller@... {
> 		...
> 		interrupt-parent = <&widget-irqs>;
> 		interrupts = <0>;
> 	};
> };
> 

OK, this is brand new syntax. I’m all for it if it makes things easier.

> Use of the /plugin/ keyword is rather different from existing
> practice, so we may want a new one instead.
> 

It’s a bit weird looking and is bound to cause confusion.
How about something like /expansion/ ?

> The idea is that this would be compiled to something like:
> 
> /dts-v1/;
> 
> / {
> 	socket-type = "foo,widget-socket";
> 	compatible = "foo,whirligig-widget";
> 
> 	fragment@0 {
> 		target-alias = "i2c";
> 		__overlay__ {
> 			whirligig-controller@... {
> 				...
> 				interrupt-parent = <0xffffffff>;
> 				interrupts = <0>;
> 			};
> 		};
> 	};
> 	__phandle_fixups__ {
> 		/* These are (path, property, offset) tuples) */
> 		widget-irqs =
> 			"/fragment@0/__overlay__/whirligig-controller@...",
> 			"interrupt-parent", <0>;
> 	};

I’m not quite sure this is going to work for multiple use of widget-irqs handle,
but it’s a detail for now.

What is the action undertaken when a bus is activated? Looks like it’s going to
be similar to my patch where the target/alias bus is given a status=“okay”; property
and activated, after all subnodes that contain i2c devices are copied there. 
> };
> 
> 
> Suppose then there's a new version of the board.  This extends the
> widget socket in a backwards compatible way, but there are now two
> interchangeable sockets, and they're wired up to different irqs and
> i2c lines on the baseboard:
> 
> /dts-v1/;
> 
> / {
> 	compatible = "foo,newboard";
> 	ranges;
> 	soc@... {
> 		ranges;	
> 		mmio: mmio-bus@... {
> 			#address-cells = <2>;
> 			#size-cells = <2>;
> 			ranges;
> 		};
> 		i2c0: i2c@... {
> 		};
> 		i2c1: i2c@... {
> 		};
> 		intc: intc@... {
> 		};
> 	};
> 
> 	connectors {
> 		widget1 {
> 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> 			w1_irqs: irqs {
> 				interrupt-controller;
> 				#address-cells = <0>;
> 				#interrupt-cells = <1>;
> 				interrupt-map-mask = <0xffffffff>;
> 				interrupt-map = <
> 					0 &intc 17 0
> 					1 &intc 8 0
> 				>;
> 			};
> 			aliases = {
> 				i2c = &i2c0;
> 				intc = &w1_irqs;
> 				mmio = &mmio;
> 			};
> 		};
> 		widget2 {
> 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> 			w2_irqs: irqs {
> 				interrupt-controller;
> 				#address-cells = <0>;
> 				#interrupt-cells = <1>;
> 				interrupt-map-mask = <0xffffffff>;
> 				interrupt-map = <
> 					0 &intc 9 0
> 					1 &intc 10 0
> 				>;
> 			};
> 			aliases = {
> 				i2c = &i2c1;
> 				widget-irqs = &w2_irqs;
> 				mmio = &mmio;
> 			};
> 		};
> 	};
> };
> 
> 
> A socketed device could also have it's own connectors - the contrived
> example below has a little 256 byte mmio space (maybe some sort of LPC
> thingy?):
> 
> 
> /dts-v1/;
> 
> /plugin/ foo,widget-socket-v2 {
> 	compatible = "foo,superduper-widget};
> 
> 	connectors {
> 		compatible = "foo,super-socket";
> 		aliases {
> 			superbus = &superbus;
> 		};	
> 	};
> };
> 
> &mmio {
> 	superbus: super-bridge@100000000 {
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		ranges = <0x0  0xabcd0000 0x12345600  0x100>;
> 	};
> };
> 
> &i2c {
> 	super-controller@... {
> 		...
> 	};
> 	duper-controller@... {
> 	};
> };
> 
> Thoughts?
> 

It’s a step in the right direction, especially if we nail down the syntax.

> 
> -- 
> 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

Regards

— Pantelis

Re: DT connectors, thoughts

[David Gibson]

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

On Wed, Jul 20, 2016 at 11:59:44PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> Spent some time looking at this, and it looks like it’s going to the right direction.
> 
> Comments inline.
> 
> > On Jul 18, 2016, at 17:20 , David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > Hi,
> > 
> > Here's some of my thoughts on how a connector format for the DT could
> > be done.  Sorry it's taken longer than I hoped - I've been pretty
> > swamped in my day job.
> > 
> > This is pretty early thoughts, but gives an outline of the approach I
> > prefer.
> > 
> > So.. start with an example of a board DT including a widget socket,
> > which contains pins for an MMIO bus, an i2c bus and 2 interrupt lines.
> > 
> > /dts-v1/;
> > 
> > / {
> > 	compatible = "foo,oldboard";
> > 	ranges;
> > 	soc@... {
> > 		ranges;
> > 		mmio: mmio-bus@... {
> > 			#address-cells = <2>;
> > 			#size-cells = <2>;
> > 			ranges;
> > 		};
> 
> MMIO busses are going the way of the dodo and we have serious problems
> handling them in linux in a connector (and a portable manner).
> While we have drivers for GPMC devices we don’t have an in kernel framework
> for handling them.
> 
> A single address range does not contain enough information to program a GPMC interface
> with all the timings and chip select options. It might be possible to declare a
> pre-define memory window on the connector, but it’s use on a real system might
> be limited.

Ok.  I think the example has some value in showing how MMIO ranges and
mapping could be expressed even if it's only part of something more
complex than a simple MMIO bus.

For example I could imagine a connector which includes PCI and some
irq lines.  The PCI part is probable, of course, but a PCI device
wired to one of the hard interrupt lines instead of a PCI interrupt
line would need some DT information.  Of course non-Express, non-MSI
PCI is pretty much extinct too, but it's not much of a stretch to
imagine that something which requires some portion of MMIO mapping is
out there or will come along.

> I think it’s best we focus on standard busses like i2c/spi/i2s/mmc and gpios and
> interrupts for now.
> 
> > 		i2c: i2c@... {
> > 		};
> > 		intc: intc@... {
> > 			#interrupt-cells = <2>;
> > 		};
> > 	};
> > 
> > 	connectors {
> > 		widget1 {
> > 			compatible = "foo,widget-socket";
> > 			w1_irqs: irqs {
> > 				interrupt-controller;
> > 				#address-cells = <0>;
> > 				#interrupt-cells = <1>;
> > 				interrupt-map-mask = <0xffffffff>;
> > 				interrupt-map = <
> > 					0 &intc 7 0
> > 					1 &intc 8 0
> > 				>;
> > 			};
> 
> This is fine. We need an interrupt controller node.

Actually I think we only need an interrupt nexus, not an interrupt
controller (in IEEE1275 terminology).  (An interrupt controller would
generally require it's own driver, to ack/mask irqs, whereas this just
demonstrates the routing to an existing interrupt controller).  Which
makes that example slightly incorrect (it shouldn't have the
interrupt-controller property).

> In a similar manner we need GPIOs too for every GPIO option on the
> connector. Could we fold this in the same node?

IIRC the GPIO binding is pretty much modeled on the interrupt binding
and has a similar "nexus" concept.  I was expecting the same thing for
GPIO.  It's expressed with different properties to those for irqs,
obviously, so I guess it could be in the same node.  Whether it's
clearer to have them in the same or separate nodes I suspect would
depend on the specifics of the board.

> > 			aliases = {
> > 				i2c = &i2c;
> > 				intc = &w1_irqs;
> > 				mmio = &mmio;
> > 			};
> > 		};
> > 	};
> > };
> > 
> > Note that the symbols are local to the connector, and explicitly
> > listed, rather than including all labels in the tree.  This is to
> > enforce (or at the very least encourage) plugins to only access those
> > parts of the base tree.
> > 
> > Note also the use of an interrupt nexus node contained within the
> > connector to control which irqs the socketed device can use.  I think
> > this needs some work to properly handle unit addresses, but hope
> > that's enough to give the rough idea.
> > 
> > So, what does the thing that goes in the socket look like?  I'm
> > thinking some new dts syntax like this:
> > 
> > /dts-v1/;
> > 
> > /plugin/ foo,widget-socket {
> > 	compatible = "foo,whirligig-widget";
> > };
> > 
> > &i2c {
> > 	whirligig-controller@... {
> > 		...
> > 		interrupt-parent = <&widget-irqs>;
> > 		interrupts = <0>;
> > 	};
> > };
> > 
> 
> OK, this is brand new syntax. I’m all for it if it makes things easier.
> 
> > Use of the /plugin/ keyword is rather different from existing
> > practice, so we may want a new one instead.
> > 
> 
> It’s a bit weird looking and is bound to cause confusion.
> How about something like /expansion/ ?

That could work.

> > The idea is that this would be compiled to something like:
> > 
> > /dts-v1/;
> > 
> > / {
> > 	socket-type = "foo,widget-socket";
> > 	compatible = "foo,whirligig-widget";
> > 
> > 	fragment@0 {
> > 		target-alias = "i2c";
> > 		__overlay__ {
> > 			whirligig-controller@... {
> > 				...
> > 				interrupt-parent = <0xffffffff>;
> > 				interrupts = <0>;
> > 			};
> > 		};
> > 	};
> > 	__phandle_fixups__ {
> > 		/* These are (path, property, offset) tuples) */
> > 		widget-irqs =
> > 			"/fragment@0/__overlay__/whirligig-controller@...",
> > 			"interrupt-parent", <0>;
> > 	};
> 
> I’m not quite sure this is going to work for multiple use of widget-irqs handle,
> but it’s a detail for now.

Just concatenate all the tuples, so path, property, offset, path,
property, offset, etc..

> What is the action undertaken when a bus is activated? Looks like it’s going to
> be similar to my patch where the target/alias bus is given a status=“okay”; property
> and activated, after all subnodes that contain i2c devices are copied there. 

Erm.. what exactly do you mean by "activated"?  At the moment you
could put a status="okay" in the plugin component, and that would be
applied (as long as it goes in one of the accessible attachment
points).

Which does bring up a point.  I did wonder if the approach above
allows the plugin to do too much - e.g. overriding properties in the
i2c controller node, rather than just adding children.  So I did
wonder if we wanted a restriction that only new nodes can be added at
the top level of the plugin fragment.

Alternatively that might be achievable by (as a recommended / best
practice) putting a "container" subnode under each attachable bus on
the master dt and pointing the aliases at that instead of the actual
base bus controller.  With the right 'ranges' etc. that might
accomplish what's needed without extra semantics, but I'm not certain.

Ah.. which makes me think of another point.  In this proposal the
aliases is used to control both where fragments can be attached, and
what nodes can be referenced by phandle.  But we probably want to
split those concepts: e.g. the plugin will need to reference the
interrupt controller / nexus, but probably shouldn't be allowed to
override its properties.

> > };
> > 
> > 
> > Suppose then there's a new version of the board.  This extends the
> > widget socket in a backwards compatible way, but there are now two
> > interchangeable sockets, and they're wired up to different irqs and
> > i2c lines on the baseboard:
> > 
> > /dts-v1/;
> > 
> > / {
> > 	compatible = "foo,newboard";
> > 	ranges;
> > 	soc@... {
> > 		ranges;	
> > 		mmio: mmio-bus@... {
> > 			#address-cells = <2>;
> > 			#size-cells = <2>;
> > 			ranges;
> > 		};
> > 		i2c0: i2c@... {
> > 		};
> > 		i2c1: i2c@... {
> > 		};
> > 		intc: intc@... {
> > 		};
> > 	};
> > 
> > 	connectors {
> > 		widget1 {
> > 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> > 			w1_irqs: irqs {
> > 				interrupt-controller;
> > 				#address-cells = <0>;
> > 				#interrupt-cells = <1>;
> > 				interrupt-map-mask = <0xffffffff>;
> > 				interrupt-map = <
> > 					0 &intc 17 0
> > 					1 &intc 8 0
> > 				>;
> > 			};
> > 			aliases = {
> > 				i2c = &i2c0;
> > 				intc = &w1_irqs;
> > 				mmio = &mmio;
> > 			};
> > 		};
> > 		widget2 {
> > 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> > 			w2_irqs: irqs {
> > 				interrupt-controller;
> > 				#address-cells = <0>;
> > 				#interrupt-cells = <1>;
> > 				interrupt-map-mask = <0xffffffff>;
> > 				interrupt-map = <
> > 					0 &intc 9 0
> > 					1 &intc 10 0
> > 				>;
> > 			};
> > 			aliases = {
> > 				i2c = &i2c1;
> > 				widget-irqs = &w2_irqs;
> > 				mmio = &mmio;
> > 			};
> > 		};
> > 	};
> > };
> > 
> > 
> > A socketed device could also have it's own connectors - the contrived
> > example below has a little 256 byte mmio space (maybe some sort of LPC
> > thingy?):
> > 
> > 
> > /dts-v1/;
> > 
> > /plugin/ foo,widget-socket-v2 {
> > 	compatible = "foo,superduper-widget};
> > 
> > 	connectors {
> > 		compatible = "foo,super-socket";
> > 		aliases {
> > 			superbus = &superbus;
> > 		};	
> > 	};
> > };
> > 
> > &mmio {
> > 	superbus: super-bridge@100000000 {
> > 		#address-cells = <1>;
> > 		#size-cells = <1>;
> > 		ranges = <0x0  0xabcd0000 0x12345600  0x100>;
> > 	};
> > };
> > 
> > &i2c {
> > 	super-controller@... {
> > 		...
> > 	};
> > 	duper-controller@... {
> > 	};
> > };
> > 
> > Thoughts?
> > 
> 
> It’s a step in the right direction, especially if we nail down the
> syntax.

Excellent.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: DT connectors, thoughts

[Pantelis Antoniou]

Hi David,

> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> On Wed, Jul 20, 2016 at 11:59:44PM +0300, Pantelis Antoniou wrote:
>> Hi David,
>> 
>> Spent some time looking at this, and it looks like it’s going to the right direction.
>> 
>> Comments inline.
>> 
>>> On Jul 18, 2016, at 17:20 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>> 
>>> Hi,
>>> 
>>> Here's some of my thoughts on how a connector format for the DT could
>>> be done.  Sorry it's taken longer than I hoped - I've been pretty
>>> swamped in my day job.
>>> 
>>> This is pretty early thoughts, but gives an outline of the approach I
>>> prefer.
>>> 
>>> So.. start with an example of a board DT including a widget socket,
>>> which contains pins for an MMIO bus, an i2c bus and 2 interrupt lines.
>>> 
>>> /dts-v1/;
>>> 
>>> / {
>>> 	compatible = "foo,oldboard";
>>> 	ranges;
>>> 	soc@... {
>>> 		ranges;
>>> 		mmio: mmio-bus@... {
>>> 			#address-cells = <2>;
>>> 			#size-cells = <2>;
>>> 			ranges;
>>> 		};
>> 
>> MMIO busses are going the way of the dodo and we have serious problems
>> handling them in linux in a connector (and a portable manner).
>> While we have drivers for GPMC devices we don’t have an in kernel framework
>> for handling them.
>> 
>> A single address range does not contain enough information to program a GPMC interface
>> with all the timings and chip select options. It might be possible to declare a
>> pre-define memory window on the connector, but it’s use on a real system might
>> be limited.
> 
> Ok.  I think the example has some value in showing how MMIO ranges and
> mapping could be expressed even if it's only part of something more
> complex than a simple MMIO bus.
> 

Yep.

> For example I could imagine a connector which includes PCI and some
> irq lines.  The PCI part is probable, of course, but a PCI device
> wired to one of the hard interrupt lines instead of a PCI interrupt
> line would need some DT information.  Of course non-Express, non-MSI
> PCI is pretty much extinct too, but it's not much of a stretch to
> imagine that something which requires some portion of MMIO mapping is
> out there or will come along.

Yes, this is actually a real system we’re developing against. Non PCI-E
and non-MSI PCI is not extinct, it’s still kicking about, although not
in server/desktop class machines.

We should be able to figure things out.  

> 
>> I think it’s best we focus on standard busses like i2c/spi/i2s/mmc and gpios and
>> interrupts for now.
>> 
>>> 		i2c: i2c@... {
>>> 		};
>>> 		intc: intc@... {
>>> 			#interrupt-cells = <2>;
>>> 		};
>>> 	};
>>> 
>>> 	connectors {
>>> 		widget1 {
>>> 			compatible = "foo,widget-socket";
>>> 			w1_irqs: irqs {
>>> 				interrupt-controller;
>>> 				#address-cells = <0>;
>>> 				#interrupt-cells = <1>;
>>> 				interrupt-map-mask = <0xffffffff>;
>>> 				interrupt-map = <
>>> 					0 &intc 7 0
>>> 					1 &intc 8 0
>>> 				>;
>>> 			};
>> 
>> This is fine. We need an interrupt controller node.
> 
> Actually I think we only need an interrupt nexus, not an interrupt
> controller (in IEEE1275 terminology).  (An interrupt controller would
> generally require it's own driver, to ack/mask irqs, whereas this just
> demonstrates the routing to an existing interrupt controller).  Which
> makes that example slightly incorrect (it shouldn't have the
> interrupt-controller property).

Hmm, as far as I can tell we only have a concept of an interrupt controller
in the kernel. An interrupt nexus is something new. We should get by without
a driver but hacking the interrupt lookup path at DT.
 
> 
>> In a similar manner we need GPIOs too for every GPIO option on the
>> connector. Could we fold this in the same node?
> 
> IIRC the GPIO binding is pretty much modeled on the interrupt binding
> and has a similar "nexus" concept.  I was expecting the same thing for
> GPIO.  It's expressed with different properties to those for irqs,
> obviously, so I guess it could be in the same node.  Whether it's
> clearer to have them in the same or separate nodes I suspect would
> depend on the specifics of the board.
> 

Agreed. I should note that it’s pretty standard for a gpio controller to
advertise itself as an interrupt controller too.

>>> 			aliases = {
>>> 				i2c = &i2c;
>>> 				intc = &w1_irqs;
>>> 				mmio = &mmio;
>>> 			};
>>> 		};
>>> 	};
>>> };
>>> 
>>> Note that the symbols are local to the connector, and explicitly
>>> listed, rather than including all labels in the tree.  This is to
>>> enforce (or at the very least encourage) plugins to only access those
>>> parts of the base tree.
>>> 
>>> Note also the use of an interrupt nexus node contained within the
>>> connector to control which irqs the socketed device can use.  I think
>>> this needs some work to properly handle unit addresses, but hope
>>> that's enough to give the rough idea.
>>> 
>>> So, what does the thing that goes in the socket look like?  I'm
>>> thinking some new dts syntax like this:
>>> 
>>> /dts-v1/;
>>> 
>>> /plugin/ foo,widget-socket {
>>> 	compatible = "foo,whirligig-widget";
>>> };
>>> 
>>> &i2c {
>>> 	whirligig-controller@... {
>>> 		...
>>> 		interrupt-parent = <&widget-irqs>;
>>> 		interrupts = <0>;
>>> 	};
>>> };
>>> 
>> 
>> OK, this is brand new syntax. I’m all for it if it makes things easier.
>> 
>>> Use of the /plugin/ keyword is rather different from existing
>>> practice, so we may want a new one instead.
>>> 
>> 
>> It’s a bit weird looking and is bound to cause confusion.
>> How about something like /expansion/ ?
> 
> That could work.
> 
>>> The idea is that this would be compiled to something like:
>>> 
>>> /dts-v1/;
>>> 
>>> / {
>>> 	socket-type = "foo,widget-socket";
>>> 	compatible = "foo,whirligig-widget";
>>> 
>>> 	fragment@0 {
>>> 		target-alias = "i2c";
>>> 		__overlay__ {
>>> 			whirligig-controller@... {
>>> 				...
>>> 				interrupt-parent = <0xffffffff>;
>>> 				interrupts = <0>;
>>> 			};
>>> 		};
>>> 	};
>>> 	__phandle_fixups__ {
>>> 		/* These are (path, property, offset) tuples) */
>>> 		widget-irqs =
>>> 			"/fragment@0/__overlay__/whirligig-controller@...",
>>> 			"interrupt-parent", <0>;
>>> 	};
>> 
>> I’m not quite sure this is going to work for multiple use of widget-irqs handle,
>> but it’s a detail for now.
> 
> Just concatenate all the tuples, so path, property, offset, path,
> property, offset, etc..
> 

Note that parsing that property is going to be really awkward.

It’s [string] [string] [cell], …

We don’t have accessors for something like this.

>> What is the action undertaken when a bus is activated? Looks like it’s going to
>> be similar to my patch where the target/alias bus is given a status=“okay”; property
>> and activated, after all subnodes that contain i2c devices are copied there. 
> 
> Erm.. what exactly do you mean by "activated"?  At the moment you
> could put a status="okay" in the plugin component, and that would be
> applied (as long as it goes in one of the accessible attachment
> points).
> 

I mean that the bus is by default non-activated. When a portable
connector overlay is applied and the bus is referenced then the
board level bus must be enabled.

> Which does bring up a point.  I did wonder if the approach above
> allows the plugin to do too much - e.g. overriding properties in the
> i2c controller node, rather than just adding children.  So I did
> wonder if we wanted a restriction that only new nodes can be added at
> the top level of the plugin fragment.
> 

My RFC patch handles those cases. A connector device declares what kind
of properties are allowed to be copied to the board level bus node
(i.e. clock-freq, speed etc), and whether subnodes are supposed to be
copied there (for i2c client devices etc).

> Alternatively that might be achievable by (as a recommended / best
> practice) putting a "container" subnode under each attachable bus on
> the master dt and pointing the aliases at that instead of the actual
> base bus controller.  With the right 'ranges' etc. that might
> accomplish what's needed without extra semantics, but I'm not certain.
> 

Err, I would need an example to grok this.

> Ah.. which makes me think of another point.  In this proposal the
> aliases is used to control both where fragments can be attached, and
> what nodes can be referenced by phandle.  But we probably want to
> split those concepts: e.g. the plugin will need to reference the
> interrupt controller / nexus, but probably shouldn't be allowed to
> override its properties.
> 

Yep.

>>> };
>>> 
>>> 
>>> Suppose then there's a new version of the board.  This extends the
>>> widget socket in a backwards compatible way, but there are now two
>>> interchangeable sockets, and they're wired up to different irqs and
>>> i2c lines on the baseboard:
>>> 
>>> /dts-v1/;
>>> 
>>> / {
>>> 	compatible = "foo,newboard";
>>> 	ranges;
>>> 	soc@... {
>>> 		ranges;	
>>> 		mmio: mmio-bus@... {
>>> 			#address-cells = <2>;
>>> 			#size-cells = <2>;
>>> 			ranges;
>>> 		};
>>> 		i2c0: i2c@... {
>>> 		};
>>> 		i2c1: i2c@... {
>>> 		};
>>> 		intc: intc@... {
>>> 		};
>>> 	};
>>> 
>>> 	connectors {
>>> 		widget1 {
>>> 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
>>> 			w1_irqs: irqs {
>>> 				interrupt-controller;
>>> 				#address-cells = <0>;
>>> 				#interrupt-cells = <1>;
>>> 				interrupt-map-mask = <0xffffffff>;
>>> 				interrupt-map = <
>>> 					0 &intc 17 0
>>> 					1 &intc 8 0
>>> 				>;
>>> 			};
>>> 			aliases = {
>>> 				i2c = &i2c0;
>>> 				intc = &w1_irqs;
>>> 				mmio = &mmio;
>>> 			};
>>> 		};
>>> 		widget2 {
>>> 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
>>> 			w2_irqs: irqs {
>>> 				interrupt-controller;
>>> 				#address-cells = <0>;
>>> 				#interrupt-cells = <1>;
>>> 				interrupt-map-mask = <0xffffffff>;
>>> 				interrupt-map = <
>>> 					0 &intc 9 0
>>> 					1 &intc 10 0
>>> 				>;
>>> 			};
>>> 			aliases = {
>>> 				i2c = &i2c1;
>>> 				widget-irqs = &w2_irqs;
>>> 				mmio = &mmio;
>>> 			};
>>> 		};
>>> 	};
>>> };
>>> 
>>> 
>>> A socketed device could also have it's own connectors - the contrived
>>> example below has a little 256 byte mmio space (maybe some sort of LPC
>>> thingy?):
>>> 
>>> 
>>> /dts-v1/;
>>> 
>>> /plugin/ foo,widget-socket-v2 {
>>> 	compatible = "foo,superduper-widget};
>>> 
>>> 	connectors {
>>> 		compatible = "foo,super-socket";
>>> 		aliases {
>>> 			superbus = &superbus;
>>> 		};	
>>> 	};
>>> };
>>> 
>>> &mmio {
>>> 	superbus: super-bridge@100000000 {
>>> 		#address-cells = <1>;
>>> 		#size-cells = <1>;
>>> 		ranges = <0x0  0xabcd0000 0x12345600  0x100>;
>>> 	};
>>> };
>>> 
>>> &i2c {
>>> 	super-controller@... {
>>> 		...
>>> 	};
>>> 	duper-controller@... {
>>> 	};
>>> };
>>> 
>>> Thoughts?
>>> 
>> 
>> It’s a step in the right direction, especially if we nail down the
>> syntax.
> 
> Excellent.
> 

Regards

— Pantelis

> -- 
> 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

Re: DT connectors, thoughts

[Rob Herring]

On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi David,
>
>> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>
>> On Wed, Jul 20, 2016 at 11:59:44PM +0300, Pantelis Antoniou wrote:
>>> Hi David,
>>>
>>> Spent some time looking at this, and it looks like it’s going to the right direction.
>>>
>>> Comments inline.
>>>
>>>> On Jul 18, 2016, at 17:20 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Here's some of my thoughts on how a connector format for the DT could
>>>> be done.  Sorry it's taken longer than I hoped - I've been pretty
>>>> swamped in my day job.
>>>>
>>>> This is pretty early thoughts, but gives an outline of the approach I
>>>> prefer.

[...]

>>>>             i2c: i2c@... {
>>>>             };
>>>>             intc: intc@... {
>>>>                     #interrupt-cells = <2>;
>>>>             };
>>>>     };
>>>>
>>>>     connectors {
>>>>             widget1 {
>>>>                     compatible = "foo,widget-socket";
>>>>                     w1_irqs: irqs {
>>>>                             interrupt-controller;
>>>>                             #address-cells = <0>;
>>>>                             #interrupt-cells = <1>;
>>>>                             interrupt-map-mask = <0xffffffff>;
>>>>                             interrupt-map = <
>>>>                                     0 &intc 7 0
>>>>                                     1 &intc 8 0
>>>>                             >;
>>>>                     };
>>>
>>> This is fine. We need an interrupt controller node.
>>
>> Actually I think we only need an interrupt nexus, not an interrupt
>> controller (in IEEE1275 terminology).  (An interrupt controller would
>> generally require it's own driver, to ack/mask irqs, whereas this just
>> demonstrates the routing to an existing interrupt controller).  Which
>> makes that example slightly incorrect (it shouldn't have the
>> interrupt-controller property).
>
> Hmm, as far as I can tell we only have a concept of an interrupt controller
> in the kernel. An interrupt nexus is something new. We should get by without
> a driver but hacking the interrupt lookup path at DT.

Interrupt nexus is the interrupt-map property which is fully
supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
for connector gpios, too.

Rob

Re: DT connectors, thoughts

[Pantelis Antoniou]

Hi Rob,

> On Jul 21, 2016, at 22:09 , Rob Herring <robh+dt@kernel.org> wrote:
> 
> On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Hi David,
>> 
>>> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>> 
>>> On Wed, Jul 20, 2016 at 11:59:44PM +0300, Pantelis Antoniou wrote:
>>>> Hi David,
>>>> 
>>>> Spent some time looking at this, and it looks like it’s going to the right direction.
>>>> 
>>>> Comments inline.
>>>> 
>>>>> On Jul 18, 2016, at 17:20 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> Here's some of my thoughts on how a connector format for the DT could
>>>>> be done.  Sorry it's taken longer than I hoped - I've been pretty
>>>>> swamped in my day job.
>>>>> 
>>>>> This is pretty early thoughts, but gives an outline of the approach I
>>>>> prefer.
> 
> [...]
> 
>>>>>            i2c: i2c@... {
>>>>>            };
>>>>>            intc: intc@... {
>>>>>                    #interrupt-cells = <2>;
>>>>>            };
>>>>>    };
>>>>> 
>>>>>    connectors {
>>>>>            widget1 {
>>>>>                    compatible = "foo,widget-socket";
>>>>>                    w1_irqs: irqs {
>>>>>                            interrupt-controller;
>>>>>                            #address-cells = <0>;
>>>>>                            #interrupt-cells = <1>;
>>>>>                            interrupt-map-mask = <0xffffffff>;
>>>>>                            interrupt-map = <
>>>>>                                    0 &intc 7 0
>>>>>                                    1 &intc 8 0
>>>>>> ;
>>>>>                    };
>>>> 
>>>> This is fine. We need an interrupt controller node.
>>> 
>>> Actually I think we only need an interrupt nexus, not an interrupt
>>> controller (in IEEE1275 terminology).  (An interrupt controller would
>>> generally require it's own driver, to ack/mask irqs, whereas this just
>>> demonstrates the routing to an existing interrupt controller).  Which
>>> makes that example slightly incorrect (it shouldn't have the
>>> interrupt-controller property).
>> 
>> Hmm, as far as I can tell we only have a concept of an interrupt controller
>> in the kernel. An interrupt nexus is something new. We should get by without
>> a driver but hacking the interrupt lookup path at DT.
> 
> Interrupt nexus is the interrupt-map property which is fully
> supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
> for connector gpios, too.
> 

Is interrupt-map enough to cover all our cases? On all the cases that I see it
used is in the context of PCI or some sort of bus.

Is the example above well defined? As far as I can tell interrupt-controller is not
needed.
 
> Rob

Regards

— Pantelis

Re: DT connectors, thoughts

[Rob Herring]

On Thu, Jul 21, 2016 at 2:15 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Jul 21, 2016, at 22:09 , Rob Herring <robh+dt@kernel.org> wrote:
>>
>> On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Hi David,
>>>
>>>> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>
>>>> On Wed, Jul 20, 2016 at 11:59:44PM +0300, Pantelis Antoniou wrote:
>>>>> Hi David,
>>>>>
>>>>> Spent some time looking at this, and it looks like it’s going to the right direction.
>>>>>
>>>>> Comments inline.
>>>>>
>>>>>> On Jul 18, 2016, at 17:20 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Here's some of my thoughts on how a connector format for the DT could
>>>>>> be done.  Sorry it's taken longer than I hoped - I've been pretty
>>>>>> swamped in my day job.
>>>>>>
>>>>>> This is pretty early thoughts, but gives an outline of the approach I
>>>>>> prefer.
>>
>> [...]
>>
>>>>>>            i2c: i2c@... {
>>>>>>            };
>>>>>>            intc: intc@... {
>>>>>>                    #interrupt-cells = <2>;
>>>>>>            };
>>>>>>    };
>>>>>>
>>>>>>    connectors {
>>>>>>            widget1 {
>>>>>>                    compatible = "foo,widget-socket";
>>>>>>                    w1_irqs: irqs {
>>>>>>                            interrupt-controller;
>>>>>>                            #address-cells = <0>;
>>>>>>                            #interrupt-cells = <1>;
>>>>>>                            interrupt-map-mask = <0xffffffff>;
>>>>>>                            interrupt-map = <
>>>>>>                                    0 &intc 7 0
>>>>>>                                    1 &intc 8 0
>>>>>>> ;
>>>>>>                    };
>>>>>
>>>>> This is fine. We need an interrupt controller node.
>>>>
>>>> Actually I think we only need an interrupt nexus, not an interrupt
>>>> controller (in IEEE1275 terminology).  (An interrupt controller would
>>>> generally require it's own driver, to ack/mask irqs, whereas this just
>>>> demonstrates the routing to an existing interrupt controller).  Which
>>>> makes that example slightly incorrect (it shouldn't have the
>>>> interrupt-controller property).
>>>
>>> Hmm, as far as I can tell we only have a concept of an interrupt controller
>>> in the kernel. An interrupt nexus is something new. We should get by without
>>> a driver but hacking the interrupt lookup path at DT.
>>
>> Interrupt nexus is the interrupt-map property which is fully
>> supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
>> for connector gpios, too.
>>
>
> Is interrupt-map enough to cover all our cases? On all the cases that I see it
> used is in the context of PCI or some sort of bus.

I think it should be. IIRC, one of the ARM, Ltd. boards uses it in a
non-PCI context.

> Is the example above well defined? As far as I can tell interrupt-controller is not
> needed.

interrupt-controller should actually be dropped as that is supposed to
be mutually exclusive to interrupt-map, but I think the kernel doesn't
care.

Rob

Re: DT connectors, thoughts

[David Gibson]

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

On Thu, Jul 21, 2016 at 10:15:36PM +0300, Pantelis Antoniou wrote:
> Hi Rob,
> 
> > On Jul 21, 2016, at 22:09 , Rob Herring <robh+dt@kernel.org> wrote:
> > 
> > On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
> > <pantelis.antoniou@konsulko.com> wrote:
> >> Hi David,
> >> 
> >>> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.dropbear.id.au> wrote:
> >>> 
> >>> On Wed, Jul 20, 2016 at 11:59:44PM +0300, Pantelis Antoniou wrote:
> >>>> Hi David,
> >>>> 
> >>>> Spent some time looking at this, and it looks like it’s going to the right direction.
> >>>> 
> >>>> Comments inline.
> >>>> 
> >>>>> On Jul 18, 2016, at 17:20 , David Gibson <david@gibson.dropbear.id.au> wrote:
> >>>>> 
> >>>>> Hi,
> >>>>> 
> >>>>> Here's some of my thoughts on how a connector format for the DT could
> >>>>> be done.  Sorry it's taken longer than I hoped - I've been pretty
> >>>>> swamped in my day job.
> >>>>> 
> >>>>> This is pretty early thoughts, but gives an outline of the approach I
> >>>>> prefer.
> > 
> > [...]
> > 
> >>>>>            i2c: i2c@... {
> >>>>>            };
> >>>>>            intc: intc@... {
> >>>>>                    #interrupt-cells = <2>;
> >>>>>            };
> >>>>>    };
> >>>>> 
> >>>>>    connectors {
> >>>>>            widget1 {
> >>>>>                    compatible = "foo,widget-socket";
> >>>>>                    w1_irqs: irqs {
> >>>>>                            interrupt-controller;
> >>>>>                            #address-cells = <0>;
> >>>>>                            #interrupt-cells = <1>;
> >>>>>                            interrupt-map-mask = <0xffffffff>;
> >>>>>                            interrupt-map = <
> >>>>>                                    0 &intc 7 0
> >>>>>                                    1 &intc 8 0
> >>>>>> ;
> >>>>>                    };
> >>>> 
> >>>> This is fine. We need an interrupt controller node.
> >>> 
> >>> Actually I think we only need an interrupt nexus, not an interrupt
> >>> controller (in IEEE1275 terminology).  (An interrupt controller would
> >>> generally require it's own driver, to ack/mask irqs, whereas this just
> >>> demonstrates the routing to an existing interrupt controller).  Which
> >>> makes that example slightly incorrect (it shouldn't have the
> >>> interrupt-controller property).
> >> 
> >> Hmm, as far as I can tell we only have a concept of an interrupt controller
> >> in the kernel. An interrupt nexus is something new. We should get by without
> >> a driver but hacking the interrupt lookup path at DT.
> > 
> > Interrupt nexus is the interrupt-map property which is fully
> > supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
> > for connector gpios, too.
> > 
> 
> Is interrupt-map enough to cover all our cases? On all the cases that I see it

That's the most common use, but it's pretty general.  It basically
maps (source irq desc, source unit address) tuples to (dest interrupt
controller, dest irq desc) tuples.  The interrupt-map-mask lets us
ignore some parts of that input.

One of the weirder uses was in the DTs for the DMA controller used for
the Ethernet on many PPC 4xx chips.  Those had multiple irqs, wired to
different interrupt controllers on some SoCs, but the DT only allows
to specify a single interrupt-parent for a device.  We worked around
that by including an interrupt nexus in the node itself, which mapped
a fake local irq number (just an index) to the right irqs and
controllers.

> Is the example above well defined? As far as I can tell interrupt-controller is not
> needed.

The 'interrupt-controller' property should not be there, that was an
error on my part.  The rest should be ok.

The plugin piece would need to specify the interrupt-parent as w1_irs
on all its nodes, otherwise they'd just inherit from the parent bus
for their fragment, which might have odd results.

We should probably see if we can figure out a way to enforce that.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: DT connectors, thoughts

[David Gibson]

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

On Thu, Jul 21, 2016 at 10:15:36PM +0300, Pantelis Antoniou wrote:
> Hi Rob,
> 
> > On Jul 21, 2016, at 22:09 , Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > 
> > On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
> > <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> >> Hi David,
> >> 
> >>> On Jul 21, 2016, at 16:42 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >>> 
> >>> On Wed, Jul 20, 2016 at 11:59:44PM +0300, Pantelis Antoniou wrote:
> >>>> Hi David,
> >>>> 
> >>>> Spent some time looking at this, and it looks like it’s going to the right direction.
> >>>> 
> >>>> Comments inline.
> >>>> 
> >>>>> On Jul 18, 2016, at 17:20 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6rL0yjEaa7Ru@public.gmane.orgau> wrote:
> >>>>> 
> >>>>> Hi,
> >>>>> 
> >>>>> Here's some of my thoughts on how a connector format for the DT could
> >>>>> be done.  Sorry it's taken longer than I hoped - I've been pretty
> >>>>> swamped in my day job.
> >>>>> 
> >>>>> This is pretty early thoughts, but gives an outline of the approach I
> >>>>> prefer.
> > 
> > [...]
> > 
> >>>>>            i2c: i2c@... {
> >>>>>            };
> >>>>>            intc: intc@... {
> >>>>>                    #interrupt-cells = <2>;
> >>>>>            };
> >>>>>    };
> >>>>> 
> >>>>>    connectors {
> >>>>>            widget1 {
> >>>>>                    compatible = "foo,widget-socket";
> >>>>>                    w1_irqs: irqs {
> >>>>>                            interrupt-controller;
> >>>>>                            #address-cells = <0>;
> >>>>>                            #interrupt-cells = <1>;
> >>>>>                            interrupt-map-mask = <0xffffffff>;
> >>>>>                            interrupt-map = <
> >>>>>                                    0 &intc 7 0
> >>>>>                                    1 &intc 8 0
> >>>>>> ;
> >>>>>                    };
> >>>> 
> >>>> This is fine. We need an interrupt controller node.
> >>> 
> >>> Actually I think we only need an interrupt nexus, not an interrupt
> >>> controller (in IEEE1275 terminology).  (An interrupt controller would
> >>> generally require it's own driver, to ack/mask irqs, whereas this just
> >>> demonstrates the routing to an existing interrupt controller).  Which
> >>> makes that example slightly incorrect (it shouldn't have the
> >>> interrupt-controller property).
> >> 
> >> Hmm, as far as I can tell we only have a concept of an interrupt controller
> >> in the kernel. An interrupt nexus is something new. We should get by without
> >> a driver but hacking the interrupt lookup path at DT.
> > 
> > Interrupt nexus is the interrupt-map property which is fully
> > supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
> > for connector gpios, too.
> > 
> 
> Is interrupt-map enough to cover all our cases? On all the cases that I see it

That's the most common use, but it's pretty general.  It basically
maps (source irq desc, source unit address) tuples to (dest interrupt
controller, dest irq desc) tuples.  The interrupt-map-mask lets us
ignore some parts of that input.

One of the weirder uses was in the DTs for the DMA controller used for
the Ethernet on many PPC 4xx chips.  Those had multiple irqs, wired to
different interrupt controllers on some SoCs, but the DT only allows
to specify a single interrupt-parent for a device.  We worked around
that by including an interrupt nexus in the node itself, which mapped
a fake local irq number (just an index) to the right irqs and
controllers.

> Is the example above well defined? As far as I can tell interrupt-controller is not
> needed.

The 'interrupt-controller' property should not be there, that was an
error on my part.  The rest should be ok.

The plugin piece would need to specify the interrupt-parent as w1_irs
on all its nodes, otherwise they'd just inherit from the parent bus
for their fragment, which might have odd results.

We should probably see if we can figure out a way to enforce that.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: DT connectors, thoughts

[David Gibson]

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

On Thu, Jul 21, 2016 at 02:09:18PM -0500, Rob Herring wrote:
> On Thu, Jul 21, 2016 at 9:14 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
> > Hi David,
> >
> >> On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.dropbear.id.au> wrote:
> >>
> >> On Wed, Jul 20, 2016 at 11:59:44PM +0300, Pantelis Antoniou wrote:
> >>> Hi David,
> >>>
> >>> Spent some time looking at this, and it looks like it’s going to the right direction.
> >>>
> >>> Comments inline.
> >>>
> >>>> On Jul 18, 2016, at 17:20 , David Gibson <david@gibson.dropbear.id.au> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> Here's some of my thoughts on how a connector format for the DT could
> >>>> be done.  Sorry it's taken longer than I hoped - I've been pretty
> >>>> swamped in my day job.
> >>>>
> >>>> This is pretty early thoughts, but gives an outline of the approach I
> >>>> prefer.
> 
> [...]
> 
> >>>>             i2c: i2c@... {
> >>>>             };
> >>>>             intc: intc@... {
> >>>>                     #interrupt-cells = <2>;
> >>>>             };
> >>>>     };
> >>>>
> >>>>     connectors {
> >>>>             widget1 {
> >>>>                     compatible = "foo,widget-socket";
> >>>>                     w1_irqs: irqs {
> >>>>                             interrupt-controller;
> >>>>                             #address-cells = <0>;
> >>>>                             #interrupt-cells = <1>;
> >>>>                             interrupt-map-mask = <0xffffffff>;
> >>>>                             interrupt-map = <
> >>>>                                     0 &intc 7 0
> >>>>                                     1 &intc 8 0
> >>>>                             >;
> >>>>                     };
> >>>
> >>> This is fine. We need an interrupt controller node.
> >>
> >> Actually I think we only need an interrupt nexus, not an interrupt
> >> controller (in IEEE1275 terminology).  (An interrupt controller would
> >> generally require it's own driver, to ack/mask irqs, whereas this just
> >> demonstrates the routing to an existing interrupt controller).  Which
> >> makes that example slightly incorrect (it shouldn't have the
> >> interrupt-controller property).
> >
> > Hmm, as far as I can tell we only have a concept of an interrupt controller
> > in the kernel. An interrupt nexus is something new. We should get by without
> > a driver but hacking the interrupt lookup path at DT.
> 
> Interrupt nexus is the interrupt-map property which is fully
> supported. I'd expect we'll end up with a gpio nexus (i.e. gpio-map)
> for connector gpios, too.

Exactly.  I don't know if a gpio-map is already defined, but if it's
not it should be.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: DT connectors, thoughts

[David Gibson]

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

On Thu, Jul 21, 2016 at 05:14:33PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> > On Jul 21, 2016, at 16:42 , David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > On Wed, Jul 20, 2016 at 11:59:44PM +0300, Pantelis Antoniou wrote:
> >> Hi David,
> >> 
> >> Spent some time looking at this, and it looks like it’s going to the right direction.
> >> 
> >> Comments inline.
> >> 
> >>> On Jul 18, 2016, at 17:20 , David Gibson <david@gibson.dropbear.id.au> wrote:
> >>> 
> >>> Hi,
> >>> 
> >>> Here's some of my thoughts on how a connector format for the DT could
> >>> be done.  Sorry it's taken longer than I hoped - I've been pretty
> >>> swamped in my day job.
> >>> 
> >>> This is pretty early thoughts, but gives an outline of the approach I
> >>> prefer.
> >>> 
> >>> So.. start with an example of a board DT including a widget socket,
> >>> which contains pins for an MMIO bus, an i2c bus and 2 interrupt lines.
> >>> 
> >>> /dts-v1/;
> >>> 
> >>> / {
> >>> 	compatible = "foo,oldboard";
> >>> 	ranges;
> >>> 	soc@... {
> >>> 		ranges;
> >>> 		mmio: mmio-bus@... {
> >>> 			#address-cells = <2>;
> >>> 			#size-cells = <2>;
> >>> 			ranges;
> >>> 		};
> >> 
> >> MMIO busses are going the way of the dodo and we have serious problems
> >> handling them in linux in a connector (and a portable manner).
> >> While we have drivers for GPMC devices we don’t have an in kernel framework
> >> for handling them.
> >> 
> >> A single address range does not contain enough information to program a GPMC interface
> >> with all the timings and chip select options. It might be possible to declare a
> >> pre-define memory window on the connector, but it’s use on a real system might
> >> be limited.
> > 
> > Ok.  I think the example has some value in showing how MMIO ranges and
> > mapping could be expressed even if it's only part of something more
> > complex than a simple MMIO bus.
> > 
> 
> Yep.
> 
> > For example I could imagine a connector which includes PCI and some
> > irq lines.  The PCI part is probable, of course, but a PCI device
> > wired to one of the hard interrupt lines instead of a PCI interrupt
> > line would need some DT information.  Of course non-Express, non-MSI
> > PCI is pretty much extinct too, but it's not much of a stretch to
> > imagine that something which requires some portion of MMIO mapping is
> > out there or will come along.
> 
> Yes, this is actually a real system we’re developing against. Non PCI-E
> and non-MSI PCI is not extinct, it’s still kicking about, although not
> in server/desktop class machines.
> 
> We should be able to figure things out.  
> 
> > 
> >> I think it’s best we focus on standard busses like i2c/spi/i2s/mmc and gpios and
> >> interrupts for now.
> >> 
> >>> 		i2c: i2c@... {
> >>> 		};
> >>> 		intc: intc@... {
> >>> 			#interrupt-cells = <2>;
> >>> 		};
> >>> 	};
> >>> 
> >>> 	connectors {
> >>> 		widget1 {
> >>> 			compatible = "foo,widget-socket";
> >>> 			w1_irqs: irqs {
> >>> 				interrupt-controller;
> >>> 				#address-cells = <0>;
> >>> 				#interrupt-cells = <1>;
> >>> 				interrupt-map-mask = <0xffffffff>;
> >>> 				interrupt-map = <
> >>> 					0 &intc 7 0
> >>> 					1 &intc 8 0
> >>> 				>;
> >>> 			};
> >> 
> >> This is fine. We need an interrupt controller node.
> > 
> > Actually I think we only need an interrupt nexus, not an interrupt
> > controller (in IEEE1275 terminology).  (An interrupt controller would
> > generally require it's own driver, to ack/mask irqs, whereas this just
> > demonstrates the routing to an existing interrupt controller).  Which
> > makes that example slightly incorrect (it shouldn't have the
> > interrupt-controller property).
> 
> Hmm, as far as I can tell we only have a concept of an interrupt controller
> in the kernel. An interrupt nexus is something new. We should get by without
> a driver but hacking the interrupt lookup path at DT.

So, the kernel shouldn't need the concept of nexus outside the DT
parsing code.  During the DT parsing the kernel will use the
interrupt-map in the nexus to work out which interrupt controllers
each devices are ultimately wired up to.

> >> In a similar manner we need GPIOs too for every GPIO option on the
> >> connector. Could we fold this in the same node?
> > 
> > IIRC the GPIO binding is pretty much modeled on the interrupt binding
> > and has a similar "nexus" concept.  I was expecting the same thing for
> > GPIO.  It's expressed with different properties to those for irqs,
> > obviously, so I guess it could be in the same node.  Whether it's
> > clearer to have them in the same or separate nodes I suspect would
> > depend on the specifics of the board.
> 
> Agreed. I should note that it’s pretty standard for a gpio controller to
> advertise itself as an interrupt controller too.

Ok.

> >>> 			aliases = {
> >>> 				i2c = &i2c;
> >>> 				intc = &w1_irqs;
> >>> 				mmio = &mmio;
> >>> 			};
> >>> 		};
> >>> 	};
> >>> };
> >>> 
> >>> Note that the symbols are local to the connector, and explicitly
> >>> listed, rather than including all labels in the tree.  This is to
> >>> enforce (or at the very least encourage) plugins to only access those
> >>> parts of the base tree.
> >>> 
> >>> Note also the use of an interrupt nexus node contained within the
> >>> connector to control which irqs the socketed device can use.  I think
> >>> this needs some work to properly handle unit addresses, but hope
> >>> that's enough to give the rough idea.
> >>> 
> >>> So, what does the thing that goes in the socket look like?  I'm
> >>> thinking some new dts syntax like this:
> >>> 
> >>> /dts-v1/;
> >>> 
> >>> /plugin/ foo,widget-socket {
> >>> 	compatible = "foo,whirligig-widget";
> >>> };
> >>> 
> >>> &i2c {
> >>> 	whirligig-controller@... {
> >>> 		...
> >>> 		interrupt-parent = <&widget-irqs>;
> >>> 		interrupts = <0>;
> >>> 	};
> >>> };
> >>> 
> >> 
> >> OK, this is brand new syntax. I’m all for it if it makes things easier.
> >> 
> >>> Use of the /plugin/ keyword is rather different from existing
> >>> practice, so we may want a new one instead.
> >>> 
> >> 
> >> It’s a bit weird looking and is bound to cause confusion.
> >> How about something like /expansion/ ?
> > 
> > That could work.
> > 
> >>> The idea is that this would be compiled to something like:
> >>> 
> >>> /dts-v1/;
> >>> 
> >>> / {
> >>> 	socket-type = "foo,widget-socket";
> >>> 	compatible = "foo,whirligig-widget";
> >>> 
> >>> 	fragment@0 {
> >>> 		target-alias = "i2c";
> >>> 		__overlay__ {
> >>> 			whirligig-controller@... {
> >>> 				...
> >>> 				interrupt-parent = <0xffffffff>;
> >>> 				interrupts = <0>;
> >>> 			};
> >>> 		};
> >>> 	};
> >>> 	__phandle_fixups__ {
> >>> 		/* These are (path, property, offset) tuples) */
> >>> 		widget-irqs =
> >>> 			"/fragment@0/__overlay__/whirligig-controller@...",
> >>> 			"interrupt-parent", <0>;
> >>> 	};
> >> 
> >> I’m not quite sure this is going to work for multiple use of widget-irqs handle,
> >> but it’s a detail for now.
> > 
> > Just concatenate all the tuples, so path, property, offset, path,
> > property, offset, etc..
> > 
> 
> Note that parsing that property is going to be really awkward.
> 
> It’s [string] [string] [cell], …
> 
> We don’t have accessors for something like this.

Hm, shouldn't be that bad - I would have thought it was easier than
string parsing at any rate.  Note that going back to 1275 days there
are existing bindings which define properties with mixed string and
cell data.

But I'm not particularly attached to this format, feel free to suggest
a better one.

> >> What is the action undertaken when a bus is activated? Looks like it’s going to
> >> be similar to my patch where the target/alias bus is given a status=“okay”; property
> >> and activated, after all subnodes that contain i2c devices are copied there. 
> > 
> > Erm.. what exactly do you mean by "activated"?  At the moment you
> > could put a status="okay" in the plugin component, and that would be
> > applied (as long as it goes in one of the accessible attachment
> > points).
> > 
> 
> I mean that the bus is by default non-activated. When a portable
> connector overlay is applied and the bus is referenced then the
> board level bus must be enabled.

Ok.  I can see a couple of approaches for that.  One is for the
overlay to set a property in the bus node to mark it as active.
status="okey" is the obvious one but there are other possibilities.
That does raise questions about exactly what the overlay is allowed to
overwrite, of course.

The other possibility is for the overlay to just add the child node
and make sure the driver in the kernel activates the bus or not
depending on whether there are any child devices under it.  This
approach seems like it would be reasonably good practice on the kernel
side anyway, and might have less chance for multiple plugin DTs to
conflict with each other.

> > Which does bring up a point.  I did wonder if the approach above
> > allows the plugin to do too much - e.g. overriding properties in the
> > i2c controller node, rather than just adding children.  So I did
> > wonder if we wanted a restriction that only new nodes can be added at
> > the top level of the plugin fragment.
> > 
> 
> My RFC patch handles those cases. A connector device declares what kind
> of properties are allowed to be copied to the board level bus node
> (i.e. clock-freq, speed etc), and whether subnodes are supposed to be
> copied there (for i2c client devices etc).

Ok, probably makes sense to merge those aspects of our two proposals
if we can.

> > Alternatively that might be achievable by (as a recommended / best
> > practice) putting a "container" subnode under each attachable bus on
> > the master dt and pointing the aliases at that instead of the actual
> > base bus controller.  With the right 'ranges' etc. that might
> > accomplish what's needed without extra semantics, but I'm not certain.
> 
> Err, I would need an example to grok this.

...
	i2c@XXX {
		/* i2c controller node */
		device@YYY {
			/* fixed on-board device */
		};
		device@ZZZ {
			/* fixed on-board device */
		};
		socket_i2c: socket-devices {
			ranges;
		};
	};
...

So the idea is the connector description would reference socket_i2c,
rather than the i2c controller itself.  That means that the plugin
can't override properties on the i2c controller, but can add i2c devices.

> > Ah.. which makes me think of another point.  In this proposal the
> > aliases is used to control both where fragments can be attached, and
> > what nodes can be referenced by phandle.  But we probably want to
> > split those concepts: e.g. the plugin will need to reference the
> > interrupt controller / nexus, but probably shouldn't be allowed to
> > override its properties.
> 
> Yep.
> 
> >>> };
> >>> 
> >>> 
> >>> Suppose then there's a new version of the board.  This extends the
> >>> widget socket in a backwards compatible way, but there are now two
> >>> interchangeable sockets, and they're wired up to different irqs and
> >>> i2c lines on the baseboard:
> >>> 
> >>> /dts-v1/;
> >>> 
> >>> / {
> >>> 	compatible = "foo,newboard";
> >>> 	ranges;
> >>> 	soc@... {
> >>> 		ranges;	
> >>> 		mmio: mmio-bus@... {
> >>> 			#address-cells = <2>;
> >>> 			#size-cells = <2>;
> >>> 			ranges;
> >>> 		};
> >>> 		i2c0: i2c@... {
> >>> 		};
> >>> 		i2c1: i2c@... {
> >>> 		};
> >>> 		intc: intc@... {
> >>> 		};
> >>> 	};
> >>> 
> >>> 	connectors {
> >>> 		widget1 {
> >>> 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> >>> 			w1_irqs: irqs {
> >>> 				interrupt-controller;
> >>> 				#address-cells = <0>;
> >>> 				#interrupt-cells = <1>;
> >>> 				interrupt-map-mask = <0xffffffff>;
> >>> 				interrupt-map = <
> >>> 					0 &intc 17 0
> >>> 					1 &intc 8 0
> >>> 				>;
> >>> 			};
> >>> 			aliases = {
> >>> 				i2c = &i2c0;
> >>> 				intc = &w1_irqs;
> >>> 				mmio = &mmio;
> >>> 			};
> >>> 		};
> >>> 		widget2 {
> >>> 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> >>> 			w2_irqs: irqs {
> >>> 				interrupt-controller;
> >>> 				#address-cells = <0>;
> >>> 				#interrupt-cells = <1>;
> >>> 				interrupt-map-mask = <0xffffffff>;
> >>> 				interrupt-map = <
> >>> 					0 &intc 9 0
> >>> 					1 &intc 10 0
> >>> 				>;
> >>> 			};
> >>> 			aliases = {
> >>> 				i2c = &i2c1;
> >>> 				widget-irqs = &w2_irqs;
> >>> 				mmio = &mmio;
> >>> 			};
> >>> 		};
> >>> 	};
> >>> };
> >>> 
> >>> 
> >>> A socketed device could also have it's own connectors - the contrived
> >>> example below has a little 256 byte mmio space (maybe some sort of LPC
> >>> thingy?):
> >>> 
> >>> 
> >>> /dts-v1/;
> >>> 
> >>> /plugin/ foo,widget-socket-v2 {
> >>> 	compatible = "foo,superduper-widget};
> >>> 
> >>> 	connectors {
> >>> 		compatible = "foo,super-socket";
> >>> 		aliases {
> >>> 			superbus = &superbus;
> >>> 		};	
> >>> 	};
> >>> };
> >>> 
> >>> &mmio {
> >>> 	superbus: super-bridge@100000000 {
> >>> 		#address-cells = <1>;
> >>> 		#size-cells = <1>;
> >>> 		ranges = <0x0  0xabcd0000 0x12345600  0x100>;
> >>> 	};
> >>> };
> >>> 
> >>> &i2c {
> >>> 	super-controller@... {
> >>> 		...
> >>> 	};
> >>> 	duper-controller@... {
> >>> 	};
> >>> };
> >>> 
> >>> Thoughts?
> >>> 
> >> 
> >> It’s a step in the right direction, especially if we nail down the
> >> syntax.
> > 
> > Excellent.
> > 
> 
> Regards
> 
> — Pantelis
> 
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: DT connectors, thoughts

[Pantelis Antoniou]

Hi David,

Spent some time looking at this, and it looks like it’s going to the right direction.

Comments inline.

> On Jul 18, 2016, at 17:20 , David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> Hi,
> 
> Here's some of my thoughts on how a connector format for the DT could
> be done.  Sorry it's taken longer than I hoped - I've been pretty
> swamped in my day job.
> 
> This is pretty early thoughts, but gives an outline of the approach I
> prefer.
> 
> So.. start with an example of a board DT including a widget socket,
> which contains pins for an MMIO bus, an i2c bus and 2 interrupt lines.
> 
> /dts-v1/;
> 
> / {
> 	compatible = "foo,oldboard";
> 	ranges;
> 	soc@... {
> 		ranges;
> 		mmio: mmio-bus@... {
> 			#address-cells = <2>;
> 			#size-cells = <2>;
> 			ranges;
> 		};

MMIO busses are going the way of the dodo and we have serious problems
handling them in linux in a connector (and a portable manner).
While we have drivers for GPMC devices we don’t have an in kernel framework
for handling them.

A single address range does not contain enough information to program a GPMC interface
with all the timings and chip select options. It might be possible to declare a
pre-define memory window on the connector, but it’s use on a real system might
be limited.

I think it’s best we focus on standard busses like i2c/spi/i2s/mmc and gpios and
interrupts for now.

> 		i2c: i2c@... {
> 		};
> 		intc: intc@... {
> 			#interrupt-cells = <2>;
> 		};
> 	};
> 
> 	connectors {
> 		widget1 {
> 			compatible = "foo,widget-socket";
> 			w1_irqs: irqs {
> 				interrupt-controller;
> 				#address-cells = <0>;
> 				#interrupt-cells = <1>;
> 				interrupt-map-mask = <0xffffffff>;
> 				interrupt-map = <
> 					0 &intc 7 0
> 					1 &intc 8 0
> 				>;
> 			};

This is fine. We need an interrupt controller node.

In a similar manner we need GPIOs too for every GPIO option on the
connector. Could we fold this in the same node?

> 			aliases = {
> 				i2c = &i2c;
> 				intc = &w1_irqs;
> 				mmio = &mmio;
> 			};
> 		};
> 	};
> };
> 
> Note that the symbols are local to the connector, and explicitly
> listed, rather than including all labels in the tree.  This is to
> enforce (or at the very least encourage) plugins to only access those
> parts of the base tree.
> 
> Note also the use of an interrupt nexus node contained within the
> connector to control which irqs the socketed device can use.  I think
> this needs some work to properly handle unit addresses, but hope
> that's enough to give the rough idea.
> 
> So, what does the thing that goes in the socket look like?  I'm
> thinking some new dts syntax like this:
> 
> /dts-v1/;
> 
> /plugin/ foo,widget-socket {
> 	compatible = "foo,whirligig-widget";
> };
> 
> &i2c {
> 	whirligig-controller@... {
> 		...
> 		interrupt-parent = <&widget-irqs>;
> 		interrupts = <0>;
> 	};
> };
> 

OK, this is brand new syntax. I’m all for it if it makes things easier.

> Use of the /plugin/ keyword is rather different from existing
> practice, so we may want a new one instead.
> 

It’s a bit weird looking and is bound to cause confusion.
How about something like /expansion/ ?

> The idea is that this would be compiled to something like:
> 
> /dts-v1/;
> 
> / {
> 	socket-type = "foo,widget-socket";
> 	compatible = "foo,whirligig-widget";
> 
> 	fragment@0 {
> 		target-alias = "i2c";
> 		__overlay__ {
> 			whirligig-controller@... {
> 				...
> 				interrupt-parent = <0xffffffff>;
> 				interrupts = <0>;
> 			};
> 		};
> 	};
> 	__phandle_fixups__ {
> 		/* These are (path, property, offset) tuples) */
> 		widget-irqs =
> 			"/fragment@0/__overlay__/whirligig-controller@...",
> 			"interrupt-parent", <0>;
> 	};

I’m not quite sure this is going to work for multiple use of widget-irqs handle,
but it’s a detail for now.

What is the action undertaken when a bus is activated? Looks like it’s going to
be similar to my patch where the target/alias bus is given a status=“okay”; property
and activated, after all subnodes that contain i2c devices are copied there. 
> };
> 
> 
> Suppose then there's a new version of the board.  This extends the
> widget socket in a backwards compatible way, but there are now two
> interchangeable sockets, and they're wired up to different irqs and
> i2c lines on the baseboard:
> 
> /dts-v1/;
> 
> / {
> 	compatible = "foo,newboard";
> 	ranges;
> 	soc@... {
> 		ranges;	
> 		mmio: mmio-bus@... {
> 			#address-cells = <2>;
> 			#size-cells = <2>;
> 			ranges;
> 		};
> 		i2c0: i2c@... {
> 		};
> 		i2c1: i2c@... {
> 		};
> 		intc: intc@... {
> 		};
> 	};
> 
> 	connectors {
> 		widget1 {
> 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> 			w1_irqs: irqs {
> 				interrupt-controller;
> 				#address-cells = <0>;
> 				#interrupt-cells = <1>;
> 				interrupt-map-mask = <0xffffffff>;
> 				interrupt-map = <
> 					0 &intc 17 0
> 					1 &intc 8 0
> 				>;
> 			};
> 			aliases = {
> 				i2c = &i2c0;
> 				intc = &w1_irqs;
> 				mmio = &mmio;
> 			};
> 		};
> 		widget2 {
> 			compatible = "foo,widget-socket-v2", "foo,widget-socket";
> 			w2_irqs: irqs {
> 				interrupt-controller;
> 				#address-cells = <0>;
> 				#interrupt-cells = <1>;
> 				interrupt-map-mask = <0xffffffff>;
> 				interrupt-map = <
> 					0 &intc 9 0
> 					1 &intc 10 0
> 				>;
> 			};
> 			aliases = {
> 				i2c = &i2c1;
> 				widget-irqs = &w2_irqs;
> 				mmio = &mmio;
> 			};
> 		};
> 	};
> };
> 
> 
> A socketed device could also have it's own connectors - the contrived
> example below has a little 256 byte mmio space (maybe some sort of LPC
> thingy?):
> 
> 
> /dts-v1/;
> 
> /plugin/ foo,widget-socket-v2 {
> 	compatible = "foo,superduper-widget};
> 
> 	connectors {
> 		compatible = "foo,super-socket";
> 		aliases {
> 			superbus = &superbus;
> 		};	
> 	};
> };
> 
> &mmio {
> 	superbus: super-bridge@100000000 {
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		ranges = <0x0  0xabcd0000 0x12345600  0x100>;
> 	};
> };
> 
> &i2c {
> 	super-controller@... {
> 		...
> 	};
> 	duper-controller@... {
> 	};
> };
> 
> Thoughts?
> 

It’s a step in the right direction, especially if we nail down the syntax.

> 
> -- 
> 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

Regards

— Pantelis

Re: DT connectors, thoughts

[Rob Herring]

On Mon, Jul 18, 2016 at 9:20 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> Hi,
>
> Here's some of my thoughts on how a connector format for the DT could
> be done.  Sorry it's taken longer than I hoped - I've been pretty
> swamped in my day job.
>
> This is pretty early thoughts, but gives an outline of the approach I
> prefer.
>
> So.. start with an example of a board DT including a widget socket,
> which contains pins for an MMIO bus, an i2c bus and 2 interrupt lines.
>
> /dts-v1/;
>
> / {
>         compatible = "foo,oldboard";
>         ranges;
>         soc@... {
>                 ranges;
>                 mmio: mmio-bus@... {
>                         #address-cells = <2>;
>                         #size-cells = <2>;
>                         ranges;
>                 };
>                 i2c: i2c@... {
>                 };
>                 intc: intc@... {
>                         #interrupt-cells = <2>;
>                 };
>         };
>
>         connectors {
>                 widget1 {
>                         compatible = "foo,widget-socket";
>                         w1_irqs: irqs {
>                                 interrupt-controller;
>                                 #address-cells = <0>;
>                                 #interrupt-cells = <1>;
>                                 interrupt-map-mask = <0xffffffff>;
>                                 interrupt-map = <
>                                         0 &intc 7 0
>                                         1 &intc 8 0
>                                 >;
>                         };
>                         aliases = {
>                                 i2c = &i2c;
>                                 intc = &w1_irqs;

I understand how you are using i2c alias, but not the intc. It would
help if the same names were not used in multiple places unless they
are the same thing.

What does using aliases here buy us vs. just properties with a phandle?

>                                 mmio = &mmio;
>                         };
>                 };
>         };
> };
>
> Note that the symbols are local to the connector, and explicitly
> listed, rather than including all labels in the tree.  This is to
> enforce (or at the very least encourage) plugins to only access those
> parts of the base tree.
>
> Note also the use of an interrupt nexus node contained within the
> connector to control which irqs the socketed device can use.  I think
> this needs some work to properly handle unit addresses, but hope
> that's enough to give the rough idea.
>
> So, what does the thing that goes in the socket look like?  I'm
> thinking some new dts syntax like this:
>
> /dts-v1/;
>
> /plugin/ foo,widget-socket {
>         compatible = "foo,whirligig-widget";
> };
>
> &i2c {
>         whirligig-controller@... {
>                 ...
>                 interrupt-parent = <&widget-irqs>;
>                 interrupts = <0>;
>         };
> };
>
> Use of the /plugin/ keyword is rather different from existing
> practice, so we may want a new one instead.
>
> The idea is that this would be compiled to something like:
>
> /dts-v1/;
>
> / {
>         socket-type = "foo,widget-socket";
>         compatible = "foo,whirligig-widget";
>
>         fragment@0 {
>                 target-alias = "i2c";

Yet another way to express the target... Every new feature for
overlays seems to define a new way. My thinking was the target is a
connector node and all devices are under it. In your case, the
connector is not part of the hierarchy for any devices in the overlay.
That may simplify adding OS support, but seems to be a less accurate
representation of the h/w.

>                 __overlay__ {
>                         whirligig-controller@... {
>                                 ...
>                                 interrupt-parent = <0xffffffff>;
>                                 interrupts = <0>;
>                         };
>                 };
>         };
>         __phandle_fixups__ {
>                 /* These are (path, property, offset) tuples) */
>                 widget-irqs =
>                         "/fragment@0/__overlay__/whirligig-controller@...",
>                         "interrupt-parent", <0>;
>         };
> };

Re: DT connectors, thoughts

[David Gibson]

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

On Thu, Jul 21, 2016 at 02:15:57PM -0500, Rob Herring wrote:
> On Mon, Jul 18, 2016 at 9:20 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > Hi,
> >
> > Here's some of my thoughts on how a connector format for the DT could
> > be done.  Sorry it's taken longer than I hoped - I've been pretty
> > swamped in my day job.
> >
> > This is pretty early thoughts, but gives an outline of the approach I
> > prefer.
> >
> > So.. start with an example of a board DT including a widget socket,
> > which contains pins for an MMIO bus, an i2c bus and 2 interrupt lines.
> >
> > /dts-v1/;
> >
> > / {
> >         compatible = "foo,oldboard";
> >         ranges;
> >         soc@... {
> >                 ranges;
> >                 mmio: mmio-bus@... {
> >                         #address-cells = <2>;
> >                         #size-cells = <2>;
> >                         ranges;
> >                 };
> >                 i2c: i2c@... {
> >                 };
> >                 intc: intc@... {
> >                         #interrupt-cells = <2>;
> >                 };
> >         };
> >
> >         connectors {
> >                 widget1 {
> >                         compatible = "foo,widget-socket";
> >                         w1_irqs: irqs {
> >                                 interrupt-controller;
> >                                 #address-cells = <0>;
> >                                 #interrupt-cells = <1>;
> >                                 interrupt-map-mask = <0xffffffff>;
> >                                 interrupt-map = <
> >                                         0 &intc 7 0
> >                                         1 &intc 8 0
> >                                 >;
> >                         };
> >                         aliases = {
> >                                 i2c = &i2c;
> >                                 intc = &w1_irqs;
> 
> I understand how you are using i2c alias, but not the intc. It would
> help if the same names were not used in multiple places unless they
> are the same thing.

Yes, sorry.  We have both the /soc/intc node which is the base board's
master interrupt controller.  Then we have the connector local 'intc'
alias which describes the local interrupt space for just the
connector.

> What does using aliases here buy us vs. just properties with a
> phandle?

Um.. I'm not sure what you mean.

> >                                 mmio = &mmio;
> >                         };
> >                 };
> >         };
> > };
> >
> > Note that the symbols are local to the connector, and explicitly
> > listed, rather than including all labels in the tree.  This is to
> > enforce (or at the very least encourage) plugins to only access those
> > parts of the base tree.
> >
> > Note also the use of an interrupt nexus node contained within the
> > connector to control which irqs the socketed device can use.  I think
> > this needs some work to properly handle unit addresses, but hope
> > that's enough to give the rough idea.
> >
> > So, what does the thing that goes in the socket look like?  I'm
> > thinking some new dts syntax like this:
> >
> > /dts-v1/;
> >
> > /plugin/ foo,widget-socket {
> >         compatible = "foo,whirligig-widget";
> > };
> >
> > &i2c {
> >         whirligig-controller@... {
> >                 ...
> >                 interrupt-parent = <&widget-irqs>;
> >                 interrupts = <0>;
> >         };
> > };
> >
> > Use of the /plugin/ keyword is rather different from existing
> > practice, so we may want a new one instead.
> >
> > The idea is that this would be compiled to something like:
> >
> > /dts-v1/;
> >
> > / {
> >         socket-type = "foo,widget-socket";
> >         compatible = "foo,whirligig-widget";
> >
> >         fragment@0 {
> >                 target-alias = "i2c";
> 
> Yet another way to express the target... Every new feature for
> overlays seems to define a new way.

Well, yes.  Frankly I think that's because the original ways of
describing the target were not well thought out.  Using a phandle is
awkward because it will always be -1 until fixups are applied, which
seems a bit pointless (why not have the alias/label directly).  Using
a full path means that the overlay can overwrite anywhere in the base
tree which doesn't seem like good practice.

> My thinking was the target is a
> connector node and all devices are under it.

I thought about this, and I think it's technically possible, but it
gets really ugly.  The trouble is that connectors frequently have pins
onto multiple buses.  In order to combine those into a single parent
node, we'd have to combine the address spaces of all those buses.
That can be done using some complex multi-cell encoding, but it won't
be pretty.  Then to make the connection point be a single node in the
base tree you'd essentially have to combine all the buses used in the
connector throughout the base board DT, so that ugly multi-cell
encoding will have to be used throughout most of the base DT, not just
in the connector stuff.

> In your case, the
> connector is not part of the hierarchy for any devices in the overlay.
> That may simplify adding OS support, but seems to be a less accurate
> representation of the h/w.

I think its the best we can do.  Because connectors usually combine
multiple logically distinct buses, they really don't exist as a
well-defined point in a single heirarchy.

> >                 __overlay__ {
> >                         whirligig-controller@... {
> >                                 ...
> >                                 interrupt-parent = <0xffffffff>;
> >                                 interrupts = <0>;
> >                         };
> >                 };
> >         };
> >         __phandle_fixups__ {
> >                 /* These are (path, property, offset) tuples) */
> >                 widget-irqs =
> >                         "/fragment@0/__overlay__/whirligig-controller@...",
> >                         "interrupt-parent", <0>;
> >         };
> > };
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: DT connectors, thoughts

[Stephen Boyd]

Quoting David Gibson (2016-07-21 21:25:56)
> On Thu, Jul 21, 2016 at 02:15:57PM -0500, Rob Herring wrote:
> > On Mon, Jul 18, 2016 at 9:20 AM, David Gibson
> > <david@gibson.dropbear.id.au> wrote:
> > 
> > I understand how you are using i2c alias, but not the intc. It would
> > help if the same names were not used in multiple places unless they
> > are the same thing.
> 
> Yes, sorry.  We have both the /soc/intc node which is the base board's
> master interrupt controller.  Then we have the connector local 'intc'
> alias which describes the local interrupt space for just the
> connector.
> 
> > What does using aliases here buy us vs. just properties with a
> > phandle?
> 
> Um.. I'm not sure what you mean.

I think Rob means drop the aliases node and just have:

	property = &phandle;

In this example:

	i2c = &i2c;
	intc = &w1_irqs;
	mmio = &mmio;

or perhaps to have a list of phandles and names that map to them?

	targets = <&i2c>, <&w1_irqs>, <&mmio>;
	target-names = "i2c", "intc", "mmio";

?

> 
> > >                                 mmio = &mmio;
> > >                         };
> > >                 };
> > >         };
> > > };
> > >
> > > Note that the symbols are local to the connector, and explicitly
> > > listed, rather than including all labels in the tree.  This is to
> > > enforce (or at the very least encourage) plugins to only access those
> > > parts of the base tree.
> > >
> > > Note also the use of an interrupt nexus node contained within the
> > > connector to control which irqs the socketed device can use.  I think
> > > this needs some work to properly handle unit addresses, but hope
> > > that's enough to give the rough idea.
> > >
> > > So, what does the thing that goes in the socket look like?  I'm
> > > thinking some new dts syntax like this:
> > >
> > > /dts-v1/;
> > >
> > > /plugin/ foo,widget-socket {
> > >         compatible = "foo,whirligig-widget";
> > > };
> > >
> > > &i2c {
> > >         whirligig-controller@... {
> > >                 ...
> > >                 interrupt-parent = <&widget-irqs>;
> > >                 interrupts = <0>;
> > >         };
> > > };

How would we support an expansion board that goes onto two
sockets/connectors provided by the baseboard when the connectors
"export" the same phandle aliases? From what I can tell with this design
we'll be unable to describe a device on the expansion board that is
wired to properties provided by the two connectors.

Re: DT connectors, thoughts

[David Gibson]

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

On Thu, Aug 25, 2016 at 06:38:41PM -0700, Stephen Boyd wrote:
> Quoting David Gibson (2016-07-21 21:25:56)
> > On Thu, Jul 21, 2016 at 02:15:57PM -0500, Rob Herring wrote:
> > > On Mon, Jul 18, 2016 at 9:20 AM, David Gibson
> > > <david@gibson.dropbear.id.au> wrote:
> > > 
> > > I understand how you are using i2c alias, but not the intc. It would
> > > help if the same names were not used in multiple places unless they
> > > are the same thing.
> > 
> > Yes, sorry.  We have both the /soc/intc node which is the base board's
> > master interrupt controller.  Then we have the connector local 'intc'
> > alias which describes the local interrupt space for just the
> > connector.
> > 
> > > What does using aliases here buy us vs. just properties with a
> > > phandle?
> > 
> > Um.. I'm not sure what you mean.
> 
> I think Rob means drop the aliases node and just have:

Oh, ok.  The reason for the aliases node is that putting the aliases
(or whatever you want to call them) in the top level connector node
limits what potential extensions we can make to the connector format.
The aliases can essentially have any property name, so they could
collide with additional "metadata" properties we might want to add.

> 	property = &phandle;

Note that the example above is misleading.  Without the < >, the label
reference will expand to a path string, not a phandle.

> In this example:
> 
> 	i2c = &i2c;
> 	intc = &w1_irqs;
> 	mmio = &mmio;
> 
> or perhaps to have a list of phandles and names that map to them?
> 
> 	targets = <&i2c>, <&w1_irqs>, <&mmio>;
> 	target-names = "i2c", "intc", "mmio";

That would work, but a) using phandles instead of paths introduces an
additional lookup for no particular benegit and b) it seems harder to
parse.  In addition the aliases format suggested matches the global
aliases property already defined by IEEE1275, so it seems a good place
to start.

> 
> ?
> 
> > 
> > > >                                 mmio = &mmio;
> > > >                         };
> > > >                 };
> > > >         };
> > > > };
> > > >
> > > > Note that the symbols are local to the connector, and explicitly
> > > > listed, rather than including all labels in the tree.  This is to
> > > > enforce (or at the very least encourage) plugins to only access those
> > > > parts of the base tree.
> > > >
> > > > Note also the use of an interrupt nexus node contained within the
> > > > connector to control which irqs the socketed device can use.  I think
> > > > this needs some work to properly handle unit addresses, but hope
> > > > that's enough to give the rough idea.
> > > >
> > > > So, what does the thing that goes in the socket look like?  I'm
> > > > thinking some new dts syntax like this:
> > > >
> > > > /dts-v1/;
> > > >
> > > > /plugin/ foo,widget-socket {
> > > >         compatible = "foo,whirligig-widget";
> > > > };
> > > >
> > > > &i2c {
> > > >         whirligig-controller@... {
> > > >                 ...
> > > >                 interrupt-parent = <&widget-irqs>;
> > > >                 interrupts = <0>;
> > > >         };
> > > > };
> 
> How would we support an expansion board that goes onto two
> sockets/connectors provided by the baseboard when the connectors
> "export" the same phandle aliases? From what I can tell with this design
> we'll be unable to describe a device on the expansion board that is
> wired to properties provided by the two connectors.

Ok, so there are two parts to this.

1) Allowing a plugin to use multiple connectors.

I thought a bit about this case, but didn't address it for
simplicity.  That would require a different syntax, so we can rethink
this if it's a use case we think we need.

2) Dealing with alias collisions between connector types

This one is fairly straightforward to handle.  By default, we'll use
labels from connectors we plug into "as is".  However, we can add a
syntax that allows us to locally rename labels from a connector (for
those familiar with Python, think "import foo from bar as baz").


So, combining those thoughts together, I'm thinking dtc format for
something connecting to two different widget sockets (pretty much the
worst case) would look something like:

/plugin/ foo,widget-socket {
};

/plugin/ foo,widget-socket {
	realias {
		i2c-b = "i2c";
		intc-b = "intc";
		mmio-b = "mmio";
	};
};

&i2c {
	.. devices on the i2c from the first plug ..
};

&i2c-b {
	.. devices on the i2c from the second plug ..
};

Obviously we'd also need to devise an encoding for this to compile
into, since the one I proposed previously won't work in this case.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: DT connectors, thoughts

[Stephen Boyd]

Quoting David Gibson (2016-08-29 06:45:11)
> On Thu, Aug 25, 2016 at 06:38:41PM -0700, Stephen Boyd wrote:
> > Quoting David Gibson (2016-07-21 21:25:56)
> > > On Thu, Jul 21, 2016 at 02:15:57PM -0500, Rob Herring wrote:
> > > > On Mon, Jul 18, 2016 at 9:20 AM, David Gibson
> > > > <david@gibson.dropbear.id.au> wrote:
> > > > 
> > > > I understand how you are using i2c alias, but not the intc. It would
> > > > help if the same names were not used in multiple places unless they
> > > > are the same thing.
> > > 
> > > Yes, sorry.  We have both the /soc/intc node which is the base board's
> > > master interrupt controller.  Then we have the connector local 'intc'
> > > alias which describes the local interrupt space for just the
> > > connector.
> > > 
> > > > What does using aliases here buy us vs. just properties with a
> > > > phandle?
> > > 
> > > Um.. I'm not sure what you mean.
> > 
> > I think Rob means drop the aliases node and just have:
> 
> Oh, ok.  The reason for the aliases node is that putting the aliases
> (or whatever you want to call them) in the top level connector node
> limits what potential extensions we can make to the connector format.
> The aliases can essentially have any property name, so they could
> collide with additional "metadata" properties we might want to add.

Agreed. Putting them into a subnode will prevent any collisions, but
what sorts of collisions would there even be? Presumably the one making
up the connector binding will be choosing the phandles they want to
export with specific properties, and during that time they can also
choose to have other properties that don't conflict?

> 
> > 
> > How would we support an expansion board that goes onto two
> > sockets/connectors provided by the baseboard when the connectors
> > "export" the same phandle aliases? From what I can tell with this design
> > we'll be unable to describe a device on the expansion board that is
> > wired to properties provided by the two connectors.
> 
> Ok, so there are two parts to this.
> 
> 1) Allowing a plugin to use multiple connectors.
> 
> I thought a bit about this case, but didn't address it for
> simplicity.  That would require a different syntax, so we can rethink
> this if it's a use case we think we need.

Yes it would be nice to design for this case as well.

> 
> 2) Dealing with alias collisions between connector types
> 
> This one is fairly straightforward to handle.  By default, we'll use
> labels from connectors we plug into "as is".  However, we can add a
> syntax that allows us to locally rename labels from a connector (for
> those familiar with Python, think "import foo from bar as baz").
> 
> 
> So, combining those thoughts together, I'm thinking dtc format for
> something connecting to two different widget sockets (pretty much the
> worst case) would look something like:
> 
> /plugin/ foo,widget-socket {
> };
> 
> /plugin/ foo,widget-socket {
>         realias {
>                 i2c-b = "i2c";
>                 intc-b = "intc";
>                 mmio-b = "mmio";
>         };
> };
> 
> &i2c {
>         .. devices on the i2c from the first plug ..
> };
> 
> &i2c-b {
>         .. devices on the i2c from the second plug ..
> };
> 
> Obviously we'd also need to devise an encoding for this to compile
> into, since the one I proposed previously won't work in this case.
> 

I suppose we can distribute the realias nodes when we compile the plugin
into overlay fragments. The socket matching is a little vague though.
How would we know which socket to apply to when we have two identical
looking sockets? I'm thinking we could put some of that information into
the fragment itself.

/{
	compatible = "foo,whirlgig-widget";

	fragment@0 { /* corresponds to i2c in example above */
		target-socket = "foo,widget-socket-a";
		target-alias = "i2c";
		__overlay__ {
			....
		};
	};

	fragment@1 { /* corresponds to i2c-b in example above */
		target-socket = "foo,widget-socket-b";
		target-alias = "i2c";
		__overlay__ {
			...
		};
	};
};

If we have two identical connectors maybe we'll have to enforce that the
connectors have some more specific unique compatible string so that we
can match up the right socket. But I don't see how we can require that
the overlays know this detail if they only care about one socket and
could go into either one of them. In that case we should have the loader
ask the user which socket they connected this extension board to?

I was also thinking it would be better to leave the gpio-map and
interrupt-map properties at the connector level. For example:

	widget1 {
		compatible = "foo,widget-socket";
		interrupt-map-mask = <0xffffffff>;
		interrupt-map = <0 &intc 7 0>,
				<1 &intc 8 0>;
	};

and then we could put a label on the plugin/expansion syntax so we can
reference the connector as a whole:

	/plugin/ connector: foo,widget-socket {
		compatible = "foo,whirlgig-widget";
	};

	&i2c {
		device@40 {
			interrupt-parent = <&connector>;
			interrupts = <1>;
		};
	};

I also thought about making another alias inside the connector node to
point to itself, but that fails when you get into the situation of two
connectors and collisions, unless you rename them of course. It felt
better to leave that choice to the overlay though.

In conclusion, I see a few topics/patterns emerging:

1) Expose phandles through the connectors in some way that allows us to
   limit what the plugin/expansion boards can modify or use

2) Have some flexible syntax to remap cell sizes from the baseboard
   through the connector to provide a consistent connector size (i.e.
   remap interrupts and gpios from multiple sources, etc. into a fixed
   number of cells)

3) Allow plugin/expansion boards to use multiple connectors from the
   baseboard in a consistent way

4) Attempt to maintain almost all of the current overlay syntax with
   syntactic sugar

5) Connectors on expansion boards should be supported

Is there anything else we should add to this list?

Re: DT connectors, thoughts

[David Gibson]

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

On Mon, Aug 29, 2016 at 07:07:49PM -0700, Stephen Boyd wrote:
> Quoting David Gibson (2016-08-29 06:45:11)
> > On Thu, Aug 25, 2016 at 06:38:41PM -0700, Stephen Boyd wrote:
> > > Quoting David Gibson (2016-07-21 21:25:56)
> > > > On Thu, Jul 21, 2016 at 02:15:57PM -0500, Rob Herring wrote:
> > > > > On Mon, Jul 18, 2016 at 9:20 AM, David Gibson
> > > > > <david@gibson.dropbear.id.au> wrote:
> > > > > 
> > > > > I understand how you are using i2c alias, but not the intc. It would
> > > > > help if the same names were not used in multiple places unless they
> > > > > are the same thing.
> > > > 
> > > > Yes, sorry.  We have both the /soc/intc node which is the base board's
> > > > master interrupt controller.  Then we have the connector local 'intc'
> > > > alias which describes the local interrupt space for just the
> > > > connector.
> > > > 
> > > > > What does using aliases here buy us vs. just properties with a
> > > > > phandle?
> > > > 
> > > > Um.. I'm not sure what you mean.
> > > 
> > > I think Rob means drop the aliases node and just have:
> > 
> > Oh, ok.  The reason for the aliases node is that putting the aliases
> > (or whatever you want to call them) in the top level connector node
> > limits what potential extensions we can make to the connector format.
> > The aliases can essentially have any property name, so they could
> > collide with additional "metadata" properties we might want to add.
> 
> Agreed. Putting them into a subnode will prevent any collisions, but
> what sorts of collisions would there even be? Presumably the one making
> up the connector binding will be choosing the phandles they want to
> export with specific properties, and during that time they can also
> choose to have other properties that don't conflict?

Hmm.. I suppose.  It still seems conceptually cleaner to me to have
them in their own namespace.

> > > How would we support an expansion board that goes onto two
> > > sockets/connectors provided by the baseboard when the connectors
> > > "export" the same phandle aliases? From what I can tell with this design
> > > we'll be unable to describe a device on the expansion board that is
> > > wired to properties provided by the two connectors.
> > 
> > Ok, so there are two parts to this.
> > 
> > 1) Allowing a plugin to use multiple connectors.
> > 
> > I thought a bit about this case, but didn't address it for
> > simplicity.  That would require a different syntax, so we can rethink
> > this if it's a use case we think we need.
> 
> Yes it would be nice to design for this case as well.
> 
> > 
> > 2) Dealing with alias collisions between connector types
> > 
> > This one is fairly straightforward to handle.  By default, we'll use
> > labels from connectors we plug into "as is".  However, we can add a
> > syntax that allows us to locally rename labels from a connector (for
> > those familiar with Python, think "import foo from bar as baz").
> > 
> > 
> > So, combining those thoughts together, I'm thinking dtc format for
> > something connecting to two different widget sockets (pretty much the
> > worst case) would look something like:
> > 
> > /plugin/ foo,widget-socket {
> > };
> > 
> > /plugin/ foo,widget-socket {
> >         realias {
> >                 i2c-b = "i2c";
> >                 intc-b = "intc";
> >                 mmio-b = "mmio";
> >         };
> > };
> > 
> > &i2c {
> >         .. devices on the i2c from the first plug ..
> > };
> > 
> > &i2c-b {
> >         .. devices on the i2c from the second plug ..
> > };
> > 
> > Obviously we'd also need to devise an encoding for this to compile
> > into, since the one I proposed previously won't work in this case.
> > 
> 
> I suppose we can distribute the realias nodes when we compile the plugin
> into overlay fragments. The socket matching is a little vague though.
> How would we know which socket to apply to when we have two identical
> looking sockets? I'm thinking we could put some of that information into
> the fragment itself.

So, my assumption in this example was that the plugin could plug into
*any* two widget sockets.  If it needs to connect to specific ones,
then pretty much by definition, the sockets aren't really of
indistinguishable type.

> 
> /{
> 	compatible = "foo,whirlgig-widget";
> 
> 	fragment@0 { /* corresponds to i2c in example above */
> 		target-socket = "foo,widget-socket-a";
> 		target-alias = "i2c";
> 		__overlay__ {
> 			....
> 		};
> 	};
> 
> 	fragment@1 { /* corresponds to i2c-b in example above */
> 		target-socket = "foo,widget-socket-b";
> 		target-alias = "i2c";
> 		__overlay__ {
> 			...
> 		};
> 	};
> };

We don't need any new construct here.  In this case the sockets aren't
100% compatible, which we can notate with
	compatible = "widget-socket-a", "widget-socket";
In the base board.

Devices which can plug into any widget socket will use target-socket =
"widget-socket", those which require a specific one (including
requiring both) can specifically list "widget-socket-a" and/or
"widget-socket-b".

> If we have two identical connectors maybe we'll have to enforce that the
> connectors have some more specific unique compatible string so that we
> can match up the right socket. But I don't see how we can require that
> the overlays know this detail if they only care about one socket and
> could go into either one of them. In that case we should have the loader
> ask the user which socket they connected this extension board to?
> 
> I was also thinking it would be better to leave the gpio-map and
> interrupt-map properties at the connector level. For example:
> 
> 	widget1 {
> 		compatible = "foo,widget-socket";
> 		interrupt-map-mask = <0xffffffff>;
> 		interrupt-map = <0 &intc 7 0>,
> 				<1 &intc 8 0>;
> 	};

That could work - but we should (and implicitly, do) support either
way.  Using subnodes might be useful for particularly complex irq or
gpio mappings.

> and then we could put a label on the plugin/expansion syntax so we can
> reference the connector as a whole:
> 
> 	/plugin/ connector: foo,widget-socket {
> 		compatible = "foo,whirlgig-widget";
> 	};
> 
> 	&i2c {
> 		device@40 {
> 			interrupt-parent = <&connector>;
> 			interrupts = <1>;
> 		};
> 	};
> 
> I also thought about making another alias inside the connector node to
> point to itself, but that fails when you get into the situation of two
> connectors and collisions, unless you rename them of course. It felt
> better to leave that choice to the overlay though.
> 
> In conclusion, I see a few topics/patterns emerging:
> 
> 1) Expose phandles through the connectors in some way that allows us to
>    limit what the plugin/expansion boards can modify or use

Yes, I definitely think we want that.

> 2) Have some flexible syntax to remap cell sizes from the baseboard
>    through the connector to provide a consistent connector size (i.e.
>    remap interrupts and gpios from multiple sources, etc. into a fixed
>    number of cells)

I don't think we need any new constructs here.  If there are
mismatches we can put dummy bridges with appropriate ranges properties
on one side or the other.

The only thing I see that might want some help is that the connector
type should certainly imply a specific set of cell widths for all the
included buses.  So possibly we should supply some stuff to help
enforce that.

> 3) Allow plugin/expansion boards to use multiple connectors from the
>    baseboard in a consistent way

Seems reasonable.

> 4) Attempt to maintain almost all of the current overlay syntax with
>    syntactic sugar

I'm not really sure what you mean by that.

> 5) Connectors on expansion boards should be supported

Again, seems like a good goal to me.

> 
> Is there anything else we should add to this list?
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: DT connectors, thoughts

[Stephen Boyd]

Quoting David Gibson (2016-08-30 16:55:23)
> On Mon, Aug 29, 2016 at 07:07:49PM -0700, Stephen Boyd wrote:
> > Quoting David Gibson (2016-08-29 06:45:11)
> > > So, combining those thoughts together, I'm thinking dtc format for
> > > something connecting to two different widget sockets (pretty much the
> > > worst case) would look something like:
> > > 
> > > /plugin/ foo,widget-socket {
> > > };
> > > 
> > > /plugin/ foo,widget-socket {
> > >         realias {
> > >                 i2c-b = "i2c";
> > >                 intc-b = "intc";
> > >                 mmio-b = "mmio";
> > >         };
> > > };
> > > 
> > > &i2c {
> > >         .. devices on the i2c from the first plug ..
> > > };
> > > 
> > > &i2c-b {
> > >         .. devices on the i2c from the second plug ..
> > > };
> > > 
> > > Obviously we'd also need to devise an encoding for this to compile
> > > into, since the one I proposed previously won't work in this case.
> > > 
> > 
> > I suppose we can distribute the realias nodes when we compile the plugin
> > into overlay fragments. The socket matching is a little vague though.
> > How would we know which socket to apply to when we have two identical
> > looking sockets? I'm thinking we could put some of that information into
> > the fragment itself.
> 
> So, my assumption in this example was that the plugin could plug into
> *any* two widget sockets.  If it needs to connect to specific ones,
> then pretty much by definition, the sockets aren't really of
> indistinguishable type.

Ah ok. It wasn't clear on how we target where the extension board
connects into the full tree. It sounds like the decision of where to
plug in the extension board falls to the overlay manager then?

> 
> > 
> > /{
> >       compatible = "foo,whirlgig-widget";
> > 
> >       fragment@0 { /* corresponds to i2c in example above */
> >               target-socket = "foo,widget-socket-a";
> >               target-alias = "i2c";
> >               __overlay__ {
> >                       ....
> >               };
> >       };
> > 
> >       fragment@1 { /* corresponds to i2c-b in example above */
> >               target-socket = "foo,widget-socket-b";
> >               target-alias = "i2c";
> >               __overlay__ {
> >                       ...
> >               };
> >       };
> > };
> 
> We don't need any new construct here.  In this case the sockets aren't
> 100% compatible, which we can notate with
>         compatible = "widget-socket-a", "widget-socket";
> In the base board.
> 
> Devices which can plug into any widget socket will use target-socket =
> "widget-socket", those which require a specific one (including
> requiring both) can specifically list "widget-socket-a" and/or
> "widget-socket-b".

Agreed. How do we express the realias node in the overlay format though?
I was trying to come up with some way to make that work by
redistributing the alias to the fragments but that doesn't work so well.

> 
> > If we have two identical connectors maybe we'll have to enforce that the
> > connectors have some more specific unique compatible string so that we
> > can match up the right socket. But I don't see how we can require that
> > the overlays know this detail if they only care about one socket and
> > could go into either one of them. In that case we should have the loader
> > ask the user which socket they connected this extension board to?
> > 
> > I was also thinking it would be better to leave the gpio-map and
> > interrupt-map properties at the connector level. For example:
> > 
> >       widget1 {
> >               compatible = "foo,widget-socket";
> >               interrupt-map-mask = <0xffffffff>;
> >               interrupt-map = <0 &intc 7 0>,
> >                               <1 &intc 8 0>;
> >       };
> 
> That could work - but we should (and implicitly, do) support either
> way.  Using subnodes might be useful for particularly complex irq or
> gpio mappings.

Sounds good.

> 
> > and then we could put a label on the plugin/expansion syntax so we can
> > reference the connector as a whole:
> > 
> >       /plugin/ connector: foo,widget-socket {
> >               compatible = "foo,whirlgig-widget";
> >       };
> > 
> >       &i2c {
> >               device@40 {
> >                       interrupt-parent = <&connector>;
> >                       interrupts = <1>;
> >               };
> >       };
> > 
> > I also thought about making another alias inside the connector node to
> > point to itself, but that fails when you get into the situation of two
> > connectors and collisions, unless you rename them of course. It felt
> > better to leave that choice to the overlay though.
> > 
> > In conclusion, I see a few topics/patterns emerging:
> > 
> > 1) Expose phandles through the connectors in some way that allows us to
> >    limit what the plugin/expansion boards can modify or use
> 
> Yes, I definitely think we want that.
> 
> > 2) Have some flexible syntax to remap cell sizes from the baseboard
> >    through the connector to provide a consistent connector size (i.e.
> >    remap interrupts and gpios from multiple sources, etc. into a fixed
> >    number of cells)
> 
> I don't think we need any new constructs here.  If there are
> mismatches we can put dummy bridges with appropriate ranges properties
> on one side or the other.
> 
> The only thing I see that might want some help is that the connector
> type should certainly imply a specific set of cell widths for all the
> included buses.  So possibly we should supply some stuff to help
> enforce that.

I'm specifically thinking that anything that has a #<name>-cells
associated with it (#gpio-cells, #clock-cells, #power-domain-cells,
etc.) would need to have an associated <name>-map property and
associated parsing code so that we can make a consistent cell width for
the connector. If we have existing ways to make this work, e.g.
interrupt-map or ranges, then we don't need.

> 
> > 3) Allow plugin/expansion boards to use multiple connectors from the
> >    baseboard in a consistent way
> 
> Seems reasonable.
> 
> > 4) Attempt to maintain almost all of the current overlay syntax with
> >    syntactic sugar
> 
> I'm not really sure what you mean by that.
> 

I mean that we're not really trying to change the general structure of
the DT overlay syntax. At least from what I can tell we're planning to
convert this /plugin/ format into a DT overlay that gets compiled into
binary all inside DTC.

Re: DT connectors, thoughts

[David Gibson]

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

On Wed, Sep 07, 2016 at 04:44:58PM -0700, Stephen Boyd wrote:
> Quoting David Gibson (2016-08-30 16:55:23)
> > On Mon, Aug 29, 2016 at 07:07:49PM -0700, Stephen Boyd wrote:
> > > Quoting David Gibson (2016-08-29 06:45:11)
> > > > So, combining those thoughts together, I'm thinking dtc format for
> > > > something connecting to two different widget sockets (pretty much the
> > > > worst case) would look something like:
> > > > 
> > > > /plugin/ foo,widget-socket {
> > > > };
> > > > 
> > > > /plugin/ foo,widget-socket {
> > > >         realias {
> > > >                 i2c-b = "i2c";
> > > >                 intc-b = "intc";
> > > >                 mmio-b = "mmio";
> > > >         };
> > > > };
> > > > 
> > > > &i2c {
> > > >         .. devices on the i2c from the first plug ..
> > > > };
> > > > 
> > > > &i2c-b {
> > > >         .. devices on the i2c from the second plug ..
> > > > };
> > > > 
> > > > Obviously we'd also need to devise an encoding for this to compile
> > > > into, since the one I proposed previously won't work in this case.
> > > > 
> > > 
> > > I suppose we can distribute the realias nodes when we compile the plugin
> > > into overlay fragments. The socket matching is a little vague though.
> > > How would we know which socket to apply to when we have two identical
> > > looking sockets? I'm thinking we could put some of that information into
> > > the fragment itself.
> > 
> > So, my assumption in this example was that the plugin could plug into
> > *any* two widget sockets.  If it needs to connect to specific ones,
> > then pretty much by definition, the sockets aren't really of
> > indistinguishable type.
> 
> Ah ok. It wasn't clear on how we target where the extension board
> connects into the full tree. It sounds like the decision of where to
> plug in the extension board falls to the overlay manager then?

Yes.  The whole point of connectors is that they're interchangeable,
so neither the "plug" side nor the "socket" side should specify which
instance of the other part they're connected to.

So to resolve the whole tree in this case, extra information would be
needed.  Ideally it would come from probing (even something ad-hoc
like an ID register), failing that it would need to be admin
configured.

> > > /{
> > >       compatible = "foo,whirlgig-widget";
> > > 
> > >       fragment@0 { /* corresponds to i2c in example above */
> > >               target-socket = "foo,widget-socket-a";
> > >               target-alias = "i2c";
> > >               __overlay__ {
> > >                       ....
> > >               };
> > >       };
> > > 
> > >       fragment@1 { /* corresponds to i2c-b in example above */
> > >               target-socket = "foo,widget-socket-b";
> > >               target-alias = "i2c";
> > >               __overlay__ {
> > >                       ...
> > >               };
> > >       };
> > > };
> > 
> > We don't need any new construct here.  In this case the sockets aren't
> > 100% compatible, which we can notate with
> >         compatible = "widget-socket-a", "widget-socket";
> > In the base board.
> > 
> > Devices which can plug into any widget socket will use target-socket =
> > "widget-socket", those which require a specific one (including
> > requiring both) can specifically list "widget-socket-a" and/or
> > "widget-socket-b".
> 
> Agreed. How do we express the realias node in the overlay format though?
> I was trying to come up with some way to make that work by
> redistributing the alias to the fragments but that doesn't work so
> well.

So, I'm thinking of the overlay dt info in 3 pieces

1) Global metadata

  Not much here yet, but it seems like it could be useful.

2) Per-connector metadata

  This specifies what sort of "socket" each "plug" on the overlay
  needs to connect to, and any other information we need that's
  specific to a particular "plug"

3) DT fragments

  The actual device info applied once we've resolved all the aliases
  and whatnot.

The realias information would need to be in part (2)

How we encode those pieces into the final overlay dt is pretty close
to an arbitrary choice.

> > > If we have two identical connectors maybe we'll have to enforce that the
> > > connectors have some more specific unique compatible string so that we
> > > can match up the right socket. But I don't see how we can require that
> > > the overlays know this detail if they only care about one socket and
> > > could go into either one of them. In that case we should have the loader
> > > ask the user which socket they connected this extension board to?
> > > 
> > > I was also thinking it would be better to leave the gpio-map and
> > > interrupt-map properties at the connector level. For example:
> > > 
> > >       widget1 {
> > >               compatible = "foo,widget-socket";
> > >               interrupt-map-mask = <0xffffffff>;
> > >               interrupt-map = <0 &intc 7 0>,
> > >                               <1 &intc 8 0>;
> > >       };
> > 
> > That could work - but we should (and implicitly, do) support either
> > way.  Using subnodes might be useful for particularly complex irq or
> > gpio mappings.
> 
> Sounds good.
> 
> > 
> > > and then we could put a label on the plugin/expansion syntax so we can
> > > reference the connector as a whole:
> > > 
> > >       /plugin/ connector: foo,widget-socket {
> > >               compatible = "foo,whirlgig-widget";
> > >       };
> > > 
> > >       &i2c {
> > >               device@40 {
> > >                       interrupt-parent = <&connector>;
> > >                       interrupts = <1>;
> > >               };
> > >       };
> > > 
> > > I also thought about making another alias inside the connector node to
> > > point to itself, but that fails when you get into the situation of two
> > > connectors and collisions, unless you rename them of course. It felt
> > > better to leave that choice to the overlay though.
> > > 
> > > In conclusion, I see a few topics/patterns emerging:
> > > 
> > > 1) Expose phandles through the connectors in some way that allows us to
> > >    limit what the plugin/expansion boards can modify or use
> > 
> > Yes, I definitely think we want that.
> > 
> > > 2) Have some flexible syntax to remap cell sizes from the baseboard
> > >    through the connector to provide a consistent connector size (i.e.
> > >    remap interrupts and gpios from multiple sources, etc. into a fixed
> > >    number of cells)
> > 
> > I don't think we need any new constructs here.  If there are
> > mismatches we can put dummy bridges with appropriate ranges properties
> > on one side or the other.
> > 
> > The only thing I see that might want some help is that the connector
> > type should certainly imply a specific set of cell widths for all the
> > included buses.  So possibly we should supply some stuff to help
> > enforce that.
> 
> I'm specifically thinking that anything that has a #<name>-cells
> associated with it (#gpio-cells, #clock-cells, #power-domain-cells,
> etc.) would need to have an associated <name>-map property and
> associated parsing code so that we can make a consistent cell width for
> the connector. If we have existing ways to make this work, e.g.
> interrupt-map or ranges, then we don't need.

Yes, I agree.  All the "interrupt tree like" things need interrupt
tree like nexus nodes, with a map and map-mask.

However, we also need some way of ensuring that the #address-cells and
#size-cells for any buses we expose match properly.

Or maybe we need to invent something like a cross between 'ranges' and
'interrupt-map' that can remap ranges on one address space to the
address space attached to a different node.

> 
> > 
> > > 3) Allow plugin/expansion boards to use multiple connectors from the
> > >    baseboard in a consistent way
> > 
> > Seems reasonable.
> > 
> > > 4) Attempt to maintain almost all of the current overlay syntax with
> > >    syntactic sugar
> > 
> > I'm not really sure what you mean by that.
> > 
> 
> I mean that we're not really trying to change the general structure of
> the DT overlay syntax. At least from what I can tell we're planning to
> convert this /plugin/ format into a DT overlay that gets compiled into
> binary all inside DTC.

Roughly speaking, yes.  However I think we should take the opportunity
to address design flaws in the current overlay encoding as they come
up (e.g. the fact that it uses phandles which *always* have to be
resolved - better to directly use an alias / symbol / label).

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: DT connectors, thoughts

[Stephen Boyd]

Quoting David Gibson (2016-09-07 17:26:11)
> On Wed, Sep 07, 2016 at 04:44:58PM -0700, Stephen Boyd wrote:
> > Quoting David Gibson (2016-08-30 16:55:23)
> > > So, my assumption in this example was that the plugin could plug into
> > > *any* two widget sockets.  If it needs to connect to specific ones,
> > > then pretty much by definition, the sockets aren't really of
> > > indistinguishable type.
> > 
> > Ah ok. It wasn't clear on how we target where the extension board
> > connects into the full tree. It sounds like the decision of where to
> > plug in the extension board falls to the overlay manager then?
> 
> Yes.  The whole point of connectors is that they're interchangeable,
> so neither the "plug" side nor the "socket" side should specify which
> instance of the other part they're connected to.

Agreed. Another item in the list!

> 
> So to resolve the whole tree in this case, extra information would be
> needed.  Ideally it would come from probing (even something ad-hoc
> like an ID register), failing that it would need to be admin
> configured.

Sure.

> > 
> > Agreed. How do we express the realias node in the overlay format though?
> > I was trying to come up with some way to make that work by
> > redistributing the alias to the fragments but that doesn't work so
> > well.
> 
> So, I'm thinking of the overlay dt info in 3 pieces
> 
> 1) Global metadata
> 
>   Not much here yet, but it seems like it could be useful.
> 
> 2) Per-connector metadata
> 
>   This specifies what sort of "socket" each "plug" on the overlay
>   needs to connect to, and any other information we need that's
>   specific to a particular "plug"
> 
> 3) DT fragments
> 
>   The actual device info applied once we've resolved all the aliases
>   and whatnot.
> 
> The realias information would need to be in part (2)
> 
> How we encode those pieces into the final overlay dt is pretty close
> to an arbitrary choice.

Ok. Perhaps we could have a node in the root of the overlay for (2)
which we can use to lookup the aliases when resolving them.

/ {
	// Global metadata (1)
	compatible = "foo,whirlgig-widget";
	
	// Per-connector metadata (2)
	socket_a: socket@0 {
		compatible = "foo,widget-socket";
	};

	socket_b: socket@1 {
		compatible = "foo,widget-socket";
		realias {
			i2c-b = "i2c";
			intc-b = "intc";
			mmio-b = "mmio";
		};
	};

	// Fragments (3)
	fragment@0 {
		target-alias = "i2c";
		__overlay__ {

		};
	};
};

Then when we resolve aliases we would look through the set of sockets
and look for realias nodes to figure out which node to target. It may
also be better if we have a sockets container node and a fragments
container node to partition the two things.

> > > 
> > > I don't think we need any new constructs here.  If there are
> > > mismatches we can put dummy bridges with appropriate ranges properties
> > > on one side or the other.
> > > 
> > > The only thing I see that might want some help is that the connector
> > > type should certainly imply a specific set of cell widths for all the
> > > included buses.  So possibly we should supply some stuff to help
> > > enforce that.
> > 
> > I'm specifically thinking that anything that has a #<name>-cells
> > associated with it (#gpio-cells, #clock-cells, #power-domain-cells,
> > etc.) would need to have an associated <name>-map property and
> > associated parsing code so that we can make a consistent cell width for
> > the connector. If we have existing ways to make this work, e.g.
> > interrupt-map or ranges, then we don't need.
> 
> Yes, I agree.  All the "interrupt tree like" things need interrupt
> tree like nexus nodes, with a map and map-mask.
> 
> However, we also need some way of ensuring that the #address-cells and
> #size-cells for any buses we expose match properly.
> 
> Or maybe we need to invent something like a cross between 'ranges' and
> 'interrupt-map' that can remap ranges on one address space to the
> address space attached to a different node.

Ok. I can't think of any use case for remapping reg properties across
connectors. At least, for plug in boards we typically pass through i2c,
spi, uart, usb, dsi, hdmi, mi2s, etc. and I don't think we're changing
the reg properties for the nodes we would be placing on those busses.
Maybe PCIe is one case where this would matter though? I haven't thought
of how to handle that.

> > > > 4) Attempt to maintain almost all of the current overlay syntax with
> > > >    syntactic sugar
> > > 
> > > I'm not really sure what you mean by that.
> > > 
> > 
> > I mean that we're not really trying to change the general structure of
> > the DT overlay syntax. At least from what I can tell we're planning to
> > convert this /plugin/ format into a DT overlay that gets compiled into
> > binary all inside DTC.
> 
> Roughly speaking, yes.  However I think we should take the opportunity
> to address design flaws in the current overlay encoding as they come
> up (e.g. the fact that it uses phandles which *always* have to be
> resolved - better to directly use an alias / symbol / label).
> 

Alright. I was hoping to do some quick prototyping without changing DTC
to handle a different overlay encoding but that doesn't sound possible.