On Tue, Jun 28, 2016 at 7:30 PM, Neil Armstrong [off-list ref] wrote:

On 06/25/2016 07:50 PM, Jassi Brar wrote:

quoted

-#define INTR_STAT_OFS  0x0
-#define INTR_SET_OFS   0x8
-#define INTR_CLR_OFS   0x10
-
-#define MHU_LP_OFFSET  0x0
-#define MHU_HP_OFFSET  0x20
-#define MHU_SEC_OFFSET 0x200
-#define TX_REG_OFFSET  0x100
+#define INTR_SET_OFS   0x0
+#define INTR_STAT_OFS  0x4
+#define INTR_CLR_OFS   0x8

-#define MHU_CHANS      3
+#define MHU_LP_OFFSET  0x10
+#define MHU_HP_OFFSET  0x1c
+
+#define TX_REG_OFFSET  0x24
+
+#define MHU_CHANS      2

^^^^^^^^ this is precisely the difference if we ignore cosmetic
differences. So the IP is essentially the same.

Well, no. The overall design is similar. but clearly it's a different IP.

If your this patch works on your platform, then the diff from arm_mhu
tells the controller is the same but only with re-ordered registers.
And you already call it 'MHU' :)

quoted

[snip]

quoted

-------------------------------------------------------------------------------------------
From: Neil Armstrong <redacted>
Subject: [PATCH] mailbox: arm_mhu: Add support for Amlogic Meson MHU

Is there some version of MHU specified anywhere in manuals? It seems
weird Amlogic took the IP and only rearranged the registers. Maybe the
order is specific to non-Amba version of the IP?  Lets call it that.

I think Amlogic took an early Juno platform release and re-implemented the
MHU using the same concept but following their design rules.

quoted

quoted

To achieve this integration, add support for generic probe from amba
or platform.
Move all register offsets to a data structure passed in either amba id or
platform dt id match table.

Signed-off-by: Neil Armstrong <redacted>
---
 drivers/mailbox/arm_mhu.c | 217 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 181 insertions(+), 36 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 99befa7..d7fb843 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c

@@ -22,45 +22,68 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/amba/bus.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
 #include <linux/mailbox_controller.h>

-#define INTR_STAT_OFS  0x0
-#define INTR_SET_OFS   0x8
-#define INTR_CLR_OFS   0x10
+#define MHU_INTR_STAT_OFS      0x0
+#define MHU_INTR_SET_OFS       0x8
+#define MHU_INTR_CLR_OFS       0x10

 #define MHU_LP_OFFSET  0x0
 #define MHU_HP_OFFSET  0x20
 #define MHU_SEC_OFFSET 0x200
-#define TX_REG_OFFSET  0x100
+#define MHU_TX_REG_OFFSET      0x100

-#define MHU_CHANS      3
+#define MESON_INTR_SET_OFS     0x0
+#define MESON_INTR_STAT_OFS    0x4
+#define MESON_INTR_CLR_OFS     0x8
+
+#define MESON_MHU_LP_OFFSET    0x10
+#define MESON_MHU_HP_OFFSET    0x1c
+#define MESON_MHU_TX_OFFSET    0x24
+
+#define MAX_MHU_CHANS  3

MHU_CHANS always 3 doesn't hurt. Lets keep it unchanged.

quoted

 struct mhu_link {
        unsigned irq;
-       void __iomem *tx_reg;
-       void __iomem *rx_reg;
+       void __iomem *tx_stat_reg;
+       void __iomem *tx_set_reg;
+       void __iomem *tx_clr_reg;
+       void __iomem *rx_stat_reg;
+       void __iomem *rx_set_reg;
+       void __iomem *rx_clr_reg;
 };

Yeah, this is OK.

quoted

 struct arm_mhu {
        void __iomem *base;
-       struct mhu_link mlink[MHU_CHANS];
-       struct mbox_chan chan[MHU_CHANS];
+       struct mhu_link mlink[MAX_MHU_CHANS];
+       struct mbox_chan chan[MAX_MHU_CHANS];
        struct mbox_controller mbox;
 };

just leave it MHU_CHANS

quoted

+struct arm_mhu_data {
+       unsigned int channels;
+       int rx_offsets[MAX_MHU_CHANS];
+       int tx_offsets[MAX_MHU_CHANS];
+       unsigned int intr_stat_offs;
+       unsigned int intr_set_offs;
+       unsigned int intr_clr_offs;
+};

This is unnecessary. Please remove it and code will be simpler -
assign rx/tx_regs directly in probe.

I won't assume the platform driver is only for Amlogic, it does not
make sense.

It is unlikely other platforms will come with more random register
arrangements of MHU.

quoted

quoted

+
 static irqreturn_t mhu_rx_interrupt(int irq, void *p)
 {
        struct mbox_chan *chan = p;
        struct mhu_link *mlink = chan->con_priv;
        u32 val;

-       val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
+       val = readl_relaxed(mlink->rx_stat_reg);
        if (!val)
                return IRQ_NONE;

        mbox_chan_received_data(chan, (void *)&val);

-       writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS);
+       writel_relaxed(val, mlink->rx_clr_reg);

        return IRQ_HANDLED;
 }

@@ -68,7 +91,7 @@ static irqreturn_t mhu_rx_interrupt(int irq, void *p)
 static bool mhu_last_tx_done(struct mbox_chan *chan)
 {
        struct mhu_link *mlink = chan->con_priv;
-       u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
+       u32 val = readl_relaxed(mlink->tx_stat_reg);

        return (val == 0);
 }

@@ -78,7 +101,7 @@ static int mhu_send_data(struct mbox_chan *chan, void *data)
        struct mhu_link *mlink = chan->con_priv;
        u32 *arg = data;

-       writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
+       writel_relaxed(*arg, mlink->tx_set_reg);

        return 0;
 }

@@ -89,8 +112,8 @@ static int mhu_startup(struct mbox_chan *chan)
        u32 val;
        int ret;

-       val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
-       writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
+       val = readl_relaxed(mlink->tx_stat_reg);
+       writel_relaxed(val, mlink->tx_clr_reg);

        ret = request_irq(mlink->irq, mhu_rx_interrupt,
                          IRQF_SHARED, "mhu_link", chan);

@@ -117,52 +140,155 @@ static const struct mbox_chan_ops mhu_ops = {
        .last_tx_done = mhu_last_tx_done,
 };

-static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
+static struct arm_mhu_data arm_mhu_amba_data = {
+       .channels = 3,
+       .rx_offsets = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET},
+       .tx_offsets = {MHU_LP_OFFSET + MHU_TX_REG_OFFSET,
+                      MHU_HP_OFFSET + MHU_TX_REG_OFFSET,
+                      MHU_SEC_OFFSET + MHU_TX_REG_OFFSET},
+       .intr_stat_offs = MHU_INTR_STAT_OFS,
+       .intr_set_offs = MHU_INTR_SET_OFS,
+       .intr_clr_offs = MHU_INTR_CLR_OFS,
+};
+
+static const struct arm_mhu_data meson_mhu_data = {
+       .channels = 2,
+       .rx_offsets = {MESON_MHU_LP_OFFSET, MESON_MHU_HP_OFFSET},
+       .tx_offsets = {MESON_MHU_LP_OFFSET + MESON_MHU_TX_OFFSET,
+                      MESON_MHU_HP_OFFSET + MESON_MHU_TX_OFFSET},
+       .intr_stat_offs = MESON_INTR_STAT_OFS,
+       .intr_set_offs = MESON_INTR_SET_OFS,
+       .intr_clr_offs = MESON_INTR_CLR_OFS,
+};
+

These could be directly set in amba vs platform probes ?

It does not make sense to assume the platform driver is only for
amlogic gxbb, it could match other vendors aswell.

Perhaps you didn't get my suggestion.

The amba could force a single struct, but it's smarter to use the
same mechanism and associate the struct to an ID.

quoted

Thanks.

My main question is : do you really want to transform this simple driver into
a dirty multi-bus generic mailbox driver ?
The meson_mhu is only 199 lines and this patch adds 181 lines to the arm_mhu driver.

I'll personally push to have two separate drivers here.

It is a shame if we need to copy a driver only due to changed register
offsets. Let me give it a shot and see how worse off we would be.

Thanks.