Re: [PATCH v1 1/4] usb: serial: mxuport: add support for more MXU50U UART devices

Re: [PATCH v1 1/4] usb: serial: mxuport: add support for more MXU50U UART devices

[Johan Hovold]

On Tue, Mar 24, 2026 at 11:50:38AM +0800, Crescent Hsieh wrote:
> Add support for additional Moxa devices using the MXU50U UART family.

Please mention which models since its not obvious from the PID defines
which mostly reflects the PID itself.

You should amend the driver header as well which lists the currently
supported models.

> Extend the device ID table and port-count handling for 3-, 5-, 6- and
> 7-port devices, and update firmware selection to use the matching
> MXU50U firmware images and version offsets.
> 
> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
> ---
>  drivers/usb/serial/mxuport.c | 132 +++++++++++++++++++++++++++++++----
>  1 file changed, 118 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index ad5fdf55a02e..034b506322c2 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c
> @@ -39,6 +39,25 @@
>  #define MX_UPORT1613_PID	0x1613
>  #define MX_UPORT1653_PID	0x1653
>  
> +#define MX_UPORT1252_PID	0x1252
> +#define MX_UPORT1253_PID	0x1253
> +#define MX_UPORT1411_PID	0x1411
> +#define MX_UPORT1452_PID	0x1452
> +#define MX_UPORT1453_PID	0x1453
> +#define MX_UPORT1619_PID	0x1619
> +#define MX_UPORT1659_PID	0x1659
> +#define MX_UPORT165A_PID	0x165A
> +#define MX_UPORT165B_PID	0x165B
> +
> +#define MX_MU250U_PID		0x0250
> +#define MX_MU450U_PID		0x0450
> +#define MX_MU850U_PID		0x0850
> +
> +#define MX_MU850U_6PORT_PID	0x7002
> +#define MX_MUX50U_3PORT_PID	0x7003
> +#define MX_MU850U_5PORT_PID	0x7004
> +#define MX_MU850U_7PORT_PID	0x7005
> +
>  /* Definitions for USB info */
>  #define HEADER_SIZE		4
>  #define EVENT_LENGTH		8
> @@ -48,6 +67,9 @@
>  #define VER_ADDR_1		0x20
>  #define VER_ADDR_2		0x24
>  #define VER_ADDR_3		0x28
> +#define NEW_ADDR_1		0x86
> +#define NEW_ADDR_2		0x88
> +#define NEW_ADDR_3		0x8A

These are still offsets for the version so please include that in the
name. And "new" tends to get old, better to include the family name, so
something like:

	MXU50U_VER_ADDR_n

>  /* Definitions for USB vendor request */
>  #define RQ_VENDOR_NONE			0x00
> @@ -147,9 +169,13 @@
>  #define MX_WAIT_FOR_SEND_NEXT		0x0080
>  
>  #define MX_UPORT_2_PORT			BIT(0)
> -#define MX_UPORT_4_PORT			BIT(1)
> -#define MX_UPORT_8_PORT			BIT(2)
> -#define MX_UPORT_16_PORT		BIT(3)
> +#define MX_UPORT_3_PORT			BIT(1)
> +#define MX_UPORT_4_PORT			BIT(2)
> +#define MX_UPORT_5_PORT			BIT(3)
> +#define MX_UPORT_6_PORT			BIT(4)
> +#define MX_UPORT_7_PORT			BIT(5)
> +#define MX_UPORT_8_PORT			BIT(6)
> +#define MX_UPORT_16_PORT		BIT(7)

We don't need 8 bits to encode up to 16 ports. Please rebase this on the
patch I just submitted here which cleans this up:

	https://lore.kernel.org/r/20260507141114.284470-1-johan@kernel.org

> @@ -1053,6 +1119,7 @@ static int mxuport_probe(struct usb_serial *serial,
>  	int local_ver;
>  	char buf[32];
>  	int err;
> +	bool is_mux50u = false;
>  
>  	/* Load our firmware */
>  	err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_QUERY_FW_CONFIG, 0, 0);
> @@ -1065,12 +1132,41 @@ static int mxuport_probe(struct usb_serial *serial,
>  	if (err < 0)
>  		return err;
>  
> -	dev_dbg(&serial->interface->dev, "Device firmware version v%x.%x.%x\n",
> +	dev_dbg(&serial->interface->dev, "Device firmware version v%d.%d.%d\n",

This looks like an unrelated change. Please do this in preparatory patch
explaining why you prefer decimal notation.

>  		(version & 0xff0000) >> 16,
>  		(version & 0xff00) >> 8,
>  		(version & 0xff));
>  
> -	snprintf(buf, sizeof(buf) - 1, "moxa/moxa-%04x.fw", productid);
> +	switch (productid) {
> +	case MX_UPORT1252_PID:
> +	case MX_UPORT1253_PID:
> +	case MX_UPORT1411_PID:
> +	case MX_UPORT1452_PID:
> +	case MX_UPORT1453_PID:
> +	case MX_UPORT1619_PID:
> +	case MX_UPORT1659_PID:
> +	case MX_UPORT165A_PID:
> +	case MX_UPORT165B_PID:
> +		is_mux50u = true;
> +		snprintf(buf, sizeof(buf) - 1, "moxa/moxa-up-mux50u.fw");
> +
> +		break;
> +	case MX_MU250U_PID:
> +	case MX_MU450U_PID:
> +	case MX_MU850U_PID:
> +	case MX_MU850U_6PORT_PID:
> +	case MX_MUX50U_3PORT_PID:
> +	case MX_MU850U_5PORT_PID:
> +	case MX_MU850U_7PORT_PID:
> +		is_mux50u = true;
> +		snprintf(buf, sizeof(buf) - 1, "moxa/moxa-pf-mux50u.fw");

We shouldn't have to maintain two tables with PIDs so please encode this
information in the id-table (using two new flags) if you really can't
query the device about this.

We already request the running firmware version. That's not enough to
determine if this is a mux50u device? Perhaps there is some other way?
Can the descriptors be used (e.g. bcdUSB)?

What does "up" vs "pf" stand for? Please mention this in commit message
as well (i.e. the two subfamilies).

> +
> +		break;
> +	default:
> +		snprintf(buf, sizeof(buf) - 1, "moxa/moxa-%04x.fw", productid);
> +
> +		break;
> +	}
>  
>  	err = request_firmware(&fw_p, buf, &serial->interface->dev);
>  	if (err) {
> @@ -1080,14 +1176,22 @@ static int mxuport_probe(struct usb_serial *serial,
>  		/* Use the firmware already in the device */
>  		err = 0;
>  	} else {
> -		local_ver = ((fw_p->data[VER_ADDR_1] << 16) |
> -			     (fw_p->data[VER_ADDR_2] << 8) |
> -			     fw_p->data[VER_ADDR_3]);
> +		if (is_mux50u) {
> +			local_ver = ((fw_p->data[NEW_ADDR_1] << 16) |
> +				     (fw_p->data[NEW_ADDR_2] << 8) |
> +				     (fw_p->data[NEW_ADDR_3]));
> +		} else {
> +			local_ver = ((fw_p->data[VER_ADDR_1] << 16) |
> +				     (fw_p->data[VER_ADDR_2] << 8) |
> +				     (fw_p->data[VER_ADDR_3]));
> +		}

This is probably better handled in a helper function which can make sure
that the right offsets are used depending on the family. For example:

	mxuport_parse_fw_version(const struct firmware *fw, bool mux50u)

>  		dev_dbg(&serial->interface->dev,
> -			"Available firmware version v%x.%x.%x\n",
> -			fw_p->data[VER_ADDR_1], fw_p->data[VER_ADDR_2],
> -			fw_p->data[VER_ADDR_3]);
> -		if (local_ver > version) {
> +			"Available firmware version v%d.%d.%d\n",
> +			(local_ver & 0xff0000) >> 16,
> +			(local_ver & 0xff00) >> 8,
> +			(local_ver & 0xff));
> +
> +		if (local_ver != version) {

This looks like a behaviour change that should be motivated. Why is it
OK to downgrade the device fw?

>  			err = mxuport_download_fw(serial, fw_p);
>  			if (err)
>  				goto out;
> @@ -1098,7 +1202,7 @@ static int mxuport_probe(struct usb_serial *serial,
>  	}
>  
>  	dev_info(&serial->interface->dev,
> -		 "Using device firmware version v%x.%x.%x\n",
> +		 "Using device firmware version v%d.%d.%d\n",
>  		 (version & 0xff0000) >> 16,
>  		 (version & 0xff00) >> 8,
>  		 (version & 0xff));

Johan

Re: [PATCH v1 2/4] usb: serial: mxuport: handle SEND_NEXT tx flow control

[Johan Hovold]

On Tue, Mar 24, 2026 at 11:50:39AM +0800, Crescent Hsieh wrote:
> Track the transmitted payload size per port and stop queueing more data
> once a bulk-out transfer reaches the device buffer threshold.
> 
> Resume transmission when the device reports UPORT_EVENT_SEND_NEXT, and
> reset the TX flow-control state when the port is opened.
> 
> This prevents the driver from queueing more TX data until the device
> reports that it is ready to accept the next transfer.

This explains what the patch does but says nothing about why you think
this is needed (which is the more important part).

The currently supported devices seems to work fine without this and
introducing this scheme should impact throughput negatively.

> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
> ---
>  drivers/usb/serial/mxuport.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index 034b506322c2..4d29a431cefd 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c
> @@ -179,6 +179,8 @@
>  
>  /* This structure holds all of the local port information */
>  struct mxuport_port {
> +	u32 sent_payload;
> +	u32 hold_reason;
>  	u8 mcr_state;		/* Last MCR state */
>  	u8 msr_state;		/* Last MSR state */
>  	struct mutex mutex;	/* Protects mcr_state */
> @@ -250,9 +252,13 @@ MODULE_DEVICE_TABLE(usb, mxuport_idtable);
>  static int mxuport_prepare_write_buffer(struct usb_serial_port *port,
>  					void *dest, size_t size)
>  {
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
>  	u8 *buf = dest;
>  	int count;
>  
> +	if (mxport->hold_reason & MX_WAIT_FOR_SEND_NEXT)
> +		return 0;

The generic write implementation does not support drivers returning zero
here (and has already made sure that there is data in the fifo) so if
this is at all needed that may need to be addressed.

> +
>  	count = kfifo_out_locked(&port->write_fifo, buf + HEADER_SIZE,
>  				 size - HEADER_SIZE,
>  				 &port->lock);
> @@ -263,6 +269,13 @@ static int mxuport_prepare_write_buffer(struct usb_serial_port *port,
>  	dev_dbg(&port->dev, "%s - size %zd count %d\n", __func__,
>  		size, count);
>  
> +	mxport->sent_payload += count;
> +
> +	if (mxport->sent_payload >= port->bulk_out_size) {
> +		mxport->hold_reason |= MX_WAIT_FOR_SEND_NEXT;
> +		buf[0] |= 0x80;
> +	}
> +
>  	return count + HEADER_SIZE;
>  }
>  
> @@ -484,6 +497,9 @@ static void mxuport_lsr_event(struct usb_serial_port *port, u8 buf[4])
>  static void mxuport_process_read_urb_event(struct usb_serial_port *port,
>  					   u8 buf[4], u32 event)
>  {
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +	unsigned long flags;
> +
>  	dev_dbg(&port->dev, "%s - receive event : %04x\n", __func__, event);
>  
>  	switch (event) {
> @@ -492,6 +508,13 @@ static void mxuport_process_read_urb_event(struct usb_serial_port *port,
>  		 * Sent as part of the flow control on device buffers.
>  		 * Not currently used.
>  		 */
> +		if (mxport->hold_reason & MX_WAIT_FOR_SEND_NEXT) {
> +			spin_lock_irqsave(&mxport->spinlock, flags);
> +			mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
> +			mxport->sent_payload = 0;
> +			usb_serial_generic_write_start(port, GFP_ATOMIC);
> +			spin_unlock_irqrestore(&mxport->spinlock, flags);
> +		}

The locking here looks questionable.

>  		break;
>  	case UPORT_EVENT_MSR:
>  		mxuport_msr_event(port, buf);
> @@ -1318,6 +1341,9 @@ static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
>  	 * returns.
>  	 */
>  	mxport->msr_state = 0;
> +	mxport->sent_payload = 0;
> +	mxport->hold_reason = 0;
> +	kfifo_reset(&port->write_fifo);

The fifo is already cleared on close.

>  
>  	return err;
>  }

Johan

Re: [PATCH v1 3/4] usb: serial: mxuport: support serial interface mode configuration

[Johan Hovold]

On Tue, Mar 24, 2026 at 11:50:40AM +0800, Crescent Hsieh wrote:
> Add support for configuring the serial interface mode through
> TIOCSRS485 and TIOCGRS485 using struct serial_rs485.
> 
> Sanitize the requested RS-485 settings and map them to the device
> interface modes before issuing the vendor command to the firmware.
> 
> This allows userspace to switch between RS232, RS422, 2-wire RS485,
> and 4-wire RS485, and to query the current per-port configuration.
> 
> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
 
> +static void mxuport_sanitize_serial_rs485(struct serial_rs485 *rs485)
> +{
> +	if (!(rs485->flags & SER_RS485_ENABLED)) {
> +		memset(rs485, 0, sizeof(*rs485));
> +		return;
> +	}
> +	if (rs485->flags & SER_RS485_MODE_RS422) {
> +		rs485->flags &= (SER_RS485_ENABLED | SER_RS485_MODE_RS422);
> +		return;
> +	}
> +	rs485->flags &= (SER_RS485_ENABLED | SER_RS485_RX_DURING_TX);

I think you need to clear the other unsupported fields as well (i.e. the
delays).

> +
> +	memset(rs485->padding, 0, sizeof(rs485->padding));
> +}
> +
> +static int mxuport_rs485_config(struct usb_serial_port *port,
> +				struct serial_rs485 *rs485)
> +{
> +	struct usb_serial *serial = port->serial;
> +	u16 mode = MX_INT_RS232;
> +
> +	if (rs485->flags & SER_RS485_ENABLED) {
> +		if (rs485->flags & SER_RS485_MODE_RS422)
> +			mode = MX_INT_RS422;
> +		else if (rs485->flags & SER_RS485_RX_DURING_TX)
> +			mode = MX_INT_4W_RS485;
> +		else
> +			mode = MX_INT_2W_RS485;
> +	}
> +
> +	return mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_INTERFACE, mode,
> +				     port->port_number);
> +}
> +
> +static int mxuport_get_rs485_config(struct usb_serial_port *port,
> +				    struct serial_rs485 __user *rs485)
> +{
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +
> +	if (copy_to_user(rs485, &mxport->rs485, sizeof(mxport->rs485)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int mxuport_set_rs485_config(struct usb_serial_port *port,
> +				    struct serial_rs485 __user *rs485_user)
> +{
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +	struct serial_rs485 rs485;
> +	int ret;
> +
> +	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
> +		return -EFAULT;
> +
> +	mxuport_sanitize_serial_rs485(&rs485);
> +
> +	ret = mxuport_rs485_config(port, &rs485);
> +	if (!ret)
> +		mxport->rs485 = rs485;

We need some locking here as these ioctls can execute in parallel. 

> +
> +	if (copy_to_user(rs485_user, &mxport->rs485, sizeof(mxport->rs485)))
> +		return -EFAULT;
> +
> +	return 0;
> +}

Johan

Re: [PATCH v1 4/4] usb: serial: mxuport: add sysfs control for UART FIFO

[Johan Hovold]

On Tue, Mar 24, 2026 at 11:50:41AM +0800, Crescent Hsieh wrote:
> Add a per-port sysfs attribute, uart_fifo, to allow userspace to enable
> or disable the UART FIFO at runtime.

Why would you want to do that?

We should try to avoid driver specific sysfs knobs.

> Map the requested state to the RQ_VENDOR_SET_FIFO_DISABLE vendor command
> and track the current FIFO setting in the per-port private data.
> 
> Initialize the FIFO state during port probe and remove the sysfs
> attribute when the port is released.

Johan