[PATCH v2 0/3] paz00 updates for 3.3

[PATCH v2 0/3] paz00 updates for 3.3

[Marc Dietrich]

Hi,

the following three patches are intended for the 3.3 merge window and are based
on linux-next.

The first one adds wakeup support similar to seaboard, but with the difference
that the wakeup gpio is connected to the embedded controller instead of a real
gpio key.

The second one adds a device tree for paz00. There is a nvec node which defines
resources similar to the i2c controller and also adds additional required 
properties for the embedded controller (which handles keyboard,touchpad, leds,
and suspend functions - and some other stuff).
The way it is initialized will probably change in the future, but we like to
include it for now as it makes debugging more easier.

The final patch adds device tree support for the nvec driver so that the previous
commit is useful.

Thanks

Marc

Changes since v1
    - replace addition of the embedded controller to the board file by a device-tree
      based implementation.

Marc Dietrich (3):
  ARM: tegra: paz00: add support for wakeup gpio key
  arm/dt: tegra: add dts file for paz00
  staging: nvec: add device tree support

 arch/arm/boot/dts/tegra-paz00.dts |   70 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-tegra/Makefile      |    1 +
 arch/arm/mach-tegra/Makefile.boot |    1 +
 arch/arm/mach-tegra/board-dt.c    |    3 ++
 arch/arm/mach-tegra/board-paz00.c |   29 +++++++++++++++-
 arch/arm/mach-tegra/board-paz00.h |    3 ++
 drivers/staging/nvec/nvec.c       |   39 +++++++++++++++++++-
 7 files changed, 143 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/boot/dts/tegra-paz00.dts

-- 
1.7.5.4

[PATCH v2 1/3] ARM: tegra: paz00: add support for wakeup gpio key

[Marc Dietrich]

This adds support for a wakeup gpio which is connected to the
embedded controller. This will be used later on for wakeup from suspend.

Signed-off-by: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>
---
 arch/arm/mach-tegra/board-paz00.c |   29 ++++++++++++++++++++++++++++-
 arch/arm/mach-tegra/board-paz00.h |    3 +++
 2 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-tegra/board-paz00.c b/arch/arm/mach-tegra/board-paz00.c
index 602f8dd..35fe372 100644
--- a/arch/arm/mach-tegra/board-paz00.c
+++ b/arch/arm/mach-tegra/board-paz00.c
@@ -23,8 +23,11 @@
 #include <linux/serial_8250.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
+#include <linux/gpio.h>
+#include <linux/gpio_keys.h>
 #include <linux/pda_power.h>
 #include <linux/io.h>
+#include <linux/input.h>
 #include <linux/i2c.h>
 #include <linux/rfkill-gpio.h>
 
@@ -36,7 +39,6 @@
 #include <mach/iomap.h>
 #include <mach/irqs.h>
 #include <mach/sdhci.h>
-#include <mach/gpio.h>
 
 #include "board.h"
 #include "board-paz00.h"
@@ -114,12 +116,37 @@ static struct platform_device leds_gpio = {
         },
 };
 
+static struct gpio_keys_button paz00_gpio_keys_buttons[] = {
+	{
+		.code		= KEY_POWER,
+		.gpio		= TEGRA_GPIO_POWERKEY,
+		.active_low	= 1,
+		.desc		= "Power",
+		.type		= EV_KEY,
+		.wakeup		= 1,
+	},
+};
+
+static struct gpio_keys_platform_data paz00_gpio_keys = {
+	.buttons	= paz00_gpio_keys_buttons,
+	.nbuttons	= ARRAY_SIZE(paz00_gpio_keys_buttons),
+};
+
+static struct platform_device gpio_keys_device = {
+	.name	= "gpio-keys",
+	.id	= -1,
+	.dev	= {
+		.platform_data = &paz00_gpio_keys,
+	},
+};
+
 static struct platform_device *paz00_devices[] __initdata = {
 	&debug_uart,
 	&tegra_sdhci_device4,
 	&tegra_sdhci_device1,
 	&wifi_rfkill_device,
 	&leds_gpio,
+	&gpio_keys_device,
 };
 
 static void paz00_i2c_init(void)
diff --git a/arch/arm/mach-tegra/board-paz00.h b/arch/arm/mach-tegra/board-paz00.h
index 2dc1899..de5d22b 100644
--- a/arch/arm/mach-tegra/board-paz00.h
+++ b/arch/arm/mach-tegra/board-paz00.h
@@ -32,6 +32,9 @@
 #define TEGRA_WIFI_RST		TEGRA_GPIO_PD1
 #define TEGRA_WIFI_LED		TEGRA_GPIO_PD0
 
+/* WakeUp */
+#define TEGRA_GPIO_POWERKEY	TEGRA_GPIO_PJ7
+
 void paz00_pinmux_init(void);
 
 #endif
-- 
1.7.5.4

[PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Marc Dietrich]

This adds a dts file for paz00. As a side effect, this also enables
the embedded controller which controls the keyboard, touchpad, power,
leds, and some other functions.

Signed-off-by: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>
---
 arch/arm/boot/dts/tegra-paz00.dts |   70 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-tegra/Makefile      |    1 +
 arch/arm/mach-tegra/Makefile.boot |    1 +
 arch/arm/mach-tegra/board-dt.c    |    3 ++
 4 files changed, 75 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/boot/dts/tegra-paz00.dts

diff --git a/arch/arm/boot/dts/tegra-paz00.dts b/arch/arm/boot/dts/tegra-paz00.dts
new file mode 100644
index 0000000..9bd471e
--- /dev/null
+++ b/arch/arm/boot/dts/tegra-paz00.dts
@@ -0,0 +1,70 @@
+/dts-v1/;
+
+/memreserve/ 0x1c000000 0x04000000;
+/include/ "tegra20.dtsi"
+
+/ {
+	model = "Toshiba AC100 / Dynabook AZ";
+	compatible = "compal,paz00", "nvidia,tegra20";
+
+	chosen {
+		bootargs = "mem=448@0 console=ttyS0,115200n8 root=/dev/mmcblk1p1";
+	};
+
+	memory@0 {
+		reg = <0x00000000 0x20000000>;
+	};
+
+	i2c@7000c000 {
+		clock-frequency = <400000>;
+	};
+
+	i2c@7000c400 {
+		clock-frequency = <400000>;
+	};
+
+	i2c@7000c500 {
+		status = "disable";
+	};
+
+	nvec@7000c500 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,nvec";
+		reg = <0x7000C500 0x100>;
+		interrupts = <124>;
+		clock-frequency = <80000>;
+		request-gpios = <&gpio 170 0>;
+		slave-addr = <138>;
+	};
+
+	i2c@7000d000 {
+		clock-frequency = <400000>;
+	};
+
+	serial@70006000 {
+		clock-frequency = <216000000>;
+	};
+
+	serial@70006300 {
+		clock-frequency = <216000000>;
+	};
+
+	sdhci@c8000000 {
+		cd-gpios = <&gpio 173 0>; /* gpio PV5 */
+		wp-gpios = <&gpio 57 0>;  /* gpio PH1 */
+		power-gpios = <&gpio 155 0>; /* gpio PT3 */
+	};
+
+	sdhci@c8000200 {
+		status = "disable";
+	};
+
+	sdhci@c8000400 {
+		status = "disable";
+	};
+
+	sdhci@c8000600 {
+		support-8bit;
+	};
+};
diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
index 91a07e1..70826a9 100644
--- a/arch/arm/mach-tegra/Makefile
+++ b/arch/arm/mach-tegra/Makefile
@@ -31,6 +31,7 @@ obj-${CONFIG_MACH_SEABOARD}             += board-seaboard-pinmux.o
 
 obj-${CONFIG_MACH_TEGRA_DT}             += board-dt.o
 obj-${CONFIG_MACH_TEGRA_DT}             += board-harmony-pinmux.o
+obj-$(CONFIG_MACH_TEGRA_DT)             += board-paz00-pinmux.o
 obj-${CONFIG_MACH_TEGRA_DT}             += board-seaboard-pinmux.o
 
 obj-${CONFIG_MACH_TRIMSLICE}            += board-trimslice.o
diff --git a/arch/arm/mach-tegra/Makefile.boot b/arch/arm/mach-tegra/Makefile.boot
index bd12c9f..152f9fb 100644
--- a/arch/arm/mach-tegra/Makefile.boot
+++ b/arch/arm/mach-tegra/Makefile.boot
@@ -3,5 +3,6 @@ params_phys-$(CONFIG_ARCH_TEGRA_2x_SOC)	:= 0x00000100
 initrd_phys-$(CONFIG_ARCH_TEGRA_2x_SOC)	:= 0x00800000
 
 dtb-$(CONFIG_MACH_HARMONY) += tegra-harmony.dtb
+dtb-$(CONFIG_MACH_PAZ00) += tegra-paz00.dtb
 dtb-$(CONFIG_MACH_SEABOARD) += tegra-seaboard.dtb
 dtb-$(CONFIG_MACH_VENTANA) += tegra-ventana.dtb
diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
index d368f8d..379660e 100644
--- a/arch/arm/mach-tegra/board-dt.c
+++ b/arch/arm/mach-tegra/board-dt.c
@@ -46,6 +46,7 @@
 #include "devices.h"
 
 void harmony_pinmux_init(void);
+void paz00_pinmux_init(void);
 void seaboard_pinmux_init(void);
 void ventana_pinmux_init(void);
 
@@ -85,6 +86,7 @@ static struct {
 	void (*init)(void);
 } pinmux_configs[] = {
 	{ "nvidia,harmony", harmony_pinmux_init },
+	{ "compal,paz00", paz00_pinmux_init },
 	{ "nvidia,seaboard", seaboard_pinmux_init },
 	{ "nvidia,ventana", ventana_pinmux_init },
 };
@@ -120,6 +122,7 @@ static void __init tegra_dt_init(void)
 
 static const char * tegra_dt_board_compat[] = {
 	"nvidia,harmony",
+	"compal,paz00",
 	"nvidia,seaboard",
 	"nvidia,ventana",
 	NULL
-- 
1.7.5.4

[PATCH v2 3/3] staging: nvec: add device tree support

[Marc Dietrich]

This adds device tree support to the nvec driver. By using this method
it is no longer necessary to specify platform data through a board
file.

Cc: devel-tBiZLqfeLfOHmIFyCCdPziST3g8Odh+X@public.gmane.org
Cc: Julian Andres Klode <jak-4HMq4SXA452hPH1hqNUYSQ@public.gmane.org>
Signed-off-by: Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/staging/nvec/nvec.c |   39 +++++++++++++++++++++++++++++++++++++--
 1 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index fb0f095..731eeeb 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -26,6 +26,8 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/irq.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/list.h>
 #include <linux/mfd/core.h>
 #include <linux/mutex.h>
@@ -35,6 +37,8 @@
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 
+#include <asm/byteorder.h>
+
 #include <mach/clk.h>
 #include <mach/iomap.h>
 
@@ -718,6 +722,7 @@ static int __devinit tegra_nvec_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct resource *iomem;
 	void __iomem *base;
+	const unsigned int *prop;
 
 	nvec = kzalloc(sizeof(struct nvec_chip), GFP_KERNEL);
 	if (nvec == NULL) {
@@ -726,8 +731,26 @@ static int __devinit tegra_nvec_probe(struct platform_device *pdev)
 	}
 	platform_set_drvdata(pdev, nvec);
 	nvec->dev = &pdev->dev;
-	nvec->gpio = pdata->gpio;
-	nvec->i2c_addr = pdata->i2c_addr;
+
+	if (pdata) {
+		nvec->gpio = pdata->gpio;
+		nvec->i2c_addr = pdata->i2c_addr;
+	} else if (nvec->dev->of_node) {
+		nvec->gpio = of_get_named_gpio(nvec->dev->of_node, "request-gpios", 0);
+		if (nvec->gpio < 0) {
+			dev_err(&pdev->dev, "no gpio specified");
+			goto failed;
+		}
+		prop = of_get_property(nvec->dev->of_node, "slave-addr", NULL);
+		if (!prop) {
+			dev_err(&pdev->dev, "no i2c address specified");
+			goto failed;
+		}
+		nvec->i2c_addr = be32_to_cpup(prop);
+	} else {
+		dev_err(&pdev->dev, "no platform data\n");
+		goto failed;
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
@@ -892,6 +915,17 @@ static int tegra_nvec_resume(struct platform_device *pdev)
 #define tegra_nvec_resume NULL
 #endif
 
+#if defined(CONFIG_OF)
+/* Match table for of_platform binding */
+static const struct of_device_id nvidia_nvec_of_match[] __devinitconst = {
+	{ .compatible = "nvidia,nvec", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, nvidia_nvec_of_match);
+#else
+#define nvidia_nvec_of_match NULL
+#endif
+
 static struct platform_driver nvec_device_driver = {
 	.probe   = tegra_nvec_probe,
 	.remove  = __devexit_p(tegra_nvec_remove),
@@ -900,6 +934,7 @@ static struct platform_driver nvec_device_driver = {
 	.driver  = {
 		.name = "nvec",
 		.owner = THIS_MODULE,
+		.of_match_table = nvidia_nvec_of_match,
 	}
 };
 
-- 
1.7.5.4

RE: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Stephen Warren]

Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> This adds a dts file for paz00. As a side effect, this also enables
> the embedded controller which controls the keyboard, touchpad, power,
> leds, and some other functions.
...
> +++ b/arch/arm/boot/dts/tegra-paz00.dts
> @@ -0,0 +1,70 @@
> +/dts-v1/;
> +
> +/memreserve/ 0x1c000000 0x04000000;
> +/include/ "tegra20.dtsi"
> +
> +/ {
> +	model = "Toshiba AC100 / Dynabook AZ";
> +	compatible = "compal,paz00", "nvidia,tegra20";
> +
> +	chosen {
> +		bootargs = "mem=448@0 console=ttyS0,115200n8 root=/dev/mmcblk1p1";

You shouldn't need the mem= parameter here; it wasn't in your first patch set.

> +	};
> +
> +	memory@0 {
> +		reg = <0x00000000 0x20000000>;
> +	};
> +
> +	i2c@7000c000 {
> +		clock-frequency = <400000>;
> +	};
> +
> +	i2c@7000c400 {
> +		clock-frequency = <400000>;
> +	};
> +
> +	i2c@7000c500 {
> +		status = "disable";
> +	};
> +
> +	nvec@7000c500 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "nvidia,nvec";

I'm not convinced that's the correct compatible value, but I should
discuss this more in the nvec patch...

> +		reg = <0x7000C500 0x100>;
> +		interrupts = <124>;
> +		clock-frequency = <80000>;
> +		request-gpios = <&gpio 170 0>;
> +		slave-addr = <138>;
> +	};


> +++ b/arch/arm/mach-tegra/Makefile
> @@ -31,6 +31,7 @@ obj-${CONFIG_MACH_SEABOARD}             += board-seaboard-pinmux.o
> 
>  obj-${CONFIG_MACH_TEGRA_DT}             += board-dt.o
>  obj-${CONFIG_MACH_TEGRA_DT}             += board-harmony-pinmux.o
> +obj-$(CONFIG_MACH_TEGRA_DT)             += board-paz00-pinmux.o
>  obj-${CONFIG_MACH_TEGRA_DT}             += board-seaboard-pinmux.o

What branch is your patch based on? Those lines use braces not brackets
in the latest code (incorrect yes, but that's what's there; Olof said
he'll fix it for the next merge window). Do you have some local patches
in your branch before this patchset? Note: Feel free to add your lines
using the correct syntax; I'm just wondering about the existing context
in your patch.

-- 
nvpublic

RE: [PATCH v2 3/3] staging: nvec: add device tree support

[Stephen Warren]

Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> This adds device tree support to the nvec driver. By using this method
> it is no longer necessary to specify platform data through a board
> file.

You should document the binding in Documentation/devicetree/bindings.

> @@ -892,6 +915,17 @@ static int tegra_nvec_resume(struct platform_device *pdev)
>  #define tegra_nvec_resume NULL
>  #endif
> 
> +#if defined(CONFIG_OF)

I think you can just remove the ifdef and always include this code. Yes, it'll
result in slightly more rodata when !CONFIG_OF, but !CONFIG_OF isn't going to
exist or be useful for Tegra for that much longer.

> +/* Match table for of_platform binding */
> +static const struct of_device_id nvidia_nvec_of_match[] __devinitconst = {
> +	{ .compatible = "nvidia,nvec", },

I'm not sure that nvidia,nvec is the right value, but need a little more
background.

It's my understanding that how this works is a little micro-controller
exists on the board, handles various devices like the keyboard, and sends
data to Tegra by making I2C master transactions. Isn't it the case that
the micro-controller (or at least the SW running on it) is board-specific,
and the same for the I2C protocol? If so, nvidia,nvec is a little generic;
we probably need to name it compal,paz00-ec or something like that?

Either way, we should probably include some kind of version number in
the compatible property so we can support upgrades to the protocol if
needed.

-- 
nvpublic

Re: [PATCH v2 3/3] staging: nvec: add device tree support

[Julian Andres Klode]

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

On Thu, Oct 27, 2011 at 12:17:25PM -0700, Stephen Warren wrote:
> Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > This adds device tree support to the nvec driver. By using this method
> > it is no longer necessary to specify platform data through a board
> > file.
> 
> You should document the binding in Documentation/devicetree/bindings.
> 
> > @@ -892,6 +915,17 @@ static int tegra_nvec_resume(struct platform_device *pdev)
> >  #define tegra_nvec_resume NULL
> >  #endif
> > 
> > +#if defined(CONFIG_OF)
> 
> I think you can just remove the ifdef and always include this code. Yes, it'll
> result in slightly more rodata when !CONFIG_OF, but !CONFIG_OF isn't going to
> exist or be useful for Tegra for that much longer.

Often things do not actually build anymore without CONFIG_OF -- For example,
the code in -next failed to build a month ago or so (don't know if that's
still the case, though).
 
> > +/* Match table for of_platform binding */
> > +static const struct of_device_id nvidia_nvec_of_match[] __devinitconst = {
> > +	{ .compatible = "nvidia,nvec", },
> 
> I'm not sure that nvidia,nvec is the right value, but need a little more
> background.
> 
> It's my understanding that how this works is a little micro-controller
> exists on the board, handles various devices like the keyboard, and sends
> data to Tegra by making I2C master transactions. Isn't it the case that
> the micro-controller (or at least the SW running on it) is board-specific,
> and the same for the I2C protocol? If so, nvidia,nvec is a little generic;
> we probably need to name it compal,paz00-ec or something like that?

nvec means Nvidia Embedded Controller, and the protocol is not
device-specific anyway. There are some device-specific things,
the controller has some commands reserved for OEM usage. We do
not use those yet.

For an example of a non-paz00 device: The Advent Vega tablet and
similar (those with boards called "shuttle") also use nvec.

If you want to know details, you could search someone at your
company who knows more about it. There should be a few people
knowing things about nvec.

-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

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

Re: RE: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Marc Dietrich]

Am Donnerstag, 27. Oktober 2011, 09:50:03 schrieb Stephen Warren:
> Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > This adds a dts file for paz00. As a side effect, this also enables
> > the embedded controller which controls the keyboard, touchpad, power,
> > leds, and some other functions.
> 
> ...
> 
> > +++ b/arch/arm/boot/dts/tegra-paz00.dts
> > @@ -0,0 +1,70 @@
> > +/dts-v1/;
> > +
> > +/memreserve/ 0x1c000000 0x04000000;
> > +/include/ "tegra20.dtsi"
> > +
> > +/ {
> > +	model = "Toshiba AC100 / Dynabook AZ";
> > +	compatible = "compal,paz00", "nvidia,tegra20";
> > +
> > +	chosen {
> > +		bootargs = "mem=448@0 console=ttyS0,115200n8 root=/dev/mmcblk1p1";
> 
> You shouldn't need the mem= parameter here; it wasn't in your first patch set.

that's because I forgot it. Sorry, I didn't mentioned it in the changelog. I wonder 
why mem= is still needed. I've seen patches on the chromeos tree which try to reserve 
the gpu memory on demand. While we are at it, what is the vmalloc=192M used by most 
other boards for?

> > +	};
> > +
> > +	memory@0 {
> > +		reg = <0x00000000 0x20000000>;
> > +	};
> > +
> > +	i2c@7000c000 {
> > +		clock-frequency = <400000>;
> > +	};
> > +
> > +	i2c@7000c400 {
> > +		clock-frequency = <400000>;
> > +	};
> > +
> > +	i2c@7000c500 {
> > +		status = "disable";
> > +	};
> > +
> > +	nvec@7000c500 {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		compatible = "nvidia,nvec";
> 
> I'm not convinced that's the correct compatible value, but I should
> discuss this more in the nvec patch...
> 
> > +		reg = <0x7000C500 0x100>;
> > +		interrupts = <124>;
> > +		clock-frequency = <80000>;
> > +		request-gpios = <&gpio 170 0>;
> > +		slave-addr = <138>;
> > +	};
> > 
> > 
> > +++ b/arch/arm/mach-tegra/Makefile
> > @@ -31,6 +31,7 @@ obj-${CONFIG_MACH_SEABOARD}             +=
> > board-seaboard-pinmux.o> 
> >  obj-${CONFIG_MACH_TEGRA_DT}             += board-dt.o
> >  obj-${CONFIG_MACH_TEGRA_DT}             += board-harmony-pinmux.o
> > 
> > +obj-$(CONFIG_MACH_TEGRA_DT)             += board-paz00-pinmux.o
> > 
> >  obj-${CONFIG_MACH_TEGRA_DT}             += board-seaboard-pinmux.o
> 
> What branch is your patch based on? Those lines use braces not brackets
> in the latest code (incorrect yes, but that's what's there; Olof said
> he'll fix it for the next merge window). Do you have some local patches
> in your branch before this patchset? Note: Feel free to add your lines
> using the correct syntax; I'm just wondering about the existing context
> in your patch.

branch is based on linux-next. It will create conflicts anyway during merge (given 
your trimslice update).

Marc

Re: RE: [PATCH v2 3/3] staging: nvec: add device tree support

[Marc Dietrich]

Am Donnerstag, 27. Oktober 2011, 12:17:25 schrieb Stephen Warren:
> Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > This adds device tree support to the nvec driver. By using this method
> > it is no longer necessary to specify platform data through a board
> > file.
> 
> You should document the binding in Documentation/devicetree/bindings.

oh, I feared that ... Will go in to v3.

> > @@ -892,6 +915,17 @@ static int tegra_nvec_resume(struct platform_device *pdev)
> > 
> >  #define tegra_nvec_resume NULL
> >  #endif
> > 
> > +#if defined(CONFIG_OF)
> 
> I think you can just remove the ifdef and always include this code. Yes, it'll
> result in slightly more rodata when !CONFIG_OF, but !CONFIG_OF isn't going to
> exist or be useful for Tegra for that much longer.

ok, I will check if it causes build regressions first.

> 
> > +/* Match table for of_platform binding */
> > +static const struct of_device_id nvidia_nvec_of_match[] __devinitconst = {
> > +	{ .compatible = "nvidia,nvec", },
> 
> I'm not sure that nvidia,nvec is the right value, but need a little more
> background.
> 
> It's my understanding that how this works is a little micro-controller
> exists on the board, handles various devices like the keyboard, and sends
> data to Tegra by making I2C master transactions. Isn't it the case that
> the micro-controller (or at least the SW running on it) is board-specific,
> and the same for the I2C protocol? If so, nvidia,nvec is a little generic;
> we probably need to name it compal,paz00-ec or something like that?

The firmware (for the 8051 mc inside the keyboard controller) is likely made by 
Compal, but as Julian already said, the EC protocol definition is very likely from 
NVIDIA itself. Compal just implemented it for the master. You may refer to 
<http://nv-
tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commitdiff;h=12114faf442a8c6aac81a9702712077364db0e82>
Also this protocol is not board specific as many first generation boards/device use 
it, so "nvidia,nvec" should be correct here.

> Either way, we should probably include some kind of version number in
> the compatible property so we can support upgrades to the protocol if
> needed.

You may ask your colleagues on that topic, but it seems that the protocol is dead 
already, e.g. it wasn't implemented for the new-world kernels (>= .36) anymore.

Marc

RE: RE: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Stephen Warren]

Marc Dietrich wrote at Friday, October 28, 2011 4:30 AM:
> Am Donnerstag, 27. Oktober 2011, 09:50:03 schrieb Stephen Warren:
> > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > This adds a dts file for paz00. As a side effect, this also enables
> > > the embedded controller which controls the keyboard, touchpad, power,
> > > leds, and some other functions.
> > ...
> > > +++ b/arch/arm/boot/dts/tegra-paz00.dts
> > > @@ -0,0 +1,70 @@
> > > +/dts-v1/;
> > > +
> > > +/memreserve/ 0x1c000000 0x04000000;
> > > +/include/ "tegra20.dtsi"
> > > +
> > > +/ {
> > > +	model = "Toshiba AC100 / Dynabook AZ";
> > > +	compatible = "compal,paz00", "nvidia,tegra20";
> > > +
> > > +	chosen {
> > > +		bootargs = "mem=448@0 console=ttyS0,115200n8 root=/dev/mmcblk1p1";
> >
> > You shouldn't need the mem= parameter here; it wasn't in your first patch set.
> 
> that's because I forgot it. Sorry, I didn't mentioned it in the changelog. I wonder
> why mem= is still needed.

I wonder if this is some conflict between ATAGs and the DT-specified
memory node.

As far as I can tell, the kernel's memory information typically comes
from ATAGs. Some boards override what's in the ATAGs in their machine
descriptor's fixup routine, e.g. see tegra_paz00_fixup(). Presumably,
this is because of buggy bootloaders sending incorrect memory information
in the ATAGs. Do you have any way to check the ATAGs that the bootloader
is sending? Probably, you could enable DEBUG_LL and using the low-level
debugging functions to dump some of that information to the UART early
during boot.

When boards boot from DT, there is no fixup function to override the
bootloader's ATAGs. I also see a bunch of code to set up the memory
information from DT e.g. setup_machine_fdt()'s call to:

	of_scan_flat_dt(early_init_dt_scan_memory, NULL);

... but I assume that happens before the ATAGs are processed, and the
buggy ATAGs end up overriding the information in the DT file. And further,
I assume that specifying "mem=" on the command-line overrides that, thus
solving the problem.

It'd be awesome if you could validate this; the simplest way is probably
to:

a) Remove mem= from the command-line
b) Modify arch/arm/kernel/setup.c:parse_tag_mem32() to do nothing;
   comment out the call to arm_add_memory()
c) Test booting, and check what RAM size the kernel thinks you have.

If that works, then ATAGs are the problem. We probably need to modify the
core ARM code not to use memory ATAGs when there is a DT?

I'm mainly pushing on this because adding "mem=" to the command-line in
the DT file isn't a great solution; when people start using bootloaders
that rewrite the DT to include the user-specified command-line, then every
user is going to have to start specifying "mem=" in their command-lines.

> I've seen patches on the chromeos tree which try to reserve the gpu
> memory on demand. While we are at it, what is the vmalloc=192M used
> by most other boards for?

I'm not sure what vmalloc= does exactly. I somewhat doubt it's necessary
until we have code that uses the GPU in mainline. The same goes for the
/memreserve/ DT entries. I've been thinking of submitting a patch to
remove this cruft, but haven't gotten around to it yet.

-- 
nvpublic

[PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Stephen Warren]

Marc Dietrich wrote at Friday, October 28, 2011 4:30 AM:
> Am Donnerstag, 27. Oktober 2011, 09:50:03 schrieb Stephen Warren:
> > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > This adds a dts file for paz00. As a side effect, this also enables
> > > the embedded controller which controls the keyboard, touchpad, power,
> > > leds, and some other functions.
> > ...
> > > +++ b/arch/arm/boot/dts/tegra-paz00.dts
> > > @@ -0,0 +1,70 @@
> > > +/dts-v1/;
> > > +
> > > +/memreserve/ 0x1c000000 0x04000000;
> > > +/include/ "tegra20.dtsi"
> > > +
> > > +/ {
> > > +	model = "Toshiba AC100 / Dynabook AZ";
> > > +	compatible = "compal,paz00", "nvidia,tegra20";
> > > +
> > > +	chosen {
> > > +		bootargs = "mem=448 at 0 console=ttyS0,115200n8 root=/dev/mmcblk1p1";
> >
> > You shouldn't need the mem= parameter here; it wasn't in your first patch set.
> 
> that's because I forgot it. Sorry, I didn't mentioned it in the changelog. I wonder
> why mem= is still needed.

I wonder if this is some conflict between ATAGs and the DT-specified
memory node.

As far as I can tell, the kernel's memory information typically comes
from ATAGs. Some boards override what's in the ATAGs in their machine
descriptor's fixup routine, e.g. see tegra_paz00_fixup(). Presumably,
this is because of buggy bootloaders sending incorrect memory information
in the ATAGs. Do you have any way to check the ATAGs that the bootloader
is sending? Probably, you could enable DEBUG_LL and using the low-level
debugging functions to dump some of that information to the UART early
during boot.

When boards boot from DT, there is no fixup function to override the
bootloader's ATAGs. I also see a bunch of code to set up the memory
information from DT e.g. setup_machine_fdt()'s call to:

	of_scan_flat_dt(early_init_dt_scan_memory, NULL);

... but I assume that happens before the ATAGs are processed, and the
buggy ATAGs end up overriding the information in the DT file. And further,
I assume that specifying "mem=" on the command-line overrides that, thus
solving the problem.

It'd be awesome if you could validate this; the simplest way is probably
to:

a) Remove mem= from the command-line
b) Modify arch/arm/kernel/setup.c:parse_tag_mem32() to do nothing;
   comment out the call to arm_add_memory()
c) Test booting, and check what RAM size the kernel thinks you have.

If that works, then ATAGs are the problem. We probably need to modify the
core ARM code not to use memory ATAGs when there is a DT?

I'm mainly pushing on this because adding "mem=" to the command-line in
the DT file isn't a great solution; when people start using bootloaders
that rewrite the DT to include the user-specified command-line, then every
user is going to have to start specifying "mem=" in their command-lines.

> I've seen patches on the chromeos tree which try to reserve the gpu
> memory on demand. While we are at it, what is the vmalloc=192M used
> by most other boards for?

I'm not sure what vmalloc= does exactly. I somewhat doubt it's necessary
until we have code that uses the GPU in mainline. The same goes for the
/memreserve/ DT entries. I've been thinking of submitting a patch to
remove this cruft, but haven't gotten around to it yet.

-- 
nvpublic

RE: RE: [PATCH v2 3/3] staging: nvec: add device tree support

[Stephen Warren]

Marc Dietrich wrote at Friday, October 28, 2011 5:02 AM:
> Am Donnerstag, 27. Oktober 2011, 12:17:25 schrieb Stephen Warren:
> > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > This adds device tree support to the nvec driver. By using this method
> > > it is no longer necessary to specify platform data through a board
> > > file.
...
> > > +/* Match table for of_platform binding */
> > > +static const struct of_device_id nvidia_nvec_of_match[] __devinitconst = {
> > > +	{ .compatible = "nvidia,nvec", },
> >
> > I'm not sure that nvidia,nvec is the right value, but need a little more
> > background.
> >
> > It's my understanding that how this works is a little micro-controller
> > exists on the board, handles various devices like the keyboard, and sends
> > data to Tegra by making I2C master transactions. Isn't it the case that
> > the micro-controller (or at least the SW running on it) is board-specific,
> > and the same for the I2C protocol? If so, nvidia,nvec is a little generic;
> > we probably need to name it compal,paz00-ec or something like that?
> 
> The firmware (for the 8051 mc inside the keyboard controller) is likely made by
> Compal, but as Julian already said, the EC protocol definition is very likely from
> NVIDIA itself. Compal just implemented it for the master. You may refer to
> <http://nv-
> tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commitdiff;h=12114faf442a8c6aac81a9702712077364db0e82>
> Also this protocol is not board specific as many first generation boards/device use
> it, so "nvidia,nvec" should be correct here.
> 
> > Either way, we should probably include some kind of version number in
> > the compatible property so we can support upgrades to the protocol if
> > needed.
> 
> You may ask your colleagues on that topic, but it seems that the protocol is dead
> already, e.g. it wasn't implemented for the new-world kernels (>= .36) anymore.

OK, I asked internally and it sounds like this is /probably/ standardized.

That said, there are apparently some OEMs who did change the protocol
and do something slightly different. I'm trying to confirm whether PAZ00
was one of them. I guess not if PAZ00 works with the standard driver that
you linked to.

So the good news is that there's an internal specification for this
protocol, and we might be able to release it. I'll let you know if/when
there are updates on this.

I'd like to call this "nvidia,nvec-1.0" to version this compatible
property; that's the specification version in the latest document that
I saw. While we do seemed to have abandoned this approach, I want to
make sure this is extensible if someone suddenly decides to go back to
it and creates a 2.0 in the future. Does that seem reasonable?

-- 
nvpublic

Re: RE: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Russell King - ARM Linux]

On Fri, Oct 28, 2011 at 09:49:49AM -0700, Stephen Warren wrote:
> When boards boot from DT, there is no fixup function to override the
> bootloader's ATAGs. I also see a bunch of code to set up the memory
> information from DT e.g. setup_machine_fdt()'s call to:
> 
> 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> 
> ... but I assume that happens before the ATAGs are processed, and the
> buggy ATAGs end up overriding the information in the DT file.

As far as the uncompressed kernel is concerned, there is either ATAG
or DT information, never both.  If the boot loader provides ATAGs and
the zImage has a DT appended to it, the zImage decompressor merges the
ATAGs into the appended DT and passes the DT to the kernel.

So anyone who currently 'fixes' their broken boot loader via the fixup
function by directly manipulating the ATAGS is going to hit the DT
image instead.

> > I've seen patches on the chromeos tree which try to reserve the gpu
> > memory on demand. While we are at it, what is the vmalloc=192M used
> > by most other boards for?
> 
> I'm not sure what vmalloc= does exactly. I somewhat doubt it's necessary
> until we have code that uses the GPU in mainline.

It defines the size of the vmalloc and ioremap area - and limits the
maximum amount of low memory (non-highmem) available to the kernel to
achieve the specified size of the vmalloc area.  Any additional memory
will be discarded, or in the presence of a highmem enabled kernel, will
be turned into highmem.

[PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Russell King - ARM Linux]

On Fri, Oct 28, 2011 at 09:49:49AM -0700, Stephen Warren wrote:
> When boards boot from DT, there is no fixup function to override the
> bootloader's ATAGs. I also see a bunch of code to set up the memory
> information from DT e.g. setup_machine_fdt()'s call to:
> 
> 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> 
> ... but I assume that happens before the ATAGs are processed, and the
> buggy ATAGs end up overriding the information in the DT file.

As far as the uncompressed kernel is concerned, there is either ATAG
or DT information, never both.  If the boot loader provides ATAGs and
the zImage has a DT appended to it, the zImage decompressor merges the
ATAGs into the appended DT and passes the DT to the kernel.

So anyone who currently 'fixes' their broken boot loader via the fixup
function by directly manipulating the ATAGS is going to hit the DT
image instead.

> > I've seen patches on the chromeos tree which try to reserve the gpu
> > memory on demand. While we are at it, what is the vmalloc=192M used
> > by most other boards for?
> 
> I'm not sure what vmalloc= does exactly. I somewhat doubt it's necessary
> until we have code that uses the GPU in mainline.

It defines the size of the vmalloc and ioremap area - and limits the
maximum amount of low memory (non-highmem) available to the kernel to
achieve the specified size of the vmalloc area.  Any additional memory
will be discarded, or in the presence of a highmem enabled kernel, will
be turned into highmem.

Re: RE: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Grant Likely]

On Sat, Oct 29, 2011 at 09:43:30AM +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 28, 2011 at 09:49:49AM -0700, Stephen Warren wrote:
> > When boards boot from DT, there is no fixup function to override the
> > bootloader's ATAGs. I also see a bunch of code to set up the memory
> > information from DT e.g. setup_machine_fdt()'s call to:
> > 
> > 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > 
> > ... but I assume that happens before the ATAGs are processed, and the
> > buggy ATAGs end up overriding the information in the DT file.
> 
> As far as the uncompressed kernel is concerned, there is either ATAG
> or DT information, never both.  If the boot loader provides ATAGs and
> the zImage has a DT appended to it, the zImage decompressor merges the
> ATAGs into the appended DT and passes the DT to the kernel.
> 
> So anyone who currently 'fixes' their broken boot loader via the fixup
> function by directly manipulating the ATAGS is going to hit the DT
> image instead.

Ugh.  Not pretty.  A platform could still have board specific fixup
code that matches on the compatible string that deals with broken
firmware ATAGs, but I don't think that is what we want to do.

We could handle it by filling the .dts file with an empty 'reg'
property for memory, and only push in the legacy ATAG data if reg is
indeed empty.  That gives the option of overriding ATAGs if the DT
already specifies memory.  Need to think about this more.

g.

[PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Grant Likely]

On Sat, Oct 29, 2011 at 09:43:30AM +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 28, 2011 at 09:49:49AM -0700, Stephen Warren wrote:
> > When boards boot from DT, there is no fixup function to override the
> > bootloader's ATAGs. I also see a bunch of code to set up the memory
> > information from DT e.g. setup_machine_fdt()'s call to:
> > 
> > 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > 
> > ... but I assume that happens before the ATAGs are processed, and the
> > buggy ATAGs end up overriding the information in the DT file.
> 
> As far as the uncompressed kernel is concerned, there is either ATAG
> or DT information, never both.  If the boot loader provides ATAGs and
> the zImage has a DT appended to it, the zImage decompressor merges the
> ATAGs into the appended DT and passes the DT to the kernel.
> 
> So anyone who currently 'fixes' their broken boot loader via the fixup
> function by directly manipulating the ATAGS is going to hit the DT
> image instead.

Ugh.  Not pretty.  A platform could still have board specific fixup
code that matches on the compatible string that deals with broken
firmware ATAGs, but I don't think that is what we want to do.

We could handle it by filling the .dts file with an empty 'reg'
property for memory, and only push in the legacy ATAG data if reg is
indeed empty.  That gives the option of overriding ATAGs if the DT
already specifies memory.  Need to think about this more.

g.

Re: RE: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Russell King - ARM Linux]

On Sat, Oct 29, 2011 at 01:03:20PM +0200, Grant Likely wrote:
> On Sat, Oct 29, 2011 at 09:43:30AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Oct 28, 2011 at 09:49:49AM -0700, Stephen Warren wrote:
> > > When boards boot from DT, there is no fixup function to override the
> > > bootloader's ATAGs. I also see a bunch of code to set up the memory
> > > information from DT e.g. setup_machine_fdt()'s call to:
> > > 
> > > 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > > 
> > > ... but I assume that happens before the ATAGs are processed, and the
> > > buggy ATAGs end up overriding the information in the DT file.
> > 
> > As far as the uncompressed kernel is concerned, there is either ATAG
> > or DT information, never both.  If the boot loader provides ATAGs and
> > the zImage has a DT appended to it, the zImage decompressor merges the
> > ATAGs into the appended DT and passes the DT to the kernel.
> > 
> > So anyone who currently 'fixes' their broken boot loader via the fixup
> > function by directly manipulating the ATAGS is going to hit the DT
> > image instead.
> 
> Ugh.  Not pretty.  A platform could still have board specific fixup
> code that matches on the compatible string that deals with broken
> firmware ATAGs, but I don't think that is what we want to do.

No.  The point I was making is that by the time you get to the fixup
function with a DT or even into the uncompressed kernel, even if the
boot loader provided ATAGs, your ATAGs have long since been obliterated
and are no longer available.

[PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Russell King - ARM Linux]

On Sat, Oct 29, 2011 at 01:03:20PM +0200, Grant Likely wrote:
> On Sat, Oct 29, 2011 at 09:43:30AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Oct 28, 2011 at 09:49:49AM -0700, Stephen Warren wrote:
> > > When boards boot from DT, there is no fixup function to override the
> > > bootloader's ATAGs. I also see a bunch of code to set up the memory
> > > information from DT e.g. setup_machine_fdt()'s call to:
> > > 
> > > 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > > 
> > > ... but I assume that happens before the ATAGs are processed, and the
> > > buggy ATAGs end up overriding the information in the DT file.
> > 
> > As far as the uncompressed kernel is concerned, there is either ATAG
> > or DT information, never both.  If the boot loader provides ATAGs and
> > the zImage has a DT appended to it, the zImage decompressor merges the
> > ATAGs into the appended DT and passes the DT to the kernel.
> > 
> > So anyone who currently 'fixes' their broken boot loader via the fixup
> > function by directly manipulating the ATAGS is going to hit the DT
> > image instead.
> 
> Ugh.  Not pretty.  A platform could still have board specific fixup
> code that matches on the compatible string that deals with broken
> firmware ATAGs, but I don't think that is what we want to do.

No.  The point I was making is that by the time you get to the fixup
function with a DT or even into the uncompressed kernel, even if the
boot loader provided ATAGs, your ATAGs have long since been obliterated
and are no longer available.

Re: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Marc Dietrich]

On Friday 28 October 2011 09:49:49 Stephen Warren wrote:
> Marc Dietrich wrote at Friday, October 28, 2011 4:30 AM:
> > Am Donnerstag, 27. Oktober 2011, 09:50:03 schrieb Stephen Warren:
> > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > > This adds a dts file for paz00. As a side effect, this also
> > > > enables
> > > > the embedded controller which controls the keyboard, touchpad,
> > > > power,
> > > > leds, and some other functions.
> > > 
> > > ...
> > > 
> > > > +++ b/arch/arm/boot/dts/tegra-paz00.dts
> > > > @@ -0,0 +1,70 @@
> > > > +/dts-v1/;
> > > > +
> > > > +/memreserve/ 0x1c000000 0x04000000;
> > > > +/include/ "tegra20.dtsi"
> > > > +
> > > > +/ {
> > > > +	model = "Toshiba AC100 / Dynabook AZ";
> > > > +	compatible = "compal,paz00", "nvidia,tegra20";
> > > > +
> > > > +	chosen {
> > > > +		bootargs = "mem=448@0 console=ttyS0,115200n8
> > > > root=/dev/mmcblk1p1";
> > > 
> > > You shouldn't need the mem= parameter here; it wasn't in your first
> > > patch set.> 
> > that's because I forgot it. Sorry, I didn't mentioned it in the
> > changelog. I wonder why mem= is still needed.
> 
> I wonder if this is some conflict between ATAGs and the DT-specified
> memory node.
> 
> As far as I can tell, the kernel's memory information typically comes
> from ATAGs. Some boards override what's in the ATAGs in their machine
> descriptor's fixup routine, e.g. see tegra_paz00_fixup(). Presumably,
> this is because of buggy bootloaders sending incorrect memory information
> in the ATAGs. Do you have any way to check the ATAGs that the bootloader
> is sending? Probably, you could enable DEBUG_LL and using the low-level
> debugging functions to dump some of that information to the UART early
> during boot.

I got the ATAGS from /proc/atags and there is no memory entry there, only 
initrd and cmdline (which is the one from nvflash).

The machine also has a fixup routine, but it also boots without (I'm sure it 
was necessary in the past),

> When boards boot from DT, there is no fixup function to override the
> bootloader's ATAGs. 

... so I guess this is why it also works with DT.

> I also see a bunch of code to set up the memory
> information from DT e.g. setup_machine_fdt()'s call to:
> 
> 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> 
> ... but I assume that happens before the ATAGs are processed, and the
> buggy ATAGs end up overriding the information in the DT file. And further,
> I assume that specifying "mem=" on the command-line overrides that, thus
> solving the problem.
> 
> It'd be awesome if you could validate this; the simplest way is probably
> to:
> 
> a) Remove mem= from the command-line

I tested several variations. Without DT, the fixup can compensate a missing 
mem entry in the kernel command line, but with a mem= from the kernel command 
line, a fixup is not needed.

With DT, the command line specified from nvflash is ignored and the one from 
DT comes into the game. If it is also missing there, system detects 512 MB 
(which is physical right, but we cannot reserve memory for the gpu). The fixup 
is indeed ignored in this case.

> b) Modify arch/arm/kernel/setup.c:parse_tag_mem32() to do nothing;
>    comment out the call to arm_add_memory()

I leave this out because the bootloader does not send memory info in the 
ATAGS.

> c) Test booting, and check what RAM size the kernel thinks you have.

see above. RAM detection works, but it's not what we want ...

> If that works, then ATAGs are the problem. We probably need to modify the
> core ARM code not to use memory ATAGs when there is a DT?
> 
> I'm mainly pushing on this because adding "mem=" to the command-line in
> the DT file isn't a great solution; when people start using bootloaders
> that rewrite the DT to include the user-specified command-line, then every
> user is going to have to start specifying "mem=" in their command-lines.

Well, I see two options for our case:

	a) use the mem=448M@0 in the DT so the gpu can get its memory
	b) upstream the chromeos changes to reserve gpu memory "on the fly" from 
the autodetected 512M (as it should be IMHO).

> > I've seen patches on the chromeos tree which try to reserve the gpu
> > memory on demand. While we are at it, what is the vmalloc=192M used
> > by most other boards for?
> 
> I'm not sure what vmalloc= does exactly. I somewhat doubt it's necessary
> until we have code that uses the GPU in mainline. The same goes for the
> /memreserve/ DT entries. I've been thinking of submitting a patch to
> remove this cruft, but haven't gotten around to it yet.

[PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Marc Dietrich]

On Friday 28 October 2011 09:49:49 Stephen Warren wrote:
> Marc Dietrich wrote at Friday, October 28, 2011 4:30 AM:
> > Am Donnerstag, 27. Oktober 2011, 09:50:03 schrieb Stephen Warren:
> > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > > This adds a dts file for paz00. As a side effect, this also
> > > > enables
> > > > the embedded controller which controls the keyboard, touchpad,
> > > > power,
> > > > leds, and some other functions.
> > > 
> > > ...
> > > 
> > > > +++ b/arch/arm/boot/dts/tegra-paz00.dts
> > > > @@ -0,0 +1,70 @@
> > > > +/dts-v1/;
> > > > +
> > > > +/memreserve/ 0x1c000000 0x04000000;
> > > > +/include/ "tegra20.dtsi"
> > > > +
> > > > +/ {
> > > > +	model = "Toshiba AC100 / Dynabook AZ";
> > > > +	compatible = "compal,paz00", "nvidia,tegra20";
> > > > +
> > > > +	chosen {
> > > > +		bootargs = "mem=448 at 0 console=ttyS0,115200n8
> > > > root=/dev/mmcblk1p1";
> > > 
> > > You shouldn't need the mem= parameter here; it wasn't in your first
> > > patch set.> 
> > that's because I forgot it. Sorry, I didn't mentioned it in the
> > changelog. I wonder why mem= is still needed.
> 
> I wonder if this is some conflict between ATAGs and the DT-specified
> memory node.
> 
> As far as I can tell, the kernel's memory information typically comes
> from ATAGs. Some boards override what's in the ATAGs in their machine
> descriptor's fixup routine, e.g. see tegra_paz00_fixup(). Presumably,
> this is because of buggy bootloaders sending incorrect memory information
> in the ATAGs. Do you have any way to check the ATAGs that the bootloader
> is sending? Probably, you could enable DEBUG_LL and using the low-level
> debugging functions to dump some of that information to the UART early
> during boot.

I got the ATAGS from /proc/atags and there is no memory entry there, only 
initrd and cmdline (which is the one from nvflash).

The machine also has a fixup routine, but it also boots without (I'm sure it 
was necessary in the past),

> When boards boot from DT, there is no fixup function to override the
> bootloader's ATAGs. 

... so I guess this is why it also works with DT.

> I also see a bunch of code to set up the memory
> information from DT e.g. setup_machine_fdt()'s call to:
> 
> 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> 
> ... but I assume that happens before the ATAGs are processed, and the
> buggy ATAGs end up overriding the information in the DT file. And further,
> I assume that specifying "mem=" on the command-line overrides that, thus
> solving the problem.
> 
> It'd be awesome if you could validate this; the simplest way is probably
> to:
> 
> a) Remove mem= from the command-line

I tested several variations. Without DT, the fixup can compensate a missing 
mem entry in the kernel command line, but with a mem= from the kernel command 
line, a fixup is not needed.

With DT, the command line specified from nvflash is ignored and the one from 
DT comes into the game. If it is also missing there, system detects 512 MB 
(which is physical right, but we cannot reserve memory for the gpu). The fixup 
is indeed ignored in this case.

> b) Modify arch/arm/kernel/setup.c:parse_tag_mem32() to do nothing;
>    comment out the call to arm_add_memory()

