[RFC,v2,2/8] bus: mhi: host: add new interfaces to handle MHI channels directly

From: Baochen Qiang <quic_bqiang@quicinc.com>

When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
by themselves.  Similarly, MHI stack will also not create new MHI device since
old devices were not destroyed, so MHI hosts need to prepare channels as well.
Hence add these two interfaces to make that possible.

Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30

Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
 include/linux/mhi.h         |  20 ++++++-
 2 files changed, 126 insertions(+), 1 deletion(-)

On Mon, Nov 27, 2023 at 06:20:16PM +0200, Kalle Valo wrote:
> From: Baochen Qiang <quic_bqiang@quicinc.com>
> 
> When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
> by themselves.  Similarly, MHI stack will also not create new MHI device since
> old devices were not destroyed, so MHI hosts need to prepare channels as well.
> Hence add these two interfaces to make that possible.
> 
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
> 
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
>  drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
>  include/linux/mhi.h         |  20 ++++++-
>  2 files changed, 126 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index d80975f4bba8..3f677fc628ad 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -1669,6 +1669,58 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
>  }
>  EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
>  
> +static int ____mhi_prepare_for_transfer(struct device *dev, void *data)

"__mhi_prepare_all_for_transfer"

> +{
> +	struct mhi_device *mhi_dev;
> +	struct mhi_chan *ul_chan, *dl_chan;
> +	enum mhi_ee_type ee = MHI_EE_MAX;

Reverse Xmas order, please.

> +
> +	if (dev->bus != &mhi_bus_type)
> +		return 0;
> +
> +	mhi_dev = to_mhi_device(dev);
> +
> +	/* Only prepare virtual devices that are attached to bus */

"Only prepare virtual devices for the channels". Here and below.

> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> +		return 0;
> +
> +	/* There are cases where there is no MHI client driver matches
> +	 * this device, we are not allowed to do prepare for it.
> +	 */

Use the preferred style for comment:

	/*
	 * ...
	 */

> +	if (!mhi_dev->id)
> +		return 0;
> +
> +	ul_chan = mhi_dev->ul_chan;
> +	dl_chan = mhi_dev->dl_chan;
> +
> +	/*
> +	 * If execution environment is specified, remove only those devices that
> +	 * started in them based on ee_mask for the channels as we move on to a
> +	 * different execution environment
> +	 */
> +	if (data)
> +		ee = *(enum mhi_ee_type *)data;
> +
> +	if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
> +		return 0;
> +
> +

Remove extra newline.

> +	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
> +		return 0;
> +
> +	if (dl_chan->pre_alloc)
> +		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
> +	else
> +		return mhi_prepare_for_transfer(mhi_dev);
> +}
> +
> +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl)
> +{
> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
> +				     ____mhi_prepare_for_transfer);
> +}
> +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
> +
>  void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>  {
>  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> @@ -1684,3 +1736,58 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>  	}
>  }
>  EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
> +
> +static int ____mhi_unprepare_from_transfer(struct device *dev, void *data)

__mhi_unprepare_all_from_transfer

> +{
> +	struct mhi_device *mhi_dev;
> +	struct mhi_chan *ul_chan, *dl_chan;
> +	enum mhi_ee_type ee = MHI_EE_MAX;
> +
> +	if (dev->bus != &mhi_bus_type)
> +		return 0;
> +
> +	mhi_dev = to_mhi_device(dev);
> +
> +	/* Only unprepare virtual devices that are attached to bus */
> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> +		return 0;
> +
> +	/* There are cases where there is no MHI client driver matches
> +	 * this device, so it is not probed or prepared, no need to
> +	 * do unprepare for it.
> +	 */
> +	if (!mhi_dev->id)
> +		return 0;
> +
> +	ul_chan = mhi_dev->ul_chan;
> +	dl_chan = mhi_dev->dl_chan;
> +
> +	/*
> +	 * If execution environment is specified, remove only those devices that
> +	 * started in them based on ee_mask for the channels as we move on to a
> +	 * different execution environment
> +	 */
> +	if (data)
> +		ee = *(enum mhi_ee_type *)data;
> +
> +	if (ul_chan) {
> +		if (ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
> +			return 0;
> +	}
> +
> +	if (dl_chan) {
> +		if (ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
> +			return 0;
> +	}
> +
> +	mhi_unprepare_from_transfer(mhi_dev);
> +
> +	return 0;
> +}
> +
> +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl)
> +{
> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
> +				     ____mhi_unprepare_from_transfer);
> +}
> +EXPORT_SYMBOL_GPL(mhi_unprepare_all_from_transfer);
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index ae092bc8b97e..dcf62a57056a 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
>   * destroy struct devices. This is a variant for mhi_power_down() and is a
>   * workaround to make it possible to use mhi_power_up() in a resume
>   * handler. When using this variant the caller must also call
> - * mhi_prepare_all_for_transfer_autoqueue() and
> + * mhi_prepare_all_for_transfer() and

This change belongs to previous patch.

>   * mhi_unprepare_all_from_transfer().
>   *
>   * @mhi_cntrl: MHI controller
> @@ -842,4 +842,22 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>   */
>  bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
>  
> +/**
> + * mhi_prepare_all_for_transfer - if you are using
> + * mhi_power_down_no_destroy() variant this needs to be called after
> + * calling mhi_power_up().

Add info about what this API does also.

> + *
> + * @mhi_cntrl: MHI controller
> + */
> +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl);
> +
> +/**
> + * mhi_unprepare_all_from_transfer - if you are using
> + * mhi_power_down_no_destroy() variant this function needs to be called
> + * before calling mhi_power_down_no_destroy().

Same as above.

- Mani

> + *
> + * @mhi_cntrl: MHI controller
> + */
> +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
> +
>  #endif /* _MHI_H_ */
> -- 
> 2.39.2
> 
>

On 1/31/2024 2:19 AM, Manivannan Sadhasivam wrote:
> On Mon, Nov 27, 2023 at 06:20:16PM +0200, Kalle Valo wrote:
>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>
>> When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
>> by themselves.  Similarly, MHI stack will also not create new MHI device since
>> old devices were not destroyed, so MHI hosts need to prepare channels as well.
>> Hence add these two interfaces to make that possible.
>>
>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>>
>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> ---
>>   drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
>>   include/linux/mhi.h         |  20 ++++++-
>>   2 files changed, 126 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
>> index d80975f4bba8..3f677fc628ad 100644
>> --- a/drivers/bus/mhi/host/main.c
>> +++ b/drivers/bus/mhi/host/main.c
>> @@ -1669,6 +1669,58 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
>>   }
>>   EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
>>   
>> +static int ____mhi_prepare_for_transfer(struct device *dev, void *data)
> 
> "__mhi_prepare_all_for_transfer"

