Message-ID: <g3bz2wcmdvrv626mltjjf3czribogpsz5ttuhv24ioege4q7fl@pgmspcgoyso6>
Date: Tue, 13 Jan 2026 13:32:09 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Sivareddy Surasani <sivareddy.surasani@....qualcomm.com>
Cc: Jonathan Corbet <corbet@....net>, Arnd Bergmann <arnd@...db.de>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, mhi@...ts.linux.dev, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, 
	Upal Kumar Saha <upal.saha@....qualcomm.com>, Himanshu Shukla <quic_himashuk@...cinc.com>, 
	Vivek Pernamitta <vivek.pernamitta@....qualcomm.com>
Subject: Re: [PATCH 03/11] bus: mhi: host: Add support for queuing multiple
 DMA buffers

On Thu, Dec 11, 2025 at 01:37:35PM +0530, Sivareddy Surasani wrote:
> From: Vivek Pernamitta <vivek.pernamitta@....qualcomm.com>
> 
> Optimize MHI clients by allowing them to queue multiple DMA buffers for a
> given transfer without ringing the channel doorbell for every queue. This
> avoids unnecessary link access. Introduce the exported API mhi_queue_n_dma
> to pass an array of MHI buffers and MHI flags.
> 

Why do you need a new API? Why can't the client work with existing
mhi_queue_buf() API? I don't quite understand the need for a 'DMA' API since the
existing mhi_queue_buf() API allows the client to either DMA map the buffer
beforehand or it can do it internally.

> Currently, the BEI flag is set for all TREs based on the interrupt
> moderation timer value.

> MHI clients are not allowed to block event
> interrupts at runtime.

This is not true as per the spec. Host is allowed to set BEI flag for any TRE
during runtime, but the Linux MHI stack allows only to configure the BEI flag
statically during controller driver probe based on intmod value.

> If interrupt moderation is disabled for an event
> ring, the client is allowed to poll on events posted on the event ring by
> blocking the MSI.

'...blocking the interrupt for the completion event'. Here MSI is not
appropriate as it is *one of the* interrupt mechanism.

> If interrupt moderation is enabled, the BEI flag passed
> in the queue API is overridden to maintain the current implementation tied
> to the interrupt moderation timer value.
> 
> For scatter-gather transfers, MHI clients should set the MHI_SG transfer
> flag. This flag allows skipping the issuance of transfer callbacks per TRE
> and only issuing a single callback when the last TRE is processed.
> 

Spec defines that scatter gather transfers are identified using the 'chain' bit.
So I don't see a need for a separate flag.

Until the above points are clarified, I'm not inclined to look into the code.

- Mani

> Signed-off-by: Vivek Pernamitta <vivek.pernamitta@....qualcomm.com>
> Signed-off-by: Sivareddy Surasani <sivareddy.surasani@....qualcomm.com>
> ---
>  drivers/bus/mhi/host/internal.h |   8 ++
>  drivers/bus/mhi/host/main.c     | 203 +++++++++++++++++++++++++++++++++++-----
>  include/linux/mhi.h             |  26 +++++
>  3 files changed, 213 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
> index 7937bb1f742c..97bf6a70b9fa 100644
> --- a/drivers/bus/mhi/host/internal.h
> +++ b/drivers/bus/mhi/host/internal.h
> @@ -236,6 +236,7 @@ struct mhi_buf_info {
>  	enum dma_data_direction dir;
>  	bool used; /* Indicates whether the buffer is used or not */
>  	bool pre_mapped; /* Already pre-mapped by client */
> +	bool sg_enabled; /* perform sg and return single completion call back */
>  };
>  
>  struct mhi_event {
> @@ -414,6 +415,13 @@ irqreturn_t mhi_intvec_handler(int irq_number, void *dev);
>  
>  int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
>  		struct mhi_buf_info *info, enum mhi_flags flags);
> +int mhi_gen_n_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
> +		  struct mhi_buf *bufs, enum mhi_flags flags[],
> +		  unsigned int num);
> +int __mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
> +		  struct mhi_buf_info *info, enum mhi_flags flags,
> +		  struct mhi_ring *buf_ring,  struct mhi_ring *tre_ring);
> +
>  int mhi_map_single_no_bb(struct mhi_controller *mhi_cntrl,
>  			 struct mhi_buf_info *buf_info);
>  int mhi_map_single_use_bb(struct mhi_controller *mhi_cntrl,
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index 861551274319..7beb848ca5c1 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -605,7 +605,8 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
>  		struct mhi_ring_element *local_rp, *ev_tre;
>  		void *dev_rp, *next_rp;
>  		struct mhi_buf_info *buf_info;
> -		u16 xfer_len;
> +		u16 xfer_len, total_tre_len = 0;
> +		bool send_cb = false;
>  
>  		if (!is_valid_ring_ptr(tre_ring, ptr)) {
>  			dev_err(&mhi_cntrl->mhi_dev->dev,
> @@ -635,10 +636,14 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
>  		while (local_rp != dev_rp) {
>  			buf_info = buf_ring->rp;
>  			/* If it's the last TRE, get length from the event */
> -			if (local_rp == ev_tre)
> +			if (local_rp == ev_tre) {
>  				xfer_len = MHI_TRE_GET_EV_LEN(event);
> -			else
> +				send_cb = true;
> +			} else {
>  				xfer_len = buf_info->len;
> +			}
> +
> +			total_tre_len += xfer_len;
>  
>  			/* Unmap if it's not pre-mapped by client */
>  			if (likely(!buf_info->pre_mapped))
> @@ -655,13 +660,28 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
>  
>  			read_unlock_bh(&mhi_chan->lock);
>  
> +
>  			/* notify client */
> -			mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
> +			if (buf_info->sg_enabled) {
> +				if (send_cb) {
> +					result.bytes_xferd = total_tre_len;
> +					mhi_chan->xfer_cb(mhi_chan->mhi_dev,
> +							  &result);
> +				}
> +			} else {
> +				mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
> +			}
>  
>  			if (mhi_chan->dir == DMA_TO_DEVICE) {
>  				atomic_dec(&mhi_cntrl->pending_pkts);
> -				/* Release the reference got from mhi_queue() */
> -				mhi_cntrl->runtime_put(mhi_cntrl);
> +				/*
> +				 * In case of scatter gather send_cb is set to true only
> +				 * for the last TRE, runtime_put should be called for
> +				 * last TRE instead of every buffer i.e, when send_cb
> +				 * is true else runtime_put count will not be balanced
> +				 */
> +				if (!buf_info->sg_enabled || send_cb)
> +					mhi_cntrl->runtime_put(mhi_cntrl);
>  			}
>  
>  			/*
> @@ -1192,25 +1212,14 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>  }
>  EXPORT_SYMBOL_GPL(mhi_queue_skb);
>  
> -int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
> -			struct mhi_buf_info *info, enum mhi_flags flags)
> +int __mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
> +		  struct mhi_buf_info *info, enum mhi_flags flags,
> +		  struct mhi_ring *buf_ring,  struct mhi_ring *tre_ring)
>  {
> -	struct mhi_ring *buf_ring, *tre_ring;
>  	struct mhi_ring_element *mhi_tre;
>  	struct mhi_buf_info *buf_info;
>  	int eot, eob, chain, bei;
> -	int ret = 0;
> -
> -	/* Protect accesses for reading and incrementing WP */
> -	write_lock_bh(&mhi_chan->lock);
> -
> -	if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> -
> -	buf_ring = &mhi_chan->buf_ring;
> -	tre_ring = &mhi_chan->tre_ring;
> +	int ret;
>  
>  	buf_info = buf_ring->wp;
>  	WARN_ON(buf_info->used);
> @@ -1227,24 +1236,55 @@ int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
>  	if (!info->pre_mapped) {
>  		ret = mhi_cntrl->map_single(mhi_cntrl, buf_info);
>  		if (ret)
> -			goto out;
> +			return ret;
>  	}
>  
> +	trace_mhi_gen_tre(mhi_cntrl, mhi_chan, mhi_tre);
> +
>  	eob = !!(flags & MHI_EOB);
>  	eot = !!(flags & MHI_EOT);
>  	chain = !!(flags & MHI_CHAIN);
> -	bei = !!(mhi_chan->intmod);
> +
> +	buf_info->sg_enabled = !!(flags & MHI_SG);
> +
> +	/* honor bei flag if interrupt moderation is disabled */
> +	bei = !!(mhi_chan->intmod ?
> +				mhi_chan->intmod : flags & MHI_BEI);
>  
>  	mhi_tre = tre_ring->wp;
>  	mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr);
>  	mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(info->len);
>  	mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, eot, eob, chain);
>  
> -	trace_mhi_gen_tre(mhi_cntrl, mhi_chan, mhi_tre);
> +	if (mhi_chan->dir == DMA_TO_DEVICE)
> +		atomic_inc(&mhi_cntrl->pending_pkts);
> +
>  	/* increment WP */
>  	mhi_add_ring_element(mhi_cntrl, tre_ring);
>  	mhi_add_ring_element(mhi_cntrl, buf_ring);
>  
> +	return 0;
> +}
> +
> +int mhi_gen_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
> +			struct mhi_buf_info *info, enum mhi_flags flags)
> +{
> +	struct mhi_ring *buf_ring, *tre_ring;
> +	int ret = 0;
> +
> +	/* Protect accesses for reading and incrementing WP */
> +	write_lock_bh(&mhi_chan->lock);
> +
> +	if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	buf_ring = &mhi_chan->buf_ring;
> +	tre_ring = &mhi_chan->tre_ring;
> +
> +	ret = __mhi_gen_tre(mhi_cntrl, mhi_chan, info, flags, buf_ring, tre_ring);
> +
>  out:
>  	write_unlock_bh(&mhi_chan->lock);
>  
> @@ -1264,6 +1304,121 @@ int mhi_queue_buf(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>  }
>  EXPORT_SYMBOL_GPL(mhi_queue_buf);
>  
> +int mhi_gen_n_tre(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
> +		  struct mhi_buf *bufs, enum mhi_flags flags[],
> +		  unsigned int num)
> +{
> +	struct mhi_ring *buf_ring, *tre_ring;
> +	void *cur_buf_ring_wp, *cur_tre_ring_wp;
> +	int i = 0, j, ret;
> +	struct mhi_buf_info buf_info = {0};
> +	struct mhi_buf_info *info;
> +
> +	if (mhi_chan->ch_state != MHI_CH_STATE_ENABLED) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	buf_ring = &mhi_chan->buf_ring;
> +	tre_ring = &mhi_chan->tre_ring;
> +
> +	cur_buf_ring_wp = buf_ring->wp;
> +	cur_tre_ring_wp = tre_ring->wp;
> +
> +	while (num-- > 0) {
> +		buf_info.wp = tre_ring->wp;
> +		buf_info.p_addr = bufs[i].dma_addr;
> +		buf_info.cb_buf = bufs[i].buf;
> +		buf_info.len = bufs[i].len;
> +		buf_info.pre_mapped = bufs[i].streaming_dma;
> +
> +		ret = __mhi_gen_tre(mhi_cntrl, mhi_chan, &buf_info, flags[i], buf_ring, tre_ring);
> +		if (ret)
> +			goto error;
> +
> +		i++;
> +
> +		/**
> +		 * When multiple packets are queued in single queue_n_dma call
> +		 * runtime_get should be called for each packet to balance
> +		 * runtime_put and runtime_get count,
> +		 * because once we get MSI's from the device,
> +		 * we call runtime_put for each packet in parse_xfer_event
> +		 */
> +		if (!buf_info.sg_enabled)
> +			mhi_cntrl->runtime_get(mhi_cntrl);
> +	}
> +
> +	/**
> +	 * If it is a scatter gather transfer, runtime_get
> +	 * should be called only once as we call runtime_put
> +	 * only for last TRE in the parse_xfer_event
> +	 */
> +	if (buf_info.wp && buf_info.sg_enabled)
> +		mhi_cntrl->runtime_get(mhi_cntrl);
> +
> +	return 0;
> +error:
> +	buf_ring->wp = cur_buf_ring_wp;
> +	tre_ring->wp = cur_buf_ring_wp;
> +
> +	for (j = i - 1; j >= 0; j--) {
> +		atomic_dec(&mhi_cntrl->pending_pkts);
> +		info = cur_buf_ring_wp;
> +		if (!bufs[i].dma_addr)
> +			mhi_cntrl->unmap_single(mhi_cntrl, info);
> +
> +		cur_buf_ring_wp += buf_ring->el_size;
> +		if (cur_buf_ring_wp >= buf_ring->base + buf_ring->len)
> +			cur_buf_ring_wp = buf_ring->base;
> +	}
> +
> +out:
> +	return ret;
> +
> +}
> +
> +int mhi_queue_n_dma(struct mhi_device *mhi_dev, enum dma_data_direction dir,
> +		    struct mhi_buf *bufs, enum mhi_flags mflags[],
> +		    unsigned int num)
> +{
> +	unsigned long flags;
> +	int ret;
> +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> +	struct mhi_chan *mhi_chan = (dir == DMA_TO_DEVICE) ? mhi_dev->ul_chan :
> +							     mhi_dev->dl_chan;
> +
> +	if (unlikely(MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)))
> +		return -EIO;
> +
> +	write_lock_irqsave(&mhi_chan->lock, flags);
> +
> +	if (get_nr_avail_ring_elements(mhi_cntrl, &mhi_chan->tre_ring) < num) {
> +		ret = -EAGAIN;
> +		goto error;
> +	}
> +
> +	ret = mhi_gen_n_tre(mhi_dev->mhi_cntrl, mhi_chan, bufs, mflags,
> +			    num);
> +	if (ret)
> +		goto error;
> +
> +	/* Assert dev_wake (to exit/prevent M1/M2)*/
> +	mhi_cntrl->wake_toggle(mhi_cntrl);
> +
> +	if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)))
> +		mhi_ring_chan_db(mhi_cntrl, mhi_chan);
> +
> +	if (dir == DMA_FROM_DEVICE)
> +		mhi_cntrl->runtime_put(mhi_cntrl);
> +
> +error:
> +	write_unlock_irqrestore(&mhi_chan->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(mhi_queue_n_dma);
> +
>  bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir)
>  {
>  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index dd372b0123a6..360770ddef70 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -52,11 +52,15 @@ enum mhi_callback {
>   * @MHI_EOB: End of buffer for bulk transfer
>   * @MHI_EOT: End of transfer
>   * @MHI_CHAIN: Linked transfer
> + * @MHI_BEI: Block event interrupt
> + * @MHI_SG: scatter-gather enabled, single xfer call back to client
>   */
>  enum mhi_flags {
>  	MHI_EOB = BIT(0),
>  	MHI_EOT = BIT(1),
>  	MHI_CHAIN = BIT(2),
> +	MHI_BEI = BIT(3),
> +	MHI_SG = BIT(4),
>  };
>  
>  /**
> @@ -497,6 +501,7 @@ struct mhi_result {
>   *        ECA - Event context array data
>   *        CCA - Channel context array data
>   * @dma_addr: IOMMU address of the buffer
> + * @streaming_dma: Set this flag by client for pre allocated streaming dma address
>   * @len: # of bytes
>   */
>  struct mhi_buf {
> @@ -504,6 +509,7 @@ struct mhi_buf {
>  	const char *name;
>  	dma_addr_t dma_addr;
>  	size_t len;
> +	bool streaming_dma;
>  };
>  
>  /**
> @@ -770,6 +776,13 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev);
>   */
>  void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev);
>  
> +/**
> + * mhi_poll - Poll for any available data in DL direction
> + * @mhi_dev: Device associated with the channels
> + * @budget: # of events to process
> + */
> +int mhi_poll(struct mhi_device *mhi_dev, u32 budget);
> +
>  /**
>   * mhi_queue_buf - Send or receive raw buffers from client device over MHI
>   *                 channel
> @@ -782,6 +795,19 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev);
>  int mhi_queue_buf(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>  		  void *buf, size_t len, enum mhi_flags mflags);
>  
> +/**
> + * mhi_queue_n_dma - Send or receive n DMA mapped buffers from client device
> + *                 over MHI channel
> + * @mhi_dev: Device associated with the channels
> + * @dir: DMA direction for the channel
> + * @mhi_buf[]: Array of mhi_buf for holding the DMA mapped data and len
> + * @mflags[]: Array of MHI transfer flags used for the transfer
> + * @num: Number of transfers
> + */
> +int mhi_queue_n_dma(struct mhi_device *mhi_dev, enum dma_data_direction dir,
> +		  struct mhi_buf *bufs, enum mhi_flags mflags[],
> +		  unsigned int num);
> +
>  /**
>   * mhi_queue_skb - Send or receive SKBs from client device over MHI channel
>   * @mhi_dev: Device associated with the channels
> 
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists