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

[Timur Tabi <timur@freescale.com>]

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