simplefb: add clock handling

Signed-off-by Luc Verhaegen <[email protected]>

---
 drivers/video/fbdev/simplefb.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

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

index 210f3a0..32be590 100644
@@ -41,6 +41,8 @@ static struct fb_var_screeninfo simplefb_var = {
        .vmode          = FB_VMODE_NONINTERLACED,
 };
 
+#define PSEUDO_PALETTE_SIZE 16
+
 static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
                              u_int transp, struct fb_info *info)
 {

@@ -50,7 +52,7 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
        u32 cb = blue >> (16 - info->var.blue.length);
        u32 value;
 
-       if (regno >= 16)
+       if (regno >= PSEUDO_PALETTE_SIZE)
                return -EINVAL;
 
        value = (cr << info->var.red.offset) |

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

 static int simplefb_probe(struct platform_device *pdev)
 {
        int ret;
        struct simplefb_params params;
        struct fb_info *info;
+       struct simplefb_par *par;
        struct resource *mem;
 
        if (fb_get_options("simplefb", NULL))

@@ -188,11 +195,13 @@ static int simplefb_probe(struct platform_device *pdev)
                return -EINVAL;
        }
 
-       info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev);
+       info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev);
        if (!info)
                return -ENOMEM;
        platform_set_drvdata(pdev, info);
 
+       par = info->par;
+
        info->fix = simplefb_fix;
        info->fix.smem_start = mem->start;
        info->fix.smem_len = resource_size(mem);

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

        dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
                             info->fix.smem_start, info->fix.smem_len,
--

1.7.7

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

drivers/video/fbdev/simplefb.c static int simplefb_probe(struct platform_device *pdev) · line 237

Quoted 6 lines

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

@@ -225,7 +234,7 @@ static int simplefb_probe(struct platform_device *pdev)
                framebuffer_release(info);
                return -ENODEV;
        }
-       info->pseudo_palette = (void *)(info + 1);
+       info->pseudo_palette = (void *) par->palette;

I think coding-style is this (i.e., no whitespace):

    info->pseudo_palette = (void*)par->palette;

Quoted 9 lines · David Herrmann

drivers/video/fbdev/simplefb.c static int simplefb_probe(struct platform_device *pdev) · line 237

Quoted 6 lines

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

@@ -225,7 +234,7 @@ static int simplefb_probe(struct platform_device *pdev)
                framebuffer_release(info);
                return -ENODEV;
        }
-       info->pseudo_palette = (void *)(info + 1);
+       info->pseudo_palette = (void *) par->palette;

I think coding-style is this (i.e., no whitespace):

    info->pseudo_palette = (void*)par->palette;

<casts-are-evil>
Is this cast even needed?
</casts-are-evil>

Signed-off-by Luc Verhaegen <[email protected]>

Signed-off-by Luc Verhaegen <[email protected]>

---
 drivers/video/fbdev/simplefb.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

drivers/video/fbdev/simplefb.c +5 -7

index 32be590..72a4f20 100644
@@ -220,8 +220,8 @@ static int simplefb_probe(struct platform_device *pdev)
 
        info->apertures = alloc_apertures(1);
        if (!info->apertures) {
-               framebuffer_release(info);
-               return -ENOMEM;
+               ret = -ENOMEM;
+               goto error_fb_release;
        }
        info->apertures->ranges[0].base = info->fix.smem_start;
        info->apertures->ranges[0].size = info->fix.smem_len;

@@ -231,8 +231,8 @@ static int simplefb_probe(struct platform_device *pdev)
        info->screen_base = ioremap_wc(info->fix.smem_start,
                                       info->fix.smem_len);
        if (!info->screen_base) {
-               framebuffer_release(info);
-               return -ENODEV;
+               ret = -ENODEV;
+               goto error_fb_release;
        }
        info->pseudo_palette = (void *) par->palette;
 

@@ -247,14 +247,20 @@ static int simplefb_probe(struct platform_device *pdev)
        ret = register_framebuffer(info);
        if (ret < 0) {
                dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
-               iounmap(info->screen_base);
-               framebuffer_release(info);
-               return ret;
+               goto error_unmap;
        }
 
        dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);

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

 }
 
 static int simplefb_remove(struct platform_device *pdev)
--

1.7.7

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

Hi

Quoted 42 lines · Luc Verhaegen

Signed-off-by Luc Verhaegen <[email protected]>

---
 drivers/video/fbdev/simplefb.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

drivers/video/fbdev/simplefb.c +5 -7

index 32be590..72a4f20 100644
@@ -220,8 +220,8 @@ static int simplefb_probe(struct platform_device *pdev)
 
        info->apertures = alloc_apertures(1);
        if (!info->apertures) {
-               framebuffer_release(info);
-               return -ENOMEM;
+               ret = -ENOMEM;
+               goto error_fb_release;
        }
        info->apertures->ranges[0].base = info->fix.smem_start;
        info->apertures->ranges[0].size = info->fix.smem_len;

@@ -231,8 +231,8 @@ static int simplefb_probe(struct platform_device *pdev)
        info->screen_base = ioremap_wc(info->fix.smem_start,
                                       info->fix.smem_len);
        if (!info->screen_base) {
-               framebuffer_release(info);
-               return -ENODEV;
+               ret = -ENODEV;
+               goto error_fb_release;
        }
        info->pseudo_palette = (void *) par->palette;
 

@@ -247,14 +247,20 @@ static int simplefb_probe(struct platform_device *pdev)
        ret = register_framebuffer(info);
        if (ret < 0) {
                dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
-               iounmap(info->screen_base);
-               framebuffer_release(info);
-               return ret;
+               goto error_unmap;
        }
 
        dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);

drivers/video/fbdev/simplefb.c static int simplefb_probe(struct platform_device *pdev) · line 263

Quoted 10 lines

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

 
        return 0;
+
+ error_unmap:
+       iounmap(info->screen_base);
+
+ error_fb_release:
+       framebuffer_release(info);
+
+       return ret;

Again, I'd use different coding-style, but I will leave that to Stephen and Tomi:

Reviewed-by David Herrmann <[email protected]>

Quoted 59 lines · David Herrmann

Hi

Quoted 42 lines · Luc Verhaegen

Signed-off-by Luc Verhaegen <[email protected]>

---
 drivers/video/fbdev/simplefb.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

drivers/video/fbdev/simplefb.c +5 -7

index 32be590..72a4f20 100644
@@ -220,8 +220,8 @@ static int simplefb_probe(struct platform_device *pdev)
 
        info->apertures = alloc_apertures(1);
        if (!info->apertures) {
-               framebuffer_release(info);
-               return -ENOMEM;
+               ret = -ENOMEM;
+               goto error_fb_release;
        }
        info->apertures->ranges[0].base = info->fix.smem_start;
        info->apertures->ranges[0].size = info->fix.smem_len;

@@ -231,8 +231,8 @@ static int simplefb_probe(struct platform_device *pdev)
        info->screen_base = ioremap_wc(info->fix.smem_start,
                                       info->fix.smem_len);
        if (!info->screen_base) {
-               framebuffer_release(info);
-               return -ENODEV;
+               ret = -ENODEV;
+               goto error_fb_release;
        }
        info->pseudo_palette = (void *) par->palette;
 

@@ -247,14 +247,20 @@ static int simplefb_probe(struct platform_device *pdev)
        ret = register_framebuffer(info);
        if (ret < 0) {
                dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
-               iounmap(info->screen_base);
-               framebuffer_release(info);
-               return ret;
+               goto error_unmap;
        }
 
        dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);

drivers/video/fbdev/simplefb.c static int simplefb_probe(struct platform_device *pdev) · line 263

Quoted 10 lines

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

 
        return 0;
+
+ error_unmap:
+       iounmap(info->screen_base);
+
+ error_fb_release:
+       framebuffer_release(info);
+
+       return ret;

Again, I'd use different coding-style, but I will leave that to Stephen and Tomi:

Quoted 1 line

Reviewed-by David Herrmann <[email protected]>

While the discussion about the last two patches rages on, can you state what coding style changes you would like to see here, as i am not clear as to what exactly is off with the above code.

Hi

I'd skip the leading whitespace and the newlines, like this:

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

+error_unmap:
+        iounmap(info->screen_base);
+error_fb_release:
+        framebuffer_release(info);
+        return ret;

at least that's my conception how we format error paths in drivers/video/.

The next commit will handle clocks correctly, so that these do not get automatically disabled on certain SoC simplefb implementations. As a result, the removal of this simplefb driver, and the release of the clocks, is rather final, and only a full display driver can work after this. So, it makes sense to also flag the dt node as disabled, even though it has no real value today.

Signed-off-by Luc Verhaegen <[email protected]>

---
 drivers/video/fbdev/simplefb.c |   43 ++++++++++++++++++++++++++++++++++++---
 1 files changed, 39 insertions(+), 4 deletions(-)

drivers/video/fbdev/simplefb.c +39 -5

index 72a4f20..74c4b2a 100644
@@ -138,6 +138,32 @@ static int simplefb_parse_dt(struct platform_device *pdev,
        return 0;
 }
 
+/*
+ * Make sure that nothing tries to inadvertedly re-use this node...
+ */
+static int
+simplefb_dt_disable(struct platform_device *pdev)
+{
+       struct device_node *np = pdev->dev.of_node;
+       struct property *property;
+       int ret;
+
+       property = kzalloc(sizeof(struct property), GFP_KERNEL);
+       if (!property)
+               return -ENOMEM;
+
+       property->name = "status";
+       property->value = "disabled";
+       property->length = strlen(property->value) + 1;
+
+       ret = of_update_property(np, property);
+       if (ret)
+               dev_err(&pdev->dev, "%s: failed to update property: %d\n",
+                       __func__, ret);
+
+       return ret;
+}
+
 static int simplefb_parse_pd(struct platform_device *pdev,
                             struct simplefb_params *params)
 {

@@ -187,17 +213,20 @@ static int simplefb_probe(struct platform_device *pdev)
                ret = simplefb_parse_dt(pdev, &params);
 
        if (ret)
-               return ret;
+               goto error_dt_disable;
 
        mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
        if (!mem) {
                dev_err(&pdev->dev, "No memory resource\n");
-               return -EINVAL;
+               ret = -EINVAL;
+               goto error_dt_disable;
        }
 
        info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev);
-       if (!info)
-               return -ENOMEM;
+       if (!info) {
+               ret = -ENOMEM;
+               goto error_dt_disable;
+       }
        platform_set_drvdata(pdev, info);
 
        par = info->par;

@@ -260,6 +289,10 @@ static int simplefb_probe(struct platform_device *pdev)
  error_fb_release:
        framebuffer_release(info);
 
+ error_dt_disable:
+       if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node)
+               simplefb_dt_disable(pdev);
+
        return ret;
 }
 

@@ -269,6 +302,8 @@ static int simplefb_remove(struct platform_device *pdev)
 
        unregister_framebuffer(info);
        framebuffer_release(info);
+       if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node)
+               simplefb_dt_disable(pdev);
 
        return 0;
 }
--

1.7.7

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 8 lines · Luc Verhaegen

The next commit will handle clocks correctly, so that these do not get automatically disabled on certain SoC simplefb implementations. As a result, the removal of this simplefb driver, and the release of the clocks, is rather final, and only a full display driver can work after this. So, it makes sense to also flag the dt node as disabled, even though it has no real value today.

Signed-off-by Luc Verhaegen <[email protected]>

Please, no.

Drivers should not be modifying the device tree without and exceptionally good reason for doing so. Drivers are to treat the DT as immutable.

* the exception is an overlay driver which add new devices to the kernel. Definitely not the case here.

Hi

Quoted 12 lines · Grant Likely

Quoted 8 lines · Luc Verhaegen

The next commit will handle clocks correctly, so that these do not get automatically disabled on certain SoC simplefb implementations. As a result, the removal of this simplefb driver, and the release of the clocks, is rather final, and only a full display driver can work after this. So, it makes sense to also flag the dt node as disabled, even though it has no real value today.

Signed-off-by Luc Verhaegen <[email protected]>

Please, no.

Quoted 6 lines

Drivers should not be modifying the device tree without and exceptionally good reason for doing so. Drivers are to treat the DT as immutable.

* the exception is an overlay driver which add new devices to the kernel. Definitely not the case here.

Why?

I think we have exactly that case:
* DT describes the real hw properly and those parts are immutable
* Additionally, bootloaders create firmware-framebuffers and create simple-framebuffer devices for them. Those are valid as long as no driver reconfigured the real hw.
* Once a real hw-driver loads, it might destroy the existing framebuffers, thus, it should also destroy the platform device.
* If the real hw-driver is unloaded, it might re-create the FB and thus create a new (or enable the old) platform device.

Or, in a nutshell: A "simple-framebuffer" device is basically a platform-device for framebuffers. Framebuffers can be created and destroyed during runtime. The reason we create platform-devices for them, is to allow dummy drivers to be probed. Real hw-drivers obviously bind to the parent bus device.

Other ideas are obviously welcome, but so far all of the other ideas sounded like big hacks (like remove_conflicting_framebuffers() so far..).

Thanks
David

Quoted 6 lines · David Herrmann

Quoted 2 lines · Grant Likely

* the exception is an overlay driver which add new devices to the kernel. Definitely not the case here.

Why?

The majority of the DT code is based on the assumption of a static tree. Pantelis has been working on being able to modify it at runtime with overlays, but he has had to go through a lot of rework because it is not a trivial task. When you get into modifying the DT, you need to have a lot more understanding of the side effects to changing the tree. The DT structure also has a lifecycle that can go beyond the current lifecycle of the kernel. The kexec tool will extract the current tree from the kernel, make the appropriate modifications, and use that to boot the next kernel. Allowing any driver to modify the tree has side effects beyond just the current kernel.

In this specific case, it will interact badly with the work Pantelis is doing to make platform devices work with overlays. Modifying the status property will cause the associated struct device to get removed in the middle of probing a driver for that device! That will most likely cause an oops.

Besides, Luc straight out *said*: "...even though it has no real value today". In what circumstance is that justification for modifying the tree?

The majority of the DT code is based on the assumption of a static tree. Pantelis has been working on being able to modify it at runtime with overlays, but he has had to go through a lot of rework because it is not a trivial task. When you get into modifying the DT, you need to have a lot more understanding of the side effects to changing the tree. The DT structure also has a lifecycle that can go beyond the current lifecycle of the kernel. The kexec tool will extract the current tree from the kernel, make the appropriate modifications, and use that to boot the next kernel. Allowing any driver to modify the tree has side effects beyond just the current kernel.

In this specific case, it will interact badly with the work Pantelis is doing to make platform devices work with overlays. Modifying the status property will cause the associated struct device to get removed in the middle of probing a driver for that device! That will most likely cause an oops.

Besides, Luc straight out *said*: "...even though it has no real value today". In what circumstance is that justification for modifying the tree?

Quoted 18 lines · Grant Likely

The majority of the DT code is based on the assumption of a static tree. Pantelis has been working on being able to modify it at runtime with overlays, but he has had to go through a lot of rework because it is not a trivial task. When you get into modifying the DT, you need to have a lot more understanding of the side effects to changing the tree. The DT structure also has a lifecycle that can go beyond the current lifecycle of the kernel. The kexec tool will extract the current tree from the kernel, make the appropriate modifications, and use that to boot the next kernel. Allowing any driver to modify the tree has side effects beyond just the current kernel.

In this specific case, it will interact badly with the work Pantelis is doing to make platform devices work with overlays. Modifying the status property will cause the associated struct device to get removed in the middle of probing a driver for that device! That will most likely cause an oops.

Quoted 3 lines

Besides, Luc straight out *said*: "...even though it has no real value today". In what circumstance is that justification for modifying the tree?

With that sentence i meant that given the current state of things, it has no real value.

It has no value currently as re-probing simplefb is not going to happen. But it's not a big leap to turn simplefb into a proper module. Not that that makes much sense, but that's never stopped anyone.

To me it seemed simple, dt is what drives simplefb, so dt then also becomes responsible for making sure that simplefb or another driver does not attempt to blindly use this info again. The way this is implemented i do not care for in any way, i just knew that i could not do nothing here, given the catastrophic effect disabling the clocks has on simplefb on sunxi. Given the discussion that errupted here, i'd say that this does need some resolution, and altering the dt is going to have to be part of the solution.

In any case, i will gladly drop this patch, as it is not absolutely necessary. But it should be very clear that there is no going back on this dt node after the clocks were released once.

Luc Verhaegen.

Quoted 39 lines · Luc Verhaegen

Quoted 18 lines · Grant Likely

The majority of the DT code is based on the assumption of a static tree. Pantelis has been working on being able to modify it at runtime with overlays, but he has had to go through a lot of rework because it is not a trivial task. When you get into modifying the DT, you need to have a lot more understanding of the side effects to changing the tree. The DT structure also has a lifecycle that can go beyond the current lifecycle of the kernel. The kexec tool will extract the current tree from the kernel, make the appropriate modifications, and use that to boot the next kernel. Allowing any driver to modify the tree has side effects beyond just the current kernel.

In this specific case, it will interact badly with the work Pantelis is doing to make platform devices work with overlays. Modifying the status property will cause the associated struct device to get removed in the middle of probing a driver for that device! That will most likely cause an oops.

Quoted 3 lines

Besides, Luc straight out *said*: "...even though it has no real value today". In what circumstance is that justification for modifying the tree?

With that sentence i meant that given the current state of things, it has no real value.

It has no value currently as re-probing simplefb is not going to happen. But it's not a big leap to turn simplefb into a proper module. Not that that makes much sense, but that's never stopped anyone.

To me it seemed simple, dt is what drives simplefb, so dt then also becomes responsible for making sure that simplefb or another driver does not attempt to blindly use this info again. The way this is implemented i do not care for in any way, i just knew that i could not do nothing here, given the catastrophic effect disabling the clocks has on simplefb on sunxi. Given the discussion that errupted here, i'd say that this does need some resolution, and altering the dt is going to have to be part of the solution.

Quoted 5 lines

In any case, i will gladly drop this patch, as it is not absolutely necessary. But it should be very clear that there is no going back on this dt node after the clocks were released once.

Luc Verhaegen.

What about approaching this from the other end? U-Boot could add a property named "once-only" or so.

Luc Verhaegen.

--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com. For more options, visit https://groups.google.com/d/optout.

Quoted 46 lines · Luc Verhaegen

Quoted 44 lines · Luc Verhaegen

Quoted 18 lines · Grant Likely

The majority of the DT code is based on the assumption of a static tree. Pantelis has been working on being able to modify it at runtime with overlays, but he has had to go through a lot of rework because it is not a trivial task. When you get into modifying the DT, you need to have a lot more understanding of the side effects to changing the tree. The DT structure also has a lifecycle that can go beyond the current lifecycle of the kernel. The kexec tool will extract the current tree from the kernel, make the appropriate modifications, and use that to boot the next kernel. Allowing any driver to modify the tree has side effects beyond just the current kernel.

In this specific case, it will interact badly with the work Pantelis is doing to make platform devices work with overlays. Modifying the status property will cause the associated struct device to get removed in the middle of probing a driver for that device! That will most likely cause an oops.

Quoted 3 lines

Besides, Luc straight out *said*: "...even though it has no real value today". In what circumstance is that justification for modifying the tree?

With that sentence i meant that given the current state of things, it has no real value.

It has no value currently as re-probing simplefb is not going to happen. But it's not a big leap to turn simplefb into a proper module. Not that that makes much sense, but that's never stopped anyone.

To me it seemed simple, dt is what drives simplefb, so dt then also becomes responsible for making sure that simplefb or another driver does not attempt to blindly use this info again. The way this is implemented i do not care for in any way, i just knew that i could not do nothing here, given the catastrophic effect disabling the clocks has on simplefb on sunxi. Given the discussion that errupted here, i'd say that this does need some resolution, and altering the dt is going to have to be part of the solution.

In any case, i will gladly drop this patch, as it is not absolutely necessary. But it should be very clear that there is no going back on this dt node after the clocks were released once.

Luc Verhaegen.

Quoted 2 lines

What about approaching this from the other end? U-Boot could add a property named "once-only" or so.

Device tree is supposed to be a static description of the hardware usable on all operating systems. It is the wrong mechanism for communicating between uboot and the kernel. Use something like atags or the kernel command line to tell the kernel that the console has already been set up.

The switch over from simple to KMS should not be done via a node add/del to the device tree either. No one has removed the device from the system, the device tree should not be changing.

BTW, Is anyone currently using the simple framebuffer for early console? For early console we would want to start using it well before setting up platform devices.

Quoted 4 lines · Grant Likely

BTW, Is anyone currently using the simple framebuffer for early console? For early console we would want to start using it well before setting up platform devices.

The code that sets up simplefb for sunxi is primarily for providing a console in u-boot (using the ancient cfbconsole infrastructure). From what i can tell, the u-boot code for rpi is about showing a splash screen (using the much newer lcd infrastructure).

Quoted 5 lines · Luc Verhaegen

The code that sets up simplefb for sunxi is primarily for providing a console in u-boot (using the ancient cfbconsole infrastructure). From what i can tell, the u-boot code for rpi is about showing a splash screen (using the much newer lcd infrastructure).

There's no splash screen on the Pi in the upstream U-Boot code. The LCD displays the U-Boot console/stdout. If USB support for the Pi's USB host is ever sent upstream, that will allow the user to use USB/LCD for U-Boot control, rather than serial.

Quoted 4 lines · Grant Likely

BTW, Is anyone currently using the simple framebuffer for early console? For early console we would want to start using it well before setting up platform devices.

The code that sets up simplefb for sunxi is primarily for providing a console in u-boot (using the ancient cfbconsole infrastructure). From what i can tell, the u-boot code for rpi is about showing a splash screen (using the much newer lcd infrastructure).

With u-boot showing a console, it really seemed only a small step to add simplefb. And quite a few people in our sunxi community are interested in it, primarily for u-boot and early console, and only secondarily as a stop-gap for a full driver.

Both of which make sense and should be supported, so I agree we need to find a solution.

The problem I think comes down to the handoff mechanism. There are a lot of different video controllers which could all be configured for a simple framebuffer.

I don't think the "run this only once" test is the right approach. Sometimes the simple framebuffer will never be torn down. It is conceivable that a simple framebuffer will get /added/ at runtime with an overlay, or even rebound to the driver. The simple framebuffer driver really needs to be explicitly told from outside itself that it is being taken over since it doesn't actually have the information to know when a framebuffer becomes invalid. Only the real video driver can provide that information.

How does the sunxi driver currently take over from the simplefb?

Quoted 47 lines · jonsmirl at gmail.com

Quoted 46 lines · Luc Verhaegen

Quoted 44 lines · Luc Verhaegen

Quoted 18 lines · Grant Likely

The majority of the DT code is based on the assumption of a static tree. Pantelis has been working on being able to modify it at runtime with overlays, but he has had to go through a lot of rework because it is not a trivial task. When you get into modifying the DT, you need to have a lot more understanding of the side effects to changing the tree. The DT structure also has a lifecycle that can go beyond the current lifecycle of the kernel. The kexec tool will extract the current tree from the kernel, make the appropriate modifications, and use that to boot the next kernel. Allowing any driver to modify the tree has side effects beyond just the current kernel.

In this specific case, it will interact badly with the work Pantelis is doing to make platform devices work with overlays. Modifying the status property will cause the associated struct device to get removed in the middle of probing a driver for that device! That will most likely cause an oops.

Quoted 3 lines

Besides, Luc straight out *said*: "...even though it has no real value today". In what circumstance is that justification for modifying the tree?

With that sentence i meant that given the current state of things, it has no real value.

It has no value currently as re-probing simplefb is not going to happen. But it's not a big leap to turn simplefb into a proper module. Not that that makes much sense, but that's never stopped anyone.

To me it seemed simple, dt is what drives simplefb, so dt then also becomes responsible for making sure that simplefb or another driver does not attempt to blindly use this info again. The way this is implemented i do not care for in any way, i just knew that i could not do nothing here, given the catastrophic effect disabling the clocks has on simplefb on sunxi. Given the discussion that errupted here, i'd say that this does need some resolution, and altering the dt is going to have to be part of the solution.

In any case, i will gladly drop this patch, as it is not absolutely necessary. But it should be very clear that there is no going back on this dt node after the clocks were released once.

Luc Verhaegen.

Quoted 8 lines

Quoted 2 lines

What about approaching this from the other end? U-Boot could add a property named "once-only" or so.

Device tree is supposed to be a static description of the hardware usable on all operating systems. It is the wrong mechanism for communicating between uboot and the kernel. Use something like atags or the kernel command line to tell the kernel that the console has already been set up.

Not accurate. While it is primarily hardware description, it is also used for firmware communication. There is loads of precedence for this. The /chosen node is the most significant example, but there are other places where the tree is used to provide state. For example, the current-speed property on UART nodes.

Quoted 4 lines

The switch over from simple to KMS should not be done via a node add/del to the device tree either. No one has removed the device from the system, the device tree should not be changing.

The simple-framebuffer binding appears to be insufficient in this regard in that it doesn't have any linkage with the actual device providing the framebuffer. Ideally, I would put the simple framebuffer state directly into the video device node and use the chosen node to point to the stdout device (probably with the stdout-path property). Then the driver already knows it can just ignore the simple properties because it owns the device node when it binds.

That said, simple-framebuffer as it stands is in use so we're not going to deprecate it. I would like to see an addition that specifies how a controller can be associated with a simple framebuffer node.

BTW, Is anyone currently using the simple framebuffer for early console? For early console we would want to start using it well before setting up platform devices.

g.

Quoted 57 lines · Grant Likely

Quoted 47 lines · jonsmirl at gmail.com

Quoted 46 lines · Luc Verhaegen

Quoted 44 lines · Luc Verhaegen

Quoted 18 lines · Grant Likely

The majority of the DT code is based on the assumption of a static tree. Pantelis has been working on being able to modify it at runtime with overlays, but he has had to go through a lot of rework because it is not a trivial task. When you get into modifying the DT, you need to have a lot more understanding of the side effects to changing the tree. The DT structure also has a lifecycle that can go beyond the current lifecycle of the kernel. The kexec tool will extract the current tree from the kernel, make the appropriate modifications, and use that to boot the next kernel. Allowing any driver to modify the tree has side effects beyond just the current kernel.

In this specific case, it will interact badly with the work Pantelis is doing to make platform devices work with overlays. Modifying the status property will cause the associated struct device to get removed in the middle of probing a driver for that device! That will most likely cause an oops.

Quoted 3 lines

Besides, Luc straight out *said*: "...even though it has no real value today". In what circumstance is that justification for modifying the tree?

With that sentence i meant that given the current state of things, it has no real value.

It has no value currently as re-probing simplefb is not going to happen. But it's not a big leap to turn simplefb into a proper module. Not that that makes much sense, but that's never stopped anyone.

To me it seemed simple, dt is what drives simplefb, so dt then also becomes responsible for making sure that simplefb or another driver does not attempt to blindly use this info again. The way this is implemented i do not care for in any way, i just knew that i could not do nothing here, given the catastrophic effect disabling the clocks has on simplefb on sunxi. Given the discussion that errupted here, i'd say that this does need some resolution, and altering the dt is going to have to be part of the solution.

In any case, i will gladly drop this patch, as it is not absolutely necessary. But it should be very clear that there is no going back on this dt node after the clocks were released once.

Luc Verhaegen.

Quoted 8 lines

Quoted 2 lines

What about approaching this from the other end? U-Boot could add a property named "once-only" or so.

Device tree is supposed to be a static description of the hardware usable on all operating systems. It is the wrong mechanism for communicating between uboot and the kernel. Use something like atags or the kernel command line to tell the kernel that the console has already been set up.

Quoted 5 lines

Not accurate. While it is primarily hardware description, it is also used for firmware communication. There is loads of precedence for this. The /chosen node is the most significant example, but there are other places where the tree is used to provide state. For example, the current-speed property on UART nodes.

I do seem to recall you telling me a long time ago that those chosen nodes were a mistake (or maybe it was Matt Sealey). I'm pretty wary of opening to door to device trees carrying a bunch of state. Five years from now the DT is going to look like a Christmas tree.

Quoted 64 lines · jonsmirl at gmail.com

Quoted 62 lines · Grant Likely

Quoted 47 lines · jonsmirl at gmail.com

Quoted 46 lines · Luc Verhaegen

Quoted 44 lines · Luc Verhaegen

Quoted 18 lines · Grant Likely

The majority of the DT code is based on the assumption of a static tree. Pantelis has been working on being able to modify it at runtime with overlays, but he has had to go through a lot of rework because it is not a trivial task. When you get into modifying the DT, you need to have a lot more understanding of the side effects to changing the tree. The DT structure also has a lifecycle that can go beyond the current lifecycle of the kernel. The kexec tool will extract the current tree from the kernel, make the appropriate modifications, and use that to boot the next kernel. Allowing any driver to modify the tree has side effects beyond just the current kernel.

In this specific case, it will interact badly with the work Pantelis is doing to make platform devices work with overlays. Modifying the status property will cause the associated struct device to get removed in the middle of probing a driver for that device! That will most likely cause an oops.

Quoted 3 lines

Besides, Luc straight out *said*: "...even though it has no real value today". In what circumstance is that justification for modifying the tree?

With that sentence i meant that given the current state of things, it has no real value.

It has no value currently as re-probing simplefb is not going to happen. But it's not a big leap to turn simplefb into a proper module. Not that that makes much sense, but that's never stopped anyone.

To me it seemed simple, dt is what drives simplefb, so dt then also becomes responsible for making sure that simplefb or another driver does not attempt to blindly use this info again. The way this is implemented i do not care for in any way, i just knew that i could not do nothing here, given the catastrophic effect disabling the clocks has on simplefb on sunxi. Given the discussion that errupted here, i'd say that this does need some resolution, and altering the dt is going to have to be part of the solution.

In any case, i will gladly drop this patch, as it is not absolutely necessary. But it should be very clear that there is no going back on this dt node after the clocks were released once.

Luc Verhaegen.

Quoted 8 lines

Quoted 2 lines

What about approaching this from the other end? U-Boot could add a property named "once-only" or so.

Device tree is supposed to be a static description of the hardware usable on all operating systems. It is the wrong mechanism for communicating between uboot and the kernel. Use something like atags or the kernel command line to tell the kernel that the console has already been set up.

Not accurate. While it is primarily hardware description, it is also used for firmware communication. There is loads of precedence for this. The /chosen node is the most significant example, but there are other places where the tree is used to provide state. For example, the current-speed property on UART nodes.

Quoted 4 lines

I do seem to recall you telling me a long time ago that those chosen nodes were a mistake (or maybe it was Matt Sealey). I'm pretty wary of opening to door to device trees carrying a bunch of state. Five years from now the DT is going to look like a Christmas tree.

Wasn't me. Carrying state in the DT provided by firmware is perfectly reasonable in my opinion.

g.

So what are the rules going to be? Once the OS is up and starts changing things the state in the DT and the OS are going to diverge. How does this get handled for a kernel driver that can load/unload? Should the kernel remove those state nodes as soon as it alters the state?

tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl at gmail.com:

Quoted 5 lines

So what are the rules going to be? Once the OS is up and starts changing things the state in the DT and the OS are going to diverge. How does this get handled for a kernel driver that can load/unload? Should the kernel remove those state nodes as soon as it alters the state?

hardware that is no longer there should also not be represented/active in DT.

A simple fb that have been torn down is no more, no different from never having been there. It can be argued that it never was in the first place (i.e. that it is not actually hardware) but that it another can of worms and both have their benefits and drawbacks. One thing is certain however, as far as simplefb is concerned it is hardware, not really any different from a persistent framebuffer with hardwired settings in hardware.

Regarding kexec, it's the responsibility of the kernel doing kexec to make sure DT and hardware matches for the next started kernel. With hardware that can be reconfigured it is not safe to assume that the DT can be passed as-is from before the kernel reconfigured hardware. If the currently running kernel wants to set up a framebuffer for simplefb use by the next kernel than it's free to do so, but in either case it needs to provide the right information to next kernel.

If it was given a simplefb node, but then killed the simplefb then no simplefb node should be provided to the next kernel. If it was not given a simplefb node, but have configured the hardware suitable for simplefb node then it may provide a simplefb node.

A UART that have changed rate no longer have that rate.

Same applies to numerous other items as well. If a kernel remaps IRQs, bus addresses, or whatever in hardware aspects where such changes is possible then DT needs to be updated as well. A bit less common than a simplefb being destroyed by driver reconfiguring the framebuffer, but if you look at it from a little distance then the problem is exactly the same. Hardware is not 100% static, and therefore hardware description also can not be 100% static. And hardware is getting more and more dynamically reconfigurable.

