[PATCH v3 02/18] fdt: Add header guard to fdtdec.h

[PATCH v3 02/18] fdt: Add header guard to fdtdec.h

[Simon Glass]

This makes it easier to include this header from other headers.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/fdtdec.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index fab577e..c30947a 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -19,6 +19,8 @@
  * MA 02111-1307 USA
  */
 
+#ifndef __fdtdec_h
+#define __fdtdec_h
 
 /*
  * This file contains convenience functions for decoding useful and
@@ -382,3 +384,4 @@ int fdtdec_get_byte_array(const void *blob, int node, const char *prop_name,
  */
 const u8 *fdtdec_locate_byte_array(const void *blob, int node,
 			     const char *prop_name, int count);
+#endif
-- 
1.7.7.3

Re: [PATCH v3 02/18] fdt: Add header guard to fdtdec.h

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 410 bytes --]

On Thursday 12 July 2012 11:25:02 Simon Glass wrote:
> This makes it easier to include this header from other headers.

Acked-by: Mike Frysinger <vapier@gentoo.org>

> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
>
>  const u8 *fdtdec_locate_byte_array(const void *blob, int node,
>  			     const char *prop_name, int count);
> +#endif

personally, i like seeing a blank line before the final #endif
-mike

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

[-- Attachment #2: Type: text/plain, Size: 134 bytes --]

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[PATCH v3 01/18] fdt: Tidy debugging, add to fdtdec_get_int/addr()

[Simon Glass]

The new debugging shows the value of integers and addresses read
from the device tree and tidy up GPIO output.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v3:
- Remove spurious newline from fdtdec_get_addr() debug output
- Tidy up fdtdec_decode_gpios() debug output

 lib/fdtdec.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index cc09e06..45a0fcf 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -80,11 +80,16 @@ fdt_addr_t fdtdec_get_addr(const void *blob, int node,
 	const fdt_addr_t *cell;
 	int len;
 
-	debug("get_addr: %s\n", prop_name);
+	debug("%s: %s: ", __func__, prop_name);
 	cell = fdt_getprop(blob, node, prop_name, &len);
 	if (cell && (len == sizeof(fdt_addr_t) ||
-			len == sizeof(fdt_addr_t) * 2))
-		return fdt_addr_to_cpu(*cell);
+			len == sizeof(fdt_addr_t) * 2)) {
+		fdt_addr_t addr = fdt_addr_to_cpu(*cell);
+
+		debug("%p\n", (void *)addr);
+		return addr;
+	}
+	debug("(not found)\n");
 	return FDT_ADDR_T_NONE;
 }
 
@@ -94,10 +99,15 @@ s32 fdtdec_get_int(const void *blob, int node, const char *prop_name,
 	const s32 *cell;
 	int len;
 
-	debug("get_size: %s\n", prop_name);
+	debug("%s: %s: ", __func__, prop_name);
 	cell = fdt_getprop(blob, node, prop_name, &len);
-	if (cell && len >= sizeof(s32))
-		return fdt32_to_cpu(cell[0]);
+	if (cell && len >= sizeof(s32)) {
+		s32 val = fdt32_to_cpu(cell[0]);
+
+		debug("%#x (%d)\n", val, val);
+		return val;
+	}
+	debug("(not found)\n");
 	return default_val;
 }
 
@@ -329,6 +339,7 @@ int fdtdec_lookup_phandle(const void *blob, int node, const char *prop_name)
 	const u32 *phandle;
 	int lookup;
 
+	debug("%s: %s\n", __func__, prop_name);
 	phandle = fdt_getprop(blob, node, prop_name, NULL);
 	if (!phandle)
 		return -FDT_ERR_NOTFOUND;
@@ -428,7 +439,7 @@ static int fdtdec_decode_gpios(const void *blob, int node,
 	assert(max_count > 0);
 	prop = fdt_get_property(blob, node, prop_name, &len);
 	if (!prop) {
-		debug("FDT: %s: property '%s' missing\n", __func__, prop_name);
+		debug("%s: property '%s' missing\n", __func__, prop_name);
 		return -FDT_ERR_NOTFOUND;
 	}
 
@@ -437,7 +448,7 @@ static int fdtdec_decode_gpios(const void *blob, int node,
 	cell = (u32 *)prop->data;
 	len /= sizeof(u32) * 3;		/* 3 cells per GPIO record */
 	if (len > max_count) {
-		debug("FDT: %s: too many GPIOs / cells for "
+		debug(" %s: too many GPIOs / cells for "
 			"property '%s'\n", __func__, prop_name);
 		return -FDT_ERR_BADLAYOUT;
 	}
-- 
1.7.7.3

[PATCH v3 05/18] tegra: fdt: Add pwm binding and node

[Simon Glass]

This binding will apparently soon be in linux-next. Bring it in now
since we need to do something, and may as well try to target what
Linux will have.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v3:
- Add new commit for pwm binding and node

 arch/arm/dts/tegra20.dtsi                    |    7 +++++++
 doc/device-tree-bindings/pwm/tegra20-pwm.txt |   18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)
 create mode 100644 doc/device-tree-bindings/pwm/tegra20-pwm.txt

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index f95be58..e7d1688 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -204,4 +204,11 @@
 		compatible = "nvidia,tegra20-kbc";
 		reg = <0x7000e200 0x0078>;
 	};
+
+	pwm: pwm@7000a000 {
+		compatible = "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
+
 };
diff --git a/doc/device-tree-bindings/pwm/tegra20-pwm.txt b/doc/device-tree-bindings/pwm/tegra20-pwm.txt
new file mode 100644
index 0000000..bbbeedb
--- /dev/null
+++ b/doc/device-tree-bindings/pwm/tegra20-pwm.txt
@@ -0,0 +1,18 @@
+Tegra SoC PWFM controller
+
+Required properties:
+- compatible: should be one of:
+  - "nvidia,tegra20-pwm"
+  - "nvidia,tegra30-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: On Tegra the number of cells used to specify a PWM is 2. The
+  first cell specifies the per-chip index of the PWM to use and the second
+  cell is the duty cycle in nanoseconds.
+
+Example:
+
+	pwm: pwm@7000a000 {
+		compatible = "nvidia,tegra20-pwm";
+		reg = <0x7000a000 0x100>;
+		#pwm-cells = <2>;
+	};
-- 
1.7.7.3

[PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Simon Glass]

Add LCD definitions and also a proposed binding for LCD displays.

The PWM is as per what will likely be committed to linux-next soon.

The displaymode binding comes from a proposal here:

http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html

The panel binding is new, and fills a need to specify the panel
timings and other tegra-specific information. Should a binding appear
that allows the pwm to handle this automatically, we can revisit
this.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v2:
- Add nvidia prefix to device tree properties

Changes in v3:
- Add new panel binding to fit with tegra display controller binding
- Bring in proposed tegra display controller binding
- Use displaymode binding for fdt

 arch/arm/dts/tegra20.dtsi                      |   89 ++++++++++++++++++++++++
 doc/device-tree-bindings/video/displaymode.txt |   42 +++++++++++
 doc/device-tree-bindings/video/tegra20-dc.txt  |   89 ++++++++++++++++++++++++
 3 files changed, 220 insertions(+), 0 deletions(-)
 create mode 100644 doc/device-tree-bindings/video/displaymode.txt
 create mode 100644 doc/device-tree-bindings/video/tegra20-dc.txt

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index e7d1688..0b2ca3b 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -211,4 +211,93 @@
 		#pwm-cells = <2>;
 	};
 
