Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

From: Jassi Brar
Date: Sat Jun 25 2016 - 13:51:04 EST

-#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.

[snip]

> -------------------------------------------------------------------------------------------
> From: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> 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.


> 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 <narmstrong@xxxxxxxxxxxx>
> ---
> 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.

> 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.


>
> 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

>
> +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.


> +
> 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 ?

Thanks.