[PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Hans de Goede]

Since simplefb nodes do not relate directly to hw typically they have been
placed in the root of the devicetree. As the represent runtime information
having them as sub-nodes of /chosen is more logical, specify this.

Also specify when to set the chosen stdout-path property to a simplefb node.

For reliable handover to a hardware specific driver, that driver needs to
know which simplefb to unregister when taking over, specify how the hw driver
can find the matching simplefb node.

Last add some advice on how to fill and use simplefb nodes from a firmware
pov.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
--
Changes in v2:
-Add stdout-path to the example code
---
 .../bindings/video/simple-framebuffer.txt          | 38 +++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 8f35718..8b7ecf6 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
 the bootloader, with the assumption that the display hardware has already
 been set up to scan out from the memory pointed to by the reg property.
 
+Since simplefb nodes represent runtime information they must be sub-nodes of
+the chosen node (*). The primary display node must be named framebuffer0,
+additional nodes must be called framebuffer1, etc.
+
+If a simplefb node represents the preferred console for user interaction,
+then the chosen node's stdout-path property must point to it.
+
+If the devicetree contains nodes for the display hardware used by a simplefb,
+then one of the hw nodes must have a property called "framebuffer" pointing to
+the framebuffer# node in chosen, so that the operating system knows which
+simplefb to disable when handing over control to a driver for the real
+hardware. The bindings for the hw nodes must specify which node contains the
+framebuffer property.
+
+It is advised that devicetree files contain pre-filled, disabled framebuffer#
+nodes, so that the firmware only needs to update the mode information and
+enable them. This way if e.g. later on support for more display clocks get
+added, the simplefb nodes will already contain this info and the firmware
+does not need to be updated.
+
 Required properties:
 - compatible: "simple-framebuffer"
 - reg: Should contain the location and size of the framebuffer memory.
@@ -22,11 +42,27 @@ Optional properties:
 
 Example:
 
-	framebuffer {
+chosen {
+	framebuffer0 {
 		compatible = "simple-framebuffer";
 		reg = <0x1d385000 (1600 * 1200 * 2)>;
 		width = <1600>;
 		height = <1200>;
 		stride = <(1600 * 2)>;
 		format = "r5g6b5";
+		clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
 	};
+	stdout-path = &framebuffer0;
+};
+
+soc@01c00000 {
+	lcdc0: lcdc@1c0c000 {
+		compatible = "allwinner,sun4i-a10-lcdc";
+		framebuffer = <&framebuffer0>;
+	};
+};
+
+
+*) Older devicetree files may have a compatible = "simple-framebuffer" node
+in a different place, operating systems must first enumerate any framebuffer#
+nodes found under chosen and then check for other compatible nodes.
-- 
2.1.0

[PATCH v2 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Hans de Goede]

Update simplefb to support the new preferred location for simplefb dt nodes
under /chosen.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
--
Changes in v2:
-Make name array larger in case we ever encounter more then 10000 framebuffers
---
 drivers/video/fbdev/simplefb.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index cd96edd..2705af8 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -27,6 +27,7 @@
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/clk-provider.h>
+#include <linux/of_platform.h>
 
 static struct fb_fix_screeninfo simplefb_fix = {
 	.id		= "simple",
@@ -385,7 +386,37 @@ static struct platform_driver simplefb_driver = {
 	.probe = simplefb_probe,
 	.remove = simplefb_remove,
 };
-module_platform_driver(simplefb_driver);
+
+static int __init simplefb_init(void)
+{
+	int i, ret;
+	char name[32];
+	struct device_node *np;
+
+	ret = platform_driver_register(&simplefb_driver);
+	if (ret)
+		return ret;
+
+	for (i = 0; ; i++) {
+		snprintf(name, sizeof(name), "framebuffer%d", i);
+		np = of_find_node_by_name(of_chosen, name);
+		if (!np)
+			break;
+
+		/* of_platform_device_create will check status for us */
+		of_platform_device_create(np, NULL, NULL);
+	}
+
+	return 0;
+}
+
+static void __exit simplefb_exit(void)
+{
+	platform_driver_unregister(&simplefb_driver);
+}
+
+module_init(simplefb_init);
+module_exit(simplefb_exit);
 
 MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
 MODULE_DESCRIPTION("Simple framebuffer driver");
-- 
2.1.0

Re: [PATCH v2 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

[Grant Likely]

On Thu, Nov 13, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Update simplefb to support the new preferred location for simplefb dt nodes
> under /chosen.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> --
> Changes in v2:
> -Make name array larger in case we ever encounter more then 10000 framebuffers

I just replied on v1, but for completeness I'm replying here. Nak on
this method. for_each_child_of_node() should be used instead.

g.

> ---
>  drivers/video/fbdev/simplefb.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index cd96edd..2705af8 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -27,6 +27,7 @@
>  #include <linux/platform_data/simplefb.h>
>  #include <linux/platform_device.h>
>  #include <linux/clk-provider.h>
> +#include <linux/of_platform.h>
>
>  static struct fb_fix_screeninfo simplefb_fix = {
>         .id             = "simple",
> @@ -385,7 +386,37 @@ static struct platform_driver simplefb_driver = {
>         .probe = simplefb_probe,
>         .remove = simplefb_remove,
>  };
> -module_platform_driver(simplefb_driver);
> +
> +static int __init simplefb_init(void)
> +{
> +       int i, ret;
> +       char name[32];
> +       struct device_node *np;
> +
> +       ret = platform_driver_register(&simplefb_driver);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; ; i++) {
> +               snprintf(name, sizeof(name), "framebuffer%d", i);
> +               np = of_find_node_by_name(of_chosen, name);
> +               if (!np)
> +                       break;
> +
> +               /* of_platform_device_create will check status for us */
> +               of_platform_device_create(np, NULL, NULL);
> +       }
> +
> +       return 0;
> +}
> +
> +static void __exit simplefb_exit(void)
> +{
> +       platform_driver_unregister(&simplefb_driver);
> +}
> +
> +module_init(simplefb_init);
> +module_exit(simplefb_exit);
>
>  MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
>  MODULE_DESCRIPTION("Simple framebuffer driver");
> --
> 2.1.0
>

[PATCH v2 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Hans de Goede]

One of the reasons for having the simplefb nodes in /chosen, and doing
explicit enumeration of the nodes there, is too allow enumerating them sooner,
so that we get a console earlier on.

Doing this earlier then fs_initcall is not useful, since the fb only turns into
a console when fbcon intializes, which is a fs_initcall too.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/fbdev/simplefb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 2705af8..48e292a 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
 	platform_driver_unregister(&simplefb_driver);
 }
 
-module_init(simplefb_init);
+/*
+ * While this can be a module, if builtin it's most likely the console
+ * So let's leave module_exit but move module_init to an earlier place
+ */
+fs_initcall(simplefb_init);
 module_exit(simplefb_exit);
 
 MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
-- 
2.1.0

[PATCH v2 4/4] fbcon: Change fbcon_init from module_init to fs_initcall

[Hans de Goede]

Various fb drivers register themselves before module_init so as to have a
console as early as possible, this is of little use if fbcon does not initialze
early too.

Fbcon cannot initialize earlier then fs_initcall, because then the creation
of /sys/class/graphics/fbcon will fail.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/console/fbcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index eb976ee..ea43724 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -3624,7 +3624,7 @@ static int __init fb_console_init(void)
 	return 0;
 }
 
-module_init(fb_console_init);
+fs_initcall(fb_console_init);
 
 #ifdef MODULE
 
-- 
2.1.0

Re: [PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

On Thu, Nov 13, 2014 at 09:50:07AM +0100, Hans de Goede wrote:
> Since simplefb nodes do not relate directly to hw typically they have been
> placed in the root of the devicetree. As the represent runtime information
> having them as sub-nodes of /chosen is more logical, specify this.
> 
> Also specify when to set the chosen stdout-path property to a simplefb node.
> 
> For reliable handover to a hardware specific driver, that driver needs to
> know which simplefb to unregister when taking over, specify how the hw driver
> can find the matching simplefb node.
> 
> Last add some advice on how to fill and use simplefb nodes from a firmware
> pov.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Grant Likely]

On Thu, Nov 13, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Since simplefb nodes do not relate directly to hw typically they have been
> placed in the root of the devicetree. As the represent runtime information
> having them as sub-nodes of /chosen is more logical, specify this.
>
> Also specify when to set the chosen stdout-path property to a simplefb node.
>
> For reliable handover to a hardware specific driver, that driver needs to
> know which simplefb to unregister when taking over, specify how the hw driver
> can find the matching simplefb node.
>
> Last add some advice on how to fill and use simplefb nodes from a firmware
> pov.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> --
> Changes in v2:
> -Add stdout-path to the example code
> ---
>  .../bindings/video/simple-framebuffer.txt          | 38 +++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 8f35718..8b7ecf6 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>  the bootloader, with the assumption that the display hardware has already
>  been set up to scan out from the memory pointed to by the reg property.
>
> +Since simplefb nodes represent runtime information they must be sub-nodes of
> +the chosen node (*). The primary display node must be named framebuffer0,
> +additional nodes must be called framebuffer1, etc.

Thinking more about our conversation yesterday, the preferred name
should still be framebuffer@<address>. There is no reason to break
with established convention, and as mentioned in my reply on patch #2,
using the name is not the preferred way to identify a device node. So,
my recommendation is to prefer the name "framebuffer@<addr>", but if
it is inconvenient because the address isn't known, then
"framebuffer#" is acceptable. Then use /aliases for actual enumeration
which has the advantage of also working for framebuffers that aren't
in /chosen.

> +
> +If a simplefb node represents the preferred console for user interaction,
> +then the chosen node's stdout-path property must point to it.
> +
> +If the devicetree contains nodes for the display hardware used by a simplefb,
> +then one of the hw nodes must have a property called "framebuffer" pointing to
> +the framebuffer# node in chosen, so that the operating system knows which
> +simplefb to disable when handing over control to a driver for the real
> +hardware. The bindings for the hw nodes must specify which node contains the
> +framebuffer property.

I've also been thinking about this. When we talked yesterday I said I
didn't have a preference as to which direction the link between the
framebuffer and the display hardware pointed. Thinking about it now,
between the two nodes, the HW node is pretty much static, but the FB
node is dynamic and in most scenarios will be enabled by firmware, and
then disabled by the operating system. If the framebuffer property is
in the HW node, then that means the firmware needs to modify two nodes
to set up the linkage correctly. It needs to enable the framebuffer,
and then add the link. If one of several possible FB nodes was enabled
by the firmware, it need to modify the display controller node to
match. And, when the framebuffer is disabled, the HW node is left with
a dangling link that kexec won't necessarily know how to fix up.

If instead the link goes the other way, as a property in the
framebuffer node pointing to display controller, all of the
functionality still stays the same, but the display controller node
never needs to change when framebuffers are
enabled/disabled/added/removed. Since the location and/or compatible
value of framebuffers is always well known, (aside from the fact that
the simplefb driver binds to them), we can have a single API that will
return all the framebuffers associated to a given display controller
node. The real HW driver will be able to use that to find its
framebuffers and coordinate handover between them.

g.

> +
> +It is advised that devicetree files contain pre-filled, disabled framebuffer#
> +nodes, so that the firmware only needs to update the mode information and
> +enable them. This way if e.g. later on support for more display clocks get
> +added, the simplefb nodes will already contain this info and the firmware
> +does not need to be updated.
> +
>  Required properties:
>  - compatible: "simple-framebuffer"
>  - reg: Should contain the location and size of the framebuffer memory.
> @@ -22,11 +42,27 @@ Optional properties:
>
>  Example:
>
> -       framebuffer {
> +chosen {
> +       framebuffer0 {
>                 compatible = "simple-framebuffer";
>                 reg = <0x1d385000 (1600 * 1200 * 2)>;
>                 width = <1600>;
>                 height = <1200>;
>                 stride = <(1600 * 2)>;
>                 format = "r5g6b5";
> +               clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
>         };
> +       stdout-path = &framebuffer0;
> +};
> +
> +soc@01c00000 {
> +       lcdc0: lcdc@1c0c000 {
> +               compatible = "allwinner,sun4i-a10-lcdc";
> +               framebuffer = <&framebuffer0>;
> +       };
> +};
> +
> +
> +*) Older devicetree files may have a compatible = "simple-framebuffer" node
> +in a different place, operating systems must first enumerate any framebuffer#
> +nodes found under chosen and then check for other compatible nodes.
> --
> 2.1.0
>

Re: [PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Grant Likely]

On Thu, Nov 13, 2014 at 11:02 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Thu, Nov 13, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Since simplefb nodes do not relate directly to hw typically they have been
>> placed in the root of the devicetree. As the represent runtime information
>> having them as sub-nodes of /chosen is more logical, specify this.
>>
>> Also specify when to set the chosen stdout-path property to a simplefb node.
>>
>> For reliable handover to a hardware specific driver, that driver needs to
>> know which simplefb to unregister when taking over, specify how the hw driver
>> can find the matching simplefb node.
>>
>> Last add some advice on how to fill and use simplefb nodes from a firmware
>> pov.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> --
>> Changes in v2:
>> -Add stdout-path to the example code
>> ---
>>  .../bindings/video/simple-framebuffer.txt          | 38 +++++++++++++++++++++-
>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index 8f35718..8b7ecf6 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>>  the bootloader, with the assumption that the display hardware has already
>>  been set up to scan out from the memory pointed to by the reg property.
>>
>> +Since simplefb nodes represent runtime information they must be sub-nodes of
>> +the chosen node (*). The primary display node must be named framebuffer0,
>> +additional nodes must be called framebuffer1, etc.
>
> Thinking more about our conversation yesterday, the preferred name
> should still be framebuffer@<address>. There is no reason to break
> with established convention, and as mentioned in my reply on patch #2,
> using the name is not the preferred way to identify a device node. So,
> my recommendation is to prefer the name "framebuffer@<addr>", but if
> it is inconvenient because the address isn't known, then
> "framebuffer#" is acceptable. Then use /aliases for actual enumeration
> which has the advantage of also working for framebuffers that aren't
> in /chosen.

Some more thoughts about aliases.

There is a natural tension between enumerate framebuffers or
enumerating displays. There is precedence for displays to be named
'display...' and to use /aliases/display* to enumerate them. What do
we do for framebuffers? Does it make sense to have a separate
/aliases/framebuffer* enumeration, or would it be better to have a
single 'display' namespace so that the same name is used right
through?

Right now I'm leaning towards using /aliases/display* for everything,
and making simplefb understand that /aliases/display# could either
point directly to the framebuffer when there is not node for the
hardware, or point to the display node. This would make it easy to
have consistent names for display. For example, with the kexec
scenario, if /aliases/display0 points at the display node then the
alias won't need to be changed between first boot when firmware sets
up a framebuffer, and kexec boot after the kernel has torn down the
original framebuffer. If /aliases/display0 points to the framebuffer
node directly, then the linkage from /aliases/display0 to the HW
display node will be lost when the framebuffer gets torn down.

Doing it this way does make two assumptions though. It assumes that
each display will have its own node somewhere so that no two
framebuffers will point to the same node. Second, it assumes that we
have a mechanism to handoff a display name (/dev/fb*)? from simplefb
to another device. I don't know enough about the fbdev subsystem to
know whether or not this will be a problem. Or if there are any
namespace conflicts between fbdev and drm.

Implementation would be simple to do. If the simple framebuffer node
has a 'display' property, then it will call of_alias_get_id() on the
target of that phandle. Otherwise it will call of_alias_get_id() on
itself.

Thoughts?

g.

>
>> +
>> +If a simplefb node represents the preferred console for user interaction,
>> +then the chosen node's stdout-path property must point to it.
>> +
>> +If the devicetree contains nodes for the display hardware used by a simplefb,
>> +then one of the hw nodes must have a property called "framebuffer" pointing to
>> +the framebuffer# node in chosen, so that the operating system knows which
>> +simplefb to disable when handing over control to a driver for the real
>> +hardware. The bindings for the hw nodes must specify which node contains the
>> +framebuffer property.
>
> I've also been thinking about this. When we talked yesterday I said I
> didn't have a preference as to which direction the link between the
> framebuffer and the display hardware pointed. Thinking about it now,
> between the two nodes, the HW node is pretty much static, but the FB
> node is dynamic and in most scenarios will be enabled by firmware, and
> then disabled by the operating system. If the framebuffer property is
> in the HW node, then that means the firmware needs to modify two nodes
> to set up the linkage correctly. It needs to enable the framebuffer,
> and then add the link. If one of several possible FB nodes was enabled
> by the firmware, it need to modify the display controller node to
> match. And, when the framebuffer is disabled, the HW node is left with
> a dangling link that kexec won't necessarily know how to fix up.
>
> If instead the link goes the other way, as a property in the
> framebuffer node pointing to display controller, all of the
> functionality still stays the same, but the display controller node
> never needs to change when framebuffers are
> enabled/disabled/added/removed. Since the location and/or compatible
> value of framebuffers is always well known, (aside from the fact that
> the simplefb driver binds to them), we can have a single API that will
> return all the framebuffers associated to a given display controller
> node. The real HW driver will be able to use that to find its
> framebuffers and coordinate handover between them.
>
> g.
>
>> +
>> +It is advised that devicetree files contain pre-filled, disabled framebuffer#
>> +nodes, so that the firmware only needs to update the mode information and
>> +enable them. This way if e.g. later on support for more display clocks get
>> +added, the simplefb nodes will already contain this info and the firmware
>> +does not need to be updated.
>> +
>>  Required properties:
>>  - compatible: "simple-framebuffer"
>>  - reg: Should contain the location and size of the framebuffer memory.
>> @@ -22,11 +42,27 @@ Optional properties:
>>
>>  Example:
>>
>> -       framebuffer {
>> +chosen {
>> +       framebuffer0 {
>>                 compatible = "simple-framebuffer";
>>                 reg = <0x1d385000 (1600 * 1200 * 2)>;
>>                 width = <1600>;
>>                 height = <1200>;
>>                 stride = <(1600 * 2)>;
>>                 format = "r5g6b5";
>> +               clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
>>         };
>> +       stdout-path = &framebuffer0;
>> +};
>> +
>> +soc@01c00000 {
>> +       lcdc0: lcdc@1c0c000 {
>> +               compatible = "allwinner,sun4i-a10-lcdc";
>> +               framebuffer = <&framebuffer0>;
>> +       };
>> +};
>> +
>> +
>> +*) Older devicetree files may have a compatible = "simple-framebuffer" node
>> +in a different place, operating systems must first enumerate any framebuffer#
>> +nodes found under chosen and then check for other compatible nodes.
>> --
>> 2.1.0
>>

Re: [PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Hans de Goede]

Hi,

On 11/13/2014 01:03 PM, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 11:02 AM, Grant Likely <grant.likely@linaro.org> wrote:
>> On Thu, Nov 13, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Since simplefb nodes do not relate directly to hw typically they have been
>>> placed in the root of the devicetree. As the represent runtime information
>>> having them as sub-nodes of /chosen is more logical, specify this.
>>>
>>> Also specify when to set the chosen stdout-path property to a simplefb node.
>>>
>>> For reliable handover to a hardware specific driver, that driver needs to
>>> know which simplefb to unregister when taking over, specify how the hw driver
>>> can find the matching simplefb node.
>>>
>>> Last add some advice on how to fill and use simplefb nodes from a firmware
>>> pov.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>> --
>>> Changes in v2:
>>> -Add stdout-path to the example code
>>> ---
>>>  .../bindings/video/simple-framebuffer.txt          | 38 +++++++++++++++++++++-
>>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> index 8f35718..8b7ecf6 100644
>>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>>>  the bootloader, with the assumption that the display hardware has already
>>>  been set up to scan out from the memory pointed to by the reg property.
>>>
>>> +Since simplefb nodes represent runtime information they must be sub-nodes of
>>> +the chosen node (*). The primary display node must be named framebuffer0,
>>> +additional nodes must be called framebuffer1, etc.
>>
>> Thinking more about our conversation yesterday, the preferred name
>> should still be framebuffer@<address>. There is no reason to break
>> with established convention, and as mentioned in my reply on patch #2,
>> using the name is not the preferred way to identify a device node. So,
>> my recommendation is to prefer the name "framebuffer@<addr>", but if
>> it is inconvenient because the address isn't known, then
>> "framebuffer#" is acceptable. Then use /aliases for actual enumeration
>> which has the advantage of also working for framebuffers that aren't
>> in /chosen.
> 
> Some more thoughts about aliases.
> 
> There is a natural tension between enumerate framebuffers or
> enumerating displays. There is precedence for displays to be named
> 'display...' and to use /aliases/display* to enumerate them. What do
> we do for framebuffers? Does it make sense to have a separate
> /aliases/framebuffer* enumeration, or would it be better to have a
> single 'display' namespace so that the same name is used right
> through?
> 
> Right now I'm leaning towards using /aliases/display* for everything,
> and making simplefb understand that /aliases/display# could either
> point directly to the framebuffer when there is not node for the
> hardware, or point to the display node. This would make it easy to
> have consistent names for display. For example, with the kexec
> scenario, if /aliases/display0 points at the display node then the
> alias won't need to be changed between first boot when firmware sets
> up a framebuffer, and kexec boot after the kernel has torn down the
> original framebuffer. If /aliases/display0 points to the framebuffer
> node directly, then the linkage from /aliases/display0 to the HW
> display node will be lost when the framebuffer gets torn down.
> 
> Doing it this way does make two assumptions though. It assumes that
> each display will have its own node somewhere so that no two
> framebuffers will point to the same node. Second, it assumes that we
> have a mechanism to handoff a display name (/dev/fb*)? from simplefb
> to another device. I don't know enough about the fbdev subsystem to
> know whether or not this will be a problem. Or if there are any
> namespace conflicts between fbdev and drm.
> 
> Implementation would be simple to do. If the simple framebuffer node
> has a 'display' property, then it will call of_alias_get_id() on the
> target of that phandle. Otherwise it will call of_alias_get_id() on
> itself.
> 
> Thoughts?

Re-using display# aliases, and specifying that if the simplefb node has
a display property, that then the id of the node the display property
points to should be used, and otherwise the id of the simplefb node itself
sounds like a good idea.

I'll go work on a v3 and put this in (and update the example for this
as well).

Regards,

Hans

Re: [PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Hans de Goede]

Hi,

On 11/13/2014 12:02 PM, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Since simplefb nodes do not relate directly to hw typically they have been
>> placed in the root of the devicetree. As the represent runtime information
>> having them as sub-nodes of /chosen is more logical, specify this.
>>
>> Also specify when to set the chosen stdout-path property to a simplefb node.
>>
>> For reliable handover to a hardware specific driver, that driver needs to
>> know which simplefb to unregister when taking over, specify how the hw driver
>> can find the matching simplefb node.
>>
>> Last add some advice on how to fill and use simplefb nodes from a firmware
>> pov.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> --
>> Changes in v2:
>> -Add stdout-path to the example code
>> ---
>>  .../bindings/video/simple-framebuffer.txt          | 38 +++++++++++++++++++++-
>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index 8f35718..8b7ecf6 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>>  the bootloader, with the assumption that the display hardware has already
>>  been set up to scan out from the memory pointed to by the reg property.
>>
>> +Since simplefb nodes represent runtime information they must be sub-nodes of
>> +the chosen node (*). The primary display node must be named framebuffer0,
>> +additional nodes must be called framebuffer1, etc.
> 
> Thinking more about our conversation yesterday, the preferred name
> should still be framebuffer@<address>. There is no reason to break
> with established convention, and as mentioned in my reply on patch #2,
> using the name is not the preferred way to identify a device node. So,
> my recommendation is to prefer the name "framebuffer@<addr>", but if
> it is inconvenient because the address isn't known, then
> "framebuffer#" is acceptable.

Since we want to use pre-filled simplefb nodes already present in the devicetree,
and u-boot is the one chosing the framebuffer address we don't know the
address to put in the dts file beforehand, so framebuffer# it is.

