diff mbox

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

Message ID 1416224086-5698-3-git-send-email-hdegoede@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hans de Goede Nov. 17, 2014, 11:34 a.m. UTC
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

Comments

Grant Likely Nov. 17, 2014, 12:18 p.m. UTC | #1
On Mon, Nov 17, 2014 at 11:34 AM, Hans de Goede <hdegoede@redhat.com> 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>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  .../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>;
> +               status = "disabled";
> +       };
> +};
> --
> 2.1.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Campbell Nov. 17, 2014, 12:22 p.m. UTC | #2
On Mon, 2014-11-17 at 12:34 +0100, Hans de Goede wrote:
> +- sunxi,pipeline:
> +  <0> for the de_be0 -> lcdc0 -> output pipeline
> +  <1> for the de_be1 -> lcdc1 -> output pipeline

Can we express this as a more descriptive string rather than a number,
to better handle future developments?

Otherwise both patches in this series look good to me.

> +- 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>;
> +		status = "disabled";
> +	};
> +};


--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard Nov. 17, 2014, 12:47 p.m. UTC | #3
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?

Maxime
Hans de Goede Nov. 17, 2014, 1:07 p.m. UTC | #4
Hi,

On 11/17/2014 01:47 PM, Maxime Ripard 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 agree that using clock-names is more human readable then phandle's,
but that is simply not how clocks are specified anywhere in dt.

Regards,

Hans


--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Likely Nov. 17, 2014, 1:39 p.m. UTC | #5
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.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard Nov. 17, 2014, 3:55 p.m. UTC | #6
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.

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.

Maxime
Hans de Goede Nov. 18, 2014, 8:15 a.m. UTC | #7
Hi,

On 11/17/2014 04:55 PM, Maxime Ripard 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"

Huh? We do have that guarantee, or at least that a simplefb bindings
compatible driver is, the "sunxi,framebuffer" bindings live in:

Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt

With the filename already being a big hint, further more it states that:

"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 is being used. As such they are solely intended for
firmware / bootloader use, and the OS should ignore them."

Which to me seems to guarantee that we're talking about a simple-framebuffer
compatible node here. The scope of these extensions are very narrow, there
only purpose is to allow the bootloader to find the right pre-populated simplefb
node, everything else is specified by the simplefb bindings, not these
extensions.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Campbell Nov. 18, 2014, 8:47 a.m. UTC | #8
On Tue, 2014-11-18 at 09:15 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/17/2014 04:55 PM, Maxime Ripard 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"
> 
> Huh? We do have that guarantee, or at least that a simplefb bindings
> compatible driver is, the "sunxi,framebuffer" bindings live in:
> 
> Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt
> 
> With the filename already being a big hint, further more it states that:
> 
> "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 is being used. As such they are solely intended for
> firmware / bootloader use, and the OS should ignore them."

There may be an argument here for using the slightly redundant compat
string "allwinner,simple-framebuffer" in order to reinforce that this is
an extension to simple-framebuffer, as opposed to a binding for a
completely separate "non-simple" sunxi framebuffer driver which I think
is what Maxime may be imagining.

Alternatively (or as well) perhaps these extension should be a series of
extensions (one per platform) described in "appendices" of the main
simple-framebuffer.txt. I know Grant expressed a preference for a
separate document though and I think just using the more expressive name
above would be sufficient, but thought I'd mention it as a possible
option.

Ian.

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede Nov. 18, 2014, 9:09 a.m. UTC | #9
Hi,

On 11/18/2014 09:47 AM, Ian Campbell wrote:
> On Tue, 2014-11-18 at 09:15 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/17/2014 04:55 PM, Maxime Ripard 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"
>>
>> Huh? We do have that guarantee, or at least that a simplefb bindings
>> compatible driver is, the "sunxi,framebuffer" bindings live in:
>>
>> Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt
>>
>> With the filename already being a big hint, further more it states that:
>>
>> "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 is being used. As such they are solely intended for
>> firmware / bootloader use, and the OS should ignore them."
> 
> There may be an argument here for using the slightly redundant compat
> string "allwinner,simple-framebuffer" in order to reinforce that this is
> an extension to simple-framebuffer, as opposed to a binding for a
> completely separate "non-simple" sunxi framebuffer driver which I think
> is what Maxime may be imagining.
> 
> Alternatively (or as well) perhaps these extension should be a series of
> extensions (one per platform) described in "appendices" of the main
> simple-framebuffer.txt. I know Grant expressed a preference for a
> separate document though and I think just using the more expressive name
> above would be sufficient, but thought I'd mention it as a possible
> option.

That is a good suggestion, as discussed on irc, lets go with that.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard Nov. 18, 2014, 9:29 a.m. UTC | #10
On Tue, Nov 18, 2014 at 09:15:34AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/17/2014 04:55 PM, Maxime Ripard 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"
> 
> Huh? We do have that guarantee, or at least that a simplefb bindings
> compatible driver is, the "sunxi,framebuffer" bindings live in:
> 
> Documentation/devicetree/bindings/video/simple-framebuffer-sunxi.txt
> 
> With the filename already being a big hint, further more it states that:

Yeah, well, that can change. The compatible itself can't, but we're
free to move around the drivers however we see fit (as long as the
user-space is not impacted)

> "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 is being used. As such they are solely intended for
> firmware / bootloader use, and the OS should ignore them."
> 
> Which to me seems to guarantee that we're talking about a simple-framebuffer
> compatible node here. The scope of these extensions are very narrow, there
> only purpose is to allow the bootloader to find the right pre-populated simplefb
> node, everything else is specified by the simplefb bindings, not these
> extensions.

My point was that what we might need to know these clocks at some
point in the future. But if you don't believe we will, it's fine for
me.

Maxime
Grant Likely Nov. 19, 2014, 2:05 p.m. UTC | #11
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.

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

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard Nov. 21, 2014, 2:36 p.m. UTC | #12
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
diff mbox

Patch

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>;
+		status = "disabled";
+	};
+};