[PATCH v4 1/3] dt-bindings: soc: qcom: Add local-bd-address property to WCNSS-BT

[PATCH v4 1/3] dt-bindings: soc: qcom: Add local-bd-address property to WCNSS-BT

[Loic Poulain]

Add optional local-bd-address property which is a 6-byte array
storing the assigned BD address. Since having a unique BD address
is critical, a per-device property value should be allocated.
This property is usually added by the boot loader which has access
to the provisioned data.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 v2: Set device as unconfigured if default address detected
     Add warning if BD addr retrieved from DT
 v3: if no addr retrieved from DT, unconditionally set
     the invalid BD addr flag.
     swap and set bdaddr in the platform probe
 v4: Add dt-bindings documentation
     split patch in two parts (setup, dt prop)
     use local-bd-address name instead of local-mac-address

 Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
index 4ea39e9186a7..1edfcdc6e267 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
@@ -37,6 +37,12 @@ The following properties are defined to the bluetooth node:
 	Definition: must be:
 		    "qcom,wcnss-bt"
 
+- local-bd-address:
+	Usage: optional
+	Value type: <u8 array>
+	Definition: should specify the unique 6-byte BD address assigned to the
+		    BT controller. Usually added at runtime by the boot loader.
+
 == WiFi
 The following properties are defined to the WiFi node:
 
-- 
2.13.0

[PATCH v4 2/3] Bluetooth: btqcomsmd: BD address setup

[Loic Poulain]

This patch implements the hdev setup function.
wcnss-bt does not have persistent memory to store
an allocated BD address. The device is therefore
marked as unconfigured if no BD address has been
previously retrieved.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 v2: Set device as unconfigured if default address detected
     Add warning if BD addr retrieved from DT
 v3: if no addr retrieved from DT, unconditionally set
     the invalid BD addr flag.
     swap and set bdaddr in the platform probe
 v4: Add dt-bindings documentation
     split patch in two parts (setup, dt prop)
     use local-bd-address name instead of local-mac-address

 drivers/bluetooth/btqcomsmd.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index ef730c173d4b..c70fae75c4ad 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c
@@ -26,6 +26,7 @@
 struct btqcomsmd {
 	struct hci_dev *hdev;
 
+	bdaddr_t bdaddr;
 	struct rpmsg_endpoint *acl_channel;
 	struct rpmsg_endpoint *cmd_channel;
 };
@@ -100,6 +101,29 @@ static int btqcomsmd_close(struct hci_dev *hdev)
 	return 0;
 }
 
+static int btqcomsmd_setup(struct hci_dev *hdev)
+{
+	struct btqcomsmd *btq = hci_get_drvdata(hdev);
+	struct sk_buff *skb;
+
+	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb))
+		return PTR_ERR(skb);
+	kfree_skb(skb);
+
+	/* Device does not have persistent storage for BD address.
+	 * Mark the device with invalid BD addr flag if no address
+	 * retrieved during probe.
+	 */
+	if (!bacmp(&btq->bdaddr, BDADDR_ANY)) {
+		bt_dev_info(hdev, "No BD address configured");
+		set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
+		return 0;
+	}
+
+	return qca_set_bdaddr_rome(hdev, &btq->bdaddr);
+}
+
 static int btqcomsmd_probe(struct platform_device *pdev)
 {
 	struct btqcomsmd *btq;
@@ -135,6 +159,7 @@ static int btqcomsmd_probe(struct platform_device *pdev)
 	hdev->open = btqcomsmd_open;
 	hdev->close = btqcomsmd_close;
 	hdev->send = btqcomsmd_send;
+	hdev->setup = btqcomsmd_setup;
 	hdev->set_bdaddr = qca_set_bdaddr_rome;
 
 	ret = hci_register_dev(hdev);
-- 
2.13.0

Re: [PATCH v4 2/3] Bluetooth: btqcomsmd: BD address setup

[Marcel Holtmann]

Hi Loic,

> This patch implements the hdev setup function.
> wcnss-bt does not have persistent memory to store
> an allocated BD address. The device is therefore
> marked as unconfigured if no BD address has been
> previously retrieved.
> 
> Signed-off-by: Loic Poulain <loic.poulain-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> v2: Set device as unconfigured if default address detected
>     Add warning if BD addr retrieved from DT
> v3: if no addr retrieved from DT, unconditionally set
>     the invalid BD addr flag.
>     swap and set bdaddr in the platform probe
> v4: Add dt-bindings documentation
>     split patch in two parts (setup, dt prop)
>     use local-bd-address name instead of local-mac-address
> 
> drivers/bluetooth/btqcomsmd.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
> index ef730c173d4b..c70fae75c4ad 100644
> --- a/drivers/bluetooth/btqcomsmd.c
> +++ b/drivers/bluetooth/btqcomsmd.c
> @@ -26,6 +26,7 @@
> struct btqcomsmd {
> 	struct hci_dev *hdev;
> 
> +	bdaddr_t bdaddr;
> 	struct rpmsg_endpoint *acl_channel;
> 	struct rpmsg_endpoint *cmd_channel;
> };
> @@ -100,6 +101,29 @@ static int btqcomsmd_close(struct hci_dev *hdev)
> 	return 0;
> }
> 
> +static int btqcomsmd_setup(struct hci_dev *hdev)
> +{
> +	struct btqcomsmd *btq = hci_get_drvdata(hdev);
> +	struct sk_buff *skb;
> +
> +	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb))
> +		return PTR_ERR(skb);
> +	kfree_skb(skb);
> +
> +	/* Device does not have persistent storage for BD address.
> +	 * Mark the device with invalid BD addr flag if no address
> +	 * retrieved during probe.
> +	 */
> +	if (!bacmp(&btq->bdaddr, BDADDR_ANY)) {
> +		bt_dev_info(hdev, "No BD address configured");
> +		set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
> +		return 0;
> +	}
> +
> +	return qca_set_bdaddr_rome(hdev, &btq->bdaddr);
> +}
> +

actually I applied a modified version of your patch that also sets invalid BD address when changing the address fails. I think there is no good reason to fail ->setup(). Let them fix that in userspace since the device is actually functional.

Regards

Marcel

[PATCH v4 3/3] Bluetooth: btqcomsmd: retieve BD address from DT

[Loic Poulain]

Retrieve BD address from the DT local-bd-address property.
This address must be unique and is usually added in the DT
by the bootloader which has access to the provisioned data.

Signed-off-by: Loic Poulain <loic.poulain-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 v2: Set device as unconfigured if default address detected
     Add warning if BD addr retrieved from DT
 v3: if no addr retrieved from DT, unconditionally set
     the invalid BD addr flag.
     swap and set bdaddr in the platform probe
 v4: Add dt-bindings documentation
     split patch in two parts (setup, dt prop)
     use local-bd-address name instead of local-mac-address

 drivers/bluetooth/btqcomsmd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index c70fae75c4ad..08a83cb30af6 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c
@@ -15,6 +15,8 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/rpmsg.h>
+#include <linux/of.h>
+
 #include <linux/soc/qcom/wcnss_ctrl.h>
 #include <linux/platform_device.h>
 
@@ -126,6 +128,7 @@ static int btqcomsmd_setup(struct hci_dev *hdev)
 
 static int btqcomsmd_probe(struct platform_device *pdev)
 {
+	const bdaddr_t *local_bd_addr;
 	struct btqcomsmd *btq;
 	struct hci_dev *hdev;
 	void *wcnss;
@@ -147,6 +150,18 @@ static int btqcomsmd_probe(struct platform_device *pdev)
 	if (IS_ERR(btq->cmd_channel))
 		return PTR_ERR(btq->cmd_channel);
 
+	/* The local-bd-address DT property is usually injected by the
+	 * bootloader which has access to the allocated BD address.
+	 */
+	local_bd_addr = of_get_property(pdev->dev.of_node, "local-mac-address",
+					&ret);
+	if (local_bd_addr && ret == sizeof(bdaddr_t)) {
+		/* local-bd-address stored with most significant byte first */
+		baswap(&btq->bdaddr, local_bd_addr);
+		BT_INFO("BD address %pMR retrieved from device-tree",
+			&btq->bdaddr);
+	}
+
 	hdev = hci_alloc_dev();
 	if (!hdev)
 		return -ENOMEM;
-- 
2.13.0

Re: [PATCH v4 3/3] Bluetooth: btqcomsmd: retieve BD address from DT

[Marcel Holtmann]

Hi Loic,

> Retrieve BD address from the DT local-bd-address property.
> This address must be unique and is usually added in the DT
> by the bootloader which has access to the provisioned data.
> 
> Signed-off-by: Loic Poulain <loic.poulain-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> v2: Set device as unconfigured if default address detected
>     Add warning if BD addr retrieved from DT
> v3: if no addr retrieved from DT, unconditionally set
>     the invalid BD addr flag.
>     swap and set bdaddr in the platform probe
> v4: Add dt-bindings documentation
>     split patch in two parts (setup, dt prop)
>     use local-bd-address name instead of local-mac-address
> 
> drivers/bluetooth/btqcomsmd.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
> index c70fae75c4ad..08a83cb30af6 100644
> --- a/drivers/bluetooth/btqcomsmd.c
> +++ b/drivers/bluetooth/btqcomsmd.c
> @@ -15,6 +15,8 @@
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/rpmsg.h>
> +#include <linux/of.h>
> +
> #include <linux/soc/qcom/wcnss_ctrl.h>
> #include <linux/platform_device.h>
> 
> @@ -126,6 +128,7 @@ static int btqcomsmd_setup(struct hci_dev *hdev)
> 
> static int btqcomsmd_probe(struct platform_device *pdev)
> {
> +	const bdaddr_t *local_bd_addr;
> 	struct btqcomsmd *btq;
> 	struct hci_dev *hdev;
> 	void *wcnss;
> @@ -147,6 +150,18 @@ static int btqcomsmd_probe(struct platform_device *pdev)
> 	if (IS_ERR(btq->cmd_channel))
> 		return PTR_ERR(btq->cmd_channel);
> 
> +	/* The local-bd-address DT property is usually injected by the
> +	 * bootloader which has access to the allocated BD address.
> +	 */
> +	local_bd_addr = of_get_property(pdev->dev.of_node, "local-mac-address",
> +					&ret);
> +	if (local_bd_addr && ret == sizeof(bdaddr_t)) {
> +		/* local-bd-address stored with most significant byte first */
> +		baswap(&btq->bdaddr, local_bd_addr);

actually my preference would be really that the address is coming from DT in the right order. We are defining what local-bd-address means. So lets do that in Bluetooth specified order and not confuse people even more.

And btw. I am fine if you just use const bdaddr_t *bdaddr here as temporary variable name.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] Bluetooth: btqcomsmd: retieve BD address from DT

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3540 bytes --]

Hi Loic,

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.13]
[cannot apply to bluetooth-next/master next-20170907]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Loic-Poulain/dt-bindings-soc-qcom-Add-local-bd-address-property-to-WCNSS-BT/20170908-102410
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   drivers//bluetooth/btqcomsmd.c: In function 'btqcomsmd_probe':
>> drivers//bluetooth/btqcomsmd.c:160:3: warning: passing argument 2 of 'baswap' discards 'const' qualifier from pointer target type
      baswap(&btq->bdaddr, local_bd_addr);
      ^
   In file included from drivers//bluetooth/btqcomsmd.c:23:0:
   include/net/bluetooth/bluetooth.h:236:6: note: expected 'struct bdaddr_t *' but argument is of type 'const struct bdaddr_t *'
    void baswap(bdaddr_t *dst, bdaddr_t *src);
         ^

