Bluetooth: hci_ll: Get BD address from NVMEM (was "bluetooth: hci_ll: Get MAC address from NVMEM")
Subsystems (4)
v2 cover Bluetooth: hci_ll: Get BD address from NVMEM (was "bluetooth: hci_ll: Get MAC address from NVMEM")
This series adds supporting getting the BD address from a NVMEM provider for "LL" HCI controllers (Texas Instruments).
v2 changes:
* Fixed typos in dt-bindings
* Use "bd-address" instead of "mac-address"
* Updated dt-bindings to specify the byte order of "bd-address"
* New patch "Bluetooth: hci_ll: add support for setting public address"
* Dropped patch "Bluetooth: hci_ll: add constant for vendor-specific command" that is already in bluetooth-next
* Rework error handling
* Use bdaddr_t, bacmp and other bluetooth utils
David Lechner (3):
Bluetooth: hci_ll: add support for setting public address
dt-bindings: Add optional nvmem BD address bindings to ti,wlink-st
Bluetooth: hci_ll: Add optional nvmem BD address source
.../devicetree/bindings/net/ti,wilink-st.txt | 5 ++ drivers/bluetooth/hci_ll.c | 71 ++++++++++++++++++++++ 2 files changed, 76 insertions(+)
Signature · 1 line
2.7.4
PATCH v2 1/3 Bluetooth: hci_ll: add support for setting public address
This adds support for setting the public address on Texas Instruments Bluetooth chips using a vendor-specific command.
This has been tested on a CC2560A. The TI wiki also indicates that this command should work on TI WL17xx/WL18xx Bluetooth chips.
---
v2 changes:
* This is a new patch in v2
drivers/bluetooth/hci_ll.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
drivers/bluetooth/hci_ll.c +17 -0
@@ -57,6 +57,7 @@#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 */
@@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)return err; } +static int ll_set_bdaddr (struct hci_dev *hdev,const bdaddr_t *bdaddr) +{ +bdaddr_t bdaddr_swapped; +struct sk_buff *skb; + +baswap (&bdaddr_swapped, bdaddr); + skb =__hci_cmd_sync (hdev,HCI_VS_WRITE_BD_ADDR ,sizeof (bdaddr_t), + &bdaddr_swapped,HCI_INIT_TIMEOUT ); +if (!IS_ERR (skb)) +kfree_skb (skb); + +return PTR_ERR_OR_ZERO (skb); +} +static int ll_setup (struct hci_uart *hu) {int err, retry =3 ;
@@ -674,6 +689,8 @@ static int ll_setup(struct hci_uart *hu) lldev =serdev_device_get_drvdata (serdev); + hu->hdev->set_bdaddr = ll_set_bdaddr; +serdev_device_set_flow_control (serdev, true); do {
Signature · 1 line
2.7.4
Marcel Holtmann
maintainerRe: [PATCH v2 1/3] Bluetooth: hci_ll: add support for setting public address
Hi David,
Quoted 32 lines
This adds support for setting the public address on Texas Instruments Bluetooth chips using a vendor-specific command.
This has been tested on a CC2560A. The TI wiki also indicates that this command should work on TI WL17xx/WL18xx Bluetooth chips.
Signed-off-by David Lechner <[email protected]>---
v2 changes:
* This is a new patch in v2
drivers/bluetooth/hci_ll.c| 17 +++++++++++++++++1 file changed, 17 insertions(+)drivers/bluetooth/hci_ll.c +2
-0@@ -57,6 +57,7 @@#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 */ @@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)return err; } +static int ll_set_bdaddr (struct hci_dev *hdev,const bdaddr_t *bdaddr)
drivers/bluetooth/hci_ll.c static int download_firmware(struct ll_device *lldev) · line 676
Quoted 10 lines
drivers/bluetooth/hci_ll.c +10
-0+{ +bdaddr_t bdaddr_swapped; +struct sk_buff *skb; + +baswap (&bdaddr_swapped, bdaddr); + skb =__hci_cmd_sync (hdev,HCI_VS_WRITE_BD_ADDR ,sizeof (bdaddr_t), + &bdaddr_swapped,HCI_INIT_TIMEOUT ); +if (!IS_ERR (skb)) +kfree_skb (skb); +
You have a trailing whitespace here.
Does the HCI command really expect the BD_ADDR in the swapped order. The caller of hdev->set_bdaddr while provide it in the same order as the HCI Read BD Address command and everything in HCI. So it seems odd that you have to swap it for the vendor command.
So have you actually tested this with btmgmt public-add <xx:xx..> and checked that the address comes out correctly. I think ll_set_bdaddr should function correctly for the mgmt interface. And if needed any other caller outside of mgmt has to do the swapping.
Regards
Marcel
Re: [PATCH v2 1/3] Bluetooth: hci_ll: add support for setting public address
Quoted 47 lines · Marcel Holtmann
Hi David,
Quoted 32 lines
This adds support for setting the public address on Texas Instruments Bluetooth chips using a vendor-specific command.
This has been tested on a CC2560A. The TI wiki also indicates that this command should work on TI WL17xx/WL18xx Bluetooth chips.
Signed-off-by David Lechner <[email protected]>---
v2 changes:
* This is a new patch in v2
drivers/bluetooth/hci_ll.c| 17 +++++++++++++++++1 file changed, 17 insertions(+)drivers/bluetooth/hci_ll.c +2
-0@@ -57,6 +57,7 @@#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 */ @@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)return err; } +static int ll_set_bdaddr (struct hci_dev *hdev,const bdaddr_t *bdaddr)drivers/bluetooth/hci_ll.c static int download_firmware(struct ll_device *lldev) · line 676
Quoted 10 lines
drivers/bluetooth/hci_ll.c +10
-0+{ +bdaddr_t bdaddr_swapped; +struct sk_buff *skb; + +baswap (&bdaddr_swapped, bdaddr); + skb =__hci_cmd_sync (hdev,HCI_VS_WRITE_BD_ADDR ,sizeof (bdaddr_t), + &bdaddr_swapped,HCI_INIT_TIMEOUT ); +if (!IS_ERR (skb)) +kfree_skb (skb); +You have a trailing whitespace here.
Quoted 4 lines
Does the HCI command really expect the
BD_ADDRin the swapped order. The caller ofhdev->set_bdaddrwhile provide it in the same order as the HCI Read BD Address command and everything in HCI. So it seems odd that you have to swap it for the vendor command.So have you actually tested this with btmgmt public-add <xx:xx..> and checked that the address comes out correctly. I think
ll_set_bdaddrshould function correctly for the mgmt interface. And if needed any other caller outside of mgmt has to do the swapping.
I did test using btmgmt public-address 00:11:22:33:44:55, which is how I found out that the order needed to be swapped. Like you, I was surprised. I couldn't find any documentation from TI saying what the order is supposed to be, so I can only assume that because this works, it is indeed correct as-is.
Marcel Holtmann
maintainerRe: [PATCH v2 1/3] Bluetooth: hci_ll: add support for setting public address
Hi David,
Quoted 46 lines
Quoted 31 lines
Quoted 31 lines
This adds support for setting the public address on Texas Instruments Bluetooth chips using a vendor-specific command.
This has been tested on a CC2560A. The TI wiki also indicates that this command should work on TI WL17xx/WL18xx Bluetooth chips.
Signed-off-by David Lechner <[email protected]>---
v2 changes:
* This is a new patch in v2
drivers/bluetooth/hci_ll.c| 17 +++++++++++++++++1 file changed, 17 insertions(+)drivers/bluetooth/hci_ll.c +1
-0@@ -57,6 +57,7 @@#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 */ @@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)return err; }Quoted 14 lines
drivers/bluetooth/hci_ll.c static int download_firmware(struct ll_device *lldev) · line 676
Quoted 11 lines
drivers/bluetooth/hci_ll.c +11
-0+static int ll_set_bdaddr (struct hci_dev *hdev,const bdaddr_t *bdaddr) +{ +bdaddr_t bdaddr_swapped; +struct sk_buff *skb; + +baswap (&bdaddr_swapped, bdaddr); + skb =__hci_cmd_sync (hdev,HCI_VS_WRITE_BD_ADDR ,sizeof (bdaddr_t), + &bdaddr_swapped,HCI_INIT_TIMEOUT ); +if (!IS_ERR (skb)) +kfree_skb (skb); +You have a trailing whitespace here.
Does the HCI command really expect theBD_ADDRin the swapped order. The caller ofhdev->set_bdaddrwhile provide it in the same order as the HCI Read BD Address command and everything in HCI. So it seems odd that you have to swap it for the vendor command. So have you actually tested this with btmgmt public-add <xx:xx..> and checked that the address comes out correctly. I thinkll_set_bdaddrshould function correctly for the mgmt interface. And if needed any other caller outside of mgmt has to do the swapping.
Quoted 1 line
I did test using btmgmt public-address 00:11:22:33:44:55, which is how I found out that the order needed to be swapped. Like you, I was surprised. I couldn't find any documentation from TI saying what the order is supposed to be, so I can only assume that because this works, it is indeed correct as-is.
then please add a comment for that and I would appreciate to have the parts from btmon showing the public-addr command and the following HCI Read BD Address command as part of the commit message. Just for being able to dig this out at some later point if needed.
Regards
Marcel
PATCH v2 2/3 dt-bindings: Add optional nvmem BD address bindings to ti,wlink-st
In reply to David LechnerThis adds optional nvmem consumer properties to the ti,wlink-st device tree bindings to allow specifying the BD address.
---
v2 changes:
* Renamed "mac-address" to "bd-address"
* Fixed typos in example
* Specify byte order of "bd-address"
Documentation/devicetree/bindings/net/ti,wilink-st.txt | 5 +++++ 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/net/ti,wilink-st.txt +5 -0
@@ -32,6 +32,9 @@ 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 6 byte BD address + with the most significant byte first (big-endian). + - nvmem-cell-names: "bd-address" (required when nvmem-cells is specified) Example:
@@ -43,5 +46,7 @@ Example: enable-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>; clocks = <&clk32k_wl18xx>; clock-names = "ext_clock"; + nvmem-cells = <&bd_address>; + nvmem-cell-names = "bd-address"; }; };
Signature · 1 line
2.7.4
Marcel Holtmann
maintainerRe: [PATCH v2 2/3] dt-bindings: Add optional nvmem BD address bindings to ti,wlink-st
Hi David,
Quoted 29 lines
This adds optional nvmem consumer properties to the ti,wlink-st device tree bindings to allow specifying the BD address.
Signed-off-by David Lechner <[email protected]>---
v2 changes:
* Renamed "mac-address" to "bd-address"
* Fixed typos in example
* Specify byte order of "bd-address"
Documentation/devicetree/bindings/net/ti,wilink-st.txt| 5 +++++1 file changed, 5 insertions(+)Documentation/devicetree/bindings/net/ti,wilink-st.txt +3
-0@@ -32,6 +32,9 @@ 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 6 byte BD address + with the most significant byte first (big-endian). + - nvmem-cell-names: "bd-address" (required when nvmem-cells is specified)Example:
Documentation/devicetree/bindings/net/ti,wilink-st.txt Example: · line 50
Quoted 5 lines
Documentation/devicetree/bindings/net/ti,wilink-st.txt +2
-0@@ -43,5 +46,7 @@ Example: enable-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>; clocks = <&clk32k_wl18xx>; clock-names = "ext_clock"; + nvmem-cells = <&bd_address>; + nvmem-cell-names = "bd-address”;
For me this looks good, but I like to get an extra ACK from Rob on this.
Regards
Marcel
Re: [PATCH v2 2/3] dt-bindings: Add optional nvmem BD address bindings to ti,wlink-st
In reply to David LechnerQuoted 6 lines · David Lechner
This adds optional nvmem consumer properties to the ti,wlink-st device tree bindings to allow specifying the BD address.
Signed-off-by David Lechner <[email protected]>---
Quoted 7 lines
v2 changes:
* Renamed "mac-address" to "bd-address"
* Fixed typos in example
* Specify byte order of "bd-address"Documentation/devicetree/bindings/net/ti,wilink-st.txt | 5 +++++ 1 file changed, 5 insertions(+)
PATCH v2 3/3 Bluetooth: hci_ll: Add optional nvmem BD address source
In reply to David LechnerThis adds an optional nvmem consumer to get a BD address from an external source. The BD 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).
---
v2 changes:
* Add support for HCI_QUIRK_INVALID_BDADDR when there is an error getting the BD address from nvmem
* Rework error handling
* rename "mac-address" to "bd-address"
* use bdaddr_t, bacmp and other bluetooth helper functions
* use ll_set_bdaddr() from new, separate patch
drivers/bluetooth/hci_ll.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
drivers/bluetooth/hci_ll.c +55 -0
@@ -53,6 +53,7 @@#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"
@@ -90,6 +91,7 @@ struct ll_device {struct serdev_device *serdev;struct gpio_desc *enable_gpio;struct clk *ext_clk; +bdaddr_t bdaddr; };struct ll_struct {
@@ -715,6 +717,19 @@ static int ll_setup(struct hci_uart *hu)if (err)return err; +/* Set BD address if one was specified at probe */ +if (!bacmp (&lldev->bdaddr,BDADDR_NONE )) { +/* +* This means that there was an error getting the BD address +* during probe, so mark the device as having a bad address. +*/ +set_bit (HCI_QUIRK_INVALID_BDADDR , &hu->hdev->quirks); + }else if (bacmp (&lldev->bdaddr,BDADDR_ANY )) { + err =ll_set_bdaddr (hu->hdev, &lldev->bdaddr); +if (err) +set_bit (HCI_QUIRK_INVALID_BDADDR , &hu->hdev->quirks); + } +/* Operational speed if any */ if (hu->oper_speed) speed = hu->oper_speed;
@@ -743,6 +758,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 );
@@ -764,6 +780,45 @@ 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 BD address from nvram */ + bdaddr_cell =nvmem_cell_get (&serdev->dev,"bd-address" ); +if (IS_ERR (bdaddr_cell)) { +int err =PTR_ERR (bdaddr_cell); + +if (err == -EPROBE_DEFER ) +return err; + +/* +* ENOENT means there is no matching nvmem cell and ENOSYS +* means that nvmem is not enabled in the kernel configuration. +*/ +if (err != -ENOENT && err != -ENOSYS ) { +/* +* If there was some other error, give userspace a +* chance to fix the problem instead of failing to load +* the driver. Using BDADDR_NONE as a flag that is +* tested later in the setup function. +*/ +dev_warn (&serdev->dev, +"Failed to get \"bd-address\" nvmem cell (%d)\n" , + err); +bacpy (&lldev->bdaddr,BDADDR_NONE ); + } + }else { +bdaddr_t *bdaddr; +int len; + + bdaddr =nvmem_cell_read (bdaddr_cell, &len); +if (len !=sizeof (bdaddr_t)) { +dev_err (&serdev->dev,"Invalid nvmem bd-address length\n" ); +nvmem_cell_put (bdaddr_cell); +return -EINVAL ; + } + +baswap (&lldev->bdaddr, bdaddr); +nvmem_cell_put (bdaddr_cell); + } +return hci_uart_register_device (hu, &llp); }
Signature · 1 line
2.7.4
Marcel Holtmann
maintainerRe: [PATCH v2 3/3] Bluetooth: hci_ll: Add optional nvmem BD address source
Hi David,
Quoted 75 lines
This adds an optional nvmem consumer to get a BD address from an external source. The BD 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 <[email protected]>---
v2 changes:
* Add support forHCI_QUIRK_INVALID_BDADDRwhen there is an error getting the BD address from nvmem
* Rework error handling
* rename "mac-address" to "bd-address"
* usebdaddr_t, bacmp and other bluetooth helper functions
* usell_set_bdaddr()from new, separate patch
drivers/bluetooth/hci_ll.c| 55 ++++++++++++++++++++++++++++++++++++++++++++++1 file changed, 55 insertions(+)drivers/bluetooth/hci_ll.c +18
-0@@ -53,6 +53,7 @@#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" @@ -90,6 +91,7 @@ struct ll_device {struct serdev_device *serdev;struct gpio_desc *enable_gpio;struct clk *ext_clk; +bdaddr_t bdaddr; };struct ll_struct {@@ -715,6 +717,19 @@ static int ll_setup(struct hci_uart *hu)if (err)return err; +/* Set BD address if one was specified at probe */ +if (!bacmp (&lldev->bdaddr,BDADDR_NONE )) { +/* +* This means that there was an error getting the BD address +* during probe, so mark the device as having a bad address. +*/ +set_bit (HCI_QUIRK_INVALID_BDADDR , &hu->hdev->quirks); + }else if (bacmp (&lldev->bdaddr,BDADDR_ANY )) { + err =ll_set_bdaddr (hu->hdev, &lldev->bdaddr); +if (err) +set_bit (HCI_QUIRK_INVALID_BDADDR , &hu->hdev->quirks); + } +/* Operational speed if any */ if (hu->oper_speed) speed = hu->oper_speed;@@ -743,6 +758,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 );@@ -764,6 +780,45 @@ 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 BD address from nvram */ + bdaddr_cell =nvmem_cell_get (&serdev->dev,"bd-address" );
drivers/bluetooth/hci_ll.c static int hci_ti_probe(struct serdev_device *serdev) · line 794
Quoted 10 lines
drivers/bluetooth/hci_ll.c +10
-0+if (IS_ERR (bdaddr_cell)) { +int err =PTR_ERR (bdaddr_cell); + +if (err == -EPROBE_DEFER ) +return err; + +/* +* ENOENT means there is no matching nvmem cell and ENOSYS +* means that nvmem is not enabled in the kernel configuration. +*/
Fix the comment style to this:
/* foo * bar */
Quoted 14 lines
drivers/bluetooth/hci_ll.c +14
-0+if (err != -ENOENT && err != -ENOSYS ) { +/* +* If there was some other error, give userspace a +* chance to fix the problem instead of failing to load +* the driver. Using BDADDR_NONE as a flag that is +* tested later in the setup function. +*/ +dev_warn (&serdev->dev, +"Failed to get \"bd-address\" nvmem cell (%d)\n" , + err); +bacpy (&lldev->bdaddr,BDADDR_NONE ); + } + }else { +bdaddr_t *bdaddr;
drivers/bluetooth/hci_ll.c static int hci_ti_probe(struct serdev_device *serdev) · line 818
Quoted 10 lines
drivers/bluetooth/hci_ll.c +10
-0+int len; + + bdaddr =nvmem_cell_read (bdaddr_cell, &len); +if (len !=sizeof (bdaddr_t)) { +dev_err (&serdev->dev,"Invalid nvmem bd-address length\n" ); +nvmem_cell_put (bdaddr_cell); +return -EINVAL ; + } + +baswap (&lldev->bdaddr, bdaddr);
This swapping needs a comment. Explain the format of the NVMEM storage and also which the HCI vendor command takes.
drivers/bluetooth/hci_ll.c static int hci_ti_probe(struct serdev_device *serdev) · line 823
Quoted 5 lines
drivers/bluetooth/hci_ll.c +3
-0+nvmem_cell_put (bdaddr_cell); + } +return hci_uart_register_device (hu, &llp); }
Regards
Marcel