[U-Boot,1/2] lcd: add functions to setup up simplefb device tree

simple-framebuffer is a new device tree binding that describes a pre-
configured frame-buffer memory region and its format. The Linux kernel
contains a driver that supports this binding. Implement functions to
create a DT node (or fill in an existing node) with parameters that
describe the framebuffer format that U-Boot is using.

This will be immediately used by the Raspberry Pi board in U-Boot, and
likely will be used by the Samsung ARM ChromeBook support soon too. It
could well be used by many other boards (e.g. Tegra boards with built-in
LCD panels, which aren't yet supported by the Linux kernel).

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
 common/lcd.c  |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/lcd.h |    5 ++++
 2 files changed, 92 insertions(+)

Hi Stephen,

On Tue, May 7, 2013 at 10:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> simple-framebuffer is a new device tree binding that describes a pre-
> configured frame-buffer memory region and its format. The Linux kernel
> contains a driver that supports this binding. Implement functions to
> create a DT node (or fill in an existing node) with parameters that
> describe the framebuffer format that U-Boot is using.
>
> This will be immediately used by the Raspberry Pi board in U-Boot, and
> likely will be used by the Samsung ARM ChromeBook support soon too. It
> could well be used by many other boards (e.g. Tegra boards with built-in
> LCD panels, which aren't yet supported by the Linux kernel).
>
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
>  common/lcd.c  |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/lcd.h |    5 ++++
>  2 files changed, 92 insertions(+)

This looks OK - is a better place for it than the common lcd code? I
presume it is here because it accesses panel_info?

Also is there a binding file we can bring in?

>
> diff --git a/common/lcd.c b/common/lcd.c
> index edae835..3a60484 100644
> --- a/common/lcd.c
> +++ b/common/lcd.c
> @@ -57,6 +57,10 @@
>  #include <atmel_lcdc.h>
>  #endif
>
> +#if defined(CONFIG_LCD_DT_SIMPLEFB)
> +#include <libfdt.h>
> +#endif
> +
>  /************************************************************************/
>  /* ** FONT DATA                                                                */
>  /************************************************************************/
> @@ -1182,3 +1186,86 @@ int lcd_get_screen_columns(void)
>  {
>         return CONSOLE_COLS;
>  }
> +
> +#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;
> +       const char format[] =
> +#if LCD_BPP == LCD_COLOR16
> +               "r5g6b5";
> +#else
> +               "";
> +#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)

Can we not automatically find the node and update it?

> +{
> +       const char compat[] = "simple-framebuffer";
> +       const char disabled[] = "disabled";
> +       int off, ret;
> +
> +       off = fdt_add_subnode(blob, 0, "framebuffer");
> +       if (off < 0)
> +               return -1;
> +
> +       ret = fdt_setprop(blob, off, "status", disabled, sizeof(disabled));
> +       if (ret < 0)
> +               return -1;
> +
> +       ret = fdt_setprop(blob, off, "compatible", compat, sizeof(compat));
> +       if (ret < 0)
> +               return -1;
> +
> +       return lcd_dt_simplefb_configure_node(blob, off);
> +}
> +
> +int lcd_dt_simplefb_enable_existing_node(void *blob)
> +{
> +       int off;
> +
> +       off = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer");

Do we ever need to support more than one node, iwc perhaps we want to
pass in the offset? If not, then see above question re searching.

> +       if (off < 0)
> +               return -1;
> +
> +       return lcd_dt_simplefb_configure_node(blob, off);
> +}
> +#endif
> diff --git a/include/lcd.h b/include/lcd.h
> index c6e7fc5..7c5410d 100644
> --- a/include/lcd.h
> +++ b/include/lcd.h
> @@ -324,6 +324,11 @@ void lcd_show_board_info(void);
>  /* Return the size of the LCD frame buffer, and the line length */
>  int lcd_get_size(int *line_length);
>
> +#if defined(CONFIG_LCD_DT_SIMPLEFB)

Probably don't need this #ifdef? It will complicate things if we use
the autoconf series.

> +int lcd_dt_simplefb_add_node(void *blob);
> +int lcd_dt_simplefb_enable_existing_node(void *blob);
> +#endif
> +
>  /************************************************************************/
>  /* ** BITMAP DISPLAY SUPPORT                                           */
>  /************************************************************************/
> --
> 1.7.10.4
>

Regards,
Simon

On 05/08/2013 09:33 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Tue, May 7, 2013 at 10:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> simple-framebuffer is a new device tree binding that describes a pre-
>> configured frame-buffer memory region and its format. The Linux kernel
>> contains a driver that supports this binding. Implement functions to
>> create a DT node (or fill in an existing node) with parameters that
>> describe the framebuffer format that U-Boot is using.
>>
>> This will be immediately used by the Raspberry Pi board in U-Boot, and
>> likely will be used by the Samsung ARM ChromeBook support soon too. It
>> could well be used by many other boards (e.g. Tegra boards with built-in
>> LCD panels, which aren't yet supported by the Linux kernel).

> This looks OK - is a better place for it than the common lcd code? I
> presume it is here because it accesses panel_info?

I believe it really is common code; the DT content this code generates
is defined by a generic binding and isn't SoC-/board-specific. Perhaps a
common DT-related file would be OK too, although as you mention I'm not
sure if any other file would have access to the required LCD variables.

> Also is there a binding file we can bring in?

Yes, there's a simple-framebuffer.txt I could copy from the kernel.

>> +int lcd_dt_simplefb_add_node(void *blob)
> 
> Can we not automatically find the node and update it?

Some DTs might have the node already exist and want to edit it, whereas
others might not contain it at all and need it added. This is rather up
to the author of individual boards' DTs. I wanted to make the code
explicitly select between those two options so that the U-Boot board
author always thought about exactly which behaviour they wanted.

>> +int lcd_dt_simplefb_enable_existing_node(void *blob)
>> +{
>> +       int off;
>> +
>> +       off = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer");
> 
> Do we ever need to support more than one node, iwc perhaps we want to
> pass in the offset? If not, then see above question re searching.

I don't think U-Boot supports multiple panels does it? If the DT
contained multiple nodes to start with, I think they'd have to all be
initially disabled since some firmware would be required to fill in the
fields before they were useful.

As such, finding and editing the first node in all cases seems to make
sense for now. If this ever becomes false, we can add an index parameter
quite easily without significant impact.

>> diff --git a/include/lcd.h b/include/lcd.h

>> +#if defined(CONFIG_LCD_DT_SIMPLEFB)
> 
> Probably don't need this #ifdef? It will complicate things if we use
> the autoconf series.

What's the autoconf series? I did this in order to get compile errors
rather than link errors if the functions were used without being
enabled, but I guess most linkers generate useful error messages so
perhaps this isn't needed.

Hi Stephen,

On Fri, May 10, 2013 at 9:35 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/08/2013 09:33 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Tue, May 7, 2013 at 10:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> simple-framebuffer is a new device tree binding that describes a pre-
>>> configured frame-buffer memory region and its format. The Linux kernel
>>> contains a driver that supports this binding. Implement functions to
>>> create a DT node (or fill in an existing node) with parameters that
>>> describe the framebuffer format that U-Boot is using.
>>>
>>> This will be immediately used by the Raspberry Pi board in U-Boot, and
>>> likely will be used by the Samsung ARM ChromeBook support soon too. It
>>> could well be used by many other boards (e.g. Tegra boards with built-in
>>> LCD panels, which aren't yet supported by the Linux kernel).
>
>> This looks OK - is a better place for it than the common lcd code? I
>> presume it is here because it accesses panel_info?
>
> I believe it really is common code; the DT content this code generates
> is defined by a generic binding and isn't SoC-/board-specific. Perhaps a
> common DT-related file would be OK too, although as you mention I'm not
> sure if any other file would have access to the required LCD variables.
>
>> Also is there a binding file we can bring in?
>
> Yes, there's a simple-framebuffer.txt I could copy from the kernel.

Yes, it looks good.

>
>>> +int lcd_dt_simplefb_add_node(void *blob)
>>
>> Can we not automatically find the node and update it?
>
> Some DTs might have the node already exist and want to edit it, whereas
> others might not contain it at all and need it added. This is rather up
> to the author of individual boards' DTs. I wanted to make the code
> explicitly select between those two options so that the U-Boot board
> author always thought about exactly which behaviour they wanted.

OK.

>
>>> +int lcd_dt_simplefb_enable_existing_node(void *blob)
>>> +{
>>> +       int off;
>>> +
>>> +       off = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer");
>>
>> Do we ever need to support more than one node, iwc perhaps we want to
>> pass in the offset? If not, then see above question re searching.
>
> I don't think U-Boot supports multiple panels does it? If the DT
> contained multiple nodes to start with, I think they'd have to all be
> initially disabled since some firmware would be required to fill in the
> fields before they were useful.
>
> As such, finding and editing the first node in all cases seems to make
> sense for now. If this ever becomes false, we can add an index parameter
> quite easily without significant impact.

Sounds reasonable. I guess U-Boot will support multiple panels once
driver model is fully installed, but for now it does not.

>
>>> diff --git a/include/lcd.h b/include/lcd.h
>
>>> +#if defined(CONFIG_LCD_DT_SIMPLEFB)
>>
>> Probably don't need this #ifdef? It will complicate things if we use
>> the autoconf series.
>
> What's the autoconf series? I did this in order to get compile errors
> rather than link errors if the functions were used without being
> enabled, but I guess most linkers generate useful error messages so
> perhaps this isn't needed.
>

It was posted 26 Feb - first patch is here:
http://patchwork.ozlabs.org/patch/223278/

Also while you are in review mode, I'd appreciate any comment you have
on U-Boot driver model:

http://patchwork.ozlabs.org/patch/242451/

Regards,
Simon