linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mailbox: Hi3660: Fixup mailbox state machine malfunction issue
@ 2018-12-03  3:55 Kevin Wangtao
  2018-12-03  6:13 ` [PATCH v2] " Kevin Wangtao
  0 siblings, 1 reply; 4+ messages in thread
From: Kevin Wangtao @ 2018-12-03  3:55 UTC (permalink / raw)
  To: jassisinghbrar
  Cc: linux-kernel, leo.yan, gengyanping, suzhuangluan, Kevin Wangtao

Current mailbox driver of Hi3660 release the mailbox directly
before sending a new message which may cause last message lost
and next message sending doesn't take effect actually.

This patch fixs this issue by following the right progress below,
each time before sending a message, mailbox driver will check
whether the mailbox is in ready state, if last message has been
acknowledged, the mailbox driver will clear the ack state to turn
the mailbox to ready state again.

Signed-off-by: Kevin Wangtao <kevin.wangtao@hisilicon.com>
---
 drivers/mailbox/hi3660-mailbox.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
index 3eea6b6..035b71a 100644
--- a/drivers/mailbox/hi3660-mailbox.c
+++ b/drivers/mailbox/hi3660-mailbox.c
@@ -38,6 +38,7 @@
 #define MBOX_AUTOMATIC_ACK		1
 
 #define MBOX_STATE_IDLE			BIT(4)
+#define MBOX_STATE_READY		BIT(5)
 #define MBOX_STATE_ACK			BIT(7)
 
 #define MBOX_MSG_LEN			8
@@ -91,8 +92,8 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
 	unsigned long val;
 	unsigned int ret;
 
-	/* Mailbox is idle so directly bail out */
-	if (readl(base + MBOX_MODE_REG) & MBOX_STATE_IDLE)
+	/* Mailbox is ready to use */
+	if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY)
 		return 0;
 
 	/* Wait for acknowledge from remote */
@@ -103,9 +104,9 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
 		return ret;
 	}
 
-	/* Ensure channel is released */
-	writel(0xffffffff, base + MBOX_IMASK_REG);
-	writel(BIT(mchan->ack_irq), base + MBOX_SRC_REG);
+	/* clear ack state, mailbox will get back to ready state */
+	writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG);
+
 	return 0;
 }
 
@@ -160,10 +161,6 @@ static int hi3660_mbox_startup(struct mbox_chan *chan)
 {
 	int ret;
 
-	ret = hi3660_mbox_check_state(chan);
-	if (ret)
-		return ret;
-
 	ret = hi3660_mbox_unlock(chan);
 	if (ret)
 		return ret;
@@ -183,10 +180,11 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
 	void __iomem *base = MBOX_BASE(mbox, ch);
 	u32 *buf = msg;
 	unsigned int i;
+	int ret;
 
-	/* Ensure channel is released */
-	writel_relaxed(0xffffffff, base + MBOX_IMASK_REG);
-	writel_relaxed(BIT(mchan->ack_irq), base + MBOX_SRC_REG);
+	ret = hi3660_mbox_check_state(chan);
+	if (ret)
+		return ret;
 
 	/* Clear mask for destination interrupt */
 	writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG);
-- 
2.8.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH v2] mailbox: Hi3660: Fixup mailbox state machine malfunction issue
  2018-12-03  3:55 [PATCH] mailbox: Hi3660: Fixup mailbox state machine malfunction issue Kevin Wangtao
@ 2018-12-03  6:13 ` Kevin Wangtao
  2018-12-07  6:14   ` leo.yan
  2018-12-10 18:31   ` Valentin Schneider
  0 siblings, 2 replies; 4+ messages in thread
From: Kevin Wangtao @ 2018-12-03  6:13 UTC (permalink / raw)
  To: jassisinghbrar
  Cc: linux-kernel, leo.yan, gengyanping, suzhuangluan, Kevin Wangtao

Current mailbox driver of Hi3660 release the mailbox directly
before sending a new message which may cause last message lost
and next message sending doesn't take effect actually.

This patch fixes this issue by following the right process below,
each time before sending a message, mailbox driver will check
whether the mailbox is in ready state, if last message has been
acknowledged, mailbox driver will clear the ack state to turn
the mailbox to ready state again.

Signed-off-by: Kevin Wangtao <kevin.wangtao@hisilicon.com>
---
Changes v1 -> v2:
 - update commit message

 drivers/mailbox/hi3660-mailbox.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
index 3eea6b6..035b71a 100644
--- a/drivers/mailbox/hi3660-mailbox.c
+++ b/drivers/mailbox/hi3660-mailbox.c
@@ -38,6 +38,7 @@
 #define MBOX_AUTOMATIC_ACK		1
 
 #define MBOX_STATE_IDLE			BIT(4)
+#define MBOX_STATE_READY		BIT(5)
 #define MBOX_STATE_ACK			BIT(7)
 
 #define MBOX_MSG_LEN			8
@@ -91,8 +92,8 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
 	unsigned long val;
 	unsigned int ret;
 
-	/* Mailbox is idle so directly bail out */
-	if (readl(base + MBOX_MODE_REG) & MBOX_STATE_IDLE)
+	/* Mailbox is ready to use */
+	if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY)
 		return 0;
 
 	/* Wait for acknowledge from remote */
@@ -103,9 +104,9 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
 		return ret;
 	}
 
-	/* Ensure channel is released */
-	writel(0xffffffff, base + MBOX_IMASK_REG);
-	writel(BIT(mchan->ack_irq), base + MBOX_SRC_REG);
+	/* clear ack state, mailbox will get back to ready state */
+	writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG);
+
 	return 0;
 }
 
@@ -160,10 +161,6 @@ static int hi3660_mbox_startup(struct mbox_chan *chan)
 {
 	int ret;
 
-	ret = hi3660_mbox_check_state(chan);
-	if (ret)
-		return ret;
-
 	ret = hi3660_mbox_unlock(chan);
 	if (ret)
 		return ret;
@@ -183,10 +180,11 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
 	void __iomem *base = MBOX_BASE(mbox, ch);
 	u32 *buf = msg;
 	unsigned int i;
+	int ret;
 
-	/* Ensure channel is released */
-	writel_relaxed(0xffffffff, base + MBOX_IMASK_REG);
-	writel_relaxed(BIT(mchan->ack_irq), base + MBOX_SRC_REG);
+	ret = hi3660_mbox_check_state(chan);
+	if (ret)
+		return ret;
 
 	/* Clear mask for destination interrupt */
 	writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG);
-- 
2.8.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] mailbox: Hi3660: Fixup mailbox state machine malfunction issue
  2018-12-03  6:13 ` [PATCH v2] " Kevin Wangtao
@ 2018-12-07  6:14   ` leo.yan
  2018-12-10 18:31   ` Valentin Schneider
  1 sibling, 0 replies; 4+ messages in thread
From: leo.yan @ 2018-12-07  6:14 UTC (permalink / raw)
  To: Kevin Wangtao
  Cc: jassisinghbrar, linux-kernel, gengyanping, suzhuangluan,
	John Stultz, Guodong Xu

On Mon, Dec 03, 2018 at 02:13:08PM +0800, Kevin Wangtao wrote:
> Current mailbox driver of Hi3660 release the mailbox directly
> before sending a new message which may cause last message lost
> and next message sending doesn't take effect actually.
> 
> This patch fixes this issue by following the right process below,
> each time before sending a message, mailbox driver will check
> whether the mailbox is in ready state, if last message has been
> acknowledged, mailbox driver will clear the ack state to turn
> the mailbox to ready state again.
> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@hisilicon.com>

Thanks for sending out this patch, Kevin.

I tested this patch at my side with two cases (one is dhry2 for
performance testing for every OPPs, and another is stress test
two clusters frequency change concurrently), both of them can
pass after applied this patch.

Reviewed-and-tested-by: Leo Yan <leo.yan@linaro.org>

Jassi, please review and pick it up if it's okay for you.

Thanks,
Leo Yan

> ---
> Changes v1 -> v2:
>  - update commit message
> 
>  drivers/mailbox/hi3660-mailbox.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> index 3eea6b6..035b71a 100644
> --- a/drivers/mailbox/hi3660-mailbox.c
> +++ b/drivers/mailbox/hi3660-mailbox.c
> @@ -38,6 +38,7 @@
>  #define MBOX_AUTOMATIC_ACK		1
>  
>  #define MBOX_STATE_IDLE			BIT(4)
> +#define MBOX_STATE_READY		BIT(5)
>  #define MBOX_STATE_ACK			BIT(7)
>  
>  #define MBOX_MSG_LEN			8
> @@ -91,8 +92,8 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
>  	unsigned long val;
>  	unsigned int ret;
>  
> -	/* Mailbox is idle so directly bail out */
> -	if (readl(base + MBOX_MODE_REG) & MBOX_STATE_IDLE)
> +	/* Mailbox is ready to use */
> +	if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY)
>  		return 0;
>  
>  	/* Wait for acknowledge from remote */
> @@ -103,9 +104,9 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
>  		return ret;
>  	}
>  
> -	/* Ensure channel is released */
> -	writel(0xffffffff, base + MBOX_IMASK_REG);
> -	writel(BIT(mchan->ack_irq), base + MBOX_SRC_REG);
> +	/* clear ack state, mailbox will get back to ready state */
> +	writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG);
> +
>  	return 0;
>  }
>  
> @@ -160,10 +161,6 @@ static int hi3660_mbox_startup(struct mbox_chan *chan)
>  {
>  	int ret;
>  
> -	ret = hi3660_mbox_check_state(chan);
> -	if (ret)
> -		return ret;
> -
>  	ret = hi3660_mbox_unlock(chan);
>  	if (ret)
>  		return ret;
> @@ -183,10 +180,11 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
>  	void __iomem *base = MBOX_BASE(mbox, ch);
>  	u32 *buf = msg;
>  	unsigned int i;
> +	int ret;
>  
> -	/* Ensure channel is released */
> -	writel_relaxed(0xffffffff, base + MBOX_IMASK_REG);
> -	writel_relaxed(BIT(mchan->ack_irq), base + MBOX_SRC_REG);
> +	ret = hi3660_mbox_check_state(chan);
> +	if (ret)
> +		return ret;
>  
>  	/* Clear mask for destination interrupt */
>  	writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG);
> -- 
> 2.8.1
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] mailbox: Hi3660: Fixup mailbox state machine malfunction issue
  2018-12-03  6:13 ` [PATCH v2] " Kevin Wangtao
  2018-12-07  6:14   ` leo.yan
@ 2018-12-10 18:31   ` Valentin Schneider
  1 sibling, 0 replies; 4+ messages in thread
From: Valentin Schneider @ 2018-12-10 18:31 UTC (permalink / raw)
  To: Kevin Wangtao, jassisinghbrar
  Cc: linux-kernel, leo.yan, gengyanping, suzhuangluan

Hi,

On 03/12/2018 06:13, Kevin Wangtao wrote:
> Current mailbox driver of Hi3660 release the mailbox directly
> before sending a new message which may cause last message lost
> and next message sending doesn't take effect actually.
> 
> This patch fixes this issue by following the right process below,
> each time before sending a message, mailbox driver will check
> whether the mailbox is in ready state, if last message has been
> acknowledged, mailbox driver will clear the ack state to turn
> the mailbox to ready state again.
> 
> Signed-off-by: Kevin Wangtao <kevin.wangtao@hisilicon.com>

Validated using sysbench at each OPP ([1]). Test passes with patch and fails
without:

| CPU  |     OPP | Base | Patch |
|------+---------+------+-------|
| CPU0 |  533000 |  104 |   104 |
| CPU0 |  999000 |  104 |   201 |
| CPU0 |  140200 |  285 |   285 |
| CPU0 | 1709000 |  285 |   349 |
| CPU0 | 1844000 |  377 |   377 |
|------+---------+------+-------|
| CPU4 |  903000 |  249 |   248 |
| CPU4 | 1421000 |  249 |   394 |
| CPU4 | 1805000 |  500 |   500 |
| CPU4 | 2112000 |  499 |   583 |
| CPU4 | 2362000 |  653 |   654 |

We need this pretty badly, otherwise frequency setting is a coin toss.

Tested-by: Valentin Schneider <valentin.schneider@arm.com>

Thanks,
Valentin



[1]: https://github.com/ARM-software/lisa/blob/next/lisa/tests/kernel/cpufreq/sanity.py#L23

Ran the above with this script on each kernel:

----->8-----
#!/usr/bin/env python3

from lisa.env import TestEnv
from lisa.tests.kernel.cpufreq.sanity import UserspaceSanity

te = TestEnv.from_cli()

print(UserspaceSanity.from_testenv(te).test_performance_sanity())
-----8<-----


> ---
> Changes v1 -> v2:
>  - update commit message
> 
>  drivers/mailbox/hi3660-mailbox.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> index 3eea6b6..035b71a 100644
> --- a/drivers/mailbox/hi3660-mailbox.c
> +++ b/drivers/mailbox/hi3660-mailbox.c
> @@ -38,6 +38,7 @@
>  #define MBOX_AUTOMATIC_ACK		1
>  
>  #define MBOX_STATE_IDLE			BIT(4)
> +#define MBOX_STATE_READY		BIT(5)
>  #define MBOX_STATE_ACK			BIT(7)
>  
>  #define MBOX_MSG_LEN			8
> @@ -91,8 +92,8 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
>  	unsigned long val;
>  	unsigned int ret;
>  
> -	/* Mailbox is idle so directly bail out */
> -	if (readl(base + MBOX_MODE_REG) & MBOX_STATE_IDLE)
> +	/* Mailbox is ready to use */
> +	if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY)
>  		return 0;
>  
>  	/* Wait for acknowledge from remote */
> @@ -103,9 +104,9 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan)
>  		return ret;
>  	}
>  
> -	/* Ensure channel is released */
> -	writel(0xffffffff, base + MBOX_IMASK_REG);
> -	writel(BIT(mchan->ack_irq), base + MBOX_SRC_REG);
> +	/* clear ack state, mailbox will get back to ready state */
> +	writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG);
> +
>  	return 0;
>  }
>  
> @@ -160,10 +161,6 @@ static int hi3660_mbox_startup(struct mbox_chan *chan)
>  {
>  	int ret;
>  
> -	ret = hi3660_mbox_check_state(chan);
> -	if (ret)
> -		return ret;
> -
>  	ret = hi3660_mbox_unlock(chan);
>  	if (ret)
>  		return ret;
> @@ -183,10 +180,11 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
>  	void __iomem *base = MBOX_BASE(mbox, ch);
>  	u32 *buf = msg;
>  	unsigned int i;
> +	int ret;
>  
> -	/* Ensure channel is released */
> -	writel_relaxed(0xffffffff, base + MBOX_IMASK_REG);
> -	writel_relaxed(BIT(mchan->ack_irq), base + MBOX_SRC_REG);
> +	ret = hi3660_mbox_check_state(chan);
> +	if (ret)
> +		return ret;
>  
>  	/* Clear mask for destination interrupt */
>  	writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG);
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-10 18:31 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-03  3:55 [PATCH] mailbox: Hi3660: Fixup mailbox state machine malfunction issue Kevin Wangtao
2018-12-03  6:13 ` [PATCH v2] " Kevin Wangtao
2018-12-07  6:14   ` leo.yan
2018-12-10 18:31   ` Valentin Schneider

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).