[PATCH v2 0/6] ARM: tegra: add nvec keyboard support for paz00

[PATCH v2 0/6] ARM: tegra: add nvec keyboard support for paz00

[Andrey Danin]

This patch series introduces keyboard support for AC100 (board paz00).

 I2C slave mode was implemented for i2c core and tegra-i2c.

 NVEC code from linux kernel was reworked to use tegra-i2c driver.

 Keytable header file is copied from linux kernel but modified
 to fix styles and remove unused code.

 Based on u-boot-tegra/next.

CC: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
CC: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>
CC: Julian Andres Klode <jak-4HMq4SXA452hPH1hqNUYSQ@public.gmane.org>
CC: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: ac100-oU9gvf+ajcQ97yFScArB1dHuzzzSOjJt@public.gmane.org
---
 Changes for v2:
	- I2C slave mode for i2c-core and tegra-i2c implemented
	- Fixed NVEC dt bindings 
	- NVEC driver was reworked to use tegra-i2c
	- fixed incorrect keys handling in nvec-keyboard driver
	- patch is splitted to smaller parts

Andrey Danin (6):
  i2c: add slave mode support
  ARM: tegra: i2c: add slave mode support
  ARM: tegra: i2c: add nvec driver
  ARM: tegra: nvec: add keyboard support
  ARM: tegra: paz00: add dt bindings for nvec
  ARM: tegra: paz00: enable nvec keyboard

 arch/arm/include/asm/arch-tegra/tegra_i2c.h        |    6 +
 arch/arm/include/asm/arch-tegra/tegra_nvec.h       |  130 +++++++++
 .../include/asm/arch-tegra/tegra_nvec_keyboard.h   |  304 ++++++++++++++++++++
 board/compal/dts/tegra20-paz00.dts                 |    8 +-
 board/nvidia/common/board.c                        |   12 +
 drivers/i2c/Makefile                               |    1 +
 drivers/i2c/i2c_core.c                             |   13 +
 drivers/i2c/tegra_i2c.c                            |  199 ++++++++++++-
 drivers/i2c/tegra_nvec.c                           |  294 +++++++++++++++++++
 drivers/input/Makefile                             |    3 +
 drivers/input/tegra-nvec-kbc.c                     |  215 ++++++++++++++
 include/configs/paz00.h                            |    9 +
 include/configs/tegra-common-post.h                |    2 +
 include/fdtdec.h                                   |    1 +
 include/i2c.h                                      |   30 +-
 lib/fdtdec.c                                       |    1 +
 16 files changed, 1223 insertions(+), 5 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-tegra/tegra_nvec.h
 create mode 100644 arch/arm/include/asm/arch-tegra/tegra_nvec_keyboard.h
 create mode 100644 drivers/i2c/tegra_nvec.c
 create mode 100644 drivers/input/tegra-nvec-kbc.c

-- 
1.7.9.5

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

[PATCH v2 5/6] ARM: tegra: paz00: add dt bindings for nvec

[Andrey Danin]

Signed-off-by: Andrey Danin <danindrey-JGs/UdohzUI@public.gmane.org>
CC: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
CC: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>
CC: Julian Andres Klode <jak-4HMq4SXA452hPH1hqNUYSQ@public.gmane.org>
CC: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: ac100-oU9gvf+ajcQ97yFScArB1dHuzzzSOjJt@public.gmane.org
---
 Changes for v2:
 	- Separated from enabling keyboard patch
	- Changed NVEC dt bindings 

 arch/arm/dts/tegra20-paz00.dts |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/tegra20-paz00.dts b/arch/arm/dts/tegra20-paz00.dts
