On Mon, Jul 04, 2016 at 10:39:54PM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>

Not sure if this is still an RFC.  I think it needs to have a Kconfig
option.  It should probably also have runtime enable option.

[...]
> --- a/Makefile
> +++ b/Makefile
> @@ -29,7 +29,7 @@ LD32BIT_FLAG:=-melf_i386
>  
>  # Source files
>  SRCBOTH=misc.c stacks.c output.c string.c block.c cdrom.c disk.c mouse.c 
> kbd.c \
> -    system.c serial.c clock.c resume.c pnpbios.c vgahooks.c pcibios.c apm.c \
> +    system.c serial.c sercon.c clock.c resume.c pnpbios.c vgahooks.c 
> pcibios.c apm.c \

How about console.c instead of sercon.c?

[...]
> --- /dev/null
> +++ b/src/sercon.c
> @@ -0,0 +1,545 @@

There should be a copyright statement.

[...]
> +VARLOW u8 sercon_char[8];
> +VARLOW u8 sercon_attr[8];

Out of curiosity, is 8 needed for effective "uncluttering" or is it an
optimization?  (That is, would a size of 1 suffice for the common
case?)

[...]
> +static void sercon_lazy_putchar(u8 chr, u8 attr, u8 teletype)
> +{
> +    u8 idx;
> +
> +    if (cursor_pos_row() != GET_LOW(sercon_row_last) ||
> +        cursor_pos_col() <  GET_LOW(sercon_col_last) ||
> +        cursor_pos_col() >= GET_LOW(sercon_col_last) + 
> ARRAY_SIZE(sercon_char)) {
> +        sercon_lazy_flush();
> +    }
> +
> +    idx = cursor_pos_col() - GET_LOW(sercon_col_last);
> +    SET_LOW(sercon_char[idx], chr);
> +    if (teletype)
> +        cursor_pos_set(cursor_pos_row(),
> +                       cursor_pos_col()+1);

This doesn't handle end of column wrapping properly.

> +    else
> +        SET_LOW(sercon_attr[idx], attr);
> +}

FYI, once sercon_1013 is implemented then 'teletype' is not mutually
exclusive with 'use_attr'.

[...]
> +static void sercon_1000(struct bregs *regs)
> +{
> +    int mode, rows, cols;
> +
> +    mode = regs->al;
> +    switch (mode) {
> +    case 0x03:
> +    default:
> +        cols = 80;
> +        rows = 25;
> +        regs->al = 0x30;
> +    }
> +    SET_LOW(sercon_col_last, 0);
> +    SET_LOW(sercon_row_last, 0);
> +
> +    cursor_pos_set(0, 0);
> +    SET_BDA(video_mode, mode);
> +    SET_BDA(video_cols, cols);
> +    SET_BDA(video_rows, rows-1);
> +    SET_BDA(cursor_type, 0x0007);
> +
> +    sercon_term_reset();
> +    sercon_term_clear_screen();

Should only clear screen if high-bit of regs->al not set.

> +/* Set text-mode cursor shape */
> +static void sercon_1001(struct bregs *regs)
> +{
> +    /* dummy (add show/hide cursor?) */
> +}

Not sure if hiding the cursor is necessary, but it should
SET_BDA(cursor_type, regs->cx)

[...]
> +static void sercon_1003(struct bregs *regs)
> +{
> +    regs->ax = 0;
> +    regs->ch = 0;
> +    regs->cl = 7;

regs->cx = GET_BDA(cursor_type)

Not sure why regs->ax is cleared.

[...]
> +static void sercon_1009(struct bregs *regs)
> +{
> +    u16 count = regs->cx;
> +
> +    if (count == 1) {
> +        sercon_lazy_putchar(regs->al, regs->bl, 0);
> +
> +    } else if (regs->al == 0x20 &&
> +               video_rows() * video_cols() == count &&
> +               cursor_pos_row() == 0 &&
> +               cursor_pos_col() == 0) {
> +        /* override everything with spaces -> this is clear screen */
> +        sercon_lazy_flush();
> +        sercon_set_attr(regs->bl);
> +        sercon_term_clear_screen();
> +
> +    } else {
> +        sercon_lazy_flush();
> +        sercon_set_attr(regs->bl);
> +        while (count) {
> +            sercon_putchar(regs->al);

At a minimum that should be sercon_print_utf8(), though could
sercon_lazy_putchar() be used?

[...]
> +static void sercon_100e(struct bregs *regs)
> +{
> +    switch (regs->al) {
> +    case 7:
> +        // beep
> +        break;

sercon_putchar(0x07)

> +void VISIBLE16
> +sercon_10(struct bregs *regs)
> +{
> +    if (!GET_LOW(sercon_port))
> +        return;
> +
> +    switch (regs->ah) {
> +    case 0x00: sercon_1000(regs); break;
> +    case 0x01: sercon_1001(regs); break;
> +    case 0x02: sercon_1002(regs); break;
> +    case 0x03: sercon_1003(regs); break;
> +    case 0x06: sercon_1006(regs); break;
> +    case 0x08: sercon_1008(regs); break;
> +    case 0x09: sercon_1009(regs); break;
> +    case 0x0e: sercon_100e(regs); break;
> +    case 0x0f: sercon_100f(regs); break;
> +    case 0x4f: sercon_104f(regs); break;
> +    default:   sercon_10XX(regs); break;
> +    }
> +}

I noticed sercon_1007, sercon_100a, and sercon_1013 aren't
implemented.  I guess they aren't used frequently, but they've been
around since the original IBM MDA adapter.

[...]
> +void VISIBLE16
> +sercon_check_event(void)

Not sure VISIBLE16 is needed.

> +{
> +    u16 addr = GET_LOW(sercon_port);
> +    u16 keycode;
> +    u8 byte, count = 0;
> +    int seq, chr, len;
> +
> +    // check to see if there is a active serial port
> +    if (!addr)
> +        return;
> +    if (inb(addr + SEROFF_LSR) == 0xFF)
> +        return;
> +
> +    // flush pending output
> +    sercon_lazy_flush();
> +
> +    // read all available data
> +    while (inb(addr + SEROFF_LSR) & 0x01) {
> +        byte = inb(addr + SEROFF_DATA);
> +        if (GET_LOW(rx_bytes) < sizeof(rx_buf)) {
> +            SET_LOW(rx_buf[rx_bytes], byte);
> +            SET_LOW(rx_bytes, GET_LOW(rx_bytes) + 1);
> +            count++;
> +        }
> +    }
> +
> +next_char:
> +    // no (more) input data
> +    if (!GET_LOW(rx_bytes))
> +        return;
> +
> +    // lookup escape sequences
> +    if (GET_LOW(rx_bytes) > 1 && GET_LOW(rx_buf[0]) == 0x1b) {
> +        for (seq = 0; seq < ARRAY_SIZE(termseq); seq++) {
> +            len = GET_GLOBAL(termseq[seq].len);
> +            if (GET_LOW(rx_bytes) < len + 1)
> +                continue;
> +            for (chr = 0; chr < len; chr++) {
> +                if (GET_GLOBAL(termseq[seq].seq[chr]) != GET_LOW(rx_buf[chr 
> + 1]))
> +                    break;
> +            }
> +            if (chr == len) {
> +                enqueue_key(GET_GLOBAL(termseq[seq].keycode));
> +                shiftbuf(len + 1);
> +                goto next_char;
> +            }
> +        }
> +    }
> +
> +    // Seems we got a escape sequence we didn't recognise.
> +    //  -> If we received data wait for more, maybe it is just incomplete.
> +    if (GET_LOW(rx_buf[0]) == 0x1b && count)
> +        return;
> +
> +    // Handle input as individual chars.
> +    chr = GET_LOW(rx_buf[0]);
> +    keycode = ascii_to_keycode(chr);
> +    if (keycode)
> +        enqueue_key(keycode);
> +    shiftbuf(1);
> +    goto next_char;
> +}

I would prefer if we could avoid backwards gotos.

In general, looks good.
-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios

Reply via email to