* Re: [PATCH 2/2] arm, fbdev, omap2, LLVMLinux: Remove nested function from omapfb
From: Felipe Balbi @ 2014-09-27 16:46 UTC (permalink / raw)
To: Behan Webster
Cc: plagnioj, tomi.valkeinen, daniel.vetter, geert, laurent.pinchart,
linux-fbdev, linux-kernel, linux-omap, robdclark, Arnd Bergmann
In-Reply-To: <1411780253-4492-3-git-send-email-behanw@converseincode.com>
[-- Attachment #1: Type: text/plain, Size: 571 bytes --]
On Fri, Sep 26, 2014 at 06:10:53PM -0700, Behan Webster wrote:
> Replace the use of nested functions where a normal function will suffice.
>
> Nested functions are not liked by upstream kernel developers in general. Their
> use breaks the use of clang as a compiler, and doesn't make the code any
> better.
>
> This code now works for both gcc and clang.
>
> Signed-off-by: Behan Webster <behanw@converseincode.com>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] arm, fbdev, omap2, LLVMLinux: Remove nested function from omap2 dss
From: Behan Webster @ 2014-09-27 22:53 UTC (permalink / raw)
To: balbi
Cc: plagnioj, tomi.valkeinen, daniel.vetter, geert, laurent.pinchart,
linux-fbdev, linux-kernel, linux-omap, robdclark, Arnd Bergmann
In-Reply-To: <20140927164554.GA7076@saruman>
On 09/27/14 09:46, Felipe Balbi wrote:
> On Fri, Sep 26, 2014 at 06:10:52PM -0700, Behan Webster wrote:
>> Replace the use of nested functions where a normal function will suffice.
>>
>> Nested functions are not liked by upstream kernel developers in general. Their
>> use breaks the use of clang as a compiler, and doesn't make the code any
>> better.
>>
>> This code now works for both gcc and clang.
>>
>> Signed-off-by: Behan Webster <behanw@converseincode.com>
>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>> Cc: Arnd Bergmann <arnd@arndb.de>
> another one that make sense :-) And probably also deserves
> checkpatch/coccinelle/sparse.
Indeed! That would be very appreciated.
The clang static analyzer already points this one out. :)
Behan
--
Behan Webster
behanw@converseincode.com
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Mike Turquette @ 2014-09-27 23:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140902092508.GR15297@lukather>
Quoting Maxime Ripard (2014-09-02 02:25:08)
> On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
> > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
> > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
> > > > I would think the memory should still be reserved anyway to make sure
> > > > nothing else is writing over it. And it's in the device tree anyway
> > > > because the driver needs to know where to put framebuffer content. So
> > > > the point I was trying to make is that we can't treat the memory in the
> > > > same way as clocks because it needs to be explicitly managed. Whereas
> > > > clocks don't. The driver is simply too generic to know what to do with
> > > > the clocks.
> > >
> > > You agreed on the fact that the only thing we need to do with the
> > > clocks is claim them. Really, I don't find what's complicated there
> > > (or not generic).
> >
> > That's not what I agreed on. What I said is that the only thing we need
> > to do with the clocks is nothing. They are already in the state that
> > they need to be.
>
> Claim was probably a poor choice of words, but still. We have to keep
> the clock running, and both the solution you've been giving and this
> patch do so in a generic way.
>
> > > > It doesn't know what frequency they should be running at
> > >
> > > We don't care about that. Just like we don't care about which
> > > frequency is the memory bus running at. It will just run at whatever
> > > frequency is appropriate.
> >
> > Exactly. And you shouldn't have to care about them at all. Firmware has
> > already configured the clocks to run at the correct frequencies, and it
> > has made sure that they are enabled.
> >
> > > > or what they're used for
> > >
> > > And we don't care about that either. You're not interested in what
> > > output the framebuffer is setup to use, which is pretty much the same
> > > here, this is the same thing here.
> >
> > That's precisely what I've been saying. The only thing that simplefb
> > cares about is the memory it should be using and the format of the
> > pixels (and how many of them) it writes into that memory. Everything
> > else is assumed to have been set up.
> >
> > Including clocks.
>
> We're really discussing in circles here.
>
> Mike?
>
-EHIGHLATENCYRESPONSE
I forgot about this thread. Sorry.
In an attempt to provide the least helpful answer possible, I will stay
clear of all of the stuff relating to "how simple should simplefb be"
and the related reserved memory discussion.
A few times in this thread it is stated that we can't prevent unused
clocks from being disabled. That is only partially true.
The clock framework DOES provide a flag to prevent UNUSED clocks from
being disabled at late_initcall-time by a clock "garbage collector"
mechanism. Maxime and others familiar with the clock framework are aware
of this.
What the framework doesn't do is to allow for a magic flag in DT, in
platform_data or elsewhere that says, "don't let me get turned off until
the right driver claims me". That would be an external or alternative
method for preventing a clock from being disabled. We have a method for
preventing clocks from being disabled. It is as follows:
struct clk *my_clk = clk_get(...);
clk_prepare_enable(my_clk);
That is how it should be done. Period.
With that said I think that Luc's approach is very sensible. I'm not
sure what purpose in the universe DT is supposed to serve if not for
_this_exact_case_. We have a nice abstracted driver, usable by many
people. The hardware details of how it is hooked up at the board level
can all be hidden behind stable DT bindings that everyone already uses.
What more could you want?
Regards,
Mike
> Your opinion would be very valuable.
>
> > > > so by any definition of what DT should describe they're useless for
> > > > this virtual device.
> > > >
> > > > Furthermore it's fairly likely that as your kernel support progresses
> > > > you'll find that the driver all of a sudden needs to manage some other
> > > > type of resource that you just haven't needed until now because it may
> > > > default to being always on. Then you'll have a hard time keeping
> > > > backwards-compatibility and will have to resort to the kinds of hacks
> > > > that you don't want to see in the kernel.
> > >
> > > Not such a hard time. An older DT wouldn't define the new requirements
> > > anyway, so they wouldn't be used, and we would end up in pretty much
> > > the current case.
> >
> > Except that you have firmware in the wild that sets up an incomplete
> > simplefb node and if you don't want to break compatibility you need to
> > provide fallbacks for the resources that aren't listed in the DT node.
> > And given that those fallbacks are all very board specific you'll need
> > to find ways to keep them enabled if you want to keep existing setups
> > working.
>
> How would an *optional* property break those users?
>
> If you don't need any clock to be kept running (or are hiding them
> under the carpet), of course you don't need such a property.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
^ permalink raw reply
* [PATCH 1/4] dt-bindings: Add a clocks property to the simple-framebuffer binding
From: Hans de Goede @ 2014-09-28 12:43 UTC (permalink / raw)
To: linux-arm-kernel
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@redhat.com>
---
Documentation/devicetree/bindings/video/simple-framebuffer.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 70c26f3..e75478e 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -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
^ permalink raw reply related
* [PATCH 2/4] simplefb: formalize pseudo palette handling
From: Hans de Goede @ 2014-09-28 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1411908218-20947-1-git-send-email-hdegoede@redhat.com>
From: Luc Verhaegen <libv@skynet.be>
Signed-off-by: Luc Verhaegen <libv@skynet.be>
[hdegoede@redhat.com: drop unnecessary void * cast]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
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
^ permalink raw reply related
* [PATCH 3/4] simplefb: add goto error path to probe
From: Hans de Goede @ 2014-09-28 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1411908218-20947-1-git-send-email-hdegoede@redhat.com>
From: Luc Verhaegen <libv@skynet.be>
Signed-off-by: Luc Verhaegen <libv@skynet.be>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/video/fbdev/simplefb.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index ec112c1..b7d5c08 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -220,8 +220,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->apertures = alloc_apertures(1);
if (!info->apertures) {
- framebuffer_release(info);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto error_fb_release;
}
info->apertures->ranges[0].base = info->fix.smem_start;
info->apertures->ranges[0].size = info->fix.smem_len;
@@ -231,8 +231,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->screen_base = ioremap_wc(info->fix.smem_start,
info->fix.smem_len);
if (!info->screen_base) {
- framebuffer_release(info);
- return -ENODEV;
+ ret = -ENODEV;
+ goto error_fb_release;
}
info->pseudo_palette = par->palette;
@@ -247,14 +247,20 @@ static int simplefb_probe(struct platform_device *pdev)
ret = register_framebuffer(info);
if (ret < 0) {
dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
- iounmap(info->screen_base);
- framebuffer_release(info);
- return ret;
+ goto error_unmap;
}
dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
return 0;
+
+ error_unmap:
+ iounmap(info->screen_base);
+
+ error_fb_release:
+ framebuffer_release(info);
+
+ return ret;
}
static int simplefb_remove(struct platform_device *pdev)
--
2.1.0
^ permalink raw reply related
* [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-09-28 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1411908218-20947-1-git-send-email-hdegoede@redhat.com>
From: Luc Verhaegen <libv@skynet.be>
This claims and enables clocks listed in the simple framebuffer dt node.
This is needed so that the display engine, in case the required clocks
are known by the kernel code and are described in the dt, will remain
properly enabled.
Signed-off-by: Luc Verhaegen <libv@skynet.be>
[hdegoede@redhat.com: drop dev_err on kzalloc failure]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/video/fbdev/simplefb.c | 100 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 98 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index b7d5c08..f329cc1 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",
@@ -165,8 +166,98 @@ static int simplefb_parse_pd(struct platform_device *pdev,
return 0;
}
+/*
+ * Clock handling code.
+ *
+ * Here we handle the clocks property of our "simple-framebuffer" dt node.
+ * This is necessary so that we can make sure that any clocks needed by
+ * the display engine that the bootloader set up for us (and for which it
+ * provided a simplefb dt node), stay up, for the life of the simplefb
+ * driver.
+ *
+ * When the driver unloads, we cleanly disable, and then release the clocks.
+ */
+struct simplefb_clock {
+ struct list_head list;
+ struct clk *clock;
+};
+
+/*
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up simplefb, and the clock definitions in the device tree. Chances are
+ * that there are no adverse effects, and if there are, a clean teardown of
+ * the fb probe will not help us much either. So just complain and carry on,
+ * and hope that the user actually gets a working fb at the end of things.
+ */
+static void
+simplefb_clocks_init(struct platform_device *pdev, struct list_head *list)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int clock_count, i;
+
+ INIT_LIST_HEAD(list);
+
+ if (dev_get_platdata(&pdev->dev) || !np)
+ return;
+
+ clock_count = of_clk_get_parent_count(np);
+ for (i = 0; i < clock_count; i++) {
+ struct simplefb_clock *entry;
+ struct clk *clock = of_clk_get(np, i);
+ int ret;
+
+ if (IS_ERR(clock)) {
+ dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
+ __func__, i, PTR_ERR(clock));
+ continue;
+ }
+
+ ret = clk_prepare_enable(clock);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "%s: failed to enable clock %d: %d\n",
+ __func__, i, ret);
+ clk_put(clock);
+ continue;
+ }
+
+ entry = kzalloc(sizeof(struct simplefb_clock), GFP_KERNEL);
+ if (!entry) {
+ clk_disable_unprepare(clock);
+ clk_put(clock);
+ continue;
+ }
+
+ entry->clock = clock;
+ /*
+ * add to the front of the list, so we disable clocks in the
+ * correct order.
+ */
+ list_add(&entry->list, list);
+ }
+}
+
+static void
+simplefb_clocks_destroy(struct list_head *list)
+{
+ struct list_head *pos, *n;
+
+ list_for_each_safe(pos, n, list) {
+ struct simplefb_clock *entry + container_of(pos, struct simplefb_clock, list);
+
+ list_del(&entry->list);
+
+ clk_disable_unprepare(entry->clock);
+ clk_put(entry->clock);
+ kfree(entry);
+ }
+}
+
struct simplefb_par {
u32 palette[PSEUDO_PALETTE_SIZE];
+ struct list_head clock_list[1];
};
static int simplefb_probe(struct platform_device *pdev)
@@ -236,6 +327,8 @@ static int simplefb_probe(struct platform_device *pdev)
}
info->pseudo_palette = par->palette;
+ simplefb_clocks_init(pdev, par->clock_list);
+
dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
info->fix.smem_start, info->fix.smem_len,
info->screen_base);
@@ -247,14 +340,15 @@ static int simplefb_probe(struct platform_device *pdev)
ret = register_framebuffer(info);
if (ret < 0) {
dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
- goto error_unmap;
+ goto error_clocks;
}
dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
return 0;
- error_unmap:
+ error_clocks:
+ simplefb_clocks_destroy(par->clock_list);
iounmap(info->screen_base);
error_fb_release:
@@ -266,8 +360,10 @@ static int simplefb_probe(struct platform_device *pdev)
static int simplefb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ struct simplefb_par *par = info->par;
unregister_framebuffer(info);
+ simplefb_clocks_destroy(par->clock_list);
framebuffer_release(info);
return 0;
--
2.1.0
^ permalink raw reply related
* Re: [PATCH 1/4] dt-bindings: Add a clocks property to the simple-framebuffer binding
From: Mike Turquette @ 2014-09-28 18:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1411908218-20947-1-git-send-email-hdegoede@redhat.com>
On Sun, Sep 28, 2014 at 5:43 AM, Hans de Goede <hdegoede@redhat.com> 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@redhat.com>
Acked-by: Mike Turquette <mturquette@linaro.org>
or
Reviewed-by: Mike Turquette <mturquette@linaro.org>
I don't know what is the right thing with these binding definitions...
Also I have one suggestion below:
> ---
> Documentation/devicetree/bindings/video/simple-framebuffer.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 70c26f3..e75478e 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -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 {
It might be nice to add the clocks property to the example. Something like:
clocks = <&osc 1>, <&ref 0>;
Regards,
Mike
> --
> 2.1.0
>
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: Add a clocks property to the simple-framebuffer binding
From: Hans de Goede @ 2014-09-28 18:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAPtuhThAgUThzdHAnWydDeM8unV=fjT88PH3Ww6h5_ezeGEOYg@mail.gmail.com>
Hi,
On 09/28/2014 08:37 PM, Mike Turquette wrote:
> On Sun, Sep 28, 2014 at 5:43 AM, Hans de Goede <hdegoede@redhat.com> 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@redhat.com>
>
> Acked-by: Mike Turquette <mturquette@linaro.org>
> or
> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>
> I don't know what is the right thing with these binding definitions...
>
> Also I have one suggestion below:
>
>> ---
>> Documentation/devicetree/bindings/video/simple-framebuffer.txt | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index 70c26f3..e75478e 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -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 {
>
> It might be nice to add the clocks property to the example. Something like:
>
> clocks = <&osc 1>, <&ref 0>;
Given the optional nature of the clocks, I've deliberately left them out,
but if people want to I can do a v2 with a clocks property added to the example.
Regards,
Hans
^ permalink raw reply
* 用了一次你就会觉得是最好的短信发送平台 =?utf-8
From: 小范 @ 2014-09-29 3:03 UTC (permalink / raw)
To: linux-fbdev
DQogIOi/meS4quefreS/oeWPkemAgeW5s+WPsOWunuWcqOaYr+WkquWlveeUqOS6hu+8jOWGjeS5
n+S4jeaEgeaJvuS4jeWIsOWlveeahOW5s+WPsOS6hu+8jOWPquimgeaYr+S9oOWPkeeahOWGheWu
ueaYr+ato+inhOeahO+8jOi9u+advuWwseWPr+S7peWPkemAgeWHuuWOu+OAguWKoFFRMTkzOTg5
NzI15YW35L2T5ZKo6K+i77yM5Y+v5Lul5byA6LSm5Y+35rWL5pWI5p6c44CCDQoNCg0KDQoNCg0K
IA0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0K
DQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoN
Cg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0K
DQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCg0KDQoNCuS5n+S4jeWlveWvueWlueiC
huaXoOW/jOaDrueahOeMpeeQkOOAgiAgICDku5bku47luorkuIrlnZDotbfmnaXvvIzliLDlhazl
hbHljavnlJ/pl7TmtJfkuobmiorohLjvvIzplZzlrZDkuK3nmoToh6rlt7Hov5jnnJ/mnInlh6Dl
iIbluIXmsJTvvIznu5noh6rlt7HkuIDkuKroh6rkv6HnmoTlvq7nrJHvvIzlj4jlgZrkuobkuKro
o4XpgLznmoTmt7Hmsonooajmg4XvvIzkuIvmpbzljrvljr/mlL/lupzlpKfpmaLmib7ooaXor77i
gKbigKbigJ0gICAg5Lyv5q+N5q2k5pe25LuO6YeM5bGL6LWw5Ye65p2l77yM6L+e5b+Z5Yi25q2i
77yM6YGT77ya4oCc5Yir5Lmx5p2l77yM56OV5L2P5L2g5byf5byf77yf4oCdICAgIOWkj+aTjuWk
qeS4gOe/u+eZveecvO+8jOmBk++8muKAnOS7luaJjeWkmumHje+8n+aIkeS8muWkseaJi++8n+KA
nSAgICDkuI3ov4fov5jmmK/miorlvJ/lvJ/nqLPnqLPlnLDmlL7lnKjlnLDkuIrjgIIgICAg5LuW
5aW95YOP6auY5bqm5qaC5ous77yM4oCc5aSp5LiL5L2V5Lq65LiN6K+G5ZCb4oCd5Y+v5piv5LiA
54K56YO95LiN5aS45byg77yM5Y+q5piv546w5Zyo5oCO5LmI6IO96K+05piO77yf6Zq+6YGT6K+0
6Ieq5bex56m/6LaK6L+H5p2l55qE77yf5Zyo5oiR55qE5YmN5LiW77yM5L2g5bCx5piv5ZCN5ruh
5YWo55CD55qE5aSn5Lq654mp77yfICAgIOadqOaZr+WInemBk++8muKAnOWUie+8jOWls+S6uu+8
jOaIkeaDs+W+l+W8gO+8jOWlueWPquaYr+mHjOOAgiAgICDpmYjnuqLosYbmhI/lkbPmt7Hplb/l
nLDnnIvkuoblpI/lsI/mtJvkuIDnnLzvvIzpgZPvvJrigJzkvaDku6zkubDov4fnpajkuoblkKfv
vIzmiJHljrvkubDnpajvvIzmsqHkubDnmoTor53miJHluK7kvaDku6zluKbjgILigJ0gICAg5aSP
5bCP5rSb5b+D5Lit6aqC6YGT77yM6I2J5rOl6ams77yM5aW95YOP5oiR5by65pq05LqG5L2g5a62
5L2V6K+X6Z+15Ly85Zyw44CCICAgIOmZiOe6ouixhuKAnOi6q+S9k+WjsOWQn+WUseedgOS4gOmm
lkJhYnlmYWNl55qE44CK6ZmM55Sf5Lq655qE55y844CL44CCICAgIOS4pOS6uuWGsumFkuS/neim
geS6huS4pOeTtueZvuWoge+8jOaFouaFouWcsOWWneedgO+8jOecr+edgOi/keinhuecvO+8jOeb
ruWFieWcqOaVtOS4qumFkuWQp+adpeWbnuepv+aire+8jOWvu+aJvuW/g+S7queahOeMjueJqeOA
giAgICDigJzlmL/vvIzkvaDnnIvpgqPovrnpgqPkuKTljLnigJTigJTlpKfmtIvpqazvvIHosJHp
gZPjgIIgICAg5YmN5LiW6Ieq5bex54i25Lqy6L+Z5LmI5pyJ5bm96buY5oSf77yM6Ieq5bex5oCO
5LmI5LuO5p2l5rKh5pyJ5Y2w6LGh77yf5ZSJ77yM5aSn57qm5piv5LuW5ZCO5p2l5LuV6YCU5LiN
6aG677yM5oqR6YOB5LqG5LiA55Sf77yM6L+Z56eN5aSp54S255qE5LmQ5aSp6KKr6Imw6Zq+55qE
55Sf5rS75raI56Oo5o6J5LqG5ZCn44CCICAgIOaXoueEtuiHquW3seS7iueUn+mHjeadpe+8jOWw
seS4jeiDveWcqOiuqQkNCueUn+S9oOermei1t+adpe+8jOS9oOaJvuaIkeS7rOWNq+eUn+WxgOac
ieWVpeS6i++8n+KAneWkj+i/keS4nOa4qeiogOmBk++8jOKAnOaIkeaYr+WNq+eUn+WxgOWJr+Wx
gOmVv++8jOacieS7gOS5iOS6i++8jOS9oOWRiuivieaIkeOAguKAnSAgICDigJzlk47lkYDvvIzl
jp/mnaXmmK/lpKfpooblr7zllYrvvIHmiJHlj6/mib7nnYDkurrkuobjgILigJ3ogIHmsYnlj4jm
v4Dliqjlj4jmga3mlazlnLDor7TvvIzkuJzku6XlpI/lsI/mtJvmiYDor7TnmoTigJzooaXplIXm
s5XigJ3ov4XpgJ/lnLDlj43otKXkuLrog5zvvIzmjIHkuYXkvZzmiJjnmoTor53vvIznjovkv4rk
vJ/kuZ/kuI3mmK/lr7nmiYvjgIIgICAg5L2G5piv5q2k5pe255qE5L2V5Lqs55Sf5ZKM5Y2i5Yab
6LaF55qE6L+Z5Zy65paX5LqJ6KaB5piv5byA5oiY55qE6K+d77yM5pW05L2T5p2l6K+077yM5L2V
5Lqs55Sf5YiZ5aSE5LqO5byx5Yq/5Zyw5L2N44CC5bO75bOw6Ieq5bex5Lmf5rKh5LuA5LmI5aW9
5Yqe5rOV77yM6Kej5pWR5Lq66LSo5Y6G5p2l5piv5omA5pyJ56qB5Y+R5LqL5Lu25Lit5pyA5Zuw
6Zq+55qE77yM5LuW5Lus5bCP5bCP5Y6/5Z+O6K2m5Yqb5Y+I5LiN5YWF6Laz77yM5rKh5pyJ54uZ
5Ye75omL77yb5Y2z5L2/5pyJ54uZ5Ye75omL77yM6ZmE6L+R5Lmf5rKh5pyJ6auY55qE5bu6562R
77yM5Zyw5b2i5a+55LuW5Lus5Lmf5b6I5LiN5Yip77yM77yM5YCY6Iul6KaB5LuO5qW8576k5LyX
77yM55av5Lq65p6c5b2T572X5rGJ5p6c5Y2W77yB5ZCD5q275Lq66YO95LiN55+l6YGT5oCO5LmI
5q2755qE44CC4oCdICAgIOS4jeaZk+W+l+S7gOS5iOaXtuWAme+8jOavjeS6suiuuOWwj+abvOW3
sue7j+WFs+S6hueUteinhu+8jOWdkOWcqOS7luS7rOi6q+i+ue+8jOWQrOedgOeItuWtkOS6jOS6
uuWVhuiuruWkp+S6i++8jOi/meS8muemgeS4jeS9j+aPkuWYtOi1t+adpeOAgiAgICDlpI/ov5Hk
uJzmraTml7blv4PlpI/lsI/mtJvmiZPph4/nnYDmn7PmnIjvvIzlpbnkuIrouqvnqb/nnYDkuIDk
u7bmrL7lvI/mlrDpopbnmoTnn63mrL7nmoToloToloTnmoTnvr3nu5LmnI3vvIzkuIvouqvnqb/n
nYDkuIDku7bpu5HoibLnmoTniZvku5Too6TvvIzlj6vkuIrnqb/nnYDkuIDlj4zljYrpq5jot5/n
moTpu5HoibLnmq7pnovvvIzmlbTkuKrkurrmmL7lvpfkuq3kuq3njonnq4vvvIzkuIDlj4zmvIbp
u5HnmoTlpKfnnLznnZvnnavmr5vmmL7lvpflj4jplb/ph43mnaXkuozljYHlubTvvIzmmK/orqno
h6rlt7HotY7nvarnmoTkuYjvvJ8gICAg5Lq655Sf6KeC5q+U6L6D5oKy6KeC55qE5Y+U5pys5Y2O
5pu+57uP6K+06L+H77yM5pmu6YCa5Lq65Y+q5piv5oOz6L+H5aaC5L2V5bqm6L+H5pe26Ze077yM
6ICM5LiN5q275aaC5L2V5Yip55So5pe26Ze044CC6Ieq5bex6K+75Yiw6L+Z5Y+l6K+d55qE5pe2
5YCZ77yM5bey57uP5piv6ICM56uL5LmL5bm077yM6YKj5pe25YCZ77yM5Y+q5pyJ5o2PCQ0K
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 8:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140927235601.19023.31593@quantum>
[-- Attachment #1.1: Type: text/plain, Size: 6679 bytes --]
On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
> Quoting Maxime Ripard (2014-09-02 02:25:08)
> > On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
> > > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
> > > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
> > > > > I would think the memory should still be reserved anyway to make sure
> > > > > nothing else is writing over it. And it's in the device tree anyway
> > > > > because the driver needs to know where to put framebuffer content. So
> > > > > the point I was trying to make is that we can't treat the memory in the
> > > > > same way as clocks because it needs to be explicitly managed. Whereas
> > > > > clocks don't. The driver is simply too generic to know what to do with
> > > > > the clocks.
> > > >
> > > > You agreed on the fact that the only thing we need to do with the
> > > > clocks is claim them. Really, I don't find what's complicated there
> > > > (or not generic).
> > >
> > > That's not what I agreed on. What I said is that the only thing we need
> > > to do with the clocks is nothing. They are already in the state that
> > > they need to be.
> >
> > Claim was probably a poor choice of words, but still. We have to keep
> > the clock running, and both the solution you've been giving and this
> > patch do so in a generic way.
> >
> > > > > It doesn't know what frequency they should be running at
> > > >
> > > > We don't care about that. Just like we don't care about which
> > > > frequency is the memory bus running at. It will just run at whatever
> > > > frequency is appropriate.
> > >
> > > Exactly. And you shouldn't have to care about them at all. Firmware has
> > > already configured the clocks to run at the correct frequencies, and it
> > > has made sure that they are enabled.
> > >
> > > > > or what they're used for
> > > >
> > > > And we don't care about that either. You're not interested in what
> > > > output the framebuffer is setup to use, which is pretty much the same
> > > > here, this is the same thing here.
> > >
> > > That's precisely what I've been saying. The only thing that simplefb
> > > cares about is the memory it should be using and the format of the
> > > pixels (and how many of them) it writes into that memory. Everything
> > > else is assumed to have been set up.
> > >
> > > Including clocks.
> >
> > We're really discussing in circles here.
> >
> > Mike?
> >
>
> -EHIGHLATENCYRESPONSE
>
> I forgot about this thread. Sorry.
>
> In an attempt to provide the least helpful answer possible, I will stay
> clear of all of the stuff relating to "how simple should simplefb be"
> and the related reserved memory discussion.
>
> A few times in this thread it is stated that we can't prevent unused
> clocks from being disabled. That is only partially true.
>
> The clock framework DOES provide a flag to prevent UNUSED clocks from
> being disabled at late_initcall-time by a clock "garbage collector"
> mechanism. Maxime and others familiar with the clock framework are aware
> of this.
>
> What the framework doesn't do is to allow for a magic flag in DT, in
> platform_data or elsewhere that says, "don't let me get turned off until
> the right driver claims me". That would be an external or alternative
> method for preventing a clock from being disabled. We have a method for
> preventing clocks from being disabled. It is as follows:
>
> struct clk *my_clk = clk_get(...);
> clk_prepare_enable(my_clk);
>
> That is how it should be done. Period.
>
> With that said I think that Luc's approach is very sensible. I'm not
> sure what purpose in the universe DT is supposed to serve if not for
> _this_exact_case_. We have a nice abstracted driver, usable by many
> people. The hardware details of how it is hooked up at the board level
> can all be hidden behind stable DT bindings that everyone already uses.
simplefb doesn't deal at all with hardware details. It simply uses what
firmware has set up, which is the only reason why it will work for many
people. What is passed in via its device tree node is the minimum amount
of information needed to draw something into the framebuffer. Also note
that the simplefb device tree node is not statically added to a DTS file
but needs to be dynamically generated by firmware at runtime.
If we start extending the binding with board-level details we end up
duplicating the device tree node for the proper video device. Also note
that it won't stop at clocks. Other setups will require regulators to be
listed in this device tree node as well so that they don't get disabled
at late_initcall. And the regulator bindings don't provide a method to
list an arbitrary number of clocks in a single property in the way that
the clocks property works.
There may be also resets involved. Fortunately the reset framework is
minimalistic enough not to care about asserting all unused resets at
late_initcall. And other things like power domains may also need to be
kept on.
Passing in clock information via the device tree already requires a non-
trivial amount of code in the firmware. A similar amount of code would
be necessary for each type of resource that needs to be kept enabled. In
addition to the above some devices may also require resources that have
no generic bindings. That just doesn't scale.
The only reasonable thing for simplefb to do is not deal with any kind
of resource at all (except perhaps area that contains the framebuffer
memory).
So how about instead of requiring resources to be explicitly claimed we
introduce something like the below patch? The intention being to give
"firmware device" drivers a way of signalling to the clock framework
that they need rely on clocks set up by firmware and when they no longer
need them. This implements essentially what Mark (CC'ing again on this
subthread) suggested earlier in this thread. Basically, it will allow
drivers to determine the time when unused clocks are really unused. It
will of course only work when used correctly by drivers. For the case of
simplefb I'd expect its .probe() implementation to call the new
clk_ignore_unused() function and once it has handed over control of the
display hardware to the real driver it can call clk_unignore_unused() to
signal that all unused clocks that it cares about have now been claimed.
This is "reference counted" and can therefore be used by more than a
single driver if necessary. Similar functionality could be added for
other resource subsystems as needed.
Thierry
[-- Attachment #1.2: 0001-clk-Allow-drivers-to-reference-unused-clocks.patch --]
[-- Type: text/x-diff, Size: 5397 bytes --]
From 5e2521feab41b71fc80b1b41d4eb6ec967919eed Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Mon, 29 Sep 2014 08:10:49 +0200
Subject: [PATCH] clk: Allow drivers to reference unused clocks
Some drivers are designed to take over a virtual device set up at boot
time by firmware. This can be useful for early boot output on a display
device when no debug serial console is available or for transitioning
from firmware to the Linux kernel in a seemless way.
This type of driver relies on firmware to have set up hardware in a way
that it can scan out a framebuffer using the display hardware. Some of
the resources used by the display hardware (clocks, power supplies, ...)
will typically be turned off (at late_initcall time) by the respective
Linux kernel subsystems unless explicitly claimed by some hardware-
specific driver. However, if this driver is built as a module (loaded
from a filesystem) or defer probing, they will not claim the resources
until long after late_initcall time. This doesn't allow for a seemless
transition.
It can also happen that the hardware-specific driver is never loaded. It
may be that no such driver exists, or it fails to load. Users may even
decide not to load it on purpose, perhaps because it is buggy. Instead
of turning the display off in such cases, a better option is to keep
running with the existing framebuffer, which may also be helpful in
diagnosing why the real driver wasn't loaded.
This patch provides a way for drivers that make use of clocks set up by
firmware to prevent clocks from being automatically disabled. Similarly
a way is provided to signal that they no longer need the clocks (when
the hardware-specific driver has taken over for example).
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
drivers/clk/clk.c | 58 +++++++++++++++++++++++++++++++++++------------------
include/linux/clk.h | 26 ++++++++++++++++++++++++
2 files changed, 65 insertions(+), 19 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 52d58279a612..2007f10e244c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -486,38 +486,58 @@ out:
return;
}
-static bool clk_ignore_unused;
-static int __init clk_ignore_unused_setup(char *__unused)
+static unsigned long clk_ignore_unused_count = 1;
+
+void clk_ignore_unused(void)
{
- clk_ignore_unused = true;
- return 1;
+ clk_prepare_lock();
+
+ if (clk_ignore_unused_count == 0)
+ pr_warn("clk: unused clocks already disabled\n");
+ else
+ clk_ignore_unused_count++;
+
+ clk_prepare_unlock();
}
-__setup("clk_ignore_unused", clk_ignore_unused_setup);
-static int clk_disable_unused(void)
+void clk_unignore_unused(void)
{
struct clk *clk;
- if (clk_ignore_unused) {
- pr_warn("clk: Not disabling unused clocks\n");
- return 0;
- }
-
clk_prepare_lock();
- hlist_for_each_entry(clk, &clk_root_list, child_node)
- clk_disable_unused_subtree(clk);
+ if (--clk_ignore_unused_count == 0) {
+ hlist_for_each_entry(clk, &clk_root_list, child_node)
+ clk_disable_unused_subtree(clk);
- hlist_for_each_entry(clk, &clk_orphan_list, child_node)
- clk_disable_unused_subtree(clk);
+ hlist_for_each_entry(clk, &clk_orphan_list, child_node)
+ clk_disable_unused_subtree(clk);
- hlist_for_each_entry(clk, &clk_root_list, child_node)
- clk_unprepare_unused_subtree(clk);
+ hlist_for_each_entry(clk, &clk_root_list, child_node)
+ clk_unprepare_unused_subtree(clk);
- hlist_for_each_entry(clk, &clk_orphan_list, child_node)
- clk_unprepare_unused_subtree(clk);
+ hlist_for_each_entry(clk, &clk_orphan_list, child_node)
+ clk_unprepare_unused_subtree(clk);
+ }
clk_prepare_unlock();
+}
+
+static int __init clk_ignore_unused_setup(char *__unused)
+{
+ clk_ignore_unused();
+ return 1;
+}
+__setup("clk_ignore_unused", clk_ignore_unused_setup);
+
+static int clk_disable_unused(void)
+{
+ clk_unignore_unused();
+
+ if (clk_ignore_unused_count > 0) {
+ pr_warn("clk: Not disabling unused clocks\n");
+ return 0;
+ }
return 0;
}
diff --git a/include/linux/clk.h b/include/linux/clk.h
index afb44bfaf8d1..b587f2b2f2f5 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -106,6 +106,24 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
*/
long clk_get_accuracy(struct clk *clk);
+/**
+ * clk_ignore_unused - request that unused clocks be kept running
+ *
+ * This function can be used by drivers to request that unused clocks are kept
+ * running. It is useful for drivers that take over hardware previously set up
+ * by firmware and which may not explicitly claim all clocks.
+ */
+void clk_ignore_unused(void);
+
+/**
+ * clk_unignore_unused - release unused clocks
+ *
+ * Use this function to undo the effects of clk_ignore_unused(). It is meant
+ * to be called by drivers for firmware devices after they've handed off the
+ * device to a proper hardware-specific driver.
+ */
+void clk_unignore_unused(void);
+
#else
static inline long clk_get_accuracy(struct clk *clk)
@@ -113,6 +131,14 @@ static inline long clk_get_accuracy(struct clk *clk)
return -ENOTSUPP;
}
+static inline void clk_ignore_unused(void)
+{
+}
+
+static inline void clk_unignore_unused(void)
+{
+}
+
#endif
/**
--
2.1.0
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply related
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Geert Uytterhoeven @ 2014-09-29 8:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929080637.GB12506@ulmo>
Hi Thierry,
(CC linux-pm, as PM is the real reason behind disabling unused clocks)
(CC gregkh and lkml, for driver core)
On Mon, Sep 29, 2014 at 10:06 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
>> Quoting Maxime Ripard (2014-09-02 02:25:08)
>> > On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
>> > > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
>> > > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
>> > > > > I would think the memory should still be reserved anyway to make sure
>> > > > > nothing else is writing over it. And it's in the device tree anyway
>> > > > > because the driver needs to know where to put framebuffer content. So
>> > > > > the point I was trying to make is that we can't treat the memory in the
>> > > > > same way as clocks because it needs to be explicitly managed. Whereas
>> > > > > clocks don't. The driver is simply too generic to know what to do with
>> > > > > the clocks.
>> > > >
>> > > > You agreed on the fact that the only thing we need to do with the
>> > > > clocks is claim them. Really, I don't find what's complicated there
>> > > > (or not generic).
>> > >
>> > > That's not what I agreed on. What I said is that the only thing we need
>> > > to do with the clocks is nothing. They are already in the state that
>> > > they need to be.
>> >
>> > Claim was probably a poor choice of words, but still. We have to keep
>> > the clock running, and both the solution you've been giving and this
>> > patch do so in a generic way.
>> >
>> > > > > It doesn't know what frequency they should be running at
>> > > >
>> > > > We don't care about that. Just like we don't care about which
>> > > > frequency is the memory bus running at. It will just run at whatever
>> > > > frequency is appropriate.
>> > >
>> > > Exactly. And you shouldn't have to care about them at all. Firmware has
>> > > already configured the clocks to run at the correct frequencies, and it
>> > > has made sure that they are enabled.
>> > >
>> > > > > or what they're used for
>> > > >
>> > > > And we don't care about that either. You're not interested in what
>> > > > output the framebuffer is setup to use, which is pretty much the same
>> > > > here, this is the same thing here.
>> > >
>> > > That's precisely what I've been saying. The only thing that simplefb
>> > > cares about is the memory it should be using and the format of the
>> > > pixels (and how many of them) it writes into that memory. Everything
>> > > else is assumed to have been set up.
>> > >
>> > > Including clocks.
>> >
>> > We're really discussing in circles here.
>> >
>> > Mike?
>> >
>>
>> -EHIGHLATENCYRESPONSE
>>
>> I forgot about this thread. Sorry.
>>
>> In an attempt to provide the least helpful answer possible, I will stay
>> clear of all of the stuff relating to "how simple should simplefb be"
>> and the related reserved memory discussion.
>>
>> A few times in this thread it is stated that we can't prevent unused
>> clocks from being disabled. That is only partially true.
>>
>> The clock framework DOES provide a flag to prevent UNUSED clocks from
>> being disabled at late_initcall-time by a clock "garbage collector"
>> mechanism. Maxime and others familiar with the clock framework are aware
>> of this.
>>
>> What the framework doesn't do is to allow for a magic flag in DT, in
>> platform_data or elsewhere that says, "don't let me get turned off until
>> the right driver claims me". That would be an external or alternative
>> method for preventing a clock from being disabled. We have a method for
>> preventing clocks from being disabled. It is as follows:
>>
>> struct clk *my_clk = clk_get(...);
>> clk_prepare_enable(my_clk);
>>
>> That is how it should be done. Period.
>>
>> With that said I think that Luc's approach is very sensible. I'm not
>> sure what purpose in the universe DT is supposed to serve if not for
>> _this_exact_case_. We have a nice abstracted driver, usable by many
>> people. The hardware details of how it is hooked up at the board level
>> can all be hidden behind stable DT bindings that everyone already uses.
>
> simplefb doesn't deal at all with hardware details. It simply uses what
> firmware has set up, which is the only reason why it will work for many
It doesn't deal with "hardware details for hardware components for which
no driver is available (yet)". That's why the hardware is still in a usable
state, after the firmware has set it up.
Clocks, regulators, PM domains typically have system-wide implications,
and thus need system-wide drivers (in the absence of such drivers,
things would work as-is).
Note that the driver still requests resources (ioremap the frame buffer),
so it needs to know about that tiny piece of hardware detail.
> people. What is passed in via its device tree node is the minimum amount
> of information needed to draw something into the framebuffer. Also note
> that the simplefb device tree node is not statically added to a DTS file
> but needs to be dynamically generated by firmware at runtime.
The latter indeed complicates things. But see below... [*]
> If we start extending the binding with board-level details we end up
> duplicating the device tree node for the proper video device. Also note
> that it won't stop at clocks. Other setups will require regulators to be
> listed in this device tree node as well so that they don't get disabled
> at late_initcall. And the regulator bindings don't provide a method to
> list an arbitrary number of clocks in a single property in the way that
> the clocks property works.
Then (optional) regulator support needs to be added.
> There may be also resets involved. Fortunately the reset framework is
> minimalistic enough not to care about asserting all unused resets at
> late_initcall. And other things like power domains may also need to be
> kept on.
Fortunately, unlike clocks, PM domains are first class citizens in the
device framework, as they're handled by the driver core.
So just adding a power-domains property to DTS will work, without any
driver change.
> Passing in clock information via the device tree already requires a non-
> trivial amount of code in the firmware. A similar amount of code would
> be necessary for each type of resource that needs to be kept enabled. In
> addition to the above some devices may also require resources that have
> no generic bindings. That just doesn't scale.
[*] The firmware does need to make sure the clocks, regulators, PM domains,
... are up and running for the initial video mode, too. So it already needs
to have this knowledge (unless enabled by SoC reset-state).
> The only reasonable thing for simplefb to do is not deal with any kind
> of resource at all (except perhaps area that contains the framebuffer
> memory).
>
> So how about instead of requiring resources to be explicitly claimed we
> introduce something like the below patch? The intention being to give
> "firmware device" drivers a way of signalling to the clock framework
> that they need rely on clocks set up by firmware and when they no longer
> need them. This implements essentially what Mark (CC'ing again on this
> subthread) suggested earlier in this thread. Basically, it will allow
> drivers to determine the time when unused clocks are really unused. It
> will of course only work when used correctly by drivers. For the case of
> simplefb I'd expect its .probe() implementation to call the new
> clk_ignore_unused() function and once it has handed over control of the
> display hardware to the real driver it can call clk_unignore_unused() to
> signal that all unused clocks that it cares about have now been claimed.
> This is "reference counted" and can therefore be used by more than a
> single driver if necessary. Similar functionality could be added for
> other resource subsystems as needed.
This still won't work for modules, right? Or am I missing something?
With modules you will never know in advance what will be used and what
won't be used, so you need to keep all clocks, regulators, PM domains, ...
up and running?
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
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 8:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdV8job81-sf88Dx6w=0GZsWD02h94picAtYw2BtSz=Kgw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 11470 bytes --]
On Mon, Sep 29, 2014 at 10:27:41AM +0200, Geert Uytterhoeven wrote:
> Hi Thierry,
>
> (CC linux-pm, as PM is the real reason behind disabling unused clocks)
> (CC gregkh and lkml, for driver core)
>
> On Mon, Sep 29, 2014 at 10:06 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
> >> Quoting Maxime Ripard (2014-09-02 02:25:08)
> >> > On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
> >> > > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
> >> > > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
> >> > > > > I would think the memory should still be reserved anyway to make sure
> >> > > > > nothing else is writing over it. And it's in the device tree anyway
> >> > > > > because the driver needs to know where to put framebuffer content. So
> >> > > > > the point I was trying to make is that we can't treat the memory in the
> >> > > > > same way as clocks because it needs to be explicitly managed. Whereas
> >> > > > > clocks don't. The driver is simply too generic to know what to do with
> >> > > > > the clocks.
> >> > > >
> >> > > > You agreed on the fact that the only thing we need to do with the
> >> > > > clocks is claim them. Really, I don't find what's complicated there
> >> > > > (or not generic).
> >> > >
> >> > > That's not what I agreed on. What I said is that the only thing we need
> >> > > to do with the clocks is nothing. They are already in the state that
> >> > > they need to be.
> >> >
> >> > Claim was probably a poor choice of words, but still. We have to keep
> >> > the clock running, and both the solution you've been giving and this
> >> > patch do so in a generic way.
> >> >
> >> > > > > It doesn't know what frequency they should be running at
> >> > > >
> >> > > > We don't care about that. Just like we don't care about which
> >> > > > frequency is the memory bus running at. It will just run at whatever
> >> > > > frequency is appropriate.
> >> > >
> >> > > Exactly. And you shouldn't have to care about them at all. Firmware has
> >> > > already configured the clocks to run at the correct frequencies, and it
> >> > > has made sure that they are enabled.
> >> > >
> >> > > > > or what they're used for
> >> > > >
> >> > > > And we don't care about that either. You're not interested in what
> >> > > > output the framebuffer is setup to use, which is pretty much the same
> >> > > > here, this is the same thing here.
> >> > >
> >> > > That's precisely what I've been saying. The only thing that simplefb
> >> > > cares about is the memory it should be using and the format of the
> >> > > pixels (and how many of them) it writes into that memory. Everything
> >> > > else is assumed to have been set up.
> >> > >
> >> > > Including clocks.
> >> >
> >> > We're really discussing in circles here.
> >> >
> >> > Mike?
> >> >
> >>
> >> -EHIGHLATENCYRESPONSE
> >>
> >> I forgot about this thread. Sorry.
> >>
> >> In an attempt to provide the least helpful answer possible, I will stay
> >> clear of all of the stuff relating to "how simple should simplefb be"
> >> and the related reserved memory discussion.
> >>
> >> A few times in this thread it is stated that we can't prevent unused
> >> clocks from being disabled. That is only partially true.
> >>
> >> The clock framework DOES provide a flag to prevent UNUSED clocks from
> >> being disabled at late_initcall-time by a clock "garbage collector"
> >> mechanism. Maxime and others familiar with the clock framework are aware
> >> of this.
> >>
> >> What the framework doesn't do is to allow for a magic flag in DT, in
> >> platform_data or elsewhere that says, "don't let me get turned off until
> >> the right driver claims me". That would be an external or alternative
> >> method for preventing a clock from being disabled. We have a method for
> >> preventing clocks from being disabled. It is as follows:
> >>
> >> struct clk *my_clk = clk_get(...);
> >> clk_prepare_enable(my_clk);
> >>
> >> That is how it should be done. Period.
> >>
> >> With that said I think that Luc's approach is very sensible. I'm not
> >> sure what purpose in the universe DT is supposed to serve if not for
> >> _this_exact_case_. We have a nice abstracted driver, usable by many
> >> people. The hardware details of how it is hooked up at the board level
> >> can all be hidden behind stable DT bindings that everyone already uses.
> >
> > simplefb doesn't deal at all with hardware details. It simply uses what
> > firmware has set up, which is the only reason why it will work for many
>
> It doesn't deal with "hardware details for hardware components for which
> no driver is available (yet)". That's why the hardware is still in a usable
> state, after the firmware has set it up.
>
> Clocks, regulators, PM domains typically have system-wide implications,
> and thus need system-wide drivers (in the absence of such drivers,
> things would work as-is).
>
> Note that the driver still requests resources (ioremap the frame buffer),
> so it needs to know about that tiny piece of hardware detail.
That's not a hardware detail. Or at least it isn't hardware specific. It
is needed and the same irrespective of display hardware. Clocks, power
domains, regulators and all those are not always the same.
> > people. What is passed in via its device tree node is the minimum amount
> > of information needed to draw something into the framebuffer. Also note
> > that the simplefb device tree node is not statically added to a DTS file
> > but needs to be dynamically generated by firmware at runtime.
>
> The latter indeed complicates things. But see below... [*]
>
> > If we start extending the binding with board-level details we end up
> > duplicating the device tree node for the proper video device. Also note
> > that it won't stop at clocks. Other setups will require regulators to be
> > listed in this device tree node as well so that they don't get disabled
> > at late_initcall. And the regulator bindings don't provide a method to
> > list an arbitrary number of clocks in a single property in the way that
> > the clocks property works.
>
> Then (optional) regulator support needs to be added.
Can you elaborate?
> > There may be also resets involved. Fortunately the reset framework is
> > minimalistic enough not to care about asserting all unused resets at
> > late_initcall. And other things like power domains may also need to be
> > kept on.
>
> Fortunately, unlike clocks, PM domains are first class citizens in the
> device framework, as they're handled by the driver core.
> So just adding a power-domains property to DTS will work, without any
> driver change.
Well, the device driver would also need to call into the PM runtime
framework to enable all the first class citizen magic. But even if it
were to do that, you'd still need to add all the domains to the DTB.
Note that I'm saying DT*B* here, because the firmware needs to fill in
those properties after the DTS has been compiled. And since most of
these resources are linked via phandle you actually need to resolve
these first before you can fill in the new properties of this
dynamically created node.
So firmware needs to know exactly what device tree node to look for,
find a corresponding phandle and then put the phandle value in the
simplefb device tree node. And it needs to know intimate details about
the clock provider binding because it needs to add an appropriate
specifier, too.
And then all of a sudden something that was supposed to be simple and
generic needs to know the specifics of some hardware device.
> > Passing in clock information via the device tree already requires a non-
> > trivial amount of code in the firmware. A similar amount of code would
> > be necessary for each type of resource that needs to be kept enabled. In
> > addition to the above some devices may also require resources that have
> > no generic bindings. That just doesn't scale.
>
> [*] The firmware does need to make sure the clocks, regulators, PM domains,
> ... are up and running for the initial video mode, too. So it already needs
> to have this knowledge (unless enabled by SoC reset-state).
Certainly. But not all firmware will use a DTB (or in fact the same DTB
as the kernel) to derive this information from, so it still needs to
inspect the DTB that will be passed to the kernel and painfully extract
information from various nodes and put it into a new node.
But that's not even the issue. You say yourself that the firmware set
everything up itself already. My point is: why should the kernel have to
do everything again, only to come to the conclusion that it doesn't have
to touch hardware at all because it's already in the state that it
should be?
In fact, Linux will at some point set things up from scratch anyway with
a fully-fledged driver. But doing it in simplefb too would be doubly
wasteful.
> > The only reasonable thing for simplefb to do is not deal with any kind
> > of resource at all (except perhaps area that contains the framebuffer
> > memory).
> >
> > So how about instead of requiring resources to be explicitly claimed we
> > introduce something like the below patch? The intention being to give
> > "firmware device" drivers a way of signalling to the clock framework
> > that they need rely on clocks set up by firmware and when they no longer
> > need them. This implements essentially what Mark (CC'ing again on this
> > subthread) suggested earlier in this thread. Basically, it will allow
> > drivers to determine the time when unused clocks are really unused. It
> > will of course only work when used correctly by drivers. For the case of
> > simplefb I'd expect its .probe() implementation to call the new
> > clk_ignore_unused() function and once it has handed over control of the
> > display hardware to the real driver it can call clk_unignore_unused() to
> > signal that all unused clocks that it cares about have now been claimed.
> > This is "reference counted" and can therefore be used by more than a
> > single driver if necessary. Similar functionality could be added for
> > other resource subsystems as needed.
>
> This still won't work for modules, right? Or am I missing something?
> With modules you will never know in advance what will be used and what
> won't be used, so you need to keep all clocks, regulators, PM domains, ...
> up and running?
No. The way this works is that your firmware shim driver, simplefb in
this case, will call clk_ignore_unused() to tell the clock framework
that it uses clocks set up by the firmware, and therefore requests that
no clocks should be considered unused (for now). Later on when the
proper driver has successfully taken over from the shim driver, the shim
driver can unregister itself and call clk_unignore_unused(), which will
drop its "reference" on the unused clocks. When all references have been
dropped the clock framework will then disable all remaining unused
clocks.
In practice this will mean that all unused clocks will remain in their
current state until the shim driver relinquishes control to the proper
OS driver. And if that never happens then we're at least still left with
a working framebuffer.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Geert Uytterhoeven @ 2014-09-29 9:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929085421.GH12506@ulmo>
Hi Thierry,
On Mon, Sep 29, 2014 at 10:54 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Sep 29, 2014 at 10:27:41AM +0200, Geert Uytterhoeven wrote:
>> (CC linux-pm, as PM is the real reason behind disabling unused clocks)
>> (CC gregkh and lkml, for driver core)
>>
>> On Mon, Sep 29, 2014 at 10:06 AM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > If we start extending the binding with board-level details we end up
>> > duplicating the device tree node for the proper video device. Also note
>> > that it won't stop at clocks. Other setups will require regulators to be
>> > listed in this device tree node as well so that they don't get disabled
>> > at late_initcall. And the regulator bindings don't provide a method to
>> > list an arbitrary number of clocks in a single property in the way that
>> > the clocks property works.
>>
>> Then (optional) regulator support needs to be added.
>
> Can you elaborate?
I'm not so familiar with regulators, but I guess it's similar to clocks?
>> > There may be also resets involved. Fortunately the reset framework is
>> > minimalistic enough not to care about asserting all unused resets at
>> > late_initcall. And other things like power domains may also need to be
>> > kept on.
>>
>> Fortunately, unlike clocks, PM domains are first class citizens in the
>> device framework, as they're handled by the driver core.
>> So just adding a power-domains property to DTS will work, without any
>> driver change.
>
> Well, the device driver would also need to call into the PM runtime
> framework to enable all the first class citizen magic. But even if it
> were to do that, you'd still need to add all the domains to the DTB.
Powering up domains can be done solely by the device-specific PM
domain code, without PM runtime. If simplefb is tied to the PM domain
and doesn't do any PM runtime, the domain will stay powered on
(only unused PM domains are powered down via late_initcall).
> Note that I'm saying DT*B* here, because the firmware needs to fill in
> those properties after the DTS has been compiled. And since most of
> these resources are linked via phandle you actually need to resolve
> these first before you can fill in the new properties of this
> dynamically created node.
>
> So firmware needs to know exactly what device tree node to look for,
> find a corresponding phandle and then put the phandle value in the
> simplefb device tree node. And it needs to know intimate details about
> the clock provider binding because it needs to add an appropriate
> specifier, too.
Indeed. Complicated.
> And then all of a sudden something that was supposed to be simple and
> generic needs to know the specifics of some hardware device.
And suddenly we wish we could write a real driver and put the stuff in
the DTS, not DTB...
>> > The only reasonable thing for simplefb to do is not deal with any kind
>> > of resource at all (except perhaps area that contains the framebuffer
>> > memory).
>> >
>> > So how about instead of requiring resources to be explicitly claimed we
>> > introduce something like the below patch? The intention being to give
>> > "firmware device" drivers a way of signalling to the clock framework
>> > that they need rely on clocks set up by firmware and when they no longer
>> > need them. This implements essentially what Mark (CC'ing again on this
>> > subthread) suggested earlier in this thread. Basically, it will allow
>> > drivers to determine the time when unused clocks are really unused. It
>> > will of course only work when used correctly by drivers. For the case of
>> > simplefb I'd expect its .probe() implementation to call the new
>> > clk_ignore_unused() function and once it has handed over control of the
>> > display hardware to the real driver it can call clk_unignore_unused() to
>> > signal that all unused clocks that it cares about have now been claimed.
>> > This is "reference counted" and can therefore be used by more than a
>> > single driver if necessary. Similar functionality could be added for
>> > other resource subsystems as needed.
>>
>> This still won't work for modules, right? Or am I missing something?
>> With modules you will never know in advance what will be used and what
>> won't be used, so you need to keep all clocks, regulators, PM domains, ...
>> up and running?
>
> No. The way this works is that your firmware shim driver, simplefb in
> this case, will call clk_ignore_unused() to tell the clock framework
> that it uses clocks set up by the firmware, and therefore requests that
> no clocks should be considered unused (for now). Later on when the
> proper driver has successfully taken over from the shim driver, the shim
> driver can unregister itself and call clk_unignore_unused(), which will
> drop its "reference" on the unused clocks. When all references have been
> dropped the clock framework will then disable all remaining unused
> clocks.
So the shim must be built-in, not modular.
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
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-09-29 9:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929080637.GB12506@ulmo>
[-- Attachment #1: Type: text/plain, Size: 8240 bytes --]
On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
> On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
> > Quoting Maxime Ripard (2014-09-02 02:25:08)
> > > On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
> > > > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
> > > > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
> > > > > > I would think the memory should still be reserved anyway to make sure
> > > > > > nothing else is writing over it. And it's in the device tree anyway
> > > > > > because the driver needs to know where to put framebuffer content. So
> > > > > > the point I was trying to make is that we can't treat the memory in the
> > > > > > same way as clocks because it needs to be explicitly managed. Whereas
> > > > > > clocks don't. The driver is simply too generic to know what to do with
> > > > > > the clocks.
> > > > >
> > > > > You agreed on the fact that the only thing we need to do with the
> > > > > clocks is claim them. Really, I don't find what's complicated there
> > > > > (or not generic).
> > > >
> > > > That's not what I agreed on. What I said is that the only thing we need
> > > > to do with the clocks is nothing. They are already in the state that
> > > > they need to be.
> > >
> > > Claim was probably a poor choice of words, but still. We have to keep
> > > the clock running, and both the solution you've been giving and this
> > > patch do so in a generic way.
> > >
> > > > > > It doesn't know what frequency they should be running at
> > > > >
> > > > > We don't care about that. Just like we don't care about which
> > > > > frequency is the memory bus running at. It will just run at whatever
> > > > > frequency is appropriate.
> > > >
> > > > Exactly. And you shouldn't have to care about them at all. Firmware has
> > > > already configured the clocks to run at the correct frequencies, and it
> > > > has made sure that they are enabled.
> > > >
> > > > > > or what they're used for
> > > > >
> > > > > And we don't care about that either. You're not interested in what
> > > > > output the framebuffer is setup to use, which is pretty much the same
> > > > > here, this is the same thing here.
> > > >
> > > > That's precisely what I've been saying. The only thing that simplefb
> > > > cares about is the memory it should be using and the format of the
> > > > pixels (and how many of them) it writes into that memory. Everything
> > > > else is assumed to have been set up.
> > > >
> > > > Including clocks.
> > >
> > > We're really discussing in circles here.
> > >
> > > Mike?
> > >
> >
> > -EHIGHLATENCYRESPONSE
> >
> > I forgot about this thread. Sorry.
> >
> > In an attempt to provide the least helpful answer possible, I will stay
> > clear of all of the stuff relating to "how simple should simplefb be"
> > and the related reserved memory discussion.
> >
> > A few times in this thread it is stated that we can't prevent unused
> > clocks from being disabled. That is only partially true.
> >
> > The clock framework DOES provide a flag to prevent UNUSED clocks from
> > being disabled at late_initcall-time by a clock "garbage collector"
> > mechanism. Maxime and others familiar with the clock framework are aware
> > of this.
> >
> > What the framework doesn't do is to allow for a magic flag in DT, in
> > platform_data or elsewhere that says, "don't let me get turned off until
> > the right driver claims me". That would be an external or alternative
> > method for preventing a clock from being disabled. We have a method for
> > preventing clocks from being disabled. It is as follows:
> >
> > struct clk *my_clk = clk_get(...);
> > clk_prepare_enable(my_clk);
> >
> > That is how it should be done. Period.
> >
> > With that said I think that Luc's approach is very sensible. I'm not
> > sure what purpose in the universe DT is supposed to serve if not for
> > _this_exact_case_. We have a nice abstracted driver, usable by many
> > people. The hardware details of how it is hooked up at the board level
> > can all be hidden behind stable DT bindings that everyone already uses.
>
> simplefb doesn't deal at all with hardware details. It simply uses what
> firmware has set up, which is the only reason why it will work for many
> people. What is passed in via its device tree node is the minimum amount
> of information needed to draw something into the framebuffer. Also note
> that the simplefb device tree node is not statically added to a DTS file
> but needs to be dynamically generated by firmware at runtime.
Which makes the whole even simpler, since the firmware already knows
all about which clocks it had to enable.
> If we start extending the binding with board-level details we end up
> duplicating the device tree node for the proper video device. Also note
> that it won't stop at clocks. Other setups will require regulators to be
> listed in this device tree node as well so that they don't get disabled
> at late_initcall. And the regulator bindings don't provide a method to
> list an arbitrary number of clocks in a single property in the way that
> the clocks property works.
>
> There may be also resets involved. Fortunately the reset framework is
> minimalistic enough not to care about asserting all unused resets at
> late_initcall. And other things like power domains may also need to be
> kept on.
>
> Passing in clock information via the device tree already requires a non-
> trivial amount of code in the firmware. A similar amount of code would
> be necessary for each type of resource that needs to be kept enabled. In
> addition to the above some devices may also require resources that have
> no generic bindings. That just doesn't scale.
>
> The only reasonable thing for simplefb to do is not deal with any kind
> of resource at all (except perhaps area that contains the framebuffer
> memory).
You should really read that thread:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/284726.html
It's quite interesting, because you'll see that:
A) Your approach, even on the platform you're working on, doesn't
work. Or at least, isn't reliable.
B) Other maintainers, precisely like Mark, came to the same conclusion
than Mike.
> So how about instead of requiring resources to be explicitly claimed we
> introduce something like the below patch? The intention being to give
> "firmware device" drivers a way of signalling to the clock framework
> that they need rely on clocks set up by firmware and when they no longer
> need them. This implements essentially what Mark (CC'ing again on this
> subthread) suggested earlier in this thread. Basically, it will allow
> drivers to determine the time when unused clocks are really unused. It
> will of course only work when used correctly by drivers. For the case of
> simplefb I'd expect its .probe() implementation to call the new
> clk_ignore_unused() function and once it has handed over control of the
> display hardware to the real driver it can call clk_unignore_unused() to
> signal that all unused clocks that it cares about have now been claimed.
> This is "reference counted" and can therefore be used by more than a
> single driver if necessary. Similar functionality could be added for
> other resource subsystems as needed.
So, just to be clear, instead of doing a generic clk_get and
clk_prepare_enable, you're willing to do a just as much generic
clk_ignore_unused call?
How is that less generic? You know that you are going to call that for
regulator, reset, power domains, just as you would have needed to with
the proper API, unless that with this kind of solution, you would have
to modify *every* framework that might interact with any resource
involved in getting simplefb running?
Plus, speaking more specifically about the clocks, that won't prevent
your clock to be shut down as a side effect of a later clk_disable
call from another driver.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 9:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdU0_GjbOjVE9Vnp703TnsdmkN-nxCxrigCWO7RPMzPRyw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5016 bytes --]
On Mon, Sep 29, 2014 at 11:10:53AM +0200, Geert Uytterhoeven wrote:
> Hi Thierry,
>
> On Mon, Sep 29, 2014 at 10:54 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Sep 29, 2014 at 10:27:41AM +0200, Geert Uytterhoeven wrote:
> >> (CC linux-pm, as PM is the real reason behind disabling unused clocks)
> >> (CC gregkh and lkml, for driver core)
> >>
> >> On Mon, Sep 29, 2014 at 10:06 AM, Thierry Reding
> >> <thierry.reding@gmail.com> wrote:
> >> > If we start extending the binding with board-level details we end up
> >> > duplicating the device tree node for the proper video device. Also note
> >> > that it won't stop at clocks. Other setups will require regulators to be
> >> > listed in this device tree node as well so that they don't get disabled
> >> > at late_initcall. And the regulator bindings don't provide a method to
> >> > list an arbitrary number of clocks in a single property in the way that
> >> > the clocks property works.
> >>
> >> Then (optional) regulator support needs to be added.
> >
> > Can you elaborate?
>
> I'm not so familiar with regulators, but I guess it's similar to clocks?
The bindings are different. Essentially what you use is a *-supply
property per regulator. There is no way to specify more than one
regulator in a single property.
So if you want to keep that generic you have to do crazy things like:
simplefb {
enable-0-supply = <®1>;
enable-1-supply = <®2>;
...
};
I suppose a more generic supplies property could be created to support
this use-case, but I think this kind of proves my point. The only way
that the original proposal is going to work for other resources is if
they follow the same kind of binding. I don't think it makes sense to
introduce such a prerequisite merely because it would make life easy
for some exotic driver with a very specific application.
> > And then all of a sudden something that was supposed to be simple and
> > generic needs to know the specifics of some hardware device.
>
> And suddenly we wish we could write a real driver and put the stuff in
> the DTS, not DTB...
Oh, there's no doubt a real driver would be preferrable. Note that
simplefb is only meant to be a shim to pass a framebuffer from firmware
to kernel. In some cases it can be used with longer lifetime, like for
example if you really want to have graphical output but the real driver
isn't there yet.
Being a shim driver is precisely the reason why I think the binding
shouldn't be extended to cover all possible types of resources. That
should all go into the binding for the real device.
> >> > The only reasonable thing for simplefb to do is not deal with any kind
> >> > of resource at all (except perhaps area that contains the framebuffer
> >> > memory).
> >> >
> >> > So how about instead of requiring resources to be explicitly claimed we
> >> > introduce something like the below patch? The intention being to give
> >> > "firmware device" drivers a way of signalling to the clock framework
> >> > that they need rely on clocks set up by firmware and when they no longer
> >> > need them. This implements essentially what Mark (CC'ing again on this
> >> > subthread) suggested earlier in this thread. Basically, it will allow
> >> > drivers to determine the time when unused clocks are really unused. It
> >> > will of course only work when used correctly by drivers. For the case of
> >> > simplefb I'd expect its .probe() implementation to call the new
> >> > clk_ignore_unused() function and once it has handed over control of the
> >> > display hardware to the real driver it can call clk_unignore_unused() to
> >> > signal that all unused clocks that it cares about have now been claimed.
> >> > This is "reference counted" and can therefore be used by more than a
> >> > single driver if necessary. Similar functionality could be added for
> >> > other resource subsystems as needed.
> >>
> >> This still won't work for modules, right? Or am I missing something?
> >> With modules you will never know in advance what will be used and what
> >> won't be used, so you need to keep all clocks, regulators, PM domains, ...
> >> up and running?
> >
> > No. The way this works is that your firmware shim driver, simplefb in
> > this case, will call clk_ignore_unused() to tell the clock framework
> > that it uses clocks set up by the firmware, and therefore requests that
> > no clocks should be considered unused (for now). Later on when the
> > proper driver has successfully taken over from the shim driver, the shim
> > driver can unregister itself and call clk_unignore_unused(), which will
> > drop its "reference" on the unused clocks. When all references have been
> > dropped the clock framework will then disable all remaining unused
> > clocks.
>
> So the shim must be built-in, not modular.
Correct. Making it a module isn't very useful in my opinion. You'd loose
all the advantages.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Michal Suchanek @ 2014-09-29 9:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929085421.GH12506@ulmo>
On 29 September 2014 10:54, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Sep 29, 2014 at 10:27:41AM +0200, Geert Uytterhoeven wrote:
>> Hi Thierry,
>>
>> (CC linux-pm, as PM is the real reason behind disabling unused clocks)
>> (CC gregkh and lkml, for driver core)
>>
>> On Mon, Sep 29, 2014 at 10:06 AM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
>> >> Quoting Maxime Ripard (2014-09-02 02:25:08)
>> >> > On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
>> >> > > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
>> >> > > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
>> >> > > > > I would think the memory should still be reserved anyway to make sure
>> >> > > > > nothing else is writing over it. And it's in the device tree anyway
>> >> > > > > because the driver needs to know where to put framebuffer content. So
>> >> > > > > the point I was trying to make is that we can't treat the memory in the
>> >> > > > > same way as clocks because it needs to be explicitly managed. Whereas
>> >> > > > > clocks don't. The driver is simply too generic to know what to do with
>> >> > > > > the clocks.
>> >> > > >
>> >> > > > You agreed on the fact that the only thing we need to do with the
>> >> > > > clocks is claim them. Really, I don't find what's complicated there
>> >> > > > (or not generic).
>> >> > >
>> >> > > That's not what I agreed on. What I said is that the only thing we need
>> >> > > to do with the clocks is nothing. They are already in the state that
>> >> > > they need to be.
>> >> >
>> >> > Claim was probably a poor choice of words, but still. We have to keep
>> >> > the clock running, and both the solution you've been giving and this
>> >> > patch do so in a generic way.
>> >> >
>> >> > > > > It doesn't know what frequency they should be running at
>> >> > > >
>> >> > > > We don't care about that. Just like we don't care about which
>> >> > > > frequency is the memory bus running at. It will just run at whatever
>> >> > > > frequency is appropriate.
>> >> > >
>> >> > > Exactly. And you shouldn't have to care about them at all. Firmware has
>> >> > > already configured the clocks to run at the correct frequencies, and it
>> >> > > has made sure that they are enabled.
>> >> > >
>> >> > > > > or what they're used for
>> >> > > >
>> >> > > > And we don't care about that either. You're not interested in what
>> >> > > > output the framebuffer is setup to use, which is pretty much the same
>> >> > > > here, this is the same thing here.
>> >> > >
>> >> > > That's precisely what I've been saying. The only thing that simplefb
>> >> > > cares about is the memory it should be using and the format of the
>> >> > > pixels (and how many of them) it writes into that memory. Everything
>> >> > > else is assumed to have been set up.
>> >> > >
>> >> > > Including clocks.
>> >> >
>> >> > We're really discussing in circles here.
>> >> >
>> >> > Mike?
>> >> >
>> >>
>> >> -EHIGHLATENCYRESPONSE
>> >>
>> >> I forgot about this thread. Sorry.
>> >>
>> >> In an attempt to provide the least helpful answer possible, I will stay
>> >> clear of all of the stuff relating to "how simple should simplefb be"
>> >> and the related reserved memory discussion.
>> >>
>> >> A few times in this thread it is stated that we can't prevent unused
>> >> clocks from being disabled. That is only partially true.
>> >>
>> >> The clock framework DOES provide a flag to prevent UNUSED clocks from
>> >> being disabled at late_initcall-time by a clock "garbage collector"
>> >> mechanism. Maxime and others familiar with the clock framework are aware
>> >> of this.
>> >>
>> >> What the framework doesn't do is to allow for a magic flag in DT, in
>> >> platform_data or elsewhere that says, "don't let me get turned off until
>> >> the right driver claims me". That would be an external or alternative
>> >> method for preventing a clock from being disabled. We have a method for
>> >> preventing clocks from being disabled. It is as follows:
>> >>
>> >> struct clk *my_clk = clk_get(...);
>> >> clk_prepare_enable(my_clk);
>> >>
>> >> That is how it should be done. Period.
>> >>
>> >> With that said I think that Luc's approach is very sensible. I'm not
>> >> sure what purpose in the universe DT is supposed to serve if not for
>> >> _this_exact_case_. We have a nice abstracted driver, usable by many
>> >> people. The hardware details of how it is hooked up at the board level
>> >> can all be hidden behind stable DT bindings that everyone already uses.
>> >
>> > simplefb doesn't deal at all with hardware details. It simply uses what
>> > firmware has set up, which is the only reason why it will work for many
>>
>> It doesn't deal with "hardware details for hardware components for which
>> no driver is available (yet)". That's why the hardware is still in a usable
>> state, after the firmware has set it up.
>>
>> Clocks, regulators, PM domains typically have system-wide implications,
>> and thus need system-wide drivers (in the absence of such drivers,
>> things would work as-is).
>>
>> Note that the driver still requests resources (ioremap the frame buffer),
>> so it needs to know about that tiny piece of hardware detail.
>
> That's not a hardware detail. Or at least it isn't hardware specific. It
> is needed and the same irrespective of display hardware. Clocks, power
> domains, regulators and all those are not always the same.
The framebuffer address, format, etc. is as hardware specific as the
clocks needed to run the crtc, regulators to power the backlight, etc.
You see this from the point of view of a platform that has not clock
driver. Then the platform has no clock information whatsoever, for any
driver. That's platform specific too.
Why do we have to go back to this *again*?
>
>> > There may be also resets involved. Fortunately the reset framework is
>> > minimalistic enough not to care about asserting all unused resets at
>> > late_initcall. And other things like power domains may also need to be
>> > kept on.
>>
>> Fortunately, unlike clocks, PM domains are first class citizens in the
>> device framework, as they're handled by the driver core.
>> So just adding a power-domains property to DTS will work, without any
>> driver change.
>
> Well, the device driver would also need to call into the PM runtime
> framework to enable all the first class citizen magic. But even if it
> were to do that, you'd still need to add all the domains to the DTB.
>
> Note that I'm saying DT*B* here, because the firmware needs to fill in
> those properties after the DTS has been compiled. And since most of
> these resources are linked via phandle you actually need to resolve
> these first before you can fill in the new properties of this
> dynamically created node.
>
> So firmware needs to know exactly what device tree node to look for,
> find a corresponding phandle and then put the phandle value in the
> simplefb device tree node. And it needs to know intimate details about
> the clock provider binding because it needs to add an appropriate
> specifier, too.
>
> And then all of a sudden something that was supposed to be simple and
> generic needs to know the specifics of some hardware device.
Note however that all the specific is in the firmware driver. The
linux simplefb driver only needs to read DT entries which is generic
hardware-neutral thing. It needs to handle clocks on platform that do
have a clock driver, yes. There is no way around that. See below.
>
>> > Passing in clock information via the device tree already requires a non-
>> > trivial amount of code in the firmware. A similar amount of code would
>> > be necessary for each type of resource that needs to be kept enabled. In
>> > addition to the above some devices may also require resources that have
>> > no generic bindings. That just doesn't scale.
>>
>> [*] The firmware does need to make sure the clocks, regulators, PM domains,
>> ... are up and running for the initial video mode, too. So it already needs
>> to have this knowledge (unless enabled by SoC reset-state).
>
> Certainly. But not all firmware will use a DTB (or in fact the same DTB
> as the kernel) to derive this information from, so it still needs to
> inspect the DTB that will be passed to the kernel and painfully extract
> information from various nodes and put it into a new node.
>
> But that's not even the issue. You say yourself that the firmware set
> everything up itself already. My point is: why should the kernel have to
> do everything again, only to come to the conclusion that it doesn't have
> to touch hardware at all because it's already in the state that it
> should be?
It will do nothing. It will just read from DTB what firmware has set
up and inform all the relevant generic frameworks that these resources
are in use. The device-specific driver (if any) will then keep the
resource in question enabled. So all that is this patch doing is to
correct the kernel's resource bookkeeping.
>
> In fact, Linux will at some point set things up from scratch anyway with
> a fully-fledged driver. But doing it in simplefb too would be doubly
> wasteful.
It may or may not. And by having the bookkeeping straight we give that
choice to the user.
>
>> > The only reasonable thing for simplefb to do is not deal with any kind
>> > of resource at all (except perhaps area that contains the framebuffer
>> > memory).
>> >
>> > So how about instead of requiring resources to be explicitly claimed we
>> > introduce something like the below patch? The intention being to give
>> > "firmware device" drivers a way of signalling to the clock framework
>> > that they need rely on clocks set up by firmware and when they no longer
>> > need them. This implements essentially what Mark (CC'ing again on this
>> > subthread) suggested earlier in this thread. Basically, it will allow
>> > drivers to determine the time when unused clocks are really unused. It
>> > will of course only work when used correctly by drivers. For the case of
>> > simplefb I'd expect its .probe() implementation to call the new
>> > clk_ignore_unused() function and once it has handed over control of the
>> > display hardware to the real driver it can call clk_unignore_unused() to
>> > signal that all unused clocks that it cares about have now been claimed.
>> > This is "reference counted" and can therefore be used by more than a
>> > single driver if necessary. Similar functionality could be added for
>> > other resource subsystems as needed.
>>
>> This still won't work for modules, right? Or am I missing something?
>> With modules you will never know in advance what will be used and what
>> won't be used, so you need to keep all clocks, regulators, PM domains, ...
>> up and running?
Simplefb cannot work as module on any platform. Once kernel reclaims
the resources used by simplefb it cannot be started because kernel has
no way to reenable the resources (memory, clocks, etc). Reclaiming the
simplefb memory is currently not supported but once it is added
loading simplefb as module is no-go on any platform.
>
> No. The way this works is that your firmware shim driver, simplefb in
> this case, will call clk_ignore_unused() to tell the clock framework
> that it uses clocks set up by the firmware, and therefore requests that
> no clocks should be considered unused (for now). Later on when the
> proper driver has successfully taken over from the shim driver, the shim
> driver can unregister itself and call clk_unignore_unused(), which will
> drop its "reference" on the unused clocks. When all references have been
> dropped the clock framework will then disable all remaining unused
> clocks.
> In practice this will mean that all unused clocks will remain in their
> current state until the shim driver relinquishes control to the proper
> OS driver. And if that never happens then we're at least still left with
> a working framebuffer.
This does not work. Firstly if you do not have a full driver (because
there is none or because kernel did not find the module, ...) then you
could run with the shim driver and have all clocks managed properly.
This is not possible with clk_ignore_unused.
Secondly clk_ignore_unused does not really mark clock as used. It only
skips disabling them at certain point at kernel startup so that kernel
survives that point and clock problems can be debugged. It is not
meant as an option to be used for running the kernel indefinitely. It
is indeed flawed: if at later point enabling or disabling a clock
results in a parent clock becoming unused it is still disabled. This
can affect many clocks if enabling a clock results in clock
reparenting.
Sure, clk_ignore_unused could be fixed to actually mark all clocks
enabled at boot time as used so that they are never disabled but
that's orthogonal to fixing simplefb so that when it is in use the
clock framework can work normally and can be tested, debugged, and
used for saving power by disabling unused clocks.
So please stop repeating that managing system resources is
system-specific and not fit for simplefb driver. It's been said enough
times, and enough times pointed out that simplefb driver must as any
other driver manage its resources (or more specifically tell the
kernel to manage them) to work properly. And that reading a list of
resources from DT and telling kernel to manage them is not system
specific, only specific to resource frameworks enabled on the platform
in question. That is you may need support for as many resource
frameworks as the platform has if they are ever used for framebuffer.
Thanks
Michal
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 10:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929092301.GC4388@lukather>
[-- Attachment #1: Type: text/plain, Size: 6141 bytes --]
On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
> On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
[...]
> > simplefb doesn't deal at all with hardware details. It simply uses what
> > firmware has set up, which is the only reason why it will work for many
> > people. What is passed in via its device tree node is the minimum amount
> > of information needed to draw something into the framebuffer. Also note
> > that the simplefb device tree node is not statically added to a DTS file
> > but needs to be dynamically generated by firmware at runtime.
>
> Which makes the whole even simpler, since the firmware already knows
> all about which clocks it had to enable.
It makes things very complicated in the firmware because it now needs to
be able to generate DTB content that we would otherwise be able to do
much easier with a text editor.
> > If we start extending the binding with board-level details we end up
> > duplicating the device tree node for the proper video device. Also note
> > that it won't stop at clocks. Other setups will require regulators to be
> > listed in this device tree node as well so that they don't get disabled
> > at late_initcall. And the regulator bindings don't provide a method to
> > list an arbitrary number of clocks in a single property in the way that
> > the clocks property works.
> >
> > There may be also resets involved. Fortunately the reset framework is
> > minimalistic enough not to care about asserting all unused resets at
> > late_initcall. And other things like power domains may also need to be
> > kept on.
> >
> > Passing in clock information via the device tree already requires a non-
> > trivial amount of code in the firmware. A similar amount of code would
> > be necessary for each type of resource that needs to be kept enabled. In
> > addition to the above some devices may also require resources that have
> > no generic bindings. That just doesn't scale.
> >
> > The only reasonable thing for simplefb to do is not deal with any kind
> > of resource at all (except perhaps area that contains the framebuffer
> > memory).
>
> You should really read that thread:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/284726.html
>
> It's quite interesting, because you'll see that:
> A) Your approach, even on the platform you're working on, doesn't
> work. Or at least, isn't reliable.
What platform exactly do you think I'm working on? Why do you think what
I proposed isn't going to work or be reliable? I don't see any arguments
in the thread that would imply that.
> B) Other maintainers, precisely like Mark, came to the same conclusion
> than Mike.
Well, and others didn't.
Also I think if you read that thread and look at my proposal it matches
exactly what was discussed as one of the solutions at one point in the
thread.
> > So how about instead of requiring resources to be explicitly claimed we
> > introduce something like the below patch? The intention being to give
> > "firmware device" drivers a way of signalling to the clock framework
> > that they need rely on clocks set up by firmware and when they no longer
> > need them. This implements essentially what Mark (CC'ing again on this
> > subthread) suggested earlier in this thread. Basically, it will allow
> > drivers to determine the time when unused clocks are really unused. It
> > will of course only work when used correctly by drivers. For the case of
> > simplefb I'd expect its .probe() implementation to call the new
> > clk_ignore_unused() function and once it has handed over control of the
> > display hardware to the real driver it can call clk_unignore_unused() to
> > signal that all unused clocks that it cares about have now been claimed.
> > This is "reference counted" and can therefore be used by more than a
> > single driver if necessary. Similar functionality could be added for
> > other resource subsystems as needed.
>
> So, just to be clear, instead of doing a generic clk_get and
> clk_prepare_enable, you're willing to do a just as much generic
> clk_ignore_unused call?
Yes.
> How is that less generic?
It's more generic. That's the whole point.
The difference is that with the solution I proposed we don't have to
keep track of all the resources. We know that firmware has set them up
and we know that a real driver will properly take them over at some
point, so duplicating what the real driver does within the simplefb
driver is just that, duplication. We don't allow duplication anywhere
else in the kernel, why should simplefb be an exception?
> You know that you are going to call that for regulator, reset, power
> domains, just as you would have needed to with the proper API, unless
> that with this kind of solution, you would have to modify *every*
> framework that might interact with any resource involved in getting
> simplefb running?
We have to add handling for every kind of resource either way. Also if
this evolves into a common pattern we can easily wrap it up in a single
function call.
> Plus, speaking more specifically about the clocks, that won't prevent
> your clock to be shut down as a side effect of a later clk_disable
> call from another driver.
If we need to prevent that, then that's something that could be fixed,
too. But both this and the other thread at least agree on the fact that
simplefb is a shim driver that's going to be replaced by something real
at some point, so hopefully concurrent users aren't the problem because
that would cause the real driver to break too.
Also note that if some other driver could call clk_disable() it could
just as easily call clk_set_rate() and break simplefb.
Furthermore isn't it a bug for a driver to call clk_disable() before a
preceding clk_enable()? There are patches being worked on that will
enable per-user clocks and as I understand it they will specifically
disallow drivers to disable the hardware clock if other drivers are
still keeping them on via their own referenc.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Geert Uytterhoeven @ 2014-09-29 10:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929101805.GB26008@ulmo>
Hi Thierry,
On Mon, Sep 29, 2014 at 12:18 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>> How is that less generic?
>
> It's more generic. That's the whole point.
>
> The difference is that with the solution I proposed we don't have to
> keep track of all the resources. We know that firmware has set them up
> and we know that a real driver will properly take them over at some
> point, so duplicating what the real driver does within the simplefb
> driver is just that, duplication. We don't allow duplication anywhere
> else in the kernel, why should simplefb be an exception?
>
>> You know that you are going to call that for regulator, reset, power
>> domains, just as you would have needed to with the proper API, unless
>> that with this kind of solution, you would have to modify *every*
>> framework that might interact with any resource involved in getting
>> simplefb running?
>
> We have to add handling for every kind of resource either way. Also if
> this evolves into a common pattern we can easily wrap it up in a single
> function call.
disable_all_power_management(), as this is not limited to clocks.
>> Plus, speaking more specifically about the clocks, that won't prevent
>> your clock to be shut down as a side effect of a later clk_disable
>> call from another driver.
> Furthermore isn't it a bug for a driver to call clk_disable() before a
> preceding clk_enable()? There are patches being worked on that will
> enable per-user clocks and as I understand it they will specifically
> disallow drivers to disable the hardware clock if other drivers are
> still keeping them on via their own referenc.
Calling clk_disable() preceding clk_enable() is a bug.
Calling clk_disable() after clk_enable() will disable the clock (and
its parents)
if the clock subsystem thinks there are no other users, which is what will
happen here.
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
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 10:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOMqctThLAX7UfO+ogeKkgBwsvBywUi598DcbnpgTkkcww1CZA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 13787 bytes --]
On Mon, Sep 29, 2014 at 11:44:19AM +0200, Michal Suchanek wrote:
> On 29 September 2014 10:54, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Mon, Sep 29, 2014 at 10:27:41AM +0200, Geert Uytterhoeven wrote:
> >> Hi Thierry,
> >>
> >> (CC linux-pm, as PM is the real reason behind disabling unused clocks)
> >> (CC gregkh and lkml, for driver core)
> >>
> >> On Mon, Sep 29, 2014 at 10:06 AM, Thierry Reding
> >> <thierry.reding@gmail.com> wrote:
> >> > On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
> >> >> Quoting Maxime Ripard (2014-09-02 02:25:08)
> >> >> > On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
> >> >> > > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
> >> >> > > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
> >> >> > > > > I would think the memory should still be reserved anyway to make sure
> >> >> > > > > nothing else is writing over it. And it's in the device tree anyway
> >> >> > > > > because the driver needs to know where to put framebuffer content. So
> >> >> > > > > the point I was trying to make is that we can't treat the memory in the
> >> >> > > > > same way as clocks because it needs to be explicitly managed. Whereas
> >> >> > > > > clocks don't. The driver is simply too generic to know what to do with
> >> >> > > > > the clocks.
> >> >> > > >
> >> >> > > > You agreed on the fact that the only thing we need to do with the
> >> >> > > > clocks is claim them. Really, I don't find what's complicated there
> >> >> > > > (or not generic).
> >> >> > >
> >> >> > > That's not what I agreed on. What I said is that the only thing we need
> >> >> > > to do with the clocks is nothing. They are already in the state that
> >> >> > > they need to be.
> >> >> >
> >> >> > Claim was probably a poor choice of words, but still. We have to keep
> >> >> > the clock running, and both the solution you've been giving and this
> >> >> > patch do so in a generic way.
> >> >> >
> >> >> > > > > It doesn't know what frequency they should be running at
> >> >> > > >
> >> >> > > > We don't care about that. Just like we don't care about which
> >> >> > > > frequency is the memory bus running at. It will just run at whatever
> >> >> > > > frequency is appropriate.
> >> >> > >
> >> >> > > Exactly. And you shouldn't have to care about them at all. Firmware has
> >> >> > > already configured the clocks to run at the correct frequencies, and it
> >> >> > > has made sure that they are enabled.
> >> >> > >
> >> >> > > > > or what they're used for
> >> >> > > >
> >> >> > > > And we don't care about that either. You're not interested in what
> >> >> > > > output the framebuffer is setup to use, which is pretty much the same
> >> >> > > > here, this is the same thing here.
> >> >> > >
> >> >> > > That's precisely what I've been saying. The only thing that simplefb
> >> >> > > cares about is the memory it should be using and the format of the
> >> >> > > pixels (and how many of them) it writes into that memory. Everything
> >> >> > > else is assumed to have been set up.
> >> >> > >
> >> >> > > Including clocks.
> >> >> >
> >> >> > We're really discussing in circles here.
> >> >> >
> >> >> > Mike?
> >> >> >
> >> >>
> >> >> -EHIGHLATENCYRESPONSE
> >> >>
> >> >> I forgot about this thread. Sorry.
> >> >>
> >> >> In an attempt to provide the least helpful answer possible, I will stay
> >> >> clear of all of the stuff relating to "how simple should simplefb be"
> >> >> and the related reserved memory discussion.
> >> >>
> >> >> A few times in this thread it is stated that we can't prevent unused
> >> >> clocks from being disabled. That is only partially true.
> >> >>
> >> >> The clock framework DOES provide a flag to prevent UNUSED clocks from
> >> >> being disabled at late_initcall-time by a clock "garbage collector"
> >> >> mechanism. Maxime and others familiar with the clock framework are aware
> >> >> of this.
> >> >>
> >> >> What the framework doesn't do is to allow for a magic flag in DT, in
> >> >> platform_data or elsewhere that says, "don't let me get turned off until
> >> >> the right driver claims me". That would be an external or alternative
> >> >> method for preventing a clock from being disabled. We have a method for
> >> >> preventing clocks from being disabled. It is as follows:
> >> >>
> >> >> struct clk *my_clk = clk_get(...);
> >> >> clk_prepare_enable(my_clk);
> >> >>
> >> >> That is how it should be done. Period.
> >> >>
> >> >> With that said I think that Luc's approach is very sensible. I'm not
> >> >> sure what purpose in the universe DT is supposed to serve if not for
> >> >> _this_exact_case_. We have a nice abstracted driver, usable by many
> >> >> people. The hardware details of how it is hooked up at the board level
> >> >> can all be hidden behind stable DT bindings that everyone already uses.
> >> >
> >> > simplefb doesn't deal at all with hardware details. It simply uses what
> >> > firmware has set up, which is the only reason why it will work for many
> >>
> >> It doesn't deal with "hardware details for hardware components for which
> >> no driver is available (yet)". That's why the hardware is still in a usable
> >> state, after the firmware has set it up.
> >>
> >> Clocks, regulators, PM domains typically have system-wide implications,
> >> and thus need system-wide drivers (in the absence of such drivers,
> >> things would work as-is).
> >>
> >> Note that the driver still requests resources (ioremap the frame buffer),
> >> so it needs to know about that tiny piece of hardware detail.
> >
> > That's not a hardware detail. Or at least it isn't hardware specific. It
> > is needed and the same irrespective of display hardware. Clocks, power
> > domains, regulators and all those are not always the same.
>
> The framebuffer address, format, etc. is as hardware specific as the
> clocks needed to run the crtc, regulators to power the backlight, etc.
Framebuffer address and format are not hardware specific. Every display
driver requires them.
> You see this from the point of view of a platform that has not clock
> driver.
Wrong. What platform's point of view do you think I look at this from?
> Then the platform has no clock information whatsoever, for any
> driver. That's platform specific too.
>
> Why do we have to go back to this *again*?
What exactly are we going back to again?
> >> > There may be also resets involved. Fortunately the reset framework is
> >> > minimalistic enough not to care about asserting all unused resets at
> >> > late_initcall. And other things like power domains may also need to be
> >> > kept on.
> >>
> >> Fortunately, unlike clocks, PM domains are first class citizens in the
> >> device framework, as they're handled by the driver core.
> >> So just adding a power-domains property to DTS will work, without any
> >> driver change.
> >
> > Well, the device driver would also need to call into the PM runtime
> > framework to enable all the first class citizen magic. But even if it
> > were to do that, you'd still need to add all the domains to the DTB.
> >
> > Note that I'm saying DT*B* here, because the firmware needs to fill in
> > those properties after the DTS has been compiled. And since most of
> > these resources are linked via phandle you actually need to resolve
> > these first before you can fill in the new properties of this
> > dynamically created node.
> >
> > So firmware needs to know exactly what device tree node to look for,
> > find a corresponding phandle and then put the phandle value in the
> > simplefb device tree node. And it needs to know intimate details about
> > the clock provider binding because it needs to add an appropriate
> > specifier, too.
> >
> > And then all of a sudden something that was supposed to be simple and
> > generic needs to know the specifics of some hardware device.
>
> Note however that all the specific is in the firmware driver. The
> linux simplefb driver only needs to read DT entries which is generic
> hardware-neutral thing.
No. The DT entries are very hardware-specific. That's precisely the
reason why I think it's wrong to put this into the simplefb node,
because simplefb is an abstraction of the real hardware underneath.
> >> > Passing in clock information via the device tree already requires a non-
> >> > trivial amount of code in the firmware. A similar amount of code would
> >> > be necessary for each type of resource that needs to be kept enabled. In
> >> > addition to the above some devices may also require resources that have
> >> > no generic bindings. That just doesn't scale.
> >>
> >> [*] The firmware does need to make sure the clocks, regulators, PM domains,
> >> ... are up and running for the initial video mode, too. So it already needs
> >> to have this knowledge (unless enabled by SoC reset-state).
> >
> > Certainly. But not all firmware will use a DTB (or in fact the same DTB
> > as the kernel) to derive this information from, so it still needs to
> > inspect the DTB that will be passed to the kernel and painfully extract
> > information from various nodes and put it into a new node.
> >
> > But that's not even the issue. You say yourself that the firmware set
> > everything up itself already. My point is: why should the kernel have to
> > do everything again, only to come to the conclusion that it doesn't have
> > to touch hardware at all because it's already in the state that it
> > should be?
>
> It will do nothing. It will just read from DTB what firmware has set
> up and inform all the relevant generic frameworks that these resources
> are in use. The device-specific driver (if any) will then keep the
> resource in question enabled. So all that is this patch doing is to
> correct the kernel's resource bookkeeping.
It's the device-specific driver that should be doing the book-keeping.
simplefb is only a stop-gap until a proper driver has been loaded. It
should never be considered a fully-functional driver, because it does
not know anything about the display hardware.
> > In fact, Linux will at some point set things up from scratch anyway with
> > a fully-fledged driver. But doing it in simplefb too would be doubly
> > wasteful.
>
> It may or may not. And by having the bookkeeping straight we give that
> choice to the user.
What I'm proposing isn't really all that different. All it does is
prevent resources from being considered unused by default if a driver
that we know relies on an unspecified set of resources is active.
> > No. The way this works is that your firmware shim driver, simplefb in
> > this case, will call clk_ignore_unused() to tell the clock framework
> > that it uses clocks set up by the firmware, and therefore requests that
> > no clocks should be considered unused (for now). Later on when the
> > proper driver has successfully taken over from the shim driver, the shim
> > driver can unregister itself and call clk_unignore_unused(), which will
> > drop its "reference" on the unused clocks. When all references have been
> > dropped the clock framework will then disable all remaining unused
> > clocks.
>
> > In practice this will mean that all unused clocks will remain in their
> > current state until the shim driver relinquishes control to the proper
> > OS driver. And if that never happens then we're at least still left with
> > a working framebuffer.
>
> This does not work. Firstly if you do not have a full driver (because
> there is none or because kernel did not find the module, ...) then you
> could run with the shim driver and have all clocks managed properly.
> This is not possible with clk_ignore_unused.
Huh? Why not?
> Secondly clk_ignore_unused does not really mark clock as used.
Right. If that were the case it'd be called clk_use_unused() or similar.
> It only
> skips disabling them at certain point at kernel startup so that kernel
> survives that point and clock problems can be debugged. It is not
> meant as an option to be used for running the kernel indefinitely. It
> is indeed flawed: if at later point enabling or disabling a clock
> results in a parent clock becoming unused it is still disabled. This
> can affect many clocks if enabling a clock results in clock
> reparenting.
>
> Sure, clk_ignore_unused could be fixed to actually mark all clocks
> enabled at boot time as used so that they are never disabled but
> that's orthogonal to fixing simplefb so that when it is in use the
> clock framework can work normally and can be tested, debugged, and
> used for saving power by disabling unused clocks.
Look, nobody's claiming that using the clk_ignore_unused command-line
argument is a good long-term solution. And it doesn't have to. Once you
have a proper display driver for your platform the problem goes away
entirely.
That is unless you also want to use simplefb for early boot and seamless
transition between firmware and kernel, in which case we'll return to
this discussion.
> So please stop repeating that managing system resources is
> system-specific and not fit for simplefb driver. It's been said enough
> times, and enough times pointed out that simplefb driver must as any
> other driver manage its resources (or more specifically tell the
> kernel to manage them) to work properly.
simplefb isn't anything like any other driver. If you want a proper
driver, go write a DRM/KMS driver and we can all stop having this
discussion. That's the right thing to do.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-09-29 10:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdU4u7K5NUk+p2iT-Sc_PuOjOT2FpFBj6TQMm2ThOuVVqQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2492 bytes --]
On Mon, Sep 29, 2014 at 12:35:17PM +0200, Geert Uytterhoeven wrote:
> Hi Thierry,
>
> On Mon, Sep 29, 2014 at 12:18 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> >> How is that less generic?
> >
> > It's more generic. That's the whole point.
> >
> > The difference is that with the solution I proposed we don't have to
> > keep track of all the resources. We know that firmware has set them up
> > and we know that a real driver will properly take them over at some
> > point, so duplicating what the real driver does within the simplefb
> > driver is just that, duplication. We don't allow duplication anywhere
> > else in the kernel, why should simplefb be an exception?
> >
> >> You know that you are going to call that for regulator, reset, power
> >> domains, just as you would have needed to with the proper API, unless
> >> that with this kind of solution, you would have to modify *every*
> >> framework that might interact with any resource involved in getting
> >> simplefb running?
> >
> > We have to add handling for every kind of resource either way. Also if
> > this evolves into a common pattern we can easily wrap it up in a single
> > function call.
>
> disable_all_power_management(), as this is not limited to clocks.
Right. But it isn't all power management either. It just shouldn't turn
everything unused off. Clocks, regulators, power domains and so on which
are used can very well be power managed.
> >> Plus, speaking more specifically about the clocks, that won't prevent
> >> your clock to be shut down as a side effect of a later clk_disable
> >> call from another driver.
>
> > Furthermore isn't it a bug for a driver to call clk_disable() before a
> > preceding clk_enable()? There are patches being worked on that will
> > enable per-user clocks and as I understand it they will specifically
> > disallow drivers to disable the hardware clock if other drivers are
> > still keeping them on via their own referenc.
>
> Calling clk_disable() preceding clk_enable() is a bug.
>
> Calling clk_disable() after clk_enable() will disable the clock (and
> its parents)
> if the clock subsystem thinks there are no other users, which is what will
> happen here.
Right. I'm not sure this is really applicable to this situation, though.
Either way, if there are other users of a clock then they will just as
likely want to modify the rate at which point simplefb will break just
as badly.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Julian Calaby @ 2014-09-29 11:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929101805.GB26008@ulmo>
Hi Thierry,
On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
>> On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
> [...]
>> > simplefb doesn't deal at all with hardware details. It simply uses what
>> > firmware has set up, which is the only reason why it will work for many
>> > people. What is passed in via its device tree node is the minimum amount
>> > of information needed to draw something into the framebuffer. Also note
>> > that the simplefb device tree node is not statically added to a DTS file
>> > but needs to be dynamically generated by firmware at runtime.
>>
>> Which makes the whole even simpler, since the firmware already knows
>> all about which clocks it had to enable.
>
> It makes things very complicated in the firmware because it now needs to
> be able to generate DTB content that we would otherwise be able to do
> much easier with a text editor.
As far as the kernel is concerned, this is a solved problem.
Firmware is going to be doing some dark magic to set up the hardware
to be a dumb frame buffer and some other stuff to add the simplefb
device node - so by this point, adding the clocks (or whatever)
required by the hardware should be fairly uncomplicated - the firmware
already knows the hardware intimately. As for the actual device tree
manipulations, U-boot (or whatever) will probably just grow some
helper functions to make this simple.
Alternatively, it could simply add the relevant data to an existing
device node and munge it's compatible property so simplefb picks it
up.
>> > If we start extending the binding with board-level details we end up
>> > duplicating the device tree node for the proper video device. Also note
>> > that it won't stop at clocks. Other setups will require regulators to be
>> > listed in this device tree node as well so that they don't get disabled
>> > at late_initcall. And the regulator bindings don't provide a method to
>> > list an arbitrary number of clocks in a single property in the way that
>> > the clocks property works.
>> >
>> > There may be also resets involved. Fortunately the reset framework is
>> > minimalistic enough not to care about asserting all unused resets at
>> > late_initcall. And other things like power domains may also need to be
>> > kept on.
>> >
>> > Passing in clock information via the device tree already requires a non-
>> > trivial amount of code in the firmware. A similar amount of code would
>> > be necessary for each type of resource that needs to be kept enabled. In
>> > addition to the above some devices may also require resources that have
>> > no generic bindings. That just doesn't scale.
>> >
>> > The only reasonable thing for simplefb to do is not deal with any kind
>> > of resource at all (except perhaps area that contains the framebuffer
>> > memory).
>>
>> You should really read that thread:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/284726.html
>>
>> It's quite interesting, because you'll see that:
>> A) Your approach, even on the platform you're working on, doesn't
>> work. Or at least, isn't reliable.
>
> What platform exactly do you think I'm working on? Why do you think what
> I proposed isn't going to work or be reliable? I don't see any arguments
> in the thread that would imply that.
>
>> B) Other maintainers, precisely like Mark, came to the same conclusion
>> than Mike.
>
> Well, and others didn't.
>
> Also I think if you read that thread and look at my proposal it matches
> exactly what was discussed as one of the solutions at one point in the
> thread.
>
>> > So how about instead of requiring resources to be explicitly claimed we
>> > introduce something like the below patch? The intention being to give
>> > "firmware device" drivers a way of signalling to the clock framework
>> > that they need rely on clocks set up by firmware and when they no longer
>> > need them. This implements essentially what Mark (CC'ing again on this
>> > subthread) suggested earlier in this thread. Basically, it will allow
>> > drivers to determine the time when unused clocks are really unused. It
>> > will of course only work when used correctly by drivers. For the case of
>> > simplefb I'd expect its .probe() implementation to call the new
>> > clk_ignore_unused() function and once it has handed over control of the
>> > display hardware to the real driver it can call clk_unignore_unused() to
>> > signal that all unused clocks that it cares about have now been claimed.
>> > This is "reference counted" and can therefore be used by more than a
>> > single driver if necessary. Similar functionality could be added for
>> > other resource subsystems as needed.
>>
>> So, just to be clear, instead of doing a generic clk_get and
>> clk_prepare_enable, you're willing to do a just as much generic
>> clk_ignore_unused call?
>
> Yes.
>
>> How is that less generic?
>
> It's more generic. That's the whole point.
>
> The difference is that with the solution I proposed we don't have to
> keep track of all the resources. We know that firmware has set them up
> and we know that a real driver will properly take them over at some
> point, so duplicating what the real driver does within the simplefb
> driver is just that, duplication. We don't allow duplication anywhere
> else in the kernel, why should simplefb be an exception?
The various subsystems could just grow some accessor functions (where
required) which, with devm, should be as simple as ~10 lines per
subsystem. With ~5 subsystems this is ~50 lines of code; Or we can
disable the automatic disabling of resources and put the system in a
potentially unstable state; Or each subarch could use some shim driver
or some arch code to grab the clocks for simplefb, and we end up with
each subarch having it's own, slightly different version of what is
essentially the same code; Or we have to have every single KMS driver
built in and bloat the kernel.
With ~50 lines of generic code added to simplefb, distros get their
slim multi-subarch kernels, your driver is still generic, power
management works and users get pretty graphics from bootloader to
desktop.
>> You know that you are going to call that for regulator, reset, power
>> domains, just as you would have needed to with the proper API, unless
>> that with this kind of solution, you would have to modify *every*
>> framework that might interact with any resource involved in getting
>> simplefb running?
>
> We have to add handling for every kind of resource either way. Also if
> this evolves into a common pattern we can easily wrap it up in a single
> function call.
>
>> Plus, speaking more specifically about the clocks, that won't prevent
>> your clock to be shut down as a side effect of a later clk_disable
>> call from another driver.
>
> If we need to prevent that, then that's something that could be fixed,
> too. But both this and the other thread at least agree on the fact that
> simplefb is a shim driver that's going to be replaced by something real
> at some point, so hopefully concurrent users aren't the problem because
> that would cause the real driver to break too.
>
> Also note that if some other driver could call clk_disable() it could
> just as easily call clk_set_rate() and break simplefb.
If simplefb has called clk_get() then nobody can disable the clocks
it's depending on.
> Furthermore isn't it a bug for a driver to call clk_disable() before a
> preceding clk_enable()? There are patches being worked on that will
> enable per-user clocks and as I understand it they will specifically
> disallow drivers to disable the hardware clock if other drivers are
> still keeping them on via their own referenc.
>
> Thierry
Thanks,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Geert Uytterhoeven @ 2014-09-29 11:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929104454.GD26008@ulmo>
Hi Thierry,
On Mon, Sep 29, 2014 at 12:44 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>> >> You know that you are going to call that for regulator, reset, power
>> >> domains, just as you would have needed to with the proper API, unless
>> >> that with this kind of solution, you would have to modify *every*
>> >> framework that might interact with any resource involved in getting
>> >> simplefb running?
>> >
>> > We have to add handling for every kind of resource either way. Also if
>> > this evolves into a common pattern we can easily wrap it up in a single
>> > function call.
>>
>> disable_all_power_management(), as this is not limited to clocks.
>
> Right. But it isn't all power management either. It just shouldn't turn
> everything unused off. Clocks, regulators, power domains and so on which
> are used can very well be power managed.
No they can't, as the clock/regulator/PM domain core cannot know if any
of the used ones are also used by a shim driver like simplefb.
Clocks and regulators may be shared. PM domains can contain multiple
hardware blocks. Without more knowledge, the only safe thing is not
disabling anything.
>> >> Plus, speaking more specifically about the clocks, that won't prevent
>> >> your clock to be shut down as a side effect of a later clk_disable
>> >> call from another driver.
>>
>> > Furthermore isn't it a bug for a driver to call clk_disable() before a
>> > preceding clk_enable()? There are patches being worked on that will
>> > enable per-user clocks and as I understand it they will specifically
>> > disallow drivers to disable the hardware clock if other drivers are
>> > still keeping them on via their own referenc.
>>
>> Calling clk_disable() preceding clk_enable() is a bug.
>>
>> Calling clk_disable() after clk_enable() will disable the clock (and
>> its parents)
>> if the clock subsystem thinks there are no other users, which is what will
>> happen here.
>
> Right. I'm not sure this is really applicable to this situation, though.
Yes it is: if all users of a clock/regulator/PM domain are gone, it will
be disabled. Bad luck for simplefb still needing them.
> Either way, if there are other users of a clock then they will just as
> likely want to modify the rate at which point simplefb will break just
> as badly.
BTW, this can also happen for clocks that are properly used.
I guess the clock core code does some arbitration to handle such cases.
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
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-09-29 11:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929104454.GD26008@ulmo>
[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]
On Mon, Sep 29, 2014 at 12:44:57PM +0200, Thierry Reding wrote:
> > >> Plus, speaking more specifically about the clocks, that won't prevent
> > >> your clock to be shut down as a side effect of a later clk_disable
> > >> call from another driver.
> >
> > > Furthermore isn't it a bug for a driver to call clk_disable() before a
> > > preceding clk_enable()? There are patches being worked on that will
> > > enable per-user clocks and as I understand it they will specifically
> > > disallow drivers to disable the hardware clock if other drivers are
> > > still keeping them on via their own referenc.
> >
> > Calling clk_disable() preceding clk_enable() is a bug.
> >
> > Calling clk_disable() after clk_enable() will disable the clock (and
> > its parents)
> > if the clock subsystem thinks there are no other users, which is what will
> > happen here.
>
> Right. I'm not sure this is really applicable to this situation, though.
It's actually very easy to do. Have a driver that probes, enables its
clock, fails to probe for any reason, call clk_disable in its exit
path. If there's no other user at that time of this particular clock
tree, it will be shut down. Bam. You just lost your framebuffer.
Really, it's just that simple, and relying on the fact that some other
user of the same clock tree will always be their is beyond fragile.
> Either way, if there are other users of a clock then they will just as
> likely want to modify the rate at which point simplefb will break just
> as badly.
And this can be handled just as well. Register a clock notifier,
refuse any rate change, done. But of course, that would require having
a clock handle.
Now, how would *you* prevent such a change?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-09-29 11:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140929101805.GB26008@ulmo>
[-- Attachment #1: Type: text/plain, Size: 7829 bytes --]
On Mon, Sep 29, 2014 at 12:18:08PM +0200, Thierry Reding wrote:
> On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
> > On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
> [...]
> > > simplefb doesn't deal at all with hardware details. It simply uses what
> > > firmware has set up, which is the only reason why it will work for many
> > > people. What is passed in via its device tree node is the minimum amount
> > > of information needed to draw something into the framebuffer. Also note
> > > that the simplefb device tree node is not statically added to a DTS file
> > > but needs to be dynamically generated by firmware at runtime.
> >
> > Which makes the whole even simpler, since the firmware already knows
> > all about which clocks it had to enable.
>
> It makes things very complicated in the firmware because it now needs to
> be able to generate DTB content that we would otherwise be able to do
> much easier with a text editor.
Didn't you just say that it was dynamically generated at runtime? So
we can just ignore the "text editor" case.
> > > If we start extending the binding with board-level details we end up
> > > duplicating the device tree node for the proper video device. Also note
> > > that it won't stop at clocks. Other setups will require regulators to be
> > > listed in this device tree node as well so that they don't get disabled
> > > at late_initcall. And the regulator bindings don't provide a method to
> > > list an arbitrary number of clocks in a single property in the way that
> > > the clocks property works.
> > >
> > > There may be also resets involved. Fortunately the reset framework is
> > > minimalistic enough not to care about asserting all unused resets at
> > > late_initcall. And other things like power domains may also need to be
> > > kept on.
> > >
> > > Passing in clock information via the device tree already requires a non-
> > > trivial amount of code in the firmware. A similar amount of code would
> > > be necessary for each type of resource that needs to be kept enabled. In
> > > addition to the above some devices may also require resources that have
> > > no generic bindings. That just doesn't scale.
> > >
> > > The only reasonable thing for simplefb to do is not deal with any kind
> > > of resource at all (except perhaps area that contains the framebuffer
> > > memory).
> >
> > You should really read that thread:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/284726.html
> >
> > It's quite interesting, because you'll see that:
> > A) Your approach, even on the platform you're working on, doesn't
> > work. Or at least, isn't reliable.
>
> What platform exactly do you think I'm working on?
My bad, I thought it was a tegra SoC in there. I should have read more
carefully obviously.
> Why do you think what I proposed isn't going to work or be reliable?
> I don't see any arguments in the thread that would imply that.
The fact that it broke in the first place?
> > B) Other maintainers, precisely like Mark, came to the same conclusion
> > than Mike.
>
> Well, and others didn't.
We've been talking about both clocks and regulators up to now. I can
see Mike and Mark both suggesting to use the usual clocks and
regulators APIs, either in that thread or the one I pointed out.
> Also I think if you read that thread and look at my proposal it matches
> exactly what was discussed as one of the solutions at one point in the
> thread.
I've seen it, and replied to that already.
> > > So how about instead of requiring resources to be explicitly claimed we
> > > introduce something like the below patch? The intention being to give
> > > "firmware device" drivers a way of signalling to the clock framework
> > > that they need rely on clocks set up by firmware and when they no longer
> > > need them. This implements essentially what Mark (CC'ing again on this
> > > subthread) suggested earlier in this thread. Basically, it will allow
> > > drivers to determine the time when unused clocks are really unused. It
> > > will of course only work when used correctly by drivers. For the case of
> > > simplefb I'd expect its .probe() implementation to call the new
> > > clk_ignore_unused() function and once it has handed over control of the
> > > display hardware to the real driver it can call clk_unignore_unused() to
> > > signal that all unused clocks that it cares about have now been claimed.
> > > This is "reference counted" and can therefore be used by more than a
> > > single driver if necessary. Similar functionality could be added for
> > > other resource subsystems as needed.
> >
> > So, just to be clear, instead of doing a generic clk_get and
> > clk_prepare_enable, you're willing to do a just as much generic
> > clk_ignore_unused call?
>
> Yes.
>
> > How is that less generic?
>
> It's more generic. That's the whole point.
>
> The difference is that with the solution I proposed we don't have to
> keep track of all the resources. We know that firmware has set them up
> and we know that a real driver will properly take them over at some
> point
You keep saying that... and you know that you can't make this
assumption.
> so duplicating what the real driver does within the simplefb driver
> is just that, duplication. We don't allow duplication anywhere else
> in the kernel, why should simplefb be an exception?
Oh come on. Since when a clk_prepare_enable call is duplication?
If so, then we really have a duplication issue, and it's not just for
the clock API.
> > You know that you are going to call that for regulator, reset, power
> > domains, just as you would have needed to with the proper API, unless
> > that with this kind of solution, you would have to modify *every*
> > framework that might interact with any resource involved in getting
> > simplefb running?
>
> We have to add handling for every kind of resource either way. Also if
> this evolves into a common pattern we can easily wrap it up in a single
> function call.
Unless that in one case, we already have everything needed to handle
everything properly, and in another, you keep hacking more and more
into the involved frameworks.
> > Plus, speaking more specifically about the clocks, that won't prevent
> > your clock to be shut down as a side effect of a later clk_disable
> > call from another driver.
>
> If we need to prevent that, then that's something that could be fixed,
> too.
See, you keep hacking it more...
> But both this and the other thread at least agree on the fact that
> simplefb is a shim driver that's going to be replaced by something real
> at some point
I think our definition of "at some point" diverges. Yours seem to be
"at some point during the boot process", which might not even happen
in some platforms (or at least in the foreseeable kernel releases).
> so hopefully concurrent users aren't the problem because that would
> cause the real driver to break too.
>
> Also note that if some other driver could call clk_disable() it could
> just as easily call clk_set_rate() and break simplefb.
This is not true, see my other reply.
> Furthermore isn't it a bug for a driver to call clk_disable() before a
> preceding clk_enable()? There are patches being worked on that will
> enable per-user clocks and as I understand it they will specifically
> disallow drivers to disable the hardware clock if other drivers are
> still keeping them on via their own referenc.
It is, but I was talking about a clk_disable after a clk_enable.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox