Re: [PATCH 2/2] dt-bindings: simplefb-sunxi: Add sunxi simplefb extensions

[Maxime Ripard <maxime.ripard@free-electrons.com>]

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

On Wed, Nov 19, 2014 at 02:05:48PM +0000, Grant Likely wrote:
> On Mon, Nov 17, 2014 at 3:55 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Mon, Nov 17, 2014 at 01:39:18PM +0000, Grant Likely wrote:
> >> On Mon, Nov 17, 2014 at 12:47 PM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > On Mon, Nov 17, 2014 at 12:34:46PM +0100, Hans de Goede wrote:
> >> >> If pre-filled framebuffer nodes are used, the firmware may need extra
> >> >> properties to find the right node. This documents the properties to use
> >> >> for this on sunxi platforms.
> >> >>
> >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> >> ---
> >> >>  .../bindings/video/simple-framebuffer-sunxi.txt    | 33 ++++++++++++++++++++++
> >> >>  1 file changed, 33 insertions(+)
> >> >>  create mode 100644 Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt b/Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt
> >> >> new file mode 100644
> >> >> index 0000000..84ca264
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt
> >> >> @@ -0,0 +1,33 @@
> >> >> +Sunxi specific Simple Framebuffer bindings
> >> >> +
> >> >> +This binding documents sunxi specific extensions to the simple-framebuffer
> >> >> +bindings. The sunxi simplefb u-boot code relies on the devicetree containing
> >> >> +pre-populated simplefb nodes.
> >> >> +
> >> >> +These extensions are intended so that u-boot can select the right node based
> >> >> +on which pipeline and output is being used. As such they are solely intended
> >> >> +for firmware / bootloader use, and the OS should ignore them.
> >> >> +
> >> >> +Required properties:
> >> >> +- compatible: "sunxi,framebuffer"
> >> >> +- sunxi,pipeline:
> >> >> +  <0> for the de_be0 -> lcdc0 -> output pipeline
> >> >> +  <1> for the de_be1 -> lcdc1 -> output pipeline
> >> >> +- sunxi,output: One of: "hdmi", "lcd", "vga", and "composite"
> >> >> +
> >> >> +Example:
> >> >> +
> >> >> +chosen {
> >> >> +     #address-cells = <1>;
> >> >> +     #size-cells = <1>;
> >> >> +     ranges;
> >> >> +
> >> >> +     framebuffer@0 {
> >> >> +             compatible = "sunxi,framebuffer", "simple-framebuffer";
> >> >> +             sunxi,pipeline = <0>;
> >> >> +             sunxi,output = "hdmi";
> >> >> +             clocks = <&pll5 1>, <&ahb_gates 36>, <&ahb_gates 43>,
> >> >> +                      <&ahb_gates 44>;
> >> >
> >> > If we're going that way, then maybe having to specify clock-names
> >> > would be better in order to know which clock is what?
> >>
> >> I wouldn't go that way with this binding since the driver has no need
> >> to differentiate between the clocks, and driver authors shouldn't be
> >> encouraged to do so. The purpose of the clocks in this node is only
> >> for itemizing dependencies, not for how to configure the clocks.
> >> Firmware shouldn't care at all about the clocks list, it only needs to
> >> find the correct pre-populated node to fill in and enable.
> >
> > Well, if we want to play the "DT as an ABI" stuff, you have no
> > guarantee that in the future, simplefb will still be the driver bound
> > to "sunxi,framebuffer", and that this driver doesn't need to
> > differentiate which clocks it needs to protect and which are not
> > needed. Especially with the combination of the output that you now
> > hardcode.
> 
> As long as the node claims compatibility with "simple-framebuffer",
> then it must abide by the simple framebuffer rules. It cannot depend
> on the driver having special knowledge about what to do with the
> clocks. It is fine for a driver that understands "sunxi,framebuffer"
> to enable additional features over and above simple-framebuffer,
> providing it doesn't break the vanilla "simple-framebuffer" interface.
> 
> The simple-framebuffer binding is clear that it will treat the clocks
> as dependencies only.

Isn't the compatible list precisely made to be able to define more
specific behaviours if needed?

If I get a list with "sunxi,framebuffer", "simple-framebuffer", I've
always thought that sunxi,framebuffer was defining exactly what the
device was about, and that simple-framebuffer was only aimed at
providing roughly the same functionnality, possibly with degraded
features, and with a compatible DT bindings.

I don't see how if we require a clock-names list in sunxi,framebuffer
that would break the binding of simple-framebuffer. It's just more
information, that might prove useful in the future.

> > Of course, if now we don't care at all about this ABI thing, it's
> > completely fine for me. But I'll archive this email preciously.
> 
> I do not in any way mean to say it is okay to break ABI.
> 
> Also, my comments above are advice that I would give to U-Boot
> authors, not something that should go in the binding. The
> simple-framebuffer binding doesn't need to know about the clock names
> at all, and if the assumption is that firmware will merely enable a
> pre-populated framebuffer node in the tree, then it probably doesn't
> need to parse the clocks property either. However, if it actually is
> better for the U-Boot implementation to have clocks with names in the
> node, then I'm not going to object.

I'm not speaking about u-boot here, but the kernel itself.

What if we want from that pipeline property to enable (or rather keep
enabled) only a few clocks, and not all of them?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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