[U-Boot-Users] [PATCH] RFC: generic property fixup mechanism for LIBFDT

[Jerry Van Baren]

Grant Likely wrote:
> On 8/23/07, Jerry Van Baren <gerald.vanbaren at smiths-aerospace.com> wrote:
>> The first half of Grant's comment was:
>>  > These 4 functions are pretty close to identical (except for the
>>  > parameter to cpu_to_be32()).  Surely there is a more compact way to do
>>  > this.
>>
>> My answer is "no, the whole reason there is one line different is
>> because it *has to be*."  As the French say about the "Y" chromosome
>> "...and viva la difference."  My first attempt, and Bartlomiej's
>> proposed patch, tried (tries) to put the value in the table, but it is a
>> *WORSE* solution because you have the problem with byteswapping or not
>> and different sizes of values.  The scorecard for putting the values in
>> the tables and also having it maintainable is 0 for 2.
> 
> Yes, this is the icky bit; but I still  argue that there is a more
> compact way to do it.  Table driven is nice when it works; but (as I
> also mentioned in my original comment) whie it doesn't its probably
> better to just fall back to something programatic
> 
> ie (totally untested):
> 
> static int fixup_prop(void *fdt, char *node, char *prop, void *val, int size)
> {
>         int nodeoffset;
>         int rc;
> 
>         nodeoffset = fdt_find_node_by_path(fdt, node);
>         if (nodeoffset < 0) {
>                 debug("Couldn't find %s: %s\n", node, fdt_strerror(nodeoffset));
>                 return nodeoffset;
>         }
> 
>         rc = fdt_get_property(fdt, nodeoffset, prop, 0)
>         if (rc)
>                 goto err;
> 
>         rc = fdt_setprop(fdt, nodeoffset, prop, val, size);
>         if (rc)
>                 goto err;
	goto ret;
> err:
>         debug("Problem setting %s = %s: %s\n", node, prop, fdt_strerror(rc));
ret:		// hey, we *CAN* program FORTRAN in C  ;-)
>         return rc;
> }

This is too simplistic for the known cases.
* You pass in a size, but can only handle integers (rc is int).  If the 
size is 6 bytes (MAC), Bad Things[tm] will happen.  If it is 1 or 2 
bytes, it probably will work, but is unclean type-wise.

* The rules are different for some properties: some are set-always and 
some are set only if it already exists (e.g. local-mac-address).

By the time you write enough "general" functions to cover all the cases 
currently covered, I suspect you will have the same number of functions 
as the current implementation.  If not, we have too many "setter" 
functions in the current implementation and they should be coalesced.  :-)

It is possible that I _did_ write more "setter" functions than 
necessary.  I plan to look at it, but have been busy. :-/  Adding LIBFDT 
support to other CPUs and factoring out the common routines would help 
with a review.

> static int fixup_int_prop(void *fdt, char *node, char *prop, u32 val)
> {
>         val = cpu_to_be32(val);
>         return ft_fixup_int_prop(fdt, node, prop, &val, sizeof(val));
> }
> 
> void
> ft_cpu_setup(void *blob, bd_t *bd)
> {
>         fixup_int_prop(blob, "/cpus/" OF_CPU, "timebase-frequency", OF_TBCLK);
>         fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->bi_busfreq);
>         fixup_int_prop(blob, "/cpus/" OF_CPU, "clock-frequency",
> bd->bi_intfreq);
>         fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->ipbfreq);
>         fixup_prop(blob, "/" OF_SOC "/ethernet at 3000", "mac-address",
>                    bd->bi_enetaddr, 6);
>         fixup_prop(blob, "/" OF_SOC "/ethernet at 3000", "local-mac-address",
>                    bd->bi_enetaddr, 6);
> }
> 
> This adds 2 helper functions instead of 5, and one of them is the same
> path for all of the fixups.  Drops the table approach entirely due to
> the problems with extracting values out of bd.  (Which is what I meant
> in the third part of my original comment)

I think 2 is too optimistic, but it is still possible this approach 
would be better than the current 5.  The above looks better, but I claim 
only because it is oversimplified - I contend the 2 helper functions 
will expand to 5 functions that basically are the current "setter" 
functions.  On the other hand, maybe not.  It is worth trying.

I thought a table approach would be more obvious than the #ifdef 
spaghetti that was in the original code, but I'm not all that wild about 
the resulting code.  In my implementation, with the table and the 
"setter" functions separated, it still results in obfuscation, 
especially where table entries and corresponding "setter" functions are 
#ifdefed. :-(  Note that the above proposal doesn't help a lot because 
the routine calling the new helper function and the helper functions are 
still separated.  :-/

Part of what I'm unhappy about is that I ended up with two sets of 
#ifdefs where there is conditional code: one in the table and one for 
the corresponding "setter" function.  The above proposal may help, but 
only if the "helper" functions are truly generalized and I am somewhat 
skeptical about 100% success.

> The 2 new helpers could also be generalized for use by all boards.

s/2/n/ (where n is probably 5) and I would agree 100%. :-/

> Cheers,
> g.

Best regards,
gvb