diff mbox series

[v2] USB: serial: qcserial: Support for SDX55 based Sierra Wireless 5G modules

Message ID 20210611135842.14415-1-stefan.bruens@rwth-aachen.de
State New
Headers show
Series [v2] USB: serial: qcserial: Support for SDX55 based Sierra Wireless 5G modules | expand

Commit Message

Stefan Brüns June 11, 2021, 1:58 p.m. UTC
The devices exposes two different interface compositions:
- QDL mode, single interface
- MBIM mode, MBIM class compliant plus AT/DM(/ADB)

Current firmware versions (up to 01.07.19) do not expose an NMEA port.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/usb/serial/qcserial.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Johan Hovold June 24, 2021, 7:28 a.m. UTC | #1
[ +CC: Daniele and Bjørn ]

On Fri, Jun 11, 2021 at 03:58:41PM +0200, Stefan Brüns wrote:
> The devices exposes two different interface compositions:

> - QDL mode, single interface

> - MBIM mode, MBIM class compliant plus AT/DM(/ADB)

> 

> Current firmware versions (up to 01.07.19) do not expose an NMEA port.


We already have at least one SDX55 based modem (FN980) supported by the
option driver. Any particular reason why you chose to add it to qcserial
instead of option?

Note that the FN980 also needs the ZLP flag set in QDL (flashing) mode,
I'd assume this one needs it too.

Could you please also post the output of usb-devices (or lsusb -v) for
this device in MBIM mode?

> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

> ---

>  drivers/usb/serial/qcserial.c | 29 +++++++++++++++++++++++++++++

>  1 file changed, 29 insertions(+)

> 

> diff --git a/drivers/usb/serial/qcserial.c b/drivers/usb/serial/qcserial.c

> index 83da8236e3c8..4ff325a14c98 100644

> --- a/drivers/usb/serial/qcserial.c

> +++ b/drivers/usb/serial/qcserial.c

> @@ -26,12 +26,15 @@ enum qcserial_layouts {

>  	QCSERIAL_G1K = 1,	/* Gobi 1000 */

>  	QCSERIAL_SWI = 2,	/* Sierra Wireless */

>  	QCSERIAL_HWI = 3,	/* Huawei */

> +	QCSERIAL_SWI2 = 4,	/* Sierra Wireless */

>  };

>  

>  #define DEVICE_G1K(v, p) \

>  	USB_DEVICE(v, p), .driver_info = QCSERIAL_G1K

>  #define DEVICE_SWI(v, p) \

>  	USB_DEVICE(v, p), .driver_info = QCSERIAL_SWI

> +#define DEVICE_SWI2(v, p) \

> +	USB_DEVICE(v, p), .driver_info = QCSERIAL_SWI2

>  #define DEVICE_HWI(v, p) \

>  	USB_DEVICE(v, p), .driver_info = QCSERIAL_HWI

>  

> @@ -181,6 +184,10 @@ static const struct usb_device_id id_table[] = {

>  	{DEVICE_SWI(0x413c, 0x81d1)},   /* Dell Wireless 5818 */

>  	{DEVICE_SWI(0x413c, 0x81d2)},   /* Dell Wireless 5818 */

>  

> +	/* SDX55 based Sierra Wireless devices */

> +	{DEVICE_SWI2(0x1199, 0x90d2)},	/* Sierra Wireless EM919x QDL */

> +	{DEVICE_SWI2(0x1199, 0x90d3)},	/* Sierra Wireless EM919x */

> +

>  	/* Huawei devices */

>  	{DEVICE_HWI(0x03f0, 0x581d)},	/* HP lt4112 LTE/HSPA+ Gobi 4G Modem (Huawei me906e) */

>  

> @@ -359,6 +366,28 @@ static int qcprobe(struct usb_serial *serial, const struct usb_device_id *id)

>  			break;

>  		}

>  		break;

> +	case QCSERIAL_SWI2:

> +		/*

> +		 * Sierra Wireless SDX55 in MBIM mode:

> +		 * 0/1: MBIM Control/Data

> +		 * 3: AT-capable modem port

> +		 * 4: DM/DIAG (use libqcdm from ModemManager for communication)

> +		 * 5: ADB

> +		 */

> +		switch (ifnum) {

> +		case 3:

> +			dev_dbg(dev, "Modem port found\n");

> +			sendsetup = true;

> +			break;

> +		case 4:

> +			dev_dbg(dev, "DM/DIAG interface found\n");

> +			break;

> +		default:

> +			/* don't claim any unsupported interface */

> +			altsetting = -1;

> +			break;

> +		}

> +		break;

>  	case QCSERIAL_HWI:

>  		/*

>  		 * Huawei devices map functions by subclass + protocol


Johan
Bjørn Mork June 24, 2021, 11:15 a.m. UTC | #2
Johan Hovold <johan@kernel.org> writes:

> Could you please also post the output of usb-devices (or lsusb -v) for

> this device in MBIM mode?


Yes, this would be nice to have.

I suspect that this device is like other SDX55 devices we've seen, using
class/subclass/function to map the vendor specific functions
too. Dropping static interface numbers.  If correct, then the patch is
bogus and the interface numbers might change based on firmware version
and configuration.


Bjørn
Stefan Brüns July 1, 2021, 4:28 p.m. UTC | #3
On Donnerstag, 24. Juni 2021 13:15:07 CEST Bjørn Mork wrote:
> Johan Hovold <johan@kernel.org> writes:

> > Could you please also post the output of usb-devices (or lsusb -v) for

> > this device in MBIM mode?

> 

> Yes, this would be nice to have.


See below.
 
> I suspect that this device is like other SDX55 devices we've seen, using

> class/subclass/function to map the vendor specific functions

> too. Dropping static interface numbers.  If correct, then the patch is

> bogus and the interface numbers might change based on firmware version

> and configuration.


Do you really expect Sierra do to something sensible? According to their 
documentation functions are matched by interface numbers.

They still use broken interface descriptors with holes in interface numbering 
(i.e. interface number 2 does not exist, which violates the USB standard).

Regards, Stefan


lsusb -v -d 1199:


Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.10
  bDeviceClass            0 
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0        64
  idVendor           0x1199 Sierra Wireless, Inc.
  idProduct          0x90d3 
  bcdDevice            0.06
  iManufacturer           1 Sierra Wireless, Incorporated
  iProduct                2 Sierra Wireless EM9191
  iSerial                 3 8W<nnnnnnnnnnnnn>
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x00a7
    bNumInterfaces          4
    bConfigurationValue     1
    iConfiguration          4 
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
    MaxPower              500mA
    Interface Association:
      bLength                 8
      bDescriptorType        11
      bFirstInterface         0
      bInterfaceCount         2
      bFunctionClass          2 Communications
      bFunctionSubClass      14 
      bFunctionProtocol       0 
      iFunction               0 
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         2 Communications
      bInterfaceSubClass     14 
      bInterfaceProtocol      0 
      iInterface              0 
      CDC Header:
        bcdCDC               1.10
      CDC Union:
        bMasterInterface        0
        bSlaveInterface         1 
      CDC MBIM:
        bcdMBIMVersion       1.00
        wMaxControlMessage   4096
        bNumberFilters       32
        bMaxFilterSize       128
        wMaxSegmentSize      2048
        bmNetworkCapabilities 0x20
          8-byte ntb input size
      CDC MBIM Extended:
        bcdMBIMExtendedVersion           1.00
        bMaxOutstandingCommandMessages     64
        wMTU                             1500
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               9
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       0
      bNumEndpoints           0
      bInterfaceClass        10 CDC Data
      bInterfaceSubClass      0 
      bInterfaceProtocol      2 
      iInterface              0 
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       1
      bNumEndpoints           2
      bInterfaceClass        10 CDC Data
      bInterfaceSubClass      0 
      bInterfaceProtocol      2 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x8e  EP 14 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0200  1x 512 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x0f  EP 15 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0200  1x 512 bytes
        bInterval               0
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        3
      bAlternateSetting       0
      bNumEndpoints           3
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0 
      bInterfaceProtocol      0 
      iInterface              0 
      ** UNRECOGNIZED:  05 24 00 10 01
      ** UNRECOGNIZED:  05 24 01 00 00
      ** UNRECOGNIZED:  04 24 02 02
      ** UNRECOGNIZED:  05 24 06 00 00
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x000a  1x 10 bytes
        bInterval               9
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0200  1x 512 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x01  EP 1 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0200  1x 512 bytes
        bInterval               0
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        4
      bAlternateSetting       0
      bNumEndpoints           2
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass    255 Vendor Specific Subclass
      bInterfaceProtocol     48 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0200  1x 512 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0200  1x 512 bytes
        bInterval               0




-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
Stefan Brüns July 1, 2021, 4:41 p.m. UTC | #4
On Donnerstag, 24. Juni 2021 09:28:59 CEST Johan Hovold wrote:
> [ +CC: Daniele and Bjørn ]

> 

> On Fri, Jun 11, 2021 at 03:58:41PM +0200, Stefan Brüns wrote:

> > The devices exposes two different interface compositions:

> > - QDL mode, single interface

> > - MBIM mode, MBIM class compliant plus AT/DM(/ADB)

> > 

> > Current firmware versions (up to 01.07.19) do not expose an NMEA port.

> 

> We already have at least one SDX55 based modem (FN980) supported by the

> option driver. Any particular reason why you chose to add it to qcserial

> instead of option?

> 

Support for qualcomm based modems are spread over option and qcserial. All 
other Sierra devices are supported by qcserial.
 
> Note that the FN980 also needs the ZLP flag set in QDL (flashing) mode,

> I'd assume this one needs it too.


It depends if you implement the Firehose protocol in accordance to the 
specification or not. 80-NG319-1 (Firehose specification) explicitly states to 
pad any XML command packet which is an exact multiple of 512 bytes with an 
extra newline or other whitespace character.

Regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
Bjørn Mork July 2, 2021, 7:32 a.m. UTC | #5
Stefan Brüns <stefan.bruens@rwth-aachen.de> writes:

> On Donnerstag, 24. Juni 2021 13:15:07 CEST Bjørn Mork wrote:

>> Johan Hovold <johan@kernel.org> writes:

>> > Could you please also post the output of usb-devices (or lsusb -v) for

>> > this device in MBIM mode?

>> 

>> Yes, this would be nice to have.

>

> See below.

>  

>> I suspect that this device is like other SDX55 devices we've seen, using

>> class/subclass/function to map the vendor specific functions

>> too. Dropping static interface numbers.  If correct, then the patch is

>> bogus and the interface numbers might change based on firmware version

>> and configuration.

>

> Do you really expect Sierra do to something sensible? According to their 

> documentation functions are matched by interface numbers.


Well... I expect them to be as sensible as any other vendor.  And I
expect docs to be intendend as guidance only ;-)

> They still use broken interface descriptors with holes in interface numbering 

> (i.e. interface number 2 does not exist, which violates the USB standard).


Right. Wrt the violation, I think that train left a decade ago.  

>     Interface Descriptor:

>       bLength                 9

>       bDescriptorType         4

>       bInterfaceNumber        3

>       bAlternateSetting       0

>       bNumEndpoints           3

>       bInterfaceClass       255 Vendor Specific Class

>       bInterfaceSubClass      0 

>       bInterfaceProtocol      0 

[..]
>     Interface Descriptor:

>       bLength                 9

>       bDescriptorType         4

>       bInterfaceNumber        4

>       bAlternateSetting       0

>       bNumEndpoints           2

>       bInterfaceClass       255 Vendor Specific Class

>       bInterfaceSubClass    255 Vendor Specific Subclass

>       bInterfaceProtocol     48 



So Sierra do follow the same pattern we've seen on other X55 devices:

ff/00/00 - AT
ff/ff/30 - QCDM

See commits

 accf227de4d2 ("USB: serial: option: Add support for Quectel RM500Q")
 d6c1ddd938d8 ("USB: serial: option: add Quectel EM160R-GL")

for other examples.

This obviously doesn't make any difference if your configuration is the
only one.  But I believe that is unlikely.  There are probably ways the
layout can be changed, even if currenly not documented.  The advantage
of class/subclass/protocol matching to function is that it works
regardless of the number of functions and their interface number.


Bjørn
Johan Hovold July 2, 2021, 8:27 a.m. UTC | #6
On Thu, Jul 01, 2021 at 06:41:26PM +0200, Stefan Brüns wrote:
> On Donnerstag, 24. Juni 2021 09:28:59 CEST Johan Hovold wrote:

> > [ +CC: Daniele and Bjørn ]

> > 

> > On Fri, Jun 11, 2021 at 03:58:41PM +0200, Stefan Brüns wrote:

> > > The devices exposes two different interface compositions:

> > > - QDL mode, single interface

> > > - MBIM mode, MBIM class compliant plus AT/DM(/ADB)

> > > 

> > > Current firmware versions (up to 01.07.19) do not expose an NMEA port.

> > 

> > We already have at least one SDX55 based modem (FN980) supported by the

> > option driver. Any particular reason why you chose to add it to qcserial

> > instead of option?

> > 

> Support for qualcomm based modems are spread over option and qcserial. All 

> other Sierra devices are supported by qcserial.


Ok, but we may still end up adding this one to option if matching on the
interface protocol works.

> > Note that the FN980 also needs the ZLP flag set in QDL (flashing) mode,

> > I'd assume this one needs it too.

> 

> It depends if you implement the Firehose protocol in accordance to the 

> specification or not. 80-NG319-1 (Firehose specification) explicitly states to 

> pad any XML command packet which is an exact multiple of 512 bytes with an 

> extra newline or other whitespace character.


Thanks for the details. If you're referring to the device-side
implementation it seems a bit fragile to not just set the ZLP flag since
apparently there are some non-standard implementations out there. But
sure, we can do that later if needed.

Johan
diff mbox series

Patch

diff --git a/drivers/usb/serial/qcserial.c b/drivers/usb/serial/qcserial.c
index 83da8236e3c8..4ff325a14c98 100644
--- a/drivers/usb/serial/qcserial.c
+++ b/drivers/usb/serial/qcserial.c
@@ -26,12 +26,15 @@  enum qcserial_layouts {
 	QCSERIAL_G1K = 1,	/* Gobi 1000 */
 	QCSERIAL_SWI = 2,	/* Sierra Wireless */
 	QCSERIAL_HWI = 3,	/* Huawei */
+	QCSERIAL_SWI2 = 4,	/* Sierra Wireless */
 };
 
 #define DEVICE_G1K(v, p) \
 	USB_DEVICE(v, p), .driver_info = QCSERIAL_G1K
 #define DEVICE_SWI(v, p) \
 	USB_DEVICE(v, p), .driver_info = QCSERIAL_SWI
+#define DEVICE_SWI2(v, p) \
+	USB_DEVICE(v, p), .driver_info = QCSERIAL_SWI2
 #define DEVICE_HWI(v, p) \
 	USB_DEVICE(v, p), .driver_info = QCSERIAL_HWI
 
@@ -181,6 +184,10 @@  static const struct usb_device_id id_table[] = {
 	{DEVICE_SWI(0x413c, 0x81d1)},   /* Dell Wireless 5818 */
 	{DEVICE_SWI(0x413c, 0x81d2)},   /* Dell Wireless 5818 */
 
+	/* SDX55 based Sierra Wireless devices */
+	{DEVICE_SWI2(0x1199, 0x90d2)},	/* Sierra Wireless EM919x QDL */
+	{DEVICE_SWI2(0x1199, 0x90d3)},	/* Sierra Wireless EM919x */
+
 	/* Huawei devices */
 	{DEVICE_HWI(0x03f0, 0x581d)},	/* HP lt4112 LTE/HSPA+ Gobi 4G Modem (Huawei me906e) */
 
@@ -359,6 +366,28 @@  static int qcprobe(struct usb_serial *serial, const struct usb_device_id *id)
 			break;
 		}
 		break;
+	case QCSERIAL_SWI2:
+		/*
+		 * Sierra Wireless SDX55 in MBIM mode:
+		 * 0/1: MBIM Control/Data
+		 * 3: AT-capable modem port
+		 * 4: DM/DIAG (use libqcdm from ModemManager for communication)
+		 * 5: ADB
+		 */
+		switch (ifnum) {
+		case 3:
+			dev_dbg(dev, "Modem port found\n");
+			sendsetup = true;
+			break;
+		case 4:
+			dev_dbg(dev, "DM/DIAG interface found\n");
+			break;
+		default:
+			/* don't claim any unsupported interface */
+			altsetting = -1;
+			break;
+		}
+		break;
 	case QCSERIAL_HWI:
 		/*
 		 * Huawei devices map functions by subclass + protocol