[PATCH v2 0/2] Add support for Infineon/Intel XMM6260 modem

[PATCH v2 0/2] Add support for Infineon/Intel XMM6260 modem

[Svyatoslav Ryhel]

The Infineon/Intel XMM6260 is a 3G-focused, slim modem platform designed
for smartphones, data cards, and Machine-to-Machine (M2M) applications.

The modem is typically connected via the application processor's USB line
in HSIC mode. To function correctly, the modem must control this line, as
it requires precise timing to initiate or de-initialize the USB connection.
This control is necessary to successfully enumerate the next stage of the
USB device loader (moving from firmware loading to the actual device
interface for example).

Patchset adds support for the generic portion of the Infineon XMM6260
baseband modem, which was used in many Tegra-, OMAP-, and Exynos-based
devices circa 2012. This driver provides generic power sequences,
manages initial communication with the application processor, handles the
SoC-specific modem powersequence, and verifies that the modem USB device
appears correctly.

While current support is relatively basic, this configuration already
allows the modem device to appear in the dmesg of my device
(LG Optimus Vu (P895)):

[    9.427014] ci_hdrc ci_hdrc.1: EHCI Host Controller
[    9.431488] ci_hdrc ci_hdrc.1: new USB bus registered, assigned bus number 1
[    9.457197] ci_hdrc ci_hdrc.1: USB 2.0 started, EHCI 1.00
[    9.460370] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 6.16
[    9.468470] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[    9.475597] usb usb1: Product: EHCI Host Controller
[    9.480508] usb usb1: Manufacturer: Linux 6.16.0+ ehci_hcd
[    9.485913] usb usb1: SerialNumber: ci_hdrc.1
[    9.490862] hub 1-0:1.0: USB hub found
[    9.494005] hub 1-0:1.0: 1 port detected
[    9.657191] usb 1-1: new high-speed USB device number 2 using ci_hdrc
[    9.844726] usb 1-1: New USB device found, idVendor=1519, idProduct=0020, bcdDevice=12.74
[    9.850530] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[    9.857594] usb 1-1: Product: HSIC Device
[    9.861606] usb 1-1: Manufacturer: Comneon
[    9.865627] usb 1-1: SerialNumber: 0123456789
[    9.908739] cdc_acm 1-1:1.0: ttyACM0: USB ACM device

This patchset is a part of larger series aiming to bring XMM6260 modem
support for Tegra devices:
https://lore.kernel.org/lkml/20260511135703.62470-1-clamor95@gmail.com/

---
Changes in v2:
- changed phy to pwrseq in schema
- adjusted Kconfig dependencies
- implemented bitmap for modem state tracking
- switched from phy to power sequencing
- in notifier added chech to filter only USB events
- in notifier added USB_DEVICE_REMOVE
- added tracking for regulator, rfkill access, usb device
  presence and poweroff calls using bitops
- moved pseq on call from work to irq handler
- improved rfkill registration logic
---

Svyatoslav Ryhel (2):
  dt-bindings: net: Document Infineon/Intel XMM6260 modem
  net: usb: Add Infineon XMM6260 Baseband modem support

 .../bindings/net/infineon,xmm6260.yaml        |  74 ++++
 drivers/net/usb/Kconfig                       |  15 +
 drivers/net/usb/Makefile                      |   1 +
 drivers/net/usb/baseband-xmm6260.c            | 378 ++++++++++++++++++
 4 files changed, 468 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/infineon,xmm6260.yaml
 create mode 100644 drivers/net/usb/baseband-xmm6260.c

-- 
2.51.0

[PATCH v2 1/2] dt-bindings: net: Document Infineon/Intel XMM6260 modem

[Svyatoslav Ryhel]

Describe the Infineon/Intel XMM6260, a 3G-focused, slim modem platform
designed for smartphones, data cards, and Machine-to-Machine (M2M)
applications.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 .../bindings/net/infineon,xmm6260.yaml        | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/infineon,xmm6260.yaml

diff --git a/Documentation/devicetree/bindings/net/infineon,xmm6260.yaml b/Documentation/devicetree/bindings/net/infineon,xmm6260.yaml
new file mode 100644
index 000000000000..ffff58e479ef
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/infineon,xmm6260.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/infineon,xmm6260.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Infineon/Intel XMM6260 embedded USB modem
+
+description:
+  The Infineon/Intel XMM6260 is a 3G-focused, slim modem platform designed
+  for smartphones, data cards, and Machine-to-Machine (M2M) applications.
+  The modem is usually connected via the application processor's USB line
+  in HSIC mode; however, to work properly, the modem must control this line.
+
+maintainers:
+  - Svyatoslav Ryhel <clamor95@gmail.com>
+
+properties:
+  compatible:
+    const: infineon,xmm6260
+
+  interrupts:
+    maxItems: 1
+
+  enable-gpios:
+    description: GPIO connected to the ON1 pin
+    maxItems: 1
+
+  reset-gpios:
+    description: GPIO connected to the RESET_PWRDWN_N pin
+    maxItems: 1
+
+  ap-wake-gpios:
+    description: GPIO connected to the EINT3 pin
+    maxItems: 1
+
+  cp-wake-gpios:
+    description: GPIO connected to the EINT2 pin
+    maxItems: 1
+
+  vbat-supply:
+    description: Supply connected to the VBAT lines.
+
+  infineon,modem-pwrseq:
+    description:
+      Contains phandle pointing to the modem's power sequence.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+required:
+  - compatible
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    modem {
+        compatible = "infineon,xmm6260";
+
+        interrupt-parent = <&gpio>;
+        interrupts = <168 IRQ_TYPE_EDGE_BOTH>;
+
+        enable-gpios = <&gpio 112 GPIO_ACTIVE_HIGH>;
+        reset-gpios = <&gpio 169 GPIO_ACTIVE_LOW>;
+
+        cp-wake-gpios = <&gpio 151 GPIO_ACTIVE_HIGH>;
+        ap-wake-gpios = <&gpio 168 GPIO_ACTIVE_HIGH>;
+
+        infineon,modem-pwrseq = <&xmm6260_modem_pwrseq>;
+        vbat-supply = <&vdd_3v3_vbat>;
+    };
-- 
2.51.0

[PATCH v2 2/2] net: usb: Add Infineon XMM6260 Baseband modem support

[Svyatoslav Ryhel]

The Infineon/Intel XMM6260 is a 3G-focused, slim modem platform designed
for smartphones, data cards, and Machine-to-Machine (M2M) applications.
The modem is usually connected via the application processor's USB line
in HSIC mode; however, to work properly, the modem must control this line

Dmesg with modem appearing on LG Optimus Vu (P895):

[    9.427014] ci_hdrc ci_hdrc.1: EHCI Host Controller
[    9.431488] ci_hdrc ci_hdrc.1: new USB bus registered, assigned bus number 1
[    9.457197] ci_hdrc ci_hdrc.1: USB 2.0 started, EHCI 1.00
[    9.460370] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 6.16
[    9.468470] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[    9.475597] usb usb1: Product: EHCI Host Controller
[    9.480508] usb usb1: Manufacturer: Linux 6.16.0+ ehci_hcd
[    9.485913] usb usb1: SerialNumber: ci_hdrc.1
[    9.490862] hub 1-0:1.0: USB hub found
[    9.494005] hub 1-0:1.0: 1 port detected
[    9.657191] usb 1-1: new high-speed USB device number 2 using ci_hdrc
[    9.844726] usb 1-1: New USB device found, idVendor=1519, idProduct=0020, bcdDevice=12.74
[    9.850530] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[    9.857594] usb 1-1: Product: HSIC Device
[    9.861606] usb 1-1: Manufacturer: Comneon
[    9.865627] usb 1-1: SerialNumber: 0123456789
[    9.908739] cdc_acm 1-1:1.0: ttyACM0: USB ACM device

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/net/usb/Kconfig            |  15 ++
 drivers/net/usb/Makefile           |   1 +
 drivers/net/usb/baseband-xmm6260.c | 378 +++++++++++++++++++++++++++++
 3 files changed, 394 insertions(+)
 create mode 100644 drivers/net/usb/baseband-xmm6260.c

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index 52a5c0922c79..d54d8db752df 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -642,4 +642,19 @@ config USB_RTL8153_ECM
 	  CONFIG_USB_RTL8152 is not set, or the RTL8153 device is not
 	  supported by r8152 driver.
 
+config USB_NET_XMM6260
+	tristate "Infineon XMM626X Baseband HSPA/HSUPA modem"
+	depends on USB_CHIPIDEA && (RFKILL || RFKILL=n)
+	help
+	  Select this if you want to use an Infineon XMM626X modem, found in
+	  devices such as the LG Optimus 4X P880, LG Optimus Vu P895, Samsung
+	  Galaxy S II (GT-I9100), and Galaxy Nexus (GT-I9250). This driver
+	  handles the modem configuration and provides a stable way to expose
+	  the modem's USB interface. To establish a connection, you will first
+	  need a userspace program to send the correct commands to the modem
+	  through its CDC ACM port, as well as a DHCP client.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called baseband-xmm6260.
+
 endif # USB_NET_DRIVERS
diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
index 4964f7b326fb..ffa532c7d7d6 100644
--- a/drivers/net/usb/Makefile
+++ b/drivers/net/usb/Makefile
@@ -42,3 +42,4 @@ obj-$(CONFIG_USB_NET_CDC_MBIM)	+= cdc_mbim.o
 obj-$(CONFIG_USB_NET_CH9200)	+= ch9200.o
 obj-$(CONFIG_USB_NET_AQC111)	+= aqc111.o
 obj-$(CONFIG_USB_RTL8153_ECM)	+= r8153_ecm.o