+	host1x {
+		compatible = "nvidia,tegra20-host1x", "simple-bus";
+		reg = <0x50000000 0x00024000>;
+		interrupts = <0 65 0x04   /* mpcore syncpt */
+			      0 67 0x04>; /* mpcore general */
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		ranges = <0x54000000 0x54000000 0x04000000>;
+
+		/* video-encoding/decoding */
+		mpe {
+			reg = <0x54040000 0x00040000>;
+			interrupts = <0 68 0x04>;
+		};
+
+		/* video input */
+		vi {
+			reg = <0x54080000 0x00040000>;
+			interrupts = <0 69 0x04>;
+		};
+
+		/* EPP */
+		epp {
+			reg = <0x540c0000 0x00040000>;
+			interrupts = <0 70 0x04>;
+		};
+
+		/* ISP */
+		isp {
+			reg = <0x54100000 0x00040000>;
+			interrupts = <0 71 0x04>;
+		};
+
+		/* 2D engine */
+		gr2d {
+			reg = <0x54140000 0x00040000>;
+			interrupts = <0 72 0x04>;
+		};
+
+		/* 3D engine */
+		gr3d {
+			reg = <0x54180000 0x00040000>;
+		};
+
+		/* display controllers */
+		dc@54200000 {
+			compatible = "nvidia,tegra20-dc";
+			reg = <0x54200000 0x00040000>;
+			interrupts = <0 73 0x04>;
+
+			rgb {
+				status = "disabled";
+			};
+		};
+
+		dc@54240000 {
+			compatible = "nvidia,tegra20-dc";
+			reg = <0x54240000 0x00040000>;
+			interrupts = <0 74 0x04>;
+
+			rgb {
+				status = "disabled";
+			};
+		};
+
+		/* outputs */
+		hdmi {
+			compatible = "nvidia,tegra20-hdmi";
+			reg = <0x54280000 0x00040000>;
+			interrupts = <0 75 0x04>;
+			status = "disabled";
+		};
+
+		tvo {
+			compatible = "nvidia,tegra20-tvo";
+			reg = <0x542c0000 0x00040000>;
+			interrupts = <0 76 0x04>;
+			status = "disabled";
+		};
+
+		dsi {
+			compatible = "nvidia,tegra20-dsi";
+			reg = <0x54300000 0x00040000>;
+			status = "disabled";
+		};
+	};
+
 };
diff --git a/doc/device-tree-bindings/video/displaymode.txt b/doc/device-tree-bindings/video/displaymode.txt
new file mode 100644
index 0000000..45ca42d
--- /dev/null
+++ b/doc/device-tree-bindings/video/displaymode.txt
@@ -0,0 +1,42 @@
+videomode bindings
+==================
+
+(from http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html)
+
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing
+   parameters in pixels
+ - upper-margin, lower-margin, vsync-len: Vertical display timing
+   parameters in lines
+ - clock: display clock in Hz
+
+Optional properties:
+ - width-mm, height-mm: Display dimensions in mm
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
+ - interlaced (bool): This is an interlaced mode
+ - doublescan (bool): This is a doublescan mode
+
+There are different ways of describing a display mode. The devicetree
+representation corresponds to the one used by the Linux Framebuffer
+framework described here in Documentation/fb/framebuffer.txt. This
+representation has been chosen because it's the only format which does
+not allow for inconsistent parameters. Unlike the Framebuffer framework
+the devicetree has the clock in Hz instead of ps.
+
+Example:
+
+	display@0 {
+		/* 1920x1080p24 */
+		clock = <52000000>;
+		xres = <1920>;
+		yres = <1080>;
+		left-margin = <25>;
+		right-margin = <25>;
+		hsync-len = <25>;
+		lower-margin = <2>;
+		upper-margin = <2>;
+		vsync-len = <2>;
+		hsync-active-high;
+	};
diff --git a/doc/device-tree-bindings/video/tegra20-dc.txt b/doc/device-tree-bindings/video/tegra20-dc.txt
new file mode 100644
index 0000000..dcfd652
--- /dev/null
+++ b/doc/device-tree-bindings/video/tegra20-dc.txt
@@ -0,0 +1,89 @@
+Display Controller
+------------------
+
+(there isn't yet a generic binding in Linux, so this describes what is in
+U-Boot, and may change based on Linux activity)
+
+The device node for a display device is as described in the document
+"Open Firmware Recommended Practice : Universal Serial Bus" with the
+following modifications and additions :
+
+Required properties :
+ - compatible : Should be "nvidia,tegra20-dc"
+
+Required subnode 'rgb'. This has the properties listed in displaymode.txt
+as well as:
+
+Required properties (rgb) :
+ - nvidia,panel : phandle of LCD panel information
+ - nvidia,bits-per-pixel: number of bits per pixel (depth)
+
+Optional properties (rgb):
+ - nvidia,frame-buffer: address of frame buffer (if omitted it will be
+		calculated)
+	- This may be useful to share an address between U-Boot and Linux and
+		avoid boot-time corruption / flicker
+
+
+The panel node describes the panel itself.
+
+Required properties (panel) :
+ - nvidia,pwm : pwm to use to set display contrast (see tegra20-pwm.txt)
+ - nvidia,panel-timings: 4 cells containing required timings in ms:
+	* delay between panel_vdd-rise and data-rise
+	* delay between data-rise and backlight_vdd-rise
+	* delay between backlight_vdd and pwm-rise
+	* delay between pwm-rise and backlight_en-rise
+
+Optional GPIO properies all have (phandle, GPIO number, flags):
+ - nvidia,backlight-enable-gpios: backlight enable GPIO
+ - nvidia,lvds-shutdown-gpios: LVDS power shutdown GPIO
+ - nvidia,backlight-vdd-gpios: backlight power GPIO
+ - nvidia,panel-vdd-gpios: panel power GPIO
+
+Example:
+
+host1x {
+	compatible = "nvidia,tegra20-host1x", "simple-bus";
+	reg = <0x50000000 0x00024000>;
+	interrupts = <0 65 0x04   /* mpcore syncpt */
+			0 67 0x04>; /* mpcore general */
+
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	ranges = <0x54000000 0x54000000 0x04000000>;
+
+	dc@54200000 {
+		compatible = "nvidia,tegra20-dc";
+		reg = <0x54200000 0x00040000>;
+		interrupts = <0 73 0x04>;
+
+		rgb {
+			status = "okay";
+			/* Seaboard has 1366x768 */
+			clock = <70600000>;
+			xres = <1366>;
+			yres = <768>;
+			left-margin = <58>;
+			right-margin = <58>;
+			hsync-len = <58>;
+			lower-margin = <4>;
+			upper-margin = <4>;
+			vsync-len = <4>;
+			hsync-active-high;
+			nvidia,frame-buffer = <0x2f680000>;
+			nvidia,bits-per-pixel = <16>;
+			nvidia,panel = <&lcd_panel>;
+		};
+	};
+};
+
+lcd_panel: panel {
+	nvidia,pwm = <&pwm 2 0>;
+	nvidia,backlight-enable-gpios = <&gpio 28 0>;	/* PD4 */
+	nvidia,lvds-shutdown-gpios = <&gpio 10 0>;	/* PB2 */
+	nvidia,backlight-vdd-gpios = <&gpio 176 0>;	/* PW0 */
+	nvidia,panel-vdd-gpios = <&gpio 22 0>;		/* PC6 */
+	nvidia,panel-timings = <4 203 17 15>;
+};
-- 
1.7.7.3

Re: [PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Simon Glass]

+Thierry

Hi,

On Thu, Jul 12, 2012 at 4:25 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Add LCD definitions and also a proposed binding for LCD displays.
>
> The PWM is as per what will likely be committed to linux-next soon.
>
> The displaymode binding comes from a proposal here:
>
> http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html
>
> The panel binding is new, and fills a need to specify the panel
> timings and other tegra-specific information. Should a binding appear
> that allows the pwm to handle this automatically, we can revisit
> this.
>

Any comments on this binding please? The main addition from Thierry's
one posted on LMKL is the LCD resolution selection.

Regards,
Simon

> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> Changes in v2:
> - Add nvidia prefix to device tree properties
>
> Changes in v3:
> - Add new panel binding to fit with tegra display controller binding
> - Bring in proposed tegra display controller binding
> - Use displaymode binding for fdt
>
>  arch/arm/dts/tegra20.dtsi                      |   89 ++++++++++++++++++++++++
>  doc/device-tree-bindings/video/displaymode.txt |   42 +++++++++++
>  doc/device-tree-bindings/video/tegra20-dc.txt  |   89 ++++++++++++++++++++++++
>  3 files changed, 220 insertions(+), 0 deletions(-)
>  create mode 100644 doc/device-tree-bindings/video/displaymode.txt
>  create mode 100644 doc/device-tree-bindings/video/tegra20-dc.txt
>
> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
> index e7d1688..0b2ca3b 100644
> --- a/arch/arm/dts/tegra20.dtsi
> +++ b/arch/arm/dts/tegra20.dtsi
> @@ -211,4 +211,93 @@
>                 #pwm-cells = <2>;
>         };
>
> +       host1x {
> +               compatible = "nvidia,tegra20-host1x", "simple-bus";
> +               reg = <0x50000000 0x00024000>;
> +               interrupts = <0 65 0x04   /* mpcore syncpt */
> +                             0 67 0x04>; /* mpcore general */
> +
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +
> +               ranges = <0x54000000 0x54000000 0x04000000>;
> +
> +               /* video-encoding/decoding */
> +               mpe {
> +                       reg = <0x54040000 0x00040000>;
> +                       interrupts = <0 68 0x04>;
> +               };
> +
> +               /* video input */
> +               vi {
> +                       reg = <0x54080000 0x00040000>;
> +                       interrupts = <0 69 0x04>;
> +               };
> +
> +               /* EPP */
> +               epp {
> +                       reg = <0x540c0000 0x00040000>;
> +                       interrupts = <0 70 0x04>;
> +               };
> +
> +               /* ISP */
> +               isp {
> +                       reg = <0x54100000 0x00040000>;
> +                       interrupts = <0 71 0x04>;
> +               };
> +
> +               /* 2D engine */
> +               gr2d {
> +                       reg = <0x54140000 0x00040000>;
> +                       interrupts = <0 72 0x04>;
> +               };
> +
> +               /* 3D engine */
> +               gr3d {
> +                       reg = <0x54180000 0x00040000>;
> +               };
> +
> +               /* display controllers */
> +               dc@54200000 {
> +                       compatible = "nvidia,tegra20-dc";
> +                       reg = <0x54200000 0x00040000>;
> +                       interrupts = <0 73 0x04>;
> +
> +                       rgb {
> +                               status = "disabled";
> +                       };
> +               };
> +
> +               dc@54240000 {
> +                       compatible = "nvidia,tegra20-dc";
> +                       reg = <0x54240000 0x00040000>;
> +                       interrupts = <0 74 0x04>;
> +
> +                       rgb {
> +                               status = "disabled";
> +                       };
> +               };
> +
> +               /* outputs */
> +               hdmi {
> +                       compatible = "nvidia,tegra20-hdmi";
> +                       reg = <0x54280000 0x00040000>;
> +                       interrupts = <0 75 0x04>;
> +                       status = "disabled";
> +               };
> +
> +               tvo {
> +                       compatible = "nvidia,tegra20-tvo";
> +                       reg = <0x542c0000 0x00040000>;
> +                       interrupts = <0 76 0x04>;
> +                       status = "disabled";
> +               };
> +
> +               dsi {
> +                       compatible = "nvidia,tegra20-dsi";
> +                       reg = <0x54300000 0x00040000>;
> +                       status = "disabled";
> +               };
> +       };
> +
>  };
> diff --git a/doc/device-tree-bindings/video/displaymode.txt b/doc/device-tree-bindings/video/displaymode.txt
> new file mode 100644
> index 0000000..45ca42d
> --- /dev/null
> +++ b/doc/device-tree-bindings/video/displaymode.txt
> @@ -0,0 +1,42 @@
> +videomode bindings
> +==================
> +
> +(from http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html)
> +
> +Required properties:
> + - xres, yres: Display resolution
> + - left-margin, right-margin, hsync-len: Horizontal Display timing
> +   parameters in pixels
> + - upper-margin, lower-margin, vsync-len: Vertical display timing
> +   parameters in lines
> + - clock: display clock in Hz
> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree
> +representation corresponds to the one used by the Linux Framebuffer
> +framework described here in Documentation/fb/framebuffer.txt. This
> +representation has been chosen because it's the only format which does
> +not allow for inconsistent parameters. Unlike the Framebuffer framework
> +the devicetree has the clock in Hz instead of ps.
> +
> +Example:
> +
> +       display@0 {
> +               /* 1920x1080p24 */
> +               clock = <52000000>;
> +               xres = <1920>;
> +               yres = <1080>;
> +               left-margin = <25>;
> +               right-margin = <25>;
> +               hsync-len = <25>;
> +               lower-margin = <2>;
> +               upper-margin = <2>;
> +               vsync-len = <2>;
> +               hsync-active-high;
> +       };
> diff --git a/doc/device-tree-bindings/video/tegra20-dc.txt b/doc/device-tree-bindings/video/tegra20-dc.txt
> new file mode 100644
> index 0000000..dcfd652
> --- /dev/null
> +++ b/doc/device-tree-bindings/video/tegra20-dc.txt
> @@ -0,0 +1,89 @@
> +Display Controller
> +------------------
> +
> +(there isn't yet a generic binding in Linux, so this describes what is in
> +U-Boot, and may change based on Linux activity)
> +
> +The device node for a display device is as described in the document
> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
> +following modifications and additions :
> +
> +Required properties :
> + - compatible : Should be "nvidia,tegra20-dc"
> +
> +Required subnode 'rgb'. This has the properties listed in displaymode.txt
> +as well as:
> +
> +Required properties (rgb) :
> + - nvidia,panel : phandle of LCD panel information
> + - nvidia,bits-per-pixel: number of bits per pixel (depth)
> +
> +Optional properties (rgb):
> + - nvidia,frame-buffer: address of frame buffer (if omitted it will be
> +               calculated)
> +       - This may be useful to share an address between U-Boot and Linux and
> +               avoid boot-time corruption / flicker
> +
> +
> +The panel node describes the panel itself.
> +
> +Required properties (panel) :
> + - nvidia,pwm : pwm to use to set display contrast (see tegra20-pwm.txt)
> + - nvidia,panel-timings: 4 cells containing required timings in ms:
> +       * delay between panel_vdd-rise and data-rise
> +       * delay between data-rise and backlight_vdd-rise
> +       * delay between backlight_vdd and pwm-rise
> +       * delay between pwm-rise and backlight_en-rise
> +
> +Optional GPIO properies all have (phandle, GPIO number, flags):
> + - nvidia,backlight-enable-gpios: backlight enable GPIO
> + - nvidia,lvds-shutdown-gpios: LVDS power shutdown GPIO
> + - nvidia,backlight-vdd-gpios: backlight power GPIO
> + - nvidia,panel-vdd-gpios: panel power GPIO
> +
> +Example:
> +
> +host1x {
> +       compatible = "nvidia,tegra20-host1x", "simple-bus";
> +       reg = <0x50000000 0x00024000>;
> +       interrupts = <0 65 0x04   /* mpcore syncpt */
> +                       0 67 0x04>; /* mpcore general */
> +
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       ranges = <0x54000000 0x54000000 0x04000000>;
> +
> +       dc@54200000 {
> +               compatible = "nvidia,tegra20-dc";
> +               reg = <0x54200000 0x00040000>;
> +               interrupts = <0 73 0x04>;
> +
> +               rgb {
> +                       status = "okay";
> +                       /* Seaboard has 1366x768 */
> +                       clock = <70600000>;
> +                       xres = <1366>;
> +                       yres = <768>;
> +                       left-margin = <58>;
> +                       right-margin = <58>;
> +                       hsync-len = <58>;
> +                       lower-margin = <4>;
> +                       upper-margin = <4>;
> +                       vsync-len = <4>;
> +                       hsync-active-high;
> +                       nvidia,frame-buffer = <0x2f680000>;
> +                       nvidia,bits-per-pixel = <16>;
> +                       nvidia,panel = <&lcd_panel>;
> +               };
> +       };
> +};
> +
> +lcd_panel: panel {
> +       nvidia,pwm = <&pwm 2 0>;
> +       nvidia,backlight-enable-gpios = <&gpio 28 0>;   /* PD4 */
> +       nvidia,lvds-shutdown-gpios = <&gpio 10 0>;      /* PB2 */
> +       nvidia,backlight-vdd-gpios = <&gpio 176 0>;     /* PW0 */
> +       nvidia,panel-vdd-gpios = <&gpio 22 0>;          /* PC6 */
> +       nvidia,panel-timings = <4 203 17 15>;
> +};
> --
> 1.7.7.3
>

