[PATCH v2 1/2] video: ARM CLCD: Add DT support

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

This patch adds basic DT bindings for the PL11x CLCD cells
and make their fbdev driver use them.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---

Changes since v1:
- minor code cleanups as suggested by Sylwester Nawrocki

 .../devicetree/bindings/video/arm,pl11x.txt        |  87 +++++++++
 drivers/video/Kconfig                              |   1 +
 drivers/video/amba-clcd.c                          | 211 +++++++++++++++++++++
 3 files changed, 299 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/arm,pl11x.txt

diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
new file mode 100644
index 0000000..7eb77aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
@@ -0,0 +1,87 @@
+* ARM PrimeCell Color LCD (CLCD) Controller PL110/PL111
+
+See also Documentation/devicetree/bindings/arm/primecell.txt
+
+Required properties:
+
+- compatible: must be one of:
+			"arm,pl110", "arm,primecell"
+			"arm,pl111", "arm,primecell"
+- reg: base address and size of the control registers block
+- interrupts: either a single interrupt specifier representing the
+		combined interrupt output (CLCDINTR) or an array of
+		four interrupt specifiers for CLCDMBEINTR,
+		CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR; in the
+		latter case interrupt names must be specified
+		(see below)
+- interrupt-names: when four interrupts are specified, their names:
+			"mbe", "vcomp", "lnbu", "fuf"
+			must be specified in order respective to the
+			interrupt specifiers
+- clocks: contains phandle and clock specifier pairs for the entries
+		in the clock-names property. See
+		Documentation/devicetree/binding/clock/clock-bindings.txt
+- clocks names: should contain "clcdclk" and "apb_pclk"
+
+Optional properties:
+
+- video-ram: phandle to a node describing specialized video memory
+		(that is *not* described in the top level "memory" node)
+		that must be used as a framebuffer, eg. due to restrictions
+		of the system interconnect; the node must contain a
+		standard reg property describing the address and the size
+		of the memory area
+- max-framebuffer-size: maximum size in bytes of the framebuffer the
+			system can handle, eg. in terms of available
+			memory bandwidth
+
+In the simplest case of a display panel being connected directly to the
+CLCD, it can be described in the node:
+
+- panel-dimensions: (optional) array of two numbers (width and height)
+			describing physical dimension in mm of the panel
+- panel-type: (required) must be "tft" or "stn", defines panel type
+- panel-tft-interface: (for "tft" panel type) array of 3 numbers defining
+			widths in bits of the R, G and B components
+- panel-tft-rb-swapped: (for "tft" panel type) if present means that
+			the R & B components are swapped on the board
+- panel-stn-color: (for "stn" panel type) if present means that
+			the panel is a colour STN display, if missing
+			is a monochrome display
+- panel-stn-dual: (for "stn" panel type) if present means that there
+			are two STN displays connected
+- panel-stn-4bit: (for monochrome "stn" panel) if present means
+			that the monochrome display is connected
+			via 4 bit-wide interface
+- display-timings: standard display timings sub-node, see
+			Documentation/devicetree/bindings/video/display-timing.txt
+
+Example:
+
+			clcd@1f0000 {
+				compatible = "arm,pl111", "arm,primecell";
+				reg = <0x1f0000 0x1000>;
+				interrupts = <14>;
+				clocks = <&v2m_oscclk1>, <&smbclk>;
+				clock-names = "clcdclk", "apb_pclk";
+
+				video-ram = <&v2m_vram>;
+				max-framebuffer-size = <614400>; /* 640x480 16bpp */
+
+				panel-type = "tft";
+				panel-tft-interface = <8 8 8>;
+				display-timings {
+					native-mode = <&v2m_clcd_timing0>;
+					v2m_clcd_timing0: vga {
+						clock-frequency = <25175000>;
+						hactive = <640>;
+						hback-porch = <40>;
+						hfront-porch = <24>;
+						hsync-len = <96>;
+						vactive = <480>;
+						vback-porch = <32>;
+						vfront-porch = <11>;
+						vsync-len = <2>;
+					};
+				};
+			};
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 4cf1e1d..375bf63 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -316,6 +316,7 @@ config FB_ARMCLCD
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
+	select VIDEOMODE_HELPERS if OF
 	help
 	  This framebuffer device driver is for the ARM PrimeCell PL110
 	  Colour LCD controller.  ARM PrimeCells provide the building
diff --git a/drivers/video/amba-clcd.c b/drivers/video/amba-clcd.c
index 0a2cce7..7f36964 100644
--- a/drivers/video/amba-clcd.c
+++ b/drivers/video/amba-clcd.c
@@ -25,6 +25,11 @@
 #include <linux/amba/clcd.h>
 #include <linux/clk.h>
 #include <linux/hardirq.h>
+#include <linux/dma-mapping.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <video/of_display_timing.h>
+#include <video/of_videomode.h>
 
 #include <asm/sizes.h>
 
@@ -542,6 +547,209 @@ static int clcdfb_register(struct clcd_fb *fb)
 	return ret;
 }
 
+#ifdef CONFIG_OF
+static int clcdfb_of_get_tft_panel(struct device_node *node,
+		struct clcd_panel *panel)
+{
+	int err;
+	u32 rgb[3];
+
+	err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3);
+	if (err)
+		return err;
+
+	/* Bypass pixel clock divider, data output on the falling edge */
+	panel->tim2 = TIM2_BCD | TIM2_IPC;
+
+	/* TFT display, vert. comp. interrupt at the start of the back porch */
+	panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
+
+	if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4)
+		panel->caps |= CLCD_CAP_444;
+	if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5)
+		panel->caps |= CLCD_CAP_5551;
+	if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5)
+		panel->caps |= CLCD_CAP_565;
+	if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8)
+		panel->caps |= CLCD_CAP_888;
+
+	if (of_get_property(node, "panel-tft-rb-swapped", NULL))
+		panel->caps &= ~CLCD_CAP_RGB;
+	else
+		panel->caps &= ~CLCD_CAP_BGR;
+
+	return 0;
+}
+
+static int clcdfb_snprintf_mode(char *buf, int size, struct fb_videomode *mode)
+{
+	return snprintf(buf, size, "%ux%u@%u", mode->xres, mode->yres,
+			mode->refresh);
+}
+
+static int clcdfb_of_init_display(struct clcd_fb *fb)
+{
+	struct device_node *node = fb->dev->dev.of_node;
+	int err, len;
+	char *mode_name;
+	u32 max_size;
+	u32 dimensions[2];
+	const char *panel_type;
+
+	fb->panel = devm_kzalloc(&fb->dev->dev, sizeof(*fb->panel), GFP_KERNEL);
+	if (!fb->panel)
+		return -ENOMEM;
+
+	err = of_get_fb_videomode(node, &fb->panel->mode, OF_USE_NATIVE_MODE);
+	if (err)
+		return err;
+
+	len = clcdfb_snprintf_mode(NULL, 0, &fb->panel->mode);
+	mode_name = devm_kzalloc(&fb->dev->dev, len + 1, GFP_KERNEL);
+	clcdfb_snprintf_mode(mode_name, len + 1, &fb->panel->mode);
+	fb->panel->mode.name = mode_name;
+
+	err = of_property_read_u32(node, "max-framebuffer-size", &max_size);
+	if (!err)
+		fb->panel->bpp = max_size / (fb->panel->mode.xres *
+				fb->panel->mode.yres) * 8;
+	else
+		fb->panel->bpp = 32;
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+	fb->panel->cntl |= CNTL_BEBO;
+#endif
+
+	if (of_property_read_u32_array(node, "panel-dimensions",
+			dimensions, 2) == 0) {
+		fb->panel->width = dimensions[0];
+		fb->panel->height = dimensions[1];
+	} else {
+		fb->panel->width = -1;
+		fb->panel->height = -1;
+	}
+
+	panel_type = of_get_property(node, "panel-type", &len);
+	if (strncmp(panel_type, "tft", len) == 0)
+		return clcdfb_of_get_tft_panel(node, fb->panel);
+	else
+		return -EINVAL;
+}
+
+static int clcdfb_of_vram_setup(struct clcd_fb *fb)
+{
+	struct device_node *node = of_parse_phandle(fb->dev->dev.of_node,
+			"video-ram", 0);
+	u64 size;
+	int err;
+
+	if (!node)
+		return -ENODEV;
+
+	err = clcdfb_of_init_display(fb);
+	if (err)
+		return err;
+
+	fb->fb.screen_base = of_iomap(node, 0);
+	if (!fb->fb.screen_base)
+		return -ENOMEM;
+
+	fb->fb.fix.smem_start = of_translate_address(node,
+			of_get_address(node, 0, &size, NULL));
+	fb->fb.fix.smem_len = size;
+
+	return 0;
+}
+
+static int clcdfb_of_vram_mmap(struct clcd_fb *fb, struct vm_area_struct *vma)
+{
+	unsigned long off, user_size, kernel_size;
+
+	off = vma->vm_pgoff << PAGE_SHIFT;
+	user_size = vma->vm_end - vma->vm_start;
+	kernel_size = fb->fb.fix.smem_len;
+
+	if (off >= kernel_size || user_size > (kernel_size - off))
+		return -ENXIO;
+
+	return remap_pfn_range(vma, vma->vm_start,
+			__phys_to_pfn(fb->fb.fix.smem_start) + vma->vm_pgoff,
+			user_size,
+			pgprot_writecombine(vma->vm_page_prot));
+}
+
+static void clcdfb_of_vram_remove(struct clcd_fb *fb)
+{
+	iounmap(fb->fb.screen_base);
+}
+
+static int clcdfb_of_dma_setup(struct clcd_fb *fb)
+{
+	unsigned long framesize;
+	dma_addr_t dma;
+	int err;
+
+	err = clcdfb_of_init_display(fb);
+	if (err)
+		return err;
+
+	framesize = fb->panel->mode.xres * fb->panel->mode.yres *
+			fb->panel->bpp / 8;
+	fb->fb.screen_base = dma_alloc_writecombine(&fb->dev->dev, framesize,
+			&dma, GFP_KERNEL);
+	if (!fb->fb.screen_base)
+		return -ENOMEM;
+
+	fb->fb.fix.smem_start = dma;
+	fb->fb.fix.smem_len = framesize;
+
+	return 0;
+}
+
+static int clcdfb_of_dma_mmap(struct clcd_fb *fb, struct vm_area_struct *vma)
+{
+	return dma_mmap_writecombine(&fb->dev->dev, vma, fb->fb.screen_base,
+			fb->fb.fix.smem_start, fb->fb.fix.smem_len);
+}
+
+static void clcdfb_of_dma_remove(struct clcd_fb *fb)
+{
+	dma_free_writecombine(&fb->dev->dev, fb->fb.fix.smem_len,
+			fb->fb.screen_base, fb->fb.fix.smem_start);
+}
+
+static struct clcd_board *clcdfb_of_get_board(struct amba_device *dev)
+{
+	struct clcd_board *board = devm_kzalloc(&dev->dev, sizeof(*board),
+			GFP_KERNEL);
+	struct device_node *node = dev->dev.of_node;
+
+	if (!board)
+		return NULL;
+
+	board->name = of_node_full_name(node);
+	board->caps = CLCD_CAP_ALL;
+	board->check = clcdfb_check;
+	board->decode = clcdfb_decode;
+	if (of_find_property(node, "video-ram", NULL)) {
+		board->setup = clcdfb_of_vram_setup;
+		board->mmap = clcdfb_of_vram_mmap;
+		board->remove = clcdfb_of_vram_remove;
+	} else {
+		board->setup = clcdfb_of_dma_setup;
+		board->mmap = clcdfb_of_dma_mmap;
+		board->remove = clcdfb_of_dma_remove;
+	}
+
+	return board;
+}
+#else
+static struct clcd_board *clcdfb_of_get_board(struct amba_dev *dev)
+{
+	return NULL;
+}
+#endif
+
 static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id)
 {
 	struct clcd_board *board = dev->dev.platform_data;
@@ -549,6 +757,9 @@ static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id)
 	int ret;
 
 	if (!board)
+		board = clcdfb_of_get_board(dev);
+
+	if (!board)
 		return -EINVAL;
 
 	ret = amba_request_regions(dev, NULL);
-- 
1.8.1.2

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

This patch adds basic DT bindings for the PL11x CLCD cells
and make their fbdev driver use them.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---

Changes since v1:
- minor code cleanups as suggested by Sylwester Nawrocki

 .../devicetree/bindings/video/arm,pl11x.txt        |  87 +++++++++
 drivers/video/Kconfig                              |   1 +
 drivers/video/amba-clcd.c                          | 211 +++++++++++++++++++++
 3 files changed, 299 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/arm,pl11x.txt

diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
new file mode 100644
index 0000000..7eb77aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
@@ -0,0 +1,87 @@
+* ARM PrimeCell Color LCD (CLCD) Controller PL110/PL111
+
+See also Documentation/devicetree/bindings/arm/primecell.txt
+
+Required properties:
+
+- compatible: must be one of:
+			"arm,pl110", "arm,primecell"
+			"arm,pl111", "arm,primecell"
+- reg: base address and size of the control registers block
+- interrupts: either a single interrupt specifier representing the
+		combined interrupt output (CLCDINTR) or an array of
+		four interrupt specifiers for CLCDMBEINTR,
+		CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR; in the
+		latter case interrupt names must be specified
+		(see below)
+- interrupt-names: when four interrupts are specified, their names:
+			"mbe", "vcomp", "lnbu", "fuf"
+			must be specified in order respective to the
+			interrupt specifiers
+- clocks: contains phandle and clock specifier pairs for the entries
+		in the clock-names property. See
+		Documentation/devicetree/binding/clock/clock-bindings.txt
+- clocks names: should contain "clcdclk" and "apb_pclk"
+
+Optional properties:
+
+- video-ram: phandle to a node describing specialized video memory
+		(that is *not* described in the top level "memory" node)
+		that must be used as a framebuffer, eg. due to restrictions
+		of the system interconnect; the node must contain a
+		standard reg property describing the address and the size
+		of the memory area
+- max-framebuffer-size: maximum size in bytes of the framebuffer the
+			system can handle, eg. in terms of available
+			memory bandwidth
+
+In the simplest case of a display panel being connected directly to the
+CLCD, it can be described in the node:
+
+- panel-dimensions: (optional) array of two numbers (width and height)
+			describing physical dimension in mm of the panel
+- panel-type: (required) must be "tft" or "stn", defines panel type
+- panel-tft-interface: (for "tft" panel type) array of 3 numbers defining
+			widths in bits of the R, G and B components
+- panel-tft-rb-swapped: (for "tft" panel type) if present means that
+			the R & B components are swapped on the board
+- panel-stn-color: (for "stn" panel type) if present means that
+			the panel is a colour STN display, if missing
+			is a monochrome display
+- panel-stn-dual: (for "stn" panel type) if present means that there
+			are two STN displays connected
+- panel-stn-4bit: (for monochrome "stn" panel) if present means
+			that the monochrome display is connected
+			via 4 bit-wide interface
+- display-timings: standard display timings sub-node, see
+			Documentation/devicetree/bindings/video/display-timing.txt
+
+Example:
+
+			clcd@1f0000 {
+				compatible = "arm,pl111", "arm,primecell";
+				reg = <0x1f0000 0x1000>;
+				interrupts = <14>;
+				clocks = <&v2m_oscclk1>, <&smbclk>;
+				clock-names = "clcdclk", "apb_pclk";
+
+				video-ram = <&v2m_vram>;
+				max-framebuffer-size = <614400>; /* 640x480 16bpp */
+
+				panel-type = "tft";
+				panel-tft-interface = <8 8 8>;
+				display-timings {
+					native-mode = <&v2m_clcd_timing0>;
+					v2m_clcd_timing0: vga {
+						clock-frequency = <25175000>;
+						hactive = <640>;
+						hback-porch = <40>;
+						hfront-porch = <24>;
+						hsync-len = <96>;
+						vactive = <480>;
+						vback-porch = <32>;
+						vfront-porch = <11>;
+						vsync-len = <2>;
+					};
+				};
+			};
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 4cf1e1d..375bf63 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -316,6 +316,7 @@ config FB_ARMCLCD
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
+	select VIDEOMODE_HELPERS if OF
 	help
 	  This framebuffer device driver is for the ARM PrimeCell PL110
 	  Colour LCD controller.  ARM PrimeCells provide the building
diff --git a/drivers/video/amba-clcd.c b/drivers/video/amba-clcd.c
index 0a2cce7..7f36964 100644
--- a/drivers/video/amba-clcd.c
+++ b/drivers/video/amba-clcd.c
@@ -25,6 +25,11 @@
 #include <linux/amba/clcd.h>
 #include <linux/clk.h>
 #include <linux/hardirq.h>
+#include <linux/dma-mapping.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <video/of_display_timing.h>
+#include <video/of_videomode.h>
 
 #include <asm/sizes.h>
 
@@ -542,6 +547,209 @@ static int clcdfb_register(struct clcd_fb *fb)
 	return ret;
 }
 
+#ifdef CONFIG_OF
+static int clcdfb_of_get_tft_panel(struct device_node *node,
+		struct clcd_panel *panel)
+{
+	int err;
+	u32 rgb[3];
+
+	err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3);
+	if (err)
+		return err;
+
+	/* Bypass pixel clock divider, data output on the falling edge */
+	panel->tim2 = TIM2_BCD | TIM2_IPC;
+
+	/* TFT display, vert. comp. interrupt at the start of the back porch */
+	panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
+
+	if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4)
+		panel->caps |= CLCD_CAP_444;
+	if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5)
+		panel->caps |= CLCD_CAP_5551;
+	if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5)
+		panel->caps |= CLCD_CAP_565;
+	if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8)
+		panel->caps |= CLCD_CAP_888;
+
+	if (of_get_property(node, "panel-tft-rb-swapped", NULL))
+		panel->caps &= ~CLCD_CAP_RGB;
+	else
+		panel->caps &= ~CLCD_CAP_BGR;
+
+	return 0;
+}
+
+static int clcdfb_snprintf_mode(char *buf, int size, struct fb_videomode *mode)
+{
+	return snprintf(buf, size, "%ux%u@%u", mode->xres, mode->yres,
+			mode->refresh);
+}
+
+static int clcdfb_of_init_display(struct clcd_fb *fb)
+{
+	struct device_node *node = fb->dev->dev.of_node;
+	int err, len;
+	char *mode_name;
+	u32 max_size;
+	u32 dimensions[2];
+	const char *panel_type;
+
+	fb->panel = devm_kzalloc(&fb->dev->dev, sizeof(*fb->panel), GFP_KERNEL);
+	if (!fb->panel)
+		return -ENOMEM;
+
+	err = of_get_fb_videomode(node, &fb->panel->mode, OF_USE_NATIVE_MODE);
+	if (err)
+		return err;
+
+	len = clcdfb_snprintf_mode(NULL, 0, &fb->panel->mode);
+	mode_name = devm_kzalloc(&fb->dev->dev, len + 1, GFP_KERNEL);
+	clcdfb_snprintf_mode(mode_name, len + 1, &fb->panel->mode);
+	fb->panel->mode.name = mode_name;
+
+	err = of_property_read_u32(node, "max-framebuffer-size", &max_size);
+	if (!err)
+		fb->panel->bpp = max_size / (fb->panel->mode.xres *
+				fb->panel->mode.yres) * 8;
+	else
+		fb->panel->bpp = 32;
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+	fb->panel->cntl |= CNTL_BEBO;
+#endif
+
+	if (of_property_read_u32_array(node, "panel-dimensions",
+			dimensions, 2) == 0) {
+		fb->panel->width = dimensions[0];
+		fb->panel->height = dimensions[1];
+	} else {
+		fb->panel->width = -1;
+		fb->panel->height = -1;
+	}
+
+	panel_type = of_get_property(node, "panel-type", &len);
+	if (strncmp(panel_type, "tft", len) == 0)
+		return clcdfb_of_get_tft_panel(node, fb->panel);
+	else
+		return -EINVAL;
+}
+
+static int clcdfb_of_vram_setup(struct clcd_fb *fb)
+{
+	struct device_node *node = of_parse_phandle(fb->dev->dev.of_node,
+			"video-ram", 0);
+	u64 size;
+	int err;
+
+	if (!node)
+		return -ENODEV;
+
+	err = clcdfb_of_init_display(fb);
+	if (err)
+		return err;
+
+	fb->fb.screen_base = of_iomap(node, 0);
+	if (!fb->fb.screen_base)
+		return -ENOMEM;
+
+	fb->fb.fix.smem_start = of_translate_address(node,
+			of_get_address(node, 0, &size, NULL));
+	fb->fb.fix.smem_len = size;
+
+	return 0;
+}
+
+static int clcdfb_of_vram_mmap(struct clcd_fb *fb, struct vm_area_struct *vma)
+{
+	unsigned long off, user_size, kernel_size;
+
+	off = vma->vm_pgoff << PAGE_SHIFT;
+	user_size = vma->vm_end - vma->vm_start;
+	kernel_size = fb->fb.fix.smem_len;
+
+	if (off >= kernel_size || user_size > (kernel_size - off))
+		return -ENXIO;
+
+	return remap_pfn_range(vma, vma->vm_start,
+			__phys_to_pfn(fb->fb.fix.smem_start) + vma->vm_pgoff,
+			user_size,
+			pgprot_writecombine(vma->vm_page_prot));
+}
+
+static void clcdfb_of_vram_remove(struct clcd_fb *fb)
+{
+	iounmap(fb->fb.screen_base);
+}
+
+static int clcdfb_of_dma_setup(struct clcd_fb *fb)
+{
+	unsigned long framesize;
+	dma_addr_t dma;
+	int err;
+
+	err = clcdfb_of_init_display(fb);
+	if (err)
+		return err;
+
+	framesize = fb->panel->mode.xres * fb->panel->mode.yres *
+			fb->panel->bpp / 8;
+	fb->fb.screen_base = dma_alloc_writecombine(&fb->dev->dev, framesize,
+			&dma, GFP_KERNEL);
+	if (!fb->fb.screen_base)
+		return -ENOMEM;
+
+	fb->fb.fix.smem_start = dma;
+	fb->fb.fix.smem_len = framesize;
+
+	return 0;
+}
+
+static int clcdfb_of_dma_mmap(struct clcd_fb *fb, struct vm_area_struct *vma)
+{
+	return dma_mmap_writecombine(&fb->dev->dev, vma, fb->fb.screen_base,
+			fb->fb.fix.smem_start, fb->fb.fix.smem_len);
+}
+
+static void clcdfb_of_dma_remove(struct clcd_fb *fb)
+{
+	dma_free_writecombine(&fb->dev->dev, fb->fb.fix.smem_len,
+			fb->fb.screen_base, fb->fb.fix.smem_start);
+}
+
+static struct clcd_board *clcdfb_of_get_board(struct amba_device *dev)
+{
+	struct clcd_board *board = devm_kzalloc(&dev->dev, sizeof(*board),
+			GFP_KERNEL);
+	struct device_node *node = dev->dev.of_node;
+
+	if (!board)
+		return NULL;
+
+	board->name = of_node_full_name(node);
+	board->caps = CLCD_CAP_ALL;
+	board->check = clcdfb_check;
+	board->decode = clcdfb_decode;
+	if (of_find_property(node, "video-ram", NULL)) {
+		board->setup = clcdfb_of_vram_setup;
+		board->mmap = clcdfb_of_vram_mmap;
+		board->remove = clcdfb_of_vram_remove;
+	} else {
+		board->setup = clcdfb_of_dma_setup;
+		board->mmap = clcdfb_of_dma_mmap;
+		board->remove = clcdfb_of_dma_remove;
+	}
+
+	return board;
+}
+#else
+static struct clcd_board *clcdfb_of_get_board(struct amba_dev *dev)
+{
+	return NULL;
+}
+#endif
+
 static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id)
 {
 	struct clcd_board *board = dev->dev.platform_data;
@@ -549,6 +757,9 @@ static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id)
 	int ret;
 
 	if (!board)
+		board = clcdfb_of_get_board(dev);
+
+	if (!board)
 		return -EINVAL;
 
 	ret = amba_request_regions(dev, NULL);
-- 
1.8.1.2

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

This patch adds basic DT bindings for the PL11x CLCD cells
and make their fbdev driver use them.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---

Changes since v1:
- minor code cleanups as suggested by Sylwester Nawrocki

 .../devicetree/bindings/video/arm,pl11x.txt        |  87 +++++++++
 drivers/video/Kconfig                              |   1 +
 drivers/video/amba-clcd.c                          | 211 +++++++++++++++++++++
 3 files changed, 299 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/arm,pl11x.txt

diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
new file mode 100644
index 0000000..7eb77aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
@@ -0,0 +1,87 @@
+* ARM PrimeCell Color LCD (CLCD) Controller PL110/PL111
+
+See also Documentation/devicetree/bindings/arm/primecell.txt
+
+Required properties:
+
+- compatible: must be one of:
+			"arm,pl110", "arm,primecell"
+			"arm,pl111", "arm,primecell"
+- reg: base address and size of the control registers block
+- interrupts: either a single interrupt specifier representing the
+		combined interrupt output (CLCDINTR) or an array of
+		four interrupt specifiers for CLCDMBEINTR,
+		CLCDVCOMPINTR, CLCDLNBUINTR, CLCDFUFINTR; in the
+		latter case interrupt names must be specified
+		(see below)
+- interrupt-names: when four interrupts are specified, their names:
+			"mbe", "vcomp", "lnbu", "fuf"
+			must be specified in order respective to the
+			interrupt specifiers
+- clocks: contains phandle and clock specifier pairs for the entries
+		in the clock-names property. See
+		Documentation/devicetree/binding/clock/clock-bindings.txt
+- clocks names: should contain "clcdclk" and "apb_pclk"
+
+Optional properties:
+
+- video-ram: phandle to a node describing specialized video memory
+		(that is *not* described in the top level "memory" node)
+		that must be used as a framebuffer, eg. due to restrictions
+		of the system interconnect; the node must contain a
+		standard reg property describing the address and the size
+		of the memory area
+- max-framebuffer-size: maximum size in bytes of the framebuffer the
+			system can handle, eg. in terms of available
+			memory bandwidth
+
+In the simplest case of a display panel being connected directly to the
+CLCD, it can be described in the node:
+
+- panel-dimensions: (optional) array of two numbers (width and height)
+			describing physical dimension in mm of the panel
+- panel-type: (required) must be "tft" or "stn", defines panel type
+- panel-tft-interface: (for "tft" panel type) array of 3 numbers defining
+			widths in bits of the R, G and B components
+- panel-tft-rb-swapped: (for "tft" panel type) if present means that
+			the R & B components are swapped on the board
+- panel-stn-color: (for "stn" panel type) if present means that
+			the panel is a colour STN display, if missing
+			is a monochrome display
+- panel-stn-dual: (for "stn" panel type) if present means that there
+			are two STN displays connected
+- panel-stn-4bit: (for monochrome "stn" panel) if present means
+			that the monochrome display is connected
+			via 4 bit-wide interface
+- display-timings: standard display timings sub-node, see
+			Documentation/devicetree/bindings/video/display-timing.txt
+
+Example:
+
+			clcd at 1f0000 {
+				compatible = "arm,pl111", "arm,primecell";
+				reg = <0x1f0000 0x1000>;
+				interrupts = <14>;
+				clocks = <&v2m_oscclk1>, <&smbclk>;
+				clock-names = "clcdclk", "apb_pclk";
+
+				video-ram = <&v2m_vram>;
+				max-framebuffer-size = <614400>; /* 640x480 16bpp */
+
+				panel-type = "tft";
+				panel-tft-interface = <8 8 8>;
+				display-timings {
+					native-mode = <&v2m_clcd_timing0>;
+					v2m_clcd_timing0: vga {
+						clock-frequency = <25175000>;
+						hactive = <640>;
+						hback-porch = <40>;
+						hfront-porch = <24>;
+						hsync-len = <96>;
+						vactive = <480>;
+						vback-porch = <32>;
+						vfront-porch = <11>;
+						vsync-len = <2>;
+					};
+				};
+			};
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 4cf1e1d..375bf63 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -316,6 +316,7 @@ config FB_ARMCLCD
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
+	select VIDEOMODE_HELPERS if OF
 	help
 	  This framebuffer device driver is for the ARM PrimeCell PL110
 	  Colour LCD controller.  ARM PrimeCells provide the building
diff --git a/drivers/video/amba-clcd.c b/drivers/video/amba-clcd.c
index 0a2cce7..7f36964 100644
--- a/drivers/video/amba-clcd.c
+++ b/drivers/video/amba-clcd.c
@@ -25,6 +25,11 @@
 #include <linux/amba/clcd.h>
 #include <linux/clk.h>
 #include <linux/hardirq.h>
+#include <linux/dma-mapping.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <video/of_display_timing.h>
+#include <video/of_videomode.h>
 
 #include <asm/sizes.h>
 
@@ -542,6 +547,209 @@ static int clcdfb_register(struct clcd_fb *fb)
 	return ret;
 }
 
+#ifdef CONFIG_OF
+static int clcdfb_of_get_tft_panel(struct device_node *node,
+		struct clcd_panel *panel)
+{
+	int err;
+	u32 rgb[3];
+
+	err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3);
+	if (err)
+		return err;
+
+	/* Bypass pixel clock divider, data output on the falling edge */
+	panel->tim2 = TIM2_BCD | TIM2_IPC;
+
+	/* TFT display, vert. comp. interrupt at the start of the back porch */
+	panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
+
+	if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4)
+		panel->caps |= CLCD_CAP_444;
+	if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5)
+		panel->caps |= CLCD_CAP_5551;
+	if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5)
+		panel->caps |= CLCD_CAP_565;
+	if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8)
+		panel->caps |= CLCD_CAP_888;
+
+	if (of_get_property(node, "panel-tft-rb-swapped", NULL))
+		panel->caps &= ~CLCD_CAP_RGB;
+	else
+		panel->caps &= ~CLCD_CAP_BGR;
+
+	return 0;
+}
+
+static int clcdfb_snprintf_mode(char *buf, int size, struct fb_videomode *mode)
+{
+	return snprintf(buf, size, "%ux%u@%u", mode->xres, mode->yres,
+			mode->refresh);
+}
+
+static int clcdfb_of_init_display(struct clcd_fb *fb)
+{
+	struct device_node *node = fb->dev->dev.of_node;
+	int err, len;
+	char *mode_name;
+	u32 max_size;
+	u32 dimensions[2];
+	const char *panel_type;
+
+	fb->panel = devm_kzalloc(&fb->dev->dev, sizeof(*fb->panel), GFP_KERNEL);
+	if (!fb->panel)
+		return -ENOMEM;
+
+	err = of_get_fb_videomode(node, &fb->panel->mode, OF_USE_NATIVE_MODE);
+	if (err)
+		return err;
+
+	len = clcdfb_snprintf_mode(NULL, 0, &fb->panel->mode);
+	mode_name = devm_kzalloc(&fb->dev->dev, len + 1, GFP_KERNEL);
+	clcdfb_snprintf_mode(mode_name, len + 1, &fb->panel->mode);
+	fb->panel->mode.name = mode_name;
+
+	err = of_property_read_u32(node, "max-framebuffer-size", &max_size);
+	if (!err)
+		fb->panel->bpp = max_size / (fb->panel->mode.xres *
+				fb->panel->mode.yres) * 8;
+	else
+		fb->panel->bpp = 32;
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+	fb->panel->cntl |= CNTL_BEBO;
+#endif
+
+	if (of_property_read_u32_array(node, "panel-dimensions",
+			dimensions, 2) == 0) {
+		fb->panel->width = dimensions[0];
+		fb->panel->height = dimensions[1];
+	} else {
+		fb->panel->width = -1;
+		fb->panel->height = -1;
+	}
+
+	panel_type = of_get_property(node, "panel-type", &len);
+	if (strncmp(panel_type, "tft", len) == 0)
+		return clcdfb_of_get_tft_panel(node, fb->panel);
+	else
+		return -EINVAL;
+}
+
+static int clcdfb_of_vram_setup(struct clcd_fb *fb)
+{
+	struct device_node *node = of_parse_phandle(fb->dev->dev.of_node,
+			"video-ram", 0);
+	u64 size;
+	int err;
+
+	if (!node)
+		return -ENODEV;
+
+	err = clcdfb_of_init_display(fb);
+	if (err)
+		return err;
+
+	fb->fb.screen_base = of_iomap(node, 0);
+	if (!fb->fb.screen_base)
+		return -ENOMEM;
+
+	fb->fb.fix.smem_start = of_translate_address(node,
+			of_get_address(node, 0, &size, NULL));
+	fb->fb.fix.smem_len = size;
+
+	return 0;
+}
+
+static int clcdfb_of_vram_mmap(struct clcd_fb *fb, struct vm_area_struct *vma)
+{
+	unsigned long off, user_size, kernel_size;
+
+	off = vma->vm_pgoff << PAGE_SHIFT;
+	user_size = vma->vm_end - vma->vm_start;
+	kernel_size = fb->fb.fix.smem_len;
+
+	if (off >= kernel_size || user_size > (kernel_size - off))
+		return -ENXIO;
+
+	return remap_pfn_range(vma, vma->vm_start,
+			__phys_to_pfn(fb->fb.fix.smem_start) + vma->vm_pgoff,
+			user_size,
+			pgprot_writecombine(vma->vm_page_prot));
+}
+
+static void clcdfb_of_vram_remove(struct clcd_fb *fb)
+{
+	iounmap(fb->fb.screen_base);
+}
+
+static int clcdfb_of_dma_setup(struct clcd_fb *fb)
+{
+	unsigned long framesize;
+	dma_addr_t dma;
+	int err;
+
+	err = clcdfb_of_init_display(fb);
+	if (err)
+		return err;
+
+	framesize = fb->panel->mode.xres * fb->panel->mode.yres *
+			fb->panel->bpp / 8;
+	fb->fb.screen_base = dma_alloc_writecombine(&fb->dev->dev, framesize,
+			&dma, GFP_KERNEL);
+	if (!fb->fb.screen_base)
+		return -ENOMEM;
+
+	fb->fb.fix.smem_start = dma;
+	fb->fb.fix.smem_len = framesize;
+
+	return 0;
+}
+
+static int clcdfb_of_dma_mmap(struct clcd_fb *fb, struct vm_area_struct *vma)
+{
+	return dma_mmap_writecombine(&fb->dev->dev, vma, fb->fb.screen_base,
+			fb->fb.fix.smem_start, fb->fb.fix.smem_len);
+}
+
+static void clcdfb_of_dma_remove(struct clcd_fb *fb)
+{
+	dma_free_writecombine(&fb->dev->dev, fb->fb.fix.smem_len,
+			fb->fb.screen_base, fb->fb.fix.smem_start);
+}
+
+static struct clcd_board *clcdfb_of_get_board(struct amba_device *dev)
+{
+	struct clcd_board *board = devm_kzalloc(&dev->dev, sizeof(*board),
+			GFP_KERNEL);
+	struct device_node *node = dev->dev.of_node;
+
+	if (!board)
+		return NULL;
+
+	board->name = of_node_full_name(node);
+	board->caps = CLCD_CAP_ALL;
+	board->check = clcdfb_check;
+	board->decode = clcdfb_decode;
+	if (of_find_property(node, "video-ram", NULL)) {
+		board->setup = clcdfb_of_vram_setup;
+		board->mmap = clcdfb_of_vram_mmap;
+		board->remove = clcdfb_of_vram_remove;
+	} else {
+		board->setup = clcdfb_of_dma_setup;
+		board->mmap = clcdfb_of_dma_mmap;
+		board->remove = clcdfb_of_dma_remove;
+	}
+
+	return board;
+}
+#else
+static struct clcd_board *clcdfb_of_get_board(struct amba_dev *dev)
+{
+	return NULL;
+}
+#endif
+
 static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id)
 {
 	struct clcd_board *board = dev->dev.platform_data;
@@ -549,6 +757,9 @@ static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id)
 	int ret;
 
 	if (!board)
+		board = clcdfb_of_get_board(dev);
+
+	if (!board)
 		return -EINVAL;
 
 	ret = amba_request_regions(dev, NULL);
-- 
1.8.1.2

[PATCH v2 2/2] ARM: vexpress: Add CLCD Device Tree properties

[Pawel Moll]

... for V2M-P1 motherboard CLCD (limited to 640x480 16bpp and using
dedicated video RAM bank) and for V2P-CA9 (up to 1024x768 16bpp).

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 arch/arm/boot/dts/vexpress-v2m-rs1.dtsi | 25 +++++++++++++++++++++++--
 arch/arm/boot/dts/vexpress-v2m.dtsi     | 25 +++++++++++++++++++++++--
 arch/arm/boot/dts/vexpress-v2p-ca9.dts  | 20 ++++++++++++++++++++
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
index ac870fb..91dff88 100644
--- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
@@ -41,7 +41,7 @@
 			bank-width = <4>;
 		};
 
-		vram@2,00000000 {
+		v2m_vram: vram@2,00000000 {
 			compatible = "arm,vexpress-vram";
 			reg = <2 0x00000000 0x00800000>;
 		};
@@ -233,6 +233,27 @@
 				interrupts = <14>;
 				clocks = <&v2m_oscclk1>, <&smbclk>;
 				clock-names = "clcdclk", "apb_pclk";
+
+				video-ram = <&v2m_vram>;
+				max-framebuffer-size = <614400>; /* 640x480 16bpp */
+
+				/* This is the best the CLCD can do here */
+				panel-type = "tft";
+				panel-tft-interface = <8 8 8>;
+				display-timings {
+					native-mode = <&v2m_clcd_timing0>;
+					v2m_clcd_timing0: vga {
+						clock-frequency = <25175000>;
+						hactive = <640>;
+						hback-porch = <40>;
+						hfront-porch = <24>;
+						hsync-len = <96>;
+						vactive = <480>;
+						vback-porch = <32>;
+						vfront-porch = <11>;
+						vsync-len = <2>;
+					};
+				};
 			};
 		};
 
@@ -282,7 +303,7 @@
 				/* CLCD clock */
 				compatible = "arm,vexpress-osc";
 				arm,vexpress-sysreg,func = <1 1>;
-				freq-range = <23750000 63500000>;
+				freq-range = <23750000 65000000>;
 				#clock-cells = <0>;
 				clock-output-names = "v2m:oscclk1";
 			};
diff --git a/arch/arm/boot/dts/vexpress-v2m.dtsi b/arch/arm/boot/dts/vexpress-v2m.dtsi
index f142036..4bc348c 100644
--- a/arch/arm/boot/dts/vexpress-v2m.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m.dtsi
@@ -40,7 +40,7 @@
 			bank-width = <4>;
 		};
 
-		vram@3,00000000 {
+		v2m_vram: vram@3,00000000 {
 			compatible = "arm,vexpress-vram";
 			reg = <3 0x00000000 0x00800000>;
 		};
@@ -232,6 +232,27 @@
 				interrupts = <14>;
 				clocks = <&v2m_oscclk1>, <&smbclk>;
 				clock-names = "clcdclk", "apb_pclk";
+
+				video-ram = <&v2m_vram>;
+				max-framebuffer-size = <614400>; /* 640x480 16bpp */
+
+				/* This is the best the CLCD can do here */
+				panel-type = "tft";
+				panel-tft-interface = <8 8 8>;
+				display-timings {
+					native-mode = <&v2m_clcd_timing0>;
+					v2m_clcd_timing0: vga {
+						clock-frequency = <25175000>;
+						hactive = <640>;
+						hback-porch = <40>;
+						hfront-porch = <24>;
+						hsync-len = <96>;
+						vactive = <480>;
+						vback-porch = <32>;
+						vfront-porch = <11>;
+						vsync-len = <2>;
+					};
+				};
 			};
 		};
 
@@ -281,7 +302,7 @@
 				/* CLCD clock */
 				compatible = "arm,vexpress-osc";
 				arm,vexpress-sysreg,func = <1 1>;
-				freq-range = <23750000 63500000>;
+				freq-range = <23750000 65000000>;
 				#clock-cells = <0>;
 				clock-output-names = "v2m:oscclk1";
 			};
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
index 62d9b22..fbe1c998 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
@@ -73,6 +73,26 @@
 		interrupts = <0 44 4>;
 		clocks = <&oscclk1>, <&oscclk2>;
 		clock-names = "clcdclk", "apb_pclk";
+
+		max-framebuffer-size = <1572864>; /* 1024x768 16bpp */
+
+		/* This is the best the CLCD can do here */
+		panel-type = "tft";
+		panel-tft-interface = <8 8 8>;
+		display-timings {
+			native-mode = <&clcd_timing0>;
+			clcd_timing0: xga {
+				clock-frequency = <63500127>;
+				hactive = <1024>;
+				hback-porch = <152>;
+				hfront-porch = <48>;
+				hsync-len = <104>;
+				vactive = <768>;
+				vback-porch = <23>;
+				vfront-porch = <3>;
+				vsync-len = <4>;
+			};
+		};
 	};
 
 	memory-controller@100e0000 {
-- 
1.8.1.2

[PATCH v2 2/2] ARM: vexpress: Add CLCD Device Tree properties

[Pawel Moll]

... for V2M-P1 motherboard CLCD (limited to 640x480 16bpp and using
dedicated video RAM bank) and for V2P-CA9 (up to 1024x768 16bpp).

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 arch/arm/boot/dts/vexpress-v2m-rs1.dtsi | 25 +++++++++++++++++++++++--
 arch/arm/boot/dts/vexpress-v2m.dtsi     | 25 +++++++++++++++++++++++--
 arch/arm/boot/dts/vexpress-v2p-ca9.dts  | 20 ++++++++++++++++++++
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
index ac870fb..91dff88 100644
--- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
@@ -41,7 +41,7 @@
 			bank-width = <4>;
 		};
 
-		vram@2,00000000 {
+		v2m_vram: vram@2,00000000 {
 			compatible = "arm,vexpress-vram";
 			reg = <2 0x00000000 0x00800000>;
 		};
@@ -233,6 +233,27 @@
 				interrupts = <14>;
 				clocks = <&v2m_oscclk1>, <&smbclk>;
 				clock-names = "clcdclk", "apb_pclk";
+
+				video-ram = <&v2m_vram>;
+				max-framebuffer-size = <614400>; /* 640x480 16bpp */
+
+				/* This is the best the CLCD can do here */
+				panel-type = "tft";
+				panel-tft-interface = <8 8 8>;
+				display-timings {
+					native-mode = <&v2m_clcd_timing0>;
+					v2m_clcd_timing0: vga {
+						clock-frequency = <25175000>;
+						hactive = <640>;
+						hback-porch = <40>;
+						hfront-porch = <24>;
+						hsync-len = <96>;
+						vactive = <480>;
+						vback-porch = <32>;
+						vfront-porch = <11>;
+						vsync-len = <2>;
+					};
+				};
 			};
 		};
 
@@ -282,7 +303,7 @@
 				/* CLCD clock */
 				compatible = "arm,vexpress-osc";
 				arm,vexpress-sysreg,func = <1 1>;
-				freq-range = <23750000 63500000>;
+				freq-range = <23750000 65000000>;
 				#clock-cells = <0>;
 				clock-output-names = "v2m:oscclk1";
 			};
diff --git a/arch/arm/boot/dts/vexpress-v2m.dtsi b/arch/arm/boot/dts/vexpress-v2m.dtsi
index f142036..4bc348c 100644
--- a/arch/arm/boot/dts/vexpress-v2m.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m.dtsi
@@ -40,7 +40,7 @@
 			bank-width = <4>;
 		};
 
-		vram@3,00000000 {
+		v2m_vram: vram@3,00000000 {
 			compatible = "arm,vexpress-vram";
 			reg = <3 0x00000000 0x00800000>;
 		};
@@ -232,6 +232,27 @@
 				interrupts = <14>;
 				clocks = <&v2m_oscclk1>, <&smbclk>;
 				clock-names = "clcdclk", "apb_pclk";
+
+				video-ram = <&v2m_vram>;
+				max-framebuffer-size = <614400>; /* 640x480 16bpp */
+
+				/* This is the best the CLCD can do here */
+				panel-type = "tft";
+				panel-tft-interface = <8 8 8>;
+				display-timings {
+					native-mode = <&v2m_clcd_timing0>;
+					v2m_clcd_timing0: vga {
+						clock-frequency = <25175000>;
+						hactive = <640>;
+						hback-porch = <40>;
+						hfront-porch = <24>;
+						hsync-len = <96>;
+						vactive = <480>;
+						vback-porch = <32>;
+						vfront-porch = <11>;
+						vsync-len = <2>;
+					};
+				};
 			};
 		};
 
@@ -281,7 +302,7 @@
 				/* CLCD clock */
 				compatible = "arm,vexpress-osc";
 				arm,vexpress-sysreg,func = <1 1>;
-				freq-range = <23750000 63500000>;
+				freq-range = <23750000 65000000>;
 				#clock-cells = <0>;
 				clock-output-names = "v2m:oscclk1";
 			};
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
index 62d9b22..fbe1c998 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
@@ -73,6 +73,26 @@
 		interrupts = <0 44 4>;
 		clocks = <&oscclk1>, <&oscclk2>;
 		clock-names = "clcdclk", "apb_pclk";
+
+		max-framebuffer-size = <1572864>; /* 1024x768 16bpp */
+
+		/* This is the best the CLCD can do here */
+		panel-type = "tft";
+		panel-tft-interface = <8 8 8>;
+		display-timings {
+			native-mode = <&clcd_timing0>;
+			clcd_timing0: xga {
+				clock-frequency = <63500127>;
+				hactive = <1024>;
+				hback-porch = <152>;
+				hfront-porch = <48>;
+				hsync-len = <104>;
+				vactive = <768>;
+				vback-porch = <23>;
+				vfront-porch = <3>;
+				vsync-len = <4>;
+			};
+		};
 	};
 
 	memory-controller@100e0000 {
-- 
1.8.1.2

[PATCH v2 2/2] ARM: vexpress: Add CLCD Device Tree properties

[Pawel Moll]

... for V2M-P1 motherboard CLCD (limited to 640x480 16bpp and using
dedicated video RAM bank) and for V2P-CA9 (up to 1024x768 16bpp).

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 arch/arm/boot/dts/vexpress-v2m-rs1.dtsi | 25 +++++++++++++++++++++++--
 arch/arm/boot/dts/vexpress-v2m.dtsi     | 25 +++++++++++++++++++++++--
 arch/arm/boot/dts/vexpress-v2p-ca9.dts  | 20 ++++++++++++++++++++
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
index ac870fb..91dff88 100644
--- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
@@ -41,7 +41,7 @@
 			bank-width = <4>;
 		};
 
-		vram at 2,00000000 {
+		v2m_vram: vram at 2,00000000 {
 			compatible = "arm,vexpress-vram";
 			reg = <2 0x00000000 0x00800000>;
 		};
@@ -233,6 +233,27 @@
 				interrupts = <14>;
 				clocks = <&v2m_oscclk1>, <&smbclk>;
 				clock-names = "clcdclk", "apb_pclk";
+
+				video-ram = <&v2m_vram>;
+				max-framebuffer-size = <614400>; /* 640x480 16bpp */
+
+				/* This is the best the CLCD can do here */
+				panel-type = "tft";
+				panel-tft-interface = <8 8 8>;
+				display-timings {
+					native-mode = <&v2m_clcd_timing0>;
+					v2m_clcd_timing0: vga {
+						clock-frequency = <25175000>;
+						hactive = <640>;
+						hback-porch = <40>;
+						hfront-porch = <24>;
+						hsync-len = <96>;
+						vactive = <480>;
+						vback-porch = <32>;
+						vfront-porch = <11>;
+						vsync-len = <2>;
+					};
+				};
 			};
 		};
 
@@ -282,7 +303,7 @@
 				/* CLCD clock */
 				compatible = "arm,vexpress-osc";
 				arm,vexpress-sysreg,func = <1 1>;
-				freq-range = <23750000 63500000>;
+				freq-range = <23750000 65000000>;
 				#clock-cells = <0>;
 				clock-output-names = "v2m:oscclk1";
 			};
diff --git a/arch/arm/boot/dts/vexpress-v2m.dtsi b/arch/arm/boot/dts/vexpress-v2m.dtsi
index f142036..4bc348c 100644
--- a/arch/arm/boot/dts/vexpress-v2m.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m.dtsi
@@ -40,7 +40,7 @@
 			bank-width = <4>;
 		};
 
-		vram at 3,00000000 {
+		v2m_vram: vram at 3,00000000 {
 			compatible = "arm,vexpress-vram";
 			reg = <3 0x00000000 0x00800000>;
 		};
@@ -232,6 +232,27 @@
 				interrupts = <14>;
 				clocks = <&v2m_oscclk1>, <&smbclk>;
 				clock-names = "clcdclk", "apb_pclk";
+
+				video-ram = <&v2m_vram>;
+				max-framebuffer-size = <614400>; /* 640x480 16bpp */
+
+				/* This is the best the CLCD can do here */
+				panel-type = "tft";
+				panel-tft-interface = <8 8 8>;
+				display-timings {
+					native-mode = <&v2m_clcd_timing0>;
+					v2m_clcd_timing0: vga {
+						clock-frequency = <25175000>;
+						hactive = <640>;
+						hback-porch = <40>;
+						hfront-porch = <24>;
+						hsync-len = <96>;
+						vactive = <480>;
+						vback-porch = <32>;
+						vfront-porch = <11>;
+						vsync-len = <2>;
+					};
+				};
 			};
 		};
 
@@ -281,7 +302,7 @@
 				/* CLCD clock */
 				compatible = "arm,vexpress-osc";
 				arm,vexpress-sysreg,func = <1 1>;
-				freq-range = <23750000 63500000>;
+				freq-range = <23750000 65000000>;
 				#clock-cells = <0>;
 				clock-output-names = "v2m:oscclk1";
 			};
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
index 62d9b22..fbe1c998 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
@@ -73,6 +73,26 @@
 		interrupts = <0 44 4>;
 		clocks = <&oscclk1>, <&oscclk2>;
 		clock-names = "clcdclk", "apb_pclk";
+
+		max-framebuffer-size = <1572864>; /* 1024x768 16bpp */
+
+		/* This is the best the CLCD can do here */
+		panel-type = "tft";
+		panel-tft-interface = <8 8 8>;
+		display-timings {
+			native-mode = <&clcd_timing0>;
+			clcd_timing0: xga {
+				clock-frequency = <63500127>;
+				hactive = <1024>;
+				hback-porch = <152>;
+				hfront-porch = <48>;
+				hsync-len = <104>;
+				vactive = <768>;
+				vback-porch = <23>;
+				vfront-porch = <3>;
+				vsync-len = <4>;
+			};
+		};
 	};
 
 	memory-controller at 100e0000 {
-- 
1.8.1.2

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Stephen Warren]

On 09/10/2013 04:25 AM, Pawel Moll wrote:
> This patch adds basic DT bindings for the PL11x CLCD cells
> and make their fbdev driver use them.

> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt

> +Optional properties:
> +
> +- video-ram: phandle to a node describing specialized video memory
> +		(that is *not* described in the top level "memory" node)
> +		that must be used as a framebuffer, eg. due to restrictions
> +		of the system interconnect; the node must contain a
> +		standard reg property describing the address and the size
> +		of the memory area

Should this use the "CMA bindings" that are being proposed at the moment?

Even if not, I'm not quite clear on what the referenced node is supposed
to contain; is just a reg property enough, so you'd see the following at
a completely arbitrary location in the DT:

framebuffer-mem {
    reg = <0x80000000 0x00100000>;
};

I'm not sure what the benefit of making this a standalone node is; why
not just put the base/size directly into the video-ram property in the
CLCD node?

> +- max-framebuffer-size: maximum size in bytes of the framebuffer the
> +			system can handle, eg. in terms of available
> +			memory bandwidth

Size doesn't imply bandwidth, due to the potential for varying bpp,
frame-rates, margin/porch sizes, etc. If this is a bandwidth limit,
shouldn't we instead represent that value directly, perhaps along with
some multiplier to convert theoretical bandwidth to practical bandwidth
(to account for memory protocol and controller overheads)?

...
> +- panel-type: (required) must be "tft" or "stn", defines panel type
...
> +- panel-stn-4bit: (for monochrome "stn" panel) if present means
> +			that the monochrome display is connected
> +			via 4 bit-wide interface

I just wanted to confirm that those are a complete/direct representation
of all the HW options the module supports?

> +- display-timings: standard display timings sub-node, see
> +			Documentation/devicetree/bindings/video/display-timing.txt

Should that be in a "Required sub-nodes" section (I assume required not
optional) rather than "Optional Properties"?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Stephen Warren]

On 09/10/2013 04:25 AM, Pawel Moll wrote:
> This patch adds basic DT bindings for the PL11x CLCD cells
> and make their fbdev driver use them.

> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt

> +Optional properties:
> +
> +- video-ram: phandle to a node describing specialized video memory
> +		(that is *not* described in the top level "memory" node)
> +		that must be used as a framebuffer, eg. due to restrictions
> +		of the system interconnect; the node must contain a
> +		standard reg property describing the address and the size
> +		of the memory area

Should this use the "CMA bindings" that are being proposed at the moment?

Even if not, I'm not quite clear on what the referenced node is supposed
to contain; is just a reg property enough, so you'd see the following at
a completely arbitrary location in the DT:

framebuffer-mem {
    reg = <0x80000000 0x00100000>;
};

I'm not sure what the benefit of making this a standalone node is; why
not just put the base/size directly into the video-ram property in the
CLCD node?

> +- max-framebuffer-size: maximum size in bytes of the framebuffer the
> +			system can handle, eg. in terms of available
> +			memory bandwidth

Size doesn't imply bandwidth, due to the potential for varying bpp,
frame-rates, margin/porch sizes, etc. If this is a bandwidth limit,
shouldn't we instead represent that value directly, perhaps along with
some multiplier to convert theoretical bandwidth to practical bandwidth
(to account for memory protocol and controller overheads)?

...
> +- panel-type: (required) must be "tft" or "stn", defines panel type
...
> +- panel-stn-4bit: (for monochrome "stn" panel) if present means
> +			that the monochrome display is connected
> +			via 4 bit-wide interface

I just wanted to confirm that those are a complete/direct representation
of all the HW options the module supports?

> +- display-timings: standard display timings sub-node, see
> +			Documentation/devicetree/bindings/video/display-timing.txt

Should that be in a "Required sub-nodes" section (I assume required not
optional) rather than "Optional Properties"?

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Stephen Warren]

On 09/10/2013 04:25 AM, Pawel Moll wrote:
> This patch adds basic DT bindings for the PL11x CLCD cells
> and make their fbdev driver use them.

> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt

> +Optional properties:
> +
> +- video-ram: phandle to a node describing specialized video memory
> +		(that is *not* described in the top level "memory" node)
> +		that must be used as a framebuffer, eg. due to restrictions
> +		of the system interconnect; the node must contain a
> +		standard reg property describing the address and the size
> +		of the memory area

Should this use the "CMA bindings" that are being proposed at the moment?

Even if not, I'm not quite clear on what the referenced node is supposed
to contain; is just a reg property enough, so you'd see the following at
a completely arbitrary location in the DT:

framebuffer-mem {
    reg = <0x80000000 0x00100000>;
};

I'm not sure what the benefit of making this a standalone node is; why
not just put the base/size directly into the video-ram property in the
CLCD node?

> +- max-framebuffer-size: maximum size in bytes of the framebuffer the
> +			system can handle, eg. in terms of available
> +			memory bandwidth

Size doesn't imply bandwidth, due to the potential for varying bpp,
frame-rates, margin/porch sizes, etc. If this is a bandwidth limit,
shouldn't we instead represent that value directly, perhaps along with
some multiplier to convert theoretical bandwidth to practical bandwidth
(to account for memory protocol and controller overheads)?

...
> +- panel-type: (required) must be "tft" or "stn", defines panel type
...
> +- panel-stn-4bit: (for monochrome "stn" panel) if present means
> +			that the monochrome display is connected
> +			via 4 bit-wide interface

I just wanted to confirm that those are a complete/direct representation
of all the HW options the module supports?

> +- display-timings: standard display timings sub-node, see
> +			Documentation/devicetree/bindings/video/display-timing.txt

Should that be in a "Required sub-nodes" section (I assume required not
optional) rather than "Optional Properties"?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Russell King - ARM Linux]

On Tue, Sep 10, 2013 at 11:25:25AM +0100, Pawel Moll wrote:
> +#ifdef CONFIG_OF
> +static int clcdfb_of_get_tft_panel(struct device_node *node,
> +		struct clcd_panel *panel)
> +{
> +	int err;
> +	u32 rgb[3];
> +
> +	err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3);
> +	if (err)
> +		return err;
> +
> +	/* Bypass pixel clock divider, data output on the falling edge */
> +	panel->tim2 = TIM2_BCD | TIM2_IPC;
> +
> +	/* TFT display, vert. comp. interrupt at the start of the back porch */
> +	panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
> +
> +	if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4)
> +		panel->caps |= CLCD_CAP_444;
> +	if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5)
> +		panel->caps |= CLCD_CAP_5551;
> +	if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5)
> +		panel->caps |= CLCD_CAP_565;
> +	if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8)
> +		panel->caps |= CLCD_CAP_888;

This definitely isn't right.  Why does a panel which has 8 bits of RGB
get to indicate that it supports everything below it?

This is not how this hardware works.  Look at table A-7 in the TRM.
If you wire the CLCD controller directly to a panel which is 8-bit RGB
only, it can't support any of the other formats.

The CLCD controller itself can only support on the hardware 8-bits of
RGB or 5 bits of RGB plus an intensity bit, so that's two formats.

However, things are complicated by ARMs addition of an external mux
on the CLCD output, which gives us the other format possibilities
by munging the signals to give appropriate formats in the framebuffer
memory.  In other words, in RGB444 mode, the mux ends up taking
red from CLD[4:1], green from CLD[9:7,5], blue from CLD[14:13,11:10].

So, it's not true that if you have a RGB888 panel, you can support
all the lower BPPs.

This is one of the dangers of directly converting what's in the kernel
to DT properties without getting the hardware documentation and fully
understanding that first - remember, DT properties are supposed to be
based on the hardware, not the Linux implementation.

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Russell King - ARM Linux]

On Tue, Sep 10, 2013 at 11:25:25AM +0100, Pawel Moll wrote:
> +#ifdef CONFIG_OF
> +static int clcdfb_of_get_tft_panel(struct device_node *node,
> +		struct clcd_panel *panel)
> +{
> +	int err;
> +	u32 rgb[3];
> +
> +	err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3);
> +	if (err)
> +		return err;
> +
> +	/* Bypass pixel clock divider, data output on the falling edge */
> +	panel->tim2 = TIM2_BCD | TIM2_IPC;
> +
> +	/* TFT display, vert. comp. interrupt at the start of the back porch */
> +	panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
> +
> +	if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4)
> +		panel->caps |= CLCD_CAP_444;
> +	if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5)
> +		panel->caps |= CLCD_CAP_5551;
> +	if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5)
> +		panel->caps |= CLCD_CAP_565;
> +	if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8)
> +		panel->caps |= CLCD_CAP_888;

This definitely isn't right.  Why does a panel which has 8 bits of RGB
get to indicate that it supports everything below it?

This is not how this hardware works.  Look at table A-7 in the TRM.
If you wire the CLCD controller directly to a panel which is 8-bit RGB
only, it can't support any of the other formats.

The CLCD controller itself can only support on the hardware 8-bits of
RGB or 5 bits of RGB plus an intensity bit, so that's two formats.

However, things are complicated by ARMs addition of an external mux
on the CLCD output, which gives us the other format possibilities
by munging the signals to give appropriate formats in the framebuffer
memory.  In other words, in RGB444 mode, the mux ends up taking
red from CLD[4:1], green from CLD[9:7,5], blue from CLD[14:13,11:10].

So, it's not true that if you have a RGB888 panel, you can support
all the lower BPPs.

This is one of the dangers of directly converting what's in the kernel
to DT properties without getting the hardware documentation and fully
understanding that first - remember, DT properties are supposed to be
based on the hardware, not the Linux implementation.

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Russell King - ARM Linux]

On Tue, Sep 10, 2013 at 11:25:25AM +0100, Pawel Moll wrote:
> +#ifdef CONFIG_OF
> +static int clcdfb_of_get_tft_panel(struct device_node *node,
> +		struct clcd_panel *panel)
> +{
> +	int err;
> +	u32 rgb[3];
> +
> +	err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3);
> +	if (err)
> +		return err;
> +
> +	/* Bypass pixel clock divider, data output on the falling edge */
> +	panel->tim2 = TIM2_BCD | TIM2_IPC;
> +
> +	/* TFT display, vert. comp. interrupt at the start of the back porch */
> +	panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
> +
> +	if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4)
> +		panel->caps |= CLCD_CAP_444;
> +	if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5)
> +		panel->caps |= CLCD_CAP_5551;
> +	if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5)
> +		panel->caps |= CLCD_CAP_565;
> +	if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8)
> +		panel->caps |= CLCD_CAP_888;

This definitely isn't right.  Why does a panel which has 8 bits of RGB
get to indicate that it supports everything below it?

This is not how this hardware works.  Look at table A-7 in the TRM.
If you wire the CLCD controller directly to a panel which is 8-bit RGB
only, it can't support any of the other formats.

The CLCD controller itself can only support on the hardware 8-bits of
RGB or 5 bits of RGB plus an intensity bit, so that's two formats.

However, things are complicated by ARMs addition of an external mux
on the CLCD output, which gives us the other format possibilities
by munging the signals to give appropriate formats in the framebuffer
memory.  In other words, in RGB444 mode, the mux ends up taking
red from CLD[4:1], green from CLD[9:7,5], blue from CLD[14:13,11:10].

So, it's not true that if you have a RGB888 panel, you can support
all the lower BPPs.

This is one of the dangers of directly converting what's in the kernel
to DT properties without getting the hardware documentation and fully
understanding that first - remember, DT properties are supposed to be
based on the hardware, not the Linux implementation.

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Tue, 2013-09-10 at 18:32 +0100, Stephen Warren wrote:
> > +Optional properties:
> > +
> > +- video-ram: phandle to a node describing specialized video memory
> > +		(that is *not* described in the top level "memory" node)
> > +		that must be used as a framebuffer, eg. due to restrictions
> > +		of the system interconnect; the node must contain a
> > +		standard reg property describing the address and the size
> > +		of the memory area
> 
> Should this use the "CMA bindings" that are being proposed at the moment?

I expected this bit to be the hardest to get through :-)

The memory in question is *not* a part of "normal" RAM, therefore CMA
doesn't even know about it. The situation I have to deal with is a
system when CLCD's AHB master can *not* access the normal RAM, so the
designers kindly ;-) provided some static RAM which it can address.

> Even if not, I'm not quite clear on what the referenced node is supposed
> to contain; is just a reg property enough, so you'd see the following at
> a completely arbitrary location in the DT:
> 
> framebuffer-mem {
>     reg = <0x80000000 0x00100000>;
> };

The place wouldn't be random, no. Getting back to my scenario, the
"video" RAM, being close to CLCD, is (obviously) also addressable by the
core, as any other memory-mapped device. So its position is determined
by the platform memory map:

\ {
	#address-cell = <2>;
	#size-cell = <2>;
	static-memory-bus {
		#address-cell = <2>;
		#size-cell = <1>;
		ranges = <2 0 0 0x18000000 64M>;
		motherboard {
			ranges; 
			video-ram {
				reg = <2 0 8MB>;
			};
		};
	};
};

From the core's perspective it's just a bit of extra memory, you could,
for example, put a MTD ram disk on it. It seems to deserve a
representation in the tree then.

> I'm not sure what the benefit of making this a standalone node is; why
> not just put the base/size directly into the video-ram property in the
> CLCD node?

This is certainly an option. I've considered this as well, but thought
that the above made more sense.

Having said that, there is actually a use case, although a very
non-probable one, for having a direct number there... The interconnect
that CLCD is connected to could have different mapping than the
processor's one. In other word, the memory seen by the core at X, could
be accessed by the CLCD at Y. Or, in even more quirky situation, the
core may not have access to the memory at all, with the graphics being
generated only by GPU (or any other third party). Then the value would
mean "the address you have to use when generating AMBA read transactions
to be get some meaningful data" and would have to be known explicitly.

I guess it won't be hard to convince me it's a better option ;-)

> > +- max-framebuffer-size: maximum size in bytes of the framebuffer the
> > +			system can handle, eg. in terms of available
> > +			memory bandwidth
> 
> Size doesn't imply bandwidth, due to the potential for varying bpp,
> frame-rates, margin/porch sizes, etc. If this is a bandwidth limit,
> shouldn't we instead represent that value directly, perhaps along with
> some multiplier to convert theoretical bandwidth to practical bandwidth
> (to account for memory protocol and controller overheads)?

It's a *memory interface* bandwidth, so synchronization fields are
irrelevant here. And the bpp limit is actually being calculated from
this value, not the other way round. But I forgot about differing frame
rates, that's fact. So it probably should be:

- max-memory-bandwidth: maximum bandwidth in bytes per second that the
			cell's memory interface can handle

and can be then used to calculate maximum bpp depending on the selected
mode.

As to the multipliers... Although I hope that the SOC designer can
provide theoretical throughput data, the only practical way of getting
the value I was able to come up with was just trying different
combinations till cross the line, so there isn't much math to be done
further.

> ...
> > +- panel-type: (required) must be "tft" or "stn", defines panel type
> ...
> > +- panel-stn-4bit: (for monochrome "stn" panel) if present means
> > +			that the monochrome display is connected
> > +			via 4 bit-wide interface
> 
> I just wanted to confirm that those are a complete/direct representation
> of all the HW options the module supports?

Yes. TFT or STN. There can be one or STN panels connected. STN(s) can be
color or mono. Mono STN(s) can be driven via 4 or 8 bit wide interfaces.
Disclaimer: I'm not trying to pretend an expert here. I'm just basing
the above on the cell's spec.

> > +- display-timings: standard display timings sub-node, see
> > +			Documentation/devicetree/bindings/video/display-timing.txt
> 
> Should that be in a "Required sub-nodes" section (I assume required not
> optional) rather than "Optional Properties"?

Right, the whole "panel" section is kept separately in a hope that CDF
(or DRM or whatever ;-) generic display pipeline bindings will deprecate
it. So the display-timings is required when optional panel properties
are present. Maybe I should split them into a separate sub-node?
Something along these lines (including the bandwidth example):

clcd@1f0000 {
        compatible = "arm,pl111", "arm,primecell";
        
        max-memory-bandwidth = <36864000>; /* bps, 640x480@60 16bpp */
        
        panel {
                type = "tft";
                tft-interface = <8 8 8>;
                display-timings {
                        ...
                }       
        };      
};

Then the panel subnode would be optional, but type and display-timings
inside would be required.

Also, I will have another look at what the CDF is offering, to make it
as future-proof as possible.

Paweł



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Tue, 2013-09-10 at 18:32 +0100, Stephen Warren wrote:
> > +Optional properties:
> > +
> > +- video-ram: phandle to a node describing specialized video memory
> > +		(that is *not* described in the top level "memory" node)
> > +		that must be used as a framebuffer, eg. due to restrictions
> > +		of the system interconnect; the node must contain a
> > +		standard reg property describing the address and the size
> > +		of the memory area
> 
> Should this use the "CMA bindings" that are being proposed at the moment?

I expected this bit to be the hardest to get through :-)

The memory in question is *not* a part of "normal" RAM, therefore CMA
doesn't even know about it. The situation I have to deal with is a
system when CLCD's AHB master can *not* access the normal RAM, so the
designers kindly ;-) provided some static RAM which it can address.

> Even if not, I'm not quite clear on what the referenced node is supposed
> to contain; is just a reg property enough, so you'd see the following at
> a completely arbitrary location in the DT:
> 
> framebuffer-mem {
>     reg = <0x80000000 0x00100000>;
> };

The place wouldn't be random, no. Getting back to my scenario, the
"video" RAM, being close to CLCD, is (obviously) also addressable by the
core, as any other memory-mapped device. So its position is determined
by the platform memory map:

\ {
	#address-cell = <2>;
	#size-cell = <2>;
	static-memory-bus {
		#address-cell = <2>;
		#size-cell = <1>;
		ranges = <2 0 0 0x18000000 64M>;
		motherboard {
			ranges; 
			video-ram {
				reg = <2 0 8MB>;
			};
		};
	};
};

From the core's perspective it's just a bit of extra memory, you could,
for example, put a MTD ram disk on it. It seems to deserve a
representation in the tree then.

> I'm not sure what the benefit of making this a standalone node is; why
> not just put the base/size directly into the video-ram property in the
> CLCD node?

This is certainly an option. I've considered this as well, but thought
that the above made more sense.

Having said that, there is actually a use case, although a very
non-probable one, for having a direct number there... The interconnect
that CLCD is connected to could have different mapping than the
processor's one. In other word, the memory seen by the core at X, could
be accessed by the CLCD at Y. Or, in even more quirky situation, the
core may not have access to the memory at all, with the graphics being
generated only by GPU (or any other third party). Then the value would
mean "the address you have to use when generating AMBA read transactions
to be get some meaningful data" and would have to be known explicitly.

I guess it won't be hard to convince me it's a better option ;-)

> > +- max-framebuffer-size: maximum size in bytes of the framebuffer the
> > +			system can handle, eg. in terms of available
> > +			memory bandwidth
> 
> Size doesn't imply bandwidth, due to the potential for varying bpp,
> frame-rates, margin/porch sizes, etc. If this is a bandwidth limit,
> shouldn't we instead represent that value directly, perhaps along with
> some multiplier to convert theoretical bandwidth to practical bandwidth
> (to account for memory protocol and controller overheads)?

It's a *memory interface* bandwidth, so synchronization fields are
irrelevant here. And the bpp limit is actually being calculated from
this value, not the other way round. But I forgot about differing frame
rates, that's fact. So it probably should be:

- max-memory-bandwidth: maximum bandwidth in bytes per second that the
			cell's memory interface can handle

and can be then used to calculate maximum bpp depending on the selected
mode.

As to the multipliers... Although I hope that the SOC designer can
provide theoretical throughput data, the only practical way of getting
the value I was able to come up with was just trying different
combinations till cross the line, so there isn't much math to be done
further.

> ...
> > +- panel-type: (required) must be "tft" or "stn", defines panel type
> ...
> > +- panel-stn-4bit: (for monochrome "stn" panel) if present means
> > +			that the monochrome display is connected
> > +			via 4 bit-wide interface
> 
> I just wanted to confirm that those are a complete/direct representation
> of all the HW options the module supports?

Yes. TFT or STN. There can be one or STN panels connected. STN(s) can be
color or mono. Mono STN(s) can be driven via 4 or 8 bit wide interfaces.
Disclaimer: I'm not trying to pretend an expert here. I'm just basing
the above on the cell's spec.

> > +- display-timings: standard display timings sub-node, see
> > +			Documentation/devicetree/bindings/video/display-timing.txt
> 
> Should that be in a "Required sub-nodes" section (I assume required not
> optional) rather than "Optional Properties"?

Right, the whole "panel" section is kept separately in a hope that CDF
(or DRM or whatever ;-) generic display pipeline bindings will deprecate
it. So the display-timings is required when optional panel properties
are present. Maybe I should split them into a separate sub-node?
Something along these lines (including the bandwidth example):

clcd@1f0000 {
        compatible = "arm,pl111", "arm,primecell";
        
        max-memory-bandwidth = <36864000>; /* bps, 640x480@60 16bpp */
        
        panel {
                type = "tft";
                tft-interface = <8 8 8>;
                display-timings {
                        ...
                }       
        };      
};

Then the panel subnode would be optional, but type and display-timings
inside would be required.

Also, I will have another look at what the CDF is offering, to make it
as future-proof as possible.

Paweł

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Tue, 2013-09-10 at 18:32 +0100, Stephen Warren wrote:
> > +Optional properties:
> > +
> > +- video-ram: phandle to a node describing specialized video memory
> > +		(that is *not* described in the top level "memory" node)
> > +		that must be used as a framebuffer, eg. due to restrictions
> > +		of the system interconnect; the node must contain a
> > +		standard reg property describing the address and the size
> > +		of the memory area
> 
> Should this use the "CMA bindings" that are being proposed at the moment?

I expected this bit to be the hardest to get through :-)

The memory in question is *not* a part of "normal" RAM, therefore CMA
doesn't even know about it. The situation I have to deal with is a
system when CLCD's AHB master can *not* access the normal RAM, so the
designers kindly ;-) provided some static RAM which it can address.

> Even if not, I'm not quite clear on what the referenced node is supposed
> to contain; is just a reg property enough, so you'd see the following at
> a completely arbitrary location in the DT:
> 
> framebuffer-mem {
>     reg = <0x80000000 0x00100000>;
> };

The place wouldn't be random, no. Getting back to my scenario, the
"video" RAM, being close to CLCD, is (obviously) also addressable by the
core, as any other memory-mapped device. So its position is determined
by the platform memory map:

\ {
	#address-cell = <2>;
	#size-cell = <2>;
	static-memory-bus {
		#address-cell = <2>;
		#size-cell = <1>;
		ranges = <2 0 0 0x18000000 64M>;
		motherboard {
			ranges; 
			video-ram {
				reg = <2 0 8MB>;
			};
		};
	};
};

>From the core's perspective it's just a bit of extra memory, you could,
for example, put a MTD ram disk on it. It seems to deserve a
representation in the tree then.

> I'm not sure what the benefit of making this a standalone node is; why
> not just put the base/size directly into the video-ram property in the
> CLCD node?

This is certainly an option. I've considered this as well, but thought
that the above made more sense.

Having said that, there is actually a use case, although a very
non-probable one, for having a direct number there... The interconnect
that CLCD is connected to could have different mapping than the
processor's one. In other word, the memory seen by the core at X, could
be accessed by the CLCD at Y. Or, in even more quirky situation, the
core may not have access to the memory at all, with the graphics being
generated only by GPU (or any other third party). Then the value would
mean "the address you have to use when generating AMBA read transactions
to be get some meaningful data" and would have to be known explicitly.

I guess it won't be hard to convince me it's a better option ;-)

> > +- max-framebuffer-size: maximum size in bytes of the framebuffer the
> > +			system can handle, eg. in terms of available
> > +			memory bandwidth
> 
> Size doesn't imply bandwidth, due to the potential for varying bpp,
> frame-rates, margin/porch sizes, etc. If this is a bandwidth limit,
> shouldn't we instead represent that value directly, perhaps along with
> some multiplier to convert theoretical bandwidth to practical bandwidth
> (to account for memory protocol and controller overheads)?

It's a *memory interface* bandwidth, so synchronization fields are
irrelevant here. And the bpp limit is actually being calculated from
this value, not the other way round. But I forgot about differing frame
rates, that's fact. So it probably should be:

- max-memory-bandwidth: maximum bandwidth in bytes per second that the
			cell's memory interface can handle

and can be then used to calculate maximum bpp depending on the selected
mode.

As to the multipliers... Although I hope that the SOC designer can
provide theoretical throughput data, the only practical way of getting
the value I was able to come up with was just trying different
combinations till cross the line, so there isn't much math to be done
further.

> ...
> > +- panel-type: (required) must be "tft" or "stn", defines panel type
> ...
> > +- panel-stn-4bit: (for monochrome "stn" panel) if present means
> > +			that the monochrome display is connected
> > +			via 4 bit-wide interface
> 
> I just wanted to confirm that those are a complete/direct representation
> of all the HW options the module supports?

Yes. TFT or STN. There can be one or STN panels connected. STN(s) can be
color or mono. Mono STN(s) can be driven via 4 or 8 bit wide interfaces.
Disclaimer: I'm not trying to pretend an expert here. I'm just basing
the above on the cell's spec.

> > +- display-timings: standard display timings sub-node, see
> > +			Documentation/devicetree/bindings/video/display-timing.txt
> 
> Should that be in a "Required sub-nodes" section (I assume required not
> optional) rather than "Optional Properties"?

Right, the whole "panel" section is kept separately in a hope that CDF
(or DRM or whatever ;-) generic display pipeline bindings will deprecate
it. So the display-timings is required when optional panel properties
are present. Maybe I should split them into a separate sub-node?
Something along these lines (including the bandwidth example):

clcd at 1f0000 {
        compatible = "arm,pl111", "arm,primecell";
        
        max-memory-bandwidth = <36864000>; /* bps, 640x480 at 60 16bpp */
        
        panel {
                type = "tft";
                tft-interface = <8 8 8>;
                display-timings {
                        ...
                }       
        };      
};

Then the panel subnode would be optional, but type and display-timings
inside would be required.

Also, I will have another look at what the CDF is offering, to make it
as future-proof as possible.

Pawe?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Tue, 2013-09-10 at 20:43 +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 10, 2013 at 11:25:25AM +0100, Pawel Moll wrote:
> > +#ifdef CONFIG_OF
> > +static int clcdfb_of_get_tft_panel(struct device_node *node,
> > +		struct clcd_panel *panel)
> > +{
> > +	int err;
> > +	u32 rgb[3];
> > +
> > +	err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Bypass pixel clock divider, data output on the falling edge */
> > +	panel->tim2 = TIM2_BCD | TIM2_IPC;
> > +
> > +	/* TFT display, vert. comp. interrupt at the start of the back porch */
> > +	panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
> > +
> > +	if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4)
> > +		panel->caps |= CLCD_CAP_444;
> > +	if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5)
> > +		panel->caps |= CLCD_CAP_5551;
> > +	if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5)
> > +		panel->caps |= CLCD_CAP_565;
> > +	if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8)
> > +		panel->caps |= CLCD_CAP_888;
> 
> This definitely isn't right.  Why does a panel which has 8 bits of RGB
> get to indicate that it supports everything below it?
> 
> This is not how this hardware works.  Look at table A-7 in the TRM.
> If you wire the CLCD controller directly to a panel which is 8-bit RGB
> only, it can't support any of the other formats.
> 
> The CLCD controller itself can only support on the hardware 8-bits of
> RGB or 5 bits of RGB plus an intensity bit, so that's two formats.

I must admit I didn't checked the PL110 CLD multiplexing scheme, naively
expecting it to be the same as PL111. My bad, thanks for pointing this
out.

The code above works for PL111 - the signals are laid so with full 888
wiring and 444 mode the bottom bits are reserved (see table A.10). In
reality they're zeros, so the colour space is narrowed.

Seems that the code will have to behave differently for PL110 and PL111
here.

> However, things are complicated by ARMs addition of an external mux
> on the CLCD output, which gives us the other format possibilities
> by munging the signals to give appropriate formats in the framebuffer
> memory.  In other words, in RGB444 mode, the mux ends up taking
> red from CLD[4:1], green from CLD[9:7,5], blue from CLD[14:13,11:10].

It's the Versatile AB/PB hacked CLCD, PL110/111 hybrid, right? It
doesn't really fall into the "arm,pl110" and "arm,pl111" compatible
value, so would need to be something like "arm,versatile,clcdc" and
require custom code. I don't think I want to solve this problem right
now.

> This is one of the dangers of directly converting what's in the kernel
> to DT properties without getting the hardware documentation and fully
> understanding that first - remember, DT properties are supposed to be
> based on the hardware, not the Linux implementation.

I don't think I've directly converted what's in the kernel to DT
properties. There is no "caps" property in the binding. There is only
information how is the cell wired up to a panel, and this information is
valid for both PL110 and PL111 as it is. My fault I've believed when was
told that "the two are basically the same." and should have compared the
TRMs in details.

Will fix the code.

Paweł



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Tue, 2013-09-10 at 20:43 +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 10, 2013 at 11:25:25AM +0100, Pawel Moll wrote:
> > +#ifdef CONFIG_OF
> > +static int clcdfb_of_get_tft_panel(struct device_node *node,
> > +		struct clcd_panel *panel)
> > +{
> > +	int err;
> > +	u32 rgb[3];
> > +
> > +	err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Bypass pixel clock divider, data output on the falling edge */
> > +	panel->tim2 = TIM2_BCD | TIM2_IPC;
> > +
> > +	/* TFT display, vert. comp. interrupt at the start of the back porch */
> > +	panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
> > +
> > +	if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4)
> > +		panel->caps |= CLCD_CAP_444;
> > +	if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5)
> > +		panel->caps |= CLCD_CAP_5551;
> > +	if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5)
> > +		panel->caps |= CLCD_CAP_565;
> > +	if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8)
> > +		panel->caps |= CLCD_CAP_888;
> 
> This definitely isn't right.  Why does a panel which has 8 bits of RGB
> get to indicate that it supports everything below it?
> 
> This is not how this hardware works.  Look at table A-7 in the TRM.
> If you wire the CLCD controller directly to a panel which is 8-bit RGB
> only, it can't support any of the other formats.
> 
> The CLCD controller itself can only support on the hardware 8-bits of
> RGB or 5 bits of RGB plus an intensity bit, so that's two formats.

I must admit I didn't checked the PL110 CLD multiplexing scheme, naively
expecting it to be the same as PL111. My bad, thanks for pointing this
out.

The code above works for PL111 - the signals are laid so with full 888
wiring and 444 mode the bottom bits are reserved (see table A.10). In
reality they're zeros, so the colour space is narrowed.

Seems that the code will have to behave differently for PL110 and PL111
here.

> However, things are complicated by ARMs addition of an external mux
> on the CLCD output, which gives us the other format possibilities
> by munging the signals to give appropriate formats in the framebuffer
> memory.  In other words, in RGB444 mode, the mux ends up taking
> red from CLD[4:1], green from CLD[9:7,5], blue from CLD[14:13,11:10].

It's the Versatile AB/PB hacked CLCD, PL110/111 hybrid, right? It
doesn't really fall into the "arm,pl110" and "arm,pl111" compatible
value, so would need to be something like "arm,versatile,clcdc" and
require custom code. I don't think I want to solve this problem right
now.

> This is one of the dangers of directly converting what's in the kernel
> to DT properties without getting the hardware documentation and fully
> understanding that first - remember, DT properties are supposed to be
> based on the hardware, not the Linux implementation.

I don't think I've directly converted what's in the kernel to DT
properties. There is no "caps" property in the binding. There is only
information how is the cell wired up to a panel, and this information is
valid for both PL110 and PL111 as it is. My fault I've believed when was
told that "the two are basically the same." and should have compared the
TRMs in details.

Will fix the code.

Paweł

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Tue, 2013-09-10 at 20:43 +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 10, 2013 at 11:25:25AM +0100, Pawel Moll wrote:
> > +#ifdef CONFIG_OF
> > +static int clcdfb_of_get_tft_panel(struct device_node *node,
> > +		struct clcd_panel *panel)
> > +{
> > +	int err;
> > +	u32 rgb[3];
> > +
> > +	err = of_property_read_u32_array(node, "panel-tft-interface", rgb, 3);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Bypass pixel clock divider, data output on the falling edge */
> > +	panel->tim2 = TIM2_BCD | TIM2_IPC;
> > +
> > +	/* TFT display, vert. comp. interrupt at the start of the back porch */
> > +	panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);
> > +
> > +	if (rgb[0] >= 4 && rgb[1] >= 4 && rgb[2] >= 4)
> > +		panel->caps |= CLCD_CAP_444;
> > +	if (rgb[0] >= 5 && rgb[1] >= 5 && rgb[2] >= 5)
> > +		panel->caps |= CLCD_CAP_5551;
> > +	if (rgb[0] >= 5 && rgb[1] >= 6 && rgb[2] >= 5)
> > +		panel->caps |= CLCD_CAP_565;
> > +	if (rgb[0] >= 8 && rgb[1] >= 8 && rgb[2] >= 8)
> > +		panel->caps |= CLCD_CAP_888;
> 
> This definitely isn't right.  Why does a panel which has 8 bits of RGB
> get to indicate that it supports everything below it?
> 
> This is not how this hardware works.  Look at table A-7 in the TRM.
> If you wire the CLCD controller directly to a panel which is 8-bit RGB
> only, it can't support any of the other formats.
> 
> The CLCD controller itself can only support on the hardware 8-bits of
> RGB or 5 bits of RGB plus an intensity bit, so that's two formats.

I must admit I didn't checked the PL110 CLD multiplexing scheme, naively
expecting it to be the same as PL111. My bad, thanks for pointing this
out.

The code above works for PL111 - the signals are laid so with full 888
wiring and 444 mode the bottom bits are reserved (see table A.10). In
reality they're zeros, so the colour space is narrowed.

Seems that the code will have to behave differently for PL110 and PL111
here.

> However, things are complicated by ARMs addition of an external mux
> on the CLCD output, which gives us the other format possibilities
> by munging the signals to give appropriate formats in the framebuffer
> memory.  In other words, in RGB444 mode, the mux ends up taking
> red from CLD[4:1], green from CLD[9:7,5], blue from CLD[14:13,11:10].

It's the Versatile AB/PB hacked CLCD, PL110/111 hybrid, right? It
doesn't really fall into the "arm,pl110" and "arm,pl111" compatible
value, so would need to be something like "arm,versatile,clcdc" and
require custom code. I don't think I want to solve this problem right
now.

> This is one of the dangers of directly converting what's in the kernel
> to DT properties without getting the hardware documentation and fully
> understanding that first - remember, DT properties are supposed to be
> based on the hardware, not the Linux implementation.

I don't think I've directly converted what's in the kernel to DT
properties. There is no "caps" property in the binding. There is only
information how is the cell wired up to a panel, and this information is
valid for both PL110 and PL111 as it is. My fault I've believed when was
told that "the two are basically the same." and should have compared the
TRMs in details.

Will fix the code.

Pawe?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Stephen Warren]

On 09/11/2013 05:45 AM, Pawel Moll wrote:
> On Tue, 2013-09-10 at 18:32 +0100, Stephen Warren wrote:
>>> +Optional properties:
>>> +
>>> +- video-ram: phandle to a node describing specialized video memory
>>> +		(that is *not* described in the top level "memory" node)
>>> +		that must be used as a framebuffer, eg. due to restrictions
>>> +		of the system interconnect; the node must contain a
>>> +		standard reg property describing the address and the size
>>> +		of the memory area
>>
>> Should this use the "CMA bindings" that are being proposed at the moment?
> 
> I expected this bit to be the hardest to get through :-)
> 
> The memory in question is *not* a part of "normal" RAM, therefore CMA
> doesn't even know about it. The situation I have to deal with is a
> system when CLCD's AHB master can *not* access the normal RAM, so the
> designers kindly ;-) provided some static RAM which it can address.
> 
>> Even if not, I'm not quite clear on what the referenced node is supposed
>> to contain; is just a reg property enough, so you'd see the following at
>> a completely arbitrary location in the DT:
>>
>> framebuffer-mem {
>>     reg = <0x80000000 0x00100000>;
>> };
> 
> The place wouldn't be random, no. Getting back to my scenario, the
> "video" RAM, being close to CLCD, is (obviously) also addressable by the
> core, as any other memory-mapped device. So its position is determined
> by the platform memory map:
> 
> \ {
> 	#address-cell = <2>;
> 	#size-cell = <2>;
> 	static-memory-bus {
> 		#address-cell = <2>;
> 		#size-cell = <1>;
> 		ranges = <2 0 0 0x18000000 64M>;
> 		motherboard {
> 			ranges; 
> 			video-ram {
> 				reg = <2 0 8MB>;
> 			};
> 		};
> 	};
> };
> 
> From the core's perspective it's just a bit of extra memory, you could,
> for example, put a MTD ram disk on it. It seems to deserve a
> representation in the tree then.

Yes, that's a good point. Perhaps it is reasonable to represent the
memory somewhere then.

I don't see why this memory couldn't exist in the regular /memory node;
it is after all (admittedly slow) RAM. Obviously you'd want to cover the
region with a /memreserve/ to avoid it being used just like any other
RAM. Perhaps the CLCD could reference the memreserve then?

Alternatively, if you want to represent the region as a regular node
rather than a /memory entry, I'd expect there to be a binding defining
what that node was, and the node to at least have a compatible value as
well. I'm not sure how you would indicate that the node should be MTD
vs. a resource for CLCD though; perhaps you'd use a different compatible
value depending on the intended use of the memory?

>> I'm not sure what the benefit of making this a standalone node is; why
>> not just put the base/size directly into the video-ram property in the
>> CLCD node?
> 
> This is certainly an option. I've considered this as well, but thought
> that the above made more sense.
> 
> Having said that, there is actually a use case, although a very
> non-probable one, for having a direct number there... The interconnect
> that CLCD is connected to could have different mapping than the
> processor's one. In other word, the memory seen by the core at X, could
> be accessed by the CLCD at Y. Or, in even more quirky situation, the
> core may not have access to the memory at all, with the graphics being
> generated only by GPU (or any other third party). Then the value would
> mean "the address you have to use when generating AMBA read transactions
> to be get some meaningful data" and would have to be known explicitly.
> 
> I guess it won't be hard to convince me it's a better option ;-)

That's true. Everything in DT is currently set up to describe the CPU's
memory map. Either we need to add some more properties to describe the
potentially different memory map for other bus masters, and/or we should
represent the various buses in DT, and any driver doing DMA should ask
each bus's driver in turn to translate the core-visible address to a bus
address so we can eventually end up with the address needed by the bus
masters.

That is indeed complex, and could at least in many situations simple be
short-circuited by putting the relevant base address in each other bus
master's own node/property. Hopefully we wouldn't get bitten by how
non-general that could be.

>>> +- max-framebuffer-size: maximum size in bytes of the framebuffer the
>>> +			system can handle, eg. in terms of available
>>> +			memory bandwidth
>>
>> Size doesn't imply bandwidth, due to the potential for varying bpp,
>> frame-rates, margin/porch sizes, etc. If this is a bandwidth limit,
>> shouldn't we instead represent that value directly, perhaps along with
>> some multiplier to convert theoretical bandwidth to practical bandwidth
>> (to account for memory protocol and controller overheads)?
> 
> It's a *memory interface* bandwidth, so synchronization fields are
> irrelevant here.

For average bandwidth, sure. However, the peak bandwidth is affected; if
your active region is 80% vs 95% of your line length, that's a
difference in the required region during the active period only. Of
course, your HW may start pre-fetching much earlier than the start of
the active region, so there are many variables.

> And the bpp limit is actually being calculated from
> this value, not the other way round. But I forgot about differing frame
> rates, that's fact. So it probably should be:
> 
> - max-memory-bandwidth: maximum bandwidth in bytes per second that the
> 			cell's memory interface can handle
> 
> and can be then used to calculate maximum bpp depending on the selected
> mode.

Yes, that's a better property definition.

> As to the multipliers... Although I hope that the SOC designer can
> provide theoretical throughput data, the only practical way of getting
> the value I was able to come up with was just trying different
> combinations till cross the line, so there isn't much math to be done
> further.

I don't know how constrained of a system CLCD is, but I do know that
mode validation is a very complex process in some real-life graphics
drivers. There are many complex calculations/modelling/heuristics
applied. I guess it would be very difficult to fully parametrize this
through DT; a real/complete solution is going to have to know enormous
detail of the memory system. So it's probably not worth pushing for DT
to represent the information required for this. I suppose in practice,
for a simple solution, the best idea is to revise max-memory-bandwidth
down (internally to the driver to maintain DT ABI I suppose) if any
problems are found in practice with the calculations.

>> ...
>>> +- panel-type: (required) must be "tft" or "stn", defines panel type
>> ...
>>> +- panel-stn-4bit: (for monochrome "stn" panel) if present means
>>> +			that the monochrome display is connected
>>> +			via 4 bit-wide interface
>>
>> I just wanted to confirm that those are a complete/direct representation
>> of all the HW options the module supports?
> 
> Yes. TFT or STN. There can be one or STN panels connected. STN(s) can be
> color or mono. Mono STN(s) can be driven via 4 or 8 bit wide interfaces.
> Disclaimer: I'm not trying to pretend an expert here. I'm just basing
> the above on the cell's spec.
> 
>>> +- display-timings: standard display timings sub-node, see
>>> +			Documentation/devicetree/bindings/video/display-timing.txt
>>
>> Should that be in a "Required sub-nodes" section (I assume required not
>> optional) rather than "Optional Properties"?
> 
> Right, the whole "panel" section is kept separately in a hope that CDF
> (or DRM or whatever ;-) generic display pipeline bindings will deprecate
> it. So the display-timings is required when optional panel properties
> are present. Maybe I should split them into a separate sub-node?
> Something along these lines (including the bandwidth example):

I would assume that TFT-vs-STN is a pretty direct representation of the
HW (IO bus to panel in particular), and hence wouldn't be replaced by
CDF? I would expect CDF to represent the more generic properties. But, I
haven't been following CDF too closely, so perhaps that's not true.

If you're expecting this binding to change if/when CDF appears, perhaps
it'd be better to wait for CDF, or plan for a new compatible property at
that time, or add some property indicating old-style configuration vs
new-style configuration once CDF is supported?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Stephen Warren]

On 09/11/2013 05:45 AM, Pawel Moll wrote:
> On Tue, 2013-09-10 at 18:32 +0100, Stephen Warren wrote:
>>> +Optional properties:
>>> +
>>> +- video-ram: phandle to a node describing specialized video memory
>>> +		(that is *not* described in the top level "memory" node)
>>> +		that must be used as a framebuffer, eg. due to restrictions
>>> +		of the system interconnect; the node must contain a
>>> +		standard reg property describing the address and the size
>>> +		of the memory area
>>
>> Should this use the "CMA bindings" that are being proposed at the moment?
> 
> I expected this bit to be the hardest to get through :-)
> 
> The memory in question is *not* a part of "normal" RAM, therefore CMA
> doesn't even know about it. The situation I have to deal with is a
> system when CLCD's AHB master can *not* access the normal RAM, so the
> designers kindly ;-) provided some static RAM which it can address.
> 
>> Even if not, I'm not quite clear on what the referenced node is supposed
>> to contain; is just a reg property enough, so you'd see the following at
>> a completely arbitrary location in the DT:
>>
>> framebuffer-mem {
>>     reg = <0x80000000 0x00100000>;
>> };
> 
> The place wouldn't be random, no. Getting back to my scenario, the
> "video" RAM, being close to CLCD, is (obviously) also addressable by the
> core, as any other memory-mapped device. So its position is determined
> by the platform memory map:
> 
> \ {
> 	#address-cell = <2>;
> 	#size-cell = <2>;
> 	static-memory-bus {
> 		#address-cell = <2>;
> 		#size-cell = <1>;
> 		ranges = <2 0 0 0x18000000 64M>;
> 		motherboard {
> 			ranges; 
> 			video-ram {
> 				reg = <2 0 8MB>;
> 			};
> 		};
> 	};
> };
> 
> From the core's perspective it's just a bit of extra memory, you could,
> for example, put a MTD ram disk on it. It seems to deserve a
> representation in the tree then.

Yes, that's a good point. Perhaps it is reasonable to represent the
memory somewhere then.

I don't see why this memory couldn't exist in the regular /memory node;
it is after all (admittedly slow) RAM. Obviously you'd want to cover the
region with a /memreserve/ to avoid it being used just like any other
RAM. Perhaps the CLCD could reference the memreserve then?

Alternatively, if you want to represent the region as a regular node
rather than a /memory entry, I'd expect there to be a binding defining
what that node was, and the node to at least have a compatible value as
well. I'm not sure how you would indicate that the node should be MTD
vs. a resource for CLCD though; perhaps you'd use a different compatible
value depending on the intended use of the memory?

>> I'm not sure what the benefit of making this a standalone node is; why
>> not just put the base/size directly into the video-ram property in the
>> CLCD node?
> 
> This is certainly an option. I've considered this as well, but thought
> that the above made more sense.
> 
> Having said that, there is actually a use case, although a very
> non-probable one, for having a direct number there... The interconnect
> that CLCD is connected to could have different mapping than the
> processor's one. In other word, the memory seen by the core at X, could
> be accessed by the CLCD at Y. Or, in even more quirky situation, the
> core may not have access to the memory at all, with the graphics being
> generated only by GPU (or any other third party). Then the value would
> mean "the address you have to use when generating AMBA read transactions
> to be get some meaningful data" and would have to be known explicitly.
> 
> I guess it won't be hard to convince me it's a better option ;-)

That's true. Everything in DT is currently set up to describe the CPU's
memory map. Either we need to add some more properties to describe the
potentially different memory map for other bus masters, and/or we should
represent the various buses in DT, and any driver doing DMA should ask
each bus's driver in turn to translate the core-visible address to a bus
address so we can eventually end up with the address needed by the bus
masters.

That is indeed complex, and could at least in many situations simple be
short-circuited by putting the relevant base address in each other bus
master's own node/property. Hopefully we wouldn't get bitten by how
non-general that could be.

>>> +- max-framebuffer-size: maximum size in bytes of the framebuffer the
>>> +			system can handle, eg. in terms of available
>>> +			memory bandwidth
>>
>> Size doesn't imply bandwidth, due to the potential for varying bpp,
>> frame-rates, margin/porch sizes, etc. If this is a bandwidth limit,
>> shouldn't we instead represent that value directly, perhaps along with
>> some multiplier to convert theoretical bandwidth to practical bandwidth
>> (to account for memory protocol and controller overheads)?
> 
> It's a *memory interface* bandwidth, so synchronization fields are
> irrelevant here.

For average bandwidth, sure. However, the peak bandwidth is affected; if
your active region is 80% vs 95% of your line length, that's a
difference in the required region during the active period only. Of
course, your HW may start pre-fetching much earlier than the start of
the active region, so there are many variables.

> And the bpp limit is actually being calculated from
> this value, not the other way round. But I forgot about differing frame
> rates, that's fact. So it probably should be:
> 
> - max-memory-bandwidth: maximum bandwidth in bytes per second that the
> 			cell's memory interface can handle
> 
> and can be then used to calculate maximum bpp depending on the selected
> mode.

Yes, that's a better property definition.

> As to the multipliers... Although I hope that the SOC designer can
> provide theoretical throughput data, the only practical way of getting
> the value I was able to come up with was just trying different
> combinations till cross the line, so there isn't much math to be done
> further.

I don't know how constrained of a system CLCD is, but I do know that
mode validation is a very complex process in some real-life graphics
drivers. There are many complex calculations/modelling/heuristics
applied. I guess it would be very difficult to fully parametrize this
through DT; a real/complete solution is going to have to know enormous
detail of the memory system. So it's probably not worth pushing for DT
to represent the information required for this. I suppose in practice,
for a simple solution, the best idea is to revise max-memory-bandwidth
down (internally to the driver to maintain DT ABI I suppose) if any
problems are found in practice with the calculations.

>> ...
>>> +- panel-type: (required) must be "tft" or "stn", defines panel type
>> ...
>>> +- panel-stn-4bit: (for monochrome "stn" panel) if present means
>>> +			that the monochrome display is connected
>>> +			via 4 bit-wide interface
>>
>> I just wanted to confirm that those are a complete/direct representation
>> of all the HW options the module supports?
> 
> Yes. TFT or STN. There can be one or STN panels connected. STN(s) can be
> color or mono. Mono STN(s) can be driven via 4 or 8 bit wide interfaces.
> Disclaimer: I'm not trying to pretend an expert here. I'm just basing
> the above on the cell's spec.
> 
>>> +- display-timings: standard display timings sub-node, see
>>> +			Documentation/devicetree/bindings/video/display-timing.txt
>>
>> Should that be in a "Required sub-nodes" section (I assume required not
>> optional) rather than "Optional Properties"?
> 
> Right, the whole "panel" section is kept separately in a hope that CDF
> (or DRM or whatever ;-) generic display pipeline bindings will deprecate
> it. So the display-timings is required when optional panel properties
> are present. Maybe I should split them into a separate sub-node?
> Something along these lines (including the bandwidth example):

I would assume that TFT-vs-STN is a pretty direct representation of the
HW (IO bus to panel in particular), and hence wouldn't be replaced by
CDF? I would expect CDF to represent the more generic properties. But, I
haven't been following CDF too closely, so perhaps that's not true.

If you're expecting this binding to change if/when CDF appears, perhaps
it'd be better to wait for CDF, or plan for a new compatible property at
that time, or add some property indicating old-style configuration vs
new-style configuration once CDF is supported?

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Stephen Warren]

On 09/11/2013 05:45 AM, Pawel Moll wrote:
> On Tue, 2013-09-10 at 18:32 +0100, Stephen Warren wrote:
>>> +Optional properties:
>>> +
>>> +- video-ram: phandle to a node describing specialized video memory
>>> +		(that is *not* described in the top level "memory" node)
>>> +		that must be used as a framebuffer, eg. due to restrictions
>>> +		of the system interconnect; the node must contain a
>>> +		standard reg property describing the address and the size
>>> +		of the memory area
>>
>> Should this use the "CMA bindings" that are being proposed at the moment?
> 
> I expected this bit to be the hardest to get through :-)
> 
> The memory in question is *not* a part of "normal" RAM, therefore CMA
> doesn't even know about it. The situation I have to deal with is a
> system when CLCD's AHB master can *not* access the normal RAM, so the
> designers kindly ;-) provided some static RAM which it can address.
> 
>> Even if not, I'm not quite clear on what the referenced node is supposed
>> to contain; is just a reg property enough, so you'd see the following at
>> a completely arbitrary location in the DT:
>>
>> framebuffer-mem {
>>     reg = <0x80000000 0x00100000>;
>> };
> 
> The place wouldn't be random, no. Getting back to my scenario, the
> "video" RAM, being close to CLCD, is (obviously) also addressable by the
> core, as any other memory-mapped device. So its position is determined
> by the platform memory map:
> 
> \ {
> 	#address-cell = <2>;
> 	#size-cell = <2>;
> 	static-memory-bus {
> 		#address-cell = <2>;
> 		#size-cell = <1>;
> 		ranges = <2 0 0 0x18000000 64M>;
> 		motherboard {
> 			ranges; 
> 			video-ram {
> 				reg = <2 0 8MB>;
> 			};
> 		};
> 	};
> };
> 
> From the core's perspective it's just a bit of extra memory, you could,
> for example, put a MTD ram disk on it. It seems to deserve a
> representation in the tree then.

Yes, that's a good point. Perhaps it is reasonable to represent the
memory somewhere then.

I don't see why this memory couldn't exist in the regular /memory node;
it is after all (admittedly slow) RAM. Obviously you'd want to cover the
region with a /memreserve/ to avoid it being used just like any other
RAM. Perhaps the CLCD could reference the memreserve then?

Alternatively, if you want to represent the region as a regular node
rather than a /memory entry, I'd expect there to be a binding defining
what that node was, and the node to at least have a compatible value as
well. I'm not sure how you would indicate that the node should be MTD
vs. a resource for CLCD though; perhaps you'd use a different compatible
value depending on the intended use of the memory?

>> I'm not sure what the benefit of making this a standalone node is; why
>> not just put the base/size directly into the video-ram property in the
>> CLCD node?
> 
> This is certainly an option. I've considered this as well, but thought
> that the above made more sense.
> 
> Having said that, there is actually a use case, although a very
> non-probable one, for having a direct number there... The interconnect
> that CLCD is connected to could have different mapping than the
> processor's one. In other word, the memory seen by the core at X, could
> be accessed by the CLCD at Y. Or, in even more quirky situation, the
> core may not have access to the memory at all, with the graphics being
> generated only by GPU (or any other third party). Then the value would
> mean "the address you have to use when generating AMBA read transactions
> to be get some meaningful data" and would have to be known explicitly.
> 
> I guess it won't be hard to convince me it's a better option ;-)

That's true. Everything in DT is currently set up to describe the CPU's
memory map. Either we need to add some more properties to describe the
potentially different memory map for other bus masters, and/or we should
represent the various buses in DT, and any driver doing DMA should ask
each bus's driver in turn to translate the core-visible address to a bus
address so we can eventually end up with the address needed by the bus
masters.

That is indeed complex, and could at least in many situations simple be
short-circuited by putting the relevant base address in each other bus
master's own node/property. Hopefully we wouldn't get bitten by how
non-general that could be.

>>> +- max-framebuffer-size: maximum size in bytes of the framebuffer the
>>> +			system can handle, eg. in terms of available
>>> +			memory bandwidth
>>
>> Size doesn't imply bandwidth, due to the potential for varying bpp,
>> frame-rates, margin/porch sizes, etc. If this is a bandwidth limit,
>> shouldn't we instead represent that value directly, perhaps along with
>> some multiplier to convert theoretical bandwidth to practical bandwidth
>> (to account for memory protocol and controller overheads)?
> 
> It's a *memory interface* bandwidth, so synchronization fields are
> irrelevant here.

For average bandwidth, sure. However, the peak bandwidth is affected; if
your active region is 80% vs 95% of your line length, that's a
difference in the required region during the active period only. Of
course, your HW may start pre-fetching much earlier than the start of
the active region, so there are many variables.

> And the bpp limit is actually being calculated from
> this value, not the other way round. But I forgot about differing frame
> rates, that's fact. So it probably should be:
> 
> - max-memory-bandwidth: maximum bandwidth in bytes per second that the
> 			cell's memory interface can handle
> 
> and can be then used to calculate maximum bpp depending on the selected
> mode.

Yes, that's a better property definition.

> As to the multipliers... Although I hope that the SOC designer can
> provide theoretical throughput data, the only practical way of getting
> the value I was able to come up with was just trying different
> combinations till cross the line, so there isn't much math to be done
> further.

I don't know how constrained of a system CLCD is, but I do know that
mode validation is a very complex process in some real-life graphics
drivers. There are many complex calculations/modelling/heuristics
applied. I guess it would be very difficult to fully parametrize this
through DT; a real/complete solution is going to have to know enormous
detail of the memory system. So it's probably not worth pushing for DT
to represent the information required for this. I suppose in practice,
for a simple solution, the best idea is to revise max-memory-bandwidth
down (internally to the driver to maintain DT ABI I suppose) if any
problems are found in practice with the calculations.

>> ...
>>> +- panel-type: (required) must be "tft" or "stn", defines panel type
>> ...
>>> +- panel-stn-4bit: (for monochrome "stn" panel) if present means
>>> +			that the monochrome display is connected
>>> +			via 4 bit-wide interface
>>
>> I just wanted to confirm that those are a complete/direct representation
>> of all the HW options the module supports?
> 
> Yes. TFT or STN. There can be one or STN panels connected. STN(s) can be
> color or mono. Mono STN(s) can be driven via 4 or 8 bit wide interfaces.
> Disclaimer: I'm not trying to pretend an expert here. I'm just basing
> the above on the cell's spec.
> 
>>> +- display-timings: standard display timings sub-node, see
>>> +			Documentation/devicetree/bindings/video/display-timing.txt
>>
>> Should that be in a "Required sub-nodes" section (I assume required not
>> optional) rather than "Optional Properties"?
> 
> Right, the whole "panel" section is kept separately in a hope that CDF
> (or DRM or whatever ;-) generic display pipeline bindings will deprecate
> it. So the display-timings is required when optional panel properties
> are present. Maybe I should split them into a separate sub-node?
> Something along these lines (including the bandwidth example):

I would assume that TFT-vs-STN is a pretty direct representation of the
HW (IO bus to panel in particular), and hence wouldn't be replaced by
CDF? I would expect CDF to represent the more generic properties. But, I
haven't been following CDF too closely, so perhaps that's not true.

If you're expecting this binding to change if/when CDF appears, perhaps
it'd be better to wait for CDF, or plan for a new compatible property at
that time, or add some property indicating old-style configuration vs
new-style configuration once CDF is supported?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Wed, 2013-09-11 at 18:39 +0100, Stephen Warren wrote:
> > From the core's perspective it's just a bit of extra memory, you could,
> > for example, put a MTD ram disk on it. It seems to deserve a
> > representation in the tree then.
> 
> Yes, that's a good point. Perhaps it is reasonable to represent the
> memory somewhere then.
> 
> I don't see why this memory couldn't exist in the regular /memory node;
> it is after all (admittedly slow) RAM. Obviously you'd want to cover the
> region with a /memreserve/ to avoid it being used just like any other
> RAM. 

This would be pretty tricky in my particular case... The sram chip lives
on a motherboard, which lives behind a static memory interface (Chip
Select + offset). And the memreserve can only take "top level" address,
which can be different depending on the test chip (SoC) or FPGA SMM
being used.

> Perhaps the CLCD could reference the memreserve then?

How do you mean reference the memreserve? It's not a node, I don't think you can get
a phandle to it...

> Alternatively, if you want to represent the region as a regular node
> rather than a /memory entry, I'd expect there to be a binding defining
> what that node was, and the node to at least have a compatible value as
> well. I'm not sure how you would indicate that the node should be MTD
> vs. a resource for CLCD though; perhaps you'd use a different compatible
> value depending on the intended use of the memory?

Yes, that's exactly what I have now:

                psram@1,00000000 { 
                        compatible = "arm,vexpress-psram", "mtd-ram";
                        reg = <1 0x00000000 0x02000000>;
                        bank-width = <4>;
                };
                
                vram@2,00000000 {
                        compatible = "arm,vexpress-vram";
                        reg = <2 0x00000000 0x00800000>;
                };

which allows mtd to use the psram.

> >> I'm not sure what the benefit of making this a standalone node is; why
> >> not just put the base/size directly into the video-ram property in the
> >> CLCD node?
> > 
> > This is certainly an option. I've considered this as well, but thought
> > that the above made more sense.
> > 
> > Having said that, there is actually a use case, although a very
> > non-probable one, for having a direct number there... The interconnect
> > that CLCD is connected to could have different mapping than the
> > processor's one. In other word, the memory seen by the core at X, could
> > be accessed by the CLCD at Y. Or, in even more quirky situation, the
> > core may not have access to the memory at all, with the graphics being
> > generated only by GPU (or any other third party). Then the value would
> > mean "the address you have to use when generating AMBA read transactions
> > to be get some meaningful data" and would have to be known explicitly.
> > 
> > I guess it won't be hard to convince me it's a better option ;-)
> 
> That's true. Everything in DT is currently set up to describe the CPU's
> memory map. Either we need to add some more properties to describe the
> potentially different memory map for other bus masters, and/or we should
> represent the various buses in DT, and any driver doing DMA should ask
> each bus's driver in turn to translate the core-visible address to a bus
> address so we can eventually end up with the address needed by the bus
> masters.
> 
> That is indeed complex, and could at least in many situations simple be
> short-circuited by putting the relevant base address in each other bus
> master's own node/property. Hopefully we wouldn't get bitten by how
> non-general that could be.

Ok, so I'll make it a arm,pl11x,specific property. Actually two
properties - one bit I missed was the dual STN panel case, where they
have separate frame buffers. Something along the lines of:

Optional:

- arm,pl11x,framebuffer-base: a pair of two values, address and size,
				defining the framebuffer for the upper
				(or the only one) panel
- arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
					when present

Being very hardware-specific, it covers all possible quirky scenarios,
at the same time does not exclude the option of using the CMA region
phandles, if a particular system needed it.

> > - max-memory-bandwidth: maximum bandwidth in bytes per second that the
> > 			cell's memory interface can handle
> > 
> > and can be then used to calculate maximum bpp depending on the selected
> > mode.
> 
> Yes, that's a better property definition.

Ok, I'll go that way then. Whether derived from the system design
characteristics or by "hands on experiments", seems hardware-y enough.

> >>> +- display-timings: standard display timings sub-node, see
> >>> +			Documentation/devicetree/bindings/video/display-timing.txt
> >>
> >> Should that be in a "Required sub-nodes" section (I assume required not
> >> optional) rather than "Optional Properties"?
> > 
> > Right, the whole "panel" section is kept separately in a hope that CDF
> > (or DRM or whatever ;-) generic display pipeline bindings will deprecate
> > it. So the display-timings is required when optional panel properties
> > are present. Maybe I should split them into a separate sub-node?
> > Something along these lines (including the bandwidth example):
> 
> I would assume that TFT-vs-STN is a pretty direct representation of the
> HW (IO bus to panel in particular), and hence wouldn't be replaced by
> CDF? I would expect CDF to represent the more generic properties. But, I
> haven't been following CDF too closely, so perhaps that's not true.

I'm not expecting CDF *bindings* to carry this information, no. I'm
expecting CDF core to provide me with this data in runtime (here's the
panel; what kind of panel are you? what modes do you provide? etc.) and
this is what my initial experiments used. The CLCD node only had the
video-ram and bandwidth-like properties plus a phandle to the display. 

> If you're expecting this binding to change if/when CDF appears, perhaps
> it'd be better to wait for CDF, or plan for a new compatible property at
> that time, or add some property indicating old-style configuration vs
> new-style configuration once CDF is supported?

I'm expecting the panel description inside the CLCD node to be
deprecated, not changed, by the mythical CDF. As to waiting for it...
well. It's been over a year since first proposal ("generic panel
framework" as was it called then ;-). And over 2 years since VE gained
"DT support except for CLCD, which should be ready soon" ;-) And than 2
or 3 new platforms had to keep auxdata only for CLCD's sake. So, sure -
we can wait :-)

Paweł



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Wed, 2013-09-11 at 18:39 +0100, Stephen Warren wrote:
> > From the core's perspective it's just a bit of extra memory, you could,
> > for example, put a MTD ram disk on it. It seems to deserve a
> > representation in the tree then.
> 
> Yes, that's a good point. Perhaps it is reasonable to represent the
> memory somewhere then.
> 
> I don't see why this memory couldn't exist in the regular /memory node;
> it is after all (admittedly slow) RAM. Obviously you'd want to cover the
> region with a /memreserve/ to avoid it being used just like any other
> RAM. 

This would be pretty tricky in my particular case... The sram chip lives
on a motherboard, which lives behind a static memory interface (Chip
Select + offset). And the memreserve can only take "top level" address,
which can be different depending on the test chip (SoC) or FPGA SMM
being used.

> Perhaps the CLCD could reference the memreserve then?

How do you mean reference the memreserve? It's not a node, I don't think you can get
a phandle to it...

> Alternatively, if you want to represent the region as a regular node
> rather than a /memory entry, I'd expect there to be a binding defining
> what that node was, and the node to at least have a compatible value as
> well. I'm not sure how you would indicate that the node should be MTD
> vs. a resource for CLCD though; perhaps you'd use a different compatible
> value depending on the intended use of the memory?

Yes, that's exactly what I have now:

                psram@1,00000000 { 
                        compatible = "arm,vexpress-psram", "mtd-ram";
                        reg = <1 0x00000000 0x02000000>;
                        bank-width = <4>;
                };
                
                vram@2,00000000 {
                        compatible = "arm,vexpress-vram";
                        reg = <2 0x00000000 0x00800000>;
                };

which allows mtd to use the psram.

> >> I'm not sure what the benefit of making this a standalone node is; why
> >> not just put the base/size directly into the video-ram property in the
> >> CLCD node?
> > 
> > This is certainly an option. I've considered this as well, but thought
> > that the above made more sense.
> > 
> > Having said that, there is actually a use case, although a very
> > non-probable one, for having a direct number there... The interconnect
> > that CLCD is connected to could have different mapping than the
> > processor's one. In other word, the memory seen by the core at X, could
> > be accessed by the CLCD at Y. Or, in even more quirky situation, the
> > core may not have access to the memory at all, with the graphics being
> > generated only by GPU (or any other third party). Then the value would
> > mean "the address you have to use when generating AMBA read transactions
> > to be get some meaningful data" and would have to be known explicitly.
> > 
> > I guess it won't be hard to convince me it's a better option ;-)
> 
> That's true. Everything in DT is currently set up to describe the CPU's
> memory map. Either we need to add some more properties to describe the
> potentially different memory map for other bus masters, and/or we should
> represent the various buses in DT, and any driver doing DMA should ask
> each bus's driver in turn to translate the core-visible address to a bus
> address so we can eventually end up with the address needed by the bus
> masters.
> 
> That is indeed complex, and could at least in many situations simple be
> short-circuited by putting the relevant base address in each other bus
> master's own node/property. Hopefully we wouldn't get bitten by how
> non-general that could be.

Ok, so I'll make it a arm,pl11x,specific property. Actually two
properties - one bit I missed was the dual STN panel case, where they
have separate frame buffers. Something along the lines of:

Optional:

- arm,pl11x,framebuffer-base: a pair of two values, address and size,
				defining the framebuffer for the upper
				(or the only one) panel
- arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
					when present

Being very hardware-specific, it covers all possible quirky scenarios,
at the same time does not exclude the option of using the CMA region
phandles, if a particular system needed it.

> > - max-memory-bandwidth: maximum bandwidth in bytes per second that the
> > 			cell's memory interface can handle
> > 
> > and can be then used to calculate maximum bpp depending on the selected
> > mode.
> 
> Yes, that's a better property definition.

Ok, I'll go that way then. Whether derived from the system design
characteristics or by "hands on experiments", seems hardware-y enough.

> >>> +- display-timings: standard display timings sub-node, see
> >>> +			Documentation/devicetree/bindings/video/display-timing.txt
> >>
> >> Should that be in a "Required sub-nodes" section (I assume required not
> >> optional) rather than "Optional Properties"?
> > 
> > Right, the whole "panel" section is kept separately in a hope that CDF
> > (or DRM or whatever ;-) generic display pipeline bindings will deprecate
> > it. So the display-timings is required when optional panel properties
> > are present. Maybe I should split them into a separate sub-node?
> > Something along these lines (including the bandwidth example):
> 
> I would assume that TFT-vs-STN is a pretty direct representation of the
> HW (IO bus to panel in particular), and hence wouldn't be replaced by
> CDF? I would expect CDF to represent the more generic properties. But, I
> haven't been following CDF too closely, so perhaps that's not true.

I'm not expecting CDF *bindings* to carry this information, no. I'm
expecting CDF core to provide me with this data in runtime (here's the
panel; what kind of panel are you? what modes do you provide? etc.) and
this is what my initial experiments used. The CLCD node only had the
video-ram and bandwidth-like properties plus a phandle to the display. 

> If you're expecting this binding to change if/when CDF appears, perhaps
> it'd be better to wait for CDF, or plan for a new compatible property at
> that time, or add some property indicating old-style configuration vs
> new-style configuration once CDF is supported?

I'm expecting the panel description inside the CLCD node to be
deprecated, not changed, by the mythical CDF. As to waiting for it...
well. It's been over a year since first proposal ("generic panel
framework" as was it called then ;-). And over 2 years since VE gained
"DT support except for CLCD, which should be ready soon" ;-) And than 2
or 3 new platforms had to keep auxdata only for CLCD's sake. So, sure -
we can wait :-)

Paweł

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Wed, 2013-09-11 at 18:39 +0100, Stephen Warren wrote:
> > From the core's perspective it's just a bit of extra memory, you could,
> > for example, put a MTD ram disk on it. It seems to deserve a
> > representation in the tree then.
> 
> Yes, that's a good point. Perhaps it is reasonable to represent the
> memory somewhere then.
> 
> I don't see why this memory couldn't exist in the regular /memory node;
> it is after all (admittedly slow) RAM. Obviously you'd want to cover the
> region with a /memreserve/ to avoid it being used just like any other
> RAM. 

This would be pretty tricky in my particular case... The sram chip lives
on a motherboard, which lives behind a static memory interface (Chip
Select + offset). And the memreserve can only take "top level" address,
which can be different depending on the test chip (SoC) or FPGA SMM
being used.

> Perhaps the CLCD could reference the memreserve then?

How do you mean reference the memreserve? It's not a node, I don't think you can get
a phandle to it...

> Alternatively, if you want to represent the region as a regular node
> rather than a /memory entry, I'd expect there to be a binding defining
> what that node was, and the node to at least have a compatible value as
> well. I'm not sure how you would indicate that the node should be MTD
> vs. a resource for CLCD though; perhaps you'd use a different compatible
> value depending on the intended use of the memory?

Yes, that's exactly what I have now:

                psram at 1,00000000 { 
                        compatible = "arm,vexpress-psram", "mtd-ram";
                        reg = <1 0x00000000 0x02000000>;
                        bank-width = <4>;
                };
                
                vram at 2,00000000 {
                        compatible = "arm,vexpress-vram";
                        reg = <2 0x00000000 0x00800000>;
                };

which allows mtd to use the psram.

> >> I'm not sure what the benefit of making this a standalone node is; why
> >> not just put the base/size directly into the video-ram property in the
> >> CLCD node?
> > 
> > This is certainly an option. I've considered this as well, but thought
> > that the above made more sense.
> > 
> > Having said that, there is actually a use case, although a very
> > non-probable one, for having a direct number there... The interconnect
> > that CLCD is connected to could have different mapping than the
> > processor's one. In other word, the memory seen by the core at X, could
> > be accessed by the CLCD at Y. Or, in even more quirky situation, the
> > core may not have access to the memory at all, with the graphics being
> > generated only by GPU (or any other third party). Then the value would
> > mean "the address you have to use when generating AMBA read transactions
> > to be get some meaningful data" and would have to be known explicitly.
> > 
> > I guess it won't be hard to convince me it's a better option ;-)
> 
> That's true. Everything in DT is currently set up to describe the CPU's
> memory map. Either we need to add some more properties to describe the
> potentially different memory map for other bus masters, and/or we should
> represent the various buses in DT, and any driver doing DMA should ask
> each bus's driver in turn to translate the core-visible address to a bus
> address so we can eventually end up with the address needed by the bus
> masters.
> 
> That is indeed complex, and could at least in many situations simple be
> short-circuited by putting the relevant base address in each other bus
> master's own node/property. Hopefully we wouldn't get bitten by how
> non-general that could be.

Ok, so I'll make it a arm,pl11x,specific property. Actually two
properties - one bit I missed was the dual STN panel case, where they
have separate frame buffers. Something along the lines of:

Optional:

- arm,pl11x,framebuffer-base: a pair of two values, address and size,
				defining the framebuffer for the upper
				(or the only one) panel
- arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
					when present

Being very hardware-specific, it covers all possible quirky scenarios,
at the same time does not exclude the option of using the CMA region
phandles, if a particular system needed it.

> > - max-memory-bandwidth: maximum bandwidth in bytes per second that the
> > 			cell's memory interface can handle
> > 
> > and can be then used to calculate maximum bpp depending on the selected
> > mode.
> 
> Yes, that's a better property definition.

Ok, I'll go that way then. Whether derived from the system design
characteristics or by "hands on experiments", seems hardware-y enough.

> >>> +- display-timings: standard display timings sub-node, see
> >>> +			Documentation/devicetree/bindings/video/display-timing.txt
> >>
> >> Should that be in a "Required sub-nodes" section (I assume required not
> >> optional) rather than "Optional Properties"?
> > 
> > Right, the whole "panel" section is kept separately in a hope that CDF
> > (or DRM or whatever ;-) generic display pipeline bindings will deprecate
> > it. So the display-timings is required when optional panel properties
> > are present. Maybe I should split them into a separate sub-node?
> > Something along these lines (including the bandwidth example):
> 
> I would assume that TFT-vs-STN is a pretty direct representation of the
> HW (IO bus to panel in particular), and hence wouldn't be replaced by
> CDF? I would expect CDF to represent the more generic properties. But, I
> haven't been following CDF too closely, so perhaps that's not true.

I'm not expecting CDF *bindings* to carry this information, no. I'm
expecting CDF core to provide me with this data in runtime (here's the
panel; what kind of panel are you? what modes do you provide? etc.) and
this is what my initial experiments used. The CLCD node only had the
video-ram and bandwidth-like properties plus a phandle to the display. 

> If you're expecting this binding to change if/when CDF appears, perhaps
> it'd be better to wait for CDF, or plan for a new compatible property at
> that time, or add some property indicating old-style configuration vs
> new-style configuration once CDF is supported?

I'm expecting the panel description inside the CLCD node to be
deprecated, not changed, by the mythical CDF. As to waiting for it...
well. It's been over a year since first proposal ("generic panel
framework" as was it called then ;-). And over 2 years since VE gained
"DT support except for CLCD, which should be ready soon" ;-) And than 2
or 3 new platforms had to keep auxdata only for CLCD's sake. So, sure -
we can wait :-)

Pawe?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Stephen Warren]

On 09/12/2013 07:03 AM, Pawel Moll wrote:
> On Wed, 2013-09-11 at 18:39 +0100, Stephen Warren wrote:
...
>> Perhaps the CLCD could reference the memreserve then?
> 
> How do you mean reference the memreserve? It's not a node, I don't think you can get
> a phandle to it...

Perhaps it's an extension that hasn't actually been implemented yet. I
thought I saw some reference to it in some long-past discussions. It's
possible that this feature has been dropped in preference to the "CMA"
bindings for defining reserved memory regions?

...
> Ok, so I'll make it a arm,pl11x,specific property. Actually two
> properties - one bit I missed was the dual STN panel case, where they
> have separate frame buffers. Something along the lines of:
> 
> Optional:
> 
> - arm,pl11x,framebuffer-base: a pair of two values, address and size,
> 				defining the framebuffer for the upper
> 				(or the only one) panel
> - arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
> 					when present
> 
> Being very hardware-specific, it covers all possible quirky scenarios,
> at the same time does not exclude the option of using the CMA region
> phandles, if a particular system needed it.

What do "upper" and "lower" mean in this context? I assume this somehow
refers to placement of the displays on the board. If so, it sounds like
there should be some better terminology; something that's specific to
the CLCD controller itself, rather than the environment it's used in.
Aalthough it does sound like this is in an FPGA and hence the controller
is board-specific, but I don't see why it couldn't be re-used in some
other FPGA-based board with different panel layout. Would "A" and "B" or
"main" and "primary" or "first" and "second" work better?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Stephen Warren]

On 09/12/2013 07:03 AM, Pawel Moll wrote:
> On Wed, 2013-09-11 at 18:39 +0100, Stephen Warren wrote:
...
>> Perhaps the CLCD could reference the memreserve then?
> 
> How do you mean reference the memreserve? It's not a node, I don't think you can get
> a phandle to it...

Perhaps it's an extension that hasn't actually been implemented yet. I
thought I saw some reference to it in some long-past discussions. It's
possible that this feature has been dropped in preference to the "CMA"
bindings for defining reserved memory regions?

...
> Ok, so I'll make it a arm,pl11x,specific property. Actually two
> properties - one bit I missed was the dual STN panel case, where they
> have separate frame buffers. Something along the lines of:
> 
> Optional:
> 
> - arm,pl11x,framebuffer-base: a pair of two values, address and size,
> 				defining the framebuffer for the upper
> 				(or the only one) panel
> - arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
> 					when present
> 
> Being very hardware-specific, it covers all possible quirky scenarios,
> at the same time does not exclude the option of using the CMA region
> phandles, if a particular system needed it.

What do "upper" and "lower" mean in this context? I assume this somehow
refers to placement of the displays on the board. If so, it sounds like
there should be some better terminology; something that's specific to
the CLCD controller itself, rather than the environment it's used in.
Aalthough it does sound like this is in an FPGA and hence the controller
is board-specific, but I don't see why it couldn't be re-used in some
other FPGA-based board with different panel layout. Would "A" and "B" or
"main" and "primary" or "first" and "second" work better?

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Stephen Warren]

On 09/12/2013 07:03 AM, Pawel Moll wrote:
> On Wed, 2013-09-11 at 18:39 +0100, Stephen Warren wrote:
...
>> Perhaps the CLCD could reference the memreserve then?
> 
> How do you mean reference the memreserve? It's not a node, I don't think you can get
> a phandle to it...

Perhaps it's an extension that hasn't actually been implemented yet. I
thought I saw some reference to it in some long-past discussions. It's
possible that this feature has been dropped in preference to the "CMA"
bindings for defining reserved memory regions?

...
> Ok, so I'll make it a arm,pl11x,specific property. Actually two
> properties - one bit I missed was the dual STN panel case, where they
> have separate frame buffers. Something along the lines of:
> 
> Optional:
> 
> - arm,pl11x,framebuffer-base: a pair of two values, address and size,
> 				defining the framebuffer for the upper
> 				(or the only one) panel
> - arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
> 					when present
> 
> Being very hardware-specific, it covers all possible quirky scenarios,
> at the same time does not exclude the option of using the CMA region
> phandles, if a particular system needed it.

What do "upper" and "lower" mean in this context? I assume this somehow
refers to placement of the displays on the board. If so, it sounds like
there should be some better terminology; something that's specific to
the CLCD controller itself, rather than the environment it's used in.
Aalthough it does sound like this is in an FPGA and hence the controller
is board-specific, but I don't see why it couldn't be re-used in some
other FPGA-based board with different panel layout. Would "A" and "B" or
"main" and "primary" or "first" and "second" work better?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Thu, 2013-09-12 at 15:55 +0100, Stephen Warren wrote:
> > Ok, so I'll make it a arm,pl11x,specific property. Actually two
> > properties - one bit I missed was the dual STN panel case, where they
> > have separate frame buffers. Something along the lines of:
> > 
> > Optional:
> > 
> > - arm,pl11x,framebuffer-base: a pair of two values, address and size,
> > 				defining the framebuffer for the upper
> > 				(or the only one) panel
> > - arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
> > 					when present
> > 
> > Being very hardware-specific, it covers all possible quirky scenarios,
> > at the same time does not exclude the option of using the CMA region
> > phandles, if a particular system needed it.
> 
> What do "upper" and "lower" mean in this context? I assume this somehow
> refers to placement of the displays on the board. If so, it sounds like
> there should be some better terminology; something that's specific to
> the CLCD controller itself,

It is very CLCD specific indeed and nothing to do with the board :-)

http://arminfo.emea.arm.com/help/topic/com.arm.doc.ddi0293c/I899134.html

"Upper and Lower Panel Frame Base Address Registers".

>  rather than the environment it's used in.
> Aalthough it does sound like this is in an FPGA and hence the controller
> is board-specific, but I don't see why it couldn't be re-used in some
> other FPGA-based board with different panel layout. Would "A" and "B" or
> "main" and "primary" or "first" and "second" work better?

Funnily enough I typed "secondary-framebuffer-base" first, but than
thought "no, let's match the spec" :-)

Paweł



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Thu, 2013-09-12 at 15:55 +0100, Stephen Warren wrote:
> > Ok, so I'll make it a arm,pl11x,specific property. Actually two
> > properties - one bit I missed was the dual STN panel case, where they
> > have separate frame buffers. Something along the lines of:
> > 
> > Optional:
> > 
> > - arm,pl11x,framebuffer-base: a pair of two values, address and size,
> > 				defining the framebuffer for the upper
> > 				(or the only one) panel
> > - arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
> > 					when present
> > 
> > Being very hardware-specific, it covers all possible quirky scenarios,
> > at the same time does not exclude the option of using the CMA region
> > phandles, if a particular system needed it.
> 
> What do "upper" and "lower" mean in this context? I assume this somehow
> refers to placement of the displays on the board. If so, it sounds like
> there should be some better terminology; something that's specific to
> the CLCD controller itself,

It is very CLCD specific indeed and nothing to do with the board :-)

http://arminfo.emea.arm.com/help/topic/com.arm.doc.ddi0293c/I899134.html

"Upper and Lower Panel Frame Base Address Registers".

>  rather than the environment it's used in.
> Aalthough it does sound like this is in an FPGA and hence the controller
> is board-specific, but I don't see why it couldn't be re-used in some
> other FPGA-based board with different panel layout. Would "A" and "B" or
> "main" and "primary" or "first" and "second" work better?

Funnily enough I typed "secondary-framebuffer-base" first, but than
thought "no, let's match the spec" :-)

Paweł

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Thu, 2013-09-12 at 15:55 +0100, Stephen Warren wrote:
> > Ok, so I'll make it a arm,pl11x,specific property. Actually two
> > properties - one bit I missed was the dual STN panel case, where they
> > have separate frame buffers. Something along the lines of:
> > 
> > Optional:
> > 
> > - arm,pl11x,framebuffer-base: a pair of two values, address and size,
> > 				defining the framebuffer for the upper
> > 				(or the only one) panel
> > - arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
> > 					when present
> > 
> > Being very hardware-specific, it covers all possible quirky scenarios,
> > at the same time does not exclude the option of using the CMA region
> > phandles, if a particular system needed it.
> 
> What do "upper" and "lower" mean in this context? I assume this somehow
> refers to placement of the displays on the board. If so, it sounds like
> there should be some better terminology; something that's specific to
> the CLCD controller itself,

It is very CLCD specific indeed and nothing to do with the board :-)

http://arminfo.emea.arm.com/help/topic/com.arm.doc.ddi0293c/I899134.html

"Upper and Lower Panel Frame Base Address Registers".

>  rather than the environment it's used in.
> Aalthough it does sound like this is in an FPGA and hence the controller
> is board-specific, but I don't see why it couldn't be re-used in some
> other FPGA-based board with different panel layout. Would "A" and "B" or
> "main" and "primary" or "first" and "second" work better?

Funnily enough I typed "secondary-framebuffer-base" first, but than
thought "no, let's match the spec" :-)

Pawe?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Thu, 2013-09-12 at 16:26 +0100, Pawel Moll wrote:
> http://arminfo.emea.arm.com/help/topic/com.arm.doc.ddi0293c/I899134.html

Sorry, should be:

http://infocenter.arm.com/help/topic/com.arm.doc.ddi0161e/I899134.html

Paweł





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Thu, 2013-09-12 at 16:26 +0100, Pawel Moll wrote:
> http://arminfo.emea.arm.com/help/topic/com.arm.doc.ddi0293c/I899134.html

Sorry, should be:

http://infocenter.arm.com/help/topic/com.arm.doc.ddi0161e/I899134.html

Paweł

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Pawel Moll]

On Thu, 2013-09-12 at 16:26 +0100, Pawel Moll wrote:
> http://arminfo.emea.arm.com/help/topic/com.arm.doc.ddi0293c/I899134.html

Sorry, should be:

http://infocenter.arm.com/help/topic/com.arm.doc.ddi0161e/I899134.html

Pawe?

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Russell King - ARM Linux]

On Wed, Sep 11, 2013 at 11:39:55AM -0600, Stephen Warren wrote:
> I don't know how constrained of a system CLCD is, but I do know that
> mode validation is a very complex process in some real-life graphics
> drivers.

Apart from maximum memory bus bandwidth, probably maximum output
bandwidth, maximum resolution (determined by what will fit in the
registers and RAM) there aren't that much constraints.  The hardware
block is quite simple in that regard.

More the problem is to deal with two situations:
1. you have a particular panel connected to it which requires a certain
   fixed timing regime.
2. you have the CLCD connected to a VGA or HDMI connector where the
   timing is dependent on the connected display.

The former would be the subject of some kind of *common* DT
representation of the timing requirements of the connected panel.
For the latter, DT needs to specify how the EDID data is retrieved,
or if there is no mechanism for that, being able to provide a set
of allowable timing parameters (such as min/max vsync, min/max
hsync, max dotclock - in other words, the data which used to be
provided to X11 in the past.)

None of that is specific to CLCD though: it's the same problem as
the SA11x0 LCD controller or any other scanned video controller.

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Russell King - ARM Linux]

On Wed, Sep 11, 2013 at 11:39:55AM -0600, Stephen Warren wrote:
> I don't know how constrained of a system CLCD is, but I do know that
> mode validation is a very complex process in some real-life graphics
> drivers.

Apart from maximum memory bus bandwidth, probably maximum output
bandwidth, maximum resolution (determined by what will fit in the
registers and RAM) there aren't that much constraints.  The hardware
block is quite simple in that regard.

More the problem is to deal with two situations:
1. you have a particular panel connected to it which requires a certain
   fixed timing regime.
2. you have the CLCD connected to a VGA or HDMI connector where the
   timing is dependent on the connected display.

The former would be the subject of some kind of *common* DT
representation of the timing requirements of the connected panel.
For the latter, DT needs to specify how the EDID data is retrieved,
or if there is no mechanism for that, being able to provide a set
of allowable timing parameters (such as min/max vsync, min/max
hsync, max dotclock - in other words, the data which used to be
provided to X11 in the past.)

None of that is specific to CLCD though: it's the same problem as
the SA11x0 LCD controller or any other scanned video controller.

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Russell King - ARM Linux]

On Wed, Sep 11, 2013 at 11:39:55AM -0600, Stephen Warren wrote:
> I don't know how constrained of a system CLCD is, but I do know that
> mode validation is a very complex process in some real-life graphics
> drivers.

Apart from maximum memory bus bandwidth, probably maximum output
bandwidth, maximum resolution (determined by what will fit in the
registers and RAM) there aren't that much constraints.  The hardware
block is quite simple in that regard.

More the problem is to deal with two situations:
1. you have a particular panel connected to it which requires a certain
   fixed timing regime.
2. you have the CLCD connected to a VGA or HDMI connector where the
   timing is dependent on the connected display.

The former would be the subject of some kind of *common* DT
representation of the timing requirements of the connected panel.
For the latter, DT needs to specify how the EDID data is retrieved,
or if there is no mechanism for that, being able to provide a set
of allowable timing parameters (such as min/max vsync, min/max
hsync, max dotclock - in other words, the data which used to be
provided to X11 in the past.)

None of that is specific to CLCD though: it's the same problem as
the SA11x0 LCD controller or any other scanned video controller.

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Russell King - ARM Linux]

On Thu, Sep 12, 2013 at 08:55:54AM -0600, Stephen Warren wrote:
> On 09/12/2013 07:03 AM, Pawel Moll wrote:
> > Ok, so I'll make it a arm,pl11x,specific property. Actually two
> > properties - one bit I missed was the dual STN panel case, where they
> > have separate frame buffers. Something along the lines of:
> > 
> > Optional:
> > 
> > - arm,pl11x,framebuffer-base: a pair of two values, address and size,
> > 				defining the framebuffer for the upper
> > 				(or the only one) panel
> > - arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
> > 					when present
> > 
> > Being very hardware-specific, it covers all possible quirky scenarios,
> > at the same time does not exclude the option of using the CMA region
> > phandles, if a particular system needed it.
> 
> What do "upper" and "lower" mean in this context? I assume this somehow
> refers to placement of the displays on the board.

CLCD is only one controller - it can only produce one image from one
framebuffer.

What this refers to is a requirement for dual STN displays - rather
than starting at one corner and working logically to the opposite
corner, these start at one corner and half way through the display at
the same time.  So you scan out the upper half of the image at the
same time that you scan out the lower half of the image.

In order to give an image on the display where the conventional formula
works:

	address = start + y * bytes_per_line + x * bits_per_pixel / 8

this means that the "upper" and "lower" start addresses are related to
the parameters of the display - you want the lower half to start at
the address where y = panel_y_res / 2 and x = 0.

You may also wish to change the start address (if you have enough RAM)
to support page flipping - where you switch between two framebuffers -
one which is currently being displayed while the other is being updated
by the CPU.  You would use this to provide smooth video playback for
example.

So, DT has no business directly encoding this information - it should
be calculated from the information provided by the panel and the
timings required, and all that the CLCD driver should know is that
"we have this RAM which starts here, and extends for N bytes" - or
to use system memory instead.

Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support

[Russell King - ARM Linux]

On Thu, Sep 12, 2013 at 08:55:54AM -0600, Stephen Warren wrote:
> On 09/12/2013 07:03 AM, Pawel Moll wrote:
> > Ok, so I'll make it a arm,pl11x,specific property. Actually two
> > properties - one bit I missed was the dual STN panel case, where they
> > have separate frame buffers. Something along the lines of:
> > 
> > Optional:
> > 
> > - arm,pl11x,framebuffer-base: a pair of two values, address and size,
> > 				defining the framebuffer for the upper
> > 				(or the only one) panel
> > - arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
> > 					when present
> > 
> > Being very hardware-specific, it covers all possible quirky scenarios,
> > at the same time does not exclude the option of using the CMA region
> > phandles, if a particular system needed it.
> 
> What do "upper" and "lower" mean in this context? I assume this somehow
> refers to placement of the displays on the board.

CLCD is only one controller - it can only produce one image from one
framebuffer.

What this refers to is a requirement for dual STN displays - rather
than starting at one corner and working logically to the opposite
corner, these start at one corner and half way through the display at
the same time.  So you scan out the upper half of the image at the
same time that you scan out the lower half of the image.

In order to give an image on the display where the conventional formula
works:

	address = start + y * bytes_per_line + x * bits_per_pixel / 8

this means that the "upper" and "lower" start addresses are related to
the parameters of the display - you want the lower half to start at
the address where y = panel_y_res / 2 and x = 0.

You may also wish to change the start address (if you have enough RAM)
to support page flipping - where you switch between two framebuffers -
one which is currently being displayed while the other is being updated
by the CPU.  You would use this to provide smooth video playback for
example.

So, DT has no business directly encoding this information - it should
be calculated from the information provided by the panel and the
timings required, and all that the CLCD driver should know is that
"we have this RAM which starts here, and extends for N bytes" - or
to use system memory instead.

[PATCH v2 1/2] video: ARM CLCD: Add DT support

[Russell King - ARM Linux]

On Thu, Sep 12, 2013 at 08:55:54AM -0600, Stephen Warren wrote:
> On 09/12/2013 07:03 AM, Pawel Moll wrote:
> > Ok, so I'll make it a arm,pl11x,specific property. Actually two
> > properties - one bit I missed was the dual STN panel case, where they
> > have separate frame buffers. Something along the lines of:
> > 
> > Optional:
> > 
> > - arm,pl11x,framebuffer-base: a pair of two values, address and size,
> > 				defining the framebuffer for the upper
> > 				(or the only one) panel
> > - arm,pl11x,lower-framebuffer-base: as above, for the lower STN panel,
> > 					when present
> > 
> > Being very hardware-specific, it covers all possible quirky scenarios,
> > at the same time does not exclude the option of using the CMA region
> > phandles, if a particular system needed it.
> 
> What do "upper" and "lower" mean in this context? I assume this somehow
> refers to placement of the displays on the board.

CLCD is only one controller - it can only produce one image from one
framebuffer.

What this refers to is a requirement for dual STN displays - rather
than starting at one corner and working logically to the opposite
corner, these start at one corner and half way through the display at
the same time.  So you scan out the upper half of the image at the
same time that you scan out the lower half of the image.

In order to give an image on the display where the conventional formula
works:

	address = start + y * bytes_per_line + x * bits_per_pixel / 8

this means that the "upper" and "lower" start addresses are related to
the parameters of the display - you want the lower half to start at
the address where y = panel_y_res / 2 and x = 0.

You may also wish to change the start address (if you have enough RAM)
to support page flipping - where you switch between two framebuffers -
one which is currently being displayed while the other is being updated
by the CPU.  You would use this to provide smooth video playback for
example.

So, DT has no business directly encoding this information - it should
be calculated from the information provided by the panel and the
timings required, and all that the CLCD driver should know is that
"we have this RAM which starts here, and extends for N bytes" - or
to use system memory instead.