Return-Path: <linux-bluetooth-owner@vger.kernel.org>
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\))
Subject: Re: [PATCH 3/5] Bluetooth: btintel: Use boot parameter from firmware
 file
From: Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <1516763869-12411-3-git-send-email-tedd.an@linux.intel.com>
Date: Wed, 24 Jan 2018 08:34:47 +0100
Cc: linux-bluetooth@vger.kernel.org, tedd.an@intel.com,
        amit.k.bag@intel.com
Message-Id: <DA44A105-D058-4703-B36F-7600C40236A1@holtmann.org>
References: <1516763869-12411-1-git-send-email-tedd.an@linux.intel.com>
 <1516763869-12411-3-git-send-email-tedd.an@linux.intel.com>
To: Tedd Ho-Jeong An <tedd.an@linux.intel.com>
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

Hi Tedd,

> Each RAM SKU has a different boot parameter which is used in
> HCI_Intel_Reset command after downloading the firmware.
> The boot parameter is embedded in the firmware data and to support
> multiple SKUs, driver reads the boot parameter while downloading
> the firmware instead of using static values per SKU.
> 
> Signed-off-by: Tedd Ho-Jeong An <tedd.an@linux.intel.com>
> ---
> drivers/bluetooth/btusb.c     | 22 ++++++++++++++++++++--
> drivers/bluetooth/hci_intel.c | 23 +++++++++++++++++++++--
> 2 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 93db5b9..979d530 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2024,8 +2024,11 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> 
> 	BT_DBG("%s", hdev->name);
> 
> -	/* The default boot parameter */
> -	boot_param = 0x00040800;
> +	/* Set the default boot parameter to 0x0 and it is updated to
> +	 * SKU specific boot parameter after reading Intel_Write_Boot_Params
> +	 * command while downloading the firmware.
> +	 */
> +	boot_param = 0x00000000;
> 
> 	calltime = ktime_get();
> 
> @@ -2269,6 +2272,21 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> 	while (fw_ptr - fw->data < fw->size) {
> 		struct hci_command_hdr *cmd = (void *)(fw_ptr + frag_len);
> 
> +		/* Each SKU has a different reset parameter to use in the
> +		 * HCI_Intel_Reset command and it is embedded in the firmware
> +		 * data. So, instead of using static value per SKU, check
> +		 * the firmware data and save it for later use.
> +		 */
> +		if (cmd->opcode == 0xfc0e) {
> +

scrap this empty line here.

> +			/* The boot parameter is the first 32-bit value
> +			 * and rest of 3 octets are reserved.
> +			 */
> +			boot_param = get_unaligned_le32(fw_ptr + sizeof(*cmd));
> +
> +			bt_dev_dbg(hdev, "boot_param=0x%x", boot_param);
> +		}
> +
> 		frag_len += sizeof(*cmd) + cmd->plen;
> 
> 		/* The parameter length of the secure send command requires
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index acac48d..2053d73 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -33,6 +33,7 @@
> #include <linux/acpi.h>
> #include <linux/interrupt.h>
> #include <linux/pm_runtime.h>
> +#include <asm/unaligned.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -562,8 +563,11 @@ static int intel_setup(struct hci_uart *hu)
> 	hu->hdev->set_diag = btintel_set_diag;
> 	hu->hdev->set_bdaddr = btintel_set_bdaddr;
> 
> -	/* Default boot parameter */
> -	boot_param = 0x00040800;
> +	/* Set the default boot parameter to 0x0 and it is updated to
> +	 * SKU specific boot parameter after reading Intel_Write_Boot_Params
> +	 * command while downloading the firmware.
> +	 */
> +	boot_param = 0x00000000;
> 
> 	calltime = ktime_get();
> 
> @@ -824,6 +828,21 @@ static int intel_setup(struct hci_uart *hu)
> 	while (fw_ptr - fw->data < fw->size) {
> 		struct hci_command_hdr *cmd = (void *)(fw_ptr + frag_len);
> 
> +		/* Each SKU has a different reset parameter to use in the
> +		 * HCI_Intel_Reset command and it is embedded in the firmware
> +		 * data. So, instead of using static value per SKU, check
> +		 * the firmware data and save it for later use.
> +		 */
> +		if (cmd->opcode == 0xfc0e) {
> +

Same here. Remove this empty line.

> +			/* The boot parameter is the first 32-bit value
> +			 * and rest of 3 octets are reserved.
> +			 */
> +			boot_param = get_unaligned_le32(fw_ptr + sizeof(*cmd));
> +
> +			bt_dev_dbg(hdev, "boot_param=0x%x", boot_param);
> +		}
> +
> 		frag_len += sizeof(*cmd) + cmd->plen;
> 
> 		bt_dev_dbg(hdev, "Patching %td/%zu", (fw_ptr - fw->data),

Regards

Marcel