Re: [PATCH 5/6] sunxi: video: Add simplefb support

Re: [PATCH 5/6] sunxi: video: Add simplefb support

[Ian Campbell]

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.

Re: [PATCH 5/6] sunxi: video: Add simplefb support

[Hans de Goede]

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

Re: [PATCH 5/6] sunxi: video: Add simplefb support

[Ian Campbell]

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-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
> >>>>
> >>>> 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.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ian Campbell]

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-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
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.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] sunxi: video: Add simplefb support

[Hans de Goede]

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

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

[Grant Likely]

On Sun, Nov 16, 2014 at 4:11 PM, Ian Campbell <ijc-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org> 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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] sunxi: video: Add simplefb support

[Hans de Goede]

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

Re: [PATCH 5/6] sunxi: video: Add simplefb support

[Ian Campbell]

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.

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

[Grant Likely]

On Mon, Nov 17, 2014 at 10:14 AM, Ian Campbell <ijc-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org> 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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html