[PATCH] powerpc/fsl: add device tree binding for QE firmware

[PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Define a binding for embedding a QE firmware blob into the device tree.  Also
define a new property for the QE node that links to a firmware node.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 .../powerpc/dts-bindings/fsl/cpm_qe/qe.txt         |   50 ++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
index 6e37be1..d9d6431 100644
--- a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
+++ b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
@@ -20,6 +20,14 @@ Required properties:
 - fsl,qe-num-riscs: define how many RISC engines the QE has.
 - fsl,qe-num-snums: define how many serial number(SNUM) the QE can use for the
   threads.
+- fsl,firmware-phandle:
+    Usage: required
+    Value type: <phandle>
+    Definition: Points to a firmware node (see "QE Firmware Node" below)
+        that contains the firmware that should be uploaded for this QE.
+        The compatible property for the firmware node should say,
+        "fsl,qe-firmware".
+
 
 Recommended properties
 - brg-frequency : the internal clock source frequency for baud-rate
@@ -59,3 +67,45 @@ Example:
 		reg = <0 c000>;
 	};
      };
+
+* QE Firmware Node
+
+This node defines a firmware binary that is embedded in the device tree, for
+the purpose of passing the firmware from bootloader to the kernel, or from
+the hypervisor to the guest.
+
+The firmware node itself contains the firmware binary contents, a compatible
+property, and any firmware-specific properties.  The node itself can be located
+anywhere, but should probably be placed at the top level.  The QE node
+that needs the firmware should define a property that links to the firmware
+node's phandle.
+
+This node is typically not defined in the DTS.  Instead, the boot loader
+normally creates the node from scratch, using a firmware binary that is already
+located in non-volatile storage or transferred from a tftp server.
+
+Required properties:
+  - compatible
+      Usage: required
+      Value type: <string>
+      Definition: A standard property.  Specify a string that indicates what
+          kind of firmware it is.  For QE, this should be "fsl,qe-firmware".
+
+   - fsl,firmware
+      Usage: required
+      Value type: <prop-encoded-array>, encoded as an array of bytes
+      Definition: A standard property.  This property contains the firmware
+          binary "blob".
+
+Example:
+	qe@e0080000 {
+		compatible = "fsl,qe";
+		fsl,firmware-phandle = <&qe_firmware>;
+		...
+	}
+
+	qe_firmware:qe-firmware {
+		compatible = "fsl,qe-firmware";
+		fsl,firmware = <0x70 0xcd 0x00 0x00 0x01 0x46 0x45 0x63 ...>
+	}
+
-- 
1.6.5

[PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Define a binding for embedding a QE firmware blob into the device tree.  Also
define a new property for the QE node that links to a firmware node.

Signed-off-by: Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 .../powerpc/dts-bindings/fsl/cpm_qe/qe.txt         |   50 ++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
index 6e37be1..d9d6431 100644
--- a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
+++ b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
@@ -20,6 +20,14 @@ Required properties:
 - fsl,qe-num-riscs: define how many RISC engines the QE has.
 - fsl,qe-num-snums: define how many serial number(SNUM) the QE can use for the
   threads.
+- fsl,firmware-phandle:
+    Usage: required
+    Value type: <phandle>
+    Definition: Points to a firmware node (see "QE Firmware Node" below)
+        that contains the firmware that should be uploaded for this QE.
+        The compatible property for the firmware node should say,
+        "fsl,qe-firmware".
+
 
 Recommended properties
 - brg-frequency : the internal clock source frequency for baud-rate
@@ -59,3 +67,45 @@ Example:
 		reg = <0 c000>;
 	};
      };
+
+* QE Firmware Node
+
+This node defines a firmware binary that is embedded in the device tree, for
+the purpose of passing the firmware from bootloader to the kernel, or from
+the hypervisor to the guest.
+
+The firmware node itself contains the firmware binary contents, a compatible
+property, and any firmware-specific properties.  The node itself can be located
+anywhere, but should probably be placed at the top level.  The QE node
+that needs the firmware should define a property that links to the firmware
+node's phandle.
+
+This node is typically not defined in the DTS.  Instead, the boot loader
+normally creates the node from scratch, using a firmware binary that is already
+located in non-volatile storage or transferred from a tftp server.
+
+Required properties:
+  - compatible
+      Usage: required
+      Value type: <string>
+      Definition: A standard property.  Specify a string that indicates what
+          kind of firmware it is.  For QE, this should be "fsl,qe-firmware".
+
+   - fsl,firmware
+      Usage: required
+      Value type: <prop-encoded-array>, encoded as an array of bytes
+      Definition: A standard property.  This property contains the firmware
+          binary "blob".
+
+Example:
+	qe@e0080000 {
+		compatible = "fsl,qe";
+		fsl,firmware-phandle = <&qe_firmware>;
+		...
+	}
+
+	qe_firmware:qe-firmware {
+		compatible = "fsl,qe-firmware";
+		fsl,firmware = <0x70 0xcd 0x00 0x00 0x01 0x46 0x45 0x63 ...>
+	}
+
-- 
1.6.5

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Tue, Mar 23, 2010 at 3:42 PM, Timur Tabi <timur@freescale.com> wrote:
> Define a binding for embedding a QE firmware blob into the device tree. =
=A0Also
> define a new property for the QE node that links to a firmware node.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> =A0.../powerpc/dts-bindings/fsl/cpm_qe/qe.txt =A0 =A0 =A0 =A0 | =A0 50 ++=
++++++++++++++++++
> =A01 files changed, 50 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt b/Docum=
entation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
> index 6e37be1..d9d6431 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
> @@ -20,6 +20,14 @@ Required properties:
> =A0- fsl,qe-num-riscs: define how many RISC engines the QE has.
> =A0- fsl,qe-num-snums: define how many serial number(SNUM) the QE can use=
 for the
> =A0 threads.
> +- fsl,firmware-phandle:
> + =A0 =A0Usage: required
> + =A0 =A0Value type: <phandle>
> + =A0 =A0Definition: Points to a firmware node (see "QE Firmware Node" be=
low)
> + =A0 =A0 =A0 =A0that contains the firmware that should be uploaded for t=
his QE.
> + =A0 =A0 =A0 =A0The compatible property for the firmware node should say=
,
> + =A0 =A0 =A0 =A0"fsl,qe-firmware".
> +

Why the phandle redirection?  Why not just put the firmware blob into
a property in the QE node, or as a subnode?

g.

>
> =A0Recommended properties
> =A0- brg-frequency : the internal clock source frequency for baud-rate
> @@ -59,3 +67,45 @@ Example:
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <0 c000>;
> =A0 =A0 =A0 =A0};
> =A0 =A0 =A0};
> +
> +* QE Firmware Node
> +
> +This node defines a firmware binary that is embedded in the device tree,=
 for
> +the purpose of passing the firmware from bootloader to the kernel, or fr=
om
> +the hypervisor to the guest.
> +
> +The firmware node itself contains the firmware binary contents, a compat=
ible
> +property, and any firmware-specific properties. =A0The node itself can b=
e located
> +anywhere, but should probably be placed at the top level. =A0The QE node
> +that needs the firmware should define a property that links to the firmw=
are
> +node's phandle.
> +
> +This node is typically not defined in the DTS. =A0Instead, the boot load=
er
> +normally creates the node from scratch, using a firmware binary that is =
already
> +located in non-volatile storage or transferred from a tftp server.
> +
> +Required properties:
> + =A0- compatible
> + =A0 =A0 =A0Usage: required
> + =A0 =A0 =A0Value type: <string>
> + =A0 =A0 =A0Definition: A standard property. =A0Specify a string that in=
dicates what
> + =A0 =A0 =A0 =A0 =A0kind of firmware it is. =A0For QE, this should be "f=
sl,qe-firmware".
> +
> + =A0 - fsl,firmware
> + =A0 =A0 =A0Usage: required
> + =A0 =A0 =A0Value type: <prop-encoded-array>, encoded as an array of byt=
es
> + =A0 =A0 =A0Definition: A standard property. =A0This property contains t=
he firmware
> + =A0 =A0 =A0 =A0 =A0binary "blob".
> +
> +Example:
> + =A0 =A0 =A0 qe@e0080000 {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,qe";
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fsl,firmware-phandle =3D <&qe_firmware>;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ...
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 qe_firmware:qe-firmware {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,qe-firmware";
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fsl,firmware =3D <0x70 0xcd 0x00 0x00 0x01 =
0x46 0x45 0x63 ...>
> + =A0 =A0 =A0 }
> +
> --
> 1.6.5
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Tue, Mar 23, 2010 at 3:42 PM, Timur Tabi <timur@freescale.com> wrote:
> Define a binding for embedding a QE firmware blob into the device tree.  Also
> define a new property for the QE node that links to a firmware node.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  .../powerpc/dts-bindings/fsl/cpm_qe/qe.txt         |   50 ++++++++++++++++++++
>  1 files changed, 50 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
> index 6e37be1..d9d6431 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
> @@ -20,6 +20,14 @@ Required properties:
>  - fsl,qe-num-riscs: define how many RISC engines the QE has.
>  - fsl,qe-num-snums: define how many serial number(SNUM) the QE can use for the
>   threads.
> +- fsl,firmware-phandle:
> +    Usage: required
> +    Value type: <phandle>
> +    Definition: Points to a firmware node (see "QE Firmware Node" below)
> +        that contains the firmware that should be uploaded for this QE.
> +        The compatible property for the firmware node should say,
> +        "fsl,qe-firmware".
> +

Why the phandle redirection?  Why not just put the firmware blob into
a property in the QE node, or as a subnode?

g.

>
>  Recommended properties
>  - brg-frequency : the internal clock source frequency for baud-rate
> @@ -59,3 +67,45 @@ Example:
>                reg = <0 c000>;
>        };
>      };
> +
> +* QE Firmware Node
> +
> +This node defines a firmware binary that is embedded in the device tree, for
> +the purpose of passing the firmware from bootloader to the kernel, or from
> +the hypervisor to the guest.
> +
> +The firmware node itself contains the firmware binary contents, a compatible
> +property, and any firmware-specific properties.  The node itself can be located
> +anywhere, but should probably be placed at the top level.  The QE node
> +that needs the firmware should define a property that links to the firmware
> +node's phandle.
> +
> +This node is typically not defined in the DTS.  Instead, the boot loader
> +normally creates the node from scratch, using a firmware binary that is already
> +located in non-volatile storage or transferred from a tftp server.
> +
> +Required properties:
> +  - compatible
> +      Usage: required
> +      Value type: <string>
> +      Definition: A standard property.  Specify a string that indicates what
> +          kind of firmware it is.  For QE, this should be "fsl,qe-firmware".
> +
> +   - fsl,firmware
> +      Usage: required
> +      Value type: <prop-encoded-array>, encoded as an array of bytes
> +      Definition: A standard property.  This property contains the firmware
> +          binary "blob".
> +
> +Example:
> +       qe@e0080000 {
> +               compatible = "fsl,qe";
> +               fsl,firmware-phandle = <&qe_firmware>;
> +               ...
> +       }
> +
> +       qe_firmware:qe-firmware {
> +               compatible = "fsl,qe-firmware";
> +               fsl,firmware = <0x70 0xcd 0x00 0x00 0x01 0x46 0x45 0x63 ...>
> +       }
> +
> --
> 1.6.5
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

On Wed, Mar 24, 2010 at 1:07 AM, Grant Likely <grant.likely@secretlab.ca> w=
rote:

>> +- fsl,firmware-phandle:
>> + =A0 =A0Usage: required
>> + =A0 =A0Value type: <phandle>
>> + =A0 =A0Definition: Points to a firmware node (see "QE Firmware Node" b=
elow)
>> + =A0 =A0 =A0 =A0that contains the firmware that should be uploaded for =
this QE.
>> + =A0 =A0 =A0 =A0The compatible property for the firmware node should sa=
y,
>> + =A0 =A0 =A0 =A0"fsl,qe-firmware".
>> +
>
> Why the phandle redirection? =A0Why not just put the firmware blob into
> a property in the QE node, or as a subnode?

Because there might be multiple QE devices on a single chip, and each
will need to upload the same firmware.  So instead of embedding the
firmware multiple times, just embed it once, and have a pointer.

I could expand the binding to say that a node should look for either
fsl,firmware-phandle *or* a child fsl,qe-firmware node, but I think
that overly complicates things.  It also makes it more complicated for
the boot loader to create the fsl,qe-firmware node, since it has to
figure out where to put that node.  I can imaging a situation where
the DTS has the fsl,firmware-phandle properties and an empty
fsl,qe-firmware node at the root, and the boot loader just fills it
in.

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

On Wed, Mar 24, 2010 at 1:07 AM, Grant Likely <grant.likely@secretlab.ca> wrote:

>> +- fsl,firmware-phandle:
>> +    Usage: required
>> +    Value type: <phandle>
>> +    Definition: Points to a firmware node (see "QE Firmware Node" below)
>> +        that contains the firmware that should be uploaded for this QE.
>> +        The compatible property for the firmware node should say,
>> +        "fsl,qe-firmware".
>> +
>
> Why the phandle redirection?  Why not just put the firmware blob into
> a property in the QE node, or as a subnode?

Because there might be multiple QE devices on a single chip, and each
will need to upload the same firmware.  So instead of embedding the
firmware multiple times, just embed it once, and have a pointer.

I could expand the binding to say that a node should look for either
fsl,firmware-phandle *or* a child fsl,qe-firmware node, but I think
that overly complicates things.  It also makes it more complicated for
the boot loader to create the fsl,qe-firmware node, since it has to
figure out where to put that node.  I can imaging a situation where
the DTS has the fsl,firmware-phandle properties and an empty
fsl,qe-firmware node at the root, and the boot loader just fills it
in.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Segher Boessenkool]

>> Why the phandle redirection?  Why not just put the firmware blob into
>> a property in the QE node, or as a subnode?
>
> Because there might be multiple QE devices on a single chip, and each
> will need to upload the same firmware.  So instead of embedding the
> firmware multiple times, just embed it once, and have a pointer.

You're messing up the binding because of a (perceived) deficiency in
the DTB format?  Or maybe just the DTS format.  Or maybe you shouldn't
even care about size here.  Or really, the device tree is the wrong
place to store firmware blobs at all.


Segher

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Segher Boessenkool]

>> Why the phandle redirection?  Why not just put the firmware blob into
>> a property in the QE node, or as a subnode?
>
> Because there might be multiple QE devices on a single chip, and each
> will need to upload the same firmware.  So instead of embedding the
> firmware multiple times, just embed it once, and have a pointer.

You're messing up the binding because of a (perceived) deficiency in
the DTB format?  Or maybe just the DTS format.  Or maybe you shouldn't
even care about size here.  Or really, the device tree is the wrong
place to store firmware blobs at all.


Segher

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Wed, Mar 24, 2010 at 11:00 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>>> Why the phandle redirection? =A0Why not just put the firmware blob into
>>> a property in the QE node, or as a subnode?
>>
>> Because there might be multiple QE devices on a single chip, and each
>> will need to upload the same firmware. =A0So instead of embedding the
>> firmware multiple times, just embed it once, and have a pointer.
>
> You're messing up the binding because of a (perceived) deficiency in
> the DTB format? =A0Or maybe just the DTS format. =A0Or maybe you shouldn'=
t
> even care about size here. =A0Or really, the device tree is the wrong
> place to store firmware blobs at all.

That is a good question.  Why is it necessary to pass the blob via the
tree?  So far we've avoided using firmware blobs in the flat trees.
Or to ask in other words; what is the use case that requires passing
via the device tree?

Also, depending on firmware to correctly squirt the firmware blob into
the dtb at boot is risky.  Even when firmware is buggy, there is
resistance to upgrading firmware on working boards because it could
result in a bricked board.  In fact, every time we depend on firmware
to modify the dtb at boot is risky, so it should only be done when
strictly necessary (I would even say that to date we've probably been
rather too liberal about getting u-boot to modify the device tree).

I would say that either the firmware should be loaded via the existing
(non-dt) firmware loading mechanism, or it should be built into the
static dtb blob.  Don't try to add it at runtime.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Wed, Mar 24, 2010 at 11:00 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>>> Why the phandle redirection?  Why not just put the firmware blob into
>>> a property in the QE node, or as a subnode?
>>
>> Because there might be multiple QE devices on a single chip, and each
>> will need to upload the same firmware.  So instead of embedding the
>> firmware multiple times, just embed it once, and have a pointer.
>
> You're messing up the binding because of a (perceived) deficiency in
> the DTB format?  Or maybe just the DTS format.  Or maybe you shouldn't
> even care about size here.  Or really, the device tree is the wrong
> place to store firmware blobs at all.

That is a good question.  Why is it necessary to pass the blob via the
tree?  So far we've avoided using firmware blobs in the flat trees.
Or to ask in other words; what is the use case that requires passing
via the device tree?

Also, depending on firmware to correctly squirt the firmware blob into
the dtb at boot is risky.  Even when firmware is buggy, there is
resistance to upgrading firmware on working boards because it could
result in a bricked board.  In fact, every time we depend on firmware
to modify the dtb at boot is risky, so it should only be done when
strictly necessary (I would even say that to date we've probably been
rather too liberal about getting u-boot to modify the device tree).

I would say that either the firmware should be loaded via the existing
(non-dt) firmware loading mechanism, or it should be built into the
static dtb blob.  Don't try to add it at runtime.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:
> On Wed, Mar 24, 2010 at 11:00 AM, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
>>>> Why the phandle redirection?  Why not just put the firmware blob into
>>>> a property in the QE node, or as a subnode?
>>>
>>> Because there might be multiple QE devices on a single chip, and each
>>> will need to upload the same firmware.  So instead of embedding the
>>> firmware multiple times, just embed it once, and have a pointer.
>>
>> You're messing up the binding because of a (perceived) deficiency in
>> the DTB format? 

Huh?  Who says anything about messing up?  I don't see anything "messed up" about including a blob of data with proper compatible properties, etc.

>> Or maybe just the DTS format.  Or maybe you shouldn't
>> even care about size here.  Or really, the device tree is the wrong
>> place to store firmware blobs at all.
> 
> That is a good question.  Why is it necessary to pass the blob via the
> tree? 

Because sometimes the firmware is needed before networking or serial I/O can function.  Today, we do one of two things on systems with QE (or QE-like microcontrollers):

1) U-Boot uploads the firmware, and may create a DTB node that provides some information about the firmware.

2) U-Boot uploads the firmware, but then gives Linux the physical address (in flash) of the firmware so that it can upload it again.

> So far we've avoided using firmware blobs in the flat trees.
> Or to ask in other words; what is the use case that requires passing
> via the device tree?

The Fman devices on the Freescale P4080 needs to have the firmware uploaded by the kernel before they will function.  I can't depend on having the firmware on the root file system, and we can't embed it in the kernel itself (because it's proprietary), so where else should I put it?  Today, we just leave it in flash and give the physical address to the Fman Linux driver via a command-line parameter.  But that doesn't work because then it means we have to have flash mapped to every partition that runs Linux.  

> Also, depending on firmware to correctly squirt the firmware blob into
> the dtb at boot is risky.  Even when firmware is buggy, there is
> resistance to upgrading firmware on working boards because it could
> result in a bricked board.

I'm not sure what you're getting at.  This has nothing to do with upgrading firmware.  The firmware is already in flash, I just need a better way of giving it to the kernel.  If you upgrade the firmware in flash, then U-Boot will automatically provide the new version to the kernel via the DTB.  I just don't see how upgrading is a factor.

>  In fact, every time we depend on firmware
> to modify the dtb at boot is risky, so it should only be done when
> strictly necessary (I would even say that to date we've probably been
> rather too liberal about getting u-boot to modify the device tree).

Embedding the firmware blob in the DTS is uglier than having U-Boot do it, IMHO.

> I would say that either the firmware should be loaded via the existing
> (non-dt) firmware loading mechanism, 

That, unfortunately, is not an option.

> or it should be built into the
> static dtb blob.  Don't try to add it at runtime.

Then how do I distribute the firmware blob?  It's not GPL, so it can't go into arch/powerpc/boot/dts/.  Are you suggesting I do this in the DTS:

/ {
	model = "MPC8323EMDS";
	compatible = "MPC8323EMDS", "MPC832xMDS", "MPC83xxMDS";
	#address-cells = <1>;
	#size-cells = <1>;

...

	qe_firmware:qe-firmware {
		compatible = "fsl,qe-firmware";
		fsl,firmware = <0x70 0xcd 0x00 0x00 0x01 0x46 0x45 0x63 ...>
	}
}

Most firmware is 8-12KB, so this will make for one ugly DTS.  Plus, there's the issue of distributing non-GPL firmware data inside a DTS, which is GPL.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:
> On Wed, Mar 24, 2010 at 11:00 AM, Segher Boessenkool
> <segher-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
>>>> Why the phandle redirection?  Why not just put the firmware blob into
>>>> a property in the QE node, or as a subnode?
>>>
>>> Because there might be multiple QE devices on a single chip, and each
>>> will need to upload the same firmware.  So instead of embedding the
>>> firmware multiple times, just embed it once, and have a pointer.
>>
>> You're messing up the binding because of a (perceived) deficiency in
>> the DTB format? 

Huh?  Who says anything about messing up?  I don't see anything "messed up" about including a blob of data with proper compatible properties, etc.

>> Or maybe just the DTS format.  Or maybe you shouldn't
>> even care about size here.  Or really, the device tree is the wrong
>> place to store firmware blobs at all.
> 
> That is a good question.  Why is it necessary to pass the blob via the
> tree? 

Because sometimes the firmware is needed before networking or serial I/O can function.  Today, we do one of two things on systems with QE (or QE-like microcontrollers):

1) U-Boot uploads the firmware, and may create a DTB node that provides some information about the firmware.

2) U-Boot uploads the firmware, but then gives Linux the physical address (in flash) of the firmware so that it can upload it again.

> So far we've avoided using firmware blobs in the flat trees.
> Or to ask in other words; what is the use case that requires passing
> via the device tree?

The Fman devices on the Freescale P4080 needs to have the firmware uploaded by the kernel before they will function.  I can't depend on having the firmware on the root file system, and we can't embed it in the kernel itself (because it's proprietary), so where else should I put it?  Today, we just leave it in flash and give the physical address to the Fman Linux driver via a command-line parameter.  But that doesn't work because then it means we have to have flash mapped to every partition that runs Linux.  

> Also, depending on firmware to correctly squirt the firmware blob into
> the dtb at boot is risky.  Even when firmware is buggy, there is
> resistance to upgrading firmware on working boards because it could
> result in a bricked board.

I'm not sure what you're getting at.  This has nothing to do with upgrading firmware.  The firmware is already in flash, I just need a better way of giving it to the kernel.  If you upgrade the firmware in flash, then U-Boot will automatically provide the new version to the kernel via the DTB.  I just don't see how upgrading is a factor.

>  In fact, every time we depend on firmware
> to modify the dtb at boot is risky, so it should only be done when
> strictly necessary (I would even say that to date we've probably been
> rather too liberal about getting u-boot to modify the device tree).

Embedding the firmware blob in the DTS is uglier than having U-Boot do it, IMHO.

> I would say that either the firmware should be loaded via the existing
> (non-dt) firmware loading mechanism, 

That, unfortunately, is not an option.

> or it should be built into the
> static dtb blob.  Don't try to add it at runtime.

Then how do I distribute the firmware blob?  It's not GPL, so it can't go into arch/powerpc/boot/dts/.  Are you suggesting I do this in the DTS:

/ {
	model = "MPC8323EMDS";
	compatible = "MPC8323EMDS", "MPC832xMDS", "MPC83xxMDS";
	#address-cells = <1>;
	#size-cells = <1>;

...

	qe_firmware:qe-firmware {
		compatible = "fsl,qe-firmware";
		fsl,firmware = <0x70 0xcd 0x00 0x00 0x01 0x46 0x45 0x63 ...>
	}
}

Most firmware is 8-12KB, so this will make for one ugly DTS.  Plus, there's the issue of distributing non-GPL firmware data inside a DTS, which is GPL.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Wed, Mar 24, 2010 at 11:31 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>> On Wed, Mar 24, 2010 at 11:00 AM, Segher Boessenkool
>> <segher@kernel.crashing.org> wrote:
>>>>> Why the phandle redirection? =A0Why not just put the firmware blob in=
to
>>>>> a property in the QE node, or as a subnode?
>>>>
>>>> Because there might be multiple QE devices on a single chip, and each
>>>> will need to upload the same firmware. =A0So instead of embedding the
>>>> firmware multiple times, just embed it once, and have a pointer.
>>>
>>> You're messing up the binding because of a (perceived) deficiency in
>>> the DTB format?
>
> Huh? =A0Who says anything about messing up? =A0I don't see anything "mess=
ed up" about including a blob of data with proper compatible properties, et=
c.
>
>>> Or maybe just the DTS format. =A0Or maybe you shouldn't
>>> even care about size here. =A0Or really, the device tree is the wrong
>>> place to store firmware blobs at all.
>>
>> That is a good question. =A0Why is it necessary to pass the blob via the
>> tree?
>
> Because sometimes the firmware is needed before networking or serial I/O =
can function. =A0Today, we do one of two things on systems with QE (or QE-l=
ike microcontrollers):
>
> 1) U-Boot uploads the firmware, and may create a DTB node that provides s=
ome information about the firmware.
>
> 2) U-Boot uploads the firmware, but then gives Linux the physical address=
 (in flash) of the firmware so that it can upload it again.
>
>> So far we've avoided using firmware blobs in the flat trees.
>> Or to ask in other words; what is the use case that requires passing
>> via the device tree?
>
> The Fman devices on the Freescale P4080 needs to have the firmware upload=
ed by the kernel before they will function. =A0I can't depend on having the=
 firmware on the root file system, and we can't embed it in the kernel itse=
lf (because it's proprietary), so where else should I put it? =A0Today, we =
just leave it in flash and give the physical address to the Fman Linux driv=
er via a command-line parameter. =A0But that doesn't work because then it m=
eans we have to have flash mapped to every partition that runs Linux.

Thanks.  This is important information when talking about his.

You can also put it into an initrd and use the Linux firmware loader.
That would also neatly solve your GPL distribution issues.

>> Also, depending on firmware to correctly squirt the firmware blob into
>> the dtb at boot is risky. =A0Even when firmware is buggy, there is
>> resistance to upgrading firmware on working boards because it could
>> result in a bricked board.
>
> I'm not sure what you're getting at. =A0This has nothing to do with upgra=
ding firmware. =A0The firmware is already in flash, I just need a better wa=
y of giving it to the kernel. =A0If you upgrade the firmware in flash, then=
 U-Boot will automatically provide the new version to the kernel via the DT=
B. =A0I just don't see how upgrading is a factor.

Heh, can't you understand my completely ambiguous statements?  Sorry
about that, I should have been clearer.  Let me rephrase...

Also, depending on U-Boot (or any other boot firmware) to correctly
squirt the QE blob into
the dtb at boot is risky.  Even when U-Boot is buggy, there is
resistance to upgrading U-Boot
on working boards because it could result in a bricked board.

>> =A0In fact, every time we depend on firmware

to revise my statement: s/firmware/U-Boot/

>> to modify the dtb at boot is risky, so it should only be done when
>> strictly necessary (I would even say that to date we've probably been
>> rather too liberal about getting u-boot to modify the device tree).
>
> Embedding the firmware blob in the DTS is uglier than having U-Boot do it=
, IMHO.

Not with the right syntax, see below...

>> I would say that either the firmware should be loaded via the existing
>> (non-dt) firmware loading mechanism,
>
> That, unfortunately, is not an option.
>
>> or it should be built into the
>> static dtb blob. =A0Don't try to add it at runtime.
>
> Then how do I distribute the firmware blob? =A0It's not GPL, so it can't =
go into arch/powerpc/boot/dts/. =A0Are you suggesting I do this in the DTS:
>
> / {
> =A0 =A0 =A0 =A0model =3D "MPC8323EMDS";
> =A0 =A0 =A0 =A0compatible =3D "MPC8323EMDS", "MPC832xMDS", "MPC83xxMDS";
> =A0 =A0 =A0 =A0#address-cells =3D <1>;
> =A0 =A0 =A0 =A0#size-cells =3D <1>;
>
> ...
>
> =A0 =A0 =A0 =A0qe_firmware:qe-firmware {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "fsl,qe-firmware";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl,firmware =3D <0x70 0xcd 0x00 0x00 0x01=
 0x46 0x45 0x63 ...>
> =A0 =A0 =A0 =A0}

You're right, that wouldn't be very nice.  Try this syntax instead:

fsl,firmware =3D /incbin/("firmware-file-name.bin");

> }
>
> Most firmware is 8-12KB, so this will make for one ugly DTS. =A0Plus, the=
re's the issue of distributing non-GPL firmware data inside a DTS, which is=
 GPL.

You've got the distribution problem that needs to be solved regardless
because it cannot be part of U-Boot either.  How do you plan to handle
QE firmware distribution and loading?

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Wed, Mar 24, 2010 at 11:31 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>> On Wed, Mar 24, 2010 at 11:00 AM, Segher Boessenkool
>> <segher@kernel.crashing.org> wrote:
>>>>> Why the phandle redirection?  Why not just put the firmware blob into
>>>>> a property in the QE node, or as a subnode?
>>>>
>>>> Because there might be multiple QE devices on a single chip, and each
>>>> will need to upload the same firmware.  So instead of embedding the
>>>> firmware multiple times, just embed it once, and have a pointer.
>>>
>>> You're messing up the binding because of a (perceived) deficiency in
>>> the DTB format?
>
> Huh?  Who says anything about messing up?  I don't see anything "messed up" about including a blob of data with proper compatible properties, etc.
>
>>> Or maybe just the DTS format.  Or maybe you shouldn't
>>> even care about size here.  Or really, the device tree is the wrong
>>> place to store firmware blobs at all.
>>
>> That is a good question.  Why is it necessary to pass the blob via the
>> tree?
>
> Because sometimes the firmware is needed before networking or serial I/O can function.  Today, we do one of two things on systems with QE (or QE-like microcontrollers):
>
> 1) U-Boot uploads the firmware, and may create a DTB node that provides some information about the firmware.
>
> 2) U-Boot uploads the firmware, but then gives Linux the physical address (in flash) of the firmware so that it can upload it again.
>
>> So far we've avoided using firmware blobs in the flat trees.
>> Or to ask in other words; what is the use case that requires passing
>> via the device tree?
>
> The Fman devices on the Freescale P4080 needs to have the firmware uploaded by the kernel before they will function.  I can't depend on having the firmware on the root file system, and we can't embed it in the kernel itself (because it's proprietary), so where else should I put it?  Today, we just leave it in flash and give the physical address to the Fman Linux driver via a command-line parameter.  But that doesn't work because then it means we have to have flash mapped to every partition that runs Linux.

Thanks.  This is important information when talking about his.

You can also put it into an initrd and use the Linux firmware loader.
That would also neatly solve your GPL distribution issues.

>> Also, depending on firmware to correctly squirt the firmware blob into
>> the dtb at boot is risky.  Even when firmware is buggy, there is
>> resistance to upgrading firmware on working boards because it could
>> result in a bricked board.
>
> I'm not sure what you're getting at.  This has nothing to do with upgrading firmware.  The firmware is already in flash, I just need a better way of giving it to the kernel.  If you upgrade the firmware in flash, then U-Boot will automatically provide the new version to the kernel via the DTB.  I just don't see how upgrading is a factor.

Heh, can't you understand my completely ambiguous statements?  Sorry
about that, I should have been clearer.  Let me rephrase...

Also, depending on U-Boot (or any other boot firmware) to correctly
squirt the QE blob into
the dtb at boot is risky.  Even when U-Boot is buggy, there is
resistance to upgrading U-Boot
on working boards because it could result in a bricked board.

>>  In fact, every time we depend on firmware

to revise my statement: s/firmware/U-Boot/

>> to modify the dtb at boot is risky, so it should only be done when
>> strictly necessary (I would even say that to date we've probably been
>> rather too liberal about getting u-boot to modify the device tree).
>
> Embedding the firmware blob in the DTS is uglier than having U-Boot do it, IMHO.

Not with the right syntax, see below...

>> I would say that either the firmware should be loaded via the existing
>> (non-dt) firmware loading mechanism,
>
> That, unfortunately, is not an option.
>
>> or it should be built into the
>> static dtb blob.  Don't try to add it at runtime.
>
> Then how do I distribute the firmware blob?  It's not GPL, so it can't go into arch/powerpc/boot/dts/.  Are you suggesting I do this in the DTS:
>
> / {
>        model = "MPC8323EMDS";
>        compatible = "MPC8323EMDS", "MPC832xMDS", "MPC83xxMDS";
>        #address-cells = <1>;
>        #size-cells = <1>;
>
> ...
>
>        qe_firmware:qe-firmware {
>                compatible = "fsl,qe-firmware";
>                fsl,firmware = <0x70 0xcd 0x00 0x00 0x01 0x46 0x45 0x63 ...>
>        }

You're right, that wouldn't be very nice.  Try this syntax instead:

fsl,firmware = /incbin/("firmware-file-name.bin");

> }
>
> Most firmware is 8-12KB, so this will make for one ugly DTS.  Plus, there's the issue of distributing non-GPL firmware data inside a DTS, which is GPL.

You've got the distribution problem that needs to be solved regardless
because it cannot be part of U-Boot either.  How do you plan to handle
QE firmware distribution and loading?

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Mitch Bradley]

>
> > Most firmware is 8-12KB, so this will make for one ugly DTS.  Plus, there's the issue of distributing non-GPL firmware data inside a DTS, which is GPL.
>   
>
> You've got the distribution problem that needs to be solved regardless
> because it cannot be part of U-Boot either.  How do you plan to handle
> QE firmware distribution and loading?
>   

Or you could use real Open Firmware, which is free as in free, instead 
of free as in whatever RMS says.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Mitch Bradley]

>
> > Most firmware is 8-12KB, so this will make for one ugly DTS.  Plus, there's the issue of distributing non-GPL firmware data inside a DTS, which is GPL.
>   
>
> You've got the distribution problem that needs to be solved regardless
> because it cannot be part of U-Boot either.  How do you plan to handle
> QE firmware distribution and loading?
>   

Or you could use real Open Firmware, which is free as in free, instead 
of free as in whatever RMS says.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:

> Thanks.  This is important information when talking about his.
> 
> You can also put it into an initrd and use the Linux firmware loader.

We may also need to support non-Linux operating systems that don't use an initrd.  For now, an initrd might work.  I don't know how I'll convince our BSP team to start using one, though.

> That would also neatly solve your GPL distribution issues.

True.

> Also, depending on U-Boot (or any other boot firmware) to correctly
> squirt the QE blob into
> the dtb at boot is risky.  Even when U-Boot is buggy, there is
> resistance to upgrading U-Boot
> on working boards because it could result in a bricked board.

Ok, that makes more sense, but we're not talking about upgrading U-Boot.  Once U-Boot has the capability to inject the QE blob into the DTB, then upgrading the QE blob won't require an upgrade to U-Boot.  The QE blob is still a QE blob.

> You're right, that wouldn't be very nice.  Try this syntax instead:
> 
> fsl,firmware = /incbin/("firmware-file-name.bin");

It's not a bad idea, but it would require firmware-file-name.bin to be distributed with the kernel itself, otherwise building the DTB will be complicated.  The path to firmware-file-name.bin would need to be hard-coded in the DTS.

> You've got the distribution problem that needs to be solved regardless
> because it cannot be part of U-Boot either.  How do you plan to handle
> QE firmware distribution and loading?

Today, we just put the QE blob somewhere in flash, and then U-Boot is told about like this:

#define CONFIG_SYS_QE_FW_ADDR	0xfff00000

Then we have GPL code in U-Boot that uploads it.  

But you do have a point -- once we embed the QE blob in the DTB, whether it's a DTB created by DTC or updated by U-Boot, we might already have a GPL issue.  I'll have to get back to you on that one.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[M. Warner Losh]

In message: <4BAA4C8A.70104@freescale.com>
            Timur Tabi <timur@freescale.com> writes: 
: Most firmware is 8-12KB, so this will make for one ugly DTS.  Plus,
: there's the issue of distributing non-GPL firmware data inside a
: DTS, which is GPL.

Is there some reason that the firmware couldn't be loaded by uboot as
part of its initialization?

Warner

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

M. Warner Losh wrote:

> Is there some reason that the firmware couldn't be loaded by uboot as
> part of its initialization?

Unfortunately, our hardware is frequently designed such that the same firmware needs to be loaded by U-Boot and then again by the kernel.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

M. Warner Losh wrote:

> Is there some reason that the firmware couldn't be loaded by uboot as
> part of its initialization?

Unfortunately, our hardware is frequently designed such that the same firmware needs to be loaded by U-Boot and then again by the kernel.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Segher Boessenkool]

>>>>> Why the phandle redirection?  Why not just put the firmware blob into
>>>>> a property in the QE node, or as a subnode?
>>>>
>>>> Because there might be multiple QE devices on a single chip, and each
>>>> will need to upload the same firmware.  So instead of embedding the
>>>> firmware multiple times, just embed it once, and have a pointer.
>>>
>>> You're messing up the binding because of a (perceived) deficiency in
>>> the DTB format?
>
> Huh?  Who says anything about messing up?

I do; I consider that indirection thing (and putting firmware blobs
in the device tree at all, but to a lesser extent) as making a mess
of your device binding.

Let's forget that I do not like putting a firmware blob in the
device tree if you can at all avoid that; Grant is on that already.

As far as I can see, you want that indirection node so that you
safe space in the DTB.  With real OF it is trivial to not have
multiple copies of the data if you want a few properties with
the same data.  There is no reason this could not be done in DTB
as well (and some way in DTS to express that, or maybe the tools
could auto-detect it, whatever).

Or you could just zip the DTB.

>> That is a good question.  Why is it necessary to pass the blob via the
>> tree?
>
> Because sometimes the firmware is needed before networking or serial I/O
> can function.

Can't you link it into the kernel then?  Seems a better place for
it to me.  Of course you said something about GPL, heh.


Segher

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Segher Boessenkool]

>>>>> Why the phandle redirection?  Why not just put the firmware blob into
>>>>> a property in the QE node, or as a subnode?
>>>>
>>>> Because there might be multiple QE devices on a single chip, and each
>>>> will need to upload the same firmware.  So instead of embedding the
>>>> firmware multiple times, just embed it once, and have a pointer.
>>>
>>> You're messing up the binding because of a (perceived) deficiency in
>>> the DTB format?
>
> Huh?  Who says anything about messing up?

I do; I consider that indirection thing (and putting firmware blobs
in the device tree at all, but to a lesser extent) as making a mess
of your device binding.

Let's forget that I do not like putting a firmware blob in the
device tree if you can at all avoid that; Grant is on that already.

As far as I can see, you want that indirection node so that you
safe space in the DTB.  With real OF it is trivial to not have
multiple copies of the data if you want a few properties with
the same data.  There is no reason this could not be done in DTB
as well (and some way in DTS to express that, or maybe the tools
could auto-detect it, whatever).

Or you could just zip the DTB.

>> That is a good question.  Why is it necessary to pass the blob via the
>> tree?
>
> Because sometimes the firmware is needed before networking or serial I/O
> can function.

Can't you link it into the kernel then?  Seems a better place for
it to me.  Of course you said something about GPL, heh.


Segher

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

On Wed, Mar 24, 2010 at 8:49 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:

> I do; I consider that indirection thing (and putting firmware blobs
> in the device tree at all, but to a lesser extent) as making a mess
> of your device binding.

I guess we'll just have to agree to disagree.

> Let's forget that I do not like putting a firmware blob in the
> device tree if you can at all avoid that; Grant is on that already.

The initrd thing is a good idea, but it doesn't help non-Linux
operating systems.  Then again, those OS's might not have any GPL
issues, so it could be a moot point.

> As far as I can see, you want that indirection node so that you
> safe space in the DTB.

No, I want the indirection node so that I can have multiple QE nodes
point to the same binary data in the DTB.  I don't want to have to do
this:

qe@e8000000 {
    fsl,firmware =3D /incbin/("firmware-file-name.bin");
    ...
}

qe@e9000000 {
    fsl,firmware =3D /incbin/("firmware-file-name.bin");
    ...
}

In this case, I have an SOC with two QE devices, so it has two QE
nodes.  Each needs to be initialized by uploading the same microcode
to it.

>=A0With real OF it is trivial to not have
> multiple copies of the data if you want a few properties with
> the same data. =A0There is no reason this could not be done in DTB
> as well (and some way in DTS to express that, or maybe the tools
> could auto-detect it, whatever).

So you're suggesting a change to DTC to support an enhanced syntax?

> Or you could just zip the DTB.

Sorry, I don't understand how that would help me.

> Can't you link it into the kernel then? =A0Seems a better place for
> it to me. =A0Of course you said something about GPL, heh.

Yes, the firmware is not GPL, so I can't link it into the kernel.
Believe me, it would have solved a lot of problems if we could.

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

On Wed, Mar 24, 2010 at 8:49 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:

> I do; I consider that indirection thing (and putting firmware blobs
> in the device tree at all, but to a lesser extent) as making a mess
> of your device binding.

I guess we'll just have to agree to disagree.

> Let's forget that I do not like putting a firmware blob in the
> device tree if you can at all avoid that; Grant is on that already.

The initrd thing is a good idea, but it doesn't help non-Linux
operating systems.  Then again, those OS's might not have any GPL
issues, so it could be a moot point.

> As far as I can see, you want that indirection node so that you
> safe space in the DTB.

No, I want the indirection node so that I can have multiple QE nodes
point to the same binary data in the DTB.  I don't want to have to do
this:

qe@e8000000 {
    fsl,firmware = /incbin/("firmware-file-name.bin");
    ...
}

qe@e9000000 {
    fsl,firmware = /incbin/("firmware-file-name.bin");
    ...
}

In this case, I have an SOC with two QE devices, so it has two QE
nodes.  Each needs to be initialized by uploading the same microcode
to it.

> With real OF it is trivial to not have
> multiple copies of the data if you want a few properties with
> the same data.  There is no reason this could not be done in DTB
> as well (and some way in DTS to express that, or maybe the tools
> could auto-detect it, whatever).

So you're suggesting a change to DTC to support an enhanced syntax?

> Or you could just zip the DTB.

Sorry, I don't understand how that would help me.

> Can't you link it into the kernel then?  Seems a better place for
> it to me.  Of course you said something about GPL, heh.

Yes, the firmware is not GPL, so I can't link it into the kernel.
Believe me, it would have solved a lot of problems if we could.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

[cc'd David Gibson]

On Thu, Mar 25, 2010 at 8:42 AM, Timur Tabi <timur@freescale.com> wrote:
> On Wed, Mar 24, 2010 at 8:49 PM, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
>
>> I do; I consider that indirection thing (and putting firmware blobs
>> in the device tree at all, but to a lesser extent) as making a mess
>> of your device binding.
>
> I guess we'll just have to agree to disagree.
>
>> Let's forget that I do not like putting a firmware blob in the
>> device tree if you can at all avoid that; Grant is on that already.
>
> The initrd thing is a good idea, but it doesn't help non-Linux
> operating systems. =A0Then again, those OS's might not have any GPL
> issues, so it could be a moot point.

The more I think about it, the more I think that the initrd is the
better approach.  Non-GPL firmware blobs are not a new problem, other
drivers have the same issue and the kernel already has a facility for
handling them.  Consistency is worth something here.  As you say, the
ideal solution would be to link the blob into the kernel and be done
with it.  <grumble>

>> As far as I can see, you want that indirection node so that you
>> safe space in the DTB.
>
> No, I want the indirection node so that I can have multiple QE nodes
> point to the same binary data in the DTB. =A0I don't want to have to do
> this:
>
> qe@e8000000 {
> =A0 =A0fsl,firmware =3D /incbin/("firmware-file-name.bin");
> =A0 =A0...
> }
>
> qe@e9000000 {
> =A0 =A0fsl,firmware =3D /incbin/("firmware-file-name.bin");
> =A0 =A0...
> }
>
> In this case, I have an SOC with two QE devices, so it has two QE
> nodes. =A0Each needs to be initialized by uploading the same microcode
> to it.
>
>>=A0With real OF it is trivial to not have
>> multiple copies of the data if you want a few properties with
>> the same data. =A0There is no reason this could not be done in DTB
>> as well (and some way in DTS to express that, or maybe the tools
>> could auto-detect it, whatever).
>
> So you're suggesting a change to DTC to support an enhanced syntax?

It isn't a problem to change dtc if we've got a good use-case for
doing so.  I've cc'd David Gibson.  He's probably got some insight on
the best way to handle this without an incompatible .dtb file format
change.

>
>> Or you could just zip the DTB.
>
> Sorry, I don't understand how that would help me.
>
>> Can't you link it into the kernel then? =A0Seems a better place for
>> it to me. =A0Of course you said something about GPL, heh.
>
> Yes, the firmware is not GPL, so I can't link it into the kernel.
> Believe me, it would have solved a lot of problems if we could.
>
> --
> Timur Tabi
> Linux kernel developer at Freescale
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

[cc'd David Gibson]

On Thu, Mar 25, 2010 at 8:42 AM, Timur Tabi <timur@freescale.com> wrote:
> On Wed, Mar 24, 2010 at 8:49 PM, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
>
>> I do; I consider that indirection thing (and putting firmware blobs
>> in the device tree at all, but to a lesser extent) as making a mess
>> of your device binding.
>
> I guess we'll just have to agree to disagree.
>
>> Let's forget that I do not like putting a firmware blob in the
>> device tree if you can at all avoid that; Grant is on that already.
>
> The initrd thing is a good idea, but it doesn't help non-Linux
> operating systems.  Then again, those OS's might not have any GPL
> issues, so it could be a moot point.

The more I think about it, the more I think that the initrd is the
better approach.  Non-GPL firmware blobs are not a new problem, other
drivers have the same issue and the kernel already has a facility for
handling them.  Consistency is worth something here.  As you say, the
ideal solution would be to link the blob into the kernel and be done
with it.  <grumble>

>> As far as I can see, you want that indirection node so that you
>> safe space in the DTB.
>
> No, I want the indirection node so that I can have multiple QE nodes
> point to the same binary data in the DTB.  I don't want to have to do
> this:
>
> qe@e8000000 {
>    fsl,firmware = /incbin/("firmware-file-name.bin");
>    ...
> }
>
> qe@e9000000 {
>    fsl,firmware = /incbin/("firmware-file-name.bin");
>    ...
> }
>
> In this case, I have an SOC with two QE devices, so it has two QE
> nodes.  Each needs to be initialized by uploading the same microcode
> to it.
>
>> With real OF it is trivial to not have
>> multiple copies of the data if you want a few properties with
>> the same data.  There is no reason this could not be done in DTB
>> as well (and some way in DTS to express that, or maybe the tools
>> could auto-detect it, whatever).
>
> So you're suggesting a change to DTC to support an enhanced syntax?

It isn't a problem to change dtc if we've got a good use-case for
doing so.  I've cc'd David Gibson.  He's probably got some insight on
the best way to handle this without an incompatible .dtb file format
change.

>
>> Or you could just zip the DTB.
>
> Sorry, I don't understand how that would help me.
>
>> Can't you link it into the kernel then?  Seems a better place for
>> it to me.  Of course you said something about GPL, heh.
>
> Yes, the firmware is not GPL, so I can't link it into the kernel.
> Believe me, it would have solved a lot of problems if we could.
>
> --
> Timur Tabi
> Linux kernel developer at Freescale
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

Grant Likely wrote:
> [cc'd David Gibson]
> 
> On Thu, Mar 25, 2010 at 8:42 AM, Timur Tabi <timur@freescale.com> wrote:
>> The initrd thing is a good idea, but it doesn't help non-Linux
>> operating systems.  Then again, those OS's might not have any GPL
>> issues, so it could be a moot point.
> 
> The more I think about it, the more I think that the initrd is the
> better approach.  Non-GPL firmware blobs are not a new problem, other
> drivers have the same issue and the kernel already has a facility for
> handling them.  Consistency is worth something here.  As you say, the
> ideal solution would be to link the blob into the kernel and be done
> with it.  <grumble>

It would be nice to not have to provide separate copies of the firmware 
to u-boot and Linux -- not from a space perspective, but support. 
Instead of having to make sure people have the right firmware in one 
place, we'd have to make sure they have it in both places.

It would also be nice to not require an initrd if one wants an NFS root. 
  That's a lot more complexity than a not-strictly-necessary phandle. :-P

And as Timur pointed out, the device tree is not just for Linux.  I 
don't think the lack of GPL makes it a moot point, as there's still some 
maintenance and support benefit to keeping it in one place -- especially 
since the appropriate firmware version often depends on the specific 
hardware revision that you have.

>>>  With real OF it is trivial to not have
>>> multiple copies of the data if you want a few properties with
>>> the same data.  There is no reason this could not be done in DTB
>>> as well (and some way in DTS to express that, or maybe the tools
>>> could auto-detect it, whatever).
>> So you're suggesting a change to DTC to support an enhanced syntax?
> 
> It isn't a problem to change dtc if we've got a good use-case for
> doing so.  I've cc'd David Gibson.  He's probably got some insight on
> the best way to handle this without an incompatible .dtb file format
> change.

What Segher suggested sounds like it's fundamentally a .dtb file format 
change.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

Grant Likely wrote:
> [cc'd David Gibson]
> 
> On Thu, Mar 25, 2010 at 8:42 AM, Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> The initrd thing is a good idea, but it doesn't help non-Linux
>> operating systems.  Then again, those OS's might not have any GPL
>> issues, so it could be a moot point.
> 
> The more I think about it, the more I think that the initrd is the
> better approach.  Non-GPL firmware blobs are not a new problem, other
> drivers have the same issue and the kernel already has a facility for
> handling them.  Consistency is worth something here.  As you say, the
> ideal solution would be to link the blob into the kernel and be done
> with it.  <grumble>

It would be nice to not have to provide separate copies of the firmware 
to u-boot and Linux -- not from a space perspective, but support. 
Instead of having to make sure people have the right firmware in one 
place, we'd have to make sure they have it in both places.

It would also be nice to not require an initrd if one wants an NFS root. 
  That's a lot more complexity than a not-strictly-necessary phandle. :-P

And as Timur pointed out, the device tree is not just for Linux.  I 
don't think the lack of GPL makes it a moot point, as there's still some 
maintenance and support benefit to keeping it in one place -- especially 
since the appropriate firmware version often depends on the specific 
hardware revision that you have.

>>>  With real OF it is trivial to not have
>>> multiple copies of the data if you want a few properties with
>>> the same data.  There is no reason this could not be done in DTB
>>> as well (and some way in DTS to express that, or maybe the tools
>>> could auto-detect it, whatever).
>> So you're suggesting a change to DTC to support an enhanced syntax?
> 
> It isn't a problem to change dtc if we've got a good use-case for
> doing so.  I've cc'd David Gibson.  He's probably got some insight on
> the best way to handle this without an incompatible .dtb file format
> change.

What Segher suggested sounds like it's fundamentally a .dtb file format 
change.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Scott Wood wrote:

> It would be nice to not have to provide separate copies of the firmware 
> to u-boot and Linux -- not from a space perspective, but support. 

My plan was to take the copy that U-Boot already knows about (via macros like CONFIG_SYS_QE_FW_ADDR) and have U-Boot dynamically embed that in the DTB.  That eliminates the support issue, and it allows us to continue to use the firmware that we already distribute in flash on our boards.

> It would also be nice to not require an initrd if one wants an NFS root. 

The more I think about it, the more I believe that this is how we're going to have to do it.  Whether or not Freescale can embed a non-GPL firmware into a device tree is still undecided.  It may require relicensing all of our device trees as dual bsd/gpl first.  Technically, we should probably do that anyway.

>   That's a lot more complexity than a not-strictly-necessary phandle. :-P

I agree with that.  

> And as Timur pointed out, the device tree is not just for Linux.  I 
> don't think the lack of GPL makes it a moot point, as there's still some 
> maintenance and support benefit to keeping it in one place -- especially 
> since the appropriate firmware version often depends on the specific 
> hardware revision that you have.

Our QE/Fman firmware is typically highly dependent on the specific SOC version.  So if we produce a new revision of an SOC, we typically have to release a new version of the firmware.  I can imagine a situation where our boards ship with every possible firmware in flash, and U-Boot has to find the right one.  In that case, U-Boot will embed the right one in the DTB when it boots the kernel.

This is why I'm opposed to a requirement that the firmware be in the DTS.  If that works for some other people's boards, that's great.  But it won't work for anything that Freescale ships.

>> It isn't a problem to change dtc if we've got a good use-case for
>> doing so.  I've cc'd David Gibson.  He's probably got some insight on
>> the best way to handle this without an incompatible .dtb file format
>> change.
> 
> What Segher suggested sounds like it's fundamentally a .dtb file format 
> change.

That's the impression I got, as well.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Scott Wood wrote:

> It would be nice to not have to provide separate copies of the firmware 
> to u-boot and Linux -- not from a space perspective, but support. 

My plan was to take the copy that U-Boot already knows about (via macros like CONFIG_SYS_QE_FW_ADDR) and have U-Boot dynamically embed that in the DTB.  That eliminates the support issue, and it allows us to continue to use the firmware that we already distribute in flash on our boards.

> It would also be nice to not require an initrd if one wants an NFS root. 

The more I think about it, the more I believe that this is how we're going to have to do it.  Whether or not Freescale can embed a non-GPL firmware into a device tree is still undecided.  It may require relicensing all of our device trees as dual bsd/gpl first.  Technically, we should probably do that anyway.

>   That's a lot more complexity than a not-strictly-necessary phandle. :-P

I agree with that.  

> And as Timur pointed out, the device tree is not just for Linux.  I 
> don't think the lack of GPL makes it a moot point, as there's still some 
> maintenance and support benefit to keeping it in one place -- especially 
> since the appropriate firmware version often depends on the specific 
> hardware revision that you have.

Our QE/Fman firmware is typically highly dependent on the specific SOC version.  So if we produce a new revision of an SOC, we typically have to release a new version of the firmware.  I can imagine a situation where our boards ship with every possible firmware in flash, and U-Boot has to find the right one.  In that case, U-Boot will embed the right one in the DTB when it boots the kernel.

This is why I'm opposed to a requirement that the firmware be in the DTS.  If that works for some other people's boards, that's great.  But it won't work for anything that Freescale ships.

>> It isn't a problem to change dtc if we've got a good use-case for
>> doing so.  I've cc'd David Gibson.  He's probably got some insight on
>> the best way to handle this without an incompatible .dtb file format
>> change.
> 
> What Segher suggested sounds like it's fundamentally a .dtb file format 
> change.

That's the impression I got, as well.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Rafal Jaworowski]

On 2010-03-25, at 17:46, Timur Tabi wrote:

> The more I think about it, the more I believe that this is how we're =
going to have to do it.  Whether or not Freescale can embed a non-GPL =
firmware into a device tree is still undecided.  It may require =
relicensing all of our device trees as dual bsd/gpl first.  Technically, =
we should probably do that anyway.

The dual-licensing process has already started, see the following :-)

http://p4db.freebsd.org/changeView.cgi?CH=3D173836
http://p4db.freebsd.org/changeView.cgi?CH=3D174365

Rafal

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Rafal Jaworowski]

On 2010-03-25, at 17:46, Timur Tabi wrote:

> The more I think about it, the more I believe that this is how we're going to have to do it.  Whether or not Freescale can embed a non-GPL firmware into a device tree is still undecided.  It may require relicensing all of our device trees as dual bsd/gpl first.  Technically, we should probably do that anyway.

The dual-licensing process has already started, see the following :-)

http://p4db.freebsd.org/changeView.cgi?CH=173836
http://p4db.freebsd.org/changeView.cgi?CH=174365

Rafal

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[M. Warner Losh]

In message: <ed82fe3e1003250742t5affee7dudca651ab3352bc16@mail.gmail.com>
            Timur Tabi <timur@freescale.com> writes:
: The initrd thing is a good idea, but it doesn't help non-Linux
: operating systems.  Then again, those OS's might not have any GPL
: issues, so it could be a moot point.

Most !linux systems are not GPL'd.  They are BSDL, primarily, or some
private license between seller and buyer.  In any event, other OSes
likely won't have the GPL issue.

But I'm confused.  If you can't distribute the binary firmware, why
can you distribute the hex dump of the firmware?

Warner

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[M. Warner Losh]

In message: <ed82fe3e1003250742t5affee7dudca651ab3352bc16-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
            Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org> writes:
: The initrd thing is a good idea, but it doesn't help non-Linux
: operating systems.  Then again, those OS's might not have any GPL
: issues, so it could be a moot point.

Most !linux systems are not GPL'd.  They are BSDL, primarily, or some
private license between seller and buyer.  In any event, other OSes
likely won't have the GPL issue.

But I'm confused.  If you can't distribute the binary firmware, why
can you distribute the hex dump of the firmware?

Warner

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

On Thu, Mar 25, 2010 at 6:53 PM, M. Warner Losh <imp@bsdimp.com> wrote:

> Most !linux systems are not GPL'd. =A0They are BSDL, primarily, or some
> private license between seller and buyer. =A0In any event, other OSes
> likely won't have the GPL issue.

You're probably right.

> But I'm confused. =A0If you can't distribute the binary firmware, why
> can you distribute the hex dump of the firmware?

We can distribute the binary firmware.  We just can't embed it in the
Linux kernel, since it's not GPL.  So it needs to be delivered to the
kernel at boot time via a non-GPL mechanism.  The device tree is one
option.  An initrd is another.  The problem Freescale has is that
modifying our BSP to use an initrd is not a trivial task, and it's a
lot more complicated than using the device tree.  For example, there's
the possibility that U-Boot and Linux will use different microcode
binaries, and it also means that the initrd becomes SOC-specific.

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

On Thu, Mar 25, 2010 at 6:53 PM, M. Warner Losh <imp-uzTCJ5RojNnQT0dZR+AlfA@public.gmane.org> wrote:

> Most !linux systems are not GPL'd.  They are BSDL, primarily, or some
> private license between seller and buyer.  In any event, other OSes
> likely won't have the GPL issue.

You're probably right.

> But I'm confused.  If you can't distribute the binary firmware, why
> can you distribute the hex dump of the firmware?

We can distribute the binary firmware.  We just can't embed it in the
Linux kernel, since it's not GPL.  So it needs to be delivered to the
kernel at boot time via a non-GPL mechanism.  The device tree is one
option.  An initrd is another.  The problem Freescale has is that
modifying our BSP to use an initrd is not a trivial task, and it's a
lot more complicated than using the device tree.  For example, there's
the possibility that U-Boot and Linux will use different microcode
binaries, and it also means that the initrd becomes SOC-specific.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

Segher Boessenkool wrote:
> As far as I can see, you want that indirection node so that you
> safe space in the DTB.

Probably more of a general desire to not duplicate things that don't 
need to be duplicated...  I don't think the space issue is critical in 
this particular case.

> With real OF it is trivial to not have
> multiple copies of the data if you want a few properties with
> the same data.  There is no reason this could not be done in DTB
> as well (and some way in DTS to express that, or maybe the tools
> could auto-detect it, whatever).

You object to some slight complexity in the device binding, and want to 
replace it with an incompatible, more complex change to the blob format?

>>> That is a good question.  Why is it necessary to pass the blob via the
>>> tree?
>> Because sometimes the firmware is needed before networking or serial I/O
>> can function.
> 
> Can't you link it into the kernel then?  Seems a better place for
> it to me.  Of course you said something about GPL, heh.

U-Boot needs it too.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

Segher Boessenkool wrote:
> As far as I can see, you want that indirection node so that you
> safe space in the DTB.

Probably more of a general desire to not duplicate things that don't 
need to be duplicated...  I don't think the space issue is critical in 
this particular case.

> With real OF it is trivial to not have
> multiple copies of the data if you want a few properties with
> the same data.  There is no reason this could not be done in DTB
> as well (and some way in DTS to express that, or maybe the tools
> could auto-detect it, whatever).

You object to some slight complexity in the device binding, and want to 
replace it with an incompatible, more complex change to the blob format?

>>> That is a good question.  Why is it necessary to pass the blob via the
>>> tree?
>> Because sometimes the firmware is needed before networking or serial I/O
>> can function.
> 
> Can't you link it into the kernel then?  Seems a better place for
> it to me.  Of course you said something about GPL, heh.

U-Boot needs it too.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Mitch Bradley]

It seems to me that there are plausible use cases for both 
direct-inclusion and indirection.  I don't see any real problems with 
either, so I would vote for specifying both alternatives.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Mitch Bradley]

It seems to me that there are plausible use cases for both 
direct-inclusion and indirection.  I don't see any real problems with 
either, so I would vote for specifying both alternatives.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb@firmworks.com> wrote:
> It seems to me that there are plausible use cases for both direct-inclusi=
on
> and indirection. =A0I don't see any real problems with either, so I would=
 vote
> for specifying both alternatives.

Ugh.  Then this one driver would need to implement both binding for
little, if any, actual benefit.  I'm sure we can come to an agreement
on one method if the firmware absolutely has to be in the tree.

Personally, my vote lies with direct-inclusion.  However, if
indirection is used, then I think it would be wise to define where
data-only nodes like this should live.  Under /chosen perhaps?  It
wouldn't be good to place it somewhere where it will be confused for
an actual device node.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> wrote:
> It seems to me that there are plausible use cases for both direct-inclusion
> and indirection.  I don't see any real problems with either, so I would vote
> for specifying both alternatives.

Ugh.  Then this one driver would need to implement both binding for
little, if any, actual benefit.  I'm sure we can come to an agreement
on one method if the firmware absolutely has to be in the tree.

Personally, my vote lies with direct-inclusion.  However, if
indirection is used, then I think it would be wise to define where
data-only nodes like this should live.  Under /chosen perhaps?  It
wouldn't be good to place it somewhere where it will be confused for
an actual device node.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:
> On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb@firmworks.com> wrote:
>> It seems to me that there are plausible use cases for both direct-inclusion
>> and indirection.  I don't see any real problems with either, so I would vote
>> for specifying both alternatives.
> 
> Ugh.  Then this one driver would need to implement both binding for
> little, if any, actual benefit. 

Although I agree that I don't like supporting both bindings, we could encapsulate the locating of the firmware node in a function.  The function will first look for a child firmware node, and if it doesn't find it, look for a fsl,firmware property.  It will return a pointer to the firmware node regardless.

> I'm sure we can come to an agreement
> on one method if the firmware absolutely has to be in the tree.

If we have to pick one, then I think the only viable choice is have a separate firmware node and a phandle pointer to it.  Otherwise, I just don't see how we can handle multiple devices needing the same firmware.

> Personally, my vote lies with direct-inclusion.  However, if
> indirection is used, then I think it would be wise to define where
> data-only nodes like this should live.  Under /chosen perhaps?  

I personally don't care that much; /chosen is okay with me, but ....

> It
> wouldn't be good to place it somewhere where it will be confused for
> an actual device node.

... what's wrong with the root node?  

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:
> On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> wrote:
>> It seems to me that there are plausible use cases for both direct-inclusion
>> and indirection.  I don't see any real problems with either, so I would vote
>> for specifying both alternatives.
> 
> Ugh.  Then this one driver would need to implement both binding for
> little, if any, actual benefit. 

Although I agree that I don't like supporting both bindings, we could encapsulate the locating of the firmware node in a function.  The function will first look for a child firmware node, and if it doesn't find it, look for a fsl,firmware property.  It will return a pointer to the firmware node regardless.

> I'm sure we can come to an agreement
> on one method if the firmware absolutely has to be in the tree.

If we have to pick one, then I think the only viable choice is have a separate firmware node and a phandle pointer to it.  Otherwise, I just don't see how we can handle multiple devices needing the same firmware.

> Personally, my vote lies with direct-inclusion.  However, if
> indirection is used, then I think it would be wise to define where
> data-only nodes like this should live.  Under /chosen perhaps?  

I personally don't care that much; /chosen is okay with me, but ....

> It
> wouldn't be good to place it somewhere where it will be confused for
> an actual device node.

... what's wrong with the root node?  

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

Timur Tabi wrote:
> Grant Likely wrote:
>> On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb@firmworks.com> wrote:
>>> It seems to me that there are plausible use cases for both direct-inclusion
>>> and indirection.  I don't see any real problems with either, so I would vote
>>> for specifying both alternatives.
>> Ugh.  Then this one driver would need to implement both binding for
>> little, if any, actual benefit. 
> 
> Although I agree that I don't like supporting both bindings, we could
> encapsulate the locating of the firmware node in a function.  The
> function will first look for a child firmware node, and if it doesn't
> find it, look for a fsl,firmware property.  It will return a pointer
> to the firmware node regardless.
> 
>> I'm sure we can come to an agreement
>> on one method if the firmware absolutely has to be in the tree.
> 
> If we have to pick one, then I think the only viable choice is have a
> separate firmware node and a phandle pointer to it.  Otherwise, I
> just don't see how we can handle multiple devices needing the same
> firmware.

You would duplicate the firmware.  I vote for supporting both -- a few 
lines in the binding code is not that big of a deal, and it would 
provide more flexibility for the tree to describe the structure of 
things -- but either way is usable.

>> Personally, my vote lies with direct-inclusion.  However, if
>> indirection is used, then I think it would be wise to define where
>> data-only nodes like this should live.  Under /chosen perhaps?  
> 
> I personally don't care that much; /chosen is okay with me, but ....
> 
>> It
>> wouldn't be good to place it somewhere where it will be confused for
>> an actual device node.
> 
> ... what's wrong with the root node?  

Nothing, IMHO.  It shouldn't get confused for anything in the absence of 
some code specifically looking for that name or compatible -- any more 
than /chosen itself is mistaken for a device.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

Timur Tabi wrote:
> Grant Likely wrote:
>> On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> wrote:
>>> It seems to me that there are plausible use cases for both direct-inclusion
>>> and indirection.  I don't see any real problems with either, so I would vote
>>> for specifying both alternatives.
>> Ugh.  Then this one driver would need to implement both binding for
>> little, if any, actual benefit. 
> 
> Although I agree that I don't like supporting both bindings, we could
> encapsulate the locating of the firmware node in a function.  The
> function will first look for a child firmware node, and if it doesn't
> find it, look for a fsl,firmware property.  It will return a pointer
> to the firmware node regardless.
> 
>> I'm sure we can come to an agreement
>> on one method if the firmware absolutely has to be in the tree.
> 
> If we have to pick one, then I think the only viable choice is have a
> separate firmware node and a phandle pointer to it.  Otherwise, I
> just don't see how we can handle multiple devices needing the same
> firmware.

You would duplicate the firmware.  I vote for supporting both -- a few 
lines in the binding code is not that big of a deal, and it would 
provide more flexibility for the tree to describe the structure of 
things -- but either way is usable.

>> Personally, my vote lies with direct-inclusion.  However, if
>> indirection is used, then I think it would be wise to define where
>> data-only nodes like this should live.  Under /chosen perhaps?  
> 
> I personally don't care that much; /chosen is okay with me, but ....
> 
>> It
>> wouldn't be good to place it somewhere where it will be confused for
>> an actual device node.
> 
> ... what's wrong with the root node?  

Nothing, IMHO.  It shouldn't get confused for anything in the absence of 
some code specifically looking for that name or compatible -- any more 
than /chosen itself is mistaken for a device.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 10:36 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>> On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb@firmworks.com> wrote=
:
>>> It seems to me that there are plausible use cases for both direct-inclu=
sion
>>> and indirection. =A0I don't see any real problems with either, so I wou=
ld vote
>>> for specifying both alternatives.
>>
>> Ugh. =A0Then this one driver would need to implement both binding for
>> little, if any, actual benefit.
>
> Although I agree that I don't like supporting both bindings, we could enc=
apsulate the locating of the firmware node in a function. =A0The function w=
ill first look for a child firmware node, and if it doesn't find it, look f=
or a fsl,firmware property. =A0It will return a pointer to the firmware nod=
e regardless.
>
>> I'm sure we can come to an agreement
>> on one method if the firmware absolutely has to be in the tree.
>
> If we have to pick one, then I think the only viable choice is have a sep=
arate firmware node and a phandle pointer to it. =A0Otherwise, I just don't=
 see how we can handle multiple devices needing the same firmware.

Wait for David to weigh in on this one before making a decision.  He
knows the dtb format best.

>> Personally, my vote lies with direct-inclusion. =A0However, if
>> indirection is used, then I think it would be wise to define where
>> data-only nodes like this should live. =A0Under /chosen perhaps?
>
> I personally don't care that much; /chosen is okay with me, but ....
>
>> It
>> wouldn't be good to place it somewhere where it will be confused for
>> an actual device node.
>
> ... what's wrong with the root node?

Take things to a logical extreme and see what it looks like.  Instead
of one QE blob, assume that you need to provide 100 firmware blobs for
various devices (yes, that number is ridiculous, but that's kind of
the point).  Essentially what I mean is make the assumption that the
problem you have is not unique, that other drivers also need firmware.
 Should it be organized?  Does it make sense to have one node in the
root for each firmware blob?  Does it make sense for each of those
nodes to have a 'compatible' property which makes it look like a
physical device?

Here's a counter proposal off the top of my head:

For indirect firmware, create a /chosen/firmware node.  Don't add a
compatible property, compatible is for devices and this node is for
blob data.  Put each firmware blob into a separate property, and make
the names reasonable (ie. mpc<blah>-qe-firmware).  Have the QE
reference the firmware blob by property name.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 10:36 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>> On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb@firmworks.com> wrote:
>>> It seems to me that there are plausible use cases for both direct-inclusion
>>> and indirection.  I don't see any real problems with either, so I would vote
>>> for specifying both alternatives.
>>
>> Ugh.  Then this one driver would need to implement both binding for
>> little, if any, actual benefit.
>
> Although I agree that I don't like supporting both bindings, we could encapsulate the locating of the firmware node in a function.  The function will first look for a child firmware node, and if it doesn't find it, look for a fsl,firmware property.  It will return a pointer to the firmware node regardless.
>
>> I'm sure we can come to an agreement
>> on one method if the firmware absolutely has to be in the tree.
>
> If we have to pick one, then I think the only viable choice is have a separate firmware node and a phandle pointer to it.  Otherwise, I just don't see how we can handle multiple devices needing the same firmware.

Wait for David to weigh in on this one before making a decision.  He
knows the dtb format best.

>> Personally, my vote lies with direct-inclusion.  However, if
>> indirection is used, then I think it would be wise to define where
>> data-only nodes like this should live.  Under /chosen perhaps?
>
> I personally don't care that much; /chosen is okay with me, but ....
>
>> It
>> wouldn't be good to place it somewhere where it will be confused for
>> an actual device node.
>
> ... what's wrong with the root node?

