RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware)

["Rosen, Rami" <rami.rosen@intel.com>]

Hi Amir,
Here are my 2 cents:

This method always returns true, should be void (unless you will change PDF_ERROR_NOTIFICATION  or other pdf values to return false), and  likewise its invocation should not check return value.

> +static bool nhi_msg_from_icm_analysis(struct tbt_nhi_ctxt *nhi_ctxt,
> +					enum pdf_value pdf,
> +					const u8 *msg, u32 msg_len)
> +{
> +	/*
> +	 * preparation for messages that won't be sent,
> +	 * currently unused in this patch.
> +	 */
> +	bool send_event = true;
> +
> +	switch (pdf) {
> +	case PDF_ERROR_NOTIFICATION:
> +		dev_err(&nhi_ctxt->pdev->dev,
> +			"controller id %#x PDF_ERROR_NOTIFICATION %hhu
> msg len %u\n",
> +			nhi_ctxt->id, msg[11], msg_len);
> +		/* fallthrough */
> +	case PDF_WRITE_CONFIGURATION_REGISTERS:
> +		/* fallthrough */
> +	case PDF_READ_CONFIGURATION_REGISTERS:
> +		if (nhi_ctxt->wait_for_icm_resp) {
> +			nhi_ctxt->wait_for_icm_resp = false;
> +			up(&nhi_ctxt->send_sem);
> +		}
> +		break;
> +
> +	case PDF_FW_TO_SW_RESPONSE:
> +		if (nhi_ctxt->wait_for_icm_resp) {
> +			nhi_ctxt->wait_for_icm_resp = false;
> +			up(&nhi_ctxt->send_sem);
> +		}
> +		break;
> +
> +	default:
> +		dev_warn(&nhi_ctxt->pdev->dev,
> +			 "controller id %#x pdf %u isn't handled/expected\n",
> +			 nhi_ctxt->id, pdf);
> +		break;
> +	}
> +
> +	return send_event;
> +}
> +

This methods always returns 0, should be void.

> +static int nhi_suspend(struct device *dev) __releases(&nhi_ctxt-
> >send_sem)
> +{
> +	struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> +	void __iomem *rx_reg, *tx_reg;
> +	u32 rx_reg_val, tx_reg_val;
> +
> +	/* must be after negotiation_events, since messages might be sent
> */
> +	nhi_ctxt->d0_exit = true;
> +
> +	rx_reg = nhi_ctxt->iobase + REG_RX_OPTIONS_BASE +
> +		 (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> +	rx_reg_val = ioread32(rx_reg) & ~REG_OPTS_E2E_EN;
> +	tx_reg = nhi_ctxt->iobase + REG_TX_OPTIONS_BASE +
> +		 (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> +	tx_reg_val = ioread32(tx_reg) & ~REG_OPTS_E2E_EN;
> +	/* disable RX flow control  */
> +	iowrite32(rx_reg_val, rx_reg);
> +	/* disable TX flow control  */
> +	iowrite32(tx_reg_val, tx_reg);
> +	/* disable RX ring  */
> +	iowrite32(rx_reg_val & ~REG_OPTS_VALID, rx_reg);
> +
> +	mutex_lock(&nhi_ctxt->d0_exit_mailbox_mutex);
> +	mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> +
> +	cancel_work_sync(&nhi_ctxt->icm_msgs_work);
> +
> +	if (nhi_ctxt->wait_for_icm_resp) {
> +		nhi_ctxt->wait_for_icm_resp = false;
> +		nhi_ctxt->ignore_icm_resp = false;
> +		/*
> +		 * if there is response, it is lost, so unlock the send
> +		 * for the next resume.
> +		 */
> +		up(&nhi_ctxt->send_sem);
> +	}
> +
> +	mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> +	mutex_unlock(&nhi_ctxt->d0_exit_mailbox_mutex);
> +
> +	/* wait for all TX to finish  */
> +	usleep_range(5 * USEC_PER_MSEC, 7 * USEC_PER_MSEC);
> +
> +	/* disable all interrupts */
> +	iowrite32(0, nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE);
> +	/* disable TX ring  */
> +	iowrite32(tx_reg_val & ~REG_OPTS_VALID, tx_reg);
> +
> +	return 0;
> +}
> +

This methods also always returns 0, should be void.

> +static int nhi_resume(struct device *dev) __acquires(&nhi_ctxt-
> >send_sem)
> +{
> +	dma_addr_t phys;
> +	struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> +	struct tbt_buf_desc *desc;
> +	void __iomem *iobase = nhi_ctxt->iobase;
> +	void __iomem *reg;
> +	int i;
> +
> +	if (nhi_ctxt->msix_entries) {
> +		iowrite32(ioread32(iobase + REG_DMA_MISC) |
> +
> 	REG_DMA_MISC_INT_AUTO_CLEAR,
> +			  iobase + REG_DMA_MISC);
> +		/*
> +		 * Vector #0, which is TX complete to ICM,
> +		 * isn't been used currently.
> +		 */
> +		nhi_set_int_vec(nhi_ctxt, 0, 1);
> +
> +		for (i = 2; i < nhi_ctxt->num_vectors; i++)
> +			nhi_set_int_vec(nhi_ctxt, nhi_ctxt->num_paths -
> (i/2),
> +					i);
> +	}
> +
> +	/* configure TX descriptors */
> +	for (i = 0, phys = nhi_ctxt->icm_ring_shared_mem_dma_addr;
> +	     i < TBT_ICM_RING_NUM_TX_BUFS;
> +	     i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> +		desc = &nhi_ctxt->icm_ring_shared_mem->tx_buf_desc[i];
> +		desc->phys = cpu_to_le64(phys);
> +		desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS);
> +	}
> +	/* configure RX descriptors */
> +	for (i = 0;
> +	     i < TBT_ICM_RING_NUM_RX_BUFS;
> +	     i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> +		desc = &nhi_ctxt->icm_ring_shared_mem->rx_buf_desc[i];
> +		desc->phys = cpu_to_le64(phys);
> +		desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS |
> +					       DESC_ATTR_INT_EN);
> +	}
> +
> +	/* configure throttling rate for interrupts */
> +	for (i = 0, reg = iobase + REG_INT_THROTTLING_RATE;
> +	     i < NUM_INT_VECTORS;
> +	     i++, reg += REG_INT_THROTTLING_RATE_STEP) {
> +		iowrite32(USEC_TO_256_NSECS(128), reg);
> +	}
> +
> +	/* configure TX for ICM ring */
> +	reg = iobase + REG_TX_RING_BASE + (TBT_ICM_RING_NUM *
> REG_RING_STEP);
> +	phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> +		offsetof(struct tbt_icm_ring_shared_memory, tx_buf_desc);
> +	iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> +	iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> +	iowrite32((TBT_ICM_RING_NUM_TX_BUFS <<
> REG_RING_SIZE_SHIFT) &
> +			REG_RING_SIZE_MASK,
> +		  reg + REG_RING_SIZE_OFFSET);
> +
> +	reg = iobase + REG_TX_OPTIONS_BASE +
> (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> +	iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> +
> +	/* configure RX for ICM ring */
> +	reg = iobase + REG_RX_RING_BASE + (TBT_ICM_RING_NUM *
> REG_RING_STEP);
> +	phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> +		offsetof(struct tbt_icm_ring_shared_memory, rx_buf_desc);
> +	iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> +	iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> +	iowrite32(((TBT_ICM_RING_NUM_RX_BUFS <<
> REG_RING_SIZE_SHIFT) &
> +			REG_RING_SIZE_MASK) |
> +		  ((TBT_ICM_RING_MAX_FRAME_SIZE <<
> REG_RING_BUF_SIZE_SHIFT) &
> +			REG_RING_BUF_SIZE_MASK),
> +		  reg + REG_RING_SIZE_OFFSET);
> +	iowrite32(((TBT_ICM_RING_NUM_RX_BUFS - 1) <<
> REG_RING_CONS_SHIFT) &
> +			REG_RING_CONS_MASK,
> +		  reg + REG_RING_CONS_PROD_OFFSET);
> +
> +	reg = iobase + REG_RX_OPTIONS_BASE +
> (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> +	iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> +
> +	/* enable RX interrupt */
> +	RING_INT_ENABLE_RX(iobase, TBT_ICM_RING_NUM, nhi_ctxt-
> >num_paths);
> +
> +	if (likely((atomic_read(&subscribers) > 0) &&
> +		   nhi_nvm_authenticated(nhi_ctxt))) {
> +		down(&nhi_ctxt->send_sem);
> +		nhi_ctxt->d0_exit = false;
> +		mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> +		/*
> +		 * interrupts are enabled here before send due to
> +		 * implicit barrier in mutex
> +		 */
> +		nhi_send_driver_ready_command(nhi_ctxt);
> +		mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> +	} else {
> +		nhi_ctxt->d0_exit = false;
> +	}
> +
> +	return 0;
> +}


Regards,
Rami Rosen
Intel Corporation