* [U-Boot-Users] [PATCH] of dump command
@ 2006-10-30 11:39 Jerry Van Baren
2006-10-30 17:43 ` Grant Likely
0 siblings, 1 reply; 4+ messages in thread
From: Jerry Van Baren @ 2006-10-30 11:39 UTC (permalink / raw)
To: u-boot
See attached patch or get from the git repo:
<http://www.cideas.us/cgi-bin/gitweb.cgi?p=u-boot/u-boot-of-cmd.git;a=summary>
This should apply cleanly to the head of the denx.de git repository and
works in conjunction with Jon Loeliger and others OF improvements.
Note that I've extended the command enable/disable mechanism by simply
adding a '1' after CONFIG/CFG, creating CONFIG1_COMMANDS and
CFG1_CMD_OF. This seemed to be the simplest way of extending the
command selection.
Signed-off-by: Jerry Van Baren <vanbaren@cideas.com>
Best regards,
gvb
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 061028-of-cmd.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20061030/d894e4b6/attachment.txt
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot-Users] [PATCH] of dump command
2006-10-30 11:39 [U-Boot-Users] [PATCH] of dump command Jerry Van Baren
@ 2006-10-30 17:43 ` Grant Likely
2006-10-30 22:22 ` Wolfgang Denk
0 siblings, 1 reply; 4+ messages in thread
From: Grant Likely @ 2006-10-30 17:43 UTC (permalink / raw)
To: u-boot
On 10/30/06, Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
> commit e57c182cdbd382e07338cfb395d8f4b98772913f
> Author: Jerry Van Baren <vanbaren@cideas.com>
> Date: Sat Oct 28 12:17:52 2006 -0400
>
> Add a "of" command to dump the flattened open firmware tree.
>
> Enhanced the ft_build tree dump to allow dumping from a variable instead
> of always dumping the whole tree. Also fixed some leading zero formatting
> and printing 8 byte variables when 64 bit vsprintf is not supported.
>
> Enhance vsprintf to handle "ll" long long specifier (it already supported
> the deprecated "q" specifier).
>
> Expanded the command configuration into another long long flag word
> since the command flag is (almost) out of bits.
These should probably be split up into separate patches. One for the
vsprintf 'll' support; one for the new command and one for the
MPC8260ADS config changes.
> diff --git a/common/Makefile b/common/Makefile
> index 07ddc95..4d883f6 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -37,7 +37,7 @@ COBJS = main.o ACEX1K.o altera.o bedbug.
> cmd_i2c.o cmd_ide.o cmd_immap.o cmd_itest.o cmd_jffs2.o \
> cmd_load.o cmd_log.o \
> cmd_mem.o cmd_mii.o cmd_misc.o cmd_mmc.o \
> - cmd_nand.o cmd_net.o cmd_nvedit.o \
> + cmd_nand.o cmd_net.o cmd_nvedit.o cmd_of.o \
> cmd_pci.o cmd_pcmcia.o cmd_portio.o \
> cmd_reginfo.o cmd_reiser.o cmd_scsi.o cmd_spi.o cmd_universe.o \
> cmd_usb.o cmd_vfd.o \
> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index 3091a58..58f50bc 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -950,7 +950,7 @@ #else /* CONFIG_OF_FLAT_TREE */
> }
>
> ft_setup(of_flat_tree, kbd, initrd_start, initrd_end);
> - /* ft_dump_blob(of_flat_tree); */
> + /* ft_dump_blob(of_flat_tree, "/"); */
This is dead code; just remove it; it's been replaced by a calls to
ft_dump_blob() in ft_setup().
>
> #if defined(CFG_INIT_RAM_LOCK) && !defined(CONFIG_E500)
> unlock_ram_in_cache();
> @@ -968,7 +968,7 @@ #endif
> cmd_start, cmd_end);
> else {
> ft_setup(of_flat_tree, kbd, initrd_start, initrd_end);
> - /* ft_dump_blob(of_flat_tree); */
> + /* ft_dump_blob(of_flat_tree, "/"); */
ditto
> (*kernel) ((bd_t *)of_flat_tree, (ulong)kernel, 0, 0, 0);
> }
> #endif /* CONFIG_OF_FLAT_TREE */
> diff --git a/common/cmd_of.c b/common/cmd_of.c
> new file mode 100644
> index 0000000..0f7973f
> --- /dev/null
> +++ b/common/cmd_of.c
> @@ -0,0 +1,58 @@
> +/*
> + * (C) Copyright 2006
> + * Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +/*
> + * Misc functions
> + */
> +#include <common.h>
> +#include <command.h>
> +#include <ft_build.h>
> +
> +#if (CONFIG1_COMMANDS & CFG1_CMD_OF)
This is not a lot of code; and it's pretty useful stuff; maybe just
make it automatically built when CONFIG_OF_FLAT_TREE is defined?
(Rather than adding a new CONFIG_COMMANDS macro at the moment)
If a new CFG_CMD_ macro set is needed; I wouldn't do it now. There is
still 1 bit left. :) If it needs to be changed, it might make sense
to audit the full list and see if there is anything that should be
removed.
Wolfgang needs to weigh in on this one.
> +
> +int do_of (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> + void *p = (void *)simple_strtoul(argv[1], NULL, 16);
> +
> + if (argc < 2) {
> + printf ("Usage:\n%s\n", cmdtp->usage);
> + return 1;
> + } else if (argc == 2) {
> + ft_dump_blob(p, "/");
> + } else {
> + ft_dump_blob(p, argv[2]);
> + }
> +
> + return 0;
> +}
> +
> +
> +U_BOOT_CMD(
> + of, 3, 0, do_of,
> + "of - Open Firmware utility commands\n",
> + "of <addr> - Dump the address as an OF tree\n"
> + "of <addr> <prop> - Dump the given property\n"
> +);
I think 'of' is a bit terse. Maybe "ofinfo", "ofdump", or "ftinfo"?
> +
> +#endif /* CFG1_CMD_OF */
> +
> diff --git a/common/ft_build.c b/common/ft_build.c
> index 980e40f..a752b5e 100644
> --- a/common/ft_build.c
> +++ b/common/ft_build.c
> @@ -233,10 +233,19 @@ static void print_data(const void *data,
> printf(" = <%04x>", be16_to_cpu(*(u16 *) data) & 0xffff);
> break;
> case 4: /* word */
> - printf(" = <%x>", be32_to_cpu(*(u32 *) data) & 0xffffffffU);
> + printf(" = <%08x>", be32_to_cpu(*(u32 *) data) & 0xffffffffU);
> break;
> case 8: /* double-word */
> - printf(" = <%qx>", be64_to_cpu(*(uint64_t *) data));
> + /*
> + * If 64 bit printing is supported, use it.
> + */
> +#ifdef CFG_64BIT_VSPRINTF
> + printf(" = <%016llx>", be64_to_cpu(*(uint64_t *) data));
> +#else
> + printf(" = <%08x", be32_to_cpu(*(u32 *) data) & 0xffffffffU);
> + data += sizeof(u32);
> + printf(" %08x>", be32_to_cpu(*(u32 *) data) & 0xffffffffU);
> +#endif
> break;
> default: /* anything else... hexdump */
> printf(" = [");
> @@ -248,7 +257,12 @@ static void print_data(const void *data,
> }
> }
>
> -void ft_dump_blob(const void *bphp)
> +/*
> + * Used by ft_dump_blob() and ft_get_prop() to build up property names
> + */
> +static char path[256], prop[256];
Is moving these out of ft_get_prop truly necessary? (I haven't dug
into the full context, but seeing this makes me scared)
> +
> +void ft_dump_blob(const void *bphp, const char *propname)
> {
> const struct boot_param_header *bph = bphp;
> const uint64_t *p_rsvmap = (const uint64_t *)
> @@ -260,7 +274,9 @@ void ft_dump_blob(const void *bphp)
> u32 tag;
> const u32 *p;
> const char *s, *t;
> + char *ss;
> int depth, sz, shift;
> + int found;
> int i;
> uint64_t addr, size;
>
> @@ -271,6 +287,8 @@ void ft_dump_blob(const void *bphp)
>
> depth = 0;
> shift = 4;
> + found = 0;
> + strcpy(path, "/");
>
> for (i = 0;; i++) {
> addr = be64_to_cpu(p_rsvmap[i * 2]);
> @@ -290,7 +308,17 @@ void ft_dump_blob(const void *bphp)
> s = (const char *)p;
> p = (u32 *) _ALIGN((unsigned long)p + strlen(s) + 1, 4);
>
> - printf("%*s%s {\n", depth * shift, "", s);
> + strcat(path, s);
> + if (found || (strcmp(path, propname) == 0)) {
> + found++;
> + if (depth == 0)
> + printf("/ {\n");
> + else
> + printf("%*s%s {\n", depth * shift, "", s);
> + }
> + /* The root path is already there as "/" */
> + if(depth != 0)
> + strcat(path, "/");
>
> depth++;
> continue;
> @@ -299,7 +327,16 @@ void ft_dump_blob(const void *bphp)
> if (tag == OF_DT_END_NODE) {
> depth--;
>
> - printf("%*s};\n", depth * shift, "");
> + path[strlen(path) - 1] = '\0';
> + ss = strrchr(path, '/');
> + if (ss != NULL)
> + ss[1] = '\0';
> +
> + if(found) {
> + printf("%*s};\n", depth * shift, "");
> + if (found-- == 0)
> + return; /* request done */
> + }
> continue;
> }
>
> @@ -317,9 +354,15 @@ void ft_dump_blob(const void *bphp)
> s = (const char *)p_strings + be32_to_cpu(*p++);
> t = (const char *)p;
> p = (const u32 *)_ALIGN((unsigned long)p + sz, 4);
> - printf("%*s%s", depth * shift, "", s);
> - print_data(t, sz);
> - printf(";\n");
> +
> + strcpy(prop, path);
> + strcat(prop, s);
> +
> + if(found || (strcmp(prop, propname) == 0)) {
> + printf("%*s%s", depth * shift, "", s);
> + print_data(t, sz);
> + printf(";\n");
> + }
> }
> }
Hmm, I'm not all that thrilled with this approach; but I understand
that some of it is driven by the current design of the ft_dump_blob
code. I'd rather have a block at the top which 'seeks' to the correct
point in the device tree and sets the depth value to zero. Then start
printing the tree until depth gets back to zero.
>
> @@ -349,7 +392,6 @@ void *ft_get_prop(void *bphp, const char
> char *s, *t;
> char *ss;
> int sz;
> - static char path[256], prop[256];
>
> path[0] = '\0';
>
> @@ -478,7 +520,7 @@ #endif
>
> #ifdef DEBUG
> printf ("recieved oftree\n");
> - ft_dump_blob(blob);
> + ft_dump_blob(blob, "/");
> #endif
>
> ft_init_cxt(&cxt, blob);
> @@ -585,7 +627,7 @@ #endif
>
> #ifdef DEBUG
> printf("final OF-tree\n");
> - ft_dump_blob(blob);
> + ft_dump_blob(blob, "/");
> #endif
> }
> #endif
> diff --git a/include/cmd_confdefs.h b/include/cmd_confdefs.h
> index cf36583..52b873d 100644
> --- a/include/cmd_confdefs.h
> +++ b/include/cmd_confdefs.h
> @@ -94,8 +94,13 @@ #define CFG_CMD_UNIVERSE 0x0800000000000
> #define CFG_CMD_EXT2 0x1000000000000000ULL /* EXT2 Support */
> #define CFG_CMD_SNTP 0x2000000000000000ULL /* SNTP support */
> #define CFG_CMD_DISPLAY 0x4000000000000000ULL /* Display support */
> +/*
> + * Start a second level of commands...
> + */
> +#define CFG1_CMD_OF 0x0000000000000001ULL /* Open Firmware dump support */
There's still one more bit left in CFG_CMD_ALL; just use it! :)
Or eliminate CFG_CMD_OF entirely.
> diff --git a/include/configs/MPC8260ADS.h b/include/configs/MPC8260ADS.h
> index 6195bca..55c6ff9 100644
> --- a/include/configs/MPC8260ADS.h
> +++ b/include/configs/MPC8260ADS.h
> @@ -237,6 +237,7 @@ #define CONFIG_COMMANDS (CFG_CMD_ALL &
> CFG_CMD_I2C | \
> CFG_CMD_PCI | \
> CFG_EXCLUDE ) )
> +#define CONFIG1_COMMANDS (CFG1_CMD_OF)
> #else
> #define CONFIG_COMMANDS (CFG_CMD_ALL & ~( \
> CMD_CFG_PCI | \
> @@ -506,4 +507,15 @@ #if CONFIG_ADSTYPE == CFG_8272ADS
> #define CONFIG_HAS_ETH1
> #endif
>
> +/* pass open firmware flat tree */
> +#define CONFIG_OF_FLAT_TREE 1
> +
> +/* maximum size of the flat tree (8K) */
> +#define OF_FLAT_TREE_MAX_SIZE 8192
> +
> +#define OF_CPU "PowerPC,MPC8272 at 0"
> +#define OF_TBCLK (CONFIG_8260_CLKIN / 4)
> +#define CONFIG_OF_HAS_BD_T 1
> +#define CONFIG_OF_HAS_UBOOT_ENV 1
> +
> #endif /* __CONFIG_H */
All of this should be in a separate patch
> diff --git a/include/ft_build.h b/include/ft_build.h
> index 89c689c..5530122 100644
> --- a/include/ft_build.h
> +++ b/include/ft_build.h
> @@ -58,7 +58,7 @@ void ft_add_rsvmap(struct ft_cxt *cxt, u
>
> void ft_setup(void *blob, bd_t * bd, ulong initrd_start, ulong initrd_end);
>
> -void ft_dump_blob(const void *bphp);
> +void ft_dump_blob(const void *bphp, const char *propname);
> void ft_merge_blob(struct ft_cxt *cxt, void *blob);
> void *ft_get_prop(void *bphp, const char *propname, int *szp);
>
> diff --git a/lib_generic/vsprintf.c b/lib_generic/vsprintf.c
> index 2740f2e..034c619 100644
> --- a/lib_generic/vsprintf.c
> +++ b/lib_generic/vsprintf.c
> @@ -256,6 +256,13 @@ #endif
> if (*fmt == 'h' || *fmt == 'l' || *fmt == 'q') {
> qualifier = *fmt;
> ++fmt;
> +#ifdef CFG_64BIT_VSPRINTF
> + /* parse ll (64 bit) and change to 'q' */
> + if (*fmt == 'l') {
> + qualifier = 'q';
> + ++fmt;
> + }
> +#endif
ditto
--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot-Users] [PATCH] of dump command
2006-10-30 17:43 ` Grant Likely
@ 2006-10-30 22:22 ` Wolfgang Denk
2006-10-31 18:41 ` Jerry Van Baren
0 siblings, 1 reply; 4+ messages in thread
From: Wolfgang Denk @ 2006-10-30 22:22 UTC (permalink / raw)
To: u-boot
Dear Grant,
thanks a lot for your thorough comments.
In message <528646bc0610300943j5752c2c7i9b66864ec678a86e@mail.gmail.com> you wrote:
>
> These should probably be split up into separate patches. One for the
> vsprintf 'll' support; one for the new command and one for the
> MPC8260ADS config changes.
Agreed. Eventually these are separate commits in Jerry's repo anyway.
Jerry?
> This is not a lot of code; and it's pretty useful stuff; maybe just
> make it automatically built when CONFIG_OF_FLAT_TREE is defined?
> (Rather than adding a new CONFIG_COMMANDS macro at the moment)
Agreed.
> If a new CFG_CMD_ macro set is needed; I wouldn't do it now. There is
> still 1 bit left. :) If it needs to be changed, it might make sense
> to audit the full list and see if there is anything that should be
> removed.
>
> Wolfgang needs to weigh in on this one.
I think we need a major cleanup of the config system, so I would like
to touch as few things in this area as possible. Here, I agree with
Grant that this should be just compiled in when OFT support is
enabled. Probably everybody will want this.
> > +U_BOOT_CMD(
> > + of, 3, 0, do_of,
> > + "of - Open Firmware utility commands\n",
> > + "of <addr> - Dump the address as an OF tree\n"
> > + "of <addr> <prop> - Dump the given property\n"
> > +);
>
> I think 'of' is a bit terse. Maybe "ofinfo", "ofdump", or "ftinfo"?
Additionally, I think that "of" [= Open Firmware] is wrong. It should
be "oft" [Open Firmware Tree] at least. The command should IMHO be
named "oftdump", but "ftdump" [FT = Flat Tree] would be ok, too.
> > + * Used by ft_dump_blob() and ft_get_prop() to build up property names
> > + */
> > +static char path[256], prop[256];
>
> Is moving these out of ft_get_prop truly necessary? (I haven't dug
> into the full context, but seeing this makes me scared)
Me too :-)
> There's still one more bit left in CFG_CMD_ALL; just use it! :)
>
> Or eliminate CFG_CMD_OF entirely.
That's what I'd suggest.
Thanks again, to both of you.
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Is there a way to determine Yesterday's date using Unix utilities?
echo "what is yesterday's date?" | /bin/mail root
-- Randal L. Schwartz in <ukbuh2y982.fsf@julie.teleport.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot-Users] [PATCH] of dump command
2006-10-30 22:22 ` Wolfgang Denk
@ 2006-10-31 18:41 ` Jerry Van Baren
0 siblings, 0 replies; 4+ messages in thread
From: Jerry Van Baren @ 2006-10-31 18:41 UTC (permalink / raw)
To: u-boot
Wow, you guys are _tough_! I'm glad you didn't grade me for CompSci 101
:-)
Three patches will follow
* vsprintf "ll" extension (pretty trivial)
* oftdump command (da meat)
* PQ2FADS changes to use the flattened OF tree (pretty trivial)
More comments embedded...
Wolfgang Denk wrote:
> Dear Grant,
>
> thanks a lot for your thorough comments.
>
> In message <528646bc0610300943j5752c2c7i9b66864ec678a86e@mail.gmail.com> you wrote:
>> These should probably be split up into separate patches. One for the
>> vsprintf 'll' support; one for the new command and one for the
>> MPC8260ADS config changes.
>
> Agreed. Eventually these are separate commits in Jerry's repo anyway.
> Jerry?
Done, patch will be submitted.
>> This is not a lot of code; and it's pretty useful stuff; maybe just
>> make it automatically built when CONFIG_OF_FLAT_TREE is defined?
>> (Rather than adding a new CONFIG_COMMANDS macro at the moment)
>
> Agreed.
Done, patch will be submitted.
>> If a new CFG_CMD_ macro set is needed; I wouldn't do it now. There is
>> still 1 bit left. :) If it needs to be changed, it might make sense
>> to audit the full list and see if there is anything that should be
>> removed.
>>
>> Wolfgang needs to weigh in on this one.
>
> I think we need a major cleanup of the config system, so I would like
> to touch as few things in this area as possible. Here, I agree with
> Grant that this should be just compiled in when OFT support is
> enabled. Probably everybody will want this.
CONFIG1_* and CFG1_CMD_* have been removed.
>>> +U_BOOT_CMD(
>>> + of, 3, 0, do_of,
>>> + "of - Open Firmware utility commands\n",
>>> + "of <addr> - Dump the address as an OF tree\n"
>>> + "of <addr> <prop> - Dump the given property\n"
>>> +);
>> I think 'of' is a bit terse. Maybe "ofinfo", "ofdump", or "ftinfo"?
>
> Additionally, I think that "of" [= Open Firmware] is wrong. It should
> be "oft" [Open Firmware Tree] at least. The command should IMHO be
> named "oftdump", but "ftdump" [FT = Flat Tree] would be ok, too.
OK, I renamed it "oftdump" (but, thanks to the beauty of Wolfgang's
minimum unique characters parsing, "of" still works ;-).
>>> + * Used by ft_dump_blob() and ft_get_prop() to build up property names
>>> + */
>>> +static char path[256], prop[256];
>> Is moving these out of ft_get_prop truly necessary? (I haven't dug
>> into the full context, but seeing this makes me scared)
>
> Me too :-)
Yes, this is ugly. The existing ft_get_prop() had the two declarations
as statics within the function declaration and I simply moved them out
so they could be re-used by ft_dump_blob(). Note that they remain
static, but they are available to ft_dump_blob() as well as
ft_get_prop() now. One alternative is to duplicate the static
declarations in both routines, but that seems extremely silly.
(I'm also not wild about the fact that the buffer lengths are fixed and
unchecked, so it is a buffer overflow vulnerability - I figure (a) I
just used existing code and (b) simply having access to u-boot is more
of a security issue than buffer overflows in u-boot.)
The problem is that the flattened OF tree only has the incremental
path/property names rather than the full name on each property (I
believe this was a version 0x10 optimization). The easiest way to match
the path/property is to build up (as you nest in) and back up (as you
unnest) the path and then do a string compare of the path/property
built-up string against the desired string. This is what the existing
ft_get_prop() did and what I extended ft_dump_blob() to do too.
I looked at least twice at doing a more clever matching and threw my
changes away because I ended up going down a blind alley. Keeping track
of the nesting and unnesting while following the tree takes some
non-trivial logic. I'm not saying that there _isn't_ a better way, but
building up the string and doing a string compare is _much_ easier and
quite likely takes less code than being clever.
>> There's still one more bit left in CFG_CMD_ALL; just use it! :)
>>
>> Or eliminate CFG_CMD_OF entirely.
>
> That's what I'd suggest.
Done.
> Thanks again, to both of you.
>
> Best regards,
>
> Wolfgang Denk
Patches to follow...
Best regards,
gvb
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-10-31 18:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-10-30 11:39 [U-Boot-Users] [PATCH] of dump command Jerry Van Baren
2006-10-30 17:43 ` Grant Likely
2006-10-30 22:22 ` Wolfgang Denk
2006-10-31 18:41 ` Jerry Van Baren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox