On Fri, 15 Sep 2023, Max Filippov wrote: > On Thu, Sep 14, 2023 at 6:07 AM Ilpo Järvinen > <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote: > > > > On Wed, 13 Sep 2023, Max Filippov wrote: > > > > > Add driver for the UART controllers of the Espressif ESP32 and ESP32S3 > > > SoCs. Hardware specification is available at the following URLs: > > > > > > https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf > > > (Chapter 13 UART Controller) > > > https://www.espressif.com/sites/default/files/documentation/esp32-s3_technical_reference_manual_en.pdf > > > (Chapter 26 UART Controller) > > > > > > Signed-off-by: Max Filippov <jcmvbkbc@xxxxxxxxx> > > > --- > > > drivers/tty/serial/Kconfig | 13 + > > > drivers/tty/serial/Makefile | 1 + > > > drivers/tty/serial/esp32_uart.c | 766 +++++++++++++++++++++++++++++++ > > > include/uapi/linux/serial_core.h | 3 + > > > 4 files changed, 783 insertions(+) > > > create mode 100644 drivers/tty/serial/esp32_uart.c > > > > > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > > > index bdc568a4ab66..d9ca6b268f01 100644 > > > --- a/drivers/tty/serial/Kconfig > > > +++ b/drivers/tty/serial/Kconfig > > > @@ -1578,6 +1578,19 @@ config SERIAL_NUVOTON_MA35D1_CONSOLE > > > but you can alter that using a kernel command line option such as > > > "console=ttyNVTx". > > > > > > +config SERIAL_ESP32 > > > + tristate "Espressif ESP32 UART support" > > > + depends on XTENSA_PLATFORM_ESP32 || (COMPILE_TEST && OF) > > > + select SERIAL_CORE > > > + select SERIAL_CORE_CONSOLE > > > + select SERIAL_EARLYCON > > > + help > > > + Driver for the UART controllers of the Espressif ESP32xx SoCs. > > > + When earlycon option is enabled the following kernel command line > > > + snippets may be used: > > > + earlycon=esp32s3uart,mmio32,0x60000000,115200n8,40000000 > > > + earlycon=esp32uart,mmio32,0x3ff40000,115200n8 > > > + > > > endmenu > > > > > > config SERIAL_MCTRL_GPIO > > > diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile > > > index 138abbc89738..7b73137df7f3 100644 > > > --- a/drivers/tty/serial/Makefile > > > +++ b/drivers/tty/serial/Makefile > > > @@ -88,6 +88,7 @@ obj-$(CONFIG_SERIAL_MILBEAUT_USIO) += milbeaut_usio.o > > > obj-$(CONFIG_SERIAL_SIFIVE) += sifive.o > > > obj-$(CONFIG_SERIAL_LITEUART) += liteuart.o > > > obj-$(CONFIG_SERIAL_SUNPLUS) += sunplus-uart.o > > > +obj-$(CONFIG_SERIAL_ESP32) += esp32_uart.o > > > > > > # GPIOLIB helpers for modem control lines > > > obj-$(CONFIG_SERIAL_MCTRL_GPIO) += serial_mctrl_gpio.o > > > diff --git a/drivers/tty/serial/esp32_uart.c b/drivers/tty/serial/esp32_uart.c > > > new file mode 100644 > > > index 000000000000..05ec0fce3a62 > > > --- /dev/null > > > +++ b/drivers/tty/serial/esp32_uart.c > > > +static u32 esp32_uart_tx_fifo_cnt(struct uart_port *port) > > > +{ > > > + return (esp32_uart_read(port, UART_STATUS_REG) & > > > + port_variant(port)->txfifo_cnt_mask) >> UART_TXFIFO_CNT_SHIFT; > > > +} > > > + > > > +static u32 esp32_uart_rx_fifo_cnt(struct uart_port *port) > > > +{ > > > + return (esp32_uart_read(port, UART_STATUS_REG) & > > > + port_variant(port)->rxfifo_cnt_mask) >> UART_RXFIFO_CNT_SHIFT; > > > > FIELD_GET(). > > I see how FIELD_GET can be used in other places, but here > port_variant(port)->rxfifo_cnt_mask is not a runtime constant and > FIELD_GET does not work with non-constant field definitions. Any > suggestions? Ah, I sorry. I probably moved around while composing the reply and it seems I returned to add the comment to a wrong line. I don't have a good suggestion for this one besides breking it into multiple lines for better readability. > > Use more lines (and a local variable) instead of splitting one line. It > > will be more readable that way. > > Ok. > > > + u32 frag = (port->uartclk * 16) / baud - div * 16; > > > + > > > + if (div <= port_variant(port)->clkdiv_mask) { > > > + esp32_uart_write(port, UART_CLKDIV_REG, > > > + div | (frag << UART_CLKDIV_FRAG_SHIFT)); > > > > FIELD_PREP(). > > Ok. > > > Also div be encapsulated into FIELD_PREP here even if it's > > shift is 0. > > This field's mask, port_variant(port)->clkdiv_mask, is, again, not a runtime > constant. Any suggestion for this? So you're saying there are two different sized fields. I suppose it can be left as is then. > > > + port->uartclk / port_variant(port)->clkdiv_mask); > > > + esp32_uart_write(port, UART_CLKDIV_REG, > > > + port_variant(port)->clkdiv_mask | > > > + UART_CLKDIV_FRAG_MASK); > > > > I think you want to make the meaning more obvious here by using > > FIELD_MAX(UART_CLKDIV_FRAG_MASK); > > No. I think FIELD_MAX makes it less obvious, because to work properly > it must be not just > FIELD_MAX(UART_CLKDIV_FRAG), > but > FIELD_PREP(UART_CLKDIV_FRAG, FIELD_MAX(UART_CLKDIV_FRAG)). Maybe add FIELD_PREP_MAX() in a preparatory patch to bitfield.h as this looks something that could be of use beyond this driver. > > > + esp32_uart_write(port, UART_CONF0_REG, conf0); > > > + esp32_uart_write(port, UART_CONF1_REG, conf1); > > > + spin_unlock_irqrestore(&port->lock, flags); > > > + > > > + /* > > > + * esp32s3 may not support 9600, passing its minimal baud rate > > > + * as the min argument would trigger a WARN inside uart_get_baud_rate > > > + */ > > > + baud = uart_get_baud_rate(port, termios, old, > > > + 0, port->uartclk / 16); > > > > This looks questionable solution to the problem mentioned in the comment. > > What happens when user asks for baudrates below the minimum supported one? > > I'll change it to refuse to change to unsupported baud rate and set default > to 115200. > > > You might need to do something touching the core code to handle the case > > where 9600 is not possible fallback. > > I'd rather make a fallback to 115200, as it's a much more reasonable default. I'll have to see the code before commenting on these. -- i.