+obj-$(CONFIG_USB_NET_XMM6260)	+= baseband-xmm6260.o
diff --git a/drivers/net/usb/baseband-xmm6260.c b/drivers/net/usb/baseband-xmm6260.c
new file mode 100644
index 000000000000..557ec79f5e2a
--- /dev/null
+++ b/drivers/net/usb/baseband-xmm6260.c
@@ -0,0 +1,378 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2011 NVIDIA Corporation
+ * Copyright (C) 2023 Svyatoslav Ryhel <clamor95@gmail.com>
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/pwrseq/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/rfkill.h>
+#include <linux/usb.h>
+
+#define BASEBAND_XMM_INIT_DELAY		5000
+
+#define BASEBAND_PRODUCT_ID_XMM6260	0x0020
+#define BASEBAND_VENDOR_ID_COMNEON	0x1519
+
+/*
+ * Starting from ver 1145 modem starts in the IPC_AP_WAKE_IRQ_READY state.
+ * AP wake interrupt keeps low util CP starts to initiate its HSIC hw. AP wake
+ * interrupt goes up during CP HSIC init (BASEBAND_XMM_IPC_AP_WAKE_INIT1 state)
+ * at this point Host USB bus must be configured in order for modem to set
+ * properly. Then interrupt goes down when CP HSIC is ready
+ * (BASEBAND_XMM_IPC_AP_WAKE_INIT2 state) and at this point XMM6260 USB device
+ * should appear and be accessible for further work. In case it does not, the
+ * cycle must repeat.
+ */
+
+/* bits [0:2] */
+enum baseband_xmm_ipc_ap_wake_state {
+	BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY,
+	BASEBAND_XMM_IPC_AP_WAKE_INIT1,
+	BASEBAND_XMM_IPC_AP_WAKE_INIT2,
+	BASEBAND_XMM_IPC_AP_WAKE_L,
+	BASEBAND_XMM_IPC_AP_WAKE_H,
+	BASEBAND_XMM_IPC_AP_WAKE_UNINIT,
+	BASEBAND_XMM_IPC_AP_WAKE_MASK = 7,
+};
+
+#define BASEBAND_XMM_IPC_AP_WAKE_MAX	3
+
+#define BASEBAND_XMM_STATE_POWERED	3 /* Tracks regulator state */
+#define BASEBAND_XMM_STATE_PROTECTED	4 /* Prevents rfkill from access */
+#define BASEBAND_XMM_STATE_PRESENT	5 /* Tracks USB device presence */
+#define BASEBAND_XMM_STATE_POWEROFF	6 /* Prevents poweroff recursive call */
+#define BASEBAND_XMM_STATE_MAX		8
+
+struct baseband_xmm_data {
+	struct device *dev;
+	struct rfkill *rfkill_dev;
+	struct pwrseq_desc *pwrseq;
+
+	DECLARE_BITMAP(state, BASEBAND_XMM_STATE_MAX);
+	int irq;
+
+	struct gpio_desc *reset_gpio;
+	struct gpio_desc *enable_gpio;
+
+	struct gpio_desc *ipc_cp_gpio;
+	struct gpio_desc *ipc_ap_gpio;
+
+	struct regulator *vbat_supply;
+
+	struct delayed_work modem_work;
+	struct notifier_block nb;
+};
+
+static int get_ipc_ap_wake(struct baseband_xmm_data *priv)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < BASEBAND_XMM_IPC_AP_WAKE_MAX; i++)
+		ret |= (test_bit(i, priv->state) << i);
+
+	return ret;
+}
+
+static void set_ipc_ap_wake(struct baseband_xmm_data *priv,
+			    enum baseband_xmm_ipc_ap_wake_state state)
+{
+	for (int i = 0; i < BASEBAND_XMM_IPC_AP_WAKE_MAX; i++)
+		if (state & BIT(i))
+			set_bit(i, priv->state);
+		else
+			clear_bit(i, priv->state);
+}
+
+static void baseband_xmm_reset(struct baseband_xmm_data *priv)
+{
+	int ret;
+
+	set_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
+
+	if (!test_bit(BASEBAND_XMM_STATE_POWERED, priv->state)) {
+		ret = regulator_enable(priv->vbat_supply);
+		if (ret)
+			dev_err(priv->dev,
+				"failed to enable vbat power supply\n");
+
+		set_bit(BASEBAND_XMM_STATE_POWERED, priv->state);
+	}
+
+	gpiod_set_value_cansleep(priv->enable_gpio, 0);
+	msleep(50);
+
+	gpiod_set_value_cansleep(priv->reset_gpio, 1);
+	msleep(200);
+	gpiod_set_value_cansleep(priv->reset_gpio, 0);
+
+	msleep(50);
+
+	/* falling edge trigger to CP */
+	gpiod_set_value_cansleep(priv->enable_gpio, 1);
+	usleep_range(50, 100);
+	gpiod_set_value_cansleep(priv->enable_gpio, 0);
+	msleep(20);
+}
+
+static void baseband_xmm_poweroff(struct baseband_xmm_data *priv)
+{
+	/*
+	 * The test_bit check prevents poweroff from being recursively
+	 * called during USB device deregistration. USB device
+	 * deregistration can be triggered by the driver by calling this
+	 * function or by some external event. The first case will cause
+	 * a recursive call by the notifier if not handled, while the
+	 * second case requires this call to handle the USB controller
+	 * properly.
+	 */
+	if (test_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state))
+		return;
+
+	set_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
+	set_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state);
+
+	pwrseq_power_off(priv->pwrseq);
+	gpiod_set_value_cansleep(priv->reset_gpio, 1);
+
+	if (test_bit(BASEBAND_XMM_STATE_POWERED, priv->state)) {
+		regulator_disable(priv->vbat_supply);
+		clear_bit(BASEBAND_XMM_STATE_POWERED, priv->state);
+	}
+	set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY);
+
+	clear_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
+	clear_bit(BASEBAND_XMM_STATE_PRESENT, priv->state);
+	clear_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state);
+}
+
+static int baseband_xmm_usb_notifier_call(struct notifier_block *nb,
+					  unsigned long action, void *data)
+{
+	struct baseband_xmm_data *priv =
+		container_of(nb, struct baseband_xmm_data, nb);
+	struct usb_device *udev;
+	u16 product, vendor;
+
+	if (action == USB_BUS_ADD || action == USB_BUS_REMOVE)
+		return NOTIFY_OK;
+
+	udev = data;
+	product = le16_to_cpu(udev->descriptor.idProduct);
+	vendor = le16_to_cpu(udev->descriptor.idVendor);
+
+	switch (action) {
+	case USB_DEVICE_ADD:
+		/* Infineon XMM6260 ID 1519:0020 */
+		if (vendor == BASEBAND_VENDOR_ID_COMNEON &&
+		    product == BASEBAND_PRODUCT_ID_XMM6260) {
+			cancel_delayed_work_sync(&priv->modem_work);
+			clear_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
+			set_bit(BASEBAND_XMM_STATE_PRESENT, priv->state);
+		}
+		break;
+
+	case USB_DEVICE_REMOVE:
+		/* Infineon XMM6260 ID 1519:0020 */
+		if (vendor == BASEBAND_VENDOR_ID_COMNEON &&
+		    product == BASEBAND_PRODUCT_ID_XMM6260)
+			baseband_xmm_poweroff(priv);
+		break;
+
+	default:
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static int baseband_xmm_set_block(void *data, bool blocked)
+{
+	struct baseband_xmm_data *priv = data;
+
+	if (test_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state))
+		return -EBUSY;
+
+	if (blocked && test_bit(BASEBAND_XMM_STATE_PRESENT, priv->state))
+		baseband_xmm_poweroff(priv);
+	else if (!blocked && !test_bit(BASEBAND_XMM_STATE_PRESENT, priv->state))
+		baseband_xmm_reset(priv);
+
+	return 0;
+}
+
+static const struct rfkill_ops baseband_xmm_rfkill_ops = {
+	.set_block = baseband_xmm_set_block,
+};
+
+static void baseband_xmm_work(struct work_struct *work)
+{
+	struct baseband_xmm_data *priv =
+		container_of(work, struct baseband_xmm_data, modem_work.work);
+
+	baseband_xmm_poweroff(priv);
+};
+
+static irqreturn_t baseband_hostwake_interrupt(int irq, void *dev_id)
+{
+	struct baseband_xmm_data *priv = dev_id;
+	int state = gpiod_get_value(priv->ipc_ap_gpio);
+
+	switch (get_ipc_ap_wake(priv)) {
+	case BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY:
+		if (!state) {
+			set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_INIT1);
+			pwrseq_power_on(priv->pwrseq);
+		}
+		break;
+
+	case BASEBAND_XMM_IPC_AP_WAKE_INIT1:
+		if (state) {
+			set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_INIT2);
+			schedule_delayed_work(&priv->modem_work,
+					      msecs_to_jiffies(BASEBAND_XMM_INIT_DELAY));
+		}
+		break;
+
+	default:
+		break;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int baseband_xmm_probe(struct platform_device *pdev)
+{
+	struct baseband_xmm_data *priv;
+	struct device *dev = &pdev->dev;
+	unsigned long irqflags;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	platform_set_drvdata(pdev, priv);
+
+	priv->vbat_supply = devm_regulator_get(dev, "vbat");
+	if (IS_ERR(priv->vbat_supply))
+		return dev_err_probe(dev, PTR_ERR(priv->vbat_supply),
+				     "failed to get vbat regulator\n");
+
+	/* Own modem gpios */
+	priv->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						   GPIOD_OUT_LOW);
+	if (IS_ERR(priv->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(priv->reset_gpio),
+				     "failed to get reset GPIO\n");
+
+	priv->enable_gpio = devm_gpiod_get_optional(dev, "enable",
+						    GPIOD_OUT_LOW);
+	if (IS_ERR(priv->enable_gpio))
+		return dev_err_probe(dev, PTR_ERR(priv->enable_gpio),
+				     "failed to get enable GPIO\n");
+
+	/* CP - AP connections */
+	priv->ipc_cp_gpio = devm_gpiod_get_optional(dev, "cp-wake",
+						    GPIOD_OUT_LOW);
+	if (IS_ERR(priv->ipc_cp_gpio))
+		return dev_err_probe(dev, PTR_ERR(priv->ipc_cp_gpio),
+				     "failed to get CP wake GPIO\n");
+
+	priv->ipc_ap_gpio = devm_gpiod_get_optional(dev, "ap-wake", GPIOD_IN);
+	if (IS_ERR(priv->ipc_ap_gpio))
+		return dev_err_probe(dev, PTR_ERR(priv->ipc_ap_gpio),
+				     "failed to get AP wake GPIO\n");
+
+	/* Modem power sequence */
+	priv->pwrseq = devm_pwrseq_get(dev, "modem-power");
+	if (IS_ERR(priv->pwrseq))
+		return dev_err_probe(dev, PTR_ERR(priv->pwrseq),
+				     "failed to get modem pwrseq");
+
+	bitmap_zero(priv->state, BASEBAND_XMM_STATE_MAX);
+	INIT_DELAYED_WORK(&priv->modem_work, baseband_xmm_work);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0)
+		return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
+
+	/*
+	 * Systems using device tree should set up interrupt via DT,
+	 * the rest will use the default edge both interrupt.
+	 */
+	irqflags = dev->of_node ? 0 : IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
+
+	ret = devm_request_threaded_irq(dev, priv->irq, NULL,
+					&baseband_hostwake_interrupt,
+					IRQF_ONESHOT | irqflags,
+					"modem-hostwake", priv);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register IRQ %d\n", priv->irq);
+
+	priv->rfkill_dev = rfkill_alloc("xmm-modem", dev, RFKILL_TYPE_WWAN,
+					&baseband_xmm_rfkill_ops, priv);
+	if (!priv->rfkill_dev)
+		return -ENOMEM;
+
+	ret = rfkill_register(priv->rfkill_dev);
+	if (ret) {
+		rfkill_destroy(priv->rfkill_dev);
+		return dev_err_probe(dev, ret,
+				     "failed to register WWAN rfkill\n");
+	}
+
+	priv->nb.notifier_call = baseband_xmm_usb_notifier_call;
+	usb_register_notify(&priv->nb);
+
+	return 0;
+}
+
+static void baseband_xmm_remove(struct platform_device *pdev)
+{
+	struct baseband_xmm_data *priv = platform_get_drvdata(pdev);
+
+	rfkill_unregister(priv->rfkill_dev);
+	rfkill_destroy(priv->rfkill_dev);
+
+	disable_irq(priv->irq);
+	cancel_delayed_work_sync(&priv->modem_work);
+
+	usb_unregister_notify(&priv->nb);
+	baseband_xmm_poweroff(priv);
+}
+
+static const struct of_device_id baseband_xmm_match[] = {
+	{ .compatible = "infineon,xmm6260" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, baseband_xmm_match);
+
+static struct platform_driver baseband_xmm_driver = {
+	.driver = {
+		.name = "baseband-xmm6260",
+		.of_match_table = baseband_xmm_match,
+	},
+	.probe = baseband_xmm_probe,
+	.remove = baseband_xmm_remove,
+};
+module_platform_driver(baseband_xmm_driver);
+
+MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");
+MODULE_DESCRIPTION("Baseband Infineon XMM6260 driver");
+MODULE_LICENSE("GPL");
-- 
2.51.0

Re: [PATCH v2 2/2] net: usb: Add Infineon XMM6260 Baseband modem support

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: usb: Add Infineon XMM6260 Baseband modem support

This patch adds a platform driver for the Infineon/Intel XMM6260
3G modem. The driver toggles GPIOs and a regulator, drives a pwrseq
provider, and uses the global USB notifier plus rfkill to react to
the modem's HSIC USB enumeration.

A few observations on the commit message and overall placement before
the inline comments:

[Low]
The commit message tail reads:

  The modem is usually connected via the application processor's USB
  line in HSIC mode; however, to work properly, the modem must control
  this line

The sentence stops there with no completion.  Since this is the central
justification for adding a custom driver, can the explanation of what
"control this line" entails and why an existing solution does not work
be added?

[Low]
The driver is placed under drivers/net/usb/ but does not implement a
net_device (no register_netdev / alloc_netdev / netdev_ops, no usbnet
binding, no tx/rx callbacks).  The dmesg in the commit message and the
Kconfig help confirm the modem enumerates as cdc_acm and that data
goes over ttyACM0 with a userspace DHCP client.

Would drivers/usb/misc/ or drivers/power/ be a more natural home, both
for routing the patch to the right maintainers and to avoid this
appearing alongside actual USB net drivers?

[Low]
Could the design be re-expressed as a pure pwrseq provider attached to
the USB device's hub-port DT node, letting the USB core gate
enumeration?  The current driver pulls in pwrseq for part of the work
but keeps regulator + GPIO toggling, an rfkill instance, a global USB
notifier, and an ad-hoc bitmap state machine alongside it.

> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index 52a5c0922c79..d54d8db752df 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -642,4 +642,19 @@ config USB_RTL8153_ECM
>  	  CONFIG_USB_RTL8152 is not set, or the RTL8153 device is not
>  	  supported by r8152 driver.
>  
> +config USB_NET_XMM6260
> +	tristate "Infineon XMM626X Baseband HSPA/HSUPA modem"

[Low]
The title says "XMM626X" (suggesting a family), but the USB notifier
hard-codes a single VID:PID 0x1519:0x0020 for XMM6260 only.  Other
626X variants such as XMM6262 will not be matched.  Should the title
be narrowed to XMM6260, or the match table extended?

> +	depends on USB_CHIPIDEA && (RFKILL || RFKILL=n)

[Low]
Does the driver actually have a software dependency on USB_CHIPIDEA?
The XMM6260 driver does not call any chipidea-specific API; it only
reacts to USB enumeration via the generic USB notifier and toggles
GPIOs/regulators/pwrseq.  Tying this to USB_CHIPIDEA prevents using
the driver on other HSIC-/USB-capable SoCs even though nothing in the
code is HCD-specific.

[ ... ]

> diff --git a/drivers/net/usb/baseband-xmm6260.c b/drivers/net/usb/baseband-xmm6260.c
> new file mode 100644
> index 000000000000..6710e46f4170
> --- /dev/null
> +++ b/drivers/net/usb/baseband-xmm6260.c
> @@ -0,0 +1,378 @@

[ ... ]

> +/* bits [0:2] */
> +enum baseband_xmm_ipc_ap_wake_state {
> +	BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY,
> +	BASEBAND_XMM_IPC_AP_WAKE_INIT1,
> +	BASEBAND_XMM_IPC_AP_WAKE_INIT2,
> +	BASEBAND_XMM_IPC_AP_WAKE_L,
> +	BASEBAND_XMM_IPC_AP_WAKE_H,
> +	BASEBAND_XMM_IPC_AP_WAKE_UNINIT,
> +	BASEBAND_XMM_IPC_AP_WAKE_MASK = 7,
> +};
> +
> +#define BASEBAND_XMM_IPC_AP_WAKE_MAX	3
> +
> +#define BASEBAND_XMM_STATE_POWERED	3 /* Tracks regulator state */
> +#define BASEBAND_XMM_STATE_PROTECTED	4 /* Prevents rfkill from access */
> +#define BASEBAND_XMM_STATE_PRESENT	5 /* Tracks USB device presence */
> +#define BASEBAND_XMM_STATE_POWEROFF	6 /* Prevents poweroff recursive call */
> +#define BASEBAND_XMM_STATE_MAX		8

[Low]
The ipc_ap_wake_state enum reaches UNINIT=5 and MASK=7, but
BASEBAND_XMM_IPC_AP_WAKE_MAX=3 caps the encode/decode loops at three
bits.  Bits 3..6 of priv->state are then repurposed as orthogonal
STATE_POWERED / PROTECTED / PRESENT / POWEROFF flags.  Nothing
asserts that the wake-state value fits in three bits, so a future
enum entry past 7, or a typo widening WAKE_MAX, would silently
overwrite the STATE_* flags.  Could a BUILD_BUG_ON or static_assert
guard this invariant?

The L / H / UNINIT / MASK enumerators are also dead today; they only
appear at their declarations.  The block comment above the enum
describes a multi-state protocol with a retry cycle, but the IRQ
handler implements only IRQ_READY and INIT1.  Should the unused
values be removed, or should the handler be completed?

[ ... ]

> +static void set_ipc_ap_wake(struct baseband_xmm_data *priv,
> +			    enum baseband_xmm_ipc_ap_wake_state state)
> +{
> +	for (int i = 0; i < BASEBAND_XMM_IPC_AP_WAKE_MAX; i++)
> +		if (state & BIT(i))
> +			set_bit(i, priv->state);
> +		else
> +			clear_bit(i, priv->state);
> +}
> +
> +static void baseband_xmm_reset(struct baseband_xmm_data *priv)
> +{
> +	int ret;
> +
> +	set_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
> +
> +	if (!test_bit(BASEBAND_XMM_STATE_POWERED, priv->state)) {
> +		ret = regulator_enable(priv->vbat_supply);
> +		if (ret)
> +			dev_err(priv->dev,
> +				"failed to enable vbat power supply\n");
> +
> +		set_bit(BASEBAND_XMM_STATE_POWERED, priv->state);
> +	}

[High]
On regulator_enable() failure, ret is logged but STATE_POWERED is
still set unconditionally.  When baseband_xmm_poweroff() later
observes STATE_POWERED it will call regulator_disable() without a
matching enable, which trips WARN_ON in the regulator core for a
negative use_count and corrupts the disable count for every other
consumer of vbat.  Should the set_bit() be moved inside an "if (!ret)"
branch, or should the function bail out on enable failure?

> +
> +	gpiod_set_value_cansleep(priv->enable_gpio, 0);
> +	msleep(50);
> +
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +	msleep(200);
> +	gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +
> +	msleep(50);
> +
> +	/* falling edge trigger to CP */
> +	gpiod_set_value_cansleep(priv->enable_gpio, 1);
> +	usleep_range(50, 100);
> +	gpiod_set_value_cansleep(priv->enable_gpio, 0);
> +	msleep(20);
> +}
> +
> +static void baseband_xmm_poweroff(struct baseband_xmm_data *priv)
> +{
> +	/*
> +	 * The test_bit check prevents poweroff from being recursively
> +	 * called during USB device deregistration. USB device
> +	 * deregistration can be triggered by the driver by calling this
> +	 * function or by some external event. The first case will cause
> +	 * a recursive call by the notifier if not handled, while the
> +	 * second case requires this call to handle the USB controller
> +	 * properly.
> +	 */
> +	if (test_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state))
> +		return;
> +
> +	set_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
> +	set_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state);
> +
> +	pwrseq_power_off(priv->pwrseq);
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +
> +	if (test_bit(BASEBAND_XMM_STATE_POWERED, priv->state)) {
> +		regulator_disable(priv->vbat_supply);
> +		clear_bit(BASEBAND_XMM_STATE_POWERED, priv->state);
> +	}
> +	set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY);
> +
> +	clear_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
> +	clear_bit(BASEBAND_XMM_STATE_PRESENT, priv->state);
> +	clear_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state);
> +}

[High]
Are STATE_PROTECTED / STATE_POWEROFF / STATE_PRESENT being used here
as ad-hoc locks rather than as state?  The pattern is set on entry,
cleared on exit:

    if (test_bit(STATE_POWEROFF, priv->state))
        return;
    set_bit(STATE_PROTECTED, ...);
    set_bit(STATE_POWEROFF, ...);
    ...
    clear_bit(STATE_PROTECTED, ...);
    clear_bit(STATE_PRESENT, ...);
    clear_bit(STATE_POWEROFF, ...);

test_bit() followed by set_bit() is not atomic, so two callers (the
USB_DEVICE_REMOVE notifier on one CPU, the rfkill set_block callback,
the delayed work, or remove() on another) can both observe POWEROFF=0
and both proceed, calling pwrseq_power_off() twice and (when
STATE_POWERED is set) regulator_disable() twice.

The same concern applies to baseband_xmm_set_block(), which reads
STATE_PROTECTED and STATE_PRESENT outside of any lock, and to the
three separate set_bit/clear_bit operations inside set_ipc_ap_wake()
that the IRQ handler reads back via three separate test_bit() calls
(potential torn read across bits 0..2).

The networking subsystem documentation calls this pattern out
explicitly: "any pattern where a flag, atomic variable, or bit
operation appears to guard a section of code rather than express
state ... is a bug".  Could a real mutex (or spinlock for the IRQ
handler reads) be used instead?

