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

[Wolfgang Denk <wd@denx.de>]

Dear Timur Tabi,

In message <4BFD3A39.4090209@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: wd at 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.