| Message ID | 1415984088-6800-6-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, 14 Nov 2014 17:54:47 +0100 Hans de Goede <hdegoede@redhat.com> wrote: ... > diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h > index 532fdb7..d7d8571 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 > @@ -219,6 +222,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 defining CONFIG_OF_BOARD_SETUP should be independent of defining CONFIG_VIDEO or CONFIG_VIDEO_DT_SIMPLEFB. DT fixup by board setup code is often done i.e. for auto-detected memory size, adjusting different node properties or adding/deleting whole nodes. If someone defines CONFIG_OF_BOARD_SETUP for these purposes and also defines CONFIG_VIDEO_DT_SIMPLEFB, the "already defined" warnings will appear. This can be left as is for now, but should be addressed in the long term. Thanks, Anatolij
Hi, On 11/14/2014 11:22 PM, Anatolij Gustschin wrote: > On Fri, 14 Nov 2014 17:54:47 +0100 > Hans de Goede <hdegoede@redhat.com> wrote: > ... >> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h >> index 532fdb7..d7d8571 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 >> @@ -219,6 +222,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 > > defining CONFIG_OF_BOARD_SETUP should be independent of defining > CONFIG_VIDEO or CONFIG_VIDEO_DT_SIMPLEFB. DT fixup by board setup > code is often done i.e. for auto-detected memory size, adjusting > different node properties or adding/deleting whole nodes. If > someone defines CONFIG_OF_BOARD_SETUP for these purposes and also > defines CONFIG_VIDEO_DT_SIMPLEFB, the "already defined" warnings > will appear. This can be left as is for now, but should be addressed > in the long term. Currently the only thing we need CONFIG_OF_BOARD_SETUP for on sunxi is CONFIG_VIDEO_DT_SIMPLEFB, so for now this makes sense, but your 100% right that long term we probably want to do this differently. Regards, Hans
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? [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? > + 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. 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. > + 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? Does u-boot not have a helper to setup a cells array including looking those up? 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. > + 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. Ian.
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
devicetree@, comments on the requirement that a node have a specific path welcomed. On Sun, 2014-11-16 at 14:52 +0100, Hans de Goede wrote: > 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. That's a shame, OS authors shouldn't really be expected to scrobble about in Linux source to figure out what a binding is. > >> + 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. AFAICT this in no way invalidates what I suggested. There's no reason why the need to populate/tweak a pre-existing node should have anything to do with where the node is in the device-tree. My suggestion literally amounts to: - offset = fdt_path_offset(blob, node); + offset = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer"); Nothing else in the code changes. You can still add the required details to the prepopulated node, no matter where it lives. I notice that v1 of these bindings was posted mid last week and were still being discussed (at v5) on Friday, where there was active discussion by DT maintainers (Grant, who hasn't yet acked it AFAICT). So why is this in the fb tree already? It seems to me that this binding is not yet at the point where u-boot should be basing implementation on it. I'm sorry if this means this feature misses the current u-boot merge window, but there will be another soon. (didn't this one close on the 8th anyway?) > > 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. I think you should use fdt_address_cells and fdt_size_cells to ensure that it really is the case that they are as you expect. Or even better use them to just DTRT regardless of what the pre-filled node's parent says. I'm not really happy with the implication that the DT has to be the exact one provided in the Linux source tree, and with the level of coupling which is implied by this patch. I should be able to write my own DT for a device so long as it follows the bindings. e.g. if *BSD supports a board (they often seem to have their own DT files) then I should be able to boot that from u-boot too, so long as all three follow the bindings. If the u-boot code is going to put additional constraints on things over and above the bindings (e.g. requiring that node to be under a particular #{address/size}-cells regimen and IMHO the specific path comes under this too despite what recent bindings updates attempt to say) then that needs to be clearly documented somewhere, and even that would make me slightly sad given how easy it would be to just make it work following the bindings in the normal way. Ian.
Hi, On 11/16/2014 03:38 PM, Ian Campbell wrote: > devicetree@, comments on the requirement that a node have a specific > path welcomed. > > On Sun, 2014-11-16 at 14:52 +0100, Hans de Goede wrote: >> 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. > > That's a shame, OS authors shouldn't really be expected to scrobble > about in Linux source to figure out what a binding is. > >>>> + 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. > > AFAICT this in no way invalidates what I suggested. There's no reason > why the need to populate/tweak a pre-existing node should have anything > to do with where the node is in the device-tree. Right, I forgot to add one important bit to my explanation, sorry, if you look at the binding then it says that the name should be suffixed with the output type, so currently the sunxi u-boot code will look for /chosen/framebuffer0-hdmi. Which will e.g. also happen on any A10 based tablets which typically have both hdmi out and an lcd screen, in the future I hope we will also get lcd support in u-boot, and then logically on tablets the lcd screen would take precedence, so then unless overriden through some config mechanism u-boot will chose to use the lcd, and will look for /chosen/framebuffer0-lcd which has a different set of clocks then /chosen/framebuffer0-hdmi. > > My suggestion literally amounts to: > - offset = fdt_path_offset(blob, node); > + offset = fdt_node_offset_by_compatible(blob, -1, "simple-framebuffer"); > > Nothing else in the code changes. You can still add the required details > to the prepopulated node, no matter where it lives. See above, we need to pick the right pre-populated node for the output type, this esp. becomes important on boxes like the mele a1000 and friends where there are 3 outputs to divide over 2 display pipelines (so we can only lit up 2 of the outputs). > I notice that v1 of these bindings was posted mid last week and were > still being discussed (at v5) on Friday, where there was active > discussion by DT maintainers (Grant, who hasn't yet acked it AFAICT). So > why is this in the fb tree already? Grant only had some minor tweaks for v4, so Tomi has merged it based on that, with a request to do any fixups as follow up patches. As for the timeline, even though some of the new binding stuff was only finalized last week, the whole discussion about this has been going one for months. > It seems to me that this binding is not yet at the point where u-boot > should be basing implementation on it. I'm sorry if this means this > feature misses the current u-boot merge window, but there will be > another soon. (didn't this one close on the 8th anyway?) The devicetree bindings have been going on for so long, that this would be the 2nd merge window this feature misses, Luc's original version was posted before the v2014.10 merge window. AFAIK Grant agrees with v5 as merged by Timo, and it would be really nice to move forward with this rather then to start yet another round of never ending discussions on this. >>> 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. > > I think you should use fdt_address_cells and fdt_size_cells to ensure > that it really is the case that they are as you expect. Or even better > use them to just DTRT regardless of what the pre-filled node's parent > says. Verifying things to be as expected seem sensible, DTRT regardless seems impossible, since u-boot will have no idea what needs to be in there if either size-cells or address-cells != 1. > I'm not really happy with the implication that the DT has to be the > exact one provided in the Linux source tree, and with the level of > coupling which is implied by this patch. Some level of coupling is always needed that is what the bindings docs are for. Specifying how the bindings should look like from a firmware pov is no different then specifying how they should look like from an os pov. > I should be able to write my own DT for a device so long as it follows > the bindings. e.g. if *BSD supports a board (they often seem to have > their own DT files) then I should be able to boot that from u-boot too, > so long as all three follow the bindings. Ack, and I don't see how what we're doing here breaks that. > If the u-boot code is going to put additional constraints on things over > and above the bindings (e.g. requiring that node to be under a > particular #{address/size}-cells regimen Bindings always specify a particular address / size cells regimen, otherwise it is not well defined how to interpret them. If there are extra address / size fields those need to have a defined meaning, so an #{address/size}-cells regimen is always implied by the bindings. In this case the address / size cells comes from how we specify any memory address on sunxi which is 1 / 1. > and IMHO the specific path > comes under this too despite what recent bindings updates attempt to > say) then that needs to be clearly documented somewhere, and even that > would make me slightly sad given how easy it would be to just make it > work following the bindings in the normal way. The path requirement comes from 2 things: 1) u-boot needs to be able to find which node belongs to which display pipeline 2) u-boot needs to be able to find the right node for the output chosen, were there may be multiple output options u-boot can chose for a single display pipeline So just searching for the node by compatible will not work. Regards, Hans
On Sun, 2014-11-16 at 16:11 +0100, Hans de Goede wrote: > Hi, > > On 11/16/2014 03:38 PM, Ian Campbell wrote: > > devicetree@, comments on the requirement that a node have a specific > > path welcomed. > > > > On Sun, 2014-11-16 at 14:52 +0100, Hans de Goede wrote: > >> 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. > > > > That's a shame, OS authors shouldn't really be expected to scrobble > > about in Linux source to figure out what a binding is. > > > >>>> + 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. > > > > AFAICT this in no way invalidates what I suggested. There's no reason > > why the need to populate/tweak a pre-existing node should have anything > > to do with where the node is in the device-tree. > > Right, I forgot to add one important bit to my explanation, sorry, if > you look at the binding then it says that the name should be suffixed > with the output type, so currently the sunxi u-boot code will look for > /chosen/framebuffer0-hdmi. Which will e.g. also happen on any A10 > based tablets which typically have both hdmi out and an lcd screen, > in the future I hope we will also get lcd support in u-boot, and then > logically on tablets the lcd screen would take precedence, so then > unless overriden through some config mechanism u-boot will chose > to use the lcd, and will look for /chosen/framebuffer0-lcd which has > a different set of clocks then /chosen/framebuffer0-hdmi. This sounds like a use for: compatible = "simple-framebuffer,hdmi", "simple-framebuffer" or some such, that's almost exactly what multiople compatible strings are for. Relying on specific naming and/or path is rather unconventional. > >>> 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. > > > > I think you should use fdt_address_cells and fdt_size_cells to ensure > > that it really is the case that they are as you expect. Or even better > > use them to just DTRT regardless of what the pre-filled node's parent > > says. > > Verifying things to be as expected seem sensible, DTRT regardless seems > impossible, since u-boot will have no idea what needs to be in there > if either size-cells or address-cells != 1. I think you have misunderstood what #size-cells / #address-cells are. They tell you the number of cells which each address/size entry uses, they do not tell you anything about the number of address/size combinations in the reg property (i.e. the number of registers, which is indeed a property of the binding). IOW for a single region at 0xabcdef12 size 0x00003456 then: #address-cells=1,#size-cells=1 => regs = <0xabcdef12 0x00003456> #address-cells=2,#size-cells=1 => regs = <0 0xabcdef12 0x00003456> #address-cells=1,#size-cells=2 => regs = <0xabcdef12 0 0x00003456> #address-cells=2,#size-cells=1 => regs = <0 0xabcdef12 0 0x00003456> The difference is between between storing the number as a u32 or a u64 (encoded as two cells), it doesn't change the meaning of anything. > > I'm not really happy with the implication that the DT has to be the > > exact one provided in the Linux source tree, and with the level of > > coupling which is implied by this patch. > > Some level of coupling is always needed that is what the bindings docs > are for. Specifying how the bindings should look like from a firmware pov > is no different then specifying how they should look like from an os pov. This code is making assumptions over and above the bindings, and the bindings themselves do not seem to be fully agreed upon yet (and contain some unconventional constructs). > The devicetree bindings have been going on for so long, that this > would be the 2nd merge window this feature misses, Luc's original > version was posted before the v2014.10 merge window. I'm afraid I don't agree that just because it has taken a long time to get something right we should commit to it before it is ready, especially when it is an ABI, which is what a DT binding is. If this scheme was acked by a DT maintainer then I'd be fine with it, but so far it hasn't been, at least not publicly in the threads I've looked at. > AFAIK Grant agrees with v5 AFAIK Grant hasn't actually said that. If he does ack it (or if someone points me to the correct mail) then I have no further objections. > > I should be able to write my own DT for a device so long as it follows > > the bindings. e.g. if *BSD supports a board (they often seem to have > > their own DT files) then I should be able to boot that from u-boot too, > > so long as all three follow the bindings. > > Ack, and I don't see how what we're doing here breaks that. An author would need to know this out of band requirement that #*-cells are required to be 1, which is an unusual requirement to start with. > > If the u-boot code is going to put additional constraints on things over > > and above the bindings (e.g. requiring that node to be under a > > particular #{address/size}-cells regimen > > Bindings always specify a particular address / size cells regimen, otherwise > it is not well defined how to interpret them. No so. Almost no bindings specify these, they are a common device-tree concept and for a given node are defined by that node's parent (or grandparent, etc), not by the node's bindings. This is all part of the way buses are represented in DT. In fact it's a bit odd to have a reg property under /chosen at all, since it doesn't really fit in with the bus structure. I've done something similar in some bindings I've authored[0], but AIUI I got that wrong and really should have used a set of non-reg properties with a single value so there was no need to parse using #*-cells (cf the initrd-start + initrd-end properties under /chosen). Sadly DT is an ABI, so for my bindings I'm kind of stuck with it for the foreseeable future. [0] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=08ed7751859dbe2d2c32d86d7df371057d7b45a4;hb=HEAD > If there are extra address / > size fields those need to have a defined meaning, so an #{address/size}-cells regimen > is always implied by the bindings. > > In this case the address / size cells comes from how we specify any memory > address on sunxi which is 1 / 1. That just happens to be what the dt's in the Linux source have written into them, there is nothing fundamental which requires this and you could perfectly validly author a sunxi DT which uses 2/2 or 2/1 etc. > > and IMHO the specific path > > comes under this too despite what recent bindings updates attempt to > > say) then that needs to be clearly documented somewhere, and even that > > would make me slightly sad given how easy it would be to just make it > > work following the bindings in the normal way. > > The path requirement comes from 2 things: > > 1) u-boot needs to be able to find which node belongs to which display pipeline > 2) u-boot needs to be able to find the right node for the output chosen, were > there may be multiple output options u-boot can chose for a single display > pipeline > > So just searching for the node by compatible will not work. It would work if you used compatible how it was intended to be used. Ian.
On Sun, 2014-11-16 at 16:11 +0000, Ian Campbell wrote: > > > AFAIK Grant agrees with v5 > > AFAIK Grant hasn't actually said that. If he does ack it (or if > someone > points me to the correct mail) then I have no further objections. I finally found <CACxGe6ssdeOnQnem1012h_jFK1wWOD_WwosjFgiAcgBoqyDt9A@mail.gmail.com> which for some reason isn't in my devicetree@ folder nor in any of the archives online I looked at earlier (I could only find it by message-id now I know it), but it was in my sunxi folder. So with Grant and Rob having accepted them you can consider my concerns with the bindings alleviated. Ian.
Hi, On 11/16/2014 06:19 PM, Ian Campbell wrote: > On Sun, 2014-11-16 at 16:11 +0000, Ian Campbell wrote: >> >>> AFAIK Grant agrees with v5 >> >> AFAIK Grant hasn't actually said that. If he does ack it (or if >> someone >> points me to the correct mail) then I have no further objections. > > I finally found > <CACxGe6ssdeOnQnem1012h_jFK1wWOD_WwosjFgiAcgBoqyDt9A@mail.gmail.com> > which for some reason isn't in my devicetree@ folder nor in any of the > archives online I looked at earlier (I could only find it by message-id > now I know it), but it was in my sunxi folder. > > So with Grant and Rob having accepted them you can consider my concerns > with the bindings alleviated. Thanks! I was about to reply to your earlier mail with lets wait for Grant to get online, and see what he has to say, but this works too. And thanks for explaining the address and size cells thing better, I did know that for address and size #cells determines u32 vs u64, I'm used to gpio-s / clock-s where extra cells are an extra parameter, hence my confusion. I'll do a v3 of the simplefb patch which takes the address and size #cells into account. Regards, Hans
On Sun, Nov 16, 2014 at 4:11 PM, Ian Campbell <ijc@hellion.org.uk> wrote: > On Sun, 2014-11-16 at 16:11 +0100, Hans de Goede wrote: >> On 11/16/2014 03:38 PM, Ian Campbell wrote: >> > On Sun, 2014-11-16 at 14:52 +0100, Hans de Goede wrote: >> >> 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. >> > >> > AFAICT this in no way invalidates what I suggested. There's no reason >> > why the need to populate/tweak a pre-existing node should have anything >> > to do with where the node is in the device-tree. >> >> Right, I forgot to add one important bit to my explanation, sorry, if >> you look at the binding then it says that the name should be suffixed >> with the output type, so currently the sunxi u-boot code will look for >> /chosen/framebuffer0-hdmi. Which will e.g. also happen on any A10 >> based tablets which typically have both hdmi out and an lcd screen, >> in the future I hope we will also get lcd support in u-boot, and then >> logically on tablets the lcd screen would take precedence, so then >> unless overriden through some config mechanism u-boot will chose >> to use the lcd, and will look for /chosen/framebuffer0-lcd which has >> a different set of clocks then /chosen/framebuffer0-hdmi. > > This sounds like a use for: > compatible = "simple-framebuffer,hdmi", "simple-framebuffer" > or some such, that's almost exactly what multiople compatible strings > are for. Relying on specific naming and/or path is rather > unconventional. Indeed, for a long time now finding nodes by path has been strongly discouraged. It makes it hard to move nodes around when needed. I'm not going to make a big deal about it because it doesn't affect the OS interface, but Ian's suggestion is sane. simple-framebuffer is enough to identify the binding, but you can use additional strings to identify the specific hardware interface that U-Boot can use to locate the node. ie. sunxi,framebuffer-hdml or some such. You can never be sure when someone will produce a board that messes with your naming assumptions. What if is suddenly needs to be named "framebuffer1-hdmi" because they added and FPGA that they want as framebuffer0? (Yes, I'm making stuff up, but I have to think about the corner cases) >> The devicetree bindings have been going on for so long, that this >> would be the 2nd merge window this feature misses, Luc's original >> version was posted before the v2014.10 merge window. > > I'm afraid I don't agree that just because it has taken a long time to > get something right we should commit to it before it is ready, > especially when it is an ABI, which is what a DT binding is. > > If this scheme was acked by a DT maintainer then I'd be fine with it, > but so far it hasn't been, at least not publicly in the threads I've > looked at. I /DO/ want comments though. Putting the node in /chosen is unconventional. I want to hear if anyone has a good reason why the framebuffers shouldn't be placed into /chosen. >> AFAIK Grant agrees with v5 > > AFAIK Grant hasn't actually said that. If he does ack it (or if someone > points me to the correct mail) then I have no further objections. My word also isn't gospel. On controversial stuff I want to have consensus. For the clock patches and had a long conversation with Rob to make sure we were both in agreement before giving my final ack. > In fact it's a bit odd to have a reg property under /chosen at all, > since it doesn't really fit in with the bus structure. I've done > something similar in some bindings I've authored[0], but AIUI I got that > wrong and really should have used a set of non-reg properties with a > single value so there was no need to parse using #*-cells (cf the > initrd-start + initrd-end properties under /chosen). Sadly DT is an ABI, > so for my bindings I'm kind of stuck with it for the foreseeable future. > > [0] > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=08ed7751859dbe2d2c32d86d7df371057d7b45a4;hb=HEAD Ironic isn't it that I though of that as precedence when I suggested /chosen! :-) I actually don't have a problem with it. We do need a way to specify runtime memory usage, and /chosen is as good a place as any, particularly when it represents things that won't necessarily be relevant on kexec or dom0 boot. The other options are under either the /memory or the /reserved-memory tree. Rob and I talked about /reserved-memory quite a lot. We could put all the framebuffer details into the memory node that reserves the framebuffer. However, I don't like that idea because it kind of makes assumptions about how the framebuffer will be located inside the memory region and doesn't allow for multiple framebuffers within a single region. g.
Hi, On 11/17/2014 10:58 AM, Grant Likely wrote: > On Sun, Nov 16, 2014 at 4:11 PM, Ian Campbell <ijc@hellion.org.uk> wrote: >> On Sun, 2014-11-16 at 16:11 +0100, Hans de Goede wrote: <snip> >>> Right, I forgot to add one important bit to my explanation, sorry, if >>> you look at the binding then it says that the name should be suffixed >>> with the output type, so currently the sunxi u-boot code will look for >>> /chosen/framebuffer0-hdmi. Which will e.g. also happen on any A10 >>> based tablets which typically have both hdmi out and an lcd screen, >>> in the future I hope we will also get lcd support in u-boot, and then >>> logically on tablets the lcd screen would take precedence, so then >>> unless overriden through some config mechanism u-boot will chose >>> to use the lcd, and will look for /chosen/framebuffer0-lcd which has >>> a different set of clocks then /chosen/framebuffer0-hdmi. >> >> This sounds like a use for: >> compatible = "simple-framebuffer,hdmi", "simple-framebuffer" >> or some such, that's almost exactly what multiople compatible strings >> are for. Relying on specific naming and/or path is rather >> unconventional. > > Indeed, for a long time now finding nodes by path has been strongly > discouraged. It makes it hard to move nodes around when needed. I'm > not going to make a big deal about it because it doesn't affect the OS > interface, but Ian's suggestion is sane. simple-framebuffer is enough > to identify the binding, but you can use additional strings to > identify the specific hardware interface that U-Boot can use to locate > the node. ie. sunxi,framebuffer-hdml or some such. You can never be > sure when someone will produce a board that messes with your naming > assumptions. What if is suddenly needs to be named "framebuffer1-hdmi" > because they added and FPGA that they want as framebuffer0? (Yes, I'm > making stuff up, but I have to think about the corner cases) I like the suggestion of using a compatible of : "sunxi,framebuffer-hdmi" although it will need to be : "sunxi,framebuffer0-hdmi" as u-boot needs to know which simplefb node to use for which display pipeline (so that the node has the right clocks and whats not). And I think it makes sense to put simple in there as it is a pre-populated simplefb node , so then we get: "sunxi,simple-framebuffer0-hdmi" And we can drop the unconventional lookup by path. I'll go on irc now, and join #devicetree, Ian, maybe you can join us there to and we can hash out something we can all agree on ? And then I'll do yet another set of patches (to apply on top on the kernel side, and a new version of the u-boot patch), and we'll hopefully end up with something which makes us all happy <snip> >> In fact it's a bit odd to have a reg property under /chosen at all, >> since it doesn't really fit in with the bus structure. I've done >> something similar in some bindings I've authored[0], but AIUI I got that >> wrong and really should have used a set of non-reg properties with a >> single value so there was no need to parse using #*-cells (cf the >> initrd-start + initrd-end properties under /chosen). Sadly DT is an ABI, >> so for my bindings I'm kind of stuck with it for the foreseeable future. >> >> [0] >> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=08ed7751859dbe2d2c32d86d7df371057d7b45a4;hb=HEAD > > Ironic isn't it that I though of that as precedence when I suggested > /chosen! :-) > > I actually don't have a problem with it. We do need a way to specify > runtime memory usage, and /chosen is as good a place as any, > particularly when it represents things that won't necessarily be > relevant on kexec or dom0 boot. > > The other options are under either the /memory or the /reserved-memory > tree. Rob and I talked about /reserved-memory quite a lot. We could > put all the framebuffer details into the memory node that reserves the > framebuffer. However, I don't like that idea because it kind of makes > assumptions about how the framebuffer will be located inside the > memory region and doesn't allow for multiple framebuffers within a > single region. I'm not a devicetree expert, but /chosen already came to my mind even before Grant suggested it, /chosen seems the right place for this to me. Note that once we drop the path based lookup the location can always be changed later (to some degree, it still needs to be in a place where the kernel will bind to it). Regards, Hans
On Mon, 2014-11-17 at 09:58 +0000, Grant Likely wrote: > I /DO/ want comments though. Putting the node in /chosen is > unconventional. I want to hear if anyone has a good reason why the > framebuffers shouldn't be placed into /chosen. I don't think putting it under /chosen is a problem at all. THe semantics of some of hte convention properties are a bit odd under there, but that's not insurmountable. > >> AFAIK Grant agrees with v5 > > > > AFAIK Grant hasn't actually said that. If he does ack it (or if someone > > points me to the correct mail) then I have no further objections. > > My word also isn't gospel. I suppose I should have said something like "I trust Grant's judgement more than my own on things relating to DT" ;-). > On controversial stuff I want to have > consensus. For the clock patches and had a long conversation with Rob > to make sure we were both in agreement before giving my final ack. > > > In fact it's a bit odd to have a reg property under /chosen at all, > > since it doesn't really fit in with the bus structure. I've done > > something similar in some bindings I've authored[0], but AIUI I got that > > wrong and really should have used a set of non-reg properties with a > > single value so there was no need to parse using #*-cells (cf the > > initrd-start + initrd-end properties under /chosen). Sadly DT is an ABI, > > so for my bindings I'm kind of stuck with it for the foreseeable future. > > > > [0] > > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=08ed7751859dbe2d2c32d86d7df371057d7b45a4;hb=HEAD > > Ironic isn't it that I though of that as precedence when I suggested > /chosen! :-) :-) > I actually don't have a problem with it. We do need a way to specify > runtime memory usage, and /chosen is as good a place as any, > particularly when it represents things that won't necessarily be > relevant on kexec or dom0 boot. The main issue which was explained to me with my Xen bindings was that reg = <> isn't all that meaningful under /chosen because it doesn't fit into the bus structure, so the #address-/size-cells stuff gets a bit strange. It's probably tolerable for things which are strictly physical RAM addresses (as opposed to mmio) since RAM isn't typically behind a visible bus. The scheme used for initrds sidesteps all those issues by using separate (multicellular) properties for the start and end regions and not using reg=<> and therefore naturally breaking the expected semantic link with bus topology which reg implies etc. > The other options are under either the /memory or the /reserved-memory > tree. Rob and I talked about /reserved-memory quite a lot. We could > put all the framebuffer details into the memory node that reserves the > framebuffer. However, I don't like that idea because it kind of makes > assumptions about how the framebuffer will be located inside the > memory region and doesn't allow for multiple framebuffers within a > single region. Yes, that sounds strictly worse than the current solution to me. Ian.
On Mon, Nov 17, 2014 at 10:14 AM, Ian Campbell <ijc@hellion.org.uk> wrote: > On Mon, 2014-11-17 at 09:58 +0000, Grant Likely wrote: >> I /DO/ want comments though. Putting the node in /chosen is >> unconventional. I want to hear if anyone has a good reason why the >> framebuffers shouldn't be placed into /chosen. > > I don't think putting it under /chosen is a problem at all. THe > semantics of some of hte convention properties are a bit odd under > there, but that's not insurmountable. > >> >> AFAIK Grant agrees with v5 >> > >> > AFAIK Grant hasn't actually said that. If he does ack it (or if someone >> > points me to the correct mail) then I have no further objections. >> >> My word also isn't gospel. > > I suppose I should have said something like "I trust Grant's judgement > more than my own on things relating to DT" ;-). > >> On controversial stuff I want to have >> consensus. For the clock patches and had a long conversation with Rob >> to make sure we were both in agreement before giving my final ack. >> >> > In fact it's a bit odd to have a reg property under /chosen at all, >> > since it doesn't really fit in with the bus structure. I've done >> > something similar in some bindings I've authored[0], but AIUI I got that >> > wrong and really should have used a set of non-reg properties with a >> > single value so there was no need to parse using #*-cells (cf the >> > initrd-start + initrd-end properties under /chosen). Sadly DT is an ABI, >> > so for my bindings I'm kind of stuck with it for the foreseeable future. >> > >> > [0] >> > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=08ed7751859dbe2d2c32d86d7df371057d7b45a4;hb=HEAD >> >> Ironic isn't it that I though of that as precedence when I suggested >> /chosen! :-) > > :-) > >> I actually don't have a problem with it. We do need a way to specify >> runtime memory usage, and /chosen is as good a place as any, >> particularly when it represents things that won't necessarily be >> relevant on kexec or dom0 boot. > > The main issue which was explained to me with my Xen bindings was that > reg = <> isn't all that meaningful under /chosen because it doesn't fit > into the bus structure, so the #address-/size-cells stuff gets a bit > strange. It's probably tolerable for things which are strictly physical > RAM addresses (as opposed to mmio) since RAM isn't typically behind a > visible bus. > > The scheme used for initrds sidesteps all those issues by using separate > (multicellular) properties for the start and end regions and not using > reg=<> and therefore naturally breaking the expected semantic link with > bus topology which reg implies etc. I quite dislike the initrd multi-property approach. It has to be parsed in a completely separate way. >> The other options are under either the /memory or the /reserved-memory >> tree. Rob and I talked about /reserved-memory quite a lot. We could >> put all the framebuffer details into the memory node that reserves the >> framebuffer. However, I don't like that idea because it kind of makes >> assumptions about how the framebuffer will be located inside the >> memory region and doesn't allow for multiple framebuffers within a >> single region. > > Yes, that sounds strictly worse than the current solution to me. Besides, the simple-framebuffer binding could easily be extended to allow a reserved-memory reference instead of a reg property if/when that becomes a better option for a specific platform. g.
diff --git a/arch/arm/include/asm/arch-sunxi/display.h b/arch/arm/include/asm/arch-sunxi/display.h index 8ac7d1b..e38a401 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 +void 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 1058771..a47f575 100644 --- a/drivers/video/sunxi_display.c +++ b/drivers/video/sunxi_display.c @@ -13,6 +13,7 @@ #include <asm/arch/display.h> #include <asm/global_data.h> #include <asm/io.h> +#include <fdtdec.h> #include <linux/fb.h> #include <video_fb.h> @@ -416,3 +417,68 @@ video_hw_init(void) return graphic_device; } + +/* + * Simplefb support. + */ +#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"; + 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); + if (offset < 0) { + eprintf("Cannot setup simplefb: %s node not found\n", node); + return; + } + + snprintf(name, sizeof(name), "framebuffer@%08lx", gd->fb_base); + ret = fdt_set_name(blob, offset, name); + if (ret < 0) + goto error; + + 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); + if (ret < 0) + goto error; + + cells[0] = cpu_to_fdt32(graphic_device->winSizeX); + ret = fdt_setprop(blob, offset, "width", cells, sizeof(cells[0])); + if (ret < 0) + goto error; + + cells[0] = cpu_to_fdt32(graphic_device->winSizeY); + ret = fdt_setprop(blob, offset, "height", cells, sizeof(cells[0])); + if (ret < 0) + goto error; + + stride = graphic_device->winSizeX * graphic_device->gdfBytesPP; + cells[0] = cpu_to_fdt32(stride); + ret = fdt_setprop(blob, offset, "stride", cells, sizeof(cells[0])); + if (ret < 0) + goto error; + + 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); + if (ret < 0) + goto error; + + return; +error: + eprintf("Cannot setup simplefb: Error setting properties\n"); +} +#endif /* CONFIG_OF_BOARD_SETUP && CONFIG_VIDEO_DT_SIMPLEFB */ diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index 532fdb7..d7d8571 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 @@ -219,6 +222,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 */