| Message ID | 1319063459-4804-5-git-send-email-dianders@chromium.org |
|---|---|
| State | Rejected |
| Headers | show |
On Wednesday 19 October 2011 18:30:59 Doug Anderson wrote: > + /* Add in earlyprintk */ > + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 earlyprintk"; > + expected_str = "root=/dev/mmcblk0p3 console="; *choke* wtf just happened here ? -mike
On Wed, Oct 19, 2011 at 3:35 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Wednesday 19 October 2011 18:30:59 Doug Anderson wrote: > > + /* Add in earlyprintk */ > > + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 > earlyprintk"; > > + expected_str = "root=/dev/mmcblk0p3 console="; > > *choke* wtf just happened here ? > Can you clarify what you're asking? Are you asking about fixup_silent_linux()? That's a function that already exists in u-boot. Its goal is to modify the command line to change the "console=blah" argument to a "console=", effectively telling Linux not to have a console. This particular patchset is extending fixup_silent_linux() to also remove "earlyprintk" from the command line, since the console isn't really silent unless you do that. ...or are you asking about the bit of unittest code to test that the stripping of "earlyprintk" really works? I was definitely not certain about whether to add the unittests in here (since there's no way to run them). My hope was that at some point in time there'd be a unit test infrastructure in u-boot and my unit tests could then be incorporated. If you'd rather not see the unit tests at all, I'm happy to strip them out. Maybe a GUI diff showing this change would help? If so, you can see it here: http://gerrit.chromium.org/gerrit/8822 ...that is where I got early reviews of this change before sending it to the list. Thanks! -Doug
On Wednesday 19 October 2011 18:46:20 Doug Anderson wrote: > On Wed, Oct 19, 2011 at 3:35 PM, Mike Frysinger <vapier@gentoo.org> wrote: > > On Wednesday 19 October 2011 18:30:59 Doug Anderson wrote: > > > + /* Add in earlyprintk */ > > > + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 > > > > earlyprintk"; > > > > > + expected_str = "root=/dev/mmcblk0p3 console="; > > > > *choke* wtf just happened here ? > > Can you clarify what you're asking? oh, you're updating the internal test code. i missed that this existed, so i thought you were blindly adding these strings for everyone. -mike
Dear Doug Anderson, In message <1319063459-4804-5-git-send-email-dianders@chromium.org> you wrote: > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > common/cmd_bootm.c | 10 +++++++++- > 1 files changed, 9 insertions(+), 1 deletions(-) As before, I see very little (actually, none at all) need for such function. Things like that should be handled usign environment variables in a clever way. Please explain why you think this code is needed. Best regards, Wolfgang Denk
Wolfgang, On Thu, Oct 20, 2011 at 7:42 AM, Wolfgang Denk <wd@denx.de> wrote: > As before, I see very little (actually, none at all) need for such > function. Things like that should be handled usign environment > variables in a clever way. > > Please explain why you think this code is needed. > I'm not sure I understand your comment. It sounds to me like you're saying that fixup_silent_linux() (which already exists in u-boot code) shouldn't be needed anymore in u-boot. ...and maybe you're considering it deprecated? Would you like me to submit a patch to remove it? It appears that fixup_silent_linux() was originally added by you in 2003 (f72da3406bf6f1c1bce9aa03b07d070413a916af). ...or are you saying that you don't see the need to change fixup_silent_linux() to also remove "earlyprintk"? Thanks! -Doug
Dear Doug Anderson, In message <CAD=FV=Ueawx_8Pw8bdni2BPbHP1p-XjsoURmRZr-1QvQ3YXd-A@mail.gmail.com> you wrote: > > I'm not sure I understand your comment. It sounds to me like you're saying > that fixup_silent_linux() (which already exists in u-boot code) shouldn't be > needed anymore in u-boot. ...and maybe you're considering it deprecated? I consider at least the way how it was done deprecated. > Would you like me to submit a patch to remove it? There are boards that use it. If you remove it, you must provide some replacement so these boards don;t break. > It appears that fixup_silent_linux() was originally added by you in 2003 > (f72da3406bf6f1c1bce9aa03b07d070413a916af). 2003... heh. By then, life was pretty much unregulated, and you could get about any code in ;-) > ...or are you saying that you don't see the need to > change fixup_silent_linux() to also remove "earlyprintk"? I think all this code is more or less dead (however I don't know who might actually actively use or even depend on it). fixup_silent_linux() was intended to remove only the "console=" argument from the kernel command line, and actually only the firrst of it if there should be several. At the time this code was written, that was only used on PPC, and the 256 byte buffer size was also the hardwired limit for the cmdline in that time's Linux kernels - so when written everything was correct (though bound to break as soon as Linux allows for longer cmdline args). If you think it would be nice to be able to perform special operations (like general substitution) on U-Boot environment variables, this should be written as a separate command that can be run from the command line, and that can be applied to all variables - not a hardwired special-cased construct for bootargs only. We can then make this command optional, and then we can even remove fixup_silent_linux() and replace it by calls to that code for the boards that need it. Best regards, Wolfgang Denk
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index f426e2f..c259bfb 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1203,7 +1203,7 @@ U_BOOT_CMD( #ifdef CONFIG_SILENT_CONSOLE /** - * Remove "console=blah" and from cmdline, replace w/ "console=". + * Remove "console=blah" and "earlyprintk" from cmdline, replace w/ "console=". * * This has the effect of telling Linux that we'd like it to have a silent * console. @@ -1241,6 +1241,7 @@ static char *do_fixup_silent_linux(const char *cmdline) strcpy(buf, cmdline); do { did_remove = remove_cmdline_param(buf, "console"); + did_remove |= remove_cmdline_param(buf, "earlyprintk"); } while (did_remove); add_cmdline_param(buf, "console=", bufsize); @@ -1313,6 +1314,13 @@ void do_fixup_silent_linux_unittest(void) assert(strcmp(result, expected_str) == 0); free(result); + /* Add in earlyprintk */ + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 earlyprintk"; + expected_str = "root=/dev/mmcblk0p3 console="; + result = do_fixup_silent_linux(original_str); + assert(strcmp(result, expected_str) == 0); + free(result); + debug("do_fixup_silent_linux_unittest: pass\n"); } #endif /* RUN_UNITTESTS */
Signed-off-by: Doug Anderson <dianders@chromium.org> --- common/cmd_bootm.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)