| Message ID | 1269380552-10418-1-git-send-email-timur@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 >
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.
>> 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
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.
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.
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.
> > > 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.
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
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.
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
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.
>>>>> 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
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.
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
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.
[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.
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. 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.
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
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?
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 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
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 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.
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 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.
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
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.
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.
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.
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.
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.
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
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
On Thu, Mar 25, 2010 at 6:53 PM, M. Warner Losh <imp@bsdimp.com> 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.
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.
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.
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.
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
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>; ... }
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 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.
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.
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?
On Fri, Mar 26, 2010 at 12:48 PM, Timur Tabi <timur@freescale.com> 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.
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.
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.
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 ...> + } +
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(-)