Take things to a logical extreme and see what it looks like.  Instead
of one QE blob, assume that you need to provide 100 firmware blobs for
various devices (yes, that number is ridiculous, but that's kind of
the point).  Essentially what I mean is make the assumption that the
problem you have is not unique, that other drivers also need firmware.
 Should it be organized?  Does it make sense to have one node in the
root for each firmware blob?  Does it make sense for each of those
nodes to have a 'compatible' property which makes it look like a
physical device?

Here's a counter proposal off the top of my head:

For indirect firmware, create a /chosen/firmware node.  Don't add a
compatible property, compatible is for devices and this node is for
blob data.  Put each firmware blob into a separate property, and make
the names reasonable (ie. mpc<blah>-qe-firmware).  Have the QE
reference the firmware blob by property name.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:
> For indirect firmware, create a /chosen/firmware node.  Don't add a
> compatible property, 

Oh, I don't like that idea at all.  The compatible property is useful for me to know *how* to parse the binary blob.  

> compatible is for devices and this node is for
> blob data.

But the blob has a format.  For QE firmware, for example, it's this:

struct qe_firmware {
	struct qe_header {
		u32 length;	/* Length of the entire structure, in bytes */
		u8 magic[3];	/* Set to { 'Q', 'E', 'F' } */
		u8 version;	/* Version of this layout. First ver is '1' */
	} header;
	u8 id[62];		/* Null-terminated identifier string */
	u8 split;		/* 0 = shared I-RAM, 1 = split I-RAM */
	u8 count;		/* Number of microcode[] structures */
	struct {
		u16 model;	/* The SOC model  */
		u8 major;	/* The SOC revision major */
		u8 minor;	/* The SOC revision minor */
	} __attribute__ ((packed)) soc;
	u8 padding[4];		/* Reserved, for alignment */
	u64 extended_modes;	/* Extended modes */
	u32 vtraps[8];		/* Virtual trap addresses */
	u8 reserved[4];		/* Reserved, for future expansion */
	struct qe_microcode {
		u8 id[32];	/* Null-terminated identifier */
		u32 traps[16];	/* Trap addresses, 0 == ignore */
		u32 eccr;	/* The value for the ECCR register */
		u32 iram_offset;/* Offset into I-RAM for the code */
		u32 count;	/* Number of 32-bit words of the code */
		u32 code_offset;/* Offset of the actual microcode */
		u8 major;	/* The microcode version major */
		u8 minor;	/* The microcode version minor */
		u8 revision;	/* The microcode version revision */
		u8 padding;	/* Reserved, for alignment */
		u8 reserved[4];	/* Reserved, for future expansion */
	} __attribute__ ((packed)) microcode[1];
	/* All microcode binaries should be located here */
	/* CRC32 should be located here, after the microcode binaries */
} __attribute__ ((packed));

>  Put each firmware blob into a separate property, and make
> the names reasonable (ie. mpc<blah>-qe-firmware).  Have the QE
> reference the firmware blob by property name.

I don't like the idea of using the property name as a pseudo-compatible string.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 11:03 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>> For indirect firmware, create a /chosen/firmware node. =A0Don't add a
>> compatible property,
>
> Oh, I don't like that idea at all. =A0The compatible property is useful f=
or me to know *how* to parse the binary blob.

Compatible is for devices.  This is not a device.  Drivers cannot bind
against it.  Use a different mechanism if you have metadata about the
blob.  If your driver doesn't know how to validate its own firmware
blobs, then you've got bigger problems.

>> compatible is for devices and this node is for
>> blob data.
>
> But the blob has a format. =A0For QE firmware, for example, it's this:
>
> struct qe_firmware {
[...]
> } __attribute__ ((packed));

So?  If your data has a structure that the device tree cares about,
the break it out into the device tree nodes/properties structure.  If
it is a blob, then only the driver cares.  The format of the blob
doesn't have any bearing on how it is encapsulated into the tree.

>> =A0Put each firmware blob into a separate property, and make
>> the names reasonable (ie. mpc<blah>-qe-firmware). =A0Have the QE
>> reference the firmware blob by property name.
>
> I don't like the idea of using the property name as a pseudo-compatible s=
tring.

It's a name, not a pseudo compatible string, and your device node will
explicitly reference it by name.  There is not backwards compatibility
or fuzzy binding issues at play here.  It is a way for your driver
node to state, "I want *that exact* firmware blob".  You could make
the property name "george" and it would still be completely clear (if
a little weird) because all the references are contained within the
tree.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 11:03 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>> For indirect firmware, create a /chosen/firmware node.  Don't add a
>> compatible property,
>
> Oh, I don't like that idea at all.  The compatible property is useful for me to know *how* to parse the binary blob.

Compatible is for devices.  This is not a device.  Drivers cannot bind
against it.  Use a different mechanism if you have metadata about the
blob.  If your driver doesn't know how to validate its own firmware
blobs, then you've got bigger problems.

>> compatible is for devices and this node is for
>> blob data.
>
> But the blob has a format.  For QE firmware, for example, it's this:
>
> struct qe_firmware {
[...]
> } __attribute__ ((packed));

So?  If your data has a structure that the device tree cares about,
the break it out into the device tree nodes/properties structure.  If
it is a blob, then only the driver cares.  The format of the blob
doesn't have any bearing on how it is encapsulated into the tree.

>>  Put each firmware blob into a separate property, and make
>> the names reasonable (ie. mpc<blah>-qe-firmware).  Have the QE
>> reference the firmware blob by property name.
>
> I don't like the idea of using the property name as a pseudo-compatible string.

It's a name, not a pseudo compatible string, and your device node will
explicitly reference it by name.  There is not backwards compatibility
or fuzzy binding issues at play here.  It is a way for your driver
node to state, "I want *that exact* firmware blob".  You could make
the property name "george" and it would still be completely clear (if
a little weird) because all the references are contained within the
tree.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:

> Compatible is for devices.  This is not a device.  Drivers cannot bind
> against it.  Use a different mechanism if you have metadata about the
> blob.  If your driver doesn't know how to validate its own firmware
> blobs, then you've got bigger problems.

Perhaps.  I left the door open to have additional firmware-specific properties in the firmware node that the driver can use to interpret the firmware blob.  Not everyone can have a nice, complete header for the firmware.  Sometimes the firmware is distributed as just raw microcode words, without any header or anything like that.

> So?  If your data has a structure that the device tree cares about,
> the break it out into the device tree nodes/properties structure.  If
> it is a blob, then only the driver cares.  The format of the blob
> doesn't have any bearing on how it is encapsulated into the tree.

Yes, the QE firmware format does include a bunch of information, but who says that will always be the case?  And what happens if we want to extend this binding to include non-QE firmware?  What happens if, down the road, we need to add properties to help us interpret the firmware?

In other words, we can't depend on the contains of the firmware blob to tell the driver everything he needs to know.

> It's a name, not a pseudo compatible string, and your device node will
> explicitly reference it by name.  There is not backwards compatibility
> or fuzzy binding issues at play here.  It is a way for your driver
> node to state, "I want *that exact* firmware blob".  You could make
> the property name "george" and it would still be completely clear (if
> a little weird) because all the references are contained within the
> tree.

Eh, I don't know.  Maybe if you bought me dinner first, you could talk me into it.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

Grant Likely wrote:
> On Thu, Mar 25, 2010 at 11:03 AM, Timur Tabi <timur@freescale.com> wrote:
>> Grant Likely wrote:
>>> For indirect firmware, create a /chosen/firmware node.  Don't add a
>>> compatible property,
>> Oh, I don't like that idea at all.  The compatible property is useful for me to know *how* to parse the binary blob.
> 
> Compatible is for devices.

Compatible is for matching.  Who cares what category the thing being 
matched is in?  What is the definition of a device, and why does it matter?

> This is not a device.  Drivers cannot bind
> against it.  Use a different mechanism if you have metadata about the
> blob.  If your driver doesn't know how to validate its own firmware
> blobs, then you've got bigger problems.

One could also say, if your hardware can't be probed at runtime, you've 
got bigger problems. :-)

What's wrong with an indication of what type of "thing" a node is 
supposed to be?  There could be multiple microcode formats, for example.

I don't know that it's strictly necessary in this case --  it looks like 
there is a magic number in the firmware blob -- but I don't understand 
the objection as a matter of principle.  These device tree discussions 
have a tendency to get awfully bikesheddy.

>>>  Put each firmware blob into a separate property, and make
>>> the names reasonable (ie. mpc<blah>-qe-firmware).  Have the QE
>>> reference the firmware blob by property name.
>> I don't like the idea of using the property name as a pseudo-compatible string.
> 
> It's a name, not a pseudo compatible string, and your device node will
> explicitly reference it by name.  There is not backwards compatibility
> or fuzzy binding issues at play here.

There is a forward compatibility issue, in that we'll have to update the 
code with every new mpc<blah> (or p<blah>rev<n>) that comes along.

Or are we supposed to pick some random chip to request the firmware for, 
like with compatibles?  What would be the point?  This isn't being used 
to bind a driver.

> It is a way for your driver
> node to state, "I want *that exact* firmware blob".

The driver wants the firmware blob that the device tree provides.  The 
device tree knows better than the driver, being that the device tree is 
the describer of the hardware.

> You could make
> the property name "george" 

If "george" is fine, then so is "fsl,firmware".  Maybe "fsl,qe-firmware".

> and it would still be completely clear (if
> a little weird) because all the references are contained within the
> tree.

How are the references contained within the tree?  The driver has to 
know which property to read.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

Grant Likely wrote:
> On Thu, Mar 25, 2010 at 11:03 AM, Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> Grant Likely wrote:
>>> For indirect firmware, create a /chosen/firmware node.  Don't add a
>>> compatible property,
>> Oh, I don't like that idea at all.  The compatible property is useful for me to know *how* to parse the binary blob.
> 
> Compatible is for devices.

Compatible is for matching.  Who cares what category the thing being 
matched is in?  What is the definition of a device, and why does it matter?

> This is not a device.  Drivers cannot bind
> against it.  Use a different mechanism if you have metadata about the
> blob.  If your driver doesn't know how to validate its own firmware
> blobs, then you've got bigger problems.

One could also say, if your hardware can't be probed at runtime, you've 
got bigger problems. :-)

What's wrong with an indication of what type of "thing" a node is 
supposed to be?  There could be multiple microcode formats, for example.

I don't know that it's strictly necessary in this case --  it looks like 
there is a magic number in the firmware blob -- but I don't understand 
the objection as a matter of principle.  These device tree discussions 
have a tendency to get awfully bikesheddy.

>>>  Put each firmware blob into a separate property, and make
>>> the names reasonable (ie. mpc<blah>-qe-firmware).  Have the QE
>>> reference the firmware blob by property name.
>> I don't like the idea of using the property name as a pseudo-compatible string.
> 
> It's a name, not a pseudo compatible string, and your device node will
> explicitly reference it by name.  There is not backwards compatibility
> or fuzzy binding issues at play here.

There is a forward compatibility issue, in that we'll have to update the 
code with every new mpc<blah> (or p<blah>rev<n>) that comes along.

Or are we supposed to pick some random chip to request the firmware for, 
like with compatibles?  What would be the point?  This isn't being used 
to bind a driver.

> It is a way for your driver
> node to state, "I want *that exact* firmware blob".

The driver wants the firmware blob that the device tree provides.  The 
device tree knows better than the driver, being that the device tree is 
the describer of the hardware.

> You could make
> the property name "george" 

If "george" is fine, then so is "fsl,firmware".  Maybe "fsl,qe-firmware".

> and it would still be completely clear (if
> a little weird) because all the references are contained within the
> tree.

How are the references contained within the tree?  The driver has to 
know which property to read.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Scott Wood wrote:

> I don't know that it's strictly necessary in this case --  it looks like 
> there is a magic number in the firmware blob -- but I don't understand 
> the objection as a matter of principle.  These device tree discussions 
> have a tendency to get awfully bikesheddy.

I don't want this discussion, and any binding definitions that come from it, to be limited to the specifics of a QE firmware.  I want to make sure that any binding that we come up with can be easily extended to any kind of firmware, including firmware that has no headers.

Many companies will distribute their firmware as a generic sequence of bytes (no header), and a simple code fragment that demonstrates uploading the binary to the hardware.  The license for these files sometimes precludes the option of wrapping that microcode inside or with another binary.  That is, if you want to distribute this firmware, it must be distributed as-is.  

Example:

Freescale buys a chip from some vendor and puts the chip on a reference board.  The chip requires some microcode to be uploaded, and the vendor distributes some binary blob and a code snippet.  Freescale embeds the code snippet in Linux, and puts the microcode somewhere in flash.  The license for the microcode says, "thou shalt not merge this microcode with any other binary".  Freescale decides, for whatever reason, that putting a header around the microcode and putting *that* in flash is a violation of the license, but having U-Boot dynamically embed it into a DTB isn't.  Ok, maybe that's a bit ridiculous, but just humor me.  In this case, it would useful to have the microcode in its own node with properties describing the microcode.

Anyway, I may have gone off in the weeds with this discussion.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 2:04 PM, Timur Tabi <timur@freescale.com> wrote:
> Scott Wood wrote:
>
>> I don't know that it's strictly necessary in this case -- =A0it looks li=
ke
>> there is a magic number in the firmware blob -- but I don't understand
>> the objection as a matter of principle. =A0These device tree discussions
>> have a tendency to get awfully bikesheddy.
>
> I don't want this discussion, and any binding definitions that come from =
it, to be limited to the specifics of a QE firmware. =A0I want to make sure=
 that any binding that we come up with can be easily extended to any kind o=
f firmware, including firmware that has no headers.
>
> Many companies will distribute their firmware as a generic sequence of by=
tes (no header), and a simple code fragment that demonstrates uploading the=
 binary to the hardware. =A0The license for these files sometimes precludes=
 the option of wrapping that microcode inside or with another binary. =A0Th=
at is, if you want to distribute this firmware, it must be distributed as-i=
s.
>
> Example:
>
> Freescale buys a chip from some vendor and puts the chip on a reference b=
oard. =A0The chip requires some microcode to be uploaded, and the vendor di=
stributes some binary blob and a code snippet. =A0Freescale embeds the code=
 snippet in Linux, and puts the microcode somewhere in flash. =A0The licens=
e for the microcode says, "thou shalt not merge this microcode with any oth=
er binary". =A0Freescale decides, for whatever reason, that putting a heade=
r around the microcode and putting *that* in flash is a violation of the li=
cense, but having U-Boot dynamically embed it into a DTB isn't. =A0Ok, mayb=
e that's a bit ridiculous, but just humor me. =A0In this case, it would use=
ful to have the microcode in its own node with properties describing the mi=
crocode.
>
> Anyway, I may have gone off in the weeds with this discussion.

No, this isn't off in the weeds.  I concede the point of needing to
store firmware in a separate node, but I still don't agree with the
argument that it needs to be anything more than an anonymous named
blob.  The fact that another node references it implicitly defines the
format the blob needs to be in.  If the blob format changes, then in
practice the existing binding is no longer compatible because older
drivers just won't work.  It probably doesn't justify creation of a
new compatible value, but I would handle firmware in a different
format with a different property name to reference it.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 2:04 PM, Timur Tabi <timur@freescale.com> wrote:
> Scott Wood wrote:
>
>> I don't know that it's strictly necessary in this case --  it looks like
>> there is a magic number in the firmware blob -- but I don't understand
>> the objection as a matter of principle.  These device tree discussions
>> have a tendency to get awfully bikesheddy.
>
> I don't want this discussion, and any binding definitions that come from it, to be limited to the specifics of a QE firmware.  I want to make sure that any binding that we come up with can be easily extended to any kind of firmware, including firmware that has no headers.
>
> Many companies will distribute their firmware as a generic sequence of bytes (no header), and a simple code fragment that demonstrates uploading the binary to the hardware.  The license for these files sometimes precludes the option of wrapping that microcode inside or with another binary.  That is, if you want to distribute this firmware, it must be distributed as-is.
>
> Example:
>
> Freescale buys a chip from some vendor and puts the chip on a reference board.  The chip requires some microcode to be uploaded, and the vendor distributes some binary blob and a code snippet.  Freescale embeds the code snippet in Linux, and puts the microcode somewhere in flash.  The license for the microcode says, "thou shalt not merge this microcode with any other binary".  Freescale decides, for whatever reason, that putting a header around the microcode and putting *that* in flash is a violation of the license, but having U-Boot dynamically embed it into a DTB isn't.  Ok, maybe that's a bit ridiculous, but just humor me.  In this case, it would useful to have the microcode in its own node with properties describing the microcode.
>
> Anyway, I may have gone off in the weeds with this discussion.

No, this isn't off in the weeds.  I concede the point of needing to
store firmware in a separate node, but I still don't agree with the
argument that it needs to be anything more than an anonymous named
blob.  The fact that another node references it implicitly defines the
format the blob needs to be in.  If the blob format changes, then in
practice the existing binding is no longer compatible because older
drivers just won't work.  It probably doesn't justify creation of a
new compatible value, but I would handle firmware in a different
format with a different property name to reference it.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:

> No, this isn't off in the weeds.  I concede the point of needing to
> store firmware in a separate node, but I still don't agree with the
> argument that it needs to be anything more than an anonymous named
> blob.

I still don't understand what's so *bad* about having some kind of binding documentation and a compatible property for this node.  Yes, I understand that compatible==device argument, but do we really have to be so strict about it?  I would like to be able to use the built-in OF functions in the kernel to validate the node (e.g. of_device_is_compatible).

And yes, I just realized the irony of using a function called of_device_is_compatible to test a node for something that isn't a device.  I'd prefer if you just ... overlooked that.

>  The fact that another node references it implicitly defines the
> format the blob needs to be in.  If the blob format changes, then in
> practice the existing binding is no longer compatible because older
> drivers just won't work.

Not necessarily.  There could be a new format for the microcode, or some new instructions for how to upload the microcode, and the driver could use the compatible format to learn how to process the microcode.  This would isolate all of the microcode-isms to the blob node.

>  It probably doesn't justify creation of a
> new compatible value, but I would handle firmware in a different
> format with a different property name to reference it.

I think it would be nicer if the property name could stay the same.  Then I wouldn't need to modify the property if I'm trying to upload a different kind of firmware.  I could then do this in the DTS:

qe@e0080000 {
	compatible = "fsl,qe";
	fsl,firmware-phandle = <&qe_firmware>;
	...
}	

qe_firmware:qe-firmware {
}

See how the qe-firmware node is blank?  It's just a placeholder for the firmware.  U-Boot would then just need to update that node when it embeds the microcode.  It would not need to modify the qe@e0080000 node.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:

> No, this isn't off in the weeds.  I concede the point of needing to
> store firmware in a separate node, but I still don't agree with the
> argument that it needs to be anything more than an anonymous named
> blob.

I still don't understand what's so *bad* about having some kind of binding documentation and a compatible property for this node.  Yes, I understand that compatible==device argument, but do we really have to be so strict about it?  I would like to be able to use the built-in OF functions in the kernel to validate the node (e.g. of_device_is_compatible).

And yes, I just realized the irony of using a function called of_device_is_compatible to test a node for something that isn't a device.  I'd prefer if you just ... overlooked that.

>  The fact that another node references it implicitly defines the
> format the blob needs to be in.  If the blob format changes, then in
> practice the existing binding is no longer compatible because older
> drivers just won't work.

Not necessarily.  There could be a new format for the microcode, or some new instructions for how to upload the microcode, and the driver could use the compatible format to learn how to process the microcode.  This would isolate all of the microcode-isms to the blob node.

>  It probably doesn't justify creation of a
> new compatible value, but I would handle firmware in a different
> format with a different property name to reference it.

I think it would be nicer if the property name could stay the same.  Then I wouldn't need to modify the property if I'm trying to upload a different kind of firmware.  I could then do this in the DTS:

qe@e0080000 {
	compatible = "fsl,qe";
	fsl,firmware-phandle = <&qe_firmware>;
	...
}	

qe_firmware:qe-firmware {
}

See how the qe-firmware node is blank?  It's just a placeholder for the firmware.  U-Boot would then just need to update that node when it embeds the microcode.  It would not need to modify the qe@e0080000 node.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 1:53 PM, Scott Wood <scottwood@freescale.com> wrote=
:
> Grant Likely wrote:
>>
>> On Thu, Mar 25, 2010 at 11:03 AM, Timur Tabi <timur@freescale.com> wrote=
:
>>>
>>> Grant Likely wrote:
>>>>
>>>> For indirect firmware, create a /chosen/firmware node. =A0Don't add a
>>>> compatible property,
>>>
>>> Oh, I don't like that idea at all. =A0The compatible property is useful=
 for
>>> me to know *how* to parse the binary blob.
>>
>> Compatible is for devices.
>
> Compatible is for matching. =A0Who cares what category the thing being ma=
tched
> is in? =A0What is the definition of a device, and why does it matter?

Compatible specifies a device and a list of devices it can emulate:

http://www.openfirmware.org/1275/proposals/Closed/Accepted/343-it.txt

>> This is not a device. =A0Drivers cannot bind
>> against it. =A0Use a different mechanism if you have metadata about the
>> blob. =A0If your driver doesn't know how to validate its own firmware
>> blobs, then you've got bigger problems.
>
> One could also say, if your hardware can't be probed at runtime, you've g=
ot
> bigger problems. :-)
>
> What's wrong with an indication of what type of "thing" a node is suppose=
d
> to be? =A0There could be multiple microcode formats, for example.
>
> I don't know that it's strictly necessary in this case -- =A0it looks lik=
e
> there is a magic number in the firmware blob -- but I don't understand th=
e
> objection as a matter of principle. =A0These device tree discussions have=
 a
> tendency to get awfully bikesheddy.

That's because the pain inflicted in getting it wrong is very high.
Consider this: The model being discussed is for U-Boot to load the
blob into the tree.  Once U-Boot is deployed there is a lot of
resistance to changing it for a lot of good reasons, even on eval
boards.  Say that firmware node thingy is put under /qe-firmware,
u-boot uses it, and the system is shipped.  Then say that the node
format is changed because of a deficiency.  Or say that the node is
moved (either intentionally, or as part of an unrelated 'cleanup').
Will any assumptions made by a deployed version of U-Boot cause
breakage?

Case in point: there are a lot of dts files in the tree using
"soc<chip>" as a node name.  If the name is changed (say to "soc" as
per generic names) then many systems won't boot because older versions
of U-Boot are hardcoded to use the soc<chip> name.

More manipulation performed by U-Boot =3D=3D more things to go horribly
horribly wrong.  :-)  We can work around breakage, but it is always
better when the design means we don't have to.

So, I'm not saying don't have properties that say what kind of thing
it is; but I am saying make it part of the QE binding proper and
isolate the scope to the binding for that node.  I'm saying be
defensive in the design and eliminate the complexities from what
u-boot needs to do.  ie. create a single named property in a well
defined location that works for any firmware blob.  U-Boot doesn't
need to care about the format.  The fact that the QE node references
that specific blob should be sufficient to define what format the blob
needs to be in.

I don't see any need to make the firmware anything more than just a
named blob.  In fact, I would be even more defensive in designing it
by not hardcoding the tree update into the U-Boot.  I'd make the fdt
command support creating properties from memory regions and squirt the
blob into the tree using a boot script instead.

>
>>>> =A0Put each firmware blob into a separate property, and make
>>>> the names reasonable (ie. mpc<blah>-qe-firmware). =A0Have the QE
>>>> reference the firmware blob by property name.
>>>
>>> I don't like the idea of using the property name as a pseudo-compatible
>>> string.
>>
>> It's a name, not a pseudo compatible string, and your device node will
>> explicitly reference it by name. =A0There is not backwards compatibility
>> or fuzzy binding issues at play here.
>
> There is a forward compatibility issue, in that we'll have to update the
> code with every new mpc<blah> (or p<blah>rev<n>) that comes along.
>
> Or are we supposed to pick some random chip to request the firmware for,
> like with compatibles? =A0What would be the point? =A0This isn't being us=
ed to
> bind a driver.

communication breakdown... see below.

>> It is a way for your driver
>> node to state, "I want *that exact* firmware blob".
>
> The driver wants the firmware blob that the device tree provides. =A0The
> device tree knows better than the driver, being that the device tree is t=
he
> describer of the hardware.

Heh.  Typo.  Sorry.  What you said is exactly what I meant.  "It is a
way for your *device node* to specify..."  Not the driver.

>> You could make
>> the property name "george"
>
> If "george" is fine, then so is "fsl,firmware". =A0Maybe "fsl,qe-firmware=
".
>
>> and it would still be completely clear (if
>> a little weird) because all the references are contained within the
>> tree.
>
> How are the references contained within the tree? =A0The driver has to kn=
ow
> which property to read.

More fallout from my typo.  I mean something like this:

/.../...../qe {
        fsl,qe-firmware-blob =3D "george";
};
chosen {
        firmware {
                george =3D /bininc/("blob.bin");
        };
};

That's what I mean by 'george' being contained within the tree.  The
QE binding will specify the fsl,qe-firmware-blob (or whatever)
property name, but the name assigned to the firmware blob the driver
doesn't actually have to care about it.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 1:53 PM, Scott Wood <scottwood@freescale.com> wrote:
> Grant Likely wrote:
>>
>> On Thu, Mar 25, 2010 at 11:03 AM, Timur Tabi <timur@freescale.com> wrote:
>>>
>>> Grant Likely wrote:
>>>>
>>>> For indirect firmware, create a /chosen/firmware node.  Don't add a
>>>> compatible property,
>>>
>>> Oh, I don't like that idea at all.  The compatible property is useful for
>>> me to know *how* to parse the binary blob.
>>
>> Compatible is for devices.
>
> Compatible is for matching.  Who cares what category the thing being matched
> is in?  What is the definition of a device, and why does it matter?

Compatible specifies a device and a list of devices it can emulate:

http://www.openfirmware.org/1275/proposals/Closed/Accepted/343-it.txt

>> This is not a device.  Drivers cannot bind
>> against it.  Use a different mechanism if you have metadata about the
>> blob.  If your driver doesn't know how to validate its own firmware
>> blobs, then you've got bigger problems.
>
> One could also say, if your hardware can't be probed at runtime, you've got
> bigger problems. :-)
>
> What's wrong with an indication of what type of "thing" a node is supposed
> to be?  There could be multiple microcode formats, for example.
>
> I don't know that it's strictly necessary in this case --  it looks like
> there is a magic number in the firmware blob -- but I don't understand the
> objection as a matter of principle.  These device tree discussions have a
> tendency to get awfully bikesheddy.

That's because the pain inflicted in getting it wrong is very high.
Consider this: The model being discussed is for U-Boot to load the
blob into the tree.  Once U-Boot is deployed there is a lot of
resistance to changing it for a lot of good reasons, even on eval
boards.  Say that firmware node thingy is put under /qe-firmware,
u-boot uses it, and the system is shipped.  Then say that the node
format is changed because of a deficiency.  Or say that the node is
moved (either intentionally, or as part of an unrelated 'cleanup').
Will any assumptions made by a deployed version of U-Boot cause
breakage?

Case in point: there are a lot of dts files in the tree using
"soc<chip>" as a node name.  If the name is changed (say to "soc" as
per generic names) then many systems won't boot because older versions
of U-Boot are hardcoded to use the soc<chip> name.

More manipulation performed by U-Boot == more things to go horribly
horribly wrong.  :-)  We can work around breakage, but it is always
better when the design means we don't have to.

So, I'm not saying don't have properties that say what kind of thing
it is; but I am saying make it part of the QE binding proper and
isolate the scope to the binding for that node.  I'm saying be
defensive in the design and eliminate the complexities from what
u-boot needs to do.  ie. create a single named property in a well
defined location that works for any firmware blob.  U-Boot doesn't
need to care about the format.  The fact that the QE node references
that specific blob should be sufficient to define what format the blob
needs to be in.

I don't see any need to make the firmware anything more than just a
named blob.  In fact, I would be even more defensive in designing it
by not hardcoding the tree update into the U-Boot.  I'd make the fdt
command support creating properties from memory regions and squirt the
blob into the tree using a boot script instead.

>
>>>>  Put each firmware blob into a separate property, and make
>>>> the names reasonable (ie. mpc<blah>-qe-firmware).  Have the QE
>>>> reference the firmware blob by property name.
>>>
>>> I don't like the idea of using the property name as a pseudo-compatible
>>> string.
>>
>> It's a name, not a pseudo compatible string, and your device node will
>> explicitly reference it by name.  There is not backwards compatibility
>> or fuzzy binding issues at play here.
>
> There is a forward compatibility issue, in that we'll have to update the
> code with every new mpc<blah> (or p<blah>rev<n>) that comes along.
>
> Or are we supposed to pick some random chip to request the firmware for,
> like with compatibles?  What would be the point?  This isn't being used to
> bind a driver.

communication breakdown... see below.

>> It is a way for your driver
>> node to state, "I want *that exact* firmware blob".
>
> The driver wants the firmware blob that the device tree provides.  The
> device tree knows better than the driver, being that the device tree is the
> describer of the hardware.

Heh.  Typo.  Sorry.  What you said is exactly what I meant.  "It is a
way for your *device node* to specify..."  Not the driver.

>> You could make
>> the property name "george"
>
> If "george" is fine, then so is "fsl,firmware".  Maybe "fsl,qe-firmware".
>
>> and it would still be completely clear (if
>> a little weird) because all the references are contained within the
>> tree.
>
> How are the references contained within the tree?  The driver has to know
> which property to read.

More fallout from my typo.  I mean something like this:

/.../...../qe {
        fsl,qe-firmware-blob = "george";
};
chosen {
        firmware {
                george = /bininc/("blob.bin");
        };
};

That's what I mean by 'george' being contained within the tree.  The
QE binding will specify the fsl,qe-firmware-blob (or whatever)
property name, but the name assigned to the firmware blob the driver
doesn't actually have to care about it.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

Grant Likely wrote:
> On Thu, Mar 25, 2010 at 1:53 PM, Scott Wood <scottwood@freescale.com> wrote:
>> Grant Likely wrote:
>>> On Thu, Mar 25, 2010 at 11:03 AM, Timur Tabi <timur@freescale.com> wrote:
>>>> Grant Likely wrote:
>>>>> For indirect firmware, create a /chosen/firmware node.  Don't add a
>>>>> compatible property,
>>>> Oh, I don't like that idea at all.  The compatible property is useful for
>>>> me to know *how* to parse the binary blob.
>>> Compatible is for devices.
>> Compatible is for matching.  Who cares what category the thing being matched
>> is in?  What is the definition of a device, and why does it matter?
> 
> Compatible specifies a device and a list of devices it can emulate:
> 
> http://www.openfirmware.org/1275/proposals/Closed/Accepted/343-it.txt

That's in a context that already assumes we're talking about a device.

I don't see the problem with interpreting it more flexibly to refer to 
compatibility with a format or binding.  ePAPR already heads in this 
direction with class compatibles such as "simple-bus", "cache", "pci", 
"iic", etc.

There's also this bit from section 6 of ePAPR: "The compatible property 
of a device node describes the specific binding (or bindings) to which 
the node complies."

>>> This is not a device.  Drivers cannot bind
>>> against it.  Use a different mechanism if you have metadata about the
>>> blob.  If your driver doesn't know how to validate its own firmware
>>> blobs, then you've got bigger problems.
>> One could also say, if your hardware can't be probed at runtime, you've got
>> bigger problems. :-)
>>
>> What's wrong with an indication of what type of "thing" a node is supposed
>> to be?  There could be multiple microcode formats, for example.
>>
>> I don't know that it's strictly necessary in this case --  it looks like
>> there is a magic number in the firmware blob -- but I don't understand the
>> objection as a matter of principle.  These device tree discussions have a
>> tendency to get awfully bikesheddy.
> 
> That's because the pain inflicted in getting it wrong is very high.

There's still a distinction between things which will actually inflict 
pain versus aesthetic considerations.  As far as I can tell, we're now 
talking about the difference between this:

device {
	fsl,firmware-phandle = <&qefw>;
};

qefw: firmware {
	compatible = "fsl,qe-foo-firmware";
	fsl,firmware = <...>;
};

and this:

device {
	fsl,qe-firmware-foo-phandle = "fsl,qe-firmware";
};

chosen {
	firmware {
		fsl,qe-firmware = <...>;
	}
};

The former involves a slightly greater chance that a driver would ignore 
the compatible and do the wrong thing on a system with 
fsl,qe-bar-firmware instead of erroring out gracefully.

The latter means any additional properties that might be needed or 
wanted to describe the firmware would have to go in the device node 
itself rather than on the entity that it describes.

I don't see either issue as being important enough to make a big fuss 
about...

> Consider this: The model being discussed is for U-Boot to load the
> blob into the tree.  Once U-Boot is deployed there is a lot of
> resistance to changing it for a lot of good reasons, even on eval
> boards.

The device firmware is hardly the only aspect of the device tree that 
would be locked down by that.  There are pros and cons of having the 
firmware be heavily involved in the process, and for better or worse 
we've gone pretty far down that road on p4080.  I think the pros 
outweigh the cons.

> Say that firmware node thingy is put under /qe-firmware,
> u-boot uses it, and the system is shipped.  Then say that the node
> format is changed because of a deficiency.  Or say that the node is
> moved (either intentionally, or as part of an unrelated 'cleanup').
> Will any assumptions made by a deployed version of U-Boot cause
> breakage?

I'd just let u-boot create the whole node from scratch.  The analogous 
problem to the soc<chip> issue would be finding the appropriate QE node, 
not anything to do with the firmware node itself.

Note that the system all of this is modeled after had the firmware 
generate the entire tree. :-)

> Case in point: there are a lot of dts files in the tree using
> "soc<chip>" as a node name.  If the name is changed (say to "soc" as
> per generic names) then many systems won't boot because older versions
> of U-Boot are hardcoded to use the soc<chip> name.
> 
> More manipulation performed by U-Boot == more things to go horribly
> horribly wrong.  :-)  We can work around breakage, but it is always
> better when the design means we don't have to.

The "soc<chip>" thing was bad because it made things harder on the 
consumer of the tree -- which happened to include u-boot.  I consider 
this an example, though, of why the trees shouldn't live in the Linux 
tree at all, except for those intended to be linked directly into the 
kernel.  The static dtb (and dts from which it is derived) is just an 
input that is used to create the actual device tree that is the subject 
of these binding documents.

For another example of firmware dependency that has nothing to do with 
it mucking around with the device tree, consider that many of the 
addresses hardcoded in the device tree are at the firmware's discretion.

U-boot in one configuration may put CCSR at 0xfe000000, and in another 
configuration may put it at 0xffe000000.  Another firmware may put it 
somewhere else entirely.  Look at the adder875 board, where we have one 
tree for u-boot and another for redboot, just because of addresses.

We've also had issues on some 83xx boards with PCI ranges not matching 
what u-boot sets up, etc.

> So, I'm not saying don't have properties that say what kind of thing
> it is; but I am saying make it part of the QE binding proper and
> isolate the scope to the binding for that node.  I'm saying be
> defensive in the design and eliminate the complexities from what
> u-boot needs to do.  ie. create a single named property in a well
> defined location that works for any firmware blob.  U-Boot doesn't
> need to care about the format.

U-boot does need to care, since it needs to use the firmware itself (and 
I suppose it gets lost when Linux initializes the hardware).  It would 
also need to care if the format were encoded in the name of the property 
in the QE node that it had to set.

> The fact that the QE node references
> that specific blob should be sufficient to define what format the blob
> needs to be in.

Not if they suddenly decide to start publishing QE firmware in a new 
format.  It wouldn't surprise me.

> I don't see any need to make the firmware anything more than just a
> named blob.  In fact, I would be even more defensive in designing it
> by not hardcoding the tree update into the U-Boot.  I'd make the fdt
> command support creating properties from memory regions and squirt the
> blob into the tree using a boot script instead.

Boot scripts are easier to for the user to break or bypass, which is a 
feature for some, but also a support headache on a project that already 
has too much user error.  The ability to explicitly run specific bootm 
phases should still give the determined user an escape from needing to 
run the u-boot fdt updates -- and if not, that's a quality of firmware 
issue, not a device binding issue.

A boot script would also need to coordinate with the u-boot code that 
uses the firmware for its own purposes to determine where the firmware 
lives (including possible complications such as having to pick from 
multiple firmwares based on runtime revision detection).  Using a boot 
script for other changes would likewise need to extract information that 
is currently generated by C code (hardware device A disabled due to an 
RCW setting, device B disabled due to an erratum (but it could still be 
enabled if the RCW didn't also enable device C), determination of 
various clocks, etc).  Not impossible, but yet more complexity.

> More fallout from my typo.  I mean something like this:
> 
> /.../...../qe {
>         fsl,qe-firmware-blob = "george";
> };
> chosen {
>         firmware {
>                 george = /bininc/("blob.bin");
>         };
> };

I see.  I don't have a problem with that, but I don't see a large 
practical difference with the existing proposal.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

Grant Likely wrote:
> On Thu, Mar 25, 2010 at 1:53 PM, Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> Grant Likely wrote:
>>> On Thu, Mar 25, 2010 at 11:03 AM, Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>>>> Grant Likely wrote:
>>>>> For indirect firmware, create a /chosen/firmware node.  Don't add a
>>>>> compatible property,
>>>> Oh, I don't like that idea at all.  The compatible property is useful for
>>>> me to know *how* to parse the binary blob.
>>> Compatible is for devices.
>> Compatible is for matching.  Who cares what category the thing being matched
>> is in?  What is the definition of a device, and why does it matter?
> 
> Compatible specifies a device and a list of devices it can emulate:
> 
> http://www.openfirmware.org/1275/proposals/Closed/Accepted/343-it.txt

That's in a context that already assumes we're talking about a device.

I don't see the problem with interpreting it more flexibly to refer to 
compatibility with a format or binding.  ePAPR already heads in this 
direction with class compatibles such as "simple-bus", "cache", "pci", 
"iic", etc.

There's also this bit from section 6 of ePAPR: "The compatible property 
of a device node describes the specific binding (or bindings) to which 
the node complies."

>>> This is not a device.  Drivers cannot bind
>>> against it.  Use a different mechanism if you have metadata about the
>>> blob.  If your driver doesn't know how to validate its own firmware
>>> blobs, then you've got bigger problems.
>> One could also say, if your hardware can't be probed at runtime, you've got
>> bigger problems. :-)
>>
>> What's wrong with an indication of what type of "thing" a node is supposed
>> to be?  There could be multiple microcode formats, for example.
>>
>> I don't know that it's strictly necessary in this case --  it looks like
>> there is a magic number in the firmware blob -- but I don't understand the
>> objection as a matter of principle.  These device tree discussions have a
>> tendency to get awfully bikesheddy.
> 
> That's because the pain inflicted in getting it wrong is very high.

There's still a distinction between things which will actually inflict 
pain versus aesthetic considerations.  As far as I can tell, we're now 
talking about the difference between this:

device {
	fsl,firmware-phandle = <&qefw>;
};

qefw: firmware {
	compatible = "fsl,qe-foo-firmware";
	fsl,firmware = <...>;
};

and this:

device {
	fsl,qe-firmware-foo-phandle = "fsl,qe-firmware";
};

chosen {
	firmware {
		fsl,qe-firmware = <...>;
	}
};

The former involves a slightly greater chance that a driver would ignore 
the compatible and do the wrong thing on a system with 
fsl,qe-bar-firmware instead of erroring out gracefully.

The latter means any additional properties that might be needed or 
wanted to describe the firmware would have to go in the device node 
itself rather than on the entity that it describes.

I don't see either issue as being important enough to make a big fuss 
about...

> Consider this: The model being discussed is for U-Boot to load the
> blob into the tree.  Once U-Boot is deployed there is a lot of
> resistance to changing it for a lot of good reasons, even on eval
> boards.

The device firmware is hardly the only aspect of the device tree that 
would be locked down by that.  There are pros and cons of having the 
firmware be heavily involved in the process, and for better or worse 
we've gone pretty far down that road on p4080.  I think the pros 
outweigh the cons.

> Say that firmware node thingy is put under /qe-firmware,
> u-boot uses it, and the system is shipped.  Then say that the node
> format is changed because of a deficiency.  Or say that the node is
> moved (either intentionally, or as part of an unrelated 'cleanup').
> Will any assumptions made by a deployed version of U-Boot cause
> breakage?

I'd just let u-boot create the whole node from scratch.  The analogous 
problem to the soc<chip> issue would be finding the appropriate QE node, 
not anything to do with the firmware node itself.

Note that the system all of this is modeled after had the firmware 
generate the entire tree. :-)

> Case in point: there are a lot of dts files in the tree using
> "soc<chip>" as a node name.  If the name is changed (say to "soc" as
> per generic names) then many systems won't boot because older versions
> of U-Boot are hardcoded to use the soc<chip> name.
> 
> More manipulation performed by U-Boot == more things to go horribly
> horribly wrong.  :-)  We can work around breakage, but it is always
> better when the design means we don't have to.

The "soc<chip>" thing was bad because it made things harder on the 
consumer of the tree -- which happened to include u-boot.  I consider 
this an example, though, of why the trees shouldn't live in the Linux 
tree at all, except for those intended to be linked directly into the 
kernel.  The static dtb (and dts from which it is derived) is just an 
input that is used to create the actual device tree that is the subject 
of these binding documents.

For another example of firmware dependency that has nothing to do with 
it mucking around with the device tree, consider that many of the 
addresses hardcoded in the device tree are at the firmware's discretion.

U-boot in one configuration may put CCSR at 0xfe000000, and in another 
configuration may put it at 0xffe000000.  Another firmware may put it 
somewhere else entirely.  Look at the adder875 board, where we have one 
tree for u-boot and another for redboot, just because of addresses.

We've also had issues on some 83xx boards with PCI ranges not matching 
what u-boot sets up, etc.

> So, I'm not saying don't have properties that say what kind of thing
> it is; but I am saying make it part of the QE binding proper and
> isolate the scope to the binding for that node.  I'm saying be
> defensive in the design and eliminate the complexities from what
> u-boot needs to do.  ie. create a single named property in a well
> defined location that works for any firmware blob.  U-Boot doesn't
> need to care about the format.

U-boot does need to care, since it needs to use the firmware itself (and 
I suppose it gets lost when Linux initializes the hardware).  It would 
also need to care if the format were encoded in the name of the property 
in the QE node that it had to set.

> The fact that the QE node references
> that specific blob should be sufficient to define what format the blob
> needs to be in.

Not if they suddenly decide to start publishing QE firmware in a new 
format.  It wouldn't surprise me.

> I don't see any need to make the firmware anything more than just a
> named blob.  In fact, I would be even more defensive in designing it
> by not hardcoding the tree update into the U-Boot.  I'd make the fdt
> command support creating properties from memory regions and squirt the
> blob into the tree using a boot script instead.

Boot scripts are easier to for the user to break or bypass, which is a 
feature for some, but also a support headache on a project that already 
has too much user error.  The ability to explicitly run specific bootm 
phases should still give the determined user an escape from needing to 
run the u-boot fdt updates -- and if not, that's a quality of firmware 
issue, not a device binding issue.

A boot script would also need to coordinate with the u-boot code that 
uses the firmware for its own purposes to determine where the firmware 
lives (including possible complications such as having to pick from 
multiple firmwares based on runtime revision detection).  Using a boot 
script for other changes would likewise need to extract information that 
is currently generated by C code (hardware device A disabled due to an 
RCW setting, device B disabled due to an erratum (but it could still be 
enabled if the RCW didn't also enable device C), determination of 
various clocks, etc).  Not impossible, but yet more complexity.

> More fallout from my typo.  I mean something like this:
> 
> /.../...../qe {
>         fsl,qe-firmware-blob = "george";
> };
> chosen {
>         firmware {
>                 george = /bininc/("blob.bin");
>         };
> };

I see.  I don't have a problem with that, but I don't see a large 
practical difference with the existing proposal.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[David Gibson]

On Thu, Mar 25, 2010 at 10:59:01AM -0600, Grant Likely wrote:
> On Thu, Mar 25, 2010 at 10:36 AM, Timur Tabi <timur@freescale.com> wrote:
> > Grant Likely wrote:
> >> On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb@firmworks.com> wrote:
> >>> It seems to me that there are plausible use cases for both direct-inclusion
> >>> and indirection.  I don't see any real problems with either, so I would vote
> >>> for specifying both alternatives.
> >>
> >> Ugh.  Then this one driver would need to implement both binding for
> >> little, if any, actual benefit.
> >
> > Although I agree that I don't like supporting both bindings, we could encapsulate the locating of the firmware node in a function.  The function will first look for a child firmware node, and if it doesn't find it, look for a fsl,firmware property.  It will return a pointer to the firmware node regardless.
> >
> >> I'm sure we can come to an agreement
> >> on one method if the firmware absolutely has to be in the tree.
> >
> > If we have to pick one, then I think the only viable choice is have a separate firmware node and a phandle pointer to it.  Otherwise, I just don't see how we can handle multiple devices needing the same firmware.
> 
> Wait for David to weigh in on this one before making a decision.  He
> knows the dtb format best.

I'll do that when I can.  Right now I'm in the middle of intense
bringup work so I won't have time for this for a while.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[David Gibson]

On Thu, Mar 25, 2010 at 10:59:01AM -0600, Grant Likely wrote:
> On Thu, Mar 25, 2010 at 10:36 AM, Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > Grant Likely wrote:
> >> On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> wrote:
> >>> It seems to me that there are plausible use cases for both direct-inclusion
> >>> and indirection.  I don't see any real problems with either, so I would vote
> >>> for specifying both alternatives.
> >>
> >> Ugh.  Then this one driver would need to implement both binding for
> >> little, if any, actual benefit.
> >
> > Although I agree that I don't like supporting both bindings, we could encapsulate the locating of the firmware node in a function.  The function will first look for a child firmware node, and if it doesn't find it, look for a fsl,firmware property.  It will return a pointer to the firmware node regardless.
> >
> >> I'm sure we can come to an agreement
> >> on one method if the firmware absolutely has to be in the tree.
> >
> > If we have to pick one, then I think the only viable choice is have a separate firmware node and a phandle pointer to it.  Otherwise, I just don't see how we can handle multiple devices needing the same firmware.
> 
> Wait for David to weigh in on this one before making a decision.  He
> knows the dtb format best.

I'll do that when I can.  Right now I'm in the middle of intense
bringup work so I won't have time for this for a while.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 10:36 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>> On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb@firmworks.com> wrote=
:
>>> It seems to me that there are plausible use cases for both direct-inclu=
sion
>>> and indirection. =A0I don't see any real problems with either, so I wou=
ld vote
>>> for specifying both alternatives.
>>
>> Ugh. =A0Then this one driver would need to implement both binding for
>> little, if any, actual benefit.
>
> Although I agree that I don't like supporting both bindings, we could enc=
apsulate the locating of the firmware node in a function. =A0The function w=
ill first look for a child firmware node, and if it doesn't find it, look f=
or a fsl,firmware property. =A0It will return a pointer to the firmware nod=
e regardless.

Wait... After eating dinner and mulling on it a bit, I think I've
changed my mind.  I have no objections left if you do it the way Mitch
suggests with one minor change:

diff --git a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
index 6e37be1..d9d6431 100644
--- a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
+++ b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
 - fsl,qe-num-riscs: define how many RISC engines the QE has.
 - fsl,qe-num-snums: define how many serial number(SNUM) the QE can use for=
 the
  threads.
+- fsl,firmware:
+    Usage: Optional.
+    Value type: <prop-encoded-array>, encoded array of bytes
+    Definition: Contains the QUICC engine firmware blob.
[plus any other properties needed for firmware metadata]
+- fsl,firmware-phandle:
+    Usage: Required if fsl,firmware property is not present.
+    Value type: <phandle>
+    Definition: Points to another fsl,qe node that has the
fsl,firmware property.

 Recommended properties
 - brg-frequency : the internal clock source frequency for baud-rate

[...]

+Example:
+       qe1: qe@e0080000 {
+               compatible =3D "fsl,qe";
+               fsl,firmware =3D /bininc/("firmware-blob.bin");  /* Or
squirted in by firmware */
+               ...
+       }
+
+       qe@e0090000 {
+               compatible =3D "fsl,qe";
+               fsl,firmware-phandle =3D <&qe1>;
+               ...
+       }

Putting the blob into just one of the qe nodes keeps everything nicely
contained with the device it actually applies to.  No debates about
the best place to put device firmware blobs or new compatible values,
and it is applicable to any device where firmware needs to be passed
via the tree.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Thu, Mar 25, 2010 at 10:36 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>> On Thu, Mar 25, 2010 at 9:29 AM, Mitch Bradley <wmb@firmworks.com> wrote:
>>> It seems to me that there are plausible use cases for both direct-inclusion
>>> and indirection.  I don't see any real problems with either, so I would vote
>>> for specifying both alternatives.
>>
>> Ugh.  Then this one driver would need to implement both binding for
>> little, if any, actual benefit.
>
> Although I agree that I don't like supporting both bindings, we could encapsulate the locating of the firmware node in a function.  The function will first look for a child firmware node, and if it doesn't find it, look for a fsl,firmware property.  It will return a pointer to the firmware node regardless.

Wait... After eating dinner and mulling on it a bit, I think I've
changed my mind.  I have no objections left if you do it the way Mitch
suggests with one minor change:

diff --git a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
index 6e37be1..d9d6431 100644
--- a/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
+++ b/Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe.txt
 - fsl,qe-num-riscs: define how many RISC engines the QE has.
 - fsl,qe-num-snums: define how many serial number(SNUM) the QE can use for the
  threads.
+- fsl,firmware:
+    Usage: Optional.
+    Value type: <prop-encoded-array>, encoded array of bytes
+    Definition: Contains the QUICC engine firmware blob.
[plus any other properties needed for firmware metadata]
+- fsl,firmware-phandle:
+    Usage: Required if fsl,firmware property is not present.
+    Value type: <phandle>
+    Definition: Points to another fsl,qe node that has the
fsl,firmware property.

 Recommended properties
 - brg-frequency : the internal clock source frequency for baud-rate

[...]

+Example:
+       qe1: qe@e0080000 {
+               compatible = "fsl,qe";
+               fsl,firmware = /bininc/("firmware-blob.bin");  /* Or
squirted in by firmware */
+               ...
+       }
+
+       qe@e0090000 {
+               compatible = "fsl,qe";
+               fsl,firmware-phandle = <&qe1>;
+               ...
+       }

Putting the blob into just one of the qe nodes keeps everything nicely
contained with the device it actually applies to.  No debates about
the best place to put device firmware blobs or new compatible values,
and it is applicable to any device where firmware needs to be passed
via the tree.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:

> +- fsl,firmware:
> +    Usage: Optional.
> +    Value type: <prop-encoded-array>, encoded array of bytes
> +    Definition: Contains the QUICC engine firmware blob.
> [plus any other properties needed for firmware metadata]

This would place the firmware metadata properties inside the QE node itself, which would break the QE binding.

> +Example:
> +       qe1: qe@e0080000 {
> +               compatible = "fsl,qe";
> +               fsl,firmware = /bininc/("firmware-blob.bin");  /* Or
> squirted in by firmware */
> +               ...
> +       }
> +
> +       qe@e0090000 {
> +               compatible = "fsl,qe";
> +               fsl,firmware-phandle = <&qe1>;
> +               ...
> +       }
> 
> Putting the blob into just one of the qe nodes keeps everything nicely
> contained with the device it actually applies to.  No debates about
> the best place to put device firmware blobs or new compatible values,
> and it is applicable to any device where firmware needs to be passed
> via the tree.

Except when you actually need to add metadata properties:

       qe1: qe@e0080000 {
               compatible = "fsl,qe";
               fsl,firmware = /bininc/("firmware-blob.bin"); 
               fsl,qe-firmware-eccr = <0x00000000 0x00001230>;
               ...
       }

(The ECCR is stored in the QE firmware blob, but let's pretend it isn't and I need to specify it)

Here, the fsl,qe-firmware-eccr property is associated with the QE itself.  This is why I want a compatible property for the firmware node, no matter where it is.  Then you can do this:

       qe1: qe@e0080000 {
               compatible = "fsl,qe";
               fsl,qe_firmware {
                       compatible="fsl,qe-firmware";
                       fsl,firmware = /bininc/("firmware-blob.bin"); 
                       fsl,qe-firmware-eccr = <0x00000000 0x00001230>;
               }
               ...
       }

Without the compatible property, the only way I'd know that the child node contains a firmware is to look at the actual name of the child node, which (as Scott and I believe) is not better than a compatible property.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:

> +- fsl,firmware:
> +    Usage: Optional.
> +    Value type: <prop-encoded-array>, encoded array of bytes
> +    Definition: Contains the QUICC engine firmware blob.
> [plus any other properties needed for firmware metadata]

This would place the firmware metadata properties inside the QE node itself, which would break the QE binding.

> +Example:
> +       qe1: qe@e0080000 {
> +               compatible = "fsl,qe";
> +               fsl,firmware = /bininc/("firmware-blob.bin");  /* Or
> squirted in by firmware */
> +               ...
> +       }
> +
> +       qe@e0090000 {
> +               compatible = "fsl,qe";
> +               fsl,firmware-phandle = <&qe1>;
> +               ...
> +       }
> 
> Putting the blob into just one of the qe nodes keeps everything nicely
> contained with the device it actually applies to.  No debates about
> the best place to put device firmware blobs or new compatible values,
> and it is applicable to any device where firmware needs to be passed
> via the tree.

Except when you actually need to add metadata properties:

       qe1: qe@e0080000 {
               compatible = "fsl,qe";
               fsl,firmware = /bininc/("firmware-blob.bin"); 
               fsl,qe-firmware-eccr = <0x00000000 0x00001230>;
               ...
       }

(The ECCR is stored in the QE firmware blob, but let's pretend it isn't and I need to specify it)

Here, the fsl,qe-firmware-eccr property is associated with the QE itself.  This is why I want a compatible property for the firmware node, no matter where it is.  Then you can do this:

       qe1: qe@e0080000 {
               compatible = "fsl,qe";
               fsl,qe_firmware {
                       compatible="fsl,qe-firmware";
                       fsl,firmware = /bininc/("firmware-blob.bin"); 
                       fsl,qe-firmware-eccr = <0x00000000 0x00001230>;
               }
               ...
       }

Without the compatible property, the only way I'd know that the child node contains a firmware is to look at the actual name of the child node, which (as Scott and I believe) is not better than a compatible property.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Fri, Mar 26, 2010 at 9:17 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>
>> +- fsl,firmware:
>> + =A0 =A0Usage: Optional.
>> + =A0 =A0Value type: <prop-encoded-array>, encoded array of bytes
>> + =A0 =A0Definition: Contains the QUICC engine firmware blob.
>> [plus any other properties needed for firmware metadata]
>
> This would place the firmware metadata properties inside the QE node itse=
lf, which would break the QE binding.

Not really true.  Adding extra properties seldom breaks a binding.
It's a wash when compared with compared with adding a node.  But
that's not really an important debate.

> Here, the fsl,qe-firmware-eccr property is associated with the QE itself.=
 =A0This is why I want a compatible property for the firmware node, no matt=
er where it is. =A0Then you can do this:
>
> =A0 =A0 =A0 qe1: qe@e0080000 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,qe";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 fsl,qe_firmware {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible=3D"fsl,qe-firmware=
";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fsl,firmware =3D /bininc/("fi=
rmware-blob.bin");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fsl,qe-firmware-eccr =3D <0x0=
0000000 0x00001230>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 ...
> =A0 =A0 =A0 }
>
> Without the compatible property, the only way I'd know that the child nod=
e contains a firmware is to look at the actual name of the child node, whic=
h (as Scott and I believe) is not better than a compatible property.