> Then use /aliases for actual enumeration
> which has the advantage of also working for framebuffers that aren't
> in /chosen.

I'll discuss the aliases in my reply to your next mail which goes into
more details about this.

> 
>> +
>> +If a simplefb node represents the preferred console for user interaction,
>> +then the chosen node's stdout-path property must point to it.
>> +
>> +If the devicetree contains nodes for the display hardware used by a simplefb,
>> +then one of the hw nodes must have a property called "framebuffer" pointing to
>> +the framebuffer# node in chosen, so that the operating system knows which
>> +simplefb to disable when handing over control to a driver for the real
>> +hardware. The bindings for the hw nodes must specify which node contains the
>> +framebuffer property.
> 
> I've also been thinking about this. When we talked yesterday I said I
> didn't have a preference as to which direction the link between the
> framebuffer and the display hardware pointed. Thinking about it now,
> between the two nodes, the HW node is pretty much static, but the FB
> node is dynamic and in most scenarios will be enabled by firmware, and
> then disabled by the operating system. If the framebuffer property is
> in the HW node, then that means the firmware needs to modify two nodes
> to set up the linkage correctly. It needs to enable the framebuffer,
> and then add the link. If one of several possible FB nodes was enabled
> by the firmware, it need to modify the display controller node to
> match. And, when the framebuffer is disabled, the HW node is left with
> a dangling link that kexec won't necessarily know how to fix up.
> 
> If instead the link goes the other way, as a property in the
> framebuffer node pointing to display controller, all of the
> functionality still stays the same, but the display controller node
> never needs to change when framebuffers are
> enabled/disabled/added/removed. Since the location and/or compatible
> value of framebuffers is always well known, (aside from the fact that
> the simplefb driver binds to them), we can have a single API that will
> return all the framebuffers associated to a given display controller
> node. The real HW driver will be able to use that to find its
> framebuffers and coordinate handover between them.

Ok, I'll change the bindings so that the link goes the other way, note that
he firmware should not be touching the link at all, we've a pre-populated
simplefb node in the devicetree for the firmware to further fill in
(and enable), and that should already contain the link.

Regards,

Hans




> 
> g.
> 
>> +
>> +It is advised that devicetree files contain pre-filled, disabled framebuffer#
>> +nodes, so that the firmware only needs to update the mode information and
>> +enable them. This way if e.g. later on support for more display clocks get
>> +added, the simplefb nodes will already contain this info and the firmware
>> +does not need to be updated.
>> +
>>  Required properties:
>>  - compatible: "simple-framebuffer"
>>  - reg: Should contain the location and size of the framebuffer memory.
>> @@ -22,11 +42,27 @@ Optional properties:
>>
>>  Example:
>>
>> -       framebuffer {
>> +chosen {
>> +       framebuffer0 {
>>                 compatible = "simple-framebuffer";
>>                 reg = <0x1d385000 (1600 * 1200 * 2)>;
>>                 width = <1600>;
>>                 height = <1200>;
>>                 stride = <(1600 * 2)>;
>>                 format = "r5g6b5";
>> +               clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
>>         };
>> +       stdout-path = &framebuffer0;
>> +};
>> +
>> +soc@01c00000 {
>> +       lcdc0: lcdc@1c0c000 {
>> +               compatible = "allwinner,sun4i-a10-lcdc";
>> +               framebuffer = <&framebuffer0>;
>> +       };
>> +};
>> +
>> +
>> +*) Older devicetree files may have a compatible = "simple-framebuffer" node
>> +in a different place, operating systems must first enumerate any framebuffer#
>> +nodes found under chosen and then check for other compatible nodes.
>> --
>> 2.1.0
>>

Re: [PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Grant Likely]

On Thu, Nov 13, 2014 at 12:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 11/13/2014 12:02 PM, Grant Likely wrote:
>> On Thu, Nov 13, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Since simplefb nodes do not relate directly to hw typically they have been
>>> placed in the root of the devicetree. As the represent runtime information
>>> having them as sub-nodes of /chosen is more logical, specify this.
>>>
>>> Also specify when to set the chosen stdout-path property to a simplefb node.
>>>
>>> For reliable handover to a hardware specific driver, that driver needs to
>>> know which simplefb to unregister when taking over, specify how the hw driver
>>> can find the matching simplefb node.
>>>
>>> Last add some advice on how to fill and use simplefb nodes from a firmware
>>> pov.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>> --
>>> Changes in v2:
>>> -Add stdout-path to the example code
>>> ---
>>>  .../bindings/video/simple-framebuffer.txt          | 38 +++++++++++++++++++++-
>>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> index 8f35718..8b7ecf6 100644
>>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>>>  the bootloader, with the assumption that the display hardware has already
>>>  been set up to scan out from the memory pointed to by the reg property.
>>>
>>> +Since simplefb nodes represent runtime information they must be sub-nodes of
>>> +the chosen node (*). The primary display node must be named framebuffer0,
>>> +additional nodes must be called framebuffer1, etc.
>>
>> Thinking more about our conversation yesterday, the preferred name
>> should still be framebuffer@<address>. There is no reason to break
>> with established convention, and as mentioned in my reply on patch #2,
>> using the name is not the preferred way to identify a device node. So,
>> my recommendation is to prefer the name "framebuffer@<addr>", but if
>> it is inconvenient because the address isn't known, then
>> "framebuffer#" is acceptable.
>
> Since we want to use pre-filled simplefb nodes already present in the devicetree,
> and u-boot is the one chosing the framebuffer address we don't know the
> address to put in the dts file beforehand, so framebuffer# it is.

fdt_set_name() is available to U-Boot to rename a node. :-)

Prepopulating with /chosen/framebuffer#, and then renaming to
/chosen/framebuffer@<addr> is perfectly reasonable.

g.

Re: [PATCH v2 1/4] dt-bindings: simplefb: Specify node location and handoff related properties

[Hans de Goede]

Hi,

On 11/13/2014 01:36 PM, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 12:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 11/13/2014 12:02 PM, Grant Likely wrote:
>>> On Thu, Nov 13, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Since simplefb nodes do not relate directly to hw typically they have been
>>>> placed in the root of the devicetree. As the represent runtime information
>>>> having them as sub-nodes of /chosen is more logical, specify this.
>>>>
>>>> Also specify when to set the chosen stdout-path property to a simplefb node.
>>>>
>>>> For reliable handover to a hardware specific driver, that driver needs to
>>>> know which simplefb to unregister when taking over, specify how the hw driver
>>>> can find the matching simplefb node.
>>>>
>>>> Last add some advice on how to fill and use simplefb nodes from a firmware
>>>> pov.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>>> --
>>>> Changes in v2:
>>>> -Add stdout-path to the example code
>>>> ---
>>>>  .../bindings/video/simple-framebuffer.txt          | 38 +++++++++++++++++++++-
>>>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>>> index 8f35718..8b7ecf6 100644
>>>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>>>> @@ -4,6 +4,26 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>>>>  the bootloader, with the assumption that the display hardware has already
>>>>  been set up to scan out from the memory pointed to by the reg property.
>>>>
>>>> +Since simplefb nodes represent runtime information they must be sub-nodes of
>>>> +the chosen node (*). The primary display node must be named framebuffer0,
>>>> +additional nodes must be called framebuffer1, etc.
>>>
>>> Thinking more about our conversation yesterday, the preferred name
>>> should still be framebuffer@<address>. There is no reason to break
>>> with established convention, and as mentioned in my reply on patch #2,
>>> using the name is not the preferred way to identify a device node. So,
>>> my recommendation is to prefer the name "framebuffer@<addr>", but if
>>> it is inconvenient because the address isn't known, then
>>> "framebuffer#" is acceptable.
>>
>> Since we want to use pre-filled simplefb nodes already present in the devicetree,
>> and u-boot is the one chosing the framebuffer address we don't know the
>> address to put in the dts file beforehand, so framebuffer# it is.
> 
> fdt_set_name() is available to U-Boot to rename a node. :-)
> 
> Prepopulating with /chosen/framebuffer#, and then renaming to
> /chosen/framebuffer@<addr> is perfectly reasonable.

Ok, that works for me.

Regards,

Hans