index 780203c..9906f3a 100644
--- a/arch/arm/dts/tegra20-paz00.dts
+++ b/arch/arm/dts/tegra20-paz00.dts
@@ -40,7 +40,13 @@
 	};
 
 	i2c@7000c500 {
-		status = "disabled";
+		status = "okay";
+		clock-frequency = <40000>;
+		slave-addr = <138>;
+		nvec {
+			compatible = "nvidia,tegra20-nvec";
+			request-gpios = <&gpio 170 0>; /* gpio PV2 */
+		};
 	};
 
 	i2c@7000d000 {
-- 
1.7.9.5

--
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: [Ac100] [PATCH v2 5/6] ARM: tegra: paz00: add dt bindings for nvec

[Stephen Warren]

On 04/26/2014 07:14 PM, Andrey Danin wrote:

This patch isn't adding DT bindings for NVEC, but rather add DT nodes.
The binding is the schema, not the content.

We need a DT binding document that's been reviewed by the DT binding
maintainers. Can you please first submit a patch to the Linux kernel
that modifies the existing I2C core and Tegra I2C controller binding
documentation to add slave mode support. Once that's fully reviewed and
ack'd, this patch series can implement support for it in U-Boot.

> diff --git a/arch/arm/dts/tegra20-paz00.dts b/arch/arm/dts/tegra20-paz00.dts

>  	i2c@7000c500 {
> -		status = "disabled";
> +		status = "okay";
> +		clock-frequency = <40000>;
> +		slave-addr = <138>;
> +		nvec {
> +			compatible = "nvidia,tegra20-nvec";
> +			request-gpios = <&gpio 170 0>; /* gpio PV2 */

The reg property is missing here. Since the i2c node has
#address-cells/#size-cells, there must be a reg property in the children.

There's nothing here to indicate that this node is a slave device rather
than a master device, and doesn't seem to be any allowance for a single
I2C controller to support both master and slave nodes at the same time
(which I think Tegra's controller can IIRC).

IIRC, I had previously suggested something like encoding master/slave
into the reg property of the I2C child nodes. We could either do:

a) Set some top-bit to indicate a slave device.

b) If #address-cells=<1>, only master devices are present. If
#address-cells=<2>, either master or slave devices could be present.
Cell 0 could be 0==master, 1==slave, and cell 1 the actual I2C bus address.

Either of those approaches would allow representing an I2C controller
that supported multiple slave addresses. Even though I think Tegra's
slave controller doesn't support that, I still think we should use a
generic binding so that I2C slave mode looks the same everywhere.
--
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: [U-Boot] [Ac100] [PATCH v2 5/6] ARM: tegra: paz00: add dtbindings for nvec

[Marc Dietrich]

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

Hi,

Am Montag, 28. April 2014, 17:04:13 schrieb Stephen Warren:
> On 04/26/2014 07:14 PM, Andrey Danin wrote:
> 
> This patch isn't adding DT bindings for NVEC, but rather add DT nodes.
> The binding is the schema, not the content.
>
> We need a DT binding document that's been reviewed by the DT binding
> maintainers. Can you please first submit a patch to the Linux kernel
> that modifies the existing I2C core and Tegra I2C controller binding
> documentation to add slave mode support. Once that's fully reviewed and
> ack'd, this patch series can implement support for it in U-Boot.

I'm sorry that I didn't came up with a proper kernel implementation, but while 
we are discussion the binding I just want to give some coins.

> > diff --git a/arch/arm/dts/tegra20-paz00.dts
> > b/arch/arm/dts/tegra20-paz00.dts> 
> >  	i2c@7000c500 {
> > 
> > -		status = "disabled";
> > +		status = "okay";
> > +		clock-frequency = <40000>;
> > +		slave-addr = <138>;
> > +		nvec {
> > +			compatible = "nvidia,tegra20-nvec";
> > +			request-gpios = <&gpio 170 0>; /* gpio PV2 */
> 
> The reg property is missing here. Since the i2c node has
> #address-cells/#size-cells, there must be a reg property in the children.
> 
> There's nothing here to indicate that this node is a slave device rather
> than a master device, and doesn't seem to be any allowance for a single
> I2C controller to support both master and slave nodes at the same time
> (which I think Tegra's controller can IIRC).
> 
> IIRC, I had previously suggested something like encoding master/slave
> into the reg property of the I2C child nodes. We could either do:
> 
> a) Set some top-bit to indicate a slave device.
> 
> b) If #address-cells=<1>, only master devices are present. If
> #address-cells=<2>, either master or slave devices could be present.
> Cell 0 could be 0==master, 1==slave, and cell 1 the actual I2C bus address.

I'm not sure if this is really needed. NVEC knows it has to configure the 
tegra controller as slave. I don't see a reason to double this fact in the 
device tree. I like the idea of how the downstream kernel does it. The driver 
calls something like tegra_i2c_init_slave with the slave address as an 
parameter. This means that the slave address is not a property of the i2c 
(slave-) controller, but of the master because it has the address hard coded 
in its firmware. So reg = <138>; would be sufficient here and it also enables 
multi-slave configs.

i2c-tegra must take care that only one transaction (slave or master) is 
running at a time. The client (e.g. nvec) is free to drop the excluse usage by 
calling tegra_i2c_stop_slave so master mode can be used again or another 
client (master device) can be started where the slave controller gets a 
different address.

Multi-master would require that only one master can hold the lock enabling 
slave mode, but that's all software stuff and not related to the binding.

Also the kernel binding would require that nvec node itself has subdevices for 
e.g. keyboard, mouse, power, ... which are connected to the internal ec bus. 
We can use the nvec protocol identifiers to assign an address here.

Ok, lets take a look at the binding now:

i2c@7000c500 {
		status = "okay";
		clock-frequency = <40000>;
		nvec@138 {
			compatible = "nvidia,nvec, simple-bus";
			#address-cells = <1>
			#size-cells = <0>;
			reg = <138>;
			request-gpios = <&gpio 170 0>; /* gpio PV2 */

			keyboard@5 {
				compatible = "nvidia,nvec-kbd";						
				reg = <5>;
			};
			mouse@6 {
				compatible = "nvidia,nvec-aux";
				reg = <6>;
				packet-size = <6>;
			};
		};
};

Only thing I'm not sure is where to put the clock-frequency because it really 
depends on the i2c clients and if we have multiple masters, we could use 
multiple frequencies. Or maybe the lowest supported must be used instead, in 
which case the clock-frequency is better placed in the controller node.

Marc

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [U-Boot] [Ac100] [PATCH v2 5/6] ARM: tegra: paz00: add dtbindings for nvec

[Stephen Warren]

On 04/30/2014 01:52 AM, Marc Dietrich wrote:
> Hi,
> 
> Am Montag, 28. April 2014, 17:04:13 schrieb Stephen Warren:
>> On 04/26/2014 07:14 PM, Andrey Danin wrote:
>>
>> This patch isn't adding DT bindings for NVEC, but rather add DT nodes.
>> The binding is the schema, not the content.
>>
>> We need a DT binding document that's been reviewed by the DT binding
>> maintainers. Can you please first submit a patch to the Linux kernel
>> that modifies the existing I2C core and Tegra I2C controller binding
>> documentation to add slave mode support. Once that's fully reviewed and
>> ack'd, this patch series can implement support for it in U-Boot.
> 
> I'm sorry that I didn't came up with a proper kernel implementation, but while 
> we are discussion the binding I just want to give some coins.

I'm not looking for a kernel driver implementation, but we do need to
the DT binding fully reviewed and accepted.

>>> diff --git a/arch/arm/dts/tegra20-paz00.dts
>>> b/arch/arm/dts/tegra20-paz00.dts> 
>>>  	i2c@7000c500 {
>>>
>>> -		status = "disabled";
>>> +		status = "okay";
>>> +		clock-frequency = <40000>;
>>> +		slave-addr = <138>;
>>> +		nvec {
>>> +			compatible = "nvidia,tegra20-nvec";
>>> +			request-gpios = <&gpio 170 0>; /* gpio PV2 */
>>
>> The reg property is missing here. Since the i2c node has
>> #address-cells/#size-cells, there must be a reg property in the children.
>>
>> There's nothing here to indicate that this node is a slave device rather
>> than a master device, and doesn't seem to be any allowance for a single
>> I2C controller to support both master and slave nodes at the same time
>> (which I think Tegra's controller can IIRC).
>>
>> IIRC, I had previously suggested something like encoding master/slave
>> into the reg property of the I2C child nodes. We could either do:
>>
>> a) Set some top-bit to indicate a slave device.
>>
>> b) If #address-cells=<1>, only master devices are present. If
>> #address-cells=<2>, either master or slave devices could be present.
>> Cell 0 could be 0==master, 1==slave, and cell 1 the actual I2C bus address.
> 
> I'm not sure if this is really needed. NVEC knows it has to configure the 
> tegra controller as slave. I don't see a reason to double this fact in the 
> device tree. I like the idea of how the downstream kernel does it. The driver 
> calls something like tegra_i2c_init_slave with the slave address as an 
> parameter. This means that the slave address is not a property of the i2c 
> (slave-) controller, but of the master because it has the address hard coded 
> in its firmware. So reg = <138>; would be sufficient here and it also enables 
> multi-slave configs.

The binding in this patch is very special-cased. Instead, we need to
create a generic binding that will work identically across arbitrary I2C
controllers, some of which do support multiple slave addresses. The
issue that caused Linus to blow up about ARM was because everyone went
off and did their own HW-specific stuff without taking a look at the big
picture. That's exactly what the binding proposed in this patch does too.
--
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: [U-Boot] [Ac100] [PATCH v2 5/6] ARM: tegra: paz00: add dtbindingsfor nvec

[Marc Dietrich]

On Wed, 30 Apr 2014 10:21:07 -0600
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 04/30/2014 01:52 AM, Marc Dietrich wrote:
> > Am Montag, 28. April 2014, 17:04:13 schrieb Stephen Warren:
> >> On 04/26/2014 07:14 PM, Andrey Danin wrote:
> >>
> >> This patch isn't adding DT bindings for NVEC, but rather add DT nodes.
> >> The binding is the schema, not the content.
> >>
> >> We need a DT binding document that's been reviewed by the DT binding
> >> maintainers. Can you please first submit a patch to the Linux kernel
> >> that modifies the existing I2C core and Tegra I2C controller binding
> >> documentation to add slave mode support. Once that's fully reviewed and
> >> ack'd, this patch series can implement support for it in U-Boot.
> > 
> > I'm sorry that I didn't came up with a proper kernel implementation, but while 
> > we are discussion the binding I just want to give some coins.
> 
> I'm not looking for a kernel driver implementation, but we do need to
> the DT binding fully reviewed and accepted.

well, a kernel implementation would have included the binding.

> >>> diff --git a/arch/arm/dts/tegra20-paz00.dts
> >>> b/arch/arm/dts/tegra20-paz00.dts> 
> >>>  	i2c@7000c500 {
> >>>
> >>> -		status = "disabled";
> >>> +		status = "okay";
> >>> +		clock-frequency = <40000>;
> >>> +		slave-addr = <138>;
> >>> +		nvec {
> >>> +			compatible = "nvidia,tegra20-nvec";
> >>> +			request-gpios = <&gpio 170 0>; /* gpio PV2 */
> >>
> >> The reg property is missing here. Since the i2c node has
> >> #address-cells/#size-cells, there must be a reg property in the children.
> >>
> >> There's nothing here to indicate that this node is a slave device rather
> >> than a master device, and doesn't seem to be any allowance for a single
> >> I2C controller to support both master and slave nodes at the same time
> >> (which I think Tegra's controller can IIRC).
> >>
> >> IIRC, I had previously suggested something like encoding master/slave
> >> into the reg property of the I2C child nodes. We could either do:
> >>
> >> a) Set some top-bit to indicate a slave device.
> >>
> >> b) If #address-cells=<1>, only master devices are present. If
> >> #address-cells=<2>, either master or slave devices could be present.
> >> Cell 0 could be 0==master, 1==slave, and cell 1 the actual I2C bus address.
> > 
> > I'm not sure if this is really needed. NVEC knows it has to configure the 
> > tegra controller as slave. I don't see a reason to double this fact in the 
> > device tree. I like the idea of how the downstream kernel does it. The driver 
> > calls something like tegra_i2c_init_slave with the slave address as an 
> > parameter. This means that the slave address is not a property of the i2c 
> > (slave-) controller, but of the master because it has the address hard coded 
> > in its firmware. So reg = <138>; would be sufficient here and it also enables 
> > multi-slave configs.
> 
> The binding in this patch is very special-cased. Instead, we need to
> create a generic binding that will work identically across arbitrary I2C
> controllers, some of which do support multiple slave addresses. 

I don't see why multi-slave capable controllers won't work. You just
increase the number in the controller's address-cells and add the slave
addresses to the master's reg property. The point here is that the slave
addresses of an i2c controller may be configured at runtime, not at
device initialization via devicetree. This also gives a higher
flexibility for different slave/master combinations.

> The issue that caused Linus to blow up about ARM was because everyone went
> off and did their own HW-specific stuff without taking a look at the big
> picture. That's exactly what the binding proposed in this patch does too.

I can only assume that your are slowly getting tired of this binding
discussion. Still there is no reason for harsh words or to delete my
proposal from the context. IMHO, I tried to be as generic as possible.

Anyway, we should move this discussion to the devicetree ml.

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