[PATCH 0/3] bluetooth: hci_ll: Get MAC address from NVMEM

This series adds supporting getting the MAC address from a NVMEM provider
for "LL" HCI controllers (Texas Instruments).

The second patch is just cleaning things up before I add another similar
vendor-specific HCI command constant in the final patch.

David Lechner (3):
  dt-bindings: Add optional nvmem MAC address bindings to ti,wlink-st
  bluetooth: hci_ll: add constant for vendor-specific command
  bluetooth: hci_ll: Add optional nvmem MAC address source

 .../devicetree/bindings/net/ti,wilink-st.txt       |  4 ++
 drivers/bluetooth/hci_ll.c                         | 43 +++++++++++++++++++++-
 2 files changed, 45 insertions(+), 2 deletions(-)

-- 
2.7.4

--
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

This adds a #define for the vendor-specific HCI command to set the
baudrate instead of using the bare 0xff36 multiple times.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/bluetooth/hci_ll.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
index 447abea..974a788 100644
--- a/drivers/bluetooth/hci_ll.c
+++ b/drivers/bluetooth/hci_ll.c

@@ -56,6 +56,9 @@
 
 #include "hci_uart.h"
 
+/* Vendor-specific HCI commands */
+#define HCI_VS_UPDATE_UART_HCI_BAUDRATE		0xff36
+
 /* HCILL commands */
 #define HCILL_GO_TO_SLEEP_IND	0x30
 #define HCILL_GO_TO_SLEEP_ACK	0x31

@@ -620,7 +623,7 @@ static int download_firmware(struct ll_device *lldev)
 		case ACTION_SEND_COMMAND:	/* action send */
 			bt_dev_dbg(lldev->hu.hdev, "S");
 			cmd = (struct hci_command *)action_ptr;
-			if (cmd->opcode == 0xff36) {
+			if (cmd->opcode == HCI_VS_UPDATE_UART_HCI_BAUDRATE) {
 				/* ignore remote change
 				 * baud rate HCI VS command
 				 */

@@ -704,7 +707,10 @@ static int ll_setup(struct hci_uart *hu)
 		speed = 0;
 
 	if (speed) {
-		struct sk_buff *skb = __hci_cmd_sync(hu->hdev, 0xff36, sizeof(speed), &speed, HCI_INIT_TIMEOUT);
+		struct sk_buff *skb;
+
+		skb = __hci_cmd_sync(hu->hdev, HCI_VS_UPDATE_UART_HCI_BAUDRATE,
+				     sizeof(speed), &speed, HCI_INIT_TIMEOUT);
 		if (!IS_ERR(skb)) {
 			kfree_skb(skb);
 			serdev_device_set_baudrate(serdev, speed);

-- 
2.7.4

This adds an optional nvmem consumer to get a MAC address from an external
source. The MAC address is then set in the Bluetooth chip after the
firmware has been loaded.

This has been tested working with a TI CC2560A chip (in a LEGO MINDSTORMS
EV3).

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/bluetooth/hci_ll.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
index 974a788..dd3b5c2 100644
--- a/drivers/bluetooth/hci_ll.c
+++ b/drivers/bluetooth/hci_ll.c

@@ -53,10 +53,12 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 #include <linux/gpio/consumer.h>
+#include <linux/nvmem-consumer.h>
 
 #include "hci_uart.h"
 
 /* Vendor-specific HCI commands */
+#define HCI_VS_WRITE_BD_ADDR			0xfc06
 #define HCI_VS_UPDATE_UART_HCI_BAUDRATE		0xff36
 
 /* HCILL commands */

@@ -89,6 +91,7 @@ struct ll_device {
 	struct serdev_device *serdev;
 	struct gpio_desc *enable_gpio;
 	struct clk *ext_clk;
+	u8 *bdaddr;
 };
 
 struct ll_struct {

@@ -698,6 +701,19 @@ static int ll_setup(struct hci_uart *hu)
 	if (err)
 		return err;
 
+	/* Set MAC address, if any */
+	if (lldev->bdaddr) {
+		struct sk_buff *skb;
+
+		skb = __hci_cmd_sync(hu->hdev, HCI_VS_WRITE_BD_ADDR, 6,
+				     lldev->bdaddr, HCI_INIT_TIMEOUT);
+		if (IS_ERR(skb))
+			bt_dev_err(hu->hdev, "Failed to set MAC address (%ld)",
+				   PTR_ERR(skb));
+		else
+			kfree_skb(skb);
+	}
+
 	/* Operational speed if any */
 	if (hu->oper_speed)
 		speed = hu->oper_speed;

@@ -726,6 +742,7 @@ static int hci_ti_probe(struct serdev_device *serdev)
 {
 	struct hci_uart *hu;
 	struct ll_device *lldev;
+	struct nvmem_cell *bdaddr_cell;
 	u32 max_speed = 3000000;
 
 	lldev = devm_kzalloc(&serdev->dev, sizeof(struct ll_device), GFP_KERNEL);

@@ -747,6 +764,22 @@ static int hci_ti_probe(struct serdev_device *serdev)
 	of_property_read_u32(serdev->dev.of_node, "max-speed", &max_speed);
 	hci_uart_set_speeds(hu, 115200, max_speed);
 
+	/* optional MAC address from nvram */
+	bdaddr_cell = nvmem_cell_get(&serdev->dev, "mac-address");
+	if (IS_ERR(bdaddr_cell)) {
+		int err = PTR_ERR(bdaddr_cell);
+
+		if (err != -ENOENT) {
+			if (err != -EPROBE_DEFER)
+				dev_err(&serdev->dev,
+					"Failed to get \"mac-address\" nvmem cell\n");
+			return err;
+		}
+	} else {
+		lldev->bdaddr = nvmem_cell_read(bdaddr_cell, NULL);
+		nvmem_cell_put(bdaddr_cell);
+	}
+
 	return hci_uart_register_device(hu, &llp);
 }
 

-- 
2.7.4

This adds optional nvmem consumer properties to the ti,wlink-st device tree
bindings to allow specifying the Bluetooth MAC address.

Signed-off-by: David Lechner <david@lechnology.com>
---
 Documentation/devicetree/bindings/net/ti,wilink-st.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,wilink-st.txt b/Documentation/devicetree/bindings/net/ti,wilink-st.txt
index 1649c1f..24eb897 100644
--- a/Documentation/devicetree/bindings/net/ti,wilink-st.txt
+++ b/Documentation/devicetree/bindings/net/ti,wilink-st.txt

@@ -32,6 +32,8 @@ Optional properties:
    See ../clocks/clock-bindings.txt for details.
  - clock-names : Must include the following entry:
    "ext_clock" (External clock provided to the TI combo chip).
+ - nvmem-cells: phandle to nvmem data cell that contains a MAC address
+ - nvmem-cell-names: "mac-address" (required when nvmem-cells is specified)
 
 Example:
 

@@ -43,5 +45,7 @@ Example:
 		enable-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
 		clocks = <&clk32k_wl18xx>;
 		clock-names = "ext_clock";
+		nvmem-cells: <&mac_address>
+		nvmem-cell-names "mac-address"
 	};
 };

-- 
2.7.4

Hi David,

This adds a #define for the vendor-specific HCI command to set the
baudrate instead of using the bare 0xff36 multiple times.

Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
---
drivers/bluetooth/hci_ll.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

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

Hi David,

the first patch should to add support for hdev->set_bdaddr. If the hardware supports changing the BD_ADDR, then it should also be exposed via Set Public Address mgmt command. For this it is required that the writing of the BD_ADDR is non-persistent over power cycles.

I would prefer if we just store it as bdaddr_t. That will work just fine when you created the set_bdaddr helper function that you need for hdev->set_bdaddr anyway.

And here I would really prefer to use bacmp. And example here would be btqcomsmd.c on how it is done.

While this is unrelated to this patch, when I seed the u32 speed and the direct use of it in a __hci_cmd_sync, then that is pretty obviously not endian safe. You might really want to fix that as well.

I prefer really to use the term “bd-address” like we discussed for the btqcomsmd.c driver as well.

	err != -ENOENT && err != -EPROBE_DEFER

+				dev_err(&serdev->dev,
+					"Failed to get \"mac-address\" nvmem cell\n”);

Also this is at most a warning. And more important, I would actually set the quirk BDADDR_INVALID here so that the device gets flagged as not having a valid address. The Set Public Address mgmt command can then be used from userspace to handle that situation.

+			return err;
+		}
+	} else {
+		lldev->bdaddr = nvmem_cell_read(bdaddr_cell, NULL);
+		nvmem_cell_put(bdaddr_cell);
+	}
+
	return hci_uart_register_device(hu, &llp);
}

Regards

Marcel

Hi David,

lets call these “bt-address” since they are not really MAC addresses. They have special formatting etc. Also I prefer that the actual format on how they are stored is described. See Documentation/devicetree/bindings/net/bluetooth.txt for an example.

Regards

Marcel

Hello!

On 12/4/2017 6:21 AM, David Lechner wrote:

   s/:/ = /?

+		nvmem-cell-names "mac-address"

   Where's =?

[...]

MBR, Sergei
--
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

On 12/4/2017 12:54 PM, Sergei Shtylyov wrote:

quoted

This adds optional nvmem consumer properties to the ti,wlink-st device tree
bindings to allow specifying the Bluetooth MAC address.

Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
---
  Documentation/devicetree/bindings/net/ti,wilink-st.txt | 4 ++++
  1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,wilink-st.txt b/Documentation/devicetree/bindings/net/ti,wilink-st.txt
index 1649c1f..24eb897 100644
--- a/Documentation/devicetree/bindings/net/ti,wilink-st.txt
+++ b/Documentation/devicetree/bindings/net/ti,wilink-st.txt

@@ -32,6 +32,8 @@ Optional properties:

     See ../clocks/clock-bindings.txt for details.
   - clock-names : Must include the following entry:
     "ext_clock" (External clock provided to the TI combo chip).
+ - nvmem-cells: phandle to nvmem data cell that contains a MAC address
+ - nvmem-cell-names: "mac-address" (required when nvmem-cells is specified)
  Example:

@@ -43,5 +45,7 @@ Example:

          enable-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
          clocks = <&clk32k_wl18xx>;
          clock-names = "ext_clock";
+        nvmem-cells: <&mac_address>

    s/:/ = /?

quoted

+        nvmem-cell-names "mac-address"

    Where's =?

   And ;? :-)

[...]

MBR, Sergei
--
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

On 12/04/2017 12:23 AM, Marcel Holtmann wrote:

Hi David,

quoted

This adds a #define for the vendor-specific HCI command to set the
baudrate instead of using the bare 0xff36 multiple times.

Signed-off-by: David Lechner <david@lechnology.com>
---
drivers/bluetooth/hci_ll.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

I am new to the bluetooth tree, so just to be sure... does this mean I should not include this patch in v2 of this series since this patch has been applied?

Hi David,

quoted

quoted

This adds a #define for the vendor-specific HCI command to set the
baudrate instead of using the bare 0xff36 multiple times.

Signed-off-by: David Lechner <david@lechnology.com>
---
drivers/bluetooth/hci_ll.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

I am new to the bluetooth tree, so just to be sure... does this mean I should not include this patch in v2 of this series since this patch has been applied?

please don’t include already applied patches. Base everything against bluetooth-next tree.

Regards

Marcel