UART interrupt is not enabled

[stdefeber]

Describe the bug

Please see:

neorv32/sw/lib/source/neorv32_uart.c

Line 106 in b3418fd

tmp |= (uint32_t)(irq_mask & (0x1fU << UART_CTRL_IRQ_RX_NEMPTY));

As it is implemented right now the URT interrupt will never be enabled.

  tmp |= (uint32_t)(irq_mask & (0x1fU << UART_CTRL_IRQ_RX_NEMPTY));

irq_mask will be assigned a 1. Doing a logic or of 1 (0x1) with 0x00400000 (0x1fU << UART_CTRL_IRQ_RX_NEMPTY) will never result in a 1 at position 22.

It should have been:

  tmp |= (uint32_t)((irq_mask & 0x1fU) << UART_CTRL_IRQ_RX_NEMPTY));

Environment:
OS:

Distributor ID: Ubuntu
Description:    Ubuntu 24.04.1 LTS
Release:        24.04
Codename:       noble

GCC-Version native

gcc (GCC) 13.3.0

GCC-Version RISCV

COLLECT_GCC=riscv32-unknown-elf-gcc
COLLECT_LTO_WRAPPER=/nix/store/g5lm83yggmkgwpyi9h1cmra2v9i81blk-neorv32-toolchain/riscv/bin/../libexec/gcc/riscv32-unknown-elf/13.2.0/lto-wrapper
Target: riscv32-unknown-elf
Configured with: /home/stnolting/tmp/rvgcc/riscv-gnu-toolchain/gcc/configure --target=riscv32-unknown-elf --prefix=/home/stnolting/tmp/riscv/bin --disable-shared --disable-threads --enable-languages=c,c++ --with-pkgversion= --with-system-zlib --enable-tls --with-newlib --with-sysroot=/home/stnolting/tmp/riscv/bin/riscv32-unknown-elf --with-native-system-header-dir=/include --disable-libmudflap --disable-libssp --disable-libquadmath --disable-libgomp --disable-nls --disable-tm-clone-registry --src=.././gcc --disable-multilib --with-abi=ilp32 --with-arch=rv32i --with-tune=rocket --with-isa-spec=20191213 'CFLAGS_FOR_TARGET=-Os    -mcmodel=medlow' 'CXXFLAGS_FOR_TARGET=-Os    -mcmodel=medlow'
Thread model: single
Supported LTO compression algorithms: zlib
gcc version 13.2.0 () 

Hardware:

• Hardware version : v1.10.5
• Implemented CPU extensions, peripherals:
  UART, GPTMR, TWI, DMA, GPIO, WDT, SPI, Extions C, M

[stnolting]

irq_mask will be assigned a 1. Doing a logic or of 1 (0x1) with 0x00400000 (0x1fU << UART_CTRL_IRQ_RX_NEMPTY) will never result in a 1 at position 22.

You are absolutely right.

However, the interrupt configuration is designed differently. Let's say you want to enable the UART's "RX FIFO not empty" interrupt by setting bit 22 of the control register:

neorv32/sw/lib/include/neorv32_uart.h

Line 65 in b3418fd

UART_CTRL_IRQ_RX_NEMPTY = 22, /**< UART control register(22) (r/w): Fire IRQ if RX FIFO not empty */

You should call up the UART setup function as follows:

neorv32/sw/example/processor_check/main.c

Line 1078 in b3418fd

neorv32_uart0_setup(BAUD_RATE, 1 << UART_CTRL_IRQ_RX_NEMPTY);

So you need to pass the actual control register bit mask.

I don't think the software documentation is really clear here though.... 🙈 Do you have any suggestions on how to improve this? Or should we change the way interrupts are passed/configured?

[stdefeber]

Ah clear. Thanks for your quick response.

However you do not shift the baudrate in the function call.

My proposal to make it clear and unambiguous would be like this:

neorv32_uart0_setup(BAUD_RATE, **IRQ_ENABLE**)

and in the uart setup

void uart_setup(uint32_t baud_rate, bool irq_enable)
...
...
tmp |= .....
tmp |= (uint32_t)((irq_enable & 0x1fU) << UART_CTRL_IRQ_RX_NEMPTY));

[stnolting]

However you do not shift the baudrate in the function call.

Right. That's just a number and not a bit mask.

My proposal to make it clear and unambiguous would be like this:

That's a nice idea. However, the UART provides 5 individual interrupt sources (logically OR-ed). So we would need 5 binary arguments - one for each interrupt condition to be enabled or not. With so many arguments we cannot use plain registers to pass everathing to the function - the compiler will have to use the stack which slows things down and increases code size 🙈 (yes, I'm from the embedded world where each byte and each cycle counts 😅).

However, maybe your proposal would make things much clearer... 🤔

[stdefeber]

Maybe the following will support the case to have the same approach to all functions ;)

Both the timer and neoled module have a similar approach.

void neorv32_gptmr_setup(int prsc, uint32_t threshold, int cont_mode) {

  ..
  tmp |= (uint32_t)(cont_mode & 0x01) << GPTMR_CTRL_MODE;
  ..
}

void neorv32_neoled_setup(uint32_t prsc, uint32_t t_total, uint32_t t_high_zero, uint32_t t_high_one, int irq_mode) {

  NEORV32_NEOLED->CTRL = 0; // reset

  uint32_t tmp = 0;
  tmp |= (uint32_t)((1           & 0x01U) << NEOLED_CTRL_EN);         // module enable
  tmp |= (uint32_t)((prsc        & 0x07U) << NEOLED_CTRL_PRSC0);      // clock pre-scaler
  tmp |= (uint32_t)((t_total     & 0x1fU) << NEOLED_CTRL_T_TOT_0);    // serial data output: total period length for one bit
  tmp |= (uint32_t)((t_high_zero & 0x1fU) << NEOLED_CTRL_T_ZERO_H_0); // serial data output: high-time for sending a '0'
  tmp |= (uint32_t)((t_high_one  & 0x1fU) << NEOLED_CTRL_T_ONE_H_0);  // serial data output: high-time for sending a '1'
  tmp |= (uint32_t)((irq_mode    & 0x01U) << NEOLED_CTRL_EN);         // interrupt mode
  NEORV32_NEOLED->CTRL = tmp;
}