* Re: [BUG] simplefb not showing any output
From: Stephen Warren @ 2013-09-10 2:34 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <CAG-2HqXhLcRX-oi4JchD=m7tZpDYyePKhWvkFU1ZZPEuqJ2JqQ@mail.gmail.com>
On 09/07/2013 06:25 AM, David Herrmann wrote:
> On Sat, Sep 7, 2013 at 1:47 PM, Tom Gundersen <teg@jklm.no> wrote:
...
>> A related question: is it expected that simplefb should be
>> significantly slower than efifb, or is that something worth looking
>> into? My boot with simplefb is roughly five seconds slower than with
>> efifb. Coincidentally, I notice the same (or similar slowdown) with
>> inteldrmfb when I see the oops (but not otherwise).
>
> That is probably related to the missing write-combine tag in ioremap.
> Stephen, any objections to this attached patch?
> Tom, if this solves the speed-issues, I will send it out once I get home.
>
> Thanks
> David
>
> (Patch also attached in case of new-lines issues)
>
> From dbfb8e12166d494cd60823cbe84134d5d1a73ec8 Mon Sep 17 00:00:00 2001
> From: David Herrmann <dh.herrmann@gmail.com>
> Date: Sat, 7 Sep 2013 14:22:01 +0200
> Subject: [PATCH] devm/simplefb: introduce and use devm_ioremap_wc()
>
> We want to use devm_ioremap_nocache() or even devm_ioremap_wc() to speed
> up fbdev writes _a lot_. As devm_ioremap_wc() doesn't exist, yet,
> introduce it along the way. Note that ioremap_wc() is aliases to
> ioremap_nocache() in asm-generic/{io,iomem}.h so we can safely expect all
> architectures to either provide it or use the same alias.
OK, that works fine on the Raspberry Pi too, so,
Tested-by: Stephen Warren <swarren@wwwdotorg.org>
^ permalink raw reply
* [PATCH v2 1/2] video: ARM CLCD: Add DT support
From: Pawel Moll @ 2013-09-10 10:25 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related
* [PATCH v2 2/2] ARM: vexpress: Add CLCD Device Tree properties
From: Pawel Moll @ 2013-09-10 10:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1378808726-32535-1-git-send-email-pawel.moll@arm.com>
... 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
^ permalink raw reply related
* [PATCH 3/3] fbdev/ps3fb: fix section mismatch warning for ps3fb_probe
From: Vladimir Murzin @ 2013-09-10 16:46 UTC (permalink / raw)
To: linux-fbdev, cbe-oss-dev, linuxppc-dev
Cc: geoff, Vladimir Murzin, tomi.valkeinen, plagnioj
While cross-building for PPC64 I've got
WARNING: drivers/video/built-in.o(.text+0x9f9ca): Section mismatch in
reference from the function .ps3fb_probe() to th e variable
.init.data:ps3fb_fix The function .ps3fb_probe() references the
variable __initdata ps3fb_fix. This is often because .ps3fb_probe
lacks a __initdata annotation or the annotation of ps3fb_fix is wrong.
WARNING: drivers/video/built-in.o(.text+0x9f9d2): Section mismatch in
reference from the function .ps3fb_probe() to the variable
.init.data:ps3fb_fix The function .ps3fb_probe() references the
variable __initdata ps3fb_fix. This is often because .ps3fb_probe
lacks a __initdata annotation or the annotation of ps3fb_fix is wrong.
WARNING: drivers/built-in.o(.text+0xe222a): Section mismatch in
reference from the function .ps3fb_probe() to the variable
.init.data:ps3fb_fix The function .ps3fb_probe() references the
variable __initdata ps3fb_fix. This is often because .ps3fb_probe
lacks a __initdata annotation or the annotation of ps3fb_fix is wrong.
WARNING: drivers/built-in.o(.text+0xe2232): Section mismatch in
reference from the function .ps3fb_probe() to the variable
.init.data:ps3fb_fix The function .ps3fb_probe() references the
variable __initdata ps3fb_fix. This is often because .ps3fb_probe
lacks a __initdata annotation or the annotation of ps3fb_fix is wrong.
WARNING: vmlinux.o(.text+0x561d4a): Section mismatch in reference from
the function .ps3fb_probe() to the variable .init.data:ps3fb_fix The
function .ps3fb_probe() references the variable __initdata ps3fb_fix.
This is often because .ps3fb_probe lacks a __initdata annotation or
the annotation of ps3fb_fix is wrong.
Mismatch was introduced with 48c68c4f "Drivers: video: remove __dev*
attributes."
Remove __init data annotation from ps3fb_fix.
Signed-off-by: Vladimir Murzin <murzin.v@gmail.com>
---
drivers/video/ps3fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/ps3fb.c b/drivers/video/ps3fb.c
index dbfe2c1..b269abd 100644
--- a/drivers/video/ps3fb.c
+++ b/drivers/video/ps3fb.c
@@ -952,7 +952,7 @@ static struct fb_ops ps3fb_ops = {
.fb_compat_ioctl = ps3fb_ioctl
};
-static struct fb_fix_screeninfo ps3fb_fix __initdata = {
+static struct fb_fix_screeninfo ps3fb_fix = {
.id = DEVICE_NAME,
.type = FB_TYPE_PACKED_PIXELS,
.visual = FB_VISUAL_TRUECOLOR,
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH 3/3] fbdev/ps3fb: fix section mismatch warning for ps3fb_probe
From: Geert Uytterhoeven @ 2013-09-10 16:56 UTC (permalink / raw)
To: Vladimir Murzin
Cc: cbe-oss-dev@lists.ozlabs.org, Linux Fbdev development list,
Geoff Levand, linuxppc-dev@lists.ozlabs.org, Tomi Valkeinen,
Jean-Christophe PLAGNIOL-VILLARD
In-Reply-To: <1378831590-2349-1-git-send-email-murzin.v@gmail.com>
On Tue, Sep 10, 2013 at 6:46 PM, Vladimir Murzin <murzin.v@gmail.com> wrote:
> diff --git a/drivers/video/ps3fb.c b/drivers/video/ps3fb.c
> index dbfe2c1..b269abd 100644
> --- a/drivers/video/ps3fb.c
> +++ b/drivers/video/ps3fb.c
> @@ -952,7 +952,7 @@ static struct fb_ops ps3fb_ops = {
> .fb_compat_ioctl = ps3fb_ioctl
> };
>
> -static struct fb_fix_screeninfo ps3fb_fix __initdata = {
> +static struct fb_fix_screeninfo ps3fb_fix = {
> .id = DEVICE_NAME,
> .type = FB_TYPE_PACKED_PIXELS,
> .visual = FB_VISUAL_TRUECOLOR,
Fixed before by Geoff, but never applied:
http://marc.info/?l=linux-fbdev&m\x136914132618389&w=3
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 3/3] fbdev/ps3fb: fix section mismatch warning for ps3fb_probe
From: Vladimir Murzin @ 2013-09-10 17:05 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: cbe-oss-dev@lists.ozlabs.org, Linux Fbdev development list,
Geoff Levand, linuxppc-dev@lists.ozlabs.org, Tomi Valkeinen,
Jean-Christophe PLAGNIOL-VILLARD
In-Reply-To: <CAMuHMdWHHsOm8qREeVVPBxcqJoa85xfgd-9jzqCnMs=Z9yVFCw@mail.gmail.com>
On Tue, Sep 10, 2013 at 06:56:33PM +0200, Geert Uytterhoeven wrote:
> On Tue, Sep 10, 2013 at 6:46 PM, Vladimir Murzin <murzin.v@gmail.com> wrote:
> > diff --git a/drivers/video/ps3fb.c b/drivers/video/ps3fb.c
> > index dbfe2c1..b269abd 100644
> > --- a/drivers/video/ps3fb.c
> > +++ b/drivers/video/ps3fb.c
> > @@ -952,7 +952,7 @@ static struct fb_ops ps3fb_ops = {
> > .fb_compat_ioctl = ps3fb_ioctl
> > };
> >
> > -static struct fb_fix_screeninfo ps3fb_fix __initdata = {
> > +static struct fb_fix_screeninfo ps3fb_fix = {
> > .id = DEVICE_NAME,
> > .type = FB_TYPE_PACKED_PIXELS,
> > .visual = FB_VISUAL_TRUECOLOR,
>
> Fixed before by Geoff, but never applied:
> http://marc.info/?l=linux-fbdev&m\x136914132618389&w=3
Great! Hope some day it will ;)
Sorry for the noise
Vladimir
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] pwm-backlight: add support for device tree gpio control
From: Thierry Reding @ 2013-09-10 17:21 UTC (permalink / raw)
To: Mike Dunn
Cc: Richard Purdie, Jingoo Han, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Grant Likely, Rob Herring, linux-pwm, linux-fbdev,
linux-kernel, devicetree, Robert Jarzmik, Marek Vasut
In-Reply-To: <1378236372-15711-1-git-send-email-mikedunn@newsguy.com>
[-- Attachment #1: Type: text/plain, Size: 3987 bytes --]
On Tue, Sep 03, 2013 at 12:26:12PM -0700, Mike Dunn wrote:
> This patch adds support for controlling an arbitrary number of gpios to the
> pwm-backlight driver. This was left as a TODO when initial device tree support
> was added by Thierry a while back. This functionality replaces the callbacks
> that are passed in the platform data for non-DT cases. Users can avail
> themselves of this feature by adding a 'gpios' property to the 'backlight' node.
> When the update_status() callback in backlight_ops runs, the gpios listed in the
> property are asserted/deasserted if the specified brightness is non-zero/zero.
>
> Tested on a pxa270-based Palm Treo 680.
>
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
>
> Thanks for looking!
>
> .../bindings/video/backlight/pwm-backlight.txt | 4 +
> drivers/video/backlight/pwm_bl.c | 128 ++++++++++++++++++---
> 2 files changed, 113 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> index 1e4fc72..4583e68 100644
> --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> @@ -14,6 +14,9 @@ Required properties:
> Optional properties:
> - pwm-names: a list of names for the PWM devices specified in the
> "pwms" property (see PWM binding[0])
> + - gpios: An arbitrary number of gpios that must be asserted when the
> + backlight is on, and de-asserted when off. They will be asserted
> + in the order they appear, and de-asserted in reverse order.
Do you have a real setup that actually needs multiple GPIOs? Usually
such a setup requires some kind of timing or other additional constraint
which can't be represented by this simple binding.
Looking at the Palm Treo code it seems like the reason why multiple
GPIOs are needed is because one is to enable the backlight, while the
other is in fact used to enable the LCD panel. I have been working on a
patch series to provide simple panel support, specifically to allow the
separation of backlight and panel power sequencing. Would such a method
work for your use-case as well? The work that I'm doing is somewhat DRM
centric, and I don't think there's a DRM driver for PXA, but perhaps it
would be a good occasion to look at converting the PXA display drivers
to DRM... =)
That said, I've had a patch similar to yours in a local tree (in fact in
the same branch as the panel work I've been doing) for quite some time,
which allows a single "enable" GPIO to be specified. I haven't gotten
around to sending it out yet, but I'll do that shortly. The patch is a
little more involved because it exposes the GPIO via platform data as
well and therefore has to update a number of board files, too. While
going over the various board files I found only a single board which can
actually make use of the new functionality (which I also converted in
the patch). Any other cases couldn't be implemented by the simple change
so I suspect they can't either using your proposed binding.
I've been trying for a while to come up with a way to support more use-
cases, and I keep coming back to the same solution. Since the DT binding
for power sequences was shot down some time ago, we probably have to
represent any kind of sequencing in C code. So instead of trying to fit
everything into a single binding, I think a more maintainable solution
would be to create separate drivers for the more complex use-cases. That
could either be done by using separate compatible values within the
pwm-backlight driver or via completely different drivers. In the latter
case we should probably think about exporting some of the pwm-backlight
functionality so that drivers can easily reuse some of the code.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support
From: Stephen Warren @ 2013-09-10 17:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1378808726-32535-1-git-send-email-pawel.moll@arm.com>
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"?
^ permalink raw reply
* Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support
From: Russell King - ARM Linux @ 2013-09-10 19:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1378808726-32535-1-git-send-email-pawel.moll@arm.com>
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.
^ permalink raw reply
* Re: [PATCH 1/2] video: ARM CLCD: Add DT support
From: Sylwester Nawrocki @ 2013-09-10 21:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1378725559.4082.28.camel@hornet>
On 09/09/2013 01:19 PM, Pawel Moll wrote:
> On Sat, 2013-09-07 at 23:55 +0100, Sylwester Nawrocki wrote:
>> H --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>>> @@ -0,0 +1,87 @@
>>> +* ARM PrimeCell Color LCD Controller (CLCD) PL110/PL111
>>
>> nit: Shouldn't the abbreviation be CLCDC ?
>
> The commonly used acronym for this cell is CLCD. For what its worth, I
> can make the line read:
>
> ARM PrimeCell Color LCD (CLCD) Controller PL110/PL111
OK, that's probably better. A meaningless detail, I've just seen mostly
CLCDC used in the PL100/PL111 TRM and LCDC would presumably be more
intuitive
for this type of IP block.
>>> +- 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
>>
>> It seems the "specialized video memory" is described by some vendor specific
>> DT binding ? Is it documented ? It sounds like you are unnecessarily
>> repeating the memory node details here.
>
> I appreciate this property being the hardest to swallow, but the problem
> it is trying to solve is quite simple, really. System can be designed in
> such a way that CLCD is *not* able to read data from the main memory. In
> such case there will be a separate block of (usually static) memory
> "next" to CLCD, which *must* be used for the framebuffer. And I've got
> two choices here: to simply define an address and size, or to define a
> phandle to a node with standard reg property. I'll be really grateful
> for ideas how to describe the situation better.
I though about reusing the binding only, the part defining reserved
(carve out) memory regions.
>> Perhaps this binding/driver should use the common reserved memory bindings,
>> see thread http://www.spinics.net/lists/devicetree/msg02880.html
>
> No, not really - as I said, this is *not* the main RAM of the system.
> CMA doesn't even know about its existence.
I'm really not an expert in this area, I'll assume we don't list such
separate memory chips in the 'memory' node.
But if such memory could be used not only by this single IP block it would
probably make sense to have it listed in memory/reserved-memory, so it
could
be used by other devices of which drivers possibly wouldn't have to contain
all the detailed dt parsing/memory handling code.
>>> +- 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
>>
>> Are this vendor specific or common properties ? Shouldn't those be prefixed
>> with the vendor name ?
>
> I have no answer to this question. My guts are telling me - nope. TFT is
> TFT, STN is STN, nothing to do with "arm,". But I welcome other
> opinions.
I thought about documenting such a common properties in a common place.
It's
not immediately clear from names of these properties that they apply to
PL110/
PL111 devices only. If we let such generic names being redefined across DT
bindings of different devices it is going to be pretty messy IMHO. Same
property in two different dts files would potentially have different
meaning.
So perhaps instead of panel-dimensions we could define common
properties, e.g.
- display-phys-width: physical horizontal dimension of a display in
millimetres
(micrometres ?);
- display-phys-height: physical vertical dimension of a display in
millimetres
(micrometres ?);
Instead of 'panel-stn-color' a boolean property 'monochrome-display',
the default
when this property was missing would be "colour display".
I'd like to leave defining such common properties to someone having more
experience
in the display area. I don't think it would take much time come up with
generic
names for that couple properties you need. Then CDF implementation
would simply
use whatever gets agreed.
>> Anyway I think we need an RFC to possibly standardize properties that are
>> common across multiple panels and have them listed in a common DT binding.
>>
>> It sounds a bit disappointing that for same class devices there are being
>> introduced custom DT properties for each available device. For instance,
>> the first 3 properties above look like they could apply to various display
>> panels and controllers.
>
> No argument from here. The Common Display Framework was supposed to
> address this issue. And the first version of this patch used it:
>
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/63417
>
> But you'll notice that it's been almost 10 months since the last version
> of CDF saw the daylight, and Laurent seems to be too busy with other
> duties to carry on with this. Additionally the KMS/DRM guys don't look
> too kindly on it. The bottom line is: there are 3 different platform
> suffering from lack of DT bindings for CLCD and I've got fed up with
> waiting. Anyway, I've intentionally kept the panel properties in a
> separate section and wrote "can be described", to keep an open way for
> future "display bindings", if and when they appear.
I might risk people start yelling at me, but perhaps you could explicitly
mark this binding as "preliminary", so it is clear that the intention was
to switch to whatever common design in future.
The recently posted v3 of the CDF uses DT bindings specified at:
Documentation/devicetree/bindings/media/video-interfaces.txt,
Documentation/devicetree/bindings/video/display-timing.txt.
Theoretically, the bindings are independent of any OS implementation, thus
I guess if you used the above bindings, perhaps extended with couple more
properties, it might have been possible to switch to any common OS API in
future, without changing the bindings. That all might be highly theoretical
though. :)
--
Thanks,
Sylwester
^ permalink raw reply
* [PATCH] video/ps3fb: Fix section mismatch warning
From: Geoff Levand @ 2013-09-10 21:59 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1369141302.3652.14.camel@clam>
Remove the __initdata attribute from the ps3fb_fix variable. This is in
follow up to the removal of the __devinit attribute on the ps3fb_probe()
routine in commit 48c68c4f1b542444f175a9e136febcecf3e704d8 (Drivers:
video: remove __dev* attributes).
Fixes build warnings like these:
WARNING: vmlinux.o Section mismatch in reference from the function .ps3fb_probe() to the variable .init.data:ps3fb_fix
The function .ps3fb_probe() references the variable __initdata ps3fb_fix.
This is often because .ps3fb_probe lacks a __initdata annotation or the annotation of ps3fb_fix is wrong.
Signed-off-by: Geoff Levand <geoff@infradead.org>
---
This is a re-send. Could we please get this applied.
-Geoff
drivers/video/ps3fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/ps3fb.c b/drivers/video/ps3fb.c
index a397271d..4819cdf 100644
--- a/drivers/video/ps3fb.c
+++ b/drivers/video/ps3fb.c
@@ -952,7 +952,7 @@ static struct fb_ops ps3fb_ops = {
.fb_compat_ioctl = ps3fb_ioctl
};
-static struct fb_fix_screeninfo ps3fb_fix __initdata = {
+static struct fb_fix_screeninfo ps3fb_fix = {
.id = DEVICE_NAME,
.type = FB_TYPE_PACKED_PIXELS,
.visual = FB_VISUAL_TRUECOLOR,
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH] video/ps3fb: Fix section mismatch warning
From: Jingoo Han @ 2013-09-11 0:51 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1369141302.3652.14.camel@clam>
On Wednesday, September 11, 2013 7:00 AM, Geoff Levand wrote:
>
> Remove the __initdata attribute from the ps3fb_fix variable. This is in
> follow up to the removal of the __devinit attribute on the ps3fb_probe()
> routine in commit 48c68c4f1b542444f175a9e136febcecf3e704d8 (Drivers:
> video: remove __dev* attributes).
>
> Fixes build warnings like these:
>
> WARNING: vmlinux.o Section mismatch in reference from the function .ps3fb_probe() to the
> variable .init.data:ps3fb_fix
> The function .ps3fb_probe() references the variable __initdata ps3fb_fix.
> This is often because .ps3fb_probe lacks a __initdata annotation or the annotation of ps3fb_fix is
> wrong.
>
> Signed-off-by: Geoff Levand <geoff@infradead.org>
Reviewed-by: Jingoo Han <jg1.han@samsung.com>
CC'ed Jean-Christophe Plagniol-Villard, Tomi Valkeinen (Framebuffer maintainer)
Currently, the maintainers of FRAMEBUFFER LAYER are
Jean-Christophe Plagniol-Villard, and Tomi Valkeinen.
So, I CC'ed them.
Best regards,
Jingoo Han
> ---
>
> This is a re-send. Could we please get this applied.
>
> -Geoff
>
> drivers/video/ps3fb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/ps3fb.c b/drivers/video/ps3fb.c
> index a397271d..4819cdf 100644
> --- a/drivers/video/ps3fb.c
> +++ b/drivers/video/ps3fb.c
> @@ -952,7 +952,7 @@ static struct fb_ops ps3fb_ops = {
> .fb_compat_ioctl = ps3fb_ioctl
> };
>
> -static struct fb_fix_screeninfo ps3fb_fix __initdata = {
> +static struct fb_fix_screeninfo ps3fb_fix = {
> .id = DEVICE_NAME,
> .type = FB_TYPE_PACKED_PIXELS,
> .visual = FB_VISUAL_TRUECOLOR,
> --
> 1.8.1.2
>
^ permalink raw reply
* Re: [PATCH/RFC v3 06/19] video: display: OF support
From: Laurent Pinchart @ 2013-09-11 11:33 UTC (permalink / raw)
To: Philipp Zabel
Cc: Laurent Pinchart, dri-devel, linux-fbdev, linux-media,
Guennadi Liakhovetski, Hans Verkuil, sylvester.nawrocki,
sakari.ailus
In-Reply-To: <1378304498.5721.42.camel@pizza.hi.pengutronix.de>
Hi Philipp,
On Wednesday 04 September 2013 16:21:38 Philipp Zabel wrote:
> Am Samstag, den 10.08.2013, 01:03 +0200 schrieb Laurent Pinchart:
> > Extend the notifier with DT node matching support, and add helper
> > functions to build the notifier and link entities based on a graph
> > representation in DT.
> >
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >
> > drivers/video/display/display-core.c | 334 ++++++++++++++++++++++++++
> > drivers/video/display/display-notifier.c | 187 +++++++++++++++++
> > include/video/display.h | 45 +++++
> > 3 files changed, 566 insertions(+)
> >
> > diff --git a/drivers/video/display/display-core.c
> > b/drivers/video/display/display-core.c index c3b47d3..328ead7 100644
> > --- a/drivers/video/display/display-core.c
> > +++ b/drivers/video/display/display-core.c
>
> [...]
>
> > @@ -420,6 +599,161 @@ int display_entity_link_graph(struct device *dev,
> > struct list_head *entities)>
> > }
> > EXPORT_SYMBOL_GPL(display_entity_link_graph);
> >
> > +#ifdef CONFIG_OF
> > +
> > +static int display_of_entity_link_entity(struct device *dev,
> > + struct display_entity *entity,
> > + struct list_head *entities,
> > + struct display_entity *root)
> > +{
> > + u32 link_flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED;
> > + const struct device_node *node = entity->dev->of_node;
>
> the current device tree matching implementation only allows one display
> entity per linux device. How about adding an of_node pointer to struct
> display_entity directly and allow multiple display entity nodes below in a
> single device node in the device tree?
That's a very good point. We had a similar issues in V4L2, with sensors that
would create several entities. However, in those cases, the sensors would be
connected to the rest of the pipeline through a single entity :
Sensor Entity 1 -> ... -> Sensor Entity N -> V4L2 pipeline ...
The core code thus had to care about a single sensor entity when building the
pipeline. We could solve the problem in a similar way for panels, but encoders
need a more elaborate solution.
I see (at least) two possibilities here, either explicitly describing all
entities that make the device in DT (as you have proposed below), or creating
a hierarchy of entities, with parent entities that can contain several child
entities. I've CC'ed Guennadi, Hans, Sylwester and Sakari to get their opinion
on the matter.
> lvds-encoder {
> channel@0 {
If I understand this correctly, your LVDS encoder has two independent
channels. In the general case a device made of multiple entities might have
those entities chained, so "channel" might not be the best term. "entity"
might be a better choice.
> port@0 {
> lvds0_input: endpoint {
> };
> };
> port@1 {
> lvds0_output: endpoint {
> };
> };
> };
> channel@1 {
> port@0 {
> lvds1_input: endpoint {
> };
> };
> lvds1: port@1 {
> lvds1_output: endpoint {
> };
> };
> };
> };
>
> > + struct media_entity *local = &entity->entity;
> > + struct device_node *ep = NULL;
> > + int ret = 0;
> > +
> > + dev_dbg(dev, "creating links for entity %s\n", local->name);
> > +
> > + while (1) {
> > + struct media_entity *remote = NULL;
> > + struct media_pad *remote_pad;
> > + struct media_pad *local_pad;
> > + struct display_of_link link;
> > + struct display_entity *ent;
> > + struct device_node *next;
> > +
> > + /* Get the next endpoint and parse its link. */
> > + next = display_of_get_next_endpoint(node, ep);
> > + if (next = NULL)
> > + break;
> > +
> > + of_node_put(ep);
> > + ep = next;
> > +
> > + dev_dbg(dev, "processing endpoint %s\n", ep->full_name);
> > +
> > + ret = display_of_parse_link(ep, &link);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to parse link for %s\n",
> > + ep->full_name);
> > + continue;
> > + }
> > +
> > + /* Skip source pads, they will be processed from the other end of
> > + * the link.
> > + */
> > + if (link.local_port >= local->num_pads) {
> > + dev_err(dev, "invalid port number %u on %s\n",
> > + link.local_port, link.local_node->full_name);
> > + display_of_put_link(&link);
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + local_pad = &local->pads[link.local_port];
> > +
> > + if (local_pad->flags & MEDIA_PAD_FL_SOURCE) {
> > + dev_dbg(dev, "skipping source port %s:%u\n",
> > + link.local_node->full_name, link.local_port);
> > + display_of_put_link(&link);
> > + continue;
> > + }
> > +
> > + /* Find the remote entity. If not found, just skip the link as
> > + * it goes out of scope of the entities handled by the notifier.
> > + */
> > + list_for_each_entry(ent, entities, list) {
> > + if (ent->dev->of_node = link.remote_node) {
> > + remote = &ent->entity;
> > + break;
> > + }
> > + }
> > +
> > + if (root->dev->of_node = link.remote_node)
> > + remote = &root->entity;
> > +
> > + if (remote = NULL) {
> > + dev_dbg(dev, "no entity found for %s\n",
> > + link.remote_node->full_name);
> > + display_of_put_link(&link);
> > + continue;
> > + }
> > +
> > + if (link.remote_port >= remote->num_pads) {
> > + dev_err(dev, "invalid port number %u on %s\n",
> > + link.remote_port, link.remote_node->full_name);
> > + display_of_put_link(&link);
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + remote_pad = &remote->pads[link.remote_port];
> > +
> > + display_of_put_link(&link);
> > +
> > + /* Create the media link. */
> > + dev_dbg(dev, "creating %s:%u -> %s:%u link\n",
> > + remote->name, remote_pad->index,
> > + local->name, local_pad->index);
> > +
> > + ret = media_entity_create_link(remote, remote_pad->index,
> > + local, local_pad->index,
> > + link_flags);
> > + if (ret < 0) {
> > + dev_err(dev,
> > + "failed to create %s:%u -> %s:%u link\n",
> > + remote->name, remote_pad->index,
> > + local->name, local_pad->index);
> > + break;
> > + }
> > + }
> > +
> > + of_node_put(ep);
> > + return ret;
> > +}
>
> [...]
>
> For example like this:
>
> diff --git a/drivers/video/display/display-core.c
> b/drivers/video/display/display-core.c index 7910c23..a04feed 100644
> --- a/drivers/video/display/display-core.c
> +++ b/drivers/video/display/display-core.c
> @@ -302,6 +302,9 @@ int display_entity_init(struct display_entity *entity,
> unsigned int num_sinks, kref_init(&entity->ref);
> entity->state = DISPLAY_ENTITY_STATE_OFF;
>
> + if (!entity->of_node && entity->dev)
> + entity->of_node = entity->dev->of_node;
> +
> num_pads = num_sinks + num_sources;
> pads = kzalloc(sizeof(*pads) * num_pads, GFP_KERNEL);
> if (pads = NULL)
> @@ -665,7 +668,7 @@ static int display_of_entity_link_entity(struct device
> *dev, struct display_entity *root)
> {
> u32 link_flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED;
> - const struct device_node *node = entity->dev->of_node;
> + const struct device_node *node = entity->of_node;
> struct media_entity *local = &entity->entity;
> struct device_node *ep = NULL;
> int num_sink, ret = 0;
> @@ -727,13 +730,13 @@ static int display_of_entity_link_entity(struct device
> *dev, * it goes out of scope of the entities handled by the notifier. */
> list_for_each_entry(ent, entities, list) {
> - if (ent->dev->of_node = link.remote_node) {
> + if (ent->of_node = link.remote_node) {
> remote = &ent->entity;
> break;
> }
> }
>
> - if (root && root->dev->of_node = link.remote_node)
> + if (root && root->of_node = link.remote_node)
> remote = &root->entity;
>
> if (remote = NULL) {
> diff --git a/drivers/video/display/display-notifier.c
> b/drivers/video/display/display-notifier.c index a3998c7..d0da6e5 100644
> --- a/drivers/video/display/display-notifier.c
> +++ b/drivers/video/display/display-notifier.c
> @@ -28,28 +28,30 @@ static DEFINE_MUTEX(display_entity_mutex);
> * Notifiers
> */
>
> -static bool match_platform(struct device *dev,
> +static bool match_platform(struct display_entity *entity,
> struct display_entity_match *match)
> {
> pr_debug("%s: matching device '%s' with name '%s'\n", __func__,
> - dev_name(dev), match->match.platform.name);
> + dev_name(entity->dev), match->match.platform.name);
>
> - return !strcmp(match->match.platform.name, dev_name(dev));
> + return !strcmp(match->match.platform.name, dev_name(entity->dev));
> }
>
> -static bool match_dt(struct device *dev, struct display_entity_match
> *match) +static bool match_dt(struct display_entity *entity,
> + struct display_entity_match *match)
> {
> pr_debug("%s: matching device node '%s' with node '%s'\n", __func__,
> - dev->of_node->full_name, match->match.dt.node->full_name);
> + entity->of_node->full_name, match->match.dt.node->full_name);
>
> - return match->match.dt.node = dev->of_node;
> + return match->match.dt.node = entity->of_node;
> }
>
> static struct display_entity_match *
> display_entity_notifier_match(struct display_entity_notifier *notifier,
> struct display_entity *entity)
> {
> - bool (*match_func)(struct device *, struct display_entity_match *);
> + bool (*match_func)(struct display_entity *,
> + struct display_entity_match *);
> struct display_entity_match *match;
>
> pr_debug("%s: matching entity '%s' (ptr 0x%p dev '%s')\n", __func__,
> @@ -66,7 +68,7 @@ display_entity_notifier_match(struct
> display_entity_notifier *notifier, break;
> }
>
> - if (match_func(entity->dev, match))
> + if (match_func(entity, match))
> return match;
> }
>
> diff --git a/include/video/display.h b/include/video/display.h
> index 4c402bee..d1f8833 100644
> --- a/include/video/display.h
> +++ b/include/video/display.h
> @@ -228,6 +228,7 @@ struct display_entity {
> struct list_head list;
> struct device *dev;
> struct module *owner;
> + struct device_node *of_node;
> struct kref ref;
>
> char name[32];
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] pwm-backlight: add support for device tree gpio control
From: Mike Dunn @ 2013-09-11 11:40 UTC (permalink / raw)
To: Thierry Reding
Cc: Richard Purdie, Jingoo Han, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Grant Likely, Rob Herring, linux-pwm, linux-fbdev,
linux-kernel, devicetree, Robert Jarzmik, Marek Vasut
In-Reply-To: <20130910172130.GC22111@ulmo>
On 09/10/2013 10:21 AM, Thierry Reding wrote:
> On Tue, Sep 03, 2013 at 12:26:12PM -0700, Mike Dunn wrote:
>> This patch adds support for controlling an arbitrary number of gpios to the
>> pwm-backlight driver. This was left as a TODO when initial device tree support
>> was added by Thierry a while back. This functionality replaces the callbacks
>> that are passed in the platform data for non-DT cases. Users can avail
>> themselves of this feature by adding a 'gpios' property to the 'backlight' node.
>> When the update_status() callback in backlight_ops runs, the gpios listed in the
>> property are asserted/deasserted if the specified brightness is non-zero/zero.
>>
>> Tested on a pxa270-based Palm Treo 680.
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>> ---
>>
>> Thanks for looking!
>>
>> .../bindings/video/backlight/pwm-backlight.txt | 4 +
>> drivers/video/backlight/pwm_bl.c | 128 ++++++++++++++++++---
>> 2 files changed, 113 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>> index 1e4fc72..4583e68 100644
>> --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>> +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>> @@ -14,6 +14,9 @@ Required properties:
>> Optional properties:
>> - pwm-names: a list of names for the PWM devices specified in the
>> "pwms" property (see PWM binding[0])
>> + - gpios: An arbitrary number of gpios that must be asserted when the
>> + backlight is on, and de-asserted when off. They will be asserted
>> + in the order they appear, and de-asserted in reverse order.
>
> Do you have a real setup that actually needs multiple GPIOs? Usually
> such a setup requires some kind of timing or other additional constraint
> which can't be represented by this simple binding.
>
> Looking at the Palm Treo code it seems like the reason why multiple
> GPIOs are needed is because one is to enable the backlight, while the
> other is in fact used to enable the LCD panel.
There are actually four GPIOs involved! (There is an embarrasingly horrible
hack in arch/arm/mach-pxa/palmtreo.c that handles the others.) One is almost
certainly simply backlight power. The other three are probably LCD related.
Timing doesn't seem to be an issue, but the order is important. This is
reverse-engineered without any schematics, and honestly, I'm still guessing with
regard to how the board is wired. It probably is not appropriate to manage all
four gpios in the backlight driver, but at least one needs to be, so I thought
I'd go ahead and prepare a patch that adds gpio support. From what you are
telling me, I guess my approach was too simplistic.
> I have been working on a
> patch series to provide simple panel support, specifically to allow the
> separation of backlight and panel power sequencing. Would such a method
> work for your use-case as well?
Sounds like it would, from what I know.
> The work that I'm doing is somewhat DRM
> centric, and I don't think there's a DRM driver for PXA, but perhaps it
> would be a good occasion to look at converting the PXA display drivers
> to DRM... =)
OK, thanks. I'll look into it. I'm clueless about DRM... All I can say at
this point is that I'm not using X windows, and there is not really any hardware
acceleration in the pxa's lcd controller.
>
> That said, I've had a patch similar to yours in a local tree (in fact in
> the same branch as the panel work I've been doing) for quite some time,
> which allows a single "enable" GPIO to be specified. I haven't gotten
> around to sending it out yet, but I'll do that shortly. The patch is a
> little more involved because it exposes the GPIO via platform data as
> well and therefore has to update a number of board files, too. While
> going over the various board files I found only a single board which can
> actually make use of the new functionality (which I also converted in
> the patch). Any other cases couldn't be implemented by the simple change
> so I suspect they can't either using your proposed binding.
Really? Are we talking about the callbacks in struct
platform_pwm_backlight_data? I thought the majority of them just
requested/released a gpio in init()/exit(), and wiggled it in notify(). I must
be missing something.
>
> I've been trying for a while to come up with a way to support more use-
> cases, and I keep coming back to the same solution. Since the DT binding
> for power sequences was shot down some time ago, we probably have to
> represent any kind of sequencing in C code. So instead of trying to fit
> everything into a single binding, I think a more maintainable solution
> would be to create separate drivers for the more complex use-cases. That
> could either be done by using separate compatible values within the
> pwm-backlight driver or via completely different drivers. In the latter
> case we should probably think about exporting some of the pwm-backlight
> functionality so that drivers can easily reuse some of the code.
I guess I'll wait for your patch and for everything else to shake out... I may
be able to help out if you want to delegate some things. Thanks for the reviews
and the info. Any other pointers appreciated. BTW, I'm working on a simple
patch to the pwm-backlight driver that will fix my inverted pwm output problem.
Thanks again,
Mike
^ permalink raw reply
* Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support
From: Pawel Moll @ 2013-09-11 11:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <522F57B9.5000001@wwwdotorg.org>
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ł
^ permalink raw reply
* Re: [PATCH/RFC v3 06/19] video: display: OF support
From: Philipp Zabel @ 2013-09-11 13:14 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Laurent Pinchart, dri-devel, linux-fbdev, linux-media,
Guennadi Liakhovetski, Hans Verkuil, sylvester.nawrocki,
sakari.ailus
In-Reply-To: <2263372.8nCBHctlWT@avalon>
Am Mittwoch, den 11.09.2013, 13:33 +0200 schrieb Laurent Pinchart:
> Hi Philipp,
>
> On Wednesday 04 September 2013 16:21:38 Philipp Zabel wrote:
> > Am Samstag, den 10.08.2013, 01:03 +0200 schrieb Laurent Pinchart:
> > > Extend the notifier with DT node matching support, and add helper
> > > functions to build the notifier and link entities based on a graph
> > > representation in DT.
> > >
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > >
> > > drivers/video/display/display-core.c | 334 ++++++++++++++++++++++++++
> > > drivers/video/display/display-notifier.c | 187 +++++++++++++++++
> > > include/video/display.h | 45 +++++
> > > 3 files changed, 566 insertions(+)
> > >
> > > diff --git a/drivers/video/display/display-core.c
> > > b/drivers/video/display/display-core.c index c3b47d3..328ead7 100644
> > > --- a/drivers/video/display/display-core.c
> > > +++ b/drivers/video/display/display-core.c
> >
> > [...]
> >
> > > @@ -420,6 +599,161 @@ int display_entity_link_graph(struct device *dev,
> > > struct list_head *entities)>
> > > }
> > > EXPORT_SYMBOL_GPL(display_entity_link_graph);
> > >
> > > +#ifdef CONFIG_OF
> > > +
> > > +static int display_of_entity_link_entity(struct device *dev,
> > > + struct display_entity *entity,
> > > + struct list_head *entities,
> > > + struct display_entity *root)
> > > +{
> > > + u32 link_flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED;
> > > + const struct device_node *node = entity->dev->of_node;
> >
> > the current device tree matching implementation only allows one display
> > entity per linux device. How about adding an of_node pointer to struct
> > display_entity directly and allow multiple display entity nodes below in a
> > single device node in the device tree?
>
> That's a very good point. We had a similar issues in V4L2, with sensors that
> would create several entities. However, in those cases, the sensors would be
> connected to the rest of the pipeline through a single entity :
>
> Sensor Entity 1 -> ... -> Sensor Entity N -> V4L2 pipeline ...
>
> The core code thus had to care about a single sensor entity when building the
> pipeline. We could solve the problem in a similar way for panels, but encoders
> need a more elaborate solution.
>
> I see (at least) two possibilities here, either explicitly describing all
> entities that make the device in DT (as you have proposed below), or creating
> a hierarchy of entities, with parent entities that can contain several child
> entities. I've CC'ed Guennadi, Hans, Sylwester and Sakari to get their opinion
> on the matter.
When you say hierarchy of entities, I imagine something like GStreamer
bins? I suspect hierarchically encapsulated entities would complicate
the pipeline/graph traversal code quite a bit, although it would
probably help to organise the graph and reduce the amount of boilerplate
needed in the device tree.
> > lvds-encoder {
> > channel@0 {
>
> If I understand this correctly, your LVDS encoder has two independent
> channels.
In this example, yes. In reality the i.MX LDB has a mux in each path, so
both inputs can be routed to both outputs. With an entity hierarchy this
could be described as a single entity with two inputs and two outputs,
containing two multiplexer entites and two encoder entities.
LDB entity with two input pads, four internal entities, and two output
pads:
,----------------------------------------.
|-----. LDB ,------. ,------. ,-----|
--| pad |--------| mux0 |--| enc0 |--| pad |--
| 0 |--. ,--| | | | | 2 |
|-----´ \/ `------´ `------´ `-----|
|-----. /\ ,------. ,------. .-----|
--| pad |--´ `--| mux1 | | enc1 |--| pad |--
| 1 |--------| |--| | | 3 |
|-----´ `------´ `------´ `-----|
`----------------------------------------´
(In guess the mux and enc entities could each be combined into one)
> In the general case a device made of multiple entities might have
> those entities chained, so "channel" might not be the best term.
> "entity" might be a better choice.
On the other hand, when describing subdevice entities in the device
tree, maybe the generic type of entity (sensor, scaler, encoder, mux,
etc.) would be useful information?
Another module where I'd like to describe the (outward facing) contained
entities in the device tree is the i.MX Image Processing Unit, which has
two capture interfaces and two display interfaces (all parallel). Those
can't be combined into a single entity because there are other internal
entities connected to them, and because the capture interfaces are v4l2
subdevices, whereas the display interfaces will be display entites.
Alternatively, this could also be described as a single entity
containing an internal structure.
IPU entity with two input pads, two internal capture entities (csi), two
display entities (di), and two output pads:
,----------------------------------------.
|-----. ,------. IPU ,------. ,-----|
--| pad |--| csi0 | | di0 |--| pad |--
| 0 | | |... | | | 2 |
|-----´ `------´ `------´ `-----|
|-----. ,------. ,------. .-----|
--| pad |--| csi1 | | di1 |--| pad |--
| 1 | | |... | | | 3 |
|-----´ `------´ `------´ `-----|
`----------------------------------------´
regards
Philipp
^ permalink raw reply
* Re: [PATCH 1/2] video: ARM CLCD: Add DT support
From: Pawel Moll @ 2013-09-11 13:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <522F91F8.6010207@gmail.com>
On Tue, 2013-09-10 at 22:41 +0100, Sylwester Nawrocki wrote:
> On 09/09/2013 01:19 PM, Pawel Moll wrote:
> > On Sat, 2013-09-07 at 23:55 +0100, Sylwester Nawrocki wrote:
> >> H --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> >>> @@ -0,0 +1,87 @@
> >>> +* ARM PrimeCell Color LCD Controller (CLCD) PL110/PL111
> >>
> >> nit: Shouldn't the abbreviation be CLCDC ?
> >
> > The commonly used acronym for this cell is CLCD. For what its worth, I
> > can make the line read:
> >
> > ARM PrimeCell Color LCD (CLCD) Controller PL110/PL111
>
> OK, that's probably better. A meaningless detail, I've just seen mostly
> CLCDC used in the PL100/PL111 TRM and LCDC would presumably be more
> intuitive
> for this type of IP block.
I think I'll completely remove the acronym. No intuition will be
necessary then :-)
> >>> +- 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
> >>
> >> It seems the "specialized video memory" is described by some vendor specific
> >> DT binding ? Is it documented ? It sounds like you are unnecessarily
> >> repeating the memory node details here.
> >
> > I appreciate this property being the hardest to swallow, but the problem
> > it is trying to solve is quite simple, really. System can be designed in
> > such a way that CLCD is *not* able to read data from the main memory. In
> > such case there will be a separate block of (usually static) memory
> > "next" to CLCD, which *must* be used for the framebuffer. And I've got
> > two choices here: to simply define an address and size, or to define a
> > phandle to a node with standard reg property. I'll be really grateful
> > for ideas how to describe the situation better.
>
> I though about reusing the binding only, the part defining reserved
> (carve out) memory regions.
Em, what exactly do you mean? Referring to the definition of "reserved
memory region" as an example how to use reg = <(address) (size)>?
(because, I don't think it can be "linux,contiguous-memory-region",
"reserved-memory-region"). Can do, however I'm not sure if it won't
cause even more confusion, as I hope that the CMA bindings will be
useful here on their own, to allocate "normal" contiguous framebuffers.
Keep in mind those two use cases are very very different.
> >> Perhaps this binding/driver should use the common reserved memory bindings,
> >> see thread http://www.spinics.net/lists/devicetree/msg02880.html
> >
> > No, not really - as I said, this is *not* the main RAM of the system.
> > CMA doesn't even know about its existence.
>
> I'm really not an expert in this area, I'll assume we don't list such
> separate memory chips in the 'memory' node.
ePAPR spec, regarding the memory node, states: "The client program may
access memory not covered by any memory reservations (see section 8.3)
using any storage attributes it chooses." (note that the reservations
mentioned are "the other" reservations, /memreserve/ ones ;-)
Now, I wouldn't want my "client program" (read: Linux kernel) to use the
"video memory" in question for general allocations, if only because of
its rather poor performance (resulting from the interconnect design, not
the memory chip characteristics), so I treat it more as a memory mapped
device which happens to have a lot of word-long registers...
> But if such memory could be used not only by this single IP block it would
> probably make sense to have it listed in memory/reserved-memory, so it
> could be used by other devices of which drivers possibly wouldn't have to
> contain all the detailed dt parsing/memory handling code.
The "video RAM" on my platform could be used for other purposes. If you
don't mind latencies :-) It's just that the CLCD itself can't use
anything else.
So, if the memory/reserved-memory described areas *not* to be used as
"normal RAM", yes - we could use it. I don't think it's the case now,
though.
Also, Steven mentioned the other option I talked about - just raw
address/size pair. See me deliberation there...
> >>> +- 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
> >>
> >> Are this vendor specific or common properties ? Shouldn't those be prefixed
> >> with the vendor name ?
> >
> > I have no answer to this question. My guts are telling me - nope. TFT is
> > TFT, STN is STN, nothing to do with "arm,". But I welcome other
> > opinions.
>
> I thought about documenting such a common properties in a common place.
> It's
> not immediately clear from names of these properties that they apply to
> PL110/
> PL111 devices only. If we let such generic names being redefined across DT
> bindings of different devices it is going to be pretty messy IMHO. Same
> property in two different dts files would potentially have different
> meaning.
>
> So perhaps instead of panel-dimensions we could define common
> properties, e.g.
>
> - display-phys-width: physical horizontal dimension of a display in
> millimetres
> (micrometres ?);
> - display-phys-height: physical vertical dimension of a display in
> millimetres
> (micrometres ?);
>
> Instead of 'panel-stn-color' a boolean property 'monochrome-display',
> the default
> when this property was missing would be "colour display".
>
> I'd like to leave defining such common properties to someone having more
> experience
> in the display area. I don't think it would take much time come up with
> generic
> names for that couple properties you need. Then CDF implementation
> would simply use whatever gets agreed.
Ok, I see what you're saying. Yes, this could be done. No, I don't claim
to have enough expertise either (micrometers??? :-O ;-) The other thing
is that I don't really expect generic CDF bindings to cover such things.
They will (hopefully) only describe what's connected with what. And the
drivers should know how. Of course they may need the dimensions & alike
in the tree (of course having them standardised would help here), but
it's not a CDF job to provide those.
Anyway, if I decide to split the panel data into a separate sub-node,
I'll look into that.
Paweł
^ permalink raw reply
* Re: [PATCH/RFC v3 06/19] video: display: OF support
From: Hans Verkuil @ 2013-09-11 13:48 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Philipp Zabel, Laurent Pinchart, dri-devel, linux-fbdev,
linux-media, Guennadi Liakhovetski, sylvester.nawrocki,
sakari.ailus
In-Reply-To: <2263372.8nCBHctlWT@avalon>
On 09/11/2013 01:33 PM, Laurent Pinchart wrote:
> Hi Philipp,
>
> On Wednesday 04 September 2013 16:21:38 Philipp Zabel wrote:
>> Am Samstag, den 10.08.2013, 01:03 +0200 schrieb Laurent Pinchart:
>>> Extend the notifier with DT node matching support, and add helper
>>> functions to build the notifier and link entities based on a graph
>>> representation in DT.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>
>>> drivers/video/display/display-core.c | 334 ++++++++++++++++++++++++++
>>> drivers/video/display/display-notifier.c | 187 +++++++++++++++++
>>> include/video/display.h | 45 +++++
>>> 3 files changed, 566 insertions(+)
>>>
>>> diff --git a/drivers/video/display/display-core.c
>>> b/drivers/video/display/display-core.c index c3b47d3..328ead7 100644
>>> --- a/drivers/video/display/display-core.c
>>> +++ b/drivers/video/display/display-core.c
>>
>> [...]
>>
>>> @@ -420,6 +599,161 @@ int display_entity_link_graph(struct device *dev,
>>> struct list_head *entities)>
>>> }
>>> EXPORT_SYMBOL_GPL(display_entity_link_graph);
>>>
>>> +#ifdef CONFIG_OF
>>> +
>>> +static int display_of_entity_link_entity(struct device *dev,
>>> + struct display_entity *entity,
>>> + struct list_head *entities,
>>> + struct display_entity *root)
>>> +{
>>> + u32 link_flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED;
>>> + const struct device_node *node = entity->dev->of_node;
>>
>> the current device tree matching implementation only allows one display
>> entity per linux device. How about adding an of_node pointer to struct
>> display_entity directly and allow multiple display entity nodes below in a
>> single device node in the device tree?
>
> That's a very good point. We had a similar issues in V4L2, with sensors that
> would create several entities. However, in those cases, the sensors would be
> connected to the rest of the pipeline through a single entity :
>
> Sensor Entity 1 -> ... -> Sensor Entity N -> V4L2 pipeline ...
>
> The core code thus had to care about a single sensor entity when building the
> pipeline. We could solve the problem in a similar way for panels, but encoders
> need a more elaborate solution.
Why? Sorry, I don't see why an encoder is different in this respect than a panel.
I'm sure I'm missing something here.
>
> I see (at least) two possibilities here, either explicitly describing all
> entities that make the device in DT (as you have proposed below), or creating
> a hierarchy of entities, with parent entities that can contain several child
> entities. I've CC'ed Guennadi, Hans, Sylwester and Sakari to get their opinion
> on the matter.
I think the way this is done today in complex devices is that the driver just
exposes itself as a single sub-device, but internally it has its own pipeline of
sub-devices. The only one that I know of (platform/s5p-tv/hdmi_drv) doesn't expose
them to a media controller, they are completely hidden inside the hdmi driver.
The ability to support hierarchies of entities would be very nice. However, I
don't know how much work that would be to implement and if it is worth the
effort.
Regards,
Hans
>
>> lvds-encoder {
>> channel@0 {
>
> If I understand this correctly, your LVDS encoder has two independent
> channels. In the general case a device made of multiple entities might have
> those entities chained, so "channel" might not be the best term. "entity"
> might be a better choice.
>
>> port@0 {
>> lvds0_input: endpoint {
>> };
>> };
>> port@1 {
>> lvds0_output: endpoint {
>> };
>> };
>> };
>> channel@1 {
>> port@0 {
>> lvds1_input: endpoint {
>> };
>> };
>> lvds1: port@1 {
>> lvds1_output: endpoint {
>> };
>> };
>> };
>> };
>>
>>> + struct media_entity *local = &entity->entity;
>>> + struct device_node *ep = NULL;
>>> + int ret = 0;
>>> +
>>> + dev_dbg(dev, "creating links for entity %s\n", local->name);
>>> +
>>> + while (1) {
>>> + struct media_entity *remote = NULL;
>>> + struct media_pad *remote_pad;
>>> + struct media_pad *local_pad;
>>> + struct display_of_link link;
>>> + struct display_entity *ent;
>>> + struct device_node *next;
>>> +
>>> + /* Get the next endpoint and parse its link. */
>>> + next = display_of_get_next_endpoint(node, ep);
>>> + if (next = NULL)
>>> + break;
>>> +
>>> + of_node_put(ep);
>>> + ep = next;
>>> +
>>> + dev_dbg(dev, "processing endpoint %s\n", ep->full_name);
>>> +
>>> + ret = display_of_parse_link(ep, &link);
>>> + if (ret < 0) {
>>> + dev_err(dev, "failed to parse link for %s\n",
>>> + ep->full_name);
>>> + continue;
>>> + }
>>> +
>>> + /* Skip source pads, they will be processed from the other end of
>>> + * the link.
>>> + */
>>> + if (link.local_port >= local->num_pads) {
>>> + dev_err(dev, "invalid port number %u on %s\n",
>>> + link.local_port, link.local_node->full_name);
>>> + display_of_put_link(&link);
>>> + ret = -EINVAL;
>>> + break;
>>> + }
>>> +
>>> + local_pad = &local->pads[link.local_port];
>>> +
>>> + if (local_pad->flags & MEDIA_PAD_FL_SOURCE) {
>>> + dev_dbg(dev, "skipping source port %s:%u\n",
>>> + link.local_node->full_name, link.local_port);
>>> + display_of_put_link(&link);
>>> + continue;
>>> + }
>>> +
>>> + /* Find the remote entity. If not found, just skip the link as
>>> + * it goes out of scope of the entities handled by the notifier.
>>> + */
>>> + list_for_each_entry(ent, entities, list) {
>>> + if (ent->dev->of_node = link.remote_node) {
>>> + remote = &ent->entity;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (root->dev->of_node = link.remote_node)
>>> + remote = &root->entity;
>>> +
>>> + if (remote = NULL) {
>>> + dev_dbg(dev, "no entity found for %s\n",
>>> + link.remote_node->full_name);
>>> + display_of_put_link(&link);
>>> + continue;
>>> + }
>>> +
>>> + if (link.remote_port >= remote->num_pads) {
>>> + dev_err(dev, "invalid port number %u on %s\n",
>>> + link.remote_port, link.remote_node->full_name);
>>> + display_of_put_link(&link);
>>> + ret = -EINVAL;
>>> + break;
>>> + }
>>> +
>>> + remote_pad = &remote->pads[link.remote_port];
>>> +
>>> + display_of_put_link(&link);
>>> +
>>> + /* Create the media link. */
>>> + dev_dbg(dev, "creating %s:%u -> %s:%u link\n",
>>> + remote->name, remote_pad->index,
>>> + local->name, local_pad->index);
>>> +
>>> + ret = media_entity_create_link(remote, remote_pad->index,
>>> + local, local_pad->index,
>>> + link_flags);
>>> + if (ret < 0) {
>>> + dev_err(dev,
>>> + "failed to create %s:%u -> %s:%u link\n",
>>> + remote->name, remote_pad->index,
>>> + local->name, local_pad->index);
>>> + break;
>>> + }
>>> + }
>>> +
>>> + of_node_put(ep);
>>> + return ret;
>>> +}
>>
>> [...]
>>
>> For example like this:
>>
>> diff --git a/drivers/video/display/display-core.c
>> b/drivers/video/display/display-core.c index 7910c23..a04feed 100644
>> --- a/drivers/video/display/display-core.c
>> +++ b/drivers/video/display/display-core.c
>> @@ -302,6 +302,9 @@ int display_entity_init(struct display_entity *entity,
>> unsigned int num_sinks, kref_init(&entity->ref);
>> entity->state = DISPLAY_ENTITY_STATE_OFF;
>>
>> + if (!entity->of_node && entity->dev)
>> + entity->of_node = entity->dev->of_node;
>> +
>> num_pads = num_sinks + num_sources;
>> pads = kzalloc(sizeof(*pads) * num_pads, GFP_KERNEL);
>> if (pads = NULL)
>> @@ -665,7 +668,7 @@ static int display_of_entity_link_entity(struct device
>> *dev, struct display_entity *root)
>> {
>> u32 link_flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED;
>> - const struct device_node *node = entity->dev->of_node;
>> + const struct device_node *node = entity->of_node;
>> struct media_entity *local = &entity->entity;
>> struct device_node *ep = NULL;
>> int num_sink, ret = 0;
>> @@ -727,13 +730,13 @@ static int display_of_entity_link_entity(struct device
>> *dev, * it goes out of scope of the entities handled by the notifier. */
>> list_for_each_entry(ent, entities, list) {
>> - if (ent->dev->of_node = link.remote_node) {
>> + if (ent->of_node = link.remote_node) {
>> remote = &ent->entity;
>> break;
>> }
>> }
>>
>> - if (root && root->dev->of_node = link.remote_node)
>> + if (root && root->of_node = link.remote_node)
>> remote = &root->entity;
>>
>> if (remote = NULL) {
>> diff --git a/drivers/video/display/display-notifier.c
>> b/drivers/video/display/display-notifier.c index a3998c7..d0da6e5 100644
>> --- a/drivers/video/display/display-notifier.c
>> +++ b/drivers/video/display/display-notifier.c
>> @@ -28,28 +28,30 @@ static DEFINE_MUTEX(display_entity_mutex);
>> * Notifiers
>> */
>>
>> -static bool match_platform(struct device *dev,
>> +static bool match_platform(struct display_entity *entity,
>> struct display_entity_match *match)
>> {
>> pr_debug("%s: matching device '%s' with name '%s'\n", __func__,
>> - dev_name(dev), match->match.platform.name);
>> + dev_name(entity->dev), match->match.platform.name);
>>
>> - return !strcmp(match->match.platform.name, dev_name(dev));
>> + return !strcmp(match->match.platform.name, dev_name(entity->dev));
>> }
>>
>> -static bool match_dt(struct device *dev, struct display_entity_match
>> *match) +static bool match_dt(struct display_entity *entity,
>> + struct display_entity_match *match)
>> {
>> pr_debug("%s: matching device node '%s' with node '%s'\n", __func__,
>> - dev->of_node->full_name, match->match.dt.node->full_name);
>> + entity->of_node->full_name, match->match.dt.node->full_name);
>>
>> - return match->match.dt.node = dev->of_node;
>> + return match->match.dt.node = entity->of_node;
>> }
>>
>> static struct display_entity_match *
>> display_entity_notifier_match(struct display_entity_notifier *notifier,
>> struct display_entity *entity)
>> {
>> - bool (*match_func)(struct device *, struct display_entity_match *);
>> + bool (*match_func)(struct display_entity *,
>> + struct display_entity_match *);
>> struct display_entity_match *match;
>>
>> pr_debug("%s: matching entity '%s' (ptr 0x%p dev '%s')\n", __func__,
>> @@ -66,7 +68,7 @@ display_entity_notifier_match(struct
>> display_entity_notifier *notifier, break;
>> }
>>
>> - if (match_func(entity->dev, match))
>> + if (match_func(entity, match))
>> return match;
>> }
>>
>> diff --git a/include/video/display.h b/include/video/display.h
>> index 4c402bee..d1f8833 100644
>> --- a/include/video/display.h
>> +++ b/include/video/display.h
>> @@ -228,6 +228,7 @@ struct display_entity {
>> struct list_head list;
>> struct device *dev;
>> struct module *owner;
>> + struct device_node *of_node;
>> struct kref ref;
>>
>> char name[32];
^ permalink raw reply
* Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support
From: Pawel Moll @ 2013-09-11 16:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130910194324.GE12758@n2100.arm.linux.org.uk>
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ł
^ permalink raw reply
* Re: [PATCH v2 1/2] video: ARM CLCD: Add DT support
From: Stephen Warren @ 2013-09-11 17:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1378899931.4082.75.camel@hornet>
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?
^ permalink raw reply
* Re: [PATCH 1/2] video: ARM CLCD: Add DT support
From: Sylwester Nawrocki @ 2013-09-11 21:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1378907036.4082.105.camel@hornet>
On 09/11/2013 03:43 PM, Pawel Moll wrote:
> On Tue, 2013-09-10 at 22:41 +0100, Sylwester Nawrocki wrote:
>> On 09/09/2013 01:19 PM, Pawel Moll wrote:
>>> On Sat, 2013-09-07 at 23:55 +0100, Sylwester Nawrocki wrote:
[...]
>>>>> +- 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
>>>>
>>>> It seems the "specialized video memory" is described by some vendor specific
>>>> DT binding ? Is it documented ? It sounds like you are unnecessarily
>>>> repeating the memory node details here.
>>>
>>> I appreciate this property being the hardest to swallow, but the problem
>>> it is trying to solve is quite simple, really. System can be designed in
>>> such a way that CLCD is *not* able to read data from the main memory. In
>>> such case there will be a separate block of (usually static) memory
>>> "next" to CLCD, which *must* be used for the framebuffer. And I've got
>>> two choices here: to simply define an address and size, or to define a
>>> phandle to a node with standard reg property. I'll be really grateful
>>> for ideas how to describe the situation better.
>>
>> I thought about reusing the binding only, the part defining reserved
>> (carve out) memory regions.
>
> Em, what exactly do you mean? Referring to the definition of "reserved
> memory region" as an example how to use reg =<(address) (size)>?
> (because, I don't think it can be "linux,contiguous-memory-region",
> "reserved-memory-region"). Can do, however I'm not sure if it won't
> cause even more confusion, as I hope that the CMA bindings will be
> useful here on their own, to allocate "normal" contiguous framebuffers.
> Keep in mind those two use cases are very very different.
Yes, I wasn't aware then that your "video RAM" is a separate chip attached
to a distinct bus.
My idea was to reuse memory node structure, including "reserved-memory"
compatible value (note there are some fixups pending and those names are
subject to change). Not sure about the property in CLCD device node
containing phandle to the memory node (currently 'memory-region').
>>>> Perhaps this binding/driver should use the common reserved memory bindings,
>>>> see thread http://www.spinics.net/lists/devicetree/msg02880.html
>>>
>>> No, not really - as I said, this is *not* the main RAM of the system.
>>> CMA doesn't even know about its existence.
>>
>> I'm really not an expert in this area, I'll assume we don't list such
>> separate memory chips in the 'memory' node.
>
> ePAPR spec, regarding the memory node, states: "The client program may
> access memory not covered by any memory reservations (see section 8.3)
> using any storage attributes it chooses." (note that the reservations
> mentioned are "the other" reservations, /memreserve/ ones ;-)
>
> Now, I wouldn't want my "client program" (read: Linux kernel) to use the
> "video memory" in question for general allocations, if only because of
> its rather poor performance (resulting from the interconnect design, not
> the memory chip characteristics), so I treat it more as a memory mapped
> device which happens to have a lot of word-long registers...
>
>> But if such memory could be used not only by this single IP block it would
>> probably make sense to have it listed in memory/reserved-memory, so it
>> could be used by other devices of which drivers possibly wouldn't have to
>> contain all the detailed dt parsing/memory handling code.
>
> The "video RAM" on my platform could be used for other purposes. If you
> don't mind latencies :-) It's just that the CLCD itself can't use
> anything else.
It seems then you just need to assign specific memory region to the device.
That's one of the main requirements the recent "CMA" memory bindings were
supposed to address.
> So, if the memory/reserved-memory described areas *not* to be used as
> "normal RAM", yes - we could use it. I don't think it's the case now,
> though.
AFAIU it's the case, you just need to have compatible property set to
"reserved-memory-region". Then the kernel will early call memblock_remove()
on that region, which will subsequently be declared for use by the device
DMA with a call to dma_declare_coherent_memory(), while populating devices
from DT [1]. It's likely I'm missing some details though, that would make
this unsuitable for your environment.
[1] http://www.spinics.net/lists/devicetree/msg02880.html
> Also, Steven mentioned the other option I talked about - just raw
> address/size pair. See me deliberation there...
>
>>>>> +- 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
>>>>
>>>> Are this vendor specific or common properties ? Shouldn't those be prefixed
>>>> with the vendor name ?
>>>
>>> I have no answer to this question. My guts are telling me - nope. TFT is
>>> TFT, STN is STN, nothing to do with "arm,". But I welcome other
>>> opinions.
>>
>> I thought about documenting such a common properties in a common place.
>> It's
>> not immediately clear from names of these properties that they apply to
>> PL110/
>> PL111 devices only. If we let such generic names being redefined across DT
>> bindings of different devices it is going to be pretty messy IMHO. Same
>> property in two different dts files would potentially have different
>> meaning.
>>
>> So perhaps instead of panel-dimensions we could define common
>> properties, e.g.
>>
>> - display-phys-width: physical horizontal dimension of a display in
>> millimetres
>> (micrometres ?);
>> - display-phys-height: physical vertical dimension of a display in
>> millimetres
>> (micrometres ?);
>>
>> Instead of 'panel-stn-color' a boolean property 'monochrome-display',
>> the default
>> when this property was missing would be "colour display".
>>
>> I'd like to leave defining such common properties to someone having more
>> experience
>> in the display area. I don't think it would take much time come up with
>> generic
>> names for that couple properties you need. Then CDF implementation
>> would simply use whatever gets agreed.
>
> Ok, I see what you're saying. Yes, this could be done. No, I don't claim
> to have enough expertise either (micrometers??? :-O ;-) The other thing
> is that I don't really expect generic CDF bindings to cover such things.
> They will (hopefully) only describe what's connected with what. And the
> drivers should know how. Of course they may need the dimensions& alike
> in the tree (of course having them standardised would help here), but
> it's not a CDF job to provide those.
Of course it's always easier to define couple of DT properties that would
cover part of functionality of some specific device. But then such
properties
should be prefixed with vendor name AFAIU, since they are not approved as
common ones and useful for wider set of devices.
From the device tree perspective CDF is just a collection of (display
related) devices and a complete set of DT properties will be needed to
describe them. Certainly some share of device-specific properties should
be expected. Links between (sub)devices can be already described in DT by
the binding documented in video-interfaces.txt.
--
Thanks,
Sylwester
^ permalink raw reply
* [PATCH 1/2] aty128fb: Use pci_dev pm_cap
From: Jon Mason @ 2013-09-11 21:35 UTC (permalink / raw)
To: linux-fbdev
Use the already existing pm_cap variable in struct pci_dev for
determining the power management offset. This saves the driver from
having to keep track of an extra variable.
Signed-off-by: Jon Mason <jdmason@kudzu.us>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/aty/aty128fb.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/video/aty/aty128fb.c b/drivers/video/aty/aty128fb.c
index a4dfe8c..b5edb6f 100644
--- a/drivers/video/aty/aty128fb.c
+++ b/drivers/video/aty/aty128fb.c
@@ -413,7 +413,6 @@ struct aty128fb_par {
int blitter_may_be_busy;
int fifo_slots; /* free slots in FIFO (64 max) */
- int pm_reg;
int crt_on, lcd_on;
struct pci_dev *pdev;
struct fb_info *next;
@@ -2016,7 +2015,6 @@ static int aty128_init(struct pci_dev *pdev, const struct pci_device_id *ent)
aty128_init_engine(par);
- par->pm_reg = pdev->pm_cap;
par->pdev = pdev;
par->asleep = 0;
par->lock_blank = 0;
@@ -2397,7 +2395,7 @@ static void aty128_set_suspend(struct aty128fb_par *par, int suspend)
u32 pmgt;
struct pci_dev *pdev = par->pdev;
- if (!par->pm_reg)
+ if (!par->pdev->pm_cap)
return;
/* Set the chip into the appropriate suspend mode (we use D2,
--
1.7.10.4
^ permalink raw reply related
* [PATCH 2/2] radeonfb: Use pci_dev pm_cap
From: Jon Mason @ 2013-09-11 21:35 UTC (permalink / raw)
To: linux-fbdev
Use the already existing pm_cap variable in struct pci_dev for
determining the power management offset. This saves the driver from
having to keep track of an extra variable. Also, use the pci_power_t
value instead of always using the raw value for PCI_D2.
Signed-off-by: Jon Mason <jdmason@kudzu.us>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/aty/radeon_pm.c | 15 ++++++---------
drivers/video/aty/radeonfb.h | 1 -
2 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/video/aty/radeon_pm.c b/drivers/video/aty/radeon_pm.c
index f7091ec..f266224 100644
--- a/drivers/video/aty/radeon_pm.c
+++ b/drivers/video/aty/radeon_pm.c
@@ -2515,13 +2515,13 @@ static void radeonfb_whack_power_state(struct radeonfb_info *rinfo, pci_power_t
for (;;) {
pci_read_config_word(rinfo->pdev,
- rinfo->pm_reg+PCI_PM_CTRL,
+ rinfo->pdev->pm_cap + PCI_PM_CTRL,
&pwr_cmd);
- if (pwr_cmd & 2)
+ if (pwr_cmd & state)
break;
- pwr_cmd = (pwr_cmd & ~PCI_PM_CTRL_STATE_MASK) | 2;
+ pwr_cmd = (pwr_cmd & ~PCI_PM_CTRL_STATE_MASK) | state;
pci_write_config_word(rinfo->pdev,
- rinfo->pm_reg+PCI_PM_CTRL,
+ rinfo->pdev->pm_cap + PCI_PM_CTRL,
pwr_cmd);
msleep(500);
}
@@ -2532,7 +2532,7 @@ static void radeon_set_suspend(struct radeonfb_info *rinfo, int suspend)
{
u32 tmp;
- if (!rinfo->pm_reg)
+ if (!rinfo->pdev->pm_cap)
return;
/* Set the chip into appropriate suspend mode (we use D2,
@@ -2804,9 +2804,6 @@ static void radeonfb_early_resume(void *data)
void radeonfb_pm_init(struct radeonfb_info *rinfo, int dynclk, int ignore_devlist, int force_sleep)
{
- /* Find PM registers in config space if any*/
- rinfo->pm_reg = rinfo->pdev->pm_cap;
-
/* Enable/Disable dynamic clocks: TODO add sysfs access */
if (rinfo->family = CHIP_FAMILY_RS480)
rinfo->dynclk = -1;
@@ -2830,7 +2827,7 @@ void radeonfb_pm_init(struct radeonfb_info *rinfo, int dynclk, int ignore_devlis
* reason. --BenH
*/
if (machine_is(powermac) && rinfo->of_node) {
- if (rinfo->is_mobility && rinfo->pm_reg &&
+ if (rinfo->is_mobility && rinfo->pdev->pm_cap &&
rinfo->family <= CHIP_FAMILY_RV250)
rinfo->pm_mode |= radeon_pm_d2;
diff --git a/drivers/video/aty/radeonfb.h b/drivers/video/aty/radeonfb.h
index 7351e66..cb84604 100644
--- a/drivers/video/aty/radeonfb.h
+++ b/drivers/video/aty/radeonfb.h
@@ -342,7 +342,6 @@ struct radeonfb_info {
int mtrr_hdl;
- int pm_reg;
u32 save_regs[100];
int asleep;
int lock_blank;
--
1.7.10.4
^ permalink raw reply related
* [PATCH 1/3] video: xilinxfb: Use standard variable name convention
From: Michal Simek @ 2013-09-12 5:54 UTC (permalink / raw)
To: linux-kernel, monstr
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Grant Likely,
Rob Herring, linux-fbdev, devicetree
[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]
s/op/pdev/ in xilinxfb_of_probe().
No functional chagnes.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
drivers/video/xilinxfb.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index 84c664e..123cd70 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -413,7 +413,7 @@ static int xilinxfb_release(struct device *dev)
* OF bus binding
*/
-static int xilinxfb_of_probe(struct platform_device *op)
+static int xilinxfb_of_probe(struct platform_device *pdev)
{
const u32 *prop;
u32 tft_access = 0;
@@ -427,7 +427,7 @@ static int xilinxfb_of_probe(struct platform_device *op)
/* Allocate the driver data region */
drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
if (!drvdata) {
- dev_err(&op->dev, "Couldn't allocate device private record\n");
+ dev_err(&pdev->dev, "Couldn't allocate device private record\n");
return -ENOMEM;
}
@@ -435,7 +435,7 @@ static int xilinxfb_of_probe(struct platform_device *op)
* To check whether the core is connected directly to DCR or BUS
* interface and initialize the tft_access accordingly.
*/
- of_property_read_u32(op->dev.of_node, "xlnx,dcr-splb-slave-if",
+ of_property_read_u32(pdev->dev.of_node, "xlnx,dcr-splb-slave-if",
&tft_access);
/*
@@ -459,29 +459,29 @@ static int xilinxfb_of_probe(struct platform_device *op)
}
#endif
- prop = of_get_property(op->dev.of_node, "phys-size", &size);
+ prop = of_get_property(pdev->dev.of_node, "phys-size", &size);
if ((prop) && (size >= sizeof(u32)*2)) {
pdata.screen_width_mm = prop[0];
pdata.screen_height_mm = prop[1];
}
- prop = of_get_property(op->dev.of_node, "resolution", &size);
+ prop = of_get_property(pdev->dev.of_node, "resolution", &size);
if ((prop) && (size >= sizeof(u32)*2)) {
pdata.xres = prop[0];
pdata.yres = prop[1];
}
- prop = of_get_property(op->dev.of_node, "virtual-resolution", &size);
+ prop = of_get_property(pdev->dev.of_node, "virtual-resolution", &size);
if ((prop) && (size >= sizeof(u32)*2)) {
pdata.xvirt = prop[0];
pdata.yvirt = prop[1];
}
- if (of_find_property(op->dev.of_node, "rotate-display", NULL))
+ if (of_find_property(pdev->dev.of_node, "rotate-display", NULL))
pdata.rotate_screen = 1;
- dev_set_drvdata(&op->dev, drvdata);
- return xilinxfb_assign(op, drvdata, &pdata);
+ dev_set_drvdata(&pdev->dev, drvdata);
+ return xilinxfb_assign(pdev, drvdata, &pdata);
}
static int xilinxfb_of_remove(struct platform_device *op)
--
1.8.2.3
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply related
* [PATCH 2/3] video: xilinxfb: Use devm_kzalloc instead of kzalloc
From: Michal Simek @ 2013-09-12 5:54 UTC (permalink / raw)
To: linux-kernel, monstr
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev
In-Reply-To: <7016a90750626ba866dddc6f85cfdd71943f6891.1378965270.git.michal.simek@xilinx.com>
[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]
Simplify driver probe and release function.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
drivers/video/xilinxfb.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index 123cd70..fd9c430 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -368,7 +368,6 @@ err_fbmem:
devm_iounmap(dev, drvdata->regs);
err_region:
- kfree(drvdata);
dev_set_drvdata(dev, NULL);
return rc;
@@ -403,7 +402,6 @@ static int xilinxfb_release(struct device *dev)
dcr_unmap(drvdata->dcr_host, drvdata->dcr_len);
#endif
- kfree(drvdata);
dev_set_drvdata(dev, NULL);
return 0;
@@ -425,7 +423,7 @@ static int xilinxfb_of_probe(struct platform_device *pdev)
pdata = xilinx_fb_default_pdata;
/* Allocate the driver data region */
- drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
+ drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
if (!drvdata) {
dev_err(&pdev->dev, "Couldn't allocate device private record\n");
return -ENOMEM;
@@ -453,7 +451,6 @@ static int xilinxfb_of_probe(struct platform_device *pdev)
drvdata->dcr_host = dcr_map(op->dev.of_node, start, drvdata->dcr_len);
if (!DCR_MAP_OK(drvdata->dcr_host)) {
dev_err(&op->dev, "invalid DCR address\n");
- kfree(drvdata);
return -ENODEV;
}
}
--
1.8.2.3
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox