Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers,uart: small improvements and cleanup #258

Merged
merged 6 commits into from
Mar 12, 2022
Merged

drivers,uart: small improvements and cleanup #258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2cafbe2
drivers,uart: set LCR after baud speed setting
wipawel Feb 14, 2022
a6f7976
drivers,uart: enable auxiliary output 2
wipawel Feb 19, 2022
5afc7b8
drivers,uart: enable UART FIFO control
wipawel Feb 19, 2022
919e3f4
drivers,uart: use ier_t type for configuring IER register
wipawel Feb 19, 2022
100ee3e
drivers,uart: improve handling of the IIR register
wipawel Feb 19, 2022
d128431
drivers,uart: set default com_port in init_uart()
wipawel Feb 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 33 additions & 15 deletions drivers/serial.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,15 @@ static inline void set_dlab(io_port_t port, bool dlab) {
static inline void set_port_mode(uart_config_t *cfg) {
lcr_t lcr = {0};

lcr.stop_bit = cfg->stop_bit;
lcr.width = cfg->frame_size;
lcr.parity = cfg->parity;

outb(cfg->port + UART_LCR_REG_OFFSET, lcr.reg);

/* Set baud speed by applying divisor to DLL+DLH */
set_dlab(cfg->port, true);
outw(cfg->port + UART_DLL_REG_OFFSET, DEFAULT_BAUD_SPEED / cfg->baud);
set_dlab(cfg->port, false);

lcr.stop_bit = cfg->stop_bit;
lcr.parity = cfg->parity;
lcr.width = cfg->frame_size;
outb(cfg->port + UART_LCR_REG_OFFSET, lcr.reg);
}

static inline bool thr_empty(io_port_t port) {
Expand All @@ -127,20 +126,33 @@ static inline bool receiver_ready(io_port_t port) {

void __text_init init_uart(uart_config_t *cfg) {
mcr_t mcr = {0};
fcr_t fcr = {0};
ier_t ier = {0};

/* Enable interrupts for received data available */
outb(cfg->port + UART_IER_REG_OFFSET, 0x01);

/* Disable FIFO control */
outb(cfg->port + UART_FCR_REG_OFFSET, 0x00);
ier.rx_avl = 1;
ier.rx_lsr_change = 0;
ier.msr_change = 1;
outb(cfg->port + UART_IER_REG_OFFSET, ier.reg);

/* Set 8n1 mode */
/* Set port mode */
set_port_mode(cfg);

/* Enable FIFO control */
fcr.enable = 1;
fcr.clear_rx = 1;
fcr.clear_tx = 1;
fcr.int_lvl = FIFO_INT_TRIGGER_LEVEL_1;
outb(cfg->port + UART_FCR_REG_OFFSET, fcr.reg);

/* Set tx/rx ready state */
mcr.dtr = 1;
mcr.rts = 1;
mcr.aux = 2;
outb(cfg->port + UART_MCR_REG_OFFSET, mcr.reg);

if (com_ports[0] == NO_COM_PORT)
com_ports[0] = cfg->port;
}

void __text_init init_uart_input(uint8_t dst_cpus) {
Expand Down Expand Up @@ -210,10 +222,16 @@ int serial_write(io_port_t port, const char *buf, size_t len) {
}

void uart_interrupt_handler(void) {
unsigned int i;
for (i = 0; i < ARRAY_SIZE(com_ports); ++i) {
uint8_t status = inb(com_ports[i] + UART_IIR_REG_OFFSET);
if ((status & UART_IIR_STATUS_MASK) == UART_IIR_RBR_READY) {
for (unsigned int i = 0; i < ARRAY_SIZE(com_ports); ++i) {
com_port_t com_port = com_ports[i];
iir_t iir;

if (com_port == NO_COM_PORT)
continue;

iir.reg = inb(com_port + UART_IIR_REG_OFFSET);
if (iir.event == UART_IIR_EVENT_RXD_AVAIL ||
iir.event == UART_IIR_EVENT_CHAR_TIMEOUT) {
uint8_t input = inb(com_ports[i] + UART_RBR_REG_OFFSET);

input_state.buf[input_state.curr] = input;
Expand Down
38 changes: 27 additions & 11 deletions include/drivers/serial.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,6 @@
#include <drivers/pic.h>
#include <ktf.h>

union line_control_register {
uint8_t reg;
struct __packed {
uint8_t width : 2, stop_bit : 1, parity : 3, break_ctrl : 1, DLAB : 1;
};
};
typedef union line_control_register lcr_t;

enum com_idx {
COM1 = 0,
COM2 = 1,
Expand All @@ -45,6 +37,7 @@ enum com_idx {
typedef enum com_idx com_idx_t;

enum com_port {
NO_COM_PORT = 0x0,
COM1_PORT = 0x3f8,
COM2_PORT = 0x2f8,
COM3_PORT = 0x3e8,
Expand Down Expand Up @@ -99,6 +92,14 @@ struct uart_config {
};
typedef struct uart_config uart_config_t;

union line_control_register {
uint8_t reg;
struct __packed {
uint8_t width : 2, stop_bit : 1, parity : 3, break_ctrl : 1, DLAB : 1;
};
};
typedef union line_control_register lcr_t;

union modem_control_register {
uint8_t reg;
struct __packed {
Expand Down Expand Up @@ -148,6 +149,24 @@ union interrupt_enable_register {
};
typedef union interrupt_enable_register ier_t;

union interrupt_identification_register {
uint8_t reg;
struct __packed {
uint8_t no_int_pend : 1, event : 3, rsvd : 1, fifo_64b : 1, fifo_status : 2;
};
Comment on lines +154 to +156
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of bit field members in C is implementation defined and for gcc it's defined by the ABI.
I'm fine with that, but we probably want a block like this?:

#if  __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
// order as above
#else 
// reverse order
#endif

Not that it matters now, but in case someone wants to add Aarch64 support or the like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is a very good point and IMO deserves a dedicated issue not to forget about it. We use similar bit-fields (where order would matter) in several other places too.

};
typedef union interrupt_identification_register iir_t;

#define UART_IIR_EVENT_MSR_CHANGE 0x0
#define UART_IIR_EVENT_THR_EMPTY 0x1
#define UART_IIR_EVENT_RXD_AVAIL 0x2
#define UART_IIR_EVENT_LSR_CHANGE 0x3
#define UART_IIR_EVENT_CHAR_TIMEOUT 0x6

#define UART_IIR_FIFO_NO_FIFO 0x0
#define UART_IIR_FIFO_UNUSABLE_FIFO 0x1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reading this: https://en.wikibooks.org/wiki/Serial_Programming/8250_UART_Programming#Interrupt_Identification_Register

Shouldn't this be 0x2?

Bit 7 Bit 6 Notes
0 1 Reserved condition
1 0 FIFO enabled, but not functioning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm other sources seem to disagree:

http://www.larvierinehart.com/serial/serialadc/serial.htm#16

https://www.lammertbies.nl/comm/info/serial-uart

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this or do we just trust either?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I tested my variant on many different devices and it works.

#define UART_IIR_FIFO_ENABLED 0x3

enum com_irq {
COM1_IRQ = 4, /* IRQ 4 */
COM2_IRQ = 3, /* IRQ 3 */
Expand All @@ -172,9 +191,6 @@ typedef enum com_irq com_irq_t;
#define UART_MSR_REG_OFFSET 0x06
#define UART_SCR_REG_OFFSET 0x07

#define UART_IIR_STATUS_MASK 0x0E /* bits 3, 2, 1 */
#define UART_IIR_RBR_READY 0x04

/* External declarations */

extern io_port_t com_ports[4];
Expand Down