Updates to MHI channel handling

Some MHI clients may want to request for pausing or resuming of the data transfers for their channels. Enable them to do so using the new APIs provided for the same.

Signed-off-by Bhaumik Bhatt <[email protected]>

---
 drivers/bus/mhi/core/main.c | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mhi.h         | 16 ++++++++++++++++
 2 files changed, 57 insertions(+)

drivers/bus/mhi/core/main.c +30 -0

index 1226933..01845c6 100644
@@ -1560,6 +1560,47 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
 }
 EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
 
+static int mhi_update_transfer_state(struct mhi_device *mhi_dev,
+                                    enum mhi_ch_state_type to_state)
+{
+       struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
+       struct mhi_chan *mhi_chan;
+       int dir, ret;
+
+       for (dir = 0; dir < 2; dir++) {
+               mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
+
+               if (!mhi_chan)
+                       continue;
+
+               /*
+                * Bail out if one of the channels fail as client will reset
+                * both upon failure
+                */
+               mutex_lock(&mhi_chan->mutex);
+               ret = mhi_update_channel_state(mhi_cntrl, mhi_chan, to_state);
+               if (ret) {
+                       mutex_unlock(&mhi_chan->mutex);
+                       return ret;
+               }
+               mutex_unlock(&mhi_chan->mutex);
+       }
+
+       return 0;
+}
+
+int mhi_pause_transfer(struct mhi_device *mhi_dev)

Hi Bhaumik,

Quoted 59 lines · Bhaumik Bhatt

Some MHI clients may want to request for pausing or resuming of the data transfers for their channels. Enable them to do so using the new APIs provided for the same.

Signed-off-by Bhaumik Bhatt <[email protected]>

---
 drivers/bus/mhi/core/main.c | 41 
+++++++++++++++++++++++++++++++++++++++++
 include/linux/mhi.h         | 16 ++++++++++++++++
 2 files changed, 57 insertions(+)

drivers/bus/mhi/core/main.c +15 -0

index 1226933..01845c6 100644
@@ -1560,6 +1560,47 @@ void mhi_unprepare_from_transfer(struct 
 mhi_device *mhi_dev)
 }
 EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
 
+static int mhi_update_transfer_state(struct mhi_device *mhi_dev,
+                                    enum mhi_ch_state_type to_state)
+{
+       struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
+       struct mhi_chan *mhi_chan;
+       int dir, ret;
+
+       for (dir = 0; dir < 2; dir++) {
+               mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
+
+               if (!mhi_chan)
+                       continue;
+
+               /*
+                * Bail out if one of the channels fail as client will 

reset

drivers/bus/mhi/core/main.c +4 -0

+                * both upon failure
+                */
+               mutex_lock(&mhi_chan->mutex);
+               ret = mhi_update_channel_state(mhi_cntrl, mhi_chan, 

to_state);

drivers/bus/mhi/core/main.c +13 -0

+               if (ret) {
+                       mutex_unlock(&mhi_chan->mutex);
+                       return ret;
+               }
+               mutex_unlock(&mhi_chan->mutex);
+       }
+
+       return 0;
+}
+
+int mhi_pause_transfer(struct mhi_device *mhi_dev)
+{
+       return mhi_update_transfer_state(mhi_dev, 

MHI_CH_STATE_TYPE_STOP);

drivers/bus/mhi/core/main.c +3 -0

+}
+EXPORT_SYMBOL_GPL(mhi_pause_transfer);
+

drivers/bus/mhi/core/main.c

Quoted 6 lines

drivers/bus/mhi/core/main.c +3 -0

+int mhi_resume_transfer(struct mhi_device *mhi_dev)
+{
+       return mhi_update_transfer_state(mhi_dev, 

MHI_CH_STATE_TYPE_START);

drivers/bus/mhi/core/main.c +2 -0

+}
+EXPORT_SYMBOL_GPL(mhi_resume_transfer);

Look like it is stop and start, not pause and resume?

TBH maybe we should rework/clarify MHI core and having well-defined states, maybe something like that:

1. When MHI core detects device for a driver, MHI core resets and initializes the channel(s), then call client driver probe function

    => channel UNKNOWN->DISABLED state
    => channel DISABLED->ENABLED state
2. When driver is ready for sending data, drivers calls mhi_start_transfer
    => Channel is ENABLED->RUNNING state
3. Driver performs normal data transfers
4. The driver can suspend/resume transfer, it stops (suspend) the channel, can
    => Channel is RUNNING->STOP
    => Channel is STOP->RUNNING
   ...
5. When device is removed, MHI core reset the channel
    => channel is (RUNNING|STOP) -> DISABLED

Today mhi_prepare_for_transfer performs both ENABLE and RUNNING transition, the idea would be to keep channel enabling/disabling in the MHI core (before/after driver probe/remove) and channel start/stop managed by the client driver.

Regards,
Loic

Hi Loic,

Quoted 63 lines · Loic Poulain

Hi Bhaumik,

Quoted 59 lines · Bhaumik Bhatt

Some MHI clients may want to request for pausing or resuming of the data transfers for their channels. Enable them to do so using the new APIs provided for the same.

Signed-off-by Bhaumik Bhatt <[email protected]>

---
 drivers/bus/mhi/core/main.c | 41
+++++++++++++++++++++++++++++++++++++++++
 include/linux/mhi.h         | 16 ++++++++++++++++
 2 files changed, 57 insertions(+)

drivers/bus/mhi/core/main.c +15 -0

index 1226933..01845c6 100644
@@ -1560,6 +1560,47 @@ void mhi_unprepare_from_transfer(struct
 mhi_device *mhi_dev)
 }
 EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
 
+static int mhi_update_transfer_state(struct mhi_device *mhi_dev,
+                                    enum mhi_ch_state_type to_state)
+{
+       struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
+       struct mhi_chan *mhi_chan;
+       int dir, ret;
+
+       for (dir = 0; dir < 2; dir++) {
+               mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
+
+               if (!mhi_chan)
+                       continue;
+
+               /*
+                * Bail out if one of the channels fail as client will

reset

drivers/bus/mhi/core/main.c +4 -0

+                * both upon failure
+                */
+               mutex_lock(&mhi_chan->mutex);
+               ret = mhi_update_channel_state(mhi_cntrl, mhi_chan,

to_state);

drivers/bus/mhi/core/main.c +13 -0

+               if (ret) {
+                       mutex_unlock(&mhi_chan->mutex);
+                       return ret;
+               }
+               mutex_unlock(&mhi_chan->mutex);
+       }
+
+       return 0;
+}
+
+int mhi_pause_transfer(struct mhi_device *mhi_dev)
+{
+       return mhi_update_transfer_state(mhi_dev,

MHI_CH_STATE_TYPE_STOP);

drivers/bus/mhi/core/main.c +3 -0

+}
+EXPORT_SYMBOL_GPL(mhi_pause_transfer);
+

Quoted 8 lines

drivers/bus/mhi/core/main.c

Quoted 6 lines

drivers/bus/mhi/core/main.c +3 -0

+int mhi_resume_transfer(struct mhi_device *mhi_dev)
+{
+       return mhi_update_transfer_state(mhi_dev,

MHI_CH_STATE_TYPE_START);

drivers/bus/mhi/core/main.c +2 -0

+}
+EXPORT_SYMBOL_GPL(mhi_resume_transfer);

Look like it is stop and start, not pause and resume?

I wanted to keep it pause and resume because it could get confusing for someone
looking at this pair of APIs, that a client driver would also need to "start"
channels after "preparing" them. Since that is not that case, and the mhi_prepare_for_transfer() API itself is supposed to also start the channels, it

would be better to keep these as "pause" and "resume" instead IMO.

Any comments in favor or "stop" and "start"?

Quoted 20 lines

TBH maybe we should rework/clarify MHI core and having well-defined states, maybe something like that:

1. When MHI core detects device for a driver, MHI core resets and initializes the channel(s), then call client driver probe function

    => channel UNKNOWN->DISABLED state
    => channel DISABLED->ENABLED state
2. When driver is ready for sending data, drivers calls
mhi_start_transfer
    => Channel is ENABLED->RUNNING state
3. Driver performs normal data transfers
4. The driver can suspend/resume transfer, it stops (suspend) the
channel, can
    => Channel is RUNNING->STOP
    => Channel is STOP->RUNNING
   ...
5. When device is removed, MHI core reset the channel
    => channel is (RUNNING|STOP) -> DISABLED

Quoted 7 lines

Today mhi_prepare_for_transfer performs both ENABLE and RUNNING transition, the idea would be to keep channel enabling/disabling in the MHI core (before/after driver probe/remove) and channel start/stop managed by the client driver.

Regards,
Loic

Your idea is good but it would not have much additional benefits and would
involve MHI core "enabling" channels and allocating memory for each channel
context when they are only declared as supported by the controller but are not
actually being put to use.

mhi_prepare_for_transfer() does both channel context initialization and starts
the channels, which is good because it allocates memory when needed. So, this
benefits system memory if a controller with support for many channels exists but
only a few channels are used.

Regarding the states to track from host:
-> DISABLED (We know channels are not active: in reset state or not probed yet)
-> ENABLED (Active and running when needed for data transfers) -> STOP (Paused: leaves the channel context as is since channels are not reset)
-> SUSPENDED (Unload in progress: Entered before resetting channels/remove())

BTW, we have the debugfs entry for "channels" that dumps the context to show
exactly what the channel states are from device perspective. We can rely on it
if needed.

If there are some comments I can add to make things clear, please let me know.

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

Hi Bhaumik,

Quoted 75 lines · Bhaumik Bhatt

Hi Loic,

Quoted 63 lines · Loic Poulain

Hi Bhaumik,

Quoted 59 lines · Bhaumik Bhatt

Some MHI clients may want to request for pausing or resuming of the data transfers for their channels. Enable them to do so using the new APIs provided for the same.

Signed-off-by Bhaumik Bhatt <[email protected]>

---
 drivers/bus/mhi/core/main.c | 41
+++++++++++++++++++++++++++++++++++++++++
 include/linux/mhi.h         | 16 ++++++++++++++++
 2 files changed, 57 insertions(+)

drivers/bus/mhi/core/main.c +15 -0

index 1226933..01845c6 100644
@@ -1560,6 +1560,47 @@ void mhi_unprepare_from_transfer(struct
 mhi_device *mhi_dev)
 }
 EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
 
+static int mhi_update_transfer_state(struct mhi_device *mhi_dev,
+                                    enum mhi_ch_state_type to_state)
+{
+       struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
+       struct mhi_chan *mhi_chan;
+       int dir, ret;
+
+       for (dir = 0; dir < 2; dir++) {
+               mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
+
+               if (!mhi_chan)
+                       continue;
+
+               /*
+                * Bail out if one of the channels fail as client will

reset

drivers/bus/mhi/core/main.c +4 -0

+                * both upon failure
+                */
+               mutex_lock(&mhi_chan->mutex);
+               ret = mhi_update_channel_state(mhi_cntrl, mhi_chan,

to_state);

drivers/bus/mhi/core/main.c +13 -0

+               if (ret) {
+                       mutex_unlock(&mhi_chan->mutex);
+                       return ret;
+               }
+               mutex_unlock(&mhi_chan->mutex);
+       }
+
+       return 0;
+}
+
+int mhi_pause_transfer(struct mhi_device *mhi_dev)
+{
+       return mhi_update_transfer_state(mhi_dev,

MHI_CH_STATE_TYPE_STOP);

drivers/bus/mhi/core/main.c +3 -0

+}
+EXPORT_SYMBOL_GPL(mhi_pause_transfer);
+

Quoted 8 lines

drivers/bus/mhi/core/main.c

Quoted 6 lines

drivers/bus/mhi/core/main.c +3 -0

+int mhi_resume_transfer(struct mhi_device *mhi_dev)
+{
+       return mhi_update_transfer_state(mhi_dev,

MHI_CH_STATE_TYPE_START);

drivers/bus/mhi/core/main.c +2 -0

+}
+EXPORT_SYMBOL_GPL(mhi_resume_transfer);

Look like it is stop and start, not pause and resume?

Quoted 7 lines

I wanted to keep it pause and resume because it could get confusing for someone
looking at this pair of APIs, that a client driver would also need to "start"
channels after "preparing" them. Since that is not that case, and the mhi_prepare_for_transfer() API itself is supposed to also start the channels, it

Yes, because prepare_for_transfer is actually init_and_start. I'm not in favor of hiding what is really done at mhi_core level, start is start and stop is stop, if it's correctly documented that should not be confusing. just saying (stop moves channels in stop state, start in enabled state), but other opinions are welcome.

Quoted 31 lines

would be better to keep these as "pause" and "resume" instead IMO.

Any comments in favor or "stop" and "start"?

Quoted 20 lines

TBH maybe we should rework/clarify MHI core and having well-defined states, maybe something like that:

1. When MHI core detects device for a driver, MHI core resets and initializes the channel(s), then call client driver probe function

    => channel UNKNOWN->DISABLED state
    => channel DISABLED->ENABLED state
2. When driver is ready for sending data, drivers calls
mhi_start_transfer
    => Channel is ENABLED->RUNNING state
3. Driver performs normal data transfers
4. The driver can suspend/resume transfer, it stops (suspend) the
channel, can
    => Channel is RUNNING->STOP
    => Channel is STOP->RUNNING
   ...
5. When device is removed, MHI core reset the channel
    => channel is (RUNNING|STOP) -> DISABLED

Quoted 7 lines

Today mhi_prepare_for_transfer performs both ENABLE and RUNNING transition, the idea would be to keep channel enabling/disabling in the MHI core (before/after driver probe/remove) and channel start/stop managed by the client driver.

Regards,
Loic

Quoted 7 lines

Your idea is good but it would not have much additional benefits and would
involve MHI core "enabling" channels and allocating memory for each channel
context when they are only declared as supported by the controller but are not
actually being put to use.

Ok, your point is valid.

Quoted 34 lines

mhi_prepare_for_transfer() does both channel context initialization and starts
the channels, which is good because it allocates memory when needed. So, this
benefits system memory if a controller with support for many channels exists but
only a few channels are used.

Regarding the states to track from host:
-> DISABLED (We know channels are not active: in reset state or not probed yet)
-> ENABLED (Active and running when needed for data transfers) -> STOP (Paused: leaves the channel context as is since channels are not
reset)
-> SUSPENDED (Unload in progress: Entered before resetting channels/remove())

BTW, we have the debugfs entry for "channels" that dumps the context to show
exactly what the channel states are from device perspective. We can rely on it
if needed.

If there are some comments I can add to make things clear, please let me know.

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

Hi Loic,

Quoted 10 lines

Quoted 9 lines

Quoted 1 line

Look like it is stop and start, not pause and resume?

I wanted to keep it pause and resume because it could get confusing for
someone
looking at this pair of APIs, that a client driver would also need to "start"
channels after "preparing" them. Since that is not that case, and the mhi_prepare_for_transfer() API itself is supposed to also start the channels, it

Quoted 6 lines

Yes, because prepare_for_transfer is actually init_and_start. I'm not in favor of hiding what is really done at mhi_core level, start is start and stop is stop, if it's correctly documented that should not be confusing. just saying (stop moves channels in stop state, start in enabled state), but other opinions are welcome.

I can rename it and have it documented in the mhi_prepare_for_transfer() API that we actually already start the channel, so it is not required to be used at first. I can improve this documentation in mhi.h as a separate patch.

Later, if a client driver wants to issue stop and start commands, it can do so.
I'm not too picky with the name. Maybe Mani or someone else may have more comments.