vim +160 drivers//bluetooth/btqcomsmd.c

   128	
   129	static int btqcomsmd_probe(struct platform_device *pdev)
   130	{
   131		const bdaddr_t *local_bd_addr;
   132		struct btqcomsmd *btq;
   133		struct hci_dev *hdev;
   134		void *wcnss;
   135		int ret;
   136	
   137		btq = devm_kzalloc(&pdev->dev, sizeof(*btq), GFP_KERNEL);
   138		if (!btq)
   139			return -ENOMEM;
   140	
   141		wcnss = dev_get_drvdata(pdev->dev.parent);
   142	
   143		btq->acl_channel = qcom_wcnss_open_channel(wcnss, "APPS_RIVA_BT_ACL",
   144							   btqcomsmd_acl_callback, btq);
   145		if (IS_ERR(btq->acl_channel))
   146			return PTR_ERR(btq->acl_channel);
   147	
   148		btq->cmd_channel = qcom_wcnss_open_channel(wcnss, "APPS_RIVA_BT_CMD",
   149							   btqcomsmd_cmd_callback, btq);
   150		if (IS_ERR(btq->cmd_channel))
   151			return PTR_ERR(btq->cmd_channel);
   152	
   153		/* The local-bd-address DT property is usually injected by the
   154		 * bootloader which has access to the allocated BD address.
   155		 */
   156		local_bd_addr = of_get_property(pdev->dev.of_node, "local-mac-address",
   157						&ret);
   158		if (local_bd_addr && ret == sizeof(bdaddr_t)) {
   159			/* local-bd-address stored with most significant byte first */
 > 160			baswap(&btq->bdaddr, local_bd_addr);
   161			BT_INFO("BD address %pMR retrieved from device-tree",
   162				&btq->bdaddr);
   163		}
   164	
   165		hdev = hci_alloc_dev();
   166		if (!hdev)
   167			return -ENOMEM;
   168	
   169		hci_set_drvdata(hdev, btq);
   170		btq->hdev = hdev;
   171		SET_HCIDEV_DEV(hdev, &pdev->dev);
   172	
   173		hdev->bus = HCI_SMD;
   174		hdev->open = btqcomsmd_open;
   175		hdev->close = btqcomsmd_close;
   176		hdev->send = btqcomsmd_send;
   177		hdev->setup = btqcomsmd_setup;
   178		hdev->set_bdaddr = qca_set_bdaddr_rome;
   179	
   180		ret = hci_register_dev(hdev);
   181		if (ret < 0) {
   182			hci_free_dev(hdev);
   183			return ret;
   184		}
   185	
   186		platform_set_drvdata(pdev, btq);
   187	
   188		return 0;
   189	}
   190	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51034 bytes --]

Re: [PATCH v4 1/3] dt-bindings: soc: qcom: Add local-bd-address property to WCNSS-BT

[Marcel Holtmann]

Hi Loic,

> Add optional local-bd-address property which is a 6-byte array
> storing the assigned BD address. Since having a unique BD address
> is critical, a per-device property value should be allocated.
> This property is usually added by the boot loader which has access
> to the provisioned data.
> 
> Signed-off-by: Loic Poulain <loic.poulain-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> v2: Set device as unconfigured if default address detected
>     Add warning if BD addr retrieved from DT
> v3: if no addr retrieved from DT, unconditionally set
>     the invalid BD addr flag.
>     swap and set bdaddr in the platform probe
> v4: Add dt-bindings documentation
>     split patch in two parts (setup, dt prop)
>     use local-bd-address name instead of local-mac-address
> 
> Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> index 4ea39e9186a7..1edfcdc6e267 100644
> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> @@ -37,6 +37,12 @@ The following properties are defined to the bluetooth node:
> 	Definition: must be:
> 		    "qcom,wcnss-bt"
> 
> +- local-bd-address:
> +	Usage: optional
> +	Value type: <u8 array>
> +	Definition: should specify the unique 6-byte BD address assigned to the
> +		    BT controller. Usually added at runtime by the boot loader.
> +

as mentioned in my other email, I would really add an example on how to format this. Especially since these are formatted in little-endian order. So adding an example of “00:11:22:33:44:55” string representation on how it converts to [ 55 44 33 22 11 00 ] would be super helpful.

It might also be useful to create a generic bluetooth.txt similar to ethernet.txt. And then just reference it from here.

Of course we need to have Rob sign off on this and also make sure the boot loader people get this done correctly.

Regards

Marcel