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

[Timur Tabi <timur@freescale.com>]

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