Patchwork [U-Boot,3/3] ARM: tegra: paz00: enable nvec keyboard support

login
register
mail settings
Submitter Andrey Danin
Date July 19, 2013, 8:47 a.m.
Message ID <1374223663-8576-4-git-send-email-danindrey@mail.ru>
Download mbox | patch
Permalink /patch/260241/
State Changes Requested
Delegated to: Tom Warren
Headers show

Comments

Andrey Danin - July 19, 2013, 8:47 a.m.
Signed-off-by: Andrey Danin <danindrey@mail.ru>
---
 board/compal/dts/tegra20-paz00.dts |    8 ++++++++
 include/configs/paz00.h            |    8 ++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)
Stephen Warren - July 19, 2013, 7:14 p.m.
On 07/19/2013 02:47 AM, Andrey Danin wrote:
> Signed-off-by: Andrey Danin <danindrey@mail.ru>

(Some patch descriptions would be useful)

> diff --git a/board/compal/dts/tegra20-paz00.dts b/board/compal/dts/tegra20-paz00.dts

> +	nvec {
> +		compatible = "nvidia,tegra20-nvec";
> +		reg = <0x7000c500 0x100>;
> +		clock-frequency = <80000>;
> +		request-gpios = <&gpio 170 0>; /* gpio PV2 */
> +		slave-addr = <138>;
> +	};

I would rather not propagate this DT binding. We need to fix the binding
to clearly separate the concepts of:

a) The I2C slave controller (which should be a standalone driver for the
Tegra I2C slave HW).

b) The protocol sent over the I2C slave channel (which would be specific
to NVEC, implement the GPIO hand-shaking, etc.).

c) The devices that communicate over the protocol (keyboard in this case).

I suspect we need separate DT nodes/sub-nodes for all of those, and a
method of hooking them all together.
Marc Dietrich - July 20, 2013, 9:12 a.m.
On Friday 19 July 2013 13:14:13 Stephen Warren wrote:
> On 07/19/2013 02:47 AM, Andrey Danin wrote:
> > Signed-off-by: Andrey Danin <danindrey@mail.ru>
> 
> (Some patch descriptions would be useful)
> 
> > diff --git a/board/compal/dts/tegra20-paz00.dts
> > b/board/compal/dts/tegra20-paz00.dts
> > 
> > +	nvec {
> > +		compatible = "nvidia,tegra20-nvec";
> > +		reg = <0x7000c500 0x100>;
> > +		clock-frequency = <80000>;
> > +		request-gpios = <&gpio 170 0>; /* gpio PV2 */
> > +		slave-addr = <138>;
> > +	};
> 
> I would rather not propagate this DT binding. We need to fix the binding
> to clearly separate the concepts of:

so here we go again. I think I have to take this on my shoulders since I 
didn't got it right yet in the kernel. 

> a) The I2C slave controller (which should be a standalone driver for the
> Tegra I2C slave HW).
> 
> b) The protocol sent over the I2C slave channel (which would be specific
> to NVEC, implement the GPIO hand-shaking, etc.).
> 
> c) The devices that communicate over the protocol (keyboard in this case).
> 
> I suspect we need separate DT nodes/sub-nodes for all of those, and a
> method of hooking them all together.

Let's skip how this may actually look like in software. Given the discussions 
we had in the past, I propose the following binding:

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

	nvec {
		compatible = "nvidia,nvec", "simple-bus";
		protocol = "smbus-request-gpio";
		request-gpios = <&gpio 170 0>; /* gpio PV2 */
		
		keyboard {
			compatible = "nvidia,nvec-keyboard";
		};
	};
};

Does this looks better?

Marc
Stephen Warren - July 21, 2013, 3:20 a.m.
On 07/20/2013 03:12 AM, Marc Dietrich wrote:
> On Friday 19 July 2013 13:14:13 Stephen Warren wrote:
...
> Let's skip how this may actually look like in software. Given the discussions 
> we had in the past, I propose the following binding:
> 
> i2c-slave@7000c500 {
> 	compatible = "nvidia,tegra20-i2c-slave";
> 	reg = <0x7000c500 0x100>;
> 	interrupts = <0 92 0x04>;
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	clock-frequency = <80000>;
> 	slave-addr = <138>;

Hex would be more common, but that's a minor issue.

> 	clocks = <&tegra_car 67>, <&tegra_car 124>;
> 	clock-names = "div-clk", "fast-clk";
> 
> 	nvec {

Above, it says #address-cells=<1>, which means this node needs a reg
property. Perhaps slave-addr should be part of the child nodes (and the
Tegra I2C controller binding would limit itself to supporting only a
single node), so that the same binding style could be applicable to I2C
slave devices that support multiple slave addresses.

> 		compatible = "nvidia,nvec", "simple-bus";
> 		protocol = "smbus-request-gpio";

What is that property for; doesn't compatible="nvidia,nvec" already
imply this, or does the NVEC spec define multiple different protocols?

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

We should use the C pre-processor to provide named constants there,
although I guess U-Boot isn't set up for that yet. The kernel is once
this is ported there, and once the 2013.07 release is out, U-Boot should
be able to support this very soon too.

> 		keyboard {

Simple-bus might require a reg property; I forget. Does the NVEC
protocol include any form of "virtual device address" that it would make
sense to put into a reg property?

> 			compatible = "nvidia,nvec-keyboard";
> 		};
> 	};
> };
> 
> Does this looks better?

Yes, overall much better.

New DT bindings should be sent to devicetree@vger.kernel.org for review.
Note that's a branch new list (it moved from a different server), so it
might be best to wait a few days for people to subscribe before sending
mail to it.
Marc Dietrich - July 22, 2013, 8:09 a.m.
Am Samstag, 20. Juli 2013, 21:20:52 schrieb Stephen Warren:
> On 07/20/2013 03:12 AM, Marc Dietrich wrote:
> > On Friday 19 July 2013 13:14:13 Stephen Warren wrote:
> ...
> 
> > Let's skip how this may actually look like in software. Given the
> > discussions we had in the past, I propose the following binding:
> > 
> > i2c-slave@7000c500 {
> > 
> > 	compatible = "nvidia,tegra20-i2c-slave";
> > 	reg = <0x7000c500 0x100>;
> > 	interrupts = <0 92 0x04>;
> > 	#address-cells = <1>;
> > 	#size-cells = <0>;
> > 	clock-frequency = <80000>;
> > 	slave-addr = <138>;
> 
> Hex would be more common, but that's a minor issue.

ok.

> > 	clocks = <&tegra_car 67>, <&tegra_car 124>;
> > 	clock-names = "div-clk", "fast-clk";
> > 	
> > 	nvec {
> 
> Above, it says #address-cells=<1>, which means this node needs a reg
> property. Perhaps slave-addr should be part of the child nodes (and the
> Tegra I2C controller binding would limit itself to supporting only a
> single node), so that the same binding style could be applicable to I2C
> slave devices that support multiple slave addresses.

you mean 

nvec@87 {
		reg = <0x87>;
		...
} ?

I think that's ok. Didn't know that Tegra can support multiple slave 
addresses. To make the binding as general as possible, we could support multi-
slave for the binding, but only support single slave in the code for now.

I guess that also warrants a "simple-bus" compatibility in the i2c-slave node.

> 
> > 		compatible = "nvidia,nvec", "simple-bus";
> > 		protocol = "smbus-request-gpio";
> 
> What is that property for; doesn't compatible="nvidia,nvec" already
> imply this, or does the NVEC spec define multiple different protocols?

The GPIO is optional, but SMBUS is required. Maybe to support master initiated 
communications only. To get rid of the ugly protocol property, we could just 
check if a valid gpio is given. I think that's what the downstream kernel also 
does. The nvec still needs to tell the slave driver which protocol to use, but 
that can be hard coded.

> > 		request-gpios = <&gpio 170 0>; /* gpio PV2 */
> 
> We should use the C pre-processor to provide named constants there,
> although I guess U-Boot isn't set up for that yet. The kernel is once
> this is ported there, and once the 2013.07 release is out, U-Boot should
> be able to support this very soon too.
> 
> > 		keyboard {
> 
> Simple-bus might require a reg property; I forget. Does the NVEC
> protocol include any form of "virtual device address" that it would make
> sense to put into a reg property?
> 
> > 			compatible = "nvidia,nvec-keyboard";

well, we could misuse the nvec function select byte for this (e.g. 5 for 
keyboard, 6 for mouse, ...), but we may run into trouble with the "system 
command = 1" which is used by several drivers.

> > 
> > Does this looks better?
> 
> Yes, overall much better.
> 
> New DT bindings should be sent to devicetree@vger.kernel.org for review.
> Note that's a branch new list (it moved from a different server), so it
> might be best to wait a few days for people to subscribe before sending
> mail to it.

Ok, I'll send out a new version and cc dtml. Maybe this some more people 
respond ;-)

Thanks for review!

Marc
Stephen Warren - July 23, 2013, 3:40 p.m.
On 07/22/2013 01:09 AM, Marc Dietrich wrote:
> Am Samstag, 20. Juli 2013, 21:20:52 schrieb Stephen Warren:
>> On 07/20/2013 03:12 AM, Marc Dietrich wrote:
>>> On Friday 19 July 2013 13:14:13 Stephen Warren wrote:
>> ...
>>
>>> Let's skip how this may actually look like in software. Given the
>>> discussions we had in the past, I propose the following binding:
>>>
>>> i2c-slave@7000c500 {

>>> 	#address-cells = <1>;
>>> 	#size-cells = <0>;

>>> 	nvec {
>>
>> Above, it says #address-cells=<1>, which means this node needs a reg
>> property. Perhaps slave-addr should be part of the child nodes (and the
>> Tegra I2C controller binding would limit itself to supporting only a
>> single node), so that the same binding style could be applicable to I2C
>> slave devices that support multiple slave addresses.
> 
> you mean 
> 
> nvec@87 {
> 		reg = <0x87>;
> 		...
> } ?

Yes.

> I think that's ok. Didn't know that Tegra can support multiple slave 
> addresses. To make the binding as general as possible, we could support multi-
> slave for the binding, but only support single slave in the code for now.

Tegra can't, but that doesn't mean some future Tegra or some other SoC
can't/won't. Putting the slave address inside the child node makes sure
the same DT schema will work in other situations, and hence be consistent.

> I guess that also warrants a "simple-bus" compatibility in the i2c-slave node.
> 
>>
>>> 		compatible = "nvidia,nvec", "simple-bus";
>>> 		protocol = "smbus-request-gpio";
>>
>> What is that property for; doesn't compatible="nvidia,nvec" already
>> imply this, or does the NVEC spec define multiple different protocols?
> 
> The GPIO is optional, but SMBUS is required. Maybe to support master initiated 
> communications only. To get rid of the ugly protocol property, we could just 
> check if a valid gpio is given. I think that's what the downstream kernel also 
> does.

Yes, I believe nvidia,nvec implies everything about the protocol then,
except for the optional GPIO which can be checked directly.

> The nvec still needs to tell the slave driver which protocol to use, but
> that can be hard coded.

I'm not sure what that means. At the controller/HW level, aren't I2C and
SMBUS the same; it's just the data within the transactions that may be
more defined by one or the other?
Marc Dietrich - July 24, 2013, 5:52 p.m.
On Tuesday 23 July 2013 08:40:42 Stephen Warren wrote:
> On 07/22/2013 01:09 AM, Marc Dietrich wrote:

[ snip the stuff we agreed upon ]

> > The nvec still needs to tell the slave driver which protocol to use, but
> > that can be hard coded.
> 
> I'm not sure what that means. At the controller/HW level, aren't I2C and
> SMBUS the same; it's just the data within the transactions that may be
> more defined by one or the other?

at this level yes, but we need to handle the underlying protocol in the ISR, 
which means that depending on the protocol (smbus or I2C), we need a different 
interrupt service routine. Currently we are doing most of the smbus protocol 
in the ISR because of timing reasons and I'm not sure if we can change this. 
We are already suffering from nvec timeouts and I fear that splitting the 
protocol out of the ISR would make things even worse.

I'm going to submit a new DT binding during the weekend. Meanwhile, Andrey is 
trying to split the uboot driver in the hw and nvec parts and also adapting it 
for the new binding.

Marc
Stephen Warren - July 25, 2013, 5:40 p.m.
On 07/24/2013 10:52 AM, Marc Dietrich wrote:
> On Tuesday 23 July 2013 08:40:42 Stephen Warren wrote:
>> On 07/22/2013 01:09 AM, Marc Dietrich wrote:
> 
> [ snip the stuff we agreed upon ]
> 
>>> The nvec still needs to tell the slave driver which protocol to use, but
>>> that can be hard coded.
>>
>> I'm not sure what that means. At the controller/HW level, aren't I2C and
>> SMBUS the same; it's just the data within the transactions that may be
>> more defined by one or the other?
> 
> at this level yes, but we need to handle the underlying protocol in the ISR, 
> which means that depending on the protocol (smbus or I2C), we need a different 
> interrupt service routine. Currently we are doing most of the smbus protocol 
> in the ISR because of timing reasons and I'm not sure if we can change this. 
> We are already suffering from nvec timeouts and I fear that splitting the 
> protocol out of the ISR would make things even worse.

I assume that the I2C slave driver's ISR would simply
directly/immediately call a function in the "protocol driver". That
should allow sufficient separation of the layers while still maintaining
minimal latency, assuming a good design of the callback's function
prototype and/or the list of I2C slave controller functions that the
callback is allowed to call into.

Patch

diff --git a/board/compal/dts/tegra20-paz00.dts b/board/compal/dts/tegra20-paz00.dts
index 780203c..2df5446 100644
--- a/board/compal/dts/tegra20-paz00.dts
+++ b/board/compal/dts/tegra20-paz00.dts
@@ -88,4 +88,12 @@ 
 		nvidia,panel-vdd-gpios = <&gpio 4 0>;		/* PA4 */
 		nvidia,panel-timings = <400 4 203 17 15>;
 	};
+
+	nvec {
+		compatible = "nvidia,tegra20-nvec";
+		reg = <0x7000c500 0x100>;
+		clock-frequency = <80000>;
+		request-gpios = <&gpio 170 0>; /* gpio PV2 */
+		slave-addr = <138>;
+	};
 };
diff --git a/include/configs/paz00.h b/include/configs/paz00.h
index eac1ef9..d711be8 100644
--- a/include/configs/paz00.h
+++ b/include/configs/paz00.h
@@ -72,6 +72,14 @@ 
 #define CONFIG_SYS_WHITE_ON_BLACK
 #define CONFIG_CONSOLE_SCROLL_LINES	10
 
+/* Keyboard support */
+#define CONFIG_KEYBOARD
+#define CONFIG_TEGRA_NVEC_KEYBOARD
+/* NVEC support */
+#define CONFIG_TEGRA_I2C
+#define CONFIG_SYS_I2C_INIT_BOARD
+#define CONFIG_TEGRA_NVEC
+
 #include "tegra-common-post.h"
 
 #endif /* __CONFIG_H */