Outstanding patches

[Jordan Crouse]

I have a number of outstanding patches that need to be ACKed or 
NAKed.  Please review if you have the time:

coreinfo submenu patch:
http://www.coreboot.org/pipermail/coreboot/2008-April/033837.html

libpayload LAR patch:
I will repost this one with changes from Peter

LAR browser for coreinfo:
http://www.coreboot.org/pipermail/coreboot/2008-April/033904.html
This has been acked, but depends on the previous two patches

Update current time and date on the screen (coreinfo):
http://www.coreboot.org/pipermail/coreboot/2008-April/033905.html
This has been acked, but depends on the submenu patch

Thanks,
Jordan

[Jordan Crouse]

> libpayload LAR patch:
> I will repost this one with changes from Peter

As promised.

[Myles Watson]

On Tue, May 6, 2008 at 2:10 PM, Jordan Crouse  wrote:
> > libpayload LAR patch:
> > I will repost this one with changes from Peter
>
> As promised.

There's a typo in this one:
lal_header -> lar_header

Besides that it looks fine.  I'm not sure how many copies of lar.h we
want floating around.  Is there a way we could consolidate?

Thanks,
Myles

[Myles Watson]

On Tue, May 6, 2008 at 2:10 PM, Jordan Crouse  wrote:
> > libpayload LAR patch:
> > I will repost this one with changes from Peter
>
> As promised.

With the typo correction.

The lar browser is nice!

Acked-by: Myles Watson

Thanks,
Myles

[Jordan Crouse]

On 07/05/08 09:10 -0600, Myles Watson wrote:
> On Tue, May 6, 2008 at 2:10 PM, Jordan Crouse  wrote:
> > > libpayload LAR patch:
> > > I will repost this one with changes from Peter
> >
> > As promised.
>
> With the typo correction.
>
> The lar browser is nice!
>
> Acked-by: Myles Watson

r3288.  Thanks.

[Myles Watson]

> I have a number of outstanding patches that need to be ACKed or
> NAKed.  Please review if you have the time:
>
> coreinfo submenu patch:
> http://www.coreboot.org/pipermail/coreboot/2008-April/033837.html
> coreinfo:  Add "a submenu"

Here's a start for this one.

Thanks,
Myles
> We were in the risk of running out of space in the option menu at
> the bottom of the screen - this turns the function keys into
> "categories" and then list specific items as part of the category.
>
> Signed-off-by: Jordan Crouse 
> Index: coreinfo/coreinfo.c
> ===================================================================
> --- coreinfo.orig/coreinfo.c	2008-04-24 18:02:23.000000000 -0600
> +++ coreinfo/coreinfo.c	2008-04-25 10:08:02.000000000 -0600
> @@ -28,24 +28,46 @@
>  extern struct coreinfo_module nvram_module;
>  extern struct coreinfo_module bootlog_module;
>
> -struct coreinfo_module *modules[] = {
> +struct coreinfo_module *system_modules[] = {
>  #ifdef CONFIG_MODULE_CPUINFO
>    &cpuinfo_module,
>  #endif
>  #ifdef CONFIG_MODULE_PCI
>    &pci_module,
>  #endif
> -#ifdef CONFIG_MODULE_COREBOOT
> &coreboot_module,
>
> -#endif
>  #ifdef CONFIG_MODULE_NVRAM
>    &nvram_module,
>  #endif
> +};
>
> +struct coreinfo_module *coreboot_modules[] = {
> +#ifdef CONFIG_MODULE_COREBOOT
> &coreboot_module,
>
> +#endif
>  #ifdef CONFIG_MODULE_BOOTLOG
>    &bootlog_module,
>  #endif
>  };

What happens if I configure it not to have either of these two modules? It
hangs for me.  Maybe because you took out the check below?  Maybe there
needs to be a new check for empty categories or an ifdef that gets rid of
categories that have no subcategories.

It works fine for me when I'm not trying to break it. :)
> +struct coreinfo_cat {
> char name[15];
> int cur;
> int count;
> struct coreinfo_module **modules;
>
> +} categories[] = {
> {
> .name = "System",
> .modules = system_modules,
> .count = ARRAY_SIZE(system_modules),
> },
> {
> .name = "Coreboot",
> .modules = coreboot_modules,
> .count = ARRAY_SIZE(coreboot_modules),
> }
>
> +};
>
> static WINDOW *modwin;
>  static int curwin;
>
> @@ -62,6 +84,26 @@
>    	waddch(win, '\304');
>  }
>
> +static void print_submenu(struct coreinfo_cat *cat)
> +{
> int i, j;
> char menu[80];
> char *ptr = menu;
> wmove(stdscr, 22, 0);
> for (j = 0; j < SCREEN_X; j++)
> waddch(stdscr, ' ');
> if (!cat->count)
> return;
> for (i = 0; i < cat->count; i++)
> ptr += sprintf(ptr, "[%c: %s] ", 'A' + i,

cat->modules[i]->name);
> mvprintw(22, 0, menu);
>
> +}
>
> static void print_menu(void)
>  {
>    int i, j;
> @@ -73,11 +115,10 @@
>    for (j = 0; j < SCREEN_X; j++)
>    	waddch(stdscr, ' ');
> for (i = 0; i < ARRAY_SIZE(modules); i++)
> ptr += sprintf(ptr, "F%d: %s ", i + 1, modules[i]->name);
> for (i = 0; i < ARRAY_SIZE(categories); i++)
> ptr += sprintf(ptr, "F%d: %s ", i + 1, categories[i].name);
>
> if (ARRAY_SIZE(modules) != 0)
> mvprintw(23, 0, menu);
> mvprintw(23, 0, menu);

This check?
> #ifdef CONFIG_SHOW_DATE_TIME
>    mvprintw(23, 59, "%02d/%02d/20%02d - %02d:%02d:%02d",
> @@ -117,6 +158,8 @@
>
> ptr += sprintf(ptr, "[ %s ]", str);
> for (i = ((SCREEN_X - len) / 2) + len; i < SCREEN_X; i++)
>   ptr += sprintf(ptr, "=");
>
> @@ -124,16 +167,35 @@
>  }
>  #endif
>
> -static void redraw_module(void)
> +static void redraw_module(struct coreinfo_cat *cat)
>  {
> if (ARRAY_SIZE(modules) == 0)
>
> if (cat->count == 0)
> return;
>
> wclear(modwin);
> modules[curwin]->redraw(modwin);
> cat->modules[cat->cur]->redraw(modwin);
> refresh();
>
> }
>
> +static void handle_category_key(struct coreinfo_cat *cat, int key)
> +{
> if (key >= 'a' && key <= 'z') {
> int index = key - 'a';
> if (index < cat->count) {
> cat->cur = index;
> 	redraw_module(cat);
> 	return;
> }
> }
> if (cat->count && cat->modules[cat->cur]->handle) {
> if (cat->modules[cat->cur]->handle(key))
> 	redraw_module(cat);
> }
>
> +}
>
> static void loop(void)
>  {
>    int key;
> @@ -141,9 +203,8 @@
>    center(0, "coreinfo v0.1");
>
> print_menu();
> if (ARRAY_SIZE(modules) != 0)
> modules[curwin]->redraw(modwin);
> refresh();
> print_submenu(&categories[curwin]);
> redraw_module(&categories[curwin]);

Or this check?
> while (1) {
>    	key = getch();
> @@ -154,18 +215,18 @@
>    	if (key >= KEY_F(1) && key <= KEY_F(9)) {
>    		unsigned char ch = key - KEY_F(1);
> 	if (ch <= ARRAY_SIZE(modules)) {
> 		if (ch == ARRAY_SIZE(modules))
> 	if (ch <= ARRAY_SIZE(categories)) {
> 		if (ch == ARRAY_SIZE(categories))
> 		continue;
> 	curwin = ch;
> 		redraw_module();
> 		print_submenu(&categories[curwin]);
> 		redraw_module(&categories[curwin]);
> 	continue;
> }
> }
> if (ARRAY_SIZE(modules) != 0 && modules[curwin]->handle)
> 	if (modules[curwin]->handle(key))
> 		redraw_module();
> handle_category_key(&categories[curwin], key);
> }
>
> }
>
> @@ -182,7 +243,7 @@
>    init_pair(2, COLOR_BLACK, COLOR_WHITE);
>    init_pair(3, COLOR_WHITE, COLOR_WHITE);
> modwin = newwin(23, 80, 1, 0);
>
> modwin = newwin(22, 80, 1, 0);
>
> wattrset(stdscr, COLOR_PAIR(1) | A_BOLD);
> wattrset(modwin, COLOR_PAIR(2));
>
> @@ -196,8 +257,11 @@
>
> refresh();
> for (i = 0; i < ARRAY_SIZE(modules); i++)
> modules[i]->init();
>
> for (i = 0; i < ARRAY_SIZE(categories); i++) {
> for(j = 0; j < categories[i].count; j++)
> 	categories[i].modules[j]->init();
>
> }
>
> loop();

[Myles Watson]

On Tue, May 6, 2008 at 2:49 PM, Myles Watson  wrote:
> > I have a number of outstanding patches that need to be ACKed or
> > NAKed.  Please review if you have the time:
> >
> > coreinfo submenu patch:
> > http://www.coreboot.org/pipermail/coreboot/2008-April/033837.html
> > coreinfo:  Add "a submenu"
>
> Here's a start for this one.

I forgot to say  that if you configure it without a couple of menus,
it refuses to build because of unused variable warnings.  I'm not sure
that -Werror is the right thing to do when it's in buildrom, even
though I don't like warnings.

Thanks,
Myles

[Jordan Crouse]

On 06/05/08 14:59 -0600, Myles Watson wrote:
> On Tue, May 6, 2008 at 2:49 PM, Myles Watson  wrote:
> > > I have a number of outstanding patches that need to be ACKed or
> > > NAKed.  Please review if you have the time:
> > >
> > > coreinfo submenu patch:
> > > http://www.coreboot.org/pipermail/coreboot/2008-April/033837.html
> > > coreinfo:  Add "a submenu"
> >
> > Here's a start for this one.
>
> I forgot to say  that if you configure it without a couple of menus,
> it refuses to build because of unused variable warnings.  I'm not sure
> that -Werror is the right thing to do when it's in buildrom, even
> though I don't like warnings.

Unused functions are bad though - can you point me to the config you used?

Jordan

[Myles Watson]

> -----Original Message-----
> From: Jordan Crouse [mailto:[e-mail]]
> Sent: Tuesday, May 06, 2008 3:05 PM
> To: Myles Watson
> Cc: [e-mail]
> Subject: Re: Outstanding patches
>
> On 06/05/08 14:59 -0600, Myles Watson wrote:
> > On Tue, May 6, 2008 at 2:49 PM, Myles Watson  wrote:
> > > > I have a number of outstanding patches that need to be ACKed or
> > > > NAKed.  Please review if you have the time:
> > > >
> > > > coreinfo submenu patch:
> > > > http://www.coreboot.org/pipermail/coreboot/2008-April/033837.html
> > > > coreinfo:  Add "a submenu"
> > >
> > > Here's a start for this one.
> >
> > I forgot to say  that if you configure it without a couple of menus,
> > it refuses to build because of unused variable warnings.  I'm not sure
> > that -Werror is the right thing to do when it's in buildrom, even
> > though I don't like warnings.
>
> Unused functions are bad though - can you point me to the config you used?

I blew it away to test the lar patch...

I will try to recreate it, but it was a rdtsc function that wasn't being
used, if that helps.

Thanks,
Myles

[Jordan Crouse]

On 06/05/08 15:17 -0600, Myles Watson wrote:
> > -----Original Message-----
> > From: Jordan Crouse [mailto:[e-mail]]
> > Sent: Tuesday, May 06, 2008 3:05 PM
> > To: Myles Watson
> > Cc: [e-mail]
> > Subject: Re: Outstanding patches
> >
> > On 06/05/08 14:59 -0600, Myles Watson wrote:
> > > On Tue, May 6, 2008 at 2:49 PM, Myles Watson  wrote:
> > > > > I have a number of outstanding patches that need to be ACKed or
> > > > > NAKed.  Please review if you have the time:
> > > > >
> > > > > coreinfo submenu patch:
> > > > > http://www.coreboot.org/pipermail/coreboot/2008-April/033837.html
> > > > > coreinfo:  Add "a submenu"
> > > >
> > > > Here's a start for this one.
> > >
> > > I forgot to say  that if you configure it without a couple of menus,
> > > it refuses to build because of unused variable warnings.  I'm not sure
> > > that -Werror is the right thing to do when it's in buildrom, even
> > > though I don't like warnings.
> >
> > Unused functions are bad though - can you point me to the config you used?
>
> I blew it away to test the lar patch...
>
> I will try to recreate it, but it was a rdtsc function that wasn't being
> used, if that helps.

Ah, yes - somebody else talked about that too.  I'll fix that.  Thanks.

Jordan

[Jordan Crouse]

On 06/05/08 14:59 -0600, Myles Watson wrote:
> On Tue, May 6, 2008 at 2:49 PM, Myles Watson  wrote:
> > > I have a number of outstanding patches that need to be ACKed or
> > > NAKed.  Please review if you have the time:
> > >
> > > coreinfo submenu patch:
> > > http://www.coreboot.org/pipermail/coreboot/2008-April/033837.html
> > > coreinfo:  Add "a submenu"
> >
> > Here's a start for this one.
>
> I forgot to say  that if you configure it without a couple of menus,
> it refuses to build because of unused variable warnings.  I'm not sure
> that -Werror is the right thing to do when it's in buildrom, even
> though I don't like warnings.

Okay, this one was easy enough - the #include <arch/rdtsc.h> in
cpuinfo_module.c just had to be moved inside of the
#ifdef CONFIG_CPUINFO_MODULE define.   It is trivial.

Jordan

[Jordan Crouse]

On 06/05/08 14:49 -0600, Myles Watson wrote:
> > I have a number of outstanding patches that need to be ACKed or
> > NAKed.  Please review if you have the time:
> >
> > coreinfo submenu patch:
> > http://www.coreboot.org/pipermail/coreboot/2008-April/033837.html
> > coreinfo:  Add "a submenu"
>
> Here's a start for this one.
>
> Thanks,
> Myles
> > We were in the risk of running out of space in the option menu at
> > the bottom of the screen - this turns the function keys into
> > "categories" and then list specific items as part of the category.
> >
> > Signed-off-by: Jordan Crouse 
> > Index: coreinfo/coreinfo.c
> > ===================================================================
> > --- coreinfo.orig/coreinfo.c	2008-04-24 18:02:23.000000000 -0600
> > +++ coreinfo/coreinfo.c	2008-04-25 10:08:02.000000000 -0600
> > @@ -28,24 +28,46 @@
> >  extern struct coreinfo_module nvram_module;
> >  extern struct coreinfo_module bootlog_module;
> >
> > -struct coreinfo_module *modules[] = {
> > +struct coreinfo_module *system_modules[] = {
> >  #ifdef CONFIG_MODULE_CPUINFO
> >    &cpuinfo_module,
> >  #endif
> >  #ifdef CONFIG_MODULE_PCI
> >    &pci_module,
> >  #endif
> > -#ifdef CONFIG_MODULE_COREBOOT
> > &coreboot_module,
> >
> > -#endif
> >  #ifdef CONFIG_MODULE_NVRAM
> >    &nvram_module,
> >  #endif
> > +};
> >
> > +struct coreinfo_module *coreboot_modules[] = {
> > +#ifdef CONFIG_MODULE_COREBOOT
> > &coreboot_module,
> >
> > +#endif
> >  #ifdef CONFIG_MODULE_BOOTLOG
> >    &bootlog_module,
> >  #endif
> >  };
>
> What happens if I configure it not to have either of these two modules? It
> hangs for me.  Maybe because you took out the check below?  Maybe there
> needs to be a new check for empty categories or an ifdef that gets rid of
> categories that have no subcategories.

Yep - we are missing the check.  I'll add them back in.  Thanks.

Jordan

[Jordan Crouse]

On 06/05/08 14:49 -0600, Myles Watson wrote:
> > I have a number of outstanding patches that need to be ACKed or
> > NAKed.  Please review if you have the time:
> >
> > coreinfo submenu patch:
> > http://www.coreboot.org/pipermail/coreboot/2008-April/033837.html
> > coreinfo:  Add "a submenu"
>
> Here's a start for this one.
>
> Thanks,
> Myles
> > We were in the risk of running out of space in the option menu at
> > the bottom of the screen - this turns the function keys into
> > "categories" and then list specific items as part of the category.
> >
> > Signed-off-by: Jordan Crouse 
> > Index: coreinfo/coreinfo.c
> > ===================================================================
> > --- coreinfo.orig/coreinfo.c	2008-04-24 18:02:23.000000000 -0600
> > +++ coreinfo/coreinfo.c	2008-04-25 10:08:02.000000000 -0600
> > @@ -28,24 +28,46 @@
> >  extern struct coreinfo_module nvram_module;
> >  extern struct coreinfo_module bootlog_module;
> >
> > -struct coreinfo_module *modules[] = {
> > +struct coreinfo_module *system_modules[] = {
> >  #ifdef CONFIG_MODULE_CPUINFO
> >    &cpuinfo_module,
> >  #endif
> >  #ifdef CONFIG_MODULE_PCI
> >    &pci_module,
> >  #endif
> > -#ifdef CONFIG_MODULE_COREBOOT
> > &coreboot_module,
> >
> > -#endif
> >  #ifdef CONFIG_MODULE_NVRAM
> >    &nvram_module,
> >  #endif
> > +};
> >
> > +struct coreinfo_module *coreboot_modules[] = {
> > +#ifdef CONFIG_MODULE_COREBOOT
> > &coreboot_module,
> >
> > +#endif
> >  #ifdef CONFIG_MODULE_BOOTLOG
> >    &bootlog_module,
> >  #endif
> >  };
>
> What happens if I configure it not to have either of these two modules? It
> hangs for me.  Maybe because you took out the check below?  Maybe there
> needs to be a new check for empty categories or an ifdef that gets rid of
> categories that have no subcategories.

New patch attached that checks for empty categories.  Tested on qemu.

Jordan

[Myles Watson]

> > What happens if I configure it not to have either of these two modules?
>
> It
> > hangs for me.  Maybe because you took out the check below?  Maybe there
> > needs to be a new check for empty categories or an ifdef that gets rid
>
> of
> > categories that have no subcategories.
>
> New patch attached that checks for empty categories.  Tested on qemu.

Acked-by: Myles Watson