On 06/25/2016 07:50 PM, Jassi Brar wrote: > -#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. > [snip] > >> ------------------------------------------------------------------------------------------- >> From: Neil Armstrong <[email protected]> >> 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. > >> 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 <[email protected]> >> --- >> 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. I won't assume the platform driver is only for Amlogic, it does not make sense. > >> + >> 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. The amba could force a single struct, but it's smarter to use the same mechanism and associate the struct to an ID. > 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. Thanks, Neil
