Hi,
the attached patch makes cbv2 add information about the serial port to lbtable. For this, a new record type is created. Payloads can then parse lbtable to figure out where to find the serial port (which is usually used by lb as console already).
A patch for grub2 exists, too, and I'll implement a compatible change for cbv3 once the record format (and overall design etc) is stable.
Regards, Patrick Georgi
* Patrick Georgi patrick@georgi-clan.de [080116 12:34]:
Signed-off-by: Patrick Georgi patrick@georgi-clan.de
I say: Acked-by: Stefan Reinauer stepan@coresystems.de
but lets wait to give someone else the chance to veto this.
Can you make a similar patch for v3 once its in v2?
Thanks,
Stefan
Index: src/include/boot/linuxbios_tables.h
--- src/include/boot/linuxbios_tables.h (Revision 3049) +++ src/include/boot/linuxbios_tables.h (Arbeitskopie) @@ -138,6 +138,13 @@ uint8_t string[0]; };
+#define LB_TAG_SERIAL 0x000e +struct lb_serial {
- uint32_t tag;
- uint32_t size;
- uint16_t ioport;
+};
/* The following structures are for the cmos definitions table */ #define LB_TAG_CMOS_OPTION_TABLE 200 /* cmos header record */ Index: src/arch/i386/boot/linuxbios_table.c =================================================================== --- src/arch/i386/boot/linuxbios_table.c (Revision 3049) +++ src/arch/i386/boot/linuxbios_table.c (Arbeitskopie) @@ -74,6 +74,18 @@ return mem; }
+struct lb_serial *lb_serial(struct lb_header *header) +{
- struct lb_record *rec;
- struct lb_serial *serial;
- rec = lb_new_record(header);
- serial = (struct lb_serial *)rec;
- serial->tag = LB_TAG_SERIAL;
- serial->size = sizeof(*serial);
- serial->ioport = TTYS0_BASE;
- return serial;
+}
struct lb_mainboard *lb_mainboard(struct lb_header *header) { struct lb_record *rec; @@ -406,8 +418,10 @@ * size of the linuxbios table. */
- /* Record our motheboard */
- /* Record our motherboard */ lb_mainboard(head);
- /* Record our console */
- lb_serial(head); /* Record our various random string information */ lb_strings(head);
-- coreboot mailing list coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Looks good, but a few comments:
On Wed, Jan 16, 2008 at 12:34:14PM +0100, Patrick Georgi wrote:
the attached patch makes cbv2 add information about the serial port to lbtable. For this, a new record type is created. Payloads can then parse lbtable to figure out where to find the serial port (which is usually used by lb as console already).
A patch for grub2 exists, too, and I'll implement a compatible change for cbv3 once the record format (and overall design etc) is stable.
Have you checked lxbios continues to work after the changes?
Index: src/arch/i386/boot/linuxbios_table.c
--- src/arch/i386/boot/linuxbios_table.c (Revision 3049) +++ src/arch/i386/boot/linuxbios_table.c (Arbeitskopie) @@ -74,6 +74,18 @@ return mem; }
+struct lb_serial *lb_serial(struct lb_header *header) +{
- struct lb_record *rec;
- struct lb_serial *serial;
- rec = lb_new_record(header);
- serial = (struct lb_serial *)rec;
- serial->tag = LB_TAG_SERIAL;
- serial->size = sizeof(*serial);
- serial->ioport = TTYS0_BASE;
This doesn't look generic enough(?) Where does TTYS0_BASE come from? Does the code expect it to be defined in auto.c or cache_as_ram_auto.c?
I think not all of the boards define it, and even if they did, TTYS0_BASE implies COM1, which is not always what you want. We need a mechanism to allow other ports here (COM2, COM3, COM4).
Yes, I think it's just a name, in theory you could assign any value to TTYS0_BASE, but you get the point.
Uwe.
* Uwe Hermann uwe@hermann-uwe.de [080116 16:03]:
Looks good, but a few comments:
On Wed, Jan 16, 2008 at 12:34:14PM +0100, Patrick Georgi wrote:
the attached patch makes cbv2 add information about the serial port to lbtable. For this, a new record type is created. Payloads can then parse lbtable to figure out where to find the serial port (which is usually used by lb as console already).
A patch for grub2 exists, too, and I'll implement a compatible change for cbv3 once the record format (and overall design etc) is stable.
Have you checked lxbios continues to work after the changes?
Index: src/arch/i386/boot/linuxbios_table.c
--- src/arch/i386/boot/linuxbios_table.c (Revision 3049) +++ src/arch/i386/boot/linuxbios_table.c (Arbeitskopie) @@ -74,6 +74,18 @@ return mem; }
+struct lb_serial *lb_serial(struct lb_header *header) +{
- struct lb_record *rec;
- struct lb_serial *serial;
- rec = lb_new_record(header);
- serial = (struct lb_serial *)rec;
- serial->tag = LB_TAG_SERIAL;
- serial->size = sizeof(*serial);
- serial->ioport = TTYS0_BASE;
This doesn't look generic enough(?) Where does TTYS0_BASE come from? Does the code expect it to be defined in auto.c or cache_as_ram_auto.c?
I think not all of the boards define it, and even if they did, TTYS0_BASE implies COM1, which is not always what you want. We need a mechanism to allow other ports here (COM2, COM3, COM4).
Yes, I think it's just a name, in theory you could assign any value to TTYS0_BASE, but you get the point.
yes, the variable should not be named TTYS0 but SERIAL_CONSOLE or something. But thats something I'd leave for v3.
On Wednesday 16 January 2008, Stefan Reinauer wrote:
- Uwe Hermann uwe@hermann-uwe.de [080116 16:03]:
This doesn't look generic enough(?) Where does TTYS0_BASE come from?
Agreed.
I think not all of the boards define it, and even if they did, TTYS0_BASE implies COM1, which is not always what you want. We need a mechanism to allow other ports here (COM2, COM3, COM4).
This goes off into the wrong direction, IMO.
Yes, I think it's just a name, in theory you could assign any value to TTYS0_BASE, but you get the point.
yes, the variable should not be named TTYS0 but SERIAL_CONSOLE or something. But thats something I'd leave for v3.
Both not appropriate, strictly. There is only one serial port, for the console at best that everyone needs to care about until an OS is loaded.
The questions are: 1. of what type is the active console? a) VGA-compatible b) 8250-compatible c) ...
2. where is it? This depends on 1.: VGA occupies a lot of unique ports, 8250 is specified sufficiently by a base address.
3. Is there additional information? A smarter payload might want to use e.g. the interrupt that we assigned to the port ;-)
Since the format is tagged, there should be no problem to reflect this.
Torsten
Am Donnerstag, den 17.01.2008, 12:55 +0100 schrieb Torsten Duwe:
On Wednesday 16 January 2008, Stefan Reinauer wrote:
- Uwe Hermann uwe@hermann-uwe.de [080116 16:03]:
This doesn't look generic enough(?) Where does TTYS0_BASE come from?
Agreed.
TTYS0_BASE seems to be the standard name used across the code base. The attached patch provides a no-op in case the configuration doesn't provide a serial port.
I think not all of the boards define it, and even if they did, TTYS0_BASE implies COM1, which is not always what you want. We need a mechanism to allow other ports here (COM2, COM3, COM4).
This goes off into the wrong direction, IMO.
The codebase doesn't seem to be prepared for multiple serial ports in the first place (I only find TTYS0_*). I'd say, we add a port number to the record once we actually support >1 ports.
Both not appropriate, strictly. There is only one serial port, for the console at best that everyone needs to care about until an OS is loaded.
As my initial query (http://www.coreboot.org/pipermail/coreboot/2008-January/029192.html) got no response, I decided to just work on the problem at hand, which is - in my case - a hand over of information about the serial port (which we don't do at all currently).
As you can see, my patch doesn't pretend that the serial port is the console, so a "you can find the console $here" record can be added without conflicts. That new record could then refer to the serial port as specified here, in case the console is on serial.
The questions are:
- of what type is the active console?
a) VGA-compatible b) 8250-compatible c) ...
Different issue (see above)
- Is there additional information? A smarter payload might want to use e.g.
the interrupt that we assigned to the port ;-)
Once we do that, the serial record could be extended by that information. Given that we have a size field in the record, that can even be done in a compatible way, or not?
I'll do patches for lxbios and v3 once this patch (and more importantly, the record format) is accepted. This patch is updated against latest svn, and provides an alternative in case TTYS0_BASE isn't defined (see the #ifdef).
Regards, Patrick Georgi
On Monday 21 January 2008, you wrote:
The codebase doesn't seem to be prepared for multiple serial ports in the first place (I only find TTYS0_*).
OK.
As my initial query (http://www.coreboot.org/pipermail/coreboot/2008-January/029192.html)
Right. That would have been the thread. Sorry.
As you can see, my patch doesn't pretend that the serial port is the console, so a "you can find the console $here" record can be added without conflicts. That new record could then refer to the serial port as specified here, in case the console is on serial.
The questions are:
- of what type is the active console?
a) VGA-compatible b) 8250-compatible c) ...
Once we do that, the serial record could be extended by that information. Given that we have a size field in the record, that can even be done in a compatible way, or not?
Absolutely. This is what I propose. Rename the record LB_TAG_CONSOLE, add a type field that is currently always 1, meaning 8250-compatible, I/O-mapped serial. Also an interrupt field would be nice, with a default "none" value.
This patch is updated against latest svn, and provides an alternative in case TTYS0_BASE isn't defined (see the #ifdef).
No more mocking about TTYS0 from my side. Do you want to make the other changes or shall I?
Torsten
Am Montag, den 21.01.2008, 22:39 +0100 schrieb Torsten Duwe:
On Monday 21 January 2008, you wrote:
The codebase doesn't seem to be prepared for multiple serial ports in the first place (I only find TTYS0_*).
OK.
Actually, in v3 it's called TTYSx_* (with a literal 'x'), probably to avoid this issue in case a different serial device is used.
Once we do that, the serial record could be extended by that information. Given that we have a size field in the record, that can even be done in a compatible way, or not?
Absolutely. This is what I propose. Rename the record LB_TAG_CONSOLE, add a type field that is currently always 1, meaning 8250-compatible, I/O-mapped serial. Also an interrupt field would be nice, with a default "none" value.
This record is intended to tell the payload about the existence and where-abouts of a serial port. A console would be a separate record with that number you propose (or something like that)
If and where a serial port is to be found is an information that should be provided even if (say) vga is being used for the console, in my opinion.
As for the layout of the LB_TAG_CONSOLE (which has to come, eventually), there was some discussion on IRC already, but I think it quickly became over-engineered.
No more mocking about TTYS0 from my side. Do you want to make the other changes or shall I?
The tag number is already taken in v3, so 0xe must be changed (just for reference).
I'll post another patch, with a new ID (a v3 patch exists and works already, that'll come, too) and work on a proposal for LB_TAG_CONSOLE - tomorrow.
Regards, Patrick Georgi
On Monday 21 January 2008, Patrick Georgi wrote:
This record is intended to tell the payload about the existence and where-abouts of a serial port. A console would be a separate record with that number you propose (or something like that)
If and where a serial port is to be found is an information that should be provided even if (say) vga is being used for the console, in my opinion.
Can you elaborate that? I cannot think of a good use a payload can make of a serial port other than console (please not PPP booting over modem ;-). And operating systems will find them their own way.
As for the layout of the LB_TAG_CONSOLE (which has to come, eventually), there was some discussion on IRC already, but I think it quickly became over-engineered.
It's good to have multiple places to discuss things :-/ Is there a log?
The tag number is already taken in v3, so 0xe must be changed (just for reference).
I'll post another patch, with a new ID (a v3 patch exists and works already, that'll come, too) and work on a proposal for LB_TAG_CONSOLE - tomorrow.
First we should all agree on what the structure is. I'd like to read the IRC log to pick up those arguments. My current opinion is that LB_TAG_CONSOLE should do it all. Maybe also LB_TAG_ALTERNATE_CONSOLE in case the payload lacks certain drivers, and can not use coreboot's preference. This would carry all console information, you would be able to describe all serial ports if you like, it's not overly complicated and can be achieved with minimal changes to your current patch.
Torsten
On Tue, Jan 22, 2008 at 11:45:08AM +0100, Torsten Duwe wrote:
I cannot think of a good use a payload can make of a serial port other than console
Some kind of recovery, management, whatever. I can't think of all uses either, but I certianly do not want to impose an arbitrary limit like "coreboot only tells you about serial ports that are the console" - I like the generic serial tag.
//Peter
Peter Stuge wrote:
On Tue, Jan 22, 2008 at 11:45:08AM +0100, Torsten Duwe wrote:
I cannot think of a good use a payload can make of a serial port other than console
Some kind of recovery, management, whatever. I can't think of all uses either, but I certianly do not want to impose an arbitrary limit like "coreboot only tells you about serial ports that are the console" - I like the generic serial tag.
Note: There's another patch from Jens Freimann which puts all of the device tree into the cb table. Unlike Patrick's patch, Jens' does not reveil any information like "coreboot is actually using this device in this and that manner". So I think those are two different things.
While I agree we want to export all devices (not only serial ports), I believe that we should also export a given amount of "management information" that can be used by payloads to make a consistent code flow through all stages of our modular design, such as: if you change the baud rate with "lxbios", it should propagate through to grub2 and not be linuxbios specific. For the user, "lxbios" is the firmware setup utility, not the coreboot utility alone.
Not sure whether those two table types should be mixed. Especially in v2. Opinions?
Stefan Reinauer schrieb:
Note: There's another patch from Jens Freimann which puts all of the device tree into the cb table. Unlike Patrick's patch, Jens' does not reveil any information like "coreboot is actually using this device in this and that manner". So I think those are two different things.
My patch also doesn't "really" say such things - so far, it only tells the reader of the table that there is a serial port to be found at a certain port. If Jens' solution is more complete (and its data format is still reasonably simple to parse), I'd opt for going that route, dropping my patch. How to tell the payload where a console is to be found can be layered on top of both approaches, I assume. Is Jens' patch for v3, or also for v2? In v3, there's a record for a pointer to the devtree, is that it?
While I agree we want to export all devices (not only serial ports), I believe that we should also export a given amount of "management information" that can be used by payloads to make a consistent code flow through all stages of our modular design, such as: if you change the baud rate with "lxbios", it should propagate through to grub2 and not be linuxbios specific. For the user, "lxbios" is the firmware setup utility, not the coreboot utility alone.
I did only the serial port with the intent that more devices can be added (somehow) on an as-needed basis. No need in designing a one-size-fits-all solution when it might not fit actual usage later on.
Regards, Patrick Georgi
Am Tue, 22 Jan 2008 20:37:59 +0100 schrieb Peter Stuge:
Some kind of recovery, management, whatever. I can't think of all uses either, but I certianly do not want to impose an arbitrary limit like "coreboot only tells you about serial ports that are the console" - I like the generic serial tag.
Attached patch changes the following vs. the version as of yesterday: - fix comment (patch adds a serial port, not a console) - change tag ID to 0xf (0xe is already taken in v3)
Regards, Patrick Georgi
On Tue, Jan 22, 2008 at 09:43:35PM +0100, Patrick Georgi wrote:
This patch adds a new record type for lbtable to provide information about a serial port. If a port is defined in the board configuration, add it to lbtable.
Signed-off-by: Patrick Georgi patrick@georgi-clan.de
Acked-by: Peter Stuge peter@stuge.se
Index: src/include/boot/coreboot_tables.h
--- src/include/boot/coreboot_tables.h (Revision 3066) +++ src/include/boot/coreboot_tables.h (Arbeitskopie) @@ -138,6 +138,13 @@ uint8_t string[0]; };
+#define LB_TAG_SERIAL 0x000f +struct lb_serial {
- uint32_t tag;
- uint32_t size;
- uint16_t ioport;
+};
/* The following structures are for the cmos definitions table */ #define LB_TAG_CMOS_OPTION_TABLE 200 /* cmos header record */ Index: src/arch/i386/boot/coreboot_table.c =================================================================== --- src/arch/i386/boot/coreboot_table.c (Revision 3066) +++ src/arch/i386/boot/coreboot_table.c (Arbeitskopie) @@ -74,6 +74,22 @@ return mem; }
+struct lb_serial *lb_serial(struct lb_header *header) +{ +#if defined(TTYS0_BASE)
- struct lb_record *rec;
- struct lb_serial *serial;
- rec = lb_new_record(header);
- serial = (struct lb_serial *)rec;
- serial->tag = LB_TAG_SERIAL;
- serial->size = sizeof(*serial);
- serial->ioport = TTYS0_BASE;
- return serial;
+#else
- return header;
+#endif +}
struct lb_mainboard *lb_mainboard(struct lb_header *header) { struct lb_record *rec; @@ -406,8 +422,10 @@ * size of the coreboot table. */
- /* Record our motheboard */
- /* Record our motherboard */ lb_mainboard(head);
- /* Record the serial port, if present */
- lb_serial(head); /* Record our various random string information */ lb_strings(head);
On Tuesday 22 January 2008, Peter Stuge wrote:
On Tue, Jan 22, 2008 at 09:43:35PM +0100, Patrick Georgi wrote:
This patch adds a new record type for lbtable to provide information about a serial port. If a port is defined in the board configuration, add it to lbtable.
Signed-off-by: Patrick Georgi patrick@georgi-clan.de
Acked-by: Peter Stuge peter@stuge.se
I don't like it. Not because of the change itself, but because IMHO it is more inportant to record a default console first. We're behind legacy BIOS with its int0x10, and this will widen the gap.
But no NAK, maybe you have a cunning plan.
Torsten
On Mon, Jan 21, 2008 at 01:30:36PM +0100, Patrick Georgi wrote:
As you can see, my patch doesn't pretend that the serial port is the console
..yet..
- /* Record our console */
- lb_serial(head);
Change the comment and I'll ack.
//Peter
Am Mittwoch, den 16.01.2008, 16:03 +0100 schrieb Uwe Hermann:
Have you checked lxbios continues to work after the changes?
No, will do.
I think not all of the boards define it, and even if they did,
I'll fix that.
Thanks
TTYS0_BASE implies COM1, which is not always what you want. We need a mechanism to allow other ports here (COM2, COM3, COM4).
I won't work on infrastructure improvements for v2 if I can help it.
Yes, I think it's just a name, in theory you could assign any value to TTYS0_BASE, but you get the point.
"ttys0" is rather meaningless by itself. What if it means "0th serial port we use" (emphasis on the last two words)? A stable name would be "COM1" in my opinion...
Regards, Patrick Georgi
* Patrick Georgi patrick@georgi-clan.de [080116 19:20]:
Am Mittwoch, den 16.01.2008, 16:03 +0100 schrieb Uwe Hermann:
Have you checked lxbios continues to work after the changes?
No, will do.
I think not all of the boards define it, and even if they did,
I'll fix that.
Thanks
TTYS0_BASE implies COM1, which is not always what you want. We need a mechanism to allow other ports here (COM2, COM3, COM4).
I won't work on infrastructure improvements for v2 if I can help it.
Yes, I think it's just a name, in theory you could assign any value to TTYS0_BASE, but you get the point.
"ttys0" is rather meaningless by itself. What if it means "0th serial port we use" (emphasis on the last two words)? A stable name would be "COM1" in my opinion...
Yes, that's what the SuperIO datasheets use. In theory it is possible to put io 2e8 to what is COM1 in the superio, but that doesn't make all too much sense.
Stefan
-
Patrick Georgi -
Peter Stuge -
Stefan Reinauer -
Torsten Duwe -
Uwe Hermann