Re: [PATCH] bus: mhi: Add inbound buffers allocation flag

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Jan 06, 2021 at 02:43:43PM +0100, Loic Poulain wrote:
> Currently, the MHI controller driver defines which channels should
> have their inbound buffers allocated and queued. But ideally, this is
> something that should be decided by the MHI device driver instead,

We call them, "MHI client drivers"

> which actually deals with that buffers.
> 
> Add a flag parameter to mhi_prepare_for_transfer allowing to specify
> if buffers have to be allocated and queued by the MHI stack.
> 
> Keep auto_queue flag for now, but should be removed at some point.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  drivers/bus/mhi/core/internal.h |  2 +-
>  drivers/bus/mhi/core/main.c     | 11 ++++++++---
>  drivers/net/mhi_net.c           |  2 +-
>  include/linux/mhi.h             | 12 +++++++++++-
>  net/qrtr/mhi.c                  |  2 +-
>  5 files changed, 22 insertions(+), 7 deletions(-)
> 

[...]

> diff --git a/drivers/net/mhi_net.c b/drivers/net/mhi_net.c
> index fa41d8c..b7f7f2e 100644
> --- a/drivers/net/mhi_net.c
> +++ b/drivers/net/mhi_net.c
> @@ -265,7 +265,7 @@ static int mhi_net_probe(struct mhi_device *mhi_dev,
>  	u64_stats_init(&mhi_netdev->stats.tx_syncp);
>  
>  	/* Start MHI channels */
> -	err = mhi_prepare_for_transfer(mhi_dev);
> +	err = mhi_prepare_for_transfer(mhi_dev, 0);

Eventhough I'd like Hemant to comment on this patch, AFAIU this looks to
me a controller dependent behaviour. The controller should have the
information whether a particular channel can auto queue or not then the
client driver can be agnostic.

>  	if (err)
>  		goto out_err;
>  
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 209b335..6723339 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -60,6 +60,14 @@ enum mhi_flags {
>  };
>  
>  /**
> + * enum mhi_chan_flags - MHI channel flags
> + * @MHI_CH_INBOUND_ALLOC_BUFS: Automatically allocate and queue inbound buffers
> + */
> +enum mhi_chan_flags {
> +	MHI_CH_INBOUND_ALLOC_BUFS = BIT(0),
> +};
> +
> +/**
>   * enum mhi_device_type - Device types
>   * @MHI_DEVICE_XFER: Handles data transfer
>   * @MHI_DEVICE_CONTROLLER: Control device
> @@ -705,8 +713,10 @@ void mhi_device_put(struct mhi_device *mhi_dev);
>  /**
>   * mhi_prepare_for_transfer - Setup channel for data transfer
>   * @mhi_dev: Device associated with the channels
> + * @flags: MHI channel flags
>   */
> -int mhi_prepare_for_transfer(struct mhi_device *mhi_dev);
> +int mhi_prepare_for_transfer(struct mhi_device *mhi_dev,
> +			     enum mhi_chan_flags flags);
>  
>  /**
>   * mhi_unprepare_from_transfer - Unprepare the channels
> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
> index 2bf2b19..47afded 100644
> --- a/net/qrtr/mhi.c
> +++ b/net/qrtr/mhi.c
> @@ -77,7 +77,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
>  	int rc;
>  
>  	/* start channels */
> -	rc = mhi_prepare_for_transfer(mhi_dev);
> +	rc = mhi_prepare_for_transfer(mhi_dev, MHI_CH_INBOUND_ALLOC_BUFS);

Are you sure it requires auto queued channel?

Thanks,
Mani

>  	if (rc)
>  		return rc;
>  
> -- 
> 2.7.4
>