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. 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.
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.
> 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
