From: Johan Hovold <johan@kernel.org>
To: Crescent Hsieh <crescentcy.hsieh@moxa.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
FangpingFP.Cheng@moxa.com, Epson.Chiang@moxa.com
Subject: Re: [PATCH v2 2/4] USB: serial: mxuport: add MUX50U-based device support
Date: Tue, 21 Jul 2026 16:59:05 +0200 [thread overview]
Message-ID: <al-JOStaDglCBoL1@hovoldconsulting.com> (raw)
In-Reply-To: <20260623080138.166398-4-crescentcy.hsieh@moxa.com>
On Tue, Jun 23, 2026 at 04:01:37PM +0800, Crescent Hsieh wrote:
> Add support for MUX50U-based UPort G2 devices and Platform UARTs.
>
> Encode the firmware family in the USB device id table so that the table
> remains the single source for both the number of ports and the firmware
> handling policy.
>
> The MUX50U-based devices use two shared firmware images: UPort G2
> devices use the UP firmware image (moxa-up-mux50u.fw), while Platform
> UARTs use the PF firmware image (moxa-pf-mux50u.fw). Platform UARTs are
> board-level UARTs accessed through the USB bus.
>
> For MUX50U-based devices, the factory firmware reports versions in a
> different format from the bundled runtime firmware image, so version
> comparison cannot reliably determine whether a firmware download is
> needed. The system-register remap state is used instead to check whether
> the device has already booted the runtime firmware from SRAM.
>
> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
> ---
> drivers/usb/serial/mxuport.c | 174 ++++++++++++++++++++++++++++++++---
> 1 file changed, 162 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index 5a9dd251ceff..067c8d9752e0 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c
> @@ -7,8 +7,11 @@
> *
> * Supports the following Moxa USB to serial converters:
> * 2 ports : UPort 1250, UPort 1250I
> + * UPort 1250 G2, UPort 1250I G2
> * 4 ports : UPort 1410, UPort 1450, UPort 1450I
> + * UPort 1410 G2, UPort 1450 G2, UPort 1450I G2
> * 8 ports : UPort 1610-8, UPort 1650-8
> + * UPort 1610-8 G2, UPort 1650-8 G2
> * 16 ports : UPort 1610-16, UPort 1650-16
Should you add the MUx50U ones here as well for completeness? At least
some of them seem to be missing.
But perhaps it's just me not getting how these PIDs map to products.
> */
>
> @@ -37,6 +40,24 @@
> #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
I realise this is how the existing PID defines are named, but would it
not be possible to name the defines after the products they are used
for?
> +
> +#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
> @@ -46,6 +67,13 @@
> #define MX_FW_VER_BYTE1_OFF 0x20
> #define MX_FW_VER_BYTE2_OFF 0x24
> #define MX_FW_VER_BYTE3_OFF 0x28
> +#define MX_MUX50U_FW_VER_BYTE1_OFF 0x86
> +#define MX_MUX50U_FW_VER_BYTE2_OFF 0x88
> +#define MX_MUX50U_FW_VER_BYTE3_OFF 0x8a
> +
> +#define MX_SYS_REG_REMAP_OFF 0x20
> +#define MX_PWR_REMAP_MASK 0x03
> +#define MX_REMAP_TO_SRAM 0x02
>
> struct mxuport_fw_version {
> u8 major;
> @@ -60,6 +88,12 @@ static const u16 mxuport_fw_ver_offsets[] = {
> MX_FW_VER_BYTE3_OFF,
> };
>
> +static const u16 mxuport_mux50u_fw_ver_offsets[] = {
> + MX_MUX50U_FW_VER_BYTE1_OFF,
> + MX_MUX50U_FW_VER_BYTE2_OFF,
> + MX_MUX50U_FW_VER_BYTE3_OFF,
> +};
> +
> /* Definitions for USB vendor request */
> #define RQ_VENDOR_NONE 0x00
> #define RQ_VENDOR_SET_BAUD 0x01 /* Set baud rate */
> @@ -95,6 +129,7 @@ static const u16 mxuport_fw_ver_offsets[] = {
>
> #define RQ_VENDOR_RESET_DEVICE 0x23 /* Try to reset the device */
> #define RQ_VENDOR_QUERY_FW_CONFIG 0x24
> +#define RQ_VENDOR_GET_SYS_REG 0x34
So here I guess you should have a newline separator before and after
0x34 to follow the existing style (of grouping related registers).
> #define RQ_VENDOR_GET_VERSION 0x81 /* Get firmware version */
> #define RQ_VENDOR_GET_PAGE 0x82 /* Read flash page */
> #define RQ_VENDOR_GET_ROM_PROC 0x83 /* Get ROM process state */
> @@ -169,26 +204,65 @@ struct mxuport_port {
> #define MX_PORTS_OFFSET 1
> #define MX_PORTS(n) (((n) - MX_PORTS_OFFSET) & MX_PORTS_MASK)
>
> +#define MX_FW_FAMILY_MASK GENMASK(5, 4)
> +#define MX_FW_UPORT_G1 (0 << 4)
> +#define MX_FW_UPORT_G2 (1 << 4)
> +#define MX_FW_PLATFORM_UART (2 << 4)
> +#define MX_FW_FAMILY(info) ((info) & MX_FW_FAMILY_MASK)
> +#define MX_DEVICE_INFO(ports, family) (MX_PORTS(ports) | (family))
> +
> /* Table of devices that work with this driver */
> static const struct usb_device_id mxuport_idtable[] = {
> { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1250_PID),
> - .driver_info = MX_PORTS(2) },
> + .driver_info = MX_DEVICE_INFO(2, MX_FW_UPORT_G1) },
> { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1251_PID),
> - .driver_info = MX_PORTS(2) },
> + .driver_info = MX_DEVICE_INFO(2, MX_FW_UPORT_G1) },
> { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1410_PID),
> - .driver_info = MX_PORTS(4) },
> + .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G1) },
> { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1450_PID),
> - .driver_info = MX_PORTS(4) },
> + .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G1) },
> { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1451_PID),
> - .driver_info = MX_PORTS(4) },
> + .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G1) },
> { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1618_PID),
> - .driver_info = MX_PORTS(8) },
> + .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G1) },
> { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1658_PID),
> - .driver_info = MX_PORTS(8) },
> + .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G1) },
> { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1613_PID),
> - .driver_info = MX_PORTS(16) },
> + .driver_info = MX_DEVICE_INFO(16, MX_FW_UPORT_G1) },
> { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1653_PID),
> - .driver_info = MX_PORTS(16) },
My initial reaction was that it would be better to just leave the gen1
entries alone (with family implicitly set to MX_FW_UPORT_G1), but I
guess this is fine too.
> + .driver_info = MX_DEVICE_INFO(16, MX_FW_UPORT_G1) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1252_PID),
> + .driver_info = MX_DEVICE_INFO(2, MX_FW_UPORT_G2) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1253_PID),
> + .driver_info = MX_DEVICE_INFO(2, MX_FW_UPORT_G2) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1411_PID),
> + .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G2) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1452_PID),
> + .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G2) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1453_PID),
> + .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G2) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1619_PID),
> + .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G2) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1659_PID),
> + .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G2) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT165A_PID),
> + .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G2) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT165B_PID),
> + .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G2) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_MU250U_PID),
> + .driver_info = MX_DEVICE_INFO(2, MX_FW_PLATFORM_UART) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_MU450U_PID),
> + .driver_info = MX_DEVICE_INFO(4, MX_FW_PLATFORM_UART) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_MU850U_PID),
> + .driver_info = MX_DEVICE_INFO(8, MX_FW_PLATFORM_UART) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_MU850U_6PORT_PID),
> + .driver_info = MX_DEVICE_INFO(6, MX_FW_PLATFORM_UART) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_MUX50U_3PORT_PID),
> + .driver_info = MX_DEVICE_INFO(3, MX_FW_PLATFORM_UART) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_MU850U_5PORT_PID),
> + .driver_info = MX_DEVICE_INFO(5, MX_FW_PLATFORM_UART) },
> + { USB_DEVICE(MX_USBSERIAL_VID, MX_MU850U_7PORT_PID),
> + .driver_info = MX_DEVICE_INFO(7, MX_FW_PLATFORM_UART) },
> {} /* Terminating entry */
> };
Perhaps the family defines can be shortened to make this a bit more
readable, for example:
MX_FW_UP_G1
MX_FW_UP_G2
MX_FW_PF
or even
MX_UP_G1
MX_UP_G2
MX_PF
I didn't try and see what the result looks like. Perhaps the more
verbose names are preferred.
> @@ -1083,9 +1219,12 @@ static int mxuport_download_fw(struct usb_serial *serial,
> static int mxuport_probe(struct usb_serial *serial,
> const struct usb_device_id *id)
> {
> + unsigned long features = id->driver_info;
> + const u16 *fw_version_offsets;
Nit: reverse xmas tends to be more readable
> u16 productid = le16_to_cpu(serial->dev->descriptor.idProduct);
> const struct firmware *fw_p = NULL;
> struct mxuport_fw_version version, local_ver;
> + bool download_fw, from_sram;
> char buf[32];
> int err;
Johan
next prev parent reply other threads:[~2026-07-21 14:59 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-23 8:01 [PATCH v2 0/4] USB: serial: mxuport: add MUX50U support and updates Crescent Hsieh
2026-06-23 8:01 ` [PATCH v2 1/4] USB: serial: mxuport: clean up firmware version handling Crescent Hsieh
2026-07-21 14:34 ` Johan Hovold
2026-06-23 8:01 ` [PATCH v2 2/4] USB: serial: mxuport: add MUX50U-based device support Crescent Hsieh
2026-07-21 14:59 ` Johan Hovold [this message]
2026-06-23 8:01 ` [PATCH v2 3/4] USB: serial: mxuport: handle SEND_NEXT transmit flow control Crescent Hsieh
2026-07-21 15:51 ` Johan Hovold
2026-06-23 8:01 ` [PATCH v2 4/4] USB: serial: mxuport: support RS485 mode configuration Crescent Hsieh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=al-JOStaDglCBoL1@hovoldconsulting.com \
--to=johan@kernel.org \
--cc=Epson.Chiang@moxa.com \
--cc=FangpingFP.Cheng@moxa.com \
--cc=crescentcy.hsieh@moxa.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox