[PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer binding

[PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Hans de Goede]

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Mike Turquette <mturquette@linaro.org>

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

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 70c26f3..91176ee 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -1,8 +1,8 @@
 Simple Framebuffer
 
-A simple frame-buffer describes a raw memory region that may be rendered to,
-with the assumption that the display hardware has already been set up to scan
-out from that buffer.
+A simple frame-buffer describes a frame-buffer setup by firmware or
+the bootloader, with the assumption that the display hardware has already
+been set up to scan out from the memory pointed to by the ref property.
 
 Required properties:
 - compatible: "simple-framebuffer"
@@ -14,6 +14,9 @@ Required properties:
   - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
   - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
 
+Optional properties:
+- clocks : List of clocks used by the framebuffer
+
 Example:
 
 	framebuffer {
-- 
2.1.0

Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Rob Herring]

On Fri, Oct 3, 2014 at 9:05 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>
> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>
> --
> Changes in v2:
> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
> Changes in v3:
> -Updated description to make clear simplefb deals with more then just memory

NAK. "Fixing" the description is not what I meant and does not address
my concerns. Currently, simplefb is configuration data. It is
auxiliary data about how a chunk of memory is used. Using it or not
has no side effects on the hardware setup, but you are changing that
aspect. You are mixing in a hardware description that is simply
inaccurate.

The kernel has made the decision to turn off "unused" clocks. If its
determination of what is unused is wrong, then it is not a problem to
fix in DT.

Rob

> ---
>  Documentation/devicetree/bindings/video/simple-framebuffer.txt | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 70c26f3..91176ee 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -1,8 +1,8 @@
>  Simple Framebuffer
>
> -A simple frame-buffer describes a raw memory region that may be rendered to,
> -with the assumption that the display hardware has already been set up to scan
> -out from that buffer.
> +A simple frame-buffer describes a frame-buffer setup by firmware or
> +the bootloader, with the assumption that the display hardware has already
> +been set up to scan out from the memory pointed to by the ref property.
>
>  Required properties:
>  - compatible: "simple-framebuffer"
> @@ -14,6 +14,9 @@ Required properties:
>    - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>    - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>
> +Optional properties:
> +- clocks : List of clocks used by the framebuffer
> +
>  Example:
>
>         framebuffer {
> --
> 2.1.0
>

Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Geert Uytterhoeven]

Hi Rob,

On Fri, Oct 3, 2014 at 5:57 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Fri, Oct 3, 2014 at 9:05 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>
>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>
>> --
>> Changes in v2:
>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>> Changes in v3:
>> -Updated description to make clear simplefb deals with more then just memory
>
> NAK. "Fixing" the description is not what I meant and does not address
> my concerns. Currently, simplefb is configuration data. It is
> auxiliary data about how a chunk of memory is used. Using it or not
> has no side effects on the hardware setup, but you are changing that
> aspect. You are mixing in a hardware description that is simply
> inaccurate.
>
> The kernel has made the decision to turn off "unused" clocks. If its
> determination of what is unused is wrong, then it is not a problem to
> fix in DT.

The kernel has made that decision because the driver hadn't told the
kernel that those clocks had to be enabled.
The only way for the driver to know which clocks to enable is by adding
them to the description in DT.

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

Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Rob Herring]

On Fri, Oct 3, 2014 at 11:04 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Rob,
>
> On Fri, Oct 3, 2014 at 5:57 PM, Rob Herring <robherring2@gmail.com> wrote:
>> On Fri, Oct 3, 2014 at 9:05 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>
>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>
>>> --
>>> Changes in v2:
>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>> Changes in v3:
>>> -Updated description to make clear simplefb deals with more then just memory
>>
>> NAK. "Fixing" the description is not what I meant and does not address
>> my concerns. Currently, simplefb is configuration data. It is
>> auxiliary data about how a chunk of memory is used. Using it or not
>> has no side effects on the hardware setup, but you are changing that
>> aspect. You are mixing in a hardware description that is simply
>> inaccurate.
>>
>> The kernel has made the decision to turn off "unused" clocks. If its
>> determination of what is unused is wrong, then it is not a problem to
>> fix in DT.
>
> The kernel has made that decision because the driver hadn't told the
> kernel that those clocks had to be enabled.
> The only way for the driver to know which clocks to enable is by adding
> them to the description in DT.

Lack of a proper and complete driver is still a kernel problem. Now,
if you want to accurately describe the display h/w in DT and you
happen to use the simplefb driver, I don't really care. It just needs
to be a separate binding.

A driver is not the only solution here. The SOC or clock controller
code could keep the clock on based on either the fact that the clock
is already on or it is just hardcoded. This is not a problem unique to
displays.

Rob

Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Hans de Goede]

Hi,

On 10/03/2014 06:19 PM, Rob Herring wrote:
> On Fri, Oct 3, 2014 at 11:04 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> Hi Rob,
>>
>> On Fri, Oct 3, 2014 at 5:57 PM, Rob Herring <robherring2@gmail.com> wrote:
>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>
>>>> --
>>>> Changes in v2:
>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>> Changes in v3:
>>>> -Updated description to make clear simplefb deals with more then just memory
>>>
>>> NAK. "Fixing" the description is not what I meant and does not address
>>> my concerns. Currently, simplefb is configuration data. It is
>>> auxiliary data about how a chunk of memory is used. Using it or not
>>> has no side effects on the hardware setup, but you are changing that
>>> aspect. You are mixing in a hardware description that is simply
>>> inaccurate.
>>>
>>> The kernel has made the decision to turn off "unused" clocks. If its
>>> determination of what is unused is wrong, then it is not a problem to
>>> fix in DT.
>>
>> The kernel has made that decision because the driver hadn't told the
>> kernel that those clocks had to be enabled.
>> The only way for the driver to know which clocks to enable is by adding
>> them to the description in DT.
> 
> Lack of a proper and complete driver is still a kernel problem. Now,
> if you want to accurately describe the display h/w in DT and you
> happen to use the simplefb driver, I don't really care. It just needs
> to be a separate binding.

Please read the: "[PATCH 4/4] simplefb: add clock handling code"
thread. The whole purpose we want to use simplefb for is to have a hardware
agnostic driver for early boot messages. Not all devices have a usable
serial console, so this is a must have for user-friendly debugging of
boot problems. Basically the devicetree equivalent of vgacon / efifb.

So we actually do not want to describe the hardware accurately, we want
something generic, which simplefb gives us, we just want it to be a slightly
more complete description then simplefb currently gives as, as the current
description is too limited in practice, specifically the simpefb virtual
device needs to accurately declare which clocks it uses, just like any
other real hardware device in devicetree declares which clocks it uses.

Regards,

Hans

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Hans de Goede]

Hi,

On 10/03/2014 05:57 PM, Rob Herring wrote:
> On Fri, Oct 3, 2014 at 9:05 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>
>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>
>> --
>> Changes in v2:
>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>> Changes in v3:
>> -Updated description to make clear simplefb deals with more then just memory
> 
> NAK. "Fixing" the description is not what I meant and does not address
> my concerns. Currently, simplefb is configuration data. It is
> auxiliary data about how a chunk of memory is used. Using it or not
> has no side effects on the hardware setup, but you are changing that
> aspect. You are mixing in a hardware description that is simply
> inaccurate.

Memory is hardware too, what simplefb is is best seen as a virtual device, and
even virtual devices have hardware resources they need, such as a chunk of memory,
which the kernel should not touch other then use it as a framebuffer, likewise
on some systems the virtual device needs clocks to keep running.

> The kernel has made the decision to turn off "unused" clocks. If its
> determination of what is unused is wrong, then it is not a problem to
> fix in DT.

No, it is up to DT to tell the kernel what clocks are used, that is how it works
for any other device. I don't understand why some people keep insisting simplefb
for some reason is o so very very special, because it is not special, it needs
resources, and it needs to tell the kernel about this or bad things happen.

More over it is a bit late to start making objections now. This has already been
discussed to death for weeks now, and every argument against this patch has already
been countered multiple times (including the one you are making now). Feel free to
read the entire thread in the archives under the subject:
"[PATCH 4/4] simplefb: add clock handling code"

At one point in time we need to stop bickering and make a decision, that time has
come now, so please lets get this discussion over with and merge this, so that
we can all move on and spend out time in a more productive manner.

Regards,

Hans

> 
> Rob
> 
>> ---
>>  Documentation/devicetree/bindings/video/simple-framebuffer.txt | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index 70c26f3..91176ee 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -1,8 +1,8 @@
>>  Simple Framebuffer
>>
>> -A simple frame-buffer describes a raw memory region that may be rendered to,
>> -with the assumption that the display hardware has already been set up to scan
>> -out from that buffer.
>> +A simple frame-buffer describes a frame-buffer setup by firmware or
>> +the bootloader, with the assumption that the display hardware has already
>> +been set up to scan out from the memory pointed to by the ref property.
>>
>>  Required properties:
>>  - compatible: "simple-framebuffer"
>> @@ -14,6 +14,9 @@ Required properties:
>>    - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>>    - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>>
>> +Optional properties:
>> +- clocks : List of clocks used by the framebuffer
>> +
>>  Example:
>>
>>         framebuffer {
>> --
>> 2.1.0
>>
> 

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Rob Herring]

On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 10/03/2014 05:57 PM, Rob Herring wrote:
>> On Fri, Oct 3, 2014 at 9:05 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>
>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>
>>> --
>>> Changes in v2:
>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>> Changes in v3:
>>> -Updated description to make clear simplefb deals with more then just memory
>>
>> NAK. "Fixing" the description is not what I meant and does not address
>> my concerns. Currently, simplefb is configuration data. It is
>> auxiliary data about how a chunk of memory is used. Using it or not
>> has no side effects on the hardware setup, but you are changing that
>> aspect. You are mixing in a hardware description that is simply
>> inaccurate.
>
> Memory is hardware too, what simplefb is is best seen as a virtual device, and
> even virtual devices have hardware resources they need, such as a chunk of memory,
> which the kernel should not touch other then use it as a framebuffer, likewise
> on some systems the virtual device needs clocks to keep running.
>
>> The kernel has made the decision to turn off "unused" clocks. If its
>> determination of what is unused is wrong, then it is not a problem to
>> fix in DT.
>
> No, it is up to DT to tell the kernel what clocks are used, that is how it works
> for any other device. I don't understand why some people keep insisting simplefb
> for some reason is o so very very special, because it is not special, it needs
> resources, and it needs to tell the kernel about this or bad things happen.

No, the DT describes the connections of clocks from h/w block to h/w
block. Their use is implied by the connection.

And yes, as the other thread mentioned DT is more than just hardware
information. However, what you are adding IS hardware information and
clearly has a place somewhere else. And adding anything which is not
hardware description gets much more scrutiny.

> More over it is a bit late to start making objections now. This has already been
> discussed to death for weeks now, and every argument against this patch has already
> been countered multiple times (including the one you are making now). Feel free to
> read the entire thread in the archives under the subject:
> "[PATCH 4/4] simplefb: add clock handling code"

You are on v2 and I hardly see any consensus on the v1 thread. Others
have made suggestions which I would agree with and you've basically
ignored them.

> At one point in time we need to stop bickering and make a decision, that time has
> come now, so please lets get this discussion over with and merge this, so that
> we can all move on and spend out time in a more productive manner.

Not an effective argument to get things merged.

Rob

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[jonsmirl]

On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>
>>>> --
>>>> Changes in v2:
>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>> Changes in v3:
>>>> -Updated description to make clear simplefb deals with more then just memory
>>>
>>> NAK. "Fixing" the description is not what I meant and does not address
>>> my concerns. Currently, simplefb is configuration data. It is
>>> auxiliary data about how a chunk of memory is used. Using it or not
>>> has no side effects on the hardware setup, but you are changing that
>>> aspect. You are mixing in a hardware description that is simply
>>> inaccurate.
>>
>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>> even virtual devices have hardware resources they need, such as a chunk of memory,
>> which the kernel should not touch other then use it as a framebuffer, likewise
>> on some systems the virtual device needs clocks to keep running.
>>
>>> The kernel has made the decision to turn off "unused" clocks. If its
>>> determination of what is unused is wrong, then it is not a problem to
>>> fix in DT.
>>
>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>> for any other device. I don't understand why some people keep insisting simplefb
>> for some reason is o so very very special, because it is not special, it needs
>> resources, and it needs to tell the kernel about this or bad things happen.
>
> No, the DT describes the connections of clocks from h/w block to h/w
> block. Their use is implied by the connection.
>
> And yes, as the other thread mentioned DT is more than just hardware
> information. However, what you are adding IS hardware information and
> clearly has a place somewhere else. And adding anything which is not
> hardware description gets much more scrutiny.
>
>> More over it is a bit late to start making objections now. This has already been
>> discussed to death for weeks now, and every argument against this patch has already
>> been countered multiple times (including the one you are making now). Feel free to
>> read the entire thread in the archives under the subject:
>> "[PATCH 4/4] simplefb: add clock handling code"
>
> You are on v2 and I hardly see any consensus on the v1 thread. Others
> have made suggestions which I would agree with and you've basically
> ignored them.
>
>> At one point in time we need to stop bickering and make a decision, that time has
>> come now, so please lets get this discussion over with and merge this, so that
>> we can all move on and spend out time in a more productive manner.
>
> Not an effective argument to get things merged.

This is another way to solve the problem.....

On Thu, Oct 2, 2014 at 9:46 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Oct 2, 2014 at 3:34 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>> Does the clock and regulator cleanup happen before drivers can load
>> off from initrd? I didn't think it did but I might be wrong.
>
> Yes
>
> drivers/base/power/domain.c:late_initcall(genpd_poweroff_unused);
> drivers/clk/clk.c:late_initcall_sync(clk_disable_unused);
> drivers/regulator/core.c:late_initcall_sync(regulator_init_complete);

What do you think about putting these calls onto an ioctl somewhere
and then eliminating the late_initcall(..)? A tiny user space app
could then hit that ioctl after all of the loadable device drivers are
loaded. Add the command to make this ioctl call to busybox or udev.
After all, it is not fatal if these calls aren't made, all they do is
save power. Add a link in rc.d or somewhere similar to run this app at
the appropriate time.

Switching these over to an ioctl allows a window to be opened for
device specific driver loading before the clock/regulator clean up
happens.

Now all of this mess of protecting clocks and regulator disappears.
Instead get the device specific drivers written and loaded, then run
the cleanup app which hits the ioctl(). All of the correct
clock/regulators will be claimed and then this clean code will do the
right thing.

From my perspective it appears that this cleanup is being done too
early which then triggers a need to protect things from cleanup.

>
> Gr{oetje,eeting}s,
>
>                         Geert


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



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Geert Uytterhoeven]

On Fri, Oct 3, 2014 at 10:55 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
> On Thu, Oct 2, 2014 at 9:46 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, Oct 2, 2014 at 3:34 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>>> Does the clock and regulator cleanup happen before drivers can load
>>> off from initrd? I didn't think it did but I might be wrong.
>>
>> Yes
>>
>> drivers/base/power/domain.c:late_initcall(genpd_poweroff_unused);
>> drivers/clk/clk.c:late_initcall_sync(clk_disable_unused);
>> drivers/regulator/core.c:late_initcall_sync(regulator_init_complete);
>
> What do you think about putting these calls onto an ioctl somewhere
> and then eliminating the late_initcall(..)? A tiny user space app
> could then hit that ioctl after all of the loadable device drivers are
> loaded. Add the command to make this ioctl call to busybox or udev.
> After all, it is not fatal if these calls aren't made, all they do is
> save power. Add a link in rc.d or somewhere similar to run this app at
> the appropriate time.
>
> Switching these over to an ioctl allows a window to be opened for
> device specific driver loading before the clock/regulator clean up
> happens.
>
> Now all of this mess of protecting clocks and regulator disappears.
> Instead get the device specific drivers written and loaded, then run
> the cleanup app which hits the ioctl(). All of the correct
> clock/regulators will be claimed and then this clean code will do the
> right thing.
>
> From my perspective it appears that this cleanup is being done too
> early which then triggers a need to protect things from cleanup.

Not doing the cleanup doesn't help.

If someone else calls clk_disable() on a clock which shares a parent
with the clock you're silently using, that clock will still be disabled.
This can happen at any time.

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

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[jonsmirl]

On Fri, Oct 3, 2014 at 5:26 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Oct 3, 2014 at 10:55 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>> On Thu, Oct 2, 2014 at 9:46 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Thu, Oct 2, 2014 at 3:34 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>>>> Does the clock and regulator cleanup happen before drivers can load
>>>> off from initrd? I didn't think it did but I might be wrong.
>>>
>>> Yes
>>>
>>> drivers/base/power/domain.c:late_initcall(genpd_poweroff_unused);
>>> drivers/clk/clk.c:late_initcall_sync(clk_disable_unused);
>>> drivers/regulator/core.c:late_initcall_sync(regulator_init_complete);
>>
>> What do you think about putting these calls onto an ioctl somewhere
>> and then eliminating the late_initcall(..)? A tiny user space app
>> could then hit that ioctl after all of the loadable device drivers are
>> loaded. Add the command to make this ioctl call to busybox or udev.
>> After all, it is not fatal if these calls aren't made, all they do is
>> save power. Add a link in rc.d or somewhere similar to run this app at
>> the appropriate time.
>>
>> Switching these over to an ioctl allows a window to be opened for
>> device specific driver loading before the clock/regulator clean up
>> happens.
>>
>> Now all of this mess of protecting clocks and regulator disappears.
>> Instead get the device specific drivers written and loaded, then run
>> the cleanup app which hits the ioctl(). All of the correct
>> clock/regulators will be claimed and then this clean code will do the
>> right thing.
>>
>> From my perspective it appears that this cleanup is being done too
>> early which then triggers a need to protect things from cleanup.
>
> Not doing the cleanup doesn't help.
>
> If someone else calls clk_disable() on a clock which shares a parent
> with the clock you're silently using, that clock will still be disabled.
> This can happen at any time.

Could we start all of the regulators and clocks off with a reference
count of one, but not do anything to change their state? Then this
ioctl() would decrement that extra reference. Removing the extra
reference would then disable everything that isn't claimed.


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



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Mike Turquette]

Quoting jonsmirl@gmail.com (2014-10-03 14:50:24)
> On Fri, Oct 3, 2014 at 5:26 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Oct 3, 2014 at 10:55 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
> >> On Thu, Oct 2, 2014 at 9:46 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Thu, Oct 2, 2014 at 3:34 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
> >>>> Does the clock and regulator cleanup happen before drivers can load
> >>>> off from initrd? I didn't think it did but I might be wrong.
> >>>
> >>> Yes
> >>>
> >>> drivers/base/power/domain.c:late_initcall(genpd_poweroff_unused);
> >>> drivers/clk/clk.c:late_initcall_sync(clk_disable_unused);
> >>> drivers/regulator/core.c:late_initcall_sync(regulator_init_complete);
> >>
> >> What do you think about putting these calls onto an ioctl somewhere
> >> and then eliminating the late_initcall(..)? A tiny user space app
> >> could then hit that ioctl after all of the loadable device drivers are
> >> loaded. Add the command to make this ioctl call to busybox or udev.
> >> After all, it is not fatal if these calls aren't made, all they do is
> >> save power. Add a link in rc.d or somewhere similar to run this app at
> >> the appropriate time.
> >>
> >> Switching these over to an ioctl allows a window to be opened for
> >> device specific driver loading before the clock/regulator clean up
> >> happens.
> >>
> >> Now all of this mess of protecting clocks and regulator disappears.
> >> Instead get the device specific drivers written and loaded, then run
> >> the cleanup app which hits the ioctl(). All of the correct
> >> clock/regulators will be claimed and then this clean code will do the
> >> right thing.
> >>
> >> From my perspective it appears that this cleanup is being done too
> >> early which then triggers a need to protect things from cleanup.
> >
> > Not doing the cleanup doesn't help.
> >
> > If someone else calls clk_disable() on a clock which shares a parent
> > with the clock you're silently using, that clock will still be disabled.
> > This can happen at any time.
> 
> Could we start all of the regulators and clocks off with a reference
> count of one, but not do anything to change their state? Then this
> ioctl() would decrement that extra reference. Removing the extra
> reference would then disable everything that isn't claimed.

No. That is too ugly.

Regards,
Mike

> 
> 
> >
> > 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
> 
> 
> 
> -- 
> Jon Smirl
> jonsmirl@gmail.com

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[jonsmirl]

On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>
>>>> --
>>>> Changes in v2:
>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>> Changes in v3:
>>>> -Updated description to make clear simplefb deals with more then just memory
>>>
>>> NAK. "Fixing" the description is not what I meant and does not address
>>> my concerns. Currently, simplefb is configuration data. It is
>>> auxiliary data about how a chunk of memory is used. Using it or not
>>> has no side effects on the hardware setup, but you are changing that
>>> aspect. You are mixing in a hardware description that is simply
>>> inaccurate.
>>
>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>> even virtual devices have hardware resources they need, such as a chunk of memory,
>> which the kernel should not touch other then use it as a framebuffer, likewise
>> on some systems the virtual device needs clocks to keep running.
>>
>>> The kernel has made the decision to turn off "unused" clocks. If its
>>> determination of what is unused is wrong, then it is not a problem to
>>> fix in DT.
>>
>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>> for any other device. I don't understand why some people keep insisting simplefb
>> for some reason is o so very very special, because it is not special, it needs
>> resources, and it needs to tell the kernel about this or bad things happen.
>
> No, the DT describes the connections of clocks from h/w block to h/w
> block. Their use is implied by the connection.
>
> And yes, as the other thread mentioned DT is more than just hardware
> information. However, what you are adding IS hardware information and
> clearly has a place somewhere else. And adding anything which is not
> hardware description gets much more scrutiny.
>
>> More over it is a bit late to start making objections now. This has already been
>> discussed to death for weeks now, and every argument against this patch has already
>> been countered multiple times (including the one you are making now). Feel free to
>> read the entire thread in the archives under the subject:
>> "[PATCH 4/4] simplefb: add clock handling code"
>
> You are on v2 and I hardly see any consensus on the v1 thread. Others
> have made suggestions which I would agree with and you've basically
> ignored them.
>
>> At one point in time we need to stop bickering and make a decision, that time has
>> come now, so please lets get this discussion over with and merge this, so that
>> we can all move on and spend out time in a more productive manner.
>
> Not an effective argument to get things merged.

If there is not good solution to deferring clock clean up in the
kernel, another approach is to change how simple-framebuffer is
described in the device tree....

Right now it is a stand-alone item that looks like a device node, but
it isn't a device.

framebuffer {
    compatible = "simple-framebuffer";
    reg = <0x1d385000 (1600 * 1200 * 2)>;
    width = <1600>;
    height = <1200>;
    stride = <(1600 * 2)>;
    format = "r5g6b5";
};

How about something like this?

reserved-memory {
    #address-cells = <1>;
    #size-cells = <1>;
    ranges;

    display_reserved: framebuffer@78000000 {
        reg = <0x78000000  (1600 * 1200 * 2)>;
    };
};

lcd0: lcd-controller@820000 {
    compatible = "marvell,dove-lcd";
    reg = <0x820000 0x1000>;
    interrupts = <47>;
    clocks = <&si5351 0>;
    clock-names = "ext_ref_clk_1";
};

chosen {
    boot-framebuffer {
       compatible = "simple-framebuffer";
       device = <&lcd0>;
       framebuffer = <&display_reserved>;
       width = <1600>;
       height = <1200>;
       stride = <(1600 * 2)>;
       format = "r5g6b5";
    };
}


This moves the definition of the boot framebuffer setup into the area
where bootloader info is suppose to go. Then you can use the phandle
to follow the actual device chains and protect the clocks and
regulators. To make that work you are required to provide an accurate
description of the real video hardware so that this chain can be
followed.

The main assumption here is that the syntax for clock and regulators
is standardized enough that the generic simplefb code can run the
phandle chain and discover them all. If they aren't then we messed up
and simplefb gains some complexity to handle the irregular
descriptions. That is something having a schema for device tree would
have prevented.

The compatible string should trigger simple-fb into making a 'struct
device' that isn't hooked to any real hardware. Not clear that we
should use a compatible = "" here. The device = <> points to the real
hardware. Run that phandle chain and figure out what clocks need to be
protected.

Now when the driver for dove-lcd loads there won't be anything already
attached to the hardware so there is no complex hand off needed.
dove-lcd will just need to signal simple-fb to exit Exiting will
decrement the ref counts on clocks/regulators.

The memory region also allows for a ref count mechanism which allows
it to be passed from simplefb to dove-lcd. If the ref count goes to
zero when simplefb exits, it can be reclaimed.

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



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Hans de Goede]

Hi,

On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>
>>>>> --
>>>>> Changes in v2:
>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>> Changes in v3:
>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>
>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>> my concerns. Currently, simplefb is configuration data. It is
>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>> has no side effects on the hardware setup, but you are changing that
>>>> aspect. You are mixing in a hardware description that is simply
>>>> inaccurate.
>>>
>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>> on some systems the virtual device needs clocks to keep running.
>>>
>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>> determination of what is unused is wrong, then it is not a problem to
>>>> fix in DT.
>>>
>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>> for any other device. I don't understand why some people keep insisting simplefb
>>> for some reason is o so very very special, because it is not special, it needs
>>> resources, and it needs to tell the kernel about this or bad things happen.
>>
>> No, the DT describes the connections of clocks from h/w block to h/w
>> block. Their use is implied by the connection.
>>
>> And yes, as the other thread mentioned DT is more than just hardware
>> information. However, what you are adding IS hardware information and
>> clearly has a place somewhere else. And adding anything which is not
>> hardware description gets much more scrutiny.
>>
>>> More over it is a bit late to start making objections now. This has already been
>>> discussed to death for weeks now, and every argument against this patch has already
>>> been countered multiple times (including the one you are making now). Feel free to
>>> read the entire thread in the archives under the subject:
>>> "[PATCH 4/4] simplefb: add clock handling code"
>>
>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>> have made suggestions which I would agree with and you've basically
>> ignored them.
>>
>>> At one point in time we need to stop bickering and make a decision, that time has
>>> come now, so please lets get this discussion over with and merge this, so that
>>> we can all move on and spend out time in a more productive manner.
>>
>> Not an effective argument to get things merged.
> 
> If there is not good solution to deferring clock clean up in the
> kernel, another approach is to change how simple-framebuffer is
> described in the device tree....
> 
> Right now it is a stand-alone item that looks like a device node, but
> it isn't a device.
> 
> framebuffer {
>     compatible = "simple-framebuffer";
>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>     width = <1600>;
>     height = <1200>;
>     stride = <(1600 * 2)>;
>     format = "r5g6b5";
> };
> 
> How about something like this?
> 
> reserved-memory {
>     #address-cells = <1>;
>     #size-cells = <1>;
>     ranges;
> 
>     display_reserved: framebuffer@78000000 {
>         reg = <0x78000000  (1600 * 1200 * 2)>;
>     };
> };
> 
> lcd0: lcd-controller@820000 {
>     compatible = "marvell,dove-lcd";
>     reg = <0x820000 0x1000>;
>     interrupts = <47>;
>     clocks = <&si5351 0>;
>     clock-names = "ext_ref_clk_1";
> };
> 
> chosen {
>     boot-framebuffer {
>        compatible = "simple-framebuffer";
>        device = <&lcd0>;
>        framebuffer = <&display_reserved>;
>        width = <1600>;
>        height = <1200>;
>        stride = <(1600 * 2)>;
>        format = "r5g6b5";
>     };
> }
> 
> 
> This moves the definition of the boot framebuffer setup into the area
> where bootloader info is suppose to go. Then you can use the phandle
> to follow the actual device chains and protect the clocks and
> regulators. To make that work you are required to provide an accurate
> description of the real video hardware so that this chain can be
> followed.

This will not work, first of all multiple blocks may be involved, so
the device = in the boot-framebuffer would need to be a list. That in
itself is not a problem, the problem is that the blocks used may have
multiple clocks, of which the setup mode likely uses only a few.

So if we do things this way, we end up keeping way to many clocks
enabled.

This does nicely show why the clocks really belong in the simplefb
node though. What you suggest above, would lead to simplefb having
access to the hardware info / description of the display engine
blocks, putting the clocks info in the display engine nodes, so
keeping hardware description with hardware description as people
want.

But as said that does not tell the kernel which clocks are actually
used for the setup mode, so it does not provide the info the kernel
needs. What the kernel needs is not hardware description, but a
list of clocks which are *actually used* by the setup mode.

Thinking more about this the clocks property in the simplefb node
is just like the reg property. Both are properties normally only
found in real harware nodes, not in an informational node like
simplefb.

But for the kernel to be able to actually use the simplefb it
needs to know which memory is used by the framebuffer. One could
argue that a reg property is hardware description, and thus does not
belong in the simplefb node, and that the kernel should just magically
figure out which memory is used.

Everyone sees that the kernel cannot magically figure out which memory
is used by the simplefb. Yet people expect the kernel to somehow
magically figure out which clocks are used, to avoid accidentally turning
them off.

This is not consistent, either the kernel needs to be told these things,
and then it needs to be told all of them, or hardware node properties
like a reg property do not belong in an informational node, and then the
reg property should not be in the simplefb node either, and the kernel
should somehow magically figure out where the memory lives, just like
some people are expecting the kernel to magically figure out which
clocks are used.

Regards,

Hans

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[jonsmirl]

On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>
>>>>>> --
>>>>>> Changes in v2:
>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>> Changes in v3:
>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>
>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>> has no side effects on the hardware setup, but you are changing that
>>>>> aspect. You are mixing in a hardware description that is simply
>>>>> inaccurate.
>>>>
>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>> on some systems the virtual device needs clocks to keep running.
>>>>
>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>> fix in DT.
>>>>
>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>> for some reason is o so very very special, because it is not special, it needs
>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>
>>> No, the DT describes the connections of clocks from h/w block to h/w
>>> block. Their use is implied by the connection.
>>>
>>> And yes, as the other thread mentioned DT is more than just hardware
>>> information. However, what you are adding IS hardware information and
>>> clearly has a place somewhere else. And adding anything which is not
>>> hardware description gets much more scrutiny.
>>>
>>>> More over it is a bit late to start making objections now. This has already been
>>>> discussed to death for weeks now, and every argument against this patch has already
>>>> been countered multiple times (including the one you are making now). Feel free to
>>>> read the entire thread in the archives under the subject:
>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>
>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>> have made suggestions which I would agree with and you've basically
>>> ignored them.
>>>
>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>> come now, so please lets get this discussion over with and merge this, so that
>>>> we can all move on and spend out time in a more productive manner.
>>>
>>> Not an effective argument to get things merged.
>>
>> If there is not good solution to deferring clock clean up in the
>> kernel, another approach is to change how simple-framebuffer is
>> described in the device tree....
>>
>> Right now it is a stand-alone item that looks like a device node, but
>> it isn't a device.
>>
>> framebuffer {
>>     compatible = "simple-framebuffer";
>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>     width = <1600>;
>>     height = <1200>;
>>     stride = <(1600 * 2)>;
>>     format = "r5g6b5";
>> };
>>
>> How about something like this?
>>
>> reserved-memory {
>>     #address-cells = <1>;
>>     #size-cells = <1>;
>>     ranges;
>>
>>     display_reserved: framebuffer@78000000 {
>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>     };
>> };
>>
>> lcd0: lcd-controller@820000 {
>>     compatible = "marvell,dove-lcd";
>>     reg = <0x820000 0x1000>;
>>     interrupts = <47>;
>>     clocks = <&si5351 0>;
>>     clock-names = "ext_ref_clk_1";
>> };
>>
>> chosen {
>>     boot-framebuffer {
>>        compatible = "simple-framebuffer";
>>        device = <&lcd0>;
>>        framebuffer = <&display_reserved>;
>>        width = <1600>;
>>        height = <1200>;
>>        stride = <(1600 * 2)>;
>>        format = "r5g6b5";
>>     };
>> }
>>
>>
>> This moves the definition of the boot framebuffer setup into the area
>> where bootloader info is suppose to go. Then you can use the phandle
>> to follow the actual device chains and protect the clocks and
>> regulators. To make that work you are required to provide an accurate
>> description of the real video hardware so that this chain can be
>> followed.
>
> This will not work, first of all multiple blocks may be involved, so
> the device = in the boot-framebuffer would need to be a list. That in
> itself is not a problem, the problem is that the blocks used may have
> multiple clocks, of which the setup mode likely uses only a few.
>
> So if we do things this way, we end up keeping way to many clocks
> enabled.

That doesn't hurt anything. Simplefb is not going to enable anything,
it is just protecting things from getting disabled. The whole point of
simplefb is to assume that the BIOS set things up correctly. When the
device specific driver loads everything will get sorted out and it
will own the correct clocks.

This does require a device specific driver to be written. Fairly easy
to do by providing a device specific framebuffer driver while we wait
on the KMS one.  I do not support letting the life of simplefb extend
past the initial boot window. For example - it is not going to came
back after suspend/resume.

It doesn't work for simplefb to just increment the ref count as a way
of protecting the clocks/regulator. If you inc the ref count and the
clock is off, the device specific driver can't turn it on. We have to
build a new 'protected' mechanism for the clocks/regulators. When a
clock/regulator is 'protected'  it can't turn off until both the ref
count is zero and the protected bit is off. 'protected' does not
impact turning it on.

And all of this is a problem up in the OS. Device trees are hardware
descriptions, they are not meant to be manipulated into fixing OS
level problems.

>
> This does nicely show why the clocks really belong in the simplefb
> node though. What you suggest above, would lead to simplefb having
> access to the hardware info / description of the display engine
> blocks, putting the clocks info in the display engine nodes, so
> keeping hardware description with hardware description as people
> want.
>
> But as said that does not tell the kernel which clocks are actually
> used for the setup mode, so it does not provide the info the kernel
> needs. What the kernel needs is not hardware description, but a
> list of clocks which are *actually used* by the setup mode.
>
> Thinking more about this the clocks property in the simplefb node
> is just like the reg property. Both are properties normally only
> found in real harware nodes, not in an informational node like
> simplefb.
>
> But for the kernel to be able to actually use the simplefb it
> needs to know which memory is used by the framebuffer. One could
> argue that a reg property is hardware description, and thus does not
> belong in the simplefb node, and that the kernel should just magically
> figure out which memory is used.

This is not a good parallel. That memory buffer has been given to the
hardware so my example DT is wrong, That's why we need to review these
things. It should be like this...

The device tree is reflecting ownership, not state.  If the buffer is
not under the hardware when simplefb disappears it would get freed and
the display controller still owns it and is still using it.

>> reserved-memory {
>>     #address-cells = <1>;
>>     #size-cells = <1>;
>>     ranges;
>>
>>     display_reserved: framebuffer@78000000 {
>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>     };
>> };
>>
>> lcd0: lcd-controller@820000 {
>>     compatible = "marvell,dove-lcd";
>>     reg = <0x820000 0x1000>;
>>     interrupts = <47>;
>>     clocks = <&si5351 0>;
>>     clock-names = "ext_ref_clk_1";
>>     framebuffer = <&display_reserved>;
>> };
>>
>> chosen {
>>     boot-framebuffer {
>>        compatible = "simple-framebuffer";
>>        device = <&lcd0>;
>>        width = <1600>;
>>        height = <1200>;
>>        stride = <(1600 * 2)>;
>>        format = "r5g6b5";
>>     };
>> }

It is questionable whether there should be a compatible string in the
chosen section. That compatible string is loading in a parser that
understands the section, not a device driver. The simplefb driver
could key off from 'boot-framebuffer' instead of the compatible
string.


>
> Everyone sees that the kernel cannot magically figure out which memory
> is used by the simplefb. Yet people expect the kernel to somehow
> magically figure out which clocks are used, to avoid accidentally turning
> them off.
>
> This is not consistent, either the kernel needs to be told these things,
> and then it needs to be told all of them, or hardware node properties
> like a reg property do not belong in an informational node, and then the
> reg property should not be in the simplefb node either, and the kernel
> should somehow magically figure out where the memory lives, just like
> some people are expecting the kernel to magically figure out which
> clocks are used.
>
> Regards,
>
> Hans



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Hans de Goede]

Hi,

On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>
>>>>>>> --
>>>>>>> Changes in v2:
>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>> Changes in v3:
>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>
>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>> inaccurate.
>>>>>
>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>
>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>> fix in DT.
>>>>>
>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>
>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>> block. Their use is implied by the connection.
>>>>
>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>> information. However, what you are adding IS hardware information and
>>>> clearly has a place somewhere else. And adding anything which is not
>>>> hardware description gets much more scrutiny.
>>>>
>>>>> More over it is a bit late to start making objections now. This has already been
>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>> read the entire thread in the archives under the subject:
>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>
>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>> have made suggestions which I would agree with and you've basically
>>>> ignored them.
>>>>
>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>> we can all move on and spend out time in a more productive manner.
>>>>
>>>> Not an effective argument to get things merged.
>>>
>>> If there is not good solution to deferring clock clean up in the
>>> kernel, another approach is to change how simple-framebuffer is
>>> described in the device tree....
>>>
>>> Right now it is a stand-alone item that looks like a device node, but
>>> it isn't a device.
>>>
>>> framebuffer {
>>>     compatible = "simple-framebuffer";
>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>     width = <1600>;
>>>     height = <1200>;
>>>     stride = <(1600 * 2)>;
>>>     format = "r5g6b5";
>>> };
>>>
>>> How about something like this?
>>>
>>> reserved-memory {
>>>     #address-cells = <1>;
>>>     #size-cells = <1>;
>>>     ranges;
>>>
>>>     display_reserved: framebuffer@78000000 {
>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>     };
>>> };
>>>
>>> lcd0: lcd-controller@820000 {
>>>     compatible = "marvell,dove-lcd";
>>>     reg = <0x820000 0x1000>;
>>>     interrupts = <47>;
>>>     clocks = <&si5351 0>;
>>>     clock-names = "ext_ref_clk_1";
>>> };
>>>
>>> chosen {
>>>     boot-framebuffer {
>>>        compatible = "simple-framebuffer";
>>>        device = <&lcd0>;
>>>        framebuffer = <&display_reserved>;
>>>        width = <1600>;
>>>        height = <1200>;
>>>        stride = <(1600 * 2)>;
>>>        format = "r5g6b5";
>>>     };
>>> }
>>>
>>>
>>> This moves the definition of the boot framebuffer setup into the area
>>> where bootloader info is suppose to go. Then you can use the phandle
>>> to follow the actual device chains and protect the clocks and
>>> regulators. To make that work you are required to provide an accurate
>>> description of the real video hardware so that this chain can be
>>> followed.
>>
>> This will not work, first of all multiple blocks may be involved, so
>> the device = in the boot-framebuffer would need to be a list. That in
>> itself is not a problem, the problem is that the blocks used may have
>> multiple clocks, of which the setup mode likely uses only a few.
>>
>> So if we do things this way, we end up keeping way to many clocks
>> enabled.
> 
> That doesn't hurt anything.

<snip lots of stuff based on the above>

Wrong, that does hurt things. As already discussed simply stopping the
clocks from being disabled by the unused_clks mechanism is not enough,
since clocks may be shared, and we must stop another device driver
sharing the clock and doing clk_enable; probe; clk_disable; disabling
the shared clk, which means calling clk_enable on it to mark it as
being in use. So this will hurt cause it will cause us to enable a bunch
of clks which should not be enabled.

I've been thinking more about this, and I understand that people don't
want to put hardware description in the simplefb node, but this is
not hardware description.

u-boot sets up the display-pipeline to scan out of a certain part of
memory, in order to do this it writes the memory address to some registers
in the display pipeline, so what it is passing is not hardware description
(it is not passing all possible memory addresses for the framebuffer), but
it is passing information about the state in which it has left the display
pipeline, iow it is passing state information.

Likewise when u-boot sets up the display-pipeline it choices certain
clocks to use, just like it chooses a framebuffer address, and then
it programs those clocks into registers. So what we want to pass along
here is information about the way in which the display-pipeline has
been configured, so we're passing along state information,
not hardware description, just like with the framebuffer address.

Regards,

Hans

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[jonsmirl]

On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Changes in v2:
>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>> Changes in v3:
>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>
>>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>>> inaccurate.
>>>>>>
>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>>
>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>>> fix in DT.
>>>>>>
>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>>
>>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>>> block. Their use is implied by the connection.
>>>>>
>>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>>> information. However, what you are adding IS hardware information and
>>>>> clearly has a place somewhere else. And adding anything which is not
>>>>> hardware description gets much more scrutiny.
>>>>>
>>>>>> More over it is a bit late to start making objections now. This has already been
>>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>>> read the entire thread in the archives under the subject:
>>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>>
>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>>> have made suggestions which I would agree with and you've basically
>>>>> ignored them.
>>>>>
>>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>>> we can all move on and spend out time in a more productive manner.
>>>>>
>>>>> Not an effective argument to get things merged.
>>>>
>>>> If there is not good solution to deferring clock clean up in the
>>>> kernel, another approach is to change how simple-framebuffer is
>>>> described in the device tree....
>>>>
>>>> Right now it is a stand-alone item that looks like a device node, but
>>>> it isn't a device.
>>>>
>>>> framebuffer {
>>>>     compatible = "simple-framebuffer";
>>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>>     width = <1600>;
>>>>     height = <1200>;
>>>>     stride = <(1600 * 2)>;
>>>>     format = "r5g6b5";
>>>> };
>>>>
>>>> How about something like this?
>>>>
>>>> reserved-memory {
>>>>     #address-cells = <1>;
>>>>     #size-cells = <1>;
>>>>     ranges;
>>>>
>>>>     display_reserved: framebuffer@78000000 {
>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>     };
>>>> };
>>>>
>>>> lcd0: lcd-controller@820000 {
>>>>     compatible = "marvell,dove-lcd";
>>>>     reg = <0x820000 0x1000>;
>>>>     interrupts = <47>;
>>>>     clocks = <&si5351 0>;
>>>>     clock-names = "ext_ref_clk_1";
>>>> };
>>>>
>>>> chosen {
>>>>     boot-framebuffer {
>>>>        compatible = "simple-framebuffer";
>>>>        device = <&lcd0>;
>>>>        framebuffer = <&display_reserved>;
>>>>        width = <1600>;
>>>>        height = <1200>;
>>>>        stride = <(1600 * 2)>;
>>>>        format = "r5g6b5";
>>>>     };
>>>> }
>>>>
>>>>
>>>> This moves the definition of the boot framebuffer setup into the area
>>>> where bootloader info is suppose to go. Then you can use the phandle
>>>> to follow the actual device chains and protect the clocks and
>>>> regulators. To make that work you are required to provide an accurate
>>>> description of the real video hardware so that this chain can be
>>>> followed.
>>>
>>> This will not work, first of all multiple blocks may be involved, so
>>> the device = in the boot-framebuffer would need to be a list. That in
>>> itself is not a problem, the problem is that the blocks used may have
>>> multiple clocks, of which the setup mode likely uses only a few.
>>>
>>> So if we do things this way, we end up keeping way to many clocks
>>> enabled.
>>
>> That doesn't hurt anything.
>
> <snip lots of stuff based on the above>
>
> Wrong, that does hurt things. As already discussed simply stopping the
> clocks from being disabled by the unused_clks mechanism is not enough,
> since clocks may be shared, and we must stop another device driver
> sharing the clock and doing clk_enable; probe; clk_disable; disabling
> the shared clk, which means calling clk_enable on it to mark it as
> being in use. So this will hurt cause it will cause us to enable a bunch
> of clks which should not be enabled.

I said earlier that you would need to add a protected mechanism to
clocks to handle this phase. When a clock/regulator is protected you
can turn it on but you can't turn it off. When simplefb exits it will
clear this protected status. When the protected status gets cleared
treat it as a ref count decrement and turn off the clock/regulator if
indicated to do so. If a clock is protected, all of it parents get the
protected bit set too.

Protected mode
   you can turn clocks on, but not off
   it is ref counted
  when it hits zero, look at the normal refcount and set that state

Protected mode is not long lived. It only hangs around until the real
device driver loads.

>
> I've been thinking more about this, and I understand that people don't
> want to put hardware description in the simplefb node, but this is
> not hardware description.
>
> u-boot sets up the display-pipeline to scan out of a certain part of
> memory, in order to do this it writes the memory address to some registers
> in the display pipeline, so what it is passing is not hardware description
> (it is not passing all possible memory addresses for the framebuffer), but
> it is passing information about the state in which it has left the display
> pipeline, iow it is passing state information.

Giving the buffer to a piece of hardware is more than setting state.
The hardware now owns that buffer.  That ownership needs to be managed
so that if the hardware decides it doesn't want the buffer it can be
returned to the global pool.

That's why the buffer has to go into that reserved memory section, not
the chosen section. An OS doesn't have to process chosen, it is just
there for informational purposes. To keep the OS from thinking it owns
the memory in that video buffer and can use it for OS purposes, it has
to go into that reserved memory section.

The clocks are different. We know exactly who owns those clocks, the
graphics hardware.

You want something like this where the state of the entire video path
is encoded into the chosen section. But that info is going to vary all
over the place with TV output, HDMI output, LCD panels, etc. simplefb
isn't going to be simple any more. Plus the purposes of adding all of
this complexity is just to handle the half second transition from boot
loader control to real driver.

 reserved-memory {
     #address-cells = <1>;
     #size-cells = <1>;
     ranges;

     display_reserved: framebuffer@78000000 {
         reg = <0x78000000  (1600 * 1200 * 2)>;
     };
 };

 lcd0: lcd-controller@820000 {
     compatible = "marvell,dove-lcd";
     reg = <0x820000 0x1000>;
     interrupts = <47>;
     framebuffer = <&display_reserved>;
     clocks = <&si5351 0>;
     clock-names = "ext_ref_clk_1";
 };

 chosen {
     boot-framebuffer {
        compatible = "simple-framebuffer";
        state {
            device = <&lcd0>;
            width = <1600>;
            height = <1200>;
            stride = <(1600 * 2)>;
            format = "r5g6b5";
            clocks = <&si5351 on 10mhz>;
           output = "hdmi";
           state {
                 device = <&hdmi>
                 clocks = <&xyz on 12mhz>;
          }
     };
 }


>
> Likewise when u-boot sets up the display-pipeline it choices certain
> clocks to use, just like it chooses a framebuffer address, and then
> it programs those clocks into registers. So what we want to pass along
> here is information about the way in which the display-pipeline has
> been configured, so we're passing along state information,
> not hardware description, just like with the framebuffer address.
>
> Regards,
>
> Hans
>
>
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Hans de Goede]

Hi,

On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote:
> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Changes in v2:
>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>> Changes in v3:
>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>>
>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>>>> inaccurate.
>>>>>>>
>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>>>
>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>>>> fix in DT.
>>>>>>>
>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>>>
>>>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>>>> block. Their use is implied by the connection.
>>>>>>
>>>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>>>> information. However, what you are adding IS hardware information and
>>>>>> clearly has a place somewhere else. And adding anything which is not
>>>>>> hardware description gets much more scrutiny.
>>>>>>
>>>>>>> More over it is a bit late to start making objections now. This has already been
>>>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>>>> read the entire thread in the archives under the subject:
>>>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>>>
>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>>>> have made suggestions which I would agree with and you've basically
>>>>>> ignored them.
>>>>>>
>>>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>>>> we can all move on and spend out time in a more productive manner.
>>>>>>
>>>>>> Not an effective argument to get things merged.
>>>>>
>>>>> If there is not good solution to deferring clock clean up in the
>>>>> kernel, another approach is to change how simple-framebuffer is
>>>>> described in the device tree....
>>>>>
>>>>> Right now it is a stand-alone item that looks like a device node, but
>>>>> it isn't a device.
>>>>>
>>>>> framebuffer {
>>>>>     compatible = "simple-framebuffer";
>>>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>>>     width = <1600>;
>>>>>     height = <1200>;
>>>>>     stride = <(1600 * 2)>;
>>>>>     format = "r5g6b5";
>>>>> };
>>>>>
>>>>> How about something like this?
>>>>>
>>>>> reserved-memory {
>>>>>     #address-cells = <1>;
>>>>>     #size-cells = <1>;
>>>>>     ranges;
>>>>>
>>>>>     display_reserved: framebuffer@78000000 {
>>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>     };
>>>>> };
>>>>>
>>>>> lcd0: lcd-controller@820000 {
>>>>>     compatible = "marvell,dove-lcd";
>>>>>     reg = <0x820000 0x1000>;
>>>>>     interrupts = <47>;
>>>>>     clocks = <&si5351 0>;
>>>>>     clock-names = "ext_ref_clk_1";
>>>>> };
>>>>>
>>>>> chosen {
>>>>>     boot-framebuffer {
>>>>>        compatible = "simple-framebuffer";
>>>>>        device = <&lcd0>;
>>>>>        framebuffer = <&display_reserved>;
>>>>>        width = <1600>;
>>>>>        height = <1200>;
>>>>>        stride = <(1600 * 2)>;
>>>>>        format = "r5g6b5";
>>>>>     };
>>>>> }
>>>>>
>>>>>
>>>>> This moves the definition of the boot framebuffer setup into the area
>>>>> where bootloader info is suppose to go. Then you can use the phandle
>>>>> to follow the actual device chains and protect the clocks and
>>>>> regulators. To make that work you are required to provide an accurate
>>>>> description of the real video hardware so that this chain can be
>>>>> followed.
>>>>
>>>> This will not work, first of all multiple blocks may be involved, so
>>>> the device = in the boot-framebuffer would need to be a list. That in
>>>> itself is not a problem, the problem is that the blocks used may have
>>>> multiple clocks, of which the setup mode likely uses only a few.
>>>>
>>>> So if we do things this way, we end up keeping way to many clocks
>>>> enabled.
>>>
>>> That doesn't hurt anything.
>>
>> <snip lots of stuff based on the above>
>>
>> Wrong, that does hurt things. As already discussed simply stopping the
>> clocks from being disabled by the unused_clks mechanism is not enough,
>> since clocks may be shared, and we must stop another device driver
>> sharing the clock and doing clk_enable; probe; clk_disable; disabling
>> the shared clk, which means calling clk_enable on it to mark it as
>> being in use. So this will hurt cause it will cause us to enable a bunch
>> of clks which should not be enabled.
> 
> I said earlier that you would need to add a protected mechanism to
> clocks to handle this phase. When a clock/regulator is protected you
> can turn it on but you can't turn it off. When simplefb exits it will
> clear this protected status. When the protected status gets cleared
> treat it as a ref count decrement and turn off the clock/regulator if
> indicated to do so. If a clock is protected, all of it parents get the
> protected bit set too.
> 
> Protected mode
>    you can turn clocks on, but not off
>    it is ref counted
>   when it hits zero, look at the normal refcount and set that state
> 
> Protected mode is not long lived. It only hangs around until the real
> device driver loads.

And that has already been nacked by the clk maintainer because it is
too complicated, and I agree this is tons more complicated then simply
adding the list of clocks to the simplefb node.

>> I've been thinking more about this, and I understand that people don't
>> want to put hardware description in the simplefb node, but this is
>> not hardware description.
>>
>> u-boot sets up the display-pipeline to scan out of a certain part of
>> memory, in order to do this it writes the memory address to some registers
>> in the display pipeline, so what it is passing is not hardware description
>> (it is not passing all possible memory addresses for the framebuffer), but
>> it is passing information about the state in which it has left the display
>> pipeline, iow it is passing state information.
> 
> Giving the buffer to a piece of hardware is more than setting state.
> The hardware now owns that buffer.  That ownership needs to be managed
> so that if the hardware decides it doesn't want the buffer it can be
> returned to the global pool.
> 
> That's why the buffer has to go into that reserved memory section, not
> the chosen section.

But is not in the reserved memory section, it is in the simplefb
section, and we're stuck with this because of backward compatibility.

 An OS doesn't have to process chosen, it is just
> there for informational purposes. To keep the OS from thinking it owns
> the memory in that video buffer and can use it for OS purposes, it has
> to go into that reserved memory section.
> 
> The clocks are different. We know exactly who owns those clocks, the
> graphics hardware.
> 
> You want something like this where the state of the entire video path
> is encoded into the chosen section. But that info is going to vary all
> over the place with TV output, HDMI output, LCD panels, etc. simplefb
> isn't going to be simple any more. Plus the purposes of adding all of
> this complexity is just to handle the half second transition from boot
> loader control to real driver.
> 
>  reserved-memory {
>      #address-cells = <1>;
>      #size-cells = <1>;
>      ranges;
> 
>      display_reserved: framebuffer@78000000 {
>          reg = <0x78000000  (1600 * 1200 * 2)>;
>      };
>  };
> 
>  lcd0: lcd-controller@820000 {
>      compatible = "marvell,dove-lcd";
>      reg = <0x820000 0x1000>;
>      interrupts = <47>;
>      framebuffer = <&display_reserved>;
>      clocks = <&si5351 0>;
>      clock-names = "ext_ref_clk_1";
>  };
> 
>  chosen {
>      boot-framebuffer {
>         compatible = "simple-framebuffer";
>         state {
>             device = <&lcd0>;
>             width = <1600>;
>             height = <1200>;
>             stride = <(1600 * 2)>;
>             format = "r5g6b5";
>             clocks = <&si5351 on 10mhz>;

Right, so here we get a list of clocks which are actually in use
by the simplefb, just as I've been advocating all the time already.

Note that the clock speed is not necessary, all the kernel needs to
know is that it must not change it.

So as you seem to agree that we need to pass some sort of clock state
info, then all we need to agree on now is where to put it, as said adding
the reg property to a reserved-memory node is out of the question because
of backward compat, likewise storing width, height and format in a state
sub-node are out of the question for the same reason. But other then that
I'm fine with putting the simplefb node under chosen and putting clocks
in there (without the state subnode) as you suggest above.

So we seem to be in agreement here :)

>            output = "hdmi";
>            state {
>                  device = <&hdmi>
>                  clocks = <&xyz on 12mhz>;
>           }

That level of detail won't be necessary, simplefb is supposed to be
simple, the kernel does not need to know which outputs there are, there
will always be only one for simplefb.

Regards,

Hans

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[jonsmirl]

On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote:
>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Changes in v2:
>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>> Changes in v3:
>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>>>
>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>>>>> inaccurate.
>>>>>>>>
>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>>>>
>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>>>>> fix in DT.
>>>>>>>>
>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>>>>
>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>>>>> block. Their use is implied by the connection.
>>>>>>>
>>>>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>>>>> information. However, what you are adding IS hardware information and
>>>>>>> clearly has a place somewhere else. And adding anything which is not
>>>>>>> hardware description gets much more scrutiny.
>>>>>>>
>>>>>>>> More over it is a bit late to start making objections now. This has already been
>>>>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>>>>> read the entire thread in the archives under the subject:
>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>>>>
>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>>>>> have made suggestions which I would agree with and you've basically
>>>>>>> ignored them.
>>>>>>>
>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>>>>> we can all move on and spend out time in a more productive manner.
>>>>>>>
>>>>>>> Not an effective argument to get things merged.
>>>>>>
>>>>>> If there is not good solution to deferring clock clean up in the
>>>>>> kernel, another approach is to change how simple-framebuffer is
>>>>>> described in the device tree....
>>>>>>
>>>>>> Right now it is a stand-alone item that looks like a device node, but
>>>>>> it isn't a device.
>>>>>>
>>>>>> framebuffer {
>>>>>>     compatible = "simple-framebuffer";
>>>>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>>>>     width = <1600>;
>>>>>>     height = <1200>;
>>>>>>     stride = <(1600 * 2)>;
>>>>>>     format = "r5g6b5";
>>>>>> };
>>>>>>
>>>>>> How about something like this?
>>>>>>
>>>>>> reserved-memory {
>>>>>>     #address-cells = <1>;
>>>>>>     #size-cells = <1>;
>>>>>>     ranges;
>>>>>>
>>>>>>     display_reserved: framebuffer@78000000 {
>>>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>>     };
>>>>>> };
>>>>>>
>>>>>> lcd0: lcd-controller@820000 {
>>>>>>     compatible = "marvell,dove-lcd";
>>>>>>     reg = <0x820000 0x1000>;
>>>>>>     interrupts = <47>;
>>>>>>     clocks = <&si5351 0>;
>>>>>>     clock-names = "ext_ref_clk_1";
>>>>>> };
>>>>>>
>>>>>> chosen {
>>>>>>     boot-framebuffer {
>>>>>>        compatible = "simple-framebuffer";
>>>>>>        device = <&lcd0>;
>>>>>>        framebuffer = <&display_reserved>;
>>>>>>        width = <1600>;
>>>>>>        height = <1200>;
>>>>>>        stride = <(1600 * 2)>;
>>>>>>        format = "r5g6b5";
>>>>>>     };
>>>>>> }
>>>>>>
>>>>>>
>>>>>> This moves the definition of the boot framebuffer setup into the area
>>>>>> where bootloader info is suppose to go. Then you can use the phandle
>>>>>> to follow the actual device chains and protect the clocks and
>>>>>> regulators. To make that work you are required to provide an accurate
>>>>>> description of the real video hardware so that this chain can be
>>>>>> followed.
>>>>>
>>>>> This will not work, first of all multiple blocks may be involved, so
>>>>> the device = in the boot-framebuffer would need to be a list. That in
>>>>> itself is not a problem, the problem is that the blocks used may have
>>>>> multiple clocks, of which the setup mode likely uses only a few.
>>>>>
>>>>> So if we do things this way, we end up keeping way to many clocks
>>>>> enabled.
>>>>
>>>> That doesn't hurt anything.
>>>
>>> <snip lots of stuff based on the above>
>>>
>>> Wrong, that does hurt things. As already discussed simply stopping the
>>> clocks from being disabled by the unused_clks mechanism is not enough,
>>> since clocks may be shared, and we must stop another device driver
>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling
>>> the shared clk, which means calling clk_enable on it to mark it as
>>> being in use. So this will hurt cause it will cause us to enable a bunch
>>> of clks which should not be enabled.
>>
>> I said earlier that you would need to add a protected mechanism to
>> clocks to handle this phase. When a clock/regulator is protected you
>> can turn it on but you can't turn it off. When simplefb exits it will
>> clear this protected status. When the protected status gets cleared
>> treat it as a ref count decrement and turn off the clock/regulator if
>> indicated to do so. If a clock is protected, all of it parents get the
>> protected bit set too.
>>
>> Protected mode
>>    you can turn clocks on, but not off
>>    it is ref counted
>>   when it hits zero, look at the normal refcount and set that state
>>
>> Protected mode is not long lived. It only hangs around until the real
>> device driver loads.
>
> And that has already been nacked by the clk maintainer because it is
> too complicated, and I agree this is tons more complicated then simply
> adding the list of clocks to the simplefb node.
>
>>> I've been thinking more about this, and I understand that people don't
>>> want to put hardware description in the simplefb node, but this is
>>> not hardware description.
>>>
>>> u-boot sets up the display-pipeline to scan out of a certain part of
>>> memory, in order to do this it writes the memory address to some registers
>>> in the display pipeline, so what it is passing is not hardware description
>>> (it is not passing all possible memory addresses for the framebuffer), but
>>> it is passing information about the state in which it has left the display
>>> pipeline, iow it is passing state information.
>>
>> Giving the buffer to a piece of hardware is more than setting state.
>> The hardware now owns that buffer.  That ownership needs to be managed
>> so that if the hardware decides it doesn't want the buffer it can be
>> returned to the global pool.
>>
>> That's why the buffer has to go into that reserved memory section, not
>> the chosen section.
>
> But is not in the reserved memory section, it is in the simplefb
> section, and we're stuck with this because of backward compatibility.
>
>  An OS doesn't have to process chosen, it is just
>> there for informational purposes. To keep the OS from thinking it owns
>> the memory in that video buffer and can use it for OS purposes, it has
>> to go into that reserved memory section.
>>
>> The clocks are different. We know exactly who owns those clocks, the
>> graphics hardware.
>>
>> You want something like this where the state of the entire video path
>> is encoded into the chosen section. But that info is going to vary all
>> over the place with TV output, HDMI output, LCD panels, etc. simplefb
>> isn't going to be simple any more. Plus the purposes of adding all of
>> this complexity is just to handle the half second transition from boot
>> loader control to real driver.
>>
>>  reserved-memory {
>>      #address-cells = <1>;
>>      #size-cells = <1>;
>>      ranges;
>>
>>      display_reserved: framebuffer@78000000 {
>>          reg = <0x78000000  (1600 * 1200 * 2)>;
>>      };
>>  };
>>
>>  lcd0: lcd-controller@820000 {
>>      compatible = "marvell,dove-lcd";
>>      reg = <0x820000 0x1000>;
>>      interrupts = <47>;
>>      framebuffer = <&display_reserved>;
>>      clocks = <&si5351 0>;
>>      clock-names = "ext_ref_clk_1";
>>  };
>>
>>  chosen {
>>      boot-framebuffer {
>>         compatible = "simple-framebuffer";
>>         state {
>>             device = <&lcd0>;
>>             width = <1600>;
>>             height = <1200>;
>>             stride = <(1600 * 2)>;
>>             format = "r5g6b5";
>>             clocks = <&si5351 on 10mhz>;
>
> Right, so here we get a list of clocks which are actually in use
> by the simplefb, just as I've been advocating all the time already.
>
> Note that the clock speed is not necessary, all the kernel needs to
> know is that it must not change it.
>
> So as you seem to agree that we need to pass some sort of clock state
> info, then all we need to agree on now is where to put it, as said adding
> the reg property to a reserved-memory node is out of the question because
> of backward compat, likewise storing width, height and format in a state
> sub-node are out of the question for the same reason. But other then that
> I'm fine with putting the simplefb node under chosen and putting clocks
> in there (without the state subnode) as you suggest above.
>
> So we seem to be in agreement here :)
>
>>            output = "hdmi";
>>            state {
>>                  device = <&hdmi>
>>                  clocks = <&xyz on 12mhz>;
>>           }
>
> That level of detail won't be necessary, simplefb is supposed to be
> simple, the kernel does not need to know which outputs there are, there
> will always be only one for simplefb.

I don't agree, but you are going to do it any way so I'll try and help
tkeep problem side effects I know of to a minimum. You are relying on
the BIOS to provide detailed, accurate information. Relying on BIOSes
to do that has historically been a very bad idea.

If you go the way of putting this info into the chosen section you are
going to have to mark the clocks/regulators in use for all of the
outputs too -- hdmi, TV, LCD, backlights, etc, etc. Not going to be
useful if the backlight turns off because simplefb hasn't grabbed it.

This is the only real difference between the proposals - you want
uboot to enumerate what needs to be protected. I don't trust the BIOS
to do that reliably so I'd preferred to just protect everything in the
device hardware chain until the device specific drivers load.

-------------------------------------------------------

I also still believe this is a problem up in Linux that we shouldn't
be using the device tree to fix.

It seems to me that the need for something like a 'protected' mode is
a generic problem that could be extended to all hardware. In protected
mode things can be turned on but nothing can be turned off.  Only
after the kernel has all of the necessary drivers loaded would a small
app run that hits an IOCTL and says, ok protected mode is over now
clean up these things wasting power.

Maybe it should be renamed 'boot' mode. To implement this the various
'turn off' functions would get a 'boot' mode flag. In boot mode they
track the ref counts but don't turn things off when the ref count hits
zero.  Then that ioctl() that the user space app calls runs the chains
of all of the clocks/regulators/etc and if the ref count is zero turns
them off again and clears 'boot' mode. Doesn't matter if you turn off
something again that is already off.




>
> Regards,
>
> Hans



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Hans de Goede]

Hi,

On 10/05/2014 05:07 PM, jonsmirl@gmail.com wrote:
> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote:
>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Changes in v2:
>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>> Changes in v3:
>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>>>>
>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>>>>>> inaccurate.
>>>>>>>>>
>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>>>>>
>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>>>>>> fix in DT.
>>>>>>>>>
>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>>>>>
>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>>>>>> block. Their use is implied by the connection.
>>>>>>>>
>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>>>>>> information. However, what you are adding IS hardware information and
>>>>>>>> clearly has a place somewhere else. And adding anything which is not
>>>>>>>> hardware description gets much more scrutiny.
>>>>>>>>
>>>>>>>>> More over it is a bit late to start making objections now. This has already been
>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>>>>>> read the entire thread in the archives under the subject:
>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>>>>>
>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>>>>>> have made suggestions which I would agree with and you've basically
>>>>>>>> ignored them.
>>>>>>>>
>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>>>>>> we can all move on and spend out time in a more productive manner.
>>>>>>>>
>>>>>>>> Not an effective argument to get things merged.
>>>>>>>
>>>>>>> If there is not good solution to deferring clock clean up in the
>>>>>>> kernel, another approach is to change how simple-framebuffer is
>>>>>>> described in the device tree....
>>>>>>>
>>>>>>> Right now it is a stand-alone item that looks like a device node, but
>>>>>>> it isn't a device.
>>>>>>>
>>>>>>> framebuffer {
>>>>>>>     compatible = "simple-framebuffer";
>>>>>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>>>>>     width = <1600>;
>>>>>>>     height = <1200>;
>>>>>>>     stride = <(1600 * 2)>;
>>>>>>>     format = "r5g6b5";
>>>>>>> };
>>>>>>>
>>>>>>> How about something like this?
>>>>>>>
>>>>>>> reserved-memory {
>>>>>>>     #address-cells = <1>;
>>>>>>>     #size-cells = <1>;
>>>>>>>     ranges;
>>>>>>>
>>>>>>>     display_reserved: framebuffer@78000000 {
>>>>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>>>     };
>>>>>>> };
>>>>>>>
>>>>>>> lcd0: lcd-controller@820000 {
>>>>>>>     compatible = "marvell,dove-lcd";
>>>>>>>     reg = <0x820000 0x1000>;
>>>>>>>     interrupts = <47>;
>>>>>>>     clocks = <&si5351 0>;
>>>>>>>     clock-names = "ext_ref_clk_1";
>>>>>>> };
>>>>>>>
>>>>>>> chosen {
>>>>>>>     boot-framebuffer {
>>>>>>>        compatible = "simple-framebuffer";
>>>>>>>        device = <&lcd0>;
>>>>>>>        framebuffer = <&display_reserved>;
>>>>>>>        width = <1600>;
>>>>>>>        height = <1200>;
>>>>>>>        stride = <(1600 * 2)>;
>>>>>>>        format = "r5g6b5";
>>>>>>>     };
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> This moves the definition of the boot framebuffer setup into the area
>>>>>>> where bootloader info is suppose to go. Then you can use the phandle
>>>>>>> to follow the actual device chains and protect the clocks and
>>>>>>> regulators. To make that work you are required to provide an accurate
>>>>>>> description of the real video hardware so that this chain can be
>>>>>>> followed.
>>>>>>
>>>>>> This will not work, first of all multiple blocks may be involved, so
>>>>>> the device = in the boot-framebuffer would need to be a list. That in
>>>>>> itself is not a problem, the problem is that the blocks used may have
>>>>>> multiple clocks, of which the setup mode likely uses only a few.
>>>>>>
>>>>>> So if we do things this way, we end up keeping way to many clocks
>>>>>> enabled.
>>>>>
>>>>> That doesn't hurt anything.
>>>>
>>>> <snip lots of stuff based on the above>
>>>>
>>>> Wrong, that does hurt things. As already discussed simply stopping the
>>>> clocks from being disabled by the unused_clks mechanism is not enough,
>>>> since clocks may be shared, and we must stop another device driver
>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling
>>>> the shared clk, which means calling clk_enable on it to mark it as
>>>> being in use. So this will hurt cause it will cause us to enable a bunch
>>>> of clks which should not be enabled.
>>>
>>> I said earlier that you would need to add a protected mechanism to
>>> clocks to handle this phase. When a clock/regulator is protected you
>>> can turn it on but you can't turn it off. When simplefb exits it will
>>> clear this protected status. When the protected status gets cleared
>>> treat it as a ref count decrement and turn off the clock/regulator if
>>> indicated to do so. If a clock is protected, all of it parents get the
>>> protected bit set too.
>>>
>>> Protected mode
>>>    you can turn clocks on, but not off
>>>    it is ref counted
>>>   when it hits zero, look at the normal refcount and set that state
>>>
>>> Protected mode is not long lived. It only hangs around until the real
>>> device driver loads.
>>
>> And that has already been nacked by the clk maintainer because it is
>> too complicated, and I agree this is tons more complicated then simply
>> adding the list of clocks to the simplefb node.
>>
>>>> I've been thinking more about this, and I understand that people don't
>>>> want to put hardware description in the simplefb node, but this is
>>>> not hardware description.
>>>>
>>>> u-boot sets up the display-pipeline to scan out of a certain part of
>>>> memory, in order to do this it writes the memory address to some registers
>>>> in the display pipeline, so what it is passing is not hardware description
>>>> (it is not passing all possible memory addresses for the framebuffer), but
>>>> it is passing information about the state in which it has left the display
>>>> pipeline, iow it is passing state information.
>>>
>>> Giving the buffer to a piece of hardware is more than setting state.
>>> The hardware now owns that buffer.  That ownership needs to be managed
>>> so that if the hardware decides it doesn't want the buffer it can be
>>> returned to the global pool.
>>>
>>> That's why the buffer has to go into that reserved memory section, not
>>> the chosen section.
>>
>> But is not in the reserved memory section, it is in the simplefb
>> section, and we're stuck with this because of backward compatibility.
>>
>>  An OS doesn't have to process chosen, it is just
>>> there for informational purposes. To keep the OS from thinking it owns
>>> the memory in that video buffer and can use it for OS purposes, it has
>>> to go into that reserved memory section.
>>>
>>> The clocks are different. We know exactly who owns those clocks, the
>>> graphics hardware.
>>>
>>> You want something like this where the state of the entire video path
>>> is encoded into the chosen section. But that info is going to vary all
>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb
>>> isn't going to be simple any more. Plus the purposes of adding all of
>>> this complexity is just to handle the half second transition from boot
>>> loader control to real driver.
>>>
>>>  reserved-memory {
>>>      #address-cells = <1>;
>>>      #size-cells = <1>;
>>>      ranges;
>>>
>>>      display_reserved: framebuffer@78000000 {
>>>          reg = <0x78000000  (1600 * 1200 * 2)>;
>>>      };
>>>  };
>>>
>>>  lcd0: lcd-controller@820000 {
>>>      compatible = "marvell,dove-lcd";
>>>      reg = <0x820000 0x1000>;
>>>      interrupts = <47>;
>>>      framebuffer = <&display_reserved>;
>>>      clocks = <&si5351 0>;
>>>      clock-names = "ext_ref_clk_1";
>>>  };
>>>
>>>  chosen {
>>>      boot-framebuffer {
>>>         compatible = "simple-framebuffer";
>>>         state {
>>>             device = <&lcd0>;
>>>             width = <1600>;
>>>             height = <1200>;
>>>             stride = <(1600 * 2)>;
>>>             format = "r5g6b5";
>>>             clocks = <&si5351 on 10mhz>;
>>
>> Right, so here we get a list of clocks which are actually in use
>> by the simplefb, just as I've been advocating all the time already.
>>
>> Note that the clock speed is not necessary, all the kernel needs to
>> know is that it must not change it.
>>
>> So as you seem to agree that we need to pass some sort of clock state
>> info, then all we need to agree on now is where to put it, as said adding
>> the reg property to a reserved-memory node is out of the question because
>> of backward compat, likewise storing width, height and format in a state
>> sub-node are out of the question for the same reason. But other then that
>> I'm fine with putting the simplefb node under chosen and putting clocks
>> in there (without the state subnode) as you suggest above.
>>
>> So we seem to be in agreement here :)
>>
>>>            output = "hdmi";
>>>            state {
>>>                  device = <&hdmi>
>>>                  clocks = <&xyz on 12mhz>;
>>>           }
>>
>> That level of detail won't be necessary, simplefb is supposed to be
>> simple, the kernel does not need to know which outputs there are, there
>> will always be only one for simplefb.
> 
> I don't agree, but you are going to do it any way so I'll try and help
> tkeep problem side effects I know of to a minimum. You are relying on
> the BIOS to provide detailed, accurate information. Relying on BIOSes
> to do that has historically been a very bad idea.

And here we come to the gist of your objections, I don't agree for
various reasons:

1) Despite firmware sometimes having bugs, we simply have to trust the firmware,
e.g. the reg property of simplefb could be set to point to some mmio rather then
just the framebuffer, and writing that mmio could do real bad things, yet we trust
it not to do that.

2) The firmware we're talking about here, at least for now, is u-boot which
is fully FOSS and under our control.

3) "I don't trust foo" is not a technical argument, and decisions like this
should be made on technical arguments.

Regards,

Hans

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[jonsmirl]

On Sun, Oct 5, 2014 at 11:16 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 10/05/2014 05:07 PM, jonsmirl@gmail.com wrote:
>> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote:
>>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
>>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>>>>>
>>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>>>>>>> inaccurate.
>>>>>>>>>>
>>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>>>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>>>>>>
>>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>>>>>>> fix in DT.
>>>>>>>>>>
>>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>>>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>>>>>>
>>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>>>>>>> block. Their use is implied by the connection.
>>>>>>>>>
>>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>>>>>>> information. However, what you are adding IS hardware information and
>>>>>>>>> clearly has a place somewhere else. And adding anything which is not
>>>>>>>>> hardware description gets much more scrutiny.
>>>>>>>>>
>>>>>>>>>> More over it is a bit late to start making objections now. This has already been
>>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>>>>>>> read the entire thread in the archives under the subject:
>>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>>>>>>
>>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>>>>>>> have made suggestions which I would agree with and you've basically
>>>>>>>>> ignored them.
>>>>>>>>>
>>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>>>>>>> we can all move on and spend out time in a more productive manner.
>>>>>>>>>
>>>>>>>>> Not an effective argument to get things merged.
>>>>>>>>
>>>>>>>> If there is not good solution to deferring clock clean up in the
>>>>>>>> kernel, another approach is to change how simple-framebuffer is
>>>>>>>> described in the device tree....
>>>>>>>>
>>>>>>>> Right now it is a stand-alone item that looks like a device node, but
>>>>>>>> it isn't a device.
>>>>>>>>
>>>>>>>> framebuffer {
>>>>>>>>     compatible = "simple-framebuffer";
>>>>>>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>>>>>>     width = <1600>;
>>>>>>>>     height = <1200>;
>>>>>>>>     stride = <(1600 * 2)>;
>>>>>>>>     format = "r5g6b5";
>>>>>>>> };
>>>>>>>>
>>>>>>>> How about something like this?
>>>>>>>>
>>>>>>>> reserved-memory {
>>>>>>>>     #address-cells = <1>;
>>>>>>>>     #size-cells = <1>;
>>>>>>>>     ranges;
>>>>>>>>
>>>>>>>>     display_reserved: framebuffer@78000000 {
>>>>>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>>>>     };
>>>>>>>> };
>>>>>>>>
>>>>>>>> lcd0: lcd-controller@820000 {
>>>>>>>>     compatible = "marvell,dove-lcd";
>>>>>>>>     reg = <0x820000 0x1000>;
>>>>>>>>     interrupts = <47>;
>>>>>>>>     clocks = <&si5351 0>;
>>>>>>>>     clock-names = "ext_ref_clk_1";
>>>>>>>> };
>>>>>>>>
>>>>>>>> chosen {
>>>>>>>>     boot-framebuffer {
>>>>>>>>        compatible = "simple-framebuffer";
>>>>>>>>        device = <&lcd0>;
>>>>>>>>        framebuffer = <&display_reserved>;
>>>>>>>>        width = <1600>;
>>>>>>>>        height = <1200>;
>>>>>>>>        stride = <(1600 * 2)>;
>>>>>>>>        format = "r5g6b5";
>>>>>>>>     };
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> This moves the definition of the boot framebuffer setup into the area
>>>>>>>> where bootloader info is suppose to go. Then you can use the phandle
>>>>>>>> to follow the actual device chains and protect the clocks and
>>>>>>>> regulators. To make that work you are required to provide an accurate
>>>>>>>> description of the real video hardware so that this chain can be
>>>>>>>> followed.
>>>>>>>
>>>>>>> This will not work, first of all multiple blocks may be involved, so
>>>>>>> the device = in the boot-framebuffer would need to be a list. That in
>>>>>>> itself is not a problem, the problem is that the blocks used may have
>>>>>>> multiple clocks, of which the setup mode likely uses only a few.
>>>>>>>
>>>>>>> So if we do things this way, we end up keeping way to many clocks
>>>>>>> enabled.
>>>>>>
>>>>>> That doesn't hurt anything.
>>>>>
>>>>> <snip lots of stuff based on the above>
>>>>>
>>>>> Wrong, that does hurt things. As already discussed simply stopping the
>>>>> clocks from being disabled by the unused_clks mechanism is not enough,
>>>>> since clocks may be shared, and we must stop another device driver
>>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling
>>>>> the shared clk, which means calling clk_enable on it to mark it as
>>>>> being in use. So this will hurt cause it will cause us to enable a bunch
>>>>> of clks which should not be enabled.
>>>>
>>>> I said earlier that you would need to add a protected mechanism to
>>>> clocks to handle this phase. When a clock/regulator is protected you
>>>> can turn it on but you can't turn it off. When simplefb exits it will
>>>> clear this protected status. When the protected status gets cleared
>>>> treat it as a ref count decrement and turn off the clock/regulator if
>>>> indicated to do so. If a clock is protected, all of it parents get the
>>>> protected bit set too.
>>>>
>>>> Protected mode
>>>>    you can turn clocks on, but not off
>>>>    it is ref counted
>>>>   when it hits zero, look at the normal refcount and set that state
>>>>
>>>> Protected mode is not long lived. It only hangs around until the real
>>>> device driver loads.
>>>
>>> And that has already been nacked by the clk maintainer because it is
>>> too complicated, and I agree this is tons more complicated then simply
>>> adding the list of clocks to the simplefb node.
>>>
>>>>> I've been thinking more about this, and I understand that people don't
>>>>> want to put hardware description in the simplefb node, but this is
>>>>> not hardware description.
>>>>>
>>>>> u-boot sets up the display-pipeline to scan out of a certain part of
>>>>> memory, in order to do this it writes the memory address to some registers
>>>>> in the display pipeline, so what it is passing is not hardware description
>>>>> (it is not passing all possible memory addresses for the framebuffer), but
>>>>> it is passing information about the state in which it has left the display
>>>>> pipeline, iow it is passing state information.
>>>>
>>>> Giving the buffer to a piece of hardware is more than setting state.
>>>> The hardware now owns that buffer.  That ownership needs to be managed
>>>> so that if the hardware decides it doesn't want the buffer it can be
>>>> returned to the global pool.
>>>>
>>>> That's why the buffer has to go into that reserved memory section, not
>>>> the chosen section.
>>>
>>> But is not in the reserved memory section, it is in the simplefb
>>> section, and we're stuck with this because of backward compatibility.
>>>
>>>  An OS doesn't have to process chosen, it is just
>>>> there for informational purposes. To keep the OS from thinking it owns
>>>> the memory in that video buffer and can use it for OS purposes, it has
>>>> to go into that reserved memory section.
>>>>
>>>> The clocks are different. We know exactly who owns those clocks, the
>>>> graphics hardware.
>>>>
>>>> You want something like this where the state of the entire video path
>>>> is encoded into the chosen section. But that info is going to vary all
>>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb
>>>> isn't going to be simple any more. Plus the purposes of adding all of
>>>> this complexity is just to handle the half second transition from boot
>>>> loader control to real driver.
>>>>
>>>>  reserved-memory {
>>>>      #address-cells = <1>;
>>>>      #size-cells = <1>;
>>>>      ranges;
>>>>
>>>>      display_reserved: framebuffer@78000000 {
>>>>          reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>      };
>>>>  };
>>>>
>>>>  lcd0: lcd-controller@820000 {
>>>>      compatible = "marvell,dove-lcd";
>>>>      reg = <0x820000 0x1000>;
>>>>      interrupts = <47>;
>>>>      framebuffer = <&display_reserved>;
>>>>      clocks = <&si5351 0>;
>>>>      clock-names = "ext_ref_clk_1";
>>>>  };
>>>>
>>>>  chosen {
>>>>      boot-framebuffer {
>>>>         compatible = "simple-framebuffer";
>>>>         state {
>>>>             device = <&lcd0>;
>>>>             width = <1600>;
>>>>             height = <1200>;
>>>>             stride = <(1600 * 2)>;
>>>>             format = "r5g6b5";
>>>>             clocks = <&si5351 on 10mhz>;
>>>
>>> Right, so here we get a list of clocks which are actually in use
>>> by the simplefb, just as I've been advocating all the time already.
>>>
>>> Note that the clock speed is not necessary, all the kernel needs to
>>> know is that it must not change it.
>>>
>>> So as you seem to agree that we need to pass some sort of clock state
>>> info, then all we need to agree on now is where to put it, as said adding
>>> the reg property to a reserved-memory node is out of the question because
>>> of backward compat, likewise storing width, height and format in a state
>>> sub-node are out of the question for the same reason. But other then that
>>> I'm fine with putting the simplefb node under chosen and putting clocks
>>> in there (without the state subnode) as you suggest above.
>>>
>>> So we seem to be in agreement here :)
>>>
>>>>            output = "hdmi";
>>>>            state {
>>>>                  device = <&hdmi>
>>>>                  clocks = <&xyz on 12mhz>;
>>>>           }
>>>
>>> That level of detail won't be necessary, simplefb is supposed to be
>>> simple, the kernel does not need to know which outputs there are, there
>>> will always be only one for simplefb.
>>
>> I don't agree, but you are going to do it any way so I'll try and help
>> tkeep problem side effects I know of to a minimum. You are relying on
>> the BIOS to provide detailed, accurate information. Relying on BIOSes
>> to do that has historically been a very bad idea.
>
> And here we come to the gist of your objections, I don't agree for
> various reasons:
>
> 1) Despite firmware sometimes having bugs, we simply have to trust the firmware,
> e.g. the reg property of simplefb could be set to point to some mmio rather then
> just the framebuffer, and writing that mmio could do real bad things, yet we trust
> it not to do that.
>
> 2) The firmware we're talking about here, at least for now, is u-boot which
> is fully FOSS and under our control.
>
> 3) "I don't trust foo" is not a technical argument, and decisions like this
> should be made on technical arguments.

You also realize that uboot now is going to have to be compiled with
the same DTS that the kernel is using so that the phandles will match
up. And now you are going to have to recompile your uboot everytime
that DTS changes.


>
> Regards,
>
> Hans



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Hans de Goede]

Hi,

On 10/05/2014 05:17 PM, jonsmirl@gmail.com wrote:
> On Sun, Oct 5, 2014 at 11:16 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 10/05/2014 05:07 PM, jonsmirl@gmail.com wrote:
>>> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote:
>>>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
>>>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>>>>>>
>>>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>>>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>>>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>>>>>>>> inaccurate.
>>>>>>>>>>>
>>>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>>>>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>>>>>>>
>>>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>>>>>>>> fix in DT.
>>>>>>>>>>>
>>>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>>>>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>>>>>>>
>>>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>>>>>>>> block. Their use is implied by the connection.
>>>>>>>>>>
>>>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>>>>>>>> information. However, what you are adding IS hardware information and
>>>>>>>>>> clearly has a place somewhere else. And adding anything which is not
>>>>>>>>>> hardware description gets much more scrutiny.
>>>>>>>>>>
>>>>>>>>>>> More over it is a bit late to start making objections now. This has already been
>>>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>>>>>>>> read the entire thread in the archives under the subject:
>>>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>>>>>>>
>>>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>>>>>>>> have made suggestions which I would agree with and you've basically
>>>>>>>>>> ignored them.
>>>>>>>>>>
>>>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>>>>>>>> we can all move on and spend out time in a more productive manner.
>>>>>>>>>>
>>>>>>>>>> Not an effective argument to get things merged.
>>>>>>>>>
>>>>>>>>> If there is not good solution to deferring clock clean up in the
>>>>>>>>> kernel, another approach is to change how simple-framebuffer is
>>>>>>>>> described in the device tree....
>>>>>>>>>
>>>>>>>>> Right now it is a stand-alone item that looks like a device node, but
>>>>>>>>> it isn't a device.
>>>>>>>>>
>>>>>>>>> framebuffer {
>>>>>>>>>     compatible = "simple-framebuffer";
>>>>>>>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>>>>>>>     width = <1600>;
>>>>>>>>>     height = <1200>;
>>>>>>>>>     stride = <(1600 * 2)>;
>>>>>>>>>     format = "r5g6b5";
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> How about something like this?
>>>>>>>>>
>>>>>>>>> reserved-memory {
>>>>>>>>>     #address-cells = <1>;
>>>>>>>>>     #size-cells = <1>;
>>>>>>>>>     ranges;
>>>>>>>>>
>>>>>>>>>     display_reserved: framebuffer@78000000 {
>>>>>>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>>>>>     };
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> lcd0: lcd-controller@820000 {
>>>>>>>>>     compatible = "marvell,dove-lcd";
>>>>>>>>>     reg = <0x820000 0x1000>;
>>>>>>>>>     interrupts = <47>;
>>>>>>>>>     clocks = <&si5351 0>;
>>>>>>>>>     clock-names = "ext_ref_clk_1";
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> chosen {
>>>>>>>>>     boot-framebuffer {
>>>>>>>>>        compatible = "simple-framebuffer";
>>>>>>>>>        device = <&lcd0>;
>>>>>>>>>        framebuffer = <&display_reserved>;
>>>>>>>>>        width = <1600>;
>>>>>>>>>        height = <1200>;
>>>>>>>>>        stride = <(1600 * 2)>;
>>>>>>>>>        format = "r5g6b5";
>>>>>>>>>     };
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This moves the definition of the boot framebuffer setup into the area
>>>>>>>>> where bootloader info is suppose to go. Then you can use the phandle
>>>>>>>>> to follow the actual device chains and protect the clocks and
>>>>>>>>> regulators. To make that work you are required to provide an accurate
>>>>>>>>> description of the real video hardware so that this chain can be
>>>>>>>>> followed.
>>>>>>>>
>>>>>>>> This will not work, first of all multiple blocks may be involved, so
>>>>>>>> the device = in the boot-framebuffer would need to be a list. That in
>>>>>>>> itself is not a problem, the problem is that the blocks used may have
>>>>>>>> multiple clocks, of which the setup mode likely uses only a few.
>>>>>>>>
>>>>>>>> So if we do things this way, we end up keeping way to many clocks
>>>>>>>> enabled.
>>>>>>>
>>>>>>> That doesn't hurt anything.
>>>>>>
>>>>>> <snip lots of stuff based on the above>
>>>>>>
>>>>>> Wrong, that does hurt things. As already discussed simply stopping the
>>>>>> clocks from being disabled by the unused_clks mechanism is not enough,
>>>>>> since clocks may be shared, and we must stop another device driver
>>>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling
>>>>>> the shared clk, which means calling clk_enable on it to mark it as
>>>>>> being in use. So this will hurt cause it will cause us to enable a bunch
>>>>>> of clks which should not be enabled.
>>>>>
>>>>> I said earlier that you would need to add a protected mechanism to
>>>>> clocks to handle this phase. When a clock/regulator is protected you
>>>>> can turn it on but you can't turn it off. When simplefb exits it will
>>>>> clear this protected status. When the protected status gets cleared
>>>>> treat it as a ref count decrement and turn off the clock/regulator if
>>>>> indicated to do so. If a clock is protected, all of it parents get the
>>>>> protected bit set too.
>>>>>
>>>>> Protected mode
>>>>>    you can turn clocks on, but not off
>>>>>    it is ref counted
>>>>>   when it hits zero, look at the normal refcount and set that state
>>>>>
>>>>> Protected mode is not long lived. It only hangs around until the real
>>>>> device driver loads.
>>>>
>>>> And that has already been nacked by the clk maintainer because it is
>>>> too complicated, and I agree this is tons more complicated then simply
>>>> adding the list of clocks to the simplefb node.
>>>>
>>>>>> I've been thinking more about this, and I understand that people don't
>>>>>> want to put hardware description in the simplefb node, but this is
>>>>>> not hardware description.
>>>>>>
>>>>>> u-boot sets up the display-pipeline to scan out of a certain part of
>>>>>> memory, in order to do this it writes the memory address to some registers
>>>>>> in the display pipeline, so what it is passing is not hardware description
>>>>>> (it is not passing all possible memory addresses for the framebuffer), but
>>>>>> it is passing information about the state in which it has left the display
>>>>>> pipeline, iow it is passing state information.
>>>>>
>>>>> Giving the buffer to a piece of hardware is more than setting state.
>>>>> The hardware now owns that buffer.  That ownership needs to be managed
>>>>> so that if the hardware decides it doesn't want the buffer it can be
>>>>> returned to the global pool.
>>>>>
>>>>> That's why the buffer has to go into that reserved memory section, not
>>>>> the chosen section.
>>>>
>>>> But is not in the reserved memory section, it is in the simplefb
>>>> section, and we're stuck with this because of backward compatibility.
>>>>
>>>>  An OS doesn't have to process chosen, it is just
>>>>> there for informational purposes. To keep the OS from thinking it owns
>>>>> the memory in that video buffer and can use it for OS purposes, it has
>>>>> to go into that reserved memory section.
>>>>>
>>>>> The clocks are different. We know exactly who owns those clocks, the
>>>>> graphics hardware.
>>>>>
>>>>> You want something like this where the state of the entire video path
>>>>> is encoded into the chosen section. But that info is going to vary all
>>>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb
>>>>> isn't going to be simple any more. Plus the purposes of adding all of
>>>>> this complexity is just to handle the half second transition from boot
>>>>> loader control to real driver.
>>>>>
>>>>>  reserved-memory {
>>>>>      #address-cells = <1>;
>>>>>      #size-cells = <1>;
>>>>>      ranges;
>>>>>
>>>>>      display_reserved: framebuffer@78000000 {
>>>>>          reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>      };
>>>>>  };
>>>>>
>>>>>  lcd0: lcd-controller@820000 {
>>>>>      compatible = "marvell,dove-lcd";
>>>>>      reg = <0x820000 0x1000>;
>>>>>      interrupts = <47>;
>>>>>      framebuffer = <&display_reserved>;
>>>>>      clocks = <&si5351 0>;
>>>>>      clock-names = "ext_ref_clk_1";
>>>>>  };
>>>>>
>>>>>  chosen {
>>>>>      boot-framebuffer {
>>>>>         compatible = "simple-framebuffer";
>>>>>         state {
>>>>>             device = <&lcd0>;
>>>>>             width = <1600>;
>>>>>             height = <1200>;
>>>>>             stride = <(1600 * 2)>;
>>>>>             format = "r5g6b5";
>>>>>             clocks = <&si5351 on 10mhz>;
>>>>
>>>> Right, so here we get a list of clocks which are actually in use
>>>> by the simplefb, just as I've been advocating all the time already.
>>>>
>>>> Note that the clock speed is not necessary, all the kernel needs to
>>>> know is that it must not change it.
>>>>
>>>> So as you seem to agree that we need to pass some sort of clock state
>>>> info, then all we need to agree on now is where to put it, as said adding
>>>> the reg property to a reserved-memory node is out of the question because
>>>> of backward compat, likewise storing width, height and format in a state
>>>> sub-node are out of the question for the same reason. But other then that
>>>> I'm fine with putting the simplefb node under chosen and putting clocks
>>>> in there (without the state subnode) as you suggest above.
>>>>
>>>> So we seem to be in agreement here :)
>>>>
>>>>>            output = "hdmi";
>>>>>            state {
>>>>>                  device = <&hdmi>
>>>>>                  clocks = <&xyz on 12mhz>;
>>>>>           }
>>>>
>>>> That level of detail won't be necessary, simplefb is supposed to be
>>>> simple, the kernel does not need to know which outputs there are, there
>>>> will always be only one for simplefb.
>>>
>>> I don't agree, but you are going to do it any way so I'll try and help
>>> tkeep problem side effects I know of to a minimum. You are relying on
>>> the BIOS to provide detailed, accurate information. Relying on BIOSes
>>> to do that has historically been a very bad idea.
>>
>> And here we come to the gist of your objections, I don't agree for
>> various reasons:
>>
>> 1) Despite firmware sometimes having bugs, we simply have to trust the firmware,
>> e.g. the reg property of simplefb could be set to point to some mmio rather then
>> just the framebuffer, and writing that mmio could do real bad things, yet we trust
>> it not to do that.
>>
>> 2) The firmware we're talking about here, at least for now, is u-boot which
>> is fully FOSS and under our control.
>>
>> 3) "I don't trust foo" is not a technical argument, and decisions like this
>> should be made on technical arguments.
> 
> You also realize that uboot now is going to have to be compiled with
> the same DTS that the kernel is using so that the phandles will match
> up. And now you are going to have to recompile your uboot everytime
> that DTS changes.

That is simply not true. Please stop spreading FUD about this without actually
knowing how it works / having read the code.

Regards,

Hans

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Chen-Yu Tsai]

On Sun, Oct 5, 2014 at 11:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote:
>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Changes in v2:
>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>> Changes in v3:
>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>>>>
>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>>>>>> inaccurate.
>>>>>>>>>
>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>>>>>
>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>>>>>> fix in DT.
>>>>>>>>>
>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>>>>>
>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>>>>>> block. Their use is implied by the connection.
>>>>>>>>
>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>>>>>> information. However, what you are adding IS hardware information and
>>>>>>>> clearly has a place somewhere else. And adding anything which is not
>>>>>>>> hardware description gets much more scrutiny.
>>>>>>>>
>>>>>>>>> More over it is a bit late to start making objections now. This has already been
>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>>>>>> read the entire thread in the archives under the subject:
>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>>>>>
>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>>>>>> have made suggestions which I would agree with and you've basically
>>>>>>>> ignored them.
>>>>>>>>
>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>>>>>> we can all move on and spend out time in a more productive manner.
>>>>>>>>
>>>>>>>> Not an effective argument to get things merged.
>>>>>>>
>>>>>>> If there is not good solution to deferring clock clean up in the
>>>>>>> kernel, another approach is to change how simple-framebuffer is
>>>>>>> described in the device tree....
>>>>>>>
>>>>>>> Right now it is a stand-alone item that looks like a device node, but
>>>>>>> it isn't a device.
>>>>>>>
>>>>>>> framebuffer {
>>>>>>>     compatible = "simple-framebuffer";
>>>>>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>>>>>     width = <1600>;
>>>>>>>     height = <1200>;
>>>>>>>     stride = <(1600 * 2)>;
>>>>>>>     format = "r5g6b5";
>>>>>>> };
>>>>>>>
>>>>>>> How about something like this?
>>>>>>>
>>>>>>> reserved-memory {
>>>>>>>     #address-cells = <1>;
>>>>>>>     #size-cells = <1>;
>>>>>>>     ranges;
>>>>>>>
>>>>>>>     display_reserved: framebuffer@78000000 {
>>>>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>>>     };
>>>>>>> };
>>>>>>>
>>>>>>> lcd0: lcd-controller@820000 {
>>>>>>>     compatible = "marvell,dove-lcd";
>>>>>>>     reg = <0x820000 0x1000>;
>>>>>>>     interrupts = <47>;
>>>>>>>     clocks = <&si5351 0>;
>>>>>>>     clock-names = "ext_ref_clk_1";
>>>>>>> };
>>>>>>>
>>>>>>> chosen {
>>>>>>>     boot-framebuffer {
>>>>>>>        compatible = "simple-framebuffer";
>>>>>>>        device = <&lcd0>;
>>>>>>>        framebuffer = <&display_reserved>;
>>>>>>>        width = <1600>;
>>>>>>>        height = <1200>;
>>>>>>>        stride = <(1600 * 2)>;
>>>>>>>        format = "r5g6b5";
>>>>>>>     };
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> This moves the definition of the boot framebuffer setup into the area
>>>>>>> where bootloader info is suppose to go. Then you can use the phandle
>>>>>>> to follow the actual device chains and protect the clocks and
>>>>>>> regulators. To make that work you are required to provide an accurate
>>>>>>> description of the real video hardware so that this chain can be
>>>>>>> followed.
>>>>>>
>>>>>> This will not work, first of all multiple blocks may be involved, so
>>>>>> the device = in the boot-framebuffer would need to be a list. That in
>>>>>> itself is not a problem, the problem is that the blocks used may have
>>>>>> multiple clocks, of which the setup mode likely uses only a few.
>>>>>>
>>>>>> So if we do things this way, we end up keeping way to many clocks
>>>>>> enabled.
>>>>>
>>>>> That doesn't hurt anything.
>>>>
>>>> <snip lots of stuff based on the above>
>>>>
>>>> Wrong, that does hurt things. As already discussed simply stopping the
>>>> clocks from being disabled by the unused_clks mechanism is not enough,
>>>> since clocks may be shared, and we must stop another device driver
>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling
>>>> the shared clk, which means calling clk_enable on it to mark it as
>>>> being in use. So this will hurt cause it will cause us to enable a bunch
>>>> of clks which should not be enabled.
>>>
>>> I said earlier that you would need to add a protected mechanism to
>>> clocks to handle this phase. When a clock/regulator is protected you
>>> can turn it on but you can't turn it off. When simplefb exits it will
>>> clear this protected status. When the protected status gets cleared
>>> treat it as a ref count decrement and turn off the clock/regulator if
>>> indicated to do so. If a clock is protected, all of it parents get the
>>> protected bit set too.
>>>
>>> Protected mode
>>>    you can turn clocks on, but not off
>>>    it is ref counted
>>>   when it hits zero, look at the normal refcount and set that state
>>>
>>> Protected mode is not long lived. It only hangs around until the real
>>> device driver loads.
>>
>> And that has already been nacked by the clk maintainer because it is
>> too complicated, and I agree this is tons more complicated then simply
>> adding the list of clocks to the simplefb node.
>>
>>>> I've been thinking more about this, and I understand that people don't
>>>> want to put hardware description in the simplefb node, but this is
>>>> not hardware description.
>>>>
>>>> u-boot sets up the display-pipeline to scan out of a certain part of
>>>> memory, in order to do this it writes the memory address to some registers
>>>> in the display pipeline, so what it is passing is not hardware description
>>>> (it is not passing all possible memory addresses for the framebuffer), but
>>>> it is passing information about the state in which it has left the display
>>>> pipeline, iow it is passing state information.
>>>
>>> Giving the buffer to a piece of hardware is more than setting state.
>>> The hardware now owns that buffer.  That ownership needs to be managed
>>> so that if the hardware decides it doesn't want the buffer it can be
>>> returned to the global pool.
>>>
>>> That's why the buffer has to go into that reserved memory section, not
>>> the chosen section.
>>
>> But is not in the reserved memory section, it is in the simplefb
>> section, and we're stuck with this because of backward compatibility.
>>
>>  An OS doesn't have to process chosen, it is just
>>> there for informational purposes. To keep the OS from thinking it owns
>>> the memory in that video buffer and can use it for OS purposes, it has
>>> to go into that reserved memory section.
>>>
>>> The clocks are different. We know exactly who owns those clocks, the
>>> graphics hardware.
>>>
>>> You want something like this where the state of the entire video path
>>> is encoded into the chosen section. But that info is going to vary all
>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb
>>> isn't going to be simple any more. Plus the purposes of adding all of
>>> this complexity is just to handle the half second transition from boot
>>> loader control to real driver.
>>>
>>>  reserved-memory {
>>>      #address-cells = <1>;
>>>      #size-cells = <1>;
>>>      ranges;
>>>
>>>      display_reserved: framebuffer@78000000 {
>>>          reg = <0x78000000  (1600 * 1200 * 2)>;
>>>      };
>>>  };
>>>
>>>  lcd0: lcd-controller@820000 {
>>>      compatible = "marvell,dove-lcd";
>>>      reg = <0x820000 0x1000>;
>>>      interrupts = <47>;
>>>      framebuffer = <&display_reserved>;
>>>      clocks = <&si5351 0>;
>>>      clock-names = "ext_ref_clk_1";
>>>  };
>>>
>>>  chosen {
>>>      boot-framebuffer {
>>>         compatible = "simple-framebuffer";
>>>         state {
>>>             device = <&lcd0>;
>>>             width = <1600>;
>>>             height = <1200>;
>>>             stride = <(1600 * 2)>;
>>>             format = "r5g6b5";
>>>             clocks = <&si5351 on 10mhz>;
>>
>> Right, so here we get a list of clocks which are actually in use
>> by the simplefb, just as I've been advocating all the time already.
>>
>> Note that the clock speed is not necessary, all the kernel needs to
>> know is that it must not change it.
>>
>> So as you seem to agree that we need to pass some sort of clock state
>> info, then all we need to agree on now is where to put it, as said adding
>> the reg property to a reserved-memory node is out of the question because
>> of backward compat, likewise storing width, height and format in a state
>> sub-node are out of the question for the same reason. But other then that
>> I'm fine with putting the simplefb node under chosen and putting clocks
>> in there (without the state subnode) as you suggest above.
>>
>> So we seem to be in agreement here :)
>>
>>>            output = "hdmi";
>>>            state {
>>>                  device = <&hdmi>
>>>                  clocks = <&xyz on 12mhz>;
>>>           }
>>
>> That level of detail won't be necessary, simplefb is supposed to be
>> simple, the kernel does not need to know which outputs there are, there
>> will always be only one for simplefb.
>
> I don't agree, but you are going to do it any way so I'll try and help
> tkeep problem side effects I know of to a minimum. You are relying on
> the BIOS to provide detailed, accurate information. Relying on BIOSes
> to do that has historically been a very bad idea.
>
> If you go the way of putting this info into the chosen section you are
> going to have to mark the clocks/regulators in use for all of the
> outputs too -- hdmi, TV, LCD, backlights, etc, etc. Not going to be
> useful if the backlight turns off because simplefb hasn't grabbed it.
>
> This is the only real difference between the proposals - you want
> uboot to enumerate what needs to be protected. I don't trust the BIOS
> to do that reliably so I'd preferred to just protect everything in the
> device hardware chain until the device specific drivers load.
>
> -------------------------------------------------------
>
> I also still believe this is a problem up in Linux that we shouldn't
> be using the device tree to fix.
>
> It seems to me that the need for something like a 'protected' mode is
> a generic problem that could be extended to all hardware. In protected
> mode things can be turned on but nothing can be turned off.  Only
> after the kernel has all of the necessary drivers loaded would a small
> app run that hits an IOCTL and says, ok protected mode is over now
> clean up these things wasting power.

What happens if some clock needs to be disabled?
Like clocks that must be gated before setting a new clock rate
or reparenting. The kernel supports these, and it wouldn't surprise me
if some driver actually requires this. You might end up blocking that driver
or breaking its probe function.

And what if reset controls are asserted then de-asserted in the probe function?
IIRC there are drivers that do this to reset the underlying hardware.

> Maybe it should be renamed 'boot' mode. To implement this the various
> 'turn off' functions would get a 'boot' mode flag. In boot mode they
> track the ref counts but don't turn things off when the ref count hits
> zero.  Then that ioctl() that the user space app calls runs the chains
> of all of the clocks/regulators/etc and if the ref count is zero turns
> them off again and clears 'boot' mode. Doesn't matter if you turn off
> something again that is already off.

And what if something just happened to be left on that some driver
wants to turn off? You are assuming everything not used is off,
which is not always the case.


ChenYu

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[jonsmirl]

On Sun, Oct 5, 2014 at 11:17 AM, Chen-Yu Tsai <wens@csie.org> wrote:
> On Sun, Oct 5, 2014 at 11:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote:
>>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
>>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>>>>>
>>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>>>>>>> inaccurate.
>>>>>>>>>>
>>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>>>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>>>>>>
>>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>>>>>>> fix in DT.
>>>>>>>>>>
>>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>>>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>>>>>>
>>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>>>>>>> block. Their use is implied by the connection.
>>>>>>>>>
>>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>>>>>>> information. However, what you are adding IS hardware information and
>>>>>>>>> clearly has a place somewhere else. And adding anything which is not
>>>>>>>>> hardware description gets much more scrutiny.
>>>>>>>>>
>>>>>>>>>> More over it is a bit late to start making objections now. This has already been
>>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>>>>>>> read the entire thread in the archives under the subject:
>>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>>>>>>
>>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>>>>>>> have made suggestions which I would agree with and you've basically
>>>>>>>>> ignored them.
>>>>>>>>>
>>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>>>>>>> we can all move on and spend out time in a more productive manner.
>>>>>>>>>
>>>>>>>>> Not an effective argument to get things merged.
>>>>>>>>
>>>>>>>> If there is not good solution to deferring clock clean up in the
>>>>>>>> kernel, another approach is to change how simple-framebuffer is
>>>>>>>> described in the device tree....
>>>>>>>>
>>>>>>>> Right now it is a stand-alone item that looks like a device node, but
>>>>>>>> it isn't a device.
>>>>>>>>
>>>>>>>> framebuffer {
>>>>>>>>     compatible = "simple-framebuffer";
>>>>>>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>>>>>>     width = <1600>;
>>>>>>>>     height = <1200>;
>>>>>>>>     stride = <(1600 * 2)>;
>>>>>>>>     format = "r5g6b5";
>>>>>>>> };
>>>>>>>>
>>>>>>>> How about something like this?
>>>>>>>>
>>>>>>>> reserved-memory {
>>>>>>>>     #address-cells = <1>;
>>>>>>>>     #size-cells = <1>;
>>>>>>>>     ranges;
>>>>>>>>
>>>>>>>>     display_reserved: framebuffer@78000000 {
>>>>>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>>>>     };
>>>>>>>> };
>>>>>>>>
>>>>>>>> lcd0: lcd-controller@820000 {
>>>>>>>>     compatible = "marvell,dove-lcd";
>>>>>>>>     reg = <0x820000 0x1000>;
>>>>>>>>     interrupts = <47>;
>>>>>>>>     clocks = <&si5351 0>;
>>>>>>>>     clock-names = "ext_ref_clk_1";
>>>>>>>> };
>>>>>>>>
>>>>>>>> chosen {
>>>>>>>>     boot-framebuffer {
>>>>>>>>        compatible = "simple-framebuffer";
>>>>>>>>        device = <&lcd0>;
>>>>>>>>        framebuffer = <&display_reserved>;
>>>>>>>>        width = <1600>;
>>>>>>>>        height = <1200>;
>>>>>>>>        stride = <(1600 * 2)>;
>>>>>>>>        format = "r5g6b5";
>>>>>>>>     };
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> This moves the definition of the boot framebuffer setup into the area
>>>>>>>> where bootloader info is suppose to go. Then you can use the phandle
>>>>>>>> to follow the actual device chains and protect the clocks and
>>>>>>>> regulators. To make that work you are required to provide an accurate
>>>>>>>> description of the real video hardware so that this chain can be
>>>>>>>> followed.
>>>>>>>
>>>>>>> This will not work, first of all multiple blocks may be involved, so
>>>>>>> the device = in the boot-framebuffer would need to be a list. That in
>>>>>>> itself is not a problem, the problem is that the blocks used may have
>>>>>>> multiple clocks, of which the setup mode likely uses only a few.
>>>>>>>
>>>>>>> So if we do things this way, we end up keeping way to many clocks
>>>>>>> enabled.
>>>>>>
>>>>>> That doesn't hurt anything.
>>>>>
>>>>> <snip lots of stuff based on the above>
>>>>>
>>>>> Wrong, that does hurt things. As already discussed simply stopping the
>>>>> clocks from being disabled by the unused_clks mechanism is not enough,
>>>>> since clocks may be shared, and we must stop another device driver
>>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling
>>>>> the shared clk, which means calling clk_enable on it to mark it as
>>>>> being in use. So this will hurt cause it will cause us to enable a bunch
>>>>> of clks which should not be enabled.
>>>>
>>>> I said earlier that you would need to add a protected mechanism to
>>>> clocks to handle this phase. When a clock/regulator is protected you
>>>> can turn it on but you can't turn it off. When simplefb exits it will
>>>> clear this protected status. When the protected status gets cleared
>>>> treat it as a ref count decrement and turn off the clock/regulator if
>>>> indicated to do so. If a clock is protected, all of it parents get the
>>>> protected bit set too.
>>>>
>>>> Protected mode
>>>>    you can turn clocks on, but not off
>>>>    it is ref counted
>>>>   when it hits zero, look at the normal refcount and set that state
>>>>
>>>> Protected mode is not long lived. It only hangs around until the real
>>>> device driver loads.
>>>
>>> And that has already been nacked by the clk maintainer because it is
>>> too complicated, and I agree this is tons more complicated then simply
>>> adding the list of clocks to the simplefb node.
>>>
>>>>> I've been thinking more about this, and I understand that people don't
>>>>> want to put hardware description in the simplefb node, but this is
>>>>> not hardware description.
>>>>>
>>>>> u-boot sets up the display-pipeline to scan out of a certain part of
>>>>> memory, in order to do this it writes the memory address to some registers
>>>>> in the display pipeline, so what it is passing is not hardware description
>>>>> (it is not passing all possible memory addresses for the framebuffer), but
>>>>> it is passing information about the state in which it has left the display
>>>>> pipeline, iow it is passing state information.
>>>>
>>>> Giving the buffer to a piece of hardware is more than setting state.
>>>> The hardware now owns that buffer.  That ownership needs to be managed
>>>> so that if the hardware decides it doesn't want the buffer it can be
>>>> returned to the global pool.
>>>>
>>>> That's why the buffer has to go into that reserved memory section, not
>>>> the chosen section.
>>>
>>> But is not in the reserved memory section, it is in the simplefb
>>> section, and we're stuck with this because of backward compatibility.
>>>
>>>  An OS doesn't have to process chosen, it is just
>>>> there for informational purposes. To keep the OS from thinking it owns
>>>> the memory in that video buffer and can use it for OS purposes, it has
>>>> to go into that reserved memory section.
>>>>
>>>> The clocks are different. We know exactly who owns those clocks, the
>>>> graphics hardware.
>>>>
>>>> You want something like this where the state of the entire video path
>>>> is encoded into the chosen section. But that info is going to vary all
>>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb
>>>> isn't going to be simple any more. Plus the purposes of adding all of
>>>> this complexity is just to handle the half second transition from boot
>>>> loader control to real driver.
>>>>
>>>>  reserved-memory {
>>>>      #address-cells = <1>;
>>>>      #size-cells = <1>;
>>>>      ranges;
>>>>
>>>>      display_reserved: framebuffer@78000000 {
>>>>          reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>      };
>>>>  };
>>>>
>>>>  lcd0: lcd-controller@820000 {
>>>>      compatible = "marvell,dove-lcd";
>>>>      reg = <0x820000 0x1000>;
>>>>      interrupts = <47>;
>>>>      framebuffer = <&display_reserved>;
>>>>      clocks = <&si5351 0>;
>>>>      clock-names = "ext_ref_clk_1";
>>>>  };
>>>>
>>>>  chosen {
>>>>      boot-framebuffer {
>>>>         compatible = "simple-framebuffer";
>>>>         state {
>>>>             device = <&lcd0>;
>>>>             width = <1600>;
>>>>             height = <1200>;
>>>>             stride = <(1600 * 2)>;
>>>>             format = "r5g6b5";
>>>>             clocks = <&si5351 on 10mhz>;
>>>
>>> Right, so here we get a list of clocks which are actually in use
>>> by the simplefb, just as I've been advocating all the time already.
>>>
>>> Note that the clock speed is not necessary, all the kernel needs to
>>> know is that it must not change it.
>>>
>>> So as you seem to agree that we need to pass some sort of clock state
>>> info, then all we need to agree on now is where to put it, as said adding
>>> the reg property to a reserved-memory node is out of the question because
>>> of backward compat, likewise storing width, height and format in a state
>>> sub-node are out of the question for the same reason. But other then that
>>> I'm fine with putting the simplefb node under chosen and putting clocks
>>> in there (without the state subnode) as you suggest above.
>>>
>>> So we seem to be in agreement here :)
>>>
>>>>            output = "hdmi";
>>>>            state {
>>>>                  device = <&hdmi>
>>>>                  clocks = <&xyz on 12mhz>;
>>>>           }
>>>
>>> That level of detail won't be necessary, simplefb is supposed to be
>>> simple, the kernel does not need to know which outputs there are, there
>>> will always be only one for simplefb.
>>
>> I don't agree, but you are going to do it any way so I'll try and help
>> tkeep problem side effects I know of to a minimum. You are relying on
>> the BIOS to provide detailed, accurate information. Relying on BIOSes
>> to do that has historically been a very bad idea.
>>
>> If you go the way of putting this info into the chosen section you are
>> going to have to mark the clocks/regulators in use for all of the
>> outputs too -- hdmi, TV, LCD, backlights, etc, etc. Not going to be
>> useful if the backlight turns off because simplefb hasn't grabbed it.
>>
>> This is the only real difference between the proposals - you want
>> uboot to enumerate what needs to be protected. I don't trust the BIOS
>> to do that reliably so I'd preferred to just protect everything in the
>> device hardware chain until the device specific drivers load.
>>
>> -------------------------------------------------------
>>
>> I also still believe this is a problem up in Linux that we shouldn't
>> be using the device tree to fix.
>>
>> It seems to me that the need for something like a 'protected' mode is
>> a generic problem that could be extended to all hardware. In protected
>> mode things can be turned on but nothing can be turned off.  Only
>> after the kernel has all of the necessary drivers loaded would a small
>> app run that hits an IOCTL and says, ok protected mode is over now
>> clean up these things wasting power.
>
> What happens if some clock needs to be disabled?
> Like clocks that must be gated before setting a new clock rate
> or reparenting. The kernel supports these, and it wouldn't surprise me
> if some driver actually requires this. You might end up blocking that driver
> or breaking its probe function.

Arggh, using those phandles in the chosen section means uboot is going
to have to get recompiled every time the DTS changes. I think we need
to come up with a scheme that doesn't need for uboot to be aware of
phandles.

Something like this...
uboot adds the chosen section then Linux would error out if the
framebuffer in the chosen section doesn't match the reserved memory it
is expecting.  Or make uboot smart enough to hunt down the reserved
memory section and patch it like it does with dramsize.

 reserved-memory {
     #address-cells = <1>;
     #size-cells = <1>;
     ranges;

     display_reserved: framebuffer@78000000 {
         reg = <0x78000000  (1600 * 1200 * 2)>;
     };
 };

 lcd0: lcd-controller@820000 {
     compatible = "marvell,dove-lcd";
     reg = <0x820000 0x1000>;
     interrupts = <47>;
     framebuffer = <&display_reserved>;
     clocks = <&si5351 0>;
     clock-names = "ext_ref_clk_1";
 };

 chosen {
     boot-framebuffer {
        framebuffer = <0x78000000>;
        width = <1600>;
        height = <1200>;
        stride = <(1600 * 2)>;
        format = "r5g6b5";
     };
 }



>
> And what if reset controls are asserted then de-asserted in the probe function?
> IIRC there are drivers that do this to reset the underlying hardware.
>
>> Maybe it should be renamed 'boot' mode. To implement this the various
>> 'turn off' functions would get a 'boot' mode flag. In boot mode they
>> track the ref counts but don't turn things off when the ref count hits
>> zero.  Then that ioctl() that the user space app calls runs the chains
>> of all of the clocks/regulators/etc and if the ref count is zero turns
>> them off again and clears 'boot' mode. Doesn't matter if you turn off
>> something again that is already off.
>
> And what if something just happened to be left on that some driver
> wants to turn off? You are assuming everything not used is off,
> which is not always the case.
>
>
> ChenYu



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Chen-Yu Tsai]

On Sun, Oct 5, 2014 at 11:29 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
> On Sun, Oct 5, 2014 at 11:17 AM, Chen-Yu Tsai <wens@csie.org> wrote:
>> On Sun, Oct 5, 2014 at 11:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>>> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote:
>>>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
>>>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>>>>>>
>>>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>>>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>>>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>>>>>>>> inaccurate.
>>>>>>>>>>>
>>>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>>>>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>>>>>>>
>>>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>>>>>>>> fix in DT.
>>>>>>>>>>>
>>>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>>>>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>>>>>>>
>>>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>>>>>>>> block. Their use is implied by the connection.
>>>>>>>>>>
>>>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>>>>>>>> information. However, what you are adding IS hardware information and
>>>>>>>>>> clearly has a place somewhere else. And adding anything which is not
>>>>>>>>>> hardware description gets much more scrutiny.
>>>>>>>>>>
>>>>>>>>>>> More over it is a bit late to start making objections now. This has already been
>>>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>>>>>>>> read the entire thread in the archives under the subject:
>>>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>>>>>>>
>>>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>>>>>>>> have made suggestions which I would agree with and you've basically
>>>>>>>>>> ignored them.
>>>>>>>>>>
>>>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>>>>>>>> we can all move on and spend out time in a more productive manner.
>>>>>>>>>>
>>>>>>>>>> Not an effective argument to get things merged.
>>>>>>>>>
>>>>>>>>> If there is not good solution to deferring clock clean up in the
>>>>>>>>> kernel, another approach is to change how simple-framebuffer is
>>>>>>>>> described in the device tree....
>>>>>>>>>
>>>>>>>>> Right now it is a stand-alone item that looks like a device node, but
>>>>>>>>> it isn't a device.
>>>>>>>>>
>>>>>>>>> framebuffer {
>>>>>>>>>     compatible = "simple-framebuffer";
>>>>>>>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>>>>>>>     width = <1600>;
>>>>>>>>>     height = <1200>;
>>>>>>>>>     stride = <(1600 * 2)>;
>>>>>>>>>     format = "r5g6b5";
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> How about something like this?
>>>>>>>>>
>>>>>>>>> reserved-memory {
>>>>>>>>>     #address-cells = <1>;
>>>>>>>>>     #size-cells = <1>;
>>>>>>>>>     ranges;
>>>>>>>>>
>>>>>>>>>     display_reserved: framebuffer@78000000 {
>>>>>>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>>>>>     };
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> lcd0: lcd-controller@820000 {
>>>>>>>>>     compatible = "marvell,dove-lcd";
>>>>>>>>>     reg = <0x820000 0x1000>;
>>>>>>>>>     interrupts = <47>;
>>>>>>>>>     clocks = <&si5351 0>;
>>>>>>>>>     clock-names = "ext_ref_clk_1";
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> chosen {
>>>>>>>>>     boot-framebuffer {
>>>>>>>>>        compatible = "simple-framebuffer";
>>>>>>>>>        device = <&lcd0>;
>>>>>>>>>        framebuffer = <&display_reserved>;
>>>>>>>>>        width = <1600>;
>>>>>>>>>        height = <1200>;
>>>>>>>>>        stride = <(1600 * 2)>;
>>>>>>>>>        format = "r5g6b5";
>>>>>>>>>     };
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This moves the definition of the boot framebuffer setup into the area
>>>>>>>>> where bootloader info is suppose to go. Then you can use the phandle
>>>>>>>>> to follow the actual device chains and protect the clocks and
>>>>>>>>> regulators. To make that work you are required to provide an accurate
>>>>>>>>> description of the real video hardware so that this chain can be
>>>>>>>>> followed.
>>>>>>>>
>>>>>>>> This will not work, first of all multiple blocks may be involved, so
>>>>>>>> the device = in the boot-framebuffer would need to be a list. That in
>>>>>>>> itself is not a problem, the problem is that the blocks used may have
>>>>>>>> multiple clocks, of which the setup mode likely uses only a few.
>>>>>>>>
>>>>>>>> So if we do things this way, we end up keeping way to many clocks
>>>>>>>> enabled.
>>>>>>>
>>>>>>> That doesn't hurt anything.
>>>>>>
>>>>>> <snip lots of stuff based on the above>
>>>>>>
>>>>>> Wrong, that does hurt things. As already discussed simply stopping the
>>>>>> clocks from being disabled by the unused_clks mechanism is not enough,
>>>>>> since clocks may be shared, and we must stop another device driver
>>>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling
>>>>>> the shared clk, which means calling clk_enable on it to mark it as
>>>>>> being in use. So this will hurt cause it will cause us to enable a bunch
>>>>>> of clks which should not be enabled.
>>>>>
>>>>> I said earlier that you would need to add a protected mechanism to
>>>>> clocks to handle this phase. When a clock/regulator is protected you
>>>>> can turn it on but you can't turn it off. When simplefb exits it will
>>>>> clear this protected status. When the protected status gets cleared
>>>>> treat it as a ref count decrement and turn off the clock/regulator if
>>>>> indicated to do so. If a clock is protected, all of it parents get the
>>>>> protected bit set too.
>>>>>
>>>>> Protected mode
>>>>>    you can turn clocks on, but not off
>>>>>    it is ref counted
>>>>>   when it hits zero, look at the normal refcount and set that state
>>>>>
>>>>> Protected mode is not long lived. It only hangs around until the real
>>>>> device driver loads.
>>>>
>>>> And that has already been nacked by the clk maintainer because it is
>>>> too complicated, and I agree this is tons more complicated then simply
>>>> adding the list of clocks to the simplefb node.
>>>>
>>>>>> I've been thinking more about this, and I understand that people don't
>>>>>> want to put hardware description in the simplefb node, but this is
>>>>>> not hardware description.
>>>>>>
>>>>>> u-boot sets up the display-pipeline to scan out of a certain part of
>>>>>> memory, in order to do this it writes the memory address to some registers
>>>>>> in the display pipeline, so what it is passing is not hardware description
>>>>>> (it is not passing all possible memory addresses for the framebuffer), but
>>>>>> it is passing information about the state in which it has left the display
>>>>>> pipeline, iow it is passing state information.
>>>>>
>>>>> Giving the buffer to a piece of hardware is more than setting state.
>>>>> The hardware now owns that buffer.  That ownership needs to be managed
>>>>> so that if the hardware decides it doesn't want the buffer it can be
>>>>> returned to the global pool.
>>>>>
>>>>> That's why the buffer has to go into that reserved memory section, not
>>>>> the chosen section.
>>>>
>>>> But is not in the reserved memory section, it is in the simplefb
>>>> section, and we're stuck with this because of backward compatibility.
>>>>
>>>>  An OS doesn't have to process chosen, it is just
>>>>> there for informational purposes. To keep the OS from thinking it owns
>>>>> the memory in that video buffer and can use it for OS purposes, it has
>>>>> to go into that reserved memory section.
>>>>>
>>>>> The clocks are different. We know exactly who owns those clocks, the
>>>>> graphics hardware.
>>>>>
>>>>> You want something like this where the state of the entire video path
>>>>> is encoded into the chosen section. But that info is going to vary all
>>>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb
>>>>> isn't going to be simple any more. Plus the purposes of adding all of
>>>>> this complexity is just to handle the half second transition from boot
>>>>> loader control to real driver.
>>>>>
>>>>>  reserved-memory {
>>>>>      #address-cells = <1>;
>>>>>      #size-cells = <1>;
>>>>>      ranges;
>>>>>
>>>>>      display_reserved: framebuffer@78000000 {
>>>>>          reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>      };
>>>>>  };
>>>>>
>>>>>  lcd0: lcd-controller@820000 {
>>>>>      compatible = "marvell,dove-lcd";
>>>>>      reg = <0x820000 0x1000>;
>>>>>      interrupts = <47>;
>>>>>      framebuffer = <&display_reserved>;
>>>>>      clocks = <&si5351 0>;
>>>>>      clock-names = "ext_ref_clk_1";
>>>>>  };
>>>>>
>>>>>  chosen {
>>>>>      boot-framebuffer {
>>>>>         compatible = "simple-framebuffer";
>>>>>         state {
>>>>>             device = <&lcd0>;
>>>>>             width = <1600>;
>>>>>             height = <1200>;
>>>>>             stride = <(1600 * 2)>;
>>>>>             format = "r5g6b5";
>>>>>             clocks = <&si5351 on 10mhz>;
>>>>
>>>> Right, so here we get a list of clocks which are actually in use
>>>> by the simplefb, just as I've been advocating all the time already.
>>>>
>>>> Note that the clock speed is not necessary, all the kernel needs to
>>>> know is that it must not change it.
>>>>
>>>> So as you seem to agree that we need to pass some sort of clock state
>>>> info, then all we need to agree on now is where to put it, as said adding
>>>> the reg property to a reserved-memory node is out of the question because
>>>> of backward compat, likewise storing width, height and format in a state
>>>> sub-node are out of the question for the same reason. But other then that
>>>> I'm fine with putting the simplefb node under chosen and putting clocks
>>>> in there (without the state subnode) as you suggest above.
>>>>
>>>> So we seem to be in agreement here :)
>>>>
>>>>>            output = "hdmi";
>>>>>            state {
>>>>>                  device = <&hdmi>
>>>>>                  clocks = <&xyz on 12mhz>;
>>>>>           }
>>>>
>>>> That level of detail won't be necessary, simplefb is supposed to be
>>>> simple, the kernel does not need to know which outputs there are, there
>>>> will always be only one for simplefb.
>>>
>>> I don't agree, but you are going to do it any way so I'll try and help
>>> tkeep problem side effects I know of to a minimum. You are relying on
>>> the BIOS to provide detailed, accurate information. Relying on BIOSes
>>> to do that has historically been a very bad idea.
>>>
>>> If you go the way of putting this info into the chosen section you are
>>> going to have to mark the clocks/regulators in use for all of the
>>> outputs too -- hdmi, TV, LCD, backlights, etc, etc. Not going to be
>>> useful if the backlight turns off because simplefb hasn't grabbed it.
>>>
>>> This is the only real difference between the proposals - you want
>>> uboot to enumerate what needs to be protected. I don't trust the BIOS
>>> to do that reliably so I'd preferred to just protect everything in the
>>> device hardware chain until the device specific drivers load.
>>>
>>> -------------------------------------------------------
>>>
>>> I also still believe this is a problem up in Linux that we shouldn't
>>> be using the device tree to fix.
>>>
>>> It seems to me that the need for something like a 'protected' mode is
>>> a generic problem that could be extended to all hardware. In protected
>>> mode things can be turned on but nothing can be turned off.  Only
>>> after the kernel has all of the necessary drivers loaded would a small
>>> app run that hits an IOCTL and says, ok protected mode is over now
>>> clean up these things wasting power.
>>
>> What happens if some clock needs to be disabled?
>> Like clocks that must be gated before setting a new clock rate
>> or reparenting. The kernel supports these, and it wouldn't surprise me
>> if some driver actually requires this. You might end up blocking that driver
>> or breaking its probe function.
>
> Arggh, using those phandles in the chosen section means uboot is going
> to have to get recompiled every time the DTS changes. I think we need
> to come up with a scheme that doesn't need for uboot to be aware of
> phandles.

Why is that? U-boot is perfectly capable of patching device tree blobs.

Mainline u-boot already updates the memory node, and if needed,
the reserved-memory node as well.

Someone just has to write the (ugly) code to do it, which Luc
has already done for clock phandles for sunxi.

U-boot itself does not need to use the dtb, though that seems
like the direction it's headed.

> Something like this...
> uboot adds the chosen section then Linux would error out if the
> framebuffer in the chosen section doesn't match the reserved memory it
> is expecting.  Or make uboot smart enough to hunt down the reserved
> memory section and patch it like it does with dramsize.

And someone probably will. Why is that a problem?


ChenYu


>
>  reserved-memory {
>      #address-cells = <1>;
>      #size-cells = <1>;
>      ranges;
>
>      display_reserved: framebuffer@78000000 {
>          reg = <0x78000000  (1600 * 1200 * 2)>;
>      };
>  };
>
>  lcd0: lcd-controller@820000 {
>      compatible = "marvell,dove-lcd";
>      reg = <0x820000 0x1000>;
>      interrupts = <47>;
>      framebuffer = <&display_reserved>;
>      clocks = <&si5351 0>;
>      clock-names = "ext_ref_clk_1";
>  };
>
>  chosen {
>      boot-framebuffer {
>         framebuffer = <0x78000000>;
>         width = <1600>;
>         height = <1200>;
>         stride = <(1600 * 2)>;
>         format = "r5g6b5";
>      };
>  }
>
>
>
>>
>> And what if reset controls are asserted then de-asserted in the probe function?
>> IIRC there are drivers that do this to reset the underlying hardware.
>>
>>> Maybe it should be renamed 'boot' mode. To implement this the various
>>> 'turn off' functions would get a 'boot' mode flag. In boot mode they
>>> track the ref counts but don't turn things off when the ref count hits
>>> zero.  Then that ioctl() that the user space app calls runs the chains
>>> of all of the clocks/regulators/etc and if the ref count is zero turns
>>> them off again and clears 'boot' mode. Doesn't matter if you turn off
>>> something again that is already off.
>>
>> And what if something just happened to be left on that some driver
>> wants to turn off? You are assuming everything not used is off,
>> which is not always the case.

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[jonsmirl]

On Sun, Oct 5, 2014 at 11:36 AM, Chen-Yu Tsai <wens@csie.org> wrote:
> On Sun, Oct 5, 2014 at 11:29 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>> On Sun, Oct 5, 2014 at 11:17 AM, Chen-Yu Tsai <wens@csie.org> wrote:
>>> On Sun, Oct 5, 2014 at 11:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>>>> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote:
>>>>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote:
>>>>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote:
>>>>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>>>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>>>>>>>
>>>>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address
>>>>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is
>>>>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not
>>>>>>>>>>>>> has no side effects on the hardware setup, but you are changing that
>>>>>>>>>>>>> aspect. You are mixing in a hardware description that is simply
>>>>>>>>>>>>> inaccurate.
>>>>>>>>>>>>
>>>>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>>>>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory,
>>>>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise
>>>>>>>>>>>> on some systems the virtual device needs clocks to keep running.
>>>>>>>>>>>>
>>>>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its
>>>>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to
>>>>>>>>>>>>> fix in DT.
>>>>>>>>>>>>
>>>>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>>>>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb
>>>>>>>>>>>> for some reason is o so very very special, because it is not special, it needs
>>>>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen.
>>>>>>>>>>>
>>>>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w
>>>>>>>>>>> block. Their use is implied by the connection.
>>>>>>>>>>>
>>>>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware
>>>>>>>>>>> information. However, what you are adding IS hardware information and
>>>>>>>>>>> clearly has a place somewhere else. And adding anything which is not
>>>>>>>>>>> hardware description gets much more scrutiny.
>>>>>>>>>>>
>>>>>>>>>>>> More over it is a bit late to start making objections now. This has already been
>>>>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already
>>>>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to
>>>>>>>>>>>> read the entire thread in the archives under the subject:
>>>>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code"
>>>>>>>>>>>
>>>>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others
>>>>>>>>>>> have made suggestions which I would agree with and you've basically
>>>>>>>>>>> ignored them.
>>>>>>>>>>>
>>>>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has
>>>>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that
>>>>>>>>>>>> we can all move on and spend out time in a more productive manner.
>>>>>>>>>>>
>>>>>>>>>>> Not an effective argument to get things merged.
>>>>>>>>>>
>>>>>>>>>> If there is not good solution to deferring clock clean up in the
>>>>>>>>>> kernel, another approach is to change how simple-framebuffer is
>>>>>>>>>> described in the device tree....
>>>>>>>>>>
>>>>>>>>>> Right now it is a stand-alone item that looks like a device node, but
>>>>>>>>>> it isn't a device.
>>>>>>>>>>
>>>>>>>>>> framebuffer {
>>>>>>>>>>     compatible = "simple-framebuffer";
>>>>>>>>>>     reg = <0x1d385000 (1600 * 1200 * 2)>;
>>>>>>>>>>     width = <1600>;
>>>>>>>>>>     height = <1200>;
>>>>>>>>>>     stride = <(1600 * 2)>;
>>>>>>>>>>     format = "r5g6b5";
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> How about something like this?
>>>>>>>>>>
>>>>>>>>>> reserved-memory {
>>>>>>>>>>     #address-cells = <1>;
>>>>>>>>>>     #size-cells = <1>;
>>>>>>>>>>     ranges;
>>>>>>>>>>
>>>>>>>>>>     display_reserved: framebuffer@78000000 {
>>>>>>>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>>>>>>     };
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> lcd0: lcd-controller@820000 {
>>>>>>>>>>     compatible = "marvell,dove-lcd";
>>>>>>>>>>     reg = <0x820000 0x1000>;
>>>>>>>>>>     interrupts = <47>;
>>>>>>>>>>     clocks = <&si5351 0>;
>>>>>>>>>>     clock-names = "ext_ref_clk_1";
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> chosen {
>>>>>>>>>>     boot-framebuffer {
>>>>>>>>>>        compatible = "simple-framebuffer";
>>>>>>>>>>        device = <&lcd0>;
>>>>>>>>>>        framebuffer = <&display_reserved>;
>>>>>>>>>>        width = <1600>;
>>>>>>>>>>        height = <1200>;
>>>>>>>>>>        stride = <(1600 * 2)>;
>>>>>>>>>>        format = "r5g6b5";
>>>>>>>>>>     };
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This moves the definition of the boot framebuffer setup into the area
>>>>>>>>>> where bootloader info is suppose to go. Then you can use the phandle
>>>>>>>>>> to follow the actual device chains and protect the clocks and
>>>>>>>>>> regulators. To make that work you are required to provide an accurate
>>>>>>>>>> description of the real video hardware so that this chain can be
>>>>>>>>>> followed.
>>>>>>>>>
>>>>>>>>> This will not work, first of all multiple blocks may be involved, so
>>>>>>>>> the device = in the boot-framebuffer would need to be a list. That in
>>>>>>>>> itself is not a problem, the problem is that the blocks used may have
>>>>>>>>> multiple clocks, of which the setup mode likely uses only a few.
>>>>>>>>>
>>>>>>>>> So if we do things this way, we end up keeping way to many clocks
>>>>>>>>> enabled.
>>>>>>>>
>>>>>>>> That doesn't hurt anything.
>>>>>>>
>>>>>>> <snip lots of stuff based on the above>
>>>>>>>
>>>>>>> Wrong, that does hurt things. As already discussed simply stopping the
>>>>>>> clocks from being disabled by the unused_clks mechanism is not enough,
>>>>>>> since clocks may be shared, and we must stop another device driver
>>>>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling
>>>>>>> the shared clk, which means calling clk_enable on it to mark it as
>>>>>>> being in use. So this will hurt cause it will cause us to enable a bunch
>>>>>>> of clks which should not be enabled.
>>>>>>
>>>>>> I said earlier that you would need to add a protected mechanism to
>>>>>> clocks to handle this phase. When a clock/regulator is protected you
>>>>>> can turn it on but you can't turn it off. When simplefb exits it will
>>>>>> clear this protected status. When the protected status gets cleared
>>>>>> treat it as a ref count decrement and turn off the clock/regulator if
>>>>>> indicated to do so. If a clock is protected, all of it parents get the
>>>>>> protected bit set too.
>>>>>>
>>>>>> Protected mode
>>>>>>    you can turn clocks on, but not off
>>>>>>    it is ref counted
>>>>>>   when it hits zero, look at the normal refcount and set that state
>>>>>>
>>>>>> Protected mode is not long lived. It only hangs around until the real
>>>>>> device driver loads.
>>>>>
>>>>> And that has already been nacked by the clk maintainer because it is
>>>>> too complicated, and I agree this is tons more complicated then simply
>>>>> adding the list of clocks to the simplefb node.
>>>>>
>>>>>>> I've been thinking more about this, and I understand that people don't
>>>>>>> want to put hardware description in the simplefb node, but this is
>>>>>>> not hardware description.
>>>>>>>
>>>>>>> u-boot sets up the display-pipeline to scan out of a certain part of
>>>>>>> memory, in order to do this it writes the memory address to some registers
>>>>>>> in the display pipeline, so what it is passing is not hardware description
>>>>>>> (it is not passing all possible memory addresses for the framebuffer), but
>>>>>>> it is passing information about the state in which it has left the display
>>>>>>> pipeline, iow it is passing state information.
>>>>>>
>>>>>> Giving the buffer to a piece of hardware is more than setting state.
>>>>>> The hardware now owns that buffer.  That ownership needs to be managed
>>>>>> so that if the hardware decides it doesn't want the buffer it can be
>>>>>> returned to the global pool.
>>>>>>
>>>>>> That's why the buffer has to go into that reserved memory section, not
>>>>>> the chosen section.
>>>>>
>>>>> But is not in the reserved memory section, it is in the simplefb
>>>>> section, and we're stuck with this because of backward compatibility.
>>>>>
>>>>>  An OS doesn't have to process chosen, it is just
>>>>>> there for informational purposes. To keep the OS from thinking it owns
>>>>>> the memory in that video buffer and can use it for OS purposes, it has
>>>>>> to go into that reserved memory section.
>>>>>>
>>>>>> The clocks are different. We know exactly who owns those clocks, the
>>>>>> graphics hardware.
>>>>>>
>>>>>> You want something like this where the state of the entire video path
>>>>>> is encoded into the chosen section. But that info is going to vary all
>>>>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb
>>>>>> isn't going to be simple any more. Plus the purposes of adding all of
>>>>>> this complexity is just to handle the half second transition from boot
>>>>>> loader control to real driver.
>>>>>>
>>>>>>  reserved-memory {
>>>>>>      #address-cells = <1>;
>>>>>>      #size-cells = <1>;
>>>>>>      ranges;
>>>>>>
>>>>>>      display_reserved: framebuffer@78000000 {
>>>>>>          reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>>      };
>>>>>>  };
>>>>>>
>>>>>>  lcd0: lcd-controller@820000 {
>>>>>>      compatible = "marvell,dove-lcd";
>>>>>>      reg = <0x820000 0x1000>;
>>>>>>      interrupts = <47>;
>>>>>>      framebuffer = <&display_reserved>;
>>>>>>      clocks = <&si5351 0>;
>>>>>>      clock-names = "ext_ref_clk_1";
>>>>>>  };
>>>>>>
>>>>>>  chosen {
>>>>>>      boot-framebuffer {
>>>>>>         compatible = "simple-framebuffer";
>>>>>>         state {
>>>>>>             device = <&lcd0>;
>>>>>>             width = <1600>;
>>>>>>             height = <1200>;
>>>>>>             stride = <(1600 * 2)>;
>>>>>>             format = "r5g6b5";
>>>>>>             clocks = <&si5351 on 10mhz>;
>>>>>
>>>>> Right, so here we get a list of clocks which are actually in use
>>>>> by the simplefb, just as I've been advocating all the time already.
>>>>>
>>>>> Note that the clock speed is not necessary, all the kernel needs to
>>>>> know is that it must not change it.
>>>>>
>>>>> So as you seem to agree that we need to pass some sort of clock state
>>>>> info, then all we need to agree on now is where to put it, as said adding
>>>>> the reg property to a reserved-memory node is out of the question because
>>>>> of backward compat, likewise storing width, height and format in a state
>>>>> sub-node are out of the question for the same reason. But other then that
>>>>> I'm fine with putting the simplefb node under chosen and putting clocks
>>>>> in there (without the state subnode) as you suggest above.
>>>>>
>>>>> So we seem to be in agreement here :)
>>>>>
>>>>>>            output = "hdmi";
>>>>>>            state {
>>>>>>                  device = <&hdmi>
>>>>>>                  clocks = <&xyz on 12mhz>;
>>>>>>           }
>>>>>
>>>>> That level of detail won't be necessary, simplefb is supposed to be
>>>>> simple, the kernel does not need to know which outputs there are, there
>>>>> will always be only one for simplefb.
>>>>
>>>> I don't agree, but you are going to do it any way so I'll try and help
>>>> tkeep problem side effects I know of to a minimum. You are relying on
>>>> the BIOS to provide detailed, accurate information. Relying on BIOSes
>>>> to do that has historically been a very bad idea.
>>>>
>>>> If you go the way of putting this info into the chosen section you are
>>>> going to have to mark the clocks/regulators in use for all of the
>>>> outputs too -- hdmi, TV, LCD, backlights, etc, etc. Not going to be
>>>> useful if the backlight turns off because simplefb hasn't grabbed it.
>>>>
>>>> This is the only real difference between the proposals - you want
>>>> uboot to enumerate what needs to be protected. I don't trust the BIOS
>>>> to do that reliably so I'd preferred to just protect everything in the
>>>> device hardware chain until the device specific drivers load.
>>>>
>>>> -------------------------------------------------------
>>>>
>>>> I also still believe this is a problem up in Linux that we shouldn't
>>>> be using the device tree to fix.
>>>>
>>>> It seems to me that the need for something like a 'protected' mode is
>>>> a generic problem that could be extended to all hardware. In protected
>>>> mode things can be turned on but nothing can be turned off.  Only
>>>> after the kernel has all of the necessary drivers loaded would a small
>>>> app run that hits an IOCTL and says, ok protected mode is over now
>>>> clean up these things wasting power.
>>>
>>> What happens if some clock needs to be disabled?
>>> Like clocks that must be gated before setting a new clock rate
>>> or reparenting. The kernel supports these, and it wouldn't surprise me
>>> if some driver actually requires this. You might end up blocking that driver
>>> or breaking its probe function.
>>
>> Arggh, using those phandles in the chosen section means uboot is going
>> to have to get recompiled every time the DTS changes. I think we need
>> to come up with a scheme that doesn't need for uboot to be aware of
>> phandles.
>
> Why is that? U-boot is perfectly capable of patching device tree blobs.
>
> Mainline u-boot already updates the memory node, and if needed,
> the reserved-memory node as well.
>
> Someone just has to write the (ugly) code to do it, which Luc
> has already done for clock phandles for sunxi.

So uboot is going to contain code to hunt through the Linux provided
DTB to find the correct phandles for the clocks/regulators and then
patch those phandles into the chosen section? How is uboot going to
reconcile it's concept of what those clock/regulators are with a Linux
provided DTB that is constant state of flux?

I think trying to get uboot to manipulate phandles in a Linux provided
DTB is an incredibly fragile mechanism that will be prone to breakage.

Much better to come with a scheme where uboot just inserts fixed
strings into the DTB. That last example device tree I posted removed
all of the phandles from the chosen section, but it relies on the
kernel gaining 'boot' mode.


>
> U-boot itself does not need to use the dtb, though that seems
> like the direction it's headed.
>
>> Something like this...
>> uboot adds the chosen section then Linux would error out if the
>> framebuffer in the chosen section doesn't match the reserved memory it
>> is expecting.  Or make uboot smart enough to hunt down the reserved
>> memory section and patch it like it does with dramsize.
>
> And someone probably will. Why is that a problem?
>
>
> ChenYu
>
>
>>
>>  reserved-memory {
>>      #address-cells = <1>;
>>      #size-cells = <1>;
>>      ranges;
>>
>>      display_reserved: framebuffer@78000000 {
>>          reg = <0x78000000  (1600 * 1200 * 2)>;
>>      };
>>  };
>>
>>  lcd0: lcd-controller@820000 {
>>      compatible = "marvell,dove-lcd";
>>      reg = <0x820000 0x1000>;
>>      interrupts = <47>;
>>      framebuffer = <&display_reserved>;
>>      clocks = <&si5351 0>;
>>      clock-names = "ext_ref_clk_1";
>>  };
>>
>>  chosen {
>>      boot-framebuffer {
>>         framebuffer = <0x78000000>;
>>         width = <1600>;
>>         height = <1200>;
>>         stride = <(1600 * 2)>;
>>         format = "r5g6b5";
>>      };
>>  }
>>
>>
>>
>>>
>>> And what if reset controls are asserted then de-asserted in the probe function?
>>> IIRC there are drivers that do this to reset the underlying hardware.
>>>
>>>> Maybe it should be renamed 'boot' mode. To implement this the various
>>>> 'turn off' functions would get a 'boot' mode flag. In boot mode they
>>>> track the ref counts but don't turn things off when the ref count hits
>>>> zero.  Then that ioctl() that the user space app calls runs the chains
>>>> of all of the clocks/regulators/etc and if the ref count is zero turns
>>>> them off again and clears 'boot' mode. Doesn't matter if you turn off
>>>> something again that is already off.
>>>
>>> And what if something just happened to be left on that some driver
>>> wants to turn off? You are assuming everything not used is off,
>>> which is not always the case.



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [linux-sunxi] Re: [PATCH v3] dt-bindings: Add a clocks property to the simple-framebuffer bindin

[Hans de Goede]

Hi,

On 10/03/2014 10:08 PM, Rob Herring wrote:
> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 10/03/2014 05:57 PM, Rob Herring wrote:
>>> On Fri, Oct 3, 2014 at 9:05 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>
>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>>
>>>> --
>>>> Changes in v2:
>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org>
>>>> Changes in v3:
>>>> -Updated description to make clear simplefb deals with more then just memory
>>>
>>> NAK. "Fixing" the description is not what I meant and does not address
>>> my concerns. Currently, simplefb is configuration data. It is
>>> auxiliary data about how a chunk of memory is used. Using it or not
>>> has no side effects on the hardware setup, but you are changing that
>>> aspect. You are mixing in a hardware description that is simply
>>> inaccurate.
>>
>> Memory is hardware too, what simplefb is is best seen as a virtual device, and
>> even virtual devices have hardware resources they need, such as a chunk of memory,
>> which the kernel should not touch other then use it as a framebuffer, likewise
>> on some systems the virtual device needs clocks to keep running.
>>
>>> The kernel has made the decision to turn off "unused" clocks. If its
>>> determination of what is unused is wrong, then it is not a problem to
>>> fix in DT.
>>
>> No, it is up to DT to tell the kernel what clocks are used, that is how it works
>> for any other device. I don't understand why some people keep insisting simplefb
>> for some reason is o so very very special, because it is not special, it needs
>> resources, and it needs to tell the kernel about this or bad things happen.
> 
> No, the DT describes the connections of clocks from h/w block to h/w
> block. Their use is implied by the connection.

So normally DT describes HW aka clock connections, and your objection is
that I cannot add HW description AKA clock connections to the simplefb node.

> And yes, as the other thread mentioned DT is more than just hardware
> information. However, what you are adding IS hardware information and
> clearly has a place somewhere else. And adding anything which is not
> hardware description gets much more scrutiny.

But what I'm adding is not actually clock connections (aka HW description),
the display engine has many clocks, and what is being added to the simplefb
node is which clocks are *actually* used by the video mode setup for the simplefb,
not the clock connections, not which clocks can be used by the hw-block_s_ involved
in the display engine, but which ones are actually used for the specific mode
(and output connector) setup for the simplefb.

So according to your definition this is not HW description. This is usage description,
just like the simplefb node contains which memory is used.

>> More over it is a bit late to start making objections now. This has already been
>> discussed to death for weeks now, and every argument against this patch has already
>> been countered multiple times (including the one you are making now). Feel free to
>> read the entire thread in the archives under the subject:
>> "[PATCH 4/4] simplefb: add clock handling code"
> 
> You are on v2 and I hardly see any consensus on the v1 thread. Others
> have made suggestions which I would agree with and you've basically
> ignored them.

I have NOT ignored those, that is simply not true. I've even offered an alternative
approach myself, which was quickly shot down, see:

http://www.spinics.net/lists/arm-kernel/msg367559.html

And being quickly shotdown is exactly what my alternative solution has in common
with all the other alternatives. Non of them provide the simplicity nor robustness
of simply adding clocks to the dt-node. And this is not surprising since adding
clocks to the dt-node is exactly the right mechanism to tell the kernel that certain
clocks are used for something.

Mike Turquette, the clk maintainer, agrees that this is the right thing to do.

Yet people keep arguing that simplefb is magically special and thus cannot have
clocks. Arguments which to me after discussing this for weeks in a row are starting
to sound like "you cannot do this because I say so" rather then proper technical
arguments.

It seems to me that you're arguing purely at semantics without looking at the use case
in question at all. In the end creating a workable and working solution should always
trump semantics.

Note that I'm willing to look at alternatives, as long as those are not tons
more complicated then the current proposal, and they don't introduce a number of
trouble some unhandled corner cases which the current proposal does not introduce.

Unfortunately all alternatives proposed sofar are both more complicated and introduce
unhandled corner cases. Adding the clocks to the simplefb node is very KISS and
for those who actually have looked at the requirements for the use-cases we're
discussing here also feels like a natural fit, which usually is an indication that
it is the right solution.

So since adding clocks to simplefb seems problematic for some people, how about the
following, we add a new firmwarefb binding, which is basically simplefb + clocks,
then people who want simplefb to stay clean and elegant can use the clean and
elegant simplefb bindings, and people who have a need to express clocks usage
can do so using the firmwarefb binding. Would that be acceptable to you ?

Regards,

Hans