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

[PATCH v3 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>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
--
Changes in v2:
-Add stdout-path to the example code
Changes in v3:
-Specify that the node name must be "framebuffer@<address>"
-Specify that the link to link simplefb node and display hw noded to one
 another for handover goes into the simplefb node, and must be called "display"
-Specify how aliases may be used to tell the OS how to number displays
-Update the example to reflect these changes
---
 .../bindings/video/simple-framebuffer.txt          | 51 +++++++++++++++++++++-
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 8f35718..6c7339b 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -4,6 +4,31 @@ 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 (*). Simplefb nodes must be named "framebuffer@<address>".
+
+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 the simplefb node must contain a property called "display", which
+contains a phandle pointing to the primary display hw node, so that the OS
+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 is
+considered the primary node.
+
+It is advised to add display# aliases to help the OS determine how to number
+things. If display# aliases are used, then if the simplefb node contains a
+"display" property then the /aliases/display# path must point to the display
+hw node the "display" property points to, otherwise it must point directly
+to the simplefb node.
+
+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.
@@ -19,14 +44,36 @@ Optional properties:
            are expected to already be configured correctly. The OS must
            ensure these clocks are not modified or disabled while the
            simple framebuffer remains active.
+- display : phandle pointing to the primary display hardware node
 
 Example:
 
-	framebuffer {
+aliases {
+	display0 = &lcdc0;
+}
+
+chosen {
+	framebuffer0: framebuffer@5fc00000 {
 		compatible = "simple-framebuffer";
-		reg = <0x1d385000 (1600 * 1200 * 2)>;
+		reg = <0x5fc00000 (4096 * 1024)>;
 		width = <1600>;
 		height = <1200>;
 		stride = <(1600 * 2)>;
 		format = "r5g6b5";
+		clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
+		display = <&lcdc0>;
+	};
+	stdout-path = &framebuffer0;
+};
+
+soc@01c00000 {
+	lcdc0: lcdc@1c0c000 {
+		compatible = "allwinner,sun4i-a10-lcdc";
+		...
 	};
+};
+
+
+*) Older devicetree files may have a compatible = "simple-framebuffer" node
+in a different place, operating systems must first enumerate any compatible
+nodes found under chosen and then check for other compatible nodes.
-- 
2.1.0

[PATCH v3 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
Changes in v3:
-Switch to for_each_child_of_node to loop over the nodes under chosen
---
 drivers/video/fbdev/simplefb.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index cd96edd..223f230 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,31 @@ static struct platform_driver simplefb_driver = {
 	.probe = simplefb_probe,
 	.remove = simplefb_remove,
 };
-module_platform_driver(simplefb_driver);
+
+static int __init simplefb_init(void)
+{
+	int ret;
+	struct device_node *np;
+
+	ret = platform_driver_register(&simplefb_driver);
+	if (ret)
+		return ret;
+
+	for_each_child_of_node(of_chosen, np) {
+		if (of_device_is_compatible(np, "simple-framebuffer"))
+			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 v3 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>
Acked-by: Grant Likely <grant.likely@linaro.org>
---
 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 223f230..1028a0d 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -409,7 +409,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

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

[Tomi Valkeinen]

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

On 13/11/14 15:34, Hans de Goede wrote:
> 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.

The above text is slightly confusing, as fbcon initializes at
module_init time. You only change it in the next patch. I can move the
fbcon change to be the first patch when applying, if there are no more
versions sent.

 Tomi



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

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

[Hans de Goede]

Hi,

On 11/13/2014 03:32 PM, Tomi Valkeinen wrote:
> On 13/11/14 15:34, Hans de Goede wrote:
>> 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.
> 
> The above text is slightly confusing, as fbcon initializes at
> module_init time. You only change it in the next patch. I can move the
> fbcon change to be the first patch when applying, if there are no more
> versions sent.

True, feel free to swap them when applying.

Regards,

Hans

[PATCH v3 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 v3 4/4] fbcon: Change fbcon_init from module_init to fs_initcall

[Tomi Valkeinen]

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

On 13/11/14 15:34, Hans de Goede wrote:
> 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.

Not related to this series, but makes me wonder how difficult it would
be to get fbcon and simplefb initialize at even earlier stage. Well,
something for the future.

 Tomi



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

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

[Tomi Valkeinen]

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

On 13/11/14 15:34, Hans de Goede wrote:

> +chosen {
> +	framebuffer0: framebuffer@5fc00000 {
>  		compatible = "simple-framebuffer";
> -		reg = <0x1d385000 (1600 * 1200 * 2)>;
> +		reg = <0x5fc00000 (4096 * 1024)>;

Was there a reason for this change? The former version seems much more
obvious to me, especially as this is an example.

 Tomi



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

Re: [linux-sunxi] Re: [PATCH v3 1/4] dt-bindings: simplefb: Specify node location and handoff relate

[Hans de Goede]

Hi,

On 11/13/2014 03:28 PM, Tomi Valkeinen wrote:
> On 13/11/14 15:34, Hans de Goede wrote:
> 
>> +chosen {
>> +	framebuffer0: framebuffer@5fc00000 {
>>  		compatible = "simple-framebuffer";
>> -		reg = <0x1d385000 (1600 * 1200 * 2)>;
>> +		reg = <0x5fc00000 (4096 * 1024)>;
> 
> Was there a reason for this change?

I changed the node name to match the new bindings text which
specifies that the node name must be framebuffer@<address>,
while at it I've taken a real world address range.

I assume you refer to the bit where the size of the reg property
is changed ? That indeed is not really necessary.

Regards,

Hans

Re: [linux-sunxi] Re: [PATCH v3 1/4] dt-bindings: simplefb: Specify node location and handoff relate

[Tomi Valkeinen]

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

On 13/11/14 16:46, Hans de Goede wrote:
> Hi,
> 
> On 11/13/2014 03:28 PM, Tomi Valkeinen wrote:
>> On 13/11/14 15:34, Hans de Goede wrote:
>>
>>> +chosen {
>>> +	framebuffer0: framebuffer@5fc00000 {
>>>  		compatible = "simple-framebuffer";
>>> -		reg = <0x1d385000 (1600 * 1200 * 2)>;
>>> +		reg = <0x5fc00000 (4096 * 1024)>;
>>
>> Was there a reason for this change?
> 
> I changed the node name to match the new bindings text which
> specifies that the node name must be framebuffer@<address>,
> while at it I've taken a real world address range.
> 
> I assume you refer to the bit where the size of the reg property
> is changed ? That indeed is not really necessary.

Yes, I meant the size of the reg prop. As this is an example, I think it
should be as clear as possible.

I guess a valid use case here is to set the size to larger-than-needed,
so that it gets preallocated and you don't need to worry about getting
the memory later when the system has already been running for a longer
time. But if that's the case in this example, I think it would be better
to describe it explicitly.

 Tomi



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

Re: [linux-sunxi] Re: [PATCH v3 1/4] dt-bindings: simplefb: Specify node location and handoff relate

[Grant Likely]

On Thu, Nov 13, 2014 at 3:52 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 13/11/14 16:46, Hans de Goede wrote:
>> Hi,
>>
>> On 11/13/2014 03:28 PM, Tomi Valkeinen wrote:
>>> On 13/11/14 15:34, Hans de Goede wrote:
>>>
>>>> +chosen {
>>>> +   framebuffer0: framebuffer@5fc00000 {
>>>>             compatible = "simple-framebuffer";
>>>> -           reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>> +           reg = <0x5fc00000 (4096 * 1024)>;
>>>
>>> Was there a reason for this change?
>>
>> I changed the node name to match the new bindings text which
>> specifies that the node name must be framebuffer@<address>,
>> while at it I've taken a real world address range.
>>
>> I assume you refer to the bit where the size of the reg property
>> is changed ? That indeed is not really necessary.
>
> Yes, I meant the size of the reg prop. As this is an example, I think it
> should be as clear as possible.
>
> I guess a valid use case here is to set the size to larger-than-needed,
> so that it gets preallocated and you don't need to worry about getting
> the memory later when the system has already been running for a longer
> time. But if that's the case in this example, I think it would be better
> to describe it explicitly.

Besides, we already have a mechanism for that called /reserved-memory.
Any memory used by a framebuffer needs to be described in the reserved
map anyway, and that region can be made larger than the firmware
configured framebuffer.

g.