Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\))
Subject: Re: [PATCH v1] Bluetooth: btintel: Fix boot address
From:   Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <20210820115808.15895-1-kiran.k@intel.com>
Date:   Mon, 30 Aug 2021 17:07:55 +0200
Cc:     "open list:BLUETOOTH SUBSYSTEM" <linux-bluetooth@vger.kernel.org>,
        "Srivatsa, Ravishankar" <ravishankar.srivatsa@intel.com>,
        Chethan T N <chethan.tumkur.narayan@intel.com>,
        Tedd Ho-Jeong An <tedd.an@intel.com>,
        Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Content-Transfer-Encoding: 8BIT
Message-Id: <7E8C3893-19CD-44D6-B224-A3434F65BA95@holtmann.org>
References: <20210820115808.15895-1-kiran.k@intel.com>
To:     Kiran K <kiran.k@intel.com>
Precedence: bulk

Hi Kiran,

> Cache Boot address present in firmware file which
> is later used Intel_Soft_Reset command to bring
> controller from boot mode to operational mode.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> ---
> drivers/bluetooth/btintel.c | 31 +++++++++++++------------------
> 1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index f1705b46fc88..80d6dd7ae51a 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -1037,8 +1037,9 @@ static bool btintel_firmware_version(struct hci_dev *hdev,
> 
> 			params = (void *)(fw_ptr + sizeof(*cmd));
> 
> -			bt_dev_info(hdev, "Boot Address: 0x%x",
> -				    le32_to_cpu(params->boot_addr));
> +			*boot_addr = le32_to_cpu(params->boot_addr);
> +
> +			bt_dev_info(hdev, "Boot Address: 0x%x", *boot_addr);
> 

so this hunk looks good and is described in the commit message.

> 			bt_dev_info(hdev, "Firmware Version: %u-%u.%u",
> 				    params->fw_build_num, params->fw_build_ww,
> @@ -1071,9 +1072,6 @@ int btintel_download_firmware(struct hci_dev *hdev,
> 		/* Skip version checking */
> 		break;
> 	default:
> -		/* Skip reading firmware file version in bootloader mode */
> -		if (ver->fw_variant == 0x06)
> -			break;
> 
> 		/* Skip download if firmware has the same version */
> 		if (btintel_firmware_version(hdev, ver->fw_build_num,
> @@ -1114,19 +1112,16 @@ static int btintel_download_fw_tlv(struct hci_dev *hdev,
> 	int err;
> 	u32 css_header_ver;
> 
> -	/* Skip reading firmware file version in bootloader mode */
> -	if (ver->img_type != 0x01) {
> -		/* Skip download if firmware has the same version */
> -		if (btintel_firmware_version(hdev, ver->min_fw_build_nn,
> -					     ver->min_fw_build_cw,
> -					     ver->min_fw_build_yy,
> -					     fw, boot_param)) {
> -			bt_dev_info(hdev, "Firmware already loaded");
> -			/* Return -EALREADY to indicate that firmware has
> -			 * already been loaded.
> -			 */
> -			return -EALREADY;
> -		}
> +	/* Skip download if firmware has the same version */
> +	if (btintel_firmware_version(hdev, ver->min_fw_build_nn,
> +				     ver->min_fw_build_cw,
> +				     ver->min_fw_build_yy,
> +				     fw, boot_param)) {
> +		bt_dev_info(hdev, "Firmware already loaded");
> +		/* Return -EALREADY to indicate that firmware has
> +		 * already been loaded.
> +		 */
> +		return -EALREADY;
> 	}

This part however isn’t.

Regards

Marcel