This is to prepare one single child device, I don't think a name like 
__mhi_prepare_all_for_transfer (with 'all' inside) make sense, right?
How about changing to "mhi_prepare_dev_for_transfer" or 
"mhi_prepare_single_for_transfer"?

> 
>> +{
>> +	struct mhi_device *mhi_dev;
>> +	struct mhi_chan *ul_chan, *dl_chan;
>> +	enum mhi_ee_type ee = MHI_EE_MAX;
> 
> Reverse Xmas order, please.
> 
>> +
>> +	if (dev->bus != &mhi_bus_type)
>> +		return 0;
>> +
>> +	mhi_dev = to_mhi_device(dev);
>> +
>> +	/* Only prepare virtual devices that are attached to bus */
> 
> "Only prepare virtual devices for the channels". Here and below.
> 
>> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>> +		return 0;
>> +
>> +	/* There are cases where there is no MHI client driver matches
>> +	 * this device, we are not allowed to do prepare for it.
>> +	 */
> 
> Use the preferred style for comment:
> 
> 	/*
> 	 * ...
> 	 */
> 
>> +	if (!mhi_dev->id)
>> +		return 0;
>> +
>> +	ul_chan = mhi_dev->ul_chan;
>> +	dl_chan = mhi_dev->dl_chan;
>> +
>> +	/*
>> +	 * If execution environment is specified, remove only those devices that
>> +	 * started in them based on ee_mask for the channels as we move on to a
>> +	 * different execution environment
>> +	 */
>> +	if (data)
>> +		ee = *(enum mhi_ee_type *)data;
>> +
>> +	if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>> +		return 0;
>> +
>> +
> 
> Remove extra newline.
> 
>> +	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>> +		return 0;
>> +
>> +	if (dl_chan->pre_alloc)
>> +		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
>> +	else
>> +		return mhi_prepare_for_transfer(mhi_dev);
>> +}
>> +
>> +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl)
>> +{
>> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>> +				     ____mhi_prepare_for_transfer);
>> +}
>> +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
>> +
>>   void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>>   {
>>   	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>> @@ -1684,3 +1736,58 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>>   	}
>>   }
>>   EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
>> +
>> +static int ____mhi_unprepare_from_transfer(struct device *dev, void *data)
> 
> __mhi_unprepare_all_from_transfer

same as above.

> 
>> +{
>> +	struct mhi_device *mhi_dev;
>> +	struct mhi_chan *ul_chan, *dl_chan;
>> +	enum mhi_ee_type ee = MHI_EE_MAX;
>> +
>> +	if (dev->bus != &mhi_bus_type)
>> +		return 0;
>> +
>> +	mhi_dev = to_mhi_device(dev);
>> +
>> +	/* Only unprepare virtual devices that are attached to bus */
>> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>> +		return 0;
>> +
>> +	/* There are cases where there is no MHI client driver matches
>> +	 * this device, so it is not probed or prepared, no need to
>> +	 * do unprepare for it.
>> +	 */
>> +	if (!mhi_dev->id)
>> +		return 0;
>> +
>> +	ul_chan = mhi_dev->ul_chan;
>> +	dl_chan = mhi_dev->dl_chan;
>> +
>> +	/*
>> +	 * If execution environment is specified, remove only those devices that
>> +	 * started in them based on ee_mask for the channels as we move on to a
>> +	 * different execution environment
>> +	 */
>> +	if (data)
>> +		ee = *(enum mhi_ee_type *)data;
>> +
>> +	if (ul_chan) {
>> +		if (ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>> +			return 0;
>> +	}
>> +
>> +	if (dl_chan) {
>> +		if (ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>> +			return 0;
>> +	}
>> +
>> +	mhi_unprepare_from_transfer(mhi_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl)
>> +{
>> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>> +				     ____mhi_unprepare_from_transfer);
>> +}
>> +EXPORT_SYMBOL_GPL(mhi_unprepare_all_from_transfer);
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index ae092bc8b97e..dcf62a57056a 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
>>    * destroy struct devices. This is a variant for mhi_power_down() and is a
>>    * workaround to make it possible to use mhi_power_up() in a resume
>>    * handler. When using this variant the caller must also call
>> - * mhi_prepare_all_for_transfer_autoqueue() and
>> + * mhi_prepare_all_for_transfer() and
> 
> This change belongs to previous patch.
> 
>>    * mhi_unprepare_all_from_transfer().
>>    *
>>    * @mhi_cntrl: MHI controller
>> @@ -842,4 +842,22 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>>    */
>>   bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
>>   
>> +/**
>> + * mhi_prepare_all_for_transfer - if you are using
>> + * mhi_power_down_no_destroy() variant this needs to be called after
>> + * calling mhi_power_up().
> 
> Add info about what this API does also.
> 
>> + *
>> + * @mhi_cntrl: MHI controller
>> + */
>> +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl);
>> +
>> +/**
>> + * mhi_unprepare_all_from_transfer - if you are using
>> + * mhi_power_down_no_destroy() variant this function needs to be called
>> + * before calling mhi_power_down_no_destroy().
> 
> Same as above.
> 
> - Mani
> 
>> + *
>> + * @mhi_cntrl: MHI controller
>> + */
>> +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
>> +
>>   #endif /* _MHI_H_ */
>> -- 
>> 2.39.2
>>
>>
>

On Wed, Jan 31, 2024 at 03:39:26PM +0800, Baochen Qiang wrote:
> 
> 
> On 1/31/2024 2:19 AM, Manivannan Sadhasivam wrote:
> > On Mon, Nov 27, 2023 at 06:20:16PM +0200, Kalle Valo wrote:
> > > From: Baochen Qiang <quic_bqiang@quicinc.com>
> > > 
> > > When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
> > > by themselves.  Similarly, MHI stack will also not create new MHI device since
> > > old devices were not destroyed, so MHI hosts need to prepare channels as well.
> > > Hence add these two interfaces to make that possible.
> > > 
> > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
> > > 
> > > Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> > > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> > > ---
> > >   drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
> > >   include/linux/mhi.h         |  20 ++++++-
> > >   2 files changed, 126 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> > > index d80975f4bba8..3f677fc628ad 100644
> > > --- a/drivers/bus/mhi/host/main.c
> > > +++ b/drivers/bus/mhi/host/main.c
> > > @@ -1669,6 +1669,58 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
> > >   }
> > >   EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
> > > +static int ____mhi_prepare_for_transfer(struct device *dev, void *data)
> > 
> > "__mhi_prepare_all_for_transfer"
> 
> This is to prepare one single child device, I don't think a name like
> __mhi_prepare_all_for_transfer (with 'all' inside) make sense, right?
> How about changing to "mhi_prepare_dev_for_transfer" or
> "mhi_prepare_single_for_transfer"?
> 

I think most of the checks in this function can be moved inside
mhi_prepare_for_transfer() API. With that you can just reuse the API without
adding a new helper.

For autoqueue channels, you can add another API
mhi_prepare_all_for_transfer_autoqueue() just like
mhi_prepare_for_transfer_autoqueue() to maintain uniformity.

- Mani

> > 
> > > +{
> > > +	struct mhi_device *mhi_dev;
> > > +	struct mhi_chan *ul_chan, *dl_chan;
> > > +	enum mhi_ee_type ee = MHI_EE_MAX;
> > 
> > Reverse Xmas order, please.
> > 
> > > +
> > > +	if (dev->bus != &mhi_bus_type)
> > > +		return 0;
> > > +
> > > +	mhi_dev = to_mhi_device(dev);
> > > +
> > > +	/* Only prepare virtual devices that are attached to bus */
> > 
> > "Only prepare virtual devices for the channels". Here and below.
> > 
> > > +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> > > +		return 0;
> > > +
> > > +	/* There are cases where there is no MHI client driver matches
> > > +	 * this device, we are not allowed to do prepare for it.
> > > +	 */
> > 
> > Use the preferred style for comment:
> > 
> > 	/*
> > 	 * ...
> > 	 */
> > 
> > > +	if (!mhi_dev->id)
> > > +		return 0;
> > > +
> > > +	ul_chan = mhi_dev->ul_chan;
> > > +	dl_chan = mhi_dev->dl_chan;
> > > +
> > > +	/*
> > > +	 * If execution environment is specified, remove only those devices that
> > > +	 * started in them based on ee_mask for the channels as we move on to a
> > > +	 * different execution environment
> > > +	 */
> > > +	if (data)
> > > +		ee = *(enum mhi_ee_type *)data;
> > > +
> > > +	if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
> > > +		return 0;
> > > +
> > > +
> > 
> > Remove extra newline.
> > 
> > > +	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
> > > +		return 0;
> > > +
> > > +	if (dl_chan->pre_alloc)
> > > +		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
> > > +	else
> > > +		return mhi_prepare_for_transfer(mhi_dev);
> > > +}
> > > +
> > > +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl)
> > > +{
> > > +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
> > > +				     ____mhi_prepare_for_transfer);
> > > +}
> > > +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
> > > +
> > >   void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
> > >   {
> > >   	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> > > @@ -1684,3 +1736,58 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
> > >   	}
> > >   }
> > >   EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
> > > +
> > > +static int ____mhi_unprepare_from_transfer(struct device *dev, void *data)
> > 
> > __mhi_unprepare_all_from_transfer
> 
> same as above.
> 
> > 
> > > +{
> > > +	struct mhi_device *mhi_dev;
> > > +	struct mhi_chan *ul_chan, *dl_chan;
> > > +	enum mhi_ee_type ee = MHI_EE_MAX;
> > > +
> > > +	if (dev->bus != &mhi_bus_type)
> > > +		return 0;
> > > +
> > > +	mhi_dev = to_mhi_device(dev);
> > > +
> > > +	/* Only unprepare virtual devices that are attached to bus */
> > > +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> > > +		return 0;
> > > +
> > > +	/* There are cases where there is no MHI client driver matches
> > > +	 * this device, so it is not probed or prepared, no need to
> > > +	 * do unprepare for it.
> > > +	 */
> > > +	if (!mhi_dev->id)
> > > +		return 0;
> > > +
> > > +	ul_chan = mhi_dev->ul_chan;
> > > +	dl_chan = mhi_dev->dl_chan;
> > > +
> > > +	/*
> > > +	 * If execution environment is specified, remove only those devices that
> > > +	 * started in them based on ee_mask for the channels as we move on to a
> > > +	 * different execution environment
> > > +	 */
> > > +	if (data)
> > > +		ee = *(enum mhi_ee_type *)data;
> > > +
> > > +	if (ul_chan) {
> > > +		if (ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
> > > +			return 0;
> > > +	}
> > > +
> > > +	if (dl_chan) {
> > > +		if (ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
> > > +			return 0;
> > > +	}
> > > +
> > > +	mhi_unprepare_from_transfer(mhi_dev);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl)
> > > +{
> > > +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
> > > +				     ____mhi_unprepare_from_transfer);
> > > +}
> > > +EXPORT_SYMBOL_GPL(mhi_unprepare_all_from_transfer);
> > > diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> > > index ae092bc8b97e..dcf62a57056a 100644
> > > --- a/include/linux/mhi.h
> > > +++ b/include/linux/mhi.h
> > > @@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
> > >    * destroy struct devices. This is a variant for mhi_power_down() and is a
> > >    * workaround to make it possible to use mhi_power_up() in a resume
> > >    * handler. When using this variant the caller must also call
> > > - * mhi_prepare_all_for_transfer_autoqueue() and
> > > + * mhi_prepare_all_for_transfer() and
> > 
> > This change belongs to previous patch.
> > 
> > >    * mhi_unprepare_all_from_transfer().
> > >    *
> > >    * @mhi_cntrl: MHI controller
> > > @@ -842,4 +842,22 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
> > >    */
> > >   bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
> > > +/**
> > > + * mhi_prepare_all_for_transfer - if you are using
> > > + * mhi_power_down_no_destroy() variant this needs to be called after
> > > + * calling mhi_power_up().
> > 
> > Add info about what this API does also.
> > 
> > > + *
> > > + * @mhi_cntrl: MHI controller
> > > + */
> > > +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl);
> > > +
> > > +/**
> > > + * mhi_unprepare_all_from_transfer - if you are using
> > > + * mhi_power_down_no_destroy() variant this function needs to be called
> > > + * before calling mhi_power_down_no_destroy().
> > 
> > Same as above.
> > 
> > - Mani
> > 
> > > + *
> > > + * @mhi_cntrl: MHI controller
> > > + */
> > > +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
> > > +
> > >   #endif /* _MHI_H_ */
> > > -- 
> > > 2.39.2
> > > 
> > > 
> >