If it is always a child of a qe node, then I've got no objections.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Fri, Mar 26, 2010 at 9:17 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>
>> +- fsl,firmware:
>> +    Usage: Optional.
>> +    Value type: <prop-encoded-array>, encoded array of bytes
>> +    Definition: Contains the QUICC engine firmware blob.
>> [plus any other properties needed for firmware metadata]
>
> This would place the firmware metadata properties inside the QE node itself, which would break the QE binding.

Not really true.  Adding extra properties seldom breaks a binding.
It's a wash when compared with compared with adding a node.  But
that's not really an important debate.

> Here, the fsl,qe-firmware-eccr property is associated with the QE itself.  This is why I want a compatible property for the firmware node, no matter where it is.  Then you can do this:
>
>       qe1: qe@e0080000 {
>               compatible = "fsl,qe";
>               fsl,qe_firmware {
>                       compatible="fsl,qe-firmware";
>                       fsl,firmware = /bininc/("firmware-blob.bin");
>                       fsl,qe-firmware-eccr = <0x00000000 0x00001230>;
>               }
>               ...
>       }
>
> Without the compatible property, the only way I'd know that the child node contains a firmware is to look at the actual name of the child node, which (as Scott and I believe) is not better than a compatible property.

If it is always a child of a qe node, then I've got no objections.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:
>> Without the compatible property, the only way I'd know that the child node contains a firmware is to look at the actual name of the child node, which (as Scott and I believe) is not better than a compatible property.
> If it is always a child of a qe node, then I've got no objections.

I have no problem with putting the firmware node as a child of the QE node and skipping the phandle property, but only as long as there's only one QE node.  Would you agree that this is bad:

qe1: qe@e0080000 {
        compatible = "fsl,qe";
        qefw: fsl,qe_firmware {
                compatible="fsl,qe-firmware";
                fsl,firmware = /bininc/("firmware-blob.bin");
                fsl,qe-firmware-eccr = <0x00000000 0x00001230>;
        }
        ...
}

qe2: qe@e0090000 {
        compatible = "fsl,qe";
	fsl,firmware-phandle = <&qefw>;
        ...
}

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Fri, Mar 26, 2010 at 12:39 PM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>>> Without the compatible property, the only way I'd know that the child n=
ode contains a firmware is to look at the actual name of the child node, wh=
ich (as Scott and I believe) is not better than a compatible property.
>> If it is always a child of a qe node, then I've got no objections.
>
> I have no problem with putting the firmware node as a child of the QE nod=
e and skipping the phandle property, but only as long as there's only one Q=
E node. =A0Would you agree that this is bad:
>
> qe1: qe@e0080000 {
> =A0 =A0 =A0 =A0compatible =3D "fsl,qe";
> =A0 =A0 =A0 =A0qefw: fsl,qe_firmware {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible=3D"fsl,qe-firmware";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl,firmware =3D /bininc/("firmware-blob.b=
in");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl,qe-firmware-eccr =3D <0x00000000 0x000=
01230>;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0...
> }
>
> qe2: qe@e0090000 {
> =A0 =A0 =A0 =A0compatible =3D "fsl,qe";
> =A0 =A0 =A0 =A0fsl,firmware-phandle =3D <&qefw>;
> =A0 =A0 =A0 =A0...
> }

Nah.  That looks totally fine.  Not having the firmware under a qe
node would look bad to me.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Fri, Mar 26, 2010 at 12:39 PM, Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Grant Likely wrote:
>>> Without the compatible property, the only way I'd know that the child node contains a firmware is to look at the actual name of the child node, which (as Scott and I believe) is not better than a compatible property.
>> If it is always a child of a qe node, then I've got no objections.
>
> I have no problem with putting the firmware node as a child of the QE node and skipping the phandle property, but only as long as there's only one QE node.  Would you agree that this is bad:
>
> qe1: qe@e0080000 {
>        compatible = "fsl,qe";
>        qefw: fsl,qe_firmware {
>                compatible="fsl,qe-firmware";
>                fsl,firmware = /bininc/("firmware-blob.bin");
>                fsl,qe-firmware-eccr = <0x00000000 0x00001230>;
>        }
>        ...
> }
>
> qe2: qe@e0090000 {
>        compatible = "fsl,qe";
>        fsl,firmware-phandle = <&qefw>;
>        ...
> }

Nah.  That looks totally fine.  Not having the firmware under a qe
node would look bad to me.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:

> Nah.  That looks totally fine.  Not having the firmware under a qe
> node would look bad to me.

You don't think it weird to have one QE node reference data from another QE node, or that the DTS implies that the firmware belongs to one QE more than it belongs to the other?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Timur Tabi]

Grant Likely wrote:

> Nah.  That looks totally fine.  Not having the firmware under a qe
> node would look bad to me.

You don't think it weird to have one QE node reference data from another QE node, or that the DTS implies that the firmware belongs to one QE more than it belongs to the other?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Fri, Mar 26, 2010 at 12:48 PM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>
>> Nah. =A0That looks totally fine. =A0Not having the firmware under a qe
>> node would look bad to me.
>
> You don't think it weird to have one QE node reference data from another =
QE node, or that the DTS implies that the firmware belongs to one QE more t=
han it belongs to the other?

Nope.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Fri, Mar 26, 2010 at 12:48 PM, Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Grant Likely wrote:
>
>> Nah.  That looks totally fine.  Not having the firmware under a qe
>> node would look bad to me.
>
> You don't think it weird to have one QE node reference data from another QE node, or that the DTS implies that the firmware belongs to one QE more than it belongs to the other?

Nope.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Mitch Bradley]

Timur Tabi wrote:
> Grant Likely wrote:
>
>   
>> Nah.  That looks totally fine.  Not having the firmware under a qe
>> node would look bad to me.
>>     
>
> You don't think it weird to have one QE node reference data from another QE node, or that the DTS implies that the firmware belongs to one QE more than it belongs to the other?
>   

It is certainly not symmetric.

Putting the firmware blob somewhere completely unrelated to either node 
maintains the symmetry between the two qe nodes, but only weakly 
captures the relationship between the firmware blob and the qe nodes.  
It is then necessary to invoke a strong naming convention for the 
firmware blob, because you don't have the hierarchy to do the name space 
disambiguation for you.

As I see it, the three possibilities, and their disadvantages, are:

a) Firmware blob in some random place - requires strong naming of either 
firmware blob property or node containing it.

b) Firmware blob within first qe node - asymmetric.

c) Firmware blob in new parent of both qe nodes - requires introduction 
of otherwise-unneeded hierarchy level.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Mitch Bradley]

Timur Tabi wrote:
> Grant Likely wrote:
>
>   
>> Nah.  That looks totally fine.  Not having the firmware under a qe
>> node would look bad to me.
>>     
>
> You don't think it weird to have one QE node reference data from another QE node, or that the DTS implies that the firmware belongs to one QE more than it belongs to the other?
>   

It is certainly not symmetric.

Putting the firmware blob somewhere completely unrelated to either node 
maintains the symmetry between the two qe nodes, but only weakly 
captures the relationship between the firmware blob and the qe nodes.  
It is then necessary to invoke a strong naming convention for the 
firmware blob, because you don't have the hierarchy to do the name space 
disambiguation for you.

As I see it, the three possibilities, and their disadvantages, are:

a) Firmware blob in some random place - requires strong naming of either 
firmware blob property or node containing it.

b) Firmware blob within first qe node - asymmetric.

c) Firmware blob in new parent of both qe nodes - requires introduction 
of otherwise-unneeded hierarchy level.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Fri, Mar 26, 2010 at 12:58 PM, Mitch Bradley <wmb@firmworks.com> wrote:
> a) Firmware blob in some random place - requires strong naming of either
> firmware blob property or node containing it.

BTW, this exactly the reason for all the bikesheding earlier; but I
don't care at all if it is under a QE node.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Grant Likely]

On Fri, Mar 26, 2010 at 12:58 PM, Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> wrote:
> a) Firmware blob in some random place - requires strong naming of either
> firmware blob property or node containing it.

BTW, this exactly the reason for all the bikesheding earlier; but I
don't care at all if it is under a QE node.

g.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Mitch Bradley]

Timur Tabi wrote:
> Grant Likely wrote:
>   
>>> Without the compatible property, the only way I'd know that the child node contains a firmware is to look at the actual name of the child node, which (as Scott and I believe) is not better than a compatible property.
>>>       
>> If it is always a child of a qe node, then I've got no objections.
>>     
>
> I have no problem with putting the firmware node as a child of the QE node and skipping the phandle property, but only as long as there's only one QE node.  Would you agree that this is bad:
>
> qe1: qe@e0080000 {
>         compatible = "fsl,qe";
>         qefw: fsl,qe_firmware {
>                 compatible="fsl,qe-firmware";
>                 fsl,firmware = /bininc/("firmware-blob.bin");
>                 fsl,qe-firmware-eccr = <0x00000000 0x00001230>;
>         }
>         ...
> }
>
> qe2: qe@e0090000 {
>         compatible = "fsl,qe";
> 	fsl,firmware-phandle = <&qefw>;
>         ...
> }
>
>   

It not any worse than having the firmware blob anywhere else that is not 
hierarchically related.

If one insists on purity of hierarchy, one could introduce a node above 
qe1 and qe2 and put the firmware blob in that parent node.  That 
captures the assertion that the two qe devices are in fact identical so 
the same firmware is suitable for both.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Mitch Bradley]

Timur Tabi wrote:
> Grant Likely wrote:
>   
>>> Without the compatible property, the only way I'd know that the child node contains a firmware is to look at the actual name of the child node, which (as Scott and I believe) is not better than a compatible property.
>>>       
>> If it is always a child of a qe node, then I've got no objections.
>>     
>
> I have no problem with putting the firmware node as a child of the QE node and skipping the phandle property, but only as long as there's only one QE node.  Would you agree that this is bad:
>
> qe1: qe@e0080000 {
>         compatible = "fsl,qe";
>         qefw: fsl,qe_firmware {
>                 compatible="fsl,qe-firmware";
>                 fsl,firmware = /bininc/("firmware-blob.bin");
>                 fsl,qe-firmware-eccr = <0x00000000 0x00001230>;
>         }
>         ...
> }
>
> qe2: qe@e0090000 {
>         compatible = "fsl,qe";
> 	fsl,firmware-phandle = <&qefw>;
>         ...
> }
>
>   

It not any worse than having the firmware blob anywhere else that is not 
hierarchically related.

If one insists on purity of hierarchy, one could introduce a node above 
qe1 and qe2 and put the firmware blob in that parent node.  That 
captures the assertion that the two qe devices are in fact identical so 
the same firmware is suitable for both.

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

On Wed, Mar 24, 2010 at 11:07:42AM -0600, Grant Likely wrote:
> On Wed, Mar 24, 2010 at 11:00 AM, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >>> Why the phandle redirection?  Why not just put the firmware blob into
> >>> a property in the QE node, or as a subnode?
> >>
> >> Because there might be multiple QE devices on a single chip, and each
> >> will need to upload the same firmware.  So instead of embedding the
> >> firmware multiple times, just embed it once, and have a pointer.
> >
> > You're messing up the binding because of a (perceived) deficiency in
> > the DTB format?  Or maybe just the DTS format.  Or maybe you shouldn't
> > even care about size here.  Or really, the device tree is the wrong
> > place to store firmware blobs at all.
> 
> That is a good question.  Why is it necessary to pass the blob via the
> tree?

We previously put the firmware in flash, and passed a pointer on the kernel
command line.  Not only is that more complicated and error prone, it
requires that virtualized guests be granted access to the flash.

> Also, depending on firmware to correctly squirt the firmware blob into
> the dtb at boot is risky.  Even when firmware is buggy, there is
> resistance to upgrading firmware 

We need to use different terms for the boot firmware and the device
firmware. :-)

> on working boards because it could result in a bricked board.  In fact,
> every time we depend on firmware to modify the dtb at boot is risky, so it
> should only be done when strictly necessary (I would even say that to date
> we've probably been rather too liberal about getting u-boot to modify the
> device tree).

On p4080 we've gone much farther down that road, a large chunk of the device
tree content is generated dynamically by u-boot.  Being able to use the
device tree to move information from the firmware to the OS is one of the
big benefits of using the tree, IMHO.  If you end up in a situation where
you just can't use the firmware's tree for whatever reason, ignore it and
use a kernel-provided tree and you'll be no worse off than what you'd have
us do all the time.

And it's not even really relevant to whether the firmware goes in the dtb --
you could just as easily stick it in there statically with a binary include
in the dts.

> I would say that either the firmware should be loaded via the existing
> (non-dt) firmware loading mechanism, or it should be built into the
> static dtb blob.  Don't try to add it at runtime.

The binding should only be concerned with what's in the tree, not how it
gets there.

-Scott

Re: [PATCH] powerpc/fsl: add device tree binding for QE firmware

[Scott Wood]

On Wed, Mar 24, 2010 at 11:07:42AM -0600, Grant Likely wrote:
> On Wed, Mar 24, 2010 at 11:00 AM, Segher Boessenkool
> <segher-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> >>> Why the phandle redirection?  Why not just put the firmware blob into
> >>> a property in the QE node, or as a subnode?
> >>
> >> Because there might be multiple QE devices on a single chip, and each
> >> will need to upload the same firmware.  So instead of embedding the
> >> firmware multiple times, just embed it once, and have a pointer.
> >
> > You're messing up the binding because of a (perceived) deficiency in
> > the DTB format?  Or maybe just the DTS format.  Or maybe you shouldn't
> > even care about size here.  Or really, the device tree is the wrong
> > place to store firmware blobs at all.
> 
> That is a good question.  Why is it necessary to pass the blob via the
> tree?

We previously put the firmware in flash, and passed a pointer on the kernel
command line.  Not only is that more complicated and error prone, it
requires that virtualized guests be granted access to the flash.

> Also, depending on firmware to correctly squirt the firmware blob into
> the dtb at boot is risky.  Even when firmware is buggy, there is
> resistance to upgrading firmware 

We need to use different terms for the boot firmware and the device
firmware. :-)

> on working boards because it could result in a bricked board.  In fact,
> every time we depend on firmware to modify the dtb at boot is risky, so it
> should only be done when strictly necessary (I would even say that to date
> we've probably been rather too liberal about getting u-boot to modify the
> device tree).

On p4080 we've gone much farther down that road, a large chunk of the device
tree content is generated dynamically by u-boot.  Being able to use the
device tree to move information from the firmware to the OS is one of the
big benefits of using the tree, IMHO.  If you end up in a situation where
you just can't use the firmware's tree for whatever reason, ignore it and
use a kernel-provided tree and you'll be no worse off than what you'd have
us do all the time.

And it's not even really relevant to whether the firmware goes in the dtb --
you could just as easily stick it in there statically with a binary include
in the dts.

> I would say that either the firmware should be loaded via the existing
> (non-dt) firmware loading mechanism, or it should be built into the
> static dtb blob.  Don't try to add it at runtime.

The binding should only be concerned with what's in the tree, not how it
gets there.

-Scott