Hi Luiz,
Thanks for your comments.
>Hi Kiran,
>
>On Tue, Feb 18, 2025 at 7:27 AM Kiran K <kiran.k@xxxxxxxxx> wrote:
>>
>> On hardware error, controller writes hardware error event and optional
>> vendor specific hci events in device memory in TLV format and raises
>> MSIX interrupt. Driver reads the device memory and passes the events
>> to the stack for further processing.
>>
>> Co-developed-by: Vijay Satija <vijay.satija@xxxxxxxxx>
>> Signed-off-by: Vijay Satija <vijay.satija@xxxxxxxxx>
>> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
>> ---
>> drivers/bluetooth/btintel.h | 1 +
>> drivers/bluetooth/btintel_pcie.c | 212
>> ++++++++++++++++++++++++++++++- drivers/bluetooth/btintel_pcie.h |
>> 23 ++++
>> 3 files changed, 235 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
>> index 19530ea14905..4c21e69887a3 100644
>> --- a/drivers/bluetooth/btintel.h
>> +++ b/drivers/bluetooth/btintel.h
>> @@ -190,6 +190,7 @@ enum {
>> struct btintel_data {
>> DECLARE_BITMAP(flags, __INTEL_NUM_FLAGS);
>> int (*acpi_reset_method)(struct hci_dev *hdev);
>> + u32 cnvi_top;
>> };
>>
>> #define btintel_set_flag(hdev, nr) \
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index 11e2b805c7cc..6c78472f9dad 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -51,6 +51,14 @@ MODULE_DEVICE_TABLE(pci, btintel_pcie_table);
>>
>> #define BTINTEL_PCIE_MAGIC_NUM 0xA5A5A5A5
>>
>> +#define BTINTEL_PCIE_BLZR_HWEXP_SIZE 1024
>> +#define BTINTEL_PCIE_BLZR_HWEXP_DMP_ADDR 0xB00A7C00
>> +
>> +#define BTINTEL_PCIE_SCP_HWEXP_SIZE 4096
>> +#define BTINTEL_PCIE_SCP_HWEXP_DMP_ADDR 0xB030F800
>> +
>> +#define BTINTEL_PCIE_MAGIC_NUM 0xA5A5A5A5
>> +
>> /* Alive interrupt context */
>> enum {
>> BTINTEL_PCIE_ROM,
>> @@ -353,6 +361,63 @@ static int btintel_pcie_reset_bt(struct
>btintel_pcie_data *data)
>> return reg == 0 ? 0 : -ENODEV; }
>>
>> +static void btintel_pcie_mac_init(struct btintel_pcie_data *data) {
>> + u32 reg;
>> +
>> + /* Set MAC_INIT bit to start primary bootloader */
>> + 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_wr_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG,
>> +reg); }
>> +
>> +static int btintel_pcie_get_mac_access(struct btintel_pcie_data
>> +*data) {
>> + u32 reg;
>> + int retry = 15;
>> +
>> + reg = btintel_pcie_rd_reg32(data,
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>> +
>> + reg |= BTINTEL_PCIE_CSR_FUNC_CTRL_STOP_MAC_ACCESS_DIS;
>> + reg |= BTINTEL_PCIE_CSR_FUNC_CTRL_XTAL_CLK_REQ;
>> + if ((reg & BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_STS) == 0)
>> + reg |= BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_REQ;
>> +
>> + 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_MAC_ACCESS_STS)
>> + return 0;
>> + usleep_range(1000, 1200);
>> +
>> + } while (--retry > 0);
>
>There seems to be a similar call to btintel_pcie_wr_reg32(data,
>BTINTEL_PCIE_CSR_FUNC_CTRL_REG, reg); in btintel_pcie_mac_init but that
>doesn't retry for some reason, maybe leave a comment if that is intentional,
>that said this write, read and wait is sort of weird to me, if we know it takes
>time there should probably be an event coming back, rather than polling
>multiple times in quick succession.
>
In btintel_pcie_mac_init(), only initialization of mac block is done. When Target Access is done via mac a delay is required for the hardware to settle down. I would add comment about this in the v2 version of the patch.
>> +
>> + return -ETIME;
>> +}
>> +
>> +static void btintel_pcie_release_mac_access(struct btintel_pcie_data
>> +*data) {
>> + u32 reg;
>> +
>> + reg = btintel_pcie_rd_reg32(data,
>> + BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>> +
>> + if (reg & BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_REQ)
>> + reg &= ~BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_ACCESS_REQ;
>> +
>> + if (reg & BTINTEL_PCIE_CSR_FUNC_CTRL_STOP_MAC_ACCESS_DIS)
>> + reg &=
>> + ~BTINTEL_PCIE_CSR_FUNC_CTRL_STOP_MAC_ACCESS_DIS;
>> +
>> + if (reg & BTINTEL_PCIE_CSR_FUNC_CTRL_XTAL_CLK_REQ)
>> + reg &= ~BTINTEL_PCIE_CSR_FUNC_CTRL_XTAL_CLK_REQ;
>> +
>> + btintel_pcie_wr_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG,
>> +reg); }
>> +
>> /* This function enables BT function by setting
>BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT bit in
>> * BTINTEL_PCIE_CSR_FUNC_CTRL_REG register and wait for MSI-X with
>> * BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0.
>> @@ -473,6 +538,133 @@ static inline char
>*btintel_pcie_alivectxt_state2str(u32 alive_intr_ctxt)
>> }
>> }
>>
>> +static int btintel_pcie_read_device_mem(struct btintel_pcie_data *data,
>> + void *buf, u32 dev_addr, int
>> +len) {
>> + int err;
>> + u32 *val = buf;
>> +
>> + /* Get device mac access */
>> + err = btintel_pcie_get_mac_access(data);
>> + if (err) {
>> + bt_dev_err(data->hdev, "Failed to get mac access %d", err);
>> + return err;
>> + }
>> +
>> + for (; len > 0; len -= 4, dev_addr += 4, val++)
>> + *val = btintel_pcie_rd_dev_mem(data, dev_addr);
>> +
>> + btintel_pcie_release_mac_access(data);
>> +
>> + return 0;
>> +}
>> +
>> +static void btintel_pcie_dump_hwexp(struct btintel_pcie_data *data) {
>> + struct btintel_data *intel_data = hci_get_priv(data->hdev);
>> + int len, err, offset, pending;
>> + struct sk_buff *skb;
>> + u32 addr, val;
>> + u8 *buf;
>> +
>> + struct tlv {
>> + u8 type;
>> + u16 len;
>> + u8 val[];
>> + } __packed;
>> +
>> + struct tlv *tlv;
>> +
>> + switch (intel_data->cnvi_top & 0xfff) {
>> + case BTINTEL_CNVI_BLAZARI:
>> + case BTINTEL_CNVI_BLAZARIW:
>> + /* only from step B0 onwards */
>> + if (INTEL_CNVX_TOP_STEP(intel_data->cnvi_top) != 0x01)
>> + return;
>> + len = BTINTEL_PCIE_BLZR_HWEXP_SIZE; /* exception data length */
>> + addr = BTINTEL_PCIE_BLZR_HWEXP_DMP_ADDR;
>> + break;
>> + case BTINTEL_CNVI_SCP:
>> + len = BTINTEL_PCIE_SCP_HWEXP_SIZE;
>> + addr = BTINTEL_PCIE_SCP_HWEXP_DMP_ADDR;
>> + break;
>> + default:
>> + bt_dev_err(data->hdev, "Unsupported cnvi 0x%8x", intel_data-
>>cnvi_top);
>> + return;
>> + }
>> +
>> + buf = kzalloc(len, GFP_KERNEL);
>> + if (!buf)
>> + goto exit_on_error;
>> +
>> + btintel_pcie_mac_init(data);
>> +
>> + err = btintel_pcie_read_device_mem(data, buf, addr, len);
>> + if (err)
>> + goto exit_on_error;
>> +
>> + val = get_unaligned_le32(buf);
>> + if (val != BTINTEL_PCIE_MAGIC_NUM) {
>> + bt_dev_err(data->hdev, "Invalid exception dump signature:
>0x%8.8x",
>> + val);
>> + goto exit_on_error;
>> + }
>> +
>> + offset = 4;
>> + do {
>> + pending = len - offset;
>> + if (pending < sizeof(*tlv))
>> + break;
>> + tlv = (struct tlv *)(buf + offset);
>> + if (!tlv->type) {
>> + bt_dev_dbg(data->hdev, "Invalid TLV type 0");
>> + break;
>> + }
>> + tlv->len = le16_to_cpu((__force __le16)tlv->len);
>> + offset += sizeof(*tlv);
>> + pending = len - offset;
>> + if (tlv->len > pending)
>> + break;
>> +
>> + offset += tlv->len;
>> + /* Only TLV of type = 1 are vendor HCI events */
>> + if (tlv->type != 1)
>> + continue;
>> +
>> + bt_dev_dbg(data->hdev, "Exception pkt len: %u", tlv->len);
>> + if (tlv->len > HCI_MAX_EVENT_SIZE)
>> + break;
>> + skb = bt_skb_alloc(tlv->len, GFP_KERNEL);
>> + if (!skb)
>> + goto exit_on_error;
>> + hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
>> + skb_put_data(skb, tlv->val, tlv->len);
>> +
>> + /* copy Intel specific pcie packet type */
>> + val = BTINTEL_PCIE_HCI_EVT_PKT;
>> + memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &val,
>> + BTINTEL_PCIE_HCI_TYPE_LEN);
>> +
>> + print_hex_dump(KERN_DEBUG, "Bluetooth: ",
>DUMP_PREFIX_OFFSET, 16,
>> + 1, tlv->val, tlv->len, false);
>> +
>> + skb_queue_tail(&data->rx_skb_q, skb);
>> + queue_work(data->workqueue, &data->rx_work);
>> + } while (offset < len);
>
>Could there be multiple exceptions? If there is only one the perhaps you could
As of now there will be 2 TLVs - Hardware Error event and Telemetry event. Later there can be vendor specific event related to debug.
>use eir_get_data instead of recreating the code for parsing LTV entries,
It looks we have conflict in type field used in eir_get_data() and TLVs used here. Right now, TLVs with type == 1 is considered as HCI event and is processed.
>although we could possibly rename that to make it more generic.
>
>> +
>> +exit_on_error:
>> + kfree(buf);
>> +}
>> +
>> +static void btintel_pcie_hwexp_work(struct work_struct *work) {
>> + struct btintel_pcie_data *data = container_of(work,
>> + struct btintel_pcie_data, hwexp_work);
>> + btintel_pcie_dump_hwexp(data);
>> +
>> + clear_bit(BTINTEL_PCIE_HWEXP_INPROGRESS, &data->flags); }
>> +
>> /* This function handles the MSI-X interrupt for gp0 cause (bit 0 in
>> * BTINTEL_PCIE_CSR_MSIX_HW_INT_CAUSES) which is sent for boot stage
>and image response.
>> */
>> @@ -794,6 +986,14 @@ static int btintel_pcie_recv_frame(struct
>btintel_pcie_data *data,
>> return ret;
>> }
>>
>> +static void btintel_pcie_msix_hw_exp_handler(struct btintel_pcie_data
>> +*data) {
>> + bt_dev_err(data->hdev, "Received hw exception interrupt");
>> + if (test_and_set_bit(BTINTEL_PCIE_HWEXP_INPROGRESS, &data->flags))
>> + return;
>> + queue_work(data->workqueue, &data->hwexp_work);
>
>Any particular reason why we can process the exception in the rx_work?
>That shall probably be made into a comment either way.
Unlike in USB products, controller doesn’t send HW Error event when exception happens. Controller writes hardware error event into device memory, raises MSIX and halts.
Driver reads HW error from device memory, constructs skb and pushes to rx_work. I thought its better to do all this in separate work item instead of cluttering rx_work.
>
>> +}
>> +
>> static void btintel_pcie_rx_work(struct work_struct *work) {
>> struct btintel_pcie_data *data = container_of(work, @@ -920,6
>> +1120,10 @@ static irqreturn_t btintel_pcie_irq_msix_handler(int irq, void
>*dev_id)
>> return IRQ_NONE;
>> }
>>
Thanks,
Kiran
