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.

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

[FEATURE] Extensible PL011 UART driver #12901

Open
1 task done
linguini1 opened this issue Aug 12, 2024 · 3 comments · May be fixed by #13071
Open
1 task done

[FEATURE] Extensible PL011 UART driver #12901

linguini1 opened this issue Aug 12, 2024 · 3 comments · May be fixed by #13071
Labels
Type: Enhancement New feature or request

Comments

@linguini1
Copy link
Contributor

linguini1 commented Aug 12, 2024

Is your feature request related to a problem? Please describe.

I would like to re-use the existing driver code for the PL011 UART while having it be tailored to a specific system. The current setup of the PL011 UART driver limits the amount of PL011 UARTs that are available and restricts their configuration through a driver specific Kconfig file.

Describe the solution you'd like

The current PL011 driver in nuttx/drivers/serial/uart_pl011.c supports up to 4 PL011 UARTs which must be configured using the options in the nuttx/drivers/serial/Kconfig-pl011 configuration file.

I am working on a port of NuttX for the Raspberry Pi 4, which has 5 PL011 UART interfaces.

The current PL011 UART driver will not work for this application because it is limited to four interfaces.

The interface base addresses must be configured through the driver's Kconfig file, not through pre-processor definitions that are specific to a board (the peripheral addresses for the UART interfaces on the Raspberry Pi can change depending on memory configurations).

The interfaces are configured as UART 0 through 3, but on the Raspberry Pi, there is a sixth UART interface (a special Mini-UART interface) which is UART 1. So this would affect the current numbering scheme.

What I suggest is modifying the existing driver logic so that it becomes more adjacent to a library that developers can use to implement a board/chip specific PL011 UART driver.

I would like to be able to include a header file for the PL011 UART logic which contains the type definitions for the pl011_uart_port_s type. This would allow me to create my own PL011 UART devices depending on the chip/board support like so:

#include <drivers/serial/pl011_uart.h>

#ifdef CONFIG_UART0

static struct pl011_uart_port_s g_uart0priv = {
    // Initialization stuff that is unique to pl011
    .data = {
        .baud_rate = CONFIG_UART0_BAUD,
        // etc ...
    },
};

static struct uart_dev_s g_uart0 = {
// Typical UART configuration (buffers, etc.)
.priv = &g_uart0priv,
};

#endif // CONFIG_UART0

// Repeat for UART 2 - 5 using different configurations for BAUD, buffer sizes, etc.

Ideally the library would also contain a helper function which wraps around uart_register. This function would register a PL011 UART device after initializing its operations:

// In nuttx/drivers/serial/pl011_uart.c

static struct uart_ops_s g_pl011_ops = {
// Initialized with all the `static` pl011 operation functions
};

int pl011_uart_register(char *pathname, struct uart_dev_s *dev) {
    dev->ops = &g_pl011_ops;
    return uart_register(pathname, dev);
}

This allows all the functions that implement the UART operations to remain hidden within the module as static, but provides an interface for authors of PL011 UART drivers to register their custom configured PL011 devices.

The proposed changes reduce code duplication but allows for extensibility depending on the chip board (virtually everything can be changed, just the underlying data structure is the same and the operations are re-used).

Describe alternatives you've considered

I've considered just copy-pasting the PL011 driver implementation into my board's source tree to modify the initialization according to the board's number of UART interfaces and base addresses, etc. However, this seems like a poor choice as it duplicates code and bars the Raspberry Pi port from benefiting from any upgrades/bug fixes made to the PL011 UART driver.

I've also considered changing the PL011 UART driver to support an additional interface and then use the driver Kconfig to set the base addresses, etc. (i.e. the board Kconfig depends on the driver Kconfig for some options), but this seems like a bandaid solution for two reasons:

  1. Another board may come along with more PL011 interfaces, requiring more copy-pasting of another g_pl011uart[N] device
  2. The logic for selecting the UART base addresses is already defined in preprocessor macros, and I don't want to have to script it in Kconfig instead. It feels a little gross and not as extensible.

There doesn't seem to be too many implementation relying on this driver, so I think it's still doable to make these changes. Some of the implementations are QEMU as well, which makes it easy to test if the changes broke anything.

Verification

  • I have verified before submitting the report.
@linguini1 linguini1 added the Type: Enhancement New feature or request label Aug 12, 2024
@xiaoxiang781216
Copy link
Contributor

Yes, I think it's better to expose a new function(pl011_uart_register) for your case.

@linguini1
Copy link
Contributor Author

So after exploring this solution for a little bit, I've noticed another challenge.

The PL011 UARTs on the Raspberry Pi 4B all share one single IRQ, which doesn't work with the current implementation of the IRQ attaching logic in the PL011 UART driver. Part of me wants to mess around with a "clean" solution for this, but I think it would just be faster for me to duplicate the driver code in the Pi source tree and make the required modifications.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Aug 19, 2024

So after exploring this solution for a little bit, I've noticed another challenge.

The PL011 UARTs on the Raspberry Pi 4B all share one single IRQ, which doesn't work with the current implementation of the IRQ attaching logic in the PL011 UART driver. Part of me wants to mess around with a "clean" solution for this, but I think it would just be faster for me to duplicate the driver code in the Pi source tree and make the required modifications.

you can turn on IRQCHAIN option, so the same irq can be attached more than one handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants