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

Chen-Yu Tsai wens at csie.org
Sun Oct 5 08:36:11 PDT 2014

On Sun, Oct 5, 2014 at 11:29 PM, jonsmirl at gmail.com <jonsmirl at gmail.com> wrote:
> On Sun, Oct 5, 2014 at 11:17 AM, Chen-Yu Tsai <wens at csie.org> wrote:
>> On Sun, Oct 5, 2014 at 11:07 PM, jonsmirl at gmail.com <jonsmirl at gmail.com> wrote:
>>> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede at redhat.com> wrote:
>>>> Hi,
>>>> On 10/05/2014 02:52 PM, jonsmirl at gmail.com wrote:
>>>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede at redhat.com> wrote:
>>>>>> Hi,
>>>>>> On 10/04/2014 02:38 PM, jonsmirl at gmail.com wrote:
>>>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede at redhat.com> wrote:
>>>>>>>> Hi,
>>>>>>>> On 10/04/2014 12:56 AM, jonsmirl at gmail.com wrote:
>>>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2 at gmail.com> wrote:
>>>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede at 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 at 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 at redhat.com>
>>>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette at linaro.org>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette at 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 at 78000000 {
>>>>>>>>>         reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>>>>>     };
>>>>>>>>> };
>>>>>>>>> lcd0: lcd-controller at 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 at 78000000 {
>>>>>          reg = <0x78000000  (1600 * 1200 * 2)>;
>>>>>      };
>>>>>  };
>>>>>  lcd0: lcd-controller at 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?


>  reserved-memory {
>      #address-cells = <1>;
>      #size-cells = <1>;
>      ranges;
>      display_reserved: framebuffer at 78000000 {
>          reg = <0x78000000  (1600 * 1200 * 2)>;
>      };
>  };
>  lcd0: lcd-controller at 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.

More information about the linux-arm-kernel mailing list