[coreboot] libpayload: rework console interfaces

Jordan Crouse jordan.crouse at amd.com
Fri Oct 17 16:50:30 CEST 2008


On 17/10/08 13:25 +0200, Patrick Georgi wrote:
> Hi,
> 
> attached patch removes most of the #ifdefs in libc/console.c, and
> replaces it with two queues (input, output) where drivers (serial,
> keyboard, video, usb) can attach.
> The only things left with #ifdefs are initialization (at some point the
> drivers must get a chance to register) and usb's polling, which should
> end up in most tight loops. Maybe there's a better way for usb poll.

How does poll differ from havechar?  Should we add a poll method to
the console drivers that is just null on most systems?

> I didn't use linker tricks (separate section for the device structs,
> iterating over that, etc) as I think that would have to be included in
> the build system of libpayload users. They should have it simple, stupid.

Yeah, I don't think linker tricks will buy us anything.  One thing that
concerns me is ordering.  The input should go from least reliable to 
most reliable (ie - from USB to port 60/64).  The way you have organized
it enforces that ordering.  Linker tricks wouldn't.

> Comments, critique and ideas for improvements welcome.

I like this design.  My thought was to use a static array of
structs in console.c.  I like your design better because it allows
for us to completely skip over an input method if it fails 
init.  I struggled with this very problem yesterday on a system 
without a superio - port 60/64 hangs.  My solution was to do a
"does the controller exist" check in every function - your changes
makes that only nessesary in the the init function.

> Signed-off-by: Patrick Georgi <patrick.georgi at coresystems.de>

Comments below.

> === libc/console.c
> ==================================================================
> --- libc/console.c	(revision 2218)
> +++ libc/console.c	(local)
> @@ -31,6 +31,21 @@
>  #include <libpayload.h>
>  #include <usb/usb.h>
>  
> +console_output_driver *console_out = 0;
> +console_input_driver *console_in = 0;

If not asssigned, gcc will automatically put these variables 
in the .bss section.  If they are assigned (even to zero), some
compilers will put them in the .data section increasing the size
of the binary.  Just let them float free.. :)