A DT that contains false information is generally worse than no DT / not having that information at all.

Regards
Henrik

Quoted 11 lines · Henrik Nordstr?m

tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl at gmail.com:

Quoted 5 lines

So what are the rules going to be? Once the OS is up and starts changing things the state in the DT and the OS are going to diverge. How does this get handled for a kernel driver that can load/unload? Should the kernel remove those state nodes as soon as it alters the state?

hardware that is no longer there should also not be represented/active in DT.

Quoted 7 lines

A simple fb that have been torn down is no more, no different from never having been there. It can be argued that it never was in the first place (i.e. that it is not actually hardware) but that it another can of worms and both have their benefits and drawbacks. One thing is certain however, as far as simplefb is concerned it is hardware, not really any different from a persistent framebuffer with hardwired settings in hardware.

How does the kernel know what clocks to protect? A list in the simplefb node? This list of clocks is a reason for adding simplefb to the compatible list for the real hardware. That way it won't be duplicated.

The issues with parameter conflicts between simplefb and the real hardware can be dealt with. If the real hardware wants to add the simplefb compatible field it will need to get its parameters sorted out so that they don't conflict. Clock syntax is standardized in the DTS so it shouldn't be a problem.

Quoted 26 lines · jonsmirl at gmail.com

Quoted 11 lines · Henrik Nordstr?m

tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl at gmail.com:

Quoted 5 lines

So what are the rules going to be? Once the OS is up and starts changing things the state in the DT and the OS are going to diverge. How does this get handled for a kernel driver that can load/unload? Should the kernel remove those state nodes as soon as it alters the state?

hardware that is no longer there should also not be represented/active in DT.

Quoted 7 lines

A simple fb that have been torn down is no more, no different from never having been there. It can be argued that it never was in the first place (i.e. that it is not actually hardware) but that it another can of worms and both have their benefits and drawbacks. One thing is certain however, as far as simplefb is concerned it is hardware, not really any different from a persistent framebuffer with hardwired settings in hardware.

How does the kernel know what clocks to protect? A list in the simplefb node? This list of clocks is a reason for adding simplefb to the compatible list for the real hardware. That way it won't be duplicated.

Quoted 5 lines

The issues with parameter conflicts between simplefb and the real hardware can be dealt with. If the real hardware wants to add the simplefb compatible field it will need to get its parameters sorted out so that they don't conflict. Clock syntax is standardized in the DTS so it shouldn't be a problem.

It shouldn't be, but apparently, some disagree.

Anyway, I'm not sure having the simplefb compatible would work in this use-case, mainly for two reasons:
- Most likely, the bindings are going to be very different. Not only about which properties you'll have, but also what you will place in these properties. reg for example have the memory address of the buffer in the simplefb case, while in the KMS driver case, it would have the memory address of the registers.
- There's will be a single driver probed. So if you want to go down the hand over road (which is a bit premature at this point if you ask me, but anyway), you will have no driver probed to hand over to.

Maxime

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

Hi

Quoted 10 lines · Grant Likely

Quoted 8 lines · Luc Verhaegen

The next commit will handle clocks correctly, so that these do not get automatically disabled on certain SoC simplefb implementations. As a result, the removal of this simplefb driver, and the release of the clocks, is rather final, and only a full display driver can work after this. So, it makes sense to also flag the dt node as disabled, even though it has no real value today.

Signed-off-by Luc Verhaegen <[email protected]>

Quoted 8 lines

Please, no.

Drivers should not be modifying the device tree without and exceptionally good reason for doing so. Drivers are to treat the DT as immutable.

* the exception is an overlay driver which add new devices to the kernel. Definitely not the case here.

Why? I think we have exactly that case:
* DT describes the real hw properly and those parts are immutable
* Additionally, bootloaders create firmware-framebuffers and create simple-framebuffer devices for them. Those are valid as long as no driver reconfigured the real hw.
* Once a real hw-driver loads, it might destroy the existing framebuffers, thus, it should also destroy the platform device.
* If the real hw-driver is unloaded, it might re-create the FB and thus create a new (or enable the old) platform device.

Quoted 28 lines · David Herrmann

Hi

Quoted 13 lines · Grant Likely

Quoted 9 lines · Luc Verhaegen

The next commit will handle clocks correctly, so that these do not get automatically disabled on certain SoC simplefb implementations. As a result, the removal of this simplefb driver, and the release of the clocks, is rather final, and only a full display driver can work after this. So, it makes sense to also flag the dt node as disabled, even though it has no real value today.

Signed-off-by Luc Verhaegen <[email protected]>

Quoted 8 lines

Please, no.

Drivers should not be modifying the device tree without and exceptionally good reason for doing so. Drivers are to treat the DT as immutable.

* the exception is an overlay driver which add new devices to the kernel. Definitely not the case here.

Quoted 9 lines

Why? I think we have exactly that case:
* DT describes the real hw properly and those parts are immutable
* Additionally, bootloaders create firmware-framebuffers and create simple-framebuffer devices for them. Those are valid as long as no driver reconfigured the real hw.
* Once a real hw-driver loads, it might destroy the existing framebuffers, thus, it should also destroy the platform device.
* If the real hw-driver is unloaded, it might re-create the FB and thus create a new (or enable the old) platform device.

My intention was always that a bootloader's addition of a simplefb node to the DT would be user-configurable or driven by the original DT content. As such, there shouldn't ever be both a DT node describing the "real" HW and simplefb. In other words, if the DT already has the "real" DT node, the bootloader should automatically (or under user command) not add the simpefb node.

--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Quoted 40 lines · Stephen Warren

Quoted 28 lines · David Herrmann

Hi

Quoted 13 lines · Grant Likely

Quoted 9 lines · Luc Verhaegen

The next commit will handle clocks correctly, so that these do not get automatically disabled on certain SoC simplefb implementations. As a result, the removal of this simplefb driver, and the release of the clocks, is rather final, and only a full display driver can work after this. So, it makes sense to also flag the dt node as disabled, even though it has no real value today.

Signed-off-by Luc Verhaegen <[email protected]>

Quoted 8 lines

Please, no.

Drivers should not be modifying the device tree without and exceptionally good reason for doing so. Drivers are to treat the DT as immutable.

* the exception is an overlay driver which add new devices to the kernel. Definitely not the case here.

Quoted 9 lines

Why? I think we have exactly that case:
* DT describes the real hw properly and those parts are immutable
* Additionally, bootloaders create firmware-framebuffers and create simple-framebuffer devices for them. Those are valid as long as no driver reconfigured the real hw.
* Once a real hw-driver loads, it might destroy the existing framebuffers, thus, it should also destroy the platform device.
* If the real hw-driver is unloaded, it might re-create the FB and thus create a new (or enable the old) platform device.

Quoted 6 lines

My intention was always that a bootloader's addition of a simplefb node to the DT would be user-configurable or driven by the original DT content. As such, there shouldn't ever be both a DT node describing the "real" HW and simplefb. In other words, if the DT already has the "real" DT node, the bootloader should automatically (or under user command) not add the simpefb node.

DT is just the wrong mechanism to signal this, use an ATAG or kernel command line parameter.

real hardware has compatible = "real-hardware-name, simplefb"

simplefb is built into kernel. It will attach to the device because of the compatible string. Then it can look at the command line and see if the bootloader also supported simplefb and already set things up.

Then some mechanism will have to be designed to arrange a handoff between simplefb and the chip specific KMS driver. But that's a Linux problem, not a DT one.

Quoted 66 lines · jonsmirl at gmail.com

Quoted 47 lines · Stephen Warren

Quoted 33 lines · David Herrmann

Hi

Quoted 16 lines · Grant Likely

Quoted 10 lines · Luc Verhaegen

The next commit will handle clocks correctly, so that these do not get automatically disabled on certain SoC simplefb implementations. As a result, the removal of this simplefb driver, and the release of the clocks, is rather final, and only a full display driver can work after this. So, it makes sense to also flag the dt node as disabled, even though it has no real value today.

Signed-off-by Luc Verhaegen <[email protected]>

Quoted 8 lines

Please, no.

Drivers should not be modifying the device tree without and exceptionally good reason for doing so. Drivers are to treat the DT as immutable.

* the exception is an overlay driver which add new devices to the kernel. Definitely not the case here.

Quoted 9 lines

Why? I think we have exactly that case:
* DT describes the real hw properly and those parts are immutable
* Additionally, bootloaders create firmware-framebuffers and create simple-framebuffer devices for them. Those are valid as long as no driver reconfigured the real hw.
* Once a real hw-driver loads, it might destroy the existing framebuffers, thus, it should also destroy the platform device.
* If the real hw-driver is unloaded, it might re-create the FB and thus create a new (or enable the old) platform device.

Quoted 9 lines

My intention was always that a bootloader's addition of a simplefb node to
the DT would be user-configurable or driven by the original DT content. As
such, there shouldn't ever be both a DT node describing the "real" HW and simplefb. In other words, if the DT already has the "real" DT node, the bootloader should automatically (or under user command) not add the simpefb
node.

DT is just the wrong mechanism to signal this, use an ATAG or kernel command line parameter.

real hardware has compatible = "real-hardware-name, simplefb"

Quoted 7 lines

simplefb is built into kernel. It will attach to the device because of the compatible string. Then it can look at the command line and see if the bootloader also supported simplefb and already set things up.

Then some mechanism will have to be designed to arrange a handoff between simplefb and the chip specific KMS driver. But that's a Linux problem, not a DT one.

Having a single DT node that conforms to both the binding for "real-hardware-name" and "simplefb" doesn't seem like a good approach. What if the properties required by the two bindings conflict in some way? The approach you advocate certainly hasn't ever been used AFAIK.

This claims and enables clocks listed in the simple framebuffer dt node. This is needed so that the display engine, in case the required clocks are known by the kernel code and are described in the dt, will remain properly enabled.

I think this make simplefb not simple any more, which rather goes against the whole point of it.

I specifically conceived simplefb to know about nothing more than the memory address and pixel layout of the memory buffer. I certainly don't like the idea of expanding simplefb to anything beyond that, and IIRC *not* extending is was a condition agreed when it was first merged. If more knowledge than that is required, then there needs to be a HW-specific driver to manage any clocks/resets/video registers, etc.

The correct way to handle this without a complete DRM/KMS/... driver is to avoid the clocks in question being turned off at boot. I thought there was a per-clock flag to prevent disabling an unused clock? If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?). For example, the Tegra clock driver has a clock initialization table which IIRC was used for this purpose before we got a DRM/KMS driver. That way, all the details are kept inside the kernel code, and don't end up influencing the DT representation of simplefb.

Quoted 4 lines · Luc Verhaegen

This claims and enables clocks listed in the simple framebuffer dt node. This is needed so that the display engine, in case the required clocks are known by the kernel code and are described in the dt, will remain properly enabled.

I think this make simplefb not simple any more, which rather goes against the whole point of it.

I specifically conceived simplefb to know about nothing more than the memory address and pixel layout of the memory buffer. I certainly don't like the idea of expanding simplefb to anything beyond that, and IIRC *not* extending is was a condition agreed when it was first merged. If more knowledge than that is required, then there needs to be a HW-specific driver to manage any clocks/resets/video registers, etc.

The correct way to handle this without a complete DRM/KMS/... driver is to avoid the clocks in question being turned off at boot.

I thought there was a per-clock flag to prevent disabling an unused clock?

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

For example, the Tegra clock driver has a clock initialization table which IIRC was used for this purpose before we got a DRM/KMS driver. That way, all the details are kept inside the kernel code, and don't end up influencing the DT representation of simplefb.

Quoted 9 lines · Stephen Warren

Quoted 4 lines · Luc Verhaegen

This claims and enables clocks listed in the simple framebuffer dt node. This is needed so that the display engine, in case the required clocks are known by the kernel code and are described in the dt, will remain properly enabled.

I think this make simplefb not simple any more, which rather goes against the whole point of it.

Quoted 7 lines

I specifically conceived simplefb to know about nothing more than the memory address and pixel layout of the memory buffer. I certainly don't like the idea of expanding simplefb to anything beyond that, and IIRC *not* extending is was a condition agreed when it was first merged. If more knowledge than that is required, then there needs to be a HW-specific driver to manage any clocks/resets/video registers, etc.

I'm sorry, but how is that not simple? clocks enabling is step 1 in a driver in order to communicate somehow with the controller. Reset is a different story, because arguably, if simplefb is there, the controller is already out of reset.

And I don't see why video registers are coming into the discussion here. The code Luc posted doesn't access any register, at all. It just makes sure the needed controller keep going.

Quoted 2 lines

The correct way to handle this without a complete DRM/KMS/... driver is to avoid the clocks in question being turned off at boot.

Which is exactly what this code does, using the generic DT bindings to express dependency for a given clock. How is this wrong?

Quoted 2 lines

I thought there was a per-clock flag to prevent disabling an unused clock?

No, last time I heard, Mike Turquette was against it.

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Quoted 4 lines

For example, the Tegra clock driver has a clock initialization table which IIRC was used for this purpose before we got a DRM/KMS driver. That way, all the details are kept inside the kernel code, and don't end up influencing the DT representation of simplefb.

I don't really see how the optional usage of a generic property influences badly the DT representation of simplefb.

Hi,

Quoted 48 lines · Maxime Ripard

Quoted 9 lines · Stephen Warren

Quoted 4 lines · Luc Verhaegen

This claims and enables clocks listed in the simple framebuffer dt node. This is needed so that the display engine, in case the required clocks are known by the kernel code and are described in the dt, will remain properly enabled.

I think this make simplefb not simple any more, which rather goes against the whole point of it.

Quoted 7 lines

I specifically conceived simplefb to know about nothing more than the memory address and pixel layout of the memory buffer. I certainly don't like the idea of expanding simplefb to anything beyond that, and IIRC *not* extending is was a condition agreed when it was first merged. If more knowledge than that is required, then there needs to be a HW-specific driver to manage any clocks/resets/video registers, etc.

I'm sorry, but how is that not simple? clocks enabling is step 1 in a driver in order to communicate somehow with the controller. Reset is a different story, because arguably, if simplefb is there, the controller is already out of reset.

And I don't see why video registers are coming into the discussion here. The code Luc posted doesn't access any register, at all. It just makes sure the needed controller keep going.

Quoted 2 lines

The correct way to handle this without a complete DRM/KMS/... driver is to avoid the clocks in question being turned off at boot.

Which is exactly what this code does, using the generic DT bindings to express dependency for a given clock. How is this wrong?

Quoted 2 lines

I thought there was a per-clock flag to prevent disabling an unused clock?

No, last time I heard, Mike Turquette was against it.

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Quoted 7 lines

Quoted 4 lines

For example, the Tegra clock driver has a clock initialization table which IIRC was used for this purpose before we got a DRM/KMS driver. That way, all the details are kept inside the kernel code, and don't end up influencing the DT representation of simplefb.

I don't really see how the optional usage of a generic property influences badly the DT representation of simplefb.

+1 to all that Maxime said.

Also as can be seen in other discussion on this patch set, simplefb should not be seen as something orthogonal to having a full kms driver.

So just write a full kms driver is not the answer IMHO. What we want is for a bootloader setup console to be available through simplefb bindings so that the kernel can show output without depending on module loading, and thus can show errors if things go bad before a kms driver gets loaded.

And no build kms into the kernel is not the answer. We've all been working hard to be able to build more generic kernels, so as to get generic distro support. And generic distros will build kms as modules, as there are simply to many different kms drivers to build them all in.

How many DRM drivers are there on ARM and what's the size impact of building them all into the kernel? I know from experience that it's not possible on x86 especially with efifb in the mix, but I wonder what the situation on ARM is. I only have TI, sunxi and exynos boards to test on and building in both TI drm drivers and the exynos one seems to work. Note that I'm not talking about the non-DRM abortions that maskerade as graphics drivers for ARM SoCs, only proper DRM ones.

regards,

Koen

Quoted 7 lines · Koen Kooi

How many DRM drivers are there on ARM and what's the size impact of building them all into the kernel? I know from experience that it's not possible on x86 especially with efifb in the mix, but I wonder what the situation on ARM is. I only have TI, sunxi and exynos boards to test on and building in both TI drm drivers and the exynos one seems to work. Note that I'm not talking about the non-DRM abortions that maskerade as graphics drivers for ARM SoCs, only proper DRM ones.

regards,

Koen

A quick check tells me that my current sunxi-3.4 sunxi_drm.ko measures 1.7MB

Op 14 aug. 2014, om 11:37 heeft Hans de Goede <[email protected]> het volgende geschreven:

Quoted 70 lines

Hi,

Quoted 48 lines · Maxime Ripard

Quoted 9 lines · Stephen Warren

Quoted 4 lines · Luc Verhaegen

This claims and enables clocks listed in the simple framebuffer dt node. This is needed so that the display engine, in case the required clocks are known by the kernel code and are described in the dt, will remain properly enabled.

I think this make simplefb not simple any more, which rather goes against the whole point of it.

Quoted 7 lines

I specifically conceived simplefb to know about nothing more than the memory address and pixel layout of the memory buffer. I certainly don't like the idea of expanding simplefb to anything beyond that, and IIRC *not* extending is was a condition agreed when it was first merged. If more knowledge than that is required, then there needs to be a HW-specific driver to manage any clocks/resets/video registers, etc.

I'm sorry, but how is that not simple? clocks enabling is step 1 in a driver in order to communicate somehow with the controller. Reset is a different story, because arguably, if simplefb is there, the controller is already out of reset.

And I don't see why video registers are coming into the discussion here. The code Luc posted doesn't access any register, at all. It just makes sure the needed controller keep going.

Quoted 2 lines

The correct way to handle this without a complete DRM/KMS/... driver is to avoid the clocks in question being turned off at boot.

Which is exactly what this code does, using the generic DT bindings to express dependency for a given clock. How is this wrong?

Quoted 2 lines

I thought there was a per-clock flag to prevent disabling an unused clock?

No, last time I heard, Mike Turquette was against it.

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Quoted 7 lines

Quoted 4 lines

For example, the Tegra clock driver has a clock initialization table which IIRC was used for this purpose before we got a DRM/KMS driver. That way, all the details are kept inside the kernel code, and don't end up influencing the DT representation of simplefb.

I don't really see how the optional usage of a generic property influences badly the DT representation of simplefb.

+1 to all that Maxime said.

Also as can be seen in other discussion on this patch set, simplefb should not be seen as something orthogonal to having a full kms driver.

So just write a full kms driver is not the answer IMHO. What we want is for a bootloader setup console to be available through simplefb bindings so that the kernel can show output without depending on module loading, and thus can show errors if things go bad before a kms driver gets loaded.

Quoted 5 lines

And no build kms into the kernel is not the answer. We've all been working hard to be able to build more generic kernels, so as to get generic distro support. And generic distros will build kms as modules, as there are simply to many different kms drivers to build them all in.

How many DRM drivers are there on ARM and what's the size impact of building them all into the kernel?

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Quoted 13 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 9 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Quoted 3 lines

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

If so, we really have an issue in the kernel.

Quoted 18 lines · Maxime Ripard

Quoted 16 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 9 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Quoted 1 line

Are you saying that this driver would become "dirty" with such a patch?

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

As for the suggestion to simply never disable the plls / ahb gates by blocking them from ever being disabled in the sunxi clock driver, that is not really a solution either, as we want to be able to turn these things off to safe power on screen blank once control has been turned over to the kms driver.

And while at it let me also tackle the don't use simplefb only use kms argument, that means that the clocks will be turned off until the kms module loads, which will cause noticable screen flicker / video output resync, something which we've been trying to get rid of for years now.

And no, build in the kms driver is not an answer either. That works nicely for firmware, but not for generic Linux distributions supporting a wide range of boards.

Quoted 22 lines · Hans de Goede

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 8 lines

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

It is platform specific in that your platform may require certain clocks to remain on. The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

Quoted 5 lines

As for the suggestion to simply never disable the plls / ahb gates by blocking them from ever being disabled in the sunxi clock driver, that is not really a solution either, as we want to be able to turn these things off to safe power on screen blank once control has been turned over to the kms driver.

Then perhaps part of the hand-off procedure between simplefb and DRM/KMS should involve marking PLLs or "gates" as properly managed.

Quoted 8 lines

And while at it let me also tackle the don't use simplefb only use kms argument, that means that the clocks will be turned off until the kms module loads, which will cause noticable screen flicker / video output resync, something which we've been trying to get rid of for years now.

And no, build in the kms driver is not an answer either. That works nicely for firmware, but not for generic Linux distributions supporting a wide range of boards.

Odd... I didn't offer any of those two as solutions to the problem.

Thierry

Quoted 32 lines · Thierry Reding

Quoted 22 lines · Hans de Goede

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 8 lines

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

Quoted 5 lines

It is platform specific in that your platform may require certain clocks to remain on. The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

Why can't simplefb be a driver library that is called from a device specific device driver that only claims the clocks (or regulators)? Then build all of these device specific drivers into the generic ARM kernel. They will be quite small since all they do is claim the clocks (or regulator). Maybe we can even figure out some protocol for removing the unused ones from memory later.

Later during the boot process the device specific driver can load its KMS code which has also been implemented as a driver library. Maybe use E_PROBE_DEFER to do this. Match on the device ID, claim the clocks, defer until the full KMS library can be loaded.

Hi,

Quoted 47 lines · jonsmirl at gmail.com

Quoted 32 lines · Thierry Reding

Quoted 22 lines · Hans de Goede

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 8 lines

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

Quoted 5 lines

It is platform specific in that your platform may require certain clocks to remain on. The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

Why can't simplefb be a driver library that is called from a device specific device driver that only claims the clocks (or regulators)? Then build all of these device specific drivers into the generic ARM kernel. They will be quite small since all they do is claim the clocks (or regulator). Maybe we can even figure out some protocol for removing the unused ones from memory later.

Quoted 4 lines

Later during the boot process the device specific driver can load its KMS code which has also been implemented as a driver library. Maybe use E_PROBE_DEFER to do this. Match on the device ID, claim the clocks, defer until the full KMS library can be loaded.

There is no need for all this complexity, all that is needed is for the simplefb driver to be thought to claim + enable any clocks listed in its dt node.

Out of curiosity, how does this work in practice? How does the bootloader create this entry? Does it scan the DT to see which clocks the real hardware device references and then simply copies them to the simplefb node?

Thierry

Quoted 7 lines · Thierry Reding

Out of curiosity, how does this work in practice? How does the bootloader create this entry? Does it scan the DT to see which clocks the real hardware device references and then simply copies them to the simplefb node?

Thierry

https://www.mail-archive.com/linux-sunxi at googlegroups.com/msg06619.html

Quoted 9 lines · Luc Verhaegen

Quoted 7 lines · Thierry Reding

Out of curiosity, how does this work in practice? How does the bootloader create this entry? Does it scan the DT to see which clocks the real hardware device references and then simply copies them to the simplefb node?

Thierry

Quoted 1 line

https://www.mail-archive.com/linux-sunxi at googlegroups.com/msg06619.html

That looks like a royal pain. Again, I think it'd be much simpler (but not less code, unfortunately) to do this on a per-resource basis. That way these low-level firmware drivers in the kernel can stay trivial, keeping the real complexity where they belong: in hardware-specific drivers such as DRM/KMS.

Quoted 12 lines · Thierry Reding

Quoted 10 lines · Luc Verhaegen

Quoted 7 lines · Thierry Reding

Out of curiosity, how does this work in practice? How does the bootloader create this entry? Does it scan the DT to see which clocks the real hardware device references and then simply copies them to the simplefb node?

Thierry

https://www.mail-archive.com/linux-sunxi at googlegroups.com/msg06619.html

Quoted 5 lines

That looks like a royal pain. Again, I think it'd be much simpler (but not less code, unfortunately) to do this on a per-resource basis. That way these low-level firmware drivers in the kernel can stay trivial, keeping the real complexity where they belong: in hardware-specific drivers such as DRM/KMS.

So we have to write a DRM/KMS driver in order to have display working while waiting for a DRM/KMS driver to be worked on? Come on...

Quoted 55 lines · Hans de Goede

Hi,

Quoted 51 lines · jonsmirl at gmail.com

Quoted 32 lines · Thierry Reding

Quoted 22 lines · Hans de Goede

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 8 lines

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

Quoted 5 lines

It is platform specific in that your platform may require certain clocks to remain on. The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

Why can't simplefb be a driver library that is called from a device specific device driver that only claims the clocks (or regulators)? Then build all of these device specific drivers into the generic ARM kernel. They will be quite small since all they do is claim the clocks (or regulator). Maybe we can even figure out some protocol for removing the unused ones from memory later.

Later during the boot process the device specific driver can load its KMS code which has also been implemented as a driver library. Maybe use E_PROBE_DEFER to do this. Match on the device ID, claim the clocks, defer until the full KMS library can be loaded.

Quoted 3 lines

There is no need for all this complexity, all that is needed is for the simplefb driver to be thought to claim + enable any clocks listed in its dt node.

Out of curiosity, how does this work in practice? How does the bootloader create this entry? Does it scan the DT to see which clocks the real hardware device references and then simply copies them to the simplefb node?

Thierry

Quoted 60 lines · Thierry Reding

Quoted 58 lines · Hans de Goede

Hi,

Quoted 47 lines · jonsmirl at gmail.com

Quoted 32 lines · Thierry Reding

Quoted 22 lines · Hans de Goede

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 8 lines

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

Quoted 5 lines

It is platform specific in that your platform may require certain clocks to remain on. The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

Why can't simplefb be a driver library that is called from a device specific device driver that only claims the clocks (or regulators)? Then build all of these device specific drivers into the generic ARM kernel. They will be quite small since all they do is claim the clocks (or regulator). Maybe we can even figure out some protocol for removing the unused ones from memory later.

Quoted 4 lines

Later during the boot process the device specific driver can load its KMS code which has also been implemented as a driver library. Maybe use E_PROBE_DEFER to do this. Match on the device ID, claim the clocks, defer until the full KMS library can be loaded.

There is no need for all this complexity, all that is needed is for the simplefb driver to be thought to claim + enable any clocks listed in its dt node.

Quoted 4 lines

Out of curiosity, how does this work in practice? How does the bootloader create this entry? Does it scan the DT to see which clocks the real hardware device references and then simply copies them to the simplefb node?

That's why this is such a problem. There is a general rule in Linux - one device, one driver.

Quoted 32 lines · Thierry Reding

Quoted 22 lines · Hans de Goede

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 8 lines

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

Quoted 5 lines

It is platform specific in that your platform may require certain clocks to remain on. The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

Why can't simplefb be a driver library that is called from a device specific device driver that only claims the clocks (or regulators)? Then build all of these device specific drivers into the generic ARM kernel. They will be quite small since all they do is claim the clocks (or regulator). Maybe we can even figure out some protocol for removing the unused ones from memory later.

Later during the boot process the device specific driver can load its KMS code which has also been implemented as a driver library. Maybe use E_PROBE_DEFER to do this. Match on the device ID, claim the clocks, defer until the full KMS library can be loaded.

Quoted 22 lines · Hans de Goede

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 8 lines

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

It is platform specific in that your platform may require certain clocks to remain on. The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

Quoted 4 lines

As for the suggestion to simply never disable the plls / ahb gates by blocking them from ever being disabled in the sunxi clock driver, that is not really a solution either, as we want to be able to turn these things off to safe power on screen blank once control has been turned over to the kms driver.

Then perhaps part of the hand-off procedure between simplefb and DRM/KMS should involve marking PLLs or "gates" as properly managed.

Hi,

Quoted 43 lines · Thierry Reding

Quoted 22 lines · Hans de Goede

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 8 lines

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

It is platform specific in that your platform may require certain clocks to remain on. The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

Quoted 4 lines

As for the suggestion to simply never disable the plls / ahb gates by blocking them from ever being disabled in the sunxi clock driver, that is not really a solution either, as we want to be able to turn these things off to safe power on screen blank once control has been turned over to the kms driver.

Quoted 2 lines

Then perhaps part of the hand-off procedure between simplefb and DRM/KMS should involve marking PLLs or "gates" as properly managed.

And by your earlier argument also power domains, regulators, etc. So now we need to add code to each of the clock core, power-domain core, regulator core, etc. to have them now about this initially unmanaged state thing you're introducing, as well as modify all involved clock / regulator / etc. drivers to mark certain resources as unmanaged.

Hmm... that's true. But we already have a way to deal with exactly this situation for regulators. There's a property called regulator-boot-on which a bootloader should set whet it has enabled a given regulator. It can of course also be set statically in a DTS if it's know upfront that a bootloader will always enable it. Perhaps what we need is a similar property for clocks so that the clock framework will not inadvertently turn off a clock that's still being used.

Quoted 7 lines · Thierry Reding

Hmm... that's true. But we already have a way to deal with exactly this situation for regulators. There's a property called regulator-boot-on which a bootloader should set whet it has enabled a given regulator. It can of course also be set statically in a DTS if it's know upfront that a bootloader will always enable it. Perhaps what we need is a similar property for clocks so that the clock framework will not inadvertently turn off a clock that's still being used.

Except that such a property won't work either.

Regulators with regulator-boot-on will still be disabled if there's no one to claim it. Just like what happens currently for the clocks.

Quoted 2 lines · Maxime Ripard

Regulators with regulator-boot-on will still be disabled if there's no one to claim it. Just like what happens currently for the clocks.

I was somewhat surprised by this, but it can indeed easily be verified. It seems to me somewhat at odds with the definition of such a property.

that. However the implementation will automatically disable a regulator marked boot-on if it hasn't been claimed by any driver after the initcall stage completes.

I find that rather odd since I always assumed that a regulator marked boot-on would not be touched by the core at all, assuming that firmware set it up properly and that it would be required (even if not explicitly claimed).

Two categories of drivers have this issue: drivers built as modules (or that defer probing) and therefore won't be probed until after initcalls

have run and generic low-level drivers that take over firmware devices (simplefb in this case) that don't know anything about the resource that the devices need.

Quoted 7 lines · Thierry Reding

Quoted 2 lines · Maxime Ripard

Regulators with regulator-boot-on will still be disabled if there's no one to claim it. Just like what happens currently for the clocks.

I was somewhat surprised by this, but it can indeed easily be verified. It seems to me somewhat at odds with the definition of such a property.

That depends what you think it should do - it's there for handover from the bootloader in cases where we can't read the state.

Quoted 8 lines

that. However the implementation will automatically disable a regulator marked boot-on if it hasn't been claimed by any driver after the initcall stage completes.

I find that rather odd since I always assumed that a regulator marked boot-on would not be touched by the core at all, assuming that firmware set it up properly and that it would be required (even if not explicitly claimed).

No, there's a separate always-on property if we don't want to disable.

We can't assume that the "proper" setup is that the supply should be left on.

Quoted 2 lines

Two categories of drivers have this issue: drivers built as modules (or that defer probing) and therefore won't be probed until after initcalls

We really need a later initcall to manage handover to userspace (probably triggered by a combination of userspace saying it's done doing initial module enumeration and the deferred probe grinding to a halt).

Quoted 3 lines

have run and generic low-level drivers that take over firmware devices (simplefb in this case) that don't know anything about the resource that the devices need.

I don't understand this use case, sorry - it sounds like a buggy system design and/or integration. If the firmware is managing the resource then Linux really shouldn't be talking to it at all without coordinating with firmware. How can such a system work otherwise?

Quoted 4 lines · Mark Brown

Quoted 4 lines

I find that rather odd since I always assumed that a regulator marked boot-on would not be touched by the core at all, assuming that firmware set it up properly and that it would be required (even if not explicitly claimed).

Quoted 1 line

No, there's a separate always-on property if we don't want to disable.

But always-on means that it can't ever be disabled. In this case what we'd need is a "don't disable automatically because it's needed, but we may want to switch it off at a later point to save power."

Quoted 2 lines

We can't assume that the "proper" setup is that the supply should be left on.

Right, we can't assume it, but if we're given appropriate hints I think it's fair to keep resources set up by firmware untouched.

Quoted 8 lines

Quoted 3 lines

have run and generic low-level drivers that take over firmware devices (simplefb in this case) that don't know anything about the resource that the devices need.

I don't understand this use case, sorry - it sounds like a buggy system design and/or integration. If the firmware is managing the resource then Linux really shouldn't be talking to it at all without coordinating with firmware. How can such a system work otherwise?

The firmware isn't really managing resources. It's setting up state that the kernel shouldn't be modifying. Currently the kernel assumes that no firmware exists and just disables everything that's not being used. That is a reasonable default, but it also limits what we can do. I think if we provided a good interface to communicate state between firmware and kernel then we could easily do this kind of hand-off.

Quoted 7 lines · Thierry Reding

Quoted 4 lines · Mark Brown

Quoted 4 lines

I find that rather odd since I always assumed that a regulator marked boot-on would not be touched by the core at all, assuming that firmware set it up properly and that it would be required (even if not explicitly claimed).

Quoted 5 lines

Quoted 1 line

No, there's a separate always-on property if we don't want to disable.

But always-on means that it can't ever be disabled. In this case what we'd need is a "don't disable automatically because it's needed, but we may want to switch it off at a later point to save power."

Such a property wouldn't make much sense, it should also apply to the driver at which point it's just the same as always-on.

Quoted 5 lines

Quoted 2 lines

We can't assume that the "proper" setup is that the supply should be left on.

Right, we can't assume it, but if we're given appropriate hints I think it's fair to keep resources set up by firmware untouched.

I really can't see any sensible way to do that in an OS neutral thing like DT.

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

have run and generic low-level drivers that take over firmware devices (simplefb in this case) that don't know anything about the resource that the devices need.

I don't understand this use case, sorry - it sounds like a buggy system design and/or integration. If the firmware is managing the resource then Linux really shouldn't be talking to it at all without coordinating with firmware. How can such a system work otherwise?

Quoted 6 lines

The firmware isn't really managing resources. It's setting up state that the kernel shouldn't be modifying. Currently the kernel assumes that no firmware exists and just disables everything that's not being used. That is a reasonable default, but it also limits what we can do. I think if we provided a good interface to communicate state between firmware and kernel then we could easily do this kind of hand-off.

I'm afraid that's confusing me even further. If the "firmware devices" don't know anything about the resources as you say then presumably they need to be always on since they obviously won't be able to control them and there is going to be no handoff?

Quoted 11 lines · Mark Brown

Quoted 7 lines · Thierry Reding

Quoted 2 lines · Mark Brown

We can't assume that the "proper" setup is that the supply should be left on.

Right, we can't assume it, but if we're given appropriate hints I think it's fair to keep resources set up by firmware untouched.

I really can't see any sensible way to do that in an OS neutral thing like DT.

It should be possible to simply define a property for this that the firmware can add to the device tree.

There are other subsystems for example where we don't touch resources unless they are explicitly accessed. GPIO and pinmux are two examples of those. The reason is precisely so that we don't undo anything that the firmware already set up and that may be needed during boot.

Before booting the kernel it will modify the DTB and insert a node that contains information about the framebuffer that was set up. The node contains properties that define the resolution, the format and the physical address of the framebuffer memory. An in-kernel driver will then be able to use that information very early in the boot process to show the console (to replace the serial console that's usually not available on consumer devices). Later on when a real driver can be loaded it should take over the resources from simplefb and remove it.

So the firmware does control the resources to a point where the display hardware is initialized and then it will happily forget about them. The device will keep scanning out the framebuffer and simplefb can update it with boot messages. To prevent the console from suddenly disappearing the kernel needs to make sure that the resources aren't inadvertently turned off at some more or less random point in time.

It is not clear to me where the hardware resources should be listed in DT, being it a simplefb node or part of the actual hardware device node properly marked as dynamic boot defaults or something else? It's somewhere inbetween hardware and virtual device, and somewhat volatile. As far as simplefb is concerned it is a hardware desription of the framebuffer, but for a kms driver it's no more than firmware handover of boottime settings and ceases to exists once the kms driver have reconfigured the hardware.

Quoted 8 lines · Henrik Nordstr?m

It is not clear to me where the hardware resources should be listed in DT, being it a simplefb node or part of the actual hardware device node properly marked as dynamic boot defaults or something else? It's somewhere inbetween hardware and virtual device, and somewhat volatile. As far as simplefb is concerned it is a hardware desription of the framebuffer, but for a kms driver it's no more than firmware handover of boottime settings and ceases to exists once the kms driver have reconfigured the hardware.

Is simplefb something that should be in the device tree distinctly in the first place - shouldn't it be a subset of the functionality of the video nodes? It's the same hardware being driven differently.

Quoted 11 lines · Mark Brown

Quoted 8 lines · Henrik Nordstr?m

It is not clear to me where the hardware resources should be listed in DT, being it a simplefb node or part of the actual hardware device node properly marked as dynamic boot defaults or something else? It's somewhere inbetween hardware and virtual device, and somewhat volatile. As far as simplefb is concerned it is a hardware desription of the framebuffer, but for a kms driver it's no more than firmware handover of boottime settings and ceases to exists once the kms driver have reconfigured the hardware.

Quoted 3 lines

Is simplefb something that should be in the device tree distinctly in the first place - shouldn't it be a subset of the functionality of the video nodes? It's the same hardware being driven differently.

Therorically, yes, but that would mean knowing beforehand what the final binding will look like, even before submitting the driver. Since the bindings are always reviewed, and most of the time changed slightly, that wouldn't work very well with the DT as a stable ABI policy I guess.

Hi Maxime,

Quoted 16 lines · Maxime Ripard

Quoted 11 lines · Mark Brown

Quoted 8 lines · Henrik Nordstr?m

It is not clear to me where the hardware resources should be listed in DT, being it a simplefb node or part of the actual hardware device node properly marked as dynamic boot defaults or something else? It's somewhere inbetween hardware and virtual device, and somewhat volatile. As far as simplefb is concerned it is a hardware desription of the framebuffer, but for a kms driver it's no more than firmware handover of boottime settings and ceases to exists once the kms driver have reconfigured the hardware.

Quoted 3 lines

Is simplefb something that should be in the device tree distinctly in the first place - shouldn't it be a subset of the functionality of the video nodes? It's the same hardware being driven differently.

Quoted 5 lines

Therorically, yes, but that would mean knowing beforehand what the final binding will look like, even before submitting the driver. Since the bindings are always reviewed, and most of the time changed slightly, that wouldn't work very well with the DT as a stable ABI policy I guess.

If you don't know how the bindings for a device will look like at the time of writing your DTS, you're always screwed, whether you add a simpefb node or not.

If you know how the bindings look like, just add the device, with an extra "linux,simplefb" compatibility value.
If you don't know how the bindings look like, do your utter best in guessing. Your DTS must be amended later anyway, either because you guessed wrong[*] (in case you added a node to have simplefb working), or because you have to add a real device node (in case you didn't add one for simplefb).

Quoted 14 lines

Quoted 9 lines

Quoted 3 lines

Is simplefb something that should be in the device tree distinctly in the first place - shouldn't it be a subset of the functionality of the video nodes? It's the same hardware being driven differently.

Therorically, yes, but that would mean knowing beforehand what the final binding will look like, even before submitting the driver. Since the bindings are always reviewed, and most of the time changed slightly, that wouldn't work very well with the DT as a stable ABI policy I guess.

If you don't know how the bindings for a device will look like at the time of writing your DTS, you're always screwed, whether you add a simpefb node or not.

Quoted 7 lines

If you know how the bindings look like, just add the device, with an extra "linux,simplefb" compatibility value.
If you don't know how the bindings look like, do your utter best in guessing. Your DTS must be amended later anyway, either because you guessed wrong[*] (in case you added a node to have simplefb working), or because you have to add a real device node (in case you didn't add one for simplefb).

Let's be conservative and consider the case where we would guess wrong.

If we just rely on a simplefb node, when reviewing and integrating the "new" bindings to describe accureately the various IPs involved in the display path, we would obviously create new compatibles for them. Since it's new compatibles, we can come up with any binding we'd like, without have to consider the backward compatibility, since it's a new binding.

Then, we just remove the simplefb, all is good.

Quoted 32 lines · Maxime Ripard

Quoted 14 lines

Quoted 9 lines

Quoted 3 lines

Is simplefb something that should be in the device tree distinctly in the first place - shouldn't it be a subset of the functionality of the video nodes? It's the same hardware being driven differently.

Therorically, yes, but that would mean knowing beforehand what the final binding will look like, even before submitting the driver. Since the bindings are always reviewed, and most of the time changed slightly, that wouldn't work very well with the DT as a stable ABI policy I guess.

If you don't know how the bindings for a device will look like at the time of writing your DTS, you're always screwed, whether you add a simpefb node or not.

Quoted 7 lines

If you know how the bindings look like, just add the device, with an extra "linux,simplefb" compatibility value.
If you don't know how the bindings look like, do your utter best in guessing. Your DTS must be amended later anyway, either because you guessed wrong[*] (in case you added a node to have simplefb working), or because you have to add a real device node (in case you didn't add one for simplefb).

Let's be conservative and consider the case where we would guess wrong.

If we just rely on a simplefb node, when reviewing and integrating the "new" bindings to describe accureately the various IPs involved in the display path, we would obviously create new compatibles for them. Since it's new compatibles, we can come up with any binding we'd like, without have to consider the backward compatibility, since it's a new binding.

Quoted 1 line

Then, we just remove the simplefb, all is good.

I would keep the simplefb compatible value. Else you break compatibility with old kernels that don't have your new driver.

Hi!

Quoted 30 lines · Geert Uytterhoeven

Hi Maxime,

Quoted 16 lines · Maxime Ripard

Quoted 11 lines · Mark Brown

Quoted 8 lines · Henrik Nordstr?m

It is not clear to me where the hardware resources should be listed in DT, being it a simplefb node or part of the actual hardware device node properly marked as dynamic boot defaults or something else? It's somewhere inbetween hardware and virtual device, and somewhat volatile. As far as simplefb is concerned it is a hardware desription of the framebuffer, but for a kms driver it's no more than firmware handover of boottime settings and ceases to exists once the kms driver have reconfigured the hardware.

Quoted 3 lines

Is simplefb something that should be in the device tree distinctly in the first place - shouldn't it be a subset of the functionality of the video nodes? It's the same hardware being driven differently.

Quoted 5 lines

Therorically, yes, but that would mean knowing beforehand what the final binding will look like, even before submitting the driver. Since the bindings are always reviewed, and most of the time changed slightly, that wouldn't work very well with the DT as a stable ABI policy I guess.

If you don't know how the bindings for a device will look like at the time of writing your DTS, you're always screwed, whether you add a simpefb node or not.

Quoted 7 lines

If you know how the bindings look like, just add the device, with an extra "linux,simplefb" compatibility value.
If you don't know how the bindings look like, do your utter best in guessing. Your DTS must be amended later anyway, either because you guessed wrong[*] (in case you added a node to have simplefb working), or because you have to add a real device node (in case you didn't add one for simplefb).

Let's be conservative and consider the case where we would guess wrong.

If we just rely on a simplefb node, when reviewing and integrating the "new" bindings to describe accureately the various IPs involved in the display path, we would obviously create new compatibles for them. Since it's new compatibles, we can come up with any binding we'd like, without have to consider the backward compatibility, since it's a new binding.

Then, we just remove the simplefb, all is good.

If we were to try to create our bindings for all the IPs involved, and were not pleased with the binding anymore when merging the driver, then we would have to break the bindings, since we don't introduce a new compatible anymore, but modifying an existing one.

If we just rely on a simplefb node, when reviewing and integrating the "new" bindings to describe accureately the various IPs involved in the display path, we would obviously create new compatibles for them. Since it's new compatibles, we can come up with any binding we'd like, without have to consider the backward compatibility, since it's a new binding.

Hi Maxime,

Quoted 16 lines · Maxime Ripard

Quoted 11 lines · Mark Brown

Quoted 8 lines · Henrik Nordstr?m

It is not clear to me where the hardware resources should be listed in DT, being it a simplefb node or part of the actual hardware device node properly marked as dynamic boot defaults or something else? It's somewhere inbetween hardware and virtual device, and somewhat volatile. As far as simplefb is concerned it is a hardware desription of the framebuffer, but for a kms driver it's no more than firmware handover of boottime settings and ceases to exists once the kms driver have reconfigured the hardware.

Quoted 3 lines

Is simplefb something that should be in the device tree distinctly in the first place - shouldn't it be a subset of the functionality of the video nodes? It's the same hardware being driven differently.

Quoted 5 lines

Therorically, yes, but that would mean knowing beforehand what the final binding will look like, even before submitting the driver. Since the bindings are always reviewed, and most of the time changed slightly, that wouldn't work very well with the DT as a stable ABI policy I guess.

If you don't know how the bindings for a device will look like at the time of writing your DTS, you're always screwed, whether you add a simpefb node or not.

If you know how the bindings look like, just add the device, with an extra "linux,simplefb" compatibility value.
If you don't know how the bindings look like, do your utter best in guessing. Your DTS must be amended later anyway, either because you guessed wrong[*] (in case you added a node to have simplefb working), or because you have to add a real device node (in case you didn't add one for simplefb).

[*] Actually you may have guessed right, in which case it'll continue
working as-is, and everybody will be happy.
Whether you want to keep backwards-compatibility in your future driver with the "guessed wrong" node is up to you.

Hi All,

Quoted 38 lines · Geert Uytterhoeven

Hi Maxime,

Quoted 16 lines · Maxime Ripard

Quoted 11 lines · Mark Brown

Quoted 8 lines · Henrik Nordstr?m

It is not clear to me where the hardware resources should be listed in DT, being it a simplefb node or part of the actual hardware device node properly marked as dynamic boot defaults or something else? It's somewhere inbetween hardware and virtual device, and somewhat volatile. As far as simplefb is concerned it is a hardware desription of the framebuffer, but for a kms driver it's no more than firmware handover of boottime settings and ceases to exists once the kms driver have reconfigured the hardware.

Quoted 3 lines

Is simplefb something that should be in the device tree distinctly in the first place - shouldn't it be a subset of the functionality of the video nodes? It's the same hardware being driven differently.

Quoted 5 lines

Therorically, yes, but that would mean knowing beforehand what the final binding will look like, even before submitting the driver. Since the bindings are always reviewed, and most of the time changed slightly, that wouldn't work very well with the DT as a stable ABI policy I guess.

If you don't know how the bindings for a device will look like at the time of writing your DTS, you're always screwed, whether you add a simpefb node or not.

If you know how the bindings look like, just add the device, with an extra "linux,simplefb" compatibility value.
If you don't know how the bindings look like, do your utter best in guessing. Your DTS must be amended later anyway, either because you guessed wrong[*] (in case you added a node to have simplefb working), or because you have to add a real device node (in case you didn't add one for simplefb).

Quoted 4 lines

[*] Actually you may have guessed right, in which case it'll continue
working as-is, and everybody will be happy.
Whether you want to keep backwards-compatibility in your future driver with the "guessed wrong" node is up to you.

I apologise if I'm stepping on anyone's toes or horribly misrepresenting device tree's capabilities, but couldn't we start out with something like:

disp at whatever {
    compatible = "sunxi,sun4i-disp";
    clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>;

Quoted 46 lines · Hans de Goede

Hi,

Quoted 43 lines · Thierry Reding

Quoted 22 lines · Hans de Goede

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 8 lines

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

It is platform specific in that your platform may require certain clocks to remain on. The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

Quoted 4 lines

As for the suggestion to simply never disable the plls / ahb gates by blocking them from ever being disabled in the sunxi clock driver, that is not really a solution either, as we want to be able to turn these things off to safe power on screen blank once control has been turned over to the kms driver.

Quoted 8 lines

Quoted 2 lines

Then perhaps part of the hand-off procedure between simplefb and DRM/KMS should involve marking PLLs or "gates" as properly managed.

And by your earlier argument also power domains, regulators, etc. So now we need to add code to each of the clock core, power-domain core, regulator core, etc. to have them now about this initially unmanaged state thing you're introducing, as well as modify all involved clock / regulator / etc. drivers to mark certain resources as unmanaged.

Hmm... that's true. But we already have a way to deal with exactly this situation for regulators. There's a property called regulator-boot-on which a bootloader should set whet it has enabled a given regulator. It can of course also be set statically in a DTS if it's know upfront that a bootloader will always enable it. Perhaps what we need is a similar property for clocks so that the clock framework will not inadvertently turn off a clock that's still being used.

Thierry

Quoted 26 lines · Hans de Goede

Quoted 24 lines · Thierry Reding

Quoted 14 lines · Maxime Ripard

Quoted 13 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 9 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Quoted 5 lines

Quoted 3 lines

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

Quoted 4 lines

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

It is platform specific in that your platform may require certain clocks to remain on.

The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

Quoted 32 lines · Thierry Reding

Quoted 30 lines · Hans de Goede

Quoted 19 lines · Thierry Reding

Quoted 18 lines · Maxime Ripard

Quoted 16 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 9 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Quoted 5 lines

Quoted 1 line

Are you saying that this driver would become "dirty" with such a patch?

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

Quoted 2 lines

It is platform specific in that your platform may require certain clocks to remain on.

The platform doesn't. simplefb does. simplefb is the obvious consumer for these clocks, and given the current API and abstraction we have, it should be the one claiming the clocks too.

Quoted 4 lines

The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

And I wouldn't find anything wrong with that. We're already doing so for any generic driver in the kernel (AHCI, EHCI comes to my mind first, there's probably a lot of others). Why wouldn't we do as such for this one?

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on.

Quoted 3 lines · Thierry Reding

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on.

Simplefb also requires that the memory is there and is persistent. Fine for discrete graphics cards, fine for rpi where most things are hidden from the ARM core anyway, not so fine for anybody else.

Quoted 8 lines · Luc Verhaegen

Quoted 3 lines · Thierry Reding

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on.

Simplefb also requires that the memory is there and is persistent. Fine for discrete graphics cards, fine for rpi where most things are hidden from the ARM core anyway, not so fine for anybody else.

I don't understand. This patch series isn't changing anything about the memory aspects of the driver yet it's working for you on sunxi, isn't it? So it can't be all that broken.

Thierry

Quoted 14 lines · Thierry Reding

Quoted 8 lines · Luc Verhaegen

Quoted 3 lines · Thierry Reding

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on.

Simplefb also requires that the memory is there and is persistent. Fine for discrete graphics cards, fine for rpi where most things are hidden from the ARM core anyway, not so fine for anybody else.

I don't understand. This patch series isn't changing anything about the memory aspects of the driver yet it's working for you on sunxi, isn't it? So it can't be all that broken.

Quoted 1 line

Thierry

Oh, i had to go wrestle with UBoot options and reserve 8MB off the top, which the kernel never gets told about. A nice throwback to x86 IGP bioses, a past i had thought i had left behind forgood.

Quoted 15 lines · Luc Verhaegen

Quoted 14 lines · Thierry Reding

Quoted 8 lines · Luc Verhaegen

Quoted 3 lines · Thierry Reding

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on.

Simplefb also requires that the memory is there and is persistent. Fine for discrete graphics cards, fine for rpi where most things are hidden from the ARM core anyway, not so fine for anybody else.

I don't understand. This patch series isn't changing anything about the memory aspects of the driver yet it's working for you on sunxi, isn't it? So it can't be all that broken.

Quoted 5 lines

Quoted 1 line

Thierry

Oh, i had to go wrestle with UBoot options and reserve 8MB off the top, which the kernel never gets told about. A nice throwback to x86 IGP bioses, a past i had thought i had left behind forgood.

Can you not use the reserved memory code (drivers/of/of_reserved_mem.c in the kernel)? I think that's the generally accepted way to do this with DT.

Thierry

Quoted 22 lines · Thierry Reding

Quoted 15 lines · Luc Verhaegen

Quoted 14 lines · Thierry Reding

Quoted 8 lines · Luc Verhaegen

Quoted 3 lines · Thierry Reding

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on.

Simplefb also requires that the memory is there and is persistent. Fine for discrete graphics cards, fine for rpi where most things are hidden from the ARM core anyway, not so fine for anybody else.

I don't understand. This patch series isn't changing anything about the memory aspects of the driver yet it's working for you on sunxi, isn't it? So it can't be all that broken.

Quoted 5 lines

Quoted 1 line

Thierry

Oh, i had to go wrestle with UBoot options and reserve 8MB off the top, which the kernel never gets told about. A nice throwback to x86 IGP bioses, a past i had thought i had left behind forgood.

Quoted 5 lines

Can you not use the reserved memory code (drivers/of/of_reserved_mem.c in the kernel)? I think that's the generally accepted way to do this with DT.

Thierry

It was mentioned to me, and I probably could do that, but this seemed even more DT wrangling from within u-boot, and it didn't seem finished yet. Plus, all of this already was way more wrangling than i bargained for, especially with the promise of "simple"-fb. Then there is the really awkward way in which one gets to define clocks outside of a dts/compiler (you've seen the code, right?), i really did not feel like repeating something like that.

Quoted 36 lines · Maxime Ripard

Quoted 34 lines · Thierry Reding

Quoted 22 lines · Hans de Goede

Quoted 21 lines · Thierry Reding

Quoted 19 lines · Maxime Ripard

Quoted 9 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 6 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

Quoted 7 lines

Quoted 3 lines

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Quoted 8 lines

Quoted 3 lines

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

It is platform specific in that your platform may require certain clocks to remain on.

Quoted 3 lines

The platform doesn't. simplefb does. simplefb is the obvious consumer for these clocks, and given the current API and abstraction we have, it should be the one claiming the clocks too.

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on. Other platforms may not even allow turning off the clocks.

Quoted 9 lines

Quoted 4 lines

The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has.

And I wouldn't find anything wrong with that. We're already doing so for any generic driver in the kernel (AHCI, EHCI comes to my mind first, there's probably a lot of others). Why wouldn't we do as such for this one?

Yes, and we've had similar discussions in those subsystems too.

Quoted 41 lines · Thierry Reding

Quoted 39 lines · Maxime Ripard

Quoted 27 lines · Thierry Reding

Quoted 26 lines · Hans de Goede

Quoted 24 lines · Thierry Reding

Quoted 14 lines · Maxime Ripard

Quoted 13 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 9 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Quoted 5 lines

Quoted 3 lines

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

Quoted 7 lines

Quoted 4 lines

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

It is platform specific in that your platform may require certain clocks to remain on.

The platform doesn't. simplefb does. simplefb is the obvious consumer for these clocks, and given the current API and abstraction we have, it should be the one claiming the clocks too.

Quoted 3 lines

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on. Other platforms may not even allow turning off the clocks.

Like what? the rpi? Come on. Just because the videocore is some black box we know nothing about doesn't mean we should use it as an example.

Any decent enough SoC, with a decent support in the kernel will have clocks for this, and I really wonder how simplefb will behave once its clocks will be turned off...

Quoted 46 lines · Maxime Ripard

Quoted 44 lines · Thierry Reding

Quoted 33 lines · Maxime Ripard

Quoted 32 lines · Thierry Reding

Quoted 30 lines · Hans de Goede

Quoted 19 lines · Thierry Reding

Quoted 18 lines · Maxime Ripard

Quoted 16 lines · Thierry Reding

Quoted 1 line · Maxime Ripard

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:

[...]

Quoted 9 lines

Quoted 2 lines

If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our clock driver.

I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"?

Quoted 5 lines

Quoted 1 line

Are you saying that this driver would become "dirty" with such a patch?

Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way.

This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support.

Quoted 6 lines

Quoted 2 lines

It is platform specific in that your platform may require certain clocks to remain on.

The platform doesn't. simplefb does. simplefb is the obvious consumer for these clocks, and given the current API and abstraction we have, it should be the one claiming the clocks too.

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on. Other platforms may not even allow turning off the clocks.

Quoted 2 lines

Like what? the rpi? Come on. Just because the videocore is some black box we know nothing about doesn't mean we should use it as an example.

You make it sound like the Raspberry Pi is somehow less important than sunxi.

Quoted 4 lines

Any decent enough SoC, with a decent support in the kernel will have clocks for this, and I really wonder how simplefb will behave once its clocks will be turned off...

There are other devices besides ARM SoCs that may want to use this driver and that don't have clock support.

But you're missing my point. What I'm saying is that the simplefb driver is meant to serve as a way to take over whatever framebuffer a firmware set up. Therefore I think it makes the most sense to assume that nothing needs to be controlled in any way since already been set up by firmware. Eventually there should be a driver that takes over from simplefb that knows how to properly handle the device's specifics, but that's not simplefb.

The goal of this patch series is to keep clocks from being turned off. But that's not what it does. What it does is turn clocks on to prevent them from being turned off. In my opinion that's a workaround for a deficiency in the kernel (and the firmware/kernel interface) and I think it should be fixed at the root. So a much better solution would be to establish a way for firmware to communicate to the kernel that a given resource has been enabled by firmware and shouldn't be disabled.

Quoted 6 lines

Quoted 3 lines

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on. Other platforms may not even allow turning off the clocks.

Like what? the rpi? Come on. Just because the videocore is some black box we know nothing about doesn't mean we should use it as an example.

You make it sound like the Raspberry Pi is somehow less important than sunxi.

Quoted 3 lines

Any decent enough SoC, with a decent support in the kernel will have clocks for this, and I really wonder how simplefb will behave once its clocks will be turned off...

There are other devices besides ARM SoCs that may want to use this driver and that don't have clock support.

But you're missing my point. What I'm saying is that the simplefb driver is meant to serve as a way to take over whatever framebuffer a firmware set up. Therefore I think it makes the most sense to assume that nothing needs to be controlled in any way since already been set up by firmware. Eventually there should be a driver that takes over from simplefb that knows how to properly handle the device's specifics, but that's not simplefb.

The goal of this patch series is to keep clocks from being turned off. But that's not what it does. What it does is turn clocks on to prevent them from being turned off. In my opinion that's a workaround for a deficiency in the kernel (and the firmware/kernel interface) and I think it should be fixed at the root. So a much better solution would be to establish a way for firmware to communicate to the kernel that a given resource has been enabled by firmware and shouldn't be disabled. Such a solution can be implement for all types of resources and can be reused by all drivers since they don't have to worry about these details.

Quoted 4 lines · Thierry Reding

Quoted 4 lines

Quoted 3 lines

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on. Other platforms may not even allow turning off the clocks.

Quoted 5 lines

Quoted 2 lines

Like what? the rpi? Come on. Just because the videocore is some black box we know nothing about doesn't mean we should use it as an example.

You make it sound like the Raspberry Pi is somehow less important than sunxi.

No. What I mean is that it seems like we are somehow punished, or at least blamed, for having a better and more complete kernel support.

Quoted 6 lines

Quoted 3 lines

Any decent enough SoC, with a decent support in the kernel will have clocks for this, and I really wonder how simplefb will behave once its clocks will be turned off...

There are other devices besides ARM SoCs that may want to use this driver and that don't have clock support.

And in this case, with this patch, simplefb will not claim any clock, nor will fail probing.

Quoted 7 lines

But you're missing my point. What I'm saying is that the simplefb driver is meant to serve as a way to take over whatever framebuffer a firmware set up. Therefore I think it makes the most sense to assume that nothing needs to be controlled in any way since already been set up by firmware. Eventually there should be a driver that takes over from simplefb that knows how to properly handle the device's specifics, but that's not simplefb.

I guess such a hand over if it were to happen in the kernel would involve the second driver claiming the resources before the first one release them. How is that different in this case?

Quoted 9 lines

The goal of this patch series is to keep clocks from being turned off. But that's not what it does. What it does is turn clocks on to prevent them from being turned off. In my opinion that's a workaround for a deficiency in the kernel (and the firmware/kernel interface) and I think it should be fixed at the root. So a much better solution would be to establish a way for firmware to communicate to the kernel that a given resource has been enabled by firmware and shouldn't be disabled. Such a solution can be implement for all types of resources and can be reused by all drivers since they don't have to worry about these details.

Mike Turquette repeatedly said that he was against such a DT property:
https://lkml.org/lkml/2014/5/12/693

Quoted 5 lines · Maxime Ripard

Quoted 4 lines · Thierry Reding

Quoted 4 lines

Quoted 3 lines

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on. Other platforms may not even allow turning off the clocks.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Like what? the rpi? Come on. Just because the videocore is some black box we know nothing about doesn't mean we should use it as an example.

You make it sound like the Raspberry Pi is somehow less important than sunxi.

No. What I mean is that it seems like we are somehow punished, or at least blamed, for having a better and more complete kernel support.

This isn't a competition. Nobody's punishing or blaming anyone. This is about finding the best solution for the problem at hand.

Quoted 18 lines

Quoted 6 lines

Quoted 3 lines

Any decent enough SoC, with a decent support in the kernel will have clocks for this, and I really wonder how simplefb will behave once its clocks will be turned off...

There are other devices besides ARM SoCs that may want to use this driver and that don't have clock support.

And in this case, with this patch, simplefb will not claim any clock, nor will fail probing.

Quoted 7 lines

But you're missing my point. What I'm saying is that the simplefb driver is meant to serve as a way to take over whatever framebuffer a firmware set up. Therefore I think it makes the most sense to assume that nothing needs to be controlled in any way since already been set up by firmware. Eventually there should be a driver that takes over from simplefb that knows how to properly handle the device's specifics, but that's not simplefb.

Quoted 3 lines

I guess such a hand over if it were to happen in the kernel would involve the second driver claiming the resources before the first one release them. How is that different in this case?

It's different in that that driver will be hardware specific and know exactly what clock and other resources are required. It will have a device-specific binding.

Quoted 10 lines

Quoted 9 lines

The goal of this patch series is to keep clocks from being turned off. But that's not what it does. What it does is turn clocks on to prevent them from being turned off. In my opinion that's a workaround for a deficiency in the kernel (and the firmware/kernel interface) and I think it should be fixed at the root. So a much better solution would be to establish a way for firmware to communicate to the kernel that a given resource has been enabled by firmware and shouldn't be disabled. Such a solution can be implement for all types of resources and can be reused by all drivers since they don't have to worry about these details.

Quoted 2 lines

Mike Turquette repeatedly said that he was against such a DT property:
https://lkml.org/lkml/2014/5/12/693

Mike says in that email that he's opposing the addition of a property for clocks that is the equivalent of regulator-always-on. That's not what this is about. If at all it'd be a property to mark a clock that should not be disabled by default because it's essential.

Adding Mike on this subthread too.

Either way, Mark already suggested a different alternative in another subthread, namely to add a new kind of checkpoint at which subsystems can call a "disable unused" function that's called later than a late initcall. This is not going to fix things for you immediately because the clocks will still be switched off (only later) if you don't have a real driver that's claiming the clocks. But you can work around that for now by making the relevant clocks always on and remove that workaround once a real driver is loaded that knows how to handle them properly.

Hi,

Quoted 64 lines · Thierry Reding

Quoted 5 lines · Maxime Ripard

Quoted 4 lines · Thierry Reding

Quoted 4 lines

Quoted 3 lines

No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on. Other platforms may not even allow turning off the clocks.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Like what? the rpi? Come on. Just because the videocore is some black box we know nothing about doesn't mean we should use it as an example.

You make it sound like the Raspberry Pi is somehow less important than sunxi.

No. What I mean is that it seems like we are somehow punished, or at least blamed, for having a better and more complete kernel support.

This isn't a competition. Nobody's punishing or blaming anyone. This is about finding the best solution for the problem at hand.

Quoted 18 lines

Quoted 6 lines

Quoted 3 lines

Any decent enough SoC, with a decent support in the kernel will have clocks for this, and I really wonder how simplefb will behave once its clocks will be turned off...

There are other devices besides ARM SoCs that may want to use this driver and that don't have clock support.

And in this case, with this patch, simplefb will not claim any clock, nor will fail probing.

Quoted 7 lines

But you're missing my point. What I'm saying is that the simplefb driver is meant to serve as a way to take over whatever framebuffer a firmware set up. Therefore I think it makes the most sense to assume that nothing needs to be controlled in any way since already been set up by firmware. Eventually there should be a driver that takes over from simplefb that knows how to properly handle the device's specifics, but that's not simplefb.

Quoted 3 lines

I guess such a hand over if it were to happen in the kernel would involve the second driver claiming the resources before the first one release them. How is that different in this case?

It's different in that that driver will be hardware specific and know exactly what clock and other resources are required. It will have a device-specific binding.

Quoted 10 lines

Quoted 9 lines

The goal of this patch series is to keep clocks from being turned off. But that's not what it does. What it does is turn clocks on to prevent them from being turned off. In my opinion that's a workaround for a deficiency in the kernel (and the firmware/kernel interface) and I think it should be fixed at the root. So a much better solution would be to establish a way for firmware to communicate to the kernel that a given resource has been enabled by firmware and shouldn't be disabled. Such a solution can be implement for all types of resources and can be reused by all drivers since they don't have to worry about these details.

Quoted 2 lines

Mike Turquette repeatedly said that he was against such a DT property:
https://lkml.org/lkml/2014/5/12/693

Mike says in that email that he's opposing the addition of a property for clocks that is the equivalent of regulator-always-on. That's not what this is about. If at all it'd be a property to mark a clock that should not be disabled by default because it's essential.

Adding Mike on this subthread too.

Quoted 9 lines

Either way, Mark already suggested a different alternative in another subthread, namely to add a new kind of checkpoint at which subsystems can call a "disable unused" function that's called later than a late initcall. This is not going to fix things for you immediately because the clocks will still be switched off (only later) if you don't have a real driver that's claiming the clocks. But you can work around that for now by making the relevant clocks always on and remove that workaround once a real driver is loaded that knows how to handle them properly.

This will simply not work, and is a ton more complicated then simply teaching simplefb about a clocks property, which is a really simple and clean patch.

First of all let me explain why this won't work. When should those subsystems call this "later then a late initcall" call ? the kms driver may very well be a kernel module, which will be loaded by userspace, so we would need this to be triggered by userspace, but when ?

When the initrd is done? What then if the kms driver is not in the initrd, so maybe when udev is done enumerating devices, but what then if the kernel is still enumerating hardware at this time?

Will we just put a sleep 30
in our initscripts to make sure the kernel is done scanning all busses, will that be long enough? Maybe we need to re-introduce the scsi_wait_done kernel module which was added as an ugly hack to allow userspace to wait for the kernel to have finished scanning scsi (and ata / sata) busses, for controllers found at the time the module was load. Which never worked reliable, because it would be possible that the controller itself still needed to be discovered. We've spend years cleaning up userspace enough to be able to kill scsi_wait_done. We've already been down this road of waiting for hw enumeration to be done, and the problem is that on modern systems *it is never done*.

So second of all, Thierry, what exactly is the technical argument against adding support for dealing with clocks to simplefb ?

I've heard a lot of people in favor of it,

and only pretty much you against it,

and your argument so far seems to boil down to "I don't like it",

is not really a technical sound argument IMHO. Moreover all the alternatives seem to be horribly complicated and most of them also seem to have several issues which will make them simply not work reliable.

Can you please fix you mail setup. You're addressing me directly in this email, yet I'm neither in To: nor Cc: headers. That's really annoying.

Quoted 19 lines

Quoted 9 lines

Either way, Mark already suggested a different alternative in another subthread, namely to add a new kind of checkpoint at which subsystems can call a "disable unused" function that's called later than a late initcall. This is not going to fix things for you immediately because the clocks will still be switched off (only later) if you don't have a real driver that's claiming the clocks. But you can work around that for now by making the relevant clocks always on and remove that workaround once a real driver is loaded that knows how to handle them properly.

This will simply not work, and is a ton more complicated then simply teaching simplefb about a clocks property, which is a really simple and clean patch.

First of all let me explain why this won't work. When should those subsystems call this "later then a late initcall" call ? the kms driver may very well be a kernel module, which will be loaded by userspace, so we would need this to be triggered by userspace, but when ?

Quoted 3 lines

When the initrd is done? What then if the kms driver is not in the initrd, so maybe when udev is done enumerating devices, but what then if the kernel is still enumerating hardware at this time?

Usually an initrd knows how to wait for all or a given set of devices to be probed. udev is pretty good for that.

Quoted 11 lines

Will we just put a sleep 30
in our initscripts to make sure the kernel is done scanning all busses, will that be long enough? Maybe we need to re-introduce the scsi_wait_done kernel module which was added as an ugly hack to allow userspace to wait for the kernel to have finished scanning scsi (and ata / sata) busses, for controllers found at the time the module was load. Which never worked reliable, because it would be possible that the controller itself still needed to be discovered. We've spend years cleaning up userspace enough to be able to kill scsi_wait_done. We've already been down this road of waiting for hw enumeration to be done, and the problem is that on modern systems *it is never done*.

Those are mostly integration issues. If you don't load the driver from initrd then I don't think anybody will complain when there's flicker when it finally gets loaded. The whole point of this is to have early access to the framebuffer and get display and graphics going as soon as possible, so please don't pull in hypothetical scenarios where people manually load drivers half an hour after the system has booted.

Quoted 2 lines

I've been doing low level Linux development for 10 years now and I've worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past.

Ah, so this is now officially a pissing contest, is it?

Quoted 2 lines

So second of all, Thierry, what exactly is the technical argument against adding support for dealing with clocks to simplefb ?

I've already stated technical arguments against it. You could just go back and read the thread again, or would you rather want me to repeat them for your convenience?

Quoted 1 line

I've heard a lot of people in favor of it,

Of course you have, all the people in favour of it are sunxi people that really want to see this merged because it gives them working display after U-Boot.

Quoted 2 lines

and only pretty much you against it,

At least Stephen also spoke out about it. But this quickly devolved into the kind of thread you want to get out of as quickly as possible, so I can't blame him for not continuing this discussion. In fact I'm very much regretting getting into this in the first place. You clearly have no intention to even consider any possible other solution that what was posted, so it's pretty useless to try and convince you.

Quoted 1 line

and your argument so far seems to boil down to "I don't like it",

I'm not surprised that you think so since you seem to be very selective in what you read (or register) and what you don't.

Quoted 5 lines

is not really a technical sound argument IMHO. Moreover all the alternatives seem to be horribly complicated and most of them also seem to have several issues which will make them simply not work reliable.

I'm not an unreasonable person, or at least I like to think so, and if there really isn't a better solution then I won't object to this patch any longer.

But as it stands I am not convinced that this is the best solution

so I think we should keep the discussion going for a bit and see if we really can't come up with something that will fix this for a more general case rather than just your particular use-case. It's evidently a recurring problem that others suffer from, too.

Thierry

Quoted 1 line · Thierry Reding

In another mail in this thread you've mentioned 2 uses for simplefb:

1) Serving as *the* fb until a kms driver is present
2) Allowing for output of early kernel boot messages.

I agree that those are the 2 use-cases we're dealing with here.

Note that the whole discussion we're having hear about disabling the clocks once hardware probing is "done", is moot for 1. One possible way mentioned of dealing with 1. is to mark these clocks as always on for now, and then we need to drop this hack (I cannot call this anything else then a hack), once we get kms, meaning interdependencies between the dropping patch and the kms adding patches, etc. All not very pretty.

For 2. We have what you call "mostly integration issues". Allow me to copy and paste in another bit of this thread here:

1) Serving as *the* fb until a kms driver is present

2) Allowing for output of early kernel boot messages.

Quoted 1 line

1) Serving as *the* fb until a kms driver is present

Why can't we whip together a real device specific fbdev driver and quit trying to run without a device specific driver in place? History has shown that to be bad idea except during early boot when there isn't a choice.

Quoted 1 line

2) Allowing for output of early kernel boot messages.

For this to work the kernel needs to know two things - address of the framebuffer and the x/y layout. Then just nuke this display when the kernel finishes the boot process.

Hi,

On 08/27/2014 02:44 PM, jonsmirl at gmail.com wrote:

<snip>

Quoted 2 lines

Quoted 1 line

1) Serving as *the* fb until a kms driver is present

Quoted 4 lines

Why can't we whip together a real device specific fbdev driver and quit trying to run without a device specific driver in place? History has shown that to be bad idea except during early boot when there isn't a choice.

Writing a kms driver and getting it upstream is not a simple task, and starting with a too limited implementation is dangerous as we need to get the devicetree binding first from the get go.

IOW this is not really a realistic answer to problem 1.

<snip>

Quoted 2 lines

Quoted 1 line

2) Allowing for output of early kernel boot messages.

Quoted 3 lines

For this to work the kernel needs to know two things - address of the framebuffer and the x/y layout. Then just nuke this display when the kernel finishes the boot process.

Please read my earlier mails on how completely flawed it is to write "when the kernel finishes the boot process". The problem is that there is no way to really determine when this happens.

Regards,

Hans

--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com. For more options, visit https://groups.google.com/d/optout.

Hi,

On 08/27/2014 02:44 PM, jonsmirl at gmail.com wrote:

<snip>

Quoted 6 lines

Quoted 1 line

1) Serving as *the* fb until a kms driver is present

Why can't we whip together a real device specific fbdev driver and quit trying to run without a device specific driver in place? History has shown that to be bad idea except during early boot when there isn't a choice.

Writing a kms driver and getting it upstream is not a simple task, and starting with a too limited implementation is dangerous as we need to get the devicetree binding first from the get go.

IOW this is not really a realistic answer to problem 1.

<snip>

Quoted 2 lines

Quoted 1 line

2) Allowing for output of early kernel boot messages.

Quoted 3 lines

For this to work the kernel needs to know two things - address of the framebuffer and the x/y layout. Then just nuke this display when the kernel finishes the boot process.

Please read my earlier mails on how completely flawed it is to write "when the kernel finishes the boot process". The problem is that there is no way to really determine when this happens.

Hi,

Quoted 2 lines · Thierry Reding

Can you please fix you mail setup. You're addressing me directly in this email, yet I'm neither in To: nor Cc: headers. That's really annoying.

Sorry, I'll pay closer attention that you're in the To when I'm directly addressing you next time.

Quoted 1 line · Hans de Goede

<snip>

Quoted 23 lines

Quoted 22 lines

Quoted 9 lines

Either way, Mark already suggested a different alternative in another subthread, namely to add a new kind of checkpoint at which subsystems can call a "disable unused" function that's called later than a late initcall. This is not going to fix things for you immediately because the clocks will still be switched off (only later) if you don't have a real driver that's claiming the clocks. But you can work around that for now by making the relevant clocks always on and remove that workaround once a real driver is loaded that knows how to handle them properly.

This will simply not work, and is a ton more complicated then simply teaching simplefb about a clocks property, which is a really simple and clean patch.

First of all let me explain why this won't work. When should those subsystems call this "later then a late initcall" call ? the kms driver may very well be a kernel module, which will be loaded by userspace, so we would need this to be triggered by userspace, but when ?

When the initrd is done? What then if the kms driver is not in the initrd, so maybe when udev is done enumerating devices, but what then if the kernel is still enumerating hardware at this time?

Quoted 2 lines

Usually an initrd knows how to wait for all or a given set of devices to be probed. udev is pretty good for that.

udev does not have a clue how to wait for all devices, it can be used to wait for a specific device to show up, that is true, but this requires adding specific logic for this (class of) device to the initrd init process, which is really sub-optimal.

Quoted 12 lines

Quoted 11 lines

Will we just put a sleep 30
in our initscripts to make sure the kernel is done scanning all busses, will that be long enough? Maybe we need to re-introduce the scsi_wait_done kernel module which was added as an ugly hack to allow userspace to wait for the kernel to have finished scanning scsi (and ata / sata) busses, for controllers found at the time the module was load. Which never worked reliable, because it would be possible that the controller itself still needed to be discovered. We've spend years cleaning up userspace enough to be able to kill scsi_wait_done. We've already been down this road of waiting for hw enumeration to be done, and the problem is that on modern systems *it is never done*.

Quoted 6 lines

Those are mostly integration issues. If you don't load the driver from initrd then I don't think anybody will complain when there's flicker when it finally gets loaded. The whole point of this is to have early access to the framebuffer and get display and graphics going as soon as possible, so please don't pull in hypothetical scenarios where people manually load drivers half an hour after the system has booted.

In another mail in this thread you've mentioned 2 uses for simplefb:

1) Serving as *the* fb until a kms driver is present
2) Allowing for output of early kernel boot messages.

I agree that those are the 2 use-cases we're dealing with here.

Note that the whole discussion we're having hear about disabling the clocks once hardware probing is "done", is moot for 1. One possible way mentioned of dealing with 1. is to mark these clocks as always on for now, and then we need to drop this hack (I cannot call this anything else then a hack), once we get kms, meaning interdependencies between the dropping patch and the kms adding patches, etc. All not very pretty.

For 2. We have what you call "mostly integration issues". Allow me to copy and paste in another bit of this thread here:

Quoted 3 lines

Quoted 2 lines

I've been doing low level Linux development for 10 years now and I've worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past.

Quoted 1 line

Ah, so this is now officially a pissing contest, is it?

No I'm merely trying to explain that I've a ton of experience with what you seem to think of as merely integration issues, and they are a royal PITA, and thus to be avoided at all costs. Really, please just trust me on this one, doing the whole add some later then late init call type is a bad idea.

Quoted 6 lines

Quoted 2 lines

So second of all, Thierry, what exactly is the technical argument against adding support for dealing with clocks to simplefb ?

I've already stated technical arguments against it. You could just go back and read the thread again, or would you rather want me to repeat them for your convenience?

It is a long thread, IIRC your main arguments against this are:

1) You don't want this kind of platform-specific knowledge in simplefb
2) That simplefb is supposed to be simple, and this is not simple

As for 1. several people have already argued that clocks as such are an abstract concept, found one many platforms and as such are not platform specific.

Any generic hw driver dealing with embedded platforms, be it ahci, ohci, ehci, etc. has ended up adding abstract support for things like clocks (and other resources). I say abstract support here, since to all these drivers the support was added in a way that no platform specific knowledge is necessary, they just deal with clocks not caring about the specific underlying clock implementation, clock names, etc.

This really is unavoidable. The framebuffer memory needed was mentioned as another example of a resource in this thread. You suggested using the new mem-reserve framework for this. I agree that that is the right thing to do, but even then we still need a way to tie this reserved mem to the simplefb, so that the kernel can know when it is no longer used and can be added to the general memory pool.

If you look at how almost all dt bindings work, then the dt node for a device specifies the resources needed, I don't see why simplefb would be so special that it should be different here. I agree that it is important to get the abstractions right here, but to me it seems that the right abstraction is to be consistent with how all other devices are abstracted and to add needed resources to the dt node for the simplefb.

As for 2. Yes this will add some more code to simplefb, more code but code which is not complicated in anyway, so the simplefb code would become larger but still stay simple. Were as all the suggested alternatives sofar are at least an order of magnitude more complicated, crossing many subsystems, demanding coordination with userspace, etc.

Quoted 4 lines

so I think we should keep the discussion going for a bit and see if we really can't come up with something that will fix this for a more general case rather than just your particular use-case. It's evidently a recurring problem that others suffer from, too.

Yes needing resources despite the device being behind some generic interface where the main driver code for that interface has no knowledge about such resources sofar, because e.g. on PC-s this was not necessary is a recurring problem, which has been solved already 3 times at least, see the ohci-, ehci- and ahci-platform drivers, and for all 3 the conclusion was that the best solution was to simple add the resources to the dt-node, and add code to claim and enable them.

On Wed, Aug 27, 2014 at 12:35:24PM +0200, Hans de Goede wrote:

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

So second of all, Thierry, what exactly is the technical argument against adding support for dealing with clocks to simplefb ?

I've already stated technical arguments against it. You could just go back and read the thread again, or would you rather want me to repeat them for your convenience?

Quoted 8 lines

It is a long thread, IIRC your main arguments against this are:

1) You don't want this kind of platform-specific knowledge in simplefb
2) That simplefb is supposed to be simple, and this is not simple

As for 1. several people have already argued that clocks as such are an abstract concept, found one many platforms and as such are not platform specific.

That alone doesn't justify this patch. SoCs often have a requirement to enable clocks in a specific order, for example. Other systems may need to coordinate clocks with resets in a specific order or enable power domains and such. All that is very SoC specific and if we don't make it very clear what this driver assumes about the state of the system, then we can't object to anybody adding support for those later on either. The result of that is going to be a driver that needs to handle all kinds of combinations of resources.

So while clocks themselves are fairly generic the way in which they need to be used is highly platform-specific.

Let me also reiterate what I've said a couple of times already. The issue here isn't that simplefb needs to manage these clocks in any way. Firmware has already set them up so that display works. The reason why you need this patch is so that the clock framework doesn't automatically turn them off. With the proposed patch the driver enables these clocks, but that's not what it needs to do, they are already on. It's a work around to prevent the clock framework into not disabling them.

So as far as I'm concerned this points to a fundamental issue with how the clock framework handles unused clocks. Therefore that is the problem that should be solved, not worked around.

Quoted 6 lines

Any generic hw driver dealing with embedded platforms, be it ahci, ohci, ehci, etc. has ended up adding abstract support for things like clocks (and other resources). I say abstract support here, since to all these drivers the support was added in a way that no platform specific knowledge is necessary, they just deal with clocks not caring about the specific underlying clock implementation, clock names, etc.

Yes, I know that. You and I both had a very similar discussion not so long ago. But the above aren't quite the same as simplefb. The devices follow at least a well-known standard (EHCI, OHCI, AHCI), so there's bound to be more commonalities between them than between the various display devices that simplefb potentially can support.

Interestingly though, if you look at what hardware really is supported by the generic drivers that deal with embedded platforms, there isn't all that much diversity (I've manually stripped out non-DTS occurrences since they aren't relevant for this discussion):

	arch/arm/boot/dts/rk3288.dtsi:199:              compatible = "generic-ehci";
	arch/arm/boot/dts/rk3288.dtsi:210:              compatible = "generic-ehci";

So there's a couple of SoCs and boards that actually are generic enough to work with a generic driver. And then there's a whole bunch of other drivers for hardware that's compliant with the same standard yet needs different drivers.

Quoted 6 lines

This really is unavoidable. The framebuffer memory needed was mentioned as another example of a resource in this thread. You suggested using the new mem-reserve framework for this. I agree that that is the right thing to do, but even then we still need a way to tie this reserved mem to the simplefb, so that the kernel can know when it is no longer used and can be added to the general memory pool.

Yes, memory handling seems like it isn't handled optimally by simplefb. To be fair the simplefb driver predates the reserved-memory bindings. I think it might be worthwhile to investigate on how to extend the binding to make use of that, though. I think the assumption currently is that the framebuffer memory will be lost forever (it needs to be carved out of the physical memory that the kernel gets to see). I couldn't find any code that returned reserved-memory to the kernel, but it's certainly a feature that I think we want. Especially if the firmware framebuffer is large (as in 1080p).

Quoted 7 lines

If you look at how almost all dt bindings work, then the dt node for a device specifies the resources needed, I don't see why simplefb would be so special that it should be different here. I agree that it is important to get the abstractions right here, but to me it seems that the right abstraction is to be consistent with how all other devices are abstracted and to add needed resources to the dt node for the simplefb.

But simplefb is fundamentally different from other devices. It isn't a physical device at all. It's a virtual device that reuses resources as set up by some other piece of code (firmware). It implies that there's nothing that needs to be managed. It should only create a framebuffer with the given parameters and allow the kernel (and userspace) to render into it.

The only way you can deal with such virtual, completely generic devices is by being very explicit about the requirements. For simplefb the assumptions are that firmware set everything up and passes information about what it set up to the kernel so that it can be reused.

None of the resources need to be explicitly managed because they have all been properly configured. For that reason, again, I think the right way is for the kernel not to switch off any of the used resources.

Quoted 5 lines

As for 2. Yes this will add some more code to simplefb, more code but code which is not complicated in anyway, so the simplefb code would become larger but still stay simple. Were as all the suggested alternatives sofar are at least an order of magnitude more complicated, crossing many subsystems, demanding coordination with userspace, etc.

I'm not objecting to this patch because of the complexity, but because it is an illusion that the code is in any way generic.

Hi,

Quoted 1 line · Thierry Reding

On Wed, Aug 27, 2014 at 12:35:24PM +0200, Hans de Goede wrote:

<snip>

Quoted 16 lines

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

So second of all, Thierry, what exactly is the technical argument against adding support for dealing with clocks to simplefb ?

I've already stated technical arguments against it. You could just go back and read the thread again, or would you rather want me to repeat them for your convenience?

Quoted 8 lines

It is a long thread, IIRC your main arguments against this are:

1) You don't want this kind of platform-specific knowledge in simplefb
2) That simplefb is supposed to be simple, and this is not simple

As for 1. several people have already argued that clocks as such are an abstract concept, found one many platforms and as such are not platform specific.

Quoted 8 lines

That alone doesn't justify this patch. SoCs often have a requirement to enable clocks in a specific order, for example. Other systems may need to coordinate clocks with resets in a specific order or enable power domains and such. All that is very SoC specific and if we don't make it very clear what this driver assumes about the state of the system, then we can't object to anybody adding support for those later on either. The result of that is going to be a driver that needs to handle all kinds of combinations of resources.

The problems your talking about here all have to do with ordering how things are enabled, but what we're talking about here is keeping things enabled which are already enabled by the bootloader, so there is no ordering problem.

Quoted 11 lines

So while clocks themselves are fairly generic the way in which they need to be used is highly platform-specific.

Let me also reiterate what I've said a couple of times already. The issue here isn't that simplefb needs to manage these clocks in any way. Firmware has already set them up so that display works. The reason why you need this patch is so that the clock framework doesn't automatically turn them off. With the proposed patch the driver enables these clocks, but that's not what it needs to do, they are already on. It's a work around to prevent the clock framework into not disabling them.

Quoted 3 lines

So as far as I'm concerned this points to a fundamental issue with how the clock framework handles unused clocks. Therefore that is the problem that should be solved, not worked around.

Hmm I see, in my mind the problem is not that the clk framework disables unused clocks, but that no one is marking the clocks in question as used. Someone should mark these clocks as used so that they do not get disabled.

We could add a clk_mark_in_use function and have the simplefb patch call that instead of clk_prepare_and_enable, or maybe even better just only claim the clks and teach the clk framework to not disable claimed clk in its cleanup run. That would make it clear that simplefb is not enabling anything, just claiming resource its need to avoid them getting removed from underneath simplefb, would that work for you ?

Quoted 13 lines

Quoted 6 lines

Any generic hw driver dealing with embedded platforms, be it ahci, ohci, ehci, etc. has ended up adding abstract support for things like clocks (and other resources). I say abstract support here, since to all these drivers the support was added in a way that no platform specific knowledge is necessary, they just deal with clocks not caring about the specific underlying clock implementation, clock names, etc.

Yes, I know that. You and I both had a very similar discussion not so long ago. But the above aren't quite the same as simplefb. The devices follow at least a well-known standard (EHCI, OHCI, AHCI), so there's bound to be more commonalities between them than between the various display devices that simplefb potentially can support.

Quoted 4 lines

Interestingly though, if you look at what hardware really is supported by the generic drivers that deal with embedded platforms, there isn't all that much diversity (I've manually stripped out non-DTS occurrences since they aren't relevant for this discussion):

That is because the generic platform drivers were only added recently to stop the insanity where each new soc got its own ohci-soc.c / ehci-soc.c file.

Quoted 2 lines

	arch/arm/boot/dts/rk3288.dtsi:199:              compatible = "generic-ehci";
	arch/arm/boot/dts/rk3288.dtsi:210:              compatible = "generic-ehci";

(offtopic) And I see that despite the recent addition, we actually already have our first non sunxi user, good, I didn't even know the rockchip guys were using this :)

And I know that there are quite a few more in the pipeline (waiting for their usb phy and dt patches to get merged).

Quoted 4 lines

So there's a couple of SoCs and boards that actually are generic enough to work with a generic driver. And then there's a whole bunch of other drivers for hardware that's compliant with the same standard yet needs different drivers.

No, there are a lot of other drivers which were written before someone decided that having 10-20 drivers which were 90% copy and paste of each other was a bad idea, but we're really going offtopic here.

Quoted 19 lines

Quoted 7 lines

If you look at how almost all dt bindings work, then the dt node for a device specifies the resources needed, I don't see why simplefb would be so special that it should be different here. I agree that it is important to get the abstractions right here, but to me it seems that the right abstraction is to be consistent with how all other devices are abstracted and to add needed resources to the dt node for the simplefb.

But simplefb is fundamentally different from other devices. It isn't a physical device at all. It's a virtual device that reuses resources as set up by some other piece of code (firmware). It implies that there's nothing that needs to be managed. It should only create a framebuffer with the given parameters and allow the kernel (and userspace) to render into it.

The only way you can deal with such virtual, completely generic devices is by being very explicit about the requirements. For simplefb the assumptions are that firmware set everything up and passes information about what it set up to the kernel so that it can be reused.

Quoted 3 lines

None of the resources need to be explicitly managed because they have all been properly configured. For that reason, again, I think the right way is for the kernel not to switch off any of the used resources.

The key word here is "the used resources" to me this is not about simlefb managing resources, but marking them as used as long as it needs them, like it will need to do for the reserved mem chunk.

Quoted 4 lines

The problems your talking about here all have to do with ordering how things are enabled, but what we're talking about here is keeping things enabled which are already enabled by the bootloader, so there is no ordering problem.

See what I did there? I tricked you into saying what I've been saying all along. =)

It's about keeping things enabled which have been enabled by the bootloader. That's the root of the problem.

Quoted 10 lines

Hmm I see, in my mind the problem is not that the clk framework disables unused clocks, but that no one is marking the clocks in question as used. Someone should mark these clocks as used so that they do not get disabled.

We could add a clk_mark_in_use function and have the simplefb patch call that instead of clk_prepare_and_enable, or maybe even better just only claim the clks and teach the clk framework to not disable claimed clk in its cleanup run. That would make it clear that simplefb is not enabling anything, just claiming resource its need to avoid them getting removed from underneath simplefb, would that work for you ?

That would be more accurate, but it boils down to the same problem where we still need a driver to claim the resources in some way whereas the problem is fundamentally one where the bootloader should be telling the kernel not to disable it. It's in fact the bootloader that's claiming the resources.

Quoted 3 lines

No, there are a lot of other drivers which were written before someone decided that having 10-20 drivers which were 90% copy and paste of each other was a bad idea, but we're really going offtopic here.

The copy and paste is for code that's platform specific. The clocks have different names, resets are different, supplies are different. The fact that many can currently use the same driver is likely just coincidence rather than design and it's entirely possible that at some point they'll add support for a more advanced feature that makes them incompatible with the rest of the generic drivers. And then you have a big mess because you need to add quirks all over the place.

And this isn't all that far off-topic, since simplefb also needs to deal with this kind of situation. And what I've been arguing is that in order to really be generic it has to make assumptions, one of which is that it uses only resources that it doesn't need to explicitly handle.

Quoted 3 lines

The key word here is "the used resources" to me this is not about simlefb managing resources, but marking them as used as long as it needs them, like it will need to do for the reserved mem chunk.

The difference between the clocks and the memory resource is that the driver needs to directly access the memory (it needs to map it and provide a userspace mapping for it) whereas it doesn't need to touch the clocks (except to workaround a Linux-specific implementation detail).

Hi,

On 08/27/2014 04:16 PM, Thierry Reding wrote:

<snip>

Quoted 7 lines

Quoted 4 lines

The problems your talking about here all have to do with ordering how things are enabled, but what we're talking about here is keeping things enabled which are already enabled by the bootloader, so there is no ordering problem.

See what I did there? I tricked you into saying what I've been saying all along. =)

He he :)

Quoted 2 lines

It's about keeping things enabled which have been enabled by the bootloader. That's the root of the problem.

Ok so we all seem to largely agree on most things here:

1) Somehow some clks must be marked used so as to not get disabled
2) We don't want to hardcode these clocks into the kernel (sunxi) clk driver, instead the bootloader should tell the kernel about these clocks.

So the only point of discussion left seems to be how to do 2...

<snip>

Quoted 11 lines

Quoted 10 lines

Hmm I see, in my mind the problem is not that the clk framework disables unused clocks, but that no one is marking the clocks in question as used. Someone should mark these clocks as used so that they do not get disabled.

We could add a clk_mark_in_use function and have the simplefb patch call that instead of clk_prepare_and_enable, or maybe even better just only claim the clks and teach the clk framework to not disable claimed clk in its cleanup run. That would make it clear that simplefb is not enabling anything, just claiming resource its need to avoid them getting removed from underneath simplefb, would that work for you ?

Quoted 5 lines

That would be more accurate, but it boils down to the same problem where we still need a driver to claim the resources in some way whereas the problem is fundamentally one where the bootloader should be telling the kernel not to disable it. It's in fact the bootloader that's claiming the resources.

Yes, but those resources do not belong to the bootloader in a sense that traditional bootloader / firmware claimed resources (e.g. acpi reserved resources) do. These traditional resources are claimed for ever.

Where as these resources are claimed by the bootloader to keep the simplefb it provides working, as soon as the simplefb is no longer used, they become unused.

<snip off-topic generic-ehci discussion>

Quoted 12 lines

Quoted 3 lines

No, there are a lot of other drivers which were written before someone decided that having 10-20 drivers which were 90% copy and paste of each other was a bad idea, but we're really going offtopic here.

The copy and paste is for code that's platform specific. The clocks have different names, resets are different, supplies are different. The fact that many can currently use the same driver is likely just coincidence rather than design and it's entirely possible that at some point they'll add support for a more advanced feature that makes them incompatible with the rest of the generic drivers. And then you have a big mess because you need to add quirks all over the place.

Quoted 4 lines

And this isn't all that far off-topic, since simplefb also needs to deal with this kind of situation. And what I've been arguing is that in order to really be generic it has to make assumptions, one of which is that it uses only resources that it doesn't need to explicitly handle.

I can understand that you're worried about generic ?hci drivers dealing with clocks / resets / etc. As there may be strict ordering requirements there, but for simplefb that is not the case.

All we're asking for is for a way to communicate 2 things to the kernel:

1) These resources are in use (we are not asking the kernel to do anything with them, rather the opposite, we're asking to leave them alone so no ordering issues)

2) Tie these resources to simplefb so that the kernel can know when they are no longer in use, and it may e.g. re-use the memory

To me the most logical and also most "correct" way of modelling this is to put the resources inside the simplefb dt node.

<snip>

Quoted 8 lines

Quoted 3 lines

The key word here is "the used resources" to me this is not about simlefb managing resources, but marking them as used as long as it needs them, like it will need to do for the reserved mem chunk.

The difference between the clocks and the memory resource is that the driver needs to directly access the memory (it needs to map it and provide a userspace mapping for it) whereas it doesn't need to touch the clocks (except to workaround a Linux-specific implementation detail).

Erm, no, the need to map the memory and the memory being a resource which may be released are an orthogonal problem. E.g. a system with dedicated framebuffer memory won't need to use a reserved main-memory chunk, nor need to worry about returning that mem when simplefb is no longer in use.

Regards,

Hans

--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com. For more options, visit https://groups.google.com/d/optout.

Quoted 4 lines

2) We don't want to hardcode these clocks into the kernel (sunxi) clk driver, instead the bootloader should tell the kernel about these clocks.

So the only point of discussion left seems to be how to do 2...

Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and whip together a device specific driver that claims the proper resources? And just implement the minimal about of fbdev possible? fbdev already is a driver library.

Quoted 3 lines · jonsmirl at gmail.com

Quoted 3 lines

2) We don't want to hardcode these clocks into the kernel (sunxi) clk driver, instead the bootloader should tell the kernel about these clocks.

Quoted 6 lines

Quoted 1 line

So the only point of discussion left seems to be how to do 2...

Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and whip together a device specific driver that claims the proper resources? And just implement the minimal about of fbdev possible? fbdev already is a driver library.

Like... drivers/video/fbdev/offb.c?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Quoted 11 lines · Geert Uytterhoeven

Quoted 9 lines · jonsmirl at gmail.com

Quoted 4 lines

2) We don't want to hardcode these clocks into the kernel (sunxi) clk driver, instead the bootloader should tell the kernel about these clocks.

So the only point of discussion left seems to be how to do 2...

Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and whip together a device specific driver that claims the proper resources? And just implement the minimal about of fbdev possible? fbdev already is a driver library.

Quoted 1 line

Like... drivers/video/fbdev/offb.c?

I'd probably reclassify drivers/video/fbdev/simplefb.c as a skeleton and use it as a template for making device specific versions of it.

I don't see why there is so much resistance to just making device specific fb drivers. Whenever the KMS driver gets written just disable the device specific fb driver in the build.

Quoted 18 lines · jonsmirl at gmail.com

Quoted 4 lines · Geert Uytterhoeven

Quoted 3 lines · jonsmirl at gmail.com

Quoted 3 lines

2) We don't want to hardcode these clocks into the kernel (sunxi) clk driver, instead the bootloader should tell the kernel about these clocks.

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

So the only point of discussion left seems to be how to do 2...

Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and whip together a device specific driver that claims the proper resources? And just implement the minimal about of fbdev possible? fbdev already is a driver library.

Like... drivers/video/fbdev/offb.c?

I'd probably reclassify drivers/video/fbdev/simplefb.c as a skeleton and use it as a template for making device specific versions of it.