I leave this out because the bootloader does not send memory info in the 
ATAGS.

> c) Test booting, and check what RAM size the kernel thinks you have.

see above. RAM detection works, but it's not what we want ...

> If that works, then ATAGs are the problem. We probably need to modify the
> core ARM code not to use memory ATAGs when there is a DT?
> 
> I'm mainly pushing on this because adding "mem=" to the command-line in
> the DT file isn't a great solution; when people start using bootloaders
> that rewrite the DT to include the user-specified command-line, then every
> user is going to have to start specifying "mem=" in their command-lines.

Well, I see two options for our case:

	a) use the mem=448M at 0 in the DT so the gpu can get its memory
	b) upstream the chromeos changes to reserve gpu memory "on the fly" from 
the autodetected 512M (as it should be IMHO).

> > I've seen patches on the chromeos tree which try to reserve the gpu
> > memory on demand. While we are at it, what is the vmalloc=192M used
> > by most other boards for?
> 
> I'm not sure what vmalloc= does exactly. I somewhat doubt it's necessary
> until we have code that uses the GPU in mainline. The same goes for the
> /memreserve/ DT entries. I've been thinking of submitting a patch to
> remove this cruft, but haven't gotten around to it yet.

Re: [PATCH v2 3/3] staging: nvec: add device tree support

[Marc Dietrich]

On Friday 28 October 2011 09:56:41 you wrote:
> Marc Dietrich wrote at Friday, October 28, 2011 5:02 AM:
> > Am Donnerstag, 27. Oktober 2011, 12:17:25 schrieb Stephen Warren:
> > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > > This adds device tree support to the nvec driver. By using this
> > > > method it is no longer necessary to specify platform data
> > > > through a board file.
> 
> ...
> 
> > > > +/* Match table for of_platform binding */
> > > > +static const struct of_device_id nvidia_nvec_of_match[]
> > > > __devinitconst = { +	{ .compatible = "nvidia,nvec", },
> > > 
> > > I'm not sure that nvidia,nvec is the right value, but need a little
> > > more background.
> > > 
> > > It's my understanding that how this works is a little
> > > micro-controller
> > > exists on the board, handles various devices like the keyboard, and
> > > sends data to Tegra by making I2C master transactions. Isn't it the
> > > case that the micro-controller (or at least the SW running on it)
> > > is board-specific, and the same for the I2C protocol? If so,
> > > nvidia,nvec is a little generic; we probably need to name it
> > > compal,paz00-ec or something like that?> 
> > The firmware (for the 8051 mc inside the keyboard controller) is likely
> > made by Compal, but as Julian already said, the EC protocol definition
> > is very likely from NVIDIA itself. Compal just implemented it for the
> > master. You may refer to <http://nv-
> > tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commitdiff;h=12114faf442a8c6a
> > ac81a9702712077364db0e82> Also this protocol is not board specific as
> > many first generation boards/device use it, so "nvidia,nvec" should be
> > correct here.
> > 
> > > Either way, we should probably include some kind of version number
> > > in
> > > the compatible property so we can support upgrades to the protocol
> > > if
> > > needed.
> > 
> > You may ask your colleagues on that topic, but it seems that the
> > protocol is dead already, e.g. it wasn't implemented for the new-world
> > kernels (>= .36) anymore.
> OK, I asked internally and it sounds like this is /probably/ standardized.
> 
> That said, there are apparently some OEMs who did change the protocol
> and do something slightly different. I'm trying to confirm whether PAZ00
> was one of them. I guess not if PAZ00 works with the standard driver that
> you linked to.

There are so called OEM commands which we will move to a board specific nvec 
file (e.g. nvec_paz00.c). We haven't got the chance to test it on other boards 
using it yet (e.g. toshiba folio, advent vega, ...). The tablets are mostly 
using it for power control and maybe also leds/switches. I don't think any 
other board uses keyboard / mouse functions.

> So the good news is that there's an internal specification for this
> protocol, and we might be able to release it. I'll let you know if/when
> there are updates on this.

The original source is well documentated already, but additional info is 
always welcome ;-)

> I'd like to call this "nvidia,nvec-1.0" to version this compatible
> property; that's the specification version in the latest document that
> I saw. While we do seemed to have abandoned this approach, I want to
> make sure this is extensible if someone suddenly decides to go back to
> it and creates a 2.0 in the future. Does that seem reasonable?

mmh, I can't see why we should add it now. There is no V2 I can see in my 
limited view. If your company plans to expand the protocol you can either 
enhance our driver or create a new one (nvec2), which can add a nvidia,nvec-2 
compatibility property (we can also change ours to nvidia,nvec-1 at the same 
time, but that's not required). 

Marc

Re: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Grant Likely]

On Sun, Oct 30, 2011 at 09:39:37PM +0100, Marc Dietrich wrote:
> On Friday 28 October 2011 09:49:49 Stephen Warren wrote:
> > I also see a bunch of code to set up the memory
> > information from DT e.g. setup_machine_fdt()'s call to:
> > 
> > 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > 
> > ... but I assume that happens before the ATAGs are processed, and the
> > buggy ATAGs end up overriding the information in the DT file. And further,
> > I assume that specifying "mem=" on the command-line overrides that, thus
> > solving the problem.
> > 
> > It'd be awesome if you could validate this; the simplest way is probably
> > to:
> > 
> > a) Remove mem= from the command-line
> 
> I tested several variations. Without DT, the fixup can compensate a missing 
> mem entry in the kernel command line, but with a mem= from the kernel command 
> line, a fixup is not needed.
> 
> With DT, the command line specified from nvflash is ignored and the one from 
> DT comes into the game. If it is also missing there, system detects 512 MB 
> (which is physical right, but we cannot reserve memory for the gpu). The fixup 
> is indeed ignored in this case.

The /memreserve/ fields are supposed to allow you to do this.

> 
> > b) Modify arch/arm/kernel/setup.c:parse_tag_mem32() to do nothing;
> >    comment out the call to arm_add_memory()
> 
> I leave this out because the bootloader does not send memory info in the 
> ATAGS.
> 
> > c) Test booting, and check what RAM size the kernel thinks you have.
> 
> see above. RAM detection works, but it's not what we want ...
> 
> > If that works, then ATAGs are the problem. We probably need to modify the
> > core ARM code not to use memory ATAGs when there is a DT?
> > 
> > I'm mainly pushing on this because adding "mem=" to the command-line in
> > the DT file isn't a great solution; when people start using bootloaders
> > that rewrite the DT to include the user-specified command-line, then every
> > user is going to have to start specifying "mem=" in their command-lines.
> 
> Well, I see two options for our case:
> 
> 	a) use the mem=448M@0 in the DT so the gpu can get its memory
> 	b) upstream the chromeos changes to reserve gpu memory "on the fly" from 
> the autodetected 512M (as it should be IMHO).

b) is the ideal.  a) is absolutely wrong.  /memreserve/ should fix the
gpu memory problem when not passing a mem= parameter.  If /memreserve/
isn't working, then I'd like to know why.

g.

[PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Grant Likely]

On Sun, Oct 30, 2011 at 09:39:37PM +0100, Marc Dietrich wrote:
> On Friday 28 October 2011 09:49:49 Stephen Warren wrote:
> > I also see a bunch of code to set up the memory
> > information from DT e.g. setup_machine_fdt()'s call to:
> > 
> > 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > 
> > ... but I assume that happens before the ATAGs are processed, and the
> > buggy ATAGs end up overriding the information in the DT file. And further,
> > I assume that specifying "mem=" on the command-line overrides that, thus
> > solving the problem.
> > 
> > It'd be awesome if you could validate this; the simplest way is probably
> > to:
> > 
> > a) Remove mem= from the command-line
> 
> I tested several variations. Without DT, the fixup can compensate a missing 
> mem entry in the kernel command line, but with a mem= from the kernel command 
> line, a fixup is not needed.
> 
> With DT, the command line specified from nvflash is ignored and the one from 
> DT comes into the game. If it is also missing there, system detects 512 MB 
> (which is physical right, but we cannot reserve memory for the gpu). The fixup 
> is indeed ignored in this case.

The /memreserve/ fields are supposed to allow you to do this.

> 
> > b) Modify arch/arm/kernel/setup.c:parse_tag_mem32() to do nothing;
> >    comment out the call to arm_add_memory()
> 
> I leave this out because the bootloader does not send memory info in the 
> ATAGS.
> 
> > c) Test booting, and check what RAM size the kernel thinks you have.
> 
> see above. RAM detection works, but it's not what we want ...
> 
> > If that works, then ATAGs are the problem. We probably need to modify the
> > core ARM code not to use memory ATAGs when there is a DT?
> > 
> > I'm mainly pushing on this because adding "mem=" to the command-line in
> > the DT file isn't a great solution; when people start using bootloaders
> > that rewrite the DT to include the user-specified command-line, then every
> > user is going to have to start specifying "mem=" in their command-lines.
> 
> Well, I see two options for our case:
> 
> 	a) use the mem=448M at 0 in the DT so the gpu can get its memory
> 	b) upstream the chromeos changes to reserve gpu memory "on the fly" from 
> the autodetected 512M (as it should be IMHO).

b) is the ideal.  a) is absolutely wrong.  /memreserve/ should fix the
gpu memory problem when not passing a mem= parameter.  If /memreserve/
isn't working, then I'd like to know why.

g.

RE: RE: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Stephen Warren]

Russell King wrote at Saturday, October 29, 2011 2:44 AM:
> On Fri, Oct 28, 2011 at 09:49:49AM -0700, Stephen Warren wrote:
> > When boards boot from DT, there is no fixup function to override the
> > bootloader's ATAGs. I also see a bunch of code to set up the memory
> > information from DT e.g. setup_machine_fdt()'s call to:
> >
> > 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> >
> > ... but I assume that happens before the ATAGs are processed, and the
> > buggy ATAGs end up overriding the information in the DT file.
> 
> As far as the uncompressed kernel is concerned, there is either ATAG
> or DT information, never both.  If the boot loader provides ATAGs and
> the zImage has a DT appended to it, the zImage decompressor merges the
> ATAGs into the appended DT and passes the DT to the kernel.
> 
> So anyone who currently 'fixes' their broken boot loader via the fixup
> function by directly manipulating the ATAGS is going to hit the DT
> image instead.

I believe the PAZ00 fixup function runs on the kernel's internal data-
structures, not the ATAGs directly, so the issue you point out isn't
a problem here:

static void __init tegra_paz00_fixup(struct tag *tags, char **cmdline,
        struct meminfo *mi)
{
        mi->nr_banks = 1;
        mi->bank[0].start = PHYS_OFFSET;
        mi->bank[0].size = 448 * SZ_1M;
}

-- 
nvpublic

[PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Stephen Warren]

Russell King wrote at Saturday, October 29, 2011 2:44 AM:
> On Fri, Oct 28, 2011 at 09:49:49AM -0700, Stephen Warren wrote:
> > When boards boot from DT, there is no fixup function to override the
> > bootloader's ATAGs. I also see a bunch of code to set up the memory
> > information from DT e.g. setup_machine_fdt()'s call to:
> >
> > 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> >
> > ... but I assume that happens before the ATAGs are processed, and the
> > buggy ATAGs end up overriding the information in the DT file.
> 
> As far as the uncompressed kernel is concerned, there is either ATAG
> or DT information, never both.  If the boot loader provides ATAGs and
> the zImage has a DT appended to it, the zImage decompressor merges the
> ATAGs into the appended DT and passes the DT to the kernel.
> 
> So anyone who currently 'fixes' their broken boot loader via the fixup
> function by directly manipulating the ATAGS is going to hit the DT
> image instead.

I believe the PAZ00 fixup function runs on the kernel's internal data-
structures, not the ATAGs directly, so the issue you point out isn't
a problem here:

static void __init tegra_paz00_fixup(struct tag *tags, char **cmdline,
        struct meminfo *mi)
{
        mi->nr_banks = 1;
        mi->bank[0].start = PHYS_OFFSET;
        mi->bank[0].size = 448 * SZ_1M;
}

-- 
nvpublic

RE: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Stephen Warren]

Marc Dietrich wrote at Sunday, October 30, 2011 2:40 PM:
> On Friday 28 October 2011 09:49:49 Stephen Warren wrote:
> > Marc Dietrich wrote at Friday, October 28, 2011 4:30 AM:
> > > Am Donnerstag, 27. Oktober 2011, 09:50:03 schrieb Stephen Warren:
> > > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > > > This adds a dts file for paz00. As a side effect, this also
> > > > > enables
> > > > > the embedded controller which controls the keyboard, touchpad,
> > > > > power,
> > > > > leds, and some other functions.
> > > >
> > > > ...
> > > >
> > > > > +++ b/arch/arm/boot/dts/tegra-paz00.dts
> > > > > @@ -0,0 +1,70 @@
> > > > > +/dts-v1/;
> > > > > +
> > > > > +/memreserve/ 0x1c000000 0x04000000;
> > > > > +/include/ "tegra20.dtsi"
> > > > > +
> > > > > +/ {
> > > > > +	model = "Toshiba AC100 / Dynabook AZ";
> > > > > +	compatible = "compal,paz00", "nvidia,tegra20";
> > > > > +
> > > > > +	chosen {
> > > > > +		bootargs = "mem=448@0 console=ttyS0,115200n8
> > > > > root=/dev/mmcblk1p1";
> > > >
> > > > You shouldn't need the mem= parameter here; it wasn't in your first
> > > > patch set.
> > > that's because I forgot it. Sorry, I didn't mentioned it in the
> > > changelog. I wonder why mem= is still needed.
> >
> > I wonder if this is some conflict between ATAGs and the DT-specified
> > memory node.
> >
> > As far as I can tell, the kernel's memory information typically comes
> > from ATAGs. Some boards override what's in the ATAGs in their machine
> > descriptor's fixup routine, e.g. see tegra_paz00_fixup(). Presumably,
> > this is because of buggy bootloaders sending incorrect memory information
> > in the ATAGs. Do you have any way to check the ATAGs that the bootloader
> > is sending? Probably, you could enable DEBUG_LL and using the low-level
> > debugging functions to dump some of that information to the UART early
> > during boot.
> 
> I got the ATAGS from /proc/atags and there is no memory entry there, only
> initrd and cmdline (which is the one from nvflash).
> 
> The machine also has a fixup routine, but it also boots without (I'm sure it
> was necessary in the past),
...
> I tested several variations. Without DT, the fixup can compensate a missing
> mem entry in the kernel command line, but with a mem= from the kernel command
> line, a fixup is not needed.

OK, that makes sense.

> With DT, the command line specified from nvflash is ignored and the one from
> DT comes into the game.

I assume you're using CONFIG_ARM_APPENDED_DTB but not CONFIG_ARM_ATAG_DTB_COMPAT.

> If it is also missing there, system detects 512 MB.

That makes sense; that's the value in the .dts file you posted.

> (which is physical right, but we cannot reserve memory for the gpu). The fixup
> is indeed ignored in this case.

It's not "ignored" as such.

When booting without DT, the machine descriptor in board-paz00.c is used,
since the descriptor is selected based on the machine ID in the descriptor,
and that descriptor includes:

	.fixup          = tegra_paz00_fixup,

... which causes that fixup function to be called.

When booting with DT, the machine descriptor in board-dt.c is used, since
the descriptor is selected based on the DT's overall compatible property,
and that descriptor doesn't refer to tegra_paz00_fixup() at all, so it's
not called.

> > b) Modify arch/arm/kernel/setup.c:parse_tag_mem32() to do nothing;
> >    comment out the call to arm_add_memory()
> 
> I leave this out because the bootloader does not send memory info in the
> ATAGS.
> 
> > c) Test booting, and check what RAM size the kernel thinks you have.
> 
> see above. RAM detection works, but it's not what we want ...

At this point, I'd argue that exposing the full 512M to the kernel /is/
what we want. There is no Tegra GPU support in the mainline kernel at
present. As such, I'd argue that we should give the entire RAM space to
the kernel to use. As and when we add GPU support to the kernel, we can
use an appropriate mechanism to take RAM away from the kernel for that
purpose.

So, I'm planning to remove all the /memreserve/ entries from the Tegra
.dts files as soon as I can find a minute to do so.

But, as Grant mentioned, the /memreserve/ .dts directive should work
right now to reserve memory. I'm not sure if you'd tried that and it didn't
work? If so, it'd be good to debug that just to make sure the mechanism
works, even if we don't intend to use it.

-- 
nvpublic

[PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Stephen Warren]

Marc Dietrich wrote at Sunday, October 30, 2011 2:40 PM:
> On Friday 28 October 2011 09:49:49 Stephen Warren wrote:
> > Marc Dietrich wrote at Friday, October 28, 2011 4:30 AM:
> > > Am Donnerstag, 27. Oktober 2011, 09:50:03 schrieb Stephen Warren:
> > > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > > > This adds a dts file for paz00. As a side effect, this also
> > > > > enables
> > > > > the embedded controller which controls the keyboard, touchpad,
> > > > > power,
> > > > > leds, and some other functions.
> > > >
> > > > ...
> > > >
> > > > > +++ b/arch/arm/boot/dts/tegra-paz00.dts
> > > > > @@ -0,0 +1,70 @@
> > > > > +/dts-v1/;
> > > > > +
> > > > > +/memreserve/ 0x1c000000 0x04000000;
> > > > > +/include/ "tegra20.dtsi"
> > > > > +
> > > > > +/ {
> > > > > +	model = "Toshiba AC100 / Dynabook AZ";
> > > > > +	compatible = "compal,paz00", "nvidia,tegra20";
> > > > > +
> > > > > +	chosen {
> > > > > +		bootargs = "mem=448 at 0 console=ttyS0,115200n8
> > > > > root=/dev/mmcblk1p1";
> > > >
> > > > You shouldn't need the mem= parameter here; it wasn't in your first
> > > > patch set.
> > > that's because I forgot it. Sorry, I didn't mentioned it in the
> > > changelog. I wonder why mem= is still needed.
> >
> > I wonder if this is some conflict between ATAGs and the DT-specified
> > memory node.
> >
> > As far as I can tell, the kernel's memory information typically comes
> > from ATAGs. Some boards override what's in the ATAGs in their machine
> > descriptor's fixup routine, e.g. see tegra_paz00_fixup(). Presumably,
> > this is because of buggy bootloaders sending incorrect memory information
> > in the ATAGs. Do you have any way to check the ATAGs that the bootloader
> > is sending? Probably, you could enable DEBUG_LL and using the low-level
> > debugging functions to dump some of that information to the UART early
> > during boot.
> 
> I got the ATAGS from /proc/atags and there is no memory entry there, only
> initrd and cmdline (which is the one from nvflash).
> 
> The machine also has a fixup routine, but it also boots without (I'm sure it
> was necessary in the past),
...
> I tested several variations. Without DT, the fixup can compensate a missing
> mem entry in the kernel command line, but with a mem= from the kernel command
> line, a fixup is not needed.

OK, that makes sense.

> With DT, the command line specified from nvflash is ignored and the one from
> DT comes into the game.

I assume you're using CONFIG_ARM_APPENDED_DTB but not CONFIG_ARM_ATAG_DTB_COMPAT.

> If it is also missing there, system detects 512 MB.

That makes sense; that's the value in the .dts file you posted.

> (which is physical right, but we cannot reserve memory for the gpu). The fixup
> is indeed ignored in this case.

It's not "ignored" as such.

When booting without DT, the machine descriptor in board-paz00.c is used,
since the descriptor is selected based on the machine ID in the descriptor,
and that descriptor includes:

	.fixup          = tegra_paz00_fixup,

... which causes that fixup function to be called.

When booting with DT, the machine descriptor in board-dt.c is used, since
the descriptor is selected based on the DT's overall compatible property,
and that descriptor doesn't refer to tegra_paz00_fixup() at all, so it's
not called.

> > b) Modify arch/arm/kernel/setup.c:parse_tag_mem32() to do nothing;
> >    comment out the call to arm_add_memory()
> 
> I leave this out because the bootloader does not send memory info in the
> ATAGS.
> 
> > c) Test booting, and check what RAM size the kernel thinks you have.
> 
> see above. RAM detection works, but it's not what we want ...

At this point, I'd argue that exposing the full 512M to the kernel /is/
what we want. There is no Tegra GPU support in the mainline kernel at
present. As such, I'd argue that we should give the entire RAM space to
the kernel to use. As and when we add GPU support to the kernel, we can
use an appropriate mechanism to take RAM away from the kernel for that
purpose.

So, I'm planning to remove all the /memreserve/ entries from the Tegra
.dts files as soon as I can find a minute to do so.

But, as Grant mentioned, the /memreserve/ .dts directive should work
right now to reserve memory. I'm not sure if you'd tried that and it didn't
work? If so, it'd be good to debug that just to make sure the mechanism
works, even if we don't intend to use it.

-- 
nvpublic

RE: [PATCH v2 3/3] staging: nvec: add device tree support

[Stephen Warren]

Marc Dietrich wrote at Sunday, October 30, 2011 2:58 PM:
> On Friday 28 October 2011 09:56:41 you wrote:
> > Marc Dietrich wrote at Friday, October 28, 2011 5:02 AM:
> > > Am Donnerstag, 27. Oktober 2011, 12:17:25 schrieb Stephen Warren:
> > > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > > > This adds device tree support to the nvec driver. By using this
> > > > > method it is no longer necessary to specify platform data
> > > > > through a board file.
> >
> > ...
> >
> > > > > +/* Match table for of_platform binding */
> > > > > +static const struct of_device_id nvidia_nvec_of_match[]
> > > > > __devinitconst = { +	{ .compatible = "nvidia,nvec", },
...
> > I'd like to call this "nvidia,nvec-1.0" to version this compatible
> > property; that's the specification version in the latest document that
> > I saw. While we do seemed to have abandoned this approach, I want to
> > make sure this is extensible if someone suddenly decides to go back to
> > it and creates a 2.0 in the future. Does that seem reasonable?
> 
> mmh, I can't see why we should add it now. There is no V2 I can see in my
> limited view. If your company plans to expand the protocol you can either
> enhance our driver or create a new one (nvec2), which can add a nvidia,nvec-2
> compatibility property (we can also change ours to nvidia,nvec-1 at the same
> time, but that's not required).

We can't rename anything in DT once we've started using it; if we release
a new kernel that changes (rather than just adds to) the compatible values
it supports, that'd cause old DT files to cease to operate against the new
kernel.

I suppose nvidia,nvec is fine. We can always add nvidia,nvec2 if we do
rev the protocol, and have the existing unnumbered value implicitly be
version 1.

> > That said, there are apparently some OEMs who did change the protocol
> > and do something slightly different. I'm trying to confirm whether PAZ00
> > was one of them. I guess not if PAZ00 works with the standard driver that
> > you linked to.
> 
> There are so called OEM commands which we will move to a board specific nvec
> file (e.g. nvec_paz00.c). We haven't got the chance to test it on other boards
> using it yet (e.g. toshiba folio, advent vega, ...). The tablets are mostly
> using it for power control and maybe also leds/switches. I don't think any
> other board uses keyboard / mouse functions.

It sounded like some OEMS had used a completely different protocol rather
than just making use of the OEM commands. Still, it isn't entirely clear
whether that's true yet. I don't think it impacts this driver/board, so
we can just ignore this for now.

-- 
nvpublic

Re: [PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Marc Dietrich]

ok, lets bring this to an end.

On Monday 31 October 2011 09:09:13 Stephen Warren wrote:
> Marc Dietrich wrote at Sunday, October 30, 2011 2:40 PM:
> > On Friday 28 October 2011 09:49:49 Stephen Warren wrote:
> > > Marc Dietrich wrote at Friday, October 28, 2011 4:30 AM:
> > > > Am Donnerstag, 27. Oktober 2011, 09:50:03 schrieb Stephen Warren:
> > > > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > > > > This adds a dts file for paz00. As a side effect, this
> > > > > > also enables the embedded controller which controls the keyboard,
> > > > > > touchpad, power, leds, and some other functions.
> > > > > > ...
> > > > > > +++ b/arch/arm/boot/dts/tegra-paz00.dts
> > > > > > @@ -0,0 +1,70 @@
> > > > > > +/dts-v1/;
> > > > > > +
> > > > > > +/memreserve/ 0x1c000000 0x04000000;
> > > > > > +/include/ "tegra20.dtsi"
> > > > > > +
> > > > > > +/ {
> > > > > > +	model = "Toshiba AC100 / Dynabook AZ";
> > > > > > +	compatible = "compal,paz00", "nvidia,tegra20";
> > > > > > +
> > > > > > +	chosen {
> > > > > > +		bootargs = "mem=448@0 console=ttyS0,115200n8
> > > > > > root=/dev/mmcblk1p1";
> > > > > 
> > > > > You shouldn't need the mem= parameter here; it wasn't in
> > > > > your first patch set.
> > > > 
> > > > that's because I forgot it. Sorry, I didn't mentioned it in the
> > > > changelog. I wonder why mem= is still needed.
> > > 
> > > I wonder if this is some conflict between ATAGs and the DT-specified
> > > memory node.
> > > 
> > > As far as I can tell, the kernel's memory information typically
> > > comes from ATAGs. Some boards override what's in the ATAGs in their
> > > machine descriptor's fixup routine, e.g. see tegra_paz00_fixup().
> > > Presumably, this is because of buggy bootloaders sending incorrect
> > > memory information in the ATAGs. Do you have any way to check the ATAGs
> > > that the bootloader is sending? Probably, you could enable DEBUG_LL
> > > and using the low-level debugging functions to dump some of that
> > > information to the UART early during boot.
> > 
> > I got the ATAGS from /proc/atags and there is no memory entry there,
> > only
> > initrd and cmdline (which is the one from nvflash).
> > 
> > The machine also has a fixup routine, but it also boots without (I'm
> > sure it was necessary in the past),
> 
> ...
> 
> > I tested several variations. Without DT, the fixup can compensate a
> > missing mem entry in the kernel command line, but with a mem= from the
> > kernel command line, a fixup is not needed.
> 
> OK, that makes sense.
> 
> > With DT, the command line specified from nvflash is ignored and the one
> > from DT comes into the game.
> 
> I assume you're using CONFIG_ARM_APPENDED_DTB but not
> CONFIG_ARM_ATAG_DTB_COMPAT.
> > If it is also missing there, system detects 512 MB.

correct.

> That makes sense; that's the value in the .dts file you posted.
> 
> > (which is physical right, but we cannot reserve memory for the gpu). The
> > fixup is indeed ignored in this case.
> 
> It's not "ignored" as such.
> 
> When booting without DT, the machine descriptor in board-paz00.c is used,
> since the descriptor is selected based on the machine ID in the descriptor,
> and that descriptor includes:
> 
> 	.fixup          = tegra_paz00_fixup,
> 
> ... which causes that fixup function to be called.
> 
> When booting with DT, the machine descriptor in board-dt.c is used, since
> the descriptor is selected based on the DT's overall compatible property,
> and that descriptor doesn't refer to tegra_paz00_fixup() at all, so it's
> not called.

ok, I think I understood now. Thanks for the detailed explanation.

> >  ...
> >
> > > c) Test booting, and check what RAM size the kernel thinks you have.
> > 
> > see above. RAM detection works, but it's not what we want ...
> 
> At this point, I'd argue that exposing the full 512M to the kernel /is/
> what we want. There is no Tegra GPU support in the mainline kernel at
> present. As such, I'd argue that we should give the entire RAM space to
> the kernel to use. As and when we add GPU support to the kernel, we can
> use an appropriate mechanism to take RAM away from the kernel for that
> purpose.

sound fine. I guess the vmalloc is also due to some gpu requirements...

> So, I'm planning to remove all the /memreserve/ entries from the Tegra
> .dts files as soon as I can find a minute to do so.
> 
> But, as Grant mentioned, the /memreserve/ .dts directive should work
> right now to reserve memory. I'm not sure if you'd tried that and it didn't
> work? If so, it'd be good to debug that just to make sure the mechanism
> works, even if we don't intend to use it.

actually, it works as Grant said. For some reasons it didn't worked here when 
I tested it. Maybe I just catched a bad setup of linux-next/bootloader/kernel 
arguments.

Thanks for you patience with me...

Marc

[PATCH v2 2/3] arm/dt: tegra: add dts file for paz00

[Marc Dietrich]

ok, lets bring this to an end.

On Monday 31 October 2011 09:09:13 Stephen Warren wrote:
> Marc Dietrich wrote at Sunday, October 30, 2011 2:40 PM:
> > On Friday 28 October 2011 09:49:49 Stephen Warren wrote:
> > > Marc Dietrich wrote at Friday, October 28, 2011 4:30 AM:
> > > > Am Donnerstag, 27. Oktober 2011, 09:50:03 schrieb Stephen Warren:
> > > > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM:
> > > > > > This adds a dts file for paz00. As a side effect, this
> > > > > > also enables the embedded controller which controls the keyboard,
> > > > > > touchpad, power, leds, and some other functions.
> > > > > > ...
> > > > > > +++ b/arch/arm/boot/dts/tegra-paz00.dts
> > > > > > @@ -0,0 +1,70 @@
> > > > > > +/dts-v1/;
> > > > > > +
> > > > > > +/memreserve/ 0x1c000000 0x04000000;
> > > > > > +/include/ "tegra20.dtsi"
> > > > > > +
> > > > > > +/ {
> > > > > > +	model = "Toshiba AC100 / Dynabook AZ";
> > > > > > +	compatible = "compal,paz00", "nvidia,tegra20";
> > > > > > +
> > > > > > +	chosen {
> > > > > > +		bootargs = "mem=448 at 0 console=ttyS0,115200n8
> > > > > > root=/dev/mmcblk1p1";
> > > > > 
> > > > > You shouldn't need the mem= parameter here; it wasn't in
> > > > > your first patch set.
> > > > 
> > > > that's because I forgot it. Sorry, I didn't mentioned it in the
> > > > changelog. I wonder why mem= is still needed.
> > > 
> > > I wonder if this is some conflict between ATAGs and the DT-specified
> > > memory node.
> > > 
> > > As far as I can tell, the kernel's memory information typically
> > > comes from ATAGs. Some boards override what's in the ATAGs in their
> > > machine descriptor's fixup routine, e.g. see tegra_paz00_fixup().
> > > Presumably, this is because of buggy bootloaders sending incorrect
> > > memory information in the ATAGs. Do you have any way to check the ATAGs
> > > that the bootloader is sending? Probably, you could enable DEBUG_LL
> > > and using the low-level debugging functions to dump some of that
> > > information to the UART early during boot.
> > 
> > I got the ATAGS from /proc/atags and there is no memory entry there,
> > only
> > initrd and cmdline (which is the one from nvflash).
> > 
> > The machine also has a fixup routine, but it also boots without (I'm
> > sure it was necessary in the past),
> 
> ...
> 
> > I tested several variations. Without DT, the fixup can compensate a
> > missing mem entry in the kernel command line, but with a mem= from the
> > kernel command line, a fixup is not needed.
> 
> OK, that makes sense.
> 
> > With DT, the command line specified from nvflash is ignored and the one
> > from DT comes into the game.
> 
> I assume you're using CONFIG_ARM_APPENDED_DTB but not
> CONFIG_ARM_ATAG_DTB_COMPAT.
> > If it is also missing there, system detects 512 MB.

correct.

> That makes sense; that's the value in the .dts file you posted.
> 
> > (which is physical right, but we cannot reserve memory for the gpu). The
> > fixup is indeed ignored in this case.
> 
> It's not "ignored" as such.
> 
> When booting without DT, the machine descriptor in board-paz00.c is used,
> since the descriptor is selected based on the machine ID in the descriptor,
> and that descriptor includes:
> 
> 	.fixup          = tegra_paz00_fixup,
> 
> ... which causes that fixup function to be called.
> 
> When booting with DT, the machine descriptor in board-dt.c is used, since
> the descriptor is selected based on the DT's overall compatible property,
> and that descriptor doesn't refer to tegra_paz00_fixup() at all, so it's
> not called.

ok, I think I understood now. Thanks for the detailed explanation.

> >  ...
> >
> > > c) Test booting, and check what RAM size the kernel thinks you have.
> > 
> > see above. RAM detection works, but it's not what we want ...
> 
> At this point, I'd argue that exposing the full 512M to the kernel /is/
> what we want. There is no Tegra GPU support in the mainline kernel at
> present. As such, I'd argue that we should give the entire RAM space to
> the kernel to use. As and when we add GPU support to the kernel, we can
> use an appropriate mechanism to take RAM away from the kernel for that
> purpose.

sound fine. I guess the vmalloc is also due to some gpu requirements...

> So, I'm planning to remove all the /memreserve/ entries from the Tegra
> .dts files as soon as I can find a minute to do so.
> 
> But, as Grant mentioned, the /memreserve/ .dts directive should work
> right now to reserve memory. I'm not sure if you'd tried that and it didn't
> work? If so, it'd be good to debug that just to make sure the mechanism
> works, even if we don't intend to use it.

actually, it works as Grant said. For some reasons it didn't worked here when 
I tested it. Maybe I just catched a bad setup of linux-next/bootloader/kernel 
arguments.

Thanks for you patience with me...

Marc