Re: [PATCH 2/4] drivers/tty/serial: add driver for the ESP32 UART

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6259 bytes --]

On Fri, 15 Sep 2023, Max Filippov wrote:

> On Thu, Sep 14, 2023 at 6:07 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> 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@gmail.com>
> > > ---
> > >  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.