| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2023100326-crushing-septic-4856@gregkh>
Date: Tue, 3 Oct 2023 14:55:41 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Max Filippov <jcmvbkbc@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
devicetree@...r.kernel.org, Jiri Slaby <jirislaby@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH v4 5/5] drivers/tty/serial: add ESP32S3 ACM device driver
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@...il.com>
> ---
> 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
Powered by blists - more mailing lists