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

[Timur Tabi]

Wolfgang Denk wrote:

>> Thanks.  Just to be clear, do you expect fdt_fw_addr always to point to a
>> FIT-wrapped firmware binary?
> 
> 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.

> 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?

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.

>> We never finished this discussion.  My point was that even if the firmware
>> is wrapped in a FIT image, the process by which the firmware is actually
>> inserted into the device tree is specific to the actual firmware.  You could
>> even say it's board-specific.
> 
> You could say that. You could also say that 2+2=5.
> 
> I will argue in both cases.

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.

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.

>> 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
"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.

> And if
> you follow my recommendation of using the name entry for type
> information (or come up with a better proposal) we also know the
> type.

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"

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
         */
}

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?

>> Therefore, there will be two pieces of code that references fdt_fw_addr.
>> The first is in boot_relocate_fdt(), which will extract the size information
>> from the FIT image that fdt_fw_addr points to.  The second is the QE code
>> which extracts the firmware from the FIT image and embeds it into the device
>> tree, in a QE-specific way.
> 
> 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?

> 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 */
	} ...

-- 
Timur Tabi
Linux kernel developer at Freescale