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 1 commit
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
11 changes: 7 additions & 4 deletions drivers/serial.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,13 @@ 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;

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
21 changes: 18 additions & 3 deletions include/drivers/serial.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,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 +190,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