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

[Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Hi,

On 10/05/2014 05:17 PM, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> On Sun, Oct 5, 2014 at 11:16 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi,
>>
>> On 10/05/2014 05:07 PM, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>>> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>> Hi,
>>>>
>>>> On 10/05/2014 02:52 PM, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>>>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10/04/2014 02:38 PM, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>>>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 10/04/2014 12:56 AM, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>>>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>>>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware /
>>>>>>>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a
>>>>>>>>>>>>> property to communicate this to the OS.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory
>>>>>>>>>>>>
>>>>>>>>>>>> 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