dt-bindings: simplefb: Specify node location and handoff related properties

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

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/video/fbdev/simplefb.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

drivers/video/fbdev/simplefb.c +4 -1

index cd96edd..be7d288 100644
@@ -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)
+{

Hi Hans,

Quoted 35 lines · Hans de Goede

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

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/video/fbdev/simplefb.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

drivers/video/fbdev/simplefb.c +11 -1

index cd96edd..be7d288 100644
@@ -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[16];
+       struct device_node *np;
+
+       ret = platform_driver_register(&simplefb_driver);
+       if (ret)
+               return ret;

drivers/video/fbdev/simplefb.c static struct platform_driver simplefb_driver = { · line 401

Quoted 3 lines

drivers/video/fbdev/simplefb.c +3 -0

+
+       for (i = 0; ; i++) {
+               snprintf(name, sizeof(name), "framebuffer%d", i);

This smells like an infinite loop: we can be pretty sure that no hardware will ever exist with more than 9999 (I think?) framebuffers, however if that ever happens this'll loop until it runs out of RAM. Maybe add a suitably high limit to the for loop?

Quoted 6 lines

Quoted 6 lines

drivers/video/fbdev/simplefb.c +6 -0

+       char name[16];
+       struct device_node *np;
+
+       ret = platform_driver_register(&simplefb_driver);
+       if (ret)
+               return ret;

Quoted 8 lines

drivers/video/fbdev/simplefb.c static struct platform_driver simplefb_driver = { · line 401

Quoted 3 lines

drivers/video/fbdev/simplefb.c +3 -0

+
+       for (i = 0; ; i++) {
+               snprintf(name, sizeof(name), "framebuffer%d", i);

This smells like an infinite loop: we can be pretty sure that no hardware will ever exist with more than 9999 (I think?) framebuffers, however if that ever happens this'll loop until it runs out of RAM. Maybe add a suitably high limit to the for loop?

The loop will stop as soon as there are no more framebuffer# nodes in chosen, so the loop is only infinite if there are infinite nodes in the devicetree, which would make the devicetree infinitely large, so this will never happen.

Hi Hans,

Quoted 15 lines · Hans de Goede

Quoted 6 lines

Quoted 6 lines

drivers/video/fbdev/simplefb.c +6 -0

+       char name[16];
+       struct device_node *np;
+
+       ret = platform_driver_register(&simplefb_driver);
+       if (ret)
+               return ret;

Quoted 8 lines

drivers/video/fbdev/simplefb.c static struct platform_driver simplefb_driver = { · line 401

Quoted 3 lines

drivers/video/fbdev/simplefb.c +3 -0

+
+       for (i = 0; ; i++) {
+               snprintf(name, sizeof(name), "framebuffer%d", i);

This smells like an infinite loop: we can be pretty sure that no hardware will ever exist with more than 9999 (I think?) framebuffers, however if that ever happens this'll loop until it runs out of RAM. Maybe add a suitably high limit to the for loop?

Quoted 3 lines

The loop will stop as soon as there are no more framebuffer# nodes in chosen, so the loop is only infinite if there are infinite nodes in the devicetree, which would make the devicetree infinitely large, so this will never happen.

If there are 10000 frame buffers, the loop will continue beyond that, as the index will be truncated to 4 digits due to the size of name[]. It will stop when (signed) i becomes negative, though ;-)

One solution is to increase the size of name[],

Hi,

Quoted 22 lines · Geert Uytterhoeven

Hi Hans,

Quoted 18 lines · Hans de Goede

Quoted 6 lines

Quoted 6 lines

drivers/video/fbdev/simplefb.c +6 -0

+       char name[16];
+       struct device_node *np;
+
+       ret = platform_driver_register(&simplefb_driver);
+       if (ret)
+               return ret;

Quoted 8 lines

drivers/video/fbdev/simplefb.c static struct platform_driver simplefb_driver = { · line 401

Quoted 3 lines

drivers/video/fbdev/simplefb.c +3 -0

+
+       for (i = 0; ; i++) {
+               snprintf(name, sizeof(name), "framebuffer%d", i);

This smells like an infinite loop: we can be pretty sure that no hardware will ever exist with more than 9999 (I think?) framebuffers, however if that ever happens this'll loop until it runs out of RAM. Maybe add a suitably high limit to the for loop?

The loop will stop as soon as there are no more framebuffer# nodes in chosen, so the loop is only infinite if there are infinite nodes in the devicetree, which would make the devicetree infinitely large, so this will never happen.

Quoted 5 lines

If there are 10000 frame buffers, the loop will continue beyond that, as the index will be truncated to 4 digits due to the size of name[]. It will stop when (signed) i becomes negative, though ;-)

One solution is to increase the size of name[],

This is probably never going to happen, but I'll increase the size of name in v2 anyways.

Hi Hans,

Quoted 35 lines · Hans de Goede

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

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/video/fbdev/simplefb.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

drivers/video/fbdev/simplefb.c +11 -1

index cd96edd..be7d288 100644
@@ -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[16];
+       struct device_node *np;
+
+       ret = platform_driver_register(&simplefb_driver);
+       if (ret)
+               return ret;

drivers/video/fbdev/simplefb.c static struct platform_driver simplefb_driver = { · line 401

Quoted 3 lines

drivers/video/fbdev/simplefb.c +3 -0

+
+       for (i = 0; ; i++) {
+               snprintf(name, sizeof(name), "framebuffer%d", i);

This smells like an infinite loop: we can be pretty sure that no hardware will ever exist with more than 9999 (I think?) framebuffers, however if that ever happens this'll loop until it runs out of RAM. Maybe add a suitably high limit to the for loop?

drivers/video/fbdev/simplefb.c static struct platform_driver simplefb_driver = { · line 392

Quoted 12 lines · Hans de Goede

drivers/video/fbdev/simplefb.c +4 -1

@@ -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;

unsigned int i;

drivers/video/fbdev/simplefb.c static struct platform_driver simplefb_driver = { · line 401

Quoted 9 lines

drivers/video/fbdev/simplefb.c +9 -0

+       char name[16];
+       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);

Quoted 4 lines · Geert Uytterhoeven

Quoted 2 lines · Hans de Goede

drivers/video/fbdev/simplefb.c +0 -0

Quoted 11 lines

drivers/video/fbdev/simplefb.c static struct platform_driver simplefb_driver = { · line 392

Quoted 9 lines

drivers/video/fbdev/simplefb.c +4 -1

@@ -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;

unsigned int i;

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 <[email protected]>

---
 drivers/video/fbdev/simplefb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/video/fbdev/simplefb.c +0 -0

index be7d288..8c0c972 100644

drivers/video/fbdev/simplefb.c static void __exit simplefb_exit(void) · line 421

Quoted 5 lines

drivers/video/fbdev/simplefb.c +4 -1

-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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Hi Maxime,

Quoted 9 lines · Maxime Ripard

drivers/video/fbdev/simplefb.c static void __exit simplefb_exit(void) · line 421

Quoted 5 lines

drivers/video/fbdev/simplefb.c +4 -1

-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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

If it depends on clocks, it won't work as a module, as CCF will have disabled all unused clocks at that point.

Quoted 14 lines · Geert Uytterhoeven

Hi Maxime,

Quoted 9 lines · Maxime Ripard

drivers/video/fbdev/simplefb.c static void __exit simplefb_exit(void) · line 421

Quoted 5 lines

drivers/video/fbdev/simplefb.c +4 -1

-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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Quoted 2 lines

If it depends on clocks, it won't work as a module, as CCF will have disabled all unused clocks at that point.

If it does depend on anything beyond clocks it won't work at all. Clocks are special because they get set up very early at boot time. If it turns out that a simplefb ever needs a regulator to remain on, and that's even quite likely to happen eventually, it's going to fail miserably, because those regulators will typically be provided by a PMIC on an I2C bus. The regulator won't be registered until very late into the boot process and a regulator_get() call will almost certainly cause the simplefb driver to defer probing.

Now deferring probing is a real showstopper for simplefb, because not only does it make the framebuffer useless as early boot console, once probing is attempted again the clocks that it would have needed to acquire to keep going will already have been switched off, too.

Hi,

Quoted 18 lines · Thierry Reding

Quoted 14 lines · Geert Uytterhoeven

Hi Maxime,

Quoted 9 lines · Maxime Ripard

drivers/video/fbdev/simplefb.c static void __exit simplefb_exit(void) · line 421

Quoted 5 lines

drivers/video/fbdev/simplefb.c +4 -1

-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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Quoted 2 lines

If it depends on clocks, it won't work as a module, as CCF will have disabled all unused clocks at that point.

Quoted 8 lines

If it does depend on anything beyond clocks it won't work at all. Clocks are special because they get set up very early at boot time. If it turns out that a simplefb ever needs a regulator to remain on, and that's even quite likely to happen eventually, it's going to fail miserably, because those regulators will typically be provided by a PMIC on an I2C bus. The regulator won't be registered until very late into the boot process and a regulator_get() call will almost certainly cause the simplefb driver to defer probing.

Right, this has been discussed already and the plan is to have simplefb continue its probe function and return success from it if it encounters any -eprobe_defer errors, while tracking which resources it misses.

And then have a late_initcall which will claim any resources which failed with -eprobe beforehand.

Quoted 4 lines

Now deferring probing is a real showstopper for simplefb, because not only does it make the framebuffer useless as early boot console, once probing is attempted again the clocks that it would have needed to acquire to keep going will already have been switched off, too.

That is not true, even with the current implementation, if all necessary drivers are built in, then simplefb will come up later, but it will still come up before the late_initcall which disables the clocks.

Once we do the split probing described above (which is something which we plan to do when it becomes necessary), then simplefb will still come up early.

Quoted 30 lines · Hans de Goede

Hi,

Quoted 18 lines · Thierry Reding

Quoted 14 lines · Geert Uytterhoeven

Hi Maxime,

Quoted 9 lines · Maxime Ripard

drivers/video/fbdev/simplefb.c static void __exit simplefb_exit(void) · line 421

Quoted 5 lines

drivers/video/fbdev/simplefb.c +4 -1

-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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Quoted 2 lines

If it depends on clocks, it won't work as a module, as CCF will have disabled all unused clocks at that point.

Quoted 8 lines

If it does depend on anything beyond clocks it won't work at all. Clocks are special because they get set up very early at boot time. If it turns out that a simplefb ever needs a regulator to remain on, and that's even quite likely to happen eventually, it's going to fail miserably, because those regulators will typically be provided by a PMIC on an I2C bus. The regulator won't be registered until very late into the boot process and a regulator_get() call will almost certainly cause the simplefb driver to defer probing.

Quoted 6 lines

Right, this has been discussed already and the plan is to have simplefb continue its probe function and return success from it if it encounters any -eprobe_defer errors, while tracking which resources it misses.

And then have a late_initcall which will claim any resources which failed with -eprobe beforehand.

How do you ensure that the late_initcall gets run before any of the other late_initcalls that disable the resources?

Also my recollection is
that deferred probing will first be triggered the first time from a late_initcall, so chances aren't very high that all resources have shown up by that time.

Quoted 8 lines

Quoted 4 lines

Now deferring probing is a real showstopper for simplefb, because not only does it make the framebuffer useless as early boot console, once probing is attempted again the clocks that it would have needed to acquire to keep going will already have been switched off, too.

That is not true, even with the current implementation, if all necessary drivers are built in, then simplefb will come up later, but it will still come up before the late_initcall which disables the clocks.

Yes, in the current implementation because clocks typically are registered very early and thus you don't hit the deferred probe. The same is not true for other types of resources where it's actually quite common to hit deferred probing (regulators is a very notorious one).

It doesn't matter whether a driver is built-in or not, once you hit deferred probing you lose.

Quoted 3 lines

Once we do the split probing described above (which is something which we plan to do when it becomes necessary), then simplefb will still come up early.

It will come up early but won't have acquired all the resources that it needs, so unless you somehow manage to order late_initcalls in exactly the way that you need them, the frameworks will still turn off what you haven't managed to claim yet.

Hi,

Quoted 38 lines · Thierry Reding

Quoted 30 lines · Hans de Goede

Hi,

Quoted 18 lines · Thierry Reding

Quoted 14 lines · Geert Uytterhoeven

Hi Maxime,

Quoted 9 lines · Maxime Ripard

Review

Quoted 5 lines

(quoted diff) +4 -1

-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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Quoted 2 lines

If it depends on clocks, it won't work as a module, as CCF will have disabled all unused clocks at that point.

Quoted 8 lines

If it does depend on anything beyond clocks it won't work at all. Clocks are special because they get set up very early at boot time. If it turns out that a simplefb ever needs a regulator to remain on, and that's even quite likely to happen eventually, it's going to fail miserably, because those regulators will typically be provided by a PMIC on an I2C bus. The regulator won't be registered until very late into the boot process and a regulator_get() call will almost certainly cause the simplefb driver to defer probing.

Quoted 6 lines

Right, this has been discussed already and the plan is to have simplefb continue its probe function and return success from it if it encounters any -eprobe_defer errors, while tracking which resources it misses.

And then have a late_initcall which will claim any resources which failed with -eprobe beforehand.

Quoted 7 lines

How do you ensure that the late_initcall gets run before any of the other late_initcalls that disable the resources?

Also my recollection is
that deferred probing will first be triggered the first time from a late_initcall, so chances aren't very high that all resources have shown up by that time.

So I just looked up the relevant code, and your right, this means that the whole model of "disable unused resources once probing is done" which we use for e.g. clocks, is already somewhat broken since there is no guarantee probing is really done when the cleanup code runs.

Quoted 17 lines

Quoted 8 lines

Quoted 4 lines

Now deferring probing is a real showstopper for simplefb, because not only does it make the framebuffer useless as early boot console, once probing is attempted again the clocks that it would have needed to acquire to keep going will already have been switched off, too.

That is not true, even with the current implementation, if all necessary drivers are built in, then simplefb will come up later, but it will still come up before the late_initcall which disables the clocks.

Yes, in the current implementation because clocks typically are registered very early and thus you don't hit the deferred probe. The same is not true for other types of resources where it's actually quite common to hit deferred probing (regulators is a very notorious one).

It doesn't matter whether a driver is built-in or not, once you hit deferred probing you lose.

Quoted 8 lines

Quoted 3 lines

Once we do the split probing described above (which is something which we plan to do when it becomes necessary), then simplefb will still come up early.

It will come up early but won't have acquired all the resources that it needs, so unless you somehow manage to order late_initcalls in exactly the way that you need them, the frameworks will still turn off what you haven't managed to claim yet.

If it is a resource which only shows up as a result of deferred probing, then it may very well not have been probed & registered yet, when the framework cleanup functions runs, and thus will not get turned off...

So yes you're right that deferred probing may cause issues, but it seems that this is not something simplefb specific, but rather a generic problem with deferred-probing vs subsys cleanup functions.

My view on this is simple, lets worry about this when we actually have a board which hits these issues, and then we'll see from there.

Hi,

Quoted 84 lines · Hans de Goede

Hi,

Quoted 38 lines · Thierry Reding

Quoted 30 lines · Hans de Goede

Hi,

Quoted 18 lines · Thierry Reding

Quoted 14 lines · Geert Uytterhoeven

Hi Maxime,

Quoted 9 lines · Maxime Ripard

Review

Quoted 5 lines

(quoted diff) +4 -1

-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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Quoted 2 lines

If it depends on clocks, it won't work as a module, as CCF will have disabled all unused clocks at that point.

Quoted 8 lines

If it does depend on anything beyond clocks it won't work at all. Clocks are special because they get set up very early at boot time. If it turns out that a simplefb ever needs a regulator to remain on, and that's even quite likely to happen eventually, it's going to fail miserably, because those regulators will typically be provided by a PMIC on an I2C bus. The regulator won't be registered until very late into the boot process and a regulator_get() call will almost certainly cause the simplefb driver to defer probing.

Quoted 6 lines

Right, this has been discussed already and the plan is to have simplefb continue its probe function and return success from it if it encounters any -eprobe_defer errors, while tracking which resources it misses.

And then have a late_initcall which will claim any resources which failed with -eprobe beforehand.

Quoted 7 lines

How do you ensure that the late_initcall gets run before any of the other late_initcalls that disable the resources?

Also my recollection is
that deferred probing will first be triggered the first time from a late_initcall, so chances aren't very high that all resources have shown up by that time.

So I just looked up the relevant code, and your right, this means that the whole model of "disable unused resources once probing is done" which we use for e.g. clocks, is already somewhat broken since there is no guarantee probing is really done when the cleanup code runs.

Quoted 17 lines

Quoted 8 lines

Quoted 4 lines

Now deferring probing is a real showstopper for simplefb, because not only does it make the framebuffer useless as early boot console, once probing is attempted again the clocks that it would have needed to acquire to keep going will already have been switched off, too.

That is not true, even with the current implementation, if all necessary drivers are built in, then simplefb will come up later, but it will still come up before the late_initcall which disables the clocks.

Yes, in the current implementation because clocks typically are registered very early and thus you don't hit the deferred probe. The same is not true for other types of resources where it's actually quite common to hit deferred probing (regulators is a very notorious one).

It doesn't matter whether a driver is built-in or not, once you hit deferred probing you lose.

Quoted 8 lines

Quoted 3 lines

Once we do the split probing described above (which is something which we plan to do when it becomes necessary), then simplefb will still come up early.

It will come up early but won't have acquired all the resources that it needs, so unless you somehow manage to order late_initcalls in exactly the way that you need them, the frameworks will still turn off what you haven't managed to claim yet.

If it is a resource which only shows up as a result of deferred probing, then it may very well not have been probed & registered yet, when the framework cleanup functions runs, and thus will not get turned off...

Quoted 6 lines

So yes you're right that deferred probing may cause issues, but it seems that this is not something simplefb specific, but rather a generic problem with deferred-probing vs subsys cleanup functions.

My view on this is simple, lets worry about this when we actually have a board which hits these issues, and then we'll see from there.

So thinking more about this, I think this is not that hard to fix.

First lets fix the generic conflict between eprobedefer and subsys cleanup functions. This can be done by:

1) Having a linked list of subsys cleanup functions to call in drivers/base/dd.c

2) Have subsystems register their cleanup function rather then using late_initcall to get it called

3) Have deferred_probe_work_func iterate over the list and call the cleanup functions once deferred_probe_active_list goes empty. It should also remove them once called, so that they only get called the first time deferred_probe_active_list goes empty (iow when the deferred probing of' build-in drivers is done).

This way cleanup functions actually get run when all probing of (buildin) drivers is done.

Then when we move simplefb to a 2 fase probe, we can simply register the framebuffer at the first probe call, and claim any resources we get, but return -eprobe_defer if some resources returned that themselves.

Then when simplefb_probe gets re-called, it can first check if it did not already register a fb, and if it did, it can use that and see which resources are missing, and only try to claim those. If some resources still return probe_defer, return eprobe_defer again, otherwise success.

Quoted 105 lines · Hans de Goede

Hi,

Quoted 84 lines · Hans de Goede

Hi,

Quoted 38 lines · Thierry Reding

Quoted 30 lines · Hans de Goede

Hi,

Quoted 18 lines · Thierry Reding

Quoted 14 lines · Geert Uytterhoeven

Hi Maxime,

Quoted 9 lines · Maxime Ripard

Review

Quoted 5 lines

(quoted diff) +4 -1

-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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Quoted 2 lines

If it depends on clocks, it won't work as a module, as CCF will have disabled all unused clocks at that point.

Quoted 8 lines

If it does depend on anything beyond clocks it won't work at all. Clocks are special because they get set up very early at boot time. If it turns out that a simplefb ever needs a regulator to remain on, and that's even quite likely to happen eventually, it's going to fail miserably, because those regulators will typically be provided by a PMIC on an I2C bus. The regulator won't be registered until very late into the boot process and a regulator_get() call will almost certainly cause the simplefb driver to defer probing.

Quoted 6 lines

Right, this has been discussed already and the plan is to have simplefb continue its probe function and return success from it if it encounters any -eprobe_defer errors, while tracking which resources it misses.

And then have a late_initcall which will claim any resources which failed with -eprobe beforehand.

Quoted 7 lines

How do you ensure that the late_initcall gets run before any of the other late_initcalls that disable the resources?

Also my recollection is
that deferred probing will first be triggered the first time from a late_initcall, so chances aren't very high that all resources have shown up by that time.

So I just looked up the relevant code, and your right, this means that the whole model of "disable unused resources once probing is done" which we use for e.g. clocks, is already somewhat broken since there is no guarantee probing is really done when the cleanup code runs.

Quoted 17 lines

Quoted 8 lines

Quoted 4 lines

Now deferring probing is a real showstopper for simplefb, because not only does it make the framebuffer useless as early boot console, once probing is attempted again the clocks that it would have needed to acquire to keep going will already have been switched off, too.

That is not true, even with the current implementation, if all necessary drivers are built in, then simplefb will come up later, but it will still come up before the late_initcall which disables the clocks.

Yes, in the current implementation because clocks typically are registered very early and thus you don't hit the deferred probe. The same is not true for other types of resources where it's actually quite common to hit deferred probing (regulators is a very notorious one).

It doesn't matter whether a driver is built-in or not, once you hit deferred probing you lose.

Quoted 8 lines

Quoted 3 lines

Once we do the split probing described above (which is something which we plan to do when it becomes necessary), then simplefb will still come up early.

It will come up early but won't have acquired all the resources that it needs, so unless you somehow manage to order late_initcalls in exactly the way that you need them, the frameworks will still turn off what you haven't managed to claim yet.

If it is a resource which only shows up as a result of deferred probing, then it may very well not have been probed & registered yet, when the framework cleanup functions runs, and thus will not get turned off...

Quoted 6 lines

So yes you're right that deferred probing may cause issues, but it seems that this is not something simplefb specific, but rather a generic problem with deferred-probing vs subsys cleanup functions.

My view on this is simple, lets worry about this when we actually have a board which hits these issues, and then we'll see from there.

So thinking more about this, I think this is not that hard to fix.

First lets fix the generic conflict between eprobedefer and subsys cleanup functions. This can be done by:

1) Having a linked list of subsys cleanup functions to call in drivers/base/dd.c

2) Have subsystems register their cleanup function rather then using late_initcall to get it called

Quoted 8 lines

3) Have deferred_probe_work_func iterate over the list and call the cleanup functions once deferred_probe_active_list goes empty. It should also remove them once called, so that they only get called the first time deferred_probe_active_list goes empty (iow when the deferred probing of' build-in drivers is done).

This way cleanup functions actually get run when all probing of (buildin) drivers is done.

That sounds like a reasonable solution to me.

Quoted 8 lines

Then when we move simplefb to a 2 fase probe, we can simply register the framebuffer at the first probe call, and claim any resources we get, but return -eprobe_defer if some resources returned that themselves.

Then when simplefb_probe gets re-called, it can first check if it did not already register a fb, and if it did, it can use that and see which resources are missing, and only try to claim those. If some resources still return probe_defer, return eprobe_defer again, otherwise success.

I'm not sure you can check that the fb was already registered. When the driver probe fails the driver core will pretty much remove any trace of the driver, so checking for an already-reagistered framebuffer isn't going to be easy.

But if you can manage to make that work it should give us a pretty good solution that should be easy to make use of by other drivers as well.

Quoted 20 lines · 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 <[email protected]>

---
 drivers/video/fbdev/simplefb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/video/fbdev/simplefb.c +0 -0

index be7d288..8c0c972 100644
@@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
 	platform_driver_unregister(&simplefb_driver);
 }
 

drivers/video/fbdev/simplefb.c static void __exit simplefb_exit(void) · line 421

Quoted 5 lines

drivers/video/fbdev/simplefb.c +4 -1

-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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Hi,

Quoted 21 lines · Maxime Ripard

Quoted 20 lines · 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 <[email protected]>

---
 drivers/video/fbdev/simplefb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/video/fbdev/simplefb.c +0 -0

index be7d288..8c0c972 100644
@@ -415,7 +415,11 @@ static void __exit simplefb_exit(void)
     platform_driver_unregister(&simplefb_driver);
 }
 

Quoted 9 lines

drivers/video/fbdev/simplefb.c static void __exit simplefb_exit(void) · line 421

Quoted 5 lines

drivers/video/fbdev/simplefb.c +4 -1

-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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

A valid point, my mean reasoning here is that some may see not being able to use it as a module as a regression, so I just kept things as is, but I do agree that it is advisable to just build it in.

Quoted 34 lines · Hans de Goede

Hi,

Quoted 30 lines · Maxime Ripard

Quoted 17 lines · 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 <[email protected]>

---
 drivers/video/fbdev/simplefb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/video/fbdev/simplefb.c +0 -0

index be7d288..8c0c972 100644

drivers/video/fbdev/simplefb.c static void __exit simplefb_exit(void) · line 421

Quoted 8 lines

drivers/video/fbdev/simplefb.c +4 -1

@@ -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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Quoted 3 lines

A valid point, my mean reasoning here is that some may see not being able to use it as a module as a regression, so I just kept things as is, but I do agree that it is advisable to just build it in.

Like a lot of things, if it is made a modules, and it breaks for the user, the user gets to keep the pieces. There are potentially some valid scenarios where it is fine to have it as a module. I don't recommend changing this unless is actually starts causing problems.

Quoted 35 lines · Grant Likely

Quoted 34 lines · Hans de Goede

Hi,

Quoted 30 lines · Maxime Ripard

Quoted 17 lines · 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 <[email protected]>

---
 drivers/video/fbdev/simplefb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/video/fbdev/simplefb.c +0 -0

index be7d288..8c0c972 100644

drivers/video/fbdev/simplefb.c static void __exit simplefb_exit(void) · line 421

Quoted 8 lines

drivers/video/fbdev/simplefb.c +4 -1

@@ -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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Quoted 8 lines

Quoted 3 lines

A valid point, my mean reasoning here is that some may see not being able to use it as a module as a regression, so I just kept things as is, but I do agree that it is advisable to just build it in.

Like a lot of things, if it is made a modules, and it breaks for the user, the user gets to keep the pieces. There are potentially some valid scenarios where it is fine to have it as a module. I don't recommend changing this unless is actually starts causing problems.

I assume that you've tested this and it actually makes a difference, correct? If so,

Acked-by Grant Likely <[email protected]>

Quoted 10 lines · Hans de Goede

Quoted 8 lines · Maxime Ripard

Quoted 4 lines

+/*
+ * 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
+ */

Not really related to this patch itself, but do we want to support simplefb as a module? It seems like it's going to be most of the time broken.

Quoted 3 lines

A valid point, my mean reasoning here is that some may see not being able to use it as a module as a regression, so I just kept things as is, but I do agree that it is advisable to just build it in.

Like a lot of things, if it is made a modules, and it breaks for the user, the user gets to keep the pieces. There are potentially some valid scenarios where it is fine to have it as a module. I don't recommend changing this unless is actually starts causing problems.

Like a lot of things, if it is made a modules, and it breaks for the user, the user gets to keep the pieces. There are potentially some

Quoted 2 lines

Like a lot of things, if it is made a modules, and it breaks for the user, the user gets to keep the pieces. There are potentially some

But the option's description should probably include some blurb about the fact that it only works as a module with some combinations of SoC+kernel.

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 <[email protected]>

---
 .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/video/simple-framebuffer.txt +0 -0

index 8f35718..95fe284 100644

Quoted 10 lines · 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.

Quoted 4 lines

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

Signed-off-by Hans de Goede <[email protected]>

Acked-by Geert Uytterhoeven <[email protected]>

Quoted 13 lines

---
 .../bindings/video/simple-framebuffer.txt          | 37 +++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/video/simple-framebuffer.txt +0 -0

index 8f35718..95fe284 100644
@@ -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.
 

Documentation/devicetree/bindings/video/simple-framebuffer.txt A simple frame-buffer describes a frame-buffer setup by firmware or · line 12

Quoted 6 lines

Documentation/devicetree/bindings/video/simple-framebuffer.txt +6 -0

+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.

You may want to add an stdout-path property to the example.