simplefb: Add clock support to simplefb

A simple-framebuffer node represents a framebuffer setup by the firmware / bootloader. Such a framebuffer may have a number of clocks in use, add a property to communicate this to the OS.

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

Reviewed-by Mike Turquette <[email protected]>

---
 Documentation/devicetree/bindings/video/simple-framebuffer.txt | 3 +++
 1 file changed, 3 insertions(+)

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

index 70c26f3..e75478e 100644

Quoted 18 lines · Hans de Goede

A simple-framebuffer node represents a framebuffer setup by the firmware / bootloader. Such a framebuffer may have a number of clocks in use, add a property to communicate this to the OS.

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

Reviewed-by Mike Turquette <[email protected]>

---
 Documentation/devicetree/bindings/video/simple-framebuffer.txt | 3 +++
 1 file changed, 3 insertions(+)

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

index 70c26f3..e75478e 100644
@@ -14,6 +14,9 @@ Required properties:
   - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
   - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
 

Documentation/devicetree/bindings/video/simple-framebuffer.txt Required properties: · line 18

Quoted 2 lines

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

+Optional properties:
+- clocks : List of clocks used by the framebuffer

A simple framebuffer represents a memory region. So now you are saying this memory region has a clock. That does not make sense and the description of simple framebuffer is no longer correct.

I assume you are trying to work around the clock framework turning off clocks on you. If you bothered to describe the clock such that it can be turned off by the kernel, you should then describe it's actual connection to the hardware.

Furthermore, when you do add actual driver(s) for the hardware, that driver will not be able to turn off the clocks because the simplefb has forced them on. I can see the cases where you want to use simple-framebuffer early for splash screen or something before the real driver is up and use it to provide the default video mode for the real driver.

From: Luc Verhaegen <[email protected]>

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

[hdegoede at redhat.com: drop dev_err on kzalloc failure]

Reviewed-by Hans de Goede <[email protected]>

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

---
 drivers/video/fbdev/simplefb.c | 100 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 2 deletions(-)

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

index b7d5c08..f329cc1 100644
@@ -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",

@@ -165,8 +166,98 @@ 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)

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

+		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) {
+			clk_disable_unprepare(clock);

Hi,

Quoted 63 lines · Hans de Goede

From: Luc Verhaegen <[email protected]>

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

[hdegoede at redhat.com: drop dev_err on kzalloc failure]

Reviewed-by Hans de Goede <[email protected]>

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

---
 drivers/video/fbdev/simplefb.c | 100 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 2 deletions(-)

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

index b7d5c08..f329cc1 100644
@@ -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",

@@ -165,8 +166,98 @@ 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);
+

drivers/video/fbdev/simplefb.c static int simplefb_parse_pd(struct platform_device *pdev, · line 204

Quoted 4 lines

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

+	if (dev_get_platdata(&pdev->dev) || !np)
+		return;
+
+	clock_count = of_clk_get_parent_count(np);

This looks like it does what you expect, but its name and the fact that it's in the clk-provider.h file makes me wonder if you're not using the wrong side of the abstraction.

drivers/video/fbdev/simplefb.c static int simplefb_parse_pd(struct platform_device *pdev, · line 207

Quoted 4 lines

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

+	for (i = 0; i < clock_count; i++) {
+		struct simplefb_clock *entry;
+		struct clk *clock = of_clk_get(np, i);

devm_clk_get?

Quoted 25 lines

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

+		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) {
+			clk_disable_unprepare(clock);
+			clk_put(clock);
+			continue;
+		}
+
+		entry->clock = clock;

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

Quoted 5 lines

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

+		/*
+		 * add to the front of the list, so we disable clocks in the
+		 * correct order.
+		 */
+		list_add(&entry->list, list);

I really don't think this whole list dance is necessary, especially after reading this comment.

Hi,

Quoted 66 lines · Maxime Ripard

Hi,

Quoted 63 lines · Hans de Goede

From: Luc Verhaegen <[email protected]>

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

[hdegoede at redhat.com: drop dev_err on kzalloc failure]

Reviewed-by Hans de Goede <[email protected]>

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

---
 drivers/video/fbdev/simplefb.c | 100 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 2 deletions(-)

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

index b7d5c08..f329cc1 100644
@@ -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",

@@ -165,8 +166,98 @@ 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);
+

Quoted 8 lines

drivers/video/fbdev/simplefb.c static int simplefb_parse_pd(struct platform_device *pdev, · line 204

Quoted 4 lines

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

+	if (dev_get_platdata(&pdev->dev) || !np)
+		return;
+
+	clock_count = of_clk_get_parent_count(np);

This looks like it does what you expect, but its name and the fact that it's in the clk-provider.h file makes me wonder if you're not using the wrong side of the abstraction.

I'll check to see if there is something better, assuming Luc does not beat me to it.

Quoted 6 lines

drivers/video/fbdev/simplefb.c static int simplefb_parse_pd(struct platform_device *pdev, · line 207

Quoted 4 lines

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

+	for (i = 0; i < clock_count; i++) {
+		struct simplefb_clock *entry;
+		struct clk *clock = of_clk_get(np, i);

devm_clk_get?

Yes that would be better.

Quoted 25 lines

Quoted 25 lines

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

+		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) {
+			clk_disable_unprepare(clock);
+			clk_put(clock);
+			continue;
+		}
+
+		entry->clock = clock;

Quoted 8 lines

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

Quoted 5 lines

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

+		/*
+		 * add to the front of the list, so we disable clocks in the
+		 * correct order.
+		 */
+		list_add(&entry->list, list);

I really don't think this whole list dance is necessary, especially after reading this comment.

So you're suggesting to just make this an array, with say 5 entries, or ... ?

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

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