Hi Luiz,
>Subject: Re: [PATCH v1 2/2] Bluetooth: btintel_pcie: Add recovery mechanism
>
>Hi Kiran,
>
>On Tue, Oct 1, 2024 at 6:29 AM Kiran K <kiran.k@xxxxxxxxx> wrote:
>>
>> This patch adds a recovery mechanism to recover controller if firmware
>> download fails due to unresponsive controller, command timeout,
>> firmware signature verification failure etc. Driver attmepts maximum
>> of 5 times before giving up.
>
>Typo attempts, that said repeating the same process 5 times seems very
>arbitrary, we should probably attempt to reload as fewer times as possible e.g.
Ack. Just to be on safer side, I introduced max of 5 attempts to download the firmware.
>1 and only for specific reasons like a timeout, etc. I wouldn't retry on signature
>verification failure since that probably would just cause the same problem over
>and over again.
I believe its better to retry even for secure send results also. In my stress reboot test (500x), I could see this issue hitting 4/500. As the secure engine block is shared by Wifi also, due to some race condition in accessing TOP registers, BT firmware verification can fail when Wifi is also getting loaded at the time.
I feel it's better to retry instead of having bluetooth completely inaccessible.
>
>As for redoing the setup, perhaps we could have a MGMT command that
>attempts to setup the controller once again, that way we can trigger the
>command from userspace to retry the setup if for example the firmware file is
>updated, etc, or you want to manually trigger a setup for validation purposes.
>
This looks to be a new feature ?? I will look into this. Thanks.
>> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
>> ---
>> drivers/bluetooth/btintel.c | 14 ++++
>> drivers/bluetooth/btintel_pcie.c | 109 +++++++++++++++++++++----------
>> drivers/bluetooth/btintel_pcie.h | 2 +
>> 3 files changed, 91 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
>> index fed1a939aad6..c14ec6c6120b 100644
>> --- a/drivers/bluetooth/btintel.c
>> +++ b/drivers/bluetooth/btintel.c
>> @@ -1252,6 +1252,12 @@ static void btintel_reset_to_bootloader(struct
>hci_dev *hdev)
>> struct intel_reset params;
>> struct sk_buff *skb;
>>
>> + /* PCIe transport uses shared hardware reset mechanism for recovery
>> + * which gets triggered in pcie *setup* function on error.
>> + */
>> + if (hdev->bus == HCI_PCI)
>> + return;
>> +
>> /* Send Intel Reset command. This will result in
>> * re-enumeration of BT controller.
>> *
>> @@ -1267,6 +1273,7 @@ static void btintel_reset_to_bootloader(struct
>hci_dev *hdev)
>> * boot_param: Boot address
>> *
>> */
>> +
>> params.reset_type = 0x01;
>> params.patch_enable = 0x01;
>> params.ddc_reload = 0x01;
>> @@ -2796,6 +2803,13 @@ int btintel_bootloader_setup_tlv(struct hci_dev
>*hdev,
>> */
>> boot_param = 0x00000000;
>>
>> + /* In case of PCIe, this function might get called multiple times with
>> + * same hdev instance if there is any error on firmware download.
>> + * Need to clear stale bits of previous firmware download attempt.
>> + */
>> + for (int i = 0; i < __INTEL_NUM_FLAGS; i++)
>> + btintel_clear_flag(hdev, i);
>> +
>> btintel_set_flag(hdev, INTEL_BOOTLOADER);
>>
>> err = btintel_prepare_fw_download_tlv(hdev, ver, &boot_param);
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index cff3e7afdff9..a525c15c47b0 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -75,24 +75,6 @@ static inline void ipc_print_urbd1(struct hci_dev
>*hdev, struct urbd1 *urbd1,
>> index, urbd1->frbd_tag, urbd1->status,
>> urbd1->fixed); }
>>
>> -static int btintel_pcie_poll_bit(struct btintel_pcie_data *data, u32 offset,
>> - u32 bits, u32 mask, int timeout_us)
>> -{
>> - int t = 0;
>> - u32 reg;
>> -
>> - do {
>> - reg = btintel_pcie_rd_reg32(data, offset);
>> -
>> - if ((reg & mask) == (bits & mask))
>> - return t;
>> - udelay(POLL_INTERVAL_US);
>> - t += POLL_INTERVAL_US;
>> - } while (t < timeout_us);
>> -
>> - return -ETIMEDOUT;
>> -}
>> -
>> static struct btintel_pcie_data *btintel_pcie_get_data(struct
>> msix_entry *entry) {
>> u8 queue = entry->entry;
>> @@ -248,10 +230,47 @@ static void btintel_pcie_reset_ia(struct
>btintel_pcie_data *data)
>> memset(data->ia.cr_tia, 0, sizeof(u16) *
>> BTINTEL_PCIE_NUM_QUEUES); }
>>
>> -static void btintel_pcie_reset_bt(struct btintel_pcie_data *data)
>> +static int btintel_pcie_reset_bt(struct btintel_pcie_data *data)
>> {
>> - btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG,
>> - BTINTEL_PCIE_CSR_FUNC_CTRL_SW_RESET);
>> + u32 reg;
>> + int retry = 3;
>> +
>> + reg = btintel_pcie_rd_reg32(data,
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>> +
>> + reg &= ~(BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_ENA |
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT |
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_INIT);
>> + reg |= BTINTEL_PCIE_CSR_FUNC_CTRL_BUS_MASTER_DISCON;
>> +
>> + btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG,
>> + reg);
>> +
>> + do {
>> + reg = btintel_pcie_rd_reg32(data,
>BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>> + if (reg & BTINTEL_PCIE_CSR_FUNC_CTRL_BUS_MASTER_STS)
>> + break;
>> + usleep_range(10000, 12000);
>> +
>> + } while (--retry > 0);
>> + usleep_range(10000, 12000);
>> +
>> + reg = btintel_pcie_rd_reg32(data,
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>> +
>> + reg &= ~(BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_ENA |
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT |
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_INIT);
>> + reg |= BTINTEL_PCIE_CSR_FUNC_CTRL_SW_RESET;
>> + btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG, reg);
>> + usleep_range(10000, 12000);
>> +
>> + reg = btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>> + bt_dev_dbg(data->hdev, "csr register after reset: 0x%8.8x",
>> + reg);
>> +
>> + reg = btintel_pcie_rd_reg32(data,
>> + BTINTEL_PCIE_CSR_BOOT_STAGE_REG);
>> +
>> + /* If shared hardware reset is success then boot stage register shall be
>> + * set to 0
>> + */
>> + return reg == 0 ? 0 : -ENODEV;
>> }
>>
>> /* This function enables BT function by setting
>> BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT bit in @@ -263,6 +282,7 @@
>static
>> void btintel_pcie_reset_bt(struct btintel_pcie_data *data) static int
>> btintel_pcie_enable_bt(struct btintel_pcie_data *data) {
>> int err;
>> + u32 reg;
>>
>> data->gp0_received = false;
>>
>> @@ -278,22 +298,17 @@ static int btintel_pcie_enable_bt(struct
>btintel_pcie_data *data)
>> data->boot_stage_cache = 0x0;
>>
>> /* Set MAC_INIT bit to start primary bootloader */
>> - btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>> + reg = btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>> + reg &= ~(BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_INIT |
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_BUS_MASTER_DISCON |
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_SW_RESET);
>> + reg |= (BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_ENA |
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT);
>>
>> - btintel_pcie_set_reg_bits(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG,
>> - BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT);
>> -
>> - /* Wait until MAC_ACCESS is granted */
>> - err = btintel_pcie_poll_bit(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG,
>> - BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_STS,
>> - BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_STS,
>> - BTINTEL_DEFAULT_MAC_ACCESS_TIMEOUT_US);
>> - if (err < 0)
>> - return -ENODEV;
>> + btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG,
>> + reg);
>>
>> /* MAC is ready. Enable BT FUNC */
>> btintel_pcie_set_reg_bits(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG,
>> - BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_ENA |
>>
>> BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_INIT);
>>
>> btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>> @@ -1376,7 +1391,7 @@ static void btintel_pcie_release_hdev(struct
>btintel_pcie_data *data)
>> data->hdev = NULL;
>> }
>>
>> -static int btintel_pcie_setup(struct hci_dev *hdev)
>> +static int btintel_pcie_setup_internal(struct hci_dev *hdev)
>> {
>> const u8 param[1] = { 0xFF };
>> struct intel_version_tlv ver_tlv; @@ -1467,6 +1482,32 @@
>> static int btintel_pcie_setup(struct hci_dev *hdev)
>> return err;
>> }
>>
>> +static int btintel_pcie_setup(struct hci_dev *hdev) {
>> + int err, fw_dl_retry = 0;
>> + struct btintel_pcie_data *data = hci_get_drvdata(hdev);
>> +
>> + while ((err = btintel_pcie_setup_internal(hdev)) && fw_dl_retry++ < 5) {
>> + bt_dev_err(hdev, "Firmware download retry count: %d",
>> + fw_dl_retry);
>> + err = btintel_pcie_reset_bt(data);
>> + if (err) {
>> + bt_dev_err(hdev, "Failed to do shr reset: %d", err);
>> + break;
>> + }
>> + usleep_range(10000, 12000);
>> + btintel_pcie_reset_ia(data);
>> + btintel_pcie_config_msix(data);
>> + err = btintel_pcie_enable_bt(data);
>> + if (err) {
>> + bt_dev_err(hdev, "Failed to enable hardware: %d", err);
>> + break;
>> + }
>> + btintel_pcie_start_rx(data);
>> + }
>> + return err;
>> +}
>> +
>> static int btintel_pcie_setup_hdev(struct btintel_pcie_data *data) {
>> int err;
>> diff --git a/drivers/bluetooth/btintel_pcie.h
>> b/drivers/bluetooth/btintel_pcie.h
>> index 8b7824ad005a..f9aada0543c4 100644
>> --- a/drivers/bluetooth/btintel_pcie.h
>> +++ b/drivers/bluetooth/btintel_pcie.h
>> @@ -23,6 +23,8 @@
>> #define BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT (BIT(6))
>> #define BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_INIT (BIT(7))
>> #define BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_STS (BIT(20))
>> +#define BTINTEL_PCIE_CSR_FUNC_CTRL_BUS_MASTER_STS (BIT(28))
>> +#define BTINTEL_PCIE_CSR_FUNC_CTRL_BUS_MASTER_DISCON (BIT(29))
>> #define BTINTEL_PCIE_CSR_FUNC_CTRL_SW_RESET (BIT(31))
>>
>> /* Value for BTINTEL_PCIE_CSR_BOOT_STAGE register */
>> --
>> 2.40.1
>>
>>
>
>
>--
>Luiz Augusto von Dentz
Regards,
Kiran
