Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1470505 > unrolled thread

Re: DT connectors, thoughts

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: DT connectors, thoughts Stephen Boyd <stephen.boyd@linaro.org> - 2016-08-26 03:50 +0200
    Re: DT connectors, thoughts David Gibson <david@gibson.dropbear.id.au> - 2016-08-29 15:50 +0200
      Re: DT connectors, thoughts David Gibson <david@gibson.dropbear.id.au> - 2016-08-31 02:40 +0200
        Re: DT connectors, thoughts David Gibson <david@gibson.dropbear.id.au> - 2016-09-08 02:30 +0200

#1470505 — Re: DT connectors, thoughts

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.

[toc] | [next] | [standalone]

#1471859

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1472960

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1478692

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web