Re: [PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4953 bytes --]

On Tue, Jul 31, 2012 at 10:27:23AM +0100, Simon Glass wrote:
> +Thierry
> 
> Hi,
> 
> On Thu, Jul 12, 2012 at 4:25 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> > Add LCD definitions and also a proposed binding for LCD displays.
> >
> > The PWM is as per what will likely be committed to linux-next soon.
> >
> > The displaymode binding comes from a proposal here:
> >
> > http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html
> >
> > The panel binding is new, and fills a need to specify the panel
> > timings and other tegra-specific information. Should a binding appear
> > that allows the pwm to handle this automatically, we can revisit
> > this.
> >
> 
> Any comments on this binding please? The main addition from Thierry's
> one posted on LMKL is the LCD resolution selection.

There's no such thing as the panel bindings on Linux. I think nobody's
done that before, there's also no suitable software abstraction, but I
suppose that should be irrelevant for this discussion.

> > +Optional properties (rgb):
> > + - nvidia,frame-buffer: address of frame buffer (if omitted it will be
> > +               calculated)
> > +       - This may be useful to share an address between U-Boot and Linux and
> > +               avoid boot-time corruption / flicker
> > +
> > +
> > +The panel node describes the panel itself.
> > +
> > +Required properties (panel) :
> > + - nvidia,pwm : pwm to use to set display contrast (see tegra20-pwm.txt)
> > + - nvidia,panel-timings: 4 cells containing required timings in ms:
> > +       * delay between panel_vdd-rise and data-rise
> > +       * delay between data-rise and backlight_vdd-rise
> > +       * delay between backlight_vdd and pwm-rise
> > +       * delay between pwm-rise and backlight_en-rise
> > +
> > +Optional GPIO properies all have (phandle, GPIO number, flags):
> > + - nvidia,backlight-enable-gpios: backlight enable GPIO
> > + - nvidia,lvds-shutdown-gpios: LVDS power shutdown GPIO
> > + - nvidia,backlight-vdd-gpios: backlight power GPIO
> > + - nvidia,panel-vdd-gpios: panel power GPIO
> > +
> > +Example:
> > +
> > +host1x {
> > +       compatible = "nvidia,tegra20-host1x", "simple-bus";
> > +       reg = <0x50000000 0x00024000>;
> > +       interrupts = <0 65 0x04   /* mpcore syncpt */
> > +                       0 67 0x04>; /* mpcore general */
> > +
> > +       #address-cells = <1>;
> > +       #size-cells = <1>;
> > +
> > +       ranges = <0x54000000 0x54000000 0x04000000>;
> > +
> > +       dc@54200000 {
> > +               compatible = "nvidia,tegra20-dc";
> > +               reg = <0x54200000 0x00040000>;
> > +               interrupts = <0 73 0x04>;
> > +
> > +               rgb {
> > +                       status = "okay";
> > +                       /* Seaboard has 1366x768 */
> > +                       clock = <70600000>;
> > +                       xres = <1366>;
> > +                       yres = <768>;
> > +                       left-margin = <58>;
> > +                       right-margin = <58>;
> > +                       hsync-len = <58>;
> > +                       lower-margin = <4>;
> > +                       upper-margin = <4>;
> > +                       vsync-len = <4>;
> > +                       hsync-active-high;
> > +                       nvidia,frame-buffer = <0x2f680000>;
> > +                       nvidia,bits-per-pixel = <16>;
> > +                       nvidia,panel = <&lcd_panel>;
> > +               };

Perhaps it would be useful to add an extra node for the mode definition,
if only to keep the data separate from that of the rgb node. I know that
there currently are no other properties, but the rgb node was supposed
to define the output or connector. If ever the same needs to be done for
any of the TVO or DSI outputs, more properties may be needed.

Furthermore the code to parse this would be more generic because you
could pass it any DT node. Of course the nvidia,-prefixed properties
wouldn't be part of that subnode because they don't define the mode as
such.

Thinking about it some more, maybe the mode information should really be
part of the panel description below.

> > +       };
> > +};
> > +
> > +lcd_panel: panel {
> > +       nvidia,pwm = <&pwm 2 0>;
> > +       nvidia,backlight-enable-gpios = <&gpio 28 0>;   /* PD4 */
> > +       nvidia,lvds-shutdown-gpios = <&gpio 10 0>;      /* PB2 */
> > +       nvidia,backlight-vdd-gpios = <&gpio 176 0>;     /* PW0 */
> > +       nvidia,panel-vdd-gpios = <&gpio 22 0>;          /* PC6 */
> > +       nvidia,panel-timings = <4 203 17 15>;
> > +};

If we can reach some kind of agreement on the power-sequencing code that
Alexandre (Cc'ed) is working on, then this can be replaced with a more
generic description. This also has the usual problem of being a non-
addressable top-level node...

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Simon Glass]

Hi Thierry,

On Tue, Jul 31, 2012 at 2:51 AM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> On Tue, Jul 31, 2012 at 10:27:23AM +0100, Simon Glass wrote:
>> +Thierry
>>
>> Hi,
>>
>> On Thu, Jul 12, 2012 at 4:25 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> > Add LCD definitions and also a proposed binding for LCD displays.
>> >
>> > The PWM is as per what will likely be committed to linux-next soon.
>> >
>> > The displaymode binding comes from a proposal here:
>> >
>> > http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html
>> >
>> > The panel binding is new, and fills a need to specify the panel
>> > timings and other tegra-specific information. Should a binding appear
>> > that allows the pwm to handle this automatically, we can revisit
>> > this.
>> >
>>
>> Any comments on this binding please? The main addition from Thierry's
>> one posted on LMKL is the LCD resolution selection.
>
> There's no such thing as the panel bindings on Linux. I think nobody's
> done that before, there's also no suitable software abstraction, but I
> suppose that should be irrelevant for this discussion.
>
>> > +Optional properties (rgb):
>> > + - nvidia,frame-buffer: address of frame buffer (if omitted it will be
>> > +               calculated)
>> > +       - This may be useful to share an address between U-Boot and Linux and
>> > +               avoid boot-time corruption / flicker
>> > +
>> > +
>> > +The panel node describes the panel itself.
>> > +
>> > +Required properties (panel) :
>> > + - nvidia,pwm : pwm to use to set display contrast (see tegra20-pwm.txt)
>> > + - nvidia,panel-timings: 4 cells containing required timings in ms:
>> > +       * delay between panel_vdd-rise and data-rise
>> > +       * delay between data-rise and backlight_vdd-rise
>> > +       * delay between backlight_vdd and pwm-rise
>> > +       * delay between pwm-rise and backlight_en-rise
>> > +
>> > +Optional GPIO properies all have (phandle, GPIO number, flags):
>> > + - nvidia,backlight-enable-gpios: backlight enable GPIO
>> > + - nvidia,lvds-shutdown-gpios: LVDS power shutdown GPIO
>> > + - nvidia,backlight-vdd-gpios: backlight power GPIO
>> > + - nvidia,panel-vdd-gpios: panel power GPIO
>> > +
>> > +Example:
>> > +
>> > +host1x {
>> > +       compatible = "nvidia,tegra20-host1x", "simple-bus";
>> > +       reg = <0x50000000 0x00024000>;
>> > +       interrupts = <0 65 0x04   /* mpcore syncpt */
>> > +                       0 67 0x04>; /* mpcore general */
>> > +
>> > +       #address-cells = <1>;
>> > +       #size-cells = <1>;
>> > +
>> > +       ranges = <0x54000000 0x54000000 0x04000000>;
>> > +
>> > +       dc@54200000 {
>> > +               compatible = "nvidia,tegra20-dc";
>> > +               reg = <0x54200000 0x00040000>;
>> > +               interrupts = <0 73 0x04>;
>> > +
>> > +               rgb {
>> > +                       status = "okay";
>> > +                       /* Seaboard has 1366x768 */
>> > +                       clock = <70600000>;
>> > +                       xres = <1366>;
>> > +                       yres = <768>;
>> > +                       left-margin = <58>;
>> > +                       right-margin = <58>;
>> > +                       hsync-len = <58>;
>> > +                       lower-margin = <4>;
>> > +                       upper-margin = <4>;
>> > +                       vsync-len = <4>;
>> > +                       hsync-active-high;
>> > +                       nvidia,frame-buffer = <0x2f680000>;
>> > +                       nvidia,bits-per-pixel = <16>;
>> > +                       nvidia,panel = <&lcd_panel>;
>> > +               };
>
> Perhaps it would be useful to add an extra node for the mode definition,
> if only to keep the data separate from that of the rgb node. I know that
> there currently are no other properties, but the rgb node was supposed
> to define the output or connector. If ever the same needs to be done for
> any of the TVO or DSI outputs, more properties may be needed.
>
> Furthermore the code to parse this would be more generic because you
> could pass it any DT node. Of course the nvidia,-prefixed properties
> wouldn't be part of that subnode because they don't define the mode as
> such.
>
> Thinking about it some more, maybe the mode information should really be
> part of the panel description below.

OK I will move it there, thanks.

>
>> > +       };
>> > +};
>> > +
>> > +lcd_panel: panel {
>> > +       nvidia,pwm = <&pwm 2 0>;
>> > +       nvidia,backlight-enable-gpios = <&gpio 28 0>;   /* PD4 */
>> > +       nvidia,lvds-shutdown-gpios = <&gpio 10 0>;      /* PB2 */
>> > +       nvidia,backlight-vdd-gpios = <&gpio 176 0>;     /* PW0 */
>> > +       nvidia,panel-vdd-gpios = <&gpio 22 0>;          /* PC6 */
>> > +       nvidia,panel-timings = <4 203 17 15>;
>> > +};
>
> If we can reach some kind of agreement on the power-sequencing code that
> Alexandre (Cc'ed) is working on, then this can be replaced with a more
> generic description. This also has the usual problem of being a non-
> addressable top-level node...

Yes, I have been following that. However, it would need quite a bit of
code to be written. I would like to leave this for a future owner if
possible.

Regards,
Simon

>
> Thierry

Re: [PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Stephen Warren]

On 07/31/2012 03:27 AM, Simon Glass wrote:
> On Thu, Jul 12, 2012 at 4:25 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> Add LCD definitions and also a proposed binding for LCD displays.
>>
>> The PWM is as per what will likely be committed to linux-next soon.
>>
>> The displaymode binding comes from a proposal here:
>>
>> http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html
>>
>> The panel binding is new, and fills a need to specify the panel
>> timings and other tegra-specific information. Should a binding appear
>> that allows the pwm to handle this automatically, we can revisit
>> this.
> 
> Any comments on this binding please? The main addition from Thierry's
> one posted on LMKL is the LCD resolution selection.

I have some concerns about the way the mode is represented in the
binding; the timing parameters are quite different to how e.g. an EDID
DTD would represent them, which I think will lead to conversion mistakes
when writing the DT.

I'm trying to get along of Sascha's original email so I can join in on
that discussion.

>> diff --git a/doc/device-tree-bindings/video/tegra20-dc.txt b/doc/device-tree-bindings/video/tegra20-dc.txt

>> +Optional properties (rgb):
>> + - nvidia,frame-buffer: address of frame buffer (if omitted it will be
>> +               calculated)
>> +       - This may be useful to share an address between U-Boot and Linux and
>> +               avoid boot-time corruption / flicker

Why can't the display driver read this out of the display registers,
instead of requiring the same information to be passed using DT too?

Re: [PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Simon Glass]

Hi Stephen,

On Tue, Jul 31, 2012 at 9:12 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 07/31/2012 03:27 AM, Simon Glass wrote:
>> On Thu, Jul 12, 2012 at 4:25 PM, Simon Glass <sjg@chromium.org> wrote:
>>> Add LCD definitions and also a proposed binding for LCD displays.
>>>
>>> The PWM is as per what will likely be committed to linux-next soon.
>>>
>>> The displaymode binding comes from a proposal here:
>>>
>>> http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html
>>>
>>> The panel binding is new, and fills a need to specify the panel
>>> timings and other tegra-specific information. Should a binding appear
>>> that allows the pwm to handle this automatically, we can revisit
>>> this.
>>
>> Any comments on this binding please? The main addition from Thierry's
>> one posted on LMKL is the LCD resolution selection.
>
> I have some concerns about the way the mode is represented in the
> binding; the timing parameters are quite different to how e.g. an EDID
> DTD would represent them, which I think will lead to conversion mistakes
> when writing the DT.
>
> I'm trying to get along of Sascha's original email so I can join in on
> that discussion.

Did you get any resolution here?

>
>>> diff --git a/doc/device-tree-bindings/video/tegra20-dc.txt b/doc/device-tree-bindings/video/tegra20-dc.txt
>
>>> +Optional properties (rgb):
>>> + - nvidia,frame-buffer: address of frame buffer (if omitted it will be
>>> +               calculated)
>>> +       - This may be useful to share an address between U-Boot and Linux and
>>> +               avoid boot-time corruption / flicker
>
> Why can't the display driver read this out of the display registers,
> instead of requiring the same information to be passed using DT too?

The U-Boot display driver needs to set this value in the display
registers - at reset I don't think we can rely on any particular
value.

Do you mean the kernel display driver? Well it could, but it doesn't.
Really this is just a way of getting U-Boot and Linux to agree on the
address, by having U-Boot know the address that the kernel will happen
to use. It isn't very robust, but we have found it useful as a means
of avoiding problems in this area.

Regards,
Simon

Re: [PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Stephen Warren]

On 09/27/2012 07:58 AM, Simon Glass wrote:
> Hi Stephen,
> 
> On Tue, Jul 31, 2012 at 9:12 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 07/31/2012 03:27 AM, Simon Glass wrote:
>>> On Thu, Jul 12, 2012 at 4:25 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>>> Add LCD definitions and also a proposed binding for LCD displays.
>>>>
>>>> The PWM is as per what will likely be committed to linux-next soon.
>>>>
>>>> The displaymode binding comes from a proposal here:
>>>>
>>>> http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html
>>>>
>>>> The panel binding is new, and fills a need to specify the panel
>>>> timings and other tegra-specific information. Should a binding appear
>>>> that allows the pwm to handle this automatically, we can revisit
>>>> this.
>>>
>>> Any comments on this binding please? The main addition from Thierry's
>>> one posted on LMKL is the LCD resolution selection.
>>
>> I have some concerns about the way the mode is represented in the
>> binding; the timing parameters are quite different to how e.g. an EDID
>> DTD would represent them, which I think will lead to conversion mistakes
>> when writing the DT.
>>
>> I'm trying to get along of Sascha's original email so I can join in on
>> that discussion.
> 
> Did you get any resolution here?

I think an updated version of those patches was published.

>>>> diff --git a/doc/device-tree-bindings/video/tegra20-dc.txt b/doc/device-tree-bindings/video/tegra20-dc.txt
>>
>>>> +Optional properties (rgb):
>>>> + - nvidia,frame-buffer: address of frame buffer (if omitted it will be
>>>> +               calculated)
>>>> +       - This may be useful to share an address between U-Boot and Linux and
>>>> +               avoid boot-time corruption / flicker
>>
>> Why can't the display driver read this out of the display registers,
>> instead of requiring the same information to be passed using DT too?
> 
> The U-Boot display driver needs to set this value in the display
> registers - at reset I don't think we can rely on any particular
> value.
> 
> Do you mean the kernel display driver?

Yes.

> Well it could, but it doesn't.

Well, there is no display driver in the upstream kernel at the moment,
so it's not possible to make assertions about what it does or not.

Any downstream kernels aren't relevant, since their DT bindings and/or
behaviour have not been through public review.

> Really this is just a way of getting U-Boot and Linux to agree on the
> address, by having U-Boot know the address that the kernel will happen
> to use. It isn't very robust, but we have found it useful as a means
> of avoiding problems in this area.

Right, but again, why can't the display driver simply read the address
out of the register; why is there a need to duplicate the data?

I guess one possibility is that the register only gives the base address
and not the size/mode of the allocated surface, but I don't recall if
this proposed binding did that either?

Re: [PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Simon Glass]

Hi Stephen,

On Thu, Sep 27, 2012 at 8:49 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/27/2012 07:58 AM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Tue, Jul 31, 2012 at 9:12 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 07/31/2012 03:27 AM, Simon Glass wrote:
>>>> On Thu, Jul 12, 2012 at 4:25 PM, Simon Glass <sjg@chromium.org> wrote:
>>>>> Add LCD definitions and also a proposed binding for LCD displays.
>>>>>
>>>>> The PWM is as per what will likely be committed to linux-next soon.
>>>>>
>>>>> The displaymode binding comes from a proposal here:
>>>>>
>>>>> http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html
>>>>>
>>>>> The panel binding is new, and fills a need to specify the panel
>>>>> timings and other tegra-specific information. Should a binding appear
>>>>> that allows the pwm to handle this automatically, we can revisit
>>>>> this.
>>>>
>>>> Any comments on this binding please? The main addition from Thierry's
>>>> one posted on LMKL is the LCD resolution selection.
>>>
>>> I have some concerns about the way the mode is represented in the
>>> binding; the timing parameters are quite different to how e.g. an EDID
>>> DTD would represent them, which I think will lead to conversion mistakes
>>> when writing the DT.
>>>
>>> I'm trying to get along of Sascha's original email so I can join in on
>>> that discussion.
>>
>> Did you get any resolution here?
>
> I think an updated version of those patches was published.
>
>>>>> diff --git a/doc/device-tree-bindings/video/tegra20-dc.txt b/doc/device-tree-bindings/video/tegra20-dc.txt
>>>
>>>>> +Optional properties (rgb):
>>>>> + - nvidia,frame-buffer: address of frame buffer (if omitted it will be
>>>>> +               calculated)
>>>>> +       - This may be useful to share an address between U-Boot and Linux and
>>>>> +               avoid boot-time corruption / flicker
>>>
>>> Why can't the display driver read this out of the display registers,
>>> instead of requiring the same information to be passed using DT too?
>>
>> The U-Boot display driver needs to set this value in the display
>> registers - at reset I don't think we can rely on any particular
>> value.
>>
>> Do you mean the kernel display driver?
>
> Yes.
>
>> Well it could, but it doesn't.
>
> Well, there is no display driver in the upstream kernel at the moment,
> so it's not possible to make assertions about what it does or not.
>
> Any downstream kernels aren't relevant, since their DT bindings and/or
> behaviour have not been through public review.

OK, but typically the kernel selects its own address for the display.
It might be quite hard for the kernel to use the one defined by
U-Boot.

>
>> Really this is just a way of getting U-Boot and Linux to agree on the
>> address, by having U-Boot know the address that the kernel will happen
>> to use. It isn't very robust, but we have found it useful as a means
>> of avoiding problems in this area.
>
> Right, but again, why can't the display driver simply read the address
> out of the register; why is there a need to duplicate the data?
>
> I guess one possibility is that the register only gives the base address
> and not the size/mode of the allocated surface, but I don't recall if
> this proposed binding did that either?

So here is my explanation:

1. U-Boot would normally put the display right near the top of memory.
Instead, it figures out where the kernel will put it (somehow this
seems to often be at a fixed address) and uses that address.

2. That means that U-Boot will now have the display exactly where the
kernel wants it.

3. When U-Boot boots the kernel, it leaves the display running,
knowing that the kernel will not overwrite the frame buffer with its
own code / data.

4. The effect is that there is a seamless display transition from
U-Boot to the kernel.

Note this is only a hint - it can be omitted i.w.c. it isn't used.

So what do you think? Should we remove this entirely, or is it a useful hack?

Regards,
Simon

Re: [PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Stephen Warren]

On 09/27/2012 02:27 PM, Simon Glass wrote:
> On Thu, Sep 27, 2012 at 8:49 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 09/27/2012 07:58 AM, Simon Glass wrote:
...
>>> Really this is just a way of getting U-Boot and Linux to agree on the
>>> address, by having U-Boot know the address that the kernel will happen
>>> to use. It isn't very robust, but we have found it useful as a means
>>> of avoiding problems in this area.
>>
>> Right, but again, why can't the display driver simply read the address
>> out of the register; why is there a need to duplicate the data?
>>
>> I guess one possibility is that the register only gives the base address
>> and not the size/mode of the allocated surface, but I don't recall if
>> this proposed binding did that either?
> 
> So here is my explanation:
> 
> 1. U-Boot would normally put the display right near the top of memory.
> Instead, it figures out where the kernel will put it (somehow this
> seems to often be at a fixed address) and uses that address.
> 
> 2. That means that U-Boot will now have the display exactly where the
> kernel wants it.

Oh, the DT property is telling U-Boot where to put the surface so that
when the kernel determines where to put it, it'll already be there?

That seems completely backwards. It's also extremely fragile; what if
the kernel suddenly starts allocating at some other random address,
which stops matching what U-Boot expects?

Far better would be for U-Boot to put the surface wherever it wants,
then manipulate the DT that's passed to the kernel to:

a) Add a /memreserve/ so that memory doesn't get re-used for anything
else during boot.

b) Add a property to the display node indicating which memreserve
represents the frame-buffer location.

When the kernel boots, it can:

* Copy the content from the U-Boot-allocated buffer to whatever display
buffer the kernel allocated before writing the new address to the HW.

* Undo the memory reservation triggered by the /memreserve/ to allow the
memory to be re-used.

> So what do you think? Should we remove this entirely, or is it a useful hack?

Yes, I think in its current form, it isn't useful.

Re: [PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Simon Glass]

Hi Stephen,

On Thu, Sep 27, 2012 at 4:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/27/2012 02:27 PM, Simon Glass wrote:
>> On Thu, Sep 27, 2012 at 8:49 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 09/27/2012 07:58 AM, Simon Glass wrote:
> ...
>>>> Really this is just a way of getting U-Boot and Linux to agree on the
>>>> address, by having U-Boot know the address that the kernel will happen
>>>> to use. It isn't very robust, but we have found it useful as a means
>>>> of avoiding problems in this area.
>>>
>>> Right, but again, why can't the display driver simply read the address
>>> out of the register; why is there a need to duplicate the data?
>>>
>>> I guess one possibility is that the register only gives the base address
>>> and not the size/mode of the allocated surface, but I don't recall if
>>> this proposed binding did that either?
>>
>> So here is my explanation:
>>
>> 1. U-Boot would normally put the display right near the top of memory.
>> Instead, it figures out where the kernel will put it (somehow this
>> seems to often be at a fixed address) and uses that address.
>>
>> 2. That means that U-Boot will now have the display exactly where the
>> kernel wants it.
>
> Oh, the DT property is telling U-Boot where to put the surface so that
> when the kernel determines where to put it, it'll already be there?
>
> That seems completely backwards. It's also extremely fragile; what if
> the kernel suddenly starts allocating at some other random address,
> which stops matching what U-Boot expects?

The screen will flicker, or may fill up with junk as the kernel boots.
We did have that happen once when the driver changed.

>
> Far better would be for U-Boot to put the surface wherever it wants,
> then manipulate the DT that's passed to the kernel to:
>
> a) Add a /memreserve/ so that memory doesn't get re-used for anything
> else during boot.
>
> b) Add a property to the display node indicating which memreserve
> represents the frame-buffer location.

Yes, I believe the Tegra video driver we used (and I take your point
that it doesn't exist upstream so there is no point in discussing it
:-) always had to use the same frame buffer address. The memory was
statically allocated. Certainly this could be encoded in the fdt.

>
> When the kernel boots, it can:
>
> * Copy the content from the U-Boot-allocated buffer to whatever display
> buffer the kernel allocated before writing the new address to the HW.
>
> * Undo the memory reservation triggered by the /memreserve/ to allow the
> memory to be re-used.
>
>> So what do you think? Should we remove this entirely, or is it a useful hack?
>
> Yes, I think in its current form, it isn't useful.

Thanks for the thorough response. Let's remove it, particularly as
there isn't even a kernel driver upstream yet.

I will do that before sending out the LCD v4 series. Thanks for your
timely comments.

Regards,
Simon

Re: [PATCH v3 06/18] tegra: fdt: Add LCD definitions for Tegra

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3848 bytes --]

