Hi Bjron,
Thanks for your comments.
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Thursday, March 28, 2024 10:38 PM
> To: K, Kiran <kiran.k@xxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Srivatsa, Ravishankar
> <ravishankar.srivatsa@xxxxxxxxx>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@xxxxxxxxx>; An, Tedd <tedd.an@xxxxxxxxx>
> Subject: Re: [PATCH v1 2/3] Bluetooth: btintel_pcie: Add support for PCIE
> transport
>
> On Thu, Mar 28, 2024 at 04:49:03PM +0530, Kiran K wrote:
> > From: Tedd Ho-Jeong An <tedd.an@xxxxxxxxx>
> >
> > Add initial code to support Intel bluetooth devices based on PICE
> > transport. This patch allocates memory for buffers, internal
> > structures, initializes interrupts for TX & RX and initializes PCIE device.
>
> s/PCIE/PCIe/ (subject, commit log, throughout) s/PICE/PCIe/ (commit log
> typo) s/This patch allocates/Allocate/ s/initializes/initialize/ (twice)
>
> > +#if BTITNEL_PCIE_ENABLE_HCI_DUMP
> > +static inline void btintel_pcie_hci_dump(const char *p, const void
> > +*b, int s) {
> > + const unsigned char *ptr = (const unsigned char *)b;
> > + char str[64];
> > + int c, i;
> > +
> > + for (i = c = 0; c < s; c++) {
> > + i += snprintf(str + i, sizeof(str) - i, "%02x ", ptr[c]);
> > + if ((c > 0 && (c + 1) % 8 == 0) || (c == s - 1)) {
> > + BT_DBG("%s: %s", p, str);
> > + i = 0;
> > + }
> > + }
>
> Maybe print_hex_dump() or similar would save you some work here?
I will remove this function as we have monitor to dump all the HCI traffic.
>
> > +/* Poll internal in microseconds */
>
> s/internal/interval/
>
> > +#define POLL_INTERVAL 10
>
> I think it's nice when the name of a timeout or interval includes the units, e.g.,
> POLL_INTERVAL_US.
Ack.
>
> > +/* Set the doorbell for RXQ to notify the device that @index(actually
> > +index-1)
> > + * is available to receive the data
>
> Typical style is to add a space before the opening paren in English text (many
> occurrences).
Ack.
>
> > +static void btintel_pcie_set_rx_db(struct btintel_pcie_data *data,
> > +u16 index) {
> > + u32 val;
> > +
> > + val = index;
> > + val |= (513 << 16);
>
> Where does 513 come from? Maybe a #define or explanatory comment?
Its specific to hardware. I will define a macro for the same.
>
> > +static void btintel_pcie_prepare_rx(struct rxq *rxq, u16 frbd_index)
> > +{
> > + struct data_buf *buf;
> > + struct frbd *frbd;
> > +
> > + /* Get the buffer of the frbd for DMA */
>
> s/frbd/FRBD/ for consistency (several occurrences).
Ack.
>
> > +static void btintel_pcie_rx_work(struct work_struct *work) {
> > + struct btintel_pcie_data *data = container_of(work,
> > + struct btintel_pcie_data, rx_work);
> > + struct sk_buff *skb;
> > + int err;
> > +
> > + /* Process the sk_buf in queue and send to the hci layer */
>
> s/hci/HCI/ for consistency.
Ack.
>
> > +/* create the sk_buff with data and save it to queue and start rx
> > +work */ static int btintel_pcie_submit_rx_work(struct
> > +btintel_pcie_data *data, u8 status,
> > + void *buf)
>
> Comment would fit on one line.
>
> There's a random mix of "rx" vs "RX" in comments, would be nice to be
> consistent.
>
Ack.
> > +static void btintel_pcie_msix_rx_handle(struct btintel_pcie_data
> > +*data) {
> > + u16 cr_hia, cr_tia;
> > + struct rxq *rxq;
> > + struct urbd1 *urbd1;
> > + struct frbd *frbd;
> > + struct data_buf *buf;
> > + int ret;
> > +
> > + cr_hia = data->ia.cr_hia[RXQ_NUM];
> > + cr_tia = data->ia.cr_tia[RXQ_NUM];
> > +
> > + BT_DBG("[RXQ] cr_hia=%u cr_tia=%u", cr_hia, cr_tia);
> > +
> > + /* Check CR_TIA and CR_HIA for change */
> > + if (cr_tia == cr_hia) {
> > + BT_ERR("[RXQ] no new CD found");
> > + return;
> > + }
> > +
> > + rxq = &data->rxq;
> > +
> > + /* The firmware sends multiple CD in a single MSIX and it needs to
> > + * process all received CDs in this interrupt.
> > + */
> > + while (cr_tia != cr_hia) {
> > + /* Get URBD1 pointed by cr_tia */
> > + urbd1 = &rxq->urbd1s[cr_tia];
> > + ipc_print_urbd1(urbd1, cr_tia);
> > +
> > + /* Get FRBD poined by urbd1->frbd_tag */
>
> s/poined/pointed to/ ?
Ack. I will remove the comment as its related to handing ring buffer indices.
>
> > + * Before sending the interrupt the HW disables it to prevent
> > + * a nested interrupt. This is done by writing 1 to the corresponding
> > + * bit in the mask register. After handling the interrupt, it should be
> > + * re-enabled by clearing this bit. This register is defined as
> > + * write 1 clear (W1C) register, meaning that it's being clear
> > + * by writing 1 to the bit.
>
> s/being clear/cleared/
Ack.
>
> > + num_irqs = pci_enable_msix_range(data->pdev, data->msix_entries,
> > + MSIX_VEC_MIN,
> > + MSIX_VEC_MAX);
>
> pci_enable_msix_range() is deprecated; consider
> pci_alloc_irq_vectors() instead.
Ack.
>
> > +static int btintel_pcie_config_pcie(struct pci_dev *pdev,
> > + struct btintel_pcie_data *data) {
> > + int err;
> > +
> > + err = pcim_enable_device(pdev);
> > + if (err) {
> > + BT_ERR("Failed to enable pci device (%d)", err);
> > + return err;
> > + }
> > + pci_set_master(pdev);
> > +
> > + /* Setup DMA mask */
>
> Superfluous comment.
Ack. I will remove all these type of obvious comments.
>
> > + BT_DBG("Set DMA_MASK(64)");
> > + err = dma_set_mask_and_coherent(&pdev->dev,
> DMA_BIT_MASK(64));
> > + if (err) {
> > + BT_DBG("Set DMA_MASK(32)");
> > + err = dma_set_mask_and_coherent(&pdev->dev,
> DMA_BIT_MASK(32));
> > + /* Both attempt failed */
> > + if (err) {
> > + BT_ERR("No suitable DMA available");
> > + return err;
> > + }
> > + }
> > +
> > + /* Get BAR to access CSR */
>
> Superfluous.
>
> > + err = pcim_iomap_regions(pdev, BIT(0), KBUILD_MODNAME);
> > + if (err) {
> > + BT_ERR("Failed to get iomap regions (%d)", err);
> > + return err;
> > + }
> > +
> > + data->base_addr = pcim_iomap_table(pdev)[0];
> > + if (!data->base_addr) {
> > + BT_ERR("Failed to get base address");
> > + return -ENODEV;
> > + }
> > +
> > + err = btintel_pcie_setup_irq(data);
> > + if (err) {
> > + BT_ERR("Failed to setup irq for msix");
> > + return err;
> > + }
> > +
> > + /* Configure MSI-X with causes list */
>
> Random mix of "MSIX" and "MSI-X". I think "MSI-X" is nicer.
Ack.
>
> > +static int btintel_pcie_setup_txq_bufs(struct btintel_pcie_data *data,
> > + struct txq *txq)
> > +{
> > + int err = 0, i;
> > + struct data_buf *buf;
> > +
> > + if (txq->count == 0) {
> > + BT_ERR("invalid parameter: txq->count");
> > + err = -EINVAL;
> > + goto exit_error;
>
> You do all the cleanup inline, so there's really no benefit to "err"
> and the "exit_error" label. You could "return -EINVAL" directly here and similar
> below.
Ack.
>
> > + }
> > +
> > + /* Allocate the same number of buffers as the descriptor */
> > + txq->bufs = kmalloc_array(txq->count, sizeof(*buf), GFP_KERNEL);
> > + if (!txq->bufs) {
> > + err = -ENOMEM;
> > + goto exit_error;
> > + }
> > +
> > + /* Allocate full chunk of data buffer for DMA first and do indexing and
> > + * initialization next, so it can be freed easily
> > + */
> > + txq->buf_v_addr = dma_alloc_coherent(&data->pdev->dev,
> > + txq->count * BUFFER_SIZE,
> > + &txq->buf_p_addr,
> > + GFP_KERNEL | __GFP_NOWARN);
> > + if (!txq->buf_v_addr) {
> > + BT_ERR("Failed to allocate DMA buf");
> > + err = -ENOMEM;
> > + kfree(txq->bufs);
> > + goto exit_error;
> > + }
> > + memset(txq->buf_v_addr, 0, txq->count * BUFFER_SIZE);
> > +
> > + BT_DBG("alloc bufs: p=0x%llx v=0x%p", txq->buf_p_addr,
> > +txq->buf_v_addr);
> > +
> > + /* Setup the allocated DMA buffer to bufs. Each data_buf should
> > + * have virtual address and physical address
> > + */
> > + for (i = 0; i < txq->count; i++) {
> > + buf = &txq->bufs[i];
> > + buf->data_p_addr = txq->buf_p_addr + (i * BUFFER_SIZE);
> > + buf->data = txq->buf_v_addr + (i * BUFFER_SIZE);
> > + }
> > +
> > +exit_error:
> > + return err;
> > +}
>
> > +static int btintel_pcie_setup_rxq_bufs(struct btintel_pcie_data *data,
> > + struct rxq *rxq)
> > +{
> > + int err = 0, i;
> > + struct data_buf *buf;
> > +
> > + if (rxq->count == 0) {
> > + BT_ERR("invalid parameter: rxq->count");
> > + err = -EINVAL;
> > + goto exit_error;
>
> Same "err", "exit_error" comment.
>
Ack.
> > + }
> > +
> > + /* Allocate the same number of buffers as the descriptor */
> > + rxq->bufs = kmalloc_array(rxq->count, sizeof(*buf), GFP_KERNEL);
> > + if (!rxq->bufs) {
> > + err = -ENOMEM;
> > + goto exit_error;
> > + }
> > +
> > + /* Allocate full chunk of data buffer for DMA first and do indexing and
> > + * initialization next, so it can be freed easily
> > + */
> > + rxq->buf_v_addr = dma_alloc_coherent(&data->pdev->dev,
> > + rxq->count * BUFFER_SIZE,
> > + &rxq->buf_p_addr,
> > + GFP_KERNEL | __GFP_NOWARN);
> > + if (!rxq->buf_v_addr) {
> > + BT_ERR("Failed to allocate DMA buf");
> > + err = -ENOMEM;
> > + kfree(rxq->bufs);
> > + goto exit_error;
> > + }
> > + memset(rxq->buf_v_addr, 0, rxq->count * BUFFER_SIZE);
> > +
> > + BT_DBG("alloc bufs: p=0x%llx v=0x%p", rxq->buf_p_addr,
> > +rxq->buf_v_addr);
> > +
> > + /* Setup the allocated DMA buffer to bufs. Each data_buf should
> > + * have virtual address and physical address
> > + */
> > + for (i = 0; i < rxq->count; i++) {
> > + buf = &rxq->bufs[i];
> > + buf->data_p_addr = rxq->buf_p_addr + (i * BUFFER_SIZE);
> > + buf->data = rxq->buf_v_addr + (i * BUFFER_SIZE);
> > + }
> > +
> > +exit_error:
> > +
> > + return err;
> > +}
>
> > +static void btintel_pcie_release_hdev(struct btintel_pcie_data *data)
> > +{
> > + struct hci_dev *hdev;
> > +
> > + hdev = data->hdev;
> > + if (hdev) {
>
> This test shouldn't be necessary if the driver is written correctly.
> We should never get here unless .probe() was successful.
Ack.
>
> > + hci_unregister_dev(hdev);
>
> There should be a corresponding hci_register_dev(). Since that's not here yet,
> I don't think the hci_unregister_dev() should be in this patch either. Looks like
> maybe just incomplete code?
>
> Oh, I see hci_register_dev() is added in patch 3/3. They should be added in
> the same patch.
Ack.
>
> > + hci_free_dev(hdev);
> > + }
> > + data->hdev = NULL;
> > +}
>
> > +static int btintel_pcie_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *ent) {
> > + int err;
> > + struct btintel_pcie_data *data;
> > +
> > + if (!pdev)
> > + return -ENODEV;
> > +
> > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + /* initialize the btintel_pcie data struct */
>
> Superfluous comment.
>
> > + data->pdev = pdev;
> > +
> > + spin_lock_init(&data->irq_lock);
> > + spin_lock_init(&data->hci_rx_lock);
> > +
> > + init_waitqueue_head(&data->gp0_wait_q);
> > + data->gp0_received = false;
> > +
> > + init_waitqueue_head(&data->tx_wait_q);
> > + data->tx_wait_done = false;
> > +
> > + data->workqueue = alloc_ordered_workqueue(KBUILD_MODNAME,
> WQ_HIGHPRI);
> > + if (!data->workqueue) {
> > + BT_ERR("Failed to create workqueue");
> > + return -ENOMEM;
> > + }
> > + skb_queue_head_init(&data->rx_skb_q);
> > + INIT_WORK(&data->rx_work, btintel_pcie_rx_work);
> > +
> > + data->boot_stage_cache = 0x00;
> > + data->img_resp_cache = 0x00;
> > +
> > + /* PCIe specific all to configure it for this device includes
> > + * enabling pice device, setting master, reading BAR[0], configuring
> > + * MSIx, setting DMA mask, and save the driver data.
>
> s/pice/PCIe/
> s/MSIx/MSI-X/
> s/save the/saving the/
>
> Whole comment is probably superfluous since this is common to all PCI
> drivers.
Ack.
>
> > + */
> > + err = btintel_pcie_config_pcie(pdev, data);
> > + if (err) {
> > + BT_ERR("Failed to config pcie (%d)", err);
> > + goto exit_error;
> > + }
> > +
> > + /* Set driver data for this PCI device */
>
> Superfluous comment.
>
> > + pci_set_drvdata(pdev, data);
> > +
> > + /* allocate the IPC struct */
> > + err = btintel_pcie_alloc(data);
> > + if (err) {
> > + BT_ERR("Failed to allocate queues(%d)", err);
> > + goto exit_error;
> > + }
> > +
> > + /* Enable BT function */
>
> Superfluous comment.
>
> > + err = btintel_pcie_enable_bt(data);
> > + if (err) {
> > + BT_ERR("Failed to start bluetooth device(%d)", err);
> > + goto exit_error;
> > + }
> > +
> > + /* CNV information (CNVi and CNVr) is in CSR */
> > + data->cnvi = btintel_pcie_rd_reg32(data, CSR_HW_REV_REG);
> > + BT_DBG("cnvi: 0x%08x", data->cnvi);
> > +
> > + data->cnvr = btintel_pcie_rd_reg32(data, CSR_RF_ID_REG);
> > + BT_DBG("cnvr: 0x%08x", data->cnvr);
> > +
> > + err = btintel_pcie_start_rx(data);
> > + if (err) {
> > + BT_ERR("Failed to start rx (%d)", err);
> > + goto exit_error;
> > + }
> > +
> > + err = btintel_pcie_setup_hdev(data);
> > + if (err) {
> > + BT_ERR("Failed to setup HCI module");
> > + goto exit_error;
> > + }
> > +
> > + return 0;
> > +
> > +exit_error:
> > + /* reset device before leave */
> > + btintel_pcie_reset_bt(data);
> > +
> > + /* clear bus mastering */
>
> Superfluous comment.
>
> > + pci_clear_master(pdev);
> > +
> > + /* Unset driver data for PCI device */
>
> Superfluous comment.
>
> > + pci_set_drvdata(pdev, NULL);
> > +
> > + return err;
> > +}
> > +
> > +static void btintel_pcie_remove(struct pci_dev *pdev) {
> > + struct btintel_pcie_data *data;
> > +
> > + if (!pdev) {
>
> Shouldn't need this check. If you get here with "pdev == NULL", the driver did
> something wrong and it's better to oops so the driver can be fixed.
Ack.
>
> > + BT_ERR("Invalid parameter: pdev");
> > + return;
> > + }
> > +
> > + data = pci_get_drvdata(pdev);
> > + if (!data) {
>
> Or this one.
Ack.
>
> > + BT_ERR("data is empty");
> > + return;
> > + }
> > +
> > + btintel_pcie_release_hdev(data);
> > +
> > + flush_work(&data->rx_work);
> > +
> > + destroy_workqueue(data->workqueue);
> > +
> > + btintel_pcie_free(data);
> > +
> > + /* reset device before leave */
> > + btintel_pcie_reset_bt(data);
> > +
> > + /* clear bus mastering */
>
> Superfluous comment.
>
> > + pci_clear_master(pdev);
> > +
> > + /* Unset driver data for PCI device */
>
> Superfluous comment.
>
> > + pci_set_drvdata(pdev, NULL);
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int btintel_pcie_suspend(struct device *dev) {
> > + /* TODO: Add support suspend */
> > + return 0;
> > +}
> > +
> > +static int btintel_pcie_resume(struct device *dev) {
> > + /* TODO: Add support resume */
> > + return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(btintel_pcie_pm_ops, btintel_pcie_suspend,
> > + btintel_pcie_resume);
> > +#endif /* CONFIG_PM */
>
> Seems pointless to include this #ifdef CONFIG_PM code until you implement it.
Ack.
>
> > +static struct pci_driver btintel_pcie_driver = {
> > + .name = KBUILD_MODNAME,
> > + .id_table = btintel_pcie_table,
> > + .probe = btintel_pcie_probe,
> > + .remove = btintel_pcie_remove,
> > +#ifdef CONFIG_PM
> > + .driver.pm = &btintel_pcie_pm_ops,
> > +#endif /* CONFIG_PM */
> > +};
>
> > +/* Default Poll time for MAC access during init*/
> > +#define DEFAULT_MAC_ACCESS_TIMEOUT 200000
>
> Would be nice to have units here in the name (us, ms, etc).
Ack.
>
> > +/* Default interrupt timeout in msec */
> > +#define DEFAULT_INTR_TIMEOUT 3000
>
> And here.
Ack.
>
> > +/* Enum for RBD buffer size mappting */
>
> s/mappting/mapping/ ?
>
> > + * @dbgc_addr: DBGC first fragmemt address
> > + * @dbgc_size: DBGC buffer size
> > + * @early_enable: Enarly debug enable
>
> s/fragmemt/fragment/
> s/Enarly/Early/
Ack.
>
> > + * @dbg_output_mode: Debug output mode
> > + * Bit[4] DBGC O/P { 0=SRAM, 1=DRAM(not relevant for NPK) }
> > + * Bit[5] DBGC I/P { 0=BDBG, 1=DBGI }
> > + * Bits[6:7] DBGI O/P(relevant if bit[5] = 1)
> > + * 0=BT DBGC, 1=WiFi DBGC, 2=NPK }
> > + * @dbg_preset: Debug preset
> > + * @ext_addr: Address of context information extension
> > + * @ext_size: Size of context information part
> > + *
> > + * Total 38 DWords
> > + *
>
> Superfluous blank line
>
> > + */
>
> Bjorn
Thanks,
Kiran
