Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
On Mon, Nov 17, 2014 at 10:14 AM, Ian Campbell wrote: > On Mon, 2014-11-17 at 09:58 +, 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
On Sun, Nov 16, 2014 at 4:11 PM, Ian Campbell 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 t
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
On Mon, 2014-11-17 at 09:58 +, 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
Hi, On 11/17/2014 10:58 AM, Grant Likely wrote: > On Sun, Nov 16, 2014 at 4:11 PM, Ian Campbell wrote: >> On Sun, 2014-11-16 at 16:11 +0100, Hans de Goede wrote: >>> 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 >> 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
Hi, On 11/16/2014 06:19 PM, Ian Campbell wrote: > On Sun, 2014-11-16 at 16:11 +, 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 > > 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
On Sun, 2014-11-16 at 16:11 +, 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 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
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
>
> 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 l
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
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
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 h
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
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
> >>
> >> 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 be
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
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
>>
>> 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
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
On Fri, 2014-11-14 at 17:54 +0100, Hans de Goede wrote:
> From: Luc Verhaegen
>
> 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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
Hi, On 11/14/2014 11:22 PM, Anatolij Gustschin wrote: > On Fri, 14 Nov 2014 17:54:47 +0100 > Hans de Goede 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
On Fri, 14 Nov 2014 17:54:47 +0100 Hans de Goede 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 5/6] sunxi: video: Add simplefb support
From: Luc Verhaegen
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
[hdego...@redhat.com: Use pre-populated simplefb node under /chosen as
disussed on the devicetree list]
Signed-off-by: Hans de Goede
---
arch/arm/include/asm/arch-sunxi/display.h | 4 ++
board/sunxi/board.c | 11 ++
drivers/video/sunxi_display.c | 66 +++
include/configs/sunxi-common.h| 8
4 files changed, 89 insertions(+)
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
#include
+#include
#include
#include
#include
@@ -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
#include
#include
+#include
#include
#include
@@ -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 */
--
2.1.0
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
