Dear Timur Tabi, In message <4bfd3a39.4090...@freescale.com> you wrote: > > > Please re-read the IRC log. Kumar explicitly stated he was trying to > > avoid making FIT images mandatory, at least for now. > > And he proposed a board-specific function that would allow this to work, but > you rejected it. So I don't still know how to implement what you want.
Well, in a way that may be image-type dependent, but that is not board-specific. > > And I explicitly > > wrote that it should be "the address of a IH_TYPE_FIRMWARE image > > then". > > So you're saying fdt_fw_addr should pointer to either a FIT image or the > older image type? What's the point in supporting the older type? Isn't it > deprecated? No, not really. It works fine for the intended purpose. Actually I still prefer it in a lot of cases because we have checksum protection of the header information, while you can have a totally corrupted DTB without really being able to detect it. > But either way, the firmware needs to be wrapped inside an image object. I > think Kumar was implying that he didn't want to make *any* image type (FIT > or legacy) mandatory. And where would you then get type and size information from? > I don't understand your position. The method by which firmware is to be > embedded in the device tree *is* specific to the kind of firmware in > question, and therefore requires knowledge of the kind of firmware. A QE > firmware is not embedded in the device tree the same way an FPGA firmware > is. This is just a fact. I also said that I see no problems with ading type specific hooks. > You keep telling me that there's a counter argument to this statement, but I > don't know what it is. You just tell me you disagree. In effect, you are > the one saying that 2+2=5. Really? > >> In contrast, you want the fdt relocation code to be able to increase the > >> size of the fdt without knowing any details about the firmware itself. > > > > That's not correct. At least we know the address and the size. > > Address and size is *not* details about the firmware itself. When I say No, but they are important properties, for example when it comes to find out by how much the DT needs to be grown. > "details about the firmware itself", I mean stuff like what kind of firmware > it is, what chip it's for, what it's supposed to do, etc. Maybe we can abstrct off most of this, and/or leave it to image type specific handlers? > Are you talking about the ih_name field in the image_header_t structure? So > for instance, if the ih_name field says "QE Firmware", we can assume assume > that it's a QE firmware, and the generic code should have something like > this in it: > > #include "qe.h" No. There would be no "qe.h" needed in that generic code. > if (strcmp(image->ih_name, "QE Firmware") == 0) { > /* > * Embed the firmware in the device tree using the binding > * definition in Documentation/powerpc/dts-bindings/fsl/cpm_qe > * /qe.txt > */ > } No. More like if (strcmp(image->ih_name, "QE Firmware") == 0) handle_fdt_fw_qe(image); or similar. Probably #ifdef'ed for machines that have enabled QE support in their board config, or with a weak handle_fdt_fw_qe() that gets filled in for QE aware boards only so the compiler optimizes away that call everywhere else. > Doesn't that seem really clunky to you? That requires the generic code to > have knowledge of every type of firmware. Wouldn't it be simpler if we just > followed Kumar's recommendation to have a board-specific __weak__ function > that handled this code? D*mn. Don't you get it. There is NOT BOARD-SPECIFIC CODE anywhere. It is feature specific. Either you enable QE support or not, but then the same code will be used for all boards enabling this feature. > > I see no inherent problems with having a generic, common part for all > > systems enabling this feature, plus eventually hooks for (additional) > > customized processing of certain firmware image types. > > So you agree with Kumar's idea of having a weak function that embeds the > firmware into the device tree, but the firmware must always be wrapped in an > image format? Yes. Note that there is NOT any board-specific code. > > Of course one can argue that making the decision on the type based on > > the name entry is a stupid thing, and come up for example with > > additional IH_TYPE entries; or even try to define subtypes. I think > > I'll leave this as an exercise to you :-) > > I'd rather not use subtypes, because I don't think we want anything like this: > > if (is_qe_firmware()) { > /* embed QE firmware */ > } else if (is_amd_fpga_firmware()) { > /* embed AMD fpga firmware */ > } ... In which way would that be worse compared to if (strcmp(image->ih_name, "QE Firmware") == 0) handle_fdt_fw_qe(image); else if (strcmp(image->ih_name, "AMD FPGA Firmware") == 0) handle_fdt_fw_amd(image); ... ? Actually it would be easier to read... Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The game of life is a game of boomerangs. Our thoughts, deeds and words return to us sooner or later with astounding accuracy. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot