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

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

[David Lechner]

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

[PATCH 1/3] dt-bindings: Add optional nvmem MAC address bindings to ti,wlink-st

[David Lechner]

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

Re: [PATCH 1/3] dt-bindings: Add optional nvmem MAC address bindings to ti,wlink-st

[Marcel Holtmann]

Hi David,

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

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.

> 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"
> 	};
> };

Regards

Marcel

Re: [PATCH 1/3] dt-bindings: Add optional nvmem MAC address bindings to ti,wlink-st

[Sergei Shtylyov]

Hello!

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

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

    s/:/ = /?

> +		nvmem-cell-names "mac-address"

    Where's =?

[...]

MBR, Sergei

Re: [PATCH 1/3] dt-bindings: Add optional nvmem MAC address bindings to ti,wlink-st

[Sergei Shtylyov]

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

>> 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>
> 
>     s/:/ = /?
> 
>> +        nvmem-cell-names "mac-address"
> 
>     Where's =?

    And ;? :-)

[...]

MBR, Sergei

[PATCH 2/3] bluetooth: hci_ll: add constant for vendor-specific command

[David Lechner]

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

Re: [PATCH 2/3] bluetooth: hci_ll: add constant for vendor-specific command

[Marcel Holtmann]

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@lechnology.com>
> ---
> drivers/bluetooth/hci_ll.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 2/3] bluetooth: hci_ll: add constant for vendor-specific command

[David Lechner]

On 12/04/2017 12:23 AM, Marcel Holtmann wrote:
> 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@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?

Re: [PATCH 2/3] bluetooth: hci_ll: add constant for vendor-specific command

[Marcel Holtmann]

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

[PATCH 3/3] bluetooth: hci_ll: Add optional nvmem MAC address source

[David Lechner]

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

Re: [PATCH 3/3] bluetooth: hci_ll: Add optional nvmem MAC address source

[Marcel Holtmann]

Hi David,

> 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

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.

> 
> /* HCILL commands */
> @@ -89,6 +91,7 @@ struct ll_device {
> 	struct serdev_device *serdev;
> 	struct gpio_desc *enable_gpio;
> 	struct clk *ext_clk;
> +	u8 *bdaddr;

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.

> };
> 
> 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) {

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

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

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.

> 	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”);

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

> +	if (IS_ERR(bdaddr_cell)) {
> +		int err = PTR_ERR(bdaddr_cell);
> +
> +		if (err != -ENOENT) {
> +			if (err != -EPROBE_DEFER)

	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