[RFC] binding for nvec mfd device

[RFC] binding for nvec mfd device

[Marc Dietrich]

Hi,

this is an RFC for an mfd device introduced here [1]. I didn't got much 
response, except from Stephen, but that bothered me enough already ;-)

I'm going describe the hw again shortly, for a more complete description, see 
[1]. What we have here is an EC which communicates to the host cpu (NVIDIA 
Tegra) via I2C. The unusual part is that the EC is I2C master while the host 
cpu is the slave. The EC has several ports to connect peripherals like 
keyboard, mouse, ... and also to control some system states (suspend, low bat, 
power key, ...). Ok, so much for the hardware description, now the binding. I 
think I integrated all comments made by Stephen in [2].

i2c-slave@7000c500 { 
	compatible = "nvidia,tegra20-i2c-slave", "simple-bus"; 
	reg = <0x7000c500 0x100>; 
	interrupts = <0 92 0x04>; 
	#address-cells = <1>; 
	#size-cells = <0>; 
	clock-frequency = <80000>; 
	clocks = <&tegra_car 67>, <&tegra_car 124>; 
	clock-names = "div-clk", "fast-clk"; 

	nvec@87 { 
		compatible = "nvidia,nvec", "simple-bus"; 
		reg = <0x87>;
		request-gpios = <&gpio 170 0>; /* gpio PV2 */ 

		keyboard { 
			compatible = "nvidia,nvec-keyboard";
			...
			<insert key mappings here>
			...
		}; 

		ps2 {
			compatible = "nvidia,nvec-ps2";
			packet-size = 6; /* for the ac100 tp */
		};

		events {
			compatible = "nvidia,nvec-events";
			...
			<add some system events here, 
				e.g. lid switch, power button>
			...
		};

		charger: ac {
			compatible = "nvidia,nvec-ac";
		};

		battery {
			compatible = "nvidia,nvec-battery";
			charger = <&charger>;
       };

		oem {
			compatible = "compal,nvec-paz00";
			/* ac100 specific extentions to the protocol */
		};
	}; 
}; 

For multi-slave capable SoCs, there can be more than one master connected to 
the i2c slave controller of the host cpu. Therefore I added "simple-bus" 
property here. Does "simple-bus" imply bus addresses? The EC registers with 
the slave controller by supplying the protocol (e.g. smbus or i2c) and an 
optional gpio.

I left out the binding description for now because the properties are pretty 
much self-describing. The current driver is in staging so everything is still 
in flux.

Marc


[1] http://www.mail-archive.com/devicetree-
discuss@lists.ozlabs.org/msg35636.html
[2] http://u-boot.10912.n7.nabble.com/PATCH-0-3-ARM-tegra-add-nvec-keyboard-
support-for-paz00-td159690.html

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

(Also Cc'ing the DT binding maintainers and hence quoting this in full)

On 07/27/2013 07:23 AM, Marc Dietrich wrote:
> Hi,
> 
> this is an RFC for an mfd device introduced here [1]. I didn't got much 
> response, except from Stephen, but that bothered me enough already ;-)
> 
> I'm going describe the hw again shortly, for a more complete description, see 
> [1]. What we have here is an EC which communicates to the host cpu (NVIDIA 
> Tegra) via I2C. The unusual part is that the EC is I2C master while the host 
> cpu is the slave. The EC has several ports to connect peripherals like 
> keyboard, mouse, ... and also to control some system states (suspend, low bat, 
> power key, ...). Ok, so much for the hardware description, now the binding. I 
> think I integrated all comments made by Stephen in [2].
> 
> i2c-slave@7000c500 { 
> 	compatible = "nvidia,tegra20-i2c-slave", "simple-bus";

Thinking more about this, I don't think simple-bus is appropriate here,
for the same reasons that i2c-master bindings don't specify simple-bus here.

"simple-bus" is usually used in a case where the node in question is
pretty much just a simple aggregation of some child nodes, and the child
nodes are the same "type" of node as the node that is the simple-bus.
For example, when grouping a variety of memory-mapped peripherals
together to provide some structure to a larger memory-mapped bus,
perhaps with a "ranges" property etc. to do some remapping. In this
case, the child nodes are basically independent of the simple-bus node,
just relying on it for a bit of DT parsing, but not at run-time.

In this case, the child nodes are a different type of object, and we
really need the driver for the i2c-slave node to know how to handle the
child nodes; we can't just go an instantiate them
directly/independently, and plug the devices into the bus that contains
the i2c-slave.

One question I have here: There is a single I2C HW module on Tegra that
can act either as a master or a slave. I don't think it can do both at
once. Should we:

a) Have a different compatible property for each use-case

b) Use the same compatible property (after all, the same HW module is
there in either case), yet have some kind of flag which tells the driver
whether to act as a master or slave. This flag could be standardized and
used by any binding for a "dual role" I2C controller.

I think it's reasonable to go with a separate compatible value in this
case, but I wonder if anyone wants to argue against it?

> 	reg = <0x7000c500 0x100>; 
> 	interrupts = <0 92 0x04>; 
> 	#address-cells = <1>; 
> 	#size-cells = <0>; 
> 	clock-frequency = <80000>; 
> 	clocks = <&tegra_car 67>, <&tegra_car 124>; 
> 	clock-names = "div-clk", "fast-clk"; 
> 
> 	nvec@87 { 
> 		compatible = "nvidia,nvec", "simple-bus"; 

Similarly here, I would drop simple-bus.

> 		reg = <0x87>;
> 		request-gpios = <&gpio 170 0>; /* gpio PV2 */ 

Nit: We should s/170/TEGRA_GPIO(V, 2)/ in the final *.dts file, and then
we can drop that comment.

> 		keyboard { 
> 			compatible = "nvidia,nvec-keyboard";
> 			...
> 			<insert key mappings here>
> 			...
> 		}; 
> 
> 		ps2 {
> 			compatible = "nvidia,nvec-ps2";
> 			packet-size = 6; /* for the ac100 tp */
> 		};
> 
> 		events {
> 			compatible = "nvidia,nvec-events";
> 			...
> 			<add some system events here, 
> 				e.g. lid switch, power button>
> 			...
> 		};
> 
> 		charger: ac {
> 			compatible = "nvidia,nvec-ac";
> 		};
> 
> 		battery {
> 			compatible = "nvidia,nvec-battery";
> 			charger = <&charger>;
>        	};
> 
> 		oem {
> 			compatible = "compal,nvec-paz00";
> 			/* ac100 specific extentions to the protocol */
> 		};
> 	}; 
> }; 

To me, this seems like a reasonable binding.

> For multi-slave capable SoCs, there can be more than one master connected to 
> the i2c slave controller of the host cpu. Therefore I added "simple-bus" 
> property here. Does "simple-bus" imply bus addresses?

This binding doesn't describe the external masters, so I'm not sure how
the presence or lack of them affects the binding at all. I argue against
using simple-bus above; the bus is not "simple".

> The EC registers with
> the slave controller by supplying the protocol (e.g. smbus or i2c) and an 
> optional gpio.

I don't think "the EC registers ...", but rather "the driver for the EC
protocol registers ..."

I don't think the optional GPIO would be provided to the I2C slave
controller in any way; handling of the GPIO is purely part of the NVEC
protocol driver, not the I2C slave driver.

> I left out the binding description for now because the properties are pretty 
> much self-describing. The current driver is in staging so everything is still 
> in flux.
> 
> Marc
> 
> [1] http://www.mail-archive.com/devicetree-
> discuss@lists.ozlabs.org/msg35636.html
> [2] http://u-boot.10912.n7.nabble.com/PATCH-0-3-ARM-tegra-add-nvec-keyboard-
> support-for-paz00-td159690.html

Re: [RFC] binding for nvec mfd device

[Mark Rutland]

On Mon, Jul 29, 2013 at 10:59:26PM +0100, Stephen Warren wrote:
> (Also Cc'ing the DT binding maintainers and hence quoting this in full)

Cheers.

> 
> On 07/27/2013 07:23 AM, Marc Dietrich wrote:
> > Hi,
> > 
> > this is an RFC for an mfd device introduced here [1]. I didn't got much 
> > response, except from Stephen, but that bothered me enough already ;-)
> > 
> > I'm going describe the hw again shortly, for a more complete description, see 
> > [1]. What we have here is an EC which communicates to the host cpu (NVIDIA 
> > Tegra) via I2C. The unusual part is that the EC is I2C master while the host 
> > cpu is the slave. The EC has several ports to connect peripherals like 
> > keyboard, mouse, ... and also to control some system states (suspend, low bat, 
> > power key, ...). Ok, so much for the hardware description, now the binding. I 
> > think I integrated all comments made by Stephen in [2].
> > 
> > i2c-slave@7000c500 { 
> > 	compatible = "nvidia,tegra20-i2c-slave", "simple-bus";
> 
> Thinking more about this, I don't think simple-bus is appropriate here,
> for the same reasons that i2c-master bindings don't specify simple-bus here.
> 
> "simple-bus" is usually used in a case where the node in question is
> pretty much just a simple aggregation of some child nodes, and the child
> nodes are the same "type" of node as the node that is the simple-bus.
> For example, when grouping a variety of memory-mapped peripherals
> together to provide some structure to a larger memory-mapped bus,
> perhaps with a "ranges" property etc. to do some remapping. In this
> case, the child nodes are basically independent of the simple-bus node,
> just relying on it for a bit of DT parsing, but not at run-time.

I'd expect any simple-bus node to be completely ignorable - if it
*requires* any configuration to child nodes to be usable, or child node
require any information from the bus (other than some addresss
translation via the ranges property), then the node is clearly not
compatible with the simple-bus concept.

If you need to instantiate child nodes as Linux platform devices, this
should be done in the driver for the bus. Relying on simple-bus to do
this is a hack.

> 
> In this case, the child nodes are a different type of object, and we
> really need the driver for the i2c-slave node to know how to handle the
> child nodes; we can't just go an instantiate them
> directly/independently, and plug the devices into the bus that contains
> the i2c-slave.

+1

> 
> One question I have here: There is a single I2C HW module on Tegra that
> can act either as a master or a slave. I don't think it can do both at
> once. Should we:

Is that configurable at run-time, or an integration decision?

> 
> a) Have a different compatible property for each use-case
> 
> b) Use the same compatible property (after all, the same HW module is
> there in either case), yet have some kind of flag which tells the driver
> whether to act as a master or slave. This flag could be standardized and
> used by any binding for a "dual role" I2C controller.
> 
> I think it's reasonable to go with a separate compatible value in this
> case, but I wonder if anyone wants to argue against it?

I believe a separate compatible string would make sense.

Thanks,
Mark.

> 
> > 	reg = <0x7000c500 0x100>; 
> > 	interrupts = <0 92 0x04>; 
> > 	#address-cells = <1>; 
> > 	#size-cells = <0>; 
> > 	clock-frequency = <80000>; 
> > 	clocks = <&tegra_car 67>, <&tegra_car 124>; 
> > 	clock-names = "div-clk", "fast-clk"; 
> > 
> > 	nvec@87 { 
> > 		compatible = "nvidia,nvec", "simple-bus"; 
> 
> Similarly here, I would drop simple-bus.
> 
> > 		reg = <0x87>;
> > 		request-gpios = <&gpio 170 0>; /* gpio PV2 */ 
> 
> Nit: We should s/170/TEGRA_GPIO(V, 2)/ in the final *.dts file, and then
> we can drop that comment.
> 
> > 		keyboard { 
> > 			compatible = "nvidia,nvec-keyboard";
> > 			...
> > 			<insert key mappings here>
> > 			...
> > 		}; 
> > 
> > 		ps2 {
> > 			compatible = "nvidia,nvec-ps2";
> > 			packet-size = 6; /* for the ac100 tp */
> > 		};
> > 
> > 		events {
> > 			compatible = "nvidia,nvec-events";
> > 			...
> > 			<add some system events here, 
> > 				e.g. lid switch, power button>
> > 			...
> > 		};
> > 
> > 		charger: ac {
> > 			compatible = "nvidia,nvec-ac";
> > 		};
> > 
> > 		battery {
> > 			compatible = "nvidia,nvec-battery";
> > 			charger = <&charger>;
> >        	};
> > 
> > 		oem {
> > 			compatible = "compal,nvec-paz00";
> > 			/* ac100 specific extentions to the protocol */
> > 		};
> > 	}; 
> > }; 
> 
> To me, this seems like a reasonable binding.
> 
> > For multi-slave capable SoCs, there can be more than one master connected to 
> > the i2c slave controller of the host cpu. Therefore I added "simple-bus" 
> > property here. Does "simple-bus" imply bus addresses?
> 
> This binding doesn't describe the external masters, so I'm not sure how
> the presence or lack of them affects the binding at all. I argue against
> using simple-bus above; the bus is not "simple".
> 
> > The EC registers with
> > the slave controller by supplying the protocol (e.g. smbus or i2c) and an 
> > optional gpio.
> 
> I don't think "the EC registers ...", but rather "the driver for the EC
> protocol registers ..."

I've lost some context here, but I hope this isn't a binding describing
driver behaviour... :)

Thanks,
Mark.

Re: [RFC] binding for nvec mfd device

[Marc Dietrich]

Am Mittwoch, 31. Juli 2013, 15:13:14 schrieb Mark Rutland:
> On Mon, Jul 29, 2013 at 10:59:26PM +0100, Stephen Warren wrote:
> > (Also Cc'ing the DT binding maintainers and hence quoting this in full)
> 
> Cheers.
> 
> > On 07/27/2013 07:23 AM, Marc Dietrich wrote:
> > > Hi,
> > > 
> > > this is an RFC for an mfd device introduced here [1]. I didn't got much
> > > response, except from Stephen, but that bothered me enough already ;-)
> > > 
> > > I'm going describe the hw again shortly, for a more complete
> > > description, see [1]. What we have here is an EC which communicates to
> > > the host cpu (NVIDIA Tegra) via I2C. The unusual part is that the EC is
> > > I2C master while the host cpu is the slave. The EC has several ports to
> > > connect peripherals like keyboard, mouse, ... and also to control some
> > > system states (suspend, low bat, power key, ...). Ok, so much for the
> > > hardware description, now the binding. I think I integrated all
> > > comments made by Stephen in [2].
> > > 
> > > i2c-slave@7000c500 {
> > > 
> > > 	compatible = "nvidia,tegra20-i2c-slave", "simple-bus";
> > 
> > Thinking more about this, I don't think simple-bus is appropriate here,
> > for the same reasons that i2c-master bindings don't specify simple-bus
> > here.
> > 
> > "simple-bus" is usually used in a case where the node in question is
> > pretty much just a simple aggregation of some child nodes, and the child
> > nodes are the same "type" of node as the node that is the simple-bus.
> > For example, when grouping a variety of memory-mapped peripherals
> > together to provide some structure to a larger memory-mapped bus,
> > perhaps with a "ranges" property etc. to do some remapping. In this
> > case, the child nodes are basically independent of the simple-bus node,
> > just relying on it for a bit of DT parsing, but not at run-time.
> 
> I'd expect any simple-bus node to be completely ignorable - if it
> *requires* any configuration to child nodes to be usable, or child node
> require any information from the bus (other than some addresss
> translation via the ranges property), then the node is clearly not
> compatible with the simple-bus concept.
> 
> If you need to instantiate child nodes as Linux platform devices, this
> should be done in the driver for the bus. Relying on simple-bus to do
> this is a hack.

ok, I'll rip this off from both nodes. I guess I should use 
of_platform_populate instead and keeping the compatible node in the childs - 
right?

> > In this case, the child nodes are a different type of object, and we
> > really need the driver for the i2c-slave node to know how to handle the
> > child nodes; we can't just go an instantiate them
> > directly/independently, and plug the devices into the bus that contains
> > the i2c-slave.
> 
> +1
> 
> > One question I have here: There is a single I2C HW module on Tegra that
> > can act either as a master or a slave. I don't think it can do both at
> 
> > once.

looking at the TRM, master and slave controllers are two hw blocks connected 
to the same bus (I wonder how they can share the same resources or the TRM 
block diagram is wrong). I don't know if both can act together at the same 
time, but I don't see what should speak against this (beside increase of 
entropy and caos). At least the isr can differentiate this by the irq status 
word. Maybe Stephen could ask internally about this.

> > Should we:
> Is that configurable at run-time, or an integration decision?
> 
> > a) Have a different compatible property for each use-case
> > 
> > b) Use the same compatible property (after all, the same HW module is
> > there in either case), yet have some kind of flag which tells the driver
> > whether to act as a master or slave. This flag could be standardized and
> > used by any binding for a "dual role" I2C controller.
> > 
> > I think it's reasonable to go with a separate compatible value in this
> > case, but I wonder if anyone wants to argue against it?
> 
> I believe a separate compatible string would make sense.

This will rule out a possible parallel operation of master and slave 
controllers on the same bus (I think an example is mentioned in the TRM as 
loopback mode). Personally, I also prefer a) because otherwise we would have 
to add slave support directly into the i2c master driver, which is likely the 
point I would hand over this work to someone at NVIDIA who understands more 
about the hw (and software in general) then I do.

--- Marc


> > > 	reg = <0x7000c500 0x100>;
> > > 	interrupts = <0 92 0x04>;
> > > 	#address-cells = <1>;
> > > 	#size-cells = <0>;
> > > 	clock-frequency = <80000>;
> > > 	clocks = <&tegra_car 67>, <&tegra_car 124>;
> > > 	clock-names = "div-clk", "fast-clk";
> > > 	
> > > 	nvec@87 {
> > > 	
> > > 		compatible = "nvidia,nvec", "simple-bus";
> > 
> > Similarly here, I would drop simple-bus.
> > 
> > > 		reg = <0x87>;
> > > 		request-gpios = <&gpio 170 0>; /* gpio PV2 */
> > 
> > Nit: We should s/170/TEGRA_GPIO(V, 2)/ in the final *.dts file, and then
> > we can drop that comment.
> > 
> > > 		keyboard {
> > > 		
> > > 			compatible = "nvidia,nvec-keyboard";
> > > 			...
> > > 			<insert key mappings here>
> > > 			...
> > > 		
> > > 		};
> > > 		
> > > 		ps2 {
> > > 		
> > > 			compatible = "nvidia,nvec-ps2";
> > > 			packet-size = 6; /* for the ac100 tp */
> > > 		
> > > 		};
> > > 		
> > > 		events {
> > > 		
> > > 			compatible = "nvidia,nvec-events";
> > > 			...
> > > 			<add some system events here,
> > > 			
> > > 				e.g. lid switch, power button>
> > > 			
> > > 			...
> > > 		
> > > 		};
> > > 		
> > > 		charger: ac {
> > > 		
> > > 			compatible = "nvidia,nvec-ac";
> > > 		
> > > 		};
> > > 		
> > > 		battery {
> > > 		
> > > 			compatible = "nvidia,nvec-battery";
> > > 			charger = <&charger>;
> > > 			
> > >        	};
> > > 		
> > > 		oem {
> > > 		
> > > 			compatible = "compal,nvec-paz00";
> > > 			/* ac100 specific extentions to the protocol */
> > > 		
> > > 		};
> > > 	
> > > 	};
> > > 
> > > };
> > 
> > To me, this seems like a reasonable binding.
> > 
> > > For multi-slave capable SoCs, there can be more than one master
> > > connected to the i2c slave controller of the host cpu. Therefore I
> > > added "simple-bus" property here. Does "simple-bus" imply bus
> > > addresses?
> > 
> > This binding doesn't describe the external masters, so I'm not sure how
> > the presence or lack of them affects the binding at all. I argue against
> > using simple-bus above; the bus is not "simple".
> > 
> > > The EC registers with
> > > the slave controller by supplying the protocol (e.g. smbus or i2c) and
> > > an
> > > optional gpio.
> > 
> > I don't think "the EC registers ...", but rather "the driver for the EC
> > protocol registers ..."
> 
> I've lost some context here, but I hope this isn't a binding describing
> driver behaviour... :)
> 
> Thanks,
> Mark.

Re: [RFC] binding for nvec mfd device

[Mark Rutland]

On Wed, Jul 31, 2013 at 03:57:13PM +0100, Marc Dietrich wrote:
> Am Mittwoch, 31. Juli 2013, 15:13:14 schrieb Mark Rutland:
> > On Mon, Jul 29, 2013 at 10:59:26PM +0100, Stephen Warren wrote:
> > > (Also Cc'ing the DT binding maintainers and hence quoting this in full)
> > 
> > Cheers.
> > 
> > > On 07/27/2013 07:23 AM, Marc Dietrich wrote:
> > > > Hi,
> > > > 
> > > > this is an RFC for an mfd device introduced here [1]. I didn't got much
> > > > response, except from Stephen, but that bothered me enough already ;-)
> > > > 
> > > > I'm going describe the hw again shortly, for a more complete
> > > > description, see [1]. What we have here is an EC which communicates to
> > > > the host cpu (NVIDIA Tegra) via I2C. The unusual part is that the EC is
> > > > I2C master while the host cpu is the slave. The EC has several ports to
> > > > connect peripherals like keyboard, mouse, ... and also to control some
> > > > system states (suspend, low bat, power key, ...). Ok, so much for the
> > > > hardware description, now the binding. I think I integrated all
> > > > comments made by Stephen in [2].
> > > > 
> > > > i2c-slave@7000c500 {
> > > > 
> > > > 	compatible = "nvidia,tegra20-i2c-slave", "simple-bus";
> > > 
> > > Thinking more about this, I don't think simple-bus is appropriate here,
> > > for the same reasons that i2c-master bindings don't specify simple-bus
> > > here.
> > > 
> > > "simple-bus" is usually used in a case where the node in question is
> > > pretty much just a simple aggregation of some child nodes, and the child
> > > nodes are the same "type" of node as the node that is the simple-bus.
> > > For example, when grouping a variety of memory-mapped peripherals
> > > together to provide some structure to a larger memory-mapped bus,
> > > perhaps with a "ranges" property etc. to do some remapping. In this
> > > case, the child nodes are basically independent of the simple-bus node,
> > > just relying on it for a bit of DT parsing, but not at run-time.
> > 
> > I'd expect any simple-bus node to be completely ignorable - if it
> > *requires* any configuration to child nodes to be usable, or child node
> > require any information from the bus (other than some addresss
> > translation via the ranges property), then the node is clearly not
> > compatible with the simple-bus concept.
> > 
> > If you need to instantiate child nodes as Linux platform devices, this
> > should be done in the driver for the bus. Relying on simple-bus to do
> > this is a hack.
> 
> ok, I'll rip this off from both nodes. I guess I should use 
> of_platform_populate instead and keeping the compatible node in the childs - 
> right?

I believe that would be a correct way of handling this.

Thanks,
Mark.

Re: [RFC] binding for nvec mfd device

[Marc Dietrich]

On Wednesday 31 July 2013 16:33:22 Mark Rutland wrote:
> On Wed, Jul 31, 2013 at 03:57:13PM +0100, Marc Dietrich wrote:
> > Am Mittwoch, 31. Juli 2013, 15:13:14 schrieb Mark Rutland:
> > > On Mon, Jul 29, 2013 at 10:59:26PM +0100, Stephen Warren wrote:
> > > > (Also Cc'ing the DT binding maintainers and hence quoting this in
> > > > full)
> > > 
> > > Cheers.
> > > 
> > > > On 07/27/2013 07:23 AM, Marc Dietrich wrote:
> > > > > Hi,
> > > > > 
> > > > > this is an RFC for an mfd device introduced here [1]. I didn't got
> > > > > much
> > > > > response, except from Stephen, but that bothered me enough already
> > > > > ;-)
> > > > > 
> > > > > I'm going describe the hw again shortly, for a more complete
> > > > > description, see [1]. What we have here is an EC which communicates
> > > > > to
> > > > > the host cpu (NVIDIA Tegra) via I2C. The unusual part is that the EC
> > > > > is
> > > > > I2C master while the host cpu is the slave. The EC has several ports
> > > > > to
> > > > > connect peripherals like keyboard, mouse, ... and also to control
> > > > > some
> > > > > system states (suspend, low bat, power key, ...). Ok, so much for
> > > > > the
> > > > > hardware description, now the binding. I think I integrated all
> > > > > comments made by Stephen in [2].
> > > > > 
> > > > > i2c-slave@7000c500 {
> > > > > 
> > > > > 	compatible = "nvidia,tegra20-i2c-slave", "simple-bus";
> > > > 
> > > > Thinking more about this, I don't think simple-bus is appropriate
> > > > here,
> > > > for the same reasons that i2c-master bindings don't specify simple-bus
> > > > here.
> > > > 
> > > > "simple-bus" is usually used in a case where the node in question is
> > > > pretty much just a simple aggregation of some child nodes, and the
> > > > child
> > > > nodes are the same "type" of node as the node that is the simple-bus.
> > > > For example, when grouping a variety of memory-mapped peripherals
> > > > together to provide some structure to a larger memory-mapped bus,
> > > > perhaps with a "ranges" property etc. to do some remapping. In this
> > > > case, the child nodes are basically independent of the simple-bus
> > > > node,
> > > > just relying on it for a bit of DT parsing, but not at run-time.
> > > 
> > > I'd expect any simple-bus node to be completely ignorable - if it
> > > *requires* any configuration to child nodes to be usable, or child node
> > > require any information from the bus (other than some addresss
> > > translation via the ranges property), then the node is clearly not
> > > compatible with the simple-bus concept.
> > > 
> > > If you need to instantiate child nodes as Linux platform devices, this
> > > should be done in the driver for the bus. Relying on simple-bus to do
> > > this is a hack.
> > 
> > ok, I'll rip this off from both nodes. I guess I should use
> > of_platform_populate instead and keeping the compatible node in the childs
> > - right?
> 
> I believe that would be a correct way of handling this.

Great. Thinking about a really generic dt representation of an i2c bus (with 
multi-slave, slave and master controllers can act in parallel), something like 
this came into my mind:


i2c@xxxxxxx {
		compatible = "nvidia,tegra-i2c", "simple-bus"; /* This time it is 
truly a simple bus, because there is no higher control instance */

		... common master/slave resources ...

		master {
				compatible = "nvidia,tegra-i2c-master";
				... resources ??? ....

			 	client1@xx {
						...
				};

			 	client2@xx {
						...
				};

				...
		}

		slave1@xxx {
				compatible = "nvidia,tegra-i2c-slave";
				... resources ??? ...

				client1@xx {
						...
				};
		};

		slave2@xxx {
				compatible = "nvidia,tegra-i2c-slave";
				... resources ??? ...

				client1@xx {
						...
				};

		};

		....
};

Marc

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

On 07/31/2013 08:13 AM, Mark Rutland wrote:
> On Mon, Jul 29, 2013 at 10:59:26PM +0100, Stephen Warren wrote:
>> On 07/27/2013 07:23 AM, Marc Dietrich wrote:
>>> Hi,
>>>
>>> this is an RFC for an mfd device introduced here [1]. I didn't got much 
>>> response, except from Stephen, but that bothered me enough already ;-)

... [propose binding for Tegra I2C slave controller, and NVEC protocol
components]

>> One question I have here: There is a single I2C HW module on Tegra that
>> can act either as a master or a slave. I don't think it can do both at
>> once. Should we: ...
> 
> Is that configurable at run-time, or an integration decision?

It's certainly a run-time decision whether to operate the HW in master
or slave mode, and this is independent from anything on the board,
except of course for whether it actually makes sense to use master or
slave mode purely based on which external components are hooked up to
the bus.

I imagine that at least by appropriately resetting or reconfiguring it,
you could switch roles at run-time too. I don't know if you can actually
actively use both the master and slave portions of the HW at the exact
same time independently though. Marc is right that the diagrams in the
documentation certainly imply you can use both at the same time,
although I'm not totally convinced looking at the registers. I'm CCing
Laxman to answer that question, since he knows the HW better than I.

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

On 07/31/2013 10:52 AM, Marc Dietrich wrote:
> On Wednesday 31 July 2013 16:33:22 Mark Rutland wrote:
>> On Wed, Jul 31, 2013 at 03:57:13PM +0100, Marc Dietrich wrote:
>>> Am Mittwoch, 31. Juli 2013, 15:13:14 schrieb Mark Rutland:
...
>>>> If you need to instantiate child nodes as Linux platform devices, this
>>>> should be done in the driver for the bus. Relying on simple-bus to do
>>>> this is a hack.
>>>
>>> ok, I'll rip this off from both nodes. I guess I should use
>>> of_platform_populate instead and keeping the compatible node in the childs
>>> - right?
>>
>> I believe that would be a correct way of handling this.
> 
> Great. Thinking about a really generic dt representation of an i2c bus (with 
> multi-slave, slave and master controllers can act in parallel), something like 
> this came into my mind:
> 
> 
> i2c@xxxxxxx {
> 		compatible = "nvidia,tegra-i2c", "simple-bus"; /* This time it is 
> truly a simple bus, because there is no higher control instance */
> 
> 		... common master/slave resources ...
> 
> 		master {
> 				compatible = "nvidia,tegra-i2c-master";
> 				... resources ??? ....
> 
> 			 	client1@xx {
> 						...
> 				};
> 
> 			 	client2@xx {
> 						...
> 				};
> 
> 				...
> 		}
> 
> 		slave1@xxx {
> 				compatible = "nvidia,tegra-i2c-slave";
> 				... resources ??? ...
> 
> 				client1@xx {
> 						...
> 				};
> 		};
> 
> 		slave2@xxx {
> 				compatible = "nvidia,tegra-i2c-slave";
> 				... resources ??? ...
> 
> 				client1@xx {
> 						...
> 				};
> 
> 		};
> 
> 		....
> };

The generic I2C bindings already define that the other chips on the I2C
bus appear directly underneath the I2C controller's DT node. Perhaps it
isn't a big issue to change that, since each I2C controller can define
the layout of its own node?

Anyway, we can probably get away without introducing multiple levels by
adding some more bits or cells into the reg address for I2C child nodes:

i2c@xxxxxxxx {
	compatible = "nvidia,tegra20-i2c";
	... resources
	#address-cells = <2>;

	codec {
		// 0 means external slave, 0x1c is slave's address
		reg = <0 0x1c>;
		...
	};

	tegraslave {
		// 0 means internal slave, 0x80 is controller's address
		reg = <1 0x80>;
		...
	};
};

... where each of those child nodes could be repeated N times. We could
also or in 0x80000000 to the reg values in the child nodes rather than
using a separate cell if we wanted.

Re: [RFC] binding for nvec mfd device

[Marc Dietrich]

Hi Stephen,

Am Mittwoch, 31. Juli 2013, 15:03:14 schrieb Stephen Warren:
> The generic I2C bindings already define that the other chips on the I2C
> bus appear directly underneath the I2C controller's DT node. Perhaps it
> isn't a big issue to change that, since each I2C controller can define
> the layout of its own node?
> 
> Anyway, we can probably get away without introducing multiple levels by
> adding some more bits or cells into the reg address for I2C child nodes:
> 
> i2c@xxxxxxxx {
> 	compatible = "nvidia,tegra20-i2c";
> 	... resources
> 	#address-cells = <2>;
> 
> 	codec {
> 		// 0 means external slave, 0x1c is slave's address
> 		reg = <0 0x1c>;
> 		...
> 	};
> 
> 	tegraslave {
> 		// 0 means internal slave, 0x80 is controller's address
> 		reg = <1 0x80>;
> 		...
> 	};
> };
> 
> ... where each of those child nodes could be repeated N times. We could
> also or in 0x80000000 to the reg values in the child nodes rather than
> using a separate cell if we wanted.

thinking a little more about this, this is way to complicated. "Master" and 
"Slave" functions are properties of the same i2c controller. Therefore, just 
adding a small property to the i2c controller node saying "enable slave 
support" is sufficient, as all the resources are shared (which is another good 
argument that it is the same device, hence same node). I would just add the 
slave i2c address, which is all the slave driver needs, e.g.
"slave-address = <8c>;" to enable slave mode operation. Both modes can be 
enabled at the same time (for loopback comm). AFAIK, if only the slave part is 
used, the master part shouldn't hurt if programmed (same as other way around).

The difficult part is how to specify the master if only the slave is used. As 
the master is naturally on the same bus as the slave, it should be a child 
node of the controller. Therefore it needs a special i2c address, maybe 
0x80000000 as you mentioned above, but this will be rejected by the i2c core. 
Instantiating the nvec as an i2c client could be another problem.

It looks like that the i2c subsystem needs some modifications to allow to 
specify a master device which is attached to a slave controller. Maybe someone 
from the i2c folks (cc'ed) can comment on this?

Marc

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

On 09/17/2013 01:53 AM, Marc Dietrich wrote:
> Hi Stephen,
> 
> Am Mittwoch, 31. Juli 2013, 15:03:14 schrieb Stephen Warren:
>> The generic I2C bindings already define that the other chips on the I2C
>> bus appear directly underneath the I2C controller's DT node. Perhaps it
>> isn't a big issue to change that, since each I2C controller can define
>> the layout of its own node?
>>
>> Anyway, we can probably get away without introducing multiple levels by
>> adding some more bits or cells into the reg address for I2C child nodes:
>>
>> i2c@xxxxxxxx {
>> 	compatible = "nvidia,tegra20-i2c";
>> 	... resources
>> 	#address-cells = <2>;
>>
>> 	codec {
>> 		// 0 means external slave, 0x1c is slave's address
>> 		reg = <0 0x1c>;
>> 		...
>> 	};
>>
>> 	tegraslave {
>> 		// 0 means internal slave, 0x80 is controller's address
>> 		reg = <1 0x80>;
>> 		...
>> 	};
>> };
>>
>> ... where each of those child nodes could be repeated N times. We could
>> also or in 0x80000000 to the reg values in the child nodes rather than
>> using a separate cell if we wanted.
> 
> thinking a little more about this, this is way to complicated.

Really, this seems extremely simple to me.

> "Master" and 
> "Slave" functions are properties of the same i2c controller. Therefore, just 
> adding a small property to the i2c controller node saying "enable slave 
> support" is sufficient, as all the resources are shared (which is another good 
> argument that it is the same device, hence same node). I would just add the 
> slave i2c address, which is all the slave driver needs, e.g.

Perhaps so for this one controller, but we should strive to create a
binding style that can work with any I2C controller that can be master
or slave. For a general binding, I think we need to support multiple
slave addresses. The style above seems to support that with little
complexity.

> "slave-address = <8c>;" to enable slave mode operation. Both modes can be 
> enabled at the same time (for loopback comm). AFAIK, if only the slave part is 
> used, the master part shouldn't hurt if programmed (same as other way around).
> 
> The difficult part is how to specify the master if only the slave is used. As 
> the master is naturally on the same bus as the slave, it should be a child 
> node of the controller. Therefore it needs a special i2c address, maybe 

Why would the external I2C master be represented in the DT bindings at
all? It's not really a SW-accessible entity; its presence is only
observable not something you can initiate interaction with.

I assume you're not talking about the Tegra I2C controller when you say
"master" above; that is already represented in the DT as the top-level
I2C node.

> 0x80000000 as you mentioned above, but this will be rejected by the i2c core. 
> Instantiating the nvec as an i2c client could be another problem.
...
> It looks like that the i2c subsystem needs some modifications to allow to 
> specify a master device which is attached to a slave controller. Maybe someone 
> from the i2c folks (cc'ed) can comment on this?

Yes, you certainly will need some enhancements to the I2C core to
support slave mode. I suggest that I2C drivers gain an .of_xlate
callback which translates child node DT reg values (addresses) into the
(master-vs-slave, I2C address) pair.

Re: [RFC] binding for nvec mfd device

[Wolfram Sang]

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


> It looks like that the i2c subsystem needs some modifications to allow to 
> specify a master device which is attached to a slave controller. Maybe someone 
> from the i2c folks (cc'ed) can comment on this?

The linux i2c core does not really support controllers being a slave.
There are probably some more obstacles on the way if you want to do
this, depends on what you want to do.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] binding for nvec mfd device

[Marc Dietrich]

Am Dienstag, 17. September 2013, 15:48:12 schrieb Stephen Warren:
> On 09/17/2013 01:53 AM, Marc Dietrich wrote:
> > Hi Stephen,
> > 
> > Am Mittwoch, 31. Juli 2013, 15:03:14 schrieb Stephen Warren:
> >> The generic I2C bindings already define that the other chips on the I2C
> >> bus appear directly underneath the I2C controller's DT node. Perhaps it
> >> isn't a big issue to change that, since each I2C controller can define
> >> the layout of its own node?
> >> 
> >> Anyway, we can probably get away without introducing multiple levels by
> >> adding some more bits or cells into the reg address for I2C child nodes:
> >> 
> >> i2c@xxxxxxxx {
> >> 
> >> 	compatible = "nvidia,tegra20-i2c";
> >> 	... resources
> >> 	#address-cells = <2>;
> >> 	
> >> 	codec {
> >> 	
> >> 		// 0 means external slave, 0x1c is slave's address
> >> 		reg = <0 0x1c>;
> >> 		...
> >> 	
> >> 	};
> >> 	
> >> 	tegraslave {
> >> 	
> >> 		// 0 means internal slave, 0x80 is controller's address
> >> 		reg = <1 0x80>;
> >> 		...
> >> 	
> >> 	};
> >> 
> >> };
> >> 
> >> ... where each of those child nodes could be repeated N times. We could
> >> also or in 0x80000000 to the reg values in the child nodes rather than
> >> using a separate cell if we wanted.
> > 
> > thinking a little more about this, this is way to complicated.
> 
> Really, this seems extremely simple to me.
> 
> > "Master" and
> > "Slave" functions are properties of the same i2c controller. Therefore,
> > just adding a small property to the i2c controller node saying "enable
> > slave support" is sufficient, as all the resources are shared (which is
> > another good argument that it is the same device, hence same node). I
> > would just add the slave i2c address, which is all the slave driver
> > needs, e.g.
> 
> Perhaps so for this one controller, but we should strive to create a
> binding style that can work with any I2C controller that can be master
> or slave. For a general binding, I think we need to support multiple
> slave addresses. The style above seems to support that with little
> complexity.

I have no idea how other implementations may look like. The downstream kernel 
seems to program the slave address in a secondary step after the controller is 
initialized [1]. So every "client" which binds to the slave driver can use its 
own address, but only one client at the same time is allowed. As said before, 
the may also exist devices (!tegra) with multiple slave addresses at the same 
time.

Your approach above seems fulfill these properties (what about a single slave 
controller without a master?), but it will be tricky for the slave to find the 
controller resources it needs. I'm thinking of an insane (but valid) 
configuration where all i2c ports of the SoC are connected together and one 
port plays the master and all others are slaves. In this case we need to add a 
property to the slave node which points to his controller instance.

> > "slave-address = <8c>;" to enable slave mode operation. Both modes can be
> > enabled at the same time (for loopback comm). AFAIK, if only the slave
> > part is used, the master part shouldn't hurt if programmed (same as other
> > way around).
> > 
> > The difficult part is how to specify the master if only the slave is used.
> > As the master is naturally on the same bus as the slave, it should be a
> > child node of the controller. Therefore it needs a special i2c address,
> > maybe
> Why would the external I2C master be represented in the DT bindings at
> all? It's not really a SW-accessible entity; its presence is only
> observable not something you can initiate interaction with.

No, we can and nvec does it regulary using a "side"-channel (a tegra gpio 
connected to a master irq input). This side-channel is non standard and each 
vendor may do it differently. But you are right, we don't need an entry for 
the master, but an entry for the driver (the other side of the communication) 
which binds to the slave controller.

> I assume you're not talking about the Tegra I2C controller when you say
> "master" above; that is already represented in the DT as the top-level
> I2C node.
> 
> > 0x80000000 as you mentioned above, but this will be rejected by the i2c
> > core. Instantiating the nvec as an i2c client could be another problem.
> 
> ...
> 
> > It looks like that the i2c subsystem needs some modifications to allow to
> > specify a master device which is attached to a slave controller. Maybe
> > someone from the i2c folks (cc'ed) can comment on this?
> 
> Yes, you certainly will need some enhancements to the I2C core to
> support slave mode. I suggest that I2C drivers gain an .of_xlate
> callback which translates child node DT reg values (addresses) into the
> (master-vs-slave, I2C address) pair.

I somehow feared that :-( Now my free time became rather limited recently and 
I don't feel that I'm the best person to solve these problem(s). Andrey said 
that he may be interested finishing at least a proper device tree 
representation (especially to get his great nvec uboot support upstreamed). 
Kernel support is a different beast and I really hope that the code which 
lives in staging area currently is not doomed because integration with the i2c 
subsystem is too complex.

Marc


[1] http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=blob;f=drivers/i2c/i2c-slave.c;h=280a860cd2e84e15b2c1aef0e938d750f56863dd;hb=rel-roth-ota-1#l120

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

On 09/23/2013 08:52 AM, Marc Dietrich wrote:
> Am Dienstag, 17. September 2013, 15:48:12 schrieb Stephen Warren:
>> On 09/17/2013 01:53 AM, Marc Dietrich wrote:
>>> Hi Stephen,
>>>
>>> Am Mittwoch, 31. Juli 2013, 15:03:14 schrieb Stephen Warren:
>>>> The generic I2C bindings already define that the other chips on the I2C
>>>> bus appear directly underneath the I2C controller's DT node. Perhaps it
>>>> isn't a big issue to change that, since each I2C controller can define
>>>> the layout of its own node?
>>>>
>>>> Anyway, we can probably get away without introducing multiple levels by
>>>> adding some more bits or cells into the reg address for I2C child nodes:
>>>>
>>>> i2c@xxxxxxxx {
>>>>
>>>> 	compatible = "nvidia,tegra20-i2c";
>>>> 	... resources
>>>> 	#address-cells = <2>;
>>>> 	
>>>> 	codec {
>>>> 	
>>>> 		// 0 means external slave, 0x1c is slave's address
>>>> 		reg = <0 0x1c>;
>>>> 		...
>>>> 	
>>>> 	};
>>>> 	
>>>> 	tegraslave {
>>>> 	
>>>> 		// 0 means internal slave, 0x80 is controller's address
>>>> 		reg = <1 0x80>;
>>>> 		...
>>>> 	
>>>> 	};
>>>>
>>>> };
>>>>
>>>> ... where each of those child nodes could be repeated N times. We could
>>>> also or in 0x80000000 to the reg values in the child nodes rather than
>>>> using a separate cell if we wanted.
>>>
>>> thinking a little more about this, this is way to complicated.
>>
>> Really, this seems extremely simple to me.
>>
>>> "Master" and
>>> "Slave" functions are properties of the same i2c controller. Therefore,
>>> just adding a small property to the i2c controller node saying "enable
>>> slave support" is sufficient, as all the resources are shared (which is
>>> another good argument that it is the same device, hence same node). I
>>> would just add the slave i2c address, which is all the slave driver
>>> needs, e.g.
>>
>> Perhaps so for this one controller, but we should strive to create a
>> binding style that can work with any I2C controller that can be master
>> or slave. For a general binding, I think we need to support multiple
>> slave addresses. The style above seems to support that with little
>> complexity.
> 
> I have no idea how other implementations may look like. The downstream kernel 
> seems to program the slave address in a secondary step after the controller is 
> initialized [1]. So every "client" which binds to the slave driver can use its 
> own address, but only one client at the same time is allowed. As said before, 
> the may also exist devices (!tegra) with multiple slave addresses at the same 
> time.
> 
> Your approach above seems fulfill these properties (what about a single slave 
> controller without a master?),

There would simply be a node for the controller, and a single child node
for the slave setup, and no child nodes for mastered devices.

> but it will be tricky for the slave to find the
> controller resources it needs.

I don't understand this. The resources are only needed by I2C driver,
and all come from the I2C driver node. If the I2C driver ends up being
split into separate master/slave portions, that information can easily
be passed from the main driver probe()/... to those other sub-parts of
the driver.

> I'm thinking of an insane (but valid)
> configuration where all i2c ports of the SoC are connected together and one 
> port plays the master and all others are slaves. In this case we need to add a 
> property to the slave node which points to his controller instance.

Why do the slave nodes care where they're mastered from?

I think you'd just have the following

/* master */
i2c@xxxxx {
    foo@0x40 {
        reg = <MASTER 0x40>;
        compatible = "nvidia,nvec";
    }
};

i2c@yyyy {
    foo@40 {
        reg = <SLAVE 0x40>;
        compatible = "nvidia,nvec-slave";
    }
};

There's no need for the slave child node to know that it is mastered
from the Tegra I2C controller; all it cares about is that there is some
I2C bus that it needs to respond to transactions upon.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] binding for nvec mfd device

[Andrey Danin]

On Mon, 2013-09-23 at 10:36 -0600, Stephen Warren wrote:
> On 09/23/2013 08:52 AM, Marc Dietrich wrote:
> > Am Dienstag, 17. September 2013, 15:48:12 schrieb Stephen Warren:
> >> On 09/17/2013 01:53 AM, Marc Dietrich wrote:
> >>> Hi Stephen,
> >>>
> >>> Am Mittwoch, 31. Juli 2013, 15:03:14 schrieb Stephen Warren:
> >>>> The generic I2C bindings already define that the other chips on the I2C
> >>>> bus appear directly underneath the I2C controller's DT node. Perhaps it
> >>>> isn't a big issue to change that, since each I2C controller can define
> >>>> the layout of its own node?
> >>>>
> >>>> Anyway, we can probably get away without introducing multiple levels by
> >>>> adding some more bits or cells into the reg address for I2C child nodes:
> >>>>
> >>>> i2c@xxxxxxxx {
> >>>>
> >>>> 	compatible = "nvidia,tegra20-i2c";
> >>>> 	... resources
> >>>> 	#address-cells = <2>;
> >>>> 	
> >>>> 	codec {
> >>>> 	
> >>>> 		// 0 means external slave, 0x1c is slave's address
> >>>> 		reg = <0 0x1c>;
> >>>> 		...
> >>>> 	
> >>>> 	};
> >>>> 	
> >>>> 	tegraslave {
> >>>> 	
> >>>> 		// 0 means internal slave, 0x80 is controller's address
> >>>> 		reg = <1 0x80>;
> >>>> 		...
> >>>> 	
> >>>> 	};
> >>>>
> >>>> };
> >>>>
> >>>> ... where each of those child nodes could be repeated N times. We could
> >>>> also or in 0x80000000 to the reg values in the child nodes rather than
> >>>> using a separate cell if we wanted.
> >>>
> >>> thinking a little more about this, this is way to complicated.
> >>
> >> Really, this seems extremely simple to me.
> >>
> >>> "Master" and
> >>> "Slave" functions are properties of the same i2c controller. Therefore,
> >>> just adding a small property to the i2c controller node saying "enable
> >>> slave support" is sufficient, as all the resources are shared (which is
> >>> another good argument that it is the same device, hence same node). I
> >>> would just add the slave i2c address, which is all the slave driver
> >>> needs, e.g.
> >>
> >> Perhaps so for this one controller, but we should strive to create a
> >> binding style that can work with any I2C controller that can be master
> >> or slave. For a general binding, I think we need to support multiple
> >> slave addresses. The style above seems to support that with little
> >> complexity.
> > 
> > I have no idea how other implementations may look like. The downstream kernel 
> > seems to program the slave address in a secondary step after the controller is 
> > initialized [1]. So every "client" which binds to the slave driver can use its 
> > own address, but only one client at the same time is allowed. As said before, 
> > the may also exist devices (!tegra) with multiple slave addresses at the same 
> > time.
> > 
> > Your approach above seems fulfill these properties (what about a single slave 
> > controller without a master?),
> 
> There would simply be a node for the controller, and a single child node
> for the slave setup, and no child nodes for mastered devices.
> 
> > but it will be tricky for the slave to find the
> > controller resources it needs.
> 
> I don't understand this. The resources are only needed by I2C driver,
> and all come from the I2C driver node. If the I2C driver ends up being
> split into separate master/slave portions, that information can easily
> be passed from the main driver probe()/... to those other sub-parts of
> the driver.
> 
> > I'm thinking of an insane (but valid)
> > configuration where all i2c ports of the SoC are connected together and one 
> > port plays the master and all others are slaves. In this case we need to add a 
> > property to the slave node which points to his controller instance.
> 
> Why do the slave nodes care where they're mastered from?
> 
> I think you'd just have the following
> 
> /* master */
> i2c@xxxxx {
>     foo@0x40 {
>         reg = <MASTER 0x40>;
>         compatible = "nvidia,nvec";
>     }
> };
> 
> i2c@yyyy {
>     foo@40 {
>         reg = <SLAVE 0x40>;
>         compatible = "nvidia,nvec-slave";
>     }
> };
> 
> There's no need for the slave child node to know that it is mastered
> from the Tegra I2C controller; all it cares about is that there is some
> I2C bus that it needs to respond to transactions upon.
> 
> 
This binding describes only case, when I2C device are connected to I2C
controller.

Assume that I2C controller #1 (@xxxxx), I2C controller #2 (@yyyy), and
nvec I2C master device are connected to same bus.
How dt must be composed in this case ? Must i2c@xxxxx and i2c@yyyy be in
parent/child relation (in terms of dt) ?


p.s. sorry, prev message was not formatted correctly, so I resend it.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] binding for nvec mfd device

[Marc Dietrich]

Am Dienstag, 24. September 2013, 11:39:21 schrieb Andrey Danin:
> On Mon, 2013-09-23 at 10:36 -0600, Stephen Warren wrote:
> > On 09/23/2013 08:52 AM, Marc Dietrich wrote:
> > > Am Dienstag, 17. September 2013, 15:48:12 schrieb Stephen Warren:
> > >> On 09/17/2013 01:53 AM, Marc Dietrich wrote:
> > >>> Hi Stephen,
> > >>> 
> > >>> Am Mittwoch, 31. Juli 2013, 15:03:14 schrieb Stephen Warren:
> > >>>> The generic I2C bindings already define that the other chips on the
> > >>>> I2C
> > >>>> bus appear directly underneath the I2C controller's DT node. Perhaps
> > >>>> it
> > >>>> isn't a big issue to change that, since each I2C controller can
> > >>>> define
> > >>>> the layout of its own node?
> > >>>> 
> > >>>> Anyway, we can probably get away without introducing multiple levels
> > >>>> by
> > >>>> adding some more bits or cells into the reg address for I2C child
> > >>>> nodes:
> > >>>> 
> > >>>> i2c@xxxxxxxx {
> > >>>> 
> > >>>> 	compatible = "nvidia,tegra20-i2c";
> > >>>> 	... resources
> > >>>> 	#address-cells = <2>;
> > >>>> 	
> > >>>> 	codec {
> > >>>> 	
> > >>>> 		// 0 means external slave, 0x1c is slave's address
> > >>>> 		reg = <0 0x1c>;
> > >>>> 		...
> > >>>> 	
> > >>>> 	};
> > >>>> 	
> > >>>> 	tegraslave {
> > >>>> 	
> > >>>> 		// 0 means internal slave, 0x80 is controller's address
> > >>>> 		reg = <1 0x80>;
> > >>>> 		...
> > >>>> 	
> > >>>> 	};
> > >>>> 
> > >>>> };
> > >>>> 
> > >>>> ... where each of those child nodes could be repeated N times. We
> > >>>> could
> > >>>> also or in 0x80000000 to the reg values in the child nodes rather
> > >>>> than
> > >>>> using a separate cell if we wanted.
> > >>> 
> > >>> thinking a little more about this, this is way to complicated.
> > >> 
> > >> Really, this seems extremely simple to me.
> > >> 
> > >>> "Master" and
> > >>> "Slave" functions are properties of the same i2c controller.
> > >>> Therefore,
> > >>> just adding a small property to the i2c controller node saying "enable
> > >>> slave support" is sufficient, as all the resources are shared (which
> > >>> is
> > >>> another good argument that it is the same device, hence same node). I
> > >>> would just add the slave i2c address, which is all the slave driver
> > >>> needs, e.g.
> > >> 
> > >> Perhaps so for this one controller, but we should strive to create a
> > >> binding style that can work with any I2C controller that can be master
> > >> or slave. For a general binding, I think we need to support multiple
> > >> slave addresses. The style above seems to support that with little
> > >> complexity.
> > > 
> > > I have no idea how other implementations may look like. The downstream
> > > kernel seems to program the slave address in a secondary step after the
> > > controller is initialized [1]. So every "client" which binds to the
> > > slave driver can use its own address, but only one client at the same
> > > time is allowed. As said before, the may also exist devices (!tegra)
> > > with multiple slave addresses at the same time.
> > > 
> > > Your approach above seems fulfill these properties (what about a single
> > > slave controller without a master?),
> > 
> > There would simply be a node for the controller, and a single child node
> > for the slave setup, and no child nodes for mastered devices.
> > 
> > > but it will be tricky for the slave to find the
> > > controller resources it needs.
> > 
> > I don't understand this. The resources are only needed by I2C driver,
> > and all come from the I2C driver node. If the I2C driver ends up being
> > split into separate master/slave portions, that information can easily
> > be passed from the main driver probe()/... to those other sub-parts of
> > the driver.
> > 
> > > I'm thinking of an insane (but valid)
> > > configuration where all i2c ports of the SoC are connected together and
> > > one
> > > port plays the master and all others are slaves. In this case we need to
> > > add a property to the slave node which points to his controller
> > > instance.> 
> > Why do the slave nodes care where they're mastered from?
> > 
> > I think you'd just have the following
> > 
> > /* master */
> > i2c@xxxxx {
> > 
> >     foo@0x40 {
> >     
> >         reg = <MASTER 0x40>;
> >         compatible = "nvidia,nvec";
> >     
> >     }
> > 
> > };
> > 
> > i2c@yyyy {
> > 
> >     foo@40 {
> >     
> >         reg = <SLAVE 0x40>;
> >         compatible = "nvidia,nvec-slave";
> >     
> >     }
> > 
> > };
> > 
> > There's no need for the slave child node to know that it is mastered
> > from the Tegra I2C controller; all it cares about is that there is some
> > I2C bus that it needs to respond to transactions upon.
> 
> This binding describes only case, when I2C device are connected to I2C
> controller.
> 
> Assume that I2C controller #1 (@xxxxx), I2C controller #2 (@yyyy), and
> nvec I2C master device are connected to same bus.
> How dt must be composed in this case ? Must i2c@xxxxx and i2c@yyyy be in
> parent/child relation (in terms of dt) ?

What I learned from the somehow unfortune discussion yesterday on IRC is that 
we cannot describe the exact i2c topology in the device tree, especially if it 
is software configuable (slave/master changing roles).

I also think there is a misunderstanding about how things get layered in 
software (and which info from device tree the drivers need). In your example 
above, there is a child node unter each controller which is responsible for 
slave transfers. The nvec can be a separate node in the device tree outside of 
the i2c structure, but needs one or more pointers to slave drivers which he 
can use to send his command byte streams to. E.g.

i2c@xxxx {
	compatible = "nvidia,tegra-i2c";
	// resources for the i2c controller
	first_slave: slave@40 {
		compatible = "nvidia,tegra-i2c-slave";
		reg = <40>; // i2c client address
		gpios = <4e>; // for side channel
	};
};

i2c@yyyy {
	compatible = "nvidia,tegra-i2c";
	// resources for the i2c controller
	second_slave: slave@41 {
		compatible = "nvidia,tegra-i2c-slave";
		reg = <41>; // i2c client address
		gpios = <4f>; // for side channel
	};
};

external_master {
	compatible = "ext,master";
	slaves = &first_slave,&second_slave;
};

or

nvec {
	compatible = "nvidia,nvec";
	slave = &first_slave;
	keyboard {
		// keyboard resources
	};
};

I'm not sure where the side channel belongs to. In case of nvec, the gpio must
be released after the first byte went over the wire, so putting it into nvec 
node is not possible (or needs a callback from the layer below).

Marc

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

On 09/24/2013 01:19 AM, Andrey Danin wrote:
> On Mon, Sep 23, 2013 at 8:36 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org
...
>     I think you'd just have the following
> 
>     /* master */
>     i2c@xxxxx {
>         foo@0x40 {
>             reg = <MASTER 0x40>;
>             compatible = "nvidia,nvec";
>         }
>     };
> 
>     i2c@yyyy {
>         foo@40 {
>             reg = <SLAVE 0x40>;
>             compatible = "nvidia,nvec-slave";
>         }
>     };
> 
>     There's no need for the slave child node to know that it is mastered
>     from the Tegra I2C controller; all it cares about is that there is some
>     I2C bus that it needs to respond to transactions upon.
> 
> This binding describes only case, when I2C device are connected to I2C
> controller.
> 
> Assume that I2C controller #1 (@xxxxx), I2C controller #2 (@yyyy), and
> nvec I2C master device are connected to same bus.
> How dt must be composed in this case ? Must i2c@xxxxx and i2c@yyyy be in
> parent/child relation (in terms of dt) ?

None of the I2C bindings currently allow one to specify that multiple of
the on-SoC controllers are connected to the same bus.

I'm not sure it's particularly useful to represent this anyway. Hardware
hooked up like this is pretty rare to start with (i.e. I know of no
board at all that's connected this way). I assume that if such HW did
exist, you'd simply assign each I2C slave to a particular I2C master,
and hence only put a DT node for it under a single DT master node.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] binding for nvec mfd device

[Stephen Warren]

On 09/24/2013 03:33 AM, Marc Dietrich wrote:
...
> What I learned from the somehow unfortune discussion yesterday on IRC is that 
> we cannot describe the exact i2c topology in the device tree, especially if it 
> is software configuable (slave/master changing roles).

Well, there's also no need to represent the exact I2C topology; we only
need to represent the parts that SW cares about. The existence of other
masters on the bus is not something that SW typically cares about. SW
can't directly interact with other masters (through just plain I2C
protocol at least), and can only observe their presence by potential
arbitration loss events.

> I also think there is a misunderstanding about how things get layered in 
> software (and which info from device tree the drivers need). In your example 
> above, there is a child node unter each controller which is responsible for 
> slave transfers. The nvec can be a separate node in the device tree outside of 
> the i2c structure, but needs one or more pointers to slave drivers which he 
> can use to send his command byte streams to. E.g.

No, in my example, the slave node /is/ the NVEC protocol. There is no
separate NVEC node outside the I2C node/tree.

> i2c@xxxx {
> 	compatible = "nvidia,tegra-i2c";
> 	// resources for the i2c controller
> 	first_slave: slave@40 {
> 		compatible = "nvidia,tegra-i2c-slave";

That's not what I wrote. This should be:

		compatible = "nvidia,nvec-slave";

... which instantiates an I2C slave protocol driver, and hooks it up so
that if the I2C slave controller receives an externally-initiated I2C
transaction to I2C address 0x40, that transaction will be passed to the
NVEC driver for processing and response.


> 		reg = <40>; // i2c client address
> 		gpios = <4e>; // for side channel
> 	};
> };

...
> external_master {
> 	compatible = "ext,master";
> 	slaves = &first_slave,&second_slave;
> };
> 
> or
> 
> nvec {
> 	compatible = "nvidia,nvec";
> 	slave = &first_slave;
> 	keyboard {
> 		// keyboard resources
> 	};
> };

So that separate nvec node is not required. All configuration of the
NVEC slave protocol should be part of the slave@40 node above.

IIRC, the way NVEC works is that a GPIO is used to request that the NVEC
IC poll the slave device if the slave device wants to initiate a
transaction with the NVEC IC. If that's correct, the model above should
work fine.

However, if when Tegra wants to make a master transaction on the I2C bus
(which would therefore have to be to a device other than the NVEC IC),
if the NVEC IC and Tegra need to arbitrate for the bus outside the plain
I2C protocol, then you'd want to model this as an I2C bus mux instead:

	i2c1: i2c@xxxx {
		compatible = "nvidia,tegra-i2c";

		foo@40 {
			reg = <SLAVE 0x40>;
			compatible = "nvidia,nvec-slave";
			keyboard {
				...
			};
			// any other parameters NVEC slave
			// protocol needs go here
		}
	};

	i2cmux {
		compatible = "nvidia,i2c-mux-nvec";
		#address-cells = <1>;
		#size-cells = <0>;
		nvec-gpios = <&gpio1 22 0>;
		i2c-parent = <&i2c1>;

		// all other devices on the I2C bus go here:
		ssd1307: oled@3c {
			compatible = "solomon,ssd1307fb-i2c";
				reg = <0x3c>;
				pwms = <&pwm 4 3000>;
				reset-gpios = <&gpio2 7 1>;
				reset-active-low;
			};
		};
	};

In this case, perhaps the drivers for the NVEC bus mux and NVEC slave
protocol might need to communicate with each-other. In which case, a
phandle between the two may be appropriate.