2020-07-02 12:02:58

by K, Kiran

[permalink] [raw]
Subject: [PATCH 0/5] Refactor firmware download

Hi,

This patchset series refactors firmware download sequence to accomodate
new generation Intel controllers. Few functions have been split to enhance
readability and reusability.

Kiran K (5):
Bluetooth: btintel: Make controller version read generic
Bluetooth: btintel: Refactor firmware header download sequence
Bluetooth: btintel: Refactor firmware payload download code
Bluetooth: btintel: Define tlv structure for new generation
Controllers
Bluetooth: btintel: Parse controller information present in TLV format

drivers/bluetooth/btintel.c | 223 ++++++++++++++++++++++++++++++----
drivers/bluetooth/btintel.h | 110 +++++++++++++++--
drivers/bluetooth/btusb.c | 73 +++++++----
drivers/bluetooth/hci_ag6xx.c | 12 +-
drivers/bluetooth/hci_intel.c | 14 ++-
5 files changed, 369 insertions(+), 63 deletions(-)

--
2.17.1


2020-07-02 12:03:39

by K, Kiran

[permalink] [raw]
Subject: [PATCH 2/5] Bluetooth: btintel: Refactor firmware header download sequence

Move firmware header download code to a separate function to
enhance readability and reusability

Signed-off-by: Kiran K <[email protected]>
Signed-off-by: Amit K Bag <[email protected]>
Signed-off-by: Raghuram Hegde <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Sathish Narasimman <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
drivers/bluetooth/btintel.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index dea96c585ecb..1c820c187421 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -646,12 +646,10 @@ int btintel_read_boot_params(struct hci_dev *hdev,
}
EXPORT_SYMBOL_GPL(btintel_read_boot_params);

-int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
- u32 *boot_param)
+static int btintel_sfi_rsa_header_secure_send(struct hci_dev *hdev,
+ const struct firmware *fw)
{
int err;
- const u8 *fw_ptr;
- u32 frag_len;

/* Start the firmware download transaction with the Init fragment
* represented by the 128 bytes of CSS header.
@@ -679,6 +677,21 @@ int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
bt_dev_err(hdev, "Failed to send firmware signature (%d)", err);
goto done;
}
+done:
+ return err;
+}
+
+int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
+ u32 *boot_param)
+{
+ int err;
+ const u8 *fw_ptr;
+ u32 frag_len;
+
+ err = btintel_sfi_rsa_header_secure_send(hdev, fw);
+ if (err)
+ goto done;
+

fw_ptr = fw->data + 644;
frag_len = 0;
--
2.17.1

2020-07-02 12:03:57

by K, Kiran

[permalink] [raw]
Subject: [PATCH 4/5] Bluetooth: btintel: Define tlv structure for new generation Controllers

Define structure used for reading controller information and
to downloading firmware in tlv format used for new generation
Intel controllers

Signed-off-by: Kiran K <[email protected]>
Signed-off-by: Amit K Bag <[email protected]>
Signed-off-by: Raghuram Hegde <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Sathish Narasimman <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
drivers/bluetooth/btintel.h | 85 +++++++++++++++++++++++++++++++++++++
1 file changed, 85 insertions(+)

diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index e476105d495b..e37f84155c50 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -6,6 +6,90 @@
* Copyright (C) 2015 Intel Corporation
*/

+/* List of tlv type */
+enum {
+ INTEL_TLV_CNVI_TOP = 0x10,
+ INTEL_TLV_CNVR_TOP,
+ INTEL_TLV_CNVI_BT,
+ INTEL_TLV_CNVR_BT,
+ INTEL_TLV_CNVI_OTP,
+ INTEL_TLV_CNVR_OTP,
+ INTEL_TLV_DEV_REV_ID,
+ INTEL_TLV_USB_VENDOR_ID,
+ INTEL_TLV_USB_PRODUCT_ID,
+ INTEL_TLV_PCIE_VENDOR_ID,
+ INTEL_TLV_PCIE_DEVICE_ID,
+ INTEL_TLV_PCIE_SUBSYSTEM_ID,
+ INTEL_TLV_IMAGE_TYPE,
+ INTEL_TLV_TIME_STAMP,
+ INTEL_TLV_BUILD_TYPE,
+ INTEL_TLV_BUILD_NUM,
+ INTEL_TLV_FW_BUILD_PRODUCT,
+ INTEL_TLV_FW_BUILD_HW,
+ INTEL_TLV_FW_STEP,
+ INTEL_TLV_BT_SPEC,
+ INTEL_TLV_MFG_NAME,
+ INTEL_TLV_HCI_REV,
+ INTEL_TLV_LMP_SUBVER,
+ INTEL_TLV_OTP_PATCH_VER,
+ INTEL_TLV_SECURE_BOOT,
+ INTEL_TLV_KEY_FROM_HDR,
+ INTEL_TLV_OTP_LOCK,
+ INTEL_TLV_API_LOCK,
+ INTEL_TLV_DEBUG_LOCK,
+ INTEL_TLV_MIN_FW,
+ INTEL_TLV_LIMITED_CCE,
+ INTEL_TLV_SBE_TYPE,
+ INTEL_TLV_OTP_BDADDR,
+ INTEL_TLV_UNLOCKED_STATE
+};
+
+struct intel_tlv {
+ u8 type;
+ u8 len;
+ u8 val[0];
+} __packed;
+
+struct intel_version_tlv {
+ u8 status;
+ u32 cnvi_top;
+ u32 cnvr_top;
+ u32 cnvi_bt;
+ u32 cnvr_bt;
+ u16 cnvi_otp;
+ u16 cnvr_otp;
+ u16 dev_rev_id;
+ u16 usb_vid;
+ u16 usb_pid;
+ u16 pcie_vendor_id;
+ u16 pcie_dev_id;
+ u16 pcie_subsys_id;
+ u8 img_type;
+ u16 timestamp;
+ u8 build_type;
+ u32 build_num;
+ u8 fw_build_prod;
+ u8 fw_build_hw;
+ u8 fw_build_step;
+ u8 bt_spec_ver;
+ u16 mfg_name;
+ u16 hci_rev;
+ u16 lmp_sub_ver;
+ u8 otp_patch_ver;
+ u8 secure_boot;
+ u8 key_from_hdr;
+ u8 otp_lock;
+ u8 api_lock;
+ u8 debug_lock;
+ u8 min_fw_build_nn;
+ u8 min_fw_build_cw;
+ u8 min_fw_build_yy;
+ u8 limited_cce;
+ u8 sbe_type;
+ bdaddr_t otp_bd_addr;
+ u8 unlocked_state;
+} __packed;
+
struct intel_version {
u8 status;
u8 hw_platform;
@@ -70,6 +154,7 @@ struct btintel_version {
bool is_tlv_supported;
union {
struct intel_version intel_version; /* legacy version */
+ struct intel_version_tlv intel_version_tlv;
};
} __packed;

--
2.17.1

2020-07-02 12:05:44

by K, Kiran

[permalink] [raw]
Subject: [PATCH 3/5] Bluetooth: btintel: Refactor firmware payload download code

Move firmware payload download code to a separate function to
enhance readability and reusability

Signed-off-by: Kiran K <[email protected]>
Signed-off-by: Amit K Bag <[email protected]>
Signed-off-by: Raghuram Hegde <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Sathish Narasimman <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
drivers/bluetooth/btintel.c | 29 ++++++++++++++++++++---------
drivers/bluetooth/btintel.h | 10 +++++-----
drivers/bluetooth/btusb.c | 2 +-
drivers/bluetooth/hci_intel.c | 2 +-
4 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 1c820c187421..d0c6576212d7 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -19,6 +19,7 @@
#define VERSION "0.1"

#define BDADDR_INTEL (&(bdaddr_t) {{0x00, 0x8b, 0x9e, 0x19, 0x03, 0x00}})
+#define RSA_HEADER_LEN 644

int btintel_check_bdaddr(struct hci_dev *hdev)
{
@@ -681,20 +682,17 @@ static int btintel_sfi_rsa_header_secure_send(struct hci_dev *hdev,
return err;
}

-int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
- u32 *boot_param)
+static int btintel_download_firmware_payload(struct hci_dev *hdev,
+ const struct firmware *fw,
+ u32 *boot_param, size_t offset)
{
int err;
const u8 *fw_ptr;
u32 frag_len;

- err = btintel_sfi_rsa_header_secure_send(hdev, fw);
- if (err)
- goto done;
-
-
- fw_ptr = fw->data + 644;
+ fw_ptr = fw->data + offset;
frag_len = 0;
+ err = -EINVAL;

while (fw_ptr - fw->data < fw->size) {
struct hci_command_hdr *cmd = (void *)(fw_ptr + frag_len);
@@ -740,7 +738,20 @@ int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
done:
return err;
}
-EXPORT_SYMBOL_GPL(btintel_download_firmware);
+
+int btintel_download_firmware_legacy(struct hci_dev *hdev,
+ const struct firmware *fw,
+ u32 *boot_param)
+{
+ int err;
+
+ err = btintel_sfi_rsa_header_secure_send(hdev, fw);
+ if (err)
+ return err;
+
+ return btintel_download_firmware_payload(hdev, fw, boot_param, RSA_HEADER_LEN);
+}
+EXPORT_SYMBOL_GPL(btintel_download_firmware_legacy);

void btintel_reset_to_bootloader(struct hci_dev *hdev)
{
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index 0865d2d4aca7..e476105d495b 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -96,8 +96,8 @@ struct regmap *btintel_regmap_init(struct hci_dev *hdev, u16 opcode_read,
int btintel_send_intel_reset(struct hci_dev *hdev, u32 boot_param);
int btintel_read_boot_params(struct hci_dev *hdev,
struct intel_boot_params *params);
-int btintel_download_firmware(struct hci_dev *dev, const struct firmware *fw,
- u32 *boot_param);
+int btintel_download_firmware_legacy(struct hci_dev *dev, const struct firmware *fw,
+ u32 *boot_param);
void btintel_reset_to_bootloader(struct hci_dev *hdev);
int btintel_read_debug_features(struct hci_dev *hdev,
struct intel_debug_features *features);
@@ -191,9 +191,9 @@ static inline int btintel_read_boot_params(struct hci_dev *hdev,
return -EOPNOTSUPP;
}

-static inline int btintel_download_firmware(struct hci_dev *dev,
- const struct firmware *fw,
- u32 *boot_param)
+static inline int btintel_download_firmware_legacy(struct hci_dev *dev,
+ const struct firmware *fw,
+ u32 *boot_param)
{
return -EOPNOTSUPP;
}
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index d06c946f7810..364da6d44ee3 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2438,7 +2438,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
set_bit(BTUSB_DOWNLOADING, &data->flags);

/* Start firmware downloading and get boot parameter */
- err = btintel_download_firmware(hdev, fw, &boot_param);
+ err = btintel_download_firmware_legacy(hdev, fw, &boot_param);
if (err < 0) {
/* When FW download fails, send Intel Reset to retry
* FW download.
diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index f30cbc66d48f..14045a464309 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -755,7 +755,7 @@ static int intel_setup(struct hci_uart *hu)
set_bit(STATE_DOWNLOADING, &intel->flags);

/* Start firmware downloading and get boot parameter */
- err = btintel_download_firmware(hdev, fw, &boot_param);
+ err = btintel_download_firmware_legacy(hdev, fw, &boot_param);
if (err < 0)
goto done;

--
2.17.1

2020-07-02 12:05:44

by K, Kiran

[permalink] [raw]
Subject: [PATCH 1/5] Bluetooth: btintel: Make controller version read generic

Make controller read vesion function more generic to support different
type of controllers.

Signed-off-by: Kiran K <[email protected]>
Signed-off-by: Amit K Bag <[email protected]>
Signed-off-by: Raghuram Hegde <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Sathish Narasimman <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
drivers/bluetooth/btintel.c | 36 ++++++++++++++----
drivers/bluetooth/btintel.h | 15 ++++++--
drivers/bluetooth/btusb.c | 71 +++++++++++++++++++++++------------
drivers/bluetooth/hci_ag6xx.c | 12 +++++-
drivers/bluetooth/hci_intel.c | 12 +++++-
5 files changed, 106 insertions(+), 40 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 5fa5be3c5598..dea96c585ecb 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -204,9 +204,15 @@ void btintel_hw_error(struct hci_dev *hdev, u8 code)
}
EXPORT_SYMBOL_GPL(btintel_hw_error);

-void btintel_version_info(struct hci_dev *hdev, struct intel_version *ver)
+void btintel_version_info(struct hci_dev *hdev, const struct btintel_version *version)
{
const char *variant;
+ const struct intel_version *ver;
+
+ if (version->is_tlv_supported)
+ return;
+
+ ver = &version->intel_version;

switch (ver->fw_variant) {
case 0x06:
@@ -335,27 +341,41 @@ int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug)
}
EXPORT_SYMBOL_GPL(btintel_set_event_mask_mfg);

-int btintel_read_version(struct hci_dev *hdev, struct intel_version *ver)
+int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
{
struct sk_buff *skb;
+ u8 *data, param, status, check_tlv;
+
+ if (!version)
+ return -EINVAL;

- skb = __hci_cmd_sync(hdev, 0xfc05, 0, NULL, HCI_CMD_TIMEOUT);
+ param = 0xFF;
+
+ skb = __hci_cmd_sync(hdev, 0xfc05, 1, &param, HCI_CMD_TIMEOUT);
if (IS_ERR(skb)) {
bt_dev_err(hdev, "Reading Intel version information failed (%ld)",
PTR_ERR(skb));
return PTR_ERR(skb);
}

- if (skb->len != sizeof(*ver)) {
- bt_dev_err(hdev, "Intel version event size mismatch");
+ data = skb->data;
+ status = *data;
+ if (status) {
+ bt_dev_err(hdev, "Intel Read Version command failed (%02x)",
+ status);
kfree_skb(skb);
- return -EILSEQ;
+ return -bt_to_errno(status);
}

- memcpy(ver, skb->data, sizeof(*ver));
+ check_tlv = *(data + 1);

+ if (skb->len == sizeof(version->intel_version) && check_tlv == 0x37) {
+ memcpy(&version->intel_version, skb->data, sizeof(version->intel_version));
+ version->is_tlv_supported = false;
+ } else {
+ version->is_tlv_supported = true;
+ }
kfree_skb(skb);
-
return 0;
}
EXPORT_SYMBOL_GPL(btintel_read_version);
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index 08e20606fb58..0865d2d4aca7 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -66,6 +66,13 @@ struct intel_debug_features {
__u8 page1[16];
} __packed;

+struct btintel_version {
+ bool is_tlv_supported;
+ union {
+ struct intel_version intel_version; /* legacy version */
+ };
+} __packed;
+
#if IS_ENABLED(CONFIG_BT_INTEL)

int btintel_check_bdaddr(struct hci_dev *hdev);
@@ -76,13 +83,13 @@ int btintel_set_diag(struct hci_dev *hdev, bool enable);
int btintel_set_diag_mfg(struct hci_dev *hdev, bool enable);
void btintel_hw_error(struct hci_dev *hdev, u8 code);

-void btintel_version_info(struct hci_dev *hdev, struct intel_version *ver);
+void btintel_version_info(struct hci_dev *hdev, const struct btintel_version *version);
int btintel_secure_send(struct hci_dev *hdev, u8 fragment_type, u32 plen,
const void *param);
int btintel_load_ddc_config(struct hci_dev *hdev, const char *ddc_name);
int btintel_set_event_mask(struct hci_dev *hdev, bool debug);
int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug);
-int btintel_read_version(struct hci_dev *hdev, struct intel_version *ver);
+int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version);

struct regmap *btintel_regmap_init(struct hci_dev *hdev, u16 opcode_read,
u16 opcode_write);
@@ -133,7 +140,7 @@ static inline void btintel_hw_error(struct hci_dev *hdev, u8 code)
}

static inline void btintel_version_info(struct hci_dev *hdev,
- struct intel_version *ver)
+ struct btintel_version *version)
{
}

@@ -160,7 +167,7 @@ static inline int btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug)
}

static inline int btintel_read_version(struct hci_dev *hdev,
- struct intel_version *ver)
+ struct btintel_version *version)
{
return -EOPNOTSUPP;
}
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index faa863dd5d0a..d06c946f7810 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1938,6 +1938,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
const u8 *fw_ptr;
int disable_patch, err;
struct intel_version ver;
+ struct btintel_version version;

BT_DBG("%s", hdev->name);

@@ -1963,10 +1964,16 @@ static int btusb_setup_intel(struct hci_dev *hdev)
* The returned information are hardware variant and revision plus
* firmware variant, revision and build number.
*/
- err = btintel_read_version(hdev, &ver);
+ err = btintel_read_version(hdev, &version);
if (err)
return err;

+ if (version.is_tlv_supported) {
+ bt_dev_err(hdev, "FW download in tlv format not supported");
+ return -EOPNOTSUPP;
+ }
+ ver = version.intel_version;
+
bt_dev_info(hdev, "read Intel version: %02x%02x%02x%02x%02x%02x%02x%02x%02x",
ver.hw_platform, ver.hw_variant, ver.hw_revision,
ver.fw_variant, ver.fw_revision, ver.fw_build_num,
@@ -2049,11 +2056,11 @@ static int btusb_setup_intel(struct hci_dev *hdev)
/* Need build number for downloaded fw patches in
* every power-on boot
*/
- err = btintel_read_version(hdev, &ver);
- if (err)
- return err;
- bt_dev_info(hdev, "Intel BT fw patch 0x%02x completed & activated",
- ver.fw_patch_num);
+ err = btintel_read_version(hdev, &version);
+ if (err)
+ return err;
+ bt_dev_info(hdev, "Intel BT fw patch 0x%02x completed & activated",
+ version.intel_version.fw_patch_num);

goto complete;

@@ -2251,11 +2258,18 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
return -EILSEQ;
}

-static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
- struct intel_boot_params *params,
- char *fw_name, size_t len,
- const char *suffix)
+static bool btusb_setup_intel_new_get_fw_name(const struct btintel_version *version,
+ struct intel_boot_params *params,
+ char *fw_name, size_t len,
+ const char *suffix)
{
+ const struct intel_version *ver;
+
+ if (version->is_tlv_supported)
+ return false;
+
+ ver = &version->intel_version;
+
switch (ver->hw_variant) {
case 0x0b: /* SfP */
case 0x0c: /* WsP */
@@ -2281,18 +2295,21 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
}

static int btusb_intel_download_firmware(struct hci_dev *hdev,
- struct intel_version *ver,
+ struct btintel_version *version,
struct intel_boot_params *params)
{
const struct firmware *fw;
u32 boot_param;
char fwname[64];
int err;
+ const struct intel_version *ver;
struct btusb_data *data = hci_get_drvdata(hdev);

- if (!ver || !params)
+ if (!version || !params)
return -EINVAL;

+ ver = &version->intel_version;
+
/* The hardware platform number has a fixed value of 0x37 and
* for now only accept this single value.
*/
@@ -2322,8 +2339,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
return -EINVAL;
}

- btintel_version_info(hdev, ver);
-
/* The firmware variant determines if the device is in bootloader
* mode or is running operational firmware. The value 0x06 identifies
* the bootloader and the value 0x23 identifies the operational
@@ -2398,7 +2413,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
* ibt-<hw_variant>-<hw_revision>-<fw_revision>.sfi.
*
*/
- err = btusb_setup_intel_new_get_fw_name(ver, params, fwname,
+ err = btusb_setup_intel_new_get_fw_name(version, params, fwname,
sizeof(fwname), "sfi");
if (!err) {
bt_dev_err(hdev, "Unsupported Intel firmware naming");
@@ -2483,6 +2498,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
unsigned long long duration;
int err;
struct intel_debug_features features;
+ struct btintel_version version;

BT_DBG("%s", hdev->name);

@@ -2494,21 +2510,28 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)

calltime = ktime_get();

- /* Read the Intel version information to determine if the device
- * is in bootloader mode or if it already has operational firmware
- * loaded.
+ /* Read controller version information and support of tlv format
*/
- err = btintel_read_version(hdev, &ver);
+ err = btintel_read_version(hdev, &version);
if (err) {
- bt_dev_err(hdev, "Intel Read version failed (%d)", err);
+ bt_dev_err(hdev, "Intel Read version new failed (%d)", err);
btintel_reset_to_bootloader(hdev);
return err;
}

- err = btusb_intel_download_firmware(hdev, &ver, &params);
+ if (version.is_tlv_supported) {
+ bt_dev_err(hdev, "Firmware download in tlv format is not supported");
+ return -EOPNOTSUPP;
+ }
+
+ btintel_version_info(hdev, &version);
+
+ err = btusb_intel_download_firmware(hdev, &version, &params);
if (err)
return err;

+ ver = version.intel_version;
+
/* controller is already having an operational firmware */
if (ver.fw_variant == 0x23)
goto finish;
@@ -2562,7 +2585,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)

clear_bit(BTUSB_BOOTLOADER, &data->flags);

- err = btusb_setup_intel_new_get_fw_name(&ver, &params, ddcname,
+ err = btusb_setup_intel_new_get_fw_name(&version, &params, ddcname,
sizeof(ddcname), "ddc");

if (!err) {
@@ -2586,11 +2609,11 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
btintel_set_debug_features(hdev, &features);

/* Read the Intel version information after loading the FW */
- err = btintel_read_version(hdev, &ver);
+ err = btintel_read_version(hdev, &version);
if (err)
return err;

- btintel_version_info(hdev, &ver);
+ btintel_version_info(hdev, &version);

finish:
/* All Intel controllers that support the Microsoft vendor
diff --git a/drivers/bluetooth/hci_ag6xx.c b/drivers/bluetooth/hci_ag6xx.c
index 1f55df93e4ce..6f6a1e061972 100644
--- a/drivers/bluetooth/hci_ag6xx.c
+++ b/drivers/bluetooth/hci_ag6xx.c
@@ -153,6 +153,7 @@ static int ag6xx_setup(struct hci_uart *hu)
struct hci_dev *hdev = hu->hdev;
struct sk_buff *skb;
struct intel_version ver;
+ struct btintel_version version;
const struct firmware *fw;
const u8 *fw_ptr;
char fwname[64];
@@ -166,11 +167,18 @@ static int ag6xx_setup(struct hci_uart *hu)
if (err)
return err;

- err = btintel_read_version(hdev, &ver);
+ err = btintel_read_version(hdev, &version);
if (err)
return err;

- btintel_version_info(hdev, &ver);
+ if (version.is_tlv_supported) {
+ bt_dev_err(hdev, "Firmware download in tlv format over ag6xx is not supported");
+ return -EOPNOTSUPP;
+ }
+
+ btintel_version_info(hdev, &version);
+
+ ver = version.intel_version;

/* The hardware platform number has a fixed value of 0x37 and
* for now only accept this single value.
diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index f1299da6eed8..f30cbc66d48f 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -532,6 +532,7 @@ static int intel_setup(struct hci_uart *hu)
struct hci_dev *hdev = hu->hdev;
struct sk_buff *skb;
struct intel_version ver;
+ struct btintel_version version;
struct intel_boot_params params;
struct list_head *p;
const struct firmware *fw;
@@ -584,10 +585,17 @@ static int intel_setup(struct hci_uart *hu)
* is in bootloader mode or if it already has operational firmware
* loaded.
*/
- err = btintel_read_version(hdev, &ver);
+ err = btintel_read_version(hdev, &version);
if (err)
return err;

+ if (version.is_tlv_supported) {
+ /* firmware download in tlv format is not supported on UART transport */
+ bt_dev_err(hdev, "Firmware download in tlv format is not supported");
+ return -EOPNOTSUPP;
+ }
+ ver = version.intel_version;
+
/* The hardware platform number has a fixed value of 0x37 and
* for now only accept this single value.
*/
@@ -614,7 +622,7 @@ static int intel_setup(struct hci_uart *hu)
return -EINVAL;
}

- btintel_version_info(hdev, &ver);
+ btintel_version_info(hdev, &version);

/* The firmware variant determines if the device is in bootloader
* mode or is running operational firmware. The value 0x06 identifies
--
2.17.1

2020-07-02 12:05:44

by K, Kiran

[permalink] [raw]
Subject: [PATCH 5/5] Bluetooth: btintel: Parse controller information present in TLV format

New generation Intel controllers returns controller information
in TLV format. Adding capability to parse and log it for debug purpose

Signed-off-by: Kiran K <[email protected]>
Signed-off-by: Amit K Bag <[email protected]>
Signed-off-by: Raghuram Hegde <[email protected]>
Reviewed-by: Chethan T N <[email protected]>
Reviewed-by: Sathish Narasimman <[email protected]>
Reviewed-by: Srivatsa Ravishankar <[email protected]>
---
drivers/bluetooth/btintel.c | 161 ++++++++++++++++++++++++++++++++----
drivers/bluetooth/btusb.c | 4 +-
2 files changed, 148 insertions(+), 17 deletions(-)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index d0c6576212d7..f0b087d97a83 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -209,27 +209,59 @@ void btintel_version_info(struct hci_dev *hdev, const struct btintel_version *ve
{
const char *variant;
const struct intel_version *ver;
+ const struct intel_version_tlv *ver_tlv;

- if (version->is_tlv_supported)
- return;
+ if (!version->is_tlv_supported) {
+ ver = &version->intel_version;
+
+ switch (ver->fw_variant) {
+ case 0x06:
+ variant = "Bootloader";
+ break;
+ case 0x23:
+ variant = "Firmware";
+ break;
+ default:
+ goto done;
+ }

- ver = &version->intel_version;
+ bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
+ variant, ver->fw_revision >> 4,
+ ver->fw_revision & 0x0f, ver->fw_build_num,
+ ver->fw_build_ww, 2000 + ver->fw_build_yy);
+ goto done;
+ }
+
+ ver_tlv = &version->intel_version_tlv;

- switch (ver->fw_variant) {
- case 0x06:
+ switch (ver_tlv->img_type) {
+ case 0x01:
variant = "Bootloader";
- break;
- case 0x23:
+ bt_dev_info(hdev, "Device revision is %u", ver_tlv->dev_rev_id);
+ bt_dev_info(hdev, "Secure boot is %s",
+ ver_tlv->secure_boot ? "enabled" : "disabled");
+ bt_dev_info(hdev, "OTP lock is %s",
+ ver_tlv->otp_lock ? "enabled" : "disabled");
+ bt_dev_info(hdev, "API lock is %s",
+ ver_tlv->api_lock ? "enabled" : "disabled");
+ bt_dev_info(hdev, "Debug lock is %s",
+ ver_tlv->debug_lock ? "enabled" : "disabled");
+ bt_dev_info(hdev, "Minimum firmware build %u week %u %u",
+ ver_tlv->min_fw_build_nn, ver_tlv->min_fw_build_cw,
+ 2000 + ver_tlv->min_fw_build_yy);
+ break;
+ case 0x03:
variant = "Firmware";
- break;
+ break;
default:
- return;
+ goto done;
}

- bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
- variant, ver->fw_revision >> 4, ver->fw_revision & 0x0f,
- ver->fw_build_num, ver->fw_build_ww,
- 2000 + ver->fw_build_yy);
+ bt_dev_info(hdev, "%s timestamp %u.%u buildtype %u build %u", variant,
+ 2000 + (ver_tlv->timestamp >> 8), ver_tlv->timestamp & 0xff,
+ ver_tlv->build_type, ver_tlv->build_num);
+done:
+ return;
}
EXPORT_SYMBOL_GPL(btintel_version_info);

@@ -346,6 +378,8 @@ int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
{
struct sk_buff *skb;
u8 *data, param, status, check_tlv;
+ struct intel_version_tlv *ver_tlv;
+ struct intel_tlv *tlv;

if (!version)
return -EINVAL;
@@ -373,9 +407,106 @@ int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
if (skb->len == sizeof(version->intel_version) && check_tlv == 0x37) {
memcpy(&version->intel_version, skb->data, sizeof(version->intel_version));
version->is_tlv_supported = false;
- } else {
- version->is_tlv_supported = true;
+ goto done;
}
+
+ bt_dev_info(hdev, "Supports tlv firmware download sequence");
+ version->is_tlv_supported = true;
+ ver_tlv = &version->intel_version_tlv;
+
+ /* Consume Command Complete Status field */
+ skb_pull(skb, 1);
+
+ /* Event parameters contatin multiple TLVs. Read each of them
+ * and only keep the required data. Also, it use existing legacy
+ * version field like hw_platform, hw_variant, and fw_variant
+ * to keep the existing setup flow
+ */
+ while (skb->len) {
+ tlv = (struct intel_tlv *)skb->data;
+ switch (tlv->type) {
+ case INTEL_TLV_CNVI_TOP:
+ ver_tlv->cnvi_top = (tlv->val[3] << 24) |
+ (tlv->val[2] << 16) |
+ (tlv->val[1] << 8) |
+ (tlv->val[0]);
+ break;
+ case INTEL_TLV_CNVR_TOP:
+ ver_tlv->cnvr_top = (tlv->val[3] << 24) |
+ (tlv->val[2] << 16) |
+ (tlv->val[1] << 8) |
+ (tlv->val[0]);
+ break;
+ case INTEL_TLV_CNVI_BT:
+ ver_tlv->cnvi_bt = (tlv->val[3] << 24) |
+ (tlv->val[2] << 16) |
+ (tlv->val[1] << 8) |
+ (tlv->val[0]);
+ break;
+ case INTEL_TLV_CNVR_BT:
+ ver_tlv->cnvr_bt = (tlv->val[3] << 24) |
+ (tlv->val[2] << 16) |
+ (tlv->val[1] << 8) |
+ (tlv->val[0]);
+ break;
+ case INTEL_TLV_USB_VENDOR_ID:
+ ver_tlv->usb_vid = (tlv->val[1] << 8) | (tlv->val[0]);
+ break;
+ case INTEL_TLV_USB_PRODUCT_ID:
+ ver_tlv->usb_pid = (tlv->val[1] << 8) | (tlv->val[0]);
+ break;
+ case INTEL_TLV_IMAGE_TYPE:
+ ver_tlv->img_type = tlv->val[0];
+ break;
+ case INTEL_TLV_TIME_STAMP:
+ ver_tlv->timestamp = (tlv->val[1] << 8) | (tlv->val[0]);
+ break;
+ case INTEL_TLV_BUILD_TYPE:
+ ver_tlv->build_type = tlv->val[0];
+ break;
+ case INTEL_TLV_BUILD_NUM:
+ ver_tlv->build_num = (tlv->val[3] << 24) |
+ (tlv->val[2] << 16) |
+ (tlv->val[1] << 8) |
+ (tlv->val[0]);
+ break;
+ case INTEL_TLV_SECURE_BOOT:
+ ver_tlv->secure_boot = tlv->val[0];
+ break;
+ case INTEL_TLV_KEY_FROM_HDR:
+ ver_tlv->key_from_hdr = tlv->val[0];
+ break;
+ case INTEL_TLV_OTP_LOCK:
+ ver_tlv->otp_lock = tlv->val[0];
+ break;
+ case INTEL_TLV_API_LOCK:
+ ver_tlv->api_lock = tlv->val[0];
+ break;
+ case INTEL_TLV_DEBUG_LOCK:
+ ver_tlv->debug_lock = tlv->val[0];
+ break;
+ case INTEL_TLV_MIN_FW:
+ ver_tlv->min_fw_build_nn = tlv->val[0];
+ ver_tlv->min_fw_build_cw = tlv->val[1];
+ ver_tlv->min_fw_build_yy = tlv->val[2];
+ break;
+ case INTEL_TLV_LIMITED_CCE:
+ ver_tlv->limited_cce = tlv->val[0];
+ break;
+ case INTEL_TLV_SBE_TYPE:
+ ver_tlv->sbe_type = tlv->val[0];
+ break;
+ case INTEL_TLV_OTP_BDADDR:
+ memcpy(&ver_tlv->otp_bd_addr, tlv->val, tlv->len);
+ break;
+ default:
+ /* Ignore rest of information */
+ break;
+ }
+ /* consume the current tlv and move to next*/
+ skb_pull(skb, tlv->len + sizeof(*tlv));
+ }
+done:
kfree_skb(skb);
return 0;
}
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 364da6d44ee3..f30b43e15a26 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2519,13 +2519,13 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
return err;
}

+ btintel_version_info(hdev, &version);
+
if (version.is_tlv_supported) {
bt_dev_err(hdev, "Firmware download in tlv format is not supported");
return -EOPNOTSUPP;
}

- btintel_version_info(hdev, &version);
-
err = btusb_intel_download_firmware(hdev, &version, &params);
if (err)
return err;
--
2.17.1

2020-07-02 13:26:56

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH 2/5] Bluetooth: btintel: Refactor firmware header download sequence

Hi Kiran,

> Move firmware header download code to a separate function to
> enhance readability and reusability
>
> Signed-off-by: Kiran K <[email protected]>
> Signed-off-by: Amit K Bag <[email protected]>
> Signed-off-by: Raghuram Hegde <[email protected]>
> Reviewed-by: Chethan T N <[email protected]>
> Reviewed-by: Sathish Narasimman <[email protected]>
> Reviewed-by: Srivatsa Ravishankar <[email protected]>
> ---
> drivers/bluetooth/btintel.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index dea96c585ecb..1c820c187421 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -646,12 +646,10 @@ int btintel_read_boot_params(struct hci_dev *hdev,
> }
> EXPORT_SYMBOL_GPL(btintel_read_boot_params);
>
> -int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
> - u32 *boot_param)
> +static int btintel_sfi_rsa_header_secure_send(struct hci_dev *hdev,
> + const struct firmware *fw)
> {
> int err;
> - const u8 *fw_ptr;
> - u32 frag_len;
>
> /* Start the firmware download transaction with the Init fragment
> * represented by the 128 bytes of CSS header.
> @@ -679,6 +677,21 @@ int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
> bt_dev_err(hdev, "Failed to send firmware signature (%d)", err);
> goto done;
> }

Here should be an extra empty line before the label.

> +done:
> + return err;
> +}
> +
> +int btintel_download_firmware(struct hci_dev *hdev, const struct firmware *fw,
> + u32 *boot_param)
> +{
> + int err;
> + const u8 *fw_ptr;
> + u32 frag_len;
> +
> + err = btintel_sfi_rsa_header_secure_send(hdev, fw);
> + if (err)
> + goto done;
> +

Scrap the extra empty line here.

>
> fw_ptr = fw->data + 644;
> frag_len = 0;

Regards

Marcel

2020-07-02 13:28:22

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH 3/5] Bluetooth: btintel: Refactor firmware payload download code

Hi Kiran,

> Move firmware payload download code to a separate function to
> enhance readability and reusability

this patch is doing more than that. It also introduces an extra public function.

Regards

Marcel

2020-07-02 13:37:06

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH 5/5] Bluetooth: btintel: Parse controller information present in TLV format

Hi Kiran,

> New generation Intel controllers returns controller information
> in TLV format. Adding capability to parse and log it for debug purpose
>
> Signed-off-by: Kiran K <[email protected]>
> Signed-off-by: Amit K Bag <[email protected]>
> Signed-off-by: Raghuram Hegde <[email protected]>
> Reviewed-by: Chethan T N <[email protected]>
> Reviewed-by: Sathish Narasimman <[email protected]>
> Reviewed-by: Srivatsa Ravishankar <[email protected]>
> ---
> drivers/bluetooth/btintel.c | 161 ++++++++++++++++++++++++++++++++----
> drivers/bluetooth/btusb.c | 4 +-
> 2 files changed, 148 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index d0c6576212d7..f0b087d97a83 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -209,27 +209,59 @@ void btintel_version_info(struct hci_dev *hdev, const struct btintel_version *ve
> {
> const char *variant;
> const struct intel_version *ver;
> + const struct intel_version_tlv *ver_tlv;
>
> - if (version->is_tlv_supported)
> - return;
> + if (!version->is_tlv_supported) {
> + ver = &version->intel_version;
> +
> + switch (ver->fw_variant) {
> + case 0x06:
> + variant = "Bootloader";
> + break;

The break; has to be indented with the variant =.

> + case 0x23:
> + variant = "Firmware";
> + break;
> + default:
> + goto done;
> + }
>
> - ver = &version->intel_version;
> + bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
> + variant, ver->fw_revision >> 4,
> + ver->fw_revision & 0x0f, ver->fw_build_num,
> + ver->fw_build_ww, 2000 + ver->fw_build_yy);
> + goto done;
> + }
> +
> + ver_tlv = &version->intel_version_tlv;
>
> - switch (ver->fw_variant) {
> - case 0x06:
> + switch (ver_tlv->img_type) {
> + case 0x01:
> variant = "Bootloader";
> - break;
> - case 0x23:
> + bt_dev_info(hdev, "Device revision is %u", ver_tlv->dev_rev_id);
> + bt_dev_info(hdev, "Secure boot is %s",
> + ver_tlv->secure_boot ? "enabled" : "disabled");
> + bt_dev_info(hdev, "OTP lock is %s",
> + ver_tlv->otp_lock ? "enabled" : "disabled");
> + bt_dev_info(hdev, "API lock is %s",
> + ver_tlv->api_lock ? "enabled" : "disabled");
> + bt_dev_info(hdev, "Debug lock is %s",
> + ver_tlv->debug_lock ? "enabled" : "disabled");
> + bt_dev_info(hdev, "Minimum firmware build %u week %u %u",
> + ver_tlv->min_fw_build_nn, ver_tlv->min_fw_build_cw,
> + 2000 + ver_tlv->min_fw_build_yy);
> + break;
> + case 0x03:
> variant = "Firmware";
> - break;
> + break;
> default:
> - return;
> + goto done;
> }
>
> - bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
> - variant, ver->fw_revision >> 4, ver->fw_revision & 0x0f,
> - ver->fw_build_num, ver->fw_build_ww,
> - 2000 + ver->fw_build_yy);
> + bt_dev_info(hdev, "%s timestamp %u.%u buildtype %u build %u", variant,
> + 2000 + (ver_tlv->timestamp >> 8), ver_tlv->timestamp & 0xff,
> + ver_tlv->build_type, ver_tlv->build_num);
> +done:
> + return;
> }
> EXPORT_SYMBOL_GPL(btintel_version_info);
>
> @@ -346,6 +378,8 @@ int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
> {
> struct sk_buff *skb;
> u8 *data, param, status, check_tlv;
> + struct intel_version_tlv *ver_tlv;
> + struct intel_tlv *tlv;
>
> if (!version)
> return -EINVAL;
> @@ -373,9 +407,106 @@ int btintel_read_version(struct hci_dev *hdev, struct btintel_version *version)
> if (skb->len == sizeof(version->intel_version) && check_tlv == 0x37) {
> memcpy(&version->intel_version, skb->data, sizeof(version->intel_version));
> version->is_tlv_supported = false;
> - } else {
> - version->is_tlv_supported = true;
> + goto done;
> }
> +
> + bt_dev_info(hdev, "Supports tlv firmware download sequence");
> + version->is_tlv_supported = true;
> + ver_tlv = &version->intel_version_tlv;
> +
> + /* Consume Command Complete Status field */
> + skb_pull(skb, 1);
> +
> + /* Event parameters contatin multiple TLVs. Read each of them
> + * and only keep the required data. Also, it use existing legacy
> + * version field like hw_platform, hw_variant, and fw_variant
> + * to keep the existing setup flow
> + */
> + while (skb->len) {
> + tlv = (struct intel_tlv *)skb->data;
> + switch (tlv->type) {
> + case INTEL_TLV_CNVI_TOP:
> + ver_tlv->cnvi_top = (tlv->val[3] << 24) |
> + (tlv->val[2] << 16) |
> + (tlv->val[1] << 8) |
> + (tlv->val[0]);

We have get_unaligned functions for that.

> + break;
> + case INTEL_TLV_CNVR_TOP:
> + ver_tlv->cnvr_top = (tlv->val[3] << 24) |
> + (tlv->val[2] << 16) |
> + (tlv->val[1] << 8) |
> + (tlv->val[0]);
> + break;
> + case INTEL_TLV_CNVI_BT:
> + ver_tlv->cnvi_bt = (tlv->val[3] << 24) |
> + (tlv->val[2] << 16) |
> + (tlv->val[1] << 8) |
> + (tlv->val[0]);
> + break;
> + case INTEL_TLV_CNVR_BT:
> + ver_tlv->cnvr_bt = (tlv->val[3] << 24) |
> + (tlv->val[2] << 16) |
> + (tlv->val[1] << 8) |
> + (tlv->val[0]);
> + break;
> + case INTEL_TLV_USB_VENDOR_ID:
> + ver_tlv->usb_vid = (tlv->val[1] << 8) | (tlv->val[0]);
> + break;
> + case INTEL_TLV_USB_PRODUCT_ID:
> + ver_tlv->usb_pid = (tlv->val[1] << 8) | (tlv->val[0]);
> + break;
> + case INTEL_TLV_IMAGE_TYPE:
> + ver_tlv->img_type = tlv->val[0];
> + break;
> + case INTEL_TLV_TIME_STAMP:
> + ver_tlv->timestamp = (tlv->val[1] << 8) | (tlv->val[0]);
> + break;
> + case INTEL_TLV_BUILD_TYPE:
> + ver_tlv->build_type = tlv->val[0];
> + break;
> + case INTEL_TLV_BUILD_NUM:
> + ver_tlv->build_num = (tlv->val[3] << 24) |
> + (tlv->val[2] << 16) |
> + (tlv->val[1] << 8) |
> + (tlv->val[0]);
> + break;
> + case INTEL_TLV_SECURE_BOOT:
> + ver_tlv->secure_boot = tlv->val[0];
> + break;
> + case INTEL_TLV_KEY_FROM_HDR:
> + ver_tlv->key_from_hdr = tlv->val[0];
> + break;
> + case INTEL_TLV_OTP_LOCK:
> + ver_tlv->otp_lock = tlv->val[0];
> + break;
> + case INTEL_TLV_API_LOCK:
> + ver_tlv->api_lock = tlv->val[0];
> + break;
> + case INTEL_TLV_DEBUG_LOCK:
> + ver_tlv->debug_lock = tlv->val[0];
> + break;
> + case INTEL_TLV_MIN_FW:
> + ver_tlv->min_fw_build_nn = tlv->val[0];
> + ver_tlv->min_fw_build_cw = tlv->val[1];
> + ver_tlv->min_fw_build_yy = tlv->val[2];
> + break;
> + case INTEL_TLV_LIMITED_CCE:
> + ver_tlv->limited_cce = tlv->val[0];
> + break;
> + case INTEL_TLV_SBE_TYPE:
> + ver_tlv->sbe_type = tlv->val[0];
> + break;
> + case INTEL_TLV_OTP_BDADDR:
> + memcpy(&ver_tlv->otp_bd_addr, tlv->val, tlv->len);
> + break;
> + default:
> + /* Ignore rest of information */
> + break;
> + }
> + /* consume the current tlv and move to next*/
> + skb_pull(skb, tlv->len + sizeof(*tlv));
> + }
> +done:
> kfree_skb(skb);
> return 0;
> }
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 364da6d44ee3..f30b43e15a26 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2519,13 +2519,13 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> return err;
> }
>
> + btintel_version_info(hdev, &version);
> +
> if (version.is_tlv_supported) {
> bt_dev_err(hdev, "Firmware download in tlv format is not supported");
> return -EOPNOTSUPP;
> }
>
> - btintel_version_info(hdev, &version);
> -
> err = btusb_intel_download_firmware(hdev, &version, &params);
> if (err)
> return err;

Regards

Marcel

2020-07-02 17:15:59

by K, Kiran

[permalink] [raw]
Subject: RE: [PATCH 2/5] Bluetooth: btintel: Refactor firmware header download sequence

Hi Marcel,

> -----Original Message-----
> From: [email protected] <linux-bluetooth-
> [email protected]> On Behalf Of Marcel Holtmann
> Sent: Thursday, July 2, 2020 6:56 PM
> To: K, Kiran <[email protected]>
> Cc: linux-bluetooth <[email protected]>; Srivatsa, Ravishankar
> <[email protected]>; Tumkur Narayan, Chethan
> <[email protected]>; [email protected]; Bag, Amit K
> <[email protected]>; Raghuram Hegde <[email protected]>
> Subject: Re: [PATCH 2/5] Bluetooth: btintel: Refactor firmware header
> download sequence
>
> Hi Kiran,
>
> > Move firmware header download code to a separate function to enhance
> > readability and reusability
> >
> > Signed-off-by: Kiran K <[email protected]>
> > Signed-off-by: Amit K Bag <[email protected]>
> > Signed-off-by: Raghuram Hegde <[email protected]>
> > Reviewed-by: Chethan T N <[email protected]>
> > Reviewed-by: Sathish Narasimman <[email protected]>
> > Reviewed-by: Srivatsa Ravishankar <[email protected]>
> > ---
> > drivers/bluetooth/btintel.c | 21 +++++++++++++++++----
> > 1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index dea96c585ecb..1c820c187421 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -646,12 +646,10 @@ int btintel_read_boot_params(struct hci_dev
> > *hdev, } EXPORT_SYMBOL_GPL(btintel_read_boot_params);
> >
> > -int btintel_download_firmware(struct hci_dev *hdev, const struct firmware
> *fw,
> > - u32 *boot_param)
> > +static int btintel_sfi_rsa_header_secure_send(struct hci_dev *hdev,
> > + const struct firmware *fw)
> > {
> > int err;
> > - const u8 *fw_ptr;
> > - u32 frag_len;
> >
> > /* Start the firmware download transaction with the Init fragment
> > * represented by the 128 bytes of CSS header.
> > @@ -679,6 +677,21 @@ int btintel_download_firmware(struct hci_dev
> *hdev, const struct firmware *fw,
> > bt_dev_err(hdev, "Failed to send firmware signature (%d)",
> err);
> > goto done;
> > }
>
> Here should be an extra empty line before the label.

Ack. I will fix it in v2.
>
> > +done:
> > + return err;
> > +}
> > +
> > +int btintel_download_firmware(struct hci_dev *hdev, const struct firmware
> *fw,
> > + u32 *boot_param)
> > +{
> > + int err;
> > + const u8 *fw_ptr;
> > + u32 frag_len;
> > +
> > + err = btintel_sfi_rsa_header_secure_send(hdev, fw);
> > + if (err)
> > + goto done;
> > +
>
> Scrap the extra empty line here.

Ack.
>
> >
> > fw_ptr = fw->data + 644;
> > frag_len = 0;
>
> Regards
>
> Marcel

Thanks,
Kiran

2020-07-02 17:22:50

by K, Kiran

[permalink] [raw]
Subject: RE: [PATCH 3/5] Bluetooth: btintel: Refactor firmware payload download code

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <[email protected]>
> Sent: Thursday, July 2, 2020 6:58 PM
> To: K, Kiran <[email protected]>
> Cc: linux-bluetooth <[email protected]>; Srivatsa, Ravishankar
> <[email protected]>; Tumkur Narayan, Chethan
> <[email protected]>; [email protected]; Bag, Amit K
> <[email protected]>; Raghuram Hegde <[email protected]>
> Subject: Re: [PATCH 3/5] Bluetooth: btintel: Refactor firmware payload
> download code
>
> Hi Kiran,
>
> > Move firmware payload download code to a separate function to enhance
> > readability and reusability
>
> this patch is doing more than that. It also introduces an extra public function.

The existing firmware download function is suffixed with "_legacy" to distinguish from an upcoming method to support firmware download in tlv format. I will retain the older one as it is and use "_tlv" suffix for new one without losing the intention of the methods. I will send the changes in the next version.
>
> Regards
>
> Marcel

Thanks,
Kiran


2020-07-02 17:27:13

by K, Kiran

[permalink] [raw]
Subject: RE: [PATCH 5/5] Bluetooth: btintel: Parse controller information present in TLV format

Hi Marcel,

> -----Original Message-----
> From: [email protected] <linux-bluetooth-
> [email protected]> On Behalf Of Marcel Holtmann
> Sent: Thursday, July 2, 2020 7:04 PM
> To: K, Kiran <[email protected]>
> Cc: linux-bluetooth <[email protected]>; Srivatsa, Ravishankar
> <[email protected]>; Tumkur Narayan, Chethan
> <[email protected]>; [email protected]; Bag, Amit K
> <[email protected]>; Raghuram Hegde <[email protected]>
> Subject: Re: [PATCH 5/5] Bluetooth: btintel: Parse controller information
> present in TLV format
>
> Hi Kiran,
>
> > New generation Intel controllers returns controller information in TLV
> > format. Adding capability to parse and log it for debug purpose
> >
> > Signed-off-by: Kiran K <[email protected]>
> > Signed-off-by: Amit K Bag <[email protected]>
> > Signed-off-by: Raghuram Hegde <[email protected]>
> > Reviewed-by: Chethan T N <[email protected]>
> > Reviewed-by: Sathish Narasimman <[email protected]>
> > Reviewed-by: Srivatsa Ravishankar <[email protected]>
> > ---
> > drivers/bluetooth/btintel.c | 161 ++++++++++++++++++++++++++++++++----
> > drivers/bluetooth/btusb.c | 4 +-
> > 2 files changed, 148 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index d0c6576212d7..f0b087d97a83 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -209,27 +209,59 @@ void btintel_version_info(struct hci_dev *hdev,
> > const struct btintel_version *ve {
> > const char *variant;
> > const struct intel_version *ver;
> > + const struct intel_version_tlv *ver_tlv;
> >
> > - if (version->is_tlv_supported)
> > - return;
> > + if (!version->is_tlv_supported) {
> > + ver = &version->intel_version;
> > +
> > + switch (ver->fw_variant) {
> > + case 0x06:
> > + variant = "Bootloader";
> > + break;
>
> The break; has to be indented with the variant =.

Ack. checkpatch.pl didn't report any warning/error for this issue :(. I will fix it in the next patch.
>
> > + case 0x23:
> > + variant = "Firmware";
> > + break;
> > + default:
> > + goto done;
> > + }
> >
> > - ver = &version->intel_version;
> > + bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
> > + variant, ver->fw_revision >> 4,
> > + ver->fw_revision & 0x0f, ver->fw_build_num,
> > + ver->fw_build_ww, 2000 + ver->fw_build_yy);
> > + goto done;
> > + }
> > +
> > + ver_tlv = &version->intel_version_tlv;
> >
> > - switch (ver->fw_variant) {
> > - case 0x06:
> > + switch (ver_tlv->img_type) {
> > + case 0x01:
> > variant = "Bootloader";
> > - break;
> > - case 0x23:
> > + bt_dev_info(hdev, "Device revision is %u", ver_tlv-
> >dev_rev_id);
> > + bt_dev_info(hdev, "Secure boot is %s",
> > + ver_tlv->secure_boot ? "enabled" : "disabled");
> > + bt_dev_info(hdev, "OTP lock is %s",
> > + ver_tlv->otp_lock ? "enabled" : "disabled");
> > + bt_dev_info(hdev, "API lock is %s",
> > + ver_tlv->api_lock ? "enabled" : "disabled");
> > + bt_dev_info(hdev, "Debug lock is %s",
> > + ver_tlv->debug_lock ? "enabled" : "disabled");
> > + bt_dev_info(hdev, "Minimum firmware build %u week %u
> %u",
> > + ver_tlv->min_fw_build_nn, ver_tlv-
> >min_fw_build_cw,
> > + 2000 + ver_tlv->min_fw_build_yy);
> > + break;
> > + case 0x03:
> > variant = "Firmware";
> > - break;
> > + break;
> > default:
> > - return;
> > + goto done;
> > }
> >
> > - bt_dev_info(hdev, "%s revision %u.%u build %u week %u %u",
> > - variant, ver->fw_revision >> 4, ver->fw_revision & 0x0f,
> > - ver->fw_build_num, ver->fw_build_ww,
> > - 2000 + ver->fw_build_yy);
> > + bt_dev_info(hdev, "%s timestamp %u.%u buildtype %u build %u",
> variant,
> > + 2000 + (ver_tlv->timestamp >> 8), ver_tlv->timestamp & 0xff,
> > + ver_tlv->build_type, ver_tlv->build_num);
> > +done:
> > + return;
> > }
> > EXPORT_SYMBOL_GPL(btintel_version_info);
> >
> > @@ -346,6 +378,8 @@ int btintel_read_version(struct hci_dev *hdev,
> > struct btintel_version *version) {
> > struct sk_buff *skb;
> > u8 *data, param, status, check_tlv;
> > + struct intel_version_tlv *ver_tlv;
> > + struct intel_tlv *tlv;
> >
> > if (!version)
> > return -EINVAL;
> > @@ -373,9 +407,106 @@ int btintel_read_version(struct hci_dev *hdev,
> struct btintel_version *version)
> > if (skb->len == sizeof(version->intel_version) && check_tlv == 0x37) {
> > memcpy(&version->intel_version, skb->data, sizeof(version-
> >intel_version));
> > version->is_tlv_supported = false;
> > - } else {
> > - version->is_tlv_supported = true;
> > + goto done;
> > }
> > +
> > + bt_dev_info(hdev, "Supports tlv firmware download sequence");
> > + version->is_tlv_supported = true;
> > + ver_tlv = &version->intel_version_tlv;
> > +
> > + /* Consume Command Complete Status field */
> > + skb_pull(skb, 1);
> > +
> > + /* Event parameters contatin multiple TLVs. Read each of them
> > + * and only keep the required data. Also, it use existing legacy
> > + * version field like hw_platform, hw_variant, and fw_variant
> > + * to keep the existing setup flow
> > + */
> > + while (skb->len) {
> > + tlv = (struct intel_tlv *)skb->data;
> > + switch (tlv->type) {
> > + case INTEL_TLV_CNVI_TOP:
> > + ver_tlv->cnvi_top = (tlv->val[3] << 24) |
> > + (tlv->val[2] << 16) |
> > + (tlv->val[1] << 8) |
> > + (tlv->val[0]);
>
> We have get_unaligned functions for that.
>
Ack.
> > + break;
> > + case INTEL_TLV_CNVR_TOP:
> > + ver_tlv->cnvr_top = (tlv->val[3] << 24) |
> > + (tlv->val[2] << 16) |
> > + (tlv->val[1] << 8) |
> > + (tlv->val[0]);
> > + break;
> > + case INTEL_TLV_CNVI_BT:
> > + ver_tlv->cnvi_bt = (tlv->val[3] << 24) |
> > + (tlv->val[2] << 16) |
> > + (tlv->val[1] << 8) |
> > + (tlv->val[0]);
> > + break;
> > + case INTEL_TLV_CNVR_BT:
> > + ver_tlv->cnvr_bt = (tlv->val[3] << 24) |
> > + (tlv->val[2] << 16) |
> > + (tlv->val[1] << 8) |
> > + (tlv->val[0]);
> > + break;
> > + case INTEL_TLV_USB_VENDOR_ID:
> > + ver_tlv->usb_vid = (tlv->val[1] << 8) | (tlv->val[0]);
> > + break;
> > + case INTEL_TLV_USB_PRODUCT_ID:
> > + ver_tlv->usb_pid = (tlv->val[1] << 8) | (tlv->val[0]);
> > + break;
> > + case INTEL_TLV_IMAGE_TYPE:
> > + ver_tlv->img_type = tlv->val[0];
> > + break;
> > + case INTEL_TLV_TIME_STAMP:
> > + ver_tlv->timestamp = (tlv->val[1] << 8) | (tlv->val[0]);
> > + break;
> > + case INTEL_TLV_BUILD_TYPE:
> > + ver_tlv->build_type = tlv->val[0];
> > + break;
> > + case INTEL_TLV_BUILD_NUM:
> > + ver_tlv->build_num = (tlv->val[3] << 24) |
> > + (tlv->val[2] << 16) |
> > + (tlv->val[1] << 8) |
> > + (tlv->val[0]);
> > + break;
> > + case INTEL_TLV_SECURE_BOOT:
> > + ver_tlv->secure_boot = tlv->val[0];
> > + break;
> > + case INTEL_TLV_KEY_FROM_HDR:
> > + ver_tlv->key_from_hdr = tlv->val[0];
> > + break;
> > + case INTEL_TLV_OTP_LOCK:
> > + ver_tlv->otp_lock = tlv->val[0];
> > + break;
> > + case INTEL_TLV_API_LOCK:
> > + ver_tlv->api_lock = tlv->val[0];
> > + break;
> > + case INTEL_TLV_DEBUG_LOCK:
> > + ver_tlv->debug_lock = tlv->val[0];
> > + break;
> > + case INTEL_TLV_MIN_FW:
> > + ver_tlv->min_fw_build_nn = tlv->val[0];
> > + ver_tlv->min_fw_build_cw = tlv->val[1];
> > + ver_tlv->min_fw_build_yy = tlv->val[2];
> > + break;
> > + case INTEL_TLV_LIMITED_CCE:
> > + ver_tlv->limited_cce = tlv->val[0];
> > + break;
> > + case INTEL_TLV_SBE_TYPE:
> > + ver_tlv->sbe_type = tlv->val[0];
> > + break;
> > + case INTEL_TLV_OTP_BDADDR:
> > + memcpy(&ver_tlv->otp_bd_addr, tlv->val, tlv->len);
> > + break;
> > + default:
> > + /* Ignore rest of information */
> > + break;
> > + }
> > + /* consume the current tlv and move to next*/
> > + skb_pull(skb, tlv->len + sizeof(*tlv));
> > + }
> > +done:
> > kfree_skb(skb);
> > return 0;
> > }
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 364da6d44ee3..f30b43e15a26 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2519,13 +2519,13 @@ static int btusb_setup_intel_new(struct hci_dev
> *hdev)
> > return err;
> > }
> >
> > + btintel_version_info(hdev, &version);
> > +
> > if (version.is_tlv_supported) {
> > bt_dev_err(hdev, "Firmware download in tlv format is not
> supported");
> > return -EOPNOTSUPP;
> > }
> >
> > - btintel_version_info(hdev, &version);
> > -
> > err = btusb_intel_download_firmware(hdev, &version, &params);
> > if (err)
> > return err;
>
> Regards
>
> Marcel

Thanks,
Kiran