[PATCH 1/3] mailbox: sprd: Introduce refcnt when clients requests/free channels

[PATCH 1/3] mailbox: sprd: Introduce refcnt when clients requests/free channels

[Orson Zhai]

From: Orson Zhai <orson.zhai@unisoc.com>

Unisoc mailbox has no way to be enabled/disabled for any single channel.
They can only be set to startup or shutdown as a whole device at same time.

Add a variable to count references to avoid mailbox FIFO being reset
unexpectedly when clients are requesting or freeing channels.

Also add a lock to dismiss possible conflicts from register r/w in
different startup or shutdown threads.

Fixes: ca27fc26cd22 ("mailbox: sprd: Add Spreadtrum mailbox driver")
Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
---
 drivers/mailbox/sprd-mailbox.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/mailbox/sprd-mailbox.c b/drivers/mailbox/sprd-mailbox.c
index f6fab24..e606f52 100644
--- a/drivers/mailbox/sprd-mailbox.c
+++ b/drivers/mailbox/sprd-mailbox.c
@@ -60,6 +60,8 @@ struct sprd_mbox_priv {
 	struct clk		*clk;
 	u32			outbox_fifo_depth;
 
+	struct mutex		lock;
+	u32			refcnt;
 	struct mbox_chan	chan[SPRD_MBOX_CHAN_MAX];
 };
 
@@ -215,18 +217,22 @@ static int sprd_mbox_startup(struct mbox_chan *chan)
 	struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
 	u32 val;
 
-	/* Select outbox FIFO mode and reset the outbox FIFO status */
-	writel(0x0, priv->outbox_base + SPRD_MBOX_FIFO_RST);
+	mutex_lock(&priv->lock);
+	if (priv->refcnt++ == 0) {
+		/* Select outbox FIFO mode and reset the outbox FIFO status */
+		writel(0x0, priv->outbox_base + SPRD_MBOX_FIFO_RST);
 
-	/* Enable inbox FIFO overflow and delivery interrupt */
-	val = readl(priv->inbox_base + SPRD_MBOX_IRQ_MSK);
-	val &= ~(SPRD_INBOX_FIFO_OVERFLOW_IRQ | SPRD_INBOX_FIFO_DELIVER_IRQ);
-	writel(val, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
+		/* Enable inbox FIFO overflow and delivery interrupt */
+		val = readl(priv->inbox_base + SPRD_MBOX_IRQ_MSK);
+		val &= ~(SPRD_INBOX_FIFO_OVERFLOW_IRQ | SPRD_INBOX_FIFO_DELIVER_IRQ);
+		writel(val, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
 
-	/* Enable outbox FIFO not empty interrupt */
-	val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
-	val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
-	writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
+		/* Enable outbox FIFO not empty interrupt */
+		val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
+		val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
+		writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
+	}
+	mutex_unlock(&priv->lock);
 
 	return 0;
 }
@@ -235,9 +241,13 @@ static void sprd_mbox_shutdown(struct mbox_chan *chan)
 {
 	struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
 
-	/* Disable inbox & outbox interrupt */
-	writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
-	writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
+	mutex_lock(&priv->lock);
+	if (--priv->refcnt == 0) {
+		/* Disable inbox & outbox interrupt */
+		writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
+		writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
+	}
+	mutex_unlock(&priv->lock);
 }
 
 static const struct mbox_chan_ops sprd_mbox_ops = {
@@ -266,6 +276,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv->dev = dev;
+	priv->refcnt = 0;
+	mutex_init(&priv->lock);
 
 	/*
 	 * The Spreadtrum mailbox uses an inbox to send messages to the target
-- 
2.7.4

[PATCH 2/3] mailbox: sprd: Add supplementary inbox support

[Orson Zhai]

From: Orson Zhai <orson.zhai@unisoc.com>

Some sensors connected to Unisoc mailbox will send data very frequently.
This makes channel 0 very busy and the messages from other remote cores
not able to be handled as soon as possible.

Then a supplementary inbox is added to the host core side for transferring
mass but not emergency messages from the remote cores, such as step
counting sensor, with an independent FIFO and interrupt.

Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
---
 drivers/mailbox/sprd-mailbox.c | 93 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 75 insertions(+), 18 deletions(-)

diff --git a/drivers/mailbox/sprd-mailbox.c b/drivers/mailbox/sprd-mailbox.c
index e606f52..74648db 100644
--- a/drivers/mailbox/sprd-mailbox.c
+++ b/drivers/mailbox/sprd-mailbox.c
@@ -11,6 +11,7 @@
 #include <linux/io.h>
 #include <linux/mailbox_controller.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 
@@ -50,13 +51,17 @@
 #define SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ		BIT(0)
 #define SPRD_OUTBOX_FIFO_IRQ_MASK		GENMASK(4, 0)
 
+#define SPRD_OUTBOX_BASE_SPAN			0x1000
 #define SPRD_MBOX_CHAN_MAX			8
+#define SPRD_SUPP_INBOX_ID_SC9860		6
 
 struct sprd_mbox_priv {
 	struct mbox_controller	mbox;
 	struct device		*dev;
 	void __iomem		*inbox_base;
 	void __iomem		*outbox_base;
+	/*  Base register address for supplementary outbox */
+	void __iomem		*supp_base;
 	struct clk		*clk;
 	u32			outbox_fifo_depth;
 
@@ -96,14 +101,13 @@ static u32 sprd_mbox_get_fifo_len(struct sprd_mbox_priv *priv, u32 fifo_sts)
 	return fifo_len;
 }
 
-static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
+static inline irqreturn_t do_outbox_isr(void __iomem *base, struct sprd_mbox_priv *priv)
 {
-	struct sprd_mbox_priv *priv = data;
 	struct mbox_chan *chan;
 	u32 fifo_sts, fifo_len, msg[2];
 	int i, id;
 
-	fifo_sts = readl(priv->outbox_base + SPRD_MBOX_FIFO_STS);
+	fifo_sts = readl(base + SPRD_MBOX_FIFO_STS);
 
 	fifo_len = sprd_mbox_get_fifo_len(priv, fifo_sts);
 	if (!fifo_len) {
@@ -112,23 +116,41 @@ static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
 	}
 
 	for (i = 0; i < fifo_len; i++) {
-		msg[0] = readl(priv->outbox_base + SPRD_MBOX_MSG_LOW);
-		msg[1] = readl(priv->outbox_base + SPRD_MBOX_MSG_HIGH);
-		id = readl(priv->outbox_base + SPRD_MBOX_ID);
+		msg[0] = readl(base + SPRD_MBOX_MSG_LOW);
+		msg[1] = readl(base + SPRD_MBOX_MSG_HIGH);
+		id = readl(base + SPRD_MBOX_ID);
 
 		chan = &priv->chan[id];
-		mbox_chan_received_data(chan, (void *)msg);
+		if (chan->cl)
+			mbox_chan_received_data(chan, (void *)msg);
+		else
+			dev_warn_ratelimited(priv->dev,
+				    "message's been dropped at ch[%d]\n", id);
 
 		/* Trigger to update outbox FIFO pointer */
-		writel(0x1, priv->outbox_base + SPRD_MBOX_TRIGGER);
+		writel(0x1, base + SPRD_MBOX_TRIGGER);
 	}
 
 	/* Clear irq status after reading all message. */
-	writel(SPRD_MBOX_IRQ_CLR, priv->outbox_base + SPRD_MBOX_IRQ_STS);
+	writel(SPRD_MBOX_IRQ_CLR, base + SPRD_MBOX_IRQ_STS);
 
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
+{
+	struct sprd_mbox_priv *priv = data;
+
+	return do_outbox_isr(priv->outbox_base, priv);
+}
+
+static irqreturn_t sprd_mbox_supp_isr(int irq, void *data)
+{
+	struct sprd_mbox_priv *priv = data;
+
+	return do_outbox_isr(priv->supp_base, priv);
+}
+
 static irqreturn_t sprd_mbox_inbox_isr(int irq, void *data)
 {
 	struct sprd_mbox_priv *priv = data;
@@ -231,6 +253,14 @@ static int sprd_mbox_startup(struct mbox_chan *chan)
 		val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
 		val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
 		writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
+
+		/* Enable supplementary outbox as the fundamental one */
+		if (priv->supp_base) {
+			writel(0x0, priv->supp_base + SPRD_MBOX_FIFO_RST);
+			val = readl(priv->supp_base + SPRD_MBOX_IRQ_MSK);
+			val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
+			writel(val, priv->supp_base + SPRD_MBOX_IRQ_MSK);
+		}
 	}
 	mutex_unlock(&priv->lock);
 
@@ -246,6 +276,10 @@ static void sprd_mbox_shutdown(struct mbox_chan *chan)
 		/* Disable inbox & outbox interrupt */
 		writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
 		writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
+
+		if (priv->supp_base)
+			writel(SPRD_OUTBOX_FIFO_IRQ_MASK,
+			       priv->supp_base + SPRD_MBOX_IRQ_MSK);
 	}
 	mutex_unlock(&priv->lock);
 }
