[PATCH v4 0/5] simplefb: add clock handling code

[PATCH v4 0/5] simplefb: add clock handling code

[Hans de Goede]

Hi Tomi,

Here is v4 of the patch-set to add clocks support to simplefb.

Changes in v2:
-Added "simplefb: Add simplefb MAINTAINERS entry" patch

Changes in v3:
-Improved description of simplefb binding

Changes in v4:
-Change clocks in simplefb from a linked-list to a (simpler) dynamically
 allocated array

v2 got some objections from some people because clocks are hardware description
and as such do not belong in a virtual device node as simplefb is. As explained
in the following thread the use of clocks in simplefb has nothing to do with   
clock topology (so hardware description), but is about which clocks of a
number of possible clocks where actually chosen by the firmware to bring
of the fb, as such this clearly is state description.

AFAIK at the end of the thread no one was disagreeing with this anymore
(I did not get any replies to my last mails explaining this).

Some of us also got together at the Plumbers conference in Dusseldorf to
discuss this, present were: David Herrmann, Maxime Ripard, Geert Uytterhoeven
and Hans de Goede (me), conclusions:
-We will add a clocks property to the simplefb devicetree bindings, so
 that u-boot setup framebuffers passed to the kernel (for early console
 support) can properly list the clocks used, and simplefb can claim them to
 avoid them getting turned off, thereby breaking the early console
-simplefb/simpledrm -> native-kms driver handoff will keep using the
 linear framebuffer address to identify which simplefb platform device to
 destroy during handoff, so that if their are multiple kms devices
 involved, the handover happens at the right moment and we don't loose
 console output anywhere (and we should be able to do a flickerfree
 handover)

Since we seem to have broad agreement on how to move forward with this, I
would like to ask you to please merge this patch-set for 3.19.

I know 3.19 is still somewhat ar away, but I would like to submit the u-boot
side of this to upstream ASAP, so can you please let me know if you plan to 
take this patch-set for 3.19 soon ?

Regards,

Hans

[PATCH v4 1/5] simplefb: Add simplefb MAINTAINERS entry

[Hans de Goede]

During the discussion about adding clock handling code to simplefb, it became
clear that simplefb currently does not have an active maintainer.

I've discussed this with Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>, the original
author of simplefb, and with his permisson I'm picking up maintainership of
simplefb.

Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 73d1aef..6e92e73 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8416,6 +8416,14 @@ F:	drivers/media/usb/siano/
 F:	drivers/media/usb/siano/
 F:	drivers/media/mmc/siano/
 
+SIMPLEFB FB DRIVER
+M:	Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
+L:	linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/video/simple-framebuffer.txt
+F:	drivers/video/fbdev/simplefb.c
+F:	include/linux/platform_data/simplefb.h
+
 SH_VEU V4L2 MEM2MEM DRIVER
 L:	linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
 S:	Orphan
-- 
2.1.0

Re: [PATCH v4 1/5] simplefb: Add simplefb MAINTAINERS entry

[Grant Likely]

On Wed, 22 Oct 2014 18:45:07 +0200
, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
 wrote:
> During the discussion about adding clock handling code to simplefb, it became
> clear that simplefb currently does not have an active maintainer.
> 
> I've discussed this with Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>, the original
> author of simplefb, and with his permisson I'm picking up maintainership of
> simplefb.
> 
> Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Reviewed-by: David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> ---
>  MAINTAINERS | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 73d1aef..6e92e73 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8416,6 +8416,14 @@ F:	drivers/media/usb/siano/
>  F:	drivers/media/usb/siano/
>  F:	drivers/media/mmc/siano/
>  
> +SIMPLEFB FB DRIVER
> +M:	Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> +L:	linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +F:	drivers/video/fbdev/simplefb.c
> +F:	include/linux/platform_data/simplefb.h
> +
>  SH_VEU V4L2 MEM2MEM DRIVER
>  L:	linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>  S:	Orphan
> -- 
> 2.1.0
> 

[PATCH v4 2/5] dt-bindings: Add a clocks property to the simple-framebuffer binding

[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 <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

--
Changes in v2:
-Added Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Changes in v3:
-Updated description to make clear simplefb deals with more then just memory
---
 Documentation/devicetree/bindings/video/simple-framebuffer.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 70c26f3..172ad5f 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -1,8 +1,8 @@
 Simple Framebuffer
 
-A simple frame-buffer describes a raw memory region that may be rendered to,
-with the assumption that the display hardware has already been set up to scan
-out from that buffer.
+A simple frame-buffer describes a frame-buffer setup by firmware or
+the bootloader, with the assumption that the display hardware has already
+been set up to scan out from the memory pointed to by the reg property.
 
 Required properties:
 - compatible: "simple-framebuffer"
@@ -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).
 
+Optional properties:
+- clocks : List of clocks used by the framebuffer
+
 Example:
 
 	framebuffer {
-- 
2.1.0

Re: [PATCH v4 2/5] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Grant Likely]

On Wed, 22 Oct 2014 18:45:08 +0200
, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
 wrote:
> 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 <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 
> --
> Changes in v2:
> -Added Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Changes in v3:
> -Updated description to make clear simplefb deals with more then just memory
> ---
>  Documentation/devicetree/bindings/video/simple-framebuffer.txt | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 70c26f3..172ad5f 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -1,8 +1,8 @@
>  Simple Framebuffer
>  
> -A simple frame-buffer describes a raw memory region that may be rendered to,
> -with the assumption that the display hardware has already been set up to scan
> -out from that buffer.
> +A simple frame-buffer describes a frame-buffer setup by firmware or
> +the bootloader, with the assumption that the display hardware has already
> +been set up to scan out from the memory pointed to by the reg property.
>  
>  Required properties:
>  - compatible: "simple-framebuffer"
> @@ -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).
>  
> +Optional properties:
> +- clocks : List of clocks used by the framebuffer

Please change to the following:
- clocks : List of clocks used by the framebuffer. Clocks listed here
           are expected to already be configured correctly. The OS must
           ensure these clocks are not modified or disabled while the
           simple framebuffer remains active.

g.

Re: [PATCH v4 2/5] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Grant Likely]

[Replying on-list and with original ccs]

On Wed, Nov 12, 2014 at 2:50 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Hi,
>
> On 11/11/2014 10:42 PM, Grant Likely wrote:
>> - clocks : List of clocks used by the framebuffer. Clocks listed here
>>            are expected to already be configured correctly. The OS must
>>            ensure these clocks are not modified or disabled while the
>>            simple framebuffer remains active.
>
> Updated in my local tree, where I've also added proper commit messages
> to the 2 patches missing them. Shall I send a v5 of the set, and may I
> add your Acked-by to this patch when sending v5?

For this whole series, please add my Acked-by.

Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

I'm okay with the bindings, and I'm okay with adding the clock
dependency to a simplefb node. I'm happy with the conversation we had
today on IRC, and I'm expecting to see a binding that covers expected
firmware/OS behaviour and association with the real hardware posted to
the list shortly.

g.

Re: [PATCH v4 2/5] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Hans de Goede]

Hi,

On 11/12/2014 03:56 PM, Grant Likely wrote:
> [Replying on-list and with original ccs]
> 
> On Wed, Nov 12, 2014 at 2:50 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi,
>>
>> On 11/11/2014 10:42 PM, Grant Likely wrote:
>>> - clocks : List of clocks used by the framebuffer. Clocks listed here
>>>            are expected to already be configured correctly. The OS must
>>>            ensure these clocks are not modified or disabled while the
>>>            simple framebuffer remains active.
>>
>> Updated in my local tree, where I've also added proper commit messages
>> to the 2 patches missing them. Shall I send a v5 of the set, and may I
>> add your Acked-by to this patch when sending v5?
> 
> For this whole series, please add my Acked-by.
> 
> Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Thanks, given the above update to the bindings text, and also some updated
commit messages, I'll post a v5 with all these added soon.

Regards,

Hans

Re: [PATCH v4 2/5] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Grant Likely]

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

Rob, seeing as we disagree on patch #5, do you have time to discuss this
evening, or tomorrow?

g.
On 12 Nov 2014 14:56, "Grant Likely" <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> [Replying on-list and with original ccs]
>
> On Wed, Nov 12, 2014 at 2:50 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> wrote:
> > Hi,
> >
> > On 11/11/2014 10:42 PM, Grant Likely wrote:
> >> - clocks : List of clocks used by the framebuffer. Clocks listed here
> >>            are expected to already be configured correctly. The OS must
> >>            ensure these clocks are not modified or disabled while the
> >>            simple framebuffer remains active.
> >
> > Updated in my local tree, where I've also added proper commit messages
> > to the 2 patches missing them. Shall I send a v5 of the set, and may I
> > add your Acked-by to this patch when sending v5?
>
> For this whole series, please add my Acked-by.
>
> Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> I'm okay with the bindings, and I'm okay with adding the clock
> dependency to a simplefb node. I'm happy with the conversation we had
> today on IRC, and I'm expecting to see a binding that covers expected
> firmware/OS behaviour and association with the real hardware posted to
> the list shortly.
>
> g.
>

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: Type: text/html, Size: 2448 bytes --]

Re: [PATCH v4 2/5] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Rob Herring]

On Wed, Nov 12, 2014 at 10:38 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Rob, seeing as we disagree on patch #5, do you have time to discuss this
> evening, or tomorrow?

Yes, either is fine.

Rob

>
> g.
>
> On 12 Nov 2014 14:56, "Grant Likely" <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>> [Replying on-list and with original ccs]
>>
>> On Wed, Nov 12, 2014 at 2:50 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> wrote:
>> > Hi,
>> >
>> > On 11/11/2014 10:42 PM, Grant Likely wrote:
>> >> - clocks : List of clocks used by the framebuffer. Clocks listed here
>> >>            are expected to already be configured correctly. The OS must
>> >>            ensure these clocks are not modified or disabled while the
>> >>            simple framebuffer remains active.
>> >
>> > Updated in my local tree, where I've also added proper commit messages
>> > to the 2 patches missing them. Shall I send a v5 of the set, and may I
>> > add your Acked-by to this patch when sending v5?
>>
>> For this whole series, please add my Acked-by.
>>
>> Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>> I'm okay with the bindings, and I'm okay with adding the clock
>> dependency to a simplefb node. I'm happy with the conversation we had
>> today on IRC, and I'm expecting to see a binding that covers expected
>> firmware/OS behaviour and association with the real hardware posted to
>> the list shortly.
>>
>> g.

[PATCH v4 3/5] simplefb: formalize pseudo palette handling

[Hans de Goede]

From: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>

Signed-off-by: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org: drop unnecessary void * cast]
Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/video/fbdev/simplefb.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 210f3a0..ec112c1 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 = 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,
-- 
2.1.0

Re: [PATCH v4 3/5] simplefb: formalize pseudo palette handling

[Grant Likely]

On Wed, 22 Oct 2014 18:45:09 +0200
, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
 wrote:
> From: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
> 

Patch description? Write a proper changelog before merging please.
Describe why the change is necessary and what was done. You need to give
future readers some information about this patch. Heck, you're listed in
MAINTAINERS, you should be *enforcing* proper changelogs.

The code looks fine though.

Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> Signed-off-by: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
> Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> [hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org: drop unnecessary void * cast]
> Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Reviewed-by: David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/video/fbdev/simplefb.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 210f3a0..ec112c1 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 = 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,
> -- 
> 2.1.0
> 

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

[Hans de Goede]

From: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>

While at it update ioremap_wc error return from ENODEV to ENOMEM.

Signed-off-by: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/video/fbdev/simplefb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index ec112c1..cdcf1fe 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 = -ENOMEM;
+		goto error_fb_release;
 	}
 	info->pseudo_palette = par->palette;
 
@@ -247,14 +247,18 @@ 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)
-- 
2.1.0

[PATCH v4 5/5] simplefb: add clock handling code

[Hans de Goede]

From: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>

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-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
[hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org: Change clks from list to dynamic array]
Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

--
Changes in v4:
-change clks from linkedlist to dynamic allocated array
-propagate EPROBE_DEFER up from simplefb_clocks_init to simplefb_probe
---
 drivers/video/fbdev/simplefb.c | 101 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index cdcf1fe..cd96edd 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",
@@ -167,8 +168,98 @@ static int simplefb_parse_pd(struct platform_device *pdev,
 
 struct simplefb_par {
 	u32 palette[PSEUDO_PALETTE_SIZE];
+	int clk_count;
+	struct clk **clks;
 };
 
+/*
+ * 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.
+ *
+ * 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 int
+simplefb_clocks_init(struct simplefb_par *par, struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct clk *clock;
+	int i, ret;
+
+	if (dev_get_platdata(&pdev->dev) || !np)
+		return 0;
+
+	par->clk_count = of_clk_get_parent_count(np);
+	if (par->clk_count <= 0)
+		return 0;
+
+	par->clks = kcalloc(par->clk_count, sizeof(struct clk *), GFP_KERNEL);
+	if (!par->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < par->clk_count; i++) {
+		clock = of_clk_get(np, i);
+		if (IS_ERR(clock)) {
+			if (PTR_ERR(clock) == -EPROBE_DEFER) {
+				while (--i >= 0) {
+					if (par->clks[i])
+						clk_put(par->clks[i]);
+				}
+				kfree(par->clks);
+				return -EPROBE_DEFER;
+			}
+			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
+				__func__, i, PTR_ERR(clock));
+			continue;
+		}
+		par->clks[i] = clock;
+	}
+
+	for (i = 0; i < par->clk_count; i++) {
+		if (par->clks[i]) {
+			ret = clk_prepare_enable(par->clks[i]);
+			if (ret) {
+				dev_err(&pdev->dev,
+					"%s: failed to enable clock %d: %d\n",
+					__func__, i, ret);
+				clk_put(par->clks[i]);
+				par->clks[i] = NULL;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static void
+simplefb_clocks_destroy(struct simplefb_par *par)
+{
+	int i;
+
+	if (!par->clks)
+		return;
+
+	for (i = 0; i < par->clk_count; i++) {
+		if (par->clks[i]) {
+			clk_disable_unprepare(par->clks[i]);
+			clk_put(par->clks[i]);
+		}
+	}
+
+	kfree(par->clks);
+}
+
 static int simplefb_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -236,6 +327,10 @@ static int simplefb_probe(struct platform_device *pdev)
 	}
 	info->pseudo_palette = par->palette;
 
+	ret = simplefb_clocks_init(par, pdev);
+	if (ret < 0)
+		goto error_unmap;
+
 	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);
@@ -247,13 +342,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_clocks:
+	simplefb_clocks_destroy(par);
 error_unmap:
 	iounmap(info->screen_base);
 error_fb_release:
@@ -264,8 +361,10 @@ error_fb_release:
 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);
 	framebuffer_release(info);
 
 	return 0;
-- 
2.1.0

Re: [PATCH v4 0/5] simplefb: add clock handling code

[Tomi Valkeinen]

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

On 22/10/14 19:45, Hans de Goede wrote:

> Since we seem to have broad agreement on how to move forward with this, I
> would like to ask you to please merge this patch-set for 3.19.
> 
> I know 3.19 is still somewhat ar away, but I would like to submit the u-boot
> side of this to upstream ASAP, so can you please let me know if you plan to 
> take this patch-set for 3.19 soon ?

This series looks fine to me, except patch 3 is missing a description,
and patch 4 makes the desc a continuation of the subject. Each patch
should have a description (independent from subject).

No need to resend, I can cook up something for those, or I can edit the
descs if you provide the text.

So if there are no strong objections, I'll queue this for 3.19.

 Tomi



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

Re: [PATCH v4 0/5] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 10/28/2014 11:44 AM, Tomi Valkeinen wrote:
> On 22/10/14 19:45, Hans de Goede wrote:
> 
>> Since we seem to have broad agreement on how to move forward with this, I
>> would like to ask you to please merge this patch-set for 3.19.
>>
>> I know 3.19 is still somewhat ar away, but I would like to submit the u-boot
>> side of this to upstream ASAP, so can you please let me know if you plan to 
>> take this patch-set for 3.19 soon ?
> 
> This series looks fine to me, except patch 3 is missing a description,

Description for patch 3:

"Add a proper struct describing simplefb private data, with the palette in there,
instead of directly storing the palette in the fb_info->par pointer."

> and patch 4 makes the desc a continuation of the subject.

Description for patch 4:

"Use the usual kernel style of goto error_foo to free resources on probe
error. This is a preparation patch for adding clocks support.

While at it update ioremap_wc error return from ENODEV to ENOMEM."

> Each patch should have a description (independent from subject).

Ok, I'll remember that for future patches.

> No need to resend, I can cook up something for those, or I can edit the
> descs if you provide the text.

Descriptions provided above, thanks!

> So if there are no strong objections, I'll queue this for 3.19.

Regards,

Hans

Re: [PATCH v4 0/5] simplefb: add clock handling code

[Rob Herring]

On Tue, Oct 28, 2014 at 6:44 PM, Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:
> On 22/10/14 19:45, Hans de Goede wrote:
>
>> Since we seem to have broad agreement on how to move forward with this, I
>> would like to ask you to please merge this patch-set for 3.19.
>>
>> I know 3.19 is still somewhat ar away, but I would like to submit the u-boot
>> side of this to upstream ASAP, so can you please let me know if you plan to
>> take this patch-set for 3.19 soon ?
>
> This series looks fine to me, except patch 3 is missing a description,
> and patch 4 makes the desc a continuation of the subject. Each patch
> should have a description (independent from subject).
>
> No need to resend, I can cook up something for those, or I can edit the
> descs if you provide the text.
>
> So if there are no strong objections, I'll queue this for 3.19.

Yes, I object to the binding still as it has not changed from what was
previously posted.

Rob

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 10/28/2014 12:11 PM, Rob Herring wrote:
> On Tue, Oct 28, 2014 at 6:44 PM, Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:
>> On 22/10/14 19:45, Hans de Goede wrote:
>>
>>> Since we seem to have broad agreement on how to move forward with this, I
>>> would like to ask you to please merge this patch-set for 3.19.
>>>
>>> I know 3.19 is still somewhat ar away, but I would like to submit the u-boot
>>> side of this to upstream ASAP, so can you please let me know if you plan to
>>> take this patch-set for 3.19 soon ?
>>
>> This series looks fine to me, except patch 3 is missing a description,
>> and patch 4 makes the desc a continuation of the subject. Each patch
>> should have a description (independent from subject).
>>
>> No need to resend, I can cook up something for those, or I can edit the
>> descs if you provide the text.
>>
>> So if there are no strong objections, I'll queue this for 3.19.
> 
> Yes, I object to the binding still as it has not changed from what was
> previously posted.

It would be helpful if you could explain why you object. Last time you
said: " You are mixing in a hardware description that is simply inaccurate."

I then explained that this is not hardware description, but runtime state
information, as it tells the kernel which clocks were chosen to drive the
display (out of typically a list of possible options, depending on which
output is used, etc.). Just like which memory address the bootloader has
chosen to scan out the video image from.

Then you got quiet, so sorry, but this time your objection really is too
late. You cannot simply go quiet halfway through a discussion and then pop
up again when a new version is posted to say "I object" yet another time,
you've had your chance to make your arguments last time, and chose to stay
quiet after I explained in detail that this is not hardware description but
state information, so now it is simply too late.

These bindings have been discussed at Plumbers with various interested people
present, and the conclusion was that this really is the best way to handle this,
so this patch is:

    Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
    Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
    Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

And David Herrman who is working on simpledrm, which will be merged soon, which
will also use the simplefb bindings also agrees. So we have the simplefb maintainer,
simpledrm maintainer, and the clk subsystem maintainer + 2 other maintainers all
agreeing on a way forward, the time for bikeshedding now really really really is
over.

Tomi, can you please let us know how you plan to proceed with this ?

Regards,

Hans

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Tomi Valkeinen]

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

Hi Hans, Rob,

On 28/10/14 13:30, Hans de Goede wrote:
> Hi,
> 
> On 10/28/2014 12:11 PM, Rob Herring wrote:

>> Yes, I object to the binding still as it has not changed from what was
>> previously posted.
> 
> It would be helpful if you could explain why you object. Last time you
> said: " You are mixing in a hardware description that is simply inaccurate."
> 
> I then explained that this is not hardware description, but runtime state
> information, as it tells the kernel which clocks were chosen to drive the
> display (out of typically a list of possible options, depending on which
> output is used, etc.). Just like which memory address the bootloader has
> chosen to scan out the video image from.
> 
> Then you got quiet, so sorry, but this time your objection really is too
> late. You cannot simply go quiet halfway through a discussion and then pop
> up again when a new version is posted to say "I object" yet another time,
> you've had your chance to make your arguments last time, and chose to stay
> quiet after I explained in detail that this is not hardware description but
> state information, so now it is simply too late.
> 
> These bindings have been discussed at Plumbers with various interested people
> present, and the conclusion was that this really is the best way to handle this,
> so this patch is:
> 
>     Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>     Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
>     Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 
> And David Herrman who is working on simpledrm, which will be merged soon, which
> will also use the simplefb bindings also agrees. So we have the simplefb maintainer,
> simpledrm maintainer, and the clk subsystem maintainer + 2 other maintainers all
> agreeing on a way forward, the time for bikeshedding now really really really is
> over.
> 
> Tomi, can you please let us know how you plan to proceed with this ?

I won't merge DT bindings via fbdev tree, if a DT maintainer says no.

I took Rob's silence to the earlier series as a silent ack for your
explanation. Obviously that was not the case.

Rob, please advice asap what should be done to the bindings to get your
ack. As Hans explained above, this discussion has been going on for a
long time, and afaik this series is the best way forward of all the
options discussed.

 Tomi



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

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Rob Herring]

On Wed, Oct 29, 2014 at 7:08 PM, Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:
> Hi Hans, Rob,
>
> On 28/10/14 13:30, Hans de Goede wrote:
>> Hi,
>>
>> On 10/28/2014 12:11 PM, Rob Herring wrote:
>
>>> Yes, I object to the binding still as it has not changed from what was
>>> previously posted.
>>
>> It would be helpful if you could explain why you object. Last time you
>> said: " You are mixing in a hardware description that is simply inaccurate."
>>
>> I then explained that this is not hardware description, but runtime state
>> information, as it tells the kernel which clocks were chosen to drive the
>> display (out of typically a list of possible options, depending on which
>> output is used, etc.). Just like which memory address the bootloader has
>> chosen to scan out the video image from.
>>
>> Then you got quiet, so sorry, but this time your objection really is too
>> late. You cannot simply go quiet halfway through a discussion and then pop
>> up again when a new version is posted to say "I object" yet another time,
>> you've had your chance to make your arguments last time, and chose to stay
>> quiet after I explained in detail that this is not hardware description but
>> state information, so now it is simply too late.
>>
>> These bindings have been discussed at Plumbers with various interested people
>> present, and the conclusion was that this really is the best way to handle this,
>> so this patch is:
>>
>>     Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>     Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>     Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
>>     Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>
>> And David Herrman who is working on simpledrm, which will be merged soon, which
>> will also use the simplefb bindings also agrees. So we have the simplefb maintainer,
>> simpledrm maintainer, and the clk subsystem maintainer + 2 other maintainers all
>> agreeing on a way forward, the time for bikeshedding now really really really is
>> over.
>>
>> Tomi, can you please let us know how you plan to proceed with this ?
>
> I won't merge DT bindings via fbdev tree, if a DT maintainer says no.
>
> I took Rob's silence to the earlier series as a silent ack for your
> explanation. Obviously that was not the case.
>
> Rob, please advice asap what should be done to the bindings to get your
> ack. As Hans explained above, this discussion has been going on for a
> long time, and afaik this series is the best way forward of all the
> options discussed.

I still think for the most part this is a kernel problem. It is a
kernel policy to turn off unused clocks. The clock framework could
just as easily decide that any clocks enabled at boot and left
un-managed (i.e. w/o a driver) are kept on until they are managed. I'm
not saying this can't be in DT, only that DT is not the only solution
here. This problem is not unique to simplefb. A serial console could
stop working if no serial driver is loaded before unused clocks are
disabled. CPU core clocks have a similar issue as well (often enabled
in platform code). I want to see this solved in a generic way for any
clock.

As regulators were also mentioned, they already have a
"regulator-boot-on" property defined. Perhaps this is suitable to be
mirrored for clocks. If it is not, then I'm wondering why we have it.
A key difference here is that the property is part of the provider and
can be dealt with in the clock driver rather than requiring a
temporary driver.

Rob

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 10/31/2014 09:47 PM, Rob Herring wrote:
> On Wed, Oct 29, 2014 at 7:08 PM, Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:
>> Hi Hans, Rob,
>>
>> On 28/10/14 13:30, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 10/28/2014 12:11 PM, Rob Herring wrote:
>>
>>>> Yes, I object to the binding still as it has not changed from what was
>>>> previously posted.
>>>
>>> It would be helpful if you could explain why you object. Last time you
>>> said: " You are mixing in a hardware description that is simply inaccurate."
>>>
>>> I then explained that this is not hardware description, but runtime state
>>> information, as it tells the kernel which clocks were chosen to drive the
>>> display (out of typically a list of possible options, depending on which
>>> output is used, etc.). Just like which memory address the bootloader has
>>> chosen to scan out the video image from.
>>>
>>> Then you got quiet, so sorry, but this time your objection really is too
>>> late. You cannot simply go quiet halfway through a discussion and then pop
>>> up again when a new version is posted to say "I object" yet another time,
>>> you've had your chance to make your arguments last time, and chose to stay
>>> quiet after I explained in detail that this is not hardware description but
>>> state information, so now it is simply too late.
>>>
>>> These bindings have been discussed at Plumbers with various interested people
>>> present, and the conclusion was that this really is the best way to handle this,
>>> so this patch is:
>>>
>>>     Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>     Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>     Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
>>>     Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>>
>>> And David Herrman who is working on simpledrm, which will be merged soon, which
>>> will also use the simplefb bindings also agrees. So we have the simplefb maintainer,
>>> simpledrm maintainer, and the clk subsystem maintainer + 2 other maintainers all
>>> agreeing on a way forward, the time for bikeshedding now really really really is
>>> over.
>>>
>>> Tomi, can you please let us know how you plan to proceed with this ?
>>
>> I won't merge DT bindings via fbdev tree, if a DT maintainer says no.
>>
>> I took Rob's silence to the earlier series as a silent ack for your
>> explanation. Obviously that was not the case.
>>
>> Rob, please advice asap what should be done to the bindings to get your
>> ack. As Hans explained above, this discussion has been going on for a
>> long time, and afaik this series is the best way forward of all the
>> options discussed.
> 
> I still think for the most part this is a kernel problem. It is a
> kernel policy to turn off unused clocks. The clock framework could
> just as easily decide that any clocks enabled at boot and left
> un-managed (i.e. w/o a driver) are kept on until they are managed.

This is not just about being unmanaged, clocks can be shared, and another
driver can actively claim the clock and turn it off, we need to protect
against this too.

> I'm not saying this can't be in DT, only that DT is not the only solution
> here.

Basically the problem is that we must keep the clock enabled for the lifetime
of the simplefb device. We've been over this in 2 very long threads already,
and many alternatives have been evaluated and found wanting.

The only 100% safe way to ensure clocks are not turned off is to tie there
lifetime to that of the simplefb device, and the only reliable way to do
that is through a clocks property.

It really is that simple, and I don't understand why people keep insisting
there must be another way, while they fail to offer a (reliable, working)
other way.

> This problem is not unique to simplefb. A serial console could
> stop working if no serial driver is loaded before unused clocks are
> disabled. CPU core clocks have a similar issue as well (often enabled
> in platform code). I want to see this solved in a generic way for any
> clock.
> 
> As regulators were also mentioned, they already have a
> "regulator-boot-on" property defined. Perhaps this is suitable to be
> mirrored for clocks. If it is not, then I'm wondering why we have it.
> A key difference here is that the property is part of the provider and
> can be dealt with in the clock driver rather than requiring a
> temporary driver.

"regulator-boot-on" tells the regulator framework to enable a regulator
at boot, but it does not protect it from getting turned of by another
driver later (it does not increase use_count).

So this is the same none solution as not turning off unmanaged clocks,
what if another driver who shares the clock, enables it during probe,
then disables it while done probing (to put things in low power state
until userspace uses the device).

This is not about not turning the clock off, this is about keeping
the clock on for the lifetime of the simplefb device, which is a different
problem, cause we must not remove a single calling point of clock_off,
put we must block all attempts to turn the clock off, which is done
by increasing its use-count, which is done by claiming + enabling it,
which needs a phandle to the clock ...

Regards,

Hans

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[jonsmirl-Re5JQEeQqe8AvxtiuMwx3w]

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

On Fri, Oct 31, 2014 at 4:47 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Oct 29, 2014 at 7:08 PM, Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
> wrote:
> > Hi Hans, Rob,
> >
> > On 28/10/14 13:30, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 10/28/2014 12:11 PM, Rob Herring wrote:
> >
> >>> Yes, I object to the binding still as it has not changed from what was
> >>> previously posted.
> >>
> >> It would be helpful if you could explain why you object. Last time you
> >> said: " You are mixing in a hardware description that is simply
> inaccurate."
> >>
> >> I then explained that this is not hardware description, but runtime
> state
> >> information, as it tells the kernel which clocks were chosen to drive
> the
> >> display (out of typically a list of possible options, depending on which
> >> output is used, etc.). Just like which memory address the bootloader has
> >> chosen to scan out the video image from.
> >>
> >> Then you got quiet, so sorry, but this time your objection really is too
> >> late. You cannot simply go quiet halfway through a discussion and then
> pop
> >> up again when a new version is posted to say "I object" yet another
> time,
> >> you've had your chance to make your arguments last time, and chose to
> stay
> >> quiet after I explained in detail that this is not hardware description
> but
> >> state information, so now it is simply too late.
> >>
> >> These bindings have been discussed at Plumbers with various interested
> people
> >> present, and the conclusion was that this really is the best way to
> handle this,
> >> so this patch is:
> >>
> >>     Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>     Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>     Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> >>     Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> >>
> >> And David Herrman who is working on simpledrm, which will be merged
> soon, which
> >> will also use the simplefb bindings also agrees. So we have the
> simplefb maintainer,
> >> simpledrm maintainer, and the clk subsystem maintainer + 2 other
> maintainers all
> >> agreeing on a way forward, the time for bikeshedding now really really
> really is
> >> over.
> >>
> >> Tomi, can you please let us know how you plan to proceed with this ?
> >
> > I won't merge DT bindings via fbdev tree, if a DT maintainer says no.
> >
> > I took Rob's silence to the earlier series as a silent ack for your
> > explanation. Obviously that was not the case.
> >
> > Rob, please advice asap what should be done to the bindings to get your
> > ack. As Hans explained above, this discussion has been going on for a
> > long time, and afaik this series is the best way forward of all the
> > options discussed.
>
> I still think for the most part this is a kernel problem. It is a
> kernel policy to turn off unused clocks. The clock framework could
> just as easily decide that any clocks enabled at boot and left
> un-managed (i.e. w/o a driver) are kept on until they are managed. I'm
> not saying this can't be in DT, only that DT is not the only solution
> here. This problem is not unique to simplefb. A serial console could
> stop working if no serial driver is loaded before unused clocks are
> disabled. CPU core clocks have a similar issue as well (often enabled
> in platform code). I want to see this solved in a generic way for any
> clock.
>

I am in agreement this point of view. This is a problem in the Linux
kernel. It is not a generic problem.

The Linux problem is that during boot device drivers are loaded in two
phases - built-in and loadable modules. The clock-regulator clean up is
happening too early. It happens after the built-in drivers load and before
the loadable modules have a chance to load. That's simply the wrong place
for that clean up happen.  A simple alternative would be to make a tiny
loadable module that triggers the clean up. Then adjust your boot scripts
to load this module after the other ones have loaded. But instead of fixing
this simple timing flaw in the Linux boot process a complex mechanism is
being created to work around it.

Simplefb is also being developed as a way of protecting the BIOS setup of
the framebuffer past the boot process and out into use as a normal user
space console.  I in no way support this use. We have experienced decades
of problems on the x86 with VGA and BIOSes that I do not wish to repeat in
the ARM world. Write a correct, device specific framebuffer driver for this
piece of hardware. That device specific driver will load before the
clock/regulator cleanup and claim all of the correct resources.



>
> As regulators were also mentioned, they already have a
> "regulator-boot-on" property defined. Perhaps this is suitable to be
> mirrored for clocks. If it is not, then I'm wondering why we have it.
> A key difference here is that the property is part of the provider and
> can be dealt with in the clock driver rather than requiring a
> temporary driver.
>
> Rob
>
> --
> 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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit https://groups.google.com/d/optout.
>



-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: Type: text/html, Size: 7968 bytes --]

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[jonsmirl-Re5JQEeQqe8AvxtiuMwx3w]

On Fri, Oct 31, 2014 at 4:47 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Wed, Oct 29, 2014 at 7:08 PM, Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:
> > Hi Hans, Rob,
> >
> > On 28/10/14 13:30, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 10/28/2014 12:11 PM, Rob Herring wrote:
> >
> >>> Yes, I object to the binding still as it has not changed from what was
> >>> previously posted.
> >>
> >> It would be helpful if you could explain why you object. Last time you
> >> said: " You are mixing in a hardware description that is simply inaccurate."
> >>
> >> I then explained that this is not hardware description, but runtime state
> >> information, as it tells the kernel which clocks were chosen to drive the
> >> display (out of typically a list of possible options, depending on which
> >> output is used, etc.). Just like which memory address the bootloader has
> >> chosen to scan out the video image from.
> >>
> >> Then you got quiet, so sorry, but this time your objection really is too
> >> late. You cannot simply go quiet halfway through a discussion and then pop
> >> up again when a new version is posted to say "I object" yet another time,
> >> you've had your chance to make your arguments last time, and chose to stay
> >> quiet after I explained in detail that this is not hardware description but
> >> state information, so now it is simply too late.
> >>
> >> These bindings have been discussed at Plumbers with various interested people
> >> present, and the conclusion was that this really is the best way to handle this,
> >> so this patch is:
> >>
> >>     Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>     Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>     Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> >>     Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> >>
> >> And David Herrman who is working on simpledrm, which will be merged soon, which
> >> will also use the simplefb bindings also agrees. So we have the simplefb maintainer,
> >> simpledrm maintainer, and the clk subsystem maintainer + 2 other maintainers all
> >> agreeing on a way forward, the time for bikeshedding now really really really is
> >> over.
> >>
> >> Tomi, can you please let us know how you plan to proceed with this ?
> >
> > I won't merge DT bindings via fbdev tree, if a DT maintainer says no.
> >
> > I took Rob's silence to the earlier series as a silent ack for your
> > explanation. Obviously that was not the case.
> >
> > Rob, please advice asap what should be done to the bindings to get your
> > ack. As Hans explained above, this discussion has been going on for a
> > long time, and afaik this series is the best way forward of all the
> > options discussed.
>
> I still think for the most part this is a kernel problem. It is a
> kernel policy to turn off unused clocks. The clock framework could
> just as easily decide that any clocks enabled at boot and left
> un-managed (i.e. w/o a driver) are kept on until they are managed. I'm
> not saying this can't be in DT, only that DT is not the only solution
> here. This problem is not unique to simplefb. A serial console could
> stop working if no serial driver is loaded before unused clocks are
> disabled. CPU core clocks have a similar issue as well (often enabled
> in platform code). I want to see this solved in a generic way for any
> clock.


I am in agreement this point of view. This is a problem in the Linux
kernel. It is not a generic problem.

The Linux problem is that during boot device drivers are loaded in two
phases - built-in and loadable modules. The clock-regulator clean up
is happening too early. It happens after the built-in drivers load and
before the loadable modules have a chance to load. That's simply the
wrong place for that clean up happen.  A simple alternative would be
to make a tiny loadable module that triggers the clean up. Then adjust
your boot scripts to load this module after the other ones have
loaded. But instead of fixing this simple timing flaw in the Linux
boot process a complex mechanism is being created to work around it.

Simplefb is also being developed as a way of protecting the BIOS setup
of the framebuffer past the boot process and out into use as a normal
user space console.  I in no way support this use. We have experienced
decades of problems on the x86 with VGA and BIOSes that I do not wish
to repeat in the ARM world. Write a correct, device specific
framebuffer driver for this piece of hardware. That device specific
driver will load before the clock/regulator cleanup and claim all of
the correct resources.

>
>
> As regulators were also mentioned, they already have a
> "regulator-boot-on" property defined. Perhaps this is suitable to be
> mirrored for clocks. If it is not, then I'm wondering why we have it.
> A key difference here is that the property is part of the provider and
> can be dealt with in the clock driver rather than requiring a
> temporary driver.
>
> Rob
>
> --
> 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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit https://groups.google.com/d/optout.




-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Neal Peacock]

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

On Nov 2, 2014 7:28 AM, "jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org" <jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Fri, Oct 31, 2014 at 4:47 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:
> >
> > On Wed, Oct 29, 2014 at 7:08 PM, Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
wrote:
> > > Hi Hans, Rob,
> > >
> > > On 28/10/14 13:30, Hans de Goede wrote:
> > >> Hi,
> > >>
> > >> On 10/28/2014 12:11 PM, Rob Herring wrote:
> > >
> > >>> Yes, I object to the binding still as it has not changed from what
was
> > >>> previously posted.
> > >>
> > >> It would be helpful if you could explain why you object. Last time
you
> > >> said: " You are mixing in a hardware description that is simply
inaccurate."
> > >>
> > >> I then explained that this is not hardware description, but runtime
state
> > >> information, as it tells the kernel which clocks were chosen to
drive the
> > >> display (out of typically a list of possible options, depending on
which
> > >> output is used, etc.). Just like which memory address the bootloader
has
> > >> chosen to scan out the video image from.
> > >>
> > >> Then you got quiet, so sorry, but this time your objection really is
too
> > >> late. You cannot simply go quiet halfway through a discussion and
then pop
> > >> up again when a new version is posted to say "I object" yet another
time,
> > >> you've had your chance to make your arguments last time, and chose
to stay
> > >> quiet after I explained in detail that this is not hardware
description but
> > >> state information, so now it is simply too late.
> > >>
> > >> These bindings have been discussed at Plumbers with various
interested people
> > >> present, and the conclusion was that this really is the best way to
handle this,
> > >> so this patch is:
> > >>
> > >>     Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >>     Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > >>     Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> > >>     Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > >>
> > >> And David Herrman who is working on simpledrm, which will be merged
soon, which
> > >> will also use the simplefb bindings also agrees. So we have the
simplefb maintainer,
> > >> simpledrm maintainer, and the clk subsystem maintainer + 2 other
maintainers all
> > >> agreeing on a way forward, the time for bikeshedding now really
really really is
> > >> over.
> > >>
> > >> Tomi, can you please let us know how you plan to proceed with this ?
> > >
> > > I won't merge DT bindings via fbdev tree, if a DT maintainer says no.
> > >
> > > I took Rob's silence to the earlier series as a silent ack for your
> > > explanation. Obviously that was not the case.
> > >
> > > Rob, please advice asap what should be done to the bindings to get
your
> > > ack. As Hans explained above, this discussion has been going on for a
> > > long time, and afaik this series is the best way forward of all the
> > > options discussed.
> >
> > I still think for the most part this is a kernel problem. It is a
> > kernel policy to turn off unused clocks. The clock framework could
> > just as easily decide that any clocks enabled at boot and left
> > un-managed (i.e. w/o a driver) are kept on until they are managed. I'm
> > not saying this can't be in DT, only that DT is not the only solution
> > here. This problem is not unique to simplefb. A serial console could
> > stop working if no serial driver is loaded before unused clocks are
> > disabled. CPU core clocks have a similar issue as well (often enabled
> > in platform code). I want to see this solved in a generic way for any
> > clock.
>
>
> I am in agreement this point of view. This is a problem in the Linux
> kernel. It is not a generic problem.
>
> The Linux problem is that during boot device drivers are loaded in two
> phases - built-in and loadable modules. The clock-regulator clean up
> is happening too early. It happens after the built-in drivers load and
> before the loadable modules have a chance to load. That's simply the
> wrong place for that clean up happen.  A simple alternative would be
> to make a tiny loadable module that triggers the clean up. Then adjust
> your boot scripts to load this module after the other ones have
> loaded. But instead of fixing this simple timing flaw in the Linux
> boot process a complex mechanism is being created to work around it.
>
> Simplefb is also being developed as a way of protecting the BIOS setup
> of the framebuffer past the boot process and out into use as a normal
> user space console.  I in no way support this use. We have experienced
> decades of problems on the x86 with VGA and BIOSes that I do not wish
> to repeat in the ARM world. Write a correct, device specific
> framebuffer driver for this piece of hardware. That device specific
> driver will load before the clock/regulator cleanup and claim all of
> the correct resources.
>
> >
> >
> > As regulators were also mentioned, they already have a
> > "regulator-boot-on" property defined. Perhaps this is suitable to be
> > mirrored for clocks. If it is not, then I'm wondering why we have it.
> > A key difference here is that the property is part of the provider and
> > can be dealt with in the clock driver rather than requiring a
> > temporary driver.

What if the hardware specific 'real' driver was a new driver that is
basically a wrapper of the simplefb but adding the clock. This seems to be
the only sunxi specific addition.

This would stand in until the 'real' driver is created. As far as I can
tell nothing about this setup would be wrong, so even if the real driver
never came in it wouldn't be a problem.

Is this a workable alternate solution?

> >
> > Rob
> >
> > --
> > 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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> > For more options, visit https://groups.google.com/d/optout.
>
>
>
>
> --
> Jon Smirl
> jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
>
> --
> 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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit https://groups.google.com/d/optout.

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: Type: text/html, Size: 9607 bytes --]

Re: [PATCH v4 0/5] simplefb: add clock handling code

[Luc Verhaegen]

On Sun, Nov 02, 2014 at 10:28:48AM -0500, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>
> Simplefb is also being developed as a way of protecting the BIOS setup
> of the framebuffer past the boot process and out into use as a normal
> user space console.  I in no way support this use. We have experienced
> decades of problems on the x86 with VGA and BIOSes that I do not wish
> to repeat in the ARM world.

Oh wow.

Luc Verhaegen, aka, the guy who made BIOS-free display drivers possible.

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Grant Likely]

On Sat, 1 Nov 2014 04:47:56 +0800
, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
 wrote:
> On Wed, Oct 29, 2014 at 7:08 PM, Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:
> > Hi Hans, Rob,
> >
> > On 28/10/14 13:30, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 10/28/2014 12:11 PM, Rob Herring wrote:
> >
> >>> Yes, I object to the binding still as it has not changed from what was
> >>> previously posted.
> >>
> >> It would be helpful if you could explain why you object. Last time you
> >> said: " You are mixing in a hardware description that is simply inaccurate."
> >>
> >> I then explained that this is not hardware description, but runtime state
> >> information, as it tells the kernel which clocks were chosen to drive the
> >> display (out of typically a list of possible options, depending on which
> >> output is used, etc.). Just like which memory address the bootloader has
> >> chosen to scan out the video image from.
> >>
> >> Then you got quiet, so sorry, but this time your objection really is too
> >> late. You cannot simply go quiet halfway through a discussion and then pop
> >> up again when a new version is posted to say "I object" yet another time,
> >> you've had your chance to make your arguments last time, and chose to stay
> >> quiet after I explained in detail that this is not hardware description but
> >> state information, so now it is simply too late.
> >>
> >> These bindings have been discussed at Plumbers with various interested people
> >> present, and the conclusion was that this really is the best way to handle this,
> >> so this patch is:
> >>
> >>     Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>     Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>     Acked-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> >>     Reviewed-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> >>
> >> And David Herrman who is working on simpledrm, which will be merged soon, which
> >> will also use the simplefb bindings also agrees. So we have the simplefb maintainer,
> >> simpledrm maintainer, and the clk subsystem maintainer + 2 other maintainers all
> >> agreeing on a way forward, the time for bikeshedding now really really really is
> >> over.
> >>
> >> Tomi, can you please let us know how you plan to proceed with this ?
> >
> > I won't merge DT bindings via fbdev tree, if a DT maintainer says no.
> >
> > I took Rob's silence to the earlier series as a silent ack for your
> > explanation. Obviously that was not the case.
> >
> > Rob, please advice asap what should be done to the bindings to get your
> > ack. As Hans explained above, this discussion has been going on for a
> > long time, and afaik this series is the best way forward of all the
> > options discussed.
> 
> I still think for the most part this is a kernel problem. It is a
> kernel policy to turn off unused clocks. The clock framework could
> just as easily decide that any clocks enabled at boot and left
> un-managed (i.e. w/o a driver) are kept on until they are managed. I'm
> not saying this can't be in DT, only that DT is not the only solution
> here. This problem is not unique to simplefb. A serial console could
> stop working if no serial driver is loaded before unused clocks are
> disabled. CPU core clocks have a similar issue as well (often enabled
> in platform code). I want to see this solved in a generic way for any
> clock.

I've spent most of today rereading the entire thread and thinking about
this issue. Given that the clock binding is a well established core
binding at this point, it is entirely reasonable for a generic driver to
have part of the binding specify the clock resources that it depends on
to keep working. The binding needs to be very clear that it is a mere
dependency, and that the OS must not make any attempt to modify the
clock settings, but it is reasonable indicate to the OS that the clocks
are used by the firmware framebuffer, and turning them off will cause
the framebuffer will break. The best place to provide that guidance is
directly in the node.

If we were talking about a resource that didn't have a well established
binding, ie, the video path configuration, then I'd still say no way
because there is no way to do it generically. Claiming clocks on the
other hand is straight forward and simple.

Aside from wanting the documentation to be more explicit about what the
driver does with the clocks, I don't have a problem with doing it.

However, I am concerned about handover. I've lost track over the entire
thread on whether the handover mechanism has been resolved, and I would
really like to have a proposed solution to this documented in the
binding. The fact that there is nothing tying the simple framebuffer to
the actual hardware backing the framebuffer is concerning. It means the
kernel needs to guess which graphics device is associated with the
framebuffer.

The best solution in my opinion is to embed the simplefb properties
directly into the graphics device node (Yes, I know some have argued
that the graphics subsystem is composed of multiple devices; That
doesn't matter. Just choose one of the device nodes; the device that
actually DMAs the video buffer is probably the correct choice). That
makes it crystal clear which graphics controller owns the framebuffer.
There is precedence in DT bindings for putting current state into the
tree. offb.c is trying to solve the exact same problem. It also means
that we can use the driver model to provide mutual exclusion between
probing simplefb and a full-featured driver.

That approach would require some modifications of the binding to make
sure properties don't conflict with the device specific properties. The
easiest way is to allow the properties to live in a sub node so that the
reg property doesn't conflict.

However, I'm not stuck on this approach. The solution must be
unambiguious, and the handover mechanism must be thought through, but it
doesn't /have/ to be what I suggested above if a proposal that can be
well argued as better.

Hans, let's talk about this tomorrow. #devicetree on freenode? It looks like
I'm committed to volunteer at my daughters school tomorrow morning, but
the afternoon looks good.

g.

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Geert Uytterhoeven]

Hi Grant,

On Tue, Nov 11, 2014 at 10:49 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> However, I am concerned about handover. I've lost track over the entire
> thread on whether the handover mechanism has been resolved, and I would
> really like to have a proposed solution to this documented in the
> binding. The fact that there is nothing tying the simple framebuffer to
> the actual hardware backing the framebuffer is concerning. It means the
> kernel needs to guess which graphics device is associated with the
> framebuffer.

We did discuss handover in Düsseldorf, and concluded that the simplefb's
regs property can be used for this.

While on a modern system with unified memory this association cannot be
derived in a generic way, a device-specific driver for the graphics hardware
can if the regs property of the simplefb node matches the address the CRTC
engine is configured for.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v4 0/5] simplefb: add clock handling code

[Grant Likely]

On Wednesday, November 12, 2014, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>
> Hi Grant,
>
> On Tue, Nov 11, 2014 at 10:49 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > However, I am concerned about handover. I've lost track over the entire
> > thread on whether the handover mechanism has been resolved, and I would
> > really like to have a proposed solution to this documented in the
> > binding. The fact that there is nothing tying the simple framebuffer to
> > the actual hardware backing the framebuffer is concerning. It means the
> > kernel needs to guess which graphics device is associated with the
> > framebuffer.
>
> We did discuss handover in Düsseldorf, and concluded that the simplefb's
> regs property can be used for this.
>
> While on a modern system with unified memory this association cannot be
> derived in a generic way, a device-specific driver for the graphics hardware
> can if the regs property of the simplefb node matches the address the CRTC
> engine is configured for.


???

Right, I'm going to be blunt here: That's just dumb. All the
capability needed is there in the DT to associate a simple FB to a
display controller, and the solution chosen is to use a heuristic?

The association needs to be explicit. I strongly prefer putting the
simple FB directly into the display controller node, but I would
consider phandle linkage also.

g.

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v4 0/5] simplefb: add clock handling code

[Geert Uytterhoeven]

Hi Grant,

On Wed, Nov 12, 2014 at 10:57 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Wednesday, November 12, 2014, Geert Uytterhoeven
> <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> On Tue, Nov 11, 2014 at 10:49 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> > However, I am concerned about handover. I've lost track over the entire
>> > thread on whether the handover mechanism has been resolved, and I would
>> > really like to have a proposed solution to this documented in the
>> > binding. The fact that there is nothing tying the simple framebuffer to
>> > the actual hardware backing the framebuffer is concerning. It means the
>> > kernel needs to guess which graphics device is associated with the
>> > framebuffer.
>>
>> We did discuss handover in Düsseldorf, and concluded that the simplefb's
>> regs property can be used for this.
>>
>> While on a modern system with unified memory this association cannot be
>> derived in a generic way, a device-specific driver for the graphics hardware
>> can if the regs property of the simplefb node matches the address the CRTC
>> engine is configured for.
>
> ???
>
> Right, I'm going to be blunt here: That's just dumb. All the
> capability needed is there in the DT to associate a simple FB to a
> display controller, and the solution chosen is to use a heuristic?
>
> The association needs to be explicit. I strongly prefer putting the
> simple FB directly into the display controller node, but I would
> consider phandle linkage also.

IFF there's a display controller node, you can put it there.
I actually proposed to have a minimal/preliminary display controller node,
but people countered that for various reasons (too many components
with multiple nodes on many systems, bindings not yet defined, etc.).

And if there's no graphics driver/bindings yet at the time the bootloader
is written, it doesn't know how to link simplefb with it in DT.
Hence the heuristic to match regs... Does that make sense?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v4 0/5] simplefb: add clock handling code

[Grant Likely]

On Wed, Nov 12, 2014 at 10:08 AM, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> Hi Grant,
>
> On Wed, Nov 12, 2014 at 10:57 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Wednesday, November 12, 2014, Geert Uytterhoeven
>> <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>>> On Tue, Nov 11, 2014 at 10:49 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> > However, I am concerned about handover. I've lost track over the entire
>>> > thread on whether the handover mechanism has been resolved, and I would
>>> > really like to have a proposed solution to this documented in the
>>> > binding. The fact that there is nothing tying the simple framebuffer to
>>> > the actual hardware backing the framebuffer is concerning. It means the
>>> > kernel needs to guess which graphics device is associated with the
>>> > framebuffer.
>>>
>>> We did discuss handover in Düsseldorf, and concluded that the simplefb's
>>> regs property can be used for this.
>>>
>>> While on a modern system with unified memory this association cannot be
>>> derived in a generic way, a device-specific driver for the graphics hardware
>>> can if the regs property of the simplefb node matches the address the CRTC
>>> engine is configured for.
>>
>> ???
>>
>> Right, I'm going to be blunt here: That's just dumb. All the
>> capability needed is there in the DT to associate a simple FB to a
>> display controller, and the solution chosen is to use a heuristic?
>>
>> The association needs to be explicit. I strongly prefer putting the
>> simple FB directly into the display controller node, but I would
>> consider phandle linkage also.
>
> IFF there's a display controller node, you can put it there.
> I actually proposed to have a minimal/preliminary display controller node,
> but people countered that for various reasons (too many components
> with multiple nodes on many systems, bindings not yet defined, etc.).
>
> And if there's no graphics driver/bindings yet at the time the bootloader
> is written, it doesn't know how to link simplefb with it in DT.
> Hence the heuristic to match regs... Does that make sense?

No, not really. The simplefb binding /should/ make it clear that
handover to the real display controller driver is an expected use
case; therefore the simplefb binding should itemize exactly how that
linkage is supposed to be expressed. When the display controller
binding is written, it should go hand in glove with the simplefb
binding. We want to be sure that generic code can be used to resolve a
display controller with (one of) the simple framebuffers.

It also sounds like some are making the assumption that firmware
should generate the simplefb node from scratch, hence all the hand
wringing about firmware not knowing what extra stuff to put into the
simplefb node. I don't think this is necessary, or even a good
approach. A disabled simplefb node can be put into the dts file with
any dependencies it needs to care about and linkage to the real
display controller. Firmware should be able to locate that node,
update the parameters as needed, and enable it. That gets out of the
cycle of updating firmware in lockstep.

g.

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v4 0/5] simplefb: add clock handling code

[Tomi Valkeinen]

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

Hi Hans,

On 22/10/14 19:45, Hans de Goede wrote:

> -We will add a clocks property to the simplefb devicetree bindings, so
>  that u-boot setup framebuffers passed to the kernel (for early console
>  support) can properly list the clocks used, and simplefb can claim them to
>  avoid them getting turned off, thereby breaking the early console

Perhaps this has been discussed earlier, but I started to wonder if
managing clocks is enough.

What about regulators (to power the LCD), gpios (to keep the LCD
enabled) and pinctrl (for video signals)? I guess any those could be
reset by the kernel if no driver uses them.

 Tomi



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

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Hans de Goede]

Hi Tomi,

On 10/31/2014 09:15 AM, Tomi Valkeinen wrote:
> Hi Hans,
> 
> On 22/10/14 19:45, Hans de Goede wrote:
> 
>> -We will add a clocks property to the simplefb devicetree bindings, so
>>  that u-boot setup framebuffers passed to the kernel (for early console
>>  support) can properly list the clocks used, and simplefb can claim them to
>>  avoid them getting turned off, thereby breaking the early console
> 
> Perhaps this has been discussed earlier, but I started to wonder if
> managing clocks is enough.
> 
> What about regulators (to power the LCD), gpios (to keep the LCD
> enabled) and pinctrl (for video signals)? I guess any those could be
> reset by the kernel if no driver uses them.

Yes this has been discussed before, AFAIK gpio / pinctrls should not be
spontaneously reset by the kernels. Regulators will be a problem, but there
we can pretty much follow whatever solution we end up with for clocks, since
they work very much alike (e.g. we could add a supply property to the
simplefb node).

Note I've no intention to add regulator support atm, we can do that when the
need arises, so that we can actually test the code.

Regards,

Hans

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Javier Martinez Canillas]

Hello Hans,

On Fri, Oct 31, 2014 at 10:00 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Hi Tomi,
>
> On 10/31/2014 09:15 AM, Tomi Valkeinen wrote:
>> Hi Hans,
>>
>> On 22/10/14 19:45, Hans de Goede wrote:
>>
>>> -We will add a clocks property to the simplefb devicetree bindings, so
>>>  that u-boot setup framebuffers passed to the kernel (for early console
>>>  support) can properly list the clocks used, and simplefb can claim them to
>>>  avoid them getting turned off, thereby breaking the early console
>>
>> Perhaps this has been discussed earlier, but I started to wonder if
>> managing clocks is enough.
>>

Managing clocks is definitely not enough. For example, adding some
regulators to the Snow Chromebook DTS broke simplefb [0]. This has
been mentioned on at least one of the many simplefb threads.

>> What about regulators (to power the LCD), gpios (to keep the LCD
>> enabled) and pinctrl (for video signals)? I guess any those could be
>> reset by the kernel if no driver uses them.
>
> Yes this has been discussed before, AFAIK gpio / pinctrls should not be
> spontaneously reset by the kernels. Regulators will be a problem, but there
> we can pretty much follow whatever solution we end up with for clocks, since
> they work very much alike (e.g. we could add a supply property to the
> simplefb node).
>

I think someone already said this but with regulators is not that easy
since you don't have an equivalent of the "clocks" property.

You need a "<name>-supply" property for each regulator that is an
input supply. Some boards may need different regulators for their
display (e.g: one to power the LCD and another to power the backlight)
so adding a single supply property will not be enough.

> Note I've no intention to add regulator support atm, we can do that when the
> need arises, so that we can actually test the code.
>

AFAIU that's exactly what Thierry was complaining about. There seems
to be only a focus on solving the clocks issue since that is needed
for sunxi but ignoring how the problem for other resources should be
solved.

The assumptions made by simplefb are quite fragile and only adding
support for managing clocks is just a partial solution that will give
a false sense of robustness since it could break later due to other
resources.

Best regards,
Javier

[0]: http://www.spinics.net/lists/arm-kernel/msg360119.html

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 10/31/2014 10:37 AM, Javier Martinez Canillas wrote:
> Hello Hans,
> 
> On Fri, Oct 31, 2014 at 10:00 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi Tomi,
>>
>> On 10/31/2014 09:15 AM, Tomi Valkeinen wrote:
>>> Hi Hans,
>>>
>>> On 22/10/14 19:45, Hans de Goede wrote:
>>>
>>>> -We will add a clocks property to the simplefb devicetree bindings, so
>>>>  that u-boot setup framebuffers passed to the kernel (for early console
>>>>  support) can properly list the clocks used, and simplefb can claim them to
>>>>  avoid them getting turned off, thereby breaking the early console
>>>
>>> Perhaps this has been discussed earlier, but I started to wonder if
>>> managing clocks is enough.
>>>
> 
> Managing clocks is definitely not enough. For example, adding some
> regulators to the Snow Chromebook DTS broke simplefb [0]. This has
> been mentioned on at least one of the many simplefb threads.

Ok.

>>> What about regulators (to power the LCD), gpios (to keep the LCD
>>> enabled) and pinctrl (for video signals)? I guess any those could be
>>> reset by the kernel if no driver uses them.
>>
>> Yes this has been discussed before, AFAIK gpio / pinctrls should not be
>> spontaneously reset by the kernels. Regulators will be a problem, but there
>> we can pretty much follow whatever solution we end up with for clocks, since
>> they work very much alike (e.g. we could add a supply property to the
>> simplefb node).
>>
> 
> I think someone already said this but with regulators is not that easy
> since you don't have an equivalent of the "clocks" property.
> 
> You need a "<name>-supply" property for each regulator that is an
> input supply. Some boards may need different regulators for their
> display (e.g: one to power the LCD and another to power the backlight)
> so adding a single supply property will not be enough.

Right so we can simply insert simplefbvdd1-supply, simplefbvdd2-supply, etc.
regulator nodes into the simplefb node, and have the simplefb kernel code look
for supplies with a name of simplefb_vdd%d in a loop until it fails to find one.

Note that the foo-supply names are local to the simplefb node, these do not
need to be the same as the ones in the nodes describing the actual hardware
blocks. Not having an array is somewhat inconvenient, but easily worked around
and once worked around this really is just the same problem as with clocks.

>> Note I've no intention to add regulator support atm, we can do that when the
>> need arises, so that we can actually test the code.
>>
> 
> AFAIU that's exactly what Thierry was complaining about. There seems
> to be only a focus on solving the clocks issue since that is needed
> for sunxi but ignoring how the problem for other resources should be
> solved.

That is simply not true, I'm fully aware that we may eventually need other
resources, e.g. regulators. I just want to take this one step at a time,
I really believe that once we have this figured out for one resource, it
becomes a lot easier to also figure out for other resources.

> The assumptions made by simplefb are quite fragile and only adding
> support for managing clocks is just a partial solution that will give
> a false sense of robustness since it could break later due to other
> resources.

The key word here being resources. We need to figure out how to do
resource management for simplefb, once we have that figured out for one
resource, the others can follow the same "design-pattern". As the long
thread about the clocks support has shown, getting consensus on this
with only one resource in play already is hard enough, so baby steps
is the way to go.

Regards,

Hans

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Javier Martinez Canillas]

Hello Hans,

Thanks for your answers.

On Fri, Oct 31, 2014 at 10:51 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>> What about regulators (to power the LCD), gpios (to keep the LCD
>>>> enabled) and pinctrl (for video signals)? I guess any those could be
>>>> reset by the kernel if no driver uses them.
>>>
>>> Yes this has been discussed before, AFAIK gpio / pinctrls should not be
>>> spontaneously reset by the kernels. Regulators will be a problem, but there
>>> we can pretty much follow whatever solution we end up with for clocks, since
>>> they work very much alike (e.g. we could add a supply property to the
>>> simplefb node).
>>>
>>
>> I think someone already said this but with regulators is not that easy
>> since you don't have an equivalent of the "clocks" property.
>>
>> You need a "<name>-supply" property for each regulator that is an
>> input supply. Some boards may need different regulators for their
>> display (e.g: one to power the LCD and another to power the backlight)
>> so adding a single supply property will not be enough.
>
> Right so we can simply insert simplefbvdd1-supply, simplefbvdd2-supply, etc.
> regulator nodes into the simplefb node, and have the simplefb kernel code look
> for supplies with a name of simplefb_vdd%d in a loop until it fails to find one.
>
> Note that the foo-supply names are local to the simplefb node, these do not
> need to be the same as the ones in the nodes describing the actual hardware
> blocks. Not having an array is somewhat inconvenient, but easily worked around
> and once worked around this really is just the same problem as with clocks.
>

Yes, I guess the question is how many regulators the driver should
support but I expect most displays don't need more than a couple.

>>> Note I've no intention to add regulator support atm, we can do that when the
>>> need arises, so that we can actually test the code.
>>>
>>
>> AFAIU that's exactly what Thierry was complaining about. There seems
>> to be only a focus on solving the clocks issue since that is needed
>> for sunxi but ignoring how the problem for other resources should be
>> solved.
>
> That is simply not true, I'm fully aware that we may eventually need other
> resources, e.g. regulators. I just want to take this one step at a time,
> I really believe that once we have this figured out for one resource, it
> becomes a lot easier to also figure out for other resources.
>

Great, I'm glad to hear that you are willing to add support for
regulators (and other resources).

>> The assumptions made by simplefb are quite fragile and only adding
>> support for managing clocks is just a partial solution that will give
>> a false sense of robustness since it could break later due to other
>> resources.
>
> The key word here being resources. We need to figure out how to do
> resource management for simplefb, once we have that figured out for one
> resource, the others can follow the same "design-pattern". As the long
> thread about the clocks support has shown, getting consensus on this
> with only one resource in play already is hard enough, so baby steps
> is the way to go.
>

I'm certainly not against adding clock management if that will make
simplefb more robust. I was just worried that other resources could be
ignored. But I'm ok of doing baby steps if there is a long term plan.

As I mentioned in another thread, I believe Doug's suggestion is the
best approach. That is to have the simplefb node with all its
resources in the DTS but with status = "disabled". The bootloader
should only fill the information about the framebuffer memory region
(location and size, width, height, format, etc) and change the
simplefb node status to "okay".

If a FDT does not have a simplefb node then the bootloader can create
one (like is made for the /choosen node in most bootloaders).

That will better document which resources are really used when someone
reads the DTS and also avoid duplicating code in the bootloaders to
fill all the information about resources but is just my humble opinion
and I really don't want to bikesheed more on this.

Best regards,
Javier

Re: [PATCH v4 0/5] simplefb: add clock handling code

[Tomi Valkeinen]

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

On 31/10/14 10:15, Tomi Valkeinen wrote:
> Hi Hans,
> 
> On 22/10/14 19:45, Hans de Goede wrote:
> 
>> -We will add a clocks property to the simplefb devicetree bindings, so
>>  that u-boot setup framebuffers passed to the kernel (for early console
>>  support) can properly list the clocks used, and simplefb can claim them to
>>  avoid them getting turned off, thereby breaking the early console
> 
> Perhaps this has been discussed earlier, but I started to wonder if
> managing clocks is enough.
> 
> What about regulators (to power the LCD), gpios (to keep the LCD
> enabled) and pinctrl (for video signals)? I guess any those could be
> reset by the kernel if no driver uses them.

Hans, ping. Have these been already discussed?

 Tomi



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

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 11/06/2014 03:18 PM, Tomi Valkeinen wrote:
> On 31/10/14 10:15, Tomi Valkeinen wrote:
>> Hi Hans,
>>
>> On 22/10/14 19:45, Hans de Goede wrote:
>>
>>> -We will add a clocks property to the simplefb devicetree bindings, so
>>>  that u-boot setup framebuffers passed to the kernel (for early console
>>>  support) can properly list the clocks used, and simplefb can claim them to
>>>  avoid them getting turned off, thereby breaking the early console
>>
>> Perhaps this has been discussed earlier, but I started to wonder if
>> managing clocks is enough.
>>
>> What about regulators (to power the LCD), gpios (to keep the LCD
>> enabled) and pinctrl (for video signals)? I guess any those could be
>> reset by the kernel if no driver uses them.
> 
> Hans, ping. Have these been already discussed?

Yes, and I already responded to your original mail on this on Oct. 31th,
for some reason my mail client drops you from the CC, when I do a reply-to-all
to one of your mails (I've added you manually this time).

My replies did go to simple-fb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org .I'll forward my 2 mails on this
to you.

Regards,

Hans

Re: Re: [PATCH v4 0/5] simplefb: add clock handling code

[Grant Likely]

On Thu, 06 Nov 2014 16:05:13 +0100
, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
 wrote:
> Hi,
> 
> On 11/06/2014 03:18 PM, Tomi Valkeinen wrote:
> > On 31/10/14 10:15, Tomi Valkeinen wrote:
> >> Hi Hans,
> >>
> >> On 22/10/14 19:45, Hans de Goede wrote:
> >>
> >>> -We will add a clocks property to the simplefb devicetree bindings, so
> >>>  that u-boot setup framebuffers passed to the kernel (for early console
> >>>  support) can properly list the clocks used, and simplefb can claim them to
> >>>  avoid them getting turned off, thereby breaking the early console
> >>
> >> Perhaps this has been discussed earlier, but I started to wonder if
> >> managing clocks is enough.
> >>
> >> What about regulators (to power the LCD), gpios (to keep the LCD
> >> enabled) and pinctrl (for video signals)? I guess any those could be
> >> reset by the kernel if no driver uses them.

I'm okay with extending the binding for each of these dependencies
/providing/ it can be argued that the way simplefb uses it is merely to
inhibit the kernel from making any changes to the resource, and then
releasing the resource when it is done. Each resource type will need to
be taken on a case-by-case basis since each has it's own behaviour. I'll
want to see that the simplefb driver doesn't need to do anything special
per device, and that the resource binding is well established and
generic.

g.