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

[Grant Likely]

On Thu, Mar 25, 2010 at 1:53 PM, Scott Wood <scottwood at freescale.com> wrote:
> Grant Likely wrote:
>>
>> On Thu, Mar 25, 2010 at 11:03 AM, Timur Tabi <timur at 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.