[PATCH 0/5] Refactor firmware download

[PATCH 0/5] Refactor firmware download

[Kiran K]

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

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

[Kiran K]

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

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
 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

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

[Kiran K]

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

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
 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

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

[Marcel Holtmann]

Hi Kiran,

> Move firmware header download code to a separate function to
> enhance readability and reusability
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> 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

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

[K, Kiran]

Hi Marcel,

> -----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org <linux-bluetooth-
> owner@vger.kernel.org> On Behalf Of Marcel Holtmann
> Sent: Thursday, July 2, 2020 6:56 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth <linux-bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; kiraank@gmail.com; Bag, Amit K
> <amit.k.bag@intel.com>; Raghuram Hegde <raghuram.hegde@intel.com>
> 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 <kiran.k@intel.com>
> > Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> > Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
> > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > ---
> > 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

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

[Kiran K]

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

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
 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

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

[Marcel Holtmann]

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

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

[K, Kiran]

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Thursday, July 2, 2020 6:58 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth <linux-bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; kiraank@gmail.com; Bag, Amit K
> <amit.k.bag@intel.com>; Raghuram Hegde <raghuram.hegde@intel.com>
> 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

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

[Kiran K]

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 <kiran.k@intel.com>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
 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

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

[Kiran K]

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 <kiran.k@intel.com>
Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
---
 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

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

[Marcel Holtmann]

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 <kiran.k@intel.com>
> Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> 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

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

[K, Kiran]

Hi Marcel,

> -----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org <linux-bluetooth-
> owner@vger.kernel.org> On Behalf Of Marcel Holtmann
> Sent: Thursday, July 2, 2020 7:04 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth <linux-bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; kiraank@gmail.com; Bag, Amit K
> <amit.k.bag@intel.com>; Raghuram Hegde <raghuram.hegde@intel.com>
> 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 <kiran.k@intel.com>
> > Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> > Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
> > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > ---
> > 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