diff mbox series

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

Message ID 1688727836-11141-10-git-send-email-quic_krichai@quicinc.com (mailing list archive)
State Superseded
Delegated to: Krzysztof Wilczyński
Headers show
Series PCI: EPC: Add support to wake up host from D3 states | expand

Commit Message

Krishna Chaitanya Chundru July 7, 2023, 11:03 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 | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Dan Carpenter July 7, 2023, 11:41 a.m. UTC | #1 
On Fri, Jul 07, 2023 at 04:33:56PM +0530, Krishna chaitanya chundru wrote:
> 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 | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> index 6008818..46a8a3c 100644
> --- a/drivers/bus/mhi/ep/main.c
> +++ b/drivers/bus/mhi/ep/main.c
> @@ -25,6 +25,27 @@ 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 bool 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 */
> +	do {
> +		msleep(M0_WAIT_DELAY_MS);
> +		mhi_ep_mmio_get_mhi_state(mhi_cntrl, &state, &mhi_reset);
> +		count++;
> +	} while (state != MHI_STATE_M0 && count < M0_WAIT_COUNT);
> +
>+	if (state != MHI_STATE_M0)
>+		return false;

Functions which return false on success are an abomination.  Also
boolean functions should be named for the question they answer such as
access_ok() or has_feature() etc.

Actually, I think it's the caller which is wrong.  This returns true on
success and false on failure.  But the caller assumes true is failure.
It suggests that this has not been tested.

> +
> +	return true;
> +}

Write it like this:

static int mhi_ep_wake_host(struct mhi_ep_cntrl *mhi_cntrl)
{
	enum mhi_state state;
	bool mhi_reset;
	int count = 0;

	mhi_cntrl->wakeup_host(mhi_cntrl);

	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 +485,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;
> +		}

Then this becomes:

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

regards,
dan carpenter
Bjorn Helgaas July 7, 2023, 3:12 p.m. UTC | #2 
On Fri, Jul 07, 2023 at 02:41:57PM +0300, Dan Carpenter wrote:
> On Fri, Jul 07, 2023 at 04:33:56PM +0530, Krishna chaitanya chundru wrote:
> > 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 | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> > index 6008818..46a8a3c 100644
> > --- a/drivers/bus/mhi/ep/main.c
> > +++ b/drivers/bus/mhi/ep/main.c
> > @@ -25,6 +25,27 @@ 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 bool 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 */
> > +	do {
> > +		msleep(M0_WAIT_DELAY_MS);
> > +		mhi_ep_mmio_get_mhi_state(mhi_cntrl, &state, &mhi_reset);
> > +		count++;
> > +	} while (state != MHI_STATE_M0 && count < M0_WAIT_COUNT);
> > +
> >+	if (state != MHI_STATE_M0)
> >+		return false;
> 
> Functions which return false on success are an abomination.  Also
> boolean functions should be named for the question they answer such
> as access_ok() or has_feature() etc.

+1.  Also nice if boolean functions do not have side effects, so in
this case, where mhi_ep_wake_host() *does* something that might fail,
I think "return 0 for success or negative error value" is easier to
read.

> Write it like this:
> 
> static int mhi_ep_wake_host(struct mhi_ep_cntrl *mhi_cntrl)
> {
> 	enum mhi_state state;
> 	bool mhi_reset;
> 	int count = 0;
> 
> 	mhi_cntrl->wakeup_host(mhi_cntrl);
> 
> 	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;
> }
Krishna Chaitanya Chundru July 11, 2023, 6:15 a.m. UTC | #3 
On 7/7/2023 8:42 PM, Bjorn Helgaas wrote:
> On Fri, Jul 07, 2023 at 02:41:57PM +0300, Dan Carpenter wrote:
>> On Fri, Jul 07, 2023 at 04:33:56PM +0530, Krishna chaitanya chundru wrote:
>>> 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 | 28 ++++++++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
>>> index 6008818..46a8a3c 100644
>>> --- a/drivers/bus/mhi/ep/main.c
>>> +++ b/drivers/bus/mhi/ep/main.c
>>> @@ -25,6 +25,27 @@ 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 bool 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 */
>>> +	do {
>>> +		msleep(M0_WAIT_DELAY_MS);
>>> +		mhi_ep_mmio_get_mhi_state(mhi_cntrl, &state, &mhi_reset);
>>> +		count++;
>>> +	} while (state != MHI_STATE_M0 && count < M0_WAIT_COUNT);
>>> +
>>> +	if (state != MHI_STATE_M0)
>>> +		return false;
>> Functions which return false on success are an abomination.  Also
>> boolean functions should be named for the question they answer such
>> as access_ok() or has_feature() etc.
> +1.  Also nice if boolean functions do not have side effects, so in
> this case, where mhi_ep_wake_host() *does* something that might fail,
> I think "return 0 for success or negative error value" is easier to
> read.

sure Dan and Bjorn I will replace bool with int return type as suggested 
in the next patch series.

- KC

>> Write it like this:
>>
>> static int mhi_ep_wake_host(struct mhi_ep_cntrl *mhi_cntrl)
>> {
>> 	enum mhi_state state;
>> 	bool mhi_reset;
>> 	int count = 0;
>>
>> 	mhi_cntrl->wakeup_host(mhi_cntrl);
>>
>> 	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;
>> }
diff mbox series

Patch

diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index 6008818..46a8a3c 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -25,6 +25,27 @@  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 bool 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 */
+	do {
+		msleep(M0_WAIT_DELAY_MS);
+		mhi_ep_mmio_get_mhi_state(mhi_cntrl, &state, &mhi_reset);
+		count++;
+	} while (state != MHI_STATE_M0 && count < M0_WAIT_COUNT);
+
+	if (state != MHI_STATE_M0)
+		return false;
+
+	return true;
+}
+
 static int mhi_ep_send_event(struct mhi_ep_cntrl *mhi_cntrl, u32 ring_idx,
 			     struct mhi_ring_element *el, bool bei)
 {
@@ -464,6 +485,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 {