diff mbox series

[v4,9/9] bus: mhi: ep: wake up host if the MHI state is in M3

Message ID 1689232218-28265-10-git-send-email-quic_krichai@quicinc.com
State New
Headers show
Series PCI: EPC: Add support to wake up host from D3 states | expand

Commit Message

Krishna chaitanya chundru July 13, 2023, 7:10 a.m. UTC
If the MHI state is in M3 then the most probably the host kept the
device in D3 hot or D3 cold, due to that endpoint transctions will not
be read by the host, so endpoint wakes up host to bring the host to D0
which eventually bring back the MHI state to M0.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/bus/mhi/ep/main.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Manivannan Sadhasivam July 28, 2023, 4:34 a.m. UTC | #1
On Thu, Jul 13, 2023 at 12:40:18PM +0530, Krishna chaitanya chundru wrote:
> If the MHI state is in M3 then the most probably the host kept the

s/then the/then

> device in D3 hot or D3 cold, due to that endpoint transctions will not

s/transctions/transactions

> be read by the host, so endpoint wakes up host to bring the host to D0

endpoint needs to wake up the host to bring the device to D0 state...

> which eventually bring back the MHI state to M0.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/bus/mhi/ep/main.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> index 6008818..46a888e 100644
> --- a/drivers/bus/mhi/ep/main.c
> +++ b/drivers/bus/mhi/ep/main.c
> @@ -25,6 +25,26 @@ static DEFINE_IDA(mhi_ep_cntrl_ida);
>  static int mhi_ep_create_device(struct mhi_ep_cntrl *mhi_cntrl, u32 ch_id);
>  static int mhi_ep_destroy_device(struct device *dev, void *data);
>  
> +static int mhi_ep_wake_host(struct mhi_ep_cntrl *mhi_cntrl)
> +{
> +	enum mhi_state state;
> +	bool mhi_reset;
> +	u32 count = 0;
> +
> +	mhi_cntrl->wakeup_host(mhi_cntrl);
> +
> +	/* Wait for Host to set the M0 state */
> +	while (count++ < M0_WAIT_COUNT) {
> +		msleep(M0_WAIT_DELAY_MS);
> +
> +		mhi_ep_mmio_get_mhi_state(mhi_cntrl, &state, &mhi_reset);
> +		if (state == MHI_STATE_M0)
> +			return 0;
> +	}
> +
> +	return -ENODEV;

ENODEV or ETIMEDOUT?

> +}
> +
>  static int mhi_ep_send_event(struct mhi_ep_cntrl *mhi_cntrl, u32 ring_idx,
>  			     struct mhi_ring_element *el, bool bei)
>  {
> @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
>  	buf_left = skb->len;
>  	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
>  
> +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
> +		if (mhi_ep_wake_host(mhi_cntrl)) {

Don't you need lock here in the case of multiple queue requests?

- Mani

> +			dev_err(dev, "Failed to wakeup host\n");
> +			return -ENODEV;
> +		}
> +	}
> +
>  	mutex_lock(&mhi_chan->lock);
>  
>  	do {
> -- 
> 2.7.4
>
Dan Carpenter July 28, 2023, 5:50 a.m. UTC | #2
On Fri, Jul 28, 2023 at 10:04:52AM +0530, Manivannan Sadhasivam wrote:
> > @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
> >  	buf_left = skb->len;
> >  	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
> >  
> > +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
> > +		if (mhi_ep_wake_host(mhi_cntrl)) {
> 
> Don't you need lock here in the case of multiple queue requests?
> 
> - Mani
> 
> > +			dev_err(dev, "Failed to wakeup host\n");
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> >  	mutex_lock(&mhi_chan->lock);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^
This lock isn't enough?

regards,
dan carpenter
Dan Carpenter July 28, 2023, 5:51 a.m. UTC | #3
On Thu, Jul 13, 2023 at 12:40:18PM +0530, Krishna chaitanya chundru wrote:
> @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
>  	buf_left = skb->len;
>  	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
>  
> +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
> +		if (mhi_ep_wake_host(mhi_cntrl)) {
> +			dev_err(dev, "Failed to wakeup host\n");
> +			return -ENODEV;
> +		}

Since you're going to be redoing this patch anyway could you please
propage the error code:

		ret = mhi_ep_wake_host(mhi_cntrl);
		if (ret) {
			dev_err(dev, "Failed to wakeup host\n");
			return ret;
		}

regards,
dan carpenter
Manivannan Sadhasivam July 28, 2023, 3:35 p.m. UTC | #4
On Fri, Jul 28, 2023 at 08:50:55AM +0300, Dan Carpenter wrote:
> On Fri, Jul 28, 2023 at 10:04:52AM +0530, Manivannan Sadhasivam wrote:
> > > @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
> > >  	buf_left = skb->len;
> > >  	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
> > >  
> > > +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
> > > +		if (mhi_ep_wake_host(mhi_cntrl)) {
> > 
> > Don't you need lock here in the case of multiple queue requests?
> > 
> > - Mani
> > 
> > > +			dev_err(dev, "Failed to wakeup host\n");
> > > +			return -ENODEV;
> > > +		}
> > > +	}
> > > +
> > >  	mutex_lock(&mhi_chan->lock);
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^
> This lock isn't enough?
> 

The position of this lock won't prevent cocurrent access to mhi_ep_wake_host().

- Mani

> regards,
> dan carpenter
Krishna chaitanya chundru July 31, 2023, 5:37 a.m. UTC | #5
On 7/28/2023 11:21 AM, Dan Carpenter wrote:
> On Thu, Jul 13, 2023 at 12:40:18PM +0530, Krishna chaitanya chundru wrote:
>> @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
>>   	buf_left = skb->len;
>>   	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
>>   
>> +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
>> +		if (mhi_ep_wake_host(mhi_cntrl)) {
>> +			dev_err(dev, "Failed to wakeup host\n");
>> +			return -ENODEV;
>> +		}
> Since you're going to be redoing this patch anyway could you please
> propage the error code:
>
> 		ret = mhi_ep_wake_host(mhi_cntrl);
> 		if (ret) {
> 			dev_err(dev, "Failed to wakeup host\n");
> 			return ret;
> 		}
>
> regards,
> dan carpenter

I will add this in next series.

- KC

>
Krishna chaitanya chundru July 31, 2023, 5:37 a.m. UTC | #6
On 7/28/2023 9:05 PM, Manivannan Sadhasivam wrote:
> On Fri, Jul 28, 2023 at 08:50:55AM +0300, Dan Carpenter wrote:
>> On Fri, Jul 28, 2023 at 10:04:52AM +0530, Manivannan Sadhasivam wrote:
>>>> @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
>>>>   	buf_left = skb->len;
>>>>   	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
>>>>   
>>>> +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
>>>> +		if (mhi_ep_wake_host(mhi_cntrl)) {
>>> Don't you need lock here in the case of multiple queue requests?
>>>
>>> - Mani
>>>
>>>> +			dev_err(dev, "Failed to wakeup host\n");
>>>> +			return -ENODEV;
>>>> +		}
>>>> +	}
>>>> +
>>>>   	mutex_lock(&mhi_chan->lock);
>>          ^^^^^^^^^^^^^^^^^^^^^^^^^^
>> This lock isn't enough?
>>
> The position of this lock won't prevent cocurrent access to mhi_ep_wake_host().
>
> - Mani
I will add the lock in the next series.
>> regards,
>> dan carpenter
diff mbox series

Patch

diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index 6008818..46a888e 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -25,6 +25,26 @@  static DEFINE_IDA(mhi_ep_cntrl_ida);
 static int mhi_ep_create_device(struct mhi_ep_cntrl *mhi_cntrl, u32 ch_id);
 static int mhi_ep_destroy_device(struct device *dev, void *data);
 
+static int mhi_ep_wake_host(struct mhi_ep_cntrl *mhi_cntrl)
+{
+	enum mhi_state state;
+	bool mhi_reset;
+	u32 count = 0;
+
+	mhi_cntrl->wakeup_host(mhi_cntrl);
+
+	/* Wait for Host to set the M0 state */
+	while (count++ < M0_WAIT_COUNT) {
+		msleep(M0_WAIT_DELAY_MS);
+
+		mhi_ep_mmio_get_mhi_state(mhi_cntrl, &state, &mhi_reset);
+		if (state == MHI_STATE_M0)
+			return 0;
+	}
+
+	return -ENODEV;
+}
+
 static int mhi_ep_send_event(struct mhi_ep_cntrl *mhi_cntrl, u32 ring_idx,
 			     struct mhi_ring_element *el, bool bei)
 {
@@ -464,6 +484,13 @@  int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
 	buf_left = skb->len;
 	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
 
+	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
+		if (mhi_ep_wake_host(mhi_cntrl)) {
+			dev_err(dev, "Failed to wakeup host\n");
+			return -ENODEV;
+		}
+	}
+
 	mutex_lock(&mhi_chan->lock);
 
 	do {