@@ -268,8 +302,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct sprd_mbox_priv *priv;
-	int ret, inbox_irq, outbox_irq;
-	unsigned long id;
+	int ret, inbox_irq, outbox_irq, supp_irq;
+	unsigned long id, supp;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -280,11 +314,15 @@ static int sprd_mbox_probe(struct platform_device *pdev)
 	mutex_init(&priv->lock);
 
 	/*
-	 * The Spreadtrum mailbox uses an inbox to send messages to the target
-	 * core, and uses an outbox to receive messages from other cores.
+	 * Unisoc mailbox uses an inbox to send messages to the target
+	 * core, and uses (an) outbox(es) to receive messages from other
+	 * cores.
+	 *
+	 * Thus in general the mailbox controller supplies 2 different
+	 * register addresses and IRQ numbers for inbox and outbox.
 	 *
-	 * Thus the mailbox controller supplies 2 different register addresses
-	 * and IRQ numbers for inbox and outbox.
+	 * If necessary, a supplementary inbox could be enabled optionally
+	 * with an independent FIFO and an extra interrupt.
 	 */
 	priv->inbox_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->inbox_base))
@@ -310,7 +348,7 @@ static int sprd_mbox_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	inbox_irq = platform_get_irq(pdev, 0);
+	inbox_irq = platform_get_irq_byname(pdev, "inbox");
 	if (inbox_irq < 0)
 		return inbox_irq;
 
@@ -321,7 +359,7 @@ static int sprd_mbox_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	outbox_irq = platform_get_irq(pdev, 1);
+	outbox_irq = platform_get_irq_byname(pdev, "outbox");
 	if (outbox_irq < 0)
 		return outbox_irq;
 
@@ -332,6 +370,24 @@ static int sprd_mbox_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* Supplementary outbox IRQ is optional */
+	supp_irq = platform_get_irq_byname(pdev, "supp-outbox");
+	if (supp_irq > 0) {
+		ret = devm_request_irq(dev, supp_irq, sprd_mbox_supp_isr,
+				       IRQF_NO_SUSPEND, dev_name(dev), priv);
+		if (ret) {
+			dev_err(dev, "failed to request outbox IRQ: %d\n", ret);
+			return ret;
+		}
+
+		supp = (unsigned long) of_device_get_match_data(dev);
+		if (!supp) {
+			dev_err(dev, "no supplementary outbox specified\n");
+			return -ENODEV;
+		}
+		priv->supp_base = priv->outbox_base + (SPRD_OUTBOX_BASE_SPAN * supp);
+	}
+
 	/* Get the default outbox FIFO depth */
 	priv->outbox_fifo_depth =
 		readl(priv->outbox_base + SPRD_MBOX_FIFO_DEPTH) + 1;
@@ -354,7 +410,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id sprd_mbox_of_match[] = {
-	{ .compatible = "sprd,sc9860-mailbox", },
+	{ .compatible = "sprd,sc9860-mailbox",
+	  .data = (void *)SPRD_SUPP_INBOX_ID_SC9860 },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, sprd_mbox_of_match);
-- 
2.7.4

[PATCH 3/3] dt-bindings: mailbox: Add interrupt-names to SPRD mailbox

[Orson Zhai]

From: Orson Zhai <orson.zhai@unisoc.com>

We add an optional supp-outbox interrupt support to driver and change to
describe interrupts with names for easy configuration in device tree files.

Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
---
 Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml
index 26a5cca..bb775a4 100644
--- a/Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml
+++ b/Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml
@@ -22,9 +22,15 @@ properties:
       - description: outbox registers' base address
 
   interrupts:
+    minItems: 2
+    maxItems: 3
+
+  interrupt-names:
     items:
-      - description: inbox interrupt
-      - description: outbox interrupt
+      enum:
+        - inbox
+        - outbox
+        - supp-outbox
 
   clocks:
     maxItems: 1
@@ -40,6 +46,7 @@ required:
   - compatible
   - reg
   - interrupts
+  - interrupt-names
   - "#mbox-cells"
   - clocks
   - clock-names
@@ -56,5 +63,6 @@ examples:
       clock-names = "enable";
       clocks = <&aon_gate 53>;
       interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
+      interrupt-names = "inbox", "outbox";
     };
 ...
-- 
2.7.4

Re: [PATCH 1/3] mailbox: sprd: Introduce refcnt when clients requests/free channels

[Baolin Wang]

Hi Orson,

On Mon, Feb 8, 2021 at 7:52 PM Orson Zhai <orsonzhai@gmail.com> wrote:
>
> From: Orson Zhai <orson.zhai@unisoc.com>
>
> Unisoc mailbox has no way to be enabled/disabled for any single channel.
> They can only be set to startup or shutdown as a whole device at same time.
>
> Add a variable to count references to avoid mailbox FIFO being reset
> unexpectedly when clients are requesting or freeing channels.
>
> Also add a lock to dismiss possible conflicts from register r/w in
> different startup or shutdown threads.
>
> Fixes: ca27fc26cd22 ("mailbox: sprd: Add Spreadtrum mailbox driver")
> Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
> ---
>  drivers/mailbox/sprd-mailbox.c | 38 +++++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mailbox/sprd-mailbox.c b/drivers/mailbox/sprd-mailbox.c
> index f6fab24..e606f52 100644
> --- a/drivers/mailbox/sprd-mailbox.c
> +++ b/drivers/mailbox/sprd-mailbox.c
> @@ -60,6 +60,8 @@ struct sprd_mbox_priv {
>         struct clk              *clk;
>         u32                     outbox_fifo_depth;
>
> +       struct mutex            lock;
> +       u32                     refcnt;
>         struct mbox_chan        chan[SPRD_MBOX_CHAN_MAX];
>  };
>
> @@ -215,18 +217,22 @@ static int sprd_mbox_startup(struct mbox_chan *chan)
>         struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
>         u32 val;
>
> -       /* Select outbox FIFO mode and reset the outbox FIFO status */
> -       writel(0x0, priv->outbox_base + SPRD_MBOX_FIFO_RST);
> +       mutex_lock(&priv->lock);
> +       if (priv->refcnt++ == 0) {
> +               /* Select outbox FIFO mode and reset the outbox FIFO status */
> +               writel(0x0, priv->outbox_base + SPRD_MBOX_FIFO_RST);
>
> -       /* Enable inbox FIFO overflow and delivery interrupt */
> -       val = readl(priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> -       val &= ~(SPRD_INBOX_FIFO_OVERFLOW_IRQ | SPRD_INBOX_FIFO_DELIVER_IRQ);
> -       writel(val, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> +               /* Enable inbox FIFO overflow and delivery interrupt */
> +               val = readl(priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> +               val &= ~(SPRD_INBOX_FIFO_OVERFLOW_IRQ | SPRD_INBOX_FIFO_DELIVER_IRQ);
> +               writel(val, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
>
> -       /* Enable outbox FIFO not empty interrupt */
> -       val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> -       val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> -       writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> +               /* Enable outbox FIFO not empty interrupt */
> +               val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> +               val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> +               writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> +       }
> +       mutex_unlock(&priv->lock);

I think using the atomic_add/sub_and_test() related APIs can remove
the mutex lock.

>
>         return 0;
>  }
> @@ -235,9 +241,13 @@ static void sprd_mbox_shutdown(struct mbox_chan *chan)
>  {
>         struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
>
> -       /* Disable inbox & outbox interrupt */
> -       writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> -       writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> +       mutex_lock(&priv->lock);
> +       if (--priv->refcnt == 0) {
> +               /* Disable inbox & outbox interrupt */
> +               writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> +               writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> +       }
> +       mutex_unlock(&priv->lock);
>  }
>
>  static const struct mbox_chan_ops sprd_mbox_ops = {
> @@ -266,6 +276,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>
>         priv->dev = dev;
> +       priv->refcnt = 0;

No need to do this, the priv structure is already cleared to 0.

> +       mutex_init(&priv->lock);
>
>         /*
>          * The Spreadtrum mailbox uses an inbox to send messages to the target
> --
> 2.7.4
>


-- 
Baolin Wang

Re: [PATCH 2/3] mailbox: sprd: Add supplementary inbox support

[Baolin Wang]

On Mon, Feb 8, 2021 at 7:52 PM Orson Zhai <orsonzhai@gmail.com> wrote:
>
> From: Orson Zhai <orson.zhai@unisoc.com>
>
> Some sensors connected to Unisoc mailbox will send data very frequently.
> This makes channel 0 very busy and the messages from other remote cores
> not able to be handled as soon as possible.
>
> Then a supplementary inbox is added to the host core side for transferring
> mass but not emergency messages from the remote cores, such as step
> counting sensor, with an independent FIFO and interrupt.

So this is another part of the mailbox hardware, containing a batch of
hardware channels? I did not see it before, its function is similar
with inbox/outbox?

>
> Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
> ---
>  drivers/mailbox/sprd-mailbox.c | 93 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 75 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mailbox/sprd-mailbox.c b/drivers/mailbox/sprd-mailbox.c
> index e606f52..74648db 100644
> --- a/drivers/mailbox/sprd-mailbox.c
> +++ b/drivers/mailbox/sprd-mailbox.c
> @@ -11,6 +11,7 @@
>  #include <linux/io.h>
>  #include <linux/mailbox_controller.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/clk.h>
>
> @@ -50,13 +51,17 @@
>  #define SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ         BIT(0)
>  #define SPRD_OUTBOX_FIFO_IRQ_MASK              GENMASK(4, 0)
>
> +#define SPRD_OUTBOX_BASE_SPAN                  0x1000
>  #define SPRD_MBOX_CHAN_MAX                     8
> +#define SPRD_SUPP_INBOX_ID_SC9860              6
>
>  struct sprd_mbox_priv {
>         struct mbox_controller  mbox;
>         struct device           *dev;
>         void __iomem            *inbox_base;
>         void __iomem            *outbox_base;
> +       /*  Base register address for supplementary outbox */
> +       void __iomem            *supp_base;
>         struct clk              *clk;
>         u32                     outbox_fifo_depth;
>
> @@ -96,14 +101,13 @@ static u32 sprd_mbox_get_fifo_len(struct sprd_mbox_priv *priv, u32 fifo_sts)
>         return fifo_len;
>  }
>
> -static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
> +static inline irqreturn_t do_outbox_isr(void __iomem *base, struct sprd_mbox_priv *priv)

No need to add an explicit 'inline' tag, the compiler can do the smart
things than us.

>  {
> -       struct sprd_mbox_priv *priv = data;
>         struct mbox_chan *chan;
>         u32 fifo_sts, fifo_len, msg[2];
>         int i, id;
>
> -       fifo_sts = readl(priv->outbox_base + SPRD_MBOX_FIFO_STS);
> +       fifo_sts = readl(base + SPRD_MBOX_FIFO_STS);
>
>         fifo_len = sprd_mbox_get_fifo_len(priv, fifo_sts);
>         if (!fifo_len) {
> @@ -112,23 +116,41 @@ static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
>         }
>
>         for (i = 0; i < fifo_len; i++) {
> -               msg[0] = readl(priv->outbox_base + SPRD_MBOX_MSG_LOW);
> -               msg[1] = readl(priv->outbox_base + SPRD_MBOX_MSG_HIGH);
> -               id = readl(priv->outbox_base + SPRD_MBOX_ID);
> +               msg[0] = readl(base + SPRD_MBOX_MSG_LOW);
> +               msg[1] = readl(base + SPRD_MBOX_MSG_HIGH);
> +               id = readl(base + SPRD_MBOX_ID);
>
>                 chan = &priv->chan[id];
> -               mbox_chan_received_data(chan, (void *)msg);
> +               if (chan->cl)
> +                       mbox_chan_received_data(chan, (void *)msg);
> +               else
> +                       dev_warn_ratelimited(priv->dev,
> +                                   "message's been dropped at ch[%d]\n", id);
>
>                 /* Trigger to update outbox FIFO pointer */
> -               writel(0x1, priv->outbox_base + SPRD_MBOX_TRIGGER);
> +               writel(0x1, base + SPRD_MBOX_TRIGGER);
>         }
>
>         /* Clear irq status after reading all message. */
> -       writel(SPRD_MBOX_IRQ_CLR, priv->outbox_base + SPRD_MBOX_IRQ_STS);
> +       writel(SPRD_MBOX_IRQ_CLR, base + SPRD_MBOX_IRQ_STS);
>
>         return IRQ_HANDLED;
>  }
>
> +static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
> +{
> +       struct sprd_mbox_priv *priv = data;
> +
> +       return do_outbox_isr(priv->outbox_base, priv);
> +}
> +
> +static irqreturn_t sprd_mbox_supp_isr(int irq, void *data)
> +{
> +       struct sprd_mbox_priv *priv = data;
> +
> +       return do_outbox_isr(priv->supp_base, priv);
> +}
> +
>  static irqreturn_t sprd_mbox_inbox_isr(int irq, void *data)
>  {
>         struct sprd_mbox_priv *priv = data;
> @@ -231,6 +253,14 @@ static int sprd_mbox_startup(struct mbox_chan *chan)
>                 val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
>                 val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
>                 writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> +
> +               /* Enable supplementary outbox as the fundamental one */
> +               if (priv->supp_base) {
> +                       writel(0x0, priv->supp_base + SPRD_MBOX_FIFO_RST);
> +                       val = readl(priv->supp_base + SPRD_MBOX_IRQ_MSK);
> +                       val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> +                       writel(val, priv->supp_base + SPRD_MBOX_IRQ_MSK);
> +               }
>         }
>         mutex_unlock(&priv->lock);
>
> @@ -246,6 +276,10 @@ static void sprd_mbox_shutdown(struct mbox_chan *chan)
>                 /* Disable inbox & outbox interrupt */
>                 writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
>                 writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> +
> +               if (priv->supp_base)
> +                       writel(SPRD_OUTBOX_FIFO_IRQ_MASK,
> +                              priv->supp_base + SPRD_MBOX_IRQ_MSK);
>         }
>         mutex_unlock(&priv->lock);
>  }
> @@ -268,8 +302,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         struct sprd_mbox_priv *priv;
> -       int ret, inbox_irq, outbox_irq;
> -       unsigned long id;
> +       int ret, inbox_irq, outbox_irq, supp_irq;
> +       unsigned long id, supp;
>
>         priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>         if (!priv)
> @@ -280,11 +314,15 @@ static int sprd_mbox_probe(struct platform_device *pdev)
>         mutex_init(&priv->lock);
>
>         /*
> -        * The Spreadtrum mailbox uses an inbox to send messages to the target
> -        * core, and uses an outbox to receive messages from other cores.
> +        * Unisoc mailbox uses an inbox to send messages to the target
> +        * core, and uses (an) outbox(es) to receive messages from other
> +        * cores.
> +        *
> +        * Thus in general the mailbox controller supplies 2 different
> +        * register addresses and IRQ numbers for inbox and outbox.
>          *
> -        * Thus the mailbox controller supplies 2 different register addresses
> -        * and IRQ numbers for inbox and outbox.
> +        * If necessary, a supplementary inbox could be enabled optionally
> +        * with an independent FIFO and an extra interrupt.
>          */
>         priv->inbox_base = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(priv->inbox_base))
> @@ -310,7 +348,7 @@ static int sprd_mbox_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> -       inbox_irq = platform_get_irq(pdev, 0);
> +       inbox_irq = platform_get_irq_byname(pdev, "inbox");

I think you should put the dt changes before this patch.

>         if (inbox_irq < 0)
>                 return inbox_irq;
>
> @@ -321,7 +359,7 @@ static int sprd_mbox_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> -       outbox_irq = platform_get_irq(pdev, 1);
> +       outbox_irq = platform_get_irq_byname(pdev, "outbox");
>         if (outbox_irq < 0)
>                 return outbox_irq;
>
> @@ -332,6 +370,24 @@ static int sprd_mbox_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       /* Supplementary outbox IRQ is optional */
> +       supp_irq = platform_get_irq_byname(pdev, "supp-outbox");
> +       if (supp_irq > 0) {
> +               ret = devm_request_irq(dev, supp_irq, sprd_mbox_supp_isr,
> +                                      IRQF_NO_SUSPEND, dev_name(dev), priv);
> +               if (ret) {
> +                       dev_err(dev, "failed to request outbox IRQ: %d\n", ret);
> +                       return ret;
> +               }
> +
> +               supp = (unsigned long) of_device_get_match_data(dev);
> +               if (!supp) {
> +                       dev_err(dev, "no supplementary outbox specified\n");
> +                       return -ENODEV;
> +               }
> +               priv->supp_base = priv->outbox_base + (SPRD_OUTBOX_BASE_SPAN * supp);
> +       }
> +
>         /* Get the default outbox FIFO depth */
>         priv->outbox_fifo_depth =
>                 readl(priv->outbox_base + SPRD_MBOX_FIFO_DEPTH) + 1;
> @@ -354,7 +410,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
>  }
>
>  static const struct of_device_id sprd_mbox_of_match[] = {
> -       { .compatible = "sprd,sc9860-mailbox", },
> +       { .compatible = "sprd,sc9860-mailbox",
> +         .data = (void *)SPRD_SUPP_INBOX_ID_SC9860 },
>         { },
>  };
>  MODULE_DEVICE_TABLE(of, sprd_mbox_of_match);
> --
> 2.7.4
>


-- 
Baolin Wang

Re: [PATCH 3/3] dt-bindings: mailbox: Add interrupt-names to SPRD mailbox

[Rob Herring]

On Mon, 08 Feb 2021 19:51:04 +0800, Orson Zhai wrote:
> From: Orson Zhai <orson.zhai@unisoc.com>
> 
> We add an optional supp-outbox interrupt support to driver and change to
> describe interrupts with names for easy configuration in device tree files.
> 
> Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
> ---
>  Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml: properties:interrupt-names:items: {'enum': ['inbox', 'outbox', 'supp-outbox']} is not of type 'array'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml: ignoring, error in schema: properties: interrupt-names: items
warning: no schema found in file: ./Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml

See https://patchwork.ozlabs.org/patch/1437629

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/3] mailbox: sprd: Introduce refcnt when clients requests/free channels

[Orson Zhai]

On Mon, Feb 08, 2021 at 10:06:47PM +0800, Baolin Wang wrote:
> Hi Orson,
> 
> On Mon, Feb 8, 2021 at 7:52 PM Orson Zhai <orsonzhai@gmail.com> wrote:
> >
> > From: Orson Zhai <orson.zhai@unisoc.com>
> >
> > Unisoc mailbox has no way to be enabled/disabled for any single channel.
> > They can only be set to startup or shutdown as a whole device at same time.
> >
> > Add a variable to count references to avoid mailbox FIFO being reset
> > unexpectedly when clients are requesting or freeing channels.
> >
> > Also add a lock to dismiss possible conflicts from register r/w in
> > different startup or shutdown threads.
> >
> > Fixes: ca27fc26cd22 ("mailbox: sprd: Add Spreadtrum mailbox driver")
> > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
> > ---
> >  drivers/mailbox/sprd-mailbox.c | 38 +++++++++++++++++++++++++-------------
> >  1 file changed, 25 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/mailbox/sprd-mailbox.c b/drivers/mailbox/sprd-mailbox.c
> > index f6fab24..e606f52 100644
> > --- a/drivers/mailbox/sprd-mailbox.c
> > +++ b/drivers/mailbox/sprd-mailbox.c
> > @@ -60,6 +60,8 @@ struct sprd_mbox_priv {
> >         struct clk              *clk;
> >         u32                     outbox_fifo_depth;
> >
> > +       struct mutex            lock;
> > +       u32                     refcnt;
> >         struct mbox_chan        chan[SPRD_MBOX_CHAN_MAX];
> >  };
> >
> > @@ -215,18 +217,22 @@ static int sprd_mbox_startup(struct mbox_chan *chan)
> >         struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
> >         u32 val;
> >
> > -       /* Select outbox FIFO mode and reset the outbox FIFO status */
> > -       writel(0x0, priv->outbox_base + SPRD_MBOX_FIFO_RST);
> > +       mutex_lock(&priv->lock);
> > +       if (priv->refcnt++ == 0) {
> > +               /* Select outbox FIFO mode and reset the outbox FIFO status */
> > +               writel(0x0, priv->outbox_base + SPRD_MBOX_FIFO_RST);
> >
> > -       /* Enable inbox FIFO overflow and delivery interrupt */
> > -       val = readl(priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > -       val &= ~(SPRD_INBOX_FIFO_OVERFLOW_IRQ | SPRD_INBOX_FIFO_DELIVER_IRQ);
> > -       writel(val, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > +               /* Enable inbox FIFO overflow and delivery interrupt */
> > +               val = readl(priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > +               val &= ~(SPRD_INBOX_FIFO_OVERFLOW_IRQ | SPRD_INBOX_FIFO_DELIVER_IRQ);
> > +               writel(val, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> >
> > -       /* Enable outbox FIFO not empty interrupt */
> > -       val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > -       val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> > -       writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > +               /* Enable outbox FIFO not empty interrupt */
> > +               val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > +               val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> > +               writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > +       }
> > +       mutex_unlock(&priv->lock);
> 
> I think using the atomic_add/sub_and_test() related APIs can remove
> the mutex lock.

Yes, atomic could make refcnt itself safe. But mutex lock is to make whole processing of
reading/writing registers safe.

Consider case like this:

  channel #1             channel #2
-------------------------------------
   startup
   .....
   shutdown               startup
     |-refcnt==0            |
     |                      |-retcnt+1
     |                      |-enable mailbox
     |-disable mailbox 

Mailbox will be wrongly disabled after client requests channel #2's startup.

> 
> >
> >         return 0;
> >  }
> > @@ -235,9 +241,13 @@ static void sprd_mbox_shutdown(struct mbox_chan *chan)
> >  {
> >         struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
> >
> > -       /* Disable inbox & outbox interrupt */
> > -       writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > -       writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > +       mutex_lock(&priv->lock);
> > +       if (--priv->refcnt == 0) {
> > +               /* Disable inbox & outbox interrupt */
> > +               writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > +               writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > +       }
> > +       mutex_unlock(&priv->lock);
> >  }
> >
> >  static const struct mbox_chan_ops sprd_mbox_ops = {
> > @@ -266,6 +276,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> >                 return -ENOMEM;
> >
> >         priv->dev = dev;
> > +       priv->refcnt = 0;
> 
> No need to do this, the priv structure is already cleared to 0.

Right, will remove at next version.

Thanks for reviewing!

-Orson

> 
> > +       mutex_init(&priv->lock);
> >
> >         /*
> >          * The Spreadtrum mailbox uses an inbox to send messages to the target
> > --
> > 2.7.4
> >
> 
> 
> -- 
> Baolin Wang

Re: [PATCH 2/3] mailbox: sprd: Add supplementary inbox support

[Orson Zhai]

On Mon, Feb 08, 2021 at 10:27:47PM +0800, Baolin Wang wrote:
> On Mon, Feb 8, 2021 at 7:52 PM Orson Zhai <orsonzhai@gmail.com> wrote:
> >
> > From: Orson Zhai <orson.zhai@unisoc.com>
> >
> > Some sensors connected to Unisoc mailbox will send data very frequently.
> > This makes channel 0 very busy and the messages from other remote cores
> > not able to be handled as soon as possible.
> >
> > Then a supplementary inbox is added to the host core side for transferring
> > mass but not emergency messages from the remote cores, such as step
> > counting sensor, with an independent FIFO and interrupt.
> 
> So this is another part of the mailbox hardware, containing a batch of
> hardware channels?

No. Actually it is an inbox assigned to one of the remote cores before but
being exposed to host cpu core for now. 

> I did not see it before, its function is similar
> with inbox/outbox?

Exactly same with any other channel.
But only the part of outbox is exposed to host side. Inbox part of this channel
is still kept for original remote core to use.

It's a trick (un-documented) from our ASIC designers to resolve some special requirements.
I was also shocked when hearing it :)

I guess other vendors will add another mailbox module to resovle this, but our guys might
consider the hardware cost...
 
> 
> >
> > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
> > ---
> >  drivers/mailbox/sprd-mailbox.c | 93 ++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 75 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mailbox/sprd-mailbox.c b/drivers/mailbox/sprd-mailbox.c
> > index e606f52..74648db 100644
> > --- a/drivers/mailbox/sprd-mailbox.c
> > +++ b/drivers/mailbox/sprd-mailbox.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/io.h>
> >  #include <linux/mailbox_controller.h>
> >  #include <linux/module.h>
> > +#include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/clk.h>
> >
> > @@ -50,13 +51,17 @@
> >  #define SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ         BIT(0)
> >  #define SPRD_OUTBOX_FIFO_IRQ_MASK              GENMASK(4, 0)
> >
> > +#define SPRD_OUTBOX_BASE_SPAN                  0x1000
> >  #define SPRD_MBOX_CHAN_MAX                     8
> > +#define SPRD_SUPP_INBOX_ID_SC9860              6
> >
> >  struct sprd_mbox_priv {
> >         struct mbox_controller  mbox;
> >         struct device           *dev;
> >         void __iomem            *inbox_base;
> >         void __iomem            *outbox_base;
> > +       /*  Base register address for supplementary outbox */
> > +       void __iomem            *supp_base;
> >         struct clk              *clk;
> >         u32                     outbox_fifo_depth;
> >
> > @@ -96,14 +101,13 @@ static u32 sprd_mbox_get_fifo_len(struct sprd_mbox_priv *priv, u32 fifo_sts)
> >         return fifo_len;
> >  }
> >
> > -static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
> > +static inline irqreturn_t do_outbox_isr(void __iomem *base, struct sprd_mbox_priv *priv)
> 
> No need to add an explicit 'inline' tag, the compiler can do the smart
> things than us.

I thought it will help to increase perfermance of isr execution before.

Will fix at next.

> 
> >  {
> > -       struct sprd_mbox_priv *priv = data;
> >         struct mbox_chan *chan;
> >         u32 fifo_sts, fifo_len, msg[2];
> >         int i, id;
> >
> > -       fifo_sts = readl(priv->outbox_base + SPRD_MBOX_FIFO_STS);
> > +       fifo_sts = readl(base + SPRD_MBOX_FIFO_STS);
> >
> >         fifo_len = sprd_mbox_get_fifo_len(priv, fifo_sts);
> >         if (!fifo_len) {
> > @@ -112,23 +116,41 @@ static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
> >         }
> >
> >         for (i = 0; i < fifo_len; i++) {
> > -               msg[0] = readl(priv->outbox_base + SPRD_MBOX_MSG_LOW);
> > -               msg[1] = readl(priv->outbox_base + SPRD_MBOX_MSG_HIGH);
> > -               id = readl(priv->outbox_base + SPRD_MBOX_ID);
> > +               msg[0] = readl(base + SPRD_MBOX_MSG_LOW);
> > +               msg[1] = readl(base + SPRD_MBOX_MSG_HIGH);
> > +               id = readl(base + SPRD_MBOX_ID);
> >
> >                 chan = &priv->chan[id];
> > -               mbox_chan_received_data(chan, (void *)msg);
> > +               if (chan->cl)
> > +                       mbox_chan_received_data(chan, (void *)msg);
> > +               else
> > +                       dev_warn_ratelimited(priv->dev,
> > +                                   "message's been dropped at ch[%d]\n", id);
> >
> >                 /* Trigger to update outbox FIFO pointer */
> > -               writel(0x1, priv->outbox_base + SPRD_MBOX_TRIGGER);
> > +               writel(0x1, base + SPRD_MBOX_TRIGGER);
> >         }
> >
> >         /* Clear irq status after reading all message. */
> > -       writel(SPRD_MBOX_IRQ_CLR, priv->outbox_base + SPRD_MBOX_IRQ_STS);
> > +       writel(SPRD_MBOX_IRQ_CLR, base + SPRD_MBOX_IRQ_STS);
> >
> >         return IRQ_HANDLED;
> >  }
> >
> > +static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
> > +{
> > +       struct sprd_mbox_priv *priv = data;
> > +
> > +       return do_outbox_isr(priv->outbox_base, priv);
> > +}
> > +
> > +static irqreturn_t sprd_mbox_supp_isr(int irq, void *data)
> > +{
> > +       struct sprd_mbox_priv *priv = data;
> > +
> > +       return do_outbox_isr(priv->supp_base, priv);
> > +}
> > +
> >  static irqreturn_t sprd_mbox_inbox_isr(int irq, void *data)
> >  {
> >         struct sprd_mbox_priv *priv = data;
> > @@ -231,6 +253,14 @@ static int sprd_mbox_startup(struct mbox_chan *chan)
> >                 val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> >                 val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> >                 writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > +
> > +               /* Enable supplementary outbox as the fundamental one */
> > +               if (priv->supp_base) {
> > +                       writel(0x0, priv->supp_base + SPRD_MBOX_FIFO_RST);
> > +                       val = readl(priv->supp_base + SPRD_MBOX_IRQ_MSK);
> > +                       val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> > +                       writel(val, priv->supp_base + SPRD_MBOX_IRQ_MSK);
> > +               }
> >         }
> >         mutex_unlock(&priv->lock);
> >
> > @@ -246,6 +276,10 @@ static void sprd_mbox_shutdown(struct mbox_chan *chan)
> >                 /* Disable inbox & outbox interrupt */
> >                 writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> >                 writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > +
> > +               if (priv->supp_base)
> > +                       writel(SPRD_OUTBOX_FIFO_IRQ_MASK,
> > +                              priv->supp_base + SPRD_MBOX_IRQ_MSK);
> >         }
> >         mutex_unlock(&priv->lock);
> >  }
> > @@ -268,8 +302,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> >  {
> >         struct device *dev = &pdev->dev;
> >         struct sprd_mbox_priv *priv;
> > -       int ret, inbox_irq, outbox_irq;
> > -       unsigned long id;
> > +       int ret, inbox_irq, outbox_irq, supp_irq;
> > +       unsigned long id, supp;
> >
> >         priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >         if (!priv)
> > @@ -280,11 +314,15 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> >         mutex_init(&priv->lock);
> >
> >         /*
> > -        * The Spreadtrum mailbox uses an inbox to send messages to the target
> > -        * core, and uses an outbox to receive messages from other cores.
> > +        * Unisoc mailbox uses an inbox to send messages to the target
> > +        * core, and uses (an) outbox(es) to receive messages from other
> > +        * cores.
> > +        *
> > +        * Thus in general the mailbox controller supplies 2 different
> > +        * register addresses and IRQ numbers for inbox and outbox.
> >          *
> > -        * Thus the mailbox controller supplies 2 different register addresses
> > -        * and IRQ numbers for inbox and outbox.
> > +        * If necessary, a supplementary inbox could be enabled optionally
> > +        * with an independent FIFO and an extra interrupt.
> >          */
> >         priv->inbox_base = devm_platform_ioremap_resource(pdev, 0);
> >         if (IS_ERR(priv->inbox_base))
> > @@ -310,7 +348,7 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> >                 return ret;
> >         }
> >
> > -       inbox_irq = platform_get_irq(pdev, 0);
> > +       inbox_irq = platform_get_irq_byname(pdev, "inbox");
> 
> I think you should put the dt changes before this patch.

OK.

Thanks,
Orson
> 
> >         if (inbox_irq < 0)
> >                 return inbox_irq;
> >
> > @@ -321,7 +359,7 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> >                 return ret;
> >         }
> >
> > -       outbox_irq = platform_get_irq(pdev, 1);
> > +       outbox_irq = platform_get_irq_byname(pdev, "outbox");
> >         if (outbox_irq < 0)
> >                 return outbox_irq;
> >
> > @@ -332,6 +370,24 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> >                 return ret;
> >         }
> >
> > +       /* Supplementary outbox IRQ is optional */
> > +       supp_irq = platform_get_irq_byname(pdev, "supp-outbox");
> > +       if (supp_irq > 0) {
> > +               ret = devm_request_irq(dev, supp_irq, sprd_mbox_supp_isr,
> > +                                      IRQF_NO_SUSPEND, dev_name(dev), priv);
> > +               if (ret) {
> > +                       dev_err(dev, "failed to request outbox IRQ: %d\n", ret);
> > +                       return ret;
> > +               }
> > +
> > +               supp = (unsigned long) of_device_get_match_data(dev);
> > +               if (!supp) {
> > +                       dev_err(dev, "no supplementary outbox specified\n");
> > +                       return -ENODEV;
> > +               }
> > +               priv->supp_base = priv->outbox_base + (SPRD_OUTBOX_BASE_SPAN * supp);
> > +       }
> > +
> >         /* Get the default outbox FIFO depth */
> >         priv->outbox_fifo_depth =
> >                 readl(priv->outbox_base + SPRD_MBOX_FIFO_DEPTH) + 1;
> > @@ -354,7 +410,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> >  }
> >
> >  static const struct of_device_id sprd_mbox_of_match[] = {
> > -       { .compatible = "sprd,sc9860-mailbox", },
> > +       { .compatible = "sprd,sc9860-mailbox",
> > +         .data = (void *)SPRD_SUPP_INBOX_ID_SC9860 },
> >         { },
> >  };
> >  MODULE_DEVICE_TABLE(of, sprd_mbox_of_match);
> > --
> > 2.7.4
> >
> 
> 
> -- 
> Baolin Wang

Re: [PATCH 1/3] mailbox: sprd: Introduce refcnt when clients requests/free channels

[Baolin Wang]

On Tue, Feb 9, 2021 at 11:28 AM Orson Zhai <orsonzhai@gmail.com> wrote:
>
> On Mon, Feb 08, 2021 at 10:06:47PM +0800, Baolin Wang wrote:
> > Hi Orson,
> >
> > On Mon, Feb 8, 2021 at 7:52 PM Orson Zhai <orsonzhai@gmail.com> wrote:
> > >
> > > From: Orson Zhai <orson.zhai@unisoc.com>
> > >
> > > Unisoc mailbox has no way to be enabled/disabled for any single channel.
> > > They can only be set to startup or shutdown as a whole device at same time.
> > >
> > > Add a variable to count references to avoid mailbox FIFO being reset
> > > unexpectedly when clients are requesting or freeing channels.
> > >
> > > Also add a lock to dismiss possible conflicts from register r/w in
> > > different startup or shutdown threads.
> > >
> > > Fixes: ca27fc26cd22 ("mailbox: sprd: Add Spreadtrum mailbox driver")
> > > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
> > > ---
> > >  drivers/mailbox/sprd-mailbox.c | 38 +++++++++++++++++++++++++-------------
> > >  1 file changed, 25 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/mailbox/sprd-mailbox.c b/drivers/mailbox/sprd-mailbox.c
> > > index f6fab24..e606f52 100644
> > > --- a/drivers/mailbox/sprd-mailbox.c
> > > +++ b/drivers/mailbox/sprd-mailbox.c
> > > @@ -60,6 +60,8 @@ struct sprd_mbox_priv {
> > >         struct clk              *clk;
> > >         u32                     outbox_fifo_depth;
> > >
> > > +       struct mutex            lock;
> > > +       u32                     refcnt;
> > >         struct mbox_chan        chan[SPRD_MBOX_CHAN_MAX];
> > >  };
> > >
> > > @@ -215,18 +217,22 @@ static int sprd_mbox_startup(struct mbox_chan *chan)
> > >         struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
> > >         u32 val;
> > >
> > > -       /* Select outbox FIFO mode and reset the outbox FIFO status */
> > > -       writel(0x0, priv->outbox_base + SPRD_MBOX_FIFO_RST);
> > > +       mutex_lock(&priv->lock);
> > > +       if (priv->refcnt++ == 0) {
> > > +               /* Select outbox FIFO mode and reset the outbox FIFO status */
> > > +               writel(0x0, priv->outbox_base + SPRD_MBOX_FIFO_RST);
> > >
> > > -       /* Enable inbox FIFO overflow and delivery interrupt */
> > > -       val = readl(priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > > -       val &= ~(SPRD_INBOX_FIFO_OVERFLOW_IRQ | SPRD_INBOX_FIFO_DELIVER_IRQ);
> > > -       writel(val, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > > +               /* Enable inbox FIFO overflow and delivery interrupt */
> > > +               val = readl(priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > > +               val &= ~(SPRD_INBOX_FIFO_OVERFLOW_IRQ | SPRD_INBOX_FIFO_DELIVER_IRQ);
> > > +               writel(val, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > >
> > > -       /* Enable outbox FIFO not empty interrupt */
> > > -       val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > > -       val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> > > -       writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > > +               /* Enable outbox FIFO not empty interrupt */
> > > +               val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > > +               val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> > > +               writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > > +       }
> > > +       mutex_unlock(&priv->lock);
> >
> > I think using the atomic_add/sub_and_test() related APIs can remove
> > the mutex lock.
>
> Yes, atomic could make refcnt itself safe. But mutex lock is to make whole processing of
> reading/writing registers safe.
>
> Consider case like this:
>
>   channel #1             channel #2
> -------------------------------------
>    startup
>    .....
>    shutdown               startup
>      |-refcnt==0            |
>      |                      |-retcnt+1
>      |                      |-enable mailbox
>      |-disable mailbox
>
> Mailbox will be wrongly disabled after client requests channel #2's startup.

Yeah, you are right. Sorry for noise.

> >
> > >
> > >         return 0;
> > >  }
> > > @@ -235,9 +241,13 @@ static void sprd_mbox_shutdown(struct mbox_chan *chan)
> > >  {
> > >         struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
> > >
> > > -       /* Disable inbox & outbox interrupt */
> > > -       writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > > -       writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > > +       mutex_lock(&priv->lock);
> > > +       if (--priv->refcnt == 0) {
> > > +               /* Disable inbox & outbox interrupt */
> > > +               writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > > +               writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > > +       }
> > > +       mutex_unlock(&priv->lock);
> > >  }
> > >
> > >  static const struct mbox_chan_ops sprd_mbox_ops = {
> > > @@ -266,6 +276,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> > >                 return -ENOMEM;
> > >
> > >         priv->dev = dev;
> > > +       priv->refcnt = 0;
> >
> > No need to do this, the priv structure is already cleared to 0.
>
> Right, will remove at next version.
>
> Thanks for reviewing!
>
> -Orson
>
> >
> > > +       mutex_init(&priv->lock);
> > >
> > >         /*
> > >          * The Spreadtrum mailbox uses an inbox to send messages to the target
> > > --
> > > 2.7.4
> > >
> >
> >
> > --
> > Baolin Wang



-- 
Baolin Wang

Re: [PATCH 2/3] mailbox: sprd: Add supplementary inbox support

[Baolin Wang]

On Tue, Feb 9, 2021 at 12:09 PM Orson Zhai <orsonzhai@gmail.com> wrote:
>
> On Mon, Feb 08, 2021 at 10:27:47PM +0800, Baolin Wang wrote:
> > On Mon, Feb 8, 2021 at 7:52 PM Orson Zhai <orsonzhai@gmail.com> wrote:
> > >
> > > From: Orson Zhai <orson.zhai@unisoc.com>
> > >
> > > Some sensors connected to Unisoc mailbox will send data very frequently.
> > > This makes channel 0 very busy and the messages from other remote cores
> > > not able to be handled as soon as possible.
> > >
> > > Then a supplementary inbox is added to the host core side for transferring
> > > mass but not emergency messages from the remote cores, such as step
> > > counting sensor, with an independent FIFO and interrupt.
> >
> > So this is another part of the mailbox hardware, containing a batch of
> > hardware channels?
>
> No. Actually it is an inbox assigned to one of the remote cores before but
> being exposed to host cpu core for now.
>
> > I did not see it before, its function is similar
> > with inbox/outbox?
>
> Exactly same with any other channel.
> But only the part of outbox is exposed to host side. Inbox part of this channel
> is still kept for original remote core to use.
>
> It's a trick (un-documented) from our ASIC designers to resolve some special requirements.
> I was also shocked when hearing it :)

Understood :)

>
> I guess other vendors will add another mailbox module to resovle this, but our guys might
> consider the hardware cost...

OK. Thanks for your explanation. It will be helpful if you can add
these backgroud into the comments in case someone else will be
confusing again for the new supplementary inbox :)

> > >
> > > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
> > > ---
> > >  drivers/mailbox/sprd-mailbox.c | 93 ++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 75 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/mailbox/sprd-mailbox.c b/drivers/mailbox/sprd-mailbox.c
> > > index e606f52..74648db 100644
> > > --- a/drivers/mailbox/sprd-mailbox.c
> > > +++ b/drivers/mailbox/sprd-mailbox.c
> > > @@ -11,6 +11,7 @@
> > >  #include <linux/io.h>
> > >  #include <linux/mailbox_controller.h>
> > >  #include <linux/module.h>
> > > +#include <linux/of_device.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/clk.h>
> > >
> > > @@ -50,13 +51,17 @@
> > >  #define SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ         BIT(0)
> > >  #define SPRD_OUTBOX_FIFO_IRQ_MASK              GENMASK(4, 0)
> > >
> > > +#define SPRD_OUTBOX_BASE_SPAN                  0x1000
> > >  #define SPRD_MBOX_CHAN_MAX                     8
> > > +#define SPRD_SUPP_INBOX_ID_SC9860              6
> > >
> > >  struct sprd_mbox_priv {
> > >         struct mbox_controller  mbox;
> > >         struct device           *dev;
> > >         void __iomem            *inbox_base;
> > >         void __iomem            *outbox_base;
> > > +       /*  Base register address for supplementary outbox */
> > > +       void __iomem            *supp_base;
> > >         struct clk              *clk;
> > >         u32                     outbox_fifo_depth;
> > >
> > > @@ -96,14 +101,13 @@ static u32 sprd_mbox_get_fifo_len(struct sprd_mbox_priv *priv, u32 fifo_sts)
> > >         return fifo_len;
> > >  }
> > >
> > > -static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
> > > +static inline irqreturn_t do_outbox_isr(void __iomem *base, struct sprd_mbox_priv *priv)
> >
> > No need to add an explicit 'inline' tag, the compiler can do the smart
> > things than us.
>
> I thought it will help to increase perfermance of isr execution before.
>
> Will fix at next.
>
> >
> > >  {
> > > -       struct sprd_mbox_priv *priv = data;
> > >         struct mbox_chan *chan;
> > >         u32 fifo_sts, fifo_len, msg[2];
> > >         int i, id;
> > >
> > > -       fifo_sts = readl(priv->outbox_base + SPRD_MBOX_FIFO_STS);
> > > +       fifo_sts = readl(base + SPRD_MBOX_FIFO_STS);
> > >
> > >         fifo_len = sprd_mbox_get_fifo_len(priv, fifo_sts);
> > >         if (!fifo_len) {
> > > @@ -112,23 +116,41 @@ static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
> > >         }
> > >
> > >         for (i = 0; i < fifo_len; i++) {
> > > -               msg[0] = readl(priv->outbox_base + SPRD_MBOX_MSG_LOW);
> > > -               msg[1] = readl(priv->outbox_base + SPRD_MBOX_MSG_HIGH);
> > > -               id = readl(priv->outbox_base + SPRD_MBOX_ID);
> > > +               msg[0] = readl(base + SPRD_MBOX_MSG_LOW);
> > > +               msg[1] = readl(base + SPRD_MBOX_MSG_HIGH);
> > > +               id = readl(base + SPRD_MBOX_ID);
> > >
> > >                 chan = &priv->chan[id];
> > > -               mbox_chan_received_data(chan, (void *)msg);
> > > +               if (chan->cl)
> > > +                       mbox_chan_received_data(chan, (void *)msg);
> > > +               else
> > > +                       dev_warn_ratelimited(priv->dev,
> > > +                                   "message's been dropped at ch[%d]\n", id);
> > >
> > >                 /* Trigger to update outbox FIFO pointer */
> > > -               writel(0x1, priv->outbox_base + SPRD_MBOX_TRIGGER);
> > > +               writel(0x1, base + SPRD_MBOX_TRIGGER);
> > >         }
> > >
> > >         /* Clear irq status after reading all message. */
> > > -       writel(SPRD_MBOX_IRQ_CLR, priv->outbox_base + SPRD_MBOX_IRQ_STS);
> > > +       writel(SPRD_MBOX_IRQ_CLR, base + SPRD_MBOX_IRQ_STS);
> > >
> > >         return IRQ_HANDLED;
> > >  }
> > >
> > > +static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
> > > +{
> > > +       struct sprd_mbox_priv *priv = data;
> > > +
> > > +       return do_outbox_isr(priv->outbox_base, priv);
> > > +}
> > > +
> > > +static irqreturn_t sprd_mbox_supp_isr(int irq, void *data)
> > > +{
> > > +       struct sprd_mbox_priv *priv = data;
> > > +
> > > +       return do_outbox_isr(priv->supp_base, priv);
> > > +}
> > > +
> > >  static irqreturn_t sprd_mbox_inbox_isr(int irq, void *data)
> > >  {
> > >         struct sprd_mbox_priv *priv = data;
> > > @@ -231,6 +253,14 @@ static int sprd_mbox_startup(struct mbox_chan *chan)
> > >                 val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > >                 val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> > >                 writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > > +
> > > +               /* Enable supplementary outbox as the fundamental one */
> > > +               if (priv->supp_base) {
> > > +                       writel(0x0, priv->supp_base + SPRD_MBOX_FIFO_RST);
> > > +                       val = readl(priv->supp_base + SPRD_MBOX_IRQ_MSK);
> > > +                       val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
> > > +                       writel(val, priv->supp_base + SPRD_MBOX_IRQ_MSK);
> > > +               }
> > >         }
> > >         mutex_unlock(&priv->lock);
> > >
> > > @@ -246,6 +276,10 @@ static void sprd_mbox_shutdown(struct mbox_chan *chan)
> > >                 /* Disable inbox & outbox interrupt */
> > >                 writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
> > >                 writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
> > > +
> > > +               if (priv->supp_base)
> > > +                       writel(SPRD_OUTBOX_FIFO_IRQ_MASK,
> > > +                              priv->supp_base + SPRD_MBOX_IRQ_MSK);
> > >         }
> > >         mutex_unlock(&priv->lock);
> > >  }
> > > @@ -268,8 +302,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> > >  {
> > >         struct device *dev = &pdev->dev;
> > >         struct sprd_mbox_priv *priv;
> > > -       int ret, inbox_irq, outbox_irq;
> > > -       unsigned long id;
> > > +       int ret, inbox_irq, outbox_irq, supp_irq;
> > > +       unsigned long id, supp;
> > >
> > >         priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > >         if (!priv)
> > > @@ -280,11 +314,15 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> > >         mutex_init(&priv->lock);
> > >
> > >         /*
> > > -        * The Spreadtrum mailbox uses an inbox to send messages to the target
> > > -        * core, and uses an outbox to receive messages from other cores.
> > > +        * Unisoc mailbox uses an inbox to send messages to the target
> > > +        * core, and uses (an) outbox(es) to receive messages from other
> > > +        * cores.
> > > +        *
> > > +        * Thus in general the mailbox controller supplies 2 different
> > > +        * register addresses and IRQ numbers for inbox and outbox.
> > >          *
> > > -        * Thus the mailbox controller supplies 2 different register addresses
> > > -        * and IRQ numbers for inbox and outbox.
> > > +        * If necessary, a supplementary inbox could be enabled optionally
> > > +        * with an independent FIFO and an extra interrupt.
> > >          */
> > >         priv->inbox_base = devm_platform_ioremap_resource(pdev, 0);
> > >         if (IS_ERR(priv->inbox_base))
> > > @@ -310,7 +348,7 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> > >                 return ret;
> > >         }
> > >
> > > -       inbox_irq = platform_get_irq(pdev, 0);
> > > +       inbox_irq = platform_get_irq_byname(pdev, "inbox");
> >
> > I think you should put the dt changes before this patch.
>
> OK.
>
> Thanks,
> Orson
> >
> > >         if (inbox_irq < 0)
> > >                 return inbox_irq;
> > >
> > > @@ -321,7 +359,7 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> > >                 return ret;
> > >         }
> > >
> > > -       outbox_irq = platform_get_irq(pdev, 1);
> > > +       outbox_irq = platform_get_irq_byname(pdev, "outbox");
> > >         if (outbox_irq < 0)
> > >                 return outbox_irq;
> > >
> > > @@ -332,6 +370,24 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> > >                 return ret;
> > >         }
> > >
> > > +       /* Supplementary outbox IRQ is optional */
> > > +       supp_irq = platform_get_irq_byname(pdev, "supp-outbox");
> > > +       if (supp_irq > 0) {
> > > +               ret = devm_request_irq(dev, supp_irq, sprd_mbox_supp_isr,
> > > +                                      IRQF_NO_SUSPEND, dev_name(dev), priv);
> > > +               if (ret) {
> > > +                       dev_err(dev, "failed to request outbox IRQ: %d\n", ret);
> > > +                       return ret;
> > > +               }
> > > +
> > > +               supp = (unsigned long) of_device_get_match_data(dev);
> > > +               if (!supp) {
> > > +                       dev_err(dev, "no supplementary outbox specified\n");
> > > +                       return -ENODEV;
> > > +               }
> > > +               priv->supp_base = priv->outbox_base + (SPRD_OUTBOX_BASE_SPAN * supp);
> > > +       }
> > > +
> > >         /* Get the default outbox FIFO depth */
> > >         priv->outbox_fifo_depth =
> > >                 readl(priv->outbox_base + SPRD_MBOX_FIFO_DEPTH) + 1;
> > > @@ -354,7 +410,8 @@ static int sprd_mbox_probe(struct platform_device *pdev)
> > >  }
> > >
> > >  static const struct of_device_id sprd_mbox_of_match[] = {
> > > -       { .compatible = "sprd,sc9860-mailbox", },
> > > +       { .compatible = "sprd,sc9860-mailbox",
> > > +         .data = (void *)SPRD_SUPP_INBOX_ID_SC9860 },
> > >         { },
> > >  };
> > >  MODULE_DEVICE_TABLE(of, sprd_mbox_of_match);
> > > --
> > > 2.7.4
> > >
> >
> >
> > --
> > Baolin Wang



-- 
Baolin Wang