Wolfgang Denk wrote:
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.
Technically, that's true, but in most cases the function that returns the address/size of the firmware would exist in board code.
For example, the MPC8323 has a bug in its QE UART support, so if you want to use the QE as a UART, you need to download a special QE firmware to work-around the bug. So even though all MPC8323 boards have a QE that can run in UART mode, only those boards that have a QE wired to an RS232 port need to enable the UART work-around, so only those boards would embed the firmware in the device tree.
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.
Ok.
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?
For example, QE firmware comes with its own header that includes size information, as well as a magic number and a CRC. See struct qe_firmware in qe.h. Since we distribute the QE firmware on our boards as-is, it would be nice if we didn't need to wrap those binaries inside a legacy/FIT image. So on an MPC8323E-MDS board, for example, the board-specific implementation of fdt_board_size() could support having fdt_fw_addr points directly to a QE firmware binary. We already have code that validates a QE firmware (see function qe_upload_firmware()), so this is safe.
For those firmware types that don't have such a header built-in, they would need to be wrapped in a legacy/FIT image.
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.
Ok.
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.
I agree, although I would say that only "size" is necessary.
"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?
I'm not sure that's a good idea. We can't predict what kind of firmware we'll need to support in the future, and a board-specific fdt_board_size() function is, in my opinion, a more elegant way to solve the problem.
#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);
Where is the function prototype for handle_fdt_fw_qe()? Wouldn't it be in qe.h?
Also, I really think the strcmp is a bad idea, because then we have to put a specific string in the ih_name field, not something unique to the actual firmware in the 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.
So it would look like board_init_r()?
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.
Ok.
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.
Kumar's function would be defined in a board-specific source file. Using the QE firmware example for the 8323 above, I would add this code to board/freescale/mpc832xemds/mpc832xemds.c:
#include "qe.h"
int fdt_board_size(void) { struct qe_firmware *qefw = getenv("fdt_fw_addr");
if (!qefw) return 0;
if (!is_valid_qe_firmware(qefw)) return 0;
return qefw->header.length; }
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); ... ?
It wouldn't be. I don't like either method.
I believe we should have a board-specific function that figures out how much extra space is needed, and just returns a single integer that the boot_relocate_fdt() uses to pad the FDT when it relocates it.
That board-specific function can do whatever it needs to do to figure out which firmware binaries need to be embedded, where they are, how big they are, and more importantly, what format they're in.
Later on, function ft_board_setup() will be the function that actually embeds the firmware into the device tree.