| Message ID | 20180722063923.30222-6-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add mailbox support for i.MX7D | expand |
Am Sonntag, den 22.07.2018, 08:39 +0200 schrieb Oleksij Rempel: > The Mailbox controller is able to send messages (up to 4 32 bit words) > between the endpoints. > > This driver was tested using the mailbox-test driver sending messages > between the Cortex-A7 and the Cortex-M4. > > > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/mailbox/Kconfig | 6 + > drivers/mailbox/Makefile | 2 + > drivers/mailbox/imx-mailbox.c | 273 ++++++++++++++++++++++++++++++++++ > 3 files changed, 281 insertions(+) > create mode 100644 drivers/mailbox/imx-mailbox.c > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > index a2bb27446dce..79060ddc380d 100644 > --- a/drivers/mailbox/Kconfig > +++ b/drivers/mailbox/Kconfig > @@ -15,6 +15,12 @@ config ARM_MHU > > The controller has 3 mailbox channels, the last of which can be > > used in Secure mode only. > > +config IMX_MBOX > > + tristate "i.MX Mailbox" > > + depends on ARCH_MXC || COMPILE_TEST > > + help > > + Mailbox implementation for i.MX Messaging Unit (MU). > + > config PLATFORM_MHU > > tristate "Platform MHU Mailbox" > > depends on OF > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile > index cc23c3a43fcd..ba2fe1b6dd62 100644 > --- a/drivers/mailbox/Makefile > +++ b/drivers/mailbox/Makefile > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o > > > obj-$(CONFIG_ARM_MHU) += arm_mhu.o > > > +obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o > + > > obj-$(CONFIG_PLATFORM_MHU) += platform_mhu.o > > > obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o > diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c > new file mode 100644 > index 000000000000..29cf2876db01 > --- /dev/null > +++ b/drivers/mailbox/imx-mailbox.c > @@ -0,0 +1,273 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de> > + */ > + > +#include <linux/clk.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/mailbox_controller.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > + > +/* Transmit Register */ > > +#define IMX_MU_xTRn(x) (0x00 + 4 * (x)) > +/* Receive Register */ > > +#define IMX_MU_xRRn(x) (0x10 + 4 * (x)) > +/* Status Register */ > > +#define IMX_MU_xSR 0x20 > > +#define IMX_MU_xSR_TEn(x) BIT(20 + (3 - (x))) > > +#define IMX_MU_xSR_RFn(x) BIT(24 + (3 - (x))) > > +#define IMX_MU_xSR_BRDIP BIT(9) > + > +/* Control Register */ > > +#define IMX_MU_xCR 0x24 > +/* Transmit Interrupt Enable */ > > +#define IMX_MU_xCR_TIEn(x) BIT(20 + (3 - (x))) > +/* Receive Interrupt Enable */ > > +#define IMX_MU_xCR_RIEn(x) BIT(24 + (3 - (x))) > + > > +#define IMX_MU_CHANS 4u > + > +struct imx_mu_con_priv { > > > + int irq; > > > + unsigned int idx; > > > + char *irq_desc; > +}; > + > +struct imx_mu_priv { > > > + struct device *dev; > > > + void __iomem *base; > + > > > + struct mbox_controller mbox; > > > + struct mbox_chan mbox_chans[IMX_MU_CHANS]; > + > > + struct imx_mu_con_priv con_priv[IMX_MU_CHANS]; > > > + struct clk *clk; > + > > > + bool side_b; > +}; > + > +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox) > +{ > > + return container_of(mbox, struct imx_mu_priv, mbox); > +} > + > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) > +{ > + iowrite32(val, priv->base + offs); This driver is never going to be used on a device with port based IO, so iowrite doesn't make much sense here, just use writel. Same comment applies to the below read function. Also, given that those functions are not really shortening the code in the user they may also be removed completely IMHO. > +} > + > +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) > +{ > > + return ioread32(priv->base + offs); > +} > + > +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32 clr) > +{ > > + u32 val; > + > > + val = imx_mu_read(priv, offs); > > + val &= ~clr; > > + val |= set; > > + imx_mu_write(priv, val, offs); > + > > + return val; > +} > + > +static irqreturn_t imx_mu_isr(int irq, void *p) > +{ > > + struct mbox_chan *chan = p; > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > + struct imx_mu_con_priv *cp = chan->con_priv; > > + u32 val, ctrl, dat; > + > > + ctrl = imx_mu_read(priv, IMX_MU_xCR); > > + val = imx_mu_read(priv, IMX_MU_xSR); > > + val &= IMX_MU_xSR_TEn(cp->idx) | IMX_MU_xSR_RFn(cp->idx); > > + val &= ctrl & (IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx)); > > + if (!val) > > + return IRQ_NONE; > + > > + if (val & IMX_MU_xSR_TEn(cp->idx)) { > > + imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->idx)); > > + mbox_chan_txdone(chan, 0); > > + } > + > > + if (val & IMX_MU_xSR_RFn(cp->idx)) { > > + dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx)); > > + mbox_chan_received_data(chan, (void *)&dat); > > + } > + > > + return IRQ_HANDLED; > +} > + > +static bool imx_mu_last_tx_done(struct mbox_chan *chan) > +{ > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > + struct imx_mu_con_priv *cp = chan->con_priv; > > + u32 val; > + > > + val = imx_mu_read(priv, IMX_MU_xSR); > > + /* test if transmit register is empty */ > + return val & IMX_MU_xSR_TEn(cp->idx); I guess "return imx_mu_read(priv, IMX_MU_xSR) & IMX_MU_xSR_TEn(cp->idx);" is shorter and equally well understood. > +} > + > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) > +{ > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > + struct imx_mu_con_priv *cp = chan->con_priv; > > + u32 *arg = data; > + > > + if (!imx_mu_last_tx_done(chan)) > > + return -EBUSY; > + > > + imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx)); > + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->idx), 0); In multi-channel mode this RMW cycle needs some kind of locking. As this register is also changed from the irq handler, this probably needs to be a irqsave spinlock. > + > > + return 0; > +} > + > +static int imx_mu_startup(struct mbox_chan *chan) > +{ > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > + struct imx_mu_con_priv *cp = chan->con_priv; > > + int ret; > + > > + cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]", > > + cp->idx); > > + if (!cp->irq_desc) > > + return -ENOMEM; > + > > + ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr, > + IRQF_SHARED, cp->irq_desc, chan); Using the devm_ variants of those functions doesn't make sense when the resources aren't tied to the device lifetime. As you are tearing them down manually in imx_mu_shutdown anyways, just use the raw variants of those functions. > + if (ret) { > > + dev_err(priv->dev, > > + "Unable to acquire IRQ %d\n", cp->irq); > > + return ret; > > + } > + > > + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->idx), 0); > + > > + return 0; > +} > + > +static void imx_mu_shutdown(struct mbox_chan *chan) > +{ > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > + struct imx_mu_con_priv *cp = chan->con_priv; > + > > + imx_mu_rmw(priv, IMX_MU_xCR, 0, > > + IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx)); > + > > + devm_free_irq(priv->dev, cp->irq, chan); > > + devm_kfree(priv->dev, cp->irq_desc); > +} > + > +static const struct mbox_chan_ops imx_mu_ops = { > > + .send_data = imx_mu_send_data, > > + .startup = imx_mu_startup, > > + .shutdown = imx_mu_shutdown, > +}; > + > +static void imx_mu_init_generic(struct imx_mu_priv *priv) > +{ > > + if (priv->side_b) > > + return; > + > > + /* Set default MU configuration */ > > + imx_mu_write(priv, 0, IMX_MU_xCR); > +} > + > +static int imx_mu_probe(struct platform_device *pdev) > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + struct resource *iomem; > > + struct imx_mu_priv *priv; > > + unsigned int i; > > + int irq, ret; > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > + > > + priv->dev = dev; > + > > + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + priv->base = devm_ioremap_resource(&pdev->dev, iomem); > > + if (IS_ERR(priv->base)) > > + return PTR_ERR(priv->base); > + > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) > > + return irq; > + > > + priv->clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(priv->clk)) { > > + if (PTR_ERR(priv->clk) != -ENOENT) > > + return PTR_ERR(priv->clk); > + > > + priv->clk = NULL; > > + } > + > > + ret = clk_prepare_enable(priv->clk); > > + if (ret) { > > + dev_err(dev, "Failed to enable clock\n"); > > + return ret; > > + } > + > > + for (i = 0; i < IMX_MU_CHANS; i++) { > > + struct imx_mu_con_priv *cp = &priv->con_priv[i]; > + > > + cp->idx = i; > > + cp->irq = irq; > > + priv->mbox_chans[i].con_priv = cp; > > + } > + > > + if (of_property_read_bool(np, "fsl,mu-side-b")) > + priv->side_b = true; No need for the if clause here. Just assign the return value from of_property_read_bool to priv->side_b. > + > > + priv->mbox.dev = dev; > > + priv->mbox.ops = &imx_mu_ops; > > + priv->mbox.chans = priv->mbox_chans; > > + priv->mbox.num_chans = IMX_MU_CHANS; > > + priv->mbox.txdone_irq = true; > + > > + platform_set_drvdata(pdev, priv); > + > > + imx_mu_init_generic(priv); > + > > + return mbox_controller_register(&priv->mbox); > +} > + > +static int imx_mu_remove(struct platform_device *pdev) > +{ > > + struct imx_mu_priv *priv = platform_get_drvdata(pdev); > + > > + mbox_controller_unregister(&priv->mbox); > > + clk_disable_unprepare(priv->clk); > + > > + return 0; > +} > + > +static const struct of_device_id imx_mu_dt_ids[] = { > > + { .compatible = "fsl,imx6sx-mu" }, > > + { }, > +}; > +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids); > + > +static struct platform_driver imx_mu_driver = { > > > + .probe = imx_mu_probe, > > > + .remove = imx_mu_remove, > > + .driver = { > > > + .name = "imx_mu", > > + .of_match_table = imx_mu_dt_ids, > > + }, > +}; > +module_platform_driver(imx_mu_driver); > + > > +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>"); > +MODULE_DESCRIPTION("Message Unit driver for i.MX"); > +MODULE_LICENSE("GPL v2");
On Mon, Jul 23, 2018 at 07:19:40PM +0200, Lucas Stach wrote: > Am Sonntag, den 22.07.2018, 08:39 +0200 schrieb Oleksij Rempel: > > The Mailbox controller is able to send messages (up to 4 32 bit words) > > between the endpoints. > > > > This driver was tested using the mailbox-test driver sending messages > > between the Cortex-A7 and the Cortex-M4. > > > > > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com> > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > drivers/mailbox/Kconfig | 6 + > > drivers/mailbox/Makefile | 2 + > > drivers/mailbox/imx-mailbox.c | 273 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 281 insertions(+) > > create mode 100644 drivers/mailbox/imx-mailbox.c > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > > index a2bb27446dce..79060ddc380d 100644 > > --- a/drivers/mailbox/Kconfig > > +++ b/drivers/mailbox/Kconfig > > @@ -15,6 +15,12 @@ config ARM_MHU > > > The controller has 3 mailbox channels, the last of which can be > > > used in Secure mode only. > > > > +config IMX_MBOX > > > + tristate "i.MX Mailbox" > > > + depends on ARCH_MXC || COMPILE_TEST > > > + help > > > + Mailbox implementation for i.MX Messaging Unit (MU). > > + > > config PLATFORM_MHU > > > tristate "Platform MHU Mailbox" > > > depends on OF > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile > > index cc23c3a43fcd..ba2fe1b6dd62 100644 > > --- a/drivers/mailbox/Makefile > > +++ b/drivers/mailbox/Makefile > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o > > > > > obj-$(CONFIG_ARM_MHU) += arm_mhu.o > > > > > +obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o > > + > > > obj-$(CONFIG_PLATFORM_MHU) += platform_mhu.o > > > > > obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o > > diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c > > new file mode 100644 > > index 000000000000..29cf2876db01 > > --- /dev/null > > +++ b/drivers/mailbox/imx-mailbox.c > > @@ -0,0 +1,273 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de> > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/interrupt.h> > > +#include <linux/io.h> > > +#include <linux/kernel.h> > > +#include <linux/mailbox_controller.h> > > +#include <linux/module.h> > > +#include <linux/of_device.h> > > + > > +/* Transmit Register */ > > > +#define IMX_MU_xTRn(x) (0x00 + 4 * (x)) > > +/* Receive Register */ > > > +#define IMX_MU_xRRn(x) (0x10 + 4 * (x)) > > +/* Status Register */ > > > +#define IMX_MU_xSR 0x20 > > > +#define IMX_MU_xSR_TEn(x) BIT(20 + (3 - (x))) > > > +#define IMX_MU_xSR_RFn(x) BIT(24 + (3 - (x))) > > > +#define IMX_MU_xSR_BRDIP BIT(9) > > + > > +/* Control Register */ > > > +#define IMX_MU_xCR 0x24 > > +/* Transmit Interrupt Enable */ > > > +#define IMX_MU_xCR_TIEn(x) BIT(20 + (3 - (x))) > > +/* Receive Interrupt Enable */ > > > +#define IMX_MU_xCR_RIEn(x) BIT(24 + (3 - (x))) > > + > > > +#define IMX_MU_CHANS 4u > > + > > +struct imx_mu_con_priv { > > > > + int irq; > > > > + unsigned int idx; > > > > + char *irq_desc; > > +}; > > + > > +struct imx_mu_priv { > > > > + struct device *dev; > > > > + void __iomem *base; > > + > > > > + struct mbox_controller mbox; > > > > + struct mbox_chan mbox_chans[IMX_MU_CHANS]; > > + > > > + struct imx_mu_con_priv con_priv[IMX_MU_CHANS]; > > > > + struct clk *clk; > > + > > > > + bool side_b; > > +}; > > + > > +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox) > > +{ > > > + return container_of(mbox, struct imx_mu_priv, mbox); > > +} > > + > > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) > > +{ > > + iowrite32(val, priv->base + offs); > > This driver is never going to be used on a device with port based IO, > so iowrite doesn't make much sense here, just use writel. Same comment > applies to the below read function. I read this and do not understand what is actual problem: https://lwn.net/Articles/102232/ "By default, these functions are simply wrappers around readb() and friends. The explicit pointer type for the argument will generate warnings, however, if a driver passes in an integer type." So, is it wrong or not? If yes, why? > Also, given that those functions are not really shortening the code in > the user they may also be removed completely IMHO. Is it subjective or technical issue? > > +} > > + > > +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) > > +{ > > > + return ioread32(priv->base + offs); > > +} > > + > > +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32 clr) > > +{ > > > + u32 val; > > + > > > + val = imx_mu_read(priv, offs); > > > + val &= ~clr; > > > + val |= set; > > > + imx_mu_write(priv, val, offs); > > + > > > + return val; > > +} > > + > > +static irqreturn_t imx_mu_isr(int irq, void *p) > > +{ > > > + struct mbox_chan *chan = p; > > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > > + struct imx_mu_con_priv *cp = chan->con_priv; > > > + u32 val, ctrl, dat; > > + > > > + ctrl = imx_mu_read(priv, IMX_MU_xCR); > > > + val = imx_mu_read(priv, IMX_MU_xSR); > > > + val &= IMX_MU_xSR_TEn(cp->idx) | IMX_MU_xSR_RFn(cp->idx); > > > + val &= ctrl & (IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx)); > > > + if (!val) > > > + return IRQ_NONE; > > + > > > + if (val & IMX_MU_xSR_TEn(cp->idx)) { > > > + imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->idx)); > > > + mbox_chan_txdone(chan, 0); > > > + } > > + > > > + if (val & IMX_MU_xSR_RFn(cp->idx)) { > > > + dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx)); > > > + mbox_chan_received_data(chan, (void *)&dat); > > > + } > > + > > > + return IRQ_HANDLED; > > +} > > + > > +static bool imx_mu_last_tx_done(struct mbox_chan *chan) > > +{ > > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > > + struct imx_mu_con_priv *cp = chan->con_priv; > > > + u32 val; > > + > > > + val = imx_mu_read(priv, IMX_MU_xSR); > > > + /* test if transmit register is empty */ > > + return val & IMX_MU_xSR_TEn(cp->idx); > > I guess > "return imx_mu_read(priv, IMX_MU_xSR) & IMX_MU_xSR_TEn(cp->idx);" is > shorter and equally well understood. Same as before: Is it subjective or technical issue? > > +} > > + > > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) > > +{ > > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > > + struct imx_mu_con_priv *cp = chan->con_priv; > > > + u32 *arg = data; > > + > > > + if (!imx_mu_last_tx_done(chan)) > > > + return -EBUSY; > > + > > > + imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx)); > > + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->idx), 0); > > In multi-channel mode this RMW cycle needs some kind of locking. As > this register is also changed from the irq handler, this probably needs > to be a irqsave spinlock. ok. > > + > > > + return 0; > > +} > > + > > +static int imx_mu_startup(struct mbox_chan *chan) > > +{ > > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > > + struct imx_mu_con_priv *cp = chan->con_priv; > > > + int ret; > > + > > > + cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]", > > > + cp->idx); > > > + if (!cp->irq_desc) > > > + return -ENOMEM; > > + > > > + ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr, > > + IRQF_SHARED, cp->irq_desc, chan); > > Using the devm_ variants of those functions doesn't make sense when the > resources aren't tied to the device lifetime. As you are tearing them > down manually in imx_mu_shutdown anyways, just use the raw variants of > those functions. So, it is not a problem and there is no advantage. Right? > > + if (ret) { > > > + dev_err(priv->dev, > > > + "Unable to acquire IRQ %d\n", cp->irq); > > > + return ret; > > > + } > > + > > > + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->idx), 0); > > + > > > + return 0; > > +} > > + > > +static void imx_mu_shutdown(struct mbox_chan *chan) > > +{ > > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > > + struct imx_mu_con_priv *cp = chan->con_priv; > > + > > > + imx_mu_rmw(priv, IMX_MU_xCR, 0, > > > + IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx)); > > + > > > + devm_free_irq(priv->dev, cp->irq, chan); > > > + devm_kfree(priv->dev, cp->irq_desc); > > +} > > + > > +static const struct mbox_chan_ops imx_mu_ops = { > > > + .send_data = imx_mu_send_data, > > > + .startup = imx_mu_startup, > > > + .shutdown = imx_mu_shutdown, > > +}; > > + > > +static void imx_mu_init_generic(struct imx_mu_priv *priv) > > +{ > > > + if (priv->side_b) > > > + return; > > + > > > + /* Set default MU configuration */ > > > + imx_mu_write(priv, 0, IMX_MU_xCR); > > +} > > + > > +static int imx_mu_probe(struct platform_device *pdev) > > +{ > > > + struct device *dev = &pdev->dev; > > > + struct device_node *np = dev->of_node; > > > + struct resource *iomem; > > > + struct imx_mu_priv *priv; > > > + unsigned int i; > > > + int irq, ret; > > + > > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > > + if (!priv) > > > + return -ENOMEM; > > + > > > + priv->dev = dev; > > + > > > + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + priv->base = devm_ioremap_resource(&pdev->dev, iomem); > > > + if (IS_ERR(priv->base)) > > > + return PTR_ERR(priv->base); > > + > > > + irq = platform_get_irq(pdev, 0); > > > + if (irq < 0) > > > + return irq; > > + > > > + priv->clk = devm_clk_get(dev, NULL); > > > + if (IS_ERR(priv->clk)) { > > > + if (PTR_ERR(priv->clk) != -ENOENT) > > > + return PTR_ERR(priv->clk); > > + > > > + priv->clk = NULL; > > > + } > > + > > > + ret = clk_prepare_enable(priv->clk); > > > + if (ret) { > > > + dev_err(dev, "Failed to enable clock\n"); > > > + return ret; > > > + } > > + > > > + for (i = 0; i < IMX_MU_CHANS; i++) { > > > + struct imx_mu_con_priv *cp = &priv->con_priv[i]; > > + > > > + cp->idx = i; > > > + cp->irq = irq; > > > + priv->mbox_chans[i].con_priv = cp; > > > + } > > + > > > + if (of_property_read_bool(np, "fsl,mu-side-b")) > > + priv->side_b = true; > > No need for the if clause here. Just assign the return value from > of_property_read_bool to priv->side_b. Not against kernel style and not broken. Is it subjective or technical issue? I assume I like more red color... > > + > > > + priv->mbox.dev = dev; > > > + priv->mbox.ops = &imx_mu_ops; > > > + priv->mbox.chans = priv->mbox_chans; > > > + priv->mbox.num_chans = IMX_MU_CHANS; > > > + priv->mbox.txdone_irq = true; > > + > > > + platform_set_drvdata(pdev, priv); > > + > > > + imx_mu_init_generic(priv); > > + > > > + return mbox_controller_register(&priv->mbox); > > +} > > + > > +static int imx_mu_remove(struct platform_device *pdev) > > +{ > > > + struct imx_mu_priv *priv = platform_get_drvdata(pdev); > > + > > > + mbox_controller_unregister(&priv->mbox); > > > + clk_disable_unprepare(priv->clk); > > + > > > + return 0; > > +} > > + > > +static const struct of_device_id imx_mu_dt_ids[] = { > > > + { .compatible = "fsl,imx6sx-mu" }, > > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids); > > + > > +static struct platform_driver imx_mu_driver = { > > > > + .probe = imx_mu_probe, > > > > + .remove = imx_mu_remove, > > > + .driver = { > > > > + .name = "imx_mu", > > > + .of_match_table = imx_mu_dt_ids, > > > + }, > > +}; > > +module_platform_driver(imx_mu_driver); > > + > > > +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>"); > > +MODULE_DESCRIPTION("Message Unit driver for i.MX"); > > +MODULE_LICENSE("GPL v2"); >
Hi Oleksij, On 07/22/2018 09:39 AM, Oleksij Rempel wrote: > The Mailbox controller is able to send messages (up to 4 32 bit words) > between the endpoints. > > This driver was tested using the mailbox-test driver sending messages > between the Cortex-A7 and the Cortex-M4. > > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> [snip] > +static int imx_mu_startup(struct mbox_chan *chan) > +{ > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > + struct imx_mu_con_priv *cp = chan->con_priv; > + int ret; > + > + cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]", > + cp->idx); > + if (!cp->irq_desc) > + return -ENOMEM; > + Again I would suggest to move this allocation to the loop in .probe function. [snip] > + > + for (i = 0; i < IMX_MU_CHANS; i++) { > + struct imx_mu_con_priv *cp = &priv->con_priv[i]; > + > + cp->idx = i; > + cp->irq = irq; ^^^^ right over here. > + priv->mbox_chans[i].con_priv = cp; > + } > + please feel free to add my technical side review tag to the next version: Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> FWIW I find that review comments from Lucas are pretty valid, I would recommend to incorporate them. -- Best wishes, Vladimir
> -----Original Message----- > From: Lucas Stach [mailto:l.stach@pengutronix.de] > Sent: Tuesday, July 24, 2018 1:20 AM > To: Oleksij Rempel <o.rempel@pengutronix.de>; Shawn Guo > <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; A.s. > Dong <aisheng.dong@nxp.com>; Vladimir Zapolskiy > <vladimir_zapolskiy@mentor.com> > Cc: dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org; > kernel@pengutronix.de; devicetree@vger.kernel.org > Subject: Re: [PATCH v6 5/5] mailbox: Add support for i.MX7D messaging unit > [...] > > +} > > + > > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) { > > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > > + struct imx_mu_con_priv *cp = chan->con_priv; > > > + u32 *arg = data; > > + > > > + if (!imx_mu_last_tx_done(chan)) > > > + return -EBUSY; > > + > > > + imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx)); > > + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->idx), 0); > > In multi-channel mode this RMW cycle needs some kind of locking. As this > register is also changed from the irq handler, this probably needs to be a > irqsave spinlock. > Good catch! They do need to be protected. Regards Dong Aisheng
> -----Original Message----- > From: Vladimir Zapolskiy [mailto:vz@mleia.com] > Sent: Tuesday, July 24, 2018 7:31 AM > To: Oleksij Rempel <o.rempel@pengutronix.de>; Shawn Guo > <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; A.s. > Dong <aisheng.dong@nxp.com>; Vladimir Zapolskiy > <vladimir_zapolskiy@mentor.com> > Cc: kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH v6 5/5] mailbox: Add support for i.MX7D messaging unit > > Hi Oleksij, > > On 07/22/2018 09:39 AM, Oleksij Rempel wrote: > > The Mailbox controller is able to send messages (up to 4 32 bit words) > > between the endpoints. > > > > This driver was tested using the mailbox-test driver sending messages > > between the Cortex-A7 and the Cortex-M4. > > > > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > [snip] > > > +static int imx_mu_startup(struct mbox_chan *chan) { > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > + struct imx_mu_con_priv *cp = chan->con_priv; > > + int ret; > > + > > + cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, > "imx_mu_chan[%i]", > > + cp->idx); > > + if (!cp->irq_desc) > > + return -ENOMEM; > > + > > Again I would suggest to move this allocation to the loop in .probe function. > > [snip] > +1 Good point. > > + > > + for (i = 0; i < IMX_MU_CHANS; i++) { > > + struct imx_mu_con_priv *cp = &priv->con_priv[i]; > > + > > + cp->idx = i; > > + cp->irq = irq; > > ^^^^ right over here. > > > + priv->mbox_chans[i].con_priv = cp; > > + } > > + > > please feel free to add my technical side review tag to the next version: > > Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> > > FWIW I find that review comments from Lucas are pretty valid, I would > recommend to incorporate them. +1 Regards Dong Aisheng > > -- > Best wishes, > Vladimir
On 24.07.2018 01:31, Vladimir Zapolskiy wrote: > Hi Oleksij, > > On 07/22/2018 09:39 AM, Oleksij Rempel wrote: >> The Mailbox controller is able to send messages (up to 4 32 bit words) >> between the endpoints. >> >> This driver was tested using the mailbox-test driver sending messages >> between the Cortex-A7 and the Cortex-M4. >> >> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com> >> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > [snip] > >> +static int imx_mu_startup(struct mbox_chan *chan) >> +{ >> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); >> + struct imx_mu_con_priv *cp = chan->con_priv; >> + int ret; >> + >> + cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]", >> + cp->idx); >> + if (!cp->irq_desc) >> + return -ENOMEM; >> + > > Again I would suggest to move this allocation to the loop in .probe function. Why? In my case I use 2 channels. Why should I allocate 4 descriptors for using only 2 of them? The SCU driver will use only one channel... > [snip] > >> + >> + for (i = 0; i < IMX_MU_CHANS; i++) { >> + struct imx_mu_con_priv *cp = &priv->con_priv[i]; >> + >> + cp->idx = i; >> + cp->irq = irq; > > ^^^^ right over here. > >> + priv->mbox_chans[i].con_priv = cp; >> + } >> + > > please feel free to add my technical side review tag to the next version: > > Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> > > FWIW I find that review comments from Lucas are pretty valid, I would > recommend to incorporate them. Ok
Hi Lucas, here is more detailed response: On 23.07.2018 19:19, Lucas Stach wrote: > Am Sonntag, den 22.07.2018, 08:39 +0200 schrieb Oleksij Rempel: >> The Mailbox controller is able to send messages (up to 4 32 bit words) >> between the endpoints. >> >> This driver was tested using the mailbox-test driver sending messages >> between the Cortex-A7 and the Cortex-M4. >> >>> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com> >>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> >> --- >> drivers/mailbox/Kconfig | 6 + >> drivers/mailbox/Makefile | 2 + >> drivers/mailbox/imx-mailbox.c | 273 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 281 insertions(+) >> create mode 100644 drivers/mailbox/imx-mailbox.c >> >> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig >> index a2bb27446dce..79060ddc380d 100644 >> --- a/drivers/mailbox/Kconfig >> +++ b/drivers/mailbox/Kconfig >> @@ -15,6 +15,12 @@ config ARM_MHU >>> The controller has 3 mailbox channels, the last of which can be >>> used in Secure mode only. >> >> +config IMX_MBOX >>> + tristate "i.MX Mailbox" >>> + depends on ARCH_MXC || COMPILE_TEST >>> + help >>> + Mailbox implementation for i.MX Messaging Unit (MU). >> + >> config PLATFORM_MHU >>> tristate "Platform MHU Mailbox" >>> depends on OF >> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile >> index cc23c3a43fcd..ba2fe1b6dd62 100644 >> --- a/drivers/mailbox/Makefile >> +++ b/drivers/mailbox/Makefile >>> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o >> >>> obj-$(CONFIG_ARM_MHU) += arm_mhu.o >> >>> +obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o >> + >>> obj-$(CONFIG_PLATFORM_MHU) += platform_mhu.o >> >>> obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o >> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c >> new file mode 100644 >> index 000000000000..29cf2876db01 >> --- /dev/null >> +++ b/drivers/mailbox/imx-mailbox.c >> @@ -0,0 +1,273 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de> >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/mailbox_controller.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> + >> +/* Transmit Register */ >>> +#define IMX_MU_xTRn(x) (0x00 + 4 * (x)) >> +/* Receive Register */ >>> +#define IMX_MU_xRRn(x) (0x10 + 4 * (x)) >> +/* Status Register */ >>> +#define IMX_MU_xSR 0x20 >>> +#define IMX_MU_xSR_TEn(x) BIT(20 + (3 - (x))) >>> +#define IMX_MU_xSR_RFn(x) BIT(24 + (3 - (x))) >>> +#define IMX_MU_xSR_BRDIP BIT(9) >> + >> +/* Control Register */ >>> +#define IMX_MU_xCR 0x24 >> +/* Transmit Interrupt Enable */ >>> +#define IMX_MU_xCR_TIEn(x) BIT(20 + (3 - (x))) >> +/* Receive Interrupt Enable */ >>> +#define IMX_MU_xCR_RIEn(x) BIT(24 + (3 - (x))) >> + >>> +#define IMX_MU_CHANS 4u >> + >> +struct imx_mu_con_priv { >>>> + int irq; >>>> + unsigned int idx; >>>> + char *irq_desc; >> +}; >> + >> +struct imx_mu_priv { >>>> + struct device *dev; >>>> + void __iomem *base; >> + >>>> + struct mbox_controller mbox; >>>> + struct mbox_chan mbox_chans[IMX_MU_CHANS]; >> + >>> + struct imx_mu_con_priv con_priv[IMX_MU_CHANS]; >>>> + struct clk *clk; >> + >>>> + bool side_b; >> +}; >> + >> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox) >> +{ >>> + return container_of(mbox, struct imx_mu_priv, mbox); >> +} >> + >> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) >> +{ >> + iowrite32(val, priv->base + offs); > > This driver is never going to be used on a device with port based IO, > so iowrite doesn't make much sense here, just use writel. Same comment > applies to the below read function. As before I don't really understand the difference. I took some time for learning and here are my findings: - historical background of ioread/iowrite functions. What I can find is this LWN article: https://lwn.net/Articles/102232/ . The main point seems to be "The first of these is a new __iomem annotation used to mark pointers to I/O memory" ... " As with __user, the __iomem marker serves a documentation role in the kernel code;" In modern kernel the difference between ioread32 and readl seems to be completely disappeared. with this LWN article (2016): https://lwn.net/Articles/698014/ the readl and reade32 are always in the same group.. If there is some documentation which will say why I should use readl() instead of ioread32() i would really love to read it. > Also, given that those functions are not really shortening the code in > the user they may also be removed completely IMHO. Wrong assumption.. I use this functions for tracing. It is just to easy to add two trace points... From technical perspective I don't see any advantage or disadvantage of having this functions. If it is my personal preference, then I decide to keep it. >> +} >> + >> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) >> +{ >>> + return ioread32(priv->base + offs); >> +} >> + >> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32 clr) >> +{ >>> + u32 val; >> + >>> + val = imx_mu_read(priv, offs); >>> + val &= ~clr; >>> + val |= set; >>> + imx_mu_write(priv, val, offs); >> + >>> + return val; >> +} >> + >> +static irqreturn_t imx_mu_isr(int irq, void *p) >> +{ >>> + struct mbox_chan *chan = p; >>> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); >>> + struct imx_mu_con_priv *cp = chan->con_priv; >>> + u32 val, ctrl, dat; >> + >>> + ctrl = imx_mu_read(priv, IMX_MU_xCR); >>> + val = imx_mu_read(priv, IMX_MU_xSR); >>> + val &= IMX_MU_xSR_TEn(cp->idx) | IMX_MU_xSR_RFn(cp->idx); >>> + val &= ctrl & (IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx)); >>> + if (!val) >>> + return IRQ_NONE; >> + >>> + if (val & IMX_MU_xSR_TEn(cp->idx)) { >>> + imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->idx)); >>> + mbox_chan_txdone(chan, 0); >>> + } >> + >>> + if (val & IMX_MU_xSR_RFn(cp->idx)) { >>> + dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx)); >>> + mbox_chan_received_data(chan, (void *)&dat); >>> + } >> + >>> + return IRQ_HANDLED; >> +} >> + >> +static bool imx_mu_last_tx_done(struct mbox_chan *chan) >> +{ >>> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); >>> + struct imx_mu_con_priv *cp = chan->con_priv; >>> + u32 val; >> + >>> + val = imx_mu_read(priv, IMX_MU_xSR); >>> + /* test if transmit register is empty */ >> + return val & IMX_MU_xSR_TEn(cp->idx); > > I guess > "return imx_mu_read(priv, IMX_MU_xSR) & IMX_MU_xSR_TEn(cp->idx);" is > shorter and equally well understood. So, previous comment was saying, "those functions are not really shortening the code" I would apply same comment: "those .. are not really shortening the code". Do we really need to make review of "personal coding style preferences".. which are not against kernel coding style in 6. review round...?! For example: kernel coding style is pink... in this case we are talking about difference between lavender pink and carnation pink. >> +} >> + >> +static int imx_mu_send_data(struct mbox_chan *chan, void *data) >> +{ >>> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); >>> + struct imx_mu_con_priv *cp = chan->con_priv; >>> + u32 *arg = data; >> + >>> + if (!imx_mu_last_tx_done(chan)) >>> + return -EBUSY; >> + >>> + imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx)); >> + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->idx), 0); > > In multi-channel mode this RMW cycle needs some kind of locking. As > this register is also changed from the irq handler, this probably needs > to be a irqsave spinlock. Thank you, i need to fix it. >> + >>> + return 0; >> +} >> + >> +static int imx_mu_startup(struct mbox_chan *chan) >> +{ >>> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); >>> + struct imx_mu_con_priv *cp = chan->con_priv; >>> + int ret; >> + >>> + cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]", >>> + cp->idx); >>> + if (!cp->irq_desc) >>> + return -ENOMEM; >> + >>> + ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr, >> + IRQF_SHARED, cp->irq_desc, chan); > > Using the devm_ variants of those functions doesn't make sense when the > resources aren't tied to the device lifetime. As you are tearing them > down manually in imx_mu_shutdown anyways, just use the raw variants of > those functions. I have nothing against it. >> + if (ret) { >>> + dev_err(priv->dev, >>> + "Unable to acquire IRQ %d\n", cp->irq); >>> + return ret; >>> + } >> + >>> + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->idx), 0); >> + >>> + return 0; >> +} >> + >> +static void imx_mu_shutdown(struct mbox_chan *chan) >> +{ >>> + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); >>> + struct imx_mu_con_priv *cp = chan->con_priv; >> + >>> + imx_mu_rmw(priv, IMX_MU_xCR, 0, >>> + IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx)); >> + >>> + devm_free_irq(priv->dev, cp->irq, chan); >>> + devm_kfree(priv->dev, cp->irq_desc); >> +} >> + >> +static const struct mbox_chan_ops imx_mu_ops = { >>> + .send_data = imx_mu_send_data, >>> + .startup = imx_mu_startup, >>> + .shutdown = imx_mu_shutdown, >> +}; >> + >> +static void imx_mu_init_generic(struct imx_mu_priv *priv) >> +{ >>> + if (priv->side_b) >>> + return; >> + >>> + /* Set default MU configuration */ >>> + imx_mu_write(priv, 0, IMX_MU_xCR); >> +} >> + >> +static int imx_mu_probe(struct platform_device *pdev) >> +{ >>> + struct device *dev = &pdev->dev; >>> + struct device_node *np = dev->of_node; >>> + struct resource *iomem; >>> + struct imx_mu_priv *priv; >>> + unsigned int i; >>> + int irq, ret; >> + >>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>> + if (!priv) >>> + return -ENOMEM; >> + >>> + priv->dev = dev; >> + >>> + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + priv->base = devm_ioremap_resource(&pdev->dev, iomem); >>> + if (IS_ERR(priv->base)) >>> + return PTR_ERR(priv->base); >> + >>> + irq = platform_get_irq(pdev, 0); >>> + if (irq < 0) >>> + return irq; >> + >>> + priv->clk = devm_clk_get(dev, NULL); >>> + if (IS_ERR(priv->clk)) { >>> + if (PTR_ERR(priv->clk) != -ENOENT) >>> + return PTR_ERR(priv->clk); >> + >>> + priv->clk = NULL; >>> + } >> + >>> + ret = clk_prepare_enable(priv->clk); >>> + if (ret) { >>> + dev_err(dev, "Failed to enable clock\n"); >>> + return ret; >>> + } >> + >>> + for (i = 0; i < IMX_MU_CHANS; i++) { >>> + struct imx_mu_con_priv *cp = &priv->con_priv[i]; >> + >>> + cp->idx = i; >>> + cp->irq = irq; >>> + priv->mbox_chans[i].con_priv = cp; >>> + } >> + >>> + if (of_property_read_bool(np, "fsl,mu-side-b")) >> + priv->side_b = true; > > No need for the if clause here. Just assign the return value from > of_property_read_bool to priv->side_b. ok. Thank you for a review!
Am Dienstag, den 24.07.2018, 07:14 +0200 schrieb Oleksij Rempel: > Hi Lucas, > > here is more detailed response: > > On 23.07.2018 19:19, Lucas Stach wrote: [...] > > > +}; > > > + > > > +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox) > > > +{ > > > > + return container_of(mbox, struct imx_mu_priv, mbox); > > > > > > +} > > > + > > > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) > > > +{ > > > + iowrite32(val, priv->base + offs); > > > > This driver is never going to be used on a device with port based IO, > > so iowrite doesn't make much sense here, just use writel. Same comment > > applies to the below read function. > > As before I don't really understand the difference. I took some time for > learning and here are my findings: > - historical background of ioread/iowrite functions. What I can find is > > this LWN article: https://lwn.net/Articles/102232/ . The main point > seems to be "The first of these is a new __iomem annotation used to mark > pointers to I/O memory" ... " As with __user, the __iomem marker serves > a documentation role in the kernel code;" > > In modern kernel the difference between ioread32 and readl seems to be > completely disappeared. > with this LWN article (2016): > https://lwn.net/Articles/698014/ > the readl and reade32 are always in the same group.. Okay, as discussed offline this is more a thing of personal preference, since this maps to the same thing internally when used on a architecture without IO ports. I slightly dislike those as they don't have a _relaxed counterpart, but other than that there should be no difference. As we don't use any of the relaxed stuff in this driver it's really about the color of the bikeshed, so feel free to keep it your way. > If there is some documentation which will say why I should use readl() > instead of ioread32() i would really love to read it. > > > Also, given that those functions are not really shortening the code in > > the user they may also be removed completely IMHO. > > Wrong assumption.. I use this functions for tracing. It is just to easy > to add two trace points... From technical perspective I don't see any > advantage or disadvantage of having this functions. > If it is my personal preference, then I decide to keep it. That IMHO implies that I'm perfectly fine with you ignoring this comment. :) [...] > > > +static int imx_mu_startup(struct mbox_chan *chan) > > > +{ > > > > > > > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > > > > > > > + struct imx_mu_con_priv *cp = chan->con_priv; > > > > + int ret; > > > > > > + > > > > > > > > + cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]", > > > > > > > > + cp->idx); > > > > > > > > + if (!cp->irq_desc) > > > > + return -ENOMEM; > > > > > > + > > > > + ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr, > > > > > > + IRQF_SHARED, cp->irq_desc, chan); > > > > Using the devm_ variants of those functions doesn't make sense when the > > resources aren't tied to the device lifetime. As you are tearing them > > down manually in imx_mu_shutdown anyways, just use the raw variants of > > those functions. > > I have nothing against it. Thanks, as this is something I feel more strongly about. As the devm_ stuff is meant to tie the lifetime of an object to the device/driver lifetime using them in a context where there is no relation at all is kind of an API abuse to me. Regards, Lucas
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index a2bb27446dce..79060ddc380d 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -15,6 +15,12 @@ config ARM_MHU The controller has 3 mailbox channels, the last of which can be used in Secure mode only. +config IMX_MBOX + tristate "i.MX Mailbox" + depends on ARCH_MXC || COMPILE_TEST + help + Mailbox implementation for i.MX Messaging Unit (MU). + config PLATFORM_MHU tristate "Platform MHU Mailbox" depends on OF diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index cc23c3a43fcd..ba2fe1b6dd62 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o obj-$(CONFIG_ARM_MHU) += arm_mhu.o +obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o + obj-$(CONFIG_PLATFORM_MHU) += platform_mhu.o obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c new file mode 100644 index 000000000000..29cf2876db01 --- /dev/null +++ b/drivers/mailbox/imx-mailbox.c @@ -0,0 +1,273 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de> + */ + +#include <linux/clk.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/mailbox_controller.h> +#include <linux/module.h> +#include <linux/of_device.h> + +/* Transmit Register */ +#define IMX_MU_xTRn(x) (0x00 + 4 * (x)) +/* Receive Register */ +#define IMX_MU_xRRn(x) (0x10 + 4 * (x)) +/* Status Register */ +#define IMX_MU_xSR 0x20 +#define IMX_MU_xSR_TEn(x) BIT(20 + (3 - (x))) +#define IMX_MU_xSR_RFn(x) BIT(24 + (3 - (x))) +#define IMX_MU_xSR_BRDIP BIT(9) + +/* Control Register */ +#define IMX_MU_xCR 0x24 +/* Transmit Interrupt Enable */ +#define IMX_MU_xCR_TIEn(x) BIT(20 + (3 - (x))) +/* Receive Interrupt Enable */ +#define IMX_MU_xCR_RIEn(x) BIT(24 + (3 - (x))) + +#define IMX_MU_CHANS 4u + +struct imx_mu_con_priv { + int irq; + unsigned int idx; + char *irq_desc; +}; + +struct imx_mu_priv { + struct device *dev; + void __iomem *base; + + struct mbox_controller mbox; + struct mbox_chan mbox_chans[IMX_MU_CHANS]; + + struct imx_mu_con_priv con_priv[IMX_MU_CHANS]; + struct clk *clk; + + bool side_b; +}; + +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox) +{ + return container_of(mbox, struct imx_mu_priv, mbox); +} + +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) +{ + iowrite32(val, priv->base + offs); +} + +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) +{ + return ioread32(priv->base + offs); +} + +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32 clr) +{ + u32 val; + + val = imx_mu_read(priv, offs); + val &= ~clr; + val |= set; + imx_mu_write(priv, val, offs); + + return val; +} + +static irqreturn_t imx_mu_isr(int irq, void *p) +{ + struct mbox_chan *chan = p; + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); + struct imx_mu_con_priv *cp = chan->con_priv; + u32 val, ctrl, dat; + + ctrl = imx_mu_read(priv, IMX_MU_xCR); + val = imx_mu_read(priv, IMX_MU_xSR); + val &= IMX_MU_xSR_TEn(cp->idx) | IMX_MU_xSR_RFn(cp->idx); + val &= ctrl & (IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx)); + if (!val) + return IRQ_NONE; + + if (val & IMX_MU_xSR_TEn(cp->idx)) { + imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->idx)); + mbox_chan_txdone(chan, 0); + } + + if (val & IMX_MU_xSR_RFn(cp->idx)) { + dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx)); + mbox_chan_received_data(chan, (void *)&dat); + } + + return IRQ_HANDLED; +} + +static bool imx_mu_last_tx_done(struct mbox_chan *chan) +{ + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); + struct imx_mu_con_priv *cp = chan->con_priv; + u32 val; + + val = imx_mu_read(priv, IMX_MU_xSR); + /* test if transmit register is empty */ + return val & IMX_MU_xSR_TEn(cp->idx); +} + +static int imx_mu_send_data(struct mbox_chan *chan, void *data) +{ + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); + struct imx_mu_con_priv *cp = chan->con_priv; + u32 *arg = data; + + if (!imx_mu_last_tx_done(chan)) + return -EBUSY; + + imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx)); + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->idx), 0); + + return 0; +} + +static int imx_mu_startup(struct mbox_chan *chan) +{ + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); + struct imx_mu_con_priv *cp = chan->con_priv; + int ret; + + cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]", + cp->idx); + if (!cp->irq_desc) + return -ENOMEM; + + ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr, + IRQF_SHARED, cp->irq_desc, chan); + if (ret) { + dev_err(priv->dev, + "Unable to acquire IRQ %d\n", cp->irq); + return ret; + } + + imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->idx), 0); + + return 0; +} + +static void imx_mu_shutdown(struct mbox_chan *chan) +{ + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); + struct imx_mu_con_priv *cp = chan->con_priv; + + imx_mu_rmw(priv, IMX_MU_xCR, 0, + IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx)); + + devm_free_irq(priv->dev, cp->irq, chan); + devm_kfree(priv->dev, cp->irq_desc); +} + +static const struct mbox_chan_ops imx_mu_ops = { + .send_data = imx_mu_send_data, + .startup = imx_mu_startup, + .shutdown = imx_mu_shutdown, +}; + +static void imx_mu_init_generic(struct imx_mu_priv *priv) +{ + if (priv->side_b) + return; + + /* Set default MU configuration */ + imx_mu_write(priv, 0, IMX_MU_xCR); +} + +static int imx_mu_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct resource *iomem; + struct imx_mu_priv *priv; + unsigned int i; + int irq, ret; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->dev = dev; + + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + priv->base = devm_ioremap_resource(&pdev->dev, iomem); + if (IS_ERR(priv->base)) + return PTR_ERR(priv->base); + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + priv->clk = devm_clk_get(dev, NULL); + if (IS_ERR(priv->clk)) { + if (PTR_ERR(priv->clk) != -ENOENT) + return PTR_ERR(priv->clk); + + priv->clk = NULL; + } + + ret = clk_prepare_enable(priv->clk); + if (ret) { + dev_err(dev, "Failed to enable clock\n"); + return ret; + } + + for (i = 0; i < IMX_MU_CHANS; i++) { + struct imx_mu_con_priv *cp = &priv->con_priv[i]; + + cp->idx = i; + cp->irq = irq; + priv->mbox_chans[i].con_priv = cp; + } + + if (of_property_read_bool(np, "fsl,mu-side-b")) + priv->side_b = true; + + priv->mbox.dev = dev; + priv->mbox.ops = &imx_mu_ops; + priv->mbox.chans = priv->mbox_chans; + priv->mbox.num_chans = IMX_MU_CHANS; + priv->mbox.txdone_irq = true; + + platform_set_drvdata(pdev, priv); + + imx_mu_init_generic(priv); + + return mbox_controller_register(&priv->mbox); +} + +static int imx_mu_remove(struct platform_device *pdev) +{ + struct imx_mu_priv *priv = platform_get_drvdata(pdev); + + mbox_controller_unregister(&priv->mbox); + clk_disable_unprepare(priv->clk); + + return 0; +} + +static const struct of_device_id imx_mu_dt_ids[] = { + { .compatible = "fsl,imx6sx-mu" }, + { }, +}; +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids); + +static struct platform_driver imx_mu_driver = { + .probe = imx_mu_probe, + .remove = imx_mu_remove, + .driver = { + .name = "imx_mu", + .of_match_table = imx_mu_dt_ids, + }, +}; +module_platform_driver(imx_mu_driver); + +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>"); +MODULE_DESCRIPTION("Message Unit driver for i.MX"); +MODULE_LICENSE("GPL v2");