> +
> +static int baseband_xmm_usb_notifier_call(struct notifier_block *nb,
> +					  unsigned long action, void *data)
> +{
> +	struct baseband_xmm_data *priv =
> +		container_of(nb, struct baseband_xmm_data, nb);
> +	struct usb_device *udev;
> +	u16 product, vendor;
> +
> +	if (action == USB_BUS_ADD || action == USB_BUS_REMOVE)
> +		return NOTIFY_OK;
> +
> +	udev = data;
> +	product = le16_to_cpu(udev->descriptor.idProduct);
> +	vendor = le16_to_cpu(udev->descriptor.idVendor);
> +
> +	switch (action) {
> +	case USB_DEVICE_ADD:
> +		/* Infineon XMM6260 ID 1519:0020 */
> +		if (vendor == BASEBAND_VENDOR_ID_COMNEON &&
> +		    product == BASEBAND_PRODUCT_ID_XMM6260) {
> +			cancel_delayed_work_sync(&priv->modem_work);
> +			clear_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
> +			set_bit(BASEBAND_XMM_STATE_PRESENT, priv->state);
> +		}
> +		break;

[Low]
Match is purely on VID:PID via the global USB notifier, with no
binding between this platform_device instance and the USB device that
enumerates.  If two XMM6260 modems are present in the system, both
driver instances react to either modem's add/remove.  Would binding a
USB device driver to that VID:PID, or routing the lifecycle through a
pwrseq attached to the hub-port node, give a stable per-instance
association?

> +
> +	case USB_DEVICE_REMOVE:
> +		/* Infineon XMM6260 ID 1519:0020 */
> +		if (vendor == BASEBAND_VENDOR_ID_COMNEON &&
> +		    product == BASEBAND_PRODUCT_ID_XMM6260)
> +			baseband_xmm_poweroff(priv);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}

[ ... ]

> +static irqreturn_t baseband_hostwake_interrupt(int irq, void *dev_id)
> +{
> +	struct baseband_xmm_data *priv = dev_id;
> +	int state = gpiod_get_value(priv->ipc_ap_gpio);

[Medium]
This handler is registered with devm_request_threaded_irq(..., NULL,
&baseband_hostwake_interrupt, IRQF_ONESHOT | irqflags, ...), so it
runs in threaded (sleepable) context.  Should this read use
gpiod_get_value_cansleep() instead of gpiod_get_value()?  If ap-wake
is sourced from a sleeping GPIO controller (I2C/SPI expander, PMIC),
gpiod_get_value() will trip the WARN_ON(desc->gdev->can_sleep) in
gpiolib on every interrupt.  All other GPIO accesses in the driver
already use the *_cansleep variants.

> +
> +	switch (get_ipc_ap_wake(priv)) {
> +	case BASEBAND_XMM_IPC_AP_WAKE_IRQ_READY:
> +		if (!state) {
> +			set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_INIT1);
> +			pwrseq_power_on(priv->pwrseq);
> +		}
> +		break;
> +
> +	case BASEBAND_XMM_IPC_AP_WAKE_INIT1:
> +		if (state) {
> +			set_ipc_ap_wake(priv, BASEBAND_XMM_IPC_AP_WAKE_INIT2);
> +			schedule_delayed_work(&priv->modem_work,
> +					      msecs_to_jiffies(BASEBAND_XMM_INIT_DELAY));
> +		}
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Medium]
Is the polarity of the IRQ_READY -> INIT1 transition consistent with
the comment above the enum?  The block comment states:

  AP wake interrupt keeps low util CP starts to initiate its HSIC hw.
  AP wake interrupt goes up during CP HSIC init
  (BASEBAND_XMM_IPC_AP_WAKE_INIT1 state)

So entering INIT1 corresponds to a rising edge (line goes high).  But
the handler enters INIT1 only when "!state" (line is low) and that's
where it calls pwrseq_power_on().  Either the comment is describing
the protocol incorrectly or the conditional checks the wrong polarity;
if the comment is accurate, the modem would never power on.  Could
this be clarified?

[Low]
For the non-DT path, irqflags is set to
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and the handler decides
which transition occurred by re-reading the GPIO.  The value seen in
the handler is the current level, not necessarily the level that
triggered the IRQ.  A coalesced fast L->H->L sequence (or a noisy
edge) can cause the handler to read a state that does not correspond
to the triggering edge, missing the INIT1 -> INIT2 transition or
jumping over states.  Should each edge be latched (e.g. by remembering
the previous level) instead of re-sampled?

> +
> +static int baseband_xmm_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	bitmap_zero(priv->state, BASEBAND_XMM_STATE_MAX);
> +	INIT_DELAYED_WORK(&priv->modem_work, baseband_xmm_work);
> +
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq < 0)
> +		return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
> +
> +	/*
> +	 * Systems using device tree should set up interrupt via DT,
> +	 * the rest will use the default edge both interrupt.
> +	 */
> +	irqflags = dev->of_node ? 0 : IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
> +
> +	ret = devm_request_threaded_irq(dev, priv->irq, NULL,
> +					&baseband_hostwake_interrupt,
> +					IRQF_ONESHOT | irqflags,
> +					"modem-hostwake", priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register IRQ %d\n", priv->irq);
> +
> +	priv->rfkill_dev = rfkill_alloc("xmm-modem", dev, RFKILL_TYPE_WWAN,
> +					&baseband_xmm_rfkill_ops, priv);
> +	if (!priv->rfkill_dev)
> +		return -ENOMEM;
> +
> +	ret = rfkill_register(priv->rfkill_dev);
> +	if (ret) {
> +		rfkill_destroy(priv->rfkill_dev);
> +		return dev_err_probe(dev, ret,
> +				     "failed to register WWAN rfkill\n");
> +	}

[High]
Once devm_request_threaded_irq() succeeds, the IRQ is live and the
handler can already step through IRQ_READY -> INIT1 (calling
pwrseq_power_on()) and on the next edge INIT1 -> INIT2, where it does:

    schedule_delayed_work(&priv->modem_work,
                          msecs_to_jiffies(BASEBAND_XMM_INIT_DELAY));

If rfkill_alloc() returns NULL or rfkill_register() fails, probe
returns an error.  devres releases the IRQ but does not cancel the
queued delayed work, and the devm_kzalloc()'d priv is then freed.
Five seconds later baseband_xmm_work() runs and dereferences priv
(priv->pwrseq, priv->state, priv->reset_gpio, priv->vbat_supply).

Should the error paths after devm_request_threaded_irq() call
cancel_delayed_work_sync(&priv->modem_work), or should the cancel be
registered via devm_add_action_or_reset()?

> +
> +	priv->nb.notifier_call = baseband_xmm_usb_notifier_call;
> +	usb_register_notify(&priv->nb);
> +
> +	return 0;
> +}