On Thu, Sep 27, 2012 at 04:37:41PM -0700, Simon Glass wrote:
> Hi Stephen,
> 
> On Thu, Sep 27, 2012 at 4:21 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> > On 09/27/2012 02:27 PM, Simon Glass wrote:
> >> On Thu, Sep 27, 2012 at 8:49 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> >>> On 09/27/2012 07:58 AM, Simon Glass wrote:
> > ...
> >>>> Really this is just a way of getting U-Boot and Linux to agree on the
> >>>> address, by having U-Boot know the address that the kernel will happen
> >>>> to use. It isn't very robust, but we have found it useful as a means
> >>>> of avoiding problems in this area.
> >>>
> >>> Right, but again, why can't the display driver simply read the address
> >>> out of the register; why is there a need to duplicate the data?
> >>>
> >>> I guess one possibility is that the register only gives the base address
> >>> and not the size/mode of the allocated surface, but I don't recall if
> >>> this proposed binding did that either?
> >>
> >> So here is my explanation:
> >>
> >> 1. U-Boot would normally put the display right near the top of memory.
> >> Instead, it figures out where the kernel will put it (somehow this
> >> seems to often be at a fixed address) and uses that address.
> >>
> >> 2. That means that U-Boot will now have the display exactly where the
> >> kernel wants it.
> >
> > Oh, the DT property is telling U-Boot where to put the surface so that
> > when the kernel determines where to put it, it'll already be there?
> >
> > That seems completely backwards. It's also extremely fragile; what if
> > the kernel suddenly starts allocating at some other random address,
> > which stops matching what U-Boot expects?
> 
> The screen will flicker, or may fill up with junk as the kernel boots.
> We did have that happen once when the driver changed.
> 
> >
> > Far better would be for U-Boot to put the surface wherever it wants,
> > then manipulate the DT that's passed to the kernel to:
> >
> > a) Add a /memreserve/ so that memory doesn't get re-used for anything
> > else during boot.
> >
> > b) Add a property to the display node indicating which memreserve
> > represents the frame-buffer location.
> 
> Yes, I believe the Tegra video driver we used (and I take your point
> that it doesn't exist upstream so there is no point in discussing it
> :-) always had to use the same frame buffer address. The memory was
> statically allocated. Certainly this could be encoded in the fdt.
> 
> >
> > When the kernel boots, it can:
> >
> > * Copy the content from the U-Boot-allocated buffer to whatever display
> > buffer the kernel allocated before writing the new address to the HW.
> >
> > * Undo the memory reservation triggered by the /memreserve/ to allow the
> > memory to be re-used.
> >
> >> So what do you think? Should we remove this entirely, or is it a useful hack?
> >
> > Yes, I think in its current form, it isn't useful.
> 
> Thanks for the thorough response. Let's remove it, particularly as
> there isn't even a kernel driver upstream yet.

We've been discussing some of these issues for quite some time and came
to the conclusion that the Tegra DRM driver should not be using static
allocations like the previous drivers did. One of the reasons is that
the amount of carveout memory needs to be fixed at boot time, which
isn't going to suit a wide range of devices well. Another, perhaps more
important reason is that starting with Tegra30 that's no longer
necessary because of the SMMU.

So in the end it wouldn't be possible to reliably determine the memory
area where the kernel will put the framebuffer. If we want a flicker-
free boot then we need to go with something more elaborate like what
Stephen suggested.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH v3 16/18] tegra: fdt: Add LCD definitions for Seaboard

[Simon Glass]

The Seaboard has a 1366x768 16bpp LCD. The backlight is controlled
by one of the PWMs.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v2:
- Update seaboard LCD definitions for new fdt binding

Changes in v3:
- Use new upstream proposed LCD definitions

 board/nvidia/dts/tegra2-seaboard.dts |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/board/nvidia/dts/tegra2-seaboard.dts b/board/nvidia/dts/tegra2-seaboard.dts
index 3352539..bfb835f 100644
--- a/board/nvidia/dts/tegra2-seaboard.dts
+++ b/board/nvidia/dts/tegra2-seaboard.dts
@@ -153,4 +153,36 @@
 			0x1f04008a>;
 		linux,fn-keymap = <0x05040002>;
 	};
+
+	host1x {
+		dc@54200000 {
+			rgb {
+				status = "okay";
+				/* Seaboard has 1366x768 */
+				clock = <70600000>;
+				xres = <1366>;
+				yres = <768>;
+				left-margin = <58>;
+				right-margin = <58>;
+				hsync-len = <58>;
+				lower-margin = <4>;
+				upper-margin = <4>;
+				vsync-len = <4>;
+				hsync-active-high;
+				nvidia,frame-buffer = <0x2f680000>;
+				nvidia,bits-per-pixel = <16>;
+				nvidia,panel = <&lcd_panel>;
+			};
+		};
+	};
+
+	lcd_panel: panel {
+		nvidia,pwm = <&pwm 2 0>;
+		nvidia,backlight-enable-gpios = <&gpio 28 0>;	/* PD4 */
+		nvidia,lvds-shutdown-gpios = <&gpio 10 0>;	/* PB2 */
+		nvidia,backlight-vdd-gpios = <&gpio 176 0>;	/* PW0 */
+		nvidia,panel-vdd-gpios = <&gpio 22 0>;		/* PC6 */
+		nvidia,panel-timings = <4 203 17 15>;
+	};
+
 };
-- 
1.7.7.3