[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale