[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/16/2014 12:50 PM, Ian Campbell wrote:
> On Fri, 2014-11-14 at 17:54 +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.
> 
> Link please, Linus's tree[0] doesn't seem to have it yet, I suppose it
> is in some maintainer tree at the moment? It's not even in linux-next
> yet though[1]. Maybe simple-framebuffer.txt isn't the right place to
> look?

Right now it is sitting here, which is the fbdev maintainers official tree:
https://git.kernel.org/cgit/linux/kernel/git/tomba/linux.git/log/?h=for-next

> 
> [0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> [1] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> 
>> +#if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
>> +void
>> +sunxi_simplefb_setup(void *blob)
>> +{
>> +	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
>> +	const char *node = "/chosen/framebuffer0-hdmi";
>> +	const char *format = "x8r8g8b8";
> 
> The bindings doc currently says:
>         
>         - format: The format of the framebuffer surface. Valid values are:
>           - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>           - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>         
> Perhaps x8r8g8b8 is defined in the updated version?

Erm, no, I don't think anyone has actually bothered to keep the list in the
binding up2date with what the kernel actually supports, x8r8g8b8 has been
supported in the kernel before sunxi + simplefb every became a topic.

> 
>> +	const char *okay = "okay";
>> +	char name[32];
>> +	fdt32_t cells[2];
>> +	int offset, stride, ret;
>> +
>> +	if (!sunxi_display.enabled)
>> +		return;
>> +
>> +	offset = fdt_path_offset(blob, node);
> 
> I think you should use fdt_node_offset_by_compatible here instead of
> hardcoding a path.

Hardcoding a path is deliberate. I don't know if you've read the
previous u-boot code for this, but it did a lot of dt parsing to
find clocks and add phandles to them, the way we eventually settled
on when discussing this is for the dts to contain pre-populated simplefb
nodes which u-boot just needs to fill with the mode info and enable, this
way as we add support for more clocks (like the module clocks for the
various display pipeline blocks), we don't need to update u-boot in lock-step,
we just add the clocks to the simplefb node in the dts file when they get
added to the dts file in the first place. See the updated bindings.

> common/lcd.c does it this way too, it also doesn't
> appear to use a node under /chosen. Perhaps this is changed in the more
> uptodate binding which I've not seen yet.

The use of /chosen is part of the updated bindings, which were discussed
in length on various lists, and then eventually I spend 2 days online with
Grant Likely in #devicetree to hash things out.

>> +	cells[0] = cpu_to_fdt32(gd->fb_base);
>> +	cells[1] = cpu_to_fdt32(CONFIG_SUNXI_FB_SIZE);
>> +	ret = fdt_setprop(blob, offset, "reg", cells, sizeof(cells[0]) * 2);
> 
> What arranges that #address-cells and #size-cells are both 1 at this
> point?

The pre-filled simplefb node.

> Does u-boot not have a helper to setup a cells array including
> looking those up?

Good question.

/me looks

Doesn't look like it, what we've available basically is a bare libfdt,
and it does not look like that can do this.

> Also the bindings doc seems to imply that size should be the actual
> configured size of the video ram region ("(1600 * 1200 * 2)") rather
> than the size of the reserved memory region. Maybe it's not important.

Heh, it is not important really / does not matter either way.

I actually tried fixing the example, in the bindings but people found it
more clear as it is.

> 
>> +	ret = fdt_setprop(blob, offset, "format", format, strlen(format) + 1);
>> +	if (ret < 0)
>> +		goto error;
>> +
>> +	ret = fdt_setprop(blob, offset, "status", okay, strlen(okay) + 1);
> 
> You can use fdt_setprop_string for these two, I think.

Yes, good one, will fix in my local tree.

Regards,

Hans