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 <[email protected]> >> >> 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
