[U-Boot] [PATCH v5 0/3] sunxi: video: Add simplefb support

[U-Boot] [PATCH v5 0/3] sunxi: video: Add simplefb support

[Hans de Goede]

Hi All,

Here is v5 of my sunxi simplefb support set.

Changes since v4:
-Make sunxi_simplefb_setup return error codes
-Add a patch to make lcd_dt_simplefb_configure_node use the new
 fdt_setup_simplefb_node helper (only compile tested!)

Changes since v3:
-Added a fdt_setup_simplefb_node() helper function (new patch)
-Changed how u-boot finds the pre-populated simplefb node to use a
 compatible string

Changes since v2:
-Detect and handle address and size #cells

Changes since v1:
-Use fdt_setprop_string for strings

Simon, even though -rc1 is out I would still like to get this upstream for
v2015.01 if possible. This is in essence just refactoring of code posted
well before -rc1. If I can get your ack on both the fdt_setup_simplefb_node
and the lcd patches, then I can take them upstream through the sunxi tree,
or if you plan to do a pull-req soon anyways, you can take them upstream
through your tree, and I'll wait for that.

Thanks & Regards,

Hans

[U-Boot] [PATCH v5 1/3] fdt_support: Add a fdt_setup_simplefb_node helper function

[Hans de Goede]

Add a generic helper to fill and enable simplefb nodes.

The first user of this will be the sunxi display code.

lcd_dt_simplefb_configure_node is also a good candidate to be converted
to use this, but that requires someone to run some tests first, as
lcd_dt_simplefb_configure_node does not honor #address-cells and #size-cells,
but simply assumes 1 and 1 for both.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 common/fdt_support.c  | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/fdt_support.h |  3 +++
 2 files changed, 68 insertions(+)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 3f64156..0ffa711 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -1523,3 +1523,68 @@ int fdt_read_range(void *fdt, int node, int n, uint64_t *child_addr,
 
 	return 0;
 }
+
+/**
+ * fdt_setup_simplefb_node - Fill and enable a simplefb node
+ *
+ * @fdt: ptr to device tree
+ * @node: offset of the simplefb node
+ * @base_address: framebuffer base address
+ * @width: width in pixels
+ * @height: height in pixels
+ * @stride: bytes per line
+ * @format: pixel format string
+ *
+ * Convenience function to fill and enable a simplefb node.
+ */
+int fdt_setup_simplefb_node(void *fdt, int node, u64 base_address, u32 width,
+			    u32 height, u32 stride, const char *format)
+{
+	char name[32];
+	fdt32_t cells[4];
+	int i, addrc, sizec, ret;
+
+	of_bus_default_count_cells(fdt, fdt_parent_offset(fdt, node),
+				   &addrc, &sizec);
+	i = 0;
+	if (addrc == 2)
+		cells[i++] = cpu_to_fdt32(base_address >> 32);
+	cells[i++] = cpu_to_fdt32(base_address);
+	if (sizec == 2)
+		cells[i++] = 0;
+	cells[i++] = cpu_to_fdt32(height * stride);
+
+	ret = fdt_setprop(fdt, node, "reg", cells, sizeof(cells[0]) * i);
+	if (ret < 0)
+		return ret;
+
+	snprintf(name, sizeof(name), "framebuffer@%llx", base_address);
+	ret = fdt_set_name(fdt, node, name);
+	if (ret < 0)
+		return ret;
+
+	cells[0] = cpu_to_fdt32(width);
+	ret = fdt_setprop(fdt, node, "width", cells, sizeof(cells[0]));
+	if (ret < 0)
+		return ret;
+
+	cells[0] = cpu_to_fdt32(height);
+	ret = fdt_setprop(fdt, node, "height", cells, sizeof(cells[0]));
+	if (ret < 0)
+		return ret;
+
+	cells[0] = cpu_to_fdt32(stride);
+	ret = fdt_setprop(fdt, node, "stride", cells, sizeof(cells[0]));
+	if (ret < 0)
+		return ret;
+
+	ret = fdt_setprop_string(fdt, node, "format", format);
+	if (ret < 0)
+		return ret;
+
+	ret = fdt_setprop_string(fdt, node, "status", "okay");
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 55cef94..d5e09e6 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -147,6 +147,9 @@ void of_bus_default_count_cells(void *blob, int parentoffset,
 int ft_verify_fdt(void *fdt);
 int arch_fixup_memory_node(void *blob);
 
+int fdt_setup_simplefb_node(void *fdt, int node, u64 base_address, u32 width,
+			    u32 height, u32 stride, const char *format);
+
 #endif /* ifdef CONFIG_OF_LIBFDT */
 
 #ifdef USE_HOSTCC
-- 
2.1.0

[U-Boot] [PATCH v5 2/3] common/lcd: Make lcd_dt_simplefb_configure_node use fdt_setup_simplefb_node

[Hans de Goede]

Change lcd_dt_simplefb_configure_node into a wrapper around the new generic
fdt_setup_simplefb_node helper function.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 common/lcd.c | 47 +++++------------------------------------------
 1 file changed, 5 insertions(+), 42 deletions(-)

diff --git a/common/lcd.c b/common/lcd.c
index 37147af..54dd294 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -30,6 +30,7 @@
 #include <splash.h>
 #include <asm/io.h>
 #include <asm/unaligned.h>
+#include <fdt_support.h>
 
 #if defined(CONFIG_CPU_PXA25X) || defined(CONFIG_CPU_PXA27X) || \
 	defined(CONFIG_CPU_MONAHANS)
@@ -1172,51 +1173,13 @@ int lcd_get_screen_columns(void)
 #if defined(CONFIG_LCD_DT_SIMPLEFB)
 static int lcd_dt_simplefb_configure_node(void *blob, int off)
 {
-	u32 stride;
-	fdt32_t cells[2];
-	int ret;
-	static const char format[] =
 #if LCD_BPP == LCD_COLOR16
-		"r5g6b5";
+	return fdt_setup_simplefb_node(blob, off, gd->fb_base,
+				       panel_info.vl_col, panel_info.vl_row,
+				       panel_info.vl_col * 2, "r5g6b5");
 #else
-		"";
+	return -1;
 #endif
-
-	if (!format[0])
-		return -1;
-
-	stride = panel_info.vl_col * 2;
-
-	cells[0] = cpu_to_fdt32(gd->fb_base);
-	cells[1] = cpu_to_fdt32(stride * panel_info.vl_row);
-	ret = fdt_setprop(blob, off, "reg", cells, sizeof(cells[0]) * 2);
-	if (ret < 0)
-		return -1;
-
-	cells[0] = cpu_to_fdt32(panel_info.vl_col);
-	ret = fdt_setprop(blob, off, "width", cells, sizeof(cells[0]));
-	if (ret < 0)
-		return -1;
-
-	cells[0] = cpu_to_fdt32(panel_info.vl_row);
-	ret = fdt_setprop(blob, off, "height", cells, sizeof(cells[0]));
-	if (ret < 0)
-		return -1;
-
-	cells[0] = cpu_to_fdt32(stride);
-	ret = fdt_setprop(blob, off, "stride", cells, sizeof(cells[0]));
-	if (ret < 0)
-		return -1;
-
-	ret = fdt_setprop(blob, off, "format", format, strlen(format) + 1);
-	if (ret < 0)
-		return -1;
-
-	ret = fdt_delprop(blob, off, "status");
-	if (ret < 0)
-		return -1;
-
-	return 0;
 }
 
 int lcd_dt_simplefb_add_node(void *blob)
-- 
2.1.0

[U-Boot] [PATCH v5 3/3] sunxi: video: Add simplefb support

[Hans de Goede]

From: Luc Verhaegen <libv@skynet.be>

Add simplefb support, note this depends on the kernel having support for
the clocks property which has recently been added to the simplefb devicetree
binding.

Signed-off-by: Luc Verhaegen <libv@skynet.be>
[hdegoede at redhat.com: Use pre-populated simplefb node under /chosen as
 disussed on the devicetree list]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/include/asm/arch-sunxi/display.h |  4 +++
 board/sunxi/board.c                       | 11 ++++++++
 drivers/video/sunxi_display.c             | 42 +++++++++++++++++++++++++++++++
 include/configs/sunxi-common.h            |  8 ++++++
 4 files changed, 65 insertions(+)

diff --git a/arch/arm/include/asm/arch-sunxi/display.h b/arch/arm/include/asm/arch-sunxi/display.h
index 8d80ceb..c9d81ba 100644
--- a/arch/arm/include/asm/arch-sunxi/display.h
+++ b/arch/arm/include/asm/arch-sunxi/display.h
@@ -195,4 +195,8 @@ struct sunxi_hdmi_reg {
 #define SUNXI_HDMI_PLL_DBG0_PLL3		(0 << 21)
 #define SUNXI_HDMI_PLL_DBG0_PLL7		(1 << 21)
 
+#ifdef CONFIG_VIDEO_DT_SIMPLEFB
+int sunxi_simplefb_setup(void *blob);
+#endif
+
 #endif /* _SUNXI_DISPLAY_H */
diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index e6ec5b8..d4530e8 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -24,6 +24,7 @@
 #endif
 #include <asm/arch/clock.h>
 #include <asm/arch/cpu.h>
+#include <asm/arch/display.h>
 #include <asm/arch/dram.h>
 #include <asm/arch/gpio.h>
 #include <asm/arch/mmc.h>
@@ -237,3 +238,13 @@ int misc_init_r(void)
 	return 0;
 }
 #endif
+
+#ifdef CONFIG_OF_BOARD_SETUP
+void
+ft_board_setup(void *blob, bd_t *bd)
+{
+#ifdef CONFIG_VIDEO_DT_SIMPLEFB
+	sunxi_simplefb_setup(blob);
+#endif
+}
+#endif /* CONFIG_OF_BOARD_SETUP */
diff --git a/drivers/video/sunxi_display.c b/drivers/video/sunxi_display.c
index 3f46c31..2f3f5db 100644
--- a/drivers/video/sunxi_display.c
+++ b/drivers/video/sunxi_display.c
@@ -13,6 +13,8 @@
 #include <asm/arch/display.h>
 #include <asm/global_data.h>
 #include <asm/io.h>
+#include <fdtdec.h>
+#include <fdt_support.h>
 #include <linux/fb.h>
 #include <video_fb.h>
 
@@ -416,3 +418,43 @@ video_hw_init(void)
 
 	return graphic_device;
 }
+
+/*
+ * Simplefb support.
+ */
+#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
+int
+sunxi_simplefb_setup(void *blob)
+{
+	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
+	int offset, ret;
+
+	if (!sunxi_display.enabled)
+		return 0;
+
+	/* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */
+	offset = fdt_node_offset_by_compatible(blob, -1,
+					       "allwinner,simple-framebuffer");
+	while (offset >= 0) {
+		ret = fdt_find_string(blob, offset, "allwinner,pipeline",
+				      "de_be0-lcd0-hdmi");
+		if (ret == 0)
+			break;
+		offset = fdt_node_offset_by_compatible(blob, offset,
+					       "allwinner,simple-framebuffer");
+	}
+	if (offset < 0) {
+		eprintf("Cannot setup simplefb: node not found\n");
+		return 0; /* Keep older kernels working */
+	}
+
+	ret = fdt_setup_simplefb_node(blob, offset, gd->fb_base,
+			graphic_device->winSizeX, graphic_device->winSizeY,
+			graphic_device->winSizeX * graphic_device->gdfBytesPP,
+			"x8r8g8b8");
+	if (ret)
+		eprintf("Cannot setup simplefb: Error setting properties\n");
+
+	return ret;
+}
+#endif /* CONFIG_OF_BOARD_SETUP && CONFIG_VIDEO_DT_SIMPLEFB */
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 900ef52..d5d907b 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -204,6 +204,9 @@
  */
 #define CONFIG_SUNXI_FB_SIZE (8 << 20)
 
+/* Do we want to initialize a simple FB? */
+#define CONFIG_VIDEO_DT_SIMPLEFB
+
 #define CONFIG_VIDEO_SUNXI
 
 #define CONFIG_CFB_CONSOLE
@@ -217,6 +220,11 @@
 
 #define CONFIG_SYS_MEM_TOP_HIDE ((CONFIG_SUNXI_FB_SIZE + 0xFFF) & ~0xFFF)
 
+/* To be able to hook simplefb into dt */
+#ifdef CONFIG_VIDEO_DT_SIMPLEFB
+#define CONFIG_OF_BOARD_SETUP
+#endif
+
 #endif /* CONFIG_VIDEO */
 
 /* Ethernet support */
-- 
2.1.0

[U-Boot] [PATCH v5 0/3] sunxi: video: Add simplefb support

[Simon Glass]

Hi Hans,

On 19 November 2014 13:31, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi All,
>
> Here is v5 of my sunxi simplefb support set.
>
> Changes since v4:
> -Make sunxi_simplefb_setup return error codes
> -Add a patch to make lcd_dt_simplefb_configure_node use the new
>  fdt_setup_simplefb_node helper (only compile tested!)
>
> Changes since v3:
> -Added a fdt_setup_simplefb_node() helper function (new patch)
> -Changed how u-boot finds the pre-populated simplefb node to use a
>  compatible string
>
> Changes since v2:
> -Detect and handle address and size #cells
>
> Changes since v1:
> -Use fdt_setprop_string for strings
>
> Simon, even though -rc1 is out I would still like to get this upstream for
> v2015.01 if possible. This is in essence just refactoring of code posted
> well before -rc1. If I can get your ack on both the fdt_setup_simplefb_node
> and the lcd patches, then I can take them upstream through the sunxi tree,
> or if you plan to do a pull-req soon anyways, you can take them upstream
> through your tree, and I'll wait for that.

That seems good to me. I'll see if I can test it when I get back. I
added Stephen also as he wrote the Raspberry Pi stuff.

Regards,
Simon

[U-Boot] [PATCH v5 1/3] fdt_support: Add a fdt_setup_simplefb_node helper function

[Stephen Warren]

On 11/19/2014 06:31 AM, Hans de Goede wrote:
> Add a generic helper to fill and enable simplefb nodes.
> 
> The first user of this will be the sunxi display code.
> 
> lcd_dt_simplefb_configure_node is also a good candidate to be converted
> to use this, but that requires someone to run some tests first, as
> lcd_dt_simplefb_configure_node does not honor #address-cells and #size-cells,
> but simply assumes 1 and 1 for both.

Sorry, I'd been ignoring all simplefb stuff and hadn't realized this was
U-Boot related and needed testing.

Anyway, I've tested patches 1 and 2 on an RPi compute module with kernel
next-20141031 and it seems to work fine.

Patches 1, 2,
Tested-by: Stephen Warren <swarren@wwwdotorg.org>

[U-Boot] [PATCH v5 3/3] sunxi: video: Add simplefb support

[Ian Campbell]

On Wed, 2014-11-19 at 14:32 +0100, Hans de Goede wrote:
> From: Luc Verhaegen <libv@skynet.be>
> 
> Add simplefb support, note this depends on the kernel having support for
> the clocks property which has recently been added to the simplefb devicetree
> binding.
> 
> Signed-off-by: Luc Verhaegen <libv@skynet.be>
> [hdegoede at redhat.com: Use pre-populated simplefb node under /chosen as
>  disussed on the devicetree list]
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ian Campbell <ijc@hellion.org.uk>.

One non-blocking queries:

> +	/* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */
> +	offset = fdt_node_offset_by_compatible(blob, -1,
> +					       "allwinner,simple-framebuffer");
> +	while (offset >= 0) {
> +		ret = fdt_find_string(blob, offset, "allwinner,pipeline",
> +				      "de_be0-lcd0-hdmi");
> +		if (ret == 0)
> +			break;
> +		offset = fdt_node_offset_by_compatible(blob, offset,
> +					       "allwinner,simple-framebuffer");
> +	}

Is this variant non-conformant with coding style?:

	int offset = -1;
	while ( (offset = fdt_node_offset_by_compatible(blob, offset,
                                                        "allwinner,simple-framebuffer") ) {
		LOOP BODY
	}

I expect it is because of the assignment within the while condition,
which is a shame, since this is one case where it is IMHO leads to
clearer code.

[U-Boot] [PATCH v5 3/3] sunxi: video: Add simplefb support

[Hans de Goede]

Hi,

On 11/20/2014 10:22 AM, Ian Campbell wrote:
> On Wed, 2014-11-19 at 14:32 +0100, Hans de Goede wrote:
>> From: Luc Verhaegen <libv@skynet.be>
>>
>> Add simplefb support, note this depends on the kernel having support for
>> the clocks property which has recently been added to the simplefb devicetree
>> binding.
>>
>> Signed-off-by: Luc Verhaegen <libv@skynet.be>
>> [hdegoede at redhat.com: Use pre-populated simplefb node under /chosen as
>>  disussed on the devicetree list]
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Acked-by: Ian Campbell <ijc@hellion.org.uk>.

Thanks, any chance you could also review v2 of the CONFIG_USB_KEYBOARD patch ?

> One non-blocking queries:
> 
>> +	/* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */
>> +	offset = fdt_node_offset_by_compatible(blob, -1,
>> +					       "allwinner,simple-framebuffer");
>> +	while (offset >= 0) {
>> +		ret = fdt_find_string(blob, offset, "allwinner,pipeline",
>> +				      "de_be0-lcd0-hdmi");
>> +		if (ret == 0)
>> +			break;
>> +		offset = fdt_node_offset_by_compatible(blob, offset,
>> +					       "allwinner,simple-framebuffer");
>> +	}
> 
> Is this variant non-conformant with coding style?:
> 
> 	int offset = -1;
> 	while ( (offset = fdt_node_offset_by_compatible(blob, offset,
>                                                         "allwinner,simple-framebuffer") ) {
> 		LOOP BODY
> 	}
> 
> I expect it is because of the assignment within the while condition,
> which is a shame, since this is one case where it is IMHO leads to
> clearer code.

AFAIK this indeed would go against the coding style, and TBH I'm also not
sure if I agree it would be cleaner (mainly because indentation would become
(even more) messy due to the 80 columns limit).

Regards,

Hans

[U-Boot] [PATCH v5 3/3] sunxi: video: Add simplefb support

[Simon Glass]

Hi Hans,

On 19 November 2014 13:32, Hans de Goede <hdegoede@redhat.com> wrote:
> From: Luc Verhaegen <libv@skynet.be>
>
> Add simplefb support, note this depends on the kernel having support for
> the clocks property which has recently been added to the simplefb devicetree
> binding.
>
> Signed-off-by: Luc Verhaegen <libv@skynet.be>
> [hdegoede at redhat.com: Use pre-populated simplefb node under /chosen as
>  disussed on the devicetree list]
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  arch/arm/include/asm/arch-sunxi/display.h |  4 +++
>  board/sunxi/board.c                       | 11 ++++++++
>  drivers/video/sunxi_display.c             | 42 +++++++++++++++++++++++++++++++
>  include/configs/sunxi-common.h            |  8 ++++++
>  4 files changed, 65 insertions(+)
>
> diff --git a/arch/arm/include/asm/arch-sunxi/display.h b/arch/arm/include/asm/arch-sunxi/display.h
> index 8d80ceb..c9d81ba 100644
> --- a/arch/arm/include/asm/arch-sunxi/display.h
> +++ b/arch/arm/include/asm/arch-sunxi/display.h
> @@ -195,4 +195,8 @@ struct sunxi_hdmi_reg {
>  #define SUNXI_HDMI_PLL_DBG0_PLL3               (0 << 21)
>  #define SUNXI_HDMI_PLL_DBG0_PLL7               (1 << 21)
>
> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB
> +int sunxi_simplefb_setup(void *blob);
> +#endif

Do you really need this #ifdef?

> +
>  #endif /* _SUNXI_DISPLAY_H */
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index e6ec5b8..d4530e8 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -24,6 +24,7 @@
>  #endif
>  #include <asm/arch/clock.h>
>  #include <asm/arch/cpu.h>
> +#include <asm/arch/display.h>
>  #include <asm/arch/dram.h>
>  #include <asm/arch/gpio.h>
>  #include <asm/arch/mmc.h>
> @@ -237,3 +238,13 @@ int misc_init_r(void)
>         return 0;
>  }
>  #endif
> +
> +#ifdef CONFIG_OF_BOARD_SETUP
> +void
> +ft_board_setup(void *blob, bd_t *bd)

void on same line? Also can you make it return int. You could look at
u-boot-fdt/next (I am waiting on resolving one patch before sending a
pull).

> +{
> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB
> +       sunxi_simplefb_setup(blob);
> +#endif
> +}
> +#endif /* CONFIG_OF_BOARD_SETUP */
> diff --git a/drivers/video/sunxi_display.c b/drivers/video/sunxi_display.c
> index 3f46c31..2f3f5db 100644
> --- a/drivers/video/sunxi_display.c
> +++ b/drivers/video/sunxi_display.c
> @@ -13,6 +13,8 @@
>  #include <asm/arch/display.h>
>  #include <asm/global_data.h>
>  #include <asm/io.h>
> +#include <fdtdec.h>
> +#include <fdt_support.h>
>  #include <linux/fb.h>
>  #include <video_fb.h>
>
> @@ -416,3 +418,43 @@ video_hw_init(void)
>
>         return graphic_device;
>  }
> +
> +/*
> + * Simplefb support.
> + */
> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
> +int
> +sunxi_simplefb_setup(void *blob)

int on same line

> +{
> +       static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
> +       int offset, ret;
> +
> +       if (!sunxi_display.enabled)
> +               return 0;
> +
> +       /* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */
> +       offset = fdt_node_offset_by_compatible(blob, -1,
> +                                              "allwinner,simple-framebuffer");
> +       while (offset >= 0) {
> +               ret = fdt_find_string(blob, offset, "allwinner,pipeline",
> +                                     "de_be0-lcd0-hdmi");
> +               if (ret == 0)
> +                       break;
> +               offset = fdt_node_offset_by_compatible(blob, offset,
> +                                              "allwinner,simple-framebuffer");
> +       }

Can you use do...while() to avoid repeating the 'offset =' line?

> +       if (offset < 0) {
> +               eprintf("Cannot setup simplefb: node not found\n");
> +               return 0; /* Keep older kernels working */
> +       }
> +
> +       ret = fdt_setup_simplefb_node(blob, offset, gd->fb_base,
> +                       graphic_device->winSizeX, graphic_device->winSizeY,
> +                       graphic_device->winSizeX * graphic_device->gdfBytesPP,
> +                       "x8r8g8b8");
> +       if (ret)
> +               eprintf("Cannot setup simplefb: Error setting properties\n");
> +
> +       return ret;
> +}
> +#endif /* CONFIG_OF_BOARD_SETUP && CONFIG_VIDEO_DT_SIMPLEFB */
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index 900ef52..d5d907b 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -204,6 +204,9 @@
>   */
>  #define CONFIG_SUNXI_FB_SIZE (8 << 20)
>
> +/* Do we want to initialize a simple FB? */
> +#define CONFIG_VIDEO_DT_SIMPLEFB
> +
>  #define CONFIG_VIDEO_SUNXI
>
>  #define CONFIG_CFB_CONSOLE
> @@ -217,6 +220,11 @@
>
>  #define CONFIG_SYS_MEM_TOP_HIDE ((CONFIG_SUNXI_FB_SIZE + 0xFFF) & ~0xFFF)
>
> +/* To be able to hook simplefb into dt */
> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB
> +#define CONFIG_OF_BOARD_SETUP
> +#endif
> +
>  #endif /* CONFIG_VIDEO */
>
>  /* Ethernet support */
> --
> 2.1.0
>

Regards,
Simon

[U-Boot] [PATCH v5 1/3] fdt_support: Add a fdt_setup_simplefb_node helper function

[Simon Glass]

Hi Hans,

On 19 November 2014 13:31, Hans de Goede <hdegoede@redhat.com> wrote:
> Add a generic helper to fill and enable simplefb nodes.
>
> The first user of this will be the sunxi display code.
>
> lcd_dt_simplefb_configure_node is also a good candidate to be converted
> to use this, but that requires someone to run some tests first, as
> lcd_dt_simplefb_configure_node does not honor #address-cells and #size-cells,
> but simply assumes 1 and 1 for both.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

See below if you respin.

> ---
>  common/fdt_support.c  | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/fdt_support.h |  3 +++
>  2 files changed, 68 insertions(+)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 3f64156..0ffa711 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -1523,3 +1523,68 @@ int fdt_read_range(void *fdt, int node, int n, uint64_t *child_addr,
>
>         return 0;
>  }
> +
> +/**
> + * fdt_setup_simplefb_node - Fill and enable a simplefb node
> + *
> + * @fdt: ptr to device tree
> + * @node: offset of the simplefb node
> + * @base_address: framebuffer base address
> + * @width: width in pixels
> + * @height: height in pixels
> + * @stride: bytes per line
> + * @format: pixel format string
> + *
> + * Convenience function to fill and enable a simplefb node.
> + */

Better to put the comment in the header IMO.

> +int fdt_setup_simplefb_node(void *fdt, int node, u64 base_address, u32 width,
> +                           u32 height, u32 stride, const char *format)
> +{
> +       char name[32];
> +       fdt32_t cells[4];
> +       int i, addrc, sizec, ret;
> +
> +       of_bus_default_count_cells(fdt, fdt_parent_offset(fdt, node),
> +                                  &addrc, &sizec);
> +       i = 0;
> +       if (addrc == 2)
> +               cells[i++] = cpu_to_fdt32(base_address >> 32);
> +       cells[i++] = cpu_to_fdt32(base_address);
> +       if (sizec == 2)
> +               cells[i++] = 0;
> +       cells[i++] = cpu_to_fdt32(height * stride);
> +
> +       ret = fdt_setprop(fdt, node, "reg", cells, sizeof(cells[0]) * i);
> +       if (ret < 0)
> +               return ret;
> +
> +       snprintf(name, sizeof(name), "framebuffer@%llx", base_address);
> +       ret = fdt_set_name(fdt, node, name);
> +       if (ret < 0)
> +               return ret;
> +
> +       cells[0] = cpu_to_fdt32(width);
> +       ret = fdt_setprop(fdt, node, "width", cells, sizeof(cells[0]));

How about fdt_setprop_u32() instead?

> +       if (ret < 0)
> +               return ret;
> +
> +       cells[0] = cpu_to_fdt32(height);
> +       ret = fdt_setprop(fdt, node, "height", cells, sizeof(cells[0]));
> +       if (ret < 0)
> +               return ret;
> +
> +       cells[0] = cpu_to_fdt32(stride);
> +       ret = fdt_setprop(fdt, node, "stride", cells, sizeof(cells[0]));
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = fdt_setprop_string(fdt, node, "format", format);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = fdt_setprop_string(fdt, node, "status", "okay");
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
> +}
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 55cef94..d5e09e6 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -147,6 +147,9 @@ void of_bus_default_count_cells(void *blob, int parentoffset,
>  int ft_verify_fdt(void *fdt);
>  int arch_fixup_memory_node(void *blob);
>
> +int fdt_setup_simplefb_node(void *fdt, int node, u64 base_address, u32 width,
> +                           u32 height, u32 stride, const char *format);
> +
>  #endif /* ifdef CONFIG_OF_LIBFDT */
>
>  #ifdef USE_HOSTCC
> --
> 2.1.0

Regards,
Simon

[U-Boot] [PATCH v5 2/3] common/lcd: Make lcd_dt_simplefb_configure_node use fdt_setup_simplefb_node

[Simon Glass]

On 19 November 2014 13:31, Hans de Goede <hdegoede@redhat.com> wrote:
> Change lcd_dt_simplefb_configure_node into a wrapper around the new generic
> fdt_setup_simplefb_node helper function.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

[U-Boot] [PATCH v5 1/3] fdt_support: Add a fdt_setup_simplefb_node helper function

[Hans de Goede]

Hi,

On 11/20/2014 06:46 PM, Simon Glass wrote:
> Hi Hans,
> 
> On 19 November 2014 13:31, Hans de Goede <hdegoede@redhat.com> wrote:
>> Add a generic helper to fill and enable simplefb nodes.
>>
>> The first user of this will be the sunxi display code.
>>
>> lcd_dt_simplefb_configure_node is also a good candidate to be converted
>> to use this, but that requires someone to run some tests first, as
>> lcd_dt_simplefb_configure_node does not honor #address-cells and #size-cells,
>> but simply assumes 1 and 1 for both.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Acked-by: Simon Glass <sjg@chromium.org>
> 
> See below if you respin.

We agreed on getting this in via the sunxi custodian tree right ? Then I
can fixup your comments (and re-test things on a sunxi board) before sending a
pull-req.

>> ---
>>  common/fdt_support.c  | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/fdt_support.h |  3 +++
>>  2 files changed, 68 insertions(+)
>>
>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>> index 3f64156..0ffa711 100644
>> --- a/common/fdt_support.c
>> +++ b/common/fdt_support.c
>> @@ -1523,3 +1523,68 @@ int fdt_read_range(void *fdt, int node, int n, uint64_t *child_addr,
>>
>>         return 0;
>>  }
>> +
>> +/**
>> + * fdt_setup_simplefb_node - Fill and enable a simplefb node
>> + *
>> + * @fdt: ptr to device tree
>> + * @node: offset of the simplefb node
>> + * @base_address: framebuffer base address
>> + * @width: width in pixels
>> + * @height: height in pixels
>> + * @stride: bytes per line
>> + * @format: pixel format string
>> + *
>> + * Convenience function to fill and enable a simplefb node.
>> + */
> 
> Better to put the comment in the header IMO.

I followed the example of other functions in fdt_support, there currently are
no doxygen comments in fdt_support.h . Still I can move this over if you want.


> 
>> +int fdt_setup_simplefb_node(void *fdt, int node, u64 base_address, u32 width,
>> +                           u32 height, u32 stride, const char *format)
>> +{
>> +       char name[32];
>> +       fdt32_t cells[4];
>> +       int i, addrc, sizec, ret;
>> +
>> +       of_bus_default_count_cells(fdt, fdt_parent_offset(fdt, node),
>> +                                  &addrc, &sizec);
>> +       i = 0;
>> +       if (addrc == 2)
>> +               cells[i++] = cpu_to_fdt32(base_address >> 32);
>> +       cells[i++] = cpu_to_fdt32(base_address);
>> +       if (sizec == 2)
>> +               cells[i++] = 0;
>> +       cells[i++] = cpu_to_fdt32(height * stride);
>> +
>> +       ret = fdt_setprop(fdt, node, "reg", cells, sizeof(cells[0]) * i);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       snprintf(name, sizeof(name), "framebuffer@%llx", base_address);
>> +       ret = fdt_set_name(fdt, node, name);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       cells[0] = cpu_to_fdt32(width);
>> +       ret = fdt_setprop(fdt, node, "width", cells, sizeof(cells[0]));
> 
> How about fdt_setprop_u32() instead?

Yeah thats much better, will fix.

> 
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       cells[0] = cpu_to_fdt32(height);
>> +       ret = fdt_setprop(fdt, node, "height", cells, sizeof(cells[0]));
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       cells[0] = cpu_to_fdt32(stride);
>> +       ret = fdt_setprop(fdt, node, "stride", cells, sizeof(cells[0]));
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = fdt_setprop_string(fdt, node, "format", format);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = fdt_setprop_string(fdt, node, "status", "okay");
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return 0;
>> +}
>> diff --git a/include/fdt_support.h b/include/fdt_support.h
>> index 55cef94..d5e09e6 100644
>> --- a/include/fdt_support.h
>> +++ b/include/fdt_support.h
>> @@ -147,6 +147,9 @@ void of_bus_default_count_cells(void *blob, int parentoffset,
>>  int ft_verify_fdt(void *fdt);
>>  int arch_fixup_memory_node(void *blob);
>>
>> +int fdt_setup_simplefb_node(void *fdt, int node, u64 base_address, u32 width,
>> +                           u32 height, u32 stride, const char *format);
>> +
>>  #endif /* ifdef CONFIG_OF_LIBFDT */
>>
>>  #ifdef USE_HOSTCC
>> --
>> 2.1.0
> 
> Regards,
> Simon
> 

Regards,

Hans

[U-Boot] [PATCH v5 3/3] sunxi: video: Add simplefb support

[Hans de Goede]

Hi,

On 11/20/2014 06:40 PM, Simon Glass wrote:
> Hi Hans,
> 
> On 19 November 2014 13:32, Hans de Goede <hdegoede@redhat.com> wrote:
>> From: Luc Verhaegen <libv@skynet.be>
>>
>> Add simplefb support, note this depends on the kernel having support for
>> the clocks property which has recently been added to the simplefb devicetree
>> binding.
>>
>> Signed-off-by: Luc Verhaegen <libv@skynet.be>
>> [hdegoede at redhat.com: Use pre-populated simplefb node under /chosen as
>>  disussed on the devicetree list]
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  arch/arm/include/asm/arch-sunxi/display.h |  4 +++
>>  board/sunxi/board.c                       | 11 ++++++++
>>  drivers/video/sunxi_display.c             | 42 +++++++++++++++++++++++++++++++
>>  include/configs/sunxi-common.h            |  8 ++++++
>>  4 files changed, 65 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/arch-sunxi/display.h b/arch/arm/include/asm/arch-sunxi/display.h
>> index 8d80ceb..c9d81ba 100644
>> --- a/arch/arm/include/asm/arch-sunxi/display.h
>> +++ b/arch/arm/include/asm/arch-sunxi/display.h
>> @@ -195,4 +195,8 @@ struct sunxi_hdmi_reg {
>>  #define SUNXI_HDMI_PLL_DBG0_PLL3               (0 << 21)
>>  #define SUNXI_HDMI_PLL_DBG0_PLL7               (1 << 21)
>>
>> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB
>> +int sunxi_simplefb_setup(void *blob);
>> +#endif
> 
> Do you really need this #ifdef?

No, I'm not the original author of this patch and I inherited this from the
original author, I'm not a big fan either, I'll drop it.

> 
>> +
>>  #endif /* _SUNXI_DISPLAY_H */
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index e6ec5b8..d4530e8 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -24,6 +24,7 @@
>>  #endif
>>  #include <asm/arch/clock.h>
>>  #include <asm/arch/cpu.h>
>> +#include <asm/arch/display.h>
>>  #include <asm/arch/dram.h>
>>  #include <asm/arch/gpio.h>
>>  #include <asm/arch/mmc.h>
>> @@ -237,3 +238,13 @@ int misc_init_r(void)
>>         return 0;
>>  }
>>  #endif
>> +
>> +#ifdef CONFIG_OF_BOARD_SETUP
>> +void
>> +ft_board_setup(void *blob, bd_t *bd)
> 
> void on same line?

Another inherited thing, will fixup (everywhere).

> Also can you make it return int. You could look at
> u-boot-fdt/next (I am waiting on resolving one patch before sending a
> pull).

Ack, will also fix.

> 
>> +{
>> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB
>> +       sunxi_simplefb_setup(blob);
>> +#endif
>> +}
>> +#endif /* CONFIG_OF_BOARD_SETUP */
>> diff --git a/drivers/video/sunxi_display.c b/drivers/video/sunxi_display.c
>> index 3f46c31..2f3f5db 100644
>> --- a/drivers/video/sunxi_display.c
>> +++ b/drivers/video/sunxi_display.c
>> @@ -13,6 +13,8 @@
>>  #include <asm/arch/display.h>
>>  #include <asm/global_data.h>
>>  #include <asm/io.h>
>> +#include <fdtdec.h>
>> +#include <fdt_support.h>
>>  #include <linux/fb.h>
>>  #include <video_fb.h>
>>
>> @@ -416,3 +418,43 @@ video_hw_init(void)
>>
>>         return graphic_device;
>>  }
>> +
>> +/*
>> + * Simplefb support.
>> + */
>> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
>> +int
>> +sunxi_simplefb_setup(void *blob)
> 
> int on same line
> 
>> +{
>> +       static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
>> +       int offset, ret;
>> +
>> +       if (!sunxi_display.enabled)
>> +               return 0;
>> +
>> +       /* Find a framebuffer node, with pipeline == "de_be0-lcd0-hdmi" */
>> +       offset = fdt_node_offset_by_compatible(blob, -1,
>> +                                              "allwinner,simple-framebuffer");
>> +       while (offset >= 0) {
>> +               ret = fdt_find_string(blob, offset, "allwinner,pipeline",
>> +                                     "de_be0-lcd0-hdmi");
>> +               if (ret == 0)
>> +                       break;
>> +               offset = fdt_node_offset_by_compatible(blob, offset,
>> +                                              "allwinner,simple-framebuffer");
>> +       }
> 
> Can you use do...while() to avoid repeating the 'offset =' line?
> 
>> +       if (offset < 0) {
>> +               eprintf("Cannot setup simplefb: node not found\n");
>> +               return 0; /* Keep older kernels working */
>> +       }
>> +
>> +       ret = fdt_setup_simplefb_node(blob, offset, gd->fb_base,
>> +                       graphic_device->winSizeX, graphic_device->winSizeY,
>> +                       graphic_device->winSizeX * graphic_device->gdfBytesPP,
>> +                       "x8r8g8b8");
>> +       if (ret)
>> +               eprintf("Cannot setup simplefb: Error setting properties\n");
>> +
>> +       return ret;
>> +}
>> +#endif /* CONFIG_OF_BOARD_SETUP && CONFIG_VIDEO_DT_SIMPLEFB */
>> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
>> index 900ef52..d5d907b 100644
>> --- a/include/configs/sunxi-common.h
>> +++ b/include/configs/sunxi-common.h
>> @@ -204,6 +204,9 @@
>>   */
>>  #define CONFIG_SUNXI_FB_SIZE (8 << 20)
>>
>> +/* Do we want to initialize a simple FB? */
>> +#define CONFIG_VIDEO_DT_SIMPLEFB
>> +
>>  #define CONFIG_VIDEO_SUNXI
>>
>>  #define CONFIG_CFB_CONSOLE
>> @@ -217,6 +220,11 @@
>>
>>  #define CONFIG_SYS_MEM_TOP_HIDE ((CONFIG_SUNXI_FB_SIZE + 0xFFF) & ~0xFFF)
>>
>> +/* To be able to hook simplefb into dt */
>> +#ifdef CONFIG_VIDEO_DT_SIMPLEFB
>> +#define CONFIG_OF_BOARD_SETUP
>> +#endif
>> +
>>  #endif /* CONFIG_VIDEO */
>>
>>  /* Ethernet support */
>> --
>> 2.1.0
>>
> 
> Regards,
> Simon

Regards,

Hans