> +void console_add_output_driver(console_output_driver *out)
> +{
> +	out->next = console_out;
> +	console_out = out;
> +}
> +
> +void console_add_input_driver(console_input_driver *in)
> +{
> +	in->next = console_in;
> +	console_in = in;
> +}
> +
>  void console_init(void)
>  {
>  #ifdef CONFIG_VIDEO_CONSOLE
> @@ -46,15 +61,14 @@
>  
>  static void device_putchar(unsigned char c)
>  {
> -#ifdef CONFIG_VIDEO_CONSOLE
> -	video_console_putchar(0x700| c);
> -#endif
> -#ifdef CONFIG_SERIAL_CONSOLE
> -	serial_putchar(c);
> -#endif
> +	console_output_driver *out = console_out;
> +	while (out != 0) {
> +		out->putchar(c);
> +		out = out->next;

for (out = console_out; out != out; out = out->next)
	out->putchar(c);

I'm just picking nits - either way is acceptable for
sure.

> +	}
>  }
>  
> -int putchar(int c)
> +int putchar(unsigned int c)
>  {
>  	c &= 0xff;
>  	if (c == '\n')
> @@ -78,19 +92,15 @@
>  
>  int havekey(void)
>  {
> -#ifdef CONFIG_USB_HID
> +#ifdef CONFIG_USB
>  	usb_poll();
>
> -	if (usbhid_havechar())
> -		return 1;
>  #endif
> -#ifdef CONFIG_SERIAL_CONSOLE
> -	if (serial_havechar())
> -		return 1;
> -#endif
> -#ifdef CONFIG_PC_KEYBOARD
> -	if (keyboard_havechar())
> -		return 1;
> -#endif
> +	console_input_driver *in = console_in;
> +	while (in != 0) {
> +		if (in->havekey())
> +			return 1;
> +		in = in->next;
> +	}
>  	return 0;
>  }
>  
> @@ -101,19 +111,15 @@
>  int getchar(void)
>  {
>  	while (1) {
> -#ifdef CONFIG_USB_HID
> +#ifdef CONFIG_USB
>  		usb_poll();
> -		if (usbhid_havechar())
> -			return usbhid_getchar();
>  #endif
> -#ifdef CONFIG_SERIAL_CONSOLE
> -		if (serial_havechar())
> -			return serial_getchar();
> -#endif
> -#ifdef CONFIG_PC_KEYBOARD
> -		if (keyboard_havechar())
> -			return keyboard_getchar();
> -#endif
> +		console_input_driver *in = console_in;
> +		while (in != 0) {
> +			if (in->havechar())
> +				return in->getchar();
> +			in = in->next;
> +		}
>  	}
>  }
>  
> === include/libpayload.h
> ==================================================================
> --- include/libpayload.h	(revision 2218)
> +++ include/libpayload.h	(local)
> @@ -143,7 +143,7 @@
>   */
>  void serial_init(void);
>  void serial_hardware_init(int port, int speed, int word_bits, int parity, int stop_bits);
> -void serial_putchar(unsigned char c);
> +void serial_putchar(unsigned int c);
>  int serial_havechar(void);
>  int serial_getchar(void);
>  void serial_clear(void);
> @@ -190,7 +190,7 @@
>   * @{
>   */
>  void console_init(void);
> -int putchar(int c);
> +int putchar(unsigned int c);
>  int puts(const char *s);
>  int havekey(void);
>  int getchar(void);
> @@ -198,6 +198,22 @@
>  
>  extern int last_putchar;
>  
> +struct console_input_driver_struct;
> +typedef struct console_input_driver_struct {
> +	struct console_input_driver_struct *next;
> +	int (*havekey) (void);
> +	int (*getchar) (void);
> +} console_input_driver;

Do we have to typedef these?  I hate typedefs.
'struct console_input_driver' is perfect to me -
it tells me everything I need to know about the
variable.

> +struct console_output_driver_struct;
> +typedef struct console_output_driver_struct {
> +	struct console_output_driver_struct *next;
> +	void (*putchar) (unsigned int);
> +} console_output_driver;
> +
> +void console_add_output_driver(console_output_driver *out);
> +void console_add_input_driver(console_input_driver *in);
> +
>  #define havechar havekey
>  /** @} */
>  
> === drivers/keyboard.c
> ==================================================================
> --- drivers/keyboard.c	(revision 2218)
> +++ drivers/keyboard.c	(local)
> @@ -327,6 +327,11 @@
>  	return 0;
>  }
>  
> +static console_input_driver cons = {
> +	.havekey = keyboard_havechar,
> +	.getchar = keyboard_getchar
> +};
>
>  void keyboard_init(void)
>  {
>  	u8 mode;
> @@ -345,5 +350,7 @@
>  
>  	/* Write the new mode */
>  	keyboard_set_mode(mode);
> +
> +	console_add_input_driver(&cons);
>  }
>  
> === drivers/serial.c
> ==================================================================
> --- drivers/serial.c	(revision 2218)
> +++ drivers/serial.c	(local)
> @@ -58,15 +58,27 @@
>  	outb(reg &= ~0x80, port + 0x03);
>  }
>  
> +static console_input_driver consin = {
> +	.havekey = serial_havechar,
> +	.getchar = serial_getchar
> +};
> +
> +static console_output_driver consout = {
> +	.putchar = serial_putchar
> +};
> +
>  void serial_init(void)
>  {
>  #ifdef CONFIG_SERIAL_SET_SPEED
>  	serial_hardware_init(IOBASE, CONFIG_SERIAL_BAUD_RATE, 8, 0, 1);
>  #endif
> +	console_add_input_driver(&consin);
> +	console_add_output_driver(&consout);
>  }
>  
> -void serial_putchar(unsigned char c)
> +void serial_putchar(unsigned int c)
>  {
> +	c &= 0xff;
>  	while ((inb(IOBASE + 0x05) & 0x20) == 0) ;
>  	outb(c, IOBASE);
>  }
> === drivers/usb/usbhid.c
> ==================================================================
> --- drivers/usb/usbhid.c	(revision 2218)
> +++ drivers/usb/usbhid.c	(local)
> @@ -145,10 +145,21 @@
>  	dev->controller->control (dev, OUT, sizeof (dev_req_t), &dr, 0, 0);
>  }
>  
> +static console_input_driver cons = {
> +	.havekey = usbhid_havechar,
> +	.getchar = usbhid_getchar
> +};
> +
>  void
>  usb_hid_init (usbdev_t *dev)
>  {
>  
> +	static int installed = 0;
> +	if (!installed) {
> +		installed = 1;
> +		console_add_input_driver (&cons);
> +	}
> +
>  	configuration_descriptor_t *cd = (configuration_descriptor_t*)dev->configuration;
>  	interface_descriptor_t *interface = (interface_descriptor_t*)(((char *) cd) + cd->bLength);
>  
> === drivers/video/video.c
> ==================================================================
> --- drivers/video/video.c	(revision 2218)
> +++ drivers/video/video.c	(local)
> @@ -111,6 +111,11 @@
>  
>  void video_console_putchar(unsigned int ch)
>  {
> +	/* replace black-on-black with light-gray-on-black.
> +	   do it here, instead of in libc/console.c */
> +	if ((ch & 0xFF00) == 0) {
> +		ch |= 0x0700;
> +	}
>  	switch(ch & 0xFF) {
>  	case '\r':
>  		cursorx = 0;
> @@ -165,6 +170,10 @@
>  	video_console_fixup_cursor();
>  }
>  
> +static console_output_driver cons = {
> +	.putchar = video_console_putchar
> +};
> +
>  int video_console_init(void)
>  {
>  		int i;
> @@ -187,6 +196,8 @@
>  			return 0;
>  		}
>  
> +		console_add_output_driver(&cons);
> +
>  		return 0;
>  }
>  

> --
> coreboot mailing list: coreboot at coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot


-- 
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.





More information about the coreboot mailing list