On 2/1/2024 6:00 PM, Manivannan Sadhasivam wrote:
> On Wed, Jan 31, 2024 at 03:39:26PM +0800, Baochen Qiang wrote:
>>
>>
>> On 1/31/2024 2:19 AM, Manivannan Sadhasivam wrote:
>>> On Mon, Nov 27, 2023 at 06:20:16PM +0200, Kalle Valo wrote:
>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>
>>>> When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
>>>> by themselves.  Similarly, MHI stack will also not create new MHI device since
>>>> old devices were not destroyed, so MHI hosts need to prepare channels as well.
>>>> Hence add these two interfaces to make that possible.
>>>>
>>>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>>>>
>>>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
>>>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>>>> ---
>>>>    drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
>>>>    include/linux/mhi.h         |  20 ++++++-
>>>>    2 files changed, 126 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
>>>> index d80975f4bba8..3f677fc628ad 100644
>>>> --- a/drivers/bus/mhi/host/main.c
>>>> +++ b/drivers/bus/mhi/host/main.c
>>>> @@ -1669,6 +1669,58 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
>>>> +static int ____mhi_prepare_for_transfer(struct device *dev, void *data)
>>>
>>> "__mhi_prepare_all_for_transfer"
>>
>> This is to prepare one single child device, I don't think a name like
>> __mhi_prepare_all_for_transfer (with 'all' inside) make sense, right?
>> How about changing to "mhi_prepare_dev_for_transfer" or
>> "mhi_prepare_single_for_transfer"?
>>
> 
> I think most of the checks in this function can be moved inside
> mhi_prepare_for_transfer() API. With that you can just reuse the API without
> adding a new helper.
> 
> For autoqueue channels, you can add another API
> mhi_prepare_all_for_transfer_autoqueue() just like
> mhi_prepare_for_transfer_autoqueue() to maintain uniformity.
> 
> - Mani
If we do that, we need to call two APIs together, does it make sense? 
 From the view of an MHI user, what we want is an API to prepare all 
channels, no matter whether a channel is configured as autoqueue or 
non-autoqueue, we don't care about it.

And besides, I don't think there is a scenario where we need to use them 
separately. So if we always need to use them together, why not merge 
them in a single API?

> 
>>>
>>>> +{
>>>> +	struct mhi_device *mhi_dev;
>>>> +	struct mhi_chan *ul_chan, *dl_chan;
>>>> +	enum mhi_ee_type ee = MHI_EE_MAX;
>>>
>>> Reverse Xmas order, please.
>>>
>>>> +
>>>> +	if (dev->bus != &mhi_bus_type)
>>>> +		return 0;
>>>> +
>>>> +	mhi_dev = to_mhi_device(dev);
>>>> +
>>>> +	/* Only prepare virtual devices that are attached to bus */
>>>
>>> "Only prepare virtual devices for the channels". Here and below.
>>>
>>>> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>>>> +		return 0;
>>>> +
>>>> +	/* There are cases where there is no MHI client driver matches
>>>> +	 * this device, we are not allowed to do prepare for it.
>>>> +	 */
>>>
>>> Use the preferred style for comment:
>>>
>>> 	/*
>>> 	 * ...
>>> 	 */
>>>
>>>> +	if (!mhi_dev->id)
>>>> +		return 0;
>>>> +
>>>> +	ul_chan = mhi_dev->ul_chan;
>>>> +	dl_chan = mhi_dev->dl_chan;
>>>> +
>>>> +	/*
>>>> +	 * If execution environment is specified, remove only those devices that
>>>> +	 * started in them based on ee_mask for the channels as we move on to a
>>>> +	 * different execution environment
>>>> +	 */
>>>> +	if (data)
>>>> +		ee = *(enum mhi_ee_type *)data;
>>>> +
>>>> +	if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>>>> +		return 0;
>>>> +
>>>> +
>>>
>>> Remove extra newline.
>>>
>>>> +	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>>>> +		return 0;
>>>> +
>>>> +	if (dl_chan->pre_alloc)
>>>> +		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
>>>> +	else
>>>> +		return mhi_prepare_for_transfer(mhi_dev);
>>>> +}
>>>> +
>>>> +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl)
>>>> +{
>>>> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>>>> +				     ____mhi_prepare_for_transfer);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
>>>> +
>>>>    void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>>>>    {
>>>>    	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>>>> @@ -1684,3 +1736,58 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>>>>    	}
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
>>>> +
>>>> +static int ____mhi_unprepare_from_transfer(struct device *dev, void *data)
>>>
>>> __mhi_unprepare_all_from_transfer
>>
>> same as above.
>>
>>>
>>>> +{
>>>> +	struct mhi_device *mhi_dev;
>>>> +	struct mhi_chan *ul_chan, *dl_chan;
>>>> +	enum mhi_ee_type ee = MHI_EE_MAX;
>>>> +
>>>> +	if (dev->bus != &mhi_bus_type)
>>>> +		return 0;
>>>> +
>>>> +	mhi_dev = to_mhi_device(dev);
>>>> +
>>>> +	/* Only unprepare virtual devices that are attached to bus */
>>>> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>>>> +		return 0;
>>>> +
>>>> +	/* There are cases where there is no MHI client driver matches
>>>> +	 * this device, so it is not probed or prepared, no need to
>>>> +	 * do unprepare for it.
>>>> +	 */
>>>> +	if (!mhi_dev->id)
>>>> +		return 0;
>>>> +
>>>> +	ul_chan = mhi_dev->ul_chan;
>>>> +	dl_chan = mhi_dev->dl_chan;
>>>> +
>>>> +	/*
>>>> +	 * If execution environment is specified, remove only those devices that
>>>> +	 * started in them based on ee_mask for the channels as we move on to a
>>>> +	 * different execution environment
>>>> +	 */
>>>> +	if (data)
>>>> +		ee = *(enum mhi_ee_type *)data;
>>>> +
>>>> +	if (ul_chan) {
>>>> +		if (ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>>>> +			return 0;
>>>> +	}
>>>> +
>>>> +	if (dl_chan) {
>>>> +		if (ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>>>> +			return 0;
>>>> +	}
>>>> +
>>>> +	mhi_unprepare_from_transfer(mhi_dev);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl)
>>>> +{
>>>> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>>>> +				     ____mhi_unprepare_from_transfer);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(mhi_unprepare_all_from_transfer);
>>>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>>>> index ae092bc8b97e..dcf62a57056a 100644
>>>> --- a/include/linux/mhi.h
>>>> +++ b/include/linux/mhi.h
>>>> @@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
>>>>     * destroy struct devices. This is a variant for mhi_power_down() and is a
>>>>     * workaround to make it possible to use mhi_power_up() in a resume
>>>>     * handler. When using this variant the caller must also call
>>>> - * mhi_prepare_all_for_transfer_autoqueue() and
>>>> + * mhi_prepare_all_for_transfer() and
>>>
>>> This change belongs to previous patch.
>>>
>>>>     * mhi_unprepare_all_from_transfer().
>>>>     *
>>>>     * @mhi_cntrl: MHI controller
>>>> @@ -842,4 +842,22 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>>>>     */
>>>>    bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
>>>> +/**
>>>> + * mhi_prepare_all_for_transfer - if you are using
>>>> + * mhi_power_down_no_destroy() variant this needs to be called after
>>>> + * calling mhi_power_up().
>>>
>>> Add info about what this API does also.
>>>
>>>> + *
>>>> + * @mhi_cntrl: MHI controller
>>>> + */
>>>> +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl);
>>>> +
>>>> +/**
>>>> + * mhi_unprepare_all_from_transfer - if you are using
>>>> + * mhi_power_down_no_destroy() variant this function needs to be called
>>>> + * before calling mhi_power_down_no_destroy().
>>>
>>> Same as above.
>>>
>>> - Mani
>>>
>>>> + *
>>>> + * @mhi_cntrl: MHI controller
>>>> + */
>>>> +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
>>>> +
>>>>    #endif /* _MHI_H_ */
>>>> -- 
>>>> 2.39.2
>>>>
>>>>
>>>
>

On Fri, Feb 02, 2024 at 02:42:58PM +0800, Baochen Qiang wrote:
> 
> 
> On 2/1/2024 6:00 PM, Manivannan Sadhasivam wrote:
> > On Wed, Jan 31, 2024 at 03:39:26PM +0800, Baochen Qiang wrote:
> > > 
> > > 
> > > On 1/31/2024 2:19 AM, Manivannan Sadhasivam wrote:
> > > > On Mon, Nov 27, 2023 at 06:20:16PM +0200, Kalle Valo wrote:
> > > > > From: Baochen Qiang <quic_bqiang@quicinc.com>
> > > > > 
> > > > > When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
> > > > > by themselves.  Similarly, MHI stack will also not create new MHI device since
> > > > > old devices were not destroyed, so MHI hosts need to prepare channels as well.
> > > > > Hence add these two interfaces to make that possible.
> > > > > 
> > > > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
> > > > > 
> > > > > Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> > > > > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> > > > > ---
> > > > >    drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
> > > > >    include/linux/mhi.h         |  20 ++++++-
> > > > >    2 files changed, 126 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> > > > > index d80975f4bba8..3f677fc628ad 100644
> > > > > --- a/drivers/bus/mhi/host/main.c
> > > > > +++ b/drivers/bus/mhi/host/main.c
> > > > > @@ -1669,6 +1669,58 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
> > > > > +static int ____mhi_prepare_for_transfer(struct device *dev, void *data)
> > > > 
> > > > "__mhi_prepare_all_for_transfer"
> > > 
> > > This is to prepare one single child device, I don't think a name like
> > > __mhi_prepare_all_for_transfer (with 'all' inside) make sense, right?
> > > How about changing to "mhi_prepare_dev_for_transfer" or
> > > "mhi_prepare_single_for_transfer"?
> > > 
> > 
> > I think most of the checks in this function can be moved inside
> > mhi_prepare_for_transfer() API. With that you can just reuse the API without
> > adding a new helper.
> > 
> > For autoqueue channels, you can add another API
> > mhi_prepare_all_for_transfer_autoqueue() just like
> > mhi_prepare_for_transfer_autoqueue() to maintain uniformity.
> > 
> > - Mani
> If we do that, we need to call two APIs together, does it make sense? From
> the view of an MHI user, what we want is an API to prepare all channels, no
> matter whether a channel is configured as autoqueue or non-autoqueue, we
> don't care about it.
> 

You are calling this API from a wrong place first up.
mhi_{prepare/unprepare}_transfer* APIs are meant to be used by the client
drivers like QRTR. Controller drivers should not call them.

What you need here is the hibernation support for QRTR itself and call these
APIs from there.

> And besides, I don't think there is a scenario where we need to use them
> separately. So if we always need to use them together, why not merge them in
> a single API?
> 

A single controller driver may expose multiple channels and those will bind to
multiple client drivers. So only the client drivers should manage the channels,
not the controller drivers themselves.

- Mani

> > 
> > > > 
> > > > > +{
> > > > > +	struct mhi_device *mhi_dev;
> > > > > +	struct mhi_chan *ul_chan, *dl_chan;
> > > > > +	enum mhi_ee_type ee = MHI_EE_MAX;
> > > > 
> > > > Reverse Xmas order, please.
> > > > 
> > > > > +
> > > > > +	if (dev->bus != &mhi_bus_type)
> > > > > +		return 0;
> > > > > +
> > > > > +	mhi_dev = to_mhi_device(dev);
> > > > > +
> > > > > +	/* Only prepare virtual devices that are attached to bus */
> > > > 
> > > > "Only prepare virtual devices for the channels". Here and below.
> > > > 
> > > > > +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> > > > > +		return 0;
> > > > > +
> > > > > +	/* There are cases where there is no MHI client driver matches
> > > > > +	 * this device, we are not allowed to do prepare for it.
> > > > > +	 */
> > > > 
> > > > Use the preferred style for comment:
> > > > 
> > > > 	/*
> > > > 	 * ...
> > > > 	 */
> > > > 
> > > > > +	if (!mhi_dev->id)
> > > > > +		return 0;
> > > > > +
> > > > > +	ul_chan = mhi_dev->ul_chan;
> > > > > +	dl_chan = mhi_dev->dl_chan;
> > > > > +
> > > > > +	/*
> > > > > +	 * If execution environment is specified, remove only those devices that
> > > > > +	 * started in them based on ee_mask for the channels as we move on to a
> > > > > +	 * different execution environment
> > > > > +	 */
> > > > > +	if (data)
> > > > > +		ee = *(enum mhi_ee_type *)data;
> > > > > +
> > > > > +	if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
> > > > > +		return 0;
> > > > > +
> > > > > +
> > > > 
> > > > Remove extra newline.
> > > > 
> > > > > +	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
> > > > > +		return 0;
> > > > > +
> > > > > +	if (dl_chan->pre_alloc)
> > > > > +		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
> > > > > +	else
> > > > > +		return mhi_prepare_for_transfer(mhi_dev);
> > > > > +}
> > > > > +
> > > > > +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl)
> > > > > +{
> > > > > +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
> > > > > +				     ____mhi_prepare_for_transfer);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
> > > > > +
> > > > >    void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
> > > > >    {
> > > > >    	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> > > > > @@ -1684,3 +1736,58 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
> > > > >    	}
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
> > > > > +
> > > > > +static int ____mhi_unprepare_from_transfer(struct device *dev, void *data)
> > > > 
> > > > __mhi_unprepare_all_from_transfer
> > > 
> > > same as above.
> > > 
> > > > 
> > > > > +{
> > > > > +	struct mhi_device *mhi_dev;
> > > > > +	struct mhi_chan *ul_chan, *dl_chan;
> > > > > +	enum mhi_ee_type ee = MHI_EE_MAX;
> > > > > +
> > > > > +	if (dev->bus != &mhi_bus_type)
> > > > > +		return 0;
> > > > > +
> > > > > +	mhi_dev = to_mhi_device(dev);
> > > > > +
> > > > > +	/* Only unprepare virtual devices that are attached to bus */
> > > > > +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> > > > > +		return 0;
> > > > > +
> > > > > +	/* There are cases where there is no MHI client driver matches
> > > > > +	 * this device, so it is not probed or prepared, no need to
> > > > > +	 * do unprepare for it.
> > > > > +	 */
> > > > > +	if (!mhi_dev->id)
> > > > > +		return 0;
> > > > > +
> > > > > +	ul_chan = mhi_dev->ul_chan;
> > > > > +	dl_chan = mhi_dev->dl_chan;
> > > > > +
> > > > > +	/*
> > > > > +	 * If execution environment is specified, remove only those devices that
> > > > > +	 * started in them based on ee_mask for the channels as we move on to a
> > > > > +	 * different execution environment
> > > > > +	 */
> > > > > +	if (data)
> > > > > +		ee = *(enum mhi_ee_type *)data;
> > > > > +
> > > > > +	if (ul_chan) {
> > > > > +		if (ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
> > > > > +			return 0;
> > > > > +	}
> > > > > +
> > > > > +	if (dl_chan) {
> > > > > +		if (ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
> > > > > +			return 0;
> > > > > +	}
> > > > > +
> > > > > +	mhi_unprepare_from_transfer(mhi_dev);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl)
> > > > > +{
> > > > > +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
> > > > > +				     ____mhi_unprepare_from_transfer);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(mhi_unprepare_all_from_transfer);
> > > > > diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> > > > > index ae092bc8b97e..dcf62a57056a 100644
> > > > > --- a/include/linux/mhi.h
> > > > > +++ b/include/linux/mhi.h
> > > > > @@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
> > > > >     * destroy struct devices. This is a variant for mhi_power_down() and is a
> > > > >     * workaround to make it possible to use mhi_power_up() in a resume
> > > > >     * handler. When using this variant the caller must also call
> > > > > - * mhi_prepare_all_for_transfer_autoqueue() and
> > > > > + * mhi_prepare_all_for_transfer() and
> > > > 
> > > > This change belongs to previous patch.
> > > > 
> > > > >     * mhi_unprepare_all_from_transfer().
> > > > >     *
> > > > >     * @mhi_cntrl: MHI controller
> > > > > @@ -842,4 +842,22 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
> > > > >     */
> > > > >    bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
> > > > > +/**
> > > > > + * mhi_prepare_all_for_transfer - if you are using
> > > > > + * mhi_power_down_no_destroy() variant this needs to be called after
> > > > > + * calling mhi_power_up().
> > > > 
> > > > Add info about what this API does also.
> > > > 
> > > > > + *
> > > > > + * @mhi_cntrl: MHI controller
> > > > > + */
> > > > > +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl);
> > > > > +
> > > > > +/**
> > > > > + * mhi_unprepare_all_from_transfer - if you are using
> > > > > + * mhi_power_down_no_destroy() variant this function needs to be called
> > > > > + * before calling mhi_power_down_no_destroy().
> > > > 
> > > > Same as above.
> > > > 
> > > > - Mani
> > > > 
> > > > > + *
> > > > > + * @mhi_cntrl: MHI controller
> > > > > + */
> > > > > +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
> > > > > +
> > > > >    #endif /* _MHI_H_ */
> > > > > -- 
> > > > > 2.39.2
> > > > > 
> > > > > 
> > > > 
> >

On 2/2/2024 3:10 PM, Manivannan Sadhasivam wrote:
> On Fri, Feb 02, 2024 at 02:42:58PM +0800, Baochen Qiang wrote:
>>
>>
>> On 2/1/2024 6:00 PM, Manivannan Sadhasivam wrote:
>>> On Wed, Jan 31, 2024 at 03:39:26PM +0800, Baochen Qiang wrote:
>>>>
>>>>
>>>> On 1/31/2024 2:19 AM, Manivannan Sadhasivam wrote:
>>>>> On Mon, Nov 27, 2023 at 06:20:16PM +0200, Kalle Valo wrote:
>>>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>>>
>>>>>> When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
>>>>>> by themselves.  Similarly, MHI stack will also not create new MHI device since
>>>>>> old devices were not destroyed, so MHI hosts need to prepare channels as well.
>>>>>> Hence add these two interfaces to make that possible.
>>>>>>
>>>>>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>>>>>>
>>>>>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>>>>>> ---
>>>>>>     drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
>>>>>>     include/linux/mhi.h         |  20 ++++++-
>>>>>>     2 files changed, 126 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
>>>>>> index d80975f4bba8..3f677fc628ad 100644
>>>>>> --- a/drivers/bus/mhi/host/main.c
>>>>>> +++ b/drivers/bus/mhi/host/main.c
>>>>>> @@ -1669,6 +1669,58 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
>>>>>>     }
>>>>>>     EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
>>>>>> +static int ____mhi_prepare_for_transfer(struct device *dev, void *data)
>>>>>
>>>>> "__mhi_prepare_all_for_transfer"
>>>>
>>>> This is to prepare one single child device, I don't think a name like
>>>> __mhi_prepare_all_for_transfer (with 'all' inside) make sense, right?
>>>> How about changing to "mhi_prepare_dev_for_transfer" or
>>>> "mhi_prepare_single_for_transfer"?
>>>>
>>>
>>> I think most of the checks in this function can be moved inside
>>> mhi_prepare_for_transfer() API. With that you can just reuse the API without
>>> adding a new helper.
>>>
>>> For autoqueue channels, you can add another API
>>> mhi_prepare_all_for_transfer_autoqueue() just like
>>> mhi_prepare_for_transfer_autoqueue() to maintain uniformity.
>>>
>>> - Mani
>> If we do that, we need to call two APIs together, does it make sense? From
>> the view of an MHI user, what we want is an API to prepare all channels, no
>> matter whether a channel is configured as autoqueue or non-autoqueue, we
>> don't care about it.
>>
> 
> You are calling this API from a wrong place first up.
> mhi_{prepare/unprepare}_transfer* APIs are meant to be used by the client
> drivers like QRTR. Controller drivers should not call them.
> 
> What you need here is the hibernation support for QRTR itself and call these
> APIs from there.

OK, I got your point. QRTR is the right place to manage MHI channels, 
not ath11k it self.
And we even don't need these two APIs if change to do it in QRTR.

> 
>> And besides, I don't think there is a scenario where we need to use them
>> separately. So if we always need to use them together, why not merge them in
>> a single API?
>>
> 
> A single controller driver may expose multiple channels and those will bind to
> multiple client drivers. So only the client drivers should manage the channels,
> not the controller drivers themselves.
Exactly.

Great thanks for the proposal, Mani. Will change accordingly in next 
version.

> 
> - Mani
> 
>>>
>>>>>
>>>>>> +{
>>>>>> +	struct mhi_device *mhi_dev;
>>>>>> +	struct mhi_chan *ul_chan, *dl_chan;
>>>>>> +	enum mhi_ee_type ee = MHI_EE_MAX;
>>>>>
>>>>> Reverse Xmas order, please.
>>>>>
>>>>>> +
>>>>>> +	if (dev->bus != &mhi_bus_type)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	mhi_dev = to_mhi_device(dev);
>>>>>> +
>>>>>> +	/* Only prepare virtual devices that are attached to bus */
>>>>>
>>>>> "Only prepare virtual devices for the channels". Here and below.
>>>>>
>>>>>> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	/* There are cases where there is no MHI client driver matches
>>>>>> +	 * this device, we are not allowed to do prepare for it.
>>>>>> +	 */
>>>>>
>>>>> Use the preferred style for comment:
>>>>>
>>>>> 	/*
>>>>> 	 * ...
>>>>> 	 */
>>>>>
>>>>>> +	if (!mhi_dev->id)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	ul_chan = mhi_dev->ul_chan;
>>>>>> +	dl_chan = mhi_dev->dl_chan;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If execution environment is specified, remove only those devices that
>>>>>> +	 * started in them based on ee_mask for the channels as we move on to a
>>>>>> +	 * different execution environment
>>>>>> +	 */
>>>>>> +	if (data)
>>>>>> +		ee = *(enum mhi_ee_type *)data;
>>>>>> +
>>>>>> +	if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>>>>>> +		return 0;
>>>>>> +
>>>>>> +
>>>>>
>>>>> Remove extra newline.
>>>>>
>>>>>> +	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	if (dl_chan->pre_alloc)
>>>>>> +		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
>>>>>> +	else
>>>>>> +		return mhi_prepare_for_transfer(mhi_dev);
>>>>>> +}
>>>>>> +
>>>>>> +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl)
>>>>>> +{
>>>>>> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>>>>>> +				     ____mhi_prepare_for_transfer);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
>>>>>> +
>>>>>>     void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>>>>>>     {
>>>>>>     	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>>>>>> @@ -1684,3 +1736,58 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>>>>>>     	}
>>>>>>     }
>>>>>>     EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
>>>>>> +
>>>>>> +static int ____mhi_unprepare_from_transfer(struct device *dev, void *data)
>>>>>
>>>>> __mhi_unprepare_all_from_transfer
>>>>
>>>> same as above.
>>>>
>>>>>
>>>>>> +{
>>>>>> +	struct mhi_device *mhi_dev;
>>>>>> +	struct mhi_chan *ul_chan, *dl_chan;
>>>>>> +	enum mhi_ee_type ee = MHI_EE_MAX;
>>>>>> +
>>>>>> +	if (dev->bus != &mhi_bus_type)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	mhi_dev = to_mhi_device(dev);
>>>>>> +
>>>>>> +	/* Only unprepare virtual devices that are attached to bus */
>>>>>> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	/* There are cases where there is no MHI client driver matches
>>>>>> +	 * this device, so it is not probed or prepared, no need to
>>>>>> +	 * do unprepare for it.
>>>>>> +	 */
>>>>>> +	if (!mhi_dev->id)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	ul_chan = mhi_dev->ul_chan;
>>>>>> +	dl_chan = mhi_dev->dl_chan;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If execution environment is specified, remove only those devices that
>>>>>> +	 * started in them based on ee_mask for the channels as we move on to a
>>>>>> +	 * different execution environment
>>>>>> +	 */
>>>>>> +	if (data)
>>>>>> +		ee = *(enum mhi_ee_type *)data;
>>>>>> +
>>>>>> +	if (ul_chan) {
>>>>>> +		if (ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>>>>>> +			return 0;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (dl_chan) {
>>>>>> +		if (ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>>>>>> +			return 0;
>>>>>> +	}
>>>>>> +
>>>>>> +	mhi_unprepare_from_transfer(mhi_dev);
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl)
>>>>>> +{
>>>>>> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>>>>>> +				     ____mhi_unprepare_from_transfer);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(mhi_unprepare_all_from_transfer);
>>>>>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>>>>>> index ae092bc8b97e..dcf62a57056a 100644
>>>>>> --- a/include/linux/mhi.h
>>>>>> +++ b/include/linux/mhi.h
>>>>>> @@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
>>>>>>      * destroy struct devices. This is a variant for mhi_power_down() and is a
>>>>>>      * workaround to make it possible to use mhi_power_up() in a resume
>>>>>>      * handler. When using this variant the caller must also call
>>>>>> - * mhi_prepare_all_for_transfer_autoqueue() and
>>>>>> + * mhi_prepare_all_for_transfer() and
>>>>>
>>>>> This change belongs to previous patch.
>>>>>
>>>>>>      * mhi_unprepare_all_from_transfer().
>>>>>>      *
>>>>>>      * @mhi_cntrl: MHI controller
>>>>>> @@ -842,4 +842,22 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>>>>>>      */
>>>>>>     bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
>>>>>> +/**
>>>>>> + * mhi_prepare_all_for_transfer - if you are using
>>>>>> + * mhi_power_down_no_destroy() variant this needs to be called after
>>>>>> + * calling mhi_power_up().
>>>>>
>>>>> Add info about what this API does also.
>>>>>
>>>>>> + *
>>>>>> + * @mhi_cntrl: MHI controller
>>>>>> + */
>>>>>> +int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl);
>>>>>> +
>>>>>> +/**
>>>>>> + * mhi_unprepare_all_from_transfer - if you are using
>>>>>> + * mhi_power_down_no_destroy() variant this function needs to be called
>>>>>> + * before calling mhi_power_down_no_destroy().
>>>>>
>>>>> Same as above.
>>>>>
>>>>> - Mani
>>>>>
>>>>>> + *
>>>>>> + * @mhi_cntrl: MHI controller
>>>>>> + */
>>>>>> +int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
>>>>>> +
>>>>>>     #endif /* _MHI_H_ */
>>>>>> -- 
>>>>>> 2.39.2
>>>>>>
>>>>>>
>>>>>
>>>
>

On Fri, Feb 02, 2024 at 06:49:19PM +0800, Baochen Qiang wrote:
> 
> 
> On 2/2/2024 3:10 PM, Manivannan Sadhasivam wrote:
> > On Fri, Feb 02, 2024 at 02:42:58PM +0800, Baochen Qiang wrote:
> > > 
> > > 
> > > On 2/1/2024 6:00 PM, Manivannan Sadhasivam wrote:
> > > > On Wed, Jan 31, 2024 at 03:39:26PM +0800, Baochen Qiang wrote:
> > > > > 
> > > > > 
> > > > > On 1/31/2024 2:19 AM, Manivannan Sadhasivam wrote:
> > > > > > On Mon, Nov 27, 2023 at 06:20:16PM +0200, Kalle Valo wrote:
> > > > > > > From: Baochen Qiang <quic_bqiang@quicinc.com>
> > > > > > > 
> > > > > > > When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
> > > > > > > by themselves.  Similarly, MHI stack will also not create new MHI device since
> > > > > > > old devices were not destroyed, so MHI hosts need to prepare channels as well.
> > > > > > > Hence add these two interfaces to make that possible.
> > > > > > > 
> > > > > > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
> > > > > > > 
> > > > > > > Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> > > > > > > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> > > > > > > ---
> > > > > > >     drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
> > > > > > >     include/linux/mhi.h         |  20 ++++++-
> > > > > > >     2 files changed, 126 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> > > > > > > index d80975f4bba8..3f677fc628ad 100644
> > > > > > > --- a/drivers/bus/mhi/host/main.c
> > > > > > > +++ b/drivers/bus/mhi/host/main.c
> > > > > > > @@ -1669,6 +1669,58 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
> > > > > > >     }
> > > > > > >     EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
> > > > > > > +static int ____mhi_prepare_for_transfer(struct device *dev, void *data)
> > > > > > 
> > > > > > "__mhi_prepare_all_for_transfer"
> > > > > 
> > > > > This is to prepare one single child device, I don't think a name like
> > > > > __mhi_prepare_all_for_transfer (with 'all' inside) make sense, right?
> > > > > How about changing to "mhi_prepare_dev_for_transfer" or
> > > > > "mhi_prepare_single_for_transfer"?
> > > > > 
> > > > 
> > > > I think most of the checks in this function can be moved inside
> > > > mhi_prepare_for_transfer() API. With that you can just reuse the API without
> > > > adding a new helper.
> > > > 
> > > > For autoqueue channels, you can add another API
> > > > mhi_prepare_all_for_transfer_autoqueue() just like
> > > > mhi_prepare_for_transfer_autoqueue() to maintain uniformity.
> > > > 
> > > > - Mani
> > > If we do that, we need to call two APIs together, does it make sense? From
> > > the view of an MHI user, what we want is an API to prepare all channels, no
> > > matter whether a channel is configured as autoqueue or non-autoqueue, we
> > > don't care about it.
> > > 
> > 
> > You are calling this API from a wrong place first up.
> > mhi_{prepare/unprepare}_transfer* APIs are meant to be used by the client
> > drivers like QRTR. Controller drivers should not call them.
> > 
> > What you need here is the hibernation support for QRTR itself and call these
> > APIs from there.
> 
> OK, I got your point. QRTR is the right place to manage MHI channels, not
> ath11k it self.
> And we even don't need these two APIs if change to do it in QRTR.
> 
> > 
> > > And besides, I don't think there is a scenario where we need to use them
> > > separately. So if we always need to use them together, why not merge them in
> > > a single API?
> > > 
> > 
> > A single controller driver may expose multiple channels and those will bind to
> > multiple client drivers. So only the client drivers should manage the channels,
> > not the controller drivers themselves.
> Exactly.
> 
> Great thanks for the proposal, Mani. Will change accordingly in next
> version.
> 

And you can drop the RFC tag in the version.

- Mani