[High]
The ordering here is rfkill_register() first, usb_register_notify()
second.  rfkill_register() schedules sync_work, which runs the
set_block callback asynchronously.  Since this driver does not set
rfkill->persistent, the default state is unblocked, so set_block(false)
calls baseband_xmm_reset() -> regulator_enable() + GPIO toggling, and
the IRQ handler can step through IRQ_READY -> INIT1 -> INIT2 and
pwrseq_power_on() the modem, which then enumerates on USB.

If usb_register_notify() has not yet run when USB enumeration
completes, the USB_DEVICE_ADD notification is missed: STATE_PRESENT
is never set, the cancel_delayed_work_sync() in the notifier never
runs, and the 5-second watchdog work then calls baseband_xmm_poweroff()
on a working modem.

Should usb_register_notify() be called before rfkill_register()?

> +
> +static void baseband_xmm_remove(struct platform_device *pdev)
> +{
> +	struct baseband_xmm_data *priv = platform_get_drvdata(pdev);
> +
> +	rfkill_unregister(priv->rfkill_dev);
> +	rfkill_destroy(priv->rfkill_dev);
> +
> +	disable_irq(priv->irq);
> +	cancel_delayed_work_sync(&priv->modem_work);
> +
> +	usb_unregister_notify(&priv->nb);
> +	baseband_xmm_poweroff(priv);
> +}

[Medium]
Between cancel_delayed_work_sync() and usb_unregister_notify(), a
USB_DEVICE_REMOVE notification can still be in flight on another CPU
and call baseband_xmm_poweroff().  usb_unregister_notify() then waits
for that callback to complete, and immediately afterwards remove()
itself calls baseband_xmm_poweroff().  Combined with the
ad-hoc-locking concern above, this can result in pwrseq_power_off()
running twice and possibly regulator_disable() running twice.

Would unregistering the notifier first (before disable_irq /
cancel_delayed_work_sync) close that window?

[ ... ]

> +module_platform_driver(baseband_xmm_driver);
> +
> +MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");

[Low]
The name spelling here (Svyatolsav) does not match the file copyright
header or the patch's Signed-off-by ("Svyatoslav"); the 's' and 'l'
are transposed.

> +MODULE_DESCRIPTION("Baseband Infineon XMM6260 driver");
> +MODULE_LICENSE("GPL");
-- 
pw-bot: cr