2022-10-26 13:18:34

by Matthias Schiffer

[permalink] [raw]
Subject: [RFC 0/5] "notify-device" for cross-driver readiness notification

This patch series is obviously missing documentation, MAINTAINERS
entries, etc., but I'd like to solicit some basic feedback on whether
this approach makes sense at all before I proceed. If it does, the
naming is also very much open for bikeshedding - I'm not too happy with
"notify-device".

The basic problem that the notify-device tries to solve is the
synchronization of firmware loading readiness between the Marvell/NXP
WLAN and Bluetooth drivers, but it may also be applicable to other
drivers.

The WLAN and Bluetooth adapters are handled by separate drivers, and may
be connected to the CPU using different interfaces (for example SDIO for
WLAN and UART for Bluetooth). However, both adapters share a single
firmware that may be uploaded via either interface.

For the SDIO+UART case, uploading the firmware via SDIO is usually
preferable, but even when the interface doesn't matter, it seems like a
good idea to clearly define which driver should handle it. To avoid
making the Bluetooth driver more complicated than necessary in this case,
we'd like to defer the probing of the driver until the firmware is ready.

For this purpose, we are introducing a notify-device, with the following
properties:

- The device is created by a driver as soon as some "readiness
condition" is satisfied
- Creating the device also binds a stub driver, so deferred probes are
triggered
- Looking up the notify device is possible via OF node / phandle reference

This approach avoids a hard dependency between the WLAN and Bluetooth
driver, and works regardless of the driver load order.

The first patch implementes the notify-device driver itself, and the
rest shows how the device could be hooked up to the mwifiex and hci_mrvl
drivers. A device tree making use of the notify-device could look like
the following:

&sdhci1 {
wifi@1 {
compatible = "marvell,sd8987";
reg = <1>;

wifi_firmware: firmware-notifier {};
};
};

&main_uart3 {
bluetooth {
compatible = "marvell,sd8987-bt";
firmware-ready = <&wifi_firmware>;
};
};


Matthias Schiffer (5):
misc: introduce notify-device driver
wireless: mwifiex: signal firmware readiness using notify-device
bluetooth: hci_mrvl: select firmwares to load by match data
bluetooth: hci_mrvl: add support for SD8987
bluetooth: hci_mrvl: allow waiting for firmware load using
notify-device

drivers/bluetooth/hci_mrvl.c | 77 ++++++++++++--
drivers/misc/Kconfig | 4 +
drivers/misc/Makefile | 1 +
drivers/misc/notify-device.c | 109 ++++++++++++++++++++
drivers/net/wireless/marvell/mwifiex/main.c | 14 +++
drivers/net/wireless/marvell/mwifiex/main.h | 1 +
include/linux/notify-device.h | 33 ++++++
7 files changed, 228 insertions(+), 11 deletions(-)
create mode 100644 drivers/misc/notify-device.c
create mode 100644 include/linux/notify-device.h

--
2.25.1



2022-10-26 13:19:44

by Matthias Schiffer

[permalink] [raw]
Subject: [RFC 3/5] bluetooth: hci_mrvl: select firmwares to load by match data

Make the driver more generic by adding a driver info struct. We also add
support for devices without firmware (for example when the firmware is
loaded by the WLAN driver on a combined module).

Signed-off-by: Matthias Schiffer <[email protected]>
---
drivers/bluetooth/hci_mrvl.c | 57 +++++++++++++++++++++++++++++-------
1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
index fbc3f7c3a5c7..5d191687a34a 100644
--- a/drivers/bluetooth/hci_mrvl.c
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -14,6 +14,7 @@
#include <linux/module.h>
#include <linux/tty.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/serdev.h>

#include <net/bluetooth/bluetooth.h>
@@ -33,6 +34,20 @@ enum {
STATE_FW_REQ_PENDING,
};

+struct mrvl_driver_info {
+ const char *firmware_helper;
+ const char *firmware;
+};
+
+static const struct mrvl_driver_info mrvl_driver_info_8897 = {
+ .firmware_helper = "mrvl/helper_uart_3000000.bin",
+ .firmware = "mrvl/uart8897_bt.bin",
+};
+
+/* Fallback for non-OF instances */
+static const struct mrvl_driver_info *const mrvl_driver_info_default =
+ &mrvl_driver_info_8897;
+
struct mrvl_data {
struct sk_buff *rx_skb;
struct sk_buff_head txq;
@@ -44,6 +59,7 @@ struct mrvl_data {

struct mrvl_serdev {
struct hci_uart hu;
+ const struct mrvl_driver_info *info;
};

struct hci_mrvl_pkt {
@@ -353,18 +369,29 @@ static int mrvl_load_firmware(struct hci_dev *hdev, const char *name)

static int mrvl_setup(struct hci_uart *hu)
{
+ const struct mrvl_driver_info *info;
int err;

- hci_uart_set_flow_control(hu, true);
+ if (hu->serdev) {
+ struct mrvl_serdev *mrvldev = serdev_device_get_drvdata(hu->serdev);

- err = mrvl_load_firmware(hu->hdev, "mrvl/helper_uart_3000000.bin");
- if (err) {
- bt_dev_err(hu->hdev, "Unable to download firmware helper");
- return -EINVAL;
+ info = mrvldev->info;
+ } else {
+ info = mrvl_driver_info_default;
}

- /* Let the final ack go out before switching the baudrate */
- hci_uart_wait_until_sent(hu);
+ if (info->firmware_helper) {
+ hci_uart_set_flow_control(hu, true);
+
+ err = mrvl_load_firmware(hu->hdev, info->firmware_helper);
+ if (err) {
+ bt_dev_err(hu->hdev, "Unable to download firmware helper");
+ return -EINVAL;
+ }
+
+ /* Let the final ack go out before switching the baudrate */
+ hci_uart_wait_until_sent(hu);
+ }

if (hu->serdev)
serdev_device_set_baudrate(hu->serdev, 3000000);
@@ -373,9 +400,11 @@ static int mrvl_setup(struct hci_uart *hu)

hci_uart_set_flow_control(hu, false);

- err = mrvl_load_firmware(hu->hdev, "mrvl/uart8897_bt.bin");
- if (err)
- return err;
+ if (info->firmware) {
+ err = mrvl_load_firmware(hu->hdev, info->firmware);
+ if (err)
+ return err;
+ }

return 0;
}
@@ -401,6 +430,12 @@ static int mrvl_serdev_probe(struct serdev_device *serdev)
if (!mrvldev)
return -ENOMEM;

+ if (IS_ENABLED(CONFIG_OF)) {
+ mrvldev->info = of_device_get_match_data(&serdev->dev);
+ if (!mrvldev->info)
+ return -ENODEV;
+ }
+
mrvldev->hu.serdev = serdev;
serdev_device_set_drvdata(serdev, mrvldev);

@@ -416,7 +451,7 @@ static void mrvl_serdev_remove(struct serdev_device *serdev)

#ifdef CONFIG_OF
static const struct of_device_id mrvl_bluetooth_of_match[] = {
- { .compatible = "mrvl,88w8897" },
+ { .compatible = "mrvl,88w8897", .data = &mrvl_driver_info_8897 },
{ },
};
MODULE_DEVICE_TABLE(of, mrvl_bluetooth_of_match);
--
2.25.1


2022-10-26 13:19:58

by Matthias Schiffer

[permalink] [raw]
Subject: [RFC 1/5] misc: introduce notify-device driver

A notify-device is a synchronization facility that allows to query
"readiness" across drivers, without creating a direct dependency between
the driver modules. The notify-device can also be used to trigger deferred
probes.

Signed-off-by: Matthias Schiffer <[email protected]>
---
drivers/misc/Kconfig | 4 ++
drivers/misc/Makefile | 1 +
drivers/misc/notify-device.c | 109 ++++++++++++++++++++++++++++++++++
include/linux/notify-device.h | 33 ++++++++++
4 files changed, 147 insertions(+)
create mode 100644 drivers/misc/notify-device.c
create mode 100644 include/linux/notify-device.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 358ad56f6524..63559e9f854c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -496,6 +496,10 @@ config VCPU_STALL_DETECTOR

If you do not intend to run this kernel as a guest, say N.

+config NOTIFY_DEVICE
+ tristate "Notify device"
+ depends on OF
+
source "drivers/misc/c2port/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index ac9b3e757ba1..1e8012112b43 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -62,3 +62,4 @@ obj-$(CONFIG_HI6421V600_IRQ) += hi6421v600-irq.o
obj-$(CONFIG_OPEN_DICE) += open-dice.o
obj-$(CONFIG_GP_PCI1XXXX) += mchp_pci1xxxx/
obj-$(CONFIG_VCPU_STALL_DETECTOR) += vcpu_stall_detector.o
+obj-$(CONFIG_NOTIFY_DEVICE) += notify-device.o
diff --git a/drivers/misc/notify-device.c b/drivers/misc/notify-device.c
new file mode 100644
index 000000000000..42e0980394ea
--- /dev/null
+++ b/drivers/misc/notify-device.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/device/class.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/notify-device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+static void notify_device_release(struct device *dev)
+{
+ of_node_put(dev->of_node);
+ kfree(dev);
+}
+
+static struct class notify_device_class = {
+ .name = "notify-device",
+ .owner = THIS_MODULE,
+ .dev_release = notify_device_release,
+};
+
+static struct platform_driver notify_device_driver = {
+ .driver = {
+ .name = "notify-device",
+ },
+};
+
+struct device *notify_device_create(struct device *parent, const char *child)
+{
+ struct device_node *node;
+ struct device *dev;
+ int err;
+
+ if (!parent->of_node)
+ return ERR_PTR(-EINVAL);
+
+ node = of_get_child_by_name(parent->of_node, child);
+ if (!node)
+ return NULL;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ of_node_put(node);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ dev_set_name(dev, "%s:%s", dev_name(parent), child);
+ dev->class = &notify_device_class;
+ dev->parent = parent;
+ dev->of_node = node;
+ err = device_register(dev);
+ if (err) {
+ put_device(dev);
+ return ERR_PTR(err);
+ }
+
+ dev->driver = &notify_device_driver.driver;
+ err = device_bind_driver(dev);
+ if (err) {
+ device_unregister(dev);
+ return ERR_PTR(err);
+ }
+
+ return dev;
+}
+EXPORT_SYMBOL_GPL(notify_device_create);
+
+void notify_device_destroy(struct device *dev)
+{
+ if (!dev)
+ return;
+
+ device_release_driver(dev);
+ device_unregister(dev);
+}
+EXPORT_SYMBOL_GPL(notify_device_destroy);
+
+struct device *notify_device_find_by_of_node(struct device_node *node)
+{
+ return class_find_device_by_of_node(&notify_device_class, node);
+}
+EXPORT_SYMBOL_GPL(notify_device_find_by_of_node);
+
+static int __init notify_device_init(void)
+{
+ int err;
+
+ err = class_register(&notify_device_class);
+ if (err)
+ return err;
+
+ err = platform_driver_register(&notify_device_driver);
+ if (err) {
+ class_unregister(&notify_device_class);
+ return err;
+ }
+
+ return 0;
+}
+
+static void __exit notify_device_exit(void)
+{
+ platform_driver_unregister(&notify_device_driver);
+ class_unregister(&notify_device_class);
+}
+
+module_init(notify_device_init);
+module_exit(notify_device_exit);
+MODULE_LICENSE("GPL");
diff --git a/include/linux/notify-device.h b/include/linux/notify-device.h
new file mode 100644
index 000000000000..f8c3e15d3b8f
--- /dev/null
+++ b/include/linux/notify-device.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_NOTIFY_DEVICE_H
+#define _LINUX_NOTIFY_DEVICE_H
+#include <linux/device.h>
+#include <linux/of.h>
+
+#ifdef CONFIG_NOTIFY_DEVICE
+
+struct device *notify_device_create(struct device *parent, const char *child);
+void notify_device_destroy(struct device *dev);
+struct device *notify_device_find_by_of_node(struct device_node *node);
+
+#else
+
+static inline struct device *notify_device_create(struct device *parent,
+ const char *child)
+{
+ return NULL;
+}
+
+static inline void notify_device_destroy(struct device *dev)
+{
+}
+
+static inline struct device *notify_device_find_by_of_node(struct device_node *node)
+{
+ return NULL;
+}
+
+#endif
+
+#endif /* _LINUX_NOTIFY_DEVICE_H */
--
2.25.1


2022-10-26 13:41:52

by bluez.test.bot

[permalink] [raw]
Subject: RE: "notify-device" for cross-driver readiness notification

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----
error: patch failed: drivers/misc/Makefile:62
error: drivers/misc/Makefile: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch


Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth