On Thu, Sep 28, 2023 at 08:16:31AM -0700, Max Filippov wrote: > Add driver for the ACM controller of the Espressif ESP32S3 Soc. Odd extra space :( > Hardware specification is available at the following URL: > > https://www.espressif.com/sites/default/files/documentation/esp32-s3_technical_reference_manual_en.pdf > (Chapter 33 USB Serial/JTAG Controller) I don't understand this driver, "ACM" is a USB host <-> gadget protocol, why do you need a platform serial driver for this? > > Signed-off-by: Max Filippov <jcmvbkbc@xxxxxxxxx> > --- > Changes v2->v3: > - use HZ instead of msecs_to_jiffies(1000) in esp32_acm_put_char_sync > - add early return to esp32_acm_transmit_buffer > - use request_irq/free_irq instead of devm_* versions > > Changes v1->v2: > - redefine register fields using BIT and GENMASK > - drop _MASK suffix from register field definitions > - drop *_SHIFT definitions where possible > - split register reads/writes and bitwise operations into multiple lines > - use u8 instead of unsigned char in internal functions > - add timeout to esp32_acm_put_char_sync > - use uart_port_tx_limited in esp32_acm_transmit_buffer > - use IRQ_RETVAL in esp32_acm_int > - drop esp32s3_acm_console_putchar and esp32s3_acm_earlycon_putchar > - turn num_read into unsigned int in esp32_acm_earlycon_read > - drop MODULE_DESCRIPTION > > drivers/tty/serial/Kconfig | 14 + > drivers/tty/serial/Makefile | 1 + > drivers/tty/serial/esp32_acm.c | 459 +++++++++++++++++++++++++++++++ > include/uapi/linux/serial_core.h | 3 + > 4 files changed, 477 insertions(+) > create mode 100644 drivers/tty/serial/esp32_acm.c > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index d9ca6b268f01..85807db8f7ce 100644 > --- a/drivers/tty/serial/Kconfig > +++ b/drivers/tty/serial/Kconfig > @@ -1591,6 +1591,20 @@ config SERIAL_ESP32 > earlycon=esp32s3uart,mmio32,0x60000000,115200n8,40000000 > earlycon=esp32uart,mmio32,0x3ff40000,115200n8 > > +config SERIAL_ESP32_ACM > + tristate "Espressif ESP32 USB ACM support" > + depends on XTENSA_PLATFORM_ESP32 || (COMPILE_TEST && OF) > + select SERIAL_CORE > + select SERIAL_CORE_CONSOLE > + select SERIAL_EARLYCON > + help > + Driver for the CDC ACM controllers of the Espressif ESP32S3 SoCs > + that share separate USB controller with the JTAG adapter. > + The device name used for this controller is ttyACM. > + When earlycon option is enabled the following kernel command line > + snippet may be used: > + earlycon=esp32s3acm,mmio32,0x60038000 > + > endmenu > > config SERIAL_MCTRL_GPIO > diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile > index 7b73137df7f3..970a292ca418 100644 > --- a/drivers/tty/serial/Makefile > +++ b/drivers/tty/serial/Makefile > @@ -89,6 +89,7 @@ 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 > +obj-$(CONFIG_SERIAL_ESP32_ACM) += esp32_acm.o > > # GPIOLIB helpers for modem control lines > obj-$(CONFIG_SERIAL_MCTRL_GPIO) += serial_mctrl_gpio.o > diff --git a/drivers/tty/serial/esp32_acm.c b/drivers/tty/serial/esp32_acm.c > new file mode 100644 > index 000000000000..f02abd2ac65e > --- /dev/null > +++ b/drivers/tty/serial/esp32_acm.c > @@ -0,0 +1,459 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later Why "or later"? I have to ask, sorry. And no copyright information? That's fine, but be sure your company's lawyers are ok with it... > + > +#include <linux/bitfield.h> > +#include <linux/bits.h> > +#include <linux/console.h> > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/irq.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/serial_core.h> > +#include <linux/slab.h> > +#include <linux/tty_flip.h> > +#include <asm/serial.h> > + > +#define DRIVER_NAME "esp32s3-acm" > +#define DEV_NAME "ttyACM" There is already a ttyACM driver in the kernel, will this conflict with that one? And are you using the same major/minor numbers for it as well? If so, how is this going to work? > +#define UART_NR 4 > + > +#define ESP32S3_ACM_TX_FIFO_SIZE 64 > + > +#define USB_SERIAL_JTAG_EP1_REG 0x00 > +#define USB_SERIAL_JTAG_EP1_CONF_REG 0x04 > +#define USB_SERIAL_JTAG_WR_DONE BIT(0) > +#define USB_SERIAL_JTAG_SERIAL_IN_EP_DATA_FREE BIT(1) > +#define USB_SERIAL_JTAG_INT_ST_REG 0x0c > +#define USB_SERIAL_JTAG_SERIAL_OUT_RECV_PKT_INT_ST BIT(2) > +#define USB_SERIAL_JTAG_SERIAL_IN_EMPTY_INT_ST BIT(3) > +#define USB_SERIAL_JTAG_INT_ENA_REG 0x10 > +#define USB_SERIAL_JTAG_SERIAL_OUT_RECV_PKT_INT_ENA BIT(2) > +#define USB_SERIAL_JTAG_SERIAL_IN_EMPTY_INT_ENA BIT(3) > +#define USB_SERIAL_JTAG_INT_CLR_REG 0x14 > +#define USB_SERIAL_JTAG_IN_EP1_ST_REG 0x2c > +#define USB_SERIAL_JTAG_IN_EP1_WR_ADDR GENMASK(8, 2) > +#define USB_SERIAL_JTAG_OUT_EP1_ST_REG 0x3c > +#define USB_SERIAL_JTAG_OUT_EP1_REC_DATA_CNT GENMASK(22, 16) > + > +static const struct of_device_id esp32s3_acm_dt_ids[] = { > + { > + .compatible = "esp,esp32s3-acm", > + }, { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, esp32s3_acm_dt_ids); > + > +static struct uart_port *esp32s3_acm_ports[UART_NR]; > + > +static void esp32s3_acm_write(struct uart_port *port, unsigned long reg, u32 v) > +{ > + writel(v, port->membase + reg); > +} > + > +static u32 esp32s3_acm_read(struct uart_port *port, unsigned long reg) > +{ > + return readl(port->membase + reg); > +} > + > +static u32 esp32s3_acm_tx_fifo_free(struct uart_port *port) > +{ > + u32 status = esp32s3_acm_read(port, USB_SERIAL_JTAG_EP1_CONF_REG); > + > + return status & USB_SERIAL_JTAG_SERIAL_IN_EP_DATA_FREE; > +} > + > +static u32 esp32s3_acm_tx_fifo_cnt(struct uart_port *port) > +{ > + u32 status = esp32s3_acm_read(port, USB_SERIAL_JTAG_IN_EP1_ST_REG); > + > + return FIELD_GET(USB_SERIAL_JTAG_IN_EP1_WR_ADDR, status); > +} > + > +static u32 esp32s3_acm_rx_fifo_cnt(struct uart_port *port) > +{ > + u32 status = esp32s3_acm_read(port, USB_SERIAL_JTAG_OUT_EP1_ST_REG); > + > + return FIELD_GET(USB_SERIAL_JTAG_OUT_EP1_REC_DATA_CNT, status); > +} > + > +/* return TIOCSER_TEMT when transmitter is not busy */ > +static unsigned int esp32s3_acm_tx_empty(struct uart_port *port) > +{ > + return esp32s3_acm_tx_fifo_cnt(port) == 0 ? TIOCSER_TEMT : 0; > +} > + > +static void esp32s3_acm_set_mctrl(struct uart_port *port, unsigned int mctrl) > +{ > +} Do you have to have empty functions for callbacks that do nothing? > --- a/include/uapi/linux/serial_core.h > +++ b/include/uapi/linux/serial_core.h > @@ -248,4 +248,7 @@ > /* Espressif ESP32 UART */ > #define PORT_ESP32UART 124 > > +/* Espressif ESP32 ACM */ > +#define PORT_ESP32ACM 125 Why are these defines needed? What in userspace is going to require them? If nothing, please do not add them. thanks, greg k-h
