All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] bus: mhi: Add inbound buffers allocation flag
@ 2021-01-06 13:43 Loic Poulain
  2021-01-08 13:44 ` Manivannan Sadhasivam
  0 siblings, 1 reply; 9+ messages in thread
From: Loic Poulain @ 2021-01-06 13:43 UTC (permalink / raw)
  To: manivannan.sadhasivam, hemantk
  Cc: linux-arm-msm, kuba, davem, netdev, Loic Poulain

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,
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/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 6f80ec3..7512602 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -664,7 +664,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
 		      struct image_info *img_info);
 void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl);
 int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
-			struct mhi_chan *mhi_chan);
+			struct mhi_chan *mhi_chan, enum mhi_chan_flags flags);
 int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
 		       struct mhi_chan *mhi_chan);
 void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl,
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 9b42540..6b6ad6b 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -1292,7 +1292,8 @@ static void __mhi_unprepare_channel(struct mhi_controller *mhi_cntrl,
 }
 
 int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
-			struct mhi_chan *mhi_chan)
+			struct mhi_chan *mhi_chan,
+			enum mhi_chan_flags flags)
 {
 	int ret = 0;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
@@ -1352,6 +1353,9 @@ int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
 	mhi_chan->ch_state = MHI_CH_STATE_ENABLED;
 	write_unlock_irq(&mhi_chan->lock);
 
+	if (mhi_chan->dir == DMA_FROM_DEVICE)
+		mhi_chan->pre_alloc = !!(flags & MHI_CH_INBOUND_ALLOC_BUFS);
+
 	/* Pre-allocate buffer for xfer ring */
 	if (mhi_chan->pre_alloc) {
 		int nr_el = get_nr_avail_ring_elements(mhi_cntrl,
@@ -1498,7 +1502,8 @@ void mhi_reset_chan(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan)
 }
 
 /* Move channel to start state */
-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)
 {
 	int ret, dir;
 	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
@@ -1509,7 +1514,7 @@ int mhi_prepare_for_transfer(struct mhi_device *mhi_dev)
 		if (!mhi_chan)
 			continue;
 
-		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan);
+		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan, flags);
 		if (ret)
 			goto error_open_chan;
 	}
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);
 	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);
 	if (rc)
 		return rc;
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] bus: mhi: Add inbound buffers allocation flag
  2021-01-06 13:43 [PATCH] bus: mhi: Add inbound buffers allocation flag Loic Poulain
@ 2021-01-08 13:44 ` Manivannan Sadhasivam
       [not found]   ` <CAMZdPi9tUUzf0hLwLUBqB=+eGQS-eNP8NtnMF-iS1ZqUfautuw@mail.gmail.com>
  0 siblings, 1 reply; 9+ messages in thread
From: Manivannan Sadhasivam @ 2021-01-08 13:44 UTC (permalink / raw)
  To: Loic Poulain; +Cc: hemantk, linux-arm-msm, kuba, davem, netdev

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
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bus: mhi: Add inbound buffers allocation flag
       [not found]   ` <CAMZdPi9tUUzf0hLwLUBqB=+eGQS-eNP8NtnMF-iS1ZqUfautuw@mail.gmail.com>
@ 2021-01-08 15:30     ` Manivannan Sadhasivam
       [not found]       ` <CAMZdPi_+wHo4q1BQScXALRaTAqNh0zxsgLsri364NvTP1h+6WQ@mail.gmail.com>
  2021-01-09  4:37       ` Hemant Kumar
  0 siblings, 2 replies; 9+ messages in thread
From: Manivannan Sadhasivam @ 2021-01-08 15:30 UTC (permalink / raw)
  To: Loic Poulain
  Cc: Hemant Kumar, linux-arm-msm, Jakub Kicinski, David Miller,
	Network Development

On Fri, Jan 08, 2021 at 03:01:59PM +0100, Loic Poulain wrote:
> Hi Mani,
> 
> On Fri, 8 Jan 2021 at 14:44, Manivannan Sadhasivam <
> manivannan.sadhasivam@linaro.org> wrote:
> 
> > 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"
> >
> 
> I'll fix that.
> 
> 
> > > 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.
> >
> 
> The client driver can not be agnostic if this information is defined on the
> controller side. In one case client driver needs to allocate (and queue)
> its own buffers and in the other case it uses the pre-allocated ones.
> Moreover, that will break compatibility if we have one controller (e.g. a
> Wifi MHI controller) that e.g. defines IPCR channels as pre-allocated and
> another one that defines IPCR channels as non-pre-allocated. Having
> pre-allocated channels is not something related to the MHI device but to
> how the host (client driver) wants to manage buffers. It would then make
> sense to let this choice to the client driver.
> 
> 
> >
> > >       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?
> >
> 
> This is how mhi-qrtr has been implemented, yes.
> 

skb is allocated in qrtr_endpoint_post(). Then how the host can pre
allocate the buffer here? Am I missing something?

Thanks,
Mani

> Regards,
> Loic

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bus: mhi: Add inbound buffers allocation flag
       [not found]       ` <CAMZdPi_+wHo4q1BQScXALRaTAqNh0zxsgLsri364NvTP1h+6WQ@mail.gmail.com>
@ 2021-01-08 17:17         ` Manivannan Sadhasivam
  0 siblings, 0 replies; 9+ messages in thread
From: Manivannan Sadhasivam @ 2021-01-08 17:17 UTC (permalink / raw)
  To: Loic Poulain
  Cc: Hemant Kumar, linux-arm-msm, Jakub Kicinski, David Miller,
	Network Development

On Fri, Jan 08, 2021 at 04:46:49PM +0100, Loic Poulain wrote:
> Hi Mani,
> 
> On Fri, 8 Jan 2021 at 16:30, Manivannan Sadhasivam <
> manivannan.sadhasivam@linaro.org> wrote:
> 
> > > > >       /* 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?
> > > >
> > >
> > > This is how mhi-qrtr has been implemented, yes.
> > >
> >
> > skb is allocated in qrtr_endpoint_post(). Then how the host can pre
> > allocate the buffer here? Am I missing something?
> >
> 
> The initial MHI buffer is pre-allocated by the MHI core, so that mhi-qrtr
> only has to register a dl_callback without having to allocate and queue its
> own buffers. On dl_callback mhi-qrtr calls qrtr_endpoint_post(data) which
> allocates an skb and copy the MHI buffer (data) into that skb. When
> mhi-qrtr dl_callback finishes, the MHI buffer is re-queued automatically by
> the MHI core.
> 

Oops... My bad! There is the "auto_queue" for dl chan. Sorry for the noise.

Thanks,
Mani

> Regards,
> Loic
> 
> 
> >
> > Thanks,
> > Mani
> >
> > > Regards,
> > > Loic
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bus: mhi: Add inbound buffers allocation flag
  2021-01-08 15:30     ` Manivannan Sadhasivam
       [not found]       ` <CAMZdPi_+wHo4q1BQScXALRaTAqNh0zxsgLsri364NvTP1h+6WQ@mail.gmail.com>
@ 2021-01-09  4:37       ` Hemant Kumar
  1 sibling, 0 replies; 9+ messages in thread
From: Hemant Kumar @ 2021-01-09  4:37 UTC (permalink / raw)
  To: Manivannan Sadhasivam, Loic Poulain
  Cc: linux-arm-msm, Jakub Kicinski, David Miller, Network Development



On 1/8/21 7:30 AM, Manivannan Sadhasivam wrote:
> On Fri, Jan 08, 2021 at 03:01:59PM +0100, Loic Poulain wrote:
>> Hi Mani,
>>
>> On Fri, 8 Jan 2021 at 14:44, Manivannan Sadhasivam <
>> manivannan.sadhasivam@linaro.org> wrote:
>>
>>> 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"
>>>
>>
>> I'll fix that.
>>
>>
>>>> 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.

I am fine with his change, and agree that MHI client driver should be 
able to make the decision.
Having said that, can we merge this on top of UCI :).
[..]

Thanks,
Hemant
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bus: mhi: Add inbound buffers allocation flag
  2020-11-09 10:35   ` Manivannan Sadhasivam
@ 2020-11-09 11:52     ` Loic Poulain
  0 siblings, 0 replies; 9+ messages in thread
From: Loic Poulain @ 2020-11-09 11:52 UTC (permalink / raw)
  To: Manivannan Sadhasivam; +Cc: linux-arm-msm, Hemant Kumar

Hi Mani,

On Mon, 9 Nov 2020 at 11:36, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> Hi Loic,
>
> On Tue, Sep 29, 2020 at 03:37:12PM +0200, Loic Poulain wrote:
> > On Wed, 23 Sep 2020 at 17:11, Loic Poulain <loic.poulain@linaro.org> 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,
> > > 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>
> > >
> >
> > Please, discard this one, since it will request ath11k and qrtr update as
> > well.
> > I'll send that in a new series.
> >
>
> Is this part of any series?

I need to rebase and submit a new one.

Regards,
Loic

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bus: mhi: Add inbound buffers allocation flag
       [not found] ` <CAMZdPi9EvP-umfpmS-yTK+5Snx-qDcpfkkq_dtExKZjXG4atMg@mail.gmail.com>
@ 2020-11-09 10:35   ` Manivannan Sadhasivam
  2020-11-09 11:52     ` Loic Poulain
  0 siblings, 1 reply; 9+ messages in thread
From: Manivannan Sadhasivam @ 2020-11-09 10:35 UTC (permalink / raw)
  To: Loic Poulain; +Cc: linux-arm-msm, Hemant Kumar

Hi Loic,

On Tue, Sep 29, 2020 at 03:37:12PM +0200, Loic Poulain wrote:
> On Wed, 23 Sep 2020 at 17:11, Loic Poulain <loic.poulain@linaro.org> 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,
> > 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>
> >
> 
> Please, discard this one, since it will request ath11k and qrtr update as
> well.
> I'll send that in a new series.
> 

Is this part of any series?

Thanks,
Mani

> Regards,
> Loic

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] bus: mhi: Add inbound buffers allocation flag
  2020-09-23 15:16 Loic Poulain
@ 2020-11-06 19:34 ` Bhaumik Bhatt
       [not found] ` <CAMZdPi9EvP-umfpmS-yTK+5Snx-qDcpfkkq_dtExKZjXG4atMg@mail.gmail.com>
  1 sibling, 0 replies; 9+ messages in thread
From: Bhaumik Bhatt @ 2020-11-06 19:34 UTC (permalink / raw)
  To: Loic Poulain; +Cc: manivannan.sadhasivam, hemantk, linux-arm-msm

Hi Loic,

On 2020-09-23 08:16, 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,
> which actually deals with that buffers.
We could say, MHI device driver can be given the option to choose if it
wants to have inbound buffers queued for them during channel preparation
instead of having it pre-defined one-time in their configuration.
> 
> Add a flag parameter to mhi_prepare_for_transfer allowing to specify
mhi_prepare_for_transfer()?
> 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.
> 
I feel there is no need to mention this line here. We can do it as a
follow-up patch to this one or address it later.
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

With some of these suggestions picked up,
Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/internal.h |  2 +-
>  drivers/bus/mhi/core/main.c     | 11 ++++++++---
>  include/linux/mhi.h             | 12 +++++++++++-
>  3 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/internal.h 
> b/drivers/bus/mhi/core/internal.h
> index 33c2320..5b636e2 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -666,7 +666,7 @@ void mhi_rddm_prepare(struct mhi_controller 
> *mhi_cntrl,
>  		      struct image_info *img_info);
>  void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl);
>  int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
> -			struct mhi_chan *mhi_chan);
> +			struct mhi_chan *mhi_chan, enum mhi_chan_flags flags);
>  int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
>  		       struct mhi_chan *mhi_chan);
>  void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl,
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index a44fedb..fb71a0e 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -1274,7 +1274,8 @@ static void __mhi_unprepare_channel(struct
> mhi_controller *mhi_cntrl,
>  }
> 
>  int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
> -			struct mhi_chan *mhi_chan)
> +			struct mhi_chan *mhi_chan,
> +			enum mhi_chan_flags flags)
>  {
>  	int ret = 0;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> @@ -1334,6 +1335,9 @@ int mhi_prepare_channel(struct mhi_controller 
> *mhi_cntrl,
>  	mhi_chan->ch_state = MHI_CH_STATE_ENABLED;
>  	write_unlock_irq(&mhi_chan->lock);
> 
> +	if (mhi_chan->dir == DMA_FROM_DEVICE)
> +		mhi_chan->pre_alloc = !!(flags & MHI_CH_INBOUND_ALLOC_BUFS);
> +
>  	/* Pre-allocate buffer for xfer ring */
>  	if (mhi_chan->pre_alloc) {
>  		int nr_el = get_nr_avail_ring_elements(mhi_cntrl,
> @@ -1480,7 +1484,8 @@ void mhi_reset_chan(struct mhi_controller
> *mhi_cntrl, struct mhi_chan *mhi_chan)
>  }
> 
>  /* Move channel to start state */
> -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)
>  {
>  	int ret, dir;
>  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> @@ -1491,7 +1496,7 @@ int mhi_prepare_for_transfer(struct mhi_device 
> *mhi_dev)
>  		if (!mhi_chan)
>  			continue;
> 
> -		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan);
> +		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan, flags);
>  		if (ret)
>  			goto error_open_chan;
>  	}
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 6522a4a..9d67e75 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
> @@ -684,8 +692,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

Thanks,
Bhaumik
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] bus: mhi: Add inbound buffers allocation flag
@ 2020-09-23 15:16 Loic Poulain
  2020-11-06 19:34 ` Bhaumik Bhatt
       [not found] ` <CAMZdPi9EvP-umfpmS-yTK+5Snx-qDcpfkkq_dtExKZjXG4atMg@mail.gmail.com>
  0 siblings, 2 replies; 9+ messages in thread
From: Loic Poulain @ 2020-09-23 15:16 UTC (permalink / raw)
  To: manivannan.sadhasivam, hemantk; +Cc: linux-arm-msm, Loic Poulain

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,
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 ++++++++---
 include/linux/mhi.h             | 12 +++++++++++-
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 33c2320..5b636e2 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -666,7 +666,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
 		      struct image_info *img_info);
 void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl);
 int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
-			struct mhi_chan *mhi_chan);
+			struct mhi_chan *mhi_chan, enum mhi_chan_flags flags);
 int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
 		       struct mhi_chan *mhi_chan);
 void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl,
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index a44fedb..fb71a0e 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -1274,7 +1274,8 @@ static void __mhi_unprepare_channel(struct mhi_controller *mhi_cntrl,
 }
 
 int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
-			struct mhi_chan *mhi_chan)
+			struct mhi_chan *mhi_chan,
+			enum mhi_chan_flags flags)
 {
 	int ret = 0;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
@@ -1334,6 +1335,9 @@ int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
 	mhi_chan->ch_state = MHI_CH_STATE_ENABLED;
 	write_unlock_irq(&mhi_chan->lock);
 
+	if (mhi_chan->dir == DMA_FROM_DEVICE)
+		mhi_chan->pre_alloc = !!(flags & MHI_CH_INBOUND_ALLOC_BUFS);
+
 	/* Pre-allocate buffer for xfer ring */
 	if (mhi_chan->pre_alloc) {
 		int nr_el = get_nr_avail_ring_elements(mhi_cntrl,
@@ -1480,7 +1484,8 @@ void mhi_reset_chan(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan)
 }
 
 /* Move channel to start state */
-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)
 {
 	int ret, dir;
 	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
@@ -1491,7 +1496,7 @@ int mhi_prepare_for_transfer(struct mhi_device *mhi_dev)
 		if (!mhi_chan)
 			continue;
 
-		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan);
+		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan, flags);
 		if (ret)
 			goto error_open_chan;
 	}
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 6522a4a..9d67e75 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
@@ -684,8 +692,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
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-01-09  4:38 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-06 13:43 [PATCH] bus: mhi: Add inbound buffers allocation flag Loic Poulain
2021-01-08 13:44 ` Manivannan Sadhasivam
     [not found]   ` <CAMZdPi9tUUzf0hLwLUBqB=+eGQS-eNP8NtnMF-iS1ZqUfautuw@mail.gmail.com>
2021-01-08 15:30     ` Manivannan Sadhasivam
     [not found]       ` <CAMZdPi_+wHo4q1BQScXALRaTAqNh0zxsgLsri364NvTP1h+6WQ@mail.gmail.com>
2021-01-08 17:17         ` Manivannan Sadhasivam
2021-01-09  4:37       ` Hemant Kumar
  -- strict thread matches above, loose matches on Subject: below --
2020-09-23 15:16 Loic Poulain
2020-11-06 19:34 ` Bhaumik Bhatt
     [not found] ` <CAMZdPi9EvP-umfpmS-yTK+5Snx-qDcpfkkq_dtExKZjXG4atMg@mail.gmail.com>
2020-11-09 10:35   ` Manivannan Sadhasivam
2020-11-09 11:52     ` Loic Poulain

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.