simplefb: add clock handling

simplefb: add clock handling

[Luc Verhaegen]

This is needed for the sunxi platform, where the u-boot initialized display 
engine gets disabled by the clocks framework if certain clocks are not 
claimed. Once these clocks are disabled, register content is lost, and there
is no turning back unless a full display driver is loaded, which kind of
beats the purpose of having simplefb running.

The lack of clock handling should plague more hardware, but so far rpi is the 
best known user of simplefb, and its stepmotherly handling of the arm core
has kept these sort of issues from the kernel.

The sunxi u-boot side code can be found here:
https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY

Patch 3 might be controversial, as this does not achieve anything real today,
since the status property in dt is only really evaluated when dealing with a
nodes memory. It still seems like a good idea to at least flag this memory or
node as disabled, as we really have no way back when the clocks get disabled.

Luc Verhaegen.

[PATCH 1/4] simplefb: formalize pseudo palette handling

[Luc Verhaegen]

Signed-off-by: Luc Verhaegen <libv@skynet.be>
---
 drivers/video/fbdev/simplefb.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 210f3a0..32be590 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -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) |
@@ -163,11 +165,16 @@ static int simplefb_parse_pd(struct platform_device *pdev,
 	return 0;
 }
 
+struct simplefb_par {
+	u32 palette[PSEUDO_PALETTE_SIZE];
+};
+
 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);
@@ -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;
 
 	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

[PATCH 2/4] simplefb: add goto error path to probe

[Luc Verhaegen]

Signed-off-by: Luc Verhaegen <libv@skynet.be>
---
 drivers/video/fbdev/simplefb.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 32be590..72a4f20 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -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);
 
 	return 0;
+
+ error_unmap:
+	iounmap(info->screen_base);
+
+ error_fb_release:
+	framebuffer_release(info);
+
+	return ret;
 }
 
 static int simplefb_remove(struct platform_device *pdev)
-- 
1.7.7

[PATCH 3/4] simplefb: disable dt node upon remove

[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 <libv@skynet.be>
---
 drivers/video/fbdev/simplefb.c |   43 ++++++++++++++++++++++++++++++++++++---
 1 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 72a4f20..74c4b2a 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -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

[PATCH 4/4] simplefb: add clock handling code

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

Signed-off-by: Luc Verhaegen <libv@skynet.be>
---
 drivers/video/fbdev/simplefb.c |  103 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 74c4b2a..481dbfd 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
+#include <linux/clk-provider.h>
 
 static struct fb_fix_screeninfo simplefb_fix = {
 	.id		= "simple",
@@ -191,8 +192,101 @@ static int simplefb_parse_pd(struct platform_device *pdev,
 	return 0;
 }
 
+/*
+ * Clock handling code.
+ *
+ * Here we handle the clocks property of our "simple-framebuffer" dt node.
+ * This is necessary so that we can make sure that any clocks needed by
+ * the display engine that the bootloader set up for us (and for which it
+ * provided a simplefb dt node), stay up, for the life of the simplefb
+ * driver.
+ *
+ * When the driver unloads, we cleanly disable, and then release the clocks.
+ */
+struct simplefb_clock {
+	struct list_head list;
+	struct clk *clock;
+};
+
+/*
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up simplefb, and the clock definitions in the device tree. Chances are
+ * that there are no adverse effects, and if there are, a clean teardown of
+ * the fb probe will not help us much either. So just complain and carry on,
+ * and hope that the user actually gets a working fb at the end of things.
+ */
+static void
+simplefb_clocks_init(struct platform_device *pdev, struct list_head *list)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int clock_count, i;
+
+	INIT_LIST_HEAD(list);
+
+	if (dev_get_platdata(&pdev->dev) || !np)
+		return;
+
+	clock_count = of_clk_get_parent_count(np);
+	for (i = 0; i < clock_count; i++) {
+		struct simplefb_clock *entry;
+		struct clk *clock = of_clk_get(np, i);
+		int ret;
+
+		if (IS_ERR(clock)) {
+			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
+			       __func__, i, PTR_ERR(clock));
+			continue;
+		}
+
+		ret = clk_prepare_enable(clock);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"%s: failed to enable clock %d: %d\n",
+			       __func__, i, ret);
+			clk_put(clock);
+			continue;
+		}
+
+		entry = kzalloc(sizeof(struct simplefb_clock), GFP_KERNEL);
+		if (!entry) {
+			dev_err(&pdev->dev,
+				"%s: failed to alloc clock %d list entry.\n",
+			       __func__, i);
+			clk_disable_unprepare(clock);
+			clk_put(clock);
+			continue;
+		}
+
+		entry->clock = clock;
+		/*
+		 * add to the front of the list, so we disable clocks in the
+		 * correct order.
+		 */
+		list_add(&entry->list, list);
+	}
+}
+
+static void
+simplefb_clocks_destroy(struct list_head *list)
+{
+	struct list_head *pos, *n;
+
+	list_for_each_safe(pos, n, list) {
+		struct simplefb_clock *entry =
+			container_of(pos, struct simplefb_clock, list);
+
+		list_del(&entry->list);
+
+		clk_disable_unprepare(entry->clock);
+		clk_put(entry->clock);
+		kfree(entry);
+	}
+}
+
 struct simplefb_par {
 	u32 palette[PSEUDO_PALETTE_SIZE];
+	struct list_head clock_list[1];
 };
 
 static int simplefb_probe(struct platform_device *pdev)
@@ -265,6 +359,8 @@ static int simplefb_probe(struct platform_device *pdev)
 	}
 	info->pseudo_palette = (void *) par->palette;
 
+	simplefb_clocks_init(pdev, par->clock_list);
+
 	dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
 			     info->fix.smem_start, info->fix.smem_len,
 			     info->screen_base);
@@ -276,14 +372,15 @@ 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);
-		goto error_unmap;
+		goto error_clocks;
 	}
 
 	dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
 
 	return 0;
 
- error_unmap:
+ error_clocks:
+	simplefb_clocks_destroy(par->clock_list);
 	iounmap(info->screen_base);
 
  error_fb_release:
@@ -299,8 +396,10 @@ static int simplefb_probe(struct platform_device *pdev)
 static int simplefb_remove(struct platform_device *pdev)
 {
 	struct fb_info *info = platform_get_drvdata(pdev);
+	struct simplefb_par *par = info->par;
 
 	unregister_framebuffer(info);
+	simplefb_clocks_destroy(par->clock_list);
 	framebuffer_release(info);
 	if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node)
 		simplefb_dt_disable(pdev);
-- 
1.7.7

[PATCH 1/4] simplefb: formalize pseudo palette handling

[David Herrmann]

Hi

On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
> Signed-off-by: Luc Verhaegen <libv@skynet.be>
> ---
>  drivers/video/fbdev/simplefb.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 210f3a0..32be590 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -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) |
> @@ -163,11 +165,16 @@ static int simplefb_parse_pd(struct platform_device *pdev,
>         return 0;
>  }
>
> +struct simplefb_par {
> +       u32 palette[PSEUDO_PALETTE_SIZE];
> +};
> +

I'd move that definition to the top of the file.

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

Patch is fine with me:
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

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

[PATCH 2/4] simplefb: add goto error path to probe

[David Herrmann]

Hi

On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
> Signed-off-by: Luc Verhaegen <libv@skynet.be>
> ---
>  drivers/video/fbdev/simplefb.c |   20 +++++++++++++-------
>  1 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 32be590..72a4f20 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -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);
>
>         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 <dh.herrmann@gmail.com>

Thanks
David

>  }
>
>  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

simplefb: add clock handling

[David Herrmann]

Hi

On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
> This is needed for the sunxi platform, where the u-boot initialized display
> engine gets disabled by the clocks framework if certain clocks are not
> claimed. Once these clocks are disabled, register content is lost, and there
> is no turning back unless a full display driver is loaded, which kind of
> beats the purpose of having simplefb running.
>
> The lack of clock handling should plague more hardware, but so far rpi is the
> best known user of simplefb, and its stepmotherly handling of the arm core
> has kept these sort of issues from the kernel.
>
> The sunxi u-boot side code can be found here:
> https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
>
> Patch 3 might be controversial, as this does not achieve anything real today,
> since the status property in dt is only really evaluated when dealing with a
> nodes memory. It still seems like a good idea to at least flag this memory or
> node as disabled, as we really have no way back when the clocks get disabled.

Hans de Goede shortly told me about this and, tbh, I am not very
pleased. Stephen tried to keep simplefd "as simple as possible", your
patch now adds hardware-specific features. To be fair, the patch is
simple and clocks are easy to handle, but I somehow fear we have to
add more and more hardware-support that is required to keep the
framebuffer active. This really defeats the purpose of simplefb.

The biggest question I have, is why do you add the clocks to your DT
at all? The framebuffer is set up by your boot-loader (or maybe
platform code) and should prepare the clocks. I don't see why we add
the clocks to DT now. If they're not added, then no-one will disable
them and simplefb works just fine, right?

Or is there some requirement to make DT a _complete_ hw-description?
Or is there some parent clock which might be used by other drivers and
controls the clock used for your display?

The only reason I see to add the clocks, is to support both, simplefb
*and* a hardware-driver. However, fbdev hand-over is horribly racy and
I'd much rather prefer a solution like sysfb that does proper handover
from primitive firmware-FBs to real hardware-drivers. In that case,
we'd have to figure out how to deal with clocks, but we could do it in
sysfb (which is meant to deal with those issues) with the benefit of
controlling hand-over directly, and allowing hw-dependent features.

My sysfb patches haven't been updated for a while, though, and you
have a working solution here. So I'm not going to NAK this, I
appreciate people working on this. But I'd like to get a discussion
started so we can at least figure out some nicer solution for the
future which might replace your code.

Btw., my current idea was to destroy the platform-devices of EFI/VGA
framebuffers during hand-over (because usually the underlying hw is
shutdown/modified). This automatically unloads simplefb and makes sure
the real hw-driver can be probed without other drivers touching the
hardware in parallel. Iff you unload the hw-driver afterwards, it can
re-create the firmware framebuffer and add a platform-device to make
simplefb load again (in case anyone really needs this).
Looking at your 3rd patch, I wonder whether this works with DT based
machines, or whether we'd have to use the "disable" mechanism you
chose. Any reason destroying the device would not work there? If yes,
I will look into the "disable-idea".

Thanks
David

simplefb: add clock handling

[Luc Verhaegen]

On Wed, Aug 13, 2014 at 09:54:21AM +0200, David Herrmann wrote:
> Hi
> 
> Hans de Goede shortly told me about this and, tbh, I am not very
> pleased. Stephen tried to keep simplefd "as simple as possible", your
> patch now adds hardware-specific features. To be fair, the patch is
> simple and clocks are easy to handle, but I somehow fear we have to
> add more and more hardware-support that is required to keep the
> framebuffer active. This really defeats the purpose of simplefb.

SimpleFB is a really deceptive name. If you lie to your ARM core all the 
time, and do most things behind its back, like RPi does, then you can 
get away with simplefb.

If not, it quickly becomes a lot more complicated, fast. This should've 
been called rpibootfb or something.

How does one deal with this dealbreaker issue of clocks? Or memory, 
apart from telling the kernel that the fb area is simple not accessible 
as normal ram. Or dpms?

> The biggest question I have, is why do you add the clocks to your DT
> at all?

I didn't, someone else did.

> The framebuffer is set up by your boot-loader (or maybe
> platform code) and should prepare the clocks. I don't see why we add
> the clocks to DT now. If they're not added, then no-one will disable
> them and simplefb works just fine, right?
> 
> Or is there some requirement to make DT a _complete_ hw-description?

Again, i am not responsible for that part. But my impression is that it 
should be absolutely complete, and i have been whining about actual bit 
offsets into registers being provided from the dt :(

> Or is there some parent clock which might be used by other drivers and
> controls the clock used for your display?

Not at this point for sunxi, but there will be.

> The only reason I see to add the clocks, is to support both, simplefb
> *and* a hardware-driver. However, fbdev hand-over is horribly racy and
> I'd much rather prefer a solution like sysfb that does proper handover
> from primitive firmware-FBs to real hardware-drivers. In that case,
> we'd have to figure out how to deal with clocks, but we could do it in
> sysfb (which is meant to deal with those issues) with the benefit of
> controlling hand-over directly, and allowing hw-dependent features.

Yes, a KMS driver is being worked on. I was, as a first order 
approximation (when i move it to mainline code), going to manually do 
some of this handover from simplefb.

> My sysfb patches haven't been updated for a while, though, and you
> have a working solution here. So I'm not going to NAK this, I
> appreciate people working on this. But I'd like to get a discussion
> started so we can at least figure out some nicer solution for the
> future which might replace your code.

So much for this being simple. again :)

> Btw., my current idea was to destroy the platform-devices of EFI/VGA
> framebuffers during hand-over (because usually the underlying hw is
> shutdown/modified). This automatically unloads simplefb and makes sure
> the real hw-driver can be probed without other drivers touching the
> hardware in parallel. Iff you unload the hw-driver afterwards, it can
> re-create the firmware framebuffer and add a platform-device to make
> simplefb load again (in case anyone really needs this).
> Looking at your 3rd patch, I wonder whether this works with DT based
> machines, or whether we'd have to use the "disable" mechanism you
> chose. Any reason destroying the device would not work there? If yes,
> I will look into the "disable-idea".

The clocks we currently claim handle the ahb gating of different display 
engines. Disable the gating bit, and all power is lost to the respective 
engine, and register content vanishes. So there is absolutely no coming 
back from that, apart from starting a proper display driver.

Luc Verhaegen.

[linux-sunxi] simplefb: add clock handling

[Koen Kooi]

Op 13 aug. 2014, om 09:54 heeft David Herrmann <dh.herrmann@gmail.com> het volgende geschreven:

> Hi
> 
> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>> This is needed for the sunxi platform, where the u-boot initialized display
>> engine gets disabled by the clocks framework if certain clocks are not
>> claimed. Once these clocks are disabled, register content is lost, and there
>> is no turning back unless a full display driver is loaded, which kind of
>> beats the purpose of having simplefb running.
>> 
>> The lack of clock handling should plague more hardware, but so far rpi is the
>> best known user of simplefb, and its stepmotherly handling of the arm core
>> has kept these sort of issues from the kernel.
>> 
>> The sunxi u-boot side code can be found here:
>> https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
>> 
>> Patch 3 might be controversial, as this does not achieve anything real today,
>> since the status property in dt is only really evaluated when dealing with a
>> nodes memory. It still seems like a good idea to at least flag this memory or
>> node as disabled, as we really have no way back when the clocks get disabled.
> 
> Hans de Goede shortly told me about this and, tbh, I am not very
> pleased. Stephen tried to keep simplefd "as simple as possible", your
> patch now adds hardware-specific features. To be fair, the patch is
> simple and clocks are easy to handle, but I somehow fear we have to
> add more and more hardware-support that is required to keep the
> framebuffer active. This really defeats the purpose of simplefb.
> 
> The biggest question I have, is why do you add the clocks to your DT
> at all? The framebuffer is set up by your boot-loader (or maybe
> platform code) and should prepare the clocks. I don't see why we add
> the clocks to DT now. If they're not added, then no-one will disable
> them and simplefb works just fine, right?

All clocks known to linux without a consumer will get disabled on most (all?) ARM systems to save power. Years ago OMAP had a Kconfig option to change that behaviour and add printk warnings for the clocks it would touch. 
To be honest, I don't get why sunxi needs a simplefb to begin with, only a proper kms/drm driver is needed which would register the clocks it needs properly. These patches and discussion seem like a lot of effort wasted on the wrong thing. But I can't complain about that since I'm not the one doing the work. 

regards,

Koen

[linux-sunxi] simplefb: add clock handling

[Hans de Goede]

Hi,

On 08/13/2014 10:21 AM, Koen Kooi wrote:
> 
> Op 13 aug. 2014, om 09:54 heeft David Herrmann <dh.herrmann@gmail.com> het volgende geschreven:
> 
>> Hi
>>
>> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>> This is needed for the sunxi platform, where the u-boot initialized display
>>> engine gets disabled by the clocks framework if certain clocks are not
>>> claimed. Once these clocks are disabled, register content is lost, and there
>>> is no turning back unless a full display driver is loaded, which kind of
>>> beats the purpose of having simplefb running.
>>>
>>> The lack of clock handling should plague more hardware, but so far rpi is the
>>> best known user of simplefb, and its stepmotherly handling of the arm core
>>> has kept these sort of issues from the kernel.
>>>
>>> The sunxi u-boot side code can be found here:
>>> https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
>>>
>>> Patch 3 might be controversial, as this does not achieve anything real today,
>>> since the status property in dt is only really evaluated when dealing with a
>>> nodes memory. It still seems like a good idea to at least flag this memory or
>>> node as disabled, as we really have no way back when the clocks get disabled.
>>
>> Hans de Goede shortly told me about this and, tbh, I am not very
>> pleased. Stephen tried to keep simplefd "as simple as possible", your
>> patch now adds hardware-specific features. To be fair, the patch is
>> simple and clocks are easy to handle, but I somehow fear we have to
>> add more and more hardware-support that is required to keep the
>> framebuffer active. This really defeats the purpose of simplefb.
>>
>> The biggest question I have, is why do you add the clocks to your DT
>> at all? The framebuffer is set up by your boot-loader (or maybe
>> platform code) and should prepare the clocks. I don't see why we add
>> the clocks to DT now. If they're not added, then no-one will disable
>> them and simplefb works just fine, right?
> 
> All clocks known to linux without a consumer will get disabled on most (all?) ARM systems to save power. Years ago OMAP had a Kconfig option to change that behaviour and add printk warnings for the clocks it would touch. 
> To be honest, I don't get why sunxi needs a simplefb to begin with, only a proper kms/drm driver is needed which would register the clocks it needs properly. These patches and discussion seem like a lot of effort wasted on the wrong thing. But I can't complain about that since I'm not the one doing the work. 

I believe that having some simple generic fb driver will be useful
on non x86, since we don't have vga-console there, and most distros
will build kms drivers as modules. Having the kernel / initrd code being
able to show output (like e.g. missing symbols in the kms drivers) seems
a very useful feature to me.

The way I envision this to work is:

u-boot lights up display, if it fails to load the kernel / ftd / ramdisk,
it can show this on the display

kernel takes over using something like simplefb (built into the kernel)
for its initial output / any error messages.

initrd loads kms, kms takes over.

This way we've a way to show error messages during boot at all times.

As we start supporting more ARM htpc boxes out of the box, telling the
user to hook up a serial console (which often involves soldering wires
to some test points) when things don't work really is not a viable
answer.

Regards,

Hans

[PATCH 3/4] simplefb: disable dt node upon remove

[Grant Likely]

On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
> 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 <libv@skynet.be>

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.

g.

> ---
>  drivers/video/fbdev/simplefb.c |   43 ++++++++++++++++++++++++++++++++++++---
>  1 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 72a4f20..74c4b2a 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -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

[PATCH 1/4] simplefb: formalize pseudo palette handling

[Geert Uytterhoeven]

On Wed, Aug 13, 2014 at 9:25 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> @@ -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>

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

[PATCH 3/4] simplefb: disable dt node upon remove

[David Herrmann]

Hi

On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>> 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 <libv@skynet.be>
>
> 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.

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

[PATCH 1/4] simplefb: formalize pseudo palette handling

[David Herrmann]

Hi

On Wed, Aug 13, 2014 at 10:46 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Aug 13, 2014 at 9:25 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> @@ -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>

"pseudo_palette" is "void*", so not at all.

Thanks
David

[PATCH 3/4] simplefb: disable dt node upon remove

[Grant Likely]

On Wed, Aug 13, 2014 at 9:49 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>> 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 <libv@skynet.be>
>>
>> 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?

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?

> 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

[PATCH 3/4] simplefb: disable dt node upon remove

[David Herrmann]

Hi

On Wed, Aug 13, 2014 at 11:23 AM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Wed, Aug 13, 2014 at 9:49 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
>> <grant.likely@secretlab.ca> wrote:
>>> * 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.

Ok, fair enough. So we leave the DT untouched. That still allows
calling device_add() / device_del() on platform-devices, right?

> 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?

Sorry, I wasn't clear enough: I'm not arguing in favor of this patch.
I just want to figure out what to do once we implement
hardware-handover for graphics devices on non-x86 (which this series
is kinda preparing for). This patch just reminded me, that we could do
this on a DT level, instead of driver-core level. But I'm fine with
avoiding that, if you warn about complications.

Thanks
David

[PATCH 3/4] simplefb: disable dt node upon remove

[Luc Verhaegen]

On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote:
> 
> 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?

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.

[linux-sunxi] simplefb: add clock handling

[Koen Kooi]

Op 13 aug. 2014, om 10:36 heeft Hans de Goede <hdegoede@redhat.com> het volgende geschreven:

> Hi,
> 
> On 08/13/2014 10:21 AM, Koen Kooi wrote:
>> 
>> Op 13 aug. 2014, om 09:54 heeft David Herrmann <dh.herrmann@gmail.com> het volgende geschreven:
>> 
>>> Hi
>>> 
>>> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>> This is needed for the sunxi platform, where the u-boot initialized display
>>>> engine gets disabled by the clocks framework if certain clocks are not
>>>> claimed. Once these clocks are disabled, register content is lost, and there
>>>> is no turning back unless a full display driver is loaded, which kind of
>>>> beats the purpose of having simplefb running.
>>>> 
>>>> The lack of clock handling should plague more hardware, but so far rpi is the
>>>> best known user of simplefb, and its stepmotherly handling of the arm core
>>>> has kept these sort of issues from the kernel.
>>>> 
>>>> The sunxi u-boot side code can be found here:
>>>> https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
>>>> 
>>>> Patch 3 might be controversial, as this does not achieve anything real today,
>>>> since the status property in dt is only really evaluated when dealing with a
>>>> nodes memory. It still seems like a good idea to at least flag this memory or
>>>> node as disabled, as we really have no way back when the clocks get disabled.
>>> 
>>> Hans de Goede shortly told me about this and, tbh, I am not very
>>> pleased. Stephen tried to keep simplefd "as simple as possible", your
>>> patch now adds hardware-specific features. To be fair, the patch is
>>> simple and clocks are easy to handle, but I somehow fear we have to
>>> add more and more hardware-support that is required to keep the
>>> framebuffer active. This really defeats the purpose of simplefb.
>>> 
>>> The biggest question I have, is why do you add the clocks to your DT
>>> at all? The framebuffer is set up by your boot-loader (or maybe
>>> platform code) and should prepare the clocks. I don't see why we add
>>> the clocks to DT now. If they're not added, then no-one will disable
>>> them and simplefb works just fine, right?
>> 
>> All clocks known to linux without a consumer will get disabled on most (all?) ARM systems to save power. Years ago OMAP had a Kconfig option to change that behaviour and add printk warnings for the clocks it would touch. 
>> To be honest, I don't get why sunxi needs a simplefb to begin with, only a proper kms/drm driver is needed which would register the clocks it needs properly. These patches and discussion seem like a lot of effort wasted on the wrong thing. But I can't complain about that since I'm not the one doing the work. 
> 
> I believe that having some simple generic fb driver will be useful
> on non x86, since we don't have vga-console there, and most distros
> will build kms drivers as modules. Having the kernel / initrd code being
> able to show output (like e.g. missing symbols in the kms drivers) seems
> a very useful feature to me.
> 
> The way I envision this to work is:
> 
> u-boot lights up display, if it fails to load the kernel / ftd / ramdisk,
> it can show this on the display
> 
> kernel takes over using something like simplefb (built into the kernel)
> for its initial output / any error messages.
> 
> initrd loads kms, kms takes over.
> 
> This way we've a way to show error messages during boot at all times.
> 
> As we start supporting more ARM htpc boxes out of the box, telling the
> user to hook up a serial console (which often involves soldering wires
> to some test points) when things don't work really is not a viable
> answer.

So what you are saying is that the only reason it is needed is because some distros choose to build DRM drivers as modules. So as soon as they stop doing that the problem goes away, right?
Worse, the experience I have with ARM DRM drivers is that they fail horrible when being built as modules, but that's a different problem.

regards,

Koen

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[Luc Verhaegen]

On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote:
> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote:
> > 
> > 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?
> 
> 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.

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

Luc Verhaegen.

[linux-sunxi] simplefb: add clock handling

[David Herrmann]

Hi

On Wed, Aug 13, 2014 at 12:16 PM, Koen Kooi <koen@dominion.thruhere.net> wrote:
> So what you are saying is that the only reason it is needed is because some distros choose to build DRM drivers as modules. So as soon as they stop doing that the problem goes away, right?
> Worse, the experience I have with ARM DRM drivers is that they fail horrible when being built as modules, but that's a different problem.

Exactly. But there's no intention to stop building them as modules.
Imagine you build a kernel that's supposed to run on multiple
different platforms (like x86), you really don't want all DRM drivers
built-in. Instead, you load the correct driver during boot-up. To
still provide early graphics access, we use simplefb.

Note that there might be legitimate reasons to make DRM drivers
built-in. But at least general purpose distros avoid that.

Thanks
David

[linux-sunxi] simplefb: add clock handling

[Hans de Goede]

Hi,

On 08/13/2014 12:16 PM, Koen Kooi wrote:
> 
> Op 13 aug. 2014, om 10:36 heeft Hans de Goede <hdegoede@redhat.com> het volgende geschreven:
> 
>> Hi,
>>
>> On 08/13/2014 10:21 AM, Koen Kooi wrote:
>>>
>>> Op 13 aug. 2014, om 09:54 heeft David Herrmann <dh.herrmann@gmail.com> het volgende geschreven:
>>>
>>>> Hi
>>>>
>>>> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>>> This is needed for the sunxi platform, where the u-boot initialized display
>>>>> engine gets disabled by the clocks framework if certain clocks are not
>>>>> claimed. Once these clocks are disabled, register content is lost, and there
>>>>> is no turning back unless a full display driver is loaded, which kind of
>>>>> beats the purpose of having simplefb running.
>>>>>
>>>>> The lack of clock handling should plague more hardware, but so far rpi is the
>>>>> best known user of simplefb, and its stepmotherly handling of the arm core
>>>>> has kept these sort of issues from the kernel.
>>>>>
>>>>> The sunxi u-boot side code can be found here:
>>>>> https://groups.google.com/forum/#!topic/linux-sunxi/dPs958sIXvY
>>>>>
>>>>> Patch 3 might be controversial, as this does not achieve anything real today,
>>>>> since the status property in dt is only really evaluated when dealing with a
>>>>> nodes memory. It still seems like a good idea to at least flag this memory or
>>>>> node as disabled, as we really have no way back when the clocks get disabled.
>>>>
>>>> Hans de Goede shortly told me about this and, tbh, I am not very
>>>> pleased. Stephen tried to keep simplefd "as simple as possible", your
>>>> patch now adds hardware-specific features. To be fair, the patch is
>>>> simple and clocks are easy to handle, but I somehow fear we have to
>>>> add more and more hardware-support that is required to keep the
>>>> framebuffer active. This really defeats the purpose of simplefb.
>>>>
>>>> The biggest question I have, is why do you add the clocks to your DT
>>>> at all? The framebuffer is set up by your boot-loader (or maybe
>>>> platform code) and should prepare the clocks. I don't see why we add
>>>> the clocks to DT now. If they're not added, then no-one will disable
>>>> them and simplefb works just fine, right?
>>>
>>> All clocks known to linux without a consumer will get disabled on most (all?) ARM systems to save power. Years ago OMAP had a Kconfig option to change that behaviour and add printk warnings for the clocks it would touch. 
>>> To be honest, I don't get why sunxi needs a simplefb to begin with, only a proper kms/drm driver is needed which would register the clocks it needs properly. These patches and discussion seem like a lot of effort wasted on the wrong thing. But I can't complain about that since I'm not the one doing the work. 
>>
>> I believe that having some simple generic fb driver will be useful
>> on non x86, since we don't have vga-console there, and most distros
>> will build kms drivers as modules. Having the kernel / initrd code being
>> able to show output (like e.g. missing symbols in the kms drivers) seems
>> a very useful feature to me.
>>
>> The way I envision this to work is:
>>
>> u-boot lights up display, if it fails to load the kernel / ftd / ramdisk,
>> it can show this on the display
>>
>> kernel takes over using something like simplefb (built into the kernel)
>> for its initial output / any error messages.
>>
>> initrd loads kms, kms takes over.
>>
>> This way we've a way to show error messages during boot at all times.
>>
>> As we start supporting more ARM htpc boxes out of the box, telling the
>> user to hook up a serial console (which often involves soldering wires
>> to some test points) when things don't work really is not a viable
>> answer.
> 
> So what you are saying is that the only reason it is needed is because some distros choose to build DRM drivers as modules. So as soon as they stop doing that the problem goes away, right?

Right, except that that is not going to happen, see David's reply also.

Regards,

Hans

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[jonsmirl at gmail.com]

On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote:
> On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote:
>> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote:
>> >
>> > 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?
>>
>> 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.
>
> 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.

Some Linux mechanism inside the kernel needs to handle that
transition. Somehow simple needs to hang onto the clocks, let go of
the device, load KMS, then exit?



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



-- 
Jon Smirl
jonsmirl at gmail.com

[linux-sunxi] simplefb: add clock handling

[Koen Kooi]

Op 13 aug. 2014, om 14:17 heeft Stefan Monnier <monnier@iro.umontreal.ca> het volgende geschreven:

>> So what you are saying is that the only reason it is needed is because some
>> distros choose to build DRM drivers as modules.  So as soon as they stop
>> doing that the problem goes away, right?
> 
> I think many people will be very happy to be able to get some visual
> feedback on their boot problems.  Having to connect a serial console is
> a deal breaker for most people.

As was said earlier in the thread, a proper DRM/KMS driver doesn't prevent that. The only thing preventing that would be:

a) kernels with graphics drivers disabled
b) kernels with graphics drivers as modules that get loaded late.

Please stop insinuation that DRM/KMS drivers can't handle boot messages.

[PATCH 4/4] simplefb: add clock handling code

[Stephen Warren]

On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
> 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.

[PATCH 3/4] simplefb: disable dt node upon remove

[Stephen Warren]

On 08/13/2014 02:49 AM, David Herrmann wrote:
> Hi
>
> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>> 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 <libv@skynet.be>
>>
>> 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.

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.

[PATCH 1/4] simplefb: formalize pseudo palette handling

[Stephen Warren]

On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
> Signed-off-by: Luc Verhaegen <libv@skynet.be>

Patch description?

Assuming any comments anyone else had are addressed, patches 1 and 2 both,
Acked-by: Stephen Warren <swarren@nvidia.com>

[PATCH 4/4] simplefb: add clock handling code

[Luc Verhaegen]

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>
> 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.

Yes. Simplefb quickly becomes anything but, doesn't it. Perhaps DenialFB 
would've been a better name for it ;p

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

How was simplefb handled on tegra? Where is the code for that? I didn't 
see anything in u-boot for instance.

But the code for handling clocks where they are supposed to be handled 
is pretty generic from where i sit.

Luc Verhaegen.

[PATCH 4/4] simplefb: add clock handling code

[Maxime Ripard]

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
> >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.

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.

> 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?

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

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

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

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

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140813/a2333ac8/attachment.sig>

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[jonsmirl at gmail.com]

On Wed, Aug 13, 2014 at 12:44 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/13/2014 02:49 AM, David Herrmann wrote:
>>
>> Hi
>>
>> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
>> <grant.likely@secretlab.ca> wrote:
>>>
>>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>>
>>>> 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 <libv@skynet.be>
>>>
>>>
>>> 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.
>
>
> 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.

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



-- 
Jon Smirl
jonsmirl at gmail.com

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[Stephen Warren]

On 08/13/2014 11:26 AM, jonsmirl at gmail.com wrote:
> On Wed, Aug 13, 2014 at 12:44 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/13/2014 02:49 AM, David Herrmann wrote:
>>>
>>> Hi
>>>
>>> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
>>> <grant.likely@secretlab.ca> wrote:
>>>>
>>>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>>>
>>>>> 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 <libv@skynet.be>
>>>>
>>>>
>>>> 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.
>>
>>
>> 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.

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.

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[jonsmirl at gmail.com]

On Wed, Aug 13, 2014 at 1:34 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/13/2014 11:26 AM, jonsmirl at gmail.com wrote:
>>
>> On Wed, Aug 13, 2014 at 12:44 PM, Stephen Warren <swarren@wwwdotorg.org>
>> wrote:
>>>
>>> On 08/13/2014 02:49 AM, David Herrmann wrote:
>>>>
>>>>
>>>> Hi
>>>>
>>>> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
>>>> <grant.likely@secretlab.ca> wrote:
>>>>>
>>>>>
>>>>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>>>>
>>>>>>
>>>>>> 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 <libv@skynet.be>
>>>>>
>>>>>
>>>>>
>>>>> 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.
>>>
>>>
>>>
>>> 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.
>
>
> 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.

I believe we do have something like this - SPI core implements a lot
of core DT functions. All SPI nodes use this core. Then hardware
specific attributes are added.

The conflicts would need to get sorted out. That's why we should have
a schema in place for device tree. Then the properties for specific
video hardware would inherit from the properties for simplefb.

-- 
Jon Smirl
jonsmirl at gmail.com

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[Grant Likely]

On Wed, Aug 13, 2014 at 1:54 PM, jonsmirl at gmail.com <jonsmirl@gmail.com> wrote:
> On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote:
>> On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote:
>>> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote:
>>> >
>>> > 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?
>>>
>>> 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.
>>
>> 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.

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

[PATCH 3/4] simplefb: disable dt node upon remove

[Luc Verhaegen]

On Wed, Aug 13, 2014 at 08:14:37PM +0100, Grant Likely wrote:
> 
> 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.

Luc Verhaegen.

[PATCH 3/4] simplefb: disable dt node upon remove

[Stephen Warren]

On 08/13/2014 01:25 PM, Luc Verhaegen wrote:
> On Wed, Aug 13, 2014 at 08:14:37PM +0100, Grant Likely wrote:
>>
>> 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).

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.

[PATCH 3/4] simplefb: disable dt node upon remove

[Grant Likely]

On Wed, Aug 13, 2014 at 8:25 PM, Luc Verhaegen <libv@skynet.be> wrote:
> On Wed, Aug 13, 2014 at 08:14:37PM +0100, Grant Likely wrote:
>>
>> 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?

g.

[PATCH 3/4] simplefb: disable dt node upon remove

[Luc Verhaegen]

On Wed, Aug 13, 2014 at 01:58:36PM -0600, Stephen Warren wrote:
> On 08/13/2014 01:25 PM, Luc Verhaegen wrote:
>>
>> 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.

Ah ok, i hadn't immediately found a tie with the lcd code and cfbconsole 
code, so apparently i didn't dig down far enough.

Luc Verhaegen.

[PATCH 3/4] simplefb: disable dt node upon remove

[Luc Verhaegen]

On Wed, Aug 13, 2014 at 09:00:07PM +0100, Grant Likely wrote:
> 
> 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.

There really are two separate issues here:
1) making sure that nothing tries to use the freshly died simplefb 
again.
2) doing a clean handover to another, usually hw specific, driver.

1 is what i hoped to superficially solve with this patch.
2 can be very smart about things as the replacement driver actually 
should know the hw.

I kind of like the idea of an "only-once" (there must be a better name 
for this) property. This allows the driver/device infrastructure to 
handle things much more cleanly, and allows me to state this from u-boot 
for sunxi, while Stephen wouldn't need to for rpi. I currently don't 
know how or where this could be handled, i just know that this smells 
like a decent solution which could solve my concern while at least 
avoiding the probe issue you mentioned earlier.

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

Not at all. A big blob of KMS code exists for our sunxi-3.4 kernel, 
where i can load the original display driver quickly and easily. This 
tree predates most of DT. I was rather hoping that the simplefb stop-gap
didn't require me to have fully engineered everything yesterday already.

Luc Verhaegen.

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[jonsmirl at gmail.com]

On Wed, Aug 13, 2014 at 3:14 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Aug 13, 2014 at 1:54 PM, jonsmirl at gmail.com <jonsmirl@gmail.com> wrote:
>> On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>> On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote:
>>>> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote:
>>>> >
>>>> > 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?
>>>>
>>>> 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.
>>>
>>> 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.

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.

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



-- 
Jon Smirl
jonsmirl at gmail.com

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 08/13/2014 07:01 PM, Maxime Ripard wrote:
> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>> On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
>>> 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.
> 
> 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.
> 
>> 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?
> 
>> I thought there was a per-clock flag to prevent disabling an unused
>> clock?
> 
> No, last time I heard, Mike Turquette was against it.
> 
>> 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.
> 
>> 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.

Regards,

Hans

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[Grant Likely]

On Wed, Aug 13, 2014 at 9:41 PM, jonsmirl at gmail.com <jonsmirl@gmail.com> wrote:
> On Wed, Aug 13, 2014 at 3:14 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Wed, Aug 13, 2014 at 1:54 PM, jonsmirl at gmail.com <jonsmirl@gmail.com> wrote:
>>> On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>> On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote:
>>>>> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote:
>>>>> >
>>>>> > 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?
>>>>>
>>>>> 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.
>>>>
>>>> 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.
>
> 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.

[PATCH 2/4] simplefb: add goto error path to probe

[Luc Verhaegen]

On Wed, Aug 13, 2014 at 09:27:46AM +0200, David Herrmann wrote:
> Hi
> 
> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
> > Signed-off-by: Luc Verhaegen <libv@skynet.be>
> > ---
> >  drivers/video/fbdev/simplefb.c |   20 +++++++++++++-------
> >  1 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> > index 32be590..72a4f20 100644
> > --- a/drivers/video/fbdev/simplefb.c
> > +++ b/drivers/video/fbdev/simplefb.c
> > @@ -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);
> >
> >         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 <dh.herrmann@gmail.com>

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.

Luc Verhaegen.

[linux-sunxi] [PATCH 4/4] simplefb: add clock handling code

[Koen Kooi]

Op 14 aug. 2014, om 11:37 heeft Hans de Goede <hdegoede@redhat.com> het volgende geschreven:

> Hi,
> 
> On 08/13/2014 07:01 PM, Maxime Ripard wrote:
>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>>> On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
>>>> 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.
>> 
>> 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.
>> 
>>> 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?
>> 
>>> I thought there was a per-clock flag to prevent disabling an unused
>>> clock?
>> 
>> No, last time I heard, Mike Turquette was against it.
>> 
>>> 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.
>> 
>>> 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

[PATCH 2/4] simplefb: add goto error path to probe

[David Herrmann]

Hi

On Thu, Aug 14, 2014 at 12:29 PM, Luc Verhaegen <libv@skynet.be> wrote:
> On Wed, Aug 13, 2014 at 09:27:46AM +0200, David Herrmann wrote:
>> Hi
>>
>> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>> > Signed-off-by: Luc Verhaegen <libv@skynet.be>
>> > ---
>> >  drivers/video/fbdev/simplefb.c |   20 +++++++++++++-------
>> >  1 files changed, 13 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>> > index 32be590..72a4f20 100644
>> > --- a/drivers/video/fbdev/simplefb.c
>> > +++ b/drivers/video/fbdev/simplefb.c
>> > @@ -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);
>> >
>> >         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 <dh.herrmann@gmail.com>
>
> 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.

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

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

Thanks
David

[PATCH 2/4] simplefb: add goto error path to probe

[Luc Verhaegen]

On Thu, Aug 14, 2014 at 12:33:55PM +0200, David Herrmann wrote:
> Hi
> 
> I'd skip the leading whitespace and the newlines, like this:
> 
> +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/.

Will do. Thanks.

Luc Verhaegen.

[linux-sunxi] [PATCH 4/4] simplefb: add clock handling code

[Luc Verhaegen]

On Thu, Aug 14, 2014 at 12:31:50PM +0200, Koen Kooi wrote:
> 
> 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, for currently about 8kloc (the binary seems excessively big 
though), which is just display. This still lacks many key features (lcd 
gpio, backlight pwm, lvds, ints), so it will grow ~1kloc still.

Also, i constantly am re-loading this (how else does one develop any 
sizable amount of code?), so it does that cleanly and correctly, which 
seems quite contrary to your experience with ARM drm drivers.

Luc Verhaegen.

[linux-sunxi] [PATCH 4/4] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 08/14/2014 12:31 PM, Koen Kooi wrote:
> 
> Op 14 aug. 2014, om 11:37 heeft Hans de Goede <hdegoede@redhat.com> het volgende geschreven:
> 
>> Hi,
>>
>> On 08/13/2014 07:01 PM, Maxime Ripard wrote:
>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>>>> On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
>>>>> 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.
>>>
>>> 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.
>>>
>>>> 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?
>>>
>>>> I thought there was a per-clock flag to prevent disabling an unused
>>>> clock?
>>>
>>> No, last time I heard, Mike Turquette was against it.
>>>
>>>> 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.
>>>
>>>> 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 don't know how many we've today, but I do know that we will have
twice as more next year, and 4x as more the year after that, etc.

IOW this does not scale.

Regards,

Hans

[linux-sunxi] [PATCH 4/4] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 08/14/2014 12:57 PM, Luc Verhaegen wrote:
> On Thu, Aug 14, 2014 at 12:31:50PM +0200, Koen Kooi wrote:
>>
>> 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

That likely is with debug-info, try running "strip --strip-debug" on the .ko
file. Note don't use plain "strip" that ruins kernel modules.

Regards,

Hans

p.s.

I'm leaving home (and the internet) for vacation tomorrow. I'll be back
on Mon Aug 25.

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[jonsmirl at gmail.com]

On Thu, Aug 14, 2014 at 6:15 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Aug 13, 2014 at 9:41 PM, jonsmirl at gmail.com <jonsmirl@gmail.com> wrote:
>> On Wed, Aug 13, 2014 at 3:14 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Wed, Aug 13, 2014 at 1:54 PM, jonsmirl at gmail.com <jonsmirl@gmail.com> wrote:
>>>> On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>>> On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote:
>>>>>> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote:
>>>>>> >
>>>>>> > 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?
>>>>>>
>>>>>> 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.
>>>>>
>>>>> 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.
>>
>> 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.

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?

>
> g.



-- 
Jon Smirl
jonsmirl at gmail.com

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[Henrik Nordström]

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

> 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

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[jonsmirl at gmail.com]

On Thu, Aug 14, 2014 at 8:45 PM, Henrik Nordstr?m
<henrik@henriknordstrom.net> wrote:
> tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl at gmail.com:
>
>> 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.

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.

You can also argue that simplefb should never occur in a DTS file. It
is something that uboot will add. And then the kernel will remove when
simplefb loads KMS.

That logic should apply to all of these dynamic fields. You don't want
stale state data in the DT.

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



-- 
Jon Smirl
jonsmirl at gmail.com

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[Maxime Ripard]

On Thu, Aug 14, 2014 at 09:27:14PM -0400, jonsmirl at gmail.com wrote:
> On Thu, Aug 14, 2014 at 8:45 PM, Henrik Nordstr?m
> <henrik@henriknordstrom.net> wrote:
> > tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl at gmail.com:
> >
> >> 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.
> 
> 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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140815/3cb2347d/attachment.sig>

[linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove

[jonsmirl at gmail.com]

On Fri, Aug 15, 2014 at 2:43 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Aug 14, 2014 at 09:27:14PM -0400, jonsmirl at gmail.com wrote:
>> On Thu, Aug 14, 2014 at 8:45 PM, Henrik Nordstr?m
>> <henrik@henriknordstrom.net> wrote:
>> > tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl at gmail.com:
>> >
>> >> 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.
>>
>> 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.
>
> 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.

The graphic drivers have already been down this road before. Simplefb
should be a driver library not a driver. DRM is also a driver library.

So to implement simplefb whip up a real skeleton driver for the video
hardware that parses the DT and then pokes the appropriate info into
the simplefb library. Initially that driver will just include the
calls out to the simplefb library. Later on it can get KMS
implemented.  This real driver will get bound to the hardware and have
a dependency that loads the simplefb driver library.

Now there is no handover problem. Only a single driver is ever
attached to the hardware. When it wants to go into KMS mode it makes a
call into the simplefb library to shut down whatever it is doing.

But you still need a mechanism for uboot to signal that it has set up
simplefb. Adding a dynamic node like this might work...

video0: video at 01c21800 {
   compatible = "allwinner,sun4i-a10-video";
   clocks = <&apb0_gates 6>, <&ir0_clk>;
   clock-names = "apb", "video";
   interrupts = <0 5 4>;
   reg = <0x01c21800 0x40>;
   status = "enabled";
   simplefb {
       new namespace for simplefb parameters
   }
};

I would have suggested a driver library earlier but it has been a
while since I worked on DRM and I forgot about it.



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



-- 
Jon Smirl
jonsmirl at gmail.com

[PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
[...]
> > 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"?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140825/ea172bb2/attachment.sig>

[PATCH 4/4] simplefb: add clock handling code

[Maxime Ripard]

On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> [...]
> > > 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"?

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

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140825/4c060f75/attachment.sig>

[PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> > On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > > On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> > [...]
> > > > 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"?
> 
> 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.

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

Can you elaborate?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140825/68317adf/attachment.sig>

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 08/25/2014 03:39 PM, Thierry Reding wrote:
> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>>> [...]
>>>>> 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"?
>>
>> 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.

I can only assume that this problem was never hit on tegra because when
kms support (and thus also a clock driver for the video plls) was introduced
simplefb support was dropped at the same time, or the gates are not being
disabled for some other reason.

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.

Regards,

Hans

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> On 08/25/2014 03:39 PM, Thierry Reding wrote:
> > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> >>> [...]
> >>>>> 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"?
> >>
> >> 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.

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.

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

> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140825/5a2fa3c4/attachment-0001.sig>

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[jonsmirl at gmail.com]

On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>> On 08/25/2014 03:39 PM, Thierry Reding wrote:
>> > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>> >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>> >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>> >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>> >>> [...]
>> >>>>> 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"?
>> >>
>> >> 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.
>
> 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.


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



-- 
Jon Smirl
jonsmirl at gmail.com

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 08/25/2014 04:16 PM, Thierry Reding wrote:
> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>> On 08/25/2014 03:39 PM, Thierry Reding wrote:
>>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>>>>> [...]
>>>>>>> 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"?
>>>>
>>>> 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.
> 
> 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.
> 
>> 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.

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.

Or we add a single simple and clean patch to the simplefb driver for dealing
with clocks, and worry about all the other hypothetical problems later...

Regards,

Hans

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 08/25/2014 04:23 PM, jonsmirl at gmail.com wrote:
> On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>>> On 08/25/2014 03:39 PM, Thierry Reding wrote:
>>>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>>>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>>>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>>>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>>>>>> [...]
>>>>>>>> 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"?
>>>>>
>>>>> 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.
>>
>> 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.

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.

Then once we want to do a handover, all is needed is a single simplefb
unregister call, at which point simplefb will disable the clocks and
release them. Note that this will be a nop as they should already be
claimed and enabled by the kms driver at this time.

Regards,

Hans

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Aug 25, 2014 at 04:23:59PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 08/25/2014 04:16 PM, Thierry Reding wrote:
> > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> >> On 08/25/2014 03:39 PM, Thierry Reding wrote:
> >>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> >>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> >>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> >>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> >>>>> [...]
> >>>>>>> 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"?
> >>>>
> >>>> 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.
> > 
> > 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.
> > 
> >> 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.
> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140825/c50267c3/attachment.sig>

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Maxime Ripard]

On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote:
> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> > On 08/25/2014 03:39 PM, Thierry Reding wrote:
> > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> > >>> [...]
> > >>>>> 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"?
> > >>
> > >> 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.
> 
> 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.

> 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?

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140825/066f03bd/attachment-0001.sig>

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Aug 25, 2014 at 10:23:26AM -0400, jonsmirl at gmail.com wrote:
> On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> >> On 08/25/2014 03:39 PM, Thierry Reding wrote:
> >> > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> >> >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> >> >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> >> >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> >> >>> [...]
> >> >>>>> 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"?
> >> >>
> >> >> 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.
> >
> > 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.

That sounds like the most scalable solution so far. On the other hand,
as I understand it, the simplefb driver was designed to take over the
framebuffer set up by firmware, so it's somewhat odd that the driver
would have to deal with resources in the first place. If we push the
resource problem into the respective subsystems we keep the simplefb
driver completely hardware agnostic.

And we'll also be solving this problem for other types of drivers at the
same time. Firmware may after all initialize clocks and other resources
for other types of devices too. Handling resources in the drivers would
therefore imply that every driver needs to cope with this.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140825/15f8788c/attachment.sig>

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Aug 25, 2014 at 04:58:54PM +0200, Maxime Ripard wrote:
> On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote:
> > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> > > On 08/25/2014 03:39 PM, Thierry Reding wrote:
> > > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> > > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> > > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> > > >>> [...]
> > > >>>>> 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"?
> > > >>
> > > >> 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.
> > 
> > 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.

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

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140825/32926166/attachment.sig>

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Maxime Ripard]

On Mon, Aug 25, 2014 at 04:53:06PM +0200, Thierry Reding wrote:
> 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.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140825/69496455/attachment.sig>

[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[jonsmirl at gmail.com]

On Mon, Aug 25, 2014 at 10:53 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Aug 25, 2014 at 04:23:59PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 08/25/2014 04:16 PM, Thierry Reding wrote:
>> > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>> >> On 08/25/2014 03:39 PM, Thierry Reding wrote:
>> >>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>> >>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>> >>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>> >>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>> >>>>> [...]
>> >>>>>>> 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"?
>> >>>>
>> >>>> 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.
>> >
>> > 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.
>> >
>> >> 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.
>>
>> 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.

There should probably be a generic 'boot-initialized;' property that
can be added to any DT device node.   Then uboot can add that property
to the device node for anything it has turned on.

You could even use i