[RFC PATCH 0/2] Add serdev support for hci h4

[RFC PATCH 0/2] Add serdev support for hci h4

[Dominique Martinet]

Hello,


A couple of questions with this patch (hence status as RFC), first for
the dt bindings part:
 - I have no idea what to do with the compatible name.
I am not affiliated with nxp (except as a customer), so I'm not entierly
comfortable just adding a new property in the nxp, namespace.
The h4 protocol is very generic and I'd think a name such as
'hci-h4,generic' make more sense as other boards would be able to
benefit from it without extra modifications... But that doesn't seem to
be how things are done with dt bindings, so can I just add an arbitrary
name?
 - I've set Marcel (who maintains the hci_h4 driver) as maintainer of
he dt-bindings unilaterally without asking him for lack of a better
idea: Marcel, are you ok with that? My first idea was making it myself
but I don't really feel competent for this.

Second for the driver itself:
 - I've just monkeyed the simplest serdev support I could come up with
and it appears to work (I'm trying to replace the following command:
btattach -B /dev/ttymxc0 -S 3000000 -P h4); perhaps there are other
settings you'd want?
I've also tried suspend and with no handler it appears to work with
an idle controller, but I'd assume we might want some pm handling at
some point if possible... Right now this is no worse than btattach,
but unlike btattach it's not easy to restart (unbind/bind the driver?)
so that might come up sooner or later; will be happy to look then.


I confirmed this works with the following dts fragment over
imx8mp.dtsi, on a board with the AW-XM458 NXP wireless+BT module.

--8<------
&uart1 {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_uart1>;
        assigned-clocks = <&clk IMX8MP_CLK_UART1>;
        assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_80M>;
        status = "okay";
        fsl,dte-mode = <1>;
        fsl,uart-has-rtscts;

        bluetooth {
                compatible = "nxp,aw-xm458-bt";
                max-speed = <3000000>;
        };
};

&iomuxc {
        pinctrl_uart1: uart1grp {
                fsl,pins = <
                        MX8MP_IOMUXC_UART1_RXD__UART1_DTE_TX    0x140
                        MX8MP_IOMUXC_UART1_TXD__UART1_DTE_RX    0x140
                        MX8MP_IOMUXC_UART3_RXD__UART1_DTE_RTS   0x140
                        MX8MP_IOMUXC_UART3_TXD__UART1_DTE_CTS   0x140
                >;
        };
}
--8<------


Dominique Martinet (2):
  dt-bindings: net: h4-bluetooth: add new bindings for hci_h4
  bluetooth/hci_h4: add serdev support

 .../devicetree/bindings/net/h4-bluetooth.yaml | 49 ++++++++++++++
 drivers/bluetooth/Kconfig                     |  1 +
 drivers/bluetooth/hci_h4.c                    | 64 +++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/h4-bluetooth.yaml

-- 
2.35.1

[RFC PATCH 1/2] dt-bindings: net: h4-bluetooth: add new bindings for hci_h4

[Dominique Martinet]

Add devicetree binding to support defining a bluetooth device using the h4
uart protocol

This was tested with a NXP wireless+BT AW-XM458 module, but might
benefit others as the H4 protocol seems often used.

Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---
 .../devicetree/bindings/net/h4-bluetooth.yaml | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/h4-bluetooth.yaml

diff --git a/Documentation/devicetree/bindings/net/h4-bluetooth.yaml b/Documentation/devicetree/bindings/net/h4-bluetooth.yaml
new file mode 100644
index 000000000000..5d11b89ca386
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/h4-bluetooth.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/h4-bluetooth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: H4 Bluetooth
+
+maintainers:
+  - Dominique Martinet <dominique.martinet@atmark-techno.com>
+
+description:
+  H4 is a common bluetooth over uart protocol.
+  For example, the AW-XM458 is a WiFi + BT module where the WiFi part is
+  connected over PCI (M.2), while BT is connected over serial speaking
+  the H4 protocol. Its firmware is sent on the PCI side.
+
+properties:
+  compatible:
+    enum:
+      - nxp,aw-xm458-bt
+
+  max-speed: true
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/clock/imx8mp-clock.h>
+
+    uart1 {
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_uart1>;
+        assigned-clocks = <&clk IMX8MP_CLK_UART1>;
+        assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_80M>;
+        status = "okay";
+        fsl,dte-mode = <1>;
+        fsl,uart-has-rtscts;
+
+
+        bluetooth {
+            compatible = "nxp,aw-xm458-bt";
+            max-speed = <3000000>;
+        };
+    };
-- 
2.35.1

[RFC PATCH 2/2] bluetooth/hci_h4: add serdev support

[Dominique Martinet]

adding serdev support to hci_h4 allows users to define h4 bluetooth
controllers in dts files

This allows users of bluetooth modules with an uart h4 interface to use
dt bindings instead of manually running btattach

Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---
 drivers/bluetooth/Kconfig  |  1 +
 drivers/bluetooth/hci_h4.c | 65 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index e30707405455..69edc76a8611 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -113,6 +113,7 @@ config BT_HCIUART_SERDEV
 config BT_HCIUART_H4
 	bool "UART (H4) protocol support"
 	depends on BT_HCIUART
+	depends on BT_HCIUART_SERDEV
 	help
 	  UART (H4) is serial protocol for communication between Bluetooth
 	  device and host. This protocol is required for most Bluetooth devices
diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
index 1d0cdf023243..b214c8a4d450 100644
--- a/drivers/bluetooth/hci_h4.c
+++ b/drivers/bluetooth/hci_h4.c
@@ -18,6 +18,8 @@
 #include <linux/ptrace.h>
 #include <linux/poll.h>
 
+#include <linux/of.h>
+#include <linux/serdev.h>
 #include <linux/slab.h>
 #include <linux/tty.h>
 #include <linux/errno.h>
@@ -32,6 +34,10 @@
 
 #include "hci_uart.h"
 
+struct h4_device {
+	struct hci_uart hu;
+};
+
 struct h4_struct {
 	struct sk_buff *rx_skb;
 	struct sk_buff_head txq;
@@ -130,6 +136,63 @@ static struct sk_buff *h4_dequeue(struct hci_uart *hu)
 	return skb_dequeue(&h4->txq);
 }
 
+static const struct hci_uart_proto h4p;
+
+static int h4_probe(struct serdev_device *serdev)
+{
+	struct h4_device *h4dev;
+	struct hci_uart *hu;
+	int ret;
+	u32 speed;
+
+	h4dev = devm_kzalloc(&serdev->dev, sizeof(*h4dev), GFP_KERNEL);
+	if (!h4dev)
+		return -ENOMEM;
+
+	hu = &h4dev->hu;
+
+	hu->serdev = serdev;
+	ret = device_property_read_u32(&serdev->dev, "max-speed", &speed);
+	if (!ret) {
+		/* h4 does not have any baudrate handling:
+		 * user oper speed from the start
+		 */
+		hu->init_speed = speed;
+		hu->oper_speed = speed;
+	}
+
+	serdev_device_set_drvdata(serdev, h4dev);
+	dev_info(&serdev->dev, "h4 device registered.\n");
+
+	return hci_uart_register_device(hu, &h4p);
+}
+
+static void h4_remove(struct serdev_device *serdev)
+{
+	struct h4_device *h4dev = serdev_device_get_drvdata(serdev);
+
+	dev_info(&serdev->dev, "h4 device unregistered.\n");
+
+	hci_uart_unregister_device(&h4dev->hu);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id h4_bluetooth_of_match[] = {
+	{ .compatible = "nxp,aw-xm458-bt" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, h4_bluetooth_of_match);
+#endif
+
+static struct serdev_device_driver h4_serdev_driver = {
+	.probe = h4_probe,
+	.remove = h4_remove,
+	.driver = {
+		.name = "hci_uart_h4",
+		.of_match_table = of_match_ptr(h4_bluetooth_of_match),
+	},
+};
+
 static const struct hci_uart_proto h4p = {
 	.id		= HCI_UART_H4,
 	.name		= "H4",
@@ -143,11 +206,13 @@ static const struct hci_uart_proto h4p = {
 
 int __init h4_init(void)
 {
+	serdev_device_driver_register(&h4_serdev_driver);
 	return hci_uart_register_proto(&h4p);
 }
 
 int __exit h4_deinit(void)
 {
+	serdev_device_driver_unregister(&h4_serdev_driver);
 	return hci_uart_unregister_proto(&h4p);
 }
 
-- 
2.35.1

Re: [RFC PATCH 1/2] dt-bindings: net: h4-bluetooth: add new bindings for hci_h4

[Krzysztof Kozlowski]

On 08/11/2022 06:55, Dominique Martinet wrote:
> Add devicetree binding to support defining a bluetooth device using the h4
> uart protocol
> 

subject: drop second redundant "bindings"

> This was tested with a NXP wireless+BT AW-XM458 module, but might
> benefit others as the H4 protocol seems often used.
> 
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> ---
>  .../devicetree/bindings/net/h4-bluetooth.yaml | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/h4-bluetooth.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/h4-bluetooth.yaml b/Documentation/devicetree/bindings/net/h4-bluetooth.yaml
> new file mode 100644
> index 000000000000..5d11b89ca386
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/h4-bluetooth.yaml

If the schema is for one specific device, then filename matching the
compatible, so nxp,aw-xm458-bt.yaml... but I understand you want to
describe here class of devices using H4 Bluetooth? Won't they need their
own specific properties?


> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/h4-bluetooth.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: H4 Bluetooth
> +
> +maintainers:
> +  - Dominique Martinet <dominique.martinet@atmark-techno.com>
> +
> +description:
> +  H4 is a common bluetooth over uart protocol.

Bluetooth
UART

> +  For example, the AW-XM458 is a WiFi + BT module where the WiFi part is
> +  connected over PCI (M.2), while BT is connected over serial speaking
> +  the H4 protocol. Its firmware is sent on the PCI side.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,aw-xm458-bt
> +
> +  max-speed: true
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/clock/imx8mp-clock.h>
> +
> +    uart1 {

uart

> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_uart1>;
> +        assigned-clocks = <&clk IMX8MP_CLK_UART1>;
> +        assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_80M>;

Drop unrelated properties.

> +        status = "okay";

Drop status.

> +        fsl,dte-mode = <1>;
> +        fsl,uart-has-rtscts;

Are these two related to this hardware?

> +
> +
> +        bluetooth {
> +            compatible = "nxp,aw-xm458-bt";
> +            max-speed = <3000000>;
> +        };
> +    };

Best regards,
Krzysztof

Re: [RFC PATCH 1/2] dt-bindings: net: h4-bluetooth: add new bindings for hci_h4

[Rob Herring]

On Tue, 08 Nov 2022 14:55:30 +0900, Dominique Martinet wrote:
> Add devicetree binding to support defining a bluetooth device using the h4
> uart protocol
> 
> This was tested with a NXP wireless+BT AW-XM458 module, but might
> benefit others as the H4 protocol seems often used.
> 
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> ---
>  .../devicetree/bindings/net/h4-bluetooth.yaml | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/h4-bluetooth.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
fsl,dte-mode: boolean property with value b'\x00\x00\x00\x01'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/h4-bluetooth.example.dtb: uart1: fsl,dte-mode: b'\x00\x00\x00\x01' is not of type 'object', 'array', 'boolean', 'null'
	From schema: /usr/local/lib/python3.10/dist-packages/dtschema/schemas/dt-core.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/h4-bluetooth.example.dtb: uart1: 'anyOf' conditional failed, one must be fixed:
	'clocks' is a required property
	'#clock-cells' is a required property
	From schema: /usr/local/lib/python3.10/dist-packages/dtschema/schemas/clock/clock.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [RFC PATCH 1/2] dt-bindings: net: h4-bluetooth: add new bindings for hci_h4

[Rob Herring]

On Mon, Nov 7, 2022 at 11:56 PM Dominique Martinet
<dominique.martinet@atmark-techno.com> wrote:
>
> Add devicetree binding to support defining a bluetooth device using the h4
> uart protocol

The protocol is mostly irrelevant to the binding. The binding is for a
particular device even if the driver is shared.

>
> This was tested with a NXP wireless+BT AW-XM458 module, but might
> benefit others as the H4 protocol seems often used.
>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> ---
>  .../devicetree/bindings/net/h4-bluetooth.yaml | 49 +++++++++++++++++++

Use the compatible string for the filename.

There's now a pending (in linux-next) net/bluetooth/ directory and a
bluetooth-controller.yaml schema which you should reference.

>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/h4-bluetooth.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/h4-bluetooth.yaml b/Documentation/devicetree/bindings/net/h4-bluetooth.yaml
> new file mode 100644
> index 000000000000..5d11b89ca386
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/h4-bluetooth.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/h4-bluetooth.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: H4 Bluetooth
> +
> +maintainers:
> +  - Dominique Martinet <dominique.martinet@atmark-techno.com>
> +
> +description:
> +  H4 is a common bluetooth over uart protocol.
> +  For example, the AW-XM458 is a WiFi + BT module where the WiFi part is
> +  connected over PCI (M.2), while BT is connected over serial speaking
> +  the H4 protocol. Its firmware is sent on the PCI side.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,aw-xm458-bt
> +
> +  max-speed: true
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/clock/imx8mp-clock.h>
> +
> +    uart1 {

serial {

> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_uart1>;
> +        assigned-clocks = <&clk IMX8MP_CLK_UART1>;
> +        assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_80M>;
> +        status = "okay";
> +        fsl,dte-mode = <1>;
> +        fsl,uart-has-rtscts;

All these properties are irrelevant to the example. Drop.

> +
> +
> +        bluetooth {
> +            compatible = "nxp,aw-xm458-bt";
> +            max-speed = <3000000>;
> +        };
> +    };
> --
> 2.35.1
>
>

Re: [RFC PATCH 2/2] bluetooth/hci_h4: add serdev support

[Andrew Lunn]

> +static int h4_probe(struct serdev_device *serdev)
> +{
> +	struct h4_device *h4dev;
> +	struct hci_uart *hu;
> +	int ret;
> +	u32 speed;
> +
> +	h4dev = devm_kzalloc(&serdev->dev, sizeof(*h4dev), GFP_KERNEL);
> +	if (!h4dev)
> +		return -ENOMEM;
> +
> +	hu = &h4dev->hu;
> +
> +	hu->serdev = serdev;
> +	ret = device_property_read_u32(&serdev->dev, "max-speed", &speed);
> +	if (!ret) {
> +		/* h4 does not have any baudrate handling:
> +		 * user oper speed from the start
> +		 */
> +		hu->init_speed = speed;
> +		hu->oper_speed = speed;
> +	}
> +
> +	serdev_device_set_drvdata(serdev, h4dev);
> +	dev_info(&serdev->dev, "h4 device registered.\n");

It is considered bad practice to spam the logs like this. dev_dbg().

> +
> +	return hci_uart_register_device(hu, &h4p);
> +}
> +
> +static void h4_remove(struct serdev_device *serdev)
> +{
> +	struct h4_device *h4dev = serdev_device_get_drvdata(serdev);
> +
> +	dev_info(&serdev->dev, "h4 device unregistered.\n");

dev_dbg().

	Andrew

Re: [RFC PATCH 2/2] bluetooth/hci_h4: add serdev support

[Krzysztof Kozlowski]

On 08/11/2022 21:38, Andrew Lunn wrote:
>> +static int h4_probe(struct serdev_device *serdev)
>> +{
>> +	struct h4_device *h4dev;
>> +	struct hci_uart *hu;
>> +	int ret;
>> +	u32 speed;
>> +
>> +	h4dev = devm_kzalloc(&serdev->dev, sizeof(*h4dev), GFP_KERNEL);
>> +	if (!h4dev)
>> +		return -ENOMEM;
>> +
>> +	hu = &h4dev->hu;
>> +
>> +	hu->serdev = serdev;
>> +	ret = device_property_read_u32(&serdev->dev, "max-speed", &speed);
>> +	if (!ret) {
>> +		/* h4 does not have any baudrate handling:
>> +		 * user oper speed from the start
>> +		 */
>> +		hu->init_speed = speed;
>> +		hu->oper_speed = speed;
>> +	}
>> +
>> +	serdev_device_set_drvdata(serdev, h4dev);
>> +	dev_info(&serdev->dev, "h4 device registered.\n");
> 
> It is considered bad practice to spam the logs like this. dev_dbg().
> 
>> +
>> +	return hci_uart_register_device(hu, &h4p);
>> +}
>> +
>> +static void h4_remove(struct serdev_device *serdev)
>> +{
>> +	struct h4_device *h4dev = serdev_device_get_drvdata(serdev);
>> +
>> +	dev_info(&serdev->dev, "h4 device unregistered.\n");
> 
> dev_dbg().

I would say none of them (the same in probe). Any prints in probe/remove
paths are considered redundant, as core already gives that information.

Best regards,
Krzysztof

Re: [RFC PATCH 1/2] dt-bindings: net: h4-bluetooth: add new bindings for hci_h4

[Dominique Martinet]

Thanks for all the replies!

All remarks make sense, I'll do my homework and send a v2 once extra
questions have been answered.

Rob Herring wrote on Tue, Nov 08, 2022 at 07:59:33AM -0600:
> On Mon, Nov 7, 2022 at 11:56 PM Dominique Martinet
> <dominique.martinet@atmark-techno.com> wrote:
> > Add devicetree binding to support defining a bluetooth device using the h4
> > uart protocol
> 
> The protocol is mostly irrelevant to the binding. The binding is for a
> particular device even if the driver is shared.

This echoes the point below: I wanted to make this a bit more generic
for other adapters, question at the end of my first reply to Krzysztof
below.

> There's now a pending (in linux-next) net/bluetooth/ directory and a
> bluetooth-controller.yaml schema which you should reference.

Will check it out and add that.

Krzysztof Kozlowski wrote on Tue, Nov 08, 2022 at 12:37:39PM +0100:
> > diff --git a/Documentation/devicetree/bindings/net/h4-bluetooth.yaml b/Documentation/devicetree/bindings/net/h4-bluetooth.yaml
> > new file mode 100644
> > index 000000000000..5d11b89ca386
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/h4-bluetooth.yaml
> 
> If the schema is for one specific device, then filename matching the
> compatible, so nxp,aw-xm458-bt.yaml... but I understand you want to
> describe here class of devices using H4 Bluetooth? Won't they need their
> own specific properties?

H4 bluetooth itself has very little configurable elements, from what I
can see about the device I'm using the actual configuration is done by
the wifi driver that uploads a "combo" firmware over the PCI side
(it's based on mwifiex, so for example mrvl/pcieuart8997_combo_v4.bin
upstream works the same way afaik)

This is a pretty terrible design, as the Bluetooth side cannot actually
know when the device is ready as the initialization takes place, but
that means there really aren't any property to give here

(I haven't reproduced during normal boot, but in particular if I run
bluetoothd before loading the wifi driver, I need to unbind/bind the
serial device from the hci_uart_h4 driver to recover bluetooth...
With that in mind it might actually be best to try to coordinate this
from userspace with btattach after all, and I'd be happy with that if I
didn't have to fight our init system so much, but as things stand having
it autoloaded by the kernel is more convenient for us... Which is
admitedly a weak reason for you all, feel free to tell me this isn't
viable)


Anyway, there probably would be other devices benefiting from this, at
the very least other cards in the mwifiex family, but I'm doing this as
a end user so I'm not comfortable adding devices I cannot test.

So with all of this (sorry for the wall of text), should I try to keep
this generic, or just give up and make it specific to nxp,aw-xm458-bt
and let whoever adds the next device rename the file?


> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/clock/imx8mp-clock.h>
> > +
> > +    uart {
> > +        fsl,dte-mode = <1>;
> > +        fsl,uart-has-rtscts;
> 
> Are these two related to this hardware?

I'd say it's related to my soc rather than the Bluetooth adapter; I
tried to give a full example but it's unrelated and I'll drop this as
well.

-- 
Dominique Martinet

Re: [RFC PATCH 1/2] dt-bindings: net: h4-bluetooth: add new bindings for hci_h4

[Dominique Martinet]

Dominique Martinet wrote on Wed, Nov 09, 2022 at 08:54:42AM +0900:
> This is a pretty terrible design, as the Bluetooth side cannot actually
> know when the device is ready as the initialization takes place, but
> that means there really aren't any property to give here
> 
> (I haven't reproduced during normal boot, but in particular if I run
> bluetoothd before loading the wifi driver, I need to unbind/bind the
> serial device from the hci_uart_h4 driver to recover bluetooth...
> With that in mind it might actually be best to try to coordinate this
> from userspace with btattach after all, and I'd be happy with that if I
> didn't have to fight our init system so much, but as things stand having
> it autoloaded by the kernel is more convenient for us... Which is
> admitedly a weak reason for you all, feel free to tell me this isn't
> viable)

This actually hasn't taken long to bite us: while the driver does work,
we get error messages early on before the firmware is loaded.
(In hindsight, I probably should have waited a few days before sending
this...)


My current workaround is to return EPROBE_DEFER until we can find a
netdev with a known name in the init namespace, but that isn't really
something I'd consider upstreamable for obvious reasons (interfaces can
be renamed or moved to different namespaces so this is inherently racy
and it's just out of place in BT code)

That makes these two patches on their own rather useless as well, so
unless one of you have an idea that's less ugly I'd guess just dropping
this is the way to go, as much as I dislike the idea of adding more
non-upstream code than we already have :(

Thank you both for your time, the replies have been very helpful and
I'll remember for next time I try to submit something!

And if you have a suggestion, I'll be happy to do some legwork to clean
this mess, so feel free to ask :)


Thanks,
-- 
Dominique

Re: [RFC PATCH 1/2] dt-bindings: net: h4-bluetooth: add new bindings for hci_h4

[Rob Herring]

On Wed, Nov 09, 2022 at 04:29:52PM +0900, Dominique Martinet wrote:
> Dominique Martinet wrote on Wed, Nov 09, 2022 at 08:54:42AM +0900:
> > This is a pretty terrible design, as the Bluetooth side cannot actually
> > know when the device is ready as the initialization takes place, but
> > that means there really aren't any property to give here
> > 
> > (I haven't reproduced during normal boot, but in particular if I run
> > bluetoothd before loading the wifi driver, I need to unbind/bind the
> > serial device from the hci_uart_h4 driver to recover bluetooth...
> > With that in mind it might actually be best to try to coordinate this
> > from userspace with btattach after all, and I'd be happy with that if I
> > didn't have to fight our init system so much, but as things stand having
> > it autoloaded by the kernel is more convenient for us... Which is
> > admitedly a weak reason for you all, feel free to tell me this isn't
> > viable)

Punting the issue to userspace is not a great solution...


> This actually hasn't taken long to bite us: while the driver does work,
> we get error messages early on before the firmware is loaded.
> (In hindsight, I probably should have waited a few days before sending
> this...)
> 
> 
> My current workaround is to return EPROBE_DEFER until we can find a
> netdev with a known name in the init namespace, but that isn't really
> something I'd consider upstreamable for obvious reasons (interfaces can
> be renamed or moved to different namespaces so this is inherently racy
> and it's just out of place in BT code)

Can't you just try to access the BT h/w in some way and defer when that 
fails?

Or perhaps use fw_devlink to create a dependency on the wifi node. I'm 
not sure offhand how exactly you do that with a custom property.

Rob

Re: [RFC PATCH 1/2] dt-bindings: net: h4-bluetooth: add new bindings for hci_h4

[Dominique Martinet]

Rob Herring wrote on Wed, Nov 09, 2022 at 04:00:05PM -0600:
> Punting the issue to userspace is not a great solution...

I can definitely agree with that :)

Userspace has the advantage of being easy to shove ugly things under the
rug, whereas I still have faint hope of keeping down the divergences we
have with upstream kernel... But that's about it.

If we can work out a solution here I'll be very happy.


Rob Herring wrote on Wed, Nov 09, 2022 at 04:00:05PM -0600:
> > This actually hasn't taken long to bite us: while the driver does work,
> > we get error messages early on before the firmware is loaded.
> > (In hindsight, I probably should have waited a few days before sending
> > this...)
> > 
> > 
> > My current workaround is to return EPROBE_DEFER until we can find a
> > netdev with a known name in the init namespace, but that isn't really
> > something I'd consider upstreamable for obvious reasons (interfaces can
> > be renamed or moved to different namespaces so this is inherently racy
> > and it's just out of place in BT code)
> 
> Can't you just try to access the BT h/w in some way and defer when that 
> fails?

This is just a serial link; I've tried poking at it a bit before the
firmware is loaded but mostly never got any reply, or while the driver
sometimes got garbage back at some point (baudrate not matching with
fresh boot default?)
Either way, no reply isn't great -- just waiting a few ms for reply or
not is not my idea of good design...

> Or perhaps use fw_devlink to create a dependency on the wifi node. I'm 
> not sure offhand how exactly you do that with a custom property.

That sounds great if we can figure how to do that!
From what I can see this doesn't look possible to express in pure
devicetree, but I see some code initializing a fwnode manually in a
constructor function with fwnode_init and a fwnode_operations vector
that has .add_links, which in turn could add a link.
... My problem at this point would be that I currently load the wireless
driver as a module as it's vendor provided out of tree... (it's loaded
through its pci alias, I guess it's udev checking depmod infos? not
familiar how that part of autoloading really works...)

But that makes me think that rather than defining the bluetooth serdev
in dts early, I could try to have the wireless driver create it once
it's ready? hmm...

Let me sleep on that a bit and have another look at both fwnode
(fw_devlink) and dynamic device creation.


Cheers,
-- 
Dominique

Re: [RFC PATCH 1/2] dt-bindings: net: h4-bluetooth: add new bindings for hci_h4

[Rob Herring]

On Thu, Nov 10, 2022 at 1:38 AM Dominique Martinet
<dominique.martinet@atmark-techno.com> wrote:
>
> Rob Herring wrote on Wed, Nov 09, 2022 at 04:00:05PM -0600:
> > Punting the issue to userspace is not a great solution...
>
> I can definitely agree with that :)
>
> Userspace has the advantage of being easy to shove ugly things under the
> rug, whereas I still have faint hope of keeping down the divergences we
> have with upstream kernel... But that's about it.
>
> If we can work out a solution here I'll be very happy.
>
>
> Rob Herring wrote on Wed, Nov 09, 2022 at 04:00:05PM -0600:
> > > This actually hasn't taken long to bite us: while the driver does work,
> > > we get error messages early on before the firmware is loaded.
> > > (In hindsight, I probably should have waited a few days before sending
> > > this...)
> > >
> > >
> > > My current workaround is to return EPROBE_DEFER until we can find a
> > > netdev with a known name in the init namespace, but that isn't really
> > > something I'd consider upstreamable for obvious reasons (interfaces can
> > > be renamed or moved to different namespaces so this is inherently racy
> > > and it's just out of place in BT code)
> >
> > Can't you just try to access the BT h/w in some way and defer when that
> > fails?
>
> This is just a serial link; I've tried poking at it a bit before the
> firmware is loaded but mostly never got any reply, or while the driver
> sometimes got garbage back at some point (baudrate not matching with
> fresh boot default?)
> Either way, no reply isn't great -- just waiting a few ms for reply or
> not is not my idea of good design...
>
> > Or perhaps use fw_devlink to create a dependency on the wifi node. I'm
> > not sure offhand how exactly you do that with a custom property.
>
> That sounds great if we can figure how to do that!
> From what I can see this doesn't look possible to express in pure
> devicetree, but I see some code initializing a fwnode manually in a
> constructor function with fwnode_init and a fwnode_operations vector
> that has .add_links, which in turn could add a link.

If the wifi node was a standard provider (clocks, resets, etc.) to the
BT node, it does just work with DT. The issue here is either you'd
have some custom property or no property and the BT side driver just
knows there is a dependency to create. That case is what .add_links is
for IIRC.

> ... My problem at this point would be that I currently load the wireless
> driver as a module as it's vendor provided out of tree... (it's loaded
> through its pci alias, I guess it's udev checking depmod infos? not
> familiar how that part of autoloading really works...)

Well, that's a fun complication. I guess it has no DT info? You can
populate your DT with the necessary PCI nodes to represent the wifi
device. Under the PCI host node, you'll need at least a root port node
and then probably the wifi device is under that. It's got to match the
hierarchy to assign a device_node ptr to the PCI device.

Module aliases are the magic that makes the autoloading work.

> But that makes me think that rather than defining the bluetooth serdev
> in dts early, I could try to have the wireless driver create it once
> it's ready? hmm...

That is yet another option. The wireless driver could create the BT
device when ready. The issue there is serdev devices created
asynchronously isn't supported. serdev looks if the serial device has
a child node and will register with serdev and create the serdev
device. Otherwise, the serial device is bound